why does radeonfb work fine in 2.6, but not in 2.4.29-pre1?

why does radeonfb work fine in 2.6, but not in 2.4.29-pre1?

[Jurriaan]

The same radeonfb-setup works fine in every 2.6 kernel I can remember
(last tested with 2.6.10-rc2-mm3) but give the dreaded 'cannot map FB'
in 2.4.29-pre1.

The card has 128 Mb of ram, and my system has 3 Mb of RAM.

Is there any reason the ioremap() call works on 2.6, but doesn't on 2.4?

I've tried searching google for hints, but nothing has turned up.

Is there any way to test 2.4 with my radeonfb and all of my memory?

Thanks,
Jurriaan
-- 
proof by exhaustion:
	An issue or two of a journal devoted to your proof is useful.
Debian (Unstable) GNU/Linux 2.6.10-rc2-mm3 2x6078 bogomips load 1.67

Re: [Linux-fbdev-devel] why does radeonfb work fine in 2.6, but not in 2.4.29-pre1?

[Jurriaan]

From: Jurriaan <thunder7@xs4all.nl>
Date: Sun, Nov 28, 2004 at 07:46:06PM +0100
> The same radeonfb-setup works fine in every 2.6 kernel I can remember
> (last tested with 2.6.10-rc2-mm3) but give the dreaded 'cannot map FB'
> in 2.4.29-pre1.
> 
> The card has 128 Mb of ram, and my system has 3 Mb of RAM.

I mean 3 Gb, of course. Sorry about that. This means I got high memory.

I'd love to know what difference 2.4 and 2.6 have in this regard.

Jurriaan
-- 
The memory management on the PowerPC can be used to frighten small children.
	Linus Torvalds
Debian (Unstable) GNU/Linux 2.6.10-rc2-mm3 2x6078 bogomips load load 0.31

Re: [Linux-fbdev-devel] why does radeonfb work fine in 2.6, but not in 2.4.29-pre1?

[Kronos]

Il Sun, Nov 28, 2004 at 07:46:06PM +0100, Jurriaan ha scritto: 
> The same radeonfb-setup works fine in every 2.6 kernel I can remember
> (last tested with 2.6.10-rc2-mm3) but give the dreaded 'cannot map FB'
> in 2.4.29-pre1.
> 
> The card has 128 Mb of ram, and my system has 3 Mb of RAM.
> 
> Is there any reason the ioremap() call works on 2.6, but doesn't on 2.4?

Driver in 2.6 only ioremap()s the memory needed for the framebuffer,
while the one in 2.4 ioremap()s all the VRAM (and fails).

> Is there any way to test 2.4 with my radeonfb and all of my memory?

I proposed the following patch some time ago (for 2.4.28-pre2 IIRC) as a
quick fix:

--- a/drivers/video/radeonfb.c	2004-06-27 22:26:56.000000000 +0200
+++ b/drivers/video/radeonfb.c	2004-06-29 19:13:24.000000000 +0200
@@ -176,7 +176,8 @@
 #define RTRACE		if(0) printk
 #endif
 
-
+#define MAX_MAPPED_VRAM (2048*2048*4)
+#define MIN_MAPPED_VRAM (1024*768*1)
 
 enum radeon_chips {
 	RADEON_QD,
@@ -499,7 +500,8 @@
 
 	short chipset;
 	unsigned char arch;
-	int video_ram;
+	unsigned int video_ram;
+	unsigned int mapped_vram;
 	u8 rev;
 	int pitch, bpp, depth;
 	int xres, yres, pixclock;
@@ -1823,8 +1825,19 @@
 		}
 	}
 
-	rinfo->fb_base = (unsigned long) ioremap (rinfo->fb_base_phys,
-				  		  rinfo->video_ram);
+	if (rinfo->video_ram < rinfo->mapped_vram)
+		rinfo->mapped_vram = rinfo->video_ram;
+	else
+		rinfo->mapped_vram = MAX_MAPPED_VRAM;
+	do {
+		rinfo->fb_base = (unsigned long) ioremap (rinfo->fb_base_phys,
+				  		  rinfo->mapped_vram);
+		if (rinfo->fb_base)
+			break;
+
+		rinfo->mapped_vram /= 2;
+	} while(rinfo->mapped_vram > MIN_MAPPED_VRAM);
+	
 	if (!rinfo->fb_base) {
 		printk ("radeonfb: cannot map FB\n");
 		iounmap ((void*)rinfo->mmio_base);
@@ -1835,6 +1848,7 @@
 		kfree (rinfo);
 		return -ENODEV;
 	}
+	RTRACE(KERN_INFO "radeonfb: mapped %dk videoram\n", rinfo->mapped_vram/1024);
 
 	/* currcon not yet configured, will be set by first switch */
 	rinfo->currcon = -1;
@@ -2204,7 +2218,7 @@
                 printk("radeonfb: using max available virtual resolution\n");
                 for (i=0; modes[i].xres != -1; i++) {
                         if (modes[i].xres * nom / den * modes[i].yres <
-                            rinfo->video_ram / 2)
+                            rinfo->mapped_vram / 2)
                                 break;
                 }
                 if (modes[i].xres == -1) {
@@ -2217,15 +2231,15 @@
                 printk("radeonfb: virtual resolution set to max of %dx%d\n",
                         v->xres_virtual, v->yres_virtual);
         } else if (v->xres_virtual == -1) {
-                v->xres_virtual = (rinfo->video_ram * den /   
+                v->xres_virtual = (rinfo->mapped_vram * den /   
                                 (nom * v->yres_virtual * 2)) & ~15;
         } else if (v->yres_virtual == -1) {
                 v->xres_virtual = (v->xres_virtual + 15) & ~15;
-                v->yres_virtual = rinfo->video_ram * den /
+                v->yres_virtual = rinfo->mapped_vram * den /
                         (nom * v->xres_virtual *2);
         } else {
                 if (v->xres_virtual * nom / den * v->yres_virtual >
-                        rinfo->video_ram) {
+                        rinfo->mapped_vram) {
                         return -EINVAL;
                 }
         }
@@ -2261,7 +2275,7 @@
 	sprintf (fix->id, "ATI Radeon %s", rinfo->name);
         
         fix->smem_start = rinfo->fb_base_phys;
-        fix->smem_len = rinfo->video_ram;
+        fix->smem_len = rinfo->mapped_vram;
 
         fix->type = disp->type;
         fix->type_aux = disp->type_aux;
@@ -2429,6 +2443,9 @@
                         return -EINVAL;
         }
 
+	if (((v.xres_virtual * v.yres_virtual * nom) / den) > rinfo->mapped_vram)
+		return -EINVAL;
+
         if (radeonfb_do_maximize(rinfo, var, &v, nom, den) < 0)
                 return -EINVAL;  
                 

Problem is that fix->smem_len is used both by FBIOGET_FSCREENINFO to
report the amount of VRAM to userspace and by read/write/mmap on fb
for bounds checking. So with my patch FBIOGET_FSCREENINFO reports mapped
VRAM instead of physical VRAM.

smem_len should be splitted in (say) smem_mapped (for read/write/mmap)
and smem_total_vram (for FBIOGET_FSCREENINFO). I'll code something
tomorrow... -ENEEDSLEEP ;)

Luca
-- 
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.

Re: [Linux-fbdev-devel] why does radeonfb work fine in 2.6, but not in 2.4.29-pre1?

[Jurriaan]

From: Kronos <kronos@people.it>
Date: Mon, Nov 29, 2004 at 10:35:10PM +0100
> Il Sun, Nov 28, 2004 at 07:46:06PM +0100, Jurriaan ha scritto: 
> > The same radeonfb-setup works fine in every 2.6 kernel I can remember
> > (last tested with 2.6.10-rc2-mm3) but give the dreaded 'cannot map FB'
> > in 2.4.29-pre1.
> > 
> > The card has 128 Mb of ram, and my system has 3 Mb of RAM.
> > 
> > Is there any reason the ioremap() call works on 2.6, but doesn't on 2.4?
> 
> Driver in 2.6 only ioremap()s the memory needed for the framebuffer,
> while the one in 2.4 ioremap()s all the VRAM (and fails).
> 
> > Is there any way to test 2.4 with my radeonfb and all of my memory?
> 
> I proposed the following patch some time ago (for 2.4.28-pre2 IIRC) as a
> quick fix:
> 
Thanks. I found that patch on google. Problem is: when I look through
the radeonfb in 2.6, I don't see any assignments to rinfo->video_ram
that indicate it maps less than the full amount.

> 
> Problem is that fix->smem_len is used both by FBIOGET_FSCREENINFO to
> report the amount of VRAM to userspace and by read/write/mmap on fb
> for bounds checking. So with my patch FBIOGET_FSCREENINFO reports mapped
> VRAM instead of physical VRAM.
> 
> smem_len should be splitted in (say) smem_mapped (for read/write/mmap)
> and smem_total_vram (for FBIOGET_FSCREENINFO). I'll code something
> tomorrow... -ENEEDSLEEP ;)
> 
Thanks,
Jurriaan
-- 
All wiyht. Rho sritched mg kegtops awound?
Debian (Unstable) GNU/Linux 2.6.10-rc2-mm3 2x6078 bogomips load load 0.24

Re: [Linux-fbdev-devel] why does radeonfb work fine in 2.6, but not in 2.4.29-pre1?

[Kronos]

Il Tue, Nov 30, 2004 at 07:55:55AM +0100, Jurriaan ha scritto: 
> From: Kronos <kronos@people.it>
> Date: Mon, Nov 29, 2004 at 10:35:10PM +0100
> > Il Sun, Nov 28, 2004 at 07:46:06PM +0100, Jurriaan ha scritto: 
> > > The same radeonfb-setup works fine in every 2.6 kernel I can remember
> > > (last tested with 2.6.10-rc2-mm3) but give the dreaded 'cannot map FB'
> > > in 2.4.29-pre1.
> > > 
> > > The card has 128 Mb of ram, and my system has 3 Mb of RAM.
> > > 
> > > Is there any reason the ioremap() call works on 2.6, but doesn't on 2.4?
> > 
> > Driver in 2.6 only ioremap()s the memory needed for the framebuffer,
> > while the one in 2.4 ioremap()s all the VRAM (and fails).
> > 
> > > Is there any way to test 2.4 with my radeonfb and all of my memory?
> > 
> > I proposed the following patch some time ago (for 2.4.28-pre2 IIRC) as a
> > quick fix:
> > 
> Thanks. I found that patch on google. Problem is: when I look through
> the radeonfb in 2.6, I don't see any assignments to rinfo->video_ram
> that indicate it maps less than the full amount.

rinfo->video_ram is the physical ram. Look at drivers/video/aty/radeon_base.c:2200

  rinfo->mapped_vram = min_t(unsigned long, MAX_MAPPED_VRAM, rinfo->video_ram);

and the following loop tries to shrink the mapped area in case of
ioremap() failure.

Can you test the following patch (against 2.2.29-pre1)? If everything is
ok I'll push to Marcelo.

--- a/include/linux/fb.h	2004-11-30 18:30:08.000000000 +0100
+++ b/include/linux/fb.h	2004-11-30 18:33:00.000000000 +0100
@@ -126,7 +126,8 @@
 					/* (physical address) */
 	__u32 mmio_len;			/* Length of Memory Mapped I/O  */
 	__u32 accel;			/* Type of acceleration available */
-	__u16 reserved[3];		/* Reserved for future compatibility */
+	__u32 mapped_vram;		/* Amount of ioremap()'ed VRAM */
+	__u16 reserved[1];		/* Reserved for future compatibility */
 };
 
 /* Interpretation of offset for color fields: All offsets are from the right,
--- a/drivers/video/fbmem.c	2004-11-30 18:30:00.000000000 +0100
+++ b/drivers/video/fbmem.c	2004-11-30 18:43:06.000000000 +0100
@@ -410,6 +410,7 @@
 	struct fb_info *info = registered_fb[fbidx];
 	struct fb_ops *fb = info->fbops;
 	struct fb_fix_screeninfo fix;
+	unsigned int size;
 
 	if (! fb || ! info->disp)
 		return -ENODEV;
@@ -418,10 +419,12 @@
 		return -EINVAL;
 
 	fb->fb_get_fix(&fix,PROC_CONSOLE(info), info);
-	if (p >= fix.smem_len)
+	size = fix.mapped_vram ? fix.mapped_vram : fix.smem_len;
+	
+	if (p >= size)
 	    return 0;
-	if (count > fix.smem_len - p)
-		count = fix.smem_len - p;
+	if (count > size - p)
+		count = size - p;
 	if (count) {
 	    char *base_addr;
 
@@ -444,6 +447,7 @@
 	struct fb_ops *fb = info->fbops;
 	struct fb_fix_screeninfo fix;
 	int err;
+	unsigned int size;
 
 	if (! fb || ! info->disp)
 		return -ENODEV;
@@ -452,11 +456,13 @@
 		return -EINVAL;
 
 	fb->fb_get_fix(&fix, PROC_CONSOLE(info), info);
-	if (p > fix.smem_len)
+	size = fix.mapped_vram ? fix.mapped_vram : fix.smem_len;
+	
+	if (p > size)
 	    return -ENOSPC;
 	err = 0;
-	if (count > fix.smem_len - p) {
-	    count = fix.smem_len - p;
+	if (count > size - p) {
+	    count = size - p;
 	    err = -ENOSPC;
 	}
 	if (count) {
@@ -619,7 +625,10 @@
 
 	/* frame buffer memory */
 	start = fix.smem_start;
-	len = PAGE_ALIGN((start & ~PAGE_MASK)+fix.smem_len);
+	if (fix.mapped_vram)
+		len = PAGE_ALIGN((start & ~PAGE_MASK) + fix.mapped_vram);
+	else
+		len = PAGE_ALIGN((start & ~PAGE_MASK) + fix.smem_len);
 	if (off >= len) {
 		/* memory mapped io */
 		off -= len;
--- a/drivers/video/radeonfb.c	2004-11-30 18:06:45.000000000 +0100
+++ b/drivers/video/radeonfb.c	2004-11-30 18:50:25.000000000 +0100
@@ -176,7 +176,8 @@
 #define RTRACE		if(0) printk
 #endif
 
-
+#define MAX_MAPPED_VRAM (2048*2048*4)
+#define MIN_MAPPED_VRAM (1024*768*1)
 
 enum radeon_chips {
 	RADEON_QD,
@@ -499,7 +500,8 @@
 
 	short chipset;
 	unsigned char arch;
-	int video_ram;
+	unsigned int video_ram;
+	unsigned int mapped_vram;
 	u8 rev;
 	int pitch, bpp, depth;
 	int xres, yres, pixclock;
@@ -1824,8 +1826,16 @@
 		}
 	}
 
-	rinfo->fb_base = (unsigned long) ioremap (rinfo->fb_base_phys,
-				  		  rinfo->video_ram);
+	rinfo->mapped_vram = min_t(unsigned int, MAX_MAPPED_VRAM, rinfo->video_ram);
+	do {
+		rinfo->fb_base = (unsigned long) ioremap (rinfo->fb_base_phys,
+				  		  rinfo->mapped_vram);
+		if (rinfo->fb_base)
+			break;
+
+		rinfo->mapped_vram /= 2;
+	} while(rinfo->mapped_vram > MIN_MAPPED_VRAM);
+	
 	if (!rinfo->fb_base) {
 		printk ("radeonfb: cannot map FB\n");
 		iounmap ((void*)rinfo->mmio_base);
@@ -1836,6 +1846,7 @@
 		kfree (rinfo);
 		return -ENODEV;
 	}
+	RTRACE(KERN_INFO "radeonfb: mapped %dk videoram\n", rinfo->mapped_vram/1024);
 
 	/* currcon not yet configured, will be set by first switch */
 	rinfo->currcon = -1;
@@ -2205,7 +2216,7 @@
                 printk("radeonfb: using max available virtual resolution\n");
                 for (i=0; modes[i].xres != -1; i++) {
                         if (modes[i].xres * nom / den * modes[i].yres <
-                            rinfo->video_ram / 2)
+                            rinfo->mapped_vram / 2)
                                 break;
                 }
                 if (modes[i].xres == -1) {
@@ -2218,15 +2229,15 @@
                 printk("radeonfb: virtual resolution set to max of %dx%d\n",
                         v->xres_virtual, v->yres_virtual);
         } else if (v->xres_virtual == -1) {
-                v->xres_virtual = (rinfo->video_ram * den /   
+                v->xres_virtual = (rinfo->mapped_vram * den /   
                                 (nom * v->yres_virtual * 2)) & ~15;
         } else if (v->yres_virtual == -1) {
                 v->xres_virtual = (v->xres_virtual + 15) & ~15;
-                v->yres_virtual = rinfo->video_ram * den /
+                v->yres_virtual = rinfo->mapped_vram * den /
                         (nom * v->xres_virtual *2);
         } else {
                 if (v->xres_virtual * nom / den * v->yres_virtual >
-                        rinfo->video_ram) {
+                        rinfo->mapped_vram) {
                         return -EINVAL;
                 }
         }
@@ -2263,6 +2274,7 @@
         
         fix->smem_start = rinfo->fb_base_phys;
         fix->smem_len = rinfo->video_ram;
+        fix->mapped_vram = rinfo->mapped_vram;
 
         fix->type = disp->type;
         fix->type_aux = disp->type_aux;
@@ -2430,6 +2442,9 @@
                         return -EINVAL;
         }
 
+	if (((v.xres_virtual * v.yres_virtual * nom) / den) > rinfo->mapped_vram)
+		return -EINVAL;
+
         if (radeonfb_do_maximize(rinfo, var, &v, nom, den) < 0)
                 return -EINVAL;  
                 


Luca
-- 
Home: http://kronoz.cjb.net
Non sempre quello che viene dopo e` progresso.
Alessandro Manzoni

[PATCH 2.4.29-pre1] radeonfb: don't try to ioreamp the entire VRAM [was: Re: [Linux-fbdev-devel] why does radeonfb work fine in 2.6, but not in 2.4.29-pre1?]

[Kronos]

Il Sun, Nov 28, 2004 at 07:46:06PM +0100, Jurriaan ha scritto: 
> The same radeonfb-setup works fine in every 2.6 kernel I can remember
> (last tested with 2.6.10-rc2-mm3) but give the dreaded 'cannot map FB'
> in 2.4.29-pre1.
> 
> The card has 128 MB of ram, and my system has 3 GB of RAM.
> 
> Is there any reason the ioremap() call works on 2.6, but doesn't on 2.4?
> 
> I've tried searching google for hints, but nothing has turned up.
> 
> Is there any way to test 2.4 with my radeonfb and all of my memory?

Hi Marcelo,
I sent you a patch a while ago but was discarded because introduced a
subtle API change. This time should be ok :)

Make fb layer aware of the difference between the ioremap()'ed VRAM and
total available VRAM.
smem_len in struct fb_fix_screeninfo still contains the amount of
physical VRAM (reported to userspace via FBIOGET_FSCREENINFO ioctl) and
the new field mapped_vram contains the amount of VRAM actually
ioremap()'ed by drivers (used in read/write/mmap operations).
Since there was unused padding at the end of struct fb_fix_screeninfo
binary compatibility with userspace utilities is retained.
If mapped_vram is not set it's assumed that the entire framebuffer is
mapped, thus other drivers are unaffected by this patch.

The patch has been tested by Jurriaan <thunder7@xs4all.nl>.

Signed-off-by: Luca Tettamanti <kronos@people.it>

--- a/include/linux/fb.h	2004-11-30 18:30:08.000000000 +0100
+++ b/include/linux/fb.h	2004-11-30 18:33:00.000000000 +0100
@@ -126,7 +126,8 @@
 					/* (physical address) */
 	__u32 mmio_len;			/* Length of Memory Mapped I/O  */
 	__u32 accel;			/* Type of acceleration available */
-	__u16 reserved[3];		/* Reserved for future compatibility */
+	__u32 mapped_vram;		/* Amount of ioremap()'ed VRAM */
+	__u16 reserved[1];		/* Reserved for future compatibility */
 };
 
 /* Interpretation of offset for color fields: All offsets are from the right,
--- a/drivers/video/fbmem.c	2004-11-30 18:30:00.000000000 +0100
+++ b/drivers/video/fbmem.c	2004-11-30 18:43:06.000000000 +0100
@@ -410,6 +410,7 @@
 	struct fb_info *info = registered_fb[fbidx];
 	struct fb_ops *fb = info->fbops;
 	struct fb_fix_screeninfo fix;
+	unsigned int size;
 
 	if (! fb || ! info->disp)
 		return -ENODEV;
@@ -418,10 +419,12 @@
 		return -EINVAL;
 
 	fb->fb_get_fix(&fix,PROC_CONSOLE(info), info);
-	if (p >= fix.smem_len)
+	size = fix.mapped_vram ? fix.mapped_vram : fix.smem_len;
+	
+	if (p >= size)
 	    return 0;
-	if (count > fix.smem_len - p)
-		count = fix.smem_len - p;
+	if (count > size - p)
+		count = size - p;
 	if (count) {
 	    char *base_addr;
 
@@ -444,6 +447,7 @@
 	struct fb_ops *fb = info->fbops;
 	struct fb_fix_screeninfo fix;
 	int err;
+	unsigned int size;
 
 	if (! fb || ! info->disp)
 		return -ENODEV;
@@ -452,11 +456,13 @@
 		return -EINVAL;
 
 	fb->fb_get_fix(&fix, PROC_CONSOLE(info), info);
-	if (p > fix.smem_len)
+	size = fix.mapped_vram ? fix.mapped_vram : fix.smem_len;
+	
+	if (p > size)
 	    return -ENOSPC;
 	err = 0;
-	if (count > fix.smem_len - p) {
-	    count = fix.smem_len - p;
+	if (count > size - p) {
+	    count = size - p;
 	    err = -ENOSPC;
 	}
 	if (count) {
@@ -619,7 +625,10 @@
 
 	/* frame buffer memory */
 	start = fix.smem_start;
-	len = PAGE_ALIGN((start & ~PAGE_MASK)+fix.smem_len);
+	if (fix.mapped_vram)
+		len = PAGE_ALIGN((start & ~PAGE_MASK) + fix.mapped_vram);
+	else
+		len = PAGE_ALIGN((start & ~PAGE_MASK) + fix.smem_len);
 	if (off >= len) {
 		/* memory mapped io */
 		off -= len;
--- a/drivers/video/radeonfb.c	2004-11-30 18:06:45.000000000 +0100
+++ b/drivers/video/radeonfb.c	2004-11-30 18:50:25.000000000 +0100
@@ -176,7 +176,8 @@
 #define RTRACE		if(0) printk
 #endif
 
-
+#define MAX_MAPPED_VRAM (2048*2048*4)
+#define MIN_MAPPED_VRAM (1024*768*1)
 
 enum radeon_chips {
 	RADEON_QD,
@@ -499,7 +500,8 @@
 
 	short chipset;
 	unsigned char arch;
-	int video_ram;
+	unsigned int video_ram;
+	unsigned int mapped_vram;
 	u8 rev;
 	int pitch, bpp, depth;
 	int xres, yres, pixclock;
@@ -1824,8 +1826,16 @@
 		}
 	}
 
-	rinfo->fb_base = (unsigned long) ioremap (rinfo->fb_base_phys,
-				  		  rinfo->video_ram);
+	rinfo->mapped_vram = min_t(unsigned int, MAX_MAPPED_VRAM, rinfo->video_ram);
+	do {
+		rinfo->fb_base = (unsigned long) ioremap (rinfo->fb_base_phys,
+				  		  rinfo->mapped_vram);
+		if (rinfo->fb_base)
+			break;
+
+		rinfo->mapped_vram /= 2;
+	} while(rinfo->mapped_vram > MIN_MAPPED_VRAM);
+	
 	if (!rinfo->fb_base) {
 		printk ("radeonfb: cannot map FB\n");
 		iounmap ((void*)rinfo->mmio_base);
@@ -1836,6 +1846,7 @@
 		kfree (rinfo);
 		return -ENODEV;
 	}
+	RTRACE(KERN_INFO "radeonfb: mapped %dk videoram\n", rinfo->mapped_vram/1024);
 
 	/* currcon not yet configured, will be set by first switch */
 	rinfo->currcon = -1;
@@ -2205,7 +2216,7 @@
                 printk("radeonfb: using max available virtual resolution\n");
                 for (i=0; modes[i].xres != -1; i++) {
                         if (modes[i].xres * nom / den * modes[i].yres <
-                            rinfo->video_ram / 2)
+                            rinfo->mapped_vram / 2)
                                 break;
                 }
                 if (modes[i].xres == -1) {
@@ -2218,15 +2229,15 @@
                 printk("radeonfb: virtual resolution set to max of %dx%d\n",
                         v->xres_virtual, v->yres_virtual);
         } else if (v->xres_virtual == -1) {
-                v->xres_virtual = (rinfo->video_ram * den /   
+                v->xres_virtual = (rinfo->mapped_vram * den /   
                                 (nom * v->yres_virtual * 2)) & ~15;
         } else if (v->yres_virtual == -1) {
                 v->xres_virtual = (v->xres_virtual + 15) & ~15;
-                v->yres_virtual = rinfo->video_ram * den /
+                v->yres_virtual = rinfo->mapped_vram * den /
                         (nom * v->xres_virtual *2);
         } else {
                 if (v->xres_virtual * nom / den * v->yres_virtual >
-                        rinfo->video_ram) {
+                        rinfo->mapped_vram) {
                         return -EINVAL;
                 }
         }
@@ -2263,6 +2274,7 @@
         
         fix->smem_start = rinfo->fb_base_phys;
         fix->smem_len = rinfo->video_ram;
+        fix->mapped_vram = rinfo->mapped_vram;
 
         fix->type = disp->type;
         fix->type_aux = disp->type_aux;
@@ -2430,6 +2442,9 @@
                         return -EINVAL;
         }
 
+	if (((v.xres_virtual * v.yres_virtual * nom) / den) > rinfo->mapped_vram)
+		return -EINVAL;
+
         if (radeonfb_do_maximize(rinfo, var, &v, nom, den) < 0)
                 return -EINVAL;  
                 


Luca
-- 
Home: http://kronoz.cjb.net
"It is more complicated than you think"
                -- The Eighth Networking Truth from RFC 1925

Re: [PATCH 2.4.29-pre1] radeonfb: don't try to ioreamp the entire VRAM [was: Re: [Linux-fbdev-devel] why does radeonfb work fine in 2.6, but not in 2.4.29-pre1?]

[Geert Uytterhoeven]

On Wed, 1 Dec 2004, Kronos wrote:
> Il Sun, Nov 28, 2004 at 07:46:06PM +0100, Jurriaan ha scritto: 
> > The same radeonfb-setup works fine in every 2.6 kernel I can remember
> > (last tested with 2.6.10-rc2-mm3) but give the dreaded 'cannot map FB'
> > in 2.4.29-pre1.
> > 
> > The card has 128 MB of ram, and my system has 3 GB of RAM.
> > 
> > Is there any reason the ioremap() call works on 2.6, but doesn't on 2.4?
> > 
> > I've tried searching google for hints, but nothing has turned up.
> > 
> > Is there any way to test 2.4 with my radeonfb and all of my memory?
> 
> I sent you a patch a while ago but was discarded because introduced a
> subtle API change. This time should be ok :)
> 
> Make fb layer aware of the difference between the ioremap()'ed VRAM and
> total available VRAM.
> smem_len in struct fb_fix_screeninfo still contains the amount of
> physical VRAM (reported to userspace via FBIOGET_FSCREENINFO ioctl) and
> the new field mapped_vram contains the amount of VRAM actually
> ioremap()'ed by drivers (used in read/write/mmap operations).
> Since there was unused padding at the end of struct fb_fix_screeninfo
> binary compatibility with userspace utilities is retained.
> If mapped_vram is not set it's assumed that the entire framebuffer is
> mapped, thus other drivers are unaffected by this patch.
> 
> The patch has been tested by Jurriaan <thunder7@xs4all.nl>.
> 
> Signed-off-by: Luca Tettamanti <kronos@people.it>
> 
> --- a/include/linux/fb.h	2004-11-30 18:30:08.000000000 +0100
> +++ b/include/linux/fb.h	2004-11-30 18:33:00.000000000 +0100
> @@ -126,7 +126,8 @@
>  					/* (physical address) */
>  	__u32 mmio_len;			/* Length of Memory Mapped I/O  */
>  	__u32 accel;			/* Type of acceleration available */
> -	__u16 reserved[3];		/* Reserved for future compatibility */
> +	__u32 mapped_vram;		/* Amount of ioremap()'ed VRAM */
> +	__u16 reserved[1];		/* Reserved for future compatibility */
>  };

I don't really like this patch. mapped_vram doesn't matter for user space at
all, so it does not belong to fb_fix_screeninfo.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH 2.4.29-pre1] radeonfb: don't try to ioreamp the entire VRAM [was: Re: [Linux-fbdev-devel] why does radeonfb work fine in 2.6, but not in 2.4.29-pre1?]

[Kronos]

Il Wed, Dec 01, 2004 at 05:25:52PM +0100, Geert Uytterhoeven ha scritto: 
> > Make fb layer aware of the difference between the ioremap()'ed VRAM and
> > total available VRAM.
> > smem_len in struct fb_fix_screeninfo still contains the amount of
> > physical VRAM (reported to userspace via FBIOGET_FSCREENINFO ioctl) and
> > the new field mapped_vram contains the amount of VRAM actually
> > ioremap()'ed by drivers (used in read/write/mmap operations).
> > Since there was unused padding at the end of struct fb_fix_screeninfo
> > binary compatibility with userspace utilities is retained.
> > If mapped_vram is not set it's assumed that the entire framebuffer is
> > mapped, thus other drivers are unaffected by this patch.
> > 
> > The patch has been tested by Jurriaan <thunder7@xs4all.nl>.
> > 
> > Signed-off-by: Luca Tettamanti <kronos@people.it>
> > 
> > --- a/include/linux/fb.h	2004-11-30 18:30:08.000000000 +0100
> > +++ b/include/linux/fb.h	2004-11-30 18:33:00.000000000 +0100
> > @@ -126,7 +126,8 @@
> >  					/* (physical address) */
> >  	__u32 mmio_len;			/* Length of Memory Mapped I/O  */
> >  	__u32 accel;			/* Type of acceleration available */
> > -	__u16 reserved[3];		/* Reserved for future compatibility */
> > +	__u32 mapped_vram;		/* Amount of ioremap()'ed VRAM */
> > +	__u16 reserved[1];		/* Reserved for future compatibility */
> >  };
> 
> I don't really like this patch. mapped_vram doesn't matter for user space at
> all, so it does not belong to fb_fix_screeninfo.

Hmm, it looked sensible to me since it's the max amount of data that
userspace can read (or write) from /dev/fb%d.
Putting mapped_vram in fb_info would be acceptable?

Luca
-- 
Home: http://kronoz.cjb.net
Carpe diem, quam minimum credula postero. (Q. Horatius Flaccus)

Re: [PATCH 2.4.29-pre1] radeonfb: don't try to ioreamp the entire VRAM [was: Re: [Linux-fbdev-devel] why does radeonfb work fine in 2.6, but not in 2.4.29-pre1?]

[Geert Uytterhoeven]

On Wed, 1 Dec 2004, Kronos wrote:
> Il Wed, Dec 01, 2004 at 05:25:52PM +0100, Geert Uytterhoeven ha scritto: 
> > > Make fb layer aware of the difference between the ioremap()'ed VRAM and
> > > total available VRAM.
> > > smem_len in struct fb_fix_screeninfo still contains the amount of
> > > physical VRAM (reported to userspace via FBIOGET_FSCREENINFO ioctl) and
> > > the new field mapped_vram contains the amount of VRAM actually
> > > ioremap()'ed by drivers (used in read/write/mmap operations).
> > > Since there was unused padding at the end of struct fb_fix_screeninfo
> > > binary compatibility with userspace utilities is retained.
> > > If mapped_vram is not set it's assumed that the entire framebuffer is
> > > mapped, thus other drivers are unaffected by this patch.
> > > 
> > > The patch has been tested by Jurriaan <thunder7@xs4all.nl>.
> > > 
> > > Signed-off-by: Luca Tettamanti <kronos@people.it>
> > > 
> > > --- a/include/linux/fb.h	2004-11-30 18:30:08.000000000 +0100
> > > +++ b/include/linux/fb.h	2004-11-30 18:33:00.000000000 +0100
> > > @@ -126,7 +126,8 @@
> > >  					/* (physical address) */
> > >  	__u32 mmio_len;			/* Length of Memory Mapped I/O  */
> > >  	__u32 accel;			/* Type of acceleration available */
> > > -	__u16 reserved[3];		/* Reserved for future compatibility */
> > > +	__u32 mapped_vram;		/* Amount of ioremap()'ed VRAM */
> > > +	__u16 reserved[1];		/* Reserved for future compatibility */
> > >  };
> > 
> > I don't really like this patch. mapped_vram doesn't matter for user space at
> > all, so it does not belong to fb_fix_screeninfo.
> 
> Hmm, it looked sensible to me since it's the max amount of data that
> userspace can read (or write) from /dev/fb%d.

That's not really a user space limitation, but a kernel `bug'.

I.e. it could be solved in the kernel if larger ioremap()s would be allowed, or
if some sliding window mapping would be used. User space shouldn't need to know
about this.

> Putting mapped_vram in fb_info would be acceptable?

That would make sure this stays in-kernel, but my other reasoning stays the
same: it's some kind of kernel bug.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH 2.4.29-pre1] radeonfb: don't try to ioreamp the entire VRAM [was: Re: [Linux-fbdev-devel] why does radeonfb work fine in 2.6, but not in 2.4.29-pre1?]

[Kronos]

Il Wed, Dec 01, 2004 at 10:25:38PM +0100, Geert Uytterhoeven ha scritto: 
> On Wed, 1 Dec 2004, Kronos wrote:
> > Il Wed, Dec 01, 2004 at 05:25:52PM +0100, Geert Uytterhoeven ha scritto: 
> > > > Make fb layer aware of the difference between the ioremap()'ed VRAM and
> > > > total available VRAM.
> > > > smem_len in struct fb_fix_screeninfo still contains the amount of
> > > > physical VRAM (reported to userspace via FBIOGET_FSCREENINFO ioctl) and
> > > > the new field mapped_vram contains the amount of VRAM actually
> > > > ioremap()'ed by drivers (used in read/write/mmap operations).
> > > > Since there was unused padding at the end of struct fb_fix_screeninfo
> > > > binary compatibility with userspace utilities is retained.
> > > > If mapped_vram is not set it's assumed that the entire framebuffer is
> > > > mapped, thus other drivers are unaffected by this patch.
> > > > 
> > > > The patch has been tested by Jurriaan <thunder7@xs4all.nl>.
> > > > 
> > > > Signed-off-by: Luca Tettamanti <kronos@people.it>
> > > > 
> > > > --- a/include/linux/fb.h	2004-11-30 18:30:08.000000000 +0100
> > > > +++ b/include/linux/fb.h	2004-11-30 18:33:00.000000000 +0100
> > > > @@ -126,7 +126,8 @@
> > > >  					/* (physical address) */
> > > >  	__u32 mmio_len;			/* Length of Memory Mapped I/O  */
> > > >  	__u32 accel;			/* Type of acceleration available */
> > > > -	__u16 reserved[3];		/* Reserved for future compatibility */
> > > > +	__u32 mapped_vram;		/* Amount of ioremap()'ed VRAM */
> > > > +	__u16 reserved[1];		/* Reserved for future compatibility */
> > > >  };
> > > 
> > > I don't really like this patch. mapped_vram doesn't matter for user space at
> > > all, so it does not belong to fb_fix_screeninfo.
> > 
> > Hmm, it looked sensible to me since it's the max amount of data that
> > userspace can read (or write) from /dev/fb%d.
> 
> That's not really a user space limitation, but a kernel `bug'.
> 
> I.e. it could be solved in the kernel if larger ioremap()s would be allowed, or
> if some sliding window mapping would be used. User space shouldn't need to know
> about this.

I see you point, but users see that their video board does not work if
the system has 1GB (or more) of RAM and they aren't happy.
With current VM split (ie. 1GB/3GB) and with that much RAM there's no
space left to map the entire FB. And AFAIK VM hacking in 2.4 is not
doable (not that I would be able to do it ;)

> > Putting mapped_vram in fb_info would be acceptable?
> 
> That would make sure this stays in-kernel, but my other reasoning stays the
> same: it's some kind of kernel bug.

What about this one:


Make fb layer aware of the difference between the ioremap()'ed VRAM and
total available VRAM.
smem_len in struct fb_fix_screeninfo contains the amount of physical
VRAM (reported to userspace via FBIOGET_FSCREENINFO ioctl) while the new
field mapped_vram in struct fb_info contains the amount of VRAM actually
ioremap()'ed by drivers (used in read/write/mmap operations).
If mapped_vram is not set it's assumed that the entire framebuffer is
mapped, thus other drivers are unaffected by this patch.

Signed-off-by: Luca Tettamanti <kronos@people.it>

--- a/include/linux/fb.h	2004-11-30 18:30:08.000000000 +0100
+++ b/include/linux/fb.h	2004-12-02 14:20:50.000000000 +0100
@@ -323,6 +323,7 @@
    struct fb_cmap cmap;                 /* Current cmap */
    struct fb_ops *fbops;
    char *screen_base;                   /* Virtual address */
+   unsigned int mapped_vram;		/* ioremap()'ed VRAM */
    struct display *disp;		/* initial display variable */
    struct vc_data *display_fg;		/* Console visible on this display */
    char fontname[40];			/* default font name */
--- a/drivers/video/fbmem.c	2004-11-30 18:30:00.000000000 +0100
+++ b/drivers/video/fbmem.c	2004-12-02 14:29:44.000000000 +0100
@@ -410,6 +410,7 @@
 	struct fb_info *info = registered_fb[fbidx];
 	struct fb_ops *fb = info->fbops;
 	struct fb_fix_screeninfo fix;
+	unsigned int size;
 
 	if (! fb || ! info->disp)
 		return -ENODEV;
@@ -418,10 +419,12 @@
 		return -EINVAL;
 
 	fb->fb_get_fix(&fix,PROC_CONSOLE(info), info);
-	if (p >= fix.smem_len)
+	size = info->mapped_vram ? info->mapped_vram : fix.smem_len;
+	
+	if (p >= size)
 	    return 0;
-	if (count > fix.smem_len - p)
-		count = fix.smem_len - p;
+	if (count > size - p)
+		count = size - p;
 	if (count) {
 	    char *base_addr;
 
@@ -444,6 +447,7 @@
 	struct fb_ops *fb = info->fbops;
 	struct fb_fix_screeninfo fix;
 	int err;
+	unsigned int size;
 
 	if (! fb || ! info->disp)
 		return -ENODEV;
@@ -452,11 +456,13 @@
 		return -EINVAL;
 
 	fb->fb_get_fix(&fix, PROC_CONSOLE(info), info);
-	if (p > fix.smem_len)
+	size = info->mapped_vram ? info->mapped_vram : fix.smem_len;
+	
+	if (p > size)
 	    return -ENOSPC;
 	err = 0;
-	if (count > fix.smem_len - p) {
-	    count = fix.smem_len - p;
+	if (count > size - p) {
+	    count = size - p;
 	    err = -ENOSPC;
 	}
 	if (count) {
@@ -619,7 +625,10 @@
 
 	/* frame buffer memory */
 	start = fix.smem_start;
-	len = PAGE_ALIGN((start & ~PAGE_MASK)+fix.smem_len);
+	if (info->mapped_vram)
+		len = PAGE_ALIGN((start & ~PAGE_MASK) + info->mapped_vram);
+	else
+		len = PAGE_ALIGN((start & ~PAGE_MASK) + fix.smem_len);
 	if (off >= len) {
 		/* memory mapped io */
 		off -= len;
--- a/drivers/video/radeonfb.c	2004-11-30 18:06:45.000000000 +0100
+++ b/drivers/video/radeonfb.c	2004-12-02 14:28:10.000000000 +0100
@@ -176,7 +176,8 @@
 #define RTRACE		if(0) printk
 #endif
 
-
+#define MAX_MAPPED_VRAM (2048*2048*4)
+#define MIN_MAPPED_VRAM (1024*768*1)
 
 enum radeon_chips {
 	RADEON_QD,
@@ -499,7 +500,7 @@
 
 	short chipset;
 	unsigned char arch;
-	int video_ram;
+	unsigned int video_ram;
 	u8 rev;
 	int pitch, bpp, depth;
 	int xres, yres, pixclock;
@@ -1626,6 +1627,7 @@
 				  const struct pci_device_id *ent)
 {
 	struct radeonfb_info *rinfo;
+	struct fb_info *fb_info;
 	struct radeon_chip_info *rci = &radeon_chip_info[ent->driver_data];
 	u32 tmp;
 	int i, j;
@@ -1640,6 +1642,7 @@
 
 	memset (rinfo, 0, sizeof (struct radeonfb_info));
 
+	fb_info = (struct fb_info *)rinfo;
 	rinfo->pdev = pdev;
 	strncpy(rinfo->name, rci->name, 16);
 	rinfo->arch = rci->arch;
@@ -1824,8 +1827,16 @@
 		}
 	}
 
-	rinfo->fb_base = (unsigned long) ioremap (rinfo->fb_base_phys,
-				  		  rinfo->video_ram);
+	fb_info->mapped_vram = min_t(unsigned int, MAX_MAPPED_VRAM, rinfo->video_ram);
+	do {
+		rinfo->fb_base = (unsigned long) ioremap (rinfo->fb_base_phys,
+				  		  fb_info->mapped_vram);
+		if (rinfo->fb_base)
+			break;
+
+		fb_info->mapped_vram /= 2;
+	} while(fb_info->mapped_vram > MIN_MAPPED_VRAM);
+	
 	if (!rinfo->fb_base) {
 		printk ("radeonfb: cannot map FB\n");
 		iounmap ((void*)rinfo->mmio_base);
@@ -1836,6 +1847,7 @@
 		kfree (rinfo);
 		return -ENODEV;
 	}
+	RTRACE(KERN_INFO "radeonfb: mapped %dk videoram\n", fb_info->mapped_vram/1024);
 
 	/* currcon not yet configured, will be set by first switch */
 	rinfo->currcon = -1;
@@ -2199,13 +2211,14 @@
                 {-1, -1}
         };
         int i;
+	struct fb_info *fb_info = (struct fb_info *)rinfo;
                 
         /* use highest possible virtual resolution */
         if (v->xres_virtual == -1 && v->yres_virtual == -1) {
                 printk("radeonfb: using max available virtual resolution\n");
                 for (i=0; modes[i].xres != -1; i++) {
                         if (modes[i].xres * nom / den * modes[i].yres <
-                            rinfo->video_ram / 2)
+                            fb_info->mapped_vram / 2)
                                 break;
                 }
                 if (modes[i].xres == -1) {
@@ -2218,15 +2231,15 @@
                 printk("radeonfb: virtual resolution set to max of %dx%d\n",
                         v->xres_virtual, v->yres_virtual);
         } else if (v->xres_virtual == -1) {
-                v->xres_virtual = (rinfo->video_ram * den /   
+                v->xres_virtual = (fb_info->mapped_vram * den /   
                                 (nom * v->yres_virtual * 2)) & ~15;
         } else if (v->yres_virtual == -1) {
                 v->xres_virtual = (v->xres_virtual + 15) & ~15;
-                v->yres_virtual = rinfo->video_ram * den /
+                v->yres_virtual = fb_info->mapped_vram * den /
                         (nom * v->xres_virtual *2);
         } else {
                 if (v->xres_virtual * nom / den * v->yres_virtual >
-                        rinfo->video_ram) {
+                        fb_info->mapped_vram) {
                         return -EINVAL;
                 }
         }
@@ -2430,6 +2443,9 @@
                         return -EINVAL;
         }
 
+	if (((v.xres_virtual * v.yres_virtual * nom) / den) > info->mapped_vram)
+		return -EINVAL;
+
         if (radeonfb_do_maximize(rinfo, var, &v, nom, den) < 0)
                 return -EINVAL;  
                 

Luca
-- 
Home: http://kronoz.cjb.net
Windows NT crashed.
I'm the Blue Screen of Death.
No one hears your screams.

Re: [PATCH 2.4.29-pre1] radeonfb: don't try to ioreamp the entire VRAM [was: Re: [Linux-fbdev-devel] why does radeonfb work fine in 2.6, but not in 2.4.29-pre1?]

[Geert Uytterhoeven]

On Thu, 2 Dec 2004, Kronos wrote:
> Il Wed, Dec 01, 2004 at 10:25:38PM +0100, Geert Uytterhoeven ha scritto: 
> > On Wed, 1 Dec 2004, Kronos wrote:
> > > Il Wed, Dec 01, 2004 at 05:25:52PM +0100, Geert Uytterhoeven ha scritto: 
> > > > > Make fb layer aware of the difference between the ioremap()'ed VRAM and
> > > > > total available VRAM.
> > > > > smem_len in struct fb_fix_screeninfo still contains the amount of
> > > > > physical VRAM (reported to userspace via FBIOGET_FSCREENINFO ioctl) and
> > > > > the new field mapped_vram contains the amount of VRAM actually
> > > > > ioremap()'ed by drivers (used in read/write/mmap operations).
> > > > > Since there was unused padding at the end of struct fb_fix_screeninfo
> > > > > binary compatibility with userspace utilities is retained.
> > > > > If mapped_vram is not set it's assumed that the entire framebuffer is
> > > > > mapped, thus other drivers are unaffected by this patch.
> > > > > 
> > > > > The patch has been tested by Jurriaan <thunder7@xs4all.nl>.
> > > > > 
> > > > > Signed-off-by: Luca Tettamanti <kronos@people.it>
> > > > > 
> > > > > --- a/include/linux/fb.h	2004-11-30 18:30:08.000000000 +0100
> > > > > +++ b/include/linux/fb.h	2004-11-30 18:33:00.000000000 +0100
> > > > > @@ -126,7 +126,8 @@
> > > > >  					/* (physical address) */
> > > > >  	__u32 mmio_len;			/* Length of Memory Mapped I/O  */
> > > > >  	__u32 accel;			/* Type of acceleration available */
> > > > > -	__u16 reserved[3];		/* Reserved for future compatibility */
> > > > > +	__u32 mapped_vram;		/* Amount of ioremap()'ed VRAM */
> > > > > +	__u16 reserved[1];		/* Reserved for future compatibility */
> > > > >  };
> > > > 
> > > > I don't really like this patch. mapped_vram doesn't matter for user space at
> > > > all, so it does not belong to fb_fix_screeninfo.
> > > 
> > > Hmm, it looked sensible to me since it's the max amount of data that
> > > userspace can read (or write) from /dev/fb%d.
> > 
> > That's not really a user space limitation, but a kernel `bug'.
> > 
> > I.e. it could be solved in the kernel if larger ioremap()s would be allowed, or
> > if some sliding window mapping would be used. User space shouldn't need to know
> > about this.
> 
> I see you point, but users see that their video board does not work if
> the system has 1GB (or more) of RAM and they aren't happy.
> With current VM split (ie. 1GB/3GB) and with that much RAM there's no
> space left to map the entire FB. And AFAIK VM hacking in 2.4 is not
> doable (not that I would be able to do it ;)
> 
> > > Putting mapped_vram in fb_info would be acceptable?
> > 
> > That would make sure this stays in-kernel, but my other reasoning stays the
> > same: it's some kind of kernel bug.
> 
> What about this one:
> 
> 
> Make fb layer aware of the difference between the ioremap()'ed VRAM and
> total available VRAM.
> smem_len in struct fb_fix_screeninfo contains the amount of physical
> VRAM (reported to userspace via FBIOGET_FSCREENINFO ioctl) while the new
> field mapped_vram in struct fb_info contains the amount of VRAM actually
> ioremap()'ed by drivers (used in read/write/mmap operations).
> If mapped_vram is not set it's assumed that the entire framebuffer is
> mapped, thus other drivers are unaffected by this patch.

Looks OK at first sight.

> --- a/include/linux/fb.h	2004-11-30 18:30:08.000000000 +0100
> +++ b/include/linux/fb.h	2004-12-02 14:20:50.000000000 +0100
> @@ -323,6 +323,7 @@
>     struct fb_cmap cmap;                 /* Current cmap */
>     struct fb_ops *fbops;
>     char *screen_base;                   /* Virtual address */
> +   unsigned int mapped_vram;		/* ioremap()'ed VRAM */

But perhaps you want to make this unsigned long? I don't think it'll take long
before we'll see graphics cards with 4 GiB or more memory, and it would be good
if 64-bit platforms could take advantage of them.

Oh no, we already have __u32 smem_len in struct fb_fix_screeninfo :-(
So you can forget my comment about 64-bit above...

Hence, you best make it `u32' (this is kernel internal) for consistency.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH 2.4.29-pre1] radeonfb: don't try to ioreamp the entire VRAM [was: Re: [Linux-fbdev-devel] why does radeonfb work fine in 2.6, but not in 2.4.29-pre1?]

[Kronos]

Il Thu, Dec 02, 2004 at 04:39:25PM +0100, Geert Uytterhoeven ha scritto: 
> On Thu, 2 Dec 2004, Kronos wrote:
> > Il Wed, Dec 01, 2004 at 10:25:38PM +0100, Geert Uytterhoeven ha scritto: 
> > > On Wed, 1 Dec 2004, Kronos wrote:
> > > > Il Wed, Dec 01, 2004 at 05:25:52PM +0100, Geert Uytterhoeven ha scritto: 
> > > > > > Make fb layer aware of the difference between the ioremap()'ed VRAM and
> > > > > > total available VRAM.
> > > > > > smem_len in struct fb_fix_screeninfo still contains the amount of
> > > > > > physical VRAM (reported to userspace via FBIOGET_FSCREENINFO ioctl) and
> > > > > > the new field mapped_vram contains the amount of VRAM actually
> > > > > > ioremap()'ed by drivers (used in read/write/mmap operations).
> > > > > > Since there was unused padding at the end of struct fb_fix_screeninfo
> > > > > > binary compatibility with userspace utilities is retained.
> > > > > > If mapped_vram is not set it's assumed that the entire framebuffer is
> > > > > > mapped, thus other drivers are unaffected by this patch.
> > > > > > 
> > > > > > The patch has been tested by Jurriaan <thunder7@xs4all.nl>.
> > > > > > 
> > > > > > Signed-off-by: Luca Tettamanti <kronos@people.it>
> > > > > > 
> > > > > > --- a/include/linux/fb.h	2004-11-30 18:30:08.000000000 +0100
> > > > > > +++ b/include/linux/fb.h	2004-11-30 18:33:00.000000000 +0100
> > > > > > @@ -126,7 +126,8 @@
> > > > > >  					/* (physical address) */
> > > > > >  	__u32 mmio_len;			/* Length of Memory Mapped I/O  */
> > > > > >  	__u32 accel;			/* Type of acceleration available */
> > > > > > -	__u16 reserved[3];		/* Reserved for future compatibility */
> > > > > > +	__u32 mapped_vram;		/* Amount of ioremap()'ed VRAM */
> > > > > > +	__u16 reserved[1];		/* Reserved for future compatibility */
> > > > > >  };
> > > > > 
> > > > > I don't really like this patch. mapped_vram doesn't matter for user space at
> > > > > all, so it does not belong to fb_fix_screeninfo.
> > > > 
> > > > Hmm, it looked sensible to me since it's the max amount of data that
> > > > userspace can read (or write) from /dev/fb%d.
> > > 
> > > That's not really a user space limitation, but a kernel `bug'.
> > > 
> > > I.e. it could be solved in the kernel if larger ioremap()s would be allowed, or
> > > if some sliding window mapping would be used. User space shouldn't need to know
> > > about this.
> > 
> > I see you point, but users see that their video board does not work if
> > the system has 1GB (or more) of RAM and they aren't happy.
> > With current VM split (ie. 1GB/3GB) and with that much RAM there's no
> > space left to map the entire FB. And AFAIK VM hacking in 2.4 is not
> > doable (not that I would be able to do it ;)
> > 
> > > > Putting mapped_vram in fb_info would be acceptable?
> > > 
> > > That would make sure this stays in-kernel, but my other reasoning stays the
> > > same: it's some kind of kernel bug.
> 
> Looks OK at first sight.
> 
> > --- a/include/linux/fb.h	2004-11-30 18:30:08.000000000 +0100
> > +++ b/include/linux/fb.h	2004-12-02 14:20:50.000000000 +0100
> > @@ -323,6 +323,7 @@
> >     struct fb_cmap cmap;                 /* Current cmap */
> >     struct fb_ops *fbops;
> >     char *screen_base;                   /* Virtual address */
> > +   unsigned int mapped_vram;		/* ioremap()'ed VRAM */
> 
> But perhaps you want to make this unsigned long? I don't think it'll take long
> before we'll see graphics cards with 4 GiB or more memory, and it would be good
> if 64-bit platforms could take advantage of them.
> 
> Oh no, we already have __u32 smem_len in struct fb_fix_screeninfo :-(
> So you can forget my comment about 64-bit above...
> 
> Hence, you best make it `u32' (this is kernel internal) for consistency.

Ok, here we go:

Make fb layer aware of the difference between the ioremap()'ed VRAM and
total available VRAM.
smem_len in struct fb_fix_screeninfo contains the amount of physical
VRAM (reported to userspace via FBIOGET_FSCREENINFO ioctl) while the new
field mapped_vram in struct fb_info contains the amount of VRAM actually
ioremap()'ed by drivers (used in read/write/mmap operations).
If mapped_vram is not set it's assumed that the entire framebuffer is
mapped, thus other drivers are unaffected by this patch.

Signed-off-by: Luca Tettamanti <kronos@people.it>

--- a/include/linux/fb.h	2004-11-30 18:30:08.000000000 +0100
+++ b/include/linux/fb.h	2004-12-02 14:20:50.000000000 +0100
@@ -323,6 +323,7 @@
    struct fb_cmap cmap;                 /* Current cmap */
    struct fb_ops *fbops;
    char *screen_base;                   /* Virtual address */
+   u32 mapped_vram;			/* ioremap()'ed VRAM */
    struct display *disp;		/* initial display variable */
    struct vc_data *display_fg;		/* Console visible on this display */
    char fontname[40];			/* default font name */
--- a/drivers/video/fbmem.c	2004-11-30 18:30:00.000000000 +0100
+++ b/drivers/video/fbmem.c	2004-12-02 14:29:44.000000000 +0100
@@ -410,6 +410,7 @@
 	struct fb_info *info = registered_fb[fbidx];
 	struct fb_ops *fb = info->fbops;
 	struct fb_fix_screeninfo fix;
+	unsigned int size;
 
 	if (! fb || ! info->disp)
 		return -ENODEV;
@@ -418,10 +419,12 @@
 		return -EINVAL;
 
 	fb->fb_get_fix(&fix,PROC_CONSOLE(info), info);
-	if (p >= fix.smem_len)
+	size = info->mapped_vram ? info->mapped_vram : fix.smem_len;
+	
+	if (p >= size)
 	    return 0;
-	if (count > fix.smem_len - p)
-		count = fix.smem_len - p;
+	if (count > size - p)
+		count = size - p;
 	if (count) {
 	    char *base_addr;
 
@@ -444,6 +447,7 @@
 	struct fb_ops *fb = info->fbops;
 	struct fb_fix_screeninfo fix;
 	int err;
+	unsigned int size;
 
 	if (! fb || ! info->disp)
 		return -ENODEV;
@@ -452,11 +456,13 @@
 		return -EINVAL;
 
 	fb->fb_get_fix(&fix, PROC_CONSOLE(info), info);
-	if (p > fix.smem_len)
+	size = info->mapped_vram ? info->mapped_vram : fix.smem_len;
+	
+	if (p > size)
 	    return -ENOSPC;
 	err = 0;
-	if (count > fix.smem_len - p) {
-	    count = fix.smem_len - p;
+	if (count > size - p) {
+	    count = size - p;
 	    err = -ENOSPC;
 	}
 	if (count) {
@@ -619,7 +625,10 @@
 
 	/* frame buffer memory */
 	start = fix.smem_start;
-	len = PAGE_ALIGN((start & ~PAGE_MASK)+fix.smem_len);
+	if (info->mapped_vram)
+		len = PAGE_ALIGN((start & ~PAGE_MASK) + info->mapped_vram);
+	else
+		len = PAGE_ALIGN((start & ~PAGE_MASK) + fix.smem_len);
 	if (off >= len) {
 		/* memory mapped io */
 		off -= len;
--- a/drivers/video/radeonfb.c	2004-11-30 18:06:45.000000000 +0100
+++ b/drivers/video/radeonfb.c	2004-12-02 14:28:10.000000000 +0100
@@ -176,7 +176,8 @@
 #define RTRACE		if(0) printk
 #endif
 
-
+#define MAX_MAPPED_VRAM (2048*2048*4)
+#define MIN_MAPPED_VRAM (1024*768*1)
 
 enum radeon_chips {
 	RADEON_QD,
@@ -499,7 +500,7 @@
 
 	short chipset;
 	unsigned char arch;
-	int video_ram;
+	unsigned int video_ram;
 	u8 rev;
 	int pitch, bpp, depth;
 	int xres, yres, pixclock;
@@ -1626,6 +1627,7 @@
 				  const struct pci_device_id *ent)
 {
 	struct radeonfb_info *rinfo;
+	struct fb_info *fb_info;
 	struct radeon_chip_info *rci = &radeon_chip_info[ent->driver_data];
 	u32 tmp;
 	int i, j;
@@ -1640,6 +1642,7 @@
 
 	memset (rinfo, 0, sizeof (struct radeonfb_info));
 
+	fb_info = (struct fb_info *)rinfo;
 	rinfo->pdev = pdev;
 	strncpy(rinfo->name, rci->name, 16);
 	rinfo->arch = rci->arch;
@@ -1824,8 +1827,16 @@
 		}
 	}
 
-	rinfo->fb_base = (unsigned long) ioremap (rinfo->fb_base_phys,
-				  		  rinfo->video_ram);
+	fb_info->mapped_vram = min_t(unsigned int, MAX_MAPPED_VRAM, rinfo->video_ram);
+	do {
+		rinfo->fb_base = (unsigned long) ioremap (rinfo->fb_base_phys,
+				  		  fb_info->mapped_vram);
+		if (rinfo->fb_base)
+			break;
+
+		fb_info->mapped_vram /= 2;
+	} while(fb_info->mapped_vram > MIN_MAPPED_VRAM);
+	
 	if (!rinfo->fb_base) {
 		printk ("radeonfb: cannot map FB\n");
 		iounmap ((void*)rinfo->mmio_base);
@@ -1836,6 +1847,7 @@
 		kfree (rinfo);
 		return -ENODEV;
 	}
+	RTRACE(KERN_INFO "radeonfb: mapped %dk videoram\n", fb_info->mapped_vram/1024);
 
 	/* currcon not yet configured, will be set by first switch */
 	rinfo->currcon = -1;
@@ -2199,13 +2211,14 @@
                 {-1, -1}
         };
         int i;
+	struct fb_info *fb_info = (struct fb_info *)rinfo;
                 
         /* use highest possible virtual resolution */
         if (v->xres_virtual == -1 && v->yres_virtual == -1) {
                 printk("radeonfb: using max available virtual resolution\n");
                 for (i=0; modes[i].xres != -1; i++) {
                         if (modes[i].xres * nom / den * modes[i].yres <
-                            rinfo->video_ram / 2)
+                            fb_info->mapped_vram / 2)
                                 break;
                 }
                 if (modes[i].xres == -1) {
@@ -2218,15 +2231,15 @@
                 printk("radeonfb: virtual resolution set to max of %dx%d\n",
                         v->xres_virtual, v->yres_virtual);
         } else if (v->xres_virtual == -1) {
-                v->xres_virtual = (rinfo->video_ram * den /   
+                v->xres_virtual = (fb_info->mapped_vram * den /   
                                 (nom * v->yres_virtual * 2)) & ~15;
         } else if (v->yres_virtual == -1) {
                 v->xres_virtual = (v->xres_virtual + 15) & ~15;
-                v->yres_virtual = rinfo->video_ram * den /
+                v->yres_virtual = fb_info->mapped_vram * den /
                         (nom * v->xres_virtual *2);
         } else {
                 if (v->xres_virtual * nom / den * v->yres_virtual >
-                        rinfo->video_ram) {
+                        fb_info->mapped_vram) {
                         return -EINVAL;
                 }
         }
@@ -2430,6 +2443,9 @@
                         return -EINVAL;
         }
 
+	if (((v.xres_virtual * v.yres_virtual * nom) / den) > info->mapped_vram)
+		return -EINVAL;
+
         if (radeonfb_do_maximize(rinfo, var, &v, nom, den) < 0)
                 return -EINVAL;  
                 

Luca
-- 
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`.

Re: [PATCH 2.4.29-pre1] radeonfb: don't try to ioreamp the entire VRAM [was: Re: [Linux-fbdev-devel] why does radeonfb work fine in 2.6, but not in 2.4.29-pre1?]

[Marcelo Tosatti]

Applied, thanks Geert for your reviewing.

On Thu, Dec 02, 2004 at 04:53:43PM +0100, Kronos wrote:
<snip>
> Ok, here we go:
> 
> Make fb layer aware of the difference between the ioremap()'ed VRAM and
> total available VRAM.
> smem_len in struct fb_fix_screeninfo contains the amount of physical
> VRAM (reported to userspace via FBIOGET_FSCREENINFO ioctl) while the new
> field mapped_vram in struct fb_info contains the amount of VRAM actually
> ioremap()'ed by drivers (used in read/write/mmap operations).
> If mapped_vram is not set it's assumed that the entire framebuffer is
> mapped, thus other drivers are unaffected by this patch.
> 
> Signed-off-by: Luca Tettamanti <kronos@people.it>
> 
> --- a/include/linux/fb.h	2004-11-30 18:30:08.000000000 +0100
> +++ b/include/linux/fb.h	2004-12-02 14:20:50.000000000 +0100
> @@ -323,6 +323,7 @@
>     struct fb_cmap cmap;                 /* Current cmap */
>     struct fb_ops *fbops;
>     char *screen_base;                   /* Virtual address */
> +   u32 mapped_vram;			/* ioremap()'ed VRAM */
>     struct display *disp;		/* initial display variable */
>     struct vc_data *display_fg;		/* Console visible on this display */
>     char fontname[40];			/* default font name */
> --- a/drivers/video/fbmem.c	2004-11-30 18:30:00.000000000 +0100
> +++ b/drivers/video/fbmem.c	2004-12-02 14:29:44.000000000 +0100
> @@ -410,6 +410,7 @@
>  	struct fb_info *info = registered_fb[fbidx];
>  	struct fb_ops *fb = info->fbops;
>  	struct fb_fix_screeninfo fix;
> +	unsigned int size;
>  
>  	if (! fb || ! info->disp)
>  		return -ENODEV;
> @@ -418,10 +419,12 @@
>  		return -EINVAL;
>  
>  	fb->fb_get_fix(&fix,PROC_CONSOLE(info), info);
> -	if (p >= fix.smem_len)
> +	size = info->mapped_vram ? info->mapped_vram : fix.smem_len;
> +	
> +	if (p >= size)
>  	    return 0;
> -	if (count > fix.smem_len - p)
> -		count = fix.smem_len - p;
> +	if (count > size - p)
> +		count = size - p;
>  	if (count) {
>  	    char *base_addr;
>  
> @@ -444,6 +447,7 @@
>  	struct fb_ops *fb = info->fbops;
>  	struct fb_fix_screeninfo fix;
>  	int err;
> +	unsigned int size;
>  
>  	if (! fb || ! info->disp)
>  		return -ENODEV;
> @@ -452,11 +456,13 @@
>  		return -EINVAL;
>  
>  	fb->fb_get_fix(&fix, PROC_CONSOLE(info), info);
> -	if (p > fix.smem_len)
> +	size = info->mapped_vram ? info->mapped_vram : fix.smem_len;
> +	
> +	if (p > size)
>  	    return -ENOSPC;
>  	err = 0;
> -	if (count > fix.smem_len - p) {
> -	    count = fix.smem_len - p;
> +	if (count > size - p) {
> +	    count = size - p;
>  	    err = -ENOSPC;
>  	}
>  	if (count) {
> @@ -619,7 +625,10 @@
>  
>  	/* frame buffer memory */
>  	start = fix.smem_start;
> -	len = PAGE_ALIGN((start & ~PAGE_MASK)+fix.smem_len);
> +	if (info->mapped_vram)
> +		len = PAGE_ALIGN((start & ~PAGE_MASK) + info->mapped_vram);
> +	else
> +		len = PAGE_ALIGN((start & ~PAGE_MASK) + fix.smem_len);
>  	if (off >= len) {
>  		/* memory mapped io */
>  		off -= len;
> --- a/drivers/video/radeonfb.c	2004-11-30 18:06:45.000000000 +0100
> +++ b/drivers/video/radeonfb.c	2004-12-02 14:28:10.000000000 +0100
> @@ -176,7 +176,8 @@
>  #define RTRACE		if(0) printk
>  #endif
>  
> -
> +#define MAX_MAPPED_VRAM (2048*2048*4)
> +#define MIN_MAPPED_VRAM (1024*768*1)
>  
>  enum radeon_chips {
>  	RADEON_QD,
> @@ -499,7 +500,7 @@
>  
>  	short chipset;
>  	unsigned char arch;
> -	int video_ram;
> +	unsigned int video_ram;
>  	u8 rev;
>  	int pitch, bpp, depth;
>  	int xres, yres, pixclock;
> @@ -1626,6 +1627,7 @@
>  				  const struct pci_device_id *ent)
>  {
>  	struct radeonfb_info *rinfo;
> +	struct fb_info *fb_info;
>  	struct radeon_chip_info *rci = &radeon_chip_info[ent->driver_data];
>  	u32 tmp;
>  	int i, j;
> @@ -1640,6 +1642,7 @@
>  
>  	memset (rinfo, 0, sizeof (struct radeonfb_info));
>  
> +	fb_info = (struct fb_info *)rinfo;
>  	rinfo->pdev = pdev;
>  	strncpy(rinfo->name, rci->name, 16);
>  	rinfo->arch = rci->arch;
> @@ -1824,8 +1827,16 @@
>  		}
>  	}
>  
> -	rinfo->fb_base = (unsigned long) ioremap (rinfo->fb_base_phys,
> -				  		  rinfo->video_ram);
> +	fb_info->mapped_vram = min_t(unsigned int, MAX_MAPPED_VRAM, rinfo->video_ram);
> +	do {
> +		rinfo->fb_base = (unsigned long) ioremap (rinfo->fb_base_phys,
> +				  		  fb_info->mapped_vram);
> +		if (rinfo->fb_base)
> +			break;
> +
> +		fb_info->mapped_vram /= 2;
> +	} while(fb_info->mapped_vram > MIN_MAPPED_VRAM);
> +	
>  	if (!rinfo->fb_base) {
>  		printk ("radeonfb: cannot map FB\n");
>  		iounmap ((void*)rinfo->mmio_base);
> @@ -1836,6 +1847,7 @@
>  		kfree (rinfo);
>  		return -ENODEV;
>  	}
> +	RTRACE(KERN_INFO "radeonfb: mapped %dk videoram\n", fb_info->mapped_vram/1024);
>  
>  	/* currcon not yet configured, will be set by first switch */
>  	rinfo->currcon = -1;
> @@ -2199,13 +2211,14 @@
>                  {-1, -1}
>          };
>          int i;
> +	struct fb_info *fb_info = (struct fb_info *)rinfo;
>                  
>          /* use highest possible virtual resolution */
>          if (v->xres_virtual == -1 && v->yres_virtual == -1) {
>                  printk("radeonfb: using max available virtual resolution\n");
>                  for (i=0; modes[i].xres != -1; i++) {
>                          if (modes[i].xres * nom / den * modes[i].yres <
> -                            rinfo->video_ram / 2)
> +                            fb_info->mapped_vram / 2)
>                                  break;
>                  }
>                  if (modes[i].xres == -1) {
> @@ -2218,15 +2231,15 @@
>                  printk("radeonfb: virtual resolution set to max of %dx%d\n",
>                          v->xres_virtual, v->yres_virtual);
>          } else if (v->xres_virtual == -1) {
> -                v->xres_virtual = (rinfo->video_ram * den /   
> +                v->xres_virtual = (fb_info->mapped_vram * den /   
>                                  (nom * v->yres_virtual * 2)) & ~15;
>          } else if (v->yres_virtual == -1) {
>                  v->xres_virtual = (v->xres_virtual + 15) & ~15;
> -                v->yres_virtual = rinfo->video_ram * den /
> +                v->yres_virtual = fb_info->mapped_vram * den /
>                          (nom * v->xres_virtual *2);
>          } else {
>                  if (v->xres_virtual * nom / den * v->yres_virtual >
> -                        rinfo->video_ram) {
> +                        fb_info->mapped_vram) {
>                          return -EINVAL;
>                  }
>          }
> @@ -2430,6 +2443,9 @@
>                          return -EINVAL;
>          }
>  
> +	if (((v.xres_virtual * v.yres_virtual * nom) / den) > info->mapped_vram)
> +		return -EINVAL;
> +
>          if (radeonfb_do_maximize(rinfo, var, &v, nom, den) < 0)
>                  return -EINVAL;  
>                  
> 
> Luca
> -- 
> 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`.

Re: [PATCH 2.4.29-pre1] radeonfb: don't try to ioreamp the entire VRAM [was: Re: [Linux-fbdev-devel] why does radeonfb work fine in 2.6, but not in 2.4.29-pre1?]

[Antonino A. Daplas]

On Thursday 02 December 2004 04:37, Kronos wrote:
> Il Wed, Dec 01, 2004 at 05:25:52PM +0100, Geert Uytterhoeven ha scritto:
> > > Make fb layer aware of the difference between the ioremap()'ed VRAM and
> > > total available VRAM.
> > > smem_len in struct fb_fix_screeninfo still contains the amount of
> > > physical VRAM (reported to userspace via FBIOGET_FSCREENINFO ioctl) and
> > > the new field mapped_vram contains the amount of VRAM actually
> > > ioremap()'ed by drivers (used in read/write/mmap operations).
> > > Since there was unused padding at the end of struct fb_fix_screeninfo
> > > binary compatibility with userspace utilities is retained.
> > > If mapped_vram is not set it's assumed that the entire framebuffer is
> > > mapped, thus other drivers are unaffected by this patch.
> > >
> > > The patch has been tested by Jurriaan <thunder7@xs4all.nl>.
> > >
> > > Signed-off-by: Luca Tettamanti <kronos@people.it>
> > >
> > > --- a/include/linux/fb.h	2004-11-30 18:30:08.000000000 +0100
> > > +++ b/include/linux/fb.h	2004-11-30 18:33:00.000000000 +0100
> > > @@ -126,7 +126,8 @@
> > >  					/* (physical address) */
> > >  	__u32 mmio_len;			/* Length of Memory Mapped I/O  */
> > >  	__u32 accel;			/* Type of acceleration available */
> > > -	__u16 reserved[3];		/* Reserved for future compatibility */
> > > +	__u32 mapped_vram;		/* Amount of ioremap()'ed VRAM */
> > > +	__u16 reserved[1];		/* Reserved for future compatibility */
> > >  };
> >
> > I don't really like this patch. mapped_vram doesn't matter for user space
> > at all, so it does not belong to fb_fix_screeninfo.
>
> Hmm, it looked sensible to me since it's the max amount of data that
> userspace can read (or write) from /dev/fb%d.

Userspace already use fix.line_length * var.yres_virtual to compute the
amount it can access, which is not necessarily equal to the mapped vram.

> Putting mapped_vram in fb_info would be acceptable?

Yes, that is a more sensible solution.

Tony