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

Hi

On Sat, Nov 30, 2013 at 2:52 PM, Bruno Prémont bonbons@linux-vserver.org wrote:

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

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

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

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

Notes:

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

Signed-off-by: Bruno Prémont bonbons@linux-vserver.org

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

diff --git a/arch/x86/include/asm/vga.h b/arch/x86/include/asm/vga.h index 44282fb..c4b9dc2 100644 --- a/arch/x86/include/asm/vga.h +++ b/arch/x86/include/asm/vga.h @@ -17,10 +17,4 @@ #define vga_readb(x) (*(x)) #define vga_writeb(x, y) (*(y) = (x))

-#ifdef CONFIG_FB_EFI -#define __ARCH_HAS_VGA_DEFAULT_DEVICE -extern struct pci_dev *vga_default_device(void); -extern void vga_set_default_device(struct pci_dev *pdev); -#endif

#endif /* _ASM_X86_VGA_H */ diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c index f5809fa..440343e 100644 --- a/arch/x86/pci/fixup.c +++ b/arch/x86/pci/fixup.c @@ -6,6 +6,7 @@ #include <linux/dmi.h> #include <linux/pci.h> #include <linux/init.h> +#include <linux/screen_info.h> #include <linux/vgaarb.h> #include <asm/pci_x86.h>

@@ -323,6 +324,26 @@ static void pci_fixup_video(struct pci_dev *pdev) struct pci_bus *bus; u16 config;

•   if (!vga_default_device()) {
  

•           resource_size_t start, end;
  

•           int i;
  

•           for (i=0; i < DEVICE_COUNT_RESOURCE; i++) {
  

•                   if (!(pci_resource_flags(pdev, i) & IORESOURCE_MEM))
  

•                           continue;
  

•                   start = pci_resource_start(pdev, i);
  

•                   end  = pci_resource_end(pdev, i);
  

•                   if (!start || !end)
  

•                           continue;
  

•                   if (screen_info.lfb_base >= start &&
  

•                           (screen_info.lfb_base + screen_info.lfb_size) < end)
  

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

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

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

•                           vga_set_default_device(pdev);
  

•           }
  

•   }
  

•   /* Is VGA routed to us? */
    bus = pdev->bus;
    while (bus) {
  

diff --git a/drivers/video/efifb.c b/drivers/video/efifb.c index 7f9ff75..fb3fb50 100644 --- a/drivers/video/efifb.c +++ b/drivers/video/efifb.c @@ -19,8 +19,6 @@

static bool request_mem_succeeded = false;

-static struct pci_dev *default_vga;

static struct fb_var_screeninfo efifb_defined = { .activate = FB_ACTIVATE_NOW, .height = -1, @@ -85,18 +83,6 @@ static struct fb_ops efifb_ops = { .fb_imageblit = cfb_imageblit, };

-struct pci_dev *vga_default_device(void) -{

•   return default_vga;
  

-}

-EXPORT_SYMBOL_GPL(vga_default_device);

-void vga_set_default_device(struct pci_dev *pdev) -{

•   default_vga = pdev;
  

-}

static int efifb_setup(char *options) { char *this_opt; @@ -127,30 +113,6 @@ static int efifb_setup(char *options) } }

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

Otherwise, the patch looks good to me. Thanks David

•   for_each_pci_dev(dev) {
  

•           int i;
  

•           if ((dev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
  

•                   continue;
  

•           for (i=0; i < DEVICE_COUNT_RESOURCE; i++) {
  

•                   resource_size_t start, end;
  

•                   if (!(pci_resource_flags(dev, i) & IORESOURCE_MEM))
  

•                           continue;
  

•                   start = pci_resource_start(dev, i);
  

•                   end  = pci_resource_end(dev, i);
  

•                   if (!start || !end)
  

•                           continue;
  

•                   if (screen_info.lfb_base >= start &&
  

•                       (screen_info.lfb_base + screen_info.lfb_size) < end)
  

•                           default_vga = dev;
  

•           }
  

•   }
  

•   return 0;
  

}