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: 12987 bytes --]

On Fri, 27 Mar 2026, Ville Syrjälä wrote:
> 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.

I'd give it a shot but I genuinely don't know where to search for
nearest neighbor filter. I searched for possible declarations of int t,
but it didn't show anything relevant. Can you point to where should I
start looking?

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

In that case I should probably rename filtercoeff as well. But since
filter_coeff is already used and global, I guess it would require more
vigilance then the simple rename above. Do you have any hints on how to
handle it without breaking everything?

BR,
Michał

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