Re: [PATCH 2/2] drm/i915/display: Make tracepoints work correctly on xe with PREEMPT_RT

[Maarten Lankhorst <dev@lankhorst.se>]

Hey,

Den 2025-08-29 kl. 13:14, skrev Jani Nikula:
> On Thu, 28 Aug 2025, Maarten Lankhorst <dev@lankhorst.se> 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 <dev@lankhorst.se>
>> ---
>>  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