From: Bjorn Helgaas <helgaas@kernel.org> To: Huacai Chen <chenhuacai@kernel.org> Cc: Huacai Chen <chenhuacai@loongson.cn>, David Airlie <airlied@linux.ie>, dri-devel@lists.freedesktop.org, linux-pci@vger.kernel.org, Bjorn Helgaas <bhelgaas@google.com>, Xuefeng Li <lixuefeng@loongson.cn>, Christoph Hellwig <hch@infradead.org>, Daniel Vetter <daniel@ffwll.ch> Subject: Re: [PATCH v2 0/9] PCI/VGA: Rework default VGA device selection Date: Fri, 23 Jul 2021 19:10:43 -0500 [thread overview] Message-ID: <20210724001043.GA448782@bjorn-Precision-5520> (raw) In-Reply-To: <CAAhV-H52feAf0Qf7xHa2uyv1veX+dBgDr3QKXjOZzpd=wcUr3Q@mail.gmail.com> 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 > >
WARNING: multiple messages have this Message-ID (diff)
From: Bjorn Helgaas <helgaas@kernel.org> To: Huacai Chen <chenhuacai@kernel.org> Cc: David Airlie <airlied@linux.ie>, linux-pci@vger.kernel.org, dri-devel@lists.freedesktop.org, Christoph Hellwig <hch@infradead.org>, Bjorn Helgaas <bhelgaas@google.com>, Xuefeng Li <lixuefeng@loongson.cn>, Huacai Chen <chenhuacai@loongson.cn> Subject: Re: [PATCH v2 0/9] PCI/VGA: Rework default VGA device selection Date: Fri, 23 Jul 2021 19:10:43 -0500 [thread overview] Message-ID: <20210724001043.GA448782@bjorn-Precision-5520> (raw) In-Reply-To: <CAAhV-H52feAf0Qf7xHa2uyv1veX+dBgDr3QKXjOZzpd=wcUr3Q@mail.gmail.com> 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 > >
next prev parent reply other threads:[~2021-07-24 0:10 UTC|newest] Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-07-22 21:29 [PATCH v2 0/9] PCI/VGA: Rework default VGA device selection Bjorn Helgaas 2021-07-22 21:29 ` Bjorn Helgaas 2021-07-22 21:29 ` [PATCH v2 1/9] PCI/VGA: Move vgaarb to drivers/pci Bjorn Helgaas 2021-07-22 21:29 ` Bjorn Helgaas 2021-07-22 21:38 ` Randy Dunlap 2021-07-22 21:38 ` Randy Dunlap 2021-07-22 21:55 ` Bjorn Helgaas 2021-07-22 21:55 ` Bjorn Helgaas 2021-07-22 21:29 ` [PATCH v2 2/9] PCI/VGA: Replace full MIT license text with SPDX identifier Bjorn Helgaas 2021-07-22 21:29 ` Bjorn Helgaas 2021-07-22 21:29 ` [PATCH v2 3/9] PCI/VGA: Use unsigned format string to print lock counts Bjorn Helgaas 2021-07-22 21:29 ` Bjorn Helgaas 2021-07-22 21:29 ` [PATCH v2 4/9] PCI/VGA: Remove empty vga_arb_device_card_gone() Bjorn Helgaas 2021-07-22 21:29 ` Bjorn Helgaas 2021-07-22 21:29 ` [PATCH v2 5/9] PCI/VGA: Move vga_arb_integrated_gpu() earlier in file Bjorn Helgaas 2021-07-22 21:29 ` Bjorn Helgaas 2021-07-22 21:29 ` [PATCH v2 6/9] PCI/VGA: Prefer vga_default_device() Bjorn Helgaas 2021-07-22 21:29 ` Bjorn Helgaas 2021-07-22 21:29 ` [PATCH v2 7/9] PCI/VGA: Split out vga_arb_update_default_device() Bjorn Helgaas 2021-07-22 21:29 ` Bjorn Helgaas 2021-07-22 21:29 ` [PATCH v2 8/9] PCI/VGA: Log bridge control messages when adding devices Bjorn Helgaas 2021-07-22 21:29 ` Bjorn Helgaas 2021-07-22 21:29 ` [PATCH v2 9/9] PCI/VGA: Rework default VGA device selection Bjorn Helgaas 2021-07-22 21:29 ` Bjorn Helgaas 2021-07-23 5:51 ` [PATCH v2 0/9] " Christoph Hellwig 2021-07-23 8:27 ` Daniel Vetter 2021-07-23 8:27 ` Daniel Vetter 2021-07-23 22:43 ` Bjorn Helgaas 2021-07-23 22:43 ` Bjorn Helgaas 2021-07-27 9:32 ` Daniel Vetter 2021-07-27 9:32 ` Daniel Vetter 2021-07-23 9:53 ` Huacai Chen 2021-07-23 9:53 ` Huacai Chen 2021-07-24 0:10 ` Bjorn Helgaas [this message] 2021-07-24 0:10 ` Bjorn Helgaas 2021-07-24 9:30 ` Huacai Chen 2021-07-24 9:30 ` Huacai Chen 2021-08-03 17:06 ` Bjorn Helgaas 2021-08-03 17:06 ` Bjorn Helgaas 2021-08-09 18:59 ` Bjorn Helgaas 2021-08-09 18:59 ` Bjorn Helgaas 2021-08-19 21:52 ` Bjorn Helgaas 2021-08-19 21:52 ` Bjorn Helgaas 2021-08-20 4:07 ` Huacai Chen 2021-08-20 4:07 ` Huacai Chen
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210724001043.GA448782@bjorn-Precision-5520 \ --to=helgaas@kernel.org \ --cc=airlied@linux.ie \ --cc=bhelgaas@google.com \ --cc=chenhuacai@kernel.org \ --cc=chenhuacai@loongson.cn \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=hch@infradead.org \ --cc=linux-pci@vger.kernel.org \ --cc=lixuefeng@loongson.cn \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.