Repository navigation
feat(agc): accept the GFX10 packet forms RADV emits - #1917
Conversation
klosixgaming-arch
left a comment
There was a problem hiding this comment.
I can approve this. It meets the "cited source / measured" bar for shader/GPU behaviour, which is the one thing I hold these PRs to hardest.
- Every relaxation is grounded: you name the driver (mihawk-99/PS5_Mesa), the title (ccleste PPSA59322), and you actually render ccleste with the change. That's the difference between "accept a new packet form" and "guess it's safe".
- The test discipline is exactly right for a validator: for each accepted form there's a neighbouring rejected form right next to it (an enable bit in
CONTEXT_CONTROL, a wrong index bank, stats/VGT_FLUSHevent indices,VGT_FLUSHon compute, dispatch modifiers beyondFORCE_START_AT_000,ACQUIRE_MEMbits above 48, out-of-range consecutive-load register). Accept-then-reject is how you keep the boundary tight. case 0x28: return;forCONTEXT_CONTROLis defensible because you only accept the form that zeroes the load/shadow enables and rejects any enable bit, compute queue, or short packet. It's a no-op, not a guess.- ACQUIRE_MEM widening from 40-bit to 48-bit is bounded and the >48 bits are still rejected.
- You explicitly defer
DMA_DATAto NOWHERE to #1709 rather than half-implementing it here — good scope discipline.
Non-blocking notes:
- "Not tested on AMD, Intel, Linux or macOS" — fine for a PM4 validator (it's backend-independent), just confirming you know the validator is platform-neutral so that's not a real gap.
- The
FORCE_START_AT_000rationale ("the driver never offsets dispatches byCOMPUTE_START_X/Y/Z") is a claim about RADV specifically. If a different RADV build or a future one did offset, this silently accepts it as zero. Worth one sentence in the debt entry that this assumesCOMPUTE_START_X/Y/Zare zero for GFX10 dispatches.
Written with AI assistance.
|
Linux run on AMD, since the description lists it as not tested: Ubuntu 24.04.5 (kernel 6.8), GCC 13.3, Release, AMD Radeon RX 7600 (RADV NAVI33, Mesa 25.2.8). This PR's head |
|
Please move the four commits added for vkQuake (partial last thread group, |
adriadam10
left a comment
There was a problem hiding this comment.
Following @oneandonlydean's request above, which is still open:
- Move the vkQuake commits out of this PR into a new one:
run RADV's dispatches with a partial last thread group,load context registers from memory with COPY_DATA,accept COPY_DATA to memory through DST_SEL 5,accept dispatches that launch waves in order. They change how many threads a dispatch runs, so they need their own review. - The two commits added after that comment (
read indirect draw register location 0 as no register,accept 4-byte-aligned indirect argument bases) also come from vkQuake and change indirect draw/dispatch decoding, so they belong in the new PR as well. - What stays here: the first two commits (the ccleste packet forms:
CONTEXT_CONTROL,SET_UCONFIG_REG_INDEX,WRITE_DATADST_SEL 5, the events,ACQUIRE_MEM,DISPATCH_*header bit /FORCE_START_AT_000,LOAD_*_REG_INDEX), with the matching tests and theDispatch.cpp/ShaderInputState.cpp/Graphics.cppchanges they need. Update the PR body to match. - The bot flagged overlap with #2156 in
Pm4.cpp; list it under Depends on if the split still touches the same lines.
Once split, this is ready for another look.
RADV builds its own command streams. Its preamble and draws use packet forms the validator refused: - CONTEXT_CONTROL that only updates the load and shadow enables to zero, which RADV emits when it does not shadow registers. It is a no-op. - SET_UCONFIG_REG_INDEX of VGT_PRIMITIVE_TYPE with index 1, next to the index-2 VGT_INDEX_TYPE write already accepted. - WRITE_DATA to memory (DST_SEL 5) from the graphics queue. - DMA_DATA with DST_SEL NOWHERE, RADV's L2 prefetch of shader code. It writes nothing and resolves to an empty store. - EVENT_WRITE PIPELINESTAT_START, PIPELINESTAT_STOP, SQ_NON_EVENT and VGT_FLUSH. The statistics counters are not modelled, and the events that would read them stay rejected. - ACQUIRE_MEM over the full 48-bit range (COHER_SIZE_HI and COHER_BASE_HI). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- DISPATCH_DIRECT and DISPATCH_INDIRECT with the SHADER_TYPE header bit, which RADV sets on every dispatch, as other packets already may. - FORCE_START_AT_000 in the dispatch initiator. The driver never offsets dispatches by COMPUTE_START_X/Y/Z, so it already starts at zero. - LOAD_CONTEXT_REG_INDEX, LOAD_SH_REG_INDEX and LOAD_UCONFIG_REG_INDEX in their consecutive-register form (a register offset and a count, DATA_FORMAT 0), which RADV uses to load DB_STENCIL_CLEAR and DB_DEPTH_CLEAR from an image's clear values. The offset-and-data form keeps its old checks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2f026c3 to
7563819
Compare
|
@adriadam10 @oneandonlydean the split is done. This PR now holds only the first two commits, the ccleste packet forms, rebased onto The six vkQuake commits (partial last thread group, The AI-assisted (Claude Code). |
adriadam10
left a comment
There was a problem hiding this comment.
Checked the build and the scoped ctest for libSceAgcDriver (312/312, agc_driver_pm4 included) on Linux, and that the new Pm4.cpp test cases fail with the validator change reverted. The split @oneandonlydean asked for is done, and each accepted form has its still-rejected neighbour in the tests. Merging.
What
RADV on the PS5 (mihawk-99/PS5_Mesa) builds its own command streams, as on Linux. Its preamble, draws and dispatches use packet forms the PM4 validator refused, so ccleste's (PPSA59322) command buffers were rejected.
Two commits.
Graphics preamble and draws:
CONTEXT_CONTROLthat only updates the load and shadow enables to zero, which RADV emits when it does not shadow registers. It is a no-op. Any enable bit, a compute queue, or a short packet is still rejected.SET_UCONFIG_REG_INDEXofVGT_PRIMITIVE_TYPEwith index 1, next to the index-2VGT_INDEX_TYPEwrite already accepted.WRITE_DATAto memory (DST_SEL5) from the graphics queue.EVENT_WRITEPIPELINESTAT_START(25),PIPELINESTAT_STOP(26),SQ_NON_EVENT(38) and, on the graphics queue,VGT_FLUSH(36). The statistics counters are not modelled, and the events that would read them stay rejected.ACQUIRE_MEMover the full 48-bit range (COHER_SIZE_HIandCOHER_BASE_HI); the bits above 48 are still rejected.Dispatches and register loads:
DISPATCH_DIRECTandDISPATCH_INDIRECTwith theSHADER_TYPEheader bit, which RADV sets on every dispatch, as other packets already may.FORCE_START_AT_000in the dispatch initiator. The driver never offsets dispatches byCOMPUTE_START_X/Y/Z, so they already start at zero.LOAD_CONTEXT_REG_INDEX,LOAD_SH_REG_INDEXandLOAD_UCONFIG_REG_INDEXin their consecutive-register form (a register offset and a count,DATA_FORMAT0). RADV uses it to loadDB_STENCIL_CLEARandDB_DEPTH_CLEARfrom an image's clear values. The offset-and-data form keeps its old checks.The four vkQuake commits added after review started (partial last thread group,
COPY_DATAto context registers,COPY_DATADST_SEL5, in-order dispatches) and the two that followed (indirect draw register location 0, 4-byte-aligned indirect argument bases) are no longer in this PR, as requested in review. They change how many threads a dispatch runs and how indirect draws and dispatches are decoded, so they will go to their own PR. That one waits until I am allowed to open new PRs again.Tests (
agc_driver_pm4): each accepted form, and next to it the neighbouring form that is still rejected (an enable bit inCONTEXT_CONTROL, a wrong index bank, event indices on the statistics andVGT_FLUSHevents,VGT_FLUSHon a compute queue, an unrelated event type, dispatch modifiers beyondFORCE_START_AT_000,ACQUIRE_MEMrange bits above 48, and an out-of-range register in the consecutive load form). Each new check fails with its change reverted.RADV's other packet,
DMA_DATAtoDST_SELNOWHERE (its L2 prefetch of shader code), is not here: #1709 implemented it and is merged.Tested
Windows 11 Pro (26200), MinGW-w64 GCC 15.2.0 (WinLibs r7), Release, NVIDIA GeForce RTX 5070 Ti.
mainecd3dba with only the first two commits. Full suite: 512 of 515 pass, 1 skipped. The two failures,agc_driver_prepared_shaders_debug_failureandagc_driver_f32_arithmetic_nan, fail the same way on cleanmainecd3dba on this machine. It was run from the generatedCTestTestfile.cmakecommands because antivirus blocksctest.exeon this machine.tools/check_conventions.pypasses.Not tested on AMD, Intel, Linux or macOS. Earlier, @johnmaia ran the full suite on an RX 7600 (RADV) with the eight-commit head: 494/494.
Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt.mdfiles or images added to the repository (attach them to this PR)Pm4.cpp, is closed)🤖 Generated with Claude Code