From ba8d429fcb75d02ee22872d2f1794198e67d8e5a Mon Sep 17 00:00:00 2001 From: Ben Doherty Date: Tue, 27 Aug 2024 10:49:35 -0700 Subject: [PATCH] Metal: annotate memory allocation failures with buffer tag (#8082) --- filament/backend/src/metal/MetalBuffer.h | 2 ++ filament/backend/src/metal/MetalBuffer.mm | 4 +-- filament/backend/src/metal/MetalDriver.h | 5 ++-- filament/backend/src/metal/MetalDriver.mm | 32 ++++++++++++++++++---- filament/backend/src/metal/MetalHandles.mm | 8 ------ 5 files changed, 33 insertions(+), 18 deletions(-) diff --git a/filament/backend/src/metal/MetalBuffer.h b/filament/backend/src/metal/MetalBuffer.h index bb3de6d27c9..265c2896238 100644 --- a/filament/backend/src/metal/MetalBuffer.h +++ b/filament/backend/src/metal/MetalBuffer.h @@ -160,6 +160,8 @@ class MetalBuffer { size_t size, bool forceGpuBuffer = false); ~MetalBuffer(); + [[nodiscard]] bool wasAllocationSuccessful() const noexcept { return mBuffer || mCpuBuffer; } + MetalBuffer(const MetalBuffer& rhs) = delete; MetalBuffer& operator=(const MetalBuffer& rhs) = delete; diff --git a/filament/backend/src/metal/MetalBuffer.mm b/filament/backend/src/metal/MetalBuffer.mm index 6070cf47fb3..a3eb80fa12d 100644 --- a/filament/backend/src/metal/MetalBuffer.mm +++ b/filament/backend/src/metal/MetalBuffer.mm @@ -53,8 +53,8 @@ mBuffer = { [context.device newBufferWithLength:size options:MTLResourceStorageModePrivate], TrackedMetalBuffer::Type::GENERIC }; } - FILAMENT_CHECK_POSTCONDITION(mBuffer) - << "Could not allocate Metal buffer of size " << size << "."; + // mBuffer might fail to be allocated. Clients can check for this by calling + // wasAllocationSuccessful(). } MetalBuffer::~MetalBuffer() { diff --git a/filament/backend/src/metal/MetalDriver.h b/filament/backend/src/metal/MetalDriver.h index e38b65b3755..6b9eac013e9 100644 --- a/filament/backend/src/metal/MetalDriver.h +++ b/filament/backend/src/metal/MetalDriver.h @@ -57,7 +57,6 @@ class MetalDriver final : public DriverBase { public: static Driver* create(MetalPlatform* platform, const Platform::DriverConfig& driverConfig); - void runAtNextTick(const std::function& fn) noexcept; private: @@ -73,10 +72,12 @@ class MetalDriver final : public DriverBase { /* * Tasks run regularly on the driver thread. + * Not thread-safe; tasks are run from the driver thead and must be enqueued from the driver + * thread. */ + void runAtNextTick(const std::function& fn) noexcept; void executeTickOps() noexcept; std::vector> mTickOps; - std::mutex mTickOpsLock; /* * Driver interface diff --git a/filament/backend/src/metal/MetalDriver.mm b/filament/backend/src/metal/MetalDriver.mm index b7d0a31deaa..b85d616c773 100644 --- a/filament/backend/src/metal/MetalDriver.mm +++ b/filament/backend/src/metal/MetalDriver.mm @@ -320,17 +320,40 @@ uint32_t vertexCount, Handle vbih) { MetalVertexBufferInfo const* const vbi = handle_cast(vbih); construct_handle(vbh, *mContext, vertexCount, vbi->bufferCount, vbih); + // No actual GPU memory is allocated here, so no need to check for allocation success. } void MetalDriver::createIndexBufferR(Handle ibh, ElementType elementType, uint32_t indexCount, BufferUsage usage) { - auto elementSize = (uint8_t) getElementTypeSize(elementType); - construct_handle(ibh, *mContext, usage, elementSize, indexCount); + auto elementSize = (uint8_t)getElementTypeSize(elementType); + auto* indexBuffer = + construct_handle(ibh, *mContext, usage, elementSize, indexCount); + auto& buffer = indexBuffer->buffer; + // If the allocation was not successful, postpone the error message until the next tick, to give + // Filament a chance to call setDebugTag on the handle; this way we get a nicer error message. + if (UTILS_UNLIKELY(!buffer.wasAllocationSuccessful())) { + const size_t byteCount = buffer.getSize(); + runAtNextTick([byteCount, this, ibh]() { + FILAMENT_CHECK_POSTCONDITION(false) + << "Could not allocate Metal index buffer of size " << byteCount + << ", tag=" << mHandleAllocator.getHandleTag(ibh.getId()).c_str_safe(); + }); + } } void MetalDriver::createBufferObjectR(Handle boh, uint32_t byteCount, BufferObjectBinding bindingType, BufferUsage usage) { - construct_handle(boh, *mContext, bindingType, usage, byteCount); + auto* bufferObject = + construct_handle(boh, *mContext, bindingType, usage, byteCount); + // If the allocation was not successful, postpone the error message until the next tick, to give + // Filament a chance to call setDebugTag on the handle; this way we get a nicer error message. + if (UTILS_UNLIKELY(!bufferObject->getBuffer()->wasAllocationSuccessful())) { + runAtNextTick([byteCount, this, boh]() { + FILAMENT_CHECK_POSTCONDITION(false) + << "Could not allocate Metal buffer of size " << byteCount + << ", tag=" << mHandleAllocator.getHandleTag(boh.getId()).c_str_safe(); + }); + } } void MetalDriver::createTextureR(Handle th, SamplerType target, uint8_t levels, @@ -2066,15 +2089,12 @@ } void MetalDriver::runAtNextTick(const std::function& fn) noexcept { - std::lock_guard const lock(mTickOpsLock); mTickOps.push_back(fn); } void MetalDriver::executeTickOps() noexcept { std::vector> ops; - mTickOpsLock.lock(); std::swap(ops, mTickOps); - mTickOpsLock.unlock(); for (const auto& f : ops) { f(); } diff --git a/filament/backend/src/metal/MetalHandles.mm b/filament/backend/src/metal/MetalHandles.mm index 9f3e8a502e8..938d4569d36 100644 --- a/filament/backend/src/metal/MetalHandles.mm +++ b/filament/backend/src/metal/MetalHandles.mm @@ -257,10 +257,6 @@ static inline MTLTextureUsage getMetalTextureUsage(TextureUsage usage) { } } -#ifndef FILAMENT_RELEASE_PRESENT_DRAWABLE_MAIN_THREAD -#define FILAMENT_RELEASE_PRESENT_DRAWABLE_MAIN_THREAD 1 -#endif - class PresentDrawableData { public: PresentDrawableData() = delete; @@ -279,14 +275,10 @@ static void maybePresentAndDestroyAsync(PresentDrawableData* that, bool shouldPr [that->mDrawable present]; } -#if FILAMENT_RELEASE_PRESENT_DRAWABLE_MAIN_THREAD == 1 // mDrawable is acquired on the driver thread. Typically, we would release this object on // the same thread, but after receiving consistent crash reports from within // [CAMetalDrawable dealloc], we suspect this object requires releasing on the main thread. dispatch_async(dispatch_get_main_queue(), ^{ cleanupAndDestroy(that); }); -#else - that->mDriver->runAtNextTick([that]() { cleanupAndDestroy(that); }); -#endif } private: