From d380fdda08d40a864667feb28b96392db26c8a11 Mon Sep 17 00:00:00 2001 From: Stephen White Date: Thu, 4 Oct 2018 16:59:05 +0000 Subject: [PATCH] Fix for D3D12 re-used bindgroup bug. If a bindgroup was re-used in multiple command buffers in a single call to queue.Submit(), it was skipping the creation of the corresponding descriptor heap for that bind group in the second command buffer. The fix is to use the command buffer's index into the submit command as part of the key for the bind group's descriptor heap. Change-Id: Ie66a0e772b10cc72bf040f090dac4c4a10f24266 Reviewed-on: https://dawn-review.googlesource.com/c/1740 Reviewed-by: Corentin Wallez Commit-Queue: Corentin Wallez Commit-Queue: Stephen White --- BUILD.gn | 5 +- src/dawn_native/d3d12/BindGroupD3D12.cpp | 7 +- src/dawn_native/d3d12/BindGroupD3D12.h | 5 +- src/dawn_native/d3d12/CommandBufferD3D12.cpp | 27 ++++--- src/dawn_native/d3d12/CommandBufferD3D12.h | 2 +- src/dawn_native/d3d12/QueueD3D12.cpp | 2 +- src/tests/end2end/BindGroupTests.cpp | 78 ++++++++++++++++++++ 7 files changed, 109 insertions(+), 17 deletions(-) create mode 100644 src/tests/end2end/BindGroupTests.cpp diff --git a/BUILD.gn b/BUILD.gn index c346ea7004..74d942df52 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -808,6 +808,7 @@ test("dawn_end2end_tests") { "src/tests/DawnTest.h", "src/tests/End2EndTestsMain.cpp", "src/tests/end2end/BasicTests.cpp", + "src/tests/end2end/BindGroupTests.cpp", "src/tests/end2end/BlendStateTests.cpp", "src/tests/end2end/BufferTests.cpp", "src/tests/end2end/ComputeCopyStorageBufferTests.cpp", @@ -912,10 +913,10 @@ if (dawn_standalone) { group("dawn_samples") { deps = [ - ":CppHelloTriangle", + ":Animometer", ":CHelloTriangle", ":ComputeBoids", - ":Animometer", + ":CppHelloTriangle", ":CubeReflection", ":glTFViewer", ] diff --git a/src/dawn_native/d3d12/BindGroupD3D12.cpp b/src/dawn_native/d3d12/BindGroupD3D12.cpp index 4908d8732d..c013d80d30 100644 --- a/src/dawn_native/d3d12/BindGroupD3D12.cpp +++ b/src/dawn_native/d3d12/BindGroupD3D12.cpp @@ -30,8 +30,10 @@ namespace dawn_native { namespace d3d12 { uint32_t* cbvUavSrvHeapOffset, const DescriptorHeapHandle& samplerHeapStart, uint32_t* samplerHeapOffset, - uint64_t serial) { + uint64_t serial, + uint32_t indexInSubmit) { mHeapSerial = serial; + mIndexInSubmit = indexInSubmit; const auto* bgl = ToBackend(GetLayout()); const auto& layout = bgl->GetBindingInfo(); @@ -94,5 +96,8 @@ namespace dawn_native { namespace d3d12 { uint64_t BindGroup::GetHeapSerial() const { return mHeapSerial; } + uint32_t BindGroup::GetIndexInSubmit() const { + return mIndexInSubmit; + } }} // namespace dawn_native::d3d12 diff --git a/src/dawn_native/d3d12/BindGroupD3D12.h b/src/dawn_native/d3d12/BindGroupD3D12.h index 8b360e4d1e..b577939127 100644 --- a/src/dawn_native/d3d12/BindGroupD3D12.h +++ b/src/dawn_native/d3d12/BindGroupD3D12.h @@ -33,15 +33,18 @@ namespace dawn_native { namespace d3d12 { uint32_t* cbvUavSrvHeapOffset, const DescriptorHeapHandle& samplerHeapStart, uint32_t* samplerHeapOffset, - uint64_t serial); + uint64_t serial, + uint32_t indexInSubmit); uint32_t GetCbvUavSrvHeapOffset() const; uint32_t GetSamplerHeapOffset() const; uint64_t GetHeapSerial() const; + uint32_t GetIndexInSubmit() const; private: uint32_t mCbvUavSrvHeapOffset; uint32_t mSamplerHeapOffset; uint64_t mHeapSerial = 0; + uint32_t mIndexInSubmit; }; }} // namespace dawn_native::d3d12 diff --git a/src/dawn_native/d3d12/CommandBufferD3D12.cpp b/src/dawn_native/d3d12/CommandBufferD3D12.cpp index 2dab383fbd..27bd589192 100644 --- a/src/dawn_native/d3d12/CommandBufferD3D12.cpp +++ b/src/dawn_native/d3d12/CommandBufferD3D12.cpp @@ -65,29 +65,32 @@ namespace dawn_native { namespace d3d12 { inCompute = inCompute_; } - void TrackSetBindGroup(BindGroup* group, uint32_t index) { + void TrackSetBindGroup(BindGroup* group, uint32_t index, uint32_t indexInSubmit) { if (bindGroups[index] != group) { bindGroups[index] = group; // Descriptors don't need to be recorded if they have already been recorded in // the heap. Indices are only updated when descriptors are recorded const uint64_t serial = device->GetSerial(); - if (group->GetHeapSerial() != serial) { + if (group->GetHeapSerial() != serial || + group->GetIndexInSubmit() != indexInSubmit) { group->RecordDescriptors(cbvSrvUavCPUDescriptorHeap, &cbvSrvUavDescriptorIndex, samplerCPUDescriptorHeap, &samplerDescriptorIndex, - serial); + serial, indexInSubmit); } } } - void TrackInheritedGroups(PipelineLayout* oldLayout, PipelineLayout* newLayout) { + void TrackInheritedGroups(PipelineLayout* oldLayout, + PipelineLayout* newLayout, + uint32_t indexInSubmit) { if (oldLayout == nullptr) { return; } uint32_t inheritUntil = oldLayout->GroupsInheritUpTo(newLayout); for (uint32_t i = 0; i < inheritUntil; ++i) { - TrackSetBindGroup(bindGroups[i], i); + TrackSetBindGroup(bindGroups[i], i, indexInSubmit); } } @@ -157,7 +160,8 @@ namespace dawn_native { namespace d3d12 { void AllocateAndSetDescriptorHeaps(Device* device, BindGroupStateTracker* bindingTracker, - CommandIterator* commands) { + CommandIterator* commands, + int indexInSubmit) { auto* descriptorHeapAllocator = device->GetDescriptorHeapAllocator(); // TODO(enga@google.com): This currently allocates CPU heaps of arbitrarily chosen sizes @@ -177,7 +181,7 @@ namespace dawn_native { namespace d3d12 { SetComputePipelineCmd* cmd = commands->NextCommand(); PipelineLayout* layout = ToBackend(cmd->pipeline->GetLayout()); - bindingTracker->TrackInheritedGroups(lastLayout, layout); + bindingTracker->TrackInheritedGroups(lastLayout, layout, indexInSubmit); lastLayout = layout; } break; @@ -185,14 +189,14 @@ namespace dawn_native { namespace d3d12 { SetRenderPipelineCmd* cmd = commands->NextCommand(); PipelineLayout* layout = ToBackend(cmd->pipeline->GetLayout()); - bindingTracker->TrackInheritedGroups(lastLayout, layout); + bindingTracker->TrackInheritedGroups(lastLayout, layout, indexInSubmit); lastLayout = layout; } break; case Command::SetBindGroup: { SetBindGroupCmd* cmd = commands->NextCommand(); BindGroup* group = ToBackend(cmd->group.Get()); - bindingTracker->TrackSetBindGroup(group, cmd->index); + bindingTracker->TrackSetBindGroup(group, cmd->index, indexInSubmit); } break; default: SkipCommand(commands, type); @@ -239,7 +243,8 @@ namespace dawn_native { namespace d3d12 { FreeCommands(&mCommands); } - void CommandBuffer::RecordCommands(ComPtr commandList) { + void CommandBuffer::RecordCommands(ComPtr commandList, + uint32_t indexInSubmit) { Device* device = ToBackend(GetDevice()); BindGroupStateTracker bindingTracker(device); @@ -248,7 +253,7 @@ namespace dawn_native { namespace d3d12 { // should have a system where commands and descriptors are recorded in parallel then the // heaps set using a small CommandList inserted just before the main CommandList. { - AllocateAndSetDescriptorHeaps(device, &bindingTracker, &mCommands); + AllocateAndSetDescriptorHeaps(device, &bindingTracker, &mCommands, indexInSubmit); bindingTracker.Reset(); ID3D12DescriptorHeap* descriptorHeaps[2] = { diff --git a/src/dawn_native/d3d12/CommandBufferD3D12.h b/src/dawn_native/d3d12/CommandBufferD3D12.h index f737b78384..e9e7bfe7f8 100644 --- a/src/dawn_native/d3d12/CommandBufferD3D12.h +++ b/src/dawn_native/d3d12/CommandBufferD3D12.h @@ -32,7 +32,7 @@ namespace dawn_native { namespace d3d12 { CommandBuffer(CommandBufferBuilder* builder); ~CommandBuffer(); - void RecordCommands(ComPtr commandList); + void RecordCommands(ComPtr commandList, uint32_t indexInSubmit); private: void RecordComputePass(ComPtr commandList, diff --git a/src/dawn_native/d3d12/QueueD3D12.cpp b/src/dawn_native/d3d12/QueueD3D12.cpp index 344f060e0a..e9866f2e70 100644 --- a/src/dawn_native/d3d12/QueueD3D12.cpp +++ b/src/dawn_native/d3d12/QueueD3D12.cpp @@ -29,7 +29,7 @@ namespace dawn_native { namespace d3d12 { device->OpenCommandList(&mCommandList); for (uint32_t i = 0; i < numCommands; ++i) { - ToBackend(commands[i])->RecordCommands(mCommandList); + ToBackend(commands[i])->RecordCommands(mCommandList, i); } ASSERT_SUCCESS(mCommandList->Close()); diff --git a/src/tests/end2end/BindGroupTests.cpp b/src/tests/end2end/BindGroupTests.cpp new file mode 100644 index 0000000000..9510e0629b --- /dev/null +++ b/src/tests/end2end/BindGroupTests.cpp @@ -0,0 +1,78 @@ +// 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/DawnTest.h" +#include "utils/DawnHelpers.h" + +class BindGroupTests : public DawnTest { +protected: + dawn::CommandBuffer CreateSimpleComputeCommandBuffer( + const dawn::ComputePipeline& pipeline, const dawn::BindGroup& bindGroup) { + dawn::CommandBufferBuilder builder = device.CreateCommandBufferBuilder(); + dawn::ComputePassEncoder pass = builder.BeginComputePass(); + pass.SetComputePipeline(pipeline); + pass.SetBindGroup(0, bindGroup); + pass.Dispatch(1, 1, 1); + pass.EndPass(); + return builder.GetResult(); + } +}; + +// Test a bindgroup reused in two command buffers in the same call to queue.Submit(). +// This test passes by not asserting or crashing. +TEST_P(BindGroupTests, ReusedBindGroupSingleSubmit) { + dawn::BindGroupLayout bgl = utils::MakeBindGroupLayout( + device, + { + {0, dawn::ShaderStageBit::Compute, dawn::BindingType::UniformBuffer }, + } + ); + dawn::PipelineLayout pl = utils::MakeBasicPipelineLayout(device, &bgl); + + const char* shader = R"( + #version 450 + layout(std140, set = 0, binding = 0) uniform Contents { + float f; + } contents; + void main() { + } + )"; + + dawn::ShaderModule module = + utils::CreateShaderModule(device, dawn::ShaderStage::Compute, shader); + dawn::ComputePipelineDescriptor cpDesc; + cpDesc.module = module.Clone(); + cpDesc.entryPoint = "main"; + cpDesc.layout = pl.Clone(); + dawn::ComputePipeline cp = device.CreateComputePipeline(&cpDesc); + + dawn::BufferDescriptor bufferDesc; + bufferDesc.size = sizeof(float); + bufferDesc.usage = dawn::BufferUsageBit::TransferDst | + dawn::BufferUsageBit::Uniform; + dawn::Buffer buffer = device.CreateBuffer(&bufferDesc); + dawn::BufferView bufferView = + buffer.CreateBufferViewBuilder().SetExtent(0, sizeof(float)).GetResult(); + dawn::BindGroup bindGroup = device.CreateBindGroupBuilder() + .SetLayout(bgl) + .SetBufferViews(0, 1, &bufferView) + .GetResult(); + + dawn::CommandBuffer cb[2]; + cb[0] = CreateSimpleComputeCommandBuffer(cp, bindGroup); + cb[1] = CreateSimpleComputeCommandBuffer(cp, bindGroup); + queue.Submit(2, cb); +} + +DAWN_INSTANTIATE_TEST(BindGroupTests, D3D12Backend, MetalBackend, OpenGLBackend, VulkanBackend);