Re: [PATCH] drm/fb-helper: Fix vt restore

From: Daniel Vetter <daniel.vetter@ffwll.ch>
To: Thomas Zimmermann <tzimmermann@suse.de>
Cc: "DRI Development" <dri-devel@lists.freedesktop.org>,
	"David Airlie" <airlied@linux.ie>,
	"Intel Graphics Development" <intel-gfx@lists.freedesktop.org>,
	shlomo@fastmail.com, stable <stable@vger.kernel.org>,
	"Daniel Vetter" <daniel.vetter@intel.com>,
	"Michel Dänzer" <michel@daenzer.net>
Subject: Re: [PATCH] drm/fb-helper: Fix vt restore
Date: Fri, 10 Jul 2020 14:38:21 +0200	[thread overview]
Message-ID: <CAKMK7uFi0nkf1fSUZaqff-oskoTKMS0Rh_69_Sd9JQ69NMhOMA@mail.gmail.com> (raw)
In-Reply-To: <ac847c3c-e93c-4a4b-c6ca-2362af7e3aa3@suse.de>

On Fri, Jul 10, 2020 at 1:31 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi Daniel,
>
> this patch might not be enougth. I started Xorg and then did 'kill -9'
> on the Xorg process. Xorg went away, but the console did not come back.

Hm don't you need to reset your terminal to text mode in that case
first? Iirc there's a sysrq. All again assuming not terribly modern
system where that stuff is all done by logind.
-Daniel

>
> Best regards
> Thomas
>
> Am 23.06.20 um 17:54 schrieb Daniel Vetter:
> > In the past we had a pile of hacks to orchestrate access between fbdev
> > emulation and native kms clients. We've tried to streamline this, by
> > always preferring the kms side above fbdev calls when a drm master
> > exists, because drm master controls access to the display resources.
> >
> > Unfortunately this breaks existing userspace, specifically Xorg. When
> > exiting Xorg first restores the console to text mode using the KDSET
> > ioctl on the vt. This does nothing, because a drm master is still
> > around. Then it drops the drm master status, which again does nothing,
> > because logind is keeping additional drm fd open to be able to
> > orchestrate vt switches. In the past this is the point where fbdev was
> > restored, as part of the ->lastclose hook on the drm side.
> >
> > Now to fix this regression we don't want to go back to letting fbdev
> > restore things whenever it feels like, or to the pile of hacks we've
> > had before. Instead try and go with a minimal exception to make the
> > KDSET case work again, and nothing else.
> >
> > This means that if userspace does a KDSET call when switching between
> > graphical compositors, there will be some flickering with fbcon
> > showing up for a bit. But a) that's not a regression and b) userspace
> > can fix it by improving the vt switching dance - logind should have
> > all the information it needs.
> >
> > While pondering all this I'm also wondering wheter we should have a
> > SWITCH_MASTER ioctl to allow race-free master status handover. But
> > that's for another day.
> >
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=208179
> > Cc: shlomo@fastmail.com
> > Reported-and-Tested-by: shlomo@fastmail.com
> > Cc: Michel Dänzer <michel@daenzer.net>
> > Fixes: 64914da24ea9 ("drm/fbdev-helper: don't force restores")
> > Cc: Noralf Trønnes <noralf@tronnes.org>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: <stable@vger.kernel.org> # v5.7+
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/drm_fb_helper.c  | 63 +++++++++++++++++++++++++-------
> >  drivers/video/fbdev/core/fbcon.c |  3 +-
> >  include/uapi/linux/fb.h          |  1 +
> >  3 files changed, 52 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index 170aa7689110..ae69bf8e9bcc 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -227,18 +227,9 @@ int drm_fb_helper_debug_leave(struct fb_info *info)
> >  }
> >  EXPORT_SYMBOL(drm_fb_helper_debug_leave);
> >
> > -/**
> > - * drm_fb_helper_restore_fbdev_mode_unlocked - restore fbdev configuration
> > - * @fb_helper: driver-allocated fbdev helper, can be NULL
> > - *
> > - * This should be called from driver's drm &drm_driver.lastclose callback
> > - * when implementing an fbcon on top of kms using this helper. This ensures that
> > - * the user isn't greeted with a black screen when e.g. X dies.
> > - *
> > - * RETURNS:
> > - * Zero if everything went ok, negative error code otherwise.
> > - */
> > -int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
> > +static int
> > +__drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper,
> > +                                         bool force)
> >  {
> >       bool do_delayed;
> >       int ret;
> > @@ -250,7 +241,16 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
> >               return 0;
> >
> >       mutex_lock(&fb_helper->lock);
> > -     ret = drm_client_modeset_commit(&fb_helper->client);
> > +     if (force) {
> > +             /*
> > +              * Yes this is the _locked version which expects the master lock
> > +              * to be held. But for forced restores we're intentionally
> > +              * racing here, see drm_fb_helper_set_par().
> > +              */
> > +             ret = drm_client_modeset_commit_locked(&fb_helper->client);
> > +     } else {
> > +             ret = drm_client_modeset_commit(&fb_helper->client);
> > +     }
> >
> >       do_delayed = fb_helper->delayed_hotplug;
> >       if (do_delayed)
> > @@ -262,6 +262,22 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
> >
> >       return ret;
> >  }
> > +
> > +/**
> > + * drm_fb_helper_restore_fbdev_mode_unlocked - restore fbdev configuration
> > + * @fb_helper: driver-allocated fbdev helper, can be NULL
> > + *
> > + * This should be called from driver's drm &drm_driver.lastclose callback
> > + * when implementing an fbcon on top of kms using this helper. This ensures that
> > + * the user isn't greeted with a black screen when e.g. X dies.
> > + *
> > + * RETURNS:
> > + * Zero if everything went ok, negative error code otherwise.
> > + */
> > +int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
> > +{
> > +     return __drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper, false);
> > +}
> >  EXPORT_SYMBOL(drm_fb_helper_restore_fbdev_mode_unlocked);
> >
> >  #ifdef CONFIG_MAGIC_SYSRQ
> > @@ -1318,6 +1334,7 @@ int drm_fb_helper_set_par(struct fb_info *info)
> >  {
> >       struct drm_fb_helper *fb_helper = info->par;
> >       struct fb_var_screeninfo *var = &info->var;
> > +     bool force;
> >
> >       if (oops_in_progress)
> >               return -EBUSY;
> > @@ -1327,7 +1344,25 @@ int drm_fb_helper_set_par(struct fb_info *info)
> >               return -EINVAL;
> >       }
> >
> > -     drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
> > +     /*
> > +      * Normally we want to make sure that a kms master takes
> > +      * precedence over fbdev, to avoid fbdev flickering and
> > +      * occasionally stealing the display status. But Xorg first sets
> > +      * the vt back to text mode using the KDSET IOCTL with KD_TEXT,
> > +      * and only after that drops the master status when exiting.
> > +      *
> > +      * In the past this was caught by drm_fb_helper_lastclose(), but
> > +      * on modern systems where logind always keeps a drm fd open to
> > +      * orchestrate the vt switching, this doesn't work.
> > +      *
> > +      * To no break the userspace ABI we have this special case here,
> > +      * which is only used for the above case. Everything else uses
> > +      * the normal commit function, which ensures that we never steal
> > +      * the display from an active drm master.
> > +      */
> > +     force = var->activate & FB_ACTIVATE_KD_TEXT;
> > +
> > +     __drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper, force);
> >
> >       return 0;
> >  }
> > diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> > index 9d28a8e3328f..e2a490c5ae08 100644
> > --- a/drivers/video/fbdev/core/fbcon.c
> > +++ b/drivers/video/fbdev/core/fbcon.c
> > @@ -2402,7 +2402,8 @@ static int fbcon_blank(struct vc_data *vc, int blank, int mode_switch)
> >               ops->graphics = 1;
> >
> >               if (!blank) {
> > -                     var.activate = FB_ACTIVATE_NOW | FB_ACTIVATE_FORCE;
> > +                     var.activate = FB_ACTIVATE_NOW | FB_ACTIVATE_FORCE |
> > +                             FB_ACTIVATE_KD_TEXT;
> >                       fb_set_var(info, &var);
> >                       ops->graphics = 0;
> >                       ops->var = info->var;
> > diff --git a/include/uapi/linux/fb.h b/include/uapi/linux/fb.h
> > index b6aac7ee1f67..4c14e8be7267 100644
> > --- a/include/uapi/linux/fb.h
> > +++ b/include/uapi/linux/fb.h
> > @@ -205,6 +205,7 @@ struct fb_bitfield {
> >  #define FB_ACTIVATE_ALL             64       /* change all VCs on this fb    */
> >  #define FB_ACTIVATE_FORCE     128    /* force apply even when no change*/
> >  #define FB_ACTIVATE_INV_MODE  256       /* invalidate videomode */
> > +#define FB_ACTIVATE_KD_TEXT   512       /* for KDSET vt ioctl */
> >
> >  #define FB_ACCELF_TEXT               1       /* (OBSOLETE) see fb_info.flags and vc_mode */
> >
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
>


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