Lock TextureManager/BufferManager during submission

Multiple threads concurrently accessing the `TextureManager`/`BufferManager` (Referred to as "resource managers") has a potential deadlock with a resource being locked while acquiring the resource manager lock while the thread owning it tries to acquire a lock on the resource resulting in a deadlock.

This has been fixed with locking of resource manager now being externally handled which ensures it can be locked prior to locking any resources, `CommandExecutor` provides accessors for retrieving the resource manager which automatically handles locking aside doing so on attachment of resources.

app/src/main/cpp/skyline/gpu/buffer_manager.cpp

@@ -12,6 +12,18 @@ namespace skyline::gpu {
         return it->guest->begin().base() < pointer;
     }
 
+    void BufferManager::lock() {
+        mutex.lock();
+    }
+
+    void BufferManager::unlock() {
+        mutex.unlock();
+    }
+
+    bool BufferManager::try_lock() {
+        return mutex.try_lock();
+    }
+
     BufferView BufferManager::FindOrCreate(GuestBuffer guestMapping, ContextTag tag) {
         /*
          * We align the buffer to the page boundary to ensure that:
@@ -22,8 +34,6 @@ namespace skyline::gpu {
         vk::DeviceSize offset{static_cast<size_t>(guestMapping.begin().base() - alignedStart)}, size{guestMapping.size()};
         guestMapping = span<u8>{alignedStart, alignedEnd};
 
-        std::scoped_lock lock(mutex);
-
         // Lookup for any buffers overlapping with the supplied guest mapping
         boost::container::small_vector<std::shared_ptr<Buffer>, 4> overlaps;
         for (auto entryIt{std::lower_bound(buffers.begin(), buffers.end(), guestMapping.end().base(), BufferLessThan)}; entryIt != buffers.begin() && (*--entryIt)->guest->begin() <= guestMapping.end();)
@@ -89,7 +99,9 @@ namespace skyline::gpu {
         return newBuffer->GetView(static_cast<vk::DeviceSize>(guestMapping.begin() - newBuffer->guest->begin()) + offset, size);
     }
 
-    BufferManager::MegaBufferSlot::MegaBufferSlot(GPU &gpu) : backing(gpu.memory.AllocateBuffer(Size)) {}
+    constexpr static vk::DeviceSize MegaBufferSize{100 * 1024 * 1024}; //!< Size in bytes of the megabuffer (100MiB)
+
+    BufferManager::MegaBufferSlot::MegaBufferSlot(GPU &gpu) : backing(gpu.memory.AllocateBuffer(MegaBufferSize)) {}
 
     MegaBuffer::MegaBuffer(BufferManager::MegaBufferSlot &slot) : slot{&slot}, freeRegion{slot.backing.subspan(PAGE_SIZE)} {}
 
@@ -135,7 +147,7 @@ namespace skyline::gpu {
     }
 
     MegaBuffer BufferManager::AcquireMegaBuffer(const std::shared_ptr<FenceCycle> &cycle) {
-        std::scoped_lock lock{mutex};
+        std::scoped_lock lock{megaBufferMutex};
 
         for (auto &slot : megaBuffers) {
             if (!slot.active.test_and_set(std::memory_order_acq_rel)) {

app/src/main/cpp/skyline/gpu/buffer_manager.h

@@ -17,6 +17,8 @@ namespace skyline::gpu {
         std::mutex mutex; //!< Synchronizes access to the buffer mappings
         std::vector<std::shared_ptr<Buffer>> buffers; //!< A sorted vector of all buffer mappings
 
+        std::mutex megaBufferMutex; //!< Synchronizes access to the allocated megabuffers
+
         friend class MegaBuffer;
 
         /**
@@ -26,7 +28,6 @@ namespace skyline::gpu {
             std::atomic_flag active{true}; //!< If the megabuffer is currently being utilized, we want to construct a buffer as active
             std::shared_ptr<FenceCycle> cycle; //!< The latest cycle on the fence, all waits must be performed through this
 
-            constexpr static vk::DeviceSize Size{100 * 1024 * 1024}; //!< Size in bytes of the megabuffer (100MiB)
             memory::Buffer backing; //!< The GPU buffer as the backing storage for the megabuffer
 
             MegaBufferSlot(GPU &gpu);
@@ -42,14 +43,34 @@ namespace skyline::gpu {
 
         BufferManager(GPU &gpu);
 
+        /**
+         * @brief Acquires an exclusive lock on the texture for the calling thread
+         * @note Naming is in accordance to the BasicLockable named requirement
+         */
+        void lock();
+
+        /**
+         * @brief Relinquishes an existing lock on the texture by the calling thread
+         * @note Naming is in accordance to the BasicLockable named requirement
+         */
+        void unlock();
+
+        /**
+         * @brief Attempts to acquire an exclusive lock but returns immediately if it's captured by another thread
+         * @note Naming is in accordance to the Lockable named requirement
+         */
+        bool try_lock();
+
         /**
          * @return A pre-existing or newly created Buffer object which covers the supplied mappings
+         * @note The buffer manager **must** be locked prior to calling this
          */
         BufferView FindOrCreate(GuestBuffer guestMapping, ContextTag tag = {});
 
         /**
          * @return A dynamically allocated megabuffer which can be used to store buffer modifications allowing them to be replayed in-sequence on the GPU
          * @note This object **must** be destroyed to be reclaimed by the manager and prevent a memory leak
+         * @note The buffer manager **doesn't** need to be locked prior to calling this
          */
         MegaBuffer AcquireMegaBuffer(const std::shared_ptr<FenceCycle> &cycle);
     };

app/src/main/cpp/skyline/gpu/interconnect/blit_context.h

@@ -118,11 +118,12 @@ namespace skyline::gpu::interconnect {
             auto srcGuestTexture{GetGuestTexture(srcSurface)};
             auto dstGuestTexture{GetGuestTexture(dstSurface)};
 
-            auto srcTextureView{gpu.texture.FindOrCreate(srcGuestTexture, executor.tag)};
-            executor.AttachTexture(&*srcTextureView);
+            auto &textureManager{executor.AcquireTextureManager()};
+            auto srcTextureView{textureManager.FindOrCreate(srcGuestTexture, executor.tag)};
+            executor.AttachTexture(srcTextureView.get());
 
-            auto dstTextureView{gpu.texture.FindOrCreate(dstGuestTexture, executor.tag)};
-            executor.AttachTexture(&*dstTextureView);
+            auto dstTextureView{textureManager.FindOrCreate(dstGuestTexture, executor.tag)};
+            executor.AttachTexture(dstTextureView.get());
 
             auto getSubresourceLayers{[](const vk::ImageSubresourceRange &range, vk::ImageAspectFlags aspect) {
                 return vk::ImageSubresourceLayers{

app/src/main/cpp/skyline/gpu/interconnect/command_executor.cpp

@@ -11,6 +11,18 @@ namespace skyline::gpu::interconnect {
         cycle->Cancel();
     }
 
+    TextureManager &CommandExecutor::AcquireTextureManager() {
+        if (!textureManagerLock)
+            textureManagerLock.emplace(gpu.texture);
+        return gpu.texture;
+    }
+
+    BufferManager &CommandExecutor::AcquireBufferManager() {
+        if (!bufferManagerLock)
+            bufferManagerLock.emplace(gpu.buffer);
+        return gpu.buffer;
+    }
+
     bool CommandExecutor::CreateRenderPassWithSubpass(vk::Rect2D renderArea, span<TextureView *> inputAttachments, span<TextureView *> colorAttachments, TextureView *depthStencilAttachment) {
         auto addSubpass{[&] {
             renderPass->AddSubpass(inputAttachments, colorAttachments, depthStencilAttachment, gpu);
@@ -85,6 +97,10 @@ namespace skyline::gpu::interconnect {
     }
 
     bool CommandExecutor::AttachTexture(TextureView *view) {
+        if (!textureManagerLock)
+            // Avoids a potential deadlock with this resource being locked while acquiring the TextureManager lock while the thread owning it tries to acquire a lock on this texture
+            textureManagerLock.emplace(gpu.texture);
+
         bool didLock{view->LockWithTag(tag)};
         if (didLock)
             attachedTextures.emplace_back(view->texture);
@@ -105,6 +121,10 @@ namespace skyline::gpu::interconnect {
     }
 
     bool CommandExecutor::AttachBuffer(BufferView &view) {
+        if (!bufferManagerLock)
+            // See AttachTexture(...)
+            bufferManagerLock.emplace(gpu.buffer);
+
         bool didLock{view->LockWithTag(tag)};
         if (didLock)
             attachedBuffers.emplace_back(view->buffer);

app/src/main/cpp/skyline/gpu/interconnect/command_executor.h

@@ -20,6 +20,9 @@ namespace skyline::gpu::interconnect {
         node::RenderPassNode *renderPass{};
         size_t subpassCount{}; //!< The number of subpasses in the current render pass
 
+        std::optional<std::scoped_lock<TextureManager>> textureManagerLock; //!< The lock on the texture manager, this is locked for the duration of the command execution from the first usage inside an execution to the submission
+        std::optional<std::scoped_lock<BufferManager>> bufferManagerLock; //!< The lock on the buffer manager, this is locked for the duration of the command execution from the first usage inside an execution to the submission
+
         /**
          * @brief A wrapper of a Texture object that has been locked beforehand and must be unlocked afterwards
          */
@@ -95,6 +98,12 @@ namespace skyline::gpu::interconnect {
 
         ~CommandExecutor();
 
+        /**
+         * @return A reference to an instance of the Texture Manager which will be locked till execution
+         * @note Any access to the texture manager while recording commands **must** be done via this
+         */
+        TextureManager &AcquireTextureManager();
+
         /**
          * @brief Attach the lifetime of the texture to the command buffer
          * @return If this is the first usage of the backing of this resource within this execution
@@ -103,6 +112,12 @@ namespace skyline::gpu::interconnect {
          */
         bool AttachTexture(TextureView *view);
 
+        /**
+         * @return A reference to an instance of the Buffer Manager which will be locked till execution
+         * @note Any access to the buffer manager while recording commands **must** be done via this
+         */
+        BufferManager &AcquireBufferManager();
+
         /**
          * @brief Attach the lifetime of a buffer to the command buffer
          * @return If this is the first usage of the backing of this resource within this execution

app/src/main/cpp/skyline/gpu/interconnect/graphics_context.h

@@ -400,7 +400,7 @@ namespace skyline::gpu::interconnect {
                 return vk::ImageViewType::e2D;
             }();
 
-            renderTarget.view = gpu.texture.FindOrCreate(renderTarget.guest, executor.tag);
+            renderTarget.view = executor.AcquireTextureManager().FindOrCreate(renderTarget.guest, executor.tag);
             return renderTarget.view.get();
         }
 
@@ -725,7 +725,7 @@ namespace skyline::gpu::interconnect {
             auto view{constantBufferCache.Lookup(constantBufferSelector.size, constantBufferSelector.iova)};
             if (!view) {
                 auto mappings{channelCtx.asCtx->gmmu.TranslateRange(constantBufferSelector.iova, constantBufferSelector.size)};
-                view = gpu.buffer.FindOrCreate(mappings.front(), executor.tag);
+                view = executor.AcquireBufferManager().FindOrCreate(mappings.front(), executor.tag);
                 constantBufferCache.Insert(constantBufferSelector.size, constantBufferSelector.iova, *view);
             }
 
@@ -916,7 +916,7 @@ namespace skyline::gpu::interconnect {
             if (mappings.size() != 1)
                 Logger::Warn("Multiple buffer mappings ({}) are not supported", mappings.size());
 
-            return gpu.buffer.FindOrCreate(mappings.front(), executor.tag);
+            return executor.AcquireBufferManager().FindOrCreate(mappings.front(), executor.tag);
         }
 
         /**
@@ -1845,7 +1845,7 @@ namespace skyline::gpu::interconnect {
             if (mappings.size() != 1)
                 Logger::Warn("Multiple buffer mappings ({}) are not supported", mappings.size());
 
-            vertexBuffer.view = gpu.buffer.FindOrCreate(mappings.front(), executor.tag);
+            vertexBuffer.view = executor.AcquireBufferManager().FindOrCreate(mappings.front(), executor.tag);
             return &vertexBuffer;
         }
 
@@ -2329,7 +2329,7 @@ namespace skyline::gpu::interconnect {
                 return textureView;
             }
 
-            auto textureView{gpu.texture.FindOrCreate(poolTexture.guest, executor.tag)};
+            auto textureView{executor.AcquireTextureManager().FindOrCreate(poolTexture.guest, executor.tag)};
             poolTexture.view = textureView;
             return textureView;
         }
@@ -2600,7 +2600,7 @@ namespace skyline::gpu::interconnect {
                 Logger::Warn("Multiple buffer mappings ({}) are not supported", mappings.size());
 
             auto mapping{mappings.front()};
-            indexBuffer.view = gpu.buffer.FindOrCreate(span<u8>(mapping.data(), size), executor.tag);
+            indexBuffer.view = executor.AcquireBufferManager().FindOrCreate(span<u8>(mapping.data(), size), executor.tag);
             return indexBuffer.view;
         }
 

app/src/main/cpp/skyline/gpu/texture_manager.cpp

@@ -6,6 +6,18 @@
 namespace skyline::gpu {
     TextureManager::TextureManager(GPU &gpu) : gpu(gpu) {}
 
+    void TextureManager::lock() {
+        mutex.lock();
+    }
+
+    void TextureManager::unlock() {
+        mutex.unlock();
+    }
+
+    bool TextureManager::try_lock() {
+        return mutex.try_lock();
+    }
+
     std::shared_ptr<TextureView> TextureManager::FindOrCreate(const GuestTexture &guestTexture, ContextTag tag) {
         auto guestMapping{guestTexture.mappings.front()};
 
@@ -23,7 +35,6 @@ namespace skyline::gpu {
          * 5) Create a new texture and insert it in the map then return it
          */
 
-        std::scoped_lock lock(mutex);
         std::shared_ptr<Texture> match{};
         auto mappingEnd{std::upper_bound(textures.begin(), textures.end(), guestMapping)}, hostMapping{mappingEnd};
         while (hostMapping != textures.begin() && (--hostMapping)->end() > guestMapping.begin()) {

app/src/main/cpp/skyline/gpu/texture_manager.h

@@ -32,8 +32,27 @@ namespace skyline::gpu {
       public:
         TextureManager(GPU &gpu);
 
+        /**
+         * @brief Acquires an exclusive lock on the texture for the calling thread
+         * @note Naming is in accordance to the BasicLockable named requirement
+         */
+        void lock();
+
+        /**
+         * @brief Relinquishes an existing lock on the texture by the calling thread
+         * @note Naming is in accordance to the BasicLockable named requirement
+         */
+        void unlock();
+
+        /**
+         * @brief Attempts to acquire an exclusive lock but returns immediately if it's captured by another thread
+         * @note Naming is in accordance to the Lockable named requirement
+         */
+        bool try_lock();
+
         /**
          * @return A pre-existing or newly created Texture object which matches the specified criteria
+         * @note The texture manager **must** be locked prior to calling this
          */
         std::shared_ptr<TextureView> FindOrCreate(const GuestTexture &guestTexture, ContextTag tag = {});
     };

app/src/main/cpp/skyline/services/hosbinder/GraphicBufferProducer.cpp

@@ -347,6 +347,8 @@ namespace skyline::service::hosbinder {
             gpu::texture::Dimensions dimensions(surface.width, surface.height);
             gpu::GuestTexture guestTexture(span<u8>{}, dimensions, format, tileConfig, vk::ImageViewType::e2D);
             guestTexture.mappings[0] = span<u8>(nvMapHandleObj->GetPointer() + surface.offset, guestTexture.GetLayerStride());
+
+            std::scoped_lock textureLock{state.gpu->texture};
             buffer.texture = state.gpu->texture.FindOrCreate(guestTexture)->texture;
         }