Hey,
Den 2025-08-29 kl. 14:08, skrev Sebastian Andrzej Siewior:
> On 2025-08-29 14:14:10 [+0300], Jani Nikula wrote:
>>> --- 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.
>
> If I may: There is also the possibility to use the _enabled() function of
> a trace point. So instead trace_intel_pipe_enable() you would so
> something like
>
> | void do_trace_intel_pipe_enable(struct intel_crtc *crtc)
> | {
> | if (!trace_intel_pipe_enable_enabled())
> | return;
> | trace_intel_pipe(crtc,
> | intel_crtc_get_vblank_counter(crtc));
> | }
>
> The _enabled() macro uses the static branches as the actual TP. The
> intel_crtc_get_vblank_counter() would only be evaluated if the TP is
> enabled and passed early.
That requires adding new macros for 23 tracepoints for something that only
affects preempt-rt,
and even then only on a specific type of uncommon output (DSI).
Aren't we also completely duplicating the functionality of tracepoints then?
>> BR,
>> Jani.
>
> Sebastian
Kind regards,
~maarten Lankhorst
