[Mesa-dev] [PATCH] glsl: do not set the 'smooth' qualifier by default on ES shaders

[Timothy Arceri]

On 12/09/17 00:57, Nicolai Hähnle wrote:
> On 11.09.2017 16:47, Ilia Mirkin wrote:
>> On Mon, Sep 11, 2017 at 10:44 AM, Nicolai Hähnle <nhaehnle at gmail.com> 
>> wrote:
>>> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>>>
>>> It breaks integer inputs and outputs on vertex processing stages
>>> (e.g. geometry stages). Instead, rely on the driver to choose
>>> smooth interpolation by default.
>>> ---
>>>
>>> How about this instead? I haven't fully thought it through, but
>>> that's where the INTERP_MODE_SMOOTH is coming from.
>>>
>>> To be honest I'm not 100% sure about what INTERP_MODE_NONE can
>>> mean, but I think it's definitely better than removing the (very
>>> valid!) assertion in the packing code.
>>
>> I couldn't understand what that assertion was asserting, esp in the
>> presence of struct in/out variables.
> 
> structs can't have per-member interpolation qualifiers. So a struct that 
> contains integer values must have FLAT or NONE interpolation as a whole.
> 
> That clashes pretty hard with that ES rule, though ("When no 
> interpolation qualifier is present, smooth interpolation is used.").
 From what I recall, NONE is just a placeholder so that the the driver 
knows to use the value set by glShadeModel(). ES doesn't have this and 
so we set this up front. I believe removing it will cause a bunch of 
tests to fail validation as they expect the default to be set and cross 
validated against stage interfaces.

I'm also a bit confused as to what the assert() is trying to catch.

> 
> On second thought, that assertion is actually irrelevant for varying 
> packing itself, but it's still pretty disturbing that it's broken by 
> this particular shader.
> 
> Cheers,
> Nicolai
> 
> 
>>
>>>
>>> ---
>>>   src/compiler/glsl/ast_to_hir.cpp | 11 -----------
>>>   1 file changed, 11 deletions(-)
>>>
>>> diff --git a/src/compiler/glsl/ast_to_hir.cpp 
>>> b/src/compiler/glsl/ast_to_hir.cpp
>>> index 98d2f94e129..cb42041642d 100644
>>> --- a/src/compiler/glsl/ast_to_hir.cpp
>>> +++ b/src/compiler/glsl/ast_to_hir.cpp
>>> @@ -3119,31 +3119,20 @@ interpret_interpolation_qualifier(const 
>>> struct ast_type_qualifier *qual,
>>>                                     struct _mesa_glsl_parse_state 
>>> *state,
>>>                                     YYLTYPE *loc)
>>>   {
>>>      glsl_interp_mode interpolation;
>>>      if (qual->flags.q.flat)
>>>         interpolation = INTERP_MODE_FLAT;
>>>      else if (qual->flags.q.noperspective)
>>>         interpolation = INTERP_MODE_NOPERSPECTIVE;
>>>      else if (qual->flags.q.smooth)
>>>         interpolation = INTERP_MODE_SMOOTH;
>>> -   else if (state->es_shader &&
>>> -            ((mode == ir_var_shader_in &&
>>> -              state->stage != MESA_SHADER_VERTEX) ||
>>> -             (mode == ir_var_shader_out &&
>>> -              state->stage != MESA_SHADER_FRAGMENT)))
>>> -      /* Section 4.3.9 (Interpolation) of the GLSL ES 3.00 spec says:
>>> -       *
>>> -       *    "When no interpolation qualifier is present, smooth 
>>> interpolation
>>> -       *    is used."
>>> -       */
>>> -      interpolation = INTERP_MODE_SMOOTH;
>>>      else
>>>         interpolation = INTERP_MODE_NONE;
>>>
>>>      validate_interpolation_qualifier(state, loc,
>>>                                       interpolation,
>>>                                       qual, var_type, mode);
>>>
>>>      return interpolation;
>>>   }
>>>
>>> -- 
>>> 2.11.0
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
>