[Mesa-dev] [PATCH v2 8/8] i965/gen6+: Add support for noperspective interpolation.

[Paul Berry]

On 26 October 2011 12:17, Eric Anholt <eric at anholt.net> wrote:

> On Tue, 25 Oct 2011 20:38:26 -0700, Paul Berry <stereotype441 at gmail.com>
> wrote:
> > This required the following changes:
> >
> > - WM setup now makes the appropriate set of barycentric coordinates
> >   (perspective vs. noperspective) available to the fragment shader,
> >   based on whether the shader requires perspective interpolation,
> >   noperspective interpolation, both, or neither.
> >
> > - The fragment shader backend now uses the appropriate set of
> >   barycentric coordiantes when interpolating, based on the
> >   interpolation mode returned by
> >   ir_variable::determine_interpolation_mode().
> >
> > - SF setup now uses _mesa_compute_flag_attributes_flat() to determine
> >   which attributes are to be flat shaded (as opposed to the old logic,
> >   which only flat shaded colors).
>
> _mesa_compute_flag_attributes_flat() doesn't appear to be a real thing.
>

Oops, it was a real thing in v1 of the series, and I forgot to change the
commit message.  I'll fix that.


>
> One code change comment below, then the whole v2 series is:
>
> Reviewed-by: Eric Anholt <eric at anholt.net>
>
> > - CLIP setup now ensures that the clipper outputs non-perspective
> >   barycentric coordinates when they are needed by the fragment shader.
> >
> > Fixes the remaining piglit tests of interpolation qualifiers that were
> > failing:
> > - interpolation-flat-*-smooth-none
> > - interpolation-flat-other-flat-none
> > - interpolation-noperspective-*
> > - interpolation-smooth-gl_*Color-flat-*
> > ---
> >  src/mesa/drivers/dri/i965/brw_context.h     |    7 ++++-
> >  src/mesa/drivers/dri/i965/brw_fs.cpp        |    9 ++++--
> >  src/mesa/drivers/dri/i965/brw_wm.c          |   40
> ++++++++++++++++++++++++---
> >  src/mesa/drivers/dri/i965/gen6_clip_state.c |   32
> +++++++++++++++++++++-
> >  src/mesa/drivers/dri/i965/gen6_sf_state.c   |   21 +++++++-------
> >  src/mesa/drivers/dri/i965/gen6_wm_state.c   |    9 +++++-
> >  src/mesa/drivers/dri/i965/gen7_clip_state.c |   12 +++++++-
> >  src/mesa/drivers/dri/i965/gen7_sf_state.c   |   21 +++++++-------
> >  src/mesa/drivers/dri/i965/gen7_wm_state.c   |   10 +++++-
> >  9 files changed, 126 insertions(+), 35 deletions(-)
> >
>
> > diff --git a/src/mesa/drivers/dri/i965/brw_wm.c
> b/src/mesa/drivers/dri/i965/brw_wm.c
> > index 546d522..923beaf 100644
> > --- a/src/mesa/drivers/dri/i965/brw_wm.c
> > +++ b/src/mesa/drivers/dri/i965/brw_wm.c
> > @@ -129,10 +129,41 @@ brw_wm_non_glsl_emit(struct brw_context *brw,
> struct brw_wm_compile *c)
> >   * (see enum brw_wm_barycentric_interp_mode) is needed by the fragment
> shader.
> >   */
> >  unsigned
> > -brw_compute_barycentric_interp_modes(void)
> > +brw_compute_barycentric_interp_modes(bool shade_model_flat,
> > +                                     const struct gl_fragment_program
> *fprog)
> >  {
> > -   /* At the moment the only interpolation mode we support is
> perspective. */
> > -   return (1 << BRW_WM_PERSPECTIVE_PIXEL_BARYCENTRIC);
> > +   unsigned barycentric_interp_modes = 0;
> > +   int attr;
> > +
> > +   /* Loop through all fragment shader inputs to figure out what
> interpolation
> > +    * modes are in use, and set the appropriate bits in
> > +    * barycentric_interp_modes.
> > +    */
> > +   for (attr = 0; attr < FRAG_ATTRIB_MAX; ++attr) {
> > +      enum glsl_interp_qualifier interp_qualifier =
> > +         fprog->InterpQualifier[attr];
> > +      bool is_gl_Color = attr == FRAG_ATTRIB_COL0 || attr ==
> FRAG_ATTRIB_COL1;
> > +
> > +      /* Ignore unused inputs. */
> > +      if (!(fprog->Base.InputsRead & BITFIELD64_BIT(attr)))
> > +         continue;
> > +
> > +      /* Ignore WPOS and FACE, because they don't require interpolation.
> */
> > +      if (attr == FRAG_ATTRIB_WPOS || attr == FRAG_ATTRIB_FACE)
> > +         continue;
> > +
> > +      if (interp_qualifier == INTERP_QUALIFIER_NOPERSPECTIVE) {
> > +         barycentric_interp_modes |=
> > +            1 << BRW_WM_NONPERSPECTIVE_PIXEL_BARYCENTRIC;
> > +      } else if (interp_qualifier == INTERP_QUALIFIER_SMOOTH ||
> > +                 (!(shade_model_flat && is_gl_Color) &&
> > +                  interp_qualifier == INTERP_QUALIFIER_NONE)) {
> > +         barycentric_interp_modes |=
> > +            1 << BRW_WM_PERSPECTIVE_PIXEL_BARYCENTRIC;
> > +      }
> > +   }
> > +
> > +   return barycentric_interp_modes;
> >  }
>
> One interesting behvaior change here is that we might not flag
> BRW_WM_PERSPECTIVE_PIXEL_BARYCENTRIC.  This looks safe, because we only
> use it for LINTERP for smooth, and for gl_FragCoord which is also an
> input.
>

In point of fact, we don't even use perspective barycentric coords for
gl_FragCoord on Gen6+, since the hardware computes it for us (it's in R27-28
of the fragment shader payload).


>
> I do wonder about the value of minimally flagging the perspective
> barycentric coord.  It seems like a lot of this would end up simpler if
> we just had a "does the program use noperspective?" flag in the program,
> and then we wouldn't have to do this work at state emit time, by
> assuming that we always use perspective.
>

Yeah, that might make some of this code faster.  The tradeoff would be that
it might slow down rendering slightly for these cases where perspective
barycentric coords aren't used, by forcing the hardware to populate two
registers that aren't going to be used.  I don't really have good intuition
about how to weigh these two possibilities against each other.


>
> On the other hand, the rule is make it work, then make it work fast.
> These patches look great as is.
>
> > diff --git a/src/mesa/drivers/dri/i965/gen7_wm_state.c
> b/src/mesa/drivers/dri/i965/gen7_wm_state.c
> > index e0004c5..2a9ebea 100644
> > --- a/src/mesa/drivers/dri/i965/gen7_wm_state.c
> > +++ b/src/mesa/drivers/dri/i965/gen7_wm_state.c
> > @@ -40,6 +40,10 @@ upload_wm_state(struct brw_context *brw)
> >        brw_fragment_program_const(brw->fragment_program);
> >     bool writes_depth = false;
> >     uint32_t dw1;
> > +   unsigned barycentric_interp_modes;
> > +
> > +   /* _NEW_LIGHT */
> > +   bool flat_shade = (ctx->Light.ShadeModel == GL_FLAT);
> >
> >     dw1 = 0;
> >     dw1 |= GEN7_WM_STATISTICS_ENABLE;
> > @@ -61,6 +65,8 @@ upload_wm_state(struct brw_context *brw)
> >        writes_depth = true;
> >        dw1 |= GEN7_WM_PSCDEPTH_ON;
> >     }
> > +   barycentric_interp_modes =
> > +      brw_compute_barycentric_interp_modes(flat_shade, &fp->program);
>
> I'd move this declaration down to the only use, or not make it a
> separate declaration at all.
>
>
You're right, separating this declaration out from its use is silly.  I
think I'd rather keep the code that depends on BRW_NEW_FRAGMENT_PROGRAM
together under the "/* BRW_NEW_FRAGMENT_PROGRAM */" comment, though, so I'll
move the use up and merge it with the declaration.


> >     /* _NEW_COLOR */
> >     if (fp->program.UsesKill || ctx->Color.AlphaEnabled)
> > @@ -72,7 +78,7 @@ upload_wm_state(struct brw_context *brw)
> >        dw1 |= GEN7_WM_DISPATCH_ENABLE;
> >     }
> >
> > -   dw1 |= brw_compute_barycentric_interp_modes() <<
> > +   dw1 |= barycentric_interp_modes <<
> >        GEN7_WM_BARYCENTRIC_INTERPOLATION_MODE_SHIFT;
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20111026/b51d45ad/attachment.html>