diff --git a/Source/Core/Core/Core.cpp b/Source/Core/Core/Core.cpp index 991f2f09dfe..61664e2202b 100644 --- a/Source/Core/Core/Core.cpp +++ b/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; - 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(true); - } + // 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(); // 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 function, bool wait_for_completion) { @@ -829,7 +832,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) @@ -847,7 +850,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) @@ -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 diff --git a/Source/Core/Core/DSPEmulator.h b/Source/Core/Core/DSPEmulator.h index edd64cabad7..2dbf405065c 100644 --- a/Source/Core/Core/DSPEmulator.h +++ b/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; diff --git a/Source/Core/Core/HW/CPU.cpp b/Source/Core/Core/HW/CPU.cpp index 2f07e384b10..d55f565c0c2 100644 --- a/Source/Core/Core/HW/CPU.cpp +++ b/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. - static bool s_have_fake_cpu_thread = false; - 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(); - - 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(); - } + m_state_cpu_idle_cvar.wait(state_lock); } - 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 - if (s_have_fake_cpu_thread) - { - 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(); + m_have_fake_cpu_thread = true; + Core::DeclareAsCPUThread(); } + 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 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 438a48763bb..887d899710f 100644 --- a/Source/Core/Core/HW/CPU.h +++ b/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). - // 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. - bool PauseAndLock(bool do_lock, bool unpause_on_unlock = true, bool control_adjacent = false); + // 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); // 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. diff --git a/Source/Core/Core/HW/DSPHLE/DSPHLE.cpp b/Source/Core/Core/HW/DSPHLE/DSPHLE.cpp index e29fbddb714..c8fc1317dea 100644 --- a/Source/Core/Core/HW/DSPHLE/DSPHLE.cpp +++ b/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 diff --git a/Source/Core/Core/HW/DSPHLE/DSPHLE.h b/Source/Core/Core/HW/DSPHLE/DSPHLE.h index 08b5c75e030..a284d4facdd 100644 --- a/Source/Core/Core/HW/DSPHLE/DSPHLE.h +++ b/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; diff --git a/Source/Core/Core/HW/DSPLLE/DSPLLE.cpp b/Source/Core/Core/HW/DSPLLE/DSPLLE.cpp index 9925dd5354c..ca18e45999b 100644 --- a/Source/Core/Core/HW/DSPLLE/DSPLLE.cpp +++ b/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(); - } - else - { - m_dsp_thread_mutex.unlock(); + m_dsp_thread_mutex.lock(); +} - 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(); - } +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..0797308feed 100644 --- a/Source/Core/Core/HW/DSPLLE/DSPLLE.h +++ b/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; diff --git a/Source/Core/DolphinQt/Host.cpp b/Source/Core/DolphinQt/Host.cpp index 46b0aa8963a..57586300cfd 100644 --- a/Source/Core/DolphinQt/Host.cpp +++ b/Source/Core/DolphinQt/Host.cpp @@ -106,9 +106,9 @@ 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.PauseAndLock(false, was_running); + fifo.RestoreState(was_running); } else { diff --git a/Source/Core/VideoCommon/Fifo.cpp b/Source/Core/VideoCommon/Fifo.cpp index 0e7bb4c3417..f183c936551 100644 --- a/Source/Core/VideoCommon/Fifo.cpp +++ b/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) - 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) +{ + if (was_running) + EmulatorState(true); } void FifoManager::Init() diff --git a/Source/Core/VideoCommon/Fifo.h b/Source/Core/VideoCommon/Fifo.h index 2fce8fdcb53..2afa110532e 100644 --- a/Source/Core/VideoCommon/Fifo.h +++ b/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; } diff --git a/Source/Core/VideoCommon/VideoConfig.cpp b/Source/Core/VideoCommon/VideoConfig.cpp index 46f8732c2fa..60668f9f2d7 100644 --- a/Source/Core/VideoCommon/VideoConfig.cpp +++ b/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 =