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

Hi Bjorn, Daniel

[...]

> 
> Is this quirk useful on any arch other than arm64?  Per
> drivers/pci/dwc/Kconfig, CONFIG_PCI_HISI depends on CONFIG_ARM64.
> 
> Would it make sense to put this quirk in arch/arm64/kernel/pci.c?

Indeed our host controller depends on ARM64 so maybe it would make
sense to move the quirk arch/arm64/kernel/pci.c; however regardless
why is it strictly required for a VGA device to be legacy one in order
to make it the default boot device?
i.e. couldn't we have:


?
Thanks
Gab

[+cc Ben, David, Daniel, Alex]

On Thu, Jul 13, 2017 at 10:29:25AM +0000, Gabriele Paoloni wrote:
> Hi Bjorn, Daniel
> 
> [...]
> 
> > 
> > Is this quirk useful on any arch other than arm64?  Per
> > drivers/pci/dwc/Kconfig, CONFIG_PCI_HISI depends on CONFIG_ARM64.
> > 
> > Would it make sense to put this quirk in arch/arm64/kernel/pci.c?
> 
> Indeed our host controller depends on ARM64 so maybe it would make
> sense to move the quirk arch/arm64/kernel/pci.c; however regardless
> why is it strictly required for a VGA device to be legacy one in order
> to make it the default boot device?
> i.e. couldn't we have:
> 
> diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> index 0f5b2dd..a6b606c 100644
> --- a/drivers/gpu/vga/vgaarb.c
> +++ b/drivers/gpu/vga/vgaarb.c
> @@ -667,8 +667,7 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
>  	/* Deal with VGA default device. Use first enabled one
>  	 * by default if arch doesn't have it's own hook
>  	 */
> -	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);
>  	}

I don't know enough about the VGA arbiter to answer this.  This test was
part of the initial implementation: deb2d2ecd43d ("PCI/GPU: implement VGA
arbitration on Linux") by Ben.

Bjorn

On Thu, 2017-07-13 at 06:29 -0500, Bjorn Helgaas wrote:
> > Indeed our host controller depends on ARM64 so maybe it would make
> > sense to move the quirk arch/arm64/kernel/pci.c; however regardless
> > why is it strictly required for a VGA device to be legacy one in order
> > to make it the default boot device?
> > i.e. couldn't we have:
> > 
> > diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> > index 0f5b2dd..a6b606c 100644
> > --- a/drivers/gpu/vga/vgaarb.c
> > +++ b/drivers/gpu/vga/vgaarb.c
> > @@ -667,8 +667,7 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
> >        /* Deal with VGA default device. Use first enabled one
> >         * by default if arch doesn't have it's own hook
> >         */
> > -     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);
> >        }
> 
> I don't know enough about the VGA arbiter to answer this.  This test was
> part of the initial implementation: deb2d2ecd43d ("PCI/GPU: implement VGA
> arbitration on Linux") by Ben.

The above simply uses the first device that has memory and IO enabled
as the default device (you don't need to have a default device).

This is essentially picking up whatever device had been initialized
by the BIOS/firmware as default. This is needed for example on x86
where the BIOS tends to only initialize one device. 

I'm not sure what problem you are trying to solve here ?

Cheers,
Ben.

On Thu, 13 Jul 2017 06:29:38 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:

> [+cc Ben, David, Daniel, Alex]
> 
> On Thu, Jul 13, 2017 at 10:29:25AM +0000, Gabriele Paoloni wrote:
> > Hi Bjorn, Daniel
> > 
> > [...]
> >   
> > > 
> > > Is this quirk useful on any arch other than arm64?  Per
> > > drivers/pci/dwc/Kconfig, CONFIG_PCI_HISI depends on CONFIG_ARM64.
> > > 
> > > Would it make sense to put this quirk in arch/arm64/kernel/pci.c?  
> > 
> > Indeed our host controller depends on ARM64 so maybe it would make
> > sense to move the quirk arch/arm64/kernel/pci.c; however regardless
> > why is it strictly required for a VGA device to be legacy one in order
> > to make it the default boot device?
> > i.e. couldn't we have:
> > 
> > diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> > index 0f5b2dd..a6b606c 100644
> > --- a/drivers/gpu/vga/vgaarb.c
> > +++ b/drivers/gpu/vga/vgaarb.c
> > @@ -667,8 +667,7 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
> >  	/* Deal with VGA default device. Use first enabled one
> >  	 * by default if arch doesn't have it's own hook
> >  	 */
> > -	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,

Alex

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

Cheers,
Ben.

Hi Ben

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

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

> Sent: 13 July 2017 21:45

> To: Bjorn Helgaas; Gabriele Paoloni

> Cc: 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; Alex

> Williamson

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

> misbehaving HiSilicon bridge

> 

> On Thu, 2017-07-13 at 06:29 -0500, Bjorn Helgaas wrote:

> > > Indeed our host controller depends on ARM64 so maybe it would make

> > > sense to move the quirk arch/arm64/kernel/pci.c; however regardless

> > > why is it strictly required for a VGA device to be legacy one in

> order

> > > to make it the default boot device?

> > > i.e. couldn't we have:

> > >

> > > diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c

> > > index 0f5b2dd..a6b606c 100644

> > > --- a/drivers/gpu/vga/vgaarb.c

> > > +++ b/drivers/gpu/vga/vgaarb.c

> > > @@ -667,8 +667,7 @@ static bool vga_arbiter_add_pci_device(struct

> pci_dev *pdev)

> > >        /* Deal with VGA default device. Use first enabled one

> > >         * by default if arch doesn't have it's own hook

> > >         */

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

> > >        }

> >

> > I don't know enough about the VGA arbiter to answer this.  This test

> was

> > part of the initial implementation: deb2d2ecd43d ("PCI/GPU: implement

> VGA

> > arbitration on Linux") by Ben.

> 

> The above simply uses the first device that has memory and IO enabled

> as the default device (you don't need to have a default device).

> 

> This is essentially picking up whatever device had been initialized

> by the BIOS/firmware as default. This is needed for example on x86

> where the BIOS tends to only initialize one device.

> 

> I'm not sure what problem you are trying to solve here ?


Well our host platform does not support legacy devices and therefore we find
ourselves without a default VGA device...

I was trying to understand why a default device has to be legacy...but I think
this was answered by both you and Alex in other follows-up...

Thanks
Gab 

> 

> Cheers,

> Ben.