Received-SPF: pass (google.com: domain of linux-kernel+bounces-55097-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 6 Feb 2024 15:21:09 +0100
From: Daniel Vetter <daniel@ffwll.ch>
To: Daniel van Vugt <daniel.van.vugt@canonical.com>
Cc: Mario Limonciello <mario.limonciello@amd.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Helge Deller <deller@gmx.de>, Daniel Vetter <daniel@ffwll.ch>,
	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
Subject: Re: [PATCH v2 2/2] fbcon: Defer console takeover for splash screens
 to first switch
Message-ID: <ZcJAVSyH3gRYx3EG@phenom.ffwll.local>
Mail-Followup-To: Daniel van Vugt <daniel.van.vugt@canonical.com>,
	Mario Limonciello <mario.limonciello@amd.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
References: <f037df4a-8a97-4fcd-b196-43f484b63d8d@amd.com>
 <20240206101100.25536-1-daniel.van.vugt@canonical.com>
 <20240206101100.25536-2-daniel.van.vugt@canonical.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240206101100.25536-2-daniel.van.vugt@canonical.com>

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch