DVDInterface: Translate Wii partition offsets for timing purposes #4883

Merged
merged 2 commits into from Feb 15, 2017

Projects

None yet

2 participants

@JosJuice
Contributor

Until now, Dolphin has been using the wrong values for calculating DVD timing for decrypted Wii reads (which Wii games essentially always use).

It would be nice to get this merged before the next progress report, because otherwise the DVD chunking section of it will won't be able to claim that our timing is accurate for Wii games.

I've tested that both GC and Wii games still work fine with this, but I haven't done any deeper timing testing.

@mmastrac Do the changes to the ScheduleReads logic look fine to you?

@JosJuice
Contributor

For the record, the decryption and address translation is done by IOS on real hardware, and mmastrac's timing testing was done without IOS as far as I know.

@mmastrac
Contributor

@JosJuice Great. I'll read through it later today.

@@ -1190,6 +1190,15 @@ void ScheduleReads(u64 dvd_offset, u32 dvd_length, bool decrypt, u32 output_addr
// If we fall within its bounds, we get DMA-speed reads.
u64 buffer_start, buffer_end;
+ // The variable offset uses the same addressing as games do.
@mmastrac
mmastrac Feb 10, 2017 Contributor

See below for more context, but I'd suggest that we omit the changes to this method. In fact, I'd move this code to the start of ExecuteReadCommand instead.

@mmastrac
mmastrac Feb 10, 2017 Contributor

Ignore this comment now that I have a better understanding.

Source/Core/Core/HW/DVDInterface.cpp
- // Where the read actually takes place on disc
- u64 rounded_offset = Common::AlignDown(dvd_offset, DVD_ECC_BLOCK_SIZE);
+ const u32 bytes_per_chunk =
+ decrypt ? DiscIO::CVolumeWiiCrypted::BLOCK_DATA_SIZE : DVD_ECC_BLOCK_SIZE;
@mmastrac
mmastrac Feb 10, 2017 Contributor

I'm not totally convinced we need to model the DVD transfers w/smaller blocks. It would be much simpler just to add a constant decryption overhead based on read size (ie: estimate the time per byte and add that into the scheduling delay).

I believe that the DMA transfers (plus decryption) for the disc to memory are quite small but the are "practically instantaneous" when you compare them against the disc read speed.

@JosJuice
JosJuice Feb 10, 2017 edited Contributor

In a Wii partition, each 32 KiB block contains 31 KiB of data that actually gets passed to the game and 1 KiB of data that only IOS uses. This means that 1 GiB of data on the DVD only contains about 0.97 GiB of data that the game uses. Since the length parameter that's passed in to ScheduleReads uses the addressing scheme where that would be 0.97 GiB, we have to convert it to actual DVD offsets, as we otherwise will get wrong values for both length and speed in CalculateRawDiscReadTime.

I'm currently not modeling any decryption delay. The intent of the PR is just to not use the wrong values for the physical DVD calculations.

@JosJuice
JosJuice Feb 10, 2017 edited Contributor

I realize now that the ticks_until_completion calculation shouldn't be using chunk_length, but rather the actual bytes read from the DVD. My guess for how it works on hardware is that IOS will read the whole block from DI the first time the PPC wants to read it, and then keep it in an IOS-internal cache as long as PPC doesn't request another block. It's only a guess, though. Is that worth implementing? The difference won't be especially big, and it's probably not longer than the time it takes to decrypt. (I don't know how long it takes to decrypt.)

}
else
{
// Unbuffered read
- ticks_until_completion += CalculateRawDiscReadTime(rounded_offset, DVD_ECC_BLOCK_SIZE);
+ ticks_until_completion += CalculateRawDiscReadTime(dvd_offset, DVD_ECC_BLOCK_SIZE);
@mmastrac
mmastrac Feb 10, 2017 Contributor

This read time appears to be using the 32kB ECC size, but in decrypt mode we're stepping dvd_offset by a much smaller amount each time through the loop.

@JosJuice
JosJuice Feb 10, 2017 Contributor

No, dvd_offset is always increased by DVD_ECC_BLOCK_SIZE at the end of the loop, regardless of decrypt.

@mmastrac
mmastrac Feb 10, 2017 Contributor

OK, this makes sense to me. I thought the decryption block size was smaller than it actually was.

Source/Core/DiscIO/VolumeWiiCrypted.h
- static const unsigned int s_block_header_size = 0x0400;
- static const unsigned int s_block_data_size = 0x7C00;
- static const unsigned int s_block_total_size = s_block_header_size + s_block_data_size;
+ static const unsigned int BLOCK_HEADER_SIZE = 0x0400;
@mmastrac
mmastrac Feb 10, 2017 Contributor

While you are in here making updates, should this be const_expr?

@JosJuice
JosJuice Feb 11, 2017 Contributor

Done.

@mmastrac
Contributor

I need to think about this a bit more, but I think that if we are going to try to be really accurate about this stuff there's other things that need to change - ie: the 1MB streaming read buffer is technically 1,015,808 bytes instead.

We could get most of the way there by only translating the offset we use for timing calculations. Block sizes would be wrong, but the games don't appear to rely on the exact size, rather than the data is DMA'd approximately linearly over the period of a read.

@JosJuice
Contributor
JosJuice commented Feb 10, 2017 edited

I need to think about this a bit more, but I think that if we are going to try to be really accurate about this stuff there's other things that need to change - ie: the 1MB streaming read buffer is technically 1,015,808 bytes instead.

No, the read buffer should have the same size regardless. The read buffer calculations are done using dvd_offset right now, and dvd_offset always represents what's on the disc, ignoring what IOS is doing.

We could get most of the way there by only translating the offset we use for timing calculations.

That's what this PR does. It uses translated 32 KiB blocks for timing calculations and untranslated 31 KiB blocks for copying data to the emulated RAM.

@mmastrac

After making sure I understood how this works, looks good to me with one small change.

Source/Core/Core/HW/DVDInterface.cpp
{
- // Where the read actually takes place on disc
- u64 rounded_offset = Common::AlignDown(dvd_offset, DVD_ECC_BLOCK_SIZE);
+ const u32 bytes_per_chunk =
@mmastrac
mmastrac Feb 12, 2017 Contributor

Let's move this outside the loop (the compiler probably optimizes it, but it's better to avoid relying on it)

@JosJuice
JosJuice Feb 12, 2017 Contributor

Done.

@JosJuice JosJuice DVDInterface: Translate Wii partition offsets for timing purposes
Until now, Dolphin has been using the wrong values
for calculating DVD timing for decrypted Wii reads
(which Wii games essentially always use).
49ec22b
@JosJuice JosJuice merged commit f80f7b6 into dolphin-emu:master Feb 15, 2017

10 checks passed

default Very basic checks passed, handed off to Buildbot.
Details
lint Build succeeded on builder lint
Details
pr-android Build succeeded on builder pr-android
Details
pr-deb-dbg-x64 Build succeeded on builder pr-deb-dbg-x64
Details
pr-deb-x64 Build succeeded on builder pr-deb-x64
Details
pr-freebsd-x64 Build succeeded on builder pr-freebsd-x64
Details
pr-osx-x64 Build succeeded on builder pr-osx-x64
Details
pr-ubu-x64 Build succeeded on builder pr-ubu-x64
Details
pr-win-dbg-x64 Build succeeded on builder pr-win-dbg-x64
Details
pr-win-x64 Build succeeded on builder pr-win-x64
Details
@JosJuice JosJuice deleted the JosJuice:dvd-timing-address-translation-2 branch Feb 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment