Reland "Graphite/Dawn: Always use asyncMap even for native" What's changed: only use SingleOwner to protect fAsyncMapCallbacks. The previous SingleOwner was used to protect the whole asyncMap and mapCallback's scope. That would be violated if the mapCallback is triggered after MapAsync call but before the asyncMap returns. Original change's description: > Graphite/Dawn: Always use asyncMap even for native > > Bug: b/422741977 > Change-Id: Ia16480df5ca276def6bd9969d43b6a697e5a4348 > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1158578 > Commit-Queue: Quyen Le <lehoangquyen@chromium.org> > Reviewed-by: Michael Ludwig <michaelludwig@google.com> Bug: b/422741977 Change-Id: I500071bdf1c049a463f415c8c13f78336cb4c87a Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1177616 Reviewed-by: Michael Ludwig <michaelludwig@google.com> Commit-Queue: Quyen Le <lehoangquyen@chromium.org>
diff --git a/src/gpu/graphite/Resource.h b/src/gpu/graphite/Resource.h index 4ad6069..6381db9 100644 --- a/src/gpu/graphite/Resource.h +++ b/src/gpu/graphite/Resource.h
@@ -331,7 +331,7 @@ const SharedContext* sharedContext() const { return fSharedContext; } - // Needs to be protected for DawnBuffer's emscripten prepareForReturnToCache + // Needs to be protected for DawnBuffer's prepareForReturnToCache void setDeleteASAP() { fDeleteASAP = DeleteASAP::kYes; } using TakeRefFunc = void (*)(void* ctx);
diff --git a/src/gpu/graphite/dawn/DawnBuffer.cpp b/src/gpu/graphite/dawn/DawnBuffer.cpp index a58a890f..e390523 100644 --- a/src/gpu/graphite/dawn/DawnBuffer.cpp +++ b/src/gpu/graphite/dawn/DawnBuffer.cpp
@@ -186,35 +186,22 @@ Protected::kNo, // Dawn doesn't support protected memory label, /*reusableRequiresPurgeable=*/buffer.GetUsage() & wgpu::BufferUsage::MapWrite, -#if defined(__EMSCRIPTEN__) // prepareForReturnToCache only needs to be called for a buffer that is mappable // for writing /* requiresPrepareForReturnToCache= */ - fBuffer.GetUsage() & wgpu::BufferUsage::MapWrite -#else - /* requiresPrepareForReturnToCache= */ false) -#endif + buffer.GetUsage() & wgpu::BufferUsage::MapWrite) , fBuffer(std::move(buffer)) { - fMapPtr = mappedAtCreationPtr; // Update the newly-created underlying GPU object's label to match the Resource's this->synchronizeBackendLabel(); } -#if defined(__EMSCRIPTEN__) bool DawnBuffer::prepareForReturnToCache(Resource::TakeRefFunc takeRef, void* takeRefCtx) { - // This function is only useful for Emscripten where we have to pre-map the buffer - // once it is returned to the cache. - SkASSERT(this->sharedContext()->caps()->bufferMapsAreAsync()); - // This implementation is almost Dawn-agnostic. However, Buffer base class doesn't have any // way of distinguishing a buffer that is mappable for writing from one mappable for reading. // We only need to re-map the former. SkASSERT(fBuffer.GetUsage() & wgpu::BufferUsage::MapWrite); - // We cannot start an async map while the GPU is still using the buffer. We asked that - // our Resource not become reusable until it was purgeable (no outstanding CPU or GPU refs) - SkASSERT(this->isPurgeable()); // Note that the map state cannot change on another thread when we are here. We got here // because there were no UsageRefs on the buffer but async mapping holds a UsageRef until it // completes. @@ -237,7 +224,7 @@ SkASSERT(this->sharedContext()->caps()->bufferMapsAreAsync()); if (proc) { - SkAutoMutexExclusive ex(fAsyncMutex); + SKGPU_ASSERT_SINGLE_OWNER(&fSingleAsyncMapCallbacksOwner) if (this->isMapped()) { proc(ctx, CallbackResult::kSuccess); return; @@ -254,6 +241,7 @@ bool isWrite = fBuffer.GetUsage() & wgpu::BufferUsage::MapWrite; auto buffer = sk_ref_sp(this); +#if defined(__EMSCRIPTEN__) fBuffer.MapAsync( isWrite ? wgpu::MapMode::Write : wgpu::MapMode::Read, 0, @@ -263,63 +251,24 @@ buffer->mapCallback(s, /*message=*/nullptr); }, buffer.release()); +#else + // Map with AllowSpontaneous so that if Dawn can trigger the callback immediately + // if possible, + fBuffer.MapAsync( + isWrite ? wgpu::MapMode::Write : wgpu::MapMode::Read, + 0, + fBuffer.GetSize(), + wgpu::CallbackMode::AllowSpontaneous, + [buffer](wgpu::MapAsyncStatus s, wgpu::StringView m) { + buffer->mapCallback(s, m); + }); +#endif } void DawnBuffer::onMap() { SKGPU_LOG_W("Synchronous buffer mapping not supported in Dawn. Failing map request."); } -#else - -void DawnBuffer::onMap() { - SkASSERT(!this->sharedContext()->caps()->bufferMapsAreAsync()); - SkASSERT(fBuffer); - SkASSERT((fBuffer.GetUsage() & wgpu::BufferUsage::MapRead) || - (fBuffer.GetUsage() & wgpu::BufferUsage::MapWrite)); - bool isWrite = fBuffer.GetUsage() & wgpu::BufferUsage::MapWrite; - - // Use wgpu::Future and WaitAny with timeout=0 to trigger callback immediately. - // This should work because our resource tracking mechanism should make sure that - // the buffer is free of any GPU use at this point. - wgpu::FutureWaitInfo mapWaitInfo{}; - - mapWaitInfo.future = fBuffer.MapAsync( - isWrite ? wgpu::MapMode::Write : wgpu::MapMode::Read, - 0, - fBuffer.GetSize(), - wgpu::CallbackMode::WaitAnyOnly, - [this](wgpu::MapAsyncStatus s, wgpu::StringView m) { this->mapCallback(s, m); }); - - wgpu::Instance instance = static_cast<const DawnSharedContext*>(sharedContext())->instance(); - [[maybe_unused]] auto status = instance.WaitAny(1, &mapWaitInfo, /*timeoutNS=*/0); - - if (status != wgpu::WaitStatus::Success) { - // WaitAny(timeout=0) might fail in this scenario: - // - Allocates a buffer. - // - Encodes a command buffer to copy a texture to this buffer. - // - Submits the command buffer. If OOM happens, this command buffer will fail to - // be submitted. - // - The buffer is *supposed* to be free of any GPU use since the command buffer that would - // have used it wasn't submitted successfully. - // - If we try to map this buffer at this point, internally Dawn will try to use GPU to - // clear this buffer to zeros, since this is its 1st use. WaitAny(timeout=0) won't work - // since the buffer now has a pending GPU clear operation. - // - // To work around this, we need to try again with a blocking WaitAny(), to wait for the - // clear operation to finish. - // Notes: - // - This fallback should be rare since it is caused by an OOM error during buffer - // readbacks. - // - For buffer writing cases, since we use mappedAtCreation, the GPU clear won't happen. - status = instance.WaitAny( - 1, &mapWaitInfo, /*timeoutNS=*/std::numeric_limits<uint64_t>::max()); - } - - SkASSERT(status == wgpu::WaitStatus::Success); - SkASSERT(mapWaitInfo.completed); -} -#endif // defined(__EMSCRIPTEN__) - void DawnBuffer::onUnmap() { SkASSERT(fBuffer); SkASSERT(this->isUnmappable()); @@ -330,7 +279,8 @@ template <typename StatusT, typename MessageT> void DawnBuffer::mapCallback(StatusT status, MessageT message) { - SkAutoMutexExclusive em(this->fAsyncMutex); + SKGPU_ASSERT_SINGLE_OWNER(&fSingleAsyncMapCallbacksOwner) + if (is_map_succeeded(status)) { if (this->fBuffer.GetUsage() & wgpu::BufferUsage::MapWrite) { this->fMapPtr = this->fBuffer.GetMappedRange();
diff --git a/src/gpu/graphite/dawn/DawnBuffer.h b/src/gpu/graphite/dawn/DawnBuffer.h index 7f6f6d4..6ce2e31 100644 --- a/src/gpu/graphite/dawn/DawnBuffer.h +++ b/src/gpu/graphite/dawn/DawnBuffer.h
@@ -11,6 +11,7 @@ #include "webgpu/webgpu_cpp.h" // NO_G3_REWRITE #include "include/core/SkRefCnt.h" +#include "include/private/base/SingleOwner.h" #include "include/private/base/SkTArray.h" #include "src/gpu/RefCntedCallback.h" #include "src/gpu/graphite/Buffer.h" @@ -38,10 +39,8 @@ void* mapAtCreationPtr, std::string_view label); -#if defined(__EMSCRIPTEN__) bool prepareForReturnToCache(Resource::TakeRefFunc takeRef, void* takeRefCtx) override; void onAsyncMap(GpuFinishedProc, GpuFinishedContext) override; -#endif void onMap() override; void onUnmap() override; @@ -57,8 +56,10 @@ void setBackendLabel(char const* label) override; wgpu::Buffer fBuffer; - SkMutex fAsyncMutex; - skia_private::STArray<1, AutoCallback> fAsyncMapCallbacks SK_GUARDED_BY(fAsyncMutex); + skia_private::STArray<1, AutoCallback> fAsyncMapCallbacks; + + // Ensure that only one thread can access fAsyncMapCallbacks. + [[maybe_unused]] SingleOwner fSingleAsyncMapCallbacksOwner; }; } // namespace skgpu::graphite
diff --git a/src/gpu/graphite/dawn/DawnCaps.cpp b/src/gpu/graphite/dawn/DawnCaps.cpp index 7e5e8a6..901f355 100644 --- a/src/gpu/graphite/dawn/DawnCaps.cpp +++ b/src/gpu/graphite/dawn/DawnCaps.cpp
@@ -397,16 +397,14 @@ // TODO: support clamp to border. fClampToBorderSupport = false; + // We use async map. + fBufferMapsAreAsync = true; + #if defined(GPU_TEST_UTILS) fDrawBufferCanBeMappedForReadback = false; #endif -#if defined(__EMSCRIPTEN__) - // For wasm, we use async map. - fBufferMapsAreAsync = true; -#else - // For Dawn native, we use direct mapping. - fBufferMapsAreAsync = false; +#if !defined(__EMSCRIPTEN__) fDrawBufferCanBeMapped = backendContext.fDevice.HasFeature(wgpu::FeatureName::BufferMapExtendedUsages);