Add validation rule for depth/stencil between pipeline and pass

depthWrite/stencilWrite in DepthStencilState in RenderPipeline
should be compatible with depthReadOnly/stencilReadOnly in
DepthStencilAttachment in RenderPass. Otherwise, you may need
to generate validation errors.

Bug: dawn:485

Change-Id: I7b541056dafc4dee4eb31f4cefbac48c0ffc4b18
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/66240
Commit-Queue: Yunchao He <yunchao.he@intel.com>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>

src/dawn_native/CommandEncoder.cpp

@@ -532,6 +532,8 @@ namespace dawn_native {
 
         uint32_t width = 0;
         uint32_t height = 0;
+        bool depthReadOnly = false;
+        bool stencilReadOnly = false;
         Ref<AttachmentState> attachmentState;
         bool success = mEncodingContext.TryEncode(
             this,
@@ -600,6 +602,9 @@ namespace dawn_native {
                     } else {
                         usageTracker.TextureViewUsedAs(view, wgpu::TextureUsage::RenderAttachment);
                     }
+
+                    depthReadOnly = descriptor->depthStencilAttachment->depthReadOnly;
+                    stencilReadOnly = descriptor->depthStencilAttachment->stencilReadOnly;
                 }
 
                 cmd->width = width;
@@ -612,9 +617,10 @@ namespace dawn_native {
             "encoding BeginRenderPass(%s).", descriptor);
 
         if (success) {
-            RenderPassEncoder* passEncoder = new RenderPassEncoder(
+            RenderPassEncoder* passEncoder =
-                device, this, &mEncodingContext, std::move(usageTracker),
+                new RenderPassEncoder(device, this, &mEncodingContext, std::move(usageTracker),
-                std::move(attachmentState), descriptor->occlusionQuerySet, width, height);
+                                      std::move(attachmentState), descriptor->occlusionQuerySet,
+                                      width, height, depthReadOnly, stencilReadOnly);
             mEncodingContext.EnterPass(passEncoder);
             return passEncoder;
         }

src/dawn_native/RenderBundleEncoder.cpp

@@ -79,7 +79,9 @@ namespace dawn_native {
                                              const RenderBundleEncoderDescriptor* descriptor)
         : RenderEncoderBase(device,
                             &mBundleEncodingContext,
-                            device->GetOrCreateAttachmentState(descriptor)),
+                            device->GetOrCreateAttachmentState(descriptor),
+                            false,
+                            false),
           mBundleEncodingContext(device, this) {
     }
 

src/dawn_native/RenderEncoderBase.cpp

@@ -32,12 +32,16 @@ namespace dawn_native {
 
     RenderEncoderBase::RenderEncoderBase(DeviceBase* device,
                                          EncodingContext* encodingContext,
-                                         Ref<AttachmentState> attachmentState)
+                                         Ref<AttachmentState> attachmentState,
+                                         bool depthReadOnly,
+                                         bool stencilReadOnly)
         : ProgrammablePassEncoder(device, encodingContext),
           mIndirectDrawMetadata(device->GetLimits()),
           mAttachmentState(std::move(attachmentState)),
           mDisableBaseVertex(device->IsToggleEnabled(Toggle::DisableBaseVertex)),
           mDisableBaseInstance(device->IsToggleEnabled(Toggle::DisableBaseInstance)) {
+        mDepthReadOnly = depthReadOnly;
+        mStencilReadOnly = stencilReadOnly;
     }
 
     RenderEncoderBase::RenderEncoderBase(DeviceBase* device,
@@ -222,6 +226,14 @@ namespace dawn_native {
                         pipeline->GetAttachmentState() != mAttachmentState.Get(),
                         "Attachment state of %s is not compatible with the attachment state of %s",
                         pipeline, this);
+
+                    DAWN_INVALID_IF(pipeline->WritesDepth() && mDepthReadOnly,
+                                    "%s writes depth while %s's depthReadOnly is true", pipeline,
+                                    this);
+
+                    DAWN_INVALID_IF(pipeline->WritesStencil() && mStencilReadOnly,
+                                    "%s writes stencil while %s's stencilReadOnly is true",
+                                    pipeline, this);
                 }
 
                 mCommandBufferState.SetRenderPipeline(pipeline);

src/dawn_native/RenderEncoderBase.h

@@ -28,7 +28,9 @@ namespace dawn_native {
       public:
         RenderEncoderBase(DeviceBase* device,
                           EncodingContext* encodingContext,
-                          Ref<AttachmentState> attachmentState);
+                          Ref<AttachmentState> attachmentState,
+                          bool depthReadOnly,
+                          bool stencilReadOnly);
 
         void APIDraw(uint32_t vertexCount,
                      uint32_t instanceCount = 1,
@@ -71,6 +73,8 @@ namespace dawn_native {
         Ref<AttachmentState> mAttachmentState;
         const bool mDisableBaseVertex;
         const bool mDisableBaseInstance;
+        bool mDepthReadOnly = false;
+        bool mStencilReadOnly = false;
     };
 
 }  // namespace dawn_native

src/dawn_native/RenderPassEncoder.cpp

@@ -55,8 +55,14 @@ namespace dawn_native {
                                          Ref<AttachmentState> attachmentState,
                                          QuerySetBase* occlusionQuerySet,
                                          uint32_t renderTargetWidth,
-                                         uint32_t renderTargetHeight)
+                                         uint32_t renderTargetHeight,
-        : RenderEncoderBase(device, encodingContext, std::move(attachmentState)),
+                                         bool depthReadOnly,
+                                         bool stencilReadOnly)
+        : RenderEncoderBase(device,
+                            encodingContext,
+                            std::move(attachmentState),
+                            depthReadOnly,
+                            stencilReadOnly),
           mCommandEncoder(commandEncoder),
           mRenderTargetWidth(renderTargetWidth),
           mRenderTargetHeight(renderTargetHeight),

src/dawn_native/RenderPassEncoder.h

@@ -32,7 +32,9 @@ namespace dawn_native {
                           Ref<AttachmentState> attachmentState,
                           QuerySetBase* occlusionQuerySet,
                           uint32_t renderTargetWidth,
-                          uint32_t renderTargetHeight);
+                          uint32_t renderTargetHeight,
+                          bool depthReadOnly,
+                          bool stencilReadOnly);
 
         static RenderPassEncoder* MakeError(DeviceBase* device,
                                             CommandEncoder* commandEncoder,

src/dawn_native/RenderPipeline.cpp

@@ -619,6 +619,19 @@ namespace dawn_native {
 
         if (mAttachmentState->HasDepthStencilAttachment()) {
             mDepthStencil = *descriptor->depthStencil;
+            mWritesDepth = mDepthStencil.depthWriteEnabled;
+            if (mDepthStencil.stencilWriteMask) {
+                if ((mPrimitive.cullMode != wgpu::CullMode::Front &&
+                     (mDepthStencil.stencilFront.failOp != wgpu::StencilOperation::Keep ||
+                      mDepthStencil.stencilFront.depthFailOp != wgpu::StencilOperation::Keep ||
+                      mDepthStencil.stencilFront.passOp != wgpu::StencilOperation::Keep)) ||
+                    (mPrimitive.cullMode != wgpu::CullMode::Back &&
+                     (mDepthStencil.stencilBack.failOp != wgpu::StencilOperation::Keep ||
+                      mDepthStencil.stencilBack.depthFailOp != wgpu::StencilOperation::Keep ||
+                      mDepthStencil.stencilBack.passOp != wgpu::StencilOperation::Keep))) {
+                    mWritesStencil = true;
+                }
+            }
         } else {
             // These default values below are useful for backends to fill information.
             // The values indicate that depth and stencil test are disabled when backends
@@ -833,6 +846,18 @@ namespace dawn_native {
         return mAttachmentState.Get();
     }
 
+    bool RenderPipelineBase::WritesDepth() const {
+        ASSERT(!IsError());
+
+        return mWritesDepth;
+    }
+
+    bool RenderPipelineBase::WritesStencil() const {
+        ASSERT(!IsError());
+
+        return mWritesStencil;
+    }
+
     size_t RenderPipelineBase::ComputeContentHash() {
         ObjectContentHasher recorder;
 

src/dawn_native/RenderPipeline.h

@@ -95,6 +95,8 @@ namespace dawn_native {
         uint32_t GetSampleCount() const;
         uint32_t GetSampleMask() const;
         bool IsAlphaToCoverageEnabled() const;
+        bool WritesDepth() const;
+        bool WritesStencil() const;
 
         const AttachmentState* GetAttachmentState() const;
 
@@ -128,6 +130,8 @@ namespace dawn_native {
         DepthStencilState mDepthStencil;
         MultisampleState mMultisample;
         bool mClampDepth = false;
+        bool mWritesDepth = false;
+        bool mWritesStencil = false;
     };
 
 }  // namespace dawn_native

src/tests/BUILD.gn

@@ -230,6 +230,7 @@ test("dawn_unittests") {
     "unittests/validation/MinimumBufferSizeValidationTests.cpp",
     "unittests/validation/MultipleDeviceTests.cpp",
     "unittests/validation/OverridableConstantsValidationTests.cpp",
+    "unittests/validation/PipelineAndPassCompatibilityTests.cpp",
     "unittests/validation/QueryValidationTests.cpp",
     "unittests/validation/QueueOnSubmittedWorkDoneValidationTests.cpp",
     "unittests/validation/QueueSubmitValidationTests.cpp",

src/tests/unittests/validation/PipelineAndPassCompatibilityTests.cpp

@@ -0,0 +1,114 @@
+// Copyright 2021 The Dawn Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "utils/ComboRenderPipelineDescriptor.h"
+#include "utils/WGPUHelpers.h"
+
+#include "tests/unittests/validation/ValidationTest.h"
+
+constexpr static uint32_t kSize = 4;
+
+namespace {
+
+    class RenderPipelineAndPassCompatibilityTests : public ValidationTest {
+      public:
+        wgpu::RenderPipeline CreatePipeline(wgpu::TextureFormat format,
+                                            bool enableDepthWrite,
+                                            bool enableStencilWrite) {
+            // Create a NoOp pipeline
+            utils::ComboRenderPipelineDescriptor pipelineDescriptor;
+            pipelineDescriptor.vertex.module = utils::CreateShaderModule(device, R"(
+                [[stage(vertex)]] fn main() -> [[builtin(position)]] vec4<f32> {
+                    return vec4<f32>();
+                })");
+            pipelineDescriptor.cFragment.module = utils::CreateShaderModule(device, R"(
+                [[stage(fragment)]] fn main() {
+                })");
+            pipelineDescriptor.cFragment.targets = nullptr;
+            pipelineDescriptor.cFragment.targetCount = 0;
+
+            // Enable depth/stencil write if needed
+            wgpu::DepthStencilState* depthStencil = pipelineDescriptor.EnableDepthStencil(format);
+            if (enableDepthWrite) {
+                depthStencil->depthWriteEnabled = true;
+            }
+            if (enableStencilWrite) {
+                depthStencil->stencilFront.failOp = wgpu::StencilOperation::Replace;
+            }
+            return device.CreateRenderPipeline(&pipelineDescriptor);
+        }
+
+        utils::ComboRenderPassDescriptor CreateRenderPassDescriptor(wgpu::TextureFormat format,
+                                                                    bool depthReadOnly,
+                                                                    bool stencilReadOnly) {
+            wgpu::TextureDescriptor textureDescriptor = {};
+            textureDescriptor.size = {kSize, kSize, 1};
+            textureDescriptor.format = format;
+            textureDescriptor.usage = wgpu::TextureUsage::RenderAttachment;
+            wgpu::Texture depthStencilTexture = device.CreateTexture(&textureDescriptor);
+
+            utils::ComboRenderPassDescriptor passDescriptor({}, depthStencilTexture.CreateView());
+            if (depthReadOnly) {
+                passDescriptor.cDepthStencilAttachmentInfo.depthReadOnly = true;
+                passDescriptor.cDepthStencilAttachmentInfo.depthLoadOp = wgpu::LoadOp::Load;
+                passDescriptor.cDepthStencilAttachmentInfo.depthStoreOp = wgpu::StoreOp::Store;
+            }
+
+            if (stencilReadOnly) {
+                passDescriptor.cDepthStencilAttachmentInfo.stencilReadOnly = true;
+                passDescriptor.cDepthStencilAttachmentInfo.stencilLoadOp = wgpu::LoadOp::Load;
+                passDescriptor.cDepthStencilAttachmentInfo.stencilStoreOp = wgpu::StoreOp::Store;
+            }
+
+            return passDescriptor;
+        }
+    };
+
+    // Test depthWrite/stencilWrite in DepthStencilState in pipeline vs
+    // depthReadOnly/stencilReadOnly in DepthStencilAttachment in pass
+    TEST_F(RenderPipelineAndPassCompatibilityTests, WriteAndReadOnlyConflictForDepthStencil) {
+        wgpu::TextureFormat kFormat = wgpu::TextureFormat::Depth24PlusStencil8;
+        // If the format has both depth and stencil aspects, depthReadOnly and stencilReadOnly
+        // should be the same. So it is not necessary to set two separate booleans like
+        // depthReadOnlyInPass and stencilReadOnlyInPass.
+        for (bool depthStencilReadOnlyInPass : {true, false}) {
+            for (bool depthWriteInPipeline : {true, false}) {
+                for (bool stencilWriteInPipeline : {true, false}) {
+                    wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+                    utils::ComboRenderPassDescriptor passDescriptor = CreateRenderPassDescriptor(
+                        kFormat, depthStencilReadOnlyInPass, depthStencilReadOnlyInPass);
+                    wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&passDescriptor);
+                    wgpu::RenderPipeline pipeline =
+                        CreatePipeline(kFormat, depthWriteInPipeline, stencilWriteInPipeline);
+                    pass.SetPipeline(pipeline);
+                    pass.Draw(3);
+                    pass.EndPass();
+                    if (depthStencilReadOnlyInPass &&
+                        (depthWriteInPipeline || stencilWriteInPipeline)) {
+                        ASSERT_DEVICE_ERROR(encoder.Finish());
+                    } else {
+                        encoder.Finish();
+                    }
+                }
+            }
+        }
+    }
+
+    // TODO(dawn:485): add more tests. For example:
+    //   - readOnly vs write for depth/stencil with renderbundle.
+    //   - depth/stencil attachment should be designated if depth/stencil test is enabled.
+    //   - pipeline and pass compatibility tests for color attachment(s).
+    //   - pipeline and pass compatibility tests for compute.
+
+}  // anonymous namespace