Re: [PATCH 4/8] drm/i915: Rename ctx->id to ctx->handle

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Thu, 26 Jun 2014 14:24:15 +0100
oscar.mateo@intel.com wrote:

> From: Oscar Mateo <oscar.mateo@intel.com>
> 
> This is an Execlists preparatory patch, since they make context ID become an
> overloaded term:
> 
> - In the software, it was used to distinguish which context userspace was
>   trying to use.
> - In the BSpec, the term is used to describe the 20-bits long field the
>   hardware uses to it to discriminate the contexts that are submitted to
>   the ELSP and inform the driver about their current status (via Context
>   Switch Interrupts and Context Status Buffers).
> 
> Initially, I tried to make the different meanings converge, but it proved
> impossible:
> 
> - The software ctx->id is per-filp, while the hardware one needs to be
>   globally unique.
> - Also, we multiplex several backing states objects per intel_context,
>   and all of them need unique HW IDs.
> - I tried adding a per-filp ID and then composing the HW context ID as:
>   ctx->id + file_priv->id + ring->id, but the fact that the hardware only
>   uses 20-bits means we have to artificially limit the number of filps or
>   contexts the userspace can create.
> 
> The ctx->handle bits are done with this Cocci patch (plus manual frobbing
> of the struct declaration):
> 
> 	@@
> 	struct intel_context c;
> 	@@
> 	- (c).id
> 	+ c.handle
> 
> 	@@
> 	struct intel_context *c;
> 	@@
> 	- (c)->id
> 	+ c->handle
> 
> Also, while we are at it, s/DEFAULT_CONTEXT_ID/DEFAULT_CONTEXT_HANDLE.
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c        |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h            |  6 +++---
>  drivers/gpu/drm/i915/i915_gem_context.c    | 12 ++++++------
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  2 +-
>  drivers/gpu/drm/i915/intel_uncore.c        |  2 +-
>  5 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index d4b8391..7484e22 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1867,7 +1867,7 @@ static int per_file_ctx(int id, void *ptr, void *data)
>  	if (i915_gem_context_is_default(ctx))
>  		seq_puts(m, "  default context:\n");
>  	else
> -		seq_printf(m, "  context %d:\n", ctx->id);
> +		seq_printf(m, "  context %d:\n", ctx->handle);
>  	ppgtt->debug_dump(ppgtt, m);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 122e942..5d2b6ab 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -584,10 +584,10 @@ struct i915_ctx_hang_stats {
>  };
>  
>  /* This must match up with the value previously used for execbuf2.rsvd1. */
> -#define DEFAULT_CONTEXT_ID 0
> +#define DEFAULT_CONTEXT_HANDLE 0
>  struct intel_context {
>  	struct kref ref;
> -	int id;
> +	int handle;
>  	bool rcs_is_initialized;
>  	uint8_t remap_slice;
>  	struct drm_i915_file_private *file_priv;
> @@ -2458,7 +2458,7 @@ static inline void i915_gem_context_unreference(struct intel_context *ctx)
>  
>  static inline bool i915_gem_context_is_default(const struct intel_context *c)
>  {
> -	return c->id == DEFAULT_CONTEXT_ID;
> +	return c->handle == DEFAULT_CONTEXT_HANDLE;
>  }
>  
>  int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index b8b9859..75e903f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -276,14 +276,14 @@ __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,
> -				DEFAULT_CONTEXT_ID, 0, GFP_KERNEL);
> +				DEFAULT_CONTEXT_HANDLE, 0, GFP_KERNEL);
>  		if (ret < 0)
>  			goto err_out;
>  	} else
> -		ret = DEFAULT_CONTEXT_ID;
> +		ret = DEFAULT_CONTEXT_HANDLE;
>  
>  	ctx->file_priv = file_priv;
> -	ctx->id = ret;
> +	ctx->handle = ret;
>  	/* NB: Mark all slices as needing a remap so that when the context first
>  	 * loads it will restore whatever remap state already exists. If there
>  	 * is no remap info, it will be a NOP. */
> @@ -787,7 +787,7 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
>  	if (IS_ERR(ctx))
>  		return PTR_ERR(ctx);
>  
> -	args->ctx_id = ctx->id;
> +	args->ctx_id = ctx->handle;
>  	DRM_DEBUG_DRIVER("HW context %d created\n", args->ctx_id);
>  
>  	return 0;
> @@ -801,7 +801,7 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
>  	struct intel_context *ctx;
>  	int ret;
>  
> -	if (args->ctx_id == DEFAULT_CONTEXT_ID)
> +	if (args->ctx_id == DEFAULT_CONTEXT_HANDLE)
>  		return -ENOENT;
>  
>  	ret = i915_mutex_lock_interruptible(dev);
> @@ -814,7 +814,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->id);
> +	idr_remove(&ctx->file_priv->context_idr, ctx->handle);
>  	i915_gem_context_unreference(ctx);
>  	mutex_unlock(&dev->struct_mutex);
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index d815ef5..c97178e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -938,7 +938,7 @@ i915_gem_validate_context(struct drm_device *dev, struct drm_file *file,
>  	struct intel_context *ctx = NULL;
>  	struct i915_ctx_hang_stats *hs;
>  
> -	if (ring->id != RCS && ctx_id != DEFAULT_CONTEXT_ID)
> +	if (ring->id != RCS && ctx_id != DEFAULT_CONTEXT_HANDLE)
>  		return ERR_PTR(-EINVAL);
>  
>  	ctx = i915_gem_context_get(file->driver_priv, ctx_id);
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 29145df..e0f0843 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1010,7 +1010,7 @@ int i915_get_reset_stats_ioctl(struct drm_device *dev,
>  	if (args->flags || args->pad)
>  		return -EINVAL;
>  
> -	if (args->ctx_id == DEFAULT_CONTEXT_ID && !capable(CAP_SYS_ADMIN))
> +	if (args->ctx_id == DEFAULT_CONTEXT_HANDLE && !capable(CAP_SYS_ADMIN))
>  		return -EPERM;
>  
>  	ret = mutex_lock_interruptible(&dev->struct_mutex);

So this handle is just the sw tracking ID right?  Would be nice to have
a patch on top commenting that in the context struct (well commenting
the whole struct really).

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center