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>
diff --git a/src/dawn_native/vulkan/BackendVk.cpp b/src/dawn_native/vulkan/BackendVk.cpp
index 1c4c1b9..7fd6462 100644
--- a/src/dawn_native/vulkan/BackendVk.cpp
+++ b/src/dawn_native/vulkan/BackendVk.cpp
@@ -52,6 +52,31 @@
# 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 @@
#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 62a5cd2..d89688c 100644
--- a/src/dawn_native/vulkan/CommandBufferVk.cpp
+++ b/src/dawn_native/vulkan/CommandBufferVk.cpp
@@ -217,10 +217,11 @@
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 b91e46f..2cc815b 100644
--- a/src/dawn_native/vulkan/RenderPassCache.cpp
+++ b/src/dawn_native/vulkan/RenderPassCache.cpp
@@ -34,6 +34,8 @@
}
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 @@
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 @@
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 @@
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 @@
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 4503e1f..29bcfd0 100644
--- a/src/dawn_native/vulkan/RenderPassCache.h
+++ b/src/dawn_native/vulkan/RenderPassCache.h
@@ -47,7 +47,8 @@
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 @@
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 556b582..d0072df 100644
--- a/src/dawn_native/vulkan/RenderPipelineVk.cpp
+++ b/src/dawn_native/vulkan/RenderPipelineVk.cpp
@@ -497,7 +497,9 @@
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 @@
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 2edf724..73519c8 100644
--- a/src/dawn_native/vulkan/TextureVk.cpp
+++ b/src/dawn_native/vulkan/TextureVk.cpp
@@ -83,6 +83,9 @@
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 @@
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 @@
}
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 @@
flags |= VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT;
}
}
+ if (usage & kReadOnlyRenderAttachment) {
+ flags |= VK_IMAGE_USAGE_DEPTH_STENCIL_ATTACHMENT_BIT;
+ }
return flags;
}
@@ -466,10 +478,17 @@
}
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 @@
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 0096dbe..97a7637 100644
--- a/src/tests/end2end/ReadOnlyDepthStencilAttachmentTests.cpp
+++ b/src/tests/end2end/ReadOnlyDepthStencilAttachmentTests.cpp
@@ -254,10 +254,10 @@
}
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()));
