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

[Ard Biesheuvel]

On 17 October 2017 at 13:05, Daniel Vetter <daniel at ffwll.ch> wrote:
> 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)?
>

For both patches

Acked-by: Ard Biesheuvel <ard.biesheuvel at linaro.org>