Re: [PATCH] VGA arbitration: draft of kernel side
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
> <[EMAIL PROTECTED]> wrote:
> > Index: linux-work/include/linux/pci.h
> > ===
> > --- linux-work.orig/include/linux/pci.h 2005-01-24 17:09:57.0
> > +1100
> > +++ linux-work/include/linux/pci.h 2005-03-08 15:26:25.0 +1100
> > @@ -1064,5 +1064,6 @@
> > #define PCIPCI_VSFX16
> > #define PCIPCI_ALIMAGIK32
> >
> > +
> > #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.0 +
> > +++ linux-work/drivers/pci/vga.c2005-03-08 18:04:57.0 +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, );
> 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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] VGA arbitration: draft of kernel side
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] VGA arbitration: draft of kernel side
On Tuesday, March 8, 2005 9:58 pm, Jon Smirl wrote: > On Wed, 09 Mar 2005 16:37:13 +1100, Benjamin Herrenschmidt > > <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] VGA arbitration: draft of kernel side
Hi Benjamin,
Few coding style nitpicks follow.
On Tue, 08 Mar 2005 18:11:59 +1100, Benjamin Herrenschmidt
<[EMAIL PROTECTED]> wrote:
> Index: linux-work/include/linux/pci.h
> ===
> --- linux-work.orig/include/linux/pci.h 2005-01-24 17:09:57.0
> +1100
> +++ linux-work/include/linux/pci.h2005-03-08 15:26:25.0 +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.0 +
> +++ linux-work/drivers/pci/vga.c 2005-03-08 18:04:57.0 +1100
> @@ -0,0 +1,403 @@
> +static LIST_HEAD(vga_list);
Please remove whitespace damage.
> +static spinlock_tvga_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, );
> + 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, );
> + 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, );
> + 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, );
> + 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, );
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
Re: [PATCH] VGA arbitration: draft of kernel side
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 <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] VGA arbitration: draft of kernel side
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] VGA arbitration: draft of kernel side
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] VGA arbitration: draft of kernel side
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 [EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] VGA arbitration: draft of kernel side
Hi Benjamin,
Few coding style nitpicks follow.
On Tue, 08 Mar 2005 18:11:59 +1100, Benjamin Herrenschmidt
[EMAIL PROTECTED] wrote:
Index: linux-work/include/linux/pci.h
===
--- linux-work.orig/include/linux/pci.h 2005-01-24 17:09:57.0
+1100
+++ linux-work/include/linux/pci.h2005-03-08 15:26:25.0 +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.0 +
+++ linux-work/drivers/pci/vga.c 2005-03-08 18:04:57.0 +1100
@@ -0,0 +1,403 @@
+static LIST_HEAD(vga_list);
Please remove whitespace damage.
+static spinlock_tvga_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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at
Re: [PATCH] VGA arbitration: draft of kernel side
On Tuesday, March 8, 2005 9:58 pm, Jon Smirl wrote: On Wed, 09 Mar 2005 16:37:13 +1100, Benjamin Herrenschmidt [EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] VGA arbitration: draft of kernel side
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] VGA arbitration: draft of kernel side
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
[EMAIL PROTECTED] wrote:
Index: linux-work/include/linux/pci.h
===
--- linux-work.orig/include/linux/pci.h 2005-01-24 17:09:57.0
+1100
+++ linux-work/include/linux/pci.h 2005-03-08 15:26:25.0 +1100
@@ -1064,5 +1064,6 @@
#define PCIPCI_VSFX16
#define PCIPCI_ALIMAGIK32
+
#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.0 +
+++ linux-work/drivers/pci/vga.c2005-03-08 18:04:57.0 +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.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] VGA arbitration: draft of kernel side
On Wed, 2005-03-09 at 00:58 -0500, Jon Smirl wrote: > On Wed, 09 Mar 2005 16:37:13 +1100, Benjamin Herrenschmidt > <[EMAIL PROTECTED]> 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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] VGA arbitration: draft of kernel side
On Wed, 09 Mar 2005 16:37:13 +1100, Benjamin Herrenschmidt <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] VGA arbitration: draft of kernel side
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/:17/ > |-- bridge -> ../../../devices/pci: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. 0xa 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 <[EMAIL PROTECTED]> - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] VGA arbitration: draft of kernel side
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/:17/ |-- bridge -> ../../../devices/pci: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. 0xa 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 [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] VGA arbitration: draft of kernel side
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.
*
* ":decodes=,owns=,locks= (mc,ic)"
*
* write: write a command to the arbiter. List of commands is:
*
* target: switch target to card (see below)
* lock : acquires locks on target ("none" is invalid io_state)
* trylock : non-blocking acquire locks on target
* unlock : release locks on target
* decodes : 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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] VGA arbitration: draft of kernel side
How do I do the 'disable all, post, renable last active' sequence in this scheme? -- Jon Smirl [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] VGA arbitration: draft of kernel side
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] VGA arbitration: draft of kernel side
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
[EMAIL PROTECTED]
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);
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, );
}
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 <[EMAIL PROTECTED]>
+ *
+ * VGA control logic for ensuring only a single enabled VGA device
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+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;
+
Re: [PATCH] VGA arbitration: draft of kernel side
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,
> > );
> > + 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(>list, _list);
> > + spin_unlock_irqrestore(_lock, flags);
>
> Missing return?
Yup.
> > + fail:
> > + spin_unlock_irqrestore(_lock, flags);
> > + kfree(vgadev);
> > +}
> > +
> > +void vga_arbiter_del_pci_device(struct pci_dev *pdev)
> > +{
> > + struct vga_device *vgadev;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(_lock, flags);
> > + vgadev = vgadev_find(pdev);
> > + if (vgadev == NULL)
> > + goto bail;
> > + if (vgadev->locks)
> > + __vga_put(vgadev, vgadev->locks);
> > + list_del(>list);
> > + bail:
> > + spin_unlock_irqrestore(_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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] VGA arbitration: draft of kernel side
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] VGA arbitration: draft of kernel side
Benjamin Herrenschmidt <[EMAIL PROTECTED]> 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.0 +
> +++ linux-work/drivers/pci/vga.c2005-03-08 18:04:57.0 +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, );
> + 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,
> );
> + 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, );
> + 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,
> );
> + 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(_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, );
> + 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, );
> +
Re: [PATCH] VGA arbitration: draft of kernel side
Benjamin Herrenschmidt [EMAIL PROTECTED] 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.0 +
+++ linux-work/drivers/pci/vga.c2005-03-08 18:04:57.0 +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;
+
Re: [PATCH] VGA arbitration: draft of kernel side
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] VGA arbitration: draft of kernel side
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.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] VGA arbitration: draft of kernel side
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
[EMAIL PROTECTED]
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 [EMAIL PROTECTED]
+ *
+ * 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,
Re: [PATCH] VGA arbitration: draft of kernel side
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] VGA arbitration: draft of kernel side
How do I do the 'disable all, post, renable last active' sequence in this scheme? -- Jon Smirl [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] VGA arbitration: draft of kernel side
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.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] VGA arbitration: draft of kernel side
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/:17/ |-- bridge - ../../../devices/pci: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. 0xa 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 [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] VGA arbitration: draft of kernel side
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/:17/ |-- bridge - ../../../devices/pci: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. 0xa 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 [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] VGA arbitration: draft of kernel side
On Wed, 09 Mar 2005 16:37:13 +1100, Benjamin Herrenschmidt [EMAIL PROTECTED] 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 [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] VGA arbitration: draft of kernel side
On Wed, 2005-03-09 at 00:58 -0500, Jon Smirl wrote: On Wed, 09 Mar 2005 16:37:13 +1100, Benjamin Herrenschmidt [EMAIL PROTECTED] 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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] VGA arbitration: draft of kernel side
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.0 +1100
+++ linux-work/include/linux/pci.h 2005-03-08 15:26:25.0 +1100
@@ -1064,5 +1064,6 @@
#define PCIPCI_VSFX16
#define PCIPCI_ALIMAGIK32
+
#endif /* __KERNEL__ */
#endif /* LINUX_PCI_H */
Index: linux-work/drivers/pci/Makefile
===
--- linux-work.orig/drivers/pci/Makefile2005-01-24 17:09:38.0
+1100
+++ linux-work/drivers/pci/Makefile 2005-03-08 15:20:23.0 +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.c2005-01-24 17:09:38.0
+1100
+++ linux-work/drivers/pci/remove.c 2005-03-08 15:25:52.0 +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);
Index: linux-work/drivers/pci/pci.h
===
--- linux-work.orig/drivers/pci/pci.h 2005-01-24 17:09:38.0 +1100
+++ linux-work/drivers/pci/pci.h2005-03-08 15:26:51.0 +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.0 +
+++ linux-work/drivers/pci/vga.c2005-03-08 18:04:57.0 +1100
@@ -0,0 +1,403 @@
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#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_headlist;
+ struct pci_dev *pdev;
+ unsigned intdecodes;/* what does it decodes */
+ unsigned intowns; /* what does it owns */
+ unsigned intlocks; /* 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, _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
[PATCH] VGA arbitration: draft of kernel side
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.0 +1100
+++ linux-work/include/linux/pci.h 2005-03-08 15:26:25.0 +1100
@@ -1064,5 +1064,6 @@
#define PCIPCI_VSFX16
#define PCIPCI_ALIMAGIK32
+
#endif /* __KERNEL__ */
#endif /* LINUX_PCI_H */
Index: linux-work/drivers/pci/Makefile
===
--- linux-work.orig/drivers/pci/Makefile2005-01-24 17:09:38.0
+1100
+++ linux-work/drivers/pci/Makefile 2005-03-08 15:20:23.0 +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.c2005-01-24 17:09:38.0
+1100
+++ linux-work/drivers/pci/remove.c 2005-03-08 15:25:52.0 +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.0 +1100
+++ linux-work/drivers/pci/pci.h2005-03-08 15:26:51.0 +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.0 +
+++ linux-work/drivers/pci/vga.c2005-03-08 18:04:57.0 +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_headlist;
+ struct pci_dev *pdev;
+ unsigned intdecodes;/* what does it decodes */
+ unsigned intowns; /* what does it owns */
+ unsigned intlocks; /* 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
