D3D12: Fix pipeline layout overflow when using dynamic offsets.
Pipeline layout incorrectly indexes into a root table array
when there are more root descriptors than root tables.
To fix, the array is dynamically sized where parameters
are appended instead of indexed into the root signature.
Bug: dawn:449
Change-Id: I6d7f65fb791d323704b1c3a3af9c871a79e32a30
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/22960
Commit-Queue: Bryan Bernhart <bryan.bernhart@intel.com>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
diff --git a/src/dawn_native/d3d12/PipelineLayoutD3D12.cpp b/src/dawn_native/d3d12/PipelineLayoutD3D12.cpp
index 7121172..2598758 100644
--- a/src/dawn_native/d3d12/PipelineLayoutD3D12.cpp
+++ b/src/dawn_native/d3d12/PipelineLayoutD3D12.cpp
@@ -69,22 +69,14 @@
MaybeError PipelineLayout::Initialize() {
Device* device = ToBackend(GetDevice());
- D3D12_ROOT_PARAMETER rootParameters[kMaxBindGroups * 2 + kMaxDynamicBufferCount];
-
- // A root parameter is one of these types
- union {
- D3D12_ROOT_DESCRIPTOR_TABLE DescriptorTable;
- D3D12_ROOT_CONSTANTS Constants;
- D3D12_ROOT_DESCRIPTOR Descriptor;
- } rootParameterValues[kMaxBindGroups * 2];
- // samplers must be in a separate descriptor table so we need at most twice as many tables
- // as bind groups
+ // Parameters are D3D12_ROOT_PARAMETER_TYPE which is either a root table, constant, or
+ // descriptor.
+ std::vector<D3D12_ROOT_PARAMETER> rootParameters;
// Ranges are D3D12_DESCRIPTOR_RANGE_TYPE_(SRV|UAV|CBV|SAMPLER)
// They are grouped together so each bind group has at most 4 ranges
D3D12_DESCRIPTOR_RANGE ranges[kMaxBindGroups * 4];
- uint32_t parameterIndex = 0;
uint32_t rangeIndex = 0;
for (uint32_t group : IterateBitSet(GetBindGroupLayoutsMask())) {
@@ -99,9 +91,8 @@
return false;
}
- auto& rootParameter = rootParameters[parameterIndex];
+ D3D12_ROOT_PARAMETER rootParameter = {};
rootParameter.ParameterType = D3D12_ROOT_PARAMETER_TYPE_DESCRIPTOR_TABLE;
- rootParameter.DescriptorTable = rootParameterValues[parameterIndex].DescriptorTable;
rootParameter.ShaderVisibility = D3D12_SHADER_VISIBILITY_ALL;
rootParameter.DescriptorTable.NumDescriptorRanges = rangeCount;
rootParameter.DescriptorTable.pDescriptorRanges = &ranges[rangeIndex];
@@ -112,17 +103,19 @@
rangeIndex++;
}
+ rootParameters.emplace_back(rootParameter);
+
return true;
};
if (SetRootDescriptorTable(bindGroupLayout->GetCbvUavSrvDescriptorTableSize(),
bindGroupLayout->GetCbvUavSrvDescriptorRanges())) {
- mCbvUavSrvRootParameterInfo[group] = parameterIndex++;
+ mCbvUavSrvRootParameterInfo[group] = rootParameters.size() - 1;
}
if (SetRootDescriptorTable(bindGroupLayout->GetSamplerDescriptorTableSize(),
bindGroupLayout->GetSamplerDescriptorRanges())) {
- mSamplerRootParameterInfo[group] = parameterIndex++;
+ mSamplerRootParameterInfo[group] = rootParameters.size() - 1;
}
// Get calculated shader register for root descriptors
@@ -142,7 +135,7 @@
continue;
}
- D3D12_ROOT_PARAMETER* rootParameter = &rootParameters[parameterIndex];
+ D3D12_ROOT_PARAMETER rootParameter = {};
// Setup root descriptor.
D3D12_ROOT_DESCRIPTOR rootDescriptor;
@@ -150,20 +143,22 @@
rootDescriptor.RegisterSpace = group;
// Set root descriptors in root signatures.
- rootParameter->Descriptor = rootDescriptor;
- mDynamicRootParameterIndices[group][dynamicBindingIndex] = parameterIndex++;
+ rootParameter.Descriptor = rootDescriptor;
+ mDynamicRootParameterIndices[group][dynamicBindingIndex] = rootParameters.size();
// Set parameter types according to bind group layout descriptor.
- rootParameter->ParameterType = RootParameterType(bindingInfo.type);
+ rootParameter.ParameterType = RootParameterType(bindingInfo.type);
// Set visibilities according to bind group layout descriptor.
- rootParameter->ShaderVisibility = ShaderVisibilityType(bindingInfo.visibility);
+ rootParameter.ShaderVisibility = ShaderVisibilityType(bindingInfo.visibility);
+
+ rootParameters.emplace_back(rootParameter);
}
}
D3D12_ROOT_SIGNATURE_DESC rootSignatureDescriptor;
- rootSignatureDescriptor.NumParameters = parameterIndex;
- rootSignatureDescriptor.pParameters = rootParameters;
+ rootSignatureDescriptor.NumParameters = rootParameters.size();
+ rootSignatureDescriptor.pParameters = rootParameters.data();
rootSignatureDescriptor.NumStaticSamplers = 0;
rootSignatureDescriptor.pStaticSamplers = nullptr;
rootSignatureDescriptor.Flags =
diff --git a/src/tests/BUILD.gn b/src/tests/BUILD.gn
index 01b3042..b9ae2fe 100644
--- a/src/tests/BUILD.gn
+++ b/src/tests/BUILD.gn
@@ -281,6 +281,7 @@
"end2end/NonzeroTextureCreationTests.cpp",
"end2end/ObjectCachingTests.cpp",
"end2end/OpArrayLengthTests.cpp",
+ "end2end/PipelineLayoutTests.cpp",
"end2end/PrimitiveTopologyTests.cpp",
"end2end/QueueTests.cpp",
"end2end/RenderBundleTests.cpp",
diff --git a/src/tests/end2end/PipelineLayoutTests.cpp b/src/tests/end2end/PipelineLayoutTests.cpp
new file mode 100644
index 0000000..6e71234
--- /dev/null
+++ b/src/tests/end2end/PipelineLayoutTests.cpp
@@ -0,0 +1,65 @@
+// Copyright 2020 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 "common/Constants.h"
+#include "tests/DawnTest.h"
+
+#include <vector>
+
+class PipelineLayoutTests : public DawnTest {};
+
+// Test creating a PipelineLayout with multiple BGLs where the first BGL uses the max number of
+// dynamic buffers. This is a regression test for crbug.com/dawn/449 which would overflow when
+// dynamic offset bindings were at max. Test is successful if the pipeline layout is created
+// without error.
+TEST_P(PipelineLayoutTests, DynamicBuffersOverflow) {
+ // Create the first bind group layout which uses max number of dynamic buffers bindings.
+ wgpu::BindGroupLayout bglA;
+ {
+ std::vector<wgpu::BindGroupLayoutEntry> entries;
+ for (uint32_t i = 0; i < kMaxDynamicStorageBufferCount; i++) {
+ entries.push_back(
+ {i, wgpu::ShaderStage::Compute, wgpu::BindingType::StorageBuffer, true});
+ }
+
+ wgpu::BindGroupLayoutDescriptor descriptor;
+ descriptor.entryCount = static_cast<uint32_t>(entries.size());
+ descriptor.entries = entries.data();
+ bglA = device.CreateBindGroupLayout(&descriptor);
+ }
+
+ // Create the second bind group layout that has one non-dynamic buffer binding.
+ wgpu::BindGroupLayout bglB;
+ {
+ wgpu::BindGroupLayoutDescriptor descriptor;
+ wgpu::BindGroupLayoutEntry entry = {0, wgpu::ShaderStage::Compute,
+ wgpu::BindingType::StorageBuffer, false};
+ descriptor.entryCount = 1;
+ descriptor.entries = &entry;
+ bglB = device.CreateBindGroupLayout(&descriptor);
+ }
+
+ // Create a pipeline layout using both bind group layouts.
+ wgpu::PipelineLayoutDescriptor descriptor;
+ std::vector<wgpu::BindGroupLayout> bindgroupLayouts = {bglA, bglB};
+ descriptor.bindGroupLayoutCount = bindgroupLayouts.size();
+ descriptor.bindGroupLayouts = bindgroupLayouts.data();
+ device.CreatePipelineLayout(&descriptor);
+}
+
+DAWN_INSTANTIATE_TEST(PipelineLayoutTests,
+ D3D12Backend(),
+ MetalBackend(),
+ OpenGLBackend(),
+ VulkanBackend());
\ No newline at end of file
