diff mbox series

[1/2] vgaarb: Select a default VGA device even if there's no legacy VGA

Message ID 20171006222428.12211.3635.stgit@bhelgaas-glaptop.roam.corp.google.com (mailing list archive)
State Not Applicable
Headers show
Series vgaarb: Select fallback default VGA device | expand

Commit Message

Bjorn Helgaas Oct. 6, 2017, 10:24 p.m. UTC 
From: Bjorn Helgaas <bhelgaas@google.com>

Daniel Axtens reported that on the HiSilicon D05 board, the VGA device is
behind a bridge that doesn't support PCI_BRIDGE_CTL_VGA, so the VGA arbiter
never selects it as the default, which means Xorg auto-detection doesn't
work.

VGA is a legacy PCI feature: a VGA device can respond to addresses, e.g.,
[mem 0xa0000-0xbffff], [io 0x3b0-0x3bb], [io 0x3c0-0x3df], etc., that are
not configurable by BARs.  Consequently, multiple VGA devices can conflict
with each other.  The VGA arbiter avoids conflicts by ensuring that those
legacy resources are only routed to one VGA device at a time.

The arbiter identifies the "default VGA" device, i.e., a legacy VGA device
that was used by boot firmware.  It selects the first device that:

  - is of PCI_CLASS_DISPLAY_VGA,
  - has both PCI_COMMAND_IO and PCI_COMMAND_MEMORY enabled, and
  - has PCI_BRIDGE_CTL_VGA set in all upstream bridges.

Some systems don't have such a device.  For example, if a host bridge
doesn't support I/O space, PCI_COMMAND_IO probably won't be enabled for any
devices below it.  Or, as on the HiSilicon D05, the VGA device may be
behind a bridge that doesn't support PCI_BRIDGE_CTL_VGA, so accesses to the
legacy VGA resources will never reach the device.

This patch extends the arbiter so that if it doesn't find a device that
meets all the above criteria, it selects the first device that:

  - is of PCI_CLASS_DISPLAY_VGA and
  - has PCI_COMMAND_IO or PCI_COMMAND_MEMORY enabled

If it doesn't find even that, it selects the first device that:

  - is of class PCI_CLASS_DISPLAY_VGA.

Such a device may not be able to use the legacy VGA resources, but most
drivers can operate the device without those.  Setting it as the default
device means its "boot_vga" sysfs file will contain "1", which Xorg (via
libpciaccess) uses to help select its default output device.

This fixes Xorg auto-detection on some arm64 systems (HiSilicon D05 in
particular; see the link below).

It also replaces the powerpc fixup_vga() quirk, albeit with slightly
different semantics: the quirk selected the first VGA device we found, and
overrode that selection with any enabled VGA device we found.  If there
were several enabled VGA devices, the *last* one we found would become the
default.

The code here instead selects the *first* enabled VGA device we find, and
if none are enabled, the first VGA device we find.

Link: http://lkml.kernel.org/r/20170901072744.2409-1-dja@axtens.net
Tested-by: Daniel Axtens <dja@axtens.net>    # arm64, ppc64-qemu-tcg
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 arch/powerpc/kernel/pci-common.c |   12 ------------
 drivers/gpu/vga/vgaarb.c         |   25 +++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 12 deletions(-)

Comments

Julien Thierry Oct. 12, 2017, 11:24 a.m. UTC | #1 
Hi Bjorn,

On 06/10/17 23:24, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Daniel Axtens reported that on the HiSilicon D05 board, the VGA device is
> behind a bridge that doesn't support PCI_BRIDGE_CTL_VGA, so the VGA arbiter
> never selects it as the default, which means Xorg auto-detection doesn't
> work.
> 
> VGA is a legacy PCI feature: a VGA device can respond to addresses, e.g.,
> [mem 0xa0000-0xbffff], [io 0x3b0-0x3bb], [io 0x3c0-0x3df], etc., that are
> not configurable by BARs.  Consequently, multiple VGA devices can conflict
> with each other.  The VGA arbiter avoids conflicts by ensuring that those
> legacy resources are only routed to one VGA device at a time.
> 
> The arbiter identifies the "default VGA" device, i.e., a legacy VGA device
> that was used by boot firmware.  It selects the first device that:
> 
>    - is of PCI_CLASS_DISPLAY_VGA,
>    - has both PCI_COMMAND_IO and PCI_COMMAND_MEMORY enabled, and
>    - has PCI_BRIDGE_CTL_VGA set in all upstream bridges.
> 
> Some systems don't have such a device.  For example, if a host bridge
> doesn't support I/O space, PCI_COMMAND_IO probably won't be enabled for any
> devices below it.  Or, as on the HiSilicon D05, the VGA device may be
> behind a bridge that doesn't support PCI_BRIDGE_CTL_VGA, so accesses to the
> legacy VGA resources will never reach the device.
> 
> This patch extends the arbiter so that if it doesn't find a device that
> meets all the above criteria, it selects the first device that:
> 
>    - is of PCI_CLASS_DISPLAY_VGA and
>    - has PCI_COMMAND_IO or PCI_COMMAND_MEMORY enabled
> 
> If it doesn't find even that, it selects the first device that:
> 
>    - is of class PCI_CLASS_DISPLAY_VGA.
> 
> Such a device may not be able to use the legacy VGA resources, but most
> drivers can operate the device without those.  Setting it as the default
> device means its "boot_vga" sysfs file will contain "1", which Xorg (via
> libpciaccess) uses to help select its default output device.
> 
> This fixes Xorg auto-detection on some arm64 systems (HiSilicon D05 in
> particular; see the link below).
> 
> It also replaces the powerpc fixup_vga() quirk, albeit with slightly
> different semantics: the quirk selected the first VGA device we found, and
> overrode that selection with any enabled VGA device we found.  If there
> were several enabled VGA devices, the *last* one we found would become the
> default.
> 
> The code here instead selects the *first* enabled VGA device we find, and
> if none are enabled, the first VGA device we find.
> 
> Link: http://lkml.kernel.org/r/20170901072744.2409-1-dja@axtens.net
> Tested-by: Daniel Axtens <dja@axtens.net>    # arm64, ppc64-qemu-tcg
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>   arch/powerpc/kernel/pci-common.c |   12 ------------
>   drivers/gpu/vga/vgaarb.c         |   25 +++++++++++++++++++++++++
>   2 files changed, 25 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> index 02831a396419..0ac7aa346c69 100644
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -1740,15 +1740,3 @@ static void fixup_hide_host_resource_fsl(struct pci_dev *dev)
>   }
>   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MOTOROLA, PCI_ANY_ID, fixup_hide_host_resource_fsl);
>   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_FREESCALE, PCI_ANY_ID, fixup_hide_host_resource_fsl);
> -
> -static void fixup_vga(struct pci_dev *pdev)
> -{
> -	u16 cmd;
> -
> -	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> -	if ((cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) || !vga_default_device())
> -		vga_set_default_device(pdev);
> -
> -}
> -DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
> -			      PCI_CLASS_DISPLAY_VGA, 8, fixup_vga);
> diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> index 76875f6299b8..aeb41f793ed4 100644
> --- a/drivers/gpu/vga/vgaarb.c
> +++ b/drivers/gpu/vga/vgaarb.c
> @@ -1468,6 +1468,31 @@ static int __init vga_arb_device_init(void)
>   			vgaarb_info(dev, "no bridge control possible\n");
>   	}
>   
> +	if (!vga_default_device()) {
> +		list_for_each_entry(vgadev, &vga_list, list) {
> +			struct device *dev = &vgadev->pdev->dev;
> +			u16 cmd;
> +
> +			pdev = vgadev->pdev;
> +			pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> +			if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
> +				vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n");
> +				vga_set_default_device(pdev);
> +				break;
> +			}
> +		}
> +	}
> +
> +	if (!vga_default_device()) {
> +		vgadev = list_first_entry_or_null(&vga_list,
> +						  struct vga_device, list);
> +		if (vgadev) {
> +			struct device *dev = &vgadev->pdev->dev;
> +			vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n");
> +			vga_set_default_device(pdev);

Isn't 'pdev' NULL here? shouldn't it be vgadev->pdev instead?

Cheers,
Lothar Waßmann Oct. 12, 2017, 12:05 p.m. UTC | #2 
Hi,

On Thu, 12 Oct 2017 12:24:10 +0100 Julien Thierry wrote:
> Hi Bjorn,
> 
> On 06/10/17 23:24, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > Daniel Axtens reported that on the HiSilicon D05 board, the VGA device is
> > behind a bridge that doesn't support PCI_BRIDGE_CTL_VGA, so the VGA arbiter
> > never selects it as the default, which means Xorg auto-detection doesn't
> > work.
> > 
> > VGA is a legacy PCI feature: a VGA device can respond to addresses, e.g.,
> > [mem 0xa0000-0xbffff], [io 0x3b0-0x3bb], [io 0x3c0-0x3df], etc., that are
> > not configurable by BARs.  Consequently, multiple VGA devices can conflict
> > with each other.  The VGA arbiter avoids conflicts by ensuring that those
> > legacy resources are only routed to one VGA device at a time.
> > 
> > The arbiter identifies the "default VGA" device, i.e., a legacy VGA device
> > that was used by boot firmware.  It selects the first device that:
> > 
> >    - is of PCI_CLASS_DISPLAY_VGA,
> >    - has both PCI_COMMAND_IO and PCI_COMMAND_MEMORY enabled, and
> >    - has PCI_BRIDGE_CTL_VGA set in all upstream bridges.
> > 
> > Some systems don't have such a device.  For example, if a host bridge
> > doesn't support I/O space, PCI_COMMAND_IO probably won't be enabled for any
> > devices below it.  Or, as on the HiSilicon D05, the VGA device may be
> > behind a bridge that doesn't support PCI_BRIDGE_CTL_VGA, so accesses to the
> > legacy VGA resources will never reach the device.
> > 
> > This patch extends the arbiter so that if it doesn't find a device that
> > meets all the above criteria, it selects the first device that:
> > 
> >    - is of PCI_CLASS_DISPLAY_VGA and
> >    - has PCI_COMMAND_IO or PCI_COMMAND_MEMORY enabled
> > 
> > If it doesn't find even that, it selects the first device that:
> > 
> >    - is of class PCI_CLASS_DISPLAY_VGA.
> > 
> > Such a device may not be able to use the legacy VGA resources, but most
> > drivers can operate the device without those.  Setting it as the default
> > device means its "boot_vga" sysfs file will contain "1", which Xorg (via
> > libpciaccess) uses to help select its default output device.
> > 
> > This fixes Xorg auto-detection on some arm64 systems (HiSilicon D05 in
> > particular; see the link below).
> > 
> > It also replaces the powerpc fixup_vga() quirk, albeit with slightly
> > different semantics: the quirk selected the first VGA device we found, and
> > overrode that selection with any enabled VGA device we found.  If there
> > were several enabled VGA devices, the *last* one we found would become the
> > default.
> > 
> > The code here instead selects the *first* enabled VGA device we find, and
> > if none are enabled, the first VGA device we find.
> > 
> > Link: http://lkml.kernel.org/r/20170901072744.2409-1-dja@axtens.net
> > Tested-by: Daniel Axtens <dja@axtens.net>    # arm64, ppc64-qemu-tcg
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >   arch/powerpc/kernel/pci-common.c |   12 ------------
> >   drivers/gpu/vga/vgaarb.c         |   25 +++++++++++++++++++++++++
> >   2 files changed, 25 insertions(+), 12 deletions(-)
> > 
> > diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> > index 02831a396419..0ac7aa346c69 100644
> > --- a/arch/powerpc/kernel/pci-common.c
> > +++ b/arch/powerpc/kernel/pci-common.c
> > @@ -1740,15 +1740,3 @@ static void fixup_hide_host_resource_fsl(struct pci_dev *dev)
> >   }
> >   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MOTOROLA, PCI_ANY_ID, fixup_hide_host_resource_fsl);
> >   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_FREESCALE, PCI_ANY_ID, fixup_hide_host_resource_fsl);
> > -
> > -static void fixup_vga(struct pci_dev *pdev)
> > -{
> > -	u16 cmd;
> > -
> > -	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> > -	if ((cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) || !vga_default_device())
> > -		vga_set_default_device(pdev);
> > -
> > -}
> > -DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
> > -			      PCI_CLASS_DISPLAY_VGA, 8, fixup_vga);
> > diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> > index 76875f6299b8..aeb41f793ed4 100644
> > --- a/drivers/gpu/vga/vgaarb.c
> > +++ b/drivers/gpu/vga/vgaarb.c
> > @@ -1468,6 +1468,31 @@ static int __init vga_arb_device_init(void)
> >   			vgaarb_info(dev, "no bridge control possible\n");
> >   	}
> >   
> > +	if (!vga_default_device()) {
> > +		list_for_each_entry(vgadev, &vga_list, list) {
> > +			struct device *dev = &vgadev->pdev->dev;
> > +			u16 cmd;
> > +
> > +			pdev = vgadev->pdev;
> > +			pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> > +			if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
> > +				vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n");
> > +				vga_set_default_device(pdev);
> > +				break;
> > +			}
> > +		}
> > +	}
> > +
> > +	if (!vga_default_device()) {
> > +		vgadev = list_first_entry_or_null(&vga_list,
> > +						  struct vga_device, list);
> > +		if (vgadev) {
> > +			struct device *dev = &vgadev->pdev->dev;
> > +			vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n");
> > +			vga_set_default_device(pdev);
> 
> Isn't 'pdev' NULL here? shouldn't it be vgadev->pdev instead?
> 
That cannot not happen, though it isn't quite obvious.
'vgadev' will only be non-NULL, when the vga_list isn't empty and in
that case pdev has been set up in the list_for_each_entry() loop above.


Lothar Waßmann
Julien Thierry Oct. 12, 2017, 12:56 p.m. UTC | #3 
On 12/10/17 13:05, Lothar Waßmann wrote:
> Hi,
> 
> On Thu, 12 Oct 2017 12:24:10 +0100 Julien Thierry wrote:
>> Hi Bjorn,
>>
>> On 06/10/17 23:24, Bjorn Helgaas wrote:
>>> From: Bjorn Helgaas <bhelgaas@google.com>
>>>
>>> Daniel Axtens reported that on the HiSilicon D05 board, the VGA device is
>>> behind a bridge that doesn't support PCI_BRIDGE_CTL_VGA, so the VGA arbiter
>>> never selects it as the default, which means Xorg auto-detection doesn't
>>> work.
>>>
>>> VGA is a legacy PCI feature: a VGA device can respond to addresses, e.g.,
>>> [mem 0xa0000-0xbffff], [io 0x3b0-0x3bb], [io 0x3c0-0x3df], etc., that are
>>> not configurable by BARs.  Consequently, multiple VGA devices can conflict
>>> with each other.  The VGA arbiter avoids conflicts by ensuring that those
>>> legacy resources are only routed to one VGA device at a time.
>>>
>>> The arbiter identifies the "default VGA" device, i.e., a legacy VGA device
>>> that was used by boot firmware.  It selects the first device that:
>>>
>>>     - is of PCI_CLASS_DISPLAY_VGA,
>>>     - has both PCI_COMMAND_IO and PCI_COMMAND_MEMORY enabled, and
>>>     - has PCI_BRIDGE_CTL_VGA set in all upstream bridges.
>>>
>>> Some systems don't have such a device.  For example, if a host bridge
>>> doesn't support I/O space, PCI_COMMAND_IO probably won't be enabled for any
>>> devices below it.  Or, as on the HiSilicon D05, the VGA device may be
>>> behind a bridge that doesn't support PCI_BRIDGE_CTL_VGA, so accesses to the
>>> legacy VGA resources will never reach the device.
>>>
>>> This patch extends the arbiter so that if it doesn't find a device that
>>> meets all the above criteria, it selects the first device that:
>>>
>>>     - is of PCI_CLASS_DISPLAY_VGA and
>>>     - has PCI_COMMAND_IO or PCI_COMMAND_MEMORY enabled
>>>
>>> If it doesn't find even that, it selects the first device that:
>>>
>>>     - is of class PCI_CLASS_DISPLAY_VGA.
>>>
>>> Such a device may not be able to use the legacy VGA resources, but most
>>> drivers can operate the device without those.  Setting it as the default
>>> device means its "boot_vga" sysfs file will contain "1", which Xorg (via
>>> libpciaccess) uses to help select its default output device.
>>>
>>> This fixes Xorg auto-detection on some arm64 systems (HiSilicon D05 in
>>> particular; see the link below).
>>>
>>> It also replaces the powerpc fixup_vga() quirk, albeit with slightly
>>> different semantics: the quirk selected the first VGA device we found, and
>>> overrode that selection with any enabled VGA device we found.  If there
>>> were several enabled VGA devices, the *last* one we found would become the
>>> default.
>>>
>>> The code here instead selects the *first* enabled VGA device we find, and
>>> if none are enabled, the first VGA device we find.
>>>
>>> Link: http://lkml.kernel.org/r/20170901072744.2409-1-dja@axtens.net
>>> Tested-by: Daniel Axtens <dja@axtens.net>    # arm64, ppc64-qemu-tcg
>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>> ---
>>>    arch/powerpc/kernel/pci-common.c |   12 ------------
>>>    drivers/gpu/vga/vgaarb.c         |   25 +++++++++++++++++++++++++
>>>    2 files changed, 25 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
>>> index 02831a396419..0ac7aa346c69 100644
>>> --- a/arch/powerpc/kernel/pci-common.c
>>> +++ b/arch/powerpc/kernel/pci-common.c
>>> @@ -1740,15 +1740,3 @@ static void fixup_hide_host_resource_fsl(struct pci_dev *dev)
>>>    }
>>>    DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MOTOROLA, PCI_ANY_ID, fixup_hide_host_resource_fsl);
>>>    DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_FREESCALE, PCI_ANY_ID, fixup_hide_host_resource_fsl);
>>> -
>>> -static void fixup_vga(struct pci_dev *pdev)
>>> -{
>>> -	u16 cmd;
>>> -
>>> -	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
>>> -	if ((cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) || !vga_default_device())
>>> -		vga_set_default_device(pdev);
>>> -
>>> -}
>>> -DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
>>> -			      PCI_CLASS_DISPLAY_VGA, 8, fixup_vga);
>>> diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
>>> index 76875f6299b8..aeb41f793ed4 100644
>>> --- a/drivers/gpu/vga/vgaarb.c
>>> +++ b/drivers/gpu/vga/vgaarb.c
>>> @@ -1468,6 +1468,31 @@ static int __init vga_arb_device_init(void)
>>>    			vgaarb_info(dev, "no bridge control possible\n");
>>>    	}
>>>    
>>> +	if (!vga_default_device()) {
>>> +		list_for_each_entry(vgadev, &vga_list, list) {
>>> +			struct device *dev = &vgadev->pdev->dev;
>>> +			u16 cmd;
>>> +
>>> +			pdev = vgadev->pdev;
>>> +			pci_read_config_word(pdev, PCI_COMMAND, &cmd);
>>> +			if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
>>> +				vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n");
>>> +				vga_set_default_device(pdev);
>>> +				break;
>>> +			}
>>> +		}
>>> +	}
>>> +
>>> +	if (!vga_default_device()) {
>>> +		vgadev = list_first_entry_or_null(&vga_list,
>>> +						  struct vga_device, list);
>>> +		if (vgadev) {
>>> +			struct device *dev = &vgadev->pdev->dev;
>>> +			vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n");
>>> +			vga_set_default_device(pdev);
>>
>> Isn't 'pdev' NULL here? shouldn't it be vgadev->pdev instead?
>>
> That cannot not happen, though it isn't quite obvious.
> 'vgadev' will only be non-NULL, when the vga_list isn't empty and in
> that case pdev has been set up in the list_for_each_entry() loop above.
> 

Indeed, it can't be NULL. However, in that case we would be using the 
pdev from the last vgadev in the list whereas this last case suggest we 
are picking the first vgadev in the list. So I don't understand why 
'pdev' is being used rather than 'vgadev-pdev'.

Thanks,
Bjorn Helgaas Oct. 13, 2017, 3:44 a.m. UTC | #4 
On Thu, Oct 12, 2017 at 12:24:10PM +0100, Julien Thierry wrote:
> Hi Bjorn,
> 
> On 06/10/17 23:24, Bjorn Helgaas wrote:
> >From: Bjorn Helgaas <bhelgaas@google.com>
> >
> >Daniel Axtens reported that on the HiSilicon D05 board, the VGA device is
> >behind a bridge that doesn't support PCI_BRIDGE_CTL_VGA, so the VGA arbiter
> >never selects it as the default, which means Xorg auto-detection doesn't
> >work.
> >
> >VGA is a legacy PCI feature: a VGA device can respond to addresses, e.g.,
> >[mem 0xa0000-0xbffff], [io 0x3b0-0x3bb], [io 0x3c0-0x3df], etc., that are
> >not configurable by BARs.  Consequently, multiple VGA devices can conflict
> >with each other.  The VGA arbiter avoids conflicts by ensuring that those
> >legacy resources are only routed to one VGA device at a time.
> >
> >The arbiter identifies the "default VGA" device, i.e., a legacy VGA device
> >that was used by boot firmware.  It selects the first device that:
> >
> >   - is of PCI_CLASS_DISPLAY_VGA,
> >   - has both PCI_COMMAND_IO and PCI_COMMAND_MEMORY enabled, and
> >   - has PCI_BRIDGE_CTL_VGA set in all upstream bridges.
> >
> >Some systems don't have such a device.  For example, if a host bridge
> >doesn't support I/O space, PCI_COMMAND_IO probably won't be enabled for any
> >devices below it.  Or, as on the HiSilicon D05, the VGA device may be
> >behind a bridge that doesn't support PCI_BRIDGE_CTL_VGA, so accesses to the
> >legacy VGA resources will never reach the device.
> >
> >This patch extends the arbiter so that if it doesn't find a device that
> >meets all the above criteria, it selects the first device that:
> >
> >   - is of PCI_CLASS_DISPLAY_VGA and
> >   - has PCI_COMMAND_IO or PCI_COMMAND_MEMORY enabled
> >
> >If it doesn't find even that, it selects the first device that:
> >
> >   - is of class PCI_CLASS_DISPLAY_VGA.
> >
> >Such a device may not be able to use the legacy VGA resources, but most
> >drivers can operate the device without those.  Setting it as the default
> >device means its "boot_vga" sysfs file will contain "1", which Xorg (via
> >libpciaccess) uses to help select its default output device.
> >
> >This fixes Xorg auto-detection on some arm64 systems (HiSilicon D05 in
> >particular; see the link below).
> >
> >It also replaces the powerpc fixup_vga() quirk, albeit with slightly
> >different semantics: the quirk selected the first VGA device we found, and
> >overrode that selection with any enabled VGA device we found.  If there
> >were several enabled VGA devices, the *last* one we found would become the
> >default.
> >
> >The code here instead selects the *first* enabled VGA device we find, and
> >if none are enabled, the first VGA device we find.
> >
> >Link: http://lkml.kernel.org/r/20170901072744.2409-1-dja@axtens.net
> >Tested-by: Daniel Axtens <dja@axtens.net>    # arm64, ppc64-qemu-tcg
> >Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >---
> >  arch/powerpc/kernel/pci-common.c |   12 ------------
> >  drivers/gpu/vga/vgaarb.c         |   25 +++++++++++++++++++++++++
> >  2 files changed, 25 insertions(+), 12 deletions(-)
> >
> >diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> >index 02831a396419..0ac7aa346c69 100644
> >--- a/arch/powerpc/kernel/pci-common.c
> >+++ b/arch/powerpc/kernel/pci-common.c
> >@@ -1740,15 +1740,3 @@ static void fixup_hide_host_resource_fsl(struct pci_dev *dev)
> >  }
> >  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MOTOROLA, PCI_ANY_ID, fixup_hide_host_resource_fsl);
> >  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_FREESCALE, PCI_ANY_ID, fixup_hide_host_resource_fsl);
> >-
> >-static void fixup_vga(struct pci_dev *pdev)
> >-{
> >-	u16 cmd;
> >-
> >-	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> >-	if ((cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) || !vga_default_device())
> >-		vga_set_default_device(pdev);
> >-
> >-}
> >-DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
> >-			      PCI_CLASS_DISPLAY_VGA, 8, fixup_vga);
> >diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> >index 76875f6299b8..aeb41f793ed4 100644
> >--- a/drivers/gpu/vga/vgaarb.c
> >+++ b/drivers/gpu/vga/vgaarb.c
> >@@ -1468,6 +1468,31 @@ static int __init vga_arb_device_init(void)
> >  			vgaarb_info(dev, "no bridge control possible\n");
> >  	}
> >+	if (!vga_default_device()) {
> >+		list_for_each_entry(vgadev, &vga_list, list) {
> >+			struct device *dev = &vgadev->pdev->dev;
> >+			u16 cmd;
> >+
> >+			pdev = vgadev->pdev;
> >+			pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> >+			if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
> >+				vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n");
> >+				vga_set_default_device(pdev);
> >+				break;
> >+			}
> >+		}
> >+	}
> >+
> >+	if (!vga_default_device()) {
> >+		vgadev = list_first_entry_or_null(&vga_list,
> >+						  struct vga_device, list);
> >+		if (vgadev) {
> >+			struct device *dev = &vgadev->pdev->dev;
> >+			vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n");
> >+			vga_set_default_device(pdev);
> 
> Isn't 'pdev' NULL here? shouldn't it be vgadev->pdev instead?

Yes, vgadev->pdev is what I intended, thanks a lot for pointing that out!

Bjorn
diff mbox series

Patch

diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index 02831a396419..0ac7aa346c69 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -1740,15 +1740,3 @@  static void fixup_hide_host_resource_fsl(struct pci_dev *dev)
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MOTOROLA, PCI_ANY_ID, fixup_hide_host_resource_fsl);
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_FREESCALE, PCI_ANY_ID, fixup_hide_host_resource_fsl);
-
-static void fixup_vga(struct pci_dev *pdev)
-{
-	u16 cmd;
-
-	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
-	if ((cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) || !vga_default_device())
-		vga_set_default_device(pdev);
-
-}
-DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
-			      PCI_CLASS_DISPLAY_VGA, 8, fixup_vga);
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index 76875f6299b8..aeb41f793ed4 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -1468,6 +1468,31 @@  static int __init vga_arb_device_init(void)
 			vgaarb_info(dev, "no bridge control possible\n");
 	}
 
+	if (!vga_default_device()) {
+		list_for_each_entry(vgadev, &vga_list, list) {
+			struct device *dev = &vgadev->pdev->dev;
+			u16 cmd;
+
+			pdev = vgadev->pdev;
+			pci_read_config_word(pdev, PCI_COMMAND, &cmd);
+			if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
+				vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n");
+				vga_set_default_device(pdev);
+				break;
+			}
+		}
+	}
+
+	if (!vga_default_device()) {
+		vgadev = list_first_entry_or_null(&vga_list,
+						  struct vga_device, list);
+		if (vgadev) {
+			struct device *dev = &vgadev->pdev->dev;
+			vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n");
+			vga_set_default_device(pdev);
+		}
+	}
+
 	pr_info("loaded\n");
 	return rc;
 }