Repository navigation
fix(agc): keep the gate run going past indirect dispatches whose arguments stay on the GPU - #5747
Conversation
89b3981 to
6232642
Compare
|
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.
|
CnRJay
left a comment
CnRJay
left a comment
There was a problem hiding this comment.
Thanks for the ping. Narrowing the stop to the paths that read the arguments on the CPU is the right idea. On a GPU-read dispatch it protects nothing, and stopping there brings back the late capture from #1787. Demon's Souls going from 3 of 5 stops to a full 1500 s run with 0 read an earlier dispatch's output is good evidence.
One gap: indirectArgumentsPath() covers only the three static reasons (gpuIndirect, thread dimensions, misalignment). VulkanDevice on main has four more CPU-read reasons, decided while the dispatch is being recorded:
cpuReason = 1:StorageTexture::FlushPendingover the 12 argument bytes;cpuReason = 8: the program uses workgroup memory;cpuReason = 2: a pending label orCopiedWriterOverlapsover the argument dwords;cpuReason = 3: no host import holds the arguments.
In each of these, GuestMemory::Read(arguments, …) reads the counts. My question is when that runs for a dispatch the look-ahead adopted. If an adopted dispatch is recorded in queue order, after the earlier dispatches of its run, those reasons are evaluated with the earlier writes in place and the narrowing is safe. If adopting it at the gate also settles its recipe, including cpuReason, then a dispatch that indirectArgumentsPath() calls GPU-read can still read its arguments on the CPU at gate time, before the earlier dispatch that writes them has run. That is exactly the case #2990 stops for. Reason 2 is the likely one: a writer from the same run whose staged region isn't registered yet in copiedWriters when the look-ahead runs.
Either answer is fine. If it's the first, a sentence in the description saying where the dynamic reasons are decided would let a maintainer check it quickly. If it's the second, the look-ahead could also stop when the arguments are covered by a write of the run and no import holds them yet. #5058's two extra checks would then go into indirectArgumentsPath() as you suggest. A test variant where the earlier dispatch writes the arguments through a misaligned or staged binding would pin it either way.
|
@CnRJay It's the first case. The look-ahead never reaches the device. While resolving, When the worker reaches the packet in queue order, it adopts those fields and only then calls I found one CPU read of indirect arguments in the Driver itself. The fill kernel path in Two new tests pin it:
Both pass on an RTX 5070 Ti and on llvmpipe, along with the ten gate and indirect tests from before. I also added a sentence to the description on where the dynamic reasons are decided. AI-assisted: yes |
d70fd22 to
c097f9a
Compare
|
Thanks, that settles it. The dynamic reasons are decided in |
c097f9a to
03f5443
Compare
adriadam10
left a comment
adriadam10
left a comment
There was a problem hiding this comment.
Checked on the PR merged onto current main: libSceAgcDriver build + ctest green (only agc_driver_tessellation fails, it also fails on main without this PR). The first two tests fail without the change (they need AgcDriverDispatchesAdoptedAhead_nid_postfix) and pass with it.
One point:
IndirectFillKernelArgumentsWrittenAhead.cpppasses with the second commit's change removed. I replacedif (resolving) return;atDispatch.cpp:63with nothing, rebuilt, andagc_driver_indirect_fill_kernel_arguments_written_aheadstill passed (128 records filled). So the test doesn't pin that behaviour. Please make the test reach the fill-kernel branch with the look-ahead active (for example, assert throughAgcDriverDispatchesAdoptedAhead_nid_postfixthat the fill kernel dispatch was not read at the gate, or change the setup so the argument read at the gate gives a different count) so that it fails without the line. If the line turns out to be unreachable in that setup, drop it from the PR.
Happy to look again once the test fails without that line.
…un writes its arguments The dispatch look-ahead stopped before every DISPATCH_INDIRECT whose arguments an earlier dispatch of the run may write, or whose earlier dispatches have unknown writes. That stop protects a CPU read of the arguments at the resolve, which only the thread-dimensions, misaligned and APS5_NO_GPU_INDIRECT paths make. A GPU-read indirect dispatch never reads its arguments on the CPU, so the stop left it and every dispatch behind it to be captured when the worker reached them, after the GPU mutex waits between dispatches. That is the late read the group capture exists to avoid: by then the title had re-armed the gate and reused its table slots, and the capture read descriptors the title had not written yet. The look-ahead now stops only before an indirect dispatch whose arguments it would read on the CPU; the path choice is shared with dispatchIndirect. AgcDriverDispatchesAdoptedAhead_nid_postfix counts the dispatches adopted from the look-ahead so a test can tell a gate-time capture from a late one. Two tests check through it that both dispatches of a gated group are captured at the gate: one whose writer stores the indirect arguments through a registered binding, and one whose writer stores them through a binding misaligned in its host import.
03f5443 to
6b14169
Compare
|
You're right, the test couldn't pin that line. With the look-ahead active, So I dropped the line and AI-assisted: yes |
adriadam10
left a comment
adriadam10
left a comment
There was a problem hiding this comment.
Re-checked on the PR merged onto current main: libSceAgcDriver build + ctest green (only agc_driver_tessellation fails, as on main). The fill-kernel commit is gone, which settles my earlier point.
With AgcDriverDispatchesAdoptedAhead_nid_postfix kept but the indirectArgumentsPath(...) != IndirectGpu condition removed from DispatchAhead.cpp:153, both agc_driver_gpu_indirect_arguments_written_ahead and agc_driver_gpu_indirect_arguments_misaligned_writer_ahead fail; with the change they pass. Merging once the full Build is green.
|
@fisherjoey Independent Windows build finding after this merge: unchanged main I reproduced it by building the fixture's object target from an otherwise clean main checkout. The file is also byte-identical on newer main The four-line fixture-only correction is now proposed in #6452, fork commit 4fe92c9c2, based on main0029. Full Windows app/libs/checkers pass. Eight related groups pass five repetitions per NVIDIA/AMD selector; this fixture also passes twenty extra fresh processes per GPU, with verified device identity. It retains the 64 KiB registered block, four-byte argument offset, and original 24-group/two-adopted-dispatch assertions. The PR records the exact tests and the unchanged Low() Cppcheck advisory. No production-driver change or title result is claimed. AI-assisted: Codex. |
|
@Amoenus thanks for catching it. I only built and ran it on Linux (GCC 16), on an RTX 5070 Ti and lavapipe. #6452 looks right to me: a 128 KiB minus one buffer always contains a 64 KiB-aligned 64 KiB block, the new AI-assisted: yes |
4 participants
What
The dispatch look-ahead added in #1864 resolves a gated compute group at the gate, before the title can rewrite the tables the group reads. #2990 made the look-ahead stop before a
DISPATCH_INDIRECTwhose 12 argument bytes an earlier dispatch of the run may write (or when an earlier dispatch has unknown writes). The stop is needed when the driver reads the arguments on the CPU at resolve time. It also fires when the arguments stay on the GPU, where the CPU never reads them, so in that case it protects nothing.With this change the look-ahead stops only for the CPU-read paths.
Driver::indirectArgumentsPath()now holds the path choice thatdispatchIndirectalready made (thread dimensions, misaligned,APS5_NO_GPU_INDIRECT), andresolveGroupAhead(DispatchAhead.cpp) asks it before stopping. The dynamic CPU-read reasons (pending image results, workgroup memory, a pending label or copied writer over the argument dwords, no host import) are decided inVulkanDevice::decideIndirect. That runs only on the queue-order pass, after the look-ahead's parked result is adopted (Driver::dispatchreturns before any device call while resolving), so it sees the earlier dispatches of the run already recorded. A new entry point,AgcDriverDispatchesAdoptedAhead_nid_postfix, counts the dispatches adopted from the look-ahead so a test can tell a gate-time capture from a late one.Why
When the stop fires on a GPU-read indirect dispatch, that dispatch and everything behind it in the group are captured when the worker reaches them, after the GPU-mutex waits between dispatches. That is the late read from #1787: by then the title has re-armed the gate and started reusing its table slots, so the capture can read a descriptor the title has not written yet.
Demon's Souls (PPSA01341 v1.07) hits this while loading the tutorial level, on compute shader
0x248a05b00, with the same messages #1787 listed:buffer descriptor uses an unsupported type(ShaderResources.cpp) andruntime image descriptor is invalid. Every indirect dispatch in that title is GPU-read. The[groupcap] ... read an earlier dispatch's outputcounter was 0 on the #1864 build and has been 150 to 760 per 10 s on every build since 10-11.@CnRJay, this narrows your stop from #2990; every path that reads the arguments on the CPU still stops. #2990's test
agc_driver_indirect_arguments_written_aheadstill passes.#5058 (@Kikoarl) changes the same lines and adds two more CPU-read reasons: arguments not covered by a registered readable range, and pending image writes to flush. Whichever of the two lands second should put the new checks in
indirectArgumentsPath(). The look-ahead then keeps stopping for them, because both places use the same function. I'll rebase this PR if #5058 goes in first.Tested
agc_driver_gpu_indirect_arguments_misaligned_writer_ahead(the earlier dispatch writes registered arguments through a binding that is notminStorageBufferOffsetAlignmentaligned). It and the gate and indirect tests (agc_driver_gpu_indirect_*,agc_driver_indirect_*,agc_driver_gated_dispatch_capture*,agc_driver_write_data_written_ahead,agc_driver_prepared_shaders_indirect, 8 tests) pass on an RTX 5070 Ti and on llvmpipe at 6b14169.agc_driver_gpu_indirect_arguments_written_ahead(fix(agc): stop the dispatch look-ahead before indirect arguments an earlier dispatch may write #2990's test, but with a GPU-readDISPATCH_INDIRECT): with main's stop it fails withthe gated group adopted 1 dispatches resolved at the gate, expected 2, and with this change it passes.agc_driver_indirect_arguments_written_ahead(fix(agc): stop the dispatch look-ahead before indirect arguments an earlier dispatch may write #2990) andagc_driver_gated_dispatch_capture(fix(agc): land cross-queue waits and capture the gated compute group at the gate #1864) still pass. All three ran on lavapipe on this branch, rebased on main 66f8947.[groupcap]reports showed0 read an earlier dispatch's output. Without the change, 3 of the last 5 runs on the same stack stopped on0x248a05b00during the level load. No performance claim.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)