From 55a00c7a1f9a188c5a512af510f8184bbdbad580 Mon Sep 17 00:00:00 2001 From: Jiawei Shao Date: Mon, 30 Sep 2019 07:27:57 +0000 Subject: [PATCH] Set writemask to 0 when no fs output matches color state on Metal and Vulkan This patch fixes an undefined behaviour on Metal and Vulkan when there is a color state whose corresponding fragment output is not declared in the fragment shader. According to Vulkan SPEC (Chapter 14.3), the input values to blending or color attachment writes are undefined for components which do not correspond to a fragment shader output. Vulkan validation layer follows the SPEC that it only allows the shader to not produce a matching output if the writemask is 0, or it will report a warning when the application is against this rule. When no fragment output matches the color state in a render pipeline, the output differs on different Metal devices. On some Metal devices the fragment output will be (0, 0, 0, 0) even if it is not declared in the shader, while on others there will be no fragment outputs and the content in the color attachments is not changed. This patch fixes this issue by setting the color write mask to 0 to prevent the undefined values being written into the color attachments. With this patch, the following end2end tests will not report warnings any more when we enable the Vulkan validation layer: ObjectCachingTest.RenderPipelineDeduplicationOnLayout/Vulkan ObjectCachingTest.RenderPipelineDeduplicationOnVertexModule/Vulkan ObjectCachingTest.RenderPipelineDeduplicationOnFragmentModule/Vulkan BUG=dawn:209 TEST=dawn_end2end_tests Change-Id: I5613daa1b9a45349ea1459fbdfe4a12d6149f0f7 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/11581 Reviewed-by: Austin Eng Reviewed-by: Corentin Wallez Commit-Queue: Jiawei Shao --- src/dawn_native/metal/RenderPipelineMTL.mm | 19 +++++-- src/dawn_native/vulkan/RenderPipelineVk.cpp | 25 ++++++--- src/tests/end2end/RenderPassTests.cpp | 58 +++++++++++++++++++-- 3 files changed, 88 insertions(+), 14 deletions(-) diff --git a/src/dawn_native/metal/RenderPipelineMTL.mm b/src/dawn_native/metal/RenderPipelineMTL.mm index c00d488cac..a4ea8c67aa 100644 --- a/src/dawn_native/metal/RenderPipelineMTL.mm +++ b/src/dawn_native/metal/RenderPipelineMTL.mm @@ -182,7 +182,12 @@ namespace dawn_native { namespace metal { } } - MTLColorWriteMask MetalColorWriteMask(dawn::ColorWriteMask writeMask) { + MTLColorWriteMask MetalColorWriteMask(dawn::ColorWriteMask writeMask, + bool isDeclaredInFragmentShader) { + if (!isDeclaredInFragmentShader) { + return MTLColorWriteMaskNone; + } + MTLColorWriteMask mask = MTLColorWriteMaskNone; if (writeMask & dawn::ColorWriteMask::Red) { @@ -202,7 +207,8 @@ namespace dawn_native { namespace metal { } void ComputeBlendDesc(MTLRenderPipelineColorAttachmentDescriptor* attachment, - const ColorStateDescriptor* descriptor) { + const ColorStateDescriptor* descriptor, + bool isDeclaredInFragmentShader) { attachment.blendingEnabled = BlendEnabled(descriptor); attachment.sourceRGBBlendFactor = MetalBlendFactor(descriptor->colorBlend.srcFactor, false); @@ -214,7 +220,8 @@ namespace dawn_native { namespace metal { attachment.destinationAlphaBlendFactor = MetalBlendFactor(descriptor->alphaBlend.dstFactor, true); attachment.alphaBlendOperation = MetalBlendOperation(descriptor->alphaBlend.operation); - attachment.writeMask = MetalColorWriteMask(descriptor->writeMask); + attachment.writeMask = + MetalColorWriteMask(descriptor->writeMask, isDeclaredInFragmentShader); } MTLStencilOperation MetalStencilOperation(dawn::StencilOperation stencilOperation) { @@ -339,11 +346,15 @@ namespace dawn_native { namespace metal { descriptorMTL.stencilAttachmentPixelFormat = MetalPixelFormat(depthStencilFormat); } + const ShaderModuleBase::FragmentOutputBaseTypes& fragmentOutputBaseTypes = + descriptor->fragmentStage->module->GetFragmentOutputBaseTypes(); for (uint32_t i : IterateBitSet(GetColorAttachmentsMask())) { descriptorMTL.colorAttachments[i].pixelFormat = MetalPixelFormat(GetColorAttachmentFormat(i)); const ColorStateDescriptor* descriptor = GetColorStateDescriptor(i); - ComputeBlendDesc(descriptorMTL.colorAttachments[i], descriptor); + bool isDeclaredInFragmentShader = fragmentOutputBaseTypes[i] != Format::Other; + ComputeBlendDesc(descriptorMTL.colorAttachments[i], descriptor, + isDeclaredInFragmentShader); } descriptorMTL.inputPrimitiveTopology = MTLInputPrimitiveTopology(GetPrimitiveTopology()); diff --git a/src/dawn_native/vulkan/RenderPipelineVk.cpp b/src/dawn_native/vulkan/RenderPipelineVk.cpp index 8b8f5bf266..eed883efec 100644 --- a/src/dawn_native/vulkan/RenderPipelineVk.cpp +++ b/src/dawn_native/vulkan/RenderPipelineVk.cpp @@ -207,7 +207,8 @@ namespace dawn_native { namespace vulkan { } } - VkColorComponentFlagBits VulkanColorWriteMask(dawn::ColorWriteMask mask) { + VkColorComponentFlags VulkanColorWriteMask(dawn::ColorWriteMask mask, + bool isDeclaredInFragmentShader) { // Vulkan and Dawn color write masks match, static assert it and return the mask static_assert(static_cast(dawn::ColorWriteMask::Red) == VK_COLOR_COMPONENT_R_BIT, @@ -222,11 +223,16 @@ namespace dawn_native { namespace vulkan { VK_COLOR_COMPONENT_A_BIT, ""); - return static_cast(mask); + // According to Vulkan SPEC (Chapter 14.3): "The input values to blending or color + // attachment writes are undefined for components which do not correspond to a fragment + // shader outputs", we set the color write mask to 0 to prevent such undefined values + // being written into the color attachments. + return isDeclaredInFragmentShader ? static_cast(mask) + : static_cast(0); } - VkPipelineColorBlendAttachmentState ComputeColorDesc( - const ColorStateDescriptor* descriptor) { + VkPipelineColorBlendAttachmentState ComputeColorDesc(const ColorStateDescriptor* descriptor, + bool isDeclaredInFragmentShader) { VkPipelineColorBlendAttachmentState attachment; attachment.blendEnable = BlendEnabled(descriptor) ? VK_TRUE : VK_FALSE; attachment.srcColorBlendFactor = VulkanBlendFactor(descriptor->colorBlend.srcFactor); @@ -235,7 +241,8 @@ namespace dawn_native { namespace vulkan { attachment.srcAlphaBlendFactor = VulkanBlendFactor(descriptor->alphaBlend.srcFactor); attachment.dstAlphaBlendFactor = VulkanBlendFactor(descriptor->alphaBlend.dstFactor); attachment.alphaBlendOp = VulkanBlendOperation(descriptor->alphaBlend.operation); - attachment.colorWriteMask = VulkanColorWriteMask(descriptor->writeMask); + attachment.colorWriteMask = + VulkanColorWriteMask(descriptor->writeMask, isDeclaredInFragmentShader); return attachment; } @@ -400,9 +407,13 @@ namespace dawn_native { namespace vulkan { // Initialize the "blend state info" that will be chained in the "create info" from the data // pre-computed in the ColorState std::array colorBlendAttachments; + const ShaderModuleBase::FragmentOutputBaseTypes& fragmentOutputBaseTypes = + descriptor->fragmentStage->module->GetFragmentOutputBaseTypes(); for (uint32_t i : IterateBitSet(GetColorAttachmentsMask())) { - const ColorStateDescriptor* descriptor = GetColorStateDescriptor(i); - colorBlendAttachments[i] = ComputeColorDesc(descriptor); + const ColorStateDescriptor* colorStateDescriptor = GetColorStateDescriptor(i); + bool isDeclaredInFragmentShader = fragmentOutputBaseTypes[i] != Format::Other; + colorBlendAttachments[i] = + ComputeColorDesc(colorStateDescriptor, isDeclaredInFragmentShader); } VkPipelineColorBlendStateCreateInfo colorBlend; colorBlend.sType = VK_STRUCTURE_TYPE_PIPELINE_COLOR_BLEND_STATE_CREATE_INFO; diff --git a/src/tests/end2end/RenderPassTests.cpp b/src/tests/end2end/RenderPassTests.cpp index 2676df6979..8b147c58de 100644 --- a/src/tests/end2end/RenderPassTests.cpp +++ b/src/tests/end2end/RenderPassTests.cpp @@ -26,8 +26,7 @@ protected: DawnTest::SetUp(); // Shaders to draw a bottom-left triangle in blue. - dawn::ShaderModule vsModule = - utils::CreateShaderModule(device, utils::SingleShaderStage::Vertex, R"( + mVSModule = utils::CreateShaderModule(device, utils::SingleShaderStage::Vertex, R"( #version 450 void main() { const vec2 pos[3] = vec2[3]( @@ -44,7 +43,7 @@ protected: })"); utils::ComboRenderPipelineDescriptor descriptor(device); - descriptor.vertexStage.module = vsModule; + descriptor.vertexStage.module = mVSModule; descriptor.cFragmentStage.module = fsModule; descriptor.primitiveTopology = dawn::PrimitiveTopology::TriangleStrip; descriptor.cColorStates[0].format = kFormat; @@ -66,6 +65,7 @@ protected: return device.CreateTexture(&descriptor); } + dawn::ShaderModule mVSModule; dawn::RenderPipeline pipeline; }; @@ -119,4 +119,56 @@ TEST_P(RenderPassTest, TwoRenderPassesInOneCommandBuffer) { EXPECT_PIXEL_RGBA8_EQ(kGreen, renderTarget2, kRTSize - 1, 1); } +// Verify that the content in the color attachment will not be changed if there is no corresponding +// fragment shader outputs in the render pipeline, the load operation is LoadOp::Load and the store +// operation is StoreOp::Store. +TEST_P(RenderPassTest, NoCorrespondingFragmentShaderOutputs) { + dawn::Texture renderTarget = CreateDefault2DTexture(); + dawn::CommandEncoder encoder = device.CreateCommandEncoder(); + + dawn::TextureView renderTargetView = renderTarget.CreateView(); + + utils::ComboRenderPassDescriptor renderPass({renderTargetView}); + renderPass.cColorAttachments[0].clearColor = {1.0f, 0.0f, 0.0f, 1.0f}; + renderPass.cColorAttachments[0].loadOp = dawn::LoadOp::Clear; + renderPass.cColorAttachments[0].storeOp = dawn::StoreOp::Store; + dawn::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass); + + { + // First we draw a blue triangle in the bottom left of renderTarget. + pass.SetPipeline(pipeline); + pass.Draw(3, 1, 0, 0); + } + + { + // Next we use a pipeline whose fragment shader has no outputs. + dawn::ShaderModule fsModule = + utils::CreateShaderModule(device, utils::SingleShaderStage::Fragment, R"( + #version 450 + void main() { + })"); + utils::ComboRenderPipelineDescriptor descriptor(device); + descriptor.vertexStage.module = mVSModule; + descriptor.cFragmentStage.module = fsModule; + descriptor.primitiveTopology = dawn::PrimitiveTopology::TriangleStrip; + descriptor.cColorStates[0].format = kFormat; + + dawn::RenderPipeline pipelineWithNoFragmentOutput = + device.CreateRenderPipeline(&descriptor); + + pass.SetPipeline(pipelineWithNoFragmentOutput); + pass.Draw(3, 1, 0, 0); + } + + pass.EndPass(); + + dawn::CommandBuffer commands = encoder.Finish(); + queue.Submit(1, &commands); + + constexpr RGBA8 kRed(255, 0, 0, 255); + constexpr RGBA8 kBlue(0, 0, 255, 255); + EXPECT_PIXEL_RGBA8_EQ(kBlue, renderTarget, 2, kRTSize - 1); + EXPECT_PIXEL_RGBA8_EQ(kRed, renderTarget, kRTSize - 1, 1); +} + DAWN_INSTANTIATE_TEST(RenderPassTest, D3D12Backend, MetalBackend, OpenGLBackend, VulkanBackend);