LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] vgaarb: Use ACPI HID name to find integrated GPU
@ 2021-05-19 13:57 Kai-Heng Feng
  2021-05-19 16:45 ` Alex Deucher
  2021-09-16 16:37 ` Bjorn Helgaas
  0 siblings, 2 replies; 7+ messages in thread
From: Kai-Heng Feng @ 2021-05-19 13:57 UTC (permalink / raw)
  To: airlied, daniel, maarten.lankhorst, mripard, tzimmermann
  Cc: alexander.deucher, Kai-Heng Feng, open list:DRM DRIVERS, open list

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) {
 			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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] vgaarb: Use ACPI HID name to find integrated GPU
  2021-05-19 13:57 [PATCH] vgaarb: Use ACPI HID name to find integrated GPU Kai-Heng Feng
@ 2021-05-19 16:45 ` Alex Deucher
  2021-05-20 18:58   ` Alex Deucher
  2021-09-16 16:37 ` Bjorn Helgaas
  1 sibling, 1 reply; 7+ messages in thread
From: Alex Deucher @ 2021-05-19 16:45 UTC (permalink / raw)
  To: Kai-Heng Feng
  Cc: Dave Airlie, Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, Deucher, Alexander, open list,
	open list:DRM DRIVERS

On Wed, May 19, 2021 at 9:57 AM Kai-Heng Feng
<kai.heng.feng@canonical.com> 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>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

Unless there are any other comments, I'll apply it tomorrow.

Alex

> ---
>  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) {
>                         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
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] vgaarb: Use ACPI HID name to find integrated GPU
  2021-05-19 16:45 ` Alex Deucher
@ 2021-05-20 18:58   ` Alex Deucher
  0 siblings, 0 replies; 7+ messages in thread
From: Alex Deucher @ 2021-05-20 18:58 UTC (permalink / raw)
  To: Kai-Heng Feng
  Cc: Dave Airlie, Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, Deucher, Alexander, open list,
	open list:DRM DRIVERS

Pushed to drm-misc-next.  Thanks!

Alex

On Wed, May 19, 2021 at 12:45 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> On Wed, May 19, 2021 at 9:57 AM Kai-Heng Feng
> <kai.heng.feng@canonical.com> 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>
>
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>
> Unless there are any other comments, I'll apply it tomorrow.
>
> Alex
>
> > ---
> >  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) {
> >                         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
> >

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] vgaarb: Use ACPI HID name to find integrated GPU
  2021-05-19 13:57 [PATCH] vgaarb: Use ACPI HID name to find integrated GPU Kai-Heng Feng
  2021-05-19 16:45 ` Alex Deucher
@ 2021-09-16 16:37 ` Bjorn Helgaas
  2021-09-17  3:49   ` Kai-Heng Feng
  1 sibling, 1 reply; 7+ messages in thread
From: Bjorn Helgaas @ 2021-09-16 16:37 UTC (permalink / raw)
  To: Kai-Heng Feng
  Cc: airlied, daniel, maarten.lankhorst, mripard, tzimmermann,
	alexander.deucher, dri-devel, linux-kernel, linux-pci,
	Huacai Chen

[+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
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] vgaarb: Use ACPI HID name to find integrated GPU
  2021-09-16 16:37 ` Bjorn Helgaas
@ 2021-09-17  3:49   ` Kai-Heng Feng
  2021-09-17 16:55     ` Bjorn Helgaas
  0 siblings, 1 reply; 7+ messages in thread
From: Kai-Heng Feng @ 2021-09-17  3:49 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: David Airlie, Daniel Vetter, Maarten Lankhorst, mripard,
	Thomas Zimmermann, Deucher, Alexander, open list:DRM DRIVERS,
	LKML, Linux PCI, Huacai Chen

On Fri, Sep 17, 2021 at 12:38 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+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?

The descending order is to keep the original behavior.

Before this patch, it breaks out of the loop as early as possible, so
the lower numbered device is picked.
This patch makes it only break out of the loop when ACPI_VIDEO_HID
device is found.
So if there are more than one device that meet "cmd & (PCI_COMMAND_IO
| PCI_COMMAND_MEMORY)", higher numbered device will be selected.
So the traverse order reversal is to keep the original behavior.

>
> 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.

There's only one integrated GPU on the affected system.

The approach is to keep the list traversal in one pass.
Is there any regression introduce by this patch?
If that's the case, we can separate the logic and find the
ACPI_VIDEO_HID in second pass.

Kai-Heng

>
> >                       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
> >

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] vgaarb: Use ACPI HID name to find integrated GPU
  2021-09-17  3:49   ` Kai-Heng Feng
@ 2021-09-17 16:55     ` Bjorn Helgaas
  2021-09-23  3:20       ` Kai-Heng Feng
  0 siblings, 1 reply; 7+ messages in thread
From: Bjorn Helgaas @ 2021-09-17 16:55 UTC (permalink / raw)
  To: Kai-Heng Feng
  Cc: David Airlie, Daniel Vetter, Maarten Lankhorst, mripard,
	Thomas Zimmermann, Deucher, Alexander, open list:DRM DRIVERS,
	LKML, Linux PCI, Huacai Chen

On Fri, Sep 17, 2021 at 11:49:45AM +0800, Kai-Heng Feng wrote:
> On Fri, Sep 17, 2021 at 12:38 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > [+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?
> 
> The descending order is to keep the original behavior.
> 
> Before this patch, it breaks out of the loop as early as possible, so
> the lower numbered device is picked.
> This patch makes it only break out of the loop when ACPI_VIDEO_HID
> device is found.
> So if there are more than one device that meet "cmd & (PCI_COMMAND_IO
> | PCI_COMMAND_MEMORY)", higher numbered device will be selected.
> So the traverse order reversal is to keep the original behavior.

Can you give an example of what you mean?  I don't quite follow how it
keeps the original behavior.

If we have this:

  0  PCI_COMMAND_MEMORY set   ACPI_VIDEO_HID
  1  PCI_COMMAND_MEMORY set   ACPI_VIDEO_HID

Previously we didn't look for ACPI_VIDEO_HID, so we chose 0, now we
choose 1, which seems wrong.  In the absence of other information, I
would prefer the lower-numbered device.

Or this:

  0  PCI_COMMAND_MEMORY set
  1  PCI_COMMAND_MEMORY set   ACPI_VIDEO_HID

Previously we chose 0; now we choose 1, which does seem right, but
we'd choose 1 regardless of the order.

Or this:

  0  PCI_COMMAND_MEMORY set   ACPI_VIDEO_HID
  1  PCI_COMMAND_MEMORY set

Previously we chose 0, now we still choose 0, which seems right but
again doesn't depend on the order.

The first case, where both devices are ACPI_VIDEO_HID, is the only one
where the order matters, and I suggest that we should be using the
original order, not the reversed order.

> > 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.
> 
> There's only one integrated GPU on the affected system.
> 
> The approach is to keep the list traversal in one pass.
> Is there any regression introduce by this patch?
> If that's the case, we can separate the logic and find the
> ACPI_VIDEO_HID in second pass.

No regression, I'm just looking at Huacai's VGA patches, which affect
this area.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] vgaarb: Use ACPI HID name to find integrated GPU
  2021-09-17 16:55     ` Bjorn Helgaas
@ 2021-09-23  3:20       ` Kai-Heng Feng
  0 siblings, 0 replies; 7+ messages in thread
From: Kai-Heng Feng @ 2021-09-23  3:20 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: David Airlie, Daniel Vetter, Maarten Lankhorst, mripard,
	Thomas Zimmermann, Deucher, Alexander, open list:DRM DRIVERS,
	LKML, Linux PCI, Huacai Chen

On Sat, Sep 18, 2021 at 12:55 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Sep 17, 2021 at 11:49:45AM +0800, Kai-Heng Feng wrote:
> > On Fri, Sep 17, 2021 at 12:38 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > >
> > > [+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?
> >
> > The descending order is to keep the original behavior.
> >
> > Before this patch, it breaks out of the loop as early as possible, so
> > the lower numbered device is picked.
> > This patch makes it only break out of the loop when ACPI_VIDEO_HID
> > device is found.
> > So if there are more than one device that meet "cmd & (PCI_COMMAND_IO
> > | PCI_COMMAND_MEMORY)", higher numbered device will be selected.
> > So the traverse order reversal is to keep the original behavior.
>
> Can you give an example of what you mean?  I don't quite follow how it
> keeps the original behavior.
>
> If we have this:
>
>   0  PCI_COMMAND_MEMORY set   ACPI_VIDEO_HID
>   1  PCI_COMMAND_MEMORY set   ACPI_VIDEO_HID
>
> Previously we didn't look for ACPI_VIDEO_HID, so we chose 0, now we
> choose 1, which seems wrong.  In the absence of other information, I
> would prefer the lower-numbered device.
>
> Or this:
>
>   0  PCI_COMMAND_MEMORY set
>   1  PCI_COMMAND_MEMORY set   ACPI_VIDEO_HID
>
> Previously we chose 0; now we choose 1, which does seem right, but
> we'd choose 1 regardless of the order.
>
> Or this:
>
>   0  PCI_COMMAND_MEMORY set   ACPI_VIDEO_HID
>   1  PCI_COMMAND_MEMORY set
>
> Previously we chose 0, now we still choose 0, which seems right but
> again doesn't depend on the order.
>
> The first case, where both devices are ACPI_VIDEO_HID, is the only one
> where the order matters, and I suggest that we should be using the
> original order, not the reversed order.

Consider this:
0  PCI_COMMAND_MEMORY set
1  PCI_COMMAND_MEMORY set

Originally device 0 will be picked. If the traverse order is kept,
device 1 will be selected instead, because none of them pass
vga_arb_integrated_gpu().

Kai-Heng

>
> > > 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.
> >
> > There's only one integrated GPU on the affected system.
> >
> > The approach is to keep the list traversal in one pass.
> > Is there any regression introduce by this patch?
> > If that's the case, we can separate the logic and find the
> > ACPI_VIDEO_HID in second pass.
>
> No regression, I'm just looking at Huacai's VGA patches, which affect
> this area.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-09-23  3:20 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-19 13:57 [PATCH] vgaarb: Use ACPI HID name to find integrated GPU Kai-Heng Feng
2021-05-19 16:45 ` Alex Deucher
2021-05-20 18:58   ` Alex Deucher
2021-09-16 16:37 ` Bjorn Helgaas
2021-09-17  3:49   ` Kai-Heng Feng
2021-09-17 16:55     ` Bjorn Helgaas
2021-09-23  3:20       ` Kai-Heng Feng

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).