diff --git a/Source/Android/jni/Host.h b/Source/Android/jni/Host.h index 3a7a3048257..0183038d9a2 100644 --- a/Source/Android/jni/Host.h +++ b/Source/Android/jni/Host.h @@ -5,37 +5,24 @@ #include -#include "Core/Core.h" - // The Core only supports using a single Host thread. // If multiple threads want to call host functions then they need to queue // sequentially for access. +// TODO: The above isn't true anymore, so we should get rid of this class. struct HostThreadLock { - explicit HostThreadLock() : m_lock(s_host_identity_mutex) { Core::DeclareAsHostThread(); } + explicit HostThreadLock() : m_lock(s_host_identity_mutex) {} - ~HostThreadLock() - { - if (m_lock.owns_lock()) - Core::UndeclareAsHostThread(); - } + ~HostThreadLock() = default; HostThreadLock(const HostThreadLock& other) = delete; HostThreadLock(HostThreadLock&& other) = delete; HostThreadLock& operator=(const HostThreadLock& other) = delete; HostThreadLock& operator=(HostThreadLock&& other) = delete; - void Lock() - { - m_lock.lock(); - Core::DeclareAsHostThread(); - } + void Lock() { m_lock.lock(); } - void Unlock() - { - m_lock.unlock(); - Core::UndeclareAsHostThread(); - } + void Unlock() { m_lock.unlock(); } private: static std::mutex s_host_identity_mutex; diff --git a/Source/Core/Core/AchievementManager.cpp b/Source/Core/Core/AchievementManager.cpp index 5423bb29e03..7a41d57c84f 100644 --- a/Source/Core/Core/AchievementManager.cpp +++ b/Source/Core/Core/AchievementManager.cpp @@ -65,7 +65,7 @@ void AchievementManager::Init(void* hwnd) { { std::lock_guard lg{m_lock}; - m_client = rc_client_create(MemoryVerifier, Request); + m_client = rc_client_create(MemoryPeeker, Request); } std::string host_url = Config::Get(Config::RA_HOST_URL); if (!host_url.empty()) @@ -170,7 +170,6 @@ void AchievementManager::LoadGame(const DiscIO::Volume* volume) } else { - rc_client_set_read_memory_function(m_client, MemoryVerifier); rc_client_begin_load_game(m_client, "", LoadGameCallback, NULL); } return; @@ -210,7 +209,6 @@ void AchievementManager::LoadGame(const DiscIO::Volume* volume) else { u32 console_id = FindConsoleID(volume->GetVolumeType()); - rc_client_set_read_memory_function(m_client, MemoryVerifier); rc_client_begin_identify_and_load_game(m_client, console_id, "", NULL, 0, LoadGameCallback, NULL); } @@ -320,22 +318,6 @@ void AchievementManager::DoFrame() if (!(IsGameLoaded() || m_dll_found) || !Core::IsCPUThread()) return; { -#ifdef RC_CLIENT_SUPPORTS_RAINTEGRATION - if (m_dll_found) - { - std::lock_guard lg{m_memory_lock}; - Core::System* system = m_system.load(std::memory_order_acquire); - if (!system) - return; - Core::CPUThreadGuard thread_guard(*system); - u32 mem2_size = (system->IsWii()) ? system->GetMemory().GetExRamSizeReal() : 0; - if (m_cloned_memory.size() != MEM1_SIZE + mem2_size) - m_cloned_memory.resize(MEM1_SIZE + mem2_size); - system->GetMemory().CopyFromEmu(m_cloned_memory.data(), 0, MEM1_SIZE); - if (mem2_size > 0) - system->GetMemory().CopyFromEmu(m_cloned_memory.data() + MEM1_SIZE, MEM2_START, mem2_size); - } -#endif // RC_CLIENT_SUPPORTS_RAINTEGRATION std::lock_guard lg{m_lock}; rc_client_do_frame(m_client); } @@ -1061,7 +1043,6 @@ void AchievementManager::LoadGameCallback(int result, const char* error_message, if (game == nullptr) return; - rc_client_set_read_memory_function(instance.m_client, MemoryPeeker); instance.FetchGameBadges(); instance.m_system.store(&Core::System::GetInstance(), std::memory_order_release); instance.update_event.Trigger({.all = true}); @@ -1337,53 +1318,11 @@ void AchievementManager::Request(const rc_api_request_t* request, }); } -// Currently, when rc_client calls the memory peek method provided in its constructor (or in -// rc_client_set_read_memory_function) it will do so on the thread that calls DoFrame, which is -// currently the host thread, with one exception: an asynchronous callback in the load game process. -// This is done to validate/invalidate each memory reference in the downloaded assets, mark assets -// as unsupported, and notify the player upon startup that there are unsupported assets and how -// many. As such, all that call needs to do is return the number of bytes that can be read with this -// call. As only the CPU and host threads are allowed to read from memory, I provide a separate -// method for this verification. In lieu of a more convenient set of steps, I provide MemoryVerifier -// to rc_client at construction, and in the Load Game callback, after the verification has been -// complete, I call rc_client_set_read_memory_function to switch to the usual MemoryPeeker for all -// future synchronous calls. -u32 AchievementManager::MemoryVerifier(u32 address, u8* buffer, u32 num_bytes, rc_client_t* client) -{ - auto& system = Core::System::GetInstance(); - u32 mem2_size = system.GetMemory().GetExRamSizeReal(); - if (address < MEM1_SIZE + mem2_size) - return std::min(MEM1_SIZE + mem2_size - address, num_bytes); - return 0; -} - u32 AchievementManager::MemoryPeeker(u32 address, u8* buffer, u32 num_bytes, rc_client_t* client) { if (buffer == nullptr) return 0u; -#ifdef RC_CLIENT_SUPPORTS_RAINTEGRATION - auto& instance = AchievementManager::GetInstance(); - if (instance.m_dll_found) - { - std::lock_guard lg{instance.m_memory_lock}; - if (u64(address) + num_bytes > instance.m_cloned_memory.size()) - { - ERROR_LOG_FMT(ACHIEVEMENTS, - "Attempt to read past memory size: size {} address {} write length {}", - instance.m_cloned_memory.size(), address, num_bytes); - return 0; - } - std::copy(instance.m_cloned_memory.begin() + address, - instance.m_cloned_memory.begin() + address + num_bytes, buffer); - return num_bytes; - } -#endif // RC_CLIENT_SUPPORTS_RAINTEGRATION auto& system = Core::System::GetInstance(); - if (!(Core::IsHostThread() || Core::IsCPUThread())) - { - ASSERT_MSG(ACHIEVEMENTS, false, "MemoryPeeker called from wrong thread"); - return 0; - } Core::CPUThreadGuard thread_guard(system); if (address > MEM1_SIZE) address += (MEM2_START - MEM1_SIZE); @@ -1607,32 +1546,17 @@ void AchievementManager::MemoryPoker(u32 address, u8* buffer, u32 num_bytes, rc_ { if (buffer == nullptr) return; - if (!(Core::IsHostThread() || Core::IsCPUThread())) - { - Core::QueueHostJob([address, buffer, num_bytes, client](Core::System& system) { - MemoryPoker(address, buffer, num_bytes, client); - }); - return; - } auto& instance = AchievementManager::GetInstance(); - if (u64(address) + num_bytes >= instance.m_cloned_memory.size()) - { - ERROR_LOG_FMT(ACHIEVEMENTS, - "Attempt to write past memory size: size {} address {} write length {}", - instance.m_cloned_memory.size(), address, num_bytes); - return; - } Core::System* system = instance.m_system.load(std::memory_order_acquire); if (!system) return; Core::CPUThreadGuard thread_guard(*system); - std::lock_guard lg{instance.m_memory_lock}; if (address < MEM1_SIZE) system->GetMemory().CopyToEmu(address, buffer, num_bytes); else system->GetMemory().CopyToEmu(address - MEM1_SIZE + MEM2_START, buffer, num_bytes); - std::copy(buffer, buffer + num_bytes, instance.m_cloned_memory.begin() + address); } + void AchievementManager::GameTitleEstimateHandler(char* buffer, u32 buffer_size, rc_client_t* client) { diff --git a/Source/Core/Core/AchievementManager.h b/Source/Core/Core/AchievementManager.h index 1d2245c24c8..b666b3f8627 100644 --- a/Source/Core/Core/AchievementManager.h +++ b/Source/Core/Core/AchievementManager.h @@ -251,7 +251,6 @@ private: static void Request(const rc_api_request_t* request, rc_client_server_callback_t callback, void* callback_data, rc_client_t* client); - static u32 MemoryVerifier(u32 address, u8* buffer, u32 num_bytes, rc_client_t* client); static u32 MemoryPeeker(u32 address, u8* buffer, u32 num_bytes, rc_client_t* client); void FetchBadge(Badge* badge, u32 badge_type, const BadgeNameFunction function, const UpdatedItems callback_data); @@ -293,8 +292,6 @@ private: bool m_dll_found = false; #ifdef RC_CLIENT_SUPPORTS_RAINTEGRATION - std::vector m_cloned_memory; - std::recursive_mutex m_memory_lock; std::string m_title_estimate; #endif // RC_CLIENT_SUPPORTS_RAINTEGRATION diff --git a/Source/Core/Core/Core.cpp b/Source/Core/Core/Core.cpp index 5241e40a29d..58da62b4e88 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; @@ -126,7 +131,6 @@ static Common::Event s_cpu_thread_job_finished; static thread_local bool tls_is_cpu_thread = false; static thread_local bool tls_is_gpu_thread = false; -static thread_local bool tls_is_host_thread = false; static void EmuThread(Core::System& system, std::unique_ptr boot, WindowSystemInfo wsi); @@ -207,11 +211,6 @@ bool IsGPUThread() return tls_is_gpu_thread; } -bool IsHostThread() -{ - return tls_is_host_thread; -} - bool WantsDeterminism() { return s_wants_determinism; @@ -221,6 +220,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 +267,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); @@ -308,21 +313,11 @@ void UndeclareAsGPUThread() tls_is_gpu_thread = false; } -void DeclareAsHostThread() -{ - tls_is_host_thread = true; -} - -void UndeclareAsHostThread() -{ - tls_is_host_thread = false; -} - // For the CPU Thread only. 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 +357,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 +422,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 +689,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 +792,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 +815,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 +835,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 +844,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 +861,7 @@ void RunOnCPUThread(Core::System& system, Common::MoveOnlyFunction funct } else { + // Queue the job function. system.GetCPU().AddCPUThreadJob(std::move(function)); } @@ -943,6 +959,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, @@ -951,7 +969,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) @@ -1013,6 +1030,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 a6e40d22573..815d049656d 100644 --- a/Source/Core/Core/Core.h +++ b/Source/Core/Core/Core.h @@ -92,10 +92,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 @@ -119,6 +118,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); @@ -127,8 +128,6 @@ void DeclareAsCPUThread(); void UndeclareAsCPUThread(); void DeclareAsGPUThread(); void UndeclareAsGPUThread(); -void DeclareAsHostThread(); -void UndeclareAsHostThread(); std::string StopMessage(bool main_thread, std::string_view message); @@ -141,11 +140,11 @@ bool IsUninitialized(Core::System& system); bool IsCPUThread(); // this tells us whether we are the CPU thread. bool IsGPUThread(); -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); @@ -160,7 +159,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); @@ -170,7 +168,6 @@ using StateChangedCallbackFunc = std::function; [[nodiscard]] Common::EventHook AddOnStateChangedCallback(StateChangedCallbackFunc callback); 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/DolphinNoGUI/MainNoGUI.cpp b/Source/Core/DolphinNoGUI/MainNoGUI.cpp index 2a7d7c8113d..9c8f938836f 100644 --- a/Source/Core/DolphinNoGUI/MainNoGUI.cpp +++ b/Source/Core/DolphinNoGUI/MainNoGUI.cpp @@ -189,8 +189,6 @@ static std::unique_ptr GetPlatform(const optparse::Values& options) int main(const int argc, char* argv[]) { - Core::DeclareAsHostThread(); - const auto parser = CommandLineParse::CreateParser(CommandLineParse::ParserOptions::OmitGUIOptions); parser->add_option("-p", "--platform") 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() diff --git a/Source/Core/DolphinQt/Main.cpp b/Source/Core/DolphinQt/Main.cpp index 49436475ae9..ce85ee1f0de 100644 --- a/Source/Core/DolphinQt/Main.cpp +++ b/Source/Core/DolphinQt/Main.cpp @@ -127,8 +127,6 @@ int main(int argc, char* argv[]) } #endif - Core::DeclareAsHostThread(); - #ifdef __APPLE__ // On macOS, a command line option matching the format "-psn_X_XXXXXX" is passed when // the application is launched for the first time. This is to set the "ProcessSerialNumber", diff --git a/Source/Core/DolphinQt/Settings.cpp b/Source/Core/DolphinQt/Settings.cpp index 76363d8e10b..f74c6589cc1 100644 --- a/Source/Core/DolphinQt/Settings.cpp +++ b/Source/Core/DolphinQt/Settings.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include "AudioCommon/AudioCommon.h" @@ -76,7 +77,7 @@ Settings::Settings() }); m_hotplug_event_hook = g_controller_interface.RegisterDevicesChangedCallback([this] { - if (Core::IsHostThread()) + if (qApp->thread() == QThread::currentThread()) { emit DevicesChanged(); } diff --git a/Source/Core/DolphinTool/ToolMain.cpp b/Source/Core/DolphinTool/ToolMain.cpp index 224ead3d7b2..058bccc7b5b 100644 --- a/Source/Core/DolphinTool/ToolMain.cpp +++ b/Source/Core/DolphinTool/ToolMain.cpp @@ -32,8 +32,6 @@ static void PrintUsage() int main(int argc, char* argv[]) { - Core::DeclareAsHostThread(); - if (argc < 2) { PrintUsage(); diff --git a/Source/UnitTests/UnitTestsMain.cpp b/Source/UnitTests/UnitTestsMain.cpp index 4eed0c60993..3edb1c19092 100644 --- a/Source/UnitTests/UnitTestsMain.cpp +++ b/Source/UnitTests/UnitTestsMain.cpp @@ -25,7 +25,6 @@ int main(int argc, char** argv) { fmt::print(stderr, "Running main() from UnitTestsMain.cpp\n"); Common::RegisterMsgAlertHandler(TestMsgHandler); - Core::DeclareAsHostThread(); ::testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS();