[PATCH v2 0/6] Per-context and per-client engine busyness

[PATCH v2 0/6] Per-context and per-client engine busyness

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

I have sent this as part of a larger series back in October '17.

First part of it is implementing a customer requirement to be able to query
engine utilization on their own contexts. This is done in patch 2, which falls
under the standard open source userspace requirements etc.

The rest of the series implements per-client engine utilization stats, something
which I think is quite interesting and missing from our stack at the moment. And
in fact, in the latest other OS update I have noticed they actually can expose
this statistics.

I have prototyped a simple top-like utility which can show you the overall
engine stats (data coming from recently merged PMU), and then using the sysfs
API from this series a sorted breakdown of currently running clients. Output of
that looks like this:

  rcs0:  88.89% busy (  0.27 qd),   0.00% wait,   0.00% sema
  bcs0:  27.50% busy (  0.00 qd),   0.00% wait,   0.00% sema
  vcs0:  86.75% busy (  0.00 qd),   0.00% wait,   0.00% sema
 vecs0:   0.00% busy (  0.00 qd),   0.00% wait,   0.00% sema

         gem_wsim[ 21806]:  rcs0:  43.18%  bcs0:  27.52%  vcs0:  86.80%  vecs0:   0.00%
             Xorg[ 21451]:  rcs0:  33.60%  bcs0:   0.00%  vcs0:   0.00%  vecs0:   0.00%
  chromium-browse[ 21670]:  rcs0:  12.16%  bcs0:   0.00%  vcs0:   0.00%  vecs0:   0.00%
             Xorg[ 21451]:  rcs0:   0.00%  bcs0:   0.00%  vcs0:   0.00%  vecs0:   0.00%
            xfwm4[ 21506]:  rcs0:   0.00%  bcs0:   0.00%  vcs0:   0.00%  vecs0:   0.00%

Patch series needs some more work, for instance the summation of busyness per-
client is not the most efficient, standard debugging and so, but seems to work
reasonably well so far.

What is also possible to do on top of this series, is to extend our current PMU
interface to allow per-task profiling via perf tool as well. I have started
implementing that at one point and could go back to it.

Tvrtko Ursulin (6):
  drm/i915: Track per-context engine busyness
  drm/i915: Allow clients to query own per-engine busyness
  drm/i915: Expose list of clients in sysfs
  drm/i915: Update client name on context create
  drm/i915: Expose per-engine client busyness
  drm/i915: Add sysfs toggle to enable per-client engine stats

 drivers/gpu/drm/i915/i915_drv.h         |  24 +++++
 drivers/gpu/drm/i915/i915_gem.c         | 178 ++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/i915_gem_context.c |  56 +++++++++-
 drivers/gpu/drm/i915/i915_gem_context.h |   6 ++
 drivers/gpu/drm/i915/i915_sysfs.c       |  80 ++++++++++++++
 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 +++++++--
 include/uapi/drm/i915_drm.h             |  11 +-
 9 files changed, 427 insertions(+), 24 deletions(-)

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

[PATCH 1/6] drm/i915: Track per-context engine busyness

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

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.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: gordon.kelly@intel.com
---
 drivers/gpu/drm/i915/i915_gem_context.h |  5 ++++
 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, 90 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
index 4bfb72f8e1cb..0ce8b9bf0f32 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.h
+++ b/drivers/gpu/drm/i915/i915_gem_context.h
@@ -157,6 +157,11 @@ struct i915_gem_context {
 		u32 *lrc_reg_state;
 		u64 lrc_desc;
 		int pin_count;
+		struct {
+			bool active;
+			ktime_t start;
+			ktime_t total;
+		} stats;
 	} engine[I915_NUM_ENGINES];
 
 	/** ring_size: size for allocating the per-engine ring buffer */
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index d572b18d39eb..9907ceedfa90 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1966,6 +1966,16 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine)
 
 		engine->stats.enabled_at = ktime_get();
 
+		/* Mark currently running context as active. */
+		if (port_isset(port)) {
+			struct drm_i915_gem_request *req = port_request(port);
+			struct intel_context *ce =
+					&req->ctx->engine[engine->id];
+
+			ce->stats.start = engine->stats.enabled_at;
+			ce->stats.active = true;
+		}
+
 		/* XXX submission method oblivious? */
 		while (num_ports-- && port_isset(port)) {
 			engine->stats.active++;
@@ -2038,6 +2048,28 @@ void intel_disable_engine_stats(struct intel_engine_cs *engine)
 	spin_unlock_irqrestore(&engine->stats.lock, flags);
 }
 
+ktime_t intel_context_engine_get_busy_time(struct i915_gem_context *ctx,
+					   struct intel_engine_cs *engine)
+{
+	struct intel_context *ce;
+	unsigned long flags;
+	ktime_t total;
+
+	ce = &ctx->engine[engine->id];
+
+	spin_lock_irqsave(&engine->stats.lock, flags);
+
+	total = ce->stats.total;
+
+	if (ce->stats.active)
+		total = ktime_add(total,
+				  ktime_sub(ktime_get(), ce->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_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 24ce781d39b7..a82ad5da6090 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -380,16 +380,19 @@ execlists_context_status_change(struct drm_i915_gem_request *rq,
 }
 
 static inline void
-execlists_context_schedule_in(struct drm_i915_gem_request *rq)
+execlists_context_schedule_in(struct drm_i915_gem_request *rq,
+			      unsigned int port)
 {
 	execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_IN);
-	intel_engine_context_in(rq->engine);
+	intel_engine_context_in(rq->engine,
+				&rq->ctx->engine[rq->engine->id],
+				port == 0);
 }
 
 static inline void
 execlists_context_schedule_out(struct drm_i915_gem_request *rq)
 {
-	intel_engine_context_out(rq->engine);
+	intel_engine_context_out(rq->engine, &rq->ctx->engine[rq->engine->id]);
 	execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_OUT);
 }
 
@@ -442,7 +445,7 @@ static void execlists_submit_ports(struct intel_engine_cs *engine)
 		if (rq) {
 			GEM_BUG_ON(count > !n);
 			if (!count++)
-				execlists_context_schedule_in(rq);
+				execlists_context_schedule_in(rq, n);
 			port_set(&port[n], port_pack(rq, count));
 			desc = execlists_update_context(rq);
 			GEM_DEBUG_EXEC(port[n].context_id = upper_32_bits(desc));
@@ -703,7 +706,8 @@ execlists_cancel_port_requests(struct intel_engine_execlists * const execlists)
 		struct drm_i915_gem_request *rq = port_request(port);
 
 		GEM_BUG_ON(!execlists->active);
-		intel_engine_context_out(rq->engine);
+		intel_engine_context_out(rq->engine,
+					 &rq->ctx->engine[rq->engine->id]);
 		execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_PREEMPTED);
 		i915_gem_request_put(rq);
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index b198df1f248c..27b727cf4017 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -4,6 +4,7 @@
 
 #include <linux/hashtable.h>
 #include "i915_gem_batch_pool.h"
+#include "i915_gem_context.h"
 #include "i915_gem_request.h"
 #include "i915_gem_timeline.h"
 #include "i915_pmu.h"
@@ -1036,25 +1037,42 @@ void intel_engine_dump(struct intel_engine_cs *engine,
 struct intel_engine_cs *
 intel_engine_lookup_user(struct drm_i915_private *i915, u8 class, u8 instance);
 
-static inline void intel_engine_context_in(struct intel_engine_cs *engine)
+static inline void
+intel_engine_context_in(struct intel_engine_cs *engine,
+			struct intel_context *ce,
+			bool submit)
 {
 	unsigned long flags;
+	ktime_t now;
 
 	if (READ_ONCE(engine->stats.enabled) == 0)
 		return;
 
 	spin_lock_irqsave(&engine->stats.lock, flags);
 
+	if (submit) {
+		now = ktime_get();
+		ce->stats.start = now;
+		ce->stats.active = true;
+	} else {
+		now = 0;
+	}
+
 	if (engine->stats.enabled > 0) {
-		if (engine->stats.active++ == 0)
-			engine->stats.start = ktime_get();
+		if (engine->stats.active++ == 0) {
+			if (!now)
+				now = ktime_get();
+			engine->stats.start = now;
+		}
 		GEM_BUG_ON(engine->stats.active == 0);
 	}
 
 	spin_unlock_irqrestore(&engine->stats.lock, flags);
 }
 
-static inline void intel_engine_context_out(struct intel_engine_cs *engine)
+static inline void
+intel_engine_context_out(struct intel_engine_cs *engine,
+			 struct intel_context *ce)
 {
 	unsigned long flags;
 
@@ -1064,14 +1082,31 @@ static inline void intel_engine_context_out(struct intel_engine_cs *engine)
 	spin_lock_irqsave(&engine->stats.lock, flags);
 
 	if (engine->stats.enabled > 0) {
+		struct execlist_port *next_port = &engine->execlists.port[1];
+		ktime_t now = ktime_get();
 		ktime_t last;
 
+		GEM_BUG_ON(!ce->stats.start);
+		ce->stats.total = ktime_add(ce->stats.total,
+					    ktime_sub(now, ce->stats.start));
+		ce->stats.active = false;
+
+		if (port_isset(next_port)) {
+			struct drm_i915_gem_request *next_req =
+						port_request(next_port);
+			struct intel_context *next_ce =
+					&next_req->ctx->engine[engine->id];
+
+			next_ce->stats.start = now;
+			next_ce->stats.active = true;
+		}
+
 		if (engine->stats.active && --engine->stats.active == 0) {
 			/*
 			 * Decrement the active context count and in case GPU
 			 * is now idle add up to the running total.
 			 */
-			last = ktime_sub(ktime_get(), engine->stats.start);
+			last = ktime_sub(now, engine->stats.start);
 
 			engine->stats.total = ktime_add(engine->stats.total,
 							last);
@@ -1081,7 +1116,7 @@ static inline void intel_engine_context_out(struct intel_engine_cs *engine)
 			 * the first event in which case we account from the
 			 * time stats gathering was turned on.
 			 */
-			last = ktime_sub(ktime_get(), engine->stats.enabled_at);
+			last = ktime_sub(now, engine->stats.enabled_at);
 
 			engine->stats.total = ktime_add(engine->stats.total,
 							last);
@@ -1091,6 +1126,9 @@ static inline void intel_engine_context_out(struct intel_engine_cs *engine)
 	spin_unlock_irqrestore(&engine->stats.lock, flags);
 }
 
+ktime_t intel_context_engine_get_busy_time(struct i915_gem_context *ctx,
+					   struct intel_engine_cs *engine);
+
 int intel_enable_engine_stats(struct intel_engine_cs *engine);
 void intel_disable_engine_stats(struct intel_engine_cs *engine);
 
-- 
2.14.1

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

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

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

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 <tvrtko.ursulin@intel.com>
Cc: gordon.kelly@intel.com
---
 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(-)

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 {
-- 
2.14.1

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

[PATCH 3/6] drm/i915: Expose list of clients in sysfs

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Expose a list of clients with open file handles in sysfs.

This will be a basis for a top-like utility showing per-client and per-
engine GPU load.

Currently we only expose each client's pid and name under opaque numbered
directories in /sys/class/drm/card0/clients/.

For instance:

/sys/class/drm/card0/clients/3/name: Xorg
/sys/class/drm/card0/clients/3/pid: 5664

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h   |  13 +++++
 drivers/gpu/drm/i915/i915_gem.c   | 112 +++++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/i915_sysfs.c |   8 +++
 3 files changed, 126 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 317953484fec..2414aa430f65 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -341,6 +341,16 @@ struct drm_i915_file_private {
  */
 #define I915_MAX_CLIENT_CONTEXT_BANS 3
 	atomic_t context_bans;
+
+	unsigned int client_sysfs_id;
+	unsigned int client_pid;
+	char *client_name;
+	struct kobject *client_root;
+
+	struct {
+		struct device_attribute pid;
+		struct device_attribute name;
+	} attr;
 };
 
 /* Interface history:
@@ -2348,6 +2358,9 @@ struct drm_i915_private {
 
 	struct i915_pmu pmu;
 
+	struct kobject *clients_root;
+	atomic_t client_serial;
+
 	/*
 	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
 	 * will be rejected. Instead look for a better place.
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7f0684ccc724..3625c0d4b039 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5520,6 +5520,89 @@ int i915_gem_freeze_late(struct drm_i915_private *dev_priv)
 	return 0;
 }
 
+static ssize_t
+show_client_name(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.name);
+
+	return snprintf(buf, PAGE_SIZE, "%s", file_priv->client_name);
+}
+
+static ssize_t
+show_client_pid(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.pid);
+
+	return snprintf(buf, PAGE_SIZE, "%u", file_priv->client_pid);
+}
+
+static int
+i915_gem_add_client(struct drm_i915_private *i915,
+		struct drm_i915_file_private *file_priv,
+		struct task_struct *task,
+		unsigned int serial)
+{
+	int ret = -ENOMEM;
+	struct device_attribute *attr;
+	char id[32];
+
+	file_priv->client_name = kstrdup(task->comm, GFP_KERNEL);
+	if (!file_priv->client_name)
+		goto err_name;
+
+	snprintf(id, sizeof(id), "%u", serial);
+	file_priv->client_root = kobject_create_and_add(id,
+							i915->clients_root);
+	if (!file_priv->client_root)
+		goto err_client;
+
+	attr = &file_priv->attr.name;
+	attr->attr.name = "name";
+	attr->attr.mode = 0444;
+	attr->show = show_client_name;
+
+	ret = sysfs_create_file(file_priv->client_root,
+				(struct attribute *)attr);
+	if (ret)
+		goto err_attr_name;
+
+	attr = &file_priv->attr.pid;
+	attr->attr.name = "pid";
+	attr->attr.mode = 0444;
+	attr->show = show_client_pid;
+
+	ret = sysfs_create_file(file_priv->client_root,
+				(struct attribute *)attr);
+	if (ret)
+		goto err_attr_pid;
+
+	file_priv->client_pid = pid_nr(get_task_pid(task, PIDTYPE_PID));
+
+	return 0;
+
+err_attr_pid:
+	sysfs_remove_file(file_priv->client_root,
+			  (struct attribute *)&file_priv->attr.name);
+err_attr_name:
+	kobject_put(file_priv->client_root);
+err_client:
+	kfree(file_priv->client_name);
+err_name:
+	return ret;
+}
+
+static void i915_gem_remove_client(struct drm_i915_file_private *file_priv)
+{
+	sysfs_remove_file(file_priv->client_root,
+			  (struct attribute *)&file_priv->attr.pid);
+	sysfs_remove_file(file_priv->client_root,
+			  (struct attribute *)&file_priv->attr.name);
+	kobject_put(file_priv->client_root);
+	kfree(file_priv->client_name);
+}
+
 void i915_gem_release(struct drm_device *dev, struct drm_file *file)
 {
 	struct drm_i915_file_private *file_priv = file->driver_priv;
@@ -5533,32 +5616,47 @@ void i915_gem_release(struct drm_device *dev, struct drm_file *file)
 	list_for_each_entry(request, &file_priv->mm.request_list, client_link)
 		request->file_priv = NULL;
 	spin_unlock(&file_priv->mm.lock);
+
+	i915_gem_remove_client(file_priv);
 }
 
 int i915_gem_open(struct drm_i915_private *i915, struct drm_file *file)
 {
+	int ret = -ENOMEM;
 	struct drm_i915_file_private *file_priv;
-	int ret;
 
 	DRM_DEBUG("\n");
 
 	file_priv = kzalloc(sizeof(*file_priv), GFP_KERNEL);
 	if (!file_priv)
-		return -ENOMEM;
+		goto err_alloc;
+
+	file_priv->client_sysfs_id = atomic_inc_return(&i915->client_serial);
+	ret = i915_gem_add_client(i915, file_priv, current,
+				  file_priv->client_sysfs_id);
+	if (ret)
+		goto err_client;
 
 	file->driver_priv = file_priv;
+	ret = i915_gem_context_open(i915, file);
+	if (ret)
+		goto err_context;
+
 	file_priv->dev_priv = i915;
 	file_priv->file = file;
+	file_priv->bsd_engine = -1;
 
 	spin_lock_init(&file_priv->mm.lock);
 	INIT_LIST_HEAD(&file_priv->mm.request_list);
 
-	file_priv->bsd_engine = -1;
-
-	ret = i915_gem_context_open(i915, file);
-	if (ret)
-		kfree(file_priv);
+	return 0;
 
+err_context:
+	i915_gem_remove_client(file_priv);
+err_client:
+	atomic_dec(&i915->client_serial);
+	kfree(file_priv);
+err_alloc:
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index c74a20b80182..ae875c8686a3 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -575,6 +575,11 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv)
 	struct device *kdev = dev_priv->drm.primary->kdev;
 	int ret;
 
+	dev_priv->clients_root =
+		kobject_create_and_add("clients", &kdev->kobj);
+	if (!dev_priv->clients_root)
+		DRM_ERROR("Per-client sysfs setup failed\n");
+
 #ifdef CONFIG_PM
 	if (HAS_RC6(dev_priv)) {
 		ret = sysfs_merge_group(&kdev->kobj,
@@ -635,4 +640,7 @@ void i915_teardown_sysfs(struct drm_i915_private *dev_priv)
 	sysfs_unmerge_group(&kdev->kobj, &rc6_attr_group);
 	sysfs_unmerge_group(&kdev->kobj, &rc6p_attr_group);
 #endif
+
+	if (dev_priv->clients_root)
+		kobject_put(dev_priv->clients_root);
 }
-- 
2.14.1

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

[PATCH 4/6] drm/i915: Update client name on context create

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Some clients have the DRM fd passed to them over a socket by the X server.

Grab the real client and pid when they create their first context and
update the exposed data for more useful enumeration.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |  8 ++++++++
 drivers/gpu/drm/i915/i915_gem.c         |  4 ++--
 drivers/gpu/drm/i915/i915_gem_context.c | 15 +++++++++++++--
 3 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2414aa430f65..a2a883aaa59e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3398,6 +3398,14 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 void i915_gem_object_unpin_from_display_plane(struct i915_vma *vma);
 int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
 				int align);
+
+int
+i915_gem_add_client(struct drm_i915_private *i915,
+		struct drm_i915_file_private *file_priv,
+		struct task_struct *task,
+		unsigned int serial);
+void i915_gem_remove_client(struct drm_i915_file_private *file_priv);
+
 int i915_gem_open(struct drm_i915_private *i915, struct drm_file *file);
 void i915_gem_release(struct drm_device *dev, struct drm_file *file);
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 3625c0d4b039..7fdd892aa32a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5538,7 +5538,7 @@ show_client_pid(struct device *kdev, struct device_attribute *attr, char *buf)
 	return snprintf(buf, PAGE_SIZE, "%u", file_priv->client_pid);
 }
 
-static int
+int
 i915_gem_add_client(struct drm_i915_private *i915,
 		struct drm_i915_file_private *file_priv,
 		struct task_struct *task,
@@ -5593,7 +5593,7 @@ i915_gem_add_client(struct drm_i915_private *i915,
 	return ret;
 }
 
-static void i915_gem_remove_client(struct drm_i915_file_private *file_priv)
+void i915_gem_remove_client(struct drm_i915_file_private *file_priv)
 {
 	sysfs_remove_file(file_priv->client_root,
 			  (struct attribute *)&file_priv->attr.pid);
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 256800fb1a3b..0f828318a4f9 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -644,6 +644,7 @@ static bool client_is_banned(struct drm_i915_file_private *file_priv)
 int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
 				  struct drm_file *file)
 {
+	unsigned int pid = pid_nr(get_task_pid(current, PIDTYPE_PID));
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_i915_gem_context_create *args = data;
 	struct drm_i915_file_private *file_priv = file->driver_priv;
@@ -658,8 +659,7 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
 
 	if (client_is_banned(file_priv)) {
 		DRM_DEBUG("client %s[%d] banned from creating ctx\n",
-			  current->comm,
-			  pid_nr(get_task_pid(current, PIDTYPE_PID)));
+			  current->comm, pid);
 
 		return -EIO;
 	}
@@ -668,6 +668,17 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		return ret;
 
+	if (file_priv->client_pid != pid) {
+		unsigned int id = file_priv->client_sysfs_id;
+
+		i915_gem_remove_client(file_priv);
+		ret = i915_gem_add_client(dev_priv, file_priv, current, id);
+		if (ret) {
+			mutex_unlock(&dev->struct_mutex);
+			return ret;
+		}
+	}
+
 	ctx = i915_gem_create_context(dev_priv, file_priv);
 	mutex_unlock(&dev->struct_mutex);
 	if (IS_ERR(ctx))
-- 
2.14.1

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

[PATCH 5/6] drm/i915: Expose per-engine client busyness

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

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 <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/i915_gem.c | 66 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a2a883aaa59e..a96cfdfcba03 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -350,6 +350,7 @@ struct drm_i915_file_private {
 	struct {
 		struct device_attribute pid;
 		struct device_attribute name;
+		struct device_attribute busy;
 	} attr;
 };
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7fdd892aa32a..c8753e4d66c4 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -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;
+	enum intel_engine_id id;
+	ssize_t ret;
+
+	ret = i915_mutex_lock_interruptible(&i915->drm);
+	if (ret)
+		return ret;
+
+	idr_for_each(&file_priv->context_idr, busy_add, &bc);
+
+	mutex_unlock(&i915->drm.struct_mutex);
+
+	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);
@@ -5595,6 +5659,8 @@ i915_gem_add_client(struct drm_i915_private *i915,
 
 void i915_gem_remove_client(struct drm_i915_file_private *file_priv)
 {
+	sysfs_remove_file(file_priv->client_root,
+			  (struct attribute *)&file_priv->attr.busy);
 	sysfs_remove_file(file_priv->client_root,
 			  (struct attribute *)&file_priv->attr.pid);
 	sysfs_remove_file(file_priv->client_root,
-- 
2.14.1

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

[PATCH 6/6] drm/i915: Add sysfs toggle to enable per-client engine stats

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

By default we are not collecting any per-engine and per-context
statistcs.

Add a new sysfs toggle to enable this facility:

$ echo 1 >/sys/class/drm/card0/clients/enable_stats

v2: Rebase.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h   |  2 ++
 drivers/gpu/drm/i915/i915_sysfs.c | 72 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a96cfdfcba03..63f96e8fe3b7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2361,6 +2361,8 @@ struct drm_i915_private {
 
 	struct kobject *clients_root;
 	atomic_t client_serial;
+	struct device_attribute client_stats_attr;
+	bool client_stats_enabled;
 
 	/*
 	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index ae875c8686a3..f250547af1ef 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -570,9 +570,67 @@ static void i915_setup_error_capture(struct device *kdev) {}
 static void i915_teardown_error_capture(struct device *kdev) {}
 #endif
 
+static ssize_t
+show_client_stats(struct device *kdev, struct device_attribute *attr, char *buf)
+{
+	struct drm_i915_private *i915 =
+		container_of(attr, struct drm_i915_private, client_stats_attr);
+
+	return snprintf(buf, PAGE_SIZE, "%u\n", i915->client_stats_enabled);
+}
+
+static ssize_t
+store_client_stats(struct device *kdev, struct device_attribute *attr,
+		   const char *buf, size_t count)
+{
+	struct drm_i915_private *i915 =
+		container_of(attr, struct drm_i915_private, client_stats_attr);
+	bool disable = false;
+	bool enable = false;
+	bool val = false;
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
+	int ret;
+
+	 /* Use RCS as proxy for all engines. */
+	if (!intel_engine_supports_stats(i915->engine[RCS]))
+		return -EINVAL;
+
+	ret = kstrtobool(buf, &val);
+	if (ret)
+		return ret;
+
+	ret = i915_mutex_lock_interruptible(&i915->drm);
+	if (ret)
+		return ret;
+
+	if (val && !i915->client_stats_enabled)
+		enable = true;
+	else if (!val && i915->client_stats_enabled)
+		disable = true;
+
+	if (!enable && !disable)
+		goto out;
+
+	for_each_engine(engine, i915, id) {
+		if (enable)
+			WARN_ON_ONCE(intel_enable_engine_stats(engine));
+		else if (disable)
+			intel_disable_engine_stats(engine);
+	}
+
+	i915->client_stats_enabled = val;
+
+out:
+	mutex_unlock(&i915->drm.struct_mutex);
+
+	return count;
+}
+
 void i915_setup_sysfs(struct drm_i915_private *dev_priv)
 {
 	struct device *kdev = dev_priv->drm.primary->kdev;
+	struct device_attribute *attr;
 	int ret;
 
 	dev_priv->clients_root =
@@ -580,6 +638,17 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv)
 	if (!dev_priv->clients_root)
 		DRM_ERROR("Per-client sysfs setup failed\n");
 
+	attr = &dev_priv->client_stats_attr;
+	attr->attr.name = "enable_stats";
+	attr->attr.mode = 0664;
+	attr->show = show_client_stats;
+	attr->store = store_client_stats;
+
+	ret = sysfs_create_file(dev_priv->clients_root,
+				(struct attribute *)attr);
+	if (ret)
+		DRM_ERROR("Per-client sysfs setup failed! (%d)\n", ret);
+
 #ifdef CONFIG_PM
 	if (HAS_RC6(dev_priv)) {
 		ret = sysfs_merge_group(&kdev->kobj,
@@ -641,6 +710,9 @@ void i915_teardown_sysfs(struct drm_i915_private *dev_priv)
 	sysfs_unmerge_group(&kdev->kobj, &rc6p_attr_group);
 #endif
 
+	sysfs_remove_file(dev_priv->clients_root,
+			  (struct attribute *)&dev_priv->client_stats_attr);
+
 	if (dev_priv->clients_root)
 		kobject_put(dev_priv->clients_root);
 }
-- 
2.14.1

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

✗ Fi.CI.BAT: failure for Per-context and per-client engine busyness (rev2)

[Patchwork]

== Series Details ==

Series: Per-context and per-client engine busyness (rev2)
URL   : https://patchwork.freedesktop.org/series/32645/
State : failure

== Summary ==

  CHK     include/config/kernel.release
  CHK     include/generated/uapi/linux/version.h
  CHK     include/generated/utsrelease.h
  CHK     include/generated/bounds.h
  CHK     include/generated/timeconst.h
  CHK     include/generated/asm-offsets.h
  CALL    scripts/checksyscalls.sh
  DESCEND  objtool
  CHK     scripts/mod/devicetable-offsets.h
  CHK     include/generated/compile.h
  CC      init/version.o
  AR      init/built-in.o
  CC      arch/x86/kernel/early-quirks.o
  AR      arch/x86/kernel/built-in.o
  AR      arch/x86/built-in.o
  CC      kernel/sys.o
  CC      kernel/trace/trace.o
  AR      kernel/trace/built-in.o
  CC      kernel/module.o
  AR      kernel/built-in.o
  GZIP    kernel/config_data.gz
  CHK     kernel/config_data.h
  UPD     kernel/config_data.h
  CC [M]  kernel/configs.o
  CC      drivers/base/firmware_class.o
  AR      drivers/base/built-in.o
  CC [M]  drivers/gpu/drm/i915/i915_drv.o
In file included from drivers/gpu/drm/i915/intel_ringbuffer.h:7:0,
                 from drivers/gpu/drm/i915/intel_lrc.h:27,
                 from drivers/gpu/drm/i915/i915_drv.h:63,
                 from drivers/gpu/drm/i915/i915_drv.c:49:
drivers/gpu/drm/i915/i915_gem_context.h:166:11: error: ‘I915_NUM_ENGINES’ undeclared here (not in a function)
  } engine[I915_NUM_ENGINES];
           ^~~~~~~~~~~~~~~~
drivers/gpu/drm/i915/i915_gem_context.h: In function ‘i915_gem_context_set_closed’:
drivers/gpu/drm/i915/i915_gem_context.h:209:2: error: implicit declaration of function ‘GEM_BUG_ON’ [-Werror=implicit-function-declaration]
  GEM_BUG_ON(i915_gem_context_is_closed(ctx));
  ^~~~~~~~~~
drivers/gpu/drm/i915/i915_gem_context.h: At top level:
drivers/gpu/drm/i915/i915_gem_context.h:282:32: error: ‘struct drm_i915_gem_request’ declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
 int i915_switch_context(struct drm_i915_gem_request *req);
                                ^~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
scripts/Makefile.build:310: recipe for target 'drivers/gpu/drm/i915/i915_drv.o' failed
make[4]: *** [drivers/gpu/drm/i915/i915_drv.o] Error 1
scripts/Makefile.build:569: recipe for target 'drivers/gpu/drm/i915' failed
make[3]: *** [drivers/gpu/drm/i915] Error 2
scripts/Makefile.build:569: recipe for target 'drivers/gpu/drm' failed
make[2]: *** [drivers/gpu/drm] Error 2
scripts/Makefile.build:569: recipe for target 'drivers/gpu' failed
make[1]: *** [drivers/gpu] Error 2
Makefile:1018: recipe for target 'drivers' failed
make: *** [drivers] Error 2

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

[PATCH v5 1/6] drm/i915: Track per-context engine busyness

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

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 <tvrtko.ursulin@intel.com>
Cc: gordon.kelly@intel.com
---
 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(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
index 4bfb72f8e1cb..7f5eebb67167 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.h
+++ b/drivers/gpu/drm/i915/i915_gem_context.h
@@ -29,6 +29,9 @@
 #include <linux/list.h>
 #include <linux/radix-tree.h>
 
+#include "i915_gem.h"
+#include "i915_gem_request.h"
+
 struct pid;
 
 struct drm_device;
@@ -157,6 +160,11 @@ struct i915_gem_context {
 		u32 *lrc_reg_state;
 		u64 lrc_desc;
 		int pin_count;
+		struct {
+			bool active;
+			ktime_t start;
+			ktime_t total;
+		} stats;
 	} engine[I915_NUM_ENGINES];
 
 	/** ring_size: size for allocating the per-engine ring buffer */
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index d572b18d39eb..9907ceedfa90 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1966,6 +1966,16 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine)
 
 		engine->stats.enabled_at = ktime_get();
 
+		/* Mark currently running context as active. */
+		if (port_isset(port)) {
+			struct drm_i915_gem_request *req = port_request(port);
+			struct intel_context *ce =
+					&req->ctx->engine[engine->id];
+
+			ce->stats.start = engine->stats.enabled_at;
+			ce->stats.active = true;
+		}
+
 		/* XXX submission method oblivious? */
 		while (num_ports-- && port_isset(port)) {
 			engine->stats.active++;
@@ -2038,6 +2048,28 @@ void intel_disable_engine_stats(struct intel_engine_cs *engine)
 	spin_unlock_irqrestore(&engine->stats.lock, flags);
 }
 
+ktime_t intel_context_engine_get_busy_time(struct i915_gem_context *ctx,
+					   struct intel_engine_cs *engine)
+{
+	struct intel_context *ce;
+	unsigned long flags;
+	ktime_t total;
+
+	ce = &ctx->engine[engine->id];
+
+	spin_lock_irqsave(&engine->stats.lock, flags);
+
+	total = ce->stats.total;
+
+	if (ce->stats.active)
+		total = ktime_add(total,
+				  ktime_sub(ktime_get(), ce->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_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 24ce781d39b7..a82ad5da6090 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -380,16 +380,19 @@ execlists_context_status_change(struct drm_i915_gem_request *rq,
 }
 
 static inline void
-execlists_context_schedule_in(struct drm_i915_gem_request *rq)
+execlists_context_schedule_in(struct drm_i915_gem_request *rq,
+			      unsigned int port)
 {
 	execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_IN);
-	intel_engine_context_in(rq->engine);
+	intel_engine_context_in(rq->engine,
+				&rq->ctx->engine[rq->engine->id],
+				port == 0);
 }
 
 static inline void
 execlists_context_schedule_out(struct drm_i915_gem_request *rq)
 {
-	intel_engine_context_out(rq->engine);
+	intel_engine_context_out(rq->engine, &rq->ctx->engine[rq->engine->id]);
 	execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_OUT);
 }
 
@@ -442,7 +445,7 @@ static void execlists_submit_ports(struct intel_engine_cs *engine)
 		if (rq) {
 			GEM_BUG_ON(count > !n);
 			if (!count++)
-				execlists_context_schedule_in(rq);
+				execlists_context_schedule_in(rq, n);
 			port_set(&port[n], port_pack(rq, count));
 			desc = execlists_update_context(rq);
 			GEM_DEBUG_EXEC(port[n].context_id = upper_32_bits(desc));
@@ -703,7 +706,8 @@ execlists_cancel_port_requests(struct intel_engine_execlists * const execlists)
 		struct drm_i915_gem_request *rq = port_request(port);
 
 		GEM_BUG_ON(!execlists->active);
-		intel_engine_context_out(rq->engine);
+		intel_engine_context_out(rq->engine,
+					 &rq->ctx->engine[rq->engine->id]);
 		execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_PREEMPTED);
 		i915_gem_request_put(rq);
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index b198df1f248c..27b727cf4017 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -4,6 +4,7 @@
 
 #include <linux/hashtable.h>
 #include "i915_gem_batch_pool.h"
+#include "i915_gem_context.h"
 #include "i915_gem_request.h"
 #include "i915_gem_timeline.h"
 #include "i915_pmu.h"
@@ -1036,25 +1037,42 @@ void intel_engine_dump(struct intel_engine_cs *engine,
 struct intel_engine_cs *
 intel_engine_lookup_user(struct drm_i915_private *i915, u8 class, u8 instance);
 
-static inline void intel_engine_context_in(struct intel_engine_cs *engine)
+static inline void
+intel_engine_context_in(struct intel_engine_cs *engine,
+			struct intel_context *ce,
+			bool submit)
 {
 	unsigned long flags;
+	ktime_t now;
 
 	if (READ_ONCE(engine->stats.enabled) == 0)
 		return;
 
 	spin_lock_irqsave(&engine->stats.lock, flags);
 
+	if (submit) {
+		now = ktime_get();
+		ce->stats.start = now;
+		ce->stats.active = true;
+	} else {
+		now = 0;
+	}
+
 	if (engine->stats.enabled > 0) {
-		if (engine->stats.active++ == 0)
-			engine->stats.start = ktime_get();
+		if (engine->stats.active++ == 0) {
+			if (!now)
+				now = ktime_get();
+			engine->stats.start = now;
+		}
 		GEM_BUG_ON(engine->stats.active == 0);
 	}
 
 	spin_unlock_irqrestore(&engine->stats.lock, flags);
 }
 
-static inline void intel_engine_context_out(struct intel_engine_cs *engine)
+static inline void
+intel_engine_context_out(struct intel_engine_cs *engine,
+			 struct intel_context *ce)
 {
 	unsigned long flags;
 
@@ -1064,14 +1082,31 @@ static inline void intel_engine_context_out(struct intel_engine_cs *engine)
 	spin_lock_irqsave(&engine->stats.lock, flags);
 
 	if (engine->stats.enabled > 0) {
+		struct execlist_port *next_port = &engine->execlists.port[1];
+		ktime_t now = ktime_get();
 		ktime_t last;
 
+		GEM_BUG_ON(!ce->stats.start);
+		ce->stats.total = ktime_add(ce->stats.total,
+					    ktime_sub(now, ce->stats.start));
+		ce->stats.active = false;
+
+		if (port_isset(next_port)) {
+			struct drm_i915_gem_request *next_req =
+						port_request(next_port);
+			struct intel_context *next_ce =
+					&next_req->ctx->engine[engine->id];
+
+			next_ce->stats.start = now;
+			next_ce->stats.active = true;
+		}
+
 		if (engine->stats.active && --engine->stats.active == 0) {
 			/*
 			 * Decrement the active context count and in case GPU
 			 * is now idle add up to the running total.
 			 */
-			last = ktime_sub(ktime_get(), engine->stats.start);
+			last = ktime_sub(now, engine->stats.start);
 
 			engine->stats.total = ktime_add(engine->stats.total,
 							last);
@@ -1081,7 +1116,7 @@ static inline void intel_engine_context_out(struct intel_engine_cs *engine)
 			 * the first event in which case we account from the
 			 * time stats gathering was turned on.
 			 */
-			last = ktime_sub(ktime_get(), engine->stats.enabled_at);
+			last = ktime_sub(now, engine->stats.enabled_at);
 
 			engine->stats.total = ktime_add(engine->stats.total,
 							last);
@@ -1091,6 +1126,9 @@ static inline void intel_engine_context_out(struct intel_engine_cs *engine)
 	spin_unlock_irqrestore(&engine->stats.lock, flags);
 }
 
+ktime_t intel_context_engine_get_busy_time(struct i915_gem_context *ctx,
+					   struct intel_engine_cs *engine);
+
 int intel_enable_engine_stats(struct intel_engine_cs *engine);
 void intel_disable_engine_stats(struct intel_engine_cs *engine);
 
-- 
2.14.1

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

✗ Fi.CI.BAT: failure for Per-context and per-client engine busyness (rev3)

[Patchwork]

== Series Details ==

Series: Per-context and per-client engine busyness (rev3)
URL   : https://patchwork.freedesktop.org/series/32645/
State : failure

== Summary ==

Series 32645v3 Per-context and per-client engine busyness
https://patchwork.freedesktop.org/api/1.0/series/32645/revisions/3/mbox/

Test debugfs_test:
        Subgroup read_all_entries:
                dmesg-warn -> DMESG-FAIL (fi-elk-e7500) fdo#103989
Test gem_exec_flush:
        Subgroup basic-wb-prw-default:
                pass       -> INCOMPLETE (fi-snb-2600)
Test gem_ringfill:
        Subgroup basic-default-hang:
                dmesg-warn -> PASS       (fi-pnv-d510) fdo#101600

fdo#103989 https://bugs.freedesktop.org/show_bug.cgi?id=103989
fdo#101600 https://bugs.freedesktop.org/show_bug.cgi?id=101600

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:435s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:428s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:372s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:512s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:275s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:526s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:526s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:532s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:473s
fi-elk-e7500     total:224  pass:168  dwarn:9   dfail:1   fail:0   skip:45 
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:278s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:543s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:422s
fi-hsw-4770r     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:430s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:411s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:492s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:445s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:468s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:511s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:463s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:538s
fi-pnv-d510      total:288  pass:223  dwarn:0   dfail:0   fail:0   skip:65  time:529s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:451s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:524s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:575s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:509s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:500s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:431s
fi-snb-2520m     total:245  pass:211  dwarn:0   dfail:0   fail:0   skip:33 
fi-snb-2600      total:59   pass:48   dwarn:0   dfail:0   fail:0   skip:10 
Blacklisted hosts:
fi-cfl-s2        total:288  pass:256  dwarn:0   dfail:0   fail:3   skip:26  time:610s
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:506s
fi-skl-guc       total:288  pass:211  dwarn:48  dfail:0   fail:1   skip:28  time:453s

fd10fae2b0f673861816511f99d4798d6b619b40 drm-tip: 2018y-01m-19d-14h-22m-15s UTC integration manifest
1d9909fd0965 drm/i915: Add sysfs toggle to enable per-client engine stats
45aee892c9ad drm/i915: Expose per-engine client busyness
813f0228546b drm/i915: Update client name on context create
9e245f148ad3 drm/i915: Expose list of clients in sysfs
1a6e191c83f8 drm/i915: Allow clients to query own per-engine busyness
01e95b47013e drm/i915: Track per-context engine busyness

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7726/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 1/6] drm/i915: Track per-context engine busyness

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-01-19 16:26:16)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> 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 <tvrtko.ursulin@intel.com>
> Cc: gordon.kelly@intel.com
> ---
>  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(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
> index 4bfb72f8e1cb..7f5eebb67167 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.h
> +++ b/drivers/gpu/drm/i915/i915_gem_context.h
> @@ -29,6 +29,9 @@
>  #include <linux/list.h>
>  #include <linux/radix-tree.h>
>  
> +#include "i915_gem.h"
> +#include "i915_gem_request.h"

Yup, we need a patch for tip for

#include "i915_gem.h"

struct drm_i915_gem_request;
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

On 19/01/2018 21:08, Chris Wilson wrote:
> 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?

Haven't extended it no, just did this:

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

Regards,

Tvrtko

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

Re: [PATCH 5/6] drm/i915: Expose per-engine client busyness

[Tvrtko Ursulin]

On 19/01/2018 13:45, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> 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 <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h |  1 +
>   drivers/gpu/drm/i915/i915_gem.c | 66 +++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 67 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a2a883aaa59e..a96cfdfcba03 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -350,6 +350,7 @@ struct drm_i915_file_private {
>   	struct {
>   		struct device_attribute pid;
>   		struct device_attribute name;
> +		struct device_attribute busy;
>   	} attr;
>   };
>   
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 7fdd892aa32a..c8753e4d66c4 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -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;
> +	enum intel_engine_id id;
> +	ssize_t ret;
> +
> +	ret = i915_mutex_lock_interruptible(&i915->drm);
> +	if (ret)
> +		return ret;
> +
> +	idr_for_each(&file_priv->context_idr, busy_add, &bc);
> +
> +	mutex_unlock(&i915->drm.struct_mutex);

This is the part which could be done more efficiently by adding an 
aggregated busyness to file_priv as a couple atomic_t (busy count and 
active count I think), and incrementing it from the 
context_in/context_out hooks as they go. Then the client busyness query 
would be both lockless and wouldn't need to iterate the 
file_priv->context_id.

It would probably be preferable to do it like that instead of having to 
take struct_mutex here. But until there is some idea if people are 
warming up to this feature in general I did not bother fully polishing it.

Regards,

Tvrtko

> +
> +	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);
> @@ -5595,6 +5659,8 @@ i915_gem_add_client(struct drm_i915_private *i915,
>   
>   void i915_gem_remove_client(struct drm_i915_file_private *file_priv)
>   {
> +	sysfs_remove_file(file_priv->client_root,
> +			  (struct attribute *)&file_priv->attr.busy);
>   	sysfs_remove_file(file_priv->client_root,
>   			  (struct attribute *)&file_priv->attr.pid);
>   	sysfs_remove_file(file_priv->client_root,
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-01-22 09:53:27)
> 
> On 19/01/2018 21:08, Chris Wilson wrote:
> > 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?
> 
> Haven't extended it no, just did this:
> 
> --- 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;
> +               };
> +       };
>   };

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.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

On 22/01/2018 10:00, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-01-22 09:53:27)
>>
>> On 19/01/2018 21:08, Chris Wilson wrote:
>>> 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?
>>
>> Haven't extended it no, just did this:
>>
>> --- 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;
>> +               };
>> +       };
>>    };
> 
> 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.

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];
};

