> -----Original Message-----
> From: Ville Syrjälä <[email protected]>
> Sent: Thursday, June 11, 2026 1:40 PM
> To: Manna, Animesh <[email protected]>
> Cc: [email protected]; [email protected]; Shankar,
> Uma <[email protected]>; Dibin Moolakadan Subrahmanian
> <[email protected]>; Nikula, Jani
> <[email protected]>
> Subject: Re: [PATCH v8 04/20] drm/i915/display: Pass target transcoder to
> intel_set_transcoder_timings()
>
> On Thu, Jun 04, 2026 at 01:24:00AM +0530, Animesh Manna wrote:
> > Let intel_set_transcoder_timings() take the target transcoder as an
> > explicit argument instead of always using crtc_state->cpu_transcoder.
> > This makes the helper reusable for callers that need to program
> > timings for a transcoder other than the CRTC's CPU transcoder.
> >
> > Update all existing callers to pass crtc_state->cpu_transcoder so
> > there is no functional change.
> >
> > Signed-off-by: Animesh Manna <[email protected]>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 35
> > ++++++++++----------
> > 1 file changed, 18 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 8e269b71f18e..9031264a34fc 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -132,7 +132,8 @@
> > #include "vlv_dsi_pll.h"
> > #include "vlv_dsi_regs.h"
> >
> > -static void intel_set_transcoder_timings(const struct
> > intel_crtc_state *crtc_state);
> > +static void intel_set_transcoder_timings(const struct intel_crtc_state
> *crtc_state,
> > + enum transcoder transcoder);
> > static void intel_set_pipe_src_size(const struct intel_crtc_state
> > *crtc_state); static void hsw_set_transconf(const struct
> > intel_crtc_state *crtc_state); static void bdw_set_pipe_misc(struct
> > intel_dsb *dsb, @@ -1504,7 +1505,7 @@ static void
> ilk_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
> > &crtc_state->dp_m2_n2);
> > }
> >
> > - intel_set_transcoder_timings(crtc_state);
> > + intel_set_transcoder_timings(crtc_state,
> > +crtc_state->cpu_transcoder);
> >
> > ilk_set_pipeconf(crtc_state);
> > }
> > @@ -1635,7 +1636,7 @@ static void hsw_configure_cpu_transcoder(const
> struct intel_crtc_state *crtc_sta
> > &crtc_state->dp_m2_n2);
> > }
> >
> > - intel_set_transcoder_timings(crtc_state);
> > + intel_set_transcoder_timings(crtc_state,
> > +crtc_state->cpu_transcoder);
> >
> > if (cpu_transcoder != TRANSCODER_EDP)
> > intel_de_write(display, TRANS_MULT(display,
> cpu_transcoder), @@
> > -2048,7 +2049,7 @@ static void i9xx_configure_cpu_transcoder(const
> struct intel_crtc_state *crtc_st
> > &crtc_state->dp_m2_n2);
> > }
> >
> > - intel_set_transcoder_timings(crtc_state);
> > + intel_set_transcoder_timings(crtc_state,
> > +crtc_state->cpu_transcoder);
> >
> > i9xx_set_pipeconf(crtc_state);
> > }
> > @@ -2664,17 +2665,17 @@ transcoder_has_vrr(const struct
> intel_crtc_state *crtc_state)
> > return HAS_VRR(display) && !transcoder_is_dsi(cpu_transcoder);
> > }
> >
> > -static void intel_set_transcoder_timings(const struct
> > intel_crtc_state *crtc_state)
> > +static void intel_set_transcoder_timings(const struct intel_crtc_state
> *crtc_state,
> > + enum transcoder transcoder)
> > {
> > struct intel_display *display = to_intel_display(crtc_state);
> > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > enum pipe pipe = crtc->pipe;
> > - enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > const struct drm_display_mode *adjusted_mode = &crtc_state-
> >hw.adjusted_mode;
> > u32 crtc_vdisplay, crtc_vtotal, crtc_vblank_start, crtc_vblank_end;
> > int vsyncshift = 0;
> >
> > - drm_WARN_ON(display->drm, transcoder_is_dsi(cpu_transcoder));
> > + drm_WARN_ON(display->drm, transcoder_is_dsi(transcoder));
>
> If you want to rename the variable, then do so in a separate patch.
> Mixing the rename with other changes is destroying the signal to noise ratio
> of the patch.
Sure, taken care in next version.
Regards,
Animesh
>
> >
> > /* We need to be careful not to changed the adjusted mode, for
> otherwise
> > * the hw state checker will get angry at the mismatch. */ @@
> > -2703,7 +2704,7 @@ static void intel_set_transcoder_timings(const struct
> intel_crtc_state *crtc_sta
> > */
> > if (DISPLAY_VER(display) >= 13) {
> > intel_de_write(display,
> > - TRANS_SET_CONTEXT_LATENCY(display,
> cpu_transcoder),
> > + TRANS_SET_CONTEXT_LATENCY(display,
> transcoder),
> > crtc_state->set_context_latency);
> >
> > /*
> > @@ -2718,16 +2719,16 @@ static void intel_set_transcoder_timings(const
> > struct intel_crtc_state *crtc_sta
> >
> > if (DISPLAY_VER(display) >= 4 && DISPLAY_VER(display) < 35)
> > intel_de_write(display,
> > - TRANS_VSYNCSHIFT(display, cpu_transcoder),
> > + TRANS_VSYNCSHIFT(display, transcoder),
> > vsyncshift);
> >
> > - intel_de_write(display, TRANS_HTOTAL(display, cpu_transcoder),
> > + intel_de_write(display, TRANS_HTOTAL(display, transcoder),
> > HACTIVE(adjusted_mode->crtc_hdisplay - 1) |
> > HTOTAL(adjusted_mode->crtc_htotal - 1));
> > - intel_de_write(display, TRANS_HBLANK(display, cpu_transcoder),
> > + intel_de_write(display, TRANS_HBLANK(display, transcoder),
> > HBLANK_START(adjusted_mode->crtc_hblank_start - 1) |
> > HBLANK_END(adjusted_mode->crtc_hblank_end - 1));
> > - intel_de_write(display, TRANS_HSYNC(display, cpu_transcoder),
> > + intel_de_write(display, TRANS_HSYNC(display, transcoder),
> > HSYNC_START(adjusted_mode->crtc_hsync_start - 1) |
> > HSYNC_END(adjusted_mode->crtc_hsync_end - 1));
> >
> > @@ -2740,13 +2741,13 @@ static void intel_set_transcoder_timings(const
> struct intel_crtc_state *crtc_sta
> > if (intel_vrr_always_use_vrr_tg(display))
> > crtc_vtotal = 1;
> >
> > - intel_de_write(display, TRANS_VTOTAL(display, cpu_transcoder),
> > + intel_de_write(display, TRANS_VTOTAL(display, transcoder),
> > VACTIVE(crtc_vdisplay - 1) |
> > VTOTAL(crtc_vtotal - 1));
> > - intel_de_write(display, TRANS_VBLANK(display, cpu_transcoder),
> > + intel_de_write(display, TRANS_VBLANK(display, transcoder),
> > VBLANK_START(crtc_vblank_start - 1) |
> > VBLANK_END(crtc_vblank_end - 1));
> > - intel_de_write(display, TRANS_VSYNC(display, cpu_transcoder),
> > + intel_de_write(display, TRANS_VSYNC(display, transcoder),
> > VSYNC_START(adjusted_mode->crtc_vsync_start - 1) |
> > VSYNC_END(adjusted_mode->crtc_vsync_end - 1));
> >
> > @@ -2754,7 +2755,7 @@ static void intel_set_transcoder_timings(const
> struct intel_crtc_state *crtc_sta
> > * programmed with the VTOTAL_EDP value. Same for VTOTAL_C.
> This is
> > * documented on the DDI_FUNC_CTL register description, EDP Input
> Select
> > * bits. */
> > - if (display->platform.haswell && cpu_transcoder ==
> TRANSCODER_EDP &&
> > + if (display->platform.haswell && transcoder == TRANSCODER_EDP
> &&
> > (pipe == PIPE_B || pipe == PIPE_C))
> > intel_de_write(display, TRANS_VTOTAL(display, pipe),
> > VACTIVE(crtc_vdisplay - 1) | @@ -2769,7 +2770,7
> @@ static
> > void intel_set_transcoder_timings(const struct intel_crtc_state *crtc_sta
> > * followed by BE which DPRX devices are unable to handle.
> > * https://groups.vesa.org/wg/DP/document/20494
> > */
> > - intel_de_write(display,
> DP_MIN_HBLANK_CTL(cpu_transcoder),
> > + intel_de_write(display, DP_MIN_HBLANK_CTL(transcoder),
> > crtc_state->min_hblank);
> > }
> > }
> > --
> > 2.29.0
>
> --
> Ville Syrjälä
> Intel
