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