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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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