: agpgart for i810 chipsets broken in 2.5.51

BUG Antonino Daplas 4 participants 18 messages Dec 10, 2002 Agpgart driver

Share & tools

Antonino Daplas

author

2002-12-10 09:20 UTC #0 lore raw

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.

Dave Jones

2002-12-10 13:03 UTC #1 lore raw

Quoted 4 lines · 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)

Yup, known problem. On my TODO.

Quoted 3 lines

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.

Quoted 1 line

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

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

Dave

Antonino Daplas

author

2002-12-10 13:46 UTC #2 lore raw

Quoted 8 lines · Dave Jones

Quoted 3 lines · Antonino Daplas
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?

Quoted 4 lines

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

Ok.

Tony

Dave Jones

2002-12-10 13:55 UTC #3 lore raw

Quoted 7 lines · Antonino Daplas

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

Quoted 3 lines

Quoted 2 lines
Quoted 1 line
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

Antonino Daplas

author

2002-12-10 14:33 UTC #4 lore raw

Quoted 4 lines · Dave Jones

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.

Quoted 1 line

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

Quoted 4 lines

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.

Quoted 4 lines

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

Antonino Daplas

author

2002-12-10 16:07 UTC #5 lore raw

In reply to Dave Jones

Quoted 9 lines · Dave Jones

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

Dave Jones

2002-12-10 16:15 UTC #6 lore raw

Quoted 6 lines · Antonino Daplas

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

Signature · 2 lines

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

Alan Cox

2002-12-10 16:24 UTC #7 lore raw

Quoted 9 lines · Dave Jones

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.

Dave Jones

2002-12-10 16:58 UTC #8 lore raw

Quoted 15 lines · Alan Cox

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

Quoted 8 lines

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

Antonino Daplas

author

2002-12-10 18:00 UTC #9 lore raw

Quoted 21 lines · Dave Jones

Quoted 20 lines · Alan Cox
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

Quoted 8 lines

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

drivers/char/agp/backend.c +20 -1

diff -Naur linux-2.5.51/drivers/char/agp/backend.c linux/drivers/char/agp/backend.c
@@ -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

drivers/char/agp/intel-agp.c +20 -1

diff -Naur linux-2.5.51/drivers/char/agp/intel-agp.c linux/drivers/char/agp/intel-agp.c
@@ -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);

Dave Jones

2002-12-10 18:13 UTC #10 lore raw

Quoted 4 lines · Antonino Daplas

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

Antonino Daplas

author

2002-12-10 18:23 UTC #11 lore raw

Quoted 3 lines · Dave Jones

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

Antonino Daplas

author

2002-12-10 16:35 UTC #12 lore raw

In reply to Dave Jones

Quoted 10 lines · Dave Jones

Quoted 6 lines · Antonino Daplas
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..

Quoted 8 lines

#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

Dave Jones

2002-12-10 14:41 UTC #13 lore raw

In reply to Antonino Daplas

Quoted 11 lines · Antonino Daplas

Quoted 8 lines · Dave Jones
Quoted 3 lines · Antonino Daplas
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

Antonino Daplas

author

2002-12-10 16:06 UTC #14 lore raw

Quoted 10 lines · Dave Jones

Quoted 9 lines · Antonino Daplas
Quoted 8 lines · Dave Jones
Quoted 3 lines · Antonino Daplas
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.

Quoted 8 lines

Quoted 2 lines
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

Dave Jones

2002-12-10 16:09 UTC #15 lore raw

Quoted 7 lines · Antonino Daplas

Quoted 5 lines
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

Antonino Daplas

author

2002-12-10 16:36 UTC #16 lore raw

Quoted 4 lines · Dave Jones

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

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
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

Antonino Daplas

author

2002-12-10 17:59 UTC #17 lore raw

In reply to Dave Jones

Quoted 4 lines · Dave Jones

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

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
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