[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 at 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