Skip to content

Commit e7e6123

Browse files
spencer-lunargspencer-lunarg
committed
layers: Improve vkCmdSetDescriptorBufferOffsetsEXT
1 parent 8e63101 commit e7e6123

11 files changed

Lines changed: 260 additions & 141 deletions

‎layers/core_checks/cc_descriptor.cpp‎

Lines changed: 77 additions & 97 deletions
Original file line numberDiff line numberDiff line change
@@ -2325,16 +2325,12 @@ bool CoreChecks::ValidateCmdSetDescriptorBufferOffsets(const vvl::CommandBuffer
23252325
skip |= ValidateCmd(cb_state, loc);
23262326

23272327
auto pipeline_layout = Get<vvl::PipelineLayout>(layout);
2328-
if (!pipeline_layout) return skip; // dynamicPipelineLayout
2328+
if (!pipeline_layout) {
2329+
return skip; // dynamicPipelineLayout
2330+
}
23292331

23302332
const bool is_2 = loc.function != Func::vkCmdSetDescriptorBufferOffsetsEXT;
23312333

2332-
if (!enabled_features.descriptorBuffer) {
2333-
const char *vuid = is_2 ? "VUID-vkCmdSetDescriptorBufferOffsets2EXT-descriptorBuffer-09470"
2334-
: "VUID-vkCmdSetDescriptorBufferOffsetsEXT-None-08060";
2335-
skip |= LogError(vuid, cb_state.Handle(), loc, "descriptorBuffer feature was not enabled.");
2336-
}
2337-
23382334
if ((firstSet + setCount) > pipeline_layout->set_layouts.size()) {
23392335
const char *vuid = is_2 ? "VUID-VkSetDescriptorBufferOffsetsInfoEXT-firstSet-08066"
23402336
: "VUID-vkCmdSetDescriptorBufferOffsetsEXT-firstSet-08066";
@@ -2347,6 +2343,17 @@ bool CoreChecks::ValidateCmdSetDescriptorBufferOffsets(const vvl::CommandBuffer
23472343
setCount = std::min(setCount, static_cast<uint32_t>(pipeline_layout->set_layouts.size()));
23482344
}
23492345

2346+
if (cb_state.descriptor_buffer_binding_info.empty()) {
2347+
const char *vuid = is_2 ? "VUID-VkSetDescriptorBufferOffsetsInfoEXT-pBufferIndices-08065"
2348+
: "VUID-vkCmdSetDescriptorBufferOffsetsEXT-pBufferIndices-08065";
2349+
const LogObjectList objlist(cb_state.Handle(), pipeline_layout->Handle());
2350+
skip |= LogError(vuid, objlist, loc,
2351+
"There have been no calls to vkCmdBindDescriptorBuffersEXT and no descriptor buffers are bound. Any "
2352+
"future call will vkCmdBindDescriptorBuffersEXT would invalidate these offsets anyway.");
2353+
// everything else won't make sense
2354+
return skip;
2355+
}
2356+
23502357
for (uint32_t i = 0; i < setCount; i++) {
23512358
const auto set_layout = pipeline_layout->set_layouts[firstSet + i];
23522359
if ((set_layout->GetCreateFlags() & VK_DESCRIPTOR_SET_LAYOUT_CREATE_DESCRIPTOR_BUFFER_BIT_EXT) == 0) {
@@ -2360,107 +2367,80 @@ bool CoreChecks::ValidateCmdSetDescriptorBufferOffsets(const vvl::CommandBuffer
23602367
continue;
23612368
}
23622369

2363-
bool valid_buffer = false;
2364-
const VkDeviceAddress offset = pOffsets[i];
23652370
const uint32_t buffer_index = pBufferIndices[i];
2366-
if (buffer_index < cb_state.descriptor_buffer_binding_info.size()) {
2367-
bool valid_binding = false;
2368-
const VkDeviceAddress start = cb_state.descriptor_buffer_binding_info[buffer_index].address;
2369-
const auto buffer_states = GetBuffersByAddress(start);
2370-
2371-
if (!buffer_states.empty()) {
2372-
const auto buffer_state_starts = GetBuffersByAddress(start + offset);
2373-
2374-
if (!buffer_state_starts.empty()) {
2375-
const auto bindings = set_layout->GetBindings();
2376-
2377-
VkDeviceSize set_layout_size = 0;
2378-
if (VkDeviceSize cached_set_layout_size = set_layout->GetLayoutSizeInBytes(); cached_set_layout_size == 0) {
2379-
DispatchGetDescriptorSetLayoutSizeEXT(cb_state.dev_data.device, set_layout->VkHandle(), &set_layout_size);
2380-
auto set_layout_ptr = const_cast<vvl::DescriptorSetLayout *>(set_layout.get());
2381-
set_layout_ptr->SetLayoutSizeInBytes(&set_layout_size);
2382-
} else {
2383-
set_layout_size = cached_set_layout_size;
2384-
}
2371+
if (buffer_index >= cb_state.descriptor_buffer_binding_info.size()) {
2372+
const char *vuid = is_2 ? "VUID-VkSetDescriptorBufferOffsetsInfoEXT-pBufferIndices-08065"
2373+
: "VUID-vkCmdSetDescriptorBufferOffsetsEXT-pBufferIndices-08065";
2374+
const LogObjectList objlist(cb_state.Handle(), set_layout->Handle(), pipeline_layout->Handle());
2375+
skip |= LogError(vuid, objlist, loc.dot(Field::pBufferIndices, i),
2376+
"is %" PRIu32 " but the command buffer only has had %zu bound descriptor buffers", pBufferIndices[i],
2377+
cb_state.descriptor_buffer_binding_info.size());
2378+
continue; // the buffer is not valid
2379+
}
23852380

2386-
if (set_layout_size > 0) {
2387-
// It looks like enough to check last binding in set
2388-
for (uint32_t j = 0; j < set_layout->GetBindingCount(); j++) {
2389-
const VkDescriptorBindingFlags flags = set_layout->GetDescriptorBindingFlagsFromIndex(j);
2390-
const bool vdc = (flags & VK_DESCRIPTOR_BINDING_VARIABLE_DESCRIPTOR_COUNT_BIT) != 0;
2391-
2392-
if (vdc) {
2393-
// If a binding is VARIABLE_DESCRIPTOR_COUNT, the effective setLayoutSize we
2394-
// must validate is just the offset of the last binding.
2395-
const auto pool = cb_state.command_pool;
2396-
uint32_t binding = set_layout->GetDescriptorSetLayoutBindingPtrFromIndex(j)->binding;
2397-
DispatchGetDescriptorSetLayoutBindingOffsetEXT(pool->dev_data.device, set_layout->VkHandle(),
2398-
binding, &set_layout_size);
2399-
2400-
// If the descriptor set only consists of VARIABLE_DESCRIPTOR_COUNT bindings, the
2401-
// offset may be 0. In this case, treat the descriptor set layout as size 1,
2402-
// so we validate that the offset is sensible.
2403-
if (set_layout->GetBindingCount() == 1) {
2404-
set_layout_size = 1;
2405-
}
2381+
const VkDescriptorBufferBindingInfoEXT &binding_info = cb_state.descriptor_buffer_binding_info[buffer_index];
2382+
const VkDeviceAddress start = binding_info.address;
2383+
const auto buffer_states = GetBuffersByAddress(start);
24062384

2407-
// There can only be one binding with VARIABLE_COUNT.
2408-
break;
2409-
}
2410-
}
2411-
}
2385+
if (buffer_states.empty()) {
2386+
const char *vuid = is_2 ? "VUID-VkSetDescriptorBufferOffsetsInfoEXT-pBufferIndices-08065"
2387+
: "VUID-vkCmdSetDescriptorBufferOffsetsEXT-pBufferIndices-08065";
2388+
const LogObjectList objlist(cb_state.Handle(), set_layout->Handle(), pipeline_layout->Handle());
2389+
skip |=
2390+
LogError(vuid, objlist, loc.dot(Field::pBufferIndices, i),
2391+
"(%" PRIu32 ") points to descriptor buffer at VkDescriptorBufferBindingInfoEXT::address (0x%" PRIxLEAST64
2392+
") but no VkBuffer was found in this address",
2393+
buffer_index, start);
2394+
continue; // the buffer is not valid
2395+
}
24122396

2413-
if (set_layout_size > 0) {
2414-
const auto buffer_state_ends = GetBuffersByAddress(start + offset + set_layout_size - 1);
2415-
if (!buffer_state_ends.empty()) {
2416-
valid_binding = true;
2417-
}
2397+
const VkDeviceAddress offset = pOffsets[i];
2398+
if (offset == 0) {
2399+
continue; // is by definition small enough as it is at the start
2400+
}
2401+
2402+
bool valid_binding = false;
2403+
VkDeviceSize set_layout_size = set_layout->GetLayoutSizeInBytes();
2404+
const auto buffer_state_starts = GetBuffersByAddress(start + offset);
2405+
if (!buffer_state_starts.empty()) {
2406+
const auto bindings = set_layout->GetBindings();
2407+
2408+
if (set_layout_size > 0) {
2409+
// Variable Descriptor Count can only be in the highest binding (the last binding)
2410+
const uint32_t last_index = set_layout->GetLastIndex();
2411+
const VkDescriptorBindingFlags flags = set_layout->GetDescriptorBindingFlagsFromIndex(last_index);
2412+
if (flags & VK_DESCRIPTOR_BINDING_VARIABLE_DESCRIPTOR_COUNT_BIT) {
2413+
// If the descriptor set only consists of VARIABLE_DESCRIPTOR_COUNT bindings, the offset may be 0. In
2414+
// this case, treat the descriptor set layout as size 1, so we validate that the offset is sensible.
2415+
if (set_layout->GetBindingCount() == 1) {
2416+
set_layout_size = 1;
2417+
} else {
2418+
// If a binding is VARIABLE_DESCRIPTOR_COUNT, the effective setLayoutSize we must validate is just
2419+
// the offset of the last binding.
2420+
const uint32_t binding = set_layout->GetDescriptorSetLayoutBindingPtrFromIndex(last_index)->binding;
2421+
DispatchGetDescriptorSetLayoutBindingOffsetEXT(device, set_layout->VkHandle(), binding, &set_layout_size);
24182422
}
24192423
}
2420-
2421-
valid_buffer = true;
24222424
}
24232425

2424-
if (!valid_binding) {
2425-
const char *vuid = is_2 ? "VUID-VkSetDescriptorBufferOffsetsInfoEXT-pOffsets-08063"
2426-
: "VUID-vkCmdSetDescriptorBufferOffsetsEXT-pOffsets-08063";
2427-
skip |= LogError(vuid, cb_state.Handle(), loc.dot(Field::pOffsets, i),
2428-
"%" PRIuLEAST64
2429-
" must be small enough such that any descriptor binding"
2430-
" referenced by layout without the VK_DESCRIPTOR_BINDING_VARIABLE_DESCRIPTOR_COUNT_BIT"
2431-
" flag computes a valid address inside the underlying VkBuffer",
2432-
pOffsets[i]);
2426+
if (set_layout_size > 0) {
2427+
const auto buffer_state_ends = GetBuffersByAddress(start + offset + set_layout_size - 1);
2428+
if (!buffer_state_ends.empty()) {
2429+
valid_binding = true;
2430+
}
24332431
}
24342432
}
24352433

2436-
if (!valid_buffer) {
2437-
const char *vuid = is_2 ? "VUID-VkSetDescriptorBufferOffsetsInfoEXT-pBufferIndices-08065"
2438-
: "VUID-vkCmdSetDescriptorBufferOffsetsEXT-pBufferIndices-08065";
2439-
skip |= LogError(vuid, cb_state.Handle(), loc.dot(Field::pBufferIndices, i),
2440-
"(%" PRIu32
2441-
") Each element of pBufferIndices must reference a valid descriptor buffer binding "
2442-
"set by a previous call to vkCmdBindDescriptorBuffersEXT in commandBuffer",
2443-
pBufferIndices[i]);
2444-
}
2445-
2446-
if (pBufferIndices[i] >= phys_dev_ext_props.descriptor_buffer_props.maxDescriptorBufferBindings) {
2447-
const char *vuid = is_2 ? "VUID-VkSetDescriptorBufferOffsetsInfoEXT-pBufferIndices-08064"
2448-
: "VUID-vkCmdSetDescriptorBufferOffsetsEXT-pBufferIndices-08064";
2449-
skip |= LogError(vuid, cb_state.Handle(), loc.dot(Field::pBufferIndices, i),
2450-
"(%" PRIu32
2451-
") "
2452-
"is greater than maxDescriptorBufferBindings (%" PRIu32 ") ",
2453-
pBufferIndices[i], phys_dev_ext_props.descriptor_buffer_props.maxDescriptorBufferBindings);
2454-
}
2455-
2456-
if (SafeModulo(offset, phys_dev_ext_props.descriptor_buffer_props.descriptorBufferOffsetAlignment) != 0) {
2457-
const char *vuid = is_2 ? "VUID-VkSetDescriptorBufferOffsetsInfoEXT-pOffsets-08061"
2458-
: "VUID-vkCmdSetDescriptorBufferOffsetsEXT-pOffsets-08061";
2459-
skip |= LogError(vuid, cb_state.Handle(), loc.dot(Field::pOffsets, i),
2460-
"(%" PRIuLEAST64
2461-
") is not aligned to descriptorBufferOffsetAlignment"
2462-
" (%" PRIuLEAST64 ")",
2463-
offset, phys_dev_ext_props.descriptor_buffer_props.descriptorBufferOffsetAlignment);
2434+
if (!valid_binding) {
2435+
const char *vuid = is_2 ? "VUID-VkSetDescriptorBufferOffsetsInfoEXT-pOffsets-08063"
2436+
: "VUID-vkCmdSetDescriptorBufferOffsetsEXT-pOffsets-08063";
2437+
const LogObjectList objlist(cb_state.Handle(), set_layout->Handle(), pipeline_layout->Handle());
2438+
skip |=
2439+
LogError(vuid, objlist, loc.dot(Field::pBufferIndices, i),
2440+
"(%" PRIu32 ") points to descriptor buffer at VkDescriptorBufferBindingInfoEXT::address (0x%" PRIxLEAST64
2441+
") and the pOffsets[%" PRIu32 "] (%" PRIu64 ") with a VkDescriptorSetLayout size %" PRIu64
2442+
" is not within any VkBuffer range",
2443+
buffer_index, start, i, offset, set_layout_size);
24642444
}
24652445
}
24662446

‎layers/state_tracker/descriptor_sets.cpp‎

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -466,22 +466,40 @@ const std::vector<VkDescriptorType> &vvl::DescriptorSetLayoutDef::GetMutableType
466466
return mutable_types_[binding];
467467
}
468468

469-
void vvl::DescriptorSetLayout::SetLayoutSizeInBytes(const VkDeviceSize *layout_size_in_bytes) {
470-
layout_size_in_bytes_ = layout_size_in_bytes ? *layout_size_in_bytes : 0;
469+
std::string vvl::DescriptorSetLayoutDef::DescribeDescriptorBufferSizeAndOffests(VkDevice device,
470+
VkDescriptorSetLayout layout) const {
471+
std::ostringstream ss;
472+
if (flags_ & VK_DESCRIPTOR_SET_LAYOUT_CREATE_DESCRIPTOR_BUFFER_BIT_EXT) {
473+
VkDeviceSize size;
474+
DispatchGetDescriptorSetLayoutSizeEXT(device, layout, &size);
475+
ss << "layout total size = " << size << '\n';
476+
for (auto binding : bindings_) {
477+
DispatchGetDescriptorSetLayoutBindingOffsetEXT(device, layout, binding.binding, &size);
478+
ss << " binding " << binding.binding << " offset = " << size << " (" << string_VkDescriptorType(binding.descriptorType)
479+
<< ", descriptorCount = " << binding.descriptorCount;
480+
if (binding.pImmutableSamplers) {
481+
ss << ", embedded sampler";
482+
}
483+
ss << ")\n";
484+
}
485+
}
486+
return ss.str();
471487
}
472488

473-
VkDeviceSize vvl::DescriptorSetLayout::GetLayoutSizeInBytes() const { return layout_size_in_bytes_; }
474-
475489
// If our layout is compatible with rh_ds_layout, return true.
476490
bool vvl::DescriptorSetLayout::IsCompatible(DescriptorSetLayout const *rh_ds_layout) const {
477491
return (this == rh_ds_layout) || (GetLayoutDef() == rh_ds_layout->GetLayoutDef());
478492
}
479493

480494
// The DescriptorSetLayout stores the per handle data for a descriptor set layout, and references the common defintion for the
481495
// handle invariant portion
482-
vvl::DescriptorSetLayout::DescriptorSetLayout(const VkDescriptorSetLayoutCreateInfo *pCreateInfo,
496+
vvl::DescriptorSetLayout::DescriptorSetLayout(VkDevice device, const VkDescriptorSetLayoutCreateInfo *pCreateInfo,
483497
const VkDescriptorSetLayout handle)
484-
: StateObject(handle, kVulkanObjectTypeDescriptorSetLayout), layout_id_(GetCanonicalId(pCreateInfo)) {}
498+
: StateObject(handle, kVulkanObjectTypeDescriptorSetLayout), layout_id_(GetCanonicalId(pCreateInfo)) {
499+
if (pCreateInfo->flags & VK_DESCRIPTOR_SET_LAYOUT_CREATE_DESCRIPTOR_BUFFER_BIT_EXT) {
500+
DispatchGetDescriptorSetLayoutSizeEXT(device, handle, &layout_size_in_bytes_);
501+
}
502+
}
485503

486504
vvl::DescriptorSet::DescriptorSet(const VkDescriptorSet handle, vvl::DescriptorPool *pool_state,
487505
const std::shared_ptr<DescriptorSetLayout const> &layout, uint32_t variable_count,

‎layers/state_tracker/descriptor_sets.h‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,8 @@ class DescriptorSetLayoutDef {
233233

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

236+
std::string DescribeDescriptorBufferSizeAndOffests(VkDevice device, VkDescriptorSetLayout layout) const;
237+
236238
private:
237239
// Only the first three data members are used for hash and equality checks, the other members are derived from them, and are
238240
// used to speed up the various lookups/queries/validations
@@ -311,7 +313,7 @@ static inline bool operator==(const DescriptorSetLayoutDef &lhs, const Descripto
311313
class DescriptorSetLayout : public StateObject {
312314
public:
313315
// Constructors and destructor
314-
DescriptorSetLayout(const VkDescriptorSetLayoutCreateInfo *pCreateInfo, const VkDescriptorSetLayout handle);
316+
DescriptorSetLayout(VkDevice device, const VkDescriptorSetLayoutCreateInfo *pCreateInfo, const VkDescriptorSetLayout handle);
315317
virtual ~DescriptorSetLayout() { Destroy(); }
316318

317319
bool HasBinding(const uint32_t binding) const { return layout_id_->HasBinding(binding); }
@@ -371,15 +373,18 @@ class DescriptorSetLayout : public StateObject {
371373
uint32_t GetNextValidBinding(const uint32_t binding) const { return layout_id_->GetNextValidBinding(binding); }
372374
bool IsPushDescriptor() const { return layout_id_->IsPushDescriptor(); }
373375

374-
void SetLayoutSizeInBytes(const VkDeviceSize *layout_size_in_bytes_);
375-
VkDeviceSize GetLayoutSizeInBytes() const;
376+
VkDeviceSize GetLayoutSizeInBytes() const { return layout_size_in_bytes_; }
376377

377378
using BindingTypeStats = DescriptorSetLayoutDef::BindingTypeStats;
378379
const BindingTypeStats &GetBindingTypeStats() const { return layout_id_->GetBindingTypeStats(); }
379380

381+
std::string DescribeDescriptorBufferSizeAndOffests(VkDevice device) const {
382+
return layout_id_->DescribeDescriptorBufferSizeAndOffests(device, VkHandle());
383+
}
384+
380385
private:
381386
DescriptorSetLayoutId layout_id_{};
382-
VkDeviceSize layout_size_in_bytes_{};
387+
VkDeviceSize layout_size_in_bytes_ = 0;
383388
};
384389

385390
// Slightly broader than type, each c++ "class" will has a corresponding "DescriptorClass"

‎layers/state_tracker/state_tracker.cpp‎

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2089,14 +2089,7 @@ void DeviceState::PostCallRecordCreateDescriptorSetLayout(VkDevice device, const
20892089
if (record_obj.result != VK_SUCCESS) {
20902090
return;
20912091
}
2092-
Add(std::make_shared<DescriptorSetLayout>(pCreateInfo, *pSetLayout));
2093-
}
2094-
2095-
void DeviceState::PostCallRecordGetDescriptorSetLayoutSizeEXT(VkDevice device, VkDescriptorSetLayout layout,
2096-
VkDeviceSize *pLayoutSizeInBytes, const RecordObject &record_obj) {
2097-
if (auto descriptor_set_layout = Get<DescriptorSetLayout>(layout)) {
2098-
descriptor_set_layout->SetLayoutSizeInBytes(pLayoutSizeInBytes);
2099-
}
2092+
Add(std::make_shared<DescriptorSetLayout>(device, pCreateInfo, *pSetLayout));
21002093
}
21012094

21022095
void DeviceState::PostCallRecordCreatePipelineLayout(VkDevice device, const VkPipelineLayoutCreateInfo *pCreateInfo,

‎layers/state_tracker/state_tracker.h‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1795,8 +1795,6 @@ class DeviceState : public vvl::base::Device {
17951795
void PostCallRecordQueueSubmit2(VkQueue queue, uint32_t submitCount, const VkSubmitInfo2* pSubmits, VkFence fence,
17961796
const RecordObject& record_obj) override;
17971797

1798-
void PostCallRecordGetDescriptorSetLayoutSizeEXT(VkDevice device, VkDescriptorSetLayout layout,
1799-
VkDeviceSize* pLayoutSizeInBytes, const RecordObject& record_obj) override;
18001798
void PostCallRecordCmdSetDescriptorBufferOffsetsEXT(VkCommandBuffer commandBuffer, VkPipelineBindPoint pipelineBindPoint,
18011799
VkPipelineLayout layout, uint32_t firstSet, uint32_t setCount,
18021800
const uint32_t* pBufferIndices, const VkDeviceSize* pOffsets,

0 commit comments

Comments
 (0)