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

[Ben Widawsky <benjamin.widawsky@intel.com>]

On 17-07-19 10:34:14, Tvrtko Ursulin wrote:
>
>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

No misunderstanding. For our current usage, busyness of all engines would be
easiest. If one of the engines doesn't contribute much to the total TDP though,
it wouldn't need to actually be included, so we could perhaps leave room for
per-engine.

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx