From 57fcd17625dad9f519e82b09dded2099787731cb Mon Sep 17 00:00:00 2001
From: Kai Ninomiya <kainino@chromium.org>
Date: Thu, 28 Jan 2021 07:03:45 +0000
Subject: [PATCH] Fix translation of storeOp "clear" on Metal

Previously, resolve was not performed if storeOp was "clear".
Resolve should occur iff a resolveTarget is provided, regardless of
storeOp - storeOp only controls the regular render target.

Test: webgpu:api,operation,render_pass,resolve:render_pass_resolve:*
Test: MultisampledRenderingTest.ResolveInto2DTexture
Bug: dawn:650
Change-Id: Ibeae984be7f82680f182246fabc6ca9a4e1af176
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/38880
Commit-Queue: Kai Ninomiya <kainino@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
---
 src/dawn_native/metal/CommandBufferMTL.mm     | 40 +++++++++++--------
 .../end2end/MultisampledRenderingTests.cpp    | 34 +++++++++-------
 2 files changed, 43 insertions(+), 31 deletions(-)

diff --git a/src/dawn_native/metal/CommandBufferMTL.mm b/src/dawn_native/metal/CommandBufferMTL.mm
index cae5ffec86..62259d00da 100644
--- a/src/dawn_native/metal/CommandBufferMTL.mm
+++ b/src/dawn_native/metal/CommandBufferMTL.mm
@@ -84,27 +84,33 @@ namespace dawn_native { namespace metal {
                 descriptor.colorAttachments[i].slice = attachmentInfo.view->GetBaseArrayLayer();
 
                 bool hasResolveTarget = attachmentInfo.resolveTarget != nullptr;
+                if (hasResolveTarget) {
+                    descriptor.colorAttachments[i].resolveTexture =
+                        ToBackend(attachmentInfo.resolveTarget->GetTexture())->GetMTLTexture();
+                    descriptor.colorAttachments[i].resolveLevel =
+                        attachmentInfo.resolveTarget->GetBaseMipLevel();
+                    descriptor.colorAttachments[i].resolveSlice =
+                        attachmentInfo.resolveTarget->GetBaseArrayLayer();
 
-                switch (attachmentInfo.storeOp) {
-                    case wgpu::StoreOp::Store:
-                        if (hasResolveTarget) {
-                            descriptor.colorAttachments[i].resolveTexture =
-                                ToBackend(attachmentInfo.resolveTarget->GetTexture())
-                                    ->GetMTLTexture();
-                            descriptor.colorAttachments[i].resolveLevel =
-                                attachmentInfo.resolveTarget->GetBaseMipLevel();
-                            descriptor.colorAttachments[i].resolveSlice =
-                                attachmentInfo.resolveTarget->GetBaseArrayLayer();
+                    switch (attachmentInfo.storeOp) {
+                        case wgpu::StoreOp::Store:
                             descriptor.colorAttachments[i].storeAction =
                                 kMTLStoreActionStoreAndMultisampleResolve;
-                        } else {
+                            break;
+                        case wgpu::StoreOp::Clear:
+                            descriptor.colorAttachments[i].storeAction =
+                                MTLStoreActionMultisampleResolve;
+                            break;
+                    }
+                } else {
+                    switch (attachmentInfo.storeOp) {
+                        case wgpu::StoreOp::Store:
                             descriptor.colorAttachments[i].storeAction = MTLStoreActionStore;
-                        }
-                        break;
-
-                    case wgpu::StoreOp::Clear:
-                        descriptor.colorAttachments[i].storeAction = MTLStoreActionDontCare;
-                        break;
+                            break;
+                        case wgpu::StoreOp::Clear:
+                            descriptor.colorAttachments[i].storeAction = MTLStoreActionDontCare;
+                            break;
+                    }
                 }
             }
 
diff --git a/src/tests/end2end/MultisampledRenderingTests.cpp b/src/tests/end2end/MultisampledRenderingTests.cpp
index ac10f3f851..f72938efda 100644
--- a/src/tests/end2end/MultisampledRenderingTests.cpp
+++ b/src/tests/end2end/MultisampledRenderingTests.cpp
@@ -265,25 +265,31 @@ class MultisampledRenderingTest : public DawnTest {
 // Test using one multisampled color attachment with resolve target can render correctly.
 TEST_P(MultisampledRenderingTest, ResolveInto2DTexture) {
     constexpr bool kTestDepth = false;
-    wgpu::CommandEncoder commandEncoder = device.CreateCommandEncoder();
     wgpu::RenderPipeline pipeline = CreateRenderPipelineWithOneOutputForTest(kTestDepth);
 
     constexpr wgpu::Color kGreen = {0.0f, 0.8f, 0.0f, 0.8f};
 
-    // Draw a green triangle.
-    {
-        utils::ComboRenderPassDescriptor renderPass = CreateComboRenderPassDescriptorForTest(
-            {mMultisampledColorView}, {mResolveView}, wgpu::LoadOp::Clear, wgpu::LoadOp::Clear,
-            kTestDepth);
-        std::array<float, 4> kUniformData = {kGreen.r, kGreen.g, kGreen.b, kGreen.a};
-        constexpr uint32_t kSize = sizeof(kUniformData);
-        EncodeRenderPassForTest(commandEncoder, renderPass, pipeline, kUniformData.data(), kSize);
+    // storeOp should not affect the result in the resolve target.
+    for (wgpu::StoreOp storeOp : {wgpu::StoreOp::Store, wgpu::StoreOp::Clear}) {
+        wgpu::CommandEncoder commandEncoder = device.CreateCommandEncoder();
+
+        // Draw a green triangle.
+        {
+            utils::ComboRenderPassDescriptor renderPass = CreateComboRenderPassDescriptorForTest(
+                {mMultisampledColorView}, {mResolveView}, wgpu::LoadOp::Clear, wgpu::LoadOp::Clear,
+                kTestDepth);
+            renderPass.cColorAttachments[0].storeOp = storeOp;
+            std::array<float, 4> kUniformData = {kGreen.r, kGreen.g, kGreen.b, kGreen.a};
+            constexpr uint32_t kSize = sizeof(kUniformData);
+            EncodeRenderPassForTest(commandEncoder, renderPass, pipeline, kUniformData.data(),
+                                    kSize);
+        }
+
+        wgpu::CommandBuffer commandBuffer = commandEncoder.Finish();
+        queue.Submit(1, &commandBuffer);
+
+        VerifyResolveTarget(kGreen, mResolveTexture);
     }
-
-    wgpu::CommandBuffer commandBuffer = commandEncoder.Finish();
-    queue.Submit(1, &commandBuffer);
-
-    VerifyResolveTarget(kGreen, mResolveTexture);
 }
 
 // Test that a single-layer multisampled texture view can be created and resolved from.