mbox series

[V5,00/11] PCI/VGA: Rework default VGA device selection

Message ID 20210911093056.1555274-1-chenhuacai@loongson.cn (mailing list archive)
Headers show
Series PCI/VGA: Rework default VGA device selection | expand

Message

Huacai Chen Sept. 11, 2021, 9:30 a.m. UTC 
My original work is at [1].

Current default VGA device selection fails in some cases:

  - On BMC 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 on MIPS system, they are enumerated by
    pcibios_init(), which typically runs *after*
    vga_arb_device_init().

So I made vga_arb_update_default_device() to replace the current vga_
arb_select_default_device(), which will be 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. And the default VGA device is updated if a better one is found
(device with legacy resources enabled is better, device owns the
firmware framebuffer is even better).

Bjorn do some rework and extension in V2. It moves the VGA arbiter to
the PCI subsystem, fixes a few nits, and breaks a few pieces to make
the main patch a little smaller.

V3 rewrite the commit log of the last patch (which is also summarized
by Bjorn).

V4 split the last patch to two steps.

V5 split big patches again and sort the patches.

All comments welcome!

[1] https://lore.kernel.org/dri-devel/20210705100503.1120643-1-chenhuacai@loongson.cn/

Bjorn Helgaas (4):
  PCI/VGA: Move vgaarb to drivers/pci
  PCI/VGA: Remove empty vga_arb_device_card_gone()
  PCI/VGA: Use unsigned format string to print lock counts
  PCI/VGA: Replace full MIT license text with SPDX identifier

Huacai Chen (7):
  PCI/VGA: Prefer vga_default_device()
  PCI/VGA: Move vga_arb_integrated_gpu() earlier in file
  PCI/VGA: Split out vga_arb_update_default_device()
  PCI/VGA: Update default VGA device if a better one found
  PCI/VGA: Update default VGA device again for X86/IA64
  PCI/VGA: Remove vga_arb_select_default_device()
  PCI/VGA: Log bridge control messages when adding devices

Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> 
---
 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.27.0

Comments

Bjorn Helgaas Sept. 14, 2021, 7:18 p.m. UTC | #1 
On Sat, Sep 11, 2021 at 05:30:45PM +0800, Huacai Chen wrote:
> My original work is at [1].
> 
> Current default VGA device selection fails in some cases:
> 
>   - On BMC 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 on MIPS system, they are enumerated by
>     pcibios_init(), which typically runs *after*
>     vga_arb_device_init().
> 
> So I made vga_arb_update_default_device() to replace the current vga_
> arb_select_default_device(), which will be 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. And the default VGA device is updated if a better one is found
> (device with legacy resources enabled is better, device owns the
> firmware framebuffer is even better).
> 
> Bjorn do some rework and extension in V2. It moves the VGA arbiter to
> the PCI subsystem, fixes a few nits, and breaks a few pieces to make
> the main patch a little smaller.
> 
> V3 rewrite the commit log of the last patch (which is also summarized
> by Bjorn).
> 
> V4 split the last patch to two steps.
> 
> V5 split big patches again and sort the patches.

Not sure if I'm missing something, or if this is an interim version
and you're working on a v6.

From https://lore.kernel.org/r/20210909175926.GA996660@bjorn-Precision-5520,
I was looking for:

  BUT as I mentioned, I want the very first patch to be the very
  simple 2-line change to vga_arb_update_default_device() that actually
  fixes your problem.

That doesn't seem to be what we have here.

> All comments welcome!
> 
> [1] https://lore.kernel.org/dri-devel/20210705100503.1120643-1-chenhuacai@loongson.cn/
> 
> Bjorn Helgaas (4):
>   PCI/VGA: Move vgaarb to drivers/pci
>   PCI/VGA: Remove empty vga_arb_device_card_gone()
>   PCI/VGA: Use unsigned format string to print lock counts
>   PCI/VGA: Replace full MIT license text with SPDX identifier
> 
> Huacai Chen (7):
>   PCI/VGA: Prefer vga_default_device()
>   PCI/VGA: Move vga_arb_integrated_gpu() earlier in file
>   PCI/VGA: Split out vga_arb_update_default_device()
>   PCI/VGA: Update default VGA device if a better one found
>   PCI/VGA: Update default VGA device again for X86/IA64
>   PCI/VGA: Remove vga_arb_select_default_device()
>   PCI/VGA: Log bridge control messages when adding devices
> 
> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> 
> ---
>  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.27.0
>
Huacai Chen Sept. 16, 2021, 8:28 a.m. UTC | #2 
Hi, Bjorn,

On Wed, Sep 15, 2021 at 3:18 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Sat, Sep 11, 2021 at 05:30:45PM +0800, Huacai Chen wrote:
> > My original work is at [1].
> >
> > Current default VGA device selection fails in some cases:
> >
> >   - On BMC 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 on MIPS system, they are enumerated by
> >     pcibios_init(), which typically runs *after*
> >     vga_arb_device_init().
> >
> > So I made vga_arb_update_default_device() to replace the current vga_
> > arb_select_default_device(), which will be 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. And the default VGA device is updated if a better one is found
> > (device with legacy resources enabled is better, device owns the
> > firmware framebuffer is even better).
> >
> > Bjorn do some rework and extension in V2. It moves the VGA arbiter to
> > the PCI subsystem, fixes a few nits, and breaks a few pieces to make
> > the main patch a little smaller.
> >
> > V3 rewrite the commit log of the last patch (which is also summarized
> > by Bjorn).
> >
> > V4 split the last patch to two steps.
> >
> > V5 split big patches again and sort the patches.
>
> Not sure if I'm missing something, or if this is an interim version
> and you're working on a v6.
>
> From https://lore.kernel.org/r/20210909175926.GA996660@bjorn-Precision-5520,
> I was looking for:
>
>   BUT as I mentioned, I want the very first patch to be the very
>   simple 2-line change to vga_arb_update_default_device() that actually
>   fixes your problem.
>
> That doesn't seem to be what we have here.
I try my best to split patches. But it seems very difficult to achieve
the goal of "simple 2-line change" because I cannot break a single
functional change.  E.g., I think I can try to split Patch-5 again,
but Patch-6 seems impossible.

Huacai

>
> > All comments welcome!
> >
> > [1] https://lore.kernel.org/dri-devel/20210705100503.1120643-1-chenhuacai@loongson.cn/
> >
> > Bjorn Helgaas (4):
> >   PCI/VGA: Move vgaarb to drivers/pci
> >   PCI/VGA: Remove empty vga_arb_device_card_gone()
> >   PCI/VGA: Use unsigned format string to print lock counts
> >   PCI/VGA: Replace full MIT license text with SPDX identifier
> >
> > Huacai Chen (7):
> >   PCI/VGA: Prefer vga_default_device()
> >   PCI/VGA: Move vga_arb_integrated_gpu() earlier in file
> >   PCI/VGA: Split out vga_arb_update_default_device()
> >   PCI/VGA: Update default VGA device if a better one found
> >   PCI/VGA: Update default VGA device again for X86/IA64
> >   PCI/VGA: Remove vga_arb_select_default_device()
> >   PCI/VGA: Log bridge control messages when adding devices
> >
> > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  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.27.0
> >