| Message ID | 20210722212920.347118-1-helgaas@kernel.org |
|---|---|
| Headers | show |
| Series | PCI/VGA: Rework default VGA device selection | expand |
On Thu, Jul 22, 2021 at 04:29:11PM -0500, Bjorn Helgaas 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]. FYI, I have a bunch of changes to this code that the drm maintainers picked up. They should show up in the next linux-next I think.
On Fri, Jul 23, 2021 at 06:51:59AM +0100, Christoph Hellwig wrote: > On Thu, Jul 22, 2021 at 04:29:11PM -0500, Bjorn Helgaas 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]. > > FYI, I have a bunch of changes to this code that the drm maintainers > picked up. They should show up in the next linux-next I think. Yeah I think for merging I think there'll be two options: - We also merge this series through drm-misc-next to avoid conflicts, but anything after that will (i.e. from 5.16-rc1 onwards) will go in through the pci tree. - You also merge Christoph's series, and we tell Linus to ignore the vgaarb changes that also come in through drm-next pull. It's a non-rebasing tree so taking them out isn't an option, and reverting feels silly. Either of the above is fine with me. Also I just noticed that the scrip has gone wrong for drm-misc-next and it's not actually yet in linux-next. I'll sort that out. Ok I just did sort that out while I forgot this reply draft here, one of our committers pushed a patch to the wrong branch. Luckily it was a broken one and the right fix is in the right branch (and already in Linus' tree), so a hard reset was all it took. So should be all in linux-next on the next update. -Daniel
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@loongson.cn/ > [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(). (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 >
On Fri, Jul 23, 2021 at 10:27:08AM +0200, Daniel Vetter wrote: > On Fri, Jul 23, 2021 at 06:51:59AM +0100, Christoph Hellwig wrote: > > On Thu, Jul 22, 2021 at 04:29:11PM -0500, Bjorn Helgaas 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]. > > > > FYI, I have a bunch of changes to this code that the drm maintainers > > picked up. They should show up in the next linux-next I think. > > Yeah I think for merging I think there'll be two options: > > - We also merge this series through drm-misc-next to avoid conflicts, but > anything after that will (i.e. from 5.16-rc1 onwards) will go in through > the pci tree. > > - You also merge Christoph's series, and we tell Linus to ignore the > vgaarb changes that also come in through drm-next pull. > > It's a non-rebasing tree so taking them out isn't an option, and reverting > feels silly. Either of the above is fine with me. Seems easiest/cleanest if I just fix this up so it applies on top of drm-misc-next, e.g., on top of this: 474596fc749c ("dt-bindings: display: simple-bridge: Add corpro,gm7123 compatible") I'll post a v3 after that rebase and working on the commit log from Huacai. Bjorn
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@loongson.cn/ > > [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-H4pn53XC7qVvwM792ppkQRnjWpPDwmrhBv8twgQu0eabQ@mail.gmail.com > (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 > >
Hi, Bjorn, On Sat, Jul 24, 2021 at 8:10 AM Bjorn Helgaas <helgaas@kernel.org> 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@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@loongson.cn/ > > > [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. Huacai > > [1] https://lore.kernel.org/r/CAAhV-H4pn53XC7qVvwM792ppkQRnjWpPDwmrhBv8twgQu0eabQ@mail.gmail.com > > > (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 > > >
On Fri, Jul 23, 2021 at 05:43:35PM -0500, Bjorn Helgaas wrote: > On Fri, Jul 23, 2021 at 10:27:08AM +0200, Daniel Vetter wrote: > > On Fri, Jul 23, 2021 at 06:51:59AM +0100, Christoph Hellwig wrote: > > > On Thu, Jul 22, 2021 at 04:29:11PM -0500, Bjorn Helgaas 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]. > > > > > > FYI, I have a bunch of changes to this code that the drm maintainers > > > picked up. They should show up in the next linux-next I think. > > > > Yeah I think for merging I think there'll be two options: > > > > - We also merge this series through drm-misc-next to avoid conflicts, but > > anything after that will (i.e. from 5.16-rc1 onwards) will go in through > > the pci tree. I meant 5.15-rc1 here ofc. Living a bit too much in the future :-) > > - You also merge Christoph's series, and we tell Linus to ignore the > > vgaarb changes that also come in through drm-next pull. > > > > It's a non-rebasing tree so taking them out isn't an option, and reverting > > feels silly. Either of the above is fine with me. > > Seems easiest/cleanest if I just fix this up so it applies on top of > drm-misc-next, e.g., on top of this: > > 474596fc749c ("dt-bindings: display: simple-bridge: Add corpro,gm7123 compatible") > > I'll post a v3 after that rebase and working on the commit log from > Huacai. Thanks, Daniel
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@kernel.org> 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@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@loongson.cn/ > > > > [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@mail.gmail.com > > > > > (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 > > > >
On Tue, Aug 03, 2021 at 12:06:44PM -0500, Bjorn Helgaas wrote: > 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@kernel.org> 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@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@loongson.cn/ > > > > > [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. To make sure this doesn't get lost: I'm hoping you can separate out and post the small patch to vga_arb_update_default_device(). I can look at the move/restructure stuff later. > > > [1] https://lore.kernel.org/r/CAAhV-H4pn53XC7qVvwM792ppkQRnjWpPDwmrhBv8twgQu0eabQ@mail.gmail.com > > > > > > > (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 > > > > >
On Mon, Aug 09, 2021 at 01:59:01PM -0500, Bjorn Helgaas wrote: > On Tue, Aug 03, 2021 at 12:06:44PM -0500, Bjorn Helgaas wrote: > > On Sat, Jul 24, 2021 at 05:30:02PM +0800, Huacai Chen wrote: > > > On Sat, Jul 24, 2021 at 8:10 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > 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. > > To make sure this doesn't get lost: I'm hoping you can separate out > and post the small patch to vga_arb_update_default_device(). > > I can look at the move/restructure stuff later. What's happening with this? I'm still assuming you can post a small patch to vga_arb_update_default_device() that's suitable for v5.15, Huacai. Otherwise I'm afraid we won't make any forward progress this cycle. Bjorn
Hi, Bjorn, On Fri, Aug 20, 2021 at 5:52 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Mon, Aug 09, 2021 at 01:59:01PM -0500, Bjorn Helgaas wrote: > > On Tue, Aug 03, 2021 at 12:06:44PM -0500, Bjorn Helgaas wrote: > > > On Sat, Jul 24, 2021 at 05:30:02PM +0800, Huacai Chen wrote: > > > > On Sat, Jul 24, 2021 at 8:10 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > > > 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. > > > > To make sure this doesn't get lost: I'm hoping you can separate out > > and post the small patch to vga_arb_update_default_device(). > > > > I can look at the move/restructure stuff later. > > What's happening with this? I'm still assuming you can post a small > patch to vga_arb_update_default_device() that's suitable for v5.15, > Huacai. > > Otherwise I'm afraid we won't make any forward progress this cycle. In my opinion these patches (including the last one) are small enough, so can I update the commit message of the last one and keep the patch content as is and send V3? Huacai > > Bjorn
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@loongson.cn/ [2] https://lore.kernel.org/r/20210720221923.GA43331@bjorn-Precision-5520 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%)