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

PowerPC: Implement broken masking for uncached unaligned writes #9964

Merged
merged 5 commits into from
Aug 4, 2021

Conversation

JosJuice
Copy link
Member

@JosJuice JosJuice commented Jul 26, 2021

This implements the behavior described in https://bugs.dolphin-emu.org/issues/12565.

The implementation requires writes to pass through slowmem in order to be affected. Since most users probably don't care about emulating this behavior and thus wouldn't want performance to get lower just for the sake of emulating it, I've added a new INI-only setting AlignmentQuirks which controls whether slowmem is forced for uncached memory.

Thank you to eigenform, delroth, phire, marcan, segher, and Extrems for all helping in one way or another with the efforts to reverse engineer this behavior, and to Rylie for reporting the issue.

@JosJuice
Copy link
Member Author

The code for raising the interrupt doesn't seem to be working properly, at least not when running my hardware test. But everything else should be working.

@MrCheeze
Copy link

MrCheeze commented Jul 26, 2021

Trying this out in Ocarina of Time. Seems like it just crashes in the situation where duplicate writes happen on console now. The below screenshots are 1 breakpoint step apart: tried the wrong, build, sorry

https://cdn.discordapp.com/attachments/85238846538674176/869361246649057280/unknown.png
https://cdn.discordapp.com/attachments/85238846538674176/869361306787012608/unknown.png

The actual build works!!!! (with AlignmentQuirks = True)

@phire
Copy link
Member

phire commented Jul 26, 2021

How much performance would it cost to enable this feature by default?

If the cost is low (less than 1% in most games) it might be worth enabling it by default. Do games even do that many uncached writes by default?

Source/Core/Core/PowerPC/MMU.h Outdated Show resolved Hide resolved
Source/Core/Core/PowerPC/MMU.cpp Outdated Show resolved Hide resolved
Source/Core/Core/PowerPC/MMU.cpp Outdated Show resolved Hide resolved
Source/Core/Core/PowerPC/MMU.cpp Outdated Show resolved Hide resolved
Source/Core/Core/PowerPC/MMU.cpp Outdated Show resolved Hide resolved
Source/Core/Core/PowerPC/MMU.cpp Outdated Show resolved Hide resolved
Source/Core/Core/PowerPC/MMU.cpp Outdated Show resolved Hide resolved
@delroth
Copy link
Member

delroth commented Jul 27, 2021

Thank you to eigenform, delroth, phire, marcan, and Extrems for all helping in one way or another with the efforts to reverse engineer this behavior.

Don't forget segher who spent like 4h explaining to me how the 60x bus works on IRC :-)

@JosJuice
Copy link
Member Author

How much performance would it cost to enable this feature by default?

If the cost is low (less than 1% in most games) it might be worth enabling it by default. Do games even do that many uncached writes by default?

@JMC47, would you like to test? (My laptop with its thermal throttling is probably not the best for testing small performance differences)

Something I want to note is that I plan to revive and fix PR #9865 at some point after this PR has been merged, and I think it would be natural to put the behavior implemented in that PR under the same AlignmentQuirks setting. In addition to needing slowmem for write-through/cache-inhibited memory, that PR also adds an extra branch to the fastmem implementation of memory accesses that cause exceptions when misaligned regardless of the type of memory (most notably all float loadstores). This will have an additional impact on the performance.

@JosJuice
Copy link
Member Author

After some discussion and some quick testing of how often games access uncached memory (100-1000 reads per frame and essentially no writes, it seems like), I've decided to remove the setting for now. Uncached reads and writes will now always go through slowmem.

@JosJuice JosJuice force-pushed the uncached-unaligned-writes branch 2 times, most recently from 7f145b9 to 89a2f92 Compare July 28, 2021 13:23
@JMC47
Copy link
Contributor

JMC47 commented Aug 4, 2021

Seems like everyone is in agreement. Just be sure to watch this for potential performance regressions down the line.

One of the following commits will add emulation of a quirk
that only happens when writing to memory which is mapped as
write-through or cache-inhibited, so let's keep track of
which memory is mapped in this way.
Write_U16_Swap leaves the upper 32 bits alone. Reimplementing this
correctly in the JIT would require more than one instruction,
so let's just call Write_U16_Swap instead, like Jit64 does.
This implements the behavior described in
https://bugs.dolphin-emu.org/issues/12565.

Thank you to eigenform, delroth, phire, marcan, segher, and Extrems
for all helping in one way or another with the efforts to reverse
engineer this behavior, and to Rylie for reporting the issue.
@Tilka Tilka merged commit 942545b into dolphin-emu:master Aug 4, 2021
@JosJuice JosJuice deleted the uncached-unaligned-writes branch August 4, 2021 21:42
@JosJuice
Copy link
Member Author

JosJuice commented Dec 24, 2021

Noticed a typo in the third commit message just now, in case anyone finds this in the future and wonders about it. It should say "upper 16 bits" instead of "upper 32 bits".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
7 participants