From 98600b489fd3b6f97ac35f53ab74887c7cc502f7 Mon Sep 17 00:00:00 2001 From: milescoviello Date: Thu, 15 Oct 2026 15:47:43 +0000 Subject: [PATCH] fix(agc): free retired depth surfaces nobody samples A depth surface stayed in the list until its device went, even after another writer retired it. A title whose depth targets move or change extent (dynamic resolution, a transient heap) adds one surface per new target and never frees the old ones. Creating a surface now frees the retired single-sample ones whose sampled views no one else holds, through the recorder so recorded work that still uses them completes first. Framebuffers cached with a depth attachment are dropped after a release, since a freed view's handle can be reused. Co-Authored-By: Claude Opus 5.5 --- .../Graphics/include/DepthSurface.hpp | 1 + .../Graphics/include/Pipeline.hpp | 1 + .../Graphics/src/DepthSurface.cpp | 28 +++++++++++ .../libSceAgcDriver/Graphics/src/Pipeline.cpp | 7 ++- .../tests/DepthSurfaceReuse.cpp | 46 ++++++++++++++++++- 5 files changed, 80 insertions(+), 3 deletions(-) diff --git a/core/libs/prx/libSceAgcDriver/Graphics/include/DepthSurface.hpp b/core/libs/prx/libSceAgcDriver/Graphics/include/DepthSurface.hpp index 2fb42ab7fb..df4e513ba8 100644 --- a/core/libs/prx/libSceAgcDriver/Graphics/include/DepthSurface.hpp +++ b/core/libs/prx/libSceAgcDriver/Graphics/include/DepthSurface.hpp @@ -17,6 +17,7 @@ class StorageTexture; VkImageView DepthSurfaceView(const Context& context, const DepthTarget& target); bool SampleLocationsCompatibleDepth(const Context& context, VkFormat format, std::uint32_t samples); std::uint64_t DepthSurfacesSerial(); +std::uint64_t DepthSurfacesReleased(); std::uint64_t DepthSliceBytes(VkExtent2D extent, std::uint32_t bytesPerTexel); std::uint64_t MultisampledDepthBytes_nid_no_patch(VkExtent2D extent, std::uint32_t bytesPerTexel, std::uint32_t samples); void RequireNoResidentDepthRead_nid_no_patch(std::uint64_t address, std::size_t bytes); diff --git a/core/libs/prx/libSceAgcDriver/Graphics/include/Pipeline.hpp b/core/libs/prx/libSceAgcDriver/Graphics/include/Pipeline.hpp index a88487a44a..01ed3aba8a 100644 --- a/core/libs/prx/libSceAgcDriver/Graphics/include/Pipeline.hpp +++ b/core/libs/prx/libSceAgcDriver/Graphics/include/Pipeline.hpp @@ -69,6 +69,7 @@ private: std::vector owned; VkExtent2D extent; std::uint32_t layers = 1; + std::uint64_t depthReleases = 0; std::shared_ptr framebuffer; }; void release() noexcept; diff --git a/core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp b/core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp index 170526d8ff..eda844d298 100644 --- a/core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp +++ b/core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp @@ -475,6 +475,10 @@ public: return std::any_of(textures.begin(), textures.end(), held) || std::any_of(stencilCopy.textures.begin(), stencilCopy.textures.end(), held); } + bool SampledElsewhere() const { + return std::any_of(textures.begin(), textures.end(), [](const auto& entry) { return entry.second.use_count() > 1; }); + } + std::uint64_t written = ++depthWrites; bool retired = false; @@ -1200,6 +1204,25 @@ bool takeClearedHtile(const DepthTarget& target) { const DepthSurface* drawnDepth = nullptr; std::atomic surfacesSerial{0}; +std::atomic surfacesReleased{0}; + +void releaseRetired(const Context& context) { + auto* recorder = Recorder::Active(); + if (recorder != nullptr && recorder->Device() != context.device) return; + auto& list = surfaces(); + for (auto it = list.begin(); it != list.end();) { + const auto& surface = **it; + if (!surface.retired || surface.context.device != context.device || surface.target.samples != 1u || surface.SampledElsewhere()) { + ++it; + continue; + } + if (drawnDepth == &surface) drawnDepth = nullptr; + std::shared_ptr released = std::move(*it); + it = list.erase(it); + surfacesReleased.fetch_add(1, std::memory_order_release); + if (recorder != nullptr) recorder->Keep(std::move(released)); + } +} std::uint32_t expandedHtileWord(std::uint32_t word, bool stencilInHtile, VkImageAspectFlags cleared) { if (!stencilInHtile) return (cleared & VK_IMAGE_ASPECT_DEPTH_BIT) != 0 ? 0xfffc000fu : word; @@ -1263,6 +1286,10 @@ std::uint64_t DepthSurfacesSerial() { return surfacesSerial.load(std::memory_order_acquire); } +std::uint64_t DepthSurfacesReleased() { + return surfacesReleased.load(std::memory_order_acquire); +} + std::uint64_t DepthSliceBytes(VkExtent2D extent, std::uint32_t bytesPerTexel) { const std::uint32_t blockWidth = bytesPerTexel == 4 ? 128u : 256u; const std::uint32_t blockHeight = bytesPerTexel == 1 ? 256u : 128u; @@ -1390,6 +1417,7 @@ VkImageView DepthSurfaceView(const Context& context, const DepthTarget& target) auto sampling = target.samples == 1u ? DepthSamplingState{} : prepareDepthSampling(context, target); creationLease = target.samples == 1u || !sampling.refusal.empty() ? GuestAllocations::Lease{} : GuestAllocations::GuestAllocationsAcquireAll_nid_postfix(); lock.lock(); + releaseRetired(context); surfacesSerial.fetch_add(1, std::memory_order_release); surfaces().push_back(std::make_unique(context, target, std::move(sampling))); auto& fresh = *surfaces().back(); diff --git a/core/libs/prx/libSceAgcDriver/Graphics/src/Pipeline.cpp b/core/libs/prx/libSceAgcDriver/Graphics/src/Pipeline.cpp index de9d1904d6..5bfd9b37bf 100644 --- a/core/libs/prx/libSceAgcDriver/Graphics/src/Pipeline.cpp +++ b/core/libs/prx/libSceAgcDriver/Graphics/src/Pipeline.cpp @@ -634,18 +634,20 @@ std::shared_ptr Pipeline::AcquireFramebuffer(std::span colorAttachments ? DepthSurfacesReleased() : 0u; if (resident) { // Entries whose views are gone can never match again and go as soon as no recorded draw holds // them (Kept keeps its framebuffer until the batch completes). - std::erase_if(framebuffers, [](const CachedFramebuffer& entry) { + std::erase_if(framebuffers, [&](const CachedFramebuffer& entry) { if (entry.framebuffer.use_count() != 1) return false; + if (entry.depthReleases != depthReleases) return true; for (std::size_t i = 0; i < entry.owners.size(); ++i) { if (entry.owned[i] && entry.owners[i].expired()) return true; } return false; }); for (auto it = framebuffers.begin(); it != framebuffers.end(); ++it) { - if (it->extent.width != extent.width || it->extent.height != extent.height || it->layers != layers || !std::equal(it->views.begin(), it->views.end(), targets.begin(), targets.end())) continue; + if (it->extent.width != extent.width || it->extent.height != extent.height || it->layers != layers || it->depthReleases != depthReleases || !std::equal(it->views.begin(), it->views.end(), targets.begin(), targets.end())) continue; // View handles are recycled once a StorageTexture is destroyed, so the owners must be the // very objects the views were made for. bool same = true; @@ -670,6 +672,7 @@ std::shared_ptr Pipeline::AcquireFramebuffer(std::span #include #include #include @@ -47,6 +48,7 @@ VkImageUsageFlags lastImageUsage = 0; std::uint32_t imagesDestroyed = 0; std::vector> imageReads; std::vector> imageWrites; +std::vector destroyedImages; VKAPI_ATTR VkResult VKAPI_CALL createImage(VkDevice, const VkImageCreateInfo* info, const VkAllocationCallbacks*, VkImage* image) { lastImageFormat = info->format; @@ -108,7 +110,10 @@ VKAPI_ATTR VkResult VKAPI_CALL getFenceStatus(VkDevice, VkFence) { VKAPI_ATTR void VKAPI_CALL freeCommandBuffers(VkDevice, VkCommandPool, std::uint32_t, const VkCommandBuffer*) {} VKAPI_ATTR void VKAPI_CALL destroyFence(VkDevice, VkFence, const VkAllocationCallbacks*) {} VKAPI_ATTR void VKAPI_CALL destroyImageView(VkDevice, VkImageView, const VkAllocationCallbacks*) {} -VKAPI_ATTR void VKAPI_CALL destroyImage(VkDevice, VkImage, const VkAllocationCallbacks*) { ++imagesDestroyed; } +VKAPI_ATTR void VKAPI_CALL destroyImage(VkDevice, VkImage image, const VkAllocationCallbacks*) { + ++imagesDestroyed; + destroyedImages.push_back(image); +} VKAPI_ATTR void VKAPI_CALL freeMemory(VkDevice, VkDeviceMemory, const VkAllocationCallbacks*) {} VKAPI_ATTR void VKAPI_CALL cmdPipelineBarrier(VkCommandBuffer, VkPipelineStageFlags, VkPipelineStageFlags, VkDependencyFlags, std::uint32_t, const VkMemoryBarrier*, std::uint32_t, const VkBufferMemoryBarrier*, std::uint32_t, const VkImageMemoryBarrier*) {} VKAPI_ATTR void VKAPI_CALL cmdClearDepthStencilImage(VkCommandBuffer, VkImage image, VkImageLayout, const VkClearDepthStencilValue*, std::uint32_t, const VkImageSubresourceRange*) { @@ -374,6 +379,44 @@ void TestMultisampleStencilAliases(Context context) { } +void TestRetiredSurfaceRelease(const Context& context) { + ClearDepthSurfaces(context.device); + constexpr VkExtent2D extent{256, 256}; + const auto bytes = DepthSliceBytes(extent, 4u); + const auto destroyed = [](VkImage image) { return std::find(destroyedImages.begin(), destroyedImages.end(), image) != destroyedImages.end(); }; + constexpr std::uint64_t moving = 0x90000000; + std::vector images; + for (std::uint64_t frame = 0; frame < 4; ++frame) { + DepthSurfaceView(context, {moving + frame * 0x100000u, 0, extent, VK_FORMAT_D32_SFLOAT, 1.0f, 0}); + images.push_back(lastImage); + RetireDepthSurfaces(context.device, moving + frame * 0x100000u, bytes); + } + const auto released = DepthSurfacesReleased(); + DepthSurfaceView(context, {moving + 4u * 0x100000u, 0, extent, VK_FORMAT_D32_SFLOAT, 1.0f, 0}); + Require(std::all_of(images.begin(), images.end(), destroyed), "a retired depth surface must free its image once a new surface is created"); + Require(!destroyed(lastImage), "the new depth surface must keep its image"); + Require(DepthSurfacesReleased() > released, "freeing retired depth surfaces must be counted for the framebuffer cache"); + + constexpr std::uint64_t sampled = 0x98000000; + DepthSurfaceView(context, {sampled, 0, extent, VK_FORMAT_D32_SFLOAT, 1.0f, 0}); + const auto sampledImage = lastImage; + auto view = lookup(context, {sampled, FormatR32Float, 256, 256}); + Require(view != nullptr, "an R32F view of a D32 surface's own extent must sample its depth plane"); + RetireDepthSurfaces(context.device, sampled, bytes); + DepthSurfaceView(context, {sampled + 0x100000u, 0, extent, VK_FORMAT_D32_SFLOAT, 1.0f, 0}); + Require(!destroyed(sampledImage), "a retired depth surface must keep its image while its sampled view is held"); + view.reset(); + DepthSurfaceView(context, {sampled + 0x200000u, 0, extent, VK_FORMAT_D32_SFLOAT, 1.0f, 0}); + Require(destroyed(sampledImage), "a retired depth surface must free its image once its sampled view is released"); + + constexpr std::uint64_t kept = 0x9c000000; + DepthSurfaceView(context, {kept, 0, extent, VK_FORMAT_D32_SFLOAT, 1.0f, 0}); + const auto keptImage = lastImage; + DepthSurfaceView(context, {kept + 0x100000u, 0, extent, VK_FORMAT_D32_SFLOAT, 1.0f, 0}); + Require(!destroyed(keptImage), "a depth surface that was not retired must keep its image"); + ClearDepthSurfaces(context.device); +} + void RunDepthSurfaceReuseTests() { static int deviceTag = 0; Context context{}; @@ -480,6 +523,7 @@ void RunDepthSurfaceReuseTests() { Require(clearsAfter(sliceHtile) == std::vector{firstImage}, "an HTILE clear of the first slice must clear that slice alone"); Require(clearsAfter(2u * sliceHtile) == std::vector{firstImage, secondImage}, "an HTILE clear over both slices must clear both"); } + TestRetiredSurfaceRelease(context); ClearDepthSurfaces(context.device); }