From 30ecf66cca03148be95f782dd2b269d382feaef9 Mon Sep 17 00:00:00 2001 From: baldurk Date: Fri, 28 Aug 2020 18:50:06 +0100 Subject: [PATCH] Fix handling of variable descriptor counts to not allocate maximum size * We previously ignored the variable descriptor size allowed by descriptor indexing, --- renderdoc/driver/vulkan/vk_common.h | 2 + renderdoc/driver/vulkan/vk_core.h | 8 +- renderdoc/driver/vulkan/vk_debug.cpp | 29 +- renderdoc/driver/vulkan/vk_info.cpp | 21 +- renderdoc/driver/vulkan/vk_info.h | 19 +- renderdoc/driver/vulkan/vk_initstate.cpp | 3 + renderdoc/driver/vulkan/vk_replay.cpp | 13 +- renderdoc/driver/vulkan/vk_shaderdebug.cpp | 37 +- .../driver/vulkan/wrappers/vk_cmd_funcs.cpp | 3 +- .../vulkan/wrappers/vk_descriptor_funcs.cpp | 69 +++- util/test/demos/CMakeLists.txt | 1 + util/test/demos/demos.vcxproj | 1 + util/test/demos/demos.vcxproj.filters | 3 + .../demos/vk/vk_descriptor_variable_count.cpp | 368 ++++++++++++++++++ .../Vulkan/VK_Descriptor_Variable_Count.py | 13 + 15 files changed, 545 insertions(+), 45 deletions(-) create mode 100644 util/test/demos/vk/vk_descriptor_variable_count.cpp create mode 100644 util/test/tests/Vulkan/VK_Descriptor_Variable_Count.py diff --git a/renderdoc/driver/vulkan/vk_common.h b/renderdoc/driver/vulkan/vk_common.h index 29412a883..877e0d3aa 100644 --- a/renderdoc/driver/vulkan/vk_common.h +++ b/renderdoc/driver/vulkan/vk_common.h @@ -458,12 +458,14 @@ struct BindingStorage ~BindingStorage() { clear(); } bytebuf inlineBytes; rdcarray binds; + uint32_t variableDescriptorCount; void clear() { inlineBytes.clear(); binds.clear(); elems.clear(); + variableDescriptorCount = 0; } void reset() diff --git a/renderdoc/driver/vulkan/vk_core.h b/renderdoc/driver/vulkan/vk_core.h index 92842a4bb..6c3767360 100644 --- a/renderdoc/driver/vulkan/vk_core.h +++ b/renderdoc/driver/vulkan/vk_core.h @@ -1040,13 +1040,9 @@ public: { return m_DescriptorSetState[descSet].layout; } - const rdcarray &GetCurrentDescSetBindings(ResourceId descSet) + const BindingStorage &GetCurrentDescSetBindingStorage(ResourceId descSet) { - return m_DescriptorSetState[descSet].data.binds; - } - const bytebuf &GetCurrentDescSetInlineData(ResourceId descSet) - { - return m_DescriptorSetState[descSet].data.inlineBytes; + return m_DescriptorSetState[descSet].data; } uint32_t GetReadbackMemoryIndex(uint32_t resourceCompatibleBitmask); diff --git a/renderdoc/driver/vulkan/vk_debug.cpp b/renderdoc/driver/vulkan/vk_debug.cpp index 703ef2e57..b43f1e90d 100644 --- a/renderdoc/driver/vulkan/vk_debug.cpp +++ b/renderdoc/driver/vulkan/vk_debug.cpp @@ -2461,6 +2461,9 @@ void VulkanReplay::PatchReservedDescriptors(const VulkanStatePipeline &pipe, creationInfo.m_DescSetLayout[creationInfo.m_PipelineLayout[pipe.descSets[i].pipeLayout] .descSetLayouts[i]]; + WrappedVulkan::DescriptorSetInfo &setInfo = + m_pDriver->m_DescriptorSetState[pipe.descSets[i].descSet]; + for(size_t b = 0; !error && b < origLayout.bindings.size(); b++) { const DescSetLayout::Binding &bind = origLayout.bindings[b]; @@ -2469,22 +2472,27 @@ void VulkanReplay::PatchReservedDescriptors(const VulkanStatePipeline &pipe, if(bind.descriptorType == VK_DESCRIPTOR_TYPE_MAX_ENUM) continue; + uint32_t descriptorCount = bind.descriptorCount; + + if(bind.variableSize) + descriptorCount = setInfo.data.variableDescriptorCount; + // make room in the pool if(bind.descriptorType == VK_DESCRIPTOR_TYPE_INLINE_UNIFORM_BLOCK_EXT) { - poolSizes[InlinePoolIndex].descriptorCount += bind.descriptorCount; + poolSizes[InlinePoolIndex].descriptorCount += descriptorCount; inlineCreateInfo.maxInlineUniformBlockBindings++; } else { - poolSizes[bind.descriptorType].descriptorCount += bind.descriptorCount; + poolSizes[bind.descriptorType].descriptorCount += descriptorCount; } VkDescriptorSetLayoutBinding newBind; // offset the binding. We offset all sets to make it easier for patching - don't need to // conditionally patch shader bindings depending on which set they're in. newBind.binding = uint32_t(b + newBindingsCount); - newBind.descriptorCount = bind.descriptorCount; + newBind.descriptorCount = descriptorCount; newBind.descriptorType = bind.descriptorType; // we only need it available for compute, just make all bindings visible otherwise dynamic @@ -2500,8 +2508,6 @@ void VulkanReplay::PatchReservedDescriptors(const VulkanStatePipeline &pipe, else newBind.stageFlags = bind.stageFlags; - uint32_t descriptorCount = bind.descriptorCount; - switch(bind.descriptorType) { case VK_DESCRIPTOR_TYPE_SAMPLER: @@ -2665,11 +2671,16 @@ void VulkanReplay::PatchReservedDescriptors(const VulkanStatePipeline &pipe, if(bind.descriptorType == VK_DESCRIPTOR_TYPE_MAX_ENUM) continue; + uint32_t descriptorCount = bind.descriptorCount; + + if(bind.variableSize) + descriptorCount = setInfo.data.variableDescriptorCount; + DescriptorSetSlot *slot = setInfo.data.binds[b]; write.dstBinding = uint32_t(b + newBindingsCount); write.dstArrayElement = 0; - write.descriptorCount = bind.descriptorCount; + write.descriptorCount = descriptorCount; write.descriptorType = bind.descriptorType; switch(write.descriptorType) @@ -2728,7 +2739,7 @@ void VulkanReplay::PatchReservedDescriptors(const VulkanStatePipeline &pipe, VkWriteDescriptorSetInlineUniformBlockEXT *inlineWrite = allocInlineWrites.back(); inlineWrite->sType = VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET_INLINE_UNIFORM_BLOCK_EXT; inlineWrite->pNext = NULL; - inlineWrite->dataSize = bind.descriptorCount; + inlineWrite->dataSize = descriptorCount; inlineWrite->pData = setInfo.data.inlineBytes.data() + slot[0].inlineOffset; write.pNext = inlineWrite; break; @@ -2740,7 +2751,7 @@ void VulkanReplay::PatchReservedDescriptors(const VulkanStatePipeline &pipe, // different if(write.descriptorType == VK_DESCRIPTOR_TYPE_INLINE_UNIFORM_BLOCK_EXT) { - write.descriptorCount = bind.descriptorCount; + write.descriptorCount = descriptorCount; descWrites.push_back(write); continue; } @@ -2748,7 +2759,7 @@ void VulkanReplay::PatchReservedDescriptors(const VulkanStatePipeline &pipe, // start with no descriptors write.descriptorCount = 0; - for(uint32_t w = 0; w < bind.descriptorCount; w++) + for(uint32_t w = 0; w < descriptorCount; w++) { // if this write is valid, we increment the descriptor count and continue if(IsValid(m_pDriver->NULLDescriptorsAllowed(), write, w - write.dstArrayElement)) diff --git a/renderdoc/driver/vulkan/vk_info.cpp b/renderdoc/driver/vulkan/vk_info.cpp index 5e4bfaadc..4466d192f 100644 --- a/renderdoc/driver/vulkan/vk_info.cpp +++ b/renderdoc/driver/vulkan/vk_info.cpp @@ -119,6 +119,10 @@ void DescSetLayout::Init(VulkanResourceManager *resourceMan, VulkanCreationInfo flags = pCreateInfo->flags; + VkDescriptorSetLayoutBindingFlagsCreateInfo *bindingFlags = + (VkDescriptorSetLayoutBindingFlagsCreateInfo *)FindNextStruct( + pCreateInfo, VK_STRUCTURE_TYPE_DESCRIPTOR_SET_LAYOUT_BINDING_FLAGS_CREATE_INFO); + // descriptor set layouts can be sparse, such that only three bindings exist // but they are at 0, 5 and 10. // We assume here that while the layouts may be sparse that's mostly to allow @@ -157,6 +161,12 @@ void DescSetLayout::Init(VulkanResourceManager *resourceMan, VulkanCreationInfo for(uint32_t s = 0; s < bindings[b].descriptorCount; s++) bindings[b].immutableSampler[s] = GetResID(pCreateInfo->pBindings[i].pImmutableSamplers[s]); } + + if(bindingFlags && + (bindingFlags->pBindingFlags[i] & VK_DESCRIPTOR_BINDING_VARIABLE_DESCRIPTOR_COUNT_BIT)) + bindings[b].variableSize = 1; + else + bindings[b].variableSize = 0; } // assign offsets in sorted bindings order, as the bindings we were provided by the application @@ -167,6 +177,11 @@ void DescSetLayout::Init(VulkanResourceManager *resourceMan, VulkanCreationInfo { bindings[b].elemOffset = elemOffset; + // don't count the descriptors in the variable size array. We'll add on the allocated size after + // this + if(bindings[b].variableSize) + break; + if(bindings[b].descriptorType == VK_DESCRIPTOR_TYPE_INLINE_UNIFORM_BLOCK_EXT) { elemOffset++; @@ -180,11 +195,13 @@ void DescSetLayout::Init(VulkanResourceManager *resourceMan, VulkanCreationInfo totalElems = elemOffset; } -void DescSetLayout::CreateBindingsArray(BindingStorage &bindingStorage) const +void DescSetLayout::CreateBindingsArray(BindingStorage &bindingStorage, uint32_t variableAllocSize) const { + bindingStorage.variableDescriptorCount = variableAllocSize; + if(!bindings.empty()) { - bindingStorage.elems.resize(totalElems); + bindingStorage.elems.resize(totalElems + variableAllocSize); bindingStorage.binds.resize(bindings.size()); if(inlineByteSize == 0) diff --git a/renderdoc/driver/vulkan/vk_info.h b/renderdoc/driver/vulkan/vk_info.h index 6fcd563eb..8fcf234da 100644 --- a/renderdoc/driver/vulkan/vk_info.h +++ b/renderdoc/driver/vulkan/vk_info.h @@ -72,7 +72,7 @@ struct DescSetLayout void Init(VulkanResourceManager *resourceMan, VulkanCreationInfo &info, const VkDescriptorSetLayoutCreateInfo *pCreateInfo); - void CreateBindingsArray(BindingStorage &bindingStorage) const; + void CreateBindingsArray(BindingStorage &bindingStorage, uint32_t variableAllocSize) const; void UpdateBindingsArray(const DescSetLayout &prevLayout, BindingStorage &bindingStorage) const; struct Binding @@ -85,15 +85,28 @@ struct DescSetLayout elemOffset(0), descriptorCount(0), stageFlags(0), + variableSize(0), immutableSampler(NULL) { } + // move the immutable sampler + Binding(Binding &&b) + : descriptorType(b.descriptorType), + elemOffset(b.elemOffset), + descriptorCount(b.descriptorCount), + stageFlags(b.stageFlags), + variableSize(b.variableSize), + immutableSampler(b.immutableSampler) + { + b.immutableSampler = NULL; + } // Copy the immutable sampler Binding(const Binding &b) : descriptorType(b.descriptorType), elemOffset(b.elemOffset), descriptorCount(b.descriptorCount), stageFlags(b.stageFlags), + variableSize(b.variableSize), immutableSampler(NULL) { if(b.immutableSampler) @@ -111,6 +124,7 @@ struct DescSetLayout elemOffset = b.elemOffset; descriptorCount = b.descriptorCount; stageFlags = b.stageFlags; + variableSize = b.variableSize; SAFE_DELETE_ARRAY(immutableSampler); if(b.immutableSampler) { @@ -123,7 +137,8 @@ struct DescSetLayout VkDescriptorType descriptorType; uint32_t elemOffset; uint32_t descriptorCount; - VkShaderStageFlags stageFlags; + VkShaderStageFlags stageFlags : 31; + uint32_t variableSize : 1; ResourceId *immutableSampler; }; rdcarray bindings; diff --git a/renderdoc/driver/vulkan/vk_initstate.cpp b/renderdoc/driver/vulkan/vk_initstate.cpp index 16989782f..5861e48ad 100644 --- a/renderdoc/driver/vulkan/vk_initstate.cpp +++ b/renderdoc/driver/vulkan/vk_initstate.cpp @@ -659,6 +659,9 @@ bool WrappedVulkan::Serialise_InitialState(SerialiserType &ser, ResourceId id, V { uint32_t descriptorCount = layout.bindings[j].descriptorCount; + if(layout.bindings[j].variableSize) + descriptorCount = m_DescriptorSetState[liveid].data.variableDescriptorCount; + if(descriptorCount == 0) continue; diff --git a/renderdoc/driver/vulkan/vk_replay.cpp b/renderdoc/driver/vulkan/vk_replay.cpp index c70c9d8a9..d7f52efcc 100644 --- a/renderdoc/driver/vulkan/vk_replay.cpp +++ b/renderdoc/driver/vulkan/vk_replay.cpp @@ -1614,7 +1614,13 @@ void VulkanReplay::SavePipelineState(uint32_t eventId) bool dynamicOffset = false; - dst.bindings[b].descriptorCount = layoutBind.descriptorCount; + uint32_t descriptorCount = layoutBind.descriptorCount; + + if(layoutBind.variableSize) + descriptorCount = m_pDriver->m_DescriptorSetState[src].data.variableDescriptorCount; + + dst.bindings[b].descriptorCount = descriptorCount; + dst.bindings[b].stageFlags = (ShaderStageMask)layoutBind.stageFlags; switch(layoutBind.descriptorType) { @@ -1851,7 +1857,7 @@ void VulkanReplay::SavePipelineState(uint32_t eventId) dst.bindings[b].binds[a].resourceResourceId = ResourceId(); dst.bindings[b].binds[a].inlineBlock = true; dst.bindings[b].binds[a].byteOffset = 0; - dst.bindings[b].binds[a].byteSize = layoutBind.descriptorCount; + dst.bindings[b].binds[a].byteSize = descriptorCount; } else if(layoutBind.descriptorType == VK_DESCRIPTOR_TYPE_STORAGE_BUFFER || layoutBind.descriptorType == VK_DESCRIPTOR_TYPE_STORAGE_BUFFER_DYNAMIC || @@ -1998,7 +2004,8 @@ void VulkanReplay::FillCBufferVariables(ResourceId pipeline, ResourceId shader, bytebuf inlineData; inlineData.assign( setData.data.inlineBytes.data() + setData.data.binds[bind.bind]->inlineOffset, - layoutBind.descriptorCount); + layoutBind.variableSize ? setData.data.variableDescriptorCount + : layoutBind.descriptorCount); StandardFillCBufferVariables(refl.resourceId, c.variables, outvars, inlineData); return; } diff --git a/renderdoc/driver/vulkan/vk_shaderdebug.cpp b/renderdoc/driver/vulkan/vk_shaderdebug.cpp index ae70fe169..4e059aaba 100644 --- a/renderdoc/driver/vulkan/vk_shaderdebug.cpp +++ b/renderdoc/driver/vulkan/vk_shaderdebug.cpp @@ -189,9 +189,10 @@ public: DescSetSnapshot &dstSet = m_DescSets[set]; - const rdcarray &curBinds = - m_pDriver->GetCurrentDescSetBindings(descSets[set].descSet); - const bytebuf &curInline = m_pDriver->GetCurrentDescSetInlineData(descSets[set].descSet); + const BindingStorage &bindStorage = + m_pDriver->GetCurrentDescSetBindingStorage(descSets[set].descSet); + const rdcarray &curBinds = bindStorage.binds; + const bytebuf &curInline = bindStorage.inlineBytes; // use the descriptor set layout from when it was bound. If the pipeline layout declared a // descriptor set layout for this set, but it's statically unused, it may be complete @@ -207,7 +208,12 @@ public: { const DescSetLayout::Binding &bindLayout = setLayout.bindings[bind]; - if(bindLayout.descriptorCount == 0) + uint32_t descriptorCount = bindLayout.descriptorCount; + + if(bindLayout.variableSize) + descriptorCount = bindStorage.variableDescriptorCount; + + if(descriptorCount == 0) continue; if(bindLayout.stageFlags & stage) @@ -226,8 +232,8 @@ public: case VK_DESCRIPTOR_TYPE_STORAGE_IMAGE: case VK_DESCRIPTOR_TYPE_INPUT_ATTACHMENT: { - dstBind.imageInfos.resize(bindLayout.descriptorCount); - for(uint32_t i = 0; i < bindLayout.descriptorCount; i++) + dstBind.imageInfos.resize(descriptorCount); + for(uint32_t i = 0; i < descriptorCount; i++) { dstBind.imageInfos[i].imageLayout = curSlots[i].imageInfo.imageLayout; dstBind.imageInfos[i].imageView = @@ -243,8 +249,8 @@ public: case VK_DESCRIPTOR_TYPE_UNIFORM_TEXEL_BUFFER: case VK_DESCRIPTOR_TYPE_STORAGE_TEXEL_BUFFER: { - dstBind.texelBuffers.resize(bindLayout.descriptorCount); - for(uint32_t i = 0; i < bindLayout.descriptorCount; i++) + dstBind.texelBuffers.resize(descriptorCount); + for(uint32_t i = 0; i < descriptorCount; i++) { dstBind.texelBuffers[i] = m_pDriver->GetResourceManager()->GetCurrentHandle( @@ -255,8 +261,8 @@ public: case VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER: case VK_DESCRIPTOR_TYPE_STORAGE_BUFFER: { - dstBind.buffers.resize(bindLayout.descriptorCount); - for(uint32_t i = 0; i < bindLayout.descriptorCount; i++) + dstBind.buffers.resize(descriptorCount); + for(uint32_t i = 0; i < descriptorCount; i++) { dstBind.buffers[i].offset = curSlots[i].bufferInfo.offset; dstBind.buffers[i].range = curSlots[i].bufferInfo.range; @@ -273,15 +279,14 @@ public: idx.bindset = (int32_t)set; idx.bind = (int32_t)bind; idx.arrayIndex = 0; - bufferCache[idx].assign(curInline.data() + curSlots->inlineOffset, - bindLayout.descriptorCount); + bufferCache[idx].assign(curInline.data() + curSlots->inlineOffset, descriptorCount); break; } case VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER_DYNAMIC: case VK_DESCRIPTOR_TYPE_STORAGE_BUFFER_DYNAMIC: { - dstBind.buffers.resize(bindLayout.descriptorCount); - for(uint32_t i = 0; i < bindLayout.descriptorCount; i++) + dstBind.buffers.resize(descriptorCount); + for(uint32_t i = 0; i < descriptorCount; i++) { dstBind.buffers[i].offset = curSlots[i].bufferInfo.offset; dstBind.buffers[i].range = curSlots[i].bufferInfo.range; @@ -303,9 +308,7 @@ public: switch(bindLayout.descriptorType) { case VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER_DYNAMIC: - case VK_DESCRIPTOR_TYPE_STORAGE_BUFFER_DYNAMIC: - dynamicOffset += bindLayout.descriptorCount; - break; + case VK_DESCRIPTOR_TYPE_STORAGE_BUFFER_DYNAMIC: dynamicOffset += descriptorCount; break; default: break; } } diff --git a/renderdoc/driver/vulkan/wrappers/vk_cmd_funcs.cpp b/renderdoc/driver/vulkan/wrappers/vk_cmd_funcs.cpp index c89402e8e..1ff0edade 100644 --- a/renderdoc/driver/vulkan/wrappers/vk_cmd_funcs.cpp +++ b/renderdoc/driver/vulkan/wrappers/vk_cmd_funcs.cpp @@ -4406,7 +4406,8 @@ void WrappedVulkan::ApplyPushDescriptorWrites(VkPipelineBindPoint pipelineBindPo if(prevLayout == ResourceId()) { - desclayout.CreateBindingsArray(m_DescriptorSetState[setId].data); + // push descriptors can't have variable count, so just pass 0 + desclayout.CreateBindingsArray(m_DescriptorSetState[setId].data, 0); } else if(prevLayout != descSetLayouts[set]) { diff --git a/renderdoc/driver/vulkan/wrappers/vk_descriptor_funcs.cpp b/renderdoc/driver/vulkan/wrappers/vk_descriptor_funcs.cpp index 6551e1d76..3b0b29a1a 100644 --- a/renderdoc/driver/vulkan/wrappers/vk_descriptor_funcs.cpp +++ b/renderdoc/driver/vulkan/wrappers/vk_descriptor_funcs.cpp @@ -457,7 +457,29 @@ bool WrappedVulkan::Serialise_vkAllocateDescriptorSets(SerialiserType &ser, VkDe // this is stored in the resource record on capture, we need to be able to look to up m_DescriptorSetState[live].layout = layoutId; - m_CreationInfo.m_DescSetLayout[layoutId].CreateBindingsArray(m_DescriptorSetState[live].data); + + // If descriptorSetCount is zero or this structure is not included in the pNext chain, + // then the variable lengths are considered to be zero. + uint32_t variableDescriptorAlloc = 0; + + if(!m_CreationInfo.m_DescSetLayout[layoutId].bindings.empty() && + m_CreationInfo.m_DescSetLayout[layoutId].bindings.back().variableSize) + { + VkDescriptorSetVariableDescriptorCountAllocateInfo *variableAlloc = + (VkDescriptorSetVariableDescriptorCountAllocateInfo *)FindNextStruct( + &AllocateInfo, + VK_STRUCTURE_TYPE_DESCRIPTOR_SET_VARIABLE_DESCRIPTOR_COUNT_ALLOCATE_INFO); + + if(variableAlloc) + { + // this struct will have been patched similar to VkDescriptorSetAllocateInfo so we look up + // the [0]th element + variableDescriptorAlloc = variableAlloc->pDescriptorCounts[0]; + } + } + + m_CreationInfo.m_DescSetLayout[layoutId].CreateBindingsArray(m_DescriptorSetState[live].data, + variableDescriptorAlloc); } AddResource(DescriptorSet, ResourceType::ShaderBinding, "Descriptor Set"); @@ -481,6 +503,22 @@ VkResult WrappedVulkan::vkAllocateDescriptorSets(VkDevice device, if(ret != VK_SUCCESS) return ret; + VkDescriptorSetVariableDescriptorCountAllocateInfo *variableAlloc = + (VkDescriptorSetVariableDescriptorCountAllocateInfo *)FindNextStruct( + pAllocateInfo, VK_STRUCTURE_TYPE_DESCRIPTOR_SET_VARIABLE_DESCRIPTOR_COUNT_ALLOCATE_INFO); + + VkDescriptorSetAllocateInfo mutableInfo = *pAllocateInfo; + + { + byte *tempMem = GetTempMemory(GetNextPatchSize(mutableInfo.pNext)); + CopyNextChainForPatching("VkDescriptorSetAllocateInfo", tempMem, + (VkBaseInStructure *)&mutableInfo); + } + + VkDescriptorSetVariableDescriptorCountAllocateInfo *mutableVariableInfo = + (VkDescriptorSetVariableDescriptorCountAllocateInfo *)FindNextStruct( + &mutableInfo, VK_STRUCTURE_TYPE_DESCRIPTOR_SET_VARIABLE_DESCRIPTOR_COUNT_ALLOCATE_INFO); + for(uint32_t i = 0; i < pAllocateInfo->descriptorSetCount; i++) { VkResourceRecord *poolrecord = NULL; @@ -489,12 +527,19 @@ VkResult WrappedVulkan::vkAllocateDescriptorSets(VkDevice device, ResourceId id; VkResourceRecord *record = NULL; bool exactReuse = false; + uint32_t variableDescriptorAlloc = 0; if(IsCaptureMode(m_State)) { layoutRecord = GetRecord(pAllocateInfo->pSetLayouts[i]); poolrecord = GetRecord(pAllocateInfo->descriptorPool); + if(!layoutRecord->descInfo->layout->bindings.empty() && + layoutRecord->descInfo->layout->bindings.back().variableSize && variableAlloc) + { + variableDescriptorAlloc = variableAlloc->pDescriptorCounts[i]; + } + if(Atomic::CmpExch32(&m_ReuseEnabled, 1, 1) == 1) { rdcarray &freelist = poolrecord->descPoolInfo->freelist; @@ -510,7 +555,8 @@ VkResult WrappedVulkan::vkAllocateDescriptorSets(VkDevice device, return a->descInfo->layout < search; }); - if(it != freelist.end() && (*it)->descInfo->layout == layoutRecord->descInfo->layout) + if(it != freelist.end() && (*it)->descInfo->layout == layoutRecord->descInfo->layout && + (*it)->descInfo->data.variableDescriptorCount == variableDescriptorAlloc) { record = freelist.takeAt(it - freelist.begin()); exactReuse = true; @@ -561,9 +607,15 @@ VkResult WrappedVulkan::vkAllocateDescriptorSets(VkDevice device, { CACHE_THREAD_SERIALISER(); - VkDescriptorSetAllocateInfo info = *pAllocateInfo; + VkDescriptorSetAllocateInfo info = mutableInfo; info.descriptorSetCount = 1; - info.pSetLayouts += i; + info.pSetLayouts = mutableInfo.pSetLayouts + i; + + if(mutableVariableInfo) + { + mutableVariableInfo->descriptorSetCount = 1; + mutableVariableInfo->pDescriptorCounts = variableAlloc->pDescriptorCounts + i; + } SCOPED_SERIALISE_CHUNK(VulkanChunk::vkAllocateDescriptorSets); Serialise_vkAllocateDescriptorSets(ser, device, &info, &pDescriptorSets[i]); @@ -577,7 +629,8 @@ VkResult WrappedVulkan::vkAllocateDescriptorSets(VkDevice device, record->AddParent(layoutRecord); record->descInfo->layout = layoutRecord->descInfo->layout; - record->descInfo->layout->CreateBindingsArray(record->descInfo->data); + record->descInfo->layout->CreateBindingsArray(record->descInfo->data, + variableDescriptorAlloc); } else { @@ -1209,6 +1262,9 @@ void WrappedVulkan::vkUpdateDescriptorSets(VkDevice device, uint32_t writeCount, // descriptors. All consecutive bindings updated via a single VkWriteDescriptorSet structure // must have identical descriptorType and stageFlags, and must all either use immutable // samplers or must all not use immutable samplers. + // + // Note we don't have to worry about this interacting with variable descriptor counts + // because the variable descriptor must be the last one, so there's no more overlap. if(curIdx >= layoutBinding->descriptorCount) { @@ -1655,6 +1711,9 @@ void WrappedVulkan::vkUpdateDescriptorSetWithTemplate( // descriptors. All consecutive bindings updated via a single VkWriteDescriptorSet structure // must have identical descriptorType and stageFlags, and must all either use immutable // samplers or must all not use immutable samplers. + // + // Note we don't have to worry about this interacting with variable descriptor counts + // because the variable descriptor must be the last one, so there's no more overlap. if(curIdx >= layoutBinding->descriptorCount) { diff --git a/util/test/demos/CMakeLists.txt b/util/test/demos/CMakeLists.txt index 4cf6059c4..a15298e46 100644 --- a/util/test/demos/CMakeLists.txt +++ b/util/test/demos/CMakeLists.txt @@ -13,6 +13,7 @@ set(VULKAN_SRC vk/vk_custom_border_color.cpp vk/vk_descriptor_index.cpp vk/vk_descriptor_reuse.cpp + vk/vk_descriptor_variable_count.cpp vk/vk_discard_rects.cpp vk/vk_discard_zoo.cpp vk/vk_draw_zoo.cpp diff --git a/util/test/demos/demos.vcxproj b/util/test/demos/demos.vcxproj index dadc3c08a..ab7cb2599 100644 --- a/util/test/demos/demos.vcxproj +++ b/util/test/demos/demos.vcxproj @@ -264,6 +264,7 @@ + diff --git a/util/test/demos/demos.vcxproj.filters b/util/test/demos/demos.vcxproj.filters index 3932bb7a5..d6aeb1fae 100644 --- a/util/test/demos/demos.vcxproj.filters +++ b/util/test/demos/demos.vcxproj.filters @@ -541,6 +541,9 @@ Vulkan\demos + + Vulkan\demos + diff --git a/util/test/demos/vk/vk_descriptor_variable_count.cpp b/util/test/demos/vk/vk_descriptor_variable_count.cpp new file mode 100644 index 000000000..454edbc27 --- /dev/null +++ b/util/test/demos/vk/vk_descriptor_variable_count.cpp @@ -0,0 +1,368 @@ +/****************************************************************************** + * The MIT License (MIT) + * + * Copyright (c) 2019-2020 Baldur Karlsson + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + ******************************************************************************/ + +#include "vk_test.h" + +RD_TEST(VK_Descriptor_Variable_Count, VulkanGraphicsTest) +{ + static constexpr const char *Description = + "Allocates descriptor sets using a variable count to ensure we don't pessimistically " + "allocate and don't do anything with un-allocated descriptors."; + + std::string common = R"EOSHADER( + +#version 450 core + +#extension GL_EXT_nonuniform_qualifier : require +#extension GL_EXT_samplerless_texture_functions : require + +struct v2f +{ + vec4 pos; + vec4 col; + vec4 uv; +}; + +)EOSHADER"; + + const std::string vertex = R"EOSHADER( + +layout(location = 0) in vec3 Position; +layout(location = 1) in vec4 Color; +layout(location = 2) in vec2 UV; + +layout(location = 0) out v2f vertOut; + +void main() +{ + vertOut.pos = vec4(Position.xyz*vec3(1,-1,1), 1); + gl_Position = vertOut.pos; + vertOut.col = Color; + vertOut.uv = vec4(UV.xy, 0, 1); +} + +)EOSHADER"; + + const std::string pixel = R"EOSHADER( + +layout(location = 0) in v2f vertIn; + +layout(location = 0, index = 0) out vec4 Color; + +layout(push_constant) uniform PushData +{ + uint bufidx; +} push; + +layout(binding = 0) uniform texture2D tex[]; + +void main() +{ + Color = texelFetch(tex[push.bufidx], ivec2(vertIn.uv.xy * vec2(4,4)), 0); +} + +)EOSHADER"; + + const static uint32_t numDescriptorSetsInLayout = 100 * 1024; + + void Prepare(int argc, char **argv) + { + devExts.push_back(VK_EXT_DESCRIPTOR_INDEXING_EXTENSION_NAME); + + // dependencies of VK_EXT_descriptor_indexing + devExts.push_back(VK_KHR_MAINTENANCE3_EXTENSION_NAME); + + // enable robustness2 if possible for NULL descriptors + optDevExts.push_back(VK_EXT_ROBUSTNESS_2_EXTENSION_NAME); + + VulkanGraphicsTest::Prepare(argc, argv); + + if(!Avail.empty()) + return; + + VkPhysicalDeviceProperties props; + vkGetPhysicalDeviceProperties(phys, &props); + + // require at least a million descriptors - we won't use them but this gives us enough headroom + // to check for overallocation + if(props.limits.maxDescriptorSetSamplers < numDescriptorSetsInLayout) + Avail = "maxDescriptorSetSamplers " + std::to_string(props.limits.maxDescriptorSetSamplers) + + " is insufficient"; + else if(props.limits.maxDescriptorSetSampledImages < numDescriptorSetsInLayout) + Avail = "maxDescriptorSetSampledImages " + + std::to_string(props.limits.maxDescriptorSetSampledImages) + " is insufficient"; + + if(!Avail.empty()) + return; + + static VkPhysicalDeviceDescriptorIndexingFeaturesEXT descIndexing = { + VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_DESCRIPTOR_INDEXING_FEATURES_EXT, + }; + + VkPhysicalDeviceDescriptorIndexingFeaturesEXT indexingAvail = descIndexing; + getPhysFeatures2(&indexingAvail); + + if(!indexingAvail.descriptorBindingPartiallyBound) + Avail = "Descriptor indexing feature 'descriptorBindingPartiallyBound' not available"; + else if(!indexingAvail.descriptorBindingVariableDescriptorCount) + Avail = + "Descriptor indexing feature 'descriptorBindingVariableDescriptorCount' not available"; + else if(!indexingAvail.runtimeDescriptorArray) + Avail = "Descriptor indexing feature 'runtimeDescriptorArray' not available"; + + descIndexing.descriptorBindingPartiallyBound = VK_TRUE; + descIndexing.descriptorBindingVariableDescriptorCount = VK_TRUE; + descIndexing.runtimeDescriptorArray = VK_TRUE; + + devInfoNext = &descIndexing; + + static VkPhysicalDeviceRobustness2FeaturesEXT robust2Feats = { + VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_ROBUSTNESS_2_FEATURES_EXT, + }; + + // enable NULL descriptors if they're supported and the extension was enabled + if(std::find(devExts.begin(), devExts.end(), VK_EXT_ROBUSTNESS_2_EXTENSION_NAME) != devExts.end()) + { + VkPhysicalDeviceRobustness2FeaturesEXT robust2avail = robust2Feats; + + getPhysFeatures2(&robust2avail); + + if(robust2avail.nullDescriptor) + robust2Feats.nullDescriptor = VK_TRUE; + robust2Feats.pNext = (void *)devInfoNext; + devInfoNext = &robust2Feats; + } + } + + int main() + { + // initialise, create window, create context, etc + if(!Init()) + return 3; + + const static uint32_t numDescriptorSets = 10 * 1024; + const static uint32_t numDescriptorsPerSet = 2; + + VkDescriptorSetLayoutBindingFlagsCreateInfoEXT descFlags = { + VK_STRUCTURE_TYPE_DESCRIPTOR_SET_LAYOUT_BINDING_FLAGS_CREATE_INFO_EXT, + }; + + VkDescriptorBindingFlagsEXT bindFlags[1] = { + VK_DESCRIPTOR_BINDING_VARIABLE_DESCRIPTOR_COUNT_BIT_EXT | + VK_DESCRIPTOR_BINDING_PARTIALLY_BOUND_BIT_EXT, + }; + + descFlags.bindingCount = 1; + descFlags.pBindingFlags = bindFlags; + + VkDescriptorSetLayout setlayout = + createDescriptorSetLayout(vkh::DescriptorSetLayoutCreateInfo( + { + { + 0, VK_DESCRIPTOR_TYPE_SAMPLED_IMAGE, + numDescriptorSetsInLayout, VK_SHADER_STAGE_FRAGMENT_BIT, + }, + }) + .next(&descFlags)); + + VkPipelineLayout layout = createPipelineLayout(vkh::PipelineLayoutCreateInfo( + { + setlayout, + }, + { + vkh::PushConstantRange(VK_SHADER_STAGE_FRAGMENT_BIT, 0, sizeof(Vec4i)), + })); + + vkh::GraphicsPipelineCreateInfo pipeCreateInfo; + + pipeCreateInfo.layout = layout; + pipeCreateInfo.renderPass = mainWindow->rp; + + pipeCreateInfo.vertexInputState.vertexBindingDescriptions = {vkh::vertexBind(0, DefaultA2V)}; + pipeCreateInfo.vertexInputState.vertexAttributeDescriptions = { + vkh::vertexAttr(0, 0, DefaultA2V, pos), vkh::vertexAttr(1, 0, DefaultA2V, col), + vkh::vertexAttr(2, 0, DefaultA2V, uv), + }; + + pipeCreateInfo.stages = { + CompileShaderModule(common + vertex, ShaderLang::glsl, ShaderStage::vert, "main"), + CompileShaderModule(common + pixel, ShaderLang::glsl, ShaderStage::frag, "main"), + }; + + VkPipeline pipe = createGraphicsPipeline(pipeCreateInfo); + + DefaultA2V tri[3] = { + {Vec3f(-0.5f, -0.5f, 0.0f), Vec4f(1.0f, 0.0f, 0.0f, 1.0f), Vec2f(0.0f, 0.0f)}, + {Vec3f(0.0f, 0.5f, 0.0f), Vec4f(0.0f, 1.0f, 0.0f, 1.0f), Vec2f(0.0f, 1.0f)}, + {Vec3f(0.5f, -0.5f, 0.0f), Vec4f(0.0f, 0.0f, 1.0f, 1.0f), Vec2f(1.0f, 0.0f)}, + }; + + AllocatedBuffer vb(this, vkh::BufferCreateInfo(sizeof(tri), VK_BUFFER_USAGE_VERTEX_BUFFER_BIT | + VK_BUFFER_USAGE_TRANSFER_DST_BIT), + VmaAllocationCreateInfo({0, VMA_MEMORY_USAGE_CPU_TO_GPU})); + + vb.upload(tri); + + AllocatedImage img( + this, vkh::ImageCreateInfo(4, 4, 0, VK_FORMAT_R32G32B32A32_SFLOAT, + VK_IMAGE_USAGE_TRANSFER_DST_BIT | VK_IMAGE_USAGE_SAMPLED_BIT), + VmaAllocationCreateInfo({0, VMA_MEMORY_USAGE_GPU_ONLY})); + + setName(img.image, "Colour Tex"); + + VkImageView imgview = createImageView( + vkh::ImageViewCreateInfo(img.image, VK_IMAGE_VIEW_TYPE_2D, VK_FORMAT_R32G32B32A32_SFLOAT)); + + Vec4f pixels[4 * 4]; + for(int i = 0; i < 4 * 4; i++) + pixels[i] = Vec4f(0.0f, 1.0f, 0.0f, 1.0f); + + AllocatedBuffer uploadBuf( + this, vkh::BufferCreateInfo(sizeof(pixels), VK_BUFFER_USAGE_TRANSFER_SRC_BIT), + VmaAllocationCreateInfo({0, VMA_MEMORY_USAGE_CPU_TO_GPU})); + + uploadBuf.upload(pixels); + + uploadBufferToImage(img.image, {4, 4, 1}, uploadBuf.buffer, + VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL); + + std::vector descsets; + VkDescriptorPool descpool = VK_NULL_HANDLE; + + descsets.resize(numDescriptorSets); + + { + CHECK_VKR(vkCreateDescriptorPool( + device, vkh::DescriptorPoolCreateInfo(numDescriptorSets, + { + {VK_DESCRIPTOR_TYPE_SAMPLED_IMAGE, + numDescriptorSets * numDescriptorsPerSet}, + }), + NULL, &descpool)); + + std::vector setLayouts(numDescriptorSets, setlayout); + std::vector counts(numDescriptorSets, numDescriptorsPerSet); + + // make the last one large-ish, to ensure that we still pass the right count through for each + // set + counts.back() = std::min(100U, numDescriptorSetsInLayout); + + VkDescriptorSetVariableDescriptorCountAllocateInfoEXT countInfo = { + VK_STRUCTURE_TYPE_DESCRIPTOR_SET_VARIABLE_DESCRIPTOR_COUNT_ALLOCATE_INFO_EXT, NULL, + numDescriptorSets, counts.data(), + }; + + VkDescriptorSetAllocateInfo allocInfo = { + VK_STRUCTURE_TYPE_DESCRIPTOR_SET_ALLOCATE_INFO, + &countInfo, + descpool, + numDescriptorSets, + setLayouts.data(), + }; + + CHECK_VKR(vkAllocateDescriptorSets(device, &allocInfo, descsets.data())); + } + + vkh::DescriptorImageInfo iminfo(imgview, VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL, + VK_NULL_HANDLE); + vkh::WriteDescriptorSet up(VK_NULL_HANDLE, 0, 0, VK_DESCRIPTOR_TYPE_SAMPLED_IMAGE, {iminfo}); + + up.pImageInfo = &iminfo; + up.dstArrayElement = 0; + up.descriptorCount = 1; + + std::vector ups; + + // fill the descriptor sets + for(uint32_t i = 0; i < numDescriptorSets; i++) + { + up.dstSet = descsets[i]; + + if(i == numDescriptorSets - 1) + up.dstArrayElement = std::min(100U, numDescriptorSetsInLayout) - 1; + + ups.push_back(up); + } + + vkh::updateDescriptorSets(device, ups); + + while(Running()) + { + VkCommandBuffer cmd = GetCommandBuffer(); + + vkBeginCommandBuffer(cmd, vkh::CommandBufferBeginInfo()); + + VkImage swapimg = + StartUsingBackbuffer(cmd, VK_ACCESS_TRANSFER_WRITE_BIT, VK_IMAGE_LAYOUT_GENERAL); + + vkCmdClearColorImage(cmd, swapimg, VK_IMAGE_LAYOUT_GENERAL, + vkh::ClearColorValue(0.2f, 0.2f, 0.2f, 1.0f), 1, + vkh::ImageSubresourceRange()); + + Vec4i idx = {}; + vkCmdPushConstants(cmd, layout, VK_SHADER_STAGE_FRAGMENT_BIT, 0, sizeof(Vec4i), &idx); + + vkCmdBeginRenderPass( + cmd, vkh::RenderPassBeginInfo(mainWindow->rp, mainWindow->GetFB(), mainWindow->scissor), + VK_SUBPASS_CONTENTS_INLINE); + + vkCmdBindPipeline(cmd, VK_PIPELINE_BIND_POINT_GRAPHICS, pipe); + vkCmdSetViewport(cmd, 0, 1, &mainWindow->viewport); + vkCmdSetScissor(cmd, 0, 1, &mainWindow->scissor); + vkh::cmdBindVertexBuffers(cmd, 0, {vb.buffer}, {0}); + + // force all descriptor sets to be referenced + for(uint32_t i = 0; i < numDescriptorSets; i++) + { + // for the last set, use the last descriptor + if(i == numDescriptorSets - 1) + idx.x = std::min(100U, numDescriptorSetsInLayout) - 1; + vkCmdPushConstants(cmd, layout, VK_SHADER_STAGE_FRAGMENT_BIT, 0, sizeof(Vec4i), &idx); + + vkCmdBindDescriptorSets(cmd, VK_PIPELINE_BIND_POINT_GRAPHICS, layout, 0, 1, &descsets[i], 0, + NULL); + + vkCmdDraw(cmd, 3, 1, 0, 0); + } + + vkCmdEndRenderPass(cmd); + + FinishUsingBackbuffer(cmd, VK_ACCESS_TRANSFER_WRITE_BIT, VK_IMAGE_LAYOUT_GENERAL); + + vkEndCommandBuffer(cmd); + + Submit(0, 1, {cmd}); + + Present(); + } + + vkDeviceWaitIdle(device); + + vkDestroyDescriptorPool(device, descpool, NULL); + + return 0; + } +}; + +REGISTER_TEST(); diff --git a/util/test/tests/Vulkan/VK_Descriptor_Variable_Count.py b/util/test/tests/Vulkan/VK_Descriptor_Variable_Count.py new file mode 100644 index 000000000..591593966 --- /dev/null +++ b/util/test/tests/Vulkan/VK_Descriptor_Variable_Count.py @@ -0,0 +1,13 @@ +import renderdoc as rd +import rdtest + + +class VK_Descriptor_Variable_Count(rdtest.TestCase): + demos_test_name = 'VK_Descriptor_Variable_Count' + + def check_capture(self): + last_draw: rd.DrawcallDescription = self.get_last_draw() + + self.controller.SetFrameEvent(last_draw.eventId, True) + + self.check_triangle(out=last_draw.copyDestination)