[patch 2.6] remove PCI_BRIDGE_CTL_VGA handling from setup-bus.c

[patch 2.6] remove PCI_BRIDGE_CTL_VGA handling from setup-bus.c

[Ivan Kokshaysky]

The setup-bus code doesn't work correctly for configurations
with more than one display adapter in the same PCI domain.
This stuff actually is a leftover of an early 2.4 PCI setup code
and apparently it stopped working after some "bridge_ctl" changes.
So the best thing we can do is just to remove it and rely on the fact
that any firmware *has* to configure VGA port forwarding for the boot
display device properly.

But then we need to ensure that the bus->bridge_ctl will always
contain valid information collected at the probe time, therefore
the following change in pci_scan_bridge() is needed.

Signed-off-by: Ivan Kokshaysky <ink@jurassic.park.msu.ru>

--- 2.6.13-rc3/drivers/pci/probe.c	Thu Jul 14 11:09:52 2005
+++ linux/drivers/pci/probe.c	Thu Jul 14 11:22:06 2005
@@ -507,7 +507,7 @@ int __devinit pci_scan_bridge(struct pci
 		pci_write_config_dword(dev, PCI_PRIMARY_BUS, buses);
 
 		if (!is_cardbus) {
-			child->bridge_ctl = PCI_BRIDGE_CTL_NO_ISA;
+			child->bridge_ctl = bctl | PCI_BRIDGE_CTL_NO_ISA;
 			/*
 			 * Adjust subordinate busnr in parent buses.
 			 * We do this before scanning for children because
--- 2.6.13-rc3/drivers/pci/setup-bus.c	Thu Jul 14 11:09:52 2005
+++ linux/drivers/pci/setup-bus.c	Thu Jul 14 11:22:54 2005
@@ -51,8 +51,6 @@ pbus_assign_resources_sorted(struct pci_
 	struct resource_list head, *list, *tmp;
 	int idx;
 
-	bus->bridge_ctl &= ~PCI_BRIDGE_CTL_VGA;
-
 	head.next = NULL;
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		u16 class = dev->class >> 8;
@@ -62,10 +60,6 @@ pbus_assign_resources_sorted(struct pci_
 		    class == PCI_CLASS_BRIDGE_HOST)
 			continue;
 
-		if (class == PCI_CLASS_DISPLAY_VGA ||
-		    class == PCI_CLASS_NOT_DEFINED_VGA)
-			bus->bridge_ctl |= PCI_BRIDGE_CTL_VGA;
-
 		pdev_sort_resources(dev, &head);
 	}
 
@@ -509,12 +503,6 @@ pci_bus_assign_resources(struct pci_bus 
 
 	pbus_assign_resources_sorted(bus);
 
-	if (bus->bridge_ctl & PCI_BRIDGE_CTL_VGA) {
-		/* Propagate presence of the VGA to upstream bridges */
-		for (b = bus; b->parent; b = b->parent) {
-			b->bridge_ctl |= PCI_BRIDGE_CTL_VGA;
-		}
-	}
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		b = dev->subordinate;
 		if (!b)

Re: [patch 2.6] remove PCI_BRIDGE_CTL_VGA handling from setup-bus.c

[Jon Smirl]

On 7/14/05, Ivan Kokshaysky <ink@jurassic.park.msu.ru> wrote:
> The setup-bus code doesn't work correctly for configurations
> with more than one display adapter in the same PCI domain.
> This stuff actually is a leftover of an early 2.4 PCI setup code
> and apparently it stopped working after some "bridge_ctl" changes.
> So the best thing we can do is just to remove it and rely on the fact
> that any firmware *has* to configure VGA port forwarding for the boot
> display device properly.

This fixes my system where the VGA display device is on the second bus.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [patch 2.6] remove PCI_BRIDGE_CTL_VGA handling from setup-bus.c

[Russell King]

On Thu, Jul 14, 2005 at 03:53:44PM +0400, Ivan Kokshaysky wrote:
> The setup-bus code doesn't work correctly for configurations
> with more than one display adapter in the same PCI domain.
> This stuff actually is a leftover of an early 2.4 PCI setup code
> and apparently it stopped working after some "bridge_ctl" changes.
> So the best thing we can do is just to remove it and rely on the fact
> that any firmware *has* to configure VGA port forwarding for the boot
> display device properly.

What happens when there is no firmware?

I'm sure this code would not have been added had there not been a reason
for it.  Do we know why it was added?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [patch 2.6] remove PCI_BRIDGE_CTL_VGA handling from setup-bus.c

[Jon Smirl]

On 7/14/05, Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> On Thu, Jul 14, 2005 at 03:53:44PM +0400, Ivan Kokshaysky wrote:
> > The setup-bus code doesn't work correctly for configurations
> > with more than one display adapter in the same PCI domain.
> > This stuff actually is a leftover of an early 2.4 PCI setup code
> > and apparently it stopped working after some "bridge_ctl" changes.
> > So the best thing we can do is just to remove it and rely on the fact
> > that any firmware *has* to configure VGA port forwarding for the boot
> > display device properly.
> 
> What happens when there is no firmware?
> 
> I'm sure this code would not have been added had there not been a reason
> for it.  Do we know why it was added?

I'm don't think it has ever been working in the 2.6 series. If you are
getting rid of it get rid of the #define PCI_BRIDGE_CTL_VGA in pci.h
too since this code was the only user.

Looking at the code as written I don't think it would work on my
machine with multiple VGA devices on different buses. I use the system
BIOS to enable the one I want and it sets up the bridges.

This code is part of VGA arbitration which BenH is addressing with a
more globally comprehensive patch. Ben's code will probably replace
it.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [patch 2.6] remove PCI_BRIDGE_CTL_VGA handling from setup-bus.c

[Ivan Kokshaysky]

On Thu, Jul 14, 2005 at 10:07:34AM -0400, Jon Smirl wrote:
> I'm don't think it has ever been working in the 2.6 series. If you are
> getting rid of it get rid of the #define PCI_BRIDGE_CTL_VGA in pci.h
> too since this code was the only user.

No. The PCI_BRIDGE_CTL_VGA is not something artificial, it just describes
some well defined hardware bit in the p2p bridge config header, so anyone
working on VGA switching API will have to use it.

> This code is part of VGA arbitration which BenH is addressing with a
> more globally comprehensive patch. Ben's code will probably replace
> it.

Yes, I've heard Ben is working on this, but I've yet to see the code. ;-)
Any pointers?

Ivan.

Re: [patch 2.6] remove PCI_BRIDGE_CTL_VGA handling from setup-bus.c

[Jon Smirl]

On 7/14/05, Ivan Kokshaysky <ink@jurassic.park.msu.ru> wrote:
> On Thu, Jul 14, 2005 at 10:07:34AM -0400, Jon Smirl wrote:
> > I'm don't think it has ever been working in the 2.6 series. If you are
> > getting rid of it get rid of the #define PCI_BRIDGE_CTL_VGA in pci.h
> > too since this code was the only user.
> 
> No. The PCI_BRIDGE_CTL_VGA is not something artificial, it just describes
> some well defined hardware bit in the p2p bridge config header, so anyone
> working on VGA switching API will have to use it.

I had the wrong define, this is the one I was thinking of IORESOURCE_BUS_HAS_VGA

> > This code is part of VGA arbitration which BenH is addressing with a
> > more globally comprehensive patch. Ben's code will probably replace
> > it.
> 
> Yes, I've heard Ben is working on this, but I've yet to see the code. ;-)
> Any pointers?
> 
> Ivan.
> 


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [patch 2.6] remove PCI_BRIDGE_CTL_VGA handling from setup-bus.c

[Ivan Kokshaysky]

On Thu, Jul 14, 2005 at 06:39:43PM -0400, Jon Smirl wrote:
> I had the wrong define, this is the one I was thinking of IORESOURCE_BUS_HAS_VGA

Oh, I definitely agree about that one. It's been unused for a couple of
years, at least. Let's kill it, please.

Ivan.

Re: [patch 2.6] remove PCI_BRIDGE_CTL_VGA handling from setup-bus.c

[Benjamin Herrenschmidt]

On Fri, 2005-07-15 at 01:46 +0400, Ivan Kokshaysky wrote:
> On Thu, Jul 14, 2005 at 10:07:34AM -0400, Jon Smirl wrote:
> > I'm don't think it has ever been working in the 2.6 series. If you are
> > getting rid of it get rid of the #define PCI_BRIDGE_CTL_VGA in pci.h
> > too since this code was the only user.
> 
> No. The PCI_BRIDGE_CTL_VGA is not something artificial, it just describes
> some well defined hardware bit in the p2p bridge config header, so anyone
> working on VGA switching API will have to use it.
> 
> > This code is part of VGA arbitration which BenH is addressing with a
> > more globally comprehensive patch. Ben's code will probably replace
> > it.
> 
> Yes, I've heard Ben is working on this, but I've yet to see the code. ;-)
> Any pointers?

I posted an early untested implementation a while ago (I don't have it
at hand sorry) and then got distracted by other things. I'll be back on
it soon though. The main "issue" I have at the moment isn't the arbiter
itself, which is not too tricky, but making things cooperate with him.
That is mostly

 - Console subsystem &| vgacon. That needs some serious work to deal
with the fact that the HW may not be accessible due to arbitration
issues. I'm considering replacing the trylock() on the console semaphore
by a per-console lock() callback here, though I yet have to decide what
happens to data in the printk buffer if you have, for example, 2 console
drivers, one of them "eats" the data, but the second one fails (returns
-EAGAIN due to lost arbitration).

 - Existing X servers & other apps using fbdev and mmap'ing /dev/fb*
directly. The current fbdev API provides no arbitration mecanism and
existing X servers just bang the hardware and toggle VGA routing
directly. To work around that, I've started toying with the VT mode.
That is, define a new KD_GRAPHICS_NEW mode that is equivalent to today's
KD_GRAPHICS, and have the "old" KD_GRAPHICS "disengage" the arbiter.
When leaving the later mode, the arbiter should "recover" from whatever
userland and/or X did.

While I've been thinking about these & possible solutions, I didn't have
time to write any actual code. Without solving those issues, though, a
VGA arbiter is fairly useless.

Ben.

Re: [patch 2.6] remove PCI_BRIDGE_CTL_VGA handling from setup-bus.c

[Ivan Kokshaysky]

On Thu, Jul 14, 2005 at 02:53:27PM +0100, Russell King wrote:
> What happens when there is no firmware?

It shouldn't be a problem. These days we have a lot of arch hooks
in the PCI layer. I'd probably start with the following:

static void __init
pcibios_enable_p2p_vga_fwd(struct pci_dev *dev)
{
	u16 class = dev->class >> 8;
	struct pci_bus *b;

	if (class == PCI_CLASS_DISPLAY_VGA ||
	    class == PCI_CLASS_NOT_DEFINED_VGA) {
		for (b = dev->bus; b->parent; b = b->parent) {
			b->bridge_ctl |= PCI_BRIDGE_CTL_VGA;
		}
	}
}

DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, pcibios_enable_p2p_vga_fwd);

> I'm sure this code would not have been added had there not been a reason
> for it.  Do we know why it was added?

It was quite a few years ago, so I don't recall all details...
I think some old alphas cannot cope with VGA behind the p2p bridges.
But I also recall that this code was problematic on alpha with multiple
VGA cards.
Note that setup-bus code in 2.4 does more reasonable thing - it
enables VGA port forwarding *only* for the first (by PCI enumeration)
VGA device, and doesn't enable this for others (if any). Unlike 2.6 :-(

Ivan.

Re: [patch 2.6] remove PCI_BRIDGE_CTL_VGA handling from setup-bus.c

[Jon Smirl]

On 7/14/05, Ivan Kokshaysky <ink@jurassic.park.msu.ru> wrote:
> It shouldn't be a problem. These days we have a lot of arch hooks
> in the PCI layer. I'd probably start with the following:

You need to take this code into account, from arch/i386/pci/fixup.c

/*
 * Fixup to mark boot BIOS video selected by BIOS before it changes
 *
 * From information provided by "Jon Smirl" <jonsmirl@gmail.com>
 *
 * The standard boot ROM sequence for an x86 machine uses the BIOS
 * to select an initial video card for boot display. This boot video
 * card will have it's BIOS copied to C0000 in system RAM.
 * IORESOURCE_ROM_SHADOW is used to associate the boot video
 * card with this copy. On laptops this copy has to be used since
 * the main ROM may be compressed or combined with another image.
 * See pci_map_rom() for use of this flag. IORESOURCE_ROM_SHADOW
 * is marked here since the boot video device will be the only enabled
 * video device at this point.
 */

static void __devinit pci_fixup_video(struct pci_dev *pdev)
{
        struct pci_dev *bridge;
        struct pci_bus *bus;
        u16 config;

        if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
                return;

        /* Is VGA routed to us? */
        bus = pdev->bus;
        while (bus) {
                bridge = bus->self;
                if (bridge) {
                        pci_read_config_word(bridge, PCI_BRIDGE_CONTROL,
                                                &config);
                        if (!(config & PCI_BRIDGE_CTL_VGA))
                                return;
                }
                bus = bus->parent;
        }
        pci_read_config_word(pdev, PCI_COMMAND, &config);
        if (config & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
                pdev->resource[PCI_ROM_RESOURCE].flags |= IORESOURCE_ROM_SHADOW;
                printk(KERN_DEBUG "Boot video device is %s\n", pci_name(pdev));
        }
}
DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, pci_fixup_video);

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [patch 2.6] remove PCI_BRIDGE_CTL_VGA handling from setup-bus.c

[Ivan Kokshaysky]

On Thu, Jul 14, 2005 at 06:42:30PM -0400, Jon Smirl wrote:
> You need to take this code into account, from arch/i386/pci/fixup.c

Yes, I've seen that (nice code, btw :-). But my code snippet has
nothing to do with x86 or any particular architecture - it just
shows that some hypothetical platform that doesn't have working
PCI firmware won't be hurt heavily by that patch. ;-)

Ivan.

Re: [patch 2.6] remove PCI_BRIDGE_CTL_VGA handling from setup-bus.c

[Grant Grundler]

On Thu, Jul 14, 2005 at 02:53:27PM +0100, Russell King wrote:
> On Thu, Jul 14, 2005 at 03:53:44PM +0400, Ivan Kokshaysky wrote:
> > The setup-bus code doesn't work correctly for configurations
> > with more than one display adapter in the same PCI domain.
> > This stuff actually is a leftover of an early 2.4 PCI setup code
> > and apparently it stopped working after some "bridge_ctl" changes.
> > So the best thing we can do is just to remove it and rely on the fact
> > that any firmware *has* to configure VGA port forwarding for the boot
> > display device properly.
> 
> What happens when there is no firmware?

I helped test/add bridge_ctl patch but PARISC general does NOT
support VGA at this time.

> I'm sure this code would not have been added had there not been a reason
> for it.  Do we know why it was added?

It was a replacement for the previous hacks and should represent essentially
the same functionality. I suspect we just didn't care about (or test)
multiheaded gfx at the time. Certainly not on parisc. This was in 2000/2001
timeframe originally.

grant