Re: [PATCH v5] drm/i915/pmu: Always sample an active ringbuffer

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> As we no longer have a precise indication of requests queued to an
> engine, make no presumptions and just sample the ring registers to see
> if the engine is busy.
>
> v2: Report busy while the ring is idling on a semaphore/event.
> v3: Give the struct a name!
> v4: Always 0 outside the powerwell; trusting the powerwell is
> accurate enough for our sampling pmu.
> v5: Protect against gen7 mmio madness and try to improve grammar
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_pmu.c         | 64 +++++++++++--------------
>  drivers/gpu/drm/i915/intel_ringbuffer.h |  2 +-
>  2 files changed, 28 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index 13d70b90dd0f..247b67b82f0e 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -148,14 +148,6 @@ void i915_pmu_gt_unparked(struct drm_i915_private *i915)
>  	spin_unlock_irq(&i915->pmu.lock);
>  }
>  
> -static bool grab_forcewake(struct drm_i915_private *i915, bool fw)
> -{
> -	if (!fw)
> -		intel_uncore_forcewake_get(i915, FORCEWAKE_ALL);
> -
> -	return true;
> -}
> -
>  static void
>  add_sample(struct i915_pmu_sample *sample, u32 val)
>  {
> @@ -168,49 +160,47 @@ engines_sample(struct drm_i915_private *dev_priv, unsigned int period_ns)
>  	struct intel_engine_cs *engine;
>  	enum intel_engine_id id;
>  	intel_wakeref_t wakeref;
> -	bool fw = false;
>  
>  	if ((dev_priv->pmu.enable & ENGINE_SAMPLE_MASK) == 0)
>  		return;
>  
> -	if (!dev_priv->gt.awake)
> -		return;
> -
> -	wakeref = intel_runtime_pm_get_if_in_use(dev_priv);
> +	wakeref = 0;
> +	if (READ_ONCE(dev_priv->gt.awake))
> +		wakeref = intel_runtime_pm_get_if_in_use(dev_priv);
>  	if (!wakeref)
>  		return;
>  
> +	spin_lock_irq(&dev_priv->uncore.lock); /* prevent concurrent mmio */

I had to dig deep in memory. Perhaps I have wanted to forget.
But yeah, gen7 will blow up on concurrent mmio access.

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

>  	for_each_engine(engine, dev_priv, id) {
> -		u32 current_seqno = intel_engine_get_seqno(engine);
> -		u32 last_seqno = intel_engine_last_submit(engine);
> +		struct intel_engine_pmu *pmu = &engine->pmu;
> +		bool busy;
>  		u32 val;
>  
> -		val = !i915_seqno_passed(current_seqno, last_seqno);
> -
> -		if (val)
> -			add_sample(&engine->pmu.sample[I915_SAMPLE_BUSY],
> -				   period_ns);
> -
> -		if (val && (engine->pmu.enable &
> -		    (BIT(I915_SAMPLE_WAIT) | BIT(I915_SAMPLE_SEMA)))) {
> -			fw = grab_forcewake(dev_priv, fw);
> -
> -			val = I915_READ_FW(RING_CTL(engine->mmio_base));
> -		} else {
> -			val = 0;
> -		}
> +		val = I915_READ_FW(RING_CTL(engine->mmio_base));
> +		if (val == 0) /* powerwell off => engine idle */
> +			continue;
>  
>  		if (val & RING_WAIT)
> -			add_sample(&engine->pmu.sample[I915_SAMPLE_WAIT],
> -				   period_ns);
> -
> +			add_sample(&pmu->sample[I915_SAMPLE_WAIT], period_ns);
>  		if (val & RING_WAIT_SEMAPHORE)
> -			add_sample(&engine->pmu.sample[I915_SAMPLE_SEMA],
> -				   period_ns);
> -	}
> +			add_sample(&pmu->sample[I915_SAMPLE_SEMA], period_ns);
>  
> -	if (fw)
> -		intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> +		/*
> +		 * While waiting on a semaphore or event, MI_MODE reports the
> +		 * ring as idle. However, previously using the seqno, and with
> +		 * execlists sampling, we account for the ring waiting as the
> +		 * engine being busy. Therefore, we record the sample as being
> +		 * busy if either waiting or !idle.
> +		 */
> +		busy = val & (RING_WAIT_SEMAPHORE | RING_WAIT);
> +		if (!busy) {
> +			val = I915_READ_FW(RING_MI_MODE(engine->mmio_base));
> +			busy = !(val & MODE_IDLE);
> +		}
> +		if (busy)
> +			add_sample(&pmu->sample[I915_SAMPLE_BUSY], period_ns);
> +	}
> +	spin_unlock_irq(&dev_priv->uncore.lock);
>  
>  	intel_runtime_pm_put(dev_priv, wakeref);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 710ffb221775..5d45ad4ecca9 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -392,7 +392,7 @@ struct intel_engine_cs {
>  		bool irq_armed;
>  	} breadcrumbs;
>  
> -	struct {
> +	struct intel_engine_pmu {
>  		/**
>  		 * @enable: Bitmask of enable sample events on this engine.
>  		 *
> -- 
> 2.20.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx