> -----Original Message-----
> From: Ville Syrjälä <[email protected]>
> Sent: Friday, May 29, 2026 4:29 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 v7 04/15] drm/i915/cmtg: Set timings for CMTG
>
> On Tue, May 26, 2026 at 07:08:00PM +0530, Animesh Manna wrote:
> > Timing registers are separate for CMTG, read transcoder register and
> > program cmtg transcoder with those values.
> >
> > v2:
> > - Use sw state instead of reading directly from hardware. [Jani]
> > - Move set_timing later after encoder enable. [Dibin]
> >
> > v3:
> > - Replace id with trans. [Jani]
> > - Program cmtg set_timing() along with primary transcoder timing.
> >
> > v4:
> > - Use _MMIO_TRANS() for cmtg registers instead of direct
> > multiplication. [Jani]
> >
> > v5:
> > - Modify register definition approach and match existing transcoder
> > definition. [Ville]
> >
> > Signed-off-by: Animesh Manna <[email protected]>
> > ---
> > drivers/gpu/drm/i915/display/intel_cmtg.c | 74 ++++++++++++++++++++
> > drivers/gpu/drm/i915/display/intel_cmtg.h | 1 +
> > drivers/gpu/drm/i915/display/intel_display.c | 4 ++
> > 3 files changed, 79 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cmtg.c
> > b/drivers/gpu/drm/i915/display/intel_cmtg.c
> > index fbc8a4f2b9cb..0e730afbb4ab 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cmtg.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cmtg.c
> > @@ -219,3 +219,77 @@ void intel_cmtg_set_clk_select(const struct
> intel_crtc_state *crtc_state)
> > if (clk_sel_set)
> > intel_de_rmw(display, CMTG_CLK_SEL, clk_sel_clr,
> clk_sel_set); }
> > +
> > +static inline enum transcoder to_cmtg_transcoder(enum transcoder
> > +cpu_transcoder) {
> > + switch (cpu_transcoder) {
> > + case TRANSCODER_A:
> > + return TRANSCODER_CMTG0;
> > + case TRANSCODER_B:
> > + return TRANSCODER_CMTG1;
> > + default:
> > + return INVALID_TRANSCODER;
> > + }
> > +}
> > +
> > +void intel_cmtg_set_timings(const struct intel_crtc_state
> > +*crtc_state, bool lrr) {
> > + struct intel_display *display = to_intel_display(crtc_state);
> > + const struct drm_display_mode *adjusted_mode = &crtc_state-
> >hw.adjusted_mode;
> > + enum transcoder cmtg_transcoder = to_cmtg_transcoder(crtc_state-
> >cpu_transcoder);
> > + u32 crtc_vdisplay, crtc_vtotal, crtc_vblank_start, crtc_vblank_end;
> > +
> > + if (!intel_cmtg_is_allowed(crtc_state))
> > + return;
> > +
> > + crtc_vdisplay = adjusted_mode->crtc_vdisplay;
> > +
> > + /*
> > + * For platforms that always use VRR Timing Generator, the
> VTOTAL.Vtotal
> > + * bits are not required. Since the support for these bits is going to
> > + * be deprecated in upcoming platforms, avoid writing these bits for
> the
> > + * platforms that do not use legacy Timing Generator.
> > + */
> > + crtc_vtotal = 1;
> > +
> > + /*
> > + * VBLANK_START not used by hw, just clear it
> > + * to make it stand out in register dumps.
> > + */
> > + crtc_vblank_start = 1;
> > +
> > + crtc_vblank_end = adjusted_mode->crtc_vblank_end;
> > +
> > + if (lrr) {
> > + intel_de_write(display,
> TRANS_SET_CONTEXT_LATENCY(display, cmtg_transcoder),
> > + crtc_state->set_context_latency);
> > + intel_de_write(display, TRANS_VBLANK(display,
> cmtg_transcoder),
> > + VBLANK_START(crtc_vblank_start - 1) |
> > + VBLANK_END(crtc_vblank_end - 1));
> > + intel_de_write(display, TRANS_VTOTAL(display,
> cmtg_transcoder),
> > + VACTIVE(crtc_vdisplay - 1) |
> > + VTOTAL(crtc_vtotal - 1));
> > + return;
> > + }
> > +
> > + intel_de_write(display, TRANS_HTOTAL(display, cmtg_transcoder),
> > + HACTIVE(adjusted_mode->crtc_hdisplay - 1) |
> > + HTOTAL(adjusted_mode->crtc_htotal - 1));
> > + intel_de_write(display, TRANS_HBLANK(display, cmtg_transcoder),
> > + HBLANK_START(adjusted_mode->crtc_hblank_start - 1) |
> > + HBLANK_END(adjusted_mode->crtc_hblank_end - 1));
> > + intel_de_write(display, TRANS_HSYNC(display, cmtg_transcoder),
> > + HSYNC_START(adjusted_mode->crtc_hsync_start - 1) |
> > + HSYNC_END(adjusted_mode->crtc_hsync_end - 1));
> > + intel_de_write(display, TRANS_VTOTAL(display, cmtg_transcoder),
> > + VACTIVE(crtc_vdisplay - 1) |
> > + VTOTAL(crtc_vtotal - 1));
> > + intel_de_write(display, TRANS_VBLANK(display, cmtg_transcoder),
> > + VBLANK_START(crtc_vblank_start - 1) |
> > + VBLANK_END(crtc_vblank_end - 1));
> > + intel_de_write(display, TRANS_VSYNC(display, cmtg_transcoder),
> > + VSYNC_START(adjusted_mode->crtc_vsync_start - 1) |
> > + VSYNC_END(adjusted_mode->crtc_vsync_end - 1));
> > + intel_de_write(display, TRANS_SET_CONTEXT_LATENCY(display,
> cmtg_transcoder),
> > + crtc_state->set_context_latency); }
>
> We already have functions to configure the timing registers. Why can't we
> just reuse those (as in pass the transcoder from the caller)?
Due to following reasons did not reuse,
1. CMTG will be enabled very specific case like single EDP on pipe-A/pipe-B
during psr2/lobf/pr-alpm.
2. Function prototype of existing functions to configure the timing registers
need to be changed.
3. Need condition check for DP_MIN_HBLANK_CTL register programming.
4. Two version of set-timings - intel_set_transcoder_timings_lrr() and
intel_set_transcoder_timings().
5. May overload intel_display.c.
I can try to reuse If you have strong objections on current implementation,
please let me know.
Also is it only for timing registers or same needed for link-m/n and vrr
registers.
Regards,
Animesh
>
> > diff --git a/drivers/gpu/drm/i915/display/intel_cmtg.h
> > b/drivers/gpu/drm/i915/display/intel_cmtg.h
> > index 87092ce6d67b..53a44f505dd2 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cmtg.h
> > +++ b/drivers/gpu/drm/i915/display/intel_cmtg.h
> > @@ -11,6 +11,7 @@
> > struct intel_display;
> > struct intel_crtc_state;
> >
> > +void intel_cmtg_set_timings(const struct intel_crtc_state
> > +*crtc_state, bool lrr);
> > void intel_cmtg_set_clk_select(const struct intel_crtc_state
> > *crtc_state); void intel_cmtg_sanitize(struct intel_display
> > *display); bool intel_cmtg_is_allowed(const struct intel_crtc_state
> > *crtc_state); diff --git
> > a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 6c8935f69db1..f8bcd1fcddca 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -60,6 +60,7 @@
> > #include "intel_bw.h"
> > #include "intel_cdclk.h"
> > #include "intel_clock_gating.h"
> > +#include "intel_cmtg.h"
> > #include "intel_color.h"
> > #include "intel_crt.h"
> > #include "intel_crtc.h"
> > @@ -2753,6 +2754,8 @@ static void intel_set_transcoder_timings(const
> struct intel_crtc_state *crtc_sta
> > intel_de_write(display,
> DP_MIN_HBLANK_CTL(cpu_transcoder),
> > crtc_state->min_hblank);
> > }
> > +
> > + intel_cmtg_set_timings(crtc_state, false);
> > }
> >
> > static void intel_set_transcoder_timings_lrr(const struct
> > intel_crtc_state *crtc_state) @@ -2814,6 +2817,7 @@ static void
> intel_set_transcoder_timings_lrr(const struct intel_crtc_state *crtc
> > VACTIVE(crtc_vdisplay - 1) |
> > VTOTAL(crtc_vtotal - 1));
> >
> > + intel_cmtg_set_timings(crtc_state, true);
> > intel_vrr_set_fixed_rr_timings(crtc_state);
> > intel_vrr_transcoder_enable(crtc_state);
> > }
> > --
> > 2.29.0
>
> --
> Ville Syrjälä
> Intel
