[Mesa-dev] [PATCH 2/6] intel/blorp_blit: Split blorp blits if they are too large
Jason Ekstrand
jason at jlekstrand.net
Wed Nov 23 22:04:15 UTC 2016
On Tue, Nov 22, 2016 at 7:03 PM, Jordan Justen <jordan.l.justen at intel.com>
wrote:
> We rename do_blorp_blit() to try_blorp_blit(), and add a return error
> if the surface size for the blit is too large. Now, do_blorp_blit() is
> rewritten to try to split the blit into smaller operations if
> try_blorp_blit() fails.
>
> Note: In this commit, try_blorp_blit() will always attempt to blit and
> never return an error, which matches the previous behavior. We will
> enable the size checking and splitting in a future commit.
>
> The motivation for this splitting is that in some cases when we
> flatten an image, it's dimensions grow, and this can then exceed the
> programmable hardware limits. An example is w-tiled+MSAA blits.
>
> Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
> ---
> src/intel/blorp/blorp_blit.c | 93 ++++++++++++++++++++++++++++++
> +++++++++++---
> 1 file changed, 87 insertions(+), 6 deletions(-)
>
> diff --git a/src/intel/blorp/blorp_blit.c b/src/intel/blorp/blorp_blit.c
> index 24c4fd6..caa3e33 100644
> --- a/src/intel/blorp/blorp_blit.c
> +++ b/src/intel/blorp/blorp_blit.c
> @@ -1530,11 +1530,18 @@ struct blt_coords {
> struct blt_axis x, y;
> };
>
> -static void
> -do_blorp_blit(struct blorp_batch *batch,
> - struct blorp_params *params,
> - struct brw_blorp_blit_prog_key *wm_prog_key,
> - const struct blt_coords *coords)
> +#define BLIT_WIDTH_TOO_LARGE 1
> +#define BLIT_HEIGHT_TOO_LARGE 2
>
Mind making this an enum? That way GDB will print out BLIT_WIDTH_TOO_LARGE
| BLIT_HEIGHT_TOO_LARGE when your debugging.
> +
> +/* Try to blit. If the surface parameters exceed the size allowed by
> hardware,
> + * then a bitmask of BLIT_WIDTH_TOO_LARGE and BLIT_HEIGHT_TOO_LARGE will
> be
> + * returned. If 0 is returned, then the blit was successful.
> + */
> +static unsigned
> +try_blorp_blit(struct blorp_batch *batch,
> + struct blorp_params *params,
> + struct brw_blorp_blit_prog_key *wm_prog_key,
> + const struct blt_coords *coords)
> {
> const struct gen_device_info *devinfo = batch->blorp->isl_dev->info;
>
> @@ -1737,7 +1744,81 @@ do_blorp_blit(struct blorp_batch *batch,
>
> brw_blorp_get_blit_kernel(batch->blorp, params, wm_prog_key);
>
> - batch->blorp->exec(batch, params);
> + unsigned result = 0;
> +
> + if (result == 0) {
> + batch->blorp->exec(batch, params);
> + }
> +
> + return result;
> +}
> +
> +/* Adjust split blit source coordinates for the current destination
> + * coordinates.
> + */
> +static void
> +adjust_split_coords(const struct blt_axis *orig,
> + struct blt_axis *split_coords,
> + float scale)
> +{
> + float delta0 = scale * (split_coords->dst0 - orig->dst0);
> + float delta1 = scale * (split_coords->dst1 - orig->dst1);
> + split_coords->src0 = orig->src0 + (scale >= 0.0 ? delta0 : delta1);
> + split_coords->src1 = orig->src1 + (scale >= 0.0 ? delta1 : delta0);
>
Ouch... My head hurts... I believe this is probably correct. Flips make
all of this so much harder to think about.
> +}
> +
> +static void
> +do_blorp_blit(struct blorp_batch *batch,
> + struct blorp_params *params,
> + struct brw_blorp_blit_prog_key *wm_prog_key,
> + const struct blt_coords *orig)
> +{
> + struct blt_coords split_coords = *orig;
> + float w = orig->x.dst1 - orig->x.dst0;
> + float h = orig->y.dst1 - orig->y.dst0;
> + float x_scale = (orig->x.src1 - orig->x.src0) / w;
> + float y_scale = (orig->y.src1 - orig->y.src0) / h;
>
I've discovered in the past that these calculations can be very sensitive
to precision. I'd recommend doubles instead of floats. See also
fa4627149dfe7cdb9f75d8e2f1bcaf1ad7006801
> + if (orig->x.mirror)
> + x_scale = -x_scale;
> + if (orig->y.mirror)
> + y_scale = -y_scale;
> +
> + bool x_done, y_done;
> + do {
> + unsigned result =
> + try_blorp_blit(batch, params, wm_prog_key, &split_coords);
> +
> + if (result & BLIT_WIDTH_TOO_LARGE) {
> + w /= 2.0;
> + assert(w >= 1.0);
> + split_coords.x.dst1 = MIN2(split_coords.x.dst0 + w,
> orig->x.dst1);
> + adjust_split_coords(&orig->x, &split_coords.x, x_scale);
> + }
> + if (result & BLIT_HEIGHT_TOO_LARGE) {
> + h /= 2.0;
> + assert(h >= 1.0);
> + split_coords.y.dst1 = MIN2(split_coords.y.dst0 + h,
> orig->y.dst1);
> + adjust_split_coords(&orig->y, &split_coords.y, y_scale);
> + }
> +
> + if (result != 0)
> + continue;
> +
> + y_done = (orig->y.dst1 - split_coords.y.dst1 < 0.5);
> + x_done = y_done && (orig->x.dst1 - split_coords.x.dst1 < 0.5);
> + if (x_done) {
> + break;
> + } else if (y_done) {
> + split_coords.x.dst0 += w;
> + split_coords.x.dst1 = MIN2(split_coords.x.dst0 + w,
> orig->x.dst1);
> + split_coords.y.dst0 = orig->y.dst0;
> + adjust_split_coords(&orig->x, &split_coords.x, x_scale);
> + } else {
> + split_coords.y.dst0 += h;
> + split_coords.y.dst1 = MIN2(split_coords.y.dst0 + h,
> orig->y.dst1);
> + adjust_split_coords(&orig->y, &split_coords.y, y_scale);
> + }
> + } while (true);
>
I applaud you for figuring out how to do this with a simple loop! I would
have probably been lazy and written it recursively:
do_blorp_blit(batch, params, key, orig, splits)
{
if (splits & BLIT_WIDTH_TOO_LARGE) {
/* Adjust for left half */
do_blorp_blit(batch, params, key, orig, splits &
~BLIT_WIDTH_TOO_LARGE);
/* Adjust for right half */
do_blorp_blit(batch, params, key, orig, splits &
~BLIT_WIDTH_TOO_LARGE);
} else if (splits & BLIT_HEIGHT_TOO_LARGE) {
/* Adjust for top half */
do_blorp_blit(batch, params, key, orig, 0);
/* Adjust for bottom half */
do_blorp_blit(batch, params, key, orig, 0);
} else {
assert(splits == 0);
splits = try_blorp_blit();
if (splits)
do_blorp_blit(batch, params, key, orig, splits);
}
}
I find that it's easier to reason about these sorts of things with
recursion. It just seems like it'd be easier to do the calculations if you
were only ever cutting things in half. Take it or leave it. I believe
your code is correct modulo some confusion around adjust_split_coords().
> }
>
> void
> --
> 2.10.2
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20161123/0244062e/attachment.html>
More information about the mesa-dev
mailing list