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

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

[Bruno Prémont]

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

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));

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

[Bjorn Helgaas]

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));

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

[Bruno Prémont]

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));

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

[David Herrmann]

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

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

[Bruno Prémont]

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

[RFC patch v2] x86: Improve boot_vga/vga_default_device() for EFI

[Bruno Prémont]

With commit b4aa0163056b6c70029b6e8619ce07c274351f42 Matthew Garret
introduced a efifb vga_default_device() so that EFI systems that do not
load shadow VBIOS or setup VGA get proper value for boot_vga PCI sysfs
attribute on the corresponding PCI device.

Xorg is refusing to detect devices when boot_vga=0 which is the case
on some EFI system (e.g. MacBookAir2,1). Xorg detects the GPU and finds
the dri device but then bails out with "no devices detected".

With introduction of sysfb/simplefb/simpledrm efifb is getting obsolete
while having native drivers for the GPU also makes selecting sysfb/efifb
optional.

Remove the efifb implementation of vga_default_device() and initialize
vgaarb's vga_default_device() with the PCI GPU that matches boot
screen_info in x86's pci_fixup_video().

Notes:
- Other architectures with PCI GPU might need a similar fixup.
- If CONFIG_VGA_ARB is unset vga_default_device() is only available
  as a stub that returns NULL, making this adjustment insufficient.
  Unsetting CONFIG_VGA_ARB requires CONFIG_EXPERT=y though.

Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
---
 arch/x86/include/asm/vga.h |  6 ------
 arch/x86/pci/fixup.c       | 21 +++++++++++++++++++++
 drivers/video/efifb.c      | 38 --------------------------------------
 3 files changed, 21 insertions(+), 44 deletions(-)

diff --git a/arch/x86/include/asm/vga.h b/arch/x86/include/asm/vga.h
index 44282fb..c4b9dc2 100644
--- a/arch/x86/include/asm/vga.h
+++ b/arch/x86/include/asm/vga.h
@@ -17,10 +17,4 @@
 #define vga_readb(x) (*(x))
 #define vga_writeb(x, y) (*(y) = (x))
 
-#ifdef CONFIG_FB_EFI
-#define __ARCH_HAS_VGA_DEFAULT_DEVICE
-extern struct pci_dev *vga_default_device(void);
-extern void vga_set_default_device(struct pci_dev *pdev);
-#endif
-
 #endif /* _ASM_X86_VGA_H */
diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index f5809fa..440343e 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -6,6 +6,7 @@
 #include <linux/dmi.h>
 #include <linux/pci.h>
 #include <linux/init.h>
+#include <linux/screen_info.h>
 #include <linux/vgaarb.h>
 #include <asm/pci_x86.h>
 
@@ -323,6 +324,26 @@ static void pci_fixup_video(struct pci_dev *pdev)
 	struct pci_bus *bus;
 	u16 config;
 
+	if (!vga_default_device()) {
+		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)
+				vga_set_default_device(pdev);
+		}
+	}
+
 	/* Is VGA routed to us? */
 	bus = pdev->bus;
 	while (bus) {
diff --git a/drivers/video/efifb.c b/drivers/video/efifb.c
index 7f9ff75..fb3fb50 100644
--- a/drivers/video/efifb.c
+++ b/drivers/video/efifb.c
@@ -19,8 +19,6 @@
 
 static bool request_mem_succeeded = false;
 
-static struct pci_dev *default_vga;
-
 static struct fb_var_screeninfo efifb_defined = {
 	.activate		= FB_ACTIVATE_NOW,
 	.height			= -1,
@@ -85,18 +83,6 @@ static struct fb_ops efifb_ops = {
 	.fb_imageblit	= cfb_imageblit,
 };
 
-struct pci_dev *vga_default_device(void)
-{
-	return default_vga;
-}
-
-EXPORT_SYMBOL_GPL(vga_default_device);
-
-void vga_set_default_device(struct pci_dev *pdev)
-{
-	default_vga = pdev;
-}
-
 static int efifb_setup(char *options)
 {
 	char *this_opt;
@@ -127,30 +113,6 @@ static int efifb_setup(char *options)
 		}
 	}
 
-	for_each_pci_dev(dev) {
-		int i;
-
-		if ((dev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
-			continue;
-
-		for (i=0; i < DEVICE_COUNT_RESOURCE; i++) {
-			resource_size_t start, end;
-
-			if (!(pci_resource_flags(dev, i) & IORESOURCE_MEM))
-				continue;
-
-			start = pci_resource_start(dev, i);
-			end  = pci_resource_end(dev, i);
-
-			if (!start || !end)
-				continue;
-
-			if (screen_info.lfb_base >= start &&
-			    (screen_info.lfb_base + screen_info.lfb_size) < end)
-				default_vga = dev;
-		}
-	}
-
 	return 0;
 }
 

Re: [RFC patch v2] x86: Improve boot_vga/vga_default_device() for EFI

[Bjorn Helgaas]

On Sat, Nov 30, 2013 at 6:52 AM, Bruno Prémont
<bonbons@linux-vserver.org> wrote:
> With commit b4aa0163056b6c70029b6e8619ce07c274351f42 Matthew Garret
> introduced a efifb vga_default_device() so that EFI systems that do not
> load shadow VBIOS or setup VGA get proper value for boot_vga PCI sysfs
> attribute on the corresponding PCI device.

I like the fact that this gets back to a single vga_default_device()
implementation.  But of course Matthew could easily have done this to
begin with, so I would defer to his judgment about this.

I also like the fact that you do this in pci_fixup_video(), where
we're already doing similar stuff.  The ia64 version should be changed
the same way (or better yet, consolidated) unless there's some reason
they need to be different.  There is a little "dig" and "hpzx1" gunk
in the ia64 version that could be factored out into some sort of
__weak architecture function.

> Xorg is refusing to detect devices when boot_vga=0 which is the case
> on some EFI system (e.g. MacBookAir2,1). Xorg detects the GPU and finds
> the dri device but then bails out with "no devices detected".
>
> With introduction of sysfb/simplefb/simpledrm efifb is getting obsolete
> while having native drivers for the GPU also makes selecting sysfb/efifb
> optional.
>
> Remove the efifb implementation of vga_default_device() and initialize
> vgaarb's vga_default_device() with the PCI GPU that matches boot
> screen_info in x86's pci_fixup_video().
>
> Notes:
> - Other architectures with PCI GPU might need a similar fixup.
> - If CONFIG_VGA_ARB is unset vga_default_device() is only available
>   as a stub that returns NULL, making this adjustment insufficient.
>   Unsetting CONFIG_VGA_ARB requires CONFIG_EXPERT=y though.
>
> Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> ---
>  arch/x86/include/asm/vga.h |  6 ------
>  arch/x86/pci/fixup.c       | 21 +++++++++++++++++++++
>  drivers/video/efifb.c      | 38 --------------------------------------
>  3 files changed, 21 insertions(+), 44 deletions(-)
>
> diff --git a/arch/x86/include/asm/vga.h b/arch/x86/include/asm/vga.h
> index 44282fb..c4b9dc2 100644
> --- a/arch/x86/include/asm/vga.h
> +++ b/arch/x86/include/asm/vga.h
> @@ -17,10 +17,4 @@
>  #define vga_readb(x) (*(x))
>  #define vga_writeb(x, y) (*(y) = (x))
>
> -#ifdef CONFIG_FB_EFI
> -#define __ARCH_HAS_VGA_DEFAULT_DEVICE
> -extern struct pci_dev *vga_default_device(void);
> -extern void vga_set_default_device(struct pci_dev *pdev);
> -#endif
> -
>  #endif /* _ASM_X86_VGA_H */
> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> index f5809fa..440343e 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -6,6 +6,7 @@
>  #include <linux/dmi.h>
>  #include <linux/pci.h>
>  #include <linux/init.h>
> +#include <linux/screen_info.h>
>  #include <linux/vgaarb.h>
>  #include <asm/pci_x86.h>
>
> @@ -323,6 +324,26 @@ static void pci_fixup_video(struct pci_dev *pdev)
>         struct pci_bus *bus;
>         u16 config;
>
> +       if (!vga_default_device()) {
> +               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)
> +                               vga_set_default_device(pdev);
> +               }
> +       }
> +
>         /* Is VGA routed to us? */
>         bus = pdev->bus;
>         while (bus) {
> diff --git a/drivers/video/efifb.c b/drivers/video/efifb.c
> index 7f9ff75..fb3fb50 100644
> --- a/drivers/video/efifb.c
> +++ b/drivers/video/efifb.c
> @@ -19,8 +19,6 @@
>
>  static bool request_mem_succeeded = false;
>
> -static struct pci_dev *default_vga;
> -
>  static struct fb_var_screeninfo efifb_defined = {
>         .activate               = FB_ACTIVATE_NOW,
>         .height                 = -1,
> @@ -85,18 +83,6 @@ static struct fb_ops efifb_ops = {
>         .fb_imageblit   = cfb_imageblit,
>  };
>
> -struct pci_dev *vga_default_device(void)
> -{
> -       return default_vga;
> -}
> -
> -EXPORT_SYMBOL_GPL(vga_default_device);
> -
> -void vga_set_default_device(struct pci_dev *pdev)
> -{
> -       default_vga = pdev;
> -}
> -
>  static int efifb_setup(char *options)
>  {
>         char *this_opt;
> @@ -127,30 +113,6 @@ static int efifb_setup(char *options)
>                 }
>         }
>
> -       for_each_pci_dev(dev) {
> -               int i;
> -
> -               if ((dev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
> -                       continue;
> -
> -               for (i=0; i < DEVICE_COUNT_RESOURCE; i++) {
> -                       resource_size_t start, end;
> -
> -                       if (!(pci_resource_flags(dev, i) & IORESOURCE_MEM))
> -                               continue;
> -
> -                       start = pci_resource_start(dev, i);
> -                       end  = pci_resource_end(dev, i);
> -
> -                       if (!start || !end)
> -                               continue;
> -
> -                       if (screen_info.lfb_base >= start &&
> -                           (screen_info.lfb_base + screen_info.lfb_size) < end)
> -                               default_vga = dev;
> -               }
> -       }
> -
>         return 0;
>  }
>

Re: [RFC patch v2] x86: Improve boot_vga/vga_default_device() for EFI

[David Herrmann]

Hi

On Sat, Nov 30, 2013 at 2:52 PM, Bruno Prémont
<bonbons@linux-vserver.org> wrote:
> With commit b4aa0163056b6c70029b6e8619ce07c274351f42 Matthew Garret
> introduced a efifb vga_default_device() so that EFI systems that do not
> load shadow VBIOS or setup VGA get proper value for boot_vga PCI sysfs
> attribute on the corresponding PCI device.
>
> Xorg is refusing to detect devices when boot_vga=0 which is the case
> on some EFI system (e.g. MacBookAir2,1). Xorg detects the GPU and finds
> the dri device but then bails out with "no devices detected".
>
> With introduction of sysfb/simplefb/simpledrm efifb is getting obsolete
> while having native drivers for the GPU also makes selecting sysfb/efifb
> optional.
>
> Remove the efifb implementation of vga_default_device() and initialize
> vgaarb's vga_default_device() with the PCI GPU that matches boot
> screen_info in x86's pci_fixup_video().
>
> Notes:
> - Other architectures with PCI GPU might need a similar fixup.
> - If CONFIG_VGA_ARB is unset vga_default_device() is only available
>   as a stub that returns NULL, making this adjustment insufficient.
>   Unsetting CONFIG_VGA_ARB requires CONFIG_EXPERT=y though.
>
> Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> ---
>  arch/x86/include/asm/vga.h |  6 ------
>  arch/x86/pci/fixup.c       | 21 +++++++++++++++++++++
>  drivers/video/efifb.c      | 38 --------------------------------------
>  3 files changed, 21 insertions(+), 44 deletions(-)
>
> diff --git a/arch/x86/include/asm/vga.h b/arch/x86/include/asm/vga.h
> index 44282fb..c4b9dc2 100644
> --- a/arch/x86/include/asm/vga.h
> +++ b/arch/x86/include/asm/vga.h
> @@ -17,10 +17,4 @@
>  #define vga_readb(x) (*(x))
>  #define vga_writeb(x, y) (*(y) = (x))
>
> -#ifdef CONFIG_FB_EFI
> -#define __ARCH_HAS_VGA_DEFAULT_DEVICE
> -extern struct pci_dev *vga_default_device(void);
> -extern void vga_set_default_device(struct pci_dev *pdev);
> -#endif
> -
>  #endif /* _ASM_X86_VGA_H */
> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> index f5809fa..440343e 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -6,6 +6,7 @@
>  #include <linux/dmi.h>
>  #include <linux/pci.h>
>  #include <linux/init.h>
> +#include <linux/screen_info.h>
>  #include <linux/vgaarb.h>
>  #include <asm/pci_x86.h>
>
> @@ -323,6 +324,26 @@ static void pci_fixup_video(struct pci_dev *pdev)
>         struct pci_bus *bus;
>         u16 config;
>
> +       if (!vga_default_device()) {
> +               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)

pci_resource_end() returns the address of the end, not the length, so
we have a double off-by-one error here, don't we?
Shouldn't this be:
                               (screen_info.lfb_base +
screen_info.lfb_size) <= end + 1)

Oh, and lfb_size is in multiples of 0xffff, so it actually needs to be:
                               (screen_info.lfb_base +
((u64)screen_info.lfb_size << 16)) <= end + 1)

I know, it's copy/paste, but we could still fix it here.

> +                               vga_set_default_device(pdev);
> +               }
> +       }
> +
>         /* Is VGA routed to us? */
>         bus = pdev->bus;
>         while (bus) {
> diff --git a/drivers/video/efifb.c b/drivers/video/efifb.c
> index 7f9ff75..fb3fb50 100644
> --- a/drivers/video/efifb.c
> +++ b/drivers/video/efifb.c
> @@ -19,8 +19,6 @@
>
>  static bool request_mem_succeeded = false;
>
> -static struct pci_dev *default_vga;
> -
>  static struct fb_var_screeninfo efifb_defined = {
>         .activate               = FB_ACTIVATE_NOW,
>         .height                 = -1,
> @@ -85,18 +83,6 @@ static struct fb_ops efifb_ops = {
>         .fb_imageblit   = cfb_imageblit,
>  };
>
> -struct pci_dev *vga_default_device(void)
> -{
> -       return default_vga;
> -}
> -
> -EXPORT_SYMBOL_GPL(vga_default_device);
> -
> -void vga_set_default_device(struct pci_dev *pdev)
> -{
> -       default_vga = pdev;
> -}
> -
>  static int efifb_setup(char *options)
>  {
>         char *this_opt;
> @@ -127,30 +113,6 @@ static int efifb_setup(char *options)
>                 }
>         }

I wonder whether we should move the efifb_setup() argument parsing to
x86/kernel/sysfb.c now. Because currently we break simplefb-conversion
and the vga_boot flag if we keep it here.

Otherwise, the patch looks good to me.
Thanks
David

>
> -       for_each_pci_dev(dev) {
> -               int i;
> -
> -               if ((dev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
> -                       continue;
> -
> -               for (i=0; i < DEVICE_COUNT_RESOURCE; i++) {
> -                       resource_size_t start, end;
> -
> -                       if (!(pci_resource_flags(dev, i) & IORESOURCE_MEM))
> -                               continue;
> -
> -                       start = pci_resource_start(dev, i);
> -                       end  = pci_resource_end(dev, i);
> -
> -                       if (!start || !end)
> -                               continue;
> -
> -                       if (screen_info.lfb_base >= start &&
> -                           (screen_info.lfb_base + screen_info.lfb_size) < end)
> -                               default_vga = dev;
> -               }
> -       }
> -
>         return 0;
>  }
>

Re: [RFC patch v2] x86: Improve boot_vga/vga_default_device() for EFI

[David Herrmann]

Hi

On Wed, Dec 18, 2013 at 4:38 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Sat, Nov 30, 2013 at 2:52 PM, Bruno Prémont
> <bonbons@linux-vserver.org> wrote:
>> With commit b4aa0163056b6c70029b6e8619ce07c274351f42 Matthew Garret
>> introduced a efifb vga_default_device() so that EFI systems that do not
>> load shadow VBIOS or setup VGA get proper value for boot_vga PCI sysfs
>> attribute on the corresponding PCI device.
>>
>> Xorg is refusing to detect devices when boot_vga=0 which is the case
>> on some EFI system (e.g. MacBookAir2,1). Xorg detects the GPU and finds
>> the dri device but then bails out with "no devices detected".
>>
>> With introduction of sysfb/simplefb/simpledrm efifb is getting obsolete
>> while having native drivers for the GPU also makes selecting sysfb/efifb
>> optional.
>>
>> Remove the efifb implementation of vga_default_device() and initialize
>> vgaarb's vga_default_device() with the PCI GPU that matches boot
>> screen_info in x86's pci_fixup_video().
>>
>> Notes:
>> - Other architectures with PCI GPU might need a similar fixup.
>> - If CONFIG_VGA_ARB is unset vga_default_device() is only available
>>   as a stub that returns NULL, making this adjustment insufficient.
>>   Unsetting CONFIG_VGA_ARB requires CONFIG_EXPERT=y though.
>>
>> Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
>> ---
>>  arch/x86/include/asm/vga.h |  6 ------
>>  arch/x86/pci/fixup.c       | 21 +++++++++++++++++++++
>>  drivers/video/efifb.c      | 38 --------------------------------------
>>  3 files changed, 21 insertions(+), 44 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/vga.h b/arch/x86/include/asm/vga.h
>> index 44282fb..c4b9dc2 100644
>> --- a/arch/x86/include/asm/vga.h
>> +++ b/arch/x86/include/asm/vga.h
>> @@ -17,10 +17,4 @@
>>  #define vga_readb(x) (*(x))
>>  #define vga_writeb(x, y) (*(y) = (x))
>>
>> -#ifdef CONFIG_FB_EFI
>> -#define __ARCH_HAS_VGA_DEFAULT_DEVICE
>> -extern struct pci_dev *vga_default_device(void);
>> -extern void vga_set_default_device(struct pci_dev *pdev);
>> -#endif
>> -
>>  #endif /* _ASM_X86_VGA_H */
>> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
>> index f5809fa..440343e 100644
>> --- a/arch/x86/pci/fixup.c
>> +++ b/arch/x86/pci/fixup.c
>> @@ -6,6 +6,7 @@
>>  #include <linux/dmi.h>
>>  #include <linux/pci.h>
>>  #include <linux/init.h>
>> +#include <linux/screen_info.h>
>>  #include <linux/vgaarb.h>
>>  #include <asm/pci_x86.h>
>>
>> @@ -323,6 +324,26 @@ static void pci_fixup_video(struct pci_dev *pdev)
>>         struct pci_bus *bus;
>>         u16 config;
>>
>> +       if (!vga_default_device()) {
>> +               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)
>
> pci_resource_end() returns the address of the end, not the length, so
> we have a double off-by-one error here, don't we?
> Shouldn't this be:
>                                (screen_info.lfb_base +
> screen_info.lfb_size) <= end + 1)
>
> Oh, and lfb_size is in multiples of 0xffff, so it actually needs to be:
>                                (screen_info.lfb_base +
> ((u64)screen_info.lfb_size << 16)) <= end + 1)

Oh, just remembered that this is only done for VESA, not EFI.
Awesome.. so the "<< 16" shift is not needed.

David

> I know, it's copy/paste, but we could still fix it here.
>
>> +                               vga_set_default_device(pdev);
>> +               }
>> +       }
>> +
>>         /* Is VGA routed to us? */
>>         bus = pdev->bus;
>>         while (bus) {
>> diff --git a/drivers/video/efifb.c b/drivers/video/efifb.c
>> index 7f9ff75..fb3fb50 100644
>> --- a/drivers/video/efifb.c
>> +++ b/drivers/video/efifb.c
>> @@ -19,8 +19,6 @@
>>
>>  static bool request_mem_succeeded = false;
>>
>> -static struct pci_dev *default_vga;
>> -
>>  static struct fb_var_screeninfo efifb_defined = {
>>         .activate               = FB_ACTIVATE_NOW,
>>         .height                 = -1,
>> @@ -85,18 +83,6 @@ static struct fb_ops efifb_ops = {
>>         .fb_imageblit   = cfb_imageblit,
>>  };
>>
>> -struct pci_dev *vga_default_device(void)
>> -{
>> -       return default_vga;
>> -}
>> -
>> -EXPORT_SYMBOL_GPL(vga_default_device);
>> -
>> -void vga_set_default_device(struct pci_dev *pdev)
>> -{
>> -       default_vga = pdev;
>> -}
>> -
>>  static int efifb_setup(char *options)
>>  {
>>         char *this_opt;
>> @@ -127,30 +113,6 @@ static int efifb_setup(char *options)
>>                 }
>>         }
>
> I wonder whether we should move the efifb_setup() argument parsing to
> x86/kernel/sysfb.c now. Because currently we break simplefb-conversion
> and the vga_boot flag if we keep it here.
>
> Otherwise, the patch looks good to me.
> Thanks
> David
>
>>
>> -       for_each_pci_dev(dev) {
>> -               int i;
>> -
>> -               if ((dev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
>> -                       continue;
>> -
>> -               for (i=0; i < DEVICE_COUNT_RESOURCE; i++) {
>> -                       resource_size_t start, end;
>> -
>> -                       if (!(pci_resource_flags(dev, i) & IORESOURCE_MEM))
>> -                               continue;
>> -
>> -                       start = pci_resource_start(dev, i);
>> -                       end  = pci_resource_end(dev, i);
>> -
>> -                       if (!start || !end)
>> -                               continue;
>> -
>> -                       if (screen_info.lfb_base >= start &&
>> -                           (screen_info.lfb_base + screen_info.lfb_size) < end)
>> -                               default_vga = dev;
>> -               }
>> -       }
>> -
>>         return 0;
>>  }
>>

Re: [RFC patch v2] x86: Improve boot_vga/vga_default_device() for EFI

[Bruno Prémont]

[replaced Matthew's RedHat email with a recently used one]

> > On Sat, Nov 30, 2013 at 2:52 PM, Bruno Prémont wrote:
> >> With commit b4aa0163056b6c70029b6e8619ce07c274351f42 Matthew Garret
> >> introduced a efifb vga_default_device() so that EFI systems that do not
> >> load shadow VBIOS or setup VGA get proper value for boot_vga PCI sysfs
> >> attribute on the corresponding PCI device.

On Mon, 9 December 2013 Bjorn Helgaas wrote
> I like the fact that this gets back to a single vga_default_device()
> implementation.  But of course Matthew could easily have done this to
> begin with, so I would defer to his judgment about this.
>
> I also like the fact that you do this in pci_fixup_video(), where
> we're already doing similar stuff.  The ia64 version should be changed
> the same way (or better yet, consolidated) unless there's some reason
> they need to be different.  There is a little "dig" and "hpzx1" gunk
> in the ia64 version that could be factored out into some sort of
> __weak architecture function.

There is also the difference in looking up the PCI devices that might need
fixup. For x86 the fixups apply only to VGA class, for IA64 on the other
hand the VGA class filter is within pci_fixup_video().
Additionally vga_default_device seems ignored by IA64 as it only sets
IORESOURCE_ROM_SHADOW resource flag.

Though this probably means that IA64's pci_fixup_video() has been less
maintained than x86 variant.

The only sensible place I see for factoring this out is to move the logic
to drivers/gpu/vga/vga_arb.c which is providing vga_default_device() and
just call it from respective arch's pci_fixup_video().

> >> Xorg is refusing to detect devices when boot_vga=0 which is the case
> >> on some EFI system (e.g. MacBookAir2,1). Xorg detects the GPU and finds
> >> the dri device but then bails out with "no devices detected".
> >>
> >> With introduction of sysfb/simplefb/simpledrm efifb is getting obsolete
> >> while having native drivers for the GPU also makes selecting sysfb/efifb
> >> optional.
> >>
> >> Remove the efifb implementation of vga_default_device() and initialize
> >> vgaarb's vga_default_device() with the PCI GPU that matches boot
> >> screen_info in x86's pci_fixup_video().
> >>
> >> Notes:
> >> - Other architectures with PCI GPU might need a similar fixup.
> >> - If CONFIG_VGA_ARB is unset vga_default_device() is only available
> >>   as a stub that returns NULL, making this adjustment insufficient.
> >>   Unsetting CONFIG_VGA_ARB requires CONFIG_EXPERT=y though.
> >>
> >> Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> >> ---
> >>  arch/x86/include/asm/vga.h |  6 ------
> >>  arch/x86/pci/fixup.c       | 21 +++++++++++++++++++++
> >>  drivers/video/efifb.c      | 38 --------------------------------------
> >>  3 files changed, 21 insertions(+), 44 deletions(-)
> >>
> >> diff --git a/arch/x86/include/asm/vga.h b/arch/x86/include/asm/vga.h
> >> index 44282fb..c4b9dc2 100644
> >> --- a/arch/x86/include/asm/vga.h
> >> +++ b/arch/x86/include/asm/vga.h
> >> @@ -17,10 +17,4 @@
> >>  #define vga_readb(x) (*(x))
> >>  #define vga_writeb(x, y) (*(y) = (x))
> >>
> >> -#ifdef CONFIG_FB_EFI
> >> -#define __ARCH_HAS_VGA_DEFAULT_DEVICE
> >> -extern struct pci_dev *vga_default_device(void);
> >> -extern void vga_set_default_device(struct pci_dev *pdev);
> >> -#endif
> >> -
> >>  #endif /* _ASM_X86_VGA_H */
> >> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> >> index f5809fa..440343e 100644
> >> --- a/arch/x86/pci/fixup.c
> >> +++ b/arch/x86/pci/fixup.c
> >> @@ -6,6 +6,7 @@
> >>  #include <linux/dmi.h>
> >>  #include <linux/pci.h>
> >>  #include <linux/init.h>
> >> +#include <linux/screen_info.h>
> >>  #include <linux/vgaarb.h>
> >>  #include <asm/pci_x86.h>
> >>
> >> @@ -323,6 +324,26 @@ static void pci_fixup_video(struct pci_dev *pdev)
> >>         struct pci_bus *bus;
> >>         u16 config;
> >>
> >> +       if (!vga_default_device()) {
> >> +               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)

On Wed, 18 December 2013 David Herrmann wrote:
> On Wed, Dec 18, 2013 at 4:38 PM, David Herrmann wrote:
> > pci_resource_end() returns the address of the end, not the length, so
> > we have a double off-by-one error here, don't we?
> > Shouldn't this be:
> >                                (screen_info.lfb_base +
> > screen_info.lfb_size) <= end + 1)

No problem for folding this into the patch, though in case it was useful
for bisecting it might still be preferable to make it a different patch.

> > Oh, and lfb_size is in multiples of 0xffff, so it actually needs to be:
> >                                (screen_info.lfb_base +
> > ((u64)screen_info.lfb_size << 16)) <= end + 1)
> 
> Oh, just remembered that this is only done for VESA, not EFI.
> Awesome.. so the "<< 16" shift is not needed.

Hm, that's annoying if lfb_size is in different units for VESA and EFI!
That would mean that any user of lfb_size would have to know if it came
in via EFI, VESA or maybe some other source?

> David
> 
> > I know, it's copy/paste, but we could still fix it here.
> >
> >> +                               vga_set_default_device(pdev);
> >> +               }
> >> +       }
> >> +
> >>         /* Is VGA routed to us? */
> >>         bus = pdev->bus;
> >>         while (bus) {
> >> diff --git a/drivers/video/efifb.c b/drivers/video/efifb.c
> >> index 7f9ff75..fb3fb50 100644
> >> --- a/drivers/video/efifb.c
> >> +++ b/drivers/video/efifb.c
> >> @@ -19,8 +19,6 @@
> >>
> >>  static bool request_mem_succeeded = false;
> >>
> >> -static struct pci_dev *default_vga;
> >> -
> >>  static struct fb_var_screeninfo efifb_defined = {
> >>         .activate               = FB_ACTIVATE_NOW,
> >>         .height                 = -1,
> >> @@ -85,18 +83,6 @@ static struct fb_ops efifb_ops = {
> >>         .fb_imageblit   = cfb_imageblit,
> >>  };
> >>
> >> -struct pci_dev *vga_default_device(void)
> >> -{
> >> -       return default_vga;
> >> -}
> >> -
> >> -EXPORT_SYMBOL_GPL(vga_default_device);
> >> -
> >> -void vga_set_default_device(struct pci_dev *pdev)
> >> -{
> >> -       default_vga = pdev;
> >> -}
> >> -
> >>  static int efifb_setup(char *options)
> >>  {
> >>         char *this_opt;
> >> @@ -127,30 +113,6 @@ static int efifb_setup(char *options)
> >>                 }
> >>         }
> >
> > I wonder whether we should move the efifb_setup() argument parsing to
> > x86/kernel/sysfb.c now. Because currently we break simplefb-conversion
> > and the vga_boot flag if we keep it here.

Probably yes though we need to be careful what happens first, pci_fixup_video()
or sysfb_init() in order to not just move the code around and still keep
that same broken simplefb-conversion/vga_boot flag for the case when
EFIFB options tune settings.

But also, fb_get_options() requires fbmem.c which might not be available
(once simpledrm enters the game or) when framebuffer support is not being
built (e.g. future when native KMS drivers don't always select KMS_FB_HELPER)
Those options might very well need to be passed via a different cmdline
option than within video=.

> > Otherwise, the patch looks good to me.
> > Thanks
> > David
> >
> >>
> >> -       for_each_pci_dev(dev) {
> >> -               int i;
> >> -
> >> -               if ((dev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
> >> -                       continue;
> >> -
> >> -               for (i=0; i < DEVICE_COUNT_RESOURCE; i++) {
> >> -                       resource_size_t start, end;
> >> -
> >> -                       if (!(pci_resource_flags(dev, i) & IORESOURCE_MEM))
> >> -                               continue;
> >> -
> >> -                       start = pci_resource_start(dev, i);
> >> -                       end  = pci_resource_end(dev, i);
> >> -
> >> -                       if (!start || !end)
> >> -                               continue;
> >> -
> >> -                       if (screen_info.lfb_base >= start &&
> >> -                           (screen_info.lfb_base + screen_info.lfb_size) < end)
> >> -                               default_vga = dev;
> >> -               }
> >> -       }
> >> -
> >>         return 0;
> >>  }
> >>

[RFC patch v3] x86: Improve boot_vga/vga_default_device() for EFI

[Bruno Prémont]

With commit b4aa0163056b6c70029b6e8619ce07c274351f42 Matthew Garret
introduced a efifb vga_default_device() so that EFI systems that do not
load shadow VBIOS or setup VGA get proper value for boot_vga PCI sysfs
attribute on the corresponding PCI device.

Xorg is refusing to detect devices when boot_vga=0 which is the case on
some EFI system (e.g. MacBookAir2,1). Xorg detects the GPU and finds
the dri device but then bails out with "no devices detected".

With introduction of sysfb/simplefb/simpledrm efifb is getting obsolete
while having native drivers for the GPU also makes selecting
sysfb/efifb optional.

Remove the efifb implementation of vga_default_device() and initialize
vgaarb's vga_default_device() with the PCI GPU that matches boot
screen_info in pci_fixup_video() [x86 and ia64 pci_fixup_video merged
into common function in vgaarb.c].

Other architectures with PCI GPU might need a similar fixup.

Note: If CONFIG_VGA_ARB is unset vga_default_device() is only available
      as a stub that returns NULL, making this adjustment insufficient.
      In addition, with the merge of x86/ia64 fixup code, this would
      also result in disabled fixup.
      Unsetting CONFIG_VGA_ARB requires CONFIG_EXPERT=y though.

Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
---
 arch/ia64/pci/fixup.c      | 56 ++++-----------------------------------------
 arch/x86/include/asm/vga.h |  6 -----
 arch/x86/pci/fixup.c       | 53 +-----------------------------------------
 drivers/gpu/vga/vgaarb.c   | 57 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/video/efifb.c      | 38 -------------------------------
 include/linux/vgaarb.h     | 37 ++++++++++++++++++++++++++++++
 6 files changed, 99 insertions(+), 148 deletions(-)

diff --git a/arch/ia64/pci/fixup.c b/arch/ia64/pci/fixup.c
index 5dc969d..b036423 100644
--- a/arch/ia64/pci/fixup.c
+++ b/arch/ia64/pci/fixup.c
@@ -5,65 +5,17 @@
 
 #include <linux/pci.h>
 #include <linux/init.h>
+#include <linux/vgaarb.h>
 
 #include <asm/machvec.h>
 
-/*
- * Fixup to mark boot BIOS video selected by BIOS before it changes
- *
- * From information provided by "Jon Smirl" <jonsmirl@gmail.com>
- *
- * The standard boot ROM sequence for an x86 machine uses the BIOS
- * to select an initial video card for boot display. This boot video
- * card will have it's BIOS copied to C0000 in system RAM.
- * IORESOURCE_ROM_SHADOW is used to associate the boot video
- * card with this copy. On laptops this copy has to be used since
- * the main ROM may be compressed or combined with another image.
- * See pci_map_rom() for use of this flag. IORESOURCE_ROM_SHADOW
- * is marked here since the boot video device will be the only enabled
- * video device at this point.
- */
-
-static void pci_fixup_video(struct pci_dev *pdev)
+static void pci_ia64_fixup_video(struct pci_dev *pdev)
 {
-	struct pci_dev *bridge;
-	struct pci_bus *bus;
-	u16 config;
-
 	if ((strcmp(ia64_platform_name, "dig") != 0)
 	    && (strcmp(ia64_platform_name, "hpzx1")  != 0))
 		return;
 	/* Maybe, this machine supports legacy memory map. */
 
-	if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
-		return;
-
-	/* Is VGA routed to us? */
-	bus = pdev->bus;
-	while (bus) {
-		bridge = bus->self;
-
-		/*
-		 * From information provided by
-		 * "David Miller" <davem@davemloft.net>
-		 * The bridge control register is valid for PCI header
-		 * type BRIDGE, or CARDBUS. Host to PCI controllers use
-		 * PCI header type NORMAL.
-		 */
-		if (bridge
-		    &&((bridge->hdr_type == PCI_HEADER_TYPE_BRIDGE)
-		       ||(bridge->hdr_type == PCI_HEADER_TYPE_CARDBUS))) {
-			pci_read_config_word(bridge, PCI_BRIDGE_CONTROL,
-						&config);
-			if (!(config & PCI_BRIDGE_CTL_VGA))
-				return;
-		}
-		bus = bus->parent;
-	}
-	pci_read_config_word(pdev, PCI_COMMAND, &config);
-	if (config & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
-		pdev->resource[PCI_ROM_RESOURCE].flags |= IORESOURCE_ROM_SHADOW;
-		dev_printk(KERN_DEBUG, &pdev->dev, "Boot video device\n");
-	}
+	pci_fixup_video(pdev);
 }
-DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, pci_fixup_video);
+DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_ANY_ID, PCI_ANY_ID, PCI_CLASS_DISPLAY_VGA, 8, pci_ia64_fixup_video);
diff --git a/arch/x86/include/asm/vga.h b/arch/x86/include/asm/vga.h
index 44282fb..c4b9dc2 100644
--- a/arch/x86/include/asm/vga.h
+++ b/arch/x86/include/asm/vga.h
@@ -17,10 +17,4 @@
 #define vga_readb(x) (*(x))
 #define vga_writeb(x, y) (*(y) = (x))
 
-#ifdef CONFIG_FB_EFI
-#define __ARCH_HAS_VGA_DEFAULT_DEVICE
-extern struct pci_dev *vga_default_device(void);
-extern void vga_set_default_device(struct pci_dev *pdev);
-#endif
-
 #endif /* _ASM_X86_VGA_H */
diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index bca9e85..22a4fbe 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -302,58 +302,7 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_MCH_PB1,	pcie_r
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_MCH_PC,	pcie_rootport_aspm_quirk);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_MCH_PC1,	pcie_rootport_aspm_quirk);
 
