[RFC 1/8] drm/i915: Have globally unique context ids, as opposed to drm file specific

From: sourab.gupta@intel.com
To: intel-gfx@lists.freedesktop.org
Cc: Insoo Woo <insoo.woo@intel.com>,
	Peter Zijlstra <a.p.zijlstra@chello.nl>,
	Jabin Wu <jabin.wu@intel.com>,
	Sourab Gupta <sourab.gupta@intel.com>
Subject: [RFC 1/8] drm/i915: Have globally unique context ids, as opposed to drm file specific
Date: Mon, 22 Jun 2015 15:20:12 +0530	[thread overview]
Message-ID: <1434966619-3979-2-git-send-email-sourab.gupta@intel.com> (raw)
In-Reply-To: <1434966619-3979-1-git-send-email-sourab.gupta@intel.com>

From: Sourab Gupta <sourab.gupta@intel.com>

Currently the context ids are specific to a drm file instance, as opposed to
being globally unique. There are some usecases, which may require globally
unique context ids. For e.g. a system level GPU profiler tool may lean upon
the context ids to associate the performance snapshots with individual contexts.
If the context ids are unique, it may do so without relying on any additional
information such as pid and drm fd.

This patch proposes an implementation of globally unique context ids, by
conceptually moving the idr table for holding the context ids, into device
private structure instead of file private structure. The case of default context
id for drm file (which is given by id=0) is handled by storing the same in
file private during context creation, and retrieving as and when required.

This patch is proposed an an enabler for the patches following in the series. In
particular, I'm looking for feedback on the pros and cons of having a globally
unique context id, and any specific inputs on this particular implementation.
This implementation can be improved upon, if agreed upon conceptually.

Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c     |  2 +-
 drivers/gpu/drm/i915/i915_drv.h         |  4 ++-
 drivers/gpu/drm/i915/i915_gem_context.c | 53 +++++++++++++++++++++++----------
 3 files changed, 41 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 47636f3..74c736c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2258,8 +2258,8 @@ static void gen6_ppgtt_info(struct seq_file *m, struct drm_device *dev)
 
 		seq_printf(m, "proc: %s\n",
 			   get_pid_task(file->pid, PIDTYPE_PID)->comm);
-		idr_for_each(&file_priv->context_idr, per_file_ctx, m);
 	}
+	idr_for_each(&dev_priv->context_idr, per_file_ctx, m);
 	seq_printf(m, "ECOCHK: 0x%08x\n", I915_READ(GAM_ECOCHK));
 }
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 50977f0..baa0234 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -320,7 +320,7 @@ struct drm_i915_file_private {
  */
 #define DRM_I915_THROTTLE_JIFFIES msecs_to_jiffies(20)
 	} mm;
-	struct idr context_idr;
+	u32 first_ctx_id;
 
 	struct intel_rps_client {
 		struct list_head link;
@@ -1754,6 +1754,8 @@ struct drm_i915_private {
 	struct intel_opregion opregion;
 	struct intel_vbt_data vbt;
 
+	struct idr context_idr;
+
 	bool preserve_bios_swizzle;
 
 	/* overlay */
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index d9ccad5..6b572c1 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -212,7 +212,8 @@ i915_gem_alloc_context_obj(struct drm_device *dev, size_t size)
 
 static struct intel_context *
 __create_hw_context(struct drm_device *dev,
-		    struct drm_i915_file_private *file_priv)
+		    struct drm_i915_file_private *file_priv,
+		    bool is_first_ctx)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_context *ctx;
@@ -237,10 +238,12 @@ __create_hw_context(struct drm_device *dev,
 
 	/* Default context will never have a file_priv */
 	if (file_priv != NULL) {
-		ret = idr_alloc(&file_priv->context_idr, ctx,
+		ret = idr_alloc(&dev_priv->context_idr, ctx,
 				DEFAULT_CONTEXT_HANDLE, 0, GFP_KERNEL);
 		if (ret < 0)
 			goto err_out;
+		if (is_first_ctx)
+			file_priv->first_ctx_id = ret;
 	} else
 		ret = DEFAULT_CONTEXT_HANDLE;
 
@@ -267,7 +270,8 @@ err_out:
  */
 static struct intel_context *
 i915_gem_create_context(struct drm_device *dev,
-			struct drm_i915_file_private *file_priv)
+			struct drm_i915_file_private *file_priv,
+			bool is_first_ctx)
 {
 	const bool is_global_default_ctx = file_priv == NULL;
 	struct intel_context *ctx;
@@ -275,7 +279,7 @@ i915_gem_create_context(struct drm_device *dev,
 
 	BUG_ON(!mutex_is_locked(&dev->struct_mutex));
 
-	ctx = __create_hw_context(dev, file_priv);
+	ctx = __create_hw_context(dev, file_priv, is_first_ctx);
 	if (IS_ERR(ctx))
 		return ctx;
 
@@ -348,6 +352,14 @@ void i915_gem_context_reset(struct drm_device *dev)
 	}
 }
 
+static int context_idr_cleanup(int id, void *p, void *data)
+{
+	struct intel_context *ctx = p;
+
+	i915_gem_context_unreference(ctx);
+	return 0;
+}
+
 int i915_gem_context_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -371,8 +383,9 @@ int i915_gem_context_init(struct drm_device *dev)
 			dev_priv->hw_context_size = 0;
 		}
 	}
+	idr_init(&dev_priv->context_idr);
 
-	ctx = i915_gem_create_context(dev, NULL);
+	ctx = i915_gem_create_context(dev, NULL, false);
 	if (IS_ERR(ctx)) {
 		DRM_ERROR("Failed to create default global context (error %ld)\n",
 			  PTR_ERR(ctx));
@@ -398,6 +411,9 @@ void i915_gem_context_fini(struct drm_device *dev)
 	struct intel_context *dctx = dev_priv->ring[RCS].default_context;
 	int i;
 
+	idr_for_each(&dev_priv->context_idr, context_idr_cleanup, NULL);
+	idr_destroy(&dev_priv->context_idr);
+
 	if (dctx->legacy_hw_ctx.rcs_state) {
 		/* The only known way to stop the gpu from accessing the hw context is
 		 * to reset it. Do this as the very last operation to avoid confusing
@@ -465,11 +481,14 @@ int i915_gem_context_enable(struct drm_i915_private *dev_priv)
 	return 0;
 }
 
-static int context_idr_cleanup(int id, void *p, void *data)
+static int cleanup_file_contexts(int id, void *p, void *data)
 {
 	struct intel_context *ctx = p;
+	struct drm_i915_file_private *file_priv = data;
+
+	if (ctx->file_priv == file_priv)
+		i915_gem_context_unreference(ctx);
 
-	i915_gem_context_unreference(ctx);
 	return 0;
 }
 
@@ -478,14 +497,11 @@ int i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
 	struct drm_i915_file_private *file_priv = file->driver_priv;
 	struct intel_context *ctx;
 
-	idr_init(&file_priv->context_idr);
-
 	mutex_lock(&dev->struct_mutex);
-	ctx = i915_gem_create_context(dev, file_priv);
+	ctx = i915_gem_create_context(dev, file_priv, true);
 	mutex_unlock(&dev->struct_mutex);
 
 	if (IS_ERR(ctx)) {
-		idr_destroy(&file_priv->context_idr);
 		return PTR_ERR(ctx);
 	}
 
@@ -495,17 +511,21 @@ int i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
 void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
 {
 	struct drm_i915_file_private *file_priv = file->driver_priv;
+	struct drm_i915_private *dev_priv = file_priv->dev_priv;
 
-	idr_for_each(&file_priv->context_idr, context_idr_cleanup, NULL);
-	idr_destroy(&file_priv->context_idr);
+	idr_for_each(&dev_priv->context_idr, cleanup_file_contexts, file_priv);
 }
 
 struct intel_context *
 i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
 {
+	struct drm_i915_private *dev_priv = file_priv->dev_priv;
 	struct intel_context *ctx;
 
-	ctx = (struct intel_context *)idr_find(&file_priv->context_idr, id);
+	if (id == 0)
+		id = file_priv->first_ctx_id;
+
+	ctx = (struct intel_context *)idr_find(&dev_priv->context_idr, id);
 	if (!ctx)
 		return ERR_PTR(-ENOENT);
 
@@ -862,7 +882,7 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		return ret;
 
-	ctx = i915_gem_create_context(dev, file_priv);
+	ctx = i915_gem_create_context(dev, file_priv, false);
 	mutex_unlock(&dev->struct_mutex);
 	if (IS_ERR(ctx))
 		return PTR_ERR(ctx);
@@ -878,6 +898,7 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
 {
 	struct drm_i915_gem_context_destroy *args = data;
 	struct drm_i915_file_private *file_priv = file->driver_priv;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_context *ctx;
 	int ret;
 
@@ -894,7 +915,7 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
 		return PTR_ERR(ctx);
 	}
 
-	idr_remove(&ctx->file_priv->context_idr, ctx->user_handle);
+	idr_remove(&dev_priv->context_idr, ctx->user_handle);
 	i915_gem_context_unreference(ctx);
 	mutex_unlock(&dev->struct_mutex);
 
-- 
1.8.5.1

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