> From: Heinrich Schuchardt <xypron.g...@gmx.de> > Date: Fri, 17 Sep 2021 04:56:31 +0200
Hi Heinrich, > On 9/16/21 3:01 PM, Mark Kettenis wrote: > > Provide correct framebuffer information for 30bpp modes. > > This is not enough to get a correct GOP implementation for the 30bpp mode. > > Have a look at efi_gop_pixel efi_vid16_to_blt_col() and > efi_blt_col_to_vid16() and where they are used. Ah right. This does indeed not support EFI_GRAPHICS_OUTPUT_PROTOCOL.Blt() correctly. I shouldn't be too hard to translate between XRGB2101010 and XRGB8888. Any ideas how I could test that code? > > Signed-off-by: Mark Kettenis <kette...@openbsd.org> > > --- > > lib/efi_loader/efi_gop.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/lib/efi_loader/efi_gop.c b/lib/efi_loader/efi_gop.c > > index 1206b2d7a2..42bf49b184 100644 > > --- a/lib/efi_loader/efi_gop.c > > +++ b/lib/efi_loader/efi_gop.c > > @@ -227,6 +227,7 @@ static efi_uintn_t gop_get_bpp(struct efi_gop *this) > > > > switch (gopobj->bpix) { > > #ifdef CONFIG_DM_VIDEO > > + case VIDEO_BPP30: > > case VIDEO_BPP32: > > #else > > case LCD_COLOR32: > > @@ -468,6 +469,7 @@ efi_status_t efi_gop_register(void) > > switch (bpix) { > > #ifdef CONFIG_DM_VIDEO > > case VIDEO_BPP16: > > + case VIDEO_BPP30: > > case VIDEO_BPP32: > > #else > > case LCD_COLOR32: > > @@ -518,6 +520,14 @@ efi_status_t efi_gop_register(void) > > #endif > > { > > gopobj->info.pixel_format = EFI_GOT_BGRA8; > > +#ifdef CONFIG_DM_VIDEO > > This symbol is not 30bpp specific. Is there a Kconfig variable that we > can use to hide 30bpp support where it is not needed? I quite deliberately didn't add a separate config option for 30bpp as there is very little code that is added to the already existing 32bpp code. In a sense 30bpp is just a different submode of the 32bpp support with just a different color map, so I thought it made sense to group it under CONFIG_VIDEO_32BPP. I did initially consider adding a separate config option for it, but it quickly turns into an #ifdef maze that makes it hard to understand the code. The CONFIG_DM_VIDEO check is just there to make sure the 30bpp is only offered in the DM model. Based on recent discussions I expect the !CONFIG_DM_VIDEO case to disappear in the near feature so adding 30bpp support for the non-DM case doesn't make sense. The EFI GOP code currently doesn't check the CONFIG_VIDEO_BPP16 and CONFIG_VIDEO_BPP32 defines to see which of these modes are enabled within U-Boot, so we don't "hide" 16bpp support on platforms that just support 32bpp either. > Which modes does the M1 support? We're not sure. It does support at least 8-bit (32bpp) and 10-bit (30bpp) color depth modes. But the firmware tends to come up with 10-bit color depth whenever it can and I'm not planning to add support for changing the framebuffer mode on the M1, since the interface to do that is "interesting" [1]. [1] See Alyssa Rozenzweig's talk at XDC 2021, https://www.youtube.com/watch?v=uTZISTjqy9Q > > > + } else if (bpix == VIDEO_BPP30) { > > + gopobj->info.pixel_format = EFI_GOT_BITMASK; > > + gopobj->info.pixel_bitmask[0] = 0x3ff00000; /* red */ > > + gopobj->info.pixel_bitmask[1] = 0x000ffc00; /* green */ > > + gopobj->info.pixel_bitmask[2] = 0x000003ff; /* blue */ > > + gopobj->info.pixel_bitmask[3] = 0xc0000000; /* reserved */ > > +#endif > > } else { > > gopobj->info.pixel_format = EFI_GOT_BITMASK; > > gopobj->info.pixel_bitmask[0] = 0xf800; /* red */ > > > >