[Mesa-dev] [PATCH] glsl/mesa: stop duplicating tess layout values
Iago Toral
itoral at igalia.com
Wed Jun 22 07:30:56 UTC 2016
On Wed, 2016-06-22 at 09:54 +1000, Timothy Arceri wrote:
> On Tue, 2016-06-21 at 17:45 +0200, Iago Toral wrote:
> > On Tue, 2016-06-21 at 12:21 +1000, Timothy Arceri wrote:
> > >
> > > We already store this in gl_shader and gl_program here we
> > > remove it from gl_shader_program and just use the values
> > > from gl_shader.
> > >
> > > This will allow us to keep the shader cache restore code as
> > > simple as it can be while making it somewhat clearer where these
> > > values originate from.
> > This sounds like a good idea, but the issue I see is that other
> > stages
> > seem to follow the same pattern and here you only change one stage...
> > If
> > we are not going to do this change for all stages to keep some
> > consistency I am not so sure that this is actually such a good idea,
> > it
> > may actually bring more confusion.
>
> Fair enough point. I only did this stage because its the only one which
> actually use the values from gl_program after linking is done (at least
> on i965).
>
> I'll do some tidy ups for the other stages too.
Aha, ok, then it is a bit special after all. In that case maybe the it
is fine as it is, dunno. If you think doing something similar for other
stages helps something then go ahead, otherwise I guess we can keep it
as is.
> >
> > I have a few minor comments below, with those fixed you can add my Rb
> > to
> > this patch,
>
> I've commented below I don't think any changes are needed :)
I see, ok!
> > but I think you should at least try to get someone else to
> > give an explicit ok for changing this only for Tess before pushing.
>
> I'll send cleanups for the other stage too.
>
> >
> > >
> > > ---
> > > src/compiler/glsl/linker.cpp | 4 ----
> > > src/mesa/main/api_validate.c | 11 ++++++-----
> > > src/mesa/main/mtypes.h | 7 -------
> > > src/mesa/main/shaderapi.c | 35 +++++++++++++++++++++++---------
> > > ---
> > > 4 files changed, 29 insertions(+), 28 deletions(-)
> > >
> > > diff --git a/src/compiler/glsl/linker.cpp
> > > b/src/compiler/glsl/linker.cpp
> > > index 9c6147b..ec71bfe 100644
> > > --- a/src/compiler/glsl/linker.cpp
> > > +++ b/src/compiler/glsl/linker.cpp
> > > @@ -1890,19 +1890,15 @@ link_tes_in_layout_qualifiers(struct
> > > gl_shader_program *prog,
> > > "primitive modes.\n");
> > > return;
> > > }
> > > - prog->TessEval.PrimitiveMode = linked_shader-
> > > >TessEval.PrimitiveMode;
> > >
> > > if (linked_shader->TessEval.Spacing == 0)
> > > linked_shader->TessEval.Spacing = GL_EQUAL;
> > > - prog->TessEval.Spacing = linked_shader->TessEval.Spacing;
> > >
> > > if (linked_shader->TessEval.VertexOrder == 0)
> > > linked_shader->TessEval.VertexOrder = GL_CCW;
> > > - prog->TessEval.VertexOrder = linked_shader-
> > > >TessEval.VertexOrder;
> > >
> > > if (linked_shader->TessEval.PointMode == -1)
> > > linked_shader->TessEval.PointMode = GL_FALSE;
> > > - prog->TessEval.PointMode = linked_shader->TessEval.PointMode;
> > > }
> > >
> > >
> > > diff --git a/src/mesa/main/api_validate.c
> > > b/src/mesa/main/api_validate.c
> > > index c7625c3..634040f 100644
> > > --- a/src/mesa/main/api_validate.c
> > > +++ b/src/mesa/main/api_validate.c
> > > @@ -206,9 +206,10 @@ _mesa_valid_prim_mode(struct gl_context *ctx,
> > > GLenum mode, const char *name)
> > > GLenum mode_before_gs = mode;
> > >
> > > if (tes) {
> > Shouldn't we also do:
> >
> > if (tes->_LinkedShaders[MESA_SHADER_TESS_EVAL]) instead of the line
> > above
> > and remove the 'tes' variable? It looks like we only use it here
> > after this change.
>
> No I don't think so. We need to null check tes aka ctx->_Shader-
> >CurrentProgram[MESA_SHADER_TESS_EVAL] as the existing code does. We
> then get the shader from the gl_shader_program struct.
>
> ctx->_Shader->CurrentProgram[MESA_SHADER_TESS_EVAL]-
> >_LinkedShaders[MESA_SHADER_TESS_EVAL]
>
> _LinkedShaders[MESA_SHADER_TESS_EVAL] should never be null in this case
> unless something has gone wrong elsewhere.
>
> >
> > >
> > > - if (tes->TessEval.PointMode)
> > > + struct gl_shader *tes_sh = tes-
> > > >_LinkedShaders[MESA_SHADER_TESS_EVAL];
> > > + if (tes_sh->TessEval.PointMode)
> > > mode_before_gs = GL_POINTS;
> > > - else if (tes->TessEval.PrimitiveMode == GL_ISOLINES)
> > > + else if (tes_sh->TessEval.PrimitiveMode == GL_ISOLINES)
> > > mode_before_gs = GL_LINES;
> > > else
> > > /* the GL_QUADS mode generates triangles too */
> > > @@ -321,10 +322,10 @@ _mesa_valid_prim_mode(struct gl_context *ctx,
> > > GLenum mode, const char *name)
> > > else if (ctx->_Shader-
> > > >CurrentProgram[MESA_SHADER_TESS_EVAL]) {
> > > struct gl_shader_program *tes =
> > > ctx->_Shader->CurrentProgram[MESA_SHADER_TESS_EVAL];
> > If I am not wrong, 'tes' is also unused here after these changes, so
> > just remove it too.
>
> Again its used below:
>
> tes->_LinkedShaders[MESA_SHADER_TESS_EVAL];
>
> >
> > >
> > > -
> > > - if (tes->TessEval.PointMode)
> > > + struct gl_shader *tes_sh = tes-
> > > >_LinkedShaders[MESA_SHADER_TESS_EVAL];
> > > + if (tes_sh->TessEval.PointMode)
> > > pass = ctx->TransformFeedback.Mode == GL_POINTS;
> > > - else if (tes->TessEval.PrimitiveMode == GL_ISOLINES)
> > > + else if (tes_sh->TessEval.PrimitiveMode == GL_ISOLINES)
> > > pass = ctx->TransformFeedback.Mode == GL_LINES;
> > > else
> > > pass = ctx->TransformFeedback.Mode == GL_TRIANGLES;
> > > diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> > > index e7ed509..dad0ac1 100644
> > > --- a/src/mesa/main/mtypes.h
> > > +++ b/src/mesa/main/mtypes.h
> > > @@ -2729,13 +2729,6 @@ struct gl_shader_program
> > > * Tessellation Evaluation shader state from layout qualifiers.
> > > */
> > > struct {
> > > - /** GL_TRIANGLES, GL_QUADS or GL_ISOLINES */
> > > - GLenum PrimitiveMode;
> > > - /** GL_EQUAL, GL_FRACTIONAL_ODD or GL_FRACTIONAL_EVEN */
> > > - GLenum Spacing;
> > > - /** GL_CW or GL_CCW */
> > > - GLenum VertexOrder;
> > > - bool PointMode;
> > > /**
> > > * True if gl_ClipDistance is written to. Copied into
> > > * gl_tess_eval_program by _mesa_copy_linked_program_data().
> > > diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
> > > index fed8b6d..605c83c 100644
> > > --- a/src/mesa/main/shaderapi.c
> > > +++ b/src/mesa/main/shaderapi.c
> > > @@ -831,26 +831,34 @@ get_programiv(struct gl_context *ctx, GLuint
> > > program, GLenum pname,
> > > case GL_TESS_GEN_MODE:
> > > if (!has_tess)
> > > break;
> > > - if (check_tes_query(ctx, shProg))
> > > - *params = shProg->TessEval.PrimitiveMode;
> > > + if (check_tes_query(ctx, shProg)) {
> > > + *params = shProg->_LinkedShaders[MESA_SHADER_TESS_EVAL]->
> > > + TessEval.PrimitiveMode;
> > > + }
> > > return;
> > > case GL_TESS_GEN_SPACING:
> > > if (!has_tess)
> > > break;
> > > - if (check_tes_query(ctx, shProg))
> > > - *params = shProg->TessEval.Spacing;
> > > + if (check_tes_query(ctx, shProg)) {
> > > + *params = shProg->_LinkedShaders[MESA_SHADER_TESS_EVAL]->
> > > + TessEval.Spacing;
> > > + }
> > > return;
> > > case GL_TESS_GEN_VERTEX_ORDER:
> > > if (!has_tess)
> > > break;
> > > - if (check_tes_query(ctx, shProg))
> > > - *params = shProg->TessEval.VertexOrder;
> > > + if (check_tes_query(ctx, shProg)) {
> > > + *params = shProg->_LinkedShaders[MESA_SHADER_TESS_EVAL]->
> > > + TessEval.VertexOrder;
> > > + }
> > > return;
> > > case GL_TESS_GEN_POINT_MODE:
> > > if (!has_tess)
> > > break;
> > > - if (check_tes_query(ctx, shProg))
> > > - *params = shProg->TessEval.PointMode;
> > > + if (check_tes_query(ctx, shProg)) {
> > > + *params = shProg->_LinkedShaders[MESA_SHADER_TESS_EVAL]->
> > > + TessEval.PointMode;
> > > + }
> > > return;
> > > default:
> > > break;
> > > @@ -2161,10 +2169,13 @@
> > > _mesa_copy_linked_program_data(gl_shader_stage type,
> > > case MESA_SHADER_TESS_EVAL: {
> > > struct gl_tess_eval_program *dst_tep =
> > > (struct gl_tess_eval_program *) dst;
> > Remove dst_tep, it is unused with these changes.
>
> Its still used :)
>
> >
> > >
> > > - dst_tep->PrimitiveMode = src->TessEval.PrimitiveMode;
> > > - dst_tep->Spacing = src->TessEval.Spacing;
> > > - dst_tep->VertexOrder = src->TessEval.VertexOrder;
> > > - dst_tep->PointMode = src->TessEval.PointMode;
> > > + struct gl_shader *tes_sh = src-
> > > >_LinkedShaders[MESA_SHADER_TESS_EVAL];
> > > + if (tes_sh) {
> > > + dst_tep->PrimitiveMode = tes_sh->TessEval.PrimitiveMode;
> > > + dst_tep->Spacing = tes_sh->TessEval.Spacing;
> > > + dst_tep->VertexOrder = tes_sh->TessEval.VertexOrder;
> > > + dst_tep->PointMode = tes_sh->TessEval.PointMode;
> > > + }
> > > dst->ClipDistanceArraySize = src-
> > > >TessEval.ClipDistanceArraySize;
> > > dst->CullDistanceArraySize = src-
> > > >TessEval.CullDistanceArraySize;
> > > break;
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
More information about the mesa-dev
mailing list