| Message ID | 20240829220106.80449-5-gustavo.sousa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Miscelaneous fixes for display tracepoints | expand |
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 >
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@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? 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@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
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,
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> --- .../drm/i915/display/intel_display_trace.h | 43 +++++++++++++------ 1 file changed, 29 insertions(+), 14 deletions(-)