On Fri, May 30, 2014 at 01:12:10PM -0400, Rob Clark wrote:
> All drm_fb_helper_restore_fbdev_mode() call sites, save one, do the same
> locking. Simplify this into drm_fb_helper_restore_fbdev_mode_unlocked().
>
> Signed-off-by: Rob Clark <robdclark@xxxxxxxxx>
Presuming I'm not blind the last such user is in the fb helper itself for
the panic handler. Should we drop the EXPORT_SYMBOL for the helper?
If we do that we might as well drop the _unlocked prefix and just remove
locking from drivers ...
-Daniel
> ---
> drivers/gpu/drm/armada/armada_fbdev.c | 4 +--
> drivers/gpu/drm/drm_fb_cma_helper.c | 9 ++----
> drivers/gpu/drm/drm_fb_helper.c | 51 +++++++++++++++++++++++--------
> drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 4 +--
> drivers/gpu/drm/gma500/psb_drv.c | 4 +--
> drivers/gpu/drm/i915/intel_fbdev.c | 6 +---
> drivers/gpu/drm/msm/msm_drv.c | 7 ++---
> drivers/gpu/drm/omapdrm/omap_drv.c | 4 +--
> drivers/gpu/drm/tegra/fb.c | 7 ++---
> include/drm/drm_fb_helper.h | 1 +
> 10 files changed, 50 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/gpu/drm/armada/armada_fbdev.c b/drivers/gpu/drm/armada/armada_fbdev.c
> index 948cb14..fd166f5 100644
> --- a/drivers/gpu/drm/armada/armada_fbdev.c
> +++ b/drivers/gpu/drm/armada/armada_fbdev.c
> @@ -181,10 +181,8 @@ void armada_fbdev_lastclose(struct drm_device *dev)
> {
> struct armada_private *priv = dev->dev_private;
>
> - drm_modeset_lock_all(dev);
> if (priv->fbdev)
> - drm_fb_helper_restore_fbdev_mode(priv->fbdev);
> - drm_modeset_unlock_all(dev);
> + drm_fb_helper_restore_fbdev_mode_unlocked(priv->fbdev);
> }
>
> void armada_fbdev_fini(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> index 61b5a47..f27c883 100644
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -429,13 +429,8 @@ EXPORT_SYMBOL_GPL(drm_fbdev_cma_fini);
> */
> void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma)
> {
> - if (fbdev_cma) {
> - struct drm_device *dev = fbdev_cma->fb_helper.dev;
> -
> - drm_modeset_lock_all(dev);
> - drm_fb_helper_restore_fbdev_mode(&fbdev_cma->fb_helper);
> - drm_modeset_unlock_all(dev);
> - }
> + if (fbdev_cma)
> + drm_fb_helper_restore_fbdev_mode_unlocked(&fbdev_cma->fb_helper);
> }
> EXPORT_SYMBOL_GPL(drm_fbdev_cma_restore_mode);
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index b891f4c..dc861a1 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -273,15 +273,7 @@ int drm_fb_helper_debug_leave(struct fb_info *info)
> }
> EXPORT_SYMBOL(drm_fb_helper_debug_leave);
>
> -/**
> - * drm_fb_helper_restore_fbdev_mode - restore fbdev configuration
> - * @fb_helper: fbcon to restore
> - *
> - * This should be called from driver's drm ->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.
> - */
> -bool drm_fb_helper_restore_fbdev_mode(struct drm_fb_helper *fb_helper)
> +static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper)
> {
> struct drm_device *dev = fb_helper->dev;
> struct drm_plane *plane;
> @@ -311,8 +303,44 @@ bool drm_fb_helper_restore_fbdev_mode(struct drm_fb_helper *fb_helper)
> }
> return error;
> }
> +/**
> + * drm_fb_helper_restore_fbdev_mode - restore fbdev configuration
> + * @fb_helper: fbcon to restore
> + * @lockless: true in drm_fb_helper_force_kernel_mode() path, to avoid
> + * blocking in panic case, everywhere else should use false
> + *
> + * This should be called from driver's drm ->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.
> + *
> + * Use this variant if you need to bypass locking (panic), or already
> + * hold all modeset locks. Otherwise use drm_fb_helper_restore_fbdev_mode_unlocked()
> + */
> +bool drm_fb_helper_restore_fbdev_mode(struct drm_fb_helper *fb_helper)
> +{
> + return restore_fbdev_mode(fb_helper);
> +}
> EXPORT_SYMBOL(drm_fb_helper_restore_fbdev_mode);
>
> +/**
> + * drm_fb_helper_restore_fbdev_mode_unlocked - restore fbdev configuration
> + * @fb_helper: fbcon to restore
> + *
> + * This should be called from driver's drm ->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.
> + */
> +bool drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
> +{
> + struct drm_device *dev = fb_helper->dev;
> + bool ret;
> + drm_modeset_lock_all(dev);
> + ret = restore_fbdev_mode(fb_helper);
> + drm_modeset_unlock_all(dev);
> + return ret;
> +}
> +EXPORT_SYMBOL(drm_fb_helper_restore_fbdev_mode_unlocked);
> +
> /*
> * restore fbcon display for all kms driver's using this helper, used for sysrq
> * and panic handling.
> @@ -824,7 +852,6 @@ EXPORT_SYMBOL(drm_fb_helper_check_var);
> int drm_fb_helper_set_par(struct fb_info *info)
> {
> struct drm_fb_helper *fb_helper = info->par;
> - struct drm_device *dev = fb_helper->dev;
> struct fb_var_screeninfo *var = &info->var;
>
> if (var->pixclock != 0) {
> @@ -832,9 +859,7 @@ int drm_fb_helper_set_par(struct fb_info *info)
> return -EINVAL;
> }
>
> - drm_modeset_lock_all(dev);
> - drm_fb_helper_restore_fbdev_mode(fb_helper);
> - drm_modeset_unlock_all(dev);
> + drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
>
> if (fb_helper->delayed_hotplug) {
> fb_helper->delayed_hotplug = false;
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> index addbf75..93e7ba6 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> @@ -375,7 +375,5 @@ void exynos_drm_fbdev_restore_mode(struct drm_device *dev)
> if (!private || !private->fb_helper)
> return;
>
> - drm_modeset_lock_all(dev);
> - drm_fb_helper_restore_fbdev_mode(private->fb_helper);
> - drm_modeset_unlock_all(dev);
> + drm_fb_helper_restore_fbdev_mode_unlocked(private->fb_helper);
> }
> diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
> index 0a3101a..59ea45e 100644
> --- a/drivers/gpu/drm/gma500/psb_drv.c
> +++ b/drivers/gpu/drm/gma500/psb_drv.c
> @@ -112,11 +112,9 @@ static void psb_driver_lastclose(struct drm_device *dev)
> struct drm_psb_private *dev_priv = dev->dev_private;
> struct psb_fbdev *fbdev = dev_priv->fbdev;
>
> - drm_modeset_lock_all(dev);
> - ret = drm_fb_helper_restore_fbdev_mode(&fbdev->psb_fb_helper);
> + ret = drm_fb_helper_restore_fbdev_mode_unlocked(&fbdev->psb_fb_helper);
> if (ret)
> DRM_DEBUG("failed to restore crtc mode\n");
> - drm_modeset_unlock_all(dev);
>
> return;
> }
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index fce4a0d..f8258ab 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -683,11 +683,7 @@ void intel_fbdev_restore_mode(struct drm_device *dev)
> if (!dev_priv->fbdev)
> return;
>
> - drm_modeset_lock_all(dev);
> -
> - ret = drm_fb_helper_restore_fbdev_mode(&dev_priv->fbdev->helper);
> + ret = drm_fb_helper_restore_fbdev_mode_unlocked(&dev_priv->fbdev->helper);
> if (ret)
> DRM_DEBUG("failed to restore crtc mode\n");
> -
> - drm_modeset_unlock_all(dev);
> }
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 50ec1be..afedd8b 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -382,11 +382,8 @@ static void msm_preclose(struct drm_device *dev, struct drm_file *file)
> static void msm_lastclose(struct drm_device *dev)
> {
> struct msm_drm_private *priv = dev->dev_private;
> - if (priv->fbdev) {
> - drm_modeset_lock_all(dev);
> - drm_fb_helper_restore_fbdev_mode(priv->fbdev);
> - drm_modeset_unlock_all(dev);
> - }
> + if (priv->fbdev)
> + drm_fb_helper_restore_fbdev_mode_unlocked(priv->fbdev);
> }
>
> static irqreturn_t msm_irq(int irq, void *arg)
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index c8270e4..002b972 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -588,9 +588,7 @@ static void dev_lastclose(struct drm_device *dev)
> }
> }
>
> - drm_modeset_lock_all(dev);
> - ret = drm_fb_helper_restore_fbdev_mode(priv->fbdev);
> - drm_modeset_unlock_all(dev);
> + ret = drm_fb_helper_restore_fbdev_mode_unlocked(priv->fbdev);
> if (ret)
> DBG("failed to restore crtc mode");
> }
> diff --git a/drivers/gpu/drm/tegra/fb.c b/drivers/gpu/drm/tegra/fb.c
> index f7fca09..9798a70 100644
> --- a/drivers/gpu/drm/tegra/fb.c
> +++ b/drivers/gpu/drm/tegra/fb.c
> @@ -346,11 +346,8 @@ static void tegra_fbdev_free(struct tegra_fbdev *fbdev)
>
> void tegra_fbdev_restore_mode(struct tegra_fbdev *fbdev)
> {
> - if (fbdev) {
> - drm_modeset_lock_all(fbdev->base.dev);
> - drm_fb_helper_restore_fbdev_mode(&fbdev->base);
> - drm_modeset_unlock_all(fbdev->base.dev);
> - }
> + if (fbdev)
> + drm_fb_helper_restore_fbdev_mode_unlocked(&fbdev->base);
> }
>
> static void tegra_fb_output_poll_changed(struct drm_device *drm)
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 6e622f7..0630b51 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -109,6 +109,7 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> struct fb_info *info);
>
> bool drm_fb_helper_restore_fbdev_mode(struct drm_fb_helper *fb_helper);
> +bool drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper);
> void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helper,
> uint32_t fb_width, uint32_t fb_height);
> void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
> --
> 1.9.3
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
