Merge pull request #14035 from Dentomologist/pauseandlock_refactoring

PauseAndLock Refactoring

Source/Core/Core/Core.cpp

@@ -779,45 +779,48 @@ void SaveScreenShot(std::string_view name)
   g_frame_dumper->SaveScreenshot(fmt::format("{}{}.png", GenerateScreenshotFolderPath(), name));
 }
 
-static bool PauseAndLock(Core::System& system, bool do_lock, bool unpause_on_unlock)
+static bool PauseAndLock(Core::System& system)
 {
   // WARNING: PauseAndLock is not fully threadsafe so is only valid on the Host Thread
 
   if (!IsRunning(system))
     return true;
 
-  bool was_unpaused = true;
+  // First pause the CPU.  This acquires a wrapper mutex and converts the current thread into
-  if (do_lock)
+  // a temporary replacement CPU Thread.
-  {
+  const bool was_unpaused = system.GetCPU().PauseAndLock();
-    // first pause the CPU
-    // This acquires a wrapper mutex and converts the current thread into
-    // a temporary replacement CPU Thread.
-    was_unpaused = system.GetCPU().PauseAndLock(true);
-  }
 
   // audio has to come after CPU, because CPU thread can wait for audio thread (m_throttle).
-  system.GetDSP().GetDSPEmulator()->PauseAndLock(do_lock);
+  system.GetDSP().GetDSPEmulator()->PauseAndLock();
 
   // video has to come after CPU, because CPU thread can wait for video thread
   // (s_efbAccessRequested).
-  system.GetFifo().PauseAndLock(do_lock, false);
+  system.GetFifo().PauseAndLock();
 
   ResetRumble();
 
-  // CPU is unlocked last because CPU::PauseAndLock contains the synchronization
-  // mechanism that prevents CPU::Break from racing.
-  if (!do_lock)
-  {
-    // The CPU is responsible for managing the Audio and FIFO state so we use its
-    // mechanism to unpause them. If we unpaused the systems above when releasing
-    // the locks then they could call CPU::Break which would require detecting it
-    // and re-pausing with CPU::SetStepping.
-    was_unpaused = system.GetCPU().PauseAndLock(false, unpause_on_unlock, true);
-  }
-
   return was_unpaused;
 }
 
+static void RestoreStateAndUnlock(Core::System& system, const bool unpause_on_unlock)
+{
+  // WARNING: RestoreStateAndUnlock is not fully threadsafe so is only valid on the Host Thread
+
+  if (!IsRunning(system))
+    return;
+
+  system.GetDSP().GetDSPEmulator()->UnpauseAndUnlock();
+  ResetRumble();
+
+  // CPU is unlocked last because CPU::RestoreStateAndUnlock contains the synchronization mechanism
+  // that prevents CPU::Break from racing.
+  //
+  // The CPU is responsible for managing the Audio and FIFO state so we use its mechanism to unpause
+  // them. If we unpaused the systems above when releasing the locks then they could call CPU::Break
+  // which would require detecting it and re-pausing with CPU::SetStepping.
+  system.GetCPU().RestoreStateAndUnlock(unpause_on_unlock);
+}
+
 void RunOnCPUThread(Core::System& system, Common::MoveOnlyFunction<void()> function,
                     bool wait_for_completion)
 {
@@ -829,7 +832,7 @@ void RunOnCPUThread(Core::System& system, Common::MoveOnlyFunction<void()> funct
   }
 
   // Pause the CPU (set it to stepping mode).
-  const bool was_running = PauseAndLock(system, true, true);
+  const bool was_running = PauseAndLock(system);
 
   // Queue the job function.
   if (wait_for_completion)
@@ -847,7 +850,7 @@ void RunOnCPUThread(Core::System& system, Common::MoveOnlyFunction<void()> funct
   }
 
   // Release the CPU thread, and let it execute the callback.
-  PauseAndLock(system, false, was_running);
+  RestoreStateAndUnlock(system, was_running);
 
   // If we're waiting for completion, block until the event fires.
   if (wait_for_completion)
@@ -1064,13 +1067,13 @@ CPUThreadGuard::CPUThreadGuard(Core::System& system)
     : m_system(system), m_was_cpu_thread(IsCPUThread())
 {
   if (!m_was_cpu_thread)
-    m_was_unpaused = PauseAndLock(system, true, true);
+    m_was_unpaused = PauseAndLock(system);
 }
 
 CPUThreadGuard::~CPUThreadGuard()
 {
   if (!m_was_cpu_thread)
-    PauseAndLock(m_system, false, m_was_unpaused);
+    RestoreStateAndUnlock(m_system, m_was_unpaused);
 }
 
 }  // namespace Core

