diff --git a/src/dawn_native/CommandEncoder.cpp b/src/dawn_native/CommandEncoder.cpp index 3c22525cea..5ccfdc3d1d 100644 --- a/src/dawn_native/CommandEncoder.cpp +++ b/src/dawn_native/CommandEncoder.cpp @@ -790,7 +790,13 @@ namespace dawn_native { 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(Command::PopDebugGroup); + mDebugGroupStackSize--; return {}; }); @@ -805,6 +811,8 @@ namespace dawn_native { char* label = allocator->AllocateData(cmd->length + 1); memcpy(label, groupLabel, cmd->length + 1); + mDebugGroupStackSize++; + return {}; }); } @@ -890,7 +898,9 @@ namespace dawn_native { 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 @@ namespace dawn_native { case Command::PopDebugGroup: { commands->NextCommand(); - DAWN_TRY(ValidateCanPopDebugGroup(debugGroupStackSize)); - debugGroupStackSize--; break; } case Command::PushDebugGroup: { const PushDebugGroupCmd* cmd = commands->NextCommand(); commands->NextData(cmd->length + 1); - debugGroupStackSize++; break; } @@ -962,8 +969,6 @@ namespace dawn_native { } } - DAWN_TRY(ValidateFinalDebugGroupStackSize(debugGroupStackSize)); - return {}; } diff --git a/src/dawn_native/CommandEncoder.h b/src/dawn_native/CommandEncoder.h index cb02521f89..f54888f60a 100644 --- a/src/dawn_native/CommandEncoder.h +++ b/src/dawn_native/CommandEncoder.h @@ -84,6 +84,8 @@ namespace dawn_native { std::set mTopLevelTextures; std::set 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 e0093dd4b3..d28bf2cc9f 100644 --- a/src/dawn_native/CommandValidation.cpp +++ b/src/dawn_native/CommandValidation.cpp @@ -33,7 +33,6 @@ namespace dawn_native { Command type, CommandBufferStateTracker* commandBufferState, const AttachmentState* attachmentState, - uint64_t* debugGroupStackSize, const char* disallowedMessage) { switch (type) { case Command::Draw: { @@ -68,15 +67,12 @@ namespace dawn_native { case Command::PopDebugGroup: { commands->NextCommand(); - DAWN_TRY(ValidateCanPopDebugGroup(*debugGroupStackSize)); - *debugGroupStackSize -= 1; break; } case Command::PushDebugGroup: { PushDebugGroupCmd* cmd = commands->NextCommand(); commands->NextData(cmd->length + 1); - *debugGroupStackSize += 1; break; } @@ -122,40 +118,21 @@ namespace dawn_native { } // 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 @@ namespace dawn_native { case Command::EndRenderPass: { commands->NextCommand(); - DAWN_TRY(ValidateFinalDebugGroupStackSize(debugGroupStackSize)); return {}; } @@ -222,7 +198,7 @@ namespace dawn_native { 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 @@ namespace dawn_native { MaybeError ValidateComputePass(CommandIterator* commands) { CommandBufferStateTracker commandBufferState; - uint64_t debugGroupStackSize = 0; - Command type; while (commands->NextCommandId(&type)) { switch (type) { case Command::EndComputePass: { commands->NextCommand(); - DAWN_TRY(ValidateFinalDebugGroupStackSize(debugGroupStackSize)); return {}; } @@ -263,15 +236,12 @@ namespace dawn_native { case Command::PopDebugGroup: { commands->NextCommand(); - DAWN_TRY(ValidateCanPopDebugGroup(debugGroupStackSize)); - debugGroupStackSize--; break; } case Command::PushDebugGroup: { PushDebugGroupCmd* cmd = commands->NextCommand(); commands->NextData(cmd->length + 1); - debugGroupStackSize++; break; } diff --git a/src/dawn_native/CommandValidation.h b/src/dawn_native/CommandValidation.h index d2794e21cd..d1deed48da 100644 --- a/src/dawn_native/CommandValidation.h +++ b/src/dawn_native/CommandValidation.h @@ -29,9 +29,6 @@ namespace dawn_native { 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 37c1322a0c..ac8eb09013 100644 --- a/src/dawn_native/ComputePassEncoder.cpp +++ b/src/dawn_native/ComputePassEncoder.cpp @@ -47,6 +47,10 @@ namespace dawn_native { void ComputePassEncoder::EndPass() { if (mEncodingContext->TryEncode(this, [&](CommandAllocator* allocator) -> MaybeError { + if (IsValidationEnabled()) { + DAWN_TRY(ValidateProgrammableEncoderEnd()); + } + allocator->Allocate(Command::EndComputePass); return {}; diff --git a/src/dawn_native/ProgrammablePassEncoder.cpp b/src/dawn_native/ProgrammablePassEncoder.cpp index 32c7bad75d..81d7868351 100644 --- a/src/dawn_native/ProgrammablePassEncoder.cpp +++ b/src/dawn_native/ProgrammablePassEncoder.cpp @@ -97,13 +97,20 @@ namespace dawn_native { : 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 @@ namespace dawn_native { 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(Command::PopDebugGroup); + mDebugGroupStackSize--; return {}; }); @@ -134,6 +147,8 @@ namespace dawn_native { char* label = allocator->AllocateData(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 9f1c56fa85..6d6f067d72 100644 --- a/src/dawn_native/ProgrammablePassEncoder.h +++ b/src/dawn_native/ProgrammablePassEncoder.h @@ -45,6 +45,7 @@ namespace dawn_native { protected: bool IsValidationEnabled() const; + MaybeError ValidateProgrammableEncoderEnd() const; // Construct an "error" programmable pass encoder. ProgrammablePassEncoder(DeviceBase* device, @@ -55,6 +56,8 @@ namespace dawn_native { 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 49700429f3..aedcfffbab 100644 --- a/src/dawn_native/RenderBundleEncoder.cpp +++ b/src/dawn_native/RenderBundleEncoder.cpp @@ -103,19 +103,29 @@ namespace dawn_native { } 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 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 e6354ab042..bc9aaacfae 100644 --- a/src/dawn_native/RenderBundleEncoder.h +++ b/src/dawn_native/RenderBundleEncoder.h @@ -42,6 +42,7 @@ namespace dawn_native { private: RenderBundleEncoder(DeviceBase* device, ErrorTag errorTag); + ResultOrError 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 1b13807ff8..d7f168e04b 100644 --- a/src/dawn_native/RenderPassEncoder.cpp +++ b/src/dawn_native/RenderPassEncoder.cpp @@ -94,15 +94,15 @@ namespace dawn_native { void RenderPassEncoder::EndPass() { if (mEncodingContext->TryEncode(this, [&](CommandAllocator* allocator) -> MaybeError { - allocator->Allocate(Command::EndRenderPass); - if (IsValidationEnabled()) { + DAWN_TRY(ValidateProgrammableEncoderEnd()); if (mOcclusionQueryActive) { return DAWN_VALIDATION_ERROR( "The occlusion query must be ended before endPass."); } } + allocator->Allocate(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 49520cb9d7..6d27abadcc 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 @@ TEST_F(DebugMarkerValidationTest, RenderUnbalancedPop) { 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 5490a154a3..c8449127d1 100644 --- a/src/tests/unittests/validation/RenderBundleValidationTests.cpp +++ b/src/tests/unittests/validation/RenderBundleValidationTests.cpp @@ -141,6 +141,28 @@ TEST_F(RenderBundleValidationTest, Empty) { 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);