Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ProcessorInterface: Reset both GPFifo and Fifo on PI_FIFO_RESET #10883

Merged
merged 6 commits into from Jul 31, 2022

Conversation

Pokechu22
Copy link
Contributor

See #10454. Fixes https://bugs.dolphin-emu.org/issues/12981. Only the last commit has behavior differences; the rest are refactoring.

Before #10454, writing 1 to PI_FIFO_RESET would do nothing (this logging is from a modified build):

VideoCommon\CommandProcessor.cpp:98 W[CP]: Dumping fifo information: Reset
CP: Read 0078fda0 Write 0078fda0 Dist 00000000/00000000 (should be equal)

GP fifo: Size 1b
33 99 ff ff 00 00 00 00 3f 80 00 00 61 20 15 61 56 61 21 3d 53 35 61 40 00 00 17
Video: Read 001c8f99 Write 001c8fe0 Dist 00000047 (computed)
81 00 04 42 b6 1e ea 43 38 e3 4c 33 99 ff ff 00 00 00 00 00 00 00 00 42 ef b8 84 43 38 e3 4c 33 99 ff ff 3f 80 00 00 00 00 00 00 42 ef b8 84 43 5f 49 b2 33 99 ff ff 3f 80 00 00 3f 80 00 00 42 b6 1e ea 43 5f 49 b2
VideoCommon\CommandProcessor.cpp:98 W[CP]: Dumping fifo information: Reset after
CP: Read 0078fda0 Write 0078fda0 Dist 00000000/00000000 (should be equal)

GP fifo: Size 1b
33 99 ff ff 00 00 00 00 3f 80 00 00 61 20 15 61 56 61 21 3d 53 35 61 40 00 00 17
Video: Read 001c8f99 Write 001c8fe0 Dist 00000047 (computed)
81 00 04 42 b6 1e ea 43 38 e3 4c 33 99 ff ff 00 00 00 00 00 00 00 00 42 ef b8 84 43 38 e3 4c 33 99 ff ff 3f 80 00 00 00 00 00 00 42 ef b8 84 43 5f 49 b2 33 99 ff ff 3f 80 00 00 3f 80 00 00 42 b6 1e ea 43 5f 49 b2

After, it would clear the FIFO used by the GPU execution (from Fifo.cpp), but not the 32-byte fifo written to directly (from GPFifo.cpp):

VideoCommon\CommandProcessor.cpp:98 W[CP]: Dumping fifo information: Reset
CP: Read 0078fda0 Write 0078fda0 Dist 00000000/00000000 (should be equal)

GP fifo: Size 1b
33 99 ff ff 00 00 00 00 3f 80 00 00 61 20 15 61 56 61 21 3d 53 35 61 40 00 00 17
Video: Read 001c8f99 Write 001c8fe0 Dist 00000047 (computed)
81 00 04 42 b6 1e ea 43 38 e3 4c 33 99 ff ff 00 00 00 00 00 00 00 00 42 ef b8 84 43 38 e3 4c 33 99 ff ff 3f 80 00 00 00 00 00 00 42 ef b8 84 43 5f 49 b2 33 99 ff ff 3f 80 00 00 3f 80 00 00 42 b6 1e ea 43 5f 49 b2
VideoCommon\CommandProcessor.cpp:98 W[CP]: Dumping fifo information: Reset after
CP: Read 0078fda0 Write 0078fda0 Dist 00000000/00000000 (should be equal)

GP fifo: Size 1b
33 99 ff ff 00 00 00 00 3f 80 00 00 61 20 15 61 56 61 21 3d 53 35 61 40 00 00 17
Video: Read 00000000 Write 00000000 Dist 00000000 (computed)

When enough data was written into the GP fifo, this would result in an unknown opcode error since 33 isn't a valid graphics command.

Now, the GP fifo is also cleared:

VideoCommon\CommandProcessor.cpp:98 W[CP]: Dumping fifo information: Reset
CP: Read 0078fda0 Write 0078fda0 Dist 00000000/00000000 (should be equal)

GP fifo: Size 1b
33 99 ff ff 00 00 00 00 3f 80 00 00 61 20 15 61 56 61 21 3d 53 35 61 40 00 00 17
Video: Read 001c8f99 Write 001c8fe0 Dist 00000047 (computed)
81 00 04 42 b6 1e ea 43 38 e3 4c 33 99 ff ff 00 00 00 00 00 00 00 00 42 ef b8 84 43 38 e3 4c 33 99 ff ff 3f 80 00 00 00 00 00 00 42 ef b8 84 43 5f 49 b2 33 99 ff ff 3f 80 00 00 3f 80 00 00 42 b6 1e ea 43 5f 49 b2
VideoCommon\CommandProcessor.cpp:98 W[CP]: Dumping fifo information: Reset after
CP: Read 0078fda0 Write 0078fda0 Dist 00000000/00000000 (should be equal)

GP fifo: Size 0

Video: Read 00000000 Write 00000000 Dist 00000000 (computed)

I still don't clear the CommandProcessor version of it (which is an intermediary between GPFifo.cpp and Fifo.cpp) as that has hardware registers that are exposed to games.

This change seems a bit dubious overall, as I feel like a hardware register wouldn't modify state internal to the CPU. The 750cl manual is vague about this too; it does say that "A mtspr to the WPAR establishes the gather address and resets the state of the facility, discarding any data in the buffer." (section 2.1.2.12/page 78), but doesn't say this in section 9.4.2 (page 327). There's also the HID2[WPE] bit, which enables/disables this functionality; the manual says "It is not expected that the HID2[WPE] bit is changed after it is initially configured. Once enabled, it is a programming error to dynamically disable this facility, as it can interfere with active write gathering operations." (section 2.1.2.4/page 66), but also says "The write gather pipe facility can be disabled by setting HID2[WPE] = 0. Before disabling the write gather pipe, the WPAR[BNE] bit should be tested to insure that all outstanding transfers from the buffer to the bus have completed." (section 9.4.2/page 328). However, GXAbortFrame does not modify either of these (they do get modified, but only well after the unknown opcode occurs), so something must happen that prevents this issue from occurring on real hardware. In any case, this fixes a regression in Dolphin.

I'm not sure what the XMM0 check was supposed to be, but the 0xCC008000 one is for the fifo and is handled elsewhere now (look for `optimizeGatherPipe`).
Copy link
Contributor

@iwubcode iwubcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good. Tested and initially Monster Lab came up with a fifo error that crashed Dolphin (portable mode, fresh install, so dual core was active) after that I tested again (multiple times) in both single/dual core and saw no issues. I imagine that single scenario was a timing fluke caused by dual core. I tested booting a number of other Wii titles. Can't recall where I've seen fifo errors crop up but the titles I tested were fine. Overall LGTM

@Pokechu22
Copy link
Contributor Author

This PR should only affect games that use PI_FIFO_RESET, which generally is used when resetting or when changing games on a multi-game disc (and a message is logged under ProcessorInterface when it's used). If the error appeared randomly during normal gameplay it's probably dual core instead.

@Pokechu22
Copy link
Contributor Author

One other example of where this comes up: the default libogc crash handler calls GX_AbortFrame. The hardware fifoplayer crashes if you don't have an SD card inserted. With the earlier implementation of PI_FIFO_RESET, you get a 0x66 unknown opcode because the reset doesn't clear everything:

VideoCommon\CommandProcessor.cpp:98 W[CP]: Dumping fifo information: Reset
CP: Read 0052cf60 Write 0052cf60 Dist 00000000/00000000 (should be equal)

GP fifo: Size 15
66 61 53 59 50 00 61 54 00 00 15 61 55 00 03 ff 61 56 00 03 ff
Video: Read 0000049c Write 000004a0 Dist 00000004 (computed)
61 04 66 66
 * GX_AbortFrame [ PC = 800284cc ]
 * GX_AbortFrame [ LR = 800287d0 ]
 * c_default_exceptionhandler [ addr = 80039668 ]
 *  ---  [ addr = 80005400 ]
 * mkdir [ addr = 800af720 ]
 * _Z15ReadStreamedDffiPFbvE [ addr = 80005a30 ]
 * main [ addr = 8000b9f8 ]
 * __crtmain [ addr = 80005980 ]
 * __lwp_thread_handler [ addr = 800212cc ]
 * __lwp_thread_handler [ addr = 80021278 ]
Control: GPREAD ON | BP OFF | Int OFF | OvF ON | UndF OFF | LINK ON
VideoCommon\CommandProcessor.cpp:98 W[CP]: Dumping fifo information: Reset after
CP: Read 0052cf60 Write 0052cf60 Dist 00000000/00000000 (should be equal)

GP fifo: Size 15
66 61 53 59 50 00 61 54 00 00 15 61 55 00 03 ff 61 56 00 03 ff
Video: Read 00000000 Write 00000000 Dist 00000000 (computed)

 * GX_AbortFrame [ PC = 800284cc ]
 * GX_AbortFrame [ LR = 800287d0 ]
 * c_default_exceptionhandler [ addr = 80039668 ]
 *  ---  [ addr = 80005400 ]
 * mkdir [ addr = 800af720 ]
 * _Z15ReadStreamedDffiPFbvE [ addr = 80005a30 ]
 * main [ addr = 8000b9f8 ]
 * __crtmain [ addr = 80005980 ]
 * __lwp_thread_handler [ addr = 800212cc ]
 * __lwp_thread_handler [ addr = 80021278 ]
Control: GPREAD ON | BP OFF | Int OFF | OvF ON | UndF OFF | LINK ON
Core\HW\ProcessorInterface.cpp:118 I[PI]: Wrote PI_FIFO_RESET: 00000000

With this PR, things behave correctly. (Also, the unknown opcode only happens if you don't have an SD card inserted and it crashes. Presumably any homebrew that crashes could run into a similar issue.)

@Tilka Tilka merged commit fb45ed3 into dolphin-emu:master Jul 31, 2022
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants