Re: [PATCH 2/3] drm/i915/display: Introduce skl_pipe_scaler_setup()

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

On 12/23/2025 1:42 PM, Nemesa Garg wrote:
> As skl_pfit_enable and skl_scaler_setup_casf
> have similar logic for pipe scaler registers
> so to avoid duplicacy introduce new helper

Perhaps 'duplication' will be apt.


> skl_pipe_scaler_setup. This helper consolidates
> common scaler setup steps and is now called
> from both skl_pfit_enable() and skl_scaler_setup_casf().

As suggested in previous patch, lets use ~75 character limit.


>
> Signed-off-by: Nemesa Garg <nemesa.garg@intel.com>
> ---
>   drivers/gpu/drm/i915/display/skl_scaler.c | 67 ++++++++++++-----------
>   drivers/gpu/drm/i915/display/skl_scaler.h |  2 +
>   2 files changed, 36 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/skl_scaler.c b/drivers/gpu/drm/i915/display/skl_scaler.c
> index 4c4deac7f9c8..abd951f7dd71 100644
> --- a/drivers/gpu/drm/i915/display/skl_scaler.c
> +++ b/drivers/gpu/drm/i915/display/skl_scaler.c
> @@ -761,41 +761,25 @@ static void skl_scaler_setup_filter(struct intel_display *display,
>   
>   void skl_scaler_setup_casf(struct intel_crtc_state *crtc_state)
>   {
> -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> -	struct intel_display *display = to_intel_display(crtc);
> +	struct intel_display *display = to_intel_display(crtc_state);

I think we can retain the order in the new function as the previous one, 
there is no change required here.


> +        struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);

Extra space before the line. In fact, I can see a lot of styling issues 
caught by the checkpatch. Please fix those.


> +	const struct intel_crtc_scaler_state *scaler_state =
> +                &crtc_state->scaler_state;
>   	struct drm_display_mode *adjusted_mode =
> -	&crtc_state->hw.adjusted_mode;
> -	struct intel_crtc_scaler_state *scaler_state =
> -		&crtc_state->scaler_state;
> -	struct drm_rect src, dest;
> -	int id, width, height;
> -	int x = 0, y = 0;
> +		&crtc_state->hw.adjusted_mode;
>   	enum pipe pipe = crtc->pipe;
> -	u32 ps_ctrl;
> +	int width, height, x = 0, y = 0;
> +	int id;
>   
>   	width = adjusted_mode->crtc_hdisplay;
>   	height = adjusted_mode->crtc_vdisplay;
>   
> -	drm_rect_init(&dest, x, y, width, height);
> -
> -	width = drm_rect_width(&dest);
> -	height = drm_rect_height(&dest);
>   	id = scaler_state->scaler_id;
>   
> -	drm_rect_init(&src, 0, 0,
> -		      drm_rect_width(&crtc_state->pipe_src) << 16,
> -		      drm_rect_height(&crtc_state->pipe_src) << 16);
> +	skl_pipe_scaler_setup(crtc_state, width, height, x, y);
>   
> -	trace_intel_pipe_scaler_update_arm(crtc, id, x, y, width, height);
> -
> -	ps_ctrl = PS_SCALER_EN | PS_BINDING_PIPE | scaler_state->scalers[id].mode |
> -		  CASF_SCALER_FILTER_SELECT;
> -
> -	intel_de_write_fw(display, SKL_PS_CTRL(pipe, id), ps_ctrl);
> -	intel_de_write_fw(display, SKL_PS_WIN_POS(pipe, id),
> -			  PS_WIN_XPOS(x) | PS_WIN_YPOS(y));
>   	intel_de_write_fw(display, SKL_PS_WIN_SZ(pipe, id),
> -			  PS_WIN_XSIZE(width) | PS_WIN_YSIZE(height));
> +                          PS_WIN_XSIZE(width) | PS_WIN_YSIZE(height));
>   }
>   
>   void skl_pfit_enable(const struct intel_crtc_state *crtc_state)
> @@ -814,7 +798,6 @@ void skl_pfit_enable(const struct intel_crtc_state *crtc_state)
>   	int hscale, vscale;
>   	struct drm_rect src;
>   	int id;
> -	u32 ps_ctrl;
>   
>   	if (!crtc_state->pch_pfit.enabled)
>   		return;
> @@ -836,10 +819,34 @@ void skl_pfit_enable(const struct intel_crtc_state *crtc_state)
>   	uv_rgb_hphase = skl_scaler_calc_phase(1, hscale, false);
>   	uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false);
>   
> +	skl_pipe_scaler_setup(crtc_state, width, height, x, y);
> +
> +	id = scaler_state->scaler_id;
> +
> +	intel_de_write_fw(display, SKL_PS_VPHASE(pipe, id),
> +			  PS_Y_PHASE(0) | PS_UV_RGB_PHASE(uv_rgb_vphase));
> +	intel_de_write_fw(display, SKL_PS_HPHASE(pipe, id),
> +			  PS_Y_PHASE(0) | PS_UV_RGB_PHASE(uv_rgb_hphase));
> +	intel_de_write_fw(display, SKL_PS_WIN_SZ(pipe, id),
> +			  PS_WIN_XSIZE(width) | PS_WIN_YSIZE(height));
> +}
> +
> +void skl_pipe_scaler_setup(const struct intel_crtc_state *crtc_state,
> +			   int width, int height, int x, int y)


Why is this exposed? I don't see this getting used outside of this file 
in this patch or the next patch.

This should be a static function. You should move this function just 
before the first caller.

> +{
> +	struct intel_display *display = to_intel_display(crtc_state);
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +	const struct intel_crtc_scaler_state *scaler_state =
> +		&crtc_state->scaler_state;
> +	enum pipe pipe = crtc->pipe;
> +	int id;
> +	u32 ps_ctrl;
> +
>   	id = scaler_state->scaler_id;
>   
>   	ps_ctrl = PS_SCALER_EN | PS_BINDING_PIPE | scaler_state->scalers[id].mode |
> -		skl_scaler_get_filter_select(crtc_state->hw.scaling_filter);
> +		  skl_scaler_get_filter_select(crtc_state->hw.scaling_filter) |
> +		  CASF_SCALER_FILTER_SELECT;


This seems incorrect. With this we are setting CASF SCALER FILTER SELECT 
for pfit case also, which we do not want.

We need to set this, only when the call is from skl_scaler_setup_casf.


Regards,

Ankit

>   
>   	trace_intel_pipe_scaler_update_arm(crtc, id, x, y, width, height);
>   
> @@ -848,14 +855,8 @@ void skl_pfit_enable(const struct intel_crtc_state *crtc_state)
>   
>   	intel_de_write_fw(display, SKL_PS_CTRL(pipe, id), ps_ctrl);
>   
> -	intel_de_write_fw(display, SKL_PS_VPHASE(pipe, id),
> -			  PS_Y_PHASE(0) | PS_UV_RGB_PHASE(uv_rgb_vphase));
> -	intel_de_write_fw(display, SKL_PS_HPHASE(pipe, id),
> -			  PS_Y_PHASE(0) | PS_UV_RGB_PHASE(uv_rgb_hphase));
>   	intel_de_write_fw(display, SKL_PS_WIN_POS(pipe, id),
>   			  PS_WIN_XPOS(x) | PS_WIN_YPOS(y));
> -	intel_de_write_fw(display, SKL_PS_WIN_SZ(pipe, id),
> -			  PS_WIN_XSIZE(width) | PS_WIN_YSIZE(height));
>   }
>   
>   void
> diff --git a/drivers/gpu/drm/i915/display/skl_scaler.h b/drivers/gpu/drm/i915/display/skl_scaler.h
> index 7e8d819c019d..94bde5d1c06a 100644
> --- a/drivers/gpu/drm/i915/display/skl_scaler.h
> +++ b/drivers/gpu/drm/i915/display/skl_scaler.h
> @@ -30,6 +30,8 @@ void skl_program_plane_scaler(struct intel_dsb *dsb,
>   			      struct intel_plane *plane,
>   			      const struct intel_crtc_state *crtc_state,
>   			      const struct intel_plane_state *plane_state);
> +void skl_pipe_scaler_setup(const struct intel_crtc_state *crtc_state,
> +			   int width, int height, int x, int y);
>   void skl_detach_scalers(struct intel_dsb *dsb,
>   			const struct intel_crtc_state *crtc_state);
>   void skl_scaler_disable(const struct intel_crtc_state *old_crtc_state);