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

[Heinrich Schuchardt]

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?


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 */
> +};
> +

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.

> +		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);
>  }
>