[dawn][native] Make DisposeCallback a callback instead of fnptr This allows the C++ API improvements of webgpu_cpp.h to be used with thi type. It required wrapping it in a DisposeCallbackInfo with a CallbackMode. Update the implementation to use a TrackedEvent instead of a TrackedTask now that there is a CallbackMode. Update tests accordingly and use the MockCppCallback helper. Bug: 386255678 Change-Id: I1aebd0e803ed39a4f33970b0180bbb40ef8b984c Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/343156 Commit-Queue: Shao, Jiawei <jiawei.shao@intel.com> Reviewed-by: Loko Kung <lokokung@google.com> Reviewed-by: Shao, Jiawei <jiawei.shao@intel.com>
diff --git a/generator/templates/api_cpp.h b/generator/templates/api_cpp.h index 9962959..c63be39 100644 --- a/generator/templates/api_cpp.h +++ b/generator/templates/api_cpp.h
@@ -799,7 +799,8 @@ struct CArgConverter; {% set SpecialCallbackInfos = [ "device lost callback info", "uncaptured error callback info", - "dawn load cache data callback info", "dawn store cache data callback info" ] %} + "dawn load cache data callback info", "dawn store cache data callback info", + "dispose callback"] %} {% for type in by_category["callback info"] if type.name.get() not in SpecialCallbackInfos %} {% set CallbackType = find_by_name(type.members, "callback").type %} template <>
diff --git a/src/dawn/dawn.json b/src/dawn/dawn.json index c483e46..45e22ac 100644 --- a/src/dawn/dawn.json +++ b/src/dawn/dawn.json
@@ -2031,11 +2031,18 @@ ] }, "dispose callback": { - "category": "function pointer", + "category": "callback function", "tags": ["dawn", "native"], "args": [ - {"name": "status", "type": "callback status"}, - {"name": "userdata", "type": "void *"} + {"name": "status", "type": "callback status"} + ] + }, + "dispose callback info": { + "category": "callback info", + "tags": ["dawn", "native"], + "members": [ + {"name": "mode", "type": "callback mode"}, + {"name": "callback", "type": "dispose callback", "default": "nullptr"} ] }, "shared buffer memory host pointer descriptor": { @@ -2046,8 +2053,7 @@ "members": [ {"name": "pointer", "type": "void *"}, {"name": "size", "type": "uint64_t"}, - {"name": "dispose callback", "type": "dispose callback"}, - {"name": "userdata", "type": "void *"} + {"name": "dispose callback info", "type": "dispose callback info"} ] }, "shared texture memory": {
diff --git a/src/dawn/native/d3d12/SharedBufferMemoryD3D12.cpp b/src/dawn/native/d3d12/SharedBufferMemoryD3D12.cpp index 4bc9eab..5254040 100644 --- a/src/dawn/native/d3d12/SharedBufferMemoryD3D12.cpp +++ b/src/dawn/native/d3d12/SharedBufferMemoryD3D12.cpp
@@ -34,6 +34,7 @@ #include "src/dawn/common/Math.h" #include "src/dawn/native/Buffer.h" #include "src/dawn/native/ChainUtils.h" +#include "src/dawn/native/Instance.h" #include "src/dawn/native/d3d/D3DError.h" #include "src/dawn/native/d3d/SharedFenceD3D.h" #include "src/dawn/native/d3d/UtilsD3D.h" @@ -169,7 +170,7 @@ void SharedBufferMemory::DestroyImpl(DestroyReason reason) { SharedBufferMemoryBase::DestroyImpl(reason); - if (mHostPointerDisposeCallback == nullptr) { + if (!mHostPointerDispose.has_value()) { ToBackend(GetDevice())->ReferenceUntilUnused(std::move(mResource)); return; } @@ -180,43 +181,39 @@ // task (instead of through Device::ReferenceUntilUnused, which is drained by a different // mechanism) guarantees the resource is released before the heap, and the heap before the // dispose callback. - struct DisposeTask : TrackTaskCallback { - DisposeTask(ComPtr<ID3D12Resource> resource, - std::unique_ptr<Heap> heap, - wgpu::DisposeCallback callback, - void* userdata) - : TrackTaskCallback(nullptr), + class DisposeEvent : public EventManager::TrackedEvent { + public: + DisposeEvent(DeviceBase* device, + ComPtr<ID3D12Resource> resource, + std::unique_ptr<Heap> heap, + WGPUDisposeCallbackInfo dispose) + : TrackedEvent(static_cast<wgpu::CallbackMode>(dispose.mode), + device->GetQueue(), + device->GetQueue()->GetPendingCommandSerial()), resource(std::move(resource)), heap(std::move(heap)), - callback(callback), - userdata(userdata) {} - ~DisposeTask() override = default; + mCallback(dispose.callback), + mUserdata1(dispose.userdata1), + mUserdata2(dispose.userdata2) {} + ~DisposeEvent() override = default; - void FinishImpl() override { Dispose(WGPUCallbackStatus_Success); } - void HandleDeviceLossImpl() override { Dispose(WGPUCallbackStatus_Error); } - void HandleShutDownImpl() override { Dispose(WGPUCallbackStatus_Error); } - - void Dispose(WGPUCallbackStatus status) { + private: + void Complete(EventCompletionType) override { resource = nullptr; heap = nullptr; - callback(status, userdata); + mCallback(WGPUCallbackStatus_Success, mUserdata1, mUserdata2); } ComPtr<ID3D12Resource> resource; std::unique_ptr<Heap> heap; - wgpu::DisposeCallback callback; - raw_ptr<void, DisableDanglingPtrDetection> userdata; + WGPUDisposeCallback mCallback = nullptr; + raw_ptr<void> mUserdata1 = nullptr; + raw_ptr<void> mUserdata2 = nullptr; }; - std::unique_ptr<DisposeTask> request = - std::make_unique<DisposeTask>(std::move(mResource), std::move(mHeap), - mHostPointerDisposeCallback, mHostPointerDisposeUserdata); - mHostPointerDisposeCallback = nullptr; - mHostPointerDisposeUserdata = nullptr; - // TODO(386255678): TrackTaskAfterEventualFlush() only marks the queue as needing a submit; it - // doesn't force one to happen immediately. If nothing ever ticks the device again, this task - // (and thus disposeCallback) may never run. - GetDevice()->GetQueue()->TrackTaskAfterEventualFlush(std::move(request)); + GetInstance()->GetEventManager()->TrackEvent(AcquireRef(new DisposeEvent( + GetDevice(), std::move(mResource), std::move(mHeap), mHostPointerDispose.value()))); + mHostPointerDispose = std::nullopt; } // static @@ -357,8 +354,9 @@ // Device::CreateSharedBufferMemory errors before or after calling this, e.g. due to unpacking // the chained struct, or a device loss / D3D12 error raised elsewhere in the meantime. absl::Cleanup disposeOnFailure = [descriptor] { - if (descriptor->disposeCallback) { - descriptor->disposeCallback(WGPUCallbackStatus_Error, descriptor->userdata); + if (descriptor->disposeCallbackInfo.callback != nullptr) { + const WGPUDisposeCallbackInfo& dispose = descriptor->disposeCallbackInfo; + dispose.callback(WGPUCallbackStatus_Error, dispose.userdata1, dispose.userdata2); } }; @@ -383,8 +381,7 @@ Ref<SharedBufferMemory> result; DAWN_TRY_ASSIGN(result, CreateFromHeap(device, label, std::move(d3d12Heap), descriptor->size, descriptor->pointer, wgpu::BufferUsage::None)); - result->mHostPointerDisposeCallback = descriptor->disposeCallback; - result->mHostPointerDisposeUserdata = descriptor->userdata; + result->mHostPointerDispose = descriptor->disposeCallbackInfo; std::move(disposeOnFailure).Cancel(); return result; }
diff --git a/src/dawn/native/d3d12/SharedBufferMemoryD3D12.h b/src/dawn/native/d3d12/SharedBufferMemoryD3D12.h index 6bf7bcc..e758450 100644 --- a/src/dawn/native/d3d12/SharedBufferMemoryD3D12.h +++ b/src/dawn/native/d3d12/SharedBufferMemoryD3D12.h
@@ -88,8 +88,7 @@ std::unique_ptr<Heap> mHeap; ComPtr<ID3D12Resource> mResource; - wgpu::DisposeCallback mHostPointerDisposeCallback = nullptr; - raw_ptr<void, DisableDanglingPtrDetection> mHostPointerDisposeUserdata = nullptr; + std::optional<WGPUDisposeCallbackInfo> mHostPointerDispose; }; } // namespace dawn::native::d3d12
diff --git a/src/dawn/tests/white_box/SharedBufferMemoryTests_win.cpp b/src/dawn/tests/white_box/SharedBufferMemoryTests_win.cpp index d143f56..7695f70 100644 --- a/src/dawn/tests/white_box/SharedBufferMemoryTests_win.cpp +++ b/src/dawn/tests/white_box/SharedBufferMemoryTests_win.cpp
@@ -710,11 +710,11 @@ wgpu::SharedBufferMemoryHostPointerDescriptor hostPointerDesc; hostPointerDesc.pointer = pointer.data(); hostPointerDesc.size = alignedSize; - hostPointerDesc.disposeCallback = [](WGPUCallbackStatus status, void* userdata) { - EXPECT_EQ(WGPUCallbackStatus_Success, status); - VirtualFree(userdata, 0, MEM_RELEASE); - }; - hostPointerDesc.userdata = allocationPtr; + hostPointerDesc.SetDisposeCallback(wgpu::CallbackMode::AllowSpontaneous, + [pointer](wgpu::CallbackStatus status) { + EXPECT_EQ(wgpu::CallbackStatus::Success, status); + VirtualFree(pointer.data(), 0, MEM_RELEASE); + }); desc.nextInChain = &hostPointerDesc; wgpu::SharedBufferMemory memory = device.ImportSharedBufferMemory(&desc); @@ -728,19 +728,22 @@ D3D12HostPointerBackend() {} }; -class SharedBufferMemoryD3D12HostPointerTests : public SharedBufferMemoryTests {}; +class SharedBufferMemoryD3D12HostPointerTests : public SharedBufferMemoryTests { + protected: + using MockDisposeCallback = testing::MockCppCallback<wgpu::DisposeCallback<void>*>; + testing::StrictMock<MockDisposeCallback> mMockDispose; +}; // Ensure that importing a nullptr host pointer results in error, and that disposeCallback is // still invoked exactly once, with an error status. TEST_P(SharedBufferMemoryD3D12HostPointerTests, NullPointerFailure) { - testing::MockCallback<wgpu::DisposeCallback> disposeCallback; - EXPECT_CALL(disposeCallback, Call(WGPUCallbackStatus_Error, nullptr)).Times(1); + EXPECT_CALL(mMockDispose, Call(wgpu::CallbackStatus::Error)).Times(1); wgpu::SharedBufferMemoryHostPointerDescriptor hostPointerDesc; hostPointerDesc.pointer = nullptr; hostPointerDesc.size = kD3D12SharedBufferMemoryHostPointerAlignment; - hostPointerDesc.disposeCallback = disposeCallback.Callback(); - hostPointerDesc.userdata = disposeCallback.MakeUserdata(nullptr); + hostPointerDesc.SetDisposeCallback(wgpu::CallbackMode::AllowSpontaneous, + mMockDispose.Callback()); wgpu::SharedBufferMemoryDescriptor desc; desc.nextInChain = &hostPointerDesc; ASSERT_DEVICE_ERROR(device.ImportSharedBufferMemory(&desc)); @@ -753,17 +756,14 @@ MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE); ASSERT_NE(ptr, nullptr); - testing::MockCallback<wgpu::DisposeCallback> disposeCallback; - EXPECT_CALL(disposeCallback, Call(WGPUCallbackStatus_Error, ptr)) - .WillOnce([](WGPUCallbackStatus, void* userdata) { - EXPECT_TRUE(VirtualFree(userdata, 0, MEM_RELEASE)); - }); + EXPECT_CALL(mMockDispose, Call(wgpu::CallbackStatus::Error)) + .WillOnce([ptr](wgpu::CallbackStatus) { EXPECT_TRUE(VirtualFree(ptr, 0, MEM_RELEASE)); }); wgpu::SharedBufferMemoryHostPointerDescriptor hostPointerDesc; hostPointerDesc.pointer = ptr; hostPointerDesc.size = 0; - hostPointerDesc.disposeCallback = disposeCallback.Callback(); - hostPointerDesc.userdata = disposeCallback.MakeUserdata(ptr); + hostPointerDesc.SetDisposeCallback(wgpu::CallbackMode::AllowSpontaneous, + mMockDispose.Callback()); wgpu::SharedBufferMemoryDescriptor desc; desc.nextInChain = &hostPointerDesc; ASSERT_DEVICE_ERROR(device.ImportSharedBufferMemory(&desc)); @@ -776,11 +776,8 @@ void* ptr = VirtualAlloc(nullptr, kAllocationSize, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE); ASSERT_NE(ptr, nullptr); - testing::MockCallback<wgpu::DisposeCallback> disposeCallback; - EXPECT_CALL(disposeCallback, Call(WGPUCallbackStatus_Error, ptr)) - .WillOnce([](WGPUCallbackStatus, void* userdata) { - EXPECT_TRUE(VirtualFree(userdata, 0, MEM_RELEASE)); - }); + EXPECT_CALL(mMockDispose, Call(wgpu::CallbackStatus::Error)) + .WillOnce([ptr](wgpu::CallbackStatus) { EXPECT_TRUE(VirtualFree(ptr, 0, MEM_RELEASE)); }); wgpu::SharedBufferMemoryHostPointerDescriptor hostPointerDesc; // SAFETY: `ptr + kD3D12SharedBufferMemoryHostPointerAlignment / 2` and the following @@ -788,8 +785,8 @@ hostPointerDesc.pointer = DAWN_UNSAFE_BUFFERS(static_cast<uint8_t*>(ptr) + kD3D12SharedBufferMemoryHostPointerAlignment / 2); hostPointerDesc.size = kD3D12SharedBufferMemoryHostPointerAlignment; - hostPointerDesc.disposeCallback = disposeCallback.Callback(); - hostPointerDesc.userdata = disposeCallback.MakeUserdata(ptr); + hostPointerDesc.SetDisposeCallback(wgpu::CallbackMode::AllowSpontaneous, + mMockDispose.Callback()); wgpu::SharedBufferMemoryDescriptor desc; desc.nextInChain = &hostPointerDesc; ASSERT_DEVICE_ERROR(device.ImportSharedBufferMemory(&desc)); @@ -802,19 +799,15 @@ MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE); ASSERT_NE(ptr, nullptr); - testing::MockCallback<wgpu::DisposeCallback> disposeCallback; - EXPECT_CALL(disposeCallback, Call(WGPUCallbackStatus_Success, ptr)) - .WillOnce([ptr](WGPUCallbackStatus status, void*) { - EXPECT_EQ(WGPUCallbackStatus_Success, status); - EXPECT_TRUE(VirtualFree(ptr, 0, MEM_RELEASE)); - }); + EXPECT_CALL(mMockDispose, Call(wgpu::CallbackStatus::Success)) + .WillOnce([ptr](wgpu::CallbackStatus) { EXPECT_TRUE(VirtualFree(ptr, 0, MEM_RELEASE)); }); { wgpu::SharedBufferMemoryHostPointerDescriptor hostPointerDesc; hostPointerDesc.pointer = ptr; hostPointerDesc.size = kD3D12SharedBufferMemoryHostPointerAlignment; - hostPointerDesc.disposeCallback = disposeCallback.Callback(); - hostPointerDesc.userdata = disposeCallback.MakeUserdata(ptr); + hostPointerDesc.SetDisposeCallback(wgpu::CallbackMode::AllowSpontaneous, + mMockDispose.Callback()); wgpu::SharedBufferMemoryDescriptor desc; desc.nextInChain = &hostPointerDesc;
diff --git a/src/dawn/wire/server/ServerInlineMemoryTransferService.cpp b/src/dawn/wire/server/ServerInlineMemoryTransferService.cpp index 2bf3f58..f607103 100644 --- a/src/dawn/wire/server/ServerInlineMemoryTransferService.cpp +++ b/src/dawn/wire/server/ServerInlineMemoryTransferService.cpp
@@ -143,13 +143,16 @@ return nullptr; } - WGPUSharedBufferMemoryHostPointerDescriptor hostPointerDesc = {}; - hostPointerDesc.chain.sType = WGPUSType_SharedBufferMemoryHostPointerDescriptor; - hostPointerDesc.pointer = mSharedMemory->GetMappedSpan().data(); - hostPointerDesc.size = mSharedMemory->GetAllocatedSize(); - // No-op in `disposeCallback` since the memory is managed outside Dawn native. - hostPointerDesc.userdata = nullptr; - hostPointerDesc.disposeCallback = [](WGPUCallbackStatus, void*) {}; + WGPUSharedBufferMemoryHostPointerDescriptor hostPointerDesc = { + .chain = {.sType = WGPUSType_SharedBufferMemoryHostPointerDescriptor}, + .pointer = mSharedMemory->GetMappedSpan().data(), + .size = mSharedMemory->GetAllocatedSize(), + .disposeCallbackInfo = + { + .mode = WGPUCallbackMode_AllowSpontaneous, + .callback = [](WGPUCallbackStatus, void*, void*) {}, + }, + }; WGPUSharedBufferMemoryDescriptor desc = {}; desc.nextInChain = &hostPointerDesc.chain;