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

[Roland Scheidegger]

Am 25.10.2017 um 16:57 schrieb 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...)

Err, I just realized this doesn't even work. If the dst block compressed
texture for instance is 5x5 and the src larger, it will wrongly trigger
all the same.

Roland