Re: [PATCH 3/9] drm/i915/casf: Move the casf state to better place

["Michał Grzelak" <michal.grzelak@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 11347 bytes --]

On Thu, 26 Mar 2026, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> The casf state is placed inside the 'hw' state for some reason.
> That is only really meant for things we have to duplicate from
> the uapi state. The rest can live on its own in our actual state.
>
> And since casf is just one aspect of the pfit/pipe scaler the
> proper place for it seems to be under pch_pfit.
>
> Cc: Nemesa Garg <nemesa.garg@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_casf.c     | 40 +++++++++----------
> .../drm/i915/display/intel_crtc_state_dump.c  |  6 +--
> drivers/gpu/drm/i915/display/intel_display.c  | 12 +++---
> .../drm/i915/display/intel_display_types.h    |  2 +-
> drivers/gpu/drm/i915/display/skl_scaler.c     |  4 +-
> 5 files changed, 32 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_casf.c b/drivers/gpu/drm/i915/display/intel_casf.c
> index 4316d8214e80..5a8ffb40d30d 100644
> --- a/drivers/gpu/drm/i915/display/intel_casf.c
> +++ b/drivers/gpu/drm/i915/display/intel_casf.c
> @@ -82,7 +82,7 @@ void intel_casf_update_strength(struct intel_crtc_state *crtc_state)
> 	int win_size;
>
> 	intel_de_rmw(display, SHARPNESS_CTL(crtc->pipe), FILTER_STRENGTH_MASK,
> -		     FILTER_STRENGTH(crtc_state->hw.casf_params.strength));
> +		     FILTER_STRENGTH(crtc_state->pch_pfit.casf.strength));
>
> 	win_size = intel_de_read(display, SKL_PS_WIN_SZ(crtc->pipe, 1));
>
> @@ -95,11 +95,11 @@ static void intel_casf_compute_win_size(struct intel_crtc_state *crtc_state)
> 	u32 total_pixels = mode->hdisplay * mode->vdisplay;
>
> 	if (total_pixels <= MAX_PIXELS_FOR_3_TAP_FILTER)
> -		crtc_state->hw.casf_params.win_size = SHARPNESS_FILTER_SIZE_3X3;
> +		crtc_state->pch_pfit.casf.win_size = SHARPNESS_FILTER_SIZE_3X3;
> 	else if (total_pixels <= MAX_PIXELS_FOR_5_TAP_FILTER)
> -		crtc_state->hw.casf_params.win_size = SHARPNESS_FILTER_SIZE_5X5;
> +		crtc_state->pch_pfit.casf.win_size = SHARPNESS_FILTER_SIZE_5X5;
> 	else
> -		crtc_state->hw.casf_params.win_size = SHARPNESS_FILTER_SIZE_7X7;
> +		crtc_state->pch_pfit.casf.win_size = SHARPNESS_FILTER_SIZE_7X7;
> }
>
> int intel_casf_compute_config(struct intel_crtc_state *crtc_state)
> @@ -110,8 +110,8 @@ int intel_casf_compute_config(struct intel_crtc_state *crtc_state)
> 		return 0;
>
> 	if (crtc_state->hw.sharpness_strength == 0) {
> -		crtc_state->hw.casf_params.enable = false;
> -		crtc_state->hw.casf_params.strength = 0;
> +		crtc_state->pch_pfit.casf.enable = false;
> +		crtc_state->pch_pfit.casf.strength = 0;
> 		return 0;
> 	}
>
> @@ -121,7 +121,7 @@ int intel_casf_compute_config(struct intel_crtc_state *crtc_state)
> 		return -EINVAL;
> 	}
>
> -	crtc_state->hw.casf_params.enable = true;
> +	crtc_state->pch_pfit.casf.enable = true;
>
> 	/*
> 	 * HW takes a value in form (1.0 + strength) in 4.4 fixed format.
> @@ -131,7 +131,7 @@ int intel_casf_compute_config(struct intel_crtc_state *crtc_state)
> 	 * 6.3125 in 4.4 format is b01100101 which is equal to 101.
> 	 * Also 85 + 16 = 101.
> 	 */
> -	crtc_state->hw.casf_params.strength =
> +	crtc_state->pch_pfit.casf.strength =
> 		min(crtc_state->hw.sharpness_strength, 0xEF) + 0x10;
>
> 	intel_casf_compute_win_size(crtc_state);
> @@ -151,19 +151,19 @@ void intel_casf_sharpness_get_config(struct intel_crtc_state *crtc_state)
> 	if (sharp & FILTER_EN) {
> 		if (drm_WARN_ON(display->drm,
> 				REG_FIELD_GET(FILTER_STRENGTH_MASK, sharp) < 16))
> -			crtc_state->hw.casf_params.strength = 0;
> +			crtc_state->pch_pfit.casf.strength = 0;
> 		else
> -			crtc_state->hw.casf_params.strength =
> +			crtc_state->pch_pfit.casf.strength =
> 				REG_FIELD_GET(FILTER_STRENGTH_MASK, sharp);
> -		crtc_state->hw.casf_params.enable = true;
> -		crtc_state->hw.casf_params.win_size =
> +		crtc_state->pch_pfit.casf.enable = true;
> +		crtc_state->pch_pfit.casf.win_size =
> 			REG_FIELD_GET(FILTER_SIZE_MASK, sharp);
> 	}
> }
>
> bool intel_casf_needs_scaler(const struct intel_crtc_state *crtc_state)
> {
> -	if (crtc_state->hw.casf_params.enable)
> +	if (crtc_state->pch_pfit.casf.enable)
> 		return true;
>
> 	return false;
> @@ -179,7 +179,7 @@ static u32 casf_coeff(struct intel_crtc_state *crtc_state, int t)
> 	struct scaler_filter_coeff value;
> 	u32 coeff;
>
> -	value = crtc_state->hw.casf_params.coeff[t];
> +	value = crtc_state->pch_pfit.casf.coeff[t];

Unrelated topic: how you see such renaming?:

int t -> int tap

> 	value.sign = 0;
>
> 	coeff = value.sign << 15 | value.exp << 12 | value.mantissa << 3;
> @@ -189,7 +189,7 @@ static u32 casf_coeff(struct intel_crtc_state *crtc_state, int t)
> /*
>  * 17 phase of 7 taps requires 119 coefficients in 60 dwords per set.
>  * To enable casf:  program scaler coefficients with the coeffients
> - * that are calculated and stored in hw.casf_params.coeff as per
> + * that are calculated and stored in pch_pfit.casf.coeff as per
>  * SCALER_COEFFICIENT_FORMAT
>  */
> static void intel_casf_write_coeff(struct intel_crtc_state *crtc_state)
> @@ -247,9 +247,9 @@ void intel_casf_scaler_compute_config(struct intel_crtc_state *crtc_state)
> 	u16 sumcoeff = 0;

Unrelated to the change: some variables have been named as sumcoeff and
others as filter_*coeff{_1,_2,_3}*. How you see renaming:

u16 sumcoeff -> u16 sum_coeff

But I might be again missing common codestyle.

Reviewed-by: Michał Grzelak <michal.grzelak@intel.com>

BR,
Michał

> 	int i;
>
> -	if (crtc_state->hw.casf_params.win_size == 0)
> +	if (crtc_state->pch_pfit.casf.win_size == 0)
> 		filtercoeff = filtercoeff_1;
> -	else if (crtc_state->hw.casf_params.win_size == 1)
> +	else if (crtc_state->pch_pfit.casf.win_size == 1)
> 		filtercoeff = filtercoeff_2;
> 	else
> 		filtercoeff = filtercoeff_3;
> @@ -259,7 +259,7 @@ void intel_casf_scaler_compute_config(struct intel_crtc_state *crtc_state)
>
> 	for (i = 0; i < SCALER_FILTER_NUM_TAPS; i++) {
> 		filter_coeff[i] = (*(filtercoeff + i) * 100 / sumcoeff);
> -		convert_sharpness_coef_binary(&crtc_state->hw.casf_params.coeff[i],
> +		convert_sharpness_coef_binary(&crtc_state->pch_pfit.casf.coeff[i],
> 					      filter_coeff[i]);
> 	}
> }
> @@ -274,9 +274,9 @@ void intel_casf_enable(struct intel_crtc_state *crtc_state)
>
> 	intel_casf_write_coeff(crtc_state);
>
> -	sharpness_ctl = FILTER_EN | FILTER_STRENGTH(crtc_state->hw.casf_params.strength);
> +	sharpness_ctl = FILTER_EN | FILTER_STRENGTH(crtc_state->pch_pfit.casf.strength);
>
> -	sharpness_ctl |= crtc_state->hw.casf_params.win_size;
> +	sharpness_ctl |= crtc_state->pch_pfit.casf.win_size;
>
> 	intel_de_write(display, SHARPNESS_CTL(crtc->pipe), sharpness_ctl);
>
> diff --git a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> index 10a9b51337fa..95993f8e5d0d 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> +++ b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> @@ -382,9 +382,9 @@ void intel_crtc_state_dump(const struct intel_crtc_state *pipe_config,
> 	intel_vdsc_state_dump(&p, 0, pipe_config);
>
> 	drm_printf(&p, "sharpness strength: %d, sharpness tap size: %d, sharpness enable: %d\n",
> -		   pipe_config->hw.casf_params.strength,
> -		   pipe_config->hw.casf_params.win_size,
> -		   pipe_config->hw.casf_params.enable);
> +		   pipe_config->pch_pfit.casf.strength,
> +		   pipe_config->pch_pfit.casf.win_size,
> +		   pipe_config->pch_pfit.casf.enable);
>
> dump_planes:
> 	if (!state)
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index a02c58b5a34d..e02e69467871 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -994,7 +994,7 @@ static bool intel_casf_enabling(const struct intel_crtc_state *new_crtc_state,
> 	if (!new_crtc_state->hw.active)
> 		return false;
>
> -	return is_enabling(hw.casf_params.enable, old_crtc_state, new_crtc_state);
> +	return is_enabling(pch_pfit.casf.enable, old_crtc_state, new_crtc_state);
> }
>
> static bool intel_casf_disabling(const struct intel_crtc_state *old_crtc_state,
> @@ -1003,7 +1003,7 @@ static bool intel_casf_disabling(const struct intel_crtc_state *old_crtc_state,
> 	if (!new_crtc_state->hw.active)
> 		return false;
>
> -	return is_disabling(hw.casf_params.enable, old_crtc_state, new_crtc_state);
> +	return is_disabling(pch_pfit.casf.enable, old_crtc_state, new_crtc_state);
> }
>
> static bool intel_crtc_lobf_enabling(const struct intel_crtc_state *old_crtc_state,
> @@ -5370,12 +5370,12 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>
> 		PIPE_CONF_CHECK_BOOL(pch_pfit.enabled);
> 		PIPE_CONF_CHECK_RECT(pch_pfit.dst);
> +		PIPE_CONF_CHECK_BOOL(pch_pfit.casf.enable);
> +		PIPE_CONF_CHECK_I(pch_pfit.casf.win_size);
> +		PIPE_CONF_CHECK_I(pch_pfit.casf.strength);
>
> 		PIPE_CONF_CHECK_I(scaler_state.scaler_id);
> 		PIPE_CONF_CHECK_I(pixel_rate);
> -		PIPE_CONF_CHECK_BOOL(hw.casf_params.enable);
> -		PIPE_CONF_CHECK_I(hw.casf_params.win_size);
> -		PIPE_CONF_CHECK_I(hw.casf_params.strength);
>
> 		PIPE_CONF_CHECK_X(gamma_mode);
> 		if (display->platform.cherryview)
> @@ -6819,7 +6819,7 @@ static void intel_pre_update_crtc(struct intel_atomic_state *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)
> +	else if (new_crtc_state->pch_pfit.casf.strength != old_crtc_state->pch_pfit.casf.strength)
> 		intel_casf_update_strength(new_crtc_state);
>
> 	intel_fbc_update(state, crtc);
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index af0d870de342..ca2581fb7bbd 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1037,7 +1037,6 @@ struct intel_crtc_state {
> 		struct drm_display_mode mode, pipe_mode, adjusted_mode;
> 		enum drm_scaling_filter scaling_filter;
> 		u8 sharpness_strength;
> -		struct intel_casf casf_params;
> 	} hw;
>
> 	/* actual state of LUTs */
> @@ -1224,6 +1223,7 @@ struct intel_crtc_state {
>
> 	/* Panel fitter placement and size for Ironlake+ */
> 	struct {
> +		struct intel_casf casf;
> 		struct drm_rect dst;
> 		bool enabled;
> 		bool force_thru;
> diff --git a/drivers/gpu/drm/i915/display/skl_scaler.c b/drivers/gpu/drm/i915/display/skl_scaler.c
> index cfa17ddb4018..e9fe5c0bf6ff 100644
> --- a/drivers/gpu/drm/i915/display/skl_scaler.c
> +++ b/drivers/gpu/drm/i915/display/skl_scaler.c
> @@ -986,13 +986,13 @@ void skl_scaler_get_config(struct intel_crtc_state *crtc_state)
> 		if (HAS_CASF(display) && id == 1)
> 			intel_casf_sharpness_get_config(crtc_state);
>
> -		if (!crtc_state->hw.casf_params.enable)
> +		if (!crtc_state->pch_pfit.casf.enable)
> 			crtc_state->pch_pfit.enabled = true;
>
> 		pos = intel_de_read(display, SKL_PS_WIN_POS(crtc->pipe, i));
> 		size = intel_de_read(display, SKL_PS_WIN_SZ(crtc->pipe, i));
>
> -		if (!crtc_state->hw.casf_params.enable)
> +		if (!crtc_state->pch_pfit.casf.enable)
> 			drm_rect_init(&crtc_state->pch_pfit.dst,
> 				      REG_FIELD_GET(PS_WIN_XPOS_MASK, pos),
> 				      REG_FIELD_GET(PS_WIN_YPOS_MASK, pos),
> -- 
> 2.52.0
>
>