Re: 2.6.17-rc1-mm1

Re: 2.6.17-rc1-mm1

[Luck, Tony]

On Tue, Apr 04, 2006 at 01:45:04AM -0700, Andrew Morton wrote:
> - VGA on ia64 is broken - the screen comes up blank.  But ia64 otherwise
>   seems to work OK.  I didn't have time to investigate.

Broken in base 2.6.17-rc1 too :-(  VGA comes up and prints a
few messages, and then goes wonky and dies.  Comparing
what I _think_ I saw with the dmesg output, it appears to
die here:

[    6.416740] ACPI: PCI Interrupt 0000:01:00.0[A] -> GSI 18 (level, low) -> IRQ 48
[    6.708439] e1000: 0000:01:00.0: e1000_probe: (PCI:33MHz:32-bit) 00:03:47:fd:bb:42
[    6.754439] e1000: eth0: e1000_probe: Intel(R) PRO/1000 Network Connection
[    6.761547] netconsole: not configured, aborting
[    6.766195] initcall at 0xa0000001007c4c30: init_netconsole+0x0/0x140(): returned with error code -22
[    6.775520] Uniform Multi-Platform E-IDE driver Revision: 7.00alpha2

    DEAD DEAD DEAD

Should have gone on to say:
[    6.781924] ide: Assuming 33MHz system bus speed for PIO modes; override with idebus=xx
[    6.790052] ICH4: IDE controller at PCI slot 0000:00:1f.1
[    6.795513] PCI: Device 0000:00:1f.1 not available because of resource collisions
[    6.803100] ACPI: PCI Interrupt 0000:00:1f.1[A]: no GSI
[    6.808403] ICH4: BIOS configuration fixed.
[    6.812646] ICH4: chipset revision 1
[    6.816271] ICH4: not 100% native mode: will probe irqs later


But I might be off by a line or two, the last bit flashed by quite quickly.

I'll start bisecting tomorrow to see when it was broken.

-Tony

Re: 2.6.17-rc1-mm1

[Zou Nan hai]

On Wed, 2006-04-05 at 07:38, Luck, Tony wrote:
> On Tue, Apr 04, 2006 at 01:45:04AM -0700, Andrew Morton wrote:
> > - VGA on ia64 is broken - the screen comes up blank.  But ia64 otherwise
> >   seems to work OK.  I didn't have time to investigate.
> 
> Broken in base 2.6.17-rc1 too :-(  VGA comes up and prints a
> few messages, and then goes wonky and dies.  Comparing
> what I _think_ I saw with the dmesg output, it appears to
> die here:
> 

The wild VGA comes from the patch which changed ioremap.

Now ioremap would not remap memory to region 6 unless that memory is
marked as EFI_MEMORY_UC by EFI.

Unfortunately on the Tiger Machine, VGA ram base was marked as
EFI_MEMORY_WB instead of EFI_MEMORY_UC...

So you can see the problem disappear if change VGA_MAP_MEM to use
ioremap_nocache.

But I am not quite sure if we can fully trust EFI on this attribute.

Zou Nan hai

Re: 2.6.17-rc1-mm1

[Bjorn Helgaas]

On Tuesday 04 April 2006 20:05, Zou Nan hai wrote:
> On Wed, 2006-04-05 at 07:38, Luck, Tony wrote:
> > On Tue, Apr 04, 2006 at 01:45:04AM -0700, Andrew Morton wrote:
> > > - VGA on ia64 is broken - the screen comes up blank.  But ia64 otherwise
> > >   seems to work OK.  I didn't have time to investigate.
> > 
> > Broken in base 2.6.17-rc1 too :-(  VGA comes up and prints a
> > few messages, and then goes wonky and dies.  Comparing
> > what I _think_ I saw with the dmesg output, it appears to
> > die here:
> 
> The wild VGA comes from the patch which changed ioremap.
> 
> Now ioremap would not remap memory to region 6 unless that memory is
> marked as EFI_MEMORY_UC by EFI.
> 
> Unfortunately on the Tiger Machine, VGA ram base was marked as
> EFI_MEMORY_WB instead of EFI_MEMORY_UC...
> 
> So you can see the problem disappear if change VGA_MAP_MEM to use
> ioremap_nocache.
> 
> But I am not quite sure if we can fully trust EFI on this attribute.

Huh.  Intel firmware used to just not mention the VGA framebuffer
(0xa0000-0xc0000) at all in the EFI memory map.  I think that was
clearly a bug.  So maybe they fixed that by marking it WB (and
hopefully UC as well).

Tiger might support WB to the VGA framebuffer, but I don't think it
make much sense for us to use it that way.  If we did, writes to the
framebuffer would just sit in the cache until forced out by something
else.  Probably using WC would be the best, but we don't have a good
interface for that yet.

Tony, if you agree with this analysis and haven't fixed it yet, here's
a trivial patch.


[IA64] always map VGA framebuffer UC, even if it supports WB

EFI on some machines, e.g., Intel Tiger, reports that the VGA framebuffer
supports WB access.  ioremap() prefers WB when possible, so it can work
when mapping main memory.

But it doesn't make sense to map a framebuffer WB, because the driver
doesn't flush explicitly, so updates won't make it to the device
immediately.

This is due to Zou Nan hai <nanhai.zou@intel.com>.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Index: work-mm5/include/asm-ia64/vga.h
=================================--- work-mm5.orig/include/asm-ia64/vga.h	2006-01-02 20:21:10.000000000 -0700
+++ work-mm5/include/asm-ia64/vga.h	2006-04-05 09:57:55.000000000 -0600
@@ -17,7 +17,7 @@
 extern unsigned long vga_console_iobase;
 extern unsigned long vga_console_membase;
 
-#define VGA_MAP_MEM(x)	((unsigned long) ioremap(vga_console_membase + (x), 0))
+#define VGA_MAP_MEM(x)	((unsigned long) ioremap_nocache(vga_console_membase + (x), 0))
 
 #define vga_readb(x)	(*(x))
 #define vga_writeb(x,y)	(*(y) = (x))

Re: 2.6.17-rc1-mm1

[Luck, Tony]

On Wed, Apr 05, 2006 at 10:15:34AM -0600, Bjorn Helgaas wrote:
> Huh.  Intel firmware used to just not mention the VGA framebuffer
> (0xa0000-0xc0000) at all in the EFI memory map.  I think that was
> clearly a bug.  So maybe they fixed that by marking it WB (and
> hopefully UC as well).

Nope ... not fixed (at least not in the f/w that I'm running). The
VGA buffer is still simply not mentioned in the EFI memory map.

The problem looks to come from this code in vgacon.c:

	vga_vram_base = VGA_MAP_MEM(vga_vram_base);
	vga_vram_end = VGA_MAP_MEM(vga_vram_end);
	vga_vram_size = vga_vram_end - vga_vram_base;

vga_vram_base is 0xb8000, and this call gets a UC return of
c0000000000b8000.  But vga_vram_end is 0xc0000 ... which is
the address of the start of a block of memory that is both
WB and UC capable.  So ioremap() gives us e0000000000c0000
(which means that vga_vram_size is 2000000000008000, surely
the biggest, baddest video card in the history of the world!).

Perhaps the right fix is to subtract 1 from vga_vram_end and pass
that into VGA_MAP_MEM(), and then add the 1 byte back when computing
the size?  But I don't know whether that might do something bad on
some other architecture that uses vgacon.c.  If this is not
acceptable, then we can fall back and use the Nanhai/Bjorn fix
of using ioremap_nocache().

Signed-off-by: Tony Luck <tony.luck@intel.com>

---

diff --git a/drivers/video/console/vgacon.c b/drivers/video/console/vgacon.c
index d5a04b6..4ca9877 100644
--- a/drivers/video/console/vgacon.c
+++ b/drivers/video/console/vgacon.c
@@ -484,8 +484,8 @@ #endif
 	}
 
 	vga_vram_base = VGA_MAP_MEM(vga_vram_base);
-	vga_vram_end = VGA_MAP_MEM(vga_vram_end);
-	vga_vram_size = vga_vram_end - vga_vram_base;
+	vga_vram_end = VGA_MAP_MEM(vga_vram_end - 1);
+	vga_vram_size = vga_vram_end - vga_vram_base + 1;
 
 	/*
 	 *      Find out if there is a graphics card present.

Re: 2.6.17-rc1-mm1

[Andrew Morton]

"Luck, Tony" <tony.luck@intel.com> wrote:
>
> On Wed, Apr 05, 2006 at 10:15:34AM -0600, Bjorn Helgaas wrote:
> > Huh.  Intel firmware used to just not mention the VGA framebuffer
> > (0xa0000-0xc0000) at all in the EFI memory map.  I think that was
> > clearly a bug.  So maybe they fixed that by marking it WB (and
> > hopefully UC as well).
> 
> Nope ... not fixed (at least not in the f/w that I'm running). The
> VGA buffer is still simply not mentioned in the EFI memory map.
> 
> The problem looks to come from this code in vgacon.c:
> 
> 	vga_vram_base = VGA_MAP_MEM(vga_vram_base);
> 	vga_vram_end = VGA_MAP_MEM(vga_vram_end);
> 	vga_vram_size = vga_vram_end - vga_vram_base;
> 
> vga_vram_base is 0xb8000, and this call gets a UC return of
> c0000000000b8000.  But vga_vram_end is 0xc0000 ... which is
> the address of the start of a block of memory that is both
> WB and UC capable.

OK, so it's really an off-by-one error.

>  So ioremap() gives us e0000000000c0000
> (which means that vga_vram_size is 2000000000008000, surely
> the biggest, baddest video card in the history of the world!).
> 
> Perhaps the right fix is to subtract 1 from vga_vram_end and pass
> that into VGA_MAP_MEM(), and then add the 1 byte back when computing
> the size?  But I don't know whether that might do something bad on
> some other architecture that uses vgacon.c.  If this is not
> acceptable, then we can fall back and use the Nanhai/Bjorn fix
> of using ioremap_nocache().
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> 
> ---
> 
> diff --git a/drivers/video/console/vgacon.c b/drivers/video/console/vgacon.c
> index d5a04b6..4ca9877 100644
> --- a/drivers/video/console/vgacon.c
> +++ b/drivers/video/console/vgacon.c
> @@ -484,8 +484,8 @@ #endif
>  	}
>  
>  	vga_vram_base = VGA_MAP_MEM(vga_vram_base);
> -	vga_vram_end = VGA_MAP_MEM(vga_vram_end);
> -	vga_vram_size = vga_vram_end - vga_vram_base;
> +	vga_vram_end = VGA_MAP_MEM(vga_vram_end - 1);
> +	vga_vram_size = vga_vram_end - vga_vram_base + 1;
>  
>  	/*
>  	 *      Find out if there is a graphics card present.

Looks like the correct fix to me.

Tony (D), can you think of any problems with that approach?

Re: 2.6.17-rc1-mm1

[Andreas Schwab]

"Luck, Tony" <tony.luck@intel.com> writes:

> diff --git a/drivers/video/console/vgacon.c b/drivers/video/console/vgacon.c
> index d5a04b6..4ca9877 100644
> --- a/drivers/video/console/vgacon.c
> +++ b/drivers/video/console/vgacon.c
> @@ -484,8 +484,8 @@ #endif
>  	}
>  
>  	vga_vram_base = VGA_MAP_MEM(vga_vram_base);
> -	vga_vram_end = VGA_MAP_MEM(vga_vram_end);
> -	vga_vram_size = vga_vram_end - vga_vram_base;
> +	vga_vram_end = VGA_MAP_MEM(vga_vram_end - 1);
> +	vga_vram_size = vga_vram_end - vga_vram_base + 1;

Better use vga_vram_end = VGA_MAP_MEM(vga_vram_end - 1) + 1, or you'll
screw up the other computations using vga_vram_end.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: 2.6.17-rc1-mm1

[Bjorn Helgaas]

On Wednesday 05 April 2006 15:17, Luck, Tony wrote:
> On Wed, Apr 05, 2006 at 10:15:34AM -0600, Bjorn Helgaas wrote:
> > Huh.  Intel firmware used to just not mention the VGA framebuffer
> > (0xa0000-0xc0000) at all in the EFI memory map.  I think that was
> > clearly a bug.  So maybe they fixed that by marking it WB (and
> > hopefully UC as well).
> 
> Nope ... not fixed (at least not in the f/w that I'm running). The
> VGA buffer is still simply not mentioned in the EFI memory map.
> 
> The problem looks to come from this code in vgacon.c:
> 
> 	vga_vram_base = VGA_MAP_MEM(vga_vram_base);
> 	vga_vram_end = VGA_MAP_MEM(vga_vram_end);
> 	vga_vram_size = vga_vram_end - vga_vram_base;
> 
> vga_vram_base is 0xb8000, and this call gets a UC return of
> c0000000000b8000.  But vga_vram_end is 0xc0000 ... which is
> the address of the start of a block of memory that is both
> WB and UC capable.  So ioremap() gives us e0000000000c0000
> (which means that vga_vram_size is 2000000000008000, surely
> the biggest, baddest video card in the history of the world!).
> 
> Perhaps the right fix is to subtract 1 from vga_vram_end and pass
> that into VGA_MAP_MEM(), and then add the 1 byte back when computing
> the size?  But I don't know whether that might do something bad on
> some other architecture that uses vgacon.c.

I think the VGA_MAP_MEM(vga_vram_end) is just bogus -- it's not
that we need to map the memory starting at vga_vram_end.  I think
it would cleaner (though much more intrusive) to do something like
the patch below, which basically just hard-codes (base, size)
rather than (base, end).

> If this is not 
> acceptable, then we can fall back and use the Nanhai/Bjorn fix
> of using ioremap_nocache().

Regardless of how we solve the vga_vram_size issue, I still think
ioremap_nocache() is more appropriate in this case.  A framebuffer
won't work like we expect if it's mapped WB, will it?

(The patch below assumes the ioremap_nocache change precedes it.)


[PATCH] vgacon: make VGA_MAP_MEM take size, remove extra use

VGA_MAP_MEM translates to ioremap() on some architectures.  It
makes sense to do this to vga_vram_base, because we're going to
access memory between vga_vram_base and vga_vram_end.

But it doesn't really make sense to map starting at vga_vram_end,
because we aren't going to access memory starting there.  On ia64,
which always has to be different, ioremapping vga_vram_end gives
you something completely incompatible with ioremapped vga_vram_start,
so vga_vram_size ends up being nonsense.

As a bonus, we often know the size up front, so we can use ioremap()
correctly, rather than giving it a zero size.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Index: work-mm5/drivers/video/console/vgacon.c
=================================--- work-mm5.orig/drivers/video/console/vgacon.c	2006-04-03 15:04:48.000000000 -0600
+++ work-mm5/drivers/video/console/vgacon.c	2006-04-05 15:48:37.000000000 -0600
@@ -391,7 +391,7 @@
 			static struct resource ega_console_resource  			    { .name = "ega", .start = 0x3B0, .end = 0x3BF };
 			vga_video_type = VIDEO_TYPE_EGAM;
-			vga_vram_end = 0xb8000;
+			vga_vram_size = 0x8000;
 			display_desc = "EGA+";
 			request_resource(&ioport_resource,
 					 &ega_console_resource);
@@ -401,7 +401,7 @@
 			static struct resource mda2_console_resource  			    { .name = "mda", .start = 0x3BF, .end = 0x3BF };
 			vga_video_type = VIDEO_TYPE_MDA;
-			vga_vram_end = 0xb2000;
+			vga_vram_size = 0x2000;
 			display_desc = "*MDA";
 			request_resource(&ioport_resource,
 					 &mda1_console_resource);
@@ -418,7 +418,7 @@
 		if ((ORIG_VIDEO_EGA_BX & 0xff) != 0x10) {
 			int i;
 
-			vga_vram_end = 0xc0000;
+			vga_vram_size = 0x8000;
 
 			if (!ORIG_VIDEO_ISVGA) {
 				static struct resource ega_console_resource
@@ -443,7 +443,7 @@
 				 * and COE=1 isn't necessarily a good idea)
 				 */
 				vga_vram_base = 0xa0000;
-				vga_vram_end = 0xb0000;
+				vga_vram_size = 0x10000;
 				outb_p(6, VGA_GFX_I);
 				outb_p(6, VGA_GFX_D);
 #endif
@@ -475,7 +475,7 @@
 			static struct resource cga_console_resource  			    { .name = "cga", .start = 0x3D4, .end = 0x3D5 };
 			vga_video_type = VIDEO_TYPE_CGA;
-			vga_vram_end = 0xba000;
+			vga_vram_size = 0x2000;
 			display_desc = "*CGA";
 			request_resource(&ioport_resource,
 					 &cga_console_resource);
@@ -483,9 +483,8 @@
 		}
 	}
 
-	vga_vram_base = VGA_MAP_MEM(vga_vram_base);
-	vga_vram_end = VGA_MAP_MEM(vga_vram_end);
-	vga_vram_size = vga_vram_end - vga_vram_base;
+	vga_vram_base = VGA_MAP_MEM(vga_vram_base, vga_vram_size);
+	vga_vram_end = vga_vram_base + vga_vram_size;
 
 	/*
 	 *      Find out if there is a graphics card present.
@@ -1020,14 +1019,14 @@
 	char *charmap;
 	
 	if (vga_video_type != VIDEO_TYPE_EGAM) {
-		charmap = (char *) VGA_MAP_MEM(colourmap);
+		charmap = (char *) VGA_MAP_MEM(colourmap, 0);
 		beg = 0x0e;
 #ifdef VGA_CAN_DO_64KB
 		if (vga_video_type = VIDEO_TYPE_VGAC)
 			beg = 0x06;
 #endif
 	} else {
-		charmap = (char *) VGA_MAP_MEM(blackwmap);
+		charmap = (char *) VGA_MAP_MEM(blackwmap, 0);
 		beg = 0x0a;
 	}
 
Index: work-mm5/include/asm-alpha/vga.h
=================================--- work-mm5.orig/include/asm-alpha/vga.h	2006-01-02 20:21:10.000000000 -0700
+++ work-mm5/include/asm-alpha/vga.h	2006-04-05 15:42:35.000000000 -0600
@@ -46,6 +46,6 @@
 #define vga_readb(a)	readb((u8 __iomem *)(a))
 #define vga_writeb(v,a)	writeb(v, (u8 __iomem *)(a))
 
-#define VGA_MAP_MEM(x)	((unsigned long) ioremap(x, 0))
+#define VGA_MAP_MEM(x,s)	((unsigned long) ioremap(x, s))
 
 #endif
Index: work-mm5/include/asm-arm/vga.h
=================================--- work-mm5.orig/include/asm-arm/vga.h	2006-01-02 20:21:10.000000000 -0700
+++ work-mm5/include/asm-arm/vga.h	2006-04-05 15:42:21.000000000 -0600
@@ -4,7 +4,7 @@
 #include <asm/hardware.h>
 #include <asm/io.h>
 
-#define VGA_MAP_MEM(x)	(PCIMEM_BASE + (x))
+#define VGA_MAP_MEM(x,s)	(PCIMEM_BASE + (x))
 
 #define vga_readb(x)	(*((volatile unsigned char *)x))
 #define vga_writeb(x,y)	(*((volatile unsigned char *)y) = (x))
Index: work-mm5/include/asm-i386/vga.h
=================================--- work-mm5.orig/include/asm-i386/vga.h	2006-01-02 20:21:10.000000000 -0700
+++ work-mm5/include/asm-i386/vga.h	2006-04-05 15:42:49.000000000 -0600
@@ -12,7 +12,7 @@
  *	access the videoram directly without any black magic.
  */
 
-#define VGA_MAP_MEM(x) (unsigned long)phys_to_virt(x)
+#define VGA_MAP_MEM(x,s) (unsigned long)phys_to_virt(x)
 
 #define vga_readb(x) (*(x))
 #define vga_writeb(x,y) (*(y) = (x))
Index: work-mm5/include/asm-ia64/vga.h
=================================--- work-mm5.orig/include/asm-ia64/vga.h	2006-04-05 09:57:55.000000000 -0600
+++ work-mm5/include/asm-ia64/vga.h	2006-04-05 15:43:09.000000000 -0600
@@ -17,7 +17,7 @@
 extern unsigned long vga_console_iobase;
 extern unsigned long vga_console_membase;
 
-#define VGA_MAP_MEM(x)	((unsigned long) ioremap_nocache(vga_console_membase + (x), 0))
+#define VGA_MAP_MEM(x,s)	((unsigned long) ioremap_nocache(vga_console_membase + (x), s))
 
 #define vga_readb(x)	(*(x))
 #define vga_writeb(x,y)	(*(y) = (x))
Index: work-mm5/include/asm-m32r/vga.h
=================================--- work-mm5.orig/include/asm-m32r/vga.h	2006-01-02 20:21:10.000000000 -0700
+++ work-mm5/include/asm-m32r/vga.h	2006-04-05 15:43:22.000000000 -0600
@@ -14,7 +14,7 @@
  *	access the videoram directly without any black magic.
  */
 
-#define VGA_MAP_MEM(x) (unsigned long)phys_to_virt(x)
+#define VGA_MAP_MEM(x,s) (unsigned long)phys_to_virt(x)
 
 #define vga_readb(x) (*(x))
 #define vga_writeb(x,y) (*(y) = (x))
Index: work-mm5/include/asm-mips/vga.h
=================================--- work-mm5.orig/include/asm-mips/vga.h	2006-03-23 10:22:17.000000000 -0700
+++ work-mm5/include/asm-mips/vga.h	2006-04-05 15:43:32.000000000 -0600
@@ -13,7 +13,7 @@
  *	access the videoram directly without any black magic.
  */
 
-#define VGA_MAP_MEM(x)	(0xb0000000L + (unsigned long)(x))
+#define VGA_MAP_MEM(x,s)	(0xb0000000L + (unsigned long)(x))
 
 #define vga_readb(x)	(*(x))
 #define vga_writeb(x,y)	(*(y) = (x))
Index: work-mm5/include/asm-powerpc/vga.h
=================================--- work-mm5.orig/include/asm-powerpc/vga.h	2006-01-02 20:21:10.000000000 -0700
+++ work-mm5/include/asm-powerpc/vga.h	2006-04-05 15:43:57.000000000 -0600
@@ -42,9 +42,9 @@
 extern unsigned long vgacon_remap_base;
 
 #ifdef __powerpc64__
-#define VGA_MAP_MEM(x) ((unsigned long) ioremap((x), 0))
+#define VGA_MAP_MEM(x,s) ((unsigned long) ioremap((x), s))
 #else
-#define VGA_MAP_MEM(x) (x + vgacon_remap_base)
+#define VGA_MAP_MEM(x,s) (x + vgacon_remap_base)
 #endif
 
 #define vga_readb(x) (*(x))
Index: work-mm5/include/asm-sparc64/vga.h
=================================--- work-mm5.orig/include/asm-sparc64/vga.h	2006-01-02 20:21:10.000000000 -0700
+++ work-mm5/include/asm-sparc64/vga.h	2006-04-05 15:44:08.000000000 -0600
@@ -28,6 +28,6 @@
 	return *addr;
 }
 
-#define VGA_MAP_MEM(x) (x)
+#define VGA_MAP_MEM(x,s) (x)
 
 #endif
Index: work-mm5/include/asm-x86_64/vga.h
=================================--- work-mm5.orig/include/asm-x86_64/vga.h	2006-01-02 20:21:10.000000000 -0700
+++ work-mm5/include/asm-x86_64/vga.h	2006-04-05 15:44:18.000000000 -0600
@@ -12,7 +12,7 @@
  *	access the videoram directly without any black magic.
  */
 
-#define VGA_MAP_MEM(x) (unsigned long)phys_to_virt(x)
+#define VGA_MAP_MEM(x,s) (unsigned long)phys_to_virt(x)
 
 #define vga_readb(x) (*(x))
 #define vga_writeb(x,y) (*(y) = (x))
Index: work-mm5/include/asm-xtensa/vga.h
=================================--- work-mm5.orig/include/asm-xtensa/vga.h	2006-01-02 20:21:10.000000000 -0700
+++ work-mm5/include/asm-xtensa/vga.h	2006-04-05 15:44:31.000000000 -0600
@@ -11,7 +11,7 @@
 #ifndef _XTENSA_VGA_H
 #define _XTENSA_VGA_H
 
-#define VGA_MAP_MEM(x) (unsigned long)phys_to_virt(x)
+#define VGA_MAP_MEM(x,s) (unsigned long)phys_to_virt(x)
 
 #define vga_readb(x)	(*(x))
 #define vga_writeb(x,y)	(*(y) = (x))
Index: work-mm5/drivers/video/console/mdacon.c
=================================--- work-mm5.orig/drivers/video/console/mdacon.c	2006-01-02 20:21:10.000000000 -0700
+++ work-mm5/drivers/video/console/mdacon.c	2006-04-05 15:46:33.000000000 -0600
@@ -313,8 +313,8 @@
 	mda_num_columns = 80;
 	mda_num_lines   = 25;
 
-	mda_vram_base = VGA_MAP_MEM(0xb0000);
 	mda_vram_len  = 0x01000;
+	mda_vram_base = VGA_MAP_MEM(0xb0000, mda_vram_len);
 
 	mda_index_port  = 0x3b4;
 	mda_value_port  = 0x3b5;
Index: work-mm5/drivers/video/vga16fb.c
=================================--- work-mm5.orig/drivers/video/vga16fb.c	2006-03-23 10:22:16.000000000 -0700
+++ work-mm5/drivers/video/vga16fb.c	2006-04-05 15:49:34.000000000 -0600
@@ -1351,7 +1351,7 @@
 	}
 
 	/* XXX share VGA_FB_PHYS and I/O region with vgacon and others */
-	info->screen_base = (void __iomem *)VGA_MAP_MEM(VGA_FB_PHYS);
+	info->screen_base = (void __iomem *)VGA_MAP_MEM(VGA_FB_PHYS, 0);
 
 	if (!info->screen_base) {
 		printk(KERN_ERR "vga16fb: unable to map device\n");

Re: 2.6.17-rc1-mm1

[Antonino A. Daplas]

Bjorn Helgaas wrote:
> On Wednesday 05 April 2006 15:17, Luck, Tony wrote:
>> On Wed, Apr 05, 2006 at 10:15:34AM -0600, Bjorn Helgaas wrote:

> [PATCH] vgacon: make VGA_MAP_MEM take size, remove extra use
> 
> VGA_MAP_MEM translates to ioremap() on some architectures.  It
> makes sense to do this to vga_vram_base, because we're going to
> access memory between vga_vram_base and vga_vram_end.
> 
> But it doesn't really make sense to map starting at vga_vram_end,
> because we aren't going to access memory starting there.  On ia64,
> which always has to be different, ioremapping vga_vram_end gives
> you something completely incompatible with ioremapped vga_vram_start,
> so vga_vram_size ends up being nonsense.
> 
> As a bonus, we often know the size up front, so we can use ioremap()
> correctly, rather than giving it a zero size.

I definitely prefer this patch.

> 
> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
Acked-by: Antonino Daplas <adaplas@pol.net>

Tony

Re: 2.6.17-rc1-mm1

[Russell King]

On Wed, Apr 05, 2006 at 04:01:08PM -0600, Bjorn Helgaas wrote:
> [PATCH] vgacon: make VGA_MAP_MEM take size, remove extra use

Ah, seems to be what I just suggested...

> @@ -1020,14 +1019,14 @@
>  	char *charmap;
>  	
>  	if (vga_video_type != VIDEO_TYPE_EGAM) {
> -		charmap = (char *) VGA_MAP_MEM(colourmap);
> +		charmap = (char *) VGA_MAP_MEM(colourmap, 0);

Don't like this though - can't we pass a real size here rather than zero?
There seems to be several clues as to the maximum size:

#define cmapsz 8192

        if (!vga_font_is_default)
                charmap += 4 * cmapsz;

                        charmap += 2 * cmapsz;
                                for (i = 0; i < cmapsz; i++)
                                        vga_writeb(arg[i], charmap + i);

so that's about 7 * cmapsz - call that 8 for completeness, which is 64K.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: 2.6.17-rc1-mm1

[Russell King]

On Thu, Apr 06, 2006 at 11:21:54AM +0100, Russell King wrote:
> On Wed, Apr 05, 2006 at 04:01:08PM -0600, Bjorn Helgaas wrote:
> > [PATCH] vgacon: make VGA_MAP_MEM take size, remove extra use
> 
> Ah, seems to be what I just suggested...
> 
> > @@ -1020,14 +1019,14 @@
> >  	char *charmap;
> >  	
> >  	if (vga_video_type != VIDEO_TYPE_EGAM) {
> > -		charmap = (char *) VGA_MAP_MEM(colourmap);
> > +		charmap = (char *) VGA_MAP_MEM(colourmap, 0);
> 
> Don't like this though - can't we pass a real size here rather than zero?
> There seems to be several clues as to the maximum size:
> 
> #define cmapsz 8192
> 
>         if (!vga_font_is_default)
>                 charmap += 4 * cmapsz;
> 
>                         charmap += 2 * cmapsz;
>                                 for (i = 0; i < cmapsz; i++)
>                                         vga_writeb(arg[i], charmap + i);
> 
> so that's about 7 * cmapsz - call that 8 for completeness, which is 64K.

Oh, and obviously, can we also have a VGA_UNMAP_MEM() macro as well please?
8)  IOW, something like this (which I cobbled together from your patch, some
of it by hand-edits - couldn't get it to apply cleanly to current -git.)

diff --git a/drivers/video/console/vgacon.c b/drivers/video/console/vgacon.c
--- a/drivers/video/console/vgacon.c
+++ b/drivers/video/console/vgacon.c
@@ -391,7 +391,7 @@ static const char __init *vgacon_startup
 			static struct resource ega_console_resource  			    { .name = "ega", .start = 0x3B0, .end = 0x3BF };
 			vga_video_type = VIDEO_TYPE_EGAM;
-			vga_vram_end = 0xb8000;
+			vga_vram_size = 0x8000;
 			display_desc = "EGA+";
 			request_resource(&ioport_resource,
 					 &ega_console_resource);
@@ -401,7 +401,7 @@ static const char __init *vgacon_startup
 			static struct resource mda2_console_resource  			    { .name = "mda", .start = 0x3BF, .end = 0x3BF };
 			vga_video_type = VIDEO_TYPE_MDA;
-			vga_vram_end = 0xb2000;
+			vga_vram_size = 0x2000;
 			display_desc = "*MDA";
 			request_resource(&ioport_resource,
 					 &mda1_console_resource);
@@ -418,7 +418,7 @@ static const char __init *vgacon_startup
 		if ((ORIG_VIDEO_EGA_BX & 0xff) != 0x10) {
 			int i;
 
-			vga_vram_end = 0xc0000;
+			vga_vram_size = 0x8000;
 
 			if (!ORIG_VIDEO_ISVGA) {
 				static struct resource ega_console_resource
@@ -443,7 +443,7 @@ static const char __init *vgacon_startup
 				 * and COE=1 isn't necessarily a good idea)
 				 */
 				vga_vram_base = 0xa0000;
-				vga_vram_end = 0xb0000;
+				vga_vram_size = 0x10000;
 				outb_p(6, VGA_GFX_I);
 				outb_p(6, VGA_GFX_D);
 #endif
@@ -475,7 +475,7 @@ static const char __init *vgacon_startup
 			static struct resource cga_console_resource  			    { .name = "cga", .start = 0x3D4, .end = 0x3D5 };
 			vga_video_type = VIDEO_TYPE_CGA;
-			vga_vram_end = 0xba000;
+			vga_vram_size = 0x2000;
 			display_desc = "*CGA";
 			request_resource(&ioport_resource,
 					 &cga_console_resource);
@@ -483,9 +483,8 @@ static const char __init *vgacon_startup
 		}
 	}
 
-	vga_vram_base = VGA_MAP_MEM(vga_vram_base);
-	vga_vram_end = VGA_MAP_MEM(vga_vram_end);
-	vga_vram_size = vga_vram_end - vga_vram_base;
+	vga_vram_base = VGA_MAP_MEM(vga_vram_base, vga_vram_size);
+	vga_vram_end = vga_vram_base + vga_vram_size;
 
 	/*
 	 *      Find out if there is a graphics card present.
@@ -1020,14 +1019,14 @@ static int vgacon_do_font_op(struct vgas
 	char *charmap;
 	
 	if (vga_video_type != VIDEO_TYPE_EGAM) {
-		charmap = (char *) VGA_MAP_MEM(colourmap);
+		charmap = (char *) VGA_MAP_MEM(colourmap, 8 * cmapsz);
 		beg = 0x0e;
 #ifdef VGA_CAN_DO_64KB
 		if (vga_video_type = VIDEO_TYPE_VGAC)
 			beg = 0x06;
 #endif
 	} else {
-		charmap = (char *) VGA_MAP_MEM(blackwmap);
+		charmap = (char *) VGA_MAP_MEM(blackwmap, 8 * cmapsz);
 		beg = 0x0a;
 	}
 
@@ -1102,6 +1101,8 @@ static int vgacon_do_font_op(struct vgas
 		}
 	}
 
+	VGA_UNMAP_MEM(charmap, 8 * cmapsz);
+
 	spin_lock_irq(&vga_lock);
 	/* First, the sequencer, Synchronous reset */
 	vga_wseq(state->vgabase, VGA_SEQ_RESET, 0x01);	
Index: work-mm5/include/asm-alpha/vga.h
=================================--- work-mm5.orig/include/asm-alpha/vga.h	2006-01-02 20:21:10.000000000 -0700
+++ work-mm5/include/asm-alpha/vga.h	2006-04-05 15:42:35.000000000 -0600
@@ -46,6 +46,7 @@
 #define vga_readb(a)	readb((u8 __iomem *)(a))
 #define vga_writeb(v,a)	writeb(v, (u8 __iomem *)(a))
 
-#define VGA_MAP_MEM(x)	((unsigned long) ioremap(x, 0))
+#define VGA_MAP_MEM(x,s)	((unsigned long) ioremap(x, s))
+#define VGA_UNMAP_MEM(x,s)	do { } while (0)
 
 #endif
Index: work-mm5/include/asm-arm/vga.h
=================================--- work-mm5.orig/include/asm-arm/vga.h	2006-01-02 20:21:10.000000000 -0700
+++ work-mm5/include/asm-arm/vga.h	2006-04-05 15:42:21.000000000 -0600
@@ -4,7 +4,8 @@
 #include <asm/hardware.h>
 #include <asm/io.h>
 
-#define VGA_MAP_MEM(x)	(PCIMEM_BASE + (x))
+#define VGA_MAP_MEM(x,s)	(PCIMEM_BASE + (x))
+#define VGA_UNMAP_MEM(x,s)	do { } while (0)
 
 #define vga_readb(x)	(*((volatile unsigned char *)x))
 #define vga_writeb(x,y)	(*((volatile unsigned char *)y) = (x))
Index: work-mm5/include/asm-i386/vga.h
=================================--- work-mm5.orig/include/asm-i386/vga.h	2006-01-02 20:21:10.000000000 -0700
+++ work-mm5/include/asm-i386/vga.h	2006-04-05 15:42:49.000000000 -0600
@@ -12,7 +12,8 @@
  *	access the videoram directly without any black magic.
  */
 
-#define VGA_MAP_MEM(x) (unsigned long)phys_to_virt(x)
+#define VGA_MAP_MEM(x,s) (unsigned long)phys_to_virt(x)
+#define VGA_UNMAP_MEM(x,s)	do { } while (0)
 
 #define vga_readb(x) (*(x))
 #define vga_writeb(x,y) (*(y) = (x))
Index: work-mm5/include/asm-ia64/vga.h
=================================--- work-mm5.orig/include/asm-ia64/vga.h	2006-04-05 09:57:55.000000000 -0600
+++ work-mm5/include/asm-ia64/vga.h	2006-04-05 15:43:09.000000000 -0600
@@ -17,7 +17,8 @@
 extern unsigned long vga_console_iobase;
 extern unsigned long vga_console_membase;
 
-#define VGA_MAP_MEM(x)	((unsigned long) ioremap_nocache(vga_console_membase + (x), 0))
+#define VGA_MAP_MEM(x,s)	((unsigned long) ioremap_nocache(vga_console_membase + (x), s))
+#define VGA_UNMAP_MEM(x,s)	do { } while (0)
 
 #define vga_readb(x)	(*(x))
 #define vga_writeb(x,y)	(*(y) = (x))
Index: work-mm5/include/asm-m32r/vga.h
=================================--- work-mm5.orig/include/asm-m32r/vga.h	2006-01-02 20:21:10.000000000 -0700
+++ work-mm5/include/asm-m32r/vga.h	2006-04-05 15:43:22.000000000 -0600
@@ -14,7 +14,8 @@
  *	access the videoram directly without any black magic.
  */
 
-#define VGA_MAP_MEM(x) (unsigned long)phys_to_virt(x)
+#define VGA_MAP_MEM(x,s) (unsigned long)phys_to_virt(x)
+#define VGA_UNMAP_MEM(x,s)	do { } while (0)
 
 #define vga_readb(x) (*(x))
 #define vga_writeb(x,y) (*(y) = (x))
Index: work-mm5/include/asm-mips/vga.h
=================================--- work-mm5.orig/include/asm-mips/vga.h	2006-03-23 10:22:17.000000000 -0700
+++ work-mm5/include/asm-mips/vga.h	2006-04-05 15:43:32.000000000 -0600
@@ -13,7 +13,8 @@
  *	access the videoram directly without any black magic.
  */
 
-#define VGA_MAP_MEM(x)	(0xb0000000L + (unsigned long)(x))
+#define VGA_MAP_MEM(x,s)	(0xb0000000L + (unsigned long)(x))
+#define VGA_UNMAP_MEM(x,s)	do { } while (0)
 
 #define vga_readb(x)	(*(x))
 #define vga_writeb(x,y)	(*(y) = (x))
Index: work-mm5/include/asm-powerpc/vga.h
=================================--- work-mm5.orig/include/asm-powerpc/vga.h	2006-01-02 20:21:10.000000000 -0700
+++ work-mm5/include/asm-powerpc/vga.h	2006-04-05 15:43:57.000000000 -0600
@@ -42,9 +42,11 @@
 extern unsigned long vgacon_remap_base;
 
 #ifdef __powerpc64__
-#define VGA_MAP_MEM(x) ((unsigned long) ioremap((x), 0))
+#define VGA_MAP_MEM(x,s) ((unsigned long) ioremap((x), s))
+#define VGA_UNMAP_MEM(x,s)	do { } while (0)
 #else
-#define VGA_MAP_MEM(x) (x + vgacon_remap_base)
+#define VGA_MAP_MEM(x,s) (x + vgacon_remap_base)
+#define VGA_UNMAP_MEM(x,s)	do { } while (0)
 #endif
 
 #define vga_readb(x) (*(x))
Index: work-mm5/include/asm-sparc64/vga.h
=================================--- work-mm5.orig/include/asm-sparc64/vga.h	2006-01-02 20:21:10.000000000 -0700
+++ work-mm5/include/asm-sparc64/vga.h	2006-04-05 15:44:08.000000000 -0600
@@ -28,6 +28,7 @@
 	return *addr;
 }
 
-#define VGA_MAP_MEM(x) (x)
+#define VGA_MAP_MEM(x,s) (x)
+#define VGA_UNMAP_MEM(x,s)	do { } while (0)
 
 #endif
Index: work-mm5/include/asm-x86_64/vga.h
=================================--- work-mm5.orig/include/asm-x86_64/vga.h	2006-01-02 20:21:10.000000000 -0700
+++ work-mm5/include/asm-x86_64/vga.h	2006-04-05 15:44:18.000000000 -0600
@@ -12,7 +12,8 @@
  *	access the videoram directly without any black magic.
  */
 
-#define VGA_MAP_MEM(x) (unsigned long)phys_to_virt(x)
+#define VGA_MAP_MEM(x,s) (unsigned long)phys_to_virt(x)
+#define VGA_UNMAP_MEM(x,s)	do { } while (0)
 
 #define vga_readb(x) (*(x))
 #define vga_writeb(x,y) (*(y) = (x))
Index: work-mm5/include/asm-xtensa/vga.h
=================================--- work-mm5.orig/include/asm-xtensa/vga.h	2006-01-02 20:21:10.000000000 -0700
+++ work-mm5/include/asm-xtensa/vga.h	2006-04-05 15:44:31.000000000 -0600
@@ -11,7 +11,8 @@
 #ifndef _XTENSA_VGA_H
 #define _XTENSA_VGA_H
 
-#define VGA_MAP_MEM(x) (unsigned long)phys_to_virt(x)
+#define VGA_MAP_MEM(x,s) (unsigned long)phys_to_virt(x)
+#define VGA_UNMAP_MEM(x,s)	do { } while (0)
 
 #define vga_readb(x)	(*(x))
 #define vga_writeb(x,y)	(*(y) = (x))
Index: work-mm5/drivers/video/console/mdacon.c
=================================--- work-mm5.orig/drivers/video/console/mdacon.c	2006-01-02 20:21:10.000000000 -0700
+++ work-mm5/drivers/video/console/mdacon.c	2006-04-05 15:46:33.000000000 -0600
@@ -313,8 +313,8 @@
 	mda_num_columns = 80;
 	mda_num_lines   = 25;
 
-	mda_vram_base = VGA_MAP_MEM(0xb0000);
 	mda_vram_len  = 0x01000;
+	mda_vram_base = VGA_MAP_MEM(0xb0000, mda_vram_len);
 
 	mda_index_port  = 0x3b4;
 	mda_value_port  = 0x3b5;
Index: work-mm5/drivers/video/vga16fb.c
=================================--- work-mm5.orig/drivers/video/vga16fb.c	2006-03-23 10:22:16.000000000 -0700
+++ work-mm5/drivers/video/vga16fb.c	2006-04-05 15:49:34.000000000 -0600
@@ -1351,7 +1351,7 @@
 	}
 
 	/* XXX share VGA_FB_PHYS and I/O region with vgacon and others */
-	info->screen_base = (void __iomem *)VGA_MAP_MEM(VGA_FB_PHYS);
+	info->screen_base = (void __iomem *)VGA_MAP_MEM(VGA_FB_PHYS, VGA_FB_PHYS_LEN);
 
 	if (!info->screen_base) {
 		printk(KERN_ERR "vga16fb: unable to map device\n");

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: 2.6.17-rc1-mm1

[Bjorn Helgaas]

On Thursday 06 April 2006 04:34, Russell King wrote:
> On Thu, Apr 06, 2006 at 11:21:54AM +0100, Russell King wrote:
> > On Wed, Apr 05, 2006 at 04:01:08PM -0600, Bjorn Helgaas wrote:
> > > [PATCH] vgacon: make VGA_MAP_MEM take size, remove extra use
> > 
> > Ah, seems to be what I just suggested...
> > 
> > > @@ -1020,14 +1019,14 @@
> > >  	char *charmap;
> > >  	
> > >  	if (vga_video_type != VIDEO_TYPE_EGAM) {
> > > -		charmap = (char *) VGA_MAP_MEM(colourmap);
> > > +		charmap = (char *) VGA_MAP_MEM(colourmap, 0);
> > 
> > Don't like this though - can't we pass a real size here rather than zero?
> > There seems to be several clues as to the maximum size:

I didn't like it either, but was too lazy to work out the actual size,
so I just preserved the previous behavior.

Andrew's put my first patch in -mm already, so I'll put this size
issue and your unmap suggestion on my to-do list.

Re: 2.6.17-rc1-mm1

[Russell King]

On Wed, Apr 05, 2006 at 02:17:57PM -0700, Luck, Tony wrote:
> On Wed, Apr 05, 2006 at 10:15:34AM -0600, Bjorn Helgaas wrote:
> > Huh.  Intel firmware used to just not mention the VGA framebuffer
> > (0xa0000-0xc0000) at all in the EFI memory map.  I think that was
> > clearly a bug.  So maybe they fixed that by marking it WB (and
> > hopefully UC as well).
> 
> Nope ... not fixed (at least not in the f/w that I'm running). The
> VGA buffer is still simply not mentioned in the EFI memory map.
> 
> The problem looks to come from this code in vgacon.c:
> 
> 	vga_vram_base = VGA_MAP_MEM(vga_vram_base);
> 	vga_vram_end = VGA_MAP_MEM(vga_vram_end);
> 	vga_vram_size = vga_vram_end - vga_vram_base;

Wouldn't it be better to do:

	vga_vram_size = vga_vram_end - vga_vram_base;
	vga_vram_base = VGA_IOREMAP(vga_vram_base, vga_vram_size);
	vga_vram_end = vga_vram_base + vga_vram_size;

and for compatibility:

#define VGA_IOREMAP(base,size)	VGA_MAP_MEM(base)

?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

RE: 2.6.17-rc1-mm1

[Luck, Tony]

> I think the VGA_MAP_MEM(vga_vram_end) is just bogus -- it's not
> that we need to map the memory starting at vga_vram_end.  I think
> it would cleaner (though much more intrusive) to do something like
> the patch below, which basically just hard-codes (base, size)
> rather than (base, end).

This patch works for me on ia64/Tiger

-Tony