From 5de1309fd0f8fbbc562d7205b91fa2722d806b6d Mon Sep 17 00:00:00 2001 From: Nuno Silva <22588915+NFSS10@users.noreply.github.com> Date: Fri, 6 Nov 2026 11:46:28 +0000 Subject: [PATCH] fix(agc): keep guest bytes when a stale depth plane is flushed A depth surface keeps mirroring guest memory after the guest has released and reused it, so a flush triggered by later GPU work over that range stores its pending results into live guest data. A pending fast clear made it worse: the storing synchronisation returned before the guest-write check, so the clear value was written over whatever the guest now keeps there. Record, per plane unit, the backing serial of the allocation registered over it at every synchronisation, and drop a unit whose allocation changed before storing. A storing synchronisation no longer skips the guest-write check when a clear is pending: the units the guest wrote are dropped and the clear is forgotten for the surface. The sampling path that retires a stale plane applies the same unit rule. --- .../Graphics/src/DepthSurface.cpp | 41 ++++++++++++++++--- docs/dev/debt/libSceAgcDriver.md | 2 + 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp b/core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp index a0c0d60ee37f..5399425ecaff 100644 --- a/core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp +++ b/core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp @@ -46,6 +46,11 @@ std::uint64_t depthWrites = 0; std::uint64_t mirrorEpoch = 0; std::atomic depthPublicationHook{nullptr}; +std::uint64_t RegisteredBacking(std::uint64_t address) { + const auto range = GuestAllocations::GuestAllocationsLookup_nid_postfix(static_cast(address), false); + return range != nullptr ? range->backingSerial : 0; +} + std::pair DepthBlockExtent(std::uint32_t bytesPerTexel) { return {bytesPerTexel == 4 ? 128u : 256u, bytesPerTexel == 1 ? 256u : 128u}; } @@ -1295,6 +1300,7 @@ class DepthSurface { staleUnits.assign(count, false); staleCount = 0; unitGeneration.assign(count, 0); + unitBacking.assign(count, 0); } bool AnyStale() const { @@ -1390,14 +1396,29 @@ class DepthSurface { return changed; } - void SyncFromMemory(const GuestTextureResource& resource) { + std::vector ChangedBackingUnits() const { + std::vector changed(unitBacking.size(), false); + for (std::size_t unit = 0; unit < unitBacking.size(); ++unit) { + changed[unit] = unitBacking[unit] != 0 && RegisteredBacking(target.address + unit * unitBytes) != unitBacking[unit]; + } + return changed; + } + + void SyncFromMemory(const GuestTextureResource& resource, bool storing = false) { std::uint64_t generation = 0; const auto changed = WrittenUnits(resource, generation); - if ((pendingClear & VK_IMAGE_ASPECT_DEPTH_BIT) != 0 || !anyUnit(changed)) { + for (std::size_t unit = 0; unit < unitBacking.size(); ++unit) unitBacking[unit] = RegisteredBacking(target.address + unit * unitBytes); + const bool clearing = (pendingClear & VK_IMAGE_ASPECT_DEPTH_BIT) != 0; + if ((clearing && !storing) || !anyUnit(changed)) { unitGeneration.assign(unitGeneration.size(), generation); return; } - ClearStale(changed); + if (clearing) { + ClearStale(); + ForgetClear(VK_IMAGE_ASPECT_DEPTH_BIT); + } else { + ClearStale(changed); + } { struct Syncing { bool& flag; @@ -1472,6 +1493,7 @@ class DepthSurface { std::size_t staleCount = 0; std::uint64_t depthMark = 0; std::uint64_t stencilMark = 0; + std::vector unitBacking; std::vector unitGeneration; std::uint64_t unitBytes = 0; bool syncing = false; @@ -2908,10 +2930,12 @@ void retireStoringStale(DepthSurface& surface) { std::uint64_t generation = 0; const auto written = surface.WrittenUnits(resource, generation); const auto removed = unmappedUnits(surface.target.address, planeBytes, surface.unitBytes, surface.staleUnits.size(), surface.depthMark); + const auto reused = surface.ChangedBackingUnits(); auto units = surface.staleUnits; for (std::size_t unit = 0; unit < units.size(); ++unit) { - if ((unit < written.size() && written[unit]) || removed[unit]) units[unit] = false; + if ((unit < written.size() && written[unit]) || removed[unit] || (unit < reused.size() && reused[unit])) units[unit] = false; } + if ((surface.pendingClear & VK_IMAGE_ASPECT_DEPTH_BIT) != 0 && anyUnit(written)) units.assign(units.size(), false); std::shared_ptr mirror; if (anyUnit(units) && (!anyUnit(written) || surface.StoresUnits(*surface.Mirror(resource)))) mirror = surface.StoreDepthPlane(resource, units); retire(surface); @@ -3293,8 +3317,15 @@ std::vector> FlushDepthPlanes(std::uint6 else surface->ClearStale(removed); if (!surface->AnyStale()) continue; } + const auto reused = surface->ChangedBackingUnits(); + if (anyUnit(reused)) { + static std::atomic reported{false}; + if (!reported.exchange(true)) std::fprintf(stderr, "[gpu] depth plane 0x%llx+0x%llx was re-registered with GPU results pending; they are dropped\n", static_cast(plane), static_cast(planeBytes)); + surface->ClearStale(reused); + if (!surface->AnyStale()) continue; + } GuestMemory::CollectWritesUncached(plane, static_cast(planeBytes)); - surface->SyncFromMemory(resource); + surface->SyncFromMemory(resource, true); auto units = scope == DepthPlaneScope::Whole ? surface->staleUnits : surface->StaleUnitsIn(planeBytes, address, end); if (anyUnit(units)) { auto mirror = surface->StoreDepthPlane(resource, units); diff --git a/docs/dev/debt/libSceAgcDriver.md b/docs/dev/debt/libSceAgcDriver.md index 9dc6d0b7344c..68a8199710a2 100644 --- a/docs/dev/debt/libSceAgcDriver.md +++ b/docs/dev/debt/libSceAgcDriver.md @@ -401,6 +401,8 @@ - [Zero color export formats](../../../core/libs/prx/libSceAgcDriver/Graphics/src/State.cpp) (libSceAgcDriver) - a target that `CB_TARGET_MASK` and `CB_SHADER_MASK` enable but whose `SPI_SHADER_COL_FORMAT` entry is `SPI_SHADER_ZERO` is left unwritten. Mesa derives `CB_SHADER_MASK` from the formats (`ac_get_cb_shader_mask`), so the console's behaviour for the combination is unmeasured here; the driver keeps later export indices and blend slots where the shader mask puts them, as it does for a `COLOR_INVALID` target. - [Device-lost fault attribution](../../../core/libs/prx/libSceAgcDriver/Execution/include/DeviceFault.hpp) (libSceAgcDriver): a device loss prints the faulting VA with its guest snapshot (owner or neighbours), a bracket window (last completed, in flight, first not started, last 8 tokens) and draw/dispatch tokens (serial, program, variant, desc). Find returns the entry by value (acquire-load, copy, acquire-reload); Claim clears the slot first so a reused token misses instead of aliasing (reuse test). Draws emit their token through the NV checkpoint backend (emission test); serial falls back to the checkpoint marker. Queues report separately with role and queue id; live resources copy under a mutex. Needs APS5_GPU_CHECKPOINTS; the marker buffer stays AMD-only, NVIDIA is draw-only. Cannot name T# or pending write-back. Vendor dump via APS5_DUMP_DEVICE_FAULT where supported. Opened for PPSA01342 (#10634): second-world load faults a write with no checkpoint reached. +- [Stale depth planes over reused guest memory](../../../core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp) (libSceAgcDriver): a depth surface outlives the guest memory it mirrors, so a flush over a later allocation stores pending results into it; a pending fast clear used to skip the guest-write check and write zero there. A flush now drops a unit whose recorded allocation (`backingSerial`) changed, and a storing sync drops the units the guest wrote and forgets the clear. Not covered: reuse inside one unchanged registration with no write since the last sync, and the stencil plane, which compares bytes with its last mark. Closes when: surfaces are retired on allocation changes (an unmap hook that keeps the `surfacesMutex`/`GuestAllocations` lock order) instead of at flush time. PPSA27616 reached this through a cubemap of 128x128 depth faces whose memory its heap had taken back. + ### Opt-in draw paths - [Draw finisher](../../../core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/DrawPipeline.cpp) (`APS5_DRAW_PIPELINE`) - opt-in. The Linux main-image page table it adds matches what Windows already does by default.