[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Tue, 5 Apr 2022 12:27:40 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=GYVut/VAL22ydjAROR+Mnb1G9/oiCc3JqVynNIb93J8=; b=cKatRWN7uFOyVbIT0q4rDOYgGhXvr4uM1LbEcOwpjdeR5XCEmGNxhPqWc5oAaChmCMUzcH2U3FnK9pdyF63ouedJSP+NSKN3e2m8Zt9zkrpR21lKchOBV2IzPBQKwU+s6REKP7j4Hs4rUtVhl3fr3l7Y3+geJL3o9pNuG6+XgH7Res2pIterAhOvRsVVdZNRDbw09gZEgsnL018nzRjene9muQ5ToiT6caCom0mA6Jzh0egGSs/jH4vSVKaId7pFhux+Wyva1A1lkVjF9gB1LBTYJuXhQ0FVXY/xLHMKJ8pSZwXeYdx8JRG3TVSe4+1yz4UMefzH02VN7u/pgEiWag==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=N8D2iR9wkmNH8D1EF4aoibn68bqh13y3wWVUHIlMPgGWeYFNiAmcbc0PlJH5j3VyWs4OhcbEJHCC947+EkxYHIoHl/TqZy9MRVAQjsCQQXgPcc45irBpnQ5r74dXOOhak05ky1B1A/p3spqQAmEVQjOdnInhkdjlD5db2Yehz/4d+/ZUOwx0apeiXvZs3gzxcelF3oGroJu+I/iI8ReQ3D5IaosFDaEZoOSl7+yqIcsmg4vb2g1I4RA71VNIazhABJNDBZgAcQGUxc++VxD+f0t2RWMinkhCkxyo/2vlE8pdTrKEAtiy61L7JpL7OWwXQqAOuHR7SfXIjQU6w9L0mQ==
  • Authentication-results: esa3.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, "Andrew Cooper" <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, "Volodymyr Babchuk" <volodymyr_babchuk@xxxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>
  • Delivery-date: Tue, 05 Apr 2022 10:28:01 +0000
  • Ironport-data: A9a23:mfjPnqC2HslliBVW/9zjw5YqxClBgxIJ4kV8jS/XYbTApDN2hjdTz jEYWD/TOPqPamagft8kYInjoEMPv8LSyINkQQY4rX1jcSlH+JHPbTi7wuYcHM8wwunrFh8PA xA2M4GYRCwMZiaA4E/raNANlFEkvU2ybuOU5NXsZ2YgHWeIdA970Ug5w7Jh39Yx6TSEK1jlV e3a8pW31GCNg1aYAkpMg05UgEoy1BhakGpwUm0WPZinjneH/5UmJMt3yZWKB2n5WuFp8tuSH I4v+l0bElTxpH/BAvv9+lryn9ZjrrT6ZWBigVIOM0Sub4QrSoXfHc/XOdJFAXq7hQllkPhQk dRtkpe8FzsXN/f+o9gYaBN3MHlhaPguFL/veRBTsOSWxkzCNXDt3+9vHAc9OohwFuRfWD8Us 6ZCcXZUM07F17neLLGTE4GAguw5K8bmJsUHs2xIxjDFF/c2B5vERs0m4PcGjG9q15wWQJ4yY eJJVWtDMAvQcSdxBWxGGaAumMPwij7gJmgwRFW9+vNsvjm7IBZK+KP2LNPfd9iORMNUtkWVv GTL+yL+GB5yHMKYzT2J43e9nNjFlCnwWJ8RPLCg//ssi1qWrkQZBQcKT1K9rb+8g1SnRtNEA 0UO/2wlqq1a3E62StjwWTWorXjCuQQTM/JSGeAn7ACGyoLP/h2UQGMDS1ZpeNEg8cM7WzEu/ luIhM/yQyxitqWPTnCQ/avSqim9UQAKKUcSaClCShEKi+QPu6lq0EiJFIw6Vvfo0JulQlkc3 gxmsgAf3rQzqsUHiZmF9Ar1uA6pnZLmbVYqs1C/sn2e0it1Y4usZoqN4Ffd7OpdIIvxcmRtr EToiODFsrlQUMjleDilBbxUQer3v6rt3Cj02wYHInU3y9i6F5dPl6h06So2GkpmO91sldTBM B6K4lM5CHO+0RKXgU5Lj2CZVp9CIUvIT42NuhXogjxmOMUZmOivpnwGWKJo9zqx+HXAaIlmU XthTe6iDGwBFYNsxyesSuEW3NcDn35ilDuJHcymk0/7jdJygUJ5r59fbTNiichjssu5TPj9q Y4DZ6NmNT0BOAEBXsUn2dFKdg1bRZTKLZv3t9ZWZoa+zvlOQwkc5wvq6ep5IeRNxv0N/s+Rp y3VchIImTLX2CycQS3XOy8LVV8adcsmxZ7NFXd3ZgjANrlKSdvH0ZrzgLNsJON9qbM6kaAvJ xTHEu3Zaslypv380211RbH2rZB4dQTtggSLPiG/ZyM4cYImTAvMkuIItCO1nMXSJkJbbfcDn oA=
  • Ironport-hdrordr: A9a23:Oqt+MKwPrGPOTnpMQVGLKrPxwuskLtp133Aq2lEZdPULSKKlfp GV88jziyWZtN9wYhEdcdDpAtjnfZr5z+8J3WBxB8bZYOCCggqVxe5ZnO7fKlHbaknDH6tmpN tdmstFeazN5DpB/L7HCWCDer5KqrT3k9HLuQ6d9QYXcegDUdAf0+4TMHfjLqQZfnggOXJvf6 Dsmfav6gDQMUg/X4CePD0oTuLDr9rEmNbPZgMHPQcu7E2rgSmz4LD3PhCE1lNGOgk/i4sKwC zgqUjU96+ju/a0xlv10HLS1Y1fnJ/ExsFYDMKBp8AJInHHixquZq5mR7qe1QpF692H2RIPqp 3hsh0gN8N85zf4eXy0mwLk303a3DMn+xbZuCilqEqmhfa8aCMxCsJHi44cWADe8VAcsNZ117 8O936FtrJMZCmw0xjV1pztbVVHh0C0qX0tnao4lHpES7YTb7dXsMg24F5VKpEdByj3gbpXXN WGNPuspcq+TGnqL0ww5gJUsZ+RtzUIb1q7q3E5y4KoO2M8pgE686MarPZv6kvouqhNDqWs3N 60QZiApIs+PvP+UpgNdtvpYfHHfFAlEii8eV57HzzcZdQ60jT22trK3Ik=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.