On Fri, Jul 10, 2020 at 1:31 PM Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxxxxxxx > > Reported-and-Tested-by: shlomo@xxxxxxxxxxxx > > Cc: Michel Dänzer <michel@xxxxxxxxxxx> > > Fixes: 64914da24ea9 ("drm/fbdev-helper: don't force restores") > > Cc: Noralf Trønnes <noralf@xxxxxxxxxxx> > > Cc: Thomas Zimmermann <tzimmermann@xxxxxxx> > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx> > > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > > Cc: Maxime Ripard <mripard@xxxxxxxxxx> > > Cc: David Airlie <airlied@xxxxxxxx> > > Cc: Daniel Vetter <daniel@xxxxxxxx> > > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx > > Cc: <stable@xxxxxxxxxxxxxxx> # v5.7+ > > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx> > > --- > > 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
