Fix save corruption in some games - #105
Open
max-las wants to merge 2 commits into
Open
Conversation
Author
|
@bbsan2k watch out: I've edited this PR since I opened it. Please tell me what you think of it |
Collaborator
|
Hmm I think I see your point. Looks like you figured out why we need the lockout timing 😉 It's kind of a hysteresis to make sure we are not excessively writing to sd card, slowing the whole operation down. The sequence to write even single bytes into the cache was chosen to make sure, the whole data management and caching is delegated to the data_interface. I added writing single bytes to the PSRAM, since I flushing an entire sector to PSRAM could lead to the checksum not being able to be responded to in time for escaping the PS1 byte timeout. What do you think about following approach_ Does it resolve your issue as well? void __time_critical_func(ps1_mc_data_interface_write_byte)(uint32_t address, uint8_t byte) {
ps1_dirty_lockout_renew();
#if WITH_PSRAM
card[address%PS1_PAGE_SIZE] = byte;
#else
card[address] = byte;
#endif
write_occured = true;
}
void __time_critical_func(ps1_mc_data_interface_write_mc)(uint32_t page) {
ps1_dirty_lockout_renew();
ps1_dirty_lock();
#if WITH_PSRAM
psram_wait_for_dma();
psram_write_dma(page * PS1_PAGE_SIZE, card, PS1_PAGE_SIZE, NULL);
psram_wait_for_dma();
#endif
ps1_dirty_mark(page);
ps1_dirty_unlock();
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I faced save corruption in some PS1 games. I one game in particular (Adibou Et L'ombre Verte - SCES-03558) I could consistently reproduce corruption by turning off the console a few seconds after saving.
It turns out the lockout was too low for this game: I measured up to 436ms of gap between lockout renewals. Increasing the lockout to 500ms fixed it. But that gap includes the lockout renewals for read (
ps1_mc_data_interface_start_dma). Measuring lockout renewals for writes only, the gap drops to 41ms. So I left the lockout to 100ms and instead stopped renewing the lockout for reads, and it worked too. That's the current state of this PR. But that leaves one question: why was the lockout renewed for reads? If there's a good reason, then I'd rollback to the 500ms fix.Also during my investigation I found two other theoretical races that I patched as well:
dma_rx_doneraises CS to 1 and it wasn't pulled back to 0 after waitingwhile(dma_active).ps1_dirty_mark(page)was called without locking inps1_mc_data_interface_write_mc. Here I mirrored the ps2 logic: write one sector in one lock (which encompassesps1_dirty_mark(page)), instead of doing all the wait, lockout renewal, and locking per byte. I think it's better.Note: I'm playing PAL games on a SuperStation One which might explain the surprising gap between lockout renewals if tests are usually targeted to a genuine NTSC PS1.