From 3367e5e0267f17e02b57ec5c55526ad561d7b11d Mon Sep 17 00:00:00 2001 From: JosJuice Date: Sat, 25 Apr 2020 00:26:51 +0200 Subject: [PATCH 1/2] DolphinQt: Fix the panic alert deadlock, GPU thread edition The fix in ef77872 worked for panic alerts from the CPU thread, but there were still problems with panic alerts from the GPU thread in dual core mode. This change attempts to fix those. --- Source/Core/Core/Core.cpp | 22 ++++++++++++++-------- Source/Core/Core/Core.h | 2 ++ Source/Core/DolphinQt/Host.cpp | 23 +++++++++++++++++++++-- Source/Core/DolphinQt/Main.cpp | 25 ++++++++++++++++++------- Source/Core/DolphinQt/RenderWidget.cpp | 8 ++++---- 5 files changed, 59 insertions(+), 21 deletions(-) diff --git a/Source/Core/Core/Core.cpp b/Source/Core/Core/Core.cpp index 1687c9900f..de3ac7f009 100644 --- a/Source/Core/Core/Core.cpp +++ b/Source/Core/Core/Core.cpp @@ -127,6 +127,7 @@ static std::queue s_host_jobs_queue; 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 void EmuThread(std::unique_ptr boot, WindowSystemInfo wsi); @@ -203,14 +204,7 @@ bool IsCPUThread() bool IsGPUThread() { - if (Core::System::GetInstance().IsDualCoreMode()) - { - return (s_emu_thread.joinable() && (s_emu_thread.get_id() == std::this_thread::get_id())); - } - else - { - return IsCPUThread(); - } + return tls_is_gpu_thread; } bool WantsDeterminism() @@ -313,6 +307,16 @@ void UndeclareAsCPUThread() tls_is_cpu_thread = false; } +void DeclareAsGPUThread() +{ + tls_is_gpu_thread = true; +} + +void UndeclareAsGPUThread() +{ + tls_is_gpu_thread = false; +} + // For the CPU Thread only. static void CPUSetInitialExecutionState(bool force_paused = false) { @@ -459,6 +463,8 @@ static void EmuThread(std::unique_ptr boot, WindowSystemInfo wsi Common::SetCurrentThreadName("Emuthread - Starting"); + DeclareAsGPUThread(); + // For a time this acts as the CPU thread... DeclareAsCPUThread(); s_frame_step = false; diff --git a/Source/Core/Core/Core.h b/Source/Core/Core/Core.h index 52042b6409..1e6e240682 100644 --- a/Source/Core/Core/Core.h +++ b/Source/Core/Core/Core.h @@ -98,6 +98,8 @@ void Shutdown(); void DeclareAsCPUThread(); void UndeclareAsCPUThread(); +void DeclareAsGPUThread(); +void UndeclareAsGPUThread(); std::string StopMessage(bool main_thread, std::string_view message); diff --git a/Source/Core/DolphinQt/Host.cpp b/Source/Core/DolphinQt/Host.cpp index bc9d6103b3..9690c1bc5b 100644 --- a/Source/Core/DolphinQt/Host.cpp +++ b/Source/Core/DolphinQt/Host.cpp @@ -3,6 +3,8 @@ #include "DolphinQt/Host.h" +#include + #include #include #include @@ -85,6 +87,19 @@ void Host::SetMainWindowHandle(void* handle) m_main_window_handle = handle; } +static void RunWithGPUThreadInactive(std::function f) +{ + // If we are the GPU thread in dual core mode, we cannot safely call + // RunAsCPUThread, since RunAsCPUThread will need to pause the GPU thread + // and the GPU thread is waiting for us to finish. Fortunately, if we know + // that the GPU thread is waiting for us, we can just run f directly. + + if (Core::IsGPUThread()) + f(); + else + Core::RunAsCPUThread(std::move(f)); +} + bool Host::GetRenderFocus() { #ifdef _WIN32 @@ -107,10 +122,12 @@ void Host::SetRenderFocus(bool focus) { m_render_focus = focus; if (g_renderer && m_render_fullscreen && g_ActiveConfig.ExclusiveFullscreenEnabled()) - Core::RunAsCPUThread([focus] { + { + RunWithGPUThreadInactive([focus] { if (!Config::Get(Config::MAIN_RENDER_TO_MAIN)) g_renderer->SetFullscreen(focus); }); + } } void Host::SetRenderFullFocus(bool focus) @@ -138,7 +155,9 @@ void Host::SetRenderFullscreen(bool fullscreen) if (g_renderer && g_renderer->IsFullscreen() != fullscreen && g_ActiveConfig.ExclusiveFullscreenEnabled()) - Core::RunAsCPUThread([fullscreen] { g_renderer->SetFullscreen(fullscreen); }); + { + RunWithGPUThreadInactive([fullscreen] { g_renderer->SetFullscreen(fullscreen); }); + } } void Host::ResizeSurface(int new_width, int new_height) diff --git a/Source/Core/DolphinQt/Main.cpp b/Source/Core/DolphinQt/Main.cpp index 507765a143..8ffd52dbe5 100644 --- a/Source/Core/DolphinQt/Main.cpp +++ b/Source/Core/DolphinQt/Main.cpp @@ -41,20 +41,31 @@ static bool QtMsgAlertHandler(const char* caption, const char* text, bool yes_no Common::MsgType style) { const bool called_from_cpu_thread = Core::IsCPUThread(); + const bool called_from_gpu_thread = Core::IsGPUThread(); std::optional r = RunOnObject(QApplication::instance(), [&] { - Common::ScopeGuard scope_guard(&Core::UndeclareAsCPUThread); + Common::ScopeGuard cpu_scope_guard(&Core::UndeclareAsCPUThread); + Common::ScopeGuard gpu_scope_guard(&Core::UndeclareAsGPUThread); + + if (!called_from_cpu_thread) + cpu_scope_guard.Dismiss(); + if (!called_from_gpu_thread) + gpu_scope_guard.Dismiss(); + if (called_from_cpu_thread) { - // Temporarily declare this as the CPU thread to avoid getting a deadlock if any DolphinQt - // code calls RunAsCPUThread while the CPU thread is blocked on this function returning. - // Notably, if the panic alert steals focus from RenderWidget, Host::SetRenderFocus gets - // called, which can attempt to use RunAsCPUThread to get us out of exclusive fullscreen. + // If the panic alert that we are about to create steals the focus from RenderWidget, + // Host::SetRenderFocus gets called, which can attempt to use RunAsCPUThread to get us out + // of exclusive fullscreen. If we don't declare ourselves as the CPU thread, RunAsCPUThread + // calls PauseAndLock, which causes a deadlock if the CPU thread is waiting on us returning. Core::DeclareAsCPUThread(); } - else + if (called_from_gpu_thread) { - scope_guard.Dismiss(); + // We also need to avoid getting a deadlock when the GPU thread is waiting on us returning. + // Declaring ourselves as the GPU thread does not alter the behavior of RunAsCPUThread or + // PauseAndLock, but it does make Host::SetRenderFocus not call RunAsCPUThread. + Core::DeclareAsGPUThread(); } ModalMessageBox message_box(QApplication::activeWindow(), Qt::ApplicationModal); diff --git a/Source/Core/DolphinQt/RenderWidget.cpp b/Source/Core/DolphinQt/RenderWidget.cpp index 2430b7a415..6c34f7ebc6 100644 --- a/Source/Core/DolphinQt/RenderWidget.cpp +++ b/Source/Core/DolphinQt/RenderWidget.cpp @@ -421,10 +421,10 @@ bool RenderWidget::event(QEvent* event) if (Config::Get(Config::MAIN_PAUSE_ON_FOCUS_LOST) && Core::GetState() == Core::State::Running) { - // If we are declared as the CPU thread, it means that the real CPU thread is waiting - // for us to finish showing a panic alert (with that panic alert likely being the cause - // of this event), so trying to pause the real CPU thread would cause a deadlock - if (!Core::IsCPUThread()) + // If we are declared as the CPU or GPU thread, it means that the real CPU or GPU thread + // is waiting for us to finish showing a panic alert (with that panic alert likely being + // the cause of this event), so trying to pause the core would cause a deadlock + if (!Core::IsCPUThread() && !Core::IsGPUThread()) Core::SetState(Core::State::Paused); } From d445d2ad36223dfec0c432aa10bbbf5504c036c6 Mon Sep 17 00:00:00 2001 From: JosJuice Date: Sat, 25 Apr 2020 10:35:14 +0200 Subject: [PATCH 2/2] DolphinQt: Improve the earlier panic alert deadlock fix If the purpose of calling SetFullscreen using RunAsCPUThread is to make sure that the GPU thread is paused, the fix in ef77872 is faulty when dual core is used and a panic alert comes from the CPU thread. This change re-adds synchronization for that case. --- Source/Core/DolphinQt/Host.cpp | 34 ++++++++++++++++++++++++++++++---- Source/Core/DolphinQt/Main.cpp | 16 +++++----------- 2 files changed, 35 insertions(+), 15 deletions(-) diff --git a/Source/Core/DolphinQt/Host.cpp b/Source/Core/DolphinQt/Host.cpp index 9690c1bc5b..876ab25123 100644 --- a/Source/Core/DolphinQt/Host.cpp +++ b/Source/Core/DolphinQt/Host.cpp @@ -36,6 +36,7 @@ #include "UICommon/DiscordPresence.h" +#include "VideoCommon/Fifo.cpp" #include "VideoCommon/RenderBase.h" #include "VideoCommon/VideoConfig.h" @@ -89,15 +90,40 @@ void Host::SetMainWindowHandle(void* handle) static void RunWithGPUThreadInactive(std::function f) { - // If we are the GPU thread in dual core mode, we cannot safely call - // RunAsCPUThread, since RunAsCPUThread will need to pause the GPU thread - // and the GPU thread is waiting for us to finish. Fortunately, if we know - // that the GPU thread is waiting for us, we can just run f directly. + // Potentially any thread which shows panic alerts can be blocked on this returning. + // This means that, in order to avoid deadlocks, we need to be careful with how we + // synchronize with other threads. Note that the panic alert handler temporarily declares + // us as the CPU and/or GPU thread if the panic alert was requested by that thread. + + // TODO: What about the unlikely case where the GPU thread calls the panic alert handler + // while the panic alert handler is processing a panic alert from the CPU thread? if (Core::IsGPUThread()) + { + // If we are the GPU thread, we can't call Core::PauseAndLock without getting a deadlock, + // since it would try to pause the GPU thread while that thread is waiting for us. + // However, since we know that the GPU thread is inactive, we can just run f directly. + f(); + } + else if (Core::IsCPUThread()) + { + // If we are the CPU thread in dual core mode, we can't call Core::PauseAndLock, for the + // same reason as above. Instead, we use Fifo::PauseAndLock to pause the GPU thread only. + // (Note that this case cannot be reached in single core mode, because in single core mode, + // the CPU and GPU threads are the same thread, and we already checked for the GPU thread.) + + const bool was_running = Core::GetState() == Core::State::Running; + Fifo::PauseAndLock(true, was_running); + f(); + Fifo::PauseAndLock(false, was_running); + } else + { + // If we reach here, we can call Core::PauseAndLock (which we do using RunAsCPUThread). + Core::RunAsCPUThread(std::move(f)); + } } bool Host::GetRenderFocus() diff --git a/Source/Core/DolphinQt/Main.cpp b/Source/Core/DolphinQt/Main.cpp index 8ffd52dbe5..6d12a0ec5a 100644 --- a/Source/Core/DolphinQt/Main.cpp +++ b/Source/Core/DolphinQt/Main.cpp @@ -44,6 +44,11 @@ static bool QtMsgAlertHandler(const char* caption, const char* text, bool yes_no const bool called_from_gpu_thread = Core::IsGPUThread(); std::optional r = RunOnObject(QApplication::instance(), [&] { + // If we were called from the CPU/GPU thread, set us as the CPU/GPU thread. + // This information is used in order to avoid deadlocks when calling e.g. + // Host::SetRenderFocus or Core::RunAsCPUThread. (Host::SetRenderFocus + // can get called automatically when a dialog steals the focus.) + Common::ScopeGuard cpu_scope_guard(&Core::UndeclareAsCPUThread); Common::ScopeGuard gpu_scope_guard(&Core::UndeclareAsGPUThread); @@ -53,20 +58,9 @@ static bool QtMsgAlertHandler(const char* caption, const char* text, bool yes_no gpu_scope_guard.Dismiss(); if (called_from_cpu_thread) - { - // If the panic alert that we are about to create steals the focus from RenderWidget, - // Host::SetRenderFocus gets called, which can attempt to use RunAsCPUThread to get us out - // of exclusive fullscreen. If we don't declare ourselves as the CPU thread, RunAsCPUThread - // calls PauseAndLock, which causes a deadlock if the CPU thread is waiting on us returning. Core::DeclareAsCPUThread(); - } if (called_from_gpu_thread) - { - // We also need to avoid getting a deadlock when the GPU thread is waiting on us returning. - // Declaring ourselves as the GPU thread does not alter the behavior of RunAsCPUThread or - // PauseAndLock, but it does make Host::SetRenderFocus not call RunAsCPUThread. Core::DeclareAsGPUThread(); - } ModalMessageBox message_box(QApplication::activeWindow(), Qt::ApplicationModal); message_box.setWindowTitle(QString::fromUtf8(caption));