[Intel-gfx] [PATCH 2/7] drm/i915: Move context special case to get()

[Ben Widawsky]

On Fri, Apr 05, 2013 at 01:41:11PM +0300, Ville Syrjälä wrote:
> On Thu, Apr 04, 2013 at 04:41:48PM -0700, Ben Widawsky wrote:
> > This allows us to make upcoming refcounting code a bit simpler, and
> > cleaner. In addition, I think it makes the interface a bit nicer if the
> > caller doesn't need to figure out default contexts and such.
> > 
> > The interface works very similarly to the gem object ref counting, and
> > I believe it makes sense to do so as we'll use it in a very similar way
> > to objects (we currently use objects as a somewhat hackish way to manage
> > context lifecycles).
> > 
> > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_context.c | 34 +++++++++++++++++++--------------
> >  1 file changed, 20 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index aa080ea..6211637 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -95,8 +95,6 @@
> >   */
> >  #define CONTEXT_ALIGN (64<<10)
> >  
> > -static struct i915_hw_context *
> > -i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id);
> >  static int do_switch(struct i915_hw_context *to);
> >  
> >  static int get_context_size(struct drm_device *dev)
> > @@ -291,9 +289,18 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
> >  }
> >  
> >  static struct i915_hw_context *
> > -i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
> > +i915_gem_context_get(struct intel_ring_buffer *ring,
> > +		     struct drm_i915_file_private *file_priv, u32 id)
> >  {
> > -	return (struct i915_hw_context *)idr_find(&file_priv->context_idr, id);
> > +	struct i915_hw_context *ctx;
> > +
> > +	if (id == DEFAULT_CONTEXT_ID)
> > +		ctx = ring->default_context;
> > +	else
> > +		ctx = (struct i915_hw_context *)
> > +			idr_find(&file_priv->context_idr, id);
> > +
> > +	return ctx;
> >  }
> >  
> >  static inline int
> > @@ -440,16 +447,12 @@ int i915_switch_context(struct intel_ring_buffer *ring,
> >  	if (ring != &dev_priv->ring[RCS])
> >  		return 0;
> >  
> > -	if (to_id == DEFAULT_CONTEXT_ID) {
> > -		to = ring->default_context;
> > -	} else {
> > -		if (file == NULL)
> > -			return -EINVAL;
> > +	if (file == NULL)
> > +		return -EINVAL;
> 
> This looks wrong. Before the NULL check was only done when to_id !=
> DEFAULT_CONTEXT_ID. Now it's done always, which means the call from
> i915_gpu_idle() will always fail.
> 

Yeah, this definitely is incorrect. I wonder why I didn't hit any
problems on the i-g-t suite. Thanks for finding it. I'll come up with
some fix locally.

> >  
> > -		to = i915_gem_context_get(file->driver_priv, to_id);
> > -		if (to == NULL)
> > -			return -ENOENT;
> > -	}
> > +	to = i915_gem_context_get(ring, file->driver_priv, to_id);
> > +	if (to == NULL)
> > +		return -ENOENT;
> >  
> >  	return do_switch(to);
> >  }
> > @@ -495,11 +498,14 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
> >  	if (!(dev->driver->driver_features & DRIVER_GEM))
> >  		return -ENODEV;
> >  
> > +	if (args->ctx_id == DEFAULT_CONTEXT_ID)
> > +		return -ENOENT;
> > +
> >  	ret = i915_mutex_lock_interruptible(dev);
> >  	if (ret)
> >  		return ret;
> >  
> > -	ctx = i915_gem_context_get(file_priv, args->ctx_id);
> > +	ctx = i915_gem_context_get(NULL, file_priv, args->ctx_id);
> >  	if (!ctx) {
> >  		mutex_unlock(&dev->struct_mutex);
> >  		return -ENOENT;
> > -- 
> > 1.8.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Ben Widawsky, Intel Open Source Technology Center