| Message ID | 20240405142737.920626-6-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: PREEMPT_RT related fixups. | expand |
On Fri, 5 Apr 2024 16:18:23 +0200 Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote: > The order of the header files is important. If this header file is > included after tracepoint.h was included then the NOTRACE here becomes a > nop. Currently this happens for two .c files which use the tracepoitns > behind DRM_I915_LOW_LEVEL_TRACEPOINTS. The NOTRACE should not be included in the individual trace files. Can you show where this is an issue. I think this is fixing the symptom and not the bug itself. -- Steve > > Cc: Steven Rostedt <rostedt@goodmis.org> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > --- > drivers/gpu/drm/i915/i915_trace.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h > index c54653cf72c95..3c51620d011b1 100644 > --- a/drivers/gpu/drm/i915/i915_trace.h > +++ b/drivers/gpu/drm/i915/i915_trace.h > @@ -326,7 +326,7 @@ DEFINE_EVENT(i915_request, i915_request_add, > TP_ARGS(rq) > ); > > -#if defined(CONFIG_DRM_I915_LOW_LEVEL_TRACEPOINTS) > +#if defined(CONFIG_DRM_I915_LOW_LEVEL_TRACEPOINTS) && !defined(NOTRACE) > DEFINE_EVENT(i915_request, i915_request_guc_submit, > TP_PROTO(struct i915_request *rq), > TP_ARGS(rq)
On 2024-04-08 13:06:50 [-0400], Steven Rostedt wrote: > On Fri, 5 Apr 2024 16:18:23 +0200 > Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote: > > > The order of the header files is important. If this header file is > > included after tracepoint.h was included then the NOTRACE here becomes a > > nop. Currently this happens for two .c files which use the tracepoitns > > behind DRM_I915_LOW_LEVEL_TRACEPOINTS. > > The NOTRACE should not be included in the individual trace files. > > Can you show where this is an issue. I think this is fixing the symptom > and not the bug itself. The previous patch in the series disables trace points. I just checked the difference with and without this one and there is none. I still have | # ls -1 /sys/kernel/debug/tracing/events/i915/ | enable | filter | g4x_wm | intel_cpu_fifo_underrun | intel_crtc_vblank_work_end | intel_crtc_vblank_work_start | intel_fbc_activate | intel_fbc_deactivate | intel_fbc_nuke | intel_frontbuffer_flush | intel_frontbuffer_invalidate | intel_memory_cxsr | intel_pch_fifo_underrun | intel_pipe_crc | intel_pipe_disable | intel_pipe_enable | intel_pipe_update_end | intel_pipe_update_start | intel_pipe_update_vblank_evaded | intel_plane_disable_arm | intel_plane_update_arm | intel_plane_update_noarm | vlv_fifo_size | vlv_wm and I *think* there were none. This then leads to | BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48 | in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 794, name: Xorg | preempt_count: 2, expected: 0 | RCU nest depth: 0, expected: 0 | 3 locks held by Xorg/794: | #0: ffffa49201d73c50 (crtc_ww_class_acquire){+.+.}-{0:0}, at: drm_mode_cursor_common+0xdf/0x250 [drm] | #1: ffff89ef11c46080 (crtc_ww_class_mutex){+.+.}-{3:3}, at: modeset_lock+0x68/0x1d0 [drm] | #2: ffff89ef2224ac70 (&uncore->lock){+.+.}-{2:2}, at: fwtable_read32+0x4d/0x280 [i915] | Preemption disabled at: | [<0000000000000000>] 0x0 | CPU: 3 PID: 794 Comm: Xorg Tainted: G W 6.9.0-rc3-rt1+ #15 | Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./Z68 Pro3-M, BIOS P2.10 04/24/2012 | Call Trace: | <TASK> | dump_stack_lvl+0x8d/0xb0 | __might_resched+0x1a3/0x260 | rt_spin_lock+0x48/0x100 | fwtable_read32+0x4d/0x280 [i915] | trace_event_raw_event_intel_pipe_update_start+0xeb/0x1d0 [i915] ^^ | intel_pipe_update_start+0x182/0x2f0 [i915] | intel_update_crtc+0x3f/0x400 [i915] | intel_commit_modeset_enables+0xab/0xd0 [i915] | intel_atomic_commit_tail+0x764/0x10b0 [i915] | intel_atomic_commit+0x318/0x360 [i915] | drm_atomic_commit+0x9e/0xd0 [drm] | drm_atomic_helper_disable_plane+0x87/0xe0 [drm_kms_helper] | drm_mode_cursor_universal+0x114/0x270 [drm] | drm_mode_cursor_common+0x11d/0x250 [drm] | drm_mode_cursor_ioctl+0x4b/0x70 [drm] | drm_ioctl_kernel+0xb4/0x110 [drm] | drm_ioctl+0x27b/0x4d0 [drm] among a few other things I was not aware of. So yes, this patch is not needed since it makes no difference but I still have trace points I would rather not have. If you a clue how to deal with this properly, I am all yours. > -- Steve Sebastian
On Tue, 9 Apr 2024 13:06:01 +0200 Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote: > among a few other things I was not aware of. > So yes, this patch is not needed since it makes no difference but I still > have trace points I would rather not have. > If you a clue how to deal with this properly, I am all yours. I believe you need to do it in the .c file: Can you try something like this? diff --git a/drivers/gpu/drm/i915/i915_trace_points.c b/drivers/gpu/drm/i915/i915_trace_points.c index 463a7177997c..ad0de6110133 100644 --- a/drivers/gpu/drm/i915/i915_trace_points.c +++ b/drivers/gpu/drm/i915/i915_trace_points.c @@ -8,7 +8,7 @@ #include "i915_drv.h" -#ifndef __CHECKER__ +#if !DEFINED(__CHECKER__) && !DEFINED(CONFIG_PREEMPT_RT) #define CREATE_TRACE_POINTS #include "i915_trace.h" #endif Or is it just that one set of trace events that is an issue? Could you just do: #if defined(CONFIG_DRM_I915_LOW_LEVEL_TRACEPOINTS) && !defined(CONFIG_PREEMPT_RT) ? -- Steve
On 2024-04-09 11:55:33 [-0400], Steven Rostedt wrote: > I believe you need to do it in the .c file: > > Can you try something like this? > … > ? I tried and nothing changed because the lowlevel config option isn't the problem. What about I drop this and replace 4/10 from this series with the patch below? After enabling all tracing I don't see any events in events/i915. Sebastian --------------------->8---------------------------- From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: Thu, 6 Dec 2018 09:52:20 +0100 Subject: [PATCH] drm/i915: Disable tracing points on PREEMPT_RT Luca Abeni reported this: | BUG: scheduling while atomic: kworker/u8:2/15203/0x00000003 | CPU: 1 PID: 15203 Comm: kworker/u8:2 Not tainted 4.19.1-rt3 #10 | Call Trace: | rt_spin_lock+0x3f/0x50 | gen6_read32+0x45/0x1d0 [i915] | g4x_get_vblank_counter+0x36/0x40 [i915] | trace_event_raw_event_i915_pipe_update_start+0x7d/0xf0 [i915] The tracing events use trace_i915_pipe_update_start() among other events use functions acquire spinlock_t locks which are transformed into sleeping locks on PREEMPT_RT. A few trace points use intel_get_crtc_scanline(), others use ->get_vblank_counter() wich also might acquire a sleeping locks on PREEMPT_RT. At the time the arguments are evaluated within trace point, preemption is disabled and so the locks must not be acquired on PREEMPT_RT. Based on this I don't see any other way than disable trace points on PREMPT_RT. Reported-by: Luca Abeni <lucabe72@gmail.com> Cc: Steven Rostedt <rostedt@goodmis.org> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- drivers/gpu/drm/i915/display/intel_display_trace.h | 4 ++++ drivers/gpu/drm/i915/i915_trace.h | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/drivers/gpu/drm/i915/display/intel_display_trace.h b/drivers/gpu/drm/i915/display/intel_display_trace.h index 7862e7cefe027..e4608d855bfba 100644 --- a/drivers/gpu/drm/i915/display/intel_display_trace.h +++ b/drivers/gpu/drm/i915/display/intel_display_trace.h @@ -9,6 +9,10 @@ #if !defined(__INTEL_DISPLAY_TRACE_H__) || defined(TRACE_HEADER_MULTI_READ) #define __INTEL_DISPLAY_TRACE_H__ +#ifdef CONFIG_PREEMPT_RT +#define NOTRACE +#endif + #include <linux/string_helpers.h> #include <linux/types.h> #include <linux/tracepoint.h> diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h index ce1cbee1b39dd..c54653cf72c95 100644 --- a/drivers/gpu/drm/i915/i915_trace.h +++ b/drivers/gpu/drm/i915/i915_trace.h @@ -6,6 +6,10 @@ #if !defined(_I915_TRACE_H_) || defined(TRACE_HEADER_MULTI_READ) #define _I915_TRACE_H_ +#ifdef CONFIG_PREEMPT_RT +#define NOTRACE +#endif + #include <linux/stringify.h> #include <linux/types.h> #include <linux/tracepoint.h>
On Wed, 10 Apr 2024 13:08:57 +0200 Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote: > On 2024-04-09 11:55:33 [-0400], Steven Rostedt wrote: > > I believe you need to do it in the .c file: > > > > Can you try something like this? > > > … > > ? > > I tried and nothing changed because the lowlevel config option isn't the > problem. What about I drop this and replace 4/10 from this series with > the patch below? After enabling all tracing I don't see any events in > events/i915. I don't see anything wrong with this approach. > > Sebastian > > --------------------->8---------------------------- > > From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > Date: Thu, 6 Dec 2018 09:52:20 +0100 > Subject: [PATCH] drm/i915: Disable tracing points on PREEMPT_RT > > Luca Abeni reported this: > | BUG: scheduling while atomic: kworker/u8:2/15203/0x00000003 > | CPU: 1 PID: 15203 Comm: kworker/u8:2 Not tainted 4.19.1-rt3 #10 > | Call Trace: > | rt_spin_lock+0x3f/0x50 > | gen6_read32+0x45/0x1d0 [i915] > | g4x_get_vblank_counter+0x36/0x40 [i915] > | trace_event_raw_event_i915_pipe_update_start+0x7d/0xf0 [i915] > > The tracing events use trace_i915_pipe_update_start() among other events > use functions acquire spinlock_t locks which are transformed into > sleeping locks on PREEMPT_RT. A few trace points use > intel_get_crtc_scanline(), others use ->get_vblank_counter() wich also > might acquire a sleeping locks on PREEMPT_RT. > At the time the arguments are evaluated within trace point, preemption > is disabled and so the locks must not be acquired on PREEMPT_RT. > > Based on this I don't see any other way than disable trace points on > PREMPT_RT. > > Reported-by: Luca Abeni <lucabe72@gmail.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > drivers/gpu/drm/i915/display/intel_display_trace.h | 4 ++++ > drivers/gpu/drm/i915/i915_trace.h | 4 ++++ > 2 files changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_trace.h b/drivers/gpu/drm/i915/display/intel_display_trace.h > index 7862e7cefe027..e4608d855bfba 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_trace.h > +++ b/drivers/gpu/drm/i915/display/intel_display_trace.h > @@ -9,6 +9,10 @@ > #if !defined(__INTEL_DISPLAY_TRACE_H__) || defined(TRACE_HEADER_MULTI_READ) > #define __INTEL_DISPLAY_TRACE_H__ > > +#ifdef CONFIG_PREEMPT_RT Hmm, should these be: #if defined(CONFIG_PREEMPT_RT) && !defined(NOTRACE) ? because it's not protected due to the TRACE_HEADER_MULTI_READ. -- Steve > +#define NOTRACE > +#endif > + > #include <linux/string_helpers.h> > #include <linux/types.h> > #include <linux/tracepoint.h> > diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h > index ce1cbee1b39dd..c54653cf72c95 100644 > --- a/drivers/gpu/drm/i915/i915_trace.h > +++ b/drivers/gpu/drm/i915/i915_trace.h > @@ -6,6 +6,10 @@ > #if !defined(_I915_TRACE_H_) || defined(TRACE_HEADER_MULTI_READ) > #define _I915_TRACE_H_ > > +#ifdef CONFIG_PREEMPT_RT > +#define NOTRACE > +#endif > + > #include <linux/stringify.h> > #include <linux/types.h> > #include <linux/tracepoint.h>
diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h index c54653cf72c95..3c51620d011b1 100644 --- a/drivers/gpu/drm/i915/i915_trace.h +++ b/drivers/gpu/drm/i915/i915_trace.h @@ -326,7 +326,7 @@ DEFINE_EVENT(i915_request, i915_request_add, TP_ARGS(rq) ); -#if defined(CONFIG_DRM_I915_LOW_LEVEL_TRACEPOINTS) +#if defined(CONFIG_DRM_I915_LOW_LEVEL_TRACEPOINTS) && !defined(NOTRACE) DEFINE_EVENT(i915_request, i915_request_guc_submit, TP_PROTO(struct i915_request *rq), TP_ARGS(rq)