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

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Ben Widawsky <ben@bwidawsk.net>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 2/7] drm/i915: Move context special case to get()
Date: Fri, 5 Apr 2013 13:41:11 +0300	[thread overview]
Message-ID: <20130405104111.GC4469@intel.com> (raw)
In-Reply-To: <1365118914-15753-3-git-send-email-ben@bwidawsk.net>

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

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

-- 
Ville Syrjälä
Intel OTC