VGA console endian bug

VGA console endian bug

[Hollis Blanchard]

These two patches came from Daniel Berlin on July 9. They completely resolve
the VGA console backwards-endian problem for me on PPC. If they have Geert's
seal of approval ;) can they be committed?

-Hollis

--- linuxppc_2_4_devel/include/asm-ppc/vga.h.old	Thu Aug  9 11:49:23 2001
+++ linuxppc_2_4_devel/include/asm-ppc/vga.h	Thu Aug  9 11:49:42 2001
@@ -37,6 +37,9 @@

 #define VT_BUF_HAVE_MEMCPYW
 #define scr_memcpyw	memcpy
+#define VT_BUF_HAVE_MEMCPYF
+#define scr_memcpyw_to memcpy
+#define scr_memcpyw_from memcpy

 #endif /* !CONFIG_VGA_CONSOLE && !CONFIG_MDA_CONSOLE */

--- linuxppc_2_4_devel/drivers/video/fbcon.c.old	Thu Aug  9 11:45:10 2001
+++ linuxppc_2_4_devel/drivers/video/fbcon.c	Thu Aug  9 11:46:23 2001
@@ -2028,13 +2028,13 @@
 	if (!conp->vc_can_do_color)
 	    *p++ ^= 0x0800;
 	else if (conp->vc_hi_font_mask == 0x100) {
-	    u16 a = *p;
+	    u16 a = scr_read(p);
 	    a = ((a) & 0x11ff) | (((a) & 0xe000) >> 4) | (((a) & 0x0e00) << 4);
-	    *p++ = a;
+	    scr_write(a, p++);
 	} else {
-	    u16 a = *p;
+	    u16 a = scr_read(p);
 	    a = ((a) & 0x88ff) | (((a) & 0x7000) >> 4) | (((a) & 0x0700) << 4);
-	    *p++ = a;
+	    scr_write(a, p++);
 	}
 	if (p == (u16 *)softback_end)
 	    p = (u16 *)softback_buf;

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: VGA console endian bug

[Geert Uytterhoeven]

On Thu, 9 Aug 2001, Hollis Blanchard wrote:
> These two patches came from Daniel Berlin on July 9. They completely resolve
> the VGA console backwards-endian problem for me on PPC. If they have Geert's
> seal of approval ;) can they be committed?
>
> -Hollis
>
> --- linuxppc_2_4_devel/include/asm-ppc/vga.h.old	Thu Aug  9 11:49:23 2001
> +++ linuxppc_2_4_devel/include/asm-ppc/vga.h	Thu Aug  9 11:49:42 2001
> @@ -37,6 +37,9 @@
>
>  #define VT_BUF_HAVE_MEMCPYW
>  #define scr_memcpyw	memcpy
> +#define VT_BUF_HAVE_MEMCPYF
> +#define scr_memcpyw_to memcpy
> +#define scr_memcpyw_from memcpy
>
>  #endif /* !CONFIG_VGA_CONSOLE && !CONFIG_MDA_CONSOLE */

The first I cannot approve: if scr_{write,read}w() swap bytes, how can it work
if scr_memcpyw_{to,from}() don't swap bytes?

> --- linuxppc_2_4_devel/drivers/video/fbcon.c.old	Thu Aug  9 11:45:10 2001
> +++ linuxppc_2_4_devel/drivers/video/fbcon.c	Thu Aug  9 11:46:23 2001
> @@ -2028,13 +2028,13 @@
>  	if (!conp->vc_can_do_color)
>  	    *p++ ^= 0x0800;
>  	else if (conp->vc_hi_font_mask == 0x100) {
> -	    u16 a = *p;
> +	    u16 a = scr_read(p);
>  	    a = ((a) & 0x11ff) | (((a) & 0xe000) >> 4) | (((a) & 0x0e00) << 4);
> -	    *p++ = a;
> +	    scr_write(a, p++);
>  	} else {
> -	    u16 a = *p;
> +	    u16 a = scr_read(p);
>  	    a = ((a) & 0x88ff) | (((a) & 0x7000) >> 4) | (((a) & 0x0700) << 4);
> -	    *p++ = a;
> +	    scr_write(a, p++);
>  	}
>  	if (p == (u16 *)softback_end)
>  	    p = (u16 *)softback_buf;

Approved :-)

And where's the 3rd patch? I once posted a patch to fix a similar bug in
drivers/char/console.c.

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


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: VGA console endian bug

[Hollis Blanchard]

On Fri, Aug 10, 2001 at 10:41:39AM +0200, Geert Uytterhoeven wrote:
>
> On Thu, 9 Aug 2001, Hollis Blanchard wrote:
> > These two patches came from Daniel Berlin on July 9. They completely resolve
> > the VGA console backwards-endian problem for me on PPC. If they have Geert's
> > seal of approval ;) can they be committed?
> >
> > -Hollis
> >
> > --- linuxppc_2_4_devel/include/asm-ppc/vga.h.old	Thu Aug  9 11:49:23 2001
> > +++ linuxppc_2_4_devel/include/asm-ppc/vga.h	Thu Aug  9 11:49:42 2001
> > @@ -37,6 +37,9 @@
> >
> >  #define VT_BUF_HAVE_MEMCPYW
> >  #define scr_memcpyw	memcpy
> > +#define VT_BUF_HAVE_MEMCPYF
> > +#define scr_memcpyw_to memcpy
> > +#define scr_memcpyw_from memcpy
> >
> >  #endif /* !CONFIG_VGA_CONSOLE && !CONFIG_MDA_CONSOLE */
>
> The first I cannot approve: if scr_{write,read}w() swap bytes, how can it work
> if scr_memcpyw_{to,from}() don't swap bytes?

I have no idea. Perhaps those functions weren't used, and that's why it
appeared to work. I have not tried with only the second patch; perhaps that
will be enough...

> And where's the 3rd patch? I once posted a patch to fix a similar bug in
> drivers/char/console.c.

I can't find it in the linuxppc-dev archives. :(

-Hollis

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: VGA console endian bug

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1726 bytes --]

On Fri, 10 Aug 2001, Hollis Blanchard wrote:
> On Fri, Aug 10, 2001 at 10:41:39AM +0200, Geert Uytterhoeven wrote:
> > On Thu, 9 Aug 2001, Hollis Blanchard wrote:
> > > These two patches came from Daniel Berlin on July 9. They completely resolve
> > > the VGA console backwards-endian problem for me on PPC. If they have Geert's
> > > seal of approval ;) can they be committed?
> > >
> > > -Hollis
> > >
> > > --- linuxppc_2_4_devel/include/asm-ppc/vga.h.old	Thu Aug  9 11:49:23 2001
> > > +++ linuxppc_2_4_devel/include/asm-ppc/vga.h	Thu Aug  9 11:49:42 2001
> > > @@ -37,6 +37,9 @@
> > >
> > >  #define VT_BUF_HAVE_MEMCPYW
> > >  #define scr_memcpyw	memcpy
> > > +#define VT_BUF_HAVE_MEMCPYF
> > > +#define scr_memcpyw_to memcpy
> > > +#define scr_memcpyw_from memcpy
> > >
> > >  #endif /* !CONFIG_VGA_CONSOLE && !CONFIG_MDA_CONSOLE */
> >
> > The first I cannot approve: if scr_{write,read}w() swap bytes, how can it work
> > if scr_memcpyw_{to,from}() don't swap bytes?
>
> I have no idea. Perhaps those functions weren't used, and that's why it
> appeared to work. I have not tried with only the second patch; perhaps that
> will be enough...
>
> > And where's the 3rd patch? I once posted a patch to fix a similar bug in
> > drivers/char/console.c.
>
> I can't find it in the linuxppc-dev archives. :(

It indeed wasn't sent to that list. I attached the mail I sent to Ben H, which
includes the mail I sent before.

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

[-- Attachment #2: Type: TEXT/PLAIN, Size: 6670 bytes --]

From geert@linux-m68k.org Sat Aug 11 10:31:13 2001
Date: Mon, 9 Jul 2001 18:45:14 +0200 (CEST)
From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Subject: Re: Fwd: Re: gpm selection vs VGA console & fbcon

	Hi Ben,

> Geert, does that patch sounds the right thing to you ? If yes,
> I'll push it all over the bk's and to Linus, I'm tired of 
> getting this bug reported ;)

> *** vga.h	Wed Jul  4 10:58:40 2001
> --- /root/linux/include/asm-ppc/vga.h	Thu Jun 28 14:22:02 2001
> *************** extern inline u16 scr_readw(volatile con
> *** 37,42 ****
> --- 37,45 ----
> 
>   #define VT_BUF_HAVE_MEMCPYW
>   #define scr_memcpyw	memcpy
> + #define VT_BUF_HAVE_MEMCPYF
> + #define scr_memcpyw_to memcpy
> + #define scr_memcpyw_from memcpy
> 
>   #endif /* !CONFIG_VGA_CONSOLE && !CONFIG_MDA_CONSOLE */

I fail to see how the first part can solve a problem: scr_memcpyw_{to,from}
copy between the VGA text buffer on the graphics card (little endian) and
native endianness, so setting them to memcpy() cannot be right. I suspect
there's another bug somewhere else...

Note that I've been having a fight with Olaf Hering about this as well :-)

BTW, the comment after the #endif is wrong, it should be

    #endif /* CONFIG_VGA_CONSOLE || CONFIG_MDA_CONSOLE */

instead of

    #endif /* !CONFIG_VGA_CONSOLE && !CONFIG_MDA_CONSOLE */

> *** fbcon.c	Tue Jul  3 17:58:41 2001
> --- /root/linux/drivers/video/fbcon.c	Wed Jun 27 14:52:59 2001
> *************** static void fbcon_invert_region(struct v
> *** 1954,1966 ****
>   	if (!conp->vc_can_do_color)
>   	    *p++ ^= 0x0800;
>   	else if (conp->vc_hi_font_mask == 0x100) {
> ! 	    u16 a = *p;
>   	    a = ((a) & 0x11ff) | (((a) & 0xe000) >> 4) | (((a) & 0x0e00) << 4);
> ! 	    *p++ = a;
>   	} else {
> ! 	    u16 a = *p;
>   	    a = ((a) & 0x88ff) | (((a) & 0x7000) >> 4) | (((a) & 0x0700) << 4);
> ! 	    *p++ = a;
>   	}
>   	if (p == (u16 *)softback_end)
>   	    p = (u16 *)softback_buf;
> --- 1954,1966 ----
>   	if (!conp->vc_can_do_color)
>   	    *p++ ^= 0x0800;
>   	else if (conp->vc_hi_font_mask == 0x100) {
> ! 	    u16 a = scr_readw(p);
>   	    a = ((a) & 0x11ff) | (((a) & 0xe000) >> 4) | (((a) & 0x0e00) << 4);
> ! 	    scr_writew(a, p++);
>   	} else {
> ! 	    u16 a = scr_readw(p);
>   	    a = ((a) & 0x88ff) | (((a) & 0x7000) >> 4) | (((a) & 0x0700) << 4);
> ! 	    scr_writew(a, p++);
>   	}
>   	if (p == (u16 *)softback_end)
>   	    p = (u16 *)softback_buf;

The second part is OK for me.

There's a similar bug in console.c: always use scr_{read,write}w() when
accessing the text/attribute buffer. See the patch (still untested by me) in
the mail below.

--snip--------------------------------------------------------------------------
>From geert@linux-m68k.org Mon Jul  9 18:38:17 2001
Date: Sat, 2 Jun 2001 21:23:32 +0200 (CEST)
From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Daniel Berlin <dan@cgsoftware.com>
Cc: Linux Frame Buffer Device Development
    <linux-fbdev-devel@lists.sourceforge.net>,
     Alan Cox <alan@lxorguk.ukuu.org.uk>, Olaf Hering <olh@suse.de>
Subject: Re: FBCon invert_region is endian unsafe,
     same with generic invert_region

	Hi Daniel,

> I'm not quite sure who the maintainer is, fbcon.c lists 5 people, and
> no one is listed in the MAINTAINERS file (i searched on fbcon).
> 
> This one has been bothering me for a long time.
> It causes text selection to fail on fbcon devices for big endian
> platforms (Because, obviously, it gets big endian data on big endian
> machines, sinceit neglects to use scr_readw/scr_writew. Dumb. You end
> up inverting, pasting each byte reversed without fixing this, so where
> you would normally have pasted/see a background inverted 'u' (0x75),
> you get a 'W' (0x57)).

    [...]

> However, it's important to note that console.c has the same damn bug
> in it's generic inversion routine.
> 
> Changing the code to the right way makes text selection work perfectly
> again, which it hasn't since 2.2.x, at least for me.

So this patch (untested) fixes it?

Olaf: is this the missing piece for you as well?

--- console-2.4.5/drivers/char/console.c.orig	Mon Feb 19 10:36:47 2001
+++ console-2.4.5/drivers/char/console.c	Sat Jun  2 21:15:10 2001
@@ -390,20 +390,28 @@
 	else {
 		u16 *q = p;
 		int cnt = count;
+		u16 a;
 
 		if (!can_do_color) {
-			while (cnt--) *q++ ^= 0x0800;
+			while (cnt--) {
+			    a = scr_readw(q);
+			    a ^= 0x0800;
+			    scr_writew(a, q);
+			    q++;
+			}
 		} else if (hi_font_mask == 0x100) {
 			while (cnt--) {
-				u16 a = *q;
+				a = scr_readw(q);
 				a = ((a) & 0x11ff) | (((a) & 0xe000) >> 4) | (((a) & 0x0e00) << 4);
-				*q++ = a;
+				scr_writew(a, q);
+				q++;
 			}
 		} else {
 			while (cnt--) {
-				u16 a = *q;
+				a = scr_readw(q);
 				a = ((a) & 0x88ff) | (((a) & 0x7000) >> 4) | (((a) & 0x0700) << 4);
-				*q++ = a;
+				scr_writew(a, q);
+				q++
 			}
 		}
 	}
--- console-2.4.5/drivers/video/fbcon.c.orig	Mon Feb 19 10:37:00 2001
+++ console-2.4.5/drivers/video/fbcon.c	Sat Jun  2 21:07:38 2001
@@ -1934,17 +1934,15 @@
 static void fbcon_invert_region(struct vc_data *conp, u16 *p, int cnt)
 {
     while (cnt--) {
+	u16 a = scr_readw(p);
 	if (!conp->vc_can_do_color)
-	    *p++ ^= 0x0800;
-	else if (conp->vc_hi_font_mask == 0x100) {
-	    u16 a = *p;
+	    a ^= 0x0800;
+	else if (conp->vc_hi_font_mask == 0x100)
 	    a = ((a) & 0x11ff) | (((a) & 0xe000) >> 4) | (((a) & 0x0e00) << 4);
-	    *p++ = a;
-	} else {
-	    u16 a = *p;
+	else
 	    a = ((a) & 0x88ff) | (((a) & 0x7000) >> 4) | (((a) & 0x0700) << 4);
-	    *p++ = a;
-	}
+	scr_writew(a, p);
+	p++;
 	if (p == (u16 *)softback_end)
 	    p = (u16 *)softback_buf;
 	if (p == (u16 *)softback_in)

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

--snip--------------------------------------------------------------------------

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: VGA console endian bug

[Olaf Hering]

On Sat, Aug 11, Geert Uytterhoeven wrote:

>
> Note that I've been having a fight with Olaf Hering about this as well :-)

And the battle goes on :)

It really fixes my VGA problems so far, but I guess the bug is somewhere
else.
We use a patch to display the kernel version above the framebuffer logo.
It works on intel and on sparc, but it gives the usual garbage for the
version chars on ppc.

It uses the fbcon_putcs() function. I will see how it works without that
vga patch.


Gruss Olaf

--
 $ man clone

BUGS
       Main feature not yet implemented...

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: VGA console endian bug

[Olaf Hering]

On Sat, Aug 11, Olaf Hering wrote:

>
> On Sat, Aug 11, Geert Uytterhoeven wrote:
>
> >
> > Note that I've been having a fight with Olaf Hering about this as well :-)
>
> And the battle goes on :)
>
> It really fixes my VGA problems so far, but I guess the bug is somewhere
> else.
> We use a patch to display the kernel version above the framebuffer logo.
> It works on intel and on sparc, but it gives the usual garbage for the
> version chars on ppc.
>
> It uses the fbcon_putcs() function. I will see how it works without that
> vga patch.

sigh,

I removed the vga patch and the version string appears still as garbage.
We have the clgen driver in the kernel and it prints its messages also
as garbage, this was always the case, also in 2.2. That does not happen
with the vga patch.

The bootlogo-version patch can be found here:
/mirror/SuSE/ftp.suse.com/pub/suse/ppc/kernel/BETA/patches/50_bootlogo-ver-2.4.5.gz



Gruss Olaf

--
 $ man clone

BUGS
       Main feature not yet implemented...

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: VGA console endian bug

[Geert Uytterhoeven]

On Sat, 11 Aug 2001, Olaf Hering wrote:
> On Sat, Aug 11, Olaf Hering wrote:
> > On Sat, Aug 11, Geert Uytterhoeven wrote:
> > > Note that I've been having a fight with Olaf Hering about this as well :-)
> >
> > And the battle goes on :)
> >
> > It really fixes my VGA problems so far, but I guess the bug is somewhere
> > else.
> > We use a patch to display the kernel version above the framebuffer logo.
> > It works on intel and on sparc, but it gives the usual garbage for the
> > version chars on ppc.
> >
> > It uses the fbcon_putcs() function. I will see how it works without that
> > vga patch.
>
> sigh,
>
> I removed the vga patch and the version string appears still as garbage.
> We have the clgen driver in the kernel and it prints its messages also
> as garbage, this was always the case, also in 2.2. That does not happen
> with the vga patch.
>
> The bootlogo-version patch can be found here:
> /mirror/SuSE/ftp.suse.com/pub/suse/ppc/kernel/BETA/patches/50_bootlogo-ver-2.4.5.gz

+       /* G.S.: Display a line above the Boot Logo to state what
+       * version of the kernel we are booting.
+       */
+       char welcometext[] = "Linux " UTS_RELEASE;
+       unsigned short welcomestring[ sizeof(welcometext) ];
+       int i;
+       for(i = 0; i < sizeof(welcometext); i++)
+           welcomestring[i] = 0x0700 | welcometext[i];
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+       fbcon_putcs( conp, welcomestring, sizeof(welcometext), 0, 0 );

That's not correct, you _must_ use scr_{read,write}w() to access screen
buffers. fbcon_putcs() expects a pointer to a shadow screen buffer.

Please try

    scr_writew(0x0700 | welcometext[i], &welcomestring[i]);

instead.

Your code works on ia32 because ia32 and VGA are both little endian.
It works on SPARC because SPARC and fbdev on SPARC are both big endian.
I guess it fails on a real VGA textconsole on SPARC too (iff SPARC would
support VGA text mode).

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


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: VGA console endian bug

[Olaf Hering]

On Sat, Aug 11, Geert Uytterhoeven wrote:

> +       for(i = 0; i < sizeof(welcometext); i++)
> +           welcomestring[i] = 0x0700 | welcometext[i];
>             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +       fbcon_putcs( conp, welcomestring, sizeof(welcometext), 0, 0 );
>
> That's not correct, you _must_ use scr_{read,write}w() to access screen
> buffers. fbcon_putcs() expects a pointer to a shadow screen buffer.
>
> Please try
>
>     scr_writew(0x0700 | welcometext[i], &welcomestring[i]);
>
> instead.
>
> Your code works on ia32 because ia32 and VGA are both little endian.
> It works on SPARC because SPARC and fbdev on SPARC are both big endian.
> I guess it fails on a real VGA textconsole on SPARC too (iff SPARC would
> support VGA text mode).

Well, that fixes "the last known problem".

But it still leaves the question whats really wrong with the vga console :)


Gruss Olaf

--
 $ man clone

BUGS
       Main feature not yet implemented...

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/