[U-Boot] [PATCH v1 08/12] efi_loader: console support for color attributes

Rob Clark robdclark at gmail.com
Wed Oct 4 20:54:31 UTC 2017


On Wed, Oct 4, 2017 at 2:53 PM, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> On 09/10/2017 03:22 PM, Rob Clark wrote:
>> Shell.efi uses this, and supporting color attributes makes things look
>> nicer.  Map the EFI fg/bg color attributes to ANSI escape sequences.
>> Not all colors have a perfect match, but spec just says "Devices
>> supporting a different number of text colors are required to emulate the
>> above colors to the best of the device’s capabilities".
>>
>> Signed-off-by: Rob Clark <robdclark at gmail.com>
>> ---
>>  include/efi_api.h            | 29 +++++++++++++++++++++++++++++
>>  lib/efi_loader/efi_console.c | 30 ++++++++++++++++++++++++++++++
>>  2 files changed, 59 insertions(+)
>>
>> diff --git a/include/efi_api.h b/include/efi_api.h
>> index 87c8ffe68e..3cc1dbac2e 100644
>> --- a/include/efi_api.h
>> +++ b/include/efi_api.h
>> @@ -426,6 +426,35 @@ struct simple_text_output_mode {
>>       EFI_GUID(0x387477c2, 0x69c7, 0x11d2, \
>>                0x8e, 0x39, 0x0, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
>>
>> +#define EFI_BLACK                0x00
>> +#define EFI_BLUE                 0x01
>> +#define EFI_GREEN                0x02
>> +#define EFI_CYAN                 0x03
>> +#define EFI_RED                  0x04
>> +#define EFI_MAGENTA              0x05
>> +#define EFI_BROWN                0x06
>> +#define EFI_LIGHTGRAY            0x07
>> +#define EFI_BRIGHT               0x08
>> +#define EFI_DARKGRAY             0x08
>> +#define EFI_LIGHTBLUE            0x09
>> +#define EFI_LIGHTGREEN           0x0a
>> +#define EFI_LIGHTCYAN            0x0b
>> +#define EFI_LIGHTRED             0x0c
>> +#define EFI_LIGHTMAGENTA         0x0d
>> +#define EFI_YELLOW               0x0e
>> +#define EFI_WHITE                0x0f
>> +#define EFI_BACKGROUND_BLACK     0x00
>> +#define EFI_BACKGROUND_BLUE      0x10
>> +#define EFI_BACKGROUND_GREEN     0x20
>> +#define EFI_BACKGROUND_CYAN      0x30
>> +#define EFI_BACKGROUND_RED       0x40
>> +#define EFI_BACKGROUND_MAGENTA   0x50
>> +#define EFI_BACKGROUND_BROWN     0x60
>> +#define EFI_BACKGROUND_LIGHTGRAY 0x70
>
> Will we ever use these constants?
>

possibly not, but it is useful to understand what is going on with
efi->ansi mapping, so I would prefer to keep them.

>
> Where are the comments explaining the defines below?
>
>> +
>> +#define EFI_ATTR_FG(attr)        ((attr) & 0x0f)
>
> This saves 8 entries in the table below.
> +#define EFI_ATTR_FG(attr)        ((attr) & 0x07)
>
>> +#define EFI_ATTR_BG(attr)        (((attr) >> 4) & 0x7)
>
> Add
> #define EFI_ATTR_BOLD(attr) (((attr) >> 3) & 0x01)
>
>> +
>>  struct efi_simple_text_output_protocol {
>>       void *reset;
>>       efi_status_t (EFIAPI *output_string)(
>> diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
>> index 2e13fdc096..fcd65ca488 100644
>> --- a/lib/efi_loader/efi_console.c
>> +++ b/lib/efi_loader/efi_console.c
>> @@ -316,12 +316,42 @@ static efi_status_t EFIAPI efi_cout_set_mode(
>>       return EFI_EXIT(EFI_SUCCESS);
>>  }
>>
>> +static const struct {
>> +     unsigned fg;
>> +     unsigned bg;
>> +} color[] = {
>> +     { 30, 40 },     /* 0: black */
>> +     { 34, 44 },     /* 1: blue */
>> +     { 32, 42 },     /* 2: green */
>> +     { 36, 46 },     /* 3: cyan */
>> +     { 31, 41 },     /* 4: red */
>> +     { 35, 45 },     /* 5: magenta */
>> +     { 30, 40 },     /* 6: brown, map to black */
>
> This should be { 33, 43 }
>
>> +     { 37, 47 },     /* 7: light grey, map to white */
>
> The entries below are redundant.
>
>> +     { 37, 47 },     /* 8: bright, map to white */
>> +     { 34, 44 },     /* 9: light blue, map to blue */
>> +     { 32, 42 },     /* A: light green, map to green */
>> +     { 36, 46 },     /* B: light cyan, map to cyan */
>> +     { 31, 41 },     /* C: light red, map to red */
>> +     { 35, 45 },     /* D: light magenta, map to magenta */
>> +     { 33, 43 },     /* E: yellow */
>> +     { 37, 47 },     /* F: white */
>> +};
>> +

I'm not totally convinced about mapping extra colors that UEFI defines
to bold.. unless you have some example of prior-art for this on other
platforms.

There are ansi extensions that allow for more colors, we could perhaps
map the extra EFI colors to the base sequence (presumably more widely
supported) followed by the extended sequence (which presumably not all
terminal emulators support)..  I'm not sure if it is worth the effort.
The current patch implements something that is at least fairly
reasonable and within the bounds of what the spec says (ie. "Devices
supporting a different number of text colors are required to emulate
the above colors to the best of the device’s capabilities.")

BR,
-R

>
> No function without a comment explaining the parameters.
>
>>  static efi_status_t EFIAPI efi_cout_set_attribute(
>>                       struct efi_simple_text_output_protocol *this,
>>                       unsigned long attribute)
>>  {
>> +     unsigned fg = EFI_ATTR_FG(attribute);
>> +     unsigned bg = EFI_ATTR_BG(attribute);
>
> Use unsigned int.
>
>> +
>>       EFI_ENTRY("%p, %lx", this, attribute);
>>
>> +     if (attribute)
>
> Attribute 0 should be black on black. No need for any if here.

yeah, except in practice this results in unreadable black on black
display.. I debugged my way through this the hard way.  The spec
doesn't make it clean, but in practice attribute==0 means to go back
to white on black.

BR,
-R

>> +             printf(ESC"[%u;%um", color[fg].fg, color[bg].bg);
>
> Use bold for light colors.
>
> unsigned int bold;
> if (EFI_ATTRIB_BOLD(attr))
>         bold = 1;
> else
>         bold = 22;
>
>
> printf(ESC"[%u;%u;%um", bold, color[fg].fg, color[bg].bg)
>
> Best regards
>
> Heinrich
>
>> +     else
>> +             printf(ESC"[37;40m");
>
>
>> +
>>       /* Just ignore attributes (colors) for now */
>>       return EFI_EXIT(EFI_UNSUPPORTED);
>>  }
>>
>


More information about the U-Boot mailing list