Quoting Tvrtko Ursulin (2018-02-02 18:38:16)
> From: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
> 
> 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.ursu...@intel.com>
> Fixes: 99e48bf98dd0 ("drm/i915: Lock out execlist tasklet while peeking 
> inside for busy-stats")
> Testcase: igt/perf_pmu/enable-race-*
> Cc: Chris Wilson <ch...@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
> Cc: Jani Nikula <jani.nik...@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.v...@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> ---
>  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to