diff --git a/Source/Core/Core/Core.cpp b/Source/Core/Core/Core.cpp index f026cb939d0..ac745a22a43 100644 --- a/Source/Core/Core/Core.cpp +++ b/Source/Core/Core/Core.cpp @@ -105,6 +105,11 @@ static bool s_is_throttler_temp_disabled = false; static bool s_frame_step = false; static std::atomic s_stop_frame_step; +// Threads other than the CPU thread must hold this when taking on the role of the CPU thread. +// The CPU thread is not required to hold this when doing normal work, but must hold it if writing +// to s_state. +static std::recursive_mutex s_core_mutex; + // The value Paused is never stored in this variable. The core is considered to be in // the Paused state if this variable is Running and the CPU reports that it's stepping. static std::atomic s_state = State::Uninitialized; @@ -221,6 +226,8 @@ bool WantsDeterminism() // BootManager.cpp bool Init(Core::System& system, std::unique_ptr boot, const WindowSystemInfo& wsi) { + std::lock_guard lock(s_core_mutex); + if (s_emu_thread.joinable()) { if (!IsUninitialized(system)) @@ -266,16 +273,20 @@ static void ResetRumble() // Called from GUI thread void Stop(Core::System& system) // - Hammertime! { - const State state = s_state.load(); - if (state == State::Stopping || state == State::Uninitialized) - return; + { + std::lock_guard lock(s_core_mutex); - AchievementManager::GetInstance().CloseGame(); + const State state = s_state.load(); + if (state == State::Stopping || state == State::Uninitialized) + return; - s_state.store(State::Stopping); + s_state.store(State::Stopping); + } NotifyStateChanged(State::Stopping); + AchievementManager::GetInstance().CloseGame(); + // Dump left over jobs HostDispatchJobs(system); @@ -322,7 +333,7 @@ void UndeclareAsHostThread() static void CPUSetInitialExecutionState(bool force_paused = false) { // The CPU starts in stepping state, and will wait until a new state is set before executing. - // SetState must be called on the host thread, so we defer it for later. + // SetState isn't safe to call from the CPU thread, so we ask the host thread to call it. QueueHostJob([force_paused](Core::System& system) { bool paused = SConfig::GetInstance().bBootToPause || force_paused; SetState(system, paused ? State::Paused : State::Running, true, true); @@ -362,10 +373,14 @@ static void CpuThread(Core::System& system, const std::optional& sa File::Delete(*savestate_path); } - // If s_state is Starting, change it to Running. But if it's already been set to Stopping - // by the host thread, don't change it. - State expected = State::Starting; - s_state.compare_exchange_strong(expected, State::Running); + { + std::unique_lock core_lock(s_core_mutex); + + // If s_state is Starting, change it to Running. But if it's already been set to Stopping + // because another thread called Stop, don't change it. + State expected = State::Starting; + s_state.compare_exchange_strong(expected, State::Running); + } { #ifndef _WIN32 @@ -423,12 +438,17 @@ static void FifoPlayerThread(Core::System& system, const std::optional boot { NotifyStateChanged(State::Starting); Common::ScopeGuard flag_guard{[] { - s_state.store(State::Uninitialized); + { + std::lock_guard lock(s_core_mutex); + s_state.store(State::Uninitialized); + } NotifyStateChanged(State::Uninitialized); @@ -682,35 +705,39 @@ static void EmuThread(Core::System& system, std::unique_ptr boot void SetState(Core::System& system, State state, bool report_state_change, bool override_achievement_restrictions) { - // State cannot be controlled until the CPU Thread is operational - if (s_state.load() != State::Running) - return; + { + std::lock_guard lock(s_core_mutex); - switch (state) - { - case State::Paused: -#ifdef USE_RETRO_ACHIEVEMENTS - if (!override_achievement_restrictions && !AchievementManager::GetInstance().CanPause()) + // State cannot be controlled until the CPU Thread is operational + if (s_state.load() != State::Running) return; -#endif // USE_RETRO_ACHIEVEMENTS - // NOTE: GetState() will return State::Paused immediately, even before anything has - // stopped (including the CPU). - system.GetCPU().SetStepping(true); // Break - Wiimote::Pause(); - ResetRumble(); + + switch (state) + { + case State::Paused: #ifdef USE_RETRO_ACHIEVEMENTS - AchievementManager::GetInstance().DoIdle(); + if (!override_achievement_restrictions && !AchievementManager::GetInstance().CanPause()) + return; #endif // USE_RETRO_ACHIEVEMENTS - break; - case State::Running: - { - system.GetCPU().SetStepping(false); - Wiimote::Resume(); - break; - } - default: - PanicAlertFmt("Invalid state"); - break; + // NOTE: GetState() will return State::Paused immediately, even before anything has + // stopped (including the CPU). + system.GetCPU().SetStepping(true); // Break + Wiimote::Pause(); + ResetRumble(); +#ifdef USE_RETRO_ACHIEVEMENTS + AchievementManager::GetInstance().DoIdle(); +#endif // USE_RETRO_ACHIEVEMENTS + break; + case State::Running: + { + system.GetCPU().SetStepping(false); + Wiimote::Resume(); + break; + } + default: + PanicAlertFmt("Invalid state"); + break; + } } // Certain callers only change the state momentarily. Sending a callback for them causes @@ -781,7 +808,7 @@ void SaveScreenShot(std::string_view name) static bool PauseAndLock(Core::System& system) { - // WARNING: PauseAndLock is not fully threadsafe so is only valid on the Host Thread + s_core_mutex.lock(); if (!IsRunning(system)) return true; @@ -804,7 +831,7 @@ static bool PauseAndLock(Core::System& system) 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 + Common::ScopeGuard scope_guard([] { s_core_mutex.unlock(); }); if (!IsRunning(system)) return; @@ -824,8 +851,7 @@ static void RestoreStateAndUnlock(Core::System& system, const bool unpause_on_un void RunOnCPUThread(Core::System& system, Common::MoveOnlyFunction function, bool wait_for_completion) { - // If the CPU thread is not running, assume there is no active CPU thread we can race against. - if (!IsRunning(system) || IsCPUThread()) + if (IsCPUThread()) { function(); return; @@ -834,10 +860,15 @@ void RunOnCPUThread(Core::System& system, Common::MoveOnlyFunction funct // Pause the CPU (set it to stepping mode). const bool was_running = PauseAndLock(system); - // Queue the job function. - if (wait_for_completion) + if (!IsRunning(system)) { - // Trigger the event after executing the function. + // If the core hasn't been started, there is no active CPU thread we can race against. + function(); + wait_for_completion = false; + } + else if (wait_for_completion) + { + // Queue the job function followed by triggering the event. s_cpu_thread_job_finished.Reset(); system.GetCPU().AddCPUThreadJob([&function] { function(); @@ -846,6 +877,7 @@ void RunOnCPUThread(Core::System& system, Common::MoveOnlyFunction funct } else { + // Queue the job function. system.GetCPU().AddCPUThreadJob(std::move(function)); } @@ -967,6 +999,8 @@ void NotifyStateChanged(Core::State state) void UpdateWantDeterminism(Core::System& system, bool initial) { + const Core::CPUThreadGuard guard(system); + // For now, this value is not itself configurable. Instead, individual // settings that depend on it, such as GPU determinism mode. should have // override options for testing, @@ -975,7 +1009,6 @@ void UpdateWantDeterminism(Core::System& system, bool initial) { NOTICE_LOG_FMT(COMMON, "Want determinism <- {}", new_want_determinism ? "true" : "false"); - const Core::CPUThreadGuard guard(system); s_wants_determinism = new_want_determinism; const auto ios = system.GetIOS(); if (ios) @@ -1037,6 +1070,9 @@ void DoFrameStep(Core::System& system) OSD::AddMessage("Frame stepping is disabled in RetroAchievements hardcore mode"); return; } + + std::lock_guard lock(s_core_mutex); + if (GetState(system) == State::Paused) { // if already paused, frame advance for 1 frame diff --git a/Source/Core/Core/Core.h b/Source/Core/Core/Core.h index 68a402772a9..72627d0038d 100644 --- a/Source/Core/Core/Core.h +++ b/Source/Core/Core/Core.h @@ -91,10 +91,9 @@ enum class ConsoleType : u32 ReservedTDEVSystem = 0x20000007, }; -// This is an RAII alternative to using PauseAndLock. If constructed from the host thread, the CPU -// thread is paused, and the current thread temporarily becomes the CPU thread. If constructed from -// the CPU thread, nothing special happens. This should only be constructed on the CPU thread or the -// host thread. +// This is an RAII alternative to using PauseAndLock. If constructed from any thread other than the +// CPU thread, the CPU thread is paused, and the current thread temporarily becomes the CPU thread. +// If constructed from the CPU thread, nothing special happens. // // Some functions use a parameter of this type to indicate that the function should only be called // from the CPU thread. If the parameter is a pointer, the function has a fallback for being called @@ -118,6 +117,8 @@ private: bool m_was_unpaused = false; }; +// These three are normally called from the Host thread. However, they can be called from any thread +// that isn't launched by the emulator core. bool Init(Core::System& system, std::unique_ptr boot, const WindowSystemInfo& wsi); void Stop(Core::System& system); void Shutdown(Core::System& system); @@ -144,7 +145,8 @@ bool IsHostThread(); bool WantsDeterminism(); -// [NOT THREADSAFE] For use by Host only +// SetState can't be called by the CPU thread, but can be called by any thread that isn't launched +// by the emulator core. void SetState(Core::System& system, State state, bool report_state_change = true, bool override_achievement_restrictions = false); State GetState(Core::System& system); @@ -159,7 +161,6 @@ void FrameUpdateOnCPUThread(); void OnFrameEnd(Core::System& system); // Run a function on the CPU thread, asynchronously. -// This is only valid to call from the host thread, since it uses PauseAndLock() internally. void RunOnCPUThread(Core::System& system, Common::MoveOnlyFunction function, bool wait_for_completion); @@ -171,7 +172,6 @@ int AddOnStateChangedCallback(StateChangedCallbackFunc callback); bool RemoveOnStateChangedCallback(int* handle); void NotifyStateChanged(Core::State state); -// Run on the Host thread when the factors change. [NOT THREADSAFE] void UpdateWantDeterminism(Core::System& system, bool initial = false); // Queue an arbitrary function to asynchronously run once on the Host thread later. diff --git a/Source/Core/DolphinQt/Host.cpp b/Source/Core/DolphinQt/Host.cpp index 57586300cfd..9407e5424b8 100644 --- a/Source/Core/DolphinQt/Host.cpp +++ b/Source/Core/DolphinQt/Host.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include @@ -245,7 +246,8 @@ bool Host_TASInputHasFocus() void Host_YieldToUI() { - qApp->processEvents(QEventLoop::ExcludeUserInputEvents); + if (qApp->thread() == QThread::currentThread()) + qApp->processEvents(QEventLoop::ExcludeUserInputEvents); } void Host_UpdateDisasmDialog()