diff mbox series

efi_loader: Fix GOP 32bpp exposure

Message ID 20180619114701.87820-1-agraf@suse.de
State Accepted
Commit 6fc2c704d40255e418857b60e86cf8d76fbcda4b
Headers show
Series efi_loader: Fix GOP 32bpp exposure | expand

Commit Message

Alexander Graf June 19, 2018, 11:47 a.m. UTC
We store pixels as BGRA in memory, as can be seen from struct efi_gop_pixel.
So we need to expose the same format to UEFI payloads to actually have them
use the correct colors.

Reported-by: Fabian Vogt <fvogt@suse.com>
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 lib/efi_loader/efi_gop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Peter Robinson June 19, 2018, 3:46 p.m. UTC | #1
On Tue, Jun 19, 2018 at 12:47 PM, Alexander Graf <agraf@suse.de> wrote:
> We store pixels as BGRA in memory, as can be seen from struct efi_gop_pixel.
> So we need to expose the same format to UEFI payloads to actually have them
> use the correct colors.
>
> Reported-by: Fabian Vogt <fvogt@suse.com>
> Signed-off-by: Alexander Graf <agraf@suse.de>
Tested-by: Peter Robinson <pbrobinson@gmail.com>

Fixes the blue penguin feet on efi consoles for me on the RPi and Pine64.

> ---
>  lib/efi_loader/efi_gop.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/efi_loader/efi_gop.c b/lib/efi_loader/efi_gop.c
> index 1afe8418e1..3a36bbcbfa 100644
> --- a/lib/efi_loader/efi_gop.c
> +++ b/lib/efi_loader/efi_gop.c
> @@ -472,7 +472,7 @@ efi_status_t efi_gop_register(void)
>         gopobj->info.version = 0;
>         gopobj->info.width = col;
>         gopobj->info.height = row;
> -       gopobj->info.pixel_format = EFI_GOT_RGBA8;
> +       gopobj->info.pixel_format = EFI_GOT_BGRA8;
>         gopobj->info.pixels_per_scanline = col;
>
>         gopobj->bpix = bpix;
> --
> 2.12.3
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
Heinrich Schuchardt June 19, 2018, 5:01 p.m. UTC | #2
On 06/19/2018 01:47 PM, Alexander Graf wrote:
> We store pixels as BGRA in memory, as can be seen from struct efi_gop_pixel.
> So we need to expose the same format to UEFI payloads to actually have them
> use the correct colors.
> 
> Reported-by: Fabian Vogt <fvogt@suse.com>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  lib/efi_loader/efi_gop.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/efi_loader/efi_gop.c b/lib/efi_loader/efi_gop.c
> index 1afe8418e1..3a36bbcbfa 100644
> --- a/lib/efi_loader/efi_gop.c
> +++ b/lib/efi_loader/efi_gop.c
> @@ -472,7 +472,7 @@ efi_status_t efi_gop_register(void)
>  	gopobj->info.version = 0;
>  	gopobj->info.width = col;
>  	gopobj->info.height = row;
> -	gopobj->info.pixel_format = EFI_GOT_RGBA8;
> +	gopobj->info.pixel_format = EFI_GOT_BGRA8;

Graphic cards have been produced both with RGB and BRG bit sequence.

According to the VESA BIOS Extension Core Functions Standard 2.0
describe a pixel we need the RedFieldPosition, GreenFieldPosition,
BlueFieldPosition, and RsvdFieldPosition fields as well as RedMaskSize,
GreenMaskSize, BlueMaskSize, and RsvdMaskSize.

For the EFI implementation we only have to consider a low endian memory
model.

Currently in drivers/video/vidconsole-uclass.c we make the following
assumptions:

16bit graphic cards have
red in bits 11..15
green in bits 5..10
blue in bis 0..4

32bit graphic cards have
red in bits 16..23
green in bits 8..15
blue in bis 0..7

of an u16 or u32 pixel.

In efi_vid16_to_blt_col() we make the same assumption.

Structure efi_gop_pixel has sequence blue, green, red, reserved which
matches the 32bit assumption in drivers/video/vidconsole-uclass.c.

struct efi_gop_pixel {
        u8 blue;
        u8 green;
        u8 red;
        u8 reserved;
};

So setting pixel_format = EFI_GOT_BGRA8 provides consistency in our
implementation but will fail on RGBA graphic cards.

@Anatolij:
Do you have an overview if we have blue in the low bits of the pixels in
all low endian U-Boot supported devices?

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

>  	gopobj->info.pixels_per_scanline = col;
>  
>  	gopobj->bpix = bpix;
>
Anatolij Gustschin June 21, 2018, 8:14 a.m. UTC | #3
Hi Heinrich,

On Tue, 19 Jun 2018 19:01:06 +0200
Heinrich Schuchardt xypron.glpk@gmx.de wrote:
...
> @Anatolij:
> Do you have an overview if we have blue in the low bits of the pixels in
> all low endian U-Boot supported devices?

No, I don't have such overview.

--
Anatolij
diff mbox series

Patch

diff --git a/lib/efi_loader/efi_gop.c b/lib/efi_loader/efi_gop.c
index 1afe8418e1..3a36bbcbfa 100644
--- a/lib/efi_loader/efi_gop.c
+++ b/lib/efi_loader/efi_gop.c
@@ -472,7 +472,7 @@  efi_status_t efi_gop_register(void)
 	gopobj->info.version = 0;
 	gopobj->info.width = col;
 	gopobj->info.height = row;
-	gopobj->info.pixel_format = EFI_GOT_RGBA8;
+	gopobj->info.pixel_format = EFI_GOT_BGRA8;
 	gopobj->info.pixels_per_scanline = col;
 
 	gopobj->bpix = bpix;