[2/3] drm/rect: Handle rounding errors in drm_rect_clip_scaled
diff mbox

Message ID 20180426082821.11129-2-maarten.lankhorst@linux.intel.com
State New
Headers show

Commit Message

Maarten Lankhorst April 26, 2018, 8:28 a.m. UTC
No matter how you perform the clip adjustments, a small
error may push the scaling factor to the other side of
0x10000. Solve this with a macro that will fixup the
scale to 0x10000 if we accidentally wrap to the other side.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_rect.c | 65 +++++++++++++++++++++++++++-----------
 1 file changed, 47 insertions(+), 18 deletions(-)

Comments

Daniel Vetter April 26, 2018, 1:32 p.m. UTC | #1
On Thu, Apr 26, 2018 at 10:28:20AM +0200, Maarten Lankhorst wrote:
> No matter how you perform the clip adjustments, a small
> error may push the scaling factor to the other side of
> 0x10000. Solve this with a macro that will fixup the
> scale to 0x10000 if we accidentally wrap to the other side.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

I think this and the previous patch are perfect candidates for in-kernel
selftests. Can I volunteer you to get those started for the kms side? We
already have a drm_mm selftest, but I think splitting things a bit might
be useful.

Or we rename that one and just stuff all the kms tests in there, dunno.
-Daniel

> ---
>  drivers/gpu/drm/drm_rect.c | 65 +++++++++++++++++++++++++++-----------
>  1 file changed, 47 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
> index b179c7c73cc5..71b6b7f5d58f 100644
> --- a/drivers/gpu/drm/drm_rect.c
> +++ b/drivers/gpu/drm/drm_rect.c
> @@ -50,6 +50,24 @@ bool drm_rect_intersect(struct drm_rect *r1, const struct drm_rect *r2)
>  }
>  EXPORT_SYMBOL(drm_rect_intersect);
>  
> +static int drm_calc_scale(int src, int dst)
> +{
> +	int scale = 0;
> +
> +	if (WARN_ON(src < 0 || dst < 0))
> +		return -EINVAL;
> +
> +	if (dst == 0)
> +		return 0;
> +
> +	if (src > (dst << 16))
> +		return DIV_ROUND_UP(src, dst);
> +	else
> +		scale = src / dst;
> +
> +	return scale;
> +}
> +
>  /**
>   * drm_rect_clip_scaled - perform a scaled clip operation
>   * @src: source window rectangle
> @@ -71,49 +89,60 @@ bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst,
>  {
>  	int diff;
>  
> +	/*
> +	 * When scale is near 0x10000 rounding errors may cause the scaling
> +	 * factor to the other side. Some hardware may support
> +	 * upsampling, but not downsampling, and that would break when
> +	 * rounding.
> +	 */
> +#define FIXUP(oldscale, fn, m, second) do { \
> +	if (oldscale != 1 << 16) { \
> +		int newscale = drm_calc_scale(fn(src), fn(dst)); \
> + \
> +		if (newscale < 0) \
> +			return false; \
> + \
> +		if ((oldscale < 0x10000) != (newscale < 0x10000)) { \
> +			if (!second) \
> +				src->m##1 = src->m##2 - ((fn(dst) - diff) << 16); \
> +			else \
> +				src->m##2 = src->m##1 + ((fn(dst) - diff) << 16); \
> +		} \
> +	} \
> + } while (0)
> +
>  	diff = clip->x1 - dst->x1;
>  	if (diff > 0) {
>  		int64_t tmp = src->x1 + (int64_t) diff * hscale;
>  		src->x1 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX);
> +		FIXUP(hscale, drm_rect_width, x, 0);
>  	}
> +
>  	diff = clip->y1 - dst->y1;
>  	if (diff > 0) {
>  		int64_t tmp = src->y1 + (int64_t) diff * vscale;
>  		src->y1 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX);
> +		FIXUP(vscale, drm_rect_height, y, 0);
>  	}
> +
>  	diff = dst->x2 - clip->x2;
>  	if (diff > 0) {
>  		int64_t tmp = src->x2 - (int64_t) diff * hscale;
>  		src->x2 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX);
> +		FIXUP(hscale, drm_rect_width, x, 1);
>  	}
>  	diff = dst->y2 - clip->y2;
>  	if (diff > 0) {
>  		int64_t tmp = src->y2 - (int64_t) diff * vscale;
>  		src->y2 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX);
> +		FIXUP(vscale, drm_rect_height, y, 1);
>  	}
> +#undef FIXUP
>  
>  	return drm_rect_intersect(dst, clip);
>  }
>  EXPORT_SYMBOL(drm_rect_clip_scaled);
>  
> -static int drm_calc_scale(int src, int dst)
> -{
> -	int scale = 0;
> -
> -	if (WARN_ON(src < 0 || dst < 0))
> -		return -EINVAL;
> -
> -	if (dst == 0)
> -		return 0;
> -
> -	if (src > (dst << 16))
> -		return DIV_ROUND_UP(src, dst);
> -	else
> -		scale = src / dst;
> -
> -	return scale;
> -}
> -
>  /**
>   * drm_rect_calc_hscale - calculate the horizontal scaling factor
>   * @src: source window rectangle
> -- 
> 2.17.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä April 26, 2018, 2:01 p.m. UTC | #2
On Thu, Apr 26, 2018 at 10:28:20AM +0200, Maarten Lankhorst wrote:
> No matter how you perform the clip adjustments, a small
> error may push the scaling factor to the other side of
> 0x10000. Solve this with a macro that will fixup the
> scale to 0x10000 if we accidentally wrap to the other side.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_rect.c | 65 +++++++++++++++++++++++++++-----------
>  1 file changed, 47 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
> index b179c7c73cc5..71b6b7f5d58f 100644
> --- a/drivers/gpu/drm/drm_rect.c
> +++ b/drivers/gpu/drm/drm_rect.c
> @@ -50,6 +50,24 @@ bool drm_rect_intersect(struct drm_rect *r1, const struct drm_rect *r2)
>  }
>  EXPORT_SYMBOL(drm_rect_intersect);
>  
> +static int drm_calc_scale(int src, int dst)
> +{
> +	int scale = 0;
> +
> +	if (WARN_ON(src < 0 || dst < 0))
> +		return -EINVAL;
> +
> +	if (dst == 0)
> +		return 0;
> +
> +	if (src > (dst << 16))
> +		return DIV_ROUND_UP(src, dst);
> +	else
> +		scale = src / dst;
> +
> +	return scale;
> +}
> +
>  /**
>   * drm_rect_clip_scaled - perform a scaled clip operation
>   * @src: source window rectangle
> @@ -71,49 +89,60 @@ bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst,
>  {
>  	int diff;
>  
> +	/*
> +	 * When scale is near 0x10000 rounding errors may cause the scaling
> +	 * factor to the other side. Some hardware may support
> +	 * upsampling, but not downsampling, and that would break when
> +	 * rounding.
> +	 */
> +#define FIXUP(oldscale, fn, m, second) do { \
> +	if (oldscale != 1 << 16) { \
> +		int newscale = drm_calc_scale(fn(src), fn(dst)); \
> + \
> +		if (newscale < 0) \
> +			return false; \
> + \
> +		if ((oldscale < 0x10000) != (newscale < 0x10000)) { \
> +			if (!second) \
> +				src->m##1 = src->m##2 - ((fn(dst) - diff) << 16); \
> +			else \
> +				src->m##2 = src->m##1 + ((fn(dst) - diff) << 16); \

This is starting to get very messy. Also are we sure the x2/y2 can't
go past the initial value here?

It's starting to feel like we should do the clip with the rounding the
orher way, which should guarantee we never flip between up and down
scaling by accident. And then just do a post-clip check for final scale
factor with the pessimistic rounding direction to make sure we stay
within the limits.

> +		} \
> +	} \
> + } while (0)
> +
>  	diff = clip->x1 - dst->x1;
>  	if (diff > 0) {
>  		int64_t tmp = src->x1 + (int64_t) diff * hscale;
>  		src->x1 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX);
> +		FIXUP(hscale, drm_rect_width, x, 0);
>  	}
> +
>  	diff = clip->y1 - dst->y1;
>  	if (diff > 0) {
>  		int64_t tmp = src->y1 + (int64_t) diff * vscale;
>  		src->y1 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX);
> +		FIXUP(vscale, drm_rect_height, y, 0);
>  	}
> +
>  	diff = dst->x2 - clip->x2;
>  	if (diff > 0) {
>  		int64_t tmp = src->x2 - (int64_t) diff * hscale;
>  		src->x2 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX);
> +		FIXUP(hscale, drm_rect_width, x, 1);
>  	}
>  	diff = dst->y2 - clip->y2;
>  	if (diff > 0) {
>  		int64_t tmp = src->y2 - (int64_t) diff * vscale;
>  		src->y2 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX);
> +		FIXUP(vscale, drm_rect_height, y, 1);
>  	}
> +#undef FIXUP
>  
>  	return drm_rect_intersect(dst, clip);
>  }
>  EXPORT_SYMBOL(drm_rect_clip_scaled);
>  
> -static int drm_calc_scale(int src, int dst)
> -{
> -	int scale = 0;
> -
> -	if (WARN_ON(src < 0 || dst < 0))
> -		return -EINVAL;
> -
> -	if (dst == 0)
> -		return 0;
> -
> -	if (src > (dst << 16))
> -		return DIV_ROUND_UP(src, dst);
> -	else
> -		scale = src / dst;
> -
> -	return scale;
> -}
> -
>  /**
>   * drm_rect_calc_hscale - calculate the horizontal scaling factor
>   * @src: source window rectangle
> -- 
> 2.17.0

Patch
diff mbox

diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
index b179c7c73cc5..71b6b7f5d58f 100644
--- a/drivers/gpu/drm/drm_rect.c
+++ b/drivers/gpu/drm/drm_rect.c
@@ -50,6 +50,24 @@  bool drm_rect_intersect(struct drm_rect *r1, const struct drm_rect *r2)
 }
 EXPORT_SYMBOL(drm_rect_intersect);
 
+static int drm_calc_scale(int src, int dst)
+{
+	int scale = 0;
+
+	if (WARN_ON(src < 0 || dst < 0))
+		return -EINVAL;
+
+	if (dst == 0)
+		return 0;
+
+	if (src > (dst << 16))
+		return DIV_ROUND_UP(src, dst);
+	else
+		scale = src / dst;
+
+	return scale;
+}
+
 /**
  * drm_rect_clip_scaled - perform a scaled clip operation
  * @src: source window rectangle
@@ -71,49 +89,60 @@  bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst,
 {
 	int diff;
 
+	/*
+	 * When scale is near 0x10000 rounding errors may cause the scaling
+	 * factor to the other side. Some hardware may support
+	 * upsampling, but not downsampling, and that would break when
+	 * rounding.
+	 */
+#define FIXUP(oldscale, fn, m, second) do { \
+	if (oldscale != 1 << 16) { \
+		int newscale = drm_calc_scale(fn(src), fn(dst)); \
+ \
+		if (newscale < 0) \
+			return false; \
+ \
+		if ((oldscale < 0x10000) != (newscale < 0x10000)) { \
+			if (!second) \
+				src->m##1 = src->m##2 - ((fn(dst) - diff) << 16); \
+			else \
+				src->m##2 = src->m##1 + ((fn(dst) - diff) << 16); \
+		} \
+	} \
+ } while (0)
+
 	diff = clip->x1 - dst->x1;
 	if (diff > 0) {
 		int64_t tmp = src->x1 + (int64_t) diff * hscale;
 		src->x1 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX);
+		FIXUP(hscale, drm_rect_width, x, 0);
 	}
+
 	diff = clip->y1 - dst->y1;
 	if (diff > 0) {
 		int64_t tmp = src->y1 + (int64_t) diff * vscale;
 		src->y1 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX);
+		FIXUP(vscale, drm_rect_height, y, 0);
 	}
+
 	diff = dst->x2 - clip->x2;
 	if (diff > 0) {
 		int64_t tmp = src->x2 - (int64_t) diff * hscale;
 		src->x2 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX);
+		FIXUP(hscale, drm_rect_width, x, 1);
 	}
 	diff = dst->y2 - clip->y2;
 	if (diff > 0) {
 		int64_t tmp = src->y2 - (int64_t) diff * vscale;
 		src->y2 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX);
+		FIXUP(vscale, drm_rect_height, y, 1);
 	}
+#undef FIXUP
 
 	return drm_rect_intersect(dst, clip);
 }
 EXPORT_SYMBOL(drm_rect_clip_scaled);
 
-static int drm_calc_scale(int src, int dst)
-{
-	int scale = 0;
-
-	if (WARN_ON(src < 0 || dst < 0))
-		return -EINVAL;
-
-	if (dst == 0)
-		return 0;
-
-	if (src > (dst << 16))
-		return DIV_ROUND_UP(src, dst);
-	else
-		scale = src / dst;
-
-	return scale;
-}
-
 /**
  * drm_rect_calc_hscale - calculate the horizontal scaling factor
  * @src: source window rectangle