renderer: remove global BufferCache locking

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

@@ -22,30 +22,9 @@ namespace Libs::Graphics {
 
 namespace {
 
-thread_local const void* g_cache_lock_owner = nullptr;
-
 constexpr uint64_t MiB           = 1024 * 1024;
 constexpr uint64_t GdsBufferSize = 64 * 1024;
 
-class FaultSafeCacheLock final {
-public:
-	FaultSafeCacheLock(const void* owner, Common::Mutex& mutex): m_mutex(mutex) {
-		if (g_cache_lock_owner != nullptr) {
-			EXIT("BufferCache: recursive cache lock acquisition\n");
-		}
-		g_cache_lock_owner = owner;
-		m_mutex.Lock();
-	}
-
-	~FaultSafeCacheLock() {
-		m_mutex.Unlock();
-		g_cache_lock_owner = nullptr;
-	}
-
-private:
-	Common::Mutex& m_mutex;
-};
-
 } // namespace
 
 uint64_t BufferCache::AlignDown(uint64_t value) noexcept {
@@ -191,8 +170,6 @@ void BufferCache::QueueGarbageDownload(std::span<const DownloadCopy> copies, Ret
 	m_scheduler.DeferOperation(
 	    [this, downloads = std::move(downloads), retire = std::move(retire)]() mutable {
 		    PublishDownloads(downloads);
-		    {
-			    FaultSafeCacheLock lock(this, m_mutex);
 		    for (const auto& range: downloads) {
 			    m_gpu_modified_ranges.Subtract(range.address, range.size);
 		    }
@@ -205,7 +182,6 @@ void BufferCache::QueueGarbageDownload(std::span<const DownloadCopy> copies, Ret
 			    EXIT("BufferCache: asynchronous garbage collection retained GPU ownership\n");
 		    }
 		    m_memory_tracker.UntrackMemory(retire.address, retire.size);
-		    }
 	    });
 }
 
@@ -276,9 +252,6 @@ void BufferCache::InvalidateMemory(uint64_t vaddr, uint64_t size) {
 	    size > TRACKER_ADDRESS_SIZE - vaddr) {
 		EXIT("BufferCache: invalid memory-invalidation range\n");
 	}
-	if (!IsRegionRegistered(vaddr, size)) {
-		return;
-	}
 	m_memory_tracker.InvalidateRegion(vaddr, size,
 	                                  [this, vaddr, size] { ReadMemory(vaddr, size, true); });
 }
@@ -298,9 +271,12 @@ void BufferCache::ReadMemory(uint64_t vaddr, uint64_t size, bool is_write) {
 }
 
 void BufferCache::ReadMemoryOnGpu(uint64_t vaddr, uint64_t size, bool is_write) {
+	// CPU invalidation reaches this point only for a GPU-owned tracker page. Resolve the exact
+	// Buffer owner on the GPU thread so the cache index remains single-thread-owned.
+	if (is_write && !IsRegionRegistered(vaddr, size)) {
+		return;
+	}
 	std::vector<DownloadCopy> copies;
-	{
-		FaultSafeCacheLock lock(this, m_mutex);
 	m_memory_tracker.ForEachDownloadRange<false>(
 	    vaddr, size,
 	    [&](uint64_t address, uint64_t bytes) noexcept {
@@ -317,11 +293,10 @@ void BufferCache::ReadMemoryOnGpu(uint64_t vaddr, uint64_t size, bool is_write)
 				    }
 				    auto& cached = *std::prev(owner)->second;
 				    if (!cached.buffer->IsInBounds(copy_address, 1)) {
-						    EXIT(
+					    EXIT("BufferCache: invalidation readback is outside its buffer owner\n");
-						        "BufferCache: invalidation readback is outside its buffer owner\n");
 				    }
-					    const auto copy_size = std::min(range.size - copied,
+				    const auto copy_size =
-					                                    cached.vaddr + cached.size - copy_address);
+				        std::min(range.size - copied, cached.vaddr + cached.size - copy_address);
 				    copies.push_back({cached.buffer, cached.buffer->Offset(copy_address),
 				                      copy_address, copy_size});
 				    copied += copy_size;
@@ -332,18 +307,15 @@ void BufferCache::ReadMemoryOnGpu(uint64_t vaddr, uint64_t size, bool is_write)
 		if (!is_write) {
 			return;
 		}
-			// A preceding read fault can consume the last GPU-owned copy after this write
+		// A preceding read fault can consume the last GPU-owned copy after this write invalidation
-			// invalidation has already chosen to flush. Complete the CPU ownership handoff even
+		// has already chosen to flush. Complete the CPU ownership handoff even though this callback
-			// though this callback no longer has bytes to download.
+		// no longer has bytes to download.
 		m_memory_tracker.MarkRegionAsCpuModified(vaddr, size);
 		return;
 	}
-	}
 	auto downloads = RecordDownloads(copies);
 	m_scheduler.FinishCurrent();
 	PublishDownloads(downloads);
-	{
-		FaultSafeCacheLock lock(this, m_mutex);
 	for (const auto& range: downloads) {
 		m_gpu_modified_ranges.Subtract(range.address, range.size);
 	}
@@ -353,7 +325,6 @@ void BufferCache::ReadMemoryOnGpu(uint64_t vaddr, uint64_t size, bool is_write)
 		m_memory_tracker.MarkRegionAsCpuModified(vaddr, size);
 	}
 }
-}
 
 void BufferCache::UnmapMemory(uint64_t vaddr, uint64_t size) {
 	if (vaddr == 0 || size == 0 || size > UINT64_MAX - vaddr) {
@@ -362,8 +333,6 @@ void BufferCache::UnmapMemory(uint64_t vaddr, uint64_t size) {
 	std::vector<DownloadCopy>                  copies;
 	std::vector<std::pair<uint64_t, uint64_t>> modified_buffers;
 	std::vector<std::pair<uint64_t, uint64_t>> retired_buffers;
-	{
-		FaultSafeCacheLock lock(this, m_mutex);
 	for (const auto& [begin, cached]: m_buffers) {
 		if (vaddr < begin + cached->size && begin < vaddr + size) {
 			retired_buffers.emplace_back(begin, cached->size);
@@ -399,7 +368,6 @@ void BufferCache::UnmapMemory(uint64_t vaddr, uint64_t size) {
 			    }
 		    });
 	}
-	}
 	if (!copies.empty()) {
 		auto downloads = RecordDownloads(copies);
 		m_scheduler.FinishCurrent();
@@ -409,8 +377,6 @@ void BufferCache::UnmapMemory(uint64_t vaddr, uint64_t size) {
 		// command stream before removing such backing.
 		m_scheduler.FinishCurrent();
 	}
-	{
-		FaultSafeCacheLock lock(this, m_mutex);
 	for (const auto& [begin, bytes]: modified_buffers) {
 		m_gpu_modified_ranges.Subtract(begin, bytes);
 		m_memory_tracker.UnmarkRegionAsGpuModified(begin, bytes);
@@ -434,7 +400,6 @@ void BufferCache::UnmapMemory(uint64_t vaddr, uint64_t size) {
 		}
 	}
 }
-}
 
 BufferCache::CachedBuffer& BufferCache::GetOrCreateBuffer(CommandBuffer& command, uint64_t vaddr,
                                                           uint64_t size) {
@@ -539,17 +504,11 @@ BufferBinding BufferCache::ObtainBuffer(CommandBuffer& command, uint64_t vaddr,
 		(void)m_texture_cache.InvalidateMemoryFromGPU(vaddr, size, true);
 	}
 
-	std::shared_ptr<Buffer> owner;
-	uint64_t                offset = 0;
-	{
-		FaultSafeCacheLock                         lock(this, m_mutex);
 	auto&                                      cached = GetOrCreateBuffer(command, vaddr, size);
 	std::vector<std::pair<uint64_t, uint64_t>> uploads;
 	m_memory_tracker.ForEachUploadRange(
 	    vaddr, size, is_written,
-		    [&](uint64_t address, uint64_t bytes) noexcept {
+	    [&](uint64_t address, uint64_t bytes) noexcept { uploads.emplace_back(address, bytes); },
-			    uploads.emplace_back(address, bytes);
-		    },
 	    [&]() noexcept {
 		    for (const auto& [address, bytes]: uploads) {
 			    Upload(command, *cached.buffer, cached.buffer->Offset(address),
@@ -559,9 +518,8 @@ BufferBinding BufferCache::ObtainBuffer(CommandBuffer& command, uint64_t vaddr,
 	if (is_written) {
 		m_gpu_modified_ranges.Add(vaddr, size);
 	}
-		owner  = cached.buffer;
+	auto     owner  = cached.buffer;
-		offset = cached.buffer->Offset(vaddr);
+	uint64_t offset = cached.buffer->Offset(vaddr);
-	}
 	if (is_formatted && is_read && !is_written) {
 		(void)SynchronizeBufferFromImage(*owner, vaddr, size);
 	}
@@ -569,19 +527,14 @@ BufferBinding BufferCache::ObtainBuffer(CommandBuffer& command, uint64_t vaddr,
 }
 
 std::shared_ptr<Buffer> BufferCache::ObtainNullBuffer() {
-	std::shared_ptr<Buffer> buffer;
-	{
-		FaultSafeCacheLock lock(this, m_mutex);
 	if (m_null_buffer != nullptr) {
 		return m_null_buffer;
 	}
-		m_null_buffer = std::make_shared<Buffer>(m_graphics, m_scheduler, MemoryUsage::DeviceLocal,
+	m_null_buffer = std::make_shared<Buffer>(m_graphics, m_scheduler, MemoryUsage::DeviceLocal, 0,
-		                                         0, AllFlags, 16);
+	                                         AllFlags, 16);
-		buffer        = m_null_buffer;
-	}
 	const std::array<uint8_t, 16> zeros {};
-	Upload(m_scheduler.Current(), *buffer, 0, zeros.data(), zeros.size());
+	Upload(m_scheduler.Current(), *m_null_buffer, 0, zeros.data(), zeros.size());
-	return buffer;
+	return m_null_buffer;
 }
 
 ImageBufferSource BufferCache::ObtainBufferForImage(uint64_t vaddr, uint64_t size) {
@@ -599,7 +552,6 @@ ImageBufferSource BufferCache::ObtainBufferForImage(uint64_t vaddr, uint64_t siz
 	};
 
 	{
-		FaultSafeCacheLock lock(this, m_mutex);
 		const bool cpu_modified            = m_memory_tracker.IsRegionCpuModified(vaddr, size);
 		const bool gpu_modified            = m_memory_tracker.IsRegionGpuModified(vaddr, size);
 		const auto dirty                   = m_gpu_modified_ranges.Intersections(vaddr, size);
@@ -674,7 +626,6 @@ ImageBufferSource BufferCache::ObtainBufferForImage(uint64_t vaddr, uint64_t siz
 	}
 	m_staging_buffer.Commit();
 
-	FaultSafeCacheLock lock(this, m_mutex);
 	const auto dirty                   = m_gpu_modified_ranges.Intersections(vaddr, size);
 	const bool has_dirty_buffer_source = !dirty.empty();
 	auto       owner                   = find_owner();
@@ -710,7 +661,6 @@ void BufferCache::WriteHostMemory(uint64_t vaddr, std::span<const uint8_t> data)
 	}
 	Libs::LibKernel::Memory::WriteBacking(vaddr, data.data(), data.size());
 
-	FaultSafeCacheLock lock(this, m_mutex);
 	const auto end = vaddr + data.size();
 	for (auto& [address, cached]: m_buffers) {
 		const auto cached_end = address + cached->size;
@@ -836,7 +786,6 @@ bool BufferCache::IsRegionRegistered(uint64_t vaddr, uint64_t size) {
 	    size > TRACKER_ADDRESS_SIZE - vaddr) {
 		EXIT("BufferCache: invalid registered-region query\n");
 	}
-	FaultSafeCacheLock lock(this, m_mutex);
 	// Cached buffers are ordered and non-overlapping. The last buffer beginning before the query
 	// end is therefore the only possible intersection.
 	const auto candidate = m_buffers.lower_bound(vaddr + size);
@@ -852,7 +801,6 @@ bool BufferCache::IsRegionGpuModified(uint64_t vaddr, uint64_t size) {
 }
 
 bool BufferCache::HasGpuDirtyBytes(uint64_t vaddr, uint64_t size) {
-	FaultSafeCacheLock lock(this, m_mutex);
 	return !m_gpu_modified_ranges.Intersections(vaddr, size).empty();
 }
 
@@ -876,7 +824,6 @@ void BufferCache::RunGarbageCollector() {
 	std::vector<RetiredBuffer>                                       retires;
 	std::vector<std::pair<RetiredBuffer, std::vector<DownloadCopy>>> dirty_retires;
 	{
-		FaultSafeCacheLock    lock(this, m_mutex);
 		std::vector<uint64_t> candidates;
 		for (const auto& [address, owner]: m_buffers) {
 			const auto& cached = *owner;
@@ -924,7 +871,6 @@ void BufferCache::RunGarbageCollector() {
 		QueueGarbageDownload(copies, std::move(retire));
 	}
 
-	FaultSafeCacheLock lock(this, m_mutex);
 	for (const auto& retire: retires) {
 		auto found = m_buffers.find(retire.address);
 		if (found == m_buffers.end() || found->second->size != retire.size ||

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

@@ -3,7 +3,6 @@
 
 #include "common/abi.h"
 #include "common/common.h"
-#include "common/threads.h"
 #include "graphics/host_gpu/memoryTracker.h"
 #include "graphics/host_gpu/rangeSet.h"
 #include "graphics/host_gpu/renderer/cache/streamBuffer.h"
@@ -60,6 +59,7 @@ public:
 	void FillBuffer(uint64_t vaddr, uint64_t size, uint32_t value, bool is_gds = false);
 	void CopyBuffer(uint64_t dst_vaddr, uint64_t src_vaddr, uint64_t size, bool dst_gds = false,
 	                bool src_gds = false);
+	// Cache-index and exact dirty-range queries require GPU-thread serialization.
 	[[nodiscard]] bool IsRegionRegistered(uint64_t vaddr, uint64_t size);
 	[[nodiscard]] bool HasGpuDirtyBytes(uint64_t vaddr, uint64_t size);
 	[[nodiscard]] bool IsRegionCpuModified(uint64_t vaddr, uint64_t size);
@@ -99,7 +99,6 @@ private:
 	GraphicContext&                                   m_graphics;
 	CommandScheduler&                                 m_scheduler;
 	Buffer                                            m_gds_buffer;
-	Common::Mutex                                     m_mutex;
 	std::shared_ptr<Buffer>                           m_null_buffer;
 	std::map<uint64_t, std::unique_ptr<CachedBuffer>> m_buffers;
 	RangeSet                                          m_gpu_modified_ranges;

tests/ShaderRecompilerComputeTests.cpp

@@ -1642,6 +1642,7 @@ public:
 			auto&              gpu_scheduler = context.GetCommandScheduler();
 			auto               processor     = std::make_unique<CommandProcessor>(context);
 			gpu.SendCommandSync([&] {
+				processor->BufferInit();
 				SubmitInfo blocked_submit;
 				blocked_submit.AddWait(blocked_timeline, release_tick);
 				gpu_scheduler.Flush(blocked_submit);
@@ -3228,40 +3229,17 @@ public:
 			        "prefix, or transferred ownership");
 
 			auto& buffer_cache = resources.GetBufferCache();
-			Require(name, "formatted lock-handoff fixture",
-			        !buffer_cache.IsRegionRegistered(mip_desc.info.data.address,
-			                                          mip_desc.info.data.size),
-			        "formatted image already had a cached Buffer owner");
 			BufferBinding mip_formatted;
-			std::binary_semaphore obtain_entered {0};
+			bool          formatted_owner_was_absent = false;
-			std::binary_semaphore obtain_complete {0};
+			gpu.SendCommandSync([&] {
-			std::binary_semaphore buffer_visible {0};
+				formatted_owner_was_absent =
-			auto                  texture_lock = TextureCacheTestAccess::Lock(texture_cache);
+				    !buffer_cache.IsRegionRegistered(mip_desc.info.data.address,
-			gpu.SendCommand([&] {
+				                                     mip_desc.info.data.size);
-				obtain_entered.release();
 				mip_formatted = buffer_cache.ObtainBuffer(command, mip_desc.info.data.address,
 				                                            mip_desc.info.data.size, false, true, true);
-				obtain_complete.release();
 			});
-			obtain_entered.acquire();
+			Require(name, "formatted cache fixture", formatted_owner_was_absent,
-			std::jthread buffer_probe([&] {
+			        "formatted image already had a cached Buffer owner");
-				while (!buffer_cache.IsRegionRegistered(mip_desc.info.data.address,
-				                                         mip_desc.info.data.size)) {
-					std::this_thread::yield();
-				}
-				buffer_visible.release();
-			});
-			const bool buffer_released_before_texture =
-			    buffer_visible.try_acquire_for(std::chrono::seconds(2));
-			texture_lock.unlock();
-			if (!buffer_released_before_texture) {
-				buffer_visible.acquire();
-			}
-			obtain_complete.acquire();
-			buffer_probe.join();
-			Require(name, "formatted cache lock handoff", buffer_released_before_texture,
-			        "formatted Buffer acquisition retained the BufferCache lock while waiting for "
-			        "TextureCache");
 			Require(name, "formatted Buffer path",
 			        mip_formatted.owner != nullptr && mip_formatted.buffer != nullptr &&
 			            texture_cache.GetImage(mip_image).IsGpuModified(),