From 53cdbead780d47c83cb4678e59fa1d2444e2256a Mon Sep 17 00:00:00 2001 From: Corentin Wallez Date: Mon, 28 Sep 2020 14:14:44 +0000 Subject: [PATCH] Use typed integers for the Buffer MapRequestID This will prevent mixing it up with other serial types in the future. Bug: dawn:442 Change-Id: Ie655d57722fcd79c82acc5aac429aed2c2741c3e Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/28920 Commit-Queue: Corentin Wallez Reviewed-by: Austin Eng --- src/dawn_native/Buffer.cpp | 19 +++++++++---------- src/dawn_native/Buffer.h | 7 ++++--- src/dawn_native/IntegerTypes.h | 11 +++++++++++ src/dawn_native/MapRequestTracker.cpp | 9 ++++----- src/dawn_native/MapRequestTracker.h | 8 +++++--- 5 files changed, 33 insertions(+), 21 deletions(-) diff --git a/src/dawn_native/Buffer.cpp b/src/dawn_native/Buffer.cpp index aa9af0a3bb..9ec821e040 100644 --- a/src/dawn_native/Buffer.cpp +++ b/src/dawn_native/Buffer.cpp @@ -138,7 +138,7 @@ namespace dawn_native { BufferBase::~BufferBase() { if (mState == BufferState::Mapped) { ASSERT(!IsError()); - CallMapCallback(mMapSerial, WGPUBufferMapAsyncStatus_DestroyedBeforeCallback); + CallMapCallback(mLastMapID, WGPUBufferMapAsyncStatus_DestroyedBeforeCallback); } } @@ -212,9 +212,9 @@ namespace dawn_native { } } - void BufferBase::CallMapCallback(uint32_t serial, WGPUBufferMapAsyncStatus status) { + void BufferBase::CallMapCallback(MapRequestID mapID, WGPUBufferMapAsyncStatus status) { ASSERT(!IsError()); - if (mMapCallback != nullptr && serial == mMapSerial) { + 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; @@ -249,8 +249,7 @@ namespace dawn_native { } ASSERT(!IsError()); - // TODO(cwallez@chromium.org): what to do on wraparound? Could cause crashes. - mMapSerial++; + mLastMapID++; mMapMode = mode; mMapOffset = offset; mMapSize = size; @@ -259,12 +258,12 @@ namespace dawn_native { mState = BufferState::Mapped; if (GetDevice()->ConsumedError(MapAsyncImpl(mode, offset, size))) { - CallMapCallback(mMapSerial, WGPUBufferMapAsyncStatus_DeviceLost); + CallMapCallback(mLastMapID, WGPUBufferMapAsyncStatus_DeviceLost); return; } MapRequestTracker* tracker = GetDevice()->GetMapRequestTracker(); - tracker->Track(this, mMapSerial); + tracker->Track(this, mLastMapID); } void* BufferBase::GetMappedRange(size_t offset, size_t size) { @@ -350,7 +349,7 @@ namespace dawn_native { // completed before the Unmap. // Callbacks are not fired if there is no callback registered, so this is correct for // mappedAtCreation = true. - CallMapCallback(mMapSerial, callbackStatus); + CallMapCallback(mLastMapID, callbackStatus); UnmapImpl(); mMapCallback = nullptr; @@ -514,8 +513,8 @@ namespace dawn_native { mState = BufferState::Destroyed; } - void BufferBase::OnMapCommandSerialFinished(uint32_t mapSerial) { - CallMapCallback(mapSerial, WGPUBufferMapAsyncStatus_Success); + void BufferBase::OnMapRequestCompleted(MapRequestID mapID) { + CallMapCallback(mapID, WGPUBufferMapAsyncStatus_Success); } bool BufferBase::IsDataInitialized() const { diff --git a/src/dawn_native/Buffer.h b/src/dawn_native/Buffer.h index fc6353c194..675b071b2a 100644 --- a/src/dawn_native/Buffer.h +++ b/src/dawn_native/Buffer.h @@ -17,6 +17,7 @@ #include "dawn_native/Error.h" #include "dawn_native/Forward.h" +#include "dawn_native/IntegerTypes.h" #include "dawn_native/ObjectBase.h" #include "dawn_native/dawn_platform.h" @@ -52,7 +53,7 @@ namespace dawn_native { wgpu::BufferUsage GetUsage() const; MaybeError MapAtCreation(); - void OnMapCommandSerialFinished(uint32_t mapSerial); + void OnMapRequestCompleted(MapRequestID mapID); MaybeError ValidateCanUseOnQueueNow() const; @@ -91,7 +92,7 @@ namespace dawn_native { virtual bool IsCPUWritableAtCreation() const = 0; MaybeError CopyFromStagingBuffer(); void* GetMappedRangeInternal(bool writable, size_t offset, size_t size); - void CallMapCallback(uint32_t serial, WGPUBufferMapAsyncStatus status); + void CallMapCallback(MapRequestID mapID, WGPUBufferMapAsyncStatus status); MaybeError ValidateMap(wgpu::BufferUsage requiredUsage, WGPUBufferMapAsyncStatus* status) const; @@ -113,7 +114,7 @@ namespace dawn_native { WGPUBufferMapCallback mMapCallback = nullptr; void* mMapUserdata = 0; - uint32_t mMapSerial = 0; + MapRequestID mLastMapID = MapRequestID(0); wgpu::MapMode mMapMode = wgpu::MapMode::None; size_t mMapOffset = 0; size_t mMapSize = 0; diff --git a/src/dawn_native/IntegerTypes.h b/src/dawn_native/IntegerTypes.h index bd31c79a08..3b72722502 100644 --- a/src/dawn_native/IntegerTypes.h +++ b/src/dawn_native/IntegerTypes.h @@ -43,6 +43,17 @@ namespace dawn_native { constexpr VertexAttributeLocation kMaxVertexAttributesTyped = VertexAttributeLocation(kMaxVertexAttributes); + // Serials are 64bit integers that are incremented by one each time to produce unique values. + // Some serials (like queue serials) are compared numerically to know which one is before + // another, while some serials are only checked for equality. We call serials only checked + // for equality IDs. + + // Buffer mapping requests are stored outside of the buffer while they are being processed and + // cannot be invalidated. Instead they are associated with an ID, and when a map request is + // finished, the mapping callback is fired only if its ID matches the ID if the last request + // that was sent. + using MapRequestID = TypedInteger; + } // namespace dawn_native #endif // DAWNNATIVE_INTEGERTYPES_H_ diff --git a/src/dawn_native/MapRequestTracker.cpp b/src/dawn_native/MapRequestTracker.cpp index faf7034696..ef7c49c882 100644 --- a/src/dawn_native/MapRequestTracker.cpp +++ b/src/dawn_native/MapRequestTracker.cpp @@ -13,12 +13,11 @@ // limitations under the License. #include "dawn_native/MapRequestTracker.h" + #include "dawn_native/Buffer.h" #include "dawn_native/Device.h" namespace dawn_native { - struct Request; - class DeviceBase; MapRequestTracker::MapRequestTracker(DeviceBase* device) : mDevice(device) { } @@ -27,10 +26,10 @@ namespace dawn_native { ASSERT(mInflightRequests.Empty()); } - void MapRequestTracker::Track(BufferBase* buffer, uint32_t mapSerial) { + void MapRequestTracker::Track(BufferBase* buffer, MapRequestID mapID) { Request request; request.buffer = buffer; - request.mapSerial = mapSerial; + request.id = mapID; mInflightRequests.Enqueue(std::move(request), mDevice->GetPendingCommandSerial()); mDevice->AddFutureCallbackSerial(mDevice->GetPendingCommandSerial()); @@ -38,7 +37,7 @@ namespace dawn_native { void MapRequestTracker::Tick(Serial finishedSerial) { for (auto& request : mInflightRequests.IterateUpTo(finishedSerial)) { - request.buffer->OnMapCommandSerialFinished(request.mapSerial); + request.buffer->OnMapRequestCompleted(request.id); } mInflightRequests.ClearUpTo(finishedSerial); } diff --git a/src/dawn_native/MapRequestTracker.h b/src/dawn_native/MapRequestTracker.h index 06eae3dbad..2afad01f7d 100644 --- a/src/dawn_native/MapRequestTracker.h +++ b/src/dawn_native/MapRequestTracker.h @@ -16,7 +16,9 @@ #define DAWNNATIVE_MAPREQUESTTRACKER_H_ #include "common/SerialQueue.h" -#include "dawn_native/Device.h" +#include "dawn_native/Buffer.h" +#include "dawn_native/Forward.h" +#include "dawn_native/IntegerTypes.h" namespace dawn_native { @@ -25,7 +27,7 @@ namespace dawn_native { MapRequestTracker(DeviceBase* device); ~MapRequestTracker(); - void Track(BufferBase* buffer, uint32_t mapSerial); + void Track(BufferBase* buffer, MapRequestID mapID); void Tick(Serial finishedSerial); private: @@ -33,7 +35,7 @@ namespace dawn_native { struct Request { Ref buffer; - uint32_t mapSerial; + MapRequestID id; }; SerialQueue mInflightRequests; };