Re: [PATCH v2 2/2] fbcon: Defer console takeover for splash screens to first switch

[Mario Limonciello <mario.limonciello@amd.com>]

On 2/7/2024 03:51, Daniel Vetter wrote:
> On Wed, Feb 07, 2024 at 10:03:10AM +0800, Daniel van Vugt wrote:
>> On 6/2/24 23:41, Mario Limonciello wrote:
>>> On 2/6/2024 08:21, Daniel Vetter wrote:
>>>> On Tue, Feb 06, 2024 at 06:10:51PM +0800, Daniel van Vugt wrote:
>>>>> Until now, deferred console takeover only meant defer until there is
>>>>> output. But that risks stepping on the toes of userspace splash screens,
>>>>> as console messages may appear before the splash screen. So check the
>>>>> command line for the expectation of userspace splash and if present then
>>>>> extend the deferral until after the first switch.
>>>>>
>>>>> V2: Added Kconfig option instead of hard coding "splash".
>>>>>
>>>>> Closes: https://bugs.launchpad.net/bugs/1970069
>>>>> Cc: Mario Limonciello <mario.limonciello@amd.com>
>>>>> Signed-off-by: Daniel van Vugt <daniel.van.vugt@canonical.com>
>>>>> ---
>>>>>    drivers/video/console/Kconfig    | 13 +++++++++++
>>>>>    drivers/video/fbdev/core/fbcon.c | 38 ++++++++++++++++++++++++++++++--
>>>>>    2 files changed, 49 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/video/console/Kconfig b/drivers/video/console/Kconfig
>>>>> index bc31db6ef7..a6e371bfb4 100644
>>>>> --- a/drivers/video/console/Kconfig
>>>>> +++ b/drivers/video/console/Kconfig
>>>>> @@ -138,6 +138,19 @@ config FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
>>>>>          by the firmware in place, rather then replacing the contents with a
>>>>>          black screen as soon as fbcon loads.
>>>>>    +config FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER_CONDITION
>>>>> +    string "Framebuffer Console Deferred Takeover Condition"
>>>>> +    depends on FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
>>>>> +    default "splash"
>>>>> +    help
>>>>> +      If enabled this defers further the framebuffer console taking over
>>>>> +      until the first console switch has occurred. And even then only if
>>>>> +      text has been output, and only if the specified parameter is found
>>>>> +      on the command line. This ensures fbcon does not interrupt userspace
>>>>> +      splash screens such as Plymouth which may be yet to start rendering
>>>>> +      at the time of the first console output. "splash" is the simplest
>>>>> +      distro-agnostic condition for this that Plymouth checks for.
>>>>
>>>> Hm this seems a bit strange since a lot of complexity that no one needs,
>>>> also my impression is that it's rather distro specific how you want this
>>>> to work. So why not just a Kconfig option that lets you choose how much
>>>> you want to delay fbcon setup, with the following options:
>>>>
>>>> - no delay at all
>>>> - dely until first output from the console (which then works for distros
>>>>     which set a log-level to suppress unwanted stuff)
>>>> - delay until first vt-switch. In that case I don't think we also need to
>>>>     delay for first output, since vt switch usually means you'll get first
>>>>     output right away (if it's a vt terminal) or you switch to a different
>>>>     graphical console (which will keep fbcon fully suppressed on the drm
>>>>     side).
>>>>
>>
>> I had similar thoughts, and had prototyped some of this already. But in the end
>> it felt like extra complexity there was no demand for.
> 
> For me this one is a bit too complex, since if you enable the vt switch
> delay you also get the output delay on top. That seems one too much and I
> can't come up with a use-case where you actually want that. So just a
> choice of one or the other or none feels cleaner.
> 
>> If you would like to specify the preferred Kconfig design then I can implement
>> it. Though I don't think there is an enumeration type. It could also be a
>> runtime enumeration (deferred_takeover) controlled by fbcon=something.
> 
> There's a choice in Kconfig, see e.g. kernel/Kconfig.hz for an example.
> 
>>> IIUC there is an "automatic" VT switch that happens with Ubuntu GRUB + Ubuntu
>>> kernels.
>>>
>>> Why?
>>>
>>> Couldn't this also be suppressed by just not doing that?
>>
>> I have not seen any automatic VT switches in debugging but now that you mention
>> it I was probably only debugging on drm-misc-next and not an Ubuntu kernel.
> 
> Hm but I don't see how the output delay would paper over a race (if there
> is one) reliable for this case? If you do vt switch for boot splash or
> login screen and don't yet have drm opened for display or something like
> that, then fbcon can sneak in anyway ...

Ubuntu has had in (at least some) kernels:

https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/unstable/commit/?id=320cfac8ef31

I'm unsure if it's still there today, but maybe it would be best if the 
author (Andy) could enlighten us?  Any idea why that didn't go upstream?

I had thought that tied with a automatic VT switch that was trying to 
hide fbcon as well.

> 
> Cheers, Sima
>>
>> - Daniel
>>
>>>
>>>> I think you could even reuse the existing
>>>> CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER for this, and then just
>>>> compile-time select which kind of notifier to register (well plus the
>>>> check for "splash" on the cmdline for the vt switch one I guess).
>>>>
>>>> Thoughts?
>>>>
>>>> Cheers, Sima
>>>>
>>>>
>>>>> +
>>>>>    config STI_CONSOLE
>>>>>        bool "STI text console"
>>>>>        depends on PARISC && HAS_IOMEM
>>>>> diff --git a/drivers/video/fbdev/core/fbcon.c
>>>>> b/drivers/video/fbdev/core/fbcon.c
>>>>> index 63af6ab034..98155d2256 100644
>>>>> --- a/drivers/video/fbdev/core/fbcon.c
>>>>> +++ b/drivers/video/fbdev/core/fbcon.c
>>>>> @@ -76,6 +76,7 @@
>>>>>    #include <linux/crc32.h> /* For counting font checksums */
>>>>>    #include <linux/uaccess.h>
>>>>>    #include <asm/irq.h>
>>>>> +#include <asm/cmdline.h>
>>>>>      #include "fbcon.h"
>>>>>    #include "fb_internal.h"
>>>>> @@ -3358,6 +3359,26 @@ static int fbcon_output_notifier(struct
>>>>> notifier_block *nb,
>>>>>          return NOTIFY_OK;
>>>>>    }
>>>>> +
>>>>> +#ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER_CONDITION
>>>>> +static int initial_console;
>>>>> +static struct notifier_block fbcon_switch_nb;
>>>>> +
>>>>> +static int fbcon_switch_notifier(struct notifier_block *nb,
>>>>> +                 unsigned long action, void *data)
>>>>> +{
>>>>> +    struct vc_data *vc = data;
>>>>> +
>>>>> +    WARN_CONSOLE_UNLOCKED();
>>>>> +
>>>>> +    if (vc->vc_num != initial_console) {
>>>>> +        dummycon_unregister_switch_notifier(&fbcon_switch_nb);
>>>>> +        dummycon_register_output_notifier(&fbcon_output_nb);
>>>>> +    }
>>>>> +
>>>>> +    return NOTIFY_OK;
>>>>> +}
>>>>> +#endif
>>>>>    #endif
>>>>>      static void fbcon_start(void)
>>>>> @@ -3370,7 +3391,16 @@ static void fbcon_start(void)
>>>>>          if (deferred_takeover) {
>>>>>            fbcon_output_nb.notifier_call = fbcon_output_notifier;
>>>>> -        dummycon_register_output_notifier(&fbcon_output_nb);
>>>>> +#ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER_CONDITION
>>>>> +        if (cmdline_find_option_bool(boot_command_line,
>>>>> +              CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER_CONDITION)) {
>>>>> +            initial_console = fg_console;
>>>>> +            fbcon_switch_nb.notifier_call = fbcon_switch_notifier;
>>>>> +            dummycon_register_switch_notifier(&fbcon_switch_nb);
>>>>> +        } else
>>>>> +#endif
>>>>> +            dummycon_register_output_notifier(&fbcon_output_nb);
>>>>> +
>>>>>            return;
>>>>>        }
>>>>>    #endif
>>>>> @@ -3417,8 +3447,12 @@ void __exit fb_console_exit(void)
>>>>>    {
>>>>>    #ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
>>>>>        console_lock();
>>>>> -    if (deferred_takeover)
>>>>> +    if (deferred_takeover) {
>>>>>            dummycon_unregister_output_notifier(&fbcon_output_nb);
>>>>> +#ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER_CONDITION
>>>>> +        dummycon_unregister_switch_notifier(&fbcon_switch_nb);
>>>>> +#endif
>>>>> +    }
>>>>>        console_unlock();
>>>>>          cancel_work_sync(&fbcon_deferred_takeover_work);
>>>>> -- 
>>>>> 2.43.0
>>>>>
>>>>
>>>
>