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." ;-) > 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. Jan
