New radeonfb, mostly untested

Hi !

Ok, enough for today, my head hurts, it builds, it detects my PowerBook's internal panel properly, I didn't test anything else yet nor x86, let me know what you think...

No dual head nor mirror yet, though I try hard to detect properly both monitors.

Ben.

diff -urN linux-2.5-mesa/drivers/video/Kconfig linuxppc-2.5-benh/drivers/video/Kconfig
@@ -659,8 +659,18 @@
 	  There is no need for enabling 'Matrox multihead support' if you have
 	  only one Matrox card in the box.
 
+config FB_RADEON_OLD
+	tristate "ATI Radeon display support (Old driver)"
+	depends on FB && PCI
+	help
+	  Choose this option if you want to use an ATI Radeon graphics card as
+	  a framebuffer device.  There are both PCI and AGP versions.  You
+	  don't need to choose this to run the Radeon in plain VGA mode.
+	  There is a product page at
+	  <http://www.ati.com/na/pages/products/pc/radeon32/index.html>.
+
 config FB_RADEON
-	tristate "ATI Radeon display support"
+	tristate "ATI Radeon display support (New driver)"
 	depends on FB && PCI
 	help
 	  Choose this option if you want to use an ATI Radeon graphics card as

@@ -673,7 +683,7 @@
 
 	  If you say M here then "I2C support" and "I2C bit-banging support" 
 	  can be build either as modules or built-in.
-	  
+
 	  There is a product page at
 	  <http://www.ati.com/na/pages/products/pc/radeon32/index.html>.
 

diff -urN linux-2.5-mesa/drivers/video/Makefile linuxppc-2.5-benh/drivers/video/Makefile
@@ -21,7 +21,7 @@
 obj-$(CONFIG_FB_Q40)              += q40fb.o cfbfillrect.o cfbcopyarea.o cfbimgblt.o
 obj-$(CONFIG_FB_ATARI)            += atafb.o
 obj-$(CONFIG_FB_68328)            += 68328fb.o
-obj-$(CONFIG_FB_RADEON)		  += radeonfb.o cfbfillrect.o cfbcopyarea.o cfbimgblt.o
+obj-$(CONFIG_FB_RADEON_OLD)	  += radeonfb.o cfbfillrect.o cfbcopyarea.o cfbimgblt.o
 obj-$(CONFIG_FB_NEOMAGIC)         += neofb.o cfbfillrect.o cfbcopyarea.o cfbimgblt.o
 obj-$(CONFIG_FB_IGA)              += igafb.o cfbfillrect.o cfbcopyarea.o cfbimgblt.o
 obj-$(CONFIG_FB_CONTROL)          += controlfb.o macmodes.o cfbfillrect.o cfbcopyarea.o cfbimgblt.o

@@ -54,12 +54,14 @@
 obj-$(CONFIG_FB_PMAGB_B)          += pmagb-b-fb.o cfbfillrect.o cfbcopyarea.o cfbimgblt.o
 obj-$(CONFIG_FB_MAXINE)           += maxinefb.o cfbfillrect.o cfbcopyarea.o cfbimgblt.o
 obj-$(CONFIG_FB_TX3912)           += tx3912fb.o cfbfillrect.o cfbcopyarea.o cfbimgblt.o
+obj-$(CONFIG_FB_IBMLCDC)	  += ibmlcdfb.o
 
 obj-$(CONFIG_FB_MATROX)		  += matrox/ cfbfillrect.o cfbcopyarea.o cfbimgblt.o
 obj-$(CONFIG_FB_RIVA)		  += riva/ cfbimgblt.o vgastate.o 
 obj-$(CONFIG_FB_SIS)		  += sis/ cfbcopyarea.o cfbfillrect.o cfbimgblt.o
 obj-$(CONFIG_FB_ATY)		  += aty/ cfbcopyarea.o cfbfillrect.o cfbimgblt.o
 obj-$(CONFIG_FB_ATY128)		  += aty/ cfbcopyarea.o cfbfillrect.o cfbimgblt.o
+obj-$(CONFIG_FB_RADEON)		  += aty/ cfbcopyarea.o cfbfillrect.o cfbimgblt.o
 obj-$(CONFIG_FB_I810)             += i810/ cfbfillrect.o cfbcopyarea.o \
 	                             cfbimgblt.o vgastate.o
 

Part of the diff is not shown here. View the raw message.

Il Sun, Sep 07, 2003 at 06:37:20PM +0200, Benjamin Herrenschmidt ha scritto:

+unmap_rom:
+ if (rinfo->mon1_EDID)
+ kfree(rinfo->mon1_EDID);
+ if (rinfo->mon2_EDID)
+ kfree(rinfo->mon2_EDID);
+ if (rinfo->mon1_modedb)
+ fb_destroy_modedb(rinfo->mon1_modedb);
+ radeon_delete_i2c_busses(rinfo);
+ if (rinfo->bios_seg)
+ radeon_unmap_ROM(rinfo, pdev);
+ iounmap ((void*)rinfo->mmio_base);
+release_mmio:
+ release_mem_region (rinfo->mmio_base_phys,
+ pci_resource_len(pdev, 2));
+release_fb:
+ release_mem_region (rinfo->fb_base_phys,
+ pci_resource_len(pdev, 0));
+free_rinfo:
+ kfree (rinfo);
+ return -ENODEV;
+}
+
+
+
+static void __devexit radeonfb_pci_unregister (struct pci_dev *pdev) +{
+ struct radeonfb_info *rinfo = pci_get_drvdata(pdev);
+
+ if (!rinfo)
+ return;
+
+ /* restore original state
+ *
+ * Doesn't quite work yet, possibly because of the PPC hacking
+ * I do on startup, disable for now. --BenH
+ */
+ radeon_write_mode (rinfo, &rinfo->init_state);
+
+#ifdef CONFIG_MTRR
+ if (rinfo->mtrr_hdl >= 0)
+ mtrr_del(rinfo->mtrr_hdl, 0, 0);
+#endif
+
+ unregister_framebuffer ((struct fb_info *) rinfo);
+
+ iounmap ((void*)rinfo->mmio_base);
+ iounmap ((void*)rinfo->fb_base);
+
+ release_mem_region (rinfo->mmio_base_phys,
+ pci_resource_len(pdev, 2));
+ release_mem_region (rinfo->fb_base_phys,
+ pci_resource_len(pdev, 0));
+
+ if (rinfo->mon1_EDID)
+ kfree(rinfo->mon1_EDID);
+ if (rinfo->mon2_EDID)
+ kfree(rinfo->mon2_EDID);
+ if (rinfo->mon1_modedb)
+ fb_destroy_modedb(rinfo->mon1_modedb);
+ radeon_delete_i2c_busses(rinfo);
+
+ kfree (rinfo);
+}

There's a problem here: sysfs attributes are unregistered after radeonfb_pci_unregister returns, so we have a small windows where mon1_EDID and mon2_EDID are invalid and edid1 and edid2 are still there in sysfs. Ditto for pci_register.

Oh, you didn't update aty/Makefile ;)

obj-$(CONFIG_FB_RADEON) += radeonfb.o radeonfb-y += radeon_base.o radeon_monitor.o radeon_pm.o radeonfb-$(CONFIG_FB_RADEON_I2C) += radeon_i2c.o

Luca

Reply-To: [email protected]
Home: http://kronoz.cjb.net
Software is like sex; it's better when it's free.
Linus Torvalds


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Yup, there is much "polish" still needed on the exit/cleanup path, though I'm waiting to see what you come up with putting class_device in there :)

It's here, not in the patch, sorry.

I won't have time to work more on this until later next week, so feel free to send your own patches/fixes/etc... If you feel it's ready to push to the mesa bk tree, let me know.

Ben.

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Il Sun, Sep 07, 2003 at 08:23:48PM +0200, Benjamin Herrenschmidt ha scritto:

edid1 and edid2 will be attached to the class_device, they will be unregistered before the "release". The problem will go away.

Btw, the class_device stuff won't go in, it's too late. I'll maintain it off tree and submit around 2.6.2.

I'm compiling it right now :)

Luca

Reply-To: [email protected]
Home: http://kronoz.cjb.net
"Di tutte le perversioni sessuali, la castita` e` la piu` strana".
Anatole France


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

I don't think it's too late... on the other hand, James is still sitting on a bunch of patches (hey James !) that should get in asap now...

Ben.

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Its not to late. The fbdev layer has bug fixes that need to go in.

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Il Tue, Sep 09, 2003 at 06:18:21PM +0100, James Simmons ha scritto:

Ok, but the class_dev stuff isn't a bug fix, it's an API change... If you really want it I can start sending patches against your bk tree.

Luca

Reply-To: [email protected]
Home: http://kronoz.cjb.net
Windows NT crashed.
I'm the Blue Screen of Death.
No one hears your screams.


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

If it's easy enough to fix existing drivers, we can cope with it, that wouldn't be the first similar breakage we did during 2.6-test and I beleive it's actually bringing sanity in :-)

Ben.

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Il Tue, Sep 09, 2003 at 08:13:19PM +0200, Benjamin Herrenschmidt ha scritto:

Most of work consists in changing "." into "->" ;) Changes to fbmem.c are very similar to Greg KH's patch:

===== drivers/video/fbmem.c 1.80 vs edited =====

@@ -1199,6 +1199,42 @@
 #endif
 };
 
+#define to_fb_info(d) container_of(d, struct fb_info, class_dev)
+static void release_fb_info(struct class_device *class_dev) {
+	struct fb_info *info = to_fb_info(class_dev);
+
+	if (info->release)
+		info->release(info);
+	
+	kfree(info);
+}
+
+static struct class fb_class = {
+	.name		= "video",
+	.release	= &release_fb_info,
+};
+
+static ssize_t show_dev(struct class_device *class_dev, char *buf) {
+	struct fb_info *info = to_fb_info(class_dev);
+	return sprintf(buf, "%u:%u\n", FB_MAJOR, info->node);
+}
+
+static CLASS_DEVICE_ATTR(dev, S_IRUGO, show_dev, NULL);
+
+static void fb_add_class_device(struct fb_info *info) {
+	info->class_dev.class = &fb_class;
+	snprintf(info->class_dev.class_id, BUS_ID_SIZE, "fb%d", info->node);
+	
+	if (class_device_register(&info->class_dev))
+		return;
+	
+	class_device_create_file(&info->class_dev, &class_device_attr_dev);
+}
+
+void fb_remove_class_device(struct fb_info *info) {
+	class_device_unregister(&info->class_dev);
+}
+
 /**
  *	register_framebuffer - registers a frame buffer device
  *	@fb_info: frame buffer info structure

@@ -1242,6 +1278,7 @@
 
 	devfs_mk_cdev(MKDEV(FB_MAJOR, i),
 			S_IFCHR | S_IRUGO | S_IWUGO, "fb/%d", i);
+	fb_add_class_device(fb_info);
 	return 0;
 }
 

@@ -1270,6 +1307,7 @@
 		kfree(fb_info->pixmap.addr);
 	registered_fb[i]=NULL;
 	num_registered_fb--;
+	fb_remove_class_device(fb_info);
 	return 0;
 }
 

@@ -1293,6 +1331,8 @@
 	devfs_mk_dir("fb");
 	if (register_chrdev(FB_MAJOR,"fb",&fb_fops))
 		printk("unable to get major %d for fb devs\n", FB_MAJOR);
+
+	class_register(&fb_class);
 
 #ifdef CONFIG_FB_OF
 	if (ofonly) {

===== include/linux/fb.h 1.53 vs edited =====

@@ -3,6 +3,7 @@
 
 #include <linux/tty.h>
 #include <linux/workqueue.h>
+#include <linux/device.h>
 #include <asm/types.h>
 #include <asm/io.h>
 

@@ -412,6 +413,8 @@
    struct vc_data *display_fg;		/* Console visible on this display */
    int currcon;				/* Current VC. */	
    void *pseudo_palette;                /* Fake palette of 16 colors */ 
+   struct class_device class_dev;
+   void (*release)(struct fb_info *);	/* Do driver specific clean up */
    /* From here on everything is device dependent */
    void *par;	
 };

And this is radeonfb (the one from mesa3d):

===== drivers/video/radeonfb.c 1.44 vs edited =====

sysfs attributes can then be attacched to class_dev, they will show up under /sys/class/video/fbX/.

Luca

Reply-To: [email protected]
Home: http://kronoz.cjb.net
Recursion n.:
	See Recursion.


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

You probably want to fix more drivers , that would help getting it merged. I will take care of all the pmac ones (controlfb, platinumfb, valkyriefb) and I can take care of aty128fb, chipsfb and radeonfb too if necessary.

Some comments though:

Il Tue, Sep 09, 2003 at 09:37:17PM +0200, Benjamin Herrenschmidt ha scritto:

I sent radeonfb as an example. I've already fixed chipsfb.c, cyber2000fb.c, hgafb.c, imsttfb.c, neofb.c, tdfxfb.c, tridentfb.c, vesafb.c and i810.

Implicit? What do you mean?

Like this?

struct fb_info *framebuffer_alloc(size_t size, struct device *dev) {
        struct fb_info *ret;

        ret = kmalloc(sizeof(struct fb_info) + size, GFP_KERNEL);
        if (!ret)
                return NULL;
        memset(ret, 0, sizeof(struct fb_info) + size);

        ret->class_dev.dev = dev;

        if (size)
                ret->par = ret + 1;

        return ret;
}

Ok.

Luca

Reply-To: [email protected]
Home: http://kronoz.cjb.net
K.R.O.N.O.S
Kinetic Replicant Optimized for Nocturnal Observation and Sabotage


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Except I would also make sure that there is some alignement of the private part, if possible on L1 cache line size

Ben.

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Il Tue, Sep 09, 2003 at 11:07:31PM +0200, Benjamin Herrenschmidt ha scritto:

struct fb_info *framebuffer_alloc(size_t size, struct device *dev) {
        unsigned char *ret;
        struct fb_info *info;
        int align;

        if (size)
                align = L1_CACHE_BYTES - (sizeof(struct fb_info) % L1_CACHE_BYTES);
        else
                align = 0;
        
        ret = kmalloc(sizeof(struct fb_info) + align + size, GFP_KERNEL);
        if (!ret)
                return NULL;
        memset(ret, 0, sizeof(struct fb_info) + size + align);
        info = (struct fb_info *)ret;
        info->class_dev.dev = dev;

        if (size)
                info->par = ret + align;

        return info;
}

How does it look? Is there a smarter way to do it?

Luca

Reply-To: [email protected]
Home: http://kronoz.cjb.net
The trouble with computers is that they do what you tell them, 
not what you want.
D. Cohen


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Il Tue, Sep 09, 2003 at 11:52:29PM +0200, Kronos ha scritto:

Ops, this should be:

                info->par = ret + sizeof(struct fb_info) + align;

Luca

Reply-To: [email protected]
Home: http://kronoz.cjb.net
Recursion n.:
        See Recursion.


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Yucky!!!

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Patches welcomed. I already have the class_device stuff in my tree. Take a look at the fbdev BK. Please make a patch aginat that tree.

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Il Fri, Sep 12, 2003 at 06:48:30PM +0100, James Simmons ha scritto:

I've just finished ;) I want to double-check everything, I'll send patches ASAP.

Luca

Reply-To: [email protected]
Home: http://kronoz.cjb.net
Let me make your mind, leave yourself behind
Be not afraid


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Exactly my comment. I was thinking about graphics' instead of video', but indeed `display' sounds better.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

I prefere graphics myself. Display sounds to generic. That is what video and graphics output is piped to. Since fbdev doesn't handle video ouput normally this is kind of fuzzy sounding.

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Il Sun, Sep 07, 2003 at 06:37:20PM +0200, Benjamin Herrenschmidt ha scritto:

It dies on boot:

ksymoops 2.4.9 on i686 2.6.0-test4-benh. Options used
-V (default)
-k /proc/ksyms (default)
-l /proc/modules (default)
-o /lib/modules/2.6.0-test4-benh/ (default)
-m /home/kronos/c/src/linux-2.5-benh/System.map (specified)

Error (regular_file): read_ksyms stat /proc/ksyms failed No modules in ksyms, skipping objects
No ksyms, skipping lsmod

Unable to handle kernel NULL pointer dereference at virtual address 00000014
c02e33c7
*pde = 00000000
Oops: 0000 [#1]
CPU:    0
EIP:    0060:[<c02e33c7>]    Not tainted

Using defaults from ksymoops -t elf32-i386 -a i386
EFLAGS: 00010202
eax: 00000000 ebx: ee49c010 ecx: 00000001 edx: ef631dd4
esi: ee49c000 edi: 00000000 ebp: ef631d90 esp: ef631d80
ds: 007b es: 007b ss: 0068

Stack: ffffffff ee49c000 ee49c000 ef631dd4 ef631e60 f09442f0 ee49c010 ee49c000 
       ef631dd4 00000008 ef631de4 ee49c010 00000000 00003a55 00000400 c0399bcc 
       ef631e0c ef631e14 c012b837 ef630000 fffffaf8 00000286 c046e445 00000280 
Call Trace:
 [<f09442f0>] radeon_check_modes+0x240/0x390 [radeonfb]
 [<c012b837>] printk+0x277/0x400
 [<c02e2315>] show_edid+0x145/0x1b0
 [<f0943af3>] radeon_probe_screens+0x133/0x5f0 [radeonfb]
 [<f0941709>] radeonfb_pci_register+0x419/0x800 [radeonfb]
 [<c0216f6b>] pci_device_probe_static+0x4b/0x60
 [<c02172d6>] __pci_device_probe+0x36/0x50
 [<c021731c>] pci_device_probe+0x2c/0x50
 [<c02a2bbd>] bus_match+0x3d/0x70
 [<c02a2d10>] driver_attach+0x70/0xb0
 [<c02a305e>] bus_add_driver+0xbe/0xd0
 [<c02a3568>] driver_register+0x88/0x90
 [<c021771e>] pci_register_driver+0x8e/0xb0
 [<f094a015>] init_module+0x15/0x40 [radeonfb]
 [<c014d0cf>] sys_init_module+0x1ff/0x3e0
 [<c010b20f>] syscall_call+0x7/0xb
Code: 8b 50 14 85 d2 75 12 83 63 54 fd 31 c0 85 c9 0f 94 c0 83 c4 

Trace; f09442f0 <_end+304b9228/3fb72f38>
Trace; c012b837 <printk+277/400>
Trace; c02e2315 <show_edid+145/1b0>
Trace; f0943af3 <_end+304b8a2b/3fb72f38>
Trace; f0941709 <_end+304b6641/3fb72f38>
Trace; c0216f6b <pci_device_probe_static+4b/60>
Trace; c02172d6 <__pci_device_probe+36/50>
Trace; c021731c <pci_device_probe+2c/50>
Trace; c02a2bbd <bus_match+3d/70>
Trace; c02a2d10 <driver_attach+70/b0>
Trace; c02a305e <bus_add_driver+be/d0>
Trace; c02a3568 <driver_register+88/90>
Trace; c021771e <pci_register_driver+8e/b0>
Trace; f094a015 <_end+304bef4d/3fb72f38>
Trace; c014d0cf <sys_init_module+1ff/3e0>
Trace; c010b20f <syscall_call+7/b>

Code; c02e33c7 <__fb_try_mode+87/c0>
00000000 <_EIP>:
Code; c02e33c7 <__fb_try_mode+87/c0> <=====
0: 8b 50 14 mov 0x14(%eax),%edx <=====
Code; c02e33ca <__fb_try_mode+8a/c0>
3: 85 d2 test %edx,%edx
Code; c02e33cc <__fb_try_mode+8c/c0>
5: 75 12 jne 19 <_EIP+0x19> c02e33e0 <__fb_try_mode+a0/c0>
Code; c02e33ce <__fb_try_mode+8e/c0>
7: 83 63 54 fd andl $0xfffffffd,0x54(%ebx)
Code; c02e33d2 <__fb_try_mode+92/c0>
b: 31 c0 xor %eax,%eax
Code; c02e33d4 <__fb_try_mode+94/c0>
d: 85 c9 test %ecx,%ecx
Code; c02e33d6 <__fb_try_mode+96/c0>
f: 0f 94 c0 sete %al
Code; c02e33d9 <__fb_try_mode+99/c0>
12: 83 c4 00 add $0x0,%esp

1 error issued. Results may not be reliable.

We are in modedb.c:411, and info->fbops is NULL. info->fbops is set in radeon_set_fbinfo, which is called after radeon_probe_screens. Moving radeon_set_fbinfo up should fix this:

===== drivers/video/aty/radeon_base.c 1.2 vs edited =====

@@ -2003,6 +2003,9 @@
 	radeon_create_i2c_busses(rinfo);
 #endif
 
+	/* set all the vital stuff */
+	radeon_set_fbinfo (rinfo);
+
 	/* Probe screen types */
 	radeon_probe_screens(rinfo, monitor_layout, ignore_edid);
 

@@ -2024,9 +2027,6 @@
 		/* initialize the engine */
 		radeon_engine_init (rinfo);
 	}
-
-	/* set all the vital stuff */
-	radeon_set_fbinfo (rinfo);
 
 	pci_set_drvdata(pdev, rinfo);
 	rinfo->next = board_list;

Luca

Reply-To: [email protected]
Home: http://kronoz.cjb.net
"It is more complicated than you think"
                -- The Eighth Networking Truth from RFC 1925


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Well... radeon_set_fbinfo relies on the screens beeing probed, at least a couple of lines in it (the depth things) though they may not be that important... (copied from the old driver, I may just rip them off, I need to verify something relative to the call to engine_init, I will probably rip it off too, it has to be done in set_par only).

strangely enough it works here, I have to check what's wrong, I suspect fb_find_mode isn't getting called for me but is for you and it's trying to call check_var.

I'll fix that and send a new patch.

Ben.

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Il Mon, Sep 08, 2003 at 08:06:07AM +0200, Benjamin Herrenschmidt ha scritto:

I pulled you patch. Now it doesn't oops but fb_find_mode is doing something strange. I added some printk:

Starting monitor auto detection...
radeonfb: I2C (port 2) ... not found
radeonfb: I2C (port 4) ... not found
radeonfb: I2C (port 3) ... found CRT display
radeonfb: Monitor 1 type CRT found
radeonfb: EDID probed
[EDID stuff]
radeonfb: Monitor 2 type no found

We are in radeon_check_modes, just before it calls fb_find_mode:

radeonfb: mode_option: 1024x768-8@60
radeonfb: has_default_mode = 0
radeonfb: rinfo->mon1_dbsize 20
radeonfb: calling fb_find_mode
radeonfb: fb_find_mode -> 1
radeonfb: rinfo->info.var.xres = 640, rinfo->info.var.yres = 480

fb_find_mode returns 1 (which should mean that we are using specified mode_option), but info.var is set to something else...
I can't see what's going wrong here.

Btw, I see that you switched to module_param. Maybe we should leave the setup function (#ifndef MODULE), otherwise the driver won't recognise option passed via kernel command line (video=....). radeonfb.option=something will work, but it is a bit ugly...

Luca

Reply-To: [email protected]
Home: http://kronoz.cjb.net
Dicono che  il cane sia  il miglior  amico dell'uomo. Secondo me  non e`
vero. Quanti dei vostri amici avete fatto castrare, recentemente?


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Il Mon, Sep 08, 2003 at 09:05:27PM +0200, Kronos ha scritto:

It fails only if compiled as module. I see that fb_find_mode (and my_atoi) are marked as __init. Maybe they get discarded after boot and when radeonfb module tries to call them it ends up in deallocated memory...

When booting with radeonfb compiled in the screen is shifted on the left. At 1024x768 it is shifted by 2 characters:

hStart = 1048, hEnd = 1184, hTotal = 1344 vStart = 771, vEnd = 777, vTotal = 806 h_total_disp = 0x7f00a7 hsync_strt_wid = 0x910422 v_total_disp = 0x2ff0325 vsync_strt_wid = 0x860302 pixclock = 15384
freq = 6500
post div = 0x4
fb_div = 0x74
ppll_div_3 = 0x20074
Console: switching to colour frame buffer device 128x48

At 1280x768 (native res of my LCD - now it works) it is shifted by 3 chars:

Detailed Monitor Information
108 MHz 1280 1328 1440 1688 1024 1025 1028 1066 +HSync +VSync

Serial No : HD 001699
Monitor Name : Philips 170S4
HorizSync : 30-82 KHz
VertRefresh : 56-76 Hz
Max Pixelclock: 140 MHz
========================================
ATI Radeon NE 9700 R300 SDR SGRAM 128 MB hStart = 1359, hEnd = 1496, hTotal = 1713 vStart = 1025, vEnd = 1028, vTotal = 1060 h_total_disp = 0x9f00d5 hsync_strt_wid = 0x910559 v_total_disp = 0x3ff0423 vsync_strt_wid = 0x830400 pixclock = 9179
freq = 10894
post div = 0x2
fb_div = 0x61
ppll_div_3 = 0x10061
Console: switching to colour frame buffer device 160x64

