Hi, Bjorn, On Sat, May 15, 2021 at 5:09 PM Huacai Chen <chenhuacai@xxxxxxxxx> wrote: > > Hi, Bjorn, > > On Fri, May 14, 2021 at 11:10 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > > > > On Fri, May 14, 2021 at 04:00:25PM +0800, Huacai Chen wrote: > > > According to PCI-to-PCI bridge spec, bit 3 of Bridge Control Register is > > > VGA Enable bit which modifies the response to VGA compatible addresses. > > > > The bridge spec is pretty old, and most of the content has been > > incorporated into the PCIe spec. I think you can cite "PCIe r5.0, sec > > 7.5.1.3.13" here instead. > > > > > If the VGA Enable bit is set, the bridge will decode and forward the > > > following accesses on the primary interface to the secondary interface. > > > > *Which* following accesses? The structure of English requires that if > > you say "the following accesses," you must continue by *listing* the > > accesses. > > > > > The ASpeed AST2500 hardward does not set the VGA Enable bit on its > > > bridge control register, which causes vgaarb subsystem don't think the > > > VGA card behind the bridge as a valid boot vga device. > > > > s/hardward/bridge/ > > s/vga/VGA/ (also in code comments and dmesg strings below) > > > > From the code, it looks like AST2500 ([1a03:2000]) is a VGA device, > > since it apparently has a VGA class code. But here you say the > > AST2500 has a Bridge Control register, which suggests that it's a > > bridge. If AST2500 is some sort of combination that includes both a > > bridge and a VGA device, please outline that topology. > > > > But the hardware defect is that some bridges forward VGA accesses even > > though their VGA Enable bit is not set? The quirk should be attached > > to broken *bridges*, not to VGA devices. > > > > If a bridge forwards VGA accesses regardless of how its VGA Enable bit > > is set, that means VGA arbitration (in vgaarb.c) cannot work > > correctly, so merely setting the default VGA device once in a quirk is > > not sufficient. You would have to somehow disable any future attempts > > to use other VGA devices. Only the VGA device below this defective > > bridge is usable. Any other VGA devices in the system would be > > useless. > > > > > So we provide a quirk to fix Xorg auto-detection. > > > > > > See similar bug: > > > > > > https://patchwork.kernel.org/project/linux-pci/patch/20170619023528.11532-1-dja@xxxxxxxxxx/ > > > > This patch was never merged. If we merged a revised version, please > > cite the SHA1 instead. > This patch has never merged, and I found that it is unnecessary after > commit a37c0f48950b56f6ef2ee637 ("vgaarb: Select a default VGA device > even if there's no legacy VGA"). Maybe this ASpeed patch is also > unnecessary. If it is still needed, I'll investigate the root cause. I found that vga_arb_device_init() and pcibios_init() are both wrapped by subsys_initcall(), which means their sequence is unpredictable. And unfortunately, in our platform vga_arb_device_init() is called before pcibios_init(), which makes vga_arb_device_init() fail to set a default vga device. This is the root cause why we thought that we still need a quirk for AST2500. I think the best solution is make vga_arb_device_init() be wrapped by subsys_initcall_sync(), do you think so? Huacai > > Huacai > > > > > Signed-off-by: Huacai Chen <chenhuacai@xxxxxxxxxxx> > > > Signed-off-by: Jingfeng Sui <suijingfeng@xxxxxxxxxxx> > > > --- > > > drivers/pci/quirks.c | 47 ++++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 47 insertions(+) > > > > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > > > index 6ab4b3bba36b..adf5490706ad 100644 > > > --- a/drivers/pci/quirks.c > > > +++ b/drivers/pci/quirks.c > > > @@ -28,6 +28,7 @@ > > > #include <linux/platform_data/x86/apple.h> > > > #include <linux/pm_runtime.h> > > > #include <linux/switchtec.h> > > > +#include <linux/vgaarb.h> > > > #include <asm/dma.h> /* isa_dma_bridge_buggy */ > > > #include "pci.h" > > > > > > @@ -297,6 +298,52 @@ static void loongson_mrrs_quirk(struct pci_dev *dev) > > > } > > > DECLARE_PCI_FIXUP_ENABLE(PCI_ANY_ID, PCI_ANY_ID, loongson_mrrs_quirk); > > > > > > + > > > +static void aspeed_fixup_vgaarb(struct pci_dev *pdev) > > > +{ > > > + struct pci_dev *bridge; > > > + struct pci_bus *bus; > > > + struct pci_dev *vdevp = NULL; > > > + u16 config; > > > + > > > + bus = pdev->bus; > > > + bridge = bus->self; > > > + > > > + /* Is VGA routed to us? */ > > > + if (bridge && (pci_is_bridge(bridge))) { > > > + pci_read_config_word(bridge, PCI_BRIDGE_CONTROL, &config); > > > + > > > + /* Yes, this bridge is PCI bridge-to-bridge spec compliant, > > > + * just return! > > > + */ > > > + if (config & PCI_BRIDGE_CTL_VGA) > > > + return; > > > + > > > + dev_warn(&pdev->dev, "VGA bridge control is not enabled\n"); > > > + } > > > > You cannot assume that a bridge is defective just because > > PCI_BRIDGE_CTL_VGA is not set. > > > > > + /* Just return if the system already have a default device */ > > > + if (vga_default_device()) > > > + return; > > > + > > > + /* No default vga device */ > > > + while ((vdevp = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, vdevp))) { > > > + if (vdevp->vendor != 0x1a03) { > > > + /* Have other vga devcie in the system, do nothing */ > > > + dev_info(&pdev->dev, "Another boot vga device: 0x%x:0x%x\n", > > > + vdevp->vendor, vdevp->device); > > > + return; > > > + } > > > + } > > > + > > > + vga_set_default_device(pdev); > > > + > > > + dev_info(&pdev->dev, "Boot vga device set as 0x%x:0x%x\n", > > > + pdev->vendor, pdev->device); > > > +} > > > +DECLARE_PCI_FIXUP_CLASS_FINAL(0x1a03, 0x2000, PCI_CLASS_DISPLAY_VGA, 8, aspeed_fixup_vgaarb); > > > + > > > + > > > /* > > > * The Mellanox Tavor device gives false positive parity errors. Disable > > > * parity error reporting. > > > -- > > > 2.27.0 > > >
