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

On Sat, 28 Mar 2026, Michał Grzelak wrote:
> 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?

Nevermind the question, apparently I crossed my eyes when looking at
cscope's search. *facepalm*.

BR,
Michał

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