[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Wed, 6 Apr 2022 16:14:30 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=R9fw/IHbr+tKaykx1VXn/RWztIvkRlufgpoz8UMrnlE=; b=Ezb9j+tiq3bM7/Mhz/zVZO5EMQQKbzZmRiEm9fBoLqP68hipBZG9dSSW1Jgwf5q3w5qHJ2ZwcDFVsqxzCmH9QFKn+gZt5h5gi+PQbTFHlP+Fc6aJu7L1UdNEB68EFyHbyvXRawmGzWeubLlpckKTvvXYOm/4VbtPmLpqzwmmH3LnSMAd91nphU2lD46IfVUJMS3dPRYsAlxxAekW6Ng5BfSDEt/YSfK71umykvTiOKCo6cNz15fEGNhhDmELo5F6bzmZuaOVcAaKqs0YRAASuhT2eaocjV+0iXUgXHyg31jW+5RTXQD7uE4/7pRUU79hYVzYsbXJ0F84RKncNGpL1Q==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=JhGMfPaDQnW1x6OZ1bszA/6/V61BVHXhOS9DLtUf2JK3OuJe79R15FaQ9n880/7ac9HbCcH9TtwH/LxTh8KppQ4ncFrrrRuhxeKjEEuU3SSCohu6grYBEKboAsFUQug55PQCia0gxJbbEpUUtdCqLXGEuWnpo4EJiiqHCJ+uulQKNbARnK3mVTb5v6WlRjqmRX1dGZzOrLN8ooTsUIutR9ExB+ZA2vTv7pNoS6GO16O3rbJvCZvClJzOAAFUmbb8MmWXzXSc9t/VxcBpfByBMWheC/MJzrF7nFVvowLij9lHqnKsD+gkEkj+dfCGu4n8A8wp2jSaFN2OszmDLjfWsQ==
  • Authentication-results: esa2.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>
  • Delivery-date: Wed, 06 Apr 2022 14:14:58 +0000
  • Ironport-data: A9a23:fmsShaNUKJ+0xTLvrR1Ql8FynXyQoLVcMsEvi/4bfWQNrUon0TxSx 2tLDT2Ba/eIYzHzLosjPdywpkxSvMLTmNBgHgto+SlhQUwRpJueD7x1DKtR0wB+jCHnZBg6h ynLQoCYdKjYdleF+lH1dOKJQUBUjclkfJKlYAL/En03FFcMpBsJ00o5wbZl2tAw27BVPivW0 T/Mi5yHULOa82Yc3lI8s8pvfzs24ZweEBtB1rAPTagjUG32zhH5P7pGTU2FFFPqQ5E8IwKPb 72rIIdVXI/u10xF5tuNyt4Xe6CRK1LYFVDmZnF+A8BOjvXez8CbP2lS2Pc0MC9qZzu1c99Zi /5V7KGIdCQVFLDpvLs6ThtlSD5FFPgTkFPHCSDXXc27ykTHdz3nwul0DVFwNoodkgp1KTgQr 7pCcmlLN03dwbLtqF64YrAEasALNs7kMZlZonh95TrYEewnUdbIRKCiCdpwgmdp2Z4SQa62i 8wxaRs2dhLwSCZ1InwTE8kZw82M1nuiSmgNwL6SjfVuuDWCpOBr65D2K8bccNGOQcRTn26bq 3jA8mC/BQsVXPSBzj6C/mOpl/X4lyrxU4IPF5W17vdvxlaUwwQ7AhAMSUGyp/X/j0ekQs9eM GQd4C9opq83nGSpU938UhuQsHOC+BkGVLJ4Eec39QWMwar8+BuCCy4PSTspQMwrsoo6SCIn0 neNnsj1Hnp/vbuNU3Wf+7yI6zSoNkA9MmsqdSICCwwf7LHeTJob10yVCIw5Sejs04OzSWqYL y22QDYWxJ4L1PMz6oaH103ip22Lu5r0FREOz1CCNo661T9RaImgbo2uzFHU6/dcMYqUJmW8U Gg4d9u2t75XU8zU/MCZaKBURezyua7ZWNHJqQQ3d6TN4QhB7JJKkWp4xDhlbHlkPc8fEdMCS B+C4FgBjHO/0ZbDUEOWX25TI5lypUQDPY68PhwxUjaoSsIvHONg1HszDXN8J0i3zCARfVgXY P93i/qEA3cAErhAxzGrXeob2rJD7nlgmTKKFcyrn0T3juH2iJuppVEtagXmggcRtv3sneko2 4wHa5viJ+t3DoUSnRU7AaZMdAtXfBDX9Lj9qtBNd/7rH+aVMDpJNhMl+pt4I9YNt/0Mzo/gp yjhMmcFmAuXrSCWcm2iNyE8AI4DqL4i9BrXywR3Zg32s5XiCK7yhJoim2wfJuB9rrE8lKIqF JHouayoW5xyd9gOwBxEBbHVp41+bhW7wwWIOiuuej8keJB8AQfO/7fZksHHrkHi0gLfWRMCn oCd
  • Ironport-hdrordr: A9a23:/igIsqhHr2gj3lHreVeZ3ltJ4XBQXukji2hC6mlwRA09TyXPrb HWoB19726QtN9xYgBDpTjjUJPrfZqyz/NICOUqUYtKPzOW21dATrsC0WK4+UyHJ8SWzIc0vp uIFZIQNDSaNzhHZKjBjjVRvLwbsaG6GAzDv5a785/NJzsaDJ1d0w==
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.