Fix 8bpp RGB fields length

Fix 8bpp RGB fields length

[Krzysztof Helt]

Hi,

I found that some fbdev drivers set RGB field's lengths incorrectly
to 6 bits. This is incorrect as the field's length says how many bits
has a color index when using color palette (6 bits = 64 colors).

Fix this error in uvesafb by dropping DAC switching to 8 bits 
completely. Advantage of this approach is making the driver shorter,
disadvantage is that some color fidelity is lost.

Regards,
Krzysztof


diff --git a/drivers/video/uvesafb.c b/drivers/video/uvesafb.c
index 6c2d37f..76661bf 100644
--- a/drivers/video/uvesafb.c
+++ b/drivers/video/uvesafb.c
@@ -300,22 +300,10 @@ static void uvesafb_setup_var(struct fb_var_screeninfo *var,
 		var->blue.offset   = 0;
 		var->transp.offset = 0;
 
-		/*
-		 * We're assuming that we can switch the DAC to 8 bits. If
-		 * this proves to be incorrect, we'll update the fields
-		 * later in set_par().
-		 */
-		if (par->vbe_ib.capabilities & VBE_CAP_CAN_SWITCH_DAC) {
-			var->red.length    = 8;
-			var->green.length  = 8;
-			var->blue.length   = 8;
-			var->transp.length = 0;
-		} else {
-			var->red.length    = 6;
-			var->green.length  = 6;
-			var->blue.length   = 6;
-			var->transp.length = 0;
-		}
+		var->red.length    = 8;
+		var->green.length  = 8;
+		var->blue.length   = 8;
+		var->transp.length = 0;
 	}
 }
 
@@ -998,16 +986,15 @@ static int uvesafb_setcolreg(unsigned regno, unsigned red, unsigned green,
 		struct fb_info *info)
 {
 	struct uvesafb_pal_entry entry;
-	int shift = 16 - info->var.green.length;
 	int err = 0;
 
 	if (regno >= info->cmap.len)
 		return -EINVAL;
 
 	if (info->var.bits_per_pixel == 8) {
-		entry.red   = red   >> shift;
-		entry.green = green >> shift;
-		entry.blue  = blue  >> shift;
+		entry.red   = red   >> 10;
+		entry.green = green >> 10;
+		entry.blue  = blue  >> 10;
 		entry.pad   = 0;
 
 		err = uvesafb_setpalette(&entry, 1, regno, info);
@@ -1047,7 +1034,6 @@ static int uvesafb_setcolreg(unsigned regno, unsigned red, unsigned green,
 static int uvesafb_setcmap(struct fb_cmap *cmap, struct fb_info *info)
 {
 	struct uvesafb_pal_entry *entries;
-	int shift = 16 - info->var.green.length;
 	int i, err = 0;
 
 	if (info->var.bits_per_pixel == 8) {
@@ -1060,9 +1046,9 @@ static int uvesafb_setcmap(struct fb_cmap *cmap, struct fb_info *info)
 			return -ENOMEM;
 
 		for (i = 0; i < cmap->len; i++) {
-			entries[i].red   = cmap->red[i]   >> shift;
-			entries[i].green = cmap->green[i] >> shift;
-			entries[i].blue  = cmap->blue[i]  >> shift;
+			entries[i].red   = cmap->red[i]   >> 10;
+			entries[i].green = cmap->green[i] >> 10;
+			entries[i].blue  = cmap->blue[i]  >> 10;
 			entries[i].pad   = 0;
 		}
 		err = uvesafb_setpalette(entries, cmap->len, cmap->start, info);
@@ -1299,26 +1285,6 @@ setmode:
 	}
 	par->mode_idx = i;
 
-	/* For 8bpp modes, always try to set the DAC to 8 bits. */
-	if (par->vbe_ib.capabilities & VBE_CAP_CAN_SWITCH_DAC &&
-	    mode->bits_per_pixel <= 8) {
-		uvesafb_reset(task);
-		task->t.regs.eax = 0x4f08;
-		task->t.regs.ebx = 0x0800;
-
-		err = uvesafb_exec(task);
-		if (err || (task->t.regs.eax & 0xffff) != 0x004f ||
-		    ((task->t.regs.ebx & 0xff00) >> 8) != 8) {
-			/*
-			 * We've failed to set the DAC palette format -
-			 * time to correct var.
-			 */
-			info->var.red.length    = 6;
-			info->var.green.length  = 6;
-			info->var.blue.length   = 6;
-		}
-	}
-
 	info->fix.visual = (info->var.bits_per_pixel == 8) ?
 				FB_VISUAL_PSEUDOCOLOR : FB_VISUAL_TRUECOLOR;
 	info->fix.line_length = mode->bytes_per_scan_line;

----------------------------------------------------------------------
Dodatkowy dzien wolny od pracy? Wypowiedz sie i wygraj nagrode!
Wejdz >>  http://link.interia.pl/f2077 


------------------------------------------------------------------------------
Open Source Business Conference (OSBC), March 24-25, 2009, San Francisco, CA
-OSBC tackles the biggest issue in open source: Open Sourcing the Enterprise
-Strategies to boost innovation and cut costs with open source participation
-Receive a $600 discount off the registration fee with the source code: SFAD
http://p.sf.net/sfu/XcvMzF8H

Re: Fix 8bpp RGB fields length

[Ville Syrjälä]

On Sun, Mar 01, 2009 at 09:55:56PM +0100, Krzysztof Helt wrote:
> Hi,
> 
> I found that some fbdev drivers set RGB field's lengths incorrectly
> to 6 bits. This is incorrect as the field's length says how many bits
> has a color index when using color palette (6 bits = 64 colors).

A slight omission in the fbdev API I suppose since the LUT entries are
nearly always 3*8bits wide. VGA being the exception.

> Fix this error in uvesafb by dropping DAC switching to 8 bits 
> completely. Advantage of this approach is making the driver shorter,
> disadvantage is that some color fidelity is lost.

I don't see much point in dropping this. It would reduce the image
quality and it looks like after your first fix this code would amount
to less than a dozen lines. But not my call anyway.

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/

------------------------------------------------------------------------------
Open Source Business Conference (OSBC), March 24-25, 2009, San Francisco, CA
-OSBC tackles the biggest issue in open source: Open Sourcing the Enterprise
-Strategies to boost innovation and cut costs with open source participation
-Receive a $600 discount off the registration fee with the source code: SFAD
http://p.sf.net/sfu/XcvMzF8H

Re: Fix 8bpp RGB fields length

[Krzysztof Helt]

On Mon, 2 Mar 2009 15:02:02 +0200
Ville Syrjälä <syrjala@sci.fi> wrote:

> On Sun, Mar 01, 2009 at 09:55:56PM +0100, Krzysztof Helt wrote:
> > Hi,
> > 
> > I found that some fbdev drivers set RGB field's lengths incorrectly
> > to 6 bits. This is incorrect as the field's length says how many bits
> > has a color index when using color palette (6 bits = 64 colors).
> 
> A slight omission in the fbdev API I suppose since the LUT entries are
> nearly always 3*8bits wide. VGA being the exception.
> 

If offsets of all RGB components are the same the length field says
how lone the pallete index is. It does not say anything how long
the LUT entries are. This is the same misunderstanding as done
inside the driver.

> > Fix this error in uvesafb by dropping DAC switching to 8 bits 
> > completely. Advantage of this approach is making the driver shorter,
> > disadvantage is that some color fidelity is lost.
> 
> I don't see much point in dropping this. It would reduce the image
> quality and it looks like after your first fix this code would amount
> to less than a dozen lines. But not my call anyway.
> 

I wonder if it affects quality at all as with 256 different pixel values
selected from over 262000 is not much more better quality that
 if these 256 values are taken from another set of values which
is only about 3% different.

My patch is a proposition not a final solution. I hope Michał will
propose something better.

Regards,
Krzysztof

----------------------------------------------------------------------
Dodatkowy dzien wolny od pracy? Wypowiedz sie i wygraj nagrode!
Wejdz >>  http://link.interia.pl/f2077 


------------------------------------------------------------------------------
Open Source Business Conference (OSBC), March 24-25, 2009, San Francisco, CA
-OSBC tackles the biggest issue in open source: Open Sourcing the Enterprise
-Strategies to boost innovation and cut costs with open source participation
-Receive a $600 discount off the registration fee with the source code: SFAD
http://p.sf.net/sfu/XcvMzF8H
_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: Fix 8bpp RGB fields length

[Ville Syrjälä]

On Mon, Mar 02, 2009 at 06:02:39PM +0100, Krzysztof Helt wrote:
> On Mon, 2 Mar 2009 15:02:02 +0200
> Ville Syrjälä <syrjala@sci.fi> wrote:
> 
> > On Sun, Mar 01, 2009 at 09:55:56PM +0100, Krzysztof Helt wrote:
> > > Hi,
> > > 
> > > I found that some fbdev drivers set RGB field's lengths incorrectly
> > > to 6 bits. This is incorrect as the field's length says how many bits
> > > has a color index when using color palette (6 bits = 64 colors).
> > 
> > A slight omission in the fbdev API I suppose since the LUT entries are
> > nearly always 3*8bits wide. VGA being the exception.
> > 
> 
> If offsets of all RGB components are the same the length field says
> how lone the pallete index is. It does not say anything how long
> the LUT entries are. This is the same misunderstanding as done
> inside the driver.

Yes. I meant that the change is correct but the API doesn't provide any
means of conveying the LUT entry size to userspace which is a bit
unfortunate.

> > > Fix this error in uvesafb by dropping DAC switching to 8 bits 
> > > completely. Advantage of this approach is making the driver shorter,
> > > disadvantage is that some color fidelity is lost.
> > 
> > I don't see much point in dropping this. It would reduce the image
> > quality and it looks like after your first fix this code would amount
> > to less than a dozen lines. But not my call anyway.
> > 
> 
> I wonder if it affects quality at all as with 256 different pixel values
> selected from over 262000 is not much more better quality that
>  if these 256 values are taken from another set of values which
> is only about 3% different.

I suppose it doesn't matter much unless the palette is tuned to produce
nice gradients.

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/

------------------------------------------------------------------------------
Open Source Business Conference (OSBC), March 24-25, 2009, San Francisco, CA
-OSBC tackles the biggest issue in open source: Open Sourcing the Enterprise
-Strategies to boost innovation and cut costs with open source participation
-Receive a $600 discount off the registration fee with the source code: SFAD
http://p.sf.net/sfu/XcvMzF8H

Re: Fix 8bpp RGB fields length

[Michal Januszewski]

On Mon, Mar 2, 2009 at 19:02, Krzysztof Helt <krzysztof.h1@poczta.fm> wrote:

>> A slight omission in the fbdev API I suppose since the LUT entries are
>> nearly always 3*8bits wide. VGA being the exception.
>
> If offsets of all RGB components are the same the length field says
> how lone the pallete index is. It does not say anything how long
> the LUT entries are. This is the same misunderstanding as done
> inside the driver.

Where is this meaning of the length field defined?  While I agree
that interpreting it as the length of the palette seems to make
more sense (considering the current FB API), there are several
places in the fbdev code contradicting this interpretation
(comments in skeletonfb.c, vfb.c, code in vga16fb.c, ..).

IMO, if we're going to fix uvesafb, we should also fix all the
other drivers and documentation along with it.

>> > Fix this error in uvesafb by dropping DAC switching to 8 bits
>> > completely. Advantage of this approach is making the driver shorter,
>> > disadvantage is that some color fidelity is lost.
>>
> [..]
>
> My patch is a proposition not a final solution. I hope Michał will
> propose something better.

I would prefer something along the lines of the patch below,
i.e. set the var fields correctly, but track and use the DAC
width internally.  This makes sense since the DAC width is
not used by userspace anyway -- the color definitions passed
to setcolreg are always 16-bit wide and it's the driver's
job to scale them down appropriately.

diff --git a/drivers/video/uvesafb.c b/drivers/video/uvesafb.c
index 6c2d37f..95819cc 100644
--- a/drivers/video/uvesafb.c
+++ b/drivers/video/uvesafb.c
@@ -55,6 +55,7 @@ static u16 maxvf	__devinitdata; /* maximum vertical
frequency */
 static u16 maxhf	__devinitdata; /* maximum horizontal frequency */
 static u16 vbemode	__devinitdata; /* force use of a specific VBE mode */
 static char *mode_option __devinitdata;
+static u8  dac_width	= 6;

 static struct uvesafb_ktask *uvfb_tasks[UVESAFB_TASKS_MAX];
 static DEFINE_MUTEX(uvfb_lock);
@@ -300,22 +301,10 @@ static void uvesafb_setup_var(struct
fb_var_screeninfo *var,
 		var->blue.offset   = 0;
 		var->transp.offset = 0;

-		/*
-		 * We're assuming that we can switch the DAC to 8 bits. If
-		 * this proves to be incorrect, we'll update the fields
-		 * later in set_par().
-		 */
-		if (par->vbe_ib.capabilities & VBE_CAP_CAN_SWITCH_DAC) {
-			var->red.length    = 8;
-			var->green.length  = 8;
-			var->blue.length   = 8;
-			var->transp.length = 0;
-		} else {
-			var->red.length    = 6;
-			var->green.length  = 6;
-			var->blue.length   = 6;
-			var->transp.length = 0;
-		}
+		var->red.length    = 8;
+		var->green.length  = 8;
+		var->blue.length   = 8;
+		var->transp.length = 0;
 	}
 }

@@ -998,7 +987,7 @@ static int uvesafb_setcolreg(unsigned regno,
unsigned red, unsigned green,
 		struct fb_info *info)
 {
 	struct uvesafb_pal_entry entry;
-	int shift = 16 - info->var.green.length;
+	int shift = 16 - dac_width;
 	int err = 0;

 	if (regno >= info->cmap.len)
@@ -1047,7 +1036,7 @@ static int uvesafb_setcolreg(unsigned regno,
unsigned red, unsigned green,
 static int uvesafb_setcmap(struct fb_cmap *cmap, struct fb_info *info)
 {
 	struct uvesafb_pal_entry *entries;
-	int shift = 16 - info->var.green.length;
+	int shift = 16 - dac_width;
 	int i, err = 0;

 	if (info->var.bits_per_pixel == 8) {
@@ -1309,13 +1298,9 @@ setmode:
 		err = uvesafb_exec(task);
 		if (err || (task->t.regs.eax & 0xffff) != 0x004f ||
 		    ((task->t.regs.ebx & 0xff00) >> 8) != 8) {
-			/*
-			 * We've failed to set the DAC palette format -
-			 * time to correct var.
-			 */
-			info->var.red.length    = 6;
-			info->var.green.length  = 6;
-			info->var.blue.length   = 6;
+			dac_width = 6;
+		} else {
+			dac_width = 8;
 		}
 	}

------------------------------------------------------------------------------
_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: Fix 8bpp RGB fields length

[Geert Uytterhoeven]

On Sun, Mar 29, 2009 at 21:14, Michal Januszewski <michalj@gmail.com> wrote:
> On Mon, Mar 2, 2009 at 19:02, Krzysztof Helt <krzysztof.h1@poczta.fm> wrote:
> A slight omission in the fbdev API I suppose since the LUT entries are
>>> nearly always 3*8bits wide. VGA being the exception.
>>
>> If offsets of all RGB components are the same the length field says
>> how lone the pallete index is. It does not say anything how long
>> the LUT entries are. This is the same misunderstanding as done
>> inside the driver.
>
> Where is this meaning of the length field defined?  While I agree
> that interpreting it as the length of the palette seems to make
> more sense (considering the current FB API), there are several
> places in the fbdev code contradicting this interpretation
> (comments in skeletonfb.c, vfb.c, code in vga16fb.c, ..).

Originally it was the width of the color component in the DAC, i.e. 6
for VGA with 18 bit color palettes.
Later is was redefined to be the width of the bitfield in the pixel
data, for consistency with
other visuals (pseudocolor is directcolor where the R, G, B, and A
bitfields overlap),
and to handle hardware where the number of palette entries is not 1 << bpp
(e.g. 64 colors in 8 bpp packed pixels).

That's why vfb.c has an (obsolete) comment:

         * Pseudocolor:
         *    uses offset = 0 && length = RAMDAC register width.
         *    var->{color}.offset is 0
         *    var->{color}.length contains widht of DAC

while the (newer) skeletonfb mentions both:

     * Pseudocolor:
     *    var->{color}.offset is 0
     *    var->{color}.length contains width of DAC or the number of unique
     *                        colors available (color depth)

vga16fb.c is also an old driver.

> IMO, if we're going to fix uvesafb, we should also fix all the
> other drivers and documentation along with it.

Yep.

>>> > Fix this error in uvesafb by dropping DAC switching to 8 bits
>>> > completely. Advantage of this approach is making the driver shorter,
>>> > disadvantage is that some color fidelity is lost.
>>>
>> [..]
>>
>> My patch is a proposition not a final solution. I hope Michał will
>> propose something better.
>
> I would prefer something along the lines of the patch below,
> i.e. set the var fields correctly, but track and use the DAC
> width internally.  This makes sense since the DAC width is
> not used by userspace anyway -- the color definitions passed
> to setcolreg are always 16-bit wide and it's the driver's
> job to scale them down appropriately.

Yes, I agree.

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

------------------------------------------------------------------------------
_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: Fix 8bpp RGB fields length

[Ville Syrjälä]

On Sun, Mar 29, 2009 at 09:26:02PM +0200, Geert Uytterhoeven wrote:
> On Sun, Mar 29, 2009 at 21:14, Michal Januszewski <michalj@gmail.com> wrote:
> > On Mon, Mar 2, 2009 at 19:02, Krzysztof Helt <krzysztof.h1@poczta.fm> wrote:
> > A slight omission in the fbdev API I suppose since the LUT entries are
> >>> nearly always 3*8bits wide. VGA being the exception.
> >>
> >> If offsets of all RGB components are the same the length field says
> >> how lone the pallete index is. It does not say anything how long
> >> the LUT entries are. This is the same misunderstanding as done
> >> inside the driver.
> >
> > Where is this meaning of the length field defined?  While I agree
> > that interpreting it as the length of the palette seems to make
> > more sense (considering the current FB API), there are several
> > places in the fbdev code contradicting this interpretation
> > (comments in skeletonfb.c, vfb.c, code in vga16fb.c, ..).
> 
> Originally it was the width of the color component in the DAC, i.e. 6
> for VGA with 18 bit color palettes.
> Later is was redefined to be the width of the bitfield in the pixel
> data, for consistency with
> other visuals (pseudocolor is directcolor where the R, G, B, and A
> bitfields overlap),
> and to handle hardware where the number of palette entries is not 1 << bpp
> (e.g. 64 colors in 8 bpp packed pixels).
> 
> That's why vfb.c has an (obsolete) comment:
> 
>          * Pseudocolor:
>          *    uses offset = 0 && length = RAMDAC register width.
>          *    var->{color}.offset is 0
>          *    var->{color}.length contains widht of DAC
> 
> while the (newer) skeletonfb mentions both:
> 
>      * Pseudocolor:
>      *    var->{color}.offset is 0
>      *    var->{color}.length contains width of DAC or the number of unique
>      *                        colors available (color depth)
> 
> vga16fb.c is also an old driver.
> 
> > IMO, if we're going to fix uvesafb, we should also fix all the
> > other drivers and documentation along with it.
> 
> Yep.

The comment explaining fb_bitfield in linux/fb.h seems to need some
fixing too ie. remove the big-endian comment as I think everyone just
assumes the fb_bitfield values are in the device's native endianness.

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/

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