[dawn][native] Allow a single ResourceTable set on RenderPassEncoder - Update dawn.json to no longer allow passing nullptr to unset the ResourceTable in RenderPassEncoder::SetResourceTable. - Update the validation in RenderPassEncoder. - Remove a lot of validation tests that were setting nullptr or multiple ResourceTables on a RenderPassEncoder. - Add validation tests for the new validation in RenderPassEncoder. - Change end2end tests to only test multiple ResourceTables in ComputePassEncoder. Simplify the code accordingly. Bug: 530981417 Change-Id: Id3c41bf55260405feecaf4210dc6ba5b6034e7ba Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/330736 Reviewed-by: Loko Kung <lokokung@google.com> Commit-Queue: Corentin Wallez <cwallez@chromium.org>
diff --git a/src/dawn/dawn.json b/src/dawn/dawn.json index d84a342..5ebbe2a 100644 --- a/src/dawn/dawn.json +++ b/src/dawn/dawn.json
@@ -3562,7 +3562,7 @@ "name": "set resource table", "tags": ["dawn"], "args": [ - {"name": "table", "type": "resource table", "optional": true} + {"name": "table", "type": "resource table"} ] } ]
diff --git a/src/dawn/native/RenderPassEncoder.cpp b/src/dawn/native/RenderPassEncoder.cpp index f36f9c8..ad91931 100644 --- a/src/dawn/native/RenderPassEncoder.cpp +++ b/src/dawn/native/RenderPassEncoder.cpp
@@ -327,6 +327,8 @@ } void RenderPassEncoder::APISetResourceTable(ResourceTableBase* table) { + DAWN_ASSERT(table != nullptr); + mEncodingContext->TryEncode( this, [&](CommandAllocator* allocator) -> MaybeError { @@ -335,9 +337,13 @@ !GetDevice()->HasFeature(Feature::ChromiumExperimentalSamplingResourceTable), "setResourceTable requires the %s feature enabled.", wgpu::FeatureName::ChromiumExperimentalSamplingResourceTable); - if (table) { - DAWN_TRY(GetDevice()->ValidateObject(table)); - } + DAWN_TRY(GetDevice()->ValidateObject(table)); + + ResourceTableBase* currentTable = mCommandBufferState.GetResourceTable(); + DAWN_INVALID_IF(currentTable != nullptr && table != currentTable, + "Changing from %s to %s is not allowed in a RenderPassEncoder (in " + "the future the table will be set in BeginRenderPass).", + currentTable, table); } mCommandBufferState.SetResourceTable(table);
diff --git a/src/dawn/tests/end2end/ResourceTableTests.cpp b/src/dawn/tests/end2end/ResourceTableTests.cpp index dd864ba..c140e65 100644 --- a/src/dawn/tests/end2end/ResourceTableTests.cpp +++ b/src/dawn/tests/end2end/ResourceTableTests.cpp
@@ -328,10 +328,12 @@ EXPECT_BUFFER_U32_RANGE_EQ(expectedU32.data(), resultBuffer, 0, expectedU32.size()); } - // For each table in `cases`, sets the `table` and dipatches on a render pass/bundle encoder, - // and validates that each `table` has a texture_2d<u32> iff the `expected` has a value, and - // that the textures have the expected value, if any. - void TestHasU8BindingsRender(std::vector<TableAndExpected> cases, bool useRenderBundles) { + // Sets the `table` and dipatches on a render pass/bundle encoder, and validates that `table` + // has a texture_2d<u32> iff `expected` has a value, and that the textures have the expected + // value, if any. + void TestHasU8BindingsRender(wgpu::ResourceTable table, + std::vector<std::optional<uint8_t>> expected, + bool useRenderBundles = false) { wgpu::ShaderModule module = utils::CreateShaderModule(device, R"( enable chromium_experimental_resource_table; @@ -340,32 +342,21 @@ } @group(0) @binding(0) var<storage, read_write> results : array<u32>; - struct Immediates { - resourceCount : u32, - offset : u32, - } - var<immediate> immediates : Immediates; + var<immediate> resourceCount : u32; @fragment fn main() -> @location(0) vec4f { - for (var i = 0u; i < immediates.resourceCount; i++) { + for (var i = 0u; i < resourceCount; i++) { if !hasResource<texture_2d<u32>>(i) { - results[immediates.offset + i] = 0xBEEF; + results[i] = 0xBEEF; } else { let tex = getResource<texture_2d<u32>>(i); - results[immediates.offset + i] = textureLoad(tex, vec2(0), 0).x; + results[i] = textureLoad(tex, vec2(0), 0).x; } } return vec4(); } )"); - // Make the result buffer large enough for all cases - size_t resultSize = 0; - for (auto& [table, expected] : cases) { - ASSERT_EQ(table.GetSize(), expected.size()); - resultSize += expected.size(); - } - wgpu::BindGroupLayout resultBGL = utils::MakeBindGroupLayout( device, {{0, wgpu::ShaderStage::Fragment, wgpu::BufferBindingType::Storage}}); @@ -382,7 +373,7 @@ // Create the result buffer. wgpu::BufferDescriptor bDesc = { .usage = wgpu::BufferUsage::Storage | wgpu::BufferUsage::CopySrc, - .size = sizeof(uint32_t) * resultSize, + .size = sizeof(uint32_t) * expected.size(), }; wgpu::Buffer resultBuffer = device.CreateBuffer(&bDesc); wgpu::BindGroup resultBG = utils::MakeBindGroup(device, resultBGL, {{0, resultBuffer}}); @@ -391,39 +382,29 @@ auto rp = utils::CreateBasicRenderPass(device, 1, 1); wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&rp.renderPassInfo); + pass.SetResourceTable(table); if (useRenderBundles) { - pass.SetResourceTable(cases[0].table); - utils::ComboRenderBundleEncoderDescriptor desc = {}; desc.SetUsesResourceTable(); desc.colorFormatCount = 1; desc.cColorFormats[0] = rp.colorFormat; wgpu::RenderBundleEncoder rbe = device.CreateRenderBundleEncoder(&desc); - uint32_t offset = 0; - for (auto& [table, expected] : cases) { - uint32_t immediates[] = {table.GetSize(), offset}; - rbe.SetImmediates(0, &immediates, sizeof(immediates)); - rbe.SetBindGroup(0, resultBG); - rbe.SetPipeline(testPipeline); - rbe.Draw(1); - offset += expected.size(); - } + uint32_t immediates = table.GetSize(); + rbe.SetImmediates(0, &immediates, sizeof(immediates)); + rbe.SetBindGroup(0, resultBG); + rbe.SetPipeline(testPipeline); + rbe.Draw(1); wgpu::RenderBundle bundle = rbe.Finish(); pass.ExecuteBundles(1, &bundle); } else { - uint32_t offset = 0; - for (auto& [table, expected] : cases) { - uint32_t immediates[] = {table.GetSize(), offset}; - pass.SetResourceTable(table); - pass.SetImmediates(0, &immediates, sizeof(immediates)); - pass.SetBindGroup(0, resultBG); - pass.SetPipeline(testPipeline); - pass.Draw(1); - offset += expected.size(); - } + uint32_t immediates = table.GetSize(); + pass.SetImmediates(0, &immediates, sizeof(immediates)); + pass.SetBindGroup(0, resultBG); + pass.SetPipeline(testPipeline); + pass.Draw(1); } pass.End(); @@ -432,30 +413,19 @@ // Check we have the expected results. std::vector<uint32_t> expectedU32; - for (auto& [_, expected] : cases) { - for (auto optValue : expected) { - expectedU32.push_back(optValue ? *optValue : 0xBEEFu); - } + for (auto optValue : expected) { + expectedU32.push_back(optValue ? *optValue : 0xBEEFu); } EXPECT_BUFFER_U32_RANGE_EQ(expectedU32.data(), resultBuffer, 0, expectedU32.size()); } // Convenience that tests cases using compute, render, and render bundle encoders - void TestHasU8BindingsAll(std::vector<TableAndExpected> cases) { - TestHasU8BindingsCompute(cases); - // TODO(https://crbug.com/530981417): When a single resource table is allowed per render - // pass, remove simplify TestHasU8Binding* to assume there is a single resource table. - if (cases.size() == 1) { - TestHasU8BindingsRender(cases, true); - } - TestHasU8BindingsRender(cases, false); - } - - // Convenience for single table void TestHasU8BindingsAll(wgpu::ResourceTable table, std::vector<std::optional<uint8_t>> expected) { - TestHasU8BindingsAll({{table, expected}}); + TestHasU8BindingsCompute({{table, expected}}); + TestHasU8BindingsRender(table, expected, true); + TestHasU8BindingsRender(table, expected, false); } // Creates a sampler by address mode @@ -2806,9 +2776,8 @@ TestHasU8BindingsAll(table, {{19}}); } -// Check that setting multiple resource table, on per dispatch/draw/executebundle, on a single pass -// works. -TEST_P(ResourceTableTests, SinglePassMultipleResourceTables) { +// Check that setting multiple resource table, on per dispatch, on a single pass works. +TEST_P(ResourceTableTests, SingleComputePassMultipleResourceTables) { // TODO(crbug.com/385158827): Fails on older WARP 10.0.19041.5794 DAWN_SUPPRESS_TEST_IF(IsWARP()); @@ -2840,9 +2809,9 @@ auto case1 = TableAndExpected(table1, {{27, {}, 29}}); auto case2 = TableAndExpected(table2, {{37, 38, {}, 40}}); - TestHasU8BindingsAll({case0, case1, case2}); - TestHasU8BindingsAll({case1, case0, case2}); - TestHasU8BindingsAll({case2, case1, case0}); + TestHasU8BindingsCompute({case0, case1, case2}); + TestHasU8BindingsCompute({case1, case0, case2}); + TestHasU8BindingsCompute({case2, case1, case0}); } // Check that logic to dirty or reuse VkDescriptorSet takes into account the resource table in the
diff --git a/src/dawn/tests/unittests/validation/ResourceTableValidationTests.cpp b/src/dawn/tests/unittests/validation/ResourceTableValidationTests.cpp index 2cba5d5..cf7b073 100644 --- a/src/dawn/tests/unittests/validation/ResourceTableValidationTests.cpp +++ b/src/dawn/tests/unittests/validation/ResourceTableValidationTests.cpp
@@ -598,10 +598,12 @@ // Failure case: invalid encoder state { + wgpu::ResourceTable resourceTable = MakeResourceTable(1); + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&rp.renderPassInfo); pass.End(); - pass.SetResourceTable(nullptr); + pass.SetResourceTable(resourceTable); ASSERT_DEVICE_ERROR(encoder.Finish()); } @@ -619,6 +621,7 @@ // Success case: valid resource table { wgpu::ResourceTable resourceTable = MakeResourceTable(1); + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&rp.renderPassInfo); pass.SetResourceTable(resourceTable); @@ -626,14 +629,30 @@ encoder.Finish(); } - // Success case: null resource table + // Success case: valid resource table set twice { + wgpu::ResourceTable resourceTable = MakeResourceTable(1); + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&rp.renderPassInfo); - pass.SetResourceTable(nullptr); + pass.SetResourceTable(resourceTable); + pass.SetResourceTable(resourceTable); pass.End(); encoder.Finish(); } + + // Error case: two different resource tables are set + { + wgpu::ResourceTable resourceTable1 = MakeResourceTable(1); + wgpu::ResourceTable resourceTable2 = MakeResourceTable(1); + + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&rp.renderPassInfo); + pass.SetResourceTable(resourceTable1); + pass.SetResourceTable(resourceTable2); + pass.End(); + ASSERT_DEVICE_ERROR(encoder.Finish()); + } } // Tests that the resource table in RenderBundle can be used in draw @@ -741,18 +760,6 @@ ASSERT_DEVICE_ERROR(encoder.Finish()); } - // Error case: render bundle using table causes an error if a ResourceTable was set then - // removed. - { - wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&rp.renderPassInfo); - pass.SetResourceTable(MakeResourceTable(1)); - pass.SetResourceTable(nullptr); - pass.ExecuteBundles(1, &rbTable); - pass.End(); - ASSERT_DEVICE_ERROR(encoder.Finish()); - } - // Error case: at least one of the render bundles executed requires a resource table set. { std::array<wgpu::RenderBundle, 3u> rbs = {rbNoTable, rbTable, rbNoTable}; @@ -785,17 +792,6 @@ ASSERT_DEVICE_ERROR(encoder.Finish()); } -// Tests calling RenderPassEncoder::SetResourceTable when the feature is disabled -TEST_F(ResourceTableValidationTestDisabled, RenderPassEncoder_SetResourceTable) { - // Failure case: feature is disabled - auto rp = utils::CreateBasicRenderPass(device, 1, 1, wgpu::TextureFormat::RGBA8Unorm); - wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&rp.renderPassInfo); - pass.SetResourceTable(nullptr); - pass.End(); - ASSERT_DEVICE_ERROR(encoder.Finish()); -} - // Tests calling RenderBundleEncoderResourceTable is not allowed when the feature is disabled TEST_F(ResourceTableValidationTestDisabled, RenderBundleEncoderResourceTable) { // Control case: RenderBundleEncoderResourceTable is not chained. @@ -948,47 +944,6 @@ resourceTable.Destroy(); // Destroy it ASSERT_DEVICE_ERROR(device.GetQueue().Submit(1, &commands)); } - - // Failure case: one of multiple resource tables has been destroyed - { - wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&rp.renderPassInfo); - pass.SetResourceTable(resourceTable); - pass.SetResourceTable(resourceTable2); - pass.SetResourceTable(resourceTable3); - pass.End(); - wgpu::CommandBuffer commands = encoder.Finish(); - resourceTable2.Destroy(); // Destroy one - ASSERT_DEVICE_ERROR(device.GetQueue().Submit(1, &commands)); - } - - // Failure case: one of multiple resource tables in another pass has been destroyed - { - wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&rp.renderPassInfo); - pass.SetResourceTable(resourceTable); - pass.SetResourceTable(resourceTable2); - pass.End(); - wgpu::RenderPassEncoder pass2 = encoder.BeginRenderPass(&rp.renderPassInfo); - pass2.SetResourceTable(resourceTable); - pass2.SetResourceTable(resourceTable3); - pass2.End(); - wgpu::CommandBuffer commands = encoder.Finish(); - resourceTable3.Destroy(); // Destroy one - ASSERT_DEVICE_ERROR(device.GetQueue().Submit(1, &commands)); - } - - // Failure case: resource table must still be valid if set, then nullptr is set - { - wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&rp.renderPassInfo); - pass.SetResourceTable(resourceTable); - pass.SetResourceTable(nullptr); // Clear it - pass.End(); - wgpu::CommandBuffer commands = encoder.Finish(); - resourceTable.Destroy(); // Destroy it - ASSERT_DEVICE_ERROR(device.GetQueue().Submit(1, &commands)); - } } // Tests that the resource table can be used in dispatch @@ -1110,33 +1065,6 @@ ASSERT_DEVICE_ERROR(wgpu::CommandBuffer commands = encoder.Finish()); } - // Failure case: `usesResourceTable` is enabled, one then nullptr set on the pass - { - wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&rp.renderPassInfo); - pass.SetResourceTable(resourceTable); // Set a valid one - pass.SetResourceTable(nullptr); // Then clear it - pass.SetPipeline(pipelineUsesTable); - pass.Draw(1); - pass.End(); - ASSERT_DEVICE_ERROR(wgpu::CommandBuffer commands = encoder.Finish()); - } - - // Success case: `usesResourceTable` is enabled, one then nullptr then another set on the - // pass - { - wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&rp.renderPassInfo); - pass.SetResourceTable(resourceTable); - pass.SetResourceTable(nullptr); // Then clear it - pass.SetResourceTable(resourceTable2); // Then set another valid one - pass.SetPipeline(pipelineUsesTable); - pass.Draw(1); - pass.End(); - wgpu::CommandBuffer commands = encoder.Finish(); - device.GetQueue().Submit(1, &commands); - } - // Success case: single pass toggles between pipelines that do not use and use a table { wgpu::CommandEncoder encoder = device.CreateCommandEncoder();