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

[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     |  4 +--
 drivers/gpu/drm/i915/i915_drv.h         |  4 ++-
 drivers/gpu/drm/i915/i915_gem_context.c | 53 +++++++++++++++++++++++----------
 3 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 47636f3..38e0026 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2254,12 +2254,10 @@ static void gen6_ppgtt_info(struct seq_file *m, struct drm_device *dev)
 	}
 
 	list_for_each_entry_reverse(file, &dev->filelist, lhead) {
-		struct drm_i915_file_private *file_priv = file->driver_priv;
-
 		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