diff mbox

[RFC] PCI: Extend boot_vga sysfs attribute lookup to fix X on MBA+EFI

Message ID 20131125205441.5046e0cb@neptune.home
State Not Applicable
Headers show

Commit Message

Bruno Prémont Nov. 25, 2013, 7:54 p.m. UTC 
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.

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().

Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
---
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(+)

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Bjorn Helgaas Nov. 25, 2013, 8:36 p.m. UTC | #1 
On Mon, Nov 25, 2013 at 12:54 PM, Bruno Prémont
<bonbons@linux-vserver.org> 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?

> 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().

> Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> ---
> 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 <linux/pci-aspm.h>
>  #include <linux/slab.h>
>  #include <linux/vgaarb.h>
> +#include <linux/screen_info.h>
>  #include <linux/pm_runtime.h>
>  #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));
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bruno Prémont Nov. 25, 2013, 9:04 p.m. UTC | #2 
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 <bonbons@linux-vserver.org>
> > ---
> > 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 <linux/pci-aspm.h>
> >  #include <linux/slab.h>
> >  #include <linux/vgaarb.h>
> > +#include <linux/screen_info.h>
> >  #include <linux/pm_runtime.h>
> >  #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));
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Herrmann Nov. 27, 2013, 8:40 p.m. UTC | #3 
Hi

On Mon, Nov 25, 2013 at 8:54 PM, Bruno Prémont
<bonbons@linux-vserver.org> 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.
>
> 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().
>
> Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> ---
> 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.

How about moving the code from efifb to arch/x86/kernel/sysfb_efi.c?
efifb is x86 only so we don't break anything by this. And all the
other efi-quirks have already been moved. Imho this would be the
easiest fix right now. But if you can spend some time to clean up the
vga_default_device() mess, please go ahead.

Btw., thanks for tracking this down. It bothered me for quite some
while that Xorg ignores my cards if I boot via efi..

Thanks
David

>
>  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 <linux/pci-aspm.h>
>  #include <linux/slab.h>
>  #include <linux/vgaarb.h>
> +#include <linux/screen_info.h>
>  #include <linux/pm_runtime.h>
>  #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));
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bruno Prémont Nov. 28, 2013, 7:22 a.m. UTC | #4 
Hi David,

On Wed, 27 Nov 2013 21:40:39 +0100 David Herrmann wrote:
> On Mon, Nov 25, 2013 at 8: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.
> >
> > 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().
> >
> > Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> > ---
> > 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.
> 
> How about moving the code from efifb to arch/x86/kernel/sysfb_efi.c?
> efifb is x86 only so we don't break anything by this. And all the
> other efi-quirks have already been moved. Imho this would be the
> easiest fix right now. But if you can spend some time to clean up the
> vga_default_device() mess, please go ahead.

Well, I don't know how things work on other arches where GPU is
a PCI device...

Possibly the issue exists for any arch whose firmware does not cause
  !!(pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW)
to be 1 and for which there is no magic vga_default_device()
re-implementation.

That's the reason why I don't think moving it to any arch/firmware
place is such a good idea.


I will cook the vga_default_device() cleanup over the weekend and
post a follow-up patch by then.

> Btw., thanks for tracking this down. It bothered me for quite some
> while that Xorg ignores my cards if I boot via efi..

I already hit it a year ago or so and did hide it by keeping
efifb enabled on the MBA (had not been looking at the cause by then
though, thinking it was some magic EFI poking needed on MBA as
FB_EFI=n worked fine on a EFI PC system with radeon).

Bruno


> Thanks
> David
> 
> >
> >  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 <linux/pci-aspm.h>
> >  #include <linux/slab.h>
> >  #include <linux/vgaarb.h>
> > +#include <linux/screen_info.h>
> >  #include <linux/pm_runtime.h>
> >  #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));
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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 <linux/pci-aspm.h>
 #include <linux/slab.h>
 #include <linux/vgaarb.h>
+#include <linux/screen_info.h>
 #include <linux/pm_runtime.h>
 #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));