ProcessorInterface: Implement PI_FIFO_RESET

[Pokechu22]

Fixes unknown opcodes 0x0f and 0xff when resetting titles, e.g. https://bugs.dolphin-emu.org/issues/12492.

I was able to consistently reproduce and test this by recording a movie with the GameCube Preview Disc. I eventually ended up getting lucky and reproducing the issue the first time around, giving a very short movie file that allowed for fast testing. I also hex-edited the movie to set it to dual core, and confirmed that the same issue happened there (and the movies synced with that change, surprisingly). PreviewDiscUnknownOpcode.zip

Here are some log files from reproducing the issue before and after this PR, with additional debug logging: OOT_old.log OOT_new.log Preview_old.log Preview_new.log (the preview ones are with PreviewDiscUnknownOpcodeShort.dtm; the OOT ones are two separate runs).

Changes I made to add more logging:

diff --git a/Source/Core/Core/HW/ProcessorInterface.cpp b/Source/Core/Core/HW/ProcessorInterface.cpp
index 1e49f6e2dc..0824f8f49c 100644
--- a/Source/Core/Core/HW/ProcessorInterface.cpp
+++ b/Source/Core/Core/HW/ProcessorInterface.cpp
@@ -18,6 +18,8 @@
 #include "Core/IOS/IOS.h"
 #include "Core/IOS/STM/STM.h"
 #include "Core/PowerPC/PowerPC.h"
+#include "VideoCommon/CommandProcessor.h"
+#include "VideoCommon/Fifo.h"
 
 namespace ProcessorInterface
 {
@@ -112,6 +114,12 @@ void RegisterMMIO(MMIO::Mapping* mmio, u32 base)
   mmio->Register(base | PI_FIFO_RESET, MMIO::InvalidRead<u32>(),
                  MMIO::ComplexWrite<u32>([](u32, u32 val) {
                    WARN_LOG_FMT(PROCESSORINTERFACE, "Fifo reset ({:08x})", val);
+                   CommandProcessor::DumpFifo("Reset");
+                   if (val != 0)
+                   {
+                     // Fifo::ResetVideoBuffer();
+                     // CommandProcessor::DumpFifo("Reset after");
+                   }
                  }));
 
   mmio->Register(base | PI_RESET_CODE, MMIO::ComplexRead<u32>([](u32) {
diff --git a/Source/Core/VideoCommon/CommandProcessor.cpp b/Source/Core/VideoCommon/CommandProcessor.cpp
index 890d7ebfa4..e8e56601fe 100644
--- a/Source/Core/VideoCommon/CommandProcessor.cpp
+++ b/Source/Core/VideoCommon/CommandProcessor.cpp
@@ -44,6 +44,30 @@ static Common::Flag s_interrupt_waiting;
 
 static bool s_is_fifo_error_seen = false;
 
+void DumpFifo(std::string_view context)
+{
+  WARN_LOG_FMT(COMMANDPROCESSOR, "Dumping fifo information: {}", context);
+  const u8* const read_ptr = Fifo::GetReadPtr();
+  const u32 read_ptr_addr = fifo.CPReadPointer.load(std::memory_order_relaxed);
+  const u8* const write_ptr = Fifo::GetWritePtr();
+  const u32 write_ptr_addr = fifo.CPWritePointer.load(std::memory_order_relaxed);
+  const u32 dist = fifo.CPReadWriteDistance.load(std::memory_order_relaxed);
+  WARN_LOG_FMT(COMMANDPROCESSOR, "Read pointer: {:08x} / {}", read_ptr_addr, fmt::ptr(read_ptr));
+  WARN_LOG_FMT(COMMANDPROCESSOR, "Write pointer: {:08x} / {}", write_ptr_addr, fmt::ptr(write_ptr));
+  WARN_LOG_FMT(COMMANDPROCESSOR, "Distance: {:x} / {:x} / {:x}", dist,
+               write_ptr_addr - read_ptr_addr, write_ptr - read_ptr);
+  WARN_LOG_FMT(COMMANDPROCESSOR, "Buffer: {:02x}",
+               fmt::join(read_ptr, std::min(read_ptr + 0x100, write_ptr), " "));
+  WARN_LOG_FMT(COMMANDPROCESSOR, "PC: {:08x}, LR: {:08x}", PC, LR);
+  WARN_LOG_FMT(COMMANDPROCESSOR, "Control: GPREAD {} | BP {} | Int {} | OvF {} | UndF {} | LINK {}",
+               fifo.bFF_GPReadEnable.load(std::memory_order_relaxed) ? "ON" : "OFF",
+               fifo.bFF_BPEnable.load(std::memory_order_relaxed) ? "ON" : "OFF",
+               fifo.bFF_BPInt.load(std::memory_order_relaxed) ? "ON" : "OFF",
+               m_CPCtrlReg.FifoOverflowIntEnable ? "ON" : "OFF",
+               m_CPCtrlReg.FifoUnderflowIntEnable ? "ON" : "OFF",
+               m_CPCtrlReg.GPLinkEnable ? "ON" : "OFF");
+}
+
 static bool IsOnThread()
 {
   return Core::System::GetInstance().IsDualCoreMode();
@@ -200,13 +224,28 @@ void RegisterMMIO(MMIO::Mapping* mmio, u32 base)
       {FIFO_LO_WATERMARK_HI, MMIO::Utils::HighPart(&fifo.CPLoWatermark), false, WMASK_HI_RESTRICT},
       // FIFO_RW_DISTANCE has some complex read code different for
       // single/dual core.
+      /*
       {FIFO_WRITE_POINTER_LO, MMIO::Utils::LowPart(&fifo.CPWritePointer), false,
        WMASK_LO_ALIGN_32BIT},
       {FIFO_WRITE_POINTER_HI, MMIO::Utils::HighPart(&fifo.CPWritePointer), false,
        WMASK_HI_RESTRICT},
+       */
       // FIFO_READ_POINTER has different code for single/dual core.
   };
 
+  mmio->Register(base | FIFO_WRITE_POINTER_LO,
+                 MMIO::DirectRead<u16>(MMIO::Utils::LowPart(&fifo.CPWritePointer)),
+                 MMIO::ComplexWrite<u16>([](u32, u16 val) {
+                   DumpFifo(fmt::format("Write {:04x} to WRITE_POINTER_LO", val));
+                   WriteLow(fifo.CPWritePointer, val & WMASK_LO_ALIGN_32BIT);
+                 }));
+  mmio->Register(base | FIFO_WRITE_POINTER_HI,
+                 MMIO::DirectRead<u16>(MMIO::Utils::HighPart(&fifo.CPWritePointer)),
+                 MMIO::ComplexWrite<u16>([WMASK_HI_RESTRICT](u32, u16 val) {
+                   DumpFifo(fmt::format("Write {:04x} to WRITE_POINTER_HI", val));
+                   WriteHigh(fifo.CPWritePointer, val & WMASK_HI_RESTRICT);
+                 }));
+
   for (auto& mapped_var : directly_mapped_vars)
   {
     mmio->Register(base | mapped_var.addr, MMIO::DirectRead<u16>(mapped_var.ptr),
@@ -296,8 +335,10 @@ void RegisterMMIO(MMIO::Mapping* mmio, u32 base)
         }
       }) :
                      MMIO::DirectRead<u16>(MMIO::Utils::LowPart(&fifo.CPReadWriteDistance)),
-      MMIO::DirectWrite<u16>(MMIO::Utils::LowPart(&fifo.CPReadWriteDistance),
-                             WMASK_LO_ALIGN_32BIT));
+      MMIO::ComplexWrite<u16>([](u32, u16 val) {
+        DumpFifo(fmt::format("Write {:04x} to RW_DISTANCE_LO", val));
+        WriteLow(fifo.CPReadWriteDistance, val & WMASK_LO_ALIGN_32BIT);
+      }));
   mmio->Register(base | FIFO_RW_DISTANCE_HI,
                  IsOnThread() ?
                      MMIO::ComplexRead<u16>([](u32) {
@@ -323,15 +364,19 @@ void RegisterMMIO(MMIO::Mapping* mmio, u32 base)
                        return fifo.CPReadWriteDistance.load(std::memory_order_relaxed) >> 16;
                      }),
                  MMIO::ComplexWrite<u16>([WMASK_HI_RESTRICT](u32, u16 val) {
+                   DumpFifo(fmt::format("Write {:04x} to RW_DISTANCE_HI", val));
                    Fifo::SyncGPUForRegisterAccess();
                    WriteHigh(fifo.CPReadWriteDistance, val & WMASK_HI_RESTRICT);
                    Fifo::RunGpu();
                  }));
-  mmio->Register(
-      base | FIFO_READ_POINTER_LO,
-      IsOnThread() ? MMIO::DirectRead<u16>(MMIO::Utils::LowPart(&fifo.SafeCPReadPointer)) :
+  mmio->Register(base | FIFO_READ_POINTER_LO,
+                 IsOnThread() ?
+                     MMIO::DirectRead<u16>(MMIO::Utils::LowPart(&fifo.SafeCPReadPointer)) :
                      MMIO::DirectRead<u16>(MMIO::Utils::LowPart(&fifo.CPReadPointer)),
-      MMIO::DirectWrite<u16>(MMIO::Utils::LowPart(&fifo.CPReadPointer), WMASK_LO_ALIGN_32BIT));
+                 MMIO::ComplexWrite<u16>([](u32, u16 val) {
+                   DumpFifo(fmt::format("Write {:04x} to READ_POINTER_LO", val));
+                   WriteLow(fifo.CPReadPointer, val & WMASK_LO_ALIGN_32BIT);
+                 }));
   mmio->Register(base | FIFO_READ_POINTER_HI,
                  IsOnThread() ? MMIO::ComplexRead<u16>([](u32) {
                    Fifo::SyncGPUForRegisterAccess();
@@ -348,6 +393,7 @@ void RegisterMMIO(MMIO::Mapping* mmio, u32 base)
                                                 std::memory_order_relaxed);
                  }) :
                                 MMIO::ComplexWrite<u16>([WMASK_HI_RESTRICT](u32, u16 val) {
+                                  DumpFifo(fmt::format("Write {:04x} to READ_POINTER_HI", val));
                                   Fifo::SyncGPUForRegisterAccess();
                                   WriteHigh(fifo.CPReadPointer, val & WMASK_HI_RESTRICT);
                                 }));
@@ -599,6 +645,7 @@ void SetCpControlRegister()
     fifo.bFF_GPReadEnable = m_CPCtrlReg.GPReadEnable;
   }
 
+  DumpFifo("CP control reg");
   DEBUG_LOG_FMT(COMMANDPROCESSOR, "\t GPREAD {} | BP {} | Int {} | OvF {} | UndF {} | LINK {}",
                 fifo.bFF_GPReadEnable.load(std::memory_order_relaxed) ? "ON" : "OFF",
                 fifo.bFF_BPEnable.load(std::memory_order_relaxed) ? "ON" : "OFF",
@@ -662,6 +709,7 @@ void HandleUnknownOpcode(u8 cmd_byte, const u8* buffer, bool preprocess)
       fifo.bFF_GPLinkEnable.load(std::memory_order_relaxed) ? "true" : "false",
       fifo.bFF_HiWatermarkInt.load(std::memory_order_relaxed) ? "true" : "false",
       fifo.bFF_LoWatermarkInt.load(std::memory_order_relaxed) ? "true" : "false", PC, LR);
+  DumpFifo("Unknown opcode");
 
   if (!s_is_fifo_error_seen && !suppress_panic_alert)
   {
diff --git a/Source/Core/VideoCommon/CommandProcessor.h b/Source/Core/VideoCommon/CommandProcessor.h
index ca2b6fbb28..7cc269bb76 100644
--- a/Source/Core/VideoCommon/CommandProcessor.h
+++ b/Source/Core/VideoCommon/CommandProcessor.h
@@ -4,6 +4,7 @@
 #pragma once
 
 #include <atomic>
+#include <string_view>
 
 #include "Common/CommonTypes.h"
 
@@ -172,4 +173,5 @@ void HandleUnknownOpcode(u8 cmd_byte, const u8* buffer, bool preprocess);
 
 u32 GetPhysicalAddressMask();
 
+void DumpFifo(std::string_view context);
 }  // namespace CommandProcessor
diff --git a/Source/Core/VideoCommon/Fifo.cpp b/Source/Core/VideoCommon/Fifo.cpp
index ccee18f40e..89b6a8b385 100644
--- a/Source/Core/VideoCommon/Fifo.cpp
+++ b/Source/Core/VideoCommon/Fifo.cpp
@@ -628,4 +628,13 @@ void Prepare()
   s_event_sync_gpu = CoreTiming::RegisterEvent("SyncGPUCallback", SyncGPUCallback);
   s_syncing_suspended = true;
 }
+
+u8* GetReadPtr()
+{
+  return s_video_buffer_read_ptr;
+}
+u8* GetWritePtr()
+{
+  return s_video_buffer_write_ptr.load(std::memory_order_relaxed);
+}
 }  // namespace Fifo
diff --git a/Source/Core/VideoCommon/Fifo.h b/Source/Core/VideoCommon/Fifo.h
index 6b640bf7ca..5ad949eb49 100644
--- a/Source/Core/VideoCommon/Fifo.h
+++ b/Source/Core/VideoCommon/Fifo.h
@@ -48,4 +48,7 @@ void EmulatorState(bool running);
 bool AtBreakpoint();
 void ResetVideoBuffer();
 
+// Only for testing
+u8* GetReadPtr();
+u8* GetWritePtr();
 }  // namespace Fifo

I had noticed that extra data was present in the FIFO when FIFO_RW_DISTANCE_HI was present, and while adding logging to try to figure out when it first appeared, I spotted the existing warning message about PI_FIFO_RESET and figured it might be relevant.

Basically, when resetting or changing games, the existing contents of the fifo don't matter since it's not going to be drawn anymore. GXAbortFrame (or something like that) is called to reset the contents of the fifo, which it does by writing 1 to PI_FIFO_RESET, waiting a bit, and then writing 0 to PI_FIFO_RESET. Then, after the new game is loaded (or the existing game is reloaded), which might be a few seconds later, GXInit is called, which sets up the fifo again and then writes some initialization stuff into it. The first data it writes is from __GX_FlushTextureState, which contains a BPMEM_IND_IMASK command (I don't actually know what that one does and Dolphin doesn't implement it, but that doesn't matter). That hex data for that command is 61 0f 0000ff. Other commands follow afterwards: BPMEM_BUSCLOCK1 (0x69) and BPMEM_BUSCLOCK0 (0x46).

We weren't handling PI_FIFO_RESET, so the contents of the FIFO weren't cleared. In many cases that won't be a problem, as if there are unprocessed graphics commands in there, it doesn't matter if we process it or not (to my understanding, only the FIFO is being reset, not the actual graphics state; the game will need to reconfigure everything from a previously-unknown state anyways). But graphics commands are done using multiple writes; for instance, BP commands are usually done by first writing the byte 0x61 and then doing a 4-byte write corresponding to the BP register and the 3-byte value (but they can also be done using 4 separate single-byte writes). If the reset happens at that point, and the game then stops what it was doing, the command will be partially written. Then, when later commands are written during the initialization process, they won't decode properly. For instance, if the game started writing a BP command before it got reset, and had written 61, then when 61 0f 0000ff is written the whole stream will instead be read as 61 61 0f0000 ff: a BP command writing to BP register 61 (BPMEM_PRELOAD_TMEMEVEN) with a value of 0f0000, followed by ff which is an unknown opcode. And similarly, if it had written the 4 of the 5 bytes in a BP command, for instance 61 4e 00 01, then the whole stream will be parsed as 61 4e 000161 0f 00 00 ff, a write of value 000161 to BPMEM_COPYYSCALE, followed by 0f which is an unknown opcode, followed by 00 which is treated as NOP and ignored (twice), followed by ff which is also an unknown opcode. Handling PI_FIFO_RESET prevents this entirely.

The old approach prior to #8090 worked because part of the fifo-initializing process is writing to FIFO_RW_DISTANCE_HI. But, that happens several seconds later, and (presumably, I haven't actually confirmed this) games might write to FIFO_RW_DISTANCE_HI in other cases (and also, we were only resetting when writing to FIFO_RW_DISTANCE_HI, not FIFO_RW_DISTANCE_LO, which just seems odd).

I haven't performed any hardware testing here yet, but this behavior seems fairly plausible.

[shuffle2]

probably only a single bit?

[Pokechu22]

Changed. My attempt at hardware-testing found that writing 2 seemed to hang everything, though I also ran into other issues (after calling GX_AbortFrame other drawing commands stopped working, but I might be doing something wrong)

[Pokechu22]

It seems like this was actually implemented in the past, but the implementation was eventually lost:

• Added in 3b7a6ce
• Changed in f0adc3a
• Changed in 8627864
• Commented out in 05feabd
• Commented-out contents lost in c7ae0eb
• Commented call lost in a3f95c1
• Remainder removed in 0aecd95

[leoetlino]

Sounds reasonable but it'd be nice to figure out if it's really correct to ignore everything but the LSB.

[shuffle2]

if we only know + implement what bit 0 does, but log all bits, seems ok?

[Pokechu22]

For what it's worth, here's my attempted hardware test - build with ./build.sh, and run with ./run.sh build/gxtest/gxtest_fifo.elf (possibly setting WIILOAD beforehand). But this test fails on hardware (giving wrong results when using GX_AbortFrame, and hanging when directly writing to PI_FIFO_RESET), and in Dolphin with this PR the test produces several unknown opcodes. So I think something's wrong (both with the test and maybe also with this implementation).

[AdmiralCurtiss]

What's the state of this, should we merge this?

[shuffle2]

For what it's worth, here's my attempted hardware test - build with ./build.sh, and run with ./run.sh build/gxtest/gxtest_fifo.elf (possibly setting WIILOAD beforehand). But this test fails on hardware (giving wrong results when using GX_AbortFrame, and hanging when directly writing to PI_FIFO_RESET), and in Dolphin with this PR the test produces several unknown opcodes. So I think something's wrong (both with the test and maybe also with this implementation).

Did you try dumping the register before modifying it / just modifying the low bit? Maybe you accidentally change other bits (but it’s unlikely I guess)

also, it looks like libogc has extra code in this path on wii hardware which waits for pixel engine memory access to stop (__GX_WaitAbortPixelEngine) maybe this impacts your test.

[Pokechu22]

What's the state of this, should we merge this?

I'm not confident that this is correct currently. It fixes some things, but I'm unsure about both the implementation and the hardware behavior it's implementing. So it shouldn't be merged currently.

Did you try dumping the register before modifying it / just modifying the low bit? Maybe you accidentally change other bits (but it’s unlikely I guess)

I don't remember if I did this. However, Libogc's GX_AbortFrame, as well as the GXAbortFrame in the Activision Demo Disc, directly write 1 and 0 to it.

also, it looks like libogc has extra code in this path on wii hardware which waits for pixel engine memory access to stop (__GX_WaitAbortPixelEngine) maybe this impacts your test.

I did try that (by calling GX_AbortFrame directly), and the result was that nothing useful happened.

GXAbortFrame seems to almost exactly match GX_AbortFrame except that instead of calling GXFlush() (which checks __gx.dirtyState and writes values if needed before writing 32 NOPs), it sets dirtyState to 0 and writes 32 NOPs immediately without writing dirty state. So it's possible that libogc has a mistake in it. Unfortunately it's annoying to rewrite GX_AbortFrame because it messes with the FIFO and the relevant functions aren't public.

[dolphin-emu-bot]

FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:

• taiko-depth on sw-lin-mesa: diff

_{^{automated-fifoci-reporter}}

[JMC47]

This isn't getting much attention in a while, and there are speedruns that rely on reset, along with things like the VC games that randomly break on this. I think it's a fairly safe thing to do.