[PATCH] fix endian problem in color_imageblit

[PATCH] fix endian problem in color_imageblit

[Paul Mackerras]

This patch fixes the endian problems in color_imageblit().  With this
patch, I get the penguin drawn properly on boot.

The main change is that on big-endian systems, when we load a pixel
from the source, we now shift it to the left-hand (most significant)
end of the word.  With this change the rest of the logic is correct on
big-endian systems.  This may not be the most efficient way to do
things but it is a simple change that works and avoids disturbing the
rest of the code.

Paul.

diff -urN linux-2.5/drivers/video/cfbimgblt.c pmac-2.5/drivers/video/cfbimgblt.c
--- linux-2.5/drivers/video/cfbimgblt.c	2002-12-10 15:20:32.000000000 +1100
+++ pmac-2.5/drivers/video/cfbimgblt.c	2002-12-12 09:14:47.000000000 +1100
@@ -103,10 +103,10 @@
 {
 	/* Draw the penguin */
 	int i, n;
-	unsigned long bitmask = SHIFT_LOW(~0UL, BITS_PER_LONG - p->var.bits_per_pixel);
+	int bpp = p->var.bits_per_pixel;
 	unsigned long *palette = (unsigned long *) p->pseudo_palette;
 	unsigned long *dst, *dst2, color = 0, val, shift;
-	unsigned long null_bits = BITS_PER_LONG - p->var.bits_per_pixel; 
+	unsigned long null_bits = BITS_PER_LONG - bpp;
 	u8 *src = image->data;
 
 	dst2 = (unsigned long *) dst1;
@@ -125,9 +125,10 @@
 		while (n--) {
 			if (p->fix.visual == FB_VISUAL_TRUECOLOR ||
 			    p->fix.visual == FB_VISUAL_DIRECTCOLOR )
-				color = palette[*src] & bitmask;
+				color = palette[*src];
 			else
-				color = *src & bitmask;	
+				color = *src;
+			color <<= LEFT_POS(bpp);
 			val |= SHIFT_HIGH(color, shift);
 			if (shift >= null_bits) {
 				FB_WRITEL(val, dst++);
@@ -136,7 +137,7 @@
 				else
 					val = SHIFT_LOW(color, BITS_PER_LONG - shift);
 			}
-			shift += p->var.bits_per_pixel;
+			shift += bpp;
 			shift &= (BITS_PER_LONG - 1);
 			src++;
 		}

Re: [Linux-fbdev-devel] [PATCH] fix endian problem in color_imageblit

[Antonino Daplas]

On Thu, 2002-12-12 at 08:41, Paul Mackerras wrote:
> This patch fixes the endian problems in color_imageblit().  With this
> patch, I get the penguin drawn properly on boot.
> 
> The main change is that on big-endian systems, when we load a pixel
> from the source, we now shift it to the left-hand (most significant)
> end of the word.  With this change the rest of the logic is correct on
> big-endian systems.  This may not be the most efficient way to do
> things but it is a simple change that works and avoids disturbing the
> rest of the code.
> 
Nice catch :-)  We also need a similar fix for slow_imageblit(), so
James can you apply the attached patch also:

Also, I noticed that some drivers load the pseudo_palette with entries 
whose length matches the length of the pixel.  The cfb_* functions 
always assume that each pseudo_palette entry is an "unsigned long", so 
bpp16 will segfault, and so will bpp24/32 for 64-bit machines.

Tony

diff -Naur linux-2.5.51/drivers/video/cfbimgblt.c linux/drivers/video/cfbimgblt.c
--- linux-2.5.51/drivers/video/cfbimgblt.c	2002-12-15 00:54:04.000000000 +0000
+++ linux/drivers/video/cfbimgblt.c	2002-12-15 00:54:21.000000000 +0000
@@ -189,6 +189,7 @@
 				color = fgcolor;
 			else 
 				color = bgcolor;
+			color <<= LEFT_POS(bpp);
 			val |= SHIFT_HIGH(color, shift);
 			
 			/* Did the bitshift spill bits to the next long? */

Tony

Re: [Linux-fbdev-devel] [PATCH] fix endian problem in color_imageblit

[James Simmons]

> Nice catch :-)  We also need a similar fix for slow_imageblit(), so
> James can you apply the attached patch also:

Applied.

> Also, I noticed that some drivers load the pseudo_palette with entries
> whose length matches the length of the pixel.  The cfb_* functions
> always assume that each pseudo_palette entry is an "unsigned long", so
> bpp16 will segfault, and so will bpp24/32 for 64-bit machines.

I just noticed that as well. Russell King pointed to it too. I fixed
the unsigned long problem in color_imageblit. All the pseudo_palette
in cfb_* are assumed u32. Is this safe for 16bpp or 8 bpp modes? I will
have test to see. The u16 problem might explain why some people have
trouble with the VESA framebuffer. To all the people having trouble
booting to certain modes for the VESA fraembuffer can you try this patch.
Tell me if it fixes your problems.

--- /usr/src/linus-2.5/drivers/video/cfbimgblt.c	Mon Dec  9 22:23:32 2002
+++ cfbimgblt.c	Fri Dec 20 12:35:25 2002
@@ -102,11 +102,10 @@
 				   unsigned long start_index, unsigned long pitch_index)
 {
 	/* Draw the penguin */
-	int i, n;
-	unsigned long bitmask = SHIFT_LOW(~0UL, BITS_PER_LONG - p->var.bits_per_pixel);
-	unsigned long *palette = (unsigned long *) p->pseudo_palette;
 	unsigned long *dst, *dst2, color = 0, val, shift;
-	unsigned long null_bits = BITS_PER_LONG - p->var.bits_per_pixel;
+	int i, n, bpp = p->var.bits_per_pixel;
+	unsigned long null_bits = BITS_PER_LONG - bpp;
+	u32 *palette = (u32 *) p->pseudo_palette;
 	u8 *src = image->data;

 	dst2 = (unsigned long *) dst1;
@@ -125,9 +124,10 @@
 		while (n--) {
 			if (p->fix.visual == FB_VISUAL_TRUECOLOR ||
 			    p->fix.visual == FB_VISUAL_DIRECTCOLOR )
-				color = palette[*src] & bitmask;
+				color = palette[*src];
 			else
-				color = *src & bitmask;
+				color = *src;
+			color <<= LEFT_POS(bpp);
 			val |= SHIFT_HIGH(color, shift);
 			if (shift >= null_bits) {
 				FB_WRITEL(val, dst++);
@@ -136,7 +136,7 @@
 				else
 					val = SHIFT_LOW(color, BITS_PER_LONG - shift);
 			}
-			shift += p->var.bits_per_pixel;
+			shift += bpp;
 			shift &= (BITS_PER_LONG - 1);
 			src++;
 		}
@@ -188,6 +188,7 @@
 				color = fgcolor;
 			else
 				color = bgcolor;
+			color <<= LEFT_POS(bpp);
 			val |= SHIFT_HIGH(color, shift);

 			/* Did the bitshift spill bits to the next long? */

Re: [PATCH] fix endian problem in color_imageblit

[Antonino Daplas]

On Sat, 2002-12-21 at 03:54, James Simmons wrote:
> 
> > Nice catch :-)  We also need a similar fix for slow_imageblit(), so
> > James can you apply the attached patch also:
> 
> Applied.
> 
> > Also, I noticed that some drivers load the pseudo_palette with entries
> > whose length matches the length of the pixel.  The cfb_* functions
> > always assume that each pseudo_palette entry is an "unsigned long", so
> > bpp16 will segfault, and so will bpp24/32 for 64-bit machines.
> 
> I just noticed that as well. Russell King pointed to it too. I fixed
> the unsigned long problem in color_imageblit. All the pseudo_palette
> in cfb_* are assumed u32. Is this safe for 16bpp or 8 bpp modes? I will

The pseudopalette will only matter for truecolor and directcolor as the
rest of the visuals bypasses the pseudopalette.  Typecasting to
"unsigned long" rather than "u32" is also safer (even for bpp16) since
in 64-bit machines, the drawing functions use fb_writeq instead of
fb_writel.  So, all drivers using the cfb_* functions should change from
"u32" to "unsigned long" _whatever_ the bit depth when loading the
pseudopalette.

Of course, drivers with their own drawing functions can use whatever
they want.

Tony 

PS:  cfb_fillrect is still limited to u32 though which can hopefully be
fixed in the future.




-------------------------------------------------------
This SF.NET email is sponsored by:  The Best Geek Holiday Gifts!
Time is running out!  Thinkgeek.com has the coolest gifts for
your favorite geek.   Let your fingers do the typing.   Visit Now.
T H I N K G E E K . C O M        http://www.thinkgeek.com/sf/

Re: [Linux-fbdev-devel] [PATCH] fix endian problem in color_imageblit

[Russell King]

On Sat, Dec 21, 2002 at 11:45:44AM +0800, Antonino Daplas wrote:
> The pseudopalette will only matter for truecolor and directcolor as the
> rest of the visuals bypasses the pseudopalette.  Typecasting to
> "unsigned long" rather than "u32" is also safer (even for bpp16) since
> in 64-bit machines, the drawing functions use fb_writeq instead of
> fb_writel.

Erm, what does the size of the drawing functions have to do with the
size of the pseudo palette.  The pseudo palette is only there to encode
the pixel values for the 16 console colours.

This means that a 64-bit pseudo palette would only be useful if you have
a graphics card that supports > 32bpp, otherwise a 32-bit pseudo palette
is perfectly adequate, even if you are using fb_writeq.

(note that fbcon doesn't support > 32bpp, so we will only ever look at
the first 32 bits, and having it be 64-bit would just be a needless
waste of space imho.)

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [Linux-fbdev-devel] [PATCH] fix endian problem in color_imageblit

[Geert Uytterhoeven]

On Sat, 21 Dec 2002, Russell King wrote:
> On Sat, Dec 21, 2002 at 11:45:44AM +0800, Antonino Daplas wrote:
> > The pseudopalette will only matter for truecolor and directcolor as the
> > rest of the visuals bypasses the pseudopalette.  Typecasting to
> > "unsigned long" rather than "u32" is also safer (even for bpp16) since
> > in 64-bit machines, the drawing functions use fb_writeq instead of
> > fb_writel.
> 
> Erm, what does the size of the drawing functions have to do with the
> size of the pseudo palette.  The pseudo palette is only there to encode
> the pixel values for the 16 console colours.

Indeed.

> This means that a 64-bit pseudo palette would only be useful if you have
> a graphics card that supports > 32bpp, otherwise a 32-bit pseudo palette
> is perfectly adequate, even if you are using fb_writeq.

Yep. Cards with bpp > 32 are rumoured to exist, but there is no fbdev support
for them right now.

> (note that fbcon doesn't support > 32bpp, so we will only ever look at
> the first 32 bits, and having it be 64-bit would just be a needless
> waste of space imho.)

And this can be fixed, once we have some fbdev driver that uses bpp > 32.

Note that the size of the entries in the pseudo palette used to depend on the
mode, i.e. u16 for bpp = 16, u32 for bpp = 24 or 32.

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: [Linux-fbdev-devel] [PATCH] fix endian problem in color_imageblit

[Russell King]

On Mon, Dec 23, 2002 at 10:07:35AM +0100, Geert Uytterhoeven wrote:
> Note that the size of the entries in the pseudo palette used to depend on the
> mode, i.e. u16 for bpp = 16, u32 for bpp = 24 or 32.

They used to.  However, reading the code in 2.5 today, this appears to be
no longer the case - they're all one size (u32).

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html