[PATCH 1/2] drm/rect: Keep the scaled clip bounded

[PATCH 1/2] drm/rect: Keep the scaled clip bounded

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Limit the scaled clip to only clip at most dst_w/h pixels.
This avoids the problem with clip_scaled() not being able
to return negative values. Since new_src_w/h is now properly
bounded we can remove the clamp()s.

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>
---
 drivers/gpu/drm/drm_rect.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
index b8363aaa9032..7762b6e9278d 100644
--- a/drivers/gpu/drm/drm_rect.c
+++ b/drivers/gpu/drm/drm_rect.c
@@ -54,7 +54,12 @@ EXPORT_SYMBOL(drm_rect_intersect);
 
 static u32 clip_scaled(u32 src, u32 dst, u32 clip)
 {
-	u64 tmp = mul_u32_u32(src, dst - clip);
+	u64 tmp;
+
+	/* Only clip what we have. Keeps the result bounded as well. */
+	clip = min(clip, dst);
+
+	tmp = mul_u32_u32(src, dst - clip);
 
 	/*
 	 * Round toward 1.0 when clipping so that we don't accidentally
@@ -89,7 +94,7 @@ bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst,
 		u32 new_src_w = clip_scaled(drm_rect_width(src),
 					    drm_rect_width(dst), diff);
 
-		src->x1 = clamp_t(int64_t, src->x2 - new_src_w, INT_MIN, INT_MAX);
+		src->x1 = src->x2 - new_src_w;
 		dst->x1 = clip->x1;
 	}
 	diff = clip->y1 - dst->y1;
@@ -97,7 +102,7 @@ bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst,
 		u32 new_src_h = clip_scaled(drm_rect_height(src),
 					    drm_rect_height(dst), diff);
 
-		src->y1 = clamp_t(int64_t, src->y2 - new_src_h, INT_MIN, INT_MAX);
+		src->y1 = src->y2 - new_src_h;
 		dst->y1 = clip->y1;
 	}
 	diff = dst->x2 - clip->x2;
@@ -105,7 +110,7 @@ bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst,
 		u32 new_src_w = clip_scaled(drm_rect_width(src),
 					    drm_rect_width(dst), diff);
 
-		src->x2 = clamp_t(int64_t, src->x1 + new_src_w, INT_MIN, INT_MAX);
+		src->x2 = src->x1 + new_src_w;
 		dst->x2 = clip->x2;
 	}
 	diff = dst->y2 - clip->y2;
@@ -113,7 +118,7 @@ bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst,
 		u32 new_src_h = clip_scaled(drm_rect_height(src),
 					    drm_rect_height(dst), diff);
 
-		src->y2 = clamp_t(int64_t, src->y1 + new_src_h, INT_MIN, INT_MAX);
+		src->y2 = src->y1 + new_src_h;
 		dst->y2 = clip->y2;
 	}
 
-- 
2.23.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/2] drm/rect: Keep the scaled clip bounded

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Limit the scaled clip to only clip at most dst_w/h pixels.
This avoids the problem with clip_scaled() not being able
to return negative values. Since new_src_w/h is now properly
bounded we can remove the clamp()s.

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>
---
 drivers/gpu/drm/drm_rect.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
index b8363aaa9032..7762b6e9278d 100644
--- a/drivers/gpu/drm/drm_rect.c
+++ b/drivers/gpu/drm/drm_rect.c
@@ -54,7 +54,12 @@ EXPORT_SYMBOL(drm_rect_intersect);
 
 static u32 clip_scaled(u32 src, u32 dst, u32 clip)
 {
-	u64 tmp = mul_u32_u32(src, dst - clip);
+	u64 tmp;
+
+	/* Only clip what we have. Keeps the result bounded as well. */
+	clip = min(clip, dst);
+
+	tmp = mul_u32_u32(src, dst - clip);
 
 	/*
 	 * Round toward 1.0 when clipping so that we don't accidentally
@@ -89,7 +94,7 @@ bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst,
 		u32 new_src_w = clip_scaled(drm_rect_width(src),
 					    drm_rect_width(dst), diff);
 
-		src->x1 = clamp_t(int64_t, src->x2 - new_src_w, INT_MIN, INT_MAX);
+		src->x1 = src->x2 - new_src_w;
 		dst->x1 = clip->x1;
 	}
 	diff = clip->y1 - dst->y1;
@@ -97,7 +102,7 @@ bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst,
 		u32 new_src_h = clip_scaled(drm_rect_height(src),
 					    drm_rect_height(dst), diff);
 
-		src->y1 = clamp_t(int64_t, src->y2 - new_src_h, INT_MIN, INT_MAX);
+		src->y1 = src->y2 - new_src_h;
 		dst->y1 = clip->y1;
 	}
 	diff = dst->x2 - clip->x2;
@@ -105,7 +110,7 @@ bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst,
 		u32 new_src_w = clip_scaled(drm_rect_width(src),
 					    drm_rect_width(dst), diff);
 
-		src->x2 = clamp_t(int64_t, src->x1 + new_src_w, INT_MIN, INT_MAX);
+		src->x2 = src->x1 + new_src_w;
 		dst->x2 = clip->x2;
 	}
 	diff = dst->y2 - clip->y2;
@@ -113,7 +118,7 @@ bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst,
 		u32 new_src_h = clip_scaled(drm_rect_height(src),
 					    drm_rect_height(dst), diff);
 
-		src->y2 = clamp_t(int64_t, src->y1 + new_src_h, INT_MIN, INT_MAX);
+		src->y2 = src->y1 + new_src_h;
 		dst->y2 = clip->y2;
 	}
 
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjala]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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?
-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

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

[Daniel Vetter]

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?
-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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

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.

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

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Benjamin GAIGNARD]

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

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

[Benjamin GAIGNARD]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

On Wed, Nov 20, 2019 at 07:11:38PM +0200, 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 ;)

I *think* the only problem is that the clip can result in a visible
source rectangle when this happens. The dst rectangle will still
be correctly invisible so hopefully not a big deal. But I guess we
might as well fix it, and I can do a selftest which makes sure
both src and dst come out invisible.

> 
> 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.

Definitely real. I'll fix it and toss in a selftest.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Wed, Nov 20, 2019 at 07:11:38PM +0200, 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 ;)

I *think* the only problem is that the clip can result in a visible
source rectangle when this happens. The dst rectangle will still
be correctly invisible so hopefully not a big deal. But I guess we
might as well fix it, and I can do a selftest which makes sure
both src and dst come out invisible.

> 
> 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.

Definitely real. I'll fix it and toss in a selftest.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel