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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Sep 23, 2024 at 05:48:29PM -0300, Gustavo Sousa wrote:
> 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@intel.com>
> >>> >> Signed-off-by: Gustavo Sousa <gustavo.sousa@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/
> 
> :-)

Hehe.

Oh well. I suppose we could have another similar macro for the
single pipe case. Or just no macros and hand roll it all. Shrug.
But we can also go with whatever you have already, and leave the
rest for the future.

Never used trace-cmd for this stuff, but I'll take your word
for it. Series is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

-- 
Ville Syrjälä
Intel