[Mesa-dev] [PATCH 3/3] draw: fix clipping of layer/vp index outputs

Jose Fonseca jfonseca at vmware.com
Thu Dec 3 06:17:45 PST 2015


On 03/12/15 00:35, sroland at vmware.com wrote:
> From: Roland Scheidegger <sroland at vmware.com>
>
> This was just plain broken. It used always the value from v0 (for vp_index)
> but would pass the value from the provoking vertex to later stages - but only
> if there was a corresponding fs input, otherwise the layer/vp index would get
> lost completely (as it would try to interpolate the (unsigned) values as
> floats).
> So, make it obey provoking vertex rules (drivers relying on draw will need to
> do the same). And make sure that the default interpolation mode (when no
> corresponding fs input is found) for them is constant.
> Also, change the code a bit so constant inputs aren't interpolated then
> copied over later.
>
> No piglit change (but I'm writing one...).
> ---
>   src/gallium/auxiliary/draw/draw_pipe_clip.c | 207 +++++++++++++++++-----------
>   1 file changed, 130 insertions(+), 77 deletions(-)
>
> diff --git a/src/gallium/auxiliary/draw/draw_pipe_clip.c b/src/gallium/auxiliary/draw/draw_pipe_clip.c
> index c22758b..7d9ea5e 100644
> --- a/src/gallium/auxiliary/draw/draw_pipe_clip.c
> +++ b/src/gallium/auxiliary/draw/draw_pipe_clip.c
> @@ -58,12 +58,17 @@
>   struct clip_stage {
>      struct draw_stage stage;      /**< base class */
>
> -   /* List of the attributes to be flatshaded. */
> -   uint num_flat_attribs;
> -   uint flat_attribs[PIPE_MAX_SHADER_OUTPUTS];
> -
> -   /* Mask of attributes in noperspective mode */
> -   boolean noperspective_attribs[PIPE_MAX_SHADER_OUTPUTS];
> +   unsigned pos_attr;
> +
> +   /* List of the attributes to be constant interpolated. */
> +   uint num_const_attribs;
> +   uint8_t const_attribs[PIPE_MAX_SHADER_OUTPUTS];
> +   /* List of the attributes to be linear interpolated. */
> +   uint num_linear_attribs;
> +   uint8_t linear_attribs[PIPE_MAX_SHADER_OUTPUTS];
> +   /* List of the attributes to be perspective interpolated. */
> +   uint num_perspect_attribs;
> +   uint8_t perspect_attribs[PIPE_MAX_SHADER_OUTPUTS];
>
>      float (*plane)[4];
>   };
> @@ -97,9 +102,9 @@ draw_viewport_index(struct draw_context *draw,
>   /* All attributes are float[4], so this is easy:
>    */
>   static void interp_attr( float dst[4],
> -			 float t,
> -			 const float in[4],
> -			 const float out[4] )
> +                         float t,
> +                         const float in[4],
> +                         const float out[4] )
>   {
>      dst[0] = LINTERP( t, out[0], in[0] );
>      dst[1] = LINTERP( t, out[1], in[1] );
> @@ -117,8 +122,8 @@ static void copy_flat( struct draw_stage *stage,
>   {
>      const struct clip_stage *clipper = clip_stage(stage);
>      uint i;
> -   for (i = 0; i < clipper->num_flat_attribs; i++) {
> -      const uint attr = clipper->flat_attribs[i];
> +   for (i = 0; i < clipper->num_const_attribs; i++) {
> +      const uint attr = clipper->const_attribs[i];
>         COPY_4FV(dst->data[attr], src->data[attr]);
>      }
>   }
> @@ -126,15 +131,13 @@ static void copy_flat( struct draw_stage *stage,
>   /* Interpolate between two vertices to produce a third.
>    */
>   static void interp( const struct clip_stage *clip,
> -		    struct vertex_header *dst,
> -		    float t,
> -		    const struct vertex_header *out,
> -		    const struct vertex_header *in,
> +                    struct vertex_header *dst,
> +                    float t,
> +                    const struct vertex_header *out,
> +                    const struct vertex_header *in,
>                       unsigned viewport_index )
>   {
> -   const unsigned nr_attrs = draw_num_shader_outputs(clip->stage.draw);
> -   const unsigned pos_attr = draw_current_shader_position_output(clip->stage.draw);
> -   const unsigned clip_attr = draw_current_shader_clipvertex_output(clip->stage.draw);
> +   const unsigned pos_attr = clip->pos_attr;
>      unsigned j;
>      float t_nopersp;
>
> @@ -168,6 +171,13 @@ static void interp( const struct clip_stage *clip,
>         dst->data[pos_attr][3] = oow;
>      }
>
> +
> +   /* interp perspective attribs */
> +   for (j = 0; j < clip->num_perspect_attribs; j++) {
> +      const unsigned attr = clip->perspect_attribs[j];
> +      interp_attr(dst->data[attr], t, in->data[attr], out->data[attr]);
> +   }
> +
>      /**
>       * Compute the t in screen-space instead of 3d space to use
>       * for noperspective interpolation.
> @@ -177,7 +187,7 @@ static void interp( const struct clip_stage *clip,
>       * pick whatever value (the interpolated point won't be in front
>       * anyway), so just use the 3d t.
>       */
> -   {
> +   if (clip->num_linear_attribs){
>         int k;
>         t_nopersp = t;
>         /* find either in.x != out.x or in.y != out.y */
> @@ -191,24 +201,17 @@ static void interp( const struct clip_stage *clip,
>               break;
>            }
>         }
> -   }
> -
> -   /* Other attributes
> -    */
> -   for (j = 0; j < nr_attrs; j++) {
> -      if (j != pos_attr && j != clip_attr) {
> -         if (clip->noperspective_attribs[j])
> -            interp_attr(dst->data[j], t_nopersp, in->data[j], out->data[j]);
> -         else
> -            interp_attr(dst->data[j], t, in->data[j], out->data[j]);
> +      for (j = 0; j < clip->num_linear_attribs; j++) {
> +         const unsigned attr = clip->linear_attribs[j];
> +         interp_attr(dst->data[attr], t_nopersp, in->data[attr], out->data[attr]);
>         }
>      }
>   }
>
>   /**
> - * Checks whether the specifed triangle is empty and if it is returns
> + * Checks whether the specified triangle is empty and if it is returns
>    * true, otherwise returns false.
> - * Triangle is considered null/empty if it's area is qual to zero.
> + * Triangle is considered null/empty if it's area is equal to zero.
>    */
>   static inline boolean
>   is_tri_null(struct draw_context *draw, const struct prim_header *header)
> @@ -234,10 +237,10 @@ is_tri_null(struct draw_context *draw, const struct prim_header *header)
>    * will be convex and the provoking vertex will always be vertex[0].
>    */
>   static void emit_poly( struct draw_stage *stage,
> -		       struct vertex_header **inlist,
> +                       struct vertex_header **inlist,
>                          const boolean *edgeflags,
> -		       unsigned n,
> -		       const struct prim_header *origPrim)
> +                       unsigned n,
> +                       const struct prim_header *origPrim)
>   {
>      struct prim_header header;
>      unsigned i;
> @@ -359,14 +362,15 @@ static inline float getclipdist(const struct clip_stage *clipper,
>    */
>   static void
>   do_clip_tri( struct draw_stage *stage,
> -	     struct prim_header *header,
> -	     unsigned clipmask )
> +             struct prim_header *header,
> +             unsigned clipmask )
>   {
>      struct clip_stage *clipper = clip_stage( stage );
>      struct vertex_header *a[MAX_CLIPPED_VERTICES];
>      struct vertex_header *b[MAX_CLIPPED_VERTICES];
>      struct vertex_header **inlist = a;
>      struct vertex_header **outlist = b;
> +   struct vertex_header *prov_vertex;
>      unsigned tmpnr = 0;
>      unsigned n = 3;
>      unsigned i;
> @@ -380,7 +384,20 @@ do_clip_tri( struct draw_stage *stage,
>      inlist[1] = header->v[1];
>      inlist[2] = header->v[2];
>
> -   viewport_index = draw_viewport_index(clipper->stage.draw, inlist[0]);
> +   /*
> +    * For d3d10, we need to take this from the leading (first) vertex.
> +    * For GL, we could do anything (as long as we advertize
> +    * GL_UNDEFINED_VERTEX for the VIEWPORT_INDEX_PROVOKING_VERTEX query),
> +    * but it needs to be consistent with what other parts (i.e. driver)
> +    * will do, and that seems easier with GL_PROVOKING_VERTEX logic.
> +    */
> +   if (stage->draw->rasterizer->flatshade_first) {
> +      prov_vertex = inlist[0];
> +   }
> +   else {
> +      prov_vertex = inlist[2];
> +   }
> +   viewport_index = draw_viewport_index(clipper->stage.draw, prov_vertex);
>
>      if (DEBUG_CLIP) {
>         const float *v0 = header->v[0]->clip;
> @@ -512,10 +529,10 @@ do_clip_tri( struct draw_stage *stage,
>
>      }
>
> -   /* If flat-shading, copy provoking vertex color to polygon vertex[0]
> +   /* If constant interpolated, copy provoking vertex attrib to polygon vertex[0]
>       */
>      if (n >= 3) {
> -      if (clipper->num_flat_attribs) {
> +      if (clipper->num_const_attribs) {
>            if (stage->draw->rasterizer->flatshade_first) {
>               if (inlist[0] != header->v[0]) {
>                  assert(tmpnr < MAX_CLIPPED_VERTICES + 1);
> @@ -547,16 +564,25 @@ do_clip_tri( struct draw_stage *stage,
>    */
>   static void
>   do_clip_line( struct draw_stage *stage,
> -	      struct prim_header *header,
> -	      unsigned clipmask )
> +              struct prim_header *header,
> +              unsigned clipmask )
>   {
>      const struct clip_stage *clipper = clip_stage( stage );
>      struct vertex_header *v0 = header->v[0];
>      struct vertex_header *v1 = header->v[1];
> +   struct vertex_header *prov_vertex;
>      float t0 = 0.0F;
>      float t1 = 0.0F;
>      struct prim_header newprim;
> -   int viewport_index = draw_viewport_index(clipper->stage.draw, v0);
> +   int viewport_index;
> +
> +   if (stage->draw->rasterizer->flatshade_first) {
> +      prov_vertex = v0;
> +   }
> +   else {
> +      prov_vertex = v1;
> +   }
> +   viewport_index = draw_viewport_index(clipper->stage.draw, prov_vertex);
>
>      while (clipmask) {
>         const unsigned plane_idx = ffs(clipmask)-1;
> @@ -567,17 +593,17 @@ do_clip_line( struct draw_stage *stage,
>            return; //discard nan
>
>         if (dp1 < 0.0F) {
> -	 float t = dp1 / (dp1 - dp0);
> +         float t = dp1 / (dp1 - dp0);
>            t1 = MAX2(t1, t);
>         }
>
>         if (dp0 < 0.0F) {
> -	 float t = dp0 / (dp0 - dp1);
> +         float t = dp0 / (dp0 - dp1);
>            t0 = MAX2(t0, t);
>         }
>
>         if (t0 + t1 >= 1.0F)
> -	 return; /* discard */
> +         return; /* discard */
>
>         clipmask &= ~(1 << plane_idx);  /* turn off this plane's bit */
>      }
> @@ -668,7 +694,7 @@ clip_first_point( struct draw_stage *stage,
>
>   static void
>   clip_line( struct draw_stage *stage,
> -	   struct prim_header *header )
> +           struct prim_header *header )
>   {
>      unsigned clipmask = (header->v[0]->clipmask |
>                           header->v[1]->clipmask);
> @@ -715,12 +741,24 @@ find_interp(const struct draw_fragment_shader *fs, int *indexed_interp,
>      if (semantic_name == TGSI_SEMANTIC_COLOR ||
>          semantic_name == TGSI_SEMANTIC_BCOLOR) {
>         interp = indexed_interp[semantic_index];
> +   } else if (semantic_name == TGSI_SEMANTIC_POSITION ||
> +              semantic_name == TGSI_SEMANTIC_CLIPVERTEX) {
> +      /* these inputs are handled specially always */
> +      return -1;
>      } else {
>         /* Otherwise, search in the FS inputs, with a decent default
>          * if we don't find it.
> +       * This probably only matters for layer, vpindex, culldist, maybe
> +       * front_face.
>          */
>         uint j;
> -      interp = TGSI_INTERPOLATE_PERSPECTIVE;
> +      if (semantic_name == TGSI_SEMANTIC_LAYER ||
> +          semantic_name == TGSI_SEMANTIC_VIEWPORT_INDEX) {
> +         interp = TGSI_INTERPOLATE_CONSTANT;
> +      }
> +      else {
> +         interp = TGSI_INTERPOLATE_PERSPECTIVE;
> +      }
>         if (fs) {
>            for (j = 0; j < fs->info.num_inputs; j++) {
>               if (semantic_name == fs->info.input_semantic_name[j] &&
> @@ -745,6 +783,9 @@ clip_init_state( struct draw_stage *stage )
>      const struct draw_fragment_shader *fs = draw->fs.fragment_shader;
>      const struct tgsi_shader_info *info = draw_get_shader_info(draw);
>      uint i, j;
> +   int indexed_interp[2];
> +
> +   clipper->pos_attr = draw_current_shader_position_output(draw);
>
>      /* We need to know for each attribute what kind of interpolation is
>       * done on it (flat, smooth or noperspective).  But the information
> @@ -765,7 +806,6 @@ clip_init_state( struct draw_stage *stage )
>      /* First pick up the interpolation mode for
>       * gl_Color/gl_SecondaryColor, with the correct default.
>       */
> -   int indexed_interp[2];
>      indexed_interp[0] = indexed_interp[1] = draw->rasterizer->flatshade ?
>         TGSI_INTERPOLATE_CONSTANT : TGSI_INTERPOLATE_PERSPECTIVE;
>
> @@ -778,29 +818,33 @@ clip_init_state( struct draw_stage *stage )
>         }
>      }
>
> -   /* Then resolve the interpolation mode for every output attribute.
> -    *
> -    * Given how the rest of the code, the most efficient way is to
> -    * have a vector of flat-mode attributes, and a mask for
> -    * noperspective attributes.
> -    */
> +   /* Then resolve the interpolation mode for every output attribute. */
>
> -   clipper->num_flat_attribs = 0;
> -   memset(clipper->noperspective_attribs, 0, sizeof(clipper->noperspective_attribs));
> +   clipper->num_const_attribs = 0;
> +   clipper->num_linear_attribs = 0;
> +   clipper->num_perspect_attribs = 0;
>      for (i = 0; i < info->num_outputs; i++) {
>         /* Find the interpolation mode for a specific attribute */
>         int interp = find_interp(fs, indexed_interp,
>                                  info->output_semantic_name[i],
>                                  info->output_semantic_index[i]);
> -      /* If it's flat, add it to the flat vector.  Otherwise update
> -       * the noperspective mask.
> -       */
> -
> -      if (interp == TGSI_INTERPOLATE_CONSTANT) {
> -         clipper->flat_attribs[clipper->num_flat_attribs] = i;
> -         clipper->num_flat_attribs++;
> -      } else
> -         clipper->noperspective_attribs[i] = interp == TGSI_INTERPOLATE_LINEAR;
> +      switch (interp) {
> +      case TGSI_INTERPOLATE_CONSTANT:
> +         clipper->const_attribs[clipper->num_const_attribs] = i;
> +         clipper->num_const_attribs++;
> +         break;
> +      case TGSI_INTERPOLATE_LINEAR:
> +         clipper->linear_attribs[clipper->num_linear_attribs] = i;
> +         clipper->num_linear_attribs++;
> +         break;
> +      case TGSI_INTERPOLATE_PERSPECTIVE:
> +         clipper->perspect_attribs[clipper->num_perspect_attribs] = i;
> +         clipper->num_perspect_attribs++;
> +         break;
> +      default:
> +         assert(interp == -1);
> +         break;
> +      }
>      }
>      /* Search the extra vertex attributes */
>      for (j = 0; j < draw->extra_shader_outputs.num; j++) {
> @@ -808,16 +852,25 @@ clip_init_state( struct draw_stage *stage )
>         int interp = find_interp(fs, indexed_interp,
>                                  draw->extra_shader_outputs.semantic_name[j],
>                                  draw->extra_shader_outputs.semantic_index[j]);
> -      /* If it's flat, add it to the flat vector.  Otherwise update
> -       * the noperspective mask.
> -       */
> -      if (interp == TGSI_INTERPOLATE_CONSTANT) {
> -         clipper->flat_attribs[clipper->num_flat_attribs] = i + j;
> -         clipper->num_flat_attribs++;
> -      } else
> -         clipper->noperspective_attribs[i + j] = interp == TGSI_INTERPOLATE_LINEAR;
> +      switch (interp) {
> +      case TGSI_INTERPOLATE_CONSTANT:
> +         clipper->const_attribs[clipper->num_const_attribs] = i + j;
> +         clipper->num_const_attribs++;
> +         break;
> +      case TGSI_INTERPOLATE_LINEAR:
> +         clipper->linear_attribs[clipper->num_linear_attribs] = i + j;
> +         clipper->num_linear_attribs++;
> +         break;
> +      case TGSI_INTERPOLATE_PERSPECTIVE:
> +         clipper->perspect_attribs[clipper->num_perspect_attribs] = i + j;
> +         clipper->num_perspect_attribs++;
> +         break;
> +      default:
> +         assert(interp == -1);
> +         break;
> +      }
>      }
> -
> +
>      stage->tri = clip_tri;
>      stage->line = clip_line;
>   }
> @@ -825,14 +878,14 @@ clip_init_state( struct draw_stage *stage )
>
>
>   static void clip_first_tri( struct draw_stage *stage,
> -			    struct prim_header *header )
> +                            struct prim_header *header )
>   {
>      clip_init_state( stage );
>      stage->tri( stage, header );
>   }
>
>   static void clip_first_line( struct draw_stage *stage,
> -			     struct prim_header *header )
> +                             struct prim_header *header )
>   {
>      clip_init_state( stage );
>      stage->line( stage, header );
> @@ -840,7 +893,7 @@ static void clip_first_line( struct draw_stage *stage,
>
>
>   static void clip_flush( struct draw_stage *stage,
> -			     unsigned flags )
> +                        unsigned flags )
>   {
>      stage->tri = clip_first_tri;
>      stage->line = clip_first_line;
>

Series looks good AFAICT.

Reviewed-by: Jose Fonseca <jfonseca at vmware.com>


More information about the mesa-dev mailing list