Re: [PATCH 06/10] drm/i915/pmu: Wire up engine busy stats to PMU

Rogozhkin, Dmitry V
2017-10-04T17:52:32Z

Nice data, thank you!

-----Original Message-----
From: Tvrtko Ursulin [mailto:tvrtko.ursulin@linux.intel.com]
Sent: Wednesday, October 4, 2017 10:35 AM
To: Tvrtko Ursulin <tursulin@ursulin.net>; Intel-gfx@lists.freedesktop.org
Cc: Chris Wilson <chris@chris-wilson.co.uk>; Rogozhkin, Dmitry V <dmitry.v.rogozhkin@intel.com>
Subject: Re: [Intel-gfx] [PATCH 06/10] drm/i915/pmu: Wire up engine busy stats to PMU

On 29/09/2017 13:34, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> We can use engine busy stats instead of the sampling timer for better
> accuracy.
>
> By doing this we replace the stohastic sampling with busyness metric
> derived directly from engine activity. This is context switch
> interrupt driven, so as accurate as we can get from software tracking.

To quantify the precision benefit with a graph:

https://people.freedesktop.org/~tursulin/sampling-vs-busy-stats.png

This represents the render engine busyness on neverball menu screen looping a few times. PMU sampling interval is 100ms. It was generated from a special build with both PMU counters running in parallel so the data is absolutely aligned.

Anomaly of busyness going over 100% is caused by me not bothering to use the actual timestamps got from PMU, but using fixed interval (perf stat -I 100). But I don't think it matters for the rough comparison and looking at the noise and error in the sampling version.

> As a secondary benefit, we can also not run the sampling timer in
> cases only busyness metric is enabled.

For this one data is not showing anything significant. I've seen the i915_sample function use <0.1% of CPU time but that's it. Probably with the original version which used MMIO it was much worse.

Regards,

Tvrtko

> v2: Rebase.
> v3:
> * Rebase, comments.
> * Leave engine busyness controls out of workers.
> v4: Checkpatch cleanup.
> v5: Added comment to pmu_needs_timer change.
> v6:
> * Rebase.
> * Fix style of some comments. (Chris Wilson)
> v7: Rebase and commit message update. (Chris Wilson)
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/i915_pmu.c | 37 +++++++++++++++++++++++++++++++--
> drivers/gpu/drm/i915/intel_ringbuffer.h | 5 +++++
> 2 files changed, 40 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c
> b/drivers/gpu/drm/i915/i915_pmu.c index f341c904c159..93c0e7ec7d75
> 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -90,6 +90,11 @@ static unsigned int event_enabled_bit(struct perf_event *event)
> return config_enabled_bit(event->attr.config);
> }
>
> +static bool supports_busy_stats(void) {
> + return i915_modparams.enable_execlists; }
> +
> static bool pmu_needs_timer(struct drm_i915_private *i915, bool gpu_active)
> {
> u64 enable;
> @@ -115,6 +120,12 @@ static bool pmu_needs_timer(struct drm_i915_private *i915, bool gpu_active)
> */
> if (!gpu_active)
> enable &= ~ENGINE_SAMPLE_MASK;
> + /*
> + * Also there is software busyness tracking available we do not
> + * need the timer for I915_SAMPLE_BUSY counter.
> + */
> + else if (supports_busy_stats())
> + enable &= ~BIT(I915_SAMPLE_BUSY);
>
> /*
> * If some bits remain it means we need the sampling timer running.
> @@ -362,6 +373,9 @@ static u64 __i915_pmu_event_read(struct perf_event
> *event)
>
> if (WARN_ON_ONCE(!engine)) {
> /* Do nothing */
> + } else if (sample == I915_SAMPLE_BUSY &&
> + engine->pmu.busy_stats) {
> + val = ktime_to_ns(intel_engine_get_busy_time(engine));
> } else {
> val = engine->pmu.sample[sample].cur;
> }
> @@ -398,6 +412,12 @@ static void i915_pmu_event_read(struct perf_event *event)
> local64_add(new - prev, &event->count);
> }
>
> +static bool engine_needs_busy_stats(struct intel_engine_cs *engine) {
> + return supports_busy_stats() &&
> + (engine->pmu.enable & BIT(I915_SAMPLE_BUSY)); }
> +
> static void i915_pmu_enable(struct perf_event *event)
> {
> struct drm_i915_private *i915 =
> @@ -437,7 +457,14 @@ static void i915_pmu_enable(struct perf_event
> *event)
>
> GEM_BUG_ON(sample >= I915_PMU_SAMPLE_BITS);
> GEM_BUG_ON(engine->pmu.enable_count[sample] == ~0);
> - engine->pmu.enable_count[sample]++;
> + if (engine->pmu.enable_count[sample]++ == 0) {
> + if (engine_needs_busy_stats(engine) &&
> + !engine->pmu.busy_stats) {
> + engine->pmu.busy_stats =
> + intel_enable_engine_stats(engine) == 0;
> + WARN_ON_ONCE(!engine->pmu.busy_stats);
> + }
> + }
> }
>
> /*
> @@ -473,8 +500,14 @@ static void i915_pmu_disable(struct perf_event *event)
> * Decrement the reference count and clear the enabled
> * bitmask when the last listener on an event goes away.
> */
> - if (--engine->pmu.enable_count[sample] == 0)
> + if (--engine->pmu.enable_count[sample] == 0) {
> engine->pmu.enable &= ~BIT(sample);
> + if (!engine_needs_busy_stats(engine) &&
> + engine->pmu.busy_stats) {
> + engine->pmu.busy_stats = false;
> + intel_disable_engine_stats(engine);
> + }
> + }
> }
>
> GEM_BUG_ON(bit >= I915_PMU_MASK_BITS); diff --git
> a/drivers/gpu/drm/i915/intel_ringbuffer.h
> b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index ee738c7694e5..0f8ccb243407 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -351,6 +351,11 @@ struct intel_engine_cs {
> * Our internal timer stores the current counters in this field.
> */
> struct i915_pmu_sample sample[I915_ENGINE_SAMPLE_MAX];
> + /**
> + * @busy_stats: Has enablement of engine stats tracking been
> + * requested.
> + */
> + bool busy_stats;
> } pmu;
>
> /*
>