[Mesa-dev] [PATCH 17/17] glsl: Don't monkey about with the interpolation modes
Timothy Arceri
timothy.arceri at collabora.com
Thu Aug 25 00:06:30 UTC 2016
On Thu, 2016-08-25 at 10:00 +1000, Timothy Arceri wrote:
> On Wed, 2016-08-24 at 15:12 -0700, 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.
>
> Are you sure this is ok? Previously we would require both to
> be INTERP_MODE_NONE and the backend would change them to smooth later
> unless glShadeModel() is set in which case they could be set to flat.
>
> So now we will allow a case were one stage is NONE, one stage is
> SMOOTH
> and glShadeModel() is flat. Previously this would fail validation,
> now
> it would pass and give a user results they might not be expecting.
To save some back and forth if you do think thats ok, and you address
my comment below. Then feel free to add my r-b to 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) {
>
>
> If we no longer set consumer here then you should also remove the if
> (consumer_var) { ... } below as it will now never be called.
>
>
>
> >
> > /* Since this varying is not being consumed by the fragment
> > shader, its
> > * interpolation type varying cannot possibly affect
> > rendering.
> > * Also, this variable is non-flat and is (or contains) an
> > integer
> > diff --git a/src/compiler/glsl/link_varyings.h
> > b/src/compiler/glsl/link_varyings.h
> > index afce56e..075bb72 100644
> > --- a/src/compiler/glsl/link_varyings.h
> > +++ b/src/compiler/glsl/link_varyings.h
> > @@ -341,4 +341,10 @@ check_against_input_limit(struct gl_context
> > *ctx,
> > gl_linked_shader *consumer,
> > unsigned num_explicit_locations);
> >
> > +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);
> > +
> > #endif /* GLSL_LINK_VARYINGS_H */
> > diff --git a/src/mesa/main/shader_query.cpp
> > b/src/mesa/main/shader_query.cpp
> > index 0eae39a..a0c764a 100644
> > --- a/src/mesa/main/shader_query.cpp
> > +++ b/src/mesa/main/shader_query.cpp
> > @@ -37,6 +37,7 @@
> > #include "compiler/glsl/glsl_symbol_table.h"
> > #include "compiler/glsl/ir.h"
> > #include "compiler/glsl/program.h"
> > +#include "compiler/glsl/link_varyings.h"
> > #include "program/hash_table.h"
> > #include "util/strndup.h"
> >
> > @@ -1611,7 +1612,9 @@ validate_io(struct gl_shader_program
> > *producer,
> > }
> > }
> >
> > - if (producer_var->interpolation != consumer_var-
> > >
> > > interpolation) {
> > + if (!interpolation_compatible(producer_stage,
> > consumer_stage,
> > + glsl_interp_mode(producer_var-
> > >
> > > interpolation),
> > + glsl_interp_mode(consumer_var-
> > >
> > > interpolation))) {
> > valid = false;
> > goto out;
> > }
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list