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

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

On 2/13/2024 23:24, 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 the first switch.

I think your comment from the earlier version that this can still happen 
on simpledrm (albeit less frequently) is very relevant here for the 
commit message.

> 
> v2: Added Kconfig option instead of hard coding "splash".
> v3: Default to disabled, not "splash". If enabled then take over on
>      switch rather than on first output after switch.
> 

These you'll want below the cutlist (---)

Also I think you should mention in the commit message that the 
indication of a userspace splash is set by the Kconfig.

> 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    | 12 +++++++++
>   drivers/video/fbdev/core/fbcon.c | 44 +++++++++++++++++++++++++++++---
>   2 files changed, 52 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/video/console/Kconfig b/drivers/video/console/Kconfig
> index bc31db6ef7..2f9435335f 100644
> --- a/drivers/video/console/Kconfig
> +++ b/drivers/video/console/Kconfig
> @@ -138,6 +138,18 @@ 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 "Command line parameter to defer takeover to first switch"
> +	depends on FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
> +	default ""
> +	help
> +	  If enabled this defers further the framebuffer console taking over
> +	  until the first console switch has occurred. And even then 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.
> +
>   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 1183e7a871..e5d841ab03 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"
> @@ -3348,7 +3349,7 @@ static int fbcon_output_notifier(struct notifier_block *nb,
>   {
>   	WARN_CONSOLE_UNLOCKED();
>   
> -	pr_info("fbcon: Taking over console\n");
> +	pr_info("fbcon: Taking over console for output\n");
>   
>   	dummycon_unregister_output_notifier(&fbcon_output_nb);
>   
> @@ -3357,6 +3358,27 @@ 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) {
> +		pr_info("fbcon: Taking over console for switch\n");
> +		dummycon_unregister_switch_notifier(&fbcon_switch_nb);
> +		schedule_work(&fbcon_deferred_takeover_work);
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +#endif
>   #endif

Once you start adding nested #ifdef, I think it's very useful to add a 
comment on the #endif to make it easier to follow the code.

IE
#endif /* CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER_CONDITION */

>   
>   static void fbcon_start(void)
> @@ -3368,8 +3390,18 @@ static void fbcon_start(void)
>   		deferred_takeover = false;
>   
>   	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
> +		{
> +			fbcon_output_nb.notifier_call = fbcon_output_notifier;
> +			dummycon_register_output_notifier(&fbcon_output_nb);
> +		}
>   		return;
>   	}
>   #endif
> @@ -3416,8 +3448,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);