[Mesa-dev] [PATCH 04/10] mesa: Check isES before calling validate_io

[Ian Romanick]

On 05/20/2016 05:46 PM, Timothy Arceri wrote:
> On Fri, 2016-05-20 at 00:25 -0700, Ian Romanick wrote:
>> From: Ian Romanick <ian.d.romanick at intel.com>
>>
>> There's going to be a second validate_io function, and checking the
>> same
>> thing twice is silly.
> 
> I think we should just do this check
> in _mesa_validate_program_pipeline() before we
> call _mesa_validate_pipeline_io() otherwise we end up looping over the
> shaders at draw time for desktop when there is no need to.
> 
> The comment above that call quotes 
> 
> "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."
> 
> However I think this is a spec bug I filed https://cvs.khronos.org/bugz
> illa/show_bug.cgi?id=15331 for it.

I think the spec is correct.  The key phrase is "can result."  This
leaves the possibility for hardware that would crash with certain types
of mismatches to reject them with a GL error at draw time.  I don't
think any desktop hardware operates like this, but I think some mobile
hardware might.  The ES group opted for uniformity among implementations
by requiring everyone to reject the same stuff... even if it could "just
work."

The thing that's a little bit weird about this code, and makes me
nervous about changing it too much, is that the checks depend on the ES
/ non-ES state of the shaders, not the API.  I'd like to understand why
that was done in the first place before mucking with it... the change
you suggest may still be the right thing to do, I'm just not 100% sure.

>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>> ---
>>  src/mesa/main/shader_query.cpp | 16 +++++++---------
>>  1 file changed, 7 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/mesa/main/shader_query.cpp
>> b/src/mesa/main/shader_query.cpp
>> index 9e18a1c..a120cb4 100644
>> --- a/src/mesa/main/shader_query.cpp
>> +++ b/src/mesa/main/shader_query.cpp
>> @@ -1372,7 +1372,7 @@ _mesa_get_program_resourceiv(struct
>> gl_shader_program *shProg,
>>  
>>  static bool
>>  validate_io(const struct gl_shader *producer,
>> -            const struct gl_shader *consumer, bool isES)
>> +            const struct gl_shader *consumer)
>>  {
>>     assert(producer && consumer);
>>     unsigned inputs = 0, outputs = 0;
>> @@ -1416,10 +1416,6 @@ validate_io(const struct gl_shader *producer,
>>      * packing makes this challenging.
>>      */
>>  
>> -   /* Currently no matching done for desktop. */
>> -   if (!isES)
>> -      return true;
>> -
>>     /* For each output in a, find input in b and do any required
>> checks. */
>>     foreach_in_list(ir_instruction, out, producer->ir) {
>>        ir_variable *out_var = out->as_variable();
>> @@ -1501,10 +1497,12 @@ _mesa_validate_pipeline_io(struct
>> gl_pipeline_object *pipeline)
>>           if (shProg[idx]->_LinkedShaders[idx]->Stage ==
>> MESA_SHADER_COMPUTE)
>>              break;
>>  
>> -         if (!validate_io(shProg[prev]->_LinkedShaders[prev],
>> -                          shProg[idx]->_LinkedShaders[idx],
>> -                          shProg[prev]->IsES || shProg[idx]->IsES))
>> -            return false;
>> +         if (shProg[prev]->IsES || shProg[idx]->IsES) {
>> +            if (!validate_io(shProg[prev]->_LinkedShaders[prev],
>> +                             shProg[idx]->_LinkedShaders[idx]))
>> +               return false;
>> +         }
>> +
>>           prev = idx;
>>        }
>>     }
>