From f356fa3730be0fc23d0ca2497c0748fd2012b51d Mon Sep 17 00:00:00 2001 From: nmzik Date: Sat, 24 Oct 2026 01:05:19 +0200 Subject: [PATCH] gpu: move the BDA sync queue from MemoryTracker into BufferCache --- src/graphics/host_gpu/memoryTracker.cpp | 15 ---- src/graphics/host_gpu/memoryTracker.h | 7 -- .../host_gpu/renderer/cache/bufferCache.cpp | 22 ++--- .../host_gpu/renderer/cache/bufferCache.h | 12 +-- .../host_gpu/renderer/renderContext.cpp | 6 +- tests/MemoryTrackerTests.cpp | 80 ------------------- tests/ShaderRecompilerComputeTests.cpp | 51 ++++++++++-- 7 files changed, 71 insertions(+), 122 deletions(-) diff --git a/src/graphics/host_gpu/memoryTracker.cpp b/src/graphics/host_gpu/memoryTracker.cpp index a3a070e9e..4c45702b8 100644 --- a/src/graphics/host_gpu/memoryTracker.cpp +++ b/src/graphics/host_gpu/memoryTracker.cpp @@ -66,18 +66,6 @@ RegionManager* MemoryTracker::GetOrCreateRegion(uint64_t index) { return ptr; } -void MemoryTracker::QueueBdaSync(uint64_t vaddr, uint64_t size) { - const auto begin = Common::AlignDown(vaddr, TRACKER_PAGE_SIZE); - const auto end = Common::AlignUp(vaddr + size, TRACKER_PAGE_SIZE); - std::lock_guard lock(m_bda_sync_mutex); - m_bda_sync.Add(begin, end - begin); -} - -RangeSet MemoryTracker::TakeBdaSync() { - std::lock_guard lock(m_bda_sync_mutex); - return std::exchange(m_bda_sync, RangeSet {}); -} - bool MemoryTracker::IsRegionCpuModified(uint64_t vaddr, uint64_t size) { CheckNotInUploadCallback(); return Iterate(vaddr, size, [](RegionManager* manager, uint64_t offset, uint64_t bytes) { @@ -100,7 +88,6 @@ void MemoryTracker::MarkRegionAsCpuModified(uint64_t vaddr, uint64_t size) { std::scoped_lock lock(manager->lock); manager->ChangeState(manager->GetCpuAddr() + offset, bytes); }); - QueueBdaSync(vaddr, size); } void MemoryTracker::MarkRegionAsGpuModified(uint64_t vaddr, uint64_t size) { @@ -141,8 +128,6 @@ void MemoryTracker::UntrackMemory(uint64_t vaddr, uint64_t size) { Iterate(vaddr, size, [](RegionManager* manager, uint64_t offset, uint64_t bytes) { manager->ChangeState(manager->GetCpuAddr() + offset, bytes); }); - locks.clear(); - QueueBdaSync(vaddr, size); } } // namespace Libs::Graphics diff --git a/src/graphics/host_gpu/memoryTracker.h b/src/graphics/host_gpu/memoryTracker.h index ff81021f2..0bc9fe547 100644 --- a/src/graphics/host_gpu/memoryTracker.h +++ b/src/graphics/host_gpu/memoryTracker.h @@ -29,10 +29,6 @@ public: void MarkRegionAsGpuModified(uint64_t vaddr, uint64_t size); void UnmarkRegionAsGpuModified(uint64_t vaddr, uint64_t size); void UntrackMemory(uint64_t vaddr, uint64_t size); - // Queue after changing ownership; a concurrent drain must not consume an event - // before the dirty pages it describes become visible. - void QueueBdaSync(uint64_t vaddr, uint64_t size); - [[nodiscard]] RangeSet TakeBdaSync(); // Removes protection from a range and flushes GPU-owned data when required. template void InvalidateRegion(uint64_t vaddr, uint64_t size, Flush&& on_flush) noexcept { @@ -55,7 +51,6 @@ public: on_flush(); } }); - QueueBdaSync(vaddr, size); } #if KYTY_BUILD == KYTY_BUILD_DEBUG void ValidateGpuDirtyPages(const RangeSet& dirty, uint64_t vaddr, uint64_t size, @@ -154,8 +149,6 @@ private: std::unique_ptr[]> m_regions; std::vector> m_region_storage; std::mutex m_region_mutex; - std::mutex m_bda_sync_mutex; - RangeSet m_bda_sync; PageManager& m_page_manager; }; diff --git a/src/graphics/host_gpu/renderer/cache/bufferCache.cpp b/src/graphics/host_gpu/renderer/cache/bufferCache.cpp index 91f30d77c..7780dc882 100644 --- a/src/graphics/host_gpu/renderer/cache/bufferCache.cpp +++ b/src/graphics/host_gpu/renderer/cache/bufferCache.cpp @@ -127,7 +127,7 @@ void BufferCache::ChangeRegister(BufferId id) { } WriteDataBuffer(m_bda_pagetable_buffer, table_offset, addresses.data(), addresses.size() * sizeof(vk::DeviceAddress)); - m_memory_tracker.QueueBdaSync(buffer.CpuAddress(), buffer.Size()); + QueueBdaSync(buffer.CpuAddress(), buffer.Size()); } else { const auto found = m_buffers.find(buffer.CpuAddress()); EXIT_IF(found == m_buffers.end() || found->second != id); @@ -299,6 +299,7 @@ void BufferCache::InvalidateMemory(uint64_t vaddr, uint64_t size) { } m_memory_tracker.InvalidateRegion(vaddr, size, [this, vaddr, size] { ReadMemory(vaddr, size, true); }); + QueueBdaSync(vaddr, size); } void BufferCache::ReadMemory(uint64_t vaddr, uint64_t size, bool is_write) { @@ -318,6 +319,7 @@ void BufferCache::ReadMemory(uint64_t vaddr, uint64_t size, bool is_write) { } if (is_write) { m_memory_tracker.MarkRegionAsCpuModified(vaddr, size); + QueueBdaSync(vaddr, size); } }); } @@ -719,14 +721,16 @@ void BufferCache::ProcessFaultBuffer() { m_fault_manager.ProcessFaultBuffer(); } -void BufferCache::SynchronizeBda(const RangeSet& mapped) { - // Take before walking: writes queued during synchronization remain pending. - const auto pending = m_memory_tracker.TakeBdaSync(); - pending.ForEach([&](uint64_t begin, uint64_t end) { - mapped.ForEachInRange(begin, end - begin, [this](uint64_t start, uint64_t finish) { - SynchronizeBuffersInRange(start, finish - start); - }); - }); +void BufferCache::QueueBdaSync(uint64_t vaddr, uint64_t size) { + const auto begin = Common::AlignDown(vaddr, TRACKER_PAGE_SIZE); + const auto end = Common::AlignUp(vaddr + size, TRACKER_PAGE_SIZE); + std::lock_guard lock(m_bda_sync_mutex); + m_bda_sync.Add(begin, end - begin); +} + +RangeSet BufferCache::TakeBdaSync() { + std::lock_guard lock(m_bda_sync_mutex); + return std::exchange(m_bda_sync, {}); } void BufferCache::SynchronizeBuffersInRange(uint64_t vaddr, uint64_t size) { diff --git a/src/graphics/host_gpu/renderer/cache/bufferCache.h b/src/graphics/host_gpu/renderer/cache/bufferCache.h index 8a30e68e1..ba2d1a7a3 100644 --- a/src/graphics/host_gpu/renderer/cache/bufferCache.h +++ b/src/graphics/host_gpu/renderer/cache/bufferCache.h @@ -13,6 +13,7 @@ #include #include +#include #include #include #include @@ -85,10 +86,10 @@ public: void MarkColorMetadataChecked(uint64_t vaddr, uint64_t size, uint32_t keys); [[nodiscard]] bool IsColorMetadataChecked(uint64_t vaddr, uint64_t size, uint32_t keys) const; void ProcessFaultBuffer(); - void SynchronizeBda(const RangeSet& mapped); - void QueueBdaSync(uint64_t vaddr, uint64_t size) { - m_memory_tracker.QueueBdaSync(vaddr, size); - } + void SynchronizeBuffersInRange(uint64_t vaddr, uint64_t size); + // Queue after the range's state change becomes visible, or a drain could consume it first. + void QueueBdaSync(uint64_t vaddr, uint64_t size); + [[nodiscard]] RangeSet TakeBdaSync(); void RunGarbageCollector(); private: @@ -122,7 +123,6 @@ private: void DeleteBuffer(BufferId id); [[nodiscard]] bool SynchronizeBuffer(Buffer& buffer, uint64_t vaddr, uint64_t size, bool is_written, bool is_texel_buffer); - void SynchronizeBuffersInRange(uint64_t vaddr, uint64_t size); [[nodiscard]] vk::Buffer UploadCopies(Buffer& buffer, std::span copies, uint64_t total_size); [[nodiscard]] bool SynchronizeBufferFromImage(Buffer& buffer, uint64_t vaddr, uint64_t size); @@ -144,6 +144,8 @@ private: // One per TileManager::ColorClearSlots key slot. std::array m_checked_color_metadata; MemoryTracker m_memory_tracker; + std::mutex m_bda_sync_mutex; + RangeSet m_bda_sync; StreamBuffer m_staging_buffer; StreamBuffer m_stream_buffer; StreamBuffer m_download_buffer; diff --git a/src/graphics/host_gpu/renderer/renderContext.cpp b/src/graphics/host_gpu/renderer/renderContext.cpp index 90a54a598..d320ab46a 100644 --- a/src/graphics/host_gpu/renderer/renderContext.cpp +++ b/src/graphics/host_gpu/renderer/renderContext.cpp @@ -134,7 +134,11 @@ void RenderContext::PrepareBda() { m_bda_logged = true; } std::shared_lock lock(m_mapped_ranges_mutex); - m_buffer_cache.SynchronizeBda(m_mapped_ranges); + m_buffer_cache.TakeBdaSync().ForEach([this](uint64_t begin, uint64_t end) { + m_mapped_ranges.ForEachInRange(begin, end - begin, [this](uint64_t start, uint64_t finish) { + m_buffer_cache.SynchronizeBuffersInRange(start, finish - start); + }); + }); m_fault_process_pending = true; } diff --git a/tests/MemoryTrackerTests.cpp b/tests/MemoryTrackerTests.cpp index 63bbc11c9..d859a492f 100644 --- a/tests/MemoryTrackerTests.cpp +++ b/tests/MemoryTrackerTests.cpp @@ -213,12 +213,6 @@ struct ProtectionCall { std::vector g_protection_log; std::mutex g_protection_log_mutex; -struct ProtectionGate { - std::binary_semaphore entered{0}; - std::binary_semaphore resume{0}; -}; -ProtectionGate *g_protection_gate = nullptr; - void ResetProtectionLog() { g_protection_calls = 0; g_protection_log.clear(); @@ -226,10 +220,6 @@ void ResetProtectionLog() { bool ProtectAddressSpace(uint64_t vaddr, uint64_t size, Common::VirtualMemory::Mode mode) { - if (g_protection_gate != nullptr && mode == Common::VirtualMemory::Mode::ReadWrite) { - g_protection_gate->entered.release(); - g_protection_gate->resume.acquire(); - } uint32_t protection = PAGE_NOACCESS; if (mode == Common::VirtualMemory::Mode::Read) { protection = PAGE_READONLY; @@ -417,74 +407,6 @@ void TestCpuDirtyUpload() { Release(memory); } -void TestBdaSyncDirtiness() { - constexpr auto page = Libs::Graphics::TRACKER_PAGE_SIZE; - TrackerHarness harness; - auto &tracker = harness.tracker; - auto *memory = Allocate(harness.page_manager, 3); - const auto address = reinterpret_cast(memory); - Check(tracker.TakeBdaSync().Empty(), "new tracker queued unowned memory"); - tracker.QueueBdaSync(address, 3 * page); - Check(tracker.IsRegionCpuModified(address, page), "new region was clean"); - auto pending = tracker.TakeBdaSync(); - Check(pending.Contains(address, 3 * page) && !pending.Intersects(address + 3 * page, page), - "mapped or registered range did not bound initial BDA synchronization"); - tracker.ForEachUploadRange(address, 3 * page, false, - [](uint64_t, uint64_t) noexcept {}, []() noexcept {}); - Check(tracker.TakeBdaSync().Empty(), "clean upload queued redundant BDA work"); - - tracker.MarkRegionAsCpuModified(address + 17, 1); - pending = tracker.TakeBdaSync(); - Check(pending.Contains(address, page) && !pending.Intersects(address + page, page) && - tracker.TakeBdaSync().Empty(), - "byte dirtiness did not drain exactly its whole tracker page"); - // A write after Take belongs to the next drain, even when its page was just taken. - tracker.MarkRegionAsCpuModified(address + 31, 1); - Check(tracker.TakeBdaSync().Contains(address, page), "re-dirty event was lost"); - tracker.ForEachUploadRange(address, page, true, - [](uint64_t, uint64_t) noexcept {}, []() noexcept {}); - Check(tracker.TakeBdaSync().Empty(), "GPU ownership queued an unnecessary upload"); - tracker.InvalidateRegion(address + 7, 1, [&] { - tracker.UnmarkRegionAsGpuModified(address, page); - tracker.MarkRegionAsCpuModified(address, page); - }); - Check(tracker.TakeBdaSync().Contains(address, page), - "GPU-to-CPU invalidation did not queue BDA synchronization"); - tracker.InvalidateRegion(address + page + 7, 1, [] {}); - Check(tracker.TakeBdaSync().Contains(address + page, page), - "CPU invalidation did not queue BDA synchronization"); - tracker.UntrackMemory(address, 3 * page); - Check(tracker.TakeBdaSync().Contains(address, 3 * page), - "untracked memory did not queue its newly dirty pages"); - Release(memory); -} - -void TestBdaSyncConcurrentDrain() { - constexpr auto page = Libs::Graphics::TRACKER_PAGE_SIZE; - TrackerHarness harness; - auto &tracker = harness.tracker; - auto *memory = Allocate(harness.page_manager, 1); - const auto address = reinterpret_cast(memory); - tracker.ForEachUploadRange(address, page, false, - [](uint64_t, uint64_t) noexcept {}, []() noexcept {}); - (void)tracker.TakeBdaSync(); - ProtectionGate gate; - g_protection_gate = &gate; - std::jthread writer([&] { tracker.MarkRegionAsCpuModified(address + 17, 1); }); - gate.entered.acquire(); - // Ownership is still changing. Publishing its event before this point would let - // a drain consume it before the writer has finished releasing page protection. - Check(tracker.TakeBdaSync().Empty(), "BDA event preceded its state transition"); - gate.resume.release(); - writer.join(); - g_protection_gate = nullptr; - Check(tracker.TakeBdaSync().Contains(address, page) && - tracker.IsRegionCpuModified(address, page), - "concurrent drain lost a completed CPU dirty transition"); - tracker.UntrackMemory(address, page); - Release(memory); -} - void TestCleanUploadPreservesOwnership() { constexpr auto page_size = Libs::Graphics::TRACKER_PAGE_SIZE; constexpr auto region_size = Libs::Graphics::TRACKER_REGION_SIZE; @@ -1280,8 +1202,6 @@ int main(int argc, char **argv) { TestQueriesDoNotRequireMappedOwnership(); TestConcurrentRegionPublication(); TestCpuDirtyUpload(); - TestBdaSyncDirtiness(); - TestBdaSyncConcurrentDrain(); TestCleanUploadPreservesOwnership(); TestRangeInvalidation(); TestGpuReacquisitionAfterInvalidation(); diff --git a/tests/ShaderRecompilerComputeTests.cpp b/tests/ShaderRecompilerComputeTests.cpp index b3ca80c4b..ef41eb9b1 100644 --- a/tests/ShaderRecompilerComputeTests.cpp +++ b/tests/ShaderRecompilerComputeTests.cpp @@ -193,6 +193,8 @@ struct BufferCacheTestAccess { return cache.SynchronizeBufferFromImage(buffer, address, size); } + static std::mutex &BdaSyncMutex(BufferCache &cache) { return cache.m_bda_sync_mutex; } + static BufferId PageOwner(const BufferCache &cache, uint64_t address) { const auto *owner = cache.m_page_table.Find( address >> BufferCache::PageTable::kPageBits); @@ -5596,10 +5598,18 @@ public: constexpr const char *name = "BdaSyncUploads"; constexpr auto page = BufferCache::CACHING_PAGESIZE; EnsureRuntimeContext(); - const auto base = LibKernel::Memory::AllocateRuntimeMemory( - 0, 3 * page, Common::VirtualMemory::Mode::ReadWrite, "bda_sync_uploads"); - Require(name, "allocation", base != 0, "guest allocation failed"); - auto *words = reinterpret_cast(base); + // Direct memory, so readbacks can publish GPU-dirty bytes. + int64_t direct = -1; + void *mapped = nullptr; + Require(name, "allocation", + LibKernel::Memory::KernelAllocateDirectMemory( + 0, LibKernel::Memory::KernelGetDirectMemorySize(), 3 * page, page, 0, &direct) == + 0 && + LibKernel::Memory::KernelMapDirectMemory(&mapped, 3 * page, 0x3, 0, direct, page) == + 0, + "guest allocation failed"); + const auto base = reinterpret_cast(mapped); + auto *words = static_cast(mapped); std::fill_n(words, 3 * page / sizeof(u32), 0x12345678u); RenderContext context(m_runtime_context); auto &scheduler = context.GetCommandScheduler(); @@ -5608,6 +5618,7 @@ public: HW::UserConfig user_config{}; HW::Shader shaders{}; scheduler.Begin(registers, user_config, shaders); + context.InitializeGpu(nullptr); context.MapMemory(base, page); context.MapMemory(base + 2 * page, page); const auto id = cache.FindBuffer(base, 3 * page); @@ -5650,6 +5661,34 @@ public: context.PrepareBda(); snapshot(2 * page, 5); + // A write fault on GPU-dirty memory flushes it on the GPU thread, which must queue the page + // itself: the faulting thread can resume only after later commands. + (void)cache.ObtainBuffer(base + 20, sizeof(u32), true, false); + std::unique_lock queue_lock(BufferCacheTestAccess::BdaSyncMutex(cache)); + std::jthread fault([&] { cache.InvalidateMemory(base + 20, sizeof(u32)); }); + while (!cache.IsRegionCpuModified(base, TRACKER_PAGE_SIZE)) { + std::this_thread::yield(); + } + std::binary_semaphore later_command{0}; + context.GetGpu().SendCommand([&] { later_command.release(); }); + Require(name, "flush queue order", + !later_command.try_acquire_for(std::chrono::milliseconds(100)), + "a GPU command ran before the write-fault flush queued its page"); + queue_lock.unlock(); + fault.join(); + later_command.acquire(); + + // Garbage collection untracks a clean buffer; its replacement queues the range on Register. + BufferCacheTestAccess::SetGarbageCollectionThresholds( + cache, 0, std::numeric_limits::max()); + cache.RunGarbageCollector(); + Require(name, "clean eviction", !cache.IsRegionRegistered(base, 3 * page), + "garbage collection kept the clean buffer"); + (void)cache.FindBuffer(base, 3 * page); + context.PrepareBda(); + Require(name, "replacement upload", !cache.IsRegionCpuModified(base, 3 * page), + "a buffer replacing an evicted one did not upload its untracked pages"); + vk::BufferMemoryBarrier barrier{}; barrier.srcAccessMask = vk::AccessFlagBits::eTransferWrite; barrier.dstAccessMask = vk::AccessFlagBits::eHostRead; @@ -5666,7 +5705,9 @@ public: context.UnmapMemory(base, 3 * page); scheduler.Finish(); DestroyBuffer(&readback); - Require(name, "release", LibKernel::Memory::FreeGuestMemory(base, 3 * page), + Require(name, "release", + LibKernel::Memory::KernelMunmap(base, 3 * page) == 0 && + LibKernel::Memory::KernelReleaseDirectMemory(direct, 3 * page) == 0, "guest allocation release failed"); std::printf("[gpu] %-32s ok\n", name); }