[PATCH] VGA arbitration: draft of kernel side

[PATCH] VGA arbitration: draft of kernel side

[Benjamin Herrenschmidt]

Hi !

Ok, so here is a first, totally untested draft for the kernel side
of the VGA arbiter.

BIG NOTE: It's really only the basic arbiter itself, which provides the
API I drafted a bit earlier, with arch hooks similar to what Alan proposed,
it does _NOT_ yet provides a userland interface (to a library providing the
same API hopefully).

It's totally untested (just tested that it builds on ppc32). Some
refinements from my original ideas is the notion of "default" VGA device
which is for vgacon (which would then be able to pass "NULL" to
vga_get/tryget/put, which is handy since current vgacon has no idea of
what a PCI device is). Possible room for a non-PCI device there, but I
haven't completely decided how to deal with that case (see comments in
the code).

Note about vgacon (or vesafb for what matters): I haven't yet looked at
what is involved in getting those adapted to the arbiter. As far as vgacon
is concerned, I strongly suspect that we'll need a way for a console write
to return -EAGAIN, telling to the printk core to come back later (either on
the next printk or from schedul'able time). That shouldn't be too difficult
tho. vesafb may need additional massaging too.

I don't know how much time I'll have on my hands to more forward with
adapting vgacon and doing a userland interface, I expect to be fairly busy
the rest of the week, so if any of you feel like picking up from there,
just let me know.

Ben.

Index: linux-work/include/linux/pci.h
===================================================================
--- linux-work.orig/include/linux/pci.h	2005-01-24 17:09:57.000000000 +1100
+++ linux-work/include/linux/pci.h	2005-03-08 15:26:25.000000000 +1100
@@ -1064,5 +1064,6 @@
 #define PCIPCI_VSFX		16
 #define PCIPCI_ALIMAGIK		32
 
+
 #endif /* __KERNEL__ */
 #endif /* LINUX_PCI_H */
Index: linux-work/drivers/pci/Makefile
===================================================================
--- linux-work.orig/drivers/pci/Makefile	2005-01-24 17:09:38.000000000 +1100
+++ linux-work/drivers/pci/Makefile	2005-03-08 15:20:23.000000000 +1100
@@ -4,7 +4,7 @@
 
 obj-y		+= access.o bus.o probe.o remove.o pci.o quirks.o \
 			names.o pci-driver.o search.o pci-sysfs.o \
-			rom.o
+			rom.o vga.o
 obj-$(CONFIG_PROC_FS) += proc.o
 
 ifndef CONFIG_SPARC64
Index: linux-work/drivers/pci/remove.c
===================================================================
--- linux-work.orig/drivers/pci/remove.c	2005-01-24 17:09:38.000000000 +1100
+++ linux-work/drivers/pci/remove.c	2005-03-08 15:25:52.000000000 +1100
@@ -26,6 +26,7 @@
 
 static void pci_destroy_dev(struct pci_dev *dev)
 {
+	vga_arbiter_del_pci_device(pdev);
 	pci_proc_detach_device(dev);
 	pci_remove_sysfs_dev_files(dev);
 	device_unregister(&dev->dev);
Index: linux-work/drivers/pci/pci.h
===================================================================
--- linux-work.orig/drivers/pci/pci.h	2005-01-24 17:09:38.000000000 +1100
+++ linux-work/drivers/pci/pci.h	2005-03-08 15:26:51.000000000 +1100
@@ -88,3 +88,7 @@
 	return NULL;
 }
 
+
+/* VGA arbiter functions */
+extern void vga_arbiter_add_pci_device(struct pci_dev *pdev);
+extern void vga_arbiter_del_pci_device(struct pci_dev *pdev);
Index: linux-work/drivers/pci/vga.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-work/drivers/pci/vga.c	2005-03-08 18:04:57.000000000 +1100
@@ -0,0 +1,403 @@
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/pci.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/list.h>
+#include <linux/sched.h>
+#include <linux/wait.h>
+#include <linux/spinlock.h>
+#include <linux/vga.h>
+
+#include "pci.h"
+
+/*
+ * We keep a list of all vga devices in the system to speed
+ * up the various operations of the arbiter
+ */
+struct vga_device
+{
+	struct list_head	list;
+	struct pci_dev		*pdev;
+	unsigned int		decodes;	/* what does it decodes */
+	unsigned int		owns;		/* what does it owns */
+	unsigned int		locks;		/* what does it locks */
+};
+
+static LIST_HEAD(		vga_list);
+static spinlock_t	       	vga_lock;
+static DECLARE_WAIT_QUEUE_HEAD(	vga_wait_queue);
+
+#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
+static struct pci_dev		*vga_default;
+#endif
+
+
+/* Find somebody in our list */
+static struct vga_device *vgadev_find(struct pci_dev *pdev)
+{
+	struct vga_device *vgadev;
+
+	list_for_each_entry(vgadev, &vga_list, list)
+		if (pdev == vgadev->pdev)
+			return vgadev;
+	return NULL;
+}
+
+/* Returns the default VGA device (vgacon's babe) */
+#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
+struct pci_dev *vga_default_device(void)
+{
+	return vga_default;
+}
+#endif
+
+/* Architecture can override enabling/disabling of a given
+ * device resources here
+ */
+#ifndef __ARCH_HAS_VGA_DISABLE_RESOURCES
+static inline void vga_disable_resources(struct pci_dev *pdev,
+					 unsigned int rsrc,
+					 unsigned int change_bridge)
+{
+	struct pci_bus *bus;
+	struct pci_dev *bridge;
+	u16 cmd;
+
+	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
+	if (rsrc & VGA_RSRC_IO)
+		cmd &= ~PCI_COMMAND_IO;
+	if (rsrc & VGA_RSRC_MEM)
+		cmd &= ~PCI_COMMAND_MEMORY;
+	pci_write_config_word(pdev, PCI_COMMAND, cmd);
+
+	if (!change_bridge)
+		return;
+
+	bus = pdev->bus;
+	while (bus) {
+		bridge = bus->self;
+		if (bridge) {
+			pci_read_config_word(bridge, PCI_BRIDGE_CONTROL, &cmd);
+			if (!(cmd & PCI_BRIDGE_CTL_VGA))
+				continue;
+			cmd &= ~PCI_BRIDGE_CTL_VGA;
+			pci_write_config_word(bridge, PCI_BRIDGE_CONTROL, cmd);
+		}
+		bus = bus->parent;
+	}
+}
+#endif
+
+#ifndef __ARCH_HAS_VGA_ENABLE_RESOURCES
+static inline void vga_enable_resources(struct pci_dev *pdev,
+					 unsigned int rsrc)
+{
+	struct pci_bus *bus;
+	struct pci_dev *bridge;
+	u16 cmd;
+
+	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
+	if (rsrc & VGA_RSRC_IO)
+		cmd |= PCI_COMMAND_IO;
+	if (rsrc & VGA_RSRC_MEM)
+		cmd |= PCI_COMMAND_MEMORY;
+	pci_write_config_word(pdev, PCI_COMMAND, cmd);
+
+	bus = pdev->bus;
+	while (bus) {
+		bridge = bus->self;
+		if (bridge) {
+			pci_read_config_word(bridge, PCI_BRIDGE_CONTROL, &cmd);
+			if (cmd & PCI_BRIDGE_CTL_VGA)
+				continue;
+			cmd |= PCI_BRIDGE_CTL_VGA;
+			pci_write_config_word(bridge, PCI_BRIDGE_CONTROL, cmd);
+		}
+		bus = bus->parent;
+	}
+}
+#endif
+
+static struct vga_device * __vga_tryget(struct vga_device *vgadev,
+					unsigned int rsrc)
+{
+	unsigned int wants, match;
+	struct vga_device *conflict;
+
+	/* Check what resources we need to acquire */
+	wants = rsrc & !vgadev->owns;
+
+	/* We already own everything, just mark locked & bye bye */
+	if (wants == 0)
+		goto lock_them;
+
+	/* Ok, we don't, let's find out how we need to kick off */
+	list_for_each_entry(conflict, &vga_list, list) {
+		unsigned int lwants = wants;
+		unsigned int change_bridge = 0;
+
+		/* Check if the architecture allows a conflict between those
+		 * 2 devices or if they are on separate domains
+		 */
+		if (!vga_conflicts(vgadev->pdev, conflict->pdev))
+			continue;
+
+		/* We have a possible conflict. before we go further, we must
+		 * check if we sit on the same bus as the conflicting device.
+		 * if we don't, then we must tie both IO and MEM resources
+		 * together since there is only a single bit controlling
+		 * VGA forwarding on P2P bridges
+		 */
+		if (vgadev->pdev->bus != conflict->pdev->bus) {
+			change_bridge = 1;
+			lwants = VGA_RSRC_IO | VGA_RSRC_MEM;
+		}
+
+		/* Check if the guy has a lock on the resource. If he does,
+		 * return the conflicting entry
+		 */
+		if (conflict->locks & lwants)
+			return conflict;
+
+		/* Ok, now check if he owns the resource we want. We don't need
+		 * to check "decodes" since it should be impossible to have
+		 * own resources you don't decode unless I have a bug in this
+		 * code...
+		 */
+		WARN_ON(conflict->owns & !conflict->decodes);
+		match = lwants & conflict->owns;
+		if (!match)
+			continue;
+
+		/* looks like he doesn't have a lock, we can steal
+		 * them from him
+		 */
+		vga_disable_resources(conflict->pdev, lwants, change_bridge);
+		conflict->owns &= ~lwants;
+	}
+
+	/* ok dude, we got it, everybody has been disabled, let's
+	 * enable us. Make sure we don't mark a bit in "owns" that
+	 * we don't also have in "decodes". We can lock resources
+	 * we don't decode but not own them.
+	 */
+	vga_enable_resources(vgadev->pdev, wants);
+	vgadev->owns |= (wants & vgadev->decodes);
+ lock_them:
+	vgadev->locks |= rsrc;
+
+	return NULL;
+}
+
+static void __vga_put(struct vga_device *vgadev, unsigned int rsrc)
+{
+	/* Just clear lock bits, we do lazy operations so we don't really
+	 * have to bother about anything else at this point
+	 */
+	vgadev->locks &= ~rsrc;
+
+	/* Kick the wait queue in case somebody was waiting */
+	wake_up_all(&vga_wait_queue);
+}
+
+int vga_get(struct pci_dev *pdev, unsigned int rsrc)
+{
+	struct vga_device *vgadev, *conflict;
+	unsigned long flags;
+        wait_queue_t wait;
+	int rc = 0;
+
+	if (pdev == NULL)
+		pdev = vga_default_device();
+	if (pdev == NULL)
+		return 0;
+	spin_lock_irqsave(&vga_lock, flags);
+	vgadev = vgadev_find(pdev);
+	if (vgadev == NULL) {
+		rc = -ENODEV;
+		goto bail;
+	}
+	for (;;) {
+		conflict = __vga_tryget(vgadev, rsrc);
+		if (conflict == NULL)
+			break;
+
+		/* We have a conflict, we wait until somebody kicks the
+		 * work queue. Currently we have one work queue that we
+		 * kick each time some resources are released, but it would
+		 * be fairly easy to have a per device one so that we only
+		 * need to attach to the conflicting device
+		 */
+		init_waitqueue_entry(&wait, current);
+		add_wait_queue(&vga_wait_queue, &wait);
+		set_current_state(TASK_INTERRUPTIBLE);
+		if (signal_pending(current)) {
+			rc = -EINTR;
+			break;
+		}
+		schedule();
+		remove_wait_queue(&vga_wait_queue, &wait);
+		set_current_state(TASK_RUNNING);
+	}
+ bail:
+	spin_unlock_irqrestore(&vga_lock, flags);
+	return rc;
+}
+
+int vga_tryget(struct pci_dev *pdev, unsigned int rsrc)
+{
+	struct vga_device *vgadev;
+	unsigned long flags;
+	int rc = 0;
+
+	if (pdev == NULL)
+		pdev = vga_default_device();
+	if (pdev == NULL)
+		return 0;
+	spin_lock_irqsave(&vga_lock, flags);
+	vgadev = vgadev_find(pdev);
+	if (vgadev == NULL) {
+		rc = -ENODEV;
+		goto bail;
+	}
+	if (__vga_tryget(vgadev, rsrc))
+		rc = -EBUSY;
+ bail:
+	spin_unlock_irqrestore(&vga_lock, flags);
+	return rc;
+}
+
+void vga_put(struct pci_dev *pdev, unsigned int rsrc)
+{
+	struct vga_device *vgadev;
+	unsigned long flags;
+
+	if (pdev == NULL)
+		pdev = vga_default_device();
+	if (pdev == NULL)
+		return;
+	spin_lock_irqsave(&vga_lock, flags);
+	vgadev = vgadev_find(pdev);
+	if (vgadev == NULL)
+		goto bail;
+	__vga_put(vgadev, rsrc);
+ bail:
+	spin_unlock_irqrestore(&vga_lock, flags);
+}
+
+
+/*
+ * Currently, we assume that the "initial" setup of the system is
+ * sane, that is we don't come up with conflicting devices, which
+ * would be annoying. We could double check and be better at
+ * deciding who is the default here, but we don't. 
+ */
+void vga_arbiter_add_pci_device(struct pci_dev *pdev)
+{
+	struct vga_device *vgadev;
+	unsigned long flags;
+	struct pci_bus *bus;
+	struct pci_dev *bridge;
+	u16 cmd;
+
+	/* Only deal with VGA class devices */
+	if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
+		return;
+
+	/* Allocate structure */
+	vgadev = kmalloc(sizeof(struct vga_device), GFP_KERNEL);
+	memset(vgadev, 0, sizeof(*vgadev));
+
+	/* Take lock & check for duplicates */
+	spin_lock_irqsave(&vga_lock, flags);
+	if (vgadev_find(pdev) != NULL) {
+		WARN_ON(1);
+		goto fail;
+	}
+	vgadev->pdev = pdev;
+
+	/* By default, assume we decode everything */
+	vgadev->decodes = VGA_RSRC_IO | VGA_RSRC_MEM;
+
+	/* Mark that we "own" resources based on our enables, we will
+	 * clear that below if the bridge isn't forwarding
+	 */
+	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
+	if (cmd & PCI_COMMAND_IO)
+		vgadev->owns |= VGA_RSRC_IO;
+	if (cmd & PCI_COMMAND_MEMORY)
+		vgadev->owns |= VGA_RSRC_MEM;
+
+	/* Check if VGA cycles can get down to us */
+	bus = pdev->bus;
+	while (bus) {
+		bridge = bus->self;
+		if (bridge) {
+			u16 l;
+			pci_read_config_word(bridge, PCI_BRIDGE_CONTROL, &l);
+			if (!(l & PCI_BRIDGE_CTL_VGA)) {
+				vgadev->owns = 0;
+				break;
+			}
+		}
+		bus = bus->parent;
+	}
+
+	/* Deal with VGA default device. Use first enabled one
+	 * by default if arch doesn't have it's own hook
+	 */
+#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
+	if (vga_default == NULL && (vgadev->owns & VGA_RSRC_MEM) &&
+	    (vgadev->owns & VGA_RSRC_IO))
+		vga_default = pdev;
+
+#endif
+
+	/* Add to the list */
+	list_add(&vgadev->list, &vga_list);
+	spin_unlock_irqrestore(&vga_lock, flags);
+ fail:
+	spin_unlock_irqrestore(&vga_lock, flags);
+	kfree(vgadev);
+}
+
+void vga_arbiter_del_pci_device(struct pci_dev *pdev)
+{
+	struct vga_device *vgadev;
+	unsigned long flags;
+
+	spin_lock_irqsave(&vga_lock, flags);
+	vgadev = vgadev_find(pdev);
+	if (vgadev == NULL)
+		goto bail;
+	if (vgadev->locks)
+		__vga_put(vgadev, vgadev->locks);
+	list_del(&vgadev->list);
+ bail:
+	spin_unlock_irqrestore(&vga_lock, flags);
+	if (vgadev)
+		kfree(vgadev);
+}
+
+void vga_set_legacy_decoding(struct pci_dev *pdev, unsigned int decodes)
+{
+	struct vga_device *vgadev;
+	unsigned long flags;
+
+	spin_lock_irqsave(&vga_lock, flags);
+	vgadev = vgadev_find(pdev);
+	if (vgadev == NULL)
+		goto bail;
+
+	vgadev->decodes = decodes;
+	vgadev->owns &= decodes;
+
+	/* XXX if somebody is going from "doesn't decode" to "decodes" state
+	 * here, additional care must be taken. Basically, we probably want
+	 * to disable it preventively in that case, but I have to make sure
+	 * of that...
+	 */
+ bail:
+	spin_unlock_irqrestore(&vga_lock, flags);
+}
Index: linux-work/include/linux/vga.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-work/include/linux/vga.h	2005-03-08 18:03:40.000000000 +1100
@@ -0,0 +1,62 @@
+/*
+ *	vga.h
+ *
+ *	VGA Arbiter functions
+ *
+ *	Copyright 2005 Benjamin Herrenschmidt
+ *	               <benh@kernel.crashing.org>
+ *
+ */
+
+#ifndef LINUX_VGA_H
+
+#include <asm/vga.h>
+
+#define VGA_RSRC_IO	0x01
+#define VGA_RSRC_MEM	0x02
+
+/* 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)
+
+/* For use by clients */
+
+extern void vga_set_legacy_decoding(struct pci_dev *pdev,
+				    unsigned int decodes);
+extern int vga_get(struct pci_dev *pdev, unsigned int rsrc);
+extern int vga_tryget(struct pci_dev *pdev, unsigned int rsrc);
+extern void vga_put(struct pci_dev *pdev, unsigned int rsrc);
+
+
+/* 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()...
+ */
+#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
+extern struct pci_dev *vga_default_device(void);
+#endif
+
+/* Architectures should define this if they have several
+ * independant PCI domains that can afford concurrent VGA decoding
+ */
+#ifndef __ARCH_HAS_VGA_CONFLICT
+static inline int vga_conflicts(struct pci_dev *p1, struct pci_dev *p2)
+{
+	return 1;
+}
+#endif
+
+#endif /* LINUX_VGA_H */

Re: [PATCH] VGA arbitration: draft of kernel side

[Kronos]

Benjamin Herrenschmidt <benh@kernel.crashing.org> ha scritto:
> Ok, so here is a first, totally untested draft for the kernel side
> of the VGA arbiter.

Hi Ben,
I've a few comments:

> Index: linux-work/drivers/pci/vga.c
> ===================================================================
> --- /dev/null   1970-01-01 00:00:00.000000000 +0000
> +++ linux-work/drivers/pci/vga.c        2005-03-08 18:04:57.000000000 +1100
[...]
> +#ifndef __ARCH_HAS_VGA_DISABLE_RESOURCES
> +static inline void vga_disable_resources(struct pci_dev *pdev,
> +                                        unsigned int rsrc,
> +                                        unsigned int change_bridge)
> +{
> +       struct pci_bus *bus;
> +       struct pci_dev *bridge;
> +       u16 cmd;
> +
> +       pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> +       if (rsrc & VGA_RSRC_IO)
> +               cmd &= ~PCI_COMMAND_IO;
> +       if (rsrc & VGA_RSRC_MEM)
> +               cmd &= ~PCI_COMMAND_MEMORY;
> +       pci_write_config_word(pdev, PCI_COMMAND, cmd);
> +
> +       if (!change_bridge)
> +               return;
> +
> +       bus = pdev->bus;
> +       while (bus) {
> +               bridge = bus->self;
> +               if (bridge) {
> +                       pci_read_config_word(bridge, PCI_BRIDGE_CONTROL, &cmd);
> +                       if (!(cmd & PCI_BRIDGE_CTL_VGA))
> +                               continue;

This seems wrong: if the condition is true the loop will restart with
the same bus device and will never stop. I think you should do:

                        if (cmd & PCI_BRIDGE_CTL_VGA) {
                                cmd &= ~PCI_BRIDGE_CTL_VGA;
                                pci_write_config_word(bridge, PCI_BRIDGE_CONTROL, cmd);
                        }

> +                       cmd &= ~PCI_BRIDGE_CTL_VGA;
> +                       pci_write_config_word(bridge, PCI_BRIDGE_CONTROL, cmd);
> +               }
> +               bus = bus->parent;
> +       }
> +}
> +#endif
> +
> +#ifndef __ARCH_HAS_VGA_ENABLE_RESOURCES
> +static inline void vga_enable_resources(struct pci_dev *pdev,
> +                                        unsigned int rsrc)
> +{
> +       struct pci_bus *bus;
> +       struct pci_dev *bridge;
> +       u16 cmd;
> +
> +       pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> +       if (rsrc & VGA_RSRC_IO)
> +               cmd |= PCI_COMMAND_IO;
> +       if (rsrc & VGA_RSRC_MEM)
> +               cmd |= PCI_COMMAND_MEMORY;
> +       pci_write_config_word(pdev, PCI_COMMAND, cmd);
> +
> +       bus = pdev->bus;
> +       while (bus) {
> +               bridge = bus->self;
> +               if (bridge) {
> +                       pci_read_config_word(bridge, PCI_BRIDGE_CONTROL, &cmd);
> +                       if (cmd & PCI_BRIDGE_CTL_VGA)
> +                               continue;

Same here.

> +                       cmd |= PCI_BRIDGE_CTL_VGA;
> +                       pci_write_config_word(bridge, PCI_BRIDGE_CONTROL, cmd);
> +               }
> +               bus = bus->parent;
> +       }
> +}
> +#endif

> +/*
> + * Currently, we assume that the "initial" setup of the system is
> + * sane, that is we don't come up with conflicting devices, which
> + * would be annoying. We could double check and be better at
> + * deciding who is the default here, but we don't. 
> + */
> +void vga_arbiter_add_pci_device(struct pci_dev *pdev)
> +{
> +       struct vga_device *vgadev;
> +       unsigned long flags;
> +       struct pci_bus *bus;
> +       struct pci_dev *bridge;
> +       u16 cmd;
> +
> +       /* Only deal with VGA class devices */
> +       if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
> +               return;
> +
> +       /* Allocate structure */
> +       vgadev = kmalloc(sizeof(struct vga_device), GFP_KERNEL);

Not checking return value of kmalloc, this is evil :P
Also it may be worth to change return type in order to signal the error.

> +       memset(vgadev, 0, sizeof(*vgadev));
> +
> +       /* Take lock & check for duplicates */
> +       spin_lock_irqsave(&vga_lock, flags);
> +       if (vgadev_find(pdev) != NULL) {
> +               WARN_ON(1);
> +               goto fail;
> +       }
> +       vgadev->pdev = pdev;
> +
> +       /* By default, assume we decode everything */
> +       vgadev->decodes = VGA_RSRC_IO | VGA_RSRC_MEM;
> +
> +       /* Mark that we "own" resources based on our enables, we will
> +        * clear that below if the bridge isn't forwarding
> +        */
> +       pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> +       if (cmd & PCI_COMMAND_IO)
> +               vgadev->owns |= VGA_RSRC_IO;
> +       if (cmd & PCI_COMMAND_MEMORY)
> +               vgadev->owns |= VGA_RSRC_MEM;
> +
> +       /* Check if VGA cycles can get down to us */
> +       bus = pdev->bus;
> +       while (bus) {
> +               bridge = bus->self;
> +               if (bridge) {
> +                       u16 l;
> +                       pci_read_config_word(bridge, PCI_BRIDGE_CONTROL, &l);
> +                       if (!(l & PCI_BRIDGE_CTL_VGA)) {
> +                               vgadev->owns = 0;
> +                               break;
> +                       }
> +               }
> +               bus = bus->parent;
> +       }
> +
> +       /* Deal with VGA default device. Use first enabled one
> +        * by default if arch doesn't have it's own hook
> +        */
> +#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
> +       if (vga_default == NULL && (vgadev->owns & VGA_RSRC_MEM) &&
> +           (vgadev->owns & VGA_RSRC_IO))
> +               vga_default = pdev;
> +
> +#endif
> +
> +       /* Add to the list */
> +       list_add(&vgadev->list, &vga_list);
> +       spin_unlock_irqrestore(&vga_lock, flags);

Missing return?

> + fail:
> +       spin_unlock_irqrestore(&vga_lock, flags);
> +       kfree(vgadev);
> +}
> +
> +void vga_arbiter_del_pci_device(struct pci_dev *pdev)
> +{
> +       struct vga_device *vgadev;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&vga_lock, flags);
> +       vgadev = vgadev_find(pdev);
> +       if (vgadev == NULL)
> +               goto bail;
> +       if (vgadev->locks)
> +               __vga_put(vgadev, vgadev->locks);
> +       list_del(&vgadev->list);
> + bail:
> +       spin_unlock_irqrestore(&vga_lock, flags);
> +       if (vgadev)
> +               kfree(vgadev);

kfree(NULL) is fine, no need to check for null pointer.

> +}


Luca
-- 
Home: http://kronoz.cjb.net
"La mia teoria scientifica preferita e` quella secondo la quale gli 
 anelli di Saturno sarebbero interamente composti dai bagagli andati 
 persi nei viaggi aerei." -- Mark Russel

Re: [PATCH] VGA arbitration: draft of kernel side

[Benjamin Herrenschmidt]

Minus the big fat ugly bug of scheduling with a spinlock in vga_get() of
course ... bah. Easy to fix tho. I'll post a new version later.

Ben.

Re: [PATCH] VGA arbitration: draft of kernel side

[Benjamin Herrenschmidt]

On Tue, 2005-03-08 at 22:29 +0100, Kronos wrote:

> > +       bus = pdev->bus;
> > +       while (bus) {
> > +               bridge = bus->self;
> > +               if (bridge) {
> > +                       pci_read_config_word(bridge, PCI_BRIDGE_CONTROL, &cmd);
> > +                       if (!(cmd & PCI_BRIDGE_CTL_VGA))
> > +                               continue;
> 
> This seems wrong: if the condition is true the loop will restart with
> the same bus device and will never stop. I think you should do:

Yup. I always try to avoid nesting if's tho, which is why I wrote it
that way :) But yes, it should be fixed.

> > +
> > +       /* Only deal with VGA class devices */
> > +       if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
> > +               return;
> > +
> > +       /* Allocate structure */
> > +       vgadev = kmalloc(sizeof(struct vga_device), GFP_KERNEL);
> 
> Not checking return value of kmalloc, this is evil :P
> Also it may be worth to change return type in order to signal the error.

Yah, yah, I know :) will fix. I'm not sure there is point signaling the
error to the PCI layer. It won't do anything good with it.

> > +#endif
> > +
> > +       /* Add to the list */
> > +       list_add(&vgadev->list, &vga_list);
> > +       spin_unlock_irqrestore(&vga_lock, flags);
> 
> Missing return?

Yup.

> > + fail:
> > +       spin_unlock_irqrestore(&vga_lock, flags);
> > +       kfree(vgadev);
> > +}
> > +
> > +void vga_arbiter_del_pci_device(struct pci_dev *pdev)
> > +{
> > +       struct vga_device *vgadev;
> > +       unsigned long flags;
> > +
> > +       spin_lock_irqsave(&vga_lock, flags);
> > +       vgadev = vgadev_find(pdev);
> > +       if (vgadev == NULL)
> > +               goto bail;
> > +       if (vgadev->locks)
> > +               __vga_put(vgadev, vgadev->locks);
> > +       list_del(&vgadev->list);
> > + bail:
> > +       spin_unlock_irqrestore(&vga_lock, flags);
> > +       if (vgadev)
> > +               kfree(vgadev);
> 
> kfree(NULL) is fine, no need to check for null pointer.
> 
Hehe, yes, but I don't like it :)

Thanks. I spotted a few other issues (I was quite tired yesterday when I
finished this code). I'll do another pass on it today. One thing is: I
don't have x86 hardware, or at least, nothing where I can have 2 VGA
cards in (I may have access to an old laptop). So I'll need help &
testers at one point.

Ben.

Re: [PATCH] VGA arbitration: draft of kernel side

[Jon Smirl]

[-- Attachment #1: Type: text/plain, Size: 697 bytes --]

This very similar to the reset support patch I have been working on.

In the reset patch there is a 'vga' attribute on each VGA device. Set
it to 0/1 to make the device active. This lets you move the console
around betweem VGA devices.

You can also set it to 3, which disables all VGA devices but remembers
the active one. Setting it to 4 disables all VGA devices then restores
the active one. To use, set it to 3, post, set it to 4.

GregKH wants this code out of the pci directory but it needs hooks
into pci_destroy_dev() to delete the arbiter. You also need a hook on
add for when a hotplug device appears.

I'll try merging my sysfs support into your code.

-- 
Jon Smirl
jonsmirl@gmail.com

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vga.patch --]
[-- Type: text/x-diff; name="vga.patch", Size: 11369 bytes --]

diff -Nru a/arch/i386/pci/fixup.c b/arch/i386/pci/fixup.c
--- a/arch/i386/pci/fixup.c	2005-02-18 15:41:21 -05:00
+++ b/arch/i386/pci/fixup.c	2005-02-18 15:41:21 -05:00
@@ -375,6 +375,6 @@
 		}
 		bus = bus->parent;
 	}
-	pdev->resource[PCI_ROM_RESOURCE].flags |= IORESOURCE_ROM_SHADOW;
+	pdev->resource[PCI_ROM_RESOURCE].flags |= IORESOURCE_ROM_SHADOW | IORESOURCE_VGA_ACTIVE;
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, pci_fixup_video);
diff -Nru a/drivers/pci/Kconfig b/drivers/pci/Kconfig
--- a/drivers/pci/Kconfig	2005-02-18 15:41:21 -05:00
+++ b/drivers/pci/Kconfig	2005-02-18 15:41:21 -05:00
@@ -47,3 +47,13 @@
 
 	  When in doubt, say Y.
 
+config VGA_CONTROL
+	bool "VGA Control"
+	depends on PCI
+	---help---
+	  Provides sysfs attributes for ensuring that only a single VGA
+	  device can be enabled per PCI domain. If a VGA device is removed
+	  via hotplug, display is routed to another VGA device if available.
+
+	  If you have more than one VGA device, say Y.
+
diff -Nru a/drivers/pci/Makefile b/drivers/pci/Makefile
--- a/drivers/pci/Makefile	2005-02-18 15:41:21 -05:00
+++ b/drivers/pci/Makefile	2005-02-18 15:41:21 -05:00
@@ -28,6 +28,7 @@
 obj-$(CONFIG_MIPS) += setup-bus.o setup-irq.o
 obj-$(CONFIG_X86_VISWS) += setup-irq.o
 obj-$(CONFIG_PCI_MSI) += msi.o
+obj-$(CONFIG_VGA_CONTROL) += vga.o
 
 #
 # ACPI Related PCI FW Functions
diff -Nru a/drivers/pci/bus.c b/drivers/pci/bus.c
--- a/drivers/pci/bus.c	2005-02-18 15:41:21 -05:00
+++ b/drivers/pci/bus.c	2005-02-18 15:41:21 -05:00
@@ -85,6 +85,9 @@
 
 	pci_proc_attach_device(dev);
 	pci_create_sysfs_dev_files(dev);
+#if CONFIG_VGA_CONTROL
+	pci_vga_add_device(dev);
+#endif
 }
 
 /**
diff -Nru a/drivers/pci/pci.h b/drivers/pci/pci.h
--- a/drivers/pci/pci.h	2005-02-18 15:41:21 -05:00
+++ b/drivers/pci/pci.h	2005-02-18 15:41:21 -05:00
@@ -11,6 +11,8 @@
 				  void (*alignf)(void *, struct resource *,
 					  	 unsigned long, unsigned long),
 				  void *alignf_data);
+extern int pci_vga_add_device(struct pci_dev *pdev);
+extern int pci_vga_remove_device(struct pci_dev *pdev);
 /* PCI /proc functions */
 #ifdef CONFIG_PROC_FS
 extern int pci_proc_attach_device(struct pci_dev *dev);
diff -Nru a/drivers/pci/remove.c b/drivers/pci/remove.c
--- a/drivers/pci/remove.c	2005-02-18 15:41:21 -05:00
+++ b/drivers/pci/remove.c	2005-02-18 15:41:21 -05:00
@@ -26,6 +26,9 @@
 
 static void pci_destroy_dev(struct pci_dev *dev)
 {
+#if CONFIG_VGA_CONTROL
+	pci_vga_remove_device(dev);
+#endif
 	pci_proc_detach_device(dev);
 	pci_remove_sysfs_dev_files(dev);
 	device_unregister(&dev->dev);
diff -Nru a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
--- a/drivers/pci/setup-bus.c	2005-02-18 15:41:21 -05:00
+++ b/drivers/pci/setup-bus.c	2005-02-18 15:41:21 -05:00
@@ -64,7 +64,9 @@
 
 		if (class == PCI_CLASS_DISPLAY_VGA ||
 		    class == PCI_CLASS_NOT_DEFINED_VGA)
-			bus->bridge_ctl |= PCI_BRIDGE_CTL_VGA;
+			/* only route to the active VGA, ignore inactive ones */
+			if  (dev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_VGA_ACTIVE)
+				bus->bridge_ctl |= PCI_BRIDGE_CTL_VGA;
 
 		pdev_sort_resources(dev, &head);
 	}
diff -Nru a/drivers/pci/vga.c b/drivers/pci/vga.c
--- /dev/null	Wed Dec 31 16:00:00 196900
+++ b/drivers/pci/vga.c	2005-02-18 15:41:21 -05:00
@@ -0,0 +1,254 @@
+/*
+ * linux/drivers/pci/vga.c
+ *
+ * (C) Copyright 2004 Jon Smirl <jonsmirl@gmail.com>
+ *
+ * VGA control logic for ensuring only a single enabled VGA device
+ */
+
+#include <linux/init.h>
+#include <linux/fs.h>
+#include <linux/cdev.h>
+#include <linux/pci.h>
+#include <linux/major.h>
+
+static int vga_initialized = 0;
+static struct pci_dev *vga_active = NULL;
+
+static void bridge_yes(struct pci_dev *pdev)
+{
+	struct pci_dev *bridge;
+	struct pci_bus *bus;
+	
+	/* Make sure the bridges route to us */
+	bus = pdev->bus;
+	while (bus) {
+		bridge = bus->self;
+		if (bridge) {
+			bus->bridge_ctl |= PCI_BRIDGE_CTL_VGA;
+			pci_write_config_word(bridge, PCI_BRIDGE_CONTROL, bus->bridge_ctl);
+		}
+		bus = bus->parent;
+	}
+}
+
+static void bridge_no(struct pci_dev *pdev)
+{
+	struct pci_dev *bridge;
+	struct pci_bus *bus;
+	
+	/* Make sure the bridges don't route to us */
+	bus = pdev->bus;
+	while (bus) {
+		bridge = bus->self;
+		if (bridge) {
+			bus->bridge_ctl &= ~PCI_BRIDGE_CTL_VGA;
+			pci_write_config_word(bridge, PCI_BRIDGE_CONTROL, bus->bridge_ctl);
+		}
+		bus = bus->parent;
+	}
+}
+
+static void vga_enable(struct pci_dev *pdev, unsigned int enable)
+{
+	u16 command;
+	
+	bridge_yes(pdev);
+
+	if (enable) {
+		pci_enable_device(pdev);
+		/* this assumes all other potential VGA devices are disabled */
+		outb(0x01 | inb(0x3C3),  0x3C3);  /* 0 - enable */
+		outb(0x08 | inb(0x46e8), 0x46e8);
+		outb(0x01 | inb(0x102),  0x102);
+		pdev->resource[PCI_ROM_RESOURCE].flags |= IORESOURCE_VGA_ACTIVE;
+		vga_active = pdev;
+
+		/* return and leave the card enabled */
+		return;
+	}
+	
+	pci_read_config_word(pdev, PCI_COMMAND, &command);
+	pci_write_config_word(pdev, PCI_COMMAND, command | PCI_COMMAND_IO | PCI_COMMAND_MEMORY);
+	
+	outb(~0x01 & inb(0x3C3),  0x3C3);
+	outb(~0x08 & inb(0x46e8), 0x46e8);
+	outb(~0x01 & inb(0x102),  0x102);
+	pdev->resource[PCI_ROM_RESOURCE].flags &= ~IORESOURCE_VGA_ACTIVE;
+	if (pdev == vga_active)
+		vga_active = NULL;
+	bridge_no(pdev);
+
+	pci_write_config_word(pdev, PCI_COMMAND, command);
+}
+
+/* echo these values to the sysfs vga attribute on a VGA device */
+enum eEnable {
+	VGA_DISABLE_THIS = 0,	/* If this VGA is enabled, disable it. */
+	VGA_ENABLE_THIS = 1,	/* Disable all VGAs then enable this VGA, mark as active VGA */
+	/* Used while resetting a board, board being reset may not be the active VGA */
+	VGA_DISABLE_ALL = 2,	/* Remember active VGA then disable all VGAa and devices */
+	VGA_ENABLE_ACTIVE = 3,	/* Make sure all VGAs are disabled, then reenable active VGA */
+};
+
+static void set_state(struct pci_dev *pdev, enum eEnable enable)
+{
+	struct pci_dev *pcidev = NULL;
+	unsigned int class;
+
+	if (enable == VGA_DISABLE_THIS)
+		if (vga_active != pdev)
+			return;
+		
+	vga_enable(vga_active, 0);
+
+	/* loop over all devices and make sure no multiple routings */
+	while ((pcidev = pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, pcidev)) != NULL) {
+		class = pcidev->class >> 8;
+
+		if (class == PCI_CLASS_DISPLAY_VGA)
+			bridge_no(pcidev);
+	}
+
+	/* loop over all devices and make sure everyone is disabled */
+	while ((pcidev = pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, pcidev)) != NULL) {
+		class = pcidev->class >> 8;
+
+		if (class == PCI_CLASS_DISPLAY_VGA)
+			vga_enable(pcidev, 0);
+	}
+
+	switch (enable) {
+		case VGA_DISABLE_THIS:
+		case VGA_DISABLE_ALL:
+			break;
+
+		/* Mark us active if requested */
+		case VGA_ENABLE_THIS:
+			vga_enable(pdev, 1);
+			break;
+	
+		/* Restore active device if requested */
+		case VGA_ENABLE_ACTIVE:
+			vga_enable(vga_active, 1);
+			break;
+	}
+}
+
+/* sysfs store for VGA device */
+static ssize_t vga_device_store(struct device *dev, const char *buf, size_t count)
+{
+	char *last;
+	struct pci_dev *pdev = to_pci_dev(dev);
+	enum eEnable enable;
+
+	/* sysfs strings are terminated by \n */
+	enable = simple_strtoul(buf, &last, 0);
+	if (last == buf)
+		return -EINVAL;
+
+	if ((enable < VGA_DISABLE_THIS) || (enable > VGA_ENABLE_ACTIVE))
+		return -EINVAL;
+
+	set_state(pdev, enable);
+
+	return count;
+}
+
+/* sysfs show for VGA device */
+static ssize_t vga_device_show(struct device *dev, char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	return sprintf(buf, "%d\n", (pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_VGA_ACTIVE) != 0);
+}
+
+static struct device_attribute vga_device_attr = __ATTR(vga, S_IRUGO|S_IWUSR, vga_device_show, vga_device_store);
+
+/* sysfs show for VGA routing bridge */
+static ssize_t vga_bridge_show(struct device *dev, char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	u16 l;
+
+	/* don't trust the shadow PCI_BRIDGE_CTL_VGA in pdev */
+	/* user space may change hardware without telling the kernel */
+	pci_read_config_word(pdev, PCI_BRIDGE_CONTROL, &l);
+	return sprintf(buf, "%d\n", (l & PCI_BRIDGE_CTL_VGA) != 0);
+}
+
+static struct device_attribute vga_bridge_attr = __ATTR(vga, S_IRUGO, vga_bridge_show, NULL);
+
+/* If the device is a VGA or a bridge, add a VGA sysfs attribute */
+int pci_vga_add_device(struct pci_dev *pdev)
+{
+	int class = pdev->class >> 8;
+
+	if (!vga_initialized)
+		return -EACCES;
+
+	if (class == PCI_CLASS_DISPLAY_VGA) {
+		device_create_file(&pdev->dev, &vga_device_attr);
+
+		/* record the active boot device when located */
+		if (pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_VGA_ACTIVE)
+			vga_active = pdev;
+		return 0;
+	}
+
+	if ((class == PCI_CLASS_BRIDGE_PCI) || (class == PCI_CLASS_BRIDGE_CARDBUS)) {
+		device_create_file(&pdev->dev, &vga_bridge_attr);
+	}
+	return 0;
+}
+
+/* If the device is a VGA or a bridge, remove the VGA sysfs attribute */
+int pci_vga_remove_device(struct pci_dev *pdev)
+{
+	struct pci_dev *pcidev = NULL;
+	int class = pdev->class >> 8;
+
+	if (!vga_initialized)
+		return -EACCES;
+
+	if (class == PCI_CLASS_DISPLAY_VGA) {
+		device_remove_file(&pdev->dev, &vga_device_attr);
+
+		/* record the active boot device when located */
+		if (vga_active == pdev) {
+			while ((pcidev = pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, pcidev)) != NULL) {
+				class = pcidev->class >> 8;
+				if (class != PCI_CLASS_DISPLAY_VGA)
+					continue;
+				if (pcidev == pdev)
+					continue;
+				set_state(pcidev, VGA_ENABLE_THIS);
+				break;
+			}
+			if (pcidev == NULL)
+				set_state(NULL, VGA_DISABLE_ALL);
+			
+		}
+		return 0;
+	}
+
+	if ((class == PCI_CLASS_BRIDGE_PCI) || (class == PCI_CLASS_BRIDGE_CARDBUS))
+		device_remove_file(&pdev->dev, &vga_bridge_attr);
+
+	return 0;
+}
+
+/* Initialize by scanning all devices */
+static int __init vga_init(void)
+{
+	struct pci_dev *pdev = NULL;
+
+	vga_initialized = 1;
+
+	while ((pdev = pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, pdev)) != NULL)
+		pci_vga_add_device(pdev);
+		
+	return 0;
+}
+
+__initcall(vga_init);
+
diff -Nru a/include/linux/ioport.h b/include/linux/ioport.h
--- a/include/linux/ioport.h	2005-02-18 15:41:21 -05:00
+++ b/include/linux/ioport.h	2005-02-18 15:41:21 -05:00
@@ -41,7 +41,6 @@
 #define IORESOURCE_CACHEABLE	0x00004000
 #define IORESOURCE_RANGELENGTH	0x00008000
 #define IORESOURCE_SHADOWABLE	0x00010000
-#define IORESOURCE_BUS_HAS_VGA	0x00080000
 
 #define IORESOURCE_DISABLED	0x10000000
 #define IORESOURCE_UNSET	0x20000000
@@ -86,6 +85,7 @@
 #define IORESOURCE_ROM_ENABLE		(1<<0)	/* ROM is enabled, same as PCI_ROM_ADDRESS_ENABLE */
 #define IORESOURCE_ROM_SHADOW		(1<<1)	/* ROM is copy at C000:0 */
 #define IORESOURCE_ROM_COPY		(1<<2)	/* ROM is alloc'd copy, resource field overlaid */
+#define IORESOURCE_VGA_ACTIVE		(1<<3)	/* VGA device is active */
 
 /* PC/ISA/whatever - the normal PC address spaces: IO and memory */
 extern struct resource ioport_resource;

Re: [PATCH] VGA arbitration: draft of kernel side

[Benjamin Herrenschmidt]

On Tue, 2005-03-08 at 18:47 -0500, Jon Smirl wrote:
> This very similar to the reset support patch I have been working on.
> 
> In the reset patch there is a 'vga' attribute on each VGA device. Set
> it to 0/1 to make the device active. This lets you move the console
> around betweem VGA devices.
> 
> You can also set it to 3, which disables all VGA devices but remembers
> the active one. Setting it to 4 disables all VGA devices then restores
> the active one. To use, set it to 3, post, set it to 4.
> 
> GregKH wants this code out of the pci directory but it needs hooks
> into pci_destroy_dev() to delete the arbiter. You also need a hook on
> add for when a hotplug device appears.
> 
> I'll try merging my sysfs support into your code.

Please wait.

I don't want that semantic for sysfs. First, I don't want to "move
around" the VGA device. This is very arch specific and will not work in
a variety if circumstances. Also, "outb's" to legacy VGA ports will only
work with some PCI domains on archs like PPC, even vgacon can't deal
with that, so let's avoid putting such "knowledge" in the arbiter
itself. I prefer for now defining a "default" vga device which is the
one used by vgacon. If you want to move vgacon around, do some arch
specific stuff or add way to change the default device, but that isn't
directly related to the arbitration process.

Also, I want the sysfs entry (or /dev if I can't get the proper
semantics in sysfs) to have open & release() callbacks like a char
device. The reason is that I want the vga "locks" owned by a process to
be automatically released if the process dies. Without that, kill -9 on
X will end up requiring a reboot in most circumstances :)

Finally, I want to keep the distinction between memory and IO enables.
That's quite important imho, since a lot of cards can operate with IO
disabled (all ATIs for example), which is good as I'm not completely
sure I can disable legacy IO port decoding on them (well, I don't know
how to do it).

Ben.
 

Re: [PATCH] VGA arbitration: draft of kernel side

[Jon Smirl]

How do I do the 'disable all, post, renable last active' sequence in
this scheme?

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] VGA arbitration: draft of kernel side

[Benjamin Herrenschmidt]

On Tue, 2005-03-08 at 22:17 -0500, Jon Smirl wrote:
> How do I do the 'disable all, post, renable last active' sequence in
> this scheme?

You don't do it that way. You vga_get(IO|MEM) the card you want to
POST, do the POST, then vga_put().

Subsequent user will get back ownership when it does vga_get(something)
again.

BTW. I have a draft of the userland API. It will be a /dev entry (so
other OSes can implement the same API, also, it's just doing too much
for sysfs, I debated it with a few kernel folks and we decided it should
be that way) :

 *  open	: open user instance of the arbitrer. by default, it's
 *                attached to the default VGA device of the system.
 *
 *  close	: close user instance, release locks
 *
 *  read	: return a string indicating the status of the target.
 *                an IO state string is of the form {mem,io,mem+io,none},
 *                mc and ic are respectively mem and io lock counts (for
 *                debugging/diagnostic only). "decodes" indicate what the
 *                card currently decodes, "owns" indicates what is currently
 *                enabled on it, and "locks" indicates what is locked by this
 *                card.
 *
 *   "<card_ID>:decodes=<io_state>,owns=<io_state>,locks=<io_state> (mc,ic)"
 *
 * write	: write a command to the arbiter. List of commands is:
 *
 *   target <card_ID>   : switch target to card <card_ID> (see below)
 *   lock <io_state>    : acquires locks on target ("none" is invalid io_state)
 *   trylock <io_state> : non-blocking acquire locks on target
 *   unlock <io_state>  : release locks on target
 *   decodes <io_state> : set the legacy decoding attributes for the card
 * 
 * poll         : event if something change on any card (not just the target)


I also added nesting counters (mostly to make things safer, though it could
be useful for scenarios where IRQ stuffs are doing a tryget kind of thing
as described in a previous message).

Ben.

Re: [PATCH] VGA arbitration: draft of kernel side

[Jon Smirl]

This is from /linux-2.5/Documentation/filesystems/sysfs-pci.txt. It
describes how ia64 is achieving legacy IO.  The VGA control code
probably needs to be coordinated with this.

--------------------------------------------------------------------------

Accessing legacy resources through sysfs

Legacy I/O port and ISA memory resources are also provided in sysfs if the
underlying platform supports them.  They're located in the PCI class heirarchy,
e.g.

        /sys/class/pci_bus/0000:17/
        |-- bridge -> ../../../devices/pci0000:17
        |-- cpuaffinity
        |-- legacy_io
        `-- legacy_mem

The legacy_io file is a read/write file that can be used by applications to
do legacy port I/O.  The application should open the file, seek to the desired
port (e.g. 0x3e8) and do a read or a write of 1, 2 or 4 bytes.  The legacy_mem
file should be mmapped with an offset corresponding to the memory offset
desired, e.g. 0xa0000 for the VGA frame buffer.  The application can then
simply dereference the returned pointer (after checking for errors of course)
to access legacy memory space.

Supporting PCI access on new platforms

In order to support PCI resource mapping as described above, Linux platform
code must define HAVE_PCI_MMAP and provide a pci_mmap_page_range function.
Platforms are free to only support subsets of the mmap functionality, but
useful return codes should be provided.

Legacy resources are protected by the HAVE_PCI_LEGACY define.  Platforms
wishing to support legacy functionality should define it and provide
pci_legacy_read, pci_legacy_write and pci_mmap_legacy_page_range functions.


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] VGA arbitration: draft of kernel side

[Benjamin Herrenschmidt]

On Tue, 2005-03-08 at 23:35 -0500, Jon Smirl wrote:
> This is from /linux-2.5/Documentation/filesystems/sysfs-pci.txt. It
> describes how ia64 is achieving legacy IO.  The VGA control code
> probably needs to be coordinated with this.

This is a different thing, and I will implement it on ppc one of these
days. This is for issuing the IO cycles on the bus. It has nothing
to do with the actual arbitration work.

> --------------------------------------------------------------------------
> 
> Accessing legacy resources through sysfs
> 
> Legacy I/O port and ISA memory resources are also provided in sysfs if the
> underlying platform supports them.  They're located in the PCI class heirarchy,
> e.g.
> 
>         /sys/class/pci_bus/0000:17/
>         |-- bridge -> ../../../devices/pci0000:17
>         |-- cpuaffinity
>         |-- legacy_io
>         `-- legacy_mem
> 
> The legacy_io file is a read/write file that can be used by applications to
> do legacy port I/O.  The application should open the file, seek to the desired
> port (e.g. 0x3e8) and do a read or a write of 1, 2 or 4 bytes.  The legacy_mem
> file should be mmapped with an offset corresponding to the memory offset
> desired, e.g. 0xa0000 for the VGA frame buffer.  The application can then
> simply dereference the returned pointer (after checking for errors of course)
> to access legacy memory space.
> 
> Supporting PCI access on new platforms
> 
> In order to support PCI resource mapping as described above, Linux platform
> code must define HAVE_PCI_MMAP and provide a pci_mmap_page_range function.
> Platforms are free to only support subsets of the mmap functionality, but
> useful return codes should be provided.
> 
> Legacy resources are protected by the HAVE_PCI_LEGACY define.  Platforms
> wishing to support legacy functionality should define it and provide
> pci_legacy_read, pci_legacy_write and pci_mmap_legacy_page_range functions.
> 
> 
-- 
Benjamin Herrenschmidt <benh@kernel.crashing.org>

Re: [PATCH] VGA arbitration: draft of kernel side

[Jon Smirl]

On Wed, 09 Mar 2005 16:37:13 +1100, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Tue, 2005-03-08 at 23:35 -0500, Jon Smirl wrote:
> > This is from /linux-2.5/Documentation/filesystems/sysfs-pci.txt. It
> > describes how ia64 is achieving legacy IO.  The VGA control code
> > probably needs to be coordinated with this.
> 
> This is a different thing, and I will implement it on ppc one of these
> days. This is for issuing the IO cycles on the bus. It has nothing
> to do with the actual arbitration work.

Each one of these legacy spaces corresponds to an allowable
simultaneous VGA use. There should be one arbiter per legacy space.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] VGA arbitration: draft of kernel side

[Benjamin Herrenschmidt]

On Wed, 2005-03-09 at 00:58 -0500, Jon Smirl wrote:
> On Wed, 09 Mar 2005 16:37:13 +1100, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > On Tue, 2005-03-08 at 23:35 -0500, Jon Smirl wrote:
> > > This is from /linux-2.5/Documentation/filesystems/sysfs-pci.txt. It
> > > describes how ia64 is achieving legacy IO.  The VGA control code
> > > probably needs to be coordinated with this.
> > 
> > This is a different thing, and I will implement it on ppc one of these
> > days. This is for issuing the IO cycles on the bus. It has nothing
> > to do with the actual arbitration work.
> 
> Each one of these legacy spaces corresponds to an allowable
> simultaneous VGA use. There should be one arbiter per legacy space.

Ugh ? They are or they are not independant, that's a platform thing and
has nothing to do with arbitration. They aren't VGA specific neither.

The arbitrer uses the vga_conflicts() callback for that purpose. It is
defined to always return 1 by default, but the platform can override it
if it has separate PCI domains, in order to tell the arbitrer wether 2
cards can conflict or not.

Based on that, the arbitrer will, or will not, let you lock the VGA
legacy resources simultaneously.

Ben.

Re: [PATCH] VGA arbitration: draft of kernel side

[Benjamin Herrenschmidt]

Ok, here's today status. I posted the patch at
http://gate.crashing.org/~benh/vga-arbiter.diff. I fixed some issues &
added support for nesting locks, I added comments/documentation to
kernel interface. It's not tested yet. It's not complete neither, the
userland interface is partially implemented (via a /dev char device),
and I may still change things here or there before I'm happy with the
result. Constructive comments appreciated.

What need to be done also is to adapt vgacon.

The problems here are multiple. vgacon itself has all the consw
callbacks that need to be dealt with. A bunch of them can't schedule,
so they would have to use vga_tryget(). What to do if that fails ?
Another problem is that the VT code will directly access the
text/attribute buffer (VGA memory). Playing tricks here promises to
be difficult. Some bits of that code even keep pointers to video
memory as local statics (look at complement_pos(), that stuff is
probably interestingly broken during the vgacon->fbcon transition)

So I suspect here a minimum of rework is needed, in a rather disgusting
area of the code.

I suppose the easiest way for now is to move the vga_trylock() as much
up as possible in the call chain. The stuff in vc_screen() (userland
context, can schedule) would use vga_get(), same with the tty operations
(they acquire the console sem, so they are a big no-no at interrupt
time, but we need to do runtime checks since weird things may happen in
tty land).

The problem is how to have that code "know" that it needs to lock VGA
resources. It will be different between vgacon, fbcon, or whatever other
low level console. Some hacks may be needed here, at least until we have
a "sane" console subsystem if we ever have ... I'll have a deeper look
tomorrow.

Ben.

Re: [PATCH] VGA arbitration: draft of kernel side

[Pekka Enberg]

Hi Benjamin,

On Tue, 2005-03-08 at 22:29 +0100, Kronos wrote:
> > kfree(NULL) is fine, no need to check for null pointer.

On Wed, 09 Mar 2005 09:46:20 +1100, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> Hehe, yes, but I don't like it :)

Please consider doing that anyway as there are ongoing janitor
projects that are removing the redundant if clauses...

                                           Pekka

Re: [PATCH] VGA arbitration: draft of kernel side

[Pekka Enberg]

Hi Benjamin,

Few coding style nitpicks follow.

On Tue, 08 Mar 2005 18:11:59 +1100, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> Index: linux-work/include/linux/pci.h
> ===================================================================
> --- linux-work.orig/include/linux/pci.h	2005-01-24 17:09:57.000000000 +1100
> +++ linux-work/include/linux/pci.h	2005-03-08 15:26:25.000000000 +1100
> @@ -1064,5 +1064,6 @@
>  #define PCIPCI_VSFX		16
>  #define PCIPCI_ALIMAGIK		32
>  
> +
>  #endif /* __KERNEL__ */
>  #endif /* LINUX_PCI_H */

Please drop whitespace noise from the patch.

> Index: linux-work/drivers/pci/vga.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-work/drivers/pci/vga.c	2005-03-08 18:04:57.000000000 +1100
> @@ -0,0 +1,403 @@
> +static LIST_HEAD(		vga_list);

Please remove whitespace damage.

> +static spinlock_t	       	vga_lock;
> +static DECLARE_WAIT_QUEUE_HEAD(	vga_wait_queue);

Ditto.

> +/* Architecture can override enabling/disabling of a given
> + * device resources here
> + */
> +#ifndef __ARCH_HAS_VGA_DISABLE_RESOURCES
> +static inline void vga_disable_resources(struct pci_dev *pdev,
> +					 unsigned int rsrc,
> +					 unsigned int change_bridge)
> +{
> +	struct pci_bus *bus;
> +	struct pci_dev *bridge;
> +	u16 cmd;
> +
> +	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> +	if (rsrc & VGA_RSRC_IO)
> +		cmd &= ~PCI_COMMAND_IO;
> +	if (rsrc & VGA_RSRC_MEM)
> +		cmd &= ~PCI_COMMAND_MEMORY;
> +	pci_write_config_word(pdev, PCI_COMMAND, cmd);
> +
> +	if (!change_bridge)
> +		return;
> +
> +	bus = pdev->bus;
> +	while (bus) {
> +		bridge = bus->self;
> +		if (bridge) {
> +			pci_read_config_word(bridge, PCI_BRIDGE_CONTROL, &cmd);
> +			if (!(cmd & PCI_BRIDGE_CTL_VGA))
> +				continue;
> +			cmd &= ~PCI_BRIDGE_CTL_VGA;
> +			pci_write_config_word(bridge, PCI_BRIDGE_CONTROL, cmd);
> +		}
> +		bus = bus->parent;

See comment below.

> +	}
> +}
> +#endif
> +
> +#ifndef __ARCH_HAS_VGA_ENABLE_RESOURCES
> +static inline void vga_enable_resources(struct pci_dev *pdev,
> +					 unsigned int rsrc)
> +{
> +	struct pci_bus *bus;
> +	struct pci_dev *bridge;
> +	u16 cmd;
> +
> +	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> +	if (rsrc & VGA_RSRC_IO)
> +		cmd |= PCI_COMMAND_IO;
> +	if (rsrc & VGA_RSRC_MEM)
> +		cmd |= PCI_COMMAND_MEMORY;
> +	pci_write_config_word(pdev, PCI_COMMAND, cmd);
> +
> +	bus = pdev->bus;
> +	while (bus) {
> +		bridge = bus->self;
> +		if (bridge) {
> +			pci_read_config_word(bridge, PCI_BRIDGE_CONTROL, &cmd);
> +			if (cmd & PCI_BRIDGE_CTL_VGA)
> +				continue;
> +			cmd |= PCI_BRIDGE_CTL_VGA;
> +			pci_write_config_word(bridge, PCI_BRIDGE_CONTROL, cmd);
> +		}
> +		bus = bus->parent;

Please consolidate both while loops into one function. One possible way would
be to do:

static void vga_update_bus(struct pci_bus *bus, unsigned int enable)
{
	while (bus) {
		bridge = bus->self;
		if (bridge) {
			pci_read_config_word(bridge, PCI_BRIDGE_CONTROL, &cmd);
			if (cmd & PCI_BRIDGE_CTL_VGA)
				continue;
			if (enable)
				cmd |= PCI_BRIDGE_CTL_VGA;
			else
				cmd &= ~PCI_BRIDGE_CTL_VGA;
			pci_write_config_word(bridge, PCI_BRIDGE_CONTROL, cmd);
		}
		bus = bus->parent;
	}
}

> +/*
> + * Currently, we assume that the "initial" setup of the system is
> + * sane, that is we don't come up with conflicting devices, which
> + * would be annoying. We could double check and be better at
> + * deciding who is the default here, but we don't. 
> + */
> +void vga_arbiter_add_pci_device(struct pci_dev *pdev)
> +{
> +	struct vga_device *vgadev;
> +	unsigned long flags;
> +	struct pci_bus *bus;
> +	struct pci_dev *bridge;
> +	u16 cmd;
> +
> +	/* Only deal with VGA class devices */
> +	if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
> +		return;
> +
> +	/* Allocate structure */
> +	vgadev = kmalloc(sizeof(struct vga_device), GFP_KERNEL);
> +	memset(vgadev, 0, sizeof(*vgadev));

Please consider using kcalloc() here.

                                          Pekka

Re: [PATCH] VGA arbitration: draft of kernel side

[Jesse Barnes]

On Tuesday, March 8, 2005 9:58 pm, Jon Smirl wrote:
> On Wed, 09 Mar 2005 16:37:13 +1100, Benjamin Herrenschmidt
>
> <benh@kernel.crashing.org> wrote:
> > On Tue, 2005-03-08 at 23:35 -0500, Jon Smirl wrote:
> > > This is from /linux-2.5/Documentation/filesystems/sysfs-pci.txt. It
> > > describes how ia64 is achieving legacy IO.  The VGA control code
> > > probably needs to be coordinated with this.
> >
> > This is a different thing, and I will implement it on ppc one of these
> > days. This is for issuing the IO cycles on the bus. It has nothing
> > to do with the actual arbitration work.
>
> Each one of these legacy spaces corresponds to an allowable
> simultaneous VGA use. There should be one arbiter per legacy space.

Jon, I think the arbiters have to be per-resource rather than per-legacy 
space.  AIUI, the arbiters are there to deal with multiple devices on the 
same bus that respond to the same cycles, like two VGA cards in one legacy 
I/O domain.  You either need to relocate one of them so that they don't have 
overlapping I/O ranges or disable one while you talk to the other.

IOW, legacy space is the whole I/O window of a given bus or PCI domain 
(granularity defined by the platform--some will only have one I/O space), and 
the arbiter's job is to arbitrate access to subsets of each window.  I think 
the the VGA stuff here complements the legacy interface rather than 
conflicting with it.

Jesse

Re: [PATCH] VGA arbitration: draft of kernel side

[Kronos]

Il Wed, Mar 09, 2005 at 09:46:20AM +1100, Benjamin Herrenschmidt ha scritto: 
> One thing is: I
> don't have x86 hardware, or at least, nothing where I can have 2 VGA
> cards in (I may have access to an old laptop). So I'll need help &
> testers at one point.

It's your lucky day ;) I've just assembled a PC with 2 PCI video card (S3
something and a Matrox Mystique) and I think that I've an old ISA video
card somewhere (if it can be usefull).
Feel free to put me on CC when you have something to test.

Luca
-- 
Home: http://kronoz.cjb.net
Colui che sorride quando le cose vanno male ha pensato a qualcuno a cui
dare la colpa.

Re: [PATCH] VGA arbitration: draft of kernel side

[Benjamin Herrenschmidt]

On Wed, 2005-03-09 at 12:45 +0200, Pekka Enberg wrote:
> Hi Benjamin,
> 
> Few coding style nitpicks follow.
> 
> On Tue, 08 Mar 2005 18:11:59 +1100, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > Index: linux-work/include/linux/pci.h
> > ===================================================================
> > --- linux-work.orig/include/linux/pci.h	2005-01-24 17:09:57.000000000 +1100
> > +++ linux-work/include/linux/pci.h	2005-03-08 15:26:25.000000000 +1100
> > @@ -1064,5 +1064,6 @@
> >  #define PCIPCI_VSFX		16
> >  #define PCIPCI_ALIMAGIK		32
> >  
> > +
> >  #endif /* __KERNEL__ */
> >  #endif /* LINUX_PCI_H */
> 
> Please drop whitespace noise from the patch.

Oh sure, will do. I'm not about to submit anything yet anyway, and it
will go through a cleanup phase. The above is just residual of quilt
picking up a file where I added something, then removed it.

> > Index: linux-work/drivers/pci/vga.c
> > ===================================================================
> > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > +++ linux-work/drivers/pci/vga.c	2005-03-08 18:04:57.000000000 +1100
> > @@ -0,0 +1,403 @@
> > +static LIST_HEAD(		vga_list);
> 
> Please remove whitespace damage.
> 
> > +static spinlock_t	       	vga_lock;
> > +static DECLARE_WAIT_QUEUE_HEAD(	vga_wait_queue);

The above isn't whitespace damage, it's aligning of the 3 variable
names properly in a column :) I dislike those DECLARE_*() macros because
of that btw. That one is a matter of style, I'm experiencing a bit with
this, but it's definitely intentional.

> 
> Please consolidate both while loops into one function. One possible way would
> be to do:
> 
> static void vga_update_bus(struct pci_bus *bus, unsigned int enable)
> {
> 	while (bus) {
> 		bridge = bus->self;
> 		if (bridge) {
> 			pci_read_config_word(bridge, PCI_BRIDGE_CONTROL, &cmd);
> 			if (cmd & PCI_BRIDGE_CTL_VGA)
> 				continue;
> 			if (enable)
> 				cmd |= PCI_BRIDGE_CTL_VGA;
> 			else
> 				cmd &= ~PCI_BRIDGE_CTL_VGA;
> 			pci_write_config_word(bridge, PCI_BRIDGE_CONTROL, cmd);
> 		}
> 		bus = bus->parent;
> 	}
> }

I think you are beeing anal here, but I'll think about it ;)

> > +/*
> > + * Currently, we assume that the "initial" setup of the system is
> > + * sane, that is we don't come up with conflicting devices, which
> > + * would be annoying. We could double check and be better at
> > + * deciding who is the default here, but we don't. 
> > + */
> > +void vga_arbiter_add_pci_device(struct pci_dev *pdev)
> > +{
> > +	struct vga_device *vgadev;
> > +	unsigned long flags;
> > +	struct pci_bus *bus;
> > +	struct pci_dev *bridge;
> > +	u16 cmd;
> > +
> > +	/* Only deal with VGA class devices */
> > +	if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
> > +		return;
> > +
> > +	/* Allocate structure */
> > +	vgadev = kmalloc(sizeof(struct vga_device), GFP_KERNEL);
> > +	memset(vgadev, 0, sizeof(*vgadev));
> 
> Please consider using kcalloc() here.

Will do.

Ben.