| Message ID | 20211104163245.11070-1-jfalempe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/fb-helper: Fix restore_fbdev when there are pending delayed hotplug | expand |
On 2021-11-04 17:32, Jocelyn Falempe wrote: > When using Xorg/Logind and an external monitor connected with an MST dock. > After disconnecting the external monitor, switching to VT may not work, > the (internal) monitor sill display Xorg, and you can't see what you are > typing in the VT. > > This is related to commit e2809c7db818df6bbd0edf843e1beb2fbc9d8541 which > introduced delayed hotplug for MST, and commit > dc5bdb68b5b369d5bc7d1de96fa64cc1737a6320 which introduced a workaround for > Xorg and logind, and add a force parameter to > __drm_fb_helper_restore_fbdev_mode_unlocked() in this case. > > When switching to VT, with Xorg and logind, if there > are pending hotplug event (like MST unplugged), the hotplug path > may not be fulfilled, because logind may drop the master a bit later. > It leads to the console not showing up on the monitor. > > So in this case, forward the "force" parameter to the hotplug event, > and ignore if there is a drm master in this case. I'm worried that this leaves a race condition which allows the still-master (which causes drm_fb_helper_hotplug_event to bail without this patch) to modify the state set by __drm_fb_helper_hotplug_event, which could still result in similar symptoms. I wonder if something like calling drm_fb_helper_hotplug_event when master is dropped and fb_helper->delayed_hotplug == true could work instead. > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 8e7a124d6c5a..c07080f661b1 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -82,6 +82,8 @@ MODULE_PARM_DESC(drm_leak_fbdev_smem, > static LIST_HEAD(kernel_fb_helper_list); > static DEFINE_MUTEX(kernel_fb_helper_lock); > > +static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper, bool force); > + > /** > * DOC: fbdev helpers > * > @@ -258,7 +260,7 @@ __drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper, > mutex_unlock(&fb_helper->lock); > > if (do_delayed) > - drm_fb_helper_hotplug_event(fb_helper); > + __drm_fb_helper_hotplug_event(fb_helper, force); > > return ret; > } > @@ -1930,6 +1932,38 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel) > } > EXPORT_SYMBOL(drm_fb_helper_initial_config); > > +static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper, bool force) > +{ > + int err = 0; > + > + if (!drm_fbdev_emulation || !fb_helper) > + return 0; > + > + mutex_lock(&fb_helper->lock); > + if (fb_helper->deferred_setup) { > + err = __drm_fb_helper_initial_config_and_unlock(fb_helper, > + fb_helper->preferred_bpp); > + return err; > + } > + if (!force) { > + if (!fb_helper->fb || !drm_master_internal_acquire(fb_helper->dev)) { > + fb_helper->delayed_hotplug = true; > + mutex_unlock(&fb_helper->lock); > + return err; > + } > + drm_master_internal_release(fb_helper->dev); > + } > + drm_dbg_kms(fb_helper->dev, "\n"); > + > + drm_client_modeset_probe(&fb_helper->client, fb_helper->fb->width, fb_helper->fb->height); > + drm_setup_crtcs_fb(fb_helper); > + mutex_unlock(&fb_helper->lock); > + > + drm_fb_helper_set_par(fb_helper->fbdev); > + > + return 0; > +} > + > /** > * drm_fb_helper_hotplug_event - respond to a hotplug notification by > * probing all the outputs attached to the fb > @@ -1953,35 +1987,7 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config); > */ > int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) > { > - int err = 0; > - > - if (!drm_fbdev_emulation || !fb_helper) > - return 0; > - > - mutex_lock(&fb_helper->lock); > - if (fb_helper->deferred_setup) { > - err = __drm_fb_helper_initial_config_and_unlock(fb_helper, > - fb_helper->preferred_bpp); > - return err; > - } > - > - if (!fb_helper->fb || !drm_master_internal_acquire(fb_helper->dev)) { > - fb_helper->delayed_hotplug = true; > - mutex_unlock(&fb_helper->lock); > - return err; > - } > - > - drm_master_internal_release(fb_helper->dev); > - > - drm_dbg_kms(fb_helper->dev, "\n"); > - > - drm_client_modeset_probe(&fb_helper->client, fb_helper->fb->width, fb_helper->fb->height); > - drm_setup_crtcs_fb(fb_helper); > - mutex_unlock(&fb_helper->lock); > - > - drm_fb_helper_set_par(fb_helper->fbdev); > - > - return 0; > + return __drm_fb_helper_hotplug_event(fb_helper, false); > } > EXPORT_SYMBOL(drm_fb_helper_hotplug_event); > >
On 05/11/2021 16:50, Michel Dänzer wrote: > On 2021-11-04 17:32, Jocelyn Falempe wrote: >> When using Xorg/Logind and an external monitor connected with an MST dock. >> After disconnecting the external monitor, switching to VT may not work, >> the (internal) monitor sill display Xorg, and you can't see what you are >> typing in the VT. >> >> This is related to commit e2809c7db818df6bbd0edf843e1beb2fbc9d8541 which >> introduced delayed hotplug for MST, and commit >> dc5bdb68b5b369d5bc7d1de96fa64cc1737a6320 which introduced a workaround for >> Xorg and logind, and add a force parameter to >> __drm_fb_helper_restore_fbdev_mode_unlocked() in this case. >> >> When switching to VT, with Xorg and logind, if there >> are pending hotplug event (like MST unplugged), the hotplug path >> may not be fulfilled, because logind may drop the master a bit later. >> It leads to the console not showing up on the monitor. >> >> So in this case, forward the "force" parameter to the hotplug event, >> and ignore if there is a drm master in this case. > > I'm worried that this leaves a race condition which allows the still-master (which causes drm_fb_helper_hotplug_event to bail without this patch) to modify the state set by __drm_fb_helper_hotplug_event, which could still result in similar symptoms. > > I wonder if something like calling drm_fb_helper_hotplug_event when master is dropped and fb_helper->delayed_hotplug == true could work instead. > Ok, I will try to make a new patch and call drm_fb_helper_hotplug_event() from drm_drop_master() instead. > >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >> index 8e7a124d6c5a..c07080f661b1 100644 >> --- a/drivers/gpu/drm/drm_fb_helper.c >> +++ b/drivers/gpu/drm/drm_fb_helper.c >> @@ -82,6 +82,8 @@ MODULE_PARM_DESC(drm_leak_fbdev_smem, >> static LIST_HEAD(kernel_fb_helper_list); >> static DEFINE_MUTEX(kernel_fb_helper_lock); >> >> +static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper, bool force); >> + >> /** >> * DOC: fbdev helpers >> * >> @@ -258,7 +260,7 @@ __drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper, >> mutex_unlock(&fb_helper->lock); >> >> if (do_delayed) >> - drm_fb_helper_hotplug_event(fb_helper); >> + __drm_fb_helper_hotplug_event(fb_helper, force); >> >> return ret; >> } >> @@ -1930,6 +1932,38 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel) >> } >> EXPORT_SYMBOL(drm_fb_helper_initial_config); >> >> +static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper, bool force) >> +{ >> + int err = 0; >> + >> + if (!drm_fbdev_emulation || !fb_helper) >> + return 0; >> + >> + mutex_lock(&fb_helper->lock); >> + if (fb_helper->deferred_setup) { >> + err = __drm_fb_helper_initial_config_and_unlock(fb_helper, >> + fb_helper->preferred_bpp); >> + return err; >> + } >> + if (!force) { >> + if (!fb_helper->fb || !drm_master_internal_acquire(fb_helper->dev)) { >> + fb_helper->delayed_hotplug = true; >> + mutex_unlock(&fb_helper->lock); >> + return err; >> + } >> + drm_master_internal_release(fb_helper->dev); >> + } >> + drm_dbg_kms(fb_helper->dev, "\n"); >> + >> + drm_client_modeset_probe(&fb_helper->client, fb_helper->fb->width, fb_helper->fb->height); >> + drm_setup_crtcs_fb(fb_helper); >> + mutex_unlock(&fb_helper->lock); >> + >> + drm_fb_helper_set_par(fb_helper->fbdev); >> + >> + return 0; >> +} >> + >> /** >> * drm_fb_helper_hotplug_event - respond to a hotplug notification by >> * probing all the outputs attached to the fb >> @@ -1953,35 +1987,7 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config); >> */ >> int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) >> { >> - int err = 0; >> - >> - if (!drm_fbdev_emulation || !fb_helper) >> - return 0; >> - >> - mutex_lock(&fb_helper->lock); >> - if (fb_helper->deferred_setup) { >> - err = __drm_fb_helper_initial_config_and_unlock(fb_helper, >> - fb_helper->preferred_bpp); >> - return err; >> - } >> - >> - if (!fb_helper->fb || !drm_master_internal_acquire(fb_helper->dev)) { >> - fb_helper->delayed_hotplug = true; >> - mutex_unlock(&fb_helper->lock); >> - return err; >> - } >> - >> - drm_master_internal_release(fb_helper->dev); >> - >> - drm_dbg_kms(fb_helper->dev, "\n"); >> - >> - drm_client_modeset_probe(&fb_helper->client, fb_helper->fb->width, fb_helper->fb->height); >> - drm_setup_crtcs_fb(fb_helper); >> - mutex_unlock(&fb_helper->lock); >> - >> - drm_fb_helper_set_par(fb_helper->fbdev); >> - >> - return 0; >> + return __drm_fb_helper_hotplug_event(fb_helper, false); >> } >> EXPORT_SYMBOL(drm_fb_helper_hotplug_event); >> >> > >
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 8e7a124d6c5a..c07080f661b1 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -82,6 +82,8 @@ MODULE_PARM_DESC(drm_leak_fbdev_smem, static LIST_HEAD(kernel_fb_helper_list); static DEFINE_MUTEX(kernel_fb_helper_lock); +static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper, bool force); + /** * DOC: fbdev helpers * @@ -258,7 +260,7 @@ __drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper, mutex_unlock(&fb_helper->lock); if (do_delayed) - drm_fb_helper_hotplug_event(fb_helper); + __drm_fb_helper_hotplug_event(fb_helper, force); return ret; } @@ -1930,6 +1932,38 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel) } EXPORT_SYMBOL(drm_fb_helper_initial_config); +static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper, bool force) +{ + int err = 0; + + if (!drm_fbdev_emulation || !fb_helper) + return 0; + + mutex_lock(&fb_helper->lock); + if (fb_helper->deferred_setup) { + err = __drm_fb_helper_initial_config_and_unlock(fb_helper, + fb_helper->preferred_bpp); + return err; + } + if (!force) { + if (!fb_helper->fb || !drm_master_internal_acquire(fb_helper->dev)) { + fb_helper->delayed_hotplug = true; + mutex_unlock(&fb_helper->lock); + return err; + } + drm_master_internal_release(fb_helper->dev); + } + drm_dbg_kms(fb_helper->dev, "\n"); + + drm_client_modeset_probe(&fb_helper->client, fb_helper->fb->width, fb_helper->fb->height); + drm_setup_crtcs_fb(fb_helper); + mutex_unlock(&fb_helper->lock); + + drm_fb_helper_set_par(fb_helper->fbdev); + + return 0; +} + /** * drm_fb_helper_hotplug_event - respond to a hotplug notification by * probing all the outputs attached to the fb @@ -1953,35 +1987,7 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config); */ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) { - int err = 0; - - if (!drm_fbdev_emulation || !fb_helper) - return 0; - - mutex_lock(&fb_helper->lock); - if (fb_helper->deferred_setup) { - err = __drm_fb_helper_initial_config_and_unlock(fb_helper, - fb_helper->preferred_bpp); - return err; - } - - if (!fb_helper->fb || !drm_master_internal_acquire(fb_helper->dev)) { - fb_helper->delayed_hotplug = true; - mutex_unlock(&fb_helper->lock); - return err; - } - - drm_master_internal_release(fb_helper->dev); - - drm_dbg_kms(fb_helper->dev, "\n"); - - drm_client_modeset_probe(&fb_helper->client, fb_helper->fb->width, fb_helper->fb->height); - drm_setup_crtcs_fb(fb_helper); - mutex_unlock(&fb_helper->lock); - - drm_fb_helper_set_par(fb_helper->fbdev); - - return 0; + return __drm_fb_helper_hotplug_event(fb_helper, false); } EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
When using Xorg/Logind and an external monitor connected with an MST dock. After disconnecting the external monitor, switching to VT may not work, the (internal) monitor sill display Xorg, and you can't see what you are typing in the VT. This is related to commit e2809c7db818df6bbd0edf843e1beb2fbc9d8541 which introduced delayed hotplug for MST, and commit dc5bdb68b5b369d5bc7d1de96fa64cc1737a6320 which introduced a workaround for Xorg and logind, and add a force parameter to __drm_fb_helper_restore_fbdev_mode_unlocked() in this case. When switching to VT, with Xorg and logind, if there are pending hotplug event (like MST unplugged), the hotplug path may not be fulfilled, because logind may drop the master a bit later. It leads to the console not showing up on the monitor. So in this case, forward the "force" parameter to the hotplug event, and ignore if there is a drm master in this case. Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> --- drivers/gpu/drm/drm_fb_helper.c | 66 ++++++++++++++++++--------------- 1 file changed, 36 insertions(+), 30 deletions(-)