Re: [PATCH 2/2] drm/rect: Keep the clipped dst rectangle in place

From: Benjamin GAIGNARD <benjamin.gaignard@st.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>,
	"Daniel Vetter" <daniel@ffwll.ch>
Cc: "intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 2/2] drm/rect: Keep the clipped dst rectangle in place
Date: Wed, 20 Nov 2019 19:55:43 +0000	[thread overview]
Message-ID: <c936c1e3-b045-4dd6-8216-ade0da19f015@st.com> (raw)
In-Reply-To: <20191120171138.GI1208@intel.com>


On 11/20/19 6:11 PM, Ville Syrjälä wrote:
> On Wed, Nov 20, 2019 at 05:43:40PM +0100, Daniel Vetter wrote:
>> On Wed, Nov 20, 2019 at 06:25:12PM +0200, Ville Syrjala wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> Now that we've constrained the clipped source rectangle such
>>> that it can't have negative dimensions doing the same for the
>>> dst rectangle seems appropriate. Should at least result in
>>> the clipped src and dst rectangles being a bit more consistent
>>> with each other.
>>>
>>> Cc: Benjamin Gaignard <benjamin.gaignard@st.com>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> selftests for this stuff? Looks like the prime example, write testcase
>> proving code is busted, fix it, everyone celebrate?
> Yeah, seems like a good idea. Though I'll have to figure out if it's
> actually broken or not ;)
>
> Hmm. Ouch. There's seems to be a div by zero lurking in there if
> dst_w/h == 0. I wonder why nothing has hit that.

At least W=1 warnings have disappear with these patches ;-)

Benjamin

>> -Daniel
>>
>>> ---
>>>   drivers/gpu/drm/drm_rect.c | 22 +++++++++++-----------
>>>   1 file changed, 11 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
>>> index 7762b6e9278d..229325fcf333 100644
>>> --- a/drivers/gpu/drm/drm_rect.c
>>> +++ b/drivers/gpu/drm/drm_rect.c
>>> @@ -52,14 +52,14 @@ bool drm_rect_intersect(struct drm_rect *r1, const struct drm_rect *r2)
>>>   }
>>>   EXPORT_SYMBOL(drm_rect_intersect);
>>>   
>>> -static u32 clip_scaled(u32 src, u32 dst, u32 clip)
>>> +static u32 clip_scaled(int src, int dst, int *clip)
>>>   {
>>>   	u64 tmp;
>>>   
>>>   	/* Only clip what we have. Keeps the result bounded as well. */
>>> -	clip = min(clip, dst);
>>> +	*clip = min(*clip, dst);
>>>   
>>> -	tmp = mul_u32_u32(src, dst - clip);
>>> +	tmp = mul_u32_u32(src, dst - *clip);
>>>   
>>>   	/*
>>>   	 * Round toward 1.0 when clipping so that we don't accidentally
>>> @@ -92,34 +92,34 @@ bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst,
>>>   	diff = clip->x1 - dst->x1;
>>>   	if (diff > 0) {
>>>   		u32 new_src_w = clip_scaled(drm_rect_width(src),
>>> -					    drm_rect_width(dst), diff);
>>> +					    drm_rect_width(dst), &diff);
>>>   
>>>   		src->x1 = src->x2 - new_src_w;
>>> -		dst->x1 = clip->x1;
>>> +		dst->x1 += diff;
>>>   	}
>>>   	diff = clip->y1 - dst->y1;
>>>   	if (diff > 0) {
>>>   		u32 new_src_h = clip_scaled(drm_rect_height(src),
>>> -					    drm_rect_height(dst), diff);
>>> +					    drm_rect_height(dst), &diff);
>>>   
>>>   		src->y1 = src->y2 - new_src_h;
>>> -		dst->y1 = clip->y1;
>>> +		dst->y1 += diff;
>>>   	}
>>>   	diff = dst->x2 - clip->x2;
>>>   	if (diff > 0) {
>>>   		u32 new_src_w = clip_scaled(drm_rect_width(src),
>>> -					    drm_rect_width(dst), diff);
>>> +					    drm_rect_width(dst), &diff);
>>>   
>>>   		src->x2 = src->x1 + new_src_w;
>>> -		dst->x2 = clip->x2;
>>> +		dst->x2 -= diff;
>>>   	}
>>>   	diff = dst->y2 - clip->y2;
>>>   	if (diff > 0) {
>>>   		u32 new_src_h = clip_scaled(drm_rect_height(src),
>>> -					    drm_rect_height(dst), diff);
>>> +					    drm_rect_height(dst), &diff);
>>>   
>>>   		src->y2 = src->y1 + new_src_h;
>>> -		dst->y2 = clip->y2;
>>> +		dst->y2 -= diff;
>>>   	}
>>>   
>>>   	return drm_rect_visible(dst);
>>> -- 
>>> 2.23.0
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx