Re: [PATCH v3] drm/i915/tracing: Enable user interrupts while intel_engine_notify is active

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx