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
Fix DI interrupt mask ioctl names #8599
Fix DI interrupt mask ioctl names #8599
Conversation
0x85 is actually DVDLowMaskCoverInterrupt, while 0x89 is DVDLowUnmaskCoverInterrupt. I'm also fairly sure that 0x87 is DVDLowUnmaskStatusInterrupts.
eca173a
to
1dd2147
Compare
Source/Core/Core/Analytics.cpp
Outdated
| "uses-DVDLowRequestRetryNumber", | ||
| "uses-DVDLowSerMeasControl", | ||
| "uses-different-partition-command", | ||
| "uses-di-interrupt-command"}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest placing a trailing comma after the last element, so that clang-format doesn't do this wonky formatting.
Technically, given we're on C++17, which has deduction guides, the type specifier can likely also be simplified down to:
constexpr std::array GAME_QUIRKS_NAMES{
// ...
};There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, that's why it did it that. I remember that happening before, actually. I've also simplified the type specification like that (and I can confirm that the static_assert after will give a compile error if it's not updated, which wouldn't be the case with e.g. std::array<const char*, static_cast<size_t>(GameQuirk::COUNT)> since it doesn't care if the array is larger than what's actually specified)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, unfortunately those builders use a version of libc++ that's too old. :/
1dd2147
to
e3c14b7
Compare
e3c14b7
to
f527f38
Compare
This is a correction to some changes in #8394. In it, I added
DVDLowUnmaskCoverInterrupt = 0x85andDVDLowEnableCoverInterrupt = 0x89(marking 0x89 as an unconfirmed name). After more investigation, I've decided that 0x85 is actuallyDVDLowMaskCoverInterrupt, while 0x89 isDVDLowUnmaskCoverInterruptand the previously-unimplemented (as the actual implementation does nothing) 0x87 isDVDLowUnmaskStatusInterrupts. Note that all of those commands are, as far as I can tell, dummied out on the PPC side to always return 1. As such, I've also made use of those commands be reported as a game quirk.To explain how I made this mistake, first note that most of my research was based on symbols for the US version of Kirby's Dream Collection in
donut.MAP. (Kirby's Return to Dream Land has a similar file namediuk.MAP)This file starts of with a bunch of useless-looking information about duplicate code:
Actual symbols only begin on line 43317:
... and so I only looked at the info pertaining to
dvd_broadway.o, on lines 46010-46066.DVDLowMaskCoverInterruptandDVDLowUnmaskStatusInterruptsare nowhere to be seen in there. Since I had only seenDVDLowUnmaskCoverInterrupt, I had one name to use, and I decided that 0x85 probably was it since I assumed "unmask" meant "disable" and 0x85 cleared bit 1 of DICVR, which disables the interrupt. That assumption was incorrect, and inconsistent with the way that YAGCD described that bit: "CVRINTMASK - Cover Interrupt Mask. 0: masked, 1: enabled".It turns out those functions were actually listed them earlier on, on lines 77-79...
... and then again on lines 19992-19997.
The fact that those functions were replaced implies that they were used somewhere. But I didn't realize that until much later. I only noticed it when looking at a symbol list for Tony Hawk's Downhill Jam, more on that later.
The way I concluded that I had the names swapped was by looking at the use of those functions on another game with symbols: both
DVDLowUnmaskStatusInterruptsandDVDLowMaskCoverInterruptare referenced byDVDInit.Here's
DVDInitfrom launcher.elf in the PAL version of Red Steel (Wii), chosen because it's a launch title... and here's
DVDInitfrom Scooby Doo Mystery Mayhem (GC)I think these make it pretty obvious that the implementation of
DVDLowMaskCoverInterruptonce matchedDICVR = 0(which is the same asDICVR = DICVR & ~4 & ~2assuming bit 0 ignores writes), which is what ioctl 0x85 does. That means thatDVDLowMaskCoverInterrupt DVDLowUnmaskCoverInterruptprobably would have the same as ioctl 0x89, which doesDICVR = (DICVR & ~4) | 2.DVDLowUnmaskStatusInterruptsbeing 0x87 is a pure guess as the IOS implementation does nothing (always returning success), but it feels right.It's worth noting that
DVDLowClearCoverInterruptis called in a few places other thanDVDInit, even though it is dummied out to always return 1: it's used in a few error states, and by BS2.The reason why I've added a quirk is that evidence indicates that at some point, the functions were not dummied out on the PCC side. This is based on the previously mentioned symbols for Tony Hawk's Downhill Jam, though, annoyingly, the symbols are for a debug version of the game that does not actually exist on the disc, or at least the PAL version of it.
Here are the relevant symbols (reordered, as the file was originally sorted by size and they were scattered all over the place).
Note that the size of
DVDLowMaskCoverInterruptandDVDLowUnmaskStatusInterruptsis 20, same asDVDLowClearCoverInterrupt. I'm not completely sure this explains everything, since I do not see any references to IOS in the full symbol list (so perhaps at that point in development, Wii games directly interacted with DI?).