On Sat, Jul 24, 2021 at 05:30:02PM +0800, Huacai Chen wrote: > Hi, Bjorn, > > On Sat, Jul 24, 2021 at 8:10 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > > > > 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@xxxxxxxxxx> wrote: > > > > > > > > From: Bjorn Helgaas <bhelgaas@xxxxxxxxxx> > > > > > > > > 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@xxxxxxxxxxx/ > > > > [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? > > Yes, that's right. I think that means I screwed up. I somehow had it in my head that the hot-add path would never set the default VGA device. But that is false. I still think we should move vgaarb.c to drivers/pci/ and get it more tightly integrated into the PCI core. BUT that's a lot of churn and obscures the simple change that fixes the problem for you. So I think the first step should be the change to vga_arb_update_default_device() so it sets the default device even when the upstream bridge doesn't implement PCI_BRIDGE_CTL_VGA. That should be a relatively small change, and I think it's better to make the fix before embarking on major restructuring. > > [1] https://lore.kernel.org/r/CAAhV-H4pn53XC7qVvwM792ppkQRnjWpPDwmrhBv8twgQu0eabQ@xxxxxxxxxxxxxx > > > > > (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 > > > >
