Re: [PATCH 2/5] intel gen4-5: Compute the interpolation status for every variable in one place.

[Paul Berry <stereotype441@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 12742 bytes --]

On 30 June 2012 11:50, Olivier Galibert <galibert@pobox.com> wrote:

> The program keys are updated accordingly, but the values are not used
> yet.
>

> Signed-off-by: Olivier Galibert <galibert@pobox.com>
> ---
>  src/mesa/drivers/dri/i965/brw_clip.c    |   82
> ++++++++++++++++++++++++++++++-
>  src/mesa/drivers/dri/i965/brw_clip.h    |    1 +
>  src/mesa/drivers/dri/i965/brw_context.h |   59 ++++++++++++++++++++++
>  src/mesa/drivers/dri/i965/brw_sf.c      |    3 +-
>  src/mesa/drivers/dri/i965/brw_sf.h      |    1 +
>  src/mesa/drivers/dri/i965/brw_wm.c      |    4 ++
>  src/mesa/drivers/dri/i965/brw_wm.h      |    1 +
>  7 files changed, 149 insertions(+), 2 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_clip.c
> b/src/mesa/drivers/dri/i965/brw_clip.c
> index d411208..52e8c47 100644
> --- a/src/mesa/drivers/dri/i965/brw_clip.c
> +++ b/src/mesa/drivers/dri/i965/brw_clip.c
> @@ -47,6 +47,83 @@
>  #define FRONT_UNFILLED_BIT  0x1
>  #define BACK_UNFILLED_BIT   0x2
>
> +/**
> + * Lookup the interpolation mode information for every element in the
> + * vue.
> + */
> +static void
> +brw_lookup_interpolation(struct brw_context *brw)
> +{
> +   /* pprog means "previous program", i.e. the last program before the
> +    * fragment shader.  It can only be the vertex shader for now, but
> +    * it may be a geometry shader in the future.
> +    */
> +   const struct gl_program *pprog = &brw->vertex_program->Base;
> +   const struct gl_fragment_program *fprog = brw->fragment_program;
> +   struct brw_vue_map *vue_map = &brw->vs.prog_data->vue_map;
> +
> +   /* Default everything to INTERP_QUALIFIER_NONE */
> +   brw_clear_interpolation_modes(brw);
>

I'm not sure that inline functions like this one add any clarity, since
they force the reader to go look up the function to know what's going on.
I would recommend replacing this with

memset(&brw->interpolation_mode, 0, sizeof(brw->interpolation_mode));


> +
> +   /* If there is no fragment shader, interpolation won't be needed,
> +    * so defaulting to none is good.
> +    */
> +   if (!fprog)
> +      return;
> +
> +   for (int i = 0; i < vue_map->num_slots; i++) {
> +      /* First lookup the vert result, skip if there isn't one */
> +      int vert_result = vue_map->slot_to_vert_result[i];
> +      if (vert_result == BRW_VERT_RESULT_MAX)
> +         continue;
> +
> +      /* HPOS is special, it must be linear
> +       */
>

I believe this is correct, but it's counterintuitive.  Can you add an
explanation as to why?


> +      if (vert_result == VERT_RESULT_HPOS) {
> +         brw_set_interpolation_mode(brw, i,
> INTERP_QUALIFIER_NOPERSPECTIVE);
> +         continue;
> +      }
> +
> +      /* There is a 1-1 mapping of vert result to frag attrib except
> +       * for BackColor and vars
> +       */
> +      int frag_attrib = vert_result;
> +      if (vert_result >= VERT_RESULT_BFC0 && vert_result <=
> VERT_RESULT_BFC1)
> +         frag_attrib = vert_result - VERT_RESULT_BFC0 + FRAG_ATTRIB_COL0;
> +      else if(vert_result >= VERT_RESULT_VAR0)
> +         frag_attrib = vert_result - VERT_RESULT_VAR0 + FRAG_ATTRIB_VAR0;
> +
> +      /* If the output is not used by the fragment shader, skip it. */
> +      if (!(fprog->Base.InputsRead & BITFIELD64_BIT(frag_attrib)))
> +         continue;
> +
> +      /* Lookup the interpolation mode */
> +      enum glsl_interp_qualifier interpolation_mode =
> fprog->InterpQualifier[frag_attrib];
> +
> +      /* If the mode is not specified, then the default varies.  Color
> +       * values follow the shader model, while all the rest uses
> +       * smooth.
> +       */
> +      if (interpolation_mode == INTERP_QUALIFIER_NONE) {
> +         if (frag_attrib >= FRAG_ATTRIB_COL0 && frag_attrib <=
> FRAG_ATTRIB_COL1)
> +            interpolation_mode = brw->intel.ctx.Light.ShadeModel ==
> GL_FLAT ? INTERP_QUALIFIER_FLAT : INTERP_QUALIFIER_SMOOTH;
> +         else
> +            interpolation_mode = INTERP_QUALIFIER_SMOOTH;
> +      }
> +
> +      /* Finally, if we have both a front color and a back color for
> +       * the same channel, the selection will be done before
> +       * interpolation and the back color copied over the front color
> +       * if necessary.  So interpolating the back color is
> +       * unnecessary.
> +       */
> +      if (vert_result >= VERT_RESULT_BFC0 && vert_result <=
> VERT_RESULT_BFC1)
> +         if (pprog->OutputsWritten & BITFIELD64_BIT(vert_result -
> VERT_RESULT_BFC0 + VERT_RESULT_COL0))
> +            interpolation_mode = INTERP_QUALIFIER_NONE;
> +
> +      brw_set_interpolation_mode(brw, i, interpolation_mode);
> +   }
> +}
>
>  static void compile_clip_prog( struct brw_context *brw,
>                              struct brw_clip_prog_key *key )
> @@ -141,6 +218,8 @@ brw_upload_clip_prog(struct brw_context *brw)
>
>     memset(&key, 0, sizeof(key));
>
> +   brw_lookup_interpolation(brw);
>

Can you add a comment "/* BRW_NEW_FRAGMENT_PROGRAM, _NEW_LIGHT */" above
this line?  That will help remind us that this function consults the
fragment program and the lighting state.


> +
>     /* Populate the key:
>      */
>     /* BRW_NEW_REDUCED_PRIMITIVE */
> @@ -150,6 +229,7 @@ brw_upload_clip_prog(struct brw_context *brw)
>     /* _NEW_LIGHT */
>     key.do_flat_shading = (ctx->Light.ShadeModel == GL_FLAT);
>     key.pv_first = (ctx->Light.ProvokingVertex ==
> GL_FIRST_VERTEX_CONVENTION);
> +   brw_copy_interpolation_modes(brw, key.interpolation_mode);
>

Similar to my comment above about brw_clear_interpolation_modes(), I would
recommend replacing this with a simple call to memcpy().


>     /* _NEW_TRANSFORM (also part of VUE map)*/
>     key.nr_userclip = _mesa_bitcount_64(ctx->Transform.ClipPlanesEnabled);
>
> @@ -258,7 +338,7 @@ const struct brw_tracked_state brw_clip_prog = {
>                 _NEW_TRANSFORM |
>                 _NEW_POLYGON |
>                 _NEW_BUFFERS),
> -      .brw   = (BRW_NEW_REDUCED_PRIMITIVE),
> +      .brw   = (BRW_NEW_FRAGMENT_PROGRAM|BRW_NEW_REDUCED_PRIMITIVE),
>        .cache = CACHE_NEW_VS_PROG
>     },
>     .emit = brw_upload_clip_prog
> diff --git a/src/mesa/drivers/dri/i965/brw_clip.h
> b/src/mesa/drivers/dri/i965/brw_clip.h
> index 9185651..6f811ae 100644
> --- a/src/mesa/drivers/dri/i965/brw_clip.h
> +++ b/src/mesa/drivers/dri/i965/brw_clip.h
> @@ -43,6 +43,7 @@
>   */
>  struct brw_clip_prog_key {
>     GLbitfield64 attrs;
> +   GLbitfield64 interpolation_mode[2]; /* copy of the main context */
>     GLuint primitive:4;
>     GLuint nr_userclip:4;
>     GLuint do_flat_shading:1;
> diff --git a/src/mesa/drivers/dri/i965/brw_context.h
> b/src/mesa/drivers/dri/i965/brw_context.h
> index 69659c4e..1a417e5 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.h
> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> @@ -1041,6 +1041,17 @@ struct brw_context
>     uint32_t render_target_format[MESA_FORMAT_COUNT];
>     bool format_supported_as_render_target[MESA_FORMAT_COUNT];
>
> +   /* Interpolation modes, two bits per vue slot, values equal to
> +    * glsl_interp_qualifier.
> +    *
> +    * Used on gen4/5 by the clipper, sf and wm stages.  Given the
> +    * update order, the clipper is resposible to update it.
>
+    *
> +    * Ignored on gen 6+
> +    */
> +
> +   GLbitfield64 interpolation_mode[2]; /* two bits per vue slot */
> +
>

I don't think the space savings is worth the complication here.  How about
changing this (and the other new interpolation_mode arrays) to:

unsigned char interpolation_mode[BRW_VERT_RESULT_MAX];

Then we can replace brw_set_interpolation_mode() and
brw_get_interpolation_mode() by simply indexing into the array.


>     /* PrimitiveRestart */
>     struct {
>        bool in_progress;
> @@ -1262,6 +1273,54 @@ brw_program_reloc(struct brw_context *brw, uint32_t
> state_offset,
>
>  bool brw_do_cubemap_normalize(struct exec_list *instructions);
>
> +/**
> + * Pre-gen6, interpolation mode has to be resolved for code generation
> + * in clipper, sf and wm.  The resolution is done once by the clipper
> + * and stored in the interpolation_mode array and in the keys.  These
> + * functions allow access to that packed array.
> + */
> +
> +/* INTERP_QUALIFIER_NONE is required to be 0, so initialization is easy */
> +static inline
> +void brw_clear_interpolation_modes(struct brw_context *brw)
> +{
> +   brw->interpolation_mode[0] = brw->interpolation_mode[1] = 0;
> +}
> +
> +static inline
> +void brw_clear_interpolation_modes_key(GLbitfield64 *interpolation_mode)
> +{
> +   interpolation_mode[0] = interpolation_mode[1] = 0;
> +}
> +
> +/* Store an interpolation mode */
> +static inline
> +void brw_set_interpolation_mode(struct brw_context *brw, int slot, enum
> glsl_interp_qualifier mode)
> +{
> +   int idx = slot >> 5;
> +   int shift = 2*(slot & 31);
> +   brw->interpolation_mode[idx] =
> +      (brw->interpolation_mode[idx] & ~(3ULL << shift))
> +      | (((GLbitfield64)mode) << shift);
> +}
> +
> +/* Retrieve an interpolation mode */
> +static inline
> +enum glsl_interp_qualifier brw_get_interpolation_mode(struct brw_context
> *brw, int slot)
> +{
> +   int idx = slot >> 5;
> +   int shift = 2*(slot & 31);
> +   return (enum glsl_interp_qualifier)((brw->interpolation_mode[idx] >>
> shift) & 3);
> +}
> +
> +/* Copy the interpolation mode information to a key */
> +static inline
> +void brw_copy_interpolation_modes(struct brw_context *brw, GLbitfield64
> *dest)
> +{
> +   dest[0] = brw->interpolation_mode[0];
> +   dest[1] = brw->interpolation_mode[1];
> +}
> +
>

If we change the type of interpolation_mode, and use memset() and memcpy()
where they make sense, I think we can eliminate the need for all these
inline functions.


>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/src/mesa/drivers/dri/i965/brw_sf.c
> b/src/mesa/drivers/dri/i965/brw_sf.c
> index 7867ab5..0cc4fc7 100644
> --- a/src/mesa/drivers/dri/i965/brw_sf.c
> +++ b/src/mesa/drivers/dri/i965/brw_sf.c
> @@ -194,6 +194,7 @@ brw_upload_sf_prog(struct brw_context *brw)
>     key.do_flat_shading = (ctx->Light.ShadeModel == GL_FLAT);
>     key.do_twoside_color = (ctx->Light.Enabled &&
> ctx->Light.Model.TwoSide) ||
>       ctx->VertexProgram._TwoSideEnabled;
> +   brw_copy_interpolation_modes(brw, key.interpolation_mode);
>

This can also be replaced with a call to memcpy().  Also, we should put a
comment "/* BRW_NEW_FRAGMENT_PROGRAM, _NEW_LIGHT */" above it.


>
>     /* _NEW_POLYGON */
>     if (key.do_twoside_color) {
> @@ -216,7 +217,7 @@ const struct brw_tracked_state brw_sf_prog = {
>     .dirty = {
>        .mesa  = (_NEW_HINT | _NEW_LIGHT | _NEW_POLYGON | _NEW_POINT |
>                  _NEW_TRANSFORM | _NEW_BUFFERS),
> -      .brw   = (BRW_NEW_REDUCED_PRIMITIVE),
> +      .brw   = (BRW_NEW_FRAGMENT_PROGRAM|BRW_NEW_REDUCED_PRIMITIVE),
>        .cache = CACHE_NEW_VS_PROG
>     },
>     .emit = brw_upload_sf_prog
> diff --git a/src/mesa/drivers/dri/i965/brw_sf.h
> b/src/mesa/drivers/dri/i965/brw_sf.h
> index f908fc0..0a8135c 100644
> --- a/src/mesa/drivers/dri/i965/brw_sf.h
> +++ b/src/mesa/drivers/dri/i965/brw_sf.h
> @@ -46,6 +46,7 @@
>
>  struct brw_sf_prog_key {
>     GLbitfield64 attrs;
> +   GLbitfield64 interpolation_mode[2]; /* copy of the main context */
>     uint8_t point_sprite_coord_replace;
>     GLuint primitive:2;
>     GLuint do_twoside_color:1;
> diff --git a/src/mesa/drivers/dri/i965/brw_wm.c
> b/src/mesa/drivers/dri/i965/brw_wm.c
> index 4a7225c..c7c1c45 100644
> --- a/src/mesa/drivers/dri/i965/brw_wm.c
> +++ b/src/mesa/drivers/dri/i965/brw_wm.c
> @@ -504,6 +504,10 @@ static void brw_wm_populate_key( struct brw_context
> *brw,
>
>     /* _NEW_LIGHT */
>     key->flat_shade = (ctx->Light.ShadeModel == GL_FLAT);
> +   if (intel->gen < 6)
> +      brw_copy_interpolation_modes(brw, key->interpolation_mode);
> +   else
> +      brw_clear_interpolation_modes_key(key->interpolation_mode);
>

The "else" part isn't necessary, since we memset() the key to zero at the
top of the function.


>
>     /* _NEW_FRAG_CLAMP | _NEW_BUFFERS */
>     key->clamp_fragment_color = ctx->Color._ClampFragmentColor;
> diff --git a/src/mesa/drivers/dri/i965/brw_wm.h
> b/src/mesa/drivers/dri/i965/brw_wm.h
> index 2cde2a0..53b83f8 100644
> --- a/src/mesa/drivers/dri/i965/brw_wm.h
> +++ b/src/mesa/drivers/dri/i965/brw_wm.h
> @@ -60,6 +60,7 @@
>  #define AA_ALWAYS    2
>
>  struct brw_wm_prog_key {
> +   GLbitfield64 interpolation_mode[2]; /* copy of the main context for
> gen4-5, 0 for gen6+ */
>     uint8_t iz_lookup;
>     GLuint stats_wm:1;
>     GLuint flat_shade:1;
> --
> 1.7.10.rc3.1.gb306
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>

[-- Attachment #1.2: Type: text/html, Size: 15829 bytes --]

[-- Attachment #2: Type: text/plain, Size: 156 bytes --]

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev