[v6] drm/panic: Add a QR code panic screen

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

Posted by Jocelyn Falempe 1 year, 5 months ago

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

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);
-- 
2.46.0

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

Posted by Jani Nikula 1 year, 5 months ago

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

-- 
Jani Nikula, Intel

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

Posted by Jocelyn Falempe 1 year, 5 months ago


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

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

Posted by Jani Nikula 1 year, 5 months ago

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

-- 
Jani Nikula, Intel

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

Posted by Jani Nikula 1 year, 5 months ago

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

-- 
Jani Nikula, Intel

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

Posted by Jocelyn Falempe 1 year, 5 months ago

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,

-- 

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

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

Posted by Hamza Mahfooz 1 year, 5 months ago

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);
> 
-- 
Hamza