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 <cwallez@chromium.org>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Stephen White <senorblanco@chromium.org>
diff --git a/BUILD.gn b/BUILD.gn
index c346ea7..74d942d 100644
--- a/BUILD.gn
+++ b/BUILD.gn
@@ -808,6 +808,7 @@
"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 @@
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 4908d87..c013d80 100644
--- a/src/dawn_native/d3d12/BindGroupD3D12.cpp
+++ b/src/dawn_native/d3d12/BindGroupD3D12.cpp
@@ -30,8 +30,10 @@
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 @@
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 8b360e4..b577939 100644
--- a/src/dawn_native/d3d12/BindGroupD3D12.h
+++ b/src/dawn_native/d3d12/BindGroupD3D12.h
@@ -33,15 +33,18 @@
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 2dab383..27bd589 100644
--- a/src/dawn_native/d3d12/CommandBufferD3D12.cpp
+++ b/src/dawn_native/d3d12/CommandBufferD3D12.cpp
@@ -65,29 +65,32 @@
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 @@
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 @@
SetComputePipelineCmd* cmd =
commands->NextCommand<SetComputePipelineCmd>();
PipelineLayout* layout = ToBackend(cmd->pipeline->GetLayout());
- bindingTracker->TrackInheritedGroups(lastLayout, layout);
+ bindingTracker->TrackInheritedGroups(lastLayout, layout, indexInSubmit);
lastLayout = layout;
} break;
@@ -185,14 +189,14 @@
SetRenderPipelineCmd* cmd =
commands->NextCommand<SetRenderPipelineCmd>();
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<SetBindGroupCmd>();
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 @@
FreeCommands(&mCommands);
}
- void CommandBuffer::RecordCommands(ComPtr<ID3D12GraphicsCommandList> commandList) {
+ void CommandBuffer::RecordCommands(ComPtr<ID3D12GraphicsCommandList> commandList,
+ uint32_t indexInSubmit) {
Device* device = ToBackend(GetDevice());
BindGroupStateTracker bindingTracker(device);
@@ -248,7 +253,7 @@
// 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 f737b78..e9e7bfe 100644
--- a/src/dawn_native/d3d12/CommandBufferD3D12.h
+++ b/src/dawn_native/d3d12/CommandBufferD3D12.h
@@ -32,7 +32,7 @@
CommandBuffer(CommandBufferBuilder* builder);
~CommandBuffer();
- void RecordCommands(ComPtr<ID3D12GraphicsCommandList> commandList);
+ void RecordCommands(ComPtr<ID3D12GraphicsCommandList> commandList, uint32_t indexInSubmit);
private:
void RecordComputePass(ComPtr<ID3D12GraphicsCommandList> commandList,
diff --git a/src/dawn_native/d3d12/QueueD3D12.cpp b/src/dawn_native/d3d12/QueueD3D12.cpp
index 344f060..e9866f2 100644
--- a/src/dawn_native/d3d12/QueueD3D12.cpp
+++ b/src/dawn_native/d3d12/QueueD3D12.cpp
@@ -29,7 +29,7 @@
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 0000000..9510e06
--- /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);
