[PATCH v3 0/3] Split default display handling out from VGA arbiter

[PATCH v3 0/3] Split default display handling out from VGA arbiter

[Daniel Axtens]

This patch set:

 - splits the default display handling out from VGA arbiter, into its
   own file and behind its own Kconfig option (and gives the functions
   better names).

 - adds extra detection of default devices. To be nominated, the vga
   arbiter and platform hooks must not have nominated a default. A
   card will then only be nominated if it has a driver attached and
   has IO or memory decoding enabled.

 - adds relevant documentation.

The practical impact of this is improved X autoconfiguration on some
arm64 systems.

Changes in v3:

 - Add documentation - thanks Daniel Vetter for pointing it out.

 - Clarify explanations. Thanks to everyone for continuing to bear
   with my incomplete understanding of PCI and provide some clarity.

 - Split refactoring and adding functionality.

Changes in v2: https://www.spinics.net/lists/linux-pci/msg64007.html

Drop all the powerpc patches. [explanation snipped]

v1: https://www.spinics.net/lists/linux-pci/msg63581.html

Regards,
Daniel

Daniel Axtens (3):
  drm: split default display handler out of VGA arbiter
  drm: add fallback default device detection
  drm: documentation for default display device

 Documentation/gpu/default_display.rst |  93 +++++++++++++++++++
 Documentation/gpu/index.rst           |   1 +
 arch/ia64/pci/fixup.c                 |   6 +-
 arch/powerpc/kernel/pci-common.c      |   6 +-
 arch/x86/pci/fixup.c                  |   6 +-
 arch/x86/video/fbdev.c                |   4 +-
 drivers/gpu/vga/Kconfig               |  12 +++
 drivers/gpu/vga/Makefile              |   1 +
 drivers/gpu/vga/default_display.c     | 163 ++++++++++++++++++++++++++++++++++
 drivers/gpu/vga/vga_switcheroo.c      |   8 +-
 drivers/gpu/vga/vgaarb.c              |  61 +++----------
 drivers/pci/pci-sysfs.c               |   4 +-
 include/linux/default_display.h       |  44 +++++++++
 include/linux/vgaarb.h                |  15 ----
 14 files changed, 344 insertions(+), 80 deletions(-)
 create mode 100644 Documentation/gpu/default_display.rst
 create mode 100644 drivers/gpu/vga/default_display.c
 create mode 100644 include/linux/default_display.h

-- 
2.11.0

[PATCH v3 1/3] drm: split default display handler out of VGA arbiter

[Daniel Axtens]

Split the small bit of code that does default VGA handling out from
the arbiter. Add a Kconfig option to allow the kernel to be built
with just the default handling, or the arbiter and default handling.

While doing this, rename the functions from vga_(set_)default_device
to pci_(set_)default_display. This makes it clear that these functions
do not rely on legacy VGA access: they're about the default PCI display.
(The device still needs to be a member of PCI_CLASS_DISPLAY_VGA.)

This should not introduce any functional change.

Signed-off-by: Daniel Axtens <dja@axtens.net>

---

I know this adds another config option and that's a bit sad, but
we can't include it unconditionally as it depends on PCI.
Suggestions welcome.

v3: split from the part where we add functionality
    rename functions
---
 arch/ia64/pci/fixup.c             |  6 +--
 arch/powerpc/kernel/pci-common.c  |  6 +--
 arch/x86/pci/fixup.c              |  6 +--
 arch/x86/video/fbdev.c            |  4 +-
 drivers/gpu/vga/Kconfig           | 12 ++++++
 drivers/gpu/vga/Makefile          |  1 +
 drivers/gpu/vga/default_display.c | 86 +++++++++++++++++++++++++++++++++++++++
 drivers/gpu/vga/vga_switcheroo.c  |  8 ++--
 drivers/gpu/vga/vgaarb.c          | 61 ++++++---------------------
 drivers/pci/pci-sysfs.c           |  4 +-
 include/linux/default_display.h   | 44 ++++++++++++++++++++
 include/linux/vgaarb.h            | 15 -------
 12 files changed, 173 insertions(+), 80 deletions(-)
 create mode 100644 drivers/gpu/vga/default_display.c
 create mode 100644 include/linux/default_display.h

diff --git a/arch/ia64/pci/fixup.c b/arch/ia64/pci/fixup.c
index 41caa99add51..a5f35e767c84 100644
--- a/arch/ia64/pci/fixup.c
+++ b/arch/ia64/pci/fixup.c
@@ -5,7 +5,7 @@
 
 #include <linux/pci.h>
 #include <linux/init.h>
-#include <linux/vgaarb.h>
+#include <linux/default_display.h>
 #include <linux/screen_info.h>
 
 #include <asm/machvec.h>
@@ -22,7 +22,7 @@
  * card with this copy. On laptops this copy has to be used since
  * the main ROM may be compressed or combined with another image.
  * See pci_map_rom() for use of this flag. Before marking the device
- * with IORESOURCE_ROM_SHADOW check if a vga_default_device is already set
+ * with IORESOURCE_ROM_SHADOW check if a pci_default_display is already set
  * by either arch code or vga-arbitration; if so only apply the fixup to this
  * already-determined primary video card.
  */
@@ -59,7 +59,7 @@ static void pci_fixup_video(struct pci_dev *pdev)
 		}
 		bus = bus->parent;
 	}
-	if (!vga_default_device() || pdev == vga_default_device()) {
+	if (!pci_default_display() || pdev == pci_default_display()) {
 		pci_read_config_word(pdev, PCI_COMMAND, &config);
 		if (config & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
 			res = &pdev->resource[PCI_ROM_RESOURCE];
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index 341a7469cab8..9f82f13ac531 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -31,7 +31,7 @@
 #include <linux/irq.h>
 #include <linux/vmalloc.h>
 #include <linux/slab.h>
-#include <linux/vgaarb.h>
+#include <linux/default_display.h>
 
 #include <asm/processor.h>
 #include <asm/io.h>
@@ -1747,8 +1747,8 @@ 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);
+	if ((cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) || !pci_default_display())
+		pci_set_default_display(pdev);
 
 }
 DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index 11e407489db0..7e32a2a80383 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -5,7 +5,7 @@
 #include <linux/delay.h>
 #include <linux/dmi.h>
 #include <linux/pci.h>
-#include <linux/vgaarb.h>
+#include <linux/default_display.h>
 #include <asm/hpet.h>
 #include <asm/pci_x86.h>
 
@@ -302,7 +302,7 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_MCH_PC1,	pcie_r
  * card with this copy. On laptops this copy has to be used since
  * the main ROM may be compressed or combined with another image.
  * See pci_map_rom() for use of this flag. Before marking the device
- * with IORESOURCE_ROM_SHADOW check if a vga_default_device is already set
+ * with IORESOURCE_ROM_SHADOW check if a pci_default_display is already set
  * by either arch code or vga-arbitration; if so only apply the fixup to this
  * already-determined primary video card.
  */
@@ -334,7 +334,7 @@ static void pci_fixup_video(struct pci_dev *pdev)
 		}
 		bus = bus->parent;
 	}
-	if (!vga_default_device() || pdev == vga_default_device()) {
+	if (!pci_default_display() || pdev == pci_default_display()) {
 		pci_read_config_word(pdev, PCI_COMMAND, &config);
 		if (config & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
 			res = &pdev->resource[PCI_ROM_RESOURCE];
diff --git a/arch/x86/video/fbdev.c b/arch/x86/video/fbdev.c
index 9fd24846d094..114bd9ac95d0 100644
--- a/arch/x86/video/fbdev.c
+++ b/arch/x86/video/fbdev.c
@@ -9,12 +9,12 @@
 #include <linux/fb.h>
 #include <linux/pci.h>
 #include <linux/module.h>
-#include <linux/vgaarb.h>
+#include <linux/default_display.h>
 
 int fb_is_primary_device(struct fb_info *info)
 {
 	struct device *device = info->device;
-	struct pci_dev *default_device = vga_default_device();
+	struct pci_dev *default_device = pci_default_display();
 	struct pci_dev *pci_dev;
 	struct resource *res;
 
diff --git a/drivers/gpu/vga/Kconfig b/drivers/gpu/vga/Kconfig
index 29437eabe095..8e6edfb6d160 100644
--- a/drivers/gpu/vga/Kconfig
+++ b/drivers/gpu/vga/Kconfig
@@ -1,3 +1,14 @@
+config DEFAULT_DISPLAY
+	bool "Default Display Device Support" if EXPERT
+	default y
+	depends on PCI
+	help
+	  Some programs find it helpful to know what PCI display device is the
+	  default. On platforms like x86 this means the device used by the BIOS
+	  to show early boot messages. On other platforms this may be an arbitrary
+	  PCI graphics card. Select this to have a default device recorded within
+	  the kernel and exposed to userspace through sysfs.
+
 config VGA_ARB
 	bool "VGA Arbitration" if EXPERT
 	default y
@@ -22,6 +33,7 @@ config VGA_SWITCHEROO
 	depends on X86
 	depends on ACPI
 	select VGA_ARB
+	select DEFAULT_DISPLAY
 	help
 	  Many laptops released in 2008/9/10 have two GPUs with a multiplexer
 	  to switch between them. This adds support for dynamic switching when
diff --git a/drivers/gpu/vga/Makefile b/drivers/gpu/vga/Makefile
index 14ca30b75d0a..3abf32c26de5 100644
--- a/drivers/gpu/vga/Makefile
+++ b/drivers/gpu/vga/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_VGA_ARB)  += vgaarb.o
+obj-$(CONFIG_DEFAULT_DISPLAY) += default_display.o
 obj-$(CONFIG_VGA_SWITCHEROO) += vga_switcheroo.o
diff --git a/drivers/gpu/vga/default_display.c b/drivers/gpu/vga/default_display.c
new file mode 100644
index 000000000000..99e4723360da
--- /dev/null
+++ b/drivers/gpu/vga/default_display.c
@@ -0,0 +1,86 @@
+/*
+ * default_display.c: What is the default/boot PCI VGA device?
+ *
+ * (C) Copyright 2005 Benjamin Herrenschmidt <benh@kernel.crashing.org>
+ * (C) Copyright 2007 Paulo R. Zanoni <przanoni@gmail.com>
+ * (C) Copyright 2007, 2009 Tiago Vignatti <vignatti@freedesktop.org>
+ * (C) Copyright 2017 Canonical Ltd. (Author: Daniel Axtens <dja@axtens.net>)
+ *
+ * (License from vgaarb.c)
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+/**
+ * DOC: overview
+ *
+ * What device should a graphics system draw to?
+ *
+ * It is helpful to have a concept of a default or boot device (for
+ * example, for autoconfiguration where there is no device speficied
+ * by the user). That should be:
+ *
+ *  1) If the platform has a concept of a boot device for early boot
+ *     messages (think BIOS displays on x86), that device.
+ *
+ *  2) Anything specified by an arch hook,
+ *     e.g. arch/powerpc/kernel/pci-common.c::fixup_vga()
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/pci.h>
+#include <linux/init.h>
+
+#include <linux/default_display.h>
+
+static struct pci_dev *vga_default;
+
+/**
+ * pci_default_display - return the default display device
+ *
+ * This represents the default or boot device.
+ *
+ * The default implementation is rather dumb and will probably only
+ * work properly on single vga card setups and/or x86 platforms.
+ */
+struct pci_dev *pci_default_display(void)
+{
+	return vga_default;
+}
+EXPORT_SYMBOL_GPL(pci_default_display);
+
+/**
+ * pci_set_default_display - set the default display device
+ * @pdev: pci device to set as default
+ *
+ * Idempotent - safe to call with the same device repeatedly.
+ *
+ * Drops a reference to the old default, if applicable. Takes a
+ * reference to the new device.
+ */
+void pci_set_default_display(struct pci_dev *pdev)
+{
+	if (vga_default == pdev)
+		return;
+
+	pci_dev_put(vga_default);
+	vga_default = pci_dev_get(pdev);
+}
diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index 3cd153c6d271..beefefc288f9 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -41,7 +41,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/seq_file.h>
 #include <linux/uaccess.h>
-#include <linux/vgaarb.h>
+#include <linux/default_display.h>
 #include <linux/vga_switcheroo.h>
 
 /**
@@ -320,7 +320,7 @@ int vga_switcheroo_register_client(struct pci_dev *pdev,
 				   bool driver_power_control)
 {
 	return register_client(pdev, ops, VGA_SWITCHEROO_UNKNOWN_ID,
-			       pdev == vga_default_device(),
+			       pdev == pci_default_display(),
 			       driver_power_control);
 }
 EXPORT_SYMBOL(vga_switcheroo_register_client);
@@ -397,7 +397,7 @@ bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev)
 		 * apple-gmux is needed on pre-retina MacBook Pro
 		 * to probe the panel if pdev is the inactive GPU.
 		 */
-		if (apple_gmux_present() && pdev != vga_default_device() &&
+		if (apple_gmux_present() && pdev != pci_default_display() &&
 		    !vgasr_priv.handler_flags)
 			return true;
 	}
@@ -659,7 +659,7 @@ static int vga_switchto_stage1(struct vga_switcheroo_client *new_client)
 	if (new_client->pwr_state == VGA_SWITCHEROO_OFF)
 		vga_switchon(new_client);
 
-	vga_set_default_device(new_client->pdev);
+	pci_set_default_display(new_client->pdev);
 	return 0;
 }
 
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index 76875f6299b8..7654bb678587 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -51,6 +51,7 @@
 
 #include <linux/uaccess.h>
 
+#include <linux/default_display.h>
 #include <linux/vgaarb.h>
 
 static void vga_arbiter_notify_clients(void);
@@ -119,9 +120,6 @@ static int vga_str_to_iostate(char *buf, int str_size, int *io_state)
 	return 1;
 }
 
-/* this is only used a cookie - it should not be dereferenced */
-static struct pci_dev *vga_default;
-
 static void vga_arb_device_card_gone(struct pci_dev *pdev);
 
 /* Find somebody in our list */
@@ -135,39 +133,6 @@ static struct vga_device *vgadev_find(struct pci_dev *pdev)
 	return NULL;
 }
 
-/**
- * vga_default_device - return the default VGA device, for vgacon
- *
- * This can be defined by the platform. The default implementation
- * is rather dumb and will probably only work properly on single
- * vga card setups and/or x86 platforms.
- *
- * If your VGA default device is not PCI, you'll have to return
- * NULL here. In this case, I assume it will not conflict with
- * any PCI card. If this is not true, I'll have to define two archs
- * hooks for enabling/disabling the VGA default device if that is
- * possible. This may be a problem with real _ISA_ VGA cards, in
- * addition to a PCI one. I don't know at this point how to deal
- * with that card. Can theirs IOs be disabled at all ? If not, then
- * I suppose it's a matter of having the proper arch hook telling
- * us about it, so we basically never allow anybody to succeed a
- * vga_get()...
- */
-struct pci_dev *vga_default_device(void)
-{
-	return vga_default;
-}
-EXPORT_SYMBOL_GPL(vga_default_device);
-
-void vga_set_default_device(struct pci_dev *pdev)
-{
-	if (vga_default == pdev)
-		return;
-
-	pci_dev_put(vga_default);
-	vga_default = pci_dev_get(pdev);
-}
-
 static inline void vga_irq_set_state(struct vga_device *vgadev, bool state)
 {
 	if (vgadev->irq_set_state)
@@ -423,7 +388,7 @@ int vga_get(struct pci_dev *pdev, unsigned int rsrc, int interruptible)
 	vga_check_first_use();
 	/* The one who calls us should check for this, but lets be sure... */
 	if (pdev == NULL)
-		pdev = vga_default_device();
+		pdev = pci_default_display();
 	if (pdev == NULL)
 		return 0;
 
@@ -490,7 +455,7 @@ int vga_tryget(struct pci_dev *pdev, unsigned int rsrc)
 
 	/* The one who calls us should check for this, but lets be sure... */
 	if (pdev == NULL)
-		pdev = vga_default_device();
+		pdev = pci_default_display();
 	if (pdev == NULL)
 		return 0;
 	spin_lock_irqsave(&vga_lock, flags);
@@ -524,7 +489,7 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc)
 
 	/* The one who calls us should check for this, but lets be sure... */
 	if (pdev == NULL)
-		pdev = vga_default_device();
+		pdev = pci_default_display();
 	if (pdev == NULL)
 		return;
 	spin_lock_irqsave(&vga_lock, flags);
@@ -667,10 +632,10 @@ 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 &&
+	if (pci_default_display() == NULL &&
 	    ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) {
 		vgaarb_info(&pdev->dev, "setting as boot VGA device\n");
-		vga_set_default_device(pdev);
+		pci_set_default_display(pdev);
 	}
 
 	vga_arbiter_check_bridge_sharing(vgadev);
@@ -704,8 +669,8 @@ static bool vga_arbiter_del_pci_device(struct pci_dev *pdev)
 		goto bail;
 	}
 
-	if (vga_default == pdev)
-		vga_set_default_device(NULL);
+	if (pci_default_display() == pdev)
+		pci_set_default_display(NULL);
 
 	if (vgadev->decodes & (VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM))
 		vga_decode_count--;
@@ -1182,7 +1147,7 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf,
 		pr_debug("client 0x%p called 'target'\n", priv);
 		/* if target is default */
 		if (!strncmp(curr_pos, "default", 7))
-			pdev = pci_dev_get(vga_default_device());
+			pdev = pci_dev_get(pci_default_display());
 		else {
 			if (!vga_pci_str_to_vars(curr_pos, remaining,
 						 &domain, &bus, &devfn)) {
@@ -1292,7 +1257,7 @@ static int vga_arb_open(struct inode *inode, struct file *file)
 	spin_unlock_irqrestore(&vga_user_lock, flags);
 
 	/* Set the client' lists of locks */
-	priv->target = vga_default_device(); /* Maybe this is still null! */
+	priv->target = pci_default_display(); /* Maybe this is still null! */
 	priv->cards[0].pdev = priv->target;
 	priv->cards[0].io_cnt = 0;
 	priv->cards[0].mem_cnt = 0;
@@ -1455,11 +1420,11 @@ static int __init vga_arb_device_init(void)
 			if (screen_info.lfb_base < start || limit >= end)
 				continue;
 
-			if (!vga_default_device())
+			if (!pci_default_display())
 				vgaarb_info(dev, "setting as boot device\n");
-			else if (vgadev->pdev != vga_default_device())
+			else if (vgadev->pdev != pci_default_display())
 				vgaarb_info(dev, "overriding boot device\n");
-			vga_set_default_device(vgadev->pdev);
+			pci_set_default_display(vgadev->pdev);
 		}
 #endif
 		if (vgadev->bridge_has_one_vga)
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 1eecfa301f7f..31a37178fa8a 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -27,7 +27,7 @@
 #include <linux/security.h>
 #include <linux/pci-aspm.h>
 #include <linux/slab.h>
-#include <linux/vgaarb.h>
+#include <linux/default_display.h>
 #include <linux/pm_runtime.h>
 #include <linux/of.h>
 #include "pci.h"
@@ -785,7 +785,7 @@ static ssize_t boot_vga_show(struct device *dev, struct device_attribute *attr,
 			     char *buf)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
-	struct pci_dev *vga_dev = vga_default_device();
+	struct pci_dev *vga_dev = pci_default_display();
 
 	if (vga_dev)
 		return sprintf(buf, "%u\n", (pdev == vga_dev));
diff --git a/include/linux/default_display.h b/include/linux/default_display.h
new file mode 100644
index 000000000000..5ccebde96b6a
--- /dev/null
+++ b/include/linux/default_display.h
@@ -0,0 +1,44 @@
+/*
+ * default_display.h: What is the default/boot PCI VGA device?
+ *
+ * (C) Copyright 2005 Benjamin Herrenschmidt <benh@kernel.crashing.org>
+ * (C) Copyright 2007 Paulo R. Zanoni <przanoni@gmail.com>
+ * (C) Copyright 2007, 2009 Tiago Vignatti <vignatti@freedesktop.org>
+ * (C) Copyright 2017 Canonical Ltd. (Author: Daniel Axtens <dja@axtens.net>)
+ *
+ * (License from vgaarb.h)
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#ifndef LINUX_DEFAULT_DISPLAY_H
+#define LINUX_DEFAULT_DISPLAY_H
+
+struct pci_dev;
+
+#ifdef CONFIG_DEFAULT_DISPLAY
+extern struct pci_dev *pci_default_display(void);
+extern void pci_set_default_display(struct pci_dev *pdev);
+#else
+static inline struct pci_dev *pci_default_display(void) { return NULL; };
+static inline void pci_set_default_display(struct pci_dev *pdev) { };
+#endif
+
+#endif
diff --git a/include/linux/vgaarb.h b/include/linux/vgaarb.h
index ee162e3e879b..5434ad4af32d 100644
--- a/include/linux/vgaarb.h
+++ b/include/linux/vgaarb.h
@@ -42,12 +42,6 @@
 #define VGA_RSRC_NORMAL_IO     0x04
 #define VGA_RSRC_NORMAL_MEM    0x08
 
-/* Passing that instead of a pci_dev to use the system "default"
- * device, that is the one used by vgacon. Archs will probably
- * have to provide their own vga_default_device();
- */
-#define VGA_DEFAULT_DEVICE     (NULL)
-
 struct pci_dev;
 
 /* For use by clients */
@@ -121,15 +115,6 @@ extern void vga_put(struct pci_dev *pdev, unsigned int rsrc);
 #define vga_put(pdev, rsrc)
 #endif
 
-
-#ifdef CONFIG_VGA_ARB
-extern struct pci_dev *vga_default_device(void);
-extern void vga_set_default_device(struct pci_dev *pdev);
-#else
-static inline struct pci_dev *vga_default_device(void) { return NULL; };
-static inline void vga_set_default_device(struct pci_dev *pdev) { };
-#endif
-
 /*
  * Architectures should define this if they have several
  * independent PCI domains that can afford concurrent VGA
-- 
2.11.0

[PATCH v3 2/3] drm: add fallback default device detection

[Daniel Axtens]

The VGA arbiter selects a default VGA device that is enabled and
reachable via the legacy VGA resources (mem 0xa0000-0xbffff, io
0x3b0-0x3bb, io 0x3c0-0x3df, etc).

(As a special case for x86 and IA64, this can be overridden by
EFI.)

If there is no such device, e.g., because there's no enabled VGA
device, the host bridge doesn't support access to those legacy
resources, or a PCI-PCI bridge doesn't have VGA Enable set, a
platform may select an arbitrary device by calling
pci_set_default_display(). powerpc does this, for example.

If there is also no platform hook, there will be no default
device nominated. This is not necessarily what we want.

Add handling for devices that aren't handled by the vga arbiter or
platform by adding a late initcall and a class enable hook. If there
is no default from vgaarb or the platform then the first VGA card
that is enabled, has a driver bound, and can decode memory or I/O
will be marked as default.

This means single-card setups on systems without access to legacy
areas and without arch hooks will work. Multi-card setups on these
systems will nominate an arbitrary device, rather than no devices.

Signed-off-by: Daniel Axtens <dja@axtens.net>

---

v3:

Split out from re-organisation for simplicity.
Add better description and better documentaion.

Thanks to (in no particular order), Daniel Vetter, Lorenzo Pieralisi,
Ard Biesheuvel and Dave Airlie. Special thanks to Ben Herrenschmidt
and Bjorn Helgass, whose prose I have borrowed.

v1:

Tested on:
 - x86_64 laptop
 - arm64 D05 board with hibmc card
 - qemu powerpc with tcg and bochs std-vga

I know this adds another config option and that's a bit sad, but
we can't include it unconditionally as it depends on PCI.
Suggestions welcome.
---
 drivers/gpu/vga/default_display.c | 77 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/drivers/gpu/vga/default_display.c b/drivers/gpu/vga/default_display.c
index 99e4723360da..b8e4a5af38e8 100644
--- a/drivers/gpu/vga/default_display.c
+++ b/drivers/gpu/vga/default_display.c
@@ -42,6 +42,10 @@
  *
  *  2) Anything specified by an arch hook,
  *     e.g. arch/powerpc/kernel/pci-common.c::fixup_vga()
+ *
+ *  3) If neither of those, then we still want to pick something. For
+ *     now, pick the first PCI VGA device with a driver bound and with
+ *     memory or I/O control on.
  */
 
 #include <linux/module.h>
@@ -53,6 +57,12 @@
 
 static struct pci_dev *vga_default;
 
+/*
+ * only go active after the late initcall so as not to interfere with
+ * the arbiter
+ */
+static bool vga_default_active = false;
+
 /**
  * pci_default_display - return the default display device
  *
@@ -84,3 +94,70 @@ void pci_set_default_display(struct pci_dev *pdev)
 	pci_dev_put(vga_default);
 	vga_default = pci_dev_get(pdev);
 }
+
+static bool vga_default_try_device(struct pci_dev *pdev)
+{
+	u16 cmd;
+
+	/* Only deal with VGA class devices */
+	if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
+		return false;
+
+	/* Only deal with devices with drivers bound */
+	if (!pdev->driver)
+		return false;
+
+	/* Require I/O or memory control */
+	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
+	if (!(cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)))
+		return false;
+
+	dev_info(&pdev->dev, "vga_default: setting as default device\n");
+	pci_set_default_display(pdev);
+	return true;
+}
+
+static int __init vga_default_init(void)
+{
+	struct pci_dev *pdev;
+
+	vga_default_active = true;
+
+	if (pci_default_display())
+		return 0;
+
+	pdev = NULL;
+	while ((pdev =
+		pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
+			       PCI_ANY_ID, pdev)) != NULL) {
+		if (vga_default_try_device(pdev))
+			return 0;
+	}
+
+	return 0;
+}
+late_initcall(vga_default_init);
+
+/*
+ * A driver could be loaded much later than late_initcall, for example
+ * if it's in a module.
+ *
+ * We want to pick that up. However, we want to make sure this does
+ * not interfere with the arbiter - it should only activate if the
+ * arbiter has already had a chance to operate. To ensure this, we set
+ * vga_default_active in the late_initcall: as the vga arbiter is a
+ * subsys initcall, it is guaranteed to fire first.
+ */
+static void vga_default_enable_hook(struct pci_dev *pdev)
+{
+       if (!vga_default_active)
+	       return;
+
+       if (pci_default_display())
+               return;
+
+       vga_default_try_device(pdev);
+}
+DECLARE_PCI_FIXUP_CLASS_ENABLE(PCI_ANY_ID, PCI_ANY_ID,
+			       PCI_CLASS_DISPLAY_VGA, 8,
+			       vga_default_enable_hook)
-- 
2.11.0

Re: [PATCH v3 2/3] drm: add fallback default device detection

[Andrew Donnellan]

On 01/09/17 17:27, Daniel Axtens wrote:
> The VGA arbiter selects a default VGA device that is enabled and
> reachable via the legacy VGA resources (mem 0xa0000-0xbffff, io
> 0x3b0-0x3bb, io 0x3c0-0x3df, etc).
> 
> (As a special case for x86 and IA64, this can be overridden by
> EFI.)
> 
> If there is no such device, e.g., because there's no enabled VGA
> device, the host bridge doesn't support access to those legacy
> resources, or a PCI-PCI bridge doesn't have VGA Enable set, a
> platform may select an arbitrary device by calling
> pci_set_default_display(). powerpc does this, for example.
> 
> If there is also no platform hook, there will be no default
> device nominated. This is not necessarily what we want.
> 
> Add handling for devices that aren't handled by the vga arbiter or
> platform by adding a late initcall and a class enable hook. If there
> is no default from vgaarb or the platform then the first VGA card
> that is enabled, has a driver bound, and can decode memory or I/O
> will be marked as default.
> 
> This means single-card setups on systems without access to legacy
> areas and without arch hooks will work. Multi-card setups on these
> systems will nominate an arbitrary device, rather than no devices.
> 
> Signed-off-by: Daniel Axtens <dja@axtens.net>
> 
> ---
> 
> v3:
> 
> Split out from re-organisation for simplicity.
> Add better description and better documentaion.
> 
> Thanks to (in no particular order), Daniel Vetter, Lorenzo Pieralisi,
> Ard Biesheuvel and Dave Airlie. Special thanks to Ben Herrenschmidt
> and Bjorn Helgass, whose prose I have borrowed.
> 
> v1:
> 
> Tested on:
>   - x86_64 laptop
>   - arm64 D05 board with hibmc card
>   - qemu powerpc with tcg and bochs std-vga
> 
> I know this adds another config option and that's a bit sad, but
> we can't include it unconditionally as it depends on PCI.
> Suggestions welcome.

Tested on our iMac G5, the fallback handler doesn't fire (which should 
be correct).

Tested-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>

> +static void vga_default_enable_hook(struct pci_dev *pdev)
> +{
> +       if (!vga_default_active)
> +	       return;
> +
> +       if (pci_default_display())
> +               return;
> +
> +       vga_default_try_device(pdev);
> +}
> +DECLARE_PCI_FIXUP_CLASS_ENABLE(PCI_ANY_ID, PCI_ANY_ID,
> +			       PCI_CLASS_DISPLAY_VGA, 8,
> +			       vga_default_enable_hook)
> 

Looks like you have some spaces/tabs inconsistencies here.

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

[PATCH v3 3/3] drm: documentation for default display device

[Daniel Axtens]

We have refactored and extended this - document it.

Signed-off-by: Daniel Axtens <dja@axtens.net>
---
 Documentation/gpu/default_display.rst | 93 +++++++++++++++++++++++++++++++++++
 Documentation/gpu/index.rst           |  1 +
 2 files changed, 94 insertions(+)
 create mode 100644 Documentation/gpu/default_display.rst

diff --git a/Documentation/gpu/default_display.rst b/Documentation/gpu/default_display.rst
new file mode 100644
index 000000000000..3c190611564e
--- /dev/null
+++ b/Documentation/gpu/default_display.rst
@@ -0,0 +1,93 @@
+=======================
+Default Display Devices
+=======================
+
+Overview
+========
+
+.. kernel-doc:: drivers/gpu/vga/default_display.c
+		:doc: overview
+
+
+Why do we need this?
+====================
+
+The default device is used to set the ``boot_vga`` per-device sysfs
+file, which is used by user-space. Most notably, Xorg reads this file
+via libpciaccess in order to facilitate auto-configuration.
+
+
+History
+=======
+
+When the VGA arbiter was introduced, it would pick a default device on
+boot. As the arbiter exists to arbitrate access to legacy resources,
+it would only pick a card that could be accessed through legacy areas.
+(See the :doc:`vgaarbiter` documentation for more.)
+
+The arbiter was later extended on x86 and IA64 to consider the EFI
+framebuffer.
+
+This is all well and good if you have legacy resources or
+EFI. However, some systems do not have either of those. For example,
+on POWER8: [0]_
+
+ - There is no IO space at all
+
+ - We configure the 32-bit MMIO window to be around 3..4G (to avoid
+   overlapping with DMA space below it).
+
+So effectively, there is no path to the legacy areas.
+
+This means the VGA arbiter will not pick a default device on these
+platforms. So, on powerpc, a class hook was added to mark a default
+device (``arch/powerpc/kernel/pci-common.c``), independent of the
+arbiter.
+
+When this issue arose on an arm64 system, it was decided that a generic
+approach would be better than more special cases. Therefore, the
+default device selection was factored out, and it now operates using
+the priority list described in the Overview.
+
+API
+===
+
+Public
+------
+
+.. kernel-doc:: drivers/gpu/vga/default_display.c
+		:export:
+
+Private
+-------
+
+.. kernel-doc:: drivers/gpu/vga/default_display.c
+		:internal:
+
+Future Work
+===========
+
+There is no support for non-PCI VGA devices being marked as default.
+The following comment, extracted from an earlier version of
+:c:func:`pci_default_display()` might help:
+
+  If your VGA default device is not PCI, you'll have to override this
+  and return NULL here. In this case, I assume it will not conflict
+  with any PCI card. If this is not true, I'll have to define two
+  archs hooks for enabling/disabling the VGA default device if that
+  is possible. 
+
+  This may be a problem with real _ISA_ VGA cards, in addition to a
+  PCI one. I don't know at this point how to deal with that card. Can
+  theirs IOs be disabled at all ? If not, then I suppose it's a matter
+  of having the proper arch hook telling us about it, so we basically
+  never allow anybody to succeed a ``vga_get()``...
+
+Currently there is also no way to support non-VGA-class PCI devices as
+default display devices.
+
+
+References
+==========
+	   
+.. [0] https://www.spinics.net/lists/linux-pci/msg64142.html
diff --git a/Documentation/gpu/index.rst b/Documentation/gpu/index.rst
index 35d673bf9b56..8083d84f2334 100644
--- a/Documentation/gpu/index.rst
+++ b/Documentation/gpu/index.rst
@@ -16,6 +16,7 @@ Linux GPU Driver Developer's Guide
    tegra
    tinydrm
    vc4
+   default_display
    vga-switcheroo
    vgaarbiter
    bridge/dw-hdmi
-- 
2.11.0

Re: [PATCH v3 0/3] Split default display handling out from VGA arbiter

[Ard Biesheuvel]

On 1 September 2017 at 08:27, Daniel Axtens <dja@axtens.net> wrote:
> This patch set:
>
>  - splits the default display handling out from VGA arbiter, into its
>    own file and behind its own Kconfig option (and gives the functions
>    better names).
>
>  - adds extra detection of default devices. To be nominated, the vga
>    arbiter and platform hooks must not have nominated a default. A
>    card will then only be nominated if it has a driver attached and
>    has IO or memory decoding enabled.
>
>  - adds relevant documentation.
>
> The practical impact of this is improved X autoconfiguration on some
> arm64 systems.
>
> Changes in v3:
>
>  - Add documentation - thanks Daniel Vetter for pointing it out.
>
>  - Clarify explanations. Thanks to everyone for continuing to bear
>    with my incomplete understanding of PCI and provide some clarity.
>
>  - Split refactoring and adding functionality.
>
> Changes in v2: https://www.spinics.net/lists/linux-pci/msg64007.html
>
> Drop all the powerpc patches. [explanation snipped]
>
> v1: https://www.spinics.net/lists/linux-pci/msg63581.html
>

If we are all in agreement that fixing X is not an option, I think
this is a reasonable approach

Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Re: [PATCH v3 0/3] Split default display handling out from VGA arbiter

[Daniel Axtens]

Hi Ard,

> If we are all in agreement that fixing X is not an option, I think
> this is a reasonable approach

This did come up in discussion at some earlier point in one of the many
spins we've done of this - I don't remember if you brought it up or
someone else did - but my concern was this: If it were just X we would
be fine, but if we go down that path I'm worried about also having to
fix Mir/Wayland/whatever-the-new-exciting-display-server-is-this-week,
ad nauseum.

> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Thanks!

Regards,
Daniel

Re: [PATCH v3 0/3] Split default display handling out from VGA arbiter

[Daniel Axtens]

Hi all,

The merge window is well over by now - is there anything else I can do
to get this series to a mergeable state? I'm not particularly across the
rules for drm-misc, so please let me know if there are things I need to
be doing.

Regards,
Daniel

Daniel Axtens <dja@axtens.net> writes:

> This patch set:
>
>  - splits the default display handling out from VGA arbiter, into its
>    own file and behind its own Kconfig option (and gives the functions
>    better names).
>
>  - adds extra detection of default devices. To be nominated, the vga
>    arbiter and platform hooks must not have nominated a default. A
>    card will then only be nominated if it has a driver attached and
>    has IO or memory decoding enabled.
>
>  - adds relevant documentation.
>
> The practical impact of this is improved X autoconfiguration on some
> arm64 systems.
>
> Changes in v3:
>
>  - Add documentation - thanks Daniel Vetter for pointing it out.
>
>  - Clarify explanations. Thanks to everyone for continuing to bear
>    with my incomplete understanding of PCI and provide some clarity.
>
>  - Split refactoring and adding functionality.
>
> Changes in v2: https://www.spinics.net/lists/linux-pci/msg64007.html
>
> Drop all the powerpc patches. [explanation snipped]
>
> v1: https://www.spinics.net/lists/linux-pci/msg63581.html
>
> Regards,
> Daniel
>
> Daniel Axtens (3):
>   drm: split default display handler out of VGA arbiter
>   drm: add fallback default device detection
>   drm: documentation for default display device
>
>  Documentation/gpu/default_display.rst |  93 +++++++++++++++++++
>  Documentation/gpu/index.rst           |   1 +
>  arch/ia64/pci/fixup.c                 |   6 +-
>  arch/powerpc/kernel/pci-common.c      |   6 +-
>  arch/x86/pci/fixup.c                  |   6 +-
>  arch/x86/video/fbdev.c                |   4 +-
>  drivers/gpu/vga/Kconfig               |  12 +++
>  drivers/gpu/vga/Makefile              |   1 +
>  drivers/gpu/vga/default_display.c     | 163 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/vga/vga_switcheroo.c      |   8 +-
>  drivers/gpu/vga/vgaarb.c              |  61 +++----------
>  drivers/pci/pci-sysfs.c               |   4 +-
>  include/linux/default_display.h       |  44 +++++++++
>  include/linux/vgaarb.h                |  15 ----
>  14 files changed, 344 insertions(+), 80 deletions(-)
>  create mode 100644 Documentation/gpu/default_display.rst
>  create mode 100644 drivers/gpu/vga/default_display.c
>  create mode 100644 include/linux/default_display.h
>
> -- 
> 2.11.0

Re: [PATCH v3 0/3] Split default display handling out from VGA arbiter

[Daniel Vetter]

On Mon, Sep 18, 2017 at 7:49 AM, Daniel Axtens <dja@axtens.net> wrote:
> Hi all,
>
> The merge window is well over by now - is there anything else I can do
> to get this series to a mergeable state? I'm not particularly across the
> rules for drm-misc, so please let me know if there are things I need to
> be doing.

drm-misc is always open for feature merging, but the patch series
lacks full r-b by someone with clue on this stuff (i.e. probably not
me). Pls find someone who can do the in-depth review. Just an Ack
feels a bit thin.
-Daniel

>
> Regards,
> Daniel
>
> Daniel Axtens <dja@axtens.net> writes:
>
>> This patch set:
>>
>>  - splits the default display handling out from VGA arbiter, into its
>>    own file and behind its own Kconfig option (and gives the functions
>>    better names).
>>
>>  - adds extra detection of default devices. To be nominated, the vga
>>    arbiter and platform hooks must not have nominated a default. A
>>    card will then only be nominated if it has a driver attached and
>>    has IO or memory decoding enabled.
>>
>>  - adds relevant documentation.
>>
>> The practical impact of this is improved X autoconfiguration on some
>> arm64 systems.
>>
>> Changes in v3:
>>
>>  - Add documentation - thanks Daniel Vetter for pointing it out.
>>
>>  - Clarify explanations. Thanks to everyone for continuing to bear
>>    with my incomplete understanding of PCI and provide some clarity.
>>
>>  - Split refactoring and adding functionality.
>>
>> Changes in v2: https://www.spinics.net/lists/linux-pci/msg64007.html
>>
>> Drop all the powerpc patches. [explanation snipped]
>>
>> v1: https://www.spinics.net/lists/linux-pci/msg63581.html
>>
>> Regards,
>> Daniel
>>
>> Daniel Axtens (3):
>>   drm: split default display handler out of VGA arbiter
>>   drm: add fallback default device detection
>>   drm: documentation for default display device
>>
>>  Documentation/gpu/default_display.rst |  93 +++++++++++++++++++
>>  Documentation/gpu/index.rst           |   1 +
>>  arch/ia64/pci/fixup.c                 |   6 +-
>>  arch/powerpc/kernel/pci-common.c      |   6 +-
>>  arch/x86/pci/fixup.c                  |   6 +-
>>  arch/x86/video/fbdev.c                |   4 +-
>>  drivers/gpu/vga/Kconfig               |  12 +++
>>  drivers/gpu/vga/Makefile              |   1 +
>>  drivers/gpu/vga/default_display.c     | 163 ++++++++++++++++++++++++++++++++++
>>  drivers/gpu/vga/vga_switcheroo.c      |   8 +-
>>  drivers/gpu/vga/vgaarb.c              |  61 +++----------
>>  drivers/pci/pci-sysfs.c               |   4 +-
>>  include/linux/default_display.h       |  44 +++++++++
>>  include/linux/vgaarb.h                |  15 ----
>>  14 files changed, 344 insertions(+), 80 deletions(-)
>>  create mode 100644 Documentation/gpu/default_display.rst
>>  create mode 100644 drivers/gpu/vga/default_display.c
>>  create mode 100644 include/linux/default_display.h
>>
>> --
>> 2.11.0
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v3 0/3] Split default display handling out from VGA arbiter

[Bjorn Helgaas]

On Fri, Sep 01, 2017 at 05:27:41PM +1000, Daniel Axtens wrote:
> This patch set:
> 
>  - splits the default display handling out from VGA arbiter, into its
>    own file and behind its own Kconfig option (and gives the functions
>    better names).
> 
>  - adds extra detection of default devices. To be nominated, the vga
>    arbiter and platform hooks must not have nominated a default. A
>    card will then only be nominated if it has a driver attached and
>    has IO or memory decoding enabled.
> 
>  - adds relevant documentation.
> 
> The practical impact of this is improved X autoconfiguration on some
> arm64 systems.

I think I gave you bad advice about trying to separate the "default
device" idea from the VGA arbiter.

It is true that the "VGA arbiter" per se is related to routing the
legacy VGA resources, and the arbiter currently only selects a default
device if it finds a device to which those resources are routed.

We have some cases where we want to select a default device that may
not support the legacy VGA resources, or where they might not be
routed to the device:

  - systems where we match the EFI framebuffer address with a BAR, and
    select that device as default,

  - powerpc systems where there may be no host bridge window that maps
    to the legacy VGA resources,

  - your ARM64 systems where the default device may be behind a bridge
    that doesn't support legacy VGA routing (PCI_BRIDGE_CTL_VGA)

But I think trying to split the "default device" part out from the VGA
arbiter ends up being overkill and making things more complicated
instead of simpler.

Would something like the following work for you as well as the powerpc
case?  On powerpc, we already use vga_set_default_device() to select a
device that doesn't use legacy VGA resources, so maybe we can just do
the same on ARM64?

I suppose there might be wrinkles in how the arbiter deals with
multiple graphics devices on those systems, since I don't think it
identifies these devices that don't use the legacy resources, but it
seems like we live with whatever those on are powerpc and probably can
on ARM64 as well.


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..9df4802c5f04 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -1468,6 +1468,21 @@ 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\n");
+				vga_set_default_device(pdev);
+				break;
+			}
+		}
+	}
+
 	pr_info("loaded\n");
 	return rc;
 }

Re: [PATCH v3 0/3] Split default display handling out from VGA arbiter

[Daniel Axtens]

Hi Bjorn,

> But I think trying to split the "default device" part out from the VGA
> arbiter ends up being overkill and making things more complicated
> instead of simpler.
Fair enough.

> Would something like the following work for you as well as the powerpc
> case?  On powerpc, we already use vga_set_default_device() to select a
> device that doesn't use legacy VGA resources, so maybe we can just do
> the same on ARM64?

It looks good. I'll try to get some time on the test system to test it
and I'll pester my friends at IBM to give it a go as well.

> I suppose there might be wrinkles in how the arbiter deals with
> multiple graphics devices on those systems, since I don't think it
> identifies these devices that don't use the legacy resources, but it
> seems like we live with whatever those on are powerpc and probably can
> on ARM64 as well.

I would say so, yes.

Thanks for sticking with this!

Regards,
Daniel

>
>
> 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..9df4802c5f04 100644
> --- a/drivers/gpu/vga/vgaarb.c
> +++ b/drivers/gpu/vga/vgaarb.c
> @@ -1468,6 +1468,21 @@ 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\n");
> +				vga_set_default_device(pdev);
> +				break;
> +			}
> +		}
> +	}
> +
>  	pr_info("loaded\n");
>  	return rc;
>  }

Re: [PATCH v3 0/3] Split default display handling out from VGA arbiter

[Daniel Axtens]

Hi Bjorn,

Yes, this works:

Tested-by: Daniel Axtens <dja@axtens.net> # arm64, ppc64-qemu-tcg


Regards,
Daniel
> On Fri, Sep 01, 2017 at 05:27:41PM +1000, Daniel Axtens wrote:
>> This patch set:
>> 
>>  - splits the default display handling out from VGA arbiter, into its
>>    own file and behind its own Kconfig option (and gives the functions
>>    better names).
>> 
>>  - adds extra detection of default devices. To be nominated, the vga
>>    arbiter and platform hooks must not have nominated a default. A
>>    card will then only be nominated if it has a driver attached and
>>    has IO or memory decoding enabled.
>> 
>>  - adds relevant documentation.
>> 
>> The practical impact of this is improved X autoconfiguration on some
>> arm64 systems.
>
> I think I gave you bad advice about trying to separate the "default
> device" idea from the VGA arbiter.
>
> It is true that the "VGA arbiter" per se is related to routing the
> legacy VGA resources, and the arbiter currently only selects a default
> device if it finds a device to which those resources are routed.
>
> We have some cases where we want to select a default device that may
> not support the legacy VGA resources, or where they might not be
> routed to the device:
>
>   - systems where we match the EFI framebuffer address with a BAR, and
>     select that device as default,
>
>   - powerpc systems where there may be no host bridge window that maps
>     to the legacy VGA resources,
>
>   - your ARM64 systems where the default device may be behind a bridge
>     that doesn't support legacy VGA routing (PCI_BRIDGE_CTL_VGA)
>
> But I think trying to split the "default device" part out from the VGA
> arbiter ends up being overkill and making things more complicated
> instead of simpler.
>
> Would something like the following work for you as well as the powerpc
> case?  On powerpc, we already use vga_set_default_device() to select a
> device that doesn't use legacy VGA resources, so maybe we can just do
> the same on ARM64?
>
> I suppose there might be wrinkles in how the arbiter deals with
> multiple graphics devices on those systems, since I don't think it
> identifies these devices that don't use the legacy resources, but it
> seems like we live with whatever those on are powerpc and probably can
> on ARM64 as well.
>
>
> 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..9df4802c5f04 100644
> --- a/drivers/gpu/vga/vgaarb.c
> +++ b/drivers/gpu/vga/vgaarb.c
> @@ -1468,6 +1468,21 @@ 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\n");
> +				vga_set_default_device(pdev);
> +				break;
> +			}
> +		}
> +	}
> +
>  	pr_info("loaded\n");
>  	return rc;
>  }

Re: [PATCH v3 0/3] Split default display handling out from VGA arbiter

[Bjorn Helgaas]

On Wed, Sep 27, 2017 at 01:52:55PM +1000, Daniel Axtens wrote:
> Hi Bjorn,
> 
> Yes, this works:
> 
> Tested-by: Daniel Axtens <dja@axtens.net> # arm64, ppc64-qemu-tcg

I guess I was assuming you'd pick this up, but that doesn't really
make sense because I didn't give you a signed-off-by or anything.
I'll post this with a changelog and signed-off-by so it doesn't get
lost.

I also noticed that I didn't correctly handle the powerpc quirk case
where it doesn't require the device to be enabled at all.  I'll try to
fix that up, too.

Bjorn

> > On Fri, Sep 01, 2017 at 05:27:41PM +1000, Daniel Axtens wrote:
> >> This patch set:
> >> 
> >>  - splits the default display handling out from VGA arbiter, into its
> >>    own file and behind its own Kconfig option (and gives the functions
> >>    better names).
> >> 
> >>  - adds extra detection of default devices. To be nominated, the vga
> >>    arbiter and platform hooks must not have nominated a default. A
> >>    card will then only be nominated if it has a driver attached and
> >>    has IO or memory decoding enabled.
> >> 
> >>  - adds relevant documentation.
> >> 
> >> The practical impact of this is improved X autoconfiguration on some
> >> arm64 systems.
> >
> > I think I gave you bad advice about trying to separate the "default
> > device" idea from the VGA arbiter.
> >
> > It is true that the "VGA arbiter" per se is related to routing the
> > legacy VGA resources, and the arbiter currently only selects a default
> > device if it finds a device to which those resources are routed.
> >
> > We have some cases where we want to select a default device that may
> > not support the legacy VGA resources, or where they might not be
> > routed to the device:
> >
> >   - systems where we match the EFI framebuffer address with a BAR, and
> >     select that device as default,
> >
> >   - powerpc systems where there may be no host bridge window that maps
> >     to the legacy VGA resources,
> >
> >   - your ARM64 systems where the default device may be behind a bridge
> >     that doesn't support legacy VGA routing (PCI_BRIDGE_CTL_VGA)
> >
> > But I think trying to split the "default device" part out from the VGA
> > arbiter ends up being overkill and making things more complicated
> > instead of simpler.
> >
> > Would something like the following work for you as well as the powerpc
> > case?  On powerpc, we already use vga_set_default_device() to select a
> > device that doesn't use legacy VGA resources, so maybe we can just do
> > the same on ARM64?
> >
> > I suppose there might be wrinkles in how the arbiter deals with
> > multiple graphics devices on those systems, since I don't think it
> > identifies these devices that don't use the legacy resources, but it
> > seems like we live with whatever those on are powerpc and probably can
> > on ARM64 as well.
> >
> >
> > 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..9df4802c5f04 100644
> > --- a/drivers/gpu/vga/vgaarb.c
> > +++ b/drivers/gpu/vga/vgaarb.c
> > @@ -1468,6 +1468,21 @@ 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\n");
> > +				vga_set_default_device(pdev);
> > +				break;
> > +			}
> > +		}
> > +	}
> > +
> >  	pr_info("loaded\n");
> >  	return rc;
> >  }
> 
>