Protect against aspectReadOnly of unused DS attachements aspects
Unused aspects of depth-stencil attachments that are tagged as read-only
used to leak that read-only state to backends, even if the validation
made it seems like they always match (they only need to match if the
texture has both aspects). This confused backends like Vulkan which
checked for depthReadOnly || stencilReadOnly to choose between code
paths.
Instead reyify the depthStencilAttachement descriptor in the frontend to
protect against garbage values being passed for aspects that aren't
present in the texture.
Adds a regression test, with the caveat that a failure is only shown by
having the VVL output and error in stderr due to an unrelated issue.
Fixed: dawn:1512
Change-Id: I35d5581e46909b7f41ff4c7553d60c6ac844a56b
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/101121
Reviewed-by: Loko Kung <lokokung@google.com>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
diff --git a/src/dawn/native/CommandEncoder.cpp b/src/dawn/native/CommandEncoder.cpp
index 6ffc08d..649fcb1 100644
--- a/src/dawn/native/CommandEncoder.cpp
+++ b/src/dawn/native/CommandEncoder.cpp
@@ -911,7 +911,38 @@
cmd->depthStencilAttachment.clearStencil =
descriptor->depthStencilAttachment->stencilClearValue;
}
+
+ // Copy parameters for the depth, reyifing the values when it is not present or
+ // readonly.
+ cmd->depthStencilAttachment.depthReadOnly = false;
+ cmd->depthStencilAttachment.depthLoadOp = wgpu::LoadOp::Load;
+ cmd->depthStencilAttachment.depthStoreOp = wgpu::StoreOp::Store;
+ if (view->GetFormat().HasDepth()) {
+ cmd->depthStencilAttachment.depthReadOnly =
+ descriptor->depthStencilAttachment->depthReadOnly;
+ if (!cmd->depthStencilAttachment.depthReadOnly) {
+ cmd->depthStencilAttachment.depthLoadOp =
+ descriptor->depthStencilAttachment->depthLoadOp;
+ cmd->depthStencilAttachment.depthStoreOp =
+ descriptor->depthStencilAttachment->depthStoreOp;
+ }
+ }
+
+ // Copy parameters for the stencil, reyifing the values when it is not present or
+ // readonly.
+ cmd->depthStencilAttachment.stencilReadOnly = false;
+ cmd->depthStencilAttachment.stencilLoadOp = wgpu::LoadOp::Load;
+ cmd->depthStencilAttachment.stencilStoreOp = wgpu::StoreOp::Store;
if (view->GetFormat().HasStencil()) {
+ cmd->depthStencilAttachment.stencilReadOnly =
+ descriptor->depthStencilAttachment->stencilReadOnly;
+ if (!cmd->depthStencilAttachment.stencilReadOnly) {
+ cmd->depthStencilAttachment.stencilLoadOp =
+ descriptor->depthStencilAttachment->stencilLoadOp;
+ cmd->depthStencilAttachment.stencilStoreOp =
+ descriptor->depthStencilAttachment->stencilStoreOp;
+ }
+
// GPURenderPassDepthStencilAttachment.stencilClearValue will be converted to
// the type of the stencil aspect of view by taking the same number of LSBs as
// the number of bits in the stencil aspect of one texel block of view.
@@ -921,35 +952,6 @@
cmd->depthStencilAttachment.clearStencil &= 0xFF;
}
- cmd->depthStencilAttachment.depthReadOnly =
- descriptor->depthStencilAttachment->depthReadOnly;
- cmd->depthStencilAttachment.stencilReadOnly =
- descriptor->depthStencilAttachment->stencilReadOnly;
-
- if (descriptor->depthStencilAttachment->depthReadOnly ||
- !IsSubset(Aspect::Depth,
- descriptor->depthStencilAttachment->view->GetAspects())) {
- cmd->depthStencilAttachment.depthLoadOp = wgpu::LoadOp::Load;
- cmd->depthStencilAttachment.depthStoreOp = wgpu::StoreOp::Store;
- } else {
- cmd->depthStencilAttachment.depthLoadOp =
- descriptor->depthStencilAttachment->depthLoadOp;
- cmd->depthStencilAttachment.depthStoreOp =
- descriptor->depthStencilAttachment->depthStoreOp;
- }
-
- if (descriptor->depthStencilAttachment->stencilReadOnly ||
- !IsSubset(Aspect::Stencil,
- descriptor->depthStencilAttachment->view->GetAspects())) {
- cmd->depthStencilAttachment.stencilLoadOp = wgpu::LoadOp::Load;
- cmd->depthStencilAttachment.stencilStoreOp = wgpu::StoreOp::Store;
- } else {
- cmd->depthStencilAttachment.stencilLoadOp =
- descriptor->depthStencilAttachment->stencilLoadOp;
- cmd->depthStencilAttachment.stencilStoreOp =
- descriptor->depthStencilAttachment->stencilStoreOp;
- }
-
if (IsReadOnlyDepthStencilAttachment(descriptor->depthStencilAttachment)) {
usageTracker.TextureViewUsedAs(view, kReadOnlyRenderAttachment);
} else {
diff --git a/src/dawn/tests/end2end/ReadOnlyDepthStencilAttachmentTests.cpp b/src/dawn/tests/end2end/ReadOnlyDepthStencilAttachmentTests.cpp
index f4e422c..bfd35c2 100644
--- a/src/dawn/tests/end2end/ReadOnlyDepthStencilAttachmentTests.cpp
+++ b/src/dawn/tests/end2end/ReadOnlyDepthStencilAttachmentTests.cpp
@@ -256,6 +256,37 @@
{kSize, kSize / 2});
}
+// Regression test for crbug.com/dawn/1512 where having aspectReadOnly for an unused aspect of a
+// depth-stencil texture would cause the attachment to be considered read-only, causing layout
+// mismatch issues.
+TEST_P(ReadOnlyDepthAttachmentTests, UnusedAspectWithReadOnly) {
+ wgpu::TextureFormat format = GetParam().mTextureFormat;
+ wgpu::Texture depthStencilTexture = CreateTexture(format, wgpu::TextureUsage::RenderAttachment);
+
+ utils::ComboRenderPassDescriptor passDescriptor({}, depthStencilTexture.CreateView());
+ if (utils::IsStencilOnlyFormat(format)) {
+ passDescriptor.cDepthStencilAttachmentInfo.depthReadOnly = true;
+ passDescriptor.cDepthStencilAttachmentInfo.depthLoadOp = wgpu::LoadOp::Undefined;
+ passDescriptor.cDepthStencilAttachmentInfo.depthStoreOp = wgpu::StoreOp::Undefined;
+ } else {
+ passDescriptor.cDepthStencilAttachmentInfo.depthReadOnly = false;
+ }
+ if (utils::IsDepthOnlyFormat(format)) {
+ passDescriptor.cDepthStencilAttachmentInfo.stencilReadOnly = true;
+ passDescriptor.cDepthStencilAttachmentInfo.stencilLoadOp = wgpu::LoadOp::Undefined;
+ passDescriptor.cDepthStencilAttachmentInfo.stencilStoreOp = wgpu::StoreOp::Undefined;
+ } else {
+ passDescriptor.cDepthStencilAttachmentInfo.stencilReadOnly = false;
+ }
+
+ wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+ wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&passDescriptor);
+ pass.End();
+ wgpu::CommandBuffer commands = encoder.Finish();
+
+ queue.Submit(1, &commands);
+}
+
class ReadOnlyStencilAttachmentTests : public ReadOnlyDepthStencilAttachmentTests {
protected:
void SetUp() override {
