Merge pull request #10883 from Pokechu22/pi-fifo-reset-gp-fifo

ProcessorInterface: Reset both GPFifo and Fifo on PI_FIFO_RESET

9 files changed, 59 insertions, 50 deletions

diff --git a/Source/Core/Core/HW/GPFifo.cpp b/Source/Core/Core/HW/GPFifo.cpp
index ef05946c2b..4de989b05b 100644
--- a/Source/Core/Core/HW/GPFifo.cpp
+++ b/Source/Core/Core/HW/GPFifo.cpp
@@ -56,9 +56,23 @@ void Init()
   memset(s_gather_pipe, 0, sizeof(s_gather_pipe));
 }
 
-bool IsEmpty()
-{
-  return GetGatherPipeCount() == 0;
+bool IsBNE()
+{
+  // TODO: It's not clear exactly when the BNE (buffer not empty) bit is set.
+  // The PPC 750cl manual says in section 2.1.2.12 "Write Pipe Address Register (WPAR)" (page 78):
+  // "A mfspr WPAR is used to read the BNE bit to check for any outstanding data transfers."
+  // In section 9.4.2 "Write Gather Pipe Operation" (page 327) it says:
+  // "Software can check WPAR[BNE] to determine if the buffer is empty or not."
+  // On page 327, it also says "The only way for software to flush out a partially full 32 byte
+  // block is to fill up the block with dummy data,."
+  // On page 328, it says: "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."
+  //
+  // GXRedirectWriteGatherPipe and GXRestoreWriteGatherPipe (used for display lists) wait for
+  // the bit to be 0 before continuing, so it can't be a case of any data existing in the FIFO;
+  // it might be a case of over 32 bytes being stored pending transfer to memory? For now, always
+  // return false since that prevents hangs in games that use display lists.
+  return false;
 }
 
 void ResetGatherPipe()
diff --git a/Source/Core/Core/HW/GPFifo.h b/Source/Core/Core/HW/GPFifo.h
index 2925e82c05..4ba023117f 100644
--- a/Source/Core/Core/HW/GPFifo.h
+++ b/Source/Core/Core/HW/GPFifo.h
@@ -9,6 +9,10 @@ class PointerWrap;
 
 namespace GPFifo
 {
+// This address is configurable in the WPAR SPR, but all games put it at this address
+// (and presumably the hardware backing this system uses this address).
+constexpr u32 GATHER_PIPE_PHYSICAL_ADDRESS = 0x0C008000;
+
 constexpr u32 GATHER_PIPE_SIZE = 32;
 constexpr u32 GATHER_PIPE_EXTRA_SIZE = GATHER_PIPE_SIZE * 16;
 
@@ -22,7 +26,7 @@ void UpdateGatherPipe();
 void CheckGatherPipe();
 void FastCheckGatherPipe();
 
-bool IsEmpty();
+bool IsBNE();
 
 // Write
 void Write8(u8 value);
diff --git a/Source/Core/Core/HW/MMIO.h b/Source/Core/Core/HW/MMIO.h
index d1be58bcfe..92c35bc5f3 100644
--- a/Source/Core/Core/HW/MMIO.h
+++ b/Source/Core/Core/HW/MMIO.h
@@ -13,6 +13,7 @@
 #include "Common/BitUtils.h"
 #include "Common/CommonTypes.h"
 #include "Core/ConfigManager.h"
+#include "Core/HW/GPFifo.h"
 #include "Core/HW/MMIOHandlers.h"
 
 namespace MMIO
@@ -43,7 +44,7 @@ const u32 NUM_MMIOS = NUM_BLOCKS * BLOCK_SIZE;
 // interface.
 inline bool IsMMIOAddress(u32 address)
 {
-  if (address == 0x0C008000)
+  if (address == GPFifo::GATHER_PIPE_PHYSICAL_ADDRESS)
     return false;  // WG Pipe
   if ((address & 0xFFFF0000) == 0x0C000000)
     return true;  // GameCube MMIOs
diff --git a/Source/Core/Core/HW/ProcessorInterface.cpp b/Source/Core/Core/HW/ProcessorInterface.cpp
index 795a169205..5e752c13f7 100644
--- a/Source/Core/Core/HW/ProcessorInterface.cpp
+++ b/Source/Core/Core/HW/ProcessorInterface.cpp
@@ -22,6 +22,10 @@
 
 namespace ProcessorInterface
 {
+constexpr u32 FLIPPER_REV_A = 0x046500B0;
+constexpr u32 FLIPPER_REV_B = 0x146500B1;
+constexpr u32 FLIPPER_REV_C = 0x246500B1;
+
 // STATE_TO_SAVE
 u32 m_InterruptCause;
 u32 m_InterruptMask;
@@ -30,10 +34,7 @@ u32 Fifo_CPUBase;
 u32 Fifo_CPUEnd;
 u32 Fifo_CPUWritePointer;
 
-static u32 m_Fifo_Reset;
 static u32 m_ResetCode;
-static u32 m_FlipperRev;
-static u32 m_Unknown;
 
 // ID and callback for scheduling reset button presses/releases
 static CoreTiming::EventType* toggleResetButton;
@@ -55,10 +56,7 @@ void DoState(PointerWrap& p)
   p.Do(Fifo_CPUBase);
   p.Do(Fifo_CPUEnd);
   p.Do(Fifo_CPUWritePointer);
-  p.Do(m_Fifo_Reset);
   p.Do(m_ResetCode);
-  p.Do(m_FlipperRev);
-  p.Do(m_Unknown);
 }
 
 void Init()
@@ -69,13 +67,6 @@ void Init()
   Fifo_CPUBase = 0;
   Fifo_CPUEnd = 0;
   Fifo_CPUWritePointer = 0;
-  /*
-  Previous Flipper IDs:
-  0x046500B0 = A
-  0x146500B1 = B
-  */
-  m_FlipperRev = 0x246500B1;  // revision C
-  m_Unknown = 0;
 
   m_ResetCode = 0;  // Cold reset
   m_InterruptCause = INT_CAUSE_RST_BUTTON | INT_CAUSE_VI;
@@ -115,7 +106,10 @@ void RegisterMMIO(MMIO::Mapping* mmio, u32 base)
                    // Used by GXAbortFrame
                    INFO_LOG_FMT(PROCESSORINTERFACE, "Wrote PI_FIFO_RESET: {:08x}", val);
                    if ((val & 1) != 0)
+                   {
+                     GPFifo::ResetGatherPipe();
                      Fifo::ResetVideoBuffer();
+                   }
                  }));
 
   mmio->Register(base | PI_RESET_CODE, MMIO::ComplexRead<u32>([](u32) {
@@ -131,7 +125,7 @@ void RegisterMMIO(MMIO::Mapping* mmio, u32 base)
                    }
                  }));
 
-  mmio->Register(base | PI_FLIPPER_REV, MMIO::DirectRead<u32>(&m_FlipperRev),
+  mmio->Register(base | PI_FLIPPER_REV, MMIO::Constant<u32>(FLIPPER_REV_C),
                  MMIO::InvalidWrite<u32>());
 
   // 16 bit reads are based on 32 bit reads.
diff --git a/Source/Core/Core/PowerPC/Interpreter/Interpreter_SystemRegisters.cpp b/Source/Core/Core/PowerPC/Interpreter/Interpreter_SystemRegisters.cpp
index 04b305ce37..8699f05b75 100644
--- a/Source/Core/Core/PowerPC/Interpreter/Interpreter_SystemRegisters.cpp
+++ b/Source/Core/Core/PowerPC/Interpreter/Interpreter_SystemRegisters.cpp
@@ -238,11 +238,14 @@ void Interpreter::mfspr(UGeckoInstruction inst)
 
   case SPR_WPAR:
   {
-    // TODO: If wpar_empty ever is false, Paper Mario hangs. Strange.
-    // Maybe WPAR is automatically flushed after a certain amount of time?
-    bool wpar_empty = true;  // GPFifo::IsEmpty();
-    if (!wpar_empty)
-      rSPR(index) |= 1;  // BNE = buffer not empty
+    // The bottom, read-only bit checks if the buffer is not empty.
+    // GXRedirectWriteGatherPipe and GXRestoreWriteGatherPipe (used for display lists) wait for
+    // this bit to be cleared before writing to SPR_WPAR again (with a value of 0x0c00800 (aka
+    // GPFifo::GATHER_PIPE_PHYSICAL_ADDRESS)).
+    // Currently, we always treat the buffer as not empty, as the exact behavior is unclear
+    // (and games that use display lists will hang if the bit doesn't eventually become zero).
+    if (GPFifo::IsBNE())
+      rSPR(index) |= 1;
     else
       rSPR(index) &= ~1;
   }
@@ -341,7 +344,8 @@ void Interpreter::mtspr(UGeckoInstruction inst)
     break;
 
   case SPR_WPAR:
-    ASSERT_MSG(POWERPC, rGPR[inst.RD] == 0x0C008000, "Gather pipe @ {:08x}", PC);
+    ASSERT_MSG(POWERPC, rSPR(SPR_WPAR) == GPFifo::GATHER_PIPE_PHYSICAL_ADDRESS,
+               "Gather pipe changed to unexpected address {:08x} @ PC {:08x}", rSPR(SPR_WPAR), PC);
     GPFifo::ResetGatherPipe();
     break;
 
diff --git a/Source/Core/Core/PowerPC/Jit64/JitAsm.cpp b/Source/Core/Core/PowerPC/Jit64/JitAsm.cpp
index b1dd505c34..1fbdc8306e 100644
--- a/Source/Core/Core/PowerPC/Jit64/JitAsm.cpp
+++ b/Source/Core/Core/PowerPC/Jit64/JitAsm.cpp
@@ -246,21 +246,4 @@ void Jit64AsmRoutineManager::GenerateCommon()
   GenQuantizedSingleLoads();
   GenQuantizedStores();
   GenQuantizedSingleStores();
-
-  // CMPSD(R(XMM0), M(&zero),
-  // TODO
-
-  // Fast write routines - special case the most common hardware write
-  // TODO: use this.
-  // Even in x86, the param values will be in the right registers.
-  /*
-  const u8 *fastMemWrite8 = AlignCode16();
-  CMP(32, R(ABI_PARAM2), Imm32(0xCC008000));
-  FixupBranch skip_fast_write = J_CC(CC_NE, false);
-  MOV(32, RSCRATCH, M(&m_gatherPipeCount));
-  MOV(8, MDisp(RSCRATCH, (u32)&m_gatherPipe), ABI_PARAM1);
-  ADD(32, 1, M(&m_gatherPipeCount));
-  RET();
-  SetJumpTarget(skip_fast_write);
-  CALL((void *)&PowerPC::Write_U8);*/
 }
diff --git a/Source/Core/Core/PowerPC/MMU.cpp b/Source/Core/Core/PowerPC/MMU.cpp
index 6c0867deb6..301ff55a6b 100644
--- a/Source/Core/Core/PowerPC/MMU.cpp
+++ b/Source/Core/Core/PowerPC/MMU.cpp
@@ -282,10 +282,18 @@ static void WriteToHardware(u32 em_address, const u32 data, const u32 size)
     wi = translated_addr.wi;
   }
 
-  // Check for a gather pipe write.
-  // Note that we must mask the address to correctly emulate certain games;
-  // Pac-Man World 3 in particular is affected by this.
-  if (flag == XCheckTLBFlag::Write && (em_address & 0xFFFFF000) == 0x0C008000)
+  // Check for a gather pipe write (which are not implemented through the MMIO system).
+  //
+  // Note that we must mask the address to correctly emulate certain games; Pac-Man World 3
+  // in particular is affected by this. (See https://bugs.dolphin-emu.org/issues/8386)
+  //
+  // The PowerPC 750CL manual says (in section 9.4.2 Write Gather Pipe Operation on page 327):
+  // "A noncacheable store to an address with bits 0-26 matching WPAR[GB_ADDR] but with bits 27-31
+  // not all zero will result in incorrect data in the buffer." So, it's possible that in some cases
+  // writes which do not exactly match the masking behave differently, but Pac-Man World 3's writes
+  // happen to behave correctly.
+  if (flag == XCheckTLBFlag::Write &&
+      (em_address & 0xFFFFF000) == GPFifo::GATHER_PIPE_PHYSICAL_ADDRESS)
   {
     switch (size)
     {
@@ -1086,7 +1094,7 @@ bool IsOptimizableGatherPipeWrite(u32 address)
     return false;
 
   // Check whether the translated address equals the address in WPAR.
-  return address == 0x0C008000;
+  return address == GPFifo::GATHER_PIPE_PHYSICAL_ADDRESS;
 }
 
 TranslateResult JitCache_TranslateAddress(u32 address)
diff --git a/Source/Core/Core/State.cpp b/Source/Core/Core/State.cpp
index bccb56a835..c0ab42a64b 100644
--- a/Source/Core/Core/State.cpp
+++ b/Source/Core/Core/State.cpp
@@ -74,7 +74,7 @@ static std::recursive_mutex g_save_thread_mutex;
 static std::thread g_save_thread;
 
 // Don't forget to increase this after doing changes on the savestate system
-constexpr u32 STATE_VERSION = 145;  // Last changed in PR 10879
+constexpr u32 STATE_VERSION = 146;  // Last changed in PR 10883
 
 // Maps savestate versions to Dolphin versions.
 // Versions after 42 don't need to be added to this list,
diff --git a/Source/UnitTests/Core/MMIOTest.cpp b/Source/UnitTests/Core/MMIOTest.cpp
index 480ba7f7b4..e10ee4e0ce 100644
--- a/Source/UnitTests/Core/MMIOTest.cpp
+++ b/Source/UnitTests/Core/MMIOTest.cpp
@@ -8,6 +8,7 @@
 #include "Common/CommonTypes.h"
 #include "Common/Config/Config.h"
 #include "Common/FileUtil.h"
+#include "Core/HW/GPFifo.h"
 #include "Core/HW/MMIO.h"
 #include "UICommon/UICommon.h"
 
@@ -40,7 +41,7 @@ TEST(IsMMIOAddress, SpecialAddresses)
   SConfig::GetInstance().bWii = true;
 
   // WG Pipe address, should not be handled by MMIO.
-  EXPECT_FALSE(MMIO::IsMMIOAddress(0x0C008000));
+  EXPECT_FALSE(MMIO::IsMMIOAddress(GPFifo::GATHER_PIPE_PHYSICAL_ADDRESS));
 
   // Locked L1 cache allocation.
   EXPECT_FALSE(MMIO::IsMMIOAddress(0xE0000000));
@@ -52,7 +53,7 @@ TEST(IsMMIOAddress, SpecialAddresses)
   // addresses.
   EXPECT_FALSE(MMIO::IsMMIOAddress(0xCC0000E0));
 
-  // And lets check some valid addresses too
+  // And let's check some valid addresses too
   EXPECT_TRUE(MMIO::IsMMIOAddress(0x0C0000E0));  // GameCube MMIOs
   EXPECT_TRUE(MMIO::IsMMIOAddress(0x0D00008C));  // Wii MMIOs
   EXPECT_TRUE(MMIO::IsMMIOAddress(0x0D800F10));  // Mirror of Wii MMIOs