[Mesa-dev] [PATCH 7/7] mesa: validate precision of varyings during ValidateProgramPipeline

[Iago Toral]

On Tue, 2015-11-10 at 12:10 +0100, Iago Toral wrote:
> On Thu, 2015-11-05 at 13:33 +0200, Tapani Pälli wrote:
> > Fixes following failing ES3.1 CTS tests:
> > 
> >    ES31-CTS.sepshaderobjs.InterfacePrecisionMatchingFloat
> >    ES31-CTS.sepshaderobjs.InterfacePrecisionMatchingInt
> >    ES31-CTS.sepshaderobjs.InterfacePrecisionMatchingUInt
> > 
> > Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> > ---
> >  src/mesa/main/pipelineobj.c    | 15 ++++++++++++
> >  src/mesa/main/shader_query.cpp | 55 ++++++++++++++++++++++++++++++++++++++++++
> >  src/mesa/main/shaderobj.h      |  3 +++
> >  3 files changed, 73 insertions(+)
> > 
> > diff --git a/src/mesa/main/pipelineobj.c b/src/mesa/main/pipelineobj.c
> > index 699a2ae..90dff13 100644
> > --- a/src/mesa/main/pipelineobj.c
> > +++ b/src/mesa/main/pipelineobj.c
> > @@ -907,6 +907,21 @@ _mesa_ValidateProgramPipeline(GLuint pipeline)
> >  
> >     _mesa_validate_program_pipeline(ctx, pipe,
> >                                     (ctx->_Shader->Name == pipe->Name));
> > +
> > +   /* Validate inputs against outputs, this cannot be done during linking
> > +    * since programs have been linked separately from each other.
> > +    *
> > +    * From OpenGL 4.5 Core spec:
> > +    *     "Separable program objects may have validation failures that cannot be
> > +    *     detected without the complete program pipeline. Mismatched interfaces,
> > +    *     improper usage of program objects together, and the same
> > +    *     state-dependent failures can result in validation errors for such
> > +    *     program objects."
> > +    *
> > +    * OpenGL ES 3.1 specification has the same text.
> > +    */
> > +   if (!_mesa_validate_pipeline_io(pipe))
> > +      pipe->Validated = GL_FALSE;
> >  }
> >  
> >  void GLAPIENTRY
> > diff --git a/src/mesa/main/shader_query.cpp b/src/mesa/main/shader_query.cpp
> > index 5cb877b..595bdea 100644
> > --- a/src/mesa/main/shader_query.cpp
> > +++ b/src/mesa/main/shader_query.cpp
> > @@ -1359,3 +1359,58 @@ _mesa_get_program_resourceiv(struct gl_shader_program *shProg,
> >     if (length)
> >        *length = amount;
> >  }
> > +
> > +static bool
> > +validate_io(const struct gl_shader *input_stage,
> > +            const struct gl_shader *output_stage)
> > +{
> > +   assert(input_stage && output_stage);
> 
> Maybe add the relevant spec quote for this:
> 
> "When both shaders are in separate programs, mismatched precision 
>  qualifiers will result in a program interface mismatch that will 
>  result in program pipeline  validation failures"

Sorry, this was cut, the complete text reads:

"When both shaders are in separate programs, mismatched precision
qualifiers will result in a program interface mismatch that will result
in program pipeline validation failures, as described in section 7.4.1
(“Shader Interface Matching”) of the OpenGL ES 3.1 Specification."

This text is from chapter "4.7.3. Precision Qualifiers" of the OpenGL ES
3.1 spec.

> > +   /* For each output in a, find input in b and do any required checks. */
> 
> s/a/input_stage
> s/b/output_stage
> 
> > +   foreach_in_list(ir_instruction, out, input_stage->ir) {
> > +      ir_variable *out_var = out->as_variable();
> > +      if (!out_var || out_var->data.mode != ir_var_shader_out)
> > +         continue;
> > +
> > +      foreach_in_list(ir_instruction, in, output_stage->ir) {
> > +         ir_variable *in_var = in->as_variable();
> > +         if (!in_var || in_var->data.mode != ir_var_shader_in)
> > +            continue;
> > +
> > +         if (strcmp(in_var->name, out_var->name) == 0) {
> > +            if (in_var->data.precision != out_var->data.precision)
> > +               return false;
> > +         }
> > +      }
> 
> This is O(n²) but we can easily make it O(n) by moving the inner loop
> outside and putting the input variables we find there in a hash table.
> Then the loop over input_stage just needs to do hash table lookups
> instead of looping for each output variable it has. What do you think?
> 
> Iago
> 
> > +   }
> > +   return true;
> > +}
> > +
> > +/**
> > + * Validate inputs against outputs in a program pipeline.
> > + */
> > +extern "C" bool
> > +_mesa_validate_pipeline_io(struct gl_pipeline_object *pipeline)
> > +{
> > +   struct gl_shader_program **shProg =
> > +      (struct gl_shader_program **) pipeline->CurrentProgram;
> > +
> > +   /* Find first active stage in pipeline. */
> > +   unsigned idx, prev = 0;
> > +   for (idx = 0; idx < ARRAY_SIZE(pipeline->CurrentProgram); idx++) {
> > +      if (shProg[idx]) {
> > +         prev = idx;
> > +         break;
> > +      }
> > +   }
> > +
> > +   for (idx = prev + 1; idx < ARRAY_SIZE(pipeline->CurrentProgram); idx++) {
> > +      if (shProg[idx]) {
> > +         if (!validate_io(shProg[prev]->_LinkedShaders[prev],
> > +                          shProg[idx]->_LinkedShaders[idx]))
> > +            return false;
> > +         prev = idx;
> > +      }
> > +   }
> > +   return true;
> > +}
> > diff --git a/src/mesa/main/shaderobj.h b/src/mesa/main/shaderobj.h
> > index 796de47..be80752 100644
> > --- a/src/mesa/main/shaderobj.h
> > +++ b/src/mesa/main/shaderobj.h
> > @@ -234,6 +234,9 @@ _mesa_shader_stage_to_subroutine_uniform(gl_shader_stage stage)
> >     }
> >  }
> >  
> > +extern bool
> > +_mesa_validate_pipeline_io(struct gl_pipeline_object *);
> > +
> >  #ifdef __cplusplus
> >  }
> >  #endif
>