Source/Core/Core/DSPEmulator.h

@@ -22,7 +22,8 @@ public:
   virtual void Shutdown() = 0;
 
   virtual void DoState(PointerWrap& p) = 0;
-  virtual void PauseAndLock(bool do_lock) = 0;
+  virtual void PauseAndLock() = 0;
+  virtual void UnpauseAndUnlock() = 0;
 
   virtual void DSP_WriteMailBoxHigh(bool cpu_mailbox, u16 value) = 0;
   virtual void DSP_WriteMailBoxLow(bool cpu_mailbox, u16 value) = 0;

Source/Core/Core/HW/CPU.cpp

@@ -347,69 +347,61 @@ void CPUManager::Continue()
   Core::NotifyStateChanged(Core::State::Running);
 }
 
-bool CPUManager::PauseAndLock(bool do_lock, bool unpause_on_unlock, bool control_adjacent)
+bool CPUManager::PauseAndLock()
 {
-  // NOTE: This is protected by m_stepping_lock.
+  m_stepping_lock.lock();
-  static bool s_have_fake_cpu_thread = false;
-  bool was_unpaused = false;
 
-  if (do_lock)
+  std::unique_lock state_lock(m_state_change_lock);
+  m_state_paused_and_locked = true;
+
+  const bool was_unpaused = m_state == State::Running;
+  SetStateLocked(State::Stepping);
+
+  while (m_state_cpu_thread_active)
   {
-    m_stepping_lock.lock();
+    m_state_cpu_idle_cvar.wait(state_lock);
-
-    std::unique_lock state_lock(m_state_change_lock);
-    m_state_paused_and_locked = true;
-
-    was_unpaused = m_state == State::Running;
-    SetStateLocked(State::Stepping);
-
-    while (m_state_cpu_thread_active)
-    {
-      m_state_cpu_idle_cvar.wait(state_lock);
-    }
-
-    if (control_adjacent)
-      RunAdjacentSystems(false);
-    state_lock.unlock();
-
-    // NOTE: It would make more sense for Core::DeclareAsCPUThread() to keep a
-    //   depth counter instead of being a boolean.
-    if (!Core::IsCPUThread())
-    {
-      s_have_fake_cpu_thread = true;
-      Core::DeclareAsCPUThread();
-    }
   }
-  else
+
+  state_lock.unlock();
+
+  // NOTE: It would make more sense for Core::DeclareAsCPUThread() to keep a
+  //   depth counter instead of being a boolean.
+  if (!Core::IsCPUThread())
   {
-    // Only need the stepping lock for this
+    m_have_fake_cpu_thread = true;
-    if (s_have_fake_cpu_thread)
+    Core::DeclareAsCPUThread();
-    {
-      s_have_fake_cpu_thread = false;
-      Core::UndeclareAsCPUThread();
-    }
-
-    {
-      std::lock_guard state_lock(m_state_change_lock);
-      if (m_state_system_request_stepping)
-      {
-        m_state_system_request_stepping = false;
-      }
-      else if (unpause_on_unlock && SetStateLocked(State::Running))
-      {
-        was_unpaused = true;
-      }
-      m_state_paused_and_locked = false;
-      m_state_cpu_cvar.notify_one();
-
-      if (control_adjacent)
-        RunAdjacentSystems(m_state == State::Running);
-    }
-    m_stepping_lock.unlock();
   }
+
   return was_unpaused;
 }
 
+void CPUManager::RestoreStateAndUnlock(const bool unpause_on_unlock)
+{
+  // Only need the stepping lock for this
+  if (m_have_fake_cpu_thread)
+  {
+    m_have_fake_cpu_thread = false;
+    Core::UndeclareAsCPUThread();
+  }
+
+  {
+    std::lock_guard state_lock(m_state_change_lock);
+    if (m_state_system_request_stepping)
+    {
+      m_state_system_request_stepping = false;
+    }
+    else if (unpause_on_unlock)
+    {
+      SetStateLocked(State::Running);
+    }
+    m_state_paused_and_locked = false;
+    m_state_cpu_cvar.notify_one();
+
+    RunAdjacentSystems(m_state == State::Running);
+  }
+  m_stepping_lock.unlock();
+}
+
 void CPUManager::AddCPUThreadJob(Common::MoveOnlyFunction<void()> function)
 {
   std::unique_lock state_lock(m_state_change_lock);

Source/Core/Core/HW/CPU.h

@@ -86,16 +86,18 @@ public:
   // Strictly read-only. A lock is required to change the value.
   const State* GetStatePtr() const;
 
-  // Locks the CPU Thread (waiting for it to become idle).
+  // Locks the CPU Thread (waiting for it to become idle). While this lock is held, the CPU Thread
-  // While this lock is held, the CPU Thread will not perform any action so it is safe to access
+  // will not perform any action so it is safe to access PowerPC, CoreTiming, etc. without racing
-  // PowerPC, CoreTiming, etc. without racing the CPU Thread.
+  // the CPU Thread.
-  // Cannot be used recursively. Must be paired as PauseAndLock(true)/PauseAndLock(false).
+  //
-  // Return value for do_lock == true is whether the state was State::Running or not.
+  // Cannot be used recursively. Cannot be used by System threads as it will deadlock. It is
-  // Return value for do_lock == false is whether the state was changed *to* State::Running or not.
+  // threadsafe otherwise.
-  // Cannot be used by System threads as it will deadlock. It is threadsafe otherwise.
+  //
-  // "control_adjacent" causes PauseAndLock to behave like SetStepping by modifying the
+  // Each call to PauseAndLock must be paired with a call to RestoreStateAndUnlock. The return value
-  //   state of the Audio and FIFO subsystems as well.
+  // for PauseAndLock is whether the state was State::Running or not and should be passed as the
-  bool PauseAndLock(bool do_lock, bool unpause_on_unlock = true, bool control_adjacent = false);
+  // argument to RestoreStateAndUnlock.
+  bool PauseAndLock();
+  void RestoreStateAndUnlock(bool unpause_on_unlock);
 
   // Adds a job to be executed during on the CPU thread. This should be combined with
   // PauseAndLock(), as while the CPU is in the run loop, it won't execute the function.
@@ -122,6 +124,8 @@ private:
   //   deadlock because of the order inversion. (A -> X,Y; B -> Y,X; A waits for
   //   B, B waits for A)
   std::mutex m_stepping_lock;
+  // Protected by m_stepping_lock.
+  bool m_have_fake_cpu_thread = false;
 
   // Primary lock. Protects changing m_state, requesting instruction stepping and
   // pause-and-locking.

Source/Core/Core/HW/DSPHLE/DSPHLE.cpp

@@ -242,7 +242,11 @@ u16 DSPHLE::DSP_ReadControlRegister()
   return m_dsp_control.Hex;
 }
 
-void DSPHLE::PauseAndLock(bool do_lock)
+void DSPHLE::PauseAndLock()
+{
+}
+
+void DSPHLE::UnpauseAndUnlock()
 {
 }
 }  // namespace DSP::HLE

Source/Core/Core/HW/DSPHLE/DSPHLE.h

@@ -34,7 +34,8 @@ public:
   void Shutdown() override;
   bool IsLLE() const override { return false; }
   void DoState(PointerWrap& p) override;
-  void PauseAndLock(bool do_lock) override;
+  void PauseAndLock() override;
+  void UnpauseAndUnlock() override;
 
   void DSP_WriteMailBoxHigh(bool cpu_mailbox, u16 value) override;
   void DSP_WriteMailBoxLow(bool cpu_mailbox, u16 value) override;

Source/Core/Core/HW/DSPLLE/DSPLLE.cpp

@@ -287,22 +287,20 @@ u32 DSPLLE::DSP_UpdateRate()
   return 12600;  // TO BE TWEAKED
 }
 
-void DSPLLE::PauseAndLock(bool do_lock)
+void DSPLLE::PauseAndLock()
 {
-  if (do_lock)
+  m_dsp_thread_mutex.lock();
-  {
+}
-    m_dsp_thread_mutex.lock();
-  }
-  else
-  {
-    m_dsp_thread_mutex.unlock();
 
-    if (m_is_dsp_on_thread)
+void DSPLLE::UnpauseAndUnlock()
-    {
+{
-      // Signal the DSP thread so it can perform any outstanding work now (if any)
+  m_dsp_thread_mutex.unlock();
-      m_ppc_event.Wait();
+
-      m_dsp_event.Set();
+  if (m_is_dsp_on_thread)
-    }
+  {
+    // Signal the DSP thread so it can perform any outstanding work now (if any)
+    m_ppc_event.Wait();
+    m_dsp_event.Set();
   }
 }
 }  // namespace DSP::LLE

Source/Core/Core/HW/DSPLLE/DSPLLE.h

@@ -26,7 +26,8 @@ public:
   void Shutdown() override;
   bool IsLLE() const override { return true; }
   void DoState(PointerWrap& p) override;
-  void PauseAndLock(bool do_lock) override;
+  void PauseAndLock() override;
+  void UnpauseAndUnlock() override;
 
   void DSP_WriteMailBoxHigh(bool cpu_mailbox, u16 value) override;
   void DSP_WriteMailBoxLow(bool cpu_mailbox, u16 value) override;

Source/Core/DolphinQt/Host.cpp

@@ -106,9 +106,9 @@ static void RunWithGPUThreadInactive(std::function<void()> f)
     auto& system = Core::System::GetInstance();
     const bool was_running = Core::GetState(system) == Core::State::Running;
     auto& fifo = system.GetFifo();
-    fifo.PauseAndLock(true, was_running);
+    fifo.PauseAndLock();
     f();
-    fifo.PauseAndLock(false, was_running);
+    fifo.RestoreState(was_running);
   }
   else
   {

Source/Core/VideoCommon/Fifo.cpp

@@ -67,23 +67,21 @@ void FifoManager::DoState(PointerWrap& p)
   p.Do(m_syncing_suspended);
 }
 
-void FifoManager::PauseAndLock(bool do_lock, bool unpause_on_unlock)
+void FifoManager::PauseAndLock()
 {
-  if (do_lock)
+  SyncGPU(SyncGPUReason::Other);
-  {
+  EmulatorState(false);
-    SyncGPU(SyncGPUReason::Other);
-    EmulatorState(false);
 
-    if (!m_system.IsDualCoreMode() || m_use_deterministic_gpu_thread)
+  if (!m_system.IsDualCoreMode() || m_use_deterministic_gpu_thread)
-      return;
+    return;
 
-    m_gpu_mainloop.WaitYield(std::chrono::milliseconds(100), Host_YieldToUI);
+  m_gpu_mainloop.WaitYield(std::chrono::milliseconds(100), Host_YieldToUI);
-  }
+}
-  else
+
-  {
+void FifoManager::RestoreState(const bool was_running)
-    if (unpause_on_unlock)
+{
-      EmulatorState(true);
+  if (was_running)
-  }
+    EmulatorState(true);
 }
 
 void FifoManager::Init()

Source/Core/VideoCommon/Fifo.h

@@ -52,7 +52,8 @@ public:
   void Shutdown();
   void Prepare();  // Must be called from the CPU thread.
   void DoState(PointerWrap& f);
-  void PauseAndLock(bool do_lock, bool unpause_on_unlock);
+  void PauseAndLock();
+  void RestoreState(bool was_running);
   void UpdateWantDeterminism(bool want);
   bool UseDeterministicGPUThread() const { return m_use_deterministic_gpu_thread; }
   bool UseSyncGPU() const { return m_config_sync_gpu; }

Source/Core/VideoCommon/VideoConfig.cpp

@@ -64,13 +64,13 @@ void VideoConfig::Refresh()
 
       const bool lock_gpu_thread = Core::IsRunning(system);
       if (lock_gpu_thread)
-        system.GetFifo().PauseAndLock(true, false);
+        system.GetFifo().PauseAndLock();
 
       g_Config.Refresh();
       g_Config.VerifyValidity();
 
       if (lock_gpu_thread)
-        system.GetFifo().PauseAndLock(false, true);
+        system.GetFifo().RestoreState(true);
     };
 
     s_config_changed_callback_id =