[PATCH 1/3] drm/rect: Round above 1 << 16 upwards to correct scale calculation functions.

[PATCH 1/3] drm/rect: Round above 1 << 16 upwards to correct scale calculation functions.

[Maarten Lankhorst]

When calculating limits we want to be as pessimistic as possible,
so we have to explicitly say whether we want to round up or down
to accurately calculate whether we are below min_scale or above
max_scale.

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

diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
index 9817c1445ba9..b179c7c73cc5 100644
--- a/drivers/gpu/drm/drm_rect.c
+++ b/drivers/gpu/drm/drm_rect.c
@@ -106,7 +106,10 @@ static int drm_calc_scale(int src, int dst)
 	if (dst == 0)
 		return 0;
 
-	scale = src / dst;
+	if (src > (dst << 16))
+		return DIV_ROUND_UP(src, dst);
+	else
+		scale = src / dst;
 
 	return scale;
 }
@@ -121,6 +124,9 @@ static int drm_calc_scale(int src, int dst)
  * Calculate the horizontal scaling factor as
  * (@src width) / (@dst width).
  *
+ * If the scale is below 1 << 16, round down, if above up. This will
+ * calculate the scale with the most pessimistic limit calculation.
+ *
  * RETURNS:
  * The horizontal scaling factor, or errno of out of limits.
  */
@@ -152,6 +158,9 @@ EXPORT_SYMBOL(drm_rect_calc_hscale);
  * Calculate the vertical scaling factor as
  * (@src height) / (@dst height).
  *
+ * If the scale is below 1 << 16, round down, if above up. This will
+ * calculate the scale with the most pessimistic limit calculation.
+ *
  * RETURNS:
  * The vertical scaling factor, or errno of out of limits.
  */
@@ -189,6 +198,9 @@ EXPORT_SYMBOL(drm_rect_calc_vscale);
  * If the calculated scaling factor is above @max_vscale,
  * decrease the height of rectangle @src to compensate.
  *
+ * If the scale is below 1 << 16, round down, if above up. This will
+ * calculate the scale with the most pessimistic limit calculation.
+ *
  * RETURNS:
  * The horizontal scaling factor.
  */
@@ -239,6 +251,9 @@ EXPORT_SYMBOL(drm_rect_calc_hscale_relaxed);
  * If the calculated scaling factor is above @max_vscale,
  * decrease the height of rectangle @src to compensate.
  *
+ * If the scale is below 1 << 16, round down, if above up. This will
+ * calculate the scale with the most pessimistic limit calculation.
+ *
  * RETURNS:
  * The vertical scaling factor.
  */
-- 
2.17.0

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

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

[Maarten Lankhorst]

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); \
+		} \
+	} \
+ } 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

[PATCH 3/3] drm/i915: Do not adjust scale when out of bounds, v2.

[Maarten Lankhorst]

With the previous patch drm_atomic_helper_check_plane_state correctly
calculates clipping and the xf86-video-intel ddx is fixed to fall back
to GPU correctly when SetPlane fails, we can remove the hack where
we try to pan/zoom when out of min/max scaling range. This was already
poor behavior where the screen didn't show what was requested, and now
instead we reject it outright. This simplifies check_sprite_plane a lot.

Changes since v1:
- Set crtc_h to the height correctly.
- Reject < 3x3 rectangles instead of making them invisible for <gen9.
  For gen9+ skl_update_scaler_plane will reject them.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 144 +++++++---------------------
 1 file changed, 35 insertions(+), 109 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index aa1dfaa692b9..970015dcc6f1 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -936,22 +936,12 @@ intel_check_sprite_plane(struct intel_plane *plane,
 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
 	struct drm_framebuffer *fb = state->base.fb;
-	int crtc_x, crtc_y;
-	unsigned int crtc_w, crtc_h;
-	uint32_t src_x, src_y, src_w, src_h;
-	struct drm_rect *src = &state->base.src;
-	struct drm_rect *dst = &state->base.dst;
-	struct drm_rect clip = {};
 	int max_stride = INTEL_GEN(dev_priv) >= 9 ? 32768 : 16384;
-	int hscale, vscale;
 	int max_scale, min_scale;
 	bool can_scale;
 	int ret;
 	uint32_t pixel_format = 0;
 
-	*src = drm_plane_state_src(&state->base);
-	*dst = drm_plane_state_dest(&state->base);
-
 	if (!fb) {
 		state->base.visible = false;
 		return 0;
@@ -990,64 +980,19 @@ intel_check_sprite_plane(struct intel_plane *plane,
 		min_scale = plane->can_scale ? 1 : (1 << 16);
 	}
 
-	/*
-	 * FIXME the following code does a bunch of fuzzy adjustments to the
-	 * coordinates and sizes. We probably need some way to decide whether
-	 * more strict checking should be done instead.
-	 */
-	drm_rect_rotate(src, fb->width << 16, fb->height << 16,
-			state->base.rotation);
-
-	hscale = drm_rect_calc_hscale_relaxed(src, dst, min_scale, max_scale);
-	BUG_ON(hscale < 0);
-
-	vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale, max_scale);
-	BUG_ON(vscale < 0);
-
-	if (crtc_state->base.enable)
-		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);
-
-	crtc_x = dst->x1;
-	crtc_y = dst->y1;
-	crtc_w = drm_rect_width(dst);
-	crtc_h = drm_rect_height(dst);
+	ret = drm_atomic_helper_check_plane_state(&state->base,
+						  &crtc_state->base,
+						  min_scale, max_scale,
+						  true, true);
+	if (ret)
+		return ret;
 
 	if (state->base.visible) {
-		/* check again in case clipping clamped the results */
-		hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
-		if (hscale < 0) {
-			DRM_DEBUG_KMS("Horizontal scaling factor out of limits\n");
-			drm_rect_debug_print("src: ", src, true);
-			drm_rect_debug_print("dst: ", dst, false);
-
-			return hscale;
-		}
-
-		vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
-		if (vscale < 0) {
-			DRM_DEBUG_KMS("Vertical scaling factor out of limits\n");
-			drm_rect_debug_print("src: ", src, true);
-			drm_rect_debug_print("dst: ", dst, false);
-
-			return vscale;
-		}
-
-		/* Make the source viewport size an exact multiple of the scaling factors. */
-		drm_rect_adjust_size(src,
-				     drm_rect_width(dst) * hscale - drm_rect_width(src),
-				     drm_rect_height(dst) * vscale - drm_rect_height(src));
-
-		drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16,
-				    state->base.rotation);
-
-		/* sanity check to make sure the src viewport wasn't enlarged */
-		WARN_ON(src->x1 < (int) state->base.src_x ||
-			src->y1 < (int) state->base.src_y ||
-			src->x2 > (int) state->base.src_x + state->base.src_w ||
-			src->y2 > (int) state->base.src_y + state->base.src_h);
+		struct drm_rect *src = &state->base.src;
+		struct drm_rect *dst = &state->base.dst;
+		unsigned int crtc_w = drm_rect_width(dst);
+		unsigned int crtc_h = drm_rect_height(dst);
+		uint32_t src_x, src_y, src_w, src_h;
 
 		/*
 		 * Hardware doesn't handle subpixel coordinates.
@@ -1060,58 +1005,39 @@ intel_check_sprite_plane(struct intel_plane *plane,
 		src_y = src->y1 >> 16;
 		src_h = drm_rect_height(src) >> 16;
 
-		if (intel_format_is_yuv(fb->format->format)) {
-			src_x &= ~1;
-			src_w &= ~1;
-
-			/*
-			 * Must keep src and dst the
-			 * same if we can't scale.
-			 */
-			if (!can_scale)
-				crtc_w &= ~1;
+		src->x1 = src_x << 16;
+		src->x2 = (src_x + src_w) << 16;
+		src->y1 = src_y << 16;
+		src->y2 = (src_y + src_h) << 16;
 
-			if (crtc_w == 0)
-				state->base.visible = false;
+		if (intel_format_is_yuv(fb->format->format) &&
+		    (src_x % 2 || src_w % 2)) {
+			DRM_DEBUG_KMS("src x/w (%u, %u) must be a multiple of 2 for YUV planes\n",
+				      src_x, src_w);
+			return -EINVAL;
 		}
-	}
 
-	/* Check size restrictions when scaling */
-	if (state->base.visible && (src_w != crtc_w || src_h != crtc_h)) {
-		unsigned int width_bytes;
-		int cpp = fb->format->cpp[0];
+		/* Check size restrictions when scaling */
+		if (src_w != crtc_w || src_h != crtc_h) {
+			unsigned int width_bytes;
+			int cpp = fb->format->cpp[0];
 
-		WARN_ON(!can_scale);
+			WARN_ON(!can_scale);
 
-		/* FIXME interlacing min height is 6 */
+			width_bytes = ((src_x * cpp) & 63) + src_w * cpp;
 
-		if (crtc_w < 3 || crtc_h < 3)
-			state->base.visible = false;
-
-		if (src_w < 3 || src_h < 3)
-			state->base.visible = false;
-
-		width_bytes = ((src_x * cpp) & 63) + src_w * cpp;
-
-		if (INTEL_GEN(dev_priv) < 9 && (src_w > 2048 || src_h > 2048 ||
-		    width_bytes > 4096 || fb->pitches[0] > 4096)) {
-			DRM_DEBUG_KMS("Source dimensions exceed hardware limits\n");
-			return -EINVAL;
+			/* FIXME interlacing min height is 6 */
+			if (INTEL_GEN(dev_priv) < 9 && (
+			     src_w < 3 || src_h < 3 ||
+			     src_w > 2048 || src_h > 2048 ||
+			     crtc_w < 3 || crtc_h < 3 ||
+			     width_bytes > 4096 || fb->pitches[0] > 4096)) {
+				DRM_DEBUG_KMS("Source dimensions exceed hardware limits\n");
+				return -EINVAL;
+			}
 		}
 	}
 
-	if (state->base.visible) {
-		src->x1 = src_x << 16;
-		src->x2 = (src_x + src_w) << 16;
-		src->y1 = src_y << 16;
-		src->y2 = (src_y + src_h) << 16;
-	}
-
-	dst->x1 = crtc_x;
-	dst->x2 = crtc_x + crtc_w;
-	dst->y1 = crtc_y;
-	dst->y2 = crtc_y + crtc_h;
-
 	if (INTEL_GEN(dev_priv) >= 9) {
 		ret = skl_check_plane_surface(crtc_state, state);
 		if (ret)
-- 
2.17.0

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

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/3] drm/rect: Round above 1 << 16 upwards to correct scale calculation functions.

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/rect: Round above 1 << 16 upwards to correct scale calculation functions.
URL   : https://patchwork.freedesktop.org/series/42317/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
3e26302006d1 drm/rect: Round above 1 << 16 upwards to correct scale calculation functions.
-:25: WARNING:UNNECESSARY_ELSE: else is not generally useful after a break or return
#25: FILE: drivers/gpu/drm/drm_rect.c:111:
+		return DIV_ROUND_UP(src, dst);
+	else

total: 0 errors, 1 warnings, 0 checks, 47 lines checked
4362fe643c06 drm/rect: Handle rounding errors in drm_rect_clip_scaled
-:33: WARNING:UNNECESSARY_ELSE: else is not generally useful after a break or return
#33: FILE: drivers/gpu/drm/drm_rect.c:65:
+		return DIV_ROUND_UP(src, dst);
+	else

-:52: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'oldscale' - possible side-effects?
#52: FILE: drivers/gpu/drm/drm_rect.c:98:
+#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)

-:52: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'fn' - possible side-effects?
#52: FILE: drivers/gpu/drm/drm_rect.c:98:
+#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)

-:55: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#55: FILE: drivers/gpu/drm/drm_rect.c:101:
+ \$

-:58: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#58: FILE: drivers/gpu/drm/drm_rect.c:104:
+ \$

-:59: CHECK:UNNECESSARY_PARENTHESES: Unnecessary parentheses around 'oldscale < 0x10000'
#59: FILE: drivers/gpu/drm/drm_rect.c:105:
+		if ((oldscale < 0x10000) != (newscale < 0x10000)) { \

-:59: CHECK:UNNECESSARY_PARENTHESES: Unnecessary parentheses around 'newscale < 0x10000'
#59: FILE: drivers/gpu/drm/drm_rect.c:105:
+		if ((oldscale < 0x10000) != (newscale < 0x10000)) { \

-:66: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#66: FILE: drivers/gpu/drm/drm_rect.c:112:
+ } while (0)$

total: 0 errors, 4 warnings, 4 checks, 102 lines checked
968d77e5a49f drm/i915: Do not adjust scale when out of bounds, v2.
-:180: CHECK:OPEN_ENDED_LINE: Lines should not end with a '('
#180: FILE: drivers/gpu/drm/i915/intel_sprite.c:1030:
+			if (INTEL_GEN(dev_priv) < 9 && (

total: 0 errors, 0 warnings, 1 checks, 179 lines checked

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

✗ Fi.CI.SPARSE: warning for series starting with [1/3] drm/rect: Round above 1 << 16 upwards to correct scale calculation functions.

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/rect: Round above 1 << 16 upwards to correct scale calculation functions.
URL   : https://patchwork.freedesktop.org/series/42317/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm/rect: Round above 1 << 16 upwards to correct scale calculation functions.
Okay!

Commit: drm/rect: Handle rounding errors in drm_rect_clip_scaled
+drivers/gpu/drm/drm_rect.c:117:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:117:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:117:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:117:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:117:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:117:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:117:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:124:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:124:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:124:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:124:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:124:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:124:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:124:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:131:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:131:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:131:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:131:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:131:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:131:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:131:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:137:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:137:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:137:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:137:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:137:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:137:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:137:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/drm_rect.c:77:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/drm_rect.c:77:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/drm_rect.c:77:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/drm_rect.c:77:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/drm_rect.c:77:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/drm_rect.c:77:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/drm_rect.c:77:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/drm_rect.c:82:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/drm_rect.c:82:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/drm_rect.c:82:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/drm_rect.c:82:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/drm_rect.c:82:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/drm_rect.c:82:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/drm_rect.c:82:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/drm_rect.c:87:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/drm_rect.c:87:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/drm_rect.c:87:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/drm_rect.c:87:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/drm_rect.c:87:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/drm_rect.c:87:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/drm_rect.c:87:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/drm_rect.c:92:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/drm_rect.c:92:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/drm_rect.c:92:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/drm_rect.c:92:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/drm_rect.c:92:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/drm_rect.c:92:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/drm_rect.c:92:27: warning: expression using sizeof(void)

Commit: drm/i915: Do not adjust scale when out of bounds, v2.
Okay!

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

✓ Fi.CI.BAT: success for series starting with [1/3] drm/rect: Round above 1 << 16 upwards to correct scale calculation functions.

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/rect: Round above 1 << 16 upwards to correct scale calculation functions.
URL   : https://patchwork.freedesktop.org/series/42317/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4100 -> Patchwork_8808 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/42317/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_8808 that come from known issues:

  === IGT changes ===

    ==== Possible fixes ====

    igt@debugfs_test@read_all_entries:
      fi-snb-2520m:       INCOMPLETE (fdo#103713) -> PASS

    igt@kms_flip@basic-flip-vs-dpms:
      fi-bxt-dsi:         INCOMPLETE (fdo#103927) -> PASS

    
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927


== Participating hosts (37 -> 35) ==

  Missing    (2): fi-ilk-m540 fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4100 -> Patchwork_8808

  CI_DRM_4100: 05cc701ce9d22b01f2f4afa9fddd521b931ed163 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4449: 0350f0e7f6a0e07281445fc3082aa70419f4aac7 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8808: 968d77e5a49fd4ff0001f0b305c67cfeadfc2b65 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4449: ad8992d3fb27fd604b9ab15e7963c42421ced85c @ git://anongit.freedesktop.org/piglit


== Linux commits ==

968d77e5a49f drm/i915: Do not adjust scale when out of bounds, v2.
4362fe643c06 drm/rect: Handle rounding errors in drm_rect_clip_scaled
3e26302006d1 drm/rect: Round above 1 << 16 upwards to correct scale calculation functions.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8808/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for series starting with [1/3] drm/rect: Round above 1 << 16 upwards to correct scale calculation functions.

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/rect: Round above 1 << 16 upwards to correct scale calculation functions.
URL   : https://patchwork.freedesktop.org/series/42317/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4100_full -> Patchwork_8808_full =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_8808_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8808_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/42317/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_8808_full:

  === IGT changes ===

    ==== Possible regressions ====

    igt@kms_color@pipe-c-ctm-0-5:
      shard-kbl:          PASS -> FAIL

    
    ==== Warnings ====

    igt@gem_exec_schedule@deep-render:
      shard-kbl:          SKIP -> PASS

    igt@gem_mocs_settings@mocs-rc6-bsd1:
      shard-kbl:          PASS -> SKIP

    
== Known issues ==

  Here are the changes found in Patchwork_8808_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_flip@2x-flip-vs-absolute-wf_vblank:
      shard-hsw:          PASS -> FAIL (fdo#100368) +1

    igt@kms_flip@blocking-absolute-wf_vblank:
      shard-kbl:          PASS -> DMESG-WARN (fdo#103558, fdo#105602) +3

    igt@kms_flip@modeset-vs-vblank-race-interruptible:
      shard-glk:          PASS -> FAIL (fdo#103060)

    igt@kms_flip@plain-flip-fb-recreate-interruptible:
      shard-glk:          PASS -> FAIL (fdo#100368) +1

    igt@kms_rotation_crc@primary-rotation-180:
      shard-apl:          PASS -> FAIL (fdo#103925)

    igt@kms_sysfs_edid_timing:
      shard-apl:          PASS -> WARN (fdo#100047)

    
    ==== Possible fixes ====

    igt@kms_cursor_legacy@2x-long-flip-vs-cursor-legacy:
      shard-hsw:          FAIL (fdo#104873) -> PASS

    igt@kms_flip@dpms-vs-vblank-race:
      shard-glk:          FAIL (fdo#103060) -> PASS

    igt@kms_flip@dpms-vs-vblank-race-interruptible:
      shard-apl:          FAIL (fdo#103060) -> PASS

    igt@kms_flip@flip-vs-expired-vblank:
      shard-glk:          FAIL (fdo#102887, fdo#105363) -> PASS

    igt@kms_flip@flip-vs-wf_vblank-interruptible:
      shard-hsw:          FAIL (fdo#103928) -> PASS
      shard-glk:          FAIL (fdo#100368) -> PASS +1
      shard-apl:          FAIL (fdo#105312) -> PASS

    igt@kms_flip@plain-flip-fb-recreate:
      shard-hsw:          FAIL (fdo#100368) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render:
      shard-apl:          FAIL (fdo#103167) -> PASS

    
  fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
  fdo#103928 https://bugs.freedesktop.org/show_bug.cgi?id=103928
  fdo#104873 https://bugs.freedesktop.org/show_bug.cgi?id=104873
  fdo#105312 https://bugs.freedesktop.org/show_bug.cgi?id=105312
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602


== Participating hosts (6 -> 5) ==

  Missing    (1): shard-glkb 


== Build changes ==

    * Linux: CI_DRM_4100 -> Patchwork_8808

  CI_DRM_4100: 05cc701ce9d22b01f2f4afa9fddd521b931ed163 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4449: 0350f0e7f6a0e07281445fc3082aa70419f4aac7 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8808: 968d77e5a49fd4ff0001f0b305c67cfeadfc2b65 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4449: ad8992d3fb27fd604b9ab15e7963c42421ced85c @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8808/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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

-- 
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: [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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx