From 76fd0479b8a5e6b4dd883d8b3d3436c3e30ce26f Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Wed, 20 Jul 2022 16:38:54 -0700 Subject: [PATCH 1/6] JitAsm: Remove old commented-out code 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`). --- Source/Core/Core/PowerPC/Jit64/JitAsm.cpp | 17 ----------------- 1 file changed, 17 deletions(-) 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);*/ } From b76f4dd5f81e786bc68150369e377d040aed12d3 Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Wed, 20 Jul 2022 17:27:37 -0700 Subject: [PATCH 2/6] ProcessorInterface: Remove unused fields --- Source/Core/Core/HW/ProcessorInterface.cpp | 19 +++++-------------- Source/Core/Core/State.cpp | 2 +- 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/Source/Core/Core/HW/ProcessorInterface.cpp b/Source/Core/Core/HW/ProcessorInterface.cpp index 795a169205..bc0321c2dd 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; @@ -131,7 +122,7 @@ void RegisterMMIO(MMIO::Mapping* mmio, u32 base) } })); - mmio->Register(base | PI_FLIPPER_REV, MMIO::DirectRead(&m_FlipperRev), + mmio->Register(base | PI_FLIPPER_REV, MMIO::Constant(FLIPPER_REV_C), MMIO::InvalidWrite()); // 16 bit reads are based on 32 bit reads. 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, From 1c833ddc3c6c58c18187ae87fb43c62e1c517dc8 Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Wed, 20 Jul 2022 17:44:49 -0700 Subject: [PATCH 3/6] Create constant for GPFifo physical address --- Source/Core/Core/HW/GPFifo.h | 4 ++++ Source/Core/Core/HW/MMIO.h | 3 ++- .../Core/PowerPC/Interpreter/Interpreter_SystemRegisters.cpp | 3 ++- Source/Core/Core/PowerPC/MMU.cpp | 5 +++-- Source/UnitTests/Core/MMIOTest.cpp | 5 +++-- 5 files changed, 14 insertions(+), 6 deletions(-) diff --git a/Source/Core/Core/HW/GPFifo.h b/Source/Core/Core/HW/GPFifo.h index 2925e82c05..b29b1b9fde 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; 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/PowerPC/Interpreter/Interpreter_SystemRegisters.cpp b/Source/Core/Core/PowerPC/Interpreter/Interpreter_SystemRegisters.cpp index 04b305ce37..b7da980db3 100644 --- a/Source/Core/Core/PowerPC/Interpreter/Interpreter_SystemRegisters.cpp +++ b/Source/Core/Core/PowerPC/Interpreter/Interpreter_SystemRegisters.cpp @@ -341,7 +341,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/MMU.cpp b/Source/Core/Core/PowerPC/MMU.cpp index 6c0867deb6..0ff7ccd88e 100644 --- a/Source/Core/Core/PowerPC/MMU.cpp +++ b/Source/Core/Core/PowerPC/MMU.cpp @@ -285,7 +285,8 @@ static void WriteToHardware(u32 em_address, const u32 data, const u32 size) // 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) + if (flag == XCheckTLBFlag::Write && + (em_address & 0xFFFFF000) == GPFifo::GATHER_PIPE_PHYSICAL_ADDRESS) { switch (size) { @@ -1086,7 +1087,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/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 From c06f203e9845d0ab08af2d32a8314062e26d9de3 Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Thu, 21 Jul 2022 14:18:35 -0700 Subject: [PATCH 4/6] MMU: Clarify masking on gather pipe address --- Source/Core/Core/PowerPC/MMU.cpp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/Source/Core/Core/PowerPC/MMU.cpp b/Source/Core/Core/PowerPC/MMU.cpp index 0ff7ccd88e..3cd9bde984 100644 --- a/Source/Core/Core/PowerPC/MMU.cpp +++ b/Source/Core/Core/PowerPC/MMU.cpp @@ -283,8 +283,15 @@ static void WriteToHardware(u32 em_address, const u32 data, const u32 size) } // 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. + // + // 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) + // + // Note that 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) { From 97412553f9e888f4bd8c4f01c240437fca696eab Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Thu, 21 Jul 2022 15:20:31 -0700 Subject: [PATCH 5/6] Add a more detailed comment about SPR_WPAR's BNE bit --- Source/Core/Core/HW/GPFifo.cpp | 18 ++++++++++++++++-- Source/Core/Core/HW/GPFifo.h | 2 +- .../Interpreter_SystemRegisters.cpp | 13 ++++++++----- Source/Core/Core/PowerPC/MMU.cpp | 12 ++++++------ 4 files changed, 31 insertions(+), 14 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() +bool IsBNE() { - return GetGatherPipeCount() == 0; + // 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 b29b1b9fde..4ba023117f 100644 --- a/Source/Core/Core/HW/GPFifo.h +++ b/Source/Core/Core/HW/GPFifo.h @@ -26,7 +26,7 @@ void UpdateGatherPipe(); void CheckGatherPipe(); void FastCheckGatherPipe(); -bool IsEmpty(); +bool IsBNE(); // Write void Write8(u8 value); diff --git a/Source/Core/Core/PowerPC/Interpreter/Interpreter_SystemRegisters.cpp b/Source/Core/Core/PowerPC/Interpreter/Interpreter_SystemRegisters.cpp index b7da980db3..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; } diff --git a/Source/Core/Core/PowerPC/MMU.cpp b/Source/Core/Core/PowerPC/MMU.cpp index 3cd9bde984..301ff55a6b 100644 --- a/Source/Core/Core/PowerPC/MMU.cpp +++ b/Source/Core/Core/PowerPC/MMU.cpp @@ -282,16 +282,16 @@ static void WriteToHardware(u32 em_address, const u32 data, const u32 size) wi = translated_addr.wi; } - // Check for a gather pipe write. + // 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) // - // Note that 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. + // 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) { From 5bbdf7ae154a5da03d66c38855f2b1890a2a3dc8 Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Fri, 22 Jul 2022 15:07:19 -0700 Subject: [PATCH 6/6] ProcessorInterface: Reset both GPFifo and Fifo on PI_FIFO_RESET Fixes https://bugs.dolphin-emu.org/issues/12981 --- Source/Core/Core/HW/ProcessorInterface.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Source/Core/Core/HW/ProcessorInterface.cpp b/Source/Core/Core/HW/ProcessorInterface.cpp index bc0321c2dd..5e752c13f7 100644 --- a/Source/Core/Core/HW/ProcessorInterface.cpp +++ b/Source/Core/Core/HW/ProcessorInterface.cpp @@ -106,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) {