Re: [PATCH v5 3/7] drm/i915/scaler: Refactor max_scale computation

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 1/15/2025 8:47 PM, Mitul Golani wrote:
> Refactor max scaling factor computation into a reusable
> function for scalers.
>
> --v2:
> - Add missing comment. [Ankit]
>
> Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>

LGTM.

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>


> ---
>   drivers/gpu/drm/i915/display/skl_scaler.c | 70 +++++++++++++----------
>   1 file changed, 41 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/skl_scaler.c b/drivers/gpu/drm/i915/display/skl_scaler.c
> index 12efa4008027..a832cdccd9fa 100644
> --- a/drivers/gpu/drm/i915/display/skl_scaler.c
> +++ b/drivers/gpu/drm/i915/display/skl_scaler.c
> @@ -319,6 +319,44 @@ static int intel_allocate_scaler(struct intel_crtc_scaler_state *scaler_state,
>   	return -1;
>   }
>   
> +static void
> +calculate_max_scale(struct intel_crtc *crtc,
> +		    bool is_yuv_semiplanar,
> +		    int scaler_id,
> +		    int *max_hscale, int *max_vscale)
> +{
> +	struct intel_display *display = to_intel_display(crtc);
> +
> +	/*
> +	 * 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(display) >= 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(display) >= 10 || !is_yuv_semiplanar) {
> +		*max_hscale = 0x30000 - 1;
> +		*max_vscale = 0x30000 - 1;
> +	} else {
> +		*max_hscale = 0x20000 - 1;
> +		*max_vscale = 0x20000 - 1;
> +	}
> +}
> +
>   static int intel_atomic_setup_scaler(struct intel_crtc_state *crtc_state,
>   				     int num_scalers_need, struct intel_crtc *crtc,
>   				     const char *name, int idx,
> @@ -388,35 +426,9 @@ static int intel_atomic_setup_scaler(struct intel_crtc_state *crtc_state,
>   		const 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(display) >= 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(display) >= 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;
> -		}
> +		calculate_max_scale(crtc,
> +				    intel_format_info_is_yuv_semiplanar(fb->format, fb->modifier),
> +				    *scaler_id, &max_hscale, &max_vscale);
>   
>   		/*
>   		 * FIXME: We should change the if-else block above to