From 11d4c0d2a203a3309c7e9eb7a783491134554660 Mon Sep 17 00:00:00 2001 From: nmzik Date: Sun, 23 Aug 2026 19:21:46 +0200 Subject: [PATCH] renderer: synchronize bounded buffer downloads --- .../host_gpu/renderer/cache/bufferCache.cpp | 192 ++++++++---------- .../host_gpu/renderer/cache/bufferCache.h | 6 +- tests/ShaderRecompilerComputeTests.cpp | 134 +++++++++--- 3 files changed, 194 insertions(+), 138 deletions(-) diff --git a/src/graphics/host_gpu/renderer/cache/bufferCache.cpp b/src/graphics/host_gpu/renderer/cache/bufferCache.cpp index 68f547ab0..d61bd0a13 100644 --- a/src/graphics/host_gpu/renderer/cache/bufferCache.cpp +++ b/src/graphics/host_gpu/renderer/cache/bufferCache.cpp @@ -50,12 +50,6 @@ struct BufferCache::DownloadCopy { uint64_t size = 0; }; -struct BufferCache::DownloadRange { - uint64_t address = 0; - uint64_t size = 0; - uint64_t offset = 0; -}; - void BufferCache::Register(BufferId id) { const auto& buffer = m_slot_buffers[id]; PageTable::PageRange pages {}; @@ -122,79 +116,62 @@ std::pair BufferCache::DownloadEnvelope(const DownloadCopy& return {begin, end - begin}; } -std::vector -BufferCache::RecordDownloads(std::span copies) { - uint64_t reservation_size = 0; +void BufferCache::DownloadBufferMemory(std::span copies) { + std::vector batch; + batch.reserve(copies.size()); + uint64_t packed_size = 0; + auto& download = m_download_buffer; + const auto flush = [&] { + const auto [mapped, base_offset] = download.Map(packed_size, DOWNLOAD_ALIGNMENT); + EXIT_IF(mapped == nullptr); + uint64_t cursor = 0; + for (const auto& copy: batch) { + const auto [source_begin, envelope_size] = DownloadEnvelope(copy); + download.CopyFrom(m_scheduler.Current(), *copy.buffer, source_begin, base_offset + cursor, + envelope_size, vk::AccessFlagBits::eMemoryWrite, vk::AccessFlags {}, + vk::AccessFlagBits::eMemoryRead | vk::AccessFlagBits::eMemoryWrite, + vk::AccessFlagBits::eHostRead); + cursor += AlignDownload(envelope_size); + } + download.Commit(); + const auto completion_tick = m_scheduler.CurrentTick(); + m_scheduler.FinishCurrent(); + m_scheduler.WaitPriorityOperations(completion_tick); + cursor = 0; + for (const auto& copy: batch) { + const auto [source_begin, envelope_size] = DownloadEnvelope(copy); + const auto offset = cursor + copy.source_offset - source_begin; + download.Invalidate(base_offset + offset, copy.size); + Libs::LibKernel::Memory::WriteBacking(copy.address, mapped + offset, copy.size); + cursor += AlignDownload(envelope_size); + } + batch.clear(); + packed_size = 0; + }; + for (auto copy: copies) { + while (copy.size != 0) { + const auto available = download.Size() - packed_size; + const auto prefix = copy.source_offset & 3u; + const auto bytes = std::min(copy.size, available - prefix); + DownloadCopy part {copy.buffer, copy.source_offset, copy.address, bytes}; + const auto [source_begin, envelope_size] = DownloadEnvelope(part); + (void)source_begin; + packed_size += AlignDownload(envelope_size); + batch.push_back(part); + copy.source_offset += bytes; + copy.address += bytes; + copy.size -= bytes; + if (packed_size == download.Size()) { + flush(); + } + } + } + if (!batch.empty()) { + flush(); + } for (const auto& copy: copies) { - const auto [source_begin, envelope_size] = DownloadEnvelope(copy); - (void)source_begin; - if (envelope_size > UINT64_MAX - (DOWNLOAD_ALIGNMENT - 1)) { - EXIT("BufferCache: download batch alignment overflow\n"); - } - const auto aligned_size = AlignDownload(envelope_size); - if (aligned_size > UINT64_MAX - reservation_size) { - EXIT("BufferCache: download batch overflow\n"); - } - reservation_size += aligned_size; + m_gpu_modified_ranges.Subtract(copy.address, copy.size); } - if (reservation_size == 0) { - return {}; - } - - auto& download = m_download_buffer; - const auto [mapped, base_offset] = download.Map(reservation_size, DOWNLOAD_ALIGNMENT); - if (mapped == nullptr) { - EXIT("BufferCache: download batch could not reserve the shared stream\n"); - } - - std::vector downloads; - downloads.reserve(copies.size()); - uint64_t cursor = 0; - for (const auto& copy: copies) { - const auto [source_begin, envelope_size] = DownloadEnvelope(copy); - const auto prefix = copy.source_offset - source_begin; - download.CopyFrom(m_scheduler.Current(), *copy.buffer, source_begin, base_offset + cursor, - envelope_size, vk::AccessFlagBits::eMemoryWrite, vk::AccessFlags {}, - vk::AccessFlagBits::eMemoryRead | vk::AccessFlagBits::eMemoryWrite, - vk::AccessFlagBits::eHostRead); - downloads.push_back({copy.address, copy.size, base_offset + cursor + prefix}); - cursor += AlignDownload(envelope_size); - } - download.Commit(); - return downloads; -} - -void BufferCache::PublishDownloads(std::span downloads) { - for (const auto& range: downloads) { - m_download_buffer.Invalidate(range.offset, range.size); - Libs::LibKernel::Memory::WriteBacking( - range.address, m_download_buffer.Mapped().data() + range.offset, range.size); - } -} - -void BufferCache::QueueGarbageDownload(std::span copies, BufferId id, - uint64_t address, uint64_t size) { - EXIT_IF(copies.empty()); - auto downloads = RecordDownloads(copies); - auto retired = downloads; - m_scheduler.DeferPriorityOperation( - [this, downloads = std::move(downloads)]() mutable { PublishDownloads(downloads); }); - m_scheduler.DeferOperation([this, retired = std::move(retired), id, address, size]() mutable { - for (const auto& range: retired) { - m_gpu_modified_ranges.Subtract(range.address, range.size); - } - // ForEachDownloadRange reports full tracker pages, and every exact GPU-owned - // interval on those pages was downloaded and removed. Clearing the original - // query therefore cannot orphan a dirty sibling on an edge page. - m_memory_tracker.UnmarkRegionAsGpuModified(address, size); - if (m_memory_tracker.IsRegionGpuModified(address, size) || - m_gpu_modified_ranges.Intersects(address, size)) { - EXIT("BufferCache: asynchronous garbage collection retained GPU ownership\n"); - } - m_memory_tracker.UntrackMemory(address, size); - EXIT_IF(!m_slot_buffers[id].is_deleted); - m_slot_buffers.erase(id); - }); } BufferCache::BufferCache(GraphicContext& graphics, CommandScheduler& scheduler, @@ -303,12 +280,7 @@ void BufferCache::ReadMemoryOnGpu(uint64_t vaddr, uint64_t size, bool is_write) m_memory_tracker.MarkRegionAsCpuModified(vaddr, size); return; } - auto downloads = RecordDownloads(copies); - m_scheduler.FinishCurrent(); - PublishDownloads(downloads); - for (const auto& range: downloads) { - m_gpu_modified_ranges.Subtract(range.address, range.size); - } + DownloadBufferMemory(copies); // The enumeration above covered whole dirty pages and every exact interval on them. m_memory_tracker.UnmarkRegionAsGpuModified(vaddr, size); if (is_write) { @@ -359,9 +331,7 @@ void BufferCache::UnmapMemory(uint64_t vaddr, uint64_t size) { }); } if (!copies.empty()) { - auto downloads = RecordDownloads(copies); - m_scheduler.FinishCurrent(); - PublishDownloads(downloads); + DownloadBufferMemory(copies); } for (const auto& [begin, bytes]: modified_buffers) { m_gpu_modified_ranges.Subtract(begin, bytes); @@ -784,10 +754,9 @@ void BufferCache::RunGarbageCollector() { return m_slot_buffers[m_buffers.at(left)].tick_accessed_last < m_slot_buffers[m_buffers.at(right)].tick_accessed_last; }); - if (candidates.size() > limit) { - candidates.resize(limit); - } - + std::vector dirty_buffers; + std::vector copies; + size_t retire_count = 0; for (const auto address: candidates) { auto found = m_buffers.find(address); EXIT_IF(found == m_buffers.end()); @@ -796,7 +765,9 @@ void BufferCache::RunGarbageCollector() { m_memory_tracker.ValidateGpuDirtyOwnership(m_gpu_modified_ranges, buffer.CpuAddress(), buffer.Size(), "garbage collection"); const bool dirty = m_memory_tracker.IsRegionGpuModified(buffer.CpuAddress(), buffer.Size()); - std::vector copies; + if (dirty && !aggressive) { + continue; + } if (dirty) { m_memory_tracker.ForEachDownloadRange( buffer.CpuAddress(), buffer.Size(), @@ -805,24 +776,37 @@ void BufferCache::RunGarbageCollector() { dirty_size, "garbage collection"); }, [&](uint64_t dirty_address, uint64_t dirty_size) noexcept { - for (const auto range: - m_gpu_modified_ranges.Intersections(dirty_address, dirty_size)) { + m_gpu_modified_ranges.ForEachIntersection( + dirty_address, dirty_size, [&](RangeSet::Range range) { copies.push_back({&buffer, range.address - buffer.CpuAddress(), range.address, range.size}); - } - }); - } - - const auto retire_address = buffer.CpuAddress(); - const auto retire_size = buffer.Size(); - if (dirty) { - EXIT_IF(copies.empty()); - Unregister(id); - QueueGarbageDownload(copies, id, retire_address, retire_size); + }); + }); + dirty_buffers.push_back(id); } else { - m_memory_tracker.UntrackMemory(retire_address, retire_size); + m_memory_tracker.UntrackMemory(buffer.CpuAddress(), buffer.Size()); DeleteBuffer(id); } + if (++retire_count == limit) { + break; + } + } + if (dirty_buffers.empty()) { + return; + } + + EXIT_IF(copies.empty()); + DownloadBufferMemory(copies); + for (const auto id: dirty_buffers) { + auto& buffer = m_slot_buffers[id]; + m_memory_tracker.UnmarkRegionAsGpuModified(buffer.CpuAddress(), buffer.Size()); + if (m_memory_tracker.IsRegionGpuModified(buffer.CpuAddress(), buffer.Size()) || + m_gpu_modified_ranges.Intersects(buffer.CpuAddress(), buffer.Size())) { + EXIT("BufferCache: garbage collection retained GPU ownership\n"); + } + m_memory_tracker.UntrackMemory(buffer.CpuAddress(), buffer.Size()); + Unregister(id); + m_slot_buffers.erase(id); } } diff --git a/src/graphics/host_gpu/renderer/cache/bufferCache.h b/src/graphics/host_gpu/renderer/cache/bufferCache.h index 9a7a35676..5a3d11716 100644 --- a/src/graphics/host_gpu/renderer/cache/bufferCache.h +++ b/src/graphics/host_gpu/renderer/cache/bufferCache.h @@ -66,7 +66,6 @@ private: friend struct BufferCacheTestAccess; struct DownloadCopy; - struct DownloadRange; using PageTable = MultiLevelPageTable; static_assert(CACHING_PAGE_SIZE == (uint64_t {1} << PageTable::kPageBits)); static constexpr uint64_t DOWNLOAD_ALIGNMENT = 64; @@ -84,10 +83,7 @@ private: [[nodiscard]] vk::Buffer UploadCopies(Buffer& buffer, std::span copies, uint64_t total_size); [[nodiscard]] bool SynchronizeBufferFromImage(Buffer& buffer, uint64_t vaddr, uint64_t size); - [[nodiscard]] std::vector RecordDownloads(std::span copies); - void PublishDownloads(std::span downloads); - void QueueGarbageDownload(std::span copies, BufferId id, uint64_t address, - uint64_t size); + void DownloadBufferMemory(std::span copies); void WriteHostMemory(uint64_t vaddr, std::span data); void ReadMemoryOnGpu(uint64_t vaddr, uint64_t size, bool is_write); diff --git a/tests/ShaderRecompilerComputeTests.cpp b/tests/ShaderRecompilerComputeTests.cpp index 8b1d1bfe7..013da0fc6 100644 --- a/tests/ShaderRecompilerComputeTests.cpp +++ b/tests/ShaderRecompilerComputeTests.cpp @@ -161,6 +161,10 @@ struct BufferCacheTestAccess { address >> BufferCache::PageTable::kPageBits); return owner == nullptr ? BufferId{} : *owner; } + + static bool IsBufferAllocated(const BufferCache &cache, BufferId id) { + return cache.m_slot_buffers.is_allocated(id); + } }; struct StreamBufferTestAccess { @@ -3113,11 +3117,6 @@ public: cache, 0, std::numeric_limits::max()); const auto gc_submission_tick = scheduler.CurrentTick(); cache.RunGarbageCollector(); - Require(name, "dirty retirement", - !cache.IsRegionRegistered(base, allocation_size), - "aged GPU-dirty buffer survived pressured " - "collection"); - uint32_t first_before_completion = 0; uint32_t second_before_completion = 0; Libs::LibKernel::Memory::TryReadBacking(base + first_offset, @@ -3126,9 +3125,10 @@ public: Libs::LibKernel::Memory::TryReadBacking(base + second_offset, &second_before_completion, sizeof(second_before_completion)); - Require(name, "deferred dirty retirement", + Require(name, "normal dirty retention", first_before_completion == first_stale && second_before_completion == second_stale && + cache.IsRegionRegistered(base, allocation_size) && cache.IsRegionGpuModified(base + first_offset, sizeof(first_value)) && cache.IsRegionGpuModified(base + second_offset, @@ -3138,9 +3138,32 @@ public: cache.HasGpuDirtyBytes(base + second_offset, sizeof(second_value)) && scheduler.CurrentTick() == gc_submission_tick, - "buffer GC published bytes or cleared dirty " - "ownership before GPU completion"); - scheduler.FinishCurrent(); + "normal GC retired or synchronized a dirty Buffer"); + BufferCacheTestAccess::SetGarbageCollectionThresholds(cache, 0, 0); + const auto older_publication_tick = scheduler.CurrentTick(); + std::binary_semaphore older_publication_entered{0}; + std::binary_semaphore release_older_publication{0}; + std::atomic gc_returned{false}; + scheduler.DeferPriorityOperation([&] { + older_publication_entered.release(); + release_older_publication.acquire(); + Libs::LibKernel::Memory::WriteBacking( + base + first_offset, &first_stale, sizeof(first_stale)); + }); + std::jthread release_publication([&] { + older_publication_entered.acquire(); + const auto deadline = std::chrono::steady_clock::now() + + std::chrono::milliseconds(100); + while (!gc_returned.load() && + std::chrono::steady_clock::now() < deadline) { + std::this_thread::yield(); + } + release_older_publication.release(); + }); + cache.RunGarbageCollector(); + gc_returned = true; + release_publication.join(); + scheduler.WaitPriorityOperations(older_publication_tick); uint32_t first_backing = 0; uint32_t second_backing = 0; @@ -3152,6 +3175,7 @@ public: Require(name, "downloaded contents", first_backing == first_value && second_backing == second_value && clean_backing == clean_value && + !cache.IsRegionRegistered(base, allocation_size) && !cache.IsRegionGpuModified(base + first_offset, sizeof(first_value)) && !cache.IsRegionGpuModified(base + second_offset, @@ -3159,9 +3183,65 @@ public: !cache.HasGpuDirtyBytes(base + first_offset, sizeof(first_value)) && !cache.HasGpuDirtyBytes(base + second_offset, - sizeof(second_value)), - "dirty GC did not publish exact ranges before " - "clearing broad page ownership"); + sizeof(second_value)) && + scheduler.CurrentTick() > gc_submission_tick, + "critical GC did not publish exact ranges before clearing " + "Buffer ownership"); + + constexpr uint64_t starvation_offset = 0x100000; + constexpr uint64_t starvation_stride = 2 * BufferCache::CACHING_PAGE_SIZE; + constexpr uint32_t starvation_count = 33; + constexpr uint32_t starvation_value = 0x31415926u; + BufferCacheTestAccess::SetGarbageCollectionThresholds( + cache, std::numeric_limits::max(), + std::numeric_limits::max()); + for (uint32_t index = 0; index < starvation_count; index++) { + const auto address = base + starvation_offset + index * starvation_stride; + (void)cache.ObtainBuffer(address, sizeof(starvation_value), true); + cache.FillBuffer(address, sizeof(starvation_value), starvation_value, + false); + } + for (uint32_t tick = 0; tick < 160; tick++) { + cache.RunGarbageCollector(); + } + constexpr uint64_t starvation_clean_offset = + starvation_offset + starvation_count * starvation_stride; + const auto starvation_clean = cache.FindBuffer( + base + starvation_clean_offset, sizeof(starvation_value)); + Require(name, "normal-GC clean candidate", + static_cast(starvation_clean), + "failed to create the clean starvation candidate"); + for (uint32_t tick = 0; tick <= 160; tick++) { + cache.RunGarbageCollector(); + } + BufferCacheTestAccess::SetGarbageCollectionThresholds( + cache, 0, std::numeric_limits::max()); + cache.RunGarbageCollector(); + Require( + name, "normal-GC dirty bypass", + cache.IsRegionRegistered(base + starvation_offset, + sizeof(starvation_value)) && + cache.IsRegionRegistered( + base + starvation_offset + + (starvation_count - 1) * starvation_stride, + sizeof(starvation_value)) && + !cache.IsRegionRegistered(base + starvation_clean_offset, + sizeof(starvation_value)), + "old dirty owners hid an eligible clean Buffer from normal GC"); + BufferCacheTestAccess::SetGarbageCollectionThresholds(cache, 0, 0); + const auto starvation_retired = + BufferCacheTestAccess::PageOwner(cache, base + starvation_offset); + cache.RunGarbageCollector(); + Require(name, "critical-GC starvation cleanup", + !cache.IsRegionRegistered(base + starvation_offset, + sizeof(starvation_value)) && + !cache.IsRegionRegistered( + base + starvation_offset + + (starvation_count - 1) * starvation_stride, + sizeof(starvation_value)) && + !BufferCacheTestAccess::IsBufferAllocated( + cache, starvation_retired), + "critical GC did not immediately free the skipped dirty owners"); auto unmap_allocation = cache.ObtainBuffer( base + unmap_offset, sizeof(unmap_value), true, false); @@ -3224,7 +3304,7 @@ public: DestroyBuffer(&partial_unmap_readback); constexpr uint64_t large_offset = 0x10000; - constexpr uint64_t large_size = 31ull * 1024 * 1024; + constexpr uint64_t large_size = 33ull * 1024 * 1024; constexpr uint32_t large_value = 0x5aa55aa5u; constexpr uint32_t large_stale = 0x12345678u; std::memcpy(memory + large_offset, &large_stale, sizeof(large_stale)); @@ -3239,17 +3319,16 @@ public: for (uint32_t tick = 0; tick <= 160; tick++) { cache.RunGarbageCollector(); } - Require(name, "near-capacity deferred retirement", + Require(name, "near-capacity synchronized retirement", !cache.IsRegionRegistered(base + large_offset, large_size), "near-capacity dirty Buffer survived pressured collection"); uint32_t large_before_completion = 0; Libs::LibKernel::Memory::TryReadBacking(base + large_offset, &large_before_completion, sizeof(large_before_completion)); - Require(name, "near-capacity backing remained deferred", - large_before_completion == large_stale, - "near-capacity Buffer GC published before its scheduler tick"); - scheduler.FinishCurrent(); + Require(name, "near-capacity backing publication", + large_before_completion == large_value, + "near-capacity Buffer GC retired ownership before publication"); const auto large_image_source = cache.ObtainBufferForImage(base + large_offset, sizeof(large_value)); Require(name, "fixed download after Buffer retirement", @@ -3307,7 +3386,6 @@ public: fixed_download && fixed_download->Handle() == fixed_download_handle, "GC aggregated disjoint retirees or replaced the download ring"); - scheduler.FinishCurrent(); uint32_t grouped_first_backing = 0; uint32_t grouped_second_backing = 0; Libs::LibKernel::Memory::TryReadBacking(base + grouped_first_offset, @@ -3341,7 +3419,7 @@ public: for (uint32_t tick = 0; tick <= 160; tick++) { cache.RunGarbageCollector(); } - Require(name, "disjoint deferred retirement", + Require(name, "disjoint synchronized retirement", !cache.IsRegionRegistered(base + disjoint_owner_offset, disjoint_owner_size), "cross-page owner survived pressured collection"); @@ -3350,10 +3428,9 @@ public: &disjoint_before_unmap, sizeof(disjoint_before_unmap)); Require( - name, "disjoint retirement remained deferred", - disjoint_before_unmap == disjoint_stale, - "whole-owner Buffer publication completed before synchronization"); - scheduler.FinishCurrent(); + name, "disjoint retirement publication", + disjoint_before_unmap == disjoint_value, + "whole-owner Buffer retired before publishing its dirty bytes"); cache.UnmapMemory(base + disjoint_owner_offset + 0x4000, 0x4000); uint32_t disjoint_after_unmap = 0; Libs::LibKernel::Memory::TryReadBacking(base + disjoint_dirty_offset, @@ -3403,7 +3480,7 @@ public: for (uint32_t tick = 0; tick <= 160; tick++) { cache.RunGarbageCollector(); } - Require(name, "reacquire deferred retirement", + Require(name, "reacquire synchronized retirement", !cache.IsRegionRegistered(base + reacquire_owner_offset, reacquire_owner_size), "fresh cross-page owner survived pressured collection"); @@ -3411,10 +3488,9 @@ public: Libs::LibKernel::Memory::TryReadBacking(base + reacquire_dirty_offset, &reacquire_before, sizeof(reacquire_before)); - Require(name, "reacquire publication remained deferred", - reacquire_before == reacquire_stale, - "fresh whole-owner publication completed before reacquisition"); - scheduler.FinishCurrent(); + Require(name, "reacquire publication completed", + reacquire_before == reacquire_value, + "fresh whole-owner ownership retired before publication"); auto reacquired = cache.ObtainBuffer(base + reacquire_disjoint_offset, sizeof(reacquire_value), true, false);