Re: [PATCH 2/4] drm/i915: initialize the context idr unconditionally

[Ben Widawsky <ben@bwidawsk.net>]

On Tue, 19 Jun 2012 16:52:30 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> It doesn't hurt and it at least prevents us from OOPSing left and
> right at quite a few places. This also allows us to simplify the code
> a bit by folding the only line of context_open into the callsite.
> 
> We obviuosly also need to run the cleanup code unconditionally, too.
> 

> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Open did a couple more things in previous incantations. I liked the idea
of having a discrete open function, so it's easy, and obvious where to
add stuff when needed. It also adds symmetry for close. One such use for
open in the past was to shoehorn the default context as a locatable
context per fd. This eliminated the need for having special
DEFAULT_CONTEXT_ID conditions.

Anyway, I like the fact that this fixes bugs, but dislike the fact that
open went away. However, since you found, and fixed the issue, I'll
defer to your wishes.

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

> ---
>  drivers/gpu/drm/i915/i915_dma.c         |    2 +-
>  drivers/gpu/drm/i915/i915_drv.h         |    1 -
>  drivers/gpu/drm/i915/i915_gem_context.c |   15 ---------------
>  3 files changed, 1 insertion(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index a47ed44..e36f6ce 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1766,7 +1766,7 @@ int i915_driver_open(struct drm_device *dev, struct drm_file *file)
>  	spin_lock_init(&file_priv->mm.lock);
>  	INIT_LIST_HEAD(&file_priv->mm.request_list);
>  
> -	i915_gem_context_open(dev, file);
> +	idr_init(&file_priv->context_idr);
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b65d156..d1b4950 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1392,7 +1392,6 @@ struct dma_buf *i915_gem_prime_export(struct drm_device *dev,
>  /* i915_gem_context.c */
>  void i915_gem_context_init(struct drm_device *dev);
>  void i915_gem_context_fini(struct drm_device *dev);
> -void i915_gem_context_open(struct drm_device *dev, struct drm_file *file);
>  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);
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 693718b..671927a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -282,17 +282,6 @@ void i915_gem_context_fini(struct drm_device *dev)
>  	do_destroy(dev_priv->ring[RCS].default_context);
>  }
>  
> -void i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
> -{
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct drm_i915_file_private *file_priv = file->driver_priv;
> -
> -	if (dev_priv->hw_contexts_disabled)
> -		return;
> -
> -	idr_init(&file_priv->context_idr);
> -}
> -
>  static int context_idr_cleanup(int id, void *p, void *data)
>  {
>  	struct drm_file *file = (struct drm_file *)data;
> @@ -311,12 +300,8 @@ static int context_idr_cleanup(int id, void *p, void *data)
>  
>  void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
>  {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_file_private *file_priv = file->driver_priv;
>  
> -	if (dev_priv->hw_contexts_disabled)
> -		return;
> -
>  	mutex_lock(&dev->struct_mutex);
>  	idr_for_each(&file_priv->context_idr, context_idr_cleanup, file);
>  	idr_destroy(&file_priv->context_idr);



-- 
Ben Widawsky, Intel Open Source Technology Center