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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux