Repository navigation
fix(agc): decode a fast-launch draw as one point per subgroup whatever its input type - #8395
Conversation
|
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.
|
|
On the two pull requests with conflicting lines, neither implements this change nor depends on it:
|
|
NVIDIA and AMD proprietary data point: RTX 3080 (NVIDIA 617.42) and AMD Radeon RDNA 2 iGPU ( AI-assisted: yes (Claude Code). |
|
@TheSovietWaffle Thanks, that is the first run of this change on NVIDIA and on AMD's own driver; I only had RADV. I added it to the Tested section. |
|
Merged current |
The agc_driver case that resolves the registered fast-launch program with a triangle strip passed with main's State.cpp too: the program is already deferred at registration, so the resolve defers the stage it cannot decode without logging anything. Resolve the strip first and require the point list to find the artifact the strip prepared (the same prepared entry hashes). With main's State.cpp the strip prepares nothing and the point list then prepares the program, so the case fails. A device that prepares the program for neither type has nothing to compare.
|
Added f4a2740, test only: the |
XairRrR
left a comment
There was a problem hiding this comment.
Reviewed — approved
The change decodes a GS_FAST_LAUNCH = 1 draw with a one-vertex, one-primitive GE_CNTL group as one point per subgroup whatever VGT_PRIMITIVE_TYPE holds. It merges cleanly, builds, and the focused AGC tests pass with the Khronos validation layer and no VUID- lines on either side. The negative control fails both new checks on an NVIDIA GPU as well as on the author's RADV.
What I ran
- PR head:
f29635063ca9e80488a1f6f18b1e4d12147e3742 main:899ae3d1ad32ccea94dc24e66fdc948ce8a88171- Merged tree (
main+ head,--no-ff):d99ad97ab97e7d05f3625c86c3646bacc1e270fa, no conflicts - Machine: Arch Linux 7.2.11-zen, GCC 16.2.1, Release, Intel Core i9-14900KF, NVIDIA GeForce RTX 4070 Ti SUPER (driver 615.78.08), in a clean worktree
git fetch origin main pull/8395/head:refs/review/8395
git checkout --detach origin/main && git merge --no-ff --no-edit refs/review/8395
git diff --check origin/main...refs/review/8395
python3 tools/check_conventions.py --base origin/main
nice -n 10 cmake --build build -j10 --target agc_driver_tests agc_driver_graphics_tests agc_driver_mesh_tests
nice -n 10 cmake --build build -j10 --target libs agc_driver_tests agc_driver_graphics_tests agc_driver_mesh_tests
VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 VK_LOADER_DEBUG=error,warn,layer \
nice -n 10 ctest --test-dir build -R '^(agc_driver|agc_driver_graphics|agc_driver_mesh)$' -V --output-on-failure -j4
# the same build and ctest command on origin/main (899ae3d1ad) in the same worktree
# negative control: git checkout origin/main -- core/libs/prx/libSceAgcDriver/Graphics/src/State.cpp, rebuild, the same ctest commandResults
| Check | Result |
|---|---|
Merge with main |
Clean |
git diff --check |
Clean |
tools/check_conventions.py --base origin/main |
All conventions checks passed. |
Build (libs and the three test targets) |
OK |
agc_driver, agc_driver_graphics, agc_driver_mesh on the merged tree, with validation |
3 of 3 pass |
The same on main |
3 of 3 pass |
Insert instance layer "VK_LAYER_KHRONOS_validation" |
3 on main, 3 on the merged tree |
was not found but was requested by env var VK_INSTANCE_LAYERS |
0 on both |
VUID- lines |
0 on both |
| Test durations | 1.6 s, 4.7 s and 6.1 s; all under 30 s |
| Negative control | agc_driver and agc_driver_graphics fail, agc_driver_mesh passes |
Merged tree
959: Mesh tests passed
1/3 Test #959: agc_driver_mesh .................. Passed 1.58 sec
934: AGC driver submit tests passed
2/3 Test #934: agc_driver ....................... Passed 4.67 sec
957: Graphics validation tests passed
3/3 Test #957: agc_driver_graphics .............. Passed 6.14 sec
100% tests passed out of 3
main
1/3 Test #959: agc_driver_mesh .................. Passed 1.75 sec
2/3 Test #934: agc_driver ....................... Passed 4.66 sec
3/3 Test #957: agc_driver_graphics .............. Passed 5.52 sec
100% tests passed out of 3
Negative control
I restored only core/libs/prx/libSceAgcDriver/Graphics/src/State.cpp from main, kept the PR's tests, rebuilt and ran the same command. Both tests that hold the new checks fail:
Output
957: AGC graphics: unexpected failure: AGC graphics: VGT_SHADER_STAGES_EN=0x482030: fast launch other than one point per subgroup is unsupported
1/3 Test #957: agc_driver_graphics ..............***Failed 0.17 sec
934: a registered fast-launch program resolved with a triangle strip was not prepared as for a point list
2/3 Test #934: agc_driver .......................***Failed 0.82 sec
3/3 Test #959: agc_driver_mesh .................. Passed 1.54 sec
33% tests passed, 2 tests failed out of 3
This confirms the description's negative control on a second vendor. The PreparedEntryHashes comparison in agc_driver also detects the change on NVIDIA, so on this device it is not just two empty sets compared with each other.
Findings
Blocking
None.
Non-blocking
state.topologystill followsVGT_PRIMITIVE_TYPEfor a fast-launch draw, so the provoking-vertex checks can refuse the draw with a misleading message.DecodeShaderStagesnow gives the mesh a point list (State.cpp:1332).DecodeStatestill setstopologyfrom the raw type, so types 5 and 21 giveTRIANGLE_FAN(State.cpp:1512-1515). With last provoking vertex observed, a fast-launch draw of type 5 or 21 therefore throwslast provoking vertex is unsupported for triangle fans assembled for a geometry program(Pipeline.cpp:234-236), although this path assembles no fan. A strip type hits the same problem through the per-vertex triangle-strip interpolation check (Pipeline.cpp:242). Both checks throw instead of drawing anything wrong, so this is not a correctness bug. Still, once a title reaches them they will point at the wrong cause. A follow-up could key the fan check onstate.stages.mesh->inputPrimitiverather thanstate.topology.- Type 21 is accepted but not tested. The decode test loops over types 2 to 6 (tests/Graphics.cpp:1187).
DecodeStatealso accepts type 21 on this path (State.cpp:1513), and now decodes it as a point list. Adding21uto the list would cover every type that the PR newly accepts on the fast-launch path. Types 7, 17 and 19 still throw because they require vertex routing.
Not verified
- Whether the console starts one subgroup per vertex for a line or triangle
VGT_PRIMITIVE_TYPEunder fast launch. The PR states this as an assumption and records it in the debt entry. I did not check the RADV sources that the description cites. - The CONTROL Resonant (PPSA34547) runs. I launched no title.
- AMD and Windows runs. I ran on NVIDIA under Linux only.
|
@XairRrR thanks for the review and for the NVIDIA run.
|
XairRrR
left a comment
There was a problem hiding this comment.
Re-checked — approved
The only new commit, c65b4ef, adds VGT_PRIMITIVE_TYPE 21 to the fast-launch decode test, as my previous review suggested. The merge with current main is clean, the focused AGC tests pass with the Khronos validation layer and log no VUID- lines on either side, and the new type 21 case fails on its own against main's State.cpp.
What changed since my last review
- One commit on top of f296350: c65b4ef, test only. It changes
{2u, 3u, 4u, 5u, 6u}to{2u, 3u, 4u, 5u, 6u, 21u}in the decode loop (tests/Graphics.cpp:1187). Production code is the same as in the commit I approved. - The author replied to both findings, and the Tested section now mentions c65b4ef.
mainmoved from 899ae3d to 478b5f1.
What I ran
- PR head:
c65b4eff5f04f9587d9d220b8e87a654edff093c main:478b5f1a93c9bb1cb79e7578f4438a42d7381353- Merged tree (
main+ head,--no-ff):a69d18318712b1c2f42a9f7e5d9fa83b283874a4, no conflicts - Machine: Arch Linux 7.2.11-zen, GCC 16.2.1, Release, Intel Core i9-14900KF, NVIDIA GeForce RTX 4070 Ti SUPER (driver 615.78.08), in a clean worktree
git fetch origin main pull/8395/head:refs/review/8395
git log --oneline f29635063c..refs/review/8395 && git diff f29635063c refs/review/8395
git checkout --detach origin/main && git merge --no-ff --no-edit refs/review/8395
git diff --check origin/main...refs/review/8395
python3 tools/check_conventions.py --base origin/main
nice -n 10 cmake --build build -j10 --target libs agc_driver_tests agc_driver_graphics_tests agc_driver_mesh_tests
VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 VK_LOADER_DEBUG=error,warn,layer \
nice -n 10 ctest --test-dir build -R '^(agc_driver|agc_driver_graphics|agc_driver_mesh)$' -V --output-on-failure -j4
# the same build and ctest command on origin/main (478b5f1a93) in the same worktree
# negative control 1: merged tree, git checkout origin/main -- core/libs/prx/libSceAgcDriver/Graphics/src/State.cpp, rebuild, the same ctest command
# negative control 2: as 1, with the decode loop narrowed to {21u}, then rebuild agc_driver_graphics_tests and run ctest -R '^agc_driver_graphics$'| Check | Result |
|---|---|
Merge with main |
Clean |
git diff --check |
Clean |
tools/check_conventions.py --base origin/main |
All conventions checks passed. |
Build (libs and the three test targets), merged tree and main |
OK |
agc_driver, agc_driver_graphics, agc_driver_mesh, merged tree, with validation |
3 of 3 pass (4.8 s, 6.3 s, 1.4 s) |
The same on main |
3 of 3 pass (4.8 s, 6.0 s, 1.6 s) |
Insert instance layer "VK_LAYER_KHRONOS_validation" |
3 on main, 3 on the merged tree |
was not found but was requested by env var VK_INSTANCE_LAYERS |
0 on both |
VUID- lines |
0 on both |
Negative control 1 (main's State.cpp) |
agc_driver and agc_driver_graphics fail, agc_driver_mesh passes |
Negative control 2 (type 21 only, main's State.cpp) |
agc_driver_graphics fails |
Merged tree
960: Mesh tests passed
1/3 Test #960: agc_driver_mesh .................. Passed 1.42 sec
935: AGC driver submit tests passed
2/3 Test #935: agc_driver ....................... Passed 4.81 sec
958: Graphics validation tests passed
3/3 Test #958: agc_driver_graphics .............. Passed 6.34 sec
100% tests passed out of 3
main
1/3 Test #960: agc_driver_mesh .................. Passed 1.58 sec
2/3 Test #935: agc_driver ....................... Passed 4.79 sec
3/3 Test #958: agc_driver_graphics .............. Passed 6.04 sec
100% tests passed out of 3
Negative controls
# 1: main's State.cpp, PR tests
958: AGC graphics: unexpected failure: AGC graphics: VGT_SHADER_STAGES_EN=0x482030: fast launch other than one point per subgroup is unsupported
1/3 Test #958: agc_driver_graphics ..............***Failed 0.18 sec
935: a registered fast-launch program resolved with a triangle strip was not prepared as for a point list
2/3 Test #935: agc_driver .......................***Failed 0.89 sec
3/3 Test #960: agc_driver_mesh .................. Passed 1.42 sec
33% tests passed, 2 tests failed out of 3
# 2: main's State.cpp, decode loop over {21u} only
1/1 Test #958: agc_driver_graphics ..............***Failed 0.26 sec
AGC graphics: unexpected failure: AGC graphics: VGT_SHADER_STAGES_EN=0x482030: fast launch other than one point per subgroup is unsupported
The full-loop control fails on type 2 before it reaches 21. So I also ran the loop with type 21 alone, which confirms that the new case covers behaviour of its own and does not just pass because the earlier types pass.
Previous findings
state.topologystill followsVGT_PRIMITIVE_TYPEfor fast launch, so the provoking-vertex and strip-interpolation checks can give the wrong cause. Still open, non-blocking. c65b4ef does not touch it (Pipeline.cpp:236 on currentmainstill checksstate.topology). The author agrees and plans a follow-up with a test for each check. They also say that neither message appears in their 17 CONTROL Resonant logs (author-reported). Both checks throw rather than draw anything wrong, so a follow-up is reasonable, and I am not holding this PR for it.- Type 21 accepted but untested. Resolved by c65b4ef (tests/Graphics.cpp:1187). It passes on the merged tree and fails alone against
main'sState.cpp(negative control 2).
New findings
Blocking
None.
Non-blocking
None.
Not verified
- Whether the console starts one subgroup per vertex for a line or triangle
VGT_PRIMITIVE_TYPEunder fast launch. This is unchanged from my last review and is recorded in the debt entry as an assumption. - The CONTROL Resonant (PPSA34547) runs and the author's RADV run of c65b4ef. Both are author-reported; I launched no title.
- AMD and Windows runs. I ran on NVIDIA under Linux only.
✅ 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 (5, 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)Validation IDs already present on main (4 IDs in 397 tests)Skipped (40)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. |
What
DecodeStateacceptsVGT_SHADER_STAGES_EN.GS_FAST_LAUNCH = 1only with a point list inVGT_PRIMITIVE_TYPE(#4702). A fast-launch draw with any other type and the sameGE_CNTLgroup of one vertex and one primitive stops the title withfast launch other than one point per subgroup is unsupported.This PR decodes such a draw as one point per subgroup whatever
VGT_PRIMITIVE_TYPEholds:radv_shader_info.c;ac_nir_lower_ngg_mesh.c: "the vertex ID which is equivalent to the index of the current workgroup within the current dispatch").GE_CNTLasks for one vertex per subgroup, which no line or triangle can fill, so the type cannot be what sizes the subgroup.Other group sizes, fast launch 2 and indexed fast-launch draws still throw; the message for other group sizes now reads
fast launch other than one vertex and one primitive per subgroup is unsupported. That a console starts one subgroup per vertex for a line or triangle type is assumed, not measured, and the technical debt entry says so.Title
CONTROL Resonant (PPSA34547) makes fast-launch draws with
VGT_SHADER_STAGES_EN0x482030,VGT_PRIMITIVE_TYPE6 (triangle strip) andGE_CNTL0x201 right after New Game, as @ps54all reported in #7302; they got past it with a local change that treats the input as a point list.On
main06d97be with #7064 (needed to reach New Game), Linux, RX 9070 XT with RADV, in the same run directory with onlylibSceAgcDriver.prxswapped:Shader 0x10041aa500 could not be prepared when the title created its pipeline (AGC graphics: VGT_SHADER_STAGES_EN=0x482030: fast launch other than one point per subgroup is unsupported);APS5_TRACE_DRAW_PROGRAMSshows 6 of those 8 programs drawn on the mesh path (0x10041aa50060 times). I stopped the run there.Further on, the title loses the GPU after New Game with or without this PR: the run without it lost the device 2 min after New Game, before any of those programs was drawn. That loss is tracked in #7302.
Tested
main5454cd2 merged in (f296350), by a merge commit; history is not rewritten.State.cppconflicted becausemainmoved the geometry subgroup decode into ameshConfiguration(inputPrimitive, fastLaunch)helper that the tessellation-geometry path also uses. The resolution keeps that helper and makes the same change as before: the fast-launch check no longer requires a point list, and the geometry path passes a point list as the input primitive for a fast-launch draw (meshConfiguration(fastLaunch ? 1u : primitive, fastLaunch)). The tests and the debt entry merged without conflicts. Againstmain, the diff is still 4 files, +16/-6.Paired validation on one machine and GPU: AMD Radeon RX 9070 XT (RADV GFX1201, Mesa 26.2.6), Arch Linux with kernel 7.2.11, GCC 16.2.1, Release.
ctest -V -R '^(agc_driver|agc_driver_graphics|agc_driver_mesh)$'withVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation(Khronos layer 1.4.363),ANYPS5_NO_SHADER_CACHE=1,ANYPS5_REQUIRE_VULKAN=1andVK_LOADER_DEBUG=layer:Insert instance layer "VK_LAYER_KHRONOS_validation"was not found but was requestedVUID-linesmain5454cd2In each run
agc_driverinserts the layer twice andagc_driver_meshonce;agc_driver_graphics, which holds the decode test, creates no Vulkan instance.Negative control,
main5454cd2 with only this PR's test changes, same settings: both tests that hold the new checks fail.agc_driver_graphicsfails withAGC graphics: unexpected failure: AGC graphics: VGT_SHADER_STAGES_EN=0x482030: fast launch other than one point per subgroup is unsupported, andagc_driverwitha registered fast-launch program resolved with a triangle strip was not prepared as for a point list.agc_driver_meshpasses.c65b4ef, on top of that head, only adds type 21 (the polygon type, which
DecodeStatealso accepts on this path) to the decode test: the three tests pass on it with the layer inserted and noVUID-lines.agc_driver_graphics: with fast launch andGE_CNTL0x201,VGT_PRIMITIVE_TYPE2 to 6 and 21 decode as the point list does (one point per subgroup,PRIM_AMP_FACTORprimitives);GE_CNTL0x402 is still refused.agc_driver: the registered fast-launch program from fix(agc): defer unrouted vertex barriers #6150 is resolved with a triangle strip before the point list, and the point list has to reuse the artifact the strip prepared (Driver::PreparedEntryHashesis the same after both resolves). On the RX 9070 XT the strip prepares one entry, with the same hash as the entrymainprepares for the point list. Withmain'sState.cppthe strip's decode fails and the stage stays deferred (without a log line, since it is already deferred at registration), so the point list prepares the program itself and the check fails. A device that cannot prepare the program for either type has nothing to compare; the decode itself is covered byagc_driver_graphics, which needs no GPU.The fast-launch mesh draws of
agc_driver_meshpass unchanged: the draw side still receives a point list.Earlier revisions of the same change, not repeated on this base:
-j8,ANYPS5_REQUIRE_VULKAN=1andANYPS5_NO_SHADER_CACHE=1on the same machine, onmain74acfad:mainand fb4caca (the head before its last, test-only commit) both pass 2396 of 2431 tests. On both, the same 34 tests are skipped andagc_driver_tessellationsegfaults (agc_driver_tessellation segfaults on RADV with Mesa 26.2+: nir_opt_varyings corrupts the TCS input offset's use list (Mesa bug) #5444).BUILD_TESTING=ON, onmain06d97be. The VM has no GPU, so the GPU tests do not run there: of 1704 tests, 1077 pass, 627 are skipped, nearly all of them GPU tests (agc_driverandagc_driver_meshamong them), and none fails.agc_driver_graphics, which holds the decode test above, runs and passes there.75720e0de, run by @TheSovietWaffle: on an RTX 3080 (NVIDIA 617.42) and an AMD Radeon RDNA 2 iGPU (1002:164e, 32.0.21045.5002),agc_driver_graphics,agc_driver,agc_driver_meshandagc_driver_tessellationpass 3 of 3 on both GPUs (that Radeon driver reports no mesh shaders).main06d97be with feat(libSceAcm): track sceAcmBatchJobPriority gap and test throw #7064.tools/check_conventions.py --base origin/main --head f29635063cwith this title and description passes, andgit diff --checkis clean.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)