Repository navigation
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>
github-actions
Bot
added
the
unblocks title
12 tasks done
|
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>
12 tasks done
…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>
…e the guest stored its clear value into A guest that stores the stencil clear value over a separate stencil plane while the host takes the plane for import stamped the write tracker, and the import refused with "guest writes raced native multisample depth import" although the plane still held its clear value. The plane is now excluded from the write-generation check and proven by its content instead, read again after a fresh collect: a store of the same value imports, a store of any other value still refuses. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
JAB-dev
force-pushed
the
fix/agc-import-d32s8-stored-stencil-clear
branch
from
October 29, 2026 23:30
d66ae35 to
790e422
Compare
Contributor
✅ 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 (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. |
Contributor
|
Same machine and method as my comment on #13021 (RX 9070 XT, AMD's Windows driver 32.0.32015.7029, validation layer on, AI-assisted: yes (Claude Code) |
12 tasks done
# Conflicts: # core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp # core/libs/prx/libSceAgcDriver/Graphics/src/MultisampleTarget.cpp
…d32s8-stored-stencil-clear
Contributor
Author
|
Merged the updated publication branch (which carries |
jotitadev
reviewed
Oct 30, 2026
jotitadev
left a comment
jotitadev
left a comment
Contributor
There was a problem hiding this comment.
Linux run of head 93673d8da5, with a negative control for the new same-value case.
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.
- Negative control. I took #13021's head (this PR's base) and added only 790e422's test changes, its
StencilPlaneReadenum value and the one hook line, with none of the import change.agc_driver_native_depth_separate_stencilthen fails on both devices withguest writes raced native multisample depth import. So the new case does catch the refusal this PR removes. - This head, focused set, 15 tests:
agc_driver_native_multisample_depth, theagc_driver_native_depth_*tests,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_fillruns its HTILE check here instead of skipping. On the RTX 3070 it checks 6 of 6 cases; on lavapipe it checks 4, and the 2-sample cases are unavailable.- With the older layer the NVIDIA CI uses (1.3.275), the focused set also passes on the RTX 3070. That layer reports
VkDeviceCreateInfo-pNext-pNextandvkCmdCopyImage-srcImage-01548/01551, which the CI already lists formain. - Full
^agcsuite (without display/flip/lds_atomics64, no layer,--timeout 60): 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.
Contributor
✅ NVIDIA GPU test: passedPR head
Failing on main too (4, 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. |
12 tasks done
# 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
…d32s8-stored-stencil-clear
Contributor
✅ 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)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 495 tests)Skipped (45)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. |
XairRrR
approved these changes
Nov 2, 2026
XairRrR
left a comment
XairRrR
left a comment
Contributor
There was a problem hiding this comment.
Reviewed — approved
The PR's own change (790e422437) proves the separate stencil plane by its content after a fresh collect, instead of by its write generation. It is sound, the focused tests pass, and the new same-value case fails when the old check is put back.
What I ran
Head 474d5989695cd1ca836b90923e0d78d3bf299d85, merged --no-ff onto main 9090972b7e91fd266819cbd77a8b058e177b5682. The merge was clean. The build tree was Release, GCC 14.2.0, Ninja, BUILD_TESTING=ON. The machine is Debian 13 (VM, 32 vCPU Xeon Gold 6136), with Vulkan through lavapipe (Mesa 26.1.6, LLVM 19) and no GPU.
git diff --check 9090972b7e91fd266819cbd77a8b058e177b5682...474d5989695cd1ca836b90923e0d78d3bf299d85
python3 tools/check_conventions.py --base 9090972b7e91fd266819cbd77a8b058e177b5682
ANYPS5_REQUIRE_VULKAN=1 ctest --test-dir build -j8 --test-load 32 --output-on-failure -R "agc_driver_native_(depth|multisample)|agc_driver_multisample_retirement|agc_driver_depth_plane_fill|agc_driver_depth_surface_from_storage"
ANYPS5_REQUIRE_VULKAN=1 ctest --test-dir build -V -R "^agc_driver_native_depth_separate_stencil$"
# negative control (see below)
ninja -C build -j32 -l32 agc_driver_native_multisample_depth_tests
ANYPS5_REQUIRE_VULKAN=1 ctest --test-dir build --output-on-failure -R "^agc_driver_native_depth_separate_stencil$"
# restore merged DepthSurface.cpp, rebuild, rerun
ANYPS5_REQUIRE_VULKAN=1 ctest --test-dir build -R "^agc_driver_native_depth_(separate_stencil|publication)$"
git status --porcelain
Results
| Check | Result |
|---|---|
git diff --check |
clean |
check_conventions.py |
0 errors, 1 warning that was already there (see Non-blocking) |
Configure / build / libs (script) |
passed |
| Selected tests (script): 2099 ran | 0 new failures, 0 timeouts; the 7 known failures (agc_driver_division, the denormal-mode tests, unwrap_self, vector_constant_factor_tininess) also fail on main |
| Validation-layer comparison (script): 757 tests | layer loaded in 756, missing in none; 0 tests with new VUID- lines |
| Focused native-depth set: 17 tests | 16 passed, 1 skipped (agc_driver_native_depth_bda_clear, a device limitation), all under 3 s each |
agc_driver_native_depth_separate_stencil |
passed, 1.8 s, checked=42 unavailable=21 |
agc_driver_native_depth_publication |
passed, 1.6 s |
| Worktree after restore | git status --porcelain empty |
separate_stencil output (merged tree)
Native depth with a separate stencil plane stored the clear value again during import: imported; stored another value: refused
Native depth with a separate stencil plane cases checked=42 unavailable=21
1/1 Test #1675: agc_driver_native_depth_separate_stencil ... Passed 1.80 sec
Negative control
main's production files cannot be restored as they are: the test needs the DepthPublicationStage::StencilPlaneRead hook, which this branch adds through #13021. So I put back only the import check from main. I replaced the block after the plane read with main's if (!GuestMemory::UnchangedSinceAll(state.initial)) return refuse("guest writes raced native multisample depth import");. The hook and the whole test source stayed the same. After a rebuild, agc_driver_native_depth_separate_stencil failed:
AGC graphics: guest writes raced native multisample depth import
0% tests passed, 1 tests failed out of 1
I then restored the merged DepthSurface.cpp and rebuilt. separate_stencil and publication passed again, and the worktree was back to the merged tree. This agrees with jotitadev's negative control and RNK123's one-expression mutation in the discussion. It is a third run of the same check, on lavapipe and GCC 14.
Findings
Blocking
None.
Non-blocking
- The diff still carries #13021. Most of the +1009 lines are the publication change (
WriteChangedIfHeld,UnchangedSinceLastCollect,PublishToGuest, the publication tests and the debt entry). This PR's own topic is the 15 lines inprepareDepthSampling(core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp:221-241in the merged tree) and the two newPlaneFaultcases. CnRJay already raised this, and the author answered and declaredDepends on: #13021. I mention it only so this PR is merged after #13021. I read the publication part only as far as it affects the import change; its full review belongs on #13021. - The import reads the stencil plane twice. It now reads the whole plane a second time after the extra collect. At 8 samples and a large extent, that is tens of MB copied again once per import. It is correct, and since it happens once per surface it is unlikely to matter. If it ever shows up in a profile, the first content check could be skipped when the second one runs.
- The
debt-lengthwarning fromcheck_conventions.pyis on the "Native multisampled depth sampling" entry (docs/dev/debt/libSceAgcDriver.md:67). That entry is already far over 1000 characters onmain, and this branch only shortens it, through #13021's commit. The warning is not from this PR.
Correctness of the change
- The plane is removed from
UnchangedSinceAllonly when it was proven by content (provedPlane). Aliased or unproven planes keep the old generation check. - After that, the plane's generation is moved forward to a fresh
CollectWritesUncached, and the plane is read again.- A store that lands before that collect is covered by the reread: a different value refuses with the same message, and the clear value is accepted.
- A store that lands after the collect is stamped after the new generation, so the existing checks at the next sample catch it. This matches the description's "still throws at its next sample, as before".
- The depth and HTILE ranges keep their original generations, so nothing changes for them.
- The behaviour is general and not tied to one title. Nothing new silently succeeds: a different value still refuses.
- Checklist lines are present: Depends on, Fixes, Unblocks PPSA19015 with its discussion, GPUs tested, and AI-assisted. The PR adds no code comments, and new tests finish well under 30 s.
Not verified
- Only lavapipe here.
separate_stencilreports 21 cases unavailable on this device, so the 2- and 8-sample same-value cases may not all have run on this machine. The discussion has reports that they run on an RTX 3070 (jotitadev) and an RX 9060 XT with RADV (RNK123). - No Windows run. I did not check the Windows path, where host imports are compared rather than write-watched.
- No title run. I did not launch the title. The author's 27-minute Arcade Game Zone run also included two other changes that are not in this PR (the publication branch and a staged 4_4_4_4 partial-write change).
- #13021's publication code got only a light read. I did not review it in depth.
…ve-d32s8-guest-planes # Conflicts: # core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp
…c-import-d32s8-stored-stencil-clear
github-actions
Bot
added
review queue
Contributor
✅ NVIDIA 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 (3, not counted against this PR)Validation IDs already present on main (4 IDs in 526 tests)Skipped (49)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. |
Contributor
✅ AMD 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 (8, not counted against this PR)Validation IDs already present on main (4 IDs in 526 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. |
…ve-d32s8-guest-planes # Conflicts: # core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
XairRrR
approved these changes
Nov 3, 2026
XairRrR
left a comment
XairRrR
left a comment
Contributor
There was a problem hiding this comment.
Re-checked — approved
Since my review of 474d598, the only changes are two merges of main: 456c2ef40b into the publication branch, which resolved a conflict in VulkanDevice.cpp, and f49ecfc3ba into this branch. The resolution is correct, and the focused tests still pass. The two new failures in the script's run also happen on main.
What changed since my last review
- The PR's own commits are unchanged. The range-diff shows all six commits identical (
0a3252876f…f6e7798ba9). - Merge
456c2ef40bresolved a conflict incore/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp.mainhad added the scratch-pool lookup (ScratchPoolFor) inrecordDispatch. The branch'sresources.RequireGuestBuffers()now comes right afterStoreDrawnLinearTouching, before the scratch-pool block. - No new author comments or replies to my review. The description and checklist are unchanged.
What I ran
Head f49ecfc3ba4ef68f14e2fba312267dde8921aba0, merged --no-ff onto main 5cb015caaaaf83f8ee431414ee6a457b5b307d49 (merged tree cf1f9afcfa321796927b3c32e6e4cd60da475eeb). Debian 13 VM, GCC 14.2.0, Release, lavapipe (Mesa 26.1.6).
git diff --check 5cb015caaaaf83f8ee431414ee6a457b5b307d49...f49ecfc3ba4ef68f14e2fba312267dde8921aba0
python3 tools/check_conventions.py --base 5cb015caaaaf83f8ee431414ee6a457b5b307d49
git show --cc 456c2ef40b64 -- core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp
ANYPS5_REQUIRE_VULKAN=1 ctest --test-dir build -j8 --test-load 32 -R "agc_driver_native_(depth|multisample)|agc_driver_multisample_retirement|agc_driver_depth_plane_fill|agc_driver_depth_surface_from_storage"
ANYPS5_REQUIRE_VULKAN=1 ctest --test-dir build --output-on-failure -R "^agc_driver_image_sample_lod_clamp_shader_lane$"
# put main's version back in the 9 production files the PR changes, then:
ninja -C build -j32 -l32 agc_driver_image_sample_lod_clamp_tests
ANYPS5_REQUIRE_VULKAN=1 ctest --test-dir build --output-on-failure -R "^agc_driver_image_sample_lod_clamp_shader_lane$"
# restore the merged files, then:
ninja -C build -j32 -l32 agc_driver_image_sample_lod_clamp_tests agc_driver_native_multisample_depth_tests
ANYPS5_REQUIRE_VULKAN=1 ctest --test-dir build -R "^agc_driver_native_depth_(separate_stencil|publication)$"
git status --porcelain
| Check | Result |
|---|---|
git diff --check |
clean |
check_conventions.py |
0 errors; the same debt-length warning as before, on docs/dev/debt/libSceAgcDriver.md:67 (already there on main) |
Build / libs (script) |
passed |
| Selected tests (script): 2235 ran | 2 new failures listed, both from main (see below); 0 timeouts |
| Validation comparison (script): 759 tests | layer loaded in 758, missing in none, 0 tests with new VUID- lines |
| Focused native-depth set: 17 tests | 16 passed, 1 skipped (agc_driver_native_depth_bda_clear, a device limitation) |
separate_stencil and publication after the restore |
passed |
| Worktree after the restore | git status --porcelain is empty |
The two failures the script lists as new do not come from this PR:
agc_driver_image_sample_lod_clamp_shader_lane. This test was added onmainbyfe18496c95, after the script's baseline096c455. With the PR's 9 production files restored frommainand the target rebuilt, it still fails with the samethread 0 component 1 is 0.000000, expected 0.062500message.unwrap_self_UnwrapTest_test_new_output_inherits_the_destination_directory_group. This subtest is new onmaintoo, from the unwrap test split (a8090ef04e). It fails onos.chownwithEINVALin this sandbox. The PR touches nothing intools/.
Previous findings
- The diff still carries #13021: Still open, non-blocking. The branch still contains the publication series. Merge #13021 first, as
Depends onsays. - The import reads the stencil plane twice: Still open, non-blocking. The code is unchanged (
DepthSurface.cppinprepareDepthSampling). - The
debt-lengthwarning: No longer applies as a finding against this PR. The warning is unchanged, and the entry was already over the limit onmain.
New findings
Blocking
None.
Non-blocking
None. In the resolved recordDispatch, RequireGuestBuffers() still runs before recorder.Commands / OverlapCommands takes the command buffer, which is what 24869a37ea intends. The ScratchPoolFor block main added sits between the two and does not take a command buffer first.
Not verified
- Negative control not rerun. The test sources and the import change are byte-identical to
474d598(range-diff=), so the negative control from my previous review still applies. - Only lavapipe. No other GPUs, no Windows run and no title run. The NVIDIA and AMD runs in the discussion for this head report 0 new failures and no new validation IDs.
…c-import-d32s8-stored-stencil-clear
github-actions
Bot
removed
the
review queue
XairRrR
approved these changes
Nov 3, 2026
XairRrR
left a comment
XairRrR
left a comment
Contributor
There was a problem hiding this comment.
Re-checked — approved
Since my review of f49ecfc, the only change is a new merge of main. It resolves a conflict in Draw.cpp with main's stencil-write-intent depth view (merged into main in 66749882e0). The resolution keeps both sides correctly. The checks pass, and so do the focused tests, including the stencil-plane test that main added.
What changed since my last review
- The PR's own commits are unchanged. delta.patch shows all six commits identical (
0a3252876f…f6e7798ba9). - Merge
6e2b8263c2(maind1ae628bc2into the publication branch) resolved a conflict incore/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp, in two places:prepareDrawandDrawWithRecipe.- What
mainchanged: it replacedDepthSurfaceView(...)withDepthSurfaceViewWithStencilWriteIntent(..., writesStencilTarget(state)). ItsNoteDepthSurfaceWritecall stayed right after the view. - What the branch already had:
NoteDepthSurfaceWritemoved later, afterresolveDrawResourcesinprepareDrawand afterCheckBufferAliasesinDrawWithRecipe. - The resolution: it takes
main's new view call and keeps the branch's laterNoteDepthSurfaceWriteplacement, atDraw.cpp:3089andDraw.cpp:3879. Neither side is lost, and the call is not duplicated. writesDepthTargetalready includeswritesStencilTarget(Draw.cpp:398), so the moved call still notes stencil-only writes.
- What
- Merge
4767e11930(the publication branch into this branch) carries only thatmainmerge. - No new author comments or replies. The description and checklist are unchanged.
What I ran
Head 4767e11930a92af5cadcb224b61eef6ee22589ad, merged --no-ff onto main d1ae628bc2f62459cce1e9872ba5632c938c71f2 (merged tree 09053e576e99960cf70ef8fa883c4ab312eb7671). Debian 13 VM, GCC 14.2.0, Release, lavapipe (Mesa 26.1.6).
git show --cc 6e2b8263c246 -- core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
git diff d1ae628bc2f62459cce1e9872ba5632c938c71f2 09053e576e99960cf70ef8fa883c4ab312eb7671 -- core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
git diff --check d1ae628bc2f62459cce1e9872ba5632c938c71f2...4767e11930a92af5cadcb224b61eef6ee22589ad
python3 tools/check_conventions.py --base d1ae628bc2f62459cce1e9872ba5632c938c71f2
ANYPS5_REQUIRE_VULKAN=1 ctest --test-dir build -j8 --test-load 32 --output-on-failure -R "agc_driver_native_(depth|multisample)|agc_driver_multisample_retirement|agc_driver_depth_plane_fill|agc_driver_depth_surface_from_storage|stencil_plane"
git status --porcelain
| Check | Result |
|---|---|
git diff --check |
clean |
check_conventions.py |
0 errors. One debt-length warning on docs/dev/debt/libSceAgcDriver.md:67, already there on main |
Build / libs (script) |
passed |
| Selected tests (script): 2199 ran | 0 new failures, 0 timeouts. The 7 known failures also fail on main |
| Validation comparison (script): 767 tests | layer loaded in 766 and missing in none; 0 tests with new VUID- lines |
Focused set: 18 tests, including agc_driver_stencil_plane_memory from main |
17 passed, 1 skipped (agc_driver_native_depth_bda_clear, a device limitation) |
| Worktree | git status --porcelain is empty |
Previous findings
- The diff still carries #13021: Still open, non-blocking. The branch still contains the publication series. Merge #13021 first, as
Depends onsays. - The import reads the stencil plane twice: Still open, non-blocking.
prepareDepthSamplinginDepthSurface.cppis unchanged.
New findings
Blocking
None.
Non-blocking
None.
Not verified
- Negative control not rerun. The PR's commits are identical to the ones I reviewed before (range-diff
=), and the merge changed no test of this PR. The earlier negative control still applies. - Only lavapipe. I made no run on another GPU, on Windows, or on a title. The NVIDIA and AMD CI runs in the discussion cover
f49ecfc, not this head.
github-actions
Bot
added
needs rebase
…ve-d32s8-guest-planes # Conflicts: # core/libs/prx/libSceAgcDriver/Execution/src/GuestMemory.cpp # docs/dev/debt/libSceAgcDriver.md
…c-import-d32s8-stored-stencil-clear
XairRrR
approved these changes
Nov 3, 2026
XairRrR
left a comment
XairRrR
left a comment
Contributor
There was a problem hiding this comment.
Re-checked — approved
Since 4767e11, the only change is a new merge of main. It resolves conflicts in GuestMemory.cpp and docs/dev/debt/libSceAgcDriver.md and keeps both sides in each. The checks and the focused tests pass. The one new VUID- the script reports comes from a test that main added, not from this PR.
What changed since my last review
- The PR's own commits are unchanged. delta.patch shows all six commits identical (
0a3252876f…f6e7798ba9). - Merge
0180bafe62(main4f41f4c375into the publication branch) resolved two conflicts:Execution/src/GuestMemory.cpp:mainaddedWriteChanged(..., byteStoreLimit)andWriteChangedKeepingGuest, both throughwriteChanged(..., keepGuest). The branch had addedWriteChangedIfHeld. The resolution keeps all three, placed next to each other. All of the branch's other tracker changes are intact:cpuUnchanged, thewalkedparameter ofrangeUnchanged,storeOwn(..., complete)andUnchangedSinceLastCollect.git diff 4f41f4c375 4e3f9208d2on this file is exactly the branch's earlier change againstd1ae628bc2.main's callers (GuestBufferMemory.cpp:6837,GuestBufferMemory.cpp:6846) and its test (tests/Pm4.cpp:895) still reachWriteChangedKeepingGuest.- Debt file: the branch's edited "Native multisampled depth clears" entry, its new "Native D32S8 guest publication" entry and its trimmed sampling entry are kept.
main's new "Native multisampled D32S8 depth without HTILE" entry is kept atdocs/dev/debt/libSceAgcDriver.md:70.
DepthSurface.cppmerged without conflicts withmain's import of D32S8 targets without HTILE. I checked how that new owner interacts with publication:- Its
sampling.initialholds two planes (DepthSurface.cpp:190), soNoteHostWritecaptures two. PublishToGuestthen throws at its explicitpublication.size() == 3ucheck (DepthSurface.cpp:2184). It fails loudly rather than mis-indexing the stencil plane as HTILE.
- Its
- Merge
a4cd5ea600(the publication branch into this branch) carries only that merge. - No new author comments or replies. The description and checklist are unchanged.
What I ran
Head a4cd5ea60044c2ea4e331a0ac99fec155d09027e, merged --no-ff onto main 4f41f4c375f5abb487dbd184fbc150f31994d762 (merged tree 4e3f9208d2d619691bc38032ab1b3250e43bab82). Debian 13 VM, GCC 14.2.0, Release, lavapipe (Mesa 26.1.6).
git show --cc 0180bafe62f4 -- core/libs/prx/libSceAgcDriver/Execution/src/GuestMemory.cpp docs/dev/debt/libSceAgcDriver.md
git diff 4f41f4c375 4e3f9208d2 -- core/libs/prx/libSceAgcDriver/Execution/src/GuestMemory.cpp core/libs/prx/libSceAgcDriver/Execution/include/GuestMemory.hpp
git diff d1ae628bc2 6e2b8263c2 -- core/libs/prx/libSceAgcDriver/Execution/src/GuestMemory.cpp
git diff --check 4f41f4c375f5abb487dbd184fbc150f31994d762...a4cd5ea60044c2ea4e331a0ac99fec155d09027e
python3 tools/check_conventions.py --base 4f41f4c375f5abb487dbd184fbc150f31994d762
env VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 VK_LOADER_DEBUG=error,warn,layer ctest --test-dir build -V -R "^agc_driver_native_depth_uniform_planes$"
ANYPS5_REQUIRE_VULKAN=1 ctest --test-dir build -j8 --test-load 32 --output-on-failure -R "agc_driver_native_(depth|multisample)|agc_driver_multisample_retirement|agc_driver_depth_plane_fill|agc_driver_depth_surface_from_storage|stencil_plane"
git status --porcelain
| Check | Result |
|---|---|
git diff --check |
clean |
check_conventions.py |
0 errors. One debt-length warning on docs/dev/debt/libSceAgcDriver.md:69 (the sampling entry, which main already flagged) |
Build / libs (script) |
passed |
| Selected tests (script): 2230 ran | 0 new failures, 0 timeouts. The 7 known failures also fail on main |
| Validation comparison (script): 770 tests | layer loaded in 769 and missing in none. One test has new VUID- lines: agc_driver_native_depth_uniform_planes, which did not run on the baseline (see below) |
| Focused set: 19 tests | 18 passed, 1 skipped (agc_driver_native_depth_bda_clear, a device limitation) |
| Worktree | git status --porcelain is empty |
About the new VUID. agc_driver_native_depth_uniform_planes was added by main. The script's baseline (d1ae628bc2) is older than that test, so it had nothing to compare against. When I ran the test alone with the layer, it passes and prints 8 × VUID-vkCmdDispatch-groupCountX-00386.
Message
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).
69144 is 129 × 67 × 8. The source is the test's own fetch:
fetch.Run(batch.Handle(), extent.width * extent.height * samples)inCheckUniformPlanesWithoutHtile(tests/execution/NativeMultisampleDepth.cpp:3165-3167, and again for the stencil plane right after it).Fetch::Run(NativeMultisampleDepth.cpp:519-522) dispatches that count asgroupCountX.- The extent is
{129u, 67u}(NativeMultisampleDepth.cpp:3112).
This PR does not touch that function. The VUID is main's, so I do not count it against this PR. It is worth a separate fix on main: split the dispatch, or use a 2D grid.
Previous findings
- The diff still carries #13021: Still open, non-blocking. The branch still contains the publication series. Merge #13021 first, as
Depends onsays. - The import reads the stencil plane twice: Still open, non-blocking. The PR's commits are unchanged (range-diff
=), and the merge did not touchprepareDepthSampling's stencil proof.
New findings
Blocking
None.
Non-blocking
- The publication debt entry omits
main's D32S8 owners without HTILE (docs/dev/debt/libSceAgcDriver.md:68).- The entry lists "D16/D32-only owners" among the cases that throw.
- Since this merge, a host-written D32S8 owner imported without HTILE (
DepthSurface.cpp:190) also throws on a guest read. Its two captured planes failpublication.size() == 3u(DepthSurface.cpp:2184). - The behaviour is correct, since it throws, but the gap is not recorded. Suggested fix: add "D32S8 owners without HTILE" to that sentence.
Not verified
- Negative control not rerun. The PR's commits are identical to the ones I reviewed before, and the merge changed none of this PR's tests. The earlier negative control still applies.
- No main-only rerun of the new VUID.
- I could not rebuild the test binary with the PR's files restored from
main: CMake regeneration is not permitted in my sandbox. - I attribute the VUID to
mainfrom the source instead. The dispatch size comes frommain's unchanged test code, which this PR does not modify.
- I could not rebuild the test binary with the PR's files restored from
- Only lavapipe. I made no run on another GPU, on Windows, or on a title. The NVIDIA and AMD CI runs in the discussion cover
f49ecfc, not this head.
github-actions
Bot
added
the
review queue
Contributor
✅ NVIDIA GPU test: passedPR head
Unstable on main (4, 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. |
12 tasks done
Contributor
✅ 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)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. |
|
Windows and NVIDIA Blackwell run, for head
Both failures fail on The new Not run: the Khronos validation layer is not installed on this machine, so this says nothing about VUIDs. PPSA19015 was not run. Edited after posting: corrected why the new test was missing from the run. AI-assisted: yes. Claude Code ran the builds and tests and drafted this comment; the numbers are from the runs on this machine. |
Contributor
|
Windows data from a second title, Spirit of the Island (PPSA13934), whose native multisample depth target is D32S8 at 8 samples. Same scripted new game to the island, Windows / RX 6700 XT. This PR's head With
So for this title the PR does what it says: the import refused on main now goes through. The title then stops at the same moment on the next limit, For context, without AI-assisted: yes. |
12 tasks done
Contributor
|
A GFX8 data point: AMD Radeon RX 570 (RADV POLARIS10), Mesa 26.2.5, SteamOS 3.8.28 (Arch distrobox), GCC 16.2.1, Release. Head On GFX8, 11 of the 17
The 1,327 AGC, shader, VideoOut and Vulkan tests, this PR on
AI-assisted: yes (Claude Code) |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Arcade Game Zone (PPSA19015) can be refused at the native multisample D32S8 import with
guest writes raced native multisample depth importalthough the separate stencil plane still holds its clear value: the guest stores the clear value over the plane while the host takes the plane, the write tracker sees a store, and the import gives up. The plane is now excluded from the write-generation check and proven by its content instead, read again after a fresh collect. A store of the clear value over the plane imports; a store of any other value is still refused with the same message. A guest that keeps storing the clear value after the import still throws at its next sample, as before.Tested
Windows 11, NVIDIA RTX 4070 SUPER, driver 610.88.
agc_driver_native_depth_separate_stencil: new cases at 2, 4 and 8 samples store the clear value into the plane during the import (imports and samples), and one stores another value (refused). The same-value case fails without the change.agc_driver_native_multisample_depthand the otheragc_driver_native_depth_*tests, 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.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)🤖 Generated with Claude Code