[PATCH 2/6] PCI/VGA: Deal with PCI VGA compatible devices only

[suijingfeng]

Hi,


On 2023/7/11 21:43, Sui Jingfeng wrote:
> From: Sui Jingfeng<suijingfeng at loongson.cn>
>
> Currently, vgaarb only cares about PCI VGA-compatible class devices.
>
> While vga_arbiter_del_pci_device() gets called unbalanced when some PCI
> device is about to be removed. This happens even during the boot process.
>
> Another reason is that the vga_arb_device_init() function is not efficient.
> Since we only care about VGA-compatible devices (pdev->class == 0x030000),
> We could filter the unqualified devices out in the vga_arb_device_init()
> function. While the current implementation is to search all PCI devices
> in a system, this is not necessary.
>
> Reviewed-by: Mario Limonciello<mario.limonciello at amd.com>
> Signed-off-by: Sui Jingfeng<suijingfeng at loongson.cn>
> ---
>   drivers/pci/vgaarb.c | 25 +++++++++++++------------
>   1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index c1bc6c983932..021116ed61cb 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;
> -
>   	/* Allocate structure */
>   	vgadev = kzalloc(sizeof(struct vga_device), GFP_KERNEL);
>   	if (vgadev == NULL) {
> @@ -1502,6 +1498,10 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,
>   
>   	vgaarb_dbg(dev, "%s\n", __func__);
>   
> +	/* Deal with VGA compatible devices only */
> +	if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
> +		return 0;
> +
>   	/* 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
>   	 * cases of hotplugable vga cards. */
> @@ -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)
> @@ -1543,13 +1543,14 @@ static int __init vga_arb_device_init(void)
>   
>   	bus_register_notifier(&pci_bus_type, &pci_notifier);
>   
> -	/* 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)
> -		vga_arbiter_add_pci_device(pdev);
> +	/*
> +	 * We add all PCI VGA compatible devices in the arbiter by default
> +	 */
> +	do {
> +		pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev);
> +		if (pdev)
> +			vga_arbiter_add_pci_device(pdev);
> +	} while (pdev);

I suddenly remember one more thing that I forget to explain.

In a previous thread[1] of previous version of this series,

Maciej seems argue that PCI_CLASS_NOT_DEFINED_VGA should be handled also.

But, even I try to handled PCI_CLASS_NOT_DEFINED_VGA at here,

this card still will not be annotate as boot_vga,

because the pci_dev_attrs_are_visible() function only consider VGA compatible devices

which (pdev->class >> 8 == PCI_CLASS_DISPLAY_VGA) is true. See [2].


Intel integrated GPU is more intelligent,

which could change their class of the PCI(e) device to 0x038000 when

multiple GPU co-exist. Even though the GPU can display. This is configurable by

the firmware, but this is trying to escape the arbitration by changing is PCI id.


So, PCI devices belong to the PCI_CLASS_DISPLAY_OTHER, PCI_CLASS_DISPLAY_3D and PCI_CLASS_DISPLAY_XGA

can not participate in the arbitration. They are all will be get filtered.

So, this is a limitation of the vgaarb itself.

While I could help to improve it in the future, what do you think?

Is my express clear?

[1] 
https://lkml.kernel.org/nouveau/alpine.DEB.2.21.2306190339590.14084@angie.orcam.me.uk/#t

[2] 
https://elixir.bootlin.com/linux/latest/source/drivers/pci/pci-sysfs.c#L1544

>   
>   	pr_info("loaded\n");
>   	return rc;
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230717/cf379811/attachment.htm>