From 110358f2e6161384b373f305985cae9dfb590cc4 Mon Sep 17 00:00:00 2001 From: Gregg Tavares Date: Thu, 25 May 2023 04:37:12 +0000 Subject: [PATCH] Compat: Reject non-matching blend state / write mask Bug: dawn:1839 Change-Id: I1284fb0c5b6a0d21603d9a9806d31e931b17b615 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/134061 Reviewed-by: Stephen White Kokoro: Kokoro Reviewed-by: Austin Eng Commit-Queue: Gregg Tavares --- src/dawn/native/Device.cpp | 2 +- src/dawn/native/RenderPipeline.cpp | 87 +++++++++++++++++-- .../tests/end2end/PipelineCachingTests.cpp | 3 + .../validation/CompatValidationTests.cpp | 82 +++++++++++++++++ 4 files changed, 165 insertions(+), 9 deletions(-) diff --git a/src/dawn/native/Device.cpp b/src/dawn/native/Device.cpp index 43e2787f1f..d8714f0d5a 100644 --- a/src/dawn/native/Device.cpp +++ b/src/dawn/native/Device.cpp @@ -1450,7 +1450,7 @@ bool DeviceBase::IsRobustnessEnabled() const { } bool DeviceBase::IsCompatibilityMode() const { - return mAdapter->GetFeatureLevel() == FeatureLevel::Compatibility; + return mAdapter != nullptr && mAdapter->GetFeatureLevel() == FeatureLevel::Compatibility; } bool DeviceBase::AllowUnsafeAPIs() const { diff --git a/src/dawn/native/RenderPipeline.cpp b/src/dawn/native/RenderPipeline.cpp index d27a8d010c..8d00eca10a 100644 --- a/src/dawn/native/RenderPipeline.cpp +++ b/src/dawn/native/RenderPipeline.cpp @@ -352,6 +352,61 @@ MaybeError ValidateColorTargetState( return {}; } +MaybeError ValidateCompatibilityColorTargetState( + const uint8_t firstColorTargetIndex, + const ColorTargetState* const firstColorTargetState, + const uint8_t targetIndex, + const ColorTargetState* target) { + DAWN_INVALID_IF(firstColorTargetState->writeMask != target->writeMask, + "targets[%u].writeMask (%s) does not match targets[%u].writeMask (%s).", + targetIndex, target->writeMask, firstColorTargetIndex, + firstColorTargetState->writeMask); + if (!firstColorTargetState->blend) { + DAWN_INVALID_IF(target->blend, + "targets[%u].blend has a blend state but targets[%u].blend does not.", + targetIndex, firstColorTargetIndex); + } else { + DAWN_INVALID_IF(!target->blend, + "targets[%u].blend has a blend state but targets[%u].blend does not.", + firstColorTargetIndex, targetIndex); + + const BlendState& currBlendState = *target->blend; + const BlendState& firstBlendState = *firstColorTargetState->blend; + + DAWN_INVALID_IF( + firstBlendState.color.operation != currBlendState.color.operation, + "targets[%u].color.operation (%s) does not match targets[%u].color.operation (%s).", + firstColorTargetIndex, firstBlendState.color.operation, targetIndex, + currBlendState.color.operation); + DAWN_INVALID_IF( + firstBlendState.color.srcFactor != currBlendState.color.srcFactor, + "targets[%u].color.srcFactor (%s) does not match targets[%u].color.srcFactor (%s).", + firstColorTargetIndex, firstBlendState.color.srcFactor, targetIndex, + currBlendState.color.srcFactor); + DAWN_INVALID_IF( + firstBlendState.color.dstFactor != currBlendState.color.dstFactor, + "targets[%u].color.dstFactor (%s) does not match targets[%u].color.dstFactor (%s).", + firstColorTargetIndex, firstBlendState.color.dstFactor, targetIndex, + currBlendState.color.dstFactor); + DAWN_INVALID_IF( + firstBlendState.alpha.operation != currBlendState.alpha.operation, + "targets[%u].alpha.operation (%s) does not match targets[%u].alpha.operation (%s).", + firstColorTargetIndex, firstBlendState.alpha.operation, targetIndex, + currBlendState.alpha.operation); + DAWN_INVALID_IF( + firstBlendState.alpha.srcFactor != currBlendState.alpha.srcFactor, + "targets[%u].alpha.srcFactor (%s) does not match targets[%u].alpha.srcFactor (%s).", + firstColorTargetIndex, firstBlendState.alpha.srcFactor, targetIndex, + currBlendState.alpha.srcFactor); + DAWN_INVALID_IF( + firstBlendState.alpha.dstFactor != currBlendState.alpha.dstFactor, + "targets[%u].alpha.dstFactor (%s) does not match targets[%u].alpha.dstFactor (%s).", + firstColorTargetIndex, firstBlendState.alpha.dstFactor, targetIndex, + currBlendState.alpha.dstFactor); + } + return {}; +} + MaybeError ValidateFragmentState(DeviceBase* device, const FragmentState* descriptor, const PipelineLayoutBase* layout, @@ -388,20 +443,36 @@ MaybeError ValidateFragmentState(DeviceBase* device, depthStencil->format, descriptor->module, descriptor->entryPoint); } + uint8_t firstColorTargetIndex = 0; + const ColorTargetState* firstColorTargetState = nullptr; ColorAttachmentFormats colorAttachmentFormats; - for (ColorAttachmentIndex i(uint8_t(0)); - i < ColorAttachmentIndex(static_cast(descriptor->targetCount)); ++i) { - const ColorTargetState* target = &descriptor->targets[static_cast(i)]; + + for (ColorAttachmentIndex attachmentIndex(uint8_t(0)); + attachmentIndex < ColorAttachmentIndex(static_cast(descriptor->targetCount)); + ++attachmentIndex) { + const uint8_t i = static_cast(attachmentIndex); + const ColorTargetState* target = &descriptor->targets[i]; + if (target->format != wgpu::TextureFormat::Undefined) { DAWN_TRY_CONTEXT( - ValidateColorTargetState(device, target, fragmentMetadata.fragmentOutputsWritten[i], - fragmentMetadata.fragmentOutputVariables[i]), - "validating targets[%u].", static_cast(i)); + ValidateColorTargetState(device, target, + fragmentMetadata.fragmentOutputsWritten[attachmentIndex], + fragmentMetadata.fragmentOutputVariables[attachmentIndex]), + "validating targets[%u].", i); colorAttachmentFormats->push_back(&device->GetValidInternalFormat(target->format)); + if (device->IsCompatibilityMode()) { + if (!firstColorTargetState) { + firstColorTargetState = target; + firstColorTargetIndex = i; + } else { + DAWN_TRY_CONTEXT(ValidateCompatibilityColorTargetState( + firstColorTargetIndex, firstColorTargetState, i, target), + "validating targets[%u] in compatibility mode.", i); + } + } } else { DAWN_INVALID_IF(target->blend, - "Color target[%u] blend state is set when the format is undefined.", - static_cast(i)); + "Color target[%u] blend state is set when the format is undefined.", i); } } DAWN_TRY(ValidateColorAttachmentBytesPerSample(device, colorAttachmentFormats)); diff --git a/src/dawn/tests/end2end/PipelineCachingTests.cpp b/src/dawn/tests/end2end/PipelineCachingTests.cpp index 1c38c5aa18..6200ef386a 100644 --- a/src/dawn/tests/end2end/PipelineCachingTests.cpp +++ b/src/dawn/tests/end2end/PipelineCachingTests.cpp @@ -460,6 +460,9 @@ TEST_P(SinglePipelineCachingTests, RenderPipelineBlobCacheShaderNegativeCases) { // Tests that pipeline creation wouldn't hit the cache if the pipelines are not exactly the same // (fragment color targets differences). TEST_P(SinglePipelineCachingTests, RenderPipelineBlobCacheNegativeCasesFragmentColorTargets) { + // In compat, all targets must have the same writeMask + DAWN_TEST_UNSUPPORTED_IF(IsCompatibilityMode()); + // First time should create and write out to the cache. { wgpu::Device device = CreateDevice(); diff --git a/src/dawn/tests/unittests/validation/CompatValidationTests.cpp b/src/dawn/tests/unittests/validation/CompatValidationTests.cpp index 8990236fc4..a1ac4dc8d2 100644 --- a/src/dawn/tests/unittests/validation/CompatValidationTests.cpp +++ b/src/dawn/tests/unittests/validation/CompatValidationTests.cpp @@ -54,5 +54,87 @@ TEST_F(CompatValidationTest, CanNotCreateCubeArrayTextureView) { cubeTexture.Destroy(); } +TEST_F(CompatValidationTest, CanNotCreatePipelineWithDifferentPerTargetBlendStateOrWriteMask) { + wgpu::ShaderModule module = utils::CreateShaderModule(device, R"( + @vertex fn vs() -> @builtin(position) vec4f { + return vec4f(0); + } + + struct FragmentOut { + @location(0) fragColor0 : vec4f, + @location(1) fragColor1 : vec4f, + @location(2) fragColor2 : vec4f, + } + + @fragment fn fs() -> FragmentOut { + var output : FragmentOut; + output.fragColor0 = vec4f(0); + output.fragColor1 = vec4f(0); + output.fragColor2 = vec4f(0); + return output; + } + )"); + + utils::ComboRenderPipelineDescriptor testDescriptor; + testDescriptor.layout = {}; + testDescriptor.vertex.module = module; + testDescriptor.vertex.entryPoint = "vs"; + testDescriptor.cFragment.module = module; + testDescriptor.cFragment.entryPoint = "fs"; + testDescriptor.cFragment.targetCount = 3; + testDescriptor.cTargets[1].format = wgpu::TextureFormat::Undefined; + + for (int i = 0; i < 10; ++i) { + wgpu::BlendState blend0; + wgpu::BlendState blend2; + + // Blend state intentionally omitted for target 1 + testDescriptor.cTargets[0].blend = &blend0; + testDescriptor.cTargets[2].blend = &blend2; + + bool expectError = true; + switch (i) { + case 0: // default + expectError = false; + break; + case 1: // no blend + testDescriptor.cTargets[0].blend = nullptr; + break; + case 2: // no blend second target + testDescriptor.cTargets[2].blend = nullptr; + break; + case 3: // color.operation + blend2.color.operation = wgpu::BlendOperation::Subtract; + break; + case 4: // color.srcFactor + blend2.color.srcFactor = wgpu::BlendFactor::SrcAlpha; + break; + case 5: // color.dstFactor + blend2.color.dstFactor = wgpu::BlendFactor::DstAlpha; + break; + case 6: // alpha.operation + blend2.alpha.operation = wgpu::BlendOperation::Subtract; + break; + case 7: // alpha.srcFactor + blend2.alpha.srcFactor = wgpu::BlendFactor::SrcAlpha; + break; + case 8: // alpha.dstFactor + blend2.alpha.dstFactor = wgpu::BlendFactor::DstAlpha; + break; + case 9: // writeMask + testDescriptor.cTargets[2].writeMask = wgpu::ColorWriteMask::Green; + break; + default: + UNREACHABLE(); + } + + if (expectError) { + ASSERT_DEVICE_ERROR(device.CreateRenderPipeline(&testDescriptor)); + } else { + device.CreateRenderPipeline(&testDescriptor); + } + } +} + } // anonymous namespace } // namespace dawn