diff --git a/src/dawn_native/BindGroupTracker.h b/src/dawn_native/BindGroupTracker.h index a3addb27b1..f7a9142083 100644 --- a/src/dawn_native/BindGroupTracker.h +++ b/src/dawn_native/BindGroupTracker.h @@ -60,13 +60,16 @@ namespace dawn_native { void OnSetPipeline(PipelineBase* pipeline) { mPipelineLayout = pipeline->GetLayout(); + } + + protected: + // The Derived class should call this before it applies bind groups. + void BeforeApply() { if (mLastAppliedPipelineLayout == mPipelineLayout) { return; } - // Keep track of the bind group layout mask to avoid marking unused bind groups as - // dirty. This also allows us to avoid computing the intersection of the dirty bind - // groups and bind group layout mask in Draw or Dispatch which is very hot code. + // Use the bind group layout mask to avoid marking unused bind groups as dirty. mBindGroupLayoutsMask = mPipelineLayout->GetBindGroupLayoutsMask(); // Changing the pipeline layout sets bind groups as dirty. If CanInheritBindGroups, @@ -88,13 +91,15 @@ namespace dawn_native { } } - protected: - // The Derived class should call this when it applies bind groups. - void DidApply() { + // The Derived class should call this after it applies bind groups. + void AfterApply() { // Reset all dirty bind groups. Dirty bind groups not in the bind group layout mask // will be dirtied again by the next pipeline change. mDirtyBindGroups.reset(); mDirtyBindGroupsObjectChangedOrIsDynamic.reset(); + // Keep track of the last applied pipeline layout. This allows us to avoid computing + // the intersection of the dirty bind groups and bind group layout mask in next Draw + // or Dispatch (which is very hot code) until the layout is changed again. mLastAppliedPipelineLayout = mPipelineLayout; } diff --git a/src/dawn_native/d3d12/CommandBufferD3D12.cpp b/src/dawn_native/d3d12/CommandBufferD3D12.cpp index 626ca9b424..cf93b20335 100644 --- a/src/dawn_native/d3d12/CommandBufferD3D12.cpp +++ b/src/dawn_native/d3d12/CommandBufferD3D12.cpp @@ -338,6 +338,8 @@ namespace dawn_native { namespace d3d12 { } MaybeError Apply(CommandRecordingContext* commandContext) { + BeforeApply(); + // Bindgroups are allocated in shader-visible descriptor heaps which are managed by a // ringbuffer. There can be a single shader-visible descriptor heap of each type bound // at any given time. This means that when we switch heaps, all other currently bound @@ -389,7 +391,7 @@ namespace dawn_native { namespace d3d12 { mDynamicOffsetCounts[index], mDynamicOffsets[index].data()); } - DidApply(); + AfterApply(); return {}; } diff --git a/src/dawn_native/metal/CommandBufferMTL.mm b/src/dawn_native/metal/CommandBufferMTL.mm index d1c5b7c6c9..e49ab1052f 100644 --- a/src/dawn_native/metal/CommandBufferMTL.mm +++ b/src/dawn_native/metal/CommandBufferMTL.mm @@ -359,13 +359,14 @@ namespace dawn_native { namespace metal { template void Apply(Encoder encoder) { + BeforeApply(); for (BindGroupIndex index : IterateBitSet(mDirtyBindGroupsObjectChangedOrIsDynamic)) { ApplyBindGroup(encoder, index, ToBackend(mBindGroups[index]), mDynamicOffsetCounts[index], mDynamicOffsets[index].data(), ToBackend(mPipelineLayout)); } - DidApply(); + AfterApply(); } private: diff --git a/src/dawn_native/opengl/CommandBufferGL.cpp b/src/dawn_native/opengl/CommandBufferGL.cpp index 39bb465bb2..1534e090d6 100644 --- a/src/dawn_native/opengl/CommandBufferGL.cpp +++ b/src/dawn_native/opengl/CommandBufferGL.cpp @@ -225,12 +225,13 @@ namespace dawn_native { namespace opengl { } void Apply(const OpenGLFunctions& gl) { + BeforeApply(); for (BindGroupIndex index : IterateBitSet(mDirtyBindGroupsObjectChangedOrIsDynamic)) { ApplyBindGroup(gl, index, mBindGroups[index], mDynamicOffsetCounts[index], mDynamicOffsets[index].data()); } - DidApply(); + AfterApply(); } private: diff --git a/src/dawn_native/vulkan/CommandBufferVk.cpp b/src/dawn_native/vulkan/CommandBufferVk.cpp index 0d6dcda580..e5ca8c61cb 100644 --- a/src/dawn_native/vulkan/CommandBufferVk.cpp +++ b/src/dawn_native/vulkan/CommandBufferVk.cpp @@ -130,6 +130,7 @@ namespace dawn_native { namespace vulkan { void Apply(Device* device, CommandRecordingContext* recordingContext, VkPipelineBindPoint bindPoint) { + BeforeApply(); for (BindGroupIndex dirtyIndex : IterateBitSet(mDirtyBindGroupsObjectChangedOrIsDynamic)) { VkDescriptorSet set = ToBackend(mBindGroups[dirtyIndex])->GetHandle(); @@ -141,7 +142,7 @@ namespace dawn_native { namespace vulkan { ToBackend(mPipelineLayout)->GetHandle(), static_cast(dirtyIndex), 1, &*set, mDynamicOffsetCounts[dirtyIndex], dynamicOffset); } - DidApply(); + AfterApply(); } }; diff --git a/src/tests/end2end/BindGroupTests.cpp b/src/tests/end2end/BindGroupTests.cpp index e30288f236..314713f683 100644 --- a/src/tests/end2end/BindGroupTests.cpp +++ b/src/tests/end2end/BindGroupTests.cpp @@ -755,6 +755,110 @@ TEST_P(BindGroupTests, DrawThenChangePipelineAndBindGroup) { EXPECT_PIXEL_RGBA8_EQ(notFilled, renderPass.color, max, max); } +// Test for crbug.com/dawn/1049, where setting a pipeline without drawing can prevent +// bind groups from being applied later +TEST_P(BindGroupTests, DrawThenChangePipelineTwiceAndBindGroup) { + // TODO(crbug.com/dawn/1055) find out why this test fails on Windows Intel D3D12 drivers. + DAWN_SUPPRESS_TEST_IF(IsIntel() && IsWindows() && IsD3D12()); + + utils::BasicRenderPass renderPass = utils::CreateBasicRenderPass(device, kRTSize, kRTSize); + + // Create a bind group layout which uses a single dynamic uniform buffer. + wgpu::BindGroupLayout uniformLayout = utils::MakeBindGroupLayout( + device, {{0, wgpu::ShaderStage::Fragment, wgpu::BufferBindingType::Uniform, true}}); + + // Create a pipeline with pipeline layout (uniform, uniform, uniform). + wgpu::RenderPipeline pipeline0 = + MakeTestPipeline(renderPass, + {wgpu::BufferBindingType::Uniform, wgpu::BufferBindingType::Uniform, + wgpu::BufferBindingType::Uniform}, + {uniformLayout, uniformLayout, uniformLayout}); + + // Create a pipeline with pipeline layout (uniform). + wgpu::RenderPipeline pipeline1 = MakeTestPipeline( + renderPass, {wgpu::BufferBindingType::Uniform, wgpu::BufferBindingType::Uniform}, + {uniformLayout, uniformLayout}); + + // Prepare color data. + // The first draw will use { color0, color1, color2 }. + // The second draw will use { color0, color1, color3 }. + // The pipeline uses additive color and alpha so the result of two draws should be + // { 2 * color0 + 2 * color1 + color2 + color3} = RGBAunorm(1, 1, 1, 1) + std::array color0 = {0.501, 0, 0, 0}; + std::array color1 = {0, 0.501, 0, 0}; + std::array color2 = {0, 0, 1, 0}; + std::array color3 = {0, 0, 0, 1}; + + size_t color0Offset = 0; + size_t color1Offset = Align(color0Offset + sizeof(color0), kMinUniformBufferOffsetAlignment); + size_t color2Offset = Align(color1Offset + sizeof(color1), kMinUniformBufferOffsetAlignment); + size_t color3Offset = Align(color2Offset + sizeof(color2), kMinUniformBufferOffsetAlignment); + + std::vector data(color3Offset + sizeof(color3), 0); + memcpy(data.data(), color0.data(), sizeof(color0)); + memcpy(data.data() + color1Offset, color1.data(), sizeof(color1)); + memcpy(data.data() + color2Offset, color2.data(), sizeof(color2)); + memcpy(data.data() + color3Offset, color3.data(), sizeof(color3)); + + // Create a uniform and storage buffer bind groups to bind the color data. + wgpu::Buffer uniformBuffer = + utils::CreateBufferFromData(device, data.data(), data.size(), wgpu::BufferUsage::Uniform); + + wgpu::BindGroup uniformBindGroup = + utils::MakeBindGroup(device, uniformLayout, {{0, uniformBuffer, 0, 4 * sizeof(float)}}); + + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass.renderPassInfo); + + // Set the pipeline to (uniform, uniform, storage) + pass.SetPipeline(pipeline0); + + // Set the first bind group to color0 in the dynamic uniform buffer. + uint32_t dynamicOffset = color0Offset; + pass.SetBindGroup(0, uniformBindGroup, 1, &dynamicOffset); + + // Set the first bind group to color1 in the dynamic uniform buffer. + dynamicOffset = color1Offset; + pass.SetBindGroup(1, uniformBindGroup, 1, &dynamicOffset); + + // Set the first bind group to color2 in the dynamic uniform buffer. + dynamicOffset = color2Offset; + pass.SetBindGroup(2, uniformBindGroup, 1, &dynamicOffset); + + // This draw will internally apply bind groups for pipeline 0. + pass.Draw(3); + + // When we set pipeline 1, which has no bind group at index 2 in its layout, it + // should not prevent bind group 2 from being used after reverting to pipeline 0. + // More specifically, internally the pipeline 1 layout should not be saved, + // because we never applied the bind groups via a Draw or Dispatch. + pass.SetPipeline(pipeline1); + + // Set the second bind group to color3 in the dynamic uniform buffer. + dynamicOffset = color3Offset; + pass.SetBindGroup(2, uniformBindGroup, 1, &dynamicOffset); + + // Revert to pipeline 0 + pass.SetPipeline(pipeline0); + + // Internally this will not re-apply the bind groups, because we already + // drew with this pipeline (setting pipeline 1 did not dirty the bind groups). + pass.Draw(3); + + pass.EndPass(); + + wgpu::CommandBuffer commands = encoder.Finish(); + queue.Submit(1, &commands); + + RGBA8 filled(255, 255, 255, 255); + RGBA8 notFilled(0, 0, 0, 0); + uint32_t min = 1, max = kRTSize - 3; + EXPECT_PIXEL_RGBA8_EQ(filled, renderPass.color, min, min); + EXPECT_PIXEL_RGBA8_EQ(filled, renderPass.color, max, min); + EXPECT_PIXEL_RGBA8_EQ(filled, renderPass.color, min, max); + EXPECT_PIXEL_RGBA8_EQ(notFilled, renderPass.color, max, max); +} + // Regression test for crbug.com/dawn/408 where dynamic offsets were applied in the wrong order. // Dynamic offsets should be applied in increasing order of binding number. TEST_P(BindGroupTests, DynamicOffsetOrder) {