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

Adjust cycle counts so they are accurate to the JIT block level (Fixes OoT virtual console and other games) #3601

Merged
merged 5 commits into from Mar 25, 2016

Conversation

phire
Copy link
Member

@phire phire commented Feb 6, 2016

Previously GlobalTimer was only updated at the end of each slice when CoreTiming::Advance() was called, so it could be upto 20,000 cycles off. This was causing huge problems with games which made heavy use of the time base register, such as OoT (virtual console) and Pokemon puzzle.

I've also made it so event scheduling will be accurate to the JIT block level, instead of accurate to the slice.

Review on Reviewable

@@ -424,6 +452,8 @@ void Advance()
slicelength = maxSliceLength;
PowerPC::ppcState.downcount = CyclesToDowncount(slicelength);
}

This comment was marked as off-topic.

@delroth
Copy link
Member

delroth commented Feb 6, 2016

Should this be a candidate for 5.0 inclusion? My gut feeling says "no" since it touches JIT stuff, but if it fixes a regression I'll let it in.

@phire
Copy link
Member Author

phire commented Feb 6, 2016

As far as I know it doesn't fix any regressions, more or less none of the Virtual Console games worked in 4.0. It would be nice to have this in 5.0 as it would rounds out our virtual console support to "more or less all of them except Majora's Mask"

I'm not really concerned about the JIT change, it's minor and takes a few min to verify. I'm much more concerned about the massive timing changes it makes.

@JMC47
Copy link
Contributor

JMC47 commented Feb 6, 2016

PAL Virtual Console games seem to crash with this change. Super Mario 64 and Super Smash Bros. PAL, and The Legend of Zelda: Majora's Mask. Honestly I'm not sure if anything outside of the NTSC Virtual Console games is working?

@JMC47
Copy link
Contributor

JMC47 commented Feb 6, 2016

Maybe something's broken on my end, I can't even boot Pokemon Puzzle League.

if (!GlobalTimerIsSane)
{
// We are doing a mid-block update, so we need to adjust for the inaccuracy in globalTimer
int downcount = DowncountToCycles(PowerPC::ppcState.downcount);

This comment was marked as off-topic.

@JMC47
Copy link
Contributor

JMC47 commented Feb 8, 2016

Fire Emblem: Radiant Dawn and Yu-gi-oh False Bound Kingdom both test positive based on the debug code for the same thing afflicting the N64 titles like Ocarina of Time. It's very possible the long term random hangs that can happen are fixed as I've not been able to reproduce them. Then again, I don't really like Fire Emblem Radiant Dawn and haven't played it much seriously. It's a hard game.

N64 games are now more or less perfect. The only exception is Majora's Mask, which has a saving issue.

Rogue Squadron 2 SEEMS more stable in a test I did on Dualcore, but, it still crashes when mashing sometimes. Seems to be less than master, at least; can play multiple missions and not have it happen. phire and degasus seem to concur that the actual bug is unrelated to this.

@JMC47
Copy link
Contributor

JMC47 commented Feb 8, 2016

Does not Fix Fire Emblem: Radiant Dawn (PAL)

Fixes REGRESSION Ed, Edd, Eddy (not booting in LLE/HLE in various configurations,) https://bugs.dolphin-emu.org/issues/7639
Fixes Overlord: Dark Legend Map Crash https://bugs.dolphin-emu.org/issues/3953
Fixes N64 Timing Issues -> https://bugs.dolphin-emu.org/issues/8982

@delroth delroth added this to the Dolphin Release 5.0 milestone Feb 8, 2016
@delroth
Copy link
Member

delroth commented Feb 8, 2016

Review status: 0 of 6 files reviewed at latest revision, 14 unresolved discussions.


Source/Core/Common/SysConf.h, line 48 [r3] (raw file):
std::vector? That way the length can go away too.


Source/Core/Common/SysConf.h, line 50 [r3] (raw file):
Initialize these members near their declaration ("u8 nameLength = 0;").


Source/Core/Common/SysConf.h, line 51 [r3] (raw file):
std::array or std::string to kill this?


Source/Core/Core/CoreTiming.cpp, line 216 [r3] (raw file):
dbg_assert?


Source/Core/Core/HW/SystemTimers.cpp, line 162 [r3] (raw file):
Spacing is wrong here.


Source/Core/Core/PowerPC/Jit64/Jit_SystemRegisters.cpp, line 277 [r3] (raw file):
Is this just for the rapid check counter or is the code actually broken right now?


Comments from the review on Reviewable.io

@phire
Copy link
Member Author

phire commented Feb 8, 2016

Review status: 0 of 6 files reviewed at latest revision, 14 unresolved discussions.


Source/Core/Common/SysConf.h, line 48 [r3] (raw file):
I think I'll move this stuff to a new PR, it definitely should go in 5.0 while the remaining stuff is still a "maybe"


Source/Core/Core/CoreTiming.cpp, line 216 [r3] (raw file):
Well, video dumping does actually call this from the GPU thread when in dual core mode.


Source/Core/Core/PowerPC/Jit64/Jit_SystemRegisters.cpp, line 277 [r3] (raw file):
Just for rapid check counter.


Comments from the review on Reviewable.io

@BhaaLseN
Copy link
Member

BhaaLseN commented Feb 8, 2016

Reviewed 1 of 4 files at r1, 2 of 2 files at r2, 4 of 4 files at r3.
Review status: all files reviewed at latest revision, 17 unresolved discussions.


Source/Core/Common/SysConf.cpp, line 170 [r3] (raw file):
Whats with the extra space between make_unique and the square bracket? :)


Source/Core/Core/CoreTiming.cpp, line 327 [r3] (raw file):
Double brackets again...


Source/Core/Core/HW/SystemTimers.cpp, line 263 [r3] (raw file):
Is there any benefit in keeping this around as ifdef'd debug code, other than giving JMC something to work with?


Comments from the review on Reviewable.io

@JMC47
Copy link
Contributor

JMC47 commented Feb 9, 2016

Fixes FMV timings in Metroid Prime 1, Metroid Prime 2, and Metroid Prime 3.

https://bugs.dolphin-emu.org/issues/9321
https://bugs.dolphin-emu.org/issues/9326

@JMC47
Copy link
Contributor

JMC47 commented Feb 10, 2016

Fixes issue https://bugs.dolphin-emu.org/issues/6938 as far as I can tell.

@JMC47
Copy link
Contributor

JMC47 commented Feb 10, 2016

Found a regression: multi-game demo discs heavily lag when attempting when doing anything. The issue is very strange; as long as you're holding a button it lags down to 0 - 1 FPS (60 VPS) O.o.

Tested 11/12 D86E01, D85E01

in about 10 seconds of play, timebase registers were checked 251363187 times, 83773753 were rapid.

@JMC47
Copy link
Contributor

JMC47 commented Feb 10, 2016

Affects but does not fix issue 7192 -> https://bugs.dolphin-emu.org/issues/7192

F-Zero GX video gets further. Still likely a dual-issue thing.

@BigheadSMZ
Copy link

Do some of the VC games require specific settings to work with this PR? I figured I'd test out a few, but a lot of VC games that are working in 4.0-8880 are now crashing Dolphin instantly with identical settings. Here's a few that I can't get working now for reference.

2020 Super Baseball (NeoGeo)
Alex Kidd (SMS)
Boogerman (Genesis)
Castlevania I/III (NES)
Castlevania Rondo of Blood (TG16)
Donkey Kong Country 1-3 (SNES)
Earthworm Jim (Genesis)
F-Zero (SNES)
Super Mario RPG (SNES)

@JMC47
Copy link
Contributor

JMC47 commented Feb 11, 2016

All of those games are working for me; in fact, I tested them myself.

I tried this Pull Request originally and apparently my NAND was corrupt, and this PR triggered that. Try running the games with Portable.txt and see if they still crash.

@phire
Copy link
Member Author

phire commented Feb 11, 2016

Yeah, might be another corrupt NAND. Could you upload the file: Documents/Dolphin Emulator/Wii/shared2/sys/SYSCONF somewhere.

@phire
Copy link
Member Author

phire commented Feb 11, 2016

Actually, I might have been wrong about it being SYSCONF that is corrupted. Could you back up the entire Documents/Dolphin Emulator/Wii/ folder before doing anything more, so you can send that to me if it turns out not to be SYSCONF

@BigheadSMZ
Copy link

Ahh that makes sense. I'm still using my personal NAND that I dumped from my Wii like 3 years ago. I still have a backup of it so I'll give that a shot. Although, it's not from a virgin Wii since I already had Homebrew Channel installed, a few emulators, and Wiiware games when I dumped it. Not sure if that will affect anything.

@JMC47
Copy link
Contributor

JMC47 commented Feb 11, 2016

I was using a dumped NAND as well.

@BigheadSMZ
Copy link

Fortunately I also had a second NAND backup from a friend's Wii on my hard drive that I forgot existed, so I got to test this twice. After a fresh extraction, the same games crash the emulator using either of the two dumps, so it looks like it's consistent with "real" NANDs.

@JosJuice
Copy link
Member

@BigheadSMZ Please remove the link to your NAND. Distributing a full NAND is illegal because it contains copyrighted software.

@phire
Copy link
Member Author

phire commented Mar 22, 2016

I tracked down the cause of the Nintendo demo disk slowdown. That have a feature that plays a random video if you don't press any buttons in 15 seconds. So every time you pressed (or held) a button, it would reset the deincrementer interrupt to 15 seconds.

It's worth noting that 15 * 486 million has overflowed the a 32 bit variable and has the 31st (sign) bit set. Which causes dolphin to schedule an event in the past (which it fires immediately). Previously, dolphin would delay this past event by upto 10,000 cycles, so the impact wasn't too huge but with my more accurate timing the event is now firing immediately and continues to be re-scheduled while the button is held down.

I'm assuming this also fixes the play_rec.dat bug, though I haven't tested yet.

phire added a commit to phire/dolphin that referenced this pull request Mar 23, 2016
This showed up as a warning while I was debugging dolphin-emu#3601
Turns out it was unrelated, but the cleaup is nice.
@JMC47
Copy link
Contributor

JMC47 commented Mar 23, 2016

Now working without needing various hacks to get past the sysconf issues.

As an added bonus, fixes an issue with the draw distance I never reported in Starfox Assault. Objects no longer flicker at the edge of the draw distance.

Previously GlobalTimer was only updated at the end of each slice
when CoreTiming::Advance() was called, so it could be upto 20,000
cycles off.

This was causing huge problems with games which made heavy use of
the time base register, such as OoT (virtual console) and Pokemon
puzzle.

I've also made it so event scheduling will be accurate to the jit
block level, instead of accurate to the slice.
Events scheduled more than 4.12 seconds in the future (2.96 seconds for
Wii games) would overflow the sign bit and get scheduled in the past
instead, causing them to fire instantly.
@phire
Copy link
Member Author

phire commented Mar 23, 2016

I've fixed the various style issues which were bought up, and created a inverted copy of lastOCfactor to minimize on divisions.

@degasus
Copy link
Member

degasus commented Mar 23, 2016

Reviewed 1 of 4 files at r6.
Review status: 1 of 4 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


Source/Core/Core/CoreTiming.cpp, line 216 [r3] (raw file):
And this is evil, but out of scope of this PR.


Source/Core/Core/CoreTiming.cpp, line 53 [r6] (raw file):
static


Comments from the review on Reviewable.io

@degasus
Copy link
Member

degasus commented Mar 23, 2016

Reviewed 2 of 4 files at r6.
Review status: 3 of 4 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


Comments from the review on Reviewable.io

@JosJuice
Copy link
Member

Review status: 3 of 4 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


Source/Core/Core/CoreTiming.cpp, line 53 [r6] (raw file):
Should this have an s_ prefix?


Source/Core/Core/CoreTiming.cpp, line 210 [r6] (raw file):
Space after if


Comments from the review on Reviewable.io

The inverse operation is more common, especially when games check the
timer rapidly. So we do the division once and store the inverted copy.
@phire
Copy link
Member Author

phire commented Mar 23, 2016

Review status: 2 of 5 files reviewed at latest revision, 3 unresolved discussions.


Source/Core/Core/CoreTiming.cpp, line 53 [r6] (raw file):
I've done both.


Comments from the review on Reviewable.io

@degasus
Copy link
Member

degasus commented Mar 23, 2016

Reviewed 1 of 1 files at r7, 1 of 1 files at r8.
Review status: 4 of 5 files reviewed at latest revision, 2 unresolved discussions.


Comments from the review on Reviewable.io

@JMC47
Copy link
Contributor

JMC47 commented Mar 25, 2016

Please finish reviewing/merge this. Would be nice for the Progress Report, which is very light right now.

@degasus
Copy link
Member

degasus commented Mar 25, 2016

The review of this PR is done. Ready to merge.


Review status: 4 of 5 files reviewed at latest revision, 2 unresolved discussions.


Comments from the review on Reviewable.io

@degasus
Copy link
Member

degasus commented Mar 25, 2016

Reviewed 1 of 4 files at r6.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from the review on Reviewable.io

@delroth delroth merged commit 8f74f1f into dolphin-emu:master Mar 25, 2016
@Tinob
Copy link
Contributor

Tinob commented Mar 29, 2016

This pr breaks boot for Metroid other M, some user reported that they where having problems with this game and reverting this change fixed the issue.

@phire
Copy link
Member Author

phire commented Mar 30, 2016

Thanks Tinob, I'm debugging this.

@JMC47
Copy link
Contributor

JMC47 commented Mar 31, 2016

LLE audio or changing the emulated CPU clock rate affects this; meaning it's probably yet another timing bug in core.

@JMC47
Copy link
Contributor

JMC47 commented Mar 31, 2016

I discovered why LLE and doesn't reset and HLE does: LLE's hang happens on DSP initialization, HLE's hang happens on LYT (which, the next step is loading the savefiles.) Instead of loading the save files, after about 7 seconds of emulated time, it aborts and loads the sysconf and resets the game.

@phire
Copy link
Member Author

phire commented Apr 9, 2016

@Tinob, I've found the cause, it's fixed in #3774 (and #3773 patches another bug caused by this PR)

@Tinob
Copy link
Contributor

Tinob commented Apr 9, 2016

@phire Thanks for letting me know, Excelent debuguing :)

@phire phire deleted the AccurateEventScheduling branch April 9, 2016 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet