From b0910e7b1aeb5b14ca61cff3861d4fd42488b121 Mon Sep 17 00:00:00 2001 From: PixelyIon Date: Sat, 2 Jul 2022 19:42:26 +0530 Subject: [PATCH] Avoid locking `Texture`/`Buffer` in trap handler We generally don't need to lock the `Texture`/`Buffer` in the trap handler, this is particularly problematic now as we hold the lock for the duration of a submission of any workloads. This leads to a large amount of contention for the lock and stalling in the signal handler when the resource may be `Clean` and can simply be switched over to `CpuDirty` without locking and utilizing atomics which is what this commit addresses. --- app/src/main/cpp/skyline/gpu/buffer.cpp | 59 ++++++---- app/src/main/cpp/skyline/gpu/buffer.h | 4 +- .../main/cpp/skyline/gpu/texture/texture.cpp | 110 ++++++++++-------- .../main/cpp/skyline/gpu/texture/texture.h | 4 +- 4 files changed, 101 insertions(+), 76 deletions(-) diff --git a/app/src/main/cpp/skyline/gpu/buffer.cpp b/app/src/main/cpp/skyline/gpu/buffer.cpp index b0620284..10736f72 100644 --- a/app/src/main/cpp/skyline/gpu/buffer.cpp +++ b/app/src/main/cpp/skyline/gpu/buffer.cpp @@ -20,6 +20,10 @@ namespace skyline::gpu { SynchronizeGuest(true); // We can skip trapping since the caller will do it WaitOnFence(); }, [this] { + DirtyState expectedState{DirtyState::Clean}; + if (dirtyState.compare_exchange_strong(expectedState, DirtyState::CpuDirty, std::memory_order_relaxed) || expectedState == DirtyState::CpuDirty) + return; // If we can transition the buffer to CPU dirty (from Clean) or if it already is CPU dirty then we can just return, we only need to do the lock and corresponding sync if the buffer is GPU dirty + std::scoped_lock lock{*this}; SynchronizeGuest(true); dirtyState = DirtyState::CpuDirty; // We need to assume the buffer is dirty since we don't know what the guest is writing @@ -97,12 +101,17 @@ namespace skyline::gpu { } void Buffer::MarkGpuDirty() { - if (dirtyState == DirtyState::GpuDirty || !guest) + if (!guest) return; + auto currentState{dirtyState.load(std::memory_order_relaxed)}; + do { + if (currentState == DirtyState::GpuDirty) + return; + } while (!dirtyState.compare_exchange_strong(currentState, DirtyState::GpuDirty, std::memory_order_relaxed)); + AdvanceSequence(); // The GPU will modify buffer contents so advance to the next sequence gpu.state.nce->RetrapRegions(*trapHandle, false); - dirtyState = DirtyState::GpuDirty; } void Buffer::WaitOnFence() { @@ -123,42 +132,47 @@ namespace skyline::gpu { } void Buffer::SynchronizeHost(bool rwTrap) { - if (dirtyState != DirtyState::CpuDirty || !guest) - return; // If the buffer has not been modified on the CPU or there's no guest buffer, there is no need to synchronize it + if (!guest) + return; - WaitOnFence(); + auto currentState{dirtyState.load(std::memory_order_relaxed)}; + do { + if (currentState != DirtyState::CpuDirty || !guest) + return; // If the buffer has not been modified on the CPU, there is no need to synchronize it + } while (!dirtyState.compare_exchange_strong(currentState, rwTrap ? DirtyState::GpuDirty : DirtyState::Clean, std::memory_order_relaxed)); TRACE_EVENT("gpu", "Buffer::SynchronizeHost"); AdvanceSequence(); // We are modifying GPU backing contents so advance to the next sequence - std::memcpy(backing.data(), mirror.data(), mirror.size()); - if (rwTrap) { - gpu.state.nce->RetrapRegions(*trapHandle, false); - dirtyState = DirtyState::GpuDirty; - } else { - gpu.state.nce->RetrapRegions(*trapHandle, true); - dirtyState = DirtyState::Clean; - } + WaitOnFence(); + + std::memcpy(backing.data(), mirror.data(), mirror.size()); + gpu.state.nce->RetrapRegions(*trapHandle, !rwTrap); // Trap any future CPU reads (optionally) + writes to this buffer } void Buffer::SynchronizeGuest(bool skipTrap, bool nonBlocking) { - if (dirtyState != DirtyState::GpuDirty || !guest) - return; // If the buffer has not been used on the GPU or there's no guest buffer, there is no need to synchronize it + if (!guest) + return; + + auto currentState{dirtyState.load(std::memory_order_relaxed)}; + do { + if (currentState != DirtyState::GpuDirty) + return; // If the buffer has not been used on the GPU, there is no need to synchronize it + } while (!dirtyState.compare_exchange_strong(currentState, DirtyState::Clean, std::memory_order_relaxed)); if (nonBlocking && !PollFence()) return; - else if (!nonBlocking) - WaitOnFence(); TRACE_EVENT("gpu", "Buffer::SynchronizeGuest"); - std::memcpy(mirror.data(), backing.data(), mirror.size()); - if (!skipTrap) gpu.state.nce->RetrapRegions(*trapHandle, true); - dirtyState = DirtyState::Clean; + if (!nonBlocking) + WaitOnFence(); + + std::memcpy(mirror.data(), backing.data(), mirror.size()); } void Buffer::SynchronizeGuestImmediate(bool isFirstUsage, const std::function &flushHostCallback) { @@ -186,8 +200,7 @@ namespace skyline::gpu { else if (dirtyState == DirtyState::GpuDirty) SynchronizeGuestImmediate(isFirstUsage, flushHostCallback); - if (dirtyState != DirtyState::Clean) - Logger::Error("Attempting to write to a dirty buffer"); // This should never happen since we do syncs in both directions above + // It's possible that the guest will arbitrarily modify the buffer contents on the CPU after the syncs and trigger the signal handler which would set the dirty state to CPU dirty, this is acceptable as there is no requirement to make writes visible immediately std::memcpy(mirror.data() + offset, data.data(), data.size()); // Always copy to mirror since any CPU side reads will need the up-to-date contents @@ -267,7 +280,7 @@ namespace skyline::gpu { bool Buffer::BufferDelegate::LockWithTag(ContextTag pTag) { bool result{}; - buffer.Lock([pTag, &result](Buffer* pBuffer) { + buffer.Lock([pTag, &result](Buffer *pBuffer) { result = pBuffer->LockWithTag(pTag); }); return result; diff --git a/app/src/main/cpp/skyline/gpu/buffer.h b/app/src/main/cpp/skyline/gpu/buffer.h index 36390404..822ef816 100644 --- a/app/src/main/cpp/skyline/gpu/buffer.h +++ b/app/src/main/cpp/skyline/gpu/buffer.h @@ -36,7 +36,9 @@ namespace skyline::gpu { Clean, //!< The CPU mappings are in sync with the GPU buffer CpuDirty, //!< The CPU mappings have been modified but the GPU buffer is not up to date GpuDirty, //!< The GPU buffer has been modified but the CPU mappings have not been updated - } dirtyState{DirtyState::CpuDirty}; //!< The state of the CPU mappings with respect to the GPU buffer + }; + std::atomic dirtyState{DirtyState::CpuDirty}; //!< The state of the CPU mappings with respect to the GPU buffer + static_assert(std::atomic::is_always_lock_free); bool everHadInlineUpdate{}; //!< Whether the buffer has ever had an inline update since it was created, if this is set then megabuffering will be attempted by views to avoid the cost of inline GPU updates diff --git a/app/src/main/cpp/skyline/gpu/texture/texture.cpp b/app/src/main/cpp/skyline/gpu/texture/texture.cpp index eae085e4..81ace468 100644 --- a/app/src/main/cpp/skyline/gpu/texture/texture.cpp +++ b/app/src/main/cpp/skyline/gpu/texture/texture.cpp @@ -98,7 +98,7 @@ namespace skyline::gpu { bool TextureView::LockWithTag(ContextTag tag) { bool result{}; - texture.Lock([tag, &result](Texture* pTexture) { + texture.Lock([tag, &result](Texture *pTexture) { result = pTexture->LockWithTag(tag); }); return result; @@ -148,6 +148,10 @@ namespace skyline::gpu { SynchronizeGuest(true); // We can skip trapping since the caller will do it WaitOnFence(); }, [this] { + DirtyState expectedState{DirtyState::Clean}; + if (dirtyState.compare_exchange_strong(expectedState, DirtyState::CpuDirty, std::memory_order_relaxed) || expectedState == DirtyState::CpuDirty) + return; // If we can transition the texture to CPU dirty (from Clean) or if it already is CPU dirty then we can just return, we only need to do the lock and corresponding sync if the texture is GPU dirty + std::scoped_lock lock{*this}; SynchronizeGuest(true); dirtyState = DirtyState::CpuDirty; // We need to assume the texture is dirty since we don't know what the guest is writing @@ -156,9 +160,7 @@ namespace skyline::gpu { } std::shared_ptr Texture::SynchronizeHostImpl() { - if (!guest) - throw exception("Synchronization of host textures requires a valid guest texture to synchronize from"); - else if (guest->dimensions != dimensions) + if (guest->dimensions != dimensions) throw exception("Guest and host dimensions being different is not supported currently"); auto pointer{mirror.data()}; @@ -576,10 +578,16 @@ namespace skyline::gpu { } void Texture::MarkGpuDirty() { - if (dirtyState == DirtyState::GpuDirty || !guest || format != guest->format) - return; // In addition to other checks, we also need to skip GPU dirty if the host format and guest format differ as we don't support re-encoding compressed textures which is when this generally occurs + if (!guest || format != guest->format) + return; // We need to skip GPU dirty if the host format and guest format differ as we don't support re-encoding compressed textures which is when this generally occurs + + auto currentState{dirtyState.load(std::memory_order_relaxed)}; + do { + if (currentState == DirtyState::GpuDirty) + return; + } while (!dirtyState.compare_exchange_strong(currentState, DirtyState::GpuDirty, std::memory_order_relaxed)); + gpu.state.nce->RetrapRegions(*trapHandle, false); - dirtyState = DirtyState::GpuDirty; } bool Texture::WaitOnBacking() { @@ -642,8 +650,14 @@ namespace skyline::gpu { } void Texture::SynchronizeHost(bool rwTrap) { - if (dirtyState != DirtyState::CpuDirty || !guest) - return; // If the texture has not been modified on the CPU or has no mappings, there is no need to synchronize it + if (!guest) + return; + + auto currentState{dirtyState.load(std::memory_order_relaxed)}; + do { + if (currentState != DirtyState::CpuDirty) + return; // If the texture has not been modified on the CPU or has no mappings, there is no need to synchronize it + } while (!dirtyState.compare_exchange_strong(currentState, rwTrap ? DirtyState::GpuDirty : DirtyState::Clean, std::memory_order_relaxed)); TRACE_EVENT("gpu", "Texture::SynchronizeHost"); @@ -657,19 +671,19 @@ namespace skyline::gpu { cycle = lCycle; } - if (rwTrap) { - gpu.state.nce->RetrapRegions(*trapHandle, false); - dirtyState = DirtyState::GpuDirty; - } else { - gpu.state.nce->RetrapRegions(*trapHandle, true); // Trap any future CPU writes to this texture - dirtyState = DirtyState::Clean; - } + gpu.state.nce->RetrapRegions(*trapHandle, !rwTrap); // Trap any future CPU reads (optionally) + writes to this texture } void Texture::SynchronizeHostWithBuffer(const vk::raii::CommandBuffer &commandBuffer, const std::shared_ptr &pCycle, bool rwTrap) { - if (dirtyState != DirtyState::CpuDirty || !guest) + if (!guest) return; + auto currentState{dirtyState.load(std::memory_order_relaxed)}; + do { + if (currentState != DirtyState::CpuDirty) + return; + } while (!dirtyState.compare_exchange_strong(currentState, rwTrap ? DirtyState::GpuDirty : DirtyState::Clean, std::memory_order_relaxed)); + TRACE_EVENT("gpu", "Texture::SynchronizeHostWithBuffer"); auto stagingBuffer{SynchronizeHostImpl()}; @@ -680,35 +694,29 @@ namespace skyline::gpu { cycle = pCycle; } - if (rwTrap) { - gpu.state.nce->RetrapRegions(*trapHandle, false); - dirtyState = DirtyState::GpuDirty; - } else { - gpu.state.nce->RetrapRegions(*trapHandle, true); // Trap any future CPU writes to this texture - dirtyState = DirtyState::Clean; - } + gpu.state.nce->RetrapRegions(*trapHandle, !rwTrap); // Trap any future CPU reads (optionally) + writes to this texture } void Texture::SynchronizeGuest(bool skipTrap) { - if (dirtyState != DirtyState::GpuDirty || layout == vk::ImageLayout::eUndefined || !guest) { - // We can skip syncing in three cases: - // * If the texture has not been used on the GPU, there is no need to synchronize it - // * If the state of the host texture is undefined then so can the guest - // * If there is no guest texture to synchronise + if (!guest) return; - } - if (layout == vk::ImageLayout::eUndefined || format != guest->format) { - // If the state of the host texture is undefined then so can the guest - // If the texture has differing formats on the guest and host, we don't support converting back in that case as it may involve recompression of a decompressed texture - if (!skipTrap) - gpu.state.nce->RetrapRegions(*trapHandle, true); - dirtyState = DirtyState::Clean; - return; - } + auto currentState{dirtyState.load(std::memory_order_relaxed)}; + do { + if (currentState != DirtyState::GpuDirty) + return; // If the buffer has not been used on the GPU, there is no need to synchronize it + } while (!dirtyState.compare_exchange_strong(currentState, DirtyState::Clean, std::memory_order_relaxed)); TRACE_EVENT("gpu", "Texture::SynchronizeGuest"); + if (!skipTrap) + gpu.state.nce->RetrapRegions(*trapHandle, true); + + if (layout == vk::ImageLayout::eUndefined || format != guest->format) + // If the state of the host texture is undefined then so can the guest + // If the texture has differing formats on the guest and host, we don't support converting back in that case as it may involve recompression of a decompressed texture + return; + WaitOnBacking(); if (tiling == vk::ImageTiling::eOptimal || !std::holds_alternative(backing)) { @@ -727,25 +735,27 @@ namespace skyline::gpu { } else { throw exception("Host -> Guest synchronization of images tiled as '{}' isn't implemented", vk::to_string(tiling)); } - - if (!skipTrap) - gpu.state.nce->RetrapRegions(*trapHandle, true); - dirtyState = DirtyState::Clean; } void Texture::SynchronizeGuestWithBuffer(const vk::raii::CommandBuffer &commandBuffer, const std::shared_ptr &pCycle) { - if (dirtyState != DirtyState::GpuDirty || !guest) + if (!guest) return; - if (layout == vk::ImageLayout::eUndefined || format != guest->format) { - // If the state of the host texture is undefined then so can the guest - // If the texture has differing formats on the guest and host, we don't support converting back in that case as it may involve recompression of a decompressed texture - dirtyState = DirtyState::Clean; - return; - } + auto currentState{dirtyState.load(std::memory_order_relaxed)}; + do { + if (currentState != DirtyState::GpuDirty) + return; // If the buffer has not been used on the GPU, there is no need to synchronize it + } while (!dirtyState.compare_exchange_strong(currentState, DirtyState::Clean, std::memory_order_relaxed)); TRACE_EVENT("gpu", "Texture::SynchronizeGuestWithBuffer"); + gpu.state.nce->RetrapRegions(*trapHandle, true); + + if (layout == vk::ImageLayout::eUndefined || format != guest->format) + // If the state of the host texture is undefined then so can the guest + // If the texture has differing formats on the guest and host, we don't support converting back in that case as it may involve recompression of a decompressed texture + return; + WaitOnBacking(); pCycle->ChainCycle(cycle); @@ -762,8 +772,6 @@ namespace skyline::gpu { } else { throw exception("Host -> Guest synchronization of images tiled as '{}' isn't implemented", vk::to_string(tiling)); } - - dirtyState = DirtyState::Clean; } std::shared_ptr Texture::GetView(vk::ImageViewType type, vk::ImageSubresourceRange range, texture::Format pFormat, vk::ComponentMapping mapping) { diff --git a/app/src/main/cpp/skyline/gpu/texture/texture.h b/app/src/main/cpp/skyline/gpu/texture/texture.h index 310579b1..ebb838e8 100644 --- a/app/src/main/cpp/skyline/gpu/texture/texture.h +++ b/app/src/main/cpp/skyline/gpu/texture/texture.h @@ -367,7 +367,9 @@ namespace skyline::gpu { Clean, //!< The CPU mappings are in sync with the GPU texture CpuDirty, //!< The CPU mappings have been modified but the GPU texture is not up to date GpuDirty, //!< The GPU texture has been modified but the CPU mappings have not been updated - } dirtyState{DirtyState::CpuDirty}; //!< The state of the CPU mappings with respect to the GPU texture + }; + std::atomic dirtyState{DirtyState::CpuDirty}; //!< The state of the CPU mappings with respect to the GPU texture + static_assert(std::atomic::is_always_lock_free); /** * @brief Storage for all metadata about a specific view into the buffer, used to prevent redundant view creation and duplication of VkBufferView(s)