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

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

[Bjorn Helgaas]

[+cc Catalin, Will, linux-arm-kernel]

On Wed, Jul 12, 2017 at 03:08:11PM +1000, Daniel Axtens wrote:
> The HiSilicon D05 board has some PCI bridges (PCI ID 19e5:1610) that
> are not spec-compliant: the VGA Enable bit is hardwired to 0 and
> writes do not change it.
> 
> The HiSilicon engineers report that the bridge does not forward the
> 0xa0000-0xbffff mem range and the 0x3b0-0x3bb and 0x3c0-0x3df I/O
> ranges.
> 
> Because the VGA Enable bit is hardwired to 0, the VGA arbiter refuses
> to mark any card behind it as the boot device. This breaks Xorg
> auto-detection.
> 
> However, the hibmc VGA card (PCI ID 19e5:1711) has been tested and is
> known to work when behind these bridges. (It does not require the
> legacy resources to operate.)
> 
> Provide a quirk so that this combination of bridge and card is eligible
> to be the default VGA card. This fixes Xorg auto-detection on the D05.
> 
> Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
> Cc: Rongrong Zou <zourongrong@gmail.com>
> Signed-off-by: Daniel Axtens <dja@axtens.net>
> ---
> 
> v3: fix commit message
> v4: fix comment (forgot to git add/git commit, sorry for the noise)
> 
> ---
>  drivers/pci/quirks.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 16e6cd86ad71..b42324cba29e 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -25,6 +25,7 @@
>  #include <linux/sched.h>
>  #include <linux/ktime.h>
>  #include <linux/mm.h>
> +#include <linux/vgaarb.h>
>  #include <asm/dma.h>	/* isa_dma_bridge_buggy */
>  #include "pci.h"
>  
> @@ -4664,3 +4665,52 @@ static void quirk_intel_no_flr(struct pci_dev *dev)
>  }
>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1502, quirk_intel_no_flr);
>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1503, quirk_intel_no_flr);
> +
> +/*
> + * The HiSilicon D05 board has some PCI bridges (PCI ID 19e5:1610)
> + * that are not spec-compliant: the VGA Enable bit is hardwired to 0
> + * and writes do not change it. The bridge does not forward legacy
> + * memory or I/O resources.
> + *
> + * Because the VGA Enable bit is hardwired to 0, the VGA arbiter
> + * refuses to mark any card behind it as the boot device. However, the
> + * hibmc VGA card (PCI ID 19e5:1711) has been tested and is known to
> + * work when behind these bridges.
> + *
> + * If we have this bridge, this card, and no default card already,
> + * mark the card as default.
> + */
> +static void hibmc_fixup_vgaarb(struct pci_dev *pdev)
> +{
> +	struct pci_dev *bridge;
> +	struct pci_bus *bus;
> +	u16 config;
> +
> +	bus = pdev->bus;
> +	bridge = bus->self;
> +	if (!bridge)
> +		return;
> +
> +	if (!pci_is_bridge(bridge))
> +		return;
> +
> +	if (bridge->vendor != PCI_VENDOR_ID_HUAWEI ||
> +	    bridge->device != 0x1610)
> +		return;
> +
> +	pci_read_config_word(bridge, PCI_BRIDGE_CONTROL,
> +			     &config);
> +	if (config & PCI_BRIDGE_CTL_VGA) {
> +		/*
> +		 * Weirdly, this bridge *is* spec compliant, so bail
> +		 * and let vgaarb do its job
> +		 */
> +		return;
> +	}
> +
> +	if (vga_default_device())
> +		return;
> +
> +	vga_set_default_device(pdev);
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0x1711, hibmc_fixup_vgaarb);

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?

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

[Gabriele Paoloni]

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

?
Thanks
Gab

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

[Bjorn Helgaas]

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

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

[Benjamin Herrenschmidt]

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.

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

[Alex Williamson]

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

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

[Benjamin Herrenschmidt]

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.

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

[Will Deacon]

Hi Bjorn,

On Wed, Jul 12, 2017 at 03:04:30PM -0500, Bjorn Helgaas wrote:
> [+cc Catalin, Will, linux-arm-kernel]
> 
> On Wed, Jul 12, 2017 at 03:08:11PM +1000, Daniel Axtens wrote:
> > The HiSilicon D05 board has some PCI bridges (PCI ID 19e5:1610) that
> > are not spec-compliant: the VGA Enable bit is hardwired to 0 and
> > writes do not change it.
> > 
> > The HiSilicon engineers report that the bridge does not forward the
> > 0xa0000-0xbffff mem range and the 0x3b0-0x3bb and 0x3c0-0x3df I/O
> > ranges.
> > 
> > Because the VGA Enable bit is hardwired to 0, the VGA arbiter refuses
> > to mark any card behind it as the boot device. This breaks Xorg
> > auto-detection.
> > 
> > However, the hibmc VGA card (PCI ID 19e5:1711) has been tested and is
> > known to work when behind these bridges. (It does not require the
> > legacy resources to operate.)
> > 
> > Provide a quirk so that this combination of bridge and card is eligible
> > to be the default VGA card. This fixes Xorg auto-detection on the D05.
> > 
> > Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
> > Cc: Rongrong Zou <zourongrong@gmail.com>
> > Signed-off-by: Daniel Axtens <dja@axtens.net>
> > ---
> > 
> > v3: fix commit message
> > v4: fix comment (forgot to git add/git commit, sorry for the noise)
> > 
> > ---
> >  drivers/pci/quirks.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 50 insertions(+)
> > 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index 16e6cd86ad71..b42324cba29e 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -25,6 +25,7 @@
> >  #include <linux/sched.h>
> >  #include <linux/ktime.h>
> >  #include <linux/mm.h>
> > +#include <linux/vgaarb.h>
> >  #include <asm/dma.h>	/* isa_dma_bridge_buggy */
> >  #include "pci.h"
> >  
> > @@ -4664,3 +4665,52 @@ static void quirk_intel_no_flr(struct pci_dev *dev)
> >  }
> >  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1502, quirk_intel_no_flr);
> >  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1503, quirk_intel_no_flr);
> > +
> > +/*
> > + * The HiSilicon D05 board has some PCI bridges (PCI ID 19e5:1610)
> > + * that are not spec-compliant: the VGA Enable bit is hardwired to 0
> > + * and writes do not change it. The bridge does not forward legacy
> > + * memory or I/O resources.
> > + *
> > + * Because the VGA Enable bit is hardwired to 0, the VGA arbiter
> > + * refuses to mark any card behind it as the boot device. However, the
> > + * hibmc VGA card (PCI ID 19e5:1711) has been tested and is known to
> > + * work when behind these bridges.
> > + *
> > + * If we have this bridge, this card, and no default card already,
> > + * mark the card as default.
> > + */
> > +static void hibmc_fixup_vgaarb(struct pci_dev *pdev)
> > +{
> > +	struct pci_dev *bridge;
> > +	struct pci_bus *bus;
> > +	u16 config;
> > +
> > +	bus = pdev->bus;
> > +	bridge = bus->self;
> > +	if (!bridge)
> > +		return;
> > +
> > +	if (!pci_is_bridge(bridge))
> > +		return;
> > +
> > +	if (bridge->vendor != PCI_VENDOR_ID_HUAWEI ||
> > +	    bridge->device != 0x1610)
> > +		return;
> > +
> > +	pci_read_config_word(bridge, PCI_BRIDGE_CONTROL,
> > +			     &config);
> > +	if (config & PCI_BRIDGE_CTL_VGA) {
> > +		/*
> > +		 * Weirdly, this bridge *is* spec compliant, so bail
> > +		 * and let vgaarb do its job
> > +		 */
> > +		return;
> > +	}
> > +
> > +	if (vga_default_device())
> > +		return;
> > +
> > +	vga_set_default_device(pdev);
> > +}
> > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0x1711, hibmc_fixup_vgaarb);
> 
> 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?

We've resisted adding PCI quirks in the arch directory so far, so I'd rather
it lived in the PCI code if you don't have a strong objection to that.
Hardware vendors have a tendency to reuse broken IP, so it's not beyond the
realms of imagination that this quirk might be needed for another part some
day (e.g. based on 32-bit ARM).

Will

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

[Gabriele Paoloni]

Hi Ben

> -----Original Message-----
> From: Benjamin Herrenschmidt [mailto:benh at kernel.crashing.org]
> Sent: 13 July 2017 21:45
> To: Bjorn Helgaas; Gabriele Paoloni
> Cc: Daniel Axtens; linux-pci at vger.kernel.org; Liuxinliang (Matthew
> Liu); Rongrong Zou; Catalin Marinas; Will Deacon; linux-arm-
> kernel at 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.

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

[Gabriele Paoloni]

Hi Ben, Alex

> -----Original Message-----
> From: Benjamin Herrenschmidt [mailto:benh at kernel.crashing.org]
> Sent: 13 July 2017 22:21
> To: Alex Williamson; Bjorn Helgaas
> Cc: Gabriele Paoloni; Daniel Axtens; linux-pci at vger.kernel.org;
> Liuxinliang (Matthew Liu); Rongrong Zou; Catalin Marinas; Will Deacon;
> linux-arm-kernel at 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?

Thanks
Gab

> 
> Cheers,
> Ben.

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

[Benjamin Herrenschmidt]

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.

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

[Alex Williamson]

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 at kernel.crashing.org]
> > Sent: 13 July 2017 22:21
> > To: Alex Williamson; Bjorn Helgaas
> > Cc: Gabriele Paoloni; Daniel Axtens; linux-pci at vger.kernel.org;
> > Liuxinliang (Matthew Liu); Rongrong Zou; Catalin Marinas; Will Deacon;
> > linux-arm-kernel at 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 v4] PCI: Support hibmc VGA cards behind a misbehaving HiSilicon bridge

[Gabriele Paoloni]

Hi Alex, Ben

> -----Original Message-----
> From: Benjamin Herrenschmidt [mailto:benh at kernel.crashing.org]
> Sent: 14 July 2017 14:50
> To: Gabriele Paoloni; Alex Williamson; Bjorn Helgaas
> Cc: Daniel Axtens; linux-pci at vger.kernel.org; Liuxinliang (Matthew
> Liu); Rongrong Zou; Catalin Marinas; Will Deacon; linux-arm-
> kernel at lists.infradead.org; David Airlie; Daniel Vetter
> Subject: Re: [PATCH v4] PCI: Support hibmc VGA cards behind a
> misbehaving HiSilicon bridge
> 
> 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 think the following one should make sure that MEM/IO response are
enabled (also as suggested by Alex)

diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index 92f1452..a90c48c 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -1424,6 +1424,21 @@ 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,
+		 * justpick up the first one in the list capable of responding to
+		 * mem and io requests*/
+		if (vga_default == NULL) {
+			u16 cmd;
+
+			pci_read_config_word(vgadev->pdev, PCI_COMMAND, &cmd);
+			if ((cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) ||
+					!vga_default_device()) {
+				vga_set_default_device(vgadev->pdev);
+				vgaarb_info(dev, "setting as boot VGA device\n");
+			}
+		}
+
 #if defined(CONFIG_X86) || defined(CONFIG_IA64)
 		/*
 		 * Override vga_arbiter_add_pci_device()'s I/O based detection


Also the above one could allow us to get rid of fixup_vga in powerpc....


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

Well from my understanding the PCI host controller driver will enumerate
all the devices in the PCI hierarchy and call pci_device_add() for each of
them, that in turn will call device_add(), at this stage if there is a
driver available for the device such driver will probe otherwise it will not.

Are you suggesting to add the code above in pci_device_add() after device_add()
and after checking that a driver has been bound for such dev?

Thanks
Gab


> 
> Ben.

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

[Benjamin Herrenschmidt]

On Fri, 2017-07-14 at 17:03 +0000, Gabriele Paoloni wrote:
> > 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.
> 
> Well from my understanding the PCI host controller driver will enumerate
> all the devices in the PCI hierarchy and call pci_device_add() for each of
> them, that in turn will call device_add(), at this stage if there is a
> driver available for the device such driver will probe otherwise it will not.
> 
> Are you suggesting to add the code above in pci_device_add() after device_add()
> and after checking that a driver has been bound for such dev?

I don't like us turning on MEM/IO decoding on a device that has
potentially not been initialized by its driver.

Ben.