From: Krzysztof H. <krz...@po...> - 2007-09-30 14:02:01
|
From: Krzysztof Helt <krz...@wp...> 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 <krz...@wp...> --- 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? >>> http://link.interia.pl/f1bbd |
From: Ondrej Z. <san...@cr...> - 2007-09-30 20:32:14
|
On Sun, Sep 30, 2007 at 04:10:20PM +0200, Krzysztof Helt wrote: > From: Krzysztof Helt <krz...@wp...> > > 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 <krz...@wp...> NACK > 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 > }; 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: san...@cr..., jabber: san...@nj...) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so." |
From: Krzysztof H. <krz...@po...> - 2007-10-01 17:35:39
|
On Sun, 30 Sep 2007 22:31:37 +0200 Ondrej Zajicek <san...@cr...> wrote: > On Sun, Sep 30, 2007 at 04:10:20PM +0200, Krzysztof Helt wrote: > > - 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. > > /* 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 |
From: Ondrej Z. <san...@cr...> - 2007-10-01 20:50:17
|
On Mon, Oct 01, 2007 at 07:43:45PM +0200, Krzysztof Helt wrote: > > > - sets r,g,b length field from the bits_per_pixel value otherwise > > > the fbset fails in simple case like switching depths:=20 > > > 8bpp ->32bpp -> 8bpp > >=20 > > When some depth and some color lengths are requested then this driver > > (or precisely svga_match_format() ) only changes them up (which is corr= ect > > behavior according to some previous email from A. Daplas). > >=20 > > 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. > >=20 >=20 > What is a difference if certain color depths allow only specific color le= ngths? 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. --=20 Elen sila lumenn' omentielvo Ondrej 'SanTiago' Zajicek (email: san...@cr..., 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." |