Received-SPF: pass (google.com: domain of linux-kernel+bounces-50496-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <f037df4a-8a97-4fcd-b196-43f484b63d8d@amd.com>
Date: Fri, 2 Feb 2024 13:46:13 -0600
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 2/2] fbcon: Defer console takeover for splash screens to
 first switch
Content-Language: en-US
To: Daniel van Vugt <daniel.van.vugt@canonical.com>
Cc: Daniel Vetter <daniel@ffwll.ch>, 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
References: <20240202085408.23251-1-daniel.van.vugt@canonical.com>
 <20240202085408.23251-2-daniel.van.vugt@canonical.com>
From: Mario Limonciello <mario.limonciello@amd.com>
In-Reply-To: <20240202085408.23251-2-daniel.van.vugt@canonical.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?REpUdkQzUGpCenBDZjU5dzlnOEQvdjUvNFZEdjdHWGl2UzdQelBpOEh4cWdm?=
 =?utf-8?B?U2tTN0FXUWpVUjMxZEl6QzFpUkY4Y1FNdVFtTUUrUjMzV1lVMnNpNWwyTzEy?=
 =?utf-8?B?SVFKRVdsdXdTdGhrbDAyQTJUNmRDaEdNMUl4QUVoOUl3RmtEVW9LRVg3WkJ3?=
 =?utf-8?B?NmVmSkN2b0crS3AzYjdPMVVtTVMwZzRaZ3FLRkZKRWxPWHErOUVQV1RmYmZB?=
 =?utf-8?B?My9MbXVSdlBuemhDVWJGZUxtZlF6SlRDVFVlUExSWlgrbFI1blN0c0lUcXpD?=
 =?utf-8?B?QzN1aTlJc1gvN3E3RGdhNEV2eW9xbHBRb3QybDRsZHI4RFNxZDVhWHZxRm1I?=
 =?utf-8?B?RUpVTTlyeElDUmROc1FhZFR2QXMwdDAyUm5pb0Y4L0NVNkNxdTRRRU1qYjVj?=
 =?utf-8?B?MXhuUHZFZTJiU29lcFYxdWpqeU10TDdZNFNqT1U5clEwK3hQSDZJdjZVQTA4?=
 =?utf-8?B?UnFHcGtGcXZhTzFtamJJOENLaCt4bUYzV1NyallUOUFZMlZRcyt6dUZUUEhM?=
 =?utf-8?B?ZGg1L3VlaTNWRWs0Yk5DWWdKNzdsT1k5UDNSc1NiMFUwSDlaWGdvRU1MZzJS?=
 =?utf-8?B?TE1Vdng3SHl5K3RYTG1uSDJNUk1FMmV1WGEwVmhxelJvSjBSM09pSWNMbmFD?=
 =?utf-8?B?RG5PU2ZlNWlaVzZJOFlZZlFZbTA3MUt6M2wrKzdaSUt0MmV3cTB4MCtDaEI4?=
 =?utf-8?B?aUkxK0dkUXdReUxhclV3WTNSR0hvYlpXNFpjL245U2pEWk1ROXpZVkhwYUxI?=
 =?utf-8?B?YmsxVlFOWmdBbDVaRFl5eGVJQVpHRWJ1TlNvaXR0SjJXWU82NzBVbmR1UGVj?=
 =?utf-8?B?eXU2eGQxVkt2ZUF1ek5VS0lRSXJOY2YrUmVybTZZRXgvME9FUm1NK1VUblFx?=
 =?utf-8?B?TXV4d3ErcXdTOUtkY2FEcTJVMVdzanVYRytQS3VrK3FNVXFqVHpOWWZxU00y?=
 =?utf-8?B?dUxIRStHcm5mcVNaa3dhb0JaZndNQmRLcnJsWnpyc3o4WFFVdS9ud09tSHlo?=
 =?utf-8?B?VHNJMU4wcGsweDE5Qjd4ZmVHTFU1R295bnZsQW9UcG51NUlVL0ZQSWxGQjU1?=
 =?utf-8?B?VTN2ZTNTZHdVKzRhbS9CanExNy9yRnI2MzZmWDAxVTBOa3EzOGpwbGNlcWYz?=
 =?utf-8?B?K2pGdGdwRjVmNjZ0QWdFNGsyZ1hUQ0dPSmRwWmtFWldocU1XU1lrQzN2V1hE?=
 =?utf-8?B?VUo5bjFwckdiYXZXV2w5RE9EQk9Fa2ZqSjN4cDZmNXNiaXVzM2s3c2tVU3pF?=
 =?utf-8?B?MFRjTGMrV1BGTmR5akdFOG9jZFdydGlOaC95bndCVklvWm9TZk5QdFlobndu?=
 =?utf-8?B?V20zaCtpZ3RvVHJkUUkrSzgzTXNtdTdua01GM3BoZmVlazhQWGJTZGpTcEhp?=
 =?utf-8?B?dnFMQWVwWjZmUUxxRU5PVFdiNFpwK0c0MkNsUEFHaVdWK3VRZXRtK1RlM1hU?=
 =?utf-8?B?akdyOGlVdm1zaFZtK0ZUVW43WDMvL1c1ck9PczdtZ0NiL0VGcjlkUkpBSEVJ?=
 =?utf-8?B?TE9tL2ZzQWxlQWtvUFNqMTRXVzEzeWxaWDhENGxlc2dZTVZTbjE4b2VNUjFT?=
 =?utf-8?B?SDluczJjN0lkY3o2WVhlcWNHa1U0Uk9QbzhjR1dnRFM5MHlEK0Z0VThkcW9F?=
 =?utf-8?B?MEZNQVhrWkJ2Rmp5cFhaM09yMzh1Rjd3MU10ZGtuOVl6cGNHWStvc1dKcmY0?=
 =?utf-8?B?cVFjT09odWFYbVRUbzFwaGh2YllPNFF3ekdFOUd3WDhHYXFTb0x2VkRYL2Z2?=
 =?utf-8?B?eXNYeklDVUdOUHpGZTZuTkpGNEtkV2dWQW82bEhFeFRtVDBkZ3BmVEVCTGFy?=
 =?utf-8?B?RHR2OU1TTFdIVFFDYUZ4ZlFSV1crNlJ5czNHV2xVUzhGM2VhaDRTODVyc0Vs?=
 =?utf-8?B?ZG5JRjFYaE93c0ZwWktuMG5BaXNPMEVqWjAxS1pNUlNwdkVXaDkxZEd4ZnhV?=
 =?utf-8?B?WFY0TTFBeUt6VjViMzhQUVlLaHVpYW02T1Njc2ROUTVYSDgzbHVqakQ3emUy?=
 =?utf-8?B?b2wrNmF3ZXVOdVBXcTFJMzhKc3BCZVdrS0hRYWxEUzJDWHZaMG9qSExGQUZR?=
 =?utf-8?B?VWxBY0tvL0owR2lyM0JJNTdRcjBScC95SDdtZGRRNUtuNytTVUdPWk5FTS9q?=
 =?utf-8?Q?7LRmzgj3VfZsZqDRCHUkSfIZu?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ea08c657-b8a5-44f4-e176-08dc24279e6d
X-MS-Exchange-CrossTenant-AuthSource: MN0PR12MB6101.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Feb 2024 19:46:16.1741
 (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: mBd4mXo7HPSm0ZL+6xnRpdJCf+sngAgLP/iQ2xsmSXJCI8uKdNPRFwbGRTffBVLAe+dVz2iLd6tCoDdmEFGybA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN0PR12MB5834

On 2/2/2024 02:53, 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 for the
> "splash" parameter (as used by Plymouth) and if present then extend the
> deferral until the first switch.
> 
> 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/fbdev/core/fbcon.c | 32 +++++++++++++++++++++++++++++---
>   1 file changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index 63af6ab034..5b9f7635f7 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"
> @@ -146,6 +147,7 @@ static inline void fbcon_map_override(void)
>   
>   #ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
>   static bool deferred_takeover = true;
> +static int initial_console = -1;
>   #else
>   #define deferred_takeover false
>   #endif
> @@ -3341,7 +3343,7 @@ static void fbcon_register_existing_fbs(struct work_struct *work)
>   	console_unlock();
>   }
>   
> -static struct notifier_block fbcon_output_nb;
> +static struct notifier_block fbcon_output_nb, fbcon_switch_nb;
>   static DECLARE_WORK(fbcon_deferred_takeover_work, fbcon_register_existing_fbs);
>   
>   static int fbcon_output_notifier(struct notifier_block *nb,
> @@ -3358,6 +3360,21 @@ static int fbcon_output_notifier(struct notifier_block *nb,
>   
>   	return NOTIFY_OK;
>   }
> +
> +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
>   
>   static void fbcon_start(void)
> @@ -3370,7 +3387,14 @@ static void fbcon_start(void)
>   
>   	if (deferred_takeover) {
>   		fbcon_output_nb.notifier_call = fbcon_output_notifier;
> -		dummycon_register_output_notifier(&fbcon_output_nb);
> +		fbcon_switch_nb.notifier_call = fbcon_switch_notifier;
> +		initial_console = fg_console;
> +
> +		if (cmdline_find_option_bool(boot_command_line, "splash"))
> +			dummycon_register_switch_notifier(&fbcon_switch_nb);

So there is a problem here that this would only apply if the distro 
happened to use "splash" which some distros use something different.

I looked at the matching plymouth code [1] and they have a bunch of 
variations of what they accept and what it does.

[1] 
https://gitlab.freedesktop.org/plymouth/plymouth/-/blob/main/src/main.c?ref_type=heads#L888

If from the kernel side we're going to have code that caters to the 
userspace behavior of plymouth we probably need to match all those cases 
they do too.

Another alternative could be to make it a kernel configuration option 
for which string to look for to activate this behavior.

> +		else
> +			dummycon_register_output_notifier(&fbcon_output_nb);
> +
>   		return;
>   	}
>   #endif
> @@ -3417,8 +3441,10 @@ 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);
> +		dummycon_unregister_switch_notifier(&fbcon_switch_nb);
> +	}
>   	console_unlock();
>   
>   	cancel_work_sync(&fbcon_deferred_takeover_work);