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

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

[Daniel Axtens]

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

Re: [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?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

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

[Gabriele Paoloni]

Hi Bjorn, Daniel

[...]

>=20
> Is this quirk useful on any arch other than arm64?  Per
> drivers/pci/dwc/Kconfig, CONFIG_PCI_HISI depends on CONFIG_ARM64.
>=20
> 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 =3D=3D NULL &&
-	    ((vgadev->owns & VGA_RSRC_LEGACY_MASK) =3D=3D VGA_RSRC_LEGACY_MASK)) =
{
+	if (vga_default =3D=3D 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

[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

Re: [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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[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

Re: [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

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

Re: [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

[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

Re: [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

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

Re: [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

[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

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

[Gabriele Paoloni]

SGkgQmVuDQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogQmVuamFtaW4g
SGVycmVuc2NobWlkdCBbbWFpbHRvOmJlbmhAa2VybmVsLmNyYXNoaW5nLm9yZ10NCj4gU2VudDog
MTMgSnVseSAyMDE3IDIxOjQ1DQo+IFRvOiBCam9ybiBIZWxnYWFzOyBHYWJyaWVsZSBQYW9sb25p
DQo+IENjOiBEYW5pZWwgQXh0ZW5zOyBsaW51eC1wY2lAdmdlci5rZXJuZWwub3JnOyBMaXV4aW5s
aWFuZyAoTWF0dGhldw0KPiBMaXUpOyBSb25ncm9uZyBab3U7IENhdGFsaW4gTWFyaW5hczsgV2ls
bCBEZWFjb247IGxpbnV4LWFybS0NCj4ga2VybmVsQGxpc3RzLmluZnJhZGVhZC5vcmc7IERhdmlk
IEFpcmxpZTsgRGFuaWVsIFZldHRlcjsgQWxleA0KPiBXaWxsaWFtc29uDQo+IFN1YmplY3Q6IFJl
OiBbUEFUQ0ggdjRdIFBDSTogU3VwcG9ydCBoaWJtYyBWR0EgY2FyZHMgYmVoaW5kIGENCj4gbWlz
YmVoYXZpbmcgSGlTaWxpY29uIGJyaWRnZQ0KPiANCj4gT24gVGh1LCAyMDE3LTA3LTEzIGF0IDA2
OjI5IC0wNTAwLCBCam9ybiBIZWxnYWFzIHdyb3RlOg0KPiA+ID4gSW5kZWVkIG91ciBob3N0IGNv
bnRyb2xsZXIgZGVwZW5kcyBvbiBBUk02NCBzbyBtYXliZSBpdCB3b3VsZCBtYWtlDQo+ID4gPiBz
ZW5zZSB0byBtb3ZlIHRoZSBxdWlyayBhcmNoL2FybTY0L2tlcm5lbC9wY2kuYzsgaG93ZXZlciBy
ZWdhcmRsZXNzDQo+ID4gPiB3aHkgaXMgaXQgc3RyaWN0bHkgcmVxdWlyZWQgZm9yIGEgVkdBIGRl
dmljZSB0byBiZSBsZWdhY3kgb25lIGluDQo+IG9yZGVyDQo+ID4gPiB0byBtYWtlIGl0IHRoZSBk
ZWZhdWx0IGJvb3QgZGV2aWNlPw0KPiA+ID4gaS5lLiBjb3VsZG4ndCB3ZSBoYXZlOg0KPiA+ID4N
Cj4gPiA+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2dwdS92Z2EvdmdhYXJiLmMgYi9kcml2ZXJzL2dw
dS92Z2EvdmdhYXJiLmMNCj4gPiA+IGluZGV4IDBmNWIyZGQuLmE2YjYwNmMgMTAwNjQ0DQo+ID4g
PiAtLS0gYS9kcml2ZXJzL2dwdS92Z2EvdmdhYXJiLmMNCj4gPiA+ICsrKyBiL2RyaXZlcnMvZ3B1
L3ZnYS92Z2FhcmIuYw0KPiA+ID4gQEAgLTY2Nyw4ICs2NjcsNyBAQCBzdGF0aWMgYm9vbCB2Z2Ff
YXJiaXRlcl9hZGRfcGNpX2RldmljZShzdHJ1Y3QNCj4gcGNpX2RldiAqcGRldikNCj4gPiA+IMKg
wqDCoMKgwqDCoMKgLyogRGVhbCB3aXRoIFZHQSBkZWZhdWx0IGRldmljZS4gVXNlIGZpcnN0IGVu
YWJsZWQgb25lDQo+ID4gPiDCoMKgwqDCoMKgwqDCoCAqIGJ5IGRlZmF1bHQgaWYgYXJjaCBkb2Vz
bid0IGhhdmUgaXQncyBvd24gaG9vaw0KPiA+ID4gwqDCoMKgwqDCoMKgwqAgKi8NCj4gPiA+IC3C
oMKgwqDCoMKgaWYgKHZnYV9kZWZhdWx0ID09IE5VTEwgJiYNCj4gPiA+IC3CoMKgwqDCoMKgwqDC
oMKgICgodmdhZGV2LT5vd25zICYgVkdBX1JTUkNfTEVHQUNZX01BU0spID09DQo+IFZHQV9SU1JD
X0xFR0FDWV9NQVNLKSkgew0KPiA+ID4gK8KgwqDCoMKgwqBpZiAodmdhX2RlZmF1bHQgPT0gTlVM
TCkgew0KPiA+ID4gwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgdmdhYXJiX2luZm8oJnBk
ZXYtPmRldiwgInNldHRpbmcgYXMgYm9vdCBWR0ENCj4gZGV2aWNlXG4iKTsNCj4gPiA+IMKgwqDC
oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoHZnYV9zZXRfZGVmYXVsdF9kZXZpY2UocGRldik7DQo+
ID4gPiDCoMKgwqDCoMKgwqDCoH0NCj4gPg0KPiA+IEkgZG9uJ3Qga25vdyBlbm91Z2ggYWJvdXQg
dGhlIFZHQSBhcmJpdGVyIHRvIGFuc3dlciB0aGlzLsKgIFRoaXMgdGVzdA0KPiB3YXMNCj4gPiBw
YXJ0IG9mIHRoZSBpbml0aWFsIGltcGxlbWVudGF0aW9uOiBkZWIyZDJlY2Q0M2QgKCJQQ0kvR1BV
OiBpbXBsZW1lbnQNCj4gVkdBDQo+ID4gYXJiaXRyYXRpb24gb24gTGludXgiKSBieSBCZW4uDQo+
IA0KPiBUaGUgYWJvdmUgc2ltcGx5IHVzZXMgdGhlIGZpcnN0IGRldmljZSB0aGF0IGhhcyBtZW1v
cnkgYW5kIElPIGVuYWJsZWQNCj4gYXMgdGhlIGRlZmF1bHQgZGV2aWNlICh5b3UgZG9uJ3QgbmVl
ZCB0byBoYXZlIGEgZGVmYXVsdCBkZXZpY2UpLg0KPiANCj4gVGhpcyBpcyBlc3NlbnRpYWxseSBw
aWNraW5nIHVwIHdoYXRldmVyIGRldmljZSBoYWQgYmVlbiBpbml0aWFsaXplZA0KPiBieSB0aGUg
QklPUy9maXJtd2FyZSBhcyBkZWZhdWx0LiBUaGlzIGlzIG5lZWRlZCBmb3IgZXhhbXBsZSBvbiB4
ODYNCj4gd2hlcmUgdGhlIEJJT1MgdGVuZHMgdG8gb25seSBpbml0aWFsaXplIG9uZSBkZXZpY2Uu
DQo+IA0KPiBJJ20gbm90IHN1cmUgd2hhdCBwcm9ibGVtIHlvdSBhcmUgdHJ5aW5nIHRvIHNvbHZl
IGhlcmUgPw0KDQpXZWxsIG91ciBob3N0IHBsYXRmb3JtIGRvZXMgbm90IHN1cHBvcnQgbGVnYWN5
IGRldmljZXMgYW5kIHRoZXJlZm9yZSB3ZSBmaW5kDQpvdXJzZWx2ZXMgd2l0aG91dCBhIGRlZmF1
bHQgVkdBIGRldmljZS4uLg0KDQpJIHdhcyB0cnlpbmcgdG8gdW5kZXJzdGFuZCB3aHkgYSBkZWZh
dWx0IGRldmljZSBoYXMgdG8gYmUgbGVnYWN5Li4uYnV0IEkgdGhpbmsNCnRoaXMgd2FzIGFuc3dl
cmVkIGJ5IGJvdGggeW91IGFuZCBBbGV4IGluIG90aGVyIGZvbGxvd3MtdXAuLi4NCg0KVGhhbmtz
DQpHYWIgDQoNCj4gDQo+IENoZWVycywNCj4gQmVuLg0KDQpfX19fX19fX19fX19fX19fX19fX19f
X19fX19fX19fX19fX19fX19fX19fX19fXwpsaW51eC1hcm0ta2VybmVsIG1haWxpbmcgbGlzdAps
aW51eC1hcm0ta2VybmVsQGxpc3RzLmluZnJhZGVhZC5vcmcKaHR0cDovL2xpc3RzLmluZnJhZGVh
ZC5vcmcvbWFpbG1hbi9saXN0aW5mby9saW51eC1hcm0ta2VybmVsCg==

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

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

[Gabriele Paoloni]

SGkgQmVuLCBBbGV4DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogQmVu
amFtaW4gSGVycmVuc2NobWlkdCBbbWFpbHRvOmJlbmhAa2VybmVsLmNyYXNoaW5nLm9yZ10NCj4g
U2VudDogMTMgSnVseSAyMDE3IDIyOjIxDQo+IFRvOiBBbGV4IFdpbGxpYW1zb247IEJqb3JuIEhl
bGdhYXMNCj4gQ2M6IEdhYnJpZWxlIFBhb2xvbmk7IERhbmllbCBBeHRlbnM7IGxpbnV4LXBjaUB2
Z2VyLmtlcm5lbC5vcmc7DQo+IExpdXhpbmxpYW5nIChNYXR0aGV3IExpdSk7IFJvbmdyb25nIFpv
dTsgQ2F0YWxpbiBNYXJpbmFzOyBXaWxsIERlYWNvbjsNCj4gbGludXgtYXJtLWtlcm5lbEBsaXN0
cy5pbmZyYWRlYWQub3JnOyBEYXZpZCBBaXJsaWU7IERhbmllbCBWZXR0ZXINCj4gU3ViamVjdDog
UmU6IFtQQVRDSCB2NF0gUENJOiBTdXBwb3J0IGhpYm1jIFZHQSBjYXJkcyBiZWhpbmQgYQ0KPiBt
aXNiZWhhdmluZyBIaVNpbGljb24gYnJpZGdlDQo+IA0KPiBPbiBUaHUsIDIwMTctMDctMTMgYXQg
MTU6MTEgLTA2MDAsIEFsZXggV2lsbGlhbXNvbiB3cm90ZToNCj4gPiA+ID4gwqDCoMKgwqDCoCAq
Lw0KPiA+ID4gPiAtwqDCoMKgaWYgKHZnYV9kZWZhdWx0ID09IE5VTEwgJiYNCj4gPiA+ID4gLcKg
wqDCoMKgwqDCoCAoKHZnYWRldi0+b3ducyAmIFZHQV9SU1JDX0xFR0FDWV9NQVNLKSA9PQ0KPiBW
R0FfUlNSQ19MRUdBQ1lfTUFTSykpIHsNCj4gPiA+ID4gK8KgwqDCoGlmICh2Z2FfZGVmYXVsdCA9
PSBOVUxMKSB7DQo+ID4gPiA+IMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgdmdhYXJiX2luZm8o
JnBkZXYtPmRldiwgInNldHRpbmcgYXMgYm9vdCBWR0ENCj4gZGV2aWNlXG4iKTsNCj4gPiA+ID4g
wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqB2Z2Ffc2V0X2RlZmF1bHRfZGV2aWNlKHBkZXYpOw0K
PiA+ID4gPiDCoMKgwqDCoMKgfQ0KPiA+DQo+ID4NCj4gPiAiTGVnYWN5IiBpcyB0aGUgYnJlYWRj
cnVtYiB3ZSB1c2UgdG8gdHJ5IHRvIHBpY2sgdGhlIHNhbWUgZGV2aWNlIGZvcg0KPiA+IGRlZmF1
bHQgVkdBIGFzIHRoZSBzeXN0ZW0gZmlybXdhcmUgdXNlZCBhcyB0aGUgYm9vdCBWR0EuwqAgVGhl
cmUgY2FuDQo+IGJlDQo+ID4gbXVsdGlwbGUgVkdBIGRldmljZXMgaW4gdGhlIHN5c3RlbSwgdGhl
IGNoYW5nZSBhYm92ZSB3b3VsZCBzaW1wbHkNCj4gbWFrZQ0KPiA+IHRoZSBmaXJzdCBkaXNjb3Zl
cmVkIGRldmljZSBiZSB0aGUgZGVmYXVsdCBWR0EuwqAgVGhhdCB3b3VsZCBicmVhaw0KPiBtYW55
LA0KPiA+IG1hbnkgc3lzdGVtcy7CoCBJZiBsZWdhY3kgVkdBIHJhbmdlcyBtZWFuIG5vdGhpbmcg
b24gQVJNNjQsIHRoZW4NCj4gZm9sbG93DQo+ID4gdGhlIHBvd2VycGMgbGVhZCBhbmQgbWFrZSBh
biBhcmNoIHF1aXJrIHRoYXQgc2ltcGx5IHNlbGVjdHMgdGhlIGZpcnN0DQo+ID4gZW5hYmxlZCBW
R0EgZGV2aWNlIGFzIHRoZSBkZWZhdWx0LsKgIFZHQSByb3V0aW5nIGlzIHBhcnQgb2YgdGhlIFBD
SQ0KPiBzcGVjDQo+ID4gdGhvdWdoLCBzbyB0aGUgZGVmYXVsdCBvZiBzZWxlY3RpbmcgdGhlIGRl
dmljZSB3aXRoIHJvdXRpbmcgZW5hYmxlZA0KPiA+IG1ha2VzIHNlbnNlLsKgIFRoYW5rcywNCj4g
DQo+ICJMZWdhY3kiIHRoZXJlIGlpcmMgYWxzbyBtZWFucyB0aGF0IGl0IGhhcyBkZWNvZGluZyBl
bmFibGVkIGF0IGJvb3QuIElmDQo+IHlvdSBwaWNrIHRoZSBmaXJzdCBvbmUgeW91IG1heSBwaWNr
IGEgZGV2aWNlIHRoYXQgZG9lc24ndCBhbmQgaGFzbid0DQo+IGJlZW4gaW5pdGlhbGl6ZWQgYnkg
YW55IGRyaXZlciAoZ29vZCBvbGQgQklPUy1pbml0aWFsaXplZCB0ZXh0IG1vZGUpLg0KDQpSaWdo
dCBzbyBlZmZlY3RpdmVseSBpZiB0aGVyZSBpcyBhIGxlZ2FjeSBkZXYgd2Ugc2hvdWxkIHBpY2sg
dGhhdCB1cDsNCmhvd2V2ZXIgd2hhdCBhYm91dCB0aGUgZm9sbG93aW5nIGNvZGU6DQoNCmRpZmYg
LS1naXQgYS9kcml2ZXJzL2dwdS92Z2EvdmdhYXJiLmMgYi9kcml2ZXJzL2dwdS92Z2EvdmdhYXJi
LmMNCmluZGV4IDkyZjE0NTIuLmFiM2FkOWEgMTAwNjQ0DQotLS0gYS9kcml2ZXJzL2dwdS92Z2Ev
dmdhYXJiLmMNCisrKyBiL2RyaXZlcnMvZ3B1L3ZnYS92Z2FhcmIuYw0KQEAgLTE0MjQsNiArMTQy
NCwxNCBAQCBzdGF0aWMgaW50IF9faW5pdCB2Z2FfYXJiX2RldmljZV9pbml0KHZvaWQpDQogDQog
CWxpc3RfZm9yX2VhY2hfZW50cnkodmdhZGV2LCAmdmdhX2xpc3QsIGxpc3QpIHsNCiAJCXN0cnVj
dCBkZXZpY2UgKmRldiA9ICZ2Z2FkZXYtPnBkZXYtPmRldjsNCisNCisJCS8qIGlmIG5vIGxlZ2Fj
eSBkZXZpY2UgaGFzIGJlZW4gc2V0IGFzIGRlZmF1bHQgVkdBDQorCQkgKiBkZXZpY2UsIGp1c3Qg
cGljayB1cCB0aGUgZmlyc3Qgb25lIGluIHRoZSBsaXN0ICovDQorCQlpZiAodmdhX2RlZmF1bHQg
PT0gTlVMTCkgew0KKwkJCXZnYWFyYl9pbmZvKGRldiwgInNldHRpbmcgYXMgYm9vdCBWR0EgZGV2
aWNlXG4iKTsNCisJCQl2Z2Ffc2V0X2RlZmF1bHRfZGV2aWNlKHZnYWRldi0+cGRldik7DQorCQl9
DQorDQogI2lmIGRlZmluZWQoQ09ORklHX1g4NikgfHwgZGVmaW5lZChDT05GSUdfSUE2NCkNCiAJ
CS8qDQogCQkgKiBPdmVycmlkZSB2Z2FfYXJiaXRlcl9hZGRfcGNpX2RldmljZSgpJ3MgSS9PIGJh
c2VkIGRldGVjdGlvbiANCg0KQWJvdmUgYWZ0ZXIgd2UgaGF2ZSBmaWxsZWQgdGhlIGxpc3Qgb2Yg
VkdBIGRldmljZXMgYnkgaXRlcmF0aW5nIG92ZXIgYWxsDQpQQ0kgZGV2aWNlcyB3ZSBjaGVjayBp
ZiBubyBsZWdhY3kgb25lIGhhcyBiZWVuIHNldCBhcyBkZWZhdWx0IFZHQSBkZXZpY2UNCnlldDog
dGhlcmVmb3JlIHdlIHNldCB0aGUgZmlyc3QgVkdBIGRldmljZSBpbiB0aGUgbGlzdCBhcyBkZWZh
dWx0IG9uZS4uLg0KDQpEbyB5b3UgdGhpbmsgaXQgd291bGQgd29yaz8NCg0KVGhhbmtzDQpHYWIN
Cg0KPiANCj4gQ2hlZXJzLA0KPiBCZW4uDQoNCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19f
X19fX19fX19fX19fX19fX19fCmxpbnV4LWFybS1rZXJuZWwgbWFpbGluZyBsaXN0CmxpbnV4LWFy
bS1rZXJuZWxAbGlzdHMuaW5mcmFkZWFkLm9yZwpodHRwOi8vbGlzdHMuaW5mcmFkZWFkLm9yZy9t
YWlsbWFuL2xpc3RpbmZvL2xpbnV4LWFybS1rZXJuZWwK

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

Re: [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

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

Re: [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
>=20
> > -----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
> >=20
> > On Thu, 2017-07-13 at 15:11 -0600, Alex Williamson wrote: =20
> > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 */
> > > > > -=C2=A0=C2=A0=C2=A0if (vga_default =3D=3D NULL &&
> > > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ((vgadev->owns & VGA_RSRC_L=
EGACY_MASK) =3D=3D =20
> > VGA_RSRC_LEGACY_MASK)) { =20
> > > > > +=C2=A0=C2=A0=C2=A0if (vga_default =3D=3D NULL) {
> > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0vgaarb_info(&pdev->dev, "setting as boot VGA =20
> > device\n"); =20
> > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0vga_set_default_device(pdev);
> > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} =20
> > >
> > >
> > > "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.=C2=A0 There =
can =20
> > be =20
> > > multiple VGA devices in the system, the change above would simply =20
> > make =20
> > > the first discovered device be the default VGA.=C2=A0 That would brea=
k =20
> > many, =20
> > > many systems.=C2=A0 If legacy VGA ranges mean nothing on ARM64, then =
=20
> > follow =20
> > > the powerpc lead and make an arch quirk that simply selects the first
> > > enabled VGA device as the default.=C2=A0 VGA routing is part of the P=
CI =20
> > spec =20
> > > though, so the default of selecting the device with routing enabled
> > > makes sense.=C2=A0 Thanks, =20
> >=20
> > "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). =
=20
>=20
> Right so effectively if there is a legacy dev we should pick that up;
> however what about the following code:
>=20
> 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)
> =20
>  	list_for_each_entry(vgadev, &vga_list, list) {
>  		struct device *dev =3D &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 =3D=3D 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=20
>=20
> 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...
>=20
> 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

[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

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

[Gabriele Paoloni]

SGkgQWxleCwgQmVuDQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogQmVu
amFtaW4gSGVycmVuc2NobWlkdCBbbWFpbHRvOmJlbmhAa2VybmVsLmNyYXNoaW5nLm9yZ10NCj4g
U2VudDogMTQgSnVseSAyMDE3IDE0OjUwDQo+IFRvOiBHYWJyaWVsZSBQYW9sb25pOyBBbGV4IFdp
bGxpYW1zb247IEJqb3JuIEhlbGdhYXMNCj4gQ2M6IERhbmllbCBBeHRlbnM7IGxpbnV4LXBjaUB2
Z2VyLmtlcm5lbC5vcmc7IExpdXhpbmxpYW5nIChNYXR0aGV3DQo+IExpdSk7IFJvbmdyb25nIFpv
dTsgQ2F0YWxpbiBNYXJpbmFzOyBXaWxsIERlYWNvbjsgbGludXgtYXJtLQ0KPiBrZXJuZWxAbGlz
dHMuaW5mcmFkZWFkLm9yZzsgRGF2aWQgQWlybGllOyBEYW5pZWwgVmV0dGVyDQo+IFN1YmplY3Q6
IFJlOiBbUEFUQ0ggdjRdIFBDSTogU3VwcG9ydCBoaWJtYyBWR0EgY2FyZHMgYmVoaW5kIGENCj4g
bWlzYmVoYXZpbmcgSGlTaWxpY29uIGJyaWRnZQ0KPiANCj4gT24gRnJpLCAyMDE3LTA3LTE0IGF0
IDEyOjI2ICswMDAwLCBHYWJyaWVsZSBQYW9sb25pIHdyb3RlOg0KPiA+IGRpZmYgLS1naXQgYS9k
cml2ZXJzL2dwdS92Z2EvdmdhYXJiLmMgYi9kcml2ZXJzL2dwdS92Z2EvdmdhYXJiLmMNCj4gPiBp
bmRleCA5MmYxNDUyLi5hYjNhZDlhIDEwMDY0NA0KPiA+IC0tLSBhL2RyaXZlcnMvZ3B1L3ZnYS92
Z2FhcmIuYw0KPiA+ICsrKyBiL2RyaXZlcnMvZ3B1L3ZnYS92Z2FhcmIuYw0KPiA+IEBAIC0xNDI0
LDYgKzE0MjQsMTQgQEAgc3RhdGljIGludCBfX2luaXQgdmdhX2FyYl9kZXZpY2VfaW5pdCh2b2lk
KQ0KPiA+DQo+ID4gwqDCoMKgwqDCoMKgwqDCoGxpc3RfZm9yX2VhY2hfZW50cnkodmdhZGV2LCAm
dmdhX2xpc3QsIGxpc3QpIHsNCj4gPiDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoHN0
cnVjdCBkZXZpY2UgKmRldiA9ICZ2Z2FkZXYtPnBkZXYtPmRldjsNCj4gPiArDQo+ID4gK8KgwqDC
oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoC8qIGlmIG5vIGxlZ2FjeSBkZXZpY2UgaGFzIGJlZW4g
c2V0IGFzIGRlZmF1bHQgVkdBDQo+ID4gK8KgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoCAq
IGRldmljZSwganVzdCBwaWNrIHVwIHRoZSBmaXJzdCBvbmUgaW4gdGhlIGxpc3QgKi8NCj4gPiAr
wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgaWYgKHZnYV9kZWZhdWx0ID09IE5VTEwpIHsN
Cj4gPiArwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoHZnYWFy
Yl9pbmZvKGRldiwgInNldHRpbmcgYXMgYm9vdCBWR0ENCj4gZGV2aWNlXG4iKTsNCj4gPiArwqDC
oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoHZnYV9zZXRfZGVmYXVs
dF9kZXZpY2UodmdhZGV2LT5wZGV2KTsNCj4gPiArwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC
oMKgfQ0KPiA+ICsNCj4gPiDCoCNpZiBkZWZpbmVkKENPTkZJR19YODYpIHx8IGRlZmluZWQoQ09O
RklHX0lBNjQpDQo+ID4gwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqAvKg0KPiA+IMKg
wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgICogT3ZlcnJpZGUgdmdhX2FyYml0ZXJfYWRk
X3BjaV9kZXZpY2UoKSdzIEkvTyBiYXNlZA0KPiBkZXRlY3Rpb24NCj4gPg0KPiA+IEFib3ZlIGFm
dGVyIHdlIGhhdmUgZmlsbGVkIHRoZSBsaXN0IG9mIFZHQSBkZXZpY2VzIGJ5IGl0ZXJhdGluZyBv
dmVyDQo+IGFsbA0KPiA+IFBDSSBkZXZpY2VzIHdlIGNoZWNrIGlmIG5vIGxlZ2FjeSBvbmUgaGFz
IGJlZW4gc2V0IGFzIGRlZmF1bHQgVkdBDQo+IGRldmljZQ0KPiA+IHlldDogdGhlcmVmb3JlIHdl
IHNldCB0aGUgZmlyc3QgVkdBIGRldmljZSBpbiB0aGUgbGlzdCBhcyBkZWZhdWx0DQo+IG9uZS4u
Lg0KPiA+DQo+ID4gRG8geW91IHRoaW5rIGl0IHdvdWxkIHdvcms/DQo+IA0KPiBJIGhvbmVzdGx5
IGRvbid0IHJlbWVtYmVyIGFsbCBvZiB0aGUgZGV0YWlscyBvZiB0aGUgYXJiaXRlciBidXQganVz
dA0KPiBtYWtlIHN1cmUgdGhhdCBpdCB3b24ndCB0aGluayB0aGF0IGRldmljZSBpcyBlbmFibGVk
IGZvciB0aGluZ3MgbGlrZQ0KPiBWR0EgZXRjLi4uIGlmIGl0J3MgbWVtb3J5L0lPIGRlY29kZSB3
ZXJlbid0IGVuYWJsZWQuDQo+IA0KDQpJIHRoaW5rIHRoZSBmb2xsb3dpbmcgb25lIHNob3VsZCBt
YWtlIHN1cmUgdGhhdCBNRU0vSU8gcmVzcG9uc2UgYXJlDQplbmFibGVkIChhbHNvIGFzIHN1Z2dl
c3RlZCBieSBBbGV4KQ0KDQpkaWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUvdmdhL3ZnYWFyYi5jIGIv
ZHJpdmVycy9ncHUvdmdhL3ZnYWFyYi5jDQppbmRleCA5MmYxNDUyLi5hOTBjNDhjIDEwMDY0NA0K
LS0tIGEvZHJpdmVycy9ncHUvdmdhL3ZnYWFyYi5jDQorKysgYi9kcml2ZXJzL2dwdS92Z2Evdmdh
YXJiLmMNCkBAIC0xNDI0LDYgKzE0MjQsMjEgQEAgc3RhdGljIGludCBfX2luaXQgdmdhX2FyYl9k
ZXZpY2VfaW5pdCh2b2lkKQ0KIA0KIAlsaXN0X2Zvcl9lYWNoX2VudHJ5KHZnYWRldiwgJnZnYV9s
aXN0LCBsaXN0KSB7DQogCQlzdHJ1Y3QgZGV2aWNlICpkZXYgPSAmdmdhZGV2LT5wZGV2LT5kZXY7
DQorDQorCQkvKiBpZiBubyBsZWdhY3kgZGV2aWNlIGhhcyBiZWVuIHNldCBhcyBkZWZhdWx0IFZH
QSBkZXZpY2UsDQorCQkgKiBqdXN0cGljayB1cCB0aGUgZmlyc3Qgb25lIGluIHRoZSBsaXN0IGNh
cGFibGUgb2YgcmVzcG9uZGluZyB0bw0KKwkJICogbWVtIGFuZCBpbyByZXF1ZXN0cyovDQorCQlp
ZiAodmdhX2RlZmF1bHQgPT0gTlVMTCkgew0KKwkJCXUxNiBjbWQ7DQorDQorCQkJcGNpX3JlYWRf
Y29uZmlnX3dvcmQodmdhZGV2LT5wZGV2LCBQQ0lfQ09NTUFORCwgJmNtZCk7DQorCQkJaWYgKChj
bWQgJiAoUENJX0NPTU1BTkRfSU8gfCBQQ0lfQ09NTUFORF9NRU1PUlkpKSB8fA0KKwkJCQkJIXZn
YV9kZWZhdWx0X2RldmljZSgpKSB7DQorCQkJCXZnYV9zZXRfZGVmYXVsdF9kZXZpY2UodmdhZGV2
LT5wZGV2KTsNCisJCQkJdmdhYXJiX2luZm8oZGV2LCAic2V0dGluZyBhcyBib290IFZHQSBkZXZp
Y2VcbiIpOw0KKwkJCX0NCisJCX0NCisNCiAjaWYgZGVmaW5lZChDT05GSUdfWDg2KSB8fCBkZWZp
bmVkKENPTkZJR19JQTY0KQ0KIAkJLyoNCiAJCSAqIE92ZXJyaWRlIHZnYV9hcmJpdGVyX2FkZF9w
Y2lfZGV2aWNlKCkncyBJL08gYmFzZWQgZGV0ZWN0aW9uDQoNCg0KQWxzbyB0aGUgYWJvdmUgb25l
IGNvdWxkIGFsbG93IHVzIHRvIGdldCByaWQgb2YgZml4dXBfdmdhIGluIHBvd2VycGMuLi4uDQoN
Cg0KPiBJJ2QgcmF0aGVyIHdlIGhhdmUgbm8gZGVmYXVsdCBkZXZpY2UgdW50aWwgYSBkcml2ZXIg
YWN0dWFsbHkgcGlja3MgdXANCj4gdGhvdWdoLCBhbmQgdGhlbiwgaWYgd2Ugc3RpbGwgaGF2ZSBu
byBkZWZhdWx0LCB1c2UgdGhlIGZpcnN0IGRyaXZlciB0bw0KPiBwaWNrIHVwLg0KDQpXZWxsIGZy
b20gbXkgdW5kZXJzdGFuZGluZyB0aGUgUENJIGhvc3QgY29udHJvbGxlciBkcml2ZXIgd2lsbCBl
bnVtZXJhdGUNCmFsbCB0aGUgZGV2aWNlcyBpbiB0aGUgUENJIGhpZXJhcmNoeSBhbmQgY2FsbCBw
Y2lfZGV2aWNlX2FkZCgpIGZvciBlYWNoIG9mDQp0aGVtLCB0aGF0IGluIHR1cm4gd2lsbCBjYWxs
IGRldmljZV9hZGQoKSwgYXQgdGhpcyBzdGFnZSBpZiB0aGVyZSBpcyBhDQpkcml2ZXIgYXZhaWxh
YmxlIGZvciB0aGUgZGV2aWNlIHN1Y2ggZHJpdmVyIHdpbGwgcHJvYmUgb3RoZXJ3aXNlIGl0IHdp
bGwgbm90Lg0KDQpBcmUgeW91IHN1Z2dlc3RpbmcgdG8gYWRkIHRoZSBjb2RlIGFib3ZlIGluIHBj
aV9kZXZpY2VfYWRkKCkgYWZ0ZXIgZGV2aWNlX2FkZCgpDQphbmQgYWZ0ZXIgY2hlY2tpbmcgdGhh
dCBhIGRyaXZlciBoYXMgYmVlbiBib3VuZCBmb3Igc3VjaCBkZXY/DQoNClRoYW5rcw0KR2FiDQoN
Cg0KPiANCj4gQmVuLg0KDQpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f
X19fX19fXwpsaW51eC1hcm0ta2VybmVsIG1haWxpbmcgbGlzdApsaW51eC1hcm0ta2VybmVsQGxp
c3RzLmluZnJhZGVhZC5vcmcKaHR0cDovL2xpc3RzLmluZnJhZGVhZC5vcmcvbWFpbG1hbi9saXN0
aW5mby9saW51eC1hcm0ta2VybmVsCg==

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

Re: [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.

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