Regards,

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

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

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-01-22 11:45:04)
> 
> On 22/01/2018 10:00, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-01-22 09:53:27)
> >>
> >> On 19/01/2018 21:08, Chris Wilson wrote:
> >>> 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?
> >>
> >> Haven't extended it no, just did this:
> >>
> >> --- 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;
> >> +               };
> >> +       };
> >>    };
> > 
> > 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.
> 
> 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];
> };

Yes, that's what I had in mind. How does that work in practice? Does it
make userspace too clumsy?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

On 22/01/2018 12:32, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-01-22 11:45:04)
>>
>> On 22/01/2018 10:00, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2018-01-22 09:53:27)
>>>>
>>>> On 19/01/2018 21:08, Chris Wilson wrote:
>>>>> 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?
>>>>
>>>> Haven't extended it no, just did this:
>>>>
>>>> --- 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;
>>>> +               };
>>>> +       };
>>>>     };
>>>
>>> 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.
>>
>> 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];
>> };

We could also get away without explicit stats enable step if so is 
desired (like the posted RFC). Enable on first query, disable on context 
destroy.

>>
>> I915_CONTEXT_GET_ENGINE_STATS:
>> value = &{
>>          __u32 num_engines;
>>          __u32 pad;

Will need length here so we don't overwrite users memory.

>>
>>          struct {
>>                  __u8 class;
>>                  __u8 instance;
>>                  __u8 pad[6];
>>
>>                  __u64 busy;
>>          } [num_engines];
>> };
> 
> Yes, that's what I had in mind. How does that work in practice? Does it
> make userspace too clumsy?

I wouldn't expect it to be a problem. Gordon?

Regards,

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