Re: [RFC 12/14] drm/i915: Interface for controling engine stats collection

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

Hi Ben,

On 18/07/2017 15:36, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Enables other i915 components to enable and disable
> the facility as needed.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_engine_cs.c  | 53 +++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_ringbuffer.h |  5 ++++
>   2 files changed, 58 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 3e5e08c6b5ef..03e7459bad06 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -29,6 +29,8 @@
>   #include "intel_lrc.h"
>   
>   DEFINE_STATIC_KEY_FALSE(i915_engine_stats_key);
> +static DEFINE_MUTEX(i915_engine_stats_mutex);
> +static int i915_engine_stats_ref;
>   
>   /* Haswell does have the CXT_SIZE register however it does not appear to be
>    * valid. Now, docs explain in dwords what is in the context object. The full
> @@ -1340,6 +1342,57 @@ void intel_engines_mark_idle(struct drm_i915_private *i915)
>   	}
>   }
>   
> +int intel_enable_engine_stats(struct drm_i915_private *dev_priv)
> +{
> +	if (!i915.enable_execlists)
> +		return -ENODEV;
> +
> +	mutex_lock(&i915_engine_stats_mutex);
> +	if (i915_engine_stats_ref++ == 0) {
> +		struct intel_engine_cs *engine;
> +		enum intel_engine_id id;
> +
> +		for_each_engine(engine, dev_priv, id) {
> +			memset(&engine->stats, 0, sizeof(engine->stats));
> +			spin_lock_init(&engine->stats.lock);
> +		}
> +
> +		static_branch_enable(&i915_engine_stats_key);
> +	}
> +	mutex_unlock(&i915_engine_stats_mutex);
> +
> +	return 0;
> +}
> +
> +void intel_disable_engine_stats(void)
> +{
> +	mutex_lock(&i915_engine_stats_mutex);
> +	if (--i915_engine_stats_ref == 0)
> +		static_branch_disable(&i915_engine_stats_key);
> +	mutex_unlock(&i915_engine_stats_mutex);
> +}
> +
> +u64 intel_engine_get_current_busy_ns(struct intel_engine_cs *engine)
> +{
> +	unsigned long flags;
> +	u64 total;
> +
> +	spin_lock_irqsave(&engine->stats.lock, flags);
> +
> +	total = engine->stats.total;
> +
> +	/*
> +	 * If the engine is executing something at the moment
> +	 * add it to the total.
> +	 */
> +	if (engine->stats.ref)
> +		total += ktime_get_real_ns() - engine->stats.start;
> +
> +	spin_unlock_irqrestore(&engine->stats.lock, flags);
> +
> +	return total;
> +}
> +
>   #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
>   #include "selftests/mock_engine.c"
>   #endif
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 2eb1e970ad06..e0f495a6d0d9 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -776,4 +776,9 @@ static inline void intel_engine_context_out(struct intel_engine_cs *engine)
>   	}
>   }
>   
> +int intel_enable_engine_stats(struct drm_i915_private *i915);
> +void intel_disable_engine_stats(void);
> +
> +u64 intel_engine_get_current_busy_ns(struct intel_engine_cs *engine);

If we exported these symbols for other modules to use, what kind of API 
would they need? Presumably not per-engine but something to give the 
aggregated busyness of all engines? Or I have misunderstood you that 
there is this requirement?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx