Hey, Den 2025-08-29 kl. 13:14, skrev Jani Nikula: > On Thu, 28 Aug 2025, Maarten Lankhorst <[email protected]> wrote: >> The display tracepoints will work, but drm_crtc_accurate_vblank_count() >> takes an irq lock. Use the less accurate drm_crtc_vblank_count() on >> affected platforms, which is simply an atomic_read64(); >> >> Signed-off-by: Maarten Lankhorst <[email protected]> >> --- >> drivers/gpu/drm/i915/display/intel_crtc.c | 9 ++-- >> drivers/gpu/drm/i915/display/intel_crtc.h | 2 +- >> .../drm/i915/display/intel_display_trace.h | 48 ++++++++++--------- >> 3 files changed, 31 insertions(+), 28 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c >> b/drivers/gpu/drm/i915/display/intel_crtc.c >> index a187db6df2d36..5c8ce35d21ca3 100644 >> --- a/drivers/gpu/drm/i915/display/intel_crtc.c >> +++ b/drivers/gpu/drm/i915/display/intel_crtc.c >> @@ -77,7 +77,7 @@ void intel_wait_for_vblank_if_active(struct intel_display >> *display, >> intel_crtc_wait_for_next_vblank(crtc); >> } >> >> -u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc) >> +u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc, bool >> update_vblank) >> { >> struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(&crtc->base); >> >> @@ -85,7 +85,8 @@ u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc) >> return 0; >> >> if (!vblank->max_vblank_count) >> - return (u32)drm_crtc_accurate_vblank_count(&crtc->base); >> + return (u32)(update_vblank ? >> drm_crtc_accurate_vblank_count(&crtc->base) : >> + drm_crtc_vblank_count(&crtc->base)); >> >> return crtc->base.funcs->get_vblank_counter(&crtc->base); >> } >> @@ -574,7 +575,7 @@ void intel_pipe_update_start(struct intel_atomic_state >> *state, >> >> crtc->debug.scanline_start = scanline; >> crtc->debug.start_vbl_time = ktime_get(); >> - crtc->debug.start_vbl_count = intel_crtc_get_vblank_counter(crtc); >> + crtc->debug.start_vbl_count = intel_crtc_get_vblank_counter(crtc, true); >> >> trace_intel_pipe_update_vblank_evaded(crtc); >> return; >> @@ -660,7 +661,7 @@ void intel_pipe_update_end(struct intel_atomic_state >> *state, >> intel_atomic_get_new_crtc_state(state, crtc); >> enum pipe pipe = crtc->pipe; >> int scanline_end = intel_get_crtc_scanline(crtc); >> - u32 end_vbl_count = intel_crtc_get_vblank_counter(crtc); >> + u32 end_vbl_count = intel_crtc_get_vblank_counter(crtc, true); >> ktime_t end_vbl_time = ktime_get(); >> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >> >> diff --git a/drivers/gpu/drm/i915/display/intel_crtc.h >> b/drivers/gpu/drm/i915/display/intel_crtc.h >> index 8c14ff8b391ea..9826d800f3bb9 100644 >> --- a/drivers/gpu/drm/i915/display/intel_crtc.h >> +++ b/drivers/gpu/drm/i915/display/intel_crtc.h >> @@ -43,7 +43,7 @@ int intel_crtc_get_pipe_from_crtc_id_ioctl(struct >> drm_device *dev, void *data, >> struct intel_crtc_state *intel_crtc_state_alloc(struct intel_crtc *crtc); >> void intel_crtc_state_reset(struct intel_crtc_state *crtc_state, >> struct intel_crtc *crtc); >> -u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc); >> +u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc, bool >> update_vblank); >> void intel_crtc_vblank_on(const struct intel_crtc_state *crtc_state); >> void intel_crtc_vblank_off(const struct intel_crtc_state *crtc_state); >> void intel_pipe_update_start(struct intel_atomic_state *state, >> diff --git a/drivers/gpu/drm/i915/display/intel_display_trace.h >> b/drivers/gpu/drm/i915/display/intel_display_trace.h >> index 27ebc32cb61a5..4e9bea671effe 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display_trace.h >> +++ b/drivers/gpu/drm/i915/display/intel_display_trace.h >> @@ -10,6 +10,8 @@ >> #define TRACE_SYSTEM xe >> #endif >> >> +#define UPDATE_VBLANK (!IS_ENABLED(CONFIG_PREEMPT_RT)) > > So I'm thinking leave intel_crtc_get_vblank_counter() alone completely, > and hide all the ugly parts inside the trace file: > > #if IS_ENABLED(CONFIG_PREEMPT_RT) > /* Avoid irq lock in tracepoints with PREEMPT_RT=y */ > static inline u32 __trace_intel_crtc_get_vblank_counter(struct intel_crtc > *crtc) > { > struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(&crtc->base); > > if (!crtc->active) > return 0; > > if (!vblank->max_vblank_count) > return (u32)drm_crtc_accurate_vblank_count(&crtc->base); > return (u32)drm_crtc_vblank_count(&crtc->base); > > return crtc->base.funcs->get_vblank_counter(&crtc->base); > } > #else > #define __trace_intel_crtc_get_vblank_counter intel_crtc_get_vblank_counter > #endif > > And then > s/intel_crtc_get_vblank_counter/__trace_intel_crtc_get_vblank_counter/ > below.
There are only 2 users to the vblank counter, vblank evasion and trace points. If we're going to be forced to do this for tracepoints, the only user left is vblank evasion. I'm still a proponent of simply using drm_crtc_vblank_count on preempt-rt unconditionally, The only other user of intel_crtc_get_vblank_counter() is in intel_pipe_update_begin/end(), We do know vblanks are enabled during vblank evasion, and interrupts are no longer disabled. As a result, if a vblank occurred the non-accurate version will still be accurate enough, especially on longer delays. I want to repropose my original patch, where drm_crtc_vblank_count() is used on preempt-rt. Kind regards, ~Maarten
