From 18f63b4e16c7fa2e77cbc5d7641a1a2264e9aa34 Mon Sep 17 00:00:00 2001 From: Dawn Autoroller Date: Wed, 21 Jul 2021 15:41:29 +0000 Subject: [PATCH] Metal: Handle failure to allocate an MTLCommandBuffer This requires restructuring the logic around MTLCommandBuffer allocation so that GetPendingCommandContext is guaranteed to never fail. Logic in the Metal backend is now similar to the Vulkan backend: the MTLCommandBuffer is prepared at device initialization time, or after a submission, such that it is always valid. A new mUsed boolean is added to CommandRecordingContext to say whether any commands have been recording. Previously mCommandBuffer was used for that purpose, but it is now always non-null. Bug: dawn:801 Change-Id: I5dc6747d1e6d538054010cc50533a03a49af921a Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/58720 Auto-Submit: Corentin Wallez Reviewed-by: Stephen White Reviewed-by: Austin Eng Commit-Queue: Corentin Wallez --- src/common/NonCopyable.h | 16 ++++++--- .../metal/CommandRecordingContext.h | 16 ++++----- .../metal/CommandRecordingContext.mm | 36 ++++++++++++------- src/dawn_native/metal/DeviceMTL.h | 2 +- src/dawn_native/metal/DeviceMTL.mm | 30 ++++++++-------- src/dawn_native/metal/QueueMTL.mm | 3 +- .../white_box/MetalAutoreleasePoolTests.mm | 4 +-- 7 files changed, 61 insertions(+), 46 deletions(-) diff --git a/src/common/NonCopyable.h b/src/common/NonCopyable.h index e711f7133a..61f15cabcf 100644 --- a/src/common/NonCopyable.h +++ b/src/common/NonCopyable.h @@ -15,10 +15,7 @@ #ifndef COMMON_NONCOPYABLE_H_ #define COMMON_NONCOPYABLE_H_ -// NonCopyable: -// the base class for the classes that are not copyable. -// - +// A base class to make a class non-copyable. class NonCopyable { protected: constexpr NonCopyable() = default; @@ -29,4 +26,15 @@ class NonCopyable { void operator=(const NonCopyable&) = delete; }; +// A base class to make a class non-movable. +class NonMovable : NonCopyable { + protected: + constexpr NonMovable() = default; + ~NonMovable() = default; + + private: + NonMovable(NonMovable&&) = delete; + void operator=(NonMovable&&) = delete; +}; + #endif diff --git a/src/dawn_native/metal/CommandRecordingContext.h b/src/dawn_native/metal/CommandRecordingContext.h index a0047a831a..5189a53e74 100644 --- a/src/dawn_native/metal/CommandRecordingContext.h +++ b/src/dawn_native/metal/CommandRecordingContext.h @@ -15,6 +15,8 @@ #define DAWNNATIVE_METAL_COMMANDRECORDINGCONTEXT_H_ #include "common/NSRef.h" +#include "common/NonCopyable.h" +#include "dawn_native/Error.h" #import @@ -22,21 +24,16 @@ namespace dawn_native { namespace metal { // This class wraps a MTLCommandBuffer and tracks which Metal encoder is open. // Only one encoder may be open at a time. - class CommandRecordingContext { + class CommandRecordingContext : NonMovable { public: CommandRecordingContext(); - CommandRecordingContext(NSPRef> commands); - - CommandRecordingContext(const CommandRecordingContext& rhs) = delete; - CommandRecordingContext& operator=(const CommandRecordingContext& rhs) = delete; - - CommandRecordingContext(CommandRecordingContext&& rhs); - CommandRecordingContext& operator=(CommandRecordingContext&& rhs); - ~CommandRecordingContext(); id GetCommands(); + void MarkUsed(); + bool WasUsed() const; + MaybeError PrepareNextCommandBuffer(id queue); NSPRef> AcquireCommands(); id EnsureBlit(); @@ -54,6 +51,7 @@ namespace dawn_native { namespace metal { NSPRef> mCompute; NSPRef> mRender; bool mInEncoder = false; + bool mUsed = false; }; }} // namespace dawn_native::metal diff --git a/src/dawn_native/metal/CommandRecordingContext.mm b/src/dawn_native/metal/CommandRecordingContext.mm index decb650676..f07c48c7ae 100644 --- a/src/dawn_native/metal/CommandRecordingContext.mm +++ b/src/dawn_native/metal/CommandRecordingContext.mm @@ -20,19 +20,6 @@ namespace dawn_native { namespace metal { CommandRecordingContext::CommandRecordingContext() = default; - CommandRecordingContext::CommandRecordingContext(NSPRef> commands) - : mCommands(std::move(commands)) { - } - - CommandRecordingContext::CommandRecordingContext(CommandRecordingContext&& rhs) - : mCommands(rhs.AcquireCommands()) { - } - - CommandRecordingContext& CommandRecordingContext::operator=(CommandRecordingContext&& rhs) { - mCommands = rhs.AcquireCommands(); - return *this; - } - CommandRecordingContext::~CommandRecordingContext() { // Commands must be acquired. ASSERT(mCommands == nullptr); @@ -42,6 +29,28 @@ namespace dawn_native { namespace metal { return mCommands.Get(); } + void CommandRecordingContext::MarkUsed() { + mUsed = true; + } + bool CommandRecordingContext::WasUsed() const { + return mUsed; + } + + MaybeError CommandRecordingContext::PrepareNextCommandBuffer(id queue) { + ASSERT(mCommands == nil); + ASSERT(!mUsed); + + // The MTLCommandBuffer will be autoreleased by default. + // The autorelease pool may drain before the command buffer is submitted. Retain so it stays + // alive. + mCommands = AcquireNSPRef([[queue commandBuffer] retain]); + if (mCommands == nil) { + return DAWN_INTERNAL_ERROR("Failed to allocate an MTLCommandBuffer"); + } + + return {}; + } + NSPRef> CommandRecordingContext::AcquireCommands() { // A blit encoder can be left open from WriteBuffer, make sure we close it. if (mCommands != nullptr) { @@ -49,6 +58,7 @@ namespace dawn_native { namespace metal { } ASSERT(!mInEncoder); + mUsed = false; return std::move(mCommands); } diff --git a/src/dawn_native/metal/DeviceMTL.h b/src/dawn_native/metal/DeviceMTL.h index 442741809b..d1881e51e6 100644 --- a/src/dawn_native/metal/DeviceMTL.h +++ b/src/dawn_native/metal/DeviceMTL.h @@ -51,7 +51,7 @@ namespace dawn_native { namespace metal { id GetMTLQueue(); CommandRecordingContext* GetPendingCommandContext(); - void SubmitPendingCommandBuffer(); + MaybeError SubmitPendingCommandBuffer(); Ref CreateTextureWrappingIOSurface(const ExternalImageDescriptor* descriptor, IOSurfaceRef ioSurface, diff --git a/src/dawn_native/metal/DeviceMTL.mm b/src/dawn_native/metal/DeviceMTL.mm index 0359698a25..359dfda4d1 100644 --- a/src/dawn_native/metal/DeviceMTL.mm +++ b/src/dawn_native/metal/DeviceMTL.mm @@ -128,6 +128,11 @@ namespace dawn_native { namespace metal { InitTogglesFromDriver(); mCommandQueue.Acquire([*mMtlDevice newCommandQueue]); + if (mCommandQueue == nil) { + return DAWN_INTERNAL_ERROR("Failed to allocate MTLCommandQueue."); + } + + DAWN_TRY(mCommandContext.PrepareNextCommandBuffer(*mCommandQueue)); if (GetAdapter()->GetSupportedExtensions().IsEnabled(Extension::TimestampQuery)) { // Make a best guess of timestamp period based on device vendor info, and converge it to @@ -281,9 +286,7 @@ namespace dawn_native { namespace metal { } MaybeError Device::TickImpl() { - if (mCommandContext.GetCommands() != nullptr) { - SubmitPendingCommandBuffer(); - } + DAWN_TRY(SubmitPendingCommandBuffer()); // Just run timestamp period calculation when timestamp extension is enabled. if (IsExtensionEnabled(Extension::TimestampQuery)) { @@ -305,20 +308,13 @@ namespace dawn_native { namespace metal { } CommandRecordingContext* Device::GetPendingCommandContext() { - if (mCommandContext.GetCommands() == nullptr) { - TRACE_EVENT0(GetPlatform(), General, "[MTLCommandQueue commandBuffer]"); - // The MTLCommandBuffer will be autoreleased by default. - // The autorelease pool may drain before the command buffer is submitted. Retain so it - // stays alive. - mCommandContext = - CommandRecordingContext(AcquireNSPRef([[*mCommandQueue commandBuffer] retain])); - } + mCommandContext.MarkUsed(); return &mCommandContext; } - void Device::SubmitPendingCommandBuffer() { - if (mCommandContext.GetCommands() == nullptr) { - return; + MaybeError Device::SubmitPendingCommandBuffer() { + if (!mCommandContext.WasUsed()) { + return {}; } IncrementLastSubmittedCommandSerial(); @@ -359,6 +355,8 @@ namespace dawn_native { namespace metal { TRACE_EVENT_ASYNC_BEGIN0(GetPlatform(), GPUWork, "DeviceMTL::SubmitPendingCommandBuffer", uint64_t(pendingSerial)); [*pendingCommands commit]; + + return mCommandContext.PrepareNextCommandBuffer(*mCommandQueue); } ResultOrError> Device::CreateStagingBuffer(size_t size) { @@ -432,7 +430,9 @@ namespace dawn_native { namespace metal { } void Device::WaitForCommandsToBeScheduled() { - SubmitPendingCommandBuffer(); + if (ConsumedError(SubmitPendingCommandBuffer())) { + return; + } // Only lock the object while we take a reference to it, otherwise we could block further // progress if the driver calls the scheduled handler (which also acquires the lock) before diff --git a/src/dawn_native/metal/QueueMTL.mm b/src/dawn_native/metal/QueueMTL.mm index bfa33ac2c1..ad1fad6f0e 100644 --- a/src/dawn_native/metal/QueueMTL.mm +++ b/src/dawn_native/metal/QueueMTL.mm @@ -42,8 +42,7 @@ namespace dawn_native { namespace metal { } TRACE_EVENT_END0(GetDevice()->GetPlatform(), Recording, "CommandBufferMTL::FillCommands"); - device->SubmitPendingCommandBuffer(); - return {}; + return device->SubmitPendingCommandBuffer(); } }} // namespace dawn_native::metal diff --git a/src/tests/white_box/MetalAutoreleasePoolTests.mm b/src/tests/white_box/MetalAutoreleasePoolTests.mm index 47632395ca..1202307e85 100644 --- a/src/tests/white_box/MetalAutoreleasePoolTests.mm +++ b/src/tests/white_box/MetalAutoreleasePoolTests.mm @@ -41,7 +41,7 @@ TEST_P(MetalAutoreleasePoolTests, CommandBufferOutlivesAutorelease) { } // Submitting the command buffer should succeed. - mMtlDevice->SubmitPendingCommandBuffer(); + ASSERT_TRUE(mMtlDevice->SubmitPendingCommandBuffer().IsSuccess()); } // Test that the MTLBlitCommandEncoder owned by the pending command context @@ -56,7 +56,7 @@ TEST_P(MetalAutoreleasePoolTests, EncoderOutlivesAutorelease) { // Submitting the command buffer should succeed. mMtlDevice->GetPendingCommandContext()->EndBlit(); - mMtlDevice->SubmitPendingCommandBuffer(); + ASSERT_TRUE(mMtlDevice->SubmitPendingCommandBuffer().IsSuccess()); } DAWN_INSTANTIATE_TEST(MetalAutoreleasePoolTests, MetalBackend());