D3D: Implemented cache for dynamic render states #475

kayru · 2014-06-08T12:39:22Z

Most render states are now cached, rather than created every time, saving roughly 50% of ApplyState().
This is similar to #365, except only limited to render state caching. Additionally, pending render state is kept in a compact structure, which avoids significant cost of hashing and comparing D3D state descriptions in #365.

galop1n · 2014-06-08T12:50:56Z

i also replace the d3d description structure for lighter version and replace the GetCrc32 with a template magic function that unroll the loop for optimal code :) Not yet pushed on my fork still :)

kayru · 2014-06-08T13:22:44Z

As far as I can tell, we don't need to ever calculate hash of state descriptions. Everything can be packed into u32, except for aniso level.

galop1n · 2014-06-08T14:00:13Z

If you strip everything that is not useful yet from the description, but i
prefer to keep them as they may be needed at some point. And even a 32bits
value need to be hashed to be converted to something that will reduce
collision in the hashmap :)

On Sun, Jun 8, 2014 at 3:22 PM, kayru notifications@github.com wrote:

As far as I can tell, we don't need to ever calculate hash of state
descriptions. Everything can be packed into u32, except for aniso level.

—
Reply to this email directly or view it on GitHub
#475 (comment).

kayru · 2014-06-08T15:02:55Z

Good point about collisions. Though, if this is identified as a real problem, members of the bitfield can be re-arranged to reduce collisions.

As for keeping things general, I'm not sure if it's needed in case of Dolphin. If generalized version is as fast and as simple, then it would make sense to use it, of course.

galop1n · 2014-06-08T15:05:38Z

For example, i made use temporary of the zbias stuff in the rasterizer state, that's an int and two float, not possible to reduce it without reducing the authorized range. And that' by far not a bottleneck anymore anyway

Source/Core/VideoBackends/D3D/Render.cpp

+			{
+				if (state.mag_filter) // linear mag filter
+				{
+					if (mip == TEXF_NONE) sampdc.Filter = D3D11_FILTER_MIN_MAG_LINEAR_MIP_POINT;


kayru · 2014-06-08T19:50:07Z

@lioncash fixed the if/else new line thing

magumagu · 2014-06-08T20:51:50Z

If the description ever does grow too big to fit into a u64, it isn't too hard to change this patch to accommodate that: it's just a matter of implementing a custom hash function. The trick of packing the values into a u32/u64 doesn't really affect most of the patch.

Other than that, the approach in this patch seems generally reasonable.

Source/Core/VideoBackends/D3D/Render.cpp

 } gx_state;

+class GXStateCache


neobrain · 2014-06-08T21:03:02Z

Is there a reason not to have something similar in VideoCommon, so that OpenGL could benefit from it, too?

kayru · 2014-06-08T21:16:32Z

Does OpenGL have the same performance issue, though?

galop1n · 2014-06-08T21:19:29Z

Probably not because GL is not object based and the state machine is lazier. Still, the reason to have something in VideoCommon exists, but at a large scale, Dolphin lacks an lightweight abstract layer to write rendering code once and not have all the copy/past that exist between gl and d3d at the moment.

magumagu · 2014-06-08T21:28:04Z

OpenGL doesn't have any equivalent to the ID3D11RasterizerState, ID3D11DepthStencilState, or ID3D11BlendState objects.

A shared sampler cache implementation probably makes sense; OpenGL already has one, though, so it would just be code refactoring.

lioncash · 2014-06-08T22:42:50Z

@kayru

fixed the if/else new line thing

Thanks :)

kayru · 2014-06-08T22:42:50Z

I've addressed most of the comments. One thing that I did not do is switch to BitField template. There are other unions/bitfields in the codebase that follow the same explicit style, such as TexMode0 in BPMemory.h.

magumagu · 2014-06-08T23:42:08Z

The BitField class is new, so not all existing code uses it. New code which depends on bitfield layout (e.g. a union with u64) should.

Also, lint is failing with the latest patch.

kayru · 2014-06-09T06:47:53Z

@magumagu there's another problem with BitField. Some of the packed members represent D3D11 enums. Using BitField requires casts like this: (D3D11_COLOR_WRITE_ENABLE)(u32)state.write_mask.
Is there a better way? If it was a custom enum, then appropriate storage type could be specified and everything would be fine.

magumagu · 2014-06-09T06:57:11Z

I think BitField supports using an enum as the field type; does that not work for you?

kayru · 2014-06-09T07:10:35Z

Yes it does, but if we're not in control over enum storage type, it may not always work.

magumagu · 2014-06-09T07:21:13Z

I don't follow. The enum is part of the DirectX SDK, therefore the representation can't change without breaking compatibility with existing projects.

kayru · 2014-06-09T07:24:05Z

Consider a situation where D3D enum is part of a union that's over 32 bits.

magumagu · 2014-06-09T07:45:00Z

Err, oh, I see what you mean.

Might make sense to modify the BitField class to allow specifying the storage type separately from the type of the value.

kayru · 2014-06-09T07:51:16Z

I think modifying BitField class is beyond the scope of this patch.

magumagu · 2014-06-09T08:27:38Z

Okay. I guess it's not really such a big deal here, given that all you're doing with the u64 is using it as the key for a hashtable.

kayru · 2014-06-09T09:02:54Z

I just encountered a pretty scary issue with BitField in unions.
When BitField is copied, then the entire storage is copied with it, not just the affected bits. This is a problem if you want to partially copy a union with BitField-s.
Example: https://gist.github.com/kayru/327ad61b698f09642ab6

magumagu · 2014-06-14T20:48:49Z

LGTM.

Source/Core/VideoBackends/D3D/StateCache.h

+
+union RasterizerState
+{
+	BitField<0, 2, D3D11_CULL_MODE> cull_mode;


kayru · 2014-10-15T07:45:47Z

Ok, I'll try to repro it. Does it happen immediately at startup?

PatrickFerry · 2014-10-15T07:53:38Z

Yes, it happens immediately on D3D(of course D3D)

JMC47 · 2014-10-15T09:01:34Z

I should say that this provides a 20 - 30% performance boost in D3D in pretty much every game. I would get more exact numbers, but there's no benefit to knowing them.

PatrickFerry · 2014-10-15T10:52:13Z

It seems that I actually have the exact same problem as the user BadKarma above.

badkarma12 · 2014-10-15T15:54:16Z

Launch another game first, end emulation and then launch mutant turtles. It
will work then.
On Oct 15, 2014 2:27 AM, "Patrick" notifications@github.com wrote:

This PR breaks Teenage Mutant Ninja Turtles(2003)
It gives the error "Failed to create rasterizer state at GfxState.cpp 262"

—
Reply to this email directly or view it on GitHub
#475 (comment).

PatrickFerry · 2014-10-15T17:13:32Z

Yeah that's what I did and it made the game work. So it's the same issue we're seeing.

kayru · 2014-10-15T17:57:40Z

I am able to repro the issue in Wind Waker. Expect fix soon :)

badkarma12 · 2014-10-15T17:58:41Z

Wind waker, brawl and a few others I can't remember.
On Oct 15, 2014 12:56 PM, "kayru" notifications@github.com wrote:

@badkarma12 https://github.com/badkarma12 what game did you test this
on?

—
Reply to this email directly or view it on GitHub
#475 (comment).

Refactored StateCache::Get() to early out for narrower indentation. Added comments to clarify ownership of objects returned by StateCache::Get().

kayru · 2014-10-15T18:25:13Z

Please try bea68c9. It fixes the WW issue. I'm going to assume that TMNT hits the same bug.

PatrickFerry · 2014-10-15T20:09:01Z

Yep. The latest patch fixes all the games in my library that had that issue.

Parlane · 2014-10-16T07:56:03Z

@dolphin-emu-bot rebuild

degasus · 2014-10-16T12:39:01Z

LGTM, however I'd be more happy to see this (mostly all of those state setters currently in Render.cpp) in videocommon. As gx_state rarely depends on D3D enums, it's a step in the right direction :) I guess it's worth to implement some general state hacks there, eg the current logicOp -> blending mappings.

Do you think it's worth to reduce the identical state space? eg disabled blending but different blending parameters?

kayru · 2014-10-16T14:31:50Z

Yeah, I think reducing the state space a little bit would be nice. Unlikely to improve performance much, but is a good practice. I'll try it out tonight.

…llapsing some states that have blending disabled

Source/Core/VideoBackends/D3D/Render.cpp

-		gx_state.blenddc.RenderTarget[0].DestBlendAlpha = val;
-
-	gx_state.blenddc.RenderTarget[0].DestBlend = val;
+	gx_state.blend.dst_blend = val;
 }

 void SetBlendOp(D3D11_BLEND_OP val)


JMC47 · 2014-10-17T10:12:43Z

On NVIDIA, I haven't found any problems with this yet; it's just seems to be faster. It's still not as fast as OpenGL in most situations, but it's an excellent step in the right direction, and doesn't seem to bring any instabilities.

On my Radeon HD5850 OpenGL and D3D seems to be the fastest backend no question. There are some exceptions (Melee is pretty close,) but in most games with this Pull Request, I see a healthy boost in speed.

I didn't notice any graphical regressions in either of my graphics cards.

@kayru I know this is slightly off topic, but you're the only one I know actively working on D3D and I dunno where else to find you. There are some pretty infamous viewport issues in D3D that make it different from OpenGL and Software Renderer in behavior. Mario Tennis's stadiums blocking the view, Luigi's Mansion's Mirrors not working, Silent Hill's flashlight being blocked by the guy's hand. There's a patch from Galop1n's fork that fixes this, and shouldn't affect anything else. I was wondering if you'd be willing to look at it, and possibly clean it up if necessary and make a Pull Request for it. I feel bad bugging you about it, but, it'd be great to have the backends all giving the same results.

Patch location: https://dl.dropboxusercontent.com/u/484730/SH%20fix.patch

galop1n · 2014-10-17T14:33:07Z

Still nothing reach te main branch from my amazing perf boost and accurate
fixes !!

My goods were stuck at the custom and even if i still sleep on the floor up
to wenesday, i am living the California dream :)

I will come again, but i send this email with my only internet access, my
phone...
On Oct 17, 2014 3:12 AM, "JMC47" notifications@github.com wrote:

On NVIDIA, I haven't found any problems with this yet; it's just seems to
be faster. It's still not as fast as OpenGL in most situations, but it's an
excellent step in the right direction, and doesn't seem to bring any
instabilities.

On my Radeon HD5850 OpenGL and D3D seems to be the fastest backend no
question. There are some exceptions (Melee is pretty close,) but in most
games with this Pull Request, I see a healthy boost in speed.

I didn't notice any graphical regressions in either of my graphics cards.

@kayru https://github.com/Kayru I know this is slightly off topic, but
you're the only one I know actively working on D3D and I dunno where else
to find you. There are some pretty infamous viewport issues in D3D that
make it different from OpenGL and Software Renderer in behavior. Mario
Tennis's stadiums blocking the view, Luigi's Mansion's Mirrors not working,
Silent Hill's flashlight being blocked by the guy's hand. There's a patch
from Galop1n's fork that fixes this, and shouldn't affect anything else. I
was wondering if you'd be willing to look at it, and possibly clean it up
if necessary and make a Pull Request for it. I feel bad bugging you about
it, but, it'd be great to have the backends all giving the same results.

Patch location:
http://dolphin-emu.googlecode.com/issues/attachment?aid=77550002000&name=SH+fix.patch&token=ABZ6GAeoyloo47isooaFX1qp4xQRvmS2vg%3A1413540538364

—
Reply to this email directly or view it on GitHub
#475 (comment).

JMC47 · 2014-10-17T18:18:06Z

@galop1n: Trust me, I'm trying to get your stuff to master! It's hard because separating out your stuff from the mega commit is tough, and going through to isolate features is difficult because a lot of the builds on the fork don't compile. Your work was amazing.

… trivial

degasus · 2014-10-17T22:34:35Z

Nothing remaining to criticize. LGTM

D3D: Implemented cache for dynamic render states

