fix(gs): write PMODE/SMODE2 directly in sceGsResetGraph instead of through colliding GIF A+D addresses - #185
Draft
smmathews wants to merge 1 commit into
Draft
Conversation
…rough colliding A+D addresses sceGsResetGraph builds a GIF A+D packet to program the display-mode registers, and used addresses 0x41 and 0x42 for PMODE and SMODE2. Those addresses are not free A+D slots: GS_REG_SCISSOR_2 is 0x41 and GS_REG_ALPHA_1 is 0x42 in the GIF drawing-register address space, and writeRegister/processGIFPacket already handle them as SCISSOR_2/ALPHA_1 writes into the active drawing context. As a result the packet's PMODE/SMODE2 entries were reinterpreted as scissor and alpha-blend writes, the drawing context got silently reprogrammed, and PMODE/SMODE2 themselves never received the values the caller asked for. PMODE and SMODE2 are privileged registers, not GIF drawing registers, so they don't belong in the A+D stream at all. This routes them directly to the privileged GSRegisters struct instead, the same way applyGsDispEnv already does for the display-env registers. The A+D packet keeps only the five entries that don't collide with anything (DISPFB1/DISPLAY1/DISPFB2/DISPLAY2/BGCOLOR), shrunk accordingly. The other five privileged registers set through this same packet stay on the A+D path deliberately: they have dedicated non-drawing pass-through cases and never collided with anything, so migrating them here would be unrelated churn. If full consistency with the direct-write convention is wanted later, migrating all seven and dropping the DMA packet entirely is the natural follow-up, but it's a separate change. Added a regression test that seeds the drawing context through the same addresses the old code collided on, runs sceGsResetGraph, and asserts PMODE/SMODE2 land in privileged state while the drawing context is untouched.
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
The GS has two distinct register address spaces that must not be confused:
a small block of privileged registers (PMODE, SMODE2, DISPFB, DISPLAY,
BGCOLOR, ...) that control CRTC/display configuration, and the GIF
A+D drawing-register address space that programs the rasterizer's
per-primitive state (PRIM, SCISSOR, ALPHA, TEST, FRAME, ...). The two
spaces share the same 0x00-0x63 numeric range, so an address that looks
like a free A+D slot can actually be a real drawing register.
sceGsResetGraphbuilt a single GIF A+D packet to program PMODE andSMODE2 (among other display-mode registers) and picked addresses 0x41
and 0x42 for them. Those addresses are not free: in
ps2xRuntime/include/runtime/ps2_gs_gpu.h,GS_REG_SCISSOR_2 == 0x41and
GS_REG_ALPHA_1 == 0x42, andGS::writeRegister(reached fromprocessGIFPacket, which the packet's DMA kick drives throughprocessPendingTransfers) handles both as real drawing-context writes:SCISSOR_2 into the second context's scissor rectangle, ALPHA_1 into the
first context's alpha-blend register. So the packet's PMODE/SMODE2
entries were silently reinterpreted as scissor/alpha writes, clobbering
whatever drawing state was active, while PMODE and SMODE2 themselves
never received the values
sceGsResetGraphcomputed for them.Fix
PMODE and SMODE2 are privileged registers, not GIF drawing registers,
so they don't belong in the A+D stream in the first place. This writes
them directly to the privileged
GSRegistersstruct(
runtime->memory().gs().pmode/.smode2) instead of routing themthrough the DMA packet — the same approach
applyGsDispEnvalreadyuses on this codebase for the equivalent display-env writes. The two
A+D entries are dropped from the packet, and the packet's GIFtag NLOOP,
allocation size, and QWC write are shrunk to match. The remaining five
entries (DISPFB1/DISPLAY1/DISPFB2/DISPLAY2/BGCOLOR) are unchanged: they
don't collide with anything, since their addresses have dedicated
non-drawing pass-through cases in
writeRegister.Why leave those five on the A+D path instead of also making them direct
writes: they were never broken, so migrating them here would be
unrelated churn outside this bug's scope. If full consistency with the
direct-write convention is wanted later, migrating all seven privileged
writes and dropping the DMA packet + kick entirely is the natural
follow-up, but it's a separate, larger change.
Risk
The two entries removed from the packet never did what they appeared
to (their payloads were being reinterpreted as scissor/alpha state, not
applied as PMODE/SMODE2), so no caller could have depended on their old
behavior. The five remaining packet entries are byte-identical to
before. The direct writes and the packet write happen in the same
if (runtime)block sceGsResetGraph already guards, so there's no newfailure path.
Rebase note
An in-flight EE-scheduler refactor relocates
resetGsSyncVState()outof this file. It does not touch
sceGsResetGraph's body, but sincethat refactor moves code around this same file, locate the function by
name rather than by line number when rebasing, and diff the two changes
against each other once it's clear which lands first.
Repro
Show the collision in-tree:
Run the regression coverage (this project's test binary runs its full
suite in one process; there is no per-suite filter):