Re: [PATCH v4 03/13] drm/i915/cmtg: Set timings for CMTG

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

On Thu, Apr 23, 2026 at 11:07:55AM +0000, Manna, Animesh wrote:
> 
> 
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Sent: Thursday, April 23, 2026 4:22 PM
> > To: Manna, Animesh <animesh.manna@intel.com>
> > Cc: Nikula, Jani <jani.nikula@intel.com>; intel-gfx@lists.freedesktop.org;
> > intel-xe@lists.freedesktop.org; Shankar, Uma <uma.shankar@intel.com>;
> > Dibin Moolakadan Subrahmanian
> > <dibin.moolakadan.subrahmanian@intel.com>
> > Subject: Re: [PATCH v4 03/13] drm/i915/cmtg: Set timings for CMTG
> > 
> > On Thu, Apr 23, 2026 at 04:55:55AM +0000, Manna, Animesh wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Sent: Friday, April 17, 2026 3:57 PM
> > > > To: Manna, Animesh <animesh.manna@intel.com>
> > > > Cc: Nikula, Jani <jani.nikula@intel.com>;
> > > > intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org;
> > > > Shankar, Uma <uma.shankar@intel.com>; Dibin Moolakadan
> > Subrahmanian
> > > > <dibin.moolakadan.subrahmanian@intel.com>
> > > > Subject: Re: [PATCH v4 03/13] drm/i915/cmtg: Set timings for CMTG
> > > >
> > > > On Fri, Apr 17, 2026 at 06:03:52AM +0000, Manna, Animesh wrote:
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Nikula, Jani <jani.nikula@intel.com>
> > > > > > Sent: Tuesday, April 14, 2026 7:03 PM
> > > > > > To: Manna, Animesh <animesh.manna@intel.com>; intel-
> > > > > > gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> > > > > > Cc: Shankar, Uma <uma.shankar@intel.com>; Dibin Moolakadan
> > > > > > Subrahmanian <dibin.moolakadan.subrahmanian@intel.com>;
> > Manna,
> > > > > > Animesh <animesh.manna@intel.com>
> > > > > > Subject: Re: [PATCH v4 03/13] drm/i915/cmtg: Set timings for
> > > > > > CMTG
> > > > > >
> > > > > > On Sun, 12 Apr 2026, Animesh Manna <animesh.manna@intel.com>
> > > > 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]
> > > > > > >
> > > > > > > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/display/intel_cmtg.c     | 61
> > > > ++++++++++++++++++-
> > > > > > >  drivers/gpu/drm/i915/display/intel_cmtg.h     |  3 +
> > > > > > >  .../gpu/drm/i915/display/intel_cmtg_regs.h    | 31 ++++++++++
> > > > > > >  drivers/gpu/drm/i915/display/intel_display.c  |  4 ++
> > > > > > >  4 files changed, 98 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cmtg.c
> > > > > > > b/drivers/gpu/drm/i915/display/intel_cmtg.c
> > > > > > > index 403f9e10a8dc..a3db1368bd83 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/intel_cmtg.c
> > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_cmtg.c
> > > > > > > @@ -4,7 +4,6 @@
> > > > > > >   */
> > > > > > >
> > > > > > >  #include <linux/string_choices.h> -#include <linux/types.h>
> > > > > > >
> > > > > > >  #include <drm/drm_device.h>
> > > > > > >  #include <drm/drm_print.h>
> > > > > > > @@ -222,3 +221,63 @@ 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);  }
> > > > > > > +
> > > > > > > +void intel_cmtg_set_timings(const struct intel_crtc_state
> > > > > > > +*crtc_state, bool lrr) {
> > > > > > > +	struct intel_display *display = to_intel_display(crtc_state);
> > > > > > > +	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;
> > > > > > > +
> > > > > > > +	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_VTOTAL_CMTG(cpu_transcoder),
> > > > > > > +			       VACTIVE(crtc_vdisplay - 1) |
> > > > > > > +			       VTOTAL(crtc_vtotal - 1));
> > > > > > > +		intel_de_write(display,
> > > > > > TRANS_VBLANK_CMTG(cpu_transcoder),
> > > > > > > +			       VBLANK_START(crtc_vblank_start - 1) |
> > > > > > > +			       VBLANK_END(crtc_vblank_end - 1));
> > > > > > > +		return;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	intel_de_write(display,
> > TRANS_HTOTAL_CMTG(cpu_transcoder),
> > > > > > > +		       HACTIVE(adjusted_mode->crtc_hdisplay - 1) |
> > > > > > > +		       HTOTAL(adjusted_mode->crtc_htotal - 1));
> > > > > > > +	intel_de_write(display,
> > TRANS_HBLANK_CMTG(cpu_transcoder),
> > > > > > > +		       HBLANK_START(adjusted_mode-
> > >crtc_hblank_start - 1) |
> > > > > > > +		       HBLANK_END(adjusted_mode->crtc_hblank_end
> > - 1));
> > > > > > > +	intel_de_write(display,
> > TRANS_HSYNC_CMTG(cpu_transcoder),
> > > > > > > +		       HSYNC_START(adjusted_mode->crtc_hsync_start
> > - 1) |
> > > > > > > +		       HSYNC_END(adjusted_mode->crtc_hsync_end -
> > 1));
> > > > > > > +	intel_de_write(display,
> > TRANS_VTOTAL_CMTG(cpu_transcoder),
> > > > > > > +		       VACTIVE(crtc_vdisplay - 1) |
> > > > > > > +		       VTOTAL(crtc_vtotal - 1));
> > > > > > > +	intel_de_write(display,
> > TRANS_VBLANK_CMTG(cpu_transcoder),
> > > > > > > +		       VBLANK_START(crtc_vblank_start - 1) |
> > > > > > > +		       VBLANK_END(crtc_vblank_end - 1));
> > > > > > > +	intel_de_write(display,
> > TRANS_VSYNC_CMTG(cpu_transcoder),
> > > > > > > +		       VSYNC_START(adjusted_mode->crtc_vsync_start -
> > 1) |
> > > > > > > +		       VSYNC_END(adjusted_mode->crtc_vsync_end -
> > 1));
> > > > > > > +	intel_de_write(display,
> > > > > > TRANS_SET_CTX_LATENCY_CMTG(cpu_transcoder),
> > > > > > > +		       crtc_state->set_context_latency); }
> > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cmtg.h
> > > > > > > b/drivers/gpu/drm/i915/display/intel_cmtg.h
> > > > > > > index 660ec513626e..53a44f505dd2 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/intel_cmtg.h
> > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_cmtg.h
> > > > > > > @@ -6,9 +6,12 @@
> > > > > > >  #ifndef __INTEL_CMTG_H__
> > > > > > >  #define __INTEL_CMTG_H__
> > > > > > >
> > > > > > > +#include <linux/types.h>
> > > > > > > +
> > > > > > >  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_cmtg_regs.h
> > > > > > > b/drivers/gpu/drm/i915/display/intel_cmtg_regs.h
> > > > > > > index 4a80b88d88fd..f7fc812d8ef0 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/intel_cmtg_regs.h
> > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_cmtg_regs.h
> > > > > > > @@ -20,4 +20,35 @@
> > > > > > >  #define TRANS_CMTG_CTL_B		_MMIO(0x6fb88)
> > > > > > >  #define  CMTG_ENABLE			REG_BIT(31)
> > > > > > >
> > > > > > > +#define _TRANS_HTOTAL_CMTG_A		0x6F000
> > > > > > > +#define _TRANS_HTOTAL_CMTG_B		0x6F100
> > > > > > > +#define TRANS_HTOTAL_CMTG(trans)
> > 	_MMIO_TRANS((trans), \
> > > > > > > +
> > _TRANS_HTOTAL_CMTG_A,
> > > > > > _TRANS_HTOTAL_CMTG_B)
> > > > > > > +#define _TRANS_HBLANK_CMTG_A		0x6F004
> > > > > > > +#define _TRANS_HBLANK_CMTG_B		0x6F104
> > > > > > > +#define TRANS_HBLANK_CMTG(trans)
> > 	_MMIO_TRANS((trans), \
> > > > > > > +
> > _TRANS_HBLANK_CMTG_A,
> > > > > > _TRANS_HBLANK_CMTG_B)
> > > > > > > +#define _TRANS_HSYNC_CMTG_A		0x6F008
> > > > > > > +#define _TRANS_HSYNC_CMTG_B		0x6F108
> > > > > > > +#define TRANS_HSYNC_CMTG(trans)
> > > > > > 	_MMIO_TRANS((trans), \
> > > > > > > +
> > _TRANS_HSYNC_CMTG_A,
> > > > > > _TRANS_HSYNC_CMTG_B)
> > > > > > > +#define _TRANS_VTOTAL_CMTG_A		0x6F00C
> > > > > > > +#define _TRANS_VTOTAL_CMTG_B		0x6F10C
> > > > > > > +#define TRANS_VTOTAL_CMTG(trans)
> > 	_MMIO_TRANS((trans), \
> > > > > > > +
> > _TRANS_VTOTAL_CMTG_A,
> > > > > > _TRANS_VTOTAL_CMTG_B)
> > > > > > > +#define _TRANS_VBLANK_CMTG_A		0x6F010
> > > > > > > +#define _TRANS_VBLANK_CMTG_B		0x6F110
> > > > > > > +#define TRANS_VBLANK_CMTG(trans)
> > 	_MMIO_TRANS((trans), \
> > > > > > > +
> > _TRANS_VBLANK_CMTG_A,
> > > > > > _TRANS_VBLANK_CMTG_B)
> > > > > > > +#define _TRANS_VSYNC_CMTG_A		0x6F014
> > > > > > > +#define _TRANS_VSYNC_CMTG_B		0x6F114
> > > > > > > +#define TRANS_VSYNC_CMTG(trans)
> > > > > > 	_MMIO_TRANS((trans), \
> > > > > > > +
> > _TRANS_VSYNC_CMTG_A,
> > > > > > _TRANS_VSYNC_CMTG_B)
> > > > > >
> > > > > > I though there was already feedback that these match the regular
> > > > > > transcoder registers.
> > > > >
> > > > > _TRANS_HTOTAL_A         0x60000
> > > > > _TRANS_HTOTAL_B         0x61000
> > > > >
> > > > > _TRANS_HTOTAL_CMTG_A            0x6F000
> > > > > _TRANS_HTOTAL_CMTG_B            0x6F100
> > > > >
> > > > > I am not clear how to match?
> > > >
> > > > #define TRANSCODER_CMTG0_OFFSET 0x6F000 #define
> > > > TRANSCODER_CMTG1_OFFSET 0x6F100
> > >
> > > Ok, just to double check my understanding, I am putting below all the
> > changes which maybe you are suggesting.
> > > Can please confirm or if I am missing something please let me know.
> > >
> > > Step1: Define offset macro.
> > > #define TRANSCODER_CMTGA_OFFSET 0x6F000 #define
> > > TRANSCODER_CMTGB_OFFSET 0x6F100
> > 
> > s/AB/01/ to actually match the spec.
> 
> Ok.
> 
> > 
> > > Step2: Add trans_cmtg_offset array in intel_display_device_info structure
> > and initialize.
> > > .trans_cmtg_offsets = {                                                      \
> > >                 [TRANSCODER_A] = TRANSCODER_CMTGA_OFFSET,
> > \
> > >                 [TRANSCODER_B] = TRANSCODER_CMTGB_OFFSET, },
> > 
> > They are just transcoders, so they go into .trans_offsets.
> > If there are any pipe register that are actually transcoder registers then we
> > may also need a sort of fake .pipe_offsets (like we have for the EDP
> > transcoder)
> 
> Only CMTG transcoder is not enough, cmtg transcoder will be enabled along with normal transcoder.
> Normal transcoder will use .trans_offsets and cmtg transcoder need separate structure .trans_cmtg_offsets for storing offset. So added separately.
> 
> > 
> > > Step3: Define INTEL_DISPLAY_DEVICE_TRANS_CMTG_OFFSET which will use
> > > trans_cmtg_offset #define
> > INTEL_DISPLAY_DEVICE_TRANS_CMTG_OFFSET(display, trans) \
> > >         (DISPLAY_INFO((display))->trans_cmtg_offsets[(trans)] - \
> > >          DISPLAY_INFO((display))->trans_offsets[TRANSCODER_A] + \
> > >          DISPLAY_MMIO_BASE((display)))
> > >
> > > Step4: Define _MMIO_TRANS2_CMTG which will use
> > INTEL_DISPLAY_DEVICE_TRANS_CMTG_OFFSET
> > > #define _MMIO_TRANS2_CMTG(display, trans, reg)
> > _MMIO(INTEL_DISPLAY_DEVICE_TRANS_CMTG_OFFSET((display), (trans)) +
> > (reg))
> > >
> > > Step5: Define TRANS_HTOTAL_CMTG
> > > #define TRANS_HTOTAL_CMTG(display, trans)
> > 	_MMIO_TRANS2_CMTG(display, (trans), _TRANS_HTOTAL_A)
> > > #define TRANS_HBLANK_CMTG(display, trans)
> > _MMIO_TRANS2_CMTG(display, (trans), _TRANS_HBLANK_A)
> > 
> > No, you just use TRANS_HTOTAL() and co.
> > 
> > Or at least that's my current thinking. Avoids all the duplicated stuff.
> 
> Same like above - Only CMTG transcoder is not enough, cmtg transcoder will be enabled along with normal transcoder.
> So, we need both TRANS_HTOTAL() and TRANS_HTOTAL_CMTG().

We just need TRANS_HTOTAL(TRANSCODER_A) and TRANS_HTOTAL(TRANSCODER_CMTG0) (or whatever 
transcoders we happen to use).

So just
enum transcoder {
...
+	 TRANSCODER_CMTG0,
+	 TRANSCODER_CMTG1,
...
};

+ #define TRANSCODER_CMTG0_OFFSET 0x6F000
+ #define TRANSCODER_CMTG1_OFFSET 0x6F100

+ .trans_offsets[TRANSCODER_CMTG0] = TRANSCODER_CMTG0_OFFSET,
+ .trans_offsets[TRANSCODER_CMTG1] = TRANSCODER_CMTG1_OFFSET,

> Please let me know for any additional details and the above change still needed or not. Because only NVL will be supporting CMTG.

The CMTG code will be used (if only for disablign CMTG) on all
platforms that have CMTG, which IIRC is ADL+.

-- 
Ville Syrjälä
Intel