Skip to content

Setup menu color customization - #263

Open
philipanda wants to merge 12 commits into
dasharofrom
setup-menu-color-customization
Open

Setup menu color customization#263
philipanda wants to merge 12 commits into
dasharofrom
setup-menu-color-customization

Conversation

@philipanda

@philipanda philipanda commented Jul 3, 2025

Copy link
Copy Markdown
Contributor

Description

Extracting hardcoded colors from setup menu into PCDs allowing for easy customisation.
Added a Dasharo theme as a bonus and PoC.

image
  • Breaking change?
    • Breaking change - Does this PR cause a break in build or boot behavior?
    • Examples: Does it add a new library class or move a module to a different repo.
  • Impacts security?
    • Security - Does this PR have a direct security impact?
    • Examples: Crypto algorithm change or buffer overflow fix.
  • Includes tests?
    • Tests - Does this PR include any explicit test code?
    • Examples: Unit tests or integration tests.

How This Was Tested

qemu-system-x86_64 -machine q35 -serial telnet:localhost:1345,server,nowait -drive if=pflash,format=raw,unit=0,file=build/coreboot.rom

Integration Instructions

@philipanda philipanda changed the title WIP Make most color values into PCDs Setup menu color customization Jul 3, 2025
@miczyg1

miczyg1 commented Jul 3, 2025

Copy link
Copy Markdown
Contributor

I just wondered recently when someone will dare to touch CustomizedDisplayLib and play with it :)

We would have to somehow match the Dasharo green colour (on dasharo.com page the hex color for green is #38d430). Of course white and black should also be present.

If I color pick the green from the picture you posted, it gives me #009800 which is not the right one.

@philipanda

Copy link
Copy Markdown
Contributor Author

I just wondered recently when someone will dare to touch CustomizedDisplayLib and play with it :)

We would have to somehow match the Dasharo green colour (on dasharo.com page the hex color for green is #38d430). Of course white and black should also be present.

If I color pick the green from the picture you posted, it gives me #009800 which is not the right one.

CustomizedDisplayLib only supports a small color palette which looks similar to VGA color palette. There are a total of 8 possible background colors and 16 text colors, so sadly there is little possibilities to play with.

There are only two green shades possible for text - green and light green, for backgrounds its only the green you see.

I can't tell if it would be feasible to extend the color palette. It might be more challenging than just exposing the colors as PSDs.

@miczyg1

miczyg1 commented Jul 3, 2025

Copy link
Copy Markdown
Contributor

I think it will be inevitable to change mGraphicsEfiColors table in MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c

Also we have to take care of serial terminal colors in TerminalConOutSetAttribute (MdeModulePkg/Universal/Console/TerminalDxe/TerminalConOut.c)

Also you may try to change the font to something more close to what we use on dasharo.com (I believe it is oswald font regular-400 or a Font Awesome 6 regular-400)

@miczyg1

miczyg1 commented Jul 3, 2025

Copy link
Copy Markdown
Contributor

Colors taken from dasharo.com that I suggest that may be suitable:

DARKGRAY - #29363b
LIGHTGRAY - #808080
LIGHTGREEN - #38d430

@philipanda

philipanda commented Jul 5, 2025

Copy link
Copy Markdown
Contributor Author

I've exposed the hex color palette as PCDs too and come up with this color scheme in graphics mode using the hex values you've given:
image

There are only 8 possible background colors, so I had to do some non-obvious "reshades":

  • EFI_LIGHTCYAN -> #FFFFFF
  • EFI_LIGHTGREEN -> #38d430
  • EFI_BROWN -> #29363b
  • EFI_LIGHTGRAY -> #808080

For now it doesn't look that great via serial though ;)
image

@miczyg1

miczyg1 commented Jul 7, 2025

Copy link
Copy Markdown
Contributor

For now it doesn't look that great via serial though ;)

Of course it doesn't :) Check out this: https://gist.github.com/viniciusdaniel/53a98cbb1d8cac1bb473da23f5708836
Maybe the 88/256 colors are supported. If not, then it would have to be implemented in TerminalConOutSetAttribute
Currently I believe we use 8/16 colors only as they are widely supported. The 88/256 palette seem to be supported by just a handful of terminals. There is also 24bit color: https://en.wikipedia.org/wiki/ANSI_escape_code#24-bit (then maybe the same graphics RGB notations could be used in serial terminal)

@miczyg1

miczyg1 commented Jul 7, 2025

Copy link
Copy Markdown
Contributor

Couple more suggestions to the color selection:

  1. Option text (Intel ME mode) and simple text (Press ESC to exit) colors maybe should be reversed? Options green and text gray. Help text is good though.
  2. Originally the hotkey help section had a black foreground to stand out a bit differently. Can it stay black? It would probably look good with green hotkey help strings.
  3. Help section should be darker. It barely stands out from the foreground:

image

Comment thread DasharoPayloadPkg/DasharoPayloadPkg.dsc Outdated
Comment thread DasharoPayloadPkg/DasharoPayloadPkg.dsc Outdated
@philipanda

Copy link
Copy Markdown
Contributor Author

I think it will be inevitable to change mGraphicsEfiColors table in MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c

Also we have to take care of serial terminal colors in TerminalConOutSetAttribute (MdeModulePkg/Universal/Console/TerminalDxe/TerminalConOut.c)

Also you may try to change the font to something more close to what we use on dasharo.com (I believe it is oswald font regular-400 or a Font Awesome 6 regular-400)

That might be more troublesome than expected 😆
It seems that the font used by GraphicsConsole.c is defined at /MdeModulePkg/Universal/Console/GraphicsConsoleDxe/LaffStd.c. That is a huge array that maps Unicode glyphs to bitmaps made of hex bytes.

Adding a support for vector fonts might be a little too much for now, but maybe it could be feasible to create a tool that could convert vector fonts into bitmaps like the one defined in LaffStd.c.

@philipanda

philipanda commented Jul 12, 2025

Copy link
Copy Markdown
Contributor Author

I've been fiddling around with converting the Oswald font to bitmaps. The results seemed promising, but in the end I think it will not be possible to properly display a proper vector font in 19x8 resolution.

That's the best results I was able to achieve. While it's possible to do some postprocessing to fix jagged edges, losing detail is inevitable. Creating such a small font requires working with pixel art.

image

The scripts for the conversion are on a separate branch as I will scrap that idea for now.

@macpijan

macpijan commented Jul 14, 2025

Copy link
Copy Markdown
Contributor

Yeah the font does not look great and would require more work.

The colors look interesting, though.

@philipanda
philipanda force-pushed the setup-menu-color-customization branch 2 times, most recently from cb616c6 to 145ff2b Compare September 16, 2025 07:44
@philipanda

Copy link
Copy Markdown
Contributor Author

Looks like the 256 colors mode might work easily on terminals that support it.
It's enough to modify the escape codes produced by the TerminalConOutSetAttribute. I don't think there would be a simple way of knowing what color modes the client terminal understands though, so the compatibility with simple terminals might get worse.

I've tried hardcoding this pinkish color for every printed character. I think I've messed something up with the escape codes, but the color definitely looks like the one I've requested (213).
image

@philipanda

philipanda commented Sep 21, 2025

Copy link
Copy Markdown
Contributor Author

And the 24 bit mode works perfectly too, at least on Gnome terminal.
The amount of work that would be needed to implement any of them seem to be similarly low. It would require only modifying how TerminalConOutSetAttribute encodes the 16 color EFI color palette into escape codes.

The color used here is the Dasharo green #38d430.
The only little issue is that the color needs to be convered from hex to decimal to use in an escape code.
image

When accessing the serial without the 24b truecolor support using xterm:

xterm -cm

Nothing is broken, just that there are no colors:
image

I think 24b color would be the best solution. I had a hard time trying to stop my terminal from interpreting them, so that makes me believe that most potential users won't have issues with that and it won't break tests via serial in OSFV.

@philipanda

Copy link
Copy Markdown
Contributor Author

Using the 24b escape codes the colors work great via on a serial console via telnet on Qemu:
image

Some of the colors, like most of the gray/white text, don't exactly match. I suspect it's an issue with mapping the RGB colors into the 16 EFI colors along the way. Maybe the bright bit is not interpreted correctly when mapping the colors.

@philipanda
philipanda force-pushed the setup-menu-color-customization branch 2 times, most recently from 4483433 to 2473d00 Compare September 21, 2025 12:17
@miczyg1

miczyg1 commented Sep 24, 2025

Copy link
Copy Markdown
Contributor

The only little issue is that the color needs to be convered from hex to decimal to use in an escape code.

Should be easy with %02u formatter for printf.

Some of the colors, like most of the gray/white text, don't exactly match. I suspect it's an issue with mapping the RGB colors into the 16 EFI colors along the way. Maybe the bright bit is not interpreted correctly when mapping the colors.

Yeah, the letters in the top section are far from being white... But hey, it looks neat already.

Comment thread DasharoPayloadPkg/vbt.bin Outdated
Comment thread MdeModulePkg/Universal/Console/TerminalDxe/TerminalConOut.c Outdated
@philipanda

philipanda commented Dec 17, 2025

Copy link
Copy Markdown
Contributor Author

It looks like whatever exact code prints the graphical version of setup menu correctly uses the code 0x0F for white foreground, but the serial console uses 0x07 instead (no bright bit). It only affects this single color. Every other shade is correct in both the graphical and text (serial) modes. I guess the value 0x07 has to be hardcoded somewhere and I don't yet know where, or it is not updated witht the DasharoColorTheme.dsc.inc as 0x07 would be the default value from EDK2

@philipanda

philipanda commented Dec 18, 2025

Copy link
Copy Markdown
Contributor Author

It looks like whatever exact code prints the graphical version of setup menu correctly uses the code 0x0F for white foreground, but the serial console uses 0x07 instead (no bright bit). It only affects this single color. Every other shade is correct in both the graphical and text (serial) modes. I guess the value 0x07 has to be hardcoded somewhere and I don't yet know where, or it is not updated witht the DasharoColorTheme.dsc.inc as 0x07 would be the default value from EDK2

The issue was the line MdeModulePkg/Universal/Console/TerminalDxe/TerminalConOut.c:564 where the bright bit was being cleared: ForegroundControl = mTerminalEfiColors[Attribute & 0x07];. Not sure why it was like that before as the foreground supports the bright bit, unlike the background. Anding with 0x0F is the way to go and fixes the issues with invalid colors on the serial console.

With that out of the way I think all issues with this PR are fixed?

@philipanda
philipanda marked this pull request as ready for review December 18, 2025 07:38
@philipanda
philipanda force-pushed the setup-menu-color-customization branch 3 times, most recently from 899a8df to e86d004 Compare December 18, 2025 07:44
@philipanda
philipanda requested a review from miczyg1 December 18, 2025 07:45
@pietrushnic

Copy link
Copy Markdown
Contributor

Looks cool. I will not judge the implementation, but definitely having our theme is a nice direction. It gives us some branding alignment.

Comment thread DasharoPayloadPkg/DasharoColorTheme.dsc.inc Outdated
return;
}

gST->ConOut->SetAttribute (gST->ConOut, KEYHELP_TEXT | KEYHELP_BACKGROUND);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it would not be easier to replace definitions of these macros in MdeModulePkg/Library/CustomizedDisplayLib/Colors.h: like this:

-#define KEYHELP_TEXT                   EFI_LIGHTGRAY
-#define KEYHELP_BACKGROUND             EFI_BACKGROUND_BLACK
+#define KEYHELP_TEXT                   PcdGet8 (PcdBrowserKeyHelpTextColor) 
+#define KEYHELP_BACKGROUND             PcdGet8 (PcdBrowserKeyHelpBackgroundColor)

Then we could remove all those Get*Color helpers

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@philipanda do you plan to change it per suggestion?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't have much time to work on the PR lately.
Your suggestion makes sense, I do plan to change that

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw that you rebased it, so I assumed you have found some time :)

@philipanda philipanda Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After looking into it deeper I start to doubt removing the helpers and using the defines instead will make it simpler or cleaner.
There were already a couple of such helpers available because some color mixes were used multiple times across the code, example GetFieldTextColor.

I think the cleanest solution would be to make a macro like:

#define GetTextColor(type) (type##Text | type##Background)

and go with defining every foreground/background combination in the Pcds and remove all the helpers.

Otherwise it will stay a mess with multiple ways of accessing them, because some places would use (define_f | define_b) and others would call the helpers.

If so then all the usages would need to be updated too.
I think it would be a good change, but I'm not sure such a cleanup should be bundled along this PR?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9f4ccf6 applies that to only the helpers that were added in this PR before.
Added missing ErrorText color PCD too
image

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be a good change, but I'm not sure such a cleanup should be bundled along this PR?

Well, you are redesigning the whole color styles. If not here, then where? Can you assure that nothing will break if you skip it in this PR?

@philipanda philipanda Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confident nothing will break if I don't touch these public functions. I won't be so sure nothing breaks in other modules if I do. If some automatic validation would be enough to ensure it, that would be good. I'm just a bit worried about the size and duration of this PR. If it's preferred to deliver full feature-complete changes, then I guess it cannot be helped :)

Comment thread MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c
@miczyg1

miczyg1 commented Jan 7, 2026

Copy link
Copy Markdown
Contributor

I'd very much like to try and get that color customization into the upstream EDK2 next and would be grateful for any tips to improve the chances of that happening. From me, I'm afraid it is not a complete feature if only the setup menu can be colored. I've never checked how other places like the popups behave. Maybe that should be worked on first.

Pop-ups are created in two ways:

  1. CreatePopUp in MdePkg/Library/UefiLib/Console.c. Attributes (colors) are taken as an argument, so it depends on the application that creates the pop-up which color it should look like. We can't possibly change it. Even if it is EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE (like until now, in most cases) I think it is fine. Unless we sed all calls to CreatePopUp and change the colors...
  2. HiiPopup->CreatePopup using the EFI_HII_POPUP_PROTOCOL. The implementation is in MdeModulePkg/Universal/DisplayEngineDxe/Popup.c and uses the color returned by GetPopupColor, which you already modified to pick color from PCDs in the CustomizedDisplayLib.

So we would only have to handle case 1. So far, I have seen EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE everywhere, except the FUM warning in DasharoModulePkg/Library/DasharoSystemFeaturesUiLib/DasharoSystemFeatures.c, which uses EFI_BLACK | EFI_BACKGROUND_RED.

@philipanda

Copy link
Copy Markdown
Contributor Author

So we would only have to handle case 1. So far, I have seen EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE everywhere, except the FUM warning in DasharoModulePkg/Library/DasharoSystemFeaturesUiLib/DasharoSystemFeatures.c, which uses EFI_BLACK | EFI_BACKGROUND_RED.

If EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE is used almost everywhere, then a new set of two PCDs could be created to represent a default popup color. For other cases, like the FUM, it should be up to the creator of the app to either hardcode the colors, use existing, or create new PCDs. In this case it shouldn't be a problem to add a PCD for the unique FUM popup to the color palette configs.

@miczyg1

miczyg1 commented Jan 8, 2026

Copy link
Copy Markdown
Contributor

So we would only have to handle case 1. So far, I have seen EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE everywhere, except the FUM warning in DasharoModulePkg/Library/DasharoSystemFeaturesUiLib/DasharoSystemFeatures.c, which uses EFI_BLACK | EFI_BACKGROUND_RED.

If EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE is used almost everywhere, then a new set of two PCDs could be created to represent a default popup color. For other cases, like the FUM, it should be up to the creator of the app to either hardcode the colors, use existing, or create new PCDs. In this case it shouldn't be a problem to add a PCD for the unique FUM popup to the color palette configs.

We already have the PCDs: gEfiMdeModulePkgTokenSpaceGuid.PcdBrowserPopupBackgroundColor,
gEfiMdeModulePkgTokenSpaceGuid.PcdBrowserPopupTextColor that are responsible for it. But the problem is, that we would have to rewrite a significant amount of code for these popups if we would like to use these.

I think the best we can do is to replace all EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE with a new macro EFI_DEFAULT_POPUP_COLOR which would be defined as 0xFFFFFFFF (UINT32_MAX). The definition of CreatePopUp in UefiLib takes the attribute as UINTN (32bit or 64bit value depending on build/architecture), so we have more bits than 8 at disposal. So, if the Attribute input parameter would be equal to 0xFFFFFFFF (EFI_DEFAULT_POPUP_COLOR), then we will use gEfiMdeModulePkgTokenSpaceGuid.PcdBrowserPopupBackgroundColor and gEfiMdeModulePkgTokenSpaceGuid.PcdBrowserPopupTextColor, otherwise use the colors passed via the Attribute parameter. That way we only have to add these tw PCDs to UefiLib and nothing else.

@philipanda

Copy link
Copy Markdown
Contributor Author

We already have the PCDs: gEfiMdeModulePkgTokenSpaceGuid.PcdBrowserPopupBackgroundColor,
gEfiMdeModulePkgTokenSpaceGuid.PcdBrowserPopupTextColor that are responsible for it

Right, I thought they were used for some other popups I've never seen before looking that the colors are not EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, but I've looked on the Dasharo palette instead of the default values, so you're right, we already have these PCDs.

@philipanda
philipanda force-pushed the setup-menu-color-customization branch from c426ae7 to 73f2613 Compare May 29, 2026 10:40
philipanda and others added 12 commits June 10, 2026 00:18
Signed-off-by: Filip Go¿a¿ <filip.golas@3mdeb.com>
Signed-off-by: Filip Gołaś <filip.golas@3mdeb.com>
Signed-off-by: Filip Go¿a¿ <filip.golas@3mdeb.com>
Signed-off-by: Filip Gołaś <filip.golas@3mdeb.com>
Signed-off-by: Filip Gołaś <filip.golas@3mdeb.com>
Signed-off-by: Filip Gołaś <filip.golas@3mdeb.com>
Signed-off-by: Filip Gołaś <filip.golas@3mdeb.com>
Signed-off-by: Filip Gołaś <filip.golas@3mdeb.com>
Signed-off-by: Filip Gołaś <filip.golas@3mdeb.com>
Signed-off-by: Filip Gołaś <filip.golas@3mdeb.com>
Signed-off-by: Filip Gołaś <philipgoclever@gmail.com>
… later

Signed-off-by: Filip Gołaś <philipgoclever@gmail.com>
@philipanda
philipanda force-pushed the setup-menu-color-customization branch from 166e673 to 9f4ccf6 Compare June 9, 2026 22:18
@philipanda

philipanda commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

I think the best we can do is to replace all EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE with a new macro EFI_DEFAULT_POPUP_COLOR which would be defined as 0xFFFFFFFF (UINT32_MAX). The definition of CreatePopUp in UefiLib takes the attribute as UINTN (32bit or 64bit value depending on build/architecture), so we have more bits than 8 at disposal. So, if the Attribute input parameter would be equal to 0xFFFFFFFF (EFI_DEFAULT_POPUP_COLOR), then we will use gEfiMdeModulePkgTokenSpaceGuid.PcdBrowserPopupBackgroundColor and gEfiMdeModulePkgTokenSpaceGuid.PcdBrowserPopupTextColor, otherwise use the colors passed via the Attribute parameter. That way we only have to add these tw PCDs to UefiLib and nothing else.

Hmm, there's an intention problem in this case. The popup creator most of the times does not care about the color, but when they do, they want to choose it specifically (like in capsules V2, green for a success, red for a failure).
With the color themes this use case will be broken as e.g. EFI_BACKGROUND_RED can be remapped by the PCDs to any color depending on the theme. Using any EFI_ defines directly should be discouraged in this PR as they don't really have a meaning.

Ideally the creator of a custom-color popup would also provide some PCDs in MdeModulePkg to configure it's color or use existing ones.
But with a limited color palette it might not be possible to satisfy everyone.

@miczyg1

miczyg1 commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

