dawn::native: Fix the check for no attachment pipelines. The check for attachments being required for render pipelines was incorrect because it was only performed if desc->fragment != nullptr, which is incorrect because desc->fragment == nullptr means that there is no color attachment, and is an error if there's no depthStencil state. Adds a regression test. Fixes unittests that were triggering this validation by mistake, and sent a CTS PR for the same purpose. Bug: 341812551 Change-Id: I1daf726e258712535a42451323c84349257aefe4 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/204934 Reviewed-by: Jiawei Shao <jiawei.shao@intel.com> Commit-Queue: Corentin Wallez <cwallez@chromium.org> Reviewed-by: Kai Ninomiya <kainino@chromium.org>
diff --git a/src/dawn/native/RenderPipeline.cpp b/src/dawn/native/RenderPipeline.cpp index 0f95190..138d39a 100644 --- a/src/dawn/native/RenderPipeline.cpp +++ b/src/dawn/native/RenderPipeline.cpp
@@ -898,16 +898,18 @@ descriptor->depthStencil, descriptor->multisample), "validating fragment state."); - bool hasStorageAttachments = - descriptor->layout != nullptr && descriptor->layout->HasAnyStorageAttachments(); - DAWN_INVALID_IF(descriptor->fragment->targetCount == 0 && !descriptor->depthStencil && - !hasStorageAttachments, - "No attachment was specified (color, depth-stencil or other)."); - DAWN_TRY(ValidateInterStageMatching(device, descriptor->vertex, vertexEntryPoint, *(descriptor->fragment), fragmentEntryPoint)); } + bool hasStorageAttachments = + descriptor->layout != nullptr && descriptor->layout->HasAnyStorageAttachments(); + bool hasColorAttachments = + descriptor->fragment != nullptr && descriptor->fragment->targetCount != 0; + bool hasDepthStencilAttachment = descriptor->depthStencil != nullptr; + DAWN_INVALID_IF(!hasColorAttachments && !hasDepthStencilAttachment && !hasStorageAttachments, + "No attachment was specified."); + return {}; }
diff --git a/src/dawn/tests/end2end/CreatePipelineAsyncTests.cpp b/src/dawn/tests/end2end/CreatePipelineAsyncTests.cpp index 9907f87..82d062a 100644 --- a/src/dawn/tests/end2end/CreatePipelineAsyncTests.cpp +++ b/src/dawn/tests/end2end/CreatePipelineAsyncTests.cpp
@@ -344,6 +344,11 @@ // TODO(crbug.com/dawn/1766): TSAN reported race conditions in NVIDIA's vk driver. DAWN_SUPPRESS_TEST_IF(IsVulkan() && IsNvidia() && IsTsan()); + wgpu::ShaderModule fsModule = utils::CreateShaderModule(device, R"( + @fragment fn main() -> @location(0) vec4f { + return vec4f(0.0, 1.0, 0.0, 1.0); + })"); + for (size_t i = 0; i < 100; i++) { utils::ComboRenderPipelineDescriptor desc; std::string shader = R"( @@ -352,7 +357,8 @@ shader += std::to_string(i); shader += ".0, 0.0, 0.0, 1.0);\n}"; desc.vertex.module = utils::CreateShaderModule(device, shader); - desc.fragment = nullptr; + desc.cFragment.module = fsModule; + desc.cTargets[0].format = wgpu::TextureFormat::RGBA8Unorm; device.CreateRenderPipelineAsync( &desc, wgpu::CallbackMode::AllowProcessEvents, @@ -375,6 +381,11 @@ // TODO(crbug.com/dawn/1766): TSAN reported race conditions in NVIDIA's vk driver. DAWN_SUPPRESS_TEST_IF(IsVulkan() && IsNvidia() && IsTsan()); + wgpu::ShaderModule fsModule = utils::CreateShaderModule(device, R"( + @fragment fn main() -> @location(0) vec4f { + return vec4f(0.0, 1.0, 0.0, 1.0); + })"); + auto f = [&](size_t t) { utils::ComboRenderPipelineDescriptor desc; std::string shader = R"( @@ -383,7 +394,8 @@ shader += std::to_string(t); shader += ".0, 0.0, 0.0, 1.0);\n}"; desc.vertex.module = utils::CreateShaderModule(device, shader); - desc.fragment = nullptr; + desc.cFragment.module = fsModule; + desc.cTargets[0].format = wgpu::TextureFormat::RGBA8Unorm; device.CreateRenderPipelineAsync( &desc, wgpu::CallbackMode::AllowProcessEvents,
diff --git a/src/dawn/tests/unittests/native/AllowedErrorTests.cpp b/src/dawn/tests/unittests/native/AllowedErrorTests.cpp index ca76375..3db6073 100644 --- a/src/dawn/tests/unittests/native/AllowedErrorTests.cpp +++ b/src/dawn/tests/unittests/native/AllowedErrorTests.cpp
@@ -243,8 +243,14 @@ TEST_F(AllowedErrorTests, CreateRenderPipeline) { Ref<ShaderModuleMock> vsModule = ShaderModuleMock::Create(mDeviceMock, kVertexShader.data()); + DepthStencilState ds = {}; + ds.format = wgpu::TextureFormat::Depth32Float; + ds.depthWriteEnabled = wgpu::OptionalBool::True; + ds.depthCompare = wgpu::CompareFunction::Always; + RenderPipelineDescriptor desc = {}; desc.vertex.module = vsModule.Get(); + desc.depthStencil = &ds; Ref<RenderPipelineMock> renderPipelineMock = RenderPipelineMock::Create(mDeviceMock, &desc); EXPECT_CALL(*renderPipelineMock.Get(), InitializeImpl) @@ -288,8 +294,14 @@ TEST_F(AllowedErrorTests, CreateRenderPipelineInternalError) { Ref<ShaderModuleMock> vsModule = ShaderModuleMock::Create(mDeviceMock, kVertexShader.data()); + DepthStencilState ds = {}; + ds.format = wgpu::TextureFormat::Depth32Float; + ds.depthWriteEnabled = wgpu::OptionalBool::True; + ds.depthCompare = wgpu::CompareFunction::Always; + RenderPipelineDescriptor desc = {}; desc.vertex.module = vsModule.Get(); + desc.depthStencil = &ds; Ref<RenderPipelineMock> renderPipelineMock = RenderPipelineMock::Create(mDeviceMock, &desc); EXPECT_CALL(*renderPipelineMock.Get(), InitializeImpl) @@ -341,8 +353,14 @@ TEST_F(AllowedErrorTests, CreateRenderPipelineAsync) { Ref<ShaderModuleMock> vsModule = ShaderModuleMock::Create(mDeviceMock, kVertexShader.data()); + DepthStencilState ds = {}; + ds.format = wgpu::TextureFormat::Depth32Float; + ds.depthWriteEnabled = wgpu::OptionalBool::True; + ds.depthCompare = wgpu::CompareFunction::Always; + RenderPipelineDescriptor desc = {}; desc.vertex.module = vsModule.Get(); + desc.depthStencil = &ds; Ref<RenderPipelineMock> renderPipelineMock = RenderPipelineMock::Create(mDeviceMock, &desc); EXPECT_CALL(*renderPipelineMock.Get(), InitializeImpl) @@ -395,8 +413,14 @@ TEST_F(AllowedErrorTests, CreateRenderPipelineAsyncInternalError) { Ref<ShaderModuleMock> vsModule = ShaderModuleMock::Create(mDeviceMock, kVertexShader.data()); + DepthStencilState ds = {}; + ds.format = wgpu::TextureFormat::Depth32Float; + ds.depthWriteEnabled = wgpu::OptionalBool::True; + ds.depthCompare = wgpu::CompareFunction::Always; + RenderPipelineDescriptor desc = {}; desc.vertex.module = vsModule.Get(); + desc.depthStencil = &ds; Ref<RenderPipelineMock> renderPipelineMock = RenderPipelineMock::Create(mDeviceMock, &desc); EXPECT_CALL(*renderPipelineMock.Get(), InitializeImpl)
diff --git a/src/dawn/tests/unittests/native/CreatePipelineAsyncEventTests.cpp b/src/dawn/tests/unittests/native/CreatePipelineAsyncEventTests.cpp index d899c48..cf7fec6 100644 --- a/src/dawn/tests/unittests/native/CreatePipelineAsyncEventTests.cpp +++ b/src/dawn/tests/unittests/native/CreatePipelineAsyncEventTests.cpp
@@ -78,8 +78,14 @@ // Verify CreateRenderPipelineAsync and the internal CreateRenderPipelineAsyncEvent behavior // on creating compute pipeline with validation error. TEST_F(CreatePipelineAsyncEventTests, InitializationValidationErrorInCreateRenderPipelineAsync) { + wgpu::DepthStencilState ds = {}; + ds.format = wgpu::TextureFormat::Depth32Float; + ds.depthWriteEnabled = wgpu::OptionalBool::True; + ds.depthCompare = wgpu::CompareFunction::Always; + wgpu::RenderPipelineDescriptor desc = {}; desc.vertex.module = utils::CreateShaderModule(device, kVertexShader.data()); + desc.depthStencil = &ds; Ref<RenderPipelineMock> renderPipelineMock = RenderPipelineMock::Create(mDeviceMock, FromCppAPI(&desc)); @@ -103,8 +109,14 @@ // Verify CreateComputePipelineAsync and the internal CreateComputePipelineAsyncEvent behavior // on creating compute pipeline with internal error. TEST_F(CreatePipelineAsyncEventTests, InitializationInternalErrorInCreateRenderPipelineAsync) { + wgpu::DepthStencilState ds = {}; + ds.format = wgpu::TextureFormat::Depth32Float; + ds.depthWriteEnabled = wgpu::OptionalBool::True; + ds.depthCompare = wgpu::CompareFunction::Always; + wgpu::RenderPipelineDescriptor desc = {}; desc.vertex.module = utils::CreateShaderModule(device, kVertexShader.data()); + desc.depthStencil = &ds; Ref<RenderPipelineMock> renderPipelineMock = RenderPipelineMock::Create(mDeviceMock, FromCppAPI(&desc)); @@ -128,8 +140,14 @@ // Test that a long async task's execution won't extend to after the device is dropped. // Device dropping should wait for that task to finish. TEST_F(CreatePipelineAsyncEventTests, LongAsyncTaskFinishesBeforeDeviceIsDropped) { + wgpu::DepthStencilState ds = {}; + ds.format = wgpu::TextureFormat::Depth32Float; + ds.depthWriteEnabled = wgpu::OptionalBool::True; + ds.depthCompare = wgpu::CompareFunction::Always; + wgpu::RenderPipelineDescriptor desc = {}; desc.vertex.module = utils::CreateShaderModule(device, kVertexShader.data()); + desc.depthStencil = &ds; Ref<RenderPipelineMock> renderPipelineMock = RenderPipelineMock::Create(mDeviceMock, FromCppAPI(&desc));
diff --git a/src/dawn/tests/unittests/validation/RenderPipelineValidationTests.cpp b/src/dawn/tests/unittests/validation/RenderPipelineValidationTests.cpp index 67b37f0..cad56a4 100644 --- a/src/dawn/tests/unittests/validation/RenderPipelineValidationTests.cpp +++ b/src/dawn/tests/unittests/validation/RenderPipelineValidationTests.cpp
@@ -1029,6 +1029,27 @@ } } +// Tests that render pipeline without attachments are disallowed. +TEST_F(RenderPipelineValidationTest, NoAttachments) { + utils::ComboRenderPipelineDescriptor desc; + desc.vertex.module = vsModule; + desc.fragment = nullptr; + ASSERT_DEVICE_ERROR(device.CreateRenderPipeline(&desc)); + + // Setting a fragment state with no targets is also 0 attachments. + wgpu::FragmentState fragment; + fragment.targetCount = 0; + fragment.module = utils::CreateShaderModule(device, R"( + @fragment fn fs() {} + )"); + desc.fragment = &fragment; + ASSERT_DEVICE_ERROR(device.CreateRenderPipeline(&desc)); + + // Control case, with a DS attachment, creating the pipeline is allowed. + desc.EnableDepthStencil(wgpu::TextureFormat::Depth32Float); + device.CreateRenderPipeline(&desc); +} + // Tests that the sample count of the render pipeline must be valid // when the alphaToCoverage mode is enabled. TEST_F(RenderPipelineValidationTest, AlphaToCoverageAndSampleCount) {