* Re: [PATCH] vgaarb: Rework default VGA device selection
2021-07-05 10:05 [PATCH] vgaarb: Rework default VGA device selection Huacai Chen
@ 2021-07-06 8:21 ` Daniel Vetter
2021-07-06 9:21 ` Huacai Chen
2021-07-07 18:40 ` Bjorn Helgaas
2021-07-20 22:19 ` Bjorn Helgaas
2 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2021-07-06 8:21 UTC (permalink / raw)
To: Huacai Chen; +Cc: David Airlie, Bjorn Helgaas, Xuefeng Li, dri-devel
On Mon, Jul 05, 2021 at 06:05:03PM +0800, Huacai Chen wrote:
> Currently, vga_arb_device_init() selects the first probed VGA device
> with VGA legacy resources enabled as the default device. However, some
> BMC-based VGA cards (e.g., AST2500 and HiSilicon D05) don't enable VGA
> legacy resources because their built-in upstream bridges don't support
> PCI_BRIDGE_CTL_VGA. This makes "no default VGA card" and breaks X.org
> auto-detection.
>
> Commit a37c0f48950b56f6ef2e ("vgaarb: Select a default VGA device even
> if there's no legacy VGA") try to solve this problem but fails on some
> platforms, because it relies on the initcall order:
>
> We should call vga_arb_device_init() after PCI enumeration, otherwise it
> may fail to select the default VGA device. Since vga_arb_device_init()
> and PCI enumeration function (i.e., pcibios_init() or acpi_init()) are
> both wrapped by subsys_initcall(), their sequence is not assured. So, it
> is possible to use subsys_initcall_sync() instead of subsys_initcall()
> to wrap vga_arb_device_init().
>
> However, the above approach still has drawbacks, it cannot handle the
> cases that a VGA card is hot-plugged, or the gpu driver is compiled as a
> module.
>
> So, as suggested by Bjorn Helgaas, this patch rework the selection:
> 1, Remove direct vga_arb_select_default_device() calls in vga_arb_
> device_init().
> 2, Rename vga_arb_select_default_device() to vga_arb_update_default_
> device(), which selects the first probed VGA device as the default
> (whether legacy resources enabled or not), and update the default
> device if a better one is found (device with legacy resources enabled
> is better, device owns the firmware framebuffer is even better).
> 3, Every time a new VGA device is probed, vga_arbiter_add_pci_device()
> is called, and vga_arb_update_default_device() is also called. So the
> hotplug case and the module case can also be handled.
>
> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
Sounds reasonable, but because this is defacto pci stuff that just isn't
in drivers/pci (maybe we should move it) I'll defer to Bjorn's review
before I apply this patch.
-Daniel
> ---
> drivers/gpu/vga/vgaarb.c | 219 +++++++++++++++++----------------------
> 1 file changed, 97 insertions(+), 122 deletions(-)
>
> diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> index 949fde433ea2..07770aae3aaf 100644
> --- a/drivers/gpu/vga/vgaarb.c
> +++ b/drivers/gpu/vga/vgaarb.c
> @@ -586,6 +586,97 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc)
> }
> EXPORT_SYMBOL(vga_put);
>
> +#if defined(CONFIG_ACPI)
> +static bool vga_arb_integrated_gpu(struct device *dev)
> +{
> + struct acpi_device *adev = ACPI_COMPANION(dev);
> +
> + return adev && !strcmp(acpi_device_hid(adev), ACPI_VIDEO_HID);
> +}
> +#else
> +static bool vga_arb_integrated_gpu(struct device *dev)
> +{
> + return false;
> +}
> +#endif
> +
> +static void vga_arb_update_default_device(struct vga_device *vgadev)
> +{
> + struct pci_dev *pdev = vgadev->pdev;
> + struct device *dev = &pdev->dev;
> + struct vga_device *vgadev_default;
> +#if defined(CONFIG_X86) || defined(CONFIG_IA64)
> + int i;
> + unsigned long flags;
> + u64 base = screen_info.lfb_base;
> + u64 size = screen_info.lfb_size;
> + u64 limit;
> + resource_size_t start, end;
> +#endif
> +
> + /* Deal with VGA default device. Use first enabled one
> + * by default if arch doesn't have it's own hook
> + */
> + if (!vga_default_device()) {
> + if ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)
> + vgaarb_info(dev, "setting as boot VGA device\n");
> + else
> + vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n");
> + vga_set_default_device(pdev);
> + }
> +
> + vgadev_default = vgadev_find(vga_default);
> +
> + /* Overridden by a better device */
> + if (vgadev_default && ((vgadev_default->owns & VGA_RSRC_LEGACY_MASK) == 0)
> + && ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) {
> + vgaarb_info(dev, "overriding boot VGA device\n");
> + vga_set_default_device(pdev);
> + }
> +
> + if (vga_arb_integrated_gpu(dev)) {
> + vgaarb_info(dev, "overriding boot VGA device\n");
> + vga_set_default_device(pdev);
> + }
> +
> +#if defined(CONFIG_X86) || defined(CONFIG_IA64)
> + if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
> + base |= (u64)screen_info.ext_lfb_base << 32;
> +
> + limit = base + size;
> +
> + /*
> + * Override vga_arbiter_add_pci_device()'s I/O based detection
> + * as it may take the wrong device (e.g. on Apple system under
> + * EFI).
> + *
> + * Select the device owning the boot framebuffer if there is
> + * one.
> + */
> +
> + /* Does firmware framebuffer belong to us? */
> + for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> + flags = pci_resource_flags(vgadev->pdev, i);
> +
> + if ((flags & IORESOURCE_MEM) == 0)
> + continue;
> +
> + start = pci_resource_start(vgadev->pdev, i);
> + end = pci_resource_end(vgadev->pdev, i);
> +
> + if (!start || !end)
> + continue;
> +
> + if (base < start || limit >= end)
> + continue;
> +
> + if (vgadev->pdev != vga_default_device())
> + vgaarb_info(dev, "overriding boot device\n");
> + vga_set_default_device(vgadev->pdev);
> + }
> +#endif
> +}
> +
> /*
> * Rules for using a bridge to control a VGA descendant decoding: if a bridge
> * has only one VGA descendant then it can be used to control the VGA routing
> @@ -643,6 +734,11 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev)
> }
> new_bus = new_bus->parent;
> }
> +
> + if (vgadev->bridge_has_one_vga == true)
> + vgaarb_info(&vgadev->pdev->dev, "bridge control possible\n");
> + else
> + vgaarb_info(&vgadev->pdev->dev, "no bridge control possible\n");
> }
>
> /*
> @@ -713,15 +809,7 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
> bus = bus->parent;
> }
>
> - /* Deal with VGA default device. Use first enabled one
> - * by default if arch doesn't have it's own hook
> - */
> - if (vga_default == NULL &&
> - ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) {
> - vgaarb_info(&pdev->dev, "setting as boot VGA device\n");
> - vga_set_default_device(pdev);
> - }
> -
> + vga_arb_update_default_device(vgadev);
> vga_arbiter_check_bridge_sharing(vgadev);
>
> /* Add to the list */
> @@ -1451,111 +1539,10 @@ static struct miscdevice vga_arb_device = {
> MISC_DYNAMIC_MINOR, "vga_arbiter", &vga_arb_device_fops
> };
>
> -#if defined(CONFIG_ACPI)
> -static bool vga_arb_integrated_gpu(struct device *dev)
> -{
> - struct acpi_device *adev = ACPI_COMPANION(dev);
> -
> - return adev && !strcmp(acpi_device_hid(adev), ACPI_VIDEO_HID);
> -}
> -#else
> -static bool vga_arb_integrated_gpu(struct device *dev)
> -{
> - return false;
> -}
> -#endif
> -
> -static void __init vga_arb_select_default_device(void)
> -{
> - struct pci_dev *pdev, *found = NULL;
> - struct vga_device *vgadev;
> -
> -#if defined(CONFIG_X86) || defined(CONFIG_IA64)
> - u64 base = screen_info.lfb_base;
> - u64 size = screen_info.lfb_size;
> - u64 limit;
> - resource_size_t start, end;
> - unsigned long flags;
> - int i;
> -
> - if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
> - base |= (u64)screen_info.ext_lfb_base << 32;
> -
> - limit = base + size;
> -
> - list_for_each_entry(vgadev, &vga_list, list) {
> - struct device *dev = &vgadev->pdev->dev;
> - /*
> - * Override vga_arbiter_add_pci_device()'s I/O based detection
> - * as it may take the wrong device (e.g. on Apple system under
> - * EFI).
> - *
> - * Select the device owning the boot framebuffer if there is
> - * one.
> - */
> -
> - /* Does firmware framebuffer belong to us? */
> - for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> - flags = pci_resource_flags(vgadev->pdev, i);
> -
> - if ((flags & IORESOURCE_MEM) == 0)
> - continue;
> -
> - start = pci_resource_start(vgadev->pdev, i);
> - end = pci_resource_end(vgadev->pdev, i);
> -
> - if (!start || !end)
> - continue;
> -
> - if (base < start || limit >= end)
> - continue;
> -
> - if (!vga_default_device())
> - vgaarb_info(dev, "setting as boot device\n");
> - else if (vgadev->pdev != vga_default_device())
> - vgaarb_info(dev, "overriding boot device\n");
> - vga_set_default_device(vgadev->pdev);
> - }
> - }
> -#endif
> -
> - if (!vga_default_device()) {
> - list_for_each_entry_reverse(vgadev, &vga_list, list) {
> - struct device *dev = &vgadev->pdev->dev;
> - u16 cmd;
> -
> - pdev = vgadev->pdev;
> - pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> - if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
> - found = pdev;
> - if (vga_arb_integrated_gpu(dev))
> - break;
> - }
> - }
> - }
> -
> - if (found) {
> - vgaarb_info(&found->dev, "setting as boot device (VGA legacy resources not available)\n");
> - vga_set_default_device(found);
> - return;
> - }
> -
> - if (!vga_default_device()) {
> - vgadev = list_first_entry_or_null(&vga_list,
> - struct vga_device, list);
> - if (vgadev) {
> - struct device *dev = &vgadev->pdev->dev;
> - vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n");
> - 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)
> @@ -1571,18 +1558,6 @@ static int __init vga_arb_device_init(void)
> 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");
> - }
> -
> - vga_arb_select_default_device();
> -
> - pr_info("loaded\n");
> return rc;
> }
> subsys_initcall(vga_arb_device_init);
> --
> 2.27.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] vgaarb: Rework default VGA device selection
2021-07-06 8:21 ` Daniel Vetter
@ 2021-07-06 9:21 ` Huacai Chen
2021-07-07 11:43 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Huacai Chen @ 2021-07-06 9:21 UTC (permalink / raw)
To: Daniel Vetter
Cc: David Airlie, Bjorn Helgaas, Xuefeng Li,
Maling list - DRI developers, Huacai Chen
Hi, Daniel,
On Tue, Jul 6, 2021 at 4:21 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Mon, Jul 05, 2021 at 06:05:03PM +0800, Huacai Chen wrote:
> > Currently, vga_arb_device_init() selects the first probed VGA device
> > with VGA legacy resources enabled as the default device. However, some
> > BMC-based VGA cards (e.g., AST2500 and HiSilicon D05) don't enable VGA
> > legacy resources because their built-in upstream bridges don't support
> > PCI_BRIDGE_CTL_VGA. This makes "no default VGA card" and breaks X.org
> > auto-detection.
> >
> > Commit a37c0f48950b56f6ef2e ("vgaarb: Select a default VGA device even
> > if there's no legacy VGA") try to solve this problem but fails on some
> > platforms, because it relies on the initcall order:
> >
> > We should call vga_arb_device_init() after PCI enumeration, otherwise it
> > may fail to select the default VGA device. Since vga_arb_device_init()
> > and PCI enumeration function (i.e., pcibios_init() or acpi_init()) are
> > both wrapped by subsys_initcall(), their sequence is not assured. So, it
> > is possible to use subsys_initcall_sync() instead of subsys_initcall()
> > to wrap vga_arb_device_init().
> >
> > However, the above approach still has drawbacks, it cannot handle the
> > cases that a VGA card is hot-plugged, or the gpu driver is compiled as a
> > module.
> >
> > So, as suggested by Bjorn Helgaas, this patch rework the selection:
> > 1, Remove direct vga_arb_select_default_device() calls in vga_arb_
> > device_init().
> > 2, Rename vga_arb_select_default_device() to vga_arb_update_default_
> > device(), which selects the first probed VGA device as the default
> > (whether legacy resources enabled or not), and update the default
> > device if a better one is found (device with legacy resources enabled
> > is better, device owns the firmware framebuffer is even better).
> > 3, Every time a new VGA device is probed, vga_arbiter_add_pci_device()
> > is called, and vga_arb_update_default_device() is also called. So the
> > hotplug case and the module case can also be handled.
> >
> > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
>
> Sounds reasonable, but because this is defacto pci stuff that just isn't
> in drivers/pci (maybe we should move it) I'll defer to Bjorn's review
> before I apply this patch.
> -Daniel
This (move to drivers/pci) has been disscussed before, I think that
can be another patch.
Huacai
>
> > ---
> > drivers/gpu/vga/vgaarb.c | 219 +++++++++++++++++----------------------
> > 1 file changed, 97 insertions(+), 122 deletions(-)
> >
> > diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> > index 949fde433ea2..07770aae3aaf 100644
> > --- a/drivers/gpu/vga/vgaarb.c
> > +++ b/drivers/gpu/vga/vgaarb.c
> > @@ -586,6 +586,97 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc)
> > }
> > EXPORT_SYMBOL(vga_put);
> >
> > +#if defined(CONFIG_ACPI)
> > +static bool vga_arb_integrated_gpu(struct device *dev)
> > +{
> > + struct acpi_device *adev = ACPI_COMPANION(dev);
> > +
> > + return adev && !strcmp(acpi_device_hid(adev), ACPI_VIDEO_HID);
> > +}
> > +#else
> > +static bool vga_arb_integrated_gpu(struct device *dev)
> > +{
> > + return false;
> > +}
> > +#endif
> > +
> > +static void vga_arb_update_default_device(struct vga_device *vgadev)
> > +{
> > + struct pci_dev *pdev = vgadev->pdev;
> > + struct device *dev = &pdev->dev;
> > + struct vga_device *vgadev_default;
> > +#if defined(CONFIG_X86) || defined(CONFIG_IA64)
> > + int i;
> > + unsigned long flags;
> > + u64 base = screen_info.lfb_base;
> > + u64 size = screen_info.lfb_size;
> > + u64 limit;
> > + resource_size_t start, end;
> > +#endif
> > +
> > + /* Deal with VGA default device. Use first enabled one
> > + * by default if arch doesn't have it's own hook
> > + */
> > + if (!vga_default_device()) {
> > + if ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)
> > + vgaarb_info(dev, "setting as boot VGA device\n");
> > + else
> > + vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n");
> > + vga_set_default_device(pdev);
> > + }
> > +
> > + vgadev_default = vgadev_find(vga_default);
> > +
> > + /* Overridden by a better device */
> > + if (vgadev_default && ((vgadev_default->owns & VGA_RSRC_LEGACY_MASK) == 0)
> > + && ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) {
> > + vgaarb_info(dev, "overriding boot VGA device\n");
> > + vga_set_default_device(pdev);
> > + }
> > +
> > + if (vga_arb_integrated_gpu(dev)) {
> > + vgaarb_info(dev, "overriding boot VGA device\n");
> > + vga_set_default_device(pdev);
> > + }
> > +
> > +#if defined(CONFIG_X86) || defined(CONFIG_IA64)
> > + if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
> > + base |= (u64)screen_info.ext_lfb_base << 32;
> > +
> > + limit = base + size;
> > +
> > + /*
> > + * Override vga_arbiter_add_pci_device()'s I/O based detection
> > + * as it may take the wrong device (e.g. on Apple system under
> > + * EFI).
> > + *
> > + * Select the device owning the boot framebuffer if there is
> > + * one.
> > + */
> > +
> > + /* Does firmware framebuffer belong to us? */
> > + for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> > + flags = pci_resource_flags(vgadev->pdev, i);
> > +
> > + if ((flags & IORESOURCE_MEM) == 0)
> > + continue;
> > +
> > + start = pci_resource_start(vgadev->pdev, i);
> > + end = pci_resource_end(vgadev->pdev, i);
> > +
> > + if (!start || !end)
> > + continue;
> > +
> > + if (base < start || limit >= end)
> > + continue;
> > +
> > + if (vgadev->pdev != vga_default_device())
> > + vgaarb_info(dev, "overriding boot device\n");
> > + vga_set_default_device(vgadev->pdev);
> > + }
> > +#endif
> > +}
> > +
> > /*
> > * Rules for using a bridge to control a VGA descendant decoding: if a bridge
> > * has only one VGA descendant then it can be used to control the VGA routing
> > @@ -643,6 +734,11 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev)
> > }
> > new_bus = new_bus->parent;
> > }
> > +
> > + if (vgadev->bridge_has_one_vga == true)
> > + vgaarb_info(&vgadev->pdev->dev, "bridge control possible\n");
> > + else
> > + vgaarb_info(&vgadev->pdev->dev, "no bridge control possible\n");
> > }
> >
> > /*
> > @@ -713,15 +809,7 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
> > bus = bus->parent;
> > }
> >
> > - /* Deal with VGA default device. Use first enabled one
> > - * by default if arch doesn't have it's own hook
> > - */
> > - if (vga_default == NULL &&
> > - ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) {
> > - vgaarb_info(&pdev->dev, "setting as boot VGA device\n");
> > - vga_set_default_device(pdev);
> > - }
> > -
> > + vga_arb_update_default_device(vgadev);
> > vga_arbiter_check_bridge_sharing(vgadev);
> >
> > /* Add to the list */
> > @@ -1451,111 +1539,10 @@ static struct miscdevice vga_arb_device = {
> > MISC_DYNAMIC_MINOR, "vga_arbiter", &vga_arb_device_fops
> > };
> >
> > -#if defined(CONFIG_ACPI)
> > -static bool vga_arb_integrated_gpu(struct device *dev)
> > -{
> > - struct acpi_device *adev = ACPI_COMPANION(dev);
> > -
> > - return adev && !strcmp(acpi_device_hid(adev), ACPI_VIDEO_HID);
> > -}
> > -#else
> > -static bool vga_arb_integrated_gpu(struct device *dev)
> > -{
> > - return false;
> > -}
> > -#endif
> > -
> > -static void __init vga_arb_select_default_device(void)
> > -{
> > - struct pci_dev *pdev, *found = NULL;
> > - struct vga_device *vgadev;
> > -
> > -#if defined(CONFIG_X86) || defined(CONFIG_IA64)
> > - u64 base = screen_info.lfb_base;
> > - u64 size = screen_info.lfb_size;
> > - u64 limit;
> > - resource_size_t start, end;
> > - unsigned long flags;
> > - int i;
> > -
> > - if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
> > - base |= (u64)screen_info.ext_lfb_base << 32;
> > -
> > - limit = base + size;
> > -
> > - list_for_each_entry(vgadev, &vga_list, list) {
> > - struct device *dev = &vgadev->pdev->dev;
> > - /*
> > - * Override vga_arbiter_add_pci_device()'s I/O based detection
> > - * as it may take the wrong device (e.g. on Apple system under
> > - * EFI).
> > - *
> > - * Select the device owning the boot framebuffer if there is
> > - * one.
> > - */
> > -
> > - /* Does firmware framebuffer belong to us? */
> > - for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> > - flags = pci_resource_flags(vgadev->pdev, i);
> > -
> > - if ((flags & IORESOURCE_MEM) == 0)
> > - continue;
> > -
> > - start = pci_resource_start(vgadev->pdev, i);
> > - end = pci_resource_end(vgadev->pdev, i);
> > -
> > - if (!start || !end)
> > - continue;
> > -
> > - if (base < start || limit >= end)
> > - continue;
> > -
> > - if (!vga_default_device())
> > - vgaarb_info(dev, "setting as boot device\n");
> > - else if (vgadev->pdev != vga_default_device())
> > - vgaarb_info(dev, "overriding boot device\n");
> > - vga_set_default_device(vgadev->pdev);
> > - }
> > - }
> > -#endif
> > -
> > - if (!vga_default_device()) {
> > - list_for_each_entry_reverse(vgadev, &vga_list, list) {
> > - struct device *dev = &vgadev->pdev->dev;
> > - u16 cmd;
> > -
> > - pdev = vgadev->pdev;
> > - pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> > - if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
> > - found = pdev;
> > - if (vga_arb_integrated_gpu(dev))
> > - break;
> > - }
> > - }
> > - }
> > -
> > - if (found) {
> > - vgaarb_info(&found->dev, "setting as boot device (VGA legacy resources not available)\n");
> > - vga_set_default_device(found);
> > - return;
> > - }
> > -
> > - if (!vga_default_device()) {
> > - vgadev = list_first_entry_or_null(&vga_list,
> > - struct vga_device, list);
> > - if (vgadev) {
> > - struct device *dev = &vgadev->pdev->dev;
> > - vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n");
> > - 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)
> > @@ -1571,18 +1558,6 @@ static int __init vga_arb_device_init(void)
> > 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");
> > - }
> > -
> > - vga_arb_select_default_device();
> > -
> > - pr_info("loaded\n");
> > return rc;
> > }
> > subsys_initcall(vga_arb_device_init);
> > --
> > 2.27.0
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] vgaarb: Rework default VGA device selection
2021-07-06 9:21 ` Huacai Chen
@ 2021-07-07 11:43 ` Daniel Vetter
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2021-07-07 11:43 UTC (permalink / raw)
To: Huacai Chen
Cc: David Airlie, Maling list - DRI developers, Bjorn Helgaas,
Xuefeng Li, Huacai Chen
On Tue, Jul 06, 2021 at 05:21:42PM +0800, Huacai Chen wrote:
> Hi, Daniel,
>
> On Tue, Jul 6, 2021 at 4:21 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Mon, Jul 05, 2021 at 06:05:03PM +0800, Huacai Chen wrote:
> > > Currently, vga_arb_device_init() selects the first probed VGA device
> > > with VGA legacy resources enabled as the default device. However, some
> > > BMC-based VGA cards (e.g., AST2500 and HiSilicon D05) don't enable VGA
> > > legacy resources because their built-in upstream bridges don't support
> > > PCI_BRIDGE_CTL_VGA. This makes "no default VGA card" and breaks X.org
> > > auto-detection.
> > >
> > > Commit a37c0f48950b56f6ef2e ("vgaarb: Select a default VGA device even
> > > if there's no legacy VGA") try to solve this problem but fails on some
> > > platforms, because it relies on the initcall order:
> > >
> > > We should call vga_arb_device_init() after PCI enumeration, otherwise it
> > > may fail to select the default VGA device. Since vga_arb_device_init()
> > > and PCI enumeration function (i.e., pcibios_init() or acpi_init()) are
> > > both wrapped by subsys_initcall(), their sequence is not assured. So, it
> > > is possible to use subsys_initcall_sync() instead of subsys_initcall()
> > > to wrap vga_arb_device_init().
> > >
> > > However, the above approach still has drawbacks, it cannot handle the
> > > cases that a VGA card is hot-plugged, or the gpu driver is compiled as a
> > > module.
> > >
> > > So, as suggested by Bjorn Helgaas, this patch rework the selection:
> > > 1, Remove direct vga_arb_select_default_device() calls in vga_arb_
> > > device_init().
> > > 2, Rename vga_arb_select_default_device() to vga_arb_update_default_
> > > device(), which selects the first probed VGA device as the default
> > > (whether legacy resources enabled or not), and update the default
> > > device if a better one is found (device with legacy resources enabled
> > > is better, device owns the firmware framebuffer is even better).
> > > 3, Every time a new VGA device is probed, vga_arbiter_add_pci_device()
> > > is called, and vga_arb_update_default_device() is also called. So the
> > > hotplug case and the module case can also be handled.
> > >
> > > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> >
> > Sounds reasonable, but because this is defacto pci stuff that just isn't
> > in drivers/pci (maybe we should move it) I'll defer to Bjorn's review
> > before I apply this patch.
> > -Daniel
> This (move to drivers/pci) has been disscussed before, I think that
> can be another patch.
Oh definitely, I just wanted to explain why I won't simply apply this
without Bjorn's review.
-Daniel
>
> Huacai
> >
> > > ---
> > > drivers/gpu/vga/vgaarb.c | 219 +++++++++++++++++----------------------
> > > 1 file changed, 97 insertions(+), 122 deletions(-)
> > >
> > > diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> > > index 949fde433ea2..07770aae3aaf 100644
> > > --- a/drivers/gpu/vga/vgaarb.c
> > > +++ b/drivers/gpu/vga/vgaarb.c
> > > @@ -586,6 +586,97 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc)
> > > }
> > > EXPORT_SYMBOL(vga_put);
> > >
> > > +#if defined(CONFIG_ACPI)
> > > +static bool vga_arb_integrated_gpu(struct device *dev)
> > > +{
> > > + struct acpi_device *adev = ACPI_COMPANION(dev);
> > > +
> > > + return adev && !strcmp(acpi_device_hid(adev), ACPI_VIDEO_HID);
> > > +}
> > > +#else
> > > +static bool vga_arb_integrated_gpu(struct device *dev)
> > > +{
> > > + return false;
> > > +}
> > > +#endif
> > > +
> > > +static void vga_arb_update_default_device(struct vga_device *vgadev)
> > > +{
> > > + struct pci_dev *pdev = vgadev->pdev;
> > > + struct device *dev = &pdev->dev;
> > > + struct vga_device *vgadev_default;
> > > +#if defined(CONFIG_X86) || defined(CONFIG_IA64)
> > > + int i;
> > > + unsigned long flags;
> > > + u64 base = screen_info.lfb_base;
> > > + u64 size = screen_info.lfb_size;
> > > + u64 limit;
> > > + resource_size_t start, end;
> > > +#endif
> > > +
> > > + /* Deal with VGA default device. Use first enabled one
> > > + * by default if arch doesn't have it's own hook
> > > + */
> > > + if (!vga_default_device()) {
> > > + if ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)
> > > + vgaarb_info(dev, "setting as boot VGA device\n");
> > > + else
> > > + vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n");
> > > + vga_set_default_device(pdev);
> > > + }
> > > +
> > > + vgadev_default = vgadev_find(vga_default);
> > > +
> > > + /* Overridden by a better device */
> > > + if (vgadev_default && ((vgadev_default->owns & VGA_RSRC_LEGACY_MASK) == 0)
> > > + && ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) {
> > > + vgaarb_info(dev, "overriding boot VGA device\n");
> > > + vga_set_default_device(pdev);
> > > + }
> > > +
> > > + if (vga_arb_integrated_gpu(dev)) {
> > > + vgaarb_info(dev, "overriding boot VGA device\n");
> > > + vga_set_default_device(pdev);
> > > + }
> > > +
> > > +#if defined(CONFIG_X86) || defined(CONFIG_IA64)
> > > + if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
> > > + base |= (u64)screen_info.ext_lfb_base << 32;
> > > +
> > > + limit = base + size;
> > > +
> > > + /*
> > > + * Override vga_arbiter_add_pci_device()'s I/O based detection
> > > + * as it may take the wrong device (e.g. on Apple system under
> > > + * EFI).
> > > + *
> > > + * Select the device owning the boot framebuffer if there is
> > > + * one.
> > > + */
> > > +
> > > + /* Does firmware framebuffer belong to us? */
> > > + for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> > > + flags = pci_resource_flags(vgadev->pdev, i);
> > > +
> > > + if ((flags & IORESOURCE_MEM) == 0)
> > > + continue;
> > > +
> > > + start = pci_resource_start(vgadev->pdev, i);
> > > + end = pci_resource_end(vgadev->pdev, i);
> > > +
> > > + if (!start || !end)
> > > + continue;
> > > +
> > > + if (base < start || limit >= end)
> > > + continue;
> > > +
> > > + if (vgadev->pdev != vga_default_device())
> > > + vgaarb_info(dev, "overriding boot device\n");
> > > + vga_set_default_device(vgadev->pdev);
> > > + }
> > > +#endif
> > > +}
> > > +
> > > /*
> > > * Rules for using a bridge to control a VGA descendant decoding: if a bridge
> > > * has only one VGA descendant then it can be used to control the VGA routing
> > > @@ -643,6 +734,11 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev)
> > > }
> > > new_bus = new_bus->parent;
> > > }
> > > +
> > > + if (vgadev->bridge_has_one_vga == true)
> > > + vgaarb_info(&vgadev->pdev->dev, "bridge control possible\n");
> > > + else
> > > + vgaarb_info(&vgadev->pdev->dev, "no bridge control possible\n");
> > > }
> > >
> > > /*
> > > @@ -713,15 +809,7 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
> > > bus = bus->parent;
> > > }
> > >
> > > - /* Deal with VGA default device. Use first enabled one
> > > - * by default if arch doesn't have it's own hook
> > > - */
> > > - if (vga_default == NULL &&
> > > - ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) {
> > > - vgaarb_info(&pdev->dev, "setting as boot VGA device\n");
> > > - vga_set_default_device(pdev);
> > > - }
> > > -
> > > + vga_arb_update_default_device(vgadev);
> > > vga_arbiter_check_bridge_sharing(vgadev);
> > >
> > > /* Add to the list */
> > > @@ -1451,111 +1539,10 @@ static struct miscdevice vga_arb_device = {
> > > MISC_DYNAMIC_MINOR, "vga_arbiter", &vga_arb_device_fops
> > > };
> > >
> > > -#if defined(CONFIG_ACPI)
> > > -static bool vga_arb_integrated_gpu(struct device *dev)
> > > -{
> > > - struct acpi_device *adev = ACPI_COMPANION(dev);
> > > -
> > > - return adev && !strcmp(acpi_device_hid(adev), ACPI_VIDEO_HID);
> > > -}
> > > -#else
> > > -static bool vga_arb_integrated_gpu(struct device *dev)
> > > -{
> > > - return false;
> > > -}
> > > -#endif
> > > -
> > > -static void __init vga_arb_select_default_device(void)
> > > -{
> > > - struct pci_dev *pdev, *found = NULL;
> > > - struct vga_device *vgadev;
> > > -
> > > -#if defined(CONFIG_X86) || defined(CONFIG_IA64)
> > > - u64 base = screen_info.lfb_base;
> > > - u64 size = screen_info.lfb_size;
> > > - u64 limit;
> > > - resource_size_t start, end;
> > > - unsigned long flags;
> > > - int i;
> > > -
> > > - if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
> > > - base |= (u64)screen_info.ext_lfb_base << 32;
> > > -
> > > - limit = base + size;
> > > -
> > > - list_for_each_entry(vgadev, &vga_list, list) {
> > > - struct device *dev = &vgadev->pdev->dev;
> > > - /*
> > > - * Override vga_arbiter_add_pci_device()'s I/O based detection
> > > - * as it may take the wrong device (e.g. on Apple system under
> > > - * EFI).
> > > - *
> > > - * Select the device owning the boot framebuffer if there is
> > > - * one.
> > > - */
> > > -
> > > - /* Does firmware framebuffer belong to us? */
> > > - for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> > > - flags = pci_resource_flags(vgadev->pdev, i);
> > > -
> > > - if ((flags & IORESOURCE_MEM) == 0)
> > > - continue;
> > > -
> > > - start = pci_resource_start(vgadev->pdev, i);
> > > - end = pci_resource_end(vgadev->pdev, i);
> > > -
> > > - if (!start || !end)
> > > - continue;
> > > -
> > > - if (base < start || limit >= end)
> > > - continue;
> > > -
> > > - if (!vga_default_device())
> > > - vgaarb_info(dev, "setting as boot device\n");
> > > - else if (vgadev->pdev != vga_default_device())
> > > - vgaarb_info(dev, "overriding boot device\n");
> > > - vga_set_default_device(vgadev->pdev);
> > > - }
> > > - }
> > > -#endif
> > > -
> > > - if (!vga_default_device()) {
> > > - list_for_each_entry_reverse(vgadev, &vga_list, list) {
> > > - struct device *dev = &vgadev->pdev->dev;
> > > - u16 cmd;
> > > -
> > > - pdev = vgadev->pdev;
> > > - pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> > > - if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
> > > - found = pdev;
> > > - if (vga_arb_integrated_gpu(dev))
> > > - break;
> > > - }
> > > - }
> > > - }
> > > -
> > > - if (found) {
> > > - vgaarb_info(&found->dev, "setting as boot device (VGA legacy resources not available)\n");
> > > - vga_set_default_device(found);
> > > - return;
> > > - }
> > > -
> > > - if (!vga_default_device()) {
> > > - vgadev = list_first_entry_or_null(&vga_list,
> > > - struct vga_device, list);
> > > - if (vgadev) {
> > > - struct device *dev = &vgadev->pdev->dev;
> > > - vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n");
> > > - 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)
> > > @@ -1571,18 +1558,6 @@ static int __init vga_arb_device_init(void)
> > > 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");
> > > - }
> > > -
> > > - vga_arb_select_default_device();
> > > -
> > > - pr_info("loaded\n");
> > > return rc;
> > > }
> > > subsys_initcall(vga_arb_device_init);
> > > --
> > > 2.27.0
> > >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vgaarb: Rework default VGA device selection
2021-07-05 10:05 [PATCH] vgaarb: Rework default VGA device selection Huacai Chen
2021-07-06 8:21 ` Daniel Vetter
@ 2021-07-07 18:40 ` Bjorn Helgaas
2021-07-20 22:19 ` Bjorn Helgaas
2 siblings, 0 replies; 6+ messages in thread
From: Bjorn Helgaas @ 2021-07-07 18:40 UTC (permalink / raw)
To: Huacai Chen; +Cc: David Airlie, Linux PCI, Xuefeng Li, DRI mailing list
[+cc linux-pci]
On Mon, Jul 5, 2021 at 5:04 AM Huacai Chen <chenhuacai@loongson.cn> wrote:
>
> Currently, vga_arb_device_init() selects the first probed VGA device
> with VGA legacy resources enabled as the default device. However, some
> BMC-based VGA cards (e.g., AST2500 and HiSilicon D05) don't enable VGA
> legacy resources because their built-in upstream bridges don't support
> PCI_BRIDGE_CTL_VGA. This makes "no default VGA card" and breaks X.org
> auto-detection.
>
> Commit a37c0f48950b56f6ef2e ("vgaarb: Select a default VGA device even
> if there's no legacy VGA") try to solve this problem but fails on some
> platforms, because it relies on the initcall order:
>
> We should call vga_arb_device_init() after PCI enumeration, otherwise it
> may fail to select the default VGA device. Since vga_arb_device_init()
> and PCI enumeration function (i.e., pcibios_init() or acpi_init()) are
> both wrapped by subsys_initcall(), their sequence is not assured. So, it
> is possible to use subsys_initcall_sync() instead of subsys_initcall()
> to wrap vga_arb_device_init().
>
> However, the above approach still has drawbacks, it cannot handle the
> cases that a VGA card is hot-plugged, or the gpu driver is compiled as a
> module.
>
> So, as suggested by Bjorn Helgaas, this patch rework the selection:
> 1, Remove direct vga_arb_select_default_device() calls in vga_arb_
> device_init().
> 2, Rename vga_arb_select_default_device() to vga_arb_update_default_
> device(), which selects the first probed VGA device as the default
> (whether legacy resources enabled or not), and update the default
> device if a better one is found (device with legacy resources enabled
> is better, device owns the firmware framebuffer is even better).
> 3, Every time a new VGA device is probed, vga_arbiter_add_pci_device()
> is called, and vga_arb_update_default_device() is also called. So the
> hotplug case and the module case can also be handled.
>
> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> ---
> drivers/gpu/vga/vgaarb.c | 219 +++++++++++++++++----------------------
> 1 file changed, 97 insertions(+), 122 deletions(-)
>
> diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> index 949fde433ea2..07770aae3aaf 100644
> --- a/drivers/gpu/vga/vgaarb.c
> +++ b/drivers/gpu/vga/vgaarb.c
> @@ -586,6 +586,97 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc)
> }
> EXPORT_SYMBOL(vga_put);
>
> +#if defined(CONFIG_ACPI)
> +static bool vga_arb_integrated_gpu(struct device *dev)
> +{
> + struct acpi_device *adev = ACPI_COMPANION(dev);
> +
> + return adev && !strcmp(acpi_device_hid(adev), ACPI_VIDEO_HID);
> +}
> +#else
> +static bool vga_arb_integrated_gpu(struct device *dev)
> +{
> + return false;
> +}
> +#endif
> +
> +static void vga_arb_update_default_device(struct vga_device *vgadev)
> +{
> + struct pci_dev *pdev = vgadev->pdev;
> + struct device *dev = &pdev->dev;
> + struct vga_device *vgadev_default;
> +#if defined(CONFIG_X86) || defined(CONFIG_IA64)
> + int i;
> + unsigned long flags;
> + u64 base = screen_info.lfb_base;
> + u64 size = screen_info.lfb_size;
> + u64 limit;
> + resource_size_t start, end;
> +#endif
> +
> + /* Deal with VGA default device. Use first enabled one
> + * by default if arch doesn't have it's own hook
> + */
> + if (!vga_default_device()) {
> + if ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)
> + vgaarb_info(dev, "setting as boot VGA device\n");
> + else
> + vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n");
> + vga_set_default_device(pdev);
> + }
> +
> + vgadev_default = vgadev_find(vga_default);
> +
> + /* Overridden by a better device */
> + if (vgadev_default && ((vgadev_default->owns & VGA_RSRC_LEGACY_MASK) == 0)
> + && ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) {
> + vgaarb_info(dev, "overriding boot VGA device\n");
> + vga_set_default_device(pdev);
> + }
> +
> + if (vga_arb_integrated_gpu(dev)) {
> + vgaarb_info(dev, "overriding boot VGA device\n");
> + vga_set_default_device(pdev);
> + }
> +
> +#if defined(CONFIG_X86) || defined(CONFIG_IA64)
> + if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
> + base |= (u64)screen_info.ext_lfb_base << 32;
> +
> + limit = base + size;
> +
> + /*
> + * Override vga_arbiter_add_pci_device()'s I/O based detection
> + * as it may take the wrong device (e.g. on Apple system under
> + * EFI).
> + *
> + * Select the device owning the boot framebuffer if there is
> + * one.
> + */
> +
> + /* Does firmware framebuffer belong to us? */
> + for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> + flags = pci_resource_flags(vgadev->pdev, i);
> +
> + if ((flags & IORESOURCE_MEM) == 0)
> + continue;
> +
> + start = pci_resource_start(vgadev->pdev, i);
> + end = pci_resource_end(vgadev->pdev, i);
> +
> + if (!start || !end)
> + continue;
> +
> + if (base < start || limit >= end)
> + continue;
> +
> + if (vgadev->pdev != vga_default_device())
> + vgaarb_info(dev, "overriding boot device\n");
> + vga_set_default_device(vgadev->pdev);
> + }
> +#endif
> +}
> +
> /*
> * Rules for using a bridge to control a VGA descendant decoding: if a bridge
> * has only one VGA descendant then it can be used to control the VGA routing
> @@ -643,6 +734,11 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev)
> }
> new_bus = new_bus->parent;
> }
> +
> + if (vgadev->bridge_has_one_vga == true)
> + vgaarb_info(&vgadev->pdev->dev, "bridge control possible\n");
> + else
> + vgaarb_info(&vgadev->pdev->dev, "no bridge control possible\n");
> }
>
> /*
> @@ -713,15 +809,7 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
> bus = bus->parent;
> }
>
> - /* Deal with VGA default device. Use first enabled one
> - * by default if arch doesn't have it's own hook
> - */
> - if (vga_default == NULL &&
> - ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) {
> - vgaarb_info(&pdev->dev, "setting as boot VGA device\n");
> - vga_set_default_device(pdev);
> - }
> -
> + vga_arb_update_default_device(vgadev);
> vga_arbiter_check_bridge_sharing(vgadev);
>
> /* Add to the list */
> @@ -1451,111 +1539,10 @@ static struct miscdevice vga_arb_device = {
> MISC_DYNAMIC_MINOR, "vga_arbiter", &vga_arb_device_fops
> };
>
> -#if defined(CONFIG_ACPI)
> -static bool vga_arb_integrated_gpu(struct device *dev)
> -{
> - struct acpi_device *adev = ACPI_COMPANION(dev);
> -
> - return adev && !strcmp(acpi_device_hid(adev), ACPI_VIDEO_HID);
> -}
> -#else
> -static bool vga_arb_integrated_gpu(struct device *dev)
> -{
> - return false;
> -}
> -#endif
> -
> -static void __init vga_arb_select_default_device(void)
> -{
> - struct pci_dev *pdev, *found = NULL;
> - struct vga_device *vgadev;
> -
> -#if defined(CONFIG_X86) || defined(CONFIG_IA64)
> - u64 base = screen_info.lfb_base;
> - u64 size = screen_info.lfb_size;
> - u64 limit;
> - resource_size_t start, end;
> - unsigned long flags;
> - int i;
> -
> - if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
> - base |= (u64)screen_info.ext_lfb_base << 32;
> -
> - limit = base + size;
> -
> - list_for_each_entry(vgadev, &vga_list, list) {
> - struct device *dev = &vgadev->pdev->dev;
> - /*
> - * Override vga_arbiter_add_pci_device()'s I/O based detection
> - * as it may take the wrong device (e.g. on Apple system under
> - * EFI).
> - *
> - * Select the device owning the boot framebuffer if there is
> - * one.
> - */
> -
> - /* Does firmware framebuffer belong to us? */
> - for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> - flags = pci_resource_flags(vgadev->pdev, i);
> -
> - if ((flags & IORESOURCE_MEM) == 0)
> - continue;
> -
> - start = pci_resource_start(vgadev->pdev, i);
> - end = pci_resource_end(vgadev->pdev, i);
> -
> - if (!start || !end)
> - continue;
> -
> - if (base < start || limit >= end)
> - continue;
> -
> - if (!vga_default_device())
> - vgaarb_info(dev, "setting as boot device\n");
> - else if (vgadev->pdev != vga_default_device())
> - vgaarb_info(dev, "overriding boot device\n");
> - vga_set_default_device(vgadev->pdev);
> - }
> - }
> -#endif
> -
> - if (!vga_default_device()) {
> - list_for_each_entry_reverse(vgadev, &vga_list, list) {
> - struct device *dev = &vgadev->pdev->dev;
> - u16 cmd;
> -
> - pdev = vgadev->pdev;
> - pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> - if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
> - found = pdev;
> - if (vga_arb_integrated_gpu(dev))
> - break;
> - }
> - }
> - }
> -
> - if (found) {
> - vgaarb_info(&found->dev, "setting as boot device (VGA legacy resources not available)\n");
> - vga_set_default_device(found);
> - return;
> - }
> -
> - if (!vga_default_device()) {
> - vgadev = list_first_entry_or_null(&vga_list,
> - struct vga_device, list);
> - if (vgadev) {
> - struct device *dev = &vgadev->pdev->dev;
> - vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n");
> - 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)
> @@ -1571,18 +1558,6 @@ static int __init vga_arb_device_init(void)
> 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");
> - }
> -
> - vga_arb_select_default_device();
> -
> - pr_info("loaded\n");
> return rc;
> }
> subsys_initcall(vga_arb_device_init);
> --
> 2.27.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] vgaarb: Rework default VGA device selection
2021-07-05 10:05 [PATCH] vgaarb: Rework default VGA device selection Huacai Chen
2021-07-06 8:21 ` Daniel Vetter
2021-07-07 18:40 ` Bjorn Helgaas
@ 2021-07-20 22:19 ` Bjorn Helgaas
2 siblings, 0 replies; 6+ messages in thread
From: Bjorn Helgaas @ 2021-07-20 22:19 UTC (permalink / raw)
To: Huacai Chen; +Cc: David Airlie, linux-pci, dri-devel, Bjorn Helgaas, Xuefeng Li
On Mon, Jul 05, 2021 at 06:05:03PM +0800, Huacai Chen wrote:
> Currently, vga_arb_device_init() selects the first probed VGA device
> with VGA legacy resources enabled as the default device. However, some
> BMC-based VGA cards (e.g., AST2500 and HiSilicon D05) don't enable VGA
> legacy resources because their built-in upstream bridges don't support
> PCI_BRIDGE_CTL_VGA. This makes "no default VGA card" and breaks X.org
> auto-detection.
>
> Commit a37c0f48950b56f6ef2e ("vgaarb: Select a default VGA device even
> if there's no legacy VGA") try to solve this problem but fails on some
> platforms, because it relies on the initcall order:
>
> We should call vga_arb_device_init() after PCI enumeration, otherwise it
> may fail to select the default VGA device. Since vga_arb_device_init()
> and PCI enumeration function (i.e., pcibios_init() or acpi_init()) are
> both wrapped by subsys_initcall(), their sequence is not assured. So, it
> is possible to use subsys_initcall_sync() instead of subsys_initcall()
> to wrap vga_arb_device_init().
>
> However, the above approach still has drawbacks, it cannot handle the
> cases that a VGA card is hot-plugged, or the gpu driver is compiled as a
> module.
>
> So, as suggested by Bjorn Helgaas, this patch rework the selection:
> 1, Remove direct vga_arb_select_default_device() calls in vga_arb_
> device_init().
> 2, Rename vga_arb_select_default_device() to vga_arb_update_default_
> device(), which selects the first probed VGA device as the default
> (whether legacy resources enabled or not), and update the default
> device if a better one is found (device with legacy resources enabled
> is better, device owns the firmware framebuffer is even better).
> 3, Every time a new VGA device is probed, vga_arbiter_add_pci_device()
> is called, and vga_arb_update_default_device() is also called. So the
> hotplug case and the module case can also be handled.
>
> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> ---
> drivers/gpu/vga/vgaarb.c | 219 +++++++++++++++++----------------------
> 1 file changed, 97 insertions(+), 122 deletions(-)
I think maybe it *would* be a good idea to move this to drivers/pci,
and I think it would be nice to make the move the very first patch in
a series because I think there may be follow-ons, and it would be nice
to get more of the history in the new location after the break.
For example, if it's in drivers/pci, we can call the arbiter directly
from the enumeration/removal code instead of using a bus notifier.
I put this on a branch so you can see what it looks like:
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=review/vga
I tried to pull out as much stuff as I could into smaller patches so
this one is a little bit more understandable.
But we definitely need some work on the commit log. The first three
paragraphs seem to describe the approach you *didn't* use. We don't
need that.
But we *do* need an explanation of why we need this patch and how it
works. Based on the patch, I assume the initcall ordering is
different on your system, which exposes a problem in
vga_arbiter_add_pci_device():
Looking at [1], I think that on your system, vga_arb_device_init()
runs before the VGA card is enumerated, so it doesn't set the card as
the default VGA device.
We do call vga_arbiter_add_pci_device() when we enumerate the device,
but it doesn't set the card as the default device either because the
bridge upstream from it doesn't support PCI_BRIDGE_CTL_VGA.
So the commit log needs to (1) explain why your initcall ordering is
unusual, and (2) explain the approach, which IIUC is basically to add
the vga_arb_select_default_device() functionality to
vga_arbiter_add_pci_device().
[1] https://lore.kernel.org/linux-pci/20210518193100.GA148462@bjorn-Precision-5520/
> diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> index 949fde433ea2..07770aae3aaf 100644
> --- a/drivers/gpu/vga/vgaarb.c
> +++ b/drivers/gpu/vga/vgaarb.c
> @@ -586,6 +586,97 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc)
> }
> EXPORT_SYMBOL(vga_put);
>
> +#if defined(CONFIG_ACPI)
> +static bool vga_arb_integrated_gpu(struct device *dev)
> +{
> + struct acpi_device *adev = ACPI_COMPANION(dev);
> +
> + return adev && !strcmp(acpi_device_hid(adev), ACPI_VIDEO_HID);
> +}
> +#else
> +static bool vga_arb_integrated_gpu(struct device *dev)
> +{
> + return false;
> +}
> +#endif
> +
> +static void vga_arb_update_default_device(struct vga_device *vgadev)
> +{
> + struct pci_dev *pdev = vgadev->pdev;
> + struct device *dev = &pdev->dev;
> + struct vga_device *vgadev_default;
> +#if defined(CONFIG_X86) || defined(CONFIG_IA64)
> + int i;
> + unsigned long flags;
> + u64 base = screen_info.lfb_base;
> + u64 size = screen_info.lfb_size;
> + u64 limit;
> + resource_size_t start, end;
> +#endif
> +
> + /* Deal with VGA default device. Use first enabled one
> + * by default if arch doesn't have it's own hook
> + */
> + if (!vga_default_device()) {
> + if ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)
> + vgaarb_info(dev, "setting as boot VGA device\n");
> + else
> + vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n");
> + vga_set_default_device(pdev);
> + }
> +
> + vgadev_default = vgadev_find(vga_default);
> +
> + /* Overridden by a better device */
> + if (vgadev_default && ((vgadev_default->owns & VGA_RSRC_LEGACY_MASK) == 0)
> + && ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) {
> + vgaarb_info(dev, "overriding boot VGA device\n");
> + vga_set_default_device(pdev);
> + }
> +
> + if (vga_arb_integrated_gpu(dev)) {
> + vgaarb_info(dev, "overriding boot VGA device\n");
> + vga_set_default_device(pdev);
> + }
> +
> +#if defined(CONFIG_X86) || defined(CONFIG_IA64)
> + if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
> + base |= (u64)screen_info.ext_lfb_base << 32;
> +
> + limit = base + size;
> +
> + /*
> + * Override vga_arbiter_add_pci_device()'s I/O based detection
> + * as it may take the wrong device (e.g. on Apple system under
> + * EFI).
> + *
> + * Select the device owning the boot framebuffer if there is
> + * one.
> + */
> +
> + /* Does firmware framebuffer belong to us? */
> + for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> + flags = pci_resource_flags(vgadev->pdev, i);
> +
> + if ((flags & IORESOURCE_MEM) == 0)
> + continue;
> +
> + start = pci_resource_start(vgadev->pdev, i);
> + end = pci_resource_end(vgadev->pdev, i);
> +
> + if (!start || !end)
> + continue;
> +
> + if (base < start || limit >= end)
> + continue;
> +
> + if (vgadev->pdev != vga_default_device())
> + vgaarb_info(dev, "overriding boot device\n");
> + vga_set_default_device(vgadev->pdev);
> + }
> +#endif
> +}
> +
> /*
> * Rules for using a bridge to control a VGA descendant decoding: if a bridge
> * has only one VGA descendant then it can be used to control the VGA routing
> @@ -643,6 +734,11 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev)
> }
> new_bus = new_bus->parent;
> }
> +
> + if (vgadev->bridge_has_one_vga == true)
> + vgaarb_info(&vgadev->pdev->dev, "bridge control possible\n");
> + else
> + vgaarb_info(&vgadev->pdev->dev, "no bridge control possible\n");
> }
>
> /*
> @@ -713,15 +809,7 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
> bus = bus->parent;
> }
>
> - /* Deal with VGA default device. Use first enabled one
> - * by default if arch doesn't have it's own hook
> - */
> - if (vga_default == NULL &&
> - ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) {
> - vgaarb_info(&pdev->dev, "setting as boot VGA device\n");
> - vga_set_default_device(pdev);
> - }
> -
> + vga_arb_update_default_device(vgadev);
> vga_arbiter_check_bridge_sharing(vgadev);
>
> /* Add to the list */
> @@ -1451,111 +1539,10 @@ static struct miscdevice vga_arb_device = {
> MISC_DYNAMIC_MINOR, "vga_arbiter", &vga_arb_device_fops
> };
>
> -#if defined(CONFIG_ACPI)
> -static bool vga_arb_integrated_gpu(struct device *dev)
> -{
> - struct acpi_device *adev = ACPI_COMPANION(dev);
> -
> - return adev && !strcmp(acpi_device_hid(adev), ACPI_VIDEO_HID);
> -}
> -#else
> -static bool vga_arb_integrated_gpu(struct device *dev)
> -{
> - return false;
> -}
> -#endif
> -
> -static void __init vga_arb_select_default_device(void)
> -{
> - struct pci_dev *pdev, *found = NULL;
> - struct vga_device *vgadev;
> -
> -#if defined(CONFIG_X86) || defined(CONFIG_IA64)
> - u64 base = screen_info.lfb_base;
> - u64 size = screen_info.lfb_size;
> - u64 limit;
> - resource_size_t start, end;
> - unsigned long flags;
> - int i;
> -
> - if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
> - base |= (u64)screen_info.ext_lfb_base << 32;
> -
> - limit = base + size;
> -
> - list_for_each_entry(vgadev, &vga_list, list) {
> - struct device *dev = &vgadev->pdev->dev;
> - /*
> - * Override vga_arbiter_add_pci_device()'s I/O based detection
> - * as it may take the wrong device (e.g. on Apple system under
> - * EFI).
> - *
> - * Select the device owning the boot framebuffer if there is
> - * one.
> - */
> -
> - /* Does firmware framebuffer belong to us? */
> - for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> - flags = pci_resource_flags(vgadev->pdev, i);
> -
> - if ((flags & IORESOURCE_MEM) == 0)
> - continue;
> -
> - start = pci_resource_start(vgadev->pdev, i);
> - end = pci_resource_end(vgadev->pdev, i);
> -
> - if (!start || !end)
> - continue;
> -
> - if (base < start || limit >= end)
> - continue;
> -
> - if (!vga_default_device())
> - vgaarb_info(dev, "setting as boot device\n");
> - else if (vgadev->pdev != vga_default_device())
> - vgaarb_info(dev, "overriding boot device\n");
> - vga_set_default_device(vgadev->pdev);
> - }
> - }
> -#endif
> -
> - if (!vga_default_device()) {
> - list_for_each_entry_reverse(vgadev, &vga_list, list) {
> - struct device *dev = &vgadev->pdev->dev;
> - u16 cmd;
> -
> - pdev = vgadev->pdev;
> - pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> - if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
> - found = pdev;
> - if (vga_arb_integrated_gpu(dev))
> - break;
> - }
> - }
> - }
> -
> - if (found) {
> - vgaarb_info(&found->dev, "setting as boot device (VGA legacy resources not available)\n");
> - vga_set_default_device(found);
> - return;
> - }
> -
> - if (!vga_default_device()) {
> - vgadev = list_first_entry_or_null(&vga_list,
> - struct vga_device, list);
> - if (vgadev) {
> - struct device *dev = &vgadev->pdev->dev;
> - vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n");
> - 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)
> @@ -1571,18 +1558,6 @@ static int __init vga_arb_device_init(void)
> 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");
> - }
> -
> - vga_arb_select_default_device();
> -
> - pr_info("loaded\n");
> return rc;
> }
> subsys_initcall(vga_arb_device_init);
> --
> 2.27.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread