| Message ID | 20190425083135.6751-2-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/fb-helper: Move modesetting code to drm_client | expand |
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_fbcon_fbt@psr-suspend.html 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); > 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 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_fbcon_fbt@psr-suspend.html > 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_fbcon_fbt@psr.html > > 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); >> 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; >> > _______________________________________________ > 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_fbcon_fbt@psr-suspend.html > > > > 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_fbcon_fbt@psr.html 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); > >> 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; > >> > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > >
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;