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

[Jocelyn Falempe <jfalempe@redhat.com>]

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