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

Alex Deucher alexdeucher at gmail.com
Fri Jun 16 13:41:07 UTC 2023


On Fri, Jun 16, 2023 at 3:11 AM Sui Jingfeng <suijingfeng at loongson.cn> wrote:
>
> Hi,
>
> On 2023/6/16 05:11, Alex Deucher wrote:
> > On Wed, Jun 14, 2023 at 6:50 AM Sui Jingfeng <suijingfeng at loongson.cn> wrote:
> >> Hi,
> >>
> >> On 2023/6/13 11:01, Sui Jingfeng wrote:
> >>> From: Sui Jingfeng <suijingfeng at loongson.cn>
> >>>
> >>> 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 at google.com>
> >>> Signed-off-by: Sui Jingfeng <suijingfeng at loongson.cn>
> >>> ---
> >>>    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
>


More information about the dri-devel mailing list