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

[Eric Anholt]

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.

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.

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.

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.

>     /* _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 --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20111026/f0be53b4/attachment-0001.pgp>