From e9827d8ea2142d49a6622c72a3371a1d1e929c1d Mon Sep 17 00:00:00 2001
From: Thierry Reding <treding@nvidia.com>
Date: Tue, 4 Jul 2017 17:18:23 +0200
Subject: [PATCH] drm/fb-helper: Add top-level lock
Git-commit: e9827d8ea2142d49a6622c72a3371a1d1e929c1d
Patch-mainline: v4.14-rc1
References: FATE#322643 bsc#1055900
Introduce a new top-level lock for the FB helper code. This will allow
better locking granularity and avoid the need to abuse modeset locking
for this purpose instead.
This patch just adds the new lock everywhere we currently grab
mode_config->mutex (explicitly, or through drm_modeset_lock_all).
Follow-up patches will push the kms locking down into only the places
that need it.
V2:
- use lockdep_assert_held
- use drm_fb_helper_for_each_connector where possible
- use the new top-level lock consistently, i.e. in all the places
we're currently acquiring mode_config.mutex.
- small polish to the kerneldoc
Tested-by: John Stultz <john.stultz@linaro.org>
Signed-off-by: Thierry Reding <treding@nvidia.com> (v1)
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Link: http://patchwork.freedesktop.org/patch/msgid/20170704151833.17304-4-daniel.vetter@ffwll.ch
Acked-by: Takashi Iwai <tiwai@suse.de>
---
drivers/gpu/drm/drm_fb_helper.c | 47 ++++++++++++++++++++++++++++++++++++----
include/drm/drm_fb_helper.h | 19 +++++++++++++++-
2 files changed, 61 insertions(+), 5 deletions(-)
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -119,7 +119,8 @@ static int __drm_fb_helper_add_one_conne
if (!drm_fbdev_emulation)
return 0;
- WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
+ lockdep_assert_held(&fb_helper->lock);
+ lockdep_assert_held(&fb_helper->dev->mode_config.mutex);
count = fb_helper->connector_count + 1;
@@ -150,11 +151,13 @@ int drm_fb_helper_add_one_connector(stru
{
int err;
+ mutex_lock(&fb_helper->lock);
mutex_lock(&fb_helper->dev->mode_config.mutex);
err = __drm_fb_helper_add_one_connector(fb_helper, connector);
mutex_unlock(&fb_helper->dev->mode_config.mutex);
+ mutex_unlock(&fb_helper->lock);
return err;
}
@@ -184,6 +187,7 @@ int drm_fb_helper_single_add_all_connect
if (!drm_fbdev_emulation)
return 0;
+ mutex_lock(&fb_helper->lock);
mutex_lock(&dev->mode_config.mutex);
drm_connector_list_iter_begin(dev, &conn_iter);
drm_for_each_connector_iter(connector, &conn_iter) {
@@ -207,6 +211,7 @@ fail:
out:
drm_connector_list_iter_end(&conn_iter);
mutex_unlock(&dev->mode_config.mutex);
+ mutex_unlock(&fb_helper->lock);
return ret;
}
@@ -221,9 +226,9 @@ static int __drm_fb_helper_remove_one_co
if (!drm_fbdev_emulation)
return 0;
- WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
+ lockdep_assert_held(&fb_helper->lock);
- for (i = 0; i < fb_helper->connector_count; i++) {
+ drm_fb_helper_for_each_connector(fb_helper, i) {
if (fb_helper->connector_info[i]->connector == connector)
break;
}
@@ -247,11 +252,13 @@ int drm_fb_helper_remove_one_connector(s
{
int err;
+ mutex_lock(&fb_helper->lock);
mutex_lock(&fb_helper->dev->mode_config.mutex);
err = __drm_fb_helper_remove_one_connector(fb_helper, connector);
mutex_unlock(&fb_helper->dev->mode_config.mutex);
+ mutex_unlock(&fb_helper->lock);
return err;
}
@@ -503,16 +510,21 @@ int drm_fb_helper_restore_fbdev_mode_unl
if (!drm_fbdev_emulation)
return -ENODEV;
+ mutex_lock(&fb_helper->lock);
drm_modeset_lock_all(dev);
+
ret = restore_fbdev_mode(fb_helper);
do_delayed = fb_helper->delayed_hotplug;
if (do_delayed)
fb_helper->delayed_hotplug = false;
+
drm_modeset_unlock_all(dev);
+ mutex_unlock(&fb_helper->lock);
if (do_delayed)
drm_fb_helper_hotplug_event(fb_helper);
+
return ret;
}
EXPORT_SYMBOL(drm_fb_helper_restore_fbdev_mode_unlocked);
@@ -562,11 +574,13 @@ static bool drm_fb_helper_force_kernel_m
if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
continue;
+ mutex_lock(&helper->lock);
drm_modeset_lock_all(dev);
ret = restore_fbdev_mode(helper);
if (ret)
error = true;
drm_modeset_unlock_all(dev);
+ mutex_unlock(&helper->lock);
}
return error;
}
@@ -606,9 +620,11 @@ static void drm_fb_helper_dpms(struct fb
/*
* For each CRTC in this fb, turn the connectors on/off.
*/
+ mutex_lock(&fb_helper->lock);
drm_modeset_lock_all(dev);
if (!drm_fb_helper_is_bound(fb_helper)) {
drm_modeset_unlock_all(dev);
+ mutex_unlock(&fb_helper->lock);
return;
}
@@ -627,6 +643,7 @@ static void drm_fb_helper_dpms(struct fb
}
}
drm_modeset_unlock_all(dev);
+ mutex_unlock(&fb_helper->lock);
}
/**
@@ -748,6 +765,7 @@ void drm_fb_helper_prepare(struct drm_de
INIT_WORK(&helper->resume_work, drm_fb_helper_resume_worker);
INIT_WORK(&helper->dirty_work, drm_fb_helper_dirty_work);
helper->dirty_clip.x1 = helper->dirty_clip.y1 = ~0;
+ mutex_init(&helper->lock);
helper->funcs = funcs;
helper->dev = dev;
}
@@ -913,6 +931,7 @@ void drm_fb_helper_fini(struct drm_fb_he
}
mutex_unlock(&kernel_fb_helper_lock);
+ mutex_destroy(&fb_helper->lock);
drm_fb_helper_crtc_free(fb_helper);
}
@@ -1243,9 +1262,11 @@ int drm_fb_helper_setcmap(struct fb_cmap
if (oops_in_progress)
return -EBUSY;
+ mutex_lock(&fb_helper->lock);
drm_modeset_lock_all(dev);
if (!drm_fb_helper_is_bound(fb_helper)) {
drm_modeset_unlock_all(dev);
+ mutex_unlock(&fb_helper->lock);
return -EBUSY;
}
@@ -1278,6 +1299,7 @@ int drm_fb_helper_setcmap(struct fb_cmap
}
out:
drm_modeset_unlock_all(dev);
+ mutex_unlock(&fb_helper->lock);
return rc;
}
EXPORT_SYMBOL(drm_fb_helper_setcmap);
@@ -1300,6 +1322,7 @@ int drm_fb_helper_ioctl(struct fb_info *
struct drm_crtc *crtc;
int ret = 0;
+ mutex_lock(&fb_helper->lock);
mutex_lock(&dev->mode_config.mutex);
if (!drm_fb_helper_is_bound(fb_helper)) {
ret = -EBUSY;
@@ -1346,6 +1369,7 @@ int drm_fb_helper_ioctl(struct fb_info *
unlock:
mutex_unlock(&dev->mode_config.mutex);
+ mutex_unlock(&fb_helper->lock);
return ret;
}
EXPORT_SYMBOL(drm_fb_helper_ioctl);
@@ -1575,9 +1599,11 @@ int drm_fb_helper_pan_display(struct fb_
if (oops_in_progress)
return -EBUSY;
+ mutex_lock(&fb_helper->lock);
drm_modeset_lock_all(dev);
if (!drm_fb_helper_is_bound(fb_helper)) {
drm_modeset_unlock_all(dev);
+ mutex_unlock(&fb_helper->lock);
return -EBUSY;
}
@@ -1586,6 +1612,7 @@ int drm_fb_helper_pan_display(struct fb_
else
ret = pan_display_legacy(var, info);
drm_modeset_unlock_all(dev);
+ mutex_unlock(&fb_helper->lock);
return ret;
}
@@ -2252,6 +2279,7 @@ static void drm_setup_crtcs(struct drm_f
DRM_DEBUG_KMS("No connectors reported connected with modes\n");
/* prevent concurrent modification of connector_count by hotplug */
+ lockdep_assert_held(&fb_helper->lock);
lockdep_assert_held(&fb_helper->dev->mode_config.mutex);
crtcs = kcalloc(fb_helper->connector_count,
@@ -2378,12 +2406,14 @@ int drm_fb_helper_initial_config(struct
if (!drm_fbdev_emulation)
return 0;
+ mutex_lock(&fb_helper->lock);
mutex_lock(&dev->mode_config.mutex);
drm_setup_crtcs(fb_helper,
dev->mode_config.max_width,
dev->mode_config.max_height);
ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
mutex_unlock(&dev->mode_config.mutex);
+ mutex_unlock(&fb_helper->lock);
if (ret)
return ret;
@@ -2431,25 +2461,34 @@ EXPORT_SYMBOL(drm_fb_helper_initial_conf
int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
{
struct drm_device *dev = fb_helper->dev;
+ int err = 0;
if (!drm_fbdev_emulation)
return 0;
+ mutex_lock(&fb_helper->lock);
mutex_lock(&dev->mode_config.mutex);
+
if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
fb_helper->delayed_hotplug = true;
mutex_unlock(&dev->mode_config.mutex);
- return 0;
+ goto unlock;
}
+
DRM_DEBUG_KMS("\n");
drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height);
mutex_unlock(&dev->mode_config.mutex);
+ mutex_unlock(&fb_helper->lock);
drm_fb_helper_set_par(fb_helper->fbdev);
return 0;
+
+unlock:
+ mutex_unlock(&fb_helper->lock);
+ return err;
}
EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -169,7 +169,6 @@ struct drm_fb_helper_connector {
* @crtc_info: per-CRTC helper state (mode, x/y offset, etc)
* @connector_count: number of connected connectors
* @connector_info_alloc_count: size of connector_info
- * @connector_info: array of per-connector information
* @funcs: driver callbacks for fb helper
* @fbdev: emulated fbdev device info struct
* @pseudo_palette: fake palette of 16 colors
@@ -191,6 +190,12 @@ struct drm_fb_helper {
struct drm_fb_helper_crtc *crtc_info;
int connector_count;
int connector_info_alloc_count;
+ /**
+ * @connector_info:
+ *
+ * Array of per-connector information. Do not iterate directly, but use
+ * drm_fb_helper_for_each_connector.
+ */
struct drm_fb_helper_connector **connector_info;
const struct drm_fb_helper_funcs *funcs;
struct fb_info *fbdev;
@@ -201,6 +206,18 @@ struct drm_fb_helper {
struct work_struct resume_work;
/**
+ * @lock:
+ *
+ * Top-level FBDEV helper lock. This protects all internal data
+ * structures and lists, such as @connector_info and @crtc_info.
+ *
+ * FIXME: fbdev emulation locking is a mess and long term we want to
+ * protect all helper internal state with this lock as well as reduce
+ * core KMS locking as much as possible.
+ */
+ struct mutex lock;
+
+ /**
* @kernel_fb_list:
*
* Entry on the global kernel_fb_helper_list, used for kgdb entry/exit.