Merge pull request #6465 from milescoviello/fix/free-retired-depth-surfaces

fix(agc): free retired depth surfaces nobody samples

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

core/libs/prx/libSceAgcDriver/Graphics/include/Pipeline.hpp

@@ -69,6 +69,7 @@ private:
         std::vector<bool> owned;
         VkExtent2D extent;
         std::uint32_t layers = 1;
+        std::uint64_t depthReleases = 0;
         std::shared_ptr<Framebuffer> framebuffer;
     };
     void release() noexcept;

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<std::uint64_t> surfacesSerial{0};
+std::atomic<std::uint64_t> 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<DepthSurface> 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<DepthSurface>(context, target, std::move(sampling)));
         auto& fresh = *surfaces().back();

core/libs/prx/libSceAgcDriver/Graphics/src/Pipeline.cpp

@@ -634,18 +634,20 @@ std::shared_ptr<Framebuffer> Pipeline::AcquireFramebuffer(std::span<const VkImag
     Require(targets.size() == attachments && owners.size() == colorAttachments, "render targets do not match the pipeline's attachments");
     bool resident = true;
     for (const auto& owner : owners) resident = resident && owner != nullptr;
+    const auto depthReleases = attachments > 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<Framebuffer> Pipeline::AcquireFramebuffer(std::span<const VkImag
     for (const auto& owner : owners) entry.owned.push_back(owner != nullptr);
     entry.extent = extent;
     entry.layers = layers;
+    entry.depthReleases = depthReleases;
     entry.framebuffer = framebuffer;
     framebuffers.push_back(std::move(entry));
     return framebuffer;

core/libs/prx/libSceAgcDriver/tests/DepthSurfaceReuse.cpp

@@ -2,6 +2,7 @@
 #include "prx/libSceAgcDriver/Graphics/include/DepthSurface.hpp"
 #include "prx/libSceAgcDriver/Graphics/include/GuestTextureResource.hpp"
 #include "prx/libSceAgcDriver/Graphics/include/Texture.hpp"
+#include <algorithm>
 #include <array>
 #include <cstdint>
 #include <map>
@@ -47,6 +48,7 @@ VkImageUsageFlags lastImageUsage = 0;
 std::uint32_t imagesDestroyed = 0;
 std::vector<std::pair<VkImage, VkImageAspectFlags>> imageReads;
 std::vector<std::pair<VkImage, VkImageAspectFlags>> imageWrites;
+std::vector<VkImage> 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<VkImage> 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<VkImage>{firstImage}, "an HTILE clear of the first slice must clear that slice alone");
         Require(clearsAfter(2u * sliceHtile) == std::vector<VkImage>{firstImage, secondImage}, "an HTILE clear over both slices must clear both");
     }
+    TestRetiredSurfaceRelease(context);
 
     ClearDepthSurfaces(context.device);
 }