From: Krzysztof Helt <hidden> Date: 2009-03-01 20:50:48
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
@@ -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'vefailedtosettheDACpaletteformat--*timetocorrectvar.-*/-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
From: Ville Syrjälä <syrjala@sci.fi> Date: 2009-03-02 13:02:12
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
From: Krzysztof Helt <hidden> Date: 2009-03-02 16:57:21
On Mon, 2 Mar 2009 15:02:02 +0200
Ville Syrjälä [off-list ref] wrote:
On Sun, Mar 01, 2009 at 09:55:56PM +0100, Krzysztof Helt wrote:
quoted
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.
quoted
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
From: Ville Syrjälä <syrjala@sci.fi> Date: 2009-03-02 17:40:59
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ä [off-list ref] wrote:
quoted
On Sun, Mar 01, 2009 at 09:55:56PM +0100, Krzysztof Helt wrote:
quoted
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.
quoted
quoted
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
From: Michal Januszewski <hidden> Date: 2009-03-29 19:14:11
On Mon, Mar 2, 2009 at 19:02, Krzysztof Helt [off-list ref] wrote:
quoted
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.
quoted
quoted
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.
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);
On Sun, Mar 29, 2009 at 21:14, Michal Januszewski [off-list ref] wrote:
On Mon, Mar 2, 2009 at 19:02, Krzysztof Helt [off-list ref] wrote:
A slight omission in the fbdev API I suppose since the LUT entries are
quoted
quoted
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.
quoted
quoted
quoted
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
From: Ville Syrjälä <syrjala@sci.fi> Date: 2009-03-29 21:18:35
On Sun, Mar 29, 2009 at 09:26:02PM +0200, Geert Uytterhoeven wrote:
On Sun, Mar 29, 2009 at 21:14, Michal Januszewski [off-list ref] wrote:
quoted
On Mon, Mar 2, 2009 at 19:02, Krzysztof Helt [off-list ref] wrote:
A slight omission in the fbdev API I suppose since the LUT entries are
quoted
quoted
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.
quoted
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/
------------------------------------------------------------------------------