MiSTer-devel / TurboGrafx16_MiSTer

TurboGrafx-16 CD / PC Engine CD for MiSTer
94 stars 57 forks source link

After Burner II (Japan) crashes within second of loading #190

Closed sdufus closed 1 year ago

sdufus commented 1 year ago

First reported here: https://misterfpga.org/viewtopic.php?p=70142

After Burner II for PCE crashes within a second of loading. MD5 checksum of AC0CDEE014E725E428FDE5774751FF8D

Tried toggling various options in the core menu but the problem persists.

dshadoff commented 1 year ago

This looks to have been introduced by the "TIMER interrupt acknowledge condition" commit #187 . Since that fix was very straightforward (and almost certainly correct), there is something else which needs to be fixed...

paulb-nl commented 1 year ago

Some observations after debugging and comparing with Mesen2:

This happens in the first timer interrupt routine:

After clearing bit 2 of $1402 it immediately jumps to another timer interrupt routine because the IRQ has not been acknowledged yet and the I flag is clear.

In Mesen this does not happen. It samples the pending interrupt at the end of each instruction but starts it one instruction later if it is still pending. In this case after the write to $1403 so the interrupt is not pending anymore.

So it seems this issue is related to the timing of starting the interrupt routine when an IRQ signal is asserted.

dshadoff commented 1 year ago

Good finding ! This seems to bring back a memory from long ago from a different emulator (either MagicEngine or Mednafen). Are you planning to make the fix, or still collecting information for now ?

paulb-nl commented 1 year ago

I am not planning make a fix for this, just collecting information so someone else can do it.

Looking at Nesdev Detailed interrupt behavior, the 6502 checks the IRQ inputs during the second half of every cycle. If the IRQ input is active then an internal signal goes high at the first half of the next cycle. Figure 6-3 In the WC65C02S datasheet shows the IRQ inputs are read during the falling edge of PHI2 which is just the end of every cycle.

For NMI this internal signal stays high until the NMI has been handled. For IRQs this internal signal goes low again if the IRQ input is not active during the second half of the cycle.

This internal signal is used to start the interrupt at the end of every instruction.

The above information may or may not apply to the HuC6280 but at least it seems starting the interrupt routine one instruction later is needed for this issue.

I think we need to latch the IRQ lines on every CE here: https://github.com/MiSTer-devel/TurboGrafx16_MiSTer/blob/41340793070891c0a1f12c841a179462eefc6795/rtl/HUC6280/HUC6280_CPU.vhd#L483-L485

Then use those latched signals instead of the IRQ*_N inputs in the if LAST_CYCLE = '1' block here: https://github.com/MiSTer-devel/TurboGrafx16_MiSTer/blob/41340793070891c0a1f12c841a179462eefc6795/rtl/HUC6280/HUC6280_CPU.vhd#L488 https://github.com/MiSTer-devel/TurboGrafx16_MiSTer/blob/41340793070891c0a1f12c841a179462eefc6795/rtl/HUC6280/HUC6280_CPU.vhd#L498-L500

dshadoff commented 1 year ago

Thanks for this detailed information ! It allowed me to see the actual situation more clearly.

My fork of Mednafen actually had the same issue.

In this particular case, I am thinking that the IRQ line doesn't need to be delayed, but the value of the IRQ mask should be sampled prior to its commit. I could be wrong in my assumption, but it would be a less-invasive change in terms of impacts to games.

I will look into applying this code change.

dshadoff commented 1 year ago

PR submitted. I'm pretty sure that this is localized enough that no other games should be affected.

dshadoff commented 1 year ago

With today's release, this issue has been tested and can be closed.