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
Core/DSPHLE: Deglobalize HLEAccelerator state in AX and AXWii UCodes. #12487
Conversation
AcceleratorSetup(&pb); | ||
HLEAccelerator accelerator; | ||
AcceleratorSetup(&accelerator, &pb); |
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 don't like this change. The accelerator is hardware functionality in the DSP where reading certain registers will return values based on the contents of ARAM (with optional ADPCM decoding and buffering to hide latency). Constructing HLEAccelerator
each time GetInputSamples
feels incorrect; I'd prefer to have it be a member of the ucode itself (like was done in #12486), even if in practice both should behave the same.
Everything else seems fine enough.
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.
Understandable, though if it's hardware functionality then I question the way it's implemented slightly differently via inheritance in different UCodes in the first place.
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 exception part is the main thing that needs separate implementations (since the exception handler is part of instruction ram and varies by ucode, so of course HLE for different ucodes will need different exception handlers). Inheritance is a bit awkward for modeling that, but I can't think of a cleaner approach. (Note that there are actually several exceptions that can be raised, though only one ever seems to matter in practice; see PR #10766.)
ReadMemory
and WriteMemory
don't really need to be implemented that way, but on LLE they use Host::ReadHostMemory
/Host::WriteHostMemory
... which normally just uses Core::System::GetInstance().GetDSP().ReadARAM
/Core::System::GetInstance().GetDSP().WriteARAM
(same as HLE). There's also the version in DSPTool
that stubs them out entirely, though... in fact it stubs out all of the DSPHost
functions. I think this was originally intended to be an abstraction to allow the DSP code to be re-used in other contexts. (I'm not sure if this was for an additional DSP executable for stand-alone emulation, or for use in other emulators, or what exactly.) It's not super relevant now though.
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.
Well I changed it... not gonna lie, from a software design perspective I think this is worse, but ymmv.
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, it's not great currently... but I think this is still a step in the right direction. At some point in the future it would be better to move the accelerator out of the ucode and into some longer-lived part of the DSP emulation (since it probably should remain alive even over ucode changes), and making it live as long as the ucode helps with that a bit. But it is a bit janky for now.
9e0ac67
to
cacf074
Compare
cacf074
to
3b0444b
Compare
Genuinely don't understand why this was global state before. The entire state of the accelerator is reset on every call to GetInputSamples(), and that's the only function that uses it.