From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from hygieia.santi-shop.eu ([78.46.175.2]:51379 "EHLO hygieia.santi-shop.eu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751510Ab3KYVEX convert rfc822-to-8bit (ORCPT ); Mon, 25 Nov 2013 16:04:23 -0500 Date: Mon, 25 Nov 2013 22:04:19 +0100 From: Bruno =?UTF-8?B?UHLDqW1vbnQ=?= To: Bjorn Helgaas Cc: dri-devel@lists.freedesktop.org, Linux PCI , Peter Jones Subject: Re: [RFC patch] PCI: Extend boot_vga sysfs attribute lookup to fix X on MBA+EFI Message-ID: <20131125220419.302ccc54@neptune.home> In-Reply-To: References: <20131125205441.5046e0cb@neptune.home> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Sender: linux-pci-owner@vger.kernel.org List-ID: On Mon, 25 November 2013 Bjorn Helgaas wrote: > On Mon, Nov 25, 2013 at 12:54 PM, Bruno Prémont wrote: > > On a MacBookAir2,1, booting to Linux with EFI though having > > no efifb built-in Xorg refuses to start with "no devices detected" > > because for the only VGA device available (NVidia Geforce 9400M) > > the sysfs attribute boot_vga is zero (instead of expected 1). > > > > When CONFIG_FB_EFI is selected, efifb does provide its own > > vga_default_device() to report the PCI device matching global > > screen_info as determined during efifb setup. > > > > Otherwise there is just a dummy or VGA_ARB's vga_default_device() > > that does not provide the right information. > > Wouldn't it be cleaner to fix vga_default_device() so it returns the > correct thing even when CONFIG_FB_EFI=n? I would rather completely drop the vga_default_device() from efifb (CONFIG_FB_EFI) and just keep vga_default_device() for vga-arbitration/vga-switcheroo. The fact that there are currently *two* instances of that function doesn't make life easy for determining who is providing it and when. drivers/gpu/vga/vgaarb.c:135 drivers/video/efifb.c:88 include/linux/vgaarb.h:190 (dummy) > > On the other hand, boot_vga_show() falls back to poking PCI > > resources to flag a PCI device as boot_vga if vga_default_device() > > returned no PCI device (NULL). > > > > To complement this PCI resource poking, this patch copies the > > validation code used to determine which PCI device to report as > > default VGA device by efifb into boot_vga_show(). > > If we do have to use logic like this, I'd like it better if it were > factored into a single function called both here and from > efifb_setup(). As of above, I would preferably drop that part of code from efifb_setup() and have the logic called only in one place and each time the same way. Otherwise efifb versus x86_sysfb+simplefb or directly going to native driver (nvoueau/radeon/...) behave differently without reason. Bruno > > Signed-off-by: Bruno Prémont > > --- > > Would it make sense to kill the corresponding code from efifb > > as it covers only a single case? > > > > The other EFI capable system I have (AMD Ilano based, Gigabyte > > mainboard does report boot_vga=1, possibly through the resources > > poking and there Xorg starts properly without efifb built in. > > > > Selecting CONFIG_X86_SYSFB (combined with CONFIG_FB_SIMPLE) does > > not help by itself, patching that one instead of PCI's boot_vga > > attribute directly would still not cover the case when neither > > of them is enabled. > > > > > > drivers/pci/pci-sysfs.c | 21 +++++++++++++++++++++ > > 1 file changed, 21 insertions(+) > > > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > > index 7128cfd..91cac71 100644 > > --- a/drivers/pci/pci-sysfs.c > > +++ b/drivers/pci/pci-sysfs.c > > @@ -28,6 +28,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include "pci.h" > > > > @@ -540,6 +541,26 @@ boot_vga_show(struct device *dev, struct device_attribute *attr, char *buf) > > if (vga_dev) > > return sprintf(buf, "%u\n", (pdev == vga_dev)); > > > > + if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA) { > > + resource_size_t start, end; > > + int i; > > + > > + for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) { > > + if (!(pci_resource_flags(pdev, i) & IORESOURCE_MEM)) > > + continue; > > + > > + start = pci_resource_start(pdev, i); > > + end = pci_resource_end(pdev, i); > > + > > + if (!start || !end) > > + continue; > > + > > + if (screen_info.lfb_base >= start && > > + (screen_info.lfb_base + screen_info.lfb_size) < end) > > + return sprintf(buf, "1\n"); > > + } > > + } > > + > > return sprintf(buf, "%u\n", > > !!(pdev->resource[PCI_ROM_RESOURCE].flags & > > IORESOURCE_ROM_SHADOW));