[Mesa-dev] [PATCH] Revert "i965: get PrimitiveMode from the program rather than the shader struct"

[Timothy Arceri]

On Wed, 2016-06-29 at 17:23 +0300, Andres Gomez wrote:
> On Thu, 2016-06-30 at 00:09 +1000, Timothy Arceri wrote:
> > 
> > On 29 June 2016 11:29:17 pm AEST, Andres Gomez <agomez at igalia.com>
> > wrote:
> > > This reverts commit 644e015f0b9236e955d679cac4bcc7a1523fc475.
> > > 
> > > PrimitiveMode from the program doesn't hold the proper value when
> > > reaching this code. We rather take it from the linked shader.
> > 
> > Usually when reverting you should say what the commit broke or what
> > you are fixing.
> 
> Yes, I should probably have been more verbose here. The original
> patch
> caused regressions in the following CTS tests:
>  * GL44-CTS.stencil_texturing.functional
>  * GL44-CTS.shading_language_420pack.binding_images
>  * GL44-CTS.shading_language_420pack.binding_samplers
>  * GL44-CTS.shading_language_420pack.binding_uniform_single_block
>  * GL44-CTS.shading_language_420pack.implicit_conversions
>  * GL44-CTS.shading_language_420pack.initializer_list
>  * GL44-CTS.shading_language_420pack.length_of_vector_and_matrix
>  * GL44-CTS.shading_language_420pack.line_continuation
> 
> All were SIGABRTing with something like:
> glcts: brw_shader.cpp:613: unsigned int
> tesslevel_outer_components(GLenum): Assertion `!"Bogus tessellation
> domain"' failed.
> 
> The reason is that the PrimitiveMode used holds a value that is
> neither
> of GL_TRIANGLES, GL_QUADS nor GL_ISOLINES.

Ok. Please include some of that in the commit message. In the end I
needed to cache this anyway. I got rid of the duplication in
gl_shader_program instead so you can have a:

Reviewed-by: Timothy Arceri <timothy.arceri at collabora.com>

It's still concerning that the value in gl_program is wrong but I don't
think we actually use it anywhere else so I guess we dont need to
worry.

> 
> > I'll take another look tomorrow but my feeling is program should
> > have
> > the correct value here. In my testing it seemed to be ok. Can you
> > explain why the value is different? 
> 
> The patch is a quick revert to the original behavior, I didn't invest
> enough time to know why brw->tess_eval_program->PrimitiveMode didn't
> have the expected value by the time that code is reached.
> 
> Thanks for taking a look at this.
> 
> Br.
> 
> > 
> > Thanks.
> > > 
> > > Signed-off-by: Andres Gomez <agomez at igalia.com>
> > > ---
> > > src/mesa/drivers/dri/i965/brw_tcs.c | 5 +++--
> > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/mesa/drivers/dri/i965/brw_tcs.c
> > > b/src/mesa/drivers/dri/i965/brw_tcs.c
> > > index d488715..2029ea5 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_tcs.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_tcs.c
> > > @@ -395,8 +395,9 @@ brw_tcs_precompile(struct gl_context *ctx,
> > >          _LinkedShaders[MESA_SHADER_TESS_CTRL]-
> > > > TessCtrl.VerticesOut;
> > >    }
> > > 
> > > -   key.tes_primitive_mode = brw->tess_eval_program ?
> > > -      brw->tess_eval_program->PrimitiveMode : GL_TRIANGLES;
> > > +   key.tes_primitive_mode =
> > > shader_prog->_LinkedShaders[MESA_SHADER_TESS_EVAL]
> > > +      ?
> > > shader_prog->_LinkedShaders[MESA_SHADER_TESS_EVAL]-
> > > > TessEval.PrimitiveMode
> > > +      : GL_TRIANGLES;
> > > 
> > >    key.outputs_written = prog->OutputsWritten;
> > >    key.patch_outputs_written = prog->PatchOutputsWritten;
> > > -- 
> > > 2.8.1
> > > 
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >