DolphinQt: Fix the panic alert deadlock, dual core edition #8763
Conversation
JosJuice
changed the title
JosJuice
force-pushed
the
panic-alert-deadlock-gpu
branch
2 times, most recently
from
aea039a
to
ce5e5be
Compare
|
This fix doesn't seem to be sufficient; I still get deadlocks with the dual-core FIFO player when stopping playback (though I don't have a way of consistently reproducing it; it just happens occasionally). Note that this is with the changes from this PR cherry-picked onto a bunch of other FIFO-related changes made by me (mostly those from #9540, but there are a few others). I don't think those would reintroduce this deadlock, though (especially since I cherry-picked these commits atop my original changes without any merge conflicts). StacktraceMain thread
Video Thread
FIFO player thread
I don't actually see any window labeled "Question" at all; it only shows up in the log. Also, the panic alert is actually on the CPU thread, so maybe this is a different issue? |
|
This specific PR is only trying to solve issues when the panic alert is from the GPU thread. I had hoped that I had cleared out all the issues with panic alerts from the CPU thread in previous PRs, though... |
|
I suppose the problem you're having is that the main thread is blocking the Qt event loop (or something like it?) while trying to stop emulation, and so when the panic alert code comes and tries to synchronize with the main thread, it deadlocks. This is a bit different from the issues I've tried to solve in past PRs and this PR – I've been dealing with deadlocks when trying to synchronize with the CPU thread or GPU thread, but you're dealing with a deadlock when synchronizing with the main thread. |
JosJuice
force-pushed
the
panic-alert-deadlock-gpu
branch
from
ce5e5be
to
8f13bcf
Compare
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.
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.
JosJuice
force-pushed
the
panic-alert-deadlock-gpu
branch
from
8f13bcf
to
d445d2a
Compare
|
FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:
automated-fifoci-reporter |
|
I tested this and it resolves the issue when using dual core and fullscreen and a panic alert appears. It worked already on OpenGL for me but this PR also makes it work on the other hardware backends too. |
|
Bumping this, also confirming solves some of my issues. Would be good to get this in asap. |
| @@ -85,6 +88,44 @@ void Host::SetMainWindowHandle(void* handle) | |||
| m_main_window_handle = handle; | |||
| } | |||
|
|
|||
| static void RunWithGPUThreadInactive(std::function<void()> f) | |||
There was a problem hiding this comment.
I believe this could be a template to avoid the overhead of the std function:
template <typename Func>
static void RunWithGPUThreadInactive(Func&& f)
{
if (Core::IsGPUThread())
{
f();
}
else if (Core::IsCPUThread())
{
const bool was_running = Core::GetState() == Core::State::Running;
Fifo::PauseAndLock(true, was_running);
f();
Fifo::PauseAndLock(false, was_running);
}
else
{
Core::RunAsCPUThread(std::forward(f));
}
}
There was a problem hiding this comment.
The most common case is the RunAsCPUThread case, in which f has to be turned into an std::function anyway. Would this be beneficial despite that?
There was a problem hiding this comment.
Ah I missed that. Probably not then. We may want to consider updating RunAsCPUThread in the future (if it's even possible), as there is some overhead for std::function
|
This PR hurts because all it really tells me is that our threading concept is poorly designed. =/ Code looks sensible enough but I can't even pretend to understand all possible deadlock causes here. A few people claim this fixes their issues, so maybe we should just merge and hope for the best? |
Yeah, the whole concept of having the core threads wait for the user to click OK on a panic alert is something that inherently carries a risk of deadlocks, unfortunately. I can't think of any way to completely avoid that without making it so that execution of the core threads at least sometimes continues without the user acknowledging the panic alert. |
|
It's been a month since the last comment and I don't think anything changed, I'm just gonna merge this... |
The fix in PR #8715 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.
I have assumed that the reason we use
RunAsCPUThreadwhen callingSetFullscreenis because we want to wait for the GPU thread to be inactive. (Because it can't be the CPU thread itself that we're waiting for, right? The CPU thread shouldn't directly interact withg_renderer.) However, I'm not entirely sure about this. If my assumption was wrong, then this PR is probably not the best solution.