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

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

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.

> 
> BR,
> Michał
> 
> >
> > Reviewed-by: Michał Grzelak <michal.grzelak@intel.com>
> >
> > BR,
> > Michał
> >
> >> 			intel_casf_sharpness_get_config(crtc_state);
> >>
> >> 		if (!crtc_state->pch_pfit.casf.enable)
> >> -- 
> >> 2.52.0
> >> 
> >> 
> >


-- 
Ville Syrjälä
Intel