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

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

On 1/8/2025 6:05 PM, Mitul Golani wrote:
> Refactor max scaling factor computation into a reusable
> function for scalers.
>
> Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> ---
>   drivers/gpu/drm/i915/display/skl_scaler.c | 63 ++++++++++++-----------
>   1 file changed, 34 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/skl_scaler.c b/drivers/gpu/drm/i915/display/skl_scaler.c
> index 4f838220f880..19ab7b38c64e 100644
> --- a/drivers/gpu/drm/i915/display/skl_scaler.c
> +++ b/drivers/gpu/drm/i915/display/skl_scaler.c
> @@ -309,6 +309,37 @@ 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) {
> +		*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,
> @@ -378,35 +409,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;

This part seems to be removed. I don't see anything mentioned in the 
commit message about this too.

If you are moving code to a new function, then it should be a simple 
move, without any functional change.

Regards,

Ankit

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