I think the best we can do is to replace all EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE with a new macro EFI_DEFAULT_POPUP_COLOR which would be defined as 0xFFFFFFFF (UINT32_MAX). The definition of CreatePopUp in UefiLib takes the attribute as UINTN (32bit or 64bit value depending on build/architecture), so we have more bits than 8 at disposal. So, if the Attribute input parameter would be equal to 0xFFFFFFFF (EFI_DEFAULT_POPUP_COLOR), then we will use gEfiMdeModulePkgTokenSpaceGuid.PcdBrowserPopupBackgroundColor and gEfiMdeModulePkgTokenSpaceGuid.PcdBrowserPopupTextColor, otherwise use the colors passed via the Attribute parameter. That way we only have to add these tw PCDs to UefiLib and nothing else.

Hmm, there's an intention problem in this case. The popup creator most of the times does not care about the color, but when they do, they want to choose it specifically (like in capsules V2, green for a success, red for a failure). With the color themes this use case will be broken as e.g. EFI_BACKGROUND_RED can be remapped by the PCDs to any color depending on the theme. Using any EFI_ defines directly should be discouraged in this PR as they don't really have a meaning.

Ideally the creator of a custom-color popup would also provide some PCDs in MdeModulePkg to configure it's color or use existing ones. But with a limited color palette it might not be possible to satisfy everyone.

And that would still work as expected:

  1. CreatePopUp with EFI_DEFAULT_POPUP_COLOR (0xffffffff) passed as param causes the use of gEfiMdeModulePkgTokenSpaceGuid.PcdBrowserPopupBackgroundColor and gEfiMdeModulePkgTokenSpaceGuid.PcdBrowserPopupTextColor.
  2. CreatePopUp with EFI_BACKGROUND_RED or anythign else, will not cause the use of PCDs gEfiMdeModulePkgTokenSpaceGuid.PcdBrowserPopupBackgroundColor and gEfiMdeModulePkgTokenSpaceGuid.PcdBrowserPopupTextColor as defaults, but instead take the EFI_BACKGROUND_RED, because it is not equal to 0xffffffff.

I have the same intention of preserving the behaviors.

Ideally the creator of a custom-color popup would also provide some PCDs in MdeModulePkg to configure it's color or use existing ones.

There is no need for that


TerminalEfiColor mTerminalEfiColors[16];

VOID CopyTerminalPaletteColor(IN UINT8* Src, OUT TerminalEfiColor* Dest) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usual order of parameters for copy methods is Copy(Dest, Src)

@philipanda

Copy link
Copy Markdown
Contributor Author

And that would still work as expected:

  1. CreatePopUp with EFI_DEFAULT_POPUP_COLOR (0xffffffff) passed as param causes the use of gEfiMdeModulePkgTokenSpaceGuid.PcdBrowserPopupBackgroundColor and gEfiMdeModulePkgTokenSpaceGuid.PcdBrowserPopupTextColor.
  2. CreatePopUp with EFI_BACKGROUND_RED or anythign else, will not cause the use of PCDs gEfiMdeModulePkgTokenSpaceGuid.PcdBrowserPopupBackgroundColor and gEfiMdeModulePkgTokenSpaceGuid.PcdBrowserPopupTextColor as defaults, but instead take the EFI_BACKGROUND_RED, because it is not equal to 0xffffffff.

Won't the "meaning" of colors be overridden here in GraphicsConsole: https://github.com/Dasharo/edk2/pull/263/changes#diff-da573dc5f5ed707edfc467f207afb3e8098a02471e651d52de8fa06e0dca3648R92-R107 ?

So choosing EFI_BACKGROUND_RED could result in an arbitrary color depending on what the theme sets for "Red": https://github.com/Dasharo/edk2/pull/263/changes#diff-6ae418dfaf0f09fb32c298d627322035bee128c1b76800be287ca6744bab54b7R64

Comment on lines +2026 to +2042
gEfiMdeModulePkgTokenSpaceGuid.PcdGraphicsConsoleColorPaletteBlack | { 0x00, 0x00, 0x00, 0x00 } | VOID* | 0x00010400
gEfiMdeModulePkgTokenSpaceGuid.PcdGraphicsConsoleColorPaletteBlue | { 0x00, 0x00, 0x00, 0x98 } | VOID* | 0x00010401
gEfiMdeModulePkgTokenSpaceGuid.PcdGraphicsConsoleColorPaletteGreen | { 0x00, 0x00, 0x98, 0x00 } | VOID* | 0x00010402
gEfiMdeModulePkgTokenSpaceGuid.PcdGraphicsConsoleColorPaletteCyan | { 0x00, 0x00, 0x98, 0x98 } | VOID* | 0x00010403
gEfiMdeModulePkgTokenSpaceGuid.PcdGraphicsConsoleColorPaletteRed | { 0x00, 0x98, 0x00, 0x00 } | VOID* | 0x00010404
gEfiMdeModulePkgTokenSpaceGuid.PcdGraphicsConsoleColorPaletteLightMagenta | { 0x00, 0x98, 0x00, 0x98 } | VOID* | 0x00010405
gEfiMdeModulePkgTokenSpaceGuid.PcdGraphicsConsoleColorPaletteBrown | { 0x00, 0x98, 0x98, 0x00 } | VOID* | 0x00010406
gEfiMdeModulePkgTokenSpaceGuid.PcdGraphicsConsoleColorPaletteLightGray | { 0x00, 0x98, 0x98, 0x98 } | VOID* | 0x00010407

gEfiMdeModulePkgTokenSpaceGuid.PcdGraphicsConsoleColorPaletteDarkGray | { 0x00, 0x30, 0x30, 0x30 } | VOID* | 0x00010408
gEfiMdeModulePkgTokenSpaceGuid.PcdGraphicsConsoleColorPaletteLightBlue | { 0x00, 0x00, 0x00, 0xFF } | VOID* | 0x00010409
gEfiMdeModulePkgTokenSpaceGuid.PcdGraphicsConsoleColorPaletteLightGreen | { 0x00, 0x00, 0xFF, 0x00 } | VOID* | 0x0001040A
gEfiMdeModulePkgTokenSpaceGuid.PcdGraphicsConsoleColorPaletteLightCyan | { 0x00, 0xFF, 0xFF, 0x00 } | VOID* | 0x0001040B
gEfiMdeModulePkgTokenSpaceGuid.PcdGraphicsConsoleColorPaletteLightRed | { 0x00, 0xFF, 0x00, 0x00 } | VOID* | 0x0001040C
gEfiMdeModulePkgTokenSpaceGuid.PcdGraphicsConsoleColorPaletteMagenta | { 0x00, 0xFF, 0x00, 0xFF } | VOID* | 0x0001040D
gEfiMdeModulePkgTokenSpaceGuid.PcdGraphicsConsoleColorPaletteYellow | { 0x00, 0xFF, 0xFF, 0x00 } | VOID* | 0x0001040E
gEfiMdeModulePkgTokenSpaceGuid.PcdGraphicsConsoleColorPaletteWhite | { 0x00, 0xFF, 0xFF, 0xFF } | VOID* | 0x0001040F

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the redundant spaces. They are not even aligned properly for readability

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought they helped with readibility by splitting it into two blocks of 8 each.
Can remove, sure

@miczyg1 miczyg1 Jun 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant the spaces near the pipes |, e.g.

  gEfiMdeModulePkgTokenSpaceGuid.PcdGraphicsConsoleColorPaletteLightRed        | { 0x00, 0xFF, 0x00, 0x00 } | VOID* | 0x0001040C
  gEfiMdeModulePkgTokenSpaceGuid.PcdGraphicsConsoleColorPaletteMagenta    | { 0x00, 0xFF, 0x00, 0xFF } | VOID* | 0x0001040D

See? Eveywhere else in the repo there is no space around pipe character |, so I would do the same here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, I see now 👍

@miczyg1

miczyg1 commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Won't the "meaning" of colors be overridden here in GraphicsConsole: https://github.com/Dasharo/edk2/pull/263/changes#diff-da573dc5f5ed707edfc467f207afb3e8098a02471e651d52de8fa06e0dca3648R92-R107 ?

So choosing EFI_BACKGROUND_RED could result in an arbitrary color depending on what the theme sets for "Red": https://github.com/Dasharo/edk2/pull/263/changes#diff-6ae418dfaf0f09fb32c298d627322035bee128c1b76800be287ca6744bab54b7R64

Right, the meaning of colors has changed because of gEfiMdeModulePkgTokenSpaceGuid.PcdGraphicsConsoleColorPalette* PCDs. I forgot that we have only 4 bits for foreground color and 4 bits for background color, and it is enforced by UEFI spec: https://uefi.org/specs/UEFI/2.11/12_Protocols_Console_Support.html#efi-simple-text-output-protocol-setattribute

Still, my suggestion remains the same with EFI_DEFAULT_POPUP_COLOR (0xffffffff). However, if the parameter is not EFI_DEFAULT_POPUP_COLOR then we have to temporarily restore the default color pallete (copy original mGraphicsEfiColors and mTerminalEfiColorsand rename them to mDefaultGraphicsEfiColors and mDefaultTerminalEfiColors, and keep our custom in the mGraphicsEfiColors and mTerminalEfiColorsand) during the time the pop up is shown.

Also, we may only override the pallete once we enter setup (exception, red pop-ups insetup). If we leave the setup, we may bring back the default one.

@philipanda

philipanda commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Still, my suggestion remains the same with EFI_DEFAULT_POPUP_COLOR (0xffffffff). However, if the parameter is not EFI_DEFAULT_POPUP_COLOR then we have to temporarily restore the default color pallete (copy original mGraphicsEfiColors and mTerminalEfiColorsand rename them to mDefaultGraphicsEfiColors and mDefaultTerminalEfiColors, and keep our custom in the mGraphicsEfiColors and mTerminalEfiColorsand) during the time the pop up is shown.

Hmm, interesing, if we're going that far, then why not support multiple palletes everywhere and switch them dynamically?
Setup-menu pallete, boot manager pallete, capsule update results pallete etc.

On the other hand, I think it would be cleanest to stop treating colors like RGB, and instead choose them semantically. Like in Colors.h, e.g. ERROR_TEXT. It would change the approach, but allow full theming. The author of a theme could decide what color they want to mean a warning, what color would mean a success etc. E.g. in capsule updates results screen, an Error color would be used instead of Red for a failure, and a Success color would be used instead of Green.
The theme could contain colors for different kinds of popups, not just a single default one where choosing anything else would mean breaking out of the theme.

I guess a way of overriding this and loading the default theme could be needed still. But if we're doing such an overhaul, why not just apply themes to everything in EDK2?

@miczyg1

miczyg1 commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

On the other hand, I think it would be cleanest to stop treating colors like RGB, and instead choose them semantically.

Note that we are still limited by the UEFI specification which clearly states 4bits for FG color and 4 bits for BG color: https://uefi.org/specs/UEFI/2.11/12_Protocols_Console_Support.html#efi-simple-text-output-protocol-setattribute
We can't break that. We have 16 colors at disposal for the terminal and that's it. We can only switch them whenever needed.

Setup-menu pallete, boot manager pallete, capsule update results pallete etc.

Do we really need that many? I think we initially wanted to customize the setup and that's enough. Everything else may stay as it was.

I guess a way of overriding this and loading the default theme could be needed still. But if we're doing such an overhaul, why not just apply themes to everything in EDK2?

Can you elaborate?

@philipanda

philipanda commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

I guess a way of overriding this and loading the default theme could be needed still. But if we're doing such an overhaul, why not just apply themes to everything in EDK2?

Can you elaborate?

I mean overriding the custom theme and going back to the default colors everyone expects might be needed just in case some screen / efi app would not want to be affected by the custom themes. It might be or not be needed. I'd say it's not needed, but might be nice to keep such posibility in mind?

By overhaul I mean making every call of CreatePopUp use the -1 special value instead of EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE. If we are to touch outside code, why not go all out and just make them explicitly use some semantic names for the popup colors.
So all the calls to CreatePopUp could look like:

CreatePopUp (POPUP_INFO, (...));
CreatePopUp (POPUP_ERROR, (...));

I can't think of a serious reason why someone would really want to have a specific color that's unaffected by the themes. I think the colors used are supposed to carry some meaning related to it, not the color RGB values.

@miczyg1

miczyg1 commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Graphical setup UI in upstream coreboot:
https://review.coreboot.org/c/coreboot/+/93892
MrChromebox/edk2@0c9ba81

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants