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

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

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

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

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