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