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

[Tvrtko Ursulin]

On 08/08/2018 12:11, Chris Wilson wrote:
> 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) ? :)

Just a miss, marking as TODO.

> 
>>            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?)

Agreed, probably better to keep it private since exported list is 
totally useless now. If there are no fundamental objections on the idea 
I'll re-spin with that change.

Regards,

Tvrtko