[Intel-gfx] [PATCH v3 1/2] drm/i915/mtl: limit second scaler vertical scaling in ver >= 14

[Intel-gfx] [PATCH v3 1/2] drm/i915/mtl: limit second scaler vertical scaling in ver >= 14

[Luca Coelho]

In newer hardware versions (i.e. display version >= 14), the second
scaler doesn't support vertical scaling.

The current implementation of the scaling limits is simplified and
only occurs when the planes are created, so we don't know which scaler
is being used.

In order to handle separate scaling limits for horizontal and vertical
scaling, and different limits per scaler, split the checks in two
phases.  We first do a simple check during plane creation and use the
best-case scenario (because we don't know the scaler that may be used
at a later point) and then do a more specific check when the scalers
are actually being set up.

Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---

In v2:
   * fix DRM_PLANE_NO_SCALING renamed macros;

In v3:
   * No changes.

drivers/gpu/drm/i915/display/i9xx_plane.c     |  4 +-
 drivers/gpu/drm/i915/display/intel_atomic.c   | 47 +++++++++++++++++++
 .../gpu/drm/i915/display/intel_atomic_plane.c | 39 +++++++++++++--
 .../gpu/drm/i915/display/intel_atomic_plane.h |  2 +-
 drivers/gpu/drm/i915/display/intel_cursor.c   |  4 +-
 drivers/gpu/drm/i915/display/intel_sprite.c   | 19 ++------
 .../drm/i915/display/skl_universal_plane.c    | 26 ++--------
 7 files changed, 91 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/i9xx_plane.c b/drivers/gpu/drm/i915/display/i9xx_plane.c
index ecaeb7dc196b..390e96f0692b 100644
--- a/drivers/gpu/drm/i915/display/i9xx_plane.c
+++ b/drivers/gpu/drm/i915/display/i9xx_plane.c
@@ -326,9 +326,7 @@ i9xx_plane_check(struct intel_crtc_state *crtc_state,
 	if (ret)
 		return ret;
 
-	ret = intel_atomic_plane_check_clipping(plane_state, crtc_state,
-						DRM_PLANE_NO_SCALING,
-						DRM_PLANE_NO_SCALING,
+	ret = intel_atomic_plane_check_clipping(plane_state, crtc_state, false,
 						i9xx_plane_has_windowing(plane));
 	if (ret)
 		return ret;
diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
index 6621aa245caf..43b1c7a227f8 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic.c
@@ -38,6 +38,7 @@
 #include "intel_atomic.h"
 #include "intel_cdclk.h"
 #include "intel_display_types.h"
+#include "intel_fb.h"
 #include "intel_global_state.h"
 #include "intel_hdcp.h"
 #include "intel_psr.h"
@@ -375,6 +376,52 @@ static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_sta
 		mode = SKL_PS_SCALER_MODE_DYN;
 	}
 
+	if (plane_state && plane_state->hw.fb) {
+		const struct drm_framebuffer *fb = plane_state->hw.fb;
+		struct drm_rect *src = &plane_state->uapi.src;
+		struct drm_rect *dst = &plane_state->uapi.dst;
+		int hscale, vscale, max_vscale, max_hscale;
+
+		if (DISPLAY_VER(dev_priv) >= 14) {
+			/*
+			 * On versions 14 and up, only the first
+			 * scaler supports a vertical scaling factor
+			 * of more than 1.0, while a horizontal
+			 * scaling factor of 3.0 is supported.
+			 */
+			max_hscale = 0x30000 - 1;
+			if (*scaler_id == 0)
+				max_vscale = 0x30000 - 1;
+			else
+				max_vscale = 0x10000;
+
+		} else if (DISPLAY_VER(dev_priv) >= 10 ||
+			   !intel_format_info_is_yuv_semiplanar(fb->format, fb->modifier)) {
+			max_hscale = 0x30000 - 1;
+			max_vscale = 0x30000 - 1;
+		} else {
+			max_hscale = 0x20000 - 1;
+			max_vscale = 0x20000 - 1;
+		}
+
+		/* Check if required scaling is within limits */
+		hscale = drm_rect_calc_hscale(src, dst, 1, max_hscale);
+		vscale = drm_rect_calc_vscale(src, dst, 1, max_vscale);
+
+		if (hscale < 0 || vscale < 0) {
+			drm_dbg_kms(&dev_priv->drm,
+				    "Scaler %d doesn't support required plane scaling\n",
+				    *scaler_id);
+			drm_rect_debug_print("src: ", src, true);
+			drm_rect_debug_print("dst: ", dst, false);
+
+			scaler_state->scalers[*scaler_id].in_use = 0;
+			*scaler_id = -1;
+
+			return;
+		}
+	}
+
 	drm_dbg_kms(&dev_priv->drm, "Attached scaler id %u.%u to %s:%d\n",
 		    intel_crtc->pipe, *scaler_id, name, idx);
 	scaler_state->scalers[*scaler_id].mode = mode;
diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
index 10e1fc9d0698..9100f328df60 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
@@ -887,7 +887,7 @@ void intel_crtc_planes_update_arm(struct intel_atomic_state *state,
 
 int intel_atomic_plane_check_clipping(struct intel_plane_state *plane_state,
 				      struct intel_crtc_state *crtc_state,
-				      int min_scale, int max_scale,
+				      bool allow_scaling,
 				      bool can_position)
 {
 	struct drm_i915_private *i915 = to_i915(plane_state->uapi.plane->dev);
@@ -897,19 +897,50 @@ int intel_atomic_plane_check_clipping(struct intel_plane_state *plane_state,
 	const struct drm_rect *clip = &crtc_state->pipe_src;
 	unsigned int rotation = plane_state->hw.rotation;
 	int hscale, vscale;
+	int max_hscale, min_hscale, max_vscale, min_vscale;
 
 	if (!fb) {
 		plane_state->uapi.visible = false;
 		return 0;
 	}
 
+	/*
+	 * At this point we don't really know the HW limitations, so
+	 * we just sanitize the values against the maximum supported
+	 * scaling.
+	 */
+	if (allow_scaling) {
+		min_vscale = 1;
+		min_hscale = 1;
+
+		if (DISPLAY_VER(i915) < 10 ||
+		    intel_format_info_is_yuv_semiplanar(fb->format,
+							fb->modifier)) {
+			max_vscale = 0x20000 - 1;
+			max_hscale = 0x20000 - 1;
+		} else {
+			max_vscale = 0x30000 - 1;
+			max_hscale = 0x30000 - 1;
+		}
+	} else {
+		min_hscale = DRM_PLANE_NO_SCALING;
+		max_hscale = DRM_PLANE_NO_SCALING;
+		min_vscale = DRM_PLANE_NO_SCALING;
+		max_vscale = DRM_PLANE_NO_SCALING;
+	}
+
 	drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation);
 
 	/* Check scaling */
-	hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
-	vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
+	hscale = drm_rect_calc_hscale(src, dst, min_hscale, max_hscale);
+	vscale = drm_rect_calc_vscale(src, dst, min_vscale, max_vscale);
 	if (hscale < 0 || vscale < 0) {
-		drm_dbg_kms(&i915->drm, "Invalid scaling of plane\n");
+		drm_dbg_kms(&i915->drm,
+			    "Invalid scaling of plane: hscale 0x%x vscale 0x%x\n",
+			    hscale, vscale);
+		drm_dbg_kms(&i915->drm,
+			    "min_hscale 0x%0x max_hscale 0x%0x min_vscale 0x%0x max_vscale 0x%0x\n",
+			    min_hscale, max_hscale, min_vscale, max_vscale);
 		drm_rect_debug_print("src: ", src, true);
 		drm_rect_debug_print("dst: ", dst, false);
 		return -ERANGE;
diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
index 74b6d3b169a7..441ef8165212 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic_plane.h
+++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
@@ -60,7 +60,7 @@ int intel_plane_calc_min_cdclk(struct intel_atomic_state *state,
 			       bool *need_cdclk_calc);
 int intel_atomic_plane_check_clipping(struct intel_plane_state *plane_state,
 				      struct intel_crtc_state *crtc_state,
-				      int min_scale, int max_scale,
+				      bool check_scaling,
 				      bool can_position);
 void intel_plane_set_invisible(struct intel_crtc_state *crtc_state,
 			       struct intel_plane_state *plane_state);
diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
index d190fa0d393b..741ec74f54f6 100644
--- a/drivers/gpu/drm/i915/display/intel_cursor.c
+++ b/drivers/gpu/drm/i915/display/intel_cursor.c
@@ -144,9 +144,7 @@ static int intel_check_cursor(struct intel_crtc_state *crtc_state,
 	}
 
 	ret = intel_atomic_plane_check_clipping(plane_state, crtc_state,
-						DRM_PLANE_NO_SCALING,
-						DRM_PLANE_NO_SCALING,
-						true);
+						false, true);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
index e6b4d24b9cd0..9ad1173a0551 100644
--- a/drivers/gpu/drm/i915/display/intel_sprite.c
+++ b/drivers/gpu/drm/i915/display/intel_sprite.c
@@ -1355,22 +1355,11 @@ g4x_sprite_check(struct intel_crtc_state *crtc_state,
 {
 	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
-	int min_scale = DRM_PLANE_NO_SCALING;
-	int max_scale = DRM_PLANE_NO_SCALING;
 	int ret;
 
-	if (g4x_fb_scalable(plane_state->hw.fb)) {
-		if (DISPLAY_VER(dev_priv) < 7) {
-			min_scale = 1;
-			max_scale = 16 << 16;
-		} else if (IS_IVYBRIDGE(dev_priv)) {
-			min_scale = 1;
-			max_scale = 2 << 16;
-		}
-	}
-
 	ret = intel_atomic_plane_check_clipping(plane_state, crtc_state,
-						min_scale, max_scale, true);
+						g4x_fb_scalable(plane_state->hw.fb),
+						true);
 	if (ret)
 		return ret;
 
@@ -1426,9 +1415,7 @@ vlv_sprite_check(struct intel_crtc_state *crtc_state,
 		return ret;
 
 	ret = intel_atomic_plane_check_clipping(plane_state, crtc_state,
-						DRM_PLANE_NO_SCALING,
-						DRM_PLANE_NO_SCALING,
-						true);
+						false, true);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
index 76490cc59d8f..e2ae6624378f 100644
--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
@@ -1463,22 +1463,6 @@ static int skl_plane_check_nv12_rotation(const struct intel_plane_state *plane_s
 	return 0;
 }
 
-static int skl_plane_max_scale(struct drm_i915_private *dev_priv,
-			       const struct drm_framebuffer *fb)
-{
-	/*
-	 * We don't yet know the final source width nor
-	 * whether we can use the HQ scaler mode. Assume
-	 * the best case.
-	 * FIXME need to properly check this later.
-	 */
-	if (DISPLAY_VER(dev_priv) >= 10 ||
-	    !intel_format_info_is_yuv_semiplanar(fb->format, fb->modifier))
-		return 0x30000 - 1;
-	else
-		return 0x20000 - 1;
-}
-
 static int intel_plane_min_width(struct intel_plane *plane,
 				 const struct drm_framebuffer *fb,
 				 int color_plane,
@@ -1862,8 +1846,7 @@ static int skl_plane_check(struct intel_crtc_state *crtc_state,
 	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
 	const struct drm_framebuffer *fb = plane_state->hw.fb;
-	int min_scale = DRM_PLANE_NO_SCALING;
-	int max_scale = DRM_PLANE_NO_SCALING;
+	bool allow_scaling;
 	int ret;
 
 	ret = skl_plane_check_fb(crtc_state, plane_state);
@@ -1871,13 +1854,10 @@ static int skl_plane_check(struct intel_crtc_state *crtc_state,
 		return ret;
 
 	/* use scaler when colorkey is not required */
-	if (!plane_state->ckey.flags && skl_fb_scalable(fb)) {
-		min_scale = 1;
-		max_scale = skl_plane_max_scale(dev_priv, fb);
-	}
+	allow_scaling = !plane_state->ckey.flags && skl_fb_scalable(fb);
 
 	ret = intel_atomic_plane_check_clipping(plane_state, crtc_state,
-						min_scale, max_scale, true);
+						allow_scaling, true);
 	if (ret)
 		return ret;
 
-- 
2.38.1

[Intel-gfx] [PATCH v3 2/2] drm/i915/mtl: Limit scaler input to 4k in plane scaling

[Luca Coelho]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 2528 bytes --]

From: Animesh Manna <animesh.manna@intel.com>

As part of die area reduction max input source modified to 4096
for MTL so modified range check logic of scaler.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---

In v2:
   * No changes;

In v3:
   * Removed stray reviewed-by tag;
   * Added my s-o-b.

drivers/gpu/drm/i915/display/skl_scaler.c | 31 +++++++++++++++++------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/skl_scaler.c b/drivers/gpu/drm/i915/display/skl_scaler.c
index d7390067b7d4..6baa07142b03 100644
--- a/drivers/gpu/drm/i915/display/skl_scaler.c
+++ b/drivers/gpu/drm/i915/display/skl_scaler.c
@@ -103,6 +103,8 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	const struct drm_display_mode *adjusted_mode =
 		&crtc_state->hw.adjusted_mode;
+	int min_src_w, min_src_h, min_dst_w, min_dst_h;
+	int max_src_w, max_src_h, max_dst_w, max_dst_h;
 
 	/*
 	 * Src coordinates are already rotated by 270 degrees for
@@ -157,15 +159,28 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
 		return -EINVAL;
 	}
 
+	min_src_w = SKL_MIN_SRC_W;
+	min_src_h = SKL_MIN_SRC_H;
+	min_dst_w = SKL_MIN_DST_W;
+	min_dst_h = SKL_MIN_DST_H;
+
+	if (DISPLAY_VER(dev_priv) >= 11 && DISPLAY_VER(dev_priv) < 14) {
+		max_src_w = ICL_MAX_SRC_W;
+		max_src_h = ICL_MAX_SRC_H;
+		max_dst_w = ICL_MAX_DST_W;
+		max_dst_h = ICL_MAX_DST_H;
+	} else {
+		max_src_w = SKL_MAX_SRC_W;
+		max_src_h = SKL_MAX_SRC_H;
+		max_dst_w = SKL_MAX_DST_W;
+		max_dst_h = SKL_MAX_DST_H;
+	}
+
 	/* range checks */
-	if (src_w < SKL_MIN_SRC_W || src_h < SKL_MIN_SRC_H ||
-	    dst_w < SKL_MIN_DST_W || dst_h < SKL_MIN_DST_H ||
-	    (DISPLAY_VER(dev_priv) >= 11 &&
-	     (src_w > ICL_MAX_SRC_W || src_h > ICL_MAX_SRC_H ||
-	      dst_w > ICL_MAX_DST_W || dst_h > ICL_MAX_DST_H)) ||
-	    (DISPLAY_VER(dev_priv) < 11 &&
-	     (src_w > SKL_MAX_SRC_W || src_h > SKL_MAX_SRC_H ||
-	      dst_w > SKL_MAX_DST_W || dst_h > SKL_MAX_DST_H)))	{
+	if (src_w < min_src_w || src_h < min_src_h ||
+	    dst_w < min_dst_w || dst_h < min_dst_h ||
+	    src_w > max_src_w || src_h > max_src_h ||
+	    dst_w > max_dst_w || dst_h > max_dst_h) {
 		drm_dbg_kms(&dev_priv->drm,
 			    "scaler_user index %u.%u: src %ux%u dst %ux%u "
 			    "size is out of scaler range\n",
-- 
2.38.1

Re: [Intel-gfx] [PATCH v3 1/2] drm/i915/mtl: limit second scaler vertical scaling in ver >= 14

[Ville Syrjälä]

On Tue, Nov 22, 2022 at 12:23:43PM +0200, Luca Coelho wrote:
> In newer hardware versions (i.e. display version >= 14), the second
> scaler doesn't support vertical scaling.
> 
> The current implementation of the scaling limits is simplified and
> only occurs when the planes are created, so we don't know which scaler
> is being used.
> 
> In order to handle separate scaling limits for horizontal and vertical
> scaling, and different limits per scaler, split the checks in two
> phases.  We first do a simple check during plane creation and use the
> best-case scenario (because we don't know the scaler that may be used
> at a later point) and then do a more specific check when the scalers
> are actually being set up.
> 
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> ---
> 
> In v2:
>    * fix DRM_PLANE_NO_SCALING renamed macros;
> 
> In v3:
>    * No changes.
> 
> drivers/gpu/drm/i915/display/i9xx_plane.c     |  4 +-
>  drivers/gpu/drm/i915/display/intel_atomic.c   | 47 +++++++++++++++++++
>  .../gpu/drm/i915/display/intel_atomic_plane.c | 39 +++++++++++++--
>  .../gpu/drm/i915/display/intel_atomic_plane.h |  2 +-
>  drivers/gpu/drm/i915/display/intel_cursor.c   |  4 +-
>  drivers/gpu/drm/i915/display/intel_sprite.c   | 19 ++------
>  .../drm/i915/display/skl_universal_plane.c    | 26 ++--------
>  7 files changed, 91 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/i9xx_plane.c b/drivers/gpu/drm/i915/display/i9xx_plane.c
> index ecaeb7dc196b..390e96f0692b 100644
> --- a/drivers/gpu/drm/i915/display/i9xx_plane.c
> +++ b/drivers/gpu/drm/i915/display/i9xx_plane.c
> @@ -326,9 +326,7 @@ i9xx_plane_check(struct intel_crtc_state *crtc_state,
>  	if (ret)
>  		return ret;
>  
> -	ret = intel_atomic_plane_check_clipping(plane_state, crtc_state,
> -						DRM_PLANE_NO_SCALING,
> -						DRM_PLANE_NO_SCALING,
> +	ret = intel_atomic_plane_check_clipping(plane_state, crtc_state, false,
>  						i9xx_plane_has_windowing(plane));
>  	if (ret)
>  		return ret;
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> index 6621aa245caf..43b1c7a227f8 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> @@ -38,6 +38,7 @@
>  #include "intel_atomic.h"
>  #include "intel_cdclk.h"
>  #include "intel_display_types.h"
> +#include "intel_fb.h"
>  #include "intel_global_state.h"
>  #include "intel_hdcp.h"
>  #include "intel_psr.h"
> @@ -375,6 +376,52 @@ static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_sta
>  		mode = SKL_PS_SCALER_MODE_DYN;
>  	}
>  
> +	if (plane_state && plane_state->hw.fb) {
> +		const struct drm_framebuffer *fb = plane_state->hw.fb;
> +		struct drm_rect *src = &plane_state->uapi.src;
> +		struct drm_rect *dst = &plane_state->uapi.dst;

We want the scale factor checks for the pfit use case too. So this
stuff shouldn't be so tied into planes. I guess we could go with
a FIXME initially.

> +		int hscale, vscale, max_vscale, max_hscale;
> +
> +		if (DISPLAY_VER(dev_priv) >= 14) {
> +			/*
> +			 * On versions 14 and up, only the first
> +			 * scaler supports a vertical scaling factor
> +			 * of more than 1.0, while a horizontal
> +			 * scaling factor of 3.0 is supported.
> +			 */
> +			max_hscale = 0x30000 - 1;
> +			if (*scaler_id == 0)
> +				max_vscale = 0x30000 - 1;
> +			else
> +				max_vscale = 0x10000;

We still have the chicken vs. egg problem here that we'd need to
consider the scale factors already when selecting the scaler.
But that could be another FIXME.

> +
> +		} else if (DISPLAY_VER(dev_priv) >= 10 ||
> +			   !intel_format_info_is_yuv_semiplanar(fb->format, fb->modifier)) {
> +			max_hscale = 0x30000 - 1;
> +			max_vscale = 0x30000 - 1;
> +		} else {
> +			max_hscale = 0x20000 - 1;
> +			max_vscale = 0x20000 - 1;
> +		}

Pre-glk hq scaler case not handled.

> +
> +		/* Check if required scaling is within limits */
> +		hscale = drm_rect_calc_hscale(src, dst, 1, max_hscale);
> +		vscale = drm_rect_calc_vscale(src, dst, 1, max_vscale);
> +
> +		if (hscale < 0 || vscale < 0) {
> +			drm_dbg_kms(&dev_priv->drm,
> +				    "Scaler %d doesn't support required plane scaling\n",
> +				    *scaler_id);
> +			drm_rect_debug_print("src: ", src, true);
> +			drm_rect_debug_print("dst: ", dst, false);
> +
> +			scaler_state->scalers[*scaler_id].in_use = 0;
> +			*scaler_id = -1;
> +
> +			return;

This would have to return an error rather than pretending that
everything is fine.

> +		}
> +	}
> +
>  	drm_dbg_kms(&dev_priv->drm, "Attached scaler id %u.%u to %s:%d\n",
>  		    intel_crtc->pipe, *scaler_id, name, idx);
>  	scaler_state->scalers[*scaler_id].mode = mode;
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> index 10e1fc9d0698..9100f328df60 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> @@ -887,7 +887,7 @@ void intel_crtc_planes_update_arm(struct intel_atomic_state *state,
>  
>  int intel_atomic_plane_check_clipping(struct intel_plane_state *plane_state,
>  				      struct intel_crtc_state *crtc_state,
> -				      int min_scale, int max_scale,
> +				      bool allow_scaling,
>  				      bool can_position)
>  {
>  	struct drm_i915_private *i915 = to_i915(plane_state->uapi.plane->dev);
> @@ -897,19 +897,50 @@ int intel_atomic_plane_check_clipping(struct intel_plane_state *plane_state,
>  	const struct drm_rect *clip = &crtc_state->pipe_src;
>  	unsigned int rotation = plane_state->hw.rotation;
>  	int hscale, vscale;
> +	int max_hscale, min_hscale, max_vscale, min_vscale;
>  
>  	if (!fb) {
>  		plane_state->uapi.visible = false;
>  		return 0;
>  	}
>  
> +	/*
> +	 * At this point we don't really know the HW limitations, so
> +	 * we just sanitize the values against the maximum supported
> +	 * scaling.
> +	 */
> +	if (allow_scaling) {
> +		min_vscale = 1;
> +		min_hscale = 1;
> +
> +		if (DISPLAY_VER(i915) < 10 ||
> +		    intel_format_info_is_yuv_semiplanar(fb->format,
> +							fb->modifier)) {
> +			max_vscale = 0x20000 - 1;
> +			max_hscale = 0x20000 - 1;
> +		} else {
> +			max_vscale = 0x30000 - 1;
> +			max_hscale = 0x30000 - 1;
> +		}
> +	} else {
> +		min_hscale = DRM_PLANE_NO_SCALING;
> +		max_hscale = DRM_PLANE_NO_SCALING;
> +		min_vscale = DRM_PLANE_NO_SCALING;
> +		max_vscale = DRM_PLANE_NO_SCALING;
> +	}

I still don't see the point in moving this hw specific knowledge
from the more hw specific files into the hw agnostic file.

> +
>  	drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation);
>  
>  	/* Check scaling */
> -	hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
> -	vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
> +	hscale = drm_rect_calc_hscale(src, dst, min_hscale, max_hscale);
> +	vscale = drm_rect_calc_vscale(src, dst, min_vscale, max_vscale);
>  	if (hscale < 0 || vscale < 0) {
> -		drm_dbg_kms(&i915->drm, "Invalid scaling of plane\n");
> +		drm_dbg_kms(&i915->drm,
> +			    "Invalid scaling of plane: hscale 0x%x vscale 0x%x\n",
> +			    hscale, vscale);
> +		drm_dbg_kms(&i915->drm,
> +			    "min_hscale 0x%0x max_hscale 0x%0x min_vscale 0x%0x max_vscale 0x%0x\n",
> +			    min_hscale, max_hscale, min_vscale, max_vscale);
>  		drm_rect_debug_print("src: ", src, true);
>  		drm_rect_debug_print("dst: ", dst, false);
>  		return -ERANGE;
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> index 74b6d3b169a7..441ef8165212 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> @@ -60,7 +60,7 @@ int intel_plane_calc_min_cdclk(struct intel_atomic_state *state,
>  			       bool *need_cdclk_calc);
>  int intel_atomic_plane_check_clipping(struct intel_plane_state *plane_state,
>  				      struct intel_crtc_state *crtc_state,
> -				      int min_scale, int max_scale,
> +				      bool check_scaling,
>  				      bool can_position);
>  void intel_plane_set_invisible(struct intel_crtc_state *crtc_state,
>  			       struct intel_plane_state *plane_state);
> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
> index d190fa0d393b..741ec74f54f6 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> @@ -144,9 +144,7 @@ static int intel_check_cursor(struct intel_crtc_state *crtc_state,
>  	}
>  
>  	ret = intel_atomic_plane_check_clipping(plane_state, crtc_state,
> -						DRM_PLANE_NO_SCALING,
> -						DRM_PLANE_NO_SCALING,
> -						true);
> +						false, true);
>  	if (ret)
>  		return ret;
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
> index e6b4d24b9cd0..9ad1173a0551 100644
> --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> @@ -1355,22 +1355,11 @@ g4x_sprite_check(struct intel_crtc_state *crtc_state,
>  {
>  	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
>  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> -	int min_scale = DRM_PLANE_NO_SCALING;
> -	int max_scale = DRM_PLANE_NO_SCALING;
>  	int ret;
>  
> -	if (g4x_fb_scalable(plane_state->hw.fb)) {
> -		if (DISPLAY_VER(dev_priv) < 7) {
> -			min_scale = 1;
> -			max_scale = 16 << 16;
> -		} else if (IS_IVYBRIDGE(dev_priv)) {
> -			min_scale = 1;
> -			max_scale = 2 << 16;
> -		}
> -	}

So what happened to these limits?

> -
>  	ret = intel_atomic_plane_check_clipping(plane_state, crtc_state,
> -						min_scale, max_scale, true);
> +						g4x_fb_scalable(plane_state->hw.fb),
> +						true);
>  	if (ret)
>  		return ret;
>  
> @@ -1426,9 +1415,7 @@ vlv_sprite_check(struct intel_crtc_state *crtc_state,
>  		return ret;
>  
>  	ret = intel_atomic_plane_check_clipping(plane_state, crtc_state,
> -						DRM_PLANE_NO_SCALING,
> -						DRM_PLANE_NO_SCALING,
> -						true);
> +						false, true);
>  	if (ret)
>  		return ret;
>  
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 76490cc59d8f..e2ae6624378f 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -1463,22 +1463,6 @@ static int skl_plane_check_nv12_rotation(const struct intel_plane_state *plane_s
>  	return 0;
>  }
>  
> -static int skl_plane_max_scale(struct drm_i915_private *dev_priv,
> -			       const struct drm_framebuffer *fb)
> -{
> -	/*
> -	 * We don't yet know the final source width nor
> -	 * whether we can use the HQ scaler mode. Assume
> -	 * the best case.
> -	 * FIXME need to properly check this later.
> -	 */

Doesn't look like that FIXME has been dealt with as far
as the hq scaler is concerned.

> -	if (DISPLAY_VER(dev_priv) >= 10 ||
> -	    !intel_format_info_is_yuv_semiplanar(fb->format, fb->modifier))
> -		return 0x30000 - 1;
> -	else
> -		return 0x20000 - 1;
> -}
> -
>  static int intel_plane_min_width(struct intel_plane *plane,
>  				 const struct drm_framebuffer *fb,
>  				 int color_plane,
> @@ -1862,8 +1846,7 @@ static int skl_plane_check(struct intel_crtc_state *crtc_state,
>  	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
>  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>  	const struct drm_framebuffer *fb = plane_state->hw.fb;
> -	int min_scale = DRM_PLANE_NO_SCALING;
> -	int max_scale = DRM_PLANE_NO_SCALING;
> +	bool allow_scaling;
>  	int ret;
>  
>  	ret = skl_plane_check_fb(crtc_state, plane_state);
> @@ -1871,13 +1854,10 @@ static int skl_plane_check(struct intel_crtc_state *crtc_state,
>  		return ret;
>  
>  	/* use scaler when colorkey is not required */
> -	if (!plane_state->ckey.flags && skl_fb_scalable(fb)) {
> -		min_scale = 1;
> -		max_scale = skl_plane_max_scale(dev_priv, fb);
> -	}
> +	allow_scaling = !plane_state->ckey.flags && skl_fb_scalable(fb);
>  
>  	ret = intel_atomic_plane_check_clipping(plane_state, crtc_state,
> -						min_scale, max_scale, true);
> +						allow_scaling, true);
>  	if (ret)
>  		return ret;
>  
> -- 
> 2.38.1

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH v3 1/2] drm/i915/mtl: limit second scaler vertical scaling in ver >= 14

[Coelho, Luciano]

On Wed, 2022-11-23 at 08:47 +0200, Ville Syrjälä wrote:
> On Tue, Nov 22, 2022 at 12:23:43PM +0200, Luca Coelho wrote:
> > In newer hardware versions (i.e. display version >= 14), the second
> > scaler doesn't support vertical scaling.
> > 
> > The current implementation of the scaling limits is simplified and
> > only occurs when the planes are created, so we don't know which scaler
> > is being used.
> > 
> > In order to handle separate scaling limits for horizontal and vertical
> > scaling, and different limits per scaler, split the checks in two
> > phases.  We first do a simple check during plane creation and use the
> > best-case scenario (because we don't know the scaler that may be used
> > at a later point) and then do a more specific check when the scalers
> > are actually being set up.
> > 
> > Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> > ---


[...]
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> > index 6621aa245caf..43b1c7a227f8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > @@ -38,6 +38,7 @@
> >  #include "intel_atomic.h"
> >  #include "intel_cdclk.h"
> >  #include "intel_display_types.h"
> > +#include "intel_fb.h"
> >  #include "intel_global_state.h"
> >  #include "intel_hdcp.h"
> >  #include "intel_psr.h"
> > @@ -375,6 +376,52 @@ static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_sta
> >  		mode = SKL_PS_SCALER_MODE_DYN;
> >  	}
> >  
> > +	if (plane_state && plane_state->hw.fb) {
> > +		const struct drm_framebuffer *fb = plane_state->hw.fb;
> > +		struct drm_rect *src = &plane_state->uapi.src;
> > +		struct drm_rect *dst = &plane_state->uapi.dst;
> 
> We want the scale factor checks for the pfit use case too. So this
> stuff shouldn't be so tied into planes. I guess we could go with
> a FIXME initially.

Okay, I'll add a FIXME.  It was tied to the plane checks before,
though, wasn't it? So nothing should have changed in that regard here.


> > +		int hscale, vscale, max_vscale, max_hscale;
> > +
> > +		if (DISPLAY_VER(dev_priv) >= 14) {
> > +			/*
> > +			 * On versions 14 and up, only the first
> > +			 * scaler supports a vertical scaling factor
> > +			 * of more than 1.0, while a horizontal
> > +			 * scaling factor of 3.0 is supported.
> > +			 */
> > +			max_hscale = 0x30000 - 1;
> > +			if (*scaler_id == 0)
> > +				max_vscale = 0x30000 - 1;
> > +			else
> > +				max_vscale = 0x10000;
> 
> We still have the chicken vs. egg problem here that we'd need to
> consider the scale factors already when selecting the scaler.
> But that could be another FIXME.

Do you mean in regards to the HQ vs. non-HQ needs?


> > +
> > +		} else if (DISPLAY_VER(dev_priv) >= 10 ||
> > +			   !intel_format_info_is_yuv_semiplanar(fb->format, fb->modifier)) {
> > +			max_hscale = 0x30000 - 1;
> > +			max_vscale = 0x30000 - 1;
> > +		} else {
> > +			max_hscale = 0x20000 - 1;
> > +			max_vscale = 0x20000 - 1;
> > +		}
> 
> Pre-glk hq scaler case not handled.

I don't recall seen this specifically checked before.  Is this the
stuff I missed from g4x_sprite_check() below? Or am I missing something
else?


> > +
> > +		/* Check if required scaling is within limits */
> > +		hscale = drm_rect_calc_hscale(src, dst, 1, max_hscale);
> > +		vscale = drm_rect_calc_vscale(src, dst, 1, max_vscale);
> > +
> > +		if (hscale < 0 || vscale < 0) {
> > +			drm_dbg_kms(&dev_priv->drm,
> > +				    "Scaler %d doesn't support required plane scaling\n",
> > +				    *scaler_id);
> > +			drm_rect_debug_print("src: ", src, true);
> > +			drm_rect_debug_print("dst: ", dst, false);
> > +
> > +			scaler_state->scalers[*scaler_id].in_use = 0;
> > +			*scaler_id = -1;
> > +
> > +			return;
> 
> This would have to return an error rather than pretending that
> everything is fine.

We were already pretending everything is fine if a scaler if we
couldn't find a free scaler, for instance, so I just kept the same
logic, clearing up the scaler_id and marking the scaler as not in use
as well.

I can convert this to return an error, of course.  But then in the "not
free scaler" case we would still just ignore it or should we change the
behavior and make it fail as well?


[...]
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > index 10e1fc9d0698..9100f328df60 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > @@ -887,7 +887,7 @@ void intel_crtc_planes_update_arm(struct intel_atomic_state *state,
> >  
> >  int intel_atomic_plane_check_clipping(struct intel_plane_state *plane_state,
> >  				      struct intel_crtc_state *crtc_state,
> > -				      int min_scale, int max_scale,
> > +				      bool allow_scaling,
> >  				      bool can_position)
> >  {
> >  	struct drm_i915_private *i915 = to_i915(plane_state->uapi.plane->dev);
> > @@ -897,19 +897,50 @@ int intel_atomic_plane_check_clipping(struct intel_plane_state *plane_state,
> >  	const struct drm_rect *clip = &crtc_state->pipe_src;
> >  	unsigned int rotation = plane_state->hw.rotation;
> >  	int hscale, vscale;
> > +	int max_hscale, min_hscale, max_vscale, min_vscale;
> >  
> >  	if (!fb) {
> >  		plane_state->uapi.visible = false;
> >  		return 0;
> >  	}
> >  
> > +	/*
> > +	 * At this point we don't really know the HW limitations, so
> > +	 * we just sanitize the values against the maximum supported
> > +	 * scaling.
> > +	 */
> > +	if (allow_scaling) {
> > +		min_vscale = 1;
> > +		min_hscale = 1;
> > +
> > +		if (DISPLAY_VER(i915) < 10 ||
> > +		    intel_format_info_is_yuv_semiplanar(fb->format,
> > +							fb->modifier)) {
> > +			max_vscale = 0x20000 - 1;
> > +			max_hscale = 0x20000 - 1;
> > +		} else {
> > +			max_vscale = 0x30000 - 1;
> > +			max_hscale = 0x30000 - 1;
> > +		}
> > +	} else {
> > +		min_hscale = DRM_PLANE_NO_SCALING;
> > +		max_hscale = DRM_PLANE_NO_SCALING;
> > +		min_vscale = DRM_PLANE_NO_SCALING;
> > +		max_vscale = DRM_PLANE_NO_SCALING;
> > +	}
> 
> I still don't see the point in moving this hw specific knowledge
> from the more hw specific files into the hw agnostic file.

Is this file really that HW agnostic? I see lots of version checks with
"if (DISPLAY_VER(x))" all over the place.

As we discussed before, I think this kind of rules should be in HW-
specific configurations, but we don't have that yet.  And I thought it
would be better to keep these decisions in a single place rather than
just calling functions in other files...

If you prefer, I can move this back to skl_universal_plane.c or some
other of the skl_*.c files, but TBH they don't seem to be the right
place for this to me either...


[...]
> > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
> > index e6b4d24b9cd0..9ad1173a0551 100644
> > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> > @@ -1355,22 +1355,11 @@ g4x_sprite_check(struct intel_crtc_state *crtc_state,
> >  {
> >  	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
> >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > -	int min_scale = DRM_PLANE_NO_SCALING;
> > -	int max_scale = DRM_PLANE_NO_SCALING;
> >  	int ret;
> >  
> > -	if (g4x_fb_scalable(plane_state->hw.fb)) {
> > -		if (DISPLAY_VER(dev_priv) < 7) {
> > -			min_scale = 1;
> > -			max_scale = 16 << 16;
> > -		} else if (IS_IVYBRIDGE(dev_priv)) {
> > -			min_scale = 1;
> > -			max_scale = 2 << 16;
> > -		}
> > -	}
> 
> So what happened to these limits?

Oh, it seems that I lost them.  I guess they should be moved to the
intel_atomic_plane_check_clipping() function.  Again, to keep it all in
a single place.  But this seems to be only required in the sprite code,
so I'm not sure what I can do.

It's a problem to have this kinds of checks everywhere. 😞


[...]
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index 76490cc59d8f..e2ae6624378f 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -1463,22 +1463,6 @@ static int skl_plane_check_nv12_rotation(const struct intel_plane_state *plane_s
> >  	return 0;
> >  }
> >  
> > -static int skl_plane_max_scale(struct drm_i915_private *dev_priv,
> > -			       const struct drm_framebuffer *fb)
> > -{
> > -	/*
> > -	 * We don't yet know the final source width nor
> > -	 * whether we can use the HQ scaler mode. Assume
> > -	 * the best case.
> > -	 * FIXME need to properly check this later.
> > -	 */
> 
> Doesn't look like that FIXME has been dealt with as far
> as the hq scaler is concerned.

We now check the limits _after_ having decided whether HQ mode is used.
So that should be covered, right?


> 
Thanks for your review!

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v3 1/2] drm/i915/mtl: limit second scaler vertical scaling in ver >= 14

[Ville Syrjälä]

On Wed, Nov 23, 2022 at 09:16:42AM +0000, Coelho, Luciano wrote:
> On Wed, 2022-11-23 at 08:47 +0200, Ville Syrjälä wrote:
> > On Tue, Nov 22, 2022 at 12:23:43PM +0200, Luca Coelho wrote:
> > > In newer hardware versions (i.e. display version >= 14), the second
> > > scaler doesn't support vertical scaling.
> > > 
> > > The current implementation of the scaling limits is simplified and
> > > only occurs when the planes are created, so we don't know which scaler
> > > is being used.
> > > 
> > > In order to handle separate scaling limits for horizontal and vertical
> > > scaling, and different limits per scaler, split the checks in two
> > > phases.  We first do a simple check during plane creation and use the
> > > best-case scenario (because we don't know the scaler that may be used
> > > at a later point) and then do a more specific check when the scalers
> > > are actually being set up.
> > > 
> > > Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> > > ---
> 
> 
> [...]
> > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > index 6621aa245caf..43b1c7a227f8 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > @@ -38,6 +38,7 @@
> > >  #include "intel_atomic.h"
> > >  #include "intel_cdclk.h"
> > >  #include "intel_display_types.h"
> > > +#include "intel_fb.h"
> > >  #include "intel_global_state.h"
> > >  #include "intel_hdcp.h"
> > >  #include "intel_psr.h"
> > > @@ -375,6 +376,52 @@ static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_sta
> > >  		mode = SKL_PS_SCALER_MODE_DYN;
> > >  	}
> > >  
> > > +	if (plane_state && plane_state->hw.fb) {
> > > +		const struct drm_framebuffer *fb = plane_state->hw.fb;
> > > +		struct drm_rect *src = &plane_state->uapi.src;
> > > +		struct drm_rect *dst = &plane_state->uapi.dst;
> > 
> > We want the scale factor checks for the pfit use case too. So this
> > stuff shouldn't be so tied into planes. I guess we could go with
> > a FIXME initially.
> 
> Okay, I'll add a FIXME.  It was tied to the plane checks before,
> though, wasn't it? So nothing should have changed in that regard here.
> 
> 
> > > +		int hscale, vscale, max_vscale, max_hscale;
> > > +
> > > +		if (DISPLAY_VER(dev_priv) >= 14) {
> > > +			/*
> > > +			 * On versions 14 and up, only the first
> > > +			 * scaler supports a vertical scaling factor
> > > +			 * of more than 1.0, while a horizontal
> > > +			 * scaling factor of 3.0 is supported.
> > > +			 */
> > > +			max_hscale = 0x30000 - 1;
> > > +			if (*scaler_id == 0)
> > > +				max_vscale = 0x30000 - 1;
> > > +			else
> > > +				max_vscale = 0x10000;
> > 
> > We still have the chicken vs. egg problem here that we'd need to
> > consider the scale factors already when selecting the scaler.
> > But that could be another FIXME.
> 
> Do you mean in regards to the HQ vs. non-HQ needs?

I mean the "no downscaling on the second scaler" thing. The
problem is that this thing walks the scaler users in order
and assigns each one a scaler in turn. So if the first user
doesn't need downscaling but the second one does then we
will fail. OTOH had we just assigned the scalers in the
reverse order we would have succeeded.

> 
> 
> > > +
> > > +		} else if (DISPLAY_VER(dev_priv) >= 10 ||
> > > +			   !intel_format_info_is_yuv_semiplanar(fb->format, fb->modifier)) {
> > > +			max_hscale = 0x30000 - 1;
> > > +			max_vscale = 0x30000 - 1;
> > > +		} else {
> > > +			max_hscale = 0x20000 - 1;
> > > +			max_vscale = 0x20000 - 1;
> > > +		}
> > 
> > Pre-glk hq scaler case not handled.
> 
> I don't recall seen this specifically checked before.  Is this the
> stuff I missed from g4x_sprite_check() below? Or am I missing something
> else?

It was broken before in the sense that it allowed up to 3.0 scale factor
whether or not the HQ scaler was used or not. Now it will reject anything
above 2.0 even if the HQ scaler is used. So I guess technically it's a bit
less broken now, but more limited. Anyways, should be possible to just
check the scaler mode and pick the correct scaling limits based on it.

> 
> 
> > > +
> > > +		/* Check if required scaling is within limits */
> > > +		hscale = drm_rect_calc_hscale(src, dst, 1, max_hscale);
> > > +		vscale = drm_rect_calc_vscale(src, dst, 1, max_vscale);
> > > +
> > > +		if (hscale < 0 || vscale < 0) {
> > > +			drm_dbg_kms(&dev_priv->drm,
> > > +				    "Scaler %d doesn't support required plane scaling\n",
> > > +				    *scaler_id);
> > > +			drm_rect_debug_print("src: ", src, true);
> > > +			drm_rect_debug_print("dst: ", dst, false);
> > > +
> > > +			scaler_state->scalers[*scaler_id].in_use = 0;
> > > +			*scaler_id = -1;
> > > +
> > > +			return;
> > 
> > This would have to return an error rather than pretending that
> > everything is fine.
> 
> We were already pretending everything is fine if a scaler if we
> couldn't find a free scaler, for instance, so I just kept the same
> logic, clearing up the scaler_id and marking the scaler as not in use
> as well.
> 
> I can convert this to return an error, of course.  But then in the "not
> free scaler" case we would still just ignore it or should we change the
> behavior and make it fail as well?

The code is a mess, but it does look like intel_atomic_setup_scalers()
should fail correctly if we can't find enough scalers.

> 
> 
> [...]
> > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > index 10e1fc9d0698..9100f328df60 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > @@ -887,7 +887,7 @@ void intel_crtc_planes_update_arm(struct intel_atomic_state *state,
> > >  
> > >  int intel_atomic_plane_check_clipping(struct intel_plane_state *plane_state,
> > >  				      struct intel_crtc_state *crtc_state,
> > > -				      int min_scale, int max_scale,
> > > +				      bool allow_scaling,
> > >  				      bool can_position)
> > >  {
> > >  	struct drm_i915_private *i915 = to_i915(plane_state->uapi.plane->dev);
> > > @@ -897,19 +897,50 @@ int intel_atomic_plane_check_clipping(struct intel_plane_state *plane_state,
> > >  	const struct drm_rect *clip = &crtc_state->pipe_src;
> > >  	unsigned int rotation = plane_state->hw.rotation;
> > >  	int hscale, vscale;
> > > +	int max_hscale, min_hscale, max_vscale, min_vscale;
> > >  
> > >  	if (!fb) {
> > >  		plane_state->uapi.visible = false;
> > >  		return 0;
> > >  	}
> > >  
> > > +	/*
> > > +	 * At this point we don't really know the HW limitations, so
> > > +	 * we just sanitize the values against the maximum supported
> > > +	 * scaling.
> > > +	 */
> > > +	if (allow_scaling) {
> > > +		min_vscale = 1;
> > > +		min_hscale = 1;
> > > +
> > > +		if (DISPLAY_VER(i915) < 10 ||
> > > +		    intel_format_info_is_yuv_semiplanar(fb->format,
> > > +							fb->modifier)) {
> > > +			max_vscale = 0x20000 - 1;
> > > +			max_hscale = 0x20000 - 1;
> > > +		} else {
> > > +			max_vscale = 0x30000 - 1;
> > > +			max_hscale = 0x30000 - 1;
> > > +		}
> > > +	} else {
> > > +		min_hscale = DRM_PLANE_NO_SCALING;
> > > +		max_hscale = DRM_PLANE_NO_SCALING;
> > > +		min_vscale = DRM_PLANE_NO_SCALING;
> > > +		max_vscale = DRM_PLANE_NO_SCALING;
> > > +	}
> > 
> > I still don't see the point in moving this hw specific knowledge
> > from the more hw specific files into the hw agnostic file.
> 
> Is this file really that HW agnostic? I see lots of version checks with
> "if (DISPLAY_VER(x))" all over the place.

It's hw agnostic in the sense that most of it applies
to all hw generations. And this function in particular is
pretty much entirely hw agnostic currently.

> 
> As we discussed before, I think this kind of rules should be in HW-
> specific configurations, but we don't have that yet.  And I thought it
> would be better to keep these decisions in a single place rather than
> just calling functions in other files...
> 
> If you prefer, I can move this back to skl_universal_plane.c or some
> other of the skl_*.c files, but TBH they don't seem to be the right
> place for this to me either...

The current place knows exactly what kind of hardware/plane its
dealing with, and thus knows its limits. Seems perfectly fine to me.

> 
> 
> [...]
> > > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > index e6b4d24b9cd0..9ad1173a0551 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > @@ -1355,22 +1355,11 @@ g4x_sprite_check(struct intel_crtc_state *crtc_state,
> > >  {
> > >  	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
> > >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > -	int min_scale = DRM_PLANE_NO_SCALING;
> > > -	int max_scale = DRM_PLANE_NO_SCALING;
> > >  	int ret;
> > >  
> > > -	if (g4x_fb_scalable(plane_state->hw.fb)) {
> > > -		if (DISPLAY_VER(dev_priv) < 7) {
> > > -			min_scale = 1;
> > > -			max_scale = 16 << 16;
> > > -		} else if (IS_IVYBRIDGE(dev_priv)) {
> > > -			min_scale = 1;
> > > -			max_scale = 2 << 16;
> > > -		}
> > > -	}
> > 
> > So what happened to these limits?
> 
> Oh, it seems that I lost them.  I guess they should be moved to the
> intel_atomic_plane_check_clipping() function.  Again, to keep it all in
> a single place.  But this seems to be only required in the sprite code,
> so I'm not sure what I can do.
> 
> It's a problem to have this kinds of checks everywhere. 😞
> 
> 
> [...]
> > > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > index 76490cc59d8f..e2ae6624378f 100644
> > > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > @@ -1463,22 +1463,6 @@ static int skl_plane_check_nv12_rotation(const struct intel_plane_state *plane_s
> > >  	return 0;
> > >  }
> > >  
> > > -static int skl_plane_max_scale(struct drm_i915_private *dev_priv,
> > > -			       const struct drm_framebuffer *fb)
> > > -{
> > > -	/*
> > > -	 * We don't yet know the final source width nor
> > > -	 * whether we can use the HQ scaler mode. Assume
> > > -	 * the best case.
> > > -	 * FIXME need to properly check this later.
> > > -	 */
> > 
> > Doesn't look like that FIXME has been dealt with as far
> > as the hq scaler is concerned.
> 
> We now check the limits _after_ having decided whether HQ mode is used.
> So that should be covered, right?

If we actaully add the hq mode check when determining the scaling
limits.

> 
> 
> > 
> Thanks for your review!
> 
> --
> Cheers,
> Luca.

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH v3 1/2] drm/i915/mtl: limit second scaler vertical scaling in ver >= 14

[Coelho, Luciano]

On Wed, 2022-11-23 at 11:57 +0200, Ville Syrjälä wrote:
> On Wed, Nov 23, 2022 at 09:16:42AM +0000, Coelho, Luciano wrote:
> > On Wed, 2022-11-23 at 08:47 +0200, Ville Syrjälä wrote:
> > > On Tue, Nov 22, 2022 at 12:23:43PM +0200, Luca Coelho wrote:
> > > > In newer hardware versions (i.e. display version >= 14), the second
> > > > scaler doesn't support vertical scaling.
> > > > 
> > > > The current implementation of the scaling limits is simplified and
> > > > only occurs when the planes are created, so we don't know which scaler
> > > > is being used.
> > > > 
> > > > In order to handle separate scaling limits for horizontal and vertical
> > > > scaling, and different limits per scaler, split the checks in two
> > > > phases.  We first do a simple check during plane creation and use the
> > > > best-case scenario (because we don't know the scaler that may be used
> > > > at a later point) and then do a more specific check when the scalers
> > > > are actually being set up.
> > > > 
> > > > Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> > > > ---
> > 
> > 
> > [...]
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > index 6621aa245caf..43b1c7a227f8 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > @@ -38,6 +38,7 @@
> > > >  #include "intel_atomic.h"
> > > >  #include "intel_cdclk.h"
> > > >  #include "intel_display_types.h"
> > > > +#include "intel_fb.h"
> > > >  #include "intel_global_state.h"
> > > >  #include "intel_hdcp.h"
> > > >  #include "intel_psr.h"
> > > > @@ -375,6 +376,52 @@ static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_sta
> > > >  		mode = SKL_PS_SCALER_MODE_DYN;
> > > >  	}
> > > >  
> > > > +	if (plane_state && plane_state->hw.fb) {
> > > > +		const struct drm_framebuffer *fb = plane_state->hw.fb;
> > > > +		struct drm_rect *src = &plane_state->uapi.src;
> > > > +		struct drm_rect *dst = &plane_state->uapi.dst;
> > > 
> > > We want the scale factor checks for the pfit use case too. So this
> > > stuff shouldn't be so tied into planes. I guess we could go with
> > > a FIXME initially.
> > 
> > Okay, I'll add a FIXME.  It was tied to the plane checks before,
> > though, wasn't it? So nothing should have changed in that regard here.
> > 
> > 
> > > > +		int hscale, vscale, max_vscale, max_hscale;
> > > > +
> > > > +		if (DISPLAY_VER(dev_priv) >= 14) {
> > > > +			/*
> > > > +			 * On versions 14 and up, only the first
> > > > +			 * scaler supports a vertical scaling factor
> > > > +			 * of more than 1.0, while a horizontal
> > > > +			 * scaling factor of 3.0 is supported.
> > > > +			 */
> > > > +			max_hscale = 0x30000 - 1;
> > > > +			if (*scaler_id == 0)
> > > > +				max_vscale = 0x30000 - 1;
> > > > +			else
> > > > +				max_vscale = 0x10000;
> > > 
> > > We still have the chicken vs. egg problem here that we'd need to
> > > consider the scale factors already when selecting the scaler.
> > > But that could be another FIXME.
> > 
> > Do you mean in regards to the HQ vs. non-HQ needs?
> 
> I mean the "no downscaling on the second scaler" thing. The
> problem is that this thing walks the scaler users in order
> and assigns each one a scaler in turn. So if the first user
> doesn't need downscaling but the second one does then we
> will fail. OTOH had we just assigned the scalers in the
> reverse order we would have succeeded.

Ah, now I get it.

So, I guess we can do a similar thing to what we already do earlier in
this function to select the first scaler if HQ is needed.  If
downscaling is needed in one of the scalers but not in the other, we
can swap them to make sure the one that needs downscaling in in the
first one.

But I agree with you in that this could be an added FIXME in this patch
and the actual change could be made in a separate patch.


> > > > +
> > > > +		} else if (DISPLAY_VER(dev_priv) >= 10 ||
> > > > +			   !intel_format_info_is_yuv_semiplanar(fb->format, fb->modifier)) {
> > > > +			max_hscale = 0x30000 - 1;
> > > > +			max_vscale = 0x30000 - 1;
> > > > +		} else {
> > > > +			max_hscale = 0x20000 - 1;
> > > > +			max_vscale = 0x20000 - 1;
> > > > +		}
> > > 
> > > Pre-glk hq scaler case not handled.
> > 
> > I don't recall seen this specifically checked before.  Is this the
> > stuff I missed from g4x_sprite_check() below? Or am I missing something
> > else?
> 
> It was broken before in the sense that it allowed up to 3.0 scale factor
> whether or not the HQ scaler was used or not. Now it will reject anything
> above 2.0 even if the HQ scaler is used. So I guess technically it's a bit
> less broken now, but more limited. Anyways, should be possible to just
> check the scaler mode and pick the correct scaling limits based on it.

I see what you mean.  But the code from before had the exact same
thing, I think? We were also checking for VER >= 10 and only then using
3.0.  For anything else the limit was 2.0.  Is your comment related to
the FIXME I removed from below? (your last comment in this email)


> > > > +
> > > > +		/* Check if required scaling is within limits */
> > > > +		hscale = drm_rect_calc_hscale(src, dst, 1, max_hscale);
> > > > +		vscale = drm_rect_calc_vscale(src, dst, 1, max_vscale);
> > > > +
> > > > +		if (hscale < 0 || vscale < 0) {
> > > > +			drm_dbg_kms(&dev_priv->drm,
> > > > +				    "Scaler %d doesn't support required plane scaling\n",
> > > > +				    *scaler_id);
> > > > +			drm_rect_debug_print("src: ", src, true);
> > > > +			drm_rect_debug_print("dst: ", dst, false);
> > > > +
> > > > +			scaler_state->scalers[*scaler_id].in_use = 0;
> > > > +			*scaler_id = -1;
> > > > +
> > > > +			return;
> > > 
> > > This would have to return an error rather than pretending that
> > > everything is fine.
> > 
> > We were already pretending everything is fine if a scaler if we
> > couldn't find a free scaler, for instance, so I just kept the same
> > logic, clearing up the scaler_id and marking the scaler as not in use
> > as well.
> > 
> > I can convert this to return an error, of course.  But then in the "not
> > free scaler" case we would still just ignore it or should we change the
> > behavior and make it fail as well?
> 
> The code is a mess, but it does look like intel_atomic_setup_scalers()
> should fail correctly if we can't find enough scalers.

Okay, so I'll change this function to return errors in both cases.


> > [...]
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > > index 10e1fc9d0698..9100f328df60 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > > @@ -887,7 +887,7 @@ void intel_crtc_planes_update_arm(struct intel_atomic_state *state,
> > > >  
> > > >  int intel_atomic_plane_check_clipping(struct intel_plane_state *plane_state,
> > > >  				      struct intel_crtc_state *crtc_state,
> > > > -				      int min_scale, int max_scale,
> > > > +				      bool allow_scaling,
> > > >  				      bool can_position)
> > > >  {
> > > >  	struct drm_i915_private *i915 = to_i915(plane_state->uapi.plane->dev);
> > > > @@ -897,19 +897,50 @@ int intel_atomic_plane_check_clipping(struct intel_plane_state *plane_state,
> > > >  	const struct drm_rect *clip = &crtc_state->pipe_src;
> > > >  	unsigned int rotation = plane_state->hw.rotation;
> > > >  	int hscale, vscale;
> > > > +	int max_hscale, min_hscale, max_vscale, min_vscale;
> > > >  
> > > >  	if (!fb) {
> > > >  		plane_state->uapi.visible = false;
> > > >  		return 0;
> > > >  	}
> > > >  
> > > > +	/*
> > > > +	 * At this point we don't really know the HW limitations, so
> > > > +	 * we just sanitize the values against the maximum supported
> > > > +	 * scaling.
> > > > +	 */
> > > > +	if (allow_scaling) {
> > > > +		min_vscale = 1;
> > > > +		min_hscale = 1;
> > > > +
> > > > +		if (DISPLAY_VER(i915) < 10 ||
> > > > +		    intel_format_info_is_yuv_semiplanar(fb->format,
> > > > +							fb->modifier)) {
> > > > +			max_vscale = 0x20000 - 1;
> > > > +			max_hscale = 0x20000 - 1;
> > > > +		} else {
> > > > +			max_vscale = 0x30000 - 1;
> > > > +			max_hscale = 0x30000 - 1;
> > > > +		}
> > > > +	} else {
> > > > +		min_hscale = DRM_PLANE_NO_SCALING;
> > > > +		max_hscale = DRM_PLANE_NO_SCALING;
> > > > +		min_vscale = DRM_PLANE_NO_SCALING;
> > > > +		max_vscale = DRM_PLANE_NO_SCALING;
> > > > +	}
> > > 
> > > I still don't see the point in moving this hw specific knowledge
> > > from the more hw specific files into the hw agnostic file.
> > 
> > Is this file really that HW agnostic? I see lots of version checks with
> > "if (DISPLAY_VER(x))" all over the place.
> 
> It's hw agnostic in the sense that most of it applies
> to all hw generations. And this function in particular is
> pretty much entirely hw agnostic currently.
> 




> > As we discussed before, I think this kind of rules should be in HW-
> > specific configurations, but we don't have that yet.  And I thought it
> > would be better to keep these decisions in a single place rather than
> > just calling functions in other files...
> > 
> > If you prefer, I can move this back to skl_universal_plane.c or some
> > other of the skl_*.c files, but TBH they don't seem to be the right
> > place for this to me either...
> 
> The current place knows exactly what kind of hardware/plane its
> dealing with, and thus knows its limits. Seems perfectly fine to me.

By "current place" you mean before this patch? I.e. in
skl_universal_plane.c?


> > [...]
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > index e6b4d24b9cd0..9ad1173a0551 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > @@ -1355,22 +1355,11 @@ g4x_sprite_check(struct intel_crtc_state *crtc_state,
> > > >  {
> > > >  	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
> > > >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > -	int min_scale = DRM_PLANE_NO_SCALING;
> > > > -	int max_scale = DRM_PLANE_NO_SCALING;
> > > >  	int ret;
> > > >  
> > > > -	if (g4x_fb_scalable(plane_state->hw.fb)) {
> > > > -		if (DISPLAY_VER(dev_priv) < 7) {
> > > > -			min_scale = 1;
> > > > -			max_scale = 16 << 16;
> > > > -		} else if (IS_IVYBRIDGE(dev_priv)) {
> > > > -			min_scale = 1;
> > > > -			max_scale = 2 << 16;
> > > > -		}
> > > > -	}
> > > 
> > > So what happened to these limits?
> > 
> > Oh, it seems that I lost them.  I guess they should be moved to the
> > intel_atomic_plane_check_clipping() function.  Again, to keep it all in
> > a single place.  But this seems to be only required in the sprite code,
> > so I'm not sure what I can do.
> > 
> > It's a problem to have this kinds of checks everywhere. 😞
> > 
> > 
> > [...]
> > > > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > > index 76490cc59d8f..e2ae6624378f 100644
> > > > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > > @@ -1463,22 +1463,6 @@ static int skl_plane_check_nv12_rotation(const struct intel_plane_state *plane_s
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -static int skl_plane_max_scale(struct drm_i915_private *dev_priv,
> > > > -			       const struct drm_framebuffer *fb)
> > > > -{
> > > > -	/*
> > > > -	 * We don't yet know the final source width nor
> > > > -	 * whether we can use the HQ scaler mode. Assume
> > > > -	 * the best case.
> > > > -	 * FIXME need to properly check this later.
> > > > -	 */
> > > 
> > > Doesn't look like that FIXME has been dealt with as far
> > > as the hq scaler is concerned.
> > 
> > We now check the limits _after_ having decided whether HQ mode is used.
> > So that should be covered, right?
> 
> If we actaully add the hq mode check when determining the scaling
> limits.

Right, so the new FIXME I'll add as discussed above should cover this.

--
Cheers,
Luca.

Re: [Intel-gfx] [PATCH v3 1/2] drm/i915/mtl: limit second scaler vertical scaling in ver >= 14

[Coelho, Luciano]

On Wed, 2022-11-23 at 12:21 +0200, Luciano Coelho wrote:
> On Wed, 2022-11-23 at 11:57 +0200, Ville Syrjälä wrote:
> > On Wed, Nov 23, 2022 at 09:16:42AM +0000, Coelho, Luciano wrote:
> > > On Wed, 2022-11-23 at 08:47 +0200, Ville Syrjälä wrote:
> > > > On Tue, Nov 22, 2022 at 12:23:43PM +0200, Luca Coelho wrote:
> > > > > In newer hardware versions (i.e. display version >= 14), the second
> > > > > scaler doesn't support vertical scaling.
> > > > > 
> > > > > The current implementation of the scaling limits is simplified and
> > > > > only occurs when the planes are created, so we don't know which scaler
> > > > > is being used.
> > > > > 
> > > > > In order to handle separate scaling limits for horizontal and vertical
> > > > > scaling, and different limits per scaler, split the checks in two
> > > > > phases.  We first do a simple check during plane creation and use the
> > > > > best-case scenario (because we don't know the scaler that may be used
> > > > > at a later point) and then do a more specific check when the scalers
> > > > > are actually being set up.
> > > > > 
> > > > > Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> > > > > ---
> > > 
> > > 
> > > [...]
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > > index 6621aa245caf..43b1c7a227f8 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > > @@ -38,6 +38,7 @@
> > > > >  #include "intel_atomic.h"
> > > > >  #include "intel_cdclk.h"
> > > > >  #include "intel_display_types.h"
> > > > > +#include "intel_fb.h"
> > > > >  #include "intel_global_state.h"
> > > > >  #include "intel_hdcp.h"
> > > > >  #include "intel_psr.h"
> > > > > @@ -375,6 +376,52 @@ static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_sta
> > > > >  		mode = SKL_PS_SCALER_MODE_DYN;
> > > > >  	}
> > > > >  
> > > > > +	if (plane_state && plane_state->hw.fb) {
> > > > > +		const struct drm_framebuffer *fb = plane_state->hw.fb;
> > > > > +		struct drm_rect *src = &plane_state->uapi.src;
> > > > > +		struct drm_rect *dst = &plane_state->uapi.dst;
> > > > 
> > > > We want the scale factor checks for the pfit use case too. So this
> > > > stuff shouldn't be so tied into planes. I guess we could go with
> > > > a FIXME initially.
> > > 
> > > Okay, I'll add a FIXME.  It was tied to the plane checks before,
> > > though, wasn't it? So nothing should have changed in that regard here.
> > > 
> > > 
> > > > > +		int hscale, vscale, max_vscale, max_hscale;
> > > > > +
> > > > > +		if (DISPLAY_VER(dev_priv) >= 14) {
> > > > > +			/*
> > > > > +			 * On versions 14 and up, only the first
> > > > > +			 * scaler supports a vertical scaling factor
> > > > > +			 * of more than 1.0, while a horizontal
> > > > > +			 * scaling factor of 3.0 is supported.
> > > > > +			 */
> > > > > +			max_hscale = 0x30000 - 1;
> > > > > +			if (*scaler_id == 0)
> > > > > +				max_vscale = 0x30000 - 1;
> > > > > +			else
> > > > > +				max_vscale = 0x10000;
> > > > 
> > > > We still have the chicken vs. egg problem here that we'd need to
> > > > consider the scale factors already when selecting the scaler.
> > > > But that could be another FIXME.
> > > 
> > > Do you mean in regards to the HQ vs. non-HQ needs?
> > 
> > I mean the "no downscaling on the second scaler" thing. The
> > problem is that this thing walks the scaler users in order
> > and assigns each one a scaler in turn. So if the first user
> > doesn't need downscaling but the second one does then we
> > will fail. OTOH had we just assigned the scalers in the
> > reverse order we would have succeeded.
> 
> Ah, now I get it.
> 
> So, I guess we can do a similar thing to what we already do earlier in
> this function to select the first scaler if HQ is needed.  If
> downscaling is needed in one of the scalers but not in the other, we
> can swap them to make sure the one that needs downscaling in in the
> first one.
> 
> But I agree with you in that this could be an added FIXME in this patch
> and the actual change could be made in a separate patch.
> 
> 
> > > > > +
> > > > > +		} else if (DISPLAY_VER(dev_priv) >= 10 ||
> > > > > +			   !intel_format_info_is_yuv_semiplanar(fb->format, fb->modifier)) {
> > > > > +			max_hscale = 0x30000 - 1;
> > > > > +			max_vscale = 0x30000 - 1;
> > > > > +		} else {
> > > > > +			max_hscale = 0x20000 - 1;
> > > > > +			max_vscale = 0x20000 - 1;
> > > > > +		}
> > > > 
> > > > Pre-glk hq scaler case not handled.
> > > 
> > > I don't recall seen this specifically checked before.  Is this the
> > > stuff I missed from g4x_sprite_check() below? Or am I missing something
> > > else?
> > 
> > It was broken before in the sense that it allowed up to 3.0 scale factor
> > whether or not the HQ scaler was used or not. Now it will reject anything
> > above 2.0 even if the HQ scaler is used. So I guess technically it's a bit
> > less broken now, but more limited. Anyways, should be possible to just
> > check the scaler mode and pick the correct scaling limits based on it.
> 
> I see what you mean.  But the code from before had the exact same
> thing, I think? We were also checking for VER >= 10 and only then using
> 3.0.  For anything else the limit was 2.0.  Is your comment related to
> the FIXME I removed from below? (your last comment in this email)
> 
> 
> > > > > +
> > > > > +		/* Check if required scaling is within limits */
> > > > > +		hscale = drm_rect_calc_hscale(src, dst, 1, max_hscale);
> > > > > +		vscale = drm_rect_calc_vscale(src, dst, 1, max_vscale);
> > > > > +
> > > > > +		if (hscale < 0 || vscale < 0) {
> > > > > +			drm_dbg_kms(&dev_priv->drm,
> > > > > +				    "Scaler %d doesn't support required plane scaling\n",
> > > > > +				    *scaler_id);
> > > > > +			drm_rect_debug_print("src: ", src, true);
> > > > > +			drm_rect_debug_print("dst: ", dst, false);
> > > > > +
> > > > > +			scaler_state->scalers[*scaler_id].in_use = 0;
> > > > > +			*scaler_id = -1;
> > > > > +
> > > > > +			return;
> > > > 
> > > > This would have to return an error rather than pretending that
> > > > everything is fine.
> > > 
> > > We were already pretending everything is fine if a scaler if we
> > > couldn't find a free scaler, for instance, so I just kept the same
> > > logic, clearing up the scaler_id and marking the scaler as not in use
> > > as well.
> > > 
> > > I can convert this to return an error, of course.  But then in the "not
> > > free scaler" case we would still just ignore it or should we change the
> > > behavior and make it fail as well?
> > 
> > The code is a mess, but it does look like intel_atomic_setup_scalers()
> > should fail correctly if we can't find enough scalers.
> 
> Okay, so I'll change this function to return errors in both cases.
> 
> 
> > > [...]
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > > > index 10e1fc9d0698..9100f328df60 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > > > @@ -887,7 +887,7 @@ void intel_crtc_planes_update_arm(struct intel_atomic_state *state,
> > > > >  
> > > > >  int intel_atomic_plane_check_clipping(struct intel_plane_state *plane_state,
> > > > >  				      struct intel_crtc_state *crtc_state,
> > > > > -				      int min_scale, int max_scale,
> > > > > +				      bool allow_scaling,
> > > > >  				      bool can_position)
> > > > >  {
> > > > >  	struct drm_i915_private *i915 = to_i915(plane_state->uapi.plane->dev);
> > > > > @@ -897,19 +897,50 @@ int intel_atomic_plane_check_clipping(struct intel_plane_state *plane_state,
> > > > >  	const struct drm_rect *clip = &crtc_state->pipe_src;
> > > > >  	unsigned int rotation = plane_state->hw.rotation;
> > > > >  	int hscale, vscale;
> > > > > +	int max_hscale, min_hscale, max_vscale, min_vscale;
> > > > >  
> > > > >  	if (!fb) {
> > > > >  		plane_state->uapi.visible = false;
> > > > >  		return 0;
> > > > >  	}
> > > > >  
> > > > > +	/*
> > > > > +	 * At this point we don't really know the HW limitations, so
> > > > > +	 * we just sanitize the values against the maximum supported
> > > > > +	 * scaling.
> > > > > +	 */
> > > > > +	if (allow_scaling) {
> > > > > +		min_vscale = 1;
> > > > > +		min_hscale = 1;
> > > > > +
> > > > > +		if (DISPLAY_VER(i915) < 10 ||
> > > > > +		    intel_format_info_is_yuv_semiplanar(fb->format,
> > > > > +							fb->modifier)) {
> > > > > +			max_vscale = 0x20000 - 1;
> > > > > +			max_hscale = 0x20000 - 1;
> > > > > +		} else {
> > > > > +			max_vscale = 0x30000 - 1;
> > > > > +			max_hscale = 0x30000 - 1;
> > > > > +		}
> > > > > +	} else {
> > > > > +		min_hscale = DRM_PLANE_NO_SCALING;
> > > > > +		max_hscale = DRM_PLANE_NO_SCALING;
> > > > > +		min_vscale = DRM_PLANE_NO_SCALING;
> > > > > +		max_vscale = DRM_PLANE_NO_SCALING;
> > > > > +	}
> > > > 
> > > > I still don't see the point in moving this hw specific knowledge
> > > > from the more hw specific files into the hw agnostic file.
> > > 
> > > Is this file really that HW agnostic? I see lots of version checks with
> > > "if (DISPLAY_VER(x))" all over the place.
> > 
> > It's hw agnostic in the sense that most of it applies
> > to all hw generations. And this function in particular is
> > pretty much entirely hw agnostic currently.
> > 
> 
> 
> 
> 
> > > As we discussed before, I think this kind of rules should be in HW-
> > > specific configurations, but we don't have that yet.  And I thought it
> > > would be better to keep these decisions in a single place rather than
> > > just calling functions in other files...
> > > 
> > > If you prefer, I can move this back to skl_universal_plane.c or some
> > > other of the skl_*.c files, but TBH they don't seem to be the right
> > > place for this to me either...
> > 
> > The current place knows exactly what kind of hardware/plane its
> > dealing with, and thus knows its limits. Seems perfectly fine to me.
> 
> By "current place" you mean before this patch? I.e. in
> skl_universal_plane.c?
> 
> 
> > > [...]
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > > index e6b4d24b9cd0..9ad1173a0551 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > > @@ -1355,22 +1355,11 @@ g4x_sprite_check(struct intel_crtc_state *crtc_state,
> > > > >  {
> > > > >  	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
> > > > >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > > -	int min_scale = DRM_PLANE_NO_SCALING;
> > > > > -	int max_scale = DRM_PLANE_NO_SCALING;
> > > > >  	int ret;
> > > > >  
> > > > > -	if (g4x_fb_scalable(plane_state->hw.fb)) {
> > > > > -		if (DISPLAY_VER(dev_priv) < 7) {
> > > > > -			min_scale = 1;
> > > > > -			max_scale = 16 << 16;
> > > > > -		} else if (IS_IVYBRIDGE(dev_priv)) {
> > > > > -			min_scale = 1;
> > > > > -			max_scale = 2 << 16;
> > > > > -		}
> > > > > -	}
> > > > 
> > > > So what happened to these limits?
> > > 
> > > Oh, it seems that I lost them.  I guess they should be moved to the
> > > intel_atomic_plane_check_clipping() function.  Again, to keep it all in
> > > a single place.  But this seems to be only required in the sprite code,
> > > so I'm not sure what I can do.
> > > 
> > > It's a problem to have this kinds of checks everywhere. 😞

I can't wrap my head around this.  With my patch, the IVIBRIDGE part is
okay, because IVIBRIDGE is version 7 and for anything below version 10
we use 2.0 as the limit.

But I don't understand why anything below version 7 has a limit of 16.0
and only for this sprite related check.  Is that really correct? Why is
it only related to the sprite checks?


> > > 
> > > [...]
> > > > > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > > > index 76490cc59d8f..e2ae6624378f 100644
> > > > > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > > > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > > > @@ -1463,22 +1463,6 @@ static int skl_plane_check_nv12_rotation(const struct intel_plane_state *plane_s
> > > > >  	return 0;
> > > > >  }
> > > > >  
> > > > > -static int skl_plane_max_scale(struct drm_i915_private *dev_priv,
> > > > > -			       const struct drm_framebuffer *fb)
> > > > > -{
> > > > > -	/*
> > > > > -	 * We don't yet know the final source width nor
> > > > > -	 * whether we can use the HQ scaler mode. Assume
> > > > > -	 * the best case.
> > > > > -	 * FIXME need to properly check this later.
> > > > > -	 */
> > > > 
> > > > Doesn't look like that FIXME has been dealt with as far
> > > > as the hq scaler is concerned.
> > > 
> > > We now check the limits _after_ having decided whether HQ mode is used.
> > > So that should be covered, right?
> > 
> > If we actaully add the hq mode check when determining the scaling
> > limits.
> 
> Right, so the new FIXME I'll add as discussed above should cover this.
> 
> --
> Cheers,
> Luca.