-/*
- * Fixup to mark boot BIOS video selected by BIOS before it changes
- *
- * From information provided by "Jon Smirl" <jonsmirl@gmail.com>
- *
- * The standard boot ROM sequence for an x86 machine uses the BIOS
- * to select an initial video card for boot display. This boot video
- * card will have it's BIOS copied to C0000 in system RAM.
- * IORESOURCE_ROM_SHADOW is used to associate the boot video
- * card with this copy. On laptops this copy has to be used since
- * the main ROM may be compressed or combined with another image.
- * See pci_map_rom() for use of this flag. IORESOURCE_ROM_SHADOW
- * is marked here since the boot video device will be the only enabled
- * video device at this point.
- */
-
-static void pci_fixup_video(struct pci_dev *pdev)
-{
-	struct pci_dev *bridge;
-	struct pci_bus *bus;
-	u16 config;
-
-	/* Is VGA routed to us? */
-	bus = pdev->bus;
-	while (bus) {
-		bridge = bus->self;
-
-		/*
-		 * From information provided by
-		 * "David Miller" <davem@davemloft.net>
-		 * The bridge control register is valid for PCI header
-		 * type BRIDGE, or CARDBUS. Host to PCI controllers use
-		 * PCI header type NORMAL.
-		 */
-		if (bridge
-		    && ((bridge->hdr_type == PCI_HEADER_TYPE_BRIDGE)
-		       || (bridge->hdr_type == PCI_HEADER_TYPE_CARDBUS))) {
-			pci_read_config_word(bridge, PCI_BRIDGE_CONTROL,
-						&config);
-			if (!(config & PCI_BRIDGE_CTL_VGA))
-				return;
-		}
-		bus = bus->parent;
-	}
-	pci_read_config_word(pdev, PCI_COMMAND, &config);
-	if (config & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
-		pdev->resource[PCI_ROM_RESOURCE].flags |= IORESOURCE_ROM_SHADOW;
-		dev_printk(KERN_DEBUG, &pdev->dev, "Boot video device\n");
-		if (!vga_default_device())
-			vga_set_default_device(pdev);
-	}
-}
+/* pci_fixup_video shared in vgaarb.c */
 DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
 				PCI_CLASS_DISPLAY_VGA, 8, pci_fixup_video);
 
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index af02597..3ab9e35 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -149,6 +149,63 @@ void vga_set_default_device(struct pci_dev *pdev)
 }
 #endif
 
+void pci_fixup_video(struct pci_dev *pdev)
+{
+	struct pci_dev *bridge;
+	struct pci_bus *bus;
+	u16 config;
+
+	if (!vga_default_device()) {
+		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)
+				vga_set_default_device(pdev);
+		}
+	}
+
+	/* Is VGA routed to us? */
+	bus = pdev->bus;
+	while (bus) {
+		bridge = bus->self;
+
+		/*
+		 * From information provided by
+		 * "David Miller" <davem@davemloft.net>
+		 * The bridge control register is valid for PCI header
+		 * type BRIDGE, or CARDBUS. Host to PCI controllers use
+		 * PCI header type NORMAL.
+		 */
+		if (bridge
+		    && ((bridge->hdr_type == PCI_HEADER_TYPE_BRIDGE)
+		       || (bridge->hdr_type == PCI_HEADER_TYPE_CARDBUS))) {
+			pci_read_config_word(bridge, PCI_BRIDGE_CONTROL,
+						&config);
+			if (!(config & PCI_BRIDGE_CTL_VGA))
+				return;
+		}
+		bus = bus->parent;
+	}
+	pci_read_config_word(pdev, PCI_COMMAND, &config);
+	if (config & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
+		pdev->resource[PCI_ROM_RESOURCE].flags |= IORESOURCE_ROM_SHADOW;
+		dev_printk(KERN_DEBUG, &pdev->dev, "Boot video device\n");
+		if (!vga_default_device())
+			vga_set_default_device(pdev);
+	}
+}
+
 static inline void vga_irq_set_state(struct vga_device *vgadev, bool state)
 {
 	if (vgadev->irq_set_state)
diff --git a/drivers/video/efifb.c b/drivers/video/efifb.c
index cd7c0df..954465e 100644
--- a/drivers/video/efifb.c
+++ b/drivers/video/efifb.c
@@ -19,8 +19,6 @@
 
 static bool request_mem_succeeded = false;
 
-static struct pci_dev *default_vga;
-
 static struct fb_var_screeninfo efifb_defined = {
 	.activate		= FB_ACTIVATE_NOW,
 	.height			= -1,
@@ -85,18 +83,6 @@ static struct fb_ops efifb_ops = {
 	.fb_imageblit	= cfb_imageblit,
 };
 
-struct pci_dev *vga_default_device(void)
-{
-	return default_vga;
-}
-
-EXPORT_SYMBOL_GPL(vga_default_device);
-
-void vga_set_default_device(struct pci_dev *pdev)
-{
-	default_vga = pdev;
-}
-
 static int efifb_setup(char *options)
 {
 	char *this_opt;
@@ -127,30 +113,6 @@ static int efifb_setup(char *options)
 		}
 	}
 
-	for_each_pci_dev(dev) {
-		int i;
-
-		if ((dev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
-			continue;
-
-		for (i=0; i < DEVICE_COUNT_RESOURCE; i++) {
-			resource_size_t start, end;
-
-			if (!(pci_resource_flags(dev, i) & IORESOURCE_MEM))
-				continue;
-
-			start = pci_resource_start(dev, i);
-			end  = pci_resource_end(dev, i);
-
-			if (!start || !end)
-				continue;
-
-			if (screen_info.lfb_base >= start &&
-			    (screen_info.lfb_base + screen_info.lfb_size) < end)
-				default_vga = dev;
-		}
-	}
-
 	return 0;
 }
 
diff --git a/include/linux/vgaarb.h b/include/linux/vgaarb.h
index 2c02f3a..6518460 100644
--- a/include/linux/vgaarb.h
+++ b/include/linux/vgaarb.h
@@ -162,6 +162,43 @@ extern void vga_put(struct pci_dev *pdev, unsigned int rsrc);
 #define vga_put(pdev, rsrc)
 #endif
 
+/**
+ *     pci_fixup_video
+ *
+ *     This can be called by arch PCI to fixup boot VGA tagging
+ *     of VGA PCI devices (e.g. for X86, IA64)
+ *
+ *     This code was initially spread/duplicated across:
+ *     - X86 PCI-fixup
+ *     - IA64 PCI-fixup
+ *     - EFI_FB
+ *
+ *     * PCI-fixup part:
+ *     Fixup to mark boot BIOS video selected by BIOS before it changes
+ *
+ *     From information provided by "Jon Smirl" <jonsmirl@gmail.com>
+ *
+ *     The standard boot ROM sequence for an x86 machine uses the BIOS
+ *     to select an initial video card for boot display. This boot video
+ *     card will have it's BIOS copied to C0000 in system RAM.
+ *     IORESOURCE_ROM_SHADOW is used to associate the boot video
+ *     card with this copy. On laptops this copy has to be used since
+ *     the main ROM may be compressed or combined with another image.
+ *     See pci_map_rom() for use of this flag. IORESOURCE_ROM_SHADOW
+ *     is marked here since the boot video device will be the only enabled
+ *     video device at this point.
+ *
+ *     * EFI_FB part:
+ *     Some EFI-based system (e.g. Intel-Macs from Apple) do not setup
+ *     shadow Video-BIOS and thus can only be detected by framebuffer
+ *     IO memory range. Flag the corresponding GPU as boot_vga.
+ */
+
+#if defined(CONFIG_VGA_ARB)
+void pci_fixup_video(struct pci_dev *pdev);
+#else
+static inline void pci_fixup_video(struct pci_dev *pdev) { }
+#endif
 
 /**
  *     vga_default_device