[Mesa-dev] [PATCH 2/2] llvmpipe: fix using 32bit rasterization mistakenly, causing overflows

Jose Fonseca jfonseca at vmware.com
Fri Jun 23 16:05:26 UTC 2017


LGTM.  Great find.


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


________________________________
From: sroland at vmware.com <sroland at vmware.com>
Sent: Friday, June 23, 2017 16:33
To: Jose Fonseca; Brian Paul; mesa-dev at lists.freedesktop.org
Cc: Roland Scheidegger
Subject: [PATCH 2/2] llvmpipe: fix using 32bit rasterization mistakenly, causing overflows

From: Roland Scheidegger <sroland at vmware.com>

We use the bounding box (triangle extents) to figure out if 32bit rasterization
could potentially overflow. However, we used the bounding box which already got
rounded up to 0 for negative coords for this, which is incorrect, leading to
overflows and hence bogus rendering in some of our private use.

It might be possible to simplify this somehow (we're now using 3 different
boxes for binning) but I don't quite see how.
---
 src/gallium/drivers/llvmpipe/lp_setup_context.h | 11 ++++---
 src/gallium/drivers/llvmpipe/lp_setup_line.c    | 20 ++++++------
 src/gallium/drivers/llvmpipe/lp_setup_point.c   |  2 +-
 src/gallium/drivers/llvmpipe/lp_setup_tri.c     | 41 ++++++++++++++++---------
 4 files changed, 43 insertions(+), 31 deletions(-)

diff --git a/src/gallium/drivers/llvmpipe/lp_setup_context.h b/src/gallium/drivers/llvmpipe/lp_setup_context.h
index 9714691..4b55fd9 100644
--- a/src/gallium/drivers/llvmpipe/lp_setup_context.h
+++ b/src/gallium/drivers/llvmpipe/lp_setup_context.h
@@ -215,10 +215,11 @@ lp_setup_alloc_triangle(struct lp_scene *scene,
                         unsigned *tri_size);

 boolean
-lp_setup_bin_triangle( struct lp_setup_context *setup,
-                       struct lp_rast_triangle *tri,
-                       const struct u_rect *bbox,
-                       int nr_planes,
-                       unsigned scissor_index );
+lp_setup_bin_triangle(struct lp_setup_context *setup,
+                      struct lp_rast_triangle *tri,
+                      const struct u_rect *bboxorig,
+                      const struct u_rect *bbox,
+                      int nr_planes,
+                      unsigned scissor_index);

 #endif
diff --git a/src/gallium/drivers/llvmpipe/lp_setup_line.c b/src/gallium/drivers/llvmpipe/lp_setup_line.c
index 018130c..d0bac5e 100644
--- a/src/gallium/drivers/llvmpipe/lp_setup_line.c
+++ b/src/gallium/drivers/llvmpipe/lp_setup_line.c
@@ -288,7 +288,9 @@ try_setup_line( struct lp_setup_context *setup,
    struct lp_rast_plane *plane;
    struct lp_line_info info;
    float width = MAX2(1.0, setup->line_width);
-   struct u_rect bbox;
+   const struct u_rect *scissor;
+   struct u_rect bbox, bboxpos;
+   boolean s_planes[4];
    unsigned tri_bytes;
    int x[4];
    int y[4];
@@ -579,10 +581,12 @@ try_setup_line( struct lp_setup_context *setup,
       return TRUE;
    }

+   bboxpos = bbox;
+
    /* Can safely discard negative regions:
     */
-   bbox.x0 = MAX2(bbox.x0, 0);
-   bbox.y0 = MAX2(bbox.y0, 0);
+   bboxpos.x0 = MAX2(bboxpos.x0, 0);
+   bboxpos.y0 = MAX2(bboxpos.y0, 0);

    nr_planes = 4;
    /*
@@ -591,8 +595,8 @@ try_setup_line( struct lp_setup_context *setup,
     */
    if (setup->scissor_test) {
       /* why not just use draw_regions */
-      boolean s_planes[4];
-      scissor_planes_needed(s_planes, &bbox, &setup->scissors[viewport_index]);
+      scissor = &setup->scissors[viewport_index];
+      scissor_planes_needed(s_planes, &bboxpos, scissor);
       nr_planes += s_planes[0] + s_planes[1] + s_planes[2] + s_planes[3];
    }

@@ -718,11 +722,7 @@ try_setup_line( struct lp_setup_context *setup,
     * (easier to evaluate) to ordinary planes.)
     */
    if (nr_planes > 4) {
-      /* why not just use draw_regions */
-      const struct u_rect *scissor = &setup->scissors[viewport_index];
       struct lp_rast_plane *plane_s = &plane[4];
-      boolean s_planes[4];
-      scissor_planes_needed(s_planes, &bbox, scissor);

       if (s_planes[0]) {
          plane_s->dcdx = -1 << 8;
@@ -755,7 +755,7 @@ try_setup_line( struct lp_setup_context *setup,
       assert(plane_s == &plane[nr_planes]);
    }

-   return lp_setup_bin_triangle(setup, line, &bbox, nr_planes, viewport_index);
+   return lp_setup_bin_triangle(setup, line, &bbox, &bboxpos, nr_planes, viewport_index);
 }


diff --git a/src/gallium/drivers/llvmpipe/lp_setup_point.c b/src/gallium/drivers/llvmpipe/lp_setup_point.c
index ddb6f0e..8cb6b83 100644
--- a/src/gallium/drivers/llvmpipe/lp_setup_point.c
+++ b/src/gallium/drivers/llvmpipe/lp_setup_point.c
@@ -513,7 +513,7 @@ try_setup_point( struct lp_setup_context *setup,
       plane[3].eo = 0;
    }

-   return lp_setup_bin_triangle(setup, point, &bbox, nr_planes, viewport_index);
+   return lp_setup_bin_triangle(setup, point, &bbox, &bbox, nr_planes, viewport_index);
 }


diff --git a/src/gallium/drivers/llvmpipe/lp_setup_tri.c b/src/gallium/drivers/llvmpipe/lp_setup_tri.c
index 324e938..39755d6 100644
--- a/src/gallium/drivers/llvmpipe/lp_setup_tri.c
+++ b/src/gallium/drivers/llvmpipe/lp_setup_tri.c
@@ -273,7 +273,9 @@ do_triangle_ccw(struct lp_setup_context *setup,
    const struct lp_setup_variant_key *key = &setup->setup.variant->key;
    struct lp_rast_triangle *tri;
    struct lp_rast_plane *plane;
-   struct u_rect bbox;
+   const struct u_rect *scissor;
+   struct u_rect bbox, bboxpos;
+   boolean s_planes[4];
    unsigned tri_bytes;
    int nr_planes = 3;
    unsigned viewport_index = 0;
@@ -332,12 +334,14 @@ do_triangle_ccw(struct lp_setup_context *setup,
       return TRUE;
    }

+   bboxpos = bbox;
+
    /* Can safely discard negative regions, but need to keep hold of
     * information about when the triangle extends past screen
     * boundaries.  See trimmed_box in lp_setup_bin_triangle().
     */
-   bbox.x0 = MAX2(bbox.x0, 0);
-   bbox.y0 = MAX2(bbox.y0, 0);
+   bboxpos.x0 = MAX2(bboxpos.x0, 0);
+   bboxpos.y0 = MAX2(bboxpos.y0, 0);

    nr_planes = 3;
    /*
@@ -346,8 +350,8 @@ do_triangle_ccw(struct lp_setup_context *setup,
     */
    if (setup->scissor_test) {
       /* why not just use draw_regions */
-      boolean s_planes[4];
-      scissor_planes_needed(s_planes, &bbox, &setup->scissors[viewport_index]);
+      scissor = &setup->scissors[viewport_index];
+      scissor_planes_needed(s_planes, &bboxpos, scissor);
       nr_planes += s_planes[0] + s_planes[1] + s_planes[2] + s_planes[3];
    }

@@ -680,10 +684,7 @@ do_triangle_ccw(struct lp_setup_context *setup,
     */
    if (nr_planes > 3) {
       /* why not just use draw_regions */
-      const struct u_rect *scissor = &setup->scissors[viewport_index];
       struct lp_rast_plane *plane_s = &plane[3];
-      boolean s_planes[4];
-      scissor_planes_needed(s_planes, &bbox, scissor);

       if (s_planes[0]) {
          plane_s->dcdx = -1 << 8;
@@ -716,7 +717,7 @@ do_triangle_ccw(struct lp_setup_context *setup,
       assert(plane_s == &plane[nr_planes]);
    }

-   return lp_setup_bin_triangle(setup, tri, &bbox, nr_planes, viewport_index);
+   return lp_setup_bin_triangle(setup, tri, &bbox, &bboxpos, nr_planes, viewport_index);
 }

 /*
@@ -747,11 +748,12 @@ floor_pot(uint32_t n)


 boolean
-lp_setup_bin_triangle( struct lp_setup_context *setup,
-                       struct lp_rast_triangle *tri,
-                       const struct u_rect *bbox,
-                       int nr_planes,
-                       unsigned viewport_index )
+lp_setup_bin_triangle(struct lp_setup_context *setup,
+                      struct lp_rast_triangle *tri,
+                      const struct u_rect *bboxorig,
+                      const struct u_rect *bbox,
+                      int nr_planes,
+                      unsigned viewport_index)
 {
    struct lp_scene *scene = setup->scene;
    struct u_rect trimmed_box = *bbox;
@@ -767,7 +769,16 @@ lp_setup_bin_triangle( struct lp_setup_context *setup,
    int max_sz = ((bbox->x1 - (bbox->x0 & ~3)) |
                  (bbox->y1 - (bbox->y0 & ~3)));
    int sz = floor_pot(max_sz);
-   boolean use_32bits = max_sz <= MAX_FIXED_LENGTH32;
+
+   /*
+    * NOTE: It is important to use the original bounding box
+    * which might contain negative values here, because if the
+    * plane math may overflow or not with the 32bit rasterization
+    * functions depends on the original extent of the triangle.
+    */
+   int max_szorig = ((bboxorig->x1 - (bboxorig->x0 & ~3)) |
+                     (bboxorig->y1 - (bboxorig->y0 & ~3)));
+   boolean use_32bits = max_szorig <= MAX_FIXED_LENGTH32;

    /* Now apply scissor, etc to the bounding box.  Could do this
     * earlier, but it confuses the logic for tri-16 and would force
--
2.7.4

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170623/ea425d65/attachment-0001.html>


More information about the mesa-dev mailing list