[2/6] drm/i915: Allow clients to query own per-engine busyness

Commit Message

Tvrtko Ursulin Jan. 19, 2018, 1:45 p.m. UTC

From: Tvrtko Ursulin <[e-mail]>

Some customers want to know how much of the GPU time are their clients
using in order to make dynamic load balancing decisions.

With the accounting infrastructure in place in the previous patch, we add
a new context param (I915_CONTEXT_GET_ENGINE_BUSY) which takes a class and
instance of an engine for which the client would like to know its
utilization.

Utilization is reported as monotonically increasing number of nano-
seconds the engine spent executing jobs belonging to this context.

v2:
 * Use intel_context_engine_get_busy_time.
 * Refactor to only use struct_mutex while initially enabling engine
   stats.

v3:
 * Fix stats enabling.

Signed-off-by: Tvrtko Ursulin <[e-mail]>
Cc: [e-mail]
---
 drivers/gpu/drm/i915/i915_gem_context.c | 41 ++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/i915_gem_context.h |  1 +
 include/uapi/drm/i915_drm.h             | 11 ++++++++-
 3 files changed, 49 insertions(+), 4 deletions(-)

Comments

Chris Wilson Jan. 19, 2018, 9:08 p.m. UTC | #1

Quoting Tvrtko Ursulin (2018-01-19 13:45:24)
> +       case I915_CONTEXT_GET_ENGINE_BUSY:
> +               engine = intel_engine_lookup_user(i915, args->class,
> +                                                 args->instance);
> +               if (!engine) {
> +                       ret = -EINVAL;
> +                       break;
> +               }
> +
> +               ce = &ctx->engine[engine->id];
> +               if (!READ_ONCE(ce->stats.enabled)) {
> +                       ret = i915_mutex_lock_interruptible(dev);
> +                       if (!ret)
> +                               break;
> +
> +                       if (!ce->stats.enabled) {
> +                               ret = intel_enable_engine_stats(engine);

* Blink.

This caught me by surprise. (Other than struct_mutex) Not too offensive,
but surprising. At the very least call out to a function to handle the
request. Where did args->class, args->instance come from? You surely
didn't extend the ioctl struct just for that?
-Chris

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 648e7536ff51..256800fb1a3b 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -126,6 +126,9 @@  static void i915_gem_context_free(struct i915_gem_context *ctx)
 	for (i = 0; i < I915_NUM_ENGINES; i++) {
 		struct intel_context *ce = &ctx->engine[i];
 
+		if (ce->stats.enabled)
+			intel_disable_engine_stats(ctx->i915->engine[i]);
+
 		if (!ce->state)
 			continue;
 
@@ -713,7 +716,10 @@  int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
 {
 	struct drm_i915_file_private *file_priv = file->driver_priv;
 	struct drm_i915_gem_context_param *args = data;
+	struct drm_i915_private *i915 = to_i915(dev);
+	struct intel_engine_cs *engine;
 	struct i915_gem_context *ctx;
+	struct intel_context *ce;
 	int ret = 0;
 
 	ctx = i915_gem_context_lookup(file_priv, args->ctx_id);
@@ -731,10 +737,10 @@  int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
 	case I915_CONTEXT_PARAM_GTT_SIZE:
 		if (ctx->ppgtt)
 			args->value = ctx->ppgtt->base.total;
-		else if (to_i915(dev)->mm.aliasing_ppgtt)
-			args->value = to_i915(dev)->mm.aliasing_ppgtt->base.total;
+		else if (i915->mm.aliasing_ppgtt)
+			args->value = i915->mm.aliasing_ppgtt->base.total;
 		else
-			args->value = to_i915(dev)->ggtt.base.total;
+			args->value = i915->ggtt.base.total;
 		break;
 	case I915_CONTEXT_PARAM_NO_ERROR_CAPTURE:
 		args->value = i915_gem_context_no_error_capture(ctx);
@@ -745,6 +751,34 @@  int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
 	case I915_CONTEXT_PARAM_PRIORITY:
 		args->value = ctx->priority;
 		break;
+	case I915_CONTEXT_GET_ENGINE_BUSY:
+		engine = intel_engine_lookup_user(i915, args->class,
+						  args->instance);
+		if (!engine) {
+			ret = -EINVAL;
+			break;
+		}
+
+		ce = &ctx->engine[engine->id];
+		if (!READ_ONCE(ce->stats.enabled)) {
+			ret = i915_mutex_lock_interruptible(dev);
+			if (!ret)
+				break;
+
+			if (!ce->stats.enabled) {
+				ret = intel_enable_engine_stats(engine);
+				if (!ret)
+					break;
+				ce->stats.enabled = true;
+			}
+
+			mutex_unlock(&dev->struct_mutex);
+		}
+
+		args->value =
+			ktime_to_ns(intel_context_engine_get_busy_time(ctx,
+								       engine));
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -820,6 +854,7 @@  int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
 		}
 		break;
 
+	case I915_CONTEXT_GET_ENGINE_BUSY:
 	default:
 		ret = -EINVAL;
 		break;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
index 0ce8b9bf0f32..2312967a5890 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.h
+++ b/drivers/gpu/drm/i915/i915_gem_context.h
@@ -158,6 +158,7 @@  struct i915_gem_context {
 		u64 lrc_desc;
 		int pin_count;
 		struct {
+			bool enabled;
 			bool active;
 			ktime_t start;
 			ktime_t total;
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index c3b98fb8b691..642a7c15e9d9 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1468,7 +1468,16 @@  struct drm_i915_gem_context_param {
 #define   I915_CONTEXT_MAX_USER_PRIORITY	1023 /* inclusive */
 #define   I915_CONTEXT_DEFAULT_PRIORITY		0
 #define   I915_CONTEXT_MIN_USER_PRIORITY	-1023 /* inclusive */
-	__u64 value;
+#define I915_CONTEXT_GET_ENGINE_BUSY	0x7
+	union {
+		__u64 value;
+		struct {
+			__u8 pad[6]; /* unused */
+
+			__u8 class;
+			__u8 instance;
+		};
+	};
 };
 
 enum drm_i915_oa_format {