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: 22+ 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 ` [PATCH v2 1/9] PCI/VGA: Move vgaarb to drivers/pci Bjorn Helgaas
2021-07-22 21:38 ` Randy Dunlap
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 ` [PATCH v2 3/9] PCI/VGA: Use unsigned format string to print lock counts 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 ` [PATCH v2 5/9] PCI/VGA: Move vga_arb_integrated_gpu() earlier in file Bjorn Helgaas
2021-07-22 21:29 ` [PATCH v2 6/9] PCI/VGA: Prefer vga_default_device() 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 ` [PATCH v2 8/9] PCI/VGA: Log bridge control messages when adding devices Bjorn Helgaas
2021-07-22 21:29 ` [PATCH v2 9/9] PCI/VGA: Rework default VGA device selection Bjorn Helgaas
[not found] ` <YPpY/zRTYK3xI6rK@infradead.org>
2021-07-23 8:27 ` [PATCH v2 0/9] " Daniel Vetter
2021-07-23 22:43 ` Bjorn Helgaas
2021-07-27 9:32 ` Daniel Vetter
2021-07-23 9:53 ` Huacai Chen
2021-07-24 0:10 ` Bjorn Helgaas [this message]
2021-07-24 9:30 ` Huacai Chen
2021-08-03 17:06 ` Bjorn Helgaas
2021-08-09 18:59 ` Bjorn Helgaas
2021-08-19 21:52 ` Bjorn Helgaas
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=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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).