On 10 January 2014 11:41, Kenneth Graunke <kenn...@whitecape.org> wrote:
> On 01/09/2014 06:19 PM, Paul Berry wrote:
> > This is possible now that ctx->Shader.CurrentProgram is an array.
> > ---
> > src/mesa/main/texstate.c | 75
> +++++++++++++++++++-----------------------------
> > 1 file changed, 29 insertions(+), 46 deletions(-)
> >
> > diff --git a/src/mesa/main/texstate.c b/src/mesa/main/texstate.c
> > index b9c76da..905a9d5 100644
> > --- a/src/mesa/main/texstate.c
> > +++ b/src/mesa/main/texstate.c
> > @@ -526,27 +526,20 @@ static void
> > update_texture_state( struct gl_context *ctx )
> > {
> > GLuint unit;
> > - struct gl_program *fprog = NULL;
> > - struct gl_program *vprog = NULL;
> > - struct gl_program *gprog = NULL;
> > + struct gl_program *prog[MESA_SHADER_STAGES];
> > GLbitfield enabledFragUnits = 0x0;
> > -
> > - if (ctx->Shader.CurrentProgram[MESA_SHADER_VERTEX] &&
> > - ctx->Shader.CurrentProgram[MESA_SHADER_VERTEX]->LinkStatus) {
> > - vprog =
> ctx->Shader.CurrentProgram[MESA_SHADER_VERTEX]->_LinkedShaders[MESA_SHADER_VERTEX]->Program;
> > - }
> > -
> > - if (ctx->Shader.CurrentProgram[MESA_SHADER_GEOMETRY] &&
> > - ctx->Shader.CurrentProgram[MESA_SHADER_GEOMETRY]->LinkStatus) {
> > - gprog =
> ctx->Shader.CurrentProgram[MESA_SHADER_GEOMETRY]->_LinkedShaders[MESA_SHADER_GEOMETRY]->Program;
> > - }
> > -
> > - if (ctx->Shader.CurrentProgram[MESA_SHADER_FRAGMENT] &&
> > - ctx->Shader.CurrentProgram[MESA_SHADER_FRAGMENT]->LinkStatus) {
> > - fprog =
> ctx->Shader.CurrentProgram[MESA_SHADER_FRAGMENT]->_LinkedShaders[MESA_SHADER_FRAGMENT]->Program;
> > - }
> > - else if (ctx->FragmentProgram._Enabled) {
> > - fprog = &ctx->FragmentProgram.Current->Base;
> > + int i;
> > +
> > + for (i = 0; i < MESA_SHADER_STAGES; i++) {
> > + if (ctx->Shader.CurrentProgram[i] &&
> > + ctx->Shader.CurrentProgram[i]->LinkStatus) {
> > + prog[i] =
> ctx->Shader.CurrentProgram[i]->_LinkedShaders[i]->Program;
> > + } else {
> > + if (i == MESA_SHADER_FRAGMENT && ctx->FragmentProgram._Enabled)
> > + prog[i] = &ctx->FragmentProgram.Current->Base;
> > + else
> > + prog[i] = NULL;
> > + }
> > }
> >
> > /* TODO: only set this if there are actual changes */
> > @@ -562,9 +555,7 @@ update_texture_state( struct gl_context *ctx )
> > */
> > for (unit = 0; unit < ctx->Const.MaxCombinedTextureImageUnits;
> unit++) {
> > struct gl_texture_unit *texUnit = &ctx->Texture.Unit[unit];
> > - GLbitfield enabledVertTargets = 0x0;
> > - GLbitfield enabledFragTargets = 0x0;
> > - GLbitfield enabledGeomTargets = 0x0;
> > + GLbitfield enabledTargetsByStage[MESA_SHADER_STAGES];
> > GLbitfield enabledTargets = 0x0;
> > GLuint texIndex;
> >
> > @@ -574,25 +565,16 @@ update_texture_state( struct gl_context *ctx )
> > * by a fragment program/program. When multiple flags are set,
> we'll
> > * settle on the one with highest priority (see below).
> > */
> > - if (vprog) {
> > - enabledVertTargets |= vprog->TexturesUsed[unit];
> > + for (i = 0; i < MESA_SHADER_STAGES; i++) {
> > + if (prog[i])
> > + enabledTargetsByStage[i] = prog[i]->TexturesUsed[unit];
> > + else if (i == MESA_SHADER_FRAGMENT)
> > + enabledTargetsByStage[i] = texUnit->Enabled;
> > + else
> > + enabledTargetsByStage[i] = 0;
> > + enabledTargets |= enabledTargetsByStage[i];
>
> This doesn't look equivalent. Previously,
> enabled{Vert,Frag,Geom}Targets would accumulate bits over every
> iteration through units. Now, enabledTargetsByStage only reflects the
> most recent unit.
>
> enabledTargets still accumulates properly, but you use
> enabledTargetsByStage below, so I think it needs to as well.
>
> You just need to use |= in both places, like the old code.
>
It's definitely possible that I got something wrong in this patch--I didn't
completely grok what the code was doing and instead strived to just
preserve the existing semantics.
But I'm pretty sure I got the enabledTargetsByStage part right. Here's
what the code used to look like--the enabled{Vert,Frag,Geom}Targets
variables were all reset to 0 every time through the loop:
for (unit = 0; unit < ctx->Const.MaxCombinedTextureImageUnits; unit++) {
struct gl_texture_unit *texUnit = &ctx->Texture.Unit[unit];
GLbitfield enabledVertTargets = 0x0;
GLbitfield enabledFragTargets = 0x0;
GLbitfield enabledGeomTargets = 0x0;
GLbitfield enabledTargets = 0x0;
GLuint texIndex;
/* Get the bitmask of texture target enables.
* enableBits will be a mask of the TEXTURE_*_BIT flags indicating
* which texture targets are enabled (fixed function) or referenced
* by a fragment program/program. When multiple flags are set, we'll
* settle on the one with highest priority (see below).
*/
if (vprog) {
enabledVertTargets |= vprog->TexturesUsed[unit];
}
if (gprog) {
enabledGeomTargets |= gprog->TexturesUsed[unit];
}
if (fprog) {
enabledFragTargets |= fprog->TexturesUsed[unit];
}
else {
/* fixed-function fragment program */
enabledFragTargets |= texUnit->Enabled;
}
enabledTargets = enabledVertTargets | enabledFragTargets |
enabledGeomTargets;
...
}
I don't know if that was intentional or a bug, but I'm pretty sure my patch
preserves the behaviour.
>
> Also, I just want to say - this code is absurdly confusing. Always has
> been. I don't envy you having to touch it. :)
>
You can say that again :) Hopefully my patch doesn't make the situation
any worse.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
