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

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel