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

[Gustavo Sousa]

Quoting Gustavo Sousa (2024-09-23 17:47:08-03:00)
>Quoting Ville Syrjälä (2024-09-23 17:18:39-03:00)
>>On Mon, Sep 23, 2024 at 05:06:00PM -0300, Gustavo Sousa wrote:
>>> 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.
>>
>>Can we use structs here or would that confuse trace-cmd as well?
>
>Ugh... Right. I'm afraid that would not work indeed.
>
>I quickly scammed through libtraceevent's event-parse.c and it looks

Oops!

s/scammed/scanned/

:-)

--
Gustavo Sousa

>like it does not support that.
>
>--
>Gustavo Sousa