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

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

On Fri, Mar 27, 2026 at 10:10:26AM +0100, Michał Grzelak wrote:
> 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

Probably a good idea. One character variable names aren't what
you'd call descriptive. This probably got copied from the
nearest neighbor filter code, so a rename should happen there too.

Or even better if someone gets rid of the duplicated code entirely
and consolidates all the filter coefficient code to one place.

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

We generally use the underscore_style for things. So I wouldn't
mind the rename.

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

Ta.

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


-- 
Ville Syrjälä
Intel