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

Tmem cache emulation option #7562

Conversation

mimimi085181
Copy link
Contributor

Patches issue #11164 https://bugs.dolphin-emu.org/issues/11164, which is a regression and was introduced in 5.0-4703 PR #5726.

The pr just makes the tmem cache emulation optional and disables it for the games that are known to be broken with it. Ideally, it would be improved/repaired to work properly, but that's beyond by my skillset. An alternative would be to remove it completely, since the currently implemented tmem cache emulation is just a hack, but that would break some effects in Spyro: A Hero's Tail.

I have chosen to enable the option by default to avoid reports of reduced performance.

Also, i wanted to put the option in the GUI right under the texture cache slider, but couldn't figure out how to align it with the other 2nd column options.

G5S - Spyro: A Hero's Tail - Setting enabled
GJU - Tak and the Power of Juju - Setting disabled
GM5 - Metal Arms: Glitch in the System - Setting disabled
GXE - Sonic Riders - Setting disabled
RS9 - SONIC RIDERS ZERO GRAVITY - Setting disabled
SHY - NHL Slapshot - Setting disabled
@JMC47
Copy link
Contributor

JMC47 commented Nov 10, 2018

Considering we need this for Sonic Riders to work, I am obviously for this until TMEM emulation can be fixed up.

@mimimi085181
Copy link
Contributor Author

What? Isn't that one of the games broken by this? Can you explain what exactly needs tmem cache emulation there?

@JMC47
Copy link
Contributor

JMC47 commented Nov 10, 2018

TMEM emulation breaks the FMVs, which is why a toggle to disable TMEM is good.

@mimimi085181
Copy link
Contributor Author

Oh good, i thought it needs tmem cache emulation for something, which would complicate things.

@JMC47
Copy link
Contributor

JMC47 commented Nov 10, 2018

Yeah, there doesn't seem to be any games broken by texmem that also need texmem.

@mimimi085181
Copy link
Contributor Author

Hmm, after seeing PR #7569 i'm wondering, if this setting should be synced for netplay as well. I can add it, if somebody tells me to.

@delroth
Copy link
Member

delroth commented Nov 17, 2018

I think the bar for introducing new hacks should be higher than "we don't understand the issue". Hacks are OK when we think there is no easy solution to the problem, not when we haven't looked enough into the problem to tell.

@mimimi085181
Copy link
Contributor Author

mimimi085181 commented Nov 17, 2018

I understand that the proper solution is to make the tmem cache emulation actually cache the exact same things that are cached on the gamecube/wii. Since that is beyond my skillset and there are at least 5 games right now that have been broken for over 1 year now, i think something should be done.

The alternative to this pr, that i'd be able to implement, would be to remove the feature again and break Spyro. Removing it instead of making it optional would have one big advantage: It would increase the incentive for somebody to implement a proper solution. The tmem cache emulation was part of Dolphin for over a year now, if there were a few other affected games, i think JMC or somebody else would have found some of them by now. If there's only 1 affected game, and it "works", i think it's hard to get somebody to work on it.

I see 4 options:

  • Merge this pr(or something similar) to get as many games as possible playable
  • Remove the feature to incentivise somebody to implement a proper solution
  • Tweak the code to "work" for all known affected games to get as many games as possible playable. (If i was to do that, i'd create an even bigger mess than it is now)
  • Leave it as is and wait for somebody to come along to fix it

I'd be genuinely glad, if i'm overlooking another, a better, alternative.

PS: Sorry, there's another bad alternative that i thought of and i dismissed it, it's a variation of the 1st alternative. Add the option to enable/disable tmem cache emulation, don't expose it to the GUI, and only turn it on for Spyro.

@tony971
Copy link

tony971 commented Jan 17, 2019

I think the bar for introducing new hacks should be higher than "we don't understand the issue". Hacks are OK when we think there is no easy solution to the problem, not when we haven't looked enough into the problem to tell.

If a feature is partially implemented and creates more regressions than it fixes, is it really a hack to make it optional?

@phire
Copy link
Member

phire commented Apr 7, 2019

I think the bar for introducing new hacks should be higher than "we don't understand the issue".

Sorry, I kind of disappeared after introducing this. Guess I should drop by and add some understanding.

I knew this might be a potential issue.

The minimal TMEM implementation was based on the assumption that if a game bound a texture, then overwrote the texture (with a efb copy or cpu write) without first unbinding, then the game would do the correct call to invalidate the TMEM.

But games might (deliberately or by accident) setup TMEM and their in such a way that the texture is guaranteed to be flushed out of memory. There are two ways to guarantee this:

  1. If you have a large texture that is too big for the configured TMEM region, and you drew it in a predictable manor (say you have a video frame, and you draw a single quad) then you can be pretty sure that the end of the texture will push the start of the texture out of TMEM.
  2. If you have two texture stages configured to use the same TMEM region and use them on alternating draw calls.

We don't have to emulate all of TMEM to fix this regression: My preferred solution is to only emulate TMEM caching when the bound texture is provably small enough to fit in TMEM.

But this is complicated by the fact that games are allowed to configure TMEM any way they want. Each stage has a configurable start address and size. Games could give each stage 128kb of TMEM. Or give the first stage 512kb and the remaining stages 64k each.

Games can even configure the stages TMEM regions to fully or partially overlap, give all stages a full shared 1MB of TMEM.

@shuffle2
Copy link
Contributor

shuffle2 commented Apr 9, 2019

It sounds like a first step could be to profile if problematic games (which people have noticed so far) are relying on case 1 or 2 of phire's explanation. If nothing/low amount hit case 2, it could be further deferred.

@mimimi085181
Copy link
Contributor Author

No need to keep this open

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