RE: [PATCH v2 09/10] drm/i915/scaler: abstract scaler searching loop

Grzelak, Michal
2026-04-20T08:19:29Z

From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>On Sat, Apr 11, 2026 at 07:45:25PM +0200, Michał Grzelak wrote:
>> Add a helper function hiding the search for scaler_id.
>>
>> 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 | 29
>> ++++++++++++++---------
>> 1 file changed, 18 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/skl_scaler.c
>> b/drivers/gpu/drm/i915/display/skl_scaler.c
>> index 4e2f4c4ffc45a..1f47c5fc1802f 100644
>> --- a/drivers/gpu/drm/i915/display/skl_scaler.c
>> +++ b/drivers/gpu/drm/i915/display/skl_scaler.c
>> @@ -836,6 +836,21 @@ void skl_pfit_enable(const struct intel_crtc_state *crtc_state)
>> PS_WIN_XSIZE(width) | PS_WIN_YSIZE(height)); }
>>
>> +static int skl_pipe_scaler_get_hw_state(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);
>> + u32 ctl;
>
>'ctl' could stay inside the loop.
>
>I suppose technically 'display' could go there as well, but we've established the convention that 'display' (if needed inside the function) is >the first variable declared. So that one should stay here.
>
>> +
>> + for (int scaler_id = 0; scaler_id < crtc->num_scalers; scaler_id++) {
>> + 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))
>> + return scaler_id;
>> + }
>> +
>> + return -1;
>> +}
>> +
>> void
>> skl_program_plane_scaler(struct intel_dsb *dsb,
>> struct intel_plane *plane,
>> @@ -950,19 +965,11 @@ 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 scaler_id;
>> - u32 pos, size;
>> -
>> /* find scaler attached to this pipe */
>> - for (scaler_id = 0; scaler_id < crtc->num_scalers; scaler_id++) {
>> - 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))
>> - break;
>> - }
>> + int scaler_id = skl_pipe_scaler_get_hw_state(crtc_state);
>
>I don't like hiding stuff with potential side effects inside the variable declaration block. Only pure functions should be called there. So it's >better to do the function call+assignment just before the <0 check.
>
>Apart from that it all looks good to me. So with that adjusted the series is
>Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>But looks like you forgot to cc intel-xe@. We do want xe CI results before merging...
>

Will repost corrected version later since now I don't have access to my dev box. And thanks for the review =)

BR,
Michał