From 6042c500bc024ad317beeac86616ce47f2ef8f2b Mon Sep 17 00:00:00 2001 From: baldurk Date: Tue, 21 Apr 2020 12:22:42 +0100 Subject: [PATCH] Ensure we don't read out of bounds displaying meshes on vulkan * The whole stride of a vertex binding must be in bounds for any attributes in it to be in bounds, so we adjust the attribute offset to put padding at the start of the strided segment wherever possible. --- renderdoc/driver/vulkan/vk_debug.cpp | 2 +- renderdoc/driver/vulkan/vk_debug.h | 13 ++- renderdoc/driver/vulkan/vk_rendermesh.cpp | 113 +++++++++++++++++----- 3 files changed, 96 insertions(+), 32 deletions(-) diff --git a/renderdoc/driver/vulkan/vk_debug.cpp b/renderdoc/driver/vulkan/vk_debug.cpp index 101f18253..fc0c8f363 100644 --- a/renderdoc/driver/vulkan/vk_debug.cpp +++ b/renderdoc/driver/vulkan/vk_debug.cpp @@ -748,7 +748,7 @@ VulkanDebugManager::~VulkanDebugManager() m_ReadbackWindow.Destroy(); for(auto it = m_CachedMeshPipelines.begin(); it != m_CachedMeshPipelines.end(); ++it) - for(uint32_t i = 0; i < MeshDisplayPipelines::ePipe_Count; i++) + for(uint32_t i = 0; i < VKMeshDisplayPipelines::ePipe_Count; i++) m_pDriver->vkDestroyPipeline(dev, it->second.pipes[i], NULL); m_pDriver->vkDestroyDescriptorPool(dev, m_ArrayMSDescriptorPool, NULL); diff --git a/renderdoc/driver/vulkan/vk_debug.h b/renderdoc/driver/vulkan/vk_debug.h index 4a4eb9d12..849223f8d 100644 --- a/renderdoc/driver/vulkan/vk_debug.h +++ b/renderdoc/driver/vulkan/vk_debug.h @@ -29,7 +29,7 @@ #include "vk_core.h" #include "vk_shader_cache.h" -struct MeshDisplayPipelines +struct VKMeshDisplayPipelines { enum { @@ -43,6 +43,9 @@ struct MeshDisplayPipelines }; VkPipeline pipes[ePipe_Count] = {}; + + uint32_t primaryStridePadding = 0; + uint32_t secondaryStridePadding = 0; }; struct CopyPixelParams; @@ -73,9 +76,9 @@ public: void CreateCustomShaderTex(uint32_t width, uint32_t height, uint32_t mip); void CreateCustomShaderPipeline(ResourceId shader, VkPipelineLayout pipeLayout); - MeshDisplayPipelines CacheMeshDisplayPipelines(VkPipelineLayout pipeLayout, - const MeshFormat &primary, - const MeshFormat &secondary); + VKMeshDisplayPipelines CacheMeshDisplayPipelines(VkPipelineLayout pipeLayout, + const MeshFormat &primary, + const MeshFormat &secondary); void PatchOutputLocation(VkShaderModule &mod, BuiltinShader shaderType, uint32_t framebufferIndex); void PatchFixedColShader(VkShaderModule &mod, float col[4]); @@ -106,7 +109,7 @@ private: GPUBuffer m_ReadbackWindow; // CacheMeshDisplayPipelines - std::map m_CachedMeshPipelines; + std::map m_CachedMeshPipelines; // CopyArrayToTex2DMS & CopyTex2DMSToArray VkDescriptorPool m_ArrayMSDescriptorPool; diff --git a/renderdoc/driver/vulkan/vk_rendermesh.cpp b/renderdoc/driver/vulkan/vk_rendermesh.cpp index 64be5fb42..9e9df8439 100644 --- a/renderdoc/driver/vulkan/vk_rendermesh.cpp +++ b/renderdoc/driver/vulkan/vk_rendermesh.cpp @@ -34,9 +34,9 @@ #define VULKAN 1 #include "data/glsl/glsl_ubos_cpp.h" -MeshDisplayPipelines VulkanDebugManager::CacheMeshDisplayPipelines(VkPipelineLayout pipeLayout, - const MeshFormat &primary, - const MeshFormat &secondary) +VKMeshDisplayPipelines VulkanDebugManager::CacheMeshDisplayPipelines(VkPipelineLayout pipeLayout, + const MeshFormat &primary, + const MeshFormat &secondary) { // generate a key to look up the map uint64_t key = 0; @@ -88,10 +88,53 @@ MeshDisplayPipelines VulkanDebugManager::CacheMeshDisplayPipelines(VkPipelineLay key |= 1ULL << bit; bit++; + // where possible we shift binding offset into attribute offset. Consider: + // + // ----------+-----------+----------------+----------------+----- + // offset=x | attribute | stride padding | next attribute | ... + // ----------+-----------+----------------+----------------+----- + // + // Unfortunately in vulkan the whole attribute+stride padding element must be in range of the + // buffer to not be clipped as out of bounds. In the case where we have a super-tightly packed + // buffer then since we are adding together binding offset + attribute offset in our + // vertexByteOffset then the final element's stride offset will poke outside of the buffer. + // + // The only feasible way to prevent against this is to set the attribute's offset to the size of + // the stride padding, and subtract that from the offset we use to bind. In effect making it + // look like this: + // + // ----------+----------------+-----------+----------------+----------------+----- + // offset=x | stride padding | attribute | stride padding | next attribute | ... + // ----------+----------------+-----------+----------------+----------------+----- + // + // That way the final attribute can be tightly packed into the buffer. + // HOWEVER this is only possible assuming that the base offset is actually larger than stride + // padding. We assume that in the cases where this workaround is needed that it is, because it + // includes the original attribute offset that we are essentially reverse-engineering here, so + // since we don't want to put the whole offset into the cache key we just put a single bit of + // 'offset bigger than stride padding' + + uint32_t primaryStridePadding = primary.vertexByteStride - GetByteSize(1, 1, 1, primaryFmt, 0); + + if(primary.vertexByteOffset > primaryStridePadding) + key |= 1ULL << bit; + bit++; + + uint32_t secondaryStridePadding = 0; + + if(secondary.vertexResourceId != ResourceId()) + { + secondaryStridePadding = secondary.vertexByteStride - GetByteSize(1, 1, 1, secondaryFmt, 0); + + if(secondary.vertexByteOffset > secondaryStridePadding) + key |= 1ULL << bit; + } + bit++; + // only 64 bits, make sure they all fit RDCASSERT(bit < 64); - MeshDisplayPipelines &cache = m_CachedMeshPipelines[key]; + VKMeshDisplayPipelines &cache = m_CachedMeshPipelines[key]; if(cache.pipes[(uint32_t)SolidShade::NoSolid] != VK_NULL_HANDLE) return cache; @@ -123,6 +166,18 @@ MeshDisplayPipelines VulkanDebugManager::CacheMeshDisplayPipelines(VkPipelineLay }, }; + if(primary.vertexByteOffset > primaryStridePadding) + { + cache.primaryStridePadding = primaryStridePadding; + vertAttrs[0].offset += primaryStridePadding; + } + + if(secondary.vertexByteOffset > secondaryStridePadding) + { + cache.secondaryStridePadding = secondaryStridePadding; + vertAttrs[1].offset += secondaryStridePadding; + } + VkPipelineVertexInputStateCreateInfo vi = { VK_STRUCTURE_TYPE_PIPELINE_VERTEX_INPUT_STATE_CREATE_INFO, NULL, 0, 1, binds, 2, vertAttrs, }; @@ -311,13 +366,13 @@ MeshDisplayPipelines VulkanDebugManager::CacheMeshDisplayPipelines(VkPipelineLay } vkr = vt->CreateGraphicsPipelines(Unwrap(m_Device), VK_NULL_HANDLE, 1, &pipeInfo, NULL, - &cache.pipes[MeshDisplayPipelines::ePipe_Wire]); + &cache.pipes[VKMeshDisplayPipelines::ePipe_Wire]); RDCASSERTEQUAL(vkr, VK_SUCCESS); ds.depthTestEnable = true; vkr = vt->CreateGraphicsPipelines(Unwrap(m_Device), VK_NULL_HANDLE, 1, &pipeInfo, NULL, - &cache.pipes[MeshDisplayPipelines::ePipe_WireDepth]); + &cache.pipes[VKMeshDisplayPipelines::ePipe_WireDepth]); RDCASSERTEQUAL(vkr, VK_SUCCESS); // solid shading pipeline @@ -325,13 +380,13 @@ MeshDisplayPipelines VulkanDebugManager::CacheMeshDisplayPipelines(VkPipelineLay ds.depthTestEnable = false; vkr = vt->CreateGraphicsPipelines(Unwrap(m_Device), VK_NULL_HANDLE, 1, &pipeInfo, NULL, - &cache.pipes[MeshDisplayPipelines::ePipe_Solid]); + &cache.pipes[VKMeshDisplayPipelines::ePipe_Solid]); RDCASSERTEQUAL(vkr, VK_SUCCESS); ds.depthTestEnable = true; vkr = vt->CreateGraphicsPipelines(Unwrap(m_Device), VK_NULL_HANDLE, 1, &pipeInfo, NULL, - &cache.pipes[MeshDisplayPipelines::ePipe_SolidDepth]); + &cache.pipes[VKMeshDisplayPipelines::ePipe_SolidDepth]); RDCASSERTEQUAL(vkr, VK_SUCCESS); if(secondary.vertexResourceId != ResourceId()) @@ -344,7 +399,7 @@ MeshDisplayPipelines VulkanDebugManager::CacheMeshDisplayPipelines(VkPipelineLay vi.vertexBindingDescriptionCount = 2; vkr = vt->CreateGraphicsPipelines(Unwrap(m_Device), VK_NULL_HANDLE, 1, &pipeInfo, NULL, - &cache.pipes[MeshDisplayPipelines::ePipe_Secondary]); + &cache.pipes[VKMeshDisplayPipelines::ePipe_Secondary]); RDCASSERTEQUAL(vkr, VK_SUCCESS); } @@ -359,11 +414,11 @@ MeshDisplayPipelines VulkanDebugManager::CacheMeshDisplayPipelines(VkPipelineLay if(stages[2].module != VK_NULL_HANDLE && ia.topology != VK_PRIMITIVE_TOPOLOGY_POINT_LIST) { vkr = vt->CreateGraphicsPipelines(Unwrap(m_Device), VK_NULL_HANDLE, 1, &pipeInfo, NULL, - &cache.pipes[MeshDisplayPipelines::ePipe_Lit]); + &cache.pipes[VKMeshDisplayPipelines::ePipe_Lit]); RDCASSERTEQUAL(vkr, VK_SUCCESS); } - for(uint32_t i = 0; i < MeshDisplayPipelines::ePipe_Count; i++) + for(uint32_t i = 0; i < VKMeshDisplayPipelines::ePipe_Count; i++) if(cache.pipes[i] != VK_NULL_HANDLE) m_pDriver->GetResourceManager()->WrapResource(Unwrap(m_Device), cache.pipes[i]); @@ -496,7 +551,7 @@ void VulkanReplay::RenderMesh(uint32_t eventId, const rdcarray &seco vt->CmdSetViewport(Unwrap(cmd), 0, 1, &viewport); } - MeshDisplayPipelines secondaryCache = GetDebugManager()->CacheMeshDisplayPipelines( + VKMeshDisplayPipelines secondaryCache = GetDebugManager()->CacheMeshDisplayPipelines( m_MeshRender.PipeLayout, secondaryDraws[i], secondaryDraws[i]); vt->CmdBindDescriptorSets(Unwrap(cmd), VK_PIPELINE_BIND_POINT_GRAPHICS, @@ -504,12 +559,12 @@ void VulkanReplay::RenderMesh(uint32_t eventId, const rdcarray &seco UnwrapPtr(m_MeshRender.DescSet), 1, &uboOffs); vt->CmdBindPipeline(Unwrap(cmd), VK_PIPELINE_BIND_POINT_GRAPHICS, - Unwrap(secondaryCache.pipes[MeshDisplayPipelines::ePipe_WireDepth])); + Unwrap(secondaryCache.pipes[VKMeshDisplayPipelines::ePipe_WireDepth])); VkBuffer vb = m_pDriver->GetResourceManager()->GetCurrentHandle(fmt.vertexResourceId); - VkDeviceSize offs = fmt.vertexByteOffset; + VkDeviceSize offs = fmt.vertexByteOffset - secondaryCache.primaryStridePadding; vt->CmdBindVertexBuffers(Unwrap(cmd), 0, 1, UnwrapPtr(vb), &offs); if(fmt.indexByteStride) @@ -556,7 +611,7 @@ void VulkanReplay::RenderMesh(uint32_t eventId, const rdcarray &seco } } - MeshDisplayPipelines cache = GetDebugManager()->CacheMeshDisplayPipelines( + VKMeshDisplayPipelines cache = GetDebugManager()->CacheMeshDisplayPipelines( m_MeshRender.PipeLayout, cfg.position, cfg.second); if(cfg.position.vertexResourceId != ResourceId()) @@ -571,6 +626,8 @@ void VulkanReplay::RenderMesh(uint32_t eventId, const rdcarray &seco if(cfg.position.instanced && cfg.position.instStepRate) offs += cfg.position.vertexByteStride * (cfg.curInstance / cfg.position.instStepRate); + offs -= cache.primaryStridePadding; + vt->CmdBindVertexBuffers(Unwrap(cmd), 0, 1, UnwrapPtr(vb), &offs); } @@ -592,6 +649,8 @@ void VulkanReplay::RenderMesh(uint32_t eventId, const rdcarray &seco if(cfg.second.instanced && cfg.second.instStepRate) offs += cfg.second.vertexByteStride * (cfg.curInstance / cfg.second.instStepRate); + offs -= cache.secondaryStridePadding; + vt->CmdBindVertexBuffers(Unwrap(cmd), 1, 1, UnwrapPtr(vb), &offs); } @@ -602,19 +661,21 @@ void VulkanReplay::RenderMesh(uint32_t eventId, const rdcarray &seco switch(solidShadeMode) { default: - case SolidShade::Solid: pipe = cache.pipes[MeshDisplayPipelines::ePipe_SolidDepth]; break; + case SolidShade::Solid: pipe = cache.pipes[VKMeshDisplayPipelines::ePipe_SolidDepth]; break; case SolidShade::Lit: - pipe = cache.pipes[MeshDisplayPipelines::ePipe_Lit]; + pipe = cache.pipes[VKMeshDisplayPipelines::ePipe_Lit]; // point list topologies don't have lighting obvious, just render them as solid if(pipe == VK_NULL_HANDLE) - pipe = cache.pipes[MeshDisplayPipelines::ePipe_SolidDepth]; + pipe = cache.pipes[VKMeshDisplayPipelines::ePipe_SolidDepth]; + break; + case SolidShade::Secondary: + pipe = cache.pipes[VKMeshDisplayPipelines::ePipe_Secondary]; break; - case SolidShade::Secondary: pipe = cache.pipes[MeshDisplayPipelines::ePipe_Secondary]; break; } // can't support lit rendering without the pipeline - maybe geometry shader wasn't supported. if(solidShadeMode == SolidShade::Lit && pipe == VK_NULL_HANDLE) - pipe = cache.pipes[MeshDisplayPipelines::ePipe_SolidDepth]; + pipe = cache.pipes[VKMeshDisplayPipelines::ePipe_SolidDepth]; uint32_t uboOffs = 0; MeshUBOData *data = (MeshUBOData *)m_MeshRender.UBO.Map(&uboOffs); @@ -687,7 +748,7 @@ void VulkanReplay::RenderMesh(uint32_t eventId, const rdcarray &seco UnwrapPtr(m_MeshRender.DescSet), 1, &uboOffs); vt->CmdBindPipeline(Unwrap(cmd), VK_PIPELINE_BIND_POINT_GRAPHICS, - Unwrap(cache.pipes[MeshDisplayPipelines::ePipe_WireDepth])); + Unwrap(cache.pipes[VKMeshDisplayPipelines::ePipe_WireDepth])); if(cfg.position.indexByteStride) { @@ -776,7 +837,7 @@ void VulkanReplay::RenderMesh(uint32_t eventId, const rdcarray &seco UnwrapPtr(m_MeshRender.DescSet), 1, &uboOffs); vt->CmdBindPipeline(Unwrap(cmd), VK_PIPELINE_BIND_POINT_GRAPHICS, - Unwrap(cache.pipes[MeshDisplayPipelines::ePipe_WireDepth])); + Unwrap(cache.pipes[VKMeshDisplayPipelines::ePipe_WireDepth])); vt->CmdDraw(Unwrap(cmd), 24, 1, 0, 0); } @@ -804,7 +865,7 @@ void VulkanReplay::RenderMesh(uint32_t eventId, const rdcarray &seco UnwrapPtr(m_MeshRender.DescSet), 1, &uboOffs); vt->CmdBindPipeline(Unwrap(cmd), VK_PIPELINE_BIND_POINT_GRAPHICS, - Unwrap(cache.pipes[MeshDisplayPipelines::ePipe_Wire])); + Unwrap(cache.pipes[VKMeshDisplayPipelines::ePipe_Wire])); vt->CmdDraw(Unwrap(cmd), 2, 1, 0, 0); @@ -865,7 +926,7 @@ void VulkanReplay::RenderMesh(uint32_t eventId, const rdcarray &seco UnwrapPtr(m_MeshRender.DescSet), 1, &uboOffs); vt->CmdBindPipeline(Unwrap(cmd), VK_PIPELINE_BIND_POINT_GRAPHICS, - Unwrap(cache.pipes[MeshDisplayPipelines::ePipe_Wire])); + Unwrap(cache.pipes[VKMeshDisplayPipelines::ePipe_Wire])); vt->CmdDraw(Unwrap(cmd), 24, 1, 0, 0); } @@ -966,7 +1027,7 @@ void VulkanReplay::RenderMesh(uint32_t eventId, const rdcarray &seco UnwrapPtr(m_MeshRender.DescSet), 1, &uboOffs); vt->CmdBindPipeline(Unwrap(cmd), VK_PIPELINE_BIND_POINT_GRAPHICS, - Unwrap(cache.pipes[MeshDisplayPipelines::ePipe_Solid])); + Unwrap(cache.pipes[VKMeshDisplayPipelines::ePipe_Solid])); //////////////////////////////////////////////////////////////// // render primitives @@ -1050,7 +1111,7 @@ void VulkanReplay::RenderMesh(uint32_t eventId, const rdcarray &seco UnwrapPtr(m_MeshRender.DescSet), 1, &uboOffs); vt->CmdBindPipeline(Unwrap(cmd), VK_PIPELINE_BIND_POINT_GRAPHICS, - Unwrap(cache.pipes[MeshDisplayPipelines::ePipe_Solid])); + Unwrap(cache.pipes[VKMeshDisplayPipelines::ePipe_Solid])); { VkDeviceSize vboffs = 0;