[PATCH] efi_loader: Improve console screen clearing and reset

[Heinrich Schuchardt]

On 4/25/22 11:23, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka at siemens.com>
>
> Ensure that the default colors are set when clearing the screen so that
> the background is properly filled and succeeding colored outputs will
> have no differently colored trail.
>
> Before clearing, ensure that no previous output of firmware or UEFI
> programs will overwritten on serial devices or other streaming consoles.
> This helps generating complete boot logs.
>
> Signed-off-by: Jan Kiszka <jan.kiszka at siemens.com>
> ---
>   lib/efi_loader/efi_console.c | 16 +++++++++++++---
>   1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
> index ba68a150172..0270b20bafe 100644
> --- a/lib/efi_loader/efi_console.c
> +++ b/lib/efi_loader/efi_console.c
> @@ -463,8 +463,18 @@ static efi_status_t EFIAPI efi_cout_set_attribute(
>   static efi_status_t EFIAPI efi_cout_clear_screen(
>   			struct efi_simple_text_output_protocol *this)
>   {
> +	unsigned int row;
> +
>   	EFI_ENTRY("%p", this);
>
> +	/* Avoid overwriting previous outputs on streaming consoles */

Thank you for addressing the inconvenience of the partially deleted
scrollback buffer.

Don't assume that the cursor is in the last line. You first have to move
it there:

     printf(ESC "%d;1H", efi_cout_modes[efi_con_mode.mode].rows);

> +	for (row = 1; row < efi_cout_modes[efi_con_mode.mode].rows; row++)
> +		printf("\n");

We may have both video output and serial output. The proposed logic may
not work out correctly if the video screen size differs from the screen
size of the serial console.

Your solution does not cover the cls command.

It will not work with the proposed UEFI boot menu
(https://patchwork.ozlabs.org/project/uboot/list/?series=297320).

A complete solution would have to reside in _serial_puts(). But I doubt
that we want to enlarge the code size there.

Therefore I am a bit skeptical about the suggested change.

> +
> +	/* Set default colors if not done yet */
> +	if (efi_con_mode.attribute == 0)
> +		efi_cout_set_attribute(this, 0x07);

The UEFI specification has this sentence: "The ClearScreen() function
clears the output device(s) display to the currently selected background
color."

So we should not switch colors here.

The currently selected background when starting the first UEFI
application depends on CONFIG_SYS_WHITE_ON_BLACK.

> +
>   	/*
>   	 * The Linux console wants both a clear and a home command. The video
>   	 * uclass does not support <ESC>[H without coordinates, yet.
> @@ -522,11 +532,11 @@ static efi_status_t EFIAPI efi_cout_reset(
>   {
>   	EFI_ENTRY("%p, %d", this, extended_verification);
>
> +	/* Trigger reset to default colors */
> +	efi_con_mode.attribute = 0;
> +
>   	/* Clear screen */
>   	EFI_CALL(efi_cout_clear_screen(this));
> -	/* Set default colors */
> -	efi_con_mode.attribute = 0x07;

This value should depend on CONFIG_SYS_WHITE_ON_BLACK:

Also efi_cout_set_attribute(efi_con_out, 0) should consider
CONFIG_SYS_WHITE_ON_BLACK.

The best thing to do is fixing efi_cout_set_attribute() and invoking

     EFI_CALL(efi_cout_set_attributed(efi_con_out, 0));

But this is a subject for a separate patch.

Best regards

Heinrich

> -	printf(ESC "[0;37;40m");
>
>   	return EFI_EXIT(EFI_SUCCESS);
>   }