[PATCH] s3fb: mode selection fixes

From: Krzysztof Helt <redacted>

This patch fixes bugs related to mode selection in the s3fb driver:
- fixes definition of the 32-bit format,
- fixes wrong error value returned when format is not supported
  by chip,
- sets r,g,b length field from the bits_per_pixel value otherwise
  the fbset fails in simple case like switching depths: 
  8bpp ->32bpp -> 8bpp

Signed-off-by: Krzysztof Helt <redacted>
---

This patch keeps selected depth while switching resolutions.

Regards,
Krzysztof

diff -urp linux-2.6.22/drivers/video/s3fb.c linux-2.6.23/drivers/video/s3fb.c

--- linux-2.6.22/drivers/video/s3fb.c	2007-07-09 01:32:17.000000000 +0200
+++ linux-2.6.23/drivers/video/s3fb.c	2007-09-30 13:26:11.000000000 +0200

@@ -58,7 +58,7 @@ static const struct svga_fb_format s3fb_
 		FB_TYPE_PACKED_PIXELS, 0,		FB_VISUAL_TRUECOLOR, 2, 4},
 	{24,  {16, 8, 0}, {8, 8, 0},  {0, 8, 0}, {0, 0, 0}, 0,
 		FB_TYPE_PACKED_PIXELS, 0,		FB_VISUAL_TRUECOLOR, 1, 2},
-	{32,  {16, 8, 0}, {8, 8, 0},  {0, 8, 0}, {0, 0, 0}, 0,
+	{32,  {16, 8, 0}, {8, 8, 0},  {0, 8, 0}, {24, 8, 0}, 0,
 		FB_TYPE_PACKED_PIXELS, 0,		FB_VISUAL_TRUECOLOR, 1, 2},
 	SVGA_FORMAT_END
 };

@@ -401,12 +401,42 @@ static int s3fb_check_var(struct fb_var_
 	struct s3fb_info *par = info->par;
 	int rv, mem, step;
 
+	/* set predefined mode for bits_per_pixel settings */
+	var->transp.offset = 0;
+	var->transp.length = 0;
+	switch(var->bits_per_pixel) {
+	case 4:
+	case 8:
+		var->red.length = 6;
+		var->red.offset = 0;
+		var->green = var->red;
+		var->blue = var->red;
+		break;
+	case 16:
+		var->red.length = 5;
+		var->blue.length = 5;
+		if (var->green.length != 6 && var->green.length != 5)
+			var->green.length = 6;
+		var->transp.length = 0;
+		break;
+	case 32:
+		var->transp.length = 8;
+		var->transp.offset = 24;
+	case 24:
+		var->red.length = 8;
+		var->green = var->red;
+		var->blue = var->red;
+		break;
+	default:
+		printk(KERN_ERR "depth not supported: %u\n", var->bits_per_pixel);
+		return -EINVAL;
+	}
 	/* Find appropriate format */
 	rv = svga_match_format (s3fb_formats, var, NULL);
 	if ((rv < 0) || ((par->chip == CHIP_988_VIRGE_VX) ? (rv == 7) : (rv == 6)))
 	{		/* 24bpp on VIRGE VX, 32bpp on others */
 		printk(KERN_ERR "fb%d: unsupported mode requested\n", info->node);
-		return rv;
+		return rv < 0 ? rv : -EINVAL;
 	}
 
 	/* Do not allow to have real resoulution larger than virtual */



----------------------------------------------------------------------

Wygrasz, czy przegrasz?

quoted

quoted

http://link.interia.pl/f1bbd

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

On Sun, Sep 30, 2007 at 04:10:20PM +0200, Krzysztof Helt wrote:

From: Krzysztof Helt <redacted>

This patch fixes bugs related to mode selection in the s3fb driver:
- fixes definition of the 32-bit format,
- fixes wrong error value returned when format is not supported
  by chip,
- sets r,g,b length field from the bits_per_pixel value otherwise
  the fbset fails in simple case like switching depths: 
  8bpp ->32bpp -> 8bpp

Signed-off-by: Krzysztof Helt <redacted>

NACK

Why do you think this is correct? I think that the highest byte is usually
just a padding. In one older version of this driver i used 'your' version of
that structure and i perceived problems with some cards (but i forgot which
cards exactly) so perhaps that on some cards the highest byte is not just a
padding, but neither alpha channel.

+	/* set predefined mode for bits_per_pixel settings */
+	var->transp.offset = 0;
+	var->transp.length = 0;
+	switch(var->bits_per_pixel) {
+	case 4:
+	case 8:
+		var->red.length = 6;
+		var->red.offset = 0;
+		var->green = var->red;
+		var->blue = var->red;
+		break;
+	case 16:
+		var->red.length = 5;
+		var->blue.length = 5;
+		if (var->green.length != 6 && var->green.length != 5)
+			var->green.length = 6;
+		var->transp.length = 0;
+		break;
+	case 32:
+		var->transp.length = 8;
+		var->transp.offset = 24;
+	case 24:
+		var->red.length = 8;
+		var->green = var->red;
+		var->blue = var->red;
+		break;
+	default:
+		printk(KERN_ERR "depth not supported: %u\n", var->bits_per_pixel);
+		return -EINVAL;
+	}

This is unnecessary, red, green and blue structures are filled by
svga_match_format().

- sets r,g,b length field from the bits_per_pixel value otherwise
  the fbset fails in simple case like switching depths: 
  8bpp ->32bpp -> 8bpp

This problem is not caused by not not filled length fields (you can examine
that fields by fbset). Problem is in strict interpretation of matching rules
by this driver and in a strange fbset behavior.

When some depth and some color lengths are requested then this driver
(or precisely svga_match_format() ) only changes them up (which is correct
behavior according to some previous email from A. Daplas).

But if you do some partial change of mode using fbset, then fbset uses
previous values for unspecified values of var structure in
FBIOPUT_VSCREENINFO call. For example if you use 32bpp and do
fbset -depth 16, then fbset calls FBIOPUT_VSCREENINFO with arguments
requesting depth 16 but color length 8 for each channel. Better way
is to put zeroes to color lengts (which is what fbset does when you
do something like 'fbset 640x480-60 -depth 16') - workaround is to use
argument -rgba 0,0,0,0  with -depth change using fbset.

I don't really care what is a correct behavior of FBIOPUT_VSCREENINFO,
but it should be consistent, explainable and fix should implement it in
a generic way in svga_match_format() function.

 	/* Find appropriate format */
 	rv = svga_match_format (s3fb_formats, var, NULL);
 	if ((rv < 0) || ((par->chip == CHIP_988_VIRGE_VX) ? (rv == 7) : (rv == 6)))
 	{		/* 24bpp on VIRGE VX, 32bpp on others */
 		printk(KERN_ERR "fb%d: unsupported mode requested\n", info->node);
-		return rv;
+		return rv < 0 ? rv : -EINVAL;
 	}

Yes, this is a bug. 'return -EINVAL' is sufficient (as EINVAL is only error
returned by svga_match_format() ) to fix it.

-- 
Elen sila lumenn' omentielvo

Ondrej 'SanTiago' Zajicek (email: santiago@crfreenet.org, jabber: santiago@njs.netlab.cz)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

On Sun, 30 Sep 2007 22:31:37 +0200
Ondrej Zajicek [off-list ref] wrote:

On Sun, Sep 30, 2007 at 04:10:20PM +0200, Krzysztof Helt wrote:

quoted

- sets r,g,b length field from the bits_per_pixel value otherwise
  the fbset fails in simple case like switching depths: 
  8bpp ->32bpp -> 8bpp

This problem is not caused by not not filled length fields (you can examine
that fields by fbset). Problem is in strict interpretation of matching rules
by this driver and in a strange fbset behavior.

When some depth and some color lengths are requested then this driver
(or precisely svga_match_format() ) only changes them up (which is correct
behavior according to some previous email from A. Daplas).

But if you do some partial change of mode using fbset, then fbset uses
previous values for unspecified values of var structure in
FBIOPUT_VSCREENINFO call. For example if you use 32bpp and do
fbset -depth 16, then fbset calls FBIOPUT_VSCREENINFO with arguments
requesting depth 16 but color length 8 for each channel. Better way
is to put zeroes to color lengths (which is what fbset does when you
do something like 'fbset 640x480-60 -depth 16') - workaround is to use
argument -rgba 0,0,0,0  with -depth change using fbset.

What is a difference if certain color depths allow only specific color lengths?
The 8bpp depth requires 6-bit color lengths. One can set them to 6 in the
driver and make it works for with just "fbset -depth 8" or leave wondering
confused user how to set it up.

quoted

 	/* Find appropriate format */
 	rv = svga_match_format (s3fb_formats, var, NULL);
 	if ((rv < 0) || ((par->chip == CHIP_988_VIRGE_VX) ? (rv == 7) : (rv == 6)))
 	{		/* 24bpp on VIRGE VX, 32bpp on others */
 		printk(KERN_ERR "fb%d: unsupported mode requested\n", info->node);
-		return rv;
+		return rv < 0 ? rv : -EINVAL;
 	}

Yes, this is a bug. 'return -EINVAL' is sufficient (as EINVAL is only error
returned by svga_match_format() ) to fix it.

This is short sighted. If the implementation of svga_match_format will 
change and will return new error values the -EINVAL will overwrite them
(so it is opposite to the original code which returned
the svga_match_format() error value).

Regards,
Krzysztof


----------------------------------------------------------------------
Tutaj sa Twoi nowi znajomi!
Sprawdz >>> http://link.interia.pl/f1bb7


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

On Mon, Oct 01, 2007 at 07:43:45PM +0200, Krzysztof Helt wrote:

quoted

quoted

- sets r,g,b length field from the bits_per_pixel value otherwise
  the fbset fails in simple case like switching depths: 
  8bpp ->32bpp -> 8bpp

When some depth and some color lengths are requested then this driver
(or precisely svga_match_format() ) only changes them up (which is correct
behavior according to some previous email from A. Daplas).

But if you do some partial change of mode using fbset, then fbset uses
previous values for unspecified values of var structure in
FBIOPUT_VSCREENINFO call. For example if you use 32bpp and do
fbset -depth 16, then fbset calls FBIOPUT_VSCREENINFO with arguments
requesting depth 16 but color length 8 for each channel. Better way
is to put zeroes to color lengths (which is what fbset does when you
do something like 'fbset 640x480-60 -depth 16') - workaround is to use
argument -rgba 0,0,0,0  with -depth change using fbset.

What is a difference if certain color depths allow only specific color lengths?

No difference. svga_match_format() interprets color lengths in
FBIOPUT_VSCREENINFO as minimal requests.

Perhaps svga_match_format() could be changed to choose any mode with
matching depth in case there is no mode with matching depth and color
lengths. I will send patch.

-- 
Elen sila lumenn' omentielvo

Ondrej 'SanTiago' Zajicek (email: santiago@crfreenet.org, jabber: santiago@njs.netlab.cz)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."