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