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: Chunking and various timing accuracy improvements #4829

Merged
merged 2 commits into from Feb 8, 2017

Conversation

@JosJuice
Copy link
Member

@JosJuice JosJuice commented Feb 5, 2017

This is a rebased and cleaned up version of PR #3701.

This PR has two primary improvements: Copying disc data to the emulated RAM in chunks instead of all in one go at the end (fixes various games, including Sonic Riders) and using more accurate timing models that take chunking and seek timing into account (makes loading times closer to consoles but doesn't fix any bugs as far as I know).

All comments on the old PR should be fixed except for the ones that I made today. EDIT: Those two comments have been addressed now.

@JosJuice JosJuice force-pushed the JosJuice:dvd_chunk branch from 2ef721e to b6b4b28 Feb 5, 2017
@mmastrac
Copy link
Contributor

@mmastrac mmastrac commented Feb 5, 2017

Thanks for doing this! I'll review shortly. Are we not using Reviewable anymore?

{
// The amount of data the buffer contains *right now*, rounded to a DVD ECC block.
buffer_end = s_read_buffer_start_offset +
Common::AlignDown((current_time - s_read_buffer_start_time) *

This comment has been minimized.

@mmastrac

mmastrac Feb 5, 2017
Contributor

Assuming that AlignDown is the same as ROUND_DOWN

This comment has been minimized.

@JosJuice

JosJuice Feb 5, 2017
Author Member

As far as I know, the only difference is that AlignDown works even when the second argument isn't a power of 2. Usage of ROUND_DOWN was replaced with AlignDown in PR #4482.

Copy link
Contributor

@mmastrac mmastrac left a comment

From a code perspective, looks good to me with one minor comment about a constant.

@@ -1120,7 +1112,8 @@ void ExecuteCommand(u32 command_0, u32 command_1, u32 command_2, u32 output_addr
// to simulate the speed of a real disc drive
if (!command_handled_by_thread)
{
CoreTiming::ScheduleEvent(ticks_until_completion, s_finish_executing_command,
// This is an arbitrary delay - needs testing to determine the appropriate value
CoreTiming::ScheduleEvent(SystemTimers::GetTicksPerSecond() / 15000, s_finish_executing_command,

This comment has been minimized.

@mmastrac

mmastrac Feb 5, 2017
Contributor

Weird. GH didn't save my comment here, but we should extract this / 15000 into a constant (66.6 microseconds).

This comment has been minimized.

@JosJuice

JosJuice Feb 5, 2017
Author Member

COMMAND_LATENCY_US is used instead now.

@mmastrac
Copy link
Contributor

@mmastrac mmastrac commented Feb 5, 2017

Bringing two old Reviewable discussions from @JosJuice here:

          if (rounded_offset != head_position)
          {
            // Unbuffered seek+read

There can probably be cases where it would be faster to wait for the head to continue as usual and read more data into a buffer that is about to become complete soon instead of seeking, considering that a seek always takes at least 45 ms according to our model. The old code always picked the faster of the two.

The drive didn't appear to be smart about optimizing for this. In fact, it was quite dumb in a lot of cases (it would throw away buffers when seeking backward, even if they overlapped). From my experimental data I never saw any evidence that it would wait for a streaming read if it was faster. I think it's safe to assume that this is not the case.

          {
            // Unbuffered seek+read
            ticks_until_completion += CalculateSeekTime(head_position, rounded_offset);

You're only adding the seek time, not the read time. It looks like a mistake, but could it be intentional because the read time for one block is baked into the value returned by CalculateSeekTime?

That's correct. I wasn't able to formulate an experiment to determine the seek time separate from the read time, so I just baked that read of one block into this number. We should add a short comment to that effect.

@JosJuice JosJuice force-pushed the JosJuice:dvd_chunk branch 2 times, most recently from 675535f to a9ae891 Feb 5, 2017
@JosJuice
Copy link
Member Author

@JosJuice JosJuice commented Feb 5, 2017

That's correct. I wasn't able to formulate an experiment to determine the seek time separate from the read time, so I just baked that read of one block into this number. We should add a short comment to that effect.

I've added that information to the comment above the definition of CalculateSeekTime.

Copy link
Contributor

@mmastrac mmastrac left a comment

LGTM!

@mmastrac
Copy link
Contributor

@mmastrac mmastrac commented Feb 5, 2017

Pulling the test strategy from the other PR:

  • Arc Rise Fantasia cutscenes (DVD-timing sensitive)
  • Sonic Riders (DVD-timing sensitive)
  • Anything GameCube
  • Any disc with two layers (eg: Last Story)
  • Save state during heavy disc access
@JosJuice JosJuice force-pushed the JosJuice:dvd_chunk branch from a9ae891 to 41ae2b4 Feb 5, 2017
@@ -71,7 +71,7 @@ static Common::Event g_compressAndDumpStateSyncEvent;
static std::thread g_save_thread;

// Don't forget to increase this after doing changes on the savestate system
static const u32 STATE_VERSION = 73; // Last changed in PR 4651
static const u32 STATE_VERSION = 74; // Last changed in PR 3701

This comment has been minimized.

@mmastrac

mmastrac Feb 6, 2017
Contributor

PR # here is wrong.

This comment has been minimized.

@JosJuice

JosJuice Feb 6, 2017
Author Member

Fixed. Though it doesn't really matter, because PR 3701 isn't going to get merged and introduce a different savestate layout.

@JosJuice JosJuice force-pushed the JosJuice:dvd_chunk branch from 41ae2b4 to 423e9a3 Feb 6, 2017
Splits DVD reads up into smaller chunks so that data is available
before the final interrupt is triggered. This better simulates the DMA
that happens on a real device, which some games will take advantage of -
by either playing back data as it is loading or by using data that is
going to be overwritten shortly by an outstanding read.
@JosJuice JosJuice force-pushed the JosJuice:dvd_chunk branch from 423e9a3 to 961f84b Feb 8, 2017
@JMC47
Copy link
Contributor

@JMC47 JMC47 commented Feb 8, 2017

Everything looks good to me. checked the INIs over.

@JosJuice JosJuice merged commit a2750a8 into dolphin-emu:master Feb 8, 2017
10 checks passed
10 checks passed
@dolphin-emu-bot
default Very basic checks passed, handed off to Buildbot.
Details
@dolphin-emu-bot
lint Build succeeded on builder lint
Details
@dolphin-emu-bot
pr-android Build succeeded on builder pr-android
Details
@dolphin-emu-bot
pr-deb-dbg-x64 Build succeeded on builder pr-deb-dbg-x64
Details
@dolphin-emu-bot
pr-deb-x64 Build succeeded on builder pr-deb-x64
Details
@dolphin-emu-bot
pr-freebsd-x64 Build succeeded on builder pr-freebsd-x64
Details
@dolphin-emu-bot
pr-osx-x64 Build succeeded on builder pr-osx-x64
Details
@dolphin-emu-bot
pr-ubu-x64 Build succeeded on builder pr-ubu-x64
Details
@dolphin-emu-bot
pr-win-dbg-x64 Build succeeded on builder pr-win-dbg-x64
Details
@dolphin-emu-bot
pr-win-x64 Build succeeded on builder pr-win-x64
Details
@JosJuice JosJuice deleted the JosJuice:dvd_chunk branch Feb 8, 2017
@mmastrac
Copy link
Contributor

@mmastrac mmastrac commented Feb 8, 2017

Thanks for taking over and landing this, @JosJuice

🎉 🎉 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants