VGA arbitration: draft of kernel side

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

Index: linux-work/drivers/pci/vga.c
===================================================================

drivers/pci/vga.c 2005-03-08 18:04:57.000000000 +1100 renamed from dev/null 1970-01-01 00:00:00.000000000 +0000 +0 -0

drivers/pci/vga.c 2005-03-08 18:04:57.000000000 +1100 renamed from dev/null 1970-01-01 00:00:00.000000000 +0000 +16 -0

+#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);
+

drivers/pci/vga.c 2005-03-08 18:04:57.000000000 +1100 renamed from dev/null 1970-01-01 00:00:00.000000000 +0000 +20 -0

+                       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)

drivers/pci/vga.c 2005-03-08 18:04:57.000000000 +1100 renamed from dev/null 1970-01-01 00:00:00.000000000 +0000 +17 -0

+                       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;

drivers/pci/vga.c 2005-03-08 18:04:57.000000000 +1100 renamed from dev/null 1970-01-01 00:00:00.000000000 +0000 +40 -0

+       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
+        */

+ fail:

drivers/pci/vga.c 2005-03-08 18:04:57.000000000 +1100 renamed from dev/null 1970-01-01 00:00:00.000000000 +0000 +10 -0

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

+}

drivers/pci/vga.c line 83

Quoted 7 lines

drivers/pci/vga.c new file +7 -0

+       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:

drivers/pci/vga.c line 309

Quoted 7 lines

drivers/pci/vga.c new file +7 -0

+
+       /* 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.

drivers/pci/vga.c line 359

Quoted 5 lines

drivers/pci/vga.c new file +5 -0

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

Missing return?

Quoted 15 lines

+ fail:

drivers/pci/vga.c new file +14 -0

+       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)

drivers/pci/vga.c line 380

Quoted 6 lines

drivers/pci/vga.c new file +6 -0

+               __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.

Quoted 1 line

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

Hehe, yes, but I don't like it :)

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.

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.

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

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.

Quoted 3 lines · 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.

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.

Quoted 5 lines · Benjamin Herrenschmidt

Quoted 3 lines · 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.

Quoted 3 lines

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.

On Wed, 09 Mar 2005 16:37:13 +1100, Benjamin Herrenschmidt

Quoted 5 lines · <[email protected]> wrote:

Quoted 3 lines · 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.

Quoted 3 lines

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.

drivers/pci/vga.c new file +23 -0

+/* 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;

drivers/pci/vga.c new file +20 -0

+	}
+}
+#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;

drivers/pci/vga.c new file +11 -0

+/*
+ * 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;

Hi Benjamin,

Few coding style nitpicks follow.

Quoted 8 lines · Benjamin Herrenschmidt

Index: linux-work/include/linux/pci.h
===================================================================

include/linux/pci.h +0 -0

@@ -1064,5 +1064,6 @@
 #define PCIPCI_VSFX		16
 #define PCIPCI_ALIMAGIK		32
 

include/linux/pci.h line 1069

Quoted 3 lines

include/linux/pci.h +1 -0

+
 #endif /* __KERNEL__ */
 #endif /* LINUX_PCI_H */

Please drop whitespace noise from the patch.

drivers/pci/vga.c line 1

Quoted 6 lines

Index: linux-work/drivers/pci/vga.c
===================================================================

drivers/pci/vga.c new file +1 -0

@@ -0,0 +1,403 @@
+static LIST_HEAD(		vga_list);

Please remove whitespace damage.

drivers/pci/vga.c line 29

Quoted 2 lines

drivers/pci/vga.c new file +2 -0

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

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;
	}
}

Quoted 15 lines

drivers/pci/vga.c new file +15 -0

+/*
+ * 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 */

drivers/pci/vga.c line 310

Quoted 6 lines

drivers/pci/vga.c new file +6 -0

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