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

[Gurchetan Singh]

On Thu, Feb 28, 2019 at 8:15 PM Roland Scheidegger <sroland at vmware.com> wrote:
>
> 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...

virgl piggybacks off u_box_test_intersection_2d to do 1d adjacency +
intersection tests.  If the preferred approach is to split off an
u_box_test_intersection_1d function, that sounds fine.

>
> 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
> >
>