From 71ce8bb6f00f4d1cbc1012270d6daefdbda4254d Mon Sep 17 00:00:00 2001 From: JosJuice Date: Wed, 16 Aug 2023 21:16:41 +0200 Subject: [PATCH] Don't call RunAsCPUThread in config callbacks In theory, our config system supports calling Set from any thread. But because we have config callbacks that call RunAsCPUThread, it's a lot more restricted in practice. Calling Set from any thread other than the host thread or the CPU thread is formally thread unsafe, and calling Set on the host thread while the CPU thread is showing a panic alert causes a deadlock. This is especially a problem because 04072f0 made the "Ignore for this session" button in panic alerts call Set. Because so many of our config callbacks want their code to run on the CPU thread, I thought it would make sense to have a centralized way to move execution to the CPU thread for config callbacks. To solve the deadlock problem, this new way is non-blocking. This means that threads other than the CPU thread might continue executing before the CPU thread is informed of the new config, but I don't think there's any problem with that. Intends to fix https://bugs.dolphin-emu.org/issues/13108. --- Source/Core/Common/Config/Config.h | 3 +- Source/Core/Core/CMakeLists.txt | 2 + Source/Core/Core/CPUThreadConfigCallback.cpp | 76 +++++++++++++++++++ Source/Core/Core/CPUThreadConfigCallback.h | 22 ++++++ Source/Core/Core/Core.cpp | 4 + Source/Core/Core/CoreTiming.cpp | 13 ++-- Source/Core/Core/FreeLookConfig.cpp | 4 +- Source/Core/Core/HW/CPU.cpp | 3 + Source/Core/Core/IOS/SDIO/SDIOSlot0.cpp | 19 ++--- .../Core/Core/PowerPC/JitCommon/JitBase.cpp | 8 +- Source/Core/DolphinLib.props | 2 + Source/Core/VideoCommon/VideoConfig.cpp | 25 ++++-- 12 files changed, 152 insertions(+), 29 deletions(-) create mode 100644 Source/Core/Core/CPUThreadConfigCallback.cpp create mode 100644 Source/Core/Core/CPUThreadConfigCallback.h diff --git a/Source/Core/Common/Config/Config.h b/Source/Core/Common/Config/Config.h index b448f234f9..4d770ee36e 100644 --- a/Source/Core/Common/Config/Config.h +++ b/Source/Core/Common/Config/Config.h @@ -22,7 +22,8 @@ void AddLayer(std::unique_ptr loader); std::shared_ptr GetLayer(LayerType layer); void RemoveLayer(LayerType layer); -// returns an ID that can be passed to RemoveConfigChangedCallback() +// Returns an ID that can be passed to RemoveConfigChangedCallback(). +// The callback may be called from any thread. size_t AddConfigChangedCallback(ConfigChangedCallback func); void RemoveConfigChangedCallback(size_t callback_id); void OnConfigChanged(); diff --git a/Source/Core/Core/CMakeLists.txt b/Source/Core/Core/CMakeLists.txt index f50185049a..40d02136c5 100644 --- a/Source/Core/Core/CMakeLists.txt +++ b/Source/Core/Core/CMakeLists.txt @@ -59,6 +59,8 @@ add_library(core Core.h CoreTiming.cpp CoreTiming.h + CPUThreadConfigCallback.cpp + CPUThreadConfigCallback.h Debugger/CodeTrace.cpp Debugger/CodeTrace.h Debugger/DebugInterface.h diff --git a/Source/Core/Core/CPUThreadConfigCallback.cpp b/Source/Core/Core/CPUThreadConfigCallback.cpp new file mode 100644 index 0000000000..2148630c22 --- /dev/null +++ b/Source/Core/Core/CPUThreadConfigCallback.cpp @@ -0,0 +1,76 @@ +// Copyright 2023 Dolphin Emulator Project +// SPDX-License-Identifier: GPL-2.0-or-later + +#include "Core/CPUThreadConfigCallback.h" + +#include + +#include "Common/Assert.h" +#include "Common/Config/Config.h" +#include "Core/Core.h" + +namespace +{ +std::atomic s_should_run_callbacks = false; + +static std::vector> s_callbacks; +static size_t s_next_callback_id = 0; + +void RunCallbacks() +{ + for (const auto& callback : s_callbacks) + callback.second(); +} + +void OnConfigChanged() +{ + if (Core::IsCPUThread()) + { + s_should_run_callbacks.store(false, std::memory_order_relaxed); + RunCallbacks(); + } + else + { + s_should_run_callbacks.store(true, std::memory_order_relaxed); + } +} + +}; // namespace + +namespace CPUThreadConfigCallback +{ +size_t AddConfigChangedCallback(Config::ConfigChangedCallback func) +{ + DEBUG_ASSERT(Core::IsCPUThread()); + + static size_t s_config_changed_callback_id = Config::AddConfigChangedCallback(&OnConfigChanged); + + const size_t callback_id = s_next_callback_id; + ++s_next_callback_id; + s_callbacks.emplace_back(std::make_pair(callback_id, std::move(func))); + return callback_id; +} + +void RemoveConfigChangedCallback(size_t callback_id) +{ + DEBUG_ASSERT(Core::IsCPUThread()); + + for (auto it = s_callbacks.begin(); it != s_callbacks.end(); ++it) + { + if (it->first == callback_id) + { + s_callbacks.erase(it); + return; + } + } +} + +void CheckForConfigChanges() +{ + DEBUG_ASSERT(Core::IsCPUThread()); + + if (s_should_run_callbacks.exchange(false, std::memory_order_relaxed)) + RunCallbacks(); +} + +}; // namespace CPUThreadConfigCallback diff --git a/Source/Core/Core/CPUThreadConfigCallback.h b/Source/Core/Core/CPUThreadConfigCallback.h new file mode 100644 index 0000000000..c43e01a9a0 --- /dev/null +++ b/Source/Core/Core/CPUThreadConfigCallback.h @@ -0,0 +1,22 @@ +// Copyright 2023 Dolphin Emulator Project +// SPDX-License-Identifier: GPL-2.0-or-later + +#pragma once + +#include "Common/Config/Config.h" + +// This file lets you register callbacks like in Common/Config/Config.h, with the difference that +// callbacks registered here are guaranteed to run on the CPU thread. Callbacks registered here may +// run with a slight delay compared to regular config callbacks. + +namespace CPUThreadConfigCallback +{ +// returns an ID that can be passed to RemoveConfigChangedCallback() +size_t AddConfigChangedCallback(Config::ConfigChangedCallback func); + +void RemoveConfigChangedCallback(size_t callback_id); + +// Should be called regularly from the CPU thread +void CheckForConfigChanges(); + +}; // namespace CPUThreadConfigCallback diff --git a/Source/Core/Core/Core.cpp b/Source/Core/Core/Core.cpp index 2865c81a9d..0fc7db59dd 100644 --- a/Source/Core/Core/Core.cpp +++ b/Source/Core/Core/Core.cpp @@ -41,6 +41,7 @@ #include "Core/AchievementManager.h" #include "Core/Boot/Boot.h" #include "Core/BootManager.h" +#include "Core/CPUThreadConfigCallback.h" #include "Core/Config/MainSettings.h" #include "Core/ConfigManager.h" #include "Core/CoreTiming.h" @@ -511,6 +512,9 @@ static void EmuThread(std::unique_ptr boot, WindowSystemInfo wsi DeclareAsCPUThread(); s_frame_step = false; + // If settings have changed since the previous run, notify callbacks. + CPUThreadConfigCallback::CheckForConfigChanges(); + // Switch the window used for inputs to the render window. This way, the cursor position // is relative to the render window, instead of the main window. ASSERT(g_controller_interface.IsInit()); diff --git a/Source/Core/Core/CoreTiming.cpp b/Source/Core/Core/CoreTiming.cpp index 8e5d489a58..2e6f6a0968 100644 --- a/Source/Core/Core/CoreTiming.cpp +++ b/Source/Core/Core/CoreTiming.cpp @@ -16,6 +16,7 @@ #include "Common/Logging/Log.h" #include "Common/SPSCQueue.h" +#include "Core/CPUThreadConfigCallback.h" #include "Core/Config/MainSettings.h" #include "Core/Core.h" #include "Core/PowerPC/PowerPC.h" @@ -88,8 +89,8 @@ void CoreTimingManager::UnregisterAllEvents() void CoreTimingManager::Init() { - m_registered_config_callback_id = Config::AddConfigChangedCallback( - [this]() { Core::RunAsCPUThread([this]() { RefreshConfig(); }); }); + m_registered_config_callback_id = + CPUThreadConfigCallback::AddConfigChangedCallback([this]() { RefreshConfig(); }); RefreshConfig(); m_last_oc_factor = m_config_oc_factor; @@ -118,7 +119,7 @@ void CoreTimingManager::Shutdown() MoveEvents(); ClearPendingEvents(); UnregisterAllEvents(); - Config::RemoveConfigChangedCallback(m_registered_config_callback_id); + CPUThreadConfigCallback::RemoveConfigChangedCallback(m_registered_config_callback_id); } void CoreTimingManager::RefreshConfig() @@ -311,11 +312,13 @@ void CoreTimingManager::MoveEvents() void CoreTimingManager::Advance() { - auto& power_pc = m_system.GetPowerPC(); - auto& ppc_state = power_pc.GetPPCState(); + CPUThreadConfigCallback::CheckForConfigChanges(); MoveEvents(); + auto& power_pc = m_system.GetPowerPC(); + auto& ppc_state = power_pc.GetPPCState(); + int cyclesExecuted = m_globals.slice_length - DowncountToCycles(ppc_state.downcount); m_globals.global_timer += cyclesExecuted; m_last_oc_factor = m_config_oc_factor; diff --git a/Source/Core/Core/FreeLookConfig.cpp b/Source/Core/Core/FreeLookConfig.cpp index 4f9ca377a1..31a673ce85 100644 --- a/Source/Core/Core/FreeLookConfig.cpp +++ b/Source/Core/Core/FreeLookConfig.cpp @@ -2,6 +2,8 @@ // SPDX-License-Identifier: GPL-2.0-or-later #include "Core/FreeLookConfig.h" + +#include "Core/CPUThreadConfigCallback.h" #include "Core/Config/FreeLookSettings.h" #include "Core/ConfigManager.h" #include "Core/Core.h" @@ -37,7 +39,7 @@ void Config::Refresh() { if (!s_has_registered_callback) { - ::Config::AddConfigChangedCallback([] { Core::RunAsCPUThread([] { s_config.Refresh(); }); }); + CPUThreadConfigCallback::AddConfigChangedCallback([] { s_config.Refresh(); }); s_has_registered_callback = true; } diff --git a/Source/Core/Core/HW/CPU.cpp b/Source/Core/Core/HW/CPU.cpp index aa90ab54d2..8a9634d527 100644 --- a/Source/Core/Core/HW/CPU.cpp +++ b/Source/Core/Core/HW/CPU.cpp @@ -10,6 +10,7 @@ #include "AudioCommon/AudioCommon.h" #include "Common/CommonTypes.h" #include "Common/Event.h" +#include "Core/CPUThreadConfigCallback.h" #include "Core/Core.h" #include "Core/Host.h" #include "Core/PowerPC/GDBStub.h" @@ -75,6 +76,7 @@ void CPUManager::Run() { m_state_cpu_cvar.wait(state_lock, [this] { return !m_state_paused_and_locked; }); ExecutePendingJobs(state_lock); + CPUThreadConfigCallback::CheckForConfigChanges(); Common::Event gdb_step_sync_event; switch (m_state) @@ -113,6 +115,7 @@ void CPUManager::Run() // Wait for step command. m_state_cpu_cvar.wait(state_lock, [this, &state_lock, &gdb_step_sync_event] { ExecutePendingJobs(state_lock); + CPUThreadConfigCallback::CheckForConfigChanges(); state_lock.unlock(); if (GDBStub::IsActive() && GDBStub::HasControl()) { diff --git a/Source/Core/Core/IOS/SDIO/SDIOSlot0.cpp b/Source/Core/Core/IOS/SDIO/SDIOSlot0.cpp index 55ee8e771b..6339ec087a 100644 --- a/Source/Core/Core/IOS/SDIO/SDIOSlot0.cpp +++ b/Source/Core/Core/IOS/SDIO/SDIOSlot0.cpp @@ -15,6 +15,7 @@ #include "Common/Logging/Log.h" #include "Common/SDCardUtil.h" +#include "Core/CPUThreadConfigCallback.h" #include "Core/Config/MainSettings.h" #include "Core/Config/SessionSettings.h" #include "Core/Core.h" @@ -32,27 +33,23 @@ SDIOSlot0Device::SDIOSlot0Device(EmulationKernel& ios, const std::string& device if (!Config::Get(Config::MAIN_ALLOW_SD_WRITES)) INFO_LOG_FMT(IOS_SD, "Writes to SD card disabled by user"); - m_config_callback_id = Config::AddConfigChangedCallback([this] { RefreshConfig(); }); + m_config_callback_id = + CPUThreadConfigCallback::AddConfigChangedCallback([this] { RefreshConfig(); }); m_sd_card_inserted = Config::Get(Config::MAIN_WII_SD_CARD); } SDIOSlot0Device::~SDIOSlot0Device() { - Config::RemoveConfigChangedCallback(m_config_callback_id); + CPUThreadConfigCallback::RemoveConfigChangedCallback(m_config_callback_id); } void SDIOSlot0Device::RefreshConfig() { - if (m_sd_card_inserted != Config::Get(Config::MAIN_WII_SD_CARD)) + const bool sd_card_inserted = Config::Get(Config::MAIN_WII_SD_CARD); + if (m_sd_card_inserted != sd_card_inserted) { - Core::RunAsCPUThread([this] { - const bool sd_card_inserted = Config::Get(Config::MAIN_WII_SD_CARD); - if (m_sd_card_inserted != sd_card_inserted) - { - m_sd_card_inserted = sd_card_inserted; - EventNotify(); - } - }); + m_sd_card_inserted = sd_card_inserted; + EventNotify(); } } diff --git a/Source/Core/Core/PowerPC/JitCommon/JitBase.cpp b/Source/Core/Core/PowerPC/JitCommon/JitBase.cpp index 887e73a402..17d9fe1bea 100644 --- a/Source/Core/Core/PowerPC/JitCommon/JitBase.cpp +++ b/Source/Core/Core/PowerPC/JitCommon/JitBase.cpp @@ -7,6 +7,8 @@ #include "Common/CommonTypes.h" #include "Common/MemoryUtil.h" #include "Common/Thread.h" + +#include "Core/CPUThreadConfigCallback.h" #include "Core/Config/MainSettings.h" #include "Core/ConfigManager.h" #include "Core/Core.h" @@ -64,14 +66,14 @@ JitBase::JitBase(Core::System& system) : m_code_buffer(code_buffer_size), m_system(system), m_ppc_state(system.GetPPCState()), m_mmu(system.GetMMU()) { - m_registered_config_callback_id = Config::AddConfigChangedCallback( - [this] { Core::RunAsCPUThread([this] { RefreshConfig(); }); }); + m_registered_config_callback_id = + CPUThreadConfigCallback::AddConfigChangedCallback([this] { RefreshConfig(); }); RefreshConfig(); } JitBase::~JitBase() { - Config::RemoveConfigChangedCallback(m_registered_config_callback_id); + CPUThreadConfigCallback::RemoveConfigChangedCallback(m_registered_config_callback_id); } void JitBase::RefreshConfig() diff --git a/Source/Core/DolphinLib.props b/Source/Core/DolphinLib.props index 1b44f15ec4..882471aabf 100644 --- a/Source/Core/DolphinLib.props +++ b/Source/Core/DolphinLib.props @@ -190,6 +190,7 @@ + @@ -833,6 +834,7 @@ + diff --git a/Source/Core/VideoCommon/VideoConfig.cpp b/Source/Core/VideoCommon/VideoConfig.cpp index 42bf178047..b00976b9b4 100644 --- a/Source/Core/VideoCommon/VideoConfig.cpp +++ b/Source/Core/VideoCommon/VideoConfig.cpp @@ -9,6 +9,7 @@ #include "Common/CommonTypes.h" #include "Common/StringUtil.h" +#include "Core/CPUThreadConfigCallback.h" #include "Core/Config/GraphicsSettings.h" #include "Core/Config/MainSettings.h" #include "Core/ConfigManager.h" @@ -19,6 +20,7 @@ #include "VideoCommon/AbstractGfx.h" #include "VideoCommon/BPFunctions.h" #include "VideoCommon/DriverDetails.h" +#include "VideoCommon/Fifo.h" #include "VideoCommon/FramebufferManager.h" #include "VideoCommon/FreeLookCamera.h" #include "VideoCommon/GraphicsModSystem/Config/GraphicsMod.h" @@ -57,14 +59,21 @@ void VideoConfig::Refresh() { // There was a race condition between the video thread and the host thread here, if // corrections need to be made by VerifyValidity(). Briefly, the config will contain - // invalid values. Instead, pause emulation first, which will flush the video thread, - // update the config and correct it, then resume emulation, after which the video - // thread will detect the config has changed and act accordingly. - Config::AddConfigChangedCallback([]() { - Core::RunAsCPUThread([]() { - g_Config.Refresh(); - g_Config.VerifyValidity(); - }); + // invalid values. Instead, pause the video thread first, update the config and correct + // it, then resume emulation, after which the video thread will detect the config has + // changed and act accordingly. + CPUThreadConfigCallback::AddConfigChangedCallback([]() { + auto& system = Core::System::GetInstance(); + + const bool lock_gpu_thread = Core::IsRunningAndStarted(); + if (lock_gpu_thread) + system.GetFifo().PauseAndLock(system, true, false); + + g_Config.Refresh(); + g_Config.VerifyValidity(); + + if (lock_gpu_thread) + system.GetFifo().PauseAndLock(system, false, true); }); s_has_registered_callback = true; }