On Sun, Apr 07, 2019 at 06:52:33PM +0200, Noralf Trønnes wrote:
> drm_fb_helper_is_bound() is used to check if DRM userspace is in control.
> This is done by looking at the fb on the primary plane. By the time
> fb-helper gets around to committing, it's possible that the facts have
> changed.
>
> Avoid this race by holding the drm_device->master_mutex lock while
> committing. When DRM userspace does its first open, it will now wait
> until fb-helper is done. The helper will stay away if there's a master.
>
> Locking rule: Always take the fb-helper lock first.
>
> v2:
> - Remove drm_fb_helper_is_bound() (Daniel Vetter)
> - No need to check fb_helper->dev->master in
> drm_fb_helper_single_fb_probe(), restore_fbdev_mode() has the check.
>
> Suggested-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> Signed-off-by: Noralf Trønnes <nor...@tronnes.org>
> ---
> drivers/gpu/drm/drm_auth.c | 20 ++++++++
> drivers/gpu/drm/drm_fb_helper.c | 90 ++++++++++++++++-----------------
> drivers/gpu/drm/drm_internal.h | 2 +
> 3 files changed, 67 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> index 1669c42c40ed..db199807b7dc 100644
> --- a/drivers/gpu/drm/drm_auth.c
> +++ b/drivers/gpu/drm/drm_auth.c
> @@ -368,3 +368,23 @@ void drm_master_put(struct drm_master **master)
> *master = NULL;
> }
> EXPORT_SYMBOL(drm_master_put);
> +
> +/* Used by drm_client and drm_fb_helper */
> +bool drm_master_internal_acquire(struct drm_device *dev)
> +{
> + mutex_lock(&dev->master_mutex);
> + if (dev->master) {
> + mutex_unlock(&dev->master_mutex);
> + return false;
> + }
> +
> + return true;
> +}
> +EXPORT_SYMBOL(drm_master_internal_acquire);
> +
> +/* Used by drm_client and drm_fb_helper */
> +void drm_master_internal_release(struct drm_device *dev)
> +{
> + mutex_unlock(&dev->master_mutex);
> +}
> +EXPORT_SYMBOL(drm_master_internal_release);
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 84791dd4a90d..a6be09ae899b 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -44,6 +44,7 @@
>
> #include "drm_crtc_internal.h"
> #include "drm_crtc_helper_internal.h"
> +#include "drm_internal.h"
>
> static bool drm_fbdev_emulation = true;
> module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
> @@ -509,7 +510,7 @@ static int restore_fbdev_mode_legacy(struct drm_fb_helper
> *fb_helper)
> return ret;
> }
>
> -static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
> +static int restore_fbdev_mode_force(struct drm_fb_helper *fb_helper)
Bikeshed: usually the function variant that's run with locks already taken
is called _locked or has a __ prefix. _force feels a bit misplaced.
> {
> struct drm_device *dev = fb_helper->dev;
>
> @@ -519,6 +520,21 @@ static int restore_fbdev_mode(struct drm_fb_helper
> *fb_helper)
> return restore_fbdev_mode_legacy(fb_helper);
> }
>
> +static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
> +{
> + struct drm_device *dev = fb_helper->dev;
> + int ret;
> +
> + if (!drm_master_internal_acquire(dev))
> + return -EBUSY;
> +
> + ret = restore_fbdev_mode_force(fb_helper);
> +
> + drm_master_internal_release(dev);
> +
> + return ret;
> +}
> +
> /**
> * drm_fb_helper_restore_fbdev_mode_unlocked - restore fbdev configuration
> * @fb_helper: driver-allocated fbdev helper, can be NULL
> @@ -556,34 +572,6 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct
> drm_fb_helper *fb_helper)
> }
> EXPORT_SYMBOL(drm_fb_helper_restore_fbdev_mode_unlocked);
>
> -static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper)
> -{
> - struct drm_device *dev = fb_helper->dev;
> - struct drm_crtc *crtc;
> - int bound = 0, crtcs_bound = 0;
> -
> - /*
> - * Sometimes user space wants everything disabled, so don't steal the
> - * display if there's a master.
> - */
> - if (READ_ONCE(dev->master))
> - return false;
> -
> - drm_for_each_crtc(crtc, dev) {
> - drm_modeset_lock(&crtc->mutex, NULL);
> - if (crtc->primary->fb)
> - crtcs_bound++;
> - if (crtc->primary->fb == fb_helper->fb)
> - bound++;
> - drm_modeset_unlock(&crtc->mutex);
> - }
> -
> - if (bound < crtcs_bound)
> - return false;
> -
> - return true;
> -}
> -
> #ifdef CONFIG_MAGIC_SYSRQ
> /*
> * restore fbcon display for all kms driver's using this helper, used for
> sysrq
> @@ -604,7 +592,7 @@ static bool drm_fb_helper_force_kernel_mode(void)
> continue;
>
> mutex_lock(&helper->lock);
> - ret = restore_fbdev_mode(helper);
> + ret = restore_fbdev_mode_force(helper);
I'd leave this as-is, because:
a) I'm too lazy to review the locking of our open/close calls to convince
myself that lastclose can't race with the next open
b) it won't hurt
c) leaves the door open to easily make our open/close more concurrent in
the future
> if (ret)
> error = true;
> mutex_unlock(&helper->lock);
> @@ -663,20 +651,22 @@ static void dpms_legacy(struct drm_fb_helper
> *fb_helper, int dpms_mode)
> static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
> {
> struct drm_fb_helper *fb_helper = info->par;
> + struct drm_device *dev = fb_helper->dev;
>
> /*
> * For each CRTC in this fb, turn the connectors on/off.
> */
> mutex_lock(&fb_helper->lock);
> - if (!drm_fb_helper_is_bound(fb_helper)) {
> - mutex_unlock(&fb_helper->lock);
> - return;
> - }
> + if (!drm_master_internal_acquire(dev))
> + goto unlock;
>
> - if (drm_drv_uses_atomic_modeset(fb_helper->dev))
> + if (drm_drv_uses_atomic_modeset(dev))
> restore_fbdev_mode_atomic(fb_helper, dpms_mode ==
> DRM_MODE_DPMS_ON);
> else
> dpms_legacy(fb_helper, dpms_mode);
> +
> + drm_master_internal_release(dev);
> +unlock:
> mutex_unlock(&fb_helper->lock);
> }
>
> @@ -1509,6 +1499,7 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct
> fb_info *info)
> int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
> {
> struct drm_fb_helper *fb_helper = info->par;
> + struct drm_device *dev = fb_helper->dev;
> int ret;
>
> if (oops_in_progress)
> @@ -1516,9 +1507,9 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct
> fb_info *info)
>
> mutex_lock(&fb_helper->lock);
>
> - if (!drm_fb_helper_is_bound(fb_helper)) {
> + if (!drm_master_internal_acquire(dev)) {
> ret = -EBUSY;
> - goto out;
> + goto unlock;
> }
>
> if (info->fix.visual == FB_VISUAL_TRUECOLOR)
> @@ -1528,7 +1519,8 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct
> fb_info *info)
> else
> ret = setcmap_legacy(cmap, info);
>
> -out:
> + drm_master_internal_release(dev);
> +unlock:
> mutex_unlock(&fb_helper->lock);
>
> return ret;
> @@ -1548,12 +1540,13 @@ int drm_fb_helper_ioctl(struct fb_info *info,
> unsigned int cmd,
> unsigned long arg)
> {
> struct drm_fb_helper *fb_helper = info->par;
> + struct drm_device *dev = fb_helper->dev;
> struct drm_mode_set *mode_set;
> struct drm_crtc *crtc;
> int ret = 0;
>
> mutex_lock(&fb_helper->lock);
> - if (!drm_fb_helper_is_bound(fb_helper)) {
> + if (!drm_master_internal_acquire(dev)) {
> ret = -EBUSY;
> goto unlock;
> }
> @@ -1591,11 +1584,12 @@ int drm_fb_helper_ioctl(struct fb_info *info,
> unsigned int cmd,
> }
>
> ret = 0;
> - goto unlock;
> + break;
> default:
> ret = -ENOTTY;
> }
>
> + drm_master_internal_release(dev);
> unlock:
> mutex_unlock(&fb_helper->lock);
> return ret;
> @@ -1847,15 +1841,18 @@ int drm_fb_helper_pan_display(struct
> fb_var_screeninfo *var,
> return -EBUSY;
>
> mutex_lock(&fb_helper->lock);
> - if (!drm_fb_helper_is_bound(fb_helper)) {
> - mutex_unlock(&fb_helper->lock);
> - return -EBUSY;
> + if (!drm_master_internal_acquire(dev)) {
> + ret = -EBUSY;
> + goto unlock;
> }
>
> if (drm_drv_uses_atomic_modeset(dev))
> ret = pan_display_atomic(var, info);
> else
> ret = pan_display_legacy(var, info);
> +
> + drm_master_internal_release(dev);
> +unlock:
> mutex_unlock(&fb_helper->lock);
>
> return ret;
> @@ -2014,7 +2011,7 @@ static int drm_fb_helper_single_fb_probe(struct
> drm_fb_helper *fb_helper,
> DRM_INFO("Cannot find any crtc or sizes\n");
>
> /* First time: disable all crtc's.. */
> - if (!fb_helper->deferred_setup &&
> !READ_ONCE(fb_helper->dev->master))
> + if (!fb_helper->deferred_setup)
> restore_fbdev_mode(fb_helper);
I think we need to return the errno here, since without that the higher
levels won't reprobe correctly. Plus we need to remap -EBUSY to -EAGAIN
(or change the check in __drm_fb_helper_initial_config_and_unlock to also
retry on -EBUSY).
> return -EAGAIN;
> }
> @@ -2842,6 +2839,7 @@ 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 || !fb_helper)
> @@ -2854,12 +2852,14 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper
> *fb_helper)
> return err;
> }
>
> - if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
> + if (!fb_helper->fb || !drm_master_internal_acquire(dev)) {
> fb_helper->delayed_hotplug = true;
> mutex_unlock(&fb_helper->lock);
> return err;
> }
>
> + drm_master_internal_release(dev);
> +
> DRM_DEBUG_KMS("\n");
>
> drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height);
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index d9a483a5fce0..3ee97c9998a2 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -91,6 +91,8 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file_priv);
> int drm_master_open(struct drm_file *file_priv);
> void drm_master_release(struct drm_file *file_priv);
> +bool drm_master_internal_acquire(struct drm_device *dev);
> +void drm_master_internal_release(struct drm_device *dev);
>
> /* drm_sysfs.c */
> extern struct class *drm_class;
With the nits addressed:
Reviewed-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> --
> 2.20.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
