renderer: invalidate buffers on memory unmap

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

@@ -296,71 +296,6 @@ void BufferCache::ReadMemoryOnGpu(uint64_t vaddr, uint64_t size, bool is_write)
 	}
 }
 
-void BufferCache::UnmapMemory(uint64_t vaddr, uint64_t size) {
-	if (vaddr == 0 || size == 0 || size > UINT64_MAX - vaddr) {
-		EXIT("BufferCache: invalid unmap range\n");
-	}
-	std::vector<DownloadCopy>                  copies;
-	std::vector<std::pair<uint64_t, uint64_t>> modified_buffers;
-	std::vector<std::pair<uint64_t, uint64_t>> retired_buffers;
-	for (const auto& [begin, id]: m_buffers) {
-		const auto& buffer = m_slot_buffers[id];
-		if (vaddr < begin + buffer.Size() && begin < vaddr + size) {
-			retired_buffers.emplace_back(begin, buffer.Size());
-		}
-	}
-	for (const auto& [begin, id]: m_buffers) {
-		const auto& buffer = m_slot_buffers[id];
-		if (vaddr >= begin + buffer.Size() || begin >= vaddr + size ||
-		    !m_memory_tracker.IsRegionGpuModified(begin, buffer.Size())) {
-			continue;
-		}
-		if (!m_gpu_modified_ranges.Intersects(begin, buffer.Size())) {
-			EXIT("BufferCache: GPU-modified buffer has no dirty ranges\n");
-		}
-		modified_buffers.emplace_back(begin, buffer.Size());
-	}
-	for (const auto& [begin, bytes]: modified_buffers) {
-		auto owner = m_buffers.find(begin);
-		if (owner == m_buffers.end() || m_slot_buffers[owner->second].Size() != bytes) {
-			EXIT("BufferCache: unmap owner changed during collection\n");
-		}
-		auto& buffer = m_slot_buffers[owner->second];
-		m_memory_tracker.ForEachDownloadRange<false>(
-		    begin, buffer.Size(),
-		    [&](uint64_t address, uint64_t bytes) noexcept {
-			    m_memory_tracker.ValidateGpuDirtyPages(m_gpu_modified_ranges, address, bytes,
-			                                           "unmap");
-		    },
-		    [&](uint64_t address, uint64_t bytes) noexcept {
-			    for (const auto& range: m_gpu_modified_ranges.Intersections(address, bytes)) {
-				    copies.push_back({&buffer, range.address - begin, range.address, range.size});
-			    }
-		    });
-	}
-	if (!copies.empty()) {
-		DownloadBufferMemory(copies);
-	}
-	for (const auto& [begin, bytes]: modified_buffers) {
-		m_gpu_modified_ranges.Subtract(begin, bytes);
-		m_memory_tracker.UnmarkRegionAsGpuModified(begin, bytes);
-	}
-	for (const auto& [begin, bytes]: retired_buffers) {
-		m_memory_tracker.MarkRegionAsCpuModified(begin, bytes);
-	}
-	if (m_gpu_modified_ranges.Intersects(vaddr, size)) {
-		EXIT("BufferCache: unmap retained dirty byte ranges\n");
-	}
-	m_memory_tracker.UntrackMemory(vaddr, size);
-	for (const auto& [begin, bytes]: retired_buffers) {
-		(void)bytes;
-		const auto found = m_buffers.find(begin);
-		if (found != m_buffers.end()) {
-			DeleteBuffer(found->second);
-		}
-	}
-}
-
 BufferId BufferCache::FindBuffer(uint64_t vaddr, uint64_t size) {
 	if (vaddr == 0) {
 		return NULL_BUFFER_ID;

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

@@ -35,7 +35,6 @@ public:
 
 	void                   InvalidateMemory(uint64_t vaddr, uint64_t size);
 	void                   ReadMemory(uint64_t vaddr, uint64_t size, bool is_write = false);
-	void                   UnmapMemory(uint64_t vaddr, uint64_t size);
 	[[nodiscard]] Buffer&  GetBuffer(BufferId id) { return m_slot_buffers[id]; }
 	[[nodiscard]] BufferId FindBuffer(uint64_t vaddr, uint64_t size);
 	[[nodiscard]] std::pair<Buffer*, uint64_t> ObtainBuffer(uint64_t vaddr, uint64_t size,

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

@@ -63,7 +63,7 @@ void GpuResourceManager::UnmapMemory(uint64_t vaddr, uint64_t size) {
 			m_scheduler.FinishCurrent();
 			m_scheduler.WaitPriorityOperations(tick);
 		}
-		m_buffer_cache.UnmapMemory(vaddr, size);
+		m_buffer_cache.InvalidateMemory(vaddr, size);
 		m_texture_cache.UnmapMemory(vaddr, size);
 		m_page_manager.OnGpuUnmap(vaddr, size);
 		std::lock_guard lock(m_mapped_ranges_mutex);

tests/ShaderRecompilerComputeTests.cpp

@@ -2922,6 +2922,7 @@ public:
     constexpr uint32_t second_stale = 0xdeadbeefu;
     constexpr uint32_t clean_value = 0xaabbccddu;
     constexpr uint32_t unmap_value = 0x91a2b3c4u;
+    constexpr uint32_t remap_value = 0xd5e6f708u;
     constexpr uint32_t partial_unmap_value = 0x62738495u;
     constexpr uint32_t partial_unmap_survivor_value = 0xa6b7c8d9u;
     constexpr uint32_t ring_fault_first_value = 0x0a1b2c3du;
@@ -2966,6 +2967,31 @@ public:
         Require(name, "dirty allocation", allocation.first != nullptr,
                 "dirty-GC buffer allocation failed");
       };
+      const auto ReadNativeValue =
+          [&](const Libs::Graphics::Buffer &buffer, uint64_t offset) {
+        auto readback =
+            CreateHostBuffer(name, sizeof(uint32_t),
+                             vk::BufferUsageFlagBits::eTransferDst, {0});
+        const vk::BufferCopy copy{offset, 0, sizeof(uint32_t)};
+        scheduler.Current().Handle().copyBuffer(buffer.Handle(), readback.buffer,
+                                                1, &copy);
+        vk::BufferMemoryBarrier barrier{};
+        barrier.sType = vk::StructureType::eBufferMemoryBarrier;
+        barrier.srcAccessMask = vk::AccessFlagBits::eTransferWrite;
+        barrier.dstAccessMask = vk::AccessFlagBits::eHostRead;
+        barrier.srcQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED;
+        barrier.dstQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED;
+        barrier.buffer = readback.buffer;
+        barrier.size = readback.size;
+        scheduler.Current().Handle().pipelineBarrier(
+            vk::PipelineStageFlagBits::eTransfer,
+            vk::PipelineStageFlagBits::eHost, {}, 0, nullptr, 1, &barrier, 0,
+            nullptr);
+        scheduler.Finish();
+        const auto value = ReadBuffer(name, readback, 1)[0];
+        DestroyBuffer(&readback);
+        return value;
+      };
 
       constexpr uint64_t index_offset = 0x180000;
       constexpr uint64_t index_page = BufferCache::CACHING_PAGE_SIZE;
@@ -3033,18 +3059,26 @@ public:
                   resolved_right == resolved_left &&
                   resolved_left_offset != resolved_right_offset,
               "saved descriptor IDs did not resolve through the merged owner");
-      cache.UnmapMemory(index_begin, index_span);
+      resources.UnmapMemory(index_begin, index_span);
-      cache.UnmapMemory(base + recycled_offset, index_page);
+      resources.UnmapMemory(base + recycled_offset, index_page);
-      Require(name, "registered-range removal",
+      Require(name, "invalidation retains owners",
-              !cache.IsRegionRegistered(index_begin, index_span) &&
+              !resources.IsMapped(index_begin, index_span) &&
-                  !BufferCacheTestAccess::PageOwner(cache, index_begin) &&
+                  !resources.IsMapped(base + recycled_offset, index_page) &&
-                  !BufferCacheTestAccess::PageOwner(
+                  cache.IsRegionRegistered(index_begin, index_span) &&
-                      cache, index_begin + index_page) &&
+                  BufferCacheTestAccess::PageOwner(cache, index_begin) ==
-                  !BufferCacheTestAccess::PageOwner(
+                      index_bridge &&
-                      cache, index_begin + 2 * index_page) &&
+                  BufferCacheTestAccess::PageOwner(
-                  !BufferCacheTestAccess::PageOwner(cache,
+                      cache, index_begin + index_page) == index_bridge &&
-                                                    base + recycled_offset),
+                  BufferCacheTestAccess::PageOwner(
-              "unmapped Buffer owner remained in the registered-range index");
+                      cache, index_begin + 2 * index_page) == index_bridge &&
+                  BufferCacheTestAccess::PageOwner(
+                      cache, base + recycled_offset) == recycled &&
+                  cache.IsRegionCpuModified(index_begin, index_span) &&
+                  cache.IsRegionCpuModified(base + recycled_offset,
+                                            index_page),
+              "CPU invalidation retired a reusable Buffer owner");
+      resources.MapMemory(index_begin, index_span);
+      resources.MapMemory(base + recycled_offset, index_page);
 
       MarkGpuWrite(base + first_offset, sizeof(first_value));
       MarkGpuWrite(base + second_offset, sizeof(second_value));
@@ -3245,23 +3279,36 @@ public:
 
       auto unmap_allocation = cache.ObtainBuffer(
           base + unmap_offset, sizeof(unmap_value), true, false);
-      Require(name, "direct-unmap allocation",
+      Require(name, "direct-invalidation allocation",
               unmap_allocation.first != nullptr,
-              "direct-unmap buffer allocation failed");
+              "direct-invalidation buffer allocation failed");
       cache.FillBuffer(base + unmap_offset, sizeof(unmap_value), unmap_value,
                        false);
-      cache.UnmapMemory(base + 0x4000, 0x4000);
+      resources.UnmapMemory(base + 0x4000, 0x4000);
       uint32_t unmap_backing = 0;
       std::memcpy(&unmap_backing, memory + unmap_offset, sizeof(unmap_backing));
-      Require(name, "direct-unmap contents",
+      Require(name, "direct-invalidation contents",
               unmap_backing == unmap_value &&
-                  !cache.IsRegionRegistered(base + 0x4000, 0x4000),
+                  !resources.IsMapped(base + 0x4000, 0x4000) &&
-              "direct dirty unmap cleared tracking before "
+                  cache.IsRegionRegistered(base + 0x4000, 0x4000) &&
-              "publishing bytes");
+                  cache.IsRegionCpuModified(base + 0x4000, 0x4000) &&
+                  !cache.IsRegionGpuModified(base + 0x4000, 0x4000) &&
+                  !cache.HasGpuDirtyBytes(base + 0x4000, 0x4000),
+              "dirty invalidation did not publish bytes and retain its owner");
+      std::memcpy(memory + unmap_offset, &remap_value, sizeof(remap_value));
+      resources.MapMemory(base + 0x4000, 0x4000);
+      const auto remapped =
+          cache.ObtainBuffer(base + unmap_offset, sizeof(remap_value), true);
+      Require(name, "same-address remap allocation",
+              remapped.first == unmap_allocation.first,
+              "same-address remap did not synchronize the retained Buffer");
+      Require(name, "same-address remap contents",
+              ReadNativeValue(*remapped.first, remapped.second) == remap_value,
+              "same-address remap reused stale native Buffer bytes");
 
       auto partial_unmap_allocation =
           cache.ObtainBuffer(base + 0x8000, 0x8000, true, false);
-      Require(name, "partial-unmap allocation",
+      Require(name, "partial-invalidation allocation",
               partial_unmap_allocation.first != nullptr,
               "cross-page cached buffer allocation failed");
       cache.FillBuffer(base + partial_unmap_offset, sizeof(partial_unmap_value),
@@ -3269,39 +3316,27 @@ public:
       cache.FillBuffer(base + partial_unmap_survivor_offset,
                        sizeof(partial_unmap_survivor_value),
                        partial_unmap_survivor_value, false);
-      cache.UnmapMemory(base + 0x8000, 0x4000);
+      resources.UnmapMemory(base + 0x8000, 0x4000);
+      Require(name, "partial-invalidation ownership",
+              !resources.IsMapped(base + 0x8000, 0x4000) &&
+                  cache.IsRegionRegistered(base + 0x8000, 0x8000) &&
+                  cache.IsRegionCpuModified(base + 0x8000, 0x4000) &&
+                  !cache.HasGpuDirtyBytes(base + partial_unmap_offset,
+                                          sizeof(partial_unmap_value)) &&
+                  cache.HasGpuDirtyBytes(
+                      base + partial_unmap_survivor_offset,
+                      sizeof(partial_unmap_survivor_value)),
+              "partial invalidation retired its owner or cleared disjoint GPU bytes");
+      resources.MapMemory(base + 0x8000, 0x4000);
       auto survivor =
           cache.ObtainBuffer(base + partial_unmap_survivor_offset,
                              sizeof(partial_unmap_survivor_value), false);
-      Require(name, "partial-unmap survivor", survivor.first != nullptr,
+      Require(name, "partial-invalidation survivor", survivor.first != nullptr,
-              "still-mapped cached-buffer remainder could not be recreated");
+              "cached-buffer remainder was not retained");
-      auto partial_unmap_readback =
+      Require(name, "partial-invalidation survivor contents",
-          CreateHostBuffer(name, sizeof(partial_unmap_survivor_value),
+              ReadNativeValue(*survivor.first, survivor.second) ==
-                           vk::BufferUsageFlagBits::eTransferDst, {0});
+                  partial_unmap_survivor_value,
-      const vk::BufferCopy survivor_copy{survivor.second, 0,
+              "partial invalidation lost disjoint native bytes");
-                                         sizeof(partial_unmap_survivor_value)};
-      scheduler.Current().Handle().copyBuffer(survivor.first->Handle(),
-                                              partial_unmap_readback.buffer, 1,
-                                              &survivor_copy);
-      vk::BufferMemoryBarrier survivor_barrier{};
-      survivor_barrier.sType = vk::StructureType::eBufferMemoryBarrier;
-      survivor_barrier.srcAccessMask = vk::AccessFlagBits::eTransferWrite;
-      survivor_barrier.dstAccessMask = vk::AccessFlagBits::eHostRead;
-      survivor_barrier.srcQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED;
-      survivor_barrier.dstQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED;
-      survivor_barrier.buffer = partial_unmap_readback.buffer;
-      survivor_barrier.size = partial_unmap_readback.size;
-      scheduler.Current().Handle().pipelineBarrier(
-          vk::PipelineStageFlagBits::eTransfer,
-          vk::PipelineStageFlagBits::eHost, {}, 0, nullptr, 1,
-          &survivor_barrier, 0, nullptr);
-      scheduler.Finish();
-      Require(name, "partial-unmap survivor contents",
-              ReadBuffer(name, partial_unmap_readback, 1) ==
-                  std::vector<u32>{partial_unmap_survivor_value},
-              "partial unmap recreated its still-mapped remainder from "
-              "uninitialized native bytes");
-      DestroyBuffer(&partial_unmap_readback);
 
       constexpr uint64_t large_offset = 0x10000;
       constexpr uint64_t large_size = 33ull * 1024 * 1024;
@@ -3431,16 +3466,17 @@ public:
           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);
+      resources.UnmapMemory(base + disjoint_owner_offset + 0x4000, 0x4000);
       uint32_t disjoint_after_unmap = 0;
       Libs::LibKernel::Memory::TryReadBacking(base + disjoint_dirty_offset,
                                               &disjoint_after_unmap,
                                               sizeof(disjoint_after_unmap));
-      Require(name, "disjoint synchronized unmap",
+      Require(name, "disjoint synchronized invalidation",
               disjoint_after_unmap == disjoint_value &&
                   !cache.HasGpuDirtyBytes(base + disjoint_dirty_offset,
                                           sizeof(disjoint_value)),
-              "disjoint unmap lost the completed whole-owner publication");
+              "disjoint invalidation lost the completed whole-owner publication");
+      resources.MapMemory(base + disjoint_owner_offset + 0x4000, 0x4000);
       auto disjoint_new_owner =
           cache.ObtainBuffer(base + disjoint_new_owner_offset,
                              sizeof(disjoint_value), true, false);