| Message ID | 20170329144401.1804-7-thierry.reding@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 29, 2017 at 04:43:56PM +0200, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > The existing drm_fb_helper_hotplug_event() function needs to take the > top-level fb-helper lock. However, the function can also be called from > code that has already taken this lock. Introduce an unlocked variant of > this function that can be used in the latter case. > > This function calls drm_fb_helper_restore_fbdev_mode_unlocked(), via > drm_fb_helper_set_par(), so we also need to introduce an unlocked copy > of that to avoid recursive locking issues. > > Similarly, the drm_fb_helper_initial_config() function ends up calling > drm_fb_helper_set_par(), via register_framebuffer(), and needs an > unlocked variant to avoid recursive locking. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > drivers/gpu/drm/drm_fb_helper.c | 167 +++++++++++++++++++++++++--------------- > 1 file changed, 104 insertions(+), 63 deletions(-) Note that this could probably be squashed into 05/11, but I left it separate for easier review since it's the only new patch in the series. Also I had originally split this into three separate patches, but the recursive call stack for the drm_fb_helper_hotplug_event() function and drm_fb_helper_restore_fbdev_mode_unlocked() made it impossible to keep bisectibility across the series. Thierry
On Wed, Mar 29, 2017 at 04:43:56PM +0200, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > The existing drm_fb_helper_hotplug_event() function needs to take the > top-level fb-helper lock. However, the function can also be called from > code that has already taken this lock. Introduce an unlocked variant of > this function that can be used in the latter case. > > This function calls drm_fb_helper_restore_fbdev_mode_unlocked(), via > drm_fb_helper_set_par(), so we also need to introduce an unlocked copy > of that to avoid recursive locking issues. > > Similarly, the drm_fb_helper_initial_config() function ends up calling > drm_fb_helper_set_par(), via register_framebuffer(), and needs an > unlocked variant to avoid recursive locking. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > drivers/gpu/drm/drm_fb_helper.c | 167 +++++++++++++++++++++++++--------------- > 1 file changed, 104 insertions(+), 63 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 860be51d92f6..21a90322531c 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -491,18 +491,10 @@ static int restore_fbdev_mode(struct drm_fb_helper *fb_helper) > return 0; > } > > -/** > - * drm_fb_helper_restore_fbdev_mode_unlocked - restore fbdev configuration > - * @fb_helper: fbcon to restore > - * > - * This should be called from driver's drm &drm_driver.lastclose callback > - * when implementing an fbcon on top of kms using this helper. This ensures that > - * the user isn't greeted with a black screen when e.g. X dies. > - * > - * RETURNS: > - * Zero if everything went ok, negative error code otherwise. > - */ > -int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) > +static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper); > + > +static int > +__drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) > { > struct drm_device *dev = fb_helper->dev; > bool do_delayed; > @@ -511,7 +503,8 @@ 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); > + WARN_ON(!mutex_is_locked(&fb_helper->lock)); lockdep_assert_held is the new cool. > + > drm_modeset_lock_all(dev); > > ret = restore_fbdev_mode(fb_helper); > @@ -521,10 +514,31 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) > 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); > + __drm_fb_helper_hotplug_event(fb_helper); > + > + return ret; > +} > + > +/** > + * drm_fb_helper_restore_fbdev_mode_unlocked - restore fbdev configuration > + * @fb_helper: fbcon to restore > + * > + * This should be called from driver's drm &drm_driver.lastclose callback > + * when implementing an fbcon on top of kms using this helper. This ensures that > + * the user isn't greeted with a black screen when e.g. X dies. > + * > + * RETURNS: > + * Zero if everything went ok, negative error code otherwise. > + */ > +int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) > +{ > + int ret; > + > + mutex_lock(&fb_helper->lock); > + ret = __drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper); > + mutex_unlock(&fb_helper->lock); > > return ret; > } > @@ -1486,7 +1500,7 @@ int drm_fb_helper_set_par(struct fb_info *info) > return -EINVAL; > } > > - drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper); > + __drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper); Nah, you need the locking still for when this is called from userspace through fbdev ioctl. > > return 0; > } > @@ -2333,6 +2347,46 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper, > kfree(enabled); > } > > +static int __drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, > + int bpp_sel) > +{ > + struct drm_device *dev = fb_helper->dev; > + struct fb_info *info; > + int ret; > + > + if (!drm_fbdev_emulation) > + return 0; > + > + WARN_ON(!mutex_is_locked(&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); > + if (ret) > + return ret; > + > + info = fb_helper->fbdev; > + info->var.pixclock = 0; > + ret = register_framebuffer(info); > + if (ret < 0) > + return ret; > + > + dev_info(dev->dev, "fb%d: %s frame buffer device\n", > + info->node, info->fix.id); > + > + mutex_lock(&kernel_fb_helper_lock); > + if (list_empty(&kernel_fb_helper_list)) > + register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op); > + > + list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list); > + mutex_unlock(&kernel_fb_helper_lock); > + > + return 0; > +} > + > /** > * drm_fb_helper_initial_config - setup a sane initial connector configuration > * @fb_helper: fb_helper device struct > @@ -2377,41 +2431,50 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper, > */ > int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel) > { > - struct drm_device *dev = fb_helper->dev; > - struct fb_info *info; > int ret; > > + mutex_lock(&fb_helper->lock); > + ret = __drm_fb_helper_initial_config(fb_helper, bpp_sel); > + mutex_unlock(&fb_helper->lock); Yeah this won't work because it'll deadlock with the register_framebuffer. You need to push it down so that it only protects the same critical section as mode_config.mutex currently ddoes. > + > + return ret; > +} > +EXPORT_SYMBOL(drm_fb_helper_initial_config); > + > +static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) > +{ > + struct drm_device *dev = fb_helper->dev; > + unsigned int width, height; > + > if (!drm_fbdev_emulation) > return 0; > > + WARN_ON(!mutex_is_locked(&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); > - if (ret) > - return ret; > > - info = fb_helper->fbdev; > - info->var.pixclock = 0; > - ret = register_framebuffer(info); > - if (ret < 0) > - return ret; > + if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) { > + fb_helper->delayed_hotplug = true; > + mutex_unlock(&dev->mode_config.mutex); > + return 0; > + } > > - dev_info(dev->dev, "fb%d: %s frame buffer device\n", > - info->node, info->fix.id); > + DRM_DEBUG_KMS("\n"); > > - mutex_lock(&kernel_fb_helper_lock); > - if (list_empty(&kernel_fb_helper_list)) > - register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op); > + width = dev->mode_config.max_width; > + height = dev->mode_config.max_height; > > - list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list); > - mutex_unlock(&kernel_fb_helper_lock); > + if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0) > + DRM_DEBUG_KMS("No connectors reported connected with modes\n"); > + > + drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height); > + > + mutex_unlock(&dev->mode_config.mutex); > + > + drm_fb_helper_set_par(fb_helper->fbdev); > > return 0; > } > -EXPORT_SYMBOL(drm_fb_helper_initial_config); > > /** > * drm_fb_helper_hotplug_event - respond to a hotplug notification by > @@ -2436,35 +2499,13 @@ 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; > + int ret; > > 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); > - 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); > + ret = __drm_fb_helper_hotplug_event(fb_helper); > mutex_unlock(&fb_helper->lock); Yeah you can't hold the lock through the entire hotplug_event process, otherwise the potential register_framebuffer will go boom on a deadlock. I'll reply to one of the other patches with what I think should be done. -Daniel > > - drm_fb_helper_set_par(fb_helper->fbdev); > - > - return 0; > - > -unlock: > - mutex_unlock(&fb_helper->lock); > - return err; > + return ret; > } > EXPORT_SYMBOL(drm_fb_helper_hotplug_event); > > -- > 2.12.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 860be51d92f6..21a90322531c 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -491,18 +491,10 @@ static int restore_fbdev_mode(struct drm_fb_helper *fb_helper) return 0; } -/** - * drm_fb_helper_restore_fbdev_mode_unlocked - restore fbdev configuration - * @fb_helper: fbcon to restore - * - * This should be called from driver's drm &drm_driver.lastclose callback - * when implementing an fbcon on top of kms using this helper. This ensures that - * the user isn't greeted with a black screen when e.g. X dies. - * - * RETURNS: - * Zero if everything went ok, negative error code otherwise. - */ -int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) +static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper); + +static int +__drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) { struct drm_device *dev = fb_helper->dev; bool do_delayed; @@ -511,7 +503,8 @@ 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); + WARN_ON(!mutex_is_locked(&fb_helper->lock)); + drm_modeset_lock_all(dev); ret = restore_fbdev_mode(fb_helper); @@ -521,10 +514,31 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) 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); + __drm_fb_helper_hotplug_event(fb_helper); + + return ret; +} + +/** + * drm_fb_helper_restore_fbdev_mode_unlocked - restore fbdev configuration + * @fb_helper: fbcon to restore + * + * This should be called from driver's drm &drm_driver.lastclose callback + * when implementing an fbcon on top of kms using this helper. This ensures that + * the user isn't greeted with a black screen when e.g. X dies. + * + * RETURNS: + * Zero if everything went ok, negative error code otherwise. + */ +int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) +{ + int ret; + + mutex_lock(&fb_helper->lock); + ret = __drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper); + mutex_unlock(&fb_helper->lock); return ret; } @@ -1486,7 +1500,7 @@ int drm_fb_helper_set_par(struct fb_info *info) return -EINVAL; } - drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper); + __drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper); return 0; } @@ -2333,6 +2347,46 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper, kfree(enabled); } +static int __drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, + int bpp_sel) +{ + struct drm_device *dev = fb_helper->dev; + struct fb_info *info; + int ret; + + if (!drm_fbdev_emulation) + return 0; + + WARN_ON(!mutex_is_locked(&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); + if (ret) + return ret; + + info = fb_helper->fbdev; + info->var.pixclock = 0; + ret = register_framebuffer(info); + if (ret < 0) + return ret; + + dev_info(dev->dev, "fb%d: %s frame buffer device\n", + info->node, info->fix.id); + + mutex_lock(&kernel_fb_helper_lock); + if (list_empty(&kernel_fb_helper_list)) + register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op); + + list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list); + mutex_unlock(&kernel_fb_helper_lock); + + return 0; +} + /** * drm_fb_helper_initial_config - setup a sane initial connector configuration * @fb_helper: fb_helper device struct @@ -2377,41 +2431,50 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper, */ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel) { - struct drm_device *dev = fb_helper->dev; - struct fb_info *info; int ret; + mutex_lock(&fb_helper->lock); + ret = __drm_fb_helper_initial_config(fb_helper, bpp_sel); + mutex_unlock(&fb_helper->lock); + + return ret; +} +EXPORT_SYMBOL(drm_fb_helper_initial_config); + +static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) +{ + struct drm_device *dev = fb_helper->dev; + unsigned int width, height; + if (!drm_fbdev_emulation) return 0; + WARN_ON(!mutex_is_locked(&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); - if (ret) - return ret; - info = fb_helper->fbdev; - info->var.pixclock = 0; - ret = register_framebuffer(info); - if (ret < 0) - return ret; + if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) { + fb_helper->delayed_hotplug = true; + mutex_unlock(&dev->mode_config.mutex); + return 0; + } - dev_info(dev->dev, "fb%d: %s frame buffer device\n", - info->node, info->fix.id); + DRM_DEBUG_KMS("\n"); - mutex_lock(&kernel_fb_helper_lock); - if (list_empty(&kernel_fb_helper_list)) - register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op); + width = dev->mode_config.max_width; + height = dev->mode_config.max_height; - list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list); - mutex_unlock(&kernel_fb_helper_lock); + if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0) + DRM_DEBUG_KMS("No connectors reported connected with modes\n"); + + drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height); + + mutex_unlock(&dev->mode_config.mutex); + + drm_fb_helper_set_par(fb_helper->fbdev); return 0; } -EXPORT_SYMBOL(drm_fb_helper_initial_config); /** * drm_fb_helper_hotplug_event - respond to a hotplug notification by @@ -2436,35 +2499,13 @@ 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; + int ret; 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); - 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); + ret = __drm_fb_helper_hotplug_event(fb_helper); mutex_unlock(&fb_helper->lock); - drm_fb_helper_set_par(fb_helper->fbdev); - - return 0; - -unlock: - mutex_unlock(&fb_helper->lock); - return err; + return ret; } EXPORT_SYMBOL(drm_fb_helper_hotplug_event);