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 <jbeul...@suse.com>
> ---
> 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.
