Re: [PATCH 13/18] drm/i915/context: create & destroy ioctls

[Daniel Vetter <daniel@ffwll.ch>]

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@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).

> +	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).

> +	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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48