[jak 2] Fix possible stereo desync in overlord #2663
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ManDude
reviewed
May 18, 2023
Ziemas
reviewed
May 21, 2023
Comment on lines
965
to
+971
| u32 GetSpuRamAddress(VagCmd* param_1) { | ||
| bool bVar1; | ||
| u32 uVar2; | ||
| u32 uVar3; | ||
| u32 uVar4; | ||
| u32 uVar5; | ||
| u32 uVar6; | ||
| u32 uVar7; | ||
|
|
||
| uVar7 = param_1->spu_stream_dma_mem_addr; | ||
| uVar6 = (param_1->voice & 0xffffU) | 0x2240; | ||
| do { | ||
| uVar5 = 0; | ||
| do { | ||
| uVar2 = sceSdGetAddr(uVar6); | ||
| uVar3 = sceSdGetAddr(uVar6); | ||
| uVar4 = sceSdGetAddr(uVar6); | ||
| // printf("got nax: %d\n", uVar3); | ||
| if ((uVar2 == uVar3) || | ||
| ((uVar3 != uVar4 && (bVar1 = uVar2 == uVar4, uVar4 = uVar5, bVar1)))) { | ||
| uVar4 = uVar2; | ||
| } | ||
| uVar5 = uVar4; | ||
| } while (uVar4 == 0); | ||
| } while ((uVar4 < uVar7) || (uVar7 + 0x4040 <= uVar4)); | ||
| return uVar4; | ||
| // this is simplified a lot from the original. | ||
| // they seem to sanity check if the value is reasonable or not, but the sanity check can fail | ||
| // if the overlord thread isn't keeping up. | ||
| // as far as I can tell, it's totally fine to discard these checks because our sceSdGetAddr | ||
| // works perfectly. | ||
| return sceSdGetAddr((param_1->voice & 0xffffU) | 0x2240); |
There was a problem hiding this comment.
There's two things going on here. There's the safety check of calling sceSdGetAddr three times, this is because the SPU address registers are composed of two 16bit registers which means there's risk of tearing when reading. This part can safely be omitted for us.
The other check seems to be that the address is contained within the allocated region (0x4040 bytes), if this is actually happening it means the voice has escaped the allocated area, this might be kinda bad if it actually happens for us.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Normally, when they allocate a VagCmd, they do a bunch of stuff to clear all the status bits and reset things
in particular the InitVAGCmd function does a lot
but for the stereo command, they do a lot less:
Which means that the new_stereo_command can just have random status bits left over from whatever the last user had.
we seem to end up in a state where byte21 is set, and this causes everything else to be wrong and off-by-one dma transfer. My guess is that the original game avoided this bug due to lucky timing that I don't understand.
I think the fix of just clearing byte21 is ok because there's no way that the old value of the byte is useful after the command is repurposed.