On 06.04.2022 16:14, Roger Pau Monné wrote: > On Wed, Apr 06, 2022 at 02:40:50PM +0200, Jan Beulich wrote: >> On 06.04.2022 11:34, Roger Pau Monné wrote: >>> On Wed, Apr 06, 2022 at 10:44:12AM +0200, Jan Beulich wrote: >>>> 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: >>>>>>>> + 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. >>> >>> Hm, the specification states in EFI_EDID_OVERRIDE_PROTOCOL.GetEdid that: >>> >>> "Returns EDID values and attributes that the Video BIOS must use" >> >> I'm tempted to say "We're not the Video BIOS." ;-) > > I think UEFI expects this to be exclusively used by legacy Video BIOS > implementations but not OSes? > >>> And since EFI_EDID_ACTIVE_PROTOCOL will return >>> EFI_EDID_OVERRIDE_PROTOCOL if present it makes me wonder whether it's >>> fine to resort to EFI_EDID_DISCOVERED_PROTOCOL if the problem is not >>> the call itself failing, but Xen failing to parse the result (because >>> of the usage of must in the sentence). >>> >>> I think it's fine to resort to EFI_EDID_DISCOVERED_PROTOCOL if >>> EFI_EDID_ACTIVE_PROTOCOL fails, but it's likely not if the call >>> succeeds but it's Xen the one failing to parse the result. >>> >>>>> 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. >>>> */ >>> >>> I think the comment is fine, but as mentioned above I wonder whether >>> by failing to parse the EDID from EFI_EDID_ACTIVE_PROTOCOL and >>> resorting to EFI_EDID_DISCOVERED_PROTOCOL we could be screwing the >>> system more than by simply failing to get video output, hence I >>> think a more conservative approach might be to just use >>> EFI_EDID_DISCOVERED_PROTOCOL if EFI_EDID_ACTIVE_PROTOCOL fails. >>> >>> As with firmware, this should mostly mimic what others do in order to >>> be on the safe side, so if GrUB does so we should likely follow suit. >> >> But they're careless in other ways; I don't want to mimic that. I would >> assume (or at least hope) that a discovered EDID still fits the system, >> perhaps not as optimally as a subsequently installed override. But then >> I also lack sufficient knowledge on all aspects which EDID may be >> relevant for, so it's all guesswork anyway afaic. > > Yes, I'm afraid I don't have any more insight. I'm slightly concerned > about this, but I guess not so much as in to block the change: > > Acked-by: Roger Pau Monné <roger....@citrix.com>
Thanks. > I would word the comment as: > > /* > * 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. > * > * Note that attempting to use the information in > * EFI_EDID_DISCOVERED_PROTOCOL when there's an override provided by > * EFI_EDID_ACTIVE_PROTOCOL could lead to issues. > */ I've extended it like this. Jan
