fix(runtime): finalize packet-builder DMAtag QWC once at close, not on every append - #173
Draft
smmathews wants to merge 1 commit into
Draft
Conversation
…n every append writePacketBuilderCurrent() eagerly overwrote a pending DMAtag's QWC field on every append to a packet under construction. Harmless when our own stubs own a block's whole open/append/close lifecycle, but wrong the moment a title links its own sceGifPk/sceVif1Pk-shaped guest code and only lands appends on our stubs: the guest's own terminate adds (delta/16 - 1) onto what it assumes is a zero-seeded field (matching real hardware, where a source-chain DMAtag's QWC is filled in only at close), doubling a value our code had already written. An over-long QWC then makes the DMAC swallow the following DMAtags as inline payload data, so the REF tags pointing at the real payload never enter the stream and the chain terminates early (in DQ8's movie feed this buried ~896 image-transfer REF tags in one over-long CNT, and the decoded frame displayed nothing). Move the one legitimate refreshPacketBuilderPendingCount() call out of writePacketBuilderCurrent() and into terminatePacketBuilderState(), so the outer DMAtag QWC is set exactly once, at close, matching the real library. This helper is shared by both the sceGifPk* and sceVif1Pk* builders, which populate the same outer-DMAtag QWC slot; both now finalize it at terminate. For a block our own stubs open and close the final value is unchanged (refreshPacketBuilderPendingCount recomputes the delta fresh rather than accumulating); for a block whose open/close are guest code, our stubs no longer touch the field before the guest's single += lands on a true zero. Same double-count family as PR ran-j#149 (sceGifPkRefLoadImage nloop), different call site (the pendingCountAddr tracking used by the Cnt/terminate path), no dependency either way. Adds two byte-level GIF regression tests: one asserting appends leave the pending CNT DMAtag QWC untouched mid-block (reintroducing the eager refresh fails it), one asserting close finalizes the QWC to the true appended qword count and clears the pending-count slot. Updates the VIF1 packet-builder test to terminate the chain before kicking the DMA -- the real usage pattern, since the DMAC reads a CNT tag's QWC from memory when it fetches the tag; the test's prior no-terminate kick only transferred because the eager per-append refresh had side-populated the field, which does not happen on hardware.
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.
Problem
writePacketBuilderCurrent()— the helper the append, reserve and align paths call toadvance the packet-builder write cursor — also patched the pending DMAtag's QWC field on
every call. A source-chain DMAtag's QWC is filled in once, when the block closes: the DMAC
reads it from memory at tag fetch, which happens after the kick.
Some titles link their own copy of the
sceGifPk/sceVif1Pkpacket-builder library andland only a few leaf entry points (
sceGifPkAddGsAD,sceGifPkCloseGifTag) on thesestubs. Their own code opens and closes the block, and that close adds
(delta/16 - 1)onto a QWC field it expects to be zero. The eager per-append patch had already written the
final count, so the guest's add doubled it: a block holding three qwords closed with
QWC 6.
A DMAtag claiming twice its real length makes the DMAC read the following tags as inline
payload, stopping at the first zero qword it takes for a terminator. In one observed
movie-playback chain this buried roughly 896 image-transfer REF tags inside a single CNT
payload; the chain ended after two tags and none of the decoded pixel data reached the GIF.
Fix
In
ps2xRuntime/src/lib/Kernel/Stubs/GS.cpp:writePacketBuilderCurrent()now only advances the write cursor; therefreshPacketBuilderPendingCount()call is removed.terminatePacketBuilderState(), the block-close path, callsrefreshPacketBuilderPendingCount()once, immediately after its final cursor advance.Blocks these stubs open and close finalize to the same value as before, because the
refresh recomputes the qword delta from
pendingCountAddrrather than accumulating. Everysite that records a new pending tag also calls
terminatePacketBuilderState()first, sothe previous tag is still finalized before its slot is reused — check with:
A guest-closed block stays untouched until the guest's own
+=lands on the zero-seededfield. No title, opcode or address is special-cased.
Basis
that tag, so the builder must fill it in at close. The guest library's
add-onto-zero terminate follows from the same rule.
sceGifPkRefLoadImagepre-seeded an A+D GIFtag's
nloop, thenclosePacketGifTag()added the true count ontop. That one stayed inside these stubs. This is the
pendingCountAddrtracking used bythe
Cnt/terminate path, which double-counts where guest code performs the close. Thetwo touch disjoint functions and neither depends on the other.
Testing
ps2_gs_tests.cpp: after three A+D appends the pending tag still reads0x10000000(id=CNT, QWC=0), cursor advanced three quadwords. Restoring the per-append refresh makes
it read QWC 3 and this fails.
ps2_gs_tests.cpp: aftersceGifPkTerminatethe tag reads0x10000003and thepending-count slot at
stateAddr+8is cleared. Dropping the added finalization callleaves QWC 0 and this fails.
ps2_memory_tests.cpp: the VIF1 chain test now callssceVif1PkTerminatebefore kickingthe DMA, and asserts the full head-tag word
0x10000001rather than a low-16 maskedcompare. It previously kicked without terminating, and passed only because the per-append
refresh had side-populated the tag.
Risk and not in scope
sceGifPk*andsceVif1Pk*builders, which populatethe same outer-DMAtag QWC slot, so both families change the same way. The same edit
closes the VIF1 double-count.
nothing, matching hardware, where the DMAC finds QWC still zero at fetch. The per-append
refresh used to mask that; the one test that leaned on the masking is updated above.
nlooppath (fix(runtime): stop double-counting nloop in sceGifPkRefLoadImage A+D header GIFtag #149's territory). No header, signature orbuilder-state changes.
Before and after, and the pending-slot call sites
Before — the cursor advance patched the QWC on every append:
After — cursor only, with finalization moved into the close path:
refreshPacketBuilderPendingCount()recomputes rather than accumulates, which is why astub-owned block finalizes to the same value under either scheme:
Among the grep's hits, the sites that record a pending tag at
stateAddr+8each store thevalue returned by an immediately preceding
terminatePacketBuilderState():sceGifPkCnt,sceGifPkEnd,sceGifPkRefLoadImage(both tag opens),sceVif1PkCall,sceVif1PkCntandsceVif1PkEnd.