Skip to content

Commit

Permalink
drm/fb-helper: Add top-level lock
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
Thierry Reding authored and Daniel Vetter committed Jul 6, 2017
1 parent 666b7cd commit e9827d8
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 5 deletions.
47 changes: 43 additions & 4 deletions drivers/gpu/drm/drm_fb_helper.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,8 @@ static int __drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
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;

Expand Down Expand Up @@ -150,11 +151,13 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
{
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;
}
Expand Down Expand Up @@ -184,6 +187,7 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
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) {
Expand All @@ -207,6 +211,7 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
out:
drm_connector_list_iter_end(&conn_iter);
mutex_unlock(&dev->mode_config.mutex);
mutex_unlock(&fb_helper->lock);

return ret;
}
Expand All @@ -221,9 +226,9 @@ static int __drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
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;
}
Expand All @@ -247,11 +252,13 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
{
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;
}
Expand Down Expand Up @@ -503,16 +510,21 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
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);
Expand Down Expand Up @@ -562,11 +574,13 @@ static bool drm_fb_helper_force_kernel_mode(void)
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;
}
Expand Down Expand Up @@ -606,9 +620,11 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
/*
* 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;
}

Expand All @@ -627,6 +643,7 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
}
}
drm_modeset_unlock_all(dev);
mutex_unlock(&fb_helper->lock);
}

/**
Expand Down Expand Up @@ -748,6 +765,7 @@ void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper,
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;
}
Expand Down Expand Up @@ -913,6 +931,7 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
}
mutex_unlock(&kernel_fb_helper_lock);

mutex_destroy(&fb_helper->lock);
drm_fb_helper_crtc_free(fb_helper);

}
Expand Down Expand Up @@ -1243,9 +1262,11 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
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;
}

Expand Down Expand Up @@ -1278,6 +1299,7 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
}
out:
drm_modeset_unlock_all(dev);
mutex_unlock(&fb_helper->lock);
return rc;
}
EXPORT_SYMBOL(drm_fb_helper_setcmap);
Expand All @@ -1300,6 +1322,7 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd,
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;
Expand Down Expand Up @@ -1346,6 +1369,7 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd,

unlock:
mutex_unlock(&dev->mode_config.mutex);
mutex_unlock(&fb_helper->lock);
return ret;
}
EXPORT_SYMBOL(drm_fb_helper_ioctl);
Expand Down Expand Up @@ -1575,9 +1599,11 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
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;
}

Expand All @@ -1586,6 +1612,7 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
else
ret = pan_display_legacy(var, info);
drm_modeset_unlock_all(dev);
mutex_unlock(&fb_helper->lock);

return ret;
}
Expand Down Expand Up @@ -2252,6 +2279,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
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,
Expand Down Expand Up @@ -2378,12 +2406,14 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
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;

Expand Down Expand Up @@ -2431,25 +2461,34 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
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);

Expand Down
19 changes: 18 additions & 1 deletion include/drm/drm_fb_helper.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
Expand All @@ -200,6 +205,18 @@ struct drm_fb_helper {
struct work_struct dirty_work;
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:
*
Expand Down

0 comments on commit e9827d8

Please sign in to comment.