From 3ac5ed8c0658dc7a509c8dfac817357133164383 Mon Sep 17 00:00:00 2001 From: PixelyIon Date: Sat, 16 Jul 2022 20:12:09 +0530 Subject: [PATCH] Attach coalesced `Buffer` if any source `Buffer` is attached A buffer that's attached to a context could be coalesced into a larger buffer which isn't attached, this would break as it wouldn't keep the buffer alive till the end of the associated context. To fix this if any source buffers are attached then the resulting coalesced buffer is also attached now. --- .../main/cpp/skyline/gpu/buffer_manager.cpp | 11 ++++++++++- app/src/main/cpp/skyline/gpu/buffer_manager.h | 3 ++- .../gpu/interconnect/command_executor.cpp | 9 ++++++++- .../gpu/interconnect/command_executor.h | 15 +++++++++++---- .../gpu/interconnect/graphics_context.h | 18 +++++++++++++----- 5 files changed, 44 insertions(+), 12 deletions(-) diff --git a/app/src/main/cpp/skyline/gpu/buffer_manager.cpp b/app/src/main/cpp/skyline/gpu/buffer_manager.cpp index fd181c08..be9c072f 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) { + 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 @@ -60,9 +60,14 @@ namespace skyline::gpu { } auto newBuffer{std::make_shared(gpu, span{lowestAddress, highestAddress}, tag, overlaps)}; + ContextLock newLock{tag, *newBuffer}; + bool hasAlreadyAttached{}; //!< If any overlapping view was already attached to the current context for (auto &overlap : overlaps) { ContextLock overlapLock{tag, *overlap}; + if (!overlapLock.isFirst) + hasAlreadyAttached = true; + buffers.erase(std::find(buffers.begin(), buffers.end(), overlap)); // Transfer all views from the overlapping buffer to the new buffer with the new buffer and updated offset, ensuring pointer stability @@ -94,6 +99,10 @@ namespace skyline::gpu { newBuffer->delegates.splice(newBuffer->delegates.end(), overlap->delegates); } + if (hasAlreadyAttached) + // We need to reattach the buffer if any overlapping views were already attached to the current context + attachBuffer(newBuffer, newLock); + buffers.insert(std::lower_bound(buffers.begin(), buffers.end(), newBuffer->guest->end().base(), BufferLessThan), newBuffer); return newBuffer->GetView(static_cast(guestMapping.begin() - newBuffer->guest->begin()) + offset, size); diff --git a/app/src/main/cpp/skyline/gpu/buffer_manager.h b/app/src/main/cpp/skyline/gpu/buffer_manager.h index 50ccc210..3cc7e669 100644 --- a/app/src/main/cpp/skyline/gpu/buffer_manager.h +++ b/app/src/main/cpp/skyline/gpu/buffer_manager.h @@ -62,10 +62,11 @@ namespace skyline::gpu { bool try_lock(); /** + * @param attachBuffer A function that attaches the buffer to the current context, this'll be called when coalesced buffers are merged into the current buffer * @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 = {}); + 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 5116a609..a6c8a103 100644 --- a/app/src/main/cpp/skyline/gpu/interconnect/command_executor.cpp +++ b/app/src/main/cpp/skyline/gpu/interconnect/command_executor.cpp @@ -137,7 +137,7 @@ namespace skyline::gpu::interconnect { return didLock; } - void CommandExecutor::AttachLockedBuffer(BufferView &view, ContextLock &lock) { + void CommandExecutor::AttachLockedBufferView(BufferView &view, ContextLock &lock) { if (!bufferManagerLock) // See AttachTexture(...) bufferManagerLock.emplace(gpu.buffer); @@ -151,6 +151,13 @@ namespace skyline::gpu::interconnect { attachedBufferDelegates.emplace(view.bufferDelegate); } + void CommandExecutor::AttachLockedBuffer(std::shared_ptr buffer, ContextLock &lock) { + if (lock.isFirst) { + attachedBuffers.emplace_back(std::move(buffer)); + lock.isFirst = false; + } + } + void CommandExecutor::AttachDependency(const std::shared_ptr &dependency) { cycle->AttachObject(dependency); } 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 23a3745f..6e134a00 100644 --- a/app/src/main/cpp/skyline/gpu/interconnect/command_executor.h +++ b/app/src/main/cpp/skyline/gpu/interconnect/command_executor.h @@ -124,7 +124,7 @@ namespace skyline::gpu::interconnect { BufferManager &AcquireBufferManager(); /** - * @brief Attach the lifetime of a buffer to the command buffer + * @brief Attach the lifetime of a buffer view to the command buffer * @return If this is the first usage of the backing of this resource within this execution * @note The supplied buffer will be locked automatically until the command buffer is submitted and must **not** be locked by the caller * @note This'll automatically handle syncing of the buffer in the most optimal way possible @@ -132,13 +132,20 @@ namespace skyline::gpu::interconnect { bool AttachBuffer(BufferView &view); /** - * @brief Attach the lifetime of a buffer that's already locked to the command buffer - * @return If this is the first usage of the backing of this resource within this execution + * @brief Attach the lifetime of a buffer view that's already locked to the command buffer * @note The supplied buffer **must** be locked with the executor's tag * @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(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 + * @note The supplied buffer **must** be locked with the executor's tag + * @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); /** * @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 323226f3..f78e0c79 100644 --- a/app/src/main/cpp/skyline/gpu/interconnect/graphics_context.h +++ b/app/src/main/cpp/skyline/gpu/interconnect/graphics_context.h @@ -647,7 +647,7 @@ namespace skyline::gpu::interconnect { // TODO: see Read() Logger::Warn("GPU dirty buffer reads for attached buffers are unimplemented"); }, [&megaBuffer, &pExecutor, srcCpuBuf, dstOffset, &view = this->view, &lock]() { - pExecutor.AttachLockedBuffer(view, lock); + pExecutor.AttachLockedBufferView(view, lock); auto srcGpuOffset{megaBuffer.Push(srcCpuBuf)}; auto srcGpuBuf{megaBuffer.GetBacking()}; @@ -727,7 +727,9 @@ 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); + view = executor.AcquireBufferManager().FindOrCreate(mappings.front(), executor.tag, [this](std::shared_ptr buffer, ContextLock &lock) { + executor.AttachLockedBuffer(buffer, lock); + }); constantBufferCache.Insert(constantBufferSelector.size, constantBufferSelector.iova, *view); } @@ -918,7 +920,9 @@ 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); + return executor.AcquireBufferManager().FindOrCreate(mappings.front(), executor.tag, [this](std::shared_ptr buffer, ContextLock &lock) { + executor.AttachLockedBuffer(buffer, lock); + }); } /** @@ -1847,7 +1851,9 @@ 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); + vertexBuffer.view = executor.AcquireBufferManager().FindOrCreate(mappings.front(), executor.tag, [this](std::shared_ptr buffer, ContextLock &lock) { + executor.AttachLockedBuffer(buffer, lock); + }); return &vertexBuffer; } @@ -2602,7 +2608,9 @@ 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); + indexBuffer.view = executor.AcquireBufferManager().FindOrCreate(span(mapping.data(), size), executor.tag, [this](std::shared_ptr buffer, ContextLock &lock) { + executor.AttachLockedBuffer(buffer, lock); + }); return indexBuffer.view; }