Commit 03a9606e authored by Noralf Trønnes's avatar Noralf Trønnes
Browse files

drm/fb-helper: Avoid race with DRM userspace 



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.

Two igt tests fail with the new 'bail out if master' rule. Work around
this by relaxing this rule for drm_fb_helper_restore_fbdev_mode_unlocked()
until the tests have been fixed. Add todo entry for this.

Locking rule: Always take the fb-helper lock first.

v5: drm_fb_helper_restore_fbdev_mode_unlocked(): Use
    restore_fbdev_mode_force()

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: default avatarDaniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: default avatarNoralf Trønnes <noralf@tronnes.org>
Reviewed-by: default avatarDaniel Vetter <daniel.vetter@ffwll.ch>
Link: https://patchwork.freedesktop.org/patch/msgid/20190506180139.6913-3-noralf@tronnes.org
parent 094aa54f

Documentation/gpu/todo.rst

+8 −0
Original line number Diff line number Diff line
@@ -281,6 +281,14 @@ it to use drm_mode_hsync() instead. 


Contact: Sean Paul



drm_fb_helper tasks

-------------------



- drm_fb_helper_restore_fbdev_mode_unlocked() should call restore_fbdev_mode()

  not the _force variant so it can bail out if there is a master. But first

  these igt tests need to be fixed: kms_fbcon_fbt@psr and

  kms_fbcon_fbt@psr-suspend.



Core refactorings

=================

drivers/gpu/drm/drm_auth.c

+20 −0
Original line number Diff line number Diff line
@@ -351,3 +351,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);

drivers/gpu/drm/drm_fb_helper.c

+56 −46
Original line number Diff line number Diff line
@@ -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 @@ out: 
	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)

{

	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
@@ -542,7 +558,17 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) 
		return 0;



	mutex_lock(&fb_helper->lock);

	ret = restore_fbdev_mode(fb_helper);

	/*

	 * TODO:

	 * We should bail out here if there is a master by dropping _force.

	 * Currently these igt tests fail if we do that:

	 * - kms_fbcon_fbt@psr

	 * - kms_fbcon_fbt@psr-suspend

	 *

	 * So first these tests need to be fixed so they drop master or don't

	 * have an fd open.

	 */

	ret = restore_fbdev_mode_force(fb_helper);



	do_delayed = fb_helper->delayed_hotplug;

	if (do_delayed)
@@ -556,34 +582,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 +602,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);

		if (ret)

			error = true;

		mutex_unlock(&helper->lock);
@@ -663,20 +661,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 +1509,7 @@ backoff: 
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 +1517,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 +1529,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 +1550,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 +1594,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 +1851,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 +2021,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);

		return -EAGAIN;

	}
@@ -3029,6 +3036,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)
@@ -3041,12 +3049,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);

drivers/gpu/drm/drm_internal.h

+2 −0
Original line number Diff line number Diff line
@@ -93,6 +93,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;

CRA Git | Maintained and supported by SUSTech CRA and CCSE