[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