[Intel-gfx] [PATCH v6 0/2] drm/i915/mtl: handle some MTL scaler limitations

[Intel-gfx] [PATCH v6 0/2] drm/i915/mtl: handle some MTL scaler limitations

[Luca Coelho]

Hi,

I stupidly sent an old version of the patches in my v5... Resending
the correct ones (which were sent as v4).

The versioning history is in the patches themselves.

Please review.

Cheers,
Luca.


Animesh Manna (1):
  drm/i915/mtl: Limit scaler input to 4k in plane scaling

Luca Coelho (1):
  drm/i915/mtl: limit second scaler vertical scaling in ver >= 14

 drivers/gpu/drm/i915/display/intel_atomic.c | 83 ++++++++++++++++++---
 drivers/gpu/drm/i915/display/skl_scaler.c   | 31 ++++++--
 2 files changed, 96 insertions(+), 18 deletions(-)

-- 
2.38.1

[Intel-gfx] [PATCH v6 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.

In v4:
   * Got rid of the changes in the general planes max scale code;
   * Added a couple of FIXMEs;
   * Made intel_atomic_setup_scaler() return an int with errors;

In v5:
   * Just resent with a cover letter.

In v6:
   * Now the correct version again (same as v4).


drivers/gpu/drm/i915/display/intel_atomic.c | 83 ++++++++++++++++++---
 1 file changed, 73 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
index 6621aa245caf..8373be283d8b 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic.c
@@ -41,6 +41,7 @@
 #include "intel_global_state.h"
 #include "intel_hdcp.h"
 #include "intel_psr.h"
+#include "intel_fb.h"
 #include "skl_universal_plane.h"
 
 /**
@@ -310,11 +311,11 @@ intel_crtc_destroy_state(struct drm_crtc *crtc,
 	kfree(crtc_state);
 }
 
-static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_state,
-				      int num_scalers_need, struct intel_crtc *intel_crtc,
-				      const char *name, int idx,
-				      struct intel_plane_state *plane_state,
-				      int *scaler_id)
+static int intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_state,
+				     int num_scalers_need, struct intel_crtc *intel_crtc,
+				     const char *name, int idx,
+				     struct intel_plane_state *plane_state,
+				     int *scaler_id)
 {
 	struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
 	int j;
@@ -334,7 +335,7 @@ static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_sta
 
 	if (drm_WARN(&dev_priv->drm, *scaler_id < 0,
 		     "Cannot find scaler for %s:%d\n", name, idx))
-		return;
+		return -EBUSY;
 
 	/* set scaler mode */
 	if (plane_state && plane_state->hw.fb &&
@@ -375,9 +376,69 @@ static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_sta
 		mode = SKL_PS_SCALER_MODE_DYN;
 	}
 
+	/*
+	 * FIXME: we should also check the scaler factors for pfit, so
+	 * this shouldn't be tied directly to planes.
+	 */
+	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;
+
+		/*
+		 * FIXME: When two scalers are needed, but only one of
+		 * them needs to downscale, we should make sure that
+		 * the one that needs downscaling support is assigned
+		 * as the first scaler, so we don't reject downscaling
+		 * unnecessarily.
+		 */
+
+		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 -EOPNOTSUPP;
+		}
+	}
+
 	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;
+
+	return 0;
 }
 
 /**
@@ -437,7 +498,7 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
 	for (i = 0; i < sizeof(scaler_state->scaler_users) * 8; i++) {
 		int *scaler_id;
 		const char *name;
-		int idx;
+		int idx, ret;
 
 		/* skip if scaler not required */
 		if (!(scaler_state->scaler_users & (1 << i)))
@@ -494,9 +555,11 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
 			scaler_id = &plane_state->scaler_id;
 		}
 
-		intel_atomic_setup_scaler(scaler_state, num_scalers_need,
-					  intel_crtc, name, idx,
-					  plane_state, scaler_id);
+		ret = intel_atomic_setup_scaler(scaler_state, num_scalers_need,
+						intel_crtc, name, idx,
+						plane_state, scaler_id);
+		if (ret < 0)
+			return ret;
 	}
 
 	return 0;
-- 
2.38.1

[Intel-gfx] [PATCH v6 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: 2438 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>
---
 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 v6 1/2] drm/i915/mtl: limit second scaler vertical scaling in ver >= 14

[Ville Syrjälä]

On Tue, Dec 20, 2022 at 02:07:23PM +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.
> 
> In v4:
>    * Got rid of the changes in the general planes max scale code;
>    * Added a couple of FIXMEs;
>    * Made intel_atomic_setup_scaler() return an int with errors;
> 
> In v5:
>    * Just resent with a cover letter.
> 
> In v6:
>    * Now the correct version again (same as v4).
> 
> 
> drivers/gpu/drm/i915/display/intel_atomic.c | 83 ++++++++++++++++++---
>  1 file changed, 73 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> index 6621aa245caf..8373be283d8b 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> @@ -41,6 +41,7 @@
>  #include "intel_global_state.h"
>  #include "intel_hdcp.h"
>  #include "intel_psr.h"
> +#include "intel_fb.h"
>  #include "skl_universal_plane.h"
>  
>  /**
> @@ -310,11 +311,11 @@ intel_crtc_destroy_state(struct drm_crtc *crtc,
>  	kfree(crtc_state);
>  }
>  
> -static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_state,
> -				      int num_scalers_need, struct intel_crtc *intel_crtc,
> -				      const char *name, int idx,
> -				      struct intel_plane_state *plane_state,
> -				      int *scaler_id)
> +static int intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_state,
> +				     int num_scalers_need, struct intel_crtc *intel_crtc,
> +				     const char *name, int idx,
> +				     struct intel_plane_state *plane_state,
> +				     int *scaler_id)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
>  	int j;
> @@ -334,7 +335,7 @@ static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_sta
>  
>  	if (drm_WARN(&dev_priv->drm, *scaler_id < 0,
>  		     "Cannot find scaler for %s:%d\n", name, idx))
> -		return;
> +		return -EBUSY;
>  
>  	/* set scaler mode */
>  	if (plane_state && plane_state->hw.fb &&
> @@ -375,9 +376,69 @@ static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_sta
>  		mode = SKL_PS_SCALER_MODE_DYN;
>  	}
>  
> +	/*
> +	 * FIXME: we should also check the scaler factors for pfit, so
> +	 * this shouldn't be tied directly to planes.
> +	 */
> +	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;

Those can be const.

> +		int hscale, vscale, max_vscale, max_hscale;
> +
> +		/*
> +		 * FIXME: When two scalers are needed, but only one of
> +		 * them needs to downscale, we should make sure that
> +		 * the one that needs downscaling support is assigned
> +		 * as the first scaler, so we don't reject downscaling
> +		 * unnecessarily.
> +		 */
> +
> +		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;
> +		}

We'd want something along these lines if we want to handle 
the hq vs. dyn scaler stuff correctly.

if (DISPLAY_VER(dev_priv) >= 14) {
	...
} else if (DISPLAY_VER(dev_priv) >= 10)
	max_hscale = 0x30000 - 1;
	max_vscale = 0x30000 - 1;
} else if (mode == NV12) {
	max_hscale = 0x20000 - 1;
	max_vscale = 0x20000 - 1;
} else if (mode == HQ || src_w <= 2048) {
	max_hscale = 0x30000 - 1;
	max_vscale = 0x30000 - 1;
} else {
	max_hscale = 0x30000 - 1;
	max_vscale = 0x20000 - 1;
}

Though we could leave that for a followup patch, in which
case perhaps add a FIXME.

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

There should be no need to undo stuff like this.

> +
> +			return -EOPNOTSUPP;

We typically just go with -EINVAL for pretty much everything.
Given the number of things that can go wrong no one can realistically
figure out what happened/how to resolve it based on the errno alone
anyway.

> +		}
> +	}
> +
>  	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;
> +
> +	return 0;
>  }
>  
>  /**
> @@ -437,7 +498,7 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
>  	for (i = 0; i < sizeof(scaler_state->scaler_users) * 8; i++) {
>  		int *scaler_id;
>  		const char *name;
> -		int idx;
> +		int idx, ret;
>  
>  		/* skip if scaler not required */
>  		if (!(scaler_state->scaler_users & (1 << i)))
> @@ -494,9 +555,11 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
>  			scaler_id = &plane_state->scaler_id;
>  		}
>  
> -		intel_atomic_setup_scaler(scaler_state, num_scalers_need,
> -					  intel_crtc, name, idx,
> -					  plane_state, scaler_id);
> +		ret = intel_atomic_setup_scaler(scaler_state, num_scalers_need,
> +						intel_crtc, name, idx,
> +						plane_state, scaler_id);
> +		if (ret < 0)
> +			return ret;
>  	}
>  
>  	return 0;
> -- 
> 2.38.1

-- 
Ville Syrjälä
Intel

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

[Ville Syrjälä]

On Tue, Dec 20, 2022 at 02:07:24PM +0200, Luca Coelho wrote:
> 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>
> ---
>  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;
> +	}

Bspec says max_src_w=4096, max_src_h=8192, max_dst_w=8192,
max_dst_h=8192.

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

-- 
Ville Syrjälä
Intel

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

[Coelho, Luciano]

On Tue, 2022-12-20 at 23:00 +0200, Ville Syrjälä wrote:
> On Tue, Dec 20, 2022 at 02:07:23PM +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.
> > 
> > In v4:
> >    * Got rid of the changes in the general planes max scale code;
> >    * Added a couple of FIXMEs;
> >    * Made intel_atomic_setup_scaler() return an int with errors;
> > 
> > In v5:
> >    * Just resent with a cover letter.
> > 
> > In v6:
> >    * Now the correct version again (same as v4).
> > 
> > 
> > drivers/gpu/drm/i915/display/intel_atomic.c | 83 ++++++++++++++++++---
> >  1 file changed, 73 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> > index 6621aa245caf..8373be283d8b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > @@ -41,6 +41,7 @@
> >  #include "intel_global_state.h"
> >  #include "intel_hdcp.h"
> >  #include "intel_psr.h"
> > +#include "intel_fb.h"
> >  #include "skl_universal_plane.h"
> >  
> >  /**
> > @@ -310,11 +311,11 @@ intel_crtc_destroy_state(struct drm_crtc *crtc,
> >  	kfree(crtc_state);
> >  }
> >  
> > -static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_state,
> > -				      int num_scalers_need, struct intel_crtc *intel_crtc,
> > -				      const char *name, int idx,
> > -				      struct intel_plane_state *plane_state,
> > -				      int *scaler_id)
> > +static int intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_state,
> > +				     int num_scalers_need, struct intel_crtc *intel_crtc,
> > +				     const char *name, int idx,
> > +				     struct intel_plane_state *plane_state,
> > +				     int *scaler_id)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
> >  	int j;
> > @@ -334,7 +335,7 @@ static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_sta
> >  
> >  	if (drm_WARN(&dev_priv->drm, *scaler_id < 0,
> >  		     "Cannot find scaler for %s:%d\n", name, idx))
> > -		return;
> > +		return -EBUSY;
> >  
> >  	/* set scaler mode */
> >  	if (plane_state && plane_state->hw.fb &&
> > @@ -375,9 +376,69 @@ static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_sta
> >  		mode = SKL_PS_SCALER_MODE_DYN;
> >  	}
> >  
> > +	/*
> > +	 * FIXME: we should also check the scaler factors for pfit, so
> > +	 * this shouldn't be tied directly to planes.
> > +	 */
> > +	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;
> 
> Those can be const.

Done.


> > +		int hscale, vscale, max_vscale, max_hscale;
> > +
> > +		/*
> > +		 * FIXME: When two scalers are needed, but only one of
> > +		 * them needs to downscale, we should make sure that
> > +		 * the one that needs downscaling support is assigned
> > +		 * as the first scaler, so we don't reject downscaling
> > +		 * unnecessarily.
> > +		 */
> > +
> > +		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;
> > +		}
> 
> We'd want something along these lines if we want to handle 
> the hq vs. dyn scaler stuff correctly.
> 
> if (DISPLAY_VER(dev_priv) >= 14) {
> 	...
> } else if (DISPLAY_VER(dev_priv) >= 10)
> 	max_hscale = 0x30000 - 1;
> 	max_vscale = 0x30000 - 1;
> } else if (mode == NV12) {
> 	max_hscale = 0x20000 - 1;
> 	max_vscale = 0x20000 - 1;
> } else if (mode == HQ || src_w <= 2048) {
> 	max_hscale = 0x30000 - 1;
> 	max_vscale = 0x30000 - 1;
> } else {
> 	max_hscale = 0x30000 - 1;
> 	max_vscale = 0x20000 - 1;
> }
> 
> Though we could leave that for a followup patch, in which
> case perhaps add a FIXME.

I added another FIXME.


> > +
> > +		/* 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;
> 
> There should be no need to undo stuff like this.

Right.  This is reminiscent from the previous version of the patch,
where we were not returning errors.  I removed this now.


> > +
> > +			return -EOPNOTSUPP;
> 
> We typically just go with -EINVAL for pretty much everything.
> Given the number of things that can go wrong no one can realistically
> figure out what happened/how to resolve it based on the errno alone
> anyway.

Okay.  My view is that by having different error codes we can at least
have a bit more clue and make it easier to track down.  I'll change it
to -EINVAL, and the -EBUSY I added earlier to -EINVAL as well.

Thanks for the review.

--
Cheers,
Luca.
> 

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

[Coelho, Luciano]

On Tue, 2022-12-20 at 23:10 +0200, Ville Syrjälä wrote:
> On Tue, Dec 20, 2022 at 02:07:24PM +0200, Luca Coelho wrote:
> > 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>
> > ---
> >  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;
> > +	}
> 
> Bspec says max_src_w=4096, max_src_h=8192, max_dst_w=8192,
> max_dst_h=8192.

Yes, thanks for pointing out! I heard that these values were changed in
the bspec after the original implementation was made internally.  I
have updated the patch accordingly now.

--
Cheers,
Luca.