| Message ID | 20180809090751.24001-1-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] drm/i915/tracing: Enable user interrupts while intel_engine_notify is active | expand |
On 09/08/2018 10:07, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Keep the user interrupt enabled and emit intel_engine_notify tracepoint > every time as long as it is enabled. Premise is that if someone is > listening, they want to see interrupts logged. > > We use tracepoint (de)registration callbacks to enable user interrupts on > all devices (future proofing and avoiding ugly global pointers) and all > engines. And in the user interrupt handler we make sure > trace_intel_engine_notify is called even when there are no waiters. > > v2: > * Improve makefile. (Chris Wilson) > * Simplify by dropping the pointeless global driver list. (Chris Wilson) > * Emit tracepoint when there are no waiters, not just the user interrupt. > * Commit message tidy. > > v3: > * Favour one return from notify_ring. > Chris Wilson: > * Reword commit message a bit for clarity. > * Add RPM references to driver (un)registration paths. > * Rename list link member. > * Handle !CONFIG_TRACEPOINTS in the header. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com> > Cc: John Harrison <John.C.Harrison@intel.com> > Cc: svetlana.kukanova@intel.com > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Also forgot to credit for tracepoint hooks discovery: Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> Regards, Tvrtko > --- > drivers/gpu/drm/i915/Makefile | 3 + > drivers/gpu/drm/i915/i915_drv.c | 5 ++ > drivers/gpu/drm/i915/i915_drv.h | 2 + > drivers/gpu/drm/i915/i915_irq.c | 5 +- > drivers/gpu/drm/i915/i915_trace.h | 50 +++++++------- > drivers/gpu/drm/i915/i915_tracing.c | 100 ++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/i915_tracing.h | 29 ++++++++ > 7 files changed, 169 insertions(+), 25 deletions(-) > create mode 100644 drivers/gpu/drm/i915/i915_tracing.c > create mode 100644 drivers/gpu/drm/i915/i915_tracing.h > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > index 5794f102f9b8..dfc940b32078 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -182,6 +182,9 @@ i915-y += i915_perf.o \ > i915_oa_cnl.o \ > i915_oa_icl.o > > +# tracing > +i915-$(CONFIG_TRACEPOINTS) += i915_tracing.o > + > ifeq ($(CONFIG_DRM_I915_GVT),y) > i915-y += intel_gvt.o > include $(src)/gvt/Makefile > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 9dce55182c3a..03e224ebc28c 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1281,6 +1281,9 @@ static void i915_driver_register(struct drm_i915_private *dev_priv) > */ > if (INTEL_INFO(dev_priv)->num_pipes) > drm_kms_helper_poll_init(dev); > + > + /* Notify our tracepoints driver has been registered. */ > + i915_tracing_register(dev_priv); > } > > /** > @@ -1292,6 +1295,8 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv) > intel_fbdev_unregister(dev_priv); > intel_audio_deinit(dev_priv); > > + i915_tracing_unregister(dev_priv); > + > /* > * After flushing the fbdev (incl. a late async config which will > * have delayed queuing of a hotplug event), then flush the hotplug > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 0b10a30b7d96..00d9e9f65739 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2141,6 +2141,8 @@ struct drm_i915_private { > > struct i915_pmu pmu; > > + struct list_head tracing_link; > + > /* > * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch > * will be rejected. Instead look for a better place. > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 8084e35b25c5..0f007a46249e 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -1157,10 +1157,10 @@ static void notify_ring(struct intel_engine_cs *engine) > const u32 seqno = intel_engine_get_seqno(engine); > struct i915_request *rq = NULL; > struct task_struct *tsk = NULL; > - struct intel_wait *wait; > + struct intel_wait *wait = NULL; > > if (unlikely(!engine->breadcrumbs.irq_armed)) > - return; > + goto out; > > rcu_read_lock(); > > @@ -1219,6 +1219,7 @@ static void notify_ring(struct intel_engine_cs *engine) > > rcu_read_unlock(); > > +out: > trace_intel_engine_notify(engine, wait); > } > > diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h > index b50c6b829715..212e7fc1e80e 100644 > --- a/drivers/gpu/drm/i915/i915_trace.h > +++ b/drivers/gpu/drm/i915/i915_trace.h > @@ -8,6 +8,7 @@ > > #include <drm/drmP.h> > #include "i915_drv.h" > +#include "i915_tracing.h" > #include "intel_drv.h" > #include "intel_ringbuffer.h" > > @@ -780,29 +781,32 @@ trace_i915_request_out(struct i915_request *rq) > #endif > #endif > > -TRACE_EVENT(intel_engine_notify, > - TP_PROTO(struct intel_engine_cs *engine, bool waiters), > - TP_ARGS(engine, waiters), > - > - TP_STRUCT__entry( > - __field(u32, dev) > - __field(u16, class) > - __field(u16, instance) > - __field(u32, seqno) > - __field(bool, waiters) > - ), > - > - TP_fast_assign( > - __entry->dev = engine->i915->drm.primary->index; > - __entry->class = engine->uabi_class; > - __entry->instance = engine->instance; > - __entry->seqno = intel_engine_get_seqno(engine); > - __entry->waiters = waiters; > - ), > - > - TP_printk("dev=%u, engine=%u:%u, seqno=%u, waiters=%u", > - __entry->dev, __entry->class, __entry->instance, > - __entry->seqno, __entry->waiters) > +TRACE_EVENT_FN(intel_engine_notify, > + TP_PROTO(struct intel_engine_cs *engine, bool waiters), > + TP_ARGS(engine, waiters), > + > + TP_STRUCT__entry( > + __field(u32, dev) > + __field(u16, class) > + __field(u16, instance) > + __field(u32, seqno) > + __field(bool, waiters) > + ), > + > + TP_fast_assign( > + __entry->dev = engine->i915->drm.primary->index; > + __entry->class = engine->uabi_class; > + __entry->instance = engine->instance; > + __entry->seqno = intel_engine_get_seqno(engine); > + __entry->waiters = waiters; > + ), > + > + TP_printk("dev=%u, engine=%u:%u, seqno=%u, waiters=%u", > + __entry->dev, __entry->class, __entry->instance, > + __entry->seqno, __entry->waiters), > + > + intel_engine_notify_tracepoint_register, > + intel_engine_notify_tracepoint_unregister > ); > > DEFINE_EVENT(i915_request, i915_request_retire, > diff --git a/drivers/gpu/drm/i915/i915_tracing.c b/drivers/gpu/drm/i915/i915_tracing.c > new file mode 100644 > index 000000000000..65074f0fcf95 > --- /dev/null > +++ b/drivers/gpu/drm/i915/i915_tracing.c > @@ -0,0 +1,100 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright © 2018 Intel Corporation > + * > + */ > + > +#include "i915_tracing.h" > + > +#include "i915_drv.h" > +#include "intel_ringbuffer.h" > + > +static DEFINE_MUTEX(driver_list_lock); > +static LIST_HEAD(driver_list); > +static bool notify_enabled; > + > +static void __i915_enable_notify(struct drm_i915_private *i915) > +{ > + struct intel_engine_cs *engine; > + enum intel_engine_id id; > + > + intel_runtime_pm_get(i915); > + > + for_each_engine(engine, i915, id) > + intel_engine_pin_breadcrumbs_irq(engine); > + > + intel_runtime_pm_put(i915); > +} > + > +static void __i915_disable_notify(struct drm_i915_private *i915) > +{ > + struct intel_engine_cs *engine; > + enum intel_engine_id id; > + > + intel_runtime_pm_get(i915); > + > + for_each_engine(engine, i915, id) > + intel_engine_unpin_breadcrumbs_irq(engine); > + > + intel_runtime_pm_put(i915); > +} > + > +void i915_tracing_register(struct drm_i915_private *i915) > +{ > + INIT_LIST_HEAD(&i915->tracing_link); > + > + mutex_lock(&driver_list_lock); > + > + list_add_tail(&i915->tracing_link, &driver_list); > + > + if (notify_enabled) > + __i915_enable_notify(i915); > + > + mutex_unlock(&driver_list_lock); > +} > + > +void i915_tracing_unregister(struct drm_i915_private *i915) > +{ > + mutex_lock(&driver_list_lock); > + > + if (notify_enabled) > + __i915_disable_notify(i915); > + > + list_del(&i915->tracing_link); > + > + mutex_unlock(&driver_list_lock); > +} > + > +int intel_engine_notify_tracepoint_register(void) > +{ > + struct drm_i915_private *i915; > + > + mutex_lock(&driver_list_lock); > + > + GEM_BUG_ON(notify_enabled); > + > + list_for_each_entry(i915, &driver_list, tracing_link) > + __i915_enable_notify(i915); > + > + notify_enabled = true; > + > + mutex_unlock(&driver_list_lock); > + > + return 0; > +} > + > +void intel_engine_notify_tracepoint_unregister(void) > +{ > + struct drm_i915_private *i915; > + > + mutex_lock(&driver_list_lock); > + > + GEM_BUG_ON(!notify_enabled); > + > + list_for_each_entry(i915, &driver_list, tracing_link) > + __i915_disable_notify(i915); > + > + notify_enabled = false; > + > + mutex_unlock(&driver_list_lock); > +} > diff --git a/drivers/gpu/drm/i915/i915_tracing.h b/drivers/gpu/drm/i915/i915_tracing.h > new file mode 100644 > index 000000000000..4d8710f5687b > --- /dev/null > +++ b/drivers/gpu/drm/i915/i915_tracing.h > @@ -0,0 +1,29 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright © 2018 Intel Corporation > + * > + */ > +#ifndef _I915_TRACING_H_ > +#define _I915_TRACING_H_ > + > +#if IS_ENABLED(CONFIG_TRACEPOINTS) > + > +#include "i915_drv.h" > + > +void i915_tracing_register(struct drm_i915_private *i915); > +void i915_tracing_unregister(struct drm_i915_private *i915); > + > +int intel_engine_notify_tracepoint_register(void); > +void intel_engine_notify_tracepoint_unregister(void); > + > +#else > + > +static inline void i915_tracing_register(struct drm_i915_private *i915) { } > +static inline void i915_tracing_unregister(struct drm_i915_private *i915) { } > + > +static inline int intel_engine_notify_tracepoint_register(void) { } > +static inline void intel_engine_notify_tracepoint_unregister(void) { } > + > +#endif > + > +#endif >
Quoting Tvrtko Ursulin (2018-08-09 10:07:51) > diff --git a/drivers/gpu/drm/i915/i915_tracing.h b/drivers/gpu/drm/i915/i915_tracing.h > new file mode 100644 > index 000000000000..4d8710f5687b > --- /dev/null > +++ b/drivers/gpu/drm/i915/i915_tracing.h > @@ -0,0 +1,29 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright © 2018 Intel Corporation > + * > + */ > +#ifndef _I915_TRACING_H_ > +#define _I915_TRACING_H_ > + > +#if IS_ENABLED(CONFIG_TRACEPOINTS) > + > +#include "i915_drv.h" Nit here. You can just have a struct drm_i915_private; forward decl, that you need for both paths. > + > +void i915_tracing_register(struct drm_i915_private *i915); > +void i915_tracing_unregister(struct drm_i915_private *i915); > + > +int intel_engine_notify_tracepoint_register(void); > +void intel_engine_notify_tracepoint_unregister(void); > + > +#else > + > +static inline void i915_tracing_register(struct drm_i915_private *i915) { } > +static inline void i915_tracing_unregister(struct drm_i915_private *i915) { } > + > +static inline int intel_engine_notify_tracepoint_register(void) { } > +static inline void intel_engine_notify_tracepoint_unregister(void) { } > + > +#endif > + > +#endif >
On 09/08/2018 10:22, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2018-08-09 10:07:51) >> diff --git a/drivers/gpu/drm/i915/i915_tracing.h b/drivers/gpu/drm/i915/i915_tracing.h >> new file mode 100644 >> index 000000000000..4d8710f5687b >> --- /dev/null >> +++ b/drivers/gpu/drm/i915/i915_tracing.h >> @@ -0,0 +1,29 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright © 2018 Intel Corporation >> + * >> + */ >> +#ifndef _I915_TRACING_H_ >> +#define _I915_TRACING_H_ >> + >> +#if IS_ENABLED(CONFIG_TRACEPOINTS) >> + >> +#include "i915_drv.h" > > Nit here. You can just have a struct drm_i915_private; forward decl, > that you need for both paths. Yes thanks, I'll do that. I also need to add some comments to i915_tracing.c since it still looks rather like a prototype. Regards, Tvrtko >> + >> +void i915_tracing_register(struct drm_i915_private *i915); >> +void i915_tracing_unregister(struct drm_i915_private *i915); >> + >> +int intel_engine_notify_tracepoint_register(void); >> +void intel_engine_notify_tracepoint_unregister(void); >> + >> +#else >> + >> +static inline void i915_tracing_register(struct drm_i915_private *i915) { } >> +static inline void i915_tracing_unregister(struct drm_i915_private *i915) { } >> + >> +static inline int intel_engine_notify_tracepoint_register(void) { } >> +static inline void intel_engine_notify_tracepoint_unregister(void) { } >> + >> +#endif >> + >> +#endif >> > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 5794f102f9b8..dfc940b32078 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -182,6 +182,9 @@ i915-y += i915_perf.o \ i915_oa_cnl.o \ i915_oa_icl.o +# tracing +i915-$(CONFIG_TRACEPOINTS) += i915_tracing.o + ifeq ($(CONFIG_DRM_I915_GVT),y) i915-y += intel_gvt.o include $(src)/gvt/Makefile diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 9dce55182c3a..03e224ebc28c 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1281,6 +1281,9 @@ static void i915_driver_register(struct drm_i915_private *dev_priv) */ if (INTEL_INFO(dev_priv)->num_pipes) drm_kms_helper_poll_init(dev); + + /* Notify our tracepoints driver has been registered. */ + i915_tracing_register(dev_priv); } /** @@ -1292,6 +1295,8 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv) intel_fbdev_unregister(dev_priv); intel_audio_deinit(dev_priv); + i915_tracing_unregister(dev_priv); + /* * After flushing the fbdev (incl. a late async config which will * have delayed queuing of a hotplug event), then flush the hotplug diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 0b10a30b7d96..00d9e9f65739 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2141,6 +2141,8 @@ struct drm_i915_private { struct i915_pmu pmu; + struct list_head tracing_link; + /* * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch * will be rejected. Instead look for a better place. diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 8084e35b25c5..0f007a46249e 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -1157,10 +1157,10 @@ static void notify_ring(struct intel_engine_cs *engine) const u32 seqno = intel_engine_get_seqno(engine); struct i915_request *rq = NULL; struct task_struct *tsk = NULL; - struct intel_wait *wait; + struct intel_wait *wait = NULL; if (unlikely(!engine->breadcrumbs.irq_armed)) - return; + goto out; rcu_read_lock(); @@ -1219,6 +1219,7 @@ static void notify_ring(struct intel_engine_cs *engine) rcu_read_unlock(); +out: trace_intel_engine_notify(engine, wait); } diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h index b50c6b829715..212e7fc1e80e 100644 --- a/drivers/gpu/drm/i915/i915_trace.h +++ b/drivers/gpu/drm/i915/i915_trace.h @@ -8,6 +8,7 @@ #include <drm/drmP.h> #include "i915_drv.h" +#include "i915_tracing.h" #include "intel_drv.h" #include "intel_ringbuffer.h" @@ -780,29 +781,32 @@ trace_i915_request_out(struct i915_request *rq) #endif #endif -TRACE_EVENT(intel_engine_notify, - TP_PROTO(struct intel_engine_cs *engine, bool waiters), - TP_ARGS(engine, waiters), - - TP_STRUCT__entry( - __field(u32, dev) - __field(u16, class) - __field(u16, instance) - __field(u32, seqno) - __field(bool, waiters) - ), - - TP_fast_assign( - __entry->dev = engine->i915->drm.primary->index; - __entry->class = engine->uabi_class; - __entry->instance = engine->instance; - __entry->seqno = intel_engine_get_seqno(engine); - __entry->waiters = waiters; - ), - - TP_printk("dev=%u, engine=%u:%u, seqno=%u, waiters=%u", - __entry->dev, __entry->class, __entry->instance, - __entry->seqno, __entry->waiters) +TRACE_EVENT_FN(intel_engine_notify, + TP_PROTO(struct intel_engine_cs *engine, bool waiters), + TP_ARGS(engine, waiters), + + TP_STRUCT__entry( + __field(u32, dev) + __field(u16, class) + __field(u16, instance) + __field(u32, seqno) + __field(bool, waiters) + ), + + TP_fast_assign( + __entry->dev = engine->i915->drm.primary->index; + __entry->class = engine->uabi_class; + __entry->instance = engine->instance; + __entry->seqno = intel_engine_get_seqno(engine); + __entry->waiters = waiters; + ), + + TP_printk("dev=%u, engine=%u:%u, seqno=%u, waiters=%u", + __entry->dev, __entry->class, __entry->instance, + __entry->seqno, __entry->waiters), + + intel_engine_notify_tracepoint_register, + intel_engine_notify_tracepoint_unregister ); DEFINE_EVENT(i915_request, i915_request_retire, diff --git a/drivers/gpu/drm/i915/i915_tracing.c b/drivers/gpu/drm/i915/i915_tracing.c new file mode 100644 index 000000000000..65074f0fcf95 --- /dev/null +++ b/drivers/gpu/drm/i915/i915_tracing.c @@ -0,0 +1,100 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright © 2018 Intel Corporation + * + */ + +#include "i915_tracing.h" + +#include "i915_drv.h" +#include "intel_ringbuffer.h" + +static DEFINE_MUTEX(driver_list_lock); +static LIST_HEAD(driver_list); +static bool notify_enabled; + +static void __i915_enable_notify(struct drm_i915_private *i915) +{ + struct intel_engine_cs *engine; + enum intel_engine_id id; + + intel_runtime_pm_get(i915); + + for_each_engine(engine, i915, id) + intel_engine_pin_breadcrumbs_irq(engine); + + intel_runtime_pm_put(i915); +} + +static void __i915_disable_notify(struct drm_i915_private *i915) +{ + struct intel_engine_cs *engine; + enum intel_engine_id id; + + intel_runtime_pm_get(i915); + + for_each_engine(engine, i915, id) + intel_engine_unpin_breadcrumbs_irq(engine); + + intel_runtime_pm_put(i915); +} + +void i915_tracing_register(struct drm_i915_private *i915) +{ + INIT_LIST_HEAD(&i915->tracing_link); + + mutex_lock(&driver_list_lock); + + list_add_tail(&i915->tracing_link, &driver_list); + + if (notify_enabled) + __i915_enable_notify(i915); + + mutex_unlock(&driver_list_lock); +} + +void i915_tracing_unregister(struct drm_i915_private *i915) +{ + mutex_lock(&driver_list_lock); + + if (notify_enabled) + __i915_disable_notify(i915); + + list_del(&i915->tracing_link); + + mutex_unlock(&driver_list_lock); +} + +int intel_engine_notify_tracepoint_register(void) +{ + struct drm_i915_private *i915; + + mutex_lock(&driver_list_lock); + + GEM_BUG_ON(notify_enabled); + + list_for_each_entry(i915, &driver_list, tracing_link) + __i915_enable_notify(i915); + + notify_enabled = true; + + mutex_unlock(&driver_list_lock); + + return 0; +} + +void intel_engine_notify_tracepoint_unregister(void) +{ + struct drm_i915_private *i915; + + mutex_lock(&driver_list_lock); + + GEM_BUG_ON(!notify_enabled); + + list_for_each_entry(i915, &driver_list, tracing_link) + __i915_disable_notify(i915); + + notify_enabled = false; + + mutex_unlock(&driver_list_lock); +} diff --git a/drivers/gpu/drm/i915/i915_tracing.h b/drivers/gpu/drm/i915/i915_tracing.h new file mode 100644 index 000000000000..4d8710f5687b --- /dev/null +++ b/drivers/gpu/drm/i915/i915_tracing.h @@ -0,0 +1,29 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright © 2018 Intel Corporation + * + */ +#ifndef _I915_TRACING_H_ +#define _I915_TRACING_H_ + +#if IS_ENABLED(CONFIG_TRACEPOINTS) + +#include "i915_drv.h" + +void i915_tracing_register(struct drm_i915_private *i915); +void i915_tracing_unregister(struct drm_i915_private *i915); + +int intel_engine_notify_tracepoint_register(void); +void intel_engine_notify_tracepoint_unregister(void); + +#else + +static inline void i915_tracing_register(struct drm_i915_private *i915) { } +static inline void i915_tracing_unregister(struct drm_i915_private *i915) { } + +static inline int intel_engine_notify_tracepoint_register(void) { } +static inline void intel_engine_notify_tracepoint_unregister(void) { } + +#endif + +#endif