Improve ContextLock semantics

`ContextLock` had unoptimal semantics in the form of direct access to the `isFirst` member which wasn't clearly defined, it's now been broken up into function calls `IsFirstUsage` and `OwnsLock` with explicit move semantics and a function for releasing the lock.

Co-authored-by: PixelyIon <pixelyion@protonmail.com>

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

@@ -73,7 +73,7 @@ namespace skyline::gpu {
                     // If the source buffer is GPU dirty we cannot directly copy over its GPU backing contents
 
                     // Only sync back the buffer if it's not attached to the current context, otherwise propagate the GPU dirtiness
-                    if (lock.isFirst) {
+                    if (lock.IsFirstUsage()) {
                         // Perform a GPU -> CPU sync on the source then do a CPU -> GPU sync for the region occupied by the source
                         // This is required since if we were created from a two buffers: one GPU dirty in the current cycle, and one GPU dirty in the previous cycle, if we marked ourselves as CPU dirty here then the GPU dirtiness from the current cycle buffer would be ignored and cause writes to be missed
                         srcBuffer->SynchronizeGuest(true);

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

@@ -24,7 +24,7 @@ namespace skyline::gpu {
         return mutex.try_lock();
     }
 
-    BufferView BufferManager::FindOrCreate(GuestBuffer guestMapping, ContextTag tag, const std::function<void(std::shared_ptr<Buffer>, ContextLock<Buffer> &)> &attachBuffer) {
+    BufferView BufferManager::FindOrCreate(GuestBuffer guestMapping, ContextTag tag, const std::function<void(std::shared_ptr<Buffer>, ContextLock<Buffer> &&)> &attachBuffer) {
         /*
          * We align the buffer to the page boundary to ensure that:
          * 1) Any buffer view has the same alignment guarantees as on the guest, this is required for UBOs, SSBOs and Texel buffers
@@ -65,7 +65,7 @@ namespace skyline::gpu {
         for (auto &overlap : overlaps) {
             ContextLock overlapLock{tag, *overlap};
 
-            if (!overlapLock.isFirst)
+            if (!overlapLock.IsFirstUsage())
                 hasAlreadyAttached = true;
 
             buffers.erase(std::find(buffers.begin(), buffers.end(), overlap));
@@ -101,7 +101,7 @@ namespace skyline::gpu {
 
         if (hasAlreadyAttached)
             // We need to reattach the buffer if any overlapping views were already attached to the current context
-            attachBuffer(newBuffer, newLock);
+            attachBuffer(newBuffer, std::move(newLock));
 
         buffers.insert(std::lower_bound(buffers.begin(), buffers.end(), newBuffer->guest->end().base(), BufferLessThan), newBuffer);
 

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

@@ -66,7 +66,7 @@ namespace skyline::gpu {
          * @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 = {}, const std::function<void(std::shared_ptr<Buffer>, ContextLock<Buffer> &)> &attachBuffer = {});
+        BufferView FindOrCreate(GuestBuffer guestMapping, ContextTag tag = {}, const std::function<void(std::shared_ptr<Buffer>, ContextLock<Buffer> &&)> &attachBuffer = {});
 
         /**
          * @return A dynamically allocated megabuffer which can be used to store buffer modifications allowing them to be replayed in-sequence on the GPU

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

@@ -137,24 +137,25 @@ namespace skyline::gpu::interconnect {
         return didLock;
     }
 
-    void CommandExecutor::AttachLockedBufferView(BufferView &view, ContextLock<BufferView> &lock) {
+    void CommandExecutor::AttachLockedBufferView(BufferView &view, ContextLock<BufferView> &&lock) {
         if (!bufferManagerLock)
             // See AttachTexture(...)
             bufferManagerLock.emplace(gpu.buffer);
 
-        if (lock.isFirst) {
+        if (lock.OwnsLock()) {
+            // Transfer ownership to executor so that the resource will stay locked for the period it is used on the GPU
             attachedBuffers.emplace_back(view->buffer);
-            lock.isFirst = false;
+            lock.Release(); // The executor will handle unlocking the lock so it doesn't need to be handled here
         }
 
         if (!attachedBufferDelegates.contains(view.bufferDelegate))
             attachedBufferDelegates.emplace(view.bufferDelegate);
     }
 
-    void CommandExecutor::AttachLockedBuffer(std::shared_ptr<Buffer> buffer, ContextLock<Buffer> &lock) {
+    void CommandExecutor::AttachLockedBuffer(std::shared_ptr<Buffer> buffer, ContextLock<Buffer> &&lock) {
-        if (lock.isFirst) {
+        if (lock.OwnsLock()) {
             attachedBuffers.emplace_back(std::move(buffer));
-            lock.isFirst = false;
+            lock.Release(); // See AttachLockedBufferView(...)
         }
     }
 

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

@@ -137,7 +137,7 @@ namespace skyline::gpu::interconnect {
          * @note There must be no other external locks on the buffer aside from the supplied lock
          * @note This'll automatically handle syncing of the buffer in the most optimal way possible
          */
-        void AttachLockedBufferView(BufferView &view, ContextLock<BufferView>& lock);
+        void AttachLockedBufferView(BufferView &view, ContextLock<BufferView> &&lock);
 
         /**
          * @brief Attach the lifetime of a buffer object that's already locked to the command buffer
@@ -145,7 +145,7 @@ namespace skyline::gpu::interconnect {
          * @note There must be no other external locks on the buffer aside from the supplied lock
          * @note This'll automatically handle syncing of the buffer in the most optimal way possible
          */
-        void AttachLockedBuffer(std::shared_ptr<Buffer> buffer, ContextLock<Buffer>& lock);
+        void AttachLockedBuffer(std::shared_ptr<Buffer> buffer, ContextLock<Buffer> &&lock);
 
         /**
          * @brief Attach the lifetime of the fence cycle dependency to the command buffer

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

@@ -627,7 +627,7 @@ namespace skyline::gpu::interconnect {
             T Read(CommandExecutor &pExecutor, size_t dstOffset) const {
                 T object;
                 ContextLock lock{pExecutor.tag, view};
-                view.Read(lock.isFirst, []() {
+                view.Read(lock.IsFirstUsage(), []() {
                     // TODO: here we should trigger a SubmitWithFlush, however that doesn't currently work due to Read being called mid-draw and attached objects not handling this case
                     Logger::Warn("GPU dirty buffer reads for attached buffers are unimplemented");
                 }, span<T>(object).template cast<u8>(), dstOffset);
@@ -643,11 +643,11 @@ namespace skyline::gpu::interconnect {
                 auto srcCpuBuf{buf.template cast<u8>()};
 
                 ContextLock lock{pExecutor.tag, view};
-                view.Write(lock.isFirst, pExecutor.cycle, []() {
+                view.Write(lock.IsFirstUsage(), pExecutor.cycle, []() {
                     // TODO: see Read()
                     Logger::Warn("GPU dirty buffer reads for attached buffers are unimplemented");
                 }, [&megaBuffer, &pExecutor, srcCpuBuf, dstOffset, &view = this->view, &lock]() {
-                    pExecutor.AttachLockedBufferView(view, lock);
+                    pExecutor.AttachLockedBufferView(view, std::move(lock));
 
                     auto srcGpuOffset{megaBuffer.Push(srcCpuBuf)};
                     auto srcGpuBuf{megaBuffer.GetBacking()};
@@ -727,8 +727,8 @@ 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 = executor.AcquireBufferManager().FindOrCreate(mappings.front(), executor.tag, [this](std::shared_ptr<Buffer> buffer, ContextLock<Buffer> &lock) {
+                view = executor.AcquireBufferManager().FindOrCreate(mappings.front(), executor.tag, [this](std::shared_ptr<Buffer> buffer, ContextLock<Buffer> &&lock) {
-                    executor.AttachLockedBuffer(buffer, lock);
+                    executor.AttachLockedBuffer(buffer, std::move(lock));
                 });
                 constantBufferCache.Insert(constantBufferSelector.size, constantBufferSelector.iova, *view);
             }
@@ -920,8 +920,8 @@ namespace skyline::gpu::interconnect {
             if (mappings.size() != 1)
                 Logger::Warn("Multiple buffer mappings ({}) are not supported", mappings.size());
 
-            return executor.AcquireBufferManager().FindOrCreate(mappings.front(), executor.tag, [this](std::shared_ptr<Buffer> buffer, ContextLock<Buffer> &lock) {
+            return executor.AcquireBufferManager().FindOrCreate(mappings.front(), executor.tag, [this](std::shared_ptr<Buffer> buffer, ContextLock<Buffer> &&lock) {
-                executor.AttachLockedBuffer(buffer, lock);
+                executor.AttachLockedBuffer(buffer, std::move(lock));
             });
         }
 
@@ -1851,8 +1851,8 @@ namespace skyline::gpu::interconnect {
             if (mappings.size() != 1)
                 Logger::Warn("Multiple buffer mappings ({}) are not supported", mappings.size());
 
-            vertexBuffer.view = executor.AcquireBufferManager().FindOrCreate(mappings.front(), executor.tag, [this](std::shared_ptr<Buffer> buffer, ContextLock<Buffer> &lock) {
+            vertexBuffer.view = executor.AcquireBufferManager().FindOrCreate(mappings.front(), executor.tag, [this](std::shared_ptr<Buffer> buffer, ContextLock<Buffer> &&lock) {
-                executor.AttachLockedBuffer(buffer, lock);
+                executor.AttachLockedBuffer(buffer, std::move(lock));
             });
             return &vertexBuffer;
         }
@@ -2608,8 +2608,8 @@ namespace skyline::gpu::interconnect {
                 Logger::Warn("Multiple buffer mappings ({}) are not supported", mappings.size());
 
             auto mapping{mappings.front()};
-            indexBuffer.view = executor.AcquireBufferManager().FindOrCreate(span<u8>(mapping.data(), size), executor.tag, [this](std::shared_ptr<Buffer> buffer, ContextLock<Buffer> &lock) {
+            indexBuffer.view = executor.AcquireBufferManager().FindOrCreate(span<u8>(mapping.data(), size), executor.tag, [this](std::shared_ptr<Buffer> buffer, ContextLock<Buffer> &&lock) {
-                executor.AttachLockedBuffer(buffer, lock);
+                executor.AttachLockedBuffer(buffer, std::move(lock));
             });
             return indexBuffer.view;
         }

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

@@ -47,27 +47,50 @@ namespace skyline::gpu {
 
     /**
      * @brief A scoped lock specially designed for classes with ContextTag-based locking
-     * @note This will unlock the tag when the scope is exited, **if** it locked the tag in the first place
+     * @note This will unlock the tag when the scope is exited, **if** it locked the tag in the first place and `Release` has not been called
      */
     template<typename T>
     class ContextLock {
       private:
         T &resource;
+        bool ownsLock; //!< If when this ContextLocked initially locked `resource`, it had never been locked for this context before
 
       public:
-        bool isFirst; //!< If this was the first lock for this context
+        ContextLock(ContextTag tag, T &resource) : resource{resource}, ownsLock{resource.LockWithTag(tag)} {}
-
-        ContextLock(ContextTag tag, T &resource) : resource{resource}, isFirst{resource.LockWithTag(tag)} {}
 
         ContextLock(const ContextLock &) = delete;
 
-        ContextLock(ContextLock &&other) noexcept : resource{other.resource}, isFirst{other.isFirst} {
+        ContextLock &operator=(const ContextLock &) = delete;
-            other.isFirst = false;
+
+        ContextLock(ContextLock &&other) noexcept : resource{other.resource}, ownsLock{other.ownsLock} {
+            other.ownsLock = false;
         }
 
         ~ContextLock() {
-            if (isFirst)
+            if (ownsLock)
                 resource.unlock();
         }
+
+        /**
+         * @return If this lock owns the context lock on the resource, the destruction of this lock will cause the resource to be unlocked
+         */
+        constexpr bool OwnsLock() const {
+            return ownsLock;
+        }
+
+        /**
+         * @return If this lock was the first lock on the resource from this context, this effectively means if it was the first usage since prior usages would have to  lock the resource
+         */
+        constexpr bool IsFirstUsage() const {
+            return ownsLock;
+        }
+
+        /**
+         * @brief Releases the ownership of this lock, the destruction of this lock will no longer unlock the underlying resource
+         * @note This will cause IsFirstUsage() to return false regardless of if this is the first usage, this must be handled correctly
+         */
+        constexpr void Release() {
+            ownsLock = false;
+        }
     };
 }

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

@@ -484,8 +484,8 @@ namespace skyline::gpu {
 
         /**
          * @brief Acquires an exclusive lock on the texture for the calling thread
-         * @param tag A tag to associate with the lock, future invocations with the same tag prior to the unlock will acquire the lock without waiting (0 is not a valid tag value and will disable tag behavior)
+         * @param tag A tag to associate with the lock, future invocations with the same tag prior to the unlock will acquire the lock without waiting (A default initialised tag will disable this behaviour)
-         * @return If the lock was acquired by this call rather than having the same tag as the holder
+         * @return If the lock was acquired by this call as opposed to the texture already being locked with the same tag
          * @note All locks using the same tag **must** be from the same thread as it'll only have one corresponding unlock() call
          */
         bool LockWithTag(ContextTag tag);