[2/5] drm/rect: Handle rounding errors in drm_rect_clip_scaled, v3.
diff mbox

Message ID 20180503112217.37292-3-maarten.lankhorst@linux.intel.com
State New
Headers show

Commit Message

Maarten Lankhorst May 3, 2018, 11:22 a.m. UTC
Instead of relying on a scale which may increase rounding errors,
clip src by doing: src * (dst - clip) / dst and rounding the result
away from 1, so the new coordinates get closer to 1. We won't need
to fix up with a magic macro afterwards, because our scaling factor
will never go to the other side of 1.

Changes since v1:
- Adjust dst immediately, else drm_rect_width/height on dst gives bogus
  results.
Change since v2:
- Get rid of macros and use 64-bits math.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c |  2 +-
 drivers/gpu/drm/drm_rect.c          | 45 ++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_sprite.c |  2 +-
 include/drm/drm_rect.h              |  3 +-
 4 files changed, 35 insertions(+), 17 deletions(-)

Comments

Ville Syrjala May 3, 2018, 1:29 p.m. UTC | #1
On Thu, May 03, 2018 at 01:22:14PM +0200, Maarten Lankhorst wrote:
> Instead of relying on a scale which may increase rounding errors,
> clip src by doing: src * (dst - clip) / dst and rounding the result
> away from 1, so the new coordinates get closer to 1. We won't need
> to fix up with a magic macro afterwards, because our scaling factor
> will never go to the other side of 1.
> 
> Changes since v1:
> - Adjust dst immediately, else drm_rect_width/height on dst gives bogus
>   results.
> Change since v2:
> - Get rid of macros and use 64-bits math.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c |  2 +-
>  drivers/gpu/drm/drm_rect.c          | 45 ++++++++++++++++++++---------
>  drivers/gpu/drm/i915/intel_sprite.c |  2 +-
>  include/drm/drm_rect.h              |  3 +-
>  4 files changed, 35 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 9cb2209f6fc8..130da5195f3b 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -766,7 +766,7 @@ int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
>  	if (crtc_state->enable)
>  		drm_mode_get_hv_timing(&crtc_state->mode, &clip.x2, &clip.y2);
>  
> -	plane_state->visible = drm_rect_clip_scaled(src, dst, &clip, hscale, vscale);
> +	plane_state->visible = drm_rect_clip_scaled(src, dst, &clip);
>  
>  	drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, rotation);
>  
> diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
> index 4735526297aa..f477063f18ea 100644
> --- a/drivers/gpu/drm/drm_rect.c
> +++ b/drivers/gpu/drm/drm_rect.c
> @@ -50,13 +50,21 @@ 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)
> +{
> +	u64 newsrc = mul_u32_u32(src, dst - clip);

'newsrc' feels slightly misleading. It would be a decent name for
the final return value of the function, but for this intermediate
value 'tmp' or something equally non specific might be better.

> +
> +	if (src < (dst << 16))
> +		return DIV_ROUND_UP_ULL(newsrc, dst);
> +	else
> +		return DIV_ROUND_DOWN_ULL(newsrc, dst);

I think we could use a comment here to explain the choice of
rounding direction. Eg.

/*
 * Round toward 1.0 when clipping so that we don't accidentally
 * change upscaling to downscaling or vice versa.
 */
?

> +}
> +
>  /**
>   * drm_rect_clip_scaled - perform a scaled clip operation
>   * @src: source window rectangle
>   * @dst: destination window rectangle
>   * @clip: clip rectangle
> - * @hscale: horizontal scaling factor
> - * @vscale: vertical scaling factor
>   *
>   * Clip rectangle @dst by rectangle @clip. Clip rectangle @src by the
>   * same amounts multiplied by @hscale and @vscale.
> @@ -66,33 +74,44 @@ EXPORT_SYMBOL(drm_rect_intersect);
>   * %false otherwise
>   */
>  bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst,
> -			  const struct drm_rect *clip,
> -			  int hscale, int vscale)
> +			  const struct drm_rect *clip)
>  {
>  	int diff;
>  
>  	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);
> +		u32 new_src_w = clip_scaled(drm_rect_width(src),
> +					    drm_rect_width(dst), diff);

Could have precomputed the src/dst width/height upfront maybe.

Oh, I guess you can't since your clip_scaled() uses the dst width/height
for more than the scaling factor. If it just computed diff*src/dst
like the original code does then precomputing would work just fine.

Your way feels a bit more complicated to me, but looks like it should
work.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +
> +		src->x1 = clamp_t(int64_t, src->x2 - new_src_w, INT_MIN, INT_MAX);
> +		dst->x1 = clip->x1;
>  	}
>  	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);
> +		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);
> +		dst->y1 = clip->y1;
>  	}
>  	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);
> +		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);
> +		dst->x2 = clip->x2;
>  	}
>  	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);
> +		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);
> +		dst->y2 = clip->y2;
>  	}
>  
> -	return drm_rect_intersect(dst, clip);
> +	return drm_rect_visible(dst);
>  }
>  EXPORT_SYMBOL(drm_rect_clip_scaled);
>  
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index aa1dfaa692b9..44d7aca222b0 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1008,7 +1008,7 @@ intel_check_sprite_plane(struct intel_plane *plane,
>  		drm_mode_get_hv_timing(&crtc_state->base.mode,
>  				       &clip.x2, &clip.y2);
>  
> -	state->base.visible = drm_rect_clip_scaled(src, dst, &clip, hscale, vscale);
> +	state->base.visible = drm_rect_clip_scaled(src, dst, &clip);
>  
>  	crtc_x = dst->x1;
>  	crtc_y = dst->y1;
> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
> index 44bc122b9ee0..6c54544a4be7 100644
> --- a/include/drm/drm_rect.h
> +++ b/include/drm/drm_rect.h
> @@ -175,8 +175,7 @@ static inline bool drm_rect_equals(const struct drm_rect *r1,
>  
>  bool drm_rect_intersect(struct drm_rect *r, const struct drm_rect *clip);
>  bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst,
> -			  const struct drm_rect *clip,
> -			  int hscale, int vscale);
> +			  const struct drm_rect *clip);
>  int drm_rect_calc_hscale(const struct drm_rect *src,
>  			 const struct drm_rect *dst,
>  			 int min_hscale, int max_hscale);
> -- 
> 2.17.0

Patch
diff mbox

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 9cb2209f6fc8..130da5195f3b 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -766,7 +766,7 @@  int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
 	if (crtc_state->enable)
 		drm_mode_get_hv_timing(&crtc_state->mode, &clip.x2, &clip.y2);
 
-	plane_state->visible = drm_rect_clip_scaled(src, dst, &clip, hscale, vscale);
+	plane_state->visible = drm_rect_clip_scaled(src, dst, &clip);
 
 	drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, rotation);
 
diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
index 4735526297aa..f477063f18ea 100644
--- a/drivers/gpu/drm/drm_rect.c
+++ b/drivers/gpu/drm/drm_rect.c
@@ -50,13 +50,21 @@  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)
+{
+	u64 newsrc = mul_u32_u32(src, dst - clip);
+
+	if (src < (dst << 16))
+		return DIV_ROUND_UP_ULL(newsrc, dst);
+	else
+		return DIV_ROUND_DOWN_ULL(newsrc, dst);
+}
+
 /**
  * drm_rect_clip_scaled - perform a scaled clip operation
  * @src: source window rectangle
  * @dst: destination window rectangle
  * @clip: clip rectangle
- * @hscale: horizontal scaling factor
- * @vscale: vertical scaling factor
  *
  * Clip rectangle @dst by rectangle @clip. Clip rectangle @src by the
  * same amounts multiplied by @hscale and @vscale.
@@ -66,33 +74,44 @@  EXPORT_SYMBOL(drm_rect_intersect);
  * %false otherwise
  */
 bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst,
-			  const struct drm_rect *clip,
-			  int hscale, int vscale)
+			  const struct drm_rect *clip)
 {
 	int diff;
 
 	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);
+		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);
+		dst->x1 = clip->x1;
 	}
 	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);
+		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);
+		dst->y1 = clip->y1;
 	}
 	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);
+		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);
+		dst->x2 = clip->x2;
 	}
 	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);
+		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);
+		dst->y2 = clip->y2;
 	}
 
-	return drm_rect_intersect(dst, clip);
+	return drm_rect_visible(dst);
 }
 EXPORT_SYMBOL(drm_rect_clip_scaled);
 
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index aa1dfaa692b9..44d7aca222b0 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1008,7 +1008,7 @@  intel_check_sprite_plane(struct intel_plane *plane,
 		drm_mode_get_hv_timing(&crtc_state->base.mode,
 				       &clip.x2, &clip.y2);
 
-	state->base.visible = drm_rect_clip_scaled(src, dst, &clip, hscale, vscale);
+	state->base.visible = drm_rect_clip_scaled(src, dst, &clip);
 
 	crtc_x = dst->x1;
 	crtc_y = dst->y1;
diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
index 44bc122b9ee0..6c54544a4be7 100644
--- a/include/drm/drm_rect.h
+++ b/include/drm/drm_rect.h
@@ -175,8 +175,7 @@  static inline bool drm_rect_equals(const struct drm_rect *r1,
 
 bool drm_rect_intersect(struct drm_rect *r, const struct drm_rect *clip);
 bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst,
-			  const struct drm_rect *clip,
-			  int hscale, int vscale);
+			  const struct drm_rect *clip);
 int drm_rect_calc_hscale(const struct drm_rect *src,
 			 const struct drm_rect *dst,
 			 int min_hscale, int max_hscale);