OpcodeDecoding: Don't raise panic alerts for unknown opcodes 0x01-0x07 #10389
Conversation
A pop-up is no longer generated for the Wiggler capsule in Mario Party 5 (https://bugs.dolphin-emu.org/issues/8104).
| if (static_cast<Opcode>(opcode) == Opcode::GX_UNKNOWN_RESET) | ||
| { | ||
| // Datel software uses this command | ||
| m_cycles += 6; |
There was a problem hiding this comment.
For datel, we don't jump 6 cycles anymore? Is this intentional? Does it break anything?
There was a problem hiding this comment.
I believe we've measured these as no-op on console
There was a problem hiding this comment.
I didn't do any timing measurement, but this results in command 0x01 (which seems to be invalid, and as far as I can tell is only accidentally used by datel, though 0x01 does also appear in the wiggler capsule vertex data) being treated the same as all other unknown commands (which are set to take 1 cycle, for some reason).
The no-op command (0x00) is currently set at 6 cycles, with a note that that may be too slow:
dolphin/Source/Core/VideoCommon/OpcodeDecoding.cpp
Lines 190 to 193 in 237947e
In any case, this change doesn't seem to impact datel titles (at least the ones I've tested), and all of these numbers are probably something that should be changed later if someone does better timing tests (I don't plan on doing them anytime soon).
There was a problem hiding this comment.
So, it's still 6 cycles regardless, gotcha.
There was a problem hiding this comment.
No, it does result in a difference (now it's 1 cycle while before it was 6 cycles), but it shouldn't affect anything other than situations that previously generated or should have generated an unknown opcode message.
There was a problem hiding this comment.
It is a behavior change (6 cycles -> 1 cycle) but I signed off based on the fact @Pokechu22 said they tested datel and didn't see any side effects.
| CommandProcessor::HandleUnknownOpcode(opcode, data, is_preprocess); | ||
| s_is_fifo_error_seen = true; | ||
| } | ||
|
|
||
| ERROR_LOG_FMT(VIDEO, "FIFO: Unknown Opcode({:#04x} @ {}, preprocessing = {})", opcode, |
There was a problem hiding this comment.
It does seem a little deceptive that this log is printed outside the 'handleunknownopcode' trigger.
There was a problem hiding this comment.
I'm assuming this was intentional
There was a problem hiding this comment.
It's intentional in that we always want the log to be printed, but I don't think there really is any reason we can't move the logging and s_is_fifo_error_seen into CommandProcessor.
…Processor That way, they're in the same place the panic alerts are generated.
|
Just so that these don't get lost, here are some additional high-res images from the hardware fifoplayer: https://imgur.com/a/DALcRjE |
A pop-up is no longer generated for the Wiggler capsule in Mario Party 5 (https://bugs.dolphin-emu.org/issues/8104).
Primitive data related to the unknown opcodes in Mario Party 5: https://gist.github.com/Pokechu22/83d5cd9443f3336f50bc5d626fb9636f
And some assorted images from the hardware fifo player: https://imgur.com/a/hx4qe42
Basically, the Wiggler capsule's animation draws a bunch of rings. Each ring has 128 vertices in it, with positions moving around a circle and texture coordinates going up by .03125 (1/32) for each horizontal step. The position, color, and texture coordinate data for each vertex is indexed into several arrays, with the first set of 4 vertices using indices 0, 1, 3, and 2. However, after all 128 vertices are written, the game writes data for 4 more vertices, again using indices 0, 1, 3, and 2. But since the game said it would only draw 128 vertices, not 132, those 4 vertices are instead interpreted as graphics commands. Opcode 0 is a NOP, and my brief hardware testing indicates that opcodes 1-7 are also treated as NOPs, so on real hardware these are ignored, but Dolphin didn't like that extra data. Now, that data is ignored (an error is logged, but no panic alert is generated).
If we modify the FIFO data to say that there are 132 vertices, then the first part of the ring is drawn twice, which indicates that vertices 129-132 normally go unused. (I also confirmed that nothing changes rendering-wise when modifying those vertices, and that the hardware FIFO player hangs if 0x18 is written into those vertices, where 0x18 is another unknown opcode; this indicates that not all unknown opcodes act like NOPs).
(As a side note, the ring also has a somewhat messed up texture (image). This is because the last quad re-uses indices from the first quad, which is fine for positions, but it means that the texture coordinates are 0.96875 and 0 instead of 0.96875 and 1, so the texture doesn't loop properly. Since the rings are normally pretty small and drawn on top of other stuff, and the Wiggler capsule is rare, this is hard to notice in normal gameplay. This could be theoretically fixed with a game patch.)
Regarding
GX_UNKNOWN_RESET, I can confirm that Datel software does use that command, but it looks like an accident (they write a 4-byte value of0x00000001, and that's from a variable that's changed in several places; they also trigger other unknown opcodes so ignoring only that one doesn't fix anything). I don't think it has any actual meaning that we need to handle.