Submitted by Boris Brezillon on Feb. 22, 2019, 1:19 p.m.

Message ID | 20190222131955.4046-1-boris.brezillon@collabora.com |
---|---|

State | New |

Headers | show |

Series |
"gallium/util: Fix off-by-one in box intersection"
( rev:
1
)
in
Mesa |

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

On Mon, Feb 25, 2019 at 12:35 AM Boris Brezillon <boris.brezillon@collabora.com> wrote: > > From: Daniel Stone <daniels@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). > > Signed-off-by: Daniel Stone <daniels@collabora.com> > Signed-off-by: Boris Brezillon <boris.brezillon@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Hello Gurchetan, On Wed, 27 Feb 2019 10:34:26 -0800 Gurchetan Singh <gurchetansingh@chromium.org> wrote: > On Mon, Feb 25, 2019 at 12:35 AM Boris Brezillon > <boris.brezillon@collabora.com> wrote: > > > > From: Daniel Stone <daniels@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. Regards, Boris [1]https://gitlab.collabora.com/bbrezillon/mesa/commits/etna-texture-atlas-18.2.4 > > > > > Signed-off-by: Daniel Stone <daniels@collabora.com> > > Signed-off-by: Boris Brezillon <boris.brezillon@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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev

On Thu, Feb 28, 2019 at 12:39 AM Boris Brezillon <boris.brezillon@collabora.com> wrote: > > Hello Gurchetan, > > On Wed, 27 Feb 2019 10:34:26 -0800 > Gurchetan Singh <gurchetansingh@chromium.org> wrote: > > > On Mon, Feb 25, 2019 at 12:35 AM Boris Brezillon > > <boris.brezillon@collabora.com> wrote: > > > > > > From: Daniel Stone <daniels@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. > > Regards, > > Boris > > [1]https://gitlab.collabora.com/bbrezillon/mesa/commits/etna-texture-atlas-18.2.4 > > > > > > > > > Signed-off-by: Daniel Stone <daniels@collabora.com> > > > Signed-off-by: Boris Brezillon <boris.brezillon@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@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev >

Am 01.03.19 um 00:28 schrieb Gurchetan Singh: > On Thu, Feb 28, 2019 at 12:39 AM Boris Brezillon > <boris.brezillon@collabora.com> wrote: >> >> Hello Gurchetan, >> >> On Wed, 27 Feb 2019 10:34:26 -0800 >> Gurchetan Singh <gurchetansingh@chromium.org> wrote: >> >>> On Mon, Feb 25, 2019 at 12:35 AM Boris Brezillon >>> <boris.brezillon@collabora.com> wrote: >>>> >>>> From: Daniel Stone <daniels@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@collabora.com> >>>> Signed-off-by: Boris Brezillon <boris.brezillon@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@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@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 >

On Thu, Feb 28, 2019 at 8:15 PM Roland Scheidegger <sroland@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@collabora.com> wrote: > >> > >> Hello Gurchetan, > >> > >> On Wed, 27 Feb 2019 10:34:26 -0800 > >> Gurchetan Singh <gurchetansingh@chromium.org> wrote: > >> > >>> On Mon, Feb 25, 2019 at 12:35 AM Boris Brezillon > >>> <boris.brezillon@collabora.com> wrote: > >>>> > >>>> From: Daniel Stone <daniels@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@collabora.com> > >>>> Signed-off-by: Boris Brezillon <boris.brezillon@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@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@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 > > >

Boris Brezillon <boris.brezillon@collabora.com> writes: > From: Daniel Stone <daniels@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. > > Signed-off-by: Daniel Stone <daniels@collabora.com> > Signed-off-by: Boris Brezillon <boris.brezillon@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]) All this min/maxing is about trying to find the bounds if width/height is negative (indicating a flip in blits), right? So, when width/height are negative, I think you end up expanding the range instead of shrinking it like you intended. (I think the negative width/height thing is a serious misfeature and we should just insist that pipe_boxes don't have it and fix up blits to pass the flip state separately) FWIW, I do definitely think this function should be fixed. It's called u_box_test_intersection, not u_box_test_intersection_or_adjacency.