[Mesa-dev] [PATCH] gallium/util: remove some block alignment assertions

[Roland Scheidegger]

Am 25.10.2017 um 16:29 schrieb Brian Paul:
> On 10/24/2017 07:06 PM, sroland at vmware.com wrote:
>> From: Roland Scheidegger <sroland at vmware.com>
>>
>> These assertions were revisited a couple of times in the past, and they
>> still weren't quite right.
>> The problem I was seeing (with some other state tracker) was a copy
>> between
>> two 512x512 s3tc textures, but from mip level 0 to mip level 8.
>> Therefore,
>> the destination has only size 2x2 (not a full block), so the box
>> width/height
>> was only 2, causing the assertion to trigger for src alignment.
>> As far as I can tell, such a copy is completely legal, and because a
>> correct
>> assertion would get ridiculously complicated just get rid of it for good.
>> ---
>>   src/gallium/auxiliary/util/u_surface.c | 8 --------
>>   1 file changed, 8 deletions(-)
>>
>> diff --git a/src/gallium/auxiliary/util/u_surface.c
>> b/src/gallium/auxiliary/util/u_surface.c
>> index 5abf966..0a79a25 100644
>> --- a/src/gallium/auxiliary/util/u_surface.c
>> +++ b/src/gallium/auxiliary/util/u_surface.c
>> @@ -324,16 +324,8 @@ util_resource_copy_region(struct pipe_context *pipe,
>>      /* 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.x + src_box.width == u_minify(src->width0,
>> src_level));
>> -   assert(src_box.height % src_bh == 0 ||
>> -          src_box.y + src_box.height == u_minify(src->height0,
>> src_level));
>>      assert(dst_box.x % dst_bw == 0);
>>      assert(dst_box.y % dst_bh == 0);
>> -   assert(dst_box.width % dst_bw == 0 ||
>> -          dst_box.x + dst_box.width == u_minify(dst->width0,
>> dst_level));
>> -   assert(dst_box.height % dst_bh == 0 ||
>> -          dst_box.y + dst_box.height == u_minify(dst->height0,
>> dst_level));
>>
>>      /* check that region boxes are not out of bounds */
>>      assert(src_box.x + src_box.width <= u_minify(src->width0,
>> src_level));
>>
> 
> Would one alternative be to put the assertions inside a conditional
> testing that the texture is at least 4x4?  That'd keep the checking in
> place for common cases.

You mean like
if (u_minify(dst->width0, dst_level) >= dst_bw) {
   assert(src_box.width % src_bw == 0 ||
          src_box.x + src_box.width == u_minify(src->width0, src_level)
}
and same for height?
And (maybe) the same for the other direction (I'm not quite sure if the
opposite direction actually should be legal - if you copy a 2x2 s3tc
texture to a larger one, you'll end up with undefined pixels albeit
certainly they will be encoded in the partial source block).
I thought about this but figured ultimately the assertions have more
complexity than the actual code. (And it's not that it is really
critical assertions, since whole blocks should always be copied in any
case, it's just that is is usually illegal to specify non-full blocks,
except there's tons of exceptions...)

Roland



> Either way,
> Reviewed-by: Brian Paul <brianp at vmware.com>
>