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

Various DI improvements #8394

Open
wants to merge 26 commits into
base: master
from

Conversation

@Pokechu22
Copy link
Contributor

Pokechu22 commented Oct 8, 2019

An early draft of my improved DI work. I still need to do some testing (and write automated hardware tests), but there is documentation of what I've figured out so far.

The GPIO change fixes bug 11818 (not DI related) and also implements GPIO-based ejection (which is used by the system menu when booting from the eject button in conjunction with the RTC flag; I don't think any games use it though.)

The RTC change should fix bug 8115 (including the case when a channel other than the system menu is running which I reported as bug 11803). Note that the trade-off is that currently, it always acts like the disc has changed and never uses the cache when first loading the system menu, though it should still use it when exiting from a game. To be more accurate about it dolphin would need to track the current disc in the configuration file, probably.

Most of the other changes are related to errors reported by the disc drive, or various commands that were not implemented before. I've made some attempts to organize these nicely into different commits, but it's still not the clearest yet.

I have only tested this with Wii games. Testing it with gamecube games would be appreciated (in particular, I suspect that the stricter DTK changes will cause issues when starting from emulated BS2, but I haven't tested it).

Although this is a draft, I would like feedback on the implementation, since some of the code is rather hacky and I'm not completely sure how to do it better.

@JMC47

This comment has been minimized.

Copy link
Contributor

JMC47 commented Oct 8, 2019

@Pokechu22 Pokechu22 force-pushed the Pokechu22:misc-di-gpio branch from 43d52c4 to fd595ce Oct 8, 2019
Copy link
Contributor

JosJuice left a comment

Haven't read this code in detail yet, but I appreciate that you're improving these things!

// Older IOS versions only accept the first range. Later versions added the extra ranges to
// check how the drive responds to out of bounds reads (an error 001 check).
Comment on lines +273 to +277

This comment has been minimized.

Copy link
@JosJuice

JosJuice Oct 8, 2019

Contributor

Do you know which versions? You could add this to GetFeatures() in IOS/VersionInfo.cpp if you want Dolphin to handle this the way a given version of IOS would.

This comment has been minimized.

Copy link
@Pokechu22

Pokechu22 Oct 8, 2019

Author Contributor

I haven't fully tested all versions (I focused on the version dated Jun 3 2009 07:49:09 and included in IOS58 and IOS80), but here's what I wrote on WiiBrew:

Versions of IOS prior to IOS30 only permitted reads in the first range[check]. Nintendo titles check on startup if they are running an IOS ≥ 30 (and ≤ 253) and if so, perform some DI checks; specifically, they attempt to read 0x20 bytes from 0x460a0000 (or from 0x7ed40000 if the byte at 0x8000319c is 0x81 — possibly related to dual-layer discs?). If this read attempt returns anything other than 2, the game will refuse to start with the message "An error has occurred. Press the Eject Button, remove the Game Disc, and turn off the power to the console. Please read the Wii Operations Manual for further instructions." If the drive error is anything other than 0x0052100 (OK/Logical block address out of range), the game will refuse to start with the message "Error #1, unauthorized device has been detected."

}
break;
// Probably only used by Wii
case READ_DVD:
ERROR_LOG(DVDINTERFACE, "DVDLowReadDvd");
break;
// Probably only used by Wii

This comment has been minimized.

Copy link
@JosJuice

JosJuice Oct 8, 2019

Contributor

You can remove these comments about GC vs Wii and direct access vs IOS access. I added them as a compromise for the code not showing which ones only are usable via direct access or IOS on a real console, but now the code does show that. (See PR #1634 for reference.)

This comment has been minimized.

Copy link
@Pokechu22

Pokechu22 Oct 8, 2019

Author Contributor

The one case where the comments still apply is commands that are exclusive to the Wii disc drive; currently Wii commands are always accessible. But that's probably still fine; I don't think that on the Wii it prohibits gamecube games from using Wii games (though there is some reference to "legacy DI mode" with 0x8000315D; I don't know what the deal is with that). (Gc-forever has a list of commands that were added by the Wii, sourced from another wiki that is now dead and only partially archived).

Source/Core/Core/HW/DVD/DVDInterface.cpp Show resolved Hide resolved
Source/Core/Core/HW/DVD/DVDInterface.cpp Outdated Show resolved Hide resolved
request.dvd_offset, request.dvd_offset + request.length);
}

DVDInterface::SetHighError(DVDInterface::ERROR_BLOCK_OOB);

This comment has been minimized.

Copy link
@JosJuice

JosJuice Oct 8, 2019

Contributor

That we have reached this place in the code doesn't necessarily mean that we attempted to read out of bounds. We can also reach this under conditions such as having the game stored on a flash drive and yanking the flash drive out in the middle of gameplay. If you want to check whether we read out of bounds, you probably want to make use of this:

virtual u64 GetSize() const = 0;

But keep in mind that it usually returns a too large value for WBFS and CISO as a result of those formats not containing size information. (The current approach has the same problem, though. Reads won't fail due to out of bounds when reading within the bounds of what GetSize() returns.)

This comment has been minimized.

Copy link
@Pokechu22

Pokechu22 Oct 28, 2019

Author Contributor

I guess I probably should reply to this: I don't plan on addressing this at the moment. The main benefit of the change is that it returns an error to the game when an actual error occurs; making it match the actual error would be ideal but would require more complex changes to actually identify what went wrong.

I'll probably look into it for a separate pull request (probably along with completely async reads where it can wait longer than normal as an option).

Source/Core/Core/HW/DVD/DVDInterface.cpp Outdated Show resolved Hide resolved
Source/Core/Core/HW/DVD/DVDInterface.cpp Outdated Show resolved Hide resolved
Source/Core/Core/HW/DVD/DVDInterface.cpp Show resolved Hide resolved
UDICFG(u32 _hex) { Hex = _hex; }
};

extern UDISR s_DISR;

This comment has been minimized.

Copy link
@lioncash

lioncash Oct 8, 2019

Member

These should probably not all be exposed, but rather have an interface built around them.

This comment has been minimized.

Copy link
@Pokechu22

Pokechu22 Oct 23, 2019

Author Contributor

I'm not sure what that would look like. The registers are the way /dev/di is linked with DVDInterface itself in IOS — there isn't a level between them. I think these structs are close to a proper interface, though some of them are possibly too much (the UDICMDBUF and UDIIMMBUF ones in particular, which has never been used beyond its hex value). Possibly UDIMAR and UDILENGTH as well — I'm not sure whether the alignment requirements are actually enforced on them or what happens if you try to write an unaligned value there.

If anything, I think some of these should be dropped down to simply u32s.

ExecuteCommand taking the 3 DICMDBUF values as parameters instead of using the registers themselves might actually be too much of an abstraction, since it means IOS's writes to the registers aren't visible to games (and since some commands write only one of the registers instead of all 3, it can't simply be changed to write them out from the params). I don't think that's too important since the registers aren't visible without ahbprot (and there isn't an ioctl to get the commands, only e.g. the length), but if that's something we want to implement, then it'll need to be changed.

This comment has been minimized.

Copy link
@JosJuice

JosJuice Oct 23, 2019

Contributor

We already have these registers set up for MMIO (so that GameCube games can use them). Would it be cleaner to make IOS write to the emulated console's memory (triggering the MMIO handlers) instead of it doing what it does now? That way, it's even closer to the original hardware, and we don't have to make as many changes in what we expose.

This comment has been minimized.

Copy link
@Pokechu22

Pokechu22 Oct 23, 2019

Author Contributor

That would be ideal, actually. Does that work? I assumed Memory::Write_U32 would only write to the main game memory and bypass MMIOs.

This comment has been minimized.

Copy link
@JosJuice

JosJuice Oct 23, 2019

Contributor

I believe Memory::Write_U32 works for that, yes.

This comment has been minimized.

Copy link
@Pokechu22

Pokechu22 Oct 25, 2019

Author Contributor

I've started implementing something like that and have changed to writing the DICMDBUF values at least. Memory::Write_U32 didn't work, but Memory::mmio_mapping->Write does. I'm still trying to figure out how to handle the other values; reading a value and writing it back is a bit awkward (especially given the r/z nature of some of the registers — IOS can do DISR = DISR & ~(DEINT|TCINT|BRKINT) | DEINTMASK but it'd be a lot uglier to do the same with the current struct). Maybe a hybrid approach is easiest, where the bitfields do actually have dedicated functions in DVDInterface instead...

This comment has been minimized.

Copy link
@Pokechu22

Pokechu22 Oct 27, 2019

Author Contributor

I've implemented it; the only things that needed special access were the interrupts (which already had a bit of abstraction via DIInterruptType); everything else can just be treated as a u32 since /dev/di only passes the value on to the emulated title.

I also did some tests and found that the alignment on DIMAR and DILENGTH is enforced; bits 0-4 are always zero regardless of what's written (even if no command is triggered). However, my testing found that bits 26-31 were kept for both, while dolphin previously was clearing them for DIMAR.

Source/Core/Core/HW/WII_IPC.h Outdated Show resolved Hide resolved
Source/Core/Core/IOS/DI/DI.cpp Show resolved Hide resolved
Source/Core/Core/IOS/DI/DI.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/DI/DI.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/DI/DI.h Outdated Show resolved Hide resolved
Source/Core/Core/IOS/DI/DI.h Outdated Show resolved Hide resolved
Source/Core/Core/HW/DVD/DVDInterface.cpp Outdated Show resolved Hide resolved
Source/Core/Core/Boot/Boot.cpp Outdated Show resolved Hide resolved
Source/Core/Core/HW/DVD/DVDThread.cpp Show resolved Hide resolved
@Pokechu22 Pokechu22 force-pushed the Pokechu22:misc-di-gpio branch from fd595ce to 79be322 Oct 8, 2019
@Pokechu22 Pokechu22 force-pushed the Pokechu22:misc-di-gpio branch from 35d3ed5 to dd5aa9b Oct 9, 2019
Source/Core/Core/HW/WII_IPC.h Outdated Show resolved Hide resolved
Source/Core/Core/HW/WII_IPC.h Outdated Show resolved Hide resolved
Source/Core/Core/HW/WII_IPC.h Outdated Show resolved Hide resolved
Source/Core/Core/HW/WII_IPC.h Outdated Show resolved Hide resolved
Source/Core/Core/HW/WII_IPC.h Outdated Show resolved Hide resolved
@Pokechu22 Pokechu22 force-pushed the Pokechu22:misc-di-gpio branch 4 times, most recently from b05ab1f to 459552a Oct 10, 2019
Source/Core/Core/HW/WII_IPC.h Outdated Show resolved Hide resolved
@Pokechu22 Pokechu22 force-pushed the Pokechu22:misc-di-gpio branch 3 times, most recently from f0b7f3e to 0f8778f Oct 11, 2019
Copy link
Member

BhaaLseN left a comment

Changes seem solid for the most part; thanks again for working on this!
The only question is what you intend to do with those TODO comments left thoughout. Will you address them here, or leave them for later PRs?

Source/Core/Core/HW/DVD/DVDInterface.cpp Outdated Show resolved Hide resolved
Source/Core/Core/HW/DVD/DVDInterface.cpp Show resolved Hide resolved
@Pokechu22

This comment has been minimized.

Copy link
Contributor Author

Pokechu22 commented Oct 18, 2019

I plan on addressing most of the TODO comments, but not all of them. I probably will not address the DTK buffer size comment in this PR (since I don't think it affects games), and I don't plan on addressing the BCA TODO either (I want to handle that in a different PR). I also don't plan on handling the GPIO AVE and SLOT_LED comment, since those are pretty far out of scope (I don't know how I'd indicate the slot LED, and I don't know anything about AVE at all).

I might handle the extra read in DVDLowReadDiskID (I've ordered a gamecube game that uses DTK, so I can do some testing regarding it). I might handle the DVDLowGetCoverStatus resetting case, but that requires some hardware testing to understand what's going on with DICVR during the reset (I've done some basic testing, and it looks weird) and I don't think it affects games. DVDLowSeek is a weird case since dolphin doesn't implement it currently, but I do plan on implementing it eventually (which will also necessitate doing something about the partition). The two disc state TODOs are basically just cases where I need to do more testing and I plan on resolving them at some point.

The comment in PerformDecryptingRead is copied from ExecuteReadCommand. That will require timing testing which I haven't done yet; I might or might not get to it.

The rest are code things that I plan on resolving before merge (or possibly have already resolved and haven't removed the comments on).

@Pokechu22 Pokechu22 force-pushed the Pokechu22:misc-di-gpio branch from 0f8778f to 11a8e9d Oct 18, 2019
@Pokechu22

This comment has been minimized.

Copy link
Contributor Author

Pokechu22 commented Oct 19, 2019

The gamecube games I ordered arrived, and I can now see that the DTK changes I have here are a bit too restrictive — DTK isn't configured when launching with emulated BS2, and also changing discs while on the IPL rejects DTK configuration for the second disc which causes issues even if neither game uses DTK (however, everything works fine if you start the IPL and then insert a disc or boot with "skip main menu" disabled).

I know how to fix the BS2 case (just add that logic into Boot_BS2Emu, which probably will involve messily duplicating more code for the time being, which is not great but at least a solution). However, I'm less sure how to solve the IPL one — I'm not sure when the drive gets back into a state that audio can be configured. I'm currently enabling DTK configuration when the drive is reset, but I don't think that's something that happens normally on a gamecube. Maybe it's instead configurable after changing discs, and that's something that's enforced in the drive itself without software needing to do a reset?

EDIT: After thinking about it a bit more, it seems pretty likely to me that it is just changing discs and not the reset. A reset works for allowing configuring it because it puts the drive into ERROR_NO_DISKID_L (0x03000000) or ERROR_CHANGE_DISK (0x02000000) (I'm not sure which at this time, and I think it's probably also complicated with Gamecube vs Wii), but changing the disc directly would also presumably do that, and you'd need to be able to configure it at that point anyways. My assumption was that it wouldn't be possible for a game to configure the buffer size and that the one the IPL set would remain in use until the console is restarted due to the possibility of removing the disc while the game is running - but if the game can configure the buffer again after it detects the disc being removed at runtime, that'd also work. And I can test that!

EDIT: Well, my brilliant idea for testing it was to, in dolphin, eject the disc while DTK audio is playing. Unfortunately, that crashes dolphin, because DVDThread has no idea what to do about that (it doesn't seem to handle read errors with DTK that well). Testing on console is going to be hard too, because I don't have a gamecube controller to actually get to a point where audio is playing (but I can probably write a test program that just calls DTK functions). I can at least decompile the game, though (and fix the crash).

@Pokechu22 Pokechu22 force-pushed the Pokechu22:misc-di-gpio branch from 11a8e9d to 944dff3 Oct 20, 2019
@Pokechu22

This comment has been minimized.

Copy link
Contributor Author

Pokechu22 commented Oct 20, 2019

I've fixed both of the DTK not getting set issues, though I still haven't hardware tested it becoming configurable again. I haven't fixed the crash, but it looks like gamecube games just react poorly in general to being ejected. I'm not sure why that's happening as there should be an interrupt that's getting sent out, but I don't know enough about gamecube interrupts to diagnose it.

Source/Core/Core/Boot/Boot_BS2Emu.cpp Show resolved Hide resolved
Source/Core/Core/HW/DVD/DVDInterface.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/DI/DI.cpp Outdated Show resolved Hide resolved
@Pokechu22 Pokechu22 force-pushed the Pokechu22:misc-di-gpio branch 2 times, most recently from 7bc0119 to ac1cc49 Oct 21, 2019
@Pokechu22

This comment has been minimized.

Copy link
Contributor Author

Pokechu22 commented Oct 22, 2019

I got around to doing some more hardware testing on my Wii, and can now address the earlier TODOs about drive states:

  • On the Wii, a disc change will result in the drive not immediately spinning until a reset with spinup. Doing most commands in this time will result in ERROR_CHANGE_DISK / ERROR_MEDIUM (0x02062800). Once it is spun up, ERROR_NO_DISKID_L / ERROR_NO_DISKID_H apply instead (0x05020401), until the ID is read. So I think my implementation of it is correct. No idea how it works on gamecube, though.
  • DTK can be configured again after resetting the drive (which must happen for it to be in the spun-up state; at least on the wii as far as I can tell there is no way to get it back to that state without a reset)
  • DTK can be configured multiple times until a read occurs (though I'm only configuring it with the same data in this test). Once a read occurs, ERROR_INV_PERIOD is returned (0x00052402).
    • Reading the disc ID a second time will remove the ability to configure DTK, same as any other read. I think the first read (when the drive is currently in ERROR_NO_DISKID_L) is special and doesn't count against DTK.
    • Starting audio playback also prohibits configuration of DTK.
    • Requesting the error and checking the audio status do not prohibit (re)configuration of DTK, though the audio status cannot be checked unless DTK is enabled (failing with ERROR_AUDIO_BUF (0x00052001).

Right now I'm not enforcing ERROR_NO_DISKID_L (I'll fix that), and there might be other commands (the Wii drive-exclusive ones in particular, but also seek) that can be used even when they're not enabled. I don't think it's too important as most of those commands aren't implemented fully anyways, but it's worth noting that that could be expanded upon later.

EDIT: The one thing I don't understand is that the Wii Menu seems to get stuck in an infinite loop if I set the error to ERROR_CHANGE_DISK / ERROR_MEDIUM and reject attempts to use DVDLowReadDiskID until the disc is spun up. So currently, I have these lines:

if ((s_error_code & LOW_ERROR_MASK) == ERROR_NO_DISKID_L ||
(s_error_code & LOW_ERROR_MASK) == ERROR_CHANGE_DISK)

but at least as far as I can tell, only the first one of those should really be there. And of course that doesn't explain at all how the disc is spun up on the Gamecube.

@Pokechu22 Pokechu22 force-pushed the Pokechu22:misc-di-gpio branch 2 times, most recently from 8430353 to 704c9c5 Oct 25, 2019
@Pokechu22 Pokechu22 changed the title [WIP] Various DI improvements Various DI improvements Oct 28, 2019
Source/Core/Core/HW/DVD/DVDInterface.h Outdated Show resolved Hide resolved
DEBUG7 = 0x800000,
};

class GPIOBit

This comment has been minimized.

Copy link
@leoetlino

leoetlino Nov 8, 2019

Member

Could this be made into a Common utility class? (part of BitUtils, Flags perhaps?)

This comment has been minimized.

Copy link
@Pokechu22

Pokechu22 Nov 9, 2019

Author Contributor

Done. (I feel like there should be a check that the enum actually has bits for its constants, but I don't know if that's a thing that can be checked or how I'd do it).

Source/Core/Core/IOS/DI/DI.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/DI/DI.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/DI/DI.cpp Outdated Show resolved Hide resolved
Source/Core/Core/Boot/Boot_BS2Emu.cpp Outdated Show resolved Hide resolved
Source/Core/Core/HW/DVD/DVDInterface.cpp Outdated Show resolved Hide resolved
Source/Core/Core/HW/DVD/DVDInterface.cpp Outdated Show resolved Hide resolved
Source/Core/Core/HW/DVD/DVDInterface.cpp Outdated Show resolved Hide resolved
Source/Core/Core/HW/DVD/DVDInterface.cpp Outdated Show resolved Hide resolved
@leoetlino

This comment has been minimized.

Copy link
Member

leoetlino commented Nov 8, 2019

Also needs a rebase ^^

Pokechu22 added 23 commits Aug 21, 2019
SLOT_LED and the AVE ones are not implemented yet, but the other Broadway ones are.
The various ioctls sometimes have different arguments than the DI command
registers, though they generally overlap.  There are also a bunch of ioctls
that don't even normally go into DVDInterface, just returning various data.
Some of the implemented ioctls are new to Dolphin.
All out of bounds reads should return the appropriate DI error, but it also
makes sense to have the error 001 read happen there.
Partitions are Wii-exclusive, and don't happen at the DVDInterface level in
IOS.  This isn't quite the cleanest fix, but it gets rid of the assumption that
a partition is open on starting the game at least.
Also, fix DVDLowStopMotor logging (which was based on the ioctl parameters)
Based on a hardware test on a Wii.  The alignment code was originally added in 7436419.
@Pokechu22 Pokechu22 force-pushed the Pokechu22:misc-di-gpio branch from 704c9c5 to 4db237f Nov 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
You can’t perform that action at this time.