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: Increase the latency for read commands #8935

Merged
merged 1 commit into from Sep 8, 2020

Conversation

JosJuice
Copy link
Member

@JosJuice JosJuice commented Jul 5, 2020

Hardware tested using cached reads.

Fixes Ed, Edd n Eddy (untested). https://bugs.dolphin-emu.org/issues/10373

@@ -1378,8 +1379,8 @@ void ScheduleReads(u64 offset, u32 length, const DiscIO::Partition& partition, u
"Schedule reads: offset=0x%" PRIx64 " length=0x%" PRIx32 " address=0x%" PRIx32, offset,
length, output_address);

// The DVD drive's minimum turnaround time on a command, based on a hardware test.
Copy link
Contributor

Choose a reason for hiding this comment

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

This line seems to indicate there was a hardware test before. Any idea what changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

If I'm understanding it correctly, the earlier hardware test determined that 300 µs was the fastest that any command can complete. This is a more complex command, so it makes sense that its latency would be higher than the latency of the simplest command.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I guess we can only ever approximate this. What effect does this have on games that don't need this change? Longer load times?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but just a tiny bit.

@Tilka
Copy link
Member

Tilka commented Jul 5, 2020

Tested and confirmed working. Previously the loading screen for the first level would get stuck and now I get into the first level.

@Pokechu22
Copy link
Contributor

Pokechu22 commented Jul 5, 2020

I can confirm that this allows getting into the first level; however, after getting into the first level, if I then pause the game, select options (by pressing right), and then select "Exit to Cul-De-Sac", the game freezes. The following is put in the log (I don't always get the invalid read part):

21:00:967 Core\PowerPC\PPCCache.cpp:192 I[PowerPC]: ICache read at 00da813c returned stale data: CACHED: 83c10020 vs. RAM: 9421ff68
21:00:967 Core\PowerPC\PPCCache.cpp:192 I[PowerPC]: ICache read at 00da8140 returned stale data: CACHED: 83e10024 vs. RAM: 7c0802a6
21:00:967 Core\PowerPC\PPCCache.cpp:192 I[PowerPC]: ICache read at 00da8144 returned stale data: CACHED: 38210028 vs. RAM: 91c10050
21:00:967 Core\PowerPC\PPCCache.cpp:192 I[PowerPC]: ICache read at 00da8148 returned stale data: CACHED: 4e800020 vs. RAM: 91e10054
21:00:968 Core\PowerPC\Jit64\Jit.cpp:784 W[PowerPC]: ISI exception at 0x6f693a70
21:00:978 Common\MsgHandler.cpp:115 E[MASTER]: Question: Invalid read from 0x64796e67, PC = 0x80144568 
21:32:390 Common\MsgHandler.cpp:115 E[MASTER]: Question: Invalid read from 0x64796e6b, PC = 0x80144570 
21:33:251 Common\MsgHandler.cpp:115 E[MASTER]: Question: Invalid read from 0x64796e67, PC = 0x80144580 

It also freezes upon selecting "Exit to main menu" and then loading the game again (though it does play, or at least start, the cutscene), with just this:

16:46:449 Core\PowerPC\PPCCache.cpp:192 I[PowerPC]: ICache read at 00dac280 returned stale data: CACHED: 4e800020 vs. RAM: 3d2080d7

This icache behavior is the same as with Scooby Doo! Mystery Mayhem (Bug 9561, a false positive where the existing icache check is returning stale data, when it probably shouldn't). I'll test.

EDIT: Tested, and the same hack that I found works with Scooby Doo! Mystery Mayhem works here (when combined with this change), namely changing this to return inmem. Both forms of exiting stop causing issues.

EDIT 2: Note that the ICache issue only comes up when MMU is disabled. When MMU is enabled, no changes beyond this PR seem to be needed.

// microseconds)
constexpr u64 COMMAND_LATENCY_US = 300;
// The minimum time it takes for the DVD drive to process a command (in microseconds)
constexpr u64 MINIMUM_COMMAND_LATENCY_US = 300;

Choose a reason for hiding this comment

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

I get that the _US, is supposed to stand for microseconds (µs), but this is redundant, since it's already explained in the comment. Like it is, it can be confused for "United States", as if this latency is specific to US consoles. I recommend that part of the name to be omitted, reading MINIMUM_COMMAND_LATENCY

Copy link
Member Author

Choose a reason for hiding this comment

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

It's only redundant if you're looking at this specific line. The suffix is useful when you see the variable name referenced in the code, as it explains why we're dividing by 1000000.

Choose a reason for hiding this comment

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

Got it!

@lioncash lioncash merged commit 48cfc32 into dolphin-emu:master Sep 8, 2020
10 checks passed
@JosJuice JosJuice deleted the di-read-latency branch September 8, 2020 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants