Re: [PATCH v7 2/8] PCI/VGA: Deal only with VGA class devices
From: Alex Deucher
Date: Fri Jun 16 2023 - 09:41:24 EST
On Fri, Jun 16, 2023 at 3:11 AM Sui Jingfeng <suijingfeng@xxxxxxxxxxx> wrote:
>
> Hi,
>
> On 2023/6/16 05:11, Alex Deucher wrote:
> > On Wed, Jun 14, 2023 at 6:50 AM Sui Jingfeng <suijingfeng@xxxxxxxxxxx> wrote:
> >> Hi,
> >>
> >> On 2023/6/13 11:01, Sui Jingfeng wrote:
> >>> From: Sui Jingfeng <suijingfeng@xxxxxxxxxxx>
> >>>
> >>> Deal only with the VGA devcie(pdev->class == 0x0300), so replace the
> >>> pci_get_subsys() function with pci_get_class(). Filter the non-PCI display
> >>> device(pdev->class != 0x0300) out. There no need to process the non-display
> >>> PCI device.
> >>>
> >>> Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> >>> Signed-off-by: Sui Jingfeng <suijingfeng@xxxxxxxxxxx>
> >>> ---
> >>> drivers/pci/vgaarb.c | 22 ++++++++++++----------
> >>> 1 file changed, 12 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> >>> index c1bc6c983932..22a505e877dc 100644
> >>> --- a/drivers/pci/vgaarb.c
> >>> +++ b/drivers/pci/vgaarb.c
> >>> @@ -754,10 +754,6 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
> >>> struct pci_dev *bridge;
> >>> u16 cmd;
> >>>
> >>> - /* Only deal with VGA class devices */
> >>> - if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
> >>> - return false;
> >>> -
> >> Hi, here is probably a bug fixing.
> >>
> >> For an example, nvidia render only GPU typically has 0x0380.
> >>
> >> as its PCI class number, but render only GPU should not participate in
> >> the arbitration.
> >>
> >> As it shouldn't snoop the legacy fixed VGA address.
> >>
> >> It(render only GPU) can not display anything.
> >>
> >>
> >> But 0x0380 >> 8 = 0x03, the filter failed.
> >>
> >>
> >>> /* Allocate structure */
> >>> vgadev = kzalloc(sizeof(struct vga_device), GFP_KERNEL);
> >>> if (vgadev == NULL) {
> >>> @@ -1500,7 +1496,9 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,
> >>> struct pci_dev *pdev = to_pci_dev(dev);
> >>> bool notify = false;
> >>>
> >>> - vgaarb_dbg(dev, "%s\n", __func__);
> >>> + /* Only deal with VGA class devices */
> >>> + if (pdev->class != PCI_CLASS_DISPLAY_VGA << 8)
> >>> + return 0;
> >> So here we only care 0x0300, my initial intent is to make an optimization,
> >>
> >> nowadays sane display graphic card should all has 0x0300 as its PCI
> >> class number, is this complete right?
> >>
> >> ```
> >>
> >> #define PCI_BASE_CLASS_DISPLAY 0x03
> >> #define PCI_CLASS_DISPLAY_VGA 0x0300
> >> #define PCI_CLASS_DISPLAY_XGA 0x0301
> >> #define PCI_CLASS_DISPLAY_3D 0x0302
> >> #define PCI_CLASS_DISPLAY_OTHER 0x0380
> >>
> >> ```
> >>
> >> Any ideas ?
> > I'm not quite sure what you are asking about here.
>
> To be honest, I'm worried about the PCI devices which has a
>
> PCI_CLASS_DISPLAY_XGA as its PCI class number.
>
> As those devices are very uncommon in the real world.
>
>
> $ find . -name "*.c" -type f | xargs grep "PCI_CLASS_DISPLAY_XGA"
>
>
> Grep the "PCI_CLASS_DISPLAY_XGA" in the linux kernel tree got ZERO,
>
> there no code reference this macro. So I think it seems safe to ignore
> the XGA ?
>
>
> PCI_CLASS_DISPLAY_3D and PCI_CLASS_DISPLAY_OTHER are used to annotate
> the render-only GPU.
>
> And render-only GPU can't decode the fixed VGA address space, it is safe
> to ignore them.
>
>
> > For vga_arb, we
> > only care about VGA class devices since those should be on the only
> > ones that might have VGA routed to them.
>
> > However, as VGA gets deprecated,
>
> We need the vgaarb for a system with multiple video card.
>
> Not only because some Legacy VGA devices implemented
>
> on PCI will typically have the same "hard-decoded" addresses;
>
> But also these video card need to participate in the arbitration,
>
> determine the default boot device.
But couldn't the boot device be determined via what whatever resources
were used by the pre-OS console? I feel like that should be separate
from vgaarb. vgaarb should handle PCI VGA routing and some other
mechanism should be used to determine what device provided the pre-OS
console.
Alex
>
>
> Nowadays, the 'VGA devices' here is stand for the Graphics card
>
> which is capable of display something on the screen.
>
> We still need vgaarb to select the default boot device.
>
>
> > you'll have more non VGA PCI classes for devices which
> > could be the pre-OS console device.
>
> Ah, we still want do this(by applying this patch) first,
>
> and then we will have the opportunity to see who will crying if
> something is broken. Will know more then.
>
> But drop this patch or revise it with more consideration is also
> acceptable.
>
>
> I asking about suggestion and/or review.
>
> > Alex
> >
> >>> /* For now we're only intereted in devices added and removed. I didn't
> >>> * test this thing here, so someone needs to double check for the
> >>> @@ -1510,6 +1508,8 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,
> >>> else if (action == BUS_NOTIFY_DEL_DEVICE)
> >>> notify = vga_arbiter_del_pci_device(pdev);
> >>>
> >>> + vgaarb_dbg(dev, "%s: action = %lu\n", __func__, action);
> >>> +
> >>> if (notify)
> >>> vga_arbiter_notify_clients();
> >>> return 0;
> >>> @@ -1534,8 +1534,8 @@ static struct miscdevice vga_arb_device = {
> >>>
> >>> static int __init vga_arb_device_init(void)
> >>> {
> >>> + struct pci_dev *pdev = NULL;
> >>> int rc;
> >>> - struct pci_dev *pdev;
> >>>
> >>> rc = misc_register(&vga_arb_device);
> >>> if (rc < 0)
> >>> @@ -1545,11 +1545,13 @@ static int __init vga_arb_device_init(void)
> >>>
> >>> /* We add all PCI devices satisfying VGA class in the arbiter by
> >>> * default */
> >>> - pdev = NULL;
> >>> - while ((pdev =
> >>> - pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
> >>> - PCI_ANY_ID, pdev)) != NULL)
> >>> + while (1) {
> >>> + pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev);
> >>> + if (!pdev)
> >>> + break;
> >>> +
> >>> vga_arbiter_add_pci_device(pdev);
> >>> + }
> >>>
> >>> pr_info("loaded\n");
> >>> return rc;
> >> --
> >> Jingfeng
> >>
> --
> Jingfeng
>