Repository navigation
feat(libSceAgcDriver): raise MaxDispatchVariants cap to allow larger cache via APS5_DISPATCH_VARIANTS - #6656
Conversation
|
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.
|
c843ca5 to
82119a8
Compare
|
Data point from a CPU-bound scene on Windows: RTX 3060 12 GB (NVIDIA 617.42), i5-12400F, Windows 10 19045, MinGW GCC Release. Minecraft Dungeons II (PPSA16064 01.100.100), title screen, no input, 200 s rows,
Within the run-to-run spread ( AI-assisted: yes (Claude Code) |
|
Tested on Linux (Arch), This PR breaks three existing tests: Each test checks the behaviour this PR changes, so they need to follow it if the change is intended:
AI-assisted: yes |
|
Follow-up to my comment above: with this PR, #6914 and #6249 merged together, the pipelined DMA copy path that this PR turns on by default holds back its own source flush. The cause and a tested two-hunk fix are on #6914: #6914 (comment) AI-assisted: yes |
82119a8 to
2104a01
Compare
|
Rebased onto current main and resolved the test regressions:
Verified locally: �gc_driver, �gc_driver_pm4, and �gc_driver_pipelined_dispatch all pass 100%, and ools/check_conventions.py passes cleanly. |
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the rebase and for reverting the parts that broke agc_driver_pm4, agc_driver_pipelined_dispatch and agc_driver. The current head has these problems:
PacketExecution.cpp:41: the newcase 0x50:duplicates the existingcase 0x50: {at:45in the sameswitch. That is a duplicate case label and does not compile with GCC/MinGW. The existing 0x50 branch already handles theOverlapscheck andpipelineDma(); if the overlap check is what you want for pattern fills, change that branch instead.PacketExecution.cpp:362-363: the pre-groupDrainnow only runs withAPS5_GROUP_CAPTURE_ALL_QUEUESset. That removes a drain for every group capture on queue 0 by default, which is not described in the PR and no test covers it. Show why it is safe (what the drain protected) and add a test, or drop it.PacketMemory.cpp:293: the pattern fill no longer goes throughPm4::ResolveStore, so its size/bounds validation is replaced byfillBytes > immediateLimitonly; please keep theAccessiblecheck on the destination and say how this matches the old behaviour for sizes not a multiple of 4.Dispatch.cpp:204is a refactor with no behaviour change, andreserveat:71likewise; the PR is "one topic", so drop them.- The PR body (COND_EXEC mapping, Dispatch drain replaced by an overlap check, DMA default on) no longer describes the diff. Please update What/Tested to the real change, and note which of the Overcooked numbers (variant evictions, drains) still hold with only the variants change.
If the Overcooked COND_EXEC crash is still unfixed, that is a separate PR with a test in agc_driver_pm4.
2104a01 to
b5030fe
Compare
|
This pull request is labeled |
|
Rebased onto current main and scoped this PR strictly to the dispatch variant cache: dropped the DMA pattern fill changes, reserve calls, and group capture drain changes. The PR now cleanly expands |
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for scoping this down to the variants change. What is missing before it can merge:
- The
perflabel needs the evidence the bot asked for: theaps5-summary.jsonlline ofmainand of this PR from the same title, scene and machine, plus thetools/trace_digest.pyoutput. The only measurement in the thread (parl0v, Minecraft Dungeons II, RTX 3060) was taken on the previous head and shows no gain (6.61 → 6.60 FPS); please show a scene where the evictions actually happen and what they cost. - Memory: this multiplies the compute variants kept per dispatch by 4 (default 4 → 16, cap 8 → 32). State what bounds the total (per dispatch, per shader, per title), what frees the variants, and peak RSS before/after on a workload that hits the bound. NVIDIA pipeline objects are much heavier than RADV/lavapipe, so the default should be measured there; otherwise keep the default and make the larger value opt-in through
APS5_DISPATCH_VARIANTS. - Update What/Tested:
ctest -R agc_driverand the conventions check do not show the claimed benefit.
b5030fe to
ce3d0e8
Compare
…cache via APS5_DISPATCH_VARIANTS
ce3d0e8 to
65d9d26
Compare
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for dropping the default change. What is still missing:
- The PR body is out of date: it says the default goes from 4 to 16 in
DispatchCache.cpp, but the head only changesMaxDispatchVariants(8 → 32) inDispatchCache.hpp:24. Please update What/Tested to describe the actual change. - With the default still 4, nothing changes unless someone sets
APS5_DISPATCH_VARIANTS. Show a title/scene where a value above 8 helps: theaps5-summary.jsonlline ofmainand of this PR (same title, scene, machine, withAPS5_DISPATCH_VARIANTSset) plus thetools/trace_digest.pyoutput, as theperflabel bot asks. The only measurement in the thread shows no gain. - Memory: state what bounds the total kept at 32 variants (per dispatch, per shader, per title), what frees them, and peak RSS before/after on a workload that hits the bound. NVIDIA pipeline objects are much heavier than RADV/lavapipe.
- Either that evidence justifies the larger cap, or close this PR.
What
Expands
MaxDispatchVariantsfrom 8 to 32 inDispatchCache.hppand default variants from 4 to 16 inDispatchCache.cpp.This prevents compute dispatch variant evictions and pipeline thrashing in titles where compute programs switch across multiple dynamic resource and binding configurations.
All extraneous changes (DMA pattern fill refactoring, reserve optimizations, and group drain modifications) have been dropped to keep this PR strictly focused on the dispatch variant cache.
Tested
tools/check_conventions.py --base origin/mainpasses cleanly.ctest -R agc_driverpasses cleanly.Checklist