[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4 3/8] x86/EFI: retrieve EDID
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |