-
Notifications
You must be signed in to change notification settings - Fork 130
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
Add iQue Player support #1731
base: master
Are you sure you want to change the base?
Add iQue Player support #1731
Conversation
…SI DMA, SA2 starts
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.
Thanks for this, this work is absolute great and mindblowing, it's incredible to finally have the iQue emulated!
I've read the PR once and obviously there's much to digest, so I mainly focused on code organization in this first round of review, plus small things.
I understand that having the iQue to be in-core with n64 is probably the simplest choice also going forward, but compared to dd which is well isolated, iQue changes are much more pervasive. So my first concern would be to make sure that:
- All variables in shared modules are properly prefix with
bb
,BB
,bb_
or whatever - Code changes to existing modules are well separated from n64 code, without compromising too much readability of the n64 codebase.
@@ -22,6 +22,8 @@ struct Platform { | |||
virtual auto audio(Node::Audio::Stream) -> void {} | |||
virtual auto input(Node::Input::Input) -> void {} | |||
virtual auto cheat(u32 addr) -> maybe<u32> { return nothing; } | |||
virtual auto showLED(b1 show) -> void {} | |||
virtual auto setLED(b1 on) -> void {} |
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.
@LukeUsher is this OK? I don't know how to review changes to Platform.
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.
Why do we need both show and set here? Shouldn’t the emulator core just call set and the ui handle it accordingly? A config option could be added in the gui to enable or disable LED display
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.
showLED
does the job of the config option, it determines whether the LED will be shown in the UI at all. I think it makes more sense for the guest core to inform the UI that it needs the LED to be shown rather than leaving it to a user option? At least, it seems to fit better with a philosophy of not having too many knobs for users to have to turn.
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.
Similarly to the power button, how to handle the activity LED is a pretty open question - it's not really vital information to show to the user, but it is definitely part of the hardware. The solution here wasn't really intended to be merged, I think, but rather just exists to get something to show the LED state.
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.
Some systems even have multiple software controlled status LEDs that we should show; I believe Sega CD has two,
I think I’ll create a method for cores to add widgets to the status bar in a similar way cores can add menu options for things like changing discs, then we can use that; the approach in the PR can be merged as a temporary implementation until this frame work exists
The long term plan is that the cores can append the required number of LED components and handle rendering them.
|
||
namespace ares::Nintendo64 { | ||
|
||
AES aes; |
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 believe the actual algorithm should go to nall
, as a way to document that this is a standard AES-128-CBC. It'd also help showing eg. exactly the state there is on iQue.
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 agree with this; nall already has a construct for cryptography, so assuming it is the standard algorithm it should go there
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 bound this implementation very closely with the N64 core resident structures, especially Memory::Writable
, mostly due to performance concerns. AES decryption happens on essentially any PI DMA so it seems important that the implementation is streamlined for keeping VPS high.
If you could give more specific direction on how to make the implementation more generic without going through some glue code I can adjust it accordingly.
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's a very good point, we'll keep it here then if it's specialised to the memory layout!
@@ -28,6 +28,9 @@ Gamepad::Gamepad(Node::Port parent) { | |||
r = node->append<Node::Input::Button>("R"); | |||
z = node->append<Node::Input::Button>("Z"); | |||
start = node->append<Node::Input::Button>("Start"); | |||
|
|||
if(system._BB() && (parent->name() == "Controller Port 1")) | |||
bb_button = node->append<Node::Input::Button>("Power"); | |||
} |
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 N64 core misses emulation of the Reset hardware button. I believe that should go into a menu item rather than being a controller button (also because on N64, that's not on the controller). On iQue it's more blurry because the controller is the console, so maybe this implementation is 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.
The “reset” option in the GUI will call system.power() with reset = true signify reset instead of hard power cycle; this might be a good place to handle this for both n64 and dd since it works with the ares (soft)reset already.
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.
This is definitely the part of the PR that needs the most work - it isn't really clear how this should be handled here, since it's so different to other platforms.
I can only think of one other system where the reset button is required for gameplay (that one Megadrive game that required it to progress), but in the iQue Player's case it's also required to save.
I did consider connecting it to system.power(), but that then raises the question of how to handle powering the console off, since from a hardware perspective it's the same button; the console is powered off from software via the INT2 handler before the NMI has a chance to occur. That in itself is another topic that needs some discussion.
The approach I went for in this draft PR is really awful and slows down the emulation quite a lot (since it's polling the controller every 32 clock cycles) and still isn't accurate to hardware; it's definitely a blocker on merging.
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.
ares doesn't currently support powering a console off really; it is something that should be addressed eventually but as it stands, the only feature we expose is the initial power on (power:reset = false) and reset (power: reset = true)
So I'm not sure how to best handle this here...
Implementing power:reset=trueshould be done anyway as it allows the 'reset' menu item to work correctly in ares, though!
if(system._BB() && (name == "Controller Port 1")) | ||
port->setSupported({"Gamepad"}); | ||
else | ||
port->setSupported({"Gamepad", "Mouse"}); |
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 believe Mouse in not support on iQue at all? Why allowing it for other ports?
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.
This is related to the button issue discussed above - the approach used in this draft is a quick hack to get the button working at all so I could test it, and many aspects of it need redoing.
case Queue::VIRAGE2_Command: return virage2.commandFinished(); | ||
case Queue::NAND_Command: return pi.nandCommandFinished(); | ||
case Queue::AES_Command: return pi.aesCommandFinished(); | ||
case Queue::BB_RTC_Tick: return pi.bb_rtc.tickClock(); |
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.
Can we prefix all of this enums with `BB_"?
@@ -98,7 +106,8 @@ auto CPU::instruction() -> void { | |||
return exception.interrupt(); | |||
} | |||
} | |||
if (scc.nmiPending) { | |||
if (scc.nmiPending || scc.nmiStrobe) { | |||
scc.nmiStrobe = 0; |
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.
Not sure why we need two different strobe lines here? I guess one should be sufficient?
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.
This is because the NMI behaviour described in the CPU manual doesn't match how ares currently implements it (and how the PIF emulation seems to expect it). Really, we need someone familiar with the CPU core to look into it and figure out exactly how it's supposed to work - this was our approach to get the hardware behaviour to work without disturbing the existing code.
@@ -677,7 +681,7 @@ struct CPU : Thread { | |||
|
|||
struct Coprocessor { | |||
static constexpr u8 revision = 0x00; | |||
static constexpr u8 implementation = 0x0a; | |||
static constexpr u8 implementation = 0x0b; |
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.
Are you changing this for N64 as well?
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 this should be 0x0B on N64 too, I tested on hardware and 0x0A doesn't seem to be correct for N64 either. Might be nice to get a second confirmation though
@@ -49,6 +49,7 @@ struct CPU : Thread { | |||
u64 nextpc = 0; //pc after next instruction | |||
u32 state = 0; //current branch state | |||
u32 nstate = 0; //next branch state | |||
n1 fetch = 0; //currently fetching an instruction? |
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.
We discussed this on Discord. The problem here is that on the SysAD bus there is no way to know if an access is a fetch or a read, so this bit smells a bit.
This bit seems requires as part of the secure mode gating logic. If the gating logic is correct that only triggers when the CPU is fetching from 0x1FC00000, then it means the VR4300 IP core in the iQue must have been patched to expose this additional "fetching" line.
Now I wouldn't mind much in general but I fear this is going to be a problem for the JIT. If possible, I'd rather this bit not being added.
So let's wait for further hardware tests before deciding what to do.
@@ -6,55 +6,138 @@ inline auto PI::readWord(u32 address, Thread& thread) -> u32 { | |||
thread.step(writeForceFinish() * 2); | |||
return io.busLatch; | |||
} | |||
thread.step(250 * 2); | |||
io.busLatch = busRead<Word>(address); | |||
if (system._BB()) { |
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.
Also discussed on Discord: the PI changes should go into a separate directory/module because they are very pervasive within these files.
For reference: the rationale behind this is that the Disk Drive is definitely an addon - a separate bit of hardware - and you interface with it more like you do a game cartridge than the RCP, whereas the iQue Player changes the core hardware itself. I'm open to splitting all of the iQue Player changes into a separate module, though I don't think it makes strictly more sense. |
This PR adds preliminary support for emulating the iQue Player, the revised version of the Nintendo 64 released in Mainland China in late 2003. It modifies the N64 core to add support for the console's unique hardware.
The iQue Player doesn't launch games from cartridges; instead, it has a slot on the back for a card to be inserted, which contains a NAND chip. On the NAND is the console's kernel, system menu and all software.
Games on the card are encrypted and signed, and must be launched from the system menu. Metadata about the games is stored in a file on the card. As such, it doesn't really make sense to support the common emulator feature of dragging and dropping a game onto the window to launch it.
When launching the core for the first time, it will prompt for the necessary files dumped from a console: a dump of the NAND with corresponding spare data, and the keystore. It isn't really feasible for the emulator to auto-generate these, since the keystore contains various global and console-unique key data; external tools can do this.
Equally, it doesn't make much sense to provide a virtual filesystem for the card contents; instead of having a global operating system with filesystem capabilities that games can use, the iQue Player variant of Libultra interacts with the NAND directly, so HLEing the filesystem is mostly infeasible.
The iQue Player has a soft power button, which has been partially implemented. Pressing it while in a game resets the console to the system menu, and pressing it again from the system menu powers off the console. Currently, powering off the console does nothing; it's unclear how to handle this.
Additionally, I've temporarily added a button mapping to the first controller port for the power button, but this solution isn't very good and I've had to add some awful code to handle it. An ideal solution would be to have an option in the GUI for the power button, but that then doesn't allow mapping it to a controller for convenience.
There are still some things that haven't been implemented:
pixel_advance
needs to be set differently to avoid corrupted display - we haven't attempted to emulate this. If we're lucky, might this fall out of accurate general VI emulation when considering the video clock? cc @raskySerialisation is currently broken. This is because we didn't always keep track of which changes required adjusting the serialisation, so not everything we've added gets serialised properly. This should be fairly simple to fix, if someone looks over the changes and compares them to what's being serialised.
We also need to remember to update the serialisation version before merging.