RE: [PATCH v2 07/10] drm/i915/scaler: unloop scaler readout that is run once

Grzelak, Michal
2026-04-20T08:22:05Z

Hi Nemesa,

-----Original Message-----
From: Garg, Nemesa <nemesa.garg@intel.com>
Sent: Tuesday, April 14, 2026 10:02 AM
To: Grzelak, Michal <michal.grzelak@intel.com>; intel-gfx@lists.freedesktop.org
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Subject: RE: [PATCH v2 07/10] drm/i915/scaler: unloop scaler readout that is run once

> -----Original Message-----
> From: Grzelak, Michal <michal.grzelak@intel.com>
> Sent: Saturday, April 11, 2026 11:15 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>; Garg, Nemesa
> <nemesa.garg@intel.com>; Grzelak, Michal <michal.grzelak@intel.com>
> Subject: [PATCH v2 07/10] drm/i915/scaler: unloop scaler readout that
> is run once
>
> Most of the loop's code is run once because of the continue statement
> at it's start and break statement at it's end. Kick it out of the loop.
>
> Cc: Nemesa Garg <nemesa.garg@intel.com>
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Michał Grzelak <michal.grzelak@intel.com>
> ---
> drivers/gpu/drm/i915/display/skl_scaler.c | 34
> ++++++++++++-----------
> 1 file changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/skl_scaler.c
> b/drivers/gpu/drm/i915/display/skl_scaler.c
> index 6d9080ec74ce0..e71d9c036a1e7 100644
> --- a/drivers/gpu/drm/i915/display/skl_scaler.c
> +++ b/drivers/gpu/drm/i915/display/skl_scaler.c
> @@ -951,36 +951,38 @@ void skl_scaler_get_config(struct
> intel_crtc_state
> *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 scaler_id;
> + u32 pos, size;
>
> /* find scaler attached to this pipe */
> for (scaler_id = 0; scaler_id < crtc->num_scalers; scaler_id++) {
> - u32 ctl, pos, size;
> + u32 ctl;
>
> ctl = intel_de_read(display, SKL_PS_CTRL(crtc->pipe, scaler_id));
> if ((ctl & (PS_SCALER_EN | PS_BINDING_MASK)) != (PS_SCALER_EN |
> PS_BINDING_PIPE))
> continue;
>
Hi Michal,

In this patch you are keeping both continue and break statement and then in patch 8 you are inverting the loop so I guess it will be better if we can squash these 2 patches so we will have all changes in one place related to this function. What do you think.

---
Since the count of lines affected is quite low, I thought of the same. I have convinced myself though to keep it that way because I think this patch is separate from the previous, and in case of breakage it would be faster to see what actually gone wrong. Also I found it non-trivial from the diff what is happening when they were squashed, so that was my rationale.

BR,
Michał
---

Thanks and Regards,
Nemesa

> - if (scaler_has_casf(display, scaler_id))
> - intel_casf_sharpness_get_config(crtc_state);
> -
> - crtc_state->pch_pfit.enabled = true;
> -
> - pos = intel_de_read(display, SKL_PS_WIN_POS(crtc->pipe,
> scaler_id));
> - size = intel_de_read(display, SKL_PS_WIN_SZ(crtc->pipe,
> scaler_id));
> -
> - drm_rect_init(&crtc_state->pch_pfit.dst,
> - REG_FIELD_GET(PS_WIN_XPOS_MASK, pos),
> - REG_FIELD_GET(PS_WIN_YPOS_MASK, pos),
> - REG_FIELD_GET(PS_WIN_XSIZE_MASK, size),
> - REG_FIELD_GET(PS_WIN_YSIZE_MASK, size));
> -
> - scaler_state->scalers[scaler_id].in_use = true;
> break;
> }
>
> if (scaler_id == crtc->num_scalers)
> return;
>
> + if (scaler_has_casf(display, scaler_id))
> + intel_casf_sharpness_get_config(crtc_state);
> +
> + crtc_state->pch_pfit.enabled = true;
> +
> + pos = intel_de_read(display, SKL_PS_WIN_POS(crtc->pipe, scaler_id));
> + size = intel_de_read(display, SKL_PS_WIN_SZ(crtc->pipe, scaler_id));
> +
> + drm_rect_init(&crtc_state->pch_pfit.dst,
> + REG_FIELD_GET(PS_WIN_XPOS_MASK, pos),
> + REG_FIELD_GET(PS_WIN_YPOS_MASK, pos),
> + REG_FIELD_GET(PS_WIN_XSIZE_MASK, size),
> + REG_FIELD_GET(PS_WIN_YSIZE_MASK, size));
> +
> + scaler_state->scalers[scaler_id].in_use = true;
> +
> scaler_state->scaler_id = scaler_id;
> if (scaler_id >= 0)
> scaler_state->scaler_users |= (1 << SKL_CRTC_INDEX);
> --
> 2.45.2