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

[Timothy Arceri]

On 14/09/17 01:22, Nicolai Hähnle wrote:
> On 12.09.2017 03:33, Timothy Arceri wrote:
>>
>>
>> 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.
> 
> So in a way, NONE just means "use the API default". The API default is 
> set by glShadeModel() in compatibility profiles. In both ES and core 
> profiles it's always "smooth" -- but only ES has this particular 
> language about using "smooth" by default in its shading language spec.
> 
> Am I really the only one weirded out by seeing integer I/O that claims 
> to use smooth interpolation?
> 
> I'd much rather not set smooth, as my patch proposes, and if there are 
> tests that trigger linking errors, relax those to allow "smooth" and 
> "none" to match.

They are CTS tests. Feel free to do it an alternative way but I don't 
think this patch alone is enough, the tests seem valid enough to me.

> 
> 
>> I'm also a bit confused as to what the assert() is trying to catch.
> 
> It's catching the weirdness of integer I/O that claims to use smooth 
> interpolation. Admittedly, the comment is wrong and this is not related 
> to varying packing, but I guarantee that people will be confused about 
> this in the future.
> 
> Cheers,
> Nicolai
> 
> 
>>
>>>
>>> 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
>>>
>>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
>