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

DVDInterface: Don't evict cache block i unless block i + 2 was read #9847

Merged
merged 2 commits into from Jun 30, 2021

Conversation

JosJuice
Copy link
Member

Intends to fix https://bugs.dolphin-emu.org/issues/12279. I have hardware tested the behavior, but I haven't tested the game.

@Avasam
Copy link
Contributor

Avasam commented Jun 27, 2021

As was mentioned on IRC, while not a "complete" test, my early results seem to show that timing is on par with GameCube, and maybe ever so slightly slower than Wii.
Timing comparison

This also fixes Pitfall: The Big Adventure (Wii port), which had the exact same issue. I can't really tell exact timing differences, because I don't have enough data. But as far as I, and the rest of the Pitfall Speed running community, is concerned, this looks good and pretty close to console.

Timing comparisons as compiled by our community: https://docs.google.com/spreadsheets/d/16kTAM4g4qUmREwehROz_tzjOS3DXJHLGa44FKWy7GNI/edit#gid=0

// Update the buffer based on this read. Based on hardware testing, the blocks which are kept are
// the most recently accessed block, the block immediately before it, and all blocks after it.
// If the block immediately before the most recently accessed block is not kept, we get too long
// loading screens in Pitfall: The Lost Expedition (https://bugs.dolphin-emu.org/issues/1227).
Copy link
Contributor

Choose a reason for hiding this comment

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

This link is wrong; it should be https://bugs.dolphin-emu.org/issues/12279 (it's missing the 9).

Also, "we get too long loading screens" sounds a bit awkward to me; maybe "loading takes longer than it should" would be better?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated the comment.

Intends to fix https://bugs.dolphin-emu.org/issues/12279.
I have hardware tested the behavior, but I haven't tested the game.
If we seek to a block that isn't in the cache, the block prior
to it doesn't end up getting read into the buffer.
Copy link
Contributor

@Pokechu22 Pokechu22 left a comment

Choose a reason for hiding this comment

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

Code looks good to me.

It might be useful to implement DICommand::Seek at some point, but it doesn't seem to be used by many games in my experience. As far as I know, it essentially acts as a read except it doesn't perform the DMA afterwards. Unfortunately I'm not sure where I read that. But it's not something that needs to be done for this PR.

@JMC47
Copy link
Contributor

JMC47 commented Jun 30, 2021

I retested again to make sure everything looked fine. I'm assuming this won't cause any regressions, but who knows.

@JMC47 JMC47 merged commit 04a1c2e into dolphin-emu:master Jun 30, 2021
11 checks passed
@JosJuice JosJuice deleted the dvd-pitfall branch June 30, 2021 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants