Repository navigation
Conversation
|
I traced the documented unstored-depth limitation through the new buffer-write path. The current regression calls the buffer reader before writing 0.75, so that first access has already run |
|
Thanks for this. Review at 064a08c: changes requested before it can go further. Confirmed from the code:
Plausible:
Performance:
Asks:
Ordering note: this merges cleanly with #7131, #7116 and #7085, but #7131 handles part of the same case (a fill writing depth memory), so the order between them will need settling. |
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
064a08c to
53b20c4
Compare
|
Thanks @MarcZX100 and @oneandonlydean. Pushed 53b20c4 (amended), PR text updated. Write after the depth draw, before the first store (MarcZX100, ask 2): every bind of a mirrored surface now collects the plane's writes and takes the tracker generation, created surfaces included, so there is always a baseline. HTILE clear vs reload (ask 1): a pending depth fast clear now skips the reload, and the reload no longer clears Dispatch scan (ask 3): only Tests (ask 5), in
The first check is red on 064a08c. The HTILE one is red when the reload drops the pending clear, as 064a08c did. All 646 Performance (ask 4): you're right that the flag stayed set. A depth target drawn every frame and never read through a buffer stays stale until a flush over its range, so one global flag is set almost all the time. I replaced it with the list of stale plane ranges, kept under its own small lock, plus an atomic hull. #7131: with both PRs, a recognized whole-plane fill becomes a pending clear with the fill value, so the reload here is skipped and the fill wins exactly. Any other write to a D32 Z_X plane reloads. That holds in either merge order. The second one needs a small rebase in |
|
Thanks for 53b20c4. Asks 1 (HTILE clear wins over the reload), 2 (WrittenSince before the store), 3 (GuestBuffers only) and 5 (tests) are closed in the code. The new checks fail with the amend reverted, so they test the fix. Still owed: the before/after numbers for ask 4 on a heavy title (frame time and Heads-up on order: #7131 is planned to merge first. After it, this PR will conflict on one line in |
53b20c4 to
f5f7fcb
Compare
|
Pushed f5f7fcb, with a correction to my previous reply. Correction. Restricting the dispatch scan to Frame times (ask 4). Arch Linux (kernel 7.2.9-zen), i9-14900KF, RTX 4070 Ti SUPER, NVIDIA 615.78.08. Local integration branch (main 864729d + open PRs #6412 #6182 #6771 #6772 #6280 + local patches), with this PR's earlier version 63f1e9078 (A) or f5f7fcb (B). Uncharted: Legacy of Thieves Collection, warm caches. Two 10-minute runs per side into a depth-heavy scene (
Perf (DWARF stacks, three busiest driver threads, heavy part): What keeps the stale state from staying set: a plane leaves the list when it is stored or reloaded, and a flush only touches planes in its range. That is why B stores about 9 times fewer planes than A, whose global flag made every flush store every stale plane. A depth target drawn every frame is still stale between its draw and the first read over its memory; a read that overlaps it stores it, and outside an active recorder that store submits and waits. No new depth errors, throws, crashes or device losses. All 15 menu frame dumps with B show the cliff and the whole cage. AI-assisted: yes |
f5f7fcb to
b596505
Compare
|
Rebase handoff — rebased onto current Verified on Linux (CachyOS, kernel 7.2.9, GCC 16.2.1, Ninja, Release, To pick it up: Head is |
b596505 to
19fc797
Compare
|
Thanks @blawesom. I rebased it myself onto AI-assisted: yes |
|
Thanks for the rebase and the f5f7fcb fixes; the flushes before each write-stamp shortcut look right. One gap, which was already in 53b20c4: Could you:
With that, dropping the flush in the storage-image loops is fully justified. On performance, our main-vs-PR gate runs will cover the comparison against plain |
|
@oneandonlydean thanks, pushed 154c405 with all three:
Negative controls on this test: without (2), with or without (1), the new step fails ( I also tried a step for the late store itself (depth draw, plain color overwrite, buffer read). It passed even without both changes. In the test, the color write-back is a CPU write the tracker sees, so the flush's write-generation check reloads the plane instead of storing it. So (1) only matters for a write-back the tracker does not see, and the test cannot reach that case. I left that step out because it passes either way. All 701 AI-assisted: yes |
19fc797 to
154c405
Compare
|
Thanks. On 154c405 the functional results are clean, but the gate measured a consistent cost against main on the same base: the level-phase median frame time is 7-9% higher (reproduced in 2 of 2 runs, both above all 3 controls), and menu FPS is 2-4% lower. Before merging we'd like to cut that cost first. From reading the head, it seems to come from three places:
Suggestions (your call on the details):
A per-frame count of Separately, could you name the case this PR fixes, as a test or a short scenario without a game title? For example: what reads the depth memory as a buffer or texture, what it sees on |
github-actions
Bot
added
the
unblocks title
154c405 to
9626d8e
Compare
|
@oneandonlydean thanks for the gate numbers. Pushed 9626d8e (amended). What changed. The A plane is now stored only when a reader's range overlaps it, and the store clears Numbers. Arch Linux, RTX 4070 Ti SUPER, NVIDIA 615.78.08, 2560x1440. PPSA05684 on my integration build (the title does not run on plain
"Mirroring off" is 9626d8e with This title runs at 2-5 fps here, so per-draw costs hide in its frame time. Your gate title runs at 20-27 fps in 4K, where a plane is about 33 MB and a store costs a larger share of a frame. I could not reproduce the 7-9% here and cannot run that title, so a re-gate would tell whether the store fix closes it. If a cost remains, the likely source is the per-bind The case it fixes, without a title. A draw writes depth into a single-sample D32 plane with the 64 KiB Z swizzle. A compute dispatch then reads the plane through a buffer V# and copies it as raw dwords into another buffer, which is later sampled as depth. On All 701 AI-assisted: yes |
|
Pushed c8a8ff1 on top: a fix for a bug in this PR that a title hits when it changes its output resolution. Case. The title switches its render mode at runtime (2560x1440 → 1920x1080 → 3840x2160 → back). Each mode binds new D32 Z_X depth targets over memory where the previous mode's planes were: the 3840x2160 plane covers both the 2560x1440 and the 1920x1080 planes, from another base. After the first switch, buffer reads of the new planes kept seeing old depth, and that stayed true after switching back to the first mode. Cause. Each surface stores through a storage image of its own plane ( Counted per 10 s on the main plane (a store lands in guest memory, and the next bind sees it as written and syncs):
Fix. A bind of a mirrorable target drops the images, and the pending store, of older surfaces whose planes overlap its plane over another range (another extent or base). A same-range surface (another stencil or HTILE address over the same plane) keeps its image. The scan runs only when a surface or a plane image was created since the surface's last scan, so steady frames do not scan. The debt entry describes the rule. Tested. All 701 No regression test yet. I tried three shapes on the harness, and all pass without the fix:
The title also has several surfaces per plane (one per stencil address), HTILE clears on every frame, and the stream-copy kernel going through the driver's packet path. I am working on a test that reproduces it and will add it here. AI-assisted: yes |
…their memory A depth surface lives in its Vulkan image, so a buffer read of its depth plane saw guest memory the surface never wrote, and a buffer write into the plane never reached the surface: binding the target again kept the old depth. - A single-sample 32-bit depth surface with the 64 KiB Z swizzle (DB_Z_INFO SW_MODE 24) keeps a storage image in that swizzle over its plane. A bind that writes or fast clears depth marks the plane stale; FlushDepthPlanes copies the depth into the storage image and stores it into guest memory. It runs from StorageTexture::FlushPending, when a buffer upload finishes (over its regions and address space), and before a dispatch for each GuestBuffers V# it binds (a cached dispatch reads them without an upload). A reused build flushes its in-place buffer regions, and an image lookup flushes the range it reads, as do the write-stamp proofs that let a cached image or a reused build skip that lookup: a stale plane's memory looks unchanged to them. - Stale planes are kept as ranges under their own lock with an atomic hull; a flush that overlaps none returns without the GPU and surface locks. - Every bind of such a surface, and every flush of its stale plane, collects the plane's writes. Written by someone else since the surface's previous bind, store or reload, the plane is reloaded from memory instead of stored, so a guest write after a depth draw is not overwritten by that draw's late store. A pending HTILE fast clear wins over the reload. - A color target flushes the stale planes under it before it retires their surfaces, so a target that loads or blends reads the stored depth. A retired surface's plane is no longer stale: a later flush never stores its old depth over what the new owner of the memory wrote. - The depth surfaces lock is recursive: a store reached from a flush under DepthSurfaceTexture re-enters it. - Draws pass whether they write depth, so read-only binds do not mark the plane stale. PPSA05684 copies its main depth plane into two ping-pong planes with a buffer compute, draws the cage's depth into them and fogs its menu from the result; without this the copy carried nothing and the fog covered the cliff and the cage. agc_driver_depth_plane_buffer_memory draws depth 0.5 into such a plane, fetches vertices from it (the plane must hold 0.5), writes 0.75 into the memory and draws at 0.5 under LESS (must pass), then checks the depth written after the reload reaches memory again. On a new plane it writes memory after a depth draw and before any store: a buffer read must keep the guest bytes and the next bind must reload them; a guest write before a depth draw must then lose to that draw. A linear texture at an offset into a plane must read what memory holds after a flush, also when it is viewed through its cached storage image after another depth draw. On a plane with HTILE, an HTILE fast clear after a guest write must win. A color target that blends ZERO/ONE over a plane just drawn at 0.5 must write back the 0.5 it loaded. AI-assisted: yes
…verlaps Each depth surface stores its plane through a storage image of its own, kept for the surface's life. A title that switches output resolution binds new depth targets over memory where the previous targets' planes were, and the old images stayed live there. Storage images over the same memory lend each other pending results, so a later store of the new plane did not reach guest memory and buffer reads kept seeing the old depth. A bind now drops the images and the pending store of older surfaces whose planes overlap its plane over another range. A same-range surface (another stencil or HTILE address) keeps its image. The scan runs only when a surface or plane image was created since the surface's last scan. AI-assisted: yes
c8a8ff1 to
132e69d
Compare
github-actions
Bot
added
needs rebase
# Conflicts: # core/libs/prx/libSceAgcDriver/Graphics/include/DepthSurface.hpp # core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
This branch has not been deployed
4 participants
What
On
maina depth surface lives only in its Vulkan image. Two buffer accesses to its depth plane go wrong:Uncharted: Legacy of Thieves Collection (PPSA05684, #4001) hits both in its main menu. A buffer compute (61440x1x1,
buffer(ro)->buffer(rw), 0xf00000 bytes) copies the main depth plane as raw memory into two ping-pong planes. The title then draws the cage's depth-only meshes into those planes, downsamples them to linear depth and fogs the scene from the result. Onmainthe copy carries nothing (89 % of the half-res depth reads 1.0), so the fog covers the cliff and the whole skeleton cage.Changes
A single-sample
D32_SFLOAT/D32_SFLOAT_S8_UINTsurface with the 64 KiB Z swizzle (DB_Z_INFO.SW_MODE24,SW_64KB_Z_X) keeps a storage image in that swizzle over its depth plane (mirrorPlane, guest format 32_FLOAT).DepthSurfaceViewnow takesdepthWritten, and both bind sites inDraw.cpppassstate.depthWrite. A bind that writes depth, or applies a recognized HTILE fill clear, marks the plane stale.New function
FlushDepthPlanes(address, bytes). It copies each stale plane that overlaps the range into its storage image, then letsStorageTexture::FlushPendingstore the image into guest memory. It is called from:StorageTexture::FlushPendingGuestBufferMemory::UploadFinish, over the regions the build binds through its descriptors (not over the whole heaps an address-based shader maps, as for pending storage images)VulkanDevice::dispatch, for each V# of itsGuestBuffersbindings. This is needed because a cached dispatch reads its buffers without an upload.fastTexture,fastRevalidate,fastRevalidateEach): a stale plane's memory has not been written yet, so it looks unchanged to themStale planes are kept as a list of ranges under their own lock, with an atomic hull. A call whose range overlaps no stale plane returns without taking
GpuMutexor the surfaces lock.Every bind of such a surface, and every flush of its stale plane, collects the plane's writes (
GuestMemory::CollectWrites). If someone else wrote the plane since the surface's previous bind, store or reload (WrittenSinceagainst the generation taken then), the surface reloads its depth from the storage image instead of storing. So a guest write made after a depth draw is not overwritten by that draw's late store, also before the first store. A guest write is ordered by the bind or flush that collects it: it replaces the depth drawn before it, and depth drawn from a later bind is stored over it.A pending HTILE fast clear wins over the reload: a tile HTILE marks cleared reads the clear value whatever the plane's bytes hold.
A color target flushes the stale planes under it before it retires their surfaces (
Draw.cpp, as the storage-image path already did), so a target that loads or blends reads the stored depth.retire()clears the surface'smemoryStaleand drops its range from the stale list, so a later flush never stores a retired plane's old depth over what the new owner of the memory wrote.A bind of a mirrorable target drops the plane images, and the pending store, of older surfaces whose planes overlap its plane over another range (another extent or base, as when a title changes its output resolution over the same memory). Those images stayed live and lent their old results over the new plane, also to buffer copies served as image copies. A same-range surface (another stencil or HTILE address) keeps its image. The scan runs only when a surface or plane image was created since the surface's last scan.
The surfaces mutex is now a
std::recursive_mutex, because a store reached from a flush underDepthSurfaceTexturere-enters it.copyPlanecan run in its ownCommandBatchwhen no recorder is active.Debt: a new entry, "Depth planes in guest memory", lists what stays incoherent: D16 planes, other swizzles, stencil, MSAA, CPU reads, reads through the heaps an address-based shader maps. It also states the ordering above and its granularity: the tracker stamps 64 KiB blocks and a store or reload covers the whole plane, so a partial guest write over unstored depth also drops that depth outside the written bytes. It also notes that console memory under a compressed HTILE holds compressed data until a DB decompress (HTILE definitions in Mesa
src/amd/common/ac_descriptors.h). The fix(libSceAgcDriver): retire a depth surface once another writer covers its memory #454 entry points to the new entry.No new GPU semantics are introduced. The bytes in guest memory come from the existing
kZ64KBXdetiler, the same equation a sampled view of the plane already uses.Tested
Arch Linux, kernel 7.2.9-zen, NVIDIA RTX 4070 Ti SUPER (driver 615.78.08), GCC 16.2.1, Release,
BUILD_TESTING=ON, onmain40315fc.agc_driver_depth_plane_buffer_memory(new,tests/execution/DepthPlaneBufferMemory.cpp, on theDepthFastClearharness, which gains a swizzle field and a vertex-buffer argument):Draws depth 0.5 into a 64x32 D32 Z_X plane, then fetches vertices from the plane. All 2048 texels must read 0.5.
Writes 0.75 into the plane's memory, then draws at 0.5 under LESS. The draw must pass.
Draws at 0.5 with depth write, then fetches again. Memory must hold that 0.5.
On a new plane: draws 0.5, writes 0.75 into memory before any store, then fetches the plane with a draw on another target. Memory must still hold 0.75, and a draw at 0.5 under LESS must pass. Then it writes 0.25 and draws 0.5 with depth write: the fetch must see 0.5.
On a plane with HTILE: stores 0.5, writes 0.25, then a whole-HTILE ZMask 0 fill. A draw at 0.5 under LESS must pass (the clear wins).
A linear R32F texture at an offset into a plane must read what memory holds after a flush, also when it is viewed through its cached storage image after another depth draw.
On a plane filled with 0.25: draws depth 0.5, then a color target at the same address that blends ZERO/ONE (keeps what it loaded), then fetches the plane. It must hold 0.5. Red without the color-target flush, with or without the
retire()change.Step 4 is red on the previous revision (064a08c): "the depth 0.5 a draw wrote before a guest write into the plane was stored over that write". Step 5 is red when the reload drops the pending clear, as 064a08c did. Step 6 is red on 53b20c4 at the cached storage image read.
Red on
mainat step 1 ("a vertex fetch from the depth plane did not see the depth 0.5 a draw wrote into it"). With step 1's check removed, it is red at step 2 ("a draw at depth 0.5 failed LESS against the 0.75 written into the depth plane's memory"). Green with the change. Steps 2 and 3 run only where guest memory is write-watched; the test prints when it is not.All 701
agc_tests pass (ctest -R ^agc_ -j8, on c8a8ff1);agc_driver_stencil_ref_exportskips on this device as onmain.Uncharted: Legacy of Thieves Collection (PPSA05684), on the local integration branch (main + open PRs): the main menu shows the cliff and the whole cage under the fog in all 15 menu frame dumps. Frame times against the earlier version are in the comments. The title does not run on plain
mainyet.Overlap with open PRs
DB_Z_WRITE_BASE. It touchesDepthSurface.cpp,GuestBufferMemory.cppandVulkanDevice.cppat other places.DepthSurfaceView(pendingClear |= htileClearedbecomesNoteHtileClear).DepthSurface.cpp.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)