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

[Tapani Pälli]

On 11/10/2015 01:15 PM, Iago Toral wrote:
> 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.

OK, I'll add spec quote.

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

I'm not sure if it's worth the trouble but I can take a try. We would 
need to also destroy hash which will take some time.

I guess in long run this should be implemented by iterating program 
resource list and maybe those should be also split to resource pools 
instead of one single list. I chose single list originally because 
typically that list is quite small (we are talking about ~10 to 20, not 
hundreds) and there are api calls that iterate through all resources at 
once. However I would like to do that change only when quest for GLES 
3.1 is over.


>> 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
>>
>
>