[Intel-gfx] [RFC 2/2] drm/i915/tracing: Enable user interrupts while intel_engine_notify is active

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-06-25 18:32:37)
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> Keep the user interrupt enabled while intel_engine_notify tracepoint is
> enabled.
> 
> We use tracepoint (de)registration callbacks to enable user interrupts on
> all devices (future proofing and avoiding ugly global pointers) and all
> engines.
> 
> Premise is that if someone is listening, they want to see interrupts
> logged.
> 
> Commit to be improved...
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/Makefile       |  1 +
>  drivers/gpu/drm/i915/i915_drv.c     |  2 +
>  drivers/gpu/drm/i915/i915_drv.h     |  2 +
>  drivers/gpu/drm/i915/i915_trace.h   | 50 ++++++++-------
>  drivers/gpu/drm/i915/i915_tracing.c | 96 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_tracing.h | 13 ++++
>  6 files changed, 141 insertions(+), 23 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 4c6adae23e18..ee082addd328 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -77,6 +77,7 @@ i915-y += i915_cmd_parser.o \
>           i915_request.o \
>           i915_timeline.o \
>           i915_trace_points.o \
> +         i915_tracing.o \

No fancy obj-$(TRACING) ? :)

>           i915_vma.o \
>           intel_breadcrumbs.o \
>           intel_engine_cs.o \
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 8a3ea18d8416..c634583baf57 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1271,6 +1271,7 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
>         INIT_LIST_HEAD(&dev_priv->driver_list_link);
>         mutex_lock(&i915_driver_list_lock);
>         list_add_tail(&dev_priv->driver_list_link, &i915_driver_list);
> +       i915_tracing_register(dev_priv);
>         mutex_unlock(&i915_driver_list_lock);
>  }
>  
> @@ -1285,6 +1286,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
>  
>         mutex_lock(&i915_driver_list_lock);
>         list_del(&dev_priv->driver_list_link);
> +       i915_tracing_unregister(dev_priv);
>         mutex_unlock(&i915_driver_list_lock);
>  
>         /*
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 685bfdca3a72..4e3230713491 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2144,6 +2144,8 @@ struct drm_i915_private {
>  
>         struct list_head driver_list_link;
>  
> +       bool engine_notify_tracepoint;
> +
>         /*
>          * 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_tracing.c b/drivers/gpu/drm/i915/i915_tracing.c
> new file mode 100644
> index 000000000000..a9f486278109
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_tracing.c
> @@ -0,0 +1,96 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright © 2018 Intel Corporation
> + *
> + */
> +
> +#include "i915_tracing.h"
> +
> +#include "i915_drv.h"
> +#include "intel_ringbuffer.h"
> +
> +void i915_tracing_register(struct drm_i915_private *i915)
> +{
> +       struct intel_engine_cs *engine;
> +       struct drm_i915_private *p;
> +       enum intel_engine_id id;
> +       bool enable = false;
> +
> +       lockdep_assert_held(&i915_driver_list_lock);
> +
> +       list_for_each_entry(p, &i915_driver_list, driver_list_link) {
> +               enable = p->engine_notify_tracepoint;
> +               if (enable)
> +                       break;
> +       }

All on this list are expected to have the same value of
engine_notify_tracepoint. As far as I can tell, engine_notify_tracepoint
is just a global and there's no particular advantage in having it
per-device. The caveat is that i915_tracing_register/unregister must be
called under the same lock as the list_add/del -- I wonder if that
device list should just be moved to i915_tracing.c and kept entirely
private? (Then any future use case would also likely instantiate their
own mutex and list?)
-Chris