Skip to content

Commit

Permalink
layers: Improve VU 08600
Browse files Browse the repository at this point in the history
  • Loading branch information
spencer-lunarg committed Jun 19, 2024
1 parent 346dc33 commit 80e8232
Show file tree
Hide file tree
Showing 11 changed files with 342 additions and 187 deletions.
314 changes: 153 additions & 161 deletions layers/core_checks/cc_drawdispatch.cpp

Large diffs are not rendered by default.

6 changes: 4 additions & 2 deletions layers/core_checks/core_validation.h
Original file line number Diff line number Diff line change
Expand Up @@ -528,8 +528,10 @@ class CoreChecks : public ValidationStateTracker {
bool ValidateDrawShaderObjectPushConstantAndLayout(const LastBound& last_bound_state, const vvl::DrawDispatchVuid& vuid) const;
bool ValidateDrawShaderObjectMesh(const LastBound& last_bound_state, const vvl::DrawDispatchVuid& vuid) const;
bool ValidateActionState(const vvl::CommandBuffer& cb_state, const VkPipelineBindPoint bind_point, const Location& loc) const;
bool ValidateActionStateDescriptors(const LastBound& last_bound_state, const VkPipelineBindPoint bind_point,
const vvl::Pipeline* pipeline, const vvl::DrawDispatchVuid& vuid) const;
bool ValidateActionStateDescriptorsPipeline(const LastBound& last_bound_state, const VkPipelineBindPoint bind_point,
const vvl::Pipeline& pipeline, const vvl::DrawDispatchVuid& vuid) const;
bool ValidateActionStateDescriptorsShaderObject(const LastBound& last_bound_state, const VkPipelineBindPoint bind_point,
const vvl::DrawDispatchVuid& vuid) const;
bool ValidateActionStatePushConstant(const LastBound& last_bound_state, const vvl::Pipeline* pipeline,
const vvl::DrawDispatchVuid& vuid) const;
bool ValidateActionStateProtectedMemory(const LastBound& last_bound_state, const VkPipelineBindPoint bind_point,
Expand Down
2 changes: 1 addition & 1 deletion layers/state_tracker/cmd_buffer_state.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1155,7 +1155,7 @@ void CommandBuffer::PushDescriptorSetState(VkPipelineBindPoint pipelineBindPoint
auto &last_bound = lastBound[lv_bind_point];
auto &push_descriptor_set = last_bound.push_descriptor_set;
// If we are disturbing the current push_desriptor_set clear it
if (!push_descriptor_set || !IsBoundSetCompatible(set, last_bound, pipeline_layout)) {
if (!push_descriptor_set || !last_bound.IsBoundSetCompatible(set, pipeline_layout)) {
last_bound.UnbindAndResetPushDescriptorSet(dev_data.CreateDescriptorSet(VK_NULL_HANDLE, nullptr, dsl, 0));
}

Expand Down
63 changes: 63 additions & 0 deletions layers/state_tracker/descriptor_sets.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,69 @@ bool operator==(const DescriptorSetLayoutDef &lhs, const DescriptorSetLayoutDef
return true;
}

std::string DescriptorSetLayoutDef::DescribeDifference(uint32_t index, const DescriptorSetLayoutDef &other) const {
std::ostringstream ss;
ss << "Set " << index << " ";
auto lhs_binding_flags = GetBindingFlags();
auto rhs_binding_flags = other.GetBindingFlags();
const auto &lhs_bindings = GetBindings();
const auto &rhs_bindings = other.GetBindings();

if (GetCreateFlags() != other.GetCreateFlags()) {
ss << "VkDescriptorSetLayoutCreateFlags " << string_VkDescriptorSetLayoutCreateFlags(GetCreateFlags()) << " doesn't match "
<< string_VkDescriptorSetLayoutCreateFlags(other.GetCreateFlags());
} else if (lhs_binding_flags.size() != rhs_binding_flags.size()) {
ss << "VkDescriptorSetLayoutBindingFlagsCreateInfo::bindingCount " << lhs_binding_flags.size() << " doesn't match "
<< rhs_binding_flags.size();
} else if (lhs_binding_flags != rhs_binding_flags) {
ss << "VkDescriptorSetLayoutBindingFlagsCreateInfo::pBindingFlags (";
for (auto flag : lhs_binding_flags) {
ss << string_VkDescriptorBindingFlags(flag) << " ";
}
ss << ") doesn't match (";
for (auto flag : rhs_binding_flags) {
ss << string_VkDescriptorBindingFlags(flag) << " ";
}
ss << ")";
} else if (GetMutableTypes() != other.GetMutableTypes()) {
// TODO - this is a 2d array, need a smarter way to print out details
ss << "Mutable types doesn't match";
} else if (lhs_bindings.size() != rhs_bindings.size()) {
ss << "binding count " << lhs_bindings.size() << " doesn't match " << rhs_bindings.size();
} else {
for (size_t i = 0; i < lhs_bindings.size(); i++) {
const auto &l = lhs_bindings[i];
const auto &r = rhs_bindings[i];
if (l.descriptorType != r.descriptorType) {
ss << "binding " << i << " descriptorType " << string_VkDescriptorType(l.descriptorType) << " doesn't match "
<< string_VkDescriptorType(r.descriptorType);
break;
} else if (l.descriptorCount != r.descriptorCount) {
ss << "binding " << i << " descriptorCount " << l.descriptorCount << " doesn't match " << r.descriptorCount;
break;
} else if (l.stageFlags != r.stageFlags) {
ss << "binding " << i << " stageFlags " << string_VkShaderStageFlags(l.stageFlags) << " doesn't match "
<< string_VkShaderStageFlags(r.stageFlags);
break;
} else if (l.pImmutableSamplers != r.pImmutableSamplers) {
ss << "binding " << i << " pImmutableSamplers " << l.pImmutableSamplers << " doesn't match "
<< r.pImmutableSamplers;
break;
} else if (l.pImmutableSamplers) {
for (uint32_t s = 0; s < l.descriptorCount; s++) {
if (l.pImmutableSamplers[s] != r.pImmutableSamplers[s]) {
ss << "binding " << i << " pImmutableSamplers[" << s << "] " << l.pImmutableSamplers[s] << " doesn't match "
<< r.pImmutableSamplers[s];
break;
}
}
}
}
}
ss << "\n";
return ss.str();
}

// Construct DescriptorSetLayout instance from given create info
// Proactively reserve and resize as possible, as the reallocation was visible in profiling
vvl::DescriptorSetLayoutDef::DescriptorSetLayoutDef(const VkDescriptorSetLayoutCreateInfo *p_create_info)
Expand Down
2 changes: 2 additions & 0 deletions layers/state_tracker/descriptor_sets.h
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,8 @@ class DescriptorSetLayoutDef {
};
const BindingTypeStats &GetBindingTypeStats() const { return binding_type_stats_; }

std::string DescribeDifference(uint32_t index, const DescriptorSetLayoutDef &other) const;

private:
// Only the first three data members are used for hash and equality checks, the other members are derived from them, and are
// used to speed up the various lookups/queries/validations
Expand Down
18 changes: 18 additions & 0 deletions layers/state_tracker/pipeline_layout_state.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,24 @@ bool PipelineLayoutCompatDef::operator==(const PipelineLayoutCompatDef &other) c
return true;
}

std::string PipelineLayoutCompatDef::DescribeDifference(const PipelineLayoutCompatDef &other) const {
std::ostringstream ss;
if (set != other.set) {
ss << "The set " << set << " is different from the non-compatible pipeline layout (" << other.set << ")\n";
} else if (push_constant_ranges != other.push_constant_ranges) {
ss << "The set push constant ranages is different from the non-compatible pipeline layout push constant ranges\n";
} else {
const auto &descriptor_set_layouts = *set_layouts_id.get();
const auto &other_ds_layouts = *other.set_layouts_id.get();
for (uint32_t i = 0; i <= set; i++) {
if (descriptor_set_layouts[i] != other_ds_layouts[i]) {
return descriptor_set_layouts[i]->DescribeDifference(i, *other_ds_layouts[i]);
}
}
}
return ss.str();
}

static PipelineLayoutCompatId GetCanonicalId(const uint32_t set_index, const PushConstantRangesId &pcr_id,
const PipelineLayoutSetLayoutsId &set_layouts_id) {
return pipeline_layout_compat_dict.LookUp(PipelineLayoutCompatDef(set_index, pcr_id, set_layouts_id));
Expand Down
2 changes: 2 additions & 0 deletions layers/state_tracker/pipeline_layout_state.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,9 @@ struct PipelineLayoutCompatDef {
PipelineLayoutCompatDef(const uint32_t set_index, const PushConstantRangesId pcr_id, const PipelineLayoutSetLayoutsId sl_id)
: set(set_index), push_constant_ranges(pcr_id), set_layouts_id(sl_id) {}
size_t hash() const;

bool operator==(const PipelineLayoutCompatDef &other) const;
std::string DescribeDifference(const PipelineLayoutCompatDef &other) const;
};

// Canonical dictionary for PipelineLayoutCompat records
Expand Down
40 changes: 40 additions & 0 deletions layers/state_tracker/pipeline_state.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1094,3 +1094,43 @@ bool LastBound::IsAnyGraphicsShaderBound() const {
IsValidShaderBound(ShaderObjectStage::TASK) ||
IsValidShaderBound(ShaderObjectStage::MESH);
}

bool LastBound::IsBoundSetCompatible(uint32_t set, const vvl::PipelineLayout &pipeline_layout) const {
if ((set >= per_set.size()) || (set >= pipeline_layout.set_compat_ids.size())) {
return false;
}
return (*(per_set[set].compat_id_for_set) == *(pipeline_layout.set_compat_ids[set]));
}

bool LastBound::IsBoundSetCompatible(uint32_t set, const vvl::ShaderObject &shader_object_state) const {
if ((set >= per_set.size()) || (set >= shader_object_state.set_compat_ids.size())) {
return false;
}
return (*(per_set[set].compat_id_for_set) == *(shader_object_state.set_compat_ids[set]));
};

std::string LastBound::DescribeNonCompatibleSet(uint32_t set, const vvl::PipelineLayout &pipeline_layout) const {
std::ostringstream ss;
if (set >= per_set.size()) {
ss << "The set (" << set << ") is out of bounds for the number of sets bound (" << per_set.size() << ")\n";
} else if (set >= pipeline_layout.set_compat_ids.size()) {
ss << "The set (" << set << ") is out of bounds for the number of sets in the non-compatible VkPipelineLayout ("
<< pipeline_layout.set_compat_ids.size() << ")\n";
} else {
return per_set[set].compat_id_for_set->DescribeDifference(*(pipeline_layout.set_compat_ids[set]));
}
return ss.str();
}

std::string LastBound::DescribeNonCompatibleSet(uint32_t set, const vvl::ShaderObject &shader_object_state) const {
std::ostringstream ss;
if (set >= per_set.size()) {
ss << "The set (" << set << ") is out of bounds for the number of sets bound (" << per_set.size() << ")\n";
} else if (set >= shader_object_state.set_compat_ids.size()) {
ss << "The set (" << set << ") is out of bounds for the number of sets in the non-compatible VkDescriptorSetLayout ("
<< shader_object_state.set_compat_ids.size() << ")\n";
} else {
return per_set[set].compat_id_for_set->DescribeDifference(*(shader_object_state.set_compat_ids[set]));
}
return ss.str();
}
12 changes: 5 additions & 7 deletions layers/state_tracker/pipeline_state.h
Original file line number Diff line number Diff line change
Expand Up @@ -697,14 +697,12 @@ struct LastBound {
bool IsValidShaderOrNullBound(ShaderObjectStage stage) const;
std::vector<vvl::ShaderObject *> GetAllBoundGraphicsShaders();
bool IsAnyGraphicsShaderBound() const;
};

static inline bool IsBoundSetCompatible(uint32_t set, const LastBound &last_bound, const vvl::PipelineLayout &pipeline_layout) {
if ((set >= last_bound.per_set.size()) || (set >= pipeline_layout.set_compat_ids.size())) {
return false;
}
return (*(last_bound.per_set[set].compat_id_for_set) == *(pipeline_layout.set_compat_ids[set]));
}
bool IsBoundSetCompatible(uint32_t set, const vvl::PipelineLayout &pipeline_layout) const;
bool IsBoundSetCompatible(uint32_t set, const vvl::ShaderObject &shader_object_state) const;
std::string DescribeNonCompatibleSet(uint32_t set, const vvl::PipelineLayout &pipeline_layout) const;
std::string DescribeNonCompatibleSet(uint32_t set, const vvl::ShaderObject &shader_object_state) const;
};

static inline bool IsPipelineLayoutSetCompat(uint32_t set, const vvl::PipelineLayout *a, const vvl::PipelineLayout *b) {
if (!a || !b) {
Expand Down
51 changes: 51 additions & 0 deletions tests/unit/descriptors.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1658,6 +1658,57 @@ TEST_F(NegativeDescriptors, DescriptorSetCompatibility) {
m_commandBuffer->end();
}

TEST_F(NegativeDescriptors, DescriptorSetCompatibilityCompute) {
RETURN_IF_SKIP(Init());

vkt::Buffer storage_buffer(*m_device, 32, VK_BUFFER_USAGE_STORAGE_BUFFER_BIT);
vkt::Buffer uniform_buffer(*m_device, 32, VK_BUFFER_USAGE_UNIFORM_BUFFER_BIT);
OneOffDescriptorSet descriptor_set_storage(m_device,
{
{0, VK_DESCRIPTOR_TYPE_STORAGE_BUFFER, 1, VK_SHADER_STAGE_ALL, nullptr},
});
OneOffDescriptorSet descriptor_set_uniform(m_device,
{
{0, VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER, 1, VK_SHADER_STAGE_ALL, nullptr},
});
descriptor_set_storage.WriteDescriptorBufferInfo(0, storage_buffer.handle(), 0, VK_WHOLE_SIZE,
VK_DESCRIPTOR_TYPE_STORAGE_BUFFER);
descriptor_set_storage.UpdateDescriptorSets();
descriptor_set_uniform.WriteDescriptorBufferInfo(0, uniform_buffer.handle(), 0, VK_WHOLE_SIZE,
VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER);
descriptor_set_uniform.UpdateDescriptorSets();

const vkt::PipelineLayout pipeline_layout_a(*m_device, {&descriptor_set_storage.layout_, &descriptor_set_storage.layout_});
const vkt::PipelineLayout pipeline_layout_b(*m_device, {&descriptor_set_storage.layout_, &descriptor_set_uniform.layout_});

char const *cs_source = R"glsl(
#version 450
layout(set = 1, binding = 0) buffer StorageBuffer_1 {
uint a;
uint b;
};
void main() {
a = b;
}
)glsl";

CreateComputePipelineHelper pipeline(*this);
pipeline.cs_ = std::make_unique<VkShaderObj>(this, cs_source, VK_SHADER_STAGE_COMPUTE_BIT);
pipeline.cp_ci_.layout = pipeline_layout_a.handle();
pipeline.CreateComputePipeline();

m_commandBuffer->begin();
vk::CmdBindDescriptorSets(m_commandBuffer->handle(), VK_PIPELINE_BIND_POINT_COMPUTE, pipeline_layout_a.handle(), 0, 1,
&descriptor_set_storage.set_, 0, nullptr);
vk::CmdBindDescriptorSets(m_commandBuffer->handle(), VK_PIPELINE_BIND_POINT_COMPUTE, pipeline_layout_b.handle(), 1, 1,
&descriptor_set_uniform.set_, 0, nullptr);
vk::CmdBindPipeline(m_commandBuffer->handle(), VK_PIPELINE_BIND_POINT_COMPUTE, pipeline.Handle());
m_errorMonitor->SetDesiredError("VUID-vkCmdDispatch-None-08600");
vk::CmdDispatch(m_commandBuffer->handle(), 1, 1, 1);
m_errorMonitor->VerifyFound();
m_commandBuffer->end();
}

TEST_F(NegativeDescriptors, DSUsageBits) {
TEST_DESCRIPTION("Attempt to update descriptor sets for images and buffers that do not have correct usage bits sets.");

Expand Down
19 changes: 3 additions & 16 deletions tests/unit/multiview.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -371,23 +371,10 @@ TEST_F(NegativeMultiview, UnboundResourcesAfterBeginRenderPassAndNextSubpass) {

// Descriptor sets
{
vkt::Buffer buffer(*m_device, 8, VK_BUFFER_USAGE_UNIFORM_BUFFER_BIT);
OneOffDescriptorSet descriptor_set{m_device, {{0, VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER, 1, VK_SHADER_STAGE_ALL, nullptr}}};

VkBufferCreateInfo bci = vku::InitStructHelper();
bci.usage = VK_BUFFER_USAGE_UNIFORM_BUFFER_BIT;
bci.size = 8;
vkt::Buffer buffer(*m_device, bci);
VkDescriptorBufferInfo buffer_info;
buffer_info.buffer = buffer.handle();
buffer_info.offset = 0;
buffer_info.range = VK_WHOLE_SIZE;
VkWriteDescriptorSet descriptor_write = vku::InitStructHelper();
descriptor_write.dstSet = descriptor_set.set_;
descriptor_write.dstBinding = 0;
descriptor_write.descriptorCount = 1;
descriptor_write.descriptorType = VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER;
descriptor_write.pBufferInfo = &buffer_info;
vk::UpdateDescriptorSets(device(), 1, &descriptor_write, 0, NULL);
descriptor_set.WriteDescriptorBufferInfo(0, buffer.handle(), 0, VK_WHOLE_SIZE);
descriptor_set.UpdateDescriptorSets();

VkPipelineLayoutCreateInfo pipeline_layout_info = vku::InitStructHelper();
pipeline_layout_info.setLayoutCount = 1;
Expand Down

0 comments on commit 80e8232

Please sign in to comment.