[Mesa-dev] [PATCH 3/3] mesa: add more draw time validation for separate shader programs

[Timothy Arceri]

On Tue, 2015-12-08 at 08:11 +0200, Tapani Pälli wrote:
> On 12/08/2015 07:15 AM, Timothy Arceri wrote:
> > On Mon, 2015-12-07 at 11:29 +0200, Tapani Pälli wrote:
> > > Validation checks that we do not have active shader stages that
> > > are
> > > not used by the pipeline.
> > > 
> > > This fixes a subtest in following CTS test:
> > > 	ES31-CTS.sepshaderobjs.StateInteraction
> > > 
> > > Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> > > ---
> > >   src/mesa/main/api_validate.c | 38
> > > ++++++++++++++++++++++++++++++++++++++
> > >   1 file changed, 38 insertions(+)
> > > 
> > > diff --git a/src/mesa/main/api_validate.c
> > > b/src/mesa/main/api_validate.c
> > > index d693ec6..6a0cbb0 100644
> > > --- a/src/mesa/main/api_validate.c
> > > +++ b/src/mesa/main/api_validate.c
> > > @@ -47,6 +47,44 @@ check_valid_to_render(struct gl_context *ctx,
> > > const char *function)
> > >   
> > >      switch (ctx->API) {
> > >      case API_OPENGLES2:
> > When moving this to pipline validation as per my previous email
> > this
> > also needs to be left enabled for desktop GL too as this rule is in
> > the
> > 4.5 core spec also.
> 
> OK cool, will enable for both.
> 
> > 
> > > +      /* If SSO in use, validate that all linked program stages
> > > are
> > > used by
> > > +       * the current pipeline.
> > > +       *
> > > +       * OpenGL ES 3.1 spec (11.1.3.11 Validation):
> > > +       *
> > > +       *    "An INVALID_OPERATION error is generated by any
> > > command
> > > that trans-
> > > +       *    fers vertices to the GL or launches compute work if
> > > the
> > > current set
> > > +       *    of active program objects cannot be executed, for
> > > reasons including:
> > > +       *
> > > +       *    ...
> > > +       *
> > > +       *    "A program object is active for at least one, but
> > > not
> > > all of
> > > +       *    the shader stages that were present when the program
> > > was
> > > linked."
> > > +       *
> > > +       */
> > > +      if (ctx->Pipeline.Current) {
> > > +          struct gl_pipeline_object *pipe = ctx
> > > ->Pipeline.Current;
> > > +          unsigned i;
> > > +          for (i = 0; i < MESA_SHADER_STAGES; i++) {
> > > +             if (!pipe->CurrentProgram[i])
> > > +                continue;
> > > +
> > > +             /* Check that each active stage of linked program
> > > is
> > > used.
> > > +              * This is done by comparing ActiveStages masks of
> > > program and
> > > +              * pipeline. Program mask must not contain active
> > > stages that
> > > +              * are not marked used by the pipeline.
> > > +              */
> > > +             struct gl_shader_program *shProg = pipe
> > > ->CurrentProgram[i];
> > > +             if (((pipe->ActiveStages & shProg->ActiveStages) ^
> > > +                 shProg->ActiveStages) != 0) {
> > The problem that I see here is that this will test to see that all
> > stages that exist in the program are used in the pipeline, however
> > it
> > doesn't check that the stages used are the stages from the program
> > the
> > could be from another program.
> > 
> > For example the pipeline could have a program with both vert and
> > frag
> > shaders where only the vert shader is marked as used and another
> > program just with a frag shader which is marked as used. In which
> > case
> > this patch would pass when it should in fact fail.
> 
> Right, did not spot this as this is something that is not tested by
> CTS. 

If its also not tested by dEQP tests then maybe we should write a
piglit test. Seems like something that could easily break.

> But we do have pointers to programs from pipeline so I *think* this 
> check could be added here.
> 
> > I believe the intention of this validation is to make sure a
> > combination like the scenario above is not used as when the vert
> > and
> > frag shader are linked in the first program then any inactive
> > varyings
> > will be optimised away which the second program containing the frag
> > shader might have been expecting.
> 
> It also does not make sense to have linked stage that is not used.
> I'm 
> currently rewriting the interface match check 'validate_io' which is
> the 
> final check that everything is ok with varyings.
> 
> > 
> > 
> > > +                _mesa_error(ctx, GL_INVALID_OPERATION,
> > > +                            "Shader program active for shader
> > > stages
> > > that "
> > > +                            "are not used by the pipeline");
> > > +                return false;
> > > +             }
> > > +          }
> > > +      }
> > > +
> > >         /* For ES2, we can draw if we have a vertex
> > > program/shader).
> > > */
> > >         return ctx->VertexProgram._Current != NULL;
> > >   
>