[BUG]: agpgart for i810 chipsets broken in 2.5.51

[BUG]: agpgart for i810 chipsets broken in 2.5.51

[Antonino Daplas]

Hi,

1.  Agpgart is broken for i810, and perhaps some of the i815 family. The
following lines (linux/drivers/char/agp/backend.c:120)

	cap_ptr = pci_find_capability(dev, PCI_CAP_ID_AGP);
	if (cap_ptr == 0) <--- /* always 0 for the i810 */
		return -ENODEV;
	agp_bridge.capndx = cap_ptr;

will always fail because they really don't have AGP.  Commenting them
out in my case correctly initializes the gart.

2.  The i810 driver for Xfree86 will also fail to load because of
version mismatch (0.99 vs 1.0).  Rolling back the version corrects the
problem.

No patches because I don't want to uglify the code :-)

Tony

PS:  I'm not on the list, please CC me.

Re: [BUG]: agpgart for i810 chipsets broken in 2.5.51

[Dave Jones]

On Tue, Dec 10, 2002 at 05:21:29PM +0500, Antonino Daplas wrote:
 > Hi,
 > 
 > 1.  Agpgart is broken for i810, and perhaps some of the i815 family. The
 > following lines (linux/drivers/char/agp/backend.c:120)

Yup, known problem. On my TODO.

 > 2.  The i810 driver for Xfree86 will also fail to load because of
 > version mismatch (0.99 vs 1.0).  Rolling back the version corrects the
 > problem.

Ugh, that's great. So X has to be patched every time the agpgart code
gets a new revision ? That sounds really unpleasant.

 > No patches because I don't want to uglify the code :-)

I'll ping you when I have something to test.

        Dave

Re: [BUG]: agpgart for i810 chipsets broken in 2.5.51

[Antonino Daplas]

On Tue, 2002-12-10 at 18:11, Dave Jones wrote:
> On Tue, Dec 10, 2002 at 05:21:29PM +0500, Antonino Daplas wrote:
>  > 2.  The i810 driver for Xfree86 will also fail to load because of
>  > version mismatch (0.99 vs 1.0).  Rolling back the version corrects the
>  > problem.
> 
> Ugh, that's great. So X has to be patched every time the agpgart code
> gets a new revision ? That sounds really unpleasant.
> 
Actually, X is complaining that the kernel version was too old, crazy
no?

>  > No patches because I don't want to uglify the code :-)
> 
> I'll ping you when I have something to test.
> 
Ok.

Tony

Re: [BUG]: agpgart for i810 chipsets broken in 2.5.51

[Dave Jones]

On Tue, Dec 10, 2002 at 09:47:24PM +0500, Antonino Daplas wrote:

 > >  > 2.  The i810 driver for Xfree86 will also fail to load because of
 > >  > version mismatch (0.99 vs 1.0).  Rolling back the version corrects the
 > >  > problem.
 > > Ugh, that's great. So X has to be patched every time the agpgart code
 > > gets a new revision ? That sounds really unpleasant.
 > Actually, X is complaining that the kernel version was too old, crazy
 > no?

Very much so. I'm grabbing the X source right now to find out exactly
what games they're playing. Hopefully I'll not regret it too much.

 > >  > No patches because I don't want to uglify the code :-)
 > > I'll ping you when I have something to test.
 > Ok.

btw, iirc you were the guy who wanted agpgart initialised sooner
due to the way the i810 framebuffer worked ?
How much sooner are we talking ? What puzzles me, is looking
at the link order in the makefiles, agpgart should already be
getting initialised before the framebuffer code, but doesn't
seem to be for reasons unknown..

Another issue on this same case, is that for this to work out,
we'll need some kconfig magick so that if the framebuffer is 'Y'
rather than 'M' the i810 gart module must be forced to 'Y' too.

        Dave

Re: [BUG]: agpgart for i810 chipsets broken in 2.5.51

[Antonino Daplas]

On Tue, 2002-12-10 at 19:03, Dave Jones wrote:
> On Tue, Dec 10, 2002 at 09:47:24PM +0500, Antonino Daplas wrote:
> 
> btw, iirc you were the guy who wanted agpgart initialised sooner
> due to the way the i810 framebuffer worked ?
Yes.

> How much sooner are we talking ? What puzzles me, is looking
Here's part of dmesg:

fb: Intel(R) 810 Framebuffer Device v0.9.0, Tony Daplas
     Video RAM      : 4096K
     Mode           : 1024x768-8bpp@67Hz
     Acceleration   : enabled
     MTRR           : enabled
     External VGA   : disabled
     Video Timings  : VESA GTF (US)
Console: switching to colour frame buffer device 128x48
pty: 256 Unix98 ptys configured
Real Time Clock Driver v1.11
Non-volatile memory driver v1.2
Linux agpgart interface v0.99 (c) Dave Jones
agpgart: Detected an Intel i810 Chipset.
agpgart: Maximum main memory to use for agp memory: 149M
agpgart: AGP aperture is 64M @ 0xe8000000


> at the link order in the makefiles, agpgart should already be
> getting initialised before the framebuffer code, but doesn't
> seem to be for reasons unknown..
>
I'm also not sure why, and even in .built-in.o.cmd, drivers/char is
before drivers/video.  Perhaps the early console_init()?  I'll try to
rebuild my kernel without the framebuffer console and let you know.
 
> Another issue on this same case, is that for this to work out,
> we'll need some kconfig magick so that if the framebuffer is 'Y'
> rather than 'M' the i810 gart module must be forced to 'Y' too.
> 
Yes, FB_I810 depends on AGP_INTEL, so the i810fb option is disabled if
agpgart is disabled.

Tony

Re: [BUG]: agpgart for i810 chipsets broken in 2.5.51

[Antonino Daplas]

On Tue, 2002-12-10 at 19:03, Dave Jones wrote:
> On Tue, Dec 10, 2002 at 09:47:24PM +0500, Antonino Daplas wrote:

> 
> btw, iirc you were the guy who wanted agpgart initialised sooner
> due to the way the i810 framebuffer worked ?
> How much sooner are we talking ? What puzzles me, is looking
> at the link order in the makefiles, agpgart should already be
> getting initialised before the framebuffer code, but doesn't
> seem to be for reasons unknown..

Tried it with framebuffer console off, same results. Moved agp before vt
in char/Makefile, same. Even manually calling agp_init() doesn't work
because what's really needed is agp_intel_init(). 

Anyway, I guess I'll stick to what I'm doing right now, periodically do
an inter_module_get("drm_agp") until it becomes available.

Tony

Re: [BUG]: agpgart for i810 chipsets broken in 2.5.51

[Dave Jones]

On Wed, Dec 11, 2002 at 12:08:22AM +0500, Antonino Daplas wrote:
 > Tried it with framebuffer console off, same results. Moved agp before vt
 > in char/Makefile, same. Even manually calling agp_init() doesn't work
 > because what's really needed is agp_intel_init(). 
 > 
 > Anyway, I guess I'll stick to what I'm doing right now, periodically do
 > an inter_module_get("drm_agp") until it becomes available.

That's really quite icky. Even putting an..

#ifdef CONFIG_FRAMEBUFFER_I810
    dev = pci_find_blah..
    agp_intel_init(dev);
#endif

before console_init() call in init/main.c seems cleaner than that imo,
(and this is still quite gross).

        Dave

-- 
| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs

Re: [BUG]: agpgart for i810 chipsets broken in 2.5.51

[Alan Cox]

On Tue, 2002-12-10 at 16:23, Dave Jones wrote:
> That's really quite icky. Even putting an..
> 
> #ifdef CONFIG_FRAMEBUFFER_I810
>     dev = pci_find_blah..
>     agp_intel_init(dev);
> #endif
> 
> before console_init() call in init/main.c seems cleaner than that imo,
> (and this is still quite gross).

Given how fragile the AGP code can be I would much rather we had the AGP
continue to initialize late. If the AGP init function is something like


int agp_required(void)
{
	static int agp_inited = 0;

	if(!agp_inited)
	{
		agp_inited = 1;
		agp_do_real_init();
	}
}

module_init(agp_required);


Then the i810 fb driver can do

	agp_required();

and force the order change only if necessary.

Re: [BUG]: agpgart for i810 chipsets broken in 2.5.51

[Dave Jones]

On Tue, Dec 10, 2002 at 05:06:17PM +0000, Alan Cox wrote:
 > Given how fragile the AGP code can be I would much rather we had the AGP
 > continue to initialize late. If the AGP init function is something like
 > 
 > 
 > int agp_required(void)
 > {
 > 	static int agp_inited = 0;
 > 
 > 	if(!agp_inited)
 > 	{
 > 		agp_inited = 1;
 > 		agp_do_real_init();
 > 	}
 > }
 > 
 > module_init(agp_required);
 > 
 > 
 > Then the i810 fb driver can do
 > 
 > 	agp_required();
 > 
 > and force the order change only if necessary.

That works for me. It's not ideal, but it's the cleanest
solution suggested so far. I'll hack a check into
agp_init() to do this, which should allow us to close bug #20 [*]

        Dave

[*] http://bugzilla.kernel.org/show_bug.cgi?id=20

Re: [BUG]: agpgart for i810 chipsets broken in 2.5.51

[Antonino Daplas]

On Tue, 2002-12-10 at 22:05, Dave Jones wrote:
> On Tue, Dec 10, 2002 at 05:06:17PM +0000, Alan Cox wrote:
>  > Given how fragile the AGP code can be I would much rather we had the AGP
>  > continue to initialize late. If the AGP init function is something like
>  > 
>  > 
>  > int agp_required(void)
>  > {
>  > 	static int agp_inited = 0;
>  > 
>  > 	if(!agp_inited)
>  > 	{
>  > 		agp_inited = 1;
>  > 		agp_do_real_init();
>  > 	}
>  > }
>  > 
>  > module_init(agp_required);
>  > 
>  > 
>  > Then the i810 fb driver can do
>  > 
>  > 	agp_required();
>  > 
>  > and force the order change only if necessary.
> 
> That works for me. It's not ideal, but it's the cleanest
> solution suggested so far. I'll hack a check into
> agp_init() to do this, which should allow us to close bug #20 [*]
> 

Oops, Alan beat me into it. It's basically the same as what I've got
except I had an i810fb specific macro.  I guess the one without the
macro is cleaner.

Tony

diff -Naur linux-2.5.51/drivers/char/agp/backend.c linux/drivers/char/agp/backend.c
--- linux-2.5.51/drivers/char/agp/backend.c	2002-12-10 20:46:58.000000000 +0000
+++ linux/drivers/char/agp/backend.c	2002-12-10 20:46:26.000000000 +0000
@@ -269,7 +269,7 @@
 	return 0;
 }
 
-int __init agp_init(void)
+int __init do_agp_init(void)
 {
 	memset(&agp_bridge, 0, sizeof(struct agp_bridge_data));
 	agp_bridge.type = NOT_SUPPORTED;
@@ -279,6 +279,25 @@
 	return 0;
 }
 
+#ifdef CONFIG_FB_I810
+int __init agp_init(void) 
+{
+	static int agp_has_init = 0;
+
+	if (agp_has_init)
+		return 0;
+
+	agp_has_init = 1;
+
+	return do_agp_init();
+}
+#else
+static int __init agp_init(void)
+{
+	return do_agp_init();
+}
+#endif
+
 #ifndef CONFIG_GART_IOMMU
 module_init(agp_init);
 #endif
diff -Naur linux-2.5.51/drivers/char/agp/intel-agp.c linux/drivers/char/agp/intel-agp.c
--- linux-2.5.51/drivers/char/agp/intel-agp.c	2002-12-10 20:47:03.000000000 +0000
+++ linux/drivers/char/agp/intel-agp.c	2002-12-10 20:46:23.000000000 +0000
@@ -1470,7 +1470,7 @@
 	.probe		= agp_intel_probe,
 };
 
-static int __init agp_intel_init(void)
+static int __init do_agp_intel_init(void)
 {
 	int ret_val;
 
@@ -1487,6 +1487,25 @@
 	pci_unregister_driver(&agp_intel_pci_driver);
 }
 
+#ifdef CONFIG_FB_I810
+int __init agp_intel_init(void) 
+{
+	static int intel_agp_has_init = 0;
+
+	if (intel_agp_has_init)
+		return 0;
+
+	intel_agp_has_init = 1;
+
+	return do_agp_intel_init();
+}
+#else
+static int __init agp_intel_init(void)
+{
+	return do_agp_intel_init();
+}
+#endif
+
 module_init(agp_intel_init);
 module_exit(agp_intel_cleanup);
 

Re: [BUG]: agpgart for i810 chipsets broken in 2.5.51

[Dave Jones]

On Wed, Dec 11, 2002 at 02:00:47AM +0500, Antonino Daplas wrote:
 > 
 > Oops, Alan beat me into it. It's basically the same as what I've got
 > except I had an i810fb specific macro.  I guess the one without the
 > macro is cleaner.

I did something similar in my pending tree, but I just made it
unconditional instead of littering lots of ifdefs.

        Dave

Re: [BUG]: agpgart for i810 chipsets broken in 2.5.51

[Antonino Daplas]

On Tue, 2002-12-10 at 23:21, Dave Jones wrote:
> On Wed, Dec 11, 2002 at 02:00:47AM +0500, Antonino Daplas wrote:
> I did something similar in my pending tree, but I just made it
> unconditional instead of littering lots of ifdefs.

That's great.  Thanks. 

Tony

Re: [BUG]: agpgart for i810 chipsets broken in 2.5.51

[Antonino Daplas]

On Tue, 2002-12-10 at 21:23, Dave Jones wrote:
> On Wed, Dec 11, 2002 at 12:08:22AM +0500, Antonino Daplas wrote:
>  > Tried it with framebuffer console off, same results. Moved agp before vt
>  > in char/Makefile, same. Even manually calling agp_init() doesn't work
>  > because what's really needed is agp_intel_init(). 
>  > 
>  > Anyway, I guess I'll stick to what I'm doing right now, periodically do
>  > an inter_module_get("drm_agp") until it becomes available.
> 
> That's really quite icky. Even putting an..
> 
> #ifdef CONFIG_FRAMEBUFFER_I810
>     dev = pci_find_blah..
>     agp_intel_init(dev);
> #endif
> 
> before console_init() call in init/main.c seems cleaner than that imo,
> (and this is still quite gross).
> 
That's fine by me.  It's basically what I'm doing in 2.4, except that I
call it directly from the driver.  I'm just not in the habit of touching
other people's code.  

agp_intel_init() will be called twice though, first by i810fb, then the
usual inits, so a flag has to be set once the function is called.

If it's okay with you, I'll do it that way (and save me some 100 lines
of code in the process :-), and submit a patch.

Tony 

Re: [BUG]: agpgart for i810 chipsets broken in 2.5.51

[Dave Jones]

On Tue, Dec 10, 2002 at 09:47:24PM +0500, Antonino Daplas wrote:
 > On Tue, 2002-12-10 at 18:11, Dave Jones wrote:
 > > On Tue, Dec 10, 2002 at 05:21:29PM +0500, Antonino Daplas wrote:
 > >  > 2.  The i810 driver for Xfree86 will also fail to load because of
 > >  > version mismatch (0.99 vs 1.0).  Rolling back the version corrects the
 > >  > problem.
 > > 
 > > Ugh, that's great. So X has to be patched every time the agpgart code
 > > gets a new revision ? That sounds really unpleasant.
 > > 
 > Actually, X is complaining that the kernel version was too old, crazy
 > no?

That chunk of X code is crap. So much so, that someone even put a
comment there (not that what they suggested was much better).

See line 122 of http://www.atomised.org/docs/XFree86-4.2.1/agp_8c-source.html

        Dave

Re: [BUG]: agpgart for i810 chipsets broken in 2.5.51

[Antonino Daplas]

On Tue, 2002-12-10 at 19:48, Dave Jones wrote:
> On Tue, Dec 10, 2002 at 09:47:24PM +0500, Antonino Daplas wrote:
>  > On Tue, 2002-12-10 at 18:11, Dave Jones wrote:
>  > > On Tue, Dec 10, 2002 at 05:21:29PM +0500, Antonino Daplas wrote:
>  > >  > 2.  The i810 driver for Xfree86 will also fail to load because of
>  > >  > version mismatch (0.99 vs 1.0).  Rolling back the version corrects the
>  > >  > problem.
>  > > 
>  > > Ugh, that's great. So X has to be patched every time the agpgart code
>  > > gets a new revision ? That sounds really unpleasant.
>  > > 
>  > Actually, X is complaining that the kernel version was too old, crazy
>  > no?
> 
> That chunk of X code is crap. So much so, that someone even put a
> comment there (not that what they suggested was much better).
> 
> See line 122 of http://www.atomised.org/docs/XFree86-4.2.1/agp_8c-source.html
> 
Ouch.  That's a sh??ty version check.  And it has to be present from
4.0.0 to 4.2.1, and if they don't correct it, 4.3.0.
 
Tony

Re: [BUG]: agpgart for i810 chipsets broken in 2.5.51

[Dave Jones]

On Wed, Dec 11, 2002 at 12:07:45AM +0500, Antonino Daplas wrote:
 > > That chunk of X code is crap. So much so, that someone even put a
 > > comment there (not that what they suggested was much better).
 > > 
 > > See line 122 of http://www.atomised.org/docs/XFree86-4.2.1/agp_8c-source.html
 > > 
 > Ouch.  That's a sh??ty version check.  And it has to be present from
 > 4.0.0 to 4.2.1, and if they don't correct it, 4.3.0.

Andreas Schwab pointed out to me, that due to the broken boolean check,
I can bump the version to 0.100 and it'll work. At least until the
X folks change/remove that code.

        Dave

Re: [BUG]: agpgart for i810 chipsets broken in 2.5.51

[Antonino Daplas]

On Tue, 2002-12-10 at 21:17, Dave Jones wrote:
> On Wed, Dec 11, 2002 at 12:07:45AM +0500, Antonino Daplas wrote:
>  > > That chunk of X code is crap. So much so, that someone even put a
>  > > comment there (not that what they suggested was much better).
>  > > 
>  > > See line 122 of http://www.atomised.org/docs/XFree86-4.2.1/agp_8c-source.html
>  > > 
>  > Ouch.  That's a sh??ty version check.  And it has to be present from
>  > 4.0.0 to 4.2.1, and if they don't correct it, 4.3.0.
> 
> Andreas Schwab pointed out to me, that due to the broken boolean check,
> I can bump the version to 0.100 and it'll work. At least until the
> X folks change/remove that code.

Okay, I'll bump the minor version and see if it actually works.

Tony

Re: [BUG]: agpgart for i810 chipsets broken in 2.5.51

[Antonino Daplas]

On Tue, 2002-12-10 at 21:17, Dave Jones wrote:
> On Wed, Dec 11, 2002 at 12:07:45AM +0500, Antonino Daplas wrote:
>  > > That chunk of X code is crap. So much so, that someone even put a
>  > > comment there (not that what they suggested was much better).
>  > > 
>  > > See line 122 of http://www.atomised.org/docs/XFree86-4.2.1/agp_8c-source.html
>  > > 
>  > Ouch.  That's a sh??ty version check.  And it has to be present from
>  > 4.0.0 to 4.2.1, and if they don't correct it, 4.3.0.
> 
> Andreas Schwab pointed out to me, that due to the broken boolean check,
> I can bump the version to 0.100 and it'll work. At least until the
> X folks change/remove that code.

I've tested using version 0.100 and it works just fine.

Tony