Re: [PATCH 3/3] drm/i915/display: Common wrapper for casf and pfit

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

On 12/23/2025 1:43 PM, Nemesa Garg wrote:
> Make a common wrapper for pipe_scaling and sharpness
> which will be invoked from hsw_crtc_enable and
> pipe_fastset. Then accoridng to the condition
s/accoridng/according
> pfit_enable or casf_enable or casf_update_strength
> can be invoked. This is done so that all pipe
> related functions can be under one umberalla.

You mean pipe scaler related functions?


>
> Signed-off-by: Nemesa Garg <nemesa.garg@intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_display.c | 24 ++++------------
>   drivers/gpu/drm/i915/display/skl_scaler.c    | 29 ++++++++++++++++++++
>   drivers/gpu/drm/i915/display/skl_scaler.h    |  3 ++
>   3 files changed, 37 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 568730ffe359..ddab4a1417ec 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -991,15 +991,6 @@ static bool audio_disabling(const struct intel_crtc_state *old_crtc_state,
>   		 memcmp(old_crtc_state->eld, new_crtc_state->eld, MAX_ELD_BYTES) != 0);
>   }
>   
> -static bool intel_casf_enabling(const struct intel_crtc_state *new_crtc_state,
> -				const struct intel_crtc_state *old_crtc_state)
> -{
> -	if (!new_crtc_state->hw.active)
> -		return false;
> -
> -	return is_enabling(hw.casf_params.casf_enable, old_crtc_state, new_crtc_state);
> -}
> -
>   static bool intel_casf_disabling(const struct intel_crtc_state *old_crtc_state,
>   				 const struct intel_crtc_state *new_crtc_state)
>   {
> @@ -1679,7 +1670,7 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
>   			glk_pipe_scaler_clock_gating_wa(pipe_crtc, true);
>   
>   		if (DISPLAY_VER(display) >= 9)
> -			skl_pfit_enable(pipe_crtc_state);
> +			skl_scaler_enable(state, crtc);
>   		else
>   			ilk_pfit_enable(pipe_crtc_state);
>   
> @@ -6635,7 +6626,8 @@ void intel_crtc_arm_fifo_underrun(struct intel_crtc *crtc,
>   	}
>   }
>   
> -static void intel_pipe_fastset(const struct intel_crtc_state *old_crtc_state,
> +static void intel_pipe_fastset(struct intel_atomic_state *state,
> +		const struct intel_crtc_state *old_crtc_state,
>   			       const struct intel_crtc_state *new_crtc_state)
>   {
>   	struct intel_display *display = to_intel_display(new_crtc_state);
> @@ -6653,8 +6645,7 @@ static void intel_pipe_fastset(const struct intel_crtc_state *old_crtc_state,
>   
>   	/* on skylake this is done by detaching scalers */
>   	if (DISPLAY_VER(display) >= 9) {
> -		if (new_crtc_state->pch_pfit.enabled)
> -			skl_pfit_enable(new_crtc_state);
> +			skl_scaler_enable(state, crtc);
>   	} else if (HAS_PCH_SPLIT(display)) {
>   		if (new_crtc_state->pch_pfit.enabled)
>   			ilk_pfit_enable(new_crtc_state);
> @@ -6706,7 +6697,7 @@ static void commit_pipe_pre_planes(struct intel_atomic_state *state,
>   			bdw_set_pipe_misc(NULL, new_crtc_state);
>   
>   		if (intel_crtc_needs_fastset(new_crtc_state))
> -			intel_pipe_fastset(old_crtc_state, new_crtc_state);
> +			intel_pipe_fastset(state, old_crtc_state, new_crtc_state);
>   	}
>   
>   	intel_psr2_program_trans_man_trk_ctl(NULL, new_crtc_state);
> @@ -6805,11 +6796,6 @@ static void intel_pre_update_crtc(struct intel_atomic_state *state,
>   			intel_vrr_set_transcoder_timings(new_crtc_state);
>   	}
>   
> -	if (intel_casf_enabling(new_crtc_state, old_crtc_state))
> -		intel_casf_enable(new_crtc_state);
> -	else if (new_crtc_state->hw.casf_params.strength != old_crtc_state->hw.casf_params.strength)
> -		intel_casf_update_strength(new_crtc_state);
> -
>   	intel_fbc_update(state, crtc);
>   
>   	drm_WARN_ON(display->drm, !intel_display_power_is_enabled(display, POWER_DOMAIN_DC_OFF));
> diff --git a/drivers/gpu/drm/i915/display/skl_scaler.c b/drivers/gpu/drm/i915/display/skl_scaler.c
> index abd951f7dd71..19a9c4a0da0a 100644
> --- a/drivers/gpu/drm/i915/display/skl_scaler.c
> +++ b/drivers/gpu/drm/i915/display/skl_scaler.c
> @@ -752,6 +752,19 @@ static void skl_scaler_setup_filter(struct intel_display *display,
>   	}
>   }
>   
> +#define is_enabling(feature, old_crtc_state, new_crtc_state) \
> +        ((!(old_crtc_state)->feature || intel_crtc_needs_modeset(new_crtc_state)) && \
> +         (new_crtc_state)->feature)

I think if this is indeed needed this should be placed in 
intel_display.h instead of having a copy here.

> +
> +static bool intel_casf_enabling(const struct intel_crtc_state *new_crtc_state,
> +                                const struct intel_crtc_state *old_crtc_state)

This doesn't seem to belong to this file.


> +{
> +        if (!new_crtc_state->hw.active)
> +                return false;
> +
> +        return is_enabling(hw.casf_params.casf_enable, old_crtc_state, new_crtc_state);
> +}
> +
>   #define CASF_SCALER_FILTER_SELECT \
>   	(PS_FILTER_PROGRAMMED | \
>   	PS_Y_VERT_FILTER_SELECT(0) | \
> @@ -859,6 +872,22 @@ void skl_pipe_scaler_setup(const struct intel_crtc_state *crtc_state,
>   			  PS_WIN_XPOS(x) | PS_WIN_YPOS(y));
>   }
>   
> +void skl_scaler_enable(struct intel_atomic_state *state,
> +                       struct intel_crtc *crtc)
> +{
> +        struct intel_crtc_state *new_crtc_state =
> +                intel_atomic_get_new_crtc_state(state, crtc);
> +        struct intel_crtc_state *old_crtc_state =
> +                intel_atomic_get_old_crtc_state(state, crtc);
> +
> +       if (new_crtc_state->pch_pfit.enabled)
> +               skl_pfit_enable(new_crtc_state);

The function `skl_pfit_enable()` was called from intel_display.c 
earlier. Since this is not used outside this file this should be a 
static function.


Regards,

Ankit

> +       else if (intel_casf_enabling(new_crtc_state, old_crtc_state))
> +                intel_casf_enable(new_crtc_state);
> +       else if (new_crtc_state->hw.casf_params.strength != old_crtc_state->hw.casf_params.strength)
> +                intel_casf_update_strength(new_crtc_state);
> +}
> +
>   void
>   skl_program_plane_scaler(struct intel_dsb *dsb,
>   			 struct intel_plane *plane,
> diff --git a/drivers/gpu/drm/i915/display/skl_scaler.h b/drivers/gpu/drm/i915/display/skl_scaler.h
> index 94bde5d1c06a..3d49836334b7 100644
> --- a/drivers/gpu/drm/i915/display/skl_scaler.h
> +++ b/drivers/gpu/drm/i915/display/skl_scaler.h
> @@ -40,6 +40,9 @@ void skl_scaler_get_config(struct intel_crtc_state *crtc_state);
>   
>   void skl_scaler_setup_casf(struct intel_crtc_state *crtc_state);
>   
> +void skl_scaler_enable(struct intel_atomic_state *state,
> +                       struct intel_crtc *crtc);
> +
>   enum drm_mode_status
>   skl_scaler_mode_valid(struct intel_display *display,
>   		      const struct drm_display_mode *mode,