Re: [PATCH 2.4.29-pre1] radeonfb: don't try to ioreamp the entireVRAM [was: Re: [Linux-fbdev-devel] why does radeonfb work fine in 2.6, butnot 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@xxxxxxxxx>.
> > > > > 
> > > > > Signed-off-by: Luca Tettamanti <kronos@xxxxxxxxx>
> > > > > 
> > > > > --- 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@xxxxxxxxxxxxxx

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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/