Re: [PATCH] drm/fb-helper: Fix restore_fbdev when there are pending delayed hotplug

[Jocelyn Falempe <jfalempe@xxxxxxxxxx>]

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);
> >