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

Jump to bottom

DVDInterface: Chunking and various timing accuracy improvements #4829

Merged
merged 2 commits into from Feb 8, 2017
Merged

DVDInterface: Chunking and various timing accuracy improvements #4829

merged 2 commits into from Feb 8, 2017

Conversation

JosJuice
Copy link
Member

@JosJuice JosJuice commented Feb 5, 2017 •
edited

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 mentioned this pull request Feb 5, 2017
Closed
@mmastrac
Copy link
Member

mmastrac commented Feb 5, 2017

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

mmastrac
mmastrac reviewed Feb 5, 2017
View reviewed changes
Source/Core/Core/HW/DVDInterface.cpp
{
// 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 was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

mmastrac
mmastrac approved these changes Feb 5, 2017
View reviewed changes
Copy link
Member

@mmastrac mmastrac left a comment

Choose a reason for hiding this comment

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

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

mmastrac
mmastrac reviewed Feb 5, 2017
View reviewed changes
Source/Core/Core/HW/DVDInterface.cpp Outdated
@@ -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 was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@mmastrac
Copy link
Member

mmastrac commented Feb 5, 2017 •
edited

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 dvd_chunk branch 2 times, most recently from 675535f to a9ae891 Compare February 5, 2017 18:40
@JosJuice
Copy link
Member Author

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.

mmastrac
mmastrac approved these changes Feb 5, 2017
View reviewed changes
Copy link
Member

@mmastrac mmastrac left a comment

Choose a reason for hiding this comment

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

LGTM!

@mmastrac
Copy link
Member

mmastrac commented Feb 5, 2017 •
edited

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

mmastrac
mmastrac reviewed Feb 6, 2017
View reviewed changes
Source/Core/Core/State.cpp Outdated
@@ -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 was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@mmastrac @JosJuice
DVD read chunking
961f84b 
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.
@JMC47
Copy link
Contributor

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
@JosJuice JosJuice deleted the dvd_chunk branch February 8, 2017 14:43
@JosJuice JosJuice mentioned this pull request Feb 8, 2017
Closed
@mmastrac
Copy link
Member

mmastrac commented Feb 8, 2017

Thanks for taking over and landing this, @JosJuice

🎉 🎉 🎉

@JosJuice JosJuice mentioned this pull request Feb 10, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmastrac mmastrac mmastrac approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
3 participants
@JosJuice @mmastrac @JMC47