On Fri, Jul 23, 2021 at 05:53:36PM +0800, Huacai Chen wrote:
Hi, Bjorn,
On Fri, Jul 23, 2021 at 5:29 AM Bjorn Helgaas helgaas@kernel.org wrote:
From: Bjorn Helgaas bhelgaas@google.com
This is a little bit of rework and extension of Huacai's nice work at [1].
It moves the VGA arbiter to the PCI subsystem, fixes a few nits, and breaks a few pieces off Huacai's patch to make the main patch a little smaller.
That last patch is still not very small, and it needs a commit log, as I mentioned at [2].
All comments welcome!
[1] https://lore.kernel.org/dri-devel/20210705100503.1120643-1-chenhuacai@loongs... [2] https://lore.kernel.org/r/20210720221923.GA43331@bjorn-Precision-5520
Thank you for your splitting. Your two questions are answered in the following.
(1) explain why your initcall ordering is unusual. The original problem happens on MIPS. vga_arb_device_init() and pcibios_init() are both wrapped by subsys_initcall(). The order of functions in the same level depends on the Makefile.
TOP level Makefile: drivers-y := drivers/ sound/ .... include arch/$(SRCARCH)/Makefile
drivers/Makefile: obj-$(CONFIG_ACPI) += acpi/ .... obj-y += gpu/
arch/mips/Makefile: drivers-$(CONFIG_PCI) += arch/mips/pci/
This makes pcibios_init() in arch/mips/pci/ placed after vga_arb_device_init() in drivers/gpu. ACPI-based systems have no problems because acpi_init() in drivers/acpi is placed before vga_arb_device_init().
Thanks for the above; that was helpful. To summarize:
- On your system, the AST2500 bridge [1a03:1150] does not implement PCI_BRIDGE_CTL_VGA [1]. 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 your MIPS system, they are enumerated by pcibios_init(), which typically runs *after* vga_arb_device_init().
So I think the critical change is actually that you made vga_arb_update_default_device(), which you call from vga_arbiter_add_pci_device(), set the default device even if it does not own the VGA resources because an upstream bridge doesn't implement PCI_BRIDGE_CTL_VGA, i.e.,
(vgadev->owns & VGA_RSRC_LEGACY_MASK) != VGA_RSRC_LEGACY_MASK
Does that seem right?
[1] https://lore.kernel.org/r/CAAhV-H4pn53XC7qVvwM792ppkQRnjWpPDwmrhBv8twgQu0eab...
(2) explain the approach, which IIUC is basically to add the vga_arb_select_default_device() functionality to vga_arbiter_add_pci_device(). vga_arb_select_default_device() has only one chance to be called, we want to make it be called every time a new vga device is added. So rename it to vga_arb_update_default_device() and move the callsite to vga_arbiter_add_pci_device().
I think you know all the information which you need now. And you can reorganize the commit message based on the existing one. As English is not my first language, the updated commit message written by me may still not be as good as you want.:)
Huacai
Bjorn Helgaas (4): PCI/VGA: Move vgaarb to drivers/pci PCI/VGA: Replace full MIT license text with SPDX identifier PCI/VGA: Use unsigned format string to print lock counts PCI/VGA: Remove empty vga_arb_device_card_gone()
Huacai Chen (5): PCI/VGA: Move vga_arb_integrated_gpu() earlier in file PCI/VGA: Prefer vga_default_device() PCI/VGA: Split out vga_arb_update_default_device() PCI/VGA: Log bridge control messages when adding devices PCI/VGA: Rework default VGA device selection
drivers/gpu/vga/Kconfig | 19 --- drivers/gpu/vga/Makefile | 1 - drivers/pci/Kconfig | 19 +++ drivers/pci/Makefile | 1 + drivers/{gpu/vga => pci}/vgaarb.c | 269 ++++++++++++------------------ 5 files changed, 126 insertions(+), 183 deletions(-) rename drivers/{gpu/vga => pci}/vgaarb.c (90%)
-- 2.25.1