From 58174f255f54f1250c40de4c232010315c3a2279 Mon Sep 17 00:00:00 2001 From: Billy Laws Date: Sat, 16 Jul 2022 22:30:57 +0530 Subject: [PATCH] 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 --- app/src/main/cpp/skyline/gpu/buffer.cpp | 2 +- .../main/cpp/skyline/gpu/buffer_manager.cpp | 6 +-- app/src/main/cpp/skyline/gpu/buffer_manager.h | 2 +- .../gpu/interconnect/command_executor.cpp | 13 ++++--- .../gpu/interconnect/command_executor.h | 4 +- .../gpu/interconnect/graphics_context.h | 22 +++++------ app/src/main/cpp/skyline/gpu/tag_allocator.h | 37 +++++++++++++++---- .../main/cpp/skyline/gpu/texture/texture.h | 4 +- 8 files changed, 57 insertions(+), 33 deletions(-) diff --git a/app/src/main/cpp/skyline/gpu/buffer.cpp b/app/src/main/cpp/skyline/gpu/buffer.cpp index 3a1b0d5c..02d04316 100644 --- a/app/src/main/cpp/skyline/gpu/buffer.cpp +++ b/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); diff --git a/app/src/main/cpp/skyline/gpu/buffer_manager.cpp b/app/src/main/cpp/skyline/gpu/buffer_manager.cpp index be9c072f..c5e2613a 100644 --- a/app/src/main/cpp/skyline/gpu/buffer_manager.cpp +++ b/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, ContextLock &)> &attachBuffer) { + BufferView BufferManager::FindOrCreate(GuestBuffer guestMapping, ContextTag tag, const std::function, ContextLock &&)> &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); diff --git a/app/src/main/cpp/skyline/gpu/buffer_manager.h b/app/src/main/cpp/skyline/gpu/buffer_manager.h index 3cc7e669..443c5f72 100644 --- a/app/src/main/cpp/skyline/gpu/buffer_manager.h +++ b/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, ContextLock &)> &attachBuffer = {}); + BufferView FindOrCreate(GuestBuffer guestMapping, ContextTag tag = {}, const std::function, ContextLock &&)> &attachBuffer = {}); /** * @return A dynamically allocated megabuffer which can be used to store buffer modifications allowing them to be replayed in-sequence on the GPU diff --git a/app/src/main/cpp/skyline/gpu/interconnect/command_executor.cpp b/app/src/main/cpp/skyline/gpu/interconnect/command_executor.cpp index a6c8a103..ef158128 100644 --- a/app/src/main/cpp/skyline/gpu/interconnect/command_executor.cpp +++ b/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 &lock) { + void CommandExecutor::AttachLockedBufferView(BufferView &view, ContextLock &&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, ContextLock &lock) { - if (lock.isFirst) { + void CommandExecutor::AttachLockedBuffer(std::shared_ptr buffer, ContextLock &&lock) { + if (lock.OwnsLock()) { attachedBuffers.emplace_back(std::move(buffer)); - lock.isFirst = false; + lock.Release(); // See AttachLockedBufferView(...) } } diff --git a/app/src/main/cpp/skyline/gpu/interconnect/command_executor.h b/app/src/main/cpp/skyline/gpu/interconnect/command_executor.h index 6e134a00..65311284 100644 --- a/app/src/main/cpp/skyline/gpu/interconnect/command_executor.h +++ b/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& lock); + void AttachLockedBufferView(BufferView &view, ContextLock &&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, ContextLock& lock); + void AttachLockedBuffer(std::shared_ptr buffer, ContextLock &&lock); /** * @brief Attach the lifetime of the fence cycle dependency to the command buffer diff --git a/app/src/main/cpp/skyline/gpu/interconnect/graphics_context.h b/app/src/main/cpp/skyline/gpu/interconnect/graphics_context.h index f78e0c79..5d59146c 100644 --- a/app/src/main/cpp/skyline/gpu/interconnect/graphics_context.h +++ b/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(object).template cast(), dstOffset); @@ -643,11 +643,11 @@ namespace skyline::gpu::interconnect { auto srcCpuBuf{buf.template cast()}; 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, ContextLock &lock) { - executor.AttachLockedBuffer(buffer, lock); + view = executor.AcquireBufferManager().FindOrCreate(mappings.front(), executor.tag, [this](std::shared_ptr buffer, ContextLock &&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, ContextLock &lock) { - executor.AttachLockedBuffer(buffer, lock); + return executor.AcquireBufferManager().FindOrCreate(mappings.front(), executor.tag, [this](std::shared_ptr buffer, ContextLock &&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, ContextLock &lock) { - executor.AttachLockedBuffer(buffer, lock); + vertexBuffer.view = executor.AcquireBufferManager().FindOrCreate(mappings.front(), executor.tag, [this](std::shared_ptr buffer, ContextLock &&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(mapping.data(), size), executor.tag, [this](std::shared_ptr buffer, ContextLock &lock) { - executor.AttachLockedBuffer(buffer, lock); + indexBuffer.view = executor.AcquireBufferManager().FindOrCreate(span(mapping.data(), size), executor.tag, [this](std::shared_ptr buffer, ContextLock &&lock) { + executor.AttachLockedBuffer(buffer, std::move(lock)); }); return indexBuffer.view; } diff --git a/app/src/main/cpp/skyline/gpu/tag_allocator.h b/app/src/main/cpp/skyline/gpu/tag_allocator.h index 159ffe3b..91b9e7e5 100644 --- a/app/src/main/cpp/skyline/gpu/tag_allocator.h +++ b/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 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}, isFirst{resource.LockWithTag(tag)} {} + ContextLock(ContextTag tag, T &resource) : resource{resource}, ownsLock{resource.LockWithTag(tag)} {} ContextLock(const ContextLock &) = delete; - ContextLock(ContextLock &&other) noexcept : resource{other.resource}, isFirst{other.isFirst} { - other.isFirst = false; + ContextLock &operator=(const ContextLock &) = delete; + + 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; + } }; } diff --git a/app/src/main/cpp/skyline/gpu/texture/texture.h b/app/src/main/cpp/skyline/gpu/texture/texture.h index d08472dd..7a213375 100644 --- a/app/src/main/cpp/skyline/gpu/texture/texture.h +++ b/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) - * @return If the lock was acquired by this call rather than having the same tag as the holder + * @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 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);