Re: [PATCH v4 3/8] x86/EFI: retrieve EDID

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Mar 31, 2022 at 11:45:36AM +0200, Jan Beulich wrote:
> When booting directly from EFI, obtaining this information from EFI is
> the only possible way. And even when booting with a boot loader
> interposed, it's more clean not to use legacy BIOS calls for this
> purpose. (The downside being that there are no "capabilities" that we
> can retrieve the EFI way.)
> 
> To achieve this we need to propagate the handle used to obtain the
> EFI_GRAPHICS_OUTPUT_PROTOCOL instance for further obtaining an
> EFI_EDID_*_PROTOCOL instance, which has been part of the spec since 2.5.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> Setting boot_edid_caps to zero isn't desirable, but arbitrarily setting
> one or both of the two low bits also doesn't seem appropriate.
> 
> GrUB also checks an "agp-internal-edid" variable. As I haven't been able
> to find any related documentation, and as GrUB being happy about the
> variable being any size (rather than at least / precisely 128 bytes),
> I didn't follow that route.
> ---
> v3: Re-base.
> v2: New.
> 
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -464,6 +464,10 @@ static void __init efi_arch_edd(void)
>  {
>  }
>  
> +static void __init efi_arch_edid(EFI_HANDLE gop_handle)
> +{
> +}
> +
>  static void __init efi_arch_memory_setup(void)
>  {
>  }
> --- a/xen/arch/x86/boot/video.S
> +++ b/xen/arch/x86/boot/video.S
> @@ -922,7 +922,14 @@ store_edid:
>          pushw   %dx
>          pushw   %di
>  
> -        cmpb    $1, bootsym(opt_edid)   # EDID disabled on cmdline (edid=no)?
> +        movb    bootsym(opt_edid), %al
> +        cmpw    $0x1313, bootsym(boot_edid_caps) # Data already retrieved?
> +        je      .Lcheck_edid
> +        cmpb    $2, %al                 # EDID forced on cmdline 
> (edid=force)?
> +        jne     .Lno_edid
> +
> +.Lcheck_edid:
> +        cmpb    $1, %al                 # EDID disabled on cmdline (edid=no)?
>          je      .Lno_edid
>  
>          leaw    vesa_glob_info, %di
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -568,6 +568,49 @@ static void __init efi_arch_video_init(E
>  #endif
>  }
>  
> +#ifdef CONFIG_VIDEO
> +static bool __init copy_edid(const void *buf, unsigned int size)
> +{
> +    /*
> +     * Be conservative - for both undersized and oversized blobs it is 
> unclear
> +     * what to actually do with them. The more that unlike the VESA BIOS
> +     * interface we also have no associated "capabilities" value (which might
> +     * carry a hint as to possible interpretation).
> +     */
> +    if ( size != ARRAY_SIZE(boot_edid_info) )
> +        return false;
> +
> +    memcpy(boot_edid_info, buf, size);
> +    boot_edid_caps = 0;
> +
> +    return true;
> +}
> +#endif
> +
> +static void __init efi_arch_edid(EFI_HANDLE gop_handle)
> +{
> +#ifdef CONFIG_VIDEO
> +    static EFI_GUID __initdata active_guid = EFI_EDID_ACTIVE_PROTOCOL_GUID;
> +    static EFI_GUID __initdata discovered_guid = 
> EFI_EDID_DISCOVERED_PROTOCOL_GUID;

Is there a need to make those static?

I think this function is either called from efi_start or
efi_multiboot, but there aren't multiple calls to it? (also both
parameters are IN only, so not to be changed by the EFI method?

I have the feeling setting them to static is done because they can't
be set to const?

> +    EFI_EDID_ACTIVE_PROTOCOL *active_edid;
> +    EFI_EDID_DISCOVERED_PROTOCOL *discovered_edid;
> +    EFI_STATUS status;
> +
> +    status = efi_bs->OpenProtocol(gop_handle, &active_guid,
> +                                  (void **)&active_edid, efi_ih, NULL,
> +                                  EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> +    if ( status == EFI_SUCCESS &&
> +         copy_edid(active_edid->Edid, active_edid->SizeOfEdid) )
> +        return;

Isn't it enough to just call EFI_EDID_ACTIVE_PROTOCOL_GUID?

>From my reading of the UEFI spec this will either return
EFI_EDID_OVERRIDE_PROTOCOL_GUID or EFI_EDID_DISCOVERED_PROTOCOL_GUID.
If EFI_EDID_OVERRIDE_PROTOCOL is set it must be used, and hence
falling back to EFI_EDID_DISCOVERED_PROTOCOL_GUID if
EFI_EDID_ACTIVE_PROTOCOL_GUID cannot be parsed would likely mean
ignoring EFI_EDID_OVERRIDE_PROTOCOL?

> +    status = efi_bs->OpenProtocol(gop_handle, &discovered_guid,
> +                                  (void **)&discovered_edid, efi_ih, NULL,
> +                                  EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> +    if ( status == EFI_SUCCESS )
> +        copy_edid(discovered_edid->Edid, discovered_edid->SizeOfEdid);
> +#endif
> +}
> +
>  static void __init efi_arch_memory_setup(void)
>  {
>      unsigned int i;
> @@ -729,6 +772,7 @@ static void __init efi_arch_flush_dcache
>  void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> *SystemTable)
>  {
>      EFI_GRAPHICS_OUTPUT_PROTOCOL *gop;
> +    EFI_HANDLE gop_handle;
>      UINTN cols, gop_mode = ~0, rows;
>  
>      __set_bit(EFI_BOOT, &efi_flags);
> @@ -742,11 +786,15 @@ void __init efi_multiboot2(EFI_HANDLE Im
>                             &cols, &rows) == EFI_SUCCESS )
>          efi_arch_console_init(cols, rows);
>  
> -    gop = efi_get_gop();
> +    gop = efi_get_gop(&gop_handle);
>  
>      if ( gop )
> +    {
>          gop_mode = efi_find_gop_mode(gop, 0, 0, 0);
>  
> +        efi_arch_edid(gop_handle);
> +    }
> +
>      efi_arch_edd();
>      efi_arch_cpu();
>  
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -118,7 +118,7 @@ static bool read_section(const EFI_LOADE
>  
>  static void efi_init(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable);
>  static void efi_console_set_mode(void);
> -static EFI_GRAPHICS_OUTPUT_PROTOCOL *efi_get_gop(void);
> +static EFI_GRAPHICS_OUTPUT_PROTOCOL *efi_get_gop(EFI_HANDLE *gop_handle);
>  static UINTN efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
>                                 UINTN cols, UINTN rows, UINTN depth);
>  static void efi_tables(void);
> @@ -758,7 +758,7 @@ static void __init efi_console_set_mode(
>          StdOut->SetMode(StdOut, best);
>  }
>  
> -static EFI_GRAPHICS_OUTPUT_PROTOCOL __init *efi_get_gop(void)
> +static EFI_GRAPHICS_OUTPUT_PROTOCOL __init *efi_get_gop(EFI_HANDLE 
> *gop_handle)
>  {
>      EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info;
>      EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
> @@ -783,7 +783,10 @@ static EFI_GRAPHICS_OUTPUT_PROTOCOL __in
>              continue;
>          status = gop->QueryMode(gop, gop->Mode->Mode, &info_size, 
> &mode_info);
>          if ( !EFI_ERROR(status) )
> +        {
> +            *gop_handle = handles[i];
>              break;
> +        }
>      }
>      if ( handles )
>          efi_bs->FreePool(handles);
> @@ -1222,6 +1225,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
>      if ( use_cfg_file )
>      {
>          EFI_FILE_HANDLE dir_handle;
> +        EFI_HANDLE gop_handle;
>          UINTN depth, cols, rows, size;
>  
>          size = cols = rows = depth = 0;
> @@ -1230,7 +1234,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
>                                 &cols, &rows) == EFI_SUCCESS )
>              efi_arch_console_init(cols, rows);
>  
> -        gop = efi_get_gop();
> +        gop = efi_get_gop(&gop_handle);
>  
>          /* Get the file system interface. */
>          dir_handle = get_parent_handle(loaded_image, &file_name);
> @@ -1360,7 +1364,11 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
>          dir_handle->Close(dir_handle);
>  
>          if ( gop && !base_video )
> +        {
>              gop_mode = efi_find_gop_mode(gop, cols, rows, depth);
> +
> +            efi_arch_edid(gop_handle);
> +        }
>      }
>  
>      /* Get the number of boot modules specified on the DT or an error (<0) */
> @@ -1387,7 +1395,6 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
>  
>      efi_arch_edd();
>  
> -    /* XXX Collect EDID info. */
>      efi_arch_cpu();
>  
>      efi_tables();
> --- a/xen/include/efi/efiprot.h
> +++ b/xen/include/efi/efiprot.h
> @@ -724,5 +724,52 @@ struct _EFI_GRAPHICS_OUTPUT_PROTOCOL {
>    EFI_GRAPHICS_OUTPUT_PROTOCOL_BLT         Blt;
>    EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE        *Mode;
>  };
> +
> +/*
> + * EFI EDID Discovered Protocol
> + * UEFI Specification Version 2.5 Section 11.9
> + */
> +#define EFI_EDID_DISCOVERED_PROTOCOL_GUID \
> +    { 0x1C0C34F6, 0xD380, 0x41FA, { 0xA0, 0x49, 0x8a, 0xD0, 0x6C, 0x1A, 
> 0x66, 0xAA} }
> +
> +typedef struct _EFI_EDID_DISCOVERED_PROTOCOL {
> +    UINT32   SizeOfEdid;
> +    UINT8   *Edid;
> +} EFI_EDID_DISCOVERED_PROTOCOL;
> +
> +/*
> + * EFI EDID Active Protocol
> + * UEFI Specification Version 2.5 Section 11.9
> + */
> +#define EFI_EDID_ACTIVE_PROTOCOL_GUID \
> +    { 0xBD8C1056, 0x9F36, 0x44EC, { 0x92, 0xA8, 0xA6, 0x33, 0x7F, 0x81, 
> 0x79, 0x86} }
> +
> +typedef struct _EFI_EDID_ACTIVE_PROTOCOL {
> +    UINT32   SizeOfEdid;
> +    UINT8   *Edid;
> +} EFI_EDID_ACTIVE_PROTOCOL;
> +
> +/*
> + * EFI EDID Override Protocol
> + * UEFI Specification Version 2.5 Section 11.9
> + */
> +#define EFI_EDID_OVERRIDE_PROTOCOL_GUID \
> +    { 0x48ECB431, 0xFB72, 0x45C0, { 0xA9, 0x22, 0xF4, 0x58, 0xFE, 0x04, 
> 0x0B, 0xD5} }
> +
> +INTERFACE_DECL(_EFI_EDID_OVERRIDE_PROTOCOL);
> +
> +typedef
> +EFI_STATUS
> +(EFIAPI *EFI_EDID_OVERRIDE_PROTOCOL_GET_EDID) (
> +  IN      struct _EFI_EDID_OVERRIDE_PROTOCOL   *This,
> +  IN      EFI_HANDLE                           *ChildHandle,
> +  OUT     UINT32                               *Attributes,
> +  IN OUT  UINTN                                *EdidSize,
> +  IN OUT  UINT8                               **Edid);
> +
> +typedef struct _EFI_EDID_OVERRIDE_PROTOCOL {
> +    EFI_EDID_OVERRIDE_PROTOCOL_GET_EDID  GetEdid;
> +} EFI_EDID_OVERRIDE_PROTOCOL;
> +
>  #endif

FWIW, EFI_EDID_OVERRIDE_PROTOCOL_GUID is not used by the patch, so I
guess it's introduced for completeness (or because it's used by
further patches).

Thanks, Roger.