From 71ce8bb6f00f4d1cbc1012270d6daefdbda4254d Mon Sep 17 00:00:00 2001 From: JosJuice Date: Wed, 16 Aug 2023 21:16:41 +0200 Subject: [PATCH 1/5] 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; } From 2b17e89336ed09db7298ca66a7c697a4db9d3cf9 Mon Sep 17 00:00:00 2001 From: JosJuice Date: Wed, 16 Aug 2023 22:16:50 +0200 Subject: [PATCH 2/5] Config: Don't clear callbacks on shutdown This fixes a problem that started happening in CoreTimingTest after the previous commit. CPUThreadConfigCallback registers a Config callback only once per run of the process, but CoreTimingTest calls Config::Shutdown after each test, and Config::Shutdown was clearing all callbacks, preventing the callback from running after that. --- Source/Core/Common/Config/Config.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/Source/Core/Common/Config/Config.cpp b/Source/Core/Common/Config/Config.cpp index ff3a8c6783..4db249d8eb 100644 --- a/Source/Core/Common/Config/Config.cpp +++ b/Source/Core/Common/Config/Config.cpp @@ -138,7 +138,6 @@ void Shutdown() WriteLock lock(s_layers_rw_lock); s_layers.clear(); - s_callbacks.clear(); } void ClearCurrentRunLayer() From 1104b93ee4944313d39bb7869dcba1ff21758d6d Mon Sep 17 00:00:00 2001 From: JosJuice Date: Wed, 16 Aug 2023 23:28:33 +0200 Subject: [PATCH 3/5] UnitTests: Declare as CPU thread when using CPUThreadConfigCallback This fixes a bunch of DEBUG_ASSERTs in the unit tests. --- Source/UnitTests/Core/PageFaultTest.cpp | 5 +++++ .../Core/PowerPC/Jit64Common/ConvertDoubleToSingle.cpp | 5 +++++ Source/UnitTests/Core/PowerPC/Jit64Common/Frsqrte.cpp | 5 +++++ .../Core/PowerPC/JitArm64/ConvertSingleDouble.cpp | 8 ++++++++ Source/UnitTests/Core/PowerPC/JitArm64/FPRF.cpp | 5 +++++ Source/UnitTests/Core/PowerPC/JitArm64/Fres.cpp | 5 +++++ Source/UnitTests/Core/PowerPC/JitArm64/Frsqrte.cpp | 5 +++++ 7 files changed, 38 insertions(+) diff --git a/Source/UnitTests/Core/PageFaultTest.cpp b/Source/UnitTests/Core/PageFaultTest.cpp index ab0cdd7bcc..3f693923e0 100644 --- a/Source/UnitTests/Core/PageFaultTest.cpp +++ b/Source/UnitTests/Core/PageFaultTest.cpp @@ -5,7 +5,9 @@ #include #include "Common/CommonTypes.h" +#include "Common/ScopeGuard.h" #include "Common/Timer.h" +#include "Core/Core.h" #include "Core/MemTools.h" #include "Core/PowerPC/JitCommon/JitBase.h" #include "Core/PowerPC/JitInterface.h" @@ -75,6 +77,9 @@ TEST(PageFault, PageFault) EXPECT_NE(data, nullptr); Common::WriteProtectMemory(data, PAGE_GRAN, false); + Core::DeclareAsCPUThread(); + Common::ScopeGuard cpu_thread_guard([] { Core::UndeclareAsCPUThread(); }); + auto& system = Core::System::GetInstance(); auto unique_pfjit = std::make_unique(system); auto& pfjit = *unique_pfjit; diff --git a/Source/UnitTests/Core/PowerPC/Jit64Common/ConvertDoubleToSingle.cpp b/Source/UnitTests/Core/PowerPC/Jit64Common/ConvertDoubleToSingle.cpp index bbf6445357..09c0f30169 100644 --- a/Source/UnitTests/Core/PowerPC/Jit64Common/ConvertDoubleToSingle.cpp +++ b/Source/UnitTests/Core/PowerPC/Jit64Common/ConvertDoubleToSingle.cpp @@ -5,7 +5,9 @@ #include #include "Common/CommonTypes.h" +#include "Common/ScopeGuard.h" #include "Common/x64ABI.h" +#include "Core/Core.h" #include "Core/PowerPC/Gekko.h" #include "Core/PowerPC/Interpreter/Interpreter_FPUtils.h" #include "Core/PowerPC/Jit64/Jit.h" @@ -52,6 +54,9 @@ public: TEST(Jit64, ConvertDoubleToSingle) { + Core::DeclareAsCPUThread(); + Common::ScopeGuard cpu_thread_guard([] { Core::UndeclareAsCPUThread(); }); + TestCommonAsmRoutines routines(Core::System::GetInstance()); for (const u64 input : double_test_values) diff --git a/Source/UnitTests/Core/PowerPC/Jit64Common/Frsqrte.cpp b/Source/UnitTests/Core/PowerPC/Jit64Common/Frsqrte.cpp index d0ce6b3653..3300c07541 100644 --- a/Source/UnitTests/Core/PowerPC/Jit64Common/Frsqrte.cpp +++ b/Source/UnitTests/Core/PowerPC/Jit64Common/Frsqrte.cpp @@ -6,7 +6,9 @@ #include "Common/BitUtils.h" #include "Common/CommonTypes.h" #include "Common/FloatUtils.h" +#include "Common/ScopeGuard.h" #include "Common/x64ABI.h" +#include "Core/Core.h" #include "Core/PowerPC/Gekko.h" #include "Core/PowerPC/Jit64/Jit.h" #include "Core/PowerPC/Jit64Common/Jit64AsmCommon.h" @@ -59,6 +61,9 @@ public: TEST(Jit64, Frsqrte) { + Core::DeclareAsCPUThread(); + Common::ScopeGuard cpu_thread_guard([] { Core::UndeclareAsCPUThread(); }); + TestCommonAsmRoutines routines(Core::System::GetInstance()); UReg_FPSCR fpscr; diff --git a/Source/UnitTests/Core/PowerPC/JitArm64/ConvertSingleDouble.cpp b/Source/UnitTests/Core/PowerPC/JitArm64/ConvertSingleDouble.cpp index 0e87fddcda..588a1622ef 100644 --- a/Source/UnitTests/Core/PowerPC/JitArm64/ConvertSingleDouble.cpp +++ b/Source/UnitTests/Core/PowerPC/JitArm64/ConvertSingleDouble.cpp @@ -7,6 +7,8 @@ #include "Common/BitUtils.h" #include "Common/CommonTypes.h" #include "Common/FPURoundMode.h" +#include "Common/ScopeGuard.h" +#include "Core/Core.h" #include "Core/PowerPC/Interpreter/Interpreter_FPUtils.h" #include "Core/PowerPC/JitArm64/Jit.h" #include "Core/System.h" @@ -120,6 +122,9 @@ private: TEST(JitArm64, ConvertDoubleToSingle) { + Core::DeclareAsCPUThread(); + Common::ScopeGuard cpu_thread_guard([] { Core::UndeclareAsCPUThread(); }); + TestConversion test(Core::System::GetInstance()); for (const u64 input : double_test_values) @@ -155,6 +160,9 @@ TEST(JitArm64, ConvertDoubleToSingle) TEST(JitArm64, ConvertSingleToDouble) { + Core::DeclareAsCPUThread(); + Common::ScopeGuard cpu_thread_guard([] { Core::UndeclareAsCPUThread(); }); + TestConversion test(Core::System::GetInstance()); for (const u32 input : single_test_values) diff --git a/Source/UnitTests/Core/PowerPC/JitArm64/FPRF.cpp b/Source/UnitTests/Core/PowerPC/JitArm64/FPRF.cpp index 285469b90f..0b7e250ae6 100644 --- a/Source/UnitTests/Core/PowerPC/JitArm64/FPRF.cpp +++ b/Source/UnitTests/Core/PowerPC/JitArm64/FPRF.cpp @@ -7,6 +7,8 @@ #include "Common/Arm64Emitter.h" #include "Common/BitUtils.h" #include "Common/CommonTypes.h" +#include "Common/ScopeGuard.h" +#include "Core/Core.h" #include "Core/PowerPC/Interpreter/Interpreter_FPUtils.h" #include "Core/PowerPC/JitArm64/Jit.h" #include "Core/PowerPC/PowerPC.h" @@ -70,6 +72,9 @@ static u32 RunUpdateFPRF(PowerPC::PowerPCState& ppc_state, const std::function Date: Wed, 16 Aug 2023 21:37:12 +0200 Subject: [PATCH 4/5] Use structs for config callback IDs This way you can't mix up regular config callback IDs and CPU thread config callback IDs. (It would be rather bad if you did!) --- Source/Core/AudioCommon/Mixer.h | 3 ++- Source/Core/Common/Config/Config.cpp | 8 ++++---- Source/Core/Common/Config/Config.h | 12 ++++++++++-- Source/Core/Core/CPUThreadConfigCallback.cpp | 13 ++++++++----- Source/Core/Core/CPUThreadConfigCallback.h | 14 +++++++++++--- Source/Core/Core/CoreTiming.h | 3 ++- Source/Core/Core/FifoPlayer/FifoPlayer.h | 3 ++- Source/Core/Core/HW/Wiimote.cpp | 2 +- Source/Core/Core/HW/WiimoteEmu/WiimoteEmu.h | 3 ++- Source/Core/Core/HW/WiimoteReal/WiimoteReal.h | 3 ++- Source/Core/Core/IOS/SDIO/SDIOSlot0.h | 3 ++- Source/Core/Core/PowerPC/JitCommon/JitBase.h | 3 ++- Source/Core/Core/PowerPC/PPCCache.h | 3 ++- .../DualShockUDPClient/DualShockUDPClient.cpp | 2 +- Source/Core/InputCommon/GCAdapter.cpp | 3 ++- Source/Core/UICommon/UICommon.cpp | 2 +- Source/Core/VideoCommon/Fifo.h | 3 ++- 17 files changed, 56 insertions(+), 27 deletions(-) diff --git a/Source/Core/AudioCommon/Mixer.h b/Source/Core/AudioCommon/Mixer.h index c27ff311c3..5a71f5e8af 100644 --- a/Source/Core/AudioCommon/Mixer.h +++ b/Source/Core/AudioCommon/Mixer.h @@ -10,6 +10,7 @@ #include "AudioCommon/SurroundDecoder.h" #include "AudioCommon/WaveFile.h" #include "Common/CommonTypes.h" +#include "Common/Config/Config.h" class PointerWrap; @@ -120,5 +121,5 @@ private: int m_config_timing_variance; bool m_config_audio_stretch; - size_t m_config_changed_callback_id; + Config::ConfigChangedCallbackID m_config_changed_callback_id; }; diff --git a/Source/Core/Common/Config/Config.cpp b/Source/Core/Common/Config/Config.cpp index 4db249d8eb..3f008fc913 100644 --- a/Source/Core/Common/Config/Config.cpp +++ b/Source/Core/Common/Config/Config.cpp @@ -16,7 +16,7 @@ namespace Config using Layers = std::map>; static Layers s_layers; -static std::vector> s_callbacks; +static std::vector> s_callbacks; static size_t s_next_callback_id = 0; static u32 s_callback_guards = 0; static std::atomic s_config_version = 0; @@ -65,15 +65,15 @@ void RemoveLayer(LayerType layer) OnConfigChanged(); } -size_t AddConfigChangedCallback(ConfigChangedCallback func) +ConfigChangedCallbackID AddConfigChangedCallback(ConfigChangedCallback func) { - const size_t callback_id = s_next_callback_id; + const ConfigChangedCallbackID 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) +void RemoveConfigChangedCallback(ConfigChangedCallbackID callback_id) { for (auto it = s_callbacks.begin(); it != s_callbacks.end(); ++it) { diff --git a/Source/Core/Common/Config/Config.h b/Source/Core/Common/Config/Config.h index 4d770ee36e..1303388566 100644 --- a/Source/Core/Common/Config/Config.h +++ b/Source/Core/Common/Config/Config.h @@ -15,6 +15,14 @@ namespace Config { +struct ConfigChangedCallbackID +{ + size_t id = -1; + + bool operator==(const ConfigChangedCallbackID&) const = default; + bool operator!=(const ConfigChangedCallbackID&) const = default; +}; + using ConfigChangedCallback = std::function; // Layer management @@ -24,8 +32,8 @@ void RemoveLayer(LayerType layer); // 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); +ConfigChangedCallbackID AddConfigChangedCallback(ConfigChangedCallback func); +void RemoveConfigChangedCallback(ConfigChangedCallbackID callback_id); void OnConfigChanged(); // Returns the number of times the config has changed in the current execution of the program diff --git a/Source/Core/Core/CPUThreadConfigCallback.cpp b/Source/Core/Core/CPUThreadConfigCallback.cpp index 2148630c22..857ba24189 100644 --- a/Source/Core/Core/CPUThreadConfigCallback.cpp +++ b/Source/Core/Core/CPUThreadConfigCallback.cpp @@ -13,7 +13,10 @@ namespace { std::atomic s_should_run_callbacks = false; -static std::vector> s_callbacks; +static std::vector< + std::pair> + s_callbacks; + static size_t s_next_callback_id = 0; void RunCallbacks() @@ -39,19 +42,19 @@ void OnConfigChanged() namespace CPUThreadConfigCallback { -size_t AddConfigChangedCallback(Config::ConfigChangedCallback func) +ConfigChangedCallbackID AddConfigChangedCallback(Config::ConfigChangedCallback func) { DEBUG_ASSERT(Core::IsCPUThread()); - static size_t s_config_changed_callback_id = Config::AddConfigChangedCallback(&OnConfigChanged); + static auto s_config_changed_callback_id = Config::AddConfigChangedCallback(&OnConfigChanged); - const size_t callback_id = s_next_callback_id; + const ConfigChangedCallbackID 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) +void RemoveConfigChangedCallback(ConfigChangedCallbackID callback_id) { DEBUG_ASSERT(Core::IsCPUThread()); diff --git a/Source/Core/Core/CPUThreadConfigCallback.h b/Source/Core/Core/CPUThreadConfigCallback.h index c43e01a9a0..404e522809 100644 --- a/Source/Core/Core/CPUThreadConfigCallback.h +++ b/Source/Core/Core/CPUThreadConfigCallback.h @@ -11,10 +11,18 @@ namespace CPUThreadConfigCallback { -// returns an ID that can be passed to RemoveConfigChangedCallback() -size_t AddConfigChangedCallback(Config::ConfigChangedCallback func); +struct ConfigChangedCallbackID +{ + size_t id = -1; -void RemoveConfigChangedCallback(size_t callback_id); + bool operator==(const ConfigChangedCallbackID&) const = default; + bool operator!=(const ConfigChangedCallbackID&) const = default; +}; + +// returns an ID that can be passed to RemoveConfigChangedCallback() +ConfigChangedCallbackID AddConfigChangedCallback(Config::ConfigChangedCallback func); + +void RemoveConfigChangedCallback(ConfigChangedCallbackID callback_id); // Should be called regularly from the CPU thread void CheckForConfigChanges(); diff --git a/Source/Core/Core/CoreTiming.h b/Source/Core/Core/CoreTiming.h index 1790dd70ee..6c60b74479 100644 --- a/Source/Core/Core/CoreTiming.h +++ b/Source/Core/Core/CoreTiming.h @@ -23,6 +23,7 @@ #include "Common/CommonTypes.h" #include "Common/SPSCQueue.h" +#include "Core/CPUThreadConfigCallback.h" class PointerWrap; @@ -182,7 +183,7 @@ private: EventType* m_ev_lost = nullptr; - size_t m_registered_config_callback_id = 0; + CPUThreadConfigCallback::ConfigChangedCallbackID m_registered_config_callback_id; float m_config_oc_factor = 0.0f; float m_config_oc_inv_factor = 0.0f; bool m_config_sync_on_skip_idle = false; diff --git a/Source/Core/Core/FifoPlayer/FifoPlayer.h b/Source/Core/Core/FifoPlayer/FifoPlayer.h index 365add85fe..b1cbd7a12d 100644 --- a/Source/Core/Core/FifoPlayer/FifoPlayer.h +++ b/Source/Core/Core/FifoPlayer/FifoPlayer.h @@ -10,6 +10,7 @@ #include #include "Common/Assert.h" +#include "Common/Config/Config.h" #include "Core/FifoPlayer/FifoDataFile.h" #include "Core/PowerPC/CPUCoreBase.h" #include "VideoCommon/CPMemory.h" @@ -189,7 +190,7 @@ private: CallbackFunc m_FileLoadedCb = nullptr; CallbackFunc m_FrameWrittenCb = nullptr; - size_t m_config_changed_callback_id; + Config::ConfigChangedCallbackID m_config_changed_callback_id; std::unique_ptr m_File; diff --git a/Source/Core/Core/HW/Wiimote.cpp b/Source/Core/Core/HW/Wiimote.cpp index 580bff8aee..13ef2139f6 100644 --- a/Source/Core/Core/HW/Wiimote.cpp +++ b/Source/Core/Core/HW/Wiimote.cpp @@ -30,7 +30,7 @@ static std::array s_last_connect_request_counter; namespace { static std::array, MAX_BBMOTES> s_wiimote_sources; -static std::optional s_config_callback_id = std::nullopt; +static std::optional s_config_callback_id = std::nullopt; WiimoteSource GetSource(unsigned int index) { diff --git a/Source/Core/Core/HW/WiimoteEmu/WiimoteEmu.h b/Source/Core/Core/HW/WiimoteEmu/WiimoteEmu.h index eb45dce496..116e393bf2 100644 --- a/Source/Core/Core/HW/WiimoteEmu/WiimoteEmu.h +++ b/Source/Core/Core/HW/WiimoteEmu/WiimoteEmu.h @@ -9,6 +9,7 @@ #include #include "Common/Common.h" +#include "Common/Config/Config.h" #include "Core/HW/WiimoteCommon/WiimoteReport.h" @@ -343,6 +344,6 @@ private: IMUCursorState m_imu_cursor_state; - size_t m_config_changed_callback_id; + Config::ConfigChangedCallbackID m_config_changed_callback_id; }; } // namespace WiimoteEmu diff --git a/Source/Core/Core/HW/WiimoteReal/WiimoteReal.h b/Source/Core/Core/HW/WiimoteReal/WiimoteReal.h index 39f13f0f44..eccf7bb22d 100644 --- a/Source/Core/Core/HW/WiimoteReal/WiimoteReal.h +++ b/Source/Core/Core/HW/WiimoteReal/WiimoteReal.h @@ -11,6 +11,7 @@ #include #include "Common/Common.h" +#include "Common/Config/Config.h" #include "Common/Event.h" #include "Common/Flag.h" #include "Common/SPSCQueue.h" @@ -157,7 +158,7 @@ private: bool m_speaker_enabled_in_dolphin_config = false; int m_balance_board_dump_port = 0; - size_t m_config_changed_callback_id; + Config::ConfigChangedCallbackID m_config_changed_callback_id; }; class WiimoteScannerBackend diff --git a/Source/Core/Core/IOS/SDIO/SDIOSlot0.h b/Source/Core/Core/IOS/SDIO/SDIOSlot0.h index c096539e70..5c0dd201a7 100644 --- a/Source/Core/Core/IOS/SDIO/SDIOSlot0.h +++ b/Source/Core/Core/IOS/SDIO/SDIOSlot0.h @@ -10,6 +10,7 @@ #include "Common/CommonTypes.h" #include "Common/IOFile.h" +#include "Core/CPUThreadConfigCallback.h" #include "Core/IOS/Device.h" #include "Core/IOS/IOS.h" @@ -166,7 +167,7 @@ private: File::IOFile m_card; - size_t m_config_callback_id; + CPUThreadConfigCallback::ConfigChangedCallbackID m_config_callback_id; bool m_sd_card_inserted = false; }; } // namespace IOS::HLE diff --git a/Source/Core/Core/PowerPC/JitCommon/JitBase.h b/Source/Core/Core/PowerPC/JitCommon/JitBase.h index e6488960e7..c2e41f8e52 100644 --- a/Source/Core/Core/PowerPC/JitCommon/JitBase.h +++ b/Source/Core/Core/PowerPC/JitCommon/JitBase.h @@ -10,6 +10,7 @@ #include "Common/BitSet.h" #include "Common/CommonTypes.h" #include "Common/x64Emitter.h" +#include "Core/CPUThreadConfigCallback.h" #include "Core/ConfigManager.h" #include "Core/MachineContext.h" #include "Core/PowerPC/CPUCoreBase.h" @@ -129,7 +130,7 @@ protected: PPCAnalyst::CodeBuffer m_code_buffer; PPCAnalyst::PPCAnalyzer analyzer; - size_t m_registered_config_callback_id; + CPUThreadConfigCallback::ConfigChangedCallbackID m_registered_config_callback_id; bool bJITOff = false; bool bJITLoadStoreOff = false; bool bJITLoadStorelXzOff = false; diff --git a/Source/Core/Core/PowerPC/PPCCache.h b/Source/Core/Core/PowerPC/PPCCache.h index 8214b116af..5d5648a71e 100644 --- a/Source/Core/Core/PowerPC/PPCCache.h +++ b/Source/Core/Core/PowerPC/PPCCache.h @@ -8,6 +8,7 @@ #include #include "Common/CommonTypes.h" +#include "Common/Config/Config.h" class PointerWrap; @@ -61,7 +62,7 @@ struct Cache struct InstructionCache : public Cache { - std::optional m_config_callback_id = std::nullopt; + std::optional m_config_callback_id = std::nullopt; bool m_disable_icache = false; diff --git a/Source/Core/InputCommon/ControllerInterface/DualShockUDPClient/DualShockUDPClient.cpp b/Source/Core/InputCommon/ControllerInterface/DualShockUDPClient/DualShockUDPClient.cpp index 4d3fa483e3..d3030a60fa 100644 --- a/Source/Core/InputCommon/ControllerInterface/DualShockUDPClient/DualShockUDPClient.cpp +++ b/Source/Core/InputCommon/ControllerInterface/DualShockUDPClient/DualShockUDPClient.cpp @@ -219,7 +219,7 @@ private: SteadyClock::time_point m_next_listports_time; std::thread m_hotplug_thread; Common::Flag m_hotplug_thread_running; - std::size_t m_config_change_callback_id; + Config::ConfigChangedCallbackID m_config_change_callback_id; }; std::unique_ptr CreateInputBackend(ControllerInterface* controller_interface) diff --git a/Source/Core/InputCommon/GCAdapter.cpp b/Source/Core/InputCommon/GCAdapter.cpp index 6334a23778..2d135970d1 100644 --- a/Source/Core/InputCommon/GCAdapter.cpp +++ b/Source/Core/InputCommon/GCAdapter.cpp @@ -23,6 +23,7 @@ #endif #include "Common/BitUtils.h" +#include "Common/Config/Config.h" #include "Common/Event.h" #include "Common/Flag.h" #include "Common/Logging/Log.h" @@ -158,7 +159,7 @@ static u8 s_endpoint_out = 0; static u64 s_last_init = 0; -static std::optional s_config_callback_id = std::nullopt; +static std::optional s_config_callback_id = std::nullopt; static bool s_is_adapter_wanted = false; static std::array s_config_rumble_enabled{}; diff --git a/Source/Core/UICommon/UICommon.cpp b/Source/Core/UICommon/UICommon.cpp index 2acd69f2c2..9b31ad5bf9 100644 --- a/Source/Core/UICommon/UICommon.cpp +++ b/Source/Core/UICommon/UICommon.cpp @@ -62,7 +62,7 @@ namespace UICommon { -static size_t s_config_changed_callback_id; +static Config::ConfigChangedCallbackID s_config_changed_callback_id; static void CreateDumpPath(std::string path) { diff --git a/Source/Core/VideoCommon/Fifo.h b/Source/Core/VideoCommon/Fifo.h index 20648562b4..2ee237c782 100644 --- a/Source/Core/VideoCommon/Fifo.h +++ b/Source/Core/VideoCommon/Fifo.h @@ -9,6 +9,7 @@ #include "Common/BlockingLoop.h" #include "Common/CommonTypes.h" +#include "Common/Config/Config.h" #include "Common/Event.h" #include "Common/Flag.h" @@ -121,7 +122,7 @@ private: bool m_syncing_suspended = false; Common::Event m_sync_wakeup_event; - std::optional m_config_callback_id = std::nullopt; + std::optional m_config_callback_id = std::nullopt; bool m_config_sync_gpu = false; int m_config_sync_gpu_max_distance = 0; int m_config_sync_gpu_min_distance = 0; From b62c25864f6d549976b94eda60763524413e896e Mon Sep 17 00:00:00 2001 From: JosJuice Date: Thu, 17 Aug 2023 09:15:09 +0200 Subject: [PATCH 5/5] CPUThreadConfigCallback: Remove some CPU thread asserts Turns out that we have two subsystems that want to register CPU thread callbacks from a different thread than the CPU thread: FreeLookConfig and VideoConfig. Both seem to happen from the host thread before the CPU thread starts, so there's no thread safety issue. But ideally, if we want to allow registering callbacks from threads other than the CPU thread, we should make registering callbacks actually thread-safe. This is an unsolved problem for the regular Config system, so I would like to leave it outside the scope of this PR. --- Source/Core/Core/CPUThreadConfigCallback.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/Source/Core/Core/CPUThreadConfigCallback.cpp b/Source/Core/Core/CPUThreadConfigCallback.cpp index 857ba24189..a5b619f27a 100644 --- a/Source/Core/Core/CPUThreadConfigCallback.cpp +++ b/Source/Core/Core/CPUThreadConfigCallback.cpp @@ -44,8 +44,6 @@ namespace CPUThreadConfigCallback { ConfigChangedCallbackID AddConfigChangedCallback(Config::ConfigChangedCallback func) { - DEBUG_ASSERT(Core::IsCPUThread()); - static auto s_config_changed_callback_id = Config::AddConfigChangedCallback(&OnConfigChanged); const ConfigChangedCallbackID callback_id{s_next_callback_id}; @@ -56,8 +54,6 @@ ConfigChangedCallbackID AddConfigChangedCallback(Config::ConfigChangedCallback f void RemoveConfigChangedCallback(ConfigChangedCallbackID callback_id) { - DEBUG_ASSERT(Core::IsCPUThread()); - for (auto it = s_callbacks.begin(); it != s_callbacks.end(); ++it) { if (it->first == callback_id)