[Mesa-dev] [PATCH 17/17] glsl: Don't monkey about with the interpolation modes

[Kenneth Graunke]

On Wednesday, August 24, 2016 3:12:43 PM PDT Ian Romanick wrote:
> From: Ian Romanick <ian.d.romanick at intel.com>
> 
> Previously we'd munge the interpolation mode so that later checks in the
> GLSL linker would pass.  The caused problems for similar checks in SSO
> IO validation.  Instead, make the check smarter, use the same check in
> both places, and don't modify the interpolation mode.
> 
> Fixes piglit tests:
> 
>     oes_geometry_shader/sso_validation/user-defined-gs-input-in-block
>     oes_geometry_shader/sso_validation/user-defined-gs-input-not-in-block
> 
> v2: Don't apply none-smooth or smooth-none matching in desktop GL at
> link time.  Suggested by Timothy Arceri.
> 
> v3: Simplify the checks.  Only allow SMOOTH/NONE matching when the
> consumer stage is fragment.
> 
> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96358
> Cc: "12.0" <mesa-stable at lists.freedesktop.org>
> Cc: Gregory Hainaut <gregory.hainaut at gmail.com>
> Cc: Ilia Mirkin <imirkin at alum.mit.edu>
> ---
>  src/compiler/glsl/ast_to_hir.cpp    | 11 -----------
>  src/compiler/glsl/link_varyings.cpp | 39 +++++++++++++++++++++++++++++++++----
>  src/compiler/glsl/link_varyings.h   |  6 ++++++
>  src/mesa/main/shader_query.cpp      |  5 ++++-
>  4 files changed, 45 insertions(+), 16 deletions(-)
> 
> diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp
> index 581367b..19282ef 100644
> --- a/src/compiler/glsl/ast_to_hir.cpp
> +++ b/src/compiler/glsl/ast_to_hir.cpp
> @@ -3023,17 +3023,6 @@ interpret_interpolation_qualifier(const struct ast_type_qualifier *qual,
>        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;
>  
> diff --git a/src/compiler/glsl/link_varyings.cpp b/src/compiler/glsl/link_varyings.cpp
> index 1bce3e0..0831492 100644
> --- a/src/compiler/glsl/link_varyings.cpp
> +++ b/src/compiler/glsl/link_varyings.cpp
> @@ -201,6 +201,34 @@ anonymous_struct_type_matches(const glsl_type *output_type,
>             to_match->record_compare(output_type);
>  }
>  
> +bool
> +interpolation_compatible(gl_shader_stage producer_stage,
> +                         gl_shader_stage consumer_stage,
> +                         enum glsl_interp_mode producer_interp,
> +                         enum glsl_interp_mode consumer_interp)
> +{
> +   /* Section 4.5 (Interpolation Qualifiers) of the GLSL ES 3.20 spec says:
> +    *
> +    *    When no interpolation qualifier is present, smooth interpolation is
> +    *    used.
> +    *
> +    * For the purpose of this function, use the ES rule even on desktop.  Link
> +    * validation for desktop will use a stronger check, but SSO IO validation
> +    * will use this check.  This allows using an undecorated output with a
> +    * "smooth" decorated input, for example.  There is no specific
> +    * justification for this in the OpenGL spec, but applications are known to
> +    * depend on this.
> +    */
> +   if (producer_interp == consumer_interp)
> +      return true;
> +
> +   return consumer_stage == MESA_SHADER_FRAGMENT &&
> +          ((producer_interp == INTERP_MODE_NONE &&
> +            consumer_interp == INTERP_MODE_SMOOTH) ||
> +           (producer_interp == INTERP_MODE_SMOOTH &&
> +            consumer_interp == INTERP_MODE_NONE));
> +}
> +
>  /**
>   * Validate the types and qualifiers of an output from one stage against the
>   * matching input to another stage.
> @@ -347,8 +375,12 @@ cross_validate_types_and_qualifiers(struct gl_shader_program *prog,
>      *     qualifiers of variables of the same name do not match.
>      *
>      */
> -   if (input->data.interpolation != output->data.interpolation &&
> -       prog->Version < 440) {
> +   if ((prog->IsES &&
> +        !interpolation_compatible(producer_stage, consumer_stage,
> +                                  glsl_interp_mode(output->data.interpolation),
> +                                  glsl_interp_mode(input->data.interpolation))) ||
> +       (!prog->IsES && prog->Version < 440 &&
> +        input->data.interpolation != output->data.interpolation)) {
>        linker_error(prog,
>                     "%s shader output `%s' specifies %s "
>                     "interpolation qualifier, "
> @@ -1390,8 +1422,7 @@ varying_matches::record(ir_variable *producer_var, ir_variable *consumer_var)
>        (producer_var->type->contains_integer() ||
>         producer_var->type->contains_double());
>  
> -   if (needs_flat_qualifier ||
> -       (consumer_stage != -1 && consumer_stage != MESA_SHADER_FRAGMENT)) {
> +   if (needs_flat_qualifier) {

The rest of this patch looks good, but I'm a bit afraid that this hunk
will break varying packing.  Currently it marks all varyings as flat
(as long as the FS isn't involved).  I guess that's probably bad for
ARB_program_interface_query.  And...since varying packing actually makes
new variables...it'll totally break ARB_piq.  I'm not sure if there's
anything we can do about it.

Another reason to ditch lower_packed_varyings ASAP.

The series is:
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
(with some reluctance on this patch)

What do you think about dropping the Cc stable on this patch?

--Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160824/1cd4de3a/attachment.sig>