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

[Timothy Arceri]

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.


> +      /* 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.

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.



> +                _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;
>