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

[Jan Beulich <jbeulich@xxxxxxxx>]

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