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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
