From bebeba29c30e651156ba2ffe7717bd8ee4573be3 Mon Sep 17 00:00:00 2001 From: Dentomologist Date: Fri, 2 Jun 2023 16:15:41 -0700 Subject: [PATCH 01/14] CPU: Convert static variable to class member Make s_have_fake_cpu_thread a class member instead. In addition to getting rid of a bit of static state, this simplifies refactoring in an upcoming commit. --- Source/Core/Core/HW/CPU.cpp | 8 +++----- Source/Core/Core/HW/CPU.h | 2 ++ 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Source/Core/Core/HW/CPU.cpp b/Source/Core/Core/HW/CPU.cpp index 82b0cb9c4b4..fb9c89f9027 100644 --- a/Source/Core/Core/HW/CPU.cpp +++ b/Source/Core/Core/HW/CPU.cpp @@ -353,8 +353,6 @@ void CPUManager::Continue() bool CPUManager::PauseAndLock(bool do_lock, bool unpause_on_unlock, bool control_adjacent) { - // NOTE: This is protected by m_stepping_lock. - static bool s_have_fake_cpu_thread = false; bool was_unpaused = false; if (do_lock) @@ -380,16 +378,16 @@ bool CPUManager::PauseAndLock(bool do_lock, bool unpause_on_unlock, bool control // depth counter instead of being a boolean. if (!Core::IsCPUThread()) { - s_have_fake_cpu_thread = true; + m_have_fake_cpu_thread = true; Core::DeclareAsCPUThread(); } } else { // Only need the stepping lock for this - if (s_have_fake_cpu_thread) + if (m_have_fake_cpu_thread) { - s_have_fake_cpu_thread = false; + m_have_fake_cpu_thread = false; Core::UndeclareAsCPUThread(); } diff --git a/Source/Core/Core/HW/CPU.h b/Source/Core/Core/HW/CPU.h index 438a48763bb..13e915ba9a4 100644 --- a/Source/Core/Core/HW/CPU.h +++ b/Source/Core/Core/HW/CPU.h @@ -122,6 +122,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. From 691743fbc4cb119ce28b4494e084218ca0cc9bb2 Mon Sep 17 00:00:00 2001 From: Dentomologist Date: Sun, 4 Jun 2023 11:53:38 -0700 Subject: [PATCH 02/14] CPU: Extract RestoreStateAndUnlock from PauseAndLock Replace call of CPUManager::PauseAndLock(do_lock=false) with new function RestoreStateAndUnlock for clarity. Callers of Core::PauseAndLock ignore the return value when do_lock is false, so in that case was_unpaused in Core::PauseAndLock doesn't need to be set and so RestoreStateAndUnlock doesn't need to return anything. --- Source/Core/Core/Core.cpp | 2 +- Source/Core/Core/HW/CPU.cpp | 54 +++++++++++++++++++------------------ Source/Core/Core/HW/CPU.h | 1 + 3 files changed, 30 insertions(+), 27 deletions(-) diff --git a/Source/Core/Core/Core.cpp b/Source/Core/Core/Core.cpp index 3292abbff9c..daea8fd77ee 100644 --- a/Source/Core/Core/Core.cpp +++ b/Source/Core/Core/Core.cpp @@ -801,7 +801,7 @@ static bool PauseAndLock(Core::System& system, bool do_lock, bool unpause_on_unl // 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); + system.GetCPU().RestoreStateAndUnlock(unpause_on_unlock, true); } return was_unpaused; diff --git a/Source/Core/Core/HW/CPU.cpp b/Source/Core/Core/HW/CPU.cpp index fb9c89f9027..02e45de595e 100644 --- a/Source/Core/Core/HW/CPU.cpp +++ b/Source/Core/Core/HW/CPU.cpp @@ -382,36 +382,38 @@ bool CPUManager::PauseAndLock(bool do_lock, bool unpause_on_unlock, bool control Core::DeclareAsCPUThread(); } } - else - { - // 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)) - { - 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, const bool control_adjacent) +{ + // 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(); + + if (control_adjacent) + RunAdjacentSystems(m_state == State::Running); + } + m_stepping_lock.unlock(); +} + void CPUManager::AddCPUThreadJob(Common::MoveOnlyFunction function) { std::unique_lock state_lock(m_state_change_lock); diff --git a/Source/Core/Core/HW/CPU.h b/Source/Core/Core/HW/CPU.h index 13e915ba9a4..615b9ff3886 100644 --- a/Source/Core/Core/HW/CPU.h +++ b/Source/Core/Core/HW/CPU.h @@ -96,6 +96,7 @@ public: // "control_adjacent" causes PauseAndLock to behave like SetStepping by modifying the // state of the Audio and FIFO subsystems as well. bool PauseAndLock(bool do_lock, bool unpause_on_unlock = true, bool control_adjacent = false); + void RestoreStateAndUnlock(bool unpause_on_unlock, bool control_adjacent); // 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. From 1a6e2856857c6bc03c162113e9f58cf0ba34caec Mon Sep 17 00:00:00 2001 From: Dentomologist Date: Sun, 4 Jun 2023 13:00:33 -0700 Subject: [PATCH 03/14] CPU: Remove default arguments for PauseAndLock For clarity in the next commit. --- Source/Core/Core/Core.cpp | 2 +- Source/Core/Core/HW/CPU.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Source/Core/Core/Core.cpp b/Source/Core/Core/Core.cpp index daea8fd77ee..6a949f147ca 100644 --- a/Source/Core/Core/Core.cpp +++ b/Source/Core/Core/Core.cpp @@ -781,7 +781,7 @@ static bool PauseAndLock(Core::System& system, bool do_lock, bool unpause_on_unl // 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); + was_unpaused = system.GetCPU().PauseAndLock(true, true, false); } // audio has to come after CPU, because CPU thread can wait for audio thread (m_throttle). diff --git a/Source/Core/Core/HW/CPU.h b/Source/Core/Core/HW/CPU.h index 615b9ff3886..104a83204ad 100644 --- a/Source/Core/Core/HW/CPU.h +++ b/Source/Core/Core/HW/CPU.h @@ -95,7 +95,7 @@ public: // 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 // state of the Audio and FIFO subsystems as well. - bool PauseAndLock(bool do_lock, bool unpause_on_unlock = true, bool control_adjacent = false); + bool PauseAndLock(bool do_lock, bool unpause_on_unlock, bool control_adjacent); void RestoreStateAndUnlock(bool unpause_on_unlock, bool control_adjacent); // Adds a job to be executed during on the CPU thread. This should be combined with From 48d48fe1af09e437f574a5fb3d6a5b62a146e08a Mon Sep 17 00:00:00 2001 From: Dentomologist Date: Sun, 4 Jun 2023 13:07:43 -0700 Subject: [PATCH 04/14] CPUManager: Remove unnecessary PauseAndLock parameters CPUManager::PauseAndLock is now only called with do_lock=true, and unpause_on_unlock only ever was used when do_lock is false (which is now handled in RestoreStateAndUnlock instead), so both parameters are unnecessary. --- Source/Core/Core/Core.cpp | 2 +- Source/Core/Core/HW/CPU.cpp | 45 +++++++++++++++++-------------------- Source/Core/Core/HW/CPU.h | 2 +- 3 files changed, 22 insertions(+), 27 deletions(-) diff --git a/Source/Core/Core/Core.cpp b/Source/Core/Core/Core.cpp index 6a949f147ca..2b005374ada 100644 --- a/Source/Core/Core/Core.cpp +++ b/Source/Core/Core/Core.cpp @@ -781,7 +781,7 @@ static bool PauseAndLock(Core::System& system, bool do_lock, bool unpause_on_unl // 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, true, false); + was_unpaused = system.GetCPU().PauseAndLock(false); } // audio has to come after CPU, because CPU thread can wait for audio thread (m_throttle). diff --git a/Source/Core/Core/HW/CPU.cpp b/Source/Core/Core/HW/CPU.cpp index 02e45de595e..9942478cb2d 100644 --- a/Source/Core/Core/HW/CPU.cpp +++ b/Source/Core/Core/HW/CPU.cpp @@ -351,36 +351,31 @@ void CPUManager::Continue() Core::NotifyStateChanged(Core::State::Running); } -bool CPUManager::PauseAndLock(bool do_lock, bool unpause_on_unlock, bool control_adjacent) +bool CPUManager::PauseAndLock(const bool control_adjacent) { - bool was_unpaused = false; + m_stepping_lock.lock(); - 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; + if (control_adjacent) + RunAdjacentSystems(false); + state_lock.unlock(); - 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()) - { - m_have_fake_cpu_thread = true; - Core::DeclareAsCPUThread(); - } + // NOTE: It would make more sense for Core::DeclareAsCPUThread() to keep a + // depth counter instead of being a boolean. + if (!Core::IsCPUThread()) + { + m_have_fake_cpu_thread = true; + Core::DeclareAsCPUThread(); } return was_unpaused; diff --git a/Source/Core/Core/HW/CPU.h b/Source/Core/Core/HW/CPU.h index 104a83204ad..b6eea7f052a 100644 --- a/Source/Core/Core/HW/CPU.h +++ b/Source/Core/Core/HW/CPU.h @@ -95,7 +95,7 @@ public: // 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 // state of the Audio and FIFO subsystems as well. - bool PauseAndLock(bool do_lock, bool unpause_on_unlock, bool control_adjacent); + bool PauseAndLock(bool control_adjacent); void RestoreStateAndUnlock(bool unpause_on_unlock, bool control_adjacent); // Adds a job to be executed during on the CPU thread. This should be combined with From 8d0dbb0ef64ffb76b64bc94166399dc2dad1af2c Mon Sep 17 00:00:00 2001 From: Dentomologist Date: Sun, 4 Jun 2023 13:19:44 -0700 Subject: [PATCH 05/14] CPUManager: Remove redundant parameter from PauseAndLock PauseAndLock was only called with control_adjacent=false. Remove the parameter and the function call that was only made when it was true. --- Source/Core/Core/Core.cpp | 2 +- Source/Core/Core/HW/CPU.cpp | 4 +--- Source/Core/Core/HW/CPU.h | 2 +- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/Source/Core/Core/Core.cpp b/Source/Core/Core/Core.cpp index 2b005374ada..b8f5891f3dc 100644 --- a/Source/Core/Core/Core.cpp +++ b/Source/Core/Core/Core.cpp @@ -781,7 +781,7 @@ static bool PauseAndLock(Core::System& system, bool do_lock, bool unpause_on_unl // 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(false); + was_unpaused = system.GetCPU().PauseAndLock(); } // audio has to come after CPU, because CPU thread can wait for audio thread (m_throttle). diff --git a/Source/Core/Core/HW/CPU.cpp b/Source/Core/Core/HW/CPU.cpp index 9942478cb2d..6f1a3c72851 100644 --- a/Source/Core/Core/HW/CPU.cpp +++ b/Source/Core/Core/HW/CPU.cpp @@ -351,7 +351,7 @@ void CPUManager::Continue() Core::NotifyStateChanged(Core::State::Running); } -bool CPUManager::PauseAndLock(const bool control_adjacent) +bool CPUManager::PauseAndLock() { m_stepping_lock.lock(); @@ -366,8 +366,6 @@ bool CPUManager::PauseAndLock(const bool control_adjacent) 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 diff --git a/Source/Core/Core/HW/CPU.h b/Source/Core/Core/HW/CPU.h index b6eea7f052a..40cb25ca6eb 100644 --- a/Source/Core/Core/HW/CPU.h +++ b/Source/Core/Core/HW/CPU.h @@ -95,7 +95,7 @@ public: // 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 // state of the Audio and FIFO subsystems as well. - bool PauseAndLock(bool control_adjacent); + bool PauseAndLock(); void RestoreStateAndUnlock(bool unpause_on_unlock, bool control_adjacent); // Adds a job to be executed during on the CPU thread. This should be combined with From 4e64d8e94f8715885d243afe4dafcda0a56a05be Mon Sep 17 00:00:00 2001 From: Dentomologist Date: Sun, 4 Jun 2023 13:22:09 -0700 Subject: [PATCH 06/14] CPUManager: Remove redundant parameter from RestoreStateAndUnlock RestoreStateAndUnlock was only called with control_adjacent=true. Remove the parameter and unconditionally call the function that was gated behind it being true. --- Source/Core/Core/Core.cpp | 2 +- Source/Core/Core/HW/CPU.cpp | 5 ++--- Source/Core/Core/HW/CPU.h | 2 +- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/Source/Core/Core/Core.cpp b/Source/Core/Core/Core.cpp index b8f5891f3dc..f7087dee032 100644 --- a/Source/Core/Core/Core.cpp +++ b/Source/Core/Core/Core.cpp @@ -801,7 +801,7 @@ static bool PauseAndLock(Core::System& system, bool do_lock, bool unpause_on_unl // 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, true); + system.GetCPU().RestoreStateAndUnlock(unpause_on_unlock); } return was_unpaused; diff --git a/Source/Core/Core/HW/CPU.cpp b/Source/Core/Core/HW/CPU.cpp index 6f1a3c72851..18034303527 100644 --- a/Source/Core/Core/HW/CPU.cpp +++ b/Source/Core/Core/HW/CPU.cpp @@ -379,7 +379,7 @@ bool CPUManager::PauseAndLock() return was_unpaused; } -void CPUManager::RestoreStateAndUnlock(const bool unpause_on_unlock, const bool control_adjacent) +void CPUManager::RestoreStateAndUnlock(const bool unpause_on_unlock) { // Only need the stepping lock for this if (m_have_fake_cpu_thread) @@ -401,8 +401,7 @@ void CPUManager::RestoreStateAndUnlock(const bool unpause_on_unlock, const bool m_state_paused_and_locked = false; m_state_cpu_cvar.notify_one(); - if (control_adjacent) - RunAdjacentSystems(m_state == State::Running); + RunAdjacentSystems(m_state == State::Running); } m_stepping_lock.unlock(); } diff --git a/Source/Core/Core/HW/CPU.h b/Source/Core/Core/HW/CPU.h index 40cb25ca6eb..4fa0c1105bc 100644 --- a/Source/Core/Core/HW/CPU.h +++ b/Source/Core/Core/HW/CPU.h @@ -96,7 +96,7 @@ public: // "control_adjacent" causes PauseAndLock to behave like SetStepping by modifying the // state of the Audio and FIFO subsystems as well. bool PauseAndLock(); - void RestoreStateAndUnlock(bool unpause_on_unlock, bool control_adjacent); + 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. From c9c8461d366f383e1e121996422bf001deae57c1 Mon Sep 17 00:00:00 2001 From: Dentomologist Date: Sun, 4 Jun 2023 13:59:53 -0700 Subject: [PATCH 07/14] Core: Extract RestoreStateAndUnlock from PauseAndLock Replace calls of PauseAndLock(do_lock=false) with new function RestoreStateAndUnlock for clarity. Callers of PauseAndLock ignored the return value when do_lock is false, so RestoreStateAndUnlock doesn't need to return anything. --- Source/Core/Core/Core.cpp | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/Source/Core/Core/Core.cpp b/Source/Core/Core/Core.cpp index f7087dee032..93db0fc28fe 100644 --- a/Source/Core/Core/Core.cpp +++ b/Source/Core/Core/Core.cpp @@ -807,6 +807,26 @@ static bool PauseAndLock(Core::System& system, bool do_lock, bool unpause_on_unl 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()->PauseAndLock(false); + system.GetFifo().PauseAndLock(false, false); + 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 function, bool wait_for_completion) { @@ -836,7 +856,7 @@ void RunOnCPUThread(Core::System& system, Common::MoveOnlyFunction 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) @@ -1059,7 +1079,7 @@ CPUThreadGuard::CPUThreadGuard(Core::System& system) CPUThreadGuard::~CPUThreadGuard() { if (!m_was_cpu_thread) - PauseAndLock(m_system, false, m_was_unpaused); + RestoreStateAndUnlock(m_system, m_was_unpaused); } } // namespace Core From 2d888ea4d3c8f7957f15ad7428555cc03334acb3 Mon Sep 17 00:00:00 2001 From: Dentomologist Date: Sun, 4 Jun 2023 14:11:52 -0700 Subject: [PATCH 08/14] Core: Remove unnecessary PauseAndLock parameters PauseAndLock is now only called with do_lock=true, and unpause_on_unlock only ever was used when do_lock is false (which is now handled in RestoreStateAndUnlock instead), so both parameters are unnecessary. --- Source/Core/Core/Core.cpp | 32 ++++++++------------------------ 1 file changed, 8 insertions(+), 24 deletions(-) diff --git a/Source/Core/Core/Core.cpp b/Source/Core/Core/Core.cpp index 93db0fc28fe..81f1695ea94 100644 --- a/Source/Core/Core/Core.cpp +++ b/Source/Core/Core/Core.cpp @@ -768,42 +768,26 @@ 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; - if (do_lock) - { - // 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(); - } + // First pause the CPU. This acquires a wrapper mutex and converts the current thread into + // a temporary replacement CPU Thread. + const bool was_unpaused = system.GetCPU().PauseAndLock(); // 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(true); // 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(true, false); 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. - system.GetCPU().RestoreStateAndUnlock(unpause_on_unlock); - } - return was_unpaused; } @@ -838,7 +822,7 @@ void RunOnCPUThread(Core::System& system, Common::MoveOnlyFunction 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) @@ -1073,7 +1057,7 @@ 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() From f628a979c435723eaec40b7081634a67b575fcb8 Mon Sep 17 00:00:00 2001 From: Dentomologist Date: Sun, 4 Jun 2023 14:30:38 -0700 Subject: [PATCH 09/14] Core: Remove pointless call to FifoManager::PauseAndLock FifoManager::PauseAndLock doesn't do anything when doLock and unpauseOnUnlock are both false, so remove the call. --- Source/Core/Core/Core.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/Source/Core/Core/Core.cpp b/Source/Core/Core/Core.cpp index 81f1695ea94..a433f656657 100644 --- a/Source/Core/Core/Core.cpp +++ b/Source/Core/Core/Core.cpp @@ -799,7 +799,6 @@ static void RestoreStateAndUnlock(Core::System& system, const bool unpause_on_un return; system.GetDSP().GetDSPEmulator()->PauseAndLock(false); - system.GetFifo().PauseAndLock(false, false); ResetRumble(); // CPU is unlocked last because CPU::RestoreStateAndUnlock contains the synchronization mechanism From f497eb519e672a0dffbcd029dc937425af754ba0 Mon Sep 17 00:00:00 2001 From: Dentomologist Date: Sun, 4 Jun 2023 17:07:07 -0700 Subject: [PATCH 10/14] FifoManager: Extract RestoreState from PauseAndLock Replace calls of FifoManager::PauseAndLock(do_lock=false) with new function RestoreState for clarity. --- Source/Core/DolphinQt/Host.cpp | 2 +- Source/Core/VideoCommon/Fifo.cpp | 6 ++++++ Source/Core/VideoCommon/Fifo.h | 1 + Source/Core/VideoCommon/VideoConfig.cpp | 2 +- 4 files changed, 9 insertions(+), 2 deletions(-) diff --git a/Source/Core/DolphinQt/Host.cpp b/Source/Core/DolphinQt/Host.cpp index 46b0aa8963a..78ff0fae8a8 100644 --- a/Source/Core/DolphinQt/Host.cpp +++ b/Source/Core/DolphinQt/Host.cpp @@ -108,7 +108,7 @@ static void RunWithGPUThreadInactive(std::function f) auto& fifo = system.GetFifo(); fifo.PauseAndLock(true, was_running); f(); - fifo.PauseAndLock(false, was_running); + fifo.RestoreState(was_running); } else { diff --git a/Source/Core/VideoCommon/Fifo.cpp b/Source/Core/VideoCommon/Fifo.cpp index 513b6554087..7e07e0bbf37 100644 --- a/Source/Core/VideoCommon/Fifo.cpp +++ b/Source/Core/VideoCommon/Fifo.cpp @@ -86,6 +86,12 @@ void FifoManager::PauseAndLock(bool do_lock, bool unpause_on_unlock) } } +void FifoManager::RestoreState(const bool was_running) +{ + if (was_running) + EmulatorState(true); +} + void FifoManager::Init() { if (!m_config_callback_id) diff --git a/Source/Core/VideoCommon/Fifo.h b/Source/Core/VideoCommon/Fifo.h index 2fce8fdcb53..c6fc6376615 100644 --- a/Source/Core/VideoCommon/Fifo.h +++ b/Source/Core/VideoCommon/Fifo.h @@ -53,6 +53,7 @@ public: void Prepare(); // Must be called from the CPU thread. void DoState(PointerWrap& f); void PauseAndLock(bool do_lock, bool unpause_on_unlock); + 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; } diff --git a/Source/Core/VideoCommon/VideoConfig.cpp b/Source/Core/VideoCommon/VideoConfig.cpp index 46f8732c2fa..4c11fb084e8 100644 --- a/Source/Core/VideoCommon/VideoConfig.cpp +++ b/Source/Core/VideoCommon/VideoConfig.cpp @@ -70,7 +70,7 @@ void VideoConfig::Refresh() g_Config.VerifyValidity(); if (lock_gpu_thread) - system.GetFifo().PauseAndLock(false, true); + system.GetFifo().RestoreState(true); }; s_config_changed_callback_id = From 933071dd573c202de8c3c722e042d37d6bfcc2d1 Mon Sep 17 00:00:00 2001 From: Dentomologist Date: Sun, 4 Jun 2023 17:18:18 -0700 Subject: [PATCH 11/14] FifoManager: Remove redundant PauseAndLock parameters PauseAndLock was only called with do_lock=true, and the function only used unpauseOnUnlock when do_lock was false. --- Source/Core/Core/Core.cpp | 2 +- Source/Core/DolphinQt/Host.cpp | 2 +- Source/Core/VideoCommon/Fifo.cpp | 20 ++++++-------------- Source/Core/VideoCommon/Fifo.h | 2 +- Source/Core/VideoCommon/VideoConfig.cpp | 2 +- 5 files changed, 10 insertions(+), 18 deletions(-) diff --git a/Source/Core/Core/Core.cpp b/Source/Core/Core/Core.cpp index a433f656657..06100980167 100644 --- a/Source/Core/Core/Core.cpp +++ b/Source/Core/Core/Core.cpp @@ -784,7 +784,7 @@ static bool PauseAndLock(Core::System& system) // video has to come after CPU, because CPU thread can wait for video thread // (s_efbAccessRequested). - system.GetFifo().PauseAndLock(true, false); + system.GetFifo().PauseAndLock(); ResetRumble(); diff --git a/Source/Core/DolphinQt/Host.cpp b/Source/Core/DolphinQt/Host.cpp index 78ff0fae8a8..57586300cfd 100644 --- a/Source/Core/DolphinQt/Host.cpp +++ b/Source/Core/DolphinQt/Host.cpp @@ -106,7 +106,7 @@ static void RunWithGPUThreadInactive(std::function 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.RestoreState(was_running); } diff --git a/Source/Core/VideoCommon/Fifo.cpp b/Source/Core/VideoCommon/Fifo.cpp index 7e07e0bbf37..ad8761c76ba 100644 --- a/Source/Core/VideoCommon/Fifo.cpp +++ b/Source/Core/VideoCommon/Fifo.cpp @@ -67,23 +67,15 @@ 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) - return; + if (!m_system.IsDualCoreMode() || m_use_deterministic_gpu_thread) + return; - m_gpu_mainloop.WaitYield(std::chrono::milliseconds(100), Host_YieldToUI); - } - else - { - if (unpause_on_unlock) - EmulatorState(true); - } + m_gpu_mainloop.WaitYield(std::chrono::milliseconds(100), Host_YieldToUI); } void FifoManager::RestoreState(const bool was_running) diff --git a/Source/Core/VideoCommon/Fifo.h b/Source/Core/VideoCommon/Fifo.h index c6fc6376615..2afa110532e 100644 --- a/Source/Core/VideoCommon/Fifo.h +++ b/Source/Core/VideoCommon/Fifo.h @@ -52,7 +52,7 @@ 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; } diff --git a/Source/Core/VideoCommon/VideoConfig.cpp b/Source/Core/VideoCommon/VideoConfig.cpp index 4c11fb084e8..60668f9f2d7 100644 --- a/Source/Core/VideoCommon/VideoConfig.cpp +++ b/Source/Core/VideoCommon/VideoConfig.cpp @@ -64,7 +64,7 @@ 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(); From d5f079d78b8a894d03dee0590bfa691962f033f2 Mon Sep 17 00:00:00 2001 From: Dentomologist Date: Wed, 13 Aug 2025 15:43:08 -0700 Subject: [PATCH 12/14] DSPEmulator: Extract UnpauseAndUnlock from PauseAndLock Replace call to PauseAndLock(do_lock=false) with new function UnpauseAndUnlock. --- Source/Core/Core/Core.cpp | 2 +- Source/Core/Core/DSPEmulator.h | 1 + Source/Core/Core/HW/DSPHLE/DSPHLE.cpp | 4 ++++ Source/Core/Core/HW/DSPHLE/DSPHLE.h | 1 + Source/Core/Core/HW/DSPLLE/DSPLLE.cpp | 12 ++++++++++++ Source/Core/Core/HW/DSPLLE/DSPLLE.h | 1 + 6 files changed, 20 insertions(+), 1 deletion(-) diff --git a/Source/Core/Core/Core.cpp b/Source/Core/Core/Core.cpp index 06100980167..429d51d3ade 100644 --- a/Source/Core/Core/Core.cpp +++ b/Source/Core/Core/Core.cpp @@ -798,7 +798,7 @@ static void RestoreStateAndUnlock(Core::System& system, const bool unpause_on_un if (!IsRunning(system)) return; - system.GetDSP().GetDSPEmulator()->PauseAndLock(false); + system.GetDSP().GetDSPEmulator()->UnpauseAndUnlock(); ResetRumble(); // CPU is unlocked last because CPU::RestoreStateAndUnlock contains the synchronization mechanism diff --git a/Source/Core/Core/DSPEmulator.h b/Source/Core/Core/DSPEmulator.h index edd64cabad7..59a400ea22a 100644 --- a/Source/Core/Core/DSPEmulator.h +++ b/Source/Core/Core/DSPEmulator.h @@ -23,6 +23,7 @@ public: virtual void DoState(PointerWrap& p) = 0; virtual void PauseAndLock(bool do_lock) = 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; diff --git a/Source/Core/Core/HW/DSPHLE/DSPHLE.cpp b/Source/Core/Core/HW/DSPHLE/DSPHLE.cpp index e29fbddb714..325d9cc3620 100644 --- a/Source/Core/Core/HW/DSPHLE/DSPHLE.cpp +++ b/Source/Core/Core/HW/DSPHLE/DSPHLE.cpp @@ -245,4 +245,8 @@ u16 DSPHLE::DSP_ReadControlRegister() void DSPHLE::PauseAndLock(bool do_lock) { } + +void DSPHLE::UnpauseAndUnlock() +{ +} } // namespace DSP::HLE diff --git a/Source/Core/Core/HW/DSPHLE/DSPHLE.h b/Source/Core/Core/HW/DSPHLE/DSPHLE.h index 08b5c75e030..7ba5c12b334 100644 --- a/Source/Core/Core/HW/DSPHLE/DSPHLE.h +++ b/Source/Core/Core/HW/DSPHLE/DSPHLE.h @@ -35,6 +35,7 @@ public: bool IsLLE() const override { return false; } void DoState(PointerWrap& p) override; void PauseAndLock(bool do_lock) override; + void UnpauseAndUnlock() override; void DSP_WriteMailBoxHigh(bool cpu_mailbox, u16 value) override; void DSP_WriteMailBoxLow(bool cpu_mailbox, u16 value) override; diff --git a/Source/Core/Core/HW/DSPLLE/DSPLLE.cpp b/Source/Core/Core/HW/DSPLLE/DSPLLE.cpp index 9925dd5354c..9e8ee5baff1 100644 --- a/Source/Core/Core/HW/DSPLLE/DSPLLE.cpp +++ b/Source/Core/Core/HW/DSPLLE/DSPLLE.cpp @@ -305,4 +305,16 @@ void DSPLLE::PauseAndLock(bool do_lock) } } } + +void DSPLLE::UnpauseAndUnlock() +{ + m_dsp_thread_mutex.unlock(); + + 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 diff --git a/Source/Core/Core/HW/DSPLLE/DSPLLE.h b/Source/Core/Core/HW/DSPLLE/DSPLLE.h index 5796fd8fee2..3deb3be039d 100644 --- a/Source/Core/Core/HW/DSPLLE/DSPLLE.h +++ b/Source/Core/Core/HW/DSPLLE/DSPLLE.h @@ -27,6 +27,7 @@ public: bool IsLLE() const override { return true; } void DoState(PointerWrap& p) override; void PauseAndLock(bool do_lock) override; + void UnpauseAndUnlock() override; void DSP_WriteMailBoxHigh(bool cpu_mailbox, u16 value) override; void DSP_WriteMailBoxLow(bool cpu_mailbox, u16 value) override; From c47db6dba7f9b0625dd18aacbbf234cc0c0a98ba Mon Sep 17 00:00:00 2001 From: Dentomologist Date: Wed, 13 Aug 2025 15:50:18 -0700 Subject: [PATCH 13/14] DSPEmulator: Remove redundant parameter and code PauseAndLock is only called with do_lock=true, so remove the parameter and modify PauseAndLock accordingly. --- Source/Core/Core/Core.cpp | 2 +- Source/Core/Core/DSPEmulator.h | 2 +- Source/Core/Core/HW/DSPHLE/DSPHLE.cpp | 2 +- Source/Core/Core/HW/DSPHLE/DSPHLE.h | 2 +- Source/Core/Core/HW/DSPLLE/DSPLLE.cpp | 18 ++---------------- Source/Core/Core/HW/DSPLLE/DSPLLE.h | 2 +- 6 files changed, 7 insertions(+), 21 deletions(-) diff --git a/Source/Core/Core/Core.cpp b/Source/Core/Core/Core.cpp index 429d51d3ade..03fc9c614b0 100644 --- a/Source/Core/Core/Core.cpp +++ b/Source/Core/Core/Core.cpp @@ -780,7 +780,7 @@ static bool PauseAndLock(Core::System& system) const bool was_unpaused = system.GetCPU().PauseAndLock(); // audio has to come after CPU, because CPU thread can wait for audio thread (m_throttle). - system.GetDSP().GetDSPEmulator()->PauseAndLock(true); + system.GetDSP().GetDSPEmulator()->PauseAndLock(); // video has to come after CPU, because CPU thread can wait for video thread // (s_efbAccessRequested). diff --git a/Source/Core/Core/DSPEmulator.h b/Source/Core/Core/DSPEmulator.h index 59a400ea22a..2dbf405065c 100644 --- a/Source/Core/Core/DSPEmulator.h +++ b/Source/Core/Core/DSPEmulator.h @@ -22,7 +22,7 @@ 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; diff --git a/Source/Core/Core/HW/DSPHLE/DSPHLE.cpp b/Source/Core/Core/HW/DSPHLE/DSPHLE.cpp index 325d9cc3620..c8fc1317dea 100644 --- a/Source/Core/Core/HW/DSPHLE/DSPHLE.cpp +++ b/Source/Core/Core/HW/DSPHLE/DSPHLE.cpp @@ -242,7 +242,7 @@ u16 DSPHLE::DSP_ReadControlRegister() return m_dsp_control.Hex; } -void DSPHLE::PauseAndLock(bool do_lock) +void DSPHLE::PauseAndLock() { } diff --git a/Source/Core/Core/HW/DSPHLE/DSPHLE.h b/Source/Core/Core/HW/DSPHLE/DSPHLE.h index 7ba5c12b334..a284d4facdd 100644 --- a/Source/Core/Core/HW/DSPHLE/DSPHLE.h +++ b/Source/Core/Core/HW/DSPHLE/DSPHLE.h @@ -34,7 +34,7 @@ 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; diff --git a/Source/Core/Core/HW/DSPLLE/DSPLLE.cpp b/Source/Core/Core/HW/DSPLLE/DSPLLE.cpp index 9e8ee5baff1..ca18e45999b 100644 --- a/Source/Core/Core/HW/DSPLLE/DSPLLE.cpp +++ b/Source/Core/Core/HW/DSPLLE/DSPLLE.cpp @@ -287,23 +287,9 @@ 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(); - } - else - { - m_dsp_thread_mutex.unlock(); - - 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(); - } - } + m_dsp_thread_mutex.lock(); } void DSPLLE::UnpauseAndUnlock() diff --git a/Source/Core/Core/HW/DSPLLE/DSPLLE.h b/Source/Core/Core/HW/DSPLLE/DSPLLE.h index 3deb3be039d..0797308feed 100644 --- a/Source/Core/Core/HW/DSPLLE/DSPLLE.h +++ b/Source/Core/Core/HW/DSPLLE/DSPLLE.h @@ -26,7 +26,7 @@ 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; From 70d8bc6fd722c9d2fb13e20cc3de41b97a25a650 Mon Sep 17 00:00:00 2001 From: Dentomologist Date: Sun, 26 Oct 2025 18:06:38 -0700 Subject: [PATCH 14/14] CPU: Update PauseAndLock comment --- Source/Core/Core/HW/CPU.h | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/Source/Core/Core/HW/CPU.h b/Source/Core/Core/HW/CPU.h index 4fa0c1105bc..887d899710f 100644 --- a/Source/Core/Core/HW/CPU.h +++ b/Source/Core/Core/HW/CPU.h @@ -86,15 +86,16 @@ 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). - // While this lock is held, the CPU Thread will not perform any action so it is safe to access - // PowerPC, CoreTiming, etc. without racing 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. - // Return value for do_lock == false is whether the state was changed *to* State::Running or not. - // 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 - // state of the Audio and FIFO subsystems as well. + // Locks the CPU Thread (waiting for it to become idle). While this lock is held, the CPU Thread + // will not perform any action so it is safe to access PowerPC, CoreTiming, etc. without racing + // the CPU Thread. + // + // Cannot be used recursively. Cannot be used by System threads as it will deadlock. It is + // threadsafe otherwise. + // + // Each call to PauseAndLock must be paired with a call to RestoreStateAndUnlock. The return value + // for PauseAndLock is whether the state was State::Running or not and should be passed as the + // argument to RestoreStateAndUnlock. bool PauseAndLock(); void RestoreStateAndUnlock(bool unpause_on_unlock);