Den 01.04.2019 09.12, skrev Daniel Vetter:
On Sat, Mar 30, 2019 at 10:07:58PM +0100, Noralf Trønnes wrote:
Den 28.03.2019 09.17, skrev Daniel Vetter:
On Tue, Mar 26, 2019 at 06:55:43PM +0100, 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.
Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Noralf Trønnes noralf@tronnes.org
I think it'd be good to reorder this earlier in the series. And I'm wondering why you didn't replace all occurences of the _is_bound() function. With the consistent master state check we're doing with this I don't think any of the fbdev checking is still needed. Plus looking at crtc->primary->fb without any locks is racy, so would be good to ditch that hack.
_is_bound() is used in drm_fb_helper_hotplug_event() to decide on delayed hotplug. The master lock won't help here. I have studied drm_fb_helper for 2 years now, and I still think it is convoluted and difficult to grasp in places. So I kept this one call site. I'm vary of making unrelated changes in this series.
Hm, why would it not work for the hotplug case? _is_bound already checks dev->master (but with races, unlike this here). And the other checks are various levels of cargo-culting. dev->master is (well, has become through retrofitting) the "who owns the display" tracking, I think fully standardizing on that makes sense. The only case where there's a difference is if:
- some compositor dropped master
- but didn't disable it's framebuffers
We actually want that for smooth vt switching, so that the new compositor could take over in a transition. Having a special case with fbdev where the compositor has to disable all its fb or fbcon won't take over feels a bit irky.
You're right ofc. I read '->deferred_setup = true' when in fact it is '->delayed_hotplug = true', so I assumed there was something that _is_bound() was protecting wrt. to setup state. I'll fix it in the next version.
Noralf.
-Daniel
The reason I included this patch is because it's used with the bootsplash example support code. I can move the patch earlier in the series.
Noralf.
-Daniel
drivers/gpu/drm/drm_auth.c | 20 ++++++++++++++ drivers/gpu/drm/drm_fb_helper.c | 49 ++++++++++++++++++++++++--------- drivers/gpu/drm/drm_internal.h | 2 ++ 3 files changed, 58 insertions(+), 13 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 4a073cd4e423..9f253fcf3f79 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -41,6 +41,8 @@ #include <drm/drm_crtc_helper.h> #include <drm/drm_atomic.h>
+#include "drm_internal.h"
static bool drm_fbdev_emulation = true; module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600); MODULE_PARM_DESC(fbdev_emulation, @@ -235,7 +237,12 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) return 0;
mutex_lock(&fb_helper->lock);
- ret = drm_client_modesets_commit(fb_helper->dev, fb_helper->modesets);
if (drm_master_internal_acquire(fb_helper->dev)) {
ret = drm_client_modesets_commit(fb_helper->dev, fb_helper->modesets);drm_master_internal_release(fb_helper->dev);} else {
ret = -EBUSY;}
do_delayed = fb_helper->delayed_hotplug; if (do_delayed)
@@ -332,13 +339,16 @@ static struct sysrq_key_op sysrq_drm_fb_helper_restore_op = { }; 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))
drm_client_modesets_dpms(fb_helper->dev, fb_helper->modesets, dpms_mode);
- if (drm_master_internal_acquire(dev)) {
drm_client_modesets_dpms(dev, fb_helper->modesets, dpms_mode);drm_master_internal_release(dev);- } mutex_unlock(&fb_helper->lock);
}
@@ -1097,6 +1107,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)
@@ -1104,9 +1115,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)
@@ -1116,7 +1127,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; @@ -1136,11 +1148,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_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; }
@@ -1177,13 +1191,15 @@ 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;
} EXPORT_SYMBOL(drm_fb_helper_ioctl); @@ -1426,15 +1442,19 @@ 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; @@ -1451,6 +1471,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, int ret = 0; int crtc_count = 0; struct drm_connector_list_iter conn_iter;
- struct drm_device *dev = fb_helper->dev; struct drm_fb_helper_surface_size sizes; struct drm_connector *connector; struct drm_mode_set *mode_set;
@@ -1593,8 +1614,10 @@ 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))drm_client_modesets_commit(fb_helper->dev, fb_helper->modesets);
if (!fb_helper->deferred_setup && drm_master_internal_acquire(dev)) {drm_client_modesets_commit(dev, fb_helper->modesets);drm_master_internal_release(dev);}diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index 251d67e04c2d..6d5d1184c084 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; -- 2.20.1