Quoting Tvrtko Ursulin (2018-02-02 18:38:16)
> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>
> Commit 99e48bf98dd0 ("drm/i915: Lock out execlist tasklet while peeking
> inside for busy-stats") added a tasklet_disable call in busy stats
> enabling, but we failed to understand that the PMU enable callback runs
> as an hard IRQ (IPI).
>
> Consequence of this is that the PMU enable callback can interrupt the
> execlists tasklet, and will then deadlock when it calls
> intel_engine_stats_enable->tasklet_disable.
>
> To fix this, I realized it is possible to move the engine stats enablement
> and disablement to PMU event init and destroy hooks. This allows for much
> simpler implementation since those hooks run in normal context (can
> sleep).
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> Fixes: 99e48bf98dd0 ("drm/i915: Lock out execlist tasklet while peeking inside for busy-stats")
> Testcase: igt/perf_pmu/enable-race-*
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> ---
> drivers/gpu/drm/i915/i915_pmu.c | 95 +++++++++++----------------------
> drivers/gpu/drm/i915/intel_ringbuffer.h | 14 -----
> 2 files changed, 31 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index ecb0198bfb7a..e97860b44004 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -287,7 +287,24 @@ static u64 count_interrupts(struct drm_i915_private *i915)
>
> static void i915_pmu_event_destroy(struct perf_event *event)
> {
> + struct drm_i915_private *i915 =
> + container_of(event->pmu, typeof(*i915), pmu.base);
> + struct intel_engine_cs *engine;
> +
> WARN_ON(event->parent);
> +
> + if (!is_engine_event(event))
> + return;
I think engine_event_destroy() for symmetry with engine_event_init() ?
> +
> + engine = intel_engine_lookup_user(i915,
> + engine_event_class(event),
> + engine_event_instance(event));
> + if (WARN_ON_ONCE(!engine))
> + return;
> +
> + if ((engine_event_sample(event) == I915_SAMPLE_BUSY) &&
Rogue ()
> + intel_engine_supports_stats(engine))
> + intel_disable_engine_stats(engine);
> }
>
> static int
> @@ -340,13 +357,23 @@ static int engine_event_init(struct perf_event *event)
> struct drm_i915_private *i915 =
> container_of(event->pmu, typeof(*i915), pmu.base);
> struct intel_engine_cs *engine;
> + u8 sample;
> + int ret;
>
> engine = intel_engine_lookup_user(i915, engine_event_class(event),
> engine_event_instance(event));
> if (!engine)
> return -ENODEV;
>
> - return engine_event_status(engine, engine_event_sample(event));
> + sample = engine_event_sample(event);
> + ret = engine_event_status(engine, sample);
> + if (ret)
> + return ret;
> +
> + if ((sample == I915_SAMPLE_BUSY) && intel_engine_supports_stats(engine))
> + ret = intel_enable_engine_stats(engine);
> +
> + return ret;
> }
Yeah, that is disgustingly simpler. On a second look nothing springs to
mind why this shouldn't work. Will sleep on it.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
