Core: Threadsafety Synchronization Fixes (Frame Advance / FifoPlayer)

Fix Frame Advance and FifoPlayer pause/unpause/stop.

CPU::EnableStepping is not atomic but is called from multiple threads
which races and leaves the system in a random state; also instruction
stepping was unstable, m_StepEvent had an almost random value because
of the dual purpose it served which could cause races where CPU::Run
would SingleStep when it was supposed to be sleeping.

FifoPlayer never FinishStateMove()d which was causing it to deadlock.
Rather than partially reimplementing CPU::Run, just use CPUCoreBase
and then call CPU::Run(). More DRY and less likely to have weird bugs
specific to the player (i.e the previous freezing on pause/stop).

Refactor PowerPC::state into CPU since it manages the state of the
CPU Thread which is controlled by CPU, not PowerPC. This simplifies
the architecture somewhat and eliminates races that can be caused by
calling PowerPC state functions directly instead of using CPU's
(because they bypassed the EnableStepping lock).

Source/Core/Core/FifoPlayer/FifoPlayer.cpp

@@ -7,12 +7,14 @@
 
 #include "Common/Assert.h"
 #include "Common/CommonTypes.h"
+#include "Common/MsgHandler.h"
 #include "Core/ConfigManager.h"
 #include "Core/Core.h"
 #include "Core/CoreTiming.h"
 #include "Core/Host.h"
 #include "Core/FifoPlayer/FifoDataFile.h"
 #include "Core/FifoPlayer/FifoPlayer.h"
+#include "Core/HW/CPU.h"
 #include "Core/HW/GPFifo.h"
 #include "Core/HW/Memmap.h"
 #include "Core/HW/ProcessorInterface.h"
@@ -58,55 +60,103 @@ void FifoPlayer::Close()
 	m_FrameRangeEnd = 0;
 }
 
-bool FifoPlayer::Play()
+class FifoPlayer::CPUCore final : public CPUCoreBase
 {
-	if (!m_File)
-		return false;
-
-	if (m_File->GetFrameCount() == 0)
-		return false;
-
-	IsPlayingBackFifologWithBrokenEFBCopies = m_File->HasBrokenEFBCopies();
-
-	m_CurrentFrame = m_FrameRangeStart;
-
-	LoadMemory();
-
-	// This loop replaces the CPU loop that occurs when a game is run
-	while (PowerPC::GetState() != PowerPC::CPU_POWERDOWN)
+public:
+	explicit CPUCore(FifoPlayer* parent)
+		: m_parent(parent)
 	{
-		if (PowerPC::GetState() == PowerPC::CPU_RUNNING)
+	}
+	CPUCore(const CPUCore&) = delete;
+	~CPUCore()
+	{
+	}
+	CPUCore& operator=(const CPUCore&) = delete;
+
+	void Init() override
+	{
+		IsPlayingBackFifologWithBrokenEFBCopies = m_parent->m_File->HasBrokenEFBCopies();
+
+		m_parent->m_CurrentFrame = m_parent->m_FrameRangeStart;
+		m_parent->LoadMemory();
+	}
+
+	void Shutdown() override
+	{
+		IsPlayingBackFifologWithBrokenEFBCopies = false;
+	}
+
+	void ClearCache() override
+	{
+		// Nothing to clear.
+	}
+
+	void SingleStep() override
+	{
+		// NOTE: AdvanceFrame() will get stuck forever in Dual Core because the FIFO
+		//   is disabled by CPU::EnableStepping(true) so the frame never gets displayed.
+		PanicAlertT("Cannot SingleStep the FIFO. Use Frame Advance instead.");
+	}
+
+	const char* GetName() override
+	{
+		return "FifoPlayer";
+	}
+
+	void Run() override
+	{
+		while (CPU::GetState() == CPU::CPU_RUNNING)
 		{
-			if (m_CurrentFrame >= m_FrameRangeEnd)
+			switch (m_parent->AdvanceFrame())
 			{
-				if (m_Loop)
-				{
-					m_CurrentFrame = m_FrameRangeStart;
-				}
-				else
-				{
-					PowerPC::Stop();
-					Host_Message(WM_USER_STOP);
-				}
-			}
-			else
-			{
-				if (m_FrameWrittenCb)
-					m_FrameWrittenCb();
+			case CPU::CPU_POWERDOWN:
+				CPU::Break();
+				Host_Message(WM_USER_STOP);
+				break;
 
-				if (m_EarlyMemoryUpdates && m_CurrentFrame == m_FrameRangeStart)
-					WriteAllMemoryUpdates();
-
-				WriteFrame(m_File->GetFrame(m_CurrentFrame), m_FrameInfo[m_CurrentFrame]);
-
-				++m_CurrentFrame;
+			case CPU::CPU_STEPPING:
+				CPU::Break();
+				Host_UpdateMainFrame();
+				break;
 			}
 		}
 	}
 
-	IsPlayingBackFifologWithBrokenEFBCopies = false;
+private:
+	FifoPlayer* m_parent;
+};
 
-	return true;
+int FifoPlayer::AdvanceFrame()
+{
+	if (m_CurrentFrame >= m_FrameRangeEnd)
+	{
+		if (!m_Loop)
+			return CPU::CPU_POWERDOWN;
+		// If there are zero frames in the range then sleep instead of busy spinning
+		if (m_FrameRangeStart >= m_FrameRangeEnd)
+			return CPU::CPU_STEPPING;
+
+		m_CurrentFrame = m_FrameRangeStart;
+	}
+
+	if (m_FrameWrittenCb)
+		m_FrameWrittenCb();
+
+	if (m_EarlyMemoryUpdates && m_CurrentFrame == m_FrameRangeStart)
+		WriteAllMemoryUpdates();
+
+	WriteFrame(m_File->GetFrame(m_CurrentFrame), m_FrameInfo[m_CurrentFrame]);
+
+	++m_CurrentFrame;
+	return CPU::CPU_RUNNING;
+}
+
+std::unique_ptr<CPUCoreBase> FifoPlayer::GetCPUCore()
+{
+	if (!m_File || m_File->GetFrameCount() == 0)
+		return nullptr;
+
+	return std::make_unique<CPUCore>(this);
 }
 
 u32 FifoPlayer::GetFrameObjectCount()

Source/Core/Core/FifoPlayer/FifoPlayer.h

@@ -4,10 +4,12 @@
 
 #pragma once
 
+#include <memory>
 #include <string>
 #include <vector>
 
 #include "Core/FifoPlayer/FifoPlaybackAnalyzer.h"
+#include "Core/PowerPC/CPUCoreBase.h"
 
 class FifoDataFile;
 struct MemoryUpdate;
@@ -50,8 +52,12 @@ public:
 	bool Open(const std::string& filename);
 	void Close();
 
-	// Play is controlled by the state of PowerPC
-	bool Play();
+	// Returns a CPUCoreBase instance that can be injected into PowerPC as a
+	// pseudo-CPU. The instance is only valid while the FifoPlayer is Open().
+	// Returns nullptr if the FifoPlayer is not initialized correctly.
+	// Play/Pause/Stop of the FifoLog can be controlled normally via the
+	// PowerPC state.
+	std::unique_ptr<CPUCoreBase> GetCPUCore();
 
 	FifoDataFile *GetFile() { return m_File; }
 
@@ -85,8 +91,12 @@ public:
 	static FifoPlayer &GetInstance();
 
 private:
+	class CPUCore;
+
 	FifoPlayer();
 
+	int AdvanceFrame();
+
 	void WriteFrame(const FifoFrameInfo& frame, const AnalyzedFrameInfo &info);
 	void WriteFramePart(u32 dataStart, u32 dataEnd, u32 &nextMemUpdate, const FifoFrameInfo& frame, const AnalyzedFrameInfo& info);