From f4fedc295ff3feb04ed85746d5279a8708602db6 Mon Sep 17 00:00:00 2001 From: baldurk Date: Wed, 14 Jun 2017 11:24:39 +0100 Subject: [PATCH] Handle descriptor set layout compatibility and sets becoming invalidated * The example case is as follows: - program binds a pipeline and 4 sets - later, program rebinds a different pipeline with incompatible layout - all 4 sets are invalid - program only binds 2 sets as new program shaders only statically use the first 2 sets. * If we try to 'restore' the invalid set bindings at any point this is invalid and can lead to crashes. We need to detect when sets have been invalidated and bail out, assuming that the shader only statically uses the sets that were valid. * Of course if that is an unfounded assumption then we are not binding sets that will be used, but this is just as invalid as leaving used sets as invalid. * We also need to change the PostVS data fetch, since we can't use the set after all used sets, we have to instead see which set is the last statically used one and use the next (even if it's in the original layout). --- renderdoc/driver/vulkan/vk_core.cpp | 29 ++++-- renderdoc/driver/vulkan/vk_core.h | 5 + renderdoc/driver/vulkan/vk_debug.cpp | 98 +++++++++++-------- renderdoc/driver/vulkan/vk_info.cpp | 39 ++++++++ renderdoc/driver/vulkan/vk_info.h | 3 + renderdoc/driver/vulkan/vk_state.cpp | 41 ++++++-- renderdoc/driver/vulkan/vk_state.h | 18 +++- .../vulkan/wrappers/vk_descriptor_funcs.cpp | 2 + 8 files changed, 176 insertions(+), 59 deletions(-) diff --git a/renderdoc/driver/vulkan/vk_core.cpp b/renderdoc/driver/vulkan/vk_core.cpp index c9d0f2cb6..bf3ab20ef 100644 --- a/renderdoc/driver/vulkan/vk_core.cpp +++ b/renderdoc/driver/vulkan/vk_core.cpp @@ -256,7 +256,7 @@ void VkInitParams::Set(const VkInstanceCreateInfo *pCreateInfo, ResourceId inst) InstanceID = inst; } -WrappedVulkan::WrappedVulkan(const char *logFilename) : m_RenderState(&m_CreationInfo) +WrappedVulkan::WrappedVulkan(const char *logFilename) : m_RenderState(this, &m_CreationInfo) { #if ENABLED(RDOC_RELEASE) const bool debugSerialiser = false; @@ -317,7 +317,6 @@ WrappedVulkan::WrappedVulkan(const char *logFilename) : m_RenderState(&m_Creatio m_DebugManager = NULL; m_pSerialiser->SetUserData(m_ResourceManager); - m_RenderState.m_ResourceManager = GetResourceManager(); m_Instance = VK_NULL_HANDLE; m_PhysicalDevice = VK_NULL_HANDLE; @@ -2361,8 +2360,7 @@ void WrappedVulkan::ReplayLog(uint32_t startEventID, uint32_t endEventID, Replay RDCASSERT(m_Partial[Secondary].resultPartialCmdBuffer == VK_NULL_HANDLE); m_Partial[Primary].Reset(); m_Partial[Secondary].Reset(); - m_RenderState = VulkanRenderState(&m_CreationInfo); - m_RenderState.m_ResourceManager = GetResourceManager(); + m_RenderState = VulkanRenderState(this, &m_CreationInfo); } VkResult vkr = VK_SUCCESS; @@ -2414,13 +2412,32 @@ void WrappedVulkan::ReplayLog(uint32_t startEventID, uint32_t endEventID, Replay VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, false, 0, NULL, 0, NULL, (uint32_t)imgBarriers.size(), &imgBarriers[0]); + const DrawcallDescription *draw = GetDrawcall(endEventID); + + bool rpUnneeded = false; + + // if we're only replaying a draw, and it's not a drawcall or dispatch, don't try and bind + // all the replay state as we don't know if it will be valid. + if(replayType == eReplay_OnlyDraw) + { + if(!draw) + { + rpUnneeded = true; + } + else if(!(draw->flags & (DrawFlags::Drawcall | DrawFlags::Dispatch))) + { + rpUnneeded = true; + } + } + // if a render pass was active, begin it and set up the partial replay state - m_RenderState.BeginRenderPassAndApplyState(cmd); + m_RenderState.BeginRenderPassAndApplyState( + cmd, rpUnneeded ? VulkanRenderState::BindNone : VulkanRenderState::BindGraphics); } else if(m_RenderState.compute.pipeline != ResourceId()) { // if we had a compute pipeline, need to bind that - m_RenderState.BindPipeline(cmd); + m_RenderState.BindPipeline(cmd, VulkanRenderState::BindCompute); } } diff --git a/renderdoc/driver/vulkan/vk_core.h b/renderdoc/driver/vulkan/vk_core.h index e0d2c39fd..e59f7a7d6 100644 --- a/renderdoc/driver/vulkan/vk_core.h +++ b/renderdoc/driver/vulkan/vk_core.h @@ -714,6 +714,11 @@ public: uint32_t GetMaxEID() { return m_Events.back().eventID; } const DrawcallDescription *GetDrawcall(uint32_t eventID); + ResourceId GetDescLayoutForDescSet(ResourceId descSet) + { + return m_DescriptorSetState[descSet].layout; + } + vector GetUsage(ResourceId id) { return m_ResourceUses[id]; } // return the pre-selected device and queue VkDevice GetDev() diff --git a/renderdoc/driver/vulkan/vk_debug.cpp b/renderdoc/driver/vulkan/vk_debug.cpp index 6646c8215..1457d8efc 100644 --- a/renderdoc/driver/vulkan/vk_debug.cpp +++ b/renderdoc/driver/vulkan/vk_debug.cpp @@ -4771,7 +4771,7 @@ void VulkanDebugManager::PatchFixedColShader(VkShaderModule &mod, float col[4]) struct VulkanQuadOverdrawCallback : public VulkanDrawcallCallback { VulkanQuadOverdrawCallback(WrappedVulkan *vk, const vector &events) - : m_pDriver(vk), m_pDebug(vk->GetDebugManager()), m_Events(events), m_PrevState(NULL) + : m_pDriver(vk), m_pDebug(vk->GetDebugManager()), m_Events(events), m_PrevState(vk, NULL) { m_pDriver->SetDrawcallCB(this); } @@ -4951,7 +4951,7 @@ struct VulkanQuadOverdrawCallback : public VulkanDrawcallCallback pipestate.graphics.descSets[pipe.first].descSet = GetResID(m_pDebug->m_QuadDescSet); if(cmd) - pipestate.BindPipeline(cmd); + pipestate.BindPipeline(cmd, VulkanRenderState::BindGraphics); } bool PostDraw(uint32_t eid, VkCommandBuffer cmd) @@ -4963,7 +4963,7 @@ struct VulkanQuadOverdrawCallback : public VulkanDrawcallCallback m_pDriver->GetRenderState() = m_PrevState; RDCASSERT(cmd); - m_pDriver->GetRenderState().BindPipeline(cmd); + m_pDriver->GetRenderState().BindPipeline(cmd, VulkanRenderState::BindGraphics); return true; } @@ -6057,7 +6057,7 @@ ResourceId VulkanDebugManager::RenderOverlay(ResourceId texid, DebugOverlay over vkr = vt->BeginCommandBuffer(Unwrap(cmd), &beginInfo); RDCASSERTEQUAL(vkr, VK_SUCCESS); - m_pDriver->m_RenderState.BeginRenderPassAndApplyState(cmd); + m_pDriver->m_RenderState.BeginRenderPassAndApplyState(cmd, VulkanRenderState::BindGraphics); VkClearAttachment blackclear = {VK_IMAGE_ASPECT_COLOR_BIT, 0, {}}; vector atts; @@ -7084,7 +7084,7 @@ inline uint32_t MakeSPIRVOp(spv::Op op, uint32_t WordCount) } static void AddOutputDumping(const ShaderReflection &refl, const SPIRVPatchData &patchData, - const char *entryName, uint32_t descSet, uint32_t vertexIndexOffset, + const char *entryName, uint32_t &descSet, uint32_t vertexIndexOffset, uint32_t instanceIndexOffset, uint32_t numVerts, vector &modSpirv, uint32_t &bufStride) { @@ -7147,12 +7147,20 @@ static void AddOutputDumping(const ShaderReflection &refl, const SPIRVPatchData size_t decorateOffset = 0; size_t typeVarOffset = 0; + descSet = 0; + size_t it = 5; while(it < spirvLength) { uint16_t WordCount = spirv[it] >> spv::WordCountShift; spv::Op opcode = spv::Op(spirv[it] & spv::OpCodeMask); + // we will use the descriptor set immediately after the last set statically used by the shader. + // This means we don't have to worry about if the descriptor set layout declares more sets which + // might be invalid and un-bindable, we just trample over the next set that's unused + if(opcode == spv::OpDecorate && spirv[it + 2] == spv::DecorationDescriptorSet) + descSet = RDCMAX(descSet, spirv[it + 3] + 1); + if(opcode == spv::OpDecorate && spirv[it + 2] == spv::DecorationBuiltIn && spirv[it + 3] == spv::BuiltInVertexIndex) vertidxID = spirv[it + 1]; @@ -7907,52 +7915,26 @@ void VulkanDebugManager::InitPostVSBuffers(uint32_t eventID) if(drawcall == NULL || drawcall->numIndices == 0 || drawcall->numInstances == 0) return; - uint32_t descSet = (uint32_t)creationInfo.m_PipelineLayout[pipeInfo.layout].descSetLayouts.size(); + // the SPIR-V patching will determine the next descriptor set to use, after all sets statically + // used by the shader. This gets around the problem where the shader only uses 0 and 1, but the + // layout declares 0-4, and 2,3,4 are invalid at bind time and we are unable to bind our new set + // 5. Instead we'll notice that only 0 and 1 are used and just use 2 ourselves (although it was in + // the original set layout, we know it's statically unused by the shader so we can safely steal + // it). + uint32_t descSet = 0; // we go through the driver for all these creations since they need to be properly // registered in order to be put in the partial replay state VkResult vkr = VK_SUCCESS; VkDevice dev = m_Device; - VkDescriptorSetLayout *descSetLayouts; - - // descSet will be the index of our new descriptor set - descSetLayouts = new VkDescriptorSetLayout[descSet + 1]; - - for(uint32_t i = 0; i < descSet; i++) - descSetLayouts[i] = GetResourceManager()->GetCurrentHandle( - creationInfo.m_PipelineLayout[pipeInfo.layout].descSetLayouts[i]); - - // this layout just says it has one storage buffer - descSetLayouts[descSet] = m_MeshFetchDescSetLayout; - - const vector &push = creationInfo.m_PipelineLayout[pipeInfo.layout].pushRanges; - - VkPipelineLayoutCreateInfo pipeLayoutInfo = { - VK_STRUCTURE_TYPE_PIPELINE_LAYOUT_CREATE_INFO, - NULL, - 0, - descSet + 1, - descSetLayouts, - (uint32_t)push.size(), - push.empty() ? NULL : &push[0], - }; - - // create pipeline layout with same descriptor set layouts, plus our mesh output set VkPipelineLayout pipeLayout; - vkr = m_pDriver->vkCreatePipelineLayout(dev, &pipeLayoutInfo, NULL, &pipeLayout); - RDCASSERTEQUAL(vkr, VK_SUCCESS); - - SAFE_DELETE_ARRAY(descSetLayouts); VkGraphicsPipelineCreateInfo pipeCreateInfo; // get pipeline create info MakeGraphicsPipelineInfo(pipeCreateInfo, state.graphics.pipeline); - // repoint pipeline layout - pipeCreateInfo.layout = pipeLayout; - // set primitive topology to point list VkPipelineInputAssemblyStateCreateInfo *ia = (VkPipelineInputAssemblyStateCreateInfo *)pipeCreateInfo.pInputAssemblyState; @@ -8153,6 +8135,42 @@ void VulkanDebugManager::InitPostVSBuffers(uint32_t eventID) descSet, vertexIndexOffset, drawcall->instanceOffset, numVerts, modSpirv, bufStride); + { + VkDescriptorSetLayout *descSetLayouts; + + // descSet will be the index of our new descriptor set + descSetLayouts = new VkDescriptorSetLayout[descSet + 1]; + + for(uint32_t i = 0; i < descSet; i++) + descSetLayouts[i] = GetResourceManager()->GetCurrentHandle( + creationInfo.m_PipelineLayout[pipeInfo.layout].descSetLayouts[i]); + + // this layout just says it has one storage buffer + descSetLayouts[descSet] = m_MeshFetchDescSetLayout; + + const vector &push = + creationInfo.m_PipelineLayout[pipeInfo.layout].pushRanges; + + VkPipelineLayoutCreateInfo pipeLayoutInfo = { + VK_STRUCTURE_TYPE_PIPELINE_LAYOUT_CREATE_INFO, + NULL, + 0, + descSet + 1, + descSetLayouts, + (uint32_t)push.size(), + push.empty() ? NULL : &push[0], + }; + + // create pipeline layout with same descriptor set layouts, plus our mesh output set + vkr = m_pDriver->vkCreatePipelineLayout(dev, &pipeLayoutInfo, NULL, &pipeLayout); + RDCASSERTEQUAL(vkr, VK_SUCCESS); + + SAFE_DELETE_ARRAY(descSetLayouts); + + // repoint pipeline layout + pipeCreateInfo.layout = pipeLayout; + } + // create vertex shader with modified code VkShaderModuleCreateInfo moduleCreateInfo = { VK_STRUCTURE_TYPE_SHADER_MODULE_CREATE_INFO, NULL, 0, @@ -8265,7 +8283,7 @@ void VulkanDebugManager::InitPostVSBuffers(uint32_t eventID) RDCASSERTEQUAL(vkr, VK_SUCCESS); // do single draw - modifiedstate.BeginRenderPassAndApplyState(cmd); + modifiedstate.BeginRenderPassAndApplyState(cmd, VulkanRenderState::BindGraphics); ObjDisp(cmd)->CmdDraw(Unwrap(cmd), drawcall->numIndices, drawcall->numInstances, drawcall->vertexOffset, drawcall->instanceOffset); modifiedstate.EndRenderPass(cmd); @@ -8406,7 +8424,7 @@ void VulkanDebugManager::InitPostVSBuffers(uint32_t eventID) m_pDriver->vkUpdateDescriptorSets(dev, 1, &write, 0, NULL); // do single draw - modifiedstate.BeginRenderPassAndApplyState(cmd); + modifiedstate.BeginRenderPassAndApplyState(cmd, VulkanRenderState::BindGraphics); ObjDisp(cmd)->CmdDrawIndexed(Unwrap(cmd), (uint32_t)indices.size(), drawcall->numInstances, 0, drawcall->baseVertex, drawcall->instanceOffset); modifiedstate.EndRenderPass(cmd); diff --git a/renderdoc/driver/vulkan/vk_info.cpp b/renderdoc/driver/vulkan/vk_info.cpp index be0a7cdd3..db18e55e2 100644 --- a/renderdoc/driver/vulkan/vk_info.cpp +++ b/renderdoc/driver/vulkan/vk_info.cpp @@ -81,6 +81,45 @@ void DescSetLayout::CreateBindingsArray(vector &descBinding } } +bool DescSetLayout::operator==(const DescSetLayout &other) const +{ + // shortcut for equality to ourselves + if(this == &other) + return true; + + // descriptor set layouts are different if they have different set of bindings. + if(bindings.size() != other.bindings.size()) + return false; + + // iterate over each binding (we know this loop indexes validly in both arrays + for(size_t i = 0; i < bindings.size(); i++) + { + const Binding &a = bindings[i]; + const Binding &b = other.bindings[i]; + + // if the type/stages/count are different, the layout is different + if(a.descriptorCount != b.descriptorCount || a.descriptorType != b.descriptorType || + a.stageFlags != b.stageFlags) + return false; + + // if one has immutable samplers but the other doesn't, they're different + if((a.immutableSampler && !b.immutableSampler) || (!a.immutableSampler && b.immutableSampler)) + return false; + + // if we DO have immutable samplers, they must all point to the same sampler objects. + if(a.immutableSampler) + { + for(uint32_t s = 0; s < a.descriptorCount; s++) + { + if(a.immutableSampler[s] != b.immutableSampler[s]) + return false; + } + } + } + + return true; +} + void VulkanCreationInfo::Pipeline::Init(VulkanResourceManager *resourceMan, VulkanCreationInfo &info, const VkGraphicsPipelineCreateInfo *pCreateInfo) { diff --git a/renderdoc/driver/vulkan/vk_info.h b/renderdoc/driver/vulkan/vk_info.h index 7fb115e96..081105a01 100644 --- a/renderdoc/driver/vulkan/vk_info.h +++ b/renderdoc/driver/vulkan/vk_info.h @@ -71,6 +71,9 @@ struct DescSetLayout vector bindings; uint32_t dynamicCount; + + bool operator==(const DescSetLayout &other) const; + bool operator!=(const DescSetLayout &other) const { return !(*this == other); } }; struct VulkanCreationInfo diff --git a/renderdoc/driver/vulkan/vk_state.cpp b/renderdoc/driver/vulkan/vk_state.cpp index 06806af66..30d8bb0e2 100644 --- a/renderdoc/driver/vulkan/vk_state.cpp +++ b/renderdoc/driver/vulkan/vk_state.cpp @@ -23,10 +23,12 @@ ******************************************************************************/ #include "vk_state.h" +#include "vk_core.h" #include "vk_info.h" #include "vk_resources.h" -VulkanRenderState::VulkanRenderState(VulkanCreationInfo *createInfo) : m_CreationInfo(createInfo) +VulkanRenderState::VulkanRenderState(WrappedVulkan *driver, VulkanCreationInfo *createInfo) + : m_CreationInfo(createInfo), m_pDriver(driver) { compute.pipeline = graphics.pipeline = renderPass = framebuffer = ResourceId(); compute.descSets.clear(); @@ -43,8 +45,6 @@ VulkanRenderState::VulkanRenderState(VulkanCreationInfo *createInfo) : m_Creatio RDCEraseEl(back); RDCEraseEl(pushconsts); - m_ResourceManager = NULL; - renderPass = ResourceId(); subpass = 0; @@ -83,7 +83,7 @@ VulkanRenderState &VulkanRenderState::operator=(const VulkanRenderState &o) return *this; } -void VulkanRenderState::BeginRenderPassAndApplyState(VkCommandBuffer cmd) +void VulkanRenderState::BeginRenderPassAndApplyState(VkCommandBuffer cmd, PipelineBinding binding) { RDCASSERT(renderPass != ResourceId()); @@ -106,7 +106,7 @@ void VulkanRenderState::BeginRenderPassAndApplyState(VkCommandBuffer cmd) }; ObjDisp(cmd)->CmdBeginRenderPass(Unwrap(cmd), &rpbegin, VK_SUBPASS_CONTENTS_INLINE); - BindPipeline(cmd); + BindPipeline(cmd, binding); if(ibuffer.buf != ResourceId()) ObjDisp(cmd)->CmdBindIndexBuffer( @@ -120,9 +120,9 @@ void VulkanRenderState::BeginRenderPassAndApplyState(VkCommandBuffer cmd) &vbuffers[i].offs); } -void VulkanRenderState::BindPipeline(VkCommandBuffer cmd) +void VulkanRenderState::BindPipeline(VkCommandBuffer cmd, PipelineBinding binding) { - if(graphics.pipeline != ResourceId()) + if(graphics.pipeline != ResourceId() && binding == BindGraphics) { ObjDisp(cmd)->CmdBindPipeline( Unwrap(cmd), VK_PIPELINE_BIND_POINT_GRAPHICS, @@ -193,6 +193,26 @@ void VulkanRenderState::BindPipeline(VkCommandBuffer cmd) if(i < graphics.descSets.size() && graphics.descSets[i].descSet != ResourceId()) { + // if we come to a descriptor set that isn't compatible, stop setting descriptor sets from + // here on. + // We can get into this situation if for example we have many sets bound at some point, then + // there's a pipeline change that causes most or all of them to be invalidated as + // incompatible, then the program only re-binds some subset that it knows is statically used + // by the next drawcall. The remaining sets are invalid, but also unused and this is + // explicitly allowed by the spec. We just have to make sure we don't try to actively bind + // an incompatible descriptor set. + ResourceId createdDescSetLayoutId = + m_pDriver->GetDescLayoutForDescSet(graphics.descSets[i].descSet); + + if(descSetLayouts[i] != createdDescSetLayoutId) + { + const DescSetLayout &createdDescLayout = + m_CreationInfo->m_DescSetLayout[createdDescSetLayoutId]; + + if(descLayout != createdDescLayout) + break; + } + // if there are dynamic buffers, pass along the offsets uint32_t *dynamicOffsets = NULL; @@ -235,7 +255,7 @@ void VulkanRenderState::BindPipeline(VkCommandBuffer cmd) } } - if(compute.pipeline != ResourceId()) + if(compute.pipeline != ResourceId() && binding == BindCompute) { ObjDisp(cmd)->CmdBindPipeline( Unwrap(cmd), VK_PIPELINE_BIND_POINT_COMPUTE, @@ -305,3 +325,8 @@ void VulkanRenderState::EndRenderPass(VkCommandBuffer cmd) { ObjDisp(cmd)->CmdEndRenderPass(Unwrap(cmd)); } + +VulkanResourceManager *VulkanRenderState::GetResourceManager() +{ + return m_pDriver->GetResourceManager(); +} diff --git a/renderdoc/driver/vulkan/vk_state.h b/renderdoc/driver/vulkan/vk_state.h index 0d142b23c..5223ad177 100644 --- a/renderdoc/driver/vulkan/vk_state.h +++ b/renderdoc/driver/vulkan/vk_state.h @@ -29,14 +29,22 @@ struct VulkanCreationInfo; class VulkanResourceManager; +class WrappedVulkan; struct VulkanRenderState { - VulkanRenderState(VulkanCreationInfo *createInfo); + enum PipelineBinding + { + BindNone = 0x0, + BindGraphics = 0x1, + BindCompute = 0x2, + }; + + VulkanRenderState(WrappedVulkan *driver, VulkanCreationInfo *createInfo); VulkanRenderState &operator=(const VulkanRenderState &o); - void BeginRenderPassAndApplyState(VkCommandBuffer cmd); + void BeginRenderPassAndApplyState(VkCommandBuffer cmd, PipelineBinding binding); void EndRenderPass(VkCommandBuffer cmd); - void BindPipeline(VkCommandBuffer cmd); + void BindPipeline(VkCommandBuffer cmd, PipelineBinding binding); // dynamic state vector views; @@ -88,7 +96,7 @@ struct VulkanRenderState }; vector vbuffers; - VulkanResourceManager *GetResourceManager() { return m_ResourceManager; } - VulkanResourceManager *m_ResourceManager; + VulkanResourceManager *GetResourceManager(); VulkanCreationInfo *m_CreationInfo; + WrappedVulkan *m_pDriver; }; diff --git a/renderdoc/driver/vulkan/wrappers/vk_descriptor_funcs.cpp b/renderdoc/driver/vulkan/wrappers/vk_descriptor_funcs.cpp index 9665b03cb..b5a859547 100644 --- a/renderdoc/driver/vulkan/wrappers/vk_descriptor_funcs.cpp +++ b/renderdoc/driver/vulkan/wrappers/vk_descriptor_funcs.cpp @@ -339,6 +339,8 @@ VkResult WrappedVulkan::vkAllocateDescriptorSets(VkDevice device, else { GetResourceManager()->AddLiveResource(id, pDescriptorSets[i]); + + m_DescriptorSetState[id].layout = GetResID(pAllocateInfo->pSetLayouts[i]); } }