dawn_native: Do debug marker validation at encoding time. This also adds missing coverage for push/pop debug group in render bundles. The RenderBundleEncoder didn't validate itself on Finish, so add a regression test for that too. The overarching goal with this CL is to do validation at encoding time which will help produce SyncScopeResourceUsage in the frontend for dispatch() calls so that they can be reused by the backends. Bug: dawn:635 Change-Id: Ie5a2d987fda3854b3145ba4b7a34994ea605e820 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/38842 Commit-Queue: Corentin Wallez <cwallez@chromium.org> Auto-Submit: Corentin Wallez <cwallez@chromium.org> Reviewed-by: Stephen White <senorblanco@chromium.org>
diff --git a/src/dawn_native/CommandEncoder.cpp b/src/dawn_native/CommandEncoder.cpp index 3c22525..5ccfdc3d 100644 --- a/src/dawn_native/CommandEncoder.cpp +++ b/src/dawn_native/CommandEncoder.cpp
@@ -790,7 +790,13 @@ void CommandEncoder::PopDebugGroup() { mEncodingContext.TryEncode(this, [&](CommandAllocator* allocator) -> MaybeError { + if (GetDevice()->IsValidationEnabled()) { + if (mDebugGroupStackSize == 0) { + return DAWN_VALIDATION_ERROR("Pop must be balanced by a corresponding Push."); + } + } allocator->Allocate<PopDebugGroupCmd>(Command::PopDebugGroup); + mDebugGroupStackSize--; return {}; }); @@ -805,6 +811,8 @@ char* label = allocator->AllocateData<char>(cmd->length + 1); memcpy(label, groupLabel, cmd->length + 1); + mDebugGroupStackSize++; + return {}; }); } @@ -890,7 +898,9 @@ DAWN_TRY(ValidatePassResourceUsage(passUsage)); } - uint64_t debugGroupStackSize = 0; + if (mDebugGroupStackSize != 0) { + return DAWN_VALIDATION_ERROR("Each Push must be balanced by a corresponding Pop."); + } commands->Reset(); Command type; @@ -936,15 +946,12 @@ case Command::PopDebugGroup: { commands->NextCommand<PopDebugGroupCmd>(); - DAWN_TRY(ValidateCanPopDebugGroup(debugGroupStackSize)); - debugGroupStackSize--; break; } case Command::PushDebugGroup: { const PushDebugGroupCmd* cmd = commands->NextCommand<PushDebugGroupCmd>(); commands->NextData<char>(cmd->length + 1); - debugGroupStackSize++; break; } @@ -962,8 +969,6 @@ } } - DAWN_TRY(ValidateFinalDebugGroupStackSize(debugGroupStackSize)); - return {}; }
diff --git a/src/dawn_native/CommandEncoder.h b/src/dawn_native/CommandEncoder.h index cb02521..f54888f 100644 --- a/src/dawn_native/CommandEncoder.h +++ b/src/dawn_native/CommandEncoder.h
@@ -84,6 +84,8 @@ std::set<TextureBase*> mTopLevelTextures; std::set<QuerySetBase*> mUsedQuerySets; QueryAvailabilityMap mQueryAvailabilityMap; + + uint64_t mDebugGroupStackSize = 0; }; } // namespace dawn_native
diff --git a/src/dawn_native/CommandValidation.cpp b/src/dawn_native/CommandValidation.cpp index e0093dd..d28bf2c 100644 --- a/src/dawn_native/CommandValidation.cpp +++ b/src/dawn_native/CommandValidation.cpp
@@ -33,7 +33,6 @@ Command type, CommandBufferStateTracker* commandBufferState, const AttachmentState* attachmentState, - uint64_t* debugGroupStackSize, const char* disallowedMessage) { switch (type) { case Command::Draw: { @@ -68,15 +67,12 @@ case Command::PopDebugGroup: { commands->NextCommand<PopDebugGroupCmd>(); - DAWN_TRY(ValidateCanPopDebugGroup(*debugGroupStackSize)); - *debugGroupStackSize -= 1; break; } case Command::PushDebugGroup: { PushDebugGroupCmd* cmd = commands->NextCommand<PushDebugGroupCmd>(); commands->NextData<char>(cmd->length + 1); - *debugGroupStackSize += 1; break; } @@ -122,40 +118,21 @@ } // namespace - MaybeError ValidateCanPopDebugGroup(uint64_t debugGroupStackSize) { - if (debugGroupStackSize == 0) { - return DAWN_VALIDATION_ERROR("Pop must be balanced by a corresponding Push."); - } - return {}; - } - - MaybeError ValidateFinalDebugGroupStackSize(uint64_t debugGroupStackSize) { - if (debugGroupStackSize != 0) { - return DAWN_VALIDATION_ERROR("Each Push must be balanced by a corresponding Pop."); - } - return {}; - } - MaybeError ValidateRenderBundle(CommandIterator* commands, const AttachmentState* attachmentState) { CommandBufferStateTracker commandBufferState; - uint64_t debugGroupStackSize = 0; - Command type; while (commands->NextCommandId(&type)) { DAWN_TRY(ValidateRenderBundleCommand(commands, type, &commandBufferState, - attachmentState, &debugGroupStackSize, + attachmentState, "Command disallowed inside a render bundle")); } - DAWN_TRY(ValidateFinalDebugGroupStackSize(debugGroupStackSize)); return {}; } MaybeError ValidateRenderPass(CommandIterator* commands, const BeginRenderPassCmd* renderPass) { CommandBufferStateTracker commandBufferState; - uint64_t debugGroupStackSize = 0; - Command type; while (commands->NextCommandId(&type)) { switch (type) { @@ -171,7 +148,6 @@ case Command::EndRenderPass: { commands->NextCommand<EndRenderPassCmd>(); - DAWN_TRY(ValidateFinalDebugGroupStackSize(debugGroupStackSize)); return {}; } @@ -222,7 +198,7 @@ default: DAWN_TRY(ValidateRenderBundleCommand( commands, type, &commandBufferState, renderPass->attachmentState.Get(), - &debugGroupStackSize, "Command disallowed inside a render pass")); + "Command disallowed inside a render pass")); } } @@ -232,14 +208,11 @@ MaybeError ValidateComputePass(CommandIterator* commands) { CommandBufferStateTracker commandBufferState; - uint64_t debugGroupStackSize = 0; - Command type; while (commands->NextCommandId(&type)) { switch (type) { case Command::EndComputePass: { commands->NextCommand<EndComputePassCmd>(); - DAWN_TRY(ValidateFinalDebugGroupStackSize(debugGroupStackSize)); return {}; } @@ -263,15 +236,12 @@ case Command::PopDebugGroup: { commands->NextCommand<PopDebugGroupCmd>(); - DAWN_TRY(ValidateCanPopDebugGroup(debugGroupStackSize)); - debugGroupStackSize--; break; } case Command::PushDebugGroup: { PushDebugGroupCmd* cmd = commands->NextCommand<PushDebugGroupCmd>(); commands->NextData<char>(cmd->length + 1); - debugGroupStackSize++; break; }
diff --git a/src/dawn_native/CommandValidation.h b/src/dawn_native/CommandValidation.h index d2794e2..d1deed4 100644 --- a/src/dawn_native/CommandValidation.h +++ b/src/dawn_native/CommandValidation.h
@@ -29,9 +29,6 @@ struct PassResourceUsage; struct TexelBlockInfo; - MaybeError ValidateCanPopDebugGroup(uint64_t debugGroupStackSize); - MaybeError ValidateFinalDebugGroupStackSize(uint64_t debugGroupStackSize); - MaybeError ValidateRenderBundle(CommandIterator* commands, const AttachmentState* attachmentState); MaybeError ValidateRenderPass(CommandIterator* commands, const BeginRenderPassCmd* renderPass);
diff --git a/src/dawn_native/ComputePassEncoder.cpp b/src/dawn_native/ComputePassEncoder.cpp index 37c1322..ac8eb09 100644 --- a/src/dawn_native/ComputePassEncoder.cpp +++ b/src/dawn_native/ComputePassEncoder.cpp
@@ -47,6 +47,10 @@ void ComputePassEncoder::EndPass() { if (mEncodingContext->TryEncode(this, [&](CommandAllocator* allocator) -> MaybeError { + if (IsValidationEnabled()) { + DAWN_TRY(ValidateProgrammableEncoderEnd()); + } + allocator->Allocate<EndComputePassCmd>(Command::EndComputePass); return {};
diff --git a/src/dawn_native/ProgrammablePassEncoder.cpp b/src/dawn_native/ProgrammablePassEncoder.cpp index 32c7bad7..81d7868 100644 --- a/src/dawn_native/ProgrammablePassEncoder.cpp +++ b/src/dawn_native/ProgrammablePassEncoder.cpp
@@ -97,13 +97,20 @@ : ObjectBase(device, errorTag), mEncodingContext(encodingContext), mUsageTracker(passType), - mValidationEnabled(false) { + mValidationEnabled(device->IsValidationEnabled()) { } bool ProgrammablePassEncoder::IsValidationEnabled() const { return mValidationEnabled; } + MaybeError ProgrammablePassEncoder::ValidateProgrammableEncoderEnd() const { + if (mDebugGroupStackSize != 0) { + return DAWN_VALIDATION_ERROR("Each Push must be balanced by a corresponding Pop."); + } + return {}; + } + void ProgrammablePassEncoder::InsertDebugMarker(const char* groupLabel) { mEncodingContext->TryEncode(this, [&](CommandAllocator* allocator) -> MaybeError { InsertDebugMarkerCmd* cmd = @@ -119,7 +126,13 @@ void ProgrammablePassEncoder::PopDebugGroup() { mEncodingContext->TryEncode(this, [&](CommandAllocator* allocator) -> MaybeError { + if (IsValidationEnabled()) { + if (mDebugGroupStackSize == 0) { + return DAWN_VALIDATION_ERROR("Pop must be balanced by a corresponding Push."); + } + } allocator->Allocate<PopDebugGroupCmd>(Command::PopDebugGroup); + mDebugGroupStackSize--; return {}; }); @@ -134,6 +147,8 @@ char* label = allocator->AllocateData<char>(cmd->length + 1); memcpy(label, groupLabel, cmd->length + 1); + mDebugGroupStackSize++; + return {}; }); }
diff --git a/src/dawn_native/ProgrammablePassEncoder.h b/src/dawn_native/ProgrammablePassEncoder.h index 9f1c56f..6d6f067 100644 --- a/src/dawn_native/ProgrammablePassEncoder.h +++ b/src/dawn_native/ProgrammablePassEncoder.h
@@ -45,6 +45,7 @@ protected: bool IsValidationEnabled() const; + MaybeError ValidateProgrammableEncoderEnd() const; // Construct an "error" programmable pass encoder. ProgrammablePassEncoder(DeviceBase* device, @@ -55,6 +56,8 @@ EncodingContext* mEncodingContext = nullptr; PassResourceUsageTracker mUsageTracker; + uint64_t mDebugGroupStackSize = 0; + private: const bool mValidationEnabled; };
diff --git a/src/dawn_native/RenderBundleEncoder.cpp b/src/dawn_native/RenderBundleEncoder.cpp index 4970042..aedcfff 100644 --- a/src/dawn_native/RenderBundleEncoder.cpp +++ b/src/dawn_native/RenderBundleEncoder.cpp
@@ -103,19 +103,29 @@ } RenderBundleBase* RenderBundleEncoder::Finish(const RenderBundleDescriptor* descriptor) { - PassResourceUsage usages = mUsageTracker.AcquireResourceUsage(); + RenderBundleBase* result = nullptr; - DeviceBase* device = GetDevice(); + if (GetDevice()->ConsumedError(FinishImpl(descriptor), &result)) { + return RenderBundleBase::MakeError(GetDevice()); + } + + return result; + } + + ResultOrError<RenderBundleBase*> RenderBundleEncoder::FinishImpl( + const RenderBundleDescriptor* descriptor) { // Even if mBundleEncodingContext.Finish() validation fails, calling it will mutate the // internal state of the encoding context. Subsequent calls to encode commands will generate // errors. - if (device->ConsumedError(mBundleEncodingContext.Finish()) || - (device->IsValidationEnabled() && - device->ConsumedError(ValidateFinish(mBundleEncodingContext.GetIterator(), usages)))) { - return RenderBundleBase::MakeError(device); + DAWN_TRY(mBundleEncodingContext.Finish()); + + PassResourceUsage usages = mUsageTracker.AcquireResourceUsage(); + if (IsValidationEnabled()) { + DAWN_TRY(GetDevice()->ValidateObject(this)); + DAWN_TRY(ValidateProgrammableEncoderEnd()); + DAWN_TRY(ValidateFinish(mBundleEncodingContext.GetIterator(), usages)); } - ASSERT(!IsError()); return new RenderBundleBase(this, descriptor, mAttachmentState.Get(), std::move(usages)); }
diff --git a/src/dawn_native/RenderBundleEncoder.h b/src/dawn_native/RenderBundleEncoder.h index e6354ab..bc9aaac 100644 --- a/src/dawn_native/RenderBundleEncoder.h +++ b/src/dawn_native/RenderBundleEncoder.h
@@ -42,6 +42,7 @@ private: RenderBundleEncoder(DeviceBase* device, ErrorTag errorTag); + ResultOrError<RenderBundleBase*> FinishImpl(const RenderBundleDescriptor* descriptor); MaybeError ValidateFinish(CommandIterator* commands, const PassResourceUsage& usages) const; EncodingContext mBundleEncodingContext;
diff --git a/src/dawn_native/RenderPassEncoder.cpp b/src/dawn_native/RenderPassEncoder.cpp index 1b13807..d7f168e 100644 --- a/src/dawn_native/RenderPassEncoder.cpp +++ b/src/dawn_native/RenderPassEncoder.cpp
@@ -94,15 +94,15 @@ void RenderPassEncoder::EndPass() { if (mEncodingContext->TryEncode(this, [&](CommandAllocator* allocator) -> MaybeError { - allocator->Allocate<EndRenderPassCmd>(Command::EndRenderPass); - if (IsValidationEnabled()) { + DAWN_TRY(ValidateProgrammableEncoderEnd()); if (mOcclusionQueryActive) { return DAWN_VALIDATION_ERROR( "The occlusion query must be ended before endPass."); } } + allocator->Allocate<EndRenderPassCmd>(Command::EndRenderPass); return {}; })) { mEncodingContext->ExitPass(this, mUsageTracker.AcquireResourceUsage());
diff --git a/src/tests/unittests/validation/DebugMarkerValidationTests.cpp b/src/tests/unittests/validation/DebugMarkerValidationTests.cpp index 49520cb..6d27aba 100644 --- a/src/tests/unittests/validation/DebugMarkerValidationTests.cpp +++ b/src/tests/unittests/validation/DebugMarkerValidationTests.cpp
@@ -14,6 +14,7 @@ #include "tests/unittests/validation/ValidationTest.h" +#include "utils/ComboRenderBundleEncoderDescriptor.h" #include "utils/WGPUHelpers.h" class DebugMarkerValidationTest : public ValidationTest {}; @@ -70,6 +71,52 @@ ASSERT_DEVICE_ERROR(encoder.Finish()); } +// Correct usage of debug markers should succeed in render bundle. +TEST_F(DebugMarkerValidationTest, RenderBundleSuccess) { + utils::ComboRenderBundleEncoderDescriptor desc; + desc.cColorFormats[0] = wgpu::TextureFormat::RGBA8Unorm; + desc.colorFormatsCount = 1; + + wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&desc); + encoder.PushDebugGroup("Event Start"); + encoder.PushDebugGroup("Event Start"); + encoder.InsertDebugMarker("Marker"); + encoder.PopDebugGroup(); + encoder.PopDebugGroup(); + + encoder.Finish(); +} + +// A PushDebugGroup call without a following PopDebugGroup produces an error in render bundle. +TEST_F(DebugMarkerValidationTest, RenderBundleUnbalancedPush) { + utils::ComboRenderBundleEncoderDescriptor desc; + desc.cColorFormats[0] = wgpu::TextureFormat::RGBA8Unorm; + desc.colorFormatsCount = 1; + + wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&desc); + encoder.PushDebugGroup("Event Start"); + encoder.PushDebugGroup("Event Start"); + encoder.InsertDebugMarker("Marker"); + encoder.PopDebugGroup(); + + ASSERT_DEVICE_ERROR(encoder.Finish()); +} + +// A PopDebugGroup call without a preceding PushDebugGroup produces an error in render bundle. +TEST_F(DebugMarkerValidationTest, RenderBundleUnbalancedPop) { + utils::ComboRenderBundleEncoderDescriptor desc; + desc.cColorFormats[0] = wgpu::TextureFormat::RGBA8Unorm; + desc.colorFormatsCount = 1; + + wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&desc); + encoder.PushDebugGroup("Event Start"); + encoder.InsertDebugMarker("Marker"); + encoder.PopDebugGroup(); + encoder.PopDebugGroup(); + + ASSERT_DEVICE_ERROR(encoder.Finish()); +} + // Correct usage of debug markers should succeed in compute pass. TEST_F(DebugMarkerValidationTest, ComputeSuccess) { wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
diff --git a/src/tests/unittests/validation/RenderBundleValidationTests.cpp b/src/tests/unittests/validation/RenderBundleValidationTests.cpp index 5490a15..c844912 100644 --- a/src/tests/unittests/validation/RenderBundleValidationTests.cpp +++ b/src/tests/unittests/validation/RenderBundleValidationTests.cpp
@@ -141,6 +141,28 @@ commandEncoder.Finish(); } +// Test that an empty error bundle encoder produces an error bundle. +// This is a regression test for error render bundle encoders containing no commands would +// produce non-error render bundles. +TEST_F(RenderBundleValidationTest, EmptyErrorEncoderProducesErrorBundle) { + DummyRenderPass renderPass(device); + + utils::ComboRenderBundleEncoderDescriptor desc = {}; + // Having 0 attachments is invalid! + desc.colorFormatsCount = 0; + + wgpu::RenderBundleEncoder renderBundleEncoder; + ASSERT_DEVICE_ERROR(renderBundleEncoder = device.CreateRenderBundleEncoder(&desc)); + wgpu::RenderBundle renderBundle; + ASSERT_DEVICE_ERROR(renderBundle = renderBundleEncoder.Finish()); + + wgpu::CommandEncoder commandEncoder = device.CreateCommandEncoder(); + wgpu::RenderPassEncoder pass = commandEncoder.BeginRenderPass(&renderPass); + pass.ExecuteBundles(1, &renderBundle); + pass.EndPass(); + ASSERT_DEVICE_ERROR(commandEncoder.Finish()); +} + // Test executing zero render bundles. TEST_F(RenderBundleValidationTest, ZeroBundles) { DummyRenderPass renderPass(device);