Re: [PATCH RFC 2/3] xen/arm: efi: Introduce and fill the vga_console_info

[Julien Grall <julien@xxxxxxx>]

Hi,

On 07/02/2022 08:53, Jan Beulich wrote:
> On 06.02.2022 20:28, Julien Grall wrote:
> > From: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > In a follow-up patch will we want to add support for EFI framebuffer
> > in dom0. Yet, Xen may not use the framebuffer, so it would be ideal
> > to not have to enable CONFIG_VIDEO/CONFIG_VGA.
> >
> > Introduce vga_console_info in a hacky way and move the code
> > to fill it up from x86 to common.
> >
> > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > ----
> >
> > This is a bit of a hack. Sent early to gather opinion on whether
> > we should enable allow Dom0 to use the EFI Framebuffer even
> > if Xen is built with CONFIG_VIDEO=n on Arm.
> I have no input here; this will need to be settled among you Arm folks.
> I have no objection to the code movement, just one nit:
>
> > @@ -1025,6 +1026,77 @@ static void __init efi_get_apple_properties(void)
> >       }
> >   }
> >   
> > +static void __init efi_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
> > +                                  UINTN info_size,
> > +                                  EFI_GRAPHICS_OUTPUT_MODE_INFORMATION 
> > *mode_info)
> > +{
> > +#if defined(CONFIG_VIDEO) || defined(CONFIG_ARM)
> > +    int bpp = 0;
> > +
> > +    switch ( mode_info->PixelFormat )
> > +    {
> > +    case PixelRedGreenBlueReserved8BitPerColor:
> > +        vga_console_info.u.vesa_lfb.red_pos = 0;
> > +        vga_console_info.u.vesa_lfb.red_size = 8;
> > +        vga_console_info.u.vesa_lfb.green_pos = 8;
> > +        vga_console_info.u.vesa_lfb.green_size = 8;
> > +        vga_console_info.u.vesa_lfb.blue_pos = 16;
> > +        vga_console_info.u.vesa_lfb.blue_size = 8;
> > +        vga_console_info.u.vesa_lfb.rsvd_pos = 24;
> > +        vga_console_info.u.vesa_lfb.rsvd_size = 8;
> > +        bpp = 32;
> > +        break;
> > +    case PixelBlueGreenRedReserved8BitPerColor:
> > +        vga_console_info.u.vesa_lfb.red_pos = 16;
> > +        vga_console_info.u.vesa_lfb.red_size = 8;
> > +        vga_console_info.u.vesa_lfb.green_pos = 8;
> > +        vga_console_info.u.vesa_lfb.green_size = 8;
> > +        vga_console_info.u.vesa_lfb.blue_pos = 0;
> > +        vga_console_info.u.vesa_lfb.blue_size = 8;
> > +        vga_console_info.u.vesa_lfb.rsvd_pos = 24;
> > +        vga_console_info.u.vesa_lfb.rsvd_size = 8;
> > +        bpp = 32;
> > +        break;
> > +    case PixelBitMask:
> > +        bpp = set_color(mode_info->PixelInformation.RedMask, bpp,
> > +                        &vga_console_info.u.vesa_lfb.red_pos,
> > +                        &vga_console_info.u.vesa_lfb.red_size);
> > +        bpp = set_color(mode_info->PixelInformation.GreenMask, bpp,
> > +                        &vga_console_info.u.vesa_lfb.green_pos,
> > +                        &vga_console_info.u.vesa_lfb.green_size);
> > +        bpp = set_color(mode_info->PixelInformation.BlueMask, bpp,
> > +                        &vga_console_info.u.vesa_lfb.blue_pos,
> > +                        &vga_console_info.u.vesa_lfb.blue_size);
> > +        if ( mode_info->PixelInformation.ReservedMask )
> > +            bpp = set_color(mode_info->PixelInformation.ReservedMask, bpp,
> > +                            &vga_console_info.u.vesa_lfb.rsvd_pos,
> > +                            &vga_console_info.u.vesa_lfb.rsvd_size);
> > +        if ( bpp > 0 )
> > +            break;
> > +        /* fall through */
> > +    default:
> > +        PrintErr(L"Current graphics mode is unsupported!\r\n");
> > +        bpp  = 0;
> > +        break;
> > +    }
> > +    if ( bpp > 0 )
> > +    {
> > +        vga_console_info.video_type = XEN_VGATYPE_EFI_LFB;
> > +        vga_console_info.u.vesa_lfb.gbl_caps = 2; /* possibly non-VGA */
> > +        vga_console_info.u.vesa_lfb.width =
> > +            mode_info->HorizontalResolution;
> > +        vga_console_info.u.vesa_lfb.height = mode_info->VerticalResolution;
> > +        vga_console_info.u.vesa_lfb.bits_per_pixel = bpp;
> > +        vga_console_info.u.vesa_lfb.bytes_per_line =
> > +            (mode_info->PixelsPerScanLine * bpp + 7) >> 3;
> > +        vga_console_info.u.vesa_lfb.lfb_base = gop->Mode->FrameBufferBase;
> > +        vga_console_info.u.vesa_lfb.ext_lfb_base = gop->Mode->FrameBufferBase 
> > >> 32;
> > +        vga_console_info.u.vesa_lfb.lfb_size =
> > +            (gop->Mode->FrameBufferSize + 0xffff) >> 16;
> > +    }
> > +#endif
> > +}
> While you move this code, could you please insert blank lines between
> non-fall-through case blocks, and perhaps another one between the switch()
> and the if() blocks? And it looks like
> - the "gop" parameter could also do with becoming pointer-to-const,
I can do that.

> - the expanded #ifdef could do with a comment briefly explaining why Arm
>    needs-special casing.
Agree. I will wait input with the others regarding the #ifdef approach 
before respinning this patch.
Cheers,

--
Julien Grall