Re: [PATCH v7 2/8] PCI/VGA: Deal only with VGA class devices

[Alex Deucher]

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.
>
> at 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.  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, you'll have more non VGA PCI classes for devices which
could be the pre-OS console device.

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
>