-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Savestates #10478
Savestates #10478
Conversation
Please test extensively this and report issues with logs. Also if you can get callstack for crashes I'll be glad. Issues remain unreported may persist after the pull requerst has been merged so please report them! |
Also feedback about reqeusted features is more than welcome! |
Also: before you are saving state, pause the emulation, go to kernel explorer and capture its contents. This really helps with debugging! |
791859a
to
321e4a8
Compare
I just made kernel explorer always expand its entries and made it larger for ease of capture. (shared memory and lwmutex you can leave uncaptured if they are too large) |
I recommend capturing kernel explorer in video at 1fps so its contents can be easily read. |
GT5 - Saved state, game continued playing fine. Finished the race I was currently on and moved on to the next in a different track, where it desynced. Loaded the savestate I had done in the previous race and the emulator crashed. Motorstorm - After saving state, the game crashed when reloading. Motorstorm Apocalypse - Saved state, when the game finished reloading the entire emulator closed. Ridge Racer 7 - Saved state, when the game finished reloading it threw a fatal error. Gran Turismo HD Concept - This game never worked in any savestate version, still a white screen when when reloading the game after saving state. Resuming the game after pausing freezes it, so the kernel explorer video was captured with the game running. 2021-06-20.10-28-55.mp4Skate 3 - Saved state, when the game finished reloading it threw a fatal error. 2021-06-20.10-47-03.mp4God of War 3 - Everything seems to work fine, except performance took a hit after saving state, lost about 5 FPS. 2021-06-20.11-14-02.mp4The Last Of Us - Saved state, when the game finished reloading it threw a fatal error. 2021-06-20.11-33-27.mp4I could get kernel explorer clips for the first few later if needed, they were tested before your latest commits |
Grand Theft Auto IV (NPUB30702) crashes rpcs3 when loading states. Fallout New Vegas (BLUS30500) freezes when loading states. |
Half Life 2 The Orange Box (BLUS30055) crashes rpcs3 when saving states, before the file is even made. |
Sonic Unleashed - Doesn't work with the save state and I got an error |
FFX Remaster - creating or loading a savestate causes the ground to render improperly. It appears black except for where characters are standing. No other observed issues. Rainbow 6 Vegas 2 - No observed issues. Tales of Xillia 2 - No observed issues. SSX - Creating or loading a savestate causes a graphical issue with the snow around your character. This issue does go away after a short amount of time. Attempting to create a savestate after restarting a drop causes the emulator to crash. It fails to create a savestate in this situation. Haze - No observed issues. Skate 2 - Creating a savestate crashes the emulator while the game reloads. Attempting to load this savestate also causes the emulator to crash while the game boots. |
DJ Hero 2 Demo - Creating a savestate crashes the emulator while the game reloads. 2021-06-20.20-51-45-1.mp4 |
LittleBigPlanet™ [BCES00611]-Savestate doesn't load |
Anyone has visual studio and can get me callstacks for crashes? |
RPCS3 crashes whenever I try to boot a game with VS attached, no idea why. Will have to figure that out first |
You need to disable "break when this exception type is thrown" for RSX access violations to work. |
5cd1a5d
to
c9804f0
Compare
Fixed UNIX builds, added debugging info for crashes. |
95014d6
to
4d6e935
Compare
3cb80e0
to
f1a3716
Compare
82b56b7
to
055820a
Compare
thread_ctrl::wait_on<atomic_wait::op_ne>(g_progr_ptotal, 0); | ||
g_fxo->get<progress_dialog_workaround>().skip_the_progress_dialog = true; | ||
|
||
// Sadly we can't postpone initializing guest time because we need ti run PPU threads |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Sadly we can't postpone initializing guest time because we need ti run PPU threads | |
// Sadly we can't postpone initializing guest time because we need to run PPU threads |
// Save data in forward order | ||
for (u32 i = _max; i; i--) | ||
{ | ||
if (auto save = (*std::prev(m_info, i))->save) save(*std::prev(m_order, i), ar); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (auto save = (*std::prev(m_info, i))->save) save(*std::prev(m_order, i), ar); | |
if (auto save = (*std::prev(m_info, i))->save) | |
{ | |
save(*std::prev(m_order, i), ar); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only got halfway, there are too many instances of the same issues anyway, but the summary is pretty simple - there are way too many changes that are not directly related to savestates included here. These really should have been submitted incrementally. The reason for this is pretty simple. Users bisect on changeset, not commit. With this uber-PR, any mistakes will break the whole thing and cause emergency reverts of large sections to unbreak. It's just too difficult to follow which part potentially affects what, potential performance impact, etc. I have no doubt there will be problems - there always are will large PRs, the only difference is that this time only the original code author will be able to fix them.
@@ -694,6 +701,7 @@ namespace fs | |||
bool trunc(u64 length) override | |||
{ | |||
obj.resize(length); | |||
update_time(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unrelated to savestates. Why have it submitted with such a massive PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is for POSIX allowence of deletion of files with opened file descriptors in savestates. In this case I convert the file descriptor to memory and added missing functionality so it can be used by cellFs.
private: | ||
stat_t m_stat{}; | ||
|
||
void update_time(bool write = false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, this should have been submitted separately, though the same can be said for large parts of this changeset.
@@ -26,11 +26,17 @@ struct loaded_npdrm_keys | |||
} | |||
|
|||
// TODO: Check if correct for ELF files usage | |||
u128 last_key() const | |||
u128 last_key(usz backwards = 0) const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also unrelated/splittable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related.
@@ -615,6 +661,11 @@ void cell_audio_thread::operator()() | |||
|
|||
thread_ctrl::scoped_priority high_prio(+1); | |||
|
|||
while (Emu.IsPaused()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An odd inclusion. Why is this here? Why now?
} | ||
|
||
SAVESTATE_INIT_POS(2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a lot of these sprinkled all over the code, they really should use an enum to ease maintenance.
@@ -1975,7 +2131,17 @@ std::pair<std::shared_ptr<lv2_overlay>, CellError> ppu_load_overlay(const ppu_ex | |||
if (prog.bin.size() > size || prog.bin.size() != prog.p_filesz) | |||
fmt::throw_exception("Invalid binary size (0x%llx, memsz=0x%x)", prog.bin.size(), size); | |||
|
|||
if (!vm::get(vm::any, 0x30000000)->falloc(addr, size)) | |||
const bool already_loaded = ar /*&& !!(_seg.flags & 0x2)*/; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seconded. The flags check is included in other similar checks elsewhere, making this stick out.
@@ -1380,6 +1391,24 @@ void ppu_thread::cpu_task() | |||
asm("DSB ISH"); | |||
#endif | |||
|
|||
// Wait until the progress dialog is closed. | |||
// We don't want to open a cell dialog while a native progress dialog is still open. | |||
thread_ctrl::wait_on<atomic_wait::op_ne>(g_progr_ptotal, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yet another standalone fix
{ | ||
if (g_cfg.core.ppu_decoder != ppu_decoder_type::llvm) | ||
{ | ||
return; | ||
} | ||
|
||
if (auto dis = g_fxo->try_get<disable_precomp_t>(); dis && dis->disable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change related to savestates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
@@ -2560,53 +2803,48 @@ extern void ppu_precompile(std::vector<std::string>& dir_queue, std::vector<lv2_ | |||
|
|||
std::string upper = fmt::to_upper(entry.name); | |||
|
|||
// Check .sprx filename | |||
if (upper.ends_with(".SPRX") && entry.name != "libfs_utility_init.sprx"sv) | |||
// Skip already loaded modules or HLEd ones |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is another improvement unrelated to the saving of states. You should have opened incremental PRs with the supporting fixes for easier risk management.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's related to savestates. It's required to skip precompilation of already loaded modules with the addition of OVL modules checking which comes with savestating MGS4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is standalone though. That's what I mean. I understand that it is needed for savestates to work, but it doesn't contain the core saving of states (serialization/deserialization) and would have been good in a part 1 PR. Again, I don't expect anything to change, but I have to give advice anyway for the sake of future PRs.
@@ -2822,20 +3060,39 @@ extern void ppu_initialize() | |||
compile_main = ppu_initialize(_main, true); | |||
} | |||
|
|||
std::vector<lv2_prx*> prx_list; | |||
std::vector<ppu_module*> module_list; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. I'm not going to check for any more standalone improvements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, related.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Standalone, not unrelated. I'm trying to think of how to merge the whole PR without too much risk.
By standalone I mean it can be submitted separately before the "main" work, e.g in a savestates part 1 PR or something.
Not that you'll do it anyway, but still I feel it would have really helped regtesting.
There are no unrelated changes in this pr. |
rpcs3/Emu/Cell/PPUThread.cpp
Outdated
@@ -2230,7 +2461,7 @@ static bool ppu_store_reservation(ppu_thread& ppu, u32 addr, u64 reg_value) | |||
{ | |||
if (count > 20000 && g_cfg.core.perf_report) [[unlikely]] | |||
{ | |||
perf_log.warning(u8"STCX: took too long: %.3fµs (%u c)", count / (utils::get_tsc_freq() / 1000'000.), count); | |||
perf_log.warning(u8"STCX: took too long: %.3fµs (%u c)", count / (utils::get_tsc_freq() / 1000'000.), count); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another corrupted unicode string.
Is this a bug? In White Knight Chronicles II (Haven't tested any other game yet), doing consecutive save states (3 total, Boot Game > Save > Load Save > Save > Load Save > Save > Load Save > Crash) will cause the savestate in question to be corrupted |
I think it can be merged once minor unicode string corruptions are fixed. |
Sorry for bumping this but how exactly this work? |
Restrictions (most will be lifted as this work progresses):
How to savestate
Ctrl+S savestates, savestate will be bootable from the game context menu later.
Versioning System
Because RPCS3 is one of the most complex emulators ever made (if not the most complex) and it is still in rapid development state, the need to update the emulation every now and then in a way which will affect savestates is inevitable.
So, while the rest of the emulators use a global version for their savestate for all components of the emulation, RPCS3 savestates will work in a smarter way which allows both rapid development of the emulation and high compatibility of old savestates.
The following actions has been taken:
"supported" versions are versions listed which are known to be compatible with current RPCS3 build. Code can support more than one version at a time! how? this is where "current" version kicks in, the "current" version is read from the savestate file itself and by its value the code knows how to use the savestates file.
In case the component is unused, its version won't even be saved to the savestate file!
Savestates modes (config.yml exclusives at the moment)
This setting is currently off by default but let me know if you want it on by default!
Additional TODO list: (besides lifting restrictions)
If you want to simulate multiple slots, you can move and restore the savestate file at that path, or you can load savestate files directly via main RPCS3 context menu.
Fixes #4218