[Mesa-dev] [PATCH 8/8] glsl/linker: Ensure the first stage of an SSO pipeline has input locs assigned

[Timothy Arceri]

On Wed, 2016-05-18 at 20:44 +1000, Timothy Arceri wrote:
> On Tue, 2016-05-17 at 20:24 -0700, Ian Romanick wrote:
> > 
> > On 05/17/2016 04:40 PM, Timothy Arceri wrote:
> > > 
> > > 
> > > On Tue, 2016-05-17 at 15:11 -0700, Ian Romanick wrote:
> > > > 
> > > > 
> > > > From: Ian Romanick <ian.d.romanick at intel.com>
> > > > 
> > > > Previously an SSO pipeline containing only a tessellation
> > > > control
> > > > shader
> > > > and a tessellation evaluation shader would not get locations
> > > > assigned
> > > > for the TCS inputs.  This would lead to assertion failures in
> > > > some
> > > > piglit tests, such as arb_program_interface_query-resource-
> > > > query.
> > > > 
> > > > That piglit test still fails on some tessellation related
> > > > subtests.
> > > > Specifically, these subtests fail:
> > > > 
> > > > 'GL_PROGRAM_INPUT(tcs) active resources' expected 2 but got 3
> > > > 'GL_PROGRAM_INPUT(tcs) max length name' expected 12 but got 16
> > > > 'GL_PROGRAM_INPUT(tcs,tes) active resources' expected 2 but got
> > > > 3
> > > > 'GL_PROGRAM_INPUT(tcs,tes) max length name' expected 12 but got
> > > > 16
> > > > 'GL_PROGRAM_OUTPUT(tcs) active resources' expected 15 but got 3
> > > > 'GL_PROGRAM_OUTPUT(tcs) max length name' expected 23 but got 12
> > > > 
> > > > Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> > > > ---
> > > >  src/compiler/glsl/linker.cpp | 9 ++++++---
> > > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/src/compiler/glsl/linker.cpp
> > > > b/src/compiler/glsl/linker.cpp
> > > > index 34b4a81..d277dd0 100644
> > > > --- a/src/compiler/glsl/linker.cpp
> > > > +++ b/src/compiler/glsl/linker.cpp
> > > > @@ -4790,7 +4790,7 @@ link_shaders(struct gl_context *ctx,
> > > > struct
> > > > gl_shader_program *prog)
> > > >            */
> > > >           int next = last;
> > > >           for (int i = next - 1; i >= 0; i--) {
> > > > -            if (prog->_LinkedShaders[i] == NULL)
> > > > +            if (prog->_LinkedShaders[i] == NULL && i != 0)
> > > Up to you but I think i != MESA_SHADER_VERTEX makes it slightly
> > > easier
> > > to follow.
> > I'm a little bit torn.  I thought about using MESA_SHADER_VERTEX,
> > but
> > that's not really the information that's important.  What's
> > important
> > is
> > always executing at the loop bound (which is 0) even when there's
> > no
> > shader there.
> In that case I don't think this won't work as intended. The
> _LinkedShaders array is defined as:
> 
>    _LinkedShaders[MESA_SHADER_STAGES]
> 
> Here a zero index is the first GL stage not the first stage in the
> program. So a program containing only geom and frag stages would hit
> the same problem.
> 
> I think we need to do something like what is done with the last stage
> above this code e.g.
> 
>    if (first < MESA_SHADER_VERTEX && prog->SeparateShader) {

Whoops that should be first > MESA_SHADER_VERTEX

>       if (!assign_varying_locations(...))
>          goto done;
>    }
> 
> I wonder if that code should be calling check_against_output_limit()
> also. I guess we probably have no SSO tests that exceed the limit.
> 
> > 
> > 
> > I think I'd want to either leave both i comparisons with zero or
> > change
> > both to MESA_SHADER_VERTEX.  Do you have an opinion?
> > 
> > > 
> > > 
> > > Otherwise the series is Reviewed-by: Timothy Arceri
> > > <timothy.arceri at collabora.com>
> > > 
> > > 
> > > > 
> > > > 
> > > >                 continue;
> > > >  
> > > >              gl_shader *const sh_i = prog->_LinkedShaders[i];
> > > > @@ -4806,8 +4806,11 @@ link_shaders(struct gl_context *ctx,
> > > > struct
> > > > gl_shader_program *prog)
> > > >                        tfeedback_decls);
> > > >  
> > > >              /* This must be done after all dead varyings are
> > > > eliminated. */
> > > > -            if (!check_against_output_limit(ctx, prog, sh_i))
> > > > -               goto done;
> > > > +            if (sh_i != NULL) {
> > > > +               if (!check_against_output_limit(ctx, prog,
> > > > sh_i))
> > > > {
> > > > +                  goto done;
> > > > +               }
> > > > +            }
> > > >              if (!check_against_input_limit(ctx, prog,
> > > > sh_next))
> > > >                 goto done;
> > > >  
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev