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>

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",
     ]

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

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

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<SetComputePipelineCmd>();
                             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<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 @@ namespace dawn_native { namespace d3d12 {
         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 @@ 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] = {

src/dawn_native/d3d12/CommandBufferD3D12.h

@@ -32,7 +32,7 @@ namespace dawn_native { namespace d3d12 {
         CommandBuffer(CommandBufferBuilder* builder);
         ~CommandBuffer();
 
-        void RecordCommands(ComPtr<ID3D12GraphicsCommandList> commandList);
+        void RecordCommands(ComPtr<ID3D12GraphicsCommandList> commandList, uint32_t indexInSubmit);
 
       private:
         void RecordComputePass(ComPtr<ID3D12GraphicsCommandList> commandList,

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());
 

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);