[Intel-gfx] [PATCH 4/8] drm/i915: Rename ctx->id to ctx->handle
Mateo Lozano, Oscar
oscar.mateo at intel.com
Tue Jul 1 17:46:51 CEST 2014
> -----Original Message-----
> From: Jesse Barnes [mailto:jbarnes at virtuousgeek.org]
> Sent: Monday, June 30, 2014 9:54 PM
> To: Mateo Lozano, Oscar
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 4/8] drm/i915: Rename ctx->id to ctx->handle
>
> On Thu, 26 Jun 2014 14:24:15 +0100
> oscar.mateo at intel.com wrote:
>
> > From: Oscar Mateo <oscar.mateo at 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 at 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).
No problem, I can do that as a follow up. How does this work? should these be kerneldoc or good ol´ code comments?
BTW: if this patch is still contended, its merging can be postponed with the rest of Execlists (the other 7 are not dependant on this one).
-- Oscar
More information about the Intel-gfx
mailing list