fix(dma): transfer the VIF chain tag half for every tag id - #190
Draft
smmathews wants to merge 1 commit into
Draft
fix(dma): transfer the VIF chain tag half for every tag id#190smmathews wants to merge 1 commit into
smmathews wants to merge 1 commit into
Conversation
The DMA chain walker only appended a chain tag's upper-half vifcodes to the VIF stream for CNT/NEXT/CALL/RET/END tags, skipping REFE/REF/REFS entirely. That gate conflates the tag id, which says where a tag's payload lives, with the tag's own upper quadword, which hardware transfers for every tag id when CHCR.TTE is set. REF/REFS/REFE are exactly the tags most likely to put something meaningful in that half, since their payload lives elsewhere; dropping it desyncs the parser because the referenced payload gets decoded as vifcode instead of being consumed as the data it is. Make the append unconditional per tag on both VIF channels instead of gating it on id, since the latched CHCR does not reliably expose TTE here and gating on it would regress chains that work today. Fixing that alone regresses PATH2 image continuation, because the interpreter used to drain pending continuation data as a raw byte grab at the top of its loop, with no regard for whether those bytes were actually a vifcode. Now that a chain can legitimately contain a real vifcode between an image DIRECT and its continuation, move the drain into the DIRECT/DIRECTHL handler itself: a pending continuation is only ever satisfied by a follow-up DIRECT/DIRECTHL, out of the front of that vifcode's own payload. Also raise the chain-tag walk cap, which was low enough to silently truncate a legitimately long chain, and add a one-time diagnostic for when it is hit so a truncation is visible instead of silent. The cap itself stays, as the only guard against a self-referential NEXT tag hanging the walker. Four tests pin all three pieces directly against processVIF1Data and PS2Memory, each written and confirmed failing against the unmodified tree before the fix, with no game content involved.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
VIF chain walker drops the tag half for REF/REFS/REFE tags
The DMA chain walker in
PS2Memory::writeIORegisterreads each chain tag'supper quadword (the 8 bytes at
TADR+8) and appends it to the VIF streamonly for tag ids 1, 2, 5, 6 and 7 (CNT/NEXT/CALL/RET/END). Ids 0, 3 and 4
(REFE/REF/REFS) were skipped entirely.
That gate conflates two different things: the tag id says where a tag's
payload lives (locally after the tag, or at a separately referenced
address), but the tag's own upper quadword is a fixed hardware field that
exists for every tag id, at the same offset, regardless of where the
payload lives. On real hardware, with CHCR.TTE set, that quadword is
transferred for every tag id.
REF/REFS/REFE are exactly the tags whose upper half is most likely to carry
something meaningful, because their payload lives elsewhere: a packet
builder can put an STCYCL+UNPACK pair (or a DIRECT header) in the tag half
itself and point the tag at a separate block of raw data. Dropping that
half is worse than dropping a CNT tag's half, because the vifcode that
would have explained how to consume the referenced block never runs; the
referenced bytes are decoded as vifcode instructions instead, and the
stream desyncs from there.
The fix makes the tag-half append unconditional per tag, for both VIF
channels (0x10008000 and 0x10009000), instead of gating it on the tag id.
Both channels are handled the same way; narrowing this to one channel would
silently regress the other.
Why unconditional, not CHCR.TTE-gated
Gating the append on CHCR.TTE would be the more literally faithful fix,
but the latched CHCR value at walker time does not reliably reflect TTE in
this runtime today, so gating on it would drop the tag half for chains that
currently render correctly. Appending unconditionally is a strict superset
of the current behavior on the VIF channels: it can only add bytes that
were previously dropped, never remove any. The residual risk is a producer
that deliberately runs with TTE clear and stores meaningful non-zero
scratch in the tag half, expecting it to be ignored; unused padding
normally decodes as a pair of harmless NOPs, and no such producer has been
observed. Getting CHCR.TTE latching right is a separate, unstarted
investigation into how this runtime merges CHCR writes, and is out of
scope here; unconditional transfer composes cleanly with a later
TTE-gated narrowing.
The PATH2 continuation drain has to move with it
Once REF/REFS/REFE tag halves are transferred, a chain can contain a real
vifcode (for instance a NOP used as padding) sitting between a DIRECT that
opened a PATH2 image transfer and the DIRECT that continues it. The
interpreter's image continuation was implemented as a raw byte drain at the
very top of the vifcode loop: whenever an image continuation was pending,
it would grab the next 16-byte-aligned chunk straight off the stream and
treat it as image pixels, with no regard for whether those bytes were
actually a vifcode. Turning on the tag-half fix without also fixing this
would regress PATH2: real vifcodes between the two DIRECTs get silently
swallowed as pixel data instead of executing, and the continuation itself
ends up reading from the wrong offset.
The fix removes that top-of-loop drain and moves the continuation logic
into the DIRECT/DIRECTHL handler itself. A pending continuation is now only
satisfied by another DIRECT/DIRECTHL vifcode: the front of that vifcode's
own payload supplies the outstanding image quadwords, and anything left
over in that same payload is treated as the DIRECT's own new content. Any
vifcode that is not a DIRECT/DIRECTHL runs normally, whether or not a
continuation is pending. This matches how a real VIF stream is
structured: everything in it is a vifcode-driven action, and "raw
quadwords with no vifcode wrapper" is not a primitive that should ever
have existed in the interpreter.
Chain-tag cap
The walker also caps the number of tags it will walk in one chain, as a
guard against a self-referential NEXT tag hanging the walker forever. The
cap was low enough that a legitimately long chain could be silently
truncated, with no indication that anything had gone wrong; the walker
would just stop, drop the tail of the chain, and carry on. The cap is
raised substantially, and hitting it now prints a one-time diagnostic
(once per process, not once per chain) so a truncation is visible instead
of silent. The cap itself stays in place; removing it entirely would trade
a silent-truncation bug for a real hang.
Acceptance tests
Four tests pin these changes directly against
PS2MemoryandprocessVIF1Data, with no game content involved:lands at the expected VU1 offset with the expected bytes. Before the
fix, the referenced quadword is decoded as vifcode instead of being
consumed as UNPACK data, so nothing lands at the expected offset.
expected startPC, given a preceding REF tag whose own half is non-empty.
Before the fix, dropping the REF tag's half desyncs the parser badly
enough that the MSCAL is never reached at all.
vifcodes, and the continuation's image quadwords come from the
continuation DIRECT's own payload. Before the fix, the raw drain eats
the NOPs as image pixels and the continuation quadwords come from the
wrong offset.
final tag's effect is observable. Before the fix, the chain is silently
truncated and the final tag's effect never happens.
Each of these was written and run against the unmodified tree first, and
each failed with exactly the signature described above before any
production code changed. Reproduction:
As background for this change: walking a real VIF1 source chain captured
from a frame and censusing the decoded vifcode stream three ways (main
behavior, unconditional-append, TTE-gated) shows the corrected,
unconditional rule is the only one under which the whole stream decodes
into exclusively legal vifcodes, with zero unrecognized opcodes and zero
physically-impossible microprogram kicks. That census is not part of the
automated gate; the four tests above are.
Overlap with other in-flight work
There is other in-flight work touching an INTC pending-interrupt latch
drain in both of these same two files; it adds a store and a new
consumer method in one file and two lines inside an IRQ block in the
other, and none of those hunks overlap the regions this change touches,
so the two are independently correct and should merge cleanly against
each other.