On 28.04.22 13:52, Heinrich Schuchardt wrote:
> On 4/25/22 11:23, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kis...@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.kis...@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
