libretro: don't rebuild the video driver for an unchanged mode - #160
Merged
Merged
Conversation
libretro#159 reports God of War crashing shortly after boot on Android. The reporter ran the triage that would have implicated the recompilers - MTVU off, VU1 softfloat on, EE interpreter - and it still crashed, which is what pointed away from CPU emulation entirely. Their log ends immediately after this: SET_SYSTEM_AV_INFO: 640x448, Aspect: 1.3333, FPS: 59.9401, Sample rate: 48000 ... video and audio drivers reinitialised ... SET_SYSTEM_AV_INFO: 640x448, Aspect: 1.3333, FPS: 59.9401, Sample rate: 48000 ... video and audio drivers reinitialised again ... Two announcements carrying identical values, each making the frontend tear down and rebuild its whole video driver, which on the HW-render path means context_destroy and a fresh negotiation. update_av_info() sent the announcement unconditionally whenever pending_update_av_info was set, without comparing against what had last gone out, and without regard for what the GS thread was doing - so the rebuild could land on top of a thread still submitting to the shared Vulkan queue. Both guards are the ones pcee2 already carries for the same defect (adf456e28d): - Announce only on a real timing change. The fps compare needs a tolerance, since NTSC reports 59.94005994 Hz against a 59.94 default and that 0.00006 Hz difference must not rebuild anything. Geometry needs no announcement on the HW-render path: SET_GEOMETRY carries it without a reinit, which is already how the widescreen hint does it, so it is forwarded that way instead. - Drain the GS thread before an announcement that does go out. The CPU thread is already parked at this point - retro_run has not resumed it. Software rendering keeps the old behaviour for a geometry change, matching pcee2 rather than widening the change. This is a hypothesis the log supports rather than a confirmed fix: nothing in that log says what the crash was, because the core has no crash reporter yet (libretro#158), and the reporter cannot get logcat without root. What can be said is that the frontend was rebuilding its video driver for no reason at the moment everything stopped, and that this is a defect either way.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the most likely cause of #159.
The reporter there ran the triage that would have implicated the recompilers — MTVU off, VU1 softfloat on, EE interpreter — and it still crashed, which pointed away from CPU emulation. Their log ends immediately after this:
Two announcements carrying identical values, each making the frontend tear down and rebuild its entire video driver — on the HW-render path,
context_destroyand a fresh negotiation.update_av_info()sent the announcement unconditionally wheneverpending_update_av_infowas set: no comparison against what had last gone out, and no regard for what the GS thread was doing, so the rebuild could land on a thread still submitting to the shared Vulkan queue.Both guards are the ones PCEE2 already carries for the same defect (
adf456e28d):SET_GEOMETRYcarries it without a reinit, which is already how the widescreen hint does it, so it is forwarded that way instead.retro_runhas not resumed it yet.Software rendering keeps the old behaviour for a geometry change, matching PCEE2 rather than widening the change.
What this is not
A confirmed fix. Nothing in that log says what the crash was: the core has no crash reporter yet (#158), and the reporter cannot get
logcatwithout root. What the log does show is the frontend rebuilding its video driver for no reason at the moment everything stopped, and that is a defect whether or not it is the one.Compiles clean; I have no Android device to run it on.