From 86d921e048483cc44898d967f0f120f166ca22e5 Mon Sep 17 00:00:00 2001 From: Rafael Cintron Date: Mon, 7 Oct 2019 15:32:10 +0000 Subject: [PATCH] Add MaybeError to d3d12::Device::ExecuteCommandList Closing a command list can fail. We need to handle the error gracefully instead of ignoring it. As a fallout from adding MaybeError to ExecuteCommandList, need to also add MaybeError to TickImpl, and OnBeforePresent. Bug:dawn:19 Change-Id: I13685f3dd731f4ab49cbff4ce4edfa960d630464 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/11841 Reviewed-by: Kai Ninomiya Reviewed-by: Corentin Wallez Commit-Queue: Rafael Cintron --- src/dawn_native/Device.cpp | 4 +++- src/dawn_native/Device.h | 2 +- src/dawn_native/SwapChain.cpp | 5 +++-- src/dawn_native/SwapChain.h | 2 +- src/dawn_native/d3d12/DeviceD3D12.cpp | 22 +++++++++++++++++----- src/dawn_native/d3d12/DeviceD3D12.h | 4 ++-- src/dawn_native/d3d12/QueueD3D12.cpp | 2 +- src/dawn_native/d3d12/SwapChainD3D12.cpp | 6 ++++-- src/dawn_native/d3d12/SwapChainD3D12.h | 2 +- src/dawn_native/metal/DeviceMTL.h | 2 +- src/dawn_native/metal/DeviceMTL.mm | 4 +++- src/dawn_native/metal/SwapChainMTL.h | 2 +- src/dawn_native/metal/SwapChainMTL.mm | 3 ++- src/dawn_native/null/DeviceNull.cpp | 6 ++++-- src/dawn_native/null/DeviceNull.h | 4 ++-- src/dawn_native/opengl/DeviceGL.cpp | 3 ++- src/dawn_native/opengl/DeviceGL.h | 2 +- src/dawn_native/opengl/SwapChainGL.cpp | 3 ++- src/dawn_native/opengl/SwapChainGL.h | 2 +- src/dawn_native/vulkan/DeviceVk.cpp | 4 +++- src/dawn_native/vulkan/DeviceVk.h | 2 +- src/dawn_native/vulkan/SwapChainVk.cpp | 4 +++- src/dawn_native/vulkan/SwapChainVk.h | 2 +- 23 files changed, 60 insertions(+), 32 deletions(-) diff --git a/src/dawn_native/Device.cpp b/src/dawn_native/Device.cpp index 2a6b915280..0f9bd64931 100644 --- a/src/dawn_native/Device.cpp +++ b/src/dawn_native/Device.cpp @@ -536,7 +536,9 @@ namespace dawn_native { // Other Device API methods void DeviceBase::Tick() { - TickImpl(); + if (ConsumedError(TickImpl())) + return; + { auto deferredResults = std::move(mDeferredCreateBufferMappedAsyncResults); for (const auto& deferred : deferredResults) { diff --git a/src/dawn_native/Device.h b/src/dawn_native/Device.h index a17d65a395..4ed2f6292e 100644 --- a/src/dawn_native/Device.h +++ b/src/dawn_native/Device.h @@ -81,7 +81,7 @@ namespace dawn_native { virtual Serial GetCompletedCommandSerial() const = 0; virtual Serial GetLastSubmittedCommandSerial() const = 0; virtual Serial GetPendingCommandSerial() const = 0; - virtual void TickImpl() = 0; + virtual MaybeError TickImpl() = 0; // Many Dawn objects are completely immutable once created which means that if two // creations are given the same arguments, they can return the same object. Reusing diff --git a/src/dawn_native/SwapChain.cpp b/src/dawn_native/SwapChain.cpp index d5f685a948..8194fe1546 100644 --- a/src/dawn_native/SwapChain.cpp +++ b/src/dawn_native/SwapChain.cpp @@ -32,7 +32,7 @@ namespace dawn_native { UNREACHABLE(); } - void OnBeforePresent(TextureBase* texture) override { + MaybeError OnBeforePresent(TextureBase* texture) override { UNREACHABLE(); } }; @@ -127,7 +127,8 @@ namespace dawn_native { } ASSERT(!IsError()); - OnBeforePresent(texture); + if (GetDevice()->ConsumedError(OnBeforePresent(texture))) + return; mImplementation.Present(mImplementation.userData); } diff --git a/src/dawn_native/SwapChain.h b/src/dawn_native/SwapChain.h index 8b0e9fdb5a..c9b6502879 100644 --- a/src/dawn_native/SwapChain.h +++ b/src/dawn_native/SwapChain.h @@ -47,7 +47,7 @@ namespace dawn_native { const DawnSwapChainImplementation& GetImplementation(); virtual TextureBase* GetNextTextureImpl(const TextureDescriptor*) = 0; - virtual void OnBeforePresent(TextureBase* texture) = 0; + virtual MaybeError OnBeforePresent(TextureBase* texture) = 0; private: MaybeError ValidateConfigure(dawn::TextureFormat format, diff --git a/src/dawn_native/d3d12/DeviceD3D12.cpp b/src/dawn_native/d3d12/DeviceD3D12.cpp index c45f8fe0c6..651b366b2f 100644 --- a/src/dawn_native/d3d12/DeviceD3D12.cpp +++ b/src/dawn_native/d3d12/DeviceD3D12.cpp @@ -109,7 +109,10 @@ namespace dawn_native { namespace d3d12 { } NextSerial(); WaitForSerial(mLastSubmittedSerial); // Wait for all in-flight commands to finish executing - TickImpl(); // Call tick one last time so resources are cleaned up + + // 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. @@ -209,7 +212,7 @@ namespace dawn_native { namespace d3d12 { return mLastSubmittedSerial + 1; } - void Device::TickImpl() { + MaybeError Device::TickImpl() { // Perform cleanup operations to free unused objects mCompletedSerial = mFence->GetCompletedValue(); @@ -222,8 +225,10 @@ namespace dawn_native { namespace d3d12 { mDescriptorHeapAllocator->Deallocate(mCompletedSerial); mMapRequestTracker->Tick(mCompletedSerial); mUsedComObjectRefs.ClearUpTo(mCompletedSerial); - ExecuteCommandList(nullptr); + DAWN_TRY(ExecuteCommandList(nullptr)); NextSerial(); + + return {}; } void Device::NextSerial() { @@ -243,13 +248,18 @@ namespace dawn_native { namespace d3d12 { mUsedComObjectRefs.Enqueue(object, GetPendingCommandSerial()); } - void Device::ExecuteCommandList(ID3D12CommandList* d3d12CommandList) { + MaybeError Device::ExecuteCommandList(ID3D12CommandList* d3d12CommandList) { UINT numLists = 0; std::array d3d12CommandLists; // If there are pending commands, prepend them to ExecuteCommandLists if (mPendingCommands.open) { - mPendingCommands.commandList->Close(); + const HRESULT hr = mPendingCommands.commandList->Close(); + if (FAILED(hr)) { + mPendingCommands.open = false; + mPendingCommands.commandList.Reset(); + return DAWN_DEVICE_LOST_ERROR("Error closing pending command list."); + } mPendingCommands.open = false; d3d12CommandLists[numLists++] = mPendingCommands.commandList.Get(); } @@ -260,6 +270,8 @@ namespace dawn_native { namespace d3d12 { mCommandQueue->ExecuteCommandLists(numLists, d3d12CommandLists.data()); mPendingCommands.commandList.Reset(); } + + return {}; } ResultOrError Device::CreateBindGroupImpl( diff --git a/src/dawn_native/d3d12/DeviceD3D12.h b/src/dawn_native/d3d12/DeviceD3D12.h index a250fadda1..e1a2fe223e 100644 --- a/src/dawn_native/d3d12/DeviceD3D12.h +++ b/src/dawn_native/d3d12/DeviceD3D12.h @@ -53,7 +53,7 @@ namespace dawn_native { namespace d3d12 { Serial GetCompletedCommandSerial() const final override; Serial GetLastSubmittedCommandSerial() const final override; - void TickImpl() override; + MaybeError TickImpl() override; ComPtr GetD3D12Device() const; ComPtr GetCommandQueue() const; @@ -78,7 +78,7 @@ namespace dawn_native { namespace d3d12 { void ReferenceUntilUnused(ComPtr object); - void ExecuteCommandList(ID3D12CommandList* d3d12CommandList); + MaybeError ExecuteCommandList(ID3D12CommandList* d3d12CommandList); ResultOrError> CreateStagingBuffer(size_t size) override; MaybeError CopyFromStagingToBuffer(StagingBufferBase* source, diff --git a/src/dawn_native/d3d12/QueueD3D12.cpp b/src/dawn_native/d3d12/QueueD3D12.cpp index c9d24b1511..acd24b8fac 100644 --- a/src/dawn_native/d3d12/QueueD3D12.cpp +++ b/src/dawn_native/d3d12/QueueD3D12.cpp @@ -33,7 +33,7 @@ namespace dawn_native { namespace d3d12 { } ASSERT_SUCCESS(mCommandList->Close()); - device->ExecuteCommandList(mCommandList.Get()); + DAWN_TRY(device->ExecuteCommandList(mCommandList.Get())); device->NextSerial(); return {}; diff --git a/src/dawn_native/d3d12/SwapChainD3D12.cpp b/src/dawn_native/d3d12/SwapChainD3D12.cpp index 8d67231b06..49cd6d5735 100644 --- a/src/dawn_native/d3d12/SwapChainD3D12.cpp +++ b/src/dawn_native/d3d12/SwapChainD3D12.cpp @@ -48,13 +48,15 @@ namespace dawn_native { namespace d3d12 { return new Texture(ToBackend(GetDevice()), descriptor, nativeTexture); } - void SwapChain::OnBeforePresent(TextureBase* texture) { + MaybeError SwapChain::OnBeforePresent(TextureBase* texture) { Device* device = ToBackend(GetDevice()); // Perform the necessary transition for the texture to be presented. ToBackend(texture)->TransitionUsageNow(device->GetPendingCommandList(), mTextureUsage); - device->ExecuteCommandList(nullptr); + DAWN_TRY(device->ExecuteCommandList(nullptr)); + + return {}; } }} // namespace dawn_native::d3d12 diff --git a/src/dawn_native/d3d12/SwapChainD3D12.h b/src/dawn_native/d3d12/SwapChainD3D12.h index 833ba48b81..151994c9b3 100644 --- a/src/dawn_native/d3d12/SwapChainD3D12.h +++ b/src/dawn_native/d3d12/SwapChainD3D12.h @@ -28,7 +28,7 @@ namespace dawn_native { namespace d3d12 { protected: TextureBase* GetNextTextureImpl(const TextureDescriptor* descriptor) override; - void OnBeforePresent(TextureBase* texture) override; + MaybeError OnBeforePresent(TextureBase* texture) override; dawn::TextureUsage mTextureUsage; }; diff --git a/src/dawn_native/metal/DeviceMTL.h b/src/dawn_native/metal/DeviceMTL.h index 4eff03f490..5d8c671e17 100644 --- a/src/dawn_native/metal/DeviceMTL.h +++ b/src/dawn_native/metal/DeviceMTL.h @@ -43,7 +43,7 @@ namespace dawn_native { namespace metal { Serial GetCompletedCommandSerial() const final override; Serial GetLastSubmittedCommandSerial() const final override; - void TickImpl() override; + MaybeError TickImpl() override; id GetMTLDevice(); id GetMTLQueue(); diff --git a/src/dawn_native/metal/DeviceMTL.mm b/src/dawn_native/metal/DeviceMTL.mm index a62aad825e..04e9d5f347 100644 --- a/src/dawn_native/metal/DeviceMTL.mm +++ b/src/dawn_native/metal/DeviceMTL.mm @@ -151,7 +151,7 @@ namespace dawn_native { namespace metal { return mLastSubmittedSerial + 1; } - void Device::TickImpl() { + MaybeError Device::TickImpl() { Serial completedSerial = GetCompletedCommandSerial(); mDynamicUploader->Deallocate(completedSerial); @@ -165,6 +165,8 @@ namespace dawn_native { namespace metal { mCompletedSerial++; mLastSubmittedSerial++; } + + return {}; } id Device::GetMTLDevice() { diff --git a/src/dawn_native/metal/SwapChainMTL.h b/src/dawn_native/metal/SwapChainMTL.h index 063add6d7a..5141ea77ee 100644 --- a/src/dawn_native/metal/SwapChainMTL.h +++ b/src/dawn_native/metal/SwapChainMTL.h @@ -28,7 +28,7 @@ namespace dawn_native { namespace metal { protected: TextureBase* GetNextTextureImpl(const TextureDescriptor* descriptor) override; - void OnBeforePresent(TextureBase* texture) override; + MaybeError OnBeforePresent(TextureBase* texture) override; }; }} // namespace dawn_native::metal diff --git a/src/dawn_native/metal/SwapChainMTL.mm b/src/dawn_native/metal/SwapChainMTL.mm index c9552125d7..92458a209f 100644 --- a/src/dawn_native/metal/SwapChainMTL.mm +++ b/src/dawn_native/metal/SwapChainMTL.mm @@ -46,7 +46,8 @@ namespace dawn_native { namespace metal { return new Texture(ToBackend(GetDevice()), descriptor, nativeTexture); } - void SwapChain::OnBeforePresent(TextureBase*) { + MaybeError SwapChain::OnBeforePresent(TextureBase*) { + return {}; } }} // namespace dawn_native::metal diff --git a/src/dawn_native/null/DeviceNull.cpp b/src/dawn_native/null/DeviceNull.cpp index 07c296fb42..08e39b2e60 100644 --- a/src/dawn_native/null/DeviceNull.cpp +++ b/src/dawn_native/null/DeviceNull.cpp @@ -197,8 +197,9 @@ namespace dawn_native { namespace null { return mLastSubmittedSerial + 1; } - void Device::TickImpl() { + MaybeError Device::TickImpl() { SubmitPendingOperations(); + return {}; } void Device::AddPendingOperation(std::unique_ptr operation) { @@ -338,7 +339,8 @@ namespace dawn_native { namespace null { return GetDevice()->CreateTexture(descriptor); } - void SwapChain::OnBeforePresent(TextureBase*) { + MaybeError SwapChain::OnBeforePresent(TextureBase*) { + return {}; } // NativeSwapChainImpl diff --git a/src/dawn_native/null/DeviceNull.h b/src/dawn_native/null/DeviceNull.h index f0664d647a..ef98719223 100644 --- a/src/dawn_native/null/DeviceNull.h +++ b/src/dawn_native/null/DeviceNull.h @@ -92,7 +92,7 @@ namespace dawn_native { namespace null { Serial GetCompletedCommandSerial() const final override; Serial GetLastSubmittedCommandSerial() const final override; Serial GetPendingCommandSerial() const override; - void TickImpl() override; + MaybeError TickImpl() override; void AddPendingOperation(std::unique_ptr operation); void SubmitPendingOperations(); @@ -201,7 +201,7 @@ namespace dawn_native { namespace null { protected: TextureBase* GetNextTextureImpl(const TextureDescriptor* descriptor) override; - void OnBeforePresent(TextureBase*) override; + MaybeError OnBeforePresent(TextureBase*) override; }; class NativeSwapChainImpl { diff --git a/src/dawn_native/opengl/DeviceGL.cpp b/src/dawn_native/opengl/DeviceGL.cpp index 02837221ff..a306e743b5 100644 --- a/src/dawn_native/opengl/DeviceGL.cpp +++ b/src/dawn_native/opengl/DeviceGL.cpp @@ -132,8 +132,9 @@ namespace dawn_native { namespace opengl { return mLastSubmittedSerial + 1; } - void Device::TickImpl() { + MaybeError Device::TickImpl() { CheckPassedFences(); + return {}; } void Device::CheckPassedFences() { diff --git a/src/dawn_native/opengl/DeviceGL.h b/src/dawn_native/opengl/DeviceGL.h index bfbdc272e5..5bafeddfcc 100644 --- a/src/dawn_native/opengl/DeviceGL.h +++ b/src/dawn_native/opengl/DeviceGL.h @@ -53,7 +53,7 @@ namespace dawn_native { namespace opengl { Serial GetCompletedCommandSerial() const final override; Serial GetLastSubmittedCommandSerial() const final override; Serial GetPendingCommandSerial() const override; - void TickImpl() override; + MaybeError TickImpl() override; ResultOrError> CreateStagingBuffer(size_t size) override; MaybeError CopyFromStagingToBuffer(StagingBufferBase* source, diff --git a/src/dawn_native/opengl/SwapChainGL.cpp b/src/dawn_native/opengl/SwapChainGL.cpp index e988bc4dea..bbd707464e 100644 --- a/src/dawn_native/opengl/SwapChainGL.cpp +++ b/src/dawn_native/opengl/SwapChainGL.cpp @@ -44,7 +44,8 @@ namespace dawn_native { namespace opengl { TextureBase::TextureState::OwnedExternal); } - void SwapChain::OnBeforePresent(TextureBase*) { + MaybeError SwapChain::OnBeforePresent(TextureBase*) { + return {}; } }} // namespace dawn_native::opengl diff --git a/src/dawn_native/opengl/SwapChainGL.h b/src/dawn_native/opengl/SwapChainGL.h index 9b7651473a..d4df7d3a09 100644 --- a/src/dawn_native/opengl/SwapChainGL.h +++ b/src/dawn_native/opengl/SwapChainGL.h @@ -30,7 +30,7 @@ namespace dawn_native { namespace opengl { protected: TextureBase* GetNextTextureImpl(const TextureDescriptor* descriptor) override; - void OnBeforePresent(TextureBase* texture) override; + MaybeError OnBeforePresent(TextureBase* texture) override; }; }} // namespace dawn_native::opengl diff --git a/src/dawn_native/vulkan/DeviceVk.cpp b/src/dawn_native/vulkan/DeviceVk.cpp index e05cef432d..e8581aac44 100644 --- a/src/dawn_native/vulkan/DeviceVk.cpp +++ b/src/dawn_native/vulkan/DeviceVk.cpp @@ -209,7 +209,7 @@ namespace dawn_native { namespace vulkan { return mLastSubmittedSerial + 1; } - void Device::TickImpl() { + MaybeError Device::TickImpl() { CheckPassedFences(); RecycleCompletedCommands(); @@ -231,6 +231,8 @@ namespace dawn_native { namespace vulkan { mCompletedSerial++; mLastSubmittedSerial++; } + + return {}; } VkInstance Device::GetVkInstance() const { diff --git a/src/dawn_native/vulkan/DeviceVk.h b/src/dawn_native/vulkan/DeviceVk.h index 529d75a26b..f4e728874c 100644 --- a/src/dawn_native/vulkan/DeviceVk.h +++ b/src/dawn_native/vulkan/DeviceVk.h @@ -83,7 +83,7 @@ namespace dawn_native { namespace vulkan { Serial GetCompletedCommandSerial() const final override; Serial GetLastSubmittedCommandSerial() const final override; - void TickImpl() override; + MaybeError TickImpl() override; ResultOrError> CreateStagingBuffer(size_t size) override; MaybeError CopyFromStagingToBuffer(StagingBufferBase* source, diff --git a/src/dawn_native/vulkan/SwapChainVk.cpp b/src/dawn_native/vulkan/SwapChainVk.cpp index 570760d15a..e9bcaf7760 100644 --- a/src/dawn_native/vulkan/SwapChainVk.cpp +++ b/src/dawn_native/vulkan/SwapChainVk.cpp @@ -46,7 +46,7 @@ namespace dawn_native { namespace vulkan { return new Texture(ToBackend(GetDevice()), descriptor, nativeTexture); } - void SwapChain::OnBeforePresent(TextureBase* texture) { + MaybeError SwapChain::OnBeforePresent(TextureBase* texture) { Device* device = ToBackend(GetDevice()); // Perform the necessary pipeline barriers for the texture to be used with the usage @@ -55,6 +55,8 @@ namespace dawn_native { namespace vulkan { ToBackend(texture)->TransitionUsageNow(recordingContext, mTextureUsage); device->SubmitPendingCommands(); + + return {}; } }} // namespace dawn_native::vulkan diff --git a/src/dawn_native/vulkan/SwapChainVk.h b/src/dawn_native/vulkan/SwapChainVk.h index 190346ceff..2e9db004d2 100644 --- a/src/dawn_native/vulkan/SwapChainVk.h +++ b/src/dawn_native/vulkan/SwapChainVk.h @@ -30,7 +30,7 @@ namespace dawn_native { namespace vulkan { protected: TextureBase* GetNextTextureImpl(const TextureDescriptor* descriptor) override; - void OnBeforePresent(TextureBase* texture) override; + MaybeError OnBeforePresent(TextureBase* texture) override; private: dawn::TextureUsage mTextureUsage;