> Subject: Re: [PATCH] drm/i915/display: Use PIPEDMC_FRMTMSTMP on display > ver >= 30 > > On Fri, 15 May 2026, Suraj Kandpal <[email protected]> wrote: > > Starting with display version 30, the per-pipe frame timestamp is read > > from the PIPEDMC register block (PIPEDMC_FRMTMSTMP) instead of the > > legacy PIPE_FRMTMSTMP MMIO. Extend PIPE_FRMTMSTMP() to take the > > display and select the appropriate register based on DISPLAY_VER(), > > and update all callers (intel_vblank, intel_initial_plane) accordingly. > > > > Bspec: 79482 > > WA: 14022946399 > > Signed-off-by: Suraj Kandpal <[email protected]> > > --- > > drivers/gpu/drm/i915/display/intel_display_regs.h | 7 +++++-- > > drivers/gpu/drm/i915/display/intel_initial_plane.c | 4 ++-- > > drivers/gpu/drm/i915/display/intel_vblank.c | 4 ++-- > > 3 files changed, 9 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_regs.h > > b/drivers/gpu/drm/i915/display/intel_display_regs.h > > index 4321f8b529da..579f802215d3 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_regs.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_regs.h > > @@ -3149,8 +3149,11 @@ enum skl_power_gate { > > /* g4x+, except vlv/chv! */ > > #define _PIPE_FRMTMSTMP_A 0x70048 > > #define _PIPE_FRMTMSTMP_B 0x71048 > > -#define PIPE_FRMTMSTMP(pipe) \ > > - _MMIO_PIPE(pipe, _PIPE_FRMTMSTMP_A, _PIPE_FRMTMSTMP_B) > > +#define _PIPEDMC_FRMTMSTMP_A 0x5f0ac > > +#define _PIPEDMC_FRMTMSTMP_B 0x5f4ac > > +#define PIPE_FRMTMSTMP(display, pipe) (DISPLAY_VER(display) >= 30 ? > \ > > + _MMIO_PIPE(pipe, _PIPEDMC_FRMTMSTMP_A, > _PIPEDMC_FRMTMSTMP_B) : \ > > + _MMIO_PIPE(pipe, _PIPE_FRMTMSTMP_A, _PIPE_FRMTMSTMP_B)) > > I'm wondering if we want to hide this difference inside the register macro, > though. > > Yes, it's the easy thing to do. > > But PIPEDMC registers belong in intel_dmc_regs.h, and it's a bit questionable > to > have something that looks like PIPE_FRMTMSTMP() suddenly end up being > PIPMEDMC_FRMTMSTMP.
True the reason I did it was to avoid all the if else cases everywhere this had been called. Also helps make sure that this WA does not get lost in case someone at some point of time decides to use PIPE_FRMTMSTMP() instead of PIPE_DMC_FRMTMSTMP(). Which makes me think if I do, choose to do the if -else way wherever this has been called. I should perhaps add a comments in intel_display_regs.h stating to use PIPE_DMC_FRMTMSTMP() instead. > > BR, > Jani. > > > PS. It's absolutely disgusting that this is named "FRMTMSTMP" in bspec. Vowels > exist for a reason. Oh I absolutely agree. Its revolting to end up defining them like this too, to maintain consistency with Bspec. Regards, Suraj Kandpal > > > > > /* g4x+, except vlv/chv! */ > > #define _PIPE_FLIPTMSTMP_A 0x7004C > > diff --git a/drivers/gpu/drm/i915/display/intel_initial_plane.c > > b/drivers/gpu/drm/i915/display/intel_initial_plane.c > > index 034fe199c2a1..004cbdb6be32 100644 > > --- a/drivers/gpu/drm/i915/display/intel_initial_plane.c > > +++ b/drivers/gpu/drm/i915/display/intel_initial_plane.c > > @@ -34,9 +34,9 @@ void intel_initial_plane_vblank_wait(struct intel_crtc > *crtc) > > return; > > } > > > > - start_ts = intel_de_read(display, PIPE_FRMTMSTMP(crtc->pipe)); > > + start_ts = intel_de_read(display, PIPE_FRMTMSTMP(display, > > +crtc->pipe)); > > > > - ret = poll_timeout_us(end_ts = intel_de_read(display, > PIPE_FRMTMSTMP(crtc->pipe)), > > + ret = poll_timeout_us(end_ts = intel_de_read(display, > > +PIPE_FRMTMSTMP(display, crtc->pipe)), > > end_ts != start_ts, 1000, 1000 * 1000, false); > > if (ret) > > drm_warn(display->drm, "[CRTC:%d:%s] early vblank wait > timed > > out\n", diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c > > b/drivers/gpu/drm/i915/display/intel_vblank.c > > index 28d81199792e..52ff47936f9e 100644 > > --- a/drivers/gpu/drm/i915/display/intel_vblank.c > > +++ b/drivers/gpu/drm/i915/display/intel_vblank.c > > @@ -157,7 +157,7 @@ static u32 > intel_crtc_scanlines_since_frame_timestamp(struct intel_crtc *crtc) > > * is sampled at every start of vertical blank. > > */ > > scan_prev_time = intel_de_read_fw(display, > > - PIPE_FRMTMSTMP(crtc- > >pipe)); > > + PIPE_FRMTMSTMP(display, > crtc->pipe)); > > > > /* > > * The TIMESTAMP_CTR register has the current @@ -166,7 > +166,7 @@ > > static u32 intel_crtc_scanlines_since_frame_timestamp(struct intel_crtc > > *crtc) > > scan_curr_time = intel_de_read_fw(display, > IVB_TIMESTAMP_CTR); > > > > scan_post_time = intel_de_read_fw(display, > > - PIPE_FRMTMSTMP(crtc- > >pipe)); > > + PIPE_FRMTMSTMP(display, > crtc->pipe)); > > } while (scan_post_time != scan_prev_time); > > > > return div_u64(mul_u32_u32(scan_curr_time - scan_prev_time, > > -- > Jani Nikula, Intel
