| Message ID | 20240812123147.81356-3-jfalempe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panic: Add a QR code panic screen | expand |
On Mon, 12 Aug 2024, Jocelyn Falempe <jfalempe@redhat.com> wrote: > Check if two rectangles overlap. > It's a bit similar to drm_rect_intersect() but this won't modify > the rectangle. > Simplifies a bit drm_panic. Based on the name, I'd expect drm_rect_overlap() to return true for *any* overlap, while this one seems to mean if one rectangle is completely within another, with no adjacent borders. I'd expect a drm_rect_overlap() to return true for this: +-------+ | +---+---+ | | | +---+ | | | +-------+ While this seems to be required instead: +-------+ | +---+ | | | | | | +---+ | +-------+ IOW, I find the name misleading. BR, Jani. > > Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> > --- > drivers/gpu/drm/drm_panic.c | 3 +-- > include/drm/drm_rect.h | 15 +++++++++++++++ > 2 files changed, 16 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c > index 0a047152f88b8..59fba23e5fd7a 100644 > --- a/drivers/gpu/drm/drm_panic.c > +++ b/drivers/gpu/drm/drm_panic.c > @@ -529,8 +529,7 @@ static void draw_panic_static_user(struct drm_scanout_buffer *sb) > /* Fill with the background color, and draw text on top */ > drm_panic_fill(sb, &r_screen, bg_color); > > - if ((r_msg.x1 >= logo_width || r_msg.y1 >= logo_height) && > - logo_width <= sb->width && logo_height <= sb->height) { > + if (!drm_rect_overlap(&r_logo, &r_msg)) { > if (logo_mono) > drm_panic_blit(sb, &r_logo, logo_mono->data, DIV_ROUND_UP(logo_width, 8), > fg_color); > diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h > index 73fcb899a01da..7bafde747d560 100644 > --- a/include/drm/drm_rect.h > +++ b/include/drm/drm_rect.h > @@ -238,6 +238,21 @@ static inline void drm_rect_fp_to_int(struct drm_rect *dst, > drm_rect_height(src) >> 16); > } > > +/** > + * drm_rect_overlap - Check if two rectangles overlap > + * @r1: first rectangle > + * @r2: second rectangle > + * > + * RETURNS: > + * %true if the rectangles overlap, %false otherwise. > + */ > +static inline bool drm_rect_overlap(const struct drm_rect *r1, > + const struct drm_rect *r2) > +{ > + return (r1->x2 > r2->x1 && r2->x2 > r1->x1 && > + r1->y2 > r2->y1 && r2->y2 > r1->y1); > +} > + > bool drm_rect_intersect(struct drm_rect *r, const struct drm_rect *clip); > bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst, > const struct drm_rect *clip);
On 8/12/24 09:49, Jani Nikula wrote: > On Mon, 12 Aug 2024, Jocelyn Falempe <jfalempe@redhat.com> wrote: >> Check if two rectangles overlap. >> It's a bit similar to drm_rect_intersect() but this won't modify >> the rectangle. >> Simplifies a bit drm_panic. > > Based on the name, I'd expect drm_rect_overlap() to return true for > *any* overlap, while this one seems to mean if one rectangle is > completely within another, with no adjacent borders. > > I'd expect a drm_rect_overlap() to return true for this: > > +-------+ > | +---+---+ > | | | > +---+ | > | | > +-------+ > > While this seems to be required instead: > > +-------+ > | +---+ | > | | | | > | +---+ | > +-------+ > > > IOW, I find the name misleading. Ya, maybe drm_rect_encloses() would be a better fit. > > BR, > Jani. > > >> >> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> >> --- >> drivers/gpu/drm/drm_panic.c | 3 +-- >> include/drm/drm_rect.h | 15 +++++++++++++++ >> 2 files changed, 16 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c >> index 0a047152f88b8..59fba23e5fd7a 100644 >> --- a/drivers/gpu/drm/drm_panic.c >> +++ b/drivers/gpu/drm/drm_panic.c >> @@ -529,8 +529,7 @@ static void draw_panic_static_user(struct drm_scanout_buffer *sb) >> /* Fill with the background color, and draw text on top */ >> drm_panic_fill(sb, &r_screen, bg_color); >> >> - if ((r_msg.x1 >= logo_width || r_msg.y1 >= logo_height) && >> - logo_width <= sb->width && logo_height <= sb->height) { >> + if (!drm_rect_overlap(&r_logo, &r_msg)) { >> if (logo_mono) >> drm_panic_blit(sb, &r_logo, logo_mono->data, DIV_ROUND_UP(logo_width, 8), >> fg_color); >> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h >> index 73fcb899a01da..7bafde747d560 100644 >> --- a/include/drm/drm_rect.h >> +++ b/include/drm/drm_rect.h >> @@ -238,6 +238,21 @@ static inline void drm_rect_fp_to_int(struct drm_rect *dst, >> drm_rect_height(src) >> 16); >> } >> >> +/** >> + * drm_rect_overlap - Check if two rectangles overlap >> + * @r1: first rectangle >> + * @r2: second rectangle >> + * >> + * RETURNS: >> + * %true if the rectangles overlap, %false otherwise. If you do end up going with that name, the returns doc ought to be: %true if @r2 is completely enclosed in @r1, %false otherwise. >> + */ >> +static inline bool drm_rect_overlap(const struct drm_rect *r1, >> + const struct drm_rect *r2) >> +{ >> + return (r1->x2 > r2->x1 && r2->x2 > r1->x1 && >> + r1->y2 > r2->y1 && r2->y2 > r1->y1); >> +} >> + >> bool drm_rect_intersect(struct drm_rect *r, const struct drm_rect *clip); >> bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst, >> const struct drm_rect *clip); >
On 12/08/2024 15:49, Jani Nikula wrote: > On Mon, 12 Aug 2024, Jocelyn Falempe <jfalempe@redhat.com> wrote: >> Check if two rectangles overlap. >> It's a bit similar to drm_rect_intersect() but this won't modify >> the rectangle. >> Simplifies a bit drm_panic. > > Based on the name, I'd expect drm_rect_overlap() to return true for > *any* overlap, while this one seems to mean if one rectangle is > completely within another, with no adjacent borders. It's what I intended, but I may have messed up the formula. > > I'd expect a drm_rect_overlap() to return true for this: > > +-------+ > | +---+---+ > | | | > +---+ | > | | > +-------+ if r1 is the top left rectangle, you've got: r1->x2 > r2->x1 => true r2->x2 > r1->x1 => true r1->y2 > r2->y1 => true r2->y2 > r1->y1 => true So they count as overlap. Checking in stackoverflow, they use the same formula: https://stackoverflow.com/questions/306316/determine-if-two-rectangles-overlap-each-other > > While this seems to be required instead: > > +-------+ > | +---+ | > | | | | > | +---+ | > +-------+ > > > IOW, I find the name misleading. > > BR, > Jani. > > >> >> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> >> --- >> drivers/gpu/drm/drm_panic.c | 3 +-- >> include/drm/drm_rect.h | 15 +++++++++++++++ >> 2 files changed, 16 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c >> index 0a047152f88b8..59fba23e5fd7a 100644 >> --- a/drivers/gpu/drm/drm_panic.c >> +++ b/drivers/gpu/drm/drm_panic.c >> @@ -529,8 +529,7 @@ static void draw_panic_static_user(struct drm_scanout_buffer *sb) >> /* Fill with the background color, and draw text on top */ >> drm_panic_fill(sb, &r_screen, bg_color); >> >> - if ((r_msg.x1 >= logo_width || r_msg.y1 >= logo_height) && >> - logo_width <= sb->width && logo_height <= sb->height) { >> + if (!drm_rect_overlap(&r_logo, &r_msg)) { >> if (logo_mono) >> drm_panic_blit(sb, &r_logo, logo_mono->data, DIV_ROUND_UP(logo_width, 8), >> fg_color); >> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h >> index 73fcb899a01da..7bafde747d560 100644 >> --- a/include/drm/drm_rect.h >> +++ b/include/drm/drm_rect.h >> @@ -238,6 +238,21 @@ static inline void drm_rect_fp_to_int(struct drm_rect *dst, >> drm_rect_height(src) >> 16); >> } >> >> +/** >> + * drm_rect_overlap - Check if two rectangles overlap >> + * @r1: first rectangle >> + * @r2: second rectangle >> + * >> + * RETURNS: >> + * %true if the rectangles overlap, %false otherwise. >> + */ >> +static inline bool drm_rect_overlap(const struct drm_rect *r1, >> + const struct drm_rect *r2) >> +{ >> + return (r1->x2 > r2->x1 && r2->x2 > r1->x1 && >> + r1->y2 > r2->y1 && r2->y2 > r1->y1); >> +} >> + >> bool drm_rect_intersect(struct drm_rect *r, const struct drm_rect *clip); >> bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst, >> const struct drm_rect *clip); >
On Mon, 12 Aug 2024, Jocelyn Falempe <jfalempe@redhat.com> wrote: > On 12/08/2024 15:49, Jani Nikula wrote: >> On Mon, 12 Aug 2024, Jocelyn Falempe <jfalempe@redhat.com> wrote: >>> Check if two rectangles overlap. >>> It's a bit similar to drm_rect_intersect() but this won't modify >>> the rectangle. >>> Simplifies a bit drm_panic. >> >> Based on the name, I'd expect drm_rect_overlap() to return true for >> *any* overlap, while this one seems to mean if one rectangle is >> completely within another, with no adjacent borders. > > It's what I intended, but I may have messed up the formula. Hmm, then I may have messed up the review. :) Gotta run now, but I'll get back. BR, Jani. >> >> I'd expect a drm_rect_overlap() to return true for this: >> >> +-------+ >> | +---+---+ >> | | | >> +---+ | >> | | >> +-------+ > > if r1 is the top left rectangle, you've got: > > r1->x2 > r2->x1 => true > r2->x2 > r1->x1 => true > r1->y2 > r2->y1 => true > r2->y2 > r1->y1 => true > > So they count as overlap. > > Checking in stackoverflow, they use the same formula: > https://stackoverflow.com/questions/306316/determine-if-two-rectangles-overlap-each-other > >> >> While this seems to be required instead: >> >> +-------+ >> | +---+ | >> | | | | >> | +---+ | >> +-------+ >> >> >> IOW, I find the name misleading. >> >> BR, >> Jani. >> >> >>> >>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> >>> --- >>> drivers/gpu/drm/drm_panic.c | 3 +-- >>> include/drm/drm_rect.h | 15 +++++++++++++++ >>> 2 files changed, 16 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c >>> index 0a047152f88b8..59fba23e5fd7a 100644 >>> --- a/drivers/gpu/drm/drm_panic.c >>> +++ b/drivers/gpu/drm/drm_panic.c >>> @@ -529,8 +529,7 @@ static void draw_panic_static_user(struct drm_scanout_buffer *sb) >>> /* Fill with the background color, and draw text on top */ >>> drm_panic_fill(sb, &r_screen, bg_color); >>> >>> - if ((r_msg.x1 >= logo_width || r_msg.y1 >= logo_height) && >>> - logo_width <= sb->width && logo_height <= sb->height) { >>> + if (!drm_rect_overlap(&r_logo, &r_msg)) { >>> if (logo_mono) >>> drm_panic_blit(sb, &r_logo, logo_mono->data, DIV_ROUND_UP(logo_width, 8), >>> fg_color); >>> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h >>> index 73fcb899a01da..7bafde747d560 100644 >>> --- a/include/drm/drm_rect.h >>> +++ b/include/drm/drm_rect.h >>> @@ -238,6 +238,21 @@ static inline void drm_rect_fp_to_int(struct drm_rect *dst, >>> drm_rect_height(src) >> 16); >>> } >>> >>> +/** >>> + * drm_rect_overlap - Check if two rectangles overlap >>> + * @r1: first rectangle >>> + * @r2: second rectangle >>> + * >>> + * RETURNS: >>> + * %true if the rectangles overlap, %false otherwise. >>> + */ >>> +static inline bool drm_rect_overlap(const struct drm_rect *r1, >>> + const struct drm_rect *r2) >>> +{ >>> + return (r1->x2 > r2->x1 && r2->x2 > r1->x1 && >>> + r1->y2 > r2->y1 && r2->y2 > r1->y1); >>> +} >>> + >>> bool drm_rect_intersect(struct drm_rect *r, const struct drm_rect *clip); >>> bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst, >>> const struct drm_rect *clip); >> >
On Mon, 12 Aug 2024, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Mon, 12 Aug 2024, Jocelyn Falempe <jfalempe@redhat.com> wrote: >> On 12/08/2024 15:49, Jani Nikula wrote: >>> On Mon, 12 Aug 2024, Jocelyn Falempe <jfalempe@redhat.com> wrote: >>>> Check if two rectangles overlap. >>>> It's a bit similar to drm_rect_intersect() but this won't modify >>>> the rectangle. >>>> Simplifies a bit drm_panic. >>> >>> Based on the name, I'd expect drm_rect_overlap() to return true for >>> *any* overlap, while this one seems to mean if one rectangle is >>> completely within another, with no adjacent borders. >> >> It's what I intended, but I may have messed up the formula. > > Hmm, then I may have messed up the review. :) Yeah, my bad, sorry for the noise. I think I was thrown off by the comparisons mixing r1 and r2 as the first operand. Something like this might have been easier for *me* to parse, but not sure if it's worth changing anything: return (a->x1 < b->x2 && a->x2 > b->x1 && a->y1 < b->y2 && a->y2 > b->y1); BR, Jani. > > Gotta run now, but I'll get back. > > BR, > Jani. > > > >>> >>> I'd expect a drm_rect_overlap() to return true for this: >>> >>> +-------+ >>> | +---+---+ >>> | | | >>> +---+ | >>> | | >>> +-------+ >> >> if r1 is the top left rectangle, you've got: >> >> r1->x2 > r2->x1 => true >> r2->x2 > r1->x1 => true >> r1->y2 > r2->y1 => true >> r2->y2 > r1->y1 => true >> >> So they count as overlap. >> >> Checking in stackoverflow, they use the same formula: >> https://stackoverflow.com/questions/306316/determine-if-two-rectangles-overlap-each-other >> >>> >>> While this seems to be required instead: >>> >>> +-------+ >>> | +---+ | >>> | | | | >>> | +---+ | >>> +-------+ >>> >>> >>> IOW, I find the name misleading. >>> >>> BR, >>> Jani. >>> >>> >>>> >>>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> >>>> --- >>>> drivers/gpu/drm/drm_panic.c | 3 +-- >>>> include/drm/drm_rect.h | 15 +++++++++++++++ >>>> 2 files changed, 16 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c >>>> index 0a047152f88b8..59fba23e5fd7a 100644 >>>> --- a/drivers/gpu/drm/drm_panic.c >>>> +++ b/drivers/gpu/drm/drm_panic.c >>>> @@ -529,8 +529,7 @@ static void draw_panic_static_user(struct drm_scanout_buffer *sb) >>>> /* Fill with the background color, and draw text on top */ >>>> drm_panic_fill(sb, &r_screen, bg_color); >>>> >>>> - if ((r_msg.x1 >= logo_width || r_msg.y1 >= logo_height) && >>>> - logo_width <= sb->width && logo_height <= sb->height) { >>>> + if (!drm_rect_overlap(&r_logo, &r_msg)) { >>>> if (logo_mono) >>>> drm_panic_blit(sb, &r_logo, logo_mono->data, DIV_ROUND_UP(logo_width, 8), >>>> fg_color); >>>> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h >>>> index 73fcb899a01da..7bafde747d560 100644 >>>> --- a/include/drm/drm_rect.h >>>> +++ b/include/drm/drm_rect.h >>>> @@ -238,6 +238,21 @@ static inline void drm_rect_fp_to_int(struct drm_rect *dst, >>>> drm_rect_height(src) >> 16); >>>> } >>>> >>>> +/** >>>> + * drm_rect_overlap - Check if two rectangles overlap >>>> + * @r1: first rectangle >>>> + * @r2: second rectangle >>>> + * >>>> + * RETURNS: >>>> + * %true if the rectangles overlap, %false otherwise. >>>> + */ >>>> +static inline bool drm_rect_overlap(const struct drm_rect *r1, >>>> + const struct drm_rect *r2) >>>> +{ >>>> + return (r1->x2 > r2->x1 && r2->x2 > r1->x1 && >>>> + r1->y2 > r2->y1 && r2->y2 > r1->y1); >>>> +} >>>> + >>>> bool drm_rect_intersect(struct drm_rect *r, const struct drm_rect *clip); >>>> bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst, >>>> const struct drm_rect *clip); >>> >>
On 13/08/2024 16:11, Jani Nikula wrote: > On Mon, 12 Aug 2024, Jani Nikula <jani.nikula@linux.intel.com> wrote: >> On Mon, 12 Aug 2024, Jocelyn Falempe <jfalempe@redhat.com> wrote: >>> On 12/08/2024 15:49, Jani Nikula wrote: >>>> On Mon, 12 Aug 2024, Jocelyn Falempe <jfalempe@redhat.com> wrote: >>>>> Check if two rectangles overlap. >>>>> It's a bit similar to drm_rect_intersect() but this won't modify >>>>> the rectangle. >>>>> Simplifies a bit drm_panic. >>>> >>>> Based on the name, I'd expect drm_rect_overlap() to return true for >>>> *any* overlap, while this one seems to mean if one rectangle is >>>> completely within another, with no adjacent borders. >>> >>> It's what I intended, but I may have messed up the formula. >> >> Hmm, then I may have messed up the review. :) > > Yeah, my bad, sorry for the noise. > > I think I was thrown off by the comparisons mixing r1 and r2 as the > first operand. Something like this might have been easier for *me* to > parse, but not sure if it's worth changing anything: > > return (a->x1 < b->x2 && a->x2 > b->x1 && > a->y1 < b->y2 && a->y2 > b->y1); You're right, I've used r1 and r2 for consistency with other functions, but for this case it's confusing, I prefer the a/b. If I send a v7 I will do this change. I can also rename when pushing, but I was already bitten by doing this. Best regards,
diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index 0a047152f88b8..59fba23e5fd7a 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -529,8 +529,7 @@ static void draw_panic_static_user(struct drm_scanout_buffer *sb) /* Fill with the background color, and draw text on top */ drm_panic_fill(sb, &r_screen, bg_color); - if ((r_msg.x1 >= logo_width || r_msg.y1 >= logo_height) && - logo_width <= sb->width && logo_height <= sb->height) { + if (!drm_rect_overlap(&r_logo, &r_msg)) { if (logo_mono) drm_panic_blit(sb, &r_logo, logo_mono->data, DIV_ROUND_UP(logo_width, 8), fg_color); diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h index 73fcb899a01da..7bafde747d560 100644 --- a/include/drm/drm_rect.h +++ b/include/drm/drm_rect.h @@ -238,6 +238,21 @@ static inline void drm_rect_fp_to_int(struct drm_rect *dst, drm_rect_height(src) >> 16); } +/** + * drm_rect_overlap - Check if two rectangles overlap + * @r1: first rectangle + * @r2: second rectangle + * + * RETURNS: + * %true if the rectangles overlap, %false otherwise. + */ +static inline bool drm_rect_overlap(const struct drm_rect *r1, + const struct drm_rect *r2) +{ + return (r1->x2 > r2->x1 && r2->x2 > r1->x1 && + r1->y2 > r2->y1 && r2->y2 > r1->y1); +} + bool drm_rect_intersect(struct drm_rect *r, const struct drm_rect *clip); bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst, const struct drm_rect *clip);
Check if two rectangles overlap. It's a bit similar to drm_rect_intersect() but this won't modify the rectangle. Simplifies a bit drm_panic. Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> --- drivers/gpu/drm/drm_panic.c | 3 +-- include/drm/drm_rect.h | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-)