[Patch] x86, ia64, efifb: Move boot_vga fixup from pci to vgaarb

[Patch] x86, ia64, efifb: Move boot_vga fixup from pci to vgaarb

[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 autodetect 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>
---
This is ported to changes in pci/fixup.c that landed in 3.15-rcs.

Is this fine to go in, if so who will take it?

I got no feedback on my last respin (on top of 3.14 a couple of weeks ago,
which added moving the ia64 pci boot_vga fixup).
I've been running this revision for a week or so on 3.15-rc kernels here
(mostly EFI-enabled system, the mentioned MBA as wells as non-Apple systems
where the patch was not required).


 arch/ia64/pci/fixup.c       | 56 ++-------------------------------
 arch/x86/include/asm/vga.h  |  6 ----
 arch/x86/pci/fixup.c        | 55 +--------------------------------
 drivers/gpu/vga/vgaarb.c    | 75 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/video/fbdev/efifb.c | 38 -----------------------
 include/linux/vgaarb.h      | 37 ++++++++++++++++++++++
 6 files changed, 116 insertions(+), 151 deletions(-)

diff --git a/arch/ia64/pci/fixup.c b/arch/ia64/pci/fixup.c
index eee069a..5df22f9 100644
--- a/arch/ia64/pci/fixup.c
+++ b/arch/ia64/pci/fixup.c
@@ -9,64 +9,14 @@
 
 #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. Before marking the device
- * with IORESOURCE_ROM_SHADOW check if a vga_default_device is already set
- * by either arch cde or vga-arbitration, if so only apply the fixup to this
- * already determined primary video card.
- */
-
-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. */
 
-	/* 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;
-	}
-	if (!vga_default_device() || pdev == vga_default_device()) {
-		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");
-			vga_set_default_device(pdev);
-		}
-	}
+	pci_fixup_video(pdev);
 }
 DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
-				PCI_CLASS_DISPLAY_VGA, 8, pci_fixup_video);
+				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 94ae9ae..b599847 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -302,60 +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. Before marking the device
- * with IORESOURCE_ROM_SHADOW check if a vga_default_device is already set
- * by either arch cde or vga-arbitration, if so only apply the fixup to this
- * already determined primary video card.
- */
-
-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;
-	}
-	if (!vga_default_device() || pdev == vga_default_device()) {
-		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");
-			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..f0fbdf6 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -149,6 +149,81 @@ void vga_set_default_device(struct pci_dev *pdev)
 }
 #endif
 
+/*
+ * 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. Before marking the device
+ * with IORESOURCE_ROM_SHADOW check if a vga_default_device is already set
+ * by either arch cde or vga-arbitration, if so only apply the fixup to this
+ * already determined primary video card.
+ */
+
+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;
+	}
+	if (!vga_default_device() || pdev == vga_default_device()) {
+		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");
+			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/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index ae9618f..a033180 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/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,
@@ -84,18 +82,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;
@@ -126,30 +112,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

Re: [Patch] x86, ia64, efifb: Move boot_vga fixup from pci to vgaarb

[Bjorn Helgaas]

On Wed, May 14, 2014 at 10:43:39PM +0200, Bruno Prémont wrote:
> With commit b4aa0163056b6c70029b6e8619ce07c274351f42 Matthew Garret

I think there's an emerging convention to use reference commits as
<12-char-SHA1> ("subject"), i.e.,

    b4aa0163056b ("efifb: Implement vga_default_device() (v2)")

for this case.  Also, s/Garret/Garrett/.

> 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 autodetect 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].

I think it would be good to split this into two patches:

  1) Merge the x86 and ia64 pci_fixup_video() implementations.  This should
  have no functional change at all.

  2) Whatever else you need to actually fix the bug.  This will be much
  smaller, and the actual bug fix will be easier to see.

It would also be nice to have a URL for a bugzilla or mailing list report
of the problem, where there might be dmesg and/or Xorg logs.

This sounds like it might be applicable for the stable kernels.  If so, you
probably should order these so the small bug-fix comes first (even though
this may mean applying the same bugfix for x86 and ia64), and the merge
second.  That way the fix can be applied to the stable kernels without the
merge.

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

I'm not quite sure what this means, or if this is hint that something is
still wrong even with this patch.  Do you mean that if CONFIG_VGA_ARB is
unset, something still doesn't work?

Maybe this will become obvious to me when you split up the patch.

> Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> ---
> This is ported to changes in pci/fixup.c that landed in 3.15-rcs.
> 
> Is this fine to go in, if so who will take it?
> 
> I got no feedback on my last respin (on top of 3.14 a couple of weeks ago,
> which added moving the ia64 pci boot_vga fixup).
> I've been running this revision for a week or so on 3.15-rc kernels here
> (mostly EFI-enabled system, the mentioned MBA as wells as non-Apple systems
> where the patch was not required).
> 
> 
>  arch/ia64/pci/fixup.c       | 56 ++-------------------------------
>  arch/x86/include/asm/vga.h  |  6 ----
>  arch/x86/pci/fixup.c        | 55 +--------------------------------
>  drivers/gpu/vga/vgaarb.c    | 75 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/video/fbdev/efifb.c | 38 -----------------------
>  include/linux/vgaarb.h      | 37 ++++++++++++++++++++++
>  6 files changed, 116 insertions(+), 151 deletions(-)
> 
> diff --git a/arch/ia64/pci/fixup.c b/arch/ia64/pci/fixup.c
> index eee069a..5df22f9 100644
> --- a/arch/ia64/pci/fixup.c
> +++ b/arch/ia64/pci/fixup.c
> @@ -9,64 +9,14 @@
>  
>  #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. Before marking the device
> - * with IORESOURCE_ROM_SHADOW check if a vga_default_device is already set
> - * by either arch cde or vga-arbitration, if so only apply the fixup to this
> - * already determined primary video card.
> - */
> -
> -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. */
>  
> -	/* 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;
> -	}
> -	if (!vga_default_device() || pdev == vga_default_device()) {
> -		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");
> -			vga_set_default_device(pdev);
> -		}
> -	}
> +	pci_fixup_video(pdev);
>  }
>  DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
> -				PCI_CLASS_DISPLAY_VGA, 8, pci_fixup_video);
> +				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 94ae9ae..b599847 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -302,60 +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. Before marking the device
> - * with IORESOURCE_ROM_SHADOW check if a vga_default_device is already set
> - * by either arch cde or vga-arbitration, if so only apply the fixup to this
> - * already determined primary video card.
> - */
> -
> -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;
> -	}
> -	if (!vga_default_device() || pdev == vga_default_device()) {
> -		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");
> -			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..f0fbdf6 100644
> --- a/drivers/gpu/vga/vgaarb.c
> +++ b/drivers/gpu/vga/vgaarb.c
> @@ -149,6 +149,81 @@ void vga_set_default_device(struct pci_dev *pdev)
>  }
>  #endif
>  
> +/*
> + * 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. Before marking the device
> + * with IORESOURCE_ROM_SHADOW check if a vga_default_device is already set
> + * by either arch cde or vga-arbitration, if so only apply the fixup to this
> + * already determined primary video card.
> + */
> +
> +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))) {

I have a patch in my pci/pci_is_bridge branch that will conflict with this,
but I'll take care of sorting that out.  You can continue to base your
patch on v3.15-rc1 or whatever you're using.

> +			pci_read_config_word(bridge, PCI_BRIDGE_CONTROL,
> +						&config);
> +			if (!(config & PCI_BRIDGE_CTL_VGA))
> +				return;
> +		}
> +		bus = bus->parent;
> +	}
> +	if (!vga_default_device() || pdev == vga_default_device()) {
> +		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");
> +			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/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index ae9618f..a033180 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/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,
> @@ -84,18 +82,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;
> @@ -126,30 +112,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.
> + */

I would fold this information into the comment at the function definition.
It looks like pci_fixup_video() is only called as a quirk, so there aren't
going to be random callers that will look for information at this
declaration.

> +
> +#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

Re: [Patch] x86, ia64, efifb: Move boot_vga fixup from pci to vgaarb

[Bruno Prémont]

On Tue, 27 May 2014 Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Wed, May 14, 2014 at 10:43:39PM +0200, Bruno Prémont wrote:
> > With commit b4aa0163056b6c70029b6e8619ce07c274351f42 Matthew Garret
> 
> I think there's an emerging convention to use reference commits as
> <12-char-SHA1> ("subject"), i.e.,
> 
>     b4aa0163056b ("efifb: Implement vga_default_device() (v2)")
> 
> for this case.  Also, s/Garret/Garrett/.

Adjusted

> > 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 autodetect 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].
> 
> I think it would be good to split this into two patches:
> 
>   1) Merge the x86 and ia64 pci_fixup_video() implementations.  This should
>   have no functional change at all.
> 
>   2) Whatever else you need to actually fix the bug.  This will be much
>   smaller, and the actual bug fix will be easier to see.
> 
> It would also be nice to have a URL for a bugzilla or mailing list report
> of the problem, where there might be dmesg and/or Xorg logs.
> 
> This sounds like it might be applicable for the stable kernels.  If so, you
> probably should order these so the small bug-fix comes first (even though
> this may mean applying the same bugfix for x86 and ia64), and the merge
> second.  That way the fix can be applied to the stable kernels without the
> merge.

Split up patches come in reply to this mail

> > 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.
> 
> I'm not quite sure what this means, or if this is hint that something is
> still wrong even with this patch.  Do you mean that if CONFIG_VGA_ARB is
> unset, something still doesn't work?
> 
> Maybe this will become obvious to me when you split up the patch.

This means that if someone unsets CONFIG_VGA_ARB (he need to set CONFIG_EXPERT
to do so) the fixup will not happen at all.

Without fixup the systems that need to set the IORESOURCE_ROM_SHADOW
flag will not have it, and boot_vga pci device attribute will be 0
more often as well.

Moving the unified function somewhere else than vgaarb.c would fix the new
IORESOURCE_ROM_SHADOW fixup dependency on VGA_ARB, though not prevent the
issue I try to fix as vga_default_device() always returns NULL without VGA_ARB.

One possible location would be drivers/pci/quirks.c.

Bruno

> > Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> > ---
> > This is ported to changes in pci/fixup.c that landed in 3.15-rcs.
> > 
> > Is this fine to go in, if so who will take it?
> > 
> > I got no feedback on my last respin (on top of 3.14 a couple of weeks ago,
> > which added moving the ia64 pci boot_vga fixup).
> > I've been running this revision for a week or so on 3.15-rc kernels here
> > (mostly EFI-enabled system, the mentioned MBA as wells as non-Apple systems
> > where the patch was not required).

[PATCH 2/2] x86, ia64: Merge common vga fixup code

[Bruno Prémont]

The fixup code for PCI VGA devices in ia64 and x86 is mostly identical.

Merge it into a single function called from both sides.

The common code is moved to vgaarb.c which makes in dependant on
CONFIG_VGA_ARB.

Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
---
 arch/ia64/pci/fixup.c    | 77 ++----------------------------------------------
 arch/x86/pci/fixup.c     | 76 +----------------------------------------------
 drivers/gpu/vga/vgaarb.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/vgaarb.h   | 37 +++++++++++++++++++++++
 4 files changed, 116 insertions(+), 149 deletions(-)

diff --git a/arch/ia64/pci/fixup.c b/arch/ia64/pci/fixup.c
index 9ed5bef..5df22f9 100644
--- a/arch/ia64/pci/fixup.c
+++ b/arch/ia64/pci/fixup.c
@@ -9,85 +9,14 @@
 
 #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. Before marking the device
- * with IORESOURCE_ROM_SHADOW check if a vga_default_device is already set
- * by either arch cde or vga-arbitration, if so only apply the fixup to this
- * already determined primary video card.
- */
-
-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 (!vga_default_device()) {
-		resource_size_t start, end;
-		int i;
-
-		/* Does firmware framebuffer belong to us? */
-		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;
-	}
-	if (!vga_default_device() || pdev == vga_default_device()) {
-		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");
-			vga_set_default_device(pdev);
-		}
-	}
+	pci_fixup_video(pdev);
 }
 DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
-				PCI_CLASS_DISPLAY_VGA, 8, pci_fixup_video);
+				PCI_CLASS_DISPLAY_VGA, 8, pci_ia64_fixup_video);
diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index 7246cf2..b599847 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -302,81 +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. Before marking the device
- * with IORESOURCE_ROM_SHADOW check if a vga_default_device is already set
- * by either arch cde or vga-arbitration, if so only apply the fixup to this
- * already determined primary video card.
- */
-
-static 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;
-
-		/* Does firmware framebuffer belong to us? */
-		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;
-	}
-	if (!vga_default_device() || pdev == vga_default_device()) {
-		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");
-			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..f0fbdf6 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -149,6 +149,81 @@ void vga_set_default_device(struct pci_dev *pdev)
 }
 #endif
 
