[Mesa-dev] [PATCH 1/2] util: update util_resource_copy_region() for GL_ARB_copy_image

[Roland Scheidegger]

Am 31.10.2015 um 14:37 schrieb Brian Paul:
> This primarily means added support for copying between compressed
> and uncompressed formats.
> ---
>  src/gallium/auxiliary/util/u_surface.c | 106 +++++++++++++++++++++++++++------
>  1 file changed, 88 insertions(+), 18 deletions(-)
> 
> diff --git a/src/gallium/auxiliary/util/u_surface.c b/src/gallium/auxiliary/util/u_surface.c
> index 70ed911..5c32876 100644
> --- a/src/gallium/auxiliary/util/u_surface.c
> +++ b/src/gallium/auxiliary/util/u_surface.c
> @@ -238,8 +238,21 @@ util_fill_box(ubyte * dst,
>  }
>  
>  
> +/** Mipmap level size computation, with minimum block size */
> +static inline unsigned
> +minify(unsigned value, unsigned levels, unsigned blocksize)
> +{
> +    return MAX2(blocksize, value >> levels);
> +}
Won't this result in a warning of unused function in release builds?

> +
> +
>  /**
>   * Fallback function for pipe->resource_copy_region().
> + * We support copying between different formats (including compressed/
> + * uncompressed) if the bytes per block or pixel matches.  If copying
> + * compressed -> uncompressed, the dst region is reduced by the block
> + * width, height.  If copying uncompressed -> compressed, the dest region
> + * is expanded by the block width, height.  See GL_ARB_copy_image.
>   * Note: (X,Y)=(0,0) is always the upper-left corner.
>   */
>  void
> @@ -249,13 +262,14 @@ util_resource_copy_region(struct pipe_context *pipe,
>                            unsigned dst_x, unsigned dst_y, unsigned dst_z,
>                            struct pipe_resource *src,
>                            unsigned src_level,
> -                          const struct pipe_box *src_box)
> +                          const struct pipe_box *src_box_in)
>  {
>     struct pipe_transfer *src_trans, *dst_trans;
>     uint8_t *dst_map;
>     const uint8_t *src_map;
>     enum pipe_format src_format, dst_format;
> -   struct pipe_box dst_box;
> +   struct pipe_box src_box, dst_box;
> +   unsigned src_bs, dst_bs, src_bw, dst_bw, src_bh, dst_bh;
>  
>     assert(src && dst);
>     if (!src || !dst)
> @@ -267,27 +281,83 @@ util_resource_copy_region(struct pipe_context *pipe,
>     src_format = src->format;
>     dst_format = dst->format;
>  
> -   assert(util_format_get_blocksize(dst_format) == util_format_get_blocksize(src_format));
> -   assert(util_format_get_blockwidth(dst_format) == util_format_get_blockwidth(src_format));
> -   assert(util_format_get_blockheight(dst_format) == util_format_get_blockheight(src_format));
> +   /* init src box */
> +   src_box = *src_box_in;
> +
> +   /* init dst box */
> +   dst_box.x = dst_x;
> +   dst_box.y = dst_y;
> +   dst_box.z = dst_z;
> +   dst_box.width  = src_box.width;
> +   dst_box.height = src_box.height;
> +   dst_box.depth  = src_box.depth;
> +
> +   src_bs = util_format_get_blocksize(src_format);
> +   src_bw = util_format_get_blockwidth(src_format);
> +   src_bh = util_format_get_blockheight(src_format);
> +   dst_bs = util_format_get_blocksize(dst_format);
> +   dst_bw = util_format_get_blockwidth(dst_format);
> +   dst_bh = util_format_get_blockheight(dst_format);
> +
> +   /* Note: all box positions and sizes are in pixels */
> +   if (src_bw > 1 && dst_bw == 1) {
> +      /* Copy from compressed to uncompressed.
> +       * Shrink dest box by the src block size.
> +       */
> +      dst_box.width /= src_bw;
> +      dst_box.height /= src_bh;
This doesn't quite look right to me. If width/height wasn't a full block
size (for small mips, or npot texture), it will round down instead of up.

> +   }
> +   else if (src_bw == 1 && dst_bw > 1) {
> +      /* Copy from uncompressed to compressed.
> +       * Expand dest box by the dest block size.
> +       */
> +      dst_box.width *= dst_bw;
> +      dst_box.height *= dst_bh;
> +   }
> +   else {
> +      /* compressed -> compressed or uncompressed -> uncompressed copy */
> +      assert(src_bw == dst_bw);
> +      assert(src_bh == dst_bh);
> +   }
> +
> +   assert(src_bs == dst_bs);
> +   if (src_bs != dst_bs) {
> +      /* This can happen if we fail to do format checking before hand.
> +       * Don't crash below.
> +       */
> +      return;
> +   }
> +
> +   /* check that region boxes are block aligned */
> +   assert(src_box.x % src_bw == 0);
> +   assert(src_box.y % src_bh == 0);
> +   assert(src_box.width % src_bw == 0 || src_box.width < src_bw);
> +   assert(src_box.height % src_bh == 0 || src_box.height < src_bh);
> +   assert(dst_box.x % dst_bw == 0);
> +   assert(dst_box.y % dst_bh == 0);
> +   assert(dst_box.width % dst_bw == 0 || dst_box.width < dst_bw);
> +   assert(dst_box.height % dst_bh == 0 || dst_box.height < src_bh);
the last term should be dst_bh.


> +
> +   /* check that region boxes are not out of bounds */
> +   assert(src_box.x + src_box.width <= minify(src->width0, src_level, src_bw));
> +   assert(src_box.y + src_box.height <= minify(src->height0, src_level, src_bh));
> +   assert(dst_box.x + dst_box.width <= minify(dst->width0, dst_level, dst_bw));
> +   assert(dst_box.y + dst_box.height <= minify(dst->height0, dst_level, dst_bh));
> +
> +   /* check that total number of src, dest bytes match */
> +   assert((src_box.width / src_bw) * (src_box.height / src_bh) * src_bs ==
> +          (dst_box.width / dst_bw) * (dst_box.height / dst_bh) * dst_bs);
This assertion is imho a bit confusing. You've already asserted src_bs
== dst_bs, so this part seems redundant.
And, some values can be 0 here even (the dst ones), albeit that's a
result for rounding down, above. But, I think the assert should round up
all individual block terms to block size in any case (this is what the
actual util_copy_box should do, too). Should only be a problem for
compressed->uncompressed though I think, otherwise it should already be
sufficiently aligned (maybe things would be less confusing with seperate
assertions for the different cases)?

>  
>     src_map = pipe->transfer_map(pipe,
>                                  src,
>                                  src_level,
>                                  PIPE_TRANSFER_READ,
> -                                src_box, &src_trans);
> +                                &src_box, &src_trans);
>     assert(src_map);
>     if (!src_map) {
>        goto no_src_map;
>     }
>  
> -   dst_box.x = dst_x;
> -   dst_box.y = dst_y;
> -   dst_box.z = dst_z;
> -   dst_box.width  = src_box->width;
> -   dst_box.height = src_box->height;
> -   dst_box.depth  = src_box->depth;
> -
>     dst_map = pipe->transfer_map(pipe,
>                                  dst,
>                                  dst_level,
> @@ -299,15 +369,15 @@ util_resource_copy_region(struct pipe_context *pipe,
>     }
>  
>     if (dst->target == PIPE_BUFFER && src->target == PIPE_BUFFER) {
> -      assert(src_box->height == 1);
> -      assert(src_box->depth == 1);
> -      memcpy(dst_map, src_map, src_box->width);
> +      assert(src_box.height == 1);
> +      assert(src_box.depth == 1);
> +      memcpy(dst_map, src_map, src_box.width);
>     } else {
>        util_copy_box(dst_map,
> -                    dst_format,
> +                    src_format,
>                      dst_trans->stride, dst_trans->layer_stride,
>                      0, 0, 0,
> -                    src_box->width, src_box->height, src_box->depth,
> +                    src_box.width, src_box.height, src_box.depth,
>                      src_map,
>                      src_trans->stride, src_trans->layer_stride,
>                      0, 0, 0);
> 

Otherwise looks good.

Roland