Re: [Intel-gfx] [PATCH v2] drm/i915/tracing: Enable user interrupts while intel_engine_notify is active
Quoting Tvrtko Ursulin (2018-08-08 16:14:24)
> From: Tvrtko Ursulin
>
> 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.
>
> For this to work we also have to add another call site of
> trace_intel_engine_notify, notably to the early return from notify_ring
> otherwise we still depend on waiters being present.
That confused me. "We use... For this to work" I took as meaning we
needed the tracepoint in the irq to enable the user interrupts. But what
you meant was that we always want to fire the irq tracepoint when it is
being traced, irrespective of clients waiting.
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0b10a30b7d96..a15f8be0ece0 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 driver_list_link;
Nit: mostly we keep to _list being the list_head, and _link being the
node. Also link tends to indicate which list it is in, e.g
obj->vma_list and vma->obj_link -- but you'll be forgiven if that's
confusing and you have a better idea.
So i915_tracing_driver_list and 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..8cba798b666d 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1159,8 +1159,10 @@ static void notify_ring(struct intel_engine_cs *engine)
> struct task_struct *tsk = NULL;
> struct intel_wait *wait;
>
> - if (unlikely(!engine->breadcrumbs.irq_armed))
> + if (unlikely(!engine->breadcrumbs.irq_armed)) {
> + trace_intel_engine_notify(engine, false);
> return;
> + }
>
> rcu_read_lock();
>
> diff --git a/drivers/gpu/drm/i915/i915_trace.h
> b/drivers/gpu/drm/i915/i915_trace.h
> index c0352a1b036c..12555d2388fd 100644
> --- a/drivers/gpu/drm/i915/i915_trace.h
> +++ b/drivers/gpu/drm/i915/i915_trace.h
> @@ -8,6 +8,7 @@
>
> #include
> #include "i915_drv.h"
> +#include "i915_tracing.h"
> #include "intel_drv.h"
> #include "intel_ringbuffer.h"
>
> @@ -750,29 +751,32 @@ TRACE_EVENT(i915_request_out,
> __entry->global_seqno, __entry->completed)
> );
>
> -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,
> +
[Intel-gfx] [PATCH v2] drm/i915/tracing: Enable user interrupts while intel_engine_notify is active
From: Tvrtko Ursulin
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.
For this to work we also have to add another call site of
trace_intel_engine_notify, notably to the early return from notify_ring
otherwise we still depend on waiters being present.
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.
Signed-off-by: Tvrtko Ursulin
Cc: Chris Wilson
Cc: Dmitry Rogozhkin
Cc: John Harrison
Cc: svetlana.kukan...@intel.com
---
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 | 4 +-
drivers/gpu/drm/i915/i915_trace.h | 50 ---
drivers/gpu/drm/i915/i915_tracing.c | 98 +
drivers/gpu/drm/i915/i915_tracing.h | 13
7 files changed, 151 insertions(+), 24 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..a15f8be0ece0 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 driver_list_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..8cba798b666d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1159,8 +1159,10 @@ static void notify_ring(struct intel_engine_cs *engine)
struct task_struct *tsk = NULL;
struct intel_wait *wait;
- if (unlikely(!engine->breadcrumbs.irq_armed))
+ if (unlikely(!engine->breadcrumbs.irq_armed)) {
+ trace_intel_engine_notify(engine, false);
return;
+ }
rcu_read_lock();
diff --git a/drivers/gpu/drm/i915/i915_trace.h
b/drivers/gpu/drm/i915/i915_trace.h
index c0352a1b036c..12555d2388fd 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -8,6 +8,7 @@
#include
#include "i915_drv.h"
+#include "i915_tracing.h"
#include "intel_drv.h"
#include "intel_ringbuffer.h"
@@ -750,29 +751,32 @@ TRACE_EVENT(i915_request_out,
__entry->global_seqno, __entry->completed)
);
-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 =
