Re: [PATCH v6 2/4] drm/rect: Add drm_rect_overlap()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 13/08/2024 16:11, Jani Nikula wrote:
On Mon, 12 Aug 2024, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
On Mon, 12 Aug 2024, Jocelyn Falempe <jfalempe@xxxxxxxxxx> wrote:
On 12/08/2024 15:49, Jani Nikula wrote:
On Mon, 12 Aug 2024, Jocelyn Falempe <jfalempe@xxxxxxxxxx> 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,

--

Jocelyn



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@xxxxxxxxxx>
---
   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);







[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux