Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 16 Sep 2021 11:37:55 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Kai-Heng Feng <kai.heng.feng@canonical.com>
Cc:     airlied@linux.ie, daniel@ffwll.ch,
        maarten.lankhorst@linux.intel.com, mripard@kernel.org,
        tzimmermann@suse.de, alexander.deucher@amd.com,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        linux-pci@vger.kernel.org, Huacai Chen <chenhuacai@kernel.org>
Subject: Re: [PATCH] vgaarb: Use ACPI HID name to find integrated GPU
Message-ID: <20210916163755.GA1620802@bjorn-Precision-5520>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210519135723.525997-1-kai.heng.feng@canonical.com>
Precedence: bulk

[+cc Huacai, linux-pci]

On Wed, May 19, 2021 at 09:57:23PM +0800, Kai-Heng Feng wrote:
> Commit 3d42f1ddc47a ("vgaarb: Keep adding VGA device in queue") assumes
> the first device is an integrated GPU. However, on AMD platforms an
> integrated GPU can have higher PCI device number than a discrete GPU.
> 
> Integrated GPU on ACPI platform generally has _DOD and _DOS method, so
> use that as predicate to find integrated GPU. If the new strategy
> doesn't work, fallback to use the first device as boot VGA.
> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  drivers/gpu/vga/vgaarb.c | 31 ++++++++++++++++++++++++++-----
>  1 file changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> index 5180c5687ee5..949fde433ea2 100644
> --- a/drivers/gpu/vga/vgaarb.c
> +++ b/drivers/gpu/vga/vgaarb.c
> @@ -50,6 +50,7 @@
>  #include <linux/screen_info.h>
>  #include <linux/vt.h>
>  #include <linux/console.h>
> +#include <linux/acpi.h>
>  
>  #include <linux/uaccess.h>
>  
> @@ -1450,9 +1451,23 @@ static struct miscdevice vga_arb_device = {
>  	MISC_DYNAMIC_MINOR, "vga_arbiter", &vga_arb_device_fops
>  };
>  
> +#if defined(CONFIG_ACPI)
> +static bool vga_arb_integrated_gpu(struct device *dev)
> +{
> +	struct acpi_device *adev = ACPI_COMPANION(dev);
> +
> +	return adev && !strcmp(acpi_device_hid(adev), ACPI_VIDEO_HID);
> +}
> +#else
> +static bool vga_arb_integrated_gpu(struct device *dev)
> +{
> +	return false;
> +}
> +#endif
> +
>  static void __init vga_arb_select_default_device(void)
>  {
> -	struct pci_dev *pdev;
> +	struct pci_dev *pdev, *found = NULL;
>  	struct vga_device *vgadev;
>  
>  #if defined(CONFIG_X86) || defined(CONFIG_IA64)
> @@ -1505,20 +1520,26 @@ static void __init vga_arb_select_default_device(void)
>  #endif
>  
>  	if (!vga_default_device()) {
> -		list_for_each_entry(vgadev, &vga_list, list) {
> +		list_for_each_entry_reverse(vgadev, &vga_list, list) {

Hi Kai-Heng, do you remember why you changed the order of this list
traversal?

I guess the list_add_tail() in vga_arbiter_add_pci_device() means
vga_list is generally ordered with small device numbers first and
large ones last.

So you pick the integrated GPU with the largest device number.  Are
there systems with more than one integrated GPU?  If so, I would
naively expect that in the absence of an indication otherwise, we'd
want the one with the *smallest* device number.

>  			struct device *dev = &vgadev->pdev->dev;
>  			u16 cmd;
>  
>  			pdev = vgadev->pdev;
>  			pci_read_config_word(pdev, PCI_COMMAND, &cmd);
>  			if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
> -				vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n");
> -				vga_set_default_device(pdev);
> -				break;
> +				found = pdev;
> +				if (vga_arb_integrated_gpu(dev))
> +					break;
>  			}
>  		}
>  	}
>  
> +	if (found) {
> +		vgaarb_info(&found->dev, "setting as boot device (VGA legacy resources not available)\n");
> +		vga_set_default_device(found);
> +		return;
> +	}
> +
>  	if (!vga_default_device()) {
>  		vgadev = list_first_entry_or_null(&vga_list,
>  						  struct vga_device, list);
> -- 
> 2.31.1
>