[dawn] remove device lock from SetLabel This change adds "no autolock": true to the "set label" methods in dawn.json for objects that inherit from ApiObjectBase. This only removes the lock for front-end label updates. Backend label settings are still protected by the device lock for now. It also adds multithreaded tests to verify that setting labels concurrently from multiple threads is safe and doesn't interfere with validation. Bug: 479457809 Change-Id: Icd8d6ad054500c6a2f0c0be8ba04f205e0b95740 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/297435 Reviewed-by: Corentin Wallez <cwallez@chromium.org> Reviewed-by: Kyle Charbonneau <kylechar@google.com> Auto-Submit: Quyen Le <lehoangquyen@chromium.org> Commit-Queue: Kyle Charbonneau <kylechar@google.com>
diff --git a/src/dawn/dawn.json b/src/dawn/dawn.json index b3dc87a..8c169db 100644 --- a/src/dawn/dawn.json +++ b/src/dawn/dawn.json
@@ -325,6 +325,7 @@ "methods": [ { "name": "set label", + "no autolock": true, "args": [ {"name": "label", "type": "string view"} ] @@ -358,6 +359,7 @@ "methods": [ { "name": "set label", + "no autolock": true, "args": [ {"name": "label", "type": "string view"} ] @@ -672,6 +674,7 @@ }, { "name": "set label", + "no autolock": true, "args": [ {"name": "label", "type": "string view"} ] @@ -815,6 +818,7 @@ "methods": [ { "name": "set label", + "no autolock": true, "args": [ {"name": "label", "type": "string view"} ] @@ -947,6 +951,7 @@ }, { "name": "set label", + "no autolock": true, "args": [ {"name": "label", "type": "string view"} ] @@ -1107,6 +1112,7 @@ }, { "name": "set label", + "no autolock": true, "args": [ {"name": "label", "type": "string view"} ] @@ -1152,6 +1158,7 @@ }, { "name": "set label", + "no autolock": true, "args": [ {"name": "label", "type": "string view"} ] @@ -1774,6 +1781,7 @@ "methods": [ { "name": "set label", + "no autolock": true, "args": [ {"name": "label", "type": "string view"} ] @@ -1840,6 +1848,7 @@ "methods": [ { "name": "set label", + "no autolock": true, "args": [ {"name": "label", "type": "string view"} ] @@ -1914,6 +1923,7 @@ "methods": [ { "name": "set label", + "no autolock": true, "args": [ {"name": "label", "type": "string view"} ] @@ -2760,6 +2770,7 @@ "methods": [ { "name": "set label", + "no autolock": true, "args": [ {"name": "label", "type": "string view"} ] @@ -2850,6 +2861,7 @@ "methods": [ { "name": "set label", + "no autolock": true, "args": [ {"name": "label", "type": "string view"} ] @@ -2943,6 +2955,7 @@ }, { "name": "set label", + "no autolock": true, "args": [ {"name": "label", "type": "string view"} ] @@ -2985,6 +2998,7 @@ "methods": [ { "name": "set label", + "no autolock": true, "args": [ {"name": "label", "type": "string view"} ] @@ -3087,6 +3101,7 @@ }, { "name": "set label", + "no autolock": true, "args": [ {"name": "label", "type": "string view"} ] @@ -3410,6 +3425,7 @@ }, { "name": "set label", + "no autolock": true, "args": [ {"name": "label", "type": "string view"} ] @@ -3445,6 +3461,7 @@ }, { "name": "set label", + "no autolock": true, "args": [ {"name": "label", "type": "string view"} ] @@ -3647,6 +3664,7 @@ "methods": [ { "name": "set label", + "no autolock": true, "args": [ {"name": "label", "type": "string view"} ] @@ -3682,6 +3700,7 @@ }, { "name": "set label", + "no autolock": true, "args": [ {"name": "label", "type": "string view"} ] @@ -4062,6 +4081,7 @@ }, { "name": "set label", + "no autolock": true, "args": [ {"name": "label", "type": "string view"} ] @@ -4373,6 +4393,7 @@ "methods": [ { "name": "set label", + "no autolock": true, "args": [ {"name": "label", "type": "string view"} ] @@ -4385,6 +4406,7 @@ "methods": [ { "name": "set label", + "no autolock": true, "args": [ {"name": "label", "type": "string view"} ]
diff --git a/src/dawn/native/ObjectBase.cpp b/src/dawn/native/ObjectBase.cpp index 66d6c72..fd1c174 100644 --- a/src/dawn/native/ObjectBase.cpp +++ b/src/dawn/native/ObjectBase.cpp
@@ -35,6 +35,7 @@ #include "dawn/native/Device.h" #include "dawn/native/ObjectLabel.h" #include "dawn/native/ObjectType_autogen.h" +#include "dawn/native/Toggles.h" #include "dawn/native/utils/WGPUHelpers.h" namespace dawn::native { @@ -189,6 +190,14 @@ } } labelObj->SetValue(std::move(label)); + + if (!GetDevice()->IsToggleEnabled(Toggle::UseUserDefinedLabelsInBackend)) { + return; + } + + // TODO(479457809): remove this once all backends' SetLabelImpl() implementations are thread + // safe + auto deviceGuard = GetDevice()->GetGuard(); SetLabelImpl(); }
diff --git a/src/dawn/native/webgpu/PhysicalDeviceWGPU.cpp b/src/dawn/native/webgpu/PhysicalDeviceWGPU.cpp index 43bdacc..8a68c72 100644 --- a/src/dawn/native/webgpu/PhysicalDeviceWGPU.cpp +++ b/src/dawn/native/webgpu/PhysicalDeviceWGPU.cpp
@@ -34,6 +34,7 @@ #include "dawn/native/ChainUtils.h" #include "dawn/native/Features_autogen.h" #include "dawn/native/Instance.h" +#include "dawn/native/Toggles.h" #include "dawn/native/webgpu/BackendWGPU.h" #include "dawn/native/webgpu/DeviceWGPU.h" @@ -154,7 +155,10 @@ TogglesState* adapterToggles) const {} void PhysicalDevice::SetupBackendDeviceToggles(dawn::platform::Platform* platform, - TogglesState* deviceToggles) const {} + TogglesState* deviceToggles) const { + // We should always use this toggle in order to capture the label. + deviceToggles->ForceSet(Toggle::UseUserDefinedLabelsInBackend, true); +} ResultOrError<Ref<DeviceBase>> PhysicalDevice::CreateDeviceImpl( AdapterBase* adapter,
diff --git a/src/dawn/tests/end2end/MultithreadTests.cpp b/src/dawn/tests/end2end/MultithreadTests.cpp index 3b29079..b95e1cf 100644 --- a/src/dawn/tests/end2end/MultithreadTests.cpp +++ b/src/dawn/tests/end2end/MultithreadTests.cpp
@@ -1136,6 +1136,122 @@ } } +// Test that creating, labeling and using various objects in parallel works. +TEST_P(MultithreadTests, SetLabelInParallel) { + // TODO(crbug.com/451928481): multithread support in GL is incomplete + DAWN_SUPPRESS_TEST_IF(IsOpenGL() || IsOpenGLES()); + + constexpr uint32_t kNumThreads = 20; + constexpr uint32_t kSize = 1; + + utils::RGBA8 initialColor(255, 255, 255, 255); + wgpu::Buffer buffer = utils::CreateBufferFromData( + device, wgpu::BufferUsage::CopySrc | wgpu::BufferUsage::CopyDst, {initialColor}); + wgpu::Texture texture = + CreateTexture(kSize, kSize, wgpu::TextureFormat::RGBA8Unorm, + wgpu::TextureUsage::CopyDst | wgpu::TextureUsage::TextureBinding); + wgpu::TextureView view = texture.CreateView(); + + // Create render pipeline once. + wgpu::ShaderModule vsModule = utils::CreateShaderModule(device, R"( + @vertex fn main(@builtin(vertex_index) i : u32) -> @builtin(position) vec4f { + const pos = array(vec2f(-1, -1), vec2f(3, -1), vec2f(-1, 3)); + return vec4f(pos[i], 0.0, 1.0); + })"); + wgpu::ShaderModule fsModule = utils::CreateShaderModule(device, R"( + @group(0) @binding(0) var s : sampler; + @group(0) @binding(1) var t : texture_2d<f32>; + @fragment fn main() -> @location(0) vec4f { + return textureSample(t, s, vec2f(0.5, 0.5)); + } + )"); + + utils::ComboRenderPipelineDescriptor pipelineDesc; + pipelineDesc.vertex.module = vsModule; + pipelineDesc.cFragment.module = fsModule; + pipelineDesc.cTargets[0].format = wgpu::TextureFormat::RGBA8Unorm; + wgpu::RenderPipeline pipeline = device.CreateRenderPipeline(&pipelineDesc); + + wgpu::Sampler sampler = device.CreateSampler(); + + utils::RGBA8 expectedColor(255, 0, 0, 255); + utils::RunInParallel(kNumThreads, [&](uint32_t index) { + // Set labels concurrently. + std::string bufferLabel = "ThreadBuffer" + std::to_string(index); + std::string textureLabel = "ThreadTexture" + std::to_string(index); + std::string viewLabel = "ThreadView" + std::to_string(index); + + buffer.SetLabel(bufferLabel.c_str()); + texture.SetLabel(textureLabel.c_str()); + view.SetLabel(viewLabel.c_str()); + + // Update buffer data with the same color. + queue.WriteBuffer(buffer, 0, &expectedColor, sizeof(expectedColor)); + + // Copy buffer to texture. + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::TexelCopyBufferInfo src = utils::CreateTexelCopyBufferInfo(buffer, 0, 256); + wgpu::TexelCopyTextureInfo dst = utils::CreateTexelCopyTextureInfo(texture); + wgpu::Extent3D copySize = {kSize, kSize, 1}; + encoder.CopyBufferToTexture(&src, &dst, ©Size); + + wgpu::BindGroup bindGroup = + utils::MakeBindGroup(device, pipeline.GetBindGroupLayout(0), {{0, sampler}, {1, view}}); + + // Render pass. + auto renderPass = utils::CreateBasicRenderPass(device, kSize, kSize); + wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass.renderPassInfo); + pass.SetPipeline(pipeline); + pass.SetBindGroup(0, bindGroup); + pass.Draw(3); + pass.End(); + wgpu::CommandBuffer commands = encoder.Finish(); + queue.Submit(1, &commands); + + EXPECT_PIXEL_RGBA8_EQ(expectedColor, renderPass.color, 0, 0); + }); +} + +// Test that setting label in parallel with a validation error doesn't race or crash. +TEST_P(MultithreadTests, SetLabelAndValidationInParallel) { + // TODO(crbug.com/451928481): multithread support in GL is incomplete + DAWN_SUPPRESS_TEST_IF(IsOpenGL() || IsOpenGLES()); + + DAWN_TEST_UNSUPPORTED_IF(HasToggleEnabled("skip_validation")); + + constexpr uint32_t kNumThreads = 20; + wgpu::Buffer buffer = CreateBuffer(4, wgpu::BufferUsage::CopySrc | wgpu::BufferUsage::CopyDst); + + utils::RunInParallel(kNumThreads, [&](uint32_t index) { + // Set label concurrently. + std::string bufferLabel = "ThreadBuffer" + std::to_string(index); + buffer.SetLabel(bufferLabel.c_str()); + + // Perform an invalid operation (copy buffer to itself). + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + encoder.CopyBufferToBuffer(buffer, 0, buffer, 0, 4); + + device.PushErrorScope(wgpu::ErrorFilter::Validation); + wgpu::CommandBuffer commands = encoder.Finish(); + queue.Submit(1, &commands); + + std::atomic<bool> errorThrown(false); + device.PopErrorScope(wgpu::CallbackMode::AllowProcessEvents, + [&errorThrown](wgpu::PopErrorScopeStatus status, wgpu::ErrorType type, + wgpu::StringView message) { + EXPECT_EQ(status, wgpu::PopErrorScopeStatus::Success); + EXPECT_EQ(type, wgpu::ErrorType::Validation); + EXPECT_THAT(std::string(message), + testing::HasSubstr("ThreadBuffer")); + errorThrown = true; + }); + + while (!errorThrown.load()) { + WaitABit(); + } + }); +} + class MultithreadCachingTests : public MultithreadTests { protected: wgpu::ShaderModule CreateComputeShaderModule() const {