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

[Brian Paul]

On 10/25/2017 09:54 AM, Roland Scheidegger wrote:
> 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.

OK, just remove the assertions then.

-Brian