On 28.04.22 13:52, Heinrich Schuchardt wrote:

On 4/25/22 11:23, Jan Kiszka wrote:

...

From: Jan Kiszka jan.kiszka@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@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);

Not necessarily - we may just inject more lines than needed. But we could also avoid that, true.

...

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

OK, but video is not a streaming output with scroll-back buffer, is it? All we need to ensure is that the geometry used here is that of the largest streaming output - and rather scroll more than needed to avoid overwriting. Other outputs should not notice the difference.

Your solution does not cover the cls command.

Not sure I can follow here yet - which cls are you referring to?

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

How will it fail? Guess I need to try this out.

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

What is the logic right now when there are multiple outputs attached?

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.

We don't do that. We only do that if none was selected so far.

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

On ClearScreen, we switch to 0x07 unconditionally (white on black) so far.

...

/*        * 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.

Indeed, I was just preserving existing behavior (or issues).

Jan