PATCH: bugfix für RadeonFB (against 2.4.22-ac4, bug in 2.6.0-test9, too)

PATCH: bugfix für RadeonFB (against 2.4.22-ac4, bug in 2.6.0-test9, too)

[Ronald Lembcke]

Hi!

As linux-fbdev-devel@lists.sourceforge.net didn't like me :(

<<< 550 rejected because 217.81.192.80 is in the DULS RBL
550 5.1.1 <linux-fbdev-devel@lists.sourceforge.net>... User unknown

I send the patches here.


All patches are against 2.4.22-ac4.

The first patch is a bugfix in radeonfb.
The bug is in 2.6.0-test9, too. The second red should be blue.

--- linux-2.4.22-ac4/drivers/video/radeonfb.c	2003-11-05 19:47:16.000000000 +0100
+++ linux-2.4.22-ac4_patched/drivers/video/radeonfb.c	2003-11-05 22:08:39.000000000 +0100
@@ -2362,7 +2362,7 @@
 			disp->visual = FB_VISUAL_DIRECTCOLOR;
 			v.red.offset = 10;
 			v.green.offset = 5;
-			v.red.offset = 0;
+			v.blue.offset = 0;
 			v.red.length = v.green.length = v.blue.length = 5;
 			v.transp.offset = v.transp.length = 0;
 			break;


The following patches make a few framebuffer-drivers behave (imho) more
sensible: (Matrox and Riva allready behave like this)

When selecting a video mode with bits_per_pixel == 16 is selected, but
green.length != 6 (... 0 because the calling programm made no assumtion
if the mode is 5/6/5 or maybe 6/5/5 or whatever) a 15 bpp mode is
selected with 5/5/5. 
That's not nice ... when 16 bpp were requested.

The patches change this behaviour. If not explictly green.length == 5 is
selected, green.length will be set to 6.

The patches for radeonfb, intelfb are straight forward, but I'm not so
sure if the patch for imsttfb is correct.


--- linux-2.4.22-ac4/drivers/video/radeonfb.c	2003-11-05 22:10:09.000000000 +0100
+++ linux-2.4.22-ac4_patched/drivers/video/radeonfb.c	2003-11-05 19:59:04.000000000 +0100
@@ -705,7 +705,7 @@
 {
 	if (var->bits_per_pixel != 16)
 		return var->bits_per_pixel;
-	return (var->green.length == 6) ? 16 : 15;
+	return (var->green.length == 5) ? 15 : 16;
 }
 
 
