From 3bce54bb3dad7ec6e352a18213127a0b7b1c4d3e Mon Sep 17 00:00:00 2001 From: baldurk Date: Sun, 13 Dec 2015 15:37:37 +0100 Subject: [PATCH] Set access masks in image memory barriers as recommended by validation * I'm not entirely convinced this validation is correct, but this is being conservative by adding more access bits (I ignore the message that complains about extra bits being set). Worst case, more access bits are set than necessary and the barriers are overly strict. --- renderdoc/driver/vulkan/vk_common.cpp | 23 +++++++++++++++++++++++ renderdoc/driver/vulkan/vk_common.h | 3 +++ renderdoc/driver/vulkan/vk_core.cpp | 8 ++++++++ renderdoc/driver/vulkan/vk_debug.cpp | 5 +++-- renderdoc/driver/vulkan/vk_initstate.cpp | 9 ++++++++- renderdoc/driver/vulkan/vk_replay.cpp | 20 ++++++++++++++------ 6 files changed, 59 insertions(+), 9 deletions(-) diff --git a/renderdoc/driver/vulkan/vk_common.cpp b/renderdoc/driver/vulkan/vk_common.cpp index 249a8c15c..07995d9ff 100644 --- a/renderdoc/driver/vulkan/vk_common.cpp +++ b/renderdoc/driver/vulkan/vk_common.cpp @@ -26,6 +26,29 @@ #include "vk_manager.h" #include "vk_resources.h" +VkAccessFlags MakeAccessMask(VkImageLayout layout) +{ + switch(layout) + { + case VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL: + return VkAccessFlags(VK_ACCESS_COLOR_ATTACHMENT_WRITE_BIT | VK_ACCESS_COLOR_ATTACHMENT_READ_BIT); + case VK_IMAGE_LAYOUT_DEPTH_STENCIL_ATTACHMENT_OPTIMAL: + return VkAccessFlags(VK_ACCESS_DEPTH_STENCIL_ATTACHMENT_WRITE_BIT | VK_ACCESS_DEPTH_STENCIL_ATTACHMENT_READ_BIT); + case VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL: + return VkAccessFlags(VK_ACCESS_TRANSFER_WRITE_BIT); + case VK_IMAGE_LAYOUT_PREINITIALIZED: + return VkAccessFlags(VK_ACCESS_HOST_WRITE_BIT); + case VK_IMAGE_LAYOUT_DEPTH_STENCIL_READ_ONLY_OPTIMAL: + return VkAccessFlags(VK_ACCESS_DEPTH_STENCIL_ATTACHMENT_READ_BIT | VK_ACCESS_SHADER_READ_BIT); + case VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL: + return VkAccessFlags(VK_ACCESS_INPUT_ATTACHMENT_READ_BIT | VK_ACCESS_SHADER_READ_BIT); + case VK_IMAGE_LAYOUT_TRANSFER_SRC_OPTIMAL: + return VkAccessFlags(VK_ACCESS_TRANSFER_READ_BIT); + } + + return VkAccessFlags(0); +} + ResourceFormat MakeResourceFormat(VkFormat fmt) { ResourceFormat ret; diff --git a/renderdoc/driver/vulkan/vk_common.h b/renderdoc/driver/vulkan/vk_common.h index 1b9ee7883..da098e1df 100644 --- a/renderdoc/driver/vulkan/vk_common.h +++ b/renderdoc/driver/vulkan/vk_common.h @@ -51,6 +51,9 @@ VkFormat MakeVkFormat(ResourceFormat fmt); PrimitiveTopology MakePrimitiveTopology(VkPrimitiveTopology Topo, uint32_t patchControlPoints); VkPrimitiveTopology MakeVkPrimitiveTopology(PrimitiveTopology Topo); +// set conservative access bits for this image layout +VkAccessFlags MakeAccessMask(VkImageLayout layout); + // structure for casting to easily iterate and template specialising Serialise struct VkGenericStruct { diff --git a/renderdoc/driver/vulkan/vk_core.cpp b/renderdoc/driver/vulkan/vk_core.cpp index 03c183694..d69ca00a6 100644 --- a/renderdoc/driver/vulkan/vk_core.cpp +++ b/renderdoc/driver/vulkan/vk_core.cpp @@ -589,7 +589,11 @@ bool WrappedVulkan::Serialise_BeginCaptureFrame(bool applyInitialState) { vector barriers; for(size_t i=0; i < imgBarriers.size(); i++) + { + imgBarriers[i].srcAccessMask = MakeAccessMask(imgBarriers[i].oldLayout); + imgBarriers[i].dstAccessMask = MakeAccessMask(imgBarriers[i].newLayout); barriers.push_back(&imgBarriers[i]); + } ObjDisp(cmd)->CmdPipelineBarrier(Unwrap(cmd), src_stages, dest_stages, false, (uint32_t)imgBarriers.size(), (const void *const *)&barriers[0]); } @@ -1783,6 +1787,10 @@ VkBool32 WrappedVulkan::DebugCallback( const char* pLayerPrefix, const char* pMsg) { + // msgCode isn't fine grained enough to ignore just this one type of message + if(pLayerPrefix[0] == 'D' && pLayerPrefix[1] == 'S' && string(pMsg).find("Additional bits in accessMask") != string::npos) + return false; + RDCWARN("[%s] %s", pLayerPrefix, pMsg); return false; } diff --git a/renderdoc/driver/vulkan/vk_debug.cpp b/renderdoc/driver/vulkan/vk_debug.cpp index 3d0193420..80575774b 100644 --- a/renderdoc/driver/vulkan/vk_debug.cpp +++ b/renderdoc/driver/vulkan/vk_debug.cpp @@ -1010,6 +1010,7 @@ VulkanDebugManager::VulkanDebugManager(WrappedVulkan *driver, VkDevice dev) }; barrier.srcAccessMask = VK_ACCESS_HOST_WRITE_BIT|VK_ACCESS_TRANSFER_WRITE_BIT; + barrier.dstAccessMask = VK_ACCESS_SHADER_READ_BIT; void *barrierptr = (void *)&barrier; @@ -1108,7 +1109,7 @@ VulkanDebugManager::VulkanDebugManager(WrappedVulkan *driver, VkDevice dev) VkImageMemoryBarrier barrier = { VK_STRUCTURE_TYPE_IMAGE_MEMORY_BARRIER, NULL, - 0, 0, + 0, VK_ACCESS_COLOR_ATTACHMENT_WRITE_BIT, VK_IMAGE_LAYOUT_UNDEFINED, VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL, VK_QUEUE_FAMILY_IGNORED, VK_QUEUE_FAMILY_IGNORED, Unwrap(m_PickPixelImage), @@ -2074,7 +2075,7 @@ ResourceId VulkanDebugManager::RenderOverlay(ResourceId texid, TextureDisplayOve VkImageMemoryBarrier barrier = { VK_STRUCTURE_TYPE_IMAGE_MEMORY_BARRIER, NULL, - 0, 0, + 0, VK_ACCESS_COLOR_ATTACHMENT_WRITE_BIT, VK_IMAGE_LAYOUT_UNDEFINED, VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL, VK_QUEUE_FAMILY_IGNORED, VK_QUEUE_FAMILY_IGNORED, Unwrap(m_OverlayImage), diff --git a/renderdoc/driver/vulkan/vk_initstate.cpp b/renderdoc/driver/vulkan/vk_initstate.cpp index 282d52acf..f5b0b8925 100644 --- a/renderdoc/driver/vulkan/vk_initstate.cpp +++ b/renderdoc/driver/vulkan/vk_initstate.cpp @@ -1553,6 +1553,7 @@ bool WrappedVulkan::Serialise_InitialState(WrappedVkRes *res) // first we update layout from undefined to destination optimal, for the copy from the buffer srcimBarrier.oldLayout = VK_IMAGE_LAYOUT_UNDEFINED; srcimBarrier.newLayout = VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL; + srcimBarrier.dstAccessMask = VK_ACCESS_TRANSFER_WRITE_BIT; ObjDisp(d)->CmdPipelineBarrier(Unwrap(cmd), VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, false, 1, &barrier); @@ -1562,6 +1563,8 @@ bool WrappedVulkan::Serialise_InitialState(WrappedVkRes *res) // state image, to the live image. srcimBarrier.oldLayout = srcimBarrier.newLayout; srcimBarrier.newLayout = VK_IMAGE_LAYOUT_TRANSFER_SRC_OPTIMAL; + srcimBarrier.srcAccessMask = VK_ACCESS_TRANSFER_WRITE_BIT; + srcimBarrier.dstAccessMask = VK_ACCESS_TRANSFER_READ_BIT; ObjDisp(d)->CmdPipelineBarrier(Unwrap(cmd), VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, false, 1, &barrier); @@ -1790,7 +1793,7 @@ void WrappedVulkan::Apply_InitialState(WrappedVkRes *live, VulkanResourceManager // finish any pending work before clear barrier.srcAccessMask = VK_ACCESS_ALL_WRITE_BITS; // clear completes before subsequent operations - barrier.dstAccessMask = VK_ACCESS_TRANSFER_READ_BIT; + barrier.dstAccessMask = VK_ACCESS_TRANSFER_WRITE_BIT; void *barrierptr = (void *)&barrier; @@ -1814,6 +1817,7 @@ void WrappedVulkan::Apply_InitialState(WrappedVkRes *live, VulkanResourceManager for (int si = 0; si < m_ImageLayouts[id].subresourceStates.size(); si++) { barrier.newLayout = m_ImageLayouts[id].subresourceStates[si].newLayout; + barrier.dstAccessMask |= MakeAccessMask(barrier.newLayout); ObjDisp(cmd)->CmdPipelineBarrier(Unwrap(cmd), VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, false, 1, &barrierptr); } @@ -1915,6 +1919,8 @@ void WrappedVulkan::Apply_InitialState(WrappedVkRes *live, VulkanResourceManager // first update the live image layout into destination optimal (the initial state // image is always and permanently in source optimal already). dstimBarrier.newLayout = VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL; + dstimBarrier.srcAccessMask = VK_ACCESS_ALL_WRITE_BITS; + dstimBarrier.dstAccessMask = VK_ACCESS_TRANSFER_WRITE_BIT; void *barrier = (void *)&dstimBarrier; @@ -1939,6 +1945,7 @@ void WrappedVulkan::Apply_InitialState(WrappedVkRes *live, VulkanResourceManager for (int si = 0; si < m_ImageLayouts[id].subresourceStates.size(); si++) { dstimBarrier.newLayout = m_ImageLayouts[id].subresourceStates[si].newLayout; + dstimBarrier.dstAccessMask |= MakeAccessMask(dstimBarrier.newLayout); ObjDisp(cmd)->CmdPipelineBarrier(Unwrap(cmd), VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, false, 1, &barrier); } diff --git a/renderdoc/driver/vulkan/vk_replay.cpp b/renderdoc/driver/vulkan/vk_replay.cpp index dd35a4b7e..f9d35d217 100644 --- a/renderdoc/driver/vulkan/vk_replay.cpp +++ b/renderdoc/driver/vulkan/vk_replay.cpp @@ -63,6 +63,7 @@ VulkanReplay::OutputWindow::OutputWindow() : wnd(NULL_WND_HANDLE), width(0), hei t.subresourceRange.aspectMask = VK_IMAGE_ASPECT_DEPTH_BIT; depthBarrier = t; + depthBarrier.srcAccessMask = depthBarrier.dstAccessMask = VK_ACCESS_DEPTH_STENCIL_ATTACHMENT_WRITE_BIT; } void VulkanReplay::OutputWindow::SetCol(VkDeviceMemory mem, VkImage img) @@ -795,9 +796,7 @@ void VulkanReplay::PickPixel(ResourceId texture, uint32_t x, uint32_t y, uint32_ void *barrier = (void *)&pickimBarrier; vt->CmdPipelineBarrier(Unwrap(cmd), VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, false, 1, &barrier); pickimBarrier.oldLayout = pickimBarrier.newLayout; - - pickimBarrier.srcAccessMask = 0; - pickimBarrier.dstAccessMask = 0; + pickimBarrier.srcAccessMask = pickimBarrier.dstAccessMask; // do copy VkBufferImageCopy region = { @@ -810,6 +809,7 @@ void VulkanReplay::PickPixel(ResourceId texture, uint32_t x, uint32_t y, uint32_ // update image layout back to color attachment pickimBarrier.newLayout = VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL; + pickimBarrier.dstAccessMask = VK_ACCESS_COLOR_ATTACHMENT_WRITE_BIT; vt->CmdPipelineBarrier(Unwrap(cmd), VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, false, 1, &barrier); vt->EndCommandBuffer(Unwrap(cmd)); @@ -1048,9 +1048,7 @@ bool VulkanReplay::RenderTextureInternal(TextureDisplay cfg, VkRenderPassBeginIn } srcimBarrier.oldLayout = srcimBarrier.newLayout; - - srcimBarrier.srcAccessMask = 0; - srcimBarrier.dstAccessMask = 0; + srcimBarrier.srcAccessMask = srcimBarrier.dstAccessMask; { vt->CmdBeginRenderPass(Unwrap(cmd), &rpbegin, VK_SUBPASS_CONTENTS_INLINE); @@ -1074,6 +1072,7 @@ bool VulkanReplay::RenderTextureInternal(TextureDisplay cfg, VkRenderPassBeginIn for (int si = 0; si < layouts.subresourceStates.size(); si++) { srcimBarrier.newLayout = layouts.subresourceStates[si].newLayout; + srcimBarrier.dstAccessMask = MakeAccessMask(srcimBarrier.newLayout); vt->CmdPipelineBarrier(Unwrap(cmd), VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, false, 1, &barrier); } @@ -2332,13 +2331,17 @@ void VulkanReplay::BindOutputWindow(uint64_t id, bool depth) outw.depthBarrier.newLayout = VK_IMAGE_LAYOUT_DEPTH_STENCIL_ATTACHMENT_OPTIMAL; outw.bbBarrier.newLayout = VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL; + outw.bbBarrier.dstAccessMask = VK_ACCESS_COLOR_ATTACHMENT_WRITE_BIT; outw.colBarrier[outw.curidx].newLayout = VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL; + outw.colBarrier[outw.curidx].dstAccessMask = VK_ACCESS_TRANSFER_WRITE_BIT; vt->CmdPipelineBarrier(Unwrap(cmd), VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, false, depth ? 3 : 2, barrier); outw.depthBarrier.oldLayout = outw.depthBarrier.newLayout; outw.bbBarrier.oldLayout = outw.bbBarrier.newLayout; + outw.bbBarrier.srcAccessMask = outw.bbBarrier.dstAccessMask; outw.colBarrier[outw.curidx].oldLayout = outw.colBarrier[outw.curidx].newLayout; + outw.colBarrier[outw.curidx].srcAccessMask = outw.colBarrier[outw.curidx].dstAccessMask; vt->EndCommandBuffer(Unwrap(cmd)); } @@ -2441,6 +2444,8 @@ void VulkanReplay::FlipOutputWindow(uint64_t id) else vt->CmdCopyImage(Unwrap(cmd), Unwrap(outw.bb), VK_IMAGE_LAYOUT_TRANSFER_SRC_OPTIMAL, Unwrap(outw.colimg[outw.curidx]), VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, 1, &cpy); + outw.bbBarrier.srcAccessMask = VK_ACCESS_TRANSFER_READ_BIT; + outw.bbBarrier.dstAccessMask = VK_ACCESS_COLOR_ATTACHMENT_WRITE_BIT; outw.bbBarrier.newLayout = VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL; outw.colBarrier[outw.curidx].newLayout = VK_IMAGE_LAYOUT_PRESENT_SRC_KHR; @@ -2451,6 +2456,7 @@ void VulkanReplay::FlipOutputWindow(uint64_t id) vt->CmdPipelineBarrier(Unwrap(cmd), VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, false, 2, barrier); outw.bbBarrier.oldLayout = outw.bbBarrier.newLayout; + outw.bbBarrier.srcAccessMask = outw.bbBarrier.dstAccessMask; outw.colBarrier[outw.curidx].oldLayout = outw.colBarrier[outw.curidx].newLayout; outw.colBarrier[outw.curidx].srcAccessMask = 0; @@ -3376,6 +3382,7 @@ bool VulkanReplay::GetMinMax(ResourceId texid, uint32_t sliceFace, uint32_t mip, for (int si = 0; si < layouts.subresourceStates.size(); si++) { srcimBarrier.newLayout = layouts.subresourceStates[si].newLayout; + srcimBarrier.dstAccessMask = MakeAccessMask(srcimBarrier.newLayout); vt->CmdPipelineBarrier(Unwrap(cmd), VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, false, 1, &barrier); } @@ -3597,6 +3604,7 @@ bool VulkanReplay::GetHistogram(ResourceId texid, uint32_t sliceFace, uint32_t m for (int si = 0; si < layouts.subresourceStates.size(); si++) { srcimBarrier.newLayout = layouts.subresourceStates[si].newLayout; + srcimBarrier.dstAccessMask = MakeAccessMask(srcimBarrier.newLayout); vt->CmdPipelineBarrier(Unwrap(cmd), VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, false, 1, &barrier); }