snes9xgit / snes9x

Snes9x - Portable Super Nintendo Entertainment System (TM) emulator
http://www.snes9x.com
Other
2.63k stars 453 forks source link

On a 4MB SuperFX ROM with a custom XML, the SCMR Register RAN bit behaves as RON bit, causing a lock up and crash. #913

Closed TrashRaccoonSnuffy closed 5 months ago

TrashRaccoonSnuffy commented 5 months ago

image The issue is, for some reason when SCMR is set to #$0D to hand over control of the RAM to the SuperFX while the program execution is happening in banks +$80, the ROM will lock up and crash snes9x. This does not happen on bsnes. (tested on bsnes plus v05)

The offending line of code:

LDY #$0D STY SFX_SCMR

when this is removed, the program executes just fine. It might be possible that this is also an issue with the XML parser not mapping banks $80+ properly, since the issue does not happen if the code is executed on bank 0 instead.

Test ROM: test.zip

Expected result: image

Error (the emulator crashes shortly after): image

bearoso commented 5 months ago

It might be possible that this is also an issue with the XML parser not mapping banks $80+ properly, since the issue does not happen if the code is executed on bank 0 instead.

I'd say that's very likely, since we don't have an XML parser for memory maps, only the heuristics.

TrashRaccoonSnuffy commented 5 months ago

Yeah banks $80+ appear to be mapped incorrectly for some reason when the ROM is 4MB (probably being mapped as the upper 2MB of the ROM), the SCMR issue appears to be a separate one.

TrashRaccoonSnuffy commented 5 months ago

The SCMR issue appears to be here

https://github.com/snes9xgit/snes9x/blob/master/fxemu.cpp#L145

where it appears to be explicitely checking if (SCMR & 0x18) == 0x18 when it should be != 0, the other problem appears to be from how the ROM is mapped which can be worked around, this change should probably be tested to make sure no SFX games break with it.

bearoso commented 5 months ago

Committed that in af4ec50. If it breaks anything that's a problem to fix down the line because it's obviously correct.

bearoso commented 5 months ago

Looking further in, the check is actually redundant, so it shouldn't break anything.

lordpeluca commented 5 months ago

The update "SuperFX: Run when any bit of SCMR is set." breaks the game "starfox 2".

With the emulator set to "SuperFx speed: 80%" the whole intro of the game looks buggy and freezes in this part:

1 SFX speed 80

It keeps playing the game music, but it doesn't go from there, even if I press start. And I try to skip the intro by pressing start, it just freezes.

With the emulator set to "SuperFx speed: 100% (as default) the intro of the game still looks buggy but I can get to the start screen, but if I press start it just freezes and it also looks buggy like alternating errors, as well as these pictures:

1 SFX speed 100 2 SFX speed 100

Before the update "SuperFX: Run when any bit of SCMR is set." and with the SuperFx speed 80% setting the game loaded normally (no bugs) and was playable:

f1 f2

PS: Game set to 80% because of a bug in Super Mario World 2: Yoshi's Island, in world 1-4 if yoshi is crushed by the first wall it will fail, it makes like a flicker when it is at 100%.

bearoso commented 5 months ago

Yeah, this could make the SuperFX run more often, which means that the speed that was estimated before is useless. On the other hand, we can probably tune it to be more accurate across different games than it was before.

lordpeluca commented 5 months ago

Well, I think for now I'll stick with the latest update of snes9x in which starfox 2 (Qt: Implement Swap Controller 1 & 2) worked fine.

Anyway, thanks for keeping this great emulator up to date. It is the best in quality and performance. Especially to those who have modest pc that we use exclusively for emulation connected to a crt tv.

Regards

TrashRaccoonSnuffy commented 5 months ago

Maybe there's another underlying SCMR issue with the ROM/RAM reads? I haven't checked but this looks more like an issue with the RAM being unable to be written to by the SuperFX for whatever reason (or probably ROM reads failing). The behaviour should be correct, though. SCMR just controls what memory the SuperFX can access and restricts access to it from the SNES. Maybe it's restricting it for both? I have no idea what SF2 stores to SCMR though.

The other thing I can imagine is that the Go flag is being set too early and SCMR is written to after, which would definitely cause issues with the execution.

SF2 seems to alternate between SCMR = 0x21 (no ROM/RAM), 0x29 (RAM only) and 0x39 (ROM/RAM) respectively. I imagine it's the 0x29 mode that's somehow causing issues (since this change makes the SuperFX run during that, no idea if it's running some sort of routine in RAM or attempting to run something in ROM, which would obviously break, but even then it works fine on BSNES)

bearoso commented 5 months ago

The permissions are checked in the fx_checkStartAddress function, so those should be correct. Before, it only executed if both flags were set, so it wouldn't have triggered with RAM only before.

The real question is why Snes9x had that in the first place, and if it was working around something or just a mistake.

TrashRaccoonSnuffy commented 5 months ago

It probably is working around something, I am unable to test this right now but it should be checked if removing the read/write restriction has any effect on the bug in the case it is related to the SCMR restrictions.

bearoso commented 5 months ago

I changed the code in fx_checkStartAddress to match the banks bsnes checks. This seems to fix Star Fox 2.

bearoso commented 5 months ago

I made an additional commit to allow execution for banks 0x80 and above. This still doesn't fix your test ROM, though. It's not mapping those banks and jumping to garbage and never returning. I also made it stop emulation when S9xMainLoop doesn't trigger an hblank after a million cycles. This should fix some of the soft locks we get when running into errors.

TrashRaccoonSnuffy commented 5 months ago

Yeah the test ROM is a complicated topic because I am not sure how to handle it and changing mapper code on the emulator could break things. I could suggest just having a check for the rom size from the header and checking if it's 0x0C (4MB) and applying the custom mapper, but at the same time..

The SuperFX mapper is a rather weird one and similar to HiROM and I assume it's mapping $80-$BF to the higher 2MB of the ROM, which I can work around but at the same time there's no official specification that exists for this and all SuperFX games are 1-2MB. If the mapping is corrected so that $C0-$FF are a linear version of the ROM and $80-$BF are the first 2MB, the test ROM should hopefully run without error. It would still just be a massive assumption, and there's no info anywhere (or an agreed standard) about this. Yoshi's Island hacks make use of the expanded area, for an example.

The code for the superfx mapping is here: https://github.com/snes9xgit/snes9x/blob/master/memmap.cpp#L2892 , I did notice that the GSU1 mode seems to do the mapping I described but I don't know how Snes9x handles it. It could be ram after all.

With said 4MB mapper though, it would preserve compatibility when expanding 2MB SuperFX roms (fastrom or not) as well to 4MB since they assume $00-$3F, $40-$5F and $80-$BF, $C0-$DF are mapped to the first 2MB and the new space would be at $E0-$FF (which was previously mapped to RAM for some unknown reason, atleast on BSNES, unsure about snes9x). XML's on bsnes for the mapper work around this issue but I imagine it's not possible on snes9x due to the internal handling of the mapper.

Thanks for the fix on the SCMR issue though!

qwertymodo commented 5 months ago

The GSU chip doesn't physically support more than 16Mbit of ROM. In fact, the GSU-1 revisions only support 8Mbit, and it is only the later GSU-2 revisions that add another address pin. The remaining address space mapped to ROM can only contain mirrored data on real hardware.

TrashRaccoonSnuffy commented 5 months ago

The GSU chip doesn't physically support more than 16Mbit of ROM. In fact, the GSU-1 revisions only support 8Mbit, and it is only the later GSU-2 revisions that add another address pin. The remaining address space mapped to ROM can only contain mirrored data on real hardware.

Yeah I knew about the GSU limitation and that is a understandable one on the real hardware which is taken into account for programming, but I'm unsure about a limitation on the SNES side of the mapping though.

TrashRaccoonSnuffy commented 5 months ago

Changing the memory map indeed makes the test ROM boot and passes the test succesfully. Test commit: https://github.com/snes9xgit/snes9x/commit/60365ff6bec2b086e5305cf501d4b7188c3dc7de

The change is probably too nosy and with the limitations mentioned it's probably not worth having it on the main repository, so i'll just keep it on a personal fork now. image

lordpeluca commented 5 months ago

The game works perfectly with the new updates.

Thank you bearoso for fixing it. And thanks to all the guys who participated in the thread for all the information written.

Regards

bearoso commented 5 months ago

I went ahead and added your commit. As long as it doesn't break real games, it's fine. We've added support for unrealistic memory maps before.

TrashRaccoonSnuffy commented 5 months ago

It's supposed to keep compatibility with the old memory mapping mode for expanded 4MB roms, so it should be fine. It's essentially just LoROM with some HiROM changes. It doesn't activate unless the ROM is > 2MB. So unless there was some SuperFX hack out there that somehow used 4MB and depended on snes9x's old mapping (YI hacks and SuperFX homebrew that expand to 4MB use the mapping in the commit and a custom snes9x fork to work), it shouldn't cause any issues.

I tested all of the SuperFX games and none of them showed issues.