Repository navigation
feat(agc): publish a written native multisample D32S8 surface to its guest planes on a guest read - #13021
Conversation
# Conflicts: # core/libs/prx/libSceAgcDriver/Graphics/src/MultisampleTarget.cpp
…guest planes on a guest read A native multisample D32S8 surface the host has written leaves its guest depth, stencil and HTILE planes stale. A guest read now takes the host samples back, retiles them into the guest layout, writes expanded HTILE words, and keeps every byte the guest itself wrote, proven by the write tracker. A byte it cannot prove (a same-value CPU store) still throws. The owner stays resident after publishing; once a guest byte was merged over the host's, a later draw on that target throws instead of re-importing a cleared image. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
@JAB-dev thanks for this. The per-byte rule (take a host byte only where the tracker proves the guest did not write it, and refuse when that can't be told) is the right shape for a publication that must not clobber a title's stores. We reviewed the commit on top of the branch (0a32528: 10 files, +807/-54; the 33 commits under it are #6300's and #9533's). This is not a full verdict on the numerics: the Z_X retile, the pipe-bank XOR and the HTILE word expansion rest on
Evidence limits worth stating in the description: the retile and the XOR are checked against your own import path, not against an independent console reading, and the title run was on a stack of four changes, not on this one alone. #13023 is built on this branch, so it waits for these changes. |
|
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.
|
… still holds the snapshot A guest store that landed after the last publication check, or between the plane commits, was overwritten by the host byte. The commit now exchanges each changed byte only while it still holds the snapshot value, refuses the whole part when the tracker saw a CPU store since the last collect, and throws when any byte was lost, keeping the guest's byte. WriteChanged keeps its previous behaviour for every other caller. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…for a held target list The tests for states that are valid but not implemented required one exact refusal message. They now accept a refusal or a later implementation: a refusal must change no guest byte, and a publication must write only host bytes and keep every byte the guest stored. A new case publishes while another thread holds the GPU mutex and the multisampled target list. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
❌ NVIDIA GPU test: failedPR head
New validation errorsThis PR changes test files, so each test below was also run on main with only the PR's test changes applied; 0 there means the PR's other changes cause the error.
agc_driver_depth_clear_hle: new validation messagesUnstable on main (4, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (2, not counted against this PR)Passed on retry (1, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validated but not compared (7, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 425 tests)Skipped (41)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
Data point on Windows with AMD's driver: AMD Radeon RX 9070 XT (RDNA 4), driver 32.0.32015.7029, Windows 11 Pro 22631, MSYS2 UCRT64 GCC 16.1, Release, Tree:
#13023 (this PR plus one commit) gives the same results on the same machine. AI-assisted: yes (Claude Code) |
# Conflicts: # core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp # core/libs/prx/libSceAgcDriver/Graphics/src/MultisampleTarget.cpp
|
@oneandonlydean thanks, pushed the changes for requests 1, 2, 4, 5 and 6; request 3 needs your call.
Rebase: |
jotitadev
left a comment
There was a problem hiding this comment.
Linux run of head bb085f7a40, since the description and the AMD data point are both Windows. On Linux NVIDIA the queued-fill HTILE check that skips on bunbeans2-arch's RX 9070 XT does run.
Setup: Ubuntu 26.04, GCC 15.2, Release. RTX 3070 (NVIDIA 595.99.02) and lavapipe (Mesa 26.0.3), one at a time via VK_DRIVER_FILES. Khronos validation layer 1.4.341, ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1, ctest -j1. The head merged --no-ff onto current main de96e3efbd with no conflicts. Every executable of the focused set was rebuilt for the tree.
- Focused set, 15 tests:
agc_driver_native_multisample_depth, theagc_driver_native_depth_*tests (includingpublication,driver_pathanddriver_queued_fill),multisample_retirement,depth_plane_fillanddepth_surface_from_storage. 15/15 in 3 of 3 runs on each device, 0VUID-lines. agc_driver_native_depth_driver_queued_fillis not skipped here:- RTX 3070:
cases checked=6 unavailable=0. Each case printscleared by a queued DMA_DATA fill of 128 KiB from its HTILE through the driver and sampled. - lavapipe:
cases checked=4 unavailable=2. The 2-sample cases are the unavailable ones; the 4- and 8-sample cases pass. - So the HTILE proof that bunbeans2-arch's Windows AMD setup skips ("host imports are compared here, not write-watched") is exercised on Linux NVIDIA.
- RTX 3070:
agc_driver_native_depth_bda_clearskips on lavapipe (the device does not keep write watching for native clear imports), as onmain.- Full
^agcsuite (without display/flip/lds_atomics64, no layer,--timeout 60) on #13023's head, which contains this branch: 1136/1136 on the RTX 3070 and 1136/1136 on lavapipe. ninja allplus everyagc*_teststarget builds. GCC prints no warnings on the lines this PR adds.
GPUs tested: NVIDIA GeForce RTX 3070 (595.99.02, Linux); lavapipe (Mesa 26.0.3, Linux).
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
✅ NVIDIA GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validated but not compared (7, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 421 tests)Skipped (42)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
# Conflicts: # core/libs/prx/libSceAgcDriver/CMakeLists.txt # core/libs/prx/libSceAgcDriver/Graphics/include/DepthSurface.hpp # core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp # core/libs/prx/libSceAgcDriver/tests/execution/NativeMultisampleDepth.cpp
|
On the order with #9742 (a colour alias over a native multisample depth plane when HTILE proves its clear), which conflicts with this PR in They also meet in a title. In Ghost of Tsushima Director's Cut (PPSA02225), the run with #9742 next stops on #9742's checklist now says |
…ve-d32s8-guest-planes # Conflicts: # core/libs/prx/libSceAgcDriver/Execution/src/GuestMemory.cpp # docs/dev/debt/libSceAgcDriver.md
XairRrR
left a comment
There was a problem hiding this comment.
Re-checked — approved
The only change since my last review is a merge of main (0180bafe62). It resolves the conflicts in GuestMemory.cpp and the AGC debt file correctly, and it brings in #14005's D32S8 owner without HTILE. The PR now refuses that owner cleanly. The focused depth tests pass. The one new validation message the script reported comes from main's own test, not from this PR.
What changed since my last review
- The PR's five commits (
0a3252876ftof6e7798ba9) are unchanged. The range-diff shows=for each one. - One new commit,
0180bafe62, mergesorigin/main4f41f4c375. It resolves conflicts inExecution/src/GuestMemory.cppanddocs/dev/debt/libSceAgcDriver.md:GuestMemory.cpp:main(#6185) splitWriteChangedinto a staticwriteChanged(..., keepGuest)and addedWriteChangedKeepingGuest. The merge keeps both ofmain's functions unchanged. The PR's own additions sit beside them:cpuUnchanged,UnchangedSinceLastCollect, thewalkedparameter ofrangeUnchanged(defaulttrue, so existing callers behave as before),storeOwn'scompleteparameter (defaulttrue) andWriteChangedIfHeld.WriteChangedis stillmain's for every other caller.- Debt file: the merge keeps
main's edits to the "Native multisampled depth sampling" entry. It also keeps the PR's dropped "Guest publication of a written surface" phrase, which was oneandonlydean's earlier request, and the new "Native D32S8 guest publication" entry.
- From
main, #14005 addsuniformPlaneTarget: D32S8 with a separate stencil plane and no HTILE, whosesampling.initialhas 2 ranges ({depth, stencil}). I checked how the PR handles these owners:NoteHostWritecaptures both ranges (DepthSurface.cpp:797).PublishToGuestthen refuses with... not implemented for this owner, becausepublication.size() == 3ufails (DepthSurface.cpp:2170). This happens before any readback dispatch.RebasePublication(2u)andRebasePublication(1u)cannot index out of range for these owners. The stencil-fill path that setsstencilClearGenerationrequiressampling.initial.size() > 2u. Both places that setclearGenerationneed an HTILE range, and this owner has none.- So a guest read of a written owner without HTILE still throws, as it does on
main, only with a different message.
- No author replies and no description edits since my last review.
What I ran
Head 0180bafe62f4dee4cdc04aee0d565be70c838460, main 4f41f4c375f5abb487dbd184fbc150f31994d762, merged --no-ff (tree d8eebd68d272cf9c6f239d7842d913b1dfd5119b) in a clean worktree. The machine is a Debian 13 VM with 2× Xeon Gold 6136, GCC 14.2.0 and lavapipe (Mesa 26.1.6, LLVM 19). It has no GPU.
git show --stat 0180bafe62f4dee4cdc04aee0d565be70c838460
git diff 4f41f4c375f5abb487dbd184fbc150f31994d762 d8eebd68d272cf9c6f239d7842d913b1dfd5119b -- core/libs/prx/libSceAgcDriver/Execution/src/GuestMemory.cpp core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp docs/dev/debt/libSceAgcDriver.md
git diff d1ae628bc2f62459cce1e9872ba5632c938c71f2 4f41f4c375f5abb487dbd184fbc150f31994d762 -- core/libs/prx/libSceAgcDriver/Execution/src/GuestMemory.cpp core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
git diff --check 4f41f4c375f5abb487dbd184fbc150f31994d762...0180bafe62f4dee4cdc04aee0d565be70c838460
python3 tools/check_conventions.py --base 4f41f4c375f5abb487dbd184fbc150f31994d762
ctest --test-dir build -j8 --test-load 32 --output-on-failure -R "agc_driver_native_depth|agc_driver_native_multisample_depth|multisample_retirement|depth_plane_fill|depth_surface_from_storage|stencil_write_intent"
VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 ctest --test-dir build -V -R "^agc_driver_native_depth_uniform_planes$"
diff <(git show 4f41f4c375:…/NativeMultisampleDepth.cpp | sed -n '/^void CheckUniformPlanesWithoutHtile/,/^}/p') <(sed -n '/^void CheckUniformPlanesWithoutHtile/,/^}/p' …/NativeMultisampleDepth.cpp)
git status --porcelain
| Check | Result |
|---|---|
git diff --check |
clean |
check_conventions.py |
0 errors, 1 warning: the debt entry at docs/dev/debt/libSceAgcDriver.md:69 has 3982 characters (unchanged) |
Configure / build / libs (script) |
ok |
| Selected tests (script, 2230 run) | 0 new failures, 0 timeouts. 7 known failures also fail on main: agc_driver_division, the denormal-mode tests, vector_constant_factor_tininess and unwrap_self_… |
| Skips (script) | 114, all expected according to check_skips.py |
| Validation (script, 770 tests, layer loaded in 769, none missing) | 1 test with a VUID not in the baseline: agc_driver_native_depth_uniform_planes, a test the baseline did not run. It comes from main; details below |
| Focused set (18 tests) | 17 passed. 1 skipped: agc_driver_native_depth_bda_clear, which skips on lavapipe on main too |
Where the VUID-vkCmdDispatch-groupCountX-00386 comes from
Native D32S8 without HTILE, samples=4: uniform planes imported and sampled, other planes refused
Validation Error: [ VUID-vkCmdDispatch-groupCountX-00386 ] | MessageID = 0x8e927036
vkCmdDispatch(): groupCountX (69144) exceeds device limit maxComputeWorkGroupCount[0] (65535).
… (4 times)
Native D32S8 without HTILE, samples=8: uniform planes imported and sampled, other planes refused
- 69144 = 129 × 67 × 8. That is the test-side
fetch.Run(batch.Handle(), extent.width * extent.height * samples)inCheckUniformPlanesWithoutHtile, which issues one workgroup per sample. It runs twice per value pair: once for the depth fetch and once for the stencil fetch. - That function came from
mainwith #14005. It is byte-identical in the merged tree (thediffabove prints nothing). - None of the PR's dispatches run for this owner:
PublishToGuestrefuses before its readback. - So this is not a finding against this PR. It belongs to #14005's test on
main, which should split the dispatch across X/Y.
I tried a second check: restore main's versions of the PR's files and rebuild, to rerun the test on main directly. The build tree could not be regenerated in this sandbox: configure_file fails with "Operation not permitted". I restored the merged files without rebuilding anything, so the binaries I tested are the script's build of the merged tree. git status --porcelain is empty.
Previous findings
NoteDepthSurfaceWritethrows for every multisample depth draw when no active surface matches (nowDepthSurface.cpp:2741). Still open, non-blocking. The code is unchanged and the description still does not mention this throw.- Unmeasured capture cost on the draw path:
NoteHostWrite(nowDepthSurface.cpp:792-808) copies every plane on each false-to-true flip. Still open, non-blocking. It now also runs for #14005's owners without HTILE, which can never be published (see the new finding), so that copy buys nothing for them. There are still no frame-time numbers. - The debt entry does not mention the "another identity" refusal (now
DepthSurface.cpp:2848), and the entry is 3982 characters. Still open, non-blocking. The debt file still does not mention it.
New findings
Blocking
None.
Non-blocking
- Owners without HTILE now reach
PublishToGuestand are refused, but neither the debt entry nor the description says so (core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp:2170,docs/dev/debt/libSceAgcDriver.md:68).- In the 2026-11-02 reply to hugodearaujo, the author said "an owner without HTILE cannot exist on this branch". That was true then. With #14005 merged into
main, a two-plane D32S8 owner now exists. - On a guest read after a draw, such an owner throws
sample-preserving guest publication of native multisample depth is not implemented for this owner. - The new debt entry lists only "D16/D32-only owners" among the owners that throw.
- Suggested fix: add "D32S8 owners without HTILE" to that list. Alternatively, now that the two-plane owner and its
--uniform-planesfixture are onmain, take hugodearaujo's two-plane change (skip the HTILE rewrite when there is none) as a follow-up. - Either way, skip the capture in
NoteHostWritefor owners that cannot be published.
- In the 2026-11-02 reply to hugodearaujo, the author said "an owner without HTILE cannot exist on this branch". That was true then. With #14005 merged into
Not verified
- Running
agc_driver_native_depth_uniform_planesonmaindirectly: the build tree could not be regenerated in this sandbox. Its VUID is attributed tomainby reading the code: the function that emits it is unchanged frommain, and the dispatch size matches the test's own fetch. - The Z_X retile, the pipe-bank XOR and the HTILE word expansion against console behaviour. They are only checked against this driver's own import path.
- The title runs (PPSA19015, PPSA06866) and any hardware GPU: this machine only has lavapipe.
- The
recordDispatch/StartDrawPassordering still has no test. I checked it only by reading the merged code. - No negative control this round. The PR's tests are unchanged and the new commit is only a merge. In an earlier round, two production mutations each made
agc_driver_native_depth_publicationfail.
✅ NVIDIA GPU test: passedPR head
Unstable on main (3, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validation IDs already present on main (4 IDs in 517 tests)Skipped (48)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
PPSA02801 on Windows with an NVIDIA GeForce RTX 4080 SUPER (driver 32.0.16.1074), two 600 s runs per arm, libraries built for each arm:
The build of the PR arm had one library fewer than the On AI-assisted (Claude Code). |
|
That order works for us: the sampling PR before this one, and the colour-alias PR after it. If the sampling PR is ready first, this PR merges main on top and takes its |
✅ AMD GPU test: passedPR head
Unstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (8, not counted against this PR)Passed on retry (1, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 517 tests)Skipped (58)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
Cross-title data from Spirit of the Island (PPSA13934), Windows / RX 6700 XT. Not a review of the change itself. On That is With this PR, I made a local merge onto
So for this title, the original throw no longer appeared in 8 runs. At the same point in the game, though, one run stopped on Nothing here argues against the change for PPSA19015. It just doesn't move Spirit past this point. AI-assisted: yes. |
|
Follow-up on the Spirit of the Island (PPSA13934) note above. The surface is D32S8, so this PR's format applies to it. I logged each native depth guard as That is AI-assisted: yes. |
#13021 takes the multisample targets lock through TargetsLock, which marks the lock held for HoldsMultisampleTargetsLock; MultisampleColorTargetsOver takes it the same way. Its declaration sits beside WithMultisampleTargetsLockForTest. The debt file keeps #13021's guest publication entry and its edit of the depth sampling entry, then this branch's two colour-target entries.
Draw.cpp conflicts where #13021 notes a depth surface write after the draw's resources are resolved. Both are kept, with the injected stop after resolve first, so an injected failure notes no depth write.
What
Arcade Game Zone (PPSA19015) stops on a guest read of a native multisample D32S8 depth surface that a draw has written, with
sample-preserving guest publication of native multisample depth is not implemented. With this change, a guest read now publishes the surface back to guest memory, and the title keeps playing: it ran for 27 minutes without a stop on this change together with the pull requests listed below.A read over a written native multisample D32S8 surface (a CPU read, a DMA source, a texture, storage, index or vertex read) now reads the samples back in the guest's layer, row, column, fragment order, retiles them into the Z_X planes with the descriptor's pipe-bank XOR, and rewrites the HTILE words to the expanded clear word. The surface keeps its host image and stays resident. The planes the guest wrote are never overwritten: the guest's bytes are snapshotted when the host takes ownership (and again after a proven whole-plane clear), and a byte is taken from the host only where the write tracker proves the guest did not write it. A byte it cannot prove (the guest stored the value the byte already held, which the tracker cannot tell from no store) throws
native multisample depth publication cannot distinguish an unwritten byte from a same-value guest storerather than guess. A capture that cannot be taken (a colour alias, a pending GPU write, an unproven store since import) leaves the owner unavailable for publication: draws keep rendering and a guest read throws the earlier refusal. D16 and D32-only owners, unproven clear transitions and publication inside a GPU completion still throw.After a guest byte was merged over the host's, a later draw on that target throws
guest writes over native multisample depthinstead of re-importing a cleared image. Publication's own stores are recorded as possible stores, not complete ones, so the HTILE clear proof is unaffected;WriteChangedand every other caller are unchanged frommain.Late guest stores. Each changed byte is exchanged with a compare-and-swap (8-byte words when aligned, bytes otherwise) only while it still holds the snapshot value, and a part is refused when the tracker saw a CPU store since its last collect. A late store of any other value is kept and the read throws
guest writes raced native multisample depth publication; planes already committed stay published and later reads throw. The one case still overwritten is a CPU store of the snapshot's own value that lands in a byte the host changes while that part commits. It is in the debt entry.Lock order.
GpuMutex,surfacesMutex, the multisample targets mutex, thenguardedMutexordepthGuardMutex, then the write tracker. The allocation lease is a snapshot taken under the registry mutex and released at once, and publication dropsdepthGuardMutexbefore the colour-target lookup. Publication runs withGpuMutexheld, so the flush hook re-enters it at depth 2 and never releases it whilesurfacesMutexis held. By reading the code, no production thread takesGpuMutexwhile holding the targets mutex (nothing enforces it, and some tests acquire targets withoutGpuMutex): acquisition, sampling, storage and resolve record commands underGpuMutex, andNoteColorMetadataFillholds the targets mutex withoutGpuMutexbut waits on nothing. Read-guard callers withoutGpuMutexare game-thread host reads and the PM4 DMA-source check; they takeGpuMutexas their outermost driver lock.Other titles.
NoteDepthSurfaceWritereturns at once for a single-sample target. For a multisample depth-writing draw it takes the surfaces lock, marks every active matching owner as host-written, and throwsa native multisample depth attachment was retired while its draw resolved resourceswhen no active owner matches the draw's target; it now runs after the resources and the recipe proof, so a draw that a proof or a resource build refuses no longer marks the surface written.Evidence limits. The Z_X retile, the pipe-bank XOR and the HTILE word expansion are checked against this driver's own import path, not against an independent reading of the console. The title run was on a stack of four changes (this one, the stencil plane change, the staged 4_4_4_4 partial write change and the D32S8 import change), not on this one alone.
Tested
Windows 11, NVIDIA RTX 4070 SUPER, driver 610.88.
agc_driver_native_depth_publication(new): 2, 4 and 8 samples with each kind of clear; guest writes to depth, stencil and HTILE pages; a driver store of the same value; a queued fill; partial unmaps, remapped and inaccessible holes; a write during the copy; a concurrent read from two threads; a colour alias, a pending draw and publication inside a GPU completion (each throws with its own message); a draw after publishing sees the published samples; a DCC colour target over a host-written surface throws instead of deadlocking. A guest store after the last validation and one between the plane commits are kept and refused;WriteChangedIfHeldis checked on unaligned heads and tails, on a byte the host leaves alone and on a byte it changes; publication while another thread holds the GPU mutex and the multisampled target list completes once it is released. States that are valid but not implemented (a GPU completion, a colour alias, a pending draw, a same-value store, a DCC target) assert that a refusal changes no guest byte and keeps the owner, and that a publication writes only host bytes and keeps every byte the guest stored.agc_driver_native_multisample_depthand the otheragc_driver_native_depth_*tests,agc_driver_multisample_retirement,DepthPlaneFill,DepthSurfaceFromStorageandNativeDepthBdaClear.agc_drivertests that touch draws, targets, multisampling and pipelines, andcheck_conventions.py.mainwith the open D32S8 import change) in all 37 focused tests (the VUIDs areVUID-VkDeviceCreateInfo-pNext-pNext,VUID-VkPhysicalDeviceProperties2-pNext-pNext,VUID-VkGraphicsPipelineCreateInfo-renderPass-06053andVUID-VkPipelineColorBlendStateCreateInfo-pAttachments-00605, all reported by the base), and no log reportswas not found but was requested by env var VK_INSTANCE_LAYERS.agc_driver_native_depth_publicationreports none.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)DepthSurface.cppand its test, and the maintainers can pick the order🤖 Generated with Claude Code