From 1318c603d945b9c6804dbd3b82fd4b14b60af3e5 Mon Sep 17 00:00:00 2001 From: Corentin Wallez Date: Thu, 9 Jul 2020 06:12:04 +0000 Subject: [PATCH] Don't rely on null::Queue::Submit resolving mapping operations. In the validation tests, we relied on Queue.Submit(0, nullptr) to resolve mapping operations. This is fragile so we replace it with a FlushMappingOperations() function that uses device.Tick() instead. This allows removing the mapSerial argument from Buffer::MapRead/WriteAsyncImpl (which was the actual goal of this CL). Bug: dawn:445 Change-Id: Id98822287370c371bebb83afb8e290e17f3c1b55 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/24381 Reviewed-by: Austin Eng Reviewed-by: Kai Ninomiya Commit-Queue: Corentin Wallez --- src/dawn_native/Buffer.cpp | 8 +- src/dawn_native/Buffer.h | 4 +- src/dawn_native/MapRequestTracker.h | 2 +- src/dawn_native/d3d12/BufferD3D12.cpp | 4 +- src/dawn_native/d3d12/BufferD3D12.h | 4 +- src/dawn_native/metal/BufferMTL.h | 4 +- src/dawn_native/metal/BufferMTL.mm | 4 +- src/dawn_native/null/DeviceNull.cpp | 29 +------ src/dawn_native/null/DeviceNull.h | 5 +- src/dawn_native/opengl/BufferGL.cpp | 4 +- src/dawn_native/opengl/BufferGL.h | 4 +- src/dawn_native/vulkan/BufferVk.cpp | 4 +- src/dawn_native/vulkan/BufferVk.h | 4 +- .../validation/BufferValidationTests.cpp | 82 +++++++++---------- .../validation/QueueSubmitValidationTests.cpp | 22 ++--- .../unittests/validation/ValidationTest.cpp | 15 ++++ .../unittests/validation/ValidationTest.h | 2 + 17 files changed, 92 insertions(+), 109 deletions(-) diff --git a/src/dawn_native/Buffer.cpp b/src/dawn_native/Buffer.cpp index de297a8a5a..5f814be5e9 100644 --- a/src/dawn_native/Buffer.cpp +++ b/src/dawn_native/Buffer.cpp @@ -64,11 +64,11 @@ namespace dawn_native { return {}; } - MaybeError MapReadAsyncImpl(uint32_t serial) override { + MaybeError MapReadAsyncImpl() override { UNREACHABLE(); return {}; } - MaybeError MapWriteAsyncImpl(uint32_t serial) override { + MaybeError MapWriteAsyncImpl() override { UNREACHABLE(); return {}; } @@ -272,7 +272,7 @@ namespace dawn_native { mMapUserdata = userdata; mState = BufferState::Mapped; - if (GetDevice()->ConsumedError(MapReadAsyncImpl(mMapSerial))) { + if (GetDevice()->ConsumedError(MapReadAsyncImpl())) { CallMapReadCallback(mMapSerial, WGPUBufferMapAsyncStatus_DeviceLost, nullptr, 0); return; } @@ -297,7 +297,7 @@ namespace dawn_native { mMapUserdata = userdata; mState = BufferState::Mapped; - if (GetDevice()->ConsumedError(MapWriteAsyncImpl(mMapSerial))) { + if (GetDevice()->ConsumedError(MapWriteAsyncImpl())) { CallMapWriteCallback(mMapSerial, WGPUBufferMapAsyncStatus_DeviceLost, nullptr, 0); return; } diff --git a/src/dawn_native/Buffer.h b/src/dawn_native/Buffer.h index b24a47e1f0..f4a53777bc 100644 --- a/src/dawn_native/Buffer.h +++ b/src/dawn_native/Buffer.h @@ -77,8 +77,8 @@ namespace dawn_native { private: virtual MaybeError MapAtCreationImpl() = 0; - virtual MaybeError MapReadAsyncImpl(uint32_t serial) = 0; - virtual MaybeError MapWriteAsyncImpl(uint32_t serial) = 0; + virtual MaybeError MapReadAsyncImpl() = 0; + virtual MaybeError MapWriteAsyncImpl() = 0; virtual void UnmapImpl() = 0; virtual void DestroyImpl() = 0; virtual void* GetMappedPointerImpl() = 0; diff --git a/src/dawn_native/MapRequestTracker.h b/src/dawn_native/MapRequestTracker.h index 0dffca18f3..68c0c86df4 100644 --- a/src/dawn_native/MapRequestTracker.h +++ b/src/dawn_native/MapRequestTracker.h @@ -41,4 +41,4 @@ namespace dawn_native { } // namespace dawn_native -#endif // DAWNNATIVE_MAPREQUESTTRACKER_H \ No newline at end of file +#endif // DAWNNATIVE_MAPREQUESTTRACKER_H diff --git a/src/dawn_native/d3d12/BufferD3D12.cpp b/src/dawn_native/d3d12/BufferD3D12.cpp index 8e8a7d7d93..6ff9f0b550 100644 --- a/src/dawn_native/d3d12/BufferD3D12.cpp +++ b/src/dawn_native/d3d12/BufferD3D12.cpp @@ -269,11 +269,11 @@ namespace dawn_native { namespace d3d12 { return {}; } - MaybeError Buffer::MapReadAsyncImpl(uint32_t serial) { + MaybeError Buffer::MapReadAsyncImpl() { return MapInternal(false, "D3D12 map read async"); } - MaybeError Buffer::MapWriteAsyncImpl(uint32_t serial) { + MaybeError Buffer::MapWriteAsyncImpl() { return MapInternal(true, "D3D12 map write async"); } diff --git a/src/dawn_native/d3d12/BufferD3D12.h b/src/dawn_native/d3d12/BufferD3D12.h index 3422ce2dad..33e0580a0f 100644 --- a/src/dawn_native/d3d12/BufferD3D12.h +++ b/src/dawn_native/d3d12/BufferD3D12.h @@ -49,8 +49,8 @@ namespace dawn_native { namespace d3d12 { private: ~Buffer() override; // Dawn API - MaybeError MapReadAsyncImpl(uint32_t serial) override; - MaybeError MapWriteAsyncImpl(uint32_t serial) override; + MaybeError MapReadAsyncImpl() override; + MaybeError MapWriteAsyncImpl() override; void UnmapImpl() override; void DestroyImpl() override; diff --git a/src/dawn_native/metal/BufferMTL.h b/src/dawn_native/metal/BufferMTL.h index f76bcefb38..74d2cdb126 100644 --- a/src/dawn_native/metal/BufferMTL.h +++ b/src/dawn_native/metal/BufferMTL.h @@ -38,8 +38,8 @@ namespace dawn_native { namespace metal { MaybeError Initialize(); ~Buffer() override; // Dawn API - MaybeError MapReadAsyncImpl(uint32_t serial) override; - MaybeError MapWriteAsyncImpl(uint32_t serial) override; + MaybeError MapReadAsyncImpl() override; + MaybeError MapWriteAsyncImpl() override; void UnmapImpl() override; void DestroyImpl() override; void* GetMappedPointerImpl() override; diff --git a/src/dawn_native/metal/BufferMTL.mm b/src/dawn_native/metal/BufferMTL.mm index debc20faa5..c14a5a2a3e 100644 --- a/src/dawn_native/metal/BufferMTL.mm +++ b/src/dawn_native/metal/BufferMTL.mm @@ -113,11 +113,11 @@ namespace dawn_native { namespace metal { return {}; } - MaybeError Buffer::MapReadAsyncImpl(uint32_t serial) { + MaybeError Buffer::MapReadAsyncImpl() { return {}; } - MaybeError Buffer::MapWriteAsyncImpl(uint32_t serial) { + MaybeError Buffer::MapWriteAsyncImpl() { return {}; } diff --git a/src/dawn_native/null/DeviceNull.cpp b/src/dawn_native/null/DeviceNull.cpp index 484a386673..71085cd5b8 100644 --- a/src/dawn_native/null/DeviceNull.cpp +++ b/src/dawn_native/null/DeviceNull.cpp @@ -270,17 +270,6 @@ namespace dawn_native { namespace null { // Buffer - struct BufferMapOperation : PendingOperation { - virtual void Execute() { - buffer->OnMapCommandSerialFinished(serial, isWrite); - } - - Ref buffer; - void* ptr; - uint32_t serial; - bool isWrite; - }; - Buffer::Buffer(Device* device, const BufferDescriptor* descriptor) : BufferBase(device, descriptor) { mBackingData = std::unique_ptr(new uint8_t[GetSize()]); @@ -315,28 +304,14 @@ namespace dawn_native { namespace null { memcpy(mBackingData.get() + bufferOffset, data, size); } - MaybeError Buffer::MapReadAsyncImpl(uint32_t serial) { - MapAsyncImplCommon(serial, false); + MaybeError Buffer::MapReadAsyncImpl() { return {}; } - MaybeError Buffer::MapWriteAsyncImpl(uint32_t serial) { - MapAsyncImplCommon(serial, true); + MaybeError Buffer::MapWriteAsyncImpl() { return {}; } - void Buffer::MapAsyncImplCommon(uint32_t serial, bool isWrite) { - ASSERT(mBackingData); - - auto operation = std::make_unique(); - operation->buffer = this; - operation->ptr = mBackingData.get(); - operation->serial = serial; - operation->isWrite = isWrite; - - ToBackend(GetDevice())->AddPendingOperation(std::move(operation)); - } - void* Buffer::GetMappedPointerImpl() { return mBackingData.get(); } diff --git a/src/dawn_native/null/DeviceNull.h b/src/dawn_native/null/DeviceNull.h index 3bda15e308..1c98664426 100644 --- a/src/dawn_native/null/DeviceNull.h +++ b/src/dawn_native/null/DeviceNull.h @@ -199,14 +199,13 @@ namespace dawn_native { namespace null { ~Buffer() override; // Dawn API - MaybeError MapReadAsyncImpl(uint32_t serial) override; - MaybeError MapWriteAsyncImpl(uint32_t serial) override; + MaybeError MapReadAsyncImpl() override; + MaybeError MapWriteAsyncImpl() override; void UnmapImpl() override; void DestroyImpl() override; bool IsMapWritable() const override; MaybeError MapAtCreationImpl() override; - void MapAsyncImplCommon(uint32_t serial, bool isWrite); void* GetMappedPointerImpl() override; std::unique_ptr mBackingData; diff --git a/src/dawn_native/opengl/BufferGL.cpp b/src/dawn_native/opengl/BufferGL.cpp index 40a8d756a8..c56cc55945 100644 --- a/src/dawn_native/opengl/BufferGL.cpp +++ b/src/dawn_native/opengl/BufferGL.cpp @@ -79,7 +79,7 @@ namespace dawn_native { namespace opengl { return {}; } - MaybeError Buffer::MapReadAsyncImpl(uint32_t serial) { + MaybeError Buffer::MapReadAsyncImpl() { const OpenGLFunctions& gl = ToBackend(GetDevice())->gl; // TODO(cwallez@chromium.org): this does GPU->CPU synchronization, we could require a high @@ -89,7 +89,7 @@ namespace dawn_native { namespace opengl { return {}; } - MaybeError Buffer::MapWriteAsyncImpl(uint32_t serial) { + MaybeError Buffer::MapWriteAsyncImpl() { const OpenGLFunctions& gl = ToBackend(GetDevice())->gl; // TODO(cwallez@chromium.org): this does GPU->CPU synchronization, we could require a high diff --git a/src/dawn_native/opengl/BufferGL.h b/src/dawn_native/opengl/BufferGL.h index ee6f12ee32..121b1d6a39 100644 --- a/src/dawn_native/opengl/BufferGL.h +++ b/src/dawn_native/opengl/BufferGL.h @@ -34,8 +34,8 @@ namespace dawn_native { namespace opengl { private: ~Buffer() override; // Dawn API - MaybeError MapReadAsyncImpl(uint32_t serial) override; - MaybeError MapWriteAsyncImpl(uint32_t serial) override; + MaybeError MapReadAsyncImpl() override; + MaybeError MapWriteAsyncImpl() override; void UnmapImpl() override; void DestroyImpl() override; diff --git a/src/dawn_native/vulkan/BufferVk.cpp b/src/dawn_native/vulkan/BufferVk.cpp index 004d196be6..11f56f5133 100644 --- a/src/dawn_native/vulkan/BufferVk.cpp +++ b/src/dawn_native/vulkan/BufferVk.cpp @@ -243,7 +243,7 @@ namespace dawn_native { namespace vulkan { return {}; } - MaybeError Buffer::MapReadAsyncImpl(uint32_t serial) { + MaybeError Buffer::MapReadAsyncImpl() { Device* device = ToBackend(GetDevice()); CommandRecordingContext* recordingContext = device->GetPendingRecordingContext(); @@ -251,7 +251,7 @@ namespace dawn_native { namespace vulkan { return {}; } - MaybeError Buffer::MapWriteAsyncImpl(uint32_t serial) { + MaybeError Buffer::MapWriteAsyncImpl() { Device* device = ToBackend(GetDevice()); CommandRecordingContext* recordingContext = device->GetPendingRecordingContext(); diff --git a/src/dawn_native/vulkan/BufferVk.h b/src/dawn_native/vulkan/BufferVk.h index 536c12aaed..6fb8592730 100644 --- a/src/dawn_native/vulkan/BufferVk.h +++ b/src/dawn_native/vulkan/BufferVk.h @@ -52,8 +52,8 @@ namespace dawn_native { namespace vulkan { void ClearBuffer(CommandRecordingContext* recordingContext, uint32_t clearValue); // Dawn API - MaybeError MapReadAsyncImpl(uint32_t serial) override; - MaybeError MapWriteAsyncImpl(uint32_t serial) override; + MaybeError MapReadAsyncImpl() override; + MaybeError MapWriteAsyncImpl() override; void UnmapImpl() override; void DestroyImpl() override; diff --git a/src/tests/unittests/validation/BufferValidationTests.cpp b/src/tests/unittests/validation/BufferValidationTests.cpp index 82d5fbadfd..f807b52322 100644 --- a/src/tests/unittests/validation/BufferValidationTests.cpp +++ b/src/tests/unittests/validation/BufferValidationTests.cpp @@ -172,7 +172,7 @@ TEST_F(BufferValidationTest, MapReadSuccess) { EXPECT_CALL(*mockBufferMapReadCallback, Call(WGPUBufferMapAsyncStatus_Success, Ne(nullptr), 4u, _)) .Times(1); - queue.Submit(0, nullptr); + WaitForAllOperations(device); buf.Unmap(); } @@ -186,7 +186,7 @@ TEST_F(BufferValidationTest, MapWriteSuccess) { EXPECT_CALL(*mockBufferMapWriteCallback, Call(WGPUBufferMapAsyncStatus_Success, Ne(nullptr), 4u, _)) .Times(1); - queue.Submit(0, nullptr); + WaitForAllOperations(device); buf.Unmap(); } @@ -264,7 +264,7 @@ TEST_F(BufferValidationTest, MapReadAlreadyMapped) { .Times(1); ASSERT_DEVICE_ERROR(buf.MapReadAsync(ToMockBufferMapReadCallback, this + 1)); - queue.Submit(0, nullptr); + WaitForAllOperations(device); } // Test map writing a buffer that is already mapped @@ -281,7 +281,7 @@ TEST_F(BufferValidationTest, MapWriteAlreadyMapped) { .Times(1); ASSERT_DEVICE_ERROR(buf.MapWriteAsync(ToMockBufferMapWriteCallback, this + 1)); - queue.Submit(0, nullptr); + WaitForAllOperations(device); } // TODO(cwallez@chromium.org) Test a MapWrite and already MapRead and vice-versa @@ -295,9 +295,8 @@ TEST_F(BufferValidationTest, MapReadUnmapBeforeResult) { .Times(1); buf.Unmap(); - // Submitting the queue makes the null backend process map request, but the callback shouldn't - // be called again - queue.Submit(0, nullptr); + // The callback shouldn't be called again. + WaitForAllOperations(device); } // Test unmapping before having the result gives UNKNOWN - for writing @@ -310,9 +309,8 @@ TEST_F(BufferValidationTest, MapWriteUnmapBeforeResult) { .Times(1); buf.Unmap(); - // Submitting the queue makes the null backend process map request, but the callback shouldn't - // be called again - queue.Submit(0, nullptr); + // The callback shouldn't be called again. + WaitForAllOperations(device); } // Test destroying the buffer before having the result gives UNKNOWN - for reading @@ -329,9 +327,8 @@ TEST_F(BufferValidationTest, DISABLED_MapReadDestroyBeforeResult) { .Times(1); } - // Submitting the queue makes the null backend process map request, but the callback shouldn't - // be called again - queue.Submit(0, nullptr); + // The callback shouldn't be called again. + WaitForAllOperations(device); } // Test destroying the buffer before having the result gives UNKNOWN - for writing @@ -348,9 +345,8 @@ TEST_F(BufferValidationTest, DISABLED_MapWriteDestroyBeforeResult) { .Times(1); } - // Submitting the queue makes the null backend process map request, but the callback shouldn't - // be called again - queue.Submit(0, nullptr); + // The callback shouldn't be called again. + WaitForAllOperations(device); } // When a MapRead is cancelled with Unmap it might still be in flight, test doing a new request @@ -370,7 +366,7 @@ TEST_F(BufferValidationTest, MapReadUnmapBeforeResultThenMapAgain) { EXPECT_CALL(*mockBufferMapReadCallback, Call(WGPUBufferMapAsyncStatus_Success, Ne(nullptr), 4u, this + 1)) .Times(1); - queue.Submit(0, nullptr); + WaitForAllOperations(device); } // TODO(cwallez@chromium.org) Test a MapWrite and already MapRead and vice-versa @@ -391,7 +387,7 @@ TEST_F(BufferValidationTest, MapWriteUnmapBeforeResultThenMapAgain) { EXPECT_CALL(*mockBufferMapWriteCallback, Call(WGPUBufferMapAsyncStatus_Success, Ne(nullptr), 4u, this + 1)) .Times(1); - queue.Submit(0, nullptr); + WaitForAllOperations(device); } // Test that the MapReadCallback isn't fired twice when unmap() is called inside the callback @@ -404,7 +400,7 @@ TEST_F(BufferValidationTest, UnmapInsideMapReadCallback) { Call(WGPUBufferMapAsyncStatus_Success, Ne(nullptr), 4u, _)) .WillOnce(InvokeWithoutArgs([&]() { buf.Unmap(); })); - queue.Submit(0, nullptr); + WaitForAllOperations(device); } // Test that the MapWriteCallback isn't fired twice when unmap() is called inside the callback @@ -417,7 +413,7 @@ TEST_F(BufferValidationTest, UnmapInsideMapWriteCallback) { Call(WGPUBufferMapAsyncStatus_Success, Ne(nullptr), 4u, _)) .WillOnce(InvokeWithoutArgs([&]() { buf.Unmap(); })); - queue.Submit(0, nullptr); + WaitForAllOperations(device); } // Test that the MapReadCallback isn't fired twice the buffer external refcount reaches 0 in the callback @@ -430,7 +426,7 @@ TEST_F(BufferValidationTest, DestroyInsideMapReadCallback) { Call(WGPUBufferMapAsyncStatus_Success, Ne(nullptr), 4u, _)) .WillOnce(InvokeWithoutArgs([&]() { buf = wgpu::Buffer(); })); - queue.Submit(0, nullptr); + WaitForAllOperations(device); } // Test that the MapWriteCallback isn't fired twice the buffer external refcount reaches 0 in the callback @@ -443,7 +439,7 @@ TEST_F(BufferValidationTest, DestroyInsideMapWriteCallback) { Call(WGPUBufferMapAsyncStatus_Success, Ne(nullptr), 4u, _)) .WillOnce(InvokeWithoutArgs([&]() { buf = wgpu::Buffer(); })); - queue.Submit(0, nullptr); + WaitForAllOperations(device); } // Test that it is valid to destroy an unmapped buffer @@ -482,7 +478,7 @@ TEST_F(BufferValidationTest, DestroyMappedBufferCausesImplicitUnmap) { Call(WGPUBufferMapAsyncStatus_Unknown, nullptr, 0, this + 0)) .Times(1); buf.Destroy(); - queue.Submit(0, nullptr); + WaitForAllOperations(device); } { wgpu::Buffer buf = CreateMapWriteBuffer(4); @@ -492,7 +488,7 @@ TEST_F(BufferValidationTest, DestroyMappedBufferCausesImplicitUnmap) { Call(WGPUBufferMapAsyncStatus_Unknown, nullptr, 0, this + 1)) .Times(1); buf.Destroy(); - queue.Submit(0, nullptr); + WaitForAllOperations(device); } } @@ -537,13 +533,13 @@ TEST_F(BufferValidationTest, MapMappedBuffer) { wgpu::Buffer buf = CreateMapReadBuffer(4); buf.MapReadAsync(ToMockBufferMapReadCallback, nullptr); ASSERT_DEVICE_ERROR(buf.MapReadAsync(ToMockBufferMapReadCallback, nullptr)); - queue.Submit(0, nullptr); + WaitForAllOperations(device); } { wgpu::Buffer buf = CreateMapWriteBuffer(4); buf.MapWriteAsync(ToMockBufferMapWriteCallback, nullptr); ASSERT_DEVICE_ERROR(buf.MapWriteAsync(ToMockBufferMapWriteCallback, nullptr)); - queue.Submit(0, nullptr); + WaitForAllOperations(device); } } @@ -552,12 +548,12 @@ TEST_F(BufferValidationTest, MapCreateBufferMappedBuffer) { { wgpu::Buffer buf = CreateBufferMapped(4, wgpu::BufferUsage::MapRead).buffer; ASSERT_DEVICE_ERROR(buf.MapReadAsync(ToMockBufferMapReadCallback, nullptr)); - queue.Submit(0, nullptr); + WaitForAllOperations(device); } { wgpu::Buffer buf = CreateBufferMapped(4, wgpu::BufferUsage::MapWrite).buffer; ASSERT_DEVICE_ERROR(buf.MapWriteAsync(ToMockBufferMapWriteCallback, nullptr)); - queue.Submit(0, nullptr); + WaitForAllOperations(device); } } @@ -566,12 +562,12 @@ TEST_F(BufferValidationTest, MapBufferMappedAtCreation) { { wgpu::Buffer buf = BufferMappedAtCreation(4, wgpu::BufferUsage::MapRead); ASSERT_DEVICE_ERROR(buf.MapReadAsync(ToMockBufferMapReadCallback, nullptr)); - queue.Submit(0, nullptr); + WaitForAllOperations(device); } { wgpu::Buffer buf = BufferMappedAtCreation(4, wgpu::BufferUsage::MapWrite); ASSERT_DEVICE_ERROR(buf.MapWriteAsync(ToMockBufferMapWriteCallback, nullptr)); - queue.Submit(0, nullptr); + WaitForAllOperations(device); } } @@ -613,7 +609,7 @@ TEST_F(BufferValidationTest, SubmitMappedBuffer) { encoder.CopyBufferToBuffer(bufA, 0, bufB, 0, 4); wgpu::CommandBuffer commands = encoder.Finish(); ASSERT_DEVICE_ERROR(queue.Submit(1, &commands)); - queue.Submit(0, nullptr); + WaitForAllOperations(device); } { wgpu::Buffer bufA = device.CreateBuffer(&descriptorA); @@ -625,7 +621,7 @@ TEST_F(BufferValidationTest, SubmitMappedBuffer) { encoder.CopyBufferToBuffer(bufA, 0, bufB, 0, 4); wgpu::CommandBuffer commands = encoder.Finish(); ASSERT_DEVICE_ERROR(queue.Submit(1, &commands)); - queue.Submit(0, nullptr); + WaitForAllOperations(device); } { wgpu::Buffer bufA = device.CreateBufferMapped(&descriptorA).buffer; @@ -635,7 +631,7 @@ TEST_F(BufferValidationTest, SubmitMappedBuffer) { encoder.CopyBufferToBuffer(bufA, 0, bufB, 0, 4); wgpu::CommandBuffer commands = encoder.Finish(); ASSERT_DEVICE_ERROR(queue.Submit(1, &commands)); - queue.Submit(0, nullptr); + WaitForAllOperations(device); } { wgpu::Buffer bufA = device.CreateBuffer(&descriptorA); @@ -645,7 +641,7 @@ TEST_F(BufferValidationTest, SubmitMappedBuffer) { encoder.CopyBufferToBuffer(bufA, 0, bufB, 0, 4); wgpu::CommandBuffer commands = encoder.Finish(); ASSERT_DEVICE_ERROR(queue.Submit(1, &commands)); - queue.Submit(0, nullptr); + WaitForAllOperations(device); } { wgpu::BufferDescriptor mappedBufferDesc = descriptorA; @@ -657,7 +653,7 @@ TEST_F(BufferValidationTest, SubmitMappedBuffer) { encoder.CopyBufferToBuffer(bufA, 0, bufB, 0, 4); wgpu::CommandBuffer commands = encoder.Finish(); ASSERT_DEVICE_ERROR(queue.Submit(1, &commands)); - queue.Submit(0, nullptr); + WaitForAllOperations(device); } { wgpu::BufferDescriptor mappedBufferDesc = descriptorB; @@ -669,7 +665,7 @@ TEST_F(BufferValidationTest, SubmitMappedBuffer) { encoder.CopyBufferToBuffer(bufA, 0, bufB, 0, 4); wgpu::CommandBuffer commands = encoder.Finish(); ASSERT_DEVICE_ERROR(queue.Submit(1, &commands)); - queue.Submit(0, nullptr); + WaitForAllOperations(device); } } @@ -764,7 +760,7 @@ TEST_F(BufferValidationTest, GetMappedRangeOnUnmappedBuffer) { EXPECT_CALL(*mockBufferMapReadCallback, Call(WGPUBufferMapAsyncStatus_Success, Ne(nullptr), 4u, _)) .Times(1); - queue.Submit(0, nullptr); + WaitForAllOperations(device); buf.Unmap(); ASSERT_EQ(nullptr, buf.GetMappedRange()); @@ -778,7 +774,7 @@ TEST_F(BufferValidationTest, GetMappedRangeOnUnmappedBuffer) { EXPECT_CALL(*mockBufferMapWriteCallback, Call(WGPUBufferMapAsyncStatus_Success, Ne(nullptr), 4u, _)) .Times(1); - queue.Submit(0, nullptr); + WaitForAllOperations(device); buf.Unmap(); ASSERT_EQ(nullptr, buf.GetMappedRange()); @@ -826,7 +822,7 @@ TEST_F(BufferValidationTest, GetMappedRangeOnDestroyedBuffer) { EXPECT_CALL(*mockBufferMapReadCallback, Call(WGPUBufferMapAsyncStatus_Success, Ne(nullptr), 4u, _)) .Times(1); - queue.Submit(0, nullptr); + WaitForAllOperations(device); buf.Destroy(); ASSERT_EQ(nullptr, buf.GetMappedRange()); @@ -840,7 +836,7 @@ TEST_F(BufferValidationTest, GetMappedRangeOnDestroyedBuffer) { EXPECT_CALL(*mockBufferMapWriteCallback, Call(WGPUBufferMapAsyncStatus_Success, Ne(nullptr), 4u, _)) .Times(1); - queue.Submit(0, nullptr); + WaitForAllOperations(device); buf.Destroy(); ASSERT_EQ(nullptr, buf.GetMappedRange()); @@ -856,7 +852,7 @@ TEST_F(BufferValidationTest, GetMappedRangeOnMappedForReading) { EXPECT_CALL(*mockBufferMapReadCallback, Call(WGPUBufferMapAsyncStatus_Success, Ne(nullptr), 4u, _)) .Times(1); - queue.Submit(0, nullptr); + WaitForAllOperations(device); ASSERT_EQ(nullptr, buf.GetMappedRange()); } @@ -889,7 +885,7 @@ TEST_F(BufferValidationTest, GetMappedRangeValidCases) { Call(WGPUBufferMapAsyncStatus_Success, Ne(nullptr), 4u, _)) .WillOnce(SaveArg<1>(&mappedPointer)); - queue.Submit(0, nullptr); + WaitForAllOperations(device); ASSERT_NE(buf.GetConstMappedRange(), nullptr); ASSERT_EQ(buf.GetConstMappedRange(), mappedPointer); @@ -905,7 +901,7 @@ TEST_F(BufferValidationTest, GetMappedRangeValidCases) { Call(WGPUBufferMapAsyncStatus_Success, Ne(nullptr), 4u, _)) .WillOnce(SaveArg<1>(&mappedPointer)); - queue.Submit(0, nullptr); + WaitForAllOperations(device); ASSERT_NE(buf.GetConstMappedRange(), nullptr); ASSERT_EQ(buf.GetConstMappedRange(), buf.GetMappedRange()); diff --git a/src/tests/unittests/validation/QueueSubmitValidationTests.cpp b/src/tests/unittests/validation/QueueSubmitValidationTests.cpp index f94bc859d6..566c7469bd 100644 --- a/src/tests/unittests/validation/QueueSubmitValidationTests.cpp +++ b/src/tests/unittests/validation/QueueSubmitValidationTests.cpp @@ -21,13 +21,6 @@ namespace { class QueueSubmitValidationTest : public ValidationTest { }; -static void StoreTrueMapWriteCallback(WGPUBufferMapAsyncStatus status, - void*, - uint64_t, - void* userdata) { - *static_cast(userdata) = true; -} - // Test submitting with a mapped buffer is disallowed TEST_F(QueueSubmitValidationTest, SubmitWithMappedBuffer) { // Create a map-write buffer. @@ -54,11 +47,14 @@ TEST_F(QueueSubmitValidationTest, SubmitWithMappedBuffer) { queue.Submit(1, &commands); // Map the buffer, submitting when the buffer is mapped should fail - bool mapWriteFinished = false; - buffer.MapWriteAsync(StoreTrueMapWriteCallback, &mapWriteFinished); - queue.Submit(0, nullptr); - ASSERT_TRUE(mapWriteFinished); + buffer.MapWriteAsync(nullptr, nullptr); + // Try submitting before the callback is fired. + ASSERT_DEVICE_ERROR(queue.Submit(1, &commands)); + + WaitForAllOperations(device); + + // Try submitting after the callback is fired. ASSERT_DEVICE_ERROR(queue.Submit(1, &commands)); // Unmap the buffer, queue submit should succeed @@ -190,10 +186,10 @@ TEST_F(QueueWriteBufferValidationTest, MappedBuffer) { { wgpu::BufferDescriptor descriptor; descriptor.size = 4; - descriptor.usage = wgpu::BufferUsage::CopyDst | wgpu::BufferUsage::MapRead; + descriptor.usage = wgpu::BufferUsage::CopySrc | wgpu::BufferUsage::MapWrite; wgpu::Buffer buf = device.CreateBuffer(&descriptor); - buf.MapReadAsync(nullptr, nullptr); + buf.MapWriteAsync(nullptr, nullptr); uint32_t value = 0; ASSERT_DEVICE_ERROR(queue.WriteBuffer(buf, 0, &value, sizeof(value))); } diff --git a/src/tests/unittests/validation/ValidationTest.cpp b/src/tests/unittests/validation/ValidationTest.cpp index 7db863d36d..21f028ad94 100644 --- a/src/tests/unittests/validation/ValidationTest.cpp +++ b/src/tests/unittests/validation/ValidationTest.cpp @@ -87,6 +87,21 @@ std::string ValidationTest::GetLastDeviceErrorMessage() const { return mDeviceErrorMessage; } +void ValidationTest::WaitForAllOperations(const wgpu::Device& device) const { + wgpu::Queue queue = device.GetDefaultQueue(); + wgpu::Fence fence = queue.CreateFence(); + + // Force the currently submitted operations to completed. + queue.Signal(fence, 1); + while (fence.GetCompletedValue() < 1) { + device.Tick(); + } + + // TODO(cwallez@chromium.org): It's not clear why we need this additional tick. Investigate it + // once WebGPU has defined the ordering of callbacks firing. + device.Tick(); +} + // static void ValidationTest::OnDeviceError(WGPUErrorType type, const char* message, void* userdata) { ASSERT(type != WGPUErrorType_NoError); diff --git a/src/tests/unittests/validation/ValidationTest.h b/src/tests/unittests/validation/ValidationTest.h index 0b165fef09..7862b4c96d 100644 --- a/src/tests/unittests/validation/ValidationTest.h +++ b/src/tests/unittests/validation/ValidationTest.h @@ -42,6 +42,8 @@ class ValidationTest : public testing::Test { bool EndExpectDeviceError(); std::string GetLastDeviceErrorMessage() const; + void WaitForAllOperations(const wgpu::Device& device) const; + // Helper functions to create objects to test validation. struct DummyRenderPass : public wgpu::RenderPassDescriptor {