* Linux 2.4.29-pre1 (was: Re: [PATCH] vga16fb: Fix frame buffer bad memory mapping) [not found] <200411252301.iAPN1mo4023046@hera.kernel.org> @ 2004-11-27 15:54 ` Geert Uytterhoeven 2004-11-30 11:29 ` Marcelo Tosatti 0 siblings, 1 reply; 4+ messages in thread From: Geert Uytterhoeven @ 2004-11-27 15:54 UTC (permalink / raw) To: vince, Marcelo Tosatti Cc: Linux Kernel Development, Linux Frame Buffer Device Development On Thu, 25 Nov 2004, Linux Kernel Mailing List wrote: > ChangeSet 1.1543, 2004/11/25 13:16:49-02:00, vince@arm.linux.org.uk > > [PATCH] vga16fb: Fix frame buffer bad memory mapping > > The vga16fb driver uses a direct ioremap on 0xa00000 to gain access to > the vga card. This is wrong on architectures other than x86, every > other driver uses VGA_MAP_MEM macro from vga.h to ensure the correct > memory mapping. > > All this patch does is add the mapping macro this has been tested and > works on ARM systems The driver no longer maps parts of kernel > workspace and modifies it. This fix is not correct! > diff -Nru a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c > --- a/drivers/video/vga16fb.c 2004-11-25 15:01:51 -08:00 > +++ b/drivers/video/vga16fb.c 2004-11-25 15:01:51 -08:00 > @@ -142,7 +142,7 @@ > memset(fix, 0, sizeof(struct fb_fix_screeninfo)); > strcpy(fix->id,"VGA16 VGA"); > > - fix->smem_start = VGA_FB_PHYS; > + fix->smem_start = VGA_MAP_MEM(VGA_FB_PHYS); > fix->smem_len = VGA_FB_PHYS_LEN; > fix->type = FB_TYPE_VGA_PLANES; > fix->visual = FB_VISUAL_PSEUDOCOLOR; fix->smem_start: Although I agree VGA_FB_PHYS is not the correct value on machines other than PC, VGA_MAP_MEM(VGA_FB_PHYS) is not appropriate either, because fix->smem_start is supposed to be a CPU _physical_ address, not a virtual address. However, this value isn't really used, except by (very rare) userspace that wants to mmap the frame buffer through /dev/mem instead of /dev/fb*, so an incorrect value doesn't really harm. > @@ -896,7 +896,7 @@ > > /* XXX share VGA_FB_PHYS region with vgacon */ > > - vga16fb.video_vbase = ioremap(VGA_FB_PHYS, VGA_FB_PHYS_LEN); > + vga16fb.video_vbase = ioremap(VGA_MAP_MEM(VGA_FB_PHYS), VGA_FB_PHYS_LEN); > if (!vga16fb.video_vbase) { > printk(KERN_ERR "vga16fb: unable to map device\n"); > return -ENOMEM; ioremap(): VGA_MAP_MEM() already a _virtual_ address: | include/asm-alpha/vga.h:#define VGA_MAP_MEM(x) ((unsigned long) ioremap((x), 0)) | include/asm-arm/vga.h:#define VGA_MAP_MEM(x) (PCIMEM_BASE + (x)) | include/asm-i386/vga.h:#define VGA_MAP_MEM(x) (unsigned long)phys_to_virt(x) | include/asm-ia64/vga.h:#define VGA_MAP_MEM(x) ((unsigned long) ioremap((x), 0)) | include/asm-mips/vga.h:#define VGA_MAP_MEM(x) ((unsigned long)0xb0000000 + (unsigned long)(x)) | include/asm-ppc/vga.h:#define VGA_MAP_MEM(x) (x + vgacon_remap_base) | include/asm-ppc64/vga.h:#define VGA_MAP_MEM(x) ((unsigned long) ioremap((x), 0)) | include/asm-sparc64/vga.h:#define VGA_MAP_MEM(x) (x) | include/asm-x86_64/vga.h:#define VGA_MAP_MEM(x) (unsigned long)phys_to_virt(x) Doing a double ioremap(), or ioremap(phys_to_virt()) will break for sure... BTW, on (PReP/CHRP) PPC ioremap() on ISA memory space `just works' because __ioremap() adds _ISA_MEM_BASE to the passed pointer if it's smaller than 16*1024*1024. And yes, vga16fb used to work fine on my CHRP LongTrail (before the machine itself died :-(. Yes, ISA memory space is a mess to do right in a portable way... 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Linux 2.4.29-pre1 (was: Re: [PATCH] vga16fb: Fix frame buffer bad memory mapping) 2004-11-27 15:54 ` Linux 2.4.29-pre1 (was: Re: [PATCH] vga16fb: Fix frame buffer bad memory mapping) Geert Uytterhoeven @ 2004-11-30 11:29 ` Marcelo Tosatti 2004-12-01 8:48 ` Geert Uytterhoeven 0 siblings, 1 reply; 4+ messages in thread From: Marcelo Tosatti @ 2004-11-30 11:29 UTC (permalink / raw) To: Geert Uytterhoeven Cc: vince, Linux Kernel Development, Linux Frame Buffer Device Development On Sat, Nov 27, 2004 at 04:54:12PM +0100, Geert Uytterhoeven wrote: > On Thu, 25 Nov 2004, Linux Kernel Mailing List wrote: > > ChangeSet 1.1543, 2004/11/25 13:16:49-02:00, vince@arm.linux.org.uk > > > > [PATCH] vga16fb: Fix frame buffer bad memory mapping > > > > The vga16fb driver uses a direct ioremap on 0xa00000 to gain access to > > the vga card. This is wrong on architectures other than x86, every > > other driver uses VGA_MAP_MEM macro from vga.h to ensure the correct > > memory mapping. > > > > All this patch does is add the mapping macro this has been tested and > > works on ARM systems The driver no longer maps parts of kernel > > workspace and modifies it. > > This fix is not correct! > > > diff -Nru a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c > > --- a/drivers/video/vga16fb.c 2004-11-25 15:01:51 -08:00 > > +++ b/drivers/video/vga16fb.c 2004-11-25 15:01:51 -08:00 > > @@ -142,7 +142,7 @@ > > memset(fix, 0, sizeof(struct fb_fix_screeninfo)); > > strcpy(fix->id,"VGA16 VGA"); > > > > - fix->smem_start = VGA_FB_PHYS; > > + fix->smem_start = VGA_MAP_MEM(VGA_FB_PHYS); > > fix->smem_len = VGA_FB_PHYS_LEN; > > fix->type = FB_TYPE_VGA_PLANES; > > fix->visual = FB_VISUAL_PSEUDOCOLOR; > > fix->smem_start: Although I agree VGA_FB_PHYS is not the correct value on > machines other than PC, VGA_MAP_MEM(VGA_FB_PHYS) is not appropriate either, > because fix->smem_start is supposed to be a CPU _physical_ address, not a > virtual address. > > However, this value isn't really used, except by (very rare) userspace that > wants to mmap the frame buffer through /dev/mem instead of /dev/fb*, so an > incorrect value doesn't really harm. > > > @@ -896,7 +896,7 @@ > > > > /* XXX share VGA_FB_PHYS region with vgacon */ > > > > - vga16fb.video_vbase = ioremap(VGA_FB_PHYS, VGA_FB_PHYS_LEN); > > + vga16fb.video_vbase = ioremap(VGA_MAP_MEM(VGA_FB_PHYS), VGA_FB_PHYS_LEN); > > if (!vga16fb.video_vbase) { > > printk(KERN_ERR "vga16fb: unable to map device\n"); > > return -ENOMEM; > > ioremap(): VGA_MAP_MEM() already a _virtual_ address: > > | include/asm-alpha/vga.h:#define VGA_MAP_MEM(x) ((unsigned long) ioremap((x), 0)) > | include/asm-arm/vga.h:#define VGA_MAP_MEM(x) (PCIMEM_BASE + (x)) > | include/asm-i386/vga.h:#define VGA_MAP_MEM(x) (unsigned long)phys_to_virt(x) > | include/asm-ia64/vga.h:#define VGA_MAP_MEM(x) ((unsigned long) ioremap((x), 0)) > | include/asm-mips/vga.h:#define VGA_MAP_MEM(x) ((unsigned long)0xb0000000 + (unsigned long)(x)) > | include/asm-ppc/vga.h:#define VGA_MAP_MEM(x) (x + vgacon_remap_base) > | include/asm-ppc64/vga.h:#define VGA_MAP_MEM(x) ((unsigned long) ioremap((x), 0)) > | include/asm-sparc64/vga.h:#define VGA_MAP_MEM(x) (x) > | include/asm-x86_64/vga.h:#define VGA_MAP_MEM(x) (unsigned long)phys_to_virt(x) > > Doing a double ioremap(), or ioremap(phys_to_virt()) will break for sure... > > BTW, on (PReP/CHRP) PPC ioremap() on ISA memory space `just works' because > __ioremap() adds _ISA_MEM_BASE to the passed pointer if it's smaller than > 16*1024*1024. And yes, vga16fb used to work fine on my CHRP LongTrail > (before the machine itself died :-(. > > Yes, ISA memory space is a mess to do right in a portable way... Geert, I'll guest I'll just revert the whole change then... Do you have a better suggestion? Thanks! ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Linux 2.4.29-pre1 (was: Re: [PATCH] vga16fb: Fix frame buffer bad memory mapping) 2004-11-30 11:29 ` Marcelo Tosatti @ 2004-12-01 8:48 ` Geert Uytterhoeven 2004-12-01 10:59 ` Marcelo Tosatti 0 siblings, 1 reply; 4+ messages in thread From: Geert Uytterhoeven @ 2004-12-01 8:48 UTC (permalink / raw) To: Marcelo Tosatti Cc: vince, Linux Kernel Development, Linux Frame Buffer Device Development On Tue, 30 Nov 2004, Marcelo Tosatti wrote: > On Sat, Nov 27, 2004 at 04:54:12PM +0100, Geert Uytterhoeven wrote: > > On Thu, 25 Nov 2004, Linux Kernel Mailing List wrote: > > > ChangeSet 1.1543, 2004/11/25 13:16:49-02:00, vince@arm.linux.org.uk > > > > > > [PATCH] vga16fb: Fix frame buffer bad memory mapping > > > > > > The vga16fb driver uses a direct ioremap on 0xa00000 to gain access to > > > the vga card. This is wrong on architectures other than x86, every > > > other driver uses VGA_MAP_MEM macro from vga.h to ensure the correct > > > memory mapping. > > > > > > All this patch does is add the mapping macro this has been tested and > > > works on ARM systems The driver no longer maps parts of kernel > > > workspace and modifies it. > > > > This fix is not correct! > > > > > diff -Nru a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c > > > --- a/drivers/video/vga16fb.c 2004-11-25 15:01:51 -08:00 > > > +++ b/drivers/video/vga16fb.c 2004-11-25 15:01:51 -08:00 > > > @@ -142,7 +142,7 @@ > > > memset(fix, 0, sizeof(struct fb_fix_screeninfo)); > > > strcpy(fix->id,"VGA16 VGA"); > > > > > > - fix->smem_start = VGA_FB_PHYS; > > > + fix->smem_start = VGA_MAP_MEM(VGA_FB_PHYS); > > > fix->smem_len = VGA_FB_PHYS_LEN; > > > fix->type = FB_TYPE_VGA_PLANES; > > > fix->visual = FB_VISUAL_PSEUDOCOLOR; > > > > fix->smem_start: Although I agree VGA_FB_PHYS is not the correct value on > > machines other than PC, VGA_MAP_MEM(VGA_FB_PHYS) is not appropriate either, > > because fix->smem_start is supposed to be a CPU _physical_ address, not a > > virtual address. > > > > However, this value isn't really used, except by (very rare) userspace that > > wants to mmap the frame buffer through /dev/mem instead of /dev/fb*, so an > > incorrect value doesn't really harm. > > > > > @@ -896,7 +896,7 @@ > > > > > > /* XXX share VGA_FB_PHYS region with vgacon */ > > > > > > - vga16fb.video_vbase = ioremap(VGA_FB_PHYS, VGA_FB_PHYS_LEN); > > > + vga16fb.video_vbase = ioremap(VGA_MAP_MEM(VGA_FB_PHYS), VGA_FB_PHYS_LEN); > > > if (!vga16fb.video_vbase) { > > > printk(KERN_ERR "vga16fb: unable to map device\n"); > > > return -ENOMEM; > > > > ioremap(): VGA_MAP_MEM() already a _virtual_ address: > > > > | include/asm-alpha/vga.h:#define VGA_MAP_MEM(x) ((unsigned long) ioremap((x), 0)) > > | include/asm-arm/vga.h:#define VGA_MAP_MEM(x) (PCIMEM_BASE + (x)) > > | include/asm-i386/vga.h:#define VGA_MAP_MEM(x) (unsigned long)phys_to_virt(x) > > | include/asm-ia64/vga.h:#define VGA_MAP_MEM(x) ((unsigned long) ioremap((x), 0)) > > | include/asm-mips/vga.h:#define VGA_MAP_MEM(x) ((unsigned long)0xb0000000 + (unsigned long)(x)) > > | include/asm-ppc/vga.h:#define VGA_MAP_MEM(x) (x + vgacon_remap_base) > > | include/asm-ppc64/vga.h:#define VGA_MAP_MEM(x) ((unsigned long) ioremap((x), 0)) > > | include/asm-sparc64/vga.h:#define VGA_MAP_MEM(x) (x) > > | include/asm-x86_64/vga.h:#define VGA_MAP_MEM(x) (unsigned long)phys_to_virt(x) > > > > Doing a double ioremap(), or ioremap(phys_to_virt()) will break for sure... > > > > BTW, on (PReP/CHRP) PPC ioremap() on ISA memory space `just works' because > > __ioremap() adds _ISA_MEM_BASE to the passed pointer if it's smaller than > > 16*1024*1024. And yes, vga16fb used to work fine on my CHRP LongTrail > > (before the machine itself died :-(. > > > > Yes, ISA memory space is a mess to do right in a portable way... > > Geert, > > I'll guest I'll just revert the whole change then... Do you have a Yes, please. > better suggestion? Replacing vga16fb.video_vbase = ioremap(VGA_FB_PHYS, VGA_FB_PHYS_LEN); by vga16fb.video_vbase = VGA_MAP_MEM(VGA_FB_PHYS); will probably work, since that's what vgacon does, but I'd like to see it tested first anyway. 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 ------------------------------------------------------- SF email is sponsored by - The IT Product Guide Read honest & candid reviews on hundreds of IT Products from real users. Discover which products truly live up to the hype. Start reading now. http://productguide.itmanagersjournal.com/ ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Linux 2.4.29-pre1 (was: Re: [PATCH] vga16fb: Fix frame buffer bad memory mapping) 2004-12-01 8:48 ` Geert Uytterhoeven @ 2004-12-01 10:59 ` Marcelo Tosatti 0 siblings, 0 replies; 4+ messages in thread From: Marcelo Tosatti @ 2004-12-01 10:59 UTC (permalink / raw) To: Geert Uytterhoeven Cc: vince, Linux Kernel Development, Linux Frame Buffer Device Development On Wed, Dec 01, 2004 at 09:48:29AM +0100, Geert Uytterhoeven wrote: > On Tue, 30 Nov 2004, Marcelo Tosatti wrote: > > On Sat, Nov 27, 2004 at 04:54:12PM +0100, Geert Uytterhoeven wrote: > > > On Thu, 25 Nov 2004, Linux Kernel Mailing List wrote: > > > > ChangeSet 1.1543, 2004/11/25 13:16:49-02:00, vince@arm.linux.org.uk > > > > > > > > [PATCH] vga16fb: Fix frame buffer bad memory mapping > > > > > > > > The vga16fb driver uses a direct ioremap on 0xa00000 to gain access to > > > > the vga card. This is wrong on architectures other than x86, every > > > > other driver uses VGA_MAP_MEM macro from vga.h to ensure the correct > > > > memory mapping. > > > > > > > > All this patch does is add the mapping macro this has been tested and > > > > works on ARM systems The driver no longer maps parts of kernel > > > > workspace and modifies it. > > > > > > This fix is not correct! > > > > > > > diff -Nru a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c > > > > --- a/drivers/video/vga16fb.c 2004-11-25 15:01:51 -08:00 > > > > +++ b/drivers/video/vga16fb.c 2004-11-25 15:01:51 -08:00 > > > > @@ -142,7 +142,7 @@ > > > > memset(fix, 0, sizeof(struct fb_fix_screeninfo)); > > > > strcpy(fix->id,"VGA16 VGA"); > > > > > > > > - fix->smem_start = VGA_FB_PHYS; > > > > + fix->smem_start = VGA_MAP_MEM(VGA_FB_PHYS); > > > > fix->smem_len = VGA_FB_PHYS_LEN; > > > > fix->type = FB_TYPE_VGA_PLANES; > > > > fix->visual = FB_VISUAL_PSEUDOCOLOR; > > > > > > fix->smem_start: Although I agree VGA_FB_PHYS is not the correct value on > > > machines other than PC, VGA_MAP_MEM(VGA_FB_PHYS) is not appropriate either, > > > because fix->smem_start is supposed to be a CPU _physical_ address, not a > > > virtual address. > > > > > > However, this value isn't really used, except by (very rare) userspace that > > > wants to mmap the frame buffer through /dev/mem instead of /dev/fb*, so an > > > incorrect value doesn't really harm. > > > > > > > @@ -896,7 +896,7 @@ > > > > > > > > /* XXX share VGA_FB_PHYS region with vgacon */ > > > > > > > > - vga16fb.video_vbase = ioremap(VGA_FB_PHYS, VGA_FB_PHYS_LEN); > > > > + vga16fb.video_vbase = ioremap(VGA_MAP_MEM(VGA_FB_PHYS), VGA_FB_PHYS_LEN); > > > > if (!vga16fb.video_vbase) { > > > > printk(KERN_ERR "vga16fb: unable to map device\n"); > > > > return -ENOMEM; > > > > > > ioremap(): VGA_MAP_MEM() already a _virtual_ address: > > > > > > | include/asm-alpha/vga.h:#define VGA_MAP_MEM(x) ((unsigned long) ioremap((x), 0)) > > > | include/asm-arm/vga.h:#define VGA_MAP_MEM(x) (PCIMEM_BASE + (x)) > > > | include/asm-i386/vga.h:#define VGA_MAP_MEM(x) (unsigned long)phys_to_virt(x) > > > | include/asm-ia64/vga.h:#define VGA_MAP_MEM(x) ((unsigned long) ioremap((x), 0)) > > > | include/asm-mips/vga.h:#define VGA_MAP_MEM(x) ((unsigned long)0xb0000000 + (unsigned long)(x)) > > > | include/asm-ppc/vga.h:#define VGA_MAP_MEM(x) (x + vgacon_remap_base) > > > | include/asm-ppc64/vga.h:#define VGA_MAP_MEM(x) ((unsigned long) ioremap((x), 0)) > > > | include/asm-sparc64/vga.h:#define VGA_MAP_MEM(x) (x) > > > | include/asm-x86_64/vga.h:#define VGA_MAP_MEM(x) (unsigned long)phys_to_virt(x) > > > > > > Doing a double ioremap(), or ioremap(phys_to_virt()) will break for sure... > > > > > > BTW, on (PReP/CHRP) PPC ioremap() on ISA memory space `just works' because > > > __ioremap() adds _ISA_MEM_BASE to the passed pointer if it's smaller than > > > 16*1024*1024. And yes, vga16fb used to work fine on my CHRP LongTrail > > > (before the machine itself died :-(. > > > > > > Yes, ISA memory space is a mess to do right in a portable way... > > > > Geert, > > > > I'll guest I'll just revert the whole change then... Do you have a > > Yes, please. Done. > > > better suggestion? > > Replacing > > vga16fb.video_vbase = ioremap(VGA_FB_PHYS, VGA_FB_PHYS_LEN); > > by > > vga16fb.video_vbase = VGA_MAP_MEM(VGA_FB_PHYS); > > will probably work, since that's what vgacon does, but I'd like to see it > tested first anyway. I think Andrew merged a similar patch into v2.6 months ago. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2004-12-01 10:59 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <200411252301.iAPN1mo4023046@hera.kernel.org>
2004-11-27 15:54 ` Linux 2.4.29-pre1 (was: Re: [PATCH] vga16fb: Fix frame buffer bad memory mapping) Geert Uytterhoeven
2004-11-30 11:29 ` Marcelo Tosatti
2004-12-01 8:48 ` Geert Uytterhoeven
2004-12-01 10:59 ` Marcelo Tosatti
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).