[PATCH 0/5] [PATCH 0/5] drm: Fix rounding errors and use scaling in i915, v2.

[PATCH 0/5] [PATCH 0/5] drm: Fix rounding errors and use scaling in i915, v2.

[Maarten Lankhorst]

There were some small rounding errors in when clamping with
1.0001 and 0.9999 scaling, solve these and add a testcase for drm
helpers, which can be used to prevent more of these errors in the
future.

The new handling in drm_rect_clip_scaled rounds scaling towards 1.0x but
because the rounding error is only 1 pixel with the new math, we won't
ever get in a situation where we go from 1.001 to .999

Maarten Lankhorst (5):
  drm/rect: Round above 1 << 16 upwards to correct scale calculation
    functions.
  drm/rect: Handle rounding errors in drm_rect_clip_scaled, v3.
  drm/i915: Do not adjust scale when out of bounds, v2.
  drm/selftests: Rename the Kconfig option to CONFIG_DRM_DEBUG_SELFTEST
  drm/selftests: Add drm helper selftest

 drivers/gpu/drm/Kconfig                       |   9 +-
 drivers/gpu/drm/Makefile                      |   2 +-
 drivers/gpu/drm/drm_atomic_helper.c           |   2 +-
 drivers/gpu/drm/drm_rect.c                    |  62 ++++-
 drivers/gpu/drm/i915/intel_sprite.c           | 144 +++-------
 drivers/gpu/drm/selftests/Makefile            |   2 +-
 .../gpu/drm/selftests/drm_helper_selftests.h  |   9 +
 drivers/gpu/drm/selftests/test-drm-helper.c   | 247 ++++++++++++++++++
 include/drm/drm_rect.h                        |   3 +-
 9 files changed, 348 insertions(+), 132 deletions(-)
 create mode 100644 drivers/gpu/drm/selftests/drm_helper_selftests.h
 create mode 100644 drivers/gpu/drm/selftests/test-drm-helper.c

-- 
2.17.0

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

[PATCH 1/5] 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 a3783ecea297..4735526297aa 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/5] drm/rect: Handle rounding errors in drm_rect_clip_scaled, v3.

[Maarten Lankhorst]

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);
+
+	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);
-- 
2.17.0

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

[PATCH 3/5] 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 44d7aca222b0..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);
-
-	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

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

[PATCH 4/5] drm/selftests: Rename the Kconfig option to CONFIG_DRM_DEBUG_SELFTEST

[Maarten Lankhorst]

We want to add more DRM selftests, and there's not much point in
having a Kconfig option for every single one of them, so make
a generic one.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/Kconfig            | 8 ++++----
 drivers/gpu/drm/Makefile           | 2 +-
 drivers/gpu/drm/selftests/Makefile | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 757825ac60df..d684855b95c2 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -49,16 +49,16 @@ config DRM_DEBUG_MM
 
 	  If in doubt, say "N".
 
-config DRM_DEBUG_MM_SELFTEST
-	tristate "kselftests for DRM range manager (struct drm_mm)"
+config DRM_DEBUG_SELFTEST
+	tristate "kselftests for DRM"
 	depends on DRM
 	depends on DEBUG_KERNEL
 	select PRIME_NUMBERS
 	select DRM_LIB_RANDOM
 	default n
 	help
-	  This option provides a kernel module that can be used to test
-	  the DRM range manager (drm_mm) and its API. This option is not
+	  This option provides kernel modules that can be used to run
+	  various selftests on parts of the DRM api. This option is not
 	  useful for distributions or general kernels, but only for kernel
 	  developers working on DRM and associated drivers.
 
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 9d66657ea117..4becc245e359 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -43,7 +43,7 @@ drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
 drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
 
 obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
-obj-$(CONFIG_DRM_DEBUG_MM_SELFTEST) += selftests/
+obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
 
 obj-$(CONFIG_DRM)	+= drm.o
 obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o
diff --git a/drivers/gpu/drm/selftests/Makefile b/drivers/gpu/drm/selftests/Makefile
index 4aebfc7f27d4..f7dd66e859a9 100644
--- a/drivers/gpu/drm/selftests/Makefile
+++ b/drivers/gpu/drm/selftests/Makefile
@@ -1 +1 @@
-obj-$(CONFIG_DRM_DEBUG_MM_SELFTEST) += test-drm_mm.o
+obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o
-- 
2.17.0

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

[PATCH 5/5] drm/selftests: Add drm helper selftest

[Maarten Lankhorst]

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/Kconfig                       |   1 +
 drivers/gpu/drm/selftests/Makefile            |   2 +-
 .../gpu/drm/selftests/drm_helper_selftests.h  |   9 +
 drivers/gpu/drm/selftests/test-drm-helper.c   | 247 ++++++++++++++++++
 4 files changed, 258 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/selftests/drm_helper_selftests.h
 create mode 100644 drivers/gpu/drm/selftests/test-drm-helper.c

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index d684855b95c2..28d059007ac2 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -55,6 +55,7 @@ config DRM_DEBUG_SELFTEST
 	depends on DEBUG_KERNEL
 	select PRIME_NUMBERS
 	select DRM_LIB_RANDOM
+	select DRM_KMS_HELPER
 	default n
 	help
 	  This option provides kernel modules that can be used to run
diff --git a/drivers/gpu/drm/selftests/Makefile b/drivers/gpu/drm/selftests/Makefile
index f7dd66e859a9..9fc349fa18e9 100644
--- a/drivers/gpu/drm/selftests/Makefile
+++ b/drivers/gpu/drm/selftests/Makefile
@@ -1 +1 @@
-obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o
+obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm-helper.o
diff --git a/drivers/gpu/drm/selftests/drm_helper_selftests.h b/drivers/gpu/drm/selftests/drm_helper_selftests.h
new file mode 100644
index 000000000000..9771290ed228
--- /dev/null
+++ b/drivers/gpu/drm/selftests/drm_helper_selftests.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* List each unit test as selftest(name, function)
+ *
+ * The name is used as both an enum and expanded as igt__name to create
+ * a module parameter. It must be unique and legal for a C identifier.
+ *
+ * Tests are executed in order by igt/drm_selftests_helper
+ */
+selftest(check_plane_state, igt_check_plane_state)
diff --git a/drivers/gpu/drm/selftests/test-drm-helper.c b/drivers/gpu/drm/selftests/test-drm-helper.c
new file mode 100644
index 000000000000..a015712b43e8
--- /dev/null
+++ b/drivers/gpu/drm/selftests/test-drm-helper.c
@@ -0,0 +1,247 @@
+/*
+ * Test cases for the drm_kms_helper functions
+ */
+
+#define pr_fmt(fmt) "drm_kms_helper: " fmt
+
+#include <linux/module.h>
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_plane_helper.h>
+#include <drm/drm_modes.h>
+
+#define TESTS "drm_helper_selftests.h"
+#include "drm_selftest.h"
+
+#define FAIL(test, msg, ...) \
+	do { \
+		if (test) { \
+			pr_err("%s/%u: " msg, __FUNCTION__, __LINE__, ##__VA_ARGS__); \
+			return -EINVAL; \
+		} \
+	} while (0)
+
+#define FAIL_ON(x) FAIL((x), "%s", "FAIL_ON(" __stringify(x) ")\n")
+
+static void set_src(struct drm_plane_state *plane_state,
+		    unsigned src_x, unsigned src_y,
+		    unsigned src_w, unsigned src_h)
+{
+	plane_state->src_x = src_x;
+	plane_state->src_y = src_y;
+	plane_state->src_w = src_w;
+	plane_state->src_h = src_h;
+}
+
+static bool check_src_eq(struct drm_plane_state *plane_state,
+			 unsigned src_x, unsigned src_y,
+			 unsigned src_w, unsigned src_h)
+{
+	if (plane_state->src.x1 < 0) {
+		pr_err("src x coordinate %x should never be below 0.\n", plane_state->src.x1);
+		drm_rect_debug_print("src: ", &plane_state->src, true);
+		return false;
+	}
+	if (plane_state->src.y1 < 0) {
+		pr_err("src y coordinate %x should never be below 0.\n", plane_state->src.y1);
+		drm_rect_debug_print("src: ", &plane_state->src, true);
+		return false;
+	}
+
+	if (plane_state->src.x1 != src_x ||
+	    plane_state->src.y1 != src_y ||
+	    drm_rect_width(&plane_state->src) != src_w ||
+	    drm_rect_height(&plane_state->src) != src_h) {
+		drm_rect_debug_print("src: ", &plane_state->src, true);
+		return false;
+	}
+
+	return true;
+}
+
+static void set_crtc(struct drm_plane_state *plane_state,
+		     int crtc_x, int crtc_y,
+		     unsigned crtc_w, unsigned crtc_h)
+{
+	plane_state->crtc_x = crtc_x;
+	plane_state->crtc_y = crtc_y;
+	plane_state->crtc_w = crtc_w;
+	plane_state->crtc_h = crtc_h;
+}
+
+static bool check_crtc_eq(struct drm_plane_state *plane_state,
+			  int crtc_x, int crtc_y,
+			  unsigned crtc_w, unsigned crtc_h)
+{
+	if (plane_state->dst.x1 != crtc_x ||
+	    plane_state->dst.y1 != crtc_y ||
+	    drm_rect_width(&plane_state->dst) != crtc_w ||
+	    drm_rect_height(&plane_state->dst) != crtc_h) {
+		drm_rect_debug_print("dst: ", &plane_state->dst, false);
+
+		return false;
+	}
+
+	return true;
+}
+
+static int igt_check_plane_state(void *ignored)
+{
+	int ret;
+
+	const struct drm_crtc_state crtc_state = {
+		.crtc = ZERO_SIZE_PTR,
+		.enable = true,
+		.active = true,
+		.mode = {
+			DRM_MODE("1024x768", 0, 65000, 1024, 1048,
+				1184, 1344, 0, 768, 771, 777, 806, 0,
+				DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC)
+		},
+	};
+	struct drm_framebuffer fb = {
+		.width = 2048,
+		.height = 2048
+	};
+	struct drm_plane_state plane_state = {
+		.crtc = ZERO_SIZE_PTR,
+		.fb = &fb,
+		.rotation = DRM_MODE_ROTATE_0
+	};
+
+	/* Simple clipping, no scaling. */
+	set_src(&plane_state, 0, 0, fb.width << 16, fb.height << 16);
+	set_crtc(&plane_state, 0, 0, fb.width, fb.height);
+	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
+						  DRM_PLANE_HELPER_NO_SCALING,
+						  DRM_PLANE_HELPER_NO_SCALING,
+						  false, false);
+	FAIL(ret < 0, "Simple clipping check should pass\n");
+	FAIL_ON(!plane_state.visible);
+	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 1024 << 16, 768 << 16));
+	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
+
+	/* Rotated clipping + reflection, no scaling. */
+	plane_state.rotation = DRM_MODE_ROTATE_90 | DRM_MODE_REFLECT_X;
+	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
+						  DRM_PLANE_HELPER_NO_SCALING,
+						  DRM_PLANE_HELPER_NO_SCALING,
+						  false, false);
+	FAIL(ret < 0, "Rotated clipping check should pass\n");
+	FAIL_ON(!plane_state.visible);
+	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 768 << 16, 1024 << 16));
+	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
+	plane_state.rotation = DRM_MODE_ROTATE_0;
+
+	/* Check whether positioning works correctly. */
+	set_src(&plane_state, 0, 0, 1023 << 16, 767 << 16);
+	set_crtc(&plane_state, 0, 0, 1023, 767);
+	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
+						  DRM_PLANE_HELPER_NO_SCALING,
+						  DRM_PLANE_HELPER_NO_SCALING,
+						  false, false);
+	FAIL(!ret, "Should not be able to position on the crtc with can_position=false\n");
+
+	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
+						  DRM_PLANE_HELPER_NO_SCALING,
+						  DRM_PLANE_HELPER_NO_SCALING,
+						  true, false);
+	FAIL(ret < 0, "Simple positioning should work\n");
+	FAIL_ON(!plane_state.visible);
+	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 1023 << 16, 767 << 16));
+	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1023, 767));
+
+	/* Simple scaling tests. */
+	set_src(&plane_state, 0, 0, 512 << 16, 384 << 16);
+	set_crtc(&plane_state, 0, 0, 1024, 768);
+	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
+						  0x8001,
+						  DRM_PLANE_HELPER_NO_SCALING,
+						  false, false);
+	FAIL(!ret, "Upscaling out of range should fail.\n");
+	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
+						  0x8000,
+						  DRM_PLANE_HELPER_NO_SCALING,
+						  false, false);
+	FAIL(ret < 0, "Upscaling exactly 2x should work\n");
+	FAIL_ON(!plane_state.visible);
+	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 512 << 16, 384 << 16));
+	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
+
+	set_src(&plane_state, 0, 0, 2048 << 16, 1536 << 16);
+	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
+						  DRM_PLANE_HELPER_NO_SCALING,
+						  0x1ffff, false, false);
+	FAIL(!ret, "Downscaling out of range should fail.\n");
+	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
+						  DRM_PLANE_HELPER_NO_SCALING,
+						  0x20000, false, false);
+	FAIL(ret < 0, "Should succeed with exact scaling limit\n");
+	FAIL_ON(!plane_state.visible);
+	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 2048 << 16, 1536 << 16));
+	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
+
+	/* Testing rounding errors. */
+	set_src(&plane_state, 0, 0, 0x40001, 0x40001);
+	set_crtc(&plane_state, 1022, 766, 4, 4);
+	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
+						  DRM_PLANE_HELPER_NO_SCALING,
+						  0x10001,
+						  true, false);
+	FAIL(ret < 0, "Should succeed by clipping to exact multiple");
+	FAIL_ON(!plane_state.visible);
+	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 2 << 16, 2 << 16));
+	FAIL_ON(!check_crtc_eq(&plane_state, 1022, 766, 2, 2));
+
+	set_src(&plane_state, 0x20001, 0x20001, 0x4040001, 0x3040001);
+	set_crtc(&plane_state, -2, -2, 1028, 772);
+	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
+						  DRM_PLANE_HELPER_NO_SCALING,
+						  0x10001,
+						  false, false);
+	FAIL(ret < 0, "Should succeed by clipping to exact multiple");
+	FAIL_ON(!plane_state.visible);
+	FAIL_ON(!check_src_eq(&plane_state, 0x40002, 0x40002, 1024 << 16, 768 << 16));
+	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
+
+	set_src(&plane_state, 0, 0, 0x3ffff, 0x3ffff);
+	set_crtc(&plane_state, 1022, 766, 4, 4);
+	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
+						  0xffff,
+						  DRM_PLANE_HELPER_NO_SCALING,
+						  true, false);
+	FAIL(ret < 0, "Should succeed by clipping to exact multiple");
+	FAIL_ON(!plane_state.visible);
+	/* Should not be rounded to 0x20001, which would be upscaling. */
+	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 2 << 16, 2 << 16));
+	FAIL_ON(!check_crtc_eq(&plane_state, 1022, 766, 2, 2));
+
+	set_src(&plane_state, 0x1ffff, 0x1ffff, 0x403ffff, 0x303ffff);
+	set_crtc(&plane_state, -2, -2, 1028, 772);
+	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
+						  0xffff,
+						  DRM_PLANE_HELPER_NO_SCALING,
+						  false, false);
+	FAIL(ret < 0, "Should succeed by clipping to exact multiple");
+	FAIL_ON(!plane_state.visible);
+	FAIL_ON(!check_src_eq(&plane_state, 0x3fffe, 0x3fffe, 1024 << 16, 768 << 16));
+	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
+
+	return 0;
+}
+
+#include "drm_selftest.c"
+
+static int __init test_drm_helper_init(void)
+{
+	int err;
+
+	err = run_selftests(selftests, ARRAY_SIZE(selftests), NULL);
+
+	return err > 0 ? 0 : err;
+}
+
+module_init(test_drm_helper_init);
+
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL");
-- 
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 drm: Fix rounding errors and use scaling in i915, v2.

[Patchwork]

== Series Details ==

Series: drm: Fix rounding errors and use scaling in i915, v2.
URL   : https://patchwork.freedesktop.org/series/42631/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
9e4d573cb796 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
864115c2cad5 drm/rect: Handle rounding errors in drm_rect_clip_scaled, v3.
7613b3e524ea 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
70ddb226fb85 drm/selftests: Rename the Kconfig option to CONFIG_DRM_DEBUG_SELFTEST
037476edddf2 drm/selftests: Add drm helper selftest
-:28: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating?
#28: 
new file mode 100644

-:48: WARNING:SPDX_LICENSE_TAG: Missing or malformed SPDX-License-Identifier tag in line 1
#48: FILE: drivers/gpu/drm/selftests/test-drm-helper.c:1:
+/*

-:63: WARNING:MACRO_WITH_FLOW_CONTROL: Macros with flow control statements should be avoided
#63: FILE: drivers/gpu/drm/selftests/test-drm-helper.c:16:
+#define FAIL(test, msg, ...) \
+	do { \
+		if (test) { \
+			pr_err("%s/%u: " msg, __FUNCTION__, __LINE__, ##__VA_ARGS__); \
+			return -EINVAL; \
+		} \
+	} while (0)

-:66: WARNING:USE_FUNC: __func__ should be used instead of gcc specific __FUNCTION__
#66: FILE: drivers/gpu/drm/selftests/test-drm-helper.c:19:
+			pr_err("%s/%u: " msg, __FUNCTION__, __LINE__, ##__VA_ARGS__); \

-:71: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'x' - possible side-effects?
#71: FILE: drivers/gpu/drm/selftests/test-drm-helper.c:24:
+#define FAIL_ON(x) FAIL((x), "%s", "FAIL_ON(" __stringify(x) ")\n")

-:74: WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
#74: FILE: drivers/gpu/drm/selftests/test-drm-helper.c:27:
+		    unsigned src_x, unsigned src_y,

-:74: WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
#74: FILE: drivers/gpu/drm/selftests/test-drm-helper.c:27:
+		    unsigned src_x, unsigned src_y,

-:75: WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
#75: FILE: drivers/gpu/drm/selftests/test-drm-helper.c:28:
+		    unsigned src_w, unsigned src_h)

-:75: WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
#75: FILE: drivers/gpu/drm/selftests/test-drm-helper.c:28:
+		    unsigned src_w, unsigned src_h)

-:84: WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
#84: FILE: drivers/gpu/drm/selftests/test-drm-helper.c:37:
+			 unsigned src_x, unsigned src_y,

-:84: WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
#84: FILE: drivers/gpu/drm/selftests/test-drm-helper.c:37:
+			 unsigned src_x, unsigned src_y,

-:85: WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
#85: FILE: drivers/gpu/drm/selftests/test-drm-helper.c:38:
+			 unsigned src_w, unsigned src_h)

-:85: WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
#85: FILE: drivers/gpu/drm/selftests/test-drm-helper.c:38:
+			 unsigned src_w, unsigned src_h)

-:111: WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
#111: FILE: drivers/gpu/drm/selftests/test-drm-helper.c:64:
+		     unsigned crtc_w, unsigned crtc_h)

-:111: WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
#111: FILE: drivers/gpu/drm/selftests/test-drm-helper.c:64:
+		     unsigned crtc_w, unsigned crtc_h)

-:121: WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
#121: FILE: drivers/gpu/drm/selftests/test-drm-helper.c:74:
+			  unsigned crtc_w, unsigned crtc_h)

-:121: WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
#121: FILE: drivers/gpu/drm/selftests/test-drm-helper.c:74:
+			  unsigned crtc_w, unsigned crtc_h)

-:145: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#145: FILE: drivers/gpu/drm/selftests/test-drm-helper.c:98:
+			DRM_MODE("1024x768", 0, 65000, 1024, 1048,
+				1184, 1344, 0, 768, 771, 777, 806, 0,

total: 0 errors, 16 warnings, 2 checks, 265 lines checked

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

✗ Fi.CI.SPARSE: warning for drm: Fix rounding errors and use scaling in i915, v2.

[Patchwork]

== Series Details ==

Series: drm: Fix rounding errors and use scaling in i915, v2.
URL   : https://patchwork.freedesktop.org/series/42631/
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, v3.
+drivers/gpu/drm/drm_rect.c:102:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:102:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:102:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:102:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:102:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:102:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:102:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:110:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:110:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:110:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:110:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:110:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:110:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:110: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)
+drivers/gpu/drm/drm_rect.c:86:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:86:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:86:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:86:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:86:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:86:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:86:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:94:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:94:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:94:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:94:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:94:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:94:27: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_rect.c:94:27: warning: expression using sizeof(void)

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

Commit: drm/selftests: Rename the Kconfig option to CONFIG_DRM_DEBUG_SELFTEST
+Error in reading or end of file.

Commit: drm/selftests: Add drm helper selftest
Okay!

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

✓ Fi.CI.BAT: success for drm: Fix rounding errors and use scaling in i915, v2.

[Patchwork]

== Series Details ==

Series: drm: Fix rounding errors and use scaling in i915, v2.
URL   : https://patchwork.freedesktop.org/series/42631/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4131 -> Patchwork_8896 =

== Summary - SUCCESS ==

  No regressions found.

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

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_frontbuffer_tracking@basic:
      fi-hsw-peppy:       PASS -> DMESG-FAIL (fdo#106103)

    
    ==== Possible fixes ====

    igt@drv_module_reload@basic-reload:
      fi-bsw-n3050:       DMESG-FAIL (fdo#106373) -> PASS

    igt@gem_exec_suspend@basic-s4-devices:
      fi-skl-guc:         FAIL (fdo#104699) -> PASS

    igt@gem_mmap_gtt@basic-small-bo-tiledx:
      fi-gdg-551:         FAIL (fdo#102575) -> PASS

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-skl-6770hq:      FAIL (fdo#100368) -> PASS

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b-frame-sequence:
      fi-skl-6770hq:      FAIL (fdo#103481) -> PASS +1

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         INCOMPLETE (fdo#103927) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
  fdo#103481 https://bugs.freedesktop.org/show_bug.cgi?id=103481
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#104699 https://bugs.freedesktop.org/show_bug.cgi?id=104699
  fdo#106103 https://bugs.freedesktop.org/show_bug.cgi?id=106103
  fdo#106373 https://bugs.freedesktop.org/show_bug.cgi?id=106373


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

  Additional (1): fi-byt-j1900 
  Missing    (3): fi-ctg-p8600 fi-ilk-m540 fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4131 -> Patchwork_8896

  CI_DRM_4131: 46d3a67e7a5611ef8af00cb7adebf03817856645 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4459: 1b8977e08031253d61b4641bc21e5c7a990d4a4f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8896: 037476edddf2d61b9a4d05f1d53eaf42faec6373 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4459: f74d92e704849610364b4474a2c67ea2008c14e0 @ git://anongit.freedesktop.org/piglit


== Linux commits ==

037476edddf2 drm/selftests: Add drm helper selftest
70ddb226fb85 drm/selftests: Rename the Kconfig option to CONFIG_DRM_DEBUG_SELFTEST
7613b3e524ea drm/i915: Do not adjust scale when out of bounds, v2.
864115c2cad5 drm/rect: Handle rounding errors in drm_rect_clip_scaled, v3.
9e4d573cb796 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_8896/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Thu, May 03, 2018 at 01:22:13PM +0200, Maarten Lankhorst wrote:
> 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 a3783ecea297..4735526297aa 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

The "if above up" part doesn't read all that well to me. Maybe write it
out fully "If the scale is above 1 << 16, round up"?

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

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

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

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

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

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

[Ville Syrjälä]

On Thu, May 03, 2018 at 01:22:15PM +0200, Maarten Lankhorst wrote:
> 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 44d7aca222b0..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);
> -
> -	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");

The debug message can now be misleading since we're checking far more
than the source size limits here. I think we should split this up into
more specific debug messages. But that could be a followup.

We should split this mess up into platform specific functions anyway.

We're still missing the scaling factor check after we throw out the
sub-pixel coordinates, but since that can only make src smaller it can
only decrease the scaling factor, which is generally safe on our hw.
Or at least used to be. Which is why we didn't check for it before either.

Anyways, looks pretty good so:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

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

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/selftests: Add drm helper selftest

[Ville Syrjälä]

On Thu, May 03, 2018 at 01:22:17PM +0200, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/Kconfig                       |   1 +
>  drivers/gpu/drm/selftests/Makefile            |   2 +-
>  .../gpu/drm/selftests/drm_helper_selftests.h  |   9 +
>  drivers/gpu/drm/selftests/test-drm-helper.c   | 247 ++++++++++++++++++
>  4 files changed, 258 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/selftests/drm_helper_selftests.h
>  create mode 100644 drivers/gpu/drm/selftests/test-drm-helper.c
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index d684855b95c2..28d059007ac2 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -55,6 +55,7 @@ config DRM_DEBUG_SELFTEST
>  	depends on DEBUG_KERNEL
>  	select PRIME_NUMBERS
>  	select DRM_LIB_RANDOM
> +	select DRM_KMS_HELPER
>  	default n
>  	help
>  	  This option provides kernel modules that can be used to run
> diff --git a/drivers/gpu/drm/selftests/Makefile b/drivers/gpu/drm/selftests/Makefile
> index f7dd66e859a9..9fc349fa18e9 100644
> --- a/drivers/gpu/drm/selftests/Makefile
> +++ b/drivers/gpu/drm/selftests/Makefile
> @@ -1 +1 @@
> -obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o
> +obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm-helper.o
> diff --git a/drivers/gpu/drm/selftests/drm_helper_selftests.h b/drivers/gpu/drm/selftests/drm_helper_selftests.h
> new file mode 100644
> index 000000000000..9771290ed228
> --- /dev/null
> +++ b/drivers/gpu/drm/selftests/drm_helper_selftests.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* List each unit test as selftest(name, function)
> + *
> + * The name is used as both an enum and expanded as igt__name to create
> + * a module parameter. It must be unique and legal for a C identifier.
> + *
> + * Tests are executed in order by igt/drm_selftests_helper
> + */
> +selftest(check_plane_state, igt_check_plane_state)
> diff --git a/drivers/gpu/drm/selftests/test-drm-helper.c b/drivers/gpu/drm/selftests/test-drm-helper.c
> new file mode 100644
> index 000000000000..a015712b43e8
> --- /dev/null
> +++ b/drivers/gpu/drm/selftests/test-drm-helper.c
> @@ -0,0 +1,247 @@
> +/*
> + * Test cases for the drm_kms_helper functions
> + */
> +
> +#define pr_fmt(fmt) "drm_kms_helper: " fmt
> +
> +#include <linux/module.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_plane_helper.h>
> +#include <drm/drm_modes.h>
> +
> +#define TESTS "drm_helper_selftests.h"
> +#include "drm_selftest.h"
> +
> +#define FAIL(test, msg, ...) \
> +	do { \
> +		if (test) { \
> +			pr_err("%s/%u: " msg, __FUNCTION__, __LINE__, ##__VA_ARGS__); \
> +			return -EINVAL; \
> +		} \
> +	} while (0)
> +
> +#define FAIL_ON(x) FAIL((x), "%s", "FAIL_ON(" __stringify(x) ")\n")
> +
> +static void set_src(struct drm_plane_state *plane_state,
> +		    unsigned src_x, unsigned src_y,
> +		    unsigned src_w, unsigned src_h)
> +{
> +	plane_state->src_x = src_x;
> +	plane_state->src_y = src_y;
> +	plane_state->src_w = src_w;
> +	plane_state->src_h = src_h;
> +}
> +
> +static bool check_src_eq(struct drm_plane_state *plane_state,
> +			 unsigned src_x, unsigned src_y,
> +			 unsigned src_w, unsigned src_h)
> +{
> +	if (plane_state->src.x1 < 0) {
> +		pr_err("src x coordinate %x should never be below 0.\n", plane_state->src.x1);
> +		drm_rect_debug_print("src: ", &plane_state->src, true);
> +		return false;
> +	}
> +	if (plane_state->src.y1 < 0) {
> +		pr_err("src y coordinate %x should never be below 0.\n", plane_state->src.y1);
> +		drm_rect_debug_print("src: ", &plane_state->src, true);
> +		return false;
> +	}
> +
> +	if (plane_state->src.x1 != src_x ||
> +	    plane_state->src.y1 != src_y ||
> +	    drm_rect_width(&plane_state->src) != src_w ||
> +	    drm_rect_height(&plane_state->src) != src_h) {
> +		drm_rect_debug_print("src: ", &plane_state->src, true);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +static void set_crtc(struct drm_plane_state *plane_state,
> +		     int crtc_x, int crtc_y,
> +		     unsigned crtc_w, unsigned crtc_h)
> +{
> +	plane_state->crtc_x = crtc_x;
> +	plane_state->crtc_y = crtc_y;
> +	plane_state->crtc_w = crtc_w;
> +	plane_state->crtc_h = crtc_h;
> +}
> +
> +static bool check_crtc_eq(struct drm_plane_state *plane_state,
> +			  int crtc_x, int crtc_y,
> +			  unsigned crtc_w, unsigned crtc_h)
> +{
> +	if (plane_state->dst.x1 != crtc_x ||
> +	    plane_state->dst.y1 != crtc_y ||
> +	    drm_rect_width(&plane_state->dst) != crtc_w ||
> +	    drm_rect_height(&plane_state->dst) != crtc_h) {
> +		drm_rect_debug_print("dst: ", &plane_state->dst, false);
> +
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +static int igt_check_plane_state(void *ignored)
> +{
> +	int ret;
> +
> +	const struct drm_crtc_state crtc_state = {
> +		.crtc = ZERO_SIZE_PTR,
> +		.enable = true,
> +		.active = true,
> +		.mode = {
> +			DRM_MODE("1024x768", 0, 65000, 1024, 1048,
> +				1184, 1344, 0, 768, 771, 777, 806, 0,
> +				DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC)
> +		},
> +	};
> +	struct drm_framebuffer fb = {
> +		.width = 2048,
> +		.height = 2048
> +	};
> +	struct drm_plane_state plane_state = {
> +		.crtc = ZERO_SIZE_PTR,
> +		.fb = &fb,
> +		.rotation = DRM_MODE_ROTATE_0
> +	};
> +
> +	/* Simple clipping, no scaling. */
> +	set_src(&plane_state, 0, 0, fb.width << 16, fb.height << 16);
> +	set_crtc(&plane_state, 0, 0, fb.width, fb.height);
> +	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
> +						  DRM_PLANE_HELPER_NO_SCALING,
> +						  DRM_PLANE_HELPER_NO_SCALING,
> +						  false, false);
> +	FAIL(ret < 0, "Simple clipping check should pass\n");
> +	FAIL_ON(!plane_state.visible);
> +	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 1024 << 16, 768 << 16));
> +	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
> +
> +	/* Rotated clipping + reflection, no scaling. */
> +	plane_state.rotation = DRM_MODE_ROTATE_90 | DRM_MODE_REFLECT_X;
> +	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
> +						  DRM_PLANE_HELPER_NO_SCALING,
> +						  DRM_PLANE_HELPER_NO_SCALING,
> +						  false, false);
> +	FAIL(ret < 0, "Rotated clipping check should pass\n");
> +	FAIL_ON(!plane_state.visible);
> +	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 768 << 16, 1024 << 16));
> +	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));

I guess we might want a few more rotated/reflected cases to make
sure the clipping affects each edge correctly.

> +	plane_state.rotation = DRM_MODE_ROTATE_0;
> +
> +	/* Check whether positioning works correctly. */
> +	set_src(&plane_state, 0, 0, 1023 << 16, 767 << 16);
> +	set_crtc(&plane_state, 0, 0, 1023, 767);
> +	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
> +						  DRM_PLANE_HELPER_NO_SCALING,
> +						  DRM_PLANE_HELPER_NO_SCALING,
> +						  false, false);
> +	FAIL(!ret, "Should not be able to position on the crtc with can_position=false\n");
> +
> +	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
> +						  DRM_PLANE_HELPER_NO_SCALING,
> +						  DRM_PLANE_HELPER_NO_SCALING,
> +						  true, false);
> +	FAIL(ret < 0, "Simple positioning should work\n");
> +	FAIL_ON(!plane_state.visible);
> +	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 1023 << 16, 767 << 16));
> +	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1023, 767));
> +
> +	/* Simple scaling tests. */
> +	set_src(&plane_state, 0, 0, 512 << 16, 384 << 16);
> +	set_crtc(&plane_state, 0, 0, 1024, 768);
> +	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
> +						  0x8001,
> +						  DRM_PLANE_HELPER_NO_SCALING,
> +						  false, false);
> +	FAIL(!ret, "Upscaling out of range should fail.\n");
> +	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
> +						  0x8000,
> +						  DRM_PLANE_HELPER_NO_SCALING,
> +						  false, false);
> +	FAIL(ret < 0, "Upscaling exactly 2x should work\n");
> +	FAIL_ON(!plane_state.visible);
> +	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 512 << 16, 384 << 16));
> +	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
> +
> +	set_src(&plane_state, 0, 0, 2048 << 16, 1536 << 16);
> +	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
> +						  DRM_PLANE_HELPER_NO_SCALING,
> +						  0x1ffff, false, false);
> +	FAIL(!ret, "Downscaling out of range should fail.\n");
> +	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
> +						  DRM_PLANE_HELPER_NO_SCALING,
> +						  0x20000, false, false);
> +	FAIL(ret < 0, "Should succeed with exact scaling limit\n");

"Downscaling exactly 2x should work\n"
to be consistent with the error from the previous test?

> +	FAIL_ON(!plane_state.visible);
> +	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 2048 << 16, 1536 << 16));
> +	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
> +
> +	/* Testing rounding errors. */
> +	set_src(&plane_state, 0, 0, 0x40001, 0x40001);
> +	set_crtc(&plane_state, 1022, 766, 4, 4);
> +	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
> +						  DRM_PLANE_HELPER_NO_SCALING,
> +						  0x10001,
> +						  true, false);
> +	FAIL(ret < 0, "Should succeed by clipping to exact multiple");

What does "exact multiple" mean for these? src and dst are not integer
multiples of each other in these tests so not sure what this is trying
to say.

> +	FAIL_ON(!plane_state.visible);
> +	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 2 << 16, 2 << 16));
> +	FAIL_ON(!check_crtc_eq(&plane_state, 1022, 766, 2, 2));
> +
> +	set_src(&plane_state, 0x20001, 0x20001, 0x4040001, 0x3040001);
> +	set_crtc(&plane_state, -2, -2, 1028, 772);
> +	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
> +						  DRM_PLANE_HELPER_NO_SCALING,
> +						  0x10001,
> +						  false, false);
> +	FAIL(ret < 0, "Should succeed by clipping to exact multiple");
> +	FAIL_ON(!plane_state.visible);
> +	FAIL_ON(!check_src_eq(&plane_state, 0x40002, 0x40002, 1024 << 16, 768 << 16));
> +	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
> +
> +	set_src(&plane_state, 0, 0, 0x3ffff, 0x3ffff);
> +	set_crtc(&plane_state, 1022, 766, 4, 4);
> +	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
> +						  0xffff,
> +						  DRM_PLANE_HELPER_NO_SCALING,
> +						  true, false);
> +	FAIL(ret < 0, "Should succeed by clipping to exact multiple");
> +	FAIL_ON(!plane_state.visible);
> +	/* Should not be rounded to 0x20001, which would be upscaling. */

"downscaling". src<dst so the "user" asked for upscaling. The other
tests don't have any comments though. Not sure why this one is
special.

> +	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 2 << 16, 2 << 16));
> +	FAIL_ON(!check_crtc_eq(&plane_state, 1022, 766, 2, 2));
> +
> +	set_src(&plane_state, 0x1ffff, 0x1ffff, 0x403ffff, 0x303ffff);
> +	set_crtc(&plane_state, -2, -2, 1028, 772);
> +	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
> +						  0xffff,
> +						  DRM_PLANE_HELPER_NO_SCALING,
> +						  false, false);
> +	FAIL(ret < 0, "Should succeed by clipping to exact multiple");
> +	FAIL_ON(!plane_state.visible);
> +	FAIL_ON(!check_src_eq(&plane_state, 0x3fffe, 0x3fffe, 1024 << 16, 768 << 16));
> +	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));

These are all very specific to the rounding behaviour you implemented,
but I guess that's what we want.

I guess we migth also want some tests for the fully clipped cases? Could
be added later though.

In general these look correct enough to me.
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +
> +	return 0;
> +}
> +
> +#include "drm_selftest.c"
> +
> +static int __init test_drm_helper_init(void)
> +{
> +	int err;
> +
> +	err = run_selftests(selftests, ARRAY_SIZE(selftests), NULL);
> +
> +	return err > 0 ? 0 : err;
> +}
> +
> +module_init(test_drm_helper_init);
> +
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL");
> -- 
> 2.17.0

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/selftests: Rename the Kconfig option to CONFIG_DRM_DEBUG_SELFTEST

[Ville Syrjälä]

On Thu, May 03, 2018 at 01:22:16PM +0200, Maarten Lankhorst wrote:
> We want to add more DRM selftests, and there's not much point in
> having a Kconfig option for every single one of them, so make
> a generic one.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Seems like a reasonable idea.

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

> ---
>  drivers/gpu/drm/Kconfig            | 8 ++++----
>  drivers/gpu/drm/Makefile           | 2 +-
>  drivers/gpu/drm/selftests/Makefile | 2 +-
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 757825ac60df..d684855b95c2 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -49,16 +49,16 @@ config DRM_DEBUG_MM
>  
>  	  If in doubt, say "N".
>  
> -config DRM_DEBUG_MM_SELFTEST
> -	tristate "kselftests for DRM range manager (struct drm_mm)"
> +config DRM_DEBUG_SELFTEST
> +	tristate "kselftests for DRM"
>  	depends on DRM
>  	depends on DEBUG_KERNEL
>  	select PRIME_NUMBERS
>  	select DRM_LIB_RANDOM
>  	default n
>  	help
> -	  This option provides a kernel module that can be used to test
> -	  the DRM range manager (drm_mm) and its API. This option is not
> +	  This option provides kernel modules that can be used to run
> +	  various selftests on parts of the DRM api. This option is not
>  	  useful for distributions or general kernels, but only for kernel
>  	  developers working on DRM and associated drivers.
>  
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 9d66657ea117..4becc245e359 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -43,7 +43,7 @@ drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
>  drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
>  
>  obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
> -obj-$(CONFIG_DRM_DEBUG_MM_SELFTEST) += selftests/
> +obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
>  
>  obj-$(CONFIG_DRM)	+= drm.o
>  obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o
> diff --git a/drivers/gpu/drm/selftests/Makefile b/drivers/gpu/drm/selftests/Makefile
> index 4aebfc7f27d4..f7dd66e859a9 100644
> --- a/drivers/gpu/drm/selftests/Makefile
> +++ b/drivers/gpu/drm/selftests/Makefile
> @@ -1 +1 @@
> -obj-$(CONFIG_DRM_DEBUG_MM_SELFTEST) += test-drm_mm.o
> +obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o
> -- 
> 2.17.0

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

✗ Fi.CI.IGT: failure for drm: Fix rounding errors and use scaling in i915, v2.

[Patchwork]

== Series Details ==

Series: drm: Fix rounding errors and use scaling in i915, v2.
URL   : https://patchwork.freedesktop.org/series/42631/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4131_full -> Patchwork_8896_full =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_8896_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8896_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/42631/revisions/1/mbox/

== Possible new issues ==

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

  === IGT changes ===

    ==== Possible regressions ====

    igt@kms_draw_crc@draw-method-rgb565-mmap-wc-xtiled:
      shard-glk:          PASS -> FAIL

    
    ==== Warnings ====

    igt@core_getstats:
      shard-glk:          PASS -> ( 2 PASS ) +86

    igt@core_prop_blob@blob-prop-core:
      shard-apl:          PASS -> ( 2 PASS ) +84

    igt@drm_import_export@flink:
      shard-hsw:          PASS -> ( 2 PASS ) +89

    igt@drm_vma_limiter_gtt:
      shard-kbl:          PASS -> ( 2 PASS ) +93

    igt@drv_hangman@error-state-capture-bsd1:
      shard-glk:          SKIP -> ( 2 SKIP ) +17

    igt@drv_module_reload@basic-no-display:
      shard-snb:          PASS -> ( 2 PASS ) +69

    igt@gem_busy@busy-bsd1:
      shard-apl:          SKIP -> ( 2 SKIP ) +17

    igt@gem_busy@extended-parallel-bsd1:
      shard-hsw:          SKIP -> ( 2 SKIP ) +14

    igt@gem_busy@extended-semaphore-blt:
      shard-snb:          SKIP -> ( 2 SKIP ) +34
      shard-kbl:          SKIP -> ( 2 SKIP ) +8

    igt@gem_mocs_settings@mocs-rc6-bsd2:
      shard-kbl:          PASS -> SKIP +1

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_suspend@fence-restore-tiled2untiled-hibernate:
      shard-kbl:          NOTRUN -> FAIL (fdo#103375) +4
      shard-snb:          NOTRUN -> FAIL (fdo#103375) +4
      shard-hsw:          NOTRUN -> FAIL (fdo#103375) +4

    igt@drv_suspend@forcewake-hibernate:
      shard-glk:          NOTRUN -> FAIL (fdo#103375) +4

    igt@drv_suspend@sysfs-reader-hibernate:
      shard-apl:          NOTRUN -> FAIL (fdo#103375) +4

    {igt@gem_concurrent_blit@4kib-tiny-prw-blt-overwrite-source-read-bcs-child}:
      shard-hsw:          NOTRUN -> INCOMPLETE (fdo#103540)

    {igt@gem_concurrent_blit@4kib-tiny-prw-blt-overwrite-source-read-rcs}:
      shard-glk:          NOTRUN -> INCOMPLETE (k.org#198133, fdo#103359)

    {igt@gem_concurrent_blit@4kib-tiny-prw-blt-sanitycheck0}:
      shard-apl:          NOTRUN -> INCOMPLETE (fdo#103927)

    {igt@gem_concurrent_blit@4kib-tiny-prw-blt-sanitycheck0-child}:
      shard-snb:          NOTRUN -> INCOMPLETE (fdo#105411)

    {igt@gem_concurrent_blit@4kib-tiny-prw-blt-sanitycheck1}:
      shard-kbl:          NOTRUN -> INCOMPLETE (fdo#103665)

    igt@gem_linear_blits@interruptible:
      shard-apl:          PASS -> INCOMPLETE (fdo#103927)

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

    igt@kms_flip@blocking-wf_vblank:
      shard-hsw:          PASS -> FAIL (fdo#100368)

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

    igt@kms_flip@flip-vs-panning-vs-hang-interruptible:
      shard-snb:          PASS -> DMESG-WARN (fdo#103821)

    igt@kms_flip@flip-vs-wf_vblank-interruptible:
      shard-apl:          PASS -> FAIL (fdo#105312)

    igt@kms_flip@wf_vblank-ts-check-interruptible:
      shard-kbl:          PASS -> FAIL (fdo#100368)

    igt@kms_setmode@basic:
      shard-glk:          PASS -> FAIL (fdo#99912)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_contexts:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS

    igt@kms_chv_cursor_fail@pipe-c-256x256-right-edge:
      shard-apl:          FAIL (fdo#104671) -> PASS

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

    igt@kms_flip@absolute-wf_vblank-interruptible:
      shard-glk:          FAIL (fdo#106087) -> PASS

    igt@kms_flip@basic-flip-vs-wf_vblank:
      shard-hsw:          FAIL (fdo#103928) -> PASS

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

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

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
  fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103821 https://bugs.freedesktop.org/show_bug.cgi?id=103821
  fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#103928 https://bugs.freedesktop.org/show_bug.cgi?id=103928
  fdo#104671 https://bugs.freedesktop.org/show_bug.cgi?id=104671
  fdo#105312 https://bugs.freedesktop.org/show_bug.cgi?id=105312
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105767 https://bugs.freedesktop.org/show_bug.cgi?id=105767
  fdo#106087 https://bugs.freedesktop.org/show_bug.cgi?id=106087
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


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

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4131 -> Patchwork_8896

  CI_DRM_4131: 46d3a67e7a5611ef8af00cb7adebf03817856645 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4459: 1b8977e08031253d61b4641bc21e5c7a990d4a4f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8896: 037476edddf2d61b9a4d05f1d53eaf42faec6373 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4459: f74d92e704849610364b4474a2c67ea2008c14e0 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

Re: [PATCH 5/5] drm/selftests: Add drm helper selftest

[Maarten Lankhorst]

Op 03-05-18 om 15:36 schreef Ville Syrjälä:
> On Thu, May 03, 2018 at 01:22:17PM +0200, Maarten Lankhorst wrote:
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/Kconfig                       |   1 +
>>  drivers/gpu/drm/selftests/Makefile            |   2 +-
>>  .../gpu/drm/selftests/drm_helper_selftests.h  |   9 +
>>  drivers/gpu/drm/selftests/test-drm-helper.c   | 247 ++++++++++++++++++
>>  4 files changed, 258 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/gpu/drm/selftests/drm_helper_selftests.h
>>  create mode 100644 drivers/gpu/drm/selftests/test-drm-helper.c
>>
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index d684855b95c2..28d059007ac2 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -55,6 +55,7 @@ config DRM_DEBUG_SELFTEST
>>  	depends on DEBUG_KERNEL
>>  	select PRIME_NUMBERS
>>  	select DRM_LIB_RANDOM
>> +	select DRM_KMS_HELPER
>>  	default n
>>  	help
>>  	  This option provides kernel modules that can be used to run
>> diff --git a/drivers/gpu/drm/selftests/Makefile b/drivers/gpu/drm/selftests/Makefile
>> index f7dd66e859a9..9fc349fa18e9 100644
>> --- a/drivers/gpu/drm/selftests/Makefile
>> +++ b/drivers/gpu/drm/selftests/Makefile
>> @@ -1 +1 @@
>> -obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o
>> +obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm-helper.o
>> diff --git a/drivers/gpu/drm/selftests/drm_helper_selftests.h b/drivers/gpu/drm/selftests/drm_helper_selftests.h
>> new file mode 100644
>> index 000000000000..9771290ed228
>> --- /dev/null
>> +++ b/drivers/gpu/drm/selftests/drm_helper_selftests.h
>> @@ -0,0 +1,9 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* List each unit test as selftest(name, function)
>> + *
>> + * The name is used as both an enum and expanded as igt__name to create
>> + * a module parameter. It must be unique and legal for a C identifier.
>> + *
>> + * Tests are executed in order by igt/drm_selftests_helper
>> + */
>> +selftest(check_plane_state, igt_check_plane_state)
>> diff --git a/drivers/gpu/drm/selftests/test-drm-helper.c b/drivers/gpu/drm/selftests/test-drm-helper.c
>> new file mode 100644
>> index 000000000000..a015712b43e8
>> --- /dev/null
>> +++ b/drivers/gpu/drm/selftests/test-drm-helper.c
>> @@ -0,0 +1,247 @@
>> +/*
>> + * Test cases for the drm_kms_helper functions
>> + */
>> +
>> +#define pr_fmt(fmt) "drm_kms_helper: " fmt
>> +
>> +#include <linux/module.h>
>> +
>> +#include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_plane_helper.h>
>> +#include <drm/drm_modes.h>
>> +
>> +#define TESTS "drm_helper_selftests.h"
>> +#include "drm_selftest.h"
>> +
>> +#define FAIL(test, msg, ...) \
>> +	do { \
>> +		if (test) { \
>> +			pr_err("%s/%u: " msg, __FUNCTION__, __LINE__, ##__VA_ARGS__); \
>> +			return -EINVAL; \
>> +		} \
>> +	} while (0)
>> +
>> +#define FAIL_ON(x) FAIL((x), "%s", "FAIL_ON(" __stringify(x) ")\n")
>> +
>> +static void set_src(struct drm_plane_state *plane_state,
>> +		    unsigned src_x, unsigned src_y,
>> +		    unsigned src_w, unsigned src_h)
>> +{
>> +	plane_state->src_x = src_x;
>> +	plane_state->src_y = src_y;
>> +	plane_state->src_w = src_w;
>> +	plane_state->src_h = src_h;
>> +}
>> +
>> +static bool check_src_eq(struct drm_plane_state *plane_state,
>> +			 unsigned src_x, unsigned src_y,
>> +			 unsigned src_w, unsigned src_h)
>> +{
>> +	if (plane_state->src.x1 < 0) {
>> +		pr_err("src x coordinate %x should never be below 0.\n", plane_state->src.x1);
>> +		drm_rect_debug_print("src: ", &plane_state->src, true);
>> +		return false;
>> +	}
>> +	if (plane_state->src.y1 < 0) {
>> +		pr_err("src y coordinate %x should never be below 0.\n", plane_state->src.y1);
>> +		drm_rect_debug_print("src: ", &plane_state->src, true);
>> +		return false;
>> +	}
>> +
>> +	if (plane_state->src.x1 != src_x ||
>> +	    plane_state->src.y1 != src_y ||
>> +	    drm_rect_width(&plane_state->src) != src_w ||
>> +	    drm_rect_height(&plane_state->src) != src_h) {
>> +		drm_rect_debug_print("src: ", &plane_state->src, true);
>> +		return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>> +static void set_crtc(struct drm_plane_state *plane_state,
>> +		     int crtc_x, int crtc_y,
>> +		     unsigned crtc_w, unsigned crtc_h)
>> +{
>> +	plane_state->crtc_x = crtc_x;
>> +	plane_state->crtc_y = crtc_y;
>> +	plane_state->crtc_w = crtc_w;
>> +	plane_state->crtc_h = crtc_h;
>> +}
>> +
>> +static bool check_crtc_eq(struct drm_plane_state *plane_state,
>> +			  int crtc_x, int crtc_y,
>> +			  unsigned crtc_w, unsigned crtc_h)
>> +{
>> +	if (plane_state->dst.x1 != crtc_x ||
>> +	    plane_state->dst.y1 != crtc_y ||
>> +	    drm_rect_width(&plane_state->dst) != crtc_w ||
>> +	    drm_rect_height(&plane_state->dst) != crtc_h) {
>> +		drm_rect_debug_print("dst: ", &plane_state->dst, false);
>> +
>> +		return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>> +static int igt_check_plane_state(void *ignored)
>> +{
>> +	int ret;
>> +
>> +	const struct drm_crtc_state crtc_state = {
>> +		.crtc = ZERO_SIZE_PTR,
>> +		.enable = true,
>> +		.active = true,
>> +		.mode = {
>> +			DRM_MODE("1024x768", 0, 65000, 1024, 1048,
>> +				1184, 1344, 0, 768, 771, 777, 806, 0,
>> +				DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC)
>> +		},
>> +	};
>> +	struct drm_framebuffer fb = {
>> +		.width = 2048,
>> +		.height = 2048
>> +	};
>> +	struct drm_plane_state plane_state = {
>> +		.crtc = ZERO_SIZE_PTR,
>> +		.fb = &fb,
>> +		.rotation = DRM_MODE_ROTATE_0
>> +	};
>> +
>> +	/* Simple clipping, no scaling. */
>> +	set_src(&plane_state, 0, 0, fb.width << 16, fb.height << 16);
>> +	set_crtc(&plane_state, 0, 0, fb.width, fb.height);
>> +	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
>> +						  DRM_PLANE_HELPER_NO_SCALING,
>> +						  DRM_PLANE_HELPER_NO_SCALING,
>> +						  false, false);
>> +	FAIL(ret < 0, "Simple clipping check should pass\n");
>> +	FAIL_ON(!plane_state.visible);
>> +	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 1024 << 16, 768 << 16));
>> +	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
>> +
>> +	/* Rotated clipping + reflection, no scaling. */
>> +	plane_state.rotation = DRM_MODE_ROTATE_90 | DRM_MODE_REFLECT_X;
>> +	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
>> +						  DRM_PLANE_HELPER_NO_SCALING,
>> +						  DRM_PLANE_HELPER_NO_SCALING,
>> +						  false, false);
>> +	FAIL(ret < 0, "Rotated clipping check should pass\n");
>> +	FAIL_ON(!plane_state.visible);
>> +	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 768 << 16, 1024 << 16));
>> +	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
> I guess we might want a few more rotated/reflected cases to make
> sure the clipping affects each edge correctly.
>
>> +	plane_state.rotation = DRM_MODE_ROTATE_0;
>> +
>> +	/* Check whether positioning works correctly. */
>> +	set_src(&plane_state, 0, 0, 1023 << 16, 767 << 16);
>> +	set_crtc(&plane_state, 0, 0, 1023, 767);
>> +	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
>> +						  DRM_PLANE_HELPER_NO_SCALING,
>> +						  DRM_PLANE_HELPER_NO_SCALING,
>> +						  false, false);
>> +	FAIL(!ret, "Should not be able to position on the crtc with can_position=false\n");
>> +
>> +	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
>> +						  DRM_PLANE_HELPER_NO_SCALING,
>> +						  DRM_PLANE_HELPER_NO_SCALING,
>> +						  true, false);
>> +	FAIL(ret < 0, "Simple positioning should work\n");
>> +	FAIL_ON(!plane_state.visible);
>> +	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 1023 << 16, 767 << 16));
>> +	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1023, 767));
>> +
>> +	/* Simple scaling tests. */
>> +	set_src(&plane_state, 0, 0, 512 << 16, 384 << 16);
>> +	set_crtc(&plane_state, 0, 0, 1024, 768);
>> +	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
>> +						  0x8001,
>> +						  DRM_PLANE_HELPER_NO_SCALING,
>> +						  false, false);
>> +	FAIL(!ret, "Upscaling out of range should fail.\n");
>> +	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
>> +						  0x8000,
>> +						  DRM_PLANE_HELPER_NO_SCALING,
>> +						  false, false);
>> +	FAIL(ret < 0, "Upscaling exactly 2x should work\n");
>> +	FAIL_ON(!plane_state.visible);
>> +	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 512 << 16, 384 << 16));
>> +	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
>> +
>> +	set_src(&plane_state, 0, 0, 2048 << 16, 1536 << 16);
>> +	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
>> +						  DRM_PLANE_HELPER_NO_SCALING,
>> +						  0x1ffff, false, false);
>> +	FAIL(!ret, "Downscaling out of range should fail.\n");
>> +	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
>> +						  DRM_PLANE_HELPER_NO_SCALING,
>> +						  0x20000, false, false);
>> +	FAIL(ret < 0, "Should succeed with exact scaling limit\n");
> "Downscaling exactly 2x should work\n"
> to be consistent with the error from the previous test?
>
>> +	FAIL_ON(!plane_state.visible);
>> +	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 2048 << 16, 1536 << 16));
>> +	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
>> +
>> +	/* Testing rounding errors. */
>> +	set_src(&plane_state, 0, 0, 0x40001, 0x40001);
>> +	set_crtc(&plane_state, 1022, 766, 4, 4);
>> +	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
>> +						  DRM_PLANE_HELPER_NO_SCALING,
>> +						  0x10001,
>> +						  true, false);
>> +	FAIL(ret < 0, "Should succeed by clipping to exact multiple");
> What does "exact multiple" mean for these? src and dst are not integer
> multiples of each other in these tests so not sure what this is trying
> to say.
>
>> +	FAIL_ON(!plane_state.visible);
>> +	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 2 << 16, 2 << 16));
>> +	FAIL_ON(!check_crtc_eq(&plane_state, 1022, 766, 2, 2));
>> +
>> +	set_src(&plane_state, 0x20001, 0x20001, 0x4040001, 0x3040001);
>> +	set_crtc(&plane_state, -2, -2, 1028, 772);
>> +	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
>> +						  DRM_PLANE_HELPER_NO_SCALING,
>> +						  0x10001,
>> +						  false, false);
>> +	FAIL(ret < 0, "Should succeed by clipping to exact multiple");
>> +	FAIL_ON(!plane_state.visible);
>> +	FAIL_ON(!check_src_eq(&plane_state, 0x40002, 0x40002, 1024 << 16, 768 << 16));
>> +	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
>> +
>> +	set_src(&plane_state, 0, 0, 0x3ffff, 0x3ffff);
>> +	set_crtc(&plane_state, 1022, 766, 4, 4);
>> +	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
>> +						  0xffff,
>> +						  DRM_PLANE_HELPER_NO_SCALING,
>> +						  true, false);
>> +	FAIL(ret < 0, "Should succeed by clipping to exact multiple");
>> +	FAIL_ON(!plane_state.visible);
>> +	/* Should not be rounded to 0x20001, which would be upscaling. */
> "downscaling". src<dst so the "user" asked for upscaling. The other
> tests don't have any comments though. Not sure why this one is
> special.
>
>> +	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 2 << 16, 2 << 16));
>> +	FAIL_ON(!check_crtc_eq(&plane_state, 1022, 766, 2, 2));
>> +
>> +	set_src(&plane_state, 0x1ffff, 0x1ffff, 0x403ffff, 0x303ffff);
>> +	set_crtc(&plane_state, -2, -2, 1028, 772);
>> +	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
>> +						  0xffff,
>> +						  DRM_PLANE_HELPER_NO_SCALING,
>> +						  false, false);
>> +	FAIL(ret < 0, "Should succeed by clipping to exact multiple");
>> +	FAIL_ON(!plane_state.visible);
>> +	FAIL_ON(!check_src_eq(&plane_state, 0x3fffe, 0x3fffe, 1024 << 16, 768 << 16));
>> +	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
> These are all very specific to the rounding behaviour you implemented,
> but I guess that's what we want.
>
> I guess we migth also want some tests for the fully clipped cases? Could
> be added later though.
>
> In general these look correct enough to me.
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>> +
>> +	return 0;
>> +}
>> +
>> +#include "drm_selftest.c"
>> +
>> +static int __init test_drm_helper_init(void)
>> +{
>> +	int err;
>> +
>> +	err = run_selftests(selftests, ARRAY_SIZE(selftests), NULL);
>> +
>> +	return err > 0 ? 0 : err;
>> +}
>> +
>> +module_init(test_drm_helper_init);
>> +
>> +MODULE_AUTHOR("Intel Corporation");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.17.0

Thanks, pushed. :)

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