[Mesa-dev] [PATCH v2 2/3] mesa: validate program usage in a pipeline

[Timothy Arceri]

On Tue, 2015-12-08 at 11:16 +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
> 
> v2: move as generic validation check for both ES and desktop
> (Timothy)
> 
> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> ---
>  src/mesa/main/context.c     |  9 +++++++++
>  src/mesa/main/pipelineobj.c | 35 +++++++++++++++++++++++++++++++++++
>  src/mesa/main/pipelineobj.h |  2 ++
>  3 files changed, 46 insertions(+)
> 
> diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c
> index be983d4..d73c984 100644
> --- a/src/mesa/main/context.c
> +++ b/src/mesa/main/context.c
> @@ -2042,6 +2042,15 @@ _mesa_valid_to_render(struct gl_context *ctx,
> const char *where)
>        }
>     }
>  
> +   /* If SSO in use, validate that all linked program stages are
> used. */
> +   if (ctx->Pipeline.Current) {
> +      if (!_mesa_active_program_stages_used(ctx->Pipeline.Current))
> {
> +         _mesa_error(ctx, GL_INVALID_OPERATION,
> +                     "Shader program active for shader stages that "
> +                     "are not used by the pipeline");

You can remove this now. The call to _mesa_validate_program_pipeline()
below should cause the error to be thrown for us when it fails.

> +      }
> +   }
> +
>     /* If a program is active and SSO not in use, check if validation
> of
>      * samplers succeeded for the active program. */
>     if (ctx->_Shader->ActiveProgram && ctx->_Shader != ctx
> ->Pipeline.Current) {
> diff --git a/src/mesa/main/pipelineobj.c
> b/src/mesa/main/pipelineobj.c
> index f4ce3ed..9067551 100644
> --- a/src/mesa/main/pipelineobj.c
> +++ b/src/mesa/main/pipelineobj.c
> @@ -782,6 +782,21 @@ program_stages_interleaved_illegally(const
> struct gl_pipeline_object *pipe)
>     return false;
>  }
>  
> +bool
> +_mesa_active_program_stages_used(struct gl_pipeline_object *pipe)
> +{
> +   unsigned i;
> +   for (i = 0; i < MESA_SHADER_STAGES; i++) {
> +      if (!pipe->CurrentProgram[i])
> +         continue;
> +      /* If program has linked but not used by pipeline. */
> +      if (pipe->CurrentProgram[i]->LinkStatus &&
> +          !pipe->CurrentProgram[i]->ValidProgramUse)

Two things here first if ValidProgramUse is false we need to redo the
validation to be sure it really is an invalid use as its possible that
the program was relinked since the UseProgramStages call and is now
valid.

Second are you sure about the LinkStatus check? Won't this cause
validation to pass when it should fail if the program was linked
successfully but ValidProgramUse is false, and we then do a relink that
fails? As the program will stay in the pipeline.


Make that three things. It seems there is already code that should be
doing this validation already. The first validation test in
_mesa_validate_program_pipeline() calls program_stages_all_active() in
a loop which should fail for this case. The code pretty much does what
I was suggesting only it always does the validation, there is no flag
to reduce calls. I guess it really shouldn't matter a huge deal since
its only done once when somethings relinked or a sampler changes.

From my quick testing on the CTS test the program in the pipeline only
has a single shader when it should have two so the test fails with this
existing code. As per my first point I believe we always need to
revalidate here to be sure that it really is an invalid use of the
program so what we really need to do is track down why there is only a
single shader in the program when it seems there should be two. If we
can resolve that we might not need to rework this code unless there
really is a performance hit.


> +         return false;
> +   }
> +   return true;
> +}
> +
>  extern GLboolean
>  _mesa_validate_program_pipeline(struct gl_context* ctx,
>                                  struct gl_pipeline_object *pipe)
> @@ -839,6 +854,26 @@ _mesa_validate_program_pipeline(struct
> gl_context* ctx,
>        return GL_FALSE;
>     }
>  
> +   /* Section 11.1.3.11 (Validation) of the OpenGL 4.5 and OpenGL ES
> 3.1
> +    * spec say:
> +    *
> +    *     "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 (!_mesa_active_program_stages_used(pipe)) {
> +      pipe->InfoLog =
> +         ralloc_strdup(pipe,
> +                       "Shader program active for shader stages that
> "
> +                       "are not used by the pipeline");
> +      return GL_FALSE;
> +   }
> +
>     /* Section 2.11.11 (Shader Execution), subheading "Validation,"
> of the
>      * OpenGL 4.1 spec says:
>      *
> diff --git a/src/mesa/main/pipelineobj.h
> b/src/mesa/main/pipelineobj.h
> index fbcb765..e56b522 100644
> --- a/src/mesa/main/pipelineobj.h
> +++ b/src/mesa/main/pipelineobj.h
> @@ -70,6 +70,8 @@ extern GLboolean
>  _mesa_validate_program_pipeline(struct gl_context * ctx,
>                                  struct gl_pipeline_object *pipe);
>  
> +extern bool
> +_mesa_active_program_stages_used(struct gl_pipeline_object *pipe);
>  
>  extern void GLAPIENTRY
>  _mesa_UseProgramStages(GLuint pipeline, GLbitfield stages, GLuint
> program);