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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.pau@xxxxxxxxxx>

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.