lioncash reviewed Jun 8, 2014
View reviewed changes

Source/Core/VideoBackends/D3D/Render.cpp

{

if (state.mag_filter) // linear mag filter

{

if (mip == TEXF_NONE) sampdc.Filter = D3D11_FILTER_MIN_MAG_LINEAR_MIP_POINT;

This comment was marked as off-topic.

Sign in to view

magumagu reviewed Jun 8, 2014
View reviewed changes

Source/Core/VideoBackends/D3D/Render.cpp

} gx_state;

class GXStateCache

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

neobrain reviewed Jun 22, 2014
View reviewed changes

Source/Core/VideoBackends/D3D/StateCache.h

union RasterizerState

{

BitField<0, 2, D3D11_CULL_MODE> cull_mode;

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

kayru force-pushed the d3d_state_cache branch from 6ddadbb to e6e7516 Compare October 15, 2014 18:22

kayru added 5 commits October 15, 2014 20:22

D3D: Implemented cache for dynamic render states

f434bd7

D3D: Moved render state cache into separate source files.

2e4667c

Refactored StateCache::Get() to early out for narrower indentation. Added comments to clarify ownership of objects returned by StateCache::Get().

D3D: State cache now uses BitField to define packed render states

e7f8032

D3D: moved render state cache implementation to D3DState.h/cpp

21655dc

D3D: Fixed uninitialized members of gx_state

bea68c9

kayru force-pushed the d3d_state_cache branch from e6e7516 to bea68c9 Compare October 15, 2014 18:23

D3D: State cache now reduces number of blend state permutations by co…

9bdfd4a

…llapsing some states that have blending disabled

degasus reviewed Oct 17, 2014
View reviewed changes

D3D: Removed SetBlendOp, SetSrcBlend and SetDestBlend as they are now…

b783968

… trivial

skidau added a commit that referenced this pull request Oct 18, 2014

Merge pull request #475 from kayru/d3d_state_cache

a5674bb

D3D: Implemented cache for dynamic render states

skidau merged commit a5674bb into dolphin-emu:master Oct 18, 2014

kayru deleted the d3d_state_cache branch October 18, 2014 17:15

D3D: Implemented cache for dynamic render states #475

D3D: Implemented cache for dynamic render states #475

Conversation

kayru commented Jun 8, 2014

galop1n commented Jun 8, 2014

kayru commented Jun 8, 2014

galop1n commented Jun 8, 2014

kayru commented Jun 8, 2014

galop1n commented Jun 8, 2014

This comment was marked as off-topic.

kayru commented Jun 8, 2014

magumagu commented Jun 8, 2014

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

neobrain commented Jun 8, 2014

kayru commented Jun 8, 2014

galop1n commented Jun 8, 2014

magumagu commented Jun 8, 2014

lioncash commented Jun 8, 2014

kayru commented Jun 8, 2014

magumagu commented Jun 8, 2014

kayru commented Jun 9, 2014

magumagu commented Jun 9, 2014

kayru commented Jun 9, 2014

magumagu commented Jun 9, 2014

kayru commented Jun 9, 2014

magumagu commented Jun 9, 2014

kayru commented Jun 9, 2014

magumagu commented Jun 9, 2014

kayru commented Jun 9, 2014

magumagu commented Jun 14, 2014

This comment was marked as off-topic.

This comment was marked as off-topic.

kayru commented Oct 15, 2014

PatrickFerry commented Oct 15, 2014

JMC47 commented Oct 15, 2014

PatrickFerry commented Oct 15, 2014

badkarma12 commented Oct 15, 2014

PatrickFerry commented Oct 15, 2014

kayru commented Oct 15, 2014

badkarma12 commented Oct 15, 2014

kayru commented Oct 15, 2014

PatrickFerry commented Oct 15, 2014

Parlane commented Oct 16, 2014

degasus commented Oct 16, 2014

kayru commented Oct 16, 2014

This comment was marked as off-topic.

This comment was marked as off-topic.

JMC47 commented Oct 17, 2014

galop1n commented Oct 17, 2014

JMC47 commented Oct 17, 2014

degasus commented Oct 17, 2014