[PATCH v2] drm/fb-helper: Call drm_fb_helper_hotplug_event() when releasing drm master

[Daniel Vetter]

On Mon, Nov 08, 2021 at 04:34:53PM +0100, 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 e2809c7db818 ("drm/fb_helper: move deferred fb
> checking into restore mode (v2)")
> 
> 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 when dropping the drm master, call the delayed hotplug function if
> needed.
> 
> v2: rewrote the patch by calling the hotplug function after dropping master
> 
> Signed-off-by: Jocelyn Falempe <jfalempe at redhat.com>

Lastclose console restore is a very gross hack, and generally doesn't work
well.

The way this is supposed to work is:
- userspace drops drm master (because drm master always wins)
- userspace switches the console back to text mode (which will restore the
  console)

I guess we could also do this from dropmaster once more (like from
lastclose), but that really feels like papering over userspace bugs. And
given what a massive mess this entire area is already, I'm not eager to
add more hacks here.

So ... can't we fix userspace?

Ofc if it's a regression then that's different, but then I think we need a
bit clearer implementation. I'm not clear on why you clear the callback
(plus the locking looks busted).
-Daniel

> ---
>  drivers/gpu/drm/drm_auth.c      | 7 +++++++
>  drivers/gpu/drm/drm_fb_helper.c | 6 +++---
>  include/drm/drm_fb_helper.h     | 4 +++-
>  3 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> index 60a6b21474b1..73acf1994d49 100644
> --- a/drivers/gpu/drm/drm_auth.c
> +++ b/drivers/gpu/drm/drm_auth.c
> @@ -35,6 +35,7 @@
>  #include <drm/drm_file.h>
>  #include <drm/drm_lease.h>
>  #include <drm/drm_print.h>
> +#include <drm/drm_fb_helper.h>
>  
>  #include "drm_internal.h"
>  #include "drm_legacy.h"
> @@ -321,6 +322,12 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
>  	}
>  
>  	drm_drop_master(dev, file_priv);
> +
> +	mutex_unlock(&dev->master_mutex);
> +	if (dev->fb_helper && dev->fb_helper->delayed_hotplug)
> +		dev->fb_helper->delayed_hotplug(dev->fb_helper);
> +	return ret;
> +
>  out_unlock:
>  	mutex_unlock(&dev->master_mutex);
>  	return ret;
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 8e7a124d6c5a..e8d8cc3f47c3 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -252,9 +252,9 @@ __drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper,
>  		ret = drm_client_modeset_commit(&fb_helper->client);
>  	}
>  
> -	do_delayed = fb_helper->delayed_hotplug;
> +	do_delayed = (fb_helper->delayed_hotplug != NULL);
>  	if (do_delayed)
> -		fb_helper->delayed_hotplug = false;
> +		fb_helper->delayed_hotplug = NULL;
>  	mutex_unlock(&fb_helper->lock);
>  
>  	if (do_delayed)
> @@ -1966,7 +1966,7 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
>  	}
>  
>  	if (!fb_helper->fb || !drm_master_internal_acquire(fb_helper->dev)) {
> -		fb_helper->delayed_hotplug = true;
> +		fb_helper->delayed_hotplug = drm_fb_helper_hotplug_event;
>  		mutex_unlock(&fb_helper->lock);
>  		return err;
>  	}
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 3af4624368d8..c2131d1a0e23 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -160,8 +160,10 @@ struct drm_fb_helper {
>  	 * A hotplug was received while fbdev wasn't in control of the DRM
>  	 * device, i.e. another KMS master was active. The output configuration
>  	 * needs to be reprobe when fbdev is in control again.
> +	 * NULL, or a pointer to the hotplug function, so it can be called
> +	 * by the drm drop function directly.
>  	 */
> -	bool delayed_hotplug;
> +	int (*delayed_hotplug)(struct drm_fb_helper *helper);
>  
>  	/**
>  	 * @deferred_setup:
> -- 
> 2.33.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch