[v4] PCI: Support hibmc VGA cards behind a misbehaving HiSilicon bridge

Message ID EE11001F9E5DDD47B7634E2F8A612F2E40B399B4@FRAEML521-MBX.china.huawei.com
State Superseded
Headers show

Commit Message

Gabriele Paoloni July 14, 2017, 12:26 p.m.
Hi Ben, Alex

> -----Original Message-----

> From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org]

> Sent: 13 July 2017 22:21

> To: Alex Williamson; Bjorn Helgaas

> Cc: Gabriele Paoloni; Daniel Axtens; linux-pci@vger.kernel.org;

> Liuxinliang (Matthew Liu); Rongrong Zou; Catalin Marinas; Will Deacon;

> linux-arm-kernel@lists.infradead.org; David Airlie; Daniel Vetter

> Subject: Re: [PATCH v4] PCI: Support hibmc VGA cards behind a

> misbehaving HiSilicon bridge

> 

> On Thu, 2017-07-13 at 15:11 -0600, Alex Williamson wrote:

> > > >       */

> > > > -   if (vga_default == NULL &&

> > > > -       ((vgadev->owns & VGA_RSRC_LEGACY_MASK) ==

> VGA_RSRC_LEGACY_MASK)) {

> > > > +   if (vga_default == NULL) {

> > > >              vgaarb_info(&pdev->dev, "setting as boot VGA

> device\n");

> > > >              vga_set_default_device(pdev);

> > > >      }

> >

> >

> > "Legacy" is the breadcrumb we use to try to pick the same device for

> > default VGA as the system firmware used as the boot VGA.  There can

> be

> > multiple VGA devices in the system, the change above would simply

> make

> > the first discovered device be the default VGA.  That would break

> many,

> > many systems.  If legacy VGA ranges mean nothing on ARM64, then

> follow

> > the powerpc lead and make an arch quirk that simply selects the first

> > enabled VGA device as the default.  VGA routing is part of the PCI

> spec

> > though, so the default of selecting the device with routing enabled

> > makes sense.  Thanks,

> 

> "Legacy" there iirc also means that it has decoding enabled at boot. If

> you pick the first one you may pick a device that doesn't and hasn't

> been initialized by any driver (good old BIOS-initialized text mode).


Right so effectively if there is a legacy dev we should pick that up;
however what about the following code:


Above after we have filled the list of VGA devices by iterating over all
PCI devices we check if no legacy one has been set as default VGA device
yet: therefore we set the first VGA device in the list as default one...

Do you think it would work?

Thanks
Gab

> 

> Cheers,

> Ben.

Comments

Benjamin Herrenschmidt July 14, 2017, 1:50 p.m. | #1
On Fri, 2017-07-14 at 12:26 +0000, Gabriele Paoloni wrote:
> diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> index 92f1452..ab3ad9a 100644
> --- a/drivers/gpu/vga/vgaarb.c
> +++ b/drivers/gpu/vga/vgaarb.c
> @@ -1424,6 +1424,14 @@ static int __init vga_arb_device_init(void)
>  
>         list_for_each_entry(vgadev, &vga_list, list) {
>                 struct device *dev = &vgadev->pdev->dev;
> +
> +               /* if no legacy device has been set as default VGA
> +                * device, just pick up the first one in the list */
> +               if (vga_default == NULL) {
> +                       vgaarb_info(dev, "setting as boot VGA device\n");
> +                       vga_set_default_device(vgadev->pdev);
> +               }
> +
>  #if defined(CONFIG_X86) || defined(CONFIG_IA64)
>                 /*
>                  * Override vga_arbiter_add_pci_device()'s I/O based detection 
> 
> Above after we have filled the list of VGA devices by iterating over all
> PCI devices we check if no legacy one has been set as default VGA device
> yet: therefore we set the first VGA device in the list as default one...
> 
> Do you think it would work?

I honestly don't remember all of the details of the arbiter but just
make sure that it won't think that device is enabled for things like
VGA etc... if it's memory/IO decode weren't enabled.

I'd rather we have no default device until a driver actually picks up
though, and then, if we still have no default, use the first driver to
pick up.

Ben.
Alex Williamson July 14, 2017, 2:43 p.m. | #2
On Fri, 14 Jul 2017 12:26:05 +0000
Gabriele Paoloni <gabriele.paoloni@huawei.com> wrote:

> Hi Ben, Alex
> 
> > -----Original Message-----
> > From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org]
> > Sent: 13 July 2017 22:21
> > To: Alex Williamson; Bjorn Helgaas
> > Cc: Gabriele Paoloni; Daniel Axtens; linux-pci@vger.kernel.org;
> > Liuxinliang (Matthew Liu); Rongrong Zou; Catalin Marinas; Will Deacon;
> > linux-arm-kernel@lists.infradead.org; David Airlie; Daniel Vetter
> > Subject: Re: [PATCH v4] PCI: Support hibmc VGA cards behind a
> > misbehaving HiSilicon bridge
> > 
> > On Thu, 2017-07-13 at 15:11 -0600, Alex Williamson wrote:  
> > > > >       */
> > > > > -   if (vga_default == NULL &&
> > > > > -       ((vgadev->owns & VGA_RSRC_LEGACY_MASK) ==  
> > VGA_RSRC_LEGACY_MASK)) {  
> > > > > +   if (vga_default == NULL) {
> > > > >              vgaarb_info(&pdev->dev, "setting as boot VGA  
> > device\n");  
> > > > >              vga_set_default_device(pdev);
> > > > >      }  
> > >
> > >
> > > "Legacy" is the breadcrumb we use to try to pick the same device for
> > > default VGA as the system firmware used as the boot VGA.  There can  
> > be  
> > > multiple VGA devices in the system, the change above would simply  
> > make  
> > > the first discovered device be the default VGA.  That would break  
> > many,  
> > > many systems.  If legacy VGA ranges mean nothing on ARM64, then  
> > follow  
> > > the powerpc lead and make an arch quirk that simply selects the first
> > > enabled VGA device as the default.  VGA routing is part of the PCI  
> > spec  
> > > though, so the default of selecting the device with routing enabled
> > > makes sense.  Thanks,  
> > 
> > "Legacy" there iirc also means that it has decoding enabled at boot. If
> > you pick the first one you may pick a device that doesn't and hasn't
> > been initialized by any driver (good old BIOS-initialized text mode).  
> 
> Right so effectively if there is a legacy dev we should pick that up;
> however what about the following code:
> 
> diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> index 92f1452..ab3ad9a 100644
> --- a/drivers/gpu/vga/vgaarb.c
> +++ b/drivers/gpu/vga/vgaarb.c
> @@ -1424,6 +1424,14 @@ static int __init vga_arb_device_init(void)
>  
>  	list_for_each_entry(vgadev, &vga_list, list) {
>  		struct device *dev = &vgadev->pdev->dev;
> +
> +		/* if no legacy device has been set as default VGA
> +		 * device, just pick up the first one in the list */
> +		if (vga_default == NULL) {
> +			vgaarb_info(dev, "setting as boot VGA device\n");
> +			vga_set_default_device(vgadev->pdev);
> +		}
> +
>  #if defined(CONFIG_X86) || defined(CONFIG_IA64)
>  		/*
>  		 * Override vga_arbiter_add_pci_device()'s I/O based detection 
> 
> Above after we have filled the list of VGA devices by iterating over all
> PCI devices we check if no legacy one has been set as default VGA device
> yet: therefore we set the first VGA device in the list as default one...
> 
> Do you think it would work?

I'd suggest at least looking for an enabled device as fixup_vga() does,
perhaps that would even be enough to remove that arch quirk.  Thanks,

Alex

Patch

diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index 92f1452..ab3ad9a 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -1424,6 +1424,14 @@  static int __init vga_arb_device_init(void)
 
 	list_for_each_entry(vgadev, &vga_list, list) {
 		struct device *dev = &vgadev->pdev->dev;
+
+		/* if no legacy device has been set as default VGA
+		 * device, just pick up the first one in the list */
+		if (vga_default == NULL) {
+			vgaarb_info(dev, "setting as boot VGA device\n");
+			vga_set_default_device(vgadev->pdev);
+		}
+
 #if defined(CONFIG_X86) || defined(CONFIG_IA64)
 		/*
 		 * Override vga_arbiter_add_pci_device()'s I/O based detection