Re: [PATCH v2 09/10] drm/i915/scaler: abstract scaler searching loop

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Sat, Apr 11, 2026 at 07:45:25PM +0200, Michał Grzelak wrote:
> Add a helper function hiding the search for scaler_id.
> 
> Cc: Nemesa Garg <nemesa.garg@intel.com>
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Michał Grzelak <michal.grzelak@intel.com>
> ---
>  drivers/gpu/drm/i915/display/skl_scaler.c | 29 ++++++++++++++---------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/skl_scaler.c b/drivers/gpu/drm/i915/display/skl_scaler.c
> index 4e2f4c4ffc45a..1f47c5fc1802f 100644
> --- a/drivers/gpu/drm/i915/display/skl_scaler.c
> +++ b/drivers/gpu/drm/i915/display/skl_scaler.c
> @@ -836,6 +836,21 @@ void skl_pfit_enable(const struct intel_crtc_state *crtc_state)
>  			  PS_WIN_XSIZE(width) | PS_WIN_YSIZE(height));
>  }
>  
> +static int skl_pipe_scaler_get_hw_state(struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_display *display = to_intel_display(crtc_state);
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +	u32 ctl;

'ctl' could stay inside the loop.

I suppose technically 'display' could go there as well, but
we've established the convention that 'display' (if needed inside
the function) is the first variable declared. So that one should
stay here.

> +
> +	for (int scaler_id = 0; scaler_id < crtc->num_scalers; scaler_id++) {
> +		ctl = intel_de_read(display, SKL_PS_CTRL(crtc->pipe, scaler_id));
> +		if ((ctl & (PS_SCALER_EN | PS_BINDING_MASK)) == (PS_SCALER_EN | PS_BINDING_PIPE))
> +			return scaler_id;
> +	}
> +
> +	return -1;
> +}
> +
>  void
>  skl_program_plane_scaler(struct intel_dsb *dsb,
>  			 struct intel_plane *plane,
> @@ -950,19 +965,11 @@ void skl_scaler_get_config(struct intel_crtc_state *crtc_state)
>  	struct intel_display *display = to_intel_display(crtc_state);
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	struct intel_crtc_scaler_state *scaler_state = &crtc_state->scaler_state;
> -	int scaler_id;
> -	u32 pos, size;
> -
>  	/* find scaler attached to this pipe */
> -	for (scaler_id = 0; scaler_id < crtc->num_scalers; scaler_id++) {
> -		u32 ctl;
> -
> -		ctl = intel_de_read(display, SKL_PS_CTRL(crtc->pipe, scaler_id));
> -		if ((ctl & (PS_SCALER_EN | PS_BINDING_MASK)) == (PS_SCALER_EN | PS_BINDING_PIPE))
> -			break;
> -	}
> +	int scaler_id = skl_pipe_scaler_get_hw_state(crtc_state);

I don't like hiding stuff with potential side effects
inside the variable declaration block. Only pure
functions should be called there. So it's better to
do the function call+assignment just before the <0 check.

Apart from that it all looks good to me. So with that adjusted
the series is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

But looks like you forgot to cc intel-xe@. We do want xe CI results
before merging...

> +	u32 pos, size;
>  
> -	if (scaler_id == crtc->num_scalers)
> +	if (scaler_id < 0)
>  		return;
>  
>  	if (scaler_has_casf(display, scaler_id))
> -- 
> 2.45.2

-- 
Ville Syrjälä
Intel