[PATCH] fix endian problem in color_imageblit

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++;
 		}

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

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? */

On Sat, 2002-12-21 at 03:54, James Simmons wrote:

quoted

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

Applied.

quoted

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/