--- linux-2.4.22-ac4/drivers/video/intel/intelfbdrv.c	2003-11-05 19:46:46.000000000 +0100
+++ linux-2.4.22-ac4_patched/drivers/video/intel/intelfbdrv.c	2003-11-05 20:00:35.000000000 +0100
@@ -444,7 +444,7 @@
 
 	switch (var->bits_per_pixel) {
 	case 16:
-		return (var->green.length == 6) ? 16 : 15;
+		return (var->green.length == 5) ? 15 : 16;
 	case 32:
 		return 24;
 	default:



--- linux-2.4.22-ac4/drivers/video/imsttfb.c	2002-02-25 20:38:07.000000000 +0100
+++ linux-2.4.22-ac4_patched/drivers/video/imsttfb.c	2003-11-05 20:47:47.000000000 +0100
@@ -1348,12 +1348,12 @@
 #endif
 			break;
 		case 16:	/* RGB 555 or 565 */
-			if (disp->var.green.length != 6)
-				disp->var.red.offset = 10;
+			if (disp->var.green.length != 5)
+				disp->var.red.offset = 11;
 			disp->var.red.length = 5;
 			disp->var.green.offset = 5;
-			if (disp->var.green.length != 6)
-				disp->var.green.length = 5;
+			if (disp->var.green.length != 5)
+				disp->var.green.length = 6;
 			disp->var.blue.offset = 0;
 			disp->var.blue.length = 5;
 			disp->var.transp.offset = 0;
@@ -1495,10 +1495,10 @@
 
 	if (con == currcon) {
 		if (oldgreenlen != disp->var.green.length) {
-			if (disp->var.green.length == 6)
-				set_565(p);
-			else
+			if (disp->var.green.length == 5)
 				set_555(p);
+			else
+				set_565(p);
 		}
 		if (oldxres != disp->var.xres || oldyres != disp->var.yres || oldbpp != disp->var.bits_per_pixel)
 			set_imstt_regvals(p, disp->var.bits_per_pixel);
@@ -1680,10 +1680,10 @@
 		if (!compute_imstt_regvals(p, new->var.xres, new->var.yres))
 			return -1;
 		if (new->var.bits_per_pixel == 16) {
-			if (new->var.green.length == 6)
-				set_565(p);
-			else
+			if (new->var.green.length == 5)
 				set_555(p);
+			else
+				set_565(p);
 		}
 		set_imstt_regvals(p, new->var.bits_per_pixel);
 	}
@@ -1861,10 +1861,10 @@
 
 	if (!noaccel && p->ramdac == IBM)
 		imstt_cursor_init(p);
-	if (p->disp.var.green.length == 6)
-		set_565(p);
-	else
+	if (p->disp.var.green.length == 5)
 		set_555(p);
+	else
+		set_565(p);
 	set_imstt_regvals(p, p->disp.var.bits_per_pixel);
 
 	p->disp.var.pixclock = 1000000 / getclkMHz(p);

Re: PATCH: bugfix für RadeonFB (against 2.4.22-ac4, bug in 2.6.0-test9, too)

[Benjamin Herrenschmidt]

> The first patch is a bugfix in radeonfb.
> The bug is in 2.6.0-test9, too. The second red should be blue.
> 
> --- linux-2.4.22-ac4/drivers/video/radeonfb.c	2003-11-05 19:47:16.000000000 +0100
> +++ linux-2.4.22-ac4_patched/drivers/video/radeonfb.c	2003-11-05 22:08:39.000000000 +0100
> @@ -2362,7 +2362,7 @@
>  			disp->visual = FB_VISUAL_DIRECTCOLOR;
>  			v.red.offset = 10;
>  			v.green.offset = 5;
> -			v.red.offset = 0;
> +			v.blue.offset = 0;
>  			v.red.length = v.green.length = v.blue.length = 5;
>  			v.transp.offset = v.transp.length = 0;
>  			break;

Thanks. Applied to my tree.

Ben.

Re: PATCH: bugfix für RadeonFB (against 2.4.22-ac4, bug in 2.6.0-test9, too)

[Geert Uytterhoeven]

On Wed, 5 Nov 2003, Ronald Lembcke wrote:
> The following patches make a few framebuffer-drivers behave (imho) more
> sensible: (Matrox and Riva allready behave like this)
> 
> When selecting a video mode with bits_per_pixel == 16 is selected, but
> green.length != 6 (... 0 because the calling programm made no assumtion
> if the mode is 5/6/5 or maybe 6/5/5 or whatever) a 15 bpp mode is
> selected with 5/5/5. 
> That's not nice ... when 16 bpp were requested.

Your change is not correct: bpp is the _physical_ bits per pixel, i.e. it's 16
for both color depth 15 (5/5/5 mode) and color depth 16 (5/6/5).

To differentiate between 5/5/5 and 5/6/5, you have to look at green.length, and
apply standard fbdev fit-our-round-up[*] rules.

Note that some hardware in addition does RGBA4444, too.

Gr{oetje,eeting}s,

						Geert

[*] If a value doesn't fit, try to round it _up_. If that fails, return an
    error.
--
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: PATCH: bugfix für RadeonFB (against 2.4.22-ac4, bug in 2.6.0-test9, too)

[Ronald Lembcke]

On Wed, Nov 19, 2003 at 10:50:22AM +0100, Geert Uytterhoeven wrote:
> Your change is not correct: bpp is the _physical_ bits per pixel, i.e. it's 16
> for both color depth 15 (5/5/5 mode) and color depth 16 (5/6/5).
> 
> To differentiate between 5/5/5 and 5/6/5, you have to look at green.length, and
> apply standard fbdev fit-our-round-up[*] rules.
I don't see where this rule applies here. I searched on google, but
did't find anything about rounding of color depth.

> Note that some hardware in addition does RGBA4444, too.
Yes, but that nothing to do with these patches.

Sorry, I don't see, where my patches for intelfb and radonfb (as said
before, I'm not sure about the imsttfb patch anyway) make the driver
less correct. Where are they wrong?

Nothing about the use of bits_per_pixel is changed.
The only change is whether 555 or 565 is default.

If those patches are wrong, than matroxfb and rivafb are buggy, too:

matrox/matroxfb_crtc2.c:
	if (var->bits_per_pixel == 16) {
		if (var->green.length == 5) {
			var->red.offset = 10;
			var->red.length = 5;
			var->green.offset = 5;
			var->green.length = 5;
			var->blue.offset = 0;
			var->blue.length = 5;
			var->transp.offset = 15;
			var->transp.length = 1;
			*mode = 15;
		} else {
			var->red.offset = 11;
			var->red.length = 5;
			var->green.offset = 5;
			var->green.length = 6;
			var->blue.offset = 0;
			var->blue.length = 5;
			var->transp.offset = 0;
			var->transp.length = 0;
		}



riva/fbdev.c
        switch (v.bits_per_pixel) {
[...]
	case 9 ... 15:
		v.green.length = 5;
		/* fall through */
	case 16:
		v.bits_per_pixel = 16;
		nom = 2;
		den = 1;
		if (v.green.length == 5) {
			/* 0rrrrrgg gggbbbbb */
			v.red.offset = 10;
			v.green.offset = 5;
			v.blue.offset = 0;
			v.red.length = 5;
			v.green.length = 5;
			v.blue.length = 5;
		} else {
			/* rrrrrggg gggbbbbb */
			v.red.offset = 11;
			v.green.offset = 5;
			v.blue.offset = 0;
			v.red.length = 5;
			v.green.length = 6;
			v.blue.length = 5;
		}
		break;


They do the same thing as intelfb and radonfb with the patches applied.
So both must be wrong or right.

Roni

Re: PATCH: bugfix für RadeonFB (against 2.4.22-ac4, bug in 2.6.0-test9, too)

[Geert Uytterhoeven]

On Wed, 19 Nov 2003, Ronald Lembcke wrote:
> On Wed, Nov 19, 2003 at 10:50:22AM +0100, Geert Uytterhoeven wrote:
> > Your change is not correct: bpp is the _physical_ bits per pixel, i.e. it's 16
> > for both color depth 15 (5/5/5 mode) and color depth 16 (5/6/5).
> > 
> > To differentiate between 5/5/5 and 5/6/5, you have to look at green.length, and
> > apply standard fbdev fit-our-round-up[*] rules.
> I don't see where this rule applies here. I searched on google, but
> did't find anything about rounding of color depth.
> 
> > Note that some hardware in addition does RGBA4444, too.
> Yes, but that nothing to do with these patches.
> 
> Sorry, I don't see, where my patches for intelfb and radonfb (as said
> before, I'm not sure about the imsttfb patch anyway) make the driver
> less correct. Where are they wrong?
> 
> Nothing about the use of bits_per_pixel is changed.
> The only change is whether 555 or 565 is default.

Which depends on the color bitmask lengths...

> If those patches are wrong, than matroxfb and rivafb are buggy, too:

Most drivers indeed don't apply the fit-our-round-up rules to the color bitmask
lengths.

My main concern is that while you touch those parts of the code, you should
make it follow the rules instead of doing some ad-hoc changes.

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