From: Bjorn Helgaas <[email protected]>
Current default VGA device selection fails in some cases because part of it
is done in the vga_arb_device_init() subsys_initcall, and some arches
enumerate PCI devices in pcibios_init(), which runs *after* that.
For example:
- On BMC system, the AST2500 bridge [1a03:1150] does not implement
PCI_BRIDGE_CTL_VGA. This is perfectly legal but means the legacy VGA
resources won't reach downstream devices unless they're included in the
usual bridge windows.
- vga_arb_select_default_device() will set a device below such a bridge
as the default VGA device as long as it has PCI_COMMAND_IO and
PCI_COMMAND_MEMORY enabled.
- vga_arbiter_add_pci_device() is called for every VGA device, either at
boot-time or at hot-add time, and it will also set the device as the
default VGA device, but ONLY if all bridges leading to it implement
PCI_BRIDGE_CTL_VGA.
- This difference between vga_arb_select_default_device() and
vga_arbiter_add_pci_device() means that a device below an AST2500 or
similar bridge can only be set as the default if it is enumerated
before vga_arb_device_init().
- On ACPI-based systems, PCI devices are enumerated by acpi_init(), which
runs before vga_arb_device_init().
- On non-ACPI systems, like on MIPS system, they are enumerated by
pcibios_init(), which typically runs *after* vga_arb_device_init().
This series consolidates all the default VGA device selection in
vga_arbiter_add_pci_device(), which is always called after enumerating a
PCI device.
Almost all the work here is Huacai's. I restructured it a little bit and
added a few trivial patches on top.
I'd like to move vgaarb.c to drivers/pci eventually, but there's another
initcall ordering snag that needs to be resolved first, so this leaves
it where it is.
Bjorn
Version history:
V0 original implementation as final quirk to set default device.
https://lore.kernel.org/r/[email protected]
V1 rework vgaarb to do all default device selection in
vga_arbiter_add_pci_device().
https://lore.kernel.org/r/[email protected]
V2 move arbiter to PCI subsystem, fix nits.
https://lore.kernel.org/r/[email protected]
V3 rewrite the commit log of the last patch (which is also summarized
by Bjorn).
https://lore.kernel.org/r/[email protected]
V4 split the last patch to two steps.
https://lore.kernel.org/r/[email protected]
V5 split Patch-9 again and sort the patches.
https://lore.kernel.org/r/[email protected]
V6 split Patch-5 again and sort the patches again.
https://lore.kernel.org/r/[email protected]
V7 stop moving vgaarb to drivers/pci because of ordering issues with
misc_init().
https://lore.kernel.org/r/[email protected]
https://lore.kernel.org/r/CAAhV-H7FhAjM-Ha42Z1dLrE4PvC9frfyeU27KHWcyWKkMftEsA@mail.gmail.com
Bjorn Helgaas (8):
vgaarb: Factor out vga_select_framebuffer_device()
vgaarb: Factor out default VGA device selection
vgaarb: Move framebuffer detection to ADD_DEVICE path
vgaarb: Move non-legacy VGA detection to ADD_DEVICE path
vgaarb: Move disabled VGA device detection to ADD_DEVICE path
vgaarb: Remove empty vga_arb_device_card_gone()
vgaarb: Use unsigned format string to print lock counts
vgaarb: Replace full MIT license text with SPDX identifier
Huacai Chen (2):
vgaarb: Move vga_arb_integrated_gpu() earlier in file
vgaarb: Log bridge control messages when adding devices
drivers/gpu/vga/vgaarb.c | 311 +++++++++++++++++++--------------------
1 file changed, 154 insertions(+), 157 deletions(-)
--
2.25.1
From: Huacai Chen <[email protected]>
Move vga_arb_integrated_gpu() earlier in file to prepare for future patch.
No functional change intended.
[bhelgaas: pull #ifdefs inside function]
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Huacai Chen <[email protected]>
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/gpu/vga/vgaarb.c | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index 569930552957..ef5ad4c432f5 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -565,6 +565,17 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc)
}
EXPORT_SYMBOL(vga_put);
+static bool vga_arb_integrated_gpu(struct device *dev)
+{
+#if defined(CONFIG_ACPI)
+ struct acpi_device *adev = ACPI_COMPANION(dev);
+
+ return adev && !strcmp(acpi_device_hid(adev), ACPI_VIDEO_HID);
+#else
+ return false;
+#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
@@ -1430,20 +1441,6 @@ 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;
--
2.25.1
From: Bjorn Helgaas <[email protected]>
On x86 and ia64, if a VGA device BARs include a framebuffer reported by
platform firmware, we select the device as the default VGA device. Factor
this code to a separate function. No functional change intended.
Signed-off-by: Bjorn Helgaas <[email protected]>
Cc: Bruno Prémont <[email protected]>
---
drivers/gpu/vga/vgaarb.c | 99 +++++++++++++++++++++-------------------
1 file changed, 53 insertions(+), 46 deletions(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index ef5ad4c432f5..36d9140c64f6 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -565,6 +565,58 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc)
}
EXPORT_SYMBOL(vga_put);
+static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
+{
+#if defined(CONFIG_X86) || defined(CONFIG_IA64)
+ struct device *dev = &pdev->dev;
+ u64 base = screen_info.lfb_base;
+ u64 size = screen_info.lfb_size;
+ u64 limit;
+ resource_size_t start, end;
+ unsigned long flags;
+ int i;
+
+ /* Select the device owning the boot framebuffer if there is one */
+
+ 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(pdev, i);
+
+ if ((flags & IORESOURCE_MEM) == 0)
+ continue;
+
+ start = pci_resource_start(pdev, i);
+ end = pci_resource_end(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 (pdev != vga_default_device())
+ vgaarb_info(dev, "overriding boot device\n");
+ vga_set_default_device(pdev);
+ }
+#endif
+}
+
static bool vga_arb_integrated_gpu(struct device *dev)
{
#if defined(CONFIG_ACPI)
@@ -1446,54 +1498,9 @@ 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);
- }
+ vga_select_framebuffer_device(vgadev->pdev);
}
-#endif
if (!vga_default_device()) {
list_for_each_entry_reverse(vgadev, &vga_list, list) {
--
2.25.1
From: Bjorn Helgaas <[email protected]>
a37c0f48950b ("vgaarb: Select a default VGA device even if there's no
legacy VGA") extended the vga_arb_device_init() subsys_initcall so that if
there are no other eligible devices, it could select a disabled VGA device
as the default.
Move this detection from vga_arb_select_default_device() to
vga_arbiter_add_pci_device() so every device, even those hot-added or
enumerated after vga_arb_device_init() is eligible for selection as the
default VGA device.
Link: https://lore.kernel.org/r/[email protected]
Based-on-patch-by: Huacai Chen <[email protected]>
Signed-off-by: Bjorn Helgaas <[email protected]>
Cc: Daniel Axtens <[email protected]>
Cc: Zhou Wang <[email protected]>
---
drivers/gpu/vga/vgaarb.c | 27 +++++++++------------------
1 file changed, 9 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index 123b81628061..ad548917e602 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -656,7 +656,8 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
* We use the first one we find, so if we've already found one,
* vgadev is no better.
*/
- if (boot_vga)
+ if (boot_vga &&
+ (boot_vga->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)
return false;
if ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)
@@ -693,6 +694,13 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
return true;
}
+ /*
+ * vgadev has neither IO nor MEM enabled. If we haven't found any
+ * other VGA devices, it is the best candidate so far.
+ */
+ if (!boot_vga)
+ return true;
+
return false;
}
@@ -1559,21 +1567,6 @@ static struct miscdevice vga_arb_device = {
MISC_DYNAMIC_MINOR, "vga_arbiter", &vga_arb_device_fops
};
-static void __init vga_arb_select_default_device(void)
-{
- struct vga_device *vgadev;
-
- 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;
@@ -1603,8 +1596,6 @@ static int __init vga_arb_device_init(void)
vgaarb_info(dev, "no bridge control possible\n");
}
- vga_arb_select_default_device();
-
pr_info("loaded\n");
return rc;
}
--
2.25.1
From: Bjorn Helgaas <[email protected]>
Previously we selected a device that owns the boot framebuffer as the
default device in vga_arb_select_default_device(). This was only done in
the vga_arb_device_init() subsys_initcall, so devices enumerated later,
e.g., by pcibios_init(), were not eligible.
Fix this by moving the framebuffer device selection from
vga_arb_select_default_device() to vga_arbiter_add_pci_device(), which is
called after every PCI device is enumerated, either by the
vga_arb_device_init() subsys_initcall or as an ADD_DEVICE notifier.
Note that if vga_arb_select_default_device() found a device owning the boot
framebuffer, it unconditionally set it to be the default VGA device, and no
subsequent device could replace it.
Link: https://lore.kernel.org/r/[email protected]
Based-on-patch-by: Huacai Chen <[email protected]>
Signed-off-by: Bjorn Helgaas <[email protected]>
Cc: Bruno Prémont <[email protected]>
---
drivers/gpu/vga/vgaarb.c | 37 +++++++++++++++++--------------------
1 file changed, 17 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index b0ae0f177c6f..aefa4f406f7d 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -72,6 +72,7 @@ struct vga_device {
unsigned int io_norm_cnt; /* normal IO count */
unsigned int mem_norm_cnt; /* normal MEM count */
bool bridge_has_one_vga;
+ bool is_framebuffer; /* BAR covers firmware framebuffer */
unsigned int (*set_decode)(struct pci_dev *pdev, bool decode);
};
@@ -565,10 +566,9 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc)
}
EXPORT_SYMBOL(vga_put);
-static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
+static bool vga_is_framebuffer_device(struct pci_dev *pdev)
{
#if defined(CONFIG_X86) || defined(CONFIG_IA64)
- struct device *dev = &pdev->dev;
u64 base = screen_info.lfb_base;
u64 size = screen_info.lfb_size;
u64 limit;
@@ -583,15 +583,6 @@ static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
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(pdev, i);
@@ -608,13 +599,10 @@ static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
if (base < start || limit >= end)
continue;
- if (!vga_default_device())
- vgaarb_info(dev, "setting as boot device\n");
- else if (pdev != vga_default_device())
- vgaarb_info(dev, "overriding boot device\n");
- vga_set_default_device(pdev);
+ return true;
}
#endif
+ return false;
}
static bool vga_arb_integrated_gpu(struct device *dev)
@@ -635,6 +623,7 @@ static bool vga_arb_integrated_gpu(struct device *dev)
static bool vga_is_boot_device(struct vga_device *vgadev)
{
struct vga_device *boot_vga = vgadev_find(vga_default_device());
+ struct pci_dev *pdev = vgadev->pdev;
/*
* We select the default VGA device in this order:
@@ -645,6 +634,18 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
* Other device (see vga_arb_select_default_device())
*/
+ /*
+ * We always prefer a firmware framebuffer, so if we've already
+ * found one, there's no need to consider vgadev.
+ */
+ if (boot_vga && boot_vga->is_framebuffer)
+ return false;
+
+ if (vga_is_framebuffer_device(pdev)) {
+ vgadev->is_framebuffer = true;
+ return true;
+ }
+
/*
* A legacy VGA device has MEM and IO enabled and any bridges
* leading to it have PCI_BRIDGE_CTL_VGA enabled so the legacy
@@ -1531,10 +1532,6 @@ static void __init vga_arb_select_default_device(void)
struct pci_dev *pdev, *found = NULL;
struct vga_device *vgadev;
- list_for_each_entry(vgadev, &vga_list, list) {
- vga_select_framebuffer_device(vgadev->pdev);
- }
-
if (!vga_default_device()) {
list_for_each_entry_reverse(vgadev, &vga_list, list) {
struct device *dev = &vgadev->pdev->dev;
--
2.25.1
From: Bjorn Helgaas <[email protected]>
vga_arb_device_card_gone() has always been empty. Remove it.
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/gpu/vga/vgaarb.c | 16 +---------------
1 file changed, 1 insertion(+), 15 deletions(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index ad548917e602..455cf048fae8 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -123,8 +123,6 @@ static int vga_str_to_iostate(char *buf, int str_size, int *io_state)
/* this is only used a cookie - it should not be dereferenced */
static struct pci_dev *vga_default;
-static void vga_arb_device_card_gone(struct pci_dev *pdev);
-
/* Find somebody in our list */
static struct vga_device *vgadev_find(struct pci_dev *pdev)
{
@@ -878,10 +876,6 @@ static bool vga_arbiter_del_pci_device(struct pci_dev *pdev)
/* Remove entry from list */
list_del(&vgadev->list);
vga_count--;
- /* Notify userland driver that the device is gone so it discards
- * it's copies of the pci_dev pointer
- */
- vga_arb_device_card_gone(pdev);
/* Wake up all possible waiters */
wake_up_all(&vga_wait_queue);
@@ -1131,9 +1125,7 @@ static ssize_t vga_arb_read(struct file *file, char __user *buf,
if (lbuf == NULL)
return -ENOMEM;
- /* Shields against vga_arb_device_card_gone (pci_dev going
- * away), and allows access to vga list
- */
+ /* Protects vga_list */
spin_lock_irqsave(&vga_lock, flags);
/* If we are targeting the default, use it */
@@ -1150,8 +1142,6 @@ static ssize_t vga_arb_read(struct file *file, char __user *buf,
/* Wow, it's not in the list, that shouldn't happen,
* let's fix us up and return invalid card
*/
- if (pdev == priv->target)
- vga_arb_device_card_gone(pdev);
spin_unlock_irqrestore(&vga_lock, flags);
len = sprintf(lbuf, "invalid");
goto done;
@@ -1495,10 +1485,6 @@ static int vga_arb_release(struct inode *inode, struct file *file)
return 0;
}
-static void vga_arb_device_card_gone(struct pci_dev *pdev)
-{
-}
-
/*
* callback any registered clients to let them know we have a
* change in VGA cards
--
2.25.1
From: Bjorn Helgaas <[email protected]>
a37c0f48950b ("vgaarb: Select a default VGA device even if there's no
legacy VGA") extended the vga_arb_device_init() subsys_initcall so it could
select a non-legacy VGA device as the default.
That failed to consider that PCI devices may be enumerated after
vga_arb_device_init(), e.g., hot-added devices or non-ACPI systems that do
PCI enumeration in pcibios_init(). Devices found then could never be
selected as the default.
One system where this is a problem is the MIPS-based Loongson where an
ASpeed AST2500 VGA device is behind a bridge that doesn't implement the VGA
Enable bit, so legacy resources are not routed to the VGA device. [1]
Fix this by moving the non-legacy VGA device selection from
vga_arb_select_default_device() to vga_arbiter_add_pci_device(), which is
called after every PCI device is enumerated, either by the
vga_arb_device_init() subsys_initcall or as an ADD_DEVICE notifier.
[1] https://lore.kernel.org/r/[email protected]
Link: https://lore.kernel.org/r/[email protected]
Link: https://lore.kernel.org/r/[email protected]
Based-on-patch-by: Huacai Chen <[email protected]>
Signed-off-by: Bjorn Helgaas <[email protected]>
Cc: Daniel Axtens <[email protected]>
Cc: Zhou Wang <[email protected]>
---
drivers/gpu/vga/vgaarb.c | 54 ++++++++++++++++++++++++----------------
1 file changed, 32 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index aefa4f406f7d..123b81628061 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -624,6 +624,7 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
{
struct vga_device *boot_vga = vgadev_find(vga_default_device());
struct pci_dev *pdev = vgadev->pdev;
+ u16 cmd, boot_cmd;
/*
* We select the default VGA device in this order:
@@ -661,6 +662,37 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
if ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)
return true;
+ /*
+ * If we haven't found a legacy VGA device, accept a non-legacy
+ * device. It may have either IO or MEM enabled, and bridges may
+ * not have PCI_BRIDGE_CTL_VGA enabled, so it may not be able to
+ * use legacy VGA resources. Prefer an integrated GPU over others.
+ */
+ pci_read_config_word(pdev, PCI_COMMAND, &cmd);
+ if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
+
+ /*
+ * An integrated GPU overrides a previous non-legacy
+ * device. We expect only a single integrated GPU, but if
+ * there are more, we use the *last* because that was the
+ * previous behavior.
+ */
+ if (vga_arb_integrated_gpu(&pdev->dev))
+ return true;
+
+ /*
+ * We prefer the first non-legacy discrete device we find.
+ * If we already found one, vgadev is no better.
+ */
+ if (boot_vga) {
+ pci_read_config_word(boot_vga->pdev, PCI_COMMAND,
+ &boot_cmd);
+ if (boot_cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY))
+ return false;
+ }
+ return true;
+ }
+
return false;
}
@@ -1529,30 +1561,8 @@ static struct miscdevice vga_arb_device = {
static void __init vga_arb_select_default_device(void)
{
- struct pci_dev *pdev, *found = NULL;
struct vga_device *vgadev;
- 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);
--
2.25.1
From: Bjorn Helgaas <[email protected]>
Default VGA device selection fails when PCI devices are enumerated after
the vga_arb_device_init() subsys_initcall.
vga_arbiter_add_pci_device() selects the first fully enabled device to
which legacy VGA resources are routed as the default VGA device. This is
an ADD_DEVICE notifier, so it runs after every PCI device is enumerated.
vga_arb_select_default_device() may select framebuffer devices, partially
enabled GPUs, or non-legacy devices that don't have legacy VGA resources
routed to them as the default VGA device. But this only happens once, from
the vga_arb_device_init() subsys_initcall, so it doesn't consider devices
enumerated after that:
acpi_init
acpi_scan_init
acpi_pci_root_init # PCI device enumeration (ACPI systems)
vga_arb_device_init
for_each_pci_device
vga_arbiter_add_pci_device # ADD_DEVICE notifier
if (VGA-owner)
vga_set_default_device <-- set default VGA
vga_arb_select_default_device # only called ONCE
for_each_vga_device
if (framebuffer)
vga_set_default_device <-- set default VGA to framebuffer
if (!vga_default_device())
if (non-legacy, integrated GPU, etc)
vga_set_default_device <-- set default VGA
if (!vga_default_device())
vga_set_default_device <-- set default VGA
pcibios_init
pcibios_scanbus # PCI device enumeration (non-ACPI systems)
...
vga_arbiter_add_pci_device # ADD_DEVICE notification
if (VGA-owner)
vga_set_default_device <-- set default VGA
Note that on non-ACPI systems, vga_arb_select_default_device() runs before
pcibios_init(), so it sees no VGA devices and can never set a framebuffer
device, a non-legacy integrated GPU, etc., as the default device.
Factor out the default VGA device selection to vga_is_boot_device(), called
from vga_arbiter_add_pci_device().
Then we can migrate the default device selection from
vga_arb_select_default_device() to the vga_arbiter_add_pci_device() path.
Link: https://lore.kernel.org/r/[email protected]
Based-on-patch-by: Huacai Chen <[email protected]>
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/gpu/vga/vgaarb.c | 45 ++++++++++++++++++++++++++++++++++------
1 file changed, 39 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index 36d9140c64f6..b0ae0f177c6f 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -628,6 +628,41 @@ static bool vga_arb_integrated_gpu(struct device *dev)
#endif
}
+/*
+ * Return true if vgadev is a better default VGA device than the best one
+ * we've seen so far.
+ */
+static bool vga_is_boot_device(struct vga_device *vgadev)
+{
+ struct vga_device *boot_vga = vgadev_find(vga_default_device());
+
+ /*
+ * We select the default VGA device in this order:
+ * Firmware framebuffer (see vga_arb_select_default_device())
+ * Legacy VGA device (owns VGA_RSRC_LEGACY_MASK)
+ * Non-legacy integrated device (see vga_arb_select_default_device())
+ * Non-legacy discrete device (see vga_arb_select_default_device())
+ * Other device (see vga_arb_select_default_device())
+ */
+
+ /*
+ * A legacy VGA device has MEM and IO enabled and any bridges
+ * leading to it have PCI_BRIDGE_CTL_VGA enabled so the legacy
+ * resources ([mem 0xa0000-0xbffff], [io 0x3b0-0x3bb], etc) are
+ * routed to it.
+ *
+ * We use the first one we find, so if we've already found one,
+ * vgadev is no better.
+ */
+ if (boot_vga)
+ return false;
+
+ if ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)
+ return true;
+
+ return false;
+}
+
/*
* 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
@@ -755,12 +790,10 @@ 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");
+ if (vga_is_boot_device(vgadev)) {
+ vgaarb_info(&pdev->dev, "setting as boot VGA device%s\n",
+ vga_default_device() ?
+ " (overriding previous)" : "");
vga_set_default_device(pdev);
}
--
2.25.1
From: Huacai Chen <[email protected]>
Previously vga_arb_device_init() iterated through all VGA devices and
indicated whether legacy VGA routing to each could be controlled by an
upstream bridge.
But we determine that information in vga_arbiter_add_pci_device(), which we
call for every device, so we can log it there without iterating through the
VGA devices again.
Note that we call vga_arbiter_check_bridge_sharing() before adding the
device to vga_list, so we have to handle the very first device separately.
Signed-off-by: Huacai Chen <[email protected]>
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/gpu/vga/vgaarb.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index 455cf048fae8..b7e6c1228fff 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -719,8 +719,10 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev)
vgadev->bridge_has_one_vga = true;
- if (list_empty(&vga_list))
+ if (list_empty(&vga_list)) {
+ vgaarb_info(&vgadev->pdev->dev, "bridge control possible\n");
return;
+ }
/* okay iterate the new devices bridge hierarachy */
new_bus = vgadev->pdev->bus;
@@ -759,6 +761,11 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev)
}
new_bus = new_bus->parent;
}
+
+ if (vgadev->bridge_has_one_vga)
+ vgaarb_info(&vgadev->pdev->dev, "bridge control possible\n");
+ else
+ vgaarb_info(&vgadev->pdev->dev, "no bridge control possible\n");
}
/*
@@ -1557,7 +1564,6 @@ 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)
@@ -1573,15 +1579,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");
- }
-
pr_info("loaded\n");
return rc;
}
--
2.25.1
From: Bjorn Helgaas <[email protected]>
In struct vga_device, io_lock_cnt and mem_lock_cnt are unsigned, but we
previously printed them with "%d", the signed decimal format. Print them
with the unsigned format "%u" instead.
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/gpu/vga/vgaarb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index b7e6c1228fff..95e37817074e 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -1156,7 +1156,7 @@ static ssize_t vga_arb_read(struct file *file, char __user *buf,
/* Fill the buffer with infos */
len = snprintf(lbuf, 1024,
- "count:%d,PCI:%s,decodes=%s,owns=%s,locks=%s(%d:%d)\n",
+ "count:%d,PCI:%s,decodes=%s,owns=%s,locks=%s(%u:%u)\n",
vga_decode_count, pci_name(pdev),
vga_iostate_to_str(vgadev->decodes),
vga_iostate_to_str(vgadev->owns),
--
2.25.1
From: Bjorn Helgaas <[email protected]>
Per Documentation/process/license-rules.rst, the SPDX MIT identifier is
equivalent to including the entire MIT license text from
LICENSES/preferred/MIT.
Replace the MIT license text with the equivalent SPDX identifier.
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/gpu/vga/vgaarb.c | 23 +----------------------
1 file changed, 1 insertion(+), 22 deletions(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index 95e37817074e..17e9d2536430 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -1,32 +1,11 @@
+// SPDX-License-Identifier: MIT
/*
* vgaarb.c: Implements the VGA arbitration. For details refer to
* Documentation/gpu/vgaarbiter.rst
*
- *
* (C) Copyright 2005 Benjamin Herrenschmidt <[email protected]>
* (C) Copyright 2007 Paulo R. Zanoni <[email protected]>
* (C) Copyright 2007, 2009 Tiago Vignatti <[email protected]>
- *
- * Permission is hereby granted, free of charge, to any person obtaining a
- * copy of this software and associated documentation files (the "Software"),
- * to deal in the Software without restriction, including without limitation
- * the rights to use, copy, modify, merge, publish, distribute, sublicense,
- * and/or sell copies of the Software, and to permit persons to whom the
- * Software is furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice (including the next
- * paragraph) shall be included in all copies or substantial portions of the
- * Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
- * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS
- * IN THE SOFTWARE.
- *
*/
#define pr_fmt(fmt) "vgaarb: " fmt
--
2.25.1
Hi, Bjorn,
On Thu, Jan 6, 2022 at 8:07 AM Bjorn Helgaas <[email protected]> wrote:
>
> From: Bjorn Helgaas <[email protected]>
>
> Previously we selected a device that owns the boot framebuffer as the
> default device in vga_arb_select_default_device(). This was only done in
> the vga_arb_device_init() subsys_initcall, so devices enumerated later,
> e.g., by pcibios_init(), were not eligible.
>
> Fix this by moving the framebuffer device selection from
> vga_arb_select_default_device() to vga_arbiter_add_pci_device(), which is
> called after every PCI device is enumerated, either by the
> vga_arb_device_init() subsys_initcall or as an ADD_DEVICE notifier.
>
> Note that if vga_arb_select_default_device() found a device owning the boot
> framebuffer, it unconditionally set it to be the default VGA device, and no
> subsequent device could replace it.
>
> Link: https://lore.kernel.org/r/[email protected]
> Based-on-patch-by: Huacai Chen <[email protected]>
> Signed-off-by: Bjorn Helgaas <[email protected]>
> Cc: Bruno Prémont <[email protected]>
> ---
> drivers/gpu/vga/vgaarb.c | 37 +++++++++++++++++--------------------
> 1 file changed, 17 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> index b0ae0f177c6f..aefa4f406f7d 100644
> --- a/drivers/gpu/vga/vgaarb.c
> +++ b/drivers/gpu/vga/vgaarb.c
> @@ -72,6 +72,7 @@ struct vga_device {
> unsigned int io_norm_cnt; /* normal IO count */
> unsigned int mem_norm_cnt; /* normal MEM count */
> bool bridge_has_one_vga;
> + bool is_framebuffer; /* BAR covers firmware framebuffer */
> unsigned int (*set_decode)(struct pci_dev *pdev, bool decode);
> };
>
> @@ -565,10 +566,9 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc)
> }
> EXPORT_SYMBOL(vga_put);
>
> -static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
> +static bool vga_is_framebuffer_device(struct pci_dev *pdev)
> {
> #if defined(CONFIG_X86) || defined(CONFIG_IA64)
> - struct device *dev = &pdev->dev;
> u64 base = screen_info.lfb_base;
> u64 size = screen_info.lfb_size;
> u64 limit;
> @@ -583,15 +583,6 @@ static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
>
> 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(pdev, i);
> @@ -608,13 +599,10 @@ static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
> if (base < start || limit >= end)
> continue;
>
> - if (!vga_default_device())
> - vgaarb_info(dev, "setting as boot device\n");
> - else if (pdev != vga_default_device())
> - vgaarb_info(dev, "overriding boot device\n");
> - vga_set_default_device(pdev);
> + return true;
> }
> #endif
> + return false;
> }
>
> static bool vga_arb_integrated_gpu(struct device *dev)
> @@ -635,6 +623,7 @@ static bool vga_arb_integrated_gpu(struct device *dev)
> static bool vga_is_boot_device(struct vga_device *vgadev)
> {
> struct vga_device *boot_vga = vgadev_find(vga_default_device());
> + struct pci_dev *pdev = vgadev->pdev;
>
> /*
> * We select the default VGA device in this order:
> @@ -645,6 +634,18 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
> * Other device (see vga_arb_select_default_device())
> */
>
> + /*
> + * We always prefer a firmware framebuffer, so if we've already
> + * found one, there's no need to consider vgadev.
> + */
> + if (boot_vga && boot_vga->is_framebuffer)
> + return false;
> +
> + if (vga_is_framebuffer_device(pdev)) {
> + vgadev->is_framebuffer = true;
> + return true;
> + }
Maybe it is better to rename vga_is_framebuffer_device() to
vga_is_firmware_device() and rename is_framebuffer to
is_fw_framebuffer?
Huacai
> +
> /*
> * A legacy VGA device has MEM and IO enabled and any bridges
> * leading to it have PCI_BRIDGE_CTL_VGA enabled so the legacy
> @@ -1531,10 +1532,6 @@ static void __init vga_arb_select_default_device(void)
> struct pci_dev *pdev, *found = NULL;
> struct vga_device *vgadev;
>
> - list_for_each_entry(vgadev, &vga_list, list) {
> - vga_select_framebuffer_device(vgadev->pdev);
> - }
> -
> if (!vga_default_device()) {
> list_for_each_entry_reverse(vgadev, &vga_list, list) {
> struct device *dev = &vgadev->pdev->dev;
> --
> 2.25.1
>
On Thu, Jan 06, 2022 at 02:44:42PM +0800, Huacai Chen wrote:
> On Thu, Jan 6, 2022 at 8:07 AM Bjorn Helgaas <[email protected]> wrote:
> > Previously we selected a device that owns the boot framebuffer as the
> > default device in vga_arb_select_default_device(). This was only done in
> > the vga_arb_device_init() subsys_initcall, so devices enumerated later,
> > e.g., by pcibios_init(), were not eligible.
> >
> > Fix this by moving the framebuffer device selection from
> > vga_arb_select_default_device() to vga_arbiter_add_pci_device(), which is
> > called after every PCI device is enumerated, either by the
> > vga_arb_device_init() subsys_initcall or as an ADD_DEVICE notifier.
> >
> > Note that if vga_arb_select_default_device() found a device owning the boot
> > framebuffer, it unconditionally set it to be the default VGA device, and no
> > subsequent device could replace it.
> >
> > Link: https://lore.kernel.org/r/[email protected]
> > Based-on-patch-by: Huacai Chen <[email protected]>
> > Signed-off-by: Bjorn Helgaas <[email protected]>
> > Cc: Bruno Pr?mont <[email protected]>
> > ---
> > drivers/gpu/vga/vgaarb.c | 37 +++++++++++++++++--------------------
> > 1 file changed, 17 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> > index b0ae0f177c6f..aefa4f406f7d 100644
> > --- a/drivers/gpu/vga/vgaarb.c
> > +++ b/drivers/gpu/vga/vgaarb.c
> > @@ -72,6 +72,7 @@ struct vga_device {
> > unsigned int io_norm_cnt; /* normal IO count */
> > unsigned int mem_norm_cnt; /* normal MEM count */
> > bool bridge_has_one_vga;
> > + bool is_framebuffer; /* BAR covers firmware framebuffer */
> > unsigned int (*set_decode)(struct pci_dev *pdev, bool decode);
> > };
> >
> > @@ -565,10 +566,9 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc)
> > }
> > EXPORT_SYMBOL(vga_put);
> >
> > -static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
> > +static bool vga_is_framebuffer_device(struct pci_dev *pdev)
> > {
> > #if defined(CONFIG_X86) || defined(CONFIG_IA64)
> > - struct device *dev = &pdev->dev;
> > u64 base = screen_info.lfb_base;
> > u64 size = screen_info.lfb_size;
> > u64 limit;
> > @@ -583,15 +583,6 @@ static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
> >
> > 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(pdev, i);
> > @@ -608,13 +599,10 @@ static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
> > if (base < start || limit >= end)
> > continue;
> >
> > - if (!vga_default_device())
> > - vgaarb_info(dev, "setting as boot device\n");
> > - else if (pdev != vga_default_device())
> > - vgaarb_info(dev, "overriding boot device\n");
> > - vga_set_default_device(pdev);
> > + return true;
> > }
> > #endif
> > + return false;
> > }
> >
> > static bool vga_arb_integrated_gpu(struct device *dev)
> > @@ -635,6 +623,7 @@ static bool vga_arb_integrated_gpu(struct device *dev)
> > static bool vga_is_boot_device(struct vga_device *vgadev)
> > {
> > struct vga_device *boot_vga = vgadev_find(vga_default_device());
> > + struct pci_dev *pdev = vgadev->pdev;
> >
> > /*
> > * We select the default VGA device in this order:
> > @@ -645,6 +634,18 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
> > * Other device (see vga_arb_select_default_device())
> > */
> >
> > + /*
> > + * We always prefer a firmware framebuffer, so if we've already
> > + * found one, there's no need to consider vgadev.
> > + */
> > + if (boot_vga && boot_vga->is_framebuffer)
> > + return false;
> > +
> > + if (vga_is_framebuffer_device(pdev)) {
> > + vgadev->is_framebuffer = true;
> > + return true;
> > + }
> Maybe it is better to rename vga_is_framebuffer_device() to
> vga_is_firmware_device() and rename is_framebuffer to
> is_fw_framebuffer?
That's a great point, thanks!
The "framebuffer" term is way too generic. *All* VGA devices have a
framebuffer, so it adds no information. This is really about finding
the device that was used by firmware.
I renamed:
vga_is_framebuffer_device() -> vga_is_firmware_default()
vga_device.is_framebuffer -> vga_device.is_firmware_default
I updated my local branch and pushed it to:
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/vga
with head 0f4caffa1297 ("vgaarb: Replace full MIT license text with
SPDX identifier").
I don't maintain drivers/gpu/vga/vgaarb.c, so this branch is just for
reference. It'll ultimately be up to the DRM folks to handle this.
I'll wait for any other comments or testing reports before reposting.
> > /*
> > * A legacy VGA device has MEM and IO enabled and any bridges
> > * leading to it have PCI_BRIDGE_CTL_VGA enabled so the legacy
> > @@ -1531,10 +1532,6 @@ static void __init vga_arb_select_default_device(void)
> > struct pci_dev *pdev, *found = NULL;
> > struct vga_device *vgadev;
> >
> > - list_for_each_entry(vgadev, &vga_list, list) {
> > - vga_select_framebuffer_device(vgadev->pdev);
> > - }
> > -
> > if (!vga_default_device()) {
> > list_for_each_entry_reverse(vgadev, &vga_list, list) {
> > struct device *dev = &vgadev->pdev->dev;
> > --
> > 2.25.1
> >
[+to Maarten, Maxime, Thomas: sorry, I forgot to use
get_maintainer.pl so I missed you the first time. Beginning of thread:
https://lore.kernel.org/all/[email protected]/#t
Git branch with this v8 + a couple trivial renames, based on v5.16-rc1:
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=0f4caffa1297]
On Wed, Jan 05, 2022 at 06:06:48PM -0600, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <[email protected]>
>
> Current default VGA device selection fails in some cases because part of it
> is done in the vga_arb_device_init() subsys_initcall, and some arches
> enumerate PCI devices in pcibios_init(), which runs *after* that.
>
> For example:
>
> - On BMC system, the AST2500 bridge [1a03:1150] does not implement
> PCI_BRIDGE_CTL_VGA. This is perfectly legal but means the legacy VGA
> resources won't reach downstream devices unless they're included in the
> usual bridge windows.
>
> - vga_arb_select_default_device() will set a device below such a bridge
> as the default VGA device as long as it has PCI_COMMAND_IO and
> PCI_COMMAND_MEMORY enabled.
>
> - vga_arbiter_add_pci_device() is called for every VGA device, either at
> boot-time or at hot-add time, and it will also set the device as the
> default VGA device, but ONLY if all bridges leading to it implement
> PCI_BRIDGE_CTL_VGA.
>
> - This difference between vga_arb_select_default_device() and
> vga_arbiter_add_pci_device() means that a device below an AST2500 or
> similar bridge can only be set as the default if it is enumerated
> before vga_arb_device_init().
>
> - On ACPI-based systems, PCI devices are enumerated by acpi_init(), which
> runs before vga_arb_device_init().
>
> - On non-ACPI systems, like on MIPS system, they are enumerated by
> pcibios_init(), which typically runs *after* vga_arb_device_init().
>
> This series consolidates all the default VGA device selection in
> vga_arbiter_add_pci_device(), which is always called after enumerating a
> PCI device.
>
> Almost all the work here is Huacai's. I restructured it a little bit and
> added a few trivial patches on top.
>
> I'd like to move vgaarb.c to drivers/pci eventually, but there's another
> initcall ordering snag that needs to be resolved first, so this leaves
> it where it is.
>
> Bjorn
>
> Version history:
> V0 original implementation as final quirk to set default device.
> https://lore.kernel.org/r/[email protected]
>
> V1 rework vgaarb to do all default device selection in
> vga_arbiter_add_pci_device().
> https://lore.kernel.org/r/[email protected]
>
> V2 move arbiter to PCI subsystem, fix nits.
> https://lore.kernel.org/r/[email protected]
>
> V3 rewrite the commit log of the last patch (which is also summarized
> by Bjorn).
> https://lore.kernel.org/r/[email protected]
>
> V4 split the last patch to two steps.
> https://lore.kernel.org/r/[email protected]
>
> V5 split Patch-9 again and sort the patches.
> https://lore.kernel.org/r/[email protected]
>
> V6 split Patch-5 again and sort the patches again.
> https://lore.kernel.org/r/[email protected]
>
> V7 stop moving vgaarb to drivers/pci because of ordering issues with
> misc_init().
> https://lore.kernel.org/r/[email protected]
> https://lore.kernel.org/r/CAAhV-H7FhAjM-Ha42Z1dLrE4PvC9frfyeU27KHWcyWKkMftEsA@mail.gmail.com
>
>
> Bjorn Helgaas (8):
> vgaarb: Factor out vga_select_framebuffer_device()
> vgaarb: Factor out default VGA device selection
> vgaarb: Move framebuffer detection to ADD_DEVICE path
> vgaarb: Move non-legacy VGA detection to ADD_DEVICE path
> vgaarb: Move disabled VGA device detection to ADD_DEVICE path
> vgaarb: Remove empty vga_arb_device_card_gone()
> vgaarb: Use unsigned format string to print lock counts
> vgaarb: Replace full MIT license text with SPDX identifier
>
> Huacai Chen (2):
> vgaarb: Move vga_arb_integrated_gpu() earlier in file
> vgaarb: Log bridge control messages when adding devices
>
> drivers/gpu/vga/vgaarb.c | 311 +++++++++++++++++++--------------------
> 1 file changed, 154 insertions(+), 157 deletions(-)
>
> --
> 2.25.1
>
>
For the whole series:
Tested-by: Huacai Chen <[email protected]>
On Fri, Jan 7, 2022 at 12:30 AM Bjorn Helgaas <[email protected]> wrote:
>
> [+to Maarten, Maxime, Thomas: sorry, I forgot to use
> get_maintainer.pl so I missed you the first time. Beginning of thread:
> https://lore.kernel.org/all/[email protected]/#t
> Git branch with this v8 + a couple trivial renames, based on v5.16-rc1:
> https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=0f4caffa1297]
>
> On Wed, Jan 05, 2022 at 06:06:48PM -0600, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <[email protected]>
> >
> > Current default VGA device selection fails in some cases because part of it
> > is done in the vga_arb_device_init() subsys_initcall, and some arches
> > enumerate PCI devices in pcibios_init(), which runs *after* that.
> >
> > For example:
> >
> > - On BMC system, the AST2500 bridge [1a03:1150] does not implement
> > PCI_BRIDGE_CTL_VGA. This is perfectly legal but means the legacy VGA
> > resources won't reach downstream devices unless they're included in the
> > usual bridge windows.
> >
> > - vga_arb_select_default_device() will set a device below such a bridge
> > as the default VGA device as long as it has PCI_COMMAND_IO and
> > PCI_COMMAND_MEMORY enabled.
> >
> > - vga_arbiter_add_pci_device() is called for every VGA device, either at
> > boot-time or at hot-add time, and it will also set the device as the
> > default VGA device, but ONLY if all bridges leading to it implement
> > PCI_BRIDGE_CTL_VGA.
> >
> > - This difference between vga_arb_select_default_device() and
> > vga_arbiter_add_pci_device() means that a device below an AST2500 or
> > similar bridge can only be set as the default if it is enumerated
> > before vga_arb_device_init().
> >
> > - On ACPI-based systems, PCI devices are enumerated by acpi_init(), which
> > runs before vga_arb_device_init().
> >
> > - On non-ACPI systems, like on MIPS system, they are enumerated by
> > pcibios_init(), which typically runs *after* vga_arb_device_init().
> >
> > This series consolidates all the default VGA device selection in
> > vga_arbiter_add_pci_device(), which is always called after enumerating a
> > PCI device.
> >
> > Almost all the work here is Huacai's. I restructured it a little bit and
> > added a few trivial patches on top.
> >
> > I'd like to move vgaarb.c to drivers/pci eventually, but there's another
> > initcall ordering snag that needs to be resolved first, so this leaves
> > it where it is.
> >
> > Bjorn
> >
> > Version history:
> > V0 original implementation as final quirk to set default device.
> > https://lore.kernel.org/r/[email protected]
> >
> > V1 rework vgaarb to do all default device selection in
> > vga_arbiter_add_pci_device().
> > https://lore.kernel.org/r/[email protected]
> >
> > V2 move arbiter to PCI subsystem, fix nits.
> > https://lore.kernel.org/r/[email protected]
> >
> > V3 rewrite the commit log of the last patch (which is also summarized
> > by Bjorn).
> > https://lore.kernel.org/r/[email protected]
> >
> > V4 split the last patch to two steps.
> > https://lore.kernel.org/r/[email protected]
> >
> > V5 split Patch-9 again and sort the patches.
> > https://lore.kernel.org/r/[email protected]
> >
> > V6 split Patch-5 again and sort the patches again.
> > https://lore.kernel.org/r/[email protected]
> >
> > V7 stop moving vgaarb to drivers/pci because of ordering issues with
> > misc_init().
> > https://lore.kernel.org/r/[email protected]
> > https://lore.kernel.org/r/CAAhV-H7FhAjM-Ha42Z1dLrE4PvC9frfyeU27KHWcyWKkMftEsA@mail.gmail.com
> >
> >
> > Bjorn Helgaas (8):
> > vgaarb: Factor out vga_select_framebuffer_device()
> > vgaarb: Factor out default VGA device selection
> > vgaarb: Move framebuffer detection to ADD_DEVICE path
> > vgaarb: Move non-legacy VGA detection to ADD_DEVICE path
> > vgaarb: Move disabled VGA device detection to ADD_DEVICE path
> > vgaarb: Remove empty vga_arb_device_card_gone()
> > vgaarb: Use unsigned format string to print lock counts
> > vgaarb: Replace full MIT license text with SPDX identifier
> >
> > Huacai Chen (2):
> > vgaarb: Move vga_arb_integrated_gpu() earlier in file
> > vgaarb: Log bridge control messages when adding devices
> >
> > drivers/gpu/vga/vgaarb.c | 311 +++++++++++++++++++--------------------
> > 1 file changed, 154 insertions(+), 157 deletions(-)
> >
> > --
> > 2.25.1
> >
> >
Hi, Bjorn,
Why this series still missing in 5.17-rc1? :(
Huacai
On Fri, Jan 7, 2022 at 12:21 AM Bjorn Helgaas <[email protected]> wrote:
>
> On Thu, Jan 06, 2022 at 02:44:42PM +0800, Huacai Chen wrote:
> > On Thu, Jan 6, 2022 at 8:07 AM Bjorn Helgaas <[email protected]> wrote:
> > > Previously we selected a device that owns the boot framebuffer as the
> > > default device in vga_arb_select_default_device(). This was only done in
> > > the vga_arb_device_init() subsys_initcall, so devices enumerated later,
> > > e.g., by pcibios_init(), were not eligible.
> > >
> > > Fix this by moving the framebuffer device selection from
> > > vga_arb_select_default_device() to vga_arbiter_add_pci_device(), which is
> > > called after every PCI device is enumerated, either by the
> > > vga_arb_device_init() subsys_initcall or as an ADD_DEVICE notifier.
> > >
> > > Note that if vga_arb_select_default_device() found a device owning the boot
> > > framebuffer, it unconditionally set it to be the default VGA device, and no
> > > subsequent device could replace it.
> > >
> > > Link: https://lore.kernel.org/r/[email protected]
> > > Based-on-patch-by: Huacai Chen <[email protected]>
> > > Signed-off-by: Bjorn Helgaas <[email protected]>
> > > Cc: Bruno Prémont <[email protected]>
> > > ---
> > > drivers/gpu/vga/vgaarb.c | 37 +++++++++++++++++--------------------
> > > 1 file changed, 17 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> > > index b0ae0f177c6f..aefa4f406f7d 100644
> > > --- a/drivers/gpu/vga/vgaarb.c
> > > +++ b/drivers/gpu/vga/vgaarb.c
> > > @@ -72,6 +72,7 @@ struct vga_device {
> > > unsigned int io_norm_cnt; /* normal IO count */
> > > unsigned int mem_norm_cnt; /* normal MEM count */
> > > bool bridge_has_one_vga;
> > > + bool is_framebuffer; /* BAR covers firmware framebuffer */
> > > unsigned int (*set_decode)(struct pci_dev *pdev, bool decode);
> > > };
> > >
> > > @@ -565,10 +566,9 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc)
> > > }
> > > EXPORT_SYMBOL(vga_put);
> > >
> > > -static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
> > > +static bool vga_is_framebuffer_device(struct pci_dev *pdev)
> > > {
> > > #if defined(CONFIG_X86) || defined(CONFIG_IA64)
> > > - struct device *dev = &pdev->dev;
> > > u64 base = screen_info.lfb_base;
> > > u64 size = screen_info.lfb_size;
> > > u64 limit;
> > > @@ -583,15 +583,6 @@ static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
> > >
> > > 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(pdev, i);
> > > @@ -608,13 +599,10 @@ static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
> > > if (base < start || limit >= end)
> > > continue;
> > >
> > > - if (!vga_default_device())
> > > - vgaarb_info(dev, "setting as boot device\n");
> > > - else if (pdev != vga_default_device())
> > > - vgaarb_info(dev, "overriding boot device\n");
> > > - vga_set_default_device(pdev);
> > > + return true;
> > > }
> > > #endif
> > > + return false;
> > > }
> > >
> > > static bool vga_arb_integrated_gpu(struct device *dev)
> > > @@ -635,6 +623,7 @@ static bool vga_arb_integrated_gpu(struct device *dev)
> > > static bool vga_is_boot_device(struct vga_device *vgadev)
> > > {
> > > struct vga_device *boot_vga = vgadev_find(vga_default_device());
> > > + struct pci_dev *pdev = vgadev->pdev;
> > >
> > > /*
> > > * We select the default VGA device in this order:
> > > @@ -645,6 +634,18 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
> > > * Other device (see vga_arb_select_default_device())
> > > */
> > >
> > > + /*
> > > + * We always prefer a firmware framebuffer, so if we've already
> > > + * found one, there's no need to consider vgadev.
> > > + */
> > > + if (boot_vga && boot_vga->is_framebuffer)
> > > + return false;
> > > +
> > > + if (vga_is_framebuffer_device(pdev)) {
> > > + vgadev->is_framebuffer = true;
> > > + return true;
> > > + }
> > Maybe it is better to rename vga_is_framebuffer_device() to
> > vga_is_firmware_device() and rename is_framebuffer to
> > is_fw_framebuffer?
>
> That's a great point, thanks!
>
> The "framebuffer" term is way too generic. *All* VGA devices have a
> framebuffer, so it adds no information. This is really about finding
> the device that was used by firmware.
>
> I renamed:
>
> vga_is_framebuffer_device() -> vga_is_firmware_default()
> vga_device.is_framebuffer -> vga_device.is_firmware_default
>
> I updated my local branch and pushed it to:
> https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/vga
> with head 0f4caffa1297 ("vgaarb: Replace full MIT license text with
> SPDX identifier").
>
> I don't maintain drivers/gpu/vga/vgaarb.c, so this branch is just for
> reference. It'll ultimately be up to the DRM folks to handle this.
>
> I'll wait for any other comments or testing reports before reposting.
>
> > > /*
> > > * A legacy VGA device has MEM and IO enabled and any bridges
> > > * leading to it have PCI_BRIDGE_CTL_VGA enabled so the legacy
> > > @@ -1531,10 +1532,6 @@ static void __init vga_arb_select_default_device(void)
> > > struct pci_dev *pdev, *found = NULL;
> > > struct vga_device *vgadev;
> > >
> > > - list_for_each_entry(vgadev, &vga_list, list) {
> > > - vga_select_framebuffer_device(vgadev->pdev);
> > > - }
> > > -
> > > if (!vga_default_device()) {
> > > list_for_each_entry_reverse(vgadev, &vga_list, list) {
> > > struct device *dev = &vgadev->pdev->dev;
> > > --
> > > 2.25.1
> > >
[+cc Maarten, Maxime, Thomas; beginning of thread:
https://lore.kernel.org/r/[email protected]]
On Tue, Jan 25, 2022 at 10:51:15AM +0800, Huacai Chen wrote:
> Hi, Bjorn,
>
> Why this series still missing in 5.17-rc1? :(
1) It was posted late in the cycle (Jan 6, when the merge window
opened Jan 9), so it was too late to expect a significant change like
this to be merged for v5.17. Right now is a good time to consider it
again so it would some time in -next.
2) As of now, this code is still in drivers/gpu, and I don't maintain
that area. It's up to the DRM folks, who are all cc'd here.
I would like to move this from drivers/gpu to drivers/pci, but that
requires a little more work to resolve the initcall ordering problem
with respect to vga_arb_device_init() and misc_init() [1].
Bjorn
[1] https://lore.kernel.org/linux-pci/CAAhV-H7FhAjM-Ha42Z1dLrE4PvC9frfyeU27KHWcyWKkMftEsA@mail.gmail.com/
> On Fri, Jan 7, 2022 at 12:21 AM Bjorn Helgaas <[email protected]> wrote:
> >
> > On Thu, Jan 06, 2022 at 02:44:42PM +0800, Huacai Chen wrote:
> > > On Thu, Jan 6, 2022 at 8:07 AM Bjorn Helgaas <[email protected]> wrote:
> > > > Previously we selected a device that owns the boot framebuffer as the
> > > > default device in vga_arb_select_default_device(). This was only done in
> > > > the vga_arb_device_init() subsys_initcall, so devices enumerated later,
> > > > e.g., by pcibios_init(), were not eligible.
> > > >
> > > > Fix this by moving the framebuffer device selection from
> > > > vga_arb_select_default_device() to vga_arbiter_add_pci_device(), which is
> > > > called after every PCI device is enumerated, either by the
> > > > vga_arb_device_init() subsys_initcall or as an ADD_DEVICE notifier.
> > > >
> > > > Note that if vga_arb_select_default_device() found a device owning the boot
> > > > framebuffer, it unconditionally set it to be the default VGA device, and no
> > > > subsequent device could replace it.
> > > >
> > > > Link: https://lore.kernel.org/r/[email protected]
> > > > Based-on-patch-by: Huacai Chen <[email protected]>
> > > > Signed-off-by: Bjorn Helgaas <[email protected]>
> > > > Cc: Bruno Pr?mont <[email protected]>
> > > > ---
> > > > drivers/gpu/vga/vgaarb.c | 37 +++++++++++++++++--------------------
> > > > 1 file changed, 17 insertions(+), 20 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> > > > index b0ae0f177c6f..aefa4f406f7d 100644
> > > > --- a/drivers/gpu/vga/vgaarb.c
> > > > +++ b/drivers/gpu/vga/vgaarb.c
> > > > @@ -72,6 +72,7 @@ struct vga_device {
> > > > unsigned int io_norm_cnt; /* normal IO count */
> > > > unsigned int mem_norm_cnt; /* normal MEM count */
> > > > bool bridge_has_one_vga;
> > > > + bool is_framebuffer; /* BAR covers firmware framebuffer */
> > > > unsigned int (*set_decode)(struct pci_dev *pdev, bool decode);
> > > > };
> > > >
> > > > @@ -565,10 +566,9 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc)
> > > > }
> > > > EXPORT_SYMBOL(vga_put);
> > > >
> > > > -static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
> > > > +static bool vga_is_framebuffer_device(struct pci_dev *pdev)
> > > > {
> > > > #if defined(CONFIG_X86) || defined(CONFIG_IA64)
> > > > - struct device *dev = &pdev->dev;
> > > > u64 base = screen_info.lfb_base;
> > > > u64 size = screen_info.lfb_size;
> > > > u64 limit;
> > > > @@ -583,15 +583,6 @@ static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
> > > >
> > > > 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(pdev, i);
> > > > @@ -608,13 +599,10 @@ static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
> > > > if (base < start || limit >= end)
> > > > continue;
> > > >
> > > > - if (!vga_default_device())
> > > > - vgaarb_info(dev, "setting as boot device\n");
> > > > - else if (pdev != vga_default_device())
> > > > - vgaarb_info(dev, "overriding boot device\n");
> > > > - vga_set_default_device(pdev);
> > > > + return true;
> > > > }
> > > > #endif
> > > > + return false;
> > > > }
> > > >
> > > > static bool vga_arb_integrated_gpu(struct device *dev)
> > > > @@ -635,6 +623,7 @@ static bool vga_arb_integrated_gpu(struct device *dev)
> > > > static bool vga_is_boot_device(struct vga_device *vgadev)
> > > > {
> > > > struct vga_device *boot_vga = vgadev_find(vga_default_device());
> > > > + struct pci_dev *pdev = vgadev->pdev;
> > > >
> > > > /*
> > > > * We select the default VGA device in this order:
> > > > @@ -645,6 +634,18 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
> > > > * Other device (see vga_arb_select_default_device())
> > > > */
> > > >
> > > > + /*
> > > > + * We always prefer a firmware framebuffer, so if we've already
> > > > + * found one, there's no need to consider vgadev.
> > > > + */
> > > > + if (boot_vga && boot_vga->is_framebuffer)
> > > > + return false;
> > > > +
> > > > + if (vga_is_framebuffer_device(pdev)) {
> > > > + vgadev->is_framebuffer = true;
> > > > + return true;
> > > > + }
> > > Maybe it is better to rename vga_is_framebuffer_device() to
> > > vga_is_firmware_device() and rename is_framebuffer to
> > > is_fw_framebuffer?
> >
> > That's a great point, thanks!
> >
> > The "framebuffer" term is way too generic. *All* VGA devices have a
> > framebuffer, so it adds no information. This is really about finding
> > the device that was used by firmware.
> >
> > I renamed:
> >
> > vga_is_framebuffer_device() -> vga_is_firmware_default()
> > vga_device.is_framebuffer -> vga_device.is_firmware_default
> >
> > I updated my local branch and pushed it to:
> > https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/vga
> > with head 0f4caffa1297 ("vgaarb: Replace full MIT license text with
> > SPDX identifier").
> >
> > I don't maintain drivers/gpu/vga/vgaarb.c, so this branch is just for
> > reference. It'll ultimately be up to the DRM folks to handle this.
> >
> > I'll wait for any other comments or testing reports before reposting.
> >
> > > > /*
> > > > * A legacy VGA device has MEM and IO enabled and any bridges
> > > > * leading to it have PCI_BRIDGE_CTL_VGA enabled so the legacy
> > > > @@ -1531,10 +1532,6 @@ static void __init vga_arb_select_default_device(void)
> > > > struct pci_dev *pdev, *found = NULL;
> > > > struct vga_device *vgadev;
> > > >
> > > > - list_for_each_entry(vgadev, &vga_list, list) {
> > > > - vga_select_framebuffer_device(vgadev->pdev);
> > > > - }
> > > > -
> > > > if (!vga_default_device()) {
> > > > list_for_each_entry_reverse(vgadev, &vga_list, list) {
> > > > struct device *dev = &vgadev->pdev->dev;
> > > > --
> > > > 2.25.1
> > > >
On Tue, Jan 25, 2022 at 11:39 PM Bjorn Helgaas <[email protected]> wrote:
>
> [+cc Maarten, Maxime, Thomas; beginning of thread:
> https://lore.kernel.org/r/[email protected]]
>
> On Tue, Jan 25, 2022 at 10:51:15AM +0800, Huacai Chen wrote:
> > Hi, Bjorn,
> >
> > Why this series still missing in 5.17-rc1? :(
>
> 1) It was posted late in the cycle (Jan 6, when the merge window
> opened Jan 9), so it was too late to expect a significant change like
> this to be merged for v5.17. Right now is a good time to consider it
> again so it would some time in -next.
>
> 2) As of now, this code is still in drivers/gpu, and I don't maintain
> that area. It's up to the DRM folks, who are all cc'd here.
>
> I would like to move this from drivers/gpu to drivers/pci, but that
> requires a little more work to resolve the initcall ordering problem
> with respect to vga_arb_device_init() and misc_init() [1].
Hmm, to me, keep it in drivers/gpu is just OK.
Huacai
>
> Bjorn
>
> [1] https://lore.kernel.org/linux-pci/CAAhV-H7FhAjM-Ha42Z1dLrE4PvC9frfyeU27KHWcyWKkMftEsA@mail.gmail.com/
>
> > On Fri, Jan 7, 2022 at 12:21 AM Bjorn Helgaas <[email protected]> wrote:
> > >
> > > On Thu, Jan 06, 2022 at 02:44:42PM +0800, Huacai Chen wrote:
> > > > On Thu, Jan 6, 2022 at 8:07 AM Bjorn Helgaas <[email protected]> wrote:
> > > > > Previously we selected a device that owns the boot framebuffer as the
> > > > > default device in vga_arb_select_default_device(). This was only done in
> > > > > the vga_arb_device_init() subsys_initcall, so devices enumerated later,
> > > > > e.g., by pcibios_init(), were not eligible.
> > > > >
> > > > > Fix this by moving the framebuffer device selection from
> > > > > vga_arb_select_default_device() to vga_arbiter_add_pci_device(), which is
> > > > > called after every PCI device is enumerated, either by the
> > > > > vga_arb_device_init() subsys_initcall or as an ADD_DEVICE notifier.
> > > > >
> > > > > Note that if vga_arb_select_default_device() found a device owning the boot
> > > > > framebuffer, it unconditionally set it to be the default VGA device, and no
> > > > > subsequent device could replace it.
> > > > >
> > > > > Link: https://lore.kernel.org/r/[email protected]
> > > > > Based-on-patch-by: Huacai Chen <[email protected]>
> > > > > Signed-off-by: Bjorn Helgaas <[email protected]>
> > > > > Cc: Bruno Prémont <[email protected]>
> > > > > ---
> > > > > drivers/gpu/vga/vgaarb.c | 37 +++++++++++++++++--------------------
> > > > > 1 file changed, 17 insertions(+), 20 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> > > > > index b0ae0f177c6f..aefa4f406f7d 100644
> > > > > --- a/drivers/gpu/vga/vgaarb.c
> > > > > +++ b/drivers/gpu/vga/vgaarb.c
> > > > > @@ -72,6 +72,7 @@ struct vga_device {
> > > > > unsigned int io_norm_cnt; /* normal IO count */
> > > > > unsigned int mem_norm_cnt; /* normal MEM count */
> > > > > bool bridge_has_one_vga;
> > > > > + bool is_framebuffer; /* BAR covers firmware framebuffer */
> > > > > unsigned int (*set_decode)(struct pci_dev *pdev, bool decode);
> > > > > };
> > > > >
> > > > > @@ -565,10 +566,9 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc)
> > > > > }
> > > > > EXPORT_SYMBOL(vga_put);
> > > > >
> > > > > -static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
> > > > > +static bool vga_is_framebuffer_device(struct pci_dev *pdev)
> > > > > {
> > > > > #if defined(CONFIG_X86) || defined(CONFIG_IA64)
> > > > > - struct device *dev = &pdev->dev;
> > > > > u64 base = screen_info.lfb_base;
> > > > > u64 size = screen_info.lfb_size;
> > > > > u64 limit;
> > > > > @@ -583,15 +583,6 @@ static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
> > > > >
> > > > > 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(pdev, i);
> > > > > @@ -608,13 +599,10 @@ static void __init vga_select_framebuffer_device(struct pci_dev *pdev)
> > > > > if (base < start || limit >= end)
> > > > > continue;
> > > > >
> > > > > - if (!vga_default_device())
> > > > > - vgaarb_info(dev, "setting as boot device\n");
> > > > > - else if (pdev != vga_default_device())
> > > > > - vgaarb_info(dev, "overriding boot device\n");
> > > > > - vga_set_default_device(pdev);
> > > > > + return true;
> > > > > }
> > > > > #endif
> > > > > + return false;
> > > > > }
> > > > >
> > > > > static bool vga_arb_integrated_gpu(struct device *dev)
> > > > > @@ -635,6 +623,7 @@ static bool vga_arb_integrated_gpu(struct device *dev)
> > > > > static bool vga_is_boot_device(struct vga_device *vgadev)
> > > > > {
> > > > > struct vga_device *boot_vga = vgadev_find(vga_default_device());
> > > > > + struct pci_dev *pdev = vgadev->pdev;
> > > > >
> > > > > /*
> > > > > * We select the default VGA device in this order:
> > > > > @@ -645,6 +634,18 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
> > > > > * Other device (see vga_arb_select_default_device())
> > > > > */
> > > > >
> > > > > + /*
> > > > > + * We always prefer a firmware framebuffer, so if we've already
> > > > > + * found one, there's no need to consider vgadev.
> > > > > + */
> > > > > + if (boot_vga && boot_vga->is_framebuffer)
> > > > > + return false;
> > > > > +
> > > > > + if (vga_is_framebuffer_device(pdev)) {
> > > > > + vgadev->is_framebuffer = true;
> > > > > + return true;
> > > > > + }
> > > > Maybe it is better to rename vga_is_framebuffer_device() to
> > > > vga_is_firmware_device() and rename is_framebuffer to
> > > > is_fw_framebuffer?
> > >
> > > That's a great point, thanks!
> > >
> > > The "framebuffer" term is way too generic. *All* VGA devices have a
> > > framebuffer, so it adds no information. This is really about finding
> > > the device that was used by firmware.
> > >
> > > I renamed:
> > >
> > > vga_is_framebuffer_device() -> vga_is_firmware_default()
> > > vga_device.is_framebuffer -> vga_device.is_firmware_default
> > >
> > > I updated my local branch and pushed it to:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/vga
> > > with head 0f4caffa1297 ("vgaarb: Replace full MIT license text with
> > > SPDX identifier").
> > >
> > > I don't maintain drivers/gpu/vga/vgaarb.c, so this branch is just for
> > > reference. It'll ultimately be up to the DRM folks to handle this.
> > >
> > > I'll wait for any other comments or testing reports before reposting.
> > >
> > > > > /*
> > > > > * A legacy VGA device has MEM and IO enabled and any bridges
> > > > > * leading to it have PCI_BRIDGE_CTL_VGA enabled so the legacy
> > > > > @@ -1531,10 +1532,6 @@ static void __init vga_arb_select_default_device(void)
> > > > > struct pci_dev *pdev, *found = NULL;
> > > > > struct vga_device *vgadev;
> > > > >
> > > > > - list_for_each_entry(vgadev, &vga_list, list) {
> > > > > - vga_select_framebuffer_device(vgadev->pdev);
> > > > > - }
> > > > > -
> > > > > if (!vga_default_device()) {
> > > > > list_for_each_entry_reverse(vgadev, &vga_list, list) {
> > > > > struct device *dev = &vgadev->pdev->dev;
> > > > > --
> > > > > 2.25.1
> > > > >
[+to Maarten, Maxime, Thomas; beginning of thread:
https://lore.kernel.org/r/[email protected]]
On Wed, Jan 05, 2022 at 06:06:48PM -0600, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <[email protected]>
>
> Current default VGA device selection fails in some cases because part of it
> is done in the vga_arb_device_init() subsys_initcall, and some arches
> enumerate PCI devices in pcibios_init(), which runs *after* that.
Where are we at with this series? Is there anything I can do to move
it forward?
Bjorn
> For example:
>
> - On BMC system, the AST2500 bridge [1a03:1150] does not implement
> PCI_BRIDGE_CTL_VGA. This is perfectly legal but means the legacy VGA
> resources won't reach downstream devices unless they're included in the
> usual bridge windows.
>
> - vga_arb_select_default_device() will set a device below such a bridge
> as the default VGA device as long as it has PCI_COMMAND_IO and
> PCI_COMMAND_MEMORY enabled.
>
> - vga_arbiter_add_pci_device() is called for every VGA device, either at
> boot-time or at hot-add time, and it will also set the device as the
> default VGA device, but ONLY if all bridges leading to it implement
> PCI_BRIDGE_CTL_VGA.
>
> - This difference between vga_arb_select_default_device() and
> vga_arbiter_add_pci_device() means that a device below an AST2500 or
> similar bridge can only be set as the default if it is enumerated
> before vga_arb_device_init().
>
> - On ACPI-based systems, PCI devices are enumerated by acpi_init(), which
> runs before vga_arb_device_init().
>
> - On non-ACPI systems, like on MIPS system, they are enumerated by
> pcibios_init(), which typically runs *after* vga_arb_device_init().
>
> This series consolidates all the default VGA device selection in
> vga_arbiter_add_pci_device(), which is always called after enumerating a
> PCI device.
>
> Almost all the work here is Huacai's. I restructured it a little bit and
> added a few trivial patches on top.
>
> I'd like to move vgaarb.c to drivers/pci eventually, but there's another
> initcall ordering snag that needs to be resolved first, so this leaves
> it where it is.
>
> Bjorn
>
> Version history:
> V0 original implementation as final quirk to set default device.
> https://lore.kernel.org/r/[email protected]
>
> V1 rework vgaarb to do all default device selection in
> vga_arbiter_add_pci_device().
> https://lore.kernel.org/r/[email protected]
>
> V2 move arbiter to PCI subsystem, fix nits.
> https://lore.kernel.org/r/[email protected]
>
> V3 rewrite the commit log of the last patch (which is also summarized
> by Bjorn).
> https://lore.kernel.org/r/[email protected]
>
> V4 split the last patch to two steps.
> https://lore.kernel.org/r/[email protected]
>
> V5 split Patch-9 again and sort the patches.
> https://lore.kernel.org/r/[email protected]
>
> V6 split Patch-5 again and sort the patches again.
> https://lore.kernel.org/r/[email protected]
>
> V7 stop moving vgaarb to drivers/pci because of ordering issues with
> misc_init().
> https://lore.kernel.org/r/[email protected]
> https://lore.kernel.org/r/CAAhV-H7FhAjM-Ha42Z1dLrE4PvC9frfyeU27KHWcyWKkMftEsA@mail.gmail.com
>
>
> Bjorn Helgaas (8):
> vgaarb: Factor out vga_select_framebuffer_device()
> vgaarb: Factor out default VGA device selection
> vgaarb: Move framebuffer detection to ADD_DEVICE path
> vgaarb: Move non-legacy VGA detection to ADD_DEVICE path
> vgaarb: Move disabled VGA device detection to ADD_DEVICE path
> vgaarb: Remove empty vga_arb_device_card_gone()
> vgaarb: Use unsigned format string to print lock counts
> vgaarb: Replace full MIT license text with SPDX identifier
>
> Huacai Chen (2):
> vgaarb: Move vga_arb_integrated_gpu() earlier in file
> vgaarb: Log bridge control messages when adding devices
>
> drivers/gpu/vga/vgaarb.c | 311 +++++++++++++++++++--------------------
> 1 file changed, 154 insertions(+), 157 deletions(-)
>
> --
> 2.25.1
>
Hey,
Op 31-01-2022 om 23:23 schreef Bjorn Helgaas:
> [+to Maarten, Maxime, Thomas; beginning of thread:
> https://lore.kernel.org/r/[email protected]]
>
> On Wed, Jan 05, 2022 at 06:06:48PM -0600, Bjorn Helgaas wrote:
>> From: Bjorn Helgaas <[email protected]>
>>
>> Current default VGA device selection fails in some cases because part of it
>> is done in the vga_arb_device_init() subsys_initcall, and some arches
>> enumerate PCI devices in pcibios_init(), which runs *after* that.
> Where are we at with this series? Is there anything I can do to move
> it forward?
>
> Bjorn
Hi Bjorn,
I'm afraid that I don't understand the vga arbiter or the vga code well enough to review.
Could you perhaps find someone who could review?
I see Chen wrote some patches and tested, so perhaps they could?
~Maarten
>> For example:
>>
>> - On BMC system, the AST2500 bridge [1a03:1150] does not implement
>> PCI_BRIDGE_CTL_VGA. This is perfectly legal but means the legacy VGA
>> resources won't reach downstream devices unless they're included in the
>> usual bridge windows.
>>
>> - vga_arb_select_default_device() will set a device below such a bridge
>> as the default VGA device as long as it has PCI_COMMAND_IO and
>> PCI_COMMAND_MEMORY enabled.
>>
>> - vga_arbiter_add_pci_device() is called for every VGA device, either at
>> boot-time or at hot-add time, and it will also set the device as the
>> default VGA device, but ONLY if all bridges leading to it implement
>> PCI_BRIDGE_CTL_VGA.
>>
>> - This difference between vga_arb_select_default_device() and
>> vga_arbiter_add_pci_device() means that a device below an AST2500 or
>> similar bridge can only be set as the default if it is enumerated
>> before vga_arb_device_init().
>>
>> - On ACPI-based systems, PCI devices are enumerated by acpi_init(), which
>> runs before vga_arb_device_init().
>>
>> - On non-ACPI systems, like on MIPS system, they are enumerated by
>> pcibios_init(), which typically runs *after* vga_arb_device_init().
>>
>> This series consolidates all the default VGA device selection in
>> vga_arbiter_add_pci_device(), which is always called after enumerating a
>> PCI device.
>>
>> Almost all the work here is Huacai's. I restructured it a little bit and
>> added a few trivial patches on top.
>>
>> I'd like to move vgaarb.c to drivers/pci eventually, but there's another
>> initcall ordering snag that needs to be resolved first, so this leaves
>> it where it is.
>>
>> Bjorn
>>
>> Version history:
>> V0 original implementation as final quirk to set default device.
>> https://lore.kernel.org/r/[email protected]
>>
>> V1 rework vgaarb to do all default device selection in
>> vga_arbiter_add_pci_device().
>> https://lore.kernel.org/r/[email protected]
>>
>> V2 move arbiter to PCI subsystem, fix nits.
>> https://lore.kernel.org/r/[email protected]
>>
>> V3 rewrite the commit log of the last patch (which is also summarized
>> by Bjorn).
>> https://lore.kernel.org/r/[email protected]
>>
>> V4 split the last patch to two steps.
>> https://lore.kernel.org/r/[email protected]
>>
>> V5 split Patch-9 again and sort the patches.
>> https://lore.kernel.org/r/[email protected]
>>
>> V6 split Patch-5 again and sort the patches again.
>> https://lore.kernel.org/r/[email protected]
>>
>> V7 stop moving vgaarb to drivers/pci because of ordering issues with
>> misc_init().
>> https://lore.kernel.org/r/[email protected]
>> https://lore.kernel.org/r/CAAhV-H7FhAjM-Ha42Z1dLrE4PvC9frfyeU27KHWcyWKkMftEsA@mail.gmail.com
>>
>>
>> Bjorn Helgaas (8):
>> vgaarb: Factor out vga_select_framebuffer_device()
>> vgaarb: Factor out default VGA device selection
>> vgaarb: Move framebuffer detection to ADD_DEVICE path
>> vgaarb: Move non-legacy VGA detection to ADD_DEVICE path
>> vgaarb: Move disabled VGA device detection to ADD_DEVICE path
>> vgaarb: Remove empty vga_arb_device_card_gone()
>> vgaarb: Use unsigned format string to print lock counts
>> vgaarb: Replace full MIT license text with SPDX identifier
>>
>> Huacai Chen (2):
>> vgaarb: Move vga_arb_integrated_gpu() earlier in file
>> vgaarb: Log bridge control messages when adding devices
>>
>> drivers/gpu/vga/vgaarb.c | 311 +++++++++++++++++++--------------------
>> 1 file changed, 154 insertions(+), 157 deletions(-)
>>
>> --
>> 2.25.1
>>
Hi, Bjorn,
On Tue, Feb 8, 2022 at 1:59 AM Bjorn Helgaas <[email protected]> wrote:
>
> On Tue, Feb 01, 2022 at 04:46:33PM +0100, Maarten Lankhorst wrote:
> > Op 31-01-2022 om 23:23 schreef Bjorn Helgaas:
> > > [+to Maarten, Maxime, Thomas; beginning of thread:
> > > https://lore.kernel.org/r/[email protected]]
> > >
> > > On Wed, Jan 05, 2022 at 06:06:48PM -0600, Bjorn Helgaas wrote:
> > >> From: Bjorn Helgaas <[email protected]>
> > >>
> > >> Current default VGA device selection fails in some cases because part of it
> > >> is done in the vga_arb_device_init() subsys_initcall, and some arches
> > >> enumerate PCI devices in pcibios_init(), which runs *after* that.
> > > Where are we at with this series? Is there anything I can do to move
> > > it forward?
> >
> > I'm afraid that I don't understand the vga arbiter or the vga code
> > well enough to review.
> >
> > Could you perhaps find someone who could review?
> >
> > I see Chen wrote some patches and tested, so perhaps they could?
>
> Huacai, any chance you could review this? I'm worried that this
> series isn't going to go anywhere unless we can find somebody to
> review it.
I have reviewed and tested the whole series, it looks good to me
(except the naming which has already changed). But I thought I cannot
add a "Reviewed-by" because I was originally a co-developer. But if
necessary,
Reviewed-by: Huacai Chen <[email protected]>
Huacai
>
> Bjorn
On Tue, Feb 01, 2022 at 04:46:33PM +0100, Maarten Lankhorst wrote:
> Op 31-01-2022 om 23:23 schreef Bjorn Helgaas:
> > [+to Maarten, Maxime, Thomas; beginning of thread:
> > https://lore.kernel.org/r/[email protected]]
> >
> > On Wed, Jan 05, 2022 at 06:06:48PM -0600, Bjorn Helgaas wrote:
> >> From: Bjorn Helgaas <[email protected]>
> >>
> >> Current default VGA device selection fails in some cases because part of it
> >> is done in the vga_arb_device_init() subsys_initcall, and some arches
> >> enumerate PCI devices in pcibios_init(), which runs *after* that.
> > Where are we at with this series? Is there anything I can do to move
> > it forward?
>
> I'm afraid that I don't understand the vga arbiter or the vga code
> well enough to review.
>
> Could you perhaps find someone who could review?
>
> I see Chen wrote some patches and tested, so perhaps they could?
Huacai, any chance you could review this? I'm worried that this
series isn't going to go anywhere unless we can find somebody to
review it.
Bjorn
On Tue, Feb 01, 2022 at 04:46:33PM +0100, Maarten Lankhorst wrote:
> Hey,
> ?
> Op 31-01-2022 om 23:23 schreef Bjorn Helgaas:
> > [+to Maarten, Maxime, Thomas; beginning of thread:
> > https://lore.kernel.org/r/[email protected]]
> >
> > On Wed, Jan 05, 2022 at 06:06:48PM -0600, Bjorn Helgaas wrote:
> >> From: Bjorn Helgaas <[email protected]>
> >>
> >> Current default VGA device selection fails in some cases because
> >> part of it is done in the vga_arb_device_init() subsys_initcall,
> >> and some arches enumerate PCI devices in pcibios_init(), which
> >> runs *after* that.
> >
> > Where are we at with this series? Is there anything I can do to
> > move it forward?
>
> I'm afraid that I don't understand the vga arbiter or the vga code
> well enough to review.
>
> Could you perhaps find someone who could review?
>
> I see Chen wrote some patches and tested, so perhaps they could?
Hi Maarten,
Huacai Chen did provide his Reviewed-by (although as he noted, the
content initially came from him anyway and my contribution was mainly
rearranging things into separate patches for each specific case).
Anything else we can to do help here?
Bjorn