From 751252e3724b07062858af91e72dadb6379244ff Mon Sep 17 00:00:00 2001 From: Rafael Cintron Date: Wed, 26 Jun 2019 08:08:46 +0000 Subject: [PATCH] D3D12: Use D3D12_BUFFER_UAV_FLAG_RAW when creating unordered access views Since SPIRV-Cross outputs HLSL shaders with RWByteAddressBuffer, we must use D3D12_BUFFER_UAV_FLAG_RAW when making the UNORDERED_ACCESS_VIEW_DESC. Using D3D12_BUFFER_UAV_FLAG_RAW requires that we use DXGI_FORMAT_R32_TYPELESS as the format of the view. DXGI_FORMAT_R32_TYPELESS requires that the element size be 4 byte aligned. Since binding.size and binding.offset are in bytes, we need to divide by 4 to obtain the element size. Bug: dawn:159 Change-Id: I52481aea1fe190d54286c099a68f3e21dbe05330 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/8341 Commit-Queue: Corentin Wallez Reviewed-by: Corentin Wallez --- src/dawn_native/d3d12/BindGroupD3D12.cpp | 19 ++++++++++++------- .../end2end/ComputeCopyStorageBufferTests.cpp | 9 ++------- src/tests/end2end/ComputeIndirectTests.cpp | 4 ---- .../end2end/ComputeSharedMemoryTests.cpp | 3 --- 4 files changed, 14 insertions(+), 21 deletions(-) diff --git a/src/dawn_native/d3d12/BindGroupD3D12.cpp b/src/dawn_native/d3d12/BindGroupD3D12.cpp index 1c5862cf0e..e81db4d261 100644 --- a/src/dawn_native/d3d12/BindGroupD3D12.cpp +++ b/src/dawn_native/d3d12/BindGroupD3D12.cpp @@ -59,16 +59,21 @@ namespace dawn_native { namespace d3d12 { case dawn::BindingType::StorageBuffer: { BufferBinding binding = GetBindingAsBufferBinding(bindingIndex); + // Since SPIRV-Cross outputs HLSL shaders with RWByteAddressBuffer, + // we must use D3D12_BUFFER_UAV_FLAG_RAW when making the + // UNORDERED_ACCESS_VIEW_DESC. Using D3D12_BUFFER_UAV_FLAG_RAW requires + // that we use DXGI_FORMAT_R32_TYPELESS as the format of the view. + // DXGI_FORMAT_R32_TYPELESS requires that the element size be 4 + // byte aligned. Since binding.size and binding.offset are in bytes, + // we need to divide by 4 to obtain the element size. D3D12_UNORDERED_ACCESS_VIEW_DESC desc; - // TODO(enga@google.com): investigate if this needs to be a constraint at the - // API level - desc.Buffer.NumElements = Align(binding.size, 256); - desc.Format = DXGI_FORMAT_UNKNOWN; + desc.Buffer.NumElements = binding.size / 4; + desc.Format = DXGI_FORMAT_R32_TYPELESS; desc.ViewDimension = D3D12_UAV_DIMENSION_BUFFER; - desc.Buffer.FirstElement = binding.offset; - desc.Buffer.StructureByteStride = 1; + desc.Buffer.FirstElement = binding.offset / 4; + desc.Buffer.StructureByteStride = 0; desc.Buffer.CounterOffsetInBytes = 0; - desc.Buffer.Flags = D3D12_BUFFER_UAV_FLAG_NONE; + desc.Buffer.Flags = D3D12_BUFFER_UAV_FLAG_RAW; d3d12Device->CreateUnorderedAccessView( ToBackend(binding.buffer)->GetD3D12Resource().Get(), nullptr, &desc, diff --git a/src/tests/end2end/ComputeCopyStorageBufferTests.cpp b/src/tests/end2end/ComputeCopyStorageBufferTests.cpp index c024d8d491..84d2c9a6b0 100644 --- a/src/tests/end2end/ComputeCopyStorageBufferTests.cpp +++ b/src/tests/end2end/ComputeCopyStorageBufferTests.cpp @@ -97,9 +97,6 @@ void ComputeCopyStorageBufferTests::BasicTest(const char* shader) { // Test that a trivial compute-shader memcpy implementation works. TEST_P(ComputeCopyStorageBufferTests, SizedArrayOfBasic) { - // TODO(cwallez@chromium.org): Fails on D3D12, could be a spirv-cross issue? - DAWN_SKIP_TEST_IF(IsD3D12()); - BasicTest(R"( #version 450 #define kInstances 4 @@ -114,7 +111,8 @@ TEST_P(ComputeCopyStorageBufferTests, SizedArrayOfBasic) { // Test that a slightly-less-trivial compute-shader memcpy implementation works. TEST_P(ComputeCopyStorageBufferTests, SizedArrayOfStruct) { - // TODO(kainino@chromium.org): Fails on D3D12. Probably due to a limitation in SPIRV-Cross? + // TODO(kainino@chromium.org): Fails on D3D12 due to SPIRV-Cross not supporting + // reading structs from ByteAddressBuffer. DAWN_SKIP_TEST_IF(IsD3D12()); BasicTest(R"( @@ -134,9 +132,6 @@ TEST_P(ComputeCopyStorageBufferTests, SizedArrayOfStruct) { // Test that a trivial compute-shader memcpy implementation works. TEST_P(ComputeCopyStorageBufferTests, UnsizedArrayOfBasic) { - // TODO(cwallez@chromium.org): Fails on D3D12, could be a spirv-cross issue? - DAWN_SKIP_TEST_IF(IsD3D12()); - BasicTest(R"( #version 450 #define kInstances 4 diff --git a/src/tests/end2end/ComputeIndirectTests.cpp b/src/tests/end2end/ComputeIndirectTests.cpp index b4b88d9283..085bca34d0 100644 --- a/src/tests/end2end/ComputeIndirectTests.cpp +++ b/src/tests/end2end/ComputeIndirectTests.cpp @@ -110,8 +110,6 @@ void ComputeIndirectTests::BasicTest(std::initializer_list bufferList, // Test basic indirect TEST_P(ComputeIndirectTests, Basic) { - // See https://bugs.chromium.org/p/dawn/issues/detail?id=159 - DAWN_SKIP_TEST_IF(IsD3D12() && IsNvidia()); // TODO(hao.x.li@intel.com): Test failing on Metal with validation layer on, which blocks // end2end tests run with validation layer in bots. Suppress this while we're fixing. // See https://bugs.chromium.org/p/dawn/issues/detail?id=139 @@ -122,8 +120,6 @@ TEST_P(ComputeIndirectTests, Basic) { // Test indirect with buffer offset TEST_P(ComputeIndirectTests, IndirectOffset) { - // See https://bugs.chromium.org/p/dawn/issues/detail?id=159 - DAWN_SKIP_TEST_IF(IsD3D12() && IsNvidia()); // TODO(hao.x.li@intel.com): Test failing on Metal with validation layer on, which blocks // end2end tests run with validation layer in bots. Suppress this while we're fixing. // See https://bugs.chromium.org/p/dawn/issues/detail?id=139 diff --git a/src/tests/end2end/ComputeSharedMemoryTests.cpp b/src/tests/end2end/ComputeSharedMemoryTests.cpp index 6434c1fdc1..96f9d759ee 100644 --- a/src/tests/end2end/ComputeSharedMemoryTests.cpp +++ b/src/tests/end2end/ComputeSharedMemoryTests.cpp @@ -80,9 +80,6 @@ void ComputeSharedMemoryTests::BasicTest(const char* shader) { // Basic shared memory test TEST_P(ComputeSharedMemoryTests, Basic) { - // See https://bugs.chromium.org/p/dawn/issues/detail?id=159 - DAWN_SKIP_TEST_IF(IsD3D12() && IsNvidia()); - BasicTest(R"( #version 450 const uint kTileSize = 4;