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.
>
> 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 :)
> 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