+/*
+ * 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. Before marking the device
+ * with IORESOURCE_ROM_SHADOW check if a vga_default_device is already set
+ * by either arch cde or vga-arbitration, if so only apply the fixup to this
+ * already determined primary video card.
+ */
+
+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;
+	}
+	if (!vga_default_device() || pdev == vga_default_device()) {
+		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");
+			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/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
-- 
1.8.5.5

[PATCH 1/2] x86, ia64: Move EFI_FB vga_default_device() initialization to pci_vga_fixup()

[Bruno Prémont]

With commit b4aa0163056b ("efifb: Implement vga_default_device() (v2)")
Matthew Garrett 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".

Note: When vga_default_device() is set boot_vga PCI sysfs attribute
reflects its state. When unset this attribute is 1 whenever
IORESOURCE_ROM_SHADOW flag is set.

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

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

diff --git a/arch/ia64/pci/fixup.c b/arch/ia64/pci/fixup.c
index eee069a..9ed5bef 100644
--- a/arch/ia64/pci/fixup.c
+++ b/arch/ia64/pci/fixup.c
@@ -37,6 +37,27 @@ static void pci_fixup_video(struct pci_dev *pdev)
 		return;
 	/* Maybe, this machine supports legacy memory map. */
 
+	if (!vga_default_device()) {
+		resource_size_t start, end;
+		int i;
+
+		/* Does firmware framebuffer belong to us? */
+		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/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 94ae9ae..7246cf2 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -325,6 +325,27 @@ 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;
+
+		/* Does firmware framebuffer belong to us? */
+		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/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index ae9618f..a033180 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/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,
@@ -84,18 +82,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;
@@ -126,30 +112,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;
 }
 
-- 
1.8.5.5

Re: [PATCH 1/2] x86, ia64: Move EFI_FB vga_default_device() initialization to pci_vga_fixup()

[Bjorn Helgaas]

On Mon, Jun 02, 2014 at 08:19:26PM +0200, Bruno Prémont wrote:
> With commit b4aa0163056b ("efifb: Implement vga_default_device() (v2)")
> Matthew Garrett 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".
> 
> Note: When vga_default_device() is set boot_vga PCI sysfs attribute
> reflects its state. When unset this attribute is 1 whenever
> IORESOURCE_ROM_SHADOW flag is set.
> 
> 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().
> 
> Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> ---
>  arch/ia64/pci/fixup.c       | 21 +++++++++++++++++++++
>  arch/x86/include/asm/vga.h  |  6 ------
>  arch/x86/pci/fixup.c        | 21 +++++++++++++++++++++
>  drivers/video/fbdev/efifb.c | 38 --------------------------------------
>  4 files changed, 42 insertions(+), 44 deletions(-)

Something went wrong here.  It seems like the [2/2] patch should have
been [1/2], and this one should be [2/2].  And this one modifies both
arch/ia64/pci/fixup.c and arch/x86/pci/fixup.c, but the other patch
mostly combines them, so I don't see how this one applies.

And there were unrelated (trivial) changes to these files, so they
need to be rebased to v3.16-rc1.  I'd take care of the rebase, but
I don't understand the other stuff I mentioned.

Bjorn

> diff --git a/arch/ia64/pci/fixup.c b/arch/ia64/pci/fixup.c
> index eee069a..9ed5bef 100644
> --- a/arch/ia64/pci/fixup.c
> +++ b/arch/ia64/pci/fixup.c
> @@ -37,6 +37,27 @@ static void pci_fixup_video(struct pci_dev *pdev)
>  		return;
>  	/* Maybe, this machine supports legacy memory map. */
>  
> +	if (!vga_default_device()) {
> +		resource_size_t start, end;
> +		int i;
> +
> +		/* Does firmware framebuffer belong to us? */
> +		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/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 94ae9ae..7246cf2 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -325,6 +325,27 @@ 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;
> +
> +		/* Does firmware framebuffer belong to us? */
> +		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/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index ae9618f..a033180 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/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,
> @@ -84,18 +82,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;
> @@ -126,30 +112,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;
>  }
>  
> -- 
> 1.8.5.5
> 

Re: [PATCH 1/2] x86, ia64: Move EFI_FB vga_default_device() initialization to pci_vga_fixup()

[Bruno Prémont]

On Tue, 17 Jun 2014 16:35:42 -0600 Bjorn Helgaas wrote:
> On Mon, Jun 02, 2014 at 08:19:26PM +0200, Bruno Prémont wrote:
> > With commit b4aa0163056b ("efifb: Implement vga_default_device() (v2)")
> > Matthew Garrett 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".
> > 
> > Note: When vga_default_device() is set boot_vga PCI sysfs attribute
> > reflects its state. When unset this attribute is 1 whenever
> > IORESOURCE_ROM_SHADOW flag is set.
> > 
> > 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().
> > 
> > Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> > ---
> >  arch/ia64/pci/fixup.c       | 21 +++++++++++++++++++++
> >  arch/x86/include/asm/vga.h  |  6 ------
> >  arch/x86/pci/fixup.c        | 21 +++++++++++++++++++++
> >  drivers/video/fbdev/efifb.c | 38 --------------------------------------
> >  4 files changed, 42 insertions(+), 44 deletions(-)
> 
> Something went wrong here.  It seems like the [2/2] patch should have
> been [1/2], and this one should be [2/2].  And this one modifies both
> arch/ia64/pci/fixup.c and arch/x86/pci/fixup.c, but the other patch
> mostly combines them, so I don't see how this one applies.

I ordered both patches the other way around from your explicit ordering
proposal following the stable kernel suggestion following it.

My patch 1/2 fixes the encountered issue and is the one that may go
stable while my patch 2/2 performs unification of common code.
The unification will need to be placed somewhere else than in vgaarb.c
if one wants to keep current fixup working with CONFIG_VGA_ARB=n.

> And there were unrelated (trivial) changes to these files, so they
> need to be rebased to v3.16-rc1.  I'd take care of the rebase, but
> I don't understand the other stuff I mentioned.

Ok

Bruno

> Bjorn
> 
> > diff --git a/arch/ia64/pci/fixup.c b/arch/ia64/pci/fixup.c
> > index eee069a..9ed5bef 100644
> > --- a/arch/ia64/pci/fixup.c
> > +++ b/arch/ia64/pci/fixup.c
> > @@ -37,6 +37,27 @@ static void pci_fixup_video(struct pci_dev *pdev)
> >  		return;
> >  	/* Maybe, this machine supports legacy memory map. */
> >  
> > +	if (!vga_default_device()) {
> > +		resource_size_t start, end;
> > +		int i;
> > +
> > +		/* Does firmware framebuffer belong to us? */
> > +		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/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 94ae9ae..7246cf2 100644
> > --- a/arch/x86/pci/fixup.c
> > +++ b/arch/x86/pci/fixup.c
> > @@ -325,6 +325,27 @@ 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;
> > +
> > +		/* Does firmware framebuffer belong to us? */
> > +		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/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> > index ae9618f..a033180 100644
> > --- a/drivers/video/fbdev/efifb.c
> > +++ b/drivers/video/fbdev/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,
> > @@ -84,18 +82,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;
> > @@ -126,30 +112,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;
> >  }
> >  
> > -- 
> > 1.8.5.5
> > 

Re: [PATCH 1/2] x86, ia64: Move EFI_FB vga_default_device() initialization to pci_vga_fixup()

[Bruno Prémont]

On Mon, 02 June 2014 Bruno Prémont <bonbons@linux-vserver.org> wrote:
> With commit b4aa0163056b ("efifb: Implement vga_default_device() (v2)")
> Matthew Garrett 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".
> 
> Note: When vga_default_device() is set boot_vga PCI sysfs attribute
> reflects its state. When unset this attribute is 1 whenever
> IORESOURCE_ROM_SHADOW flag is set.
> 
> 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().

Quite a few people trip over this issue, apparently concentrated on
Apple Macs (EFI) with Nvidia graphics.

It has shown up in at least two bugzilla reports (as side-issue):
  https://bugs.freedesktop.org/show_bug.cgi?id=58556#c16
  https://bugs.freedesktop.org/show_bug.cgi?id=76732#c34

My initial report and discussion on freenode, #nouveau:
  http://people.freedesktop.org/~cbrill/dri-log/?channel=nouveau&date=2013-11-23
followed by initial patch proposal:
  http://permalink.gmane.org/gmane.comp.video.dri.devel/95943

>From today on #nouveau:
  Tested-by: Anibal Francisco Martinez Cortina <linuxkid.zeuz@gmail.com>

As reported on #nouveau, every now and then someone trips over this issue
and falls back to enabling efifb so X server accepts to start without
explicit configuration.

It would also be worth to add for first patch:
  Cc: stable@vger.kernel.org

Bruno


> Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> ---
>  arch/ia64/pci/fixup.c       | 21 +++++++++++++++++++++
>  arch/x86/include/asm/vga.h  |  6 ------
>  arch/x86/pci/fixup.c        | 21 +++++++++++++++++++++
>  drivers/video/fbdev/efifb.c | 38 --------------------------------------
>  4 files changed, 42 insertions(+), 44 deletions(-)
> 
> diff --git a/arch/ia64/pci/fixup.c b/arch/ia64/pci/fixup.c
> index eee069a..9ed5bef 100644
> --- a/arch/ia64/pci/fixup.c
> +++ b/arch/ia64/pci/fixup.c
> @@ -37,6 +37,27 @@ static void pci_fixup_video(struct pci_dev *pdev)
>  		return;
>  	/* Maybe, this machine supports legacy memory map. */
>  
> +	if (!vga_default_device()) {
> +		resource_size_t start, end;
> +		int i;
> +
> +		/* Does firmware framebuffer belong to us? */
> +		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/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 94ae9ae..7246cf2 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -325,6 +325,27 @@ 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;
> +
> +		/* Does firmware framebuffer belong to us? */
> +		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/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index ae9618f..a033180 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/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,
> @@ -84,18 +82,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;
> @@ -126,30 +112,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;
>  }
>  

[PATCH 1/2 v2] x86, ia64: Move EFI_FB vga_default_device() initialization to pci_vga_fixup()

[Bruno Prémont]

With commit b4aa0163056b ("efifb: Implement vga_default_device() (v2)")
Matthew Garrett 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".

Note: When vga_default_device() is set boot_vga PCI sysfs attribute
reflects its state. When unset this attribute is 1 whenever
IORESOURCE_ROM_SHADOW flag is set.

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

Tested-by: Anibal Francisco Martinez Cortina <linuxkid.zeuz@gmail.com>
Cc: Matthew Garrett <matthew.garrett@nebula.com>
Cc: stable@vger.kernel.org
Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
---

Changes since v1:
  Added Tested-by, Cc

 arch/ia64/pci/fixup.c       | 21 +++++++++++++++++++++
 arch/x86/include/asm/vga.h  |  6 ------
 arch/x86/pci/fixup.c        | 21 +++++++++++++++++++++
 drivers/video/fbdev/efifb.c | 38 --------------------------------------
 4 files changed, 42 insertions(+), 44 deletions(-)

diff --git a/arch/ia64/pci/fixup.c b/arch/ia64/pci/fixup.c
index eee069a..9ed5bef 100644
--- a/arch/ia64/pci/fixup.c
+++ b/arch/ia64/pci/fixup.c
@@ -37,6 +37,27 @@ static void pci_fixup_video(struct pci_dev *pdev)
 		return;
 	/* Maybe, this machine supports legacy memory map. */
 
+	if (!vga_default_device()) {
+		resource_size_t start, end;
+		int i;
+
+		/* Does firmware framebuffer belong to us? */
+		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/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 94ae9ae..7246cf2 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -325,6 +325,27 @@ 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;
+
+		/* Does firmware framebuffer belong to us? */
+		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/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index ae9618f..a033180 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/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,
@@ -84,18 +82,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;
@@ -126,30 +112,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;
 }
 
-- 
1.8.5.5

[PATCH 2/2 v2] x86, ia64: Merge common vga fixup code

[Bruno Prémont]

The fixup code for PCI VGA devices in ia64 and x86 is mostly identical.

Merge it into a single function called from both sides.

The common code is moved to vgaarb.c which makes in dependant on
CONFIG_VGA_ARB.

Tested-by: Anibal Francisco Martinez Cortina <linuxkid.zeuz@gmail.com>
Cc: Matthew Garrett <matthew.garrett@nebula.com>
Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
---

Changes since v1:
  Added Tested-by, Cc

 arch/ia64/pci/fixup.c    | 77 ++----------------------------------------------
 arch/x86/pci/fixup.c     | 76 +----------------------------------------------
 drivers/gpu/vga/vgaarb.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/vgaarb.h   | 37 +++++++++++++++++++++++
 4 files changed, 116 insertions(+), 149 deletions(-)

diff --git a/arch/ia64/pci/fixup.c b/arch/ia64/pci/fixup.c
index 9ed5bef..5df22f9 100644
--- a/arch/ia64/pci/fixup.c
+++ b/arch/ia64/pci/fixup.c
@@ -9,85 +9,14 @@
 
 #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. Before marking the device
- * with IORESOURCE_ROM_SHADOW check if a vga_default_device is already set
- * by either arch cde or vga-arbitration, if so only apply the fixup to this
- * already determined primary video card.
- */
-
-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 (!vga_default_device()) {
-		resource_size_t start, end;
-		int i;
-
-		/* Does firmware framebuffer belong to us? */
-		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;
-	}
-	if (!vga_default_device() || pdev == vga_default_device()) {
-		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");
-			vga_set_default_device(pdev);
-		}
-	}
+	pci_fixup_video(pdev);
 }
 DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
-				PCI_CLASS_DISPLAY_VGA, 8, pci_fixup_video);
+				PCI_CLASS_DISPLAY_VGA, 8, pci_ia64_fixup_video);
diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index 7246cf2..b599847 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -302,81 +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. Before marking the device
- * with IORESOURCE_ROM_SHADOW check if a vga_default_device is already set
- * by either arch cde or vga-arbitration, if so only apply the fixup to this
- * already determined primary video card.
- */
-
-static 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;
-
-		/* Does firmware framebuffer belong to us? */
-		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;
-	}
-	if (!vga_default_device() || pdev == vga_default_device()) {
-		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");
-			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..f0fbdf6 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -149,6 +149,81 @@ void vga_set_default_device(struct pci_dev *pdev)
 }
 #endif
 
+/*
+ * 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. Before marking the device
+ * with IORESOURCE_ROM_SHADOW check if a vga_default_device is already set
+ * by either arch cde or vga-arbitration, if so only apply the fixup to this
+ * already determined primary video card.
+ */
+
+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;
+	}
+	if (!vga_default_device() || pdev == vga_default_device()) {
+		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");
+			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/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
-- 
1.8.5.5

Re: [PATCH 1/2 v2] x86, ia64: Move EFI_FB vga_default_device() initialization to pci_vga_fixup()

[Matthew Garrett]

T24gV2VkLCAyMDE0LTA2LTI1IGF0IDAwOjU1ICswMjAwLCBCcnVubyBQcsOpbW9udCB3cm90ZToN
Cg0KPiBXaXRoIGludHJvZHVjdGlvbiBvZiBzeXNmYi9zaW1wbGVmYi9zaW1wbGVkcm0gZWZpZmIg
aXMgZ2V0dGluZyBvYnNvbGV0ZQ0KPiB3aGlsZSBoYXZpbmcgbmF0aXZlIGRyaXZlcnMgZm9yIHRo
ZSBHUFUgYWxzbyBtYWtlcyBzZWxlY3RpbmcNCj4gc3lzZmIvZWZpZmIgb3B0aW9uYWwuDQo+IA0K
DQpCb3RoIGxvb2sgZ29vZCB0byBtZS4NCg0KQWNrZWQtYnk6IE1hdHRoZXcgR2FycmV0dCA8bWF0
dGhldy5nYXJyZXR0QG5lYnVsYS5jb20+DQoNCi0tIA0KTWF0dGhldyBHYXJyZXR0IDxtYXR0aGV3
LmdhcnJldHRAbmVidWxhLmNvbT4NCg==

Re: [PATCH 1/2 v2] x86, ia64: Move EFI_FB vga_default_device() initialization to pci_vga_fixup()

[Bjorn Helgaas]

On Wed, Jun 25, 2014 at 12:55:01AM +0200, Bruno Prémont wrote:
> With commit b4aa0163056b ("efifb: Implement vga_default_device() (v2)")
> Matthew Garrett 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".
> 
> Note: When vga_default_device() is set boot_vga PCI sysfs attribute
> reflects its state. When unset this attribute is 1 whenever
> IORESOURCE_ROM_SHADOW flag is set.
> 
> 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().
> 
> Tested-by: Anibal Francisco Martinez Cortina <linuxkid.zeuz@gmail.com>
> Cc: Matthew Garrett <matthew.garrett@nebula.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>

I applied both with Matthew's ack to pci/misc for v3.17, thanks!

> ---
> 
> Changes since v1:
>   Added Tested-by, Cc
> 
>  arch/ia64/pci/fixup.c       | 21 +++++++++++++++++++++
>  arch/x86/include/asm/vga.h  |  6 ------
>  arch/x86/pci/fixup.c        | 21 +++++++++++++++++++++
>  drivers/video/fbdev/efifb.c | 38 --------------------------------------
>  4 files changed, 42 insertions(+), 44 deletions(-)
> 
> diff --git a/arch/ia64/pci/fixup.c b/arch/ia64/pci/fixup.c
> index eee069a..9ed5bef 100644
> --- a/arch/ia64/pci/fixup.c
> +++ b/arch/ia64/pci/fixup.c
> @@ -37,6 +37,27 @@ static void pci_fixup_video(struct pci_dev *pdev)
>  		return;
>  	/* Maybe, this machine supports legacy memory map. */
>  
> +	if (!vga_default_device()) {
> +		resource_size_t start, end;
> +		int i;
> +
> +		/* Does firmware framebuffer belong to us? */
> +		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/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 94ae9ae..7246cf2 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -325,6 +325,27 @@ 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;
> +
> +		/* Does firmware framebuffer belong to us? */
> +		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/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index ae9618f..a033180 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/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,
> @@ -84,18 +82,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;
> @@ -126,30 +112,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;
>  }
>  
> -- 
> 1.8.5.5
> 

Re: [PATCH 1/2 v2] x86, ia64: Move EFI_FB vga_default_device() initialization to pci_vga_fixup()

[Andreas Noever]

On Sat, Jul 5, 2014 at 7:15 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Wed, Jun 25, 2014 at 12:55:01AM +0200, Bruno Prémont wrote:
>> With commit b4aa0163056b ("efifb: Implement vga_default_device() (v2)")
>> Matthew Garrett 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".
>>
>> Note: When vga_default_device() is set boot_vga PCI sysfs attribute
>> reflects its state. When unset this attribute is 1 whenever
>> IORESOURCE_ROM_SHADOW flag is set.
>>
>> 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().
>>
>> Tested-by: Anibal Francisco Martinez Cortina <linuxkid.zeuz@gmail.com>
>> Cc: Matthew Garrett <matthew.garrett@nebula.com>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
>
> I applied both with Matthew's ack to pci/misc for v3.17, thanks!

I just tried to run the latest kernel.  It failed to boot and git
bisect points to this commit (MacBookPro10,1 with Nvidia&Intel
graphics).

The (now removed) code in efifb_setup did always set default_vga, even
if it had already been set earlier. The new code in pci_fixup_video
runs only if vga_default_device() is NULL. Removing the check fixes
the regression.


The following calls to vga_set_default_device are made during boot:

vga_arbiter_add_pci_device -> vga_set_default_device(intel)
pci_fixup_video -> vga_set_default_device(intel) (there are two calls
in pci_fixup_video, this one is the one near "Boot video device")
pci_fixup_video -> vga_set_default_device(nvidia) (from the "Does
firmware framebuffer belong to us?" loop, only if I remove the check)

vga_arbiter_add_pci_device chooses intel simply because it is the
first device. Next pci_fixup_video(intel) sees that it is the default
device, sets the IORESOURCE_ROM_SHADOW flag and calls
vga_set_default_device again. And finally (if the check is removed)
pci_fixup_video(nvidia) sees that it owns the framebuffer and sets
itself as the default device which allows the system to boot again.

Does setting the ROM_SHADOW flag on (possibly) the wrong device have
any effect? Maybe we should also move this whole detection logic to
some earlier stage to make sure that the default device is correct
from the beginning and doesn't change?


>
>> ---
>>
>> Changes since v1:
>>   Added Tested-by, Cc
>>
>>  arch/ia64/pci/fixup.c       | 21 +++++++++++++++++++++
>>  arch/x86/include/asm/vga.h  |  6 ------
>>  arch/x86/pci/fixup.c        | 21 +++++++++++++++++++++
>>  drivers/video/fbdev/efifb.c | 38 --------------------------------------
>>  4 files changed, 42 insertions(+), 44 deletions(-)
>>
>> diff --git a/arch/ia64/pci/fixup.c b/arch/ia64/pci/fixup.c
>> index eee069a..9ed5bef 100644
>> --- a/arch/ia64/pci/fixup.c
>> +++ b/arch/ia64/pci/fixup.c
>> @@ -37,6 +37,27 @@ static void pci_fixup_video(struct pci_dev *pdev)
>>               return;
>>       /* Maybe, this machine supports legacy memory map. */
>>
>> +     if (!vga_default_device()) {
>> +             resource_size_t start, end;
>> +             int i;
>> +
>> +             /* Does firmware framebuffer belong to us? */
>> +             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/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 94ae9ae..7246cf2 100644
>> --- a/arch/x86/pci/fixup.c
>> +++ b/arch/x86/pci/fixup.c
>> @@ -325,6 +325,27 @@ 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;
>> +
>> +             /* Does firmware framebuffer belong to us? */
>> +             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/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
>> index ae9618f..a033180 100644
>> --- a/drivers/video/fbdev/efifb.c
>> +++ b/drivers/video/fbdev/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,
>> @@ -84,18 +82,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;
>> @@ -126,30 +112,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;
>>  }
>>
>> --
>> 1.8.5.5
>>
> --
> 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

Re: [PATCH 1/2 v2] x86, ia64: Move EFI_FB vga_default_device() initialization to pci_vga_fixup()

[Andreas Noever]

On Sun, Aug 10, 2014 at 2:21 AM, Andreas Noever
<andreas.noever@gmail.com> wrote:
> On Sat, Jul 5, 2014 at 7:15 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Wed, Jun 25, 2014 at 12:55:01AM +0200, Bruno Prémont wrote:
>>> With commit b4aa0163056b ("efifb: Implement vga_default_device() (v2)")
>>> Matthew Garrett 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".
>>>
>>> Note: When vga_default_device() is set boot_vga PCI sysfs attribute
>>> reflects its state. When unset this attribute is 1 whenever
>>> IORESOURCE_ROM_SHADOW flag is set.
>>>
>>> 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().
>>>
>>> Tested-by: Anibal Francisco Martinez Cortina <linuxkid.zeuz@gmail.com>
>>> Cc: Matthew Garrett <matthew.garrett@nebula.com>
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
>>
>> I applied both with Matthew's ack to pci/misc for v3.17, thanks!
>
> I just tried to run the latest kernel.  It failed to boot and git
> bisect points to this commit (MacBookPro10,1 with Nvidia&Intel
> graphics).
>
> The (now removed) code in efifb_setup did always set default_vga, even
> if it had already been set earlier. The new code in pci_fixup_video
> runs only if vga_default_device() is NULL. Removing the check fixes
> the regression.
>
>
> The following calls to vga_set_default_device are made during boot:
>
> vga_arbiter_add_pci_device -> vga_set_default_device(intel)
> pci_fixup_video -> vga_set_default_device(intel) (there are two calls
> in pci_fixup_video, this one is the one near "Boot video device")
> pci_fixup_video -> vga_set_default_device(nvidia) (from the "Does
> firmware framebuffer belong to us?" loop, only if I remove the check)
>
> vga_arbiter_add_pci_device chooses intel simply because it is the
> first device. Next pci_fixup_video(intel) sees that it is the default
> device, sets the IORESOURCE_ROM_SHADOW flag and calls
> vga_set_default_device again. And finally (if the check is removed)
> pci_fixup_video(nvidia) sees that it owns the framebuffer and sets
> itself as the default device which allows the system to boot again.
>
> Does setting the ROM_SHADOW flag on (possibly) the wrong device have
> any effect?
Yes it does. Removing the line changes a long standing
i915 0000:00:02.0: Invalid ROM contents
into a
i915 0000:00:02.0: BAR 6: can't assign [??? 0x00000000 flags
0x20000000] (bogus alignment).

The first is logged at KERN_ERR and the second one only at KERN_INFO.
We are making progress.

Re: [PATCH 1/2 v2] x86, ia64: Move EFI_FB vga_default_device() initialization to pci_vga_fixup()

[Bruno Prémont]

On Sun, 10 August 2014 Andreas Noever <andreas.noever@gmail.com> wrote:

> On Sun, Aug 10, 2014 at 2:21 AM, Andreas Noever
> <andreas.noever@gmail.com> wrote:
> > On Sat, Jul 5, 2014 at 7:15 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >> On Wed, Jun 25, 2014 at 12:55:01AM +0200, Bruno Prémont wrote:
> >>> With commit b4aa0163056b ("efifb: Implement vga_default_device() (v2)")
> >>> Matthew Garrett 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".
> >>>
> >>> Note: When vga_default_device() is set boot_vga PCI sysfs attribute
> >>> reflects its state. When unset this attribute is 1 whenever
> >>> IORESOURCE_ROM_SHADOW flag is set.
> >>>
> >>> 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().
> >>>
> >>> Tested-by: Anibal Francisco Martinez Cortina <linuxkid.zeuz@gmail.com>
> >>> Cc: Matthew Garrett <matthew.garrett@nebula.com>
> >>> Cc: stable@vger.kernel.org
> >>> Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> >>
> >> I applied both with Matthew's ack to pci/misc for v3.17, thanks!
> >
> > I just tried to run the latest kernel.  It failed to boot and git
> > bisect points to this commit (MacBookPro10,1 with Nvidia&Intel
> > graphics).
> >
> > The (now removed) code in efifb_setup did always set default_vga, even
> > if it had already been set earlier. The new code in pci_fixup_video
> > runs only if vga_default_device() is NULL. Removing the check fixes
> > the regression.
> >
> >
> > The following calls to vga_set_default_device are made during boot:
> >
> > vga_arbiter_add_pci_device -> vga_set_default_device(intel)
> > pci_fixup_video -> vga_set_default_device(intel) (there are two calls
> > in pci_fixup_video, this one is the one near "Boot video device")
> > pci_fixup_video -> vga_set_default_device(nvidia) (from the "Does
> > firmware framebuffer belong to us?" loop, only if I remove the check)
> >
> > vga_arbiter_add_pci_device chooses intel simply because it is the
> > first device. Next pci_fixup_video(intel) sees that it is the default
> > device, sets the IORESOURCE_ROM_SHADOW flag and calls
> > vga_set_default_device again. And finally (if the check is removed)
> > pci_fixup_video(nvidia) sees that it owns the framebuffer and sets
> > itself as the default device which allows the system to boot again.
> >
> > Does setting the ROM_SHADOW flag on (possibly) the wrong device have
> > any effect?
> Yes it does. Removing the line changes a long standing
> i915 0000:00:02.0: Invalid ROM contents
> into a
> i915 0000:00:02.0: BAR 6: can't assign [??? 0x00000000 flags
> 0x20000000] (bogus alignment).
> 
> The first is logged at KERN_ERR and the second one only at KERN_INFO.
> We are making progress.

How does your system behave if you change vga_arbiter_add_pci_device()
not to set vga_set_default_device() with the first device registered?

That is remove the 
#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
code block in vga_arbiter_add_pci_device().

How did your system behave in the past if you did not enable efifb?

Bruno

Re: [PATCH 1/2 v2] x86, ia64: Move EFI_FB vga_default_device() initialization to pci_vga_fixup()

[Andreas Noever]

On Sun, Aug 10, 2014 at 11:26 AM, Bruno Prémont
<bonbons@linux-vserver.org> wrote:
> On Sun, 10 August 2014 Andreas Noever <andreas.noever@gmail.com> wrote:
>
>> On Sun, Aug 10, 2014 at 2:21 AM, Andreas Noever
>> <andreas.noever@gmail.com> wrote:
>> > On Sat, Jul 5, 2014 at 7:15 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> >> On Wed, Jun 25, 2014 at 12:55:01AM +0200, Bruno Prémont wrote:
>> >>> With commit b4aa0163056b ("efifb: Implement vga_default_device() (v2)")
>> >>> Matthew Garrett 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".
>> >>>
>> >>> Note: When vga_default_device() is set boot_vga PCI sysfs attribute
>> >>> reflects its state. When unset this attribute is 1 whenever
>> >>> IORESOURCE_ROM_SHADOW flag is set.
>> >>>
>> >>> 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().
>> >>>
>> >>> Tested-by: Anibal Francisco Martinez Cortina <linuxkid.zeuz@gmail.com>
>> >>> Cc: Matthew Garrett <matthew.garrett@nebula.com>
>> >>> Cc: stable@vger.kernel.org
>> >>> Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
>> >>
>> >> I applied both with Matthew's ack to pci/misc for v3.17, thanks!
>> >
>> > I just tried to run the latest kernel.  It failed to boot and git
>> > bisect points to this commit (MacBookPro10,1 with Nvidia&Intel
>> > graphics).
>> >
>> > The (now removed) code in efifb_setup did always set default_vga, even
>> > if it had already been set earlier. The new code in pci_fixup_video
>> > runs only if vga_default_device() is NULL. Removing the check fixes
>> > the regression.
>> >
>> >
>> > The following calls to vga_set_default_device are made during boot:
>> >
>> > vga_arbiter_add_pci_device -> vga_set_default_device(intel)
>> > pci_fixup_video -> vga_set_default_device(intel) (there are two calls
>> > in pci_fixup_video, this one is the one near "Boot video device")
>> > pci_fixup_video -> vga_set_default_device(nvidia) (from the "Does
>> > firmware framebuffer belong to us?" loop, only if I remove the check)
>> >
>> > vga_arbiter_add_pci_device chooses intel simply because it is the
>> > first device. Next pci_fixup_video(intel) sees that it is the default
>> > device, sets the IORESOURCE_ROM_SHADOW flag and calls
>> > vga_set_default_device again. And finally (if the check is removed)
>> > pci_fixup_video(nvidia) sees that it owns the framebuffer and sets
>> > itself as the default device which allows the system to boot again.
>> >
>> > Does setting the ROM_SHADOW flag on (possibly) the wrong device have
>> > any effect?
>> Yes it does. Removing the line changes a long standing
>> i915 0000:00:02.0: Invalid ROM contents
>> into a
>> i915 0000:00:02.0: BAR 6: can't assign [??? 0x00000000 flags
>> 0x20000000] (bogus alignment).
>>
>> The first is logged at KERN_ERR and the second one only at KERN_INFO.
>> We are making progress.
>
> How does your system behave if you change vga_arbiter_add_pci_device()
> not to set vga_set_default_device() with the first device registered?
>
> That is remove the
> #ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
> code block in vga_arbiter_add_pci_device().
The system does not boot.  The Intel device is still set as the
default device in pci_fixup_video (near "Boot video device") and
prevents the nvidia device (which is initialized later) from becoming
the default one.


> How did your system behave in the past if you did not enable efifb?
I don't think that I ever did not enable efifb. It seems to have been
around for quite a while?

Andreas

Re: [PATCH 1/2 v2] x86, ia64: Move EFI_FB vga_default_device() initialization to pci_vga_fixup()

[Bruno Prémont]

On Sun, 10 August 2014 Andreas Noever <andreas.noever@gmail.com> wrote:
> On Sun, Aug 10, 2014 at 11:26 AM, Bruno Prémont wrote:
> > On Sun, 10 August 2014 Andreas Noever wrote:
> >
> >> On Sun, Aug 10, 2014 at 2:21 AM, Andreas Noeverwrote:
> >> > I just tried to run the latest kernel.  It failed to boot and git
> >> > bisect points to this commit (MacBookPro10,1 with Nvidia&Intel
> >> > graphics).
> >> >
> >> > The (now removed) code in efifb_setup did always set default_vga, even
> >> > if it had already been set earlier. The new code in pci_fixup_video
> >> > runs only if vga_default_device() is NULL. Removing the check fixes
> >> > the regression.
> >> >
> >> >
> >> > The following calls to vga_set_default_device are made during boot:
> >> >
> >> > vga_arbiter_add_pci_device -> vga_set_default_device(intel)
> >> > pci_fixup_video -> vga_set_default_device(intel) (there are two calls
> >> > in pci_fixup_video, this one is the one near "Boot video device")
> >> > pci_fixup_video -> vga_set_default_device(nvidia) (from the "Does
> >> > firmware framebuffer belong to us?" loop, only if I remove the check)
> >> >
> >> > vga_arbiter_add_pci_device chooses intel simply because it is the
> >> > first device. Next pci_fixup_video(intel) sees that it is the default
> >> > device, sets the IORESOURCE_ROM_SHADOW flag and calls
> >> > vga_set_default_device again. And finally (if the check is removed)
> >> > pci_fixup_video(nvidia) sees that it owns the framebuffer and sets
> >> > itself as the default device which allows the system to boot again.
> >> >
> >> > Does setting the ROM_SHADOW flag on (possibly) the wrong device have
> >> > any effect?
> >> Yes it does. Removing the line changes a long standing
> >> i915 0000:00:02.0: Invalid ROM contents
> >> into a
> >> i915 0000:00:02.0: BAR 6: can't assign [??? 0x00000000 flags
> >> 0x20000000] (bogus alignment).
> >>
> >> The first is logged at KERN_ERR and the second one only at KERN_INFO.
> >> We are making progress.
> >
> > How does your system behave if you change vga_arbiter_add_pci_device()
> > not to set vga_set_default_device() with the first device registered?
> >
> > That is remove the
> > #ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
> > code block in vga_arbiter_add_pci_device().
> The system does not boot.  The Intel device is still set as the
> default device in pci_fixup_video (near "Boot video device") and
> prevents the nvidia device (which is initialized later) from becoming
> the default one.
>
> > How did your system behave in the past if you did not enable efifb?
> I don't think that I ever did not enable efifb. It seems to have been
> around for quite a while?

The question here is if you system just refuses to boot as well.

The aim of my patch is to make system work (properly) when efifb is not used
(why use efifb if built-in drm drivers handle the GPU(s)?)

If you decided to replace efifb with platform-framebuffer+simplefb/simpledrm
you would probably see the same issue as that would be no efifb as well.

The presence of efifb should not be mandatory for successfully booting
and running Xorg.

> Andreas

How does you system behave with below patch on top of Linus tree?

Bruno



---
diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index c61ea57..15d0068 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -367,7 +367,7 @@ static void pci_fixup_video(struct pci_dev *pdev)
 		}
 		bus = bus->parent;
 	}
-	if (!vga_default_device() || pdev == vga_default_device()) {
+	if (pdev == vga_default_device()) {
 		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;
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index d2077f0..ac44d87 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -112,10 +112,8 @@ both:
 	return 1;
 }
 
-#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
 /* this is only used a cookie - it should not be dereferenced */
 static struct pci_dev *vga_default;
-#endif
 
 static void vga_arb_device_card_gone(struct pci_dev *pdev);
 
@@ -131,7 +129,6 @@ static struct vga_device *vgadev_find(struct pci_dev *pdev)
 }
 
 /* Returns the default VGA device (vgacon's babe) */
-#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
 struct pci_dev *vga_default_device(void)
 {
 	return vga_default;
@@ -147,7 +144,6 @@ void vga_set_default_device(struct pci_dev *pdev)
 	pci_dev_put(vga_default);
 	vga_default = pci_dev_get(pdev);
 }
-#endif
 
 static inline void vga_irq_set_state(struct vga_device *vgadev, bool state)
 {
@@ -580,10 +576,10 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
 		bus = bus->parent;
 	}
 
+#if 0
 	/* Deal with VGA default device. Use first enabled one
 	 * by default if arch doesn't have it's own hook
 	 */
-#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
 	if (vga_default == NULL &&
 	    ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK))
 		vga_set_default_device(pdev);
@@ -621,10 +617,8 @@ static bool vga_arbiter_del_pci_device(struct pci_dev *pdev)
 		goto bail;
 	}
 
-#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
 	if (vga_default == pdev)
 		vga_set_default_device(NULL);
-#endif
 
 	if (vgadev->decodes & (VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM))
 		vga_decode_count--;
diff --git a/include/linux/vgaarb.h b/include/linux/vgaarb.h
index 2c02f3a..c37bd4d 100644
--- a/include/linux/vgaarb.h
+++ b/include/linux/vgaarb.h
@@ -182,7 +182,6 @@ extern void vga_put(struct pci_dev *pdev, unsigned int rsrc);
  *     vga_get()...
  */
 
-#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
 #ifdef CONFIG_VGA_ARB
 extern struct pci_dev *vga_default_device(void);
 extern void vga_set_default_device(struct pci_dev *pdev);
@@ -190,7 +189,6 @@ extern void vga_set_default_device(struct pci_dev *pdev);
 static inline struct pci_dev *vga_default_device(void) { return NULL; };
 static inline void vga_set_default_device(struct pci_dev *pdev) { };
 #endif
-#endif
 
 /**
  *     vga_conflicts

Re: [PATCH 1/2 v2] x86, ia64: Move EFI_FB vga_default_device() initialization to pci_vga_fixup()

[Andreas Noever]

On Sun, Aug 10, 2014 at 6:34 PM, Bruno Prémont
<bonbons@linux-vserver.org> wrote:
> On Sun, 10 August 2014 Andreas Noever <andreas.noever@gmail.com> wrote:
>> On Sun, Aug 10, 2014 at 11:26 AM, Bruno Prémont wrote:
>> > On Sun, 10 August 2014 Andreas Noever wrote:
>> >
>> >> On Sun, Aug 10, 2014 at 2:21 AM, Andreas Noeverwrote:
>> >> > I just tried to run the latest kernel.  It failed to boot and git
>> >> > bisect points to this commit (MacBookPro10,1 with Nvidia&Intel
>> >> > graphics).
>> >> >
>> >> > The (now removed) code in efifb_setup did always set default_vga, even
>> >> > if it had already been set earlier. The new code in pci_fixup_video
>> >> > runs only if vga_default_device() is NULL. Removing the check fixes
>> >> > the regression.
>> >> >
>> >> >
>> >> > The following calls to vga_set_default_device are made during boot:
>> >> >
>> >> > vga_arbiter_add_pci_device -> vga_set_default_device(intel)
>> >> > pci_fixup_video -> vga_set_default_device(intel) (there are two calls
>> >> > in pci_fixup_video, this one is the one near "Boot video device")
>> >> > pci_fixup_video -> vga_set_default_device(nvidia) (from the "Does
>> >> > firmware framebuffer belong to us?" loop, only if I remove the check)
>> >> >
>> >> > vga_arbiter_add_pci_device chooses intel simply because it is the
>> >> > first device. Next pci_fixup_video(intel) sees that it is the default
>> >> > device, sets the IORESOURCE_ROM_SHADOW flag and calls
>> >> > vga_set_default_device again. And finally (if the check is removed)
>> >> > pci_fixup_video(nvidia) sees that it owns the framebuffer and sets
>> >> > itself as the default device which allows the system to boot again.
>> >> >
>> >> > Does setting the ROM_SHADOW flag on (possibly) the wrong device have
>> >> > any effect?
>> >> Yes it does. Removing the line changes a long standing
>> >> i915 0000:00:02.0: Invalid ROM contents
>> >> into a
>> >> i915 0000:00:02.0: BAR 6: can't assign [??? 0x00000000 flags
>> >> 0x20000000] (bogus alignment).
>> >>
>> >> The first is logged at KERN_ERR and the second one only at KERN_INFO.
>> >> We are making progress.
>> >
>> > How does your system behave if you change vga_arbiter_add_pci_device()
>> > not to set vga_set_default_device() with the first device registered?
>> >
>> > That is remove the
>> > #ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
>> > code block in vga_arbiter_add_pci_device().
>> The system does not boot.  The Intel device is still set as the
>> default device in pci_fixup_video (near "Boot video device") and
>> prevents the nvidia device (which is initialized later) from becoming
>> the default one.
>>
>> > How did your system behave in the past if you did not enable efifb?
>> I don't think that I ever did not enable efifb. It seems to have been
>> around for quite a while?
>
> The question here is if you system just refuses to boot as well.

I have just tested 3.16 without FB_EFI and it refuses to boot as well.

> The aim of my patch is to make system work (properly) when efifb is not used
> (why use efifb if built-in drm drivers handle the GPU(s)?)
>
> If you decided to replace efifb with platform-framebuffer+simplefb/simpledrm
> you would probably see the same issue as that would be no efifb as well.
>
> The presence of efifb should not be mandatory for successfully booting
> and running Xorg.
>
>> Andreas
>
> How does you system behave with below patch on top of Linus tree?

This patch fixes the problem (with and without FB_EFI).

Andreas


> Bruno
>
>
>
> ---
> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> index c61ea57..15d0068 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -367,7 +367,7 @@ static void pci_fixup_video(struct pci_dev *pdev)
>                 }
>                 bus = bus->parent;
>         }
> -       if (!vga_default_device() || pdev == vga_default_device()) {
> +       if (pdev == vga_default_device()) {
>                 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;
> diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> index d2077f0..ac44d87 100644
> --- a/drivers/gpu/vga/vgaarb.c
> +++ b/drivers/gpu/vga/vgaarb.c
> @@ -112,10 +112,8 @@ both:
>         return 1;
>  }
>
> -#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
>  /* this is only used a cookie - it should not be dereferenced */
>  static struct pci_dev *vga_default;
> -#endif
>
>  static void vga_arb_device_card_gone(struct pci_dev *pdev);
>
> @@ -131,7 +129,6 @@ static struct vga_device *vgadev_find(struct pci_dev *pdev)
>  }
>
>  /* Returns the default VGA device (vgacon's babe) */
> -#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
>  struct pci_dev *vga_default_device(void)
>  {
>         return vga_default;
> @@ -147,7 +144,6 @@ void vga_set_default_device(struct pci_dev *pdev)
>         pci_dev_put(vga_default);
>         vga_default = pci_dev_get(pdev);
>  }
> -#endif
>
>  static inline void vga_irq_set_state(struct vga_device *vgadev, bool state)
>  {
> @@ -580,10 +576,10 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
>                 bus = bus->parent;
>         }
>
> +#if 0
>         /* Deal with VGA default device. Use first enabled one
>          * by default if arch doesn't have it's own hook
>          */
> -#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
>         if (vga_default == NULL &&
>             ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK))
>                 vga_set_default_device(pdev);
> @@ -621,10 +617,8 @@ static bool vga_arbiter_del_pci_device(struct pci_dev *pdev)
>                 goto bail;
>         }
>
> -#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
>         if (vga_default == pdev)
>                 vga_set_default_device(NULL);
> -#endif
>
>         if (vgadev->decodes & (VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM))
>                 vga_decode_count--;
> diff --git a/include/linux/vgaarb.h b/include/linux/vgaarb.h
> index 2c02f3a..c37bd4d 100644
> --- a/include/linux/vgaarb.h
> +++ b/include/linux/vgaarb.h
> @@ -182,7 +182,6 @@ extern void vga_put(struct pci_dev *pdev, unsigned int rsrc);
>   *     vga_get()...
>   */
>
> -#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
>  #ifdef CONFIG_VGA_ARB
>  extern struct pci_dev *vga_default_device(void);
>  extern void vga_set_default_device(struct pci_dev *pdev);
> @@ -190,7 +189,6 @@ extern void vga_set_default_device(struct pci_dev *pdev);
>  static inline struct pci_dev *vga_default_device(void) { return NULL; };
>  static inline void vga_set_default_device(struct pci_dev *pdev) { };
>  #endif
> -#endif
>
>  /**
>   *     vga_conflicts

[PATCH 0/2] x86, ia64: Move EFI_FB vga_default_device() initialization to pci_vga_fixup()

[Bruno Prémont]

This series improves on commit 20cde694027e (x86, ia64: Move EFI_FB
vga_default_device() initialization to pci_vga_fixup()):
- cleanup remaining but always-true #ifndefs
- fix boot regression on dual-GPU Macs

Andreas, can you please test this series? It is a modification from
previous testing patch that should still work fine for you.
That testing patch would have been failing X startup on old BIOS systems
booted with vga=normal (or otherwise in VGA text mode).


Greg, in case you have scheduled above-mentioned commit for your next
stable iteration, please hold it back in the queue until this follow-up
has landed and can be included within the same stable update as alone
that patch regresses for Macs with dual-GPU and using efifb.

Bruno

[PATCH 1/2] vgaarb: Drop obsolete #ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE

[Bruno Prémont]

With commit 20cde694027e boot video device detection was moved from
efifb to x86 and ia64 pci/fixup.c.

Remove the left-over #ifndef check that will allways match since
the corresponding arch-specific define is gone with above patch.

Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
CC: Matthew Garrett <matthew.garrett@nebula.com>
CC: stable@vger.kernel.org # v3.5+
---
May be applied to stable as cleanup for upstream commit
20cde694027e7477cc532833e38ab9fcaa83fb64 which is marked for
stable.

 drivers/gpu/vga/vgaarb.c | 8 --------
 include/linux/vgaarb.h   | 2 --
 2 files changed, 10 deletions(-)

diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index d2077f0..257674d 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -112,10 +112,8 @@ both:
 	return 1;
 }
 
-#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
 /* this is only used a cookie - it should not be dereferenced */
 static struct pci_dev *vga_default;
-#endif
 
 static void vga_arb_device_card_gone(struct pci_dev *pdev);
 
@@ -131,7 +129,6 @@ static struct vga_device *vgadev_find(struct pci_dev *pdev)
 }
 
 /* Returns the default VGA device (vgacon's babe) */
-#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
 struct pci_dev *vga_default_device(void)
 {
 	return vga_default;
@@ -147,7 +144,6 @@ void vga_set_default_device(struct pci_dev *pdev)
 	pci_dev_put(vga_default);
 	vga_default = pci_dev_get(pdev);
 }
-#endif
 
 static inline void vga_irq_set_state(struct vga_device *vgadev, bool state)
 {
@@ -583,11 +579,9 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
 	/* Deal with VGA default device. Use first enabled one
 	 * by default if arch doesn't have it's own hook
 	 */
-#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
 	if (vga_default == NULL &&
 	    ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK))
 		vga_set_default_device(pdev);
-#endif
 
 	vga_arbiter_check_bridge_sharing(vgadev);
 
@@ -621,10 +615,8 @@ static bool vga_arbiter_del_pci_device(struct pci_dev *pdev)
 		goto bail;
 	}
 
-#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
 	if (vga_default == pdev)
 		vga_set_default_device(NULL);
-#endif
 
 	if (vgadev->decodes & (VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM))
 		vga_decode_count--;
diff --git a/include/linux/vgaarb.h b/include/linux/vgaarb.h
index 2c02f3a..c37bd4d 100644
--- a/include/linux/vgaarb.h
+++ b/include/linux/vgaarb.h
@@ -182,7 +182,6 @@ extern void vga_put(struct pci_dev *pdev, unsigned int rsrc);
  *     vga_get()...
  */
 
-#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
 #ifdef CONFIG_VGA_ARB
 extern struct pci_dev *vga_default_device(void);
 extern void vga_set_default_device(struct pci_dev *pdev);
@@ -190,7 +189,6 @@ extern void vga_set_default_device(struct pci_dev *pdev);
 static inline struct pci_dev *vga_default_device(void) { return NULL; };
 static inline void vga_set_default_device(struct pci_dev *pdev) { };
 #endif
-#endif
 
 /**
  *     vga_conflicts
-- 
1.8.5.5

[PATCH 2/2] x86, ia64: Don't default to first video device

[Bruno Prémont]

With commit 20cde694027e boot video device detection was moved from
efifb to x86 and ia64 pci/fixup.c.

For dual-GPU Apple computers above above change represents a regression
as code in efifb did forcefully override vga_default_device while the
merge did not (changed ordering of actions).

This stops setting default_vga_device when applying
IORESOURCE_ROM_SHADOW (only doing so for the detected boot GPU) and
updates logging of boot video device selection, in vgaarb which covers
VGA text-mode booting and first half of pci_fixup_video which covers
framebuffer mode (EFI, VESA).

By setting IORESOURCE_ROM_SHADOW only on effective boot GPU we also
corrects a longstanding complaint from intel driver as reported by
Andreas:
  > Does setting the ROM_SHADOW flag on (possibly) the wrong device
  > have any effect?
  Yes it does. Removing the line changes a long standing
    i915 0000:00:02.0: Invalid ROM contents
  into a
    i915 0000:00:02.0: BAR 6: can't assign [??? 0x00000000 flags
    0x20000000] (bogus alignment).
  The first is logged at KERN_ERR while the second is at KERN_INFO.

Reported-By: Andreas Noever <andreas.noever@gmail.com>
Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
CC: Matthew Garrett <matthew.garrett@nebula.com>
CC: stable@vger.kernel.org # v3.5+
---
Must be applied to stable when upstream commit
20cde694027e7477cc532833e38ab9fcaa83fb64, which is marked for
stable, gets applied.

Can be applied without patch 1/2 from this series though dropped
#ifndefs will cause this patch not to apply cleanly.


 arch/ia64/pci/fixup.c    | 9 +++++----
 arch/x86/pci/fixup.c     | 9 +++++----
 drivers/gpu/vga/vgaarb.c | 4 +++-
 3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/arch/ia64/pci/fixup.c b/arch/ia64/pci/fixup.c
index ec73b2c..05198f8 100644
--- a/arch/ia64/pci/fixup.c
+++ b/arch/ia64/pci/fixup.c
@@ -54,8 +54,10 @@ static void pci_fixup_video(struct pci_dev *pdev)
 				continue;
 
 			if (screen_info.lfb_base >= start &&
-			    (screen_info.lfb_base + screen_info.lfb_size) < end)
+			    (screen_info.lfb_base + screen_info.lfb_size) < end) {
+				dev_printk(KERN_DEBUG, &pdev->dev, "Boot video device\n");
 				vga_set_default_device(pdev);
+			}
 		}
 	}
 
@@ -79,12 +81,11 @@ static void pci_fixup_video(struct pci_dev *pdev)
 		}
 		bus = bus->parent;
 	}
-	if (!vga_default_device() || pdev == vga_default_device()) {
+	if (pdev == vga_default_device()) {
 		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");
-			vga_set_default_device(pdev);
+			dev_printk(KERN_DEBUG, &pdev->dev, "Video device with shadowed ROM\n");
 		}
 	}
 }
diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index c61ea57..5b392d2 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -342,8 +342,10 @@ static void pci_fixup_video(struct pci_dev *pdev)
 				continue;
 
 			if (screen_info.lfb_base >= start &&
-			    (screen_info.lfb_base + screen_info.lfb_size) < end)
+			    (screen_info.lfb_base + screen_info.lfb_size) < end) {
+				dev_printk(KERN_DEBUG, &pdev->dev, "Boot video device\n");
 				vga_set_default_device(pdev);
+			}
 		}
 	}
 
@@ -367,12 +369,11 @@ static void pci_fixup_video(struct pci_dev *pdev)
 		}
 		bus = bus->parent;
 	}
-	if (!vga_default_device() || pdev == vga_default_device()) {
+	if (pdev == vga_default_device()) {
 		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");
-			vga_set_default_device(pdev);
+			dev_printk(KERN_DEBUG, &pdev->dev, "Video device with shadowed ROM\n");
 		}
 	}
 }
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index 257674d..c6eeed5 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -580,8 +580,10 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
 	 * by default if arch doesn't have it's own hook
 	 */
 	if (vga_default == NULL &&
-	    ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK))
+	    ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) {
+		pr_info("vgaarb: Boot video device: PCI:%s\n", pci_name(pdev));
 		vga_set_default_device(pdev);
+	}
 
 	vga_arbiter_check_bridge_sharing(vgadev);
 
-- 
1.8.5.5

Re: [PATCH 0/2] x86, ia64: Move EFI_FB vga_default_device() initialization to pci_vga_fixup()

[Andreas Noever]

On Sat, Aug 16, 2014 at 7:21 PM, Bruno Prémont
<bonbons@linux-vserver.org> wrote:
> This series improves on commit 20cde694027e (x86, ia64: Move EFI_FB
> vga_default_device() initialization to pci_vga_fixup()):
> - cleanup remaining but always-true #ifndefs
> - fix boot regression on dual-GPU Macs
>
> Andreas, can you please test this series? It is a modification from
> previous testing patch that should still work fine for you.
> That testing patch would have been failing X startup on old BIOS systems
> booted with vga=normal (or otherwise in VGA text mode).
>
>
> Greg, in case you have scheduled above-mentioned commit for your next
> stable iteration, please hold it back in the queue until this follow-up
> has landed and can be included within the same stable update as alone
> that patch regresses for Macs with dual-GPU and using efifb.
>
> Bruno

Fails again (with and without efifb).

The vga_set_default_device in vga_arbiter_add_pci_device is at fault.
It sets the boot video device to intel. Removing it makes the system
bootable again.

Re: [PATCH 0/2] x86, ia64: Move EFI_FB vga_default_device() initialization to pci_vga_fixup()

[Bruno Prémont]

On Tue, 19 Aug 2014 17:45:00 +0200 Andreas Noever wrote:
> On Sat, Aug 16, 2014 at 7:21 PM, Bruno Prémont wrote:
> > This series improves on commit 20cde694027e (x86, ia64: Move EFI_FB
> > vga_default_device() initialization to pci_vga_fixup()):
> > - cleanup remaining but always-true #ifndefs
> > - fix boot regression on dual-GPU Macs
> >
> > Andreas, can you please test this series? It is a modification from
> > previous testing patch that should still work fine for you.
> > That testing patch would have been failing X startup on old BIOS systems
> > booted with vga=normal (or otherwise in VGA text mode).
> >
> >
> > Greg, in case you have scheduled above-mentioned commit for your next
> > stable iteration, please hold it back in the queue until this follow-up
> > has landed and can be included within the same stable update as alone
> > that patch regresses for Macs with dual-GPU and using efifb.
> >
> > Bruno
> 
> Fails again (with and without efifb).
> 
> The vga_set_default_device in vga_arbiter_add_pci_device is at fault.
> It sets the boot video device to intel. Removing it makes the system
> bootable again.

Could you provide your whole kernel log? I would like to understand
how your vga devices are setup and why it starts the wrong way.

If you can grab kernel log from both working and failing setups it
would be even better. The failing one is interesting for where exactly it
starts failing at boot.


Bruno

Re: [PATCH 0/2] x86, ia64: Move EFI_FB vga_default_device() initialization to pci_vga_fixup()

[Bruno Prémont]

On Wed, 20 Aug 2014 07:55:08 +0200 Bruno Prémont wrote:
> On Tue, 19 Aug 2014 17:45:00 +0200 Andreas Noever wrote:
> > On Sat, Aug 16, 2014 at 7:21 PM, Bruno Prémont wrote:
> > > This series improves on commit 20cde694027e (x86, ia64: Move EFI_FB
> > > vga_default_device() initialization to pci_vga_fixup()):
> > > - cleanup remaining but always-true #ifndefs
> > > - fix boot regression on dual-GPU Macs
> > >
> > > Andreas, can you please test this series? It is a modification from
> > > previous testing patch that should still work fine for you.
> > > That testing patch would have been failing X startup on old BIOS systems
> > > booted with vga=normal (or otherwise in VGA text mode).
> > >
> > >
> > > Greg, in case you have scheduled above-mentioned commit for your next
> > > stable iteration, please hold it back in the queue until this follow-up
> > > has landed and can be included within the same stable update as alone
> > > that patch regresses for Macs with dual-GPU and using efifb.
> > >
> > > Bruno
> > 
> > Fails again (with and without efifb).
> > 
> > The vga_set_default_device in vga_arbiter_add_pci_device is at fault.
> > It sets the boot video device to intel. Removing it makes the system
> > bootable again.
> 
> Could you provide your whole kernel log? I would like to understand
> how your vga devices are setup and why it starts the wrong way.
> 
> If you can grab kernel log from both working and failing setups it
> would be even better. The failing one is interesting for where exactly it
> starts failing at boot.

While collecting debug logs, please apply following patch to get
PCI command and bridge control registers as configured when vgaarb looks
at them.



diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index af02597..8c8e7af 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -554,6 +554,7 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
 	 * clear that below if the bridge isn't forwarding
 	 */
 	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
+	pr_info("vgaarb: PCI:%s PCI_COMMAND=%04x\n", pci_name(pdev), (unsigned int)cmd);
 	if (cmd & PCI_COMMAND_IO)
 		vgadev->owns |= VGA_RSRC_LEGACY_IO;
 	if (cmd & PCI_COMMAND_MEMORY)
@@ -567,6 +568,7 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
 			u16 l;
 			pci_read_config_word(bridge, PCI_BRIDGE_CONTROL,
 					     &l);
+			pr_info("vgaarb: PCI:%s, bridge PCI:%s PCI_BRIDGE_CONTROL=%04x\n", pci_name(pdev), pci_name(bridge), (unsigned int)l);
 			if (!(l & PCI_BRIDGE_CTL_VGA)) {
 				vgadev->owns = 0;
 				break;

Re: [PATCH 0/2] x86, ia64: Move EFI_FB vga_default_device() initialization to pci_vga_fixup()

[Bruno Prémont]

On Thu, 21 August 2014 Andreas Noever <andreas.noever@gmail.com> wrote:
> dmesg with your patches and vga_set_default_device commented out
> (after "vgaarb: Boot video device...") as otherwise the system won't
> boot.

Do you know more precisely where your system hangs when it does not boot?
That's the part I can't find in this thread.
Is it dead-locking/freezing or just booting without displaying anything
(though network coming up if connected, keyboard working (e.g. caps key).

Try blacklisting both i915 and nouveau modules (and each one individually)
an see how far your system gets. Also make sure your network comes up
automatically, so that even if display remains black you can check via
network if your system is alive and what it complains about.

> dmesg | grep vgaarb
> [    1.340118] vgaarb: PCI:0000:00:02.0 PCI_COMMAND=0007
> [    1.340119] vgaarb: Boot video device: PCI:0000:00:02.0
> [    1.340120] vgaarb: device added:
> PCI:0000:00:02.0,decodes=io+mem,owns=io+mem,locks=none
> [    1.340130] vgaarb: PCI:0000:01:00.0 PCI_COMMAND=0006
> [    1.340132] vgaarb: PCI:0000:01:00.0, bridge PCI:0000:00:01.0
> PCI_BRIDGE_CONTROL=0000
> [    1.340133] vgaarb: device added:
> PCI:0000:01:00.0,decodes=io+mem,owns=none,locks=none
> [    1.340135] vgaarb: loaded
> [    1.340136] vgaarb: bridge control possible 0000:01:00.0
> [    1.340136] vgaarb: no bridge control possible 0000:00:02.0
> [    3.798430] vgaarb: device changed decodes:
> PCI:0000:00:02.0,olddecodes=io+mem,decodes=none:owns=io+mem
> 
> 
> If the line is not commented out then vgaarb simply declares the first
> (enabled) device to be the default one, which is incorrect. And the
> overwrite logic in pci_fixup_video is not triggered, since a default
> device has already been set.

The initial selection I am doing does match the PCI_COMMAND flags
as set for the devices (or masked by parent bridge), but probably none
of them has active legacy VGA I/O ports.
So the question would rather be how to determine which I/O port is active
for the Intel graphics and adjust vgaarb's "decodes"/owns interpretation
on that basis (there is no I/O active for the nvidia one).
I'm thinking about selecting only device that decodes the legacy VGA I/O
range and not those with any some other I/O range.

The short-term fix probably is to just unconditionally perform the
screen_info check in pci_fixup_video() while leaving vgaarb's initial
card selection alone for legacy hardware. Thus replicating efifb's
original behavior (and also get back incorrect ROM_SHADOW flagging).
Corresponding patch below (on top of both patches in this series, but
should apply without them as well). As mentioned in the patch this
papers over the real issue.


A second step would then be to tune vgaarb's initial selection.
Bjorn, is it possible to verify which I/O ports are decoded by a PCI
device at the time of adding it to vgaarb? If so, how? I would like to
check for legacy VGA I/O range (0x03B0-0x03DF) and only let vgaarb set
a device as default if that I/O range is decoded by the device.

Bruno



> On Wed, Aug 20, 2014 at 9:11 AM, Bruno Prémont wrote:
> > On Wed, 20 Aug 2014 07:55:08 +0200 Bruno Prémont wrote:
> >> On Tue, 19 Aug 2014 17:45:00 +0200 Andreas Noever wrote:
> >> > On Sat, Aug 16, 2014 at 7:21 PM, Bruno Prémont wrote:
> >> > > This series improves on commit 20cde694027e (x86, ia64: Move EFI_FB
> >> > > vga_default_device() initialization to pci_vga_fixup()):
> >> > > - cleanup remaining but always-true #ifndefs
> >> > > - fix boot regression on dual-GPU Macs
> >> > >
> >> > > Andreas, can you please test this series? It is a modification from
> >> > > previous testing patch that should still work fine for you.
> >> > > That testing patch would have been failing X startup on old BIOS systems
> >> > > booted with vga=normal (or otherwise in VGA text mode).
> >> > >
> >> > >
> >> > > Greg, in case you have scheduled above-mentioned commit for your next
> >> > > stable iteration, please hold it back in the queue until this follow-up
> >> > > has landed and can be included within the same stable update as alone
> >> > > that patch regresses for Macs with dual-GPU and using efifb.
> >> > >
> >> > > Bruno
> >> >
> >> > Fails again (with and without efifb).
> >> >
> >> > The vga_set_default_device in vga_arbiter_add_pci_device is at fault.
> >> > It sets the boot video device to intel. Removing it makes the system
> >> > bootable again.
> >>
> >> Could you provide your whole kernel log? I would like to understand
> >> how your vga devices are setup and why it starts the wrong way.
> >>
> >> If you can grab kernel log from both working and failing setups it
> >> would be even better. The failing one is interesting for where exactly it
> >> starts failing at boot.
> >
> > While collecting debug logs, please apply following patch to get
> > PCI command and bridge control registers as configured when vgaarb looks
> > at them.

From: Bruno Prémont <bonbons@linux-vserver.org>
Subject: [PATCH] x86: Force selection of vga_default_device on screen_info

Apple dual-GPU systems get the wrong GPU choosen by vgaarb because the
built-in Intel GPU has I/O ports active and no bridge in front that
would block legacy VGA I/O ports. (though no legacy VGA is setup)

The wrong initial selection prevents system from booting properly.

The proper solution would be to improve vgaarb's initial device
selection. Until that has been done return to behavior that efifb
implemented before the move to pci_fixup_video.

The draw-back of this old operation mode is that a wrong device
gets the IORESOURCE_ROM_SHADOW flag set.

Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
CC: Matthew Garrett <matthew.garrett@nebula.com>
CC: stable@vger.kernel.org # v3.5+
---
 arch/x86/pci/fixup.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index 5b392d2..fc509d5 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -326,7 +326,10 @@ static void pci_fixup_video(struct pci_dev *pdev)
 	struct pci_bus *bus;
 	u16 config;
 
-	if (!vga_default_device()) {
+	if (!vga_default_device() || 1) {
+		/* The `|| 1` condition papers over vgaarb initial GPU selection limitation
+		 * on Apple dual-GPU systems using EFI.
+		 */
 		resource_size_t start, end;
 		int i;
 
-- 
1.8.5.5

Re: [PATCH 0/2] x86, ia64: Move EFI_FB vga_default_device() initialization to pci_vga_fixup()

[Bjorn Helgaas]

On Thu, Aug 21, 2014 at 4:34 PM, Bruno Prémont
<bonbons@linux-vserver.org> wrote:

> A second step would then be to tune vgaarb's initial selection.
> Bjorn, is it possible to verify which I/O ports are decoded by a PCI
> device at the time of adding it to vgaarb? If so, how? I would like to
> check for legacy VGA I/O range (0x03B0-0x03DF) and only let vgaarb set
> a device as default if that I/O range is decoded by the device.

I don't know of a way.  I'm pretty sure VGA devices are allowed to
respond to those legacy addresses even if there's no BAR for them, but
I haven't found a spec reference for this.  There is the VGA Enable
bit in bridges, of course (PCI Bridge spec, sec 12.1.1.  If the VGA
device is behind a bridge that doesn't have the VGA Enable bit set, it
probably isn't the default device.

Bjorn

Re: [PATCH 0/2] x86, ia64: Move EFI_FB vga_default_device() initialization to pci_vga_fixup()

[Bruno Prémont]

On Thu, 21 Aug 2014 23:39:31 -0500 Bjorn Helgaas wrote:
> On Thu, Aug 21, 2014 at 4:34 PM, Bruno Prémont wrote:
> 
> > A second step would then be to tune vgaarb's initial selection.
> > Bjorn, is it possible to verify which I/O ports are decoded by a PCI
> > device at the time of adding it to vgaarb? If so, how? I would like to
> > check for legacy VGA I/O range (0x03B0-0x03DF) and only let vgaarb set
> > a device as default if that I/O range is decoded by the device.
> 
> I don't know of a way.  I'm pretty sure VGA devices are allowed to
> respond to those legacy addresses even if there's no BAR for them, but
> I haven't found a spec reference for this.  There is the VGA Enable
> bit in bridges, of course (PCI Bridge spec, sec 12.1.1.  If the VGA
> device is behind a bridge that doesn't have the VGA Enable bit set, it
> probably isn't the default device.

Those VGA devices behind bridges are the easy ones that vgaarb selects
properly.
It's the ones not behind a bridge (integrated graphics) like the intel
one that cause problems.

For Andreas's system the discrete nvidia GPU has no I/O enabled
according to PCI_COMMAND flags while the integrated intel one does have
them (that's why the Intel GPU is chosen).

Unfortunately I don't know what makes his system choke at boot time as
he did not provide logs for the failing case.


If there is no better way to detect the proper legacy VGA device the
only remaining option would be to perform the screen_info testing in 
vga_arb_device_init() enclosed in arch #ifdef...

Bruno

Re: [PATCH 0/2] x86, ia64: Move EFI_FB vga_default_device() initialization to pci_vga_fixup()

[Bruno Prémont]

On Fri, 22 August 2014 Andreas Noever <andreas.noever@gmail.com> wrote:
> > For Andreas's system the discrete nvidia GPU has no I/O enabled
> > according to PCI_COMMAND flags while the integrated intel one does have
> > them (that's why the Intel GPU is chosen).
> >
> > Unfortunately I don't know what makes his system choke at boot time as
> > he did not provide logs for the failing case.
> Attached dmesg for the failing case (obtained via ssh).
>
> Without blacklisting a small horizontal bar of vertical green bars
> appears (no x, no console).

It's good to know that it's just the graphics (console / X) that are not
displaying properly.

> If nouveau is blacklisted then I get a console, but X will not start
> (No devices found).

The console you get is EFIFB (on the nvidia GPU to which display is routed).

Here the reason why X does not start is probably that i915 did not find
its VBIOS tables nor any connected monitor and thus X thinks "no active
output => I don't start".
Though your X would be able to start if it did not find xf86-video-intel
(intel_drv.so) and/or did find/had an explicit reference to xf86-video-fbdev
(fbdev_drv.so).

If under OSX you told your system to start on intel GPU (I think there
is an option in this direction) you system would probably boot fine as the
initial choice by vgaarb would match gmux/switcheroo settings.

> If i915 is blacklisted then I do not get a console. The screen just
> freezes after a few boot messages.

This is more interesting.

Initially you had efifb printing kernel logs until nouveau gets loaded
by udev and replaces efifb. From there on possibly applegmux does not
take over correctly (it may need both i915 and nouveau active to properly
route framebuffer to panel or connector).

Though your X should be telling the same thing as for nouveau blacklisted
as nvidia GPU is not the one having boot_vga set...

If not it may be worth finding out in what state your system exactly is
with regards to graphics.

> What is vga_default_device() used for? Is it supposed to hold the
> device that is controlling the (boot) screen? Why can't we just read
> the configuration from vga_switcheroo/gmux?

For systems not using vga_switcheroo:
  vga_default_device represents the PCI GPU that was used to boot (and
  normally handles legacy VGA I/O).
  It's never changed after boot (except eventually when a GPU gets
  hotplugged)

For systems with vga_switcheroo
  vga_default_device represents the active GPU (the one that would be
  handling legacy VGA I/O if used - and the one controlling the output
  connectors)
  vga_switcheroo is actively changing vga_default_device.


gmux is a driver for vga_switcheroo to perform the low-level platform
operations allowing switching (outputs) from one GPU to the other.


So a guess on my side would be that with both i915 and nouveau loaded
you may be able to get your display working if you can tell X to
switch GPU twice (and thus end up with matching vga_default_device
and device selected by gmux) - though I don't know how one asks for this
switch to happen.

> > If there is no better way to detect the proper legacy VGA device the
> > only remaining option would be to perform the screen_info testing in
> > vga_arb_device_init() enclosed in arch #ifdef...

I will propose a patch in this direction later this weekend.

Bruno

[PATCH 1/2 v2] vgaarb: Don't default exclusively to first video device with mem+io

[Bruno Prémont]

With commit 20cde694027e boot video device detection was moved from
efifb to x86 and ia64 pci/fixup.c.

For dual-GPU Apple computers above change represents a regression as
code in efifb did forcefully override vga_default_device while the
merge did not (vgaarb happens prior to PCI fixup).

To improve on initial device selection by vgaarb (it cannot know if
PCI device not behind bridges see/decode legacy VGA I/O or not), move
the screen_info based check from pci_video_fixup to vgaarb's init
function and use it to refine/override decision taken while adding
the individual PCI VGA devices.
This way PCI fixup has no reason to adjust vga_default_device
anymore but can depend on its value for flagging shadowed VBIOS.

This has the nice benefit of removing duplicated code but does
introduce a #if defined() block in vgaarb.
Not all architectures have screen_info and would cause compile to
fail without it.

Reported-By: Andreas Noever <andreas.noever@gmail.com>
CC: Matthew Garrett <matthew.garrett@nebula.com>
CC: stable@vger.kernel.org # v3.5+
Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
---
Andreas, does this work properly for you, including the improvement
on i915 complaint about VBIOS going from KERN_ERR to KERN_INFO?


Other arches using PCI and vgaarb that have screen_info may want
to be added to the #if defined() block or even introduce a new
CONFIG_HAVE_SCREEN_INFO or similar...


 arch/ia64/pci/fixup.c    | 24 +-----------------------
 arch/x86/pci/fixup.c     | 24 +-----------------------
 drivers/gpu/vga/vgaarb.c | 38 +++++++++++++++++++++++++++++++++++++-
 3 files changed, 39 insertions(+), 47 deletions(-)

diff --git a/arch/ia64/pci/fixup.c b/arch/ia64/pci/fixup.c
index ec73b2c..fc505d5 100644
--- a/arch/ia64/pci/fixup.c
+++ b/arch/ia64/pci/fixup.c
@@ -38,27 +38,6 @@ static void pci_fixup_video(struct pci_dev *pdev)
 		return;
 	/* Maybe, this machine supports legacy memory map. */
 
-	if (!vga_default_device()) {
-		resource_size_t start, end;
-		int i;
-
-		/* Does firmware framebuffer belong to us? */
-		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) {
@@ -83,8 +62,7 @@ static void pci_fixup_video(struct pci_dev *pdev)
 		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");
-			vga_set_default_device(pdev);
+			dev_printk(KERN_DEBUG, &pdev->dev, "Video device with shadowed ROM\n");
 		}
 	}
 }
diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index c61ea57..9a2b710 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -326,27 +326,6 @@ 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;
-
-		/* Does firmware framebuffer belong to us? */
-		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) {
@@ -371,8 +350,7 @@ static void pci_fixup_video(struct pci_dev *pdev)
 		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");
-			vga_set_default_device(pdev);
+			dev_printk(KERN_DEBUG, &pdev->dev, "Video device with shadowed ROM\n");
 		}
 	}
 }
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index d2077f0..cffdff9 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -41,6 +41,7 @@
 #include <linux/poll.h>
 #include <linux/miscdevice.h>
 #include <linux/slab.h>
+#include <linux/screen_info.h>
 
 #include <linux/uaccess.h>
 
@@ -585,8 +586,11 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
 	 */
 #ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
 	if (vga_default == NULL &&
-	    ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK))
+	    ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) {
+		pr_info("vgaarb: setting as boot device: PCI:%s\n",
+			pci_name(pdev));
 		vga_set_default_device(pdev);
+	}
 #endif
 
 	vga_arbiter_check_bridge_sharing(vgadev);
@@ -1320,6 +1324,38 @@ static int __init vga_arb_device_init(void)
 	pr_info("vgaarb: loaded\n");
 
 	list_for_each_entry(vgadev, &vga_list, list) {
+#if defined(CONFIG_X86) || defined(CONFIG_IA64)
+		/* Override I/O based detection done by vga_arbiter_add_pci_device()
+		 * as it may take the wrong device (e.g. on Apple system under EFI).
+		 *
+		 * Select the device owning the boot framebuffer if there is one.
+		 */
+		resource_size_t start, end;
+		int i;
+
+		/* Does firmware framebuffer belong to us? */
+		for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
+			if (!(pci_resource_flags(vgadev->pdev, i) & IORESOURCE_MEM))
+				continue;
+
+			start = pci_resource_start(vgadev->pdev, i);
+			end  = pci_resource_end(vgadev->pdev, i);
+
+			if (!start || !end)
+				continue;
+
+			if (screen_info.lfb_base < start ||
+			    (screen_info.lfb_base + screen_info.lfb_size) >= end)
+				continue;
+			if (!vga_default_device())
+				pr_info("vgaarb: setting as boot device: PCI:%s\n",
+					pci_name(vgadev->pdev));
+			else if (vgadev->pdev != vga_default_device())
+				pr_info("vgaarb: overriding boot device: PCI:%s\n",
+					pci_name(vgadev->pdev));
+			vga_set_default_device(vgadev->pdev);
+		}
+#endif
 		if (vgadev->bridge_has_one_vga)
 			pr_info("vgaarb: bridge control possible %s\n", pci_name(vgadev->pdev));
 		else
-- 
1.8.5.5

[PATCH 2/2 v2] vgaarb: Drop obsolete #ifndef

[Bruno Prémont]

With commit 20cde694027e boot video device detection was moved from
efifb to x86 and ia64 pci/fixup.c.

Remove the left-over #ifndef check that will allways match since
the corresponding arch-specific define is gone with above patch.

Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
CC: Matthew Garrett <matthew.garrett@nebula.com>
---

No fundamental change from previous iteration, just reversed the
patch ordering and dropped the stable@ tag as this is just clean-up.


 drivers/gpu/vga/vgaarb.c | 8 --------
 include/linux/vgaarb.h   | 2 --
 2 files changed, 10 deletions(-)

diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index cffdff9..fb28314 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -113,10 +113,8 @@ both:
 	return 1;
 }
 
-#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
 /* this is only used a cookie - it should not be dereferenced */
 static struct pci_dev *vga_default;
-#endif
 
 static void vga_arb_device_card_gone(struct pci_dev *pdev);
 
@@ -132,7 +130,6 @@ static struct vga_device *vgadev_find(struct pci_dev *pdev)
 }
 
 /* Returns the default VGA device (vgacon's babe) */
-#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
 struct pci_dev *vga_default_device(void)
 {
 	return vga_default;
@@ -148,7 +145,6 @@ void vga_set_default_device(struct pci_dev *pdev)
 	pci_dev_put(vga_default);
 	vga_default = pci_dev_get(pdev);
 }
-#endif
 
 static inline void vga_irq_set_state(struct vga_device *vgadev, bool state)
 {
@@ -584,14 +580,12 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
 	/* Deal with VGA default device. Use first enabled one
 	 * by default if arch doesn't have it's own hook
 	 */
-#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
 	if (vga_default == NULL &&
 	    ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) {
 		pr_info("vgaarb: setting as boot device: PCI:%s\n",
 			pci_name(pdev));
 		vga_set_default_device(pdev);
 	}
-#endif
 
 	vga_arbiter_check_bridge_sharing(vgadev);
 
@@ -625,10 +619,8 @@ static bool vga_arbiter_del_pci_device(struct pci_dev *pdev)
 		goto bail;
 	}
 
-#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
 	if (vga_default == pdev)
 		vga_set_default_device(NULL);
-#endif
 
 	if (vgadev->decodes & (VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM))
 		vga_decode_count--;
diff --git a/include/linux/vgaarb.h b/include/linux/vgaarb.h
index 2c02f3a..c37bd4d 100644
--- a/include/linux/vgaarb.h
+++ b/include/linux/vgaarb.h
@@ -182,7 +182,6 @@ extern void vga_put(struct pci_dev *pdev, unsigned int rsrc);
  *     vga_get()...
  */
 
-#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
 #ifdef CONFIG_VGA_ARB
 extern struct pci_dev *vga_default_device(void);
 extern void vga_set_default_device(struct pci_dev *pdev);
@@ -190,7 +189,6 @@ extern void vga_set_default_device(struct pci_dev *pdev);
 static inline struct pci_dev *vga_default_device(void) { return NULL; };
 static inline void vga_set_default_device(struct pci_dev *pdev) { };
 #endif
-#endif
 
 /**
  *     vga_conflicts
-- 
1.8.5.5

Re: [PATCH 0/2] x86, ia64: Move EFI_FB vga_default_device() initialization to pci_vga_fixup()

[Daniel Vetter]

On Fri, Aug 22, 2014 at 08:23:24AM +0200, Bruno Prémont wrote:
> On Thu, 21 Aug 2014 23:39:31 -0500 Bjorn Helgaas wrote:
> > On Thu, Aug 21, 2014 at 4:34 PM, Bruno Prémont wrote:
> > 
> > > A second step would then be to tune vgaarb's initial selection.
> > > Bjorn, is it possible to verify which I/O ports are decoded by a PCI
> > > device at the time of adding it to vgaarb? If so, how? I would like to
> > > check for legacy VGA I/O range (0x03B0-0x03DF) and only let vgaarb set
> > > a device as default if that I/O range is decoded by the device.
> > 
> > I don't know of a way.  I'm pretty sure VGA devices are allowed to
> > respond to those legacy addresses even if there's no BAR for them, but
> > I haven't found a spec reference for this.  There is the VGA Enable
> > bit in bridges, of course (PCI Bridge spec, sec 12.1.1.  If the VGA
> > device is behind a bridge that doesn't have the VGA Enable bit set, it
> > probably isn't the default device.
> 
> Those VGA devices behind bridges are the easy ones that vgaarb selects
> properly.
> It's the ones not behind a bridge (integrated graphics) like the intel
> one that cause problems.
> 
> For Andreas's system the discrete nvidia GPU has no I/O enabled
> according to PCI_COMMAND flags while the integrated intel one does have
> them (that's why the Intel GPU is chosen).
> 
> Unfortunately I don't know what makes his system choke at boot time as
> he did not provide logs for the failing case.

Very often when something goes wrong with a kms driver we hang while doing
the initial modeset. Which is all done while holding the console_lock
(because fbdev+vt locking is just insane). You can try to get a closer
look with I915_FBDEV=n which will avoid the console_lock, but which also
won't register the legacy/compat i915 fbdev emulation any more, so greatly
changes boot behaviour.

If that doesn't lead to clues the next approach is to "carefully"
drop&reacquire console_lock at a few "interesting" places to get a few
printks out over netconsole or similar. Or just hack up entire netconsole
loggin infrastructure which bypasses printk and so all the console_lock
insanity.

It's not pretty, I know :(

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 0/2] x86, ia64: Move EFI_FB vga_default_device() initialization to pci_vga_fixup()

[Bruno Prémont]

Hi Daniel,

On Mon, 25 Aug 2014 14:16:02 +0200 Daniel Vetter wrote:
> Very often when something goes wrong with a kms driver we hang while doing
> the initial modeset. Which is all done while holding the console_lock
> (because fbdev+vt locking is just insane). You can try to get a closer
> look with I915_FBDEV=n which will avoid the console_lock, but which also
> won't register the legacy/compat i915 fbdev emulation any more, so greatly
> changes boot behaviour.
> 
> If that doesn't lead to clues the next approach is to "carefully"
> drop&reacquire console_lock at a few "interesting" places to get a few
> printks out over netconsole or similar. Or just hack up entire netconsole
> loggin infrastructure which bypasses printk and so all the console_lock
> insanity.

In this case it's not that bad as Andreas could send the logs for all
cases (captured via ssh).

So probably console lock is not held (unless he did have to do
terminal-free ssh which I doubt).
It looks much more as if it's just the output routing that gets weird
on his Mac (or possibly any other dual-GPU MacBook where discrete GPU is
primary). Black screen but alive system :)

See follow-up posts in this thread.

If you have some uncommon or otherwise weird (EFI) multi-GPU systems
around and want to give my patches sent yesterday evening a try, you're
welcome! Some with non-Apple GPU multiplexer would be nice to have
tested as well.


The following part mentioned earlier by Andreas might be of interest to
you though (and my latest patch series should bring the improvement):
> > vga_arbiter_add_pci_device chooses intel simply because it is the
> > first device. Next pci_fixup_video(intel) sees that it is the default
> > device, sets the IORESOURCE_ROM_SHADOW flag and calls
> > vga_set_default_device again. And finally (if the check is removed)
> > pci_fixup_video(nvidia) sees that it owns the framebuffer and sets
> > itself as the default device which allows the system to boot again.
> >
> > Does setting the ROM_SHADOW flag on (possibly) the wrong device have
> > any effect?  
> Yes it does. Removing the line changes a long standing
> i915 0000:00:02.0: Invalid ROM contents
> into a
> i915 0000:00:02.0: BAR 6: can't assign [??? 0x00000000 flags 0x20000000] (bogus alignment).
> 
> The first is logged at KERN_ERR and the second one only at KERN_INFO.
> We are making progress.

Bruno

Re: [PATCH 1/2 v2] vgaarb: Don't default exclusively to first video device with mem+io

[Andreas Noever]

On Sun, Aug 24, 2014 at 11:09 PM, Bruno Prémont
<bonbons@linux-vserver.org> wrote:
> With commit 20cde694027e boot video device detection was moved from
> efifb to x86 and ia64 pci/fixup.c.
>
> For dual-GPU Apple computers above change represents a regression as
> code in efifb did forcefully override vga_default_device while the
> merge did not (vgaarb happens prior to PCI fixup).
>
> To improve on initial device selection by vgaarb (it cannot know if
> PCI device not behind bridges see/decode legacy VGA I/O or not), move
> the screen_info based check from pci_video_fixup to vgaarb's init
> function and use it to refine/override decision taken while adding
> the individual PCI VGA devices.
> This way PCI fixup has no reason to adjust vga_default_device
> anymore but can depend on its value for flagging shadowed VBIOS.
>
> This has the nice benefit of removing duplicated code but does
> introduce a #if defined() block in vgaarb.
> Not all architectures have screen_info and would cause compile to
> fail without it.
>
> Reported-By: Andreas Noever <andreas.noever@gmail.com>
> CC: Matthew Garrett <matthew.garrett@nebula.com>
> CC: stable@vger.kernel.org # v3.5+
> Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> ---
> Andreas, does this work properly for you, including the improvement
> on i915 complaint about VBIOS going from KERN_ERR to KERN_INFO?
Yep, thanks!

>
> Other arches using PCI and vgaarb that have screen_info may want
> to be added to the #if defined() block or even introduce a new
> CONFIG_HAVE_SCREEN_INFO or similar...
>
>
>  arch/ia64/pci/fixup.c    | 24 +-----------------------
>  arch/x86/pci/fixup.c     | 24 +-----------------------
>  drivers/gpu/vga/vgaarb.c | 38 +++++++++++++++++++++++++++++++++++++-
>  3 files changed, 39 insertions(+), 47 deletions(-)
>
> diff --git a/arch/ia64/pci/fixup.c b/arch/ia64/pci/fixup.c
> index ec73b2c..fc505d5 100644
> --- a/arch/ia64/pci/fixup.c
> +++ b/arch/ia64/pci/fixup.c
> @@ -38,27 +38,6 @@ static void pci_fixup_video(struct pci_dev *pdev)
>                 return;
>         /* Maybe, this machine supports legacy memory map. */
>
> -       if (!vga_default_device()) {
> -               resource_size_t start, end;
> -               int i;
> -
> -               /* Does firmware framebuffer belong to us? */
> -               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) {
> @@ -83,8 +62,7 @@ static void pci_fixup_video(struct pci_dev *pdev)
>                 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");
> -                       vga_set_default_device(pdev);
> +                       dev_printk(KERN_DEBUG, &pdev->dev, "Video device with shadowed ROM\n");
>                 }
>         }
>  }
> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> index c61ea57..9a2b710 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -326,27 +326,6 @@ 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;
> -
> -               /* Does firmware framebuffer belong to us? */
> -               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) {
> @@ -371,8 +350,7 @@ static void pci_fixup_video(struct pci_dev *pdev)
>                 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");
> -                       vga_set_default_device(pdev);
> +                       dev_printk(KERN_DEBUG, &pdev->dev, "Video device with shadowed ROM\n");
>                 }
>         }
>  }
> diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> index d2077f0..cffdff9 100644
> --- a/drivers/gpu/vga/vgaarb.c
> +++ b/drivers/gpu/vga/vgaarb.c
> @@ -41,6 +41,7 @@
>  #include <linux/poll.h>
>  #include <linux/miscdevice.h>
>  #include <linux/slab.h>
> +#include <linux/screen_info.h>
>
>  #include <linux/uaccess.h>
>
> @@ -585,8 +586,11 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
>          */
>  #ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
>         if (vga_default == NULL &&
> -           ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK))
> +           ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) {
> +               pr_info("vgaarb: setting as boot device: PCI:%s\n",
> +                       pci_name(pdev));
>                 vga_set_default_device(pdev);
> +       }
>  #endif
>
>         vga_arbiter_check_bridge_sharing(vgadev);
> @@ -1320,6 +1324,38 @@ static int __init vga_arb_device_init(void)
>         pr_info("vgaarb: loaded\n");
>
>         list_for_each_entry(vgadev, &vga_list, list) {
> +#if defined(CONFIG_X86) || defined(CONFIG_IA64)
> +               /* Override I/O based detection done by vga_arbiter_add_pci_device()
> +                * as it may take the wrong device (e.g. on Apple system under EFI).
> +                *
> +                * Select the device owning the boot framebuffer if there is one.
> +                */
> +               resource_size_t start, end;
> +               int i;
> +
> +               /* Does firmware framebuffer belong to us? */
> +               for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> +                       if (!(pci_resource_flags(vgadev->pdev, i) & IORESOURCE_MEM))
> +                               continue;
> +
> +                       start = pci_resource_start(vgadev->pdev, i);
> +                       end  = pci_resource_end(vgadev->pdev, i);
> +
> +                       if (!start || !end)
> +                               continue;
> +
> +                       if (screen_info.lfb_base < start ||
> +                           (screen_info.lfb_base + screen_info.lfb_size) >= end)
> +                               continue;
> +                       if (!vga_default_device())
> +                               pr_info("vgaarb: setting as boot device: PCI:%s\n",
> +                                       pci_name(vgadev->pdev));
> +                       else if (vgadev->pdev != vga_default_device())
> +                               pr_info("vgaarb: overriding boot device: PCI:%s\n",
> +                                       pci_name(vgadev->pdev));
> +                       vga_set_default_device(vgadev->pdev);
> +               }
> +#endif
>                 if (vgadev->bridge_has_one_vga)
>                         pr_info("vgaarb: bridge control possible %s\n", pci_name(vgadev->pdev));
>                 else
> --
> 1.8.5.5
>

Re: [PATCH 1/2 v2] vgaarb: Don't default exclusively to first video device with mem+io

[Bruno Prémont]

On Tue, 26 August 2014 Andreas Noever <andreas.noever@gmail.com> wrote:
> On Sun, Aug 24, 2014 at 11:09 PM, Bruno Prémont wrote:
> > With commit 20cde694027e boot video device detection was moved from
> > efifb to x86 and ia64 pci/fixup.c.
> >
> > For dual-GPU Apple computers above change represents a regression as
> > code in efifb did forcefully override vga_default_device while the
> > merge did not (vgaarb happens prior to PCI fixup).
> >
> > To improve on initial device selection by vgaarb (it cannot know if
> > PCI device not behind bridges see/decode legacy VGA I/O or not), move
> > the screen_info based check from pci_video_fixup to vgaarb's init
> > function and use it to refine/override decision taken while adding
> > the individual PCI VGA devices.
> > This way PCI fixup has no reason to adjust vga_default_device
> > anymore but can depend on its value for flagging shadowed VBIOS.
> >
> > This has the nice benefit of removing duplicated code but does
> > introduce a #if defined() block in vgaarb.
> > Not all architectures have screen_info and would cause compile to
> > fail without it.
> >
> > Reported-By: Andreas Noever <andreas.noever@gmail.com>
> > CC: Matthew Garrett <matthew.garrett@nebula.com>
> > CC: stable@vger.kernel.org # v3.5+
> > Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> > ---
> > Andreas, does this work properly for you, including the improvement
> > on i915 complaint about VBIOS going from KERN_ERR to KERN_INFO?
> Yep, thanks!
> 
> >
> > Other arches using PCI and vgaarb that have screen_info may want
> > to be added to the #if defined() block or even introduce a new
> > CONFIG_HAVE_SCREEN_INFO or similar...

Bjorn, can you queue these two patches, probably going through -next
for a week and passing them to Linus for -rc4, adding Andreas's Tested-by
to Patch 1/2 v2?

Thanks,
Bruno

Re: [PATCH 1/2 v2] vgaarb: Don't default exclusively to first video device with mem+io

[Bruno Prémont]

Bjorn,

What is missing to get these two patches pushed to Linus?

Bruno


On Thu, 28 Aug 2014 22:47:50 +0200 Bruno Prémont wrote:
> On Tue, 26 August 2014 Andreas Noever <andreas.noever@gmail.com> wrote:
> > On Sun, Aug 24, 2014 at 11:09 PM, Bruno Prémont wrote:
> > > With commit 20cde694027e boot video device detection was moved from
> > > efifb to x86 and ia64 pci/fixup.c.
> > >
> > > For dual-GPU Apple computers above change represents a regression as
> > > code in efifb did forcefully override vga_default_device while the
> > > merge did not (vgaarb happens prior to PCI fixup).
> > >
> > > To improve on initial device selection by vgaarb (it cannot know if
> > > PCI device not behind bridges see/decode legacy VGA I/O or not), move
> > > the screen_info based check from pci_video_fixup to vgaarb's init
> > > function and use it to refine/override decision taken while adding
> > > the individual PCI VGA devices.
> > > This way PCI fixup has no reason to adjust vga_default_device
> > > anymore but can depend on its value for flagging shadowed VBIOS.
> > >
> > > This has the nice benefit of removing duplicated code but does
> > > introduce a #if defined() block in vgaarb.
> > > Not all architectures have screen_info and would cause compile to
> > > fail without it.
> > >
> > > Reported-By: Andreas Noever <andreas.noever@gmail.com>
> > > CC: Matthew Garrett <matthew.garrett@nebula.com>
> > > CC: stable@vger.kernel.org # v3.5+
> > > Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> > > ---
> > > Andreas, does this work properly for you, including the improvement
> > > on i915 complaint about VBIOS going from KERN_ERR to KERN_INFO?
> > Yep, thanks!
> > 
> > >
> > > Other arches using PCI and vgaarb that have screen_info may want
> > > to be added to the #if defined() block or even introduce a new
> > > CONFIG_HAVE_SCREEN_INFO or similar...
> 
> Bjorn, can you queue these two patches, probably going through -next
> for a week and passing them to Linus for -rc4, adding Andreas's Tested-by
> to Patch 1/2 v2?
> 
> Thanks,
> Bruno

Re: [PATCH 1/2 v2] vgaarb: Don't default exclusively to first video device with mem+io

[Bjorn Helgaas]

On Fri, Sep 12, 2014 at 5:19 AM, Bruno Prémont
<bonbons@linux-vserver.org> wrote:
> Bjorn,
>
> What is missing to get these two patches pushed to Linus?

Sorry, I've been working on some other regressions and overlooked this
one.  If you open a bugzilla report and mark it as a regression, that
will help keep this on my radar.

Bjorn

> On Thu, 28 Aug 2014 22:47:50 +0200 Bruno Prémont wrote:
>> On Tue, 26 August 2014 Andreas Noever <andreas.noever@gmail.com> wrote:
>> > On Sun, Aug 24, 2014 at 11:09 PM, Bruno Prémont wrote:
>> > > With commit 20cde694027e boot video device detection was moved from
>> > > efifb to x86 and ia64 pci/fixup.c.
>> > >
>> > > For dual-GPU Apple computers above change represents a regression as
>> > > code in efifb did forcefully override vga_default_device while the
>> > > merge did not (vgaarb happens prior to PCI fixup).
>> > >
>> > > To improve on initial device selection by vgaarb (it cannot know if
>> > > PCI device not behind bridges see/decode legacy VGA I/O or not), move
>> > > the screen_info based check from pci_video_fixup to vgaarb's init
>> > > function and use it to refine/override decision taken while adding
>> > > the individual PCI VGA devices.
>> > > This way PCI fixup has no reason to adjust vga_default_device
>> > > anymore but can depend on its value for flagging shadowed VBIOS.
>> > >
>> > > This has the nice benefit of removing duplicated code but does
>> > > introduce a #if defined() block in vgaarb.
>> > > Not all architectures have screen_info and would cause compile to
>> > > fail without it.
>> > >
>> > > Reported-By: Andreas Noever <andreas.noever@gmail.com>
>> > > CC: Matthew Garrett <matthew.garrett@nebula.com>
>> > > CC: stable@vger.kernel.org # v3.5+
>> > > Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
>> > > ---
>> > > Andreas, does this work properly for you, including the improvement
>> > > on i915 complaint about VBIOS going from KERN_ERR to KERN_INFO?
>> > Yep, thanks!
>> >
>> > >
>> > > Other arches using PCI and vgaarb that have screen_info may want
>> > > to be added to the #if defined() block or even introduce a new
>> > > CONFIG_HAVE_SCREEN_INFO or similar...
>>
>> Bjorn, can you queue these two patches, probably going through -next
>> for a week and passing them to Linus for -rc4, adding Andreas's Tested-by
>> to Patch 1/2 v2?
>>
>> Thanks,
>> Bruno

Re: [PATCH 1/2 v2] vgaarb: Don't default exclusively to first video device with mem+io

[Dave Airlie]

On 13 September 2014 00:28, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Fri, Sep 12, 2014 at 5:19 AM, Bruno Prémont
> <bonbons@linux-vserver.org> wrote:
>> Bjorn,
>>
>> What is missing to get these two patches pushed to Linus?
>
> Sorry, I've been working on some other regressions and overlooked this
> one.  If you open a bugzilla report and mark it as a regression, that
> will help keep this on my radar.

My MBP is crying? can we get these merged, I can handle if if you want
to ack them for me!

Dave.

Re: [PATCH 1/2 v2] vgaarb: Don't default exclusively to first video device with mem+io

[Bjorn Helgaas]

On Fri, Sep 19, 2014 at 09:26:58AM +1000, Dave Airlie wrote:
> On 13 September 2014 00:28, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > On Fri, Sep 12, 2014 at 5:19 AM, Bruno Prémont
> > <bonbons@linux-vserver.org> wrote:
> >> Bjorn,
> >>
> >> What is missing to get these two patches pushed to Linus?
> >
> > Sorry, I've been working on some other regressions and overlooked this
> > one.  If you open a bugzilla report and mark it as a regression, that
> > will help keep this on my radar.
> 
> My MBP is crying? can we get these merged, I can handle if if you want
> to ack them for me!

Yep, it's in linux-next and I'll send Linus a pull request for the tag [1]
tomorrow.  I meant to send it tonight, but I had to fix my for-linus branch
to remove a duplicate commit and I don't want to send the pull request
immediately afterwards.

Bjorn

[1] https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/tag/?h=for-linus&id=pci-v3.17-fixes-2