Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Join the emu thread in Core::Stop. Get rid of Core::Shutdown which did that before." #2538

Merged
merged 2 commits into from Jul 25, 2015

Conversation

CrossVR
Copy link
Contributor

@CrossVR CrossVR commented Jun 6, 2015

This reverts commit ba664b3, fixing issue 8518.

This PR fixes a consistent deadlock on the D3D backend when stopping emulation while using exclusive fullscreen while having the confirm dialog disabled.

I can't reproduce the race condition @comex is referring to and I think a consistent deadlock is more important to fix than a few rare race conditions. The race conditions themselves should be fixed instead of introducing blocking calls to prevent them.

Another possibility to fix this issue if we insist on blocking the UI thread is to wait until we've exited fullscreen before we initiate the shutdown. Or we can have the GPU thread handle render window messaging itself.

@comex
Copy link
Contributor

comex commented Jun 6, 2015

The race condition was very easy to reproduce for me. If you want to avoid it, at least fix the IsRunning issue; I hate workarounds that don't fix the real problem, but I suppose reverting a commit that breaks things isn't the end of the world.

Fixing it correctly without blocking is possible but hard. In particular, even if in practice this issue is only symptomatic on shutdown, the documentation you linked clearly states that the message pump should not ever be blocked. However, PauseAndLock blocks waiting for the CPU thread, and there is a small chance the CPU thread is currently sitting in SyncGPU waiting for the GPU thread - so for a correct fix, all usage of PauseAndLock would have to be refactored into being asynchronous. Which is probably a good idea anyway, but there are a lot of locations to deal with.

If we ¯_(ツ)_/¯ and only address the issue for shutdown, the best otherwise-correct fix would probably be preventing a new game from being started (even from NetPlayClient!) until the GPU thread is gone. Otherwise we will just auto-join it when the variable is reassigned then. Alternately, we could detach the thread, but we'd have to ensure it was done with the main fifo loop that can write to globals, write to emulated memory, spawn events, etc. - might require some reordering of shutdown calls.

Mind you, I did not expect very briefly blocking the GUI thread to be an especially bad or improper fix... the delay is imperceptible under normal conditions. The only problem occurs because of this weird Windows behavior. So one may ask if there's a way to simply work around said behavior: can we add code called from both PauseAndLock and shutdown to partially run the message pump somehow...? (I guess this would be pretty hard since you want to avoid calling into wx.) Alternately, since this is only symptomatic in fullscreen mode, in theory there should be no reason for DXGI to be dealing with any windows owned by the GUI thread (it's in its own window), so it seems like something is messed up - but again, a correct fix requires considering render to main.

@Sonicadvance1
Copy link
Contributor

Core::Shutdown needs to be slapped in to the Android file as well if this is to be merged.

@CrossVR
Copy link
Contributor Author

CrossVR commented Jun 8, 2015

That's strange, wasn't it being called before this was reverted?

@Sonicadvance1
Copy link
Contributor

nope

@CrossVR CrossVR added this to the Dolphin Release 5.0 milestone Jun 8, 2015
@CrossVR CrossVR added the WIP / do not merge Work in progress (do not merge) label Jun 9, 2015
@CrossVR CrossVR changed the title Revert "Join the emu thread in Core::Stop. Get rid of Core::Shutdown which did that before." [WIP] Revert "Join the emu thread in Core::Stop. Get rid of Core::Shutdown which did that before." Jun 9, 2015
…which did that before."

This reverts commit ba664b3.

Added documentation to Core::Shutdown() to prevent breaking changes.
@CrossVR CrossVR changed the title [WIP] Revert "Join the emu thread in Core::Stop. Get rid of Core::Shutdown which did that before." Revert "Join the emu thread in Core::Stop. Get rid of Core::Shutdown which did that before." Jul 22, 2015
@CrossVR CrossVR removed the WIP / do not merge Work in progress (do not merge) label Jul 22, 2015
CrossVR added a commit that referenced this pull request Jul 25, 2015
Revert "Join the emu thread in Core::Stop. Get rid of Core::Shutdown which did that before."
@CrossVR CrossVR merged commit fbd5bb8 into dolphin-emu:master Jul 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants