From 15fec68a4f73051c8e25da68dc3c8eefeec72941 Mon Sep 17 00:00:00 2001 From: Yunchao He Date: Thu, 9 Dec 2021 21:32:28 +0000 Subject: [PATCH] Implement readonly depth/stencil attachment on Vulkan It requires to revise quite a few flags in order to implement readonly depth/stencil attachment on Vulkan. For example: - VkAccessFlags, - VkPipelineStageFlags, - VkImageUsageFlags, - and the most important: VkImageLayout. These revised flags need to be applied to many Vulkan objects. For examples: - VkImageMemoryBarriers, - IMAGE_LAYOUT revisions in descriptor set (bindings), render pass, and subpass, - and loadOp/storeOp revisions in render pass and subpass. This change also does some workarounds in order to make Vulkan validation layers happy. For example: - use DEPTH_STENCIL_READ_ONLY image layout for binding a depth/stencil texture, - add DEPTH_STENCIL_ATTACHMENT_BIT image usage for binding a depth/stencil texture. Note that STORE_OP_STORE is used for depth/stencil's storeOp for readonly attachment. It leads to Vulkan validation error. This change igores that error and let it proceeds and it works well. Bug: dawn:485 Change-Id: Ie247c9cbffbd837984b0933a905632ab5ad8862d Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/70280 Reviewed-by: Corentin Wallez Reviewed-by: Austin Eng Commit-Queue: Yunchao He --- src/dawn_native/vulkan/BackendVk.cpp | 43 ++++++++++++++-- src/dawn_native/vulkan/CommandBufferVk.cpp | 9 ++-- src/dawn_native/vulkan/RenderPassCache.cpp | 24 ++++++--- src/dawn_native/vulkan/RenderPassCache.h | 4 +- src/dawn_native/vulkan/RenderPipelineVk.cpp | 6 ++- src/dawn_native/vulkan/TextureVk.cpp | 49 ++++++++++++++----- .../ReadOnlyDepthStencilAttachmentTests.cpp | 4 +- 7 files changed, 109 insertions(+), 30 deletions(-) diff --git a/src/dawn_native/vulkan/BackendVk.cpp b/src/dawn_native/vulkan/BackendVk.cpp index 1c4c1b9070..7fd6462203 100644 --- a/src/dawn_native/vulkan/BackendVk.cpp +++ b/src/dawn_native/vulkan/BackendVk.cpp @@ -52,6 +52,31 @@ constexpr char kVulkanLibName[] = "libvulkan.so"; # error "Unimplemented Vulkan backend platform" #endif +struct SkippedMessage { + const char* messageId; + const char* messageContents; +}; + +// Array of Validation error/warning messages that will be ignored, should include bugID +constexpr SkippedMessage kSkippedMessages[] = { + // These errors are generated when simultaneously using a read-only depth/stencil attachment as + // a texture binding. This is valid Vulkan. + // + // When storeOp=NONE is not present, Dawn uses storeOp=STORE, but Vulkan validation layer + // considers the image read-only and produces a hazard. Dawn can't rely on storeOp=NONE and + // so this is not expected to be worked around. + // See http://crbug.com/dawn/1225 for more details. + {"SYNC-HAZARD-WRITE_AFTER_READ", + "depth aspect during store with storeOp VK_ATTACHMENT_STORE_OP_STORE. Access info (usage: " + "SYNC_LATE_FRAGMENT_TESTS_DEPTH_STENCIL_ATTACHMENT_WRITE, prior_usage: " + "SYNC_FRAGMENT_SHADER_SHADER_STORAGE_READ, read_barriers: VK_PIPELINE_STAGE_2_NONE_KHR, "}, + + {"SYNC-HAZARD-WRITE_AFTER_READ", + "stencil aspect during store with stencilStoreOp VK_ATTACHMENT_STORE_OP_STORE. Access info " + "(usage: SYNC_LATE_FRAGMENT_TESTS_DEPTH_STENCIL_ATTACHMENT_WRITE, prior_usage: " + "SYNC_FRAGMENT_SHADER_SHADER_STORAGE_READ, read_barriers: VK_PIPELINE_STAGE_2_NONE_KHR, "}, +}; + namespace dawn_native { namespace vulkan { namespace { @@ -63,14 +88,26 @@ namespace dawn_native { namespace vulkan { #endif // defined(DAWN_ENABLE_SWIFTSHADER) }; + // Suppress validation errors that are known. Returns false in that case. + bool ShouldReportDebugMessage(const char* messageId, const char* message) { + for (const SkippedMessage& msg : kSkippedMessages) { + if (strstr(messageId, msg.messageId) != nullptr && + strstr(message, msg.messageContents) != nullptr) { + return false; + } + } + return true; + } + VKAPI_ATTR VkBool32 VKAPI_CALL OnDebugUtilsCallback(VkDebugUtilsMessageSeverityFlagBitsEXT messageSeverity, VkDebugUtilsMessageTypeFlagsEXT /* messageTypes */, const VkDebugUtilsMessengerCallbackDataEXT* pCallbackData, void* /* pUserData */) { - dawn::WarningLog() << pCallbackData->pMessage; - ASSERT((messageSeverity & VK_DEBUG_UTILS_MESSAGE_SEVERITY_ERROR_BIT_EXT) == 0); - + if (ShouldReportDebugMessage(pCallbackData->pMessageIdName, pCallbackData->pMessage)) { + dawn::WarningLog() << pCallbackData->pMessage; + ASSERT((messageSeverity & VK_DEBUG_UTILS_MESSAGE_SEVERITY_ERROR_BIT_EXT) == 0); + } return VK_FALSE; } diff --git a/src/dawn_native/vulkan/CommandBufferVk.cpp b/src/dawn_native/vulkan/CommandBufferVk.cpp index 62a5cd2ce7..d89688cf45 100644 --- a/src/dawn_native/vulkan/CommandBufferVk.cpp +++ b/src/dawn_native/vulkan/CommandBufferVk.cpp @@ -217,10 +217,11 @@ namespace dawn_native { namespace vulkan { if (renderPass->attachmentState->HasDepthStencilAttachment()) { const auto& attachmentInfo = renderPass->depthStencilAttachment; - query.SetDepthStencil(attachmentInfo.view->GetTexture()->GetFormat().format, - attachmentInfo.depthLoadOp, attachmentInfo.depthStoreOp, - attachmentInfo.stencilLoadOp, - attachmentInfo.stencilStoreOp); + query.SetDepthStencil( + attachmentInfo.view->GetTexture()->GetFormat().format, + attachmentInfo.depthLoadOp, attachmentInfo.depthStoreOp, + attachmentInfo.stencilLoadOp, attachmentInfo.stencilStoreOp, + attachmentInfo.depthReadOnly || attachmentInfo.stencilReadOnly); } query.SetSampleCount(renderPass->attachmentState->GetSampleCount()); diff --git a/src/dawn_native/vulkan/RenderPassCache.cpp b/src/dawn_native/vulkan/RenderPassCache.cpp index b91e46fb97..2cc815b2d1 100644 --- a/src/dawn_native/vulkan/RenderPassCache.cpp +++ b/src/dawn_native/vulkan/RenderPassCache.cpp @@ -34,6 +34,8 @@ namespace dawn_native { namespace vulkan { } VkAttachmentStoreOp VulkanAttachmentStoreOp(wgpu::StoreOp op) { + // TODO(crbug.com/dawn/485): return STORE_OP_STORE_NONE_QCOM if the device has required + // extension. switch (op) { case wgpu::StoreOp::Store: return VK_ATTACHMENT_STORE_OP_STORE; @@ -62,13 +64,15 @@ namespace dawn_native { namespace vulkan { wgpu::LoadOp depthLoadOpIn, wgpu::StoreOp depthStoreOpIn, wgpu::LoadOp stencilLoadOpIn, - wgpu::StoreOp stencilStoreOpIn) { + wgpu::StoreOp stencilStoreOpIn, + bool readOnly) { hasDepthStencil = true; depthStencilFormat = format; depthLoadOp = depthLoadOpIn; depthStoreOp = depthStoreOpIn; stencilLoadOp = stencilLoadOpIn; stencilStoreOp = stencilStoreOpIn; + readOnlyDepthStencil = readOnly; } void RenderPassCacheQuery::SetSampleCount(uint32_t sampleCount) { @@ -144,17 +148,23 @@ namespace dawn_native { namespace vulkan { depthStencilAttachment = &depthStencilAttachmentRef; depthStencilAttachmentRef.attachment = attachmentCount; - depthStencilAttachmentRef.layout = VK_IMAGE_LAYOUT_DEPTH_STENCIL_ATTACHMENT_OPTIMAL; + depthStencilAttachmentRef.layout = + query.readOnlyDepthStencil ? VK_IMAGE_LAYOUT_DEPTH_STENCIL_READ_ONLY_OPTIMAL + : VK_IMAGE_LAYOUT_DEPTH_STENCIL_ATTACHMENT_OPTIMAL; attachmentDesc.flags = 0; attachmentDesc.format = VulkanImageFormat(mDevice, query.depthStencilFormat); attachmentDesc.samples = vkSampleCount; + attachmentDesc.loadOp = VulkanAttachmentLoadOp(query.depthLoadOp); attachmentDesc.storeOp = VulkanAttachmentStoreOp(query.depthStoreOp); attachmentDesc.stencilLoadOp = VulkanAttachmentLoadOp(query.stencilLoadOp); attachmentDesc.stencilStoreOp = VulkanAttachmentStoreOp(query.stencilStoreOp); - attachmentDesc.initialLayout = VK_IMAGE_LAYOUT_DEPTH_STENCIL_ATTACHMENT_OPTIMAL; - attachmentDesc.finalLayout = VK_IMAGE_LAYOUT_DEPTH_STENCIL_ATTACHMENT_OPTIMAL; + + // There is only one subpass, so it is safe to set both initialLayout and finalLayout to + // the only subpass's layout. + attachmentDesc.initialLayout = depthStencilAttachmentRef.layout; + attachmentDesc.finalLayout = depthStencilAttachmentRef.layout; ++attachmentCount; } @@ -235,7 +245,8 @@ namespace dawn_native { namespace vulkan { HashCombine(&hash, query.hasDepthStencil); if (query.hasDepthStencil) { - HashCombine(&hash, query.depthStencilFormat, query.depthLoadOp, query.stencilLoadOp); + HashCombine(&hash, query.depthStencilFormat, query.depthLoadOp, query.stencilLoadOp, + query.readOnlyDepthStencil); } HashCombine(&hash, query.sampleCount); @@ -270,7 +281,8 @@ namespace dawn_native { namespace vulkan { if (a.hasDepthStencil) { if ((a.depthStencilFormat != b.depthStencilFormat) || - (a.depthLoadOp != b.depthLoadOp) || (a.stencilLoadOp != b.stencilLoadOp)) { + (a.depthLoadOp != b.depthLoadOp) || (a.stencilLoadOp != b.stencilLoadOp) || + (a.readOnlyDepthStencil != b.readOnlyDepthStencil)) { return false; } } diff --git a/src/dawn_native/vulkan/RenderPassCache.h b/src/dawn_native/vulkan/RenderPassCache.h index 4503e1fe5e..29bcfd0f4b 100644 --- a/src/dawn_native/vulkan/RenderPassCache.h +++ b/src/dawn_native/vulkan/RenderPassCache.h @@ -47,7 +47,8 @@ namespace dawn_native { namespace vulkan { wgpu::LoadOp depthLoadOp, wgpu::StoreOp depthStoreOp, wgpu::LoadOp stencilLoadOp, - wgpu::StoreOp stencilStoreOp); + wgpu::StoreOp stencilStoreOp, + bool readOnly); void SetSampleCount(uint32_t sampleCount); ityp::bitset colorMask; @@ -62,6 +63,7 @@ namespace dawn_native { namespace vulkan { wgpu::StoreOp depthStoreOp; wgpu::LoadOp stencilLoadOp; wgpu::StoreOp stencilStoreOp; + bool readOnlyDepthStencil; uint32_t sampleCount; }; diff --git a/src/dawn_native/vulkan/RenderPipelineVk.cpp b/src/dawn_native/vulkan/RenderPipelineVk.cpp index 556b582b52..d0072dfb5d 100644 --- a/src/dawn_native/vulkan/RenderPipelineVk.cpp +++ b/src/dawn_native/vulkan/RenderPipelineVk.cpp @@ -497,7 +497,9 @@ namespace dawn_native { namespace vulkan { dynamic.pDynamicStates = dynamicStates; // Get a VkRenderPass that matches the attachment formats for this pipeline, load/store ops - // don't matter so set them all to LoadOp::Load / StoreOp::Store + // don't matter so set them all to LoadOp::Load / StoreOp::Store. Whether the render pass + // has resolve target and whether depth/stencil attachment is read-only also don't matter, + // so set them both to false. VkRenderPass renderPass = VK_NULL_HANDLE; { RenderPassCacheQuery query; @@ -510,7 +512,7 @@ namespace dawn_native { namespace vulkan { if (HasDepthStencilAttachment()) { query.SetDepthStencil(GetDepthStencilFormat(), wgpu::LoadOp::Load, wgpu::StoreOp::Store, wgpu::LoadOp::Load, - wgpu::StoreOp::Store); + wgpu::StoreOp::Store, false); } query.SetSampleCount(GetSampleCount()); diff --git a/src/dawn_native/vulkan/TextureVk.cpp b/src/dawn_native/vulkan/TextureVk.cpp index 2edf724b13..73519c8722 100644 --- a/src/dawn_native/vulkan/TextureVk.cpp +++ b/src/dawn_native/vulkan/TextureVk.cpp @@ -83,6 +83,9 @@ namespace dawn_native { namespace vulkan { VK_ACCESS_COLOR_ATTACHMENT_READ_BIT | VK_ACCESS_COLOR_ATTACHMENT_WRITE_BIT; } } + if (usage & kReadOnlyRenderAttachment) { + flags |= VK_ACCESS_DEPTH_STENCIL_ATTACHMENT_READ_BIT; + } if (usage & kPresentTextureUsage) { // The present usage is only used internally by the swapchain and is never used in // combination with other usages. @@ -129,7 +132,7 @@ namespace dawn_native { namespace vulkan { flags |= VK_PIPELINE_STAGE_FRAGMENT_SHADER_BIT | VK_PIPELINE_STAGE_COMPUTE_SHADER_BIT; } - if (usage & wgpu::TextureUsage::RenderAttachment) { + if (usage & (wgpu::TextureUsage::RenderAttachment | kReadOnlyRenderAttachment)) { if (format.HasDepthOrStencil()) { flags |= VK_PIPELINE_STAGE_EARLY_FRAGMENT_TESTS_BIT | VK_PIPELINE_STAGE_LATE_FRAGMENT_TESTS_BIT; @@ -442,6 +445,12 @@ namespace dawn_native { namespace vulkan { } if (usage & wgpu::TextureUsage::TextureBinding) { flags |= VK_IMAGE_USAGE_SAMPLED_BIT; + // If the sampled texture is a depth/stencil texture, its image layout will be set + // to DEPTH_STENCIL_READ_ONLY_OPTIMAL in order to support readonly depth/stencil + // attachment. That layout requires DEPTH_STENCIL_ATTACHMENT_BIT image usage. + if (format.HasDepthOrStencil() && format.isRenderable) { + flags |= VK_IMAGE_USAGE_DEPTH_STENCIL_ATTACHMENT_BIT; + } } if (usage & wgpu::TextureUsage::StorageBinding) { flags |= VK_IMAGE_USAGE_STORAGE_BIT; @@ -453,6 +462,9 @@ namespace dawn_native { namespace vulkan { flags |= VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT; } } + if (usage & kReadOnlyRenderAttachment) { + flags |= VK_IMAGE_USAGE_DEPTH_STENCIL_ATTACHMENT_BIT; + } return flags; } @@ -466,10 +478,17 @@ namespace dawn_native { namespace vulkan { } if (!wgpu::HasZeroOrOneBits(usage)) { - // Sampled | ReadOnlyStorage is the only possible multi-bit usage, if more appear we - // might need additional special-casing. - ASSERT(usage == wgpu::TextureUsage::TextureBinding); - return VK_IMAGE_LAYOUT_GENERAL; + // Sampled | kReadOnlyRenderAttachment is the only possible multi-bit usage, if more + // appear we might need additional special-casing. + ASSERT(usage == (wgpu::TextureUsage::TextureBinding | kReadOnlyRenderAttachment)); + + // WebGPU requires both aspects to be readonly if the attachment's format does have + // both depth and stencil aspects. Vulkan 1.0 supports readonly for both aspects too + // via DEPTH_STENCIL_READ_ONLY image layout. Vulkan 1.1 and above can support separate + // readonly for a single aspect via DEPTH_ATTACHMENT_STENCIL_READ_ONLY_OPTIMAL and + // DEPTH_READ_ONLY_STENCIL_ATTACHMENT_OPTIMAL layouts. But Vulkan 1.0 cannot support + // it, and WebGPU doesn't need that currently. + return VK_IMAGE_LAYOUT_DEPTH_STENCIL_READ_ONLY_OPTIMAL; } // Usage has a single bit so we can switch on its value directly. @@ -477,17 +496,23 @@ namespace dawn_native { namespace vulkan { case wgpu::TextureUsage::CopyDst: return VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL; - // A texture that's sampled and storage may be used as both usages in the same pass. - // When that happens, the layout must be GENERAL because that's a requirement for - // the storage usage. We can't know at bindgroup creation time if that case will - // happen so we must prepare for the pessimistic case and always use the GENERAL - // layout. + // The layout returned here is the one that will be used at bindgroup creation time. + // The bindgrpup's layout must match the runtime layout of the image when it is + // used via the bindgroup, but we don't know exactly what it will be yet. So we + // have to prepare for the pessimistic case. case wgpu::TextureUsage::TextureBinding: + // Only VK_IMAGE_LAYOUT_GENERAL can do sampling and storage access of texture at the + // same time. if (texture->GetInternalUsage() & wgpu::TextureUsage::StorageBinding) { return VK_IMAGE_LAYOUT_GENERAL; - } else { - return VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL; } + // The sampled image can be used as a readonly depth/stencil attachment at the same + // time if it is a depth/stencil renderable format, so the image layout need to be + // VK_IMAGE_LAYOUT_DEPTH_STENCIL_READ_ONLY_OPTIMAL. + if (texture->GetFormat().HasDepthOrStencil() && texture->GetFormat().isRenderable) { + return VK_IMAGE_LAYOUT_DEPTH_STENCIL_READ_ONLY_OPTIMAL; + } + return VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL; // Vulkan texture copy functions require the image to be in _one_ known layout. // Depending on whether parts of the texture have been transitioned to only CopySrc diff --git a/src/tests/end2end/ReadOnlyDepthStencilAttachmentTests.cpp b/src/tests/end2end/ReadOnlyDepthStencilAttachmentTests.cpp index 0096dbe710..97a7637441 100644 --- a/src/tests/end2end/ReadOnlyDepthStencilAttachmentTests.cpp +++ b/src/tests/end2end/ReadOnlyDepthStencilAttachmentTests.cpp @@ -254,10 +254,10 @@ TEST_P(ReadOnlyStencilAttachmentTests, Test) { } DAWN_INSTANTIATE_TEST_P(ReadOnlyDepthAttachmentTests, - {D3D12Backend()}, + {D3D12Backend(), VulkanBackend()}, std::vector(utils::kDepthFormats.begin(), utils::kDepthFormats.end())); DAWN_INSTANTIATE_TEST_P(ReadOnlyStencilAttachmentTests, - {D3D12Backend()}, + {D3D12Backend(), VulkanBackend()}, std::vector(utils::kStencilFormats.begin(), utils::kStencilFormats.end()));