Skip to content
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

EE: Expose advanced option for extra memory #11111

Merged
merged 3 commits into from
May 9, 2024

Conversation

DaZombieKiller
Copy link
Contributor

Contributes to #5803

This PR Exposes a new "Enable Extra Memory" option in the Advanced options menu:
image
The option bumps the amount of memory available to the EE to 128 MB, allowing homebrew (and a limited selection of other titles) to take advantage of expanded RAM limits. I considered exposing the option in the Emulation menu, but the feature does not seem like one that should be exposed to the average user so directly.

Once the VM has been started, any changes to the extra memory option are ignored. The option's state is cached at memory initialization time and is remembered in save states (necessitating a version bump).

When the option is enabled, the RFU060 (SetupThread) and GetMemorySize syscalls are overridden to behave as if the BIOS supported expanded RAM. Due to the fact that the size of the EE's memory can now change during the PCSX2 process lifetime, I also had to introduce new Ps2MemSize::GetExposedRam and Ps2MemSize::TotalRam members. I welcome any suggestions for a better approach here.

I have tried my best to follow existing style and convention, but as this is my first real contribution to PCSX2 I may have made some mistakes here and there. Please let me know if anything sticks out.

@MrCK1
Copy link
Member

MrCK1 commented Apr 20, 2024

I think adding "Debug" or "Dev Console" (like Duckstation) in parentheses would be better (or move to another section entirely). My initial thought is that people are going to be inclined to check it off because it sounds faster (and possibly crash their games)

@F0bes
Copy link
Member

F0bes commented Apr 20, 2024

I know @stenzek raised a concern the last time this was proposed here.

It seems like this will create another page of memory after all of the initial allocations are completed? Ensuring that from the host perspective IOP memory is still directly after the 'main' 32mb EE memory mapping?
Is this correct?

@seta-san
Copy link
Contributor

This seems a little bit less like emulation and more the beginning of creating a vm for a machine that never existed. It’s expanding the scope of the pro just for no real good reason. If you want to home brew for a more powerful machine try anything newer than a ps2.

@DaZombieKiller
Copy link
Contributor Author

I know stenzek raised a concern the last time this was proposed

My understanding is that #10082 eliminated this concern because the memory map now already reserves space for 128 MB of EERAM. That said, I am by no means an expert here.

@weirdbeardgame
Copy link
Contributor

This seems a little bit less like emulation and more the beginning of creating a vm for a machine that never existed. It’s expanding the scope of the pro just for no real good reason. If you want to home brew for a more powerful machine try anything newer than a ps2.

TOOL consoles had 128mb

@F0bes
Copy link
Member

F0bes commented Apr 20, 2024

This seems a little bit less like emulation and more the beginning of creating a vm for a machine that never existed. It’s expanding the scope of the pro just for no real good reason. If you want to home brew for a more powerful machine try anything newer than a ps2.

This has a purposeful use for modding games. You have an enormous amount of space to put extra logic / assets. Do you have a good reason why this shouldn't be added to the emulator?

I know stenzek raised a concern the last time this was proposed

My understanding is that #10082 eliminated this concern because the memory map now already reserves space for 128 MB of EERAM. That said, I am by no means an expert here.

I'll have to see what Stenzek says, I'm not too familiar with how our vtlb works.

common/Pcsx2Defs.h Outdated Show resolved Hide resolved
pcsx2/SaveState.cpp Outdated Show resolved Hide resolved
pcsx2/x86/ix86-32/iR5900.cpp Outdated Show resolved Hide resolved
pcsx2/x86/ix86-32/iR5900.cpp Outdated Show resolved Hide resolved
pcsx2/x86/ix86-32/iR5900.cpp Outdated Show resolved Hide resolved
pcsx2/MemoryTypes.h Outdated Show resolved Hide resolved
pcsx2/Memory.h Outdated Show resolved Hide resolved
@stenzek
Copy link
Member

stenzek commented Apr 21, 2024

Forgot to mention before, this should be added to VMManager::WarnAboutUnsafeSettings() as well.

@F0bes
Copy link
Member

F0bes commented Apr 24, 2024

Please don't do merge commits to update your branch. Instead rebase your branch onto our master and resolve any conflicts that way.

@Mrlinkwii
Copy link
Contributor

Mrlinkwii commented Apr 25, 2024

the Linux crash has been fixed , the pr will probably need a bit more testing but its looking good

@DaZombieKiller DaZombieKiller force-pushed the extra-memory branch 3 times, most recently from 6f09293 to 0235920 Compare April 30, 2024 00:56
@DaZombieKiller
Copy link
Contributor Author

Could you provide a link to download the modifications and try them on our own? We would sincerely appreciate it very much. Thanks in advance

You can download CI builds from the Checks tab on this pull request. Here is the latest SSE4 Windows build.

@DaZombieKiller
Copy link
Contributor Author

Could you provide a link to download the modifications and try them on our own? We would sincerely appreciate it very much. Thanks in advance

You can download CI builds from the Checks tab on this pull request. Here is the latest SSE4 Windows build.

Thanks for the clarification, but I can't find the advanced options section, I'm sorry for the inconvenience and questions asked but I'm very excited about this. Could you help me enable that option?

Advanced options are hidden by default, you need to enable them through Tools -> Show Advanced Settings. If you need any additional help I suggest joining the PCSX2 Discord to avoid derailing the issue discussion.

pcsx2/Achievements.cpp Outdated Show resolved Hide resolved
pcsx2/SaveState.cpp Outdated Show resolved Hide resolved
pcsx2/Memory.cpp Outdated Show resolved Hide resolved
pcsx2/x86/ix86-32/iR5900.cpp Outdated Show resolved Hide resolved
@DaZombieKiller DaZombieKiller force-pushed the extra-memory branch 2 times, most recently from e812f74 to 557fc7b Compare May 7, 2024 07:42
The psHu64, psHu128 and psSu64 macros are also unused, but are kept for completeness.
Copy link
Member

@stenzek stenzek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM, untested.

@kamfretoz
Copy link
Contributor

Latest changes seems to be causing crash on Linux if you try to load save state with mismatched memory size (128MB on 32MB).

NOTE: Looks to be a Linux-only issue.

Reproduction step:

  • Enable 128MB Memory on settings
  • Fire up a game and create a save state
  • Exit from the game and disable the 128MB Memory
  • Fire up the game again and try to load the save state

Crash Log

ReportErrorAsync: Failed to load save state: Memory size mismatch, save state requires 128MB, but VM currently has 32MB.
EE/iR5900 Recompiler Reset
psxUNK: 01000701
psxUNK: 00000001
psxUNK: 0001a7b0
psxUNK: 0001a7b0
psxUNK: 0001b270
Unknown R5900 COP0: 435C0000
Unknown R5900 COP0: 435C0000
EE: Unrecognized COP0 op 435c0000
EE: Unrecognized COP0 op 435c0000
*************** Unhandled SIGSEGV at 0x649489b0001b ***************
  0x00649446e684e7 CrashSignalHandler [/home/runner/work/pcsx2/pcsx2/common/CrashHandler.cpp:311]
  0x007d831c641b2f
  0x00649489b0001b
*******************************************************************
fish: Job 1, './PCSX2-linux-Qt-x64-appimage-s…' terminated by signal SIGABRT (Abort)

@stenzek
Copy link
Member

stenzek commented May 8, 2024

Hm. That probably means it's not resetting the VM on failed load.

Probably need a call to VMManager::Reset() on

https://github.com/PCSX2/pcsx2/blob/master/pcsx2/SaveState.cpp#L1151 and
https://github.com/PCSX2/pcsx2/blob/master/pcsx2/SaveState.cpp#L1166

@DaZombieKiller
Copy link
Contributor Author

I notice that there are a bunch of TLB misses reported in the log when doing this on Windows. It just happens to not always crash (which does seem consistent with the VM not being reset). Calling VMManager::Reset on a failed load indeed makes those TLB misses vanish. Hopefully that was it and this no longer crashes on Linux either.

@kamfretoz
Copy link
Contributor

Hm. That probably means it's not resetting the VM on failed load.

Probably need a call to VMManager::Reset() on

master/pcsx2/SaveState.cpp#L1151 and master/pcsx2/SaveState.cpp#L1166

That fixes it!

image

@DaZombieKiller
Copy link
Contributor Author

For testing purposes, here are some sample ELFs: 128mb-samples.zip

  • alloc_96.elf will attempt to allocate 96MB of memory and log whether or not it was successful to the EE console. It should log Failure! in 32MB mode and Success! in 128MB mode.
  • high_ep.elf is a simple hello world program with its entry point located outside of the 32MB range. Currently it will crash the emulator in 32MB mode but will run fine in 128MB mode. Is it maybe worth validating that the entry point of the elf is within range for the current memory mode and then displaying a warning to the user?

@stenzek
Copy link
Member

stenzek commented May 8, 2024

I... don't really see why the entry point being above 32MB is an issue? Unless it's the BIOS/bootstrap replacing the hardcoded TLB mappings, and the higher address isn't mapped yet.

That said, if you have more than 32MB of code/data, you have greater problems. :D

@stenzek stenzek merged commit 2fc4d02 into PCSX2:master May 9, 2024
12 checks passed
@DaZombieKiller DaZombieKiller deleted the extra-memory branch May 9, 2024 03:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants