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> 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. */ Thanks, Roger.
