[Mesa-dev] [PATCH] glsl/mesa: stop duplicating tess layout values
Iago Toral
itoral at igalia.com
Tue Jun 21 15:45:30 UTC 2016
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.
I have a few minor comments below, with those fixed you can add my Rb to
this patch, 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.
> ---
> 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.
> - 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.
> -
> - 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.
> - 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;
More information about the mesa-dev
mailing list