Re: [PATCH] vgaarb: Add module param to allow for choosing the boot VGA device

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc linux-pci]

On Mon, Jul 04, 2022 at 12:33:50AM -0400, Cal Peake wrote:
> My first attempt at solving this problem was based on a bit of a 
> misunderstanding of the code and, while it worked, could be much improved.

Can you outline the problem this solves in the commit log?

> This is a new attempt that creates the module parameter 'vgaarb.bootdev' 
> that can be passed a PCI ID (e.g. 0a:00.0) that the VGA arbiter will 
> attempt to use as the boot VGA device over any other eligible devices.
> 
> If the passed ID is invalid or is an ineligible device, the arbiter will 
> fallback to its normal method of trying to find the preferred device for 
> the job.
> 
> I've tested it thoroughly and it is working very well for me.
> 
> The patch is available below for review and, if found acceptable, can be 
> pulled via:
> 
>   git pull https://github.com/peake/linux.git vgaarb-2
> 
> -- 
> Cal Peake
> 
> Signed-off-by: Cal Peake <cp@absolutedigital.net>
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 2522b11e593f..21ac87f4a8a9 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -6518,6 +6518,13 @@
>  			This is actually a boot loader parameter; the value is
>  			passed to the kernel using a special protocol.
>  
> +	vgaarb.bootdev=	[PCI] Specify the PCI ID (e.g. 0e:00.0) of the
> +			device to use as the boot VGA device, overriding
> +			the heuristic used to normally determine which
> +			of the eligible VGA devices to use. If the device
> +			specified is not valid or not eligible, then we
> +			fallback to the heuristic.
> +
>  	vm_debug[=options]	[KNL] Available with CONFIG_DEBUG_VM=y.
>  			May slow down system boot speed, especially when
>  			enabled on systems with a large amount of memory.
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index f80b6ec88dc3..d3689b7dc63d 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -35,6 +35,34 @@
>  
>  #include <linux/vgaarb.h>
>  
> +static char *bootdev __initdata;
> +module_param(bootdev, charp, 0);
> +MODULE_PARM_DESC(bootdev, "Force boot device to the specified PCI ID");
> +
> +/*
> + * Initialize to the last possible ID to have things work as normal
> + * when no 'bootdev' option is supplied. We especially do not want
> + * this to be zero (0) since that is a valid PCI ID (00:00.0).
> + */
> +static u16 bootdev_id = 0xffff;
> +
> +static void __init parse_bootdev(char *input)
> +{
> +	unsigned int bus, dev, func;
> +	int ret;
> +
> +	if (input == NULL)
> +		return;
> +
> +	ret = sscanf(input, "%x:%x.%x", &bus, &dev, &func);
> +	if (ret != 3) {
> +		pr_warn("Improperly formatted PCI ID: %s\n", input);
> +		return;
> +	}
> +
> +	bootdev_id = PCI_DEVID(bus, PCI_DEVFN(dev, func));
> +}
> +
>  static void vga_arbiter_notify_clients(void);
>  /*
>   * We keep a list of all vga devices in the system to speed
> @@ -53,6 +81,7 @@ struct vga_device {
>  	bool bridge_has_one_vga;
>  	bool is_firmware_default;	/* device selected by firmware */
>  	unsigned int (*set_decode)(struct pci_dev *pdev, bool decode);
> +	bool is_chosen_one;		/* device specified on command line */
>  };
>  
>  static LIST_HEAD(vga_list);
> @@ -605,6 +634,7 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
>  
>  	/*
>  	 * We select the default VGA device in this order:
> +	 *   User specified device (see module param bootdev=)
>  	 *   Firmware framebuffer (see vga_arb_select_default_device())
>  	 *   Legacy VGA device (owns VGA_RSRC_LEGACY_MASK)
>  	 *   Non-legacy integrated device (see vga_arb_select_default_device())
> @@ -612,6 +642,14 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
>  	 *   Other device (see vga_arb_select_default_device())
>  	 */
>  
> +	if (boot_vga && boot_vga->is_chosen_one)
> +		return false;
> +
> +	if (bootdev_id == PCI_DEVID(pdev->bus->number, pdev->devfn)) {
> +		vgadev->is_chosen_one = true;
> +		return true;
> +	}
> +
>  	/*
>  	 * We always prefer a firmware default device, so if we've already
>  	 * found one, there's no need to consider vgadev.
> @@ -1544,6 +1582,8 @@ static int __init vga_arb_device_init(void)
>  	int rc;
>  	struct pci_dev *pdev;
>  
> +	parse_bootdev(bootdev);
> +
>  	rc = misc_register(&vga_arb_device);
>  	if (rc < 0)
>  		pr_err("error %d registering device\n", rc);