Preliminary HSP support #8067
Preliminary HSP support #8067
Conversation
CMakeLists.txt
Outdated
| if (_M_X86) | ||
| add_subdirectory(Externals/Bochs_disasm) | ||
| endif() | ||
| add_subdirectory(Externals/cpp-optparse) | ||
| add_subdirectory(Externals/glslang) | ||
| add_subdirectory(Externals/imgui) | ||
|
|
||
| set(DISABLE_FRONTENDS ON CACHE BOOL "" FORCE) |
There was a problem hiding this comment.
You may want to consider making these qualified upstream in mGBA then modifying them here, (like MGBA_DISASBLE_DEPS), etc. These variables can now potentially leak into other dependencies. The build is essentially more fragile now.
There was a problem hiding this comment.
Should have added this during the initial review, but, this can be a lot to ask for a separate project. I mean, going, "lol just change ur build option flag names", isn't realistic in some scenarios, especially if the emulator is already being used as a lib elsewhere, since that'd be a minor forward-breaking change. Because of that, I definitely won't block on this or anything if that's too much of a chaotic change (hopefully it wasn't implied that I would on the initial review message
Pinging the CMake wiz @Orphis. Is there a nicer way to avoid this that doesn't involve upstream changes, or is that the only way in this scenario?
There was a problem hiding this comment.
It depends on the extent of functionality that this option adds.
Possibly the best option would be to just use add_subdirectory(Externals/mgba EXCLUDE_FROM_ALL) without any special option. The final solution will have all the targets, but they won't be built unless they're actually used.
CMakeLists.txt
Outdated
| if (_M_X86) | ||
| add_subdirectory(Externals/Bochs_disasm) | ||
| endif() | ||
| add_subdirectory(Externals/cpp-optparse) | ||
| add_subdirectory(Externals/glslang) | ||
| add_subdirectory(Externals/imgui) | ||
|
|
||
| set(DISABLE_FRONTENDS ON CACHE BOOL "" FORCE) |
There was a problem hiding this comment.
It depends on the extent of functionality that this option adds.
Possibly the best option would be to just use add_subdirectory(Externals/mgba EXCLUDE_FROM_ALL) without any special option. The final solution will have all the targets, but they won't be built unless they're actually used.
|
Will be fixing stuff upstream as needed, don't worry. I'm at lunch right now but I have a good chunk of this stuff resolved already. Will finish when I get home. |
|
Might splitting out the base HSP changes from the GBP changes into separate PRs make sense? That was we can get the easy stuff merged first, not that it's actually useful without the GBP changes. |
endrift
force-pushed
the
hsp
branch
6 times, most recently
from
04afdef
to
08bd121
Compare
endrift
force-pushed
the
hsp
branch
4 times, most recently
from
2df8c30
to
6164a5e
Compare
| u64 CHSPDevice_ARAMExpansion::Read(u32 address) | ||
| { | ||
| u64 value; | ||
| std::memcpy(&value, &m_ptr[address & m_mask], sizeof(value)); | ||
| return Common::swap64(value); | ||
| } | ||
|
|
||
| void CHSPDevice_ARAMExpansion::Write(u32 address, u64 value) | ||
| { | ||
| value = Common::swap64(value); | ||
| std::memcpy(&value, &m_ptr[address & m_mask], sizeof(value)); | ||
| } |
There was a problem hiding this comment.
Is the swapping here needed? Note that Memory::Read_U64/Write_U64 already performs a swap, and I see that similar swapping happens in DSP.cpp when changing s_ARAM. If it shouldn't be swapped at all then maybe Memory::Write_U64_Swap (and a currently-non-existent Read_U64_Swap) should be used instead. I guess this depends on how the gameboy player uses these values.
|
What happens if a DMA starts in-bounds for ARAM, but then goes out of bounds? Currently I think that would result in writing past the end of |
|
Once the build issues + review comments are handled, I assume this is ready to merge? |
|
I've fixed everything except the ARAM loop and swapping. Unsure how important those are at the moment but I can address them soon if desired. |
There was a problem hiding this comment.
The swapping isn't too important - it's something that could be addressed in a later PR if it makes things easier there. The loop issue (#8067 (comment)) is also something that could be addressed in a later PR.
Pokechu22
dismissed
jordan-woyak’s stale review
Requested change has been resolved; there are other pending requested changes but they don't block merging
|
@dolphin-emu-bot rebuild |
|
I don't want this to bitrot again, and it's got an approval. Good enough for me. |
Repurposed for just HSP support for now, since merging that will be easier and I can get to GB Player when this interface is merged.