[Mesa-dev] [PATCH 2/2] llvmpipe: fix using 32bit rasterization mistakenly, causing overflows
Brian Paul
brianp at vmware.com
Fri Jun 23 15:50:43 UTC 2017
For both,
Reviewed-by: Brian Paul <brianp at vmware.com>
On 06/23/2017 09:33 AM, sroland at vmware.com wrote:
> 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
>
More information about the mesa-dev
mailing list