Repository navigation
fix(agc): reload a sampled depth plane the guest wrote since its surface was bound - #12238
Merged
adriadam10 merged 2 commits intoOct 30, 2026
Merged
Conversation
github-actions
Bot
added
the
unblocks title
MarcZX10
suggested changes
Oct 28, 2026
MarcZX10
left a comment
MarcZX10
left a comment
Contributor
There was a problem hiding this comment.
The targeted test and negative-control evidence are strong, and the one-line reload is placed before the sampled image is consumed. I did not find a separate functional defect in the changed path. This branch is 38 commits behind current main, though; the repository contribution rules require PR branches to stay up to date. Please rebase/update on current main (including resolving the debt-file overlap with #12210 if it is still open), rerun the focused depth-plane test and validation checks on the rebased head, then I can reassess.
Collaborator
|
@XairRrR thanks, the change is the right one: the sampled path now runs the same |
|
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.
|
MarcZX10
suggested changes
Oct 28, 2026
MarcZX10
left a comment
MarcZX10
left a comment
Contributor
There was a problem hiding this comment.
I found one additional ordering case to address alongside the requested rebase and focused retest.
| retireStoringStale(*found); | ||
| return nullptr; | ||
| } | ||
| if (sampled && !stencil && mirrorable(base)) found->SyncFromMemory(mirrorResource(base)); |
Contributor
There was a problem hiding this comment.
Please preserve a fresh CollectWritesUncached before storing a stale depth plane in FlushDepthPlanes, or add an equivalent uncached check. This path calls SyncFromMemory → WrittenUnits, which uses memoized CollectWrites. GuestMemory.hpp documents that a same-epoch memo can hide a CPU write made after an earlier collect, and that uncached collection exists to avoid stale GPU results overwriting reused memory. If FlushDepthPlanes reaches a memoized range after a guest write, changed can remain empty and StoreDepthPlane can overwrite that update with stale depth. The new test covers a write followed by another sample, but not a pending-plane flush after a collect has already memoized the range. Please retain the fresh walk or add a regression proving this ordering is safe.
github-actions
Bot
added
needs rebase
adriadam10
requested changes
Oct 28, 2026
adriadam10
left a comment
adriadam10
left a comment
Collaborator
There was a problem hiding this comment.
The change is the right one: DepthSurface.cpp:2491 runs the same SyncFromMemory behind the same mirrorable gate as the bind path, and the new test pins 0x3f400000 after a guest write, which main would read as 0x3e800000. Two things before it can land:
- Cost: as @oneandonlydean asked, the check now runs on every sample of a single-sample D32 plane (a
CollectWritesplus a generation compare when nothing was written, a unit reload when something was). Please add a profile line or a before/after frame time (APS5_PROFILE_DRAWorAPS5_TRACE_SUMMARY_EVERY) from a scene that samples such a plane many times per frame, or say how you measured it as negligible. - The ordering case @MarcZX10 raised (memoized
CollectWriteshiding a CPU write beforeFlushDepthPlanesstores a stale plane) is in code this PR does not change:FlushDepthPlanesalready callsSyncFromMemory(DepthSurface.cpp:2221) and the new line only adds a second caller. If you can show it is a real problem, it belongs in its own PR; please say in the thread whether the sample path makes it reachable in a way the bind path does not.
The branch also conflicts with main (the Depth planes in guest memory debt entry, which #12210 edits too); we will do that rebase once the cost line is in.
12 tasks done
XairRrR
force-pushed
the
fix/agc-sampled-depth-plane-reload
branch
from
October 28, 2026 19:01
016855f to
09025dd
Compare
Contributor
Author
|
Rebased on @MarcZX10 on the ordering case: Cost: I timed the added Rebased on AI-assisted: yes |
12 tasks done
adriadam10
approved these changes
Oct 28, 2026
Contributor
❌ NVIDIA GPU test: failedPR head
Failing tests
agc_driver_depth_plane_sampling: Failed (also failed when re-run alone), log excerptagc_driver_depth_plane_sampling_evicting: Failed (also failed when re-run alone), log excerptIntermittent validation messages (5, seen once in the parallel run, not reproduced when the test was re-run alone 2 times; not counted against this PR)Failing on main too (40, not counted against this PR)Passed on retry (3, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validation IDs already present on main (5 IDs in 690 tests)Skipped (61)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. Edited in place on each new push. Full logs are kept on the test machine. |
Contributor
|
@TechLevelZero This is useful additional validation: pinned revisions and hardware, a main comparison, isolated retries, explicit skips, and updating one comment all help reviewers. One important qualification: the PR adds A few changes would make the automation more actionable:
For merge throughput, focused baseline/candidate runs with identical test coverage are the highest-value first step; broad runs can supplement them. This report also does not replace the performance evidence requested in this PR, which the author has since supplied. The useful outcome is a reproducible finding an author can act on, rather than an automatic merge verdict. I reviewed the report, diff and discussion; I did not independently execute these tests. AI-assisted: yes. |
Contributor
Author
|
@TechLevelZero thanks for the run. I reproduced it: with Your container has no write watch: 78c1cb4 makes I ran
AI-assisted: yes |
12 tasks done
MarcZX10
suggested changes
Oct 29, 2026
MarcZX10
left a comment
MarcZX10
left a comment
Contributor
There was a problem hiding this comment.
Reviewed — changes requested
The new write-watch test and its no-write-watch skip address the reported test-environment issue, and the reported GPU run includes a main-code negative control; before this branch is ready, update it against current main and resolve its declared ordering with #12210.
What I ran
- Inspected PR head
78c1cb46e4017ab6865fa09df261148882ecfc38, its diff, currentmain(3a2b91981651569163768854a1fcdc7193b6667b), and the repository branch and AGC testing rules. - Checked the current GitHub status checks and ran
git diff --checkagainst the merge base. - Compared the branch with current
mainusing Git's merge-tree preview. No conflicted merge was reported, but the branch is 233 commits behind and both branches edit the AGC debt file.
Results
| Check | Result |
|---|---|
| Windows CI | Raw statuswindows pass |
| Conventions CI | Raw statusconventions pass |
| Other hosted jobs | Raw statuslinux skipping; register-context skipping; relinker skipping; relinker-asan skipping; write-watch skipping |
| Local whitespace check | Raw outputgit diff --check upstream/main...refs/review/pr-12238 — no output; exit status 0 |
| Merge preview | Raw outputmerge-tree-exit=0; preview has edits from both branches in docs/dev/debt/libSceAgcDriver.md |
Negative control
The author reports that agc_driver_depth_plane_sampling and _evicting passed with validation enabled on the candidate, and that the added sampling case failed with DepthSurface.cpp from main (0x3e800000, expected 0x3f400000). I have not independently run those GPU tests in this review.
Findings
Blocking
- The branch is 233 commits behind current
main.CONTRIBUTING.mdrequires branches to be kept up to date. Please update it and rerun the focused AGC tests and checks on the resulting head. - The checklist says
Depends on: none, but #12210 is still open and both PRs editdocs/dev/debt/libSceAgcDriver.md. Your comment says the combined result requires one branch's debt wording to be retained and “sample” added. The contribution rules require agreement on ordering and a matchingDepends on: #12210(or the agreed reverse order). Please coordinate and declare the agreed order.
Non-blocking
- A reviewer asked for a cost measurement because this adds a write collection/generation check on sampled depth planes. The latest test report does not include a profile or frame-time comparison. Please add one, or explain with measured evidence why the added work is negligible.
Sources
- PR diff and checks reviewed at
78c1cb46e4017ab6865fa09df261148882ecfc38. - Current base:
mainat3a2b91981651569163768854a1fcdc7193b6667b. - Contribution rules:
CONTRIBUTING.mdat3a2b91981651569163768854a1fcdc7193b6667b.
Not verified
I did not run a local build, focused GPU tests, or console tests. Linux and specialized hosted checks were skipped. The reported GPU results are author-provided and are not a substitute for rerunning them after updating this branch.
To get this to a good state
Agree with #12210 on merge order, set the corresponding dependency on the checklist, update this branch to current main, then rerun and report the required focused tests and validation checks on the new head. Add the requested cost measurement if available.
XairRrR
force-pushed
the
fix/agc-sampled-depth-plane-reload
branch
from
October 29, 2026 00:51
78c1cb4 to
29a96bf
Compare
Contributor
Author
|
@MarcZX10 thanks. Rebased on I ran the focused tests with the validation layer and
The description has the updated test list. Order with #12210: both PRs are mine, and #12210 goes first. The checklist now says Cost: I measured it in game, in my comment above, and it is now in the description too. The added call runs 6 to 17 thousand times per 10 s at about 1.1 us each, 0.2 to 0.4 ms of a ~260 ms frame. AI-assisted: yes |
MarcZX10
suggested changes
Oct 29, 2026
MarcZX10
left a comment
MarcZX10
left a comment
Contributor
There was a problem hiding this comment.
Re-checked — changes requested
The new sampled-depth reload and write-watch-gated regression address the previous test-scope concern, but this branch is still 36 commits behind current main 52c09d567d2fa453494dd720fcf53a98149468e4. The repository checklist requires a current-main base, and the reported tests are not on the refreshed integration.
What I ran
- Reviewed exact head
29a96bfc5d1fa648096d68d6a2e4535d20b23bbcagainst currentmain. - Ran
git merge-tree --write-treeandgit diff --check. - Inspected the production/test diff and checked the author’s reports.
Results
| Check | Result |
|---|---|
| Current-main merge preview | Clean; current main is not in the branch history |
git diff --check |
Passed |
| Focused GPU tests | Existing reports are against the earlier 3a2b919816 base |
| Current-main validation run | Not reported |
Please update onto current main and rerun the focused sampled-depth tests and required validation-layer comparison on that head. I did not run the C++ or GPU tests locally.
Contributor
❌ 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_plane_sampling_evicting: new validation messagesagc_driver_depth_plane_sampling: new validation messagesUnstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (5, not counted against this PR)Validation IDs already present on main (4 IDs in 405 tests)Skipped (39)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
|
@TechLevelZero This is useful: the GPU/driver details, exact revisions, baseline comparison, isolated retries, and failure excerpts give the author something concrete to investigate and can save reviewers time. One wording suggestion: since these failures are in a test added by this PR, “2 new failures” does not by itself establish a regression in existing behavior; “2 PR-only test failures” would be clearer. Linking full logs or artifacts, showing the validation-layer version, and distinguishing environment-dependent skips would also help reproduce the result. A single updated comment per head, as you are doing, should keep the signal high. The remaining question here is whether the failed test reflects a real issue on this GPU or a limitation of the test environment. |
…ace was bound A depth plane with the 64 KiB Z swizzle reloads the units the guest wrote (SyncFromMemory) when its surface is bound as a depth target, but sampling the plane through the surface skipped that check and read the host image. A title that copies depth into the plane with a buffer copy and samples it before drawing into it again read the depth from before the copy. PPSA05684 copies its depth pre-pass into a second depth target with a buffer copy kernel and samples that copy at once to build the half-resolution depth its volumetric fog and dust read; only later does it bind the target. The fog used the previous frame's depth, so a silhouette followed every moving character through fog and dust. Sampling a single-sample plane through its surface now runs the same reload first. agc_driver_depth_plane_sampling gains a case that draws a plane, writes the guest memory under it and samples it; it reads the drawn depth without the change. AI-assisted: yes
…write-watched Without a write watch the driver does not see the guest's CPU write to the depth plane, so GuestWriteAfterDrawCase reports that instead of failing, as the CPU-write cases of agc_driver_depth_plane_partial_store do. AI-assisted: yes
adriadam10
force-pushed
the
fix/agc-sampled-depth-plane-reload
branch
from
October 29, 2026 20:17
29a96bf to
83e01b8
Compare
Collaborator
|
Rebased onto current |
12 tasks done
Contributor
|
Since this PR, Astro Bot (PPSA21564, 01.007) renders Aerial Garden without its cloud sea and haze: below the ship there is a teal surface where there used to be white cloud (about 2 % of the bottom third of the in-level frames is cloud, against about 80 % before). Crash Site looks unchanged. Bisect. Frame dumps over the 76 AGC/shader merges between Cause, confirmed with a build that only tags the stamp sites (no behaviour change):
So the reload is right for a real guest write after the draw (your PPSA05684 case), but it orders the driver's own stores by when they are stamped rather than by when their results were recorded. Possible fix, for every title:
A test for this case: a dispatch writes an image over the plane's memory and its results stay pending; the plane is bound and drawn; the image is stored; the plane is sampled and must return the drawn depth. I have a patch along these lines in progress and can open it as a PR once it is tested, if this direction works for you. |
12 tasks done
Contributor
|
Follow-up to my comment above: the fix is now #15621 ("fix(agc): order driver stores over a depth plane by record order").
I have only run |
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
A depth plane with the 64 KiB Z swizzle reloads the units the guest wrote (
SyncFromMemory, #7154) when its surface is bound as a depth target. Sampling the plane through the surface (DepthSurfaceTexture, the single-sampleSampledpath) skipped that check and read the host image. A title that writes the plane with a buffer copy and samples it before it draws into it again therefore read the depth from before the copy.PPSA05684 (Uncharted: Legacy of Thieves Collection) does this every frame in Uncharted 4: it copies its depth pre-pass into a second depth target with a
buffer_load_format_x/buffer_store_format_xkernel, samples that copy right away to build the half-resolution depth its volumetric fog and dust read, and only later binds the target. The fog used the previous frame's depth, so at the frame rates reached here a dark or light silhouette followed every moving character through fog and dust (the orphanage's closing scene and the prison yard).Sampling a single-sample plane through its surface now runs the same reload first, so the sampled depth includes the guest's writes collected by then. The debt entry for depth planes in guest memory lists sampling among the points where guest writes are collected.
Tested
Arch Linux 7.2.11-zen, GCC 16.2.1, Ninja, Release, on
main3a2b919, NVIDIA GeForce RTX 4070 Ti SUPER (615.78.08):agc_driver_depth_plane_samplinggainsGuestWriteAfterDrawCase. It draws depth 0.25 into a 128x128 D32 plane with the 64 KiB Z swizzle and samples it (0.25), then writes 0.75 into the guest memory under it and samples it again without binding the surface. WithDepthSurface.cppfrommainit fails:D32 plane the guest wrote after a draw, sampled before its surface is bound again: texel 0 read 0x3e800000, expected 0x3f400000.DrawDepthtakes an optional depth swizzle for it.APS5_NO_WRITE_WATCH=1, or a host without a write watch), the driver cannot see the guest's write. The case then printsguest memory has no write watch: the guest write after a draw is not checkedand returns, like the CPU-write cases ofagc_driver_depth_plane_partial_store.VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 VK_LOADER_DEBUG=error,warn,layer ctest -V -R '^agc_driver_depth_plane_sampling'onmain3a2b919 and on this branch: 2/2 pass on both, and the layer is inserted in every test. No log reportswas not found but was requested by env var VK_INSTANCE_LAYERS, and neither log has aVUID-line. WithAPS5_NO_WRITE_WATCH=1, the branch also passes 2/2 with noVUID-line.ctest -j8on the first base (main3ca715e): 2620 of 2621 pass.agc_driver_mesh_barycentric_emulationfails the same way onmainon this GPU (the pixel shader did not emulate barycentrics).tools/check_conventions.py --base origin/mainpasses.SyncFromMemorycall in the prison yard fight of PPSA05684, where the fog and dust sample the copied depth plane every frame. It ran 6 to 17 thousand times per 10 s at about 1.1 us each, 7 to 16 ms per 10 s in total, so 0.2 to 0.4 ms of a ~260 ms frame.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)