Per-context and per-client engine busyness

From: Tvrtko Ursulin <[email protected]>

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 hooks already in place which track the overall engine busyness, we can extend that slightly to split that time between contexts.

v2: Fix accounting for tail updates.
v3: Rebase.
v4: Mark currently running contexts as active on stats enable.
v5: Include some headers to fix the build.

Signed-off-by Tvrtko Ursulin <[email protected]>

Cc [email protected]

---
 drivers/gpu/drm/i915/i915_gem_context.h |  8 ++++++
 drivers/gpu/drm/i915/intel_engine_cs.c  | 32 +++++++++++++++++++++
 drivers/gpu/drm/i915/intel_lrc.c        | 14 +++++----
 drivers/gpu/drm/i915/intel_ringbuffer.h | 50 +++++++++++++++++++++++++++++----
 4 files changed, 93 insertions(+), 11 deletions(-)

drivers/gpu/drm/i915/i915_gem_context.h +0 -0

index 4bfb72f8e1cb..7f5eebb67167 100644

drivers/gpu/drm/i915/i915_gem_context.c +6 -0

+       case I915_CONTEXT_GET_ENGINE_BUSY:
+               engine = intel_engine_lookup_user(i915, args->class,
+                                                 args->instance);
+               if (!engine) {
+                       ret = -EINVAL;
+                       break;

Quoting Tvrtko Ursulin (2018-01-19 13:45:24)

Quoted 6 lines

(quoted diff) +6 -0

+       case I915_CONTEXT_GET_ENGINE_BUSY:
+               engine = intel_engine_lookup_user(i915, args->class,
+                                                 args->instance);
+               if (!engine) {
+                       ret = -EINVAL;
+                       break;

Review

Quoted 10 lines

(quoted diff) +10 -0

+               }
+
+               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?

Quoted 20 lines

Quoting Tvrtko Ursulin (2018-01-19 13:45:24)

Quoted 6 lines

(quoted diff) +6 -0

+       case I915_CONTEXT_GET_ENGINE_BUSY:
+               engine = intel_engine_lookup_user(i915, args->class,
+                                                 args->instance);
+               if (!engine) {
+                       ret = -EINVAL;
+                       break;

Review

Quoted 10 lines

(quoted diff) +10 -0

+               }
+
+               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.

Quoted 4 lines

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?

Haven't extended it no, just did this:

include/uapi/drm/i915_drm.h +1 -1

@@ -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

Quoting Tvrtko Ursulin (2018-01-22 09:53:27)

Quoted 37 lines · Chris Wilson

Quoted 20 lines

Quoting Tvrtko Ursulin (2018-01-19 13:45:24)

Quoted 6 lines

(quoted diff) +6 -0

+       case I915_CONTEXT_GET_ENGINE_BUSY:
+               engine = intel_engine_lookup_user(i915, args->class,
+                                                 args->instance);
+               if (!engine) {
+                       ret = -EINVAL;
+                       break;

Review

Quoted 10 lines

(quoted diff) +10 -0

+               }
+
+               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.

Quoted 4 lines

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?

Haven't extended it no, just did this:

include/uapi/drm/i915_drm.h +1 -1

@@ -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

include/uapi/drm/i915_drm.h struct drm_i915_gem_context_param { · line 1481

Quoted 10 lines

include/uapi/drm/i915_drm.h +9 -0

+       union {
+               __u64 value;
+               struct {
+                       __u8 pad[6]; /* unused */
+
+                       __u8 class;
+                       __u8 instance;
+               };
+       };
   };

Not entirely happy about mixing in/out parameters. It's already complicated by being either an out value or an out pointer.

Closer to the original idea for context_getparam would be to return the array of engine values.

Quoted 52 lines

Quoting Tvrtko Ursulin (2018-01-22 09:53:27)

Quoted 37 lines · Chris Wilson

Quoted 20 lines

Quoting Tvrtko Ursulin (2018-01-19 13:45:24)

Quoted 6 lines

(quoted diff) +6 -0

+       case I915_CONTEXT_GET_ENGINE_BUSY:
+               engine = intel_engine_lookup_user(i915, args->class,
+                                                 args->instance);
+               if (!engine) {
+                       ret = -EINVAL;
+                       break;

Review

Quoted 10 lines

(quoted diff) +10 -0

+               }
+
+               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.

Quoted 4 lines

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?

Haven't extended it no, just did this:

include/uapi/drm/i915_drm.h +1 -1

@@ -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

include/uapi/drm/i915_drm.h struct drm_i915_gem_context_param { · line 1481

Quoted 10 lines

include/uapi/drm/i915_drm.h +9 -0

+       union {
+               __u64 value;
+               struct {
+                       __u8 pad[6]; /* unused */
+
+                       __u8 class;
+                       __u8 instance;
+               };
+       };
   };

Not entirely happy about mixing in/out parameters. It's already complicated by being either an out value or an out pointer.

Quoted 2 lines

Closer to the original idea for context_getparam would be to return the array of engine values.

It would have the advantage that multiple engines could be queried (more) atomically. How about then:

I915_CONTEXT_ENABLE_ENGINE_STATS:

value = &{
        __u32 num_engines;
        __u32 pad;
        
        struct {
                __u8 class;
                __u8 instance;
                __u8 pad[6];
        } [num_engines];
};

I915_CONTEXT_GET_ENGINE_STATS:

value = &{
        __u32 num_engines;
        __u32 pad;

        struct {
                __u8 class;
                __u8 instance;
                __u8 pad[6];

                __u64 busy;
        } [num_engines];
};

Quoting Tvrtko Ursulin (2018-01-22 11:45:04)

Quoted 71 lines · Chris Wilson

Quoted 52 lines

Quoting Tvrtko Ursulin (2018-01-22 09:53:27)

Quoted 37 lines · Chris Wilson

Quoted 20 lines

Quoting Tvrtko Ursulin (2018-01-19 13:45:24)

Quoted 6 lines

(quoted diff) +6 -0

+       case I915_CONTEXT_GET_ENGINE_BUSY:
+               engine = intel_engine_lookup_user(i915, args->class,
+                                                 args->instance);
+               if (!engine) {
+                       ret = -EINVAL;
+                       break;

Review

Quoted 10 lines

(quoted diff) +10 -0

+               }
+
+               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.

Quoted 4 lines

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?

Haven't extended it no, just did this:

include/uapi/drm/i915_drm.h +1 -1

@@ -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

include/uapi/drm/i915_drm.h struct drm_i915_gem_context_param { · line 1481

Quoted 10 lines

include/uapi/drm/i915_drm.h +9 -0

+       union {
+               __u64 value;
+               struct {
+                       __u8 pad[6]; /* unused */
+
+                       __u8 class;
+                       __u8 instance;
+               };
+       };
    };

Not entirely happy about mixing in/out parameters. It's already complicated by being either an out value or an out pointer.

Quoted 2 lines

Closer to the original idea for context_getparam would be to return the array of engine values.

It would have the advantage that multiple engines could be queried (more) atomically. How about then:

I915_CONTEXT_ENABLE_ENGINE_STATS:

value = &{
         __u32 num_engines;
         __u32 pad;
         
         struct {
                 __u8 class;
                 __u8 instance;
                 __u8 pad[6];
         } [num_engines];
};

Quoted 5 lines

I915_CONTEXT_GET_ENGINE_STATS:

value = &{
         __u32 num_engines;
         __u32 pad;

Quoted 6 lines

         struct {
                 __u8 class;
                 __u8 instance;
                 __u8 pad[6];

Quoted 3 lines

                 __u64 busy;
         } [num_engines];
};

Yes, that's what I had in mind. How does that work in practice? Does it make userspace too clumsy?

From: Tvrtko Ursulin <[email protected]>

Expose per-client and per-engine busyness under the previously added sysfs client root.

The new file is named 'busy' and contains a list of, one line for each engine, monotonically increasing nano-second resolution times each client's jobs were executing on the GPU.

$ cat /sys/class/drm/card0/clients/5/busy
32516602
0
0
0

This data can serve as an interface to implement a top like utility for GPU jobs. For instance I have prototyped a tool in IGT which produces periodic output like:

neverball[ 6011]: rcs0: 41.01% bcs0: 0.00% vcs0: 0.00% vecs0: 0.00%

      Xorg[  5664]:  rcs0:  31.16%  bcs0:   0.00%  vcs0:   0.00%  vecs0:   0.00%
     xfwm4[  5727]:  rcs0:   0.00%  bcs0:   0.00%  vcs0:   0.00%  vecs0:   0.00%

This tools can also be extended to use the i915 PMU and show overall engine busyness, and engine loads using the queue depth metric.

v2: Use intel_context_engine_get_busy_time.

Signed-off-by Tvrtko Ursulin <[email protected]>

---
  drivers/gpu/drm/i915/i915_drv.h |  1 +
  drivers/gpu/drm/i915/i915_gem.c | 66 +++++++++++++++++++++++++++++++++++++++++
  2 files changed, 67 insertions(+)

drivers/gpu/drm/i915/i915_drv.h +1 -0

index a2a883aaa59e..a96cfdfcba03 100644
@@ -350,6 +350,7 @@ struct drm_i915_file_private {
  	struct {
  		struct device_attribute pid;
  		struct device_attribute name;
+		struct device_attribute busy;
  	} attr;
  };
 

drivers/gpu/drm/i915/i915_gem.c +28 -0

index 7fdd892aa32a..c8753e4d66c4 100644
@@ -5538,6 +5538,57 @@ show_client_pid(struct device *kdev, struct device_attribute *attr, char *buf)
  	return snprintf(buf, PAGE_SIZE, "%u", file_priv->client_pid);
  }
 
+struct busy_ctx {
+	u64 total[I915_NUM_ENGINES];
+};
+
+static int busy_add(int _id, void *p, void *data)
+{
+	struct i915_gem_context *ctx = p;
+	struct busy_ctx *bc = data;
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
+
+	for_each_engine(engine, ctx->i915, id)
+		bc->total[id] +=
+		   ktime_to_ns(intel_context_engine_get_busy_time(ctx, engine));
+
+	return 0;
+}
+
+static ssize_t
+show_client_busy(struct device *kdev, struct device_attribute *attr, char *buf)
+{
+	struct drm_i915_file_private *file_priv =
+		container_of(attr, struct drm_i915_file_private, attr.busy);
+	struct drm_i915_private *i915 = file_priv->dev_priv;
+	unsigned int len = PAGE_SIZE;
+	struct busy_ctx bc = { };
+	ssize_t res = 0;
+	struct intel_engine_cs *engine;

drivers/gpu/drm/i915/i915_gem.c +26 -0

+
+	for_each_engine(engine, i915, id) {
+		ret = snprintf(buf, len, "%llu\n", bc.total[id]);
+		if (ret <= 0)
+			break;
+		res += ret;
+		len -= ret;
+		buf += ret;
+	}
+
+	return res;
+}
+
  int
  i915_gem_add_client(struct drm_i915_private *i915,
  		struct drm_i915_file_private *file_priv,

@@ -5578,10 +5629,23 @@ i915_gem_add_client(struct drm_i915_private *i915,
  	if (ret)
  		goto err_attr_pid;
 
+	attr = &file_priv->attr.busy;
+	attr->attr.name = "busy";
+	attr->attr.mode = 0444;
+	attr->show = show_client_busy;
+
+	ret = sysfs_create_file(file_priv->client_root,
+				(struct attribute *)attr);
+	if (ret)
+		goto err_attr_busy;
+
  	file_priv->client_pid = pid_nr(get_task_pid(task, PIDTYPE_PID));
 
  	return 0;
 
+err_attr_busy:
+	sysfs_remove_file(file_priv->client_root,
+			  (struct attribute *)&file_priv->attr.pid);
  err_attr_pid:
  	sysfs_remove_file(file_priv->client_root,
  			  (struct attribute *)&file_priv->attr.name);