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

DSPLLE: Split SRS into SRS and SRSH and implement conditional variants of RTI #10038

Merged
merged 9 commits into from Aug 23, 2021

Conversation

Pokechu22
Copy link
Contributor

  • The SRS instruction works differently from the LRS instruction. LRS supports, in order, $ax0.l, $ax1.l, $ax0.h, $ax1.h, $ac0.l, $ac1.l, $ac0.m, and $ac1.m. SRS, however, goes $ac0.h, $ac1.h, invalid, invalid, $ac0.l, $ac1.l, $ac0.m, $ac1.m. As such I've split SRS into SRSH and SRS. SRS with ax registers was used in the free rom, so I've replaced that. It does seem that SRSH appears in the official rom, so this change may improve accuracy.

    The behavior of these instructions can be verified with the srs_test.ds program. What was previously SRS @0x00, $AX0.H or SRS @0x00, $AX1.H does not actually make any stores, and SRS @0x00, $AX0.L or SRS @0x00, $AX1.L actually stores $ac0.h/$ac1.h (including the sign extension).

  • The RTI instruction supports conditions. I highly doubt anything actually uses this, because conditionally returning from an interrupt is rarely useful. Testing this required rejiggering some of the DSPSpy base code to allow custom interrupt handlers to be used.

    The behavior of these instructions can be verified with the rti_test.ds program. Dolphin previously ignored the RTINZ and RTIZ instructions and always had 0x3333 in $ax1.l. Now, Dolphin correctly shows 0x1111 or 0x2222 depending on whether $ax1.h is 0 (i.e. it shows, in order, 0x2222, 0x1111, 0x2222), matching hardware. I haven't actually tested the other conditional variants, and am assuming that they all match the same conditional variants used by other instructions.

Since the free DSP rom needed to be updated, I've also included Tilka/dolphin@84a7cda which makes some other rom-related changes that were not considered important enough for a separate pull request.

Since the old free DSP roms no longer work correctly, I've changed the on-screen display warnings to a AskYesNoFmtT. The message says "If you select "No", audio might be garbled.", which is not strictly accurate (the invalid SRS instructions were actually in ROM code used by the GBA uCode, and using the old ROM with this PR seems to only break GBA transfers and not audio in general). However, it would only affect users who manually copied the free ROM into their user GC folder; users with the official DSP ROM or with no DSP ROM should not see this message.

I will update the gamecube DSP manual for these new instructions in a separate pull request. I already have a large number of manual updates prepared, and would prefer to include this with them.

The test_main change causes this PR to conflict with #10032; although Git will not report a merge conflict, they should not be merged at the same time. It's an easy fix (reg_mask_test.ds will just need test_main: added to it), but it can't be performed until one of the PRs is merged.

Copy link
Member

@BhaaLseN BhaaLseN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code seems good, but I haven't tested it.

@Tilka
Copy link
Member

Tilka commented Aug 18, 2021

What if:

  • try to load ROM from file, warn if its hash doesn't match the original ROM
  • otherwise assemble free ROM from embedded text

This way we don't need to update hashes all the time.

@Pokechu22
Copy link
Contributor Author

That could work, but I didn't actually find the hashes too hard to update. I modified the hashes mismatch message to include the hashes and got them that way (and then I changed the message back).

I feel like it would be useful to have the hash comparison even if we were assembling the free ROM from text, as a sanity check to make sure the assembly worked well. So maybe it'd be better to just always include the hashes in the mismatch message?

@Tilka
Copy link
Member

Tilka commented Aug 18, 2021

I just feel silly maintaining a changelog and a list of file hashes. That's what Git is for.

@Rumi-Larry
Copy link

I just feel silly maintaining a changelog and a list of file hashes. That's what Git is for.

IMO, given the small size of it. keeping this documentation here is fine. Otherwise there would need to be another repository just for the replacement ROMS, which would have been a good idea when they were being created, but now creating a new repository for them seems silly too.

This is so that interrupt handlers can be manually specified in tests that need them.
Pokechu22 and others added 7 commits August 22, 2021 10:49
This makes the point where execution starts more obvious compared to a start_of_test label at the end of the include, and allows putting other functions at the start of the file.  This change also modifies the existing tests to build with this change.
Hardware testing indicated that SRS uses a different list of registers than LRS (specifically, acS.h can be used with SRSH but not LRS, and SRS does not support AX registers, and there are 2 encodings that do nothing).
Doesn't fix anything, hence not upstreaming this.
@Tilka Tilka merged commit 67dce9f into dolphin-emu:master Aug 23, 2021
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants