[PATCH v2 5/5] drm/i915/display: Cover all possible pipes in TP_printk()

[Gustavo Sousa]

Quoting Ville Syrjälä (2024-09-23 16:23:27-03:00)
>On Mon, Sep 23, 2024 at 04:02:54PM -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.
>> 
>> Cc: Matt Roper <matthew.d.roper at intel.com>
>> Signed-off-by: Gustavo Sousa <gustavo.sousa at intel.com>
>> ---
>>  .../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 eec9aeddad96..9bd8f1e505b0 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_trace.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_trace.h
>> @@ -31,6 +31,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"
>
>Hmm. We have a lot of tracpoints that just print these for a single
>pipe. Is there any decent way to make this macro just for one pipe,
>and then resuse it for all the tracepoints whether they trace one
>pipe or all of them?

Maybe what we could do is to have a local struct pipe_counters type
and have _PIPE_COUNTERS_FMT and _PIPE_COUNTERS_VALUES for it. Then they
could be used here as well as for the single-pipe cases.

--
Gustavo Sousa

>
>> +
>> +#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]
>>  
>>  /*
>>   * Paranoid sanity check that at least the enumeration starts at the
>> @@ -63,11 +86,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,
>> @@ -96,11 +116,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,
>> @@ -230,11 +247,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.1
>
>-- 
>Ville Syrjälä
>Intel