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

[Nicolai Hähnle]

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.


> 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


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.