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

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


  • To: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Wed, 6 Apr 2022 10:44:12 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.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=xWmlH7K5oMVHAVbbonVfpBJtic6qwjgRbrZCHQB0ptY=; b=dqbrSHrSHCezkqImfS5KXTAEYikQP9PBHzi80gdJyglDUy+yJD4dc4/X26/Ke4qhUhYFBV99vVm5AJzALwRhVqx2FWOSRtzCtvSHmI/MEYy0NZCuPMQLF1jJPOIxzUg80UfhP5t8qhWQT4nhl4maHQa92REB72zWm6uQc7pHSDqgy4QwOwwOa/6waDlro7+alocXzBQ61WMI8vPGlApQR0F4LTpvVE/N8bwg5zNSv/tI2SV6V1594IdW6kMTNdkZnRumJKv6QApqjxwOxtzGMbpSF/Cf5cAqJ2PdWPjY30qqpq4bY30foFHrGG+0Pv3XlSIv6fgpta++aa6wRvLMng==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=aGrbqwBsrYPiFymcV+vGyT64RsrfPfpS2xC4q65CuTi0F0jFcWi7ayoV8PjQXSoxU7z/TzC9HIrx+wS8pA8i6qkCPSPpd1tGMy4xWcwx+mNCCwmwgz+J9f1UDxqvKo8g/lN3zV6MV1kL740LHbMSefFfjTeGRWe38c8lxILA5Avjy3Tzdp9JLNMSLGPT/kWGv+chkheFNnc1dUH1nIRDlbn5oK0/YHadylPSZ/4TY1SLnQ4PBRe9cPJ7XxTRgxDl+12jIleOZIMqrADpHu/db1YFv38Mgo3EXY2rER/6ft3ICc5VL5LU+9ewYysZn4ZMg0FVAU8e2Gm/GTypUnvQag==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.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: Wed, 06 Apr 2022 08:44:50 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 05.04.2022 17:47, Roger Pau Monné wrote:
> On Tue, Apr 05, 2022 at 04:36:53PM +0200, Jan Beulich wrote:
>> 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.
> 
> I thought that making those const (and then annotate with __initconst)
> would already have the same effect as having it static, as there will
> be no memcpy in that case either.

You cannot annotate non-static variables with __initconst.

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

Actually it's a little different, as I realized while re-checking in
order to reply to your request below. While GrUB looks to use this
only "just in case", our use is actually to also cope with failure
in copy_edid(): In case the overridden EDID doesn't match the size
constraint (which is more strict than GrUB's), we would retry with
the "discovered" one in the hope that its size is okay.

> Could you add this as a comment here? So it's not lost on commit as
> being just a post-commit log remark.

As a result I'm unsure of the value of a comment here going beyond
what the comment in copy_edid() already says. For now I've added

    /*
     * In case an override is in place which doesn't fit copy_edid(), also try
     * obtaining the discovered EDID in the hope that it's better than nothing.
     */

In turn ...

> With that:
> 
> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

... I'll wait with applying this (thanks) until we've reached
agreement.

Jan




 


Rackspace

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