2008/12/8 Stefan Dösinger stefan@codeweavers.com:
- /* TODO: If we're called by shader_glsl_select, we know that both we get past both checks.
* is it cheaper to have a wrapper function check those? Inline some common code? Extra* parameter?*/- } else if(isStateDirty(context, STATE_PIXELSHADER) ||
isStateDirty(context, STATE_VDECL) ||isStateDirty(context, STATE_PIXELSHADERCONSTANT) ||isStateDirty(context, STATE_VERTEXSHADERCONSTANT)) {/* Will be called again later */return;
If you're going to introduce a dependency on state management in the shader backends anyway, you don't need to export constant loading functions from the shader backends at all. You might as well just call shader_select(), and let that figure out if it needs to change the shader or load any constants. That would also avoid this not-so-pretty code here.
If you're going to introduce a dependency on state management in the shader backends anyway, you don't need to export constant loading functions from the shader backends at all. You might as well just call shader_select(), and let that figure out if it needs to change the shader or load any constants. That would also avoid this not-so-pretty code here.
I don't think that would work well. We would need some shader model specific private data in each context(e.g. last vertexshader and last vertexdeclaration) to allow the shader backend to find out what to do, since the dirty state information won't suffice if the shader backend doesn't know where it was called from. It would also tie shader and shader constants, as well as different shader types and constant types together in ARB.
There are however two follow-up ideas of this patch I am toying with:
1: Make the load_constant and shader_select prototypes the same, so GLSL can use one call for all and ARB remains flexible
2: Make the shader backend provide a pipeline template instead of shader_select and shader_load_constant. That would move shader selection out of the fixed function pipeline code and allow the shader to use the state management linking facilities for these sorts of dependencies
2008/12/8 Stefan Dösinger stefan@codeweavers.com:
If you're going to introduce a dependency on state management in the shader backends anyway, you don't need to export constant loading functions from the shader backends at all. You might as well just call shader_select(), and let that figure out if it needs to change the shader or load any constants. That would also avoid this not-so-pretty code here.
I don't think that would work well. We would need some shader model specific private data in each context(e.g. last vertexshader and last vertexdeclaration) to allow the shader backend to find out what to do, since the dirty state information won't suffice if the shader backend doesn't know where it was called from.
Why not?
It would also tie shader and shader constants, as well as different shader types and constant types together in ARB.
That's an issue of course, but we should only reload dirty constants anyway. I've got some patches I could cleanup that do this.
There are however two follow-up ideas of this patch I am toying with:
1: Make the load_constant and shader_select prototypes the same, so GLSL can use one call for all and ARB remains flexible
2: Make the shader backend provide a pipeline template instead of shader_select and shader_load_constant. That would move shader selection out of the fixed function pipeline code and allow the shader to use the state management linking facilities for these sorts of dependencies
I would argue that since shaders replace vertex and/or fragment states, we shouldn't have to go through the state management for replaced pipeline stages in the first place.
I don't think that would work well. We would need some shader model
specific private data in each context(e.g. last vertexshader and last vertexdeclaration) to allow the shader backend to find out what to do, since the dirty state information won't suffice if the shader backend doesn't know where it was called from.
Why not?
How does e.g. ARB find out wether it has to reapply the pixelshader? Or how does GLSL find out if it has to apply a new GLSL program.
I would argue that since shaders replace vertex and/or fragment states, we shouldn't have to go through the state management for replaced pipeline stages in the first place.
And I argue that this should be the implementation's decision. (Read: The state manager and context.c should not treat some states specially)
2008/12/8 Stefan Dösinger stefan@codeweavers.com:
I don't think that would work well. We would need some shader model
specific private data in each context(e.g. last vertexshader and last vertexdeclaration) to allow the shader backend to find out what to do, since the dirty state information won't suffice if the shader backend doesn't know where it was called from.
Why not?
How does e.g. ARB find out wether it has to reapply the pixelshader? Or how does GLSL find out if it has to apply a new GLSL program.
I would expect the relevant state to be dirty, but you say this depends on where the function is called from?
I would argue that since shaders replace vertex and/or fragment states, we shouldn't have to go through the state management for replaced pipeline stages in the first place.
And I argue that this should be the implementation's decision. (Read: The state manager and context.c should not treat some states specially)
Well, you can hardly argue that eg. vertex shader enable is a proper vertex state to begin with.
I would expect the relevant state to be dirty, but you say this depends on where the function is called from?
States are marked clean before the application function is called. This is needed to handle e.g. the relationship between the vertex declaration and the lighting enable state.
What I mean is this: pixelshader(bla bla bla) { /* I know I have to apply the pixelshader */ shader_backend->shader_select(); }
pixelshader(bla bla bla) { /* I know I have to apply the vertex shader */ shader_backend->shader_select(); }
shader_whatever_select() { /* What do I have to do? Apply pixel shader? Or the vertex shader? or both? */ }
Well, you can hardly argue that eg. vertex shader enable is a proper vertex state to begin with.
The vertex shader enable doesn't have any say without the vertex declaration. And the vertex declaration has a similar effect on e.g. D3DRS_FOGVERTEXMODE. This state is treated differently in different shader model versions and different GL pipeline implementations on the fragment side. Is FOGVERTEXMODE a proper state or not?
What I want to say is: We open a can of worms if we try to separate first class(programmable/ffp switch) states and second class states(ffp states) in the general state manager. This should really be left up to the actual pipeline implementation.
(sidenote: The current fog code is a mess)
2008/12/8 Stefan Dösinger stefan@codeweavers.com:
I would expect the relevant state to be dirty, but you say this depends on where the function is called from?
States are marked clean before the application function is called. This is needed to handle e.g. the relationship between the vertex declaration and the lighting enable state.
What I mean is this: pixelshader(bla bla bla) { /* I know I have to apply the pixelshader */ shader_backend->shader_select(); }
pixelshader(bla bla bla) { /* I know I have to apply the vertex shader */ shader_backend->shader_select(); }
shader_whatever_select() { /* What do I have to do? Apply pixel shader? Or the vertex shader? or both? */ }
It would be up to the shader backend to determine what to apply. That might imply storing the current vertex and fragment shader id's in the context, but considering we also store those in the d3d vertex and pixel shaders I think that's acceptable for now.
Does the patch have any other advantages besides making the fixed function state handlers somewhat simpler though? I'm not sure if introducing a dependency on the state tracker in the shader backend is worth this. (I've got a better way to avoid redundant constant loads).
Well, you can hardly argue that eg. vertex shader enable is a proper vertex state to begin with.
The vertex shader enable doesn't have any say without the vertex declaration. And the vertex declaration has a similar effect on e.g. D3DRS_FOGVERTEXMODE. This state is treated differently in different shader model versions and different GL pipeline implementations on the fragment side. Is FOGVERTEXMODE a proper state or not?
Fog is not a pure vertex or fragment state, no (neither is the vertex declaration, strictly speaking, since a large part of what it does is about resource loading rather than how vertices are rendered). state_fog() makes this obvious in a rather painful way.
What I want to say is: We open a can of worms if we try to separate first class(programmable/ffp switch) states and second class states(ffp states) in the general state manager. This should really be left up to the actual pipeline implementation.
That it might not be trivial isn't really an argument against it. Either way, this is pretty much a separate issue from the original patch.
Does the patch have any other advantages besides making the fixed function state handlers somewhat simpler though? I'm not sure if introducing a dependency on the state tracker in the shader backend is worth this. (I've got a better way to avoid redundant constant loads).
The double loading fix was the main intention
The other thing I intend with the patch is to move the GLSL shader<->constant dependency checks to the GLSL code where it belongs
The patch also starts to untie vertex and pixel shaders where we can(read ARB). I think it is wrong that we tie vertex and pixel shaders together in the shader backend interface because one implementation needs it.(On a hypothetical point, we need this for a nvts or atifs shader backend(*)). This might conflict with your intentions with the patch you mentioned above though, in that case we should not do that.
It would be up to the shader backend to determine what to apply. That might imply storing the current vertex and fragment shader id's in the context, but considering we also store those in the d3d vertex and pixel shaders I think that's acceptable for now.
Agreed, I can live with it. I however think that having this instead of the dirty state checks(which aren't completely nice either) moots the point.
That it might not be trivial isn't really an argument against it. Either way, this is pretty much a separate issue from the original patch.
The thing is just that this topic comes back every other patch, and I feel like running in a cicrcle having the same arguments over that point again and again...
(*) I may need pixel shader support on my old laptop for personal reasons in a few weeks, so maybe I invest a rainy weekend of my time into it. Otoh just installing Windows may be an easier fix
2008/12/8 Stefan Dösinger stefan@codeweavers.com:
Does the patch have any other advantages besides making the fixed function state handlers somewhat simpler though? I'm not sure if introducing a dependency on the state tracker in the shader backend is worth this. (I've got a better way to avoid redundant constant loads).
The double loading fix was the main intention
The other thing I intend with the patch is to move the GLSL shader<->constant dependency checks to the GLSL code where it belongs
The patch also starts to untie vertex and pixel shaders where we can(read ARB). I think it is wrong that we tie vertex and pixel shaders together in the shader backend interface because one implementation needs it.(On a hypothetical point, we need this for a nvts or atifs shader backend(*)). This might conflict with your intentions with the patch you mentioned above though, in that case we should not do that.
The patch essentially adds a version to the constants and throws them in a queue. The disadvantage is that setting a constant becomes a bit more expensive, and cache usage becomes a bit worse if you have to load constants from deeper in the tree, but it still gives me about 10 extra fps in the CSS stress test.
I don't see how setting vertex and fragment shaders at the same time hurts us that much, it just gives the shader backend a certain degree of freedom. Please also keep issue 20 from ARB_geometry_shader4 in mind. While it's possible to do crazy stuff like mixing ARB_vertex_program with fragment shaders, you can't mix ARB_vertex_program with a geometry shader. On the other hand, if you really wanted to do that kind of mixing, you could just create two backends and always pass FALSE for usePS to one backend, and always pass FALSE for useVS to the other. It's not impossible, just ugly. Splitting the shader backend doesn't change anything there.
That it might not be trivial isn't really an argument against it. Either way, this is pretty much a separate issue from the original patch.
The thing is just that this topic comes back every other patch, and I feel like running in a cicrcle having the same arguments over that point again and again...
I do have an opinion on how the shader backend should fit in with the rest of the code. So far, I haven't heard anything that would make me change that opinion.
The patch essentially adds a version to the constants and throws them in a queue. The disadvantage is that setting a constant becomes a bit more expensive, and cache usage becomes a bit worse if you have to load constants from deeper in the tree, but it still gives me about 10 extra fps in the CSS stress test.
Whoa, that's an impressive improvement! I didn't see that much room for improvement in the CS:S performance at all.
How expensive is a shader_glsl_load_constant call if no constant was changed? I assume that no GL call is made in that case,
I don't see how setting vertex and fragment shaders at the same time hurts us that much, it just gives the shader backend a certain degree of freedom.
I'd argue that my patch gives the shader backend more freedom. A backend implementation is free to use the same function for vertexshader_select and fragmentshader_select, as we use the same prototype. All it has to do(or better, should do) is checking wether it will be called again via a isStateDirty check. However, this check is done in the current code already, just outside of the shader backend(ie, it is forced onto the backend even if it is not needed)
Please also keep issue 20 from ARB_geometry_shader4 in mind. While it's possible to do crazy stuff like mixing ARB_vertex_program with fragment shaders, you can't mix ARB_vertex_program with a geometry shader.
I don't care too much about mixing ARB with GLSL. It might be a neat trick for MacOS, but it doesn't work in the way we need it in the current architecture anyway. To work around those MacOS GLSL bugs we'd have to select the backend based on the concrete shader. Having a ARBvp+GLSLfrag shader backend selected at library load isn't any better than what we have now in terms of MacOS bugdodgeability.
On the other hand, if you really wanted to do that kind of mixing, you could just create two backends and always pass FALSE for usePS to one backend, and always pass FALSE for useVS to the other. It's not impossible, just ugly. Splitting the shader backend doesn't change anything there.
I'll come back to this should I ever send in an ATIfs or NVTS shader backend :-) . That solution is fine with me.
I do have an opinion on how the shader backend should fit in with the rest of the code. So far, I haven't heard anything that would make me change that opinion.
I think we have a different understanding of what the gobal state table infrastructure is supposed to be.
I didn't intend it as a "fixed function" state table, but rather a tool for managing all sorts of states. It includes things like blending, the scissor test or even the index buffer, which are not affected by shaders at all(since they're neither part of the vertex, fragment or geometry pipeline). So I don't think limiting the state table to the fixed function pipeline would be correct, even if shaders sat on top of it. (yes, there are many fixed function states and just a few shader states, but that's due to the nature of the two pipelines)
I don't think that the vertex shader bypasses the fixed function vertex pipeline argument works here either. We have such a relationship between states elsewhere, e.g. D3DRS_LIGHTING overrides the lights, D3DRS_ALPHABLENDENABLE overrides various alpha states etc. I'm not convinced that shaders are special in any way.
There's also the point about skipping applying fixed function states when the shader is active. I really think this should be up to the fragment pipeline implementation. The ARB fragment pipeline replacement skips the states(because it has to), the fixed function GL pipeline doesn't. Functionally it doesn't make any difference for the ffp pipeline. Performance wise you can argue both. If we apply the states it makes switching the shader on and off cheaper, if we do not apply them it helps broken games.
-
Henri Verbeet -
Stefan Dösinger