[Mesa-dev] [PATCH 1/2] util: update util_resource_copy_region() for GL_ARB_copy_image
Brian Paul
brianp at vmware.com
Mon Nov 2 10:53:43 PST 2015
On 11/02/2015 11:14 AM, Roland Scheidegger wrote:
> 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?
No, unused inline functions never warn.
>
>> +
>> +
>> /**
>> * 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.
I'll fix that. Though, IIRC, I was assuming that the region size would
be a multiple of the block size. It looks like we check for that in
_mesa_CopyImageSubData(). But that's not necessarily the case for small
mipmap levels. I think we'll need a new piglit test for that.
>
>> + }
>> + 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.
Yes.
>
>
>> +
>> + /* 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)?
Let me write a piglit test to check these corner cases. I'll update the
assertion then and post a v2.
Thanks for looking.
-Brian
>
>>
>> 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
>
More information about the mesa-dev
mailing list