commit 3b7d0858bf3557c03dd172e29f4c7b6654c060e0
author Austin Eng <enga@chromium.org> Mon Jun 01 23:42:57 2020 +0000
committer Commit Bot service account <commit-bot@chromium.org> Mon Jun 01 23:42:57 2020 +0000
tree 3fbede1151485d3e92d9fcb36e298173e38d9f99
parent 0e3c27de6a3311d5302c9d566e1fea2e369b3c8e

D3D12: Skip dynamic buffer bindings with None visibility

This was hitting an ASSERT because D3D12 doesn't have an option
to set None as the shader visibility.

Bug: dawn:448
Change-Id: I3e056e531e7d1bb89da1736bc609bfe97a2fa194
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/22324
Reviewed-by: Bryan Bernhart <bryan.bernhart@intel.com>
Commit-Queue: Austin Eng <enga@chromium.org>

src/dawn_native/d3d12/CommandBufferD3D12.cpp

diff --git a/src/dawn_native/d3d12/CommandBufferD3D12.cpp b/src/dawn_native/d3d12/CommandBufferD3D12.cpp
index ae85ef4..2321a4c 100644
--- a/src/dawn_native/d3d12/CommandBufferD3D12.cpp
+++ b/src/dawn_native/d3d12/CommandBufferD3D12.cpp
@@ -230,6 +230,14 @@
                 // Dynamic buffer bindings are packed at the beginning of the layout.
                 for (BindingIndex bindingIndex = 0; bindingIndex < dynamicOffsetCount;
                      ++bindingIndex) {
+                    const BindingInfo& bindingInfo =
+                        group->GetLayout()->GetBindingInfo(bindingIndex);
+                    if (bindingInfo.visibility == wgpu::ShaderStage::None) {
+                        // Skip dynamic buffers that are not visible. D3D12 does not have None
+                        // visibility.
+                        continue;
+                    }
+
                     uint32_t parameterIndex =
                         pipelineLayout->GetDynamicRootParameterIndex(index, bindingIndex);
                     BufferBinding binding = group->GetBindingAsBufferBinding(bindingIndex);
@@ -241,7 +249,7 @@
                     D3D12_GPU_VIRTUAL_ADDRESS bufferLocation =
                         ToBackend(binding.buffer)->GetVA() + offset;
 
-                    switch (group->GetLayout()->GetBindingInfo(bindingIndex).type) {
+                    switch (bindingInfo.type) {
                         case wgpu::BindingType::UniformBuffer:
                             if (mInCompute) {
                                 commandList->SetComputeRootConstantBufferView(parameterIndex,

src/dawn_native/d3d12/PipelineLayoutD3D12.cpp

diff --git a/src/dawn_native/d3d12/PipelineLayoutD3D12.cpp b/src/dawn_native/d3d12/PipelineLayoutD3D12.cpp
index 3c8a7f7..7121172 100644
--- a/src/dawn_native/d3d12/PipelineLayoutD3D12.cpp
+++ b/src/dawn_native/d3d12/PipelineLayoutD3D12.cpp
@@ -136,6 +136,12 @@
                 const BindingInfo& bindingInfo =
                     bindGroupLayout->GetBindingInfo(dynamicBindingIndex);
 
+                if (bindingInfo.visibility == wgpu::ShaderStage::None) {
+                    // Skip dynamic buffers that are not visible. D3D12 does not have None
+                    // visibility.
+                    continue;
+                }
+
                 D3D12_ROOT_PARAMETER* rootParameter = &rootParameters[parameterIndex];
 
                 // Setup root descriptor.
@@ -195,6 +201,8 @@
         ASSERT(group < kMaxBindGroups);
         ASSERT(bindingIndex < kMaxBindingsPerGroup);
         ASSERT(GetBindGroupLayout(group)->GetBindingInfo(bindingIndex).hasDynamicOffset);
+        ASSERT(GetBindGroupLayout(group)->GetBindingInfo(bindingIndex).visibility !=
+               wgpu::ShaderStage::None);
         return mDynamicRootParameterIndices[group][bindingIndex];
     }
 }}  // namespace dawn_native::d3d12

src/tests/end2end/BindGroupTests.cpp

diff --git a/src/tests/end2end/BindGroupTests.cpp b/src/tests/end2end/BindGroupTests.cpp
index e0f3a53..153c303 100644
--- a/src/tests/end2end/BindGroupTests.cpp
+++ b/src/tests/end2end/BindGroupTests.cpp
@@ -863,6 +863,38 @@
     queue.Submit(1, &commands);
 }
 
+// Regression test for crbug.com/dawn/448 that dynamic buffer bindings can have None visibility.
+TEST_P(BindGroupTests, DynamicBindingNoneVisibility) {
+    utils::BasicRenderPass renderPass = utils::CreateBasicRenderPass(device, kRTSize, kRTSize);
+
+    wgpu::BindGroupLayoutEntry entry = {0, wgpu::ShaderStage::None,
+                                        wgpu::BindingType::UniformBuffer, true};
+    wgpu::BindGroupLayoutDescriptor descriptor;
+    descriptor.entryCount = 1;
+    descriptor.entries = &entry;
+    wgpu::BindGroupLayout layout = device.CreateBindGroupLayout(&descriptor);
+
+    wgpu::RenderPipeline pipeline = MakeTestPipeline(renderPass, {}, {layout});
+
+    std::array<float, 4> color = {1, 0, 0, 1};
+    wgpu::Buffer uniformBuffer =
+        utils::CreateBufferFromData(device, &color, sizeof(color), wgpu::BufferUsage::Uniform);
+    wgpu::BindGroup bindGroup =
+        utils::MakeBindGroup(device, layout, {{0, uniformBuffer, 0, sizeof(color)}});
+
+    uint32_t dynamicOffset = 0;
+
+    wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+    wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass.renderPassInfo);
+    pass.SetPipeline(pipeline);
+    pass.SetBindGroup(0, bindGroup, 1, &dynamicOffset);
+    pass.Draw(3);
+    pass.EndPass();
+
+    wgpu::CommandBuffer commands = encoder.Finish();
+    queue.Submit(1, &commands);
+}
+
 // Test that bind group bindings may have unbounded and arbitrary binding numbers
 TEST_P(BindGroupTests, ArbitraryBindingNumbers) {
     utils::BasicRenderPass renderPass = utils::CreateBasicRenderPass(device, kRTSize, kRTSize);