[PATCH v2 2/2] vgaarb: Factor out EFI and fallback default device selection

[Daniel Vetter]

On Tue, Oct 17, 2017 at 01:03:46PM +1100, Daniel Axtens wrote:
> Bjorn Helgaas <bhelgaas at google.com> writes:
> 
> > The default VGA device is normally set in vga_arbiter_add_pci_device() when
> > we call it for the first enabled device that can be accessed with the
> > legacy VGA resources ([mem 0xa0000-0xbffff], etc.)
> >
> > That default device can be overridden by an EFI device that owns the boot
> > framebuffer.  As a fallback, we can also select a VGA device that can't be
> > accessed via legacy VGA resources, or a VGA device that isn't even enabled.
> >
> > Factor out this EFI and fallback selection from vga_arb_device_init() into
> > a separate vga_arb_select_default_device() function.  This doesn't change
> > any behavior, but it untangles the "bridge control possible" checking and
> > messages from the default device selection.
> >
> > Tested-by: Zhou Wang <wangzhou1 at hisilicon.com>  # D05 Hisi Hip07, Hip08
> > Signed-off-by: Bjorn Helgaas <bhelgaas at google.com>
> > ---
> >  drivers/gpu/vga/vgaarb.c |   57 ++++++++++++++++++++++++++++------------------
> >  1 file changed, 35 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> > index 8035e38d5110..d35d6d271f3f 100644
> > --- a/drivers/gpu/vga/vgaarb.c
> > +++ b/drivers/gpu/vga/vgaarb.c
> > @@ -1402,29 +1402,14 @@ static struct miscdevice vga_arb_device = {
> >  	MISC_DYNAMIC_MINOR, "vga_arbiter", &vga_arb_device_fops
> >  };
> >  
> > -static int __init vga_arb_device_init(void)
> > +static void __init vga_arb_select_default_device(void)
> >  {
> > -	int rc;
> >  	struct pci_dev *pdev;
> >  	struct vga_device *vgadev;
> >  
> > -	rc = misc_register(&vga_arb_device);
> > -	if (rc < 0)
> > -		pr_err("error %d registering device\n", rc);
> > -
> > -	bus_register_notifier(&pci_bus_type, &pci_notifier);
> > -
> > -	/* We add all pci devices satisfying vga class in the arbiter by
> > -	 * default */
> > -	pdev = NULL;
> > -	while ((pdev =
> > -		pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
> > -			       PCI_ANY_ID, pdev)) != NULL)
> > -		vga_arbiter_add_pci_device(pdev);
> > -
> > +#if defined(CONFIG_X86) || defined(CONFIG_IA64)
> >  	list_for_each_entry(vgadev, &vga_list, list) {
> >  		struct device *dev = &vgadev->pdev->dev;
> > -#if defined(CONFIG_X86) || defined(CONFIG_IA64)
> >  		/*
> >  		 * Override vga_arbiter_add_pci_device()'s I/O based detection
> >  		 * as it may take the wrong device (e.g. on Apple system under
> > @@ -1461,12 +1446,8 @@ static int __init vga_arb_device_init(void)
> >  				vgaarb_info(dev, "overriding boot device\n");
> >  			vga_set_default_device(vgadev->pdev);
> >  		}
> > -#endif
> > -		if (vgadev->bridge_has_one_vga)
> > -			vgaarb_info(dev, "bridge control possible\n");
> > -		else
> > -			vgaarb_info(dev, "no bridge control possible\n");
> >  	}
> > +#endif
> >  
> >  	if (!vga_default_device()) {
> >  		list_for_each_entry(vgadev, &vga_list, list) {
> > @@ -1492,6 +1473,38 @@ static int __init vga_arb_device_init(void)
> >  			vga_set_default_device(vgadev->pdev);
> >  		}
> >  	}
> > +}
> > +
> > +static int __init vga_arb_device_init(void)
> > +{
> > +	int rc;
> > +	struct pci_dev *pdev;
> > +	struct vga_device *vgadev;
> > +
> > +	rc = misc_register(&vga_arb_device);
> > +	if (rc < 0)
> > +		pr_err("error %d registering device\n", rc);
> > +
> > +	bus_register_notifier(&pci_bus_type, &pci_notifier);
> > +
> > +	/* We add all PCI devices satisfying VGA class in the arbiter by
> > +	 * default */
> > +	pdev = NULL;
> > +	while ((pdev =
> > +		pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
> > +			       PCI_ANY_ID, pdev)) != NULL)
> > +		vga_arbiter_add_pci_device(pdev);
> > +
> > +	list_for_each_entry(vgadev, &vga_list, list) {
> > +		struct device *dev = &vgadev->pdev->dev;
> > +
> > +		if (vgadev->bridge_has_one_vga)
> > +			vgaarb_info(dev, "bridge control possible\n");
> > +		else
> > +			vgaarb_info(dev, "no bridge control possible\n");
> > +	}
> 
> Initially I wondered if this info printk could be moved into
> vga_arbiter_check_bridge_sharing(), but it's been separated out since
> 3448a19da479b ("vgaarb: use bridges to control VGA routing where
> possible."), and upon closer examination, it seems you can't be sure a
> device doesn't share a bridge until the end of the process, so this is
> indeed correct.
> 
> Everything else also looks good to me.
> 
> Reviewed-by: Daniel Axtens <dja at axtens.net>

R-b for both patches? And ok with everyone if I pull this into drm-misc
for 4.15 (deadline is end of this week for feature-y stuff)?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch