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 <cwallez@chromium.org>
Reviewed-by: Austin Eng <enga@chromium.org>
Commit-Queue: Yunchao He <yunchao.he@intel.com>

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 */) {
+            if (ShouldReportDebugMessage(pCallbackData->pMessageIdName, pCallbackData->pMessage)) {
                 dawn::WarningLog() << pCallbackData->pMessage;
                 ASSERT((messageSeverity & VK_DEBUG_UTILS_MESSAGE_SEVERITY_ERROR_BIT_EXT) == 0);
-
+            }
             return VK_FALSE;
         }
 

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,
+                    query.SetDepthStencil(
+                        attachmentInfo.view->GetTexture()->GetFormat().format,
                         attachmentInfo.depthLoadOp, attachmentInfo.depthStoreOp,
-                                          attachmentInfo.stencilLoadOp,
+                        attachmentInfo.stencilLoadOp, attachmentInfo.stencilStoreOp,
-                                          attachmentInfo.stencilStoreOp);
+                        attachmentInfo.depthReadOnly || attachmentInfo.stencilReadOnly);
                 }
 
                 query.SetSampleCount(renderPass->attachmentState->GetSampleCount());

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;
             }
         }

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<ColorAttachmentIndex, kMaxColorAttachments> colorMask;
@@ -62,6 +63,7 @@ namespace dawn_native { namespace vulkan {
         wgpu::StoreOp depthStoreOp;
         wgpu::LoadOp stencilLoadOp;
         wgpu::StoreOp stencilStoreOp;
+        bool readOnlyDepthStencil;
 
         uint32_t sampleCount;
     };

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());

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
+            // Sampled | kReadOnlyRenderAttachment is the only possible multi-bit usage, if more
-            // might need additional special-casing.
+            // appear we might need additional special-casing.
-            ASSERT(usage == wgpu::TextureUsage::TextureBinding);
+            ASSERT(usage == (wgpu::TextureUsage::TextureBinding | kReadOnlyRenderAttachment));
-            return VK_IMAGE_LAYOUT_GENERAL;
+
+            // 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.
+                // The layout returned here is the one that will be used at bindgroup creation time.
-                // When that happens, the layout must be GENERAL because that's a requirement for
+                // The bindgrpup's layout must match the runtime layout of the image when it is
-                // the storage usage. We can't know at bindgroup creation time if that case will
+                // used via the bindgroup, but we don't know exactly what it will be yet. So we
-                // happen so we must prepare for the pessimistic case and always use the GENERAL
+                // have to prepare for the pessimistic case.
-                // layout.
             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

src/tests/end2end/ReadOnlyDepthStencilAttachmentTests.cpp

@@ -254,10 +254,10 @@ TEST_P(ReadOnlyStencilAttachmentTests, Test) {
 }
 
 DAWN_INSTANTIATE_TEST_P(ReadOnlyDepthAttachmentTests,
-                        {D3D12Backend()},
+                        {D3D12Backend(), VulkanBackend()},
                         std::vector<wgpu::TextureFormat>(utils::kDepthFormats.begin(),
                                                          utils::kDepthFormats.end()));
 DAWN_INSTANTIATE_TEST_P(ReadOnlyStencilAttachmentTests,
-                        {D3D12Backend()},
+                        {D3D12Backend(), VulkanBackend()},
                         std::vector<wgpu::TextureFormat>(utils::kStencilFormats.begin(),
                                                          utils::kStencilFormats.end()));