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 at 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
