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 <cwallez@chromium.org>
Reviewed-by: Austin Eng <enga@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Reviewed-by: Rafael Cintron <rafael.cintron@microsoft.com>
Commit-Queue: Natasha Lee <natlee@microsoft.com>

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);
     }

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<DynamicUploader> 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<ErrorScope> mRootErrorScope;

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
+        BaseDestructor();
-        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<Serial>::max();
-
-        if (mFenceEvent != nullptr) {
-            ::CloseHandle(mFenceEvent);
-        }
-
-        mUsedComObjectRefs.ClearUpTo(mCompletedSerial);
-
-        ASSERT(mUsedComObjectRefs.Empty());
-        ASSERT(!mPendingCommands.IsOpen());
     }
 
     ComPtr<ID3D12Device> 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<Serial>::max();
+
+        if (mFenceEvent != nullptr) {
+            ::CloseHandle(mFenceEvent);
+        }
+
+        mUsedComObjectRefs.ClearUpTo(mCompletedSerial);
+
+        ASSERT(mUsedComObjectRefs.Empty());
+        ASSERT(!mPendingCommands.IsOpen());
+    }
+
 }}  // namespace dawn_native::d3d12

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<ID3D12Fence> mFence;

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<MTLDevice> mMtlDevice = nil;
         id<MTLCommandQueue> mCommandQueue = nil;

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
+        BaseDestructor();
-        // 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;
     }
 
     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

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 <spirv_cross.hpp>
@@ -85,10 +86,7 @@ namespace dawn_native { namespace null {
     }
 
     Device::~Device() {
-        mDynamicUploader = nullptr;
+        BaseDestructor();
-
-        mPendingOperations.clear();
-        ASSERT(mMemoryUsage == 0);
     }
 
     ResultOrError<BindGroupBase*> 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,

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<std::unique_ptr<PendingOperation>> mPendingOperations;

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();
+        BaseDestructor();
-        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();
     }
 
     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

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;

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.
+        BaseDestructor();
-        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;
-        }
     }
 
     ResultOrError<BindGroupBase*> 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

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();