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

[Matt Roper <matthew.d.roper@intel.com>]

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@intel.com>

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?

Assuming that's the case,

Reviewed-by: Matt Roper <matthew.d.roper@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 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