PCSX2 / pcsx2

PCSX2 - The Playstation 2 Emulator
https://pcsx2.net
GNU General Public License v3.0
11.91k stars 1.63k forks source link

Boot (Full) fails if rebooting with a game already running #851

Closed Masamune3210 closed 8 years ago

Masamune3210 commented 9 years ago

After a reboot, the emulator log will throw a random Syscall Undefined and the virtual PS2 will fail to reboot until reboot is attempted again on which the emulator will either crash or successfully reboot.

Version: PCSX2 1.3.1-20150925184220 CPU: Intel i5-2500k @ 4.0 GHz GPU: nVidia GeForce GTX 660 Superclocked 3GB RAM: 11 GB DDR3 Log: https://gist.github.com/deadair3210/a7b941dacc03badcd9e9

jcdenton2k commented 8 years ago

I opened up the file and here were the active codes present in my version:

// new 16:9 hack by - ElHecht patch=1,EE,00106e10,word,3c013f4c // c480004c patch=1,EE,00106e28,word,3421cccc // 4600a7c6 patch=1,EE,00106e2c,word,4481f800 // 00000000 patch=1,EE,00106e30,word,461fa503 // 4600a503 patch=1,EE,00106e54,word,3c1b3f40 // 00000000 hor fov patch=1,EE,00106e58,word,449bf000 // 00000000 patch=1,EE,00106e70,word,461effc2 // 00000000 patch=1,EE,00106e74,word,e61f004c // 00000000

// optional zoom for cutscenes patch=1,EE,2036a0bc,word,43f90000 // 43d00000

//font fix patch=1,EE,2036CE94,word,3F400000 // 3F800000 patch=1,EE,2036CE98,word,3F400000 // 3F800000 patch=1,EE,2036CE9C,word,3F400000 // 3F800000

//black border fix patch=1,EE,0014AD80,word,24050000 patch=1,EE,0014ADA8,word,24050000 patch=1,EE,0014ADD0,word,24050000 patch=1,EE,0014AE00,word,24050000

//lower subtitles patch=1,EE,001ac8d8,word,240a018a

So I'm not sure which of those is the cause of this issue. Starting PCSX2 fresh (or just Fast Boot) is the easiest solution/workaround.

There are a few commented-out codes but obviously if they're not being loaded then they aren't the cause of the issue.

Masamune3210 commented 8 years ago

I don't have access to my computer currently, or I would test this and attempt to help diagnose more.

jcdenton2k commented 8 years ago

Since this issue is a widescreen patch issue, your best bet is to try poking the person responsible for those and passing along a link to this thread. Let them handle it while you work with them to narrow down which code(s) are doing the borking.

As I've said earlier, it may just be easier to stick a comment in the patch noting this issue and let that be that.

Masamune3210 commented 8 years ago

Unfortunatly, I am unaware of who is in charge of the widescreen patches, other than it at one point was handled by the forums and then updated at certain points. I swear at some point I tried it without the patches enabled, and still had the issues. The issue originally arised while testing and running an English translation of Kingdom Hearts Final Mix, which makes me wonder if its not a bad interaction with the patches, and the NTSC-J video mode that Final Mix uses

jcdenton2k commented 8 years ago

NTSC = 60 Hz = same video mode as other NTSC games. NTSC is the standard for most games. Doesn't matter if it says NTSC-USA or NTSC-Japan.

PAL = 50 Hz = the competing standard primarily in European markets. I feel bad for them being stuck at 50 FPS instead of 60.

Masamune3210 commented 8 years ago

I was unaware if there was a difference between J and U/C. Just shooting theories as I think about them. What exactly is a TLB Miss, knowing that might help

-----Original Message----- From: "jcdenton2k" notifications@github.com Sent: ‎7/‎23/‎2016 1:40 PM To: "PCSX2/pcsx2" pcsx2@noreply.github.com Cc: "deadair3210" kill8r@gmail.com; "Mention" mention@noreply.github.com Subject: Re: [PCSX2/pcsx2] Boot (Full) fails if rebooting with a game alreadyrunning (#851)

NTSC = 60 Hz = same video mode as other NTSC games. NTSC is the standard for most games. Doesn't matter if it says NTSC-USA or NTSC-Japan. PAL = 50 Hz = the competing standard primarily in European markets. I feel bad for them being stuck at 50 FPS instead of 60. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

jcdenton2k commented 8 years ago

Maybe one of the developers can explain 'TLB Miss' better than I can as I'm not a coder. It typically happens with a bad ISO rip and it is suggested to use ImgBurn when ripping ISOs to make sure they work properly.

In the case of this issue, the TLB Miss happens because of some conflict between a restart of the emulator and one or more of the widescreen patches and/or cheats for specific games.

Sarania commented 8 years ago

There is a difference between TLB miss [load] and TLB miss [store]. [load] is generally caused by bad ISO whereas [store] is often caused by bad codes/patches. Basically TLB is how the PS2(and other machines) deal with virtual memory. When the game asks for data at a location in virtual memory space, the TLB translates that back to physical address space. However, if the address is invalid it can "miss" i.e. point to an invalid space. [load] and [store] just signify whether it was a read or a write.

https://en.wikipedia.org/wiki/Translation_lookaside_buffer

ramapcsx2 commented 8 years ago

Even if it's caused by patches, this is still a valid problem. "Rebooting" in PCSX2 is supposed to clean the slate and forget everything about the emulation state. Maybe a bug in the patching code deinit?

Masamune3210 commented 8 years ago

From what I remember, and what I have gathered, the issue happens after rebooting from any game that has patches. That would explain why the issue was different depending on wether I was running the English translation of Final Mix, as the CRC changed so the patches no longer applied

avih commented 8 years ago

I've found a solution to this issue so that it can be closed.

No, it can't be closed.

The fact that you found a "solution" means there's a problem, hence the bug should be kept open either until the problem is fixed or until the PCSX2 team decided not to fix it. Thus far none of those has happened.

avih commented 8 years ago

Also, I think I can confirm that at least with Ico, the issue is indeed the Widescreen patches enabled (Ico does have widescreen patches). I.e. If I have Ico (PAL) running and the ws patches are enabled (regardless if the automatic game fixes are enabled or not - and there are 2 automatic configuration fixes for Ico), and then reboot-full then it fails.

But If Ico is running and I don't have the widescreen patches enabled (again, regardless if the automatic fixes are enabled or not), then I can reboot-full and it works.

So I tried to disable the Widescreen patches from the menu, and instead put the widescreen patch (5C991F4E.pnach from cheats_ws.zip) at the cheats folder - and had the same issue egain - if cheats are enabled and the patch is loaded, then boot-full fails.

So it's not related specifically to the widescreen patches "system" in pcsx2, but rather to whether or not (some kinds of?) patches are loaded - if they're loaded, e.g. when Enable Cheats is checked at the menu and the patch exists at the cheats folder - then reboot-full fails.

avih commented 8 years ago

We can probably disable all patches when the user requests a full boot and restore them after the boot starts, but that would be a workaround.

I'm guessing the real issue is incorrect or insufficient shutdown of the VM when patches are loaded, and the real fix would therefore probably be at the shutdown procedure.

Aced14 commented 8 years ago

This issue and #1310 are likely caused by the same bug. Ironically, it benefits #627 when repeatedly fast-booting.

This isn't a proper solution, but pnaches can be edited to work around the bug. If an E-type (multi-line skip) code is used, hacks can be activated only when they're supposed to. For example, an E-type code can be used to detect whether a specific memory address contains an expected value to conditionally enable all of a pnach's hacks. That way, if a user attempts to full boot while a game is already running, the E-type code will prevent the previously-running game's pnach hacks from "polluting" BIOS memory.

Many of the pnaches I've contributed to the widescreen archive are already using E-type codes to work around this bug (and another unrelated full boot pnach bug).

avih commented 8 years ago

This isn't a proper solution, but pnaches can be edited to work around the bug ...

Right, it's not a good solution.

I tracked this a bit, and it seems the issue is that we apply the patches/cheats for the previous CRC when rebooting - to the bios on full boot, and not sure whether or not to the new elf on fast boot. And this can break the bios.

This suggests that either we don't reset the CRC on [re]boot soon enough, or we do but in the grand scheme of things it doesn't propagate fast enough to where the patches are applied (seemingly at-least/also at SysCoreThread::VsyncInThread()).

But the CRC (elfCRC) does reset on time correctly, and SysCoreThread::VsyncInThread() doesn't care at all about the current CRC - it just applies whatever patches we already have initialized/loaded.

So now the issue seems that we don't unload/invalidate the current patches when rebooting.

I experimented with two approaches, and both worked for the specific issue of full boot:

  1. Either: at SysCoreThread::VsyncInThread() add if (!ElfCRC) return; - so this just doesn't apply the patches while we're at the bios or before the crc is known. It works, but IMO not great.
  2. Or: at SysCoreThread::_reset_stuff_as_needed() - if m_resetVirtualMachine then also ResetCheatsCount();.

2 seems better and does fix the issue, but actually it's only partial, since it resets the cheats (and widescreen hacks which internally are cheats too), but doesn't reset the patches count (the ones applied from the games DB). So we should probably reset the patches too.

While at it, I noticed that some places do:

if (EmuConfig.EnablePatches) ApplyPatch();
if (EmuConfig.EnableCheats)  ApplyCheat();

But ignore EmuConfig.EnableWideScreenPatches.

This happens at SysCoreThread::GameStartingInThread() and at iR5900-32.cpp::recRecompile() (but at SysCoreThread::VsyncInThread() it also checks EmuConfig.EnableWideScreenPatches).

The tests are actually almost* moot, since on every config change (or boot) the patches and cheats are reset and loaded from scratch, so there's no need to check whether the config enables them or not, as if it doesn't then they won't be loaded to begin with (* almost - since it incorrectly prevents applying the widescreen hacks unless cheats are enabled).

Overal, IMO we should do the following:

  1. Most importantly - invalidate the currently loaded patches and cheats when rebooting. This should fix this bug, and should be relatively easy.
  2. Unify patches/[widescreen]cheats handling, where the only difference would be whether or not we load them - according to the config (game db patches, "cheats" AKA user patches, and widescreen hacks patches) - the load part already works at the current code. Other than loading them on boot/config-changes, the rest of pcsx2 will have only two visible functions which handles all of them: invalidate (used for reboot and config changes before [re]-loading them) and apply (on vsync, recompile, etc, whenever they need to be applied).
avih commented 8 years ago

@refractionpcsx2 @ramapcsx2 @Sarania @prafullpcsx2 @Aced14 (or anyone else who has a good answer)

We have patch lines which start with 0,... e.g.: patch=0,EE,0010BC88,word,48468800 and also lines which start with 1,... e.g.: patch=1,EE,001110e0,word,00000000

E.g. at GameIndex.dbf we have 240 patch lines which start with 0, and 45 which start with 1 (didn't count how many of them are commented out, but we do have both 0 and 1 uncommented). At cheats_ws.zip I checked few random files, and all of them only had lines starting with 1.

I can see what difference it makes inside PCSX2, but I'm not seeing anything which documents how it should work for 0 and 1.

So my questions to you are:

Thanks.

ramapcsx2 commented 8 years ago

Oh, this is really old stuff. If I remember correctly, one was for "overwrite once", the other for "overwrite always". I could be wrong and I can't even tell which was which :p

avih commented 8 years ago

@deadair3210 @Aced14 (or others), could you please confirm (or deny) that this is now fixed with the recent commits?

Masamune3210 commented 8 years ago

Unfortunatly, I am unable to test this at any close time as my computer is not where I am

Sarania commented 8 years ago

Just FYI (since you did ping me :p ) I will confirm that what @ramapcsx2 said is also the explanation I've heard between patch=0 and patch=1. I think it's almost become forgotten knowledge nowadays as I haven't seen it brought up in a while! I think it was 0 was overwrite once and 1 was overwrite always. I could be wrong though. I think nowadays people are just taught to use =1 since in all but a few specific cases it should be the "safe" way(if I'm right about it being "overwrite always") that basically always works.

avih commented 8 years ago

Right, that's what the code does - applies patches with "0" once on startup, and patches with "1" on every vsync. I already added docs for this at Patch.h and elsewhere, and also gave this type a name patch_place_type with two values PPT_ONCE_ON_LOAD and PPT_CONTINUOUSLY.

Just for the record, @gigaherz and @ramapcsx2 agreed that the pnach format was invented by nachbrenner (a past PCSX2 developer), and hence the format name too ;)

Apparently it was common knowledge back then what do the 0/1 do, however we couldn't find even a single comment at the code or elsewhere which describes what it should do, which leaves us with a definition which follows the implementation.

As mentioned earlier, ~90% of the patches at GameIndex.dbf use 0 - to apply the patch once on startup. Apparently typically that's enough.

Off topic, well.. not really.. if there are still issues with fast or full reboot where patches are applied correctly or incorrectly, let me know. I played with it quite a bit with many reboots, enabling/disabling various types of patches/cheats/etc, changing games, rebooting full or fast, and as far as I can tell it now always works correctly.

Aced14 commented 8 years ago

@avih Thanks :)!

I came up with some fast boot to full boot tests using widescreen patches that caused BIOS crashes/log errors in an older recent build (093704f0738a42425dc9e5592195966671350662). I then built 56adb85a87c763f078b57f395cdf8fc60917ca32 in VS 2013 and retried the same tests... worked great :). 67aee8a19cf9e98bc3fe87a0f2a28eccf40e9329 and cff8cb137cc3f1f73096d54cbecc6d79cd0781ff didn't have any problems either.

My tests did crash at a later point in time, but it's unrelated to this issue. I'll post a separate issue about it later today or tomorrow.

avih commented 8 years ago

All right. Thanks :)