Re: [Mesa-dev] [PATCH] draw: clean up d3d style point clipping

2014-01-20 Thread Jose Fonseca 
Looks good to me AFAICT.

Jose

- Original Message -
 From: Roland Scheidegger srol...@vmware.com
 
 Instead of skipping x/y clipping completely if there's point_tri_clip points
 use guard band clipping. This should be easier (previously we could not
 disable
 generating the x/y bits in the clip mask for llvm path, hence requiring
 custom
 clip path), and it also allows us to enable this for tris-as-points more
 easily
 too (this would require custom tri clip filtering too otherwise). Moreover,
 some unexpected things could have happen if there's a NaN or just a huge
 number
 in some tri-turned-point, as the driver's rasterizer would need to deal with
 it
 and that might well lead to undefined behavior in typical rasterizers (which
 need to convert these numbers to fixed point). Using a guardband should hence
 be more robust, while usually guaranteeing the same results. (Only
 usually
 because unlike hw guardbands draw guardband is always just twice the vp size,
 hence small vp but large points could still lead to different results.)
 Unfortunately because the clipmask generated is completely unaffected by
 guard
 band clipping, we still need a custom clip stage for points (but not for
 tris,
 as the actual clipping there takes guard band into account).
 ---
  src/gallium/auxiliary/draw/draw_context.c  |8 ++---
  src/gallium/auxiliary/draw/draw_pipe_clip.c|   34
  ++--
  src/gallium/auxiliary/draw/draw_private.h  |2 +-
  .../auxiliary/draw/draw_pt_fetch_shade_pipeline.c  |   15 +
  .../draw/draw_pt_fetch_shade_pipeline_llvm.c   |8 +++--
  5 files changed, 42 insertions(+), 25 deletions(-)
 
 diff --git a/src/gallium/auxiliary/draw/draw_context.c
 b/src/gallium/auxiliary/draw/draw_context.c
 index 9b5bcb5..842fdd3 100644
 --- a/src/gallium/auxiliary/draw/draw_context.c
 +++ b/src/gallium/auxiliary/draw/draw_context.c
 @@ -262,10 +262,10 @@ static void update_clip_flags( struct draw_context
 *draw )
 draw-rasterizer  draw-rasterizer-depth_clip);
 draw-clip_user = draw-rasterizer 
   draw-rasterizer-clip_plane_enable != 0;
 -   draw-clip_points_xy = draw-clip_xy 
 -  (!draw-driver.bypass_clip_points ||
 -  (draw-rasterizer 
 -  !draw-rasterizer-point_tri_clip));
 +   draw-guard_band_points_xy = draw-guard_band_xy ||
 +(draw-driver.bypass_clip_points 
 +(draw-rasterizer 
 + draw-rasterizer-point_tri_clip));
  }
  
  /**
 diff --git a/src/gallium/auxiliary/draw/draw_pipe_clip.c
 b/src/gallium/auxiliary/draw/draw_pipe_clip.c
 index adfa4b6..9b339ae 100644
 --- a/src/gallium/auxiliary/draw/draw_pipe_clip.c
 +++ b/src/gallium/auxiliary/draw/draw_pipe_clip.c
 @@ -615,19 +615,33 @@ clip_point( struct draw_stage *stage,
stage-next-point( stage-next, header );
  }
  
 +
  /*
   * Clip points but ignore the first 4 (xy) clip planes.
 - * (This is necessary because we don't generate a different shader variant
 - * just for points hence xy clip bits are still generated. This is not
 really
 - * optimal because of the extra calculations both in generating clip masks
 - * and executing the clip stage but it gets the job done.)
 + * (Because the generated clip mask is completely unaffacted by guard band,
 + * we still need to manually evaluate the x/y planes if they are outside
 + * the guard band and not just outside the vp.)
   */
  static void
 -clip_point_no_xy( struct draw_stage *stage,
 -  struct prim_header *header )
 +clip_point_guard_xy( struct draw_stage *stage,
 + struct prim_header *header )
  {
 -   if ((header-v[0]-clipmask  0xfff0) == 0)
 -  stage-next-point( stage-next, header );
 +   unsigned clipmask = header-v[0]-clipmask;
 +   if ((clipmask  0x) == 0)
 +  stage-next-point(stage-next, header);
 +   else if ((clipmask  0xfff0) == 0) {
 +  while (clipmask) {
 + const unsigned plane_idx = ffs(clipmask)-1;
 + clipmask = ~(1  plane_idx);  /* turn off this plane's bit */
 + /* TODO: this should really do proper guardband clipping,
 +  * currently just throw out infs/nans.
 +  */
 + if (util_is_inf_or_nan(header-v[0]-clip[0]) ||
 + util_is_inf_or_nan(header-v[0]-clip[1]))
 +return;
 +  }
 +  stage-next-point(stage-next, header);
 +   }
  }
  
  
 @@ -636,7 +650,7 @@ static void
  clip_first_point( struct draw_stage *stage,
struct prim_header *header )
  {
 -   stage-point = stage-draw-clip_points_xy ? clip_point :
 clip_point_no_xy;
 +   stage-point = stage-draw-guard_band_points_xy ? clip_point_guard_xy :
 clip_point;
 stage-point(stage, header);
  }
  
 @@ -662,7 +676,7 @@ clip_line( struct draw_stage *stage,
  
  static void
  clip_tri( struct draw_stage

[Mesa-dev] [PATCH] draw: clean up d3d style point clipping

2014-01-17 Thread sroland 
From: Roland Scheidegger srol...@vmware.com

Instead of skipping x/y clipping completely if there's point_tri_clip points
use guard band clipping. This should be easier (previously we could not disable
generating the x/y bits in the clip mask for llvm path, hence requiring custom
clip path), and it also allows us to enable this for tris-as-points more easily
too (this would require custom tri clip filtering too otherwise). Moreover,
some unexpected things could have happen if there's a NaN or just a huge number
in some tri-turned-point, as the driver's rasterizer would need to deal with it
and that might well lead to undefined behavior in typical rasterizers (which
need to convert these numbers to fixed point). Using a guardband should hence
be more robust, while usually guaranteeing the same results. (Only usually
because unlike hw guardbands draw guardband is always just twice the vp size,
hence small vp but large points could still lead to different results.)
Unfortunately because the clipmask generated is completely unaffected by guard
band clipping, we still need a custom clip stage for points (but not for tris,
as the actual clipping there takes guard band into account).
---
 src/gallium/auxiliary/draw/draw_context.c  |8 ++---
 src/gallium/auxiliary/draw/draw_pipe_clip.c|   34 ++--
 src/gallium/auxiliary/draw/draw_private.h  |2 +-
 .../auxiliary/draw/draw_pt_fetch_shade_pipeline.c  |   15 +
 .../draw/draw_pt_fetch_shade_pipeline_llvm.c   |8 +++--
 5 files changed, 42 insertions(+), 25 deletions(-)

diff --git a/src/gallium/auxiliary/draw/draw_context.c 
b/src/gallium/auxiliary/draw/draw_context.c
index 9b5bcb5..842fdd3 100644
--- a/src/gallium/auxiliary/draw/draw_context.c
+++ b/src/gallium/auxiliary/draw/draw_context.c
@@ -262,10 +262,10 @@ static void update_clip_flags( struct draw_context *draw )
draw-rasterizer  draw-rasterizer-depth_clip);
draw-clip_user = draw-rasterizer 
  draw-rasterizer-clip_plane_enable != 0;
-   draw-clip_points_xy = draw-clip_xy 
-  (!draw-driver.bypass_clip_points ||
-  (draw-rasterizer 
-  !draw-rasterizer-point_tri_clip));
+   draw-guard_band_points_xy = draw-guard_band_xy ||
+(draw-driver.bypass_clip_points 
+(draw-rasterizer 
+ draw-rasterizer-point_tri_clip));
 }
 
 /**
diff --git a/src/gallium/auxiliary/draw/draw_pipe_clip.c 
b/src/gallium/auxiliary/draw/draw_pipe_clip.c
index adfa4b6..9b339ae 100644
--- a/src/gallium/auxiliary/draw/draw_pipe_clip.c
+++ b/src/gallium/auxiliary/draw/draw_pipe_clip.c
@@ -615,19 +615,33 @@ clip_point( struct draw_stage *stage,
   stage-next-point( stage-next, header );
 }
 
+
 /*
  * Clip points but ignore the first 4 (xy) clip planes.
- * (This is necessary because we don't generate a different shader variant
- * just for points hence xy clip bits are still generated. This is not really
- * optimal because of the extra calculations both in generating clip masks
- * and executing the clip stage but it gets the job done.)
+ * (Because the generated clip mask is completely unaffacted by guard band,
+ * we still need to manually evaluate the x/y planes if they are outside
+ * the guard band and not just outside the vp.)
  */
 static void
-clip_point_no_xy( struct draw_stage *stage,
-  struct prim_header *header )
+clip_point_guard_xy( struct draw_stage *stage,
+ struct prim_header *header )
 {
-   if ((header-v[0]-clipmask  0xfff0) == 0)
-  stage-next-point( stage-next, header );
+   unsigned clipmask = header-v[0]-clipmask;
+   if ((clipmask  0x) == 0)
+  stage-next-point(stage-next, header);
+   else if ((clipmask  0xfff0) == 0) {
+  while (clipmask) {
+ const unsigned plane_idx = ffs(clipmask)-1;
+ clipmask = ~(1  plane_idx);  /* turn off this plane's bit */
+ /* TODO: this should really do proper guardband clipping,
+  * currently just throw out infs/nans.
+  */
+ if (util_is_inf_or_nan(header-v[0]-clip[0]) ||
+ util_is_inf_or_nan(header-v[0]-clip[1]))
+return;
+  }
+  stage-next-point(stage-next, header);
+   }
 }
 
 
@@ -636,7 +650,7 @@ static void
 clip_first_point( struct draw_stage *stage,
   struct prim_header *header )
 {
-   stage-point = stage-draw-clip_points_xy ? clip_point : clip_point_no_xy;
+   stage-point = stage-draw-guard_band_points_xy ? clip_point_guard_xy : 
clip_point;
stage-point(stage, header);
 }
 
@@ -662,7 +676,7 @@ clip_line( struct draw_stage *stage,
 
 static void
 clip_tri( struct draw_stage *stage,
- struct prim_header *header )
+  struct prim_header *header )
 {
unsigned clipmask = (header-v[0]-clipmask |