[Mesa-dev] [PATCH] gallium/util: Fix off-by-one in box intersection

[Roland Scheidegger]

Am 01.03.19 um 00:28 schrieb Gurchetan Singh:
> On Thu, Feb 28, 2019 at 12:39 AM Boris Brezillon
> <boris.brezillon at collabora.com> wrote:
>>
>> Hello Gurchetan,
>>
>> On Wed, 27 Feb 2019 10:34:26 -0800
>> Gurchetan Singh <gurchetansingh at chromium.org> wrote:
>>
>>> On Mon, Feb 25, 2019 at 12:35 AM Boris Brezillon
>>> <boris.brezillon at collabora.com> wrote:
>>>>
>>>> From: Daniel Stone <daniels at collabora.com>
>>>>
>>>> pipe_boxes are x/y + width/height, rather than x0/y0 -> x1/y1. This
>>>> means that (x+width) is not included in the box.
>>>>
>>>> The box intersection check was seemingly written for inclusive regions,
>>>> and would falsely assert that adjacent boxes would overlap.
>>>>
>>>> Fix the off-by-one by being one pixel less greedy.
>>>
>>> Is there a reason for this change?  I only see this used in a warning
>>> in the nine state tracker and virgl (where reporting adjacent
>>> intersections is preferred).
>>
>> This patch was part of a series Daniel worked on to optimize texture
>> atlas updates on Vivante GPUs [1]. In the end, this work has been put
>> on hold because the perf optimization was not as high as expected, but
>> it might be resurrected at some point.
>> Anyway, back to the point. In this patchset, the pipe_region_overlaps()
>> helper needs to check when regions overlap and not when they're
>> adjacent. If other users need u_box_test_intersection_2d() to also
>> detect when boxes are adjacent, then we should definitely keep the code
>> unchanged, but maybe it's worth a comment in the code to clarify the
>> behavior.
> 
> Thanks for the information.  You can just modify this function to be
> something like:
> 
> u_box_test_intersection_2d(const struct pipe_box *a, const struct
> pipe_box *b, boolean adjacent_allowed)
> 
> [or add another function --- whatever you prefer]
> 
> That way we can keep behavior for virgl/nine unchanged.

I can't see why you'd want to know if the regions are adjacent?
If they are adjacent you can still do blits etc. without having to worry
about overwriting src regions etc.
Now for 1d regions (buffers) I could see adjacent being useful - could
use that to combine multiple ranges into one for instance. But I don't
think you'd want to use a 2d intersect test for that...

Roland



> 
>>
>> Regards,
>>
>> Boris
>>
>> [1]https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.collabora.com%2Fbbrezillon%2Fmesa%2Fcommits%2Fetna-texture-atlas-18.2.4&data=02%7C01%7Csroland%40vmware.com%7Ce72daea7c212452556f208d69dd47aa1%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636869933286320567&sdata=lHnhl1gM19Gt%2FU3KVv%2FlpBgPXFoSl4BqwZ93yHgbbRQ%3D&reserved=0
>>
>>>
>>>>
>>>> Signed-off-by: Daniel Stone <daniels at collabora.com>
>>>> Signed-off-by: Boris Brezillon <boris.brezillon at collabora.com>
>>>> ---
>>>>  src/gallium/auxiliary/util/u_box.h | 16 ++++++++--------
>>>>  1 file changed, 8 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/src/gallium/auxiliary/util/u_box.h b/src/gallium/auxiliary/util/u_box.h
>>>> index b3f478e7bfc4..ead7189ecaf8 100644
>>>> --- a/src/gallium/auxiliary/util/u_box.h
>>>> +++ b/src/gallium/auxiliary/util/u_box.h
>>>> @@ -161,15 +161,15 @@ u_box_test_intersection_2d(const struct pipe_box *a,
>>>>     unsigned i;
>>>>     int a_l[2], a_r[2], b_l[2], b_r[2];
>>>>
>>>> -   a_l[0] = MIN2(a->x, a->x + a->width);
>>>> -   a_r[0] = MAX2(a->x, a->x + a->width);
>>>> -   a_l[1] = MIN2(a->y, a->y + a->height);
>>>> -   a_r[1] = MAX2(a->y, a->y + a->height);
>>>> +   a_l[0] = MIN2(a->x, a->x + a->width - 1);
>>>> +   a_r[0] = MAX2(a->x, a->x + a->width - 1);
>>>> +   a_l[1] = MIN2(a->y, a->y + a->height - 1);
>>>> +   a_r[1] = MAX2(a->y, a->y + a->height - 1);
>>>>
>>>> -   b_l[0] = MIN2(b->x, b->x + b->width);
>>>> -   b_r[0] = MAX2(b->x, b->x + b->width);
>>>> -   b_l[1] = MIN2(b->y, b->y + b->height);
>>>> -   b_r[1] = MAX2(b->y, b->y + b->height);
>>>> +   b_l[0] = MIN2(b->x, b->x + b->width - 1);
>>>> +   b_r[0] = MAX2(b->x, b->x + b->width - 1);
>>>> +   b_l[1] = MIN2(b->y, b->y + b->height - 1);
>>>> +   b_r[1] = MAX2(b->y, b->y + b->height - 1);
>>>>
>>>>     for (i = 0; i < 2; ++i) {
>>>>        if (a_l[i] > b_r[i] || a_r[i] < b_l[i])
>>>> --
>>>> 2.20.1
>>>>
>>>> _______________________________________________
>>>> mesa-dev mailing list
>>>> mesa-dev at lists.freedesktop.org
>>>> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fmesa-dev&data=02%7C01%7Csroland%40vmware.com%7Ce72daea7c212452556f208d69dd47aa1%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636869933286320567&sdata=%2FSECNIFewcH6gECXxq94DXvX6QfN8PEKpDQd3h%2Boxz8%3D&reserved=0
>>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fmesa-dev&data=02%7C01%7Csroland%40vmware.com%7Ce72daea7c212452556f208d69dd47aa1%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636869933286320567&sdata=%2FSECNIFewcH6gECXxq94DXvX6QfN8PEKpDQd3h%2Boxz8%3D&reserved=0
>