From 2c8a17ecc72dbb30f2e414210f8a756dd74229e7 Mon Sep 17 00:00:00 2001 From: Natasha Lee Date: Mon, 16 Dec 2019 23:36:16 +0000 Subject: [PATCH] Refactor Device destructors to WaitForIdleForDestruction + Destroy To help with Device Loss, this splits Device backend destructors to WaitForIdleForDestruction and Destroy. WaitForIdleForDestruction waits for GPU to finish, checks errors and gets ready for destruction. Destroy is used to clean up and release resources used by device, does not wait for GPU or check errors. Bug: dawn:68 Change-Id: I054fd735e8d5b289365604209f38e616c723a4e7 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/14560 Reviewed-by: Corentin Wallez Reviewed-by: Austin Eng Reviewed-by: Kai Ninomiya Reviewed-by: Rafael Cintron Commit-Queue: Natasha Lee --- src/dawn_native/Device.cpp | 9 ++ src/dawn_native/Device.h | 11 ++ src/dawn_native/d3d12/DeviceD3D12.cpp | 64 ++++++----- src/dawn_native/d3d12/DeviceD3D12.h | 3 + src/dawn_native/metal/DeviceMTL.h | 2 + src/dawn_native/metal/DeviceMTL.mm | 51 +++++---- src/dawn_native/null/DeviceNull.cpp | 17 ++- src/dawn_native/null/DeviceNull.h | 3 + src/dawn_native/opengl/DeviceGL.cpp | 30 ++++-- src/dawn_native/opengl/DeviceGL.h | 2 + src/dawn_native/vulkan/DeviceVk.cpp | 149 ++++++++++++++------------ src/dawn_native/vulkan/DeviceVk.h | 3 + 12 files changed, 209 insertions(+), 135 deletions(-) diff --git a/src/dawn_native/Device.cpp b/src/dawn_native/Device.cpp index 664b81d591..9c136beaf1 100644 --- a/src/dawn_native/Device.cpp +++ b/src/dawn_native/Device.cpp @@ -94,6 +94,15 @@ namespace dawn_native { ASSERT(mCaches->shaderModules.empty()); } + void DeviceBase::BaseDestructor() { + MaybeError err = WaitForIdleForDestruction(); + if (err.IsError()) { + // Assert that errors are device loss so that we can continue with destruction + ASSERT(err.AcquireError()->GetType() == wgpu::ErrorType::DeviceLost); + } + Destroy(); + } + void DeviceBase::HandleError(wgpu::ErrorType type, const char* message) { mCurrentErrorScope->HandleError(type, message); } diff --git a/src/dawn_native/Device.h b/src/dawn_native/Device.h index e887fa7c1a..7f22df535b 100644 --- a/src/dawn_native/Device.h +++ b/src/dawn_native/Device.h @@ -192,6 +192,7 @@ namespace dawn_native { protected: void SetToggle(Toggle toggle, bool isEnabled); void ApplyToggleOverrides(const DeviceDescriptor* deviceDescriptor); + void BaseDestructor(); std::unique_ptr mDynamicUploader; @@ -251,6 +252,16 @@ namespace dawn_native { void ConsumeError(ErrorData* error); + // Destroy is used to clean up and release resources used by device, does not wait for GPU + // or check errors. + virtual void Destroy() = 0; + + // WaitForIdleForDestruction waits for GPU to finish, checks errors and gets ready for + // destruction. This is only used when properly destructing the device. For a real + // device loss, this function doesn't need to be called since the driver already closed all + // resources. + virtual MaybeError WaitForIdleForDestruction() = 0; + AdapterBase* mAdapter = nullptr; Ref mRootErrorScope; diff --git a/src/dawn_native/d3d12/DeviceD3D12.cpp b/src/dawn_native/d3d12/DeviceD3D12.cpp index 204e55c90b..352fe00495 100644 --- a/src/dawn_native/d3d12/DeviceD3D12.cpp +++ b/src/dawn_native/d3d12/DeviceD3D12.cpp @@ -17,6 +17,7 @@ #include "common/Assert.h" #include "dawn_native/BackendConnection.h" #include "dawn_native/DynamicUploader.h" +#include "dawn_native/ErrorData.h" #include "dawn_native/d3d12/AdapterD3D12.h" #include "dawn_native/d3d12/BackendD3D12.h" #include "dawn_native/d3d12/BindGroupD3D12.h" @@ -104,34 +105,7 @@ namespace dawn_native { namespace d3d12 { } Device::~Device() { - // Immediately forget about all pending commands - mPendingCommands.Release(); - - ConsumedError(NextSerial()); - // Wait for all in-flight commands to finish executing - ConsumedError(WaitForSerial(mLastSubmittedSerial)); - - // Call tick one last time so resources are cleaned up. Ignore the return value so we can - // continue shutting down in an orderly fashion. - ConsumedError(TickImpl()); - - // Free services explicitly so that they can free D3D12 resources before destruction of the - // device. - mDynamicUploader = nullptr; - - // GPU is no longer executing commands. Existing objects do not get freed until the device - // is destroyed. To ensure objects are always released, force the completed serial to be - // MAX. - mCompletedSerial = std::numeric_limits::max(); - - if (mFenceEvent != nullptr) { - ::CloseHandle(mFenceEvent); - } - - mUsedComObjectRefs.ClearUpTo(mCompletedSerial); - - ASSERT(mUsedComObjectRefs.Empty()); - ASSERT(!mPendingCommands.IsOpen()); + BaseDestructor(); } ComPtr Device::GetD3D12Device() const { @@ -417,4 +391,38 @@ namespace dawn_native { namespace d3d12 { SetToggle(Toggle::UseD3D12RenderPass, GetDeviceInfo().supportsRenderPass); } + MaybeError Device::WaitForIdleForDestruction() { + DAWN_TRY(NextSerial()); + // Wait for all in-flight commands to finish executing + DAWN_TRY(WaitForSerial(mLastSubmittedSerial)); + + // Call tick one last time so resources are cleaned up. + DAWN_TRY(TickImpl()); + + return {}; + } + + void Device::Destroy() { + // Immediately forget about all pending commands + mPendingCommands.Release(); + + // Free services explicitly so that they can free D3D12 resources before destruction of the + // device. + mDynamicUploader = nullptr; + + // GPU is no longer executing commands. Existing objects do not get freed until the device + // is destroyed. To ensure objects are always released, force the completed serial to be + // MAX. + mCompletedSerial = std::numeric_limits::max(); + + if (mFenceEvent != nullptr) { + ::CloseHandle(mFenceEvent); + } + + mUsedComObjectRefs.ClearUpTo(mCompletedSerial); + + ASSERT(mUsedComObjectRefs.Empty()); + ASSERT(!mPendingCommands.IsOpen()); + } + }} // namespace dawn_native::d3d12 diff --git a/src/dawn_native/d3d12/DeviceD3D12.h b/src/dawn_native/d3d12/DeviceD3D12.h index 2740e039bd..a29adc9ca6 100644 --- a/src/dawn_native/d3d12/DeviceD3D12.h +++ b/src/dawn_native/d3d12/DeviceD3D12.h @@ -127,6 +127,9 @@ namespace dawn_native { namespace d3d12 { TextureBase* texture, const TextureViewDescriptor* descriptor) override; + void Destroy() override; + MaybeError WaitForIdleForDestruction() override; + Serial mCompletedSerial = 0; Serial mLastSubmittedSerial = 0; ComPtr mFence; diff --git a/src/dawn_native/metal/DeviceMTL.h b/src/dawn_native/metal/DeviceMTL.h index 4424dc8910..2219ab6aa5 100644 --- a/src/dawn_native/metal/DeviceMTL.h +++ b/src/dawn_native/metal/DeviceMTL.h @@ -90,6 +90,8 @@ namespace dawn_native { namespace metal { const TextureViewDescriptor* descriptor) override; void InitTogglesFromDriver(); + void Destroy() override; + MaybeError WaitForIdleForDestruction() override; id mMtlDevice = nil; id mCommandQueue = nil; diff --git a/src/dawn_native/metal/DeviceMTL.mm b/src/dawn_native/metal/DeviceMTL.mm index 0249df12c3..6cd73a9c1e 100644 --- a/src/dawn_native/metal/DeviceMTL.mm +++ b/src/dawn_native/metal/DeviceMTL.mm @@ -18,6 +18,7 @@ #include "dawn_native/BindGroup.h" #include "dawn_native/BindGroupLayout.h" #include "dawn_native/DynamicUploader.h" +#include "dawn_native/ErrorData.h" #include "dawn_native/metal/BufferMTL.h" #include "dawn_native/metal/CommandBufferMTL.h" #include "dawn_native/metal/ComputePipelineMTL.h" @@ -53,27 +54,7 @@ namespace dawn_native { namespace metal { } Device::~Device() { - // Wait for all commands to be finished so we can free resources SubmitPendingCommandBuffer - // may not increment the pendingCommandSerial if there are no pending commands, so we can't - // store the pendingSerial before SubmitPendingCommandBuffer then wait for it to be passed. - // Instead we submit and wait for the serial before the next pendingCommandSerial. - SubmitPendingCommandBuffer(); - while (GetCompletedCommandSerial() != mLastSubmittedSerial) { - usleep(100); - } - Tick(); - - [mPendingCommands release]; - mPendingCommands = nil; - - mMapTracker = nullptr; - mDynamicUploader = nullptr; - - [mCommandQueue release]; - mCommandQueue = nil; - - [mMtlDevice release]; - mMtlDevice = nil; + BaseDestructor(); } void Device::InitTogglesFromDriver() { @@ -291,4 +272,32 @@ namespace dawn_native { namespace metal { [mLastSubmittedCommands waitUntilScheduled]; } + MaybeError Device::WaitForIdleForDestruction() { + [mPendingCommands release]; + mPendingCommands = nil; + + // Wait for all commands to be finished so we can free resources + while (GetCompletedCommandSerial() != mLastSubmittedSerial) { + usleep(100); + } + Tick(); + return {}; + } + + void Device::Destroy() { + if (mPendingCommands != nil) { + [mPendingCommands release]; + mPendingCommands = nil; + } + + mMapTracker = nullptr; + mDynamicUploader = nullptr; + + [mCommandQueue release]; + mCommandQueue = nil; + + [mMtlDevice release]; + mMtlDevice = nil; + } + }} // namespace dawn_native::metal diff --git a/src/dawn_native/null/DeviceNull.cpp b/src/dawn_native/null/DeviceNull.cpp index 872d48674c..d721751477 100644 --- a/src/dawn_native/null/DeviceNull.cpp +++ b/src/dawn_native/null/DeviceNull.cpp @@ -17,6 +17,7 @@ #include "dawn_native/BackendConnection.h" #include "dawn_native/Commands.h" #include "dawn_native/DynamicUploader.h" +#include "dawn_native/ErrorData.h" #include "dawn_native/Instance.h" #include @@ -85,10 +86,7 @@ namespace dawn_native { namespace null { } Device::~Device() { - mDynamicUploader = nullptr; - - mPendingOperations.clear(); - ASSERT(mMemoryUsage == 0); + BaseDestructor(); } ResultOrError Device::CreateBindGroupImpl( @@ -167,6 +165,17 @@ namespace dawn_native { namespace null { return std::move(stagingBuffer); } + void Device::Destroy() { + mDynamicUploader = nullptr; + + mPendingOperations.clear(); + ASSERT(mMemoryUsage == 0); + } + + MaybeError Device::WaitForIdleForDestruction() { + return {}; + } + MaybeError Device::CopyFromStagingToBuffer(StagingBufferBase* source, uint64_t sourceOffset, BufferBase* destination, diff --git a/src/dawn_native/null/DeviceNull.h b/src/dawn_native/null/DeviceNull.h index 82b37bc798..c44e027c49 100644 --- a/src/dawn_native/null/DeviceNull.h +++ b/src/dawn_native/null/DeviceNull.h @@ -130,6 +130,9 @@ namespace dawn_native { namespace null { TextureBase* texture, const TextureViewDescriptor* descriptor) override; + void Destroy() override; + MaybeError WaitForIdleForDestruction() override; + Serial mCompletedSerial = 0; Serial mLastSubmittedSerial = 0; std::vector> mPendingOperations; diff --git a/src/dawn_native/opengl/DeviceGL.cpp b/src/dawn_native/opengl/DeviceGL.cpp index 9c252ef6b6..d44bd525f3 100644 --- a/src/dawn_native/opengl/DeviceGL.cpp +++ b/src/dawn_native/opengl/DeviceGL.cpp @@ -18,6 +18,7 @@ #include "dawn_native/BindGroup.h" #include "dawn_native/BindGroupLayout.h" #include "dawn_native/DynamicUploader.h" +#include "dawn_native/ErrorData.h" #include "dawn_native/opengl/BufferGL.h" #include "dawn_native/opengl/CommandBufferGL.h" #include "dawn_native/opengl/ComputePipelineGL.h" @@ -42,17 +43,7 @@ namespace dawn_native { namespace opengl { } Device::~Device() { - CheckPassedFences(); - ASSERT(mFencesInFlight.empty()); - - // Some operations might have been started since the last submit and waiting - // on a serial that doesn't have a corresponding fence enqueued. Force all - // operations to look as if they were completed (because they were). - mCompletedSerial = mLastSubmittedSerial + 1; - - mDynamicUploader = nullptr; - - Tick(); + BaseDestructor(); } const GLFormat& Device::GetGLFormat(const Format& format) { @@ -170,4 +161,21 @@ namespace dawn_native { namespace opengl { return DAWN_UNIMPLEMENTED_ERROR("Device unable to copy from staging buffer."); } + void Device::Destroy() { + // Some operations might have been started since the last submit and waiting + // on a serial that doesn't have a corresponding fence enqueued. Force all + // operations to look as if they were completed (because they were). + mCompletedSerial = mLastSubmittedSerial + 1; + + mDynamicUploader = nullptr; + } + + MaybeError Device::WaitForIdleForDestruction() { + gl.Finish(); + CheckPassedFences(); + ASSERT(mFencesInFlight.empty()); + Tick(); + return {}; + } + }} // namespace dawn_native::opengl diff --git a/src/dawn_native/opengl/DeviceGL.h b/src/dawn_native/opengl/DeviceGL.h index 757f27cd9d..aa9770822c 100644 --- a/src/dawn_native/opengl/DeviceGL.h +++ b/src/dawn_native/opengl/DeviceGL.h @@ -86,6 +86,8 @@ namespace dawn_native { namespace opengl { const TextureViewDescriptor* descriptor) override; void CheckPassedFences(); + void Destroy() override; + MaybeError WaitForIdleForDestruction() override; Serial mCompletedSerial = 0; Serial mLastSubmittedSerial = 0; diff --git a/src/dawn_native/vulkan/DeviceVk.cpp b/src/dawn_native/vulkan/DeviceVk.cpp index 232d9b8dbf..1b998145a1 100644 --- a/src/dawn_native/vulkan/DeviceVk.cpp +++ b/src/dawn_native/vulkan/DeviceVk.cpp @@ -87,77 +87,7 @@ namespace dawn_native { namespace vulkan { } Device::~Device() { - // Immediately tag the recording context as unused so we don't try to submit it in Tick. - mRecordingContext.used = false; - fn.DestroyCommandPool(mVkDevice, mRecordingContext.commandPool, nullptr); - - VkResult waitIdleResult = fn.QueueWaitIdle(mQueue); - // Ignore the result of QueueWaitIdle: it can return OOM which we can't really do anything - // about, Device lost, which means workloads running on the GPU are no longer accessible - // (so they are as good as waited on) or success. - DAWN_UNUSED(waitIdleResult); - - CheckPassedFences(); - - // Make sure all fences are complete by explicitly waiting on them all - while (!mFencesInFlight.empty()) { - VkFence fence = mFencesInFlight.front().first; - Serial fenceSerial = mFencesInFlight.front().second; - ASSERT(fenceSerial > mCompletedSerial); - - VkResult result = VK_TIMEOUT; - do { - result = fn.WaitForFences(mVkDevice, 1, &fence, true, UINT64_MAX); - } while (result == VK_TIMEOUT); - fn.DestroyFence(mVkDevice, fence, nullptr); - - mFencesInFlight.pop(); - mCompletedSerial = fenceSerial; - } - - // Some operations might have been started since the last submit and waiting - // on a serial that doesn't have a corresponding fence enqueued. Force all - // operations to look as if they were completed (because they were). - mCompletedSerial = mLastSubmittedSerial + 1; - Tick(); - - ASSERT(mCommandsInFlight.Empty()); - for (const CommandPoolAndBuffer& commands : mUnusedCommands) { - fn.DestroyCommandPool(mVkDevice, commands.pool, nullptr); - } - mUnusedCommands.clear(); - - // TODO(jiajie.hu@intel.com): In rare cases, a DAWN_TRY() failure may leave semaphores - // untagged for deletion. But for most of the time when everything goes well, these - // assertions can be helpful in catching bugs. - ASSERT(mRecordingContext.waitSemaphores.empty()); - ASSERT(mRecordingContext.signalSemaphores.empty()); - - for (VkFence fence : mUnusedFences) { - fn.DestroyFence(mVkDevice, fence, nullptr); - } - mUnusedFences.clear(); - - // Free services explicitly so that they can free Vulkan objects before vkDestroyDevice - mDynamicUploader = nullptr; - mDescriptorSetService = nullptr; - - // Releasing the uploader enqueues buffers to be released. - // Call Tick() again to clear them before releasing the deleter. - mDeleter->Tick(mCompletedSerial); - - mDeleter = nullptr; - mMapRequestTracker = nullptr; - - // The VkRenderPasses in the cache can be destroyed immediately since all commands referring - // to them are guaranteed to be finished executing. - mRenderPassCache = nullptr; - - // VkQueues are destroyed when the VkDevice is destroyed - if (mVkDevice != VK_NULL_HANDLE) { - fn.DestroyDevice(mVkDevice, nullptr); - mVkDevice = VK_NULL_HANDLE; - } + BaseDestructor(); } ResultOrError Device::CreateBindGroupImpl( @@ -768,4 +698,81 @@ namespace dawn_native { namespace vulkan { return mResourceMemoryAllocator.get(); } + MaybeError Device::WaitForIdleForDestruction() { + VkResult waitIdleResult = fn.QueueWaitIdle(mQueue); + // Ignore the result of QueueWaitIdle: it can return OOM which we can't really do anything + // about, Device lost, which means workloads running on the GPU are no longer accessible + // (so they are as good as waited on) or success. + DAWN_UNUSED(waitIdleResult); + + CheckPassedFences(); + + // Make sure all fences are complete by explicitly waiting on them all + while (!mFencesInFlight.empty()) { + VkFence fence = mFencesInFlight.front().first; + Serial fenceSerial = mFencesInFlight.front().second; + ASSERT(fenceSerial > mCompletedSerial); + + VkResult result = VK_TIMEOUT; + do { + result = fn.WaitForFences(mVkDevice, 1, &fence, true, UINT64_MAX); + } while (result == VK_TIMEOUT); + fn.DestroyFence(mVkDevice, fence, nullptr); + + mFencesInFlight.pop(); + mCompletedSerial = fenceSerial; + } + return {}; + } + + void Device::Destroy() { + // Immediately tag the recording context as unused so we don't try to submit it in Tick. + mRecordingContext.used = false; + fn.DestroyCommandPool(mVkDevice, mRecordingContext.commandPool, nullptr); + + // Some operations might have been started since the last submit and waiting + // on a serial that doesn't have a corresponding fence enqueued. Force all + // operations to look as if they were completed (because they were). + mCompletedSerial = mLastSubmittedSerial + 1; + Tick(); + + ASSERT(mCommandsInFlight.Empty()); + for (const CommandPoolAndBuffer& commands : mUnusedCommands) { + fn.DestroyCommandPool(mVkDevice, commands.pool, nullptr); + } + mUnusedCommands.clear(); + + // TODO(jiajie.hu@intel.com): In rare cases, a DAWN_TRY() failure may leave semaphores + // untagged for deletion. But for most of the time when everything goes well, these + // assertions can be helpful in catching bugs. + ASSERT(mRecordingContext.waitSemaphores.empty()); + ASSERT(mRecordingContext.signalSemaphores.empty()); + + for (VkFence fence : mUnusedFences) { + fn.DestroyFence(mVkDevice, fence, nullptr); + } + mUnusedFences.clear(); + + // Free services explicitly so that they can free Vulkan objects before vkDestroyDevice + mDynamicUploader = nullptr; + mDescriptorSetService = nullptr; + + // Releasing the uploader enqueues buffers to be released. + // Call Tick() again to clear them before releasing the deleter. + mDeleter->Tick(mCompletedSerial); + + mDeleter = nullptr; + mMapRequestTracker = nullptr; + + // The VkRenderPasses in the cache can be destroyed immediately since all commands referring + // to them are guaranteed to be finished executing. + mRenderPassCache = nullptr; + + // VkQueues are destroyed when the VkDevice is destroyed + if (mVkDevice != VK_NULL_HANDLE) { + fn.DestroyDevice(mVkDevice, nullptr); + mVkDevice = VK_NULL_HANDLE; + } + } + }} // namespace dawn_native::vulkan diff --git a/src/dawn_native/vulkan/DeviceVk.h b/src/dawn_native/vulkan/DeviceVk.h index 13eb152587..79c9d654bf 100644 --- a/src/dawn_native/vulkan/DeviceVk.h +++ b/src/dawn_native/vulkan/DeviceVk.h @@ -130,6 +130,9 @@ namespace dawn_native { namespace vulkan { void InitTogglesFromDriver(); void ApplyDepth24PlusS8Toggle(); + void Destroy() override; + MaybeError WaitForIdleForDestruction() override; + // To make it easier to use fn it is a public const member. However // the Device is allowed to mutate them through these private methods. VulkanFunctions* GetMutableFunctions();