Re: [PATCH 8/8] drm: add drm_fb_helper_restore_fbdev_mode_unlocked()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jun 3, 2014 at 9:33 AM, Daniel Vetter <daniel@xxxxxxxx> wrote:
> 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?

only reason I could think of for leaving (and for keeping the
_unlocked suffix) was to catch merge issues.. leaving same name but
with locking removed, it would compile fine and fail at runtime ;-)

I can remove the prototype and EXPORT_SYMBOL I suppose just to be
extra sure we catch everything (because once we start doing things
atomically, it won't work if drivers still wrap it with their own
_lock_all())

BR,
-R

> 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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux