Re: [PATCH 1/2] drm: make idr_mutex a spinlock

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Aug 29, 2014 at 02:01:00PM +0200, David Herrmann wrote:
> There is no reason to use a heavy mutex for idr protection. Use a spinlock
> and make idr-allocation use idr_preload().
> 
> This patch also makes mode-object lookup irq-save, in case you ever wanna
> lookup modeset objects from interrupts. This is just a side-effect of
> avoiding a mutex.
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>

I've thought irqsave/restore are terribly serializing instructions, so
this might actually be slower than a plain mutex. And imo if it doesn't
show up in profiles it's not worth to optimize it - generally mutexes are
really fast and in most cases already nicely degenerate to spinlocks
anyway.
-Daniel

> ---
>  drivers/gpu/drm/drm_crtc.c | 34 ++++++++++++++++++++--------------
>  include/drm/drm_crtc.h     |  4 ++--
>  2 files changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 61b6978..97eba56 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -283,8 +283,10 @@ static int drm_mode_object_get_reg(struct drm_device *dev,
>  {
>  	int ret;
>  
> -	mutex_lock(&dev->mode_config.idr_mutex);
> -	ret = idr_alloc(&dev->mode_config.crtc_idr, register_obj ? obj : NULL, 1, 0, GFP_KERNEL);
> +	idr_preload(GFP_KERNEL);
> +	spin_lock_irq(&dev->mode_config.idr_lock);
> +
> +	ret = idr_alloc(&dev->mode_config.crtc_idr, register_obj ? obj : NULL, 1, 0, GFP_NOWAIT);
>  	if (ret >= 0) {
>  		/*
>  		 * Set up the object linking under the protection of the idr
> @@ -293,7 +295,9 @@ static int drm_mode_object_get_reg(struct drm_device *dev,
>  		obj->id = ret;
>  		obj->type = obj_type;
>  	}
> -	mutex_unlock(&dev->mode_config.idr_mutex);
> +
> +	spin_unlock_irq(&dev->mode_config.idr_lock);
> +	idr_preload_end();
>  
>  	return ret < 0 ? ret : 0;
>  }
> @@ -322,9 +326,9 @@ int drm_mode_object_get(struct drm_device *dev,
>  static void drm_mode_object_register(struct drm_device *dev,
>  				     struct drm_mode_object *obj)
>  {
> -	mutex_lock(&dev->mode_config.idr_mutex);
> +	spin_lock_irq(&dev->mode_config.idr_lock);
>  	idr_replace(&dev->mode_config.crtc_idr, obj, obj->id);
> -	mutex_unlock(&dev->mode_config.idr_mutex);
> +	spin_unlock_irq(&dev->mode_config.idr_lock);
>  }
>  
>  /**
> @@ -339,17 +343,18 @@ static void drm_mode_object_register(struct drm_device *dev,
>  void drm_mode_object_put(struct drm_device *dev,
>  			 struct drm_mode_object *object)
>  {
> -	mutex_lock(&dev->mode_config.idr_mutex);
> +	spin_lock_irq(&dev->mode_config.idr_lock);
>  	idr_remove(&dev->mode_config.crtc_idr, object->id);
> -	mutex_unlock(&dev->mode_config.idr_mutex);
> +	spin_unlock_irq(&dev->mode_config.idr_lock);
>  }
>  
>  static struct drm_mode_object *_object_find(struct drm_device *dev,
>  		uint32_t id, uint32_t type)
>  {
>  	struct drm_mode_object *obj = NULL;
> +	unsigned long flags;
>  
> -	mutex_lock(&dev->mode_config.idr_mutex);
> +	spin_lock_irqsave(&dev->mode_config.idr_lock, flags);
>  	obj = idr_find(&dev->mode_config.crtc_idr, id);
>  	if (obj && type != DRM_MODE_OBJECT_ANY && obj->type != type)
>  		obj = NULL;
> @@ -358,7 +363,7 @@ static struct drm_mode_object *_object_find(struct drm_device *dev,
>  	/* don't leak out unref'd fb's */
>  	if (obj && (obj->type == DRM_MODE_OBJECT_FB))
>  		obj = NULL;
> -	mutex_unlock(&dev->mode_config.idr_mutex);
> +	spin_unlock_irqrestore(&dev->mode_config.idr_lock, flags);
>  
>  	return obj;
>  }
> @@ -433,9 +438,9 @@ EXPORT_SYMBOL(drm_framebuffer_init);
>  static void __drm_framebuffer_unregister(struct drm_device *dev,
>  					 struct drm_framebuffer *fb)
>  {
> -	mutex_lock(&dev->mode_config.idr_mutex);
> +	spin_lock_irq(&dev->mode_config.idr_lock);
>  	idr_remove(&dev->mode_config.crtc_idr, fb->base.id);
> -	mutex_unlock(&dev->mode_config.idr_mutex);
> +	spin_unlock_irq(&dev->mode_config.idr_lock);
>  
>  	fb->base.id = 0;
>  }
> @@ -465,14 +470,15 @@ static struct drm_framebuffer *__drm_framebuffer_lookup(struct drm_device *dev,
>  {
>  	struct drm_mode_object *obj = NULL;
>  	struct drm_framebuffer *fb;
> +	unsigned long flags;
>  
> -	mutex_lock(&dev->mode_config.idr_mutex);
> +	spin_lock_irqsave(&dev->mode_config.idr_lock, flags);
>  	obj = idr_find(&dev->mode_config.crtc_idr, id);
>  	if (!obj || (obj->type != DRM_MODE_OBJECT_FB) || (obj->id != id))
>  		fb = NULL;
>  	else
>  		fb = obj_to_fb(obj);
> -	mutex_unlock(&dev->mode_config.idr_mutex);
> +	spin_unlock_irqrestore(&dev->mode_config.idr_lock, flags);
>  
>  	return fb;
>  }
> @@ -5049,7 +5055,7 @@ void drm_mode_config_init(struct drm_device *dev)
>  {
>  	mutex_init(&dev->mode_config.mutex);
>  	drm_modeset_lock_init(&dev->mode_config.connection_mutex);
> -	mutex_init(&dev->mode_config.idr_mutex);
> +	spin_lock_init(&dev->mode_config.idr_lock);
>  	mutex_init(&dev->mode_config.fb_lock);
>  	INIT_LIST_HEAD(&dev->mode_config.fb_list);
>  	INIT_LIST_HEAD(&dev->mode_config.crtc_list);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 77d9763..9c57b56 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -742,7 +742,7 @@ struct drm_mode_group {
>  /**
>   * drm_mode_config - Mode configuration control structure
>   * @mutex: mutex protecting KMS related lists and structures
> - * @idr_mutex: mutex for KMS ID allocation and management
> + * @idr_lock: lock for KMS ID allocation and management
>   * @crtc_idr: main KMS ID tracking object
>   * @num_fb: number of fbs available
>   * @fb_list: list of framebuffers available
> @@ -772,7 +772,7 @@ struct drm_mode_config {
>  	struct mutex mutex; /* protects configuration (mode lists etc.) */
>  	struct drm_modeset_lock connection_mutex; /* protects connector->encoder and encoder->crtc links */
>  	struct drm_modeset_acquire_ctx *acquire_ctx; /* for legacy _lock_all() / _unlock_all() */
> -	struct mutex idr_mutex; /* for IDR management */
> +	spinlock_t idr_lock; /* for IDR management */
>  	struct idr crtc_idr; /* use this idr for all IDs, fb, crtc, connector, modes - just makes life easier */
>  	/* this is limited to one for now */
>  
> -- 
> 2.1.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch