From 679ff4ea86547939ac56106933dcb1a866c9a665 Mon Sep 17 00:00:00 2001 From: Corentin Wallez Date: Wed, 7 Nov 2018 10:02:43 +0000 Subject: [PATCH] Validate that mapped buffers aren't used in submits. Likewise "presented" textures won't be available for use in submits so this sets up state-tracking and validation for textures too. Command buffer resource usage is now stored in the frontend instead of done per-backend because it is used to validate resources are allowed in the submits. Also adds a test. BUG=dawn:9 Change-Id: I0537c5113bb33a089509b4f2af4ddf4eff8051ea Reviewed-on: https://dawn-review.googlesource.com/c/2142 Reviewed-by: Corentin Wallez Commit-Queue: Corentin Wallez --- BUILD.gn | 1 + src/dawn_native/Buffer.cpp | 7 ++ src/dawn_native/Buffer.h | 2 + src/dawn_native/CommandBuffer.cpp | 27 ++++++-- src/dawn_native/CommandBuffer.h | 11 ++- src/dawn_native/PassResourceUsage.h | 7 ++ src/dawn_native/Queue.cpp | 24 ++++++- src/dawn_native/Texture.cpp | 4 ++ src/dawn_native/Texture.h | 2 + src/dawn_native/d3d12/CommandBufferD3D12.cpp | 9 ++- src/dawn_native/d3d12/CommandBufferD3D12.h | 1 - src/dawn_native/vulkan/CommandBufferVk.cpp | 9 ++- src/dawn_native/vulkan/CommandBufferVk.h | 1 - .../validation/QueueSubmitValidationTests.cpp | 69 +++++++++++++++++++ 14 files changed, 151 insertions(+), 23 deletions(-) create mode 100644 src/tests/unittests/validation/QueueSubmitValidationTests.cpp diff --git a/BUILD.gn b/BUILD.gn index e235a3b14b..cc743e10e4 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -778,6 +778,7 @@ test("dawn_unittests") { "src/tests/unittests/validation/DynamicStateCommandValidationTests.cpp", "src/tests/unittests/validation/InputStateValidationTests.cpp", "src/tests/unittests/validation/PushConstantsValidationTests.cpp", + "src/tests/unittests/validation/QueueSubmitValidationTests.cpp", "src/tests/unittests/validation/RenderPassDescriptorValidationTests.cpp", "src/tests/unittests/validation/RenderPipelineValidationTests.cpp", "src/tests/unittests/validation/ShaderModuleValidationTests.cpp", diff --git a/src/dawn_native/Buffer.cpp b/src/dawn_native/Buffer.cpp index 145ed6e641..e073491144 100644 --- a/src/dawn_native/Buffer.cpp +++ b/src/dawn_native/Buffer.cpp @@ -72,6 +72,13 @@ namespace dawn_native { return mUsage; } + MaybeError BufferBase::ValidateCanUseInSubmitNow() const { + if (mIsMapped) { + return DAWN_VALIDATION_ERROR("Buffer used in a submit while mapped"); + } + return {}; + } + void BufferBase::CallMapReadCallback(uint32_t serial, dawnBufferMapAsyncStatus status, const void* pointer) { diff --git a/src/dawn_native/Buffer.h b/src/dawn_native/Buffer.h index a80032f87f..ce10ea5b61 100644 --- a/src/dawn_native/Buffer.h +++ b/src/dawn_native/Buffer.h @@ -42,6 +42,8 @@ namespace dawn_native { uint32_t GetSize() const; dawn::BufferUsageBit GetUsage() const; + MaybeError ValidateCanUseInSubmitNow() const; + // Dawn API BufferViewBuilder* CreateBufferViewBuilder(); void SetSubData(uint32_t start, uint32_t count, const uint8_t* data); diff --git a/src/dawn_native/CommandBuffer.cpp b/src/dawn_native/CommandBuffer.cpp index 919f72c255..2a9ffe5ad1 100644 --- a/src/dawn_native/CommandBuffer.cpp +++ b/src/dawn_native/CommandBuffer.cpp @@ -286,7 +286,11 @@ namespace dawn_native { // CommandBuffer CommandBufferBase::CommandBufferBase(CommandBufferBuilder* builder) - : ObjectBase(builder->GetDevice()) { + : ObjectBase(builder->GetDevice()), mResourceUsages(builder->AcquireResourceUsages()) { + } + + const CommandBufferResourceUsage& CommandBufferBase::GetResourceUsages() const { + return mResourceUsages; } // CommandBufferBuilder @@ -308,10 +312,10 @@ namespace dawn_native { return std::move(mIterator); } - std::vector CommandBufferBuilder::AcquirePassResourceUsage() { - ASSERT(!mWerePassUsagesAcquired); - mWerePassUsagesAcquired = true; - return std::move(mPassResourceUsages); + CommandBufferResourceUsage CommandBufferBuilder::AcquireResourceUsages() { + ASSERT(!mWereResourceUsagesAcquired); + mWereResourceUsagesAcquired = true; + return std::move(mResourceUsages); } CommandBufferBase* CommandBufferBuilder::GetResultImpl() { @@ -372,6 +376,9 @@ namespace dawn_native { dawn::BufferUsageBit::TransferSrc)); DAWN_TRY(ValidateCanUseAs(copy->destination.buffer.Get(), dawn::BufferUsageBit::TransferDst)); + + mResourceUsages.topLevelBuffers.insert(copy->source.buffer.Get()); + mResourceUsages.topLevelBuffers.insert(copy->destination.buffer.Get()); } break; case Command::CopyBufferToTexture: { @@ -391,6 +398,9 @@ namespace dawn_native { dawn::BufferUsageBit::TransferSrc)); DAWN_TRY(ValidateCanUseAs(copy->destination.texture.Get(), dawn::TextureUsageBit::TransferDst)); + + mResourceUsages.topLevelBuffers.insert(copy->source.buffer.Get()); + mResourceUsages.topLevelTextures.insert(copy->destination.texture.Get()); } break; case Command::CopyTextureToBuffer: { @@ -410,6 +420,9 @@ namespace dawn_native { dawn::TextureUsageBit::TransferSrc)); DAWN_TRY(ValidateCanUseAs(copy->destination.buffer.Get(), dawn::BufferUsageBit::TransferDst)); + + mResourceUsages.topLevelTextures.insert(copy->source.texture.Get()); + mResourceUsages.topLevelBuffers.insert(copy->destination.buffer.Get()); } break; default: @@ -431,7 +444,7 @@ namespace dawn_native { mIterator.NextCommand(); DAWN_TRY(usageTracker.ValidateUsages(PassType::Compute)); - mPassResourceUsages.push_back(usageTracker.AcquireResourceUsage()); + mResourceUsages.perPass.push_back(usageTracker.AcquireResourceUsage()); return {}; } break; @@ -495,7 +508,7 @@ namespace dawn_native { mIterator.NextCommand(); DAWN_TRY(usageTracker.ValidateUsages(PassType::Render)); - mPassResourceUsages.push_back(usageTracker.AcquireResourceUsage()); + mResourceUsages.perPass.push_back(usageTracker.AcquireResourceUsage()); return {}; } break; diff --git a/src/dawn_native/CommandBuffer.h b/src/dawn_native/CommandBuffer.h index 553fe6e5a0..3565085b15 100644 --- a/src/dawn_native/CommandBuffer.h +++ b/src/dawn_native/CommandBuffer.h @@ -42,6 +42,11 @@ namespace dawn_native { class CommandBufferBase : public ObjectBase { public: CommandBufferBase(CommandBufferBuilder* builder); + + const CommandBufferResourceUsage& GetResourceUsages() const; + + private: + CommandBufferResourceUsage mResourceUsages; }; class CommandBufferBuilder : public Builder { @@ -52,7 +57,7 @@ namespace dawn_native { MaybeError ValidateGetResult(); CommandIterator AcquireCommands(); - std::vector AcquirePassResourceUsage(); + CommandBufferResourceUsage AcquireResourceUsages(); // Dawn API ComputePassEncoderBase* BeginComputePass(); @@ -116,9 +121,9 @@ namespace dawn_native { CommandIterator mIterator; bool mWasMovedToIterator = false; bool mWereCommandsAcquired = false; - bool mWerePassUsagesAcquired = false; - std::vector mPassResourceUsages; + bool mWereResourceUsagesAcquired = false; + CommandBufferResourceUsage mResourceUsages; }; } // namespace dawn_native diff --git a/src/dawn_native/PassResourceUsage.h b/src/dawn_native/PassResourceUsage.h index 5bae2e46f0..9a8c2b07db 100644 --- a/src/dawn_native/PassResourceUsage.h +++ b/src/dawn_native/PassResourceUsage.h @@ -17,6 +17,7 @@ #include "dawn_native/dawn_platform.h" +#include #include namespace dawn_native { @@ -35,6 +36,12 @@ namespace dawn_native { std::vector textureUsages; }; + struct CommandBufferResourceUsage { + std::vector perPass; + std::set topLevelBuffers; + std::set topLevelTextures; + }; + } // namespace dawn_native #endif // DAWNNATIVE_PASSRESOURCEUSAGE_H diff --git a/src/dawn_native/Queue.cpp b/src/dawn_native/Queue.cpp index 46facf5299..021a095da5 100644 --- a/src/dawn_native/Queue.cpp +++ b/src/dawn_native/Queue.cpp @@ -14,8 +14,10 @@ #include "dawn_native/Queue.h" +#include "dawn_native/Buffer.h" #include "dawn_native/CommandBuffer.h" #include "dawn_native/Device.h" +#include "dawn_native/Texture.h" namespace dawn_native { @@ -32,7 +34,27 @@ namespace dawn_native { SubmitImpl(numCommands, commands); } - MaybeError QueueBase::ValidateSubmit(uint32_t, CommandBufferBase* const*) { + MaybeError QueueBase::ValidateSubmit(uint32_t numCommands, CommandBufferBase* const* commands) { + for (uint32_t i = 0; i < numCommands; ++i) { + const CommandBufferResourceUsage& usages = commands[i]->GetResourceUsages(); + + for (const PassResourceUsage& passUsages : usages.perPass) { + for (const BufferBase* buffer : passUsages.buffers) { + DAWN_TRY(buffer->ValidateCanUseInSubmitNow()); + } + for (const TextureBase* texture : passUsages.textures) { + DAWN_TRY(texture->ValidateCanUseInSubmitNow()); + } + } + + for (const BufferBase* buffer : usages.topLevelBuffers) { + DAWN_TRY(buffer->ValidateCanUseInSubmitNow()); + } + for (const TextureBase* texture : usages.topLevelTextures) { + DAWN_TRY(texture->ValidateCanUseInSubmitNow()); + } + } + return {}; } diff --git a/src/dawn_native/Texture.cpp b/src/dawn_native/Texture.cpp index ffc5da2862..53e8d10f44 100644 --- a/src/dawn_native/Texture.cpp +++ b/src/dawn_native/Texture.cpp @@ -237,6 +237,10 @@ namespace dawn_native { return mUsage; } + MaybeError TextureBase::ValidateCanUseInSubmitNow() const { + return {}; + } + TextureViewBase* TextureBase::CreateDefaultTextureView() { TextureViewDescriptor descriptor = MakeDefaultTextureViewDescriptor(this); return GetDevice()->CreateTextureView(this, &descriptor); diff --git a/src/dawn_native/Texture.h b/src/dawn_native/Texture.h index 1817bc1198..ab34e96315 100644 --- a/src/dawn_native/Texture.h +++ b/src/dawn_native/Texture.h @@ -52,6 +52,8 @@ namespace dawn_native { uint32_t GetNumMipLevels() const; dawn::TextureUsageBit GetUsage() const; + MaybeError ValidateCanUseInSubmitNow() const; + // Dawn API TextureViewBase* CreateDefaultTextureView(); TextureViewBase* CreateTextureView(const TextureViewDescriptor* descriptor); diff --git a/src/dawn_native/d3d12/CommandBufferD3D12.cpp b/src/dawn_native/d3d12/CommandBufferD3D12.cpp index 27bd589192..65e7d6cec3 100644 --- a/src/dawn_native/d3d12/CommandBufferD3D12.cpp +++ b/src/dawn_native/d3d12/CommandBufferD3D12.cpp @@ -234,9 +234,7 @@ namespace dawn_native { namespace d3d12 { } // anonymous namespace CommandBuffer::CommandBuffer(CommandBufferBuilder* builder) - : CommandBufferBase(builder), - mCommands(builder->AcquireCommands()), - mPassResourceUsages(builder->AcquirePassResourceUsage()) { + : CommandBufferBase(builder), mCommands(builder->AcquireCommands()) { } CommandBuffer::~CommandBuffer() { @@ -281,6 +279,7 @@ namespace dawn_native { namespace d3d12 { } }; + const std::vector& passResourceUsages = GetResourceUsages().perPass; uint32_t nextPassNumber = 0; Command type; @@ -289,7 +288,7 @@ namespace dawn_native { namespace d3d12 { case Command::BeginComputePass: { mCommands.NextCommand(); - TransitionForPass(commandList, mPassResourceUsages[nextPassNumber]); + TransitionForPass(commandList, passResourceUsages[nextPassNumber]); bindingTracker.SetInComputePass(true); RecordComputePass(commandList, &bindingTracker); @@ -300,7 +299,7 @@ namespace dawn_native { namespace d3d12 { BeginRenderPassCmd* beginRenderPassCmd = mCommands.NextCommand(); - TransitionForPass(commandList, mPassResourceUsages[nextPassNumber]); + TransitionForPass(commandList, passResourceUsages[nextPassNumber]); bindingTracker.SetInComputePass(false); RecordRenderPass(commandList, &bindingTracker, ToBackend(beginRenderPassCmd->info.Get())); diff --git a/src/dawn_native/d3d12/CommandBufferD3D12.h b/src/dawn_native/d3d12/CommandBufferD3D12.h index e9e7bfe7f8..95ac8bd142 100644 --- a/src/dawn_native/d3d12/CommandBufferD3D12.h +++ b/src/dawn_native/d3d12/CommandBufferD3D12.h @@ -42,7 +42,6 @@ namespace dawn_native { namespace d3d12 { RenderPassDescriptor* renderPass); CommandIterator mCommands; - std::vector mPassResourceUsages; }; }} // namespace dawn_native::d3d12 diff --git a/src/dawn_native/vulkan/CommandBufferVk.cpp b/src/dawn_native/vulkan/CommandBufferVk.cpp index fa07a9c590..8f5c46271c 100644 --- a/src/dawn_native/vulkan/CommandBufferVk.cpp +++ b/src/dawn_native/vulkan/CommandBufferVk.cpp @@ -111,9 +111,7 @@ namespace dawn_native { namespace vulkan { } // anonymous namespace CommandBuffer::CommandBuffer(CommandBufferBuilder* builder) - : CommandBufferBase(builder), - mCommands(builder->AcquireCommands()), - mPassResourceUsages(builder->AcquirePassResourceUsage()) { + : CommandBufferBase(builder), mCommands(builder->AcquireCommands()) { } CommandBuffer::~CommandBuffer() { @@ -135,6 +133,7 @@ namespace dawn_native { namespace vulkan { } }; + const std::vector& passResourceUsages = GetResourceUsages().perPass; size_t nextPassNumber = 0; Command type; @@ -207,7 +206,7 @@ namespace dawn_native { namespace vulkan { case Command::BeginRenderPass: { BeginRenderPassCmd* cmd = mCommands.NextCommand(); - TransitionForPass(commands, mPassResourceUsages[nextPassNumber]); + TransitionForPass(commands, passResourceUsages[nextPassNumber]); RecordRenderPass(commands, ToBackend(cmd->info.Get())); nextPassNumber++; @@ -216,7 +215,7 @@ namespace dawn_native { namespace vulkan { case Command::BeginComputePass: { mCommands.NextCommand(); - TransitionForPass(commands, mPassResourceUsages[nextPassNumber]); + TransitionForPass(commands, passResourceUsages[nextPassNumber]); RecordComputePass(commands); nextPassNumber++; diff --git a/src/dawn_native/vulkan/CommandBufferVk.h b/src/dawn_native/vulkan/CommandBufferVk.h index b6499df417..2ca5a62865 100644 --- a/src/dawn_native/vulkan/CommandBufferVk.h +++ b/src/dawn_native/vulkan/CommandBufferVk.h @@ -35,7 +35,6 @@ namespace dawn_native { namespace vulkan { void RecordRenderPass(VkCommandBuffer commands, RenderPassDescriptor* renderPass); CommandIterator mCommands; - std::vector mPassResourceUsages; }; }} // namespace dawn_native::vulkan diff --git a/src/tests/unittests/validation/QueueSubmitValidationTests.cpp b/src/tests/unittests/validation/QueueSubmitValidationTests.cpp new file mode 100644 index 0000000000..37c1d4030a --- /dev/null +++ b/src/tests/unittests/validation/QueueSubmitValidationTests.cpp @@ -0,0 +1,69 @@ +// Copyright 2018 The Dawn Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "tests/unittests/validation/ValidationTest.h" + +#include "utils/DawnHelpers.h" + +namespace { + +class QueueSubmitValidationTest : public ValidationTest { +}; + +static void StoreTrueMapWriteCallback(dawnBufferMapAsyncStatus status, void*, dawnCallbackUserdata userdata) { + bool* userdataPtr = reinterpret_cast(static_cast(userdata)); + *userdataPtr = true; +} + +// Test submitting with a mapped buffer is disallowed +TEST_F(QueueSubmitValidationTest, SubmitWithMappedBuffer) { + // Create a map-write buffer. + dawn::BufferDescriptor descriptor; + descriptor.usage = dawn::BufferUsageBit::MapWrite | dawn::BufferUsageBit::TransferSrc; + descriptor.size = 4; + dawn::Buffer buffer = device.CreateBuffer(&descriptor); + + // Create a fake copy destination buffer + descriptor.usage = dawn::BufferUsageBit::TransferDst; + dawn::Buffer targetBuffer = device.CreateBuffer(&descriptor); + + // Create a command buffer that reads from the mappable buffer. + dawn::CommandBuffer commands; + { + dawn::RenderPassDescriptor renderpass = CreateSimpleRenderPass(); + dawn::CommandBufferBuilder builder = device.CreateCommandBufferBuilder(); + builder.CopyBufferToBuffer(buffer, 0, targetBuffer, 0, 4); + commands = builder.GetResult(); + } + + dawn::Queue queue = device.CreateQueue(); + + // Submitting when the buffer has never been mapped should succeed + queue.Submit(1, &commands); + + // Map the buffer, submitting when the buffer is mapped should fail + bool mapWriteFinished = false; + dawnCallbackUserdata userdata = static_cast(reinterpret_cast(&mapWriteFinished)); + buffer.MapWriteAsync(0, 4, StoreTrueMapWriteCallback, userdata); + queue.Submit(0, nullptr); + ASSERT_TRUE(mapWriteFinished); + + ASSERT_DEVICE_ERROR(queue.Submit(1, &commands)); + + // Unmap the buffer, queue submit should succeed + buffer.Unmap(); + queue.Submit(1, &commands); +} + +} // anonymous namespace