Quoting Matt Roper (2024-09-18 19:49:27-03:00)
>On Thu, Aug 29, 2024 at 07:00:47PM -0300, Gustavo Sousa wrote:
>> Tracepoints that display frame and scanline counters for all pipes were
>> added with commit 1489bba82433 ("drm/i915: Add cxsr toggle tracepoint")
>> and commit 0b2599a43ca9 ("drm/i915: Add pipe enable/disable
>> tracepoints"). At that time, we only had pipes A, B and C. Now that we
>> can also have pipe D, the TP_printk() calls are missing it.
>>
>> As a quick and dirty fix for that, let's define two common macros to be
>> used for the format and values respectively, and also ensure we raise a
>> build bug if more pipes are added to enum pipe.
>>
>> In the future, we should probably have a way of printing information for
>> available pipes only.
>>
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@xxxxxxxxx>
>
>I didn't did through the details of the tracepoint system, but I'm
>assuming you checked that the underlying structure is zero-allocated so
>that anything we don't specifically assign in TP_fast_assign will be 0
>rather than uninitialized garbage? E.g., on an ICL platform with only
>three pipes the pipe D output is guaranteed to be zero?
That's a good point. I actually missed doing this check. I just verified
this on a MTL machine by making the driver think pipe D is fused off and
I got some garbage in the trace data:
$ trace-cmd report -F 'i915/intel_pipe_\(enable\|disable\)' | grep -o 'pipe D: frame=.*' | sort | uniq -c
57 pipe D: frame=0, scanline=0
1 pipe D: frame=1752461056, scanline=11
1 pipe D: frame=4294936705, scanline=1752461056
1 pipe D: frame=48, scanline=0
1 pipe D: frame=740, scanline=6
2 pipe D: frame=808333872, scanline=0
1 pipe D: frame=976236602, scanline=66670
Then adding a patch to memset() the arrays to zero before the loop fixes
the issue:
$ trace-cmd report -F 'i915/intel_pipe_\(enable\|disable\)' | grep -o 'pipe D: frame=.*' | sort | uniq -c
64 pipe D: frame=0, scanline=0
Since this issue would be observed for fused-off pipes as well and not
only for platforms with less than 4 pipes, I'll send a v2 of this series
with such a patch. Such a patch will come before this one.
Thanks for catching this.
--
Gustavo Sousa
>
>Assuming that's the case,
>
>Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
>
>> ---
>> .../drm/i915/display/intel_display_trace.h | 43 +++++++++++++------
>> 1 file changed, 29 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_trace.h b/drivers/gpu/drm/i915/display/intel_display_trace.h
>> index 759b985c84a9..2ce66dffdfa5 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_trace.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_trace.h
>> @@ -30,6 +30,29 @@
>> #define _TRACE_PIPE_A 0
>> #define _TRACE_PIPE_B 1
>> #define _TRACE_PIPE_C 2
>> +#define _TRACE_PIPE_D 3
>> +
>> +/*
>> + * FIXME: Several TP_printk() calls below display frame and scanline numbers for
>> + * all possible pipes (regardless of whether they are available) and that is
>> + * done with a constant format string. A better approach would be to generate
>> + * that info dynamically based on available pipes, but, while we do not have
>> + * that implemented yet, let's assert that the constant format string indeed
>> + * covers all possible pipes.
>> + */
>> +static_assert(I915_MAX_PIPES - 1 == _TRACE_PIPE_D);
>> +
>> +#define _PIPES_FRAME_AND_SCANLINE_FMT \
>> + "pipe A: frame=%u, scanline=%u" \
>> + ", pipe B: frame=%u, scanline=%u" \
>> + ", pipe C: frame=%u, scanline=%u" \
>> + ", pipe D: frame=%u, scanline=%u"
>> +
>> +#define _PIPES_FRAME_AND_SCANLINE_VALUES \
>> + __entry->frame[_TRACE_PIPE_A], __entry->scanline[_TRACE_PIPE_A] \
>> + , __entry->frame[_TRACE_PIPE_B], __entry->scanline[_TRACE_PIPE_B] \
>> + , __entry->frame[_TRACE_PIPE_C], __entry->scanline[_TRACE_PIPE_C] \
>> + , __entry->frame[_TRACE_PIPE_D], __entry->scanline[_TRACE_PIPE_D]
>>
>> TRACE_EVENT(intel_pipe_enable,
>> TP_PROTO(struct intel_crtc *crtc),
>> @@ -52,11 +75,8 @@ TRACE_EVENT(intel_pipe_enable,
>> __entry->pipe_name = pipe_name(crtc->pipe);
>> ),
>>
>> - TP_printk("dev %s, pipe %c enable, pipe A: frame=%u, scanline=%u, pipe B: frame=%u, scanline=%u, pipe C: frame=%u, scanline=%u",
>> - __get_str(dev), __entry->pipe_name,
>> - __entry->frame[_TRACE_PIPE_A], __entry->scanline[_TRACE_PIPE_A],
>> - __entry->frame[_TRACE_PIPE_B], __entry->scanline[_TRACE_PIPE_B],
>> - __entry->frame[_TRACE_PIPE_C], __entry->scanline[_TRACE_PIPE_C])
>> + TP_printk("dev %s, pipe %c enable, " _PIPES_FRAME_AND_SCANLINE_FMT,
>> + __get_str(dev), __entry->pipe_name, _PIPES_FRAME_AND_SCANLINE_VALUES)
>> );
>>
>> TRACE_EVENT(intel_pipe_disable,
>> @@ -81,11 +101,8 @@ TRACE_EVENT(intel_pipe_disable,
>> __entry->pipe_name = pipe_name(crtc->pipe);
>> ),
>>
>> - TP_printk("dev %s, pipe %c disable, pipe A: frame=%u, scanline=%u, pipe B: frame=%u, scanline=%u, pipe C: frame=%u, scanline=%u",
>> - __get_str(dev), __entry->pipe_name,
>> - __entry->frame[_TRACE_PIPE_A], __entry->scanline[_TRACE_PIPE_A],
>> - __entry->frame[_TRACE_PIPE_B], __entry->scanline[_TRACE_PIPE_B],
>> - __entry->frame[_TRACE_PIPE_C], __entry->scanline[_TRACE_PIPE_C])
>> + TP_printk("dev %s, pipe %c disable, " _PIPES_FRAME_AND_SCANLINE_FMT,
>> + __get_str(dev), __entry->pipe_name, _PIPES_FRAME_AND_SCANLINE_VALUES)
>> );
>>
>> TRACE_EVENT(intel_crtc_flip_done,
>> @@ -211,11 +228,9 @@ TRACE_EVENT(intel_memory_cxsr,
>> __entry->new = new;
>> ),
>>
>> - TP_printk("dev %s, cxsr %s->%s, pipe A: frame=%u, scanline=%u, pipe B: frame=%u, scanline=%u, pipe C: frame=%u, scanline=%u",
>> + TP_printk("dev %s, cxsr %s->%s, " _PIPES_FRAME_AND_SCANLINE_FMT,
>> __get_str(dev), str_on_off(__entry->old), str_on_off(__entry->new),
>> - __entry->frame[_TRACE_PIPE_A], __entry->scanline[_TRACE_PIPE_A],
>> - __entry->frame[_TRACE_PIPE_B], __entry->scanline[_TRACE_PIPE_B],
>> - __entry->frame[_TRACE_PIPE_C], __entry->scanline[_TRACE_PIPE_C])
>> + _PIPES_FRAME_AND_SCANLINE_VALUES)
>> );
>>
>> TRACE_EVENT(g4x_wm,
>> --
>> 2.46.0
>>
>
>--
>Matt Roper
>Graphics Software Engineer
>Linux GPU Platform Enablement
>Intel Corporation
