On Wed, 2016-06-22 at 09:30 +0200, Iago Toral wrote: > 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 already did it :) https://patchwork.freedesktop.org/patch/94481/ > > > > > > > > > > > > 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@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
