| Message ID | 20170910132236.14318-9-robdclark@gmail.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | Alexander Graf |
| Headers | show |
| Series | efi_loader+video: support for Shell.efi | expand |
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@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); > } >
On Wed, Oct 4, 2017 at 2:53 PM, Heinrich Schuchardt <xypron.glpk@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@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); >> } >> >
On 10/04/2017 10:54 PM, Rob Clark wrote: > On Wed, Oct 4, 2017 at 2:53 PM, Heinrich Schuchardt <xypron.glpk@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@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. See Standard ECMA-48 - Control Functions for Coded Character Sets chapter 8.3.117 SGR - SELECT GRAPHIC RENDITION 1 - bold or increased intensity 22 - normal colour or normal intensity (neither bold nor faint) You can easily experiment in your bash shell like this: printf "\x1b[1;32;40m bold \x1b[22;32;40m normal\x1b[22;39;49m\n"; You will find that "bold" prints bold and bright in the KDE konsole and xterm. Using colors 90-97 as foreground colors produces only bright but not bold in the KDE konsole and xterm: printf "\x1b[92;40m bold \x1b[32;40m normal\x1b[22;39;49m\n"; But these codes are not defined in ECMA-48. Best regards Heinrich
On Wed, Oct 4, 2017 at 6:01 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > On 10/04/2017 10:54 PM, Rob Clark wrote: >> On Wed, Oct 4, 2017 at 2:53 PM, Heinrich Schuchardt <xypron.glpk@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@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. > > See > Standard ECMA-48 - Control Functions for Coded Character Sets > chapter 8.3.117 SGR - SELECT GRAPHIC RENDITION > > 1 - bold or increased intensity > 22 - normal colour or normal intensity (neither bold nor faint) > > You can easily experiment in your bash shell like this: > > printf "\x1b[1;32;40m bold \x1b[22;32;40m normal\x1b[22;39;49m\n"; > > You will find that "bold" prints bold and bright in the KDE konsole and > xterm. but I think we don't want (potential) font changes, just color changes.. if you can find the code in edk2 that does this, I guess it would be a reasonable precedent to follow.. but if not I wanted to avoid things that might be specific to particular terminal emulators, since I wasn't really looking forward to testing them all. Otherwise I'd just rely on the extension that allowed 256 colors.. BR, -R > Using colors 90-97 as foreground colors produces only bright but not > bold in the KDE konsole and xterm: > > printf "\x1b[92;40m bold \x1b[32;40m normal\x1b[22;39;49m\n"; > > But these codes are not defined in ECMA-48. > > Best regards > > Heinrich >
On 10/05/2017 01:19 AM, Rob Clark wrote: > On Wed, Oct 4, 2017 at 6:01 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> On 10/04/2017 10:54 PM, Rob Clark wrote: >>> On Wed, Oct 4, 2017 at 2:53 PM, Heinrich Schuchardt <xypron.glpk@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@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. >> >> See >> Standard ECMA-48 - Control Functions for Coded Character Sets >> chapter 8.3.117 SGR - SELECT GRAPHIC RENDITION >> >> 1 - bold or increased intensity >> 22 - normal colour or normal intensity (neither bold nor faint) >> >> You can easily experiment in your bash shell like this: >> >> printf "\x1b[1;32;40m bold \x1b[22;32;40m normal\x1b[22;39;49m\n"; >> >> You will find that "bold" prints bold and bright in the KDE konsole and >> xterm. > > but I think we don't want (potential) font changes, just color changes.. > > if you can find the code in edk2 that does this, I guess it would be a > reasonable precedent to follow.. but if not I wanted to avoid things > that might be specific to particular terminal emulators, since I > wasn't really looking forward to testing them all. Otherwise I'd just > rely on the extension that allowed 256 colors.. > > BR, > -R The same problem seems has led the EDK folks to a similar solution. See MdeModulePkg/Universal/Console/TerminalDxe/TerminalConOut.c Everything starts with this array: { ESC, '[', '0', 'm', ESC, '[', '4', '0', 'm', ESC, '[', '4', '0', 'm', 0 }; The first '0' is replaced by either 0 or 1 depending on brightness. mSetAttributeString[BRIGHT_CONTROL_OFFSET] = (CHAR16) ('0' + BrightControl); The first '4', '0' is replaced by the foreground color. The second '4', '0' is replaced by the background color. ECMA 48 says: 0 - default rendition, cancels the effect of any preceding SGR So you can use this instead of 22. Best regards Heinrich > >> Using colors 90-97 as foreground colors produces only bright but not >> bold in the KDE konsole and xterm: >> >> printf "\x1b[92;40m bold \x1b[32;40m normal\x1b[22;39;49m\n"; >> >> But these codes are not defined in ECMA-48. >> >> Best regards >> >> Heinrich >> >
On Wed, Oct 4, 2017 at 7:53 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > On 10/05/2017 01:19 AM, Rob Clark wrote: >> On Wed, Oct 4, 2017 at 6:01 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >>> On 10/04/2017 10:54 PM, Rob Clark wrote: >>>> On Wed, Oct 4, 2017 at 2:53 PM, Heinrich Schuchardt <xypron.glpk@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@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. >>> >>> See >>> Standard ECMA-48 - Control Functions for Coded Character Sets >>> chapter 8.3.117 SGR - SELECT GRAPHIC RENDITION >>> >>> 1 - bold or increased intensity >>> 22 - normal colour or normal intensity (neither bold nor faint) >>> >>> You can easily experiment in your bash shell like this: >>> >>> printf "\x1b[1;32;40m bold \x1b[22;32;40m normal\x1b[22;39;49m\n"; >>> >>> You will find that "bold" prints bold and bright in the KDE konsole and >>> xterm. >> >> but I think we don't want (potential) font changes, just color changes.. >> >> if you can find the code in edk2 that does this, I guess it would be a >> reasonable precedent to follow.. but if not I wanted to avoid things >> that might be specific to particular terminal emulators, since I >> wasn't really looking forward to testing them all. Otherwise I'd just >> rely on the extension that allowed 256 colors.. >> >> BR, >> -R > > The same problem seems has led the EDK folks to a similar solution. > > See > MdeModulePkg/Universal/Console/TerminalDxe/TerminalConOut.c ok, I'll have a closer look at that.. I don't feel badly about doing the same thing that edk2 does when there is doubt ;-) BR, -R > Everything starts with this array: > > { ESC, '[', '0', 'm', ESC, '[', '4', '0', 'm', ESC, '[', '4', '0', 'm', 0 }; > > The first '0' is replaced by either 0 or 1 depending on brightness. > > mSetAttributeString[BRIGHT_CONTROL_OFFSET] = > (CHAR16) ('0' + BrightControl); > > The first '4', '0' is replaced by the foreground color. > The second '4', '0' is replaced by the background color. > > ECMA 48 says: > > 0 - default rendition, cancels the effect of any preceding SGR > > So you can use this instead of 22. > > Best regards > > Heinrich > > >> >>> Using colors 90-97 as foreground colors produces only bright but not >>> bold in the KDE konsole and xterm: >>> >>> printf "\x1b[92;40m bold \x1b[32;40m normal\x1b[22;39;49m\n"; >>> >>> But these codes are not defined in ECMA-48. >>> >>> Best regards >>> >>> Heinrich >>> >> >
On Wed, Oct 4, 2017 at 8:00 PM, Rob Clark <robdclark@gmail.com> wrote: > On Wed, Oct 4, 2017 at 7:53 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> On 10/05/2017 01:19 AM, Rob Clark wrote: >>> On Wed, Oct 4, 2017 at 6:01 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >>>> On 10/04/2017 10:54 PM, Rob Clark wrote: >>>>> On Wed, Oct 4, 2017 at 2:53 PM, Heinrich Schuchardt <xypron.glpk@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@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. >>>> >>>> See >>>> Standard ECMA-48 - Control Functions for Coded Character Sets >>>> chapter 8.3.117 SGR - SELECT GRAPHIC RENDITION >>>> >>>> 1 - bold or increased intensity >>>> 22 - normal colour or normal intensity (neither bold nor faint) >>>> >>>> You can easily experiment in your bash shell like this: >>>> >>>> printf "\x1b[1;32;40m bold \x1b[22;32;40m normal\x1b[22;39;49m\n"; >>>> >>>> You will find that "bold" prints bold and bright in the KDE konsole and >>>> xterm. >>> >>> but I think we don't want (potential) font changes, just color changes.. >>> >>> if you can find the code in edk2 that does this, I guess it would be a >>> reasonable precedent to follow.. but if not I wanted to avoid things >>> that might be specific to particular terminal emulators, since I >>> wasn't really looking forward to testing them all. Otherwise I'd just >>> rely on the extension that allowed 256 colors.. >>> >>> BR, >>> -R >> >> The same problem seems has led the EDK folks to a similar solution. >> >> See >> MdeModulePkg/Universal/Console/TerminalDxe/TerminalConOut.c > > ok, I'll have a closer look at that.. I don't feel badly about doing > the same thing that edk2 does when there is doubt ;-) hmm, the semi-annoying thing will be to have to implement the vidconsole-uclass side of this.. I suppose I could ignore anything other than 0 (normal) and 1 (bold). Reverse wouldn't be too hard, but blink would be hard I think without timer interrupts (same reason that I didn't implement cursor yet) > BR, > -R > > >> Everything starts with this array: >> >> { ESC, '[', '0', 'm', ESC, '[', '4', '0', 'm', ESC, '[', '4', '0', 'm', 0 }; >> >> The first '0' is replaced by either 0 or 1 depending on brightness. >> >> mSetAttributeString[BRIGHT_CONTROL_OFFSET] = >> (CHAR16) ('0' + BrightControl); >> >> The first '4', '0' is replaced by the foreground color. >> The second '4', '0' is replaced by the background color. >> >> ECMA 48 says: >> >> 0 - default rendition, cancels the effect of any preceding SGR >> >> So you can use this instead of 22. >> >> Best regards >> >> Heinrich >> >> >>> >>>> Using colors 90-97 as foreground colors produces only bright but not >>>> bold in the KDE konsole and xterm: >>>> >>>> printf "\x1b[92;40m bold \x1b[32;40m normal\x1b[22;39;49m\n"; >>>> >>>> But these codes are not defined in ECMA-48. >>>> >>>> Best regards >>>> >>>> Heinrich >>>> >>> >>
On 10/05/2017 02:12 AM, Rob Clark wrote: > On Wed, Oct 4, 2017 at 8:00 PM, Rob Clark <robdclark@gmail.com> wrote: >> On Wed, Oct 4, 2017 at 7:53 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >>> On 10/05/2017 01:19 AM, Rob Clark wrote: >>>> On Wed, Oct 4, 2017 at 6:01 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >>>>> On 10/04/2017 10:54 PM, Rob Clark wrote: >>>>>> On Wed, Oct 4, 2017 at 2:53 PM, Heinrich Schuchardt <xypron.glpk@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@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. >>>>> >>>>> See >>>>> Standard ECMA-48 - Control Functions for Coded Character Sets >>>>> chapter 8.3.117 SGR - SELECT GRAPHIC RENDITION >>>>> >>>>> 1 - bold or increased intensity >>>>> 22 - normal colour or normal intensity (neither bold nor faint) >>>>> >>>>> You can easily experiment in your bash shell like this: >>>>> >>>>> printf "\x1b[1;32;40m bold \x1b[22;32;40m normal\x1b[22;39;49m\n"; >>>>> >>>>> You will find that "bold" prints bold and bright in the KDE konsole and >>>>> xterm. >>>> >>>> but I think we don't want (potential) font changes, just color changes.. >>>> >>>> if you can find the code in edk2 that does this, I guess it would be a >>>> reasonable precedent to follow.. but if not I wanted to avoid things >>>> that might be specific to particular terminal emulators, since I >>>> wasn't really looking forward to testing them all. Otherwise I'd just >>>> rely on the extension that allowed 256 colors.. >>>> >>>> BR, >>>> -R >>> >>> The same problem seems has led the EDK folks to a similar solution. >>> >>> See >>> MdeModulePkg/Universal/Console/TerminalDxe/TerminalConOut.c >> >> ok, I'll have a closer look at that.. I don't feel badly about doing >> the same thing that edk2 does when there is doubt ;-) > > hmm, the semi-annoying thing will be to have to implement the > vidconsole-uclass side of this.. I suppose I could ignore anything > other than 0 (normal) and 1 (bold). Reverse wouldn't be too hard, but > blink would be hard I think without timer interrupts (same reason that > I didn't implement cursor yet) It is not necessary that a cursor is blinking. The KDE konsole only has a filled block (something like Unicode U+2588). For the colors you can use a RGB lookup table. EDK uses these colors: EFI_GRAPHICS_OUTPUT_BLT_PIXEL mGraphicsEfiColors[16] = { // // B G R reserved // {0x00, 0x00, 0x00, 0x00}, // BLACK {0x98, 0x00, 0x00, 0x00}, // LIGHTBLUE {0x00, 0x98, 0x00, 0x00}, // LIGHGREEN {0x98, 0x98, 0x00, 0x00}, // LIGHCYAN {0x00, 0x00, 0x98, 0x00}, // LIGHRED {0x98, 0x00, 0x98, 0x00}, // MAGENTA {0x00, 0x98, 0x98, 0x00}, // BROWN {0x98, 0x98, 0x98, 0x00}, // LIGHTGRAY {0x30, 0x30, 0x30, 0x00}, // DARKGRAY - BRIGHT BLACK {0xff, 0x00, 0x00, 0x00}, // BLUE {0x00, 0xff, 0x00, 0x00}, // LIME {0xff, 0xff, 0x00, 0x00}, // CYAN {0x00, 0x00, 0xff, 0x00}, // RED {0xff, 0x00, 0xff, 0x00}, // FUCHSIA {0x00, 0xff, 0xff, 0x00}, // YELLOW {0xff, 0xff, 0xff, 0x00} // WHITE }; Oracle prefers darker colors and brighter grays: https://docs.oracle.com/cd/E19728-01/820-2550/term_em_colormaps.html Best regards Heinrich > >> BR, >> -R >> >> >>> Everything starts with this array: >>> >>> { ESC, '[', '0', 'm', ESC, '[', '4', '0', 'm', ESC, '[', '4', '0', 'm', 0 }; >>> >>> The first '0' is replaced by either 0 or 1 depending on brightness. >>> >>> mSetAttributeString[BRIGHT_CONTROL_OFFSET] = >>> (CHAR16) ('0' + BrightControl); >>> >>> The first '4', '0' is replaced by the foreground color. >>> The second '4', '0' is replaced by the background color. >>> >>> ECMA 48 says: >>> >>> 0 - default rendition, cancels the effect of any preceding SGR >>> >>> So you can use this instead of 22. >>> >>> Best regards >>> >>> Heinrich >>> >>> >>>> >>>>> Using colors 90-97 as foreground colors produces only bright but not >>>>> bold in the KDE konsole and xterm: >>>>> >>>>> printf "\x1b[92;40m bold \x1b[32;40m normal\x1b[22;39;49m\n"; >>>>> >>>>> But these codes are not defined in ECMA-48. >>>>> >>>>> Best regards >>>>> >>>>> Heinrich >>>>> >>>> >>> >
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 + +#define EFI_ATTR_FG(attr) ((attr) & 0x0f) +#define EFI_ATTR_BG(attr) (((attr) >> 4) & 0x7) + 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 */ + { 37, 47 }, /* 7: light grey, map to white */ + { 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 */ +}; + 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); + EFI_ENTRY("%p, %lx", this, attribute); + if (attribute) + printf(ESC"[%u;%um", color[fg].fg, color[bg].bg); + else + printf(ESC"[37;40m"); + /* Just ignore attributes (colors) for now */ return EFI_EXIT(EFI_UNSUPPORTED); }
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@gmail.com> --- include/efi_api.h | 29 +++++++++++++++++++++++++++++ lib/efi_loader/efi_console.c | 30 ++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+)