Re: [PATCH 4/9] drm/i915/casf: Extract scaler_has_casf()

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

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

On Fri, 27 Mar 2026, Ville Syrjälä wrote:
> On Fri, Mar 27, 2026 at 11:06:18AM +0100, Michał Grzelak wrote:
>> On Fri, 27 Mar 2026, Michał Grzelak wrote:
>>> On Thu, 26 Mar 2026, Ville Syrjala wrote:
>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>
>>>> Extract a small helper to determine if the scaler supports
>>>> the sharpness filter or not.
>>>>
>>>> Cc: Nemesa Garg <nemesa.garg@intel.com>
>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> ---
>>>> drivers/gpu/drm/i915/display/skl_scaler.c | 12 ++++++++----
>>>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/display/skl_scaler.c
>>>> b/drivers/gpu/drm/i915/display/skl_scaler.c
>>>> index e9fe5c0bf6ff..525afd736195 100644
>>>> --- a/drivers/gpu/drm/i915/display/skl_scaler.c
>>>> +++ b/drivers/gpu/drm/i915/display/skl_scaler.c
>>>> @@ -323,19 +323,24 @@ int skl_update_scaler_plane(struct intel_crtc_state
>>>> *crtc_state,
>>>> 				 need_scaler);
>>>> }
>>>>
>>>> +static bool scaler_has_casf(struct intel_display *display, int scaler_id)
>>>> +{
>>>> +	return HAS_CASF(display) && scaler_id == 1;
>>>> +}
>>>> +
>>>> static int intel_allocate_scaler(struct intel_crtc_scaler_state
>>>> *scaler_state,
>>>> 				 struct intel_crtc *crtc,
>>>> 				 struct intel_plane_state *plane_state,
>>>> 				 bool casf_scaler)
>>>> {
>>>> +	struct intel_display *display = to_intel_display(crtc);
>>>> 	int i;
>>>>
>>>> 	for (i = 0; i < crtc->num_scalers; i++) {
>>>> 		if (scaler_state->scalers[i].in_use)
>>>> 			continue;
>>>>
>>>> -		/* CASF needs second scaler */
>>>> -		if (!plane_state && casf_scaler && i != 1)
>>>> +		if (casf_scaler && !scaler_has_casf(display, i))
>>>> 			continue;
>>>>
>>>> 		scaler_state->scalers[i].in_use = true;
>>>> @@ -982,8 +987,7 @@ void skl_scaler_get_config(struct intel_crtc_state
>>>> *crtc_state)
>>>>
>>>> 		id = i;
>>>>
>>>> -		/* Read CASF regs for second scaler */
>>>> -		if (HAS_CASF(display) && id == 1)
>>>> +		if (scaler_has_casf(display, i))
>>>
>>> With that being changed I am wondering if we need int id at all. The
>>> only user of it is outside the loop. Since id is set to i on every
>>> loop's pass, I guess we can replace it with last value of i. And if
>>> crtc->num_scalers == -1, we wouldn't enter the loop anyway, so the id
>>> is still set to -1. The only scenario I see where it can break is e.g.
>>> when crtc->num_scalers == -2, but I have no clue if it is even possible.
>>>
>>> To be precise, I am wondering about such change (diff without your change
>>> being applied):
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/skl_scaler.c
>>> b/drivers/gpu/drm/i915/display/skl_scaler.c
>>> index 4c4deac7f9c8..78852267e60b 100644
>>> --- a/drivers/gpu/drm/i915/display/skl_scaler.c
>>> +++ b/drivers/gpu/drm/i915/display/skl_scaler.c
>>> @@ -969,7 +969,6 @@ void skl_scaler_get_config(struct intel_crtc_state
>>> *crtc_state)
>>>        struct intel_display *display = to_intel_display(crtc_state);
>>>        struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>>>        struct intel_crtc_scaler_state *scaler_state =
>>> &crtc_state->scaler_state;
>>> -       int id = -1;
>>>        int i;
>>>
>>>        /* find scaler attached to this pipe */
>>> @@ -980,8 +979,6 @@ void skl_scaler_get_config(struct intel_crtc_state
>>> *crtc_state)
>>>                if ((ctl & (PS_SCALER_EN | PS_BINDING_MASK)) != (PS_SCALER_EN
>>> | PS_BINDING_PIPE))
>>>                        continue;
>>>
>>> -               id = i;
>>> -
>>>                /* Read CASF regs for second scaler */
>>>                if (HAS_CASF(display) && id == 1)
>>>                        intel_casf_sharpness_get_config(crtc_state);
>>> @@ -1003,8 +1000,8 @@ void skl_scaler_get_config(struct intel_crtc_state
>>> *crtc_state)
>>>                break;
>>>        }
>>>
>>> -       scaler_state->scaler_id = id;
>>> -       if (id >= 0)
>>> +       scaler_state->scaler_id = crtc->num_scalers;
>>> +       if (scaler_state->scaler_id >= 0)
>>>                scaler_state->scaler_users |= (1 << SKL_CRTC_INDEX);
>>>        else
>>>                scaler_state->scaler_users &= ~(1 << SKL_CRTC_INDEX);
>>>
>>> But I don't know if the reasoning above makes any sense.
>>
>> Now I see that I missed the continue-break magic, so please disregard
>> the whole comment.
>
> I think the correct thing is to s/i/scaler_id/ and just move the
> scaler_state->scaler_id and caler_state->scaler_users assignment into
> the loop.
>
> Or even better something like this:
>
> skl_pipe_scaler_get_hw_state()
> {
> 	for (scaler_id) {
> 		ctl = ...
> 		if (ctl...)
> 			return scaler_id;
> 	}
>
> 	return -1;
> }
>
> skl_scaler_get_config()
> {
> 	scaler_id = skl_pipe_scaler_get_hw_state();
> 	if (scaler_id < 0)
> 		return;
>
> 	// do the scaler readout
> }
>
> Feel free to go at it.
>

Now it makes sense, thanks. :)

I've added you to couple of Suggested-by:, please correct me if I
mishandled it.

BR,
Michał

>>>
>>> Reviewed-by: Michał Grzelak <michal.grzelak@intel.com>
>>>
>>>> 			intel_casf_sharpness_get_config(crtc_state);
>>>>
>>>> 		if (!crtc_state->pch_pfit.casf.enable)
>>>> --
>>>> 2.52.0
>>>>
>>>>
>>>
>
>
> -- 
> Ville Syrjälä
> Intel
>