Repository navigation
docs(agc): record that AMD's Windows driver and NVIDIA keep interlock ordering with a fast link - #11449
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
The documentation update is carefully scoped to the measured Radeon 860M and RTX 5060 Ti, and the current hosted eligibility, conventions, intake and labels checks pass. The reported branch/test bases are 035ac96edd and 9656d366be, while current main is 51f5c57dd9; please sync to current main and rerun conventions before final review, as required by CONTRIBUTING. The PR overlap check reports no conflict with #11453’s separate entry.
fc00517 to
d1dea5b
Compare
MarcZX10
left a comment
There was a problem hiding this comment.
Re-reviewed rebased head d1dea5b. The rebase keeps the change to the single interlock-debt entry; the merge tree is clean against current main 32b9227. Current-main conventions and diff checks pass. The driver runs and counts are author-reported, not independently reproduced here; hosted eligibility, conventions and labels pass.
|
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.
|
THEMANnt
left a comment
There was a problem hiding this comment.
Reviewed d1dea5bff6ef78f65daf3ea8e9121cc5829155ea against author merge base f616a1397f9b618036ac7c89f461f0bd46fbd6d6 — approve the documentation change.
The referenced pipeline code disables graphics-pipeline-library linking whenever the fragment stage declares an interlock capability, regardless of vendor. The new Windows-driver observations do not change that conservative behavior or claim Intel/MoltenVK coverage. The reported GPU run counts are author measurements, not runs I independently repeated.
Validation: inspected the Markdown diff and pinned source references; relative links resolve and git diff --check passed. This is documentation/source consistency review. Title disassembly, console snapshots and hardware measurements were not independently reproduced; no runtime regression result is claimed.
d1dea5b to
721a6c7
Compare
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — no blocking findings
The one-line debt-note change at head 721a6c7 is consistent with the file, but its benchmark counts cite a test name that is not in the tree, and the driver and hardware figures are author-reported, so I withhold approval.
What I ran
- Exact head:
721a6c753f9f453b8ee856e8a2c2b2e09465b2d5. Base:b8f0a6b2025605b78eb70646bda79f458a577c82, an ancestor of current mainac26e59d9a, 1071 commits behind it. Diff: one line indocs/dev/debt/libSceAgcDriver.md(+1/-1). - No build, no compile.
- Commands:
python3 -I tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 11449 --output-dir <scratch>/precheck/11449
git grep -c pixel_interlock 721a6c753f9f453b8ee856e8a2c2b2e09465b2d5 -- ":!docs" (run by hand, local object store)
git grep -n APS5_NO_PIPELINE_LTO 721a6c753f9f453b8ee856e8a2c2b2e09465b2d5 -- core/libs/prx/libSceAgcDriver/Graphics/src/PipelineLibrary.cpp
- Hosted checks at this head: eligibility, labels and conventions succeeded; the build jobs (write-watch, register-context, relinker, relinker-asan, windows, linux) were skipped for this docs-only change.
Results
| Check | Result | Notes |
|---|---|---|
git diff --check (base..head) |
passed | precheck |
| conventions and PR template | passed | precheck |
| live dependency and mergeability | passed | precheck |
| hosted eligibility, labels, conventions | success | GitHub check runs at this head |
test-name lookup pixel_interlock outside docs |
0 matches | run by hand; the only match is the debt note line |
env var APS5_NO_PIPELINE_LTO |
present | run by hand, PipelineLibrary.cpp:183 |
| test-quality heuristics | not applicable | no test file changed |
| build, negative controls | not applicable | docs-only change |
Unedited precheck output (conventions)
All conventions checks passed.
Negative control
Not applicable: no test or code changed.
Findings
Blocking: none.
Non-blocking:
- The line cites the test
agc_driver_pixel_interlockfor "108 of 120 loaded runs". At this head that name does not exist outside the debt note:git grep pixel_interlockoutsidedocs/returns nothing. A reader cannot rerun the 108/120 count from the tree. Please name the harness that produced it, or say that it is out-of-tree. - The RTX 4090, RTX 5060 Ti and Radeon 860M counts and driver versions are author-reported and were not reproduced here. This is why approval is withheld.
- The earlier CHANGES_REQUESTED (5653366206, at
fc0051758f) asked for a sync to current main. The head still sits onb8f0a6b2, 1071 commits behind main. The change is a single docs line that does not depend on newer main, so I do not treat the staleness as blocking.
Sources
- Debt note line with the test name and counts:
AnyPS5/docs/dev/debt/libSceAgcDriver.md
Line 166 in 721a6c7
- Env var read at head:
Not verified
- All benchmark counts and driver versions in the line (author-reported, not run here).
- The external Mesa issue linked in the neighbouring line.
- The Windows and Linux build jobs; they are skipped for this docs-only change.
To get this to a good state
- Name the harness or command behind the 108/120 and 120/120 counts, and where it lives (in-tree path, or an explicit out-of-tree note).
- Sync to current main if the maintainers still want it, as the earlier review asked.
- Hosted checks on the new head are already green for the non-build jobs; no code change is requested.
… ordering with a fast link The fragment shader interlock entry said drivers other than RADV and an RTX 4090 were not measured. With the GPL exclusion for interlock pipelines removed locally and every draw fast-linked (APS5_NO_PIPELINE_LTO=1), agc_driver_pixel_interlock kept its read-modify-writes in 120 of 120 runs, 12 at a time, on a Radeon 860M with AMD's Windows driver and on an RTX 5060 Ti; RADV lost them in 108 of 120. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks. On the two points:
The new driver counts are mine, as you note; the exact setup and per-GPU table are in the description. AI-assisted: yes (Claude Code) |
721a6c7 to
cead841
Compare
What
The "Fragment shader interlock pipelines" entry in
docs/dev/debt/libSceAgcDriver.mdrecords why a pipeline whose fragment shader declares an interlock capability is never built from graphics pipeline libraries (#9185): RADV's fast link ends the primitive-ordered section before the shader's stores land. It measured RADV and an RTX 4090 and said other drivers were not measured. This adds AMD's Windows driver and a second NVIDIA generation, measured the same way, and names the drivers still unmeasured. Documentation only.Measured locally with the exclusion lifted, as #9185 did:
Pipeline.cpp: theFragmentInterlock(shaders)condition that keeps these pipelines whole was skipped when a local environment variable was set.PixelInterlock.cpp: under that variable the test's "no pipeline was linked from libraries" check was inverted to require at least one library-linked pipeline, so every counted run really drew with a fast-linked pipeline. The counter showed 2 linked pipelines per run (the wave32 and wave64 variants).APS5_NO_PIPELINE_LTO=1, so every draw used the fast link, and 12 copies ofagc_driver_pixel_interlockat once, 10 rounds, as in fix(agc): build a pipeline with a fragment shader interlock whole, not from libraries #9185's loaded runs.So only RADV's fast link loses the ordering among the drivers measured. Whether the exclusion could be narrowed to RADV is a separate question: a fast link would only matter for the draw-time compile of interlock pipelines, which I haven't timed, and Intel's drivers and MoltenVK remain unmeasured. I've left the code unchanged.
Tested
main035ac96eddon the Radeon 860M and9656d366beon the RTX 5060 Ti (between them onlyStencilReplay.cppchanged in the shader recompiler, and nothing in the pipeline or interlock code), WinLibs GCC 15.2.0 r7, Release, Windows 11 10.0.26300 on both machines; the measurement patch was local and is not part of this PR.check_conventions.py --base origin/mainpasses (the entry stays under 1000 characters).pr_overlap.pyfinds no conflicting comparison; PRs that already conflict withmainand also editdocs/dev/debt/libSceAgcDriver.mdwere not compared.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)