| Message ID | 20140810183411.19370721@neptune.home (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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.
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
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