gpu: move the BDA sync queue from MemoryTracker into BufferCache

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<true>(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<DirtySource::Cpu, true>(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<false>(vaddr, size, [](RegionManager* manager, uint64_t offset, uint64_t bytes) {
 		manager->ChangeState<DirtySource::Cpu, true>(manager->GetCpuAddr() + offset, bytes);
 	});
-	locks.clear();
-	QueueBdaSync(vaddr, size);
 }
 
 } // namespace Libs::Graphics

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 <typename Flush>
 	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<std::atomic<RegionManager*>[]> m_regions;
 	std::vector<std::unique_ptr<RegionManager>>    m_region_storage;
 	std::mutex                                     m_region_mutex;
-	std::mutex                                     m_bda_sync_mutex;
-	RangeSet                                       m_bda_sync;
 	PageManager&                                   m_page_manager;
 };
 

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) {
+void BufferCache::QueueBdaSync(uint64_t vaddr, uint64_t size) {
-	// Take before walking: writes queued during synchronization remain pending.
+	const auto begin = Common::AlignDown(vaddr, TRACKER_PAGE_SIZE);
-	const auto pending = m_memory_tracker.TakeBdaSync();
+	const auto end   = Common::AlignUp(vaddr + size, TRACKER_PAGE_SIZE);
-	pending.ForEach([&](uint64_t begin, uint64_t end) {
+	std::lock_guard lock(m_bda_sync_mutex);
-		mapped.ForEachInRange(begin, end - begin, [this](uint64_t start, uint64_t finish) {
+	m_bda_sync.Add(begin, end - begin);
-			SynchronizeBuffersInRange(start, finish - start);
+}
-		});
+
-	});
+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) {

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

@@ -13,6 +13,7 @@
 
 #include <array>
 #include <map>
+#include <mutex>
 #include <span>
 #include <utility>
 #include <vector>
@@ -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               SynchronizeBuffersInRange(uint64_t vaddr, uint64_t size);
-	void               QueueBdaSync(uint64_t vaddr, uint64_t size) {
+	// Queue after the range's state change becomes visible, or a drain could consume it first.
-		m_memory_tracker.QueueBdaSync(vaddr, size);
+	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<vk::BufferCopy> 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<RangeSet, 8>                           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;

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;
 }
 

tests/MemoryTrackerTests.cpp

@@ -213,12 +213,6 @@ struct ProtectionCall {
 std::vector<ProtectionCall> 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<uint64_t>(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<uint64_t>(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();

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(
+    // Direct memory, so readbacks can publish GPU-dirty bytes.
-        0, 3 * page, Common::VirtualMemory::Mode::ReadWrite, "bda_sync_uploads");
+    int64_t direct = -1;
-    Require(name, "allocation", base != 0, "guest allocation failed");
+    void *mapped = nullptr;
-    auto *words = reinterpret_cast<u32 *>(base);
+    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<uint64_t>(mapped);
+    auto *words = static_cast<u32 *>(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<uint64_t>::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);
   }