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

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Mar 30, 2012 at 11:55:14AM -0700, Ben Widawsky wrote:
> On Thu, 29 Mar 2012 21:35:35 +0200
> Daniel Vetter <daniel@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@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?

That bug is pre-existing since gem was merged afaik. So just when you're bored, in a
separate 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.

Imo the benefit is that we have one (and just one) clear place where we
switch context. Doing a funky switch at create time just feels wonky. Just
now I've also noticed that you call ring->get_seqno(ring) - that reads the
current seqno from the gpu hws and is absolutely not what you want. So
you'd have to add another request (and attach it to the file_priv).

I also like that with the is_initialized stored in the context, we nicely
abstract away this little detail of how we need to frob the switch command
on first use of a context.

I don't feel strongly about this, but if you want to stick with explicit
initialization, please wrap it all up in a little helper function to make
it clear that we don't care about switching and only about initializing.
Imo you should then still rip away the flags parameter from
i915_switch_context - execbuffer really doesn't (and shouldn't) care about
that detail.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48