[Mesa-dev] [PATCH 1/3] glsl/linker: fix varying packing for non-flat integer varyings.

[Paul Berry]

On 7 April 2013 12:01, Jordan Justen <jljusten at gmail.com> wrote:

> On Sun, Apr 7, 2013 at 11:42 AM, Paul Berry <stereotype441 at gmail.com>
> wrote:
> > On 7 April 2013 11:12, Jordan Justen <jljusten at gmail.com> wrote:
> >>
> >> On Sat, Apr 6, 2013 at 7:49 PM, Paul Berry <stereotype441 at gmail.com>
> >> wrote:
> >> > +   if (consumer_var == NULL) {
> >> > +      /* Since there is no consumer_var, the interpolation type of
> this
> >> > +       * varying cannot possibly affect rendering.  Also, since the
> GL
> >> > spec
> >> > +       * only requires integer varyings to be "flat" when they are
> >> > fragment
> >> > +       * shader inputs, it is possible that this variable is non-flat
> >> > and is
> >> > +       * (or contains) an integer.
> >> > +       *
> >> > +       * lower_packed_varyings requires all integer varyings to flat,
> >> > +       * regardless of where they appear.  We can trivially satisfy
> >> > that
> >> > +       * requirement by changing the interpolation type to flat here.
> >> > +       */
> >> > +      producer_var->centroid = false;
> >> > +      producer_var->interpolation = INTERP_QUALIFIER_FLAT;
> >>
> >> Should we check if producer_var's type is an integer?
> >
> > Yeah, that's a good point.  It shouldn't affect correctness,
>
> It isn't possible to have a non-integer scenario of this where we
> wouldn't want to override the interpolation type?
>

Hmm, I can't think of any such scenario.  It seems to me that as long as
consumer_var == NULL, that means that the varying isn't being consumed by
the fragment shader, so it isn't getting interpolated (or if it is, the
interpolation result is being ignored).  So the only reason the
interpolation type should matter at this point is in its effect on
lower_packed_varyings.  Of course I could still be missing something :)

I'll still go ahead and add the int check just to be on the safe side.


>
> > but it will
> > make the patch less invasive, and that seems worthwhile since I'm hoping
> > this can be cherry-picked back to 9.1.  I'll follow up with a v2 of the
> > series.
> >
> >> Could this cause issues if the producer stage is later linked to
> >> another consumer?
> >
> > That shouldn't be a problem, since at this stage of linking, we are
> > operating on a copy of the IR that is private to this particular linked
> > program.
>
> Okay, well, based on that and adding the int check above, for the series:
> Reviewed-by: Jordan Justen <jordan.l.justen at intel.com>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130407/92092095/attachment.html>