Hi Am 10.07.20 um 14:38 schrieb Daniel Vetter: > 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. I don't know. I just noticed that kill -9 did not restore the terminal and apparently it was related to these patches. Maybe it did not even work before? Best regards Thomas > -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 >> > > -- 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
Attachment:
signature.asc
Description: OpenPGP digital signature
