Metal: set both depth/stencil attachments for combined formats

Adds a toggle to workaround another issue where Metal fails to set
the depth/stencil attachment correctly for a combined depth stencil
format if just one of the attachments is used. The workaround forces
both attachments to be set, giving the unused one LoadOp::Load and
StoreOp::Store so its contents are preserved.

Bug: dawn:1389
Change-Id: Iacbefcc57b33bf11ca8fcacb03506301646fe59d
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/117175
Reviewed-by: Loko Kung <lokokung@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Austin Eng <enga@chromium.org>

src/dawn/native/Toggles.cpp

@@ -328,8 +328,19 @@ static constexpr ToggleEnumAndInfoList kToggleNameAndInfoList = {{
       "https://crbug.com/1237175"}},
     {Toggle::MetalUseCombinedDepthStencilFormatForStencil8,
      {"metal_use_combined_depth_stencil_format_for_stencil8",
-      "Use a combined depth stencil format instead of stencil8. The stencil8 format alone does not "
-      "work correctly.",
+      "Use a combined depth stencil format instead of stencil8. Works around an issue where the "
+      "stencil8 format alone does not work correctly. This toggle also causes depth stencil "
+      "attachments using a stencil8 format to also set the depth attachment in the Metal render "
+      "pass. This works around another issue where Metal fails to set the stencil attachment "
+      "correctly for a combined depth stencil format if the depth attachment is not also set.",
+      "https://crbug.com/dawn/1389"}},
+    {Toggle::MetalUseBothDepthAndStencilAttachmentsForCombinedDepthStencilFormats,
+     {"metal_use_both_depth_and_stencil_attachments_for_combined_depth_stencil_formats",
+      "In Metal, depth and stencil attachments are set separately. Setting just one without the "
+      "other does not work correctly for combined depth stencil formats on some Metal drivers. "
+      "This workarounds ensures that both are set. This situation arises during lazy clears, or "
+      "for stencil8 formats if metal_use_combined_depth_stencil_format_for_stencil8 is also "
+      "enabled.",
       "https://crbug.com/dawn/1389"}},
     {Toggle::UseTempTextureInStencilTextureToBufferCopy,
      {"use_temp_texture_in_stencil_texture_to_buffer_copy",

src/dawn/native/Toggles.h

@@ -83,6 +83,7 @@ enum class Toggle {
     VulkanSplitCommandBufferOnDepthStencilComputeSampleAfterRenderPass,
     D3D12Allocate2DTexturewithCopyDstAsCommittedResource,
     MetalUseCombinedDepthStencilFormatForStencil8,
+    MetalUseBothDepthAndStencilAttachmentsForCombinedDepthStencilFormats,
     UseTempTextureInStencilTextureToBufferCopy,
     DisallowDeprecatedAPIs,
 

src/dawn/native/metal/CommandBufferMTL.mm

@@ -169,6 +169,7 @@ NSRef<MTLComputePassDescriptor> CreateMTLComputePassDescriptor(BeginComputePassC
 }
 
 NSRef<MTLRenderPassDescriptor> CreateMTLRenderPassDescriptor(
+    const Device* device,
     BeginRenderPassCmd* renderPass,
     bool useCounterSamplingAtStageBoundary) {
     // Note that this creates a descriptor that's autoreleased so we don't use AcquireNSRef
@@ -801,11 +802,11 @@ MaybeError CommandBuffer::FillCommands(CommandRecordingContext* commandContext)
                 commandContext->EndBlit();
 
                 LazyClearRenderPassAttachments(cmd);
+                Device* device = ToBackend(GetDevice());
                 NSRef<MTLRenderPassDescriptor> descriptor = CreateMTLRenderPassDescriptor(
-                    cmd, ToBackend(GetDevice())->UseCounterSamplingAtStageBoundary());
+                    device, cmd, device->UseCounterSamplingAtStageBoundary());
                 DAWN_TRY(EncodeMetalRenderPass(
-                    ToBackend(GetDevice()), commandContext, descriptor.Get(), cmd->width,
-                    cmd->height,
+                    device, commandContext, descriptor.Get(), cmd->width, cmd->height,
                     [this](id<MTLRenderCommandEncoder> encoder, BeginRenderPassCmd* cmd)
                         -> MaybeError { return this->EncodeRenderPass(encoder, cmd); },
                     cmd));

src/dawn/native/metal/DeviceMTL.mm

@@ -260,6 +260,8 @@ void Device::InitTogglesFromDriver() {
 #if DAWN_PLATFORM_IS(MACOS)
     if (gpu_info::IsIntel(vendorId)) {
         SetToggle(Toggle::UseTempTextureInStencilTextureToBufferCopy, true);
+        SetToggle(Toggle::MetalUseBothDepthAndStencilAttachmentsForCombinedDepthStencilFormats,
+                  true);
 
         if ([NSProcessInfo.processInfo
                 isOperatingSystemAtLeastVersion:NSOperatingSystemVersion{12, 0, 0}]) {

src/dawn/native/metal/RenderPipelineMTL.mm

@@ -373,14 +373,24 @@ MaybeError RenderPipeline::Initialize() {
 
     if (HasDepthStencilAttachment()) {
         wgpu::TextureFormat depthStencilFormat = GetDepthStencilFormat();
-        const Format& internalFormat = GetDevice()->GetValidInternalFormat(depthStencilFormat);
         MTLPixelFormat metalFormat = MetalPixelFormat(GetDevice(), depthStencilFormat);
 
-        if (internalFormat.HasDepth()) {
-            descriptorMTL.depthAttachmentPixelFormat = metalFormat;
-        }
-        if (internalFormat.HasStencil()) {
-            descriptorMTL.stencilAttachmentPixelFormat = metalFormat;
+        if (GetDevice()->IsToggleEnabled(
+                Toggle::MetalUseBothDepthAndStencilAttachmentsForCombinedDepthStencilFormats)) {
+            if (GetDepthStencilAspects(metalFormat) & Aspect::Depth) {
+                descriptorMTL.depthAttachmentPixelFormat = metalFormat;
+            }
+            if (GetDepthStencilAspects(metalFormat) & Aspect::Stencil) {
+                descriptorMTL.stencilAttachmentPixelFormat = metalFormat;
+            }
+        } else {
+            const Format& internalFormat = GetDevice()->GetValidInternalFormat(depthStencilFormat);
+            if (internalFormat.HasDepth()) {
+                descriptorMTL.depthAttachmentPixelFormat = metalFormat;
+            }
+            if (internalFormat.HasStencil()) {
+                descriptorMTL.stencilAttachmentPixelFormat = metalFormat;
+            }
         }
     }
 

src/dawn/native/metal/UtilsMetal.h

@@ -32,6 +32,7 @@ enum class SingleShaderStage;
 
 namespace dawn::native::metal {
 
+Aspect GetDepthStencilAspects(MTLPixelFormat format);
 MTLCompareFunction ToMetalCompareFunction(wgpu::CompareFunction compareFunction);
 
 struct TextureBufferCopySplit {

src/dawn/native/metal/UtilsMetal.mm

@@ -140,8 +140,29 @@ void ResolveInAnotherRenderPass(
     commandContext->BeginRender(mtlRenderPassForResolve);
     commandContext->EndRender();
 }
+
 }  // anonymous namespace
 
+Aspect GetDepthStencilAspects(MTLPixelFormat format) {
+    switch (format) {
+        case MTLPixelFormatDepth16Unorm:
+        case MTLPixelFormatDepth32Float:
+            return Aspect::Depth;
+
+#if DAWN_PLATFORM_IS(MACOS)
+        case MTLPixelFormatDepth24Unorm_Stencil8:
+#endif
+        case MTLPixelFormatDepth32Float_Stencil8:
+            return Aspect::Depth | Aspect::Stencil;
+
+        case MTLPixelFormatStencil8:
+            return Aspect::Stencil;
+
+        default:
+            UNREACHABLE();
+    }
+}
+
 MTLCompareFunction ToMetalCompareFunction(wgpu::CompareFunction compareFunction) {
     switch (compareFunction) {
         case wgpu::CompareFunction::Never:
@@ -347,6 +368,34 @@ MaybeError EncodeMetalRenderPass(Device* device,
     // workarounds to happen at the same time, it handles workarounds one by one and calls
     // itself recursively to handle the next workaround if needed.
 
+    // Handle the workaround where both depth and stencil attachments must be set for a
+    // combined depth-stencil format, not just one.
+    if (device->IsToggleEnabled(
+            Toggle::MetalUseBothDepthAndStencilAttachmentsForCombinedDepthStencilFormats)) {
+        const bool hasDepthAttachment = mtlRenderPass.depthAttachment.texture != nil;
+        const bool hasStencilAttachment = mtlRenderPass.stencilAttachment.texture != nil;
+
+        if (hasDepthAttachment && !hasStencilAttachment) {
+            if (GetDepthStencilAspects([mtlRenderPass.depthAttachment.texture pixelFormat]) &
+                Aspect::Stencil) {
+                mtlRenderPass.stencilAttachment.texture = mtlRenderPass.depthAttachment.texture;
+                mtlRenderPass.stencilAttachment.level = mtlRenderPass.depthAttachment.level;
+                mtlRenderPass.stencilAttachment.slice = mtlRenderPass.depthAttachment.slice;
+                mtlRenderPass.stencilAttachment.loadAction = MTLLoadActionLoad;
+                mtlRenderPass.stencilAttachment.storeAction = MTLStoreActionStore;
+            }
+        } else if (hasStencilAttachment && !hasDepthAttachment) {
+            if (GetDepthStencilAspects([mtlRenderPass.stencilAttachment.texture pixelFormat]) &
+                Aspect::Depth) {
+                mtlRenderPass.depthAttachment.texture = mtlRenderPass.stencilAttachment.texture;
+                mtlRenderPass.depthAttachment.level = mtlRenderPass.stencilAttachment.level;
+                mtlRenderPass.depthAttachment.slice = mtlRenderPass.stencilAttachment.slice;
+                mtlRenderPass.depthAttachment.loadAction = MTLLoadActionLoad;
+                mtlRenderPass.depthAttachment.storeAction = MTLStoreActionStore;
+            }
+        }
+    }
+
     // Handles the workaround for r8unorm rg8unorm mipmap rendering being broken on some
     // devices. Render to a temporary texture instead and then copy back to the attachment.
     if (device->IsToggleEnabled(Toggle::MetalRenderR8RG8UnormSmallMipToTempTexture)) {