dma: charge DMA2 linked-list walks what the validated model charged

[TechnicallyComputers]

Resubmission of #299, which was merged prematurely and reverted in #300. Same change, re-applied on top of the revert. Opened as a draft: this needs cross-title validation and peer review before it is merged — see Review gate at the bottom.

Fixes a rendering regression from #277 (8af48ae9, "runtime: slice GPU linked-list DMA"). Slicing the walk is right and stays; only its per-node cost changes.

What broke

#277 also changed what a walk costs — 8 clocks per node header plus 5 setup clocks per non-empty node, where the pre-slicing path charged one clock per word via schedule_delayed_complete(2, actual_words, DMA_GPU_CYCLES_PER_WORD).

An ordering table is mostly empty nodes — Final Fantasy VII's runs to ot_max 4094 — and every empty slot still paid the full 8-cycle header for a packet that does not exist. Measured on FF7 (SCUS-94163):

table	measured (#277)	pre-#277 model	ratio	vs 564,480-cycle NTSC frame
4,807 nodes / 12,757 words	52,384	17,564	3.0×	9.3%
44,010 nodes / 117,507 words	499,187	161,517	3.1×	88.4%

At 88% of a frame, FF7's libgpu polls CHCR bit 24, finds the channel still busy after 790–890 reads, and force-clears the start bit at 0x800484F0 — its documented stuck-channel path:

800484B4  lw    $v0, 0($a0)          ; read CHCR
800484B8  lui   $v1, 0x0100          ; 0x01000000 = busy bit
800484BC  and   $v0, $v0, $v1        ; test busy       <-- it CHECKS
...
800484E8  ori   $v1, $v1, 0xFFFF     ; $v1 = 0xFEFFFFFF = ~(1<<24)
800484EC  and   $v0, $v0, $v1
800484F0  sw    $v0, 0($a0)          ; write back      <-- THE ABORT

cancel_async_transfer() then abandons the walk partway through and the rest of the frame's geometry never reaches GP0. 12.6% of ordering tables in a battle were aborted this way; character models rendered with missing or shattered limbs.

The guest is behaving correctly — it inspected the busy bit before clearing it. Only the cost was wrong.

The change

Header costs one clock, setup is free, so a walk costs exactly (nodes + words). When setup is free the payload boundary is the same service as the header, so the header phase falls through instead of returning — cycles_to_event() never reports zero, so returning would spend an unaccounted scheduler cycle per non-empty node. The packet is still read at its own boundary, so #277's mid-walk mutation behaviour is unchanged and its existing assertion still passes.

(nodes + words) is not a new assertion: it is the cost this runtime shipped and was validated against. A non-zero per-node overhead may well be real on hardware, but it needs a measurement behind it rather than a substituted constant.

Verification

FF7 SCUS-94163, from a save state, walk still sliced:

	before	after
cycles_max	58,933	16,972
144-node / 490-word walk	—	exactly 490 cycles
aborted ordering tables (battle)	12.6%	user-confirmed geometry correct

Test re-run and runtime rebuilt clean on top of current master after the revert.

The timing test now asserts the cost model instead of describing it: a table of empty nodes must cost exactly one cycle per node. That is the check that would have caught this.

Observability

None of the above was visible, so dma_state gains a gpu_ot block: starts, completes, cancels, starts_dropped, the last walk's nodes/words/cycles and the max, guest reads of CHCR(2) total and during a walk, and a ring of the last 8 aborts carrying the guest PC, CHCR value, how far the walk got, and how many times the guest polled before giving up. The silent early return in start_async_gpu_linked_list() is now counted as starts_dropped rather than losing an ordering table with nothing said.

PSX_GPU_LL_SYNC=1 drains a walk at start — diagnostic A/B lever only. It completes immediately as well, so it is not a faithful restoration of the pre-slicing timing and must not be used as a fix.

Known issue, deliberately not fixed here

The same commit broke gpu_frame_dump's func attribution. GP0 writes now issue from dma_advance() on the cycle scheduler, so every primitive is stamped with whatever the scheduler interrupted — all 1,803 primitives in a captured battle frame attributed to BIOS 0x1FC02B50 instead of the issuing game function. That disables gpu_frame_diff's "a function stopped drawing" verdict and gpu_frame_layers entirely.

Stamping the DMA initiator was tried and does not work — g_debug_current_func_addr reads 0 at kick time. The ra column still carries real game addresses as a fallback. Filed as a separate issue rather than shipped as a fix that does not fix it.

Review gate — do not merge until these are done

• Cross-title validation. DMA2 timing is shared bus semantics, so every supported title must be validated on one revision. No subset, no sampling. Only FF7 is validated so far.
• Peer review of the cost model and the header-phase fall-through.
• The two ~44,010-node walks in the cancel ring are unexplained. They appear only on long, high-poll transfers, consistent with the walker following an ordering table the guest had already begun rebuilding. The 3.0× ratio does not depend on them — it reproduces on the clean 4,807-node table — but a nodes_max counter would confirm whether a walk that cannot be pre-empted ever reaches that size.

🤖 Generated with Claude Code

https://claude.ai/code/session_01L9v7e6AVZe3eRP8VXey9Pz

[kerokline]

I had Claude review and rig up a a/b test for Breath of Fire 3. Here's the summary ->

Review notes for upstream (what the PR is missing)

1. 8+5 was not "a substituted constant with no measurement". It is DuckStation's model verbatim: LINKED_LIST_HEADER_READ_TICKS = 8, LINKED_LIST_BLOCK_SETUP_TICKS = 5 in src/core/dma.cpp. The PR's claim that 1+0 is "the validated model" only means it is what psxrecomp charged before #277.

2. The real defect is CPU/DMA concurrency, not the constant. psx-spx (DMA channels, "DMA Transfer Rates"): the CPU keeps running during DMA only while it touches cache, scratchpad, COP0 and GTE; any read from RAM or an I/O register stalls the CPU until the DMA is finished, resuming only between SyncMode 2 list entries. FF7's libgpu poll reads CHCR — an I/O register — so on hardware the first poll blocks until the walk completes and the 790–890-poll abort can never fire. DuckStation gets the same outcome by charging the walk's ticks to the CPU (CPU::AddPendingTicks) rather than letting it run in parallel. #277 slices the walk while the guest runs freely; that is what created the abort, and 1+0 hides it by making walks short instead of modelling the stall. The faithful fix is a stall (or drain-to-completion) on guest RAM/MMIO reads while DMA2 is active. 1+0 is acceptable as an interim, but it should be labelled as such, not as the validated hardware cost.

3. Behavioural nit. With setup = 0 the payload is now read on the same service as its header (the fall-through), not "at its own boundary" as the description says. Harmless — pre-#277 read everything at kick — but the comment overstates it.
   Counters are worth keeping regardless. gpu_ot in dma_state is what made this census a ten-minute job; the PSX_GPU_LL_SYNC lever is fine as a diagnostic.

I attached the .md file if you want to run a agent to see if yours agrees.
pr302-dma2-ot-cost-review.md

[mstan]

Would like to get a consensus before merging this one, as I believe it's been superseded by #304 . Would like to get affirmation from @kerokline and @TechnicallyComputers on whether 304 has solved the problem already. If not, can we get a follow-up PR or a refactor of this one that's rebased onto the current main?

[TechnicallyComputers]

I can test this myself to validate it fixes ff7 on master branch soon

[kerokline]

Ran the same A/B comparison with the 304 build. Here is the AI summary ->

Yes, #304 addresses the same root issue, and it supersedes #302's fix. The comparison is written up in pr302-dma2-ot-cost-review.md

Where they are identical. Both set the walk cost to header 1, setup 0, so a walk costs nodes plus words. The two #define lines are the same. #304 even carries #302's empty-ordering-table bound test verbatim, comment included. Both chase the same symptom: FF7's libgpu timing out on a long sliced walk and aborting the transfer.

Where #304 goes further on timing:

Each payload word is read from live RAM at its own one-clock event, instead of the whole packet on the header service. That also covers the Vampire Hunter D and Spot mutation cases #277 was written for.
A late scheduler service consumes every elapsed boundary rather than one.
The widescreen prepass fingerprints cached nodes and discards them on live change.
The old polygon-drop environment hack is removed.

What #304 does not have, and #302 still uniquely offers: the gpu_ot observability block. The counters, the cancel ring with guest PC and poll count, the CHCR-poll stats, and starts_dropped. That is what my census ran on, and it is the only way to see an FF7-class abort without a screenshot. Worth salvaging as a small counters-only PR rebased on master. I would not keep the walker change.

Neither fixes the hardware issue: a guest RAM or MMIO read during DMA stalls the CPU, so the poll loop can never reach its timeout. Both PRs shorten the walk instead of modelling the stall.

[mstan]

Went ahead and made #317 as a follow up to this to merge out the odds and ends. Attributed you as co-author on those.

Should be solved now. Will go ahead and close this out. If for any reason I missed something, do go ahead and open a new issue or file a new PR 👍

[TechnicallyComputers]

Yes this is superseded now and the fix is already on master.