Re: [Intel-gfx] [PATCH v4 06/11] drm/fb-helper: Make top-level lock more robust

[Daniel Vetter]

On Wed, Mar 29, 2017 at 04:43:56PM +0200, Thierry Reding wrote:
> From: Thierry Reding 
> 
> 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 
> ---
>  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 _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(_helper->lock);
> + WARN_ON(!mutex_is_locked(_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(_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 _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(_helper->lock);
> + ret = __drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
> + mutex_unlock(_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(_helper->lock));
> +
> + mutex_lock(>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(>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 

Re: [Intel-gfx] [PATCH v4 06/11] drm/fb-helper: Make top-level lock more robust

[Thierry Reding]

On Wed, Mar 29, 2017 at 04:43:56PM +0200, Thierry Reding wrote:
> From: Thierry Reding 
> 
> 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 
> ---
>  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


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v4 06/11] drm/fb-helper: Make top-level lock more robust

[Thierry Reding]

From: Thierry Reding 

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 
---
 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 _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(_helper->lock);
+   WARN_ON(!mutex_is_locked(_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(_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 _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(_helper->lock);
+   ret = __drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
+   mutex_unlock(_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(_helper->lock));
+
+   mutex_lock(>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(>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(_fb_helper_lock);
+   if (list_empty(_fb_helper_list))
+   register_sysrq_key('v', _drm_fb_helper_restore_op);
+
+   list_add(_helper->kernel_fb_list, _fb_helper_list);
+   mutex_unlock(_fb_helper_lock);
+
+   return 0;
+}
+
 /**
  * drm_fb_helper_initial_config - setup a sane initial