This is very annoying because I must re-center the screen every time I switch from console to X and viceversa. I had the same issue with the old driver + i2c code.

That doesn't happen with the standard modedb. At 1024x768 with ignore_edid it's ok. At 1280x1024 it falls back to 800x600.

Luca

Reply-To: [email protected]
Home: http://kronoz.cjb.net
"I've seen things you people wouldn't believe...
 Attack Ships on fire off the shores of Orion.
 I've watched C-beams glitter in the dark off of Tanhauser Gate.
 All those moments will be lost in time...like tears, in rain.
 Time to die." -- Roy Batty (played by Rutger Hauer)


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Exactly, they should definitely not be marked __init

Can you try tweaking the hSync and vSync polarity ?

Ben.

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Il Wed, Sep 10, 2003 at 06:48:26PM +0200, Benjamin Herrenschmidt ha scritto:

Tried, I doesn't change. I'm looking at radeonfb_set_par. What does hSyncPol and vSyncPol do? They seem useless, they aren't used anywhere. I changed h_sync_pol and v_sync_pol (first one, then the other and finally both).

Luca

Reply-To: [email protected]
Home: http://kronoz.cjb.net
K.R.O.N.O.S
Kinetic Replicant Optimized for Nocturnal Observation and Sabotage


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

The ones from the panel infos ? we should use them indeed rather than the ones in the mode...

Anyway, if your EDID returns incorrect infos, then we are screwed, or maybe we aren't calculating the horizontal margin properly when building the mode.

How does XFree works for you ? You can try comparing what XFree programs in registers with what we do.

Ben.

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Il Wed, Sep 10, 2003 at 07:50:10PM +0200, Benjamin Herrenschmidt ha scritto:

My LCD is using the VGA port, panel_info isn't used.

XFree is ok, Windows idem.

I'm doing this right now.

Luca

Reply-To: [email protected]
Home: http://kronoz.cjb.net
Una donna sposa un uomo sperando che cambi, e lui non cambiera`. Un
uomo sposa una donna sperando che non cambi, e lei cambiera`.


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Il Wed, Sep 10, 2003 at 07:50:10PM +0200, Benjamin Herrenschmidt ha scritto:

This is the modedb created from my edid:
[cut]
mode 18
mode->xres = 1280, mode->right_margin = 79
mode->hsync_len = 137, mode->left_margin = 217
mode->vres = 1024, mode->lower_margin = 1
mode->vsync_len = 3, mode->upper_margin = 32
mode->refresh = 60
mode 19
mode->xres = 1280, mode->right_margin = 48
mode->hsync_len = 112, mode->left_margin = 248
mode->vres = 1024, mode->lower_margin = 1
mode->vsync_len = 3, mode->upper_margin = 38
mode->refresh = 60

mode 19 comes from get_detailed_timing, while 18 comes from get_std_timing and timings are calculated by calc_mode_timings (which in turn calls fb_get_mode). Any chance that there is some error in fb_get_mode?

I'll check XFree ASAP.

Luca

Reply-To: [email protected]
Home: http://kronoz.cjb.net
Al termine di un pranzo di nozze mi hanno dato un
amaro alle erbe cosi' schifoso che perfino sull'etichetta
c'era un frate che vomitava.


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

I don't know the details about mode calculation and I never had any VESA spec, ask James here...

On the other hand, we really want to use the detailed timings in preferrence to the calculated modes. James didn't reply for that, but I think I'll add a flag to the modes that tell how they were calculated. I need that for flat panels too so I don't have to parse twice and I can be smarter at picking the best LCD panel mode

Ben.

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

I might be missing something jumping in the middle of the discussing but there is a GTF paper on creating VESA modes including from stratch without edid. The modes generated are pretty close to the standard VESA modes.

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Il Fri, Sep 12, 2003 at 06:43:19PM +0100, James Simmons ha scritto:

The problem is that mode calculated by fb_get_mode is different from the one that comes from get_detailed_timing.

Also, modes from fb_get_mode are different from XFree modes so every time I switch from console to X I have to re-center the screen.

This happens only when modedb is created from edid, not when using the standard modedb.

Currently create_modedb is unused (the new radeon driver is the first user of edid stuff) so we may be hitting a real bug.

Luca

Reply-To: [email protected]
Home: http://kronoz.cjb.net
#include <stdio.h> 
int main(void) {printf("\t\t\b\b\b\b\b\b");
printf("\t\t\b\b\b\b\b\b");return 0;}


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

:-(

Sounds like bugs in the code. The code was never tested before.

BTW where can I find the latest Radeon code. I like to test it myself.

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Il Fri, Sep 12, 2003 at 11:20:13PM +0100, James Simmons ha scritto:

You can find it in Ben's tree: bk://ppc.bkbits.net/linuxppc-2.5-benh

It's easier to test it userspace, I've adapted fbmon.c and modedb.c to work in userspace (trivial) so that I can fopen the dump of edid block and pass it to create_modedb.

I'll send you my edid block if you need it.

Luca

Reply-To: [email protected]
Home: http://kronoz.cjb.net
E' stato a causa di una donna che ho cominciato a bere e non ho mai
avuto la cortesia di ringraziarla.


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Il Fri, Sep 12, 2003 at 11:20:13PM +0100, James Simmons ha scritto:

Look at this:

bc 1.05
Copyright 1991, 1992, 1993, 1994, 1997, 1998 Free Software Foundation, Inc.
This is free software with ABSOLUTELY NO WARRANTY.
For details type `warranty'.
xres=1280
yres=1024
refresh=60
hactive=xres
vactive=yres
vfreq=refresh
divisor = (1000000 - (vfreq * 550))/1000
hfreq = (yres + 1) * vfreq * 1000;
hfreq=hfreq/divisor
c_val = (((40-20) * 128)/256 + 20)*1000
m_val = (128 * 600)/256
m_val = (m_val * 1000000)/hfreq;
duty_cycle = c_val - m_val;
hblank = (xres * duty_cycle)/(100000 - duty_cycle);
htotal = hactive+hblank
hsync_len = (htotal * 8)/100
hsync_len
137
hblank
433
duty_cycle
25283

hsync_len should be 112, so:

htotal = (112 * 100)/8
htotal
1400
hblank = htotal - hactive
hblank
120
duty_cycle = (hblank*100000 - xres)/(hblank + 1)
duty_cycle
99162

duty_cycle calculated by from fb_get_hblank_by_hfreq) is 25283 duty_cycle reversed from EDID detailed timing is 99162

I just downloaded GTF_V1R1.xls from vesa.org, now I'm downloading gnumeric...

Until we find the bug we can change calc_mode_timings in this way:

void calc_mode_timings(int xres, int yres, int refresh, struct fb_videomode *mode)

{
        int dbsize = sizeof(modedb)/sizeof(modedb[0]);
        struct fb_videomode *cur;
        int i;

        for (i = 0; i < dbsize; i++) {
                cur = &modedb[i];

                if (cur->xres == xres && cur->yres == yres &&
                                     cur->refreh == refresh) {
                        /* Yeah */
                        mode->xres = xres;
                        mode->yres = yres;
                        mode->pixclock = cur->pixclock;
                        mode->refresh = refresh;
                        mode->left_margin = cur->left_margin;
                        mode->right_margin = cur->right_margin;
                        mode->upper_margin = cur->upper_margin;
                        mode->lower_margin = cur->lower_margin;
                        mode->hsync_len = cur->hsync_len;
                        mode->vsync_len = cur->vsync_len;
                        mode->vmode = cur->vmode;
                        mode->sync = cur->sync;

                        return;
                }
        }

        BUG();
}

Luca

Reply-To: [email protected]
Home: http://kronoz.cjb.net
Not an editor command: Wq


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Wow ! Poor user...

Ben.

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Il Sun, Sep 14, 2003 at 07:46:36PM +0200, Benjamin Herrenschmidt ha scritto:

eheh :) It was just an example. Of course it better to print an error message and fall back to some sane default.

Luca

Reply-To: [email protected]
Home: http://kronoz.cjb.net
Io mi ricordo sempre il compleanno della mia ragazza:
cade il giorno dopo di quando lei me lo ricorda


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

The bug may actually be in the following code:

static int get_std_timing(unsigned char *block, struct fb_videomode *mode)

{
	int xres, yres = 0, refresh, ratio, i;

	xres = (block[0] + 31) * 8;
	if (xres <= 256)
		return 0;

ratio = (block[1] & 0xc0) >> 6;

 	switch (ratio) {
	case 0:
		yres = xres;
 		break;
	case 1:
		yres = (xres * 3)/4;
		break;
 	case 2:
 		yres = (xres * 4)/5;
		break;j
	case 3:
 		yres = (xres * 9)/16;
		break;
	}
	refresh = (block[1] & 0x3f) + 60;

 	for (i = 0; i < VESA_MODEDB_SIZE; i++) {
		if (vesa_modes[i].xres == xres &&
		    vesa_modes[i].yres == yres &&
 		    vesa_modes[i].refresh == refresh) {
 			*mode = vesa_modes[i];
			break;
 		} else {
			calc_mode_timings(xres, yres, refresh, mode);
			break;
 		}
 	}
	return 1;
}

----------------------------------

Maybe you can replace it with this:

static int get_std_timing(unsigned char *block, struct fb_videomode *mode)

{
	int xres, yres = 0, refresh, ratio, i, j = 0;

	xres = (block[0] + 31) * 8;
	if (xres <= 256)
		return 0;

ratio = (block[1] & 0xc0) >> 6;

 	switch (ratio) {
	case 0:
		yres = xres;
 		break;
	case 1:
		yres = (xres * 3)/4;
		break;
 	case 2:
 		yres = (xres * 4)/5;
		break;j
	case 3:
 		yres = (xres * 9)/16;
		break;
	}
	refresh = (block[1] & 0x3f) + 60;

	/* First find standard mode from the table of VESA modes */
 	for (i = 0; i < VESA_MODEDB_SIZE; i++) {
		if (vesa_modes[i].xres == xres &&
		    vesa_modes[i].yres == yres &&
 		    vesa_modes[i].refresh == refresh) {
 			*mode = vesa_modes[i];
			j = 1;
			break;
 		}
	}

	/* If mode is not found in table, calculate using GTF */
	if (!j)
		calc_mode_timings(xres, yres, refresh, mode);

	return 1;
}

Tony

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Il Mon, Sep 15, 2003 at 08:56:07AM +0800, Tony ha scritto:

[cut]

Ah, good catch. I really missed that. This would explain why XFree and radeonfb use different modelines.

James, can you apply this patch:

===== drivers/video/fbmon.c 1.9 vs edited =====

@@ -566,17 +566,19 @@
 	}
 	refresh = (block[1] & 0x3f) + 60;
 
+	/* First find standard mode from the table of VESA modes */
 	for (i = 0; i < VESA_MODEDB_SIZE; i++) {
 		if (vesa_modes[i].xres == xres && 
 		    vesa_modes[i].yres == yres &&
 		    vesa_modes[i].refresh == refresh) {
 			*mode = vesa_modes[i];
-			break;
-		} else {
-			calc_mode_timings(xres, yres, refresh, mode);
-			break;
+			return 1;
 		}
 	}
+
+	/* If mode is not found in table, calculate using GTF */
+	calc_mode_timings(xres, yres, refresh, mode);
+	
 	return 1;
 }
 

Luca

Reply-To: [email protected]
Home: http://kronoz.cjb.net
Il dottore mi ha detto di smettere di fare cene intime per quattro.
A meno che non ci siamo altre tre persone.


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

fb_get_mode() uses GTF calculations to estimate the timings. It will surely be different from VESA standard timings and manufacturer's timings (detailed). Most, if not all, modern CRT , monitors should be able to handle GTF calculated timings -- at least it will display, but the frame may be wide/narrow, tall/short, or shifted left/right/up/down. This is when you need to use fbset to fine tune the timings. However GTF calculations should be treated as last resort (manufacturer's detailed timings, VESA discrete timings, GTF -- decreasing priority).

One thing I forgot to comment in fbmon.c is that the GTF horizontal timing results are not rounded off. It may be necessary to round it off to the nearest character clock (usually 8 pixels) such as by doing a

	left_margin = (left_margin + 4) & ~7 .

I agree with Ben that modedb should have extra flags to determine which of the modes came from where. One can then check an extra flag in EDID (First detailed timing is preferred) to find what is the best mode for
the display. (The print out of show_edid() should tell you this
information).

I was also planning to extend info->monspecs to include most of the information from EDID, but I still have no time.

Regarding your problem about re-centering the screen, why not check the modeline used by XFree86 and by radeonfb? In order to prevent re-centering when switching to/from X, the timings should be the same and if not, either create a custom Xfree86 modeline from fb or a custom timing in /etc/fb.modes taken from your current Xfree86 timings.

BTW, i810fb largely uses timings generated from fb_get_modes() and I haven't heard anyone report a display problem yet. The EDID parser though is largely untested, and I've already seen a few bugs.

Tony

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Il Mon, Sep 15, 2003 at 07:59:37AM +0800, Tony ha scritto:

This is what I did. The problem is not if modelines are different (yes, they are) but _why_ they differ. Both radeonfb and Xfree are using the same EDID block and have the same VESA table...

Luca

Reply-To: [email protected]
Home: http://kronoz.cjb.net
La vasca da bagno fu inventata nel 1850, il telefono nel 1875.
Se fossi vissuto nel 1850, avrei potuto restare in vasca per 25 anni
senza sentir squillare il telefono


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

That doesn't harm. If MODULE is defined, fb_find_mode() is an inline function that knows about 640x400@70 only, cfr. <linux/fb.h>.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

But that's wrong ! Why would you want to prevent a modular driver to use fb_find_mode ? Especially in that case where I'm looking for a mode in the modedb built from EDID list...

On the other hand, if I add the flags for "detailed timings" vs "calculated" in the modedb list obtained from EDID, I will have to parse the list manually or change fb_find_mode prototype, to get to the detailed timing... Another possibility is to change my algorithm to first use the mode returned by parse_edid (which is from the detailed timings), then only fallback to fb_find_mode

Ben.

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Because the modedb itself is __init. Once you have modules, you have userspace and fbset, too.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

But the modedb I'm building from EDID isn't __init ;)

Ben.

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Cool.It would be nice to be able to modedb for modular fbdev drivers.

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf