[PATCH v2 1/1] Split VGA default device handler out of VGA arbiter
Daniel Vetter
daniel at ffwll.ch
Fri Aug 18 07:38:35 UTC 2017
On Thu, Aug 17, 2017 at 09:30:28PM +1000, Daniel Axtens wrote:
> A system without PCI legacy resources (e.g. ARM64) may find that no
> default/boot VGA device has been marked, because the VGA arbiter
> checks for legacy resource decoding before marking a card as default.
>
> 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.
>
> Add handling for devices that should be marked as default but aren't
> handled by the vga arbiter by adding a late initcall and a class
> enable hook. If there is no default from vgaarb then the first card
> that is enabled, has a driver bound, and can decode memory or I/O
> will be marked as default.
>
> Signed-off-by: Daniel Axtens <dja at axtens.net>
Looks reasonable, but I have no clue at all about this. Can you pls get
some proper review from pci/platform folks (ppc would be good to)? I can
apply to drm-misc once that's done.
Just documentation comments below.
Thanks, Daniel
>
> ---
>
> v2: 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.
> ---
> arch/ia64/pci/fixup.c | 2 +-
> arch/powerpc/kernel/pci-common.c | 2 +-
> arch/x86/pci/fixup.c | 2 +-
> arch/x86/video/fbdev.c | 2 +-
> drivers/gpu/vga/Kconfig | 12 +++
> drivers/gpu/vga/Makefile | 1 +
> drivers/gpu/vga/vga_default.c | 159 +++++++++++++++++++++++++++++++++++++++
> drivers/gpu/vga/vga_switcheroo.c | 2 +-
> drivers/gpu/vga/vgaarb.c | 41 +---------
> drivers/pci/pci-sysfs.c | 2 +-
> include/linux/vga_default.h | 44 +++++++++++
> include/linux/vgaarb.h | 14 ----
> 12 files changed, 225 insertions(+), 58 deletions(-)
> create mode 100644 drivers/gpu/vga/vga_default.c
> create mode 100644 include/linux/vga_default.h
>
> diff --git a/arch/ia64/pci/fixup.c b/arch/ia64/pci/fixup.c
> index 41caa99add51..b35d1cf4501a 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/vga_default.h>
> #include <linux/screen_info.h>
>
> #include <asm/machvec.h>
> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> index 341a7469cab8..4fd890a51d18 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/vga_default.h>
>
> #include <asm/processor.h>
> #include <asm/io.h>
> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> index 11e407489db0..b1254bc09a45 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/vga_default.h>
> #include <asm/hpet.h>
> #include <asm/pci_x86.h>
>
> diff --git a/arch/x86/video/fbdev.c b/arch/x86/video/fbdev.c
> index 9fd24846d094..62cfa74ea86e 100644
> --- a/arch/x86/video/fbdev.c
> +++ b/arch/x86/video/fbdev.c
> @@ -9,7 +9,7 @@
> #include <linux/fb.h>
> #include <linux/pci.h>
> #include <linux/module.h>
> -#include <linux/vgaarb.h>
> +#include <linux/vga_default.h>
>
> int fb_is_primary_device(struct fb_info *info)
> {
> diff --git a/drivers/gpu/vga/Kconfig b/drivers/gpu/vga/Kconfig
> index 29437eabe095..81d4105aecf6 100644
> --- a/drivers/gpu/vga/Kconfig
> +++ b/drivers/gpu/vga/Kconfig
> @@ -1,3 +1,14 @@
> +config VGA_DEFAULT
> + bool "VGA Default Device Support" if EXPERT
> + default y
> + depends on PCI
> + help
> + Some programs find it helpful to know what VGA 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 VGA_DEFAULT
> 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..1e30f90d40fb 100644
> --- a/drivers/gpu/vga/Makefile
> +++ b/drivers/gpu/vga/Makefile
> @@ -1,2 +1,3 @@
> obj-$(CONFIG_VGA_ARB) += vgaarb.o
> +obj-$(CONFIG_VGA_DEFAULT) += vga_default.o
> obj-$(CONFIG_VGA_SWITCHEROO) += vga_switcheroo.o
> diff --git a/drivers/gpu/vga/vga_default.c b/drivers/gpu/vga/vga_default.c
> new file mode 100644
> index 000000000000..f6fcb0eb1507
> --- /dev/null
> +++ b/drivers/gpu/vga/vga_default.c
> @@ -0,0 +1,159 @@
> +/*
> + * vga_default.c: What is the default/boot PCI VGA device?
> + *
> + * (C) Copyright 2005 Benjamin Herrenschmidt <benh at kernel.crashing.org>
> + * (C) Copyright 2007 Paulo R. Zanoni <przanoni at gmail.com>
> + * (C) Copyright 2007, 2009 Tiago Vignatti <vignatti at freedesktop.org>
> + * (C) Copyright 2017 Canonical Ltd. (Author: Daniel Axtens <dja at 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.
> + */
> +
> +/*
Please make this into a DOC: introduction section.
> + * What device should a graphics system draw to? In order of priority:
> + *
> + * 1) Any devices configured specifically by the user (think
> + * xorg.conf).
> + *
> + * 2) If the platform has a concept of a boot device for early boot
> + * messages (think BIOS displays on x86), that device.
> + *
> + * 3) If the platform does not have the concept of a boot device,
> + * 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>
> +#include <linux/kernel.h>
> +#include <linux/pci.h>
> +#include <linux/init.h>
> +
> +#include <linux/vga_default.h>
> +
> +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;
> +
> +/**
> + * vga_default_device - return the default VGA device
All the nice kerneldoc, but not pulled into Documentation/gpu. Can you pls
fix that and make sure the resulting docs look pretty and have all the
links still working?
> + *
> + * 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()...
I'd like reviewers to convert this lore into a solid statement of fact :-)
> + */
> +
> +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 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");
> + vga_set_default_device(pdev);
> + return true;
> +}
> +
> +static int __init vga_default_init(void)
> +{
> + struct pci_dev *pdev;
> +
> + vga_default_active = true;
> +
> + if (vga_default_device())
> + 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 (vga_default_device())
> + 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)
> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> index 3cd153c6d271..6612ec7981b6 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/vga_default.h>
> #include <linux/vga_switcheroo.h>
>
> /**
> diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> index 76875f6299b8..74683286f5f8 100644
> --- a/drivers/gpu/vga/vgaarb.c
> +++ b/drivers/gpu/vga/vgaarb.c
> @@ -51,6 +51,7 @@
>
> #include <linux/uaccess.h>
>
> +#include <linux/vga_default.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)
> @@ -667,7 +632,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 &&
> + if (vga_default_device() == 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);
> @@ -704,7 +669,7 @@ static bool vga_arbiter_del_pci_device(struct pci_dev *pdev)
> goto bail;
> }
>
> - if (vga_default == pdev)
> + if (vga_default_device() == pdev)
> vga_set_default_device(NULL);
>
> if (vgadev->decodes & (VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM))
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 2f3780b50723..c174b427ea2b 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/vga_default.h>
> #include <linux/pm_runtime.h>
> #include <linux/of.h>
> #include "pci.h"
> diff --git a/include/linux/vga_default.h b/include/linux/vga_default.h
> new file mode 100644
> index 000000000000..78df6a2a194c
> --- /dev/null
> +++ b/include/linux/vga_default.h
> @@ -0,0 +1,44 @@
> +/*
> + * vga_default.h: What is the default/boot PCI VGA device?
> + *
> + * (C) Copyright 2005 Benjamin Herrenschmidt <benh at kernel.crashing.org>
> + * (C) Copyright 2007 Paulo R. Zanoni <przanoni at gmail.com>
> + * (C) Copyright 2007, 2009 Tiago Vignatti <vignatti at freedesktop.org>
> + * (C) Copyright 2017 Canonical Ltd. (Author: Daniel Axtens <dja at 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_VGA_DEFAULT_H
> +#define LINUX_VGA_DEFAULT_H
> +
> +struct pci_dev;
> +
> +#ifdef CONFIG_VGA_DEFAULT
> +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
> +
> +#endif
> diff --git a/include/linux/vgaarb.h b/include/linux/vgaarb.h
> index ee162e3e879b..953e1955efbe 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 */
> @@ -122,14 +116,6 @@ extern void vga_put(struct pci_dev *pdev, unsigned int 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
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the dri-devel
mailing list