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: Translate and align offsets for timing purposes #1900

Closed

Conversation

JosJuice
Copy link
Member

This is a fairly large timing change for decrypted reads and a smaller timing change for non-decrypted reads.

Review on Reviewable

@@ -1291,7 +1343,7 @@ u64 SimulateDiscReadTime(u64 offset, u32 length)
}
}

g_last_read_offset = (offset + length - 2048) & ~2047;
g_last_read_offset = end_offset;

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@JosJuice JosJuice force-pushed the dvd-timing-address-translation branch 3 times, most recently from 9cd445b to 33ef393 Compare January 17, 2015 21:08
@JosJuice
Copy link
Member Author

I've added a fix for buffered decrypted reads. Not sure how good it is.

@delroth
Copy link
Member

delroth commented Jan 17, 2015

@dolphin-emu-bot rebuild (sorry, was doing some buildbot maintenance at the same time)

@JosJuice JosJuice force-pushed the dvd-timing-address-translation branch from 33ef393 to 41f5401 Compare January 18, 2015 09:31
@JosJuice JosJuice force-pushed the dvd-timing-address-translation branch 2 times, most recently from b6b8eeb to 2596b32 Compare January 27, 2015 08:42
@JosJuice JosJuice force-pushed the dvd-timing-address-translation branch from 2596b32 to fe85daf Compare February 26, 2015 11:53
@JosJuice JosJuice force-pushed the dvd-timing-address-translation branch from fe85daf to 6738d3a Compare March 13, 2015 10:23
@Buddybenj
Copy link
Contributor

Ping?

@skidau
Copy link
Contributor

skidau commented Mar 18, 2015

How did you test this? How did you tell that this is more accurate?

@JosJuice
Copy link
Member Author

I don't have a good way to test this, really... :/ I guess it would be possible to compare the loading times in some Wii game, but if we want something less approximate, writing custom homebrew software would probably be needed. CleanRip won't help with this, because it doesn't decrypt and it reads everything using big, aligned requests. But while I don't know how accurate things are with this PR, I know that the old way of completely ignoring decryption can't be right. It would require the disc drive to magically read faster whenever IOS wants to decrypt data.

@JosJuice JosJuice force-pushed the dvd-timing-address-translation branch 5 times, most recently from 733b1e6 to 03a1b55 Compare March 24, 2015 15:57
@JosJuice JosJuice force-pushed the dvd-timing-address-translation branch from 03a1b55 to 63803b7 Compare April 2, 2015 09:49
@shuffle2
Copy link
Contributor

shuffle2 commented May 8, 2015

It's hard to say if this is more accurate or not. My guess is that overall it's not really more accurate, since the proper simulation would be having our IOS HLE simulate IOS' dvd read strategy when files are accessed (in addition to something similar to what this commit does).

So, I'm not really in favor of the commit. However, if it is going to be committed, please make all the magic sizes be constant variables so that they have names.

@JosJuice JosJuice force-pushed the dvd-timing-address-translation branch from 63803b7 to 3397ccb Compare May 9, 2015 15:44
@JosJuice
Copy link
Member Author

JosJuice commented May 9, 2015

The constants from VolumeWiiCrypted are now used, along with a SECTOR_SIZE constant.

@JosJuice JosJuice force-pushed the dvd-timing-address-translation branch from 3397ccb to 5707e50 Compare June 11, 2015 16:07
@JosJuice JosJuice force-pushed the dvd-timing-address-translation branch from 5707e50 to 4211d49 Compare August 10, 2015 17:06
@JosJuice JosJuice force-pushed the dvd-timing-address-translation branch from 4211d49 to d7fed99 Compare September 4, 2015 15:42
@JosJuice JosJuice force-pushed the dvd-timing-address-translation branch from d7fed99 to 2e5c3d6 Compare September 12, 2015 15:47
@JosJuice JosJuice force-pushed the dvd-timing-address-translation branch from 2e5c3d6 to a9b1338 Compare September 21, 2015 15:07
@JosJuice JosJuice force-pushed the dvd-timing-address-translation branch 3 times, most recently from ec01089 to a791d03 Compare October 4, 2015 19:45
@JosJuice JosJuice force-pushed the dvd-timing-address-translation branch from a791d03 to 814154d Compare November 17, 2015 10:57
@JosJuice JosJuice force-pushed the dvd-timing-address-translation branch 2 times, most recently from d69f44e to 1036f0a Compare December 20, 2015 12:24
@JosJuice JosJuice force-pushed the dvd-timing-address-translation branch from 1036f0a to 5b87b21 Compare January 28, 2016 13:34
@JosJuice JosJuice force-pushed the dvd-timing-address-translation branch 2 times, most recently from 6fd6e45 to 769c7be Compare March 1, 2016 22:06
@JosJuice
Copy link
Member Author

JosJuice commented Mar 1, 2016

A hardware test by @mmastrac showed that disc drive reads are aligned to 32 KiB, the size of an ECC block, which makes sense. Because of that, I've changed this PR from aligning to 2 KiB to aligning to 32 KiB. Also, I removed the - 2048 because it didn't exist in the test results as far as I understood them. There is still code that ensures that small sequential reads will be covered by the buffered code path.

This is a fairly large timing change for decrypted reads
and a smaller timing change for non-decrypted reads.
It only roughly matches the last read offset. What we actually
use it for is determining where the buffer got its data from.
@JosJuice
Copy link
Member Author

PR #4829 makes half of this PR obsolete. The other half needs to be more or less rewritten. I'll open a new PR for that later.

@JosJuice JosJuice closed this Feb 10, 2017
@JosJuice JosJuice deleted the dvd-timing-address-translation branch February 15, 2017 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants