From e536775b409a322230e22c67c536e46243f6c40c Mon Sep 17 00:00:00 2001 From: Takahiro Date: Thu, 8 Dec 2022 04:49:32 +0000 Subject: [PATCH] Distinguish Map request callback fired by device Currently Buffer doesn't distinguish Map request callback fired by device. For example if buffer.MapAsync(), buffer.Unmap(), and buffer.MapAsync() are called in this order before the first MapAsync() finishes the MapAsync callback provided by application for the first MapAsync() is fired when Map request callback for the first MapAsync() is fired by device although the first MapAsync callback provided by application shouldn't be fired because it is already unmapped. This commit resolves this problem by assigning MapRequestId to Map request and distinguishing the callback fired by device. Change-Id: Ic29b02d27cffb254616dc7b48a60151c39f667e2 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/113222 Reviewed-by: Corentin Wallez Reviewed-by: Kai Ninomiya Kokoro: Kai Ninomiya Commit-Queue: Kai Ninomiya --- src/dawn/native/Buffer.cpp | 29 ++++++++++--------- src/dawn/native/Buffer.h | 5 ++-- .../validation/BufferValidationTests.cpp | 20 +++++++++---- 3 files changed, 33 insertions(+), 21 deletions(-) diff --git a/src/dawn/native/Buffer.cpp b/src/dawn/native/Buffer.cpp index df7e758f30..f90ebecac3 100644 --- a/src/dawn/native/Buffer.cpp +++ b/src/dawn/native/Buffer.cpp @@ -37,24 +37,25 @@ namespace dawn::native { namespace { struct MapRequestTask : TrackTaskCallback { - MapRequestTask(dawn::platform::Platform* platform, Ref buffer) - : TrackTaskCallback(platform), buffer(std::move(buffer)) {} + MapRequestTask(dawn::platform::Platform* platform, Ref buffer, MapRequestID id) + : TrackTaskCallback(platform), buffer(std::move(buffer)), id(id) {} void Finish() override { ASSERT(mSerial != kMaxExecutionSerial); TRACE_EVENT1(mPlatform, General, "Buffer::TaskInFlight::Finished", "serial", uint64_t(mSerial)); - buffer->OnMapRequestCompleted(WGPUBufferMapAsyncStatus_Success); + buffer->OnMapRequestCompleted(id, WGPUBufferMapAsyncStatus_Success); } void HandleDeviceLoss() override { - buffer->OnMapRequestCompleted(WGPUBufferMapAsyncStatus_DeviceLost); + buffer->OnMapRequestCompleted(id, WGPUBufferMapAsyncStatus_DeviceLost); } void HandleShutDown() override { - buffer->OnMapRequestCompleted(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback); + buffer->OnMapRequestCompleted(id, WGPUBufferMapAsyncStatus_DestroyedBeforeCallback); } ~MapRequestTask() override = default; private: Ref buffer; + MapRequestID id; }; class ErrorBuffer final : public BufferBase { @@ -316,9 +317,9 @@ MaybeError BufferBase::ValidateCanUseOnQueueNow() const { UNREACHABLE(); } -void BufferBase::CallMapCallback(WGPUBufferMapAsyncStatus status) { +void BufferBase::CallMapCallback(MapRequestID mapID, WGPUBufferMapAsyncStatus status) { ASSERT(!IsError()); - if (mMapCallback != nullptr) { + if (mMapCallback != nullptr && mapID == mLastMapID) { // 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; @@ -357,6 +358,7 @@ void BufferBase::APIMapAsync(wgpu::MapMode mode, } ASSERT(!IsError()); + mLastMapID++; mMapMode = mode; mMapOffset = offset; mMapSize = size; @@ -365,11 +367,11 @@ void BufferBase::APIMapAsync(wgpu::MapMode mode, mState = BufferState::PendingMap; if (GetDevice()->ConsumedError(MapAsyncImpl(mode, offset, size))) { - CallMapCallback(WGPUBufferMapAsyncStatus_DeviceLost); + CallMapCallback(mLastMapID, WGPUBufferMapAsyncStatus_DeviceLost); return; } std::unique_ptr request = - std::make_unique(GetDevice()->GetPlatform(), this); + std::make_unique(GetDevice()->GetPlatform(), this, mLastMapID); TRACE_EVENT1(GetDevice()->GetPlatform(), General, "Buffer::APIMapAsync", "serial", uint64_t(GetDevice()->GetPendingCommandSerial())); GetDevice()->GetQueue()->TrackTask(std::move(request)); @@ -439,7 +441,7 @@ void BufferBase::Unmap() { void BufferBase::UnmapInternal(WGPUBufferMapAsyncStatus callbackStatus) { if (mState == BufferState::PendingMap) { - CallMapCallback(callbackStatus); + CallMapCallback(mLastMapID, callbackStatus); UnmapImpl(); } else if (mState == BufferState::Mapped) { UnmapImpl(); @@ -552,11 +554,12 @@ MaybeError BufferBase::ValidateUnmap() const { return {}; } -void BufferBase::OnMapRequestCompleted(WGPUBufferMapAsyncStatus status) { - if (status == WGPUBufferMapAsyncStatus_Success && mState == BufferState::PendingMap) { +void BufferBase::OnMapRequestCompleted(MapRequestID mapID, WGPUBufferMapAsyncStatus status) { + if (mapID == mLastMapID && status == WGPUBufferMapAsyncStatus_Success && + mState == BufferState::PendingMap) { mState = BufferState::Mapped; } - CallMapCallback(status); + CallMapCallback(mapID, status); } bool BufferBase::NeedsInitialization() const { diff --git a/src/dawn/native/Buffer.h b/src/dawn/native/Buffer.h index 76d7f4cf65..5fab9b7e45 100644 --- a/src/dawn/native/Buffer.h +++ b/src/dawn/native/Buffer.h @@ -63,7 +63,7 @@ class BufferBase : public ApiObjectBase { wgpu::BufferUsage GetUsageExternalOnly() const; MaybeError MapAtCreation(); - void OnMapRequestCompleted(WGPUBufferMapAsyncStatus status); + void OnMapRequestCompleted(MapRequestID mapID, WGPUBufferMapAsyncStatus status); MaybeError ValidateCanUseOnQueueNow() const; @@ -110,7 +110,7 @@ class BufferBase : public ApiObjectBase { virtual bool IsCPUWritableAtCreation() const = 0; MaybeError CopyFromStagingBuffer(); - void CallMapCallback(WGPUBufferMapAsyncStatus status); + void CallMapCallback(MapRequestID mapID, WGPUBufferMapAsyncStatus status); MaybeError ValidateMapAsync(wgpu::MapMode mode, size_t offset, @@ -129,6 +129,7 @@ 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/unittests/validation/BufferValidationTests.cpp b/src/dawn/tests/unittests/validation/BufferValidationTests.cpp index 57b1dcd689..1dac4968d3 100644 --- a/src/dawn/tests/unittests/validation/BufferValidationTests.cpp +++ b/src/dawn/tests/unittests/validation/BufferValidationTests.cpp @@ -406,32 +406,40 @@ TEST_F(BufferValidationTest, MapAsync_UnmapBeforeResult) { // works as expected and we don't get the cancelled request's data. TEST_F(BufferValidationTest, MapAsync_UnmapBeforeResultAndMapAgain) { { - wgpu::Buffer buf = CreateMapReadBuffer(4); - buf.MapAsync(wgpu::MapMode::Read, 0, 4, ToMockBufferMapAsyncCallback, this + 0); + wgpu::Buffer buf = CreateMapReadBuffer(16); + buf.MapAsync(wgpu::MapMode::Read, 0, 8, ToMockBufferMapAsyncCallback, this + 0); EXPECT_CALL(*mockBufferMapAsyncCallback, Call(WGPUBufferMapAsyncStatus_UnmappedBeforeCallback, this + 0)) .Times(1); buf.Unmap(); - buf.MapAsync(wgpu::MapMode::Read, 0, 4, ToMockBufferMapAsyncCallback, this + 1); + buf.MapAsync(wgpu::MapMode::Read, 8, 8, ToMockBufferMapAsyncCallback, this + 1); EXPECT_CALL(*mockBufferMapAsyncCallback, Call(WGPUBufferMapAsyncStatus_Success, this + 1)) .Times(1); WaitForAllOperations(device); + + // Check that only the second MapAsync had an effect + ASSERT_EQ(nullptr, buf.GetConstMappedRange(0)); + ASSERT_NE(nullptr, buf.GetConstMappedRange(8)); } { - wgpu::Buffer buf = CreateMapWriteBuffer(4); - buf.MapAsync(wgpu::MapMode::Write, 0, 4, ToMockBufferMapAsyncCallback, this + 0); + wgpu::Buffer buf = CreateMapWriteBuffer(16); + buf.MapAsync(wgpu::MapMode::Write, 0, 8, ToMockBufferMapAsyncCallback, this + 0); EXPECT_CALL(*mockBufferMapAsyncCallback, Call(WGPUBufferMapAsyncStatus_UnmappedBeforeCallback, this + 0)) .Times(1); buf.Unmap(); - buf.MapAsync(wgpu::MapMode::Write, 0, 4, ToMockBufferMapAsyncCallback, this + 1); + buf.MapAsync(wgpu::MapMode::Write, 8, 8, ToMockBufferMapAsyncCallback, this + 1); EXPECT_CALL(*mockBufferMapAsyncCallback, Call(WGPUBufferMapAsyncStatus_Success, this + 1)) .Times(1); WaitForAllOperations(device); + + // Check that only the second MapAsync had an effect + ASSERT_EQ(nullptr, buf.GetConstMappedRange(0)); + ASSERT_NE(nullptr, buf.GetConstMappedRange(8)); } }