Received-SPF: pass (google.com: domain of linux-kernel+bounces-57008-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <88f3ff47-8c4b-424c-bf83-1570882cbb54@amd.com>
Date: Wed, 7 Feb 2024 14:21:05 -0600
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 2/2] fbcon: Defer console takeover for splash screens
 to first switch
To: Daniel van Vugt <daniel.van.vugt@canonical.com>,
 Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Helge Deller
 <deller@gmx.de>, Jani Nikula <jani.nikula@intel.com>,
 Danilo Krummrich <dakr@redhat.com>, linux-fbdev@vger.kernel.org,
 dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
 andy.whitcroft@canonical.com
References: <f037df4a-8a97-4fcd-b196-43f484b63d8d@amd.com>
 <20240206101100.25536-1-daniel.van.vugt@canonical.com>
 <20240206101100.25536-2-daniel.van.vugt@canonical.com>
 <ZcJAVSyH3gRYx3EG@phenom.ffwll.local>
 <e4fc61ae-97f5-4fa1-bfed-1312466a2a22@amd.com>
 <26893900-2d0d-4624-94f1-c4aa88386e9c@canonical.com>
 <ZcNSleQOrBtSn3uM@phenom.ffwll.local>
Content-Language: en-US
From: Mario Limonciello <mario.limonciello@amd.com>
In-Reply-To: <ZcNSleQOrBtSn3uM@phenom.ffwll.local>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?U1huVDcvR0t2V1V2Ty92V0tRaVhmNGVjb2g5bWwzb0VsK0lwOHh4MEJnNzRE?=
 =?utf-8?B?dWpQRVR4Skc3cDdpQzFMMiszcEQ0QzdDVURBWHJjblk1K3gwWlc0RVc3ZWxU?=
 =?utf-8?B?dFVwSkdvMFdEQmlrMzd4eHZjZHhsaERHd0JTbU5WSGV1NzNpNW92b0UrMURl?=
 =?utf-8?B?cTUycXVzUXlJc043L0xpZ3BXMGgvc2JlaWI5OWdZVWxxNjJPdmE0MDc1dE1h?=
 =?utf-8?B?Y0NWTldVOGtSZ2VLOFZpR2s1eWRZalJ5TVVQTUVzRXdWajZuY2xISFphdmhY?=
 =?utf-8?B?alFnNXA0N3RmMzNxbVRuK2ZxWFU2bjVLbUtSbkw5bG1hVW4yRk1UZ3RtdlJT?=
 =?utf-8?B?ajNiR3JXY3IzNkhUYmZkUGJFSkFJUjUxNlU1dGltSlprbDlvSUpwQ3RXdUtZ?=
 =?utf-8?B?V3RUd1dIVkFZQkdGcnlQODI0Wk5ieDBPYXdKT1hNL2RkbzhKRm40ZDhLblJZ?=
 =?utf-8?B?OTI2SDNpVEpPcFkwbkdqMFJ3N1BPMEVYL3ZackRqZkFzSExvdTl1RWlMNVpz?=
 =?utf-8?B?M2lTSkN1TGd2TTRYU1cvNHk5d1RGREJ6ZUU3KzRtbjA2UTJ2MTQydmFYdmRz?=
 =?utf-8?B?eXBHY3RIdFVGMVg3ZDJ0TEFPOWVyakJVTmNZUWxEUEJqMm1CQWRmUVNiSEFJ?=
 =?utf-8?B?U0RQbDg0L1RmT3lxWUttaDM4WG1OZnE4UEdpU0xxQkI1d3FpR3gxM05yMXZE?=
 =?utf-8?B?MjBuaHR6SEhxOFFxT3Zlb3RVQXNibXBWVDNCYUoxbnVLeWp1d0EyeE05VHpG?=
 =?utf-8?B?c0VSMmtaT1I0YSt2c3RsV0h4NXQzNjB5dS9JV0djMmtpS0tuMGI0V2RTMmw0?=
 =?utf-8?B?WVlEWkZNZnZlb1pBVDQvb201ejF2aENuT1g0WXU5cWNyZEMxcWQ3NkZCcW9O?=
 =?utf-8?B?MVBsbFdoY09Hd1ZMa0FUb2doTkFPNWdnd3VUSU5WeDEwVk9mbHBtMUxoeVpC?=
 =?utf-8?B?bGNPVDFuNDlHQ3VhaHNmYmtvTHZYKzFDN0c2cGdhRWxSOHFuWkMyb1FXZUMr?=
 =?utf-8?B?bCtianplWXpmWTkwVHZseW9yYTF3ZW9HRE96VTdOUjNIcEthVno0V1ZYdXNG?=
 =?utf-8?B?RjREMENnalJzQjNDWDA1dTVzQU9ZekREaU5KS2RrUjB1QkhXTENNZmMyaVJW?=
 =?utf-8?B?R2NEMEUzNWhIUExjaTlTU1JEQnJKYk1LZTZ1SFFQMVFkdWMvQ3J2R2R4ZC8x?=
 =?utf-8?B?czJQUmE4N0Ewa3pteitaUTZiRmYrMWI5djgvZkhJNkx4SlFud2VSa25EQngv?=
 =?utf-8?B?aEZIUmhzOUlxbGxrRS90alVvaXljeDVTcGpEcC9HcW00eEhUdDYyZ1V4Smhu?=
 =?utf-8?B?Qk40R0I3bW5nM01NdjU2bmFMMVA2TDExYUZBVWNtaFEySWZtREhaTVUvcTdh?=
 =?utf-8?B?aTBUWHg1V3NCTWVXRGRoZFNGSjI4am8wS0ljaTNCZTZtY09nUFZoUUQvd3Ra?=
 =?utf-8?B?RXl1VlNKU2xxRDdOZ1lSSGNyRVdiU2x0R1FWZksxd3NaQ2hndU90S1NJZWFF?=
 =?utf-8?B?VWlqTDA4TlE1MEVlYWgyeDlBcHhDY2t4M2swRDc0V3lXa0FablRWeFB2Mk13?=
 =?utf-8?B?OUdEYkd3enJFQldIZkxCQmVqR1ZmNXZySDlxNXdRUndzdHJrdEk3STdnTUJH?=
 =?utf-8?B?RUF3S21mRUNtdnNTTEVMajQzZzRJQWNweGZhYVIwVFlTUGhKWFZMOWRFbFpX?=
 =?utf-8?B?T0xjRWN4V2NPN0w4OVl0SFZQNEdia09BYlh1THRBcTNtdG9OM2ZjUXlwMUsw?=
 =?utf-8?B?WU1SMEVlNTJoTW5SemRiam1ZRGlORlB0L0FIbjdNdzk0ck4xNEM0QkFlRi9u?=
 =?utf-8?B?Wmo5d2Q3UjdqNmp5V2NvbHdHck1YYWxWTUFLMm9BU2xtT0lERDQ1QnVzSjlw?=
 =?utf-8?B?QzRaOXVYN2lMNytXWVRUT1EvNUt5cDlSWlh4RnVQZkFOVEJCOHRXZFg1YllP?=
 =?utf-8?B?VXlvdm1nRWN2R2R5SFQvUmh4UzJCZ2ZXMHZWQzBIU3FONFRvRWpGc0dBMGZs?=
 =?utf-8?B?d3lSZEtNKzlOVXBVaWZXRkZQOVhoMUxta3F4M2JMd0RKVVBjdVBhYzN5TTRJ?=
 =?utf-8?B?RXk4SUt5TDlDemlBclZJbkt5TjlxTjNiS1ZjSUNQcHBzMlh1TlJNZjZySmNh?=
 =?utf-8?Q?genVrikjKYSpJM7zGqxioFhv+?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5cb9dba2-9159-45e6-68dc-08dc281a50fc
X-MS-Exchange-CrossTenant-AuthSource: MN0PR12MB6101.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Feb 2024 20:21:07.4435
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: /EiamsAXSx+Y3wPia7/qvGbwrPL3IT5ZEcKjrvaVTxUD6vCijVDm8uiFIDMDMg6iFhd956lCrYMf6DWdswHVeQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR12MB8264

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
>>>>>
>>>>
>>>
>