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
DSPHLE: Move CDD1 constants to UCodes.h #10753
DSPHLE: Move CDD1 constants to UCodes.h #10753
Conversation
| MAIL_NEW_UCODE = 0xCDD10001, | ||
| MAIL_RESET = 0xCDD10002, | ||
| MAIL_CONTINUE = 0xCDD10003, | ||
|
|
||
| // CPU sends 0xBABE0000 | cmdlist_size to the DSP | ||
| MAIL_CMDLIST = 0xBABE0000, |
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.
You can leave this as is but this is only used in one place. Seems strange to me to have it in an enum called MailType all by itself. I wonder if we could kill this enum with a function or something:
bool IsCmdList(u32 val)
{
const u32 mask = 0xFFFF0000;
const u32 expected_val = 0xBABE0000;
return (val & mask) == expected_val;
}
or maybe there is something more appropriate?
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.
The way it's actually used is like this:
else if ((mail & MAIL_CMDLIST_MASK) == MAIL_CMDLIST)
{
// A command list address is going to be sent next.
set_next_is_cmdlist = true;
cmdlist_size = (u16)(mail & ~MAIL_CMDLIST_MASK);
}Probably the best choice is to just use constexpr u32 instead of enum (and it could also be put in the cpp file instead of the header, but I have less of an opinion on that front).
93cea1e
to
fab5e14
Compare
| @@ -206,31 +206,31 @@ void ZeldaUCode::HandleMailDefault(u32 mail) | |||
| PanicAlertFmt("Rendering end mail without prefix CDD1: {:08x}", mail); | |||
| } | |||
|
|
|||
| switch (mail & 0xFFFF) | |||
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.
Technically a behavior change here: If a mail that triggers the above panic alert comes in, it could previously trigger a case in this switch, and now it can't. I assume that's fine?
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.
That wasn't an intentional change; I'll try to verify which is correct now.
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.
The old behavior was correct; the zelda uCode doesn't check that the prefix is CDD1 (but it usually is in practice).
56D36052(Super Mario Sunshine) does this atinst:05db(with a jump table atinst:05d7). The byte sequence CDD1 doesn't appear in it at all (though DCD1 does appear).24B22038(NTSC GameCube IPL) does not check it at all; neither CDD1 nor DCD1 appear. The comment onSUPPORTS_GBA_CRYPTOabout "This was used before the GBA UCode and UCode switching was available." makes this seem a bit reasonable; the CDD1 and DCD1 mails are (I think) part of the powerpc uCode switching logic (which is also why they're standard between different uCode). Note that the IPL doesn't support GBA uCode, but it does (seem to) include a copy of the CARD uCode in it, which Dolphin doesn't correctly handle right now (but we have a workaround that means the CARD uCode is never needed). This version also uses the "light" protocol, while the CDD1 handling is in the default "protocol" only, so we're handling this correctly.D643001F(Super Mario Galaxy 2) does this at0734(with the jump table at0730). CDD1 doesn't appear in it at all (though DCD1 does appear).
For what it's worth, the AX uCode is the same:
-
07F88145(not sure which game I used, but it's apparently a common one) has it at0f34and the jump table at0f30; CDD1 doesn't appear in it at all (though DCD1 does appear). I'm not 100% sure how this is reached, but it does send0xdcd10002(DSP_YIELD) beforehand. I think that this is one of the few places where they process mail, though, so it's not like there's a separate state where it won't accept the mail.(I also see some other outgoing mail, but it all seems to be in special cases:
0xfbad8080at0079as the instruction after a JMPR (should be unreachable),0xbaadXXXXat007eifXXXX, the command to use, is out of bounds (negative (which is impossible based on how it's loaded) or greater than 0x13),0xecc0XXXXat0eb3if a stack overflow (exception 1) or reset (exception 0) occurred whereXXXXis the value of$st0(i.e. the PC before the exception occured), and0xfeedXXXXat0ec2if exception 2 (currently unknown) occurred (again whereXXXXis$st0). (That last one is different from the0x8071feedused by the ROM uCode). -
4CC52064(Wii Sports Resort) has it at0f99and the jump table at0f95. CDD1 doesn't appear in it at all (though DCD1 does appear).
So, the old behavior was more accurate here. (The CARD and GBA uCode, however, do check for the CDD1 prefix.)
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've fixed this and also added comments detailing how the various mails are handled.
I'll handle the AX version in #10768 because it turns out AX also has global state (but not enough global state to handle the masking accurately).
fab5e14
to
0c236fa
Compare
0c236fa
to
33d2815
Compare
| static constexpr u32 DSP_INIT = TASK_MAIL_TO_CPU | 0x0000; | ||
| // Triggers a callback. No response is sent. | ||
| static constexpr u32 DSP_RESUME = TASK_MAIL_TO_CPU | 0x0001; | ||
| // If the current task is canceled by the CPU, treated as DSP_DONE instead. Otherwise, this is |
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.
Did you mean treat it as DSP_DONE instead? Not sure how to parse this otherwise.
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.
[it is] treated [by the CPU] as [if it were] DSP_DONE instead is what I had in mind. I'll expand it.
33d2815
to
26fef1b
Compare
These are used by *all* uCodes, though not all uCodes support MAIL_RESUME or MAIL_CONTINUE.
26fef1b
to
f3c8e75
Compare
These are used by all uCodes, though not all uCodes support
MAIL_RESUMEorMAIL_CONTINUE.Extracted from my WIP card ucode HLE stuff, since this is also applicable to the homebrew asndlib/aesndlib ucode.