From d743778ed5ef4afbdb9642129e3f710a8bfc9d69 Mon Sep 17 00:00:00 2001 From: Takahiro Date: Wed, 23 Nov 2022 18:19:52 +0000 Subject: [PATCH] Also reject mapAsync if the buffer is being mapped To reflect the WebGPU spec change at https://github.com/gpuweb/gpuweb/pull/3348 Bug: chromium:1355994 Change-Id: I0159cbd9e1977a05453e3c562a2b0649a0ff930f Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/110448 Reviewed-by: Austin Eng Commit-Queue: Takahiro Reviewed-by: Corentin Wallez Kokoro: Kokoro --- src/dawn/native/Buffer.cpp | 51 ++++++++++--------- src/dawn/native/Buffer.h | 6 +-- src/dawn/tests/end2end/DeviceLostTests.cpp | 22 +++++++- .../validation/BufferValidationTests.cpp | 49 ++++++++++++++++++ 4 files changed, 99 insertions(+), 29 deletions(-) diff --git a/src/dawn/native/Buffer.cpp b/src/dawn/native/Buffer.cpp index 925682987f..d2bdb62ef4 100644 --- a/src/dawn/native/Buffer.cpp +++ b/src/dawn/native/Buffer.cpp @@ -37,25 +37,24 @@ namespace dawn::native { namespace { struct MapRequestTask : TrackTaskCallback { - MapRequestTask(dawn::platform::Platform* platform, Ref buffer, MapRequestID id) - : TrackTaskCallback(platform), buffer(std::move(buffer)), id(id) {} + MapRequestTask(dawn::platform::Platform* platform, Ref buffer) + : TrackTaskCallback(platform), buffer(std::move(buffer)) {} void Finish() override { ASSERT(mSerial != kMaxExecutionSerial); TRACE_EVENT1(mPlatform, General, "Buffer::TaskInFlight::Finished", "serial", uint64_t(mSerial)); - buffer->OnMapRequestCompleted(id, WGPUBufferMapAsyncStatus_Success); + buffer->OnMapRequestCompleted(WGPUBufferMapAsyncStatus_Success); } void HandleDeviceLoss() override { - buffer->OnMapRequestCompleted(id, WGPUBufferMapAsyncStatus_DeviceLost); + buffer->OnMapRequestCompleted(WGPUBufferMapAsyncStatus_DeviceLost); } void HandleShutDown() override { - buffer->OnMapRequestCompleted(id, WGPUBufferMapAsyncStatus_DestroyedBeforeCallback); + buffer->OnMapRequestCompleted(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback); } ~MapRequestTask() override = default; private: Ref buffer; - MapRequestID id; }; class ErrorBuffer final : public BufferBase { @@ -194,7 +193,7 @@ BufferBase::~BufferBase() { } void BufferBase::DestroyImpl() { - if (mState == BufferState::Mapped) { + if (mState == BufferState::Mapped || mState == BufferState::PendingMap) { UnmapInternal(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback); } else if (mState == BufferState::MappedAtCreation) { if (mStagingBuffer != nullptr) { @@ -311,15 +310,17 @@ MaybeError BufferBase::ValidateCanUseOnQueueNow() const { case BufferState::Mapped: case BufferState::MappedAtCreation: return DAWN_VALIDATION_ERROR("%s used in submit while mapped.", this); + case BufferState::PendingMap: + return DAWN_VALIDATION_ERROR("%s used in submit while pending map.", this); case BufferState::Unmapped: return {}; } UNREACHABLE(); } -void BufferBase::CallMapCallback(MapRequestID mapID, WGPUBufferMapAsyncStatus status) { +void BufferBase::CallMapCallback(WGPUBufferMapAsyncStatus status) { ASSERT(!IsError()); - if (mMapCallback != nullptr && mapID == mLastMapID) { + if (mMapCallback != nullptr) { // Tag the callback as fired before firing it, otherwise it could fire a second time if // for example buffer.Unmap() is called inside the application-provided callback. WGPUBufferMapCallback callback = mMapCallback; @@ -330,6 +331,8 @@ void BufferBase::CallMapCallback(MapRequestID mapID, WGPUBufferMapAsyncStatus st } else { callback(status, mMapUserdata); } + + mMapUserdata = 0; } } @@ -356,20 +359,19 @@ void BufferBase::APIMapAsync(wgpu::MapMode mode, } ASSERT(!IsError()); - mLastMapID++; mMapMode = mode; mMapOffset = offset; mMapSize = size; mMapCallback = callback; mMapUserdata = userdata; - mState = BufferState::Mapped; + mState = BufferState::PendingMap; if (GetDevice()->ConsumedError(MapAsyncImpl(mode, offset, size))) { - CallMapCallback(mLastMapID, WGPUBufferMapAsyncStatus_DeviceLost); + CallMapCallback(WGPUBufferMapAsyncStatus_DeviceLost); return; } std::unique_ptr request = - std::make_unique(GetDevice()->GetPlatform(), this, mLastMapID); + std::make_unique(GetDevice()->GetPlatform(), this); TRACE_EVENT1(GetDevice()->GetPlatform(), General, "Buffer::APIMapAsync", "serial", uint64_t(GetDevice()->GetPendingCommandSerial())); GetDevice()->GetQueue()->TrackTask(std::move(request)); @@ -438,16 +440,11 @@ void BufferBase::Unmap() { } void BufferBase::UnmapInternal(WGPUBufferMapAsyncStatus callbackStatus) { - if (mState == BufferState::Mapped) { - // A map request can only be called once, so this will fire only if the request wasn't - // completed before the Unmap. - // Callbacks are not fired if there is no callback registered, so this is correct for - // mappedAtCreation = true. - CallMapCallback(mLastMapID, callbackStatus); + if (mState == BufferState::PendingMap) { + CallMapCallback(callbackStatus); + UnmapImpl(); + } else if (mState == BufferState::Mapped) { UnmapImpl(); - - mMapCallback = nullptr; - mMapUserdata = 0; } else if (mState == BufferState::MappedAtCreation) { if (mStagingBuffer != nullptr) { GetDevice()->ConsumedError(CopyFromStagingBuffer()); @@ -483,6 +480,8 @@ MaybeError BufferBase::ValidateMapAsync(wgpu::MapMode mode, case BufferState::Mapped: case BufferState::MappedAtCreation: return DAWN_VALIDATION_ERROR("%s is already mapped.", this); + case BufferState::PendingMap: + return DAWN_VALIDATION_ERROR("%s is pending map.", this); case BufferState::Destroyed: return DAWN_VALIDATION_ERROR("%s is destroyed.", this); case BufferState::Unmapped: @@ -542,6 +541,7 @@ bool BufferBase::CanGetMappedRange(bool writable, size_t offset, size_t size) co ASSERT(bool{mMapMode & wgpu::MapMode::Read} ^ bool{mMapMode & wgpu::MapMode::Write}); return !writable || (mMapMode & wgpu::MapMode::Write); + case BufferState::PendingMap: case BufferState::Unmapped: case BufferState::Destroyed: return false; @@ -554,8 +554,11 @@ MaybeError BufferBase::ValidateUnmap() const { return {}; } -void BufferBase::OnMapRequestCompleted(MapRequestID mapID, WGPUBufferMapAsyncStatus status) { - CallMapCallback(mapID, status); +void BufferBase::OnMapRequestCompleted(WGPUBufferMapAsyncStatus status) { + if (status == WGPUBufferMapAsyncStatus_Success && mState == BufferState::PendingMap) { + mState = BufferState::Mapped; + } + CallMapCallback(status); } bool BufferBase::NeedsInitialization() const { diff --git a/src/dawn/native/Buffer.h b/src/dawn/native/Buffer.h index 6bb1503340..76d7f4cf65 100644 --- a/src/dawn/native/Buffer.h +++ b/src/dawn/native/Buffer.h @@ -44,6 +44,7 @@ class BufferBase : public ApiObjectBase { public: enum class BufferState { Unmapped, + PendingMap, Mapped, MappedAtCreation, Destroyed, @@ -62,7 +63,7 @@ class BufferBase : public ApiObjectBase { wgpu::BufferUsage GetUsageExternalOnly() const; MaybeError MapAtCreation(); - void OnMapRequestCompleted(MapRequestID mapID, WGPUBufferMapAsyncStatus status); + void OnMapRequestCompleted(WGPUBufferMapAsyncStatus status); MaybeError ValidateCanUseOnQueueNow() const; @@ -109,7 +110,7 @@ class BufferBase : public ApiObjectBase { virtual bool IsCPUWritableAtCreation() const = 0; MaybeError CopyFromStagingBuffer(); - void CallMapCallback(MapRequestID mapID, WGPUBufferMapAsyncStatus status); + void CallMapCallback(WGPUBufferMapAsyncStatus status); MaybeError ValidateMapAsync(wgpu::MapMode mode, size_t offset, @@ -128,7 +129,6 @@ class BufferBase : public ApiObjectBase { WGPUBufferMapCallback mMapCallback = nullptr; void* mMapUserdata = 0; - MapRequestID mLastMapID = MapRequestID(0); wgpu::MapMode mMapMode = wgpu::MapMode::None; size_t mMapOffset = 0; size_t mMapSize = 0; diff --git a/src/dawn/tests/end2end/DeviceLostTests.cpp b/src/dawn/tests/end2end/DeviceLostTests.cpp index 4c2b01ed85..a225c4dc03 100644 --- a/src/dawn/tests/end2end/DeviceLostTests.cpp +++ b/src/dawn/tests/end2end/DeviceLostTests.cpp @@ -55,6 +55,24 @@ class DeviceLostTest : public DawnTest { EXPECT_EQ(WGPUBufferMapAsyncStatus_DeviceLost, status); EXPECT_EQ(&fakeUserData, userdata); } + + void MapAsyncAndWait(const wgpu::Buffer& buffer, + wgpu::MapMode mode, + size_t offset, + size_t size) { + bool done = false; + buffer.MapAsync( + mode, offset, size, + [](WGPUBufferMapAsyncStatus status, void* userdata) { + ASSERT_EQ(WGPUBufferMapAsyncStatus_Success, status); + *static_cast(userdata) = true; + }, + &done); + + while (!done) { + WaitABit(); + } + } }; // Test that DeviceLostCallback is invoked when LostForTestimg is called @@ -338,7 +356,7 @@ TEST_P(DeviceLostTest, GetMappedRange_MapAsyncReading) { desc.usage = wgpu::BufferUsage::MapRead | wgpu::BufferUsage::CopyDst; wgpu::Buffer buffer = device.CreateBuffer(&desc); - buffer.MapAsync(wgpu::MapMode::Read, 0, 4, nullptr, nullptr); + MapAsyncAndWait(buffer, wgpu::MapMode::Read, 0, 4); queue.Submit(0, nullptr); const void* rangeBeforeLoss = buffer.GetConstMappedRange(); @@ -355,7 +373,7 @@ TEST_P(DeviceLostTest, GetMappedRange_MapAsyncWriting) { desc.usage = wgpu::BufferUsage::MapWrite | wgpu::BufferUsage::CopySrc; wgpu::Buffer buffer = device.CreateBuffer(&desc); - buffer.MapAsync(wgpu::MapMode::Write, 0, 4, nullptr, nullptr); + MapAsyncAndWait(buffer, wgpu::MapMode::Write, 0, 4); queue.Submit(0, nullptr); const void* rangeBeforeLoss = buffer.GetConstMappedRange(); diff --git a/src/dawn/tests/unittests/validation/BufferValidationTests.cpp b/src/dawn/tests/unittests/validation/BufferValidationTests.cpp index 3a0b4e5283..57b1dcd689 100644 --- a/src/dawn/tests/unittests/validation/BufferValidationTests.cpp +++ b/src/dawn/tests/unittests/validation/BufferValidationTests.cpp @@ -294,6 +294,7 @@ TEST_F(BufferValidationTest, MapAsync_AlreadyMapped) { { wgpu::Buffer buffer = CreateMapReadBuffer(4); buffer.MapAsync(wgpu::MapMode::Read, 0, 4, nullptr, nullptr); + WaitForAllOperations(device); AssertMapAsyncError(buffer, wgpu::MapMode::Read, 0, 4); } { @@ -303,6 +304,7 @@ TEST_F(BufferValidationTest, MapAsync_AlreadyMapped) { { wgpu::Buffer buffer = CreateMapWriteBuffer(4); buffer.MapAsync(wgpu::MapMode::Write, 0, 4, nullptr, nullptr); + WaitForAllOperations(device); AssertMapAsyncError(buffer, wgpu::MapMode::Write, 0, 4); } { @@ -311,6 +313,53 @@ TEST_F(BufferValidationTest, MapAsync_AlreadyMapped) { } } +// Test map async with a buffer that's pending map +TEST_F(BufferValidationTest, MapAsync_PendingMap) { + // Read + overlapping range + { + wgpu::Buffer buffer = CreateMapReadBuffer(4); + // The first map async call should succeed while the second one should fail + buffer.MapAsync(wgpu::MapMode::Read, 0, 4, ToMockBufferMapAsyncCallback, nullptr); + AssertMapAsyncError(buffer, wgpu::MapMode::Read, 0, 4); + EXPECT_CALL(*mockBufferMapAsyncCallback, Call(WGPUBufferMapAsyncStatus_Success, nullptr)) + .Times(1); + WaitForAllOperations(device); + } + + // Read + non-overlapping range + { + wgpu::Buffer buffer = CreateMapReadBuffer(16); + // The first map async call should succeed while the second one should fail + buffer.MapAsync(wgpu::MapMode::Read, 0, 8, ToMockBufferMapAsyncCallback, nullptr); + AssertMapAsyncError(buffer, wgpu::MapMode::Read, 8, 8); + EXPECT_CALL(*mockBufferMapAsyncCallback, Call(WGPUBufferMapAsyncStatus_Success, nullptr)) + .Times(1); + WaitForAllOperations(device); + } + + // Write + overlapping range + { + wgpu::Buffer buffer = CreateMapWriteBuffer(4); + // The first map async call should succeed while the second one should fail + buffer.MapAsync(wgpu::MapMode::Write, 0, 4, ToMockBufferMapAsyncCallback, nullptr); + AssertMapAsyncError(buffer, wgpu::MapMode::Write, 0, 4); + EXPECT_CALL(*mockBufferMapAsyncCallback, Call(WGPUBufferMapAsyncStatus_Success, nullptr)) + .Times(1); + WaitForAllOperations(device); + } + + // Write + non-overlapping range + { + wgpu::Buffer buffer = CreateMapWriteBuffer(16); + // The first map async call should succeed while the second one should fail + buffer.MapAsync(wgpu::MapMode::Write, 0, 8, ToMockBufferMapAsyncCallback, nullptr); + AssertMapAsyncError(buffer, wgpu::MapMode::Write, 8, 8); + EXPECT_CALL(*mockBufferMapAsyncCallback, Call(WGPUBufferMapAsyncStatus_Success, nullptr)) + .Times(1); + WaitForAllOperations(device); + } +} + // Test map async with a buffer that's destroyed TEST_F(BufferValidationTest, MapAsync_Destroy) { {