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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.04.2022 12:27, Roger Pau Monné wrote:
> On Thu, Mar 31, 2022 at 11:45:36AM +0200, Jan Beulich wrote:
>> --- 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?

Even if they could be const, they ought to also be static. They don't
strictly need to be, but without "static" code will be generated to
populate the on-stack variables; quite possibly the compiler would
even allocate an unnamed static variable and memcpy() from there onto
the stack.

>> +    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?

That's the theory. As per one of the post-commit-message remarks I had
looked at what GrUB does, and I decided to follow its behavior in this
regard, assuming they do what they do to work around quirks. As said
in the remark, I didn't want to go as far as also cloning their use of
the undocumented (afaik) "agp-internal-edid" variable.

>> --- 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).

Indeed (the former; there's no use in later patches).

Jan