Re: [PATCH 5/5] PCI: Support ASpeed VGA cards behind a misbehaving bridge

[Bjorn Helgaas <helgaas@kernel.org>]

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@axtens.net/

This patch was never merged.  If we merged a revised version, please
cite the SHA1 instead.

> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> Signed-off-by: Jingfeng Sui <suijingfeng@loongson.cn>
> ---
>  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
>