The Intel CI [1] was not happy with the previous version and I don't know which part it didn't like. So I'll split up the series and feed it piece by piece until I know where the problem is.
Noralf.
[1] https://patchwork.freedesktop.org/series/58597/
Noralf Trønnes (1): drm/fb-helper: Avoid race with DRM userspace
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(-)
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.vetter@ffwll.ch Signed-off-by: Noralf Trønnes noralf@tronnes.org Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch --- 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 2339f0f8f5a8..578428461391 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) { 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); 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); return -EAGAIN; } @@ -3028,6 +3025,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) @@ -3040,12 +3038,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;
Den 25.04.2019 10.31, skrev Noralf Trønnes:
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.vetter@ffwll.ch Signed-off-by: Noralf Trønnes noralf@tronnes.org Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
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 2339f0f8f5a8..578428461391 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) { 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
The Intel CI doesn't like this patch. AFAICT the reason is that the igt@kms_fbcon_fbt@psr-suspend test expects fbcon to work while it has an open fd that is master. This doesn't match the new rule of bailing out if there's a master.
Adding this debug output:
@@ -558,6 +558,17 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) return 0;
mutex_lock(&fb_helper->lock); +if (READ_ONCE(fb_helper->dev->master)) { + int level = default_message_loglevel; + + default_message_loglevel = LOGLEVEL_DEBUG; + printk("\n"); + printk("\n"); + printk("%s\n", __func__); + printk(" THERE IS A MASTER\n"); + dump_stack(); + default_message_loglevel = level; +} ret = restore_fbdev_mode_force(fb_helper);
do_delayed = fb_helper->delayed_hotplug;
Gives these log entries:
<7> [1857.940072] drm_fb_helper_restore_fbdev_mode_unlocked <7> [1857.940074] THERE IS A MASTER <7> [1857.940079] CPU: 4 PID: 8209 Comm: kms_fbcon_fbt Tainted: G U 5.1.0-rc7-CI-Trybot_4252+ #1 <7> [1857.940081] Hardware name: Intel Corporation Ice Lake Client Platform/IceLake U DDR4 SODIMM PD RVP, BIOS ICLSFWR1.R00.3121.A00.1903190527 03/19/2019 <7> [1857.940083] Call Trace: <7> [1857.940091] dump_stack+0x67/0x9b <7> [1857.940099] drm_fb_helper_restore_fbdev_mode_unlocked+0xda/0xf0 <7> [1857.940104] drm_fb_helper_set_par+0x24/0x50 <7> [1857.940188] intel_fbdev_set_par+0x11/0x40 [i915] <7> [1857.940197] fb_set_var+0x17a/0x3f0 <7> [1857.940212] ? __lock_acquire+0x49f/0x1590 <7> [1857.940230] fbcon_blank+0x192/0x2e0 <7> [1857.940235] ? __lock_acquire+0x49f/0x1590 <7> [1857.940254] do_unblank_screen+0xa1/0x170 <7> [1857.940260] vt_ioctl+0x505/0x11d0 <7> [1857.940270] tty_ioctl+0xee/0x940 <7> [1857.940274] ? lockdep_hardirqs_off+0x94/0xd0 <7> [1857.940278] ? __slab_free+0x24e/0x4f0 <7> [1857.940287] ? _raw_spin_unlock_irqrestore+0x39/0x60 <7> [1857.940291] ? kmem_cache_free+0x279/0x2e0 <7> [1857.940299] do_vfs_ioctl+0xa0/0x6e0 <7> [1857.940305] ? do_sys_open+0x13b/0x250 <7> [1857.940311] ? rcu_read_lock_sched_held+0x6f/0x80 <7> [1857.940313] ? kmem_cache_free+0x283/0x2e0 <7> [1857.940321] ksys_ioctl+0x35/0x60 <7> [1857.940328] __x64_sys_ioctl+0x11/0x20 <7> [1857.940332] do_syscall_64+0x55/0x190 <7> [1857.940337] entry_SYSCALL_64_after_hwframe+0x49/0xbe <7> [1857.940341] RIP: 0033:0x7faf8f0825d7 <7> [1857.940345] Code: b3 66 90 48 8b 05 b1 48 2d 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 81 48 2d 00 f7 d8 64 89 01 48 <7> [1857.940347] RSP: 002b:00007ffddf6a94b8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 <7> [1857.940351] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007faf8f0825d7 <7> [1857.940353] RDX: 0000000000000000 RSI: 0000000000004b3a RDI: 000000000000000a <7> [1857.940355] RBP: 0000000000000000 R08: 0000000000000000 R09: 00000000000000ab <7> [1857.940357] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000004b3a <7> [1857.940359] R13: 000000000000000a R14: 0000000000000000 R15: 0000000000000000 <6> [1861.481078] [IGT] kms_fbcon_fbt: exiting, ret=99
Patches: https://patchwork.freedesktop.org/series/59951/#rev8 Failing test: https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_4252/shard-iclb7/igt@kms_fbc...
If I make this change and don't check for master, then it's fine:
@@ -542,7 +558,7 @@ 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); + ret = restore_fbdev_mode_force(fb_helper);
do_delayed = fb_helper->delayed_hotplug; if (do_delayed)
https://patchwork.freedesktop.org/series/59951/#rev9
Should I make this change, or should the test change?
Noralf.
@@ -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);@@ -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);@@ -3028,6 +3025,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)
@@ -3040,12 +3038,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;
Den 04.05.2019 14.34, skrev Noralf Trønnes:
Den 25.04.2019 10.31, skrev Noralf Trønnes:
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.vetter@ffwll.ch Signed-off-by: Noralf Trønnes noralf@tronnes.org Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
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 2339f0f8f5a8..578428461391 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) { 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
The Intel CI doesn't like this patch. AFAICT the reason is that the igt@kms_fbcon_fbt@psr-suspend test expects fbcon to work while it has an open fd that is master. This doesn't match the new rule of bailing out if there's a master.
Adding this debug output:
@@ -558,6 +558,17 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) return 0;
mutex_lock(&fb_helper->lock); +if (READ_ONCE(fb_helper->dev->master)) {
- int level = default_message_loglevel;
- default_message_loglevel = LOGLEVEL_DEBUG;
- printk("\n");
- printk("\n");
- printk("%s\n", __func__);
- printk(" THERE IS A MASTER\n");
- dump_stack();
- default_message_loglevel = level;
+} ret = restore_fbdev_mode_force(fb_helper);
do_delayed = fb_helper->delayed_hotplug;
Gives these log entries:
<7> [1857.940072] drm_fb_helper_restore_fbdev_mode_unlocked <7> [1857.940074] THERE IS A MASTER <7> [1857.940079] CPU: 4 PID: 8209 Comm: kms_fbcon_fbt Tainted: G U 5.1.0-rc7-CI-Trybot_4252+ #1 <7> [1857.940081] Hardware name: Intel Corporation Ice Lake Client Platform/IceLake U DDR4 SODIMM PD RVP, BIOS ICLSFWR1.R00.3121.A00.1903190527 03/19/2019 <7> [1857.940083] Call Trace: <7> [1857.940091] dump_stack+0x67/0x9b <7> [1857.940099] drm_fb_helper_restore_fbdev_mode_unlocked+0xda/0xf0 <7> [1857.940104] drm_fb_helper_set_par+0x24/0x50 <7> [1857.940188] intel_fbdev_set_par+0x11/0x40 [i915] <7> [1857.940197] fb_set_var+0x17a/0x3f0 <7> [1857.940212] ? __lock_acquire+0x49f/0x1590 <7> [1857.940230] fbcon_blank+0x192/0x2e0 <7> [1857.940235] ? __lock_acquire+0x49f/0x1590 <7> [1857.940254] do_unblank_screen+0xa1/0x170 <7> [1857.940260] vt_ioctl+0x505/0x11d0 <7> [1857.940270] tty_ioctl+0xee/0x940 <7> [1857.940274] ? lockdep_hardirqs_off+0x94/0xd0 <7> [1857.940278] ? __slab_free+0x24e/0x4f0 <7> [1857.940287] ? _raw_spin_unlock_irqrestore+0x39/0x60 <7> [1857.940291] ? kmem_cache_free+0x279/0x2e0 <7> [1857.940299] do_vfs_ioctl+0xa0/0x6e0 <7> [1857.940305] ? do_sys_open+0x13b/0x250 <7> [1857.940311] ? rcu_read_lock_sched_held+0x6f/0x80 <7> [1857.940313] ? kmem_cache_free+0x283/0x2e0 <7> [1857.940321] ksys_ioctl+0x35/0x60 <7> [1857.940328] __x64_sys_ioctl+0x11/0x20 <7> [1857.940332] do_syscall_64+0x55/0x190 <7> [1857.940337] entry_SYSCALL_64_after_hwframe+0x49/0xbe <7> [1857.940341] RIP: 0033:0x7faf8f0825d7 <7> [1857.940345] Code: b3 66 90 48 8b 05 b1 48 2d 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 81 48 2d 00 f7 d8 64 89 01 48 <7> [1857.940347] RSP: 002b:00007ffddf6a94b8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 <7> [1857.940351] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007faf8f0825d7 <7> [1857.940353] RDX: 0000000000000000 RSI: 0000000000004b3a RDI: 000000000000000a <7> [1857.940355] RBP: 0000000000000000 R08: 0000000000000000 R09: 00000000000000ab <7> [1857.940357] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000004b3a <7> [1857.940359] R13: 000000000000000a R14: 0000000000000000 R15: 0000000000000000 <6> [1861.481078] [IGT] kms_fbcon_fbt: exiting, ret=99
Patches: https://patchwork.freedesktop.org/series/59951/#rev8 Failing test: https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_4252/shard-iclb7/igt@kms_fbc...
Actually there's two failing tests, this one as well:
* igt at kms_fbcon_fbt@psr: - shard-skl: NOTRUN -> [FAIL][1] [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_4252/shard-skl7/igt@kms_fbco...
If I make this change and don't check for master, then it's fine:
@@ -542,7 +558,7 @@ 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);
ret = restore_fbdev_mode_force(fb_helper);
do_delayed = fb_helper->delayed_hotplug; if (do_delayed)
https://patchwork.freedesktop.org/series/59951/#rev9
Should I make this change, or should the test change?
Noralf.
@@ -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);@@ -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);@@ -3028,6 +3025,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)
@@ -3040,12 +3038,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;
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Sun, May 05, 2019 at 11:16:54AM +0200, Noralf Trønnes wrote:
Den 04.05.2019 14.34, skrev Noralf Trønnes:
Den 25.04.2019 10.31, skrev Noralf Trønnes:
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.vetter@ffwll.ch Signed-off-by: Noralf Trønnes noralf@tronnes.org Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
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 2339f0f8f5a8..578428461391 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) { 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
The Intel CI doesn't like this patch. AFAICT the reason is that the igt@kms_fbcon_fbt@psr-suspend test expects fbcon to work while it has an open fd that is master. This doesn't match the new rule of bailing out if there's a master.
Adding this debug output:
@@ -558,6 +558,17 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) return 0;
mutex_lock(&fb_helper->lock); +if (READ_ONCE(fb_helper->dev->master)) {
- int level = default_message_loglevel;
- default_message_loglevel = LOGLEVEL_DEBUG;
- printk("\n");
- printk("\n");
- printk("%s\n", __func__);
- printk(" THERE IS A MASTER\n");
- dump_stack();
- default_message_loglevel = level;
+} ret = restore_fbdev_mode_force(fb_helper);
do_delayed = fb_helper->delayed_hotplug;
Gives these log entries:
<7> [1857.940072] drm_fb_helper_restore_fbdev_mode_unlocked <7> [1857.940074] THERE IS A MASTER <7> [1857.940079] CPU: 4 PID: 8209 Comm: kms_fbcon_fbt Tainted: G U 5.1.0-rc7-CI-Trybot_4252+ #1 <7> [1857.940081] Hardware name: Intel Corporation Ice Lake Client Platform/IceLake U DDR4 SODIMM PD RVP, BIOS ICLSFWR1.R00.3121.A00.1903190527 03/19/2019 <7> [1857.940083] Call Trace: <7> [1857.940091] dump_stack+0x67/0x9b <7> [1857.940099] drm_fb_helper_restore_fbdev_mode_unlocked+0xda/0xf0 <7> [1857.940104] drm_fb_helper_set_par+0x24/0x50 <7> [1857.940188] intel_fbdev_set_par+0x11/0x40 [i915] <7> [1857.940197] fb_set_var+0x17a/0x3f0 <7> [1857.940212] ? __lock_acquire+0x49f/0x1590 <7> [1857.940230] fbcon_blank+0x192/0x2e0 <7> [1857.940235] ? __lock_acquire+0x49f/0x1590 <7> [1857.940254] do_unblank_screen+0xa1/0x170 <7> [1857.940260] vt_ioctl+0x505/0x11d0 <7> [1857.940270] tty_ioctl+0xee/0x940 <7> [1857.940274] ? lockdep_hardirqs_off+0x94/0xd0 <7> [1857.940278] ? __slab_free+0x24e/0x4f0 <7> [1857.940287] ? _raw_spin_unlock_irqrestore+0x39/0x60 <7> [1857.940291] ? kmem_cache_free+0x279/0x2e0 <7> [1857.940299] do_vfs_ioctl+0xa0/0x6e0 <7> [1857.940305] ? do_sys_open+0x13b/0x250 <7> [1857.940311] ? rcu_read_lock_sched_held+0x6f/0x80 <7> [1857.940313] ? kmem_cache_free+0x283/0x2e0 <7> [1857.940321] ksys_ioctl+0x35/0x60 <7> [1857.940328] __x64_sys_ioctl+0x11/0x20 <7> [1857.940332] do_syscall_64+0x55/0x190 <7> [1857.940337] entry_SYSCALL_64_after_hwframe+0x49/0xbe <7> [1857.940341] RIP: 0033:0x7faf8f0825d7 <7> [1857.940345] Code: b3 66 90 48 8b 05 b1 48 2d 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 81 48 2d 00 f7 d8 64 89 01 48 <7> [1857.940347] RSP: 002b:00007ffddf6a94b8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 <7> [1857.940351] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007faf8f0825d7 <7> [1857.940353] RDX: 0000000000000000 RSI: 0000000000004b3a RDI: 000000000000000a <7> [1857.940355] RBP: 0000000000000000 R08: 0000000000000000 R09: 00000000000000ab <7> [1857.940357] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000004b3a <7> [1857.940359] R13: 000000000000000a R14: 0000000000000000 R15: 0000000000000000 <6> [1861.481078] [IGT] kms_fbcon_fbt: exiting, ret=99
Patches: https://patchwork.freedesktop.org/series/59951/#rev8 Failing test: https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_4252/shard-iclb7/igt@kms_fbc...
Actually there's two failing tests, this one as well:
- igt at kms_fbcon_fbt@psr:
- shard-skl: NOTRUN -> [FAIL][1]
I think the test should change. Our igt fbcon/fbdev tests started as some quick hacks and evolved from there ... -Daniel
If I make this change and don't check for master, then it's fine:
@@ -542,7 +558,7 @@ 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);
ret = restore_fbdev_mode_force(fb_helper);
do_delayed = fb_helper->delayed_hotplug; if (do_delayed)
https://patchwork.freedesktop.org/series/59951/#rev9
Should I make this change, or should the test change?
Noralf.
@@ -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);@@ -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);@@ -3028,6 +3025,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)
@@ -3040,12 +3038,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;
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Apr 25, 2019 at 10:31 AM Noralf Trønnes noralf@tronnes.org wrote:
The Intel CI [1] was not happy with the previous version and I don't know which part it didn't like. So I'll split up the series and feed it piece by piece until I know where the problem is.
You can also send stuff to intel-gfx-trybot@lists.fd.o for test runs that you don't want the world to see (it's still public, but no one looks really). -Daniel
Noralf.
[1] https://patchwork.freedesktop.org/series/58597/
Noralf Trønnes (1): drm/fb-helper: Avoid race with DRM userspace
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(-)
-- 2.20.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Den 25.04.2019 11.25, skrev Daniel Vetter:
On Thu, Apr 25, 2019 at 10:31 AM Noralf Trønnes noralf@tronnes.org wrote:
The Intel CI [1] was not happy with the previous version and I don't know which part it didn't like. So I'll split up the series and feed it piece by piece until I know where the problem is.
You can also send stuff to intel-gfx-trybot@lists.fd.o for test runs that you don't want the world to see (it's still public, but no one looks really).
Thanks, that's a better solution.
Noralf.
-Daniel
Noralf.
[1] https://patchwork.freedesktop.org/series/58597/
Noralf Trønnes (1): drm/fb-helper: Avoid race with DRM userspace
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(-)
-- 2.20.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Noralf Trønnes