[PATCH] efi_loader: Improve console screen clearing and reset

[Jan Kiszka]

On 28.04.22 13:52, Heinrich Schuchardt wrote:
> 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);

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

-- 
Siemens AG, Technology
Competence Center Embedded Linux