From 71e6bcf1affa9389daf343077166078c6cab52e2 Mon Sep 17 00:00:00 2001 From: shrekshao Date: Thu, 1 Dec 2022 22:23:47 +0000 Subject: [PATCH] Update validation for pass encoding beginRenderPass/beginComputePass Make validation for pass encoding aligned to spec, where descriptor validation failure will make pass invalid and stop immediately instead of defer to CommandEncoder::Finish() Bug: dawn:1602 Change-Id: I7892009e31f7565e4da43c38d365b056c9ecc22f Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/112448 Kokoro: Kokoro Reviewed-by: Corentin Wallez Commit-Queue: Shrek Shao --- src/dawn/native/CommandEncoder.cpp | 20 ++++-- src/dawn/tests/end2end/DeprecatedAPITests.cpp | 8 +-- .../end2end/SwapChainValidationTests.cpp | 10 +-- .../validation/QueryValidationTests.cpp | 65 +++++++------------ .../RenderPassDescriptorValidationTests.cpp | 16 ++--- .../RenderPipelineValidationTests.cpp | 6 +- webgpu-cts/expectations.txt | 13 ++++ 7 files changed, 66 insertions(+), 72 deletions(-) diff --git a/src/dawn/native/CommandEncoder.cpp b/src/dawn/native/CommandEncoder.cpp index 13c8431caa..95157cad47 100644 --- a/src/dawn/native/CommandEncoder.cpp +++ b/src/dawn/native/CommandEncoder.cpp @@ -781,11 +781,14 @@ ComputePassEncoder* CommandEncoder::APIBeginComputePass(const ComputePassDescrip Ref CommandEncoder::BeginComputePass(const ComputePassDescriptor* descriptor) { DeviceBase* device = GetDevice(); + // If descriptor is invalid, make pass invalid and stop immediately + if (device->ConsumedError(ValidateComputePassDescriptor(device, descriptor))) { + return ComputePassEncoder::MakeError(device, this, &mEncodingContext); + } + bool success = mEncodingContext.TryEncode( this, [&](CommandAllocator* allocator) -> MaybeError { - DAWN_TRY(ValidateComputePassDescriptor(device, descriptor)); - BeginComputePassCmd* cmd = allocator->Allocate(Command::BeginComputePass); @@ -844,17 +847,20 @@ Ref CommandEncoder::BeginRenderPass(const RenderPassDescripto uint32_t width = 0; uint32_t height = 0; + uint32_t sampleCount = 0; bool depthReadOnly = false; bool stencilReadOnly = false; Ref attachmentState; + + // If descriptor is invalid, make pass invalid and stop immediately + if (device->ConsumedError(ValidateRenderPassDescriptor(device, descriptor, &width, &height, + &sampleCount, mUsageValidationMode))) { + return RenderPassEncoder::MakeError(device, this, &mEncodingContext); + } + bool success = mEncodingContext.TryEncode( this, [&](CommandAllocator* allocator) -> MaybeError { - uint32_t sampleCount = 0; - - DAWN_TRY(ValidateRenderPassDescriptor(device, descriptor, &width, &height, &sampleCount, - mUsageValidationMode)); - ASSERT(width > 0 && height > 0 && sampleCount > 0); mEncodingContext.WillBeginRenderPass(); diff --git a/src/dawn/tests/end2end/DeprecatedAPITests.cpp b/src/dawn/tests/end2end/DeprecatedAPITests.cpp index a015a5875d..20d3fdd342 100644 --- a/src/dawn/tests/end2end/DeprecatedAPITests.cpp +++ b/src/dawn/tests/end2end/DeprecatedAPITests.cpp @@ -47,8 +47,7 @@ class DeprecationTests : public DawnTest { // Test that setting attachment rather than view for render pass color and depth/stencil attachments // is deprecated. -// TODO(dawn:1602): validation implementations need updating -TEST_P(DeprecationTests, DISABLED_ReadOnlyDepthStencilStoreLoadOpsAttachment) { +TEST_P(DeprecationTests, ReadOnlyDepthStencilStoreLoadOpsAttachment) { utils::BasicRenderPass renderPass = utils::CreateBasicRenderPass(device, 1, 1); wgpu::RenderPassEncoder pass; @@ -71,6 +70,8 @@ TEST_P(DeprecationTests, DISABLED_ReadOnlyDepthStencilStoreLoadOpsAttachment) { depthAttachment->depthLoadOp = wgpu::LoadOp::Load; depthAttachment->depthStoreOp = wgpu::StoreOp::Store; + depthAttachment->stencilLoadOp = wgpu::LoadOp::Undefined; + depthAttachment->stencilStoreOp = wgpu::StoreOp::Undefined; { wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); @@ -92,8 +93,7 @@ TEST_P(DeprecationTests, DISABLED_ReadOnlyDepthStencilStoreLoadOpsAttachment) { // Test that setting the clearColor, clearDepth, or clearStencil values for render pass attachments // is deprecated. (dawn:1269) -// TODO(dawn:1602): validation implementations need updating -TEST_P(DeprecationTests, DISABLED_AttachmentClearColor) { +TEST_P(DeprecationTests, AttachmentClearColor) { utils::BasicRenderPass renderPass = utils::CreateBasicRenderPass(device, 1, 1); wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); wgpu::RenderPassEncoder pass; diff --git a/src/dawn/tests/end2end/SwapChainValidationTests.cpp b/src/dawn/tests/end2end/SwapChainValidationTests.cpp index b02d760f14..20948edaea 100644 --- a/src/dawn/tests/end2end/SwapChainValidationTests.cpp +++ b/src/dawn/tests/end2end/SwapChainValidationTests.cpp @@ -78,16 +78,16 @@ class SwapChainValidationTests : public DawnTest { void CheckTextureViewIsValid(wgpu::TextureView view) { CheckTextureView(view, false, false); } private: - void CheckTextureView(wgpu::TextureView view, bool errorAtFinish, bool errorAtSubmit) { + void CheckTextureView(wgpu::TextureView view, bool errorAtBeginRenderPass, bool errorAtSubmit) { utils::ComboRenderPassDescriptor renderPassDesc({view}); wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPassDesc); - pass.End(); - if (errorAtFinish) { - ASSERT_DEVICE_ERROR(encoder.Finish()); + if (errorAtBeginRenderPass) { + ASSERT_DEVICE_ERROR(encoder.BeginRenderPass(&renderPassDesc)); } else { + wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPassDesc); + pass.End(); wgpu::CommandBuffer commands = encoder.Finish(); if (errorAtSubmit) { diff --git a/src/dawn/tests/unittests/validation/QueryValidationTests.cpp b/src/dawn/tests/unittests/validation/QueryValidationTests.cpp index d0289e201e..0712461316 100644 --- a/src/dawn/tests/unittests/validation/QueryValidationTests.cpp +++ b/src/dawn/tests/unittests/validation/QueryValidationTests.cpp @@ -156,9 +156,7 @@ TEST_F(OcclusionQueryValidationTest, InvalidOcclusionQuerySet) { CreateQuerySet(otherDevice, wgpu::QueryType::Occlusion, 2); renderPass.occlusionQuerySet = occlusionQuerySetOnOther; wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass); - pass.End(); - ASSERT_DEVICE_ERROR(encoder.Finish()); + ASSERT_DEVICE_ERROR(encoder.BeginRenderPass(&renderPass)); // Clear this out so we don't hold a reference. The query set // must be destroyed before the device local to this test case. @@ -335,8 +333,7 @@ TEST_F(TimestampQueryValidationTest, SetOcclusionQueryWithTimestampQuerySet) { renderPass.occlusionQuerySet = querySet; wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - encoder.BeginRenderPass(&renderPass); - ASSERT_DEVICE_ERROR(encoder.Finish()); + ASSERT_DEVICE_ERROR(encoder.BeginRenderPass(&renderPass)); } // Test timestampWrites in compute pass descriptor @@ -357,9 +354,8 @@ TEST_F(TimestampQueryValidationTest, TimestampWritesOnComputePass) { wgpu::QuerySet occlusionQuerySet = CreateQuerySet(device, wgpu::QueryType::Occlusion, 1); wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - EncodeComputePassWithTimestampWrites( - encoder, {{occlusionQuerySet, 0, wgpu::ComputePassTimestampLocation::Beginning}}); - ASSERT_DEVICE_ERROR(encoder.Finish()); + ASSERT_DEVICE_ERROR(EncodeComputePassWithTimestampWrites( + encoder, {{occlusionQuerySet, 0, wgpu::ComputePassTimestampLocation::Beginning}})); } // Fail to write timestamps to a query set created from another device @@ -369,18 +365,16 @@ TEST_F(TimestampQueryValidationTest, TimestampWritesOnComputePass) { CreateQuerySet(otherDevice, wgpu::QueryType::Timestamp, 2); wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - EncodeComputePassWithTimestampWrites( + ASSERT_DEVICE_ERROR(EncodeComputePassWithTimestampWrites( encoder, {{querySet, 0, wgpu::ComputePassTimestampLocation::Beginning}, - {querySetFromOtherDevice, 1, wgpu::ComputePassTimestampLocation::End}}); - ASSERT_DEVICE_ERROR(encoder.Finish()); + {querySetFromOtherDevice, 1, wgpu::ComputePassTimestampLocation::End}})); } // Fail to write timestamps to the query index which exceeds the number of queries in query set { wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - EncodeComputePassWithTimestampWrites( - encoder, {{querySet, 2, wgpu::ComputePassTimestampLocation::Beginning}}); - ASSERT_DEVICE_ERROR(encoder.Finish()); + ASSERT_DEVICE_ERROR(EncodeComputePassWithTimestampWrites( + encoder, {{querySet, 2, wgpu::ComputePassTimestampLocation::Beginning}})); } // Success to write timestamps to the same query index twice on same compute pass @@ -405,18 +399,16 @@ TEST_F(TimestampQueryValidationTest, TimestampWritesOnComputePass) { // Fail to write timestamps at same location of a compute pass { wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - EncodeComputePassWithTimestampWrites( + ASSERT_DEVICE_ERROR(EncodeComputePassWithTimestampWrites( encoder, {{querySet, 0, wgpu::ComputePassTimestampLocation::Beginning}, - {querySet, 1, wgpu::ComputePassTimestampLocation::Beginning}}); - ASSERT_DEVICE_ERROR(encoder.Finish()); + {querySet, 1, wgpu::ComputePassTimestampLocation::Beginning}})); } // Fail to write timestamps at invalid location of compute pass { wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - EncodeComputePassWithTimestampWrites( - encoder, {{querySet, 0, static_cast(0xFFFFFFFF)}}); - ASSERT_DEVICE_ERROR(encoder.Finish()); + ASSERT_DEVICE_ERROR(EncodeComputePassWithTimestampWrites( + encoder, {{querySet, 0, static_cast(0xFFFFFFFF)}})); } // Fail to write timestamps to a destroyed query set @@ -451,9 +443,8 @@ TEST_F(TimestampQueryValidationTest, TimestampWritesOnRenderPass) { wgpu::QuerySet occlusionQuerySet = CreateQuerySet(device, wgpu::QueryType::Occlusion, 1); wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - EncodeRenderPassWithTimestampWrites( - encoder, {{occlusionQuerySet, 0, wgpu::RenderPassTimestampLocation::Beginning}}); - ASSERT_DEVICE_ERROR(encoder.Finish()); + ASSERT_DEVICE_ERROR(EncodeRenderPassWithTimestampWrites( + encoder, {{occlusionQuerySet, 0, wgpu::RenderPassTimestampLocation::Beginning}})); } // Fail to write timestamps to a query set created from another device @@ -463,18 +454,16 @@ TEST_F(TimestampQueryValidationTest, TimestampWritesOnRenderPass) { CreateQuerySet(otherDevice, wgpu::QueryType::Timestamp, 2); wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - EncodeRenderPassWithTimestampWrites( + ASSERT_DEVICE_ERROR(EncodeRenderPassWithTimestampWrites( encoder, {{querySet, 0, wgpu::RenderPassTimestampLocation::Beginning}, - {querySetFromOtherDevice, 1, wgpu::RenderPassTimestampLocation::End}}); - ASSERT_DEVICE_ERROR(encoder.Finish()); + {querySetFromOtherDevice, 1, wgpu::RenderPassTimestampLocation::End}})); } // Fail to write timestamps to the query index which exceeds the number of queries in query set { wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - EncodeRenderPassWithTimestampWrites( - encoder, {{querySet, 2, wgpu::RenderPassTimestampLocation::Beginning}}); - ASSERT_DEVICE_ERROR(encoder.Finish()); + ASSERT_DEVICE_ERROR(EncodeRenderPassWithTimestampWrites( + encoder, {{querySet, 2, wgpu::RenderPassTimestampLocation::Beginning}})); } // Success to write timestamps to the same query index and location twice on different render @@ -494,27 +483,24 @@ TEST_F(TimestampQueryValidationTest, TimestampWritesOnRenderPass) { // Fail to write timestamps to the same query index twice on same render pass { wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - EncodeRenderPassWithTimestampWrites( + ASSERT_DEVICE_ERROR(EncodeRenderPassWithTimestampWrites( encoder, {{querySet, 0, wgpu::RenderPassTimestampLocation::Beginning}, - {querySet, 0, wgpu::RenderPassTimestampLocation::End}}); - ASSERT_DEVICE_ERROR(encoder.Finish()); + {querySet, 0, wgpu::RenderPassTimestampLocation::End}})); } // Fail to write timestamps at same location of a render pass { wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - EncodeRenderPassWithTimestampWrites( + ASSERT_DEVICE_ERROR(EncodeRenderPassWithTimestampWrites( encoder, {{querySet, 0, wgpu::RenderPassTimestampLocation::Beginning}, - {querySet, 1, wgpu::RenderPassTimestampLocation::Beginning}}); - ASSERT_DEVICE_ERROR(encoder.Finish()); + {querySet, 1, wgpu::RenderPassTimestampLocation::Beginning}})); } // Fail to write timestamps at invalid location of render pass { wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - EncodeRenderPassWithTimestampWrites( - encoder, {{querySet, 0, static_cast(0xFFFFFFFF)}}); - ASSERT_DEVICE_ERROR(encoder.Finish()); + ASSERT_DEVICE_ERROR(EncodeRenderPassWithTimestampWrites( + encoder, {{querySet, 0, static_cast(0xFFFFFFFF)}})); } // Fail to write timestamps to a destroyed query set @@ -785,8 +771,7 @@ TEST_F(PipelineStatisticsQueryValidationTest, BeginRenderPassWithPipelineStatist renderPass.occlusionQuerySet = querySet; wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - encoder.BeginRenderPass(&renderPass); - ASSERT_DEVICE_ERROR(encoder.Finish()); + ASSERT_DEVICE_ERROR(encoder.BeginRenderPass(&renderPass)); } class ResolveQuerySetValidationTest : public QuerySetValidationTest { diff --git a/src/dawn/tests/unittests/validation/RenderPassDescriptorValidationTests.cpp b/src/dawn/tests/unittests/validation/RenderPassDescriptorValidationTests.cpp index f0cfd3c8cd..87b292f325 100644 --- a/src/dawn/tests/unittests/validation/RenderPassDescriptorValidationTests.cpp +++ b/src/dawn/tests/unittests/validation/RenderPassDescriptorValidationTests.cpp @@ -27,20 +27,14 @@ namespace { class RenderPassDescriptorValidationTest : public ValidationTest { public: void AssertBeginRenderPassSuccess(const wgpu::RenderPassDescriptor* descriptor) { - wgpu::CommandEncoder commandEncoder = TestBeginRenderPass(descriptor); - commandEncoder.Finish(); - } - void AssertBeginRenderPassError(const wgpu::RenderPassDescriptor* descriptor) { - wgpu::CommandEncoder commandEncoder = TestBeginRenderPass(descriptor); - ASSERT_DEVICE_ERROR(commandEncoder.Finish()); - } - - private: - wgpu::CommandEncoder TestBeginRenderPass(const wgpu::RenderPassDescriptor* descriptor) { wgpu::CommandEncoder commandEncoder = device.CreateCommandEncoder(); wgpu::RenderPassEncoder renderPassEncoder = commandEncoder.BeginRenderPass(descriptor); renderPassEncoder.End(); - return commandEncoder; + commandEncoder.Finish(); + } + void AssertBeginRenderPassError(const wgpu::RenderPassDescriptor* descriptor) { + wgpu::CommandEncoder commandEncoder = device.CreateCommandEncoder(); + ASSERT_DEVICE_ERROR(commandEncoder.BeginRenderPass(descriptor)); } }; diff --git a/src/dawn/tests/unittests/validation/RenderPipelineValidationTests.cpp b/src/dawn/tests/unittests/validation/RenderPipelineValidationTests.cpp index 675b1d950a..927f05b485 100644 --- a/src/dawn/tests/unittests/validation/RenderPipelineValidationTests.cpp +++ b/src/dawn/tests/unittests/validation/RenderPipelineValidationTests.cpp @@ -710,11 +710,7 @@ TEST_F(RenderPipelineValidationTest, VertexOnlyPipelineRequireDepthStencilAttach utils::ComboRenderPassDescriptor renderPassDescriptor({}); wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - wgpu::RenderPassEncoder renderPass = encoder.BeginRenderPass(&renderPassDescriptor); - renderPass.SetPipeline(vertexOnlyPipeline); - renderPass.End(); - - ASSERT_DEVICE_ERROR(encoder.Finish()); + ASSERT_DEVICE_ERROR(encoder.BeginRenderPass(&renderPassDescriptor)); } // Vertex-only render pipeline can not work with color target diff --git a/webgpu-cts/expectations.txt b/webgpu-cts/expectations.txt index 956ce88eb3..66dc69758c 100644 --- a/webgpu-cts/expectations.txt +++ b/webgpu-cts/expectations.txt @@ -445,6 +445,19 @@ crbug.com/dawn/1599 [ webgpu-adapter-swiftshader ] webgpu:web_platform,copyToTex crbug.com/dawn/1082 webgpu:web_platform,external_texture,video:* [ Skip ] ################################################################################ +################################################################################ +# commandEncoder validation tests need updates in CTS +################################################################################ +crbug.com/dawn/1602 webgpu:api,validation,encoding,beginComputePass:* [ Skip ] +crbug.com/dawn/1602 webgpu:api,validation,encoding,beginRenderPass:* [ Skip ] +crbug.com/dawn/1602 webgpu:api,validation,encoding,encoder_open_state:non_pass_commands:* [ Skip ] +crbug.com/dawn/1602 webgpu:api,validation,encoding,queries,general:occlusion_query,* [ Skip ] +crbug.com/dawn/1602 webgpu:api,validation,render_pass,render_pass_descriptor:* [ Skip ] +crbug.com/dawn/1602 webgpu:api,validation,render_pass,resolve:resolve_attachment:* [ Skip ] +crbug.com/dawn/1602 webgpu:api,validation,render_pass,storeOp:* [ Skip ] +crbug.com/dawn/1602 webgpu:api,validation,resource_usages,texture,in_render_misc:subresources,set_bind_group_on_same_index_depth_stencil_texture:* [ Skip ] +################################################################################ + ################################################################################ # untriaged failures # KEEP