renderer: synchronize bounded buffer downloads

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<uint64_t, uint64_t> BufferCache::DownloadEnvelope(const DownloadCopy&
 	return {begin, end - begin};
 }
 
-std::vector<BufferCache::DownloadRange>
+void BufferCache::DownloadBufferMemory(std::span<const DownloadCopy> copies) {
-BufferCache::RecordDownloads(std::span<const DownloadCopy> copies) {
+	std::vector<DownloadCopy> batch;
-	uint64_t reservation_size = 0;
+	batch.reserve(copies.size());
-	for (const auto& copy: copies) {
+	uint64_t                  packed_size = 0;
-		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;
-	}
-	if (reservation_size == 0) {
-		return {};
-	}
-
 	auto&                     download    = m_download_buffer;
-	const auto [mapped, base_offset] = download.Map(reservation_size, DOWNLOAD_ALIGNMENT);
+	const auto flush = [&] {
-	if (mapped == nullptr) {
+		const auto [mapped, base_offset] = download.Map(packed_size, DOWNLOAD_ALIGNMENT);
-		EXIT("BufferCache: download batch could not reserve the shared stream\n");
+		EXIT_IF(mapped == nullptr);
-	}
-
-	std::vector<DownloadRange> downloads;
-	downloads.reserve(copies.size());
 		uint64_t cursor = 0;
-	for (const auto& copy: copies) {
+		for (const auto& copy: batch) {
 			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;
+		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();
-void BufferCache::PublishDownloads(std::span<const DownloadRange> downloads) {
+		packed_size = 0;
-	for (const auto& range: downloads) {
+	};
-		m_download_buffer.Invalidate(range.offset, range.size);
+	for (auto copy: copies) {
-		Libs::LibKernel::Memory::WriteBacking(
+		while (copy.size != 0) {
-		    range.address, m_download_buffer.Mapped().data() + range.offset, range.size);
+			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();
 			}
 		}
-
-void BufferCache::QueueGarbageDownload(std::span<const DownloadCopy> 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
+	if (!batch.empty()) {
-		// interval on those pages was downloaded and removed. Clearing the original
+		flush();
-		// query therefore cannot orphan a dirty sibling on an edge page.
+	}
-		m_memory_tracker.UnmarkRegionAsGpuModified(address, size);
+	for (const auto& copy: copies) {
-		if (m_memory_tracker.IsRegionGpuModified(address, size) ||
+		m_gpu_modified_ranges.Subtract(copy.address, copy.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);
+	DownloadBufferMemory(copies);
-	m_scheduler.FinishCurrent();
-	PublishDownloads(downloads);
-	for (const auto& range: downloads) {
-		m_gpu_modified_ranges.Subtract(range.address, range.size);
-	}
 	// 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);
+		DownloadBufferMemory(copies);
-		m_scheduler.FinishCurrent();
-		PublishDownloads(downloads);
 	}
 	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) {
+	std::vector<BufferId> dirty_buffers;
-		candidates.resize(limit);
+	std::vector<DownloadCopy> 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<DownloadCopy> copies;
+		if (dirty && !aggressive) {
+			continue;
+		}
 		if (dirty) {
 			m_memory_tracker.ForEachDownloadRange<false>(
 			    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.ForEachIntersection(
-				         m_gpu_modified_ranges.Intersections(dirty_address, dirty_size)) {
+				        dirty_address, dirty_size, [&](RangeSet::Range range) {
 					    copies.push_back({&buffer, range.address - buffer.CpuAddress(),
 					                      range.address, range.size});
-				    }
 				        });
-		}
+				});
-
+			dirty_buffers.push_back(id);
-		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);
 		} 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);
 	}
 }
 

src/graphics/host_gpu/renderer/cache/bufferCache.h

@@ -66,7 +66,6 @@ private:
 	friend struct BufferCacheTestAccess;
 
 	struct DownloadCopy;
-	struct DownloadRange;
 	using PageTable = MultiLevelPageTable<BufferId, 14, 40, 16>;
 	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<vk::BufferCopy> copies,
 	                                      uint64_t total_size);
 	[[nodiscard]] bool SynchronizeBufferFromImage(Buffer& buffer, uint64_t vaddr, uint64_t size);
-	[[nodiscard]] std::vector<DownloadRange> RecordDownloads(std::span<const DownloadCopy> copies);
+	void DownloadBufferMemory(std::span<const DownloadCopy> copies);
-	void PublishDownloads(std::span<const DownloadRange> downloads);
-	void QueueGarbageDownload(std::span<const DownloadCopy> copies, BufferId id, uint64_t address,
-	                          uint64_t size);
 	void WriteHostMemory(uint64_t vaddr, std::span<const uint8_t> data);
 	void ReadMemoryOnGpu(uint64_t vaddr, uint64_t size, bool is_write);
 

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<uint64_t>::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 "
+              "normal GC retired or synchronized a dirty Buffer");
-              "ownership before GPU completion");
+      BufferCacheTestAccess::SetGarbageCollectionThresholds(cache, 0, 0);
-      scheduler.FinishCurrent();
+      const auto older_publication_tick = scheduler.CurrentTick();
+      std::binary_semaphore older_publication_entered{0};
+      std::binary_semaphore release_older_publication{0};
+      std::atomic<bool> 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)),
+                                          sizeof(second_value)) &&
-              "dirty GC did not publish exact ranges before "
+                  scheduler.CurrentTick() > gc_submission_tick,
-              "clearing broad page ownership");
+              "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<uint64_t>::max(),
+          std::numeric_limits<uint64_t>::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<bool>(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<uint64_t>::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",
+      Require(name, "near-capacity backing publication",
-              large_before_completion == large_stale,
+              large_before_completion == large_value,
-              "near-capacity Buffer GC published before its scheduler tick");
+              "near-capacity Buffer GC retired ownership before publication");
-      scheduler.FinishCurrent();
       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",
+          name, "disjoint retirement publication",
-          disjoint_before_unmap == disjoint_stale,
+          disjoint_before_unmap == disjoint_value,
-          "whole-owner Buffer publication completed before synchronization");
+          "whole-owner Buffer retired before publishing its dirty bytes");
-      scheduler.FinishCurrent();
       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",
+      Require(name, "reacquire publication completed",
-              reacquire_before == reacquire_stale,
+              reacquire_before == reacquire_value,
-              "fresh whole-owner publication completed before reacquisition");
+              "fresh whole-owner ownership retired before publication");
-      scheduler.FinishCurrent();
       auto reacquired =
           cache.ObtainBuffer(base + reacquire_disjoint_offset,
                              sizeof(reacquire_value), true, false);