From 8ae9ccdd7f84c01b8b4f7c463beaaa3a1c5e6f40 Mon Sep 17 00:00:00 2001 From: baldurk Date: Mon, 25 Feb 2019 14:00:03 +0000 Subject: [PATCH] Check descriptor writes for validity before updating in PostVS fetch * This can happen for push descriptors that are declared but never used. --- renderdoc/driver/vulkan/vk_common.cpp | 35 +++++++++++++++ renderdoc/driver/vulkan/vk_common.h | 2 + renderdoc/driver/vulkan/vk_initstate.cpp | 35 --------------- renderdoc/driver/vulkan/vk_postvs.cpp | 54 +++++++++++++++++++++--- 4 files changed, 84 insertions(+), 42 deletions(-) diff --git a/renderdoc/driver/vulkan/vk_common.cpp b/renderdoc/driver/vulkan/vk_common.cpp index 883865bbf..153ffe50f 100644 --- a/renderdoc/driver/vulkan/vk_common.cpp +++ b/renderdoc/driver/vulkan/vk_common.cpp @@ -794,6 +794,41 @@ FrameRefType GetRefType(VkDescriptorType descType) return eFrameRef_Read; } +bool IsValid(const VkWriteDescriptorSet &write, uint32_t arrayElement) +{ + // this makes assumptions that only hold within the context of Serialise_InitialState below, + // specifically that if pTexelBufferView/pBufferInfo is set then we are using them. In the general + // case they can be garbage and we must ignore them based on the descriptorType + + if(write.pTexelBufferView) + return write.pTexelBufferView[arrayElement] != VK_NULL_HANDLE; + + if(write.pBufferInfo) + return write.pBufferInfo[arrayElement].buffer != VK_NULL_HANDLE; + + if(write.pImageInfo) + { + // only these two types need samplers + bool needSampler = (write.descriptorType == VK_DESCRIPTOR_TYPE_SAMPLER || + write.descriptorType == VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER); + + // but all types that aren't just a sampler need an image + bool needImage = (write.descriptorType != VK_DESCRIPTOR_TYPE_SAMPLER); + + if(needSampler && write.pImageInfo[arrayElement].sampler == VK_NULL_HANDLE) + return false; + + if(needImage && write.pImageInfo[arrayElement].imageView == VK_NULL_HANDLE) + return false; + + return true; + } + + RDCERR("Encountered VkWriteDescriptorSet with no data!"); + + return false; +} + void DescriptorSetSlot::RemoveBindRefs(VkResourceRecord *record) { if(texelBufferView != VK_NULL_HANDLE) diff --git a/renderdoc/driver/vulkan/vk_common.h b/renderdoc/driver/vulkan/vk_common.h index 195800c34..810b6303c 100644 --- a/renderdoc/driver/vulkan/vk_common.h +++ b/renderdoc/driver/vulkan/vk_common.h @@ -379,6 +379,8 @@ struct DescriptorSetSlot DECLARE_REFLECTION_STRUCT(DescriptorSetSlot); +bool IsValid(const VkWriteDescriptorSet &write, uint32_t arrayElement); + #define NUM_VK_IMAGE_ASPECTS 4 #define VK_ACCESS_ALL_READ_BITS \ (VK_ACCESS_INDIRECT_COMMAND_READ_BIT | VK_ACCESS_INDEX_READ_BIT | \ diff --git a/renderdoc/driver/vulkan/vk_initstate.cpp b/renderdoc/driver/vulkan/vk_initstate.cpp index ddbe07f1f..7b52d0905 100644 --- a/renderdoc/driver/vulkan/vk_initstate.cpp +++ b/renderdoc/driver/vulkan/vk_initstate.cpp @@ -675,41 +675,6 @@ static const char *NameOfType(VkResourceType type) return "VkResource"; } -static bool IsValid(const VkWriteDescriptorSet &write, uint32_t arrayElement) -{ - // this makes assumptions that only hold within the context of Serialise_InitialState below, - // specifically that if pTexelBufferView/pBufferInfo is set then we are using them. In the general - // case they can be garbage and we must ignore them based on the descriptorType - - if(write.pTexelBufferView) - return write.pTexelBufferView[arrayElement] != VK_NULL_HANDLE; - - if(write.pBufferInfo) - return write.pBufferInfo[arrayElement].buffer != VK_NULL_HANDLE; - - if(write.pImageInfo) - { - // only these two types need samplers - bool needSampler = (write.descriptorType == VK_DESCRIPTOR_TYPE_SAMPLER || - write.descriptorType == VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER); - - // but all types that aren't just a sampler need an image - bool needImage = (write.descriptorType != VK_DESCRIPTOR_TYPE_SAMPLER); - - if(needSampler && write.pImageInfo[arrayElement].sampler == VK_NULL_HANDLE) - return false; - - if(needImage && write.pImageInfo[arrayElement].imageView == VK_NULL_HANDLE) - return false; - - return true; - } - - RDCERR("Encountered VkWriteDescriptorSet with no data!"); - - return false; -} - // second parameter isn't used, as we might be serialising init state for a deleted resource template bool WrappedVulkan::Serialise_InitialState(SerialiserType &ser, ResourceId id, WrappedVkRes *) diff --git a/renderdoc/driver/vulkan/vk_postvs.cpp b/renderdoc/driver/vulkan/vk_postvs.cpp index 066234108..e646abdd0 100644 --- a/renderdoc/driver/vulkan/vk_postvs.cpp +++ b/renderdoc/driver/vulkan/vk_postvs.cpp @@ -1351,6 +1351,9 @@ void VulkanReplay::FetchVSOut(uint32_t eventId) // into them { std::vector descWrites; + std::vector allocImgWrites; + std::vector allocBufWrites; + std::vector allocBufViewWrites; // one for each descriptor type. 1 of each to start with plus enough for our internal resources, // we then increment for each descriptor we need to allocate @@ -1556,6 +1559,7 @@ void VulkanReplay::FetchVSOut(uint32_t eventId) for(uint32_t w = 0; w < write.descriptorCount; w++) out[w] = slot[w].imageInfo; write.pImageInfo = out; + allocImgWrites.push_back(out); break; } case VK_DESCRIPTOR_TYPE_UNIFORM_TEXEL_BUFFER: @@ -1565,6 +1569,7 @@ void VulkanReplay::FetchVSOut(uint32_t eventId) for(uint32_t w = 0; w < write.descriptorCount; w++) out[w] = slot[w].texelBufferView; write.pTexelBufferView = out; + allocBufViewWrites.push_back(out); break; } case VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER: @@ -1576,12 +1581,47 @@ void VulkanReplay::FetchVSOut(uint32_t eventId) for(uint32_t w = 0; w < write.descriptorCount; w++) out[w] = slot[w].bufferInfo; write.pBufferInfo = out; + allocBufWrites.push_back(out); break; } default: RDCERR("Unexpected descriptor type %d", write.descriptorType); } - descWrites.push_back(write); + // start with no descriptors + write.descriptorCount = 0; + + for(uint32_t w = 0; w < bind.descriptorCount; w++) + { + // if this write is valid, we increment the descriptor count and continue + if(IsValid(write, w)) + { + write.descriptorCount++; + } + else + { + // if this write isn't valid, then we first check to see if we had any previous + // pending writes in the array we were going to batch together, if so we add them. + if(write.descriptorCount > 0) + descWrites.push_back(write); + + // skip past any previous descriptors we just wrote, as well as the current invalid + // one + if(write.pBufferInfo) + write.pBufferInfo += write.descriptorCount + 1; + if(write.pImageInfo) + write.pImageInfo += write.descriptorCount + 1; + if(write.pTexelBufferView) + write.pTexelBufferView += write.descriptorCount + 1; + + // now start again from 0 descriptors, at the next array element + write.dstArrayElement += write.descriptorCount + 1; + write.descriptorCount = 0; + } + } + + // if there are any left, add them here + if(write.descriptorCount > 0) + descWrites.push_back(write); // don't leak the arrays and cause double deletes, NULL them after each time write.pImageInfo = NULL; @@ -1594,12 +1634,12 @@ void VulkanReplay::FetchVSOut(uint32_t eventId) m_pDriver->vkUpdateDescriptorSets(dev, (uint32_t)descWrites.size(), descWrites.data(), 0, NULL); // delete allocated arrays for descriptor writes - for(const VkWriteDescriptorSet &write : descWrites) - { - delete[] write.pBufferInfo; - delete[] write.pImageInfo; - delete[] write.pTexelBufferView; - } + for(VkDescriptorBufferInfo *a : allocBufWrites) + delete[] a; + for(VkDescriptorImageInfo *a : allocImgWrites) + delete[] a; + for(VkBufferView *a : allocBufViewWrites) + delete[] a; } // create pipeline layout with new descriptor set layouts