[Intel-gfx] [PATCH 2/3] drm/rect: Handle rounding errors in drm_rect_clip_scaled

[Daniel Vetter]

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 at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch