On Thu, 29 Mar 2012 21:35:35 +0200
Daniel Vetter <daniel at ffwll.ch> wrote:
> On Sun, Mar 18, 2012 at 01:39:53PM -0700, Ben Widawsky wrote:
> > Add the interfaces to allow user space to create and destroy contexts.
> > Contexts are destroyed automatically if the file descriptor for the dri
> > device is closed.
> >
> > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > ---
> > drivers/gpu/drm/i915/i915_dma.c | 2 +
> > drivers/gpu/drm/i915/i915_drv.h | 4 ++
> > drivers/gpu/drm/i915/i915_gem_context.c | 64 +++++++++++++++++++++++++++++++
> > include/drm/i915_drm.h | 16 ++++++++
> > 4 files changed, 86 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 4c7c1dc..fb3fccb 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -2333,6 +2333,8 @@ struct drm_ioctl_desc i915_ioctls[] = {
> > DRM_IOCTL_DEF_DRV(I915_OVERLAY_ATTRS, intel_overlay_attrs, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> > DRM_IOCTL_DEF_DRV(I915_SET_SPRITE_COLORKEY, intel_sprite_set_colorkey, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> > DRM_IOCTL_DEF_DRV(I915_GET_SPRITE_COLORKEY, intel_sprite_get_colorkey, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> > + DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE, i915_gem_context_create_ioctl, DRM_UNLOCKED),
> > + DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, i915_gem_context_destroy_ioctl, DRM_UNLOCKED),
> > };
> >
> > int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index c6c2ada..d49615e 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1312,6 +1312,10 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
> > int i915_switch_context(struct intel_ring_buffer *ring,
> > struct drm_file *file,
> > int to_id, u32 seqno, u32 flags);
> > +int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> > + struct drm_file *file);
> > +int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
> > + struct drm_file *file);
> >
> > /* i915_gem_gtt.c */
> > int __must_check i915_gem_init_aliasing_ppgtt(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index d9a08f2..accb3de 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -411,3 +411,67 @@ out:
> > kref_put(&to->nref, destroy_hw_context);
> > return ret;
> > }
> > +
> > +int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> > + struct drm_file *file)
> > +{
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > + struct drm_i915_gem_context_create *args = data;
> > + struct drm_i915_file_private *file_priv = file->driver_priv;
> > + struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
> > + struct i915_hw_context *ctx;
> > + int ret;
> > +
> > + if (dev_priv->hw_contexts_disabled)
> > + return -EPERM;
> > +
> > + ret = create_hw_context(dev, file_priv, &ctx);
>
> Note that the gem_alloc_object call in here is rather unsafe, due to the
> unsafe statistics-keeping in i915_gem_info_add_obj. Pre-existing looking
> goof-up, but can I maybe volunteer you to fix this? I think we need an
> mm stats spinlock because these counters could be rather big (and atomic_t
> is only 32 bits).
I can do it. In this series?
>
> > + if (ret)
> > + return ret;
> > +
> > + /* We need to do a special switch (save-only) either now, or before the
> > + * context is actually used in order to keep the context switch request
> > + * in execbuffer fairly simple.
> > + */
> > + mutex_lock(&dev->struct_mutex);
>
> > + ret = i915_switch_context(ring, file, ctx->id, ring->get_seqno(ring),
> > + I915_CONTEXT_INITIAL_SWITCH);
>
> With the hw_context->is_intialized change you could ditch this (imo).
You could ditch it, but I can't make an argument that one way is better
than another. As you stated earlier, you want me to ditch the spinlock
for the context id creation, so I'll still need to acquire struct mutex
anyway. At which point, I think it doesn't hurt to switch now too.
I'm willing to change this is you can describe a benefit for it.
>
> > + mutex_unlock(&dev->struct_mutex);
> > + if (ret)
> > + return ret;
> > +
> > + args->ctx_id = ctx->id;
> > + DRM_DEBUG_DRIVER("HW context %d created\n", args->ctx_id);
> > + return ret;
> > +}
> > +
> > +int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
> > + struct drm_file *file)
> > +{
> > + 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 i915_hw_context *ctx;
> > +
> > + if (dev_priv->hw_contexts_disabled)
> > + return -EPERM;
> > +
> > + mutex_lock(&dev->struct_mutex);
> > + ctx = i915_gem_context_get(file_priv, args->ctx_id);
> > + if (!ctx) {
> > + mutex_unlock(&dev->struct_mutex);
> > + return -EINVAL;
> > + }
> > +
> > + /* The first put is for the context ref we got just up above. The second
> > + * put should unref the original ref (and will therefore destroy the context
> > + * unless someone else holds a reference
> > + */
> > + kref_put(&ctx->nref, destroy_hw_context);
> > + kref_put(&ctx->nref, destroy_hw_context);
> > +
> > + mutex_unlock(&dev->struct_mutex);
> > +
> > + DRM_DEBUG_DRIVER("HW context %d destroyed\n", args->ctx_id);
> > + return 0;
> > +}
> > diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> > index da929bb..bead13e 100644
> > --- a/include/drm/i915_drm.h
> > +++ b/include/drm/i915_drm.h
> > @@ -200,6 +200,8 @@ typedef struct _drm_i915_sarea {
> > #define DRM_I915_GEM_EXECBUFFER2 0x29
> > #define DRM_I915_GET_SPRITE_COLORKEY 0x2a
> > #define DRM_I915_SET_SPRITE_COLORKEY 0x2b
> > +#define DRM_I915_GEM_CONTEXT_CREATE 0x2c
> > +#define DRM_I915_GEM_CONTEXT_DESTROY 0x2d
> >
> > #define DRM_IOCTL_I915_INIT DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t)
> > #define DRM_IOCTL_I915_FLUSH DRM_IO ( DRM_COMMAND_BASE + DRM_I915_FLUSH)
> > @@ -243,6 +245,9 @@ typedef struct _drm_i915_sarea {
> > #define DRM_IOCTL_I915_OVERLAY_ATTRS DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_OVERLAY_ATTRS, struct drm_intel_overlay_attrs)
> > #define DRM_IOCTL_I915_SET_SPRITE_COLORKEY DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_SET_SPRITE_COLORKEY, struct drm_intel_sprite_colorkey)
> > #define DRM_IOCTL_I915_GET_SPRITE_COLORKEY DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_SET_SPRITE_COLORKEY, struct drm_intel_sprite_colorkey)
> > +#define DRM_IOCTL_I915_GEM_CONTEXT_CREATE DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_CREATE, struct drm_i915_gem_context_create)
> > +#define DRM_IOCTL_I915_GEM_CONTEXT_DESTROY DRM_IOW (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_DESTROY, struct drm_i915_gem_context_destroy)
> > +
> >
> > /* Allow drivers to submit batchbuffers directly to hardware, relying
> > * on the security mechanisms provided by hardware.
> > @@ -885,4 +890,15 @@ struct drm_intel_sprite_colorkey {
> > __u32 flags;
> > };
> >
> > +struct drm_i915_gem_context_create {
> > + /* output: id of new context*/
> > + __u32 ctx_id;
> > + __u32 pad;
> > +};
> > +
> > +struct drm_i915_gem_context_destroy {
> > + __u32 ctx_id;
> > + __u32 pad;
> > +};
> > +
> > #endif /* _I915_DRM_H_ */
> > --
> > 1.7.9.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
