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

Remove the restriction to use efb copies only once as partial update #3752

Conversation

mimimi085181
Copy link
Contributor

I'm not entirely sure what is happening, but this optimisation is causing an issue in Sonic Riders: Zero Gravity. Apparently the issue would also be fixed by PR#3747, but this PR should also fix similar issues.

Games that use partial updates might get slower with this, so some performance regression testing would be nice. Games like New Super Mario Bros, DK Returns and Silent Hill. Testing with high graphics settings makes sense, since this would mostly end up in more work for the GPU.


This change is Reviewable

I'm not entirely sure what is happening, but this optimisation is causing an issue in Sonic Riders: Zero Gravity. Apparently the issue would also be fixed by PR#3747, but this PR should also fix similar issues.

Games that use partial updates might get slower with this, so some performance regression testing would be nice. Games like New Super Mario Bros, RS2, Zelda TP and Silent Hill. Testing with high graphics settings makes sense, since this would mostly end up in more work for the GPU.
@JMC47
Copy link
Contributor

JMC47 commented Mar 27, 2016

Fixes Sonic Riders/Sonic Riders Zero Gravity on EFB Copies to Texture

@delroth
Copy link
Member

delroth commented Mar 27, 2016

5.0 candidate?

@JMC47
Copy link
Contributor

JMC47 commented Mar 27, 2016

Both games worked on EFB Copies to Texture in 4.0, so I think we should consider it. But, these games aren't totally broken without this, so, if we find any reason not to merge it then it's not an absolute make or break feature.

@mimimi085181
Copy link
Contributor Author

I'd stil like some performance regression testing on this. But i don't know what i was thinking when i mentioned RS2 and Zelda TP, those don't use partial updates.

@JMC47
Copy link
Contributor

JMC47 commented Mar 27, 2016

New Super Mario Bros. Wii - Master - 257 - 260 fps
New Super Mario Bros. Wii - PR3752 - 256 FPS
Silent Hill Shattered Memories - Master - 13 FPS
Silent Hill Shattered Memories - PR3752 - 13 FPS

Note 3x IR

@delroth delroth added this to the Dolphin Release 5.0 milestone Mar 27, 2016
@mimimi085181
Copy link
Contributor Author

Ok, i'm not getting a performance regression either, 35 vs 35 fps at 8xIR in New Super Mario Bros.

@degasus or @phire can you take a look at this? I think this pr can't do any damage, but i'd like to get a 2nd opinion.

@phire
Copy link
Member

phire commented Mar 28, 2016

So the downside of this is that same partial texture might be created multiple times?

@mimimi085181
Copy link
Contributor Author

Yes, it would do the same thing possibly several times. This could be optimised of course, but thinking about the 5.0 release, i wanted to make something that is less likely to miss something.

@phire
Copy link
Member

phire commented Mar 28, 2016

Yeah, this is fine for now and doesn't show major performance impacts. Optimization can be done later.

LGTM

@phire
Copy link
Member

phire commented Mar 28, 2016

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

@JMC47
Copy link
Contributor

JMC47 commented Mar 30, 2016

LGTM

@degasus
Copy link
Member

degasus commented Mar 31, 2016

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


Source/Core/VideoCommon/TextureCacheBase.cpp, line 311 [r1] (raw file):
Is there still a check to not update this texture per use? How did you profile it? Also with a very high IR?


Source/Core/VideoCommon/TextureCacheBase.cpp, line 319 [r1] (raw file):
unrelated, but this case is always true because of :
TexCache::iterator iter = textures_by_address.lower_bound(entry_to_update->addr);


Comments from the review on Reviewable.io

@mimimi085181
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 2 unresolved discussions.


Source/Core/VideoCommon/TextureCacheBase.cpp, line 311 [r1] (raw file):
I was thinking of different ways to optimise this, but maybe it's better to wait for the 5.0 before starting some new optimisation. I tested New Super Mario Bros at 8xIR and got 35 vs 35 fps.


Comments from the review on Reviewable.io

@JMC47
Copy link
Contributor

JMC47 commented Apr 8, 2016

All checks have passed...

I look at ye with the merge rights with accusing eyes.

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