[PATCH RESEND] video: mxsfb: Fix colors display on lower color depth

[PATCH RESEND] video: mxsfb: Fix colors display on lower color depth

[Maxime Ripard]

The current code always registers as a 32 bits display, and uses the
hardware to drop the MSB of each color to abjust to the interface width
used by the panel.

This results on 18 bits (and probably 16 bits display as well) in colors
being displayed poorly, because the MSB are obviously the most important
bits for each color definition.

The default controller behaviour when using an interface width smaller
than the color depth is to drop the LSBs of each color, which makes more
sense because you lose the least important part of the color definition.

So, to fix the colors display, just get back to the default controller
behaviour.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
  - Copied Sascha

 drivers/video/mxsfb.c |    5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/video/mxsfb.c b/drivers/video/mxsfb.c
index 755556c..2cfaf8b 100644
--- a/drivers/video/mxsfb.c
+++ b/drivers/video/mxsfb.c
@@ -424,11 +424,6 @@ static int mxsfb_set_par(struct fb_info *fb_info)
 			return -EINVAL;
 		case STMLCDIF_16BIT:
 		case STMLCDIF_18BIT:
-			/* 24 bit to 18 bit mapping */
-			ctrl |= CTRL_DF24; /* ignore the upper 2 bits in
-					    *  each colour component
-					    */
-			break;
 		case STMLCDIF_24BIT:
 			/* real 24 bit */
 			break;
-- 
1.7.10.4

[PATCH RESEND] video: mxsfb: Fix colors display on lower color depth

[Maxime Ripard]

The current code always registers as a 32 bits display, and uses the
hardware to drop the MSB of each color to abjust to the interface width
used by the panel.

This results on 18 bits (and probably 16 bits display as well) in colors
being displayed poorly, because the MSB are obviously the most important
bits for each color definition.

The default controller behaviour when using an interface width smaller
than the color depth is to drop the LSBs of each color, which makes more
sense because you lose the least important part of the color definition.

So, to fix the colors display, just get back to the default controller
behaviour.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
  - Copied Sascha

 drivers/video/mxsfb.c |    5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/video/mxsfb.c b/drivers/video/mxsfb.c
index 755556c..2cfaf8b 100644
--- a/drivers/video/mxsfb.c
+++ b/drivers/video/mxsfb.c
@@ -424,11 +424,6 @@ static int mxsfb_set_par(struct fb_info *fb_info)
 			return -EINVAL;
 		case STMLCDIF_16BIT:
 		case STMLCDIF_18BIT:
-			/* 24 bit to 18 bit mapping */
-			ctrl |= CTRL_DF24; /* ignore the upper 2 bits in
-					    *  each colour component
-					    */
-			break;
 		case STMLCDIF_24BIT:
 			/* real 24 bit */
 			break;
-- 
1.7.10.4

[PATCH RESEND] video: mxsfb: Fix colors display on lower color depth

[Maxime Ripard]

The current code always registers as a 32 bits display, and uses the
hardware to drop the MSB of each color to abjust to the interface width
used by the panel.

This results on 18 bits (and probably 16 bits display as well) in colors
being displayed poorly, because the MSB are obviously the most important
bits for each color definition.

The default controller behaviour when using an interface width smaller
than the color depth is to drop the LSBs of each color, which makes more
sense because you lose the least important part of the color definition.

So, to fix the colors display, just get back to the default controller
behaviour.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
  - Copied Sascha

 drivers/video/mxsfb.c |    5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/video/mxsfb.c b/drivers/video/mxsfb.c
index 755556c..2cfaf8b 100644
--- a/drivers/video/mxsfb.c
+++ b/drivers/video/mxsfb.c
@@ -424,11 +424,6 @@ static int mxsfb_set_par(struct fb_info *fb_info)
 			return -EINVAL;
 		case STMLCDIF_16BIT:
 		case STMLCDIF_18BIT:
-			/* 24 bit to 18 bit mapping */
-			ctrl |= CTRL_DF24; /* ignore the upper 2 bits in
-					    *  each colour component
-					    */
-			break;
 		case STMLCDIF_24BIT:
 			/* real 24 bit */
 			break;
-- 
1.7.10.4

Re: [PATCH RESEND] video: mxsfb: Fix colors display on lower color depth

[Juergen Beisert]

Hi Maxime,

Maxime Ripard wrote:
> The current code always registers as a 32 bits display, and uses the
> hardware to drop the MSB of each color to abjust to the interface width
> used by the panel.
>
> This results on 18 bits (and probably 16 bits display as well) in colors
> being displayed poorly, because the MSB are obviously the most important
> bits for each color definition.

But the "def_rgb666" bitfield description reports correctly to use bit 0..5 
for each colour component in this mode. Maybe your userland program has a bug 
and forgets to honor the bitfield description?

> The default controller behaviour when using an interface width smaller
> than the color depth is to drop the LSBs of each color, which makes more
> sense because you lose the least important part of the color definition.
>
> So, to fix the colors display, just get back to the default controller
> behaviour.

If you do this change, you also must adapt the "def_rgb666" bitfield 
description.
This structure currently reports the lower 6 bits to be used for this mode:
  blue -> 0..5 (skip 6/7)
  green -> 8..13 (skip 14/15)
  red -> 16..21 (skip 22/23).

> [...]

jbe

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |

Re: [PATCH RESEND] video: mxsfb: Fix colors display on lower color depth

[Juergen Beisert]

Hi Maxime,

Maxime Ripard wrote:
> The current code always registers as a 32 bits display, and uses the
> hardware to drop the MSB of each color to abjust to the interface width
> used by the panel.
>
> This results on 18 bits (and probably 16 bits display as well) in colors
> being displayed poorly, because the MSB are obviously the most important
> bits for each color definition.

But the "def_rgb666" bitfield description reports correctly to use bit 0..5 
for each colour component in this mode. Maybe your userland program has a bug 
and forgets to honor the bitfield description?

> The default controller behaviour when using an interface width smaller
> than the color depth is to drop the LSBs of each color, which makes more
> sense because you lose the least important part of the color definition.
>
> So, to fix the colors display, just get back to the default controller
> behaviour.

If you do this change, you also must adapt the "def_rgb666" bitfield 
description.
This structure currently reports the lower 6 bits to be used for this mode:
  blue -> 0..5 (skip 6/7)
  green -> 8..13 (skip 14/15)
  red -> 16..21 (skip 22/23).

> [...]

jbe

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |

[PATCH RESEND] video: mxsfb: Fix colors display on lower color depth

[Juergen Beisert]

Hi Maxime,

Maxime Ripard wrote:
> The current code always registers as a 32 bits display, and uses the
> hardware to drop the MSB of each color to abjust to the interface width
> used by the panel.
>
> This results on 18 bits (and probably 16 bits display as well) in colors
> being displayed poorly, because the MSB are obviously the most important
> bits for each color definition.

But the "def_rgb666" bitfield description reports correctly to use bit 0..5 
for each colour component in this mode. Maybe your userland program has a bug 
and forgets to honor the bitfield description?

> The default controller behaviour when using an interface width smaller
> than the color depth is to drop the LSBs of each color, which makes more
> sense because you lose the least important part of the color definition.
>
> So, to fix the colors display, just get back to the default controller
> behaviour.

If you do this change, you also must adapt the "def_rgb666" bitfield 
description.
This structure currently reports the lower 6 bits to be used for this mode:
  blue -> 0..5 (skip 6/7)
  green -> 8..13 (skip 14/15)
  red -> 16..21 (skip 22/23).

> [...]

jbe

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |

Re: [PATCH RESEND] video: mxsfb: Fix colors display on lower color depth

[Maxime Ripard]

Hi Juergen,

Le 22/04/2013 11:08, Juergen Beisert a écrit :
> Hi Maxime,
> 
> Maxime Ripard wrote:
>> The current code always registers as a 32 bits display, and uses the
>> hardware to drop the MSB of each color to abjust to the interface width
>> used by the panel.
>>
>> This results on 18 bits (and probably 16 bits display as well) in colors
>> being displayed poorly, because the MSB are obviously the most important
>> bits for each color definition.
> 
> But the "def_rgb666" bitfield description reports correctly to use bit 0..5 
> for each colour component in this mode. Maybe your userland program has a bug 
> and forgets to honor the bitfield description?

Hmmm, that might be the case actually, yes, I'll check into this and let
you know. Thanks!

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH RESEND] video: mxsfb: Fix colors display on lower color depth

[Maxime Ripard]

Hi Juergen,

Le 22/04/2013 11:08, Juergen Beisert a écrit :
> Hi Maxime,
> 
> Maxime Ripard wrote:
>> The current code always registers as a 32 bits display, and uses the
>> hardware to drop the MSB of each color to abjust to the interface width
>> used by the panel.
>>
>> This results on 18 bits (and probably 16 bits display as well) in colors
>> being displayed poorly, because the MSB are obviously the most important
>> bits for each color definition.
> 
> But the "def_rgb666" bitfield description reports correctly to use bit 0..5 
> for each colour component in this mode. Maybe your userland program has a bug 
> and forgets to honor the bitfield description?

Hmmm, that might be the case actually, yes, I'll check into this and let
you know. Thanks!

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH RESEND] video: mxsfb: Fix colors display on lower color depth

[Maxime Ripard]

Hi Juergen,

Le 22/04/2013 11:08, Juergen Beisert a ?crit :
> Hi Maxime,
> 
> Maxime Ripard wrote:
>> The current code always registers as a 32 bits display, and uses the
>> hardware to drop the MSB of each color to abjust to the interface width
>> used by the panel.
>>
>> This results on 18 bits (and probably 16 bits display as well) in colors
>> being displayed poorly, because the MSB are obviously the most important
>> bits for each color definition.
> 
> But the "def_rgb666" bitfield description reports correctly to use bit 0..5 
> for each colour component in this mode. Maybe your userland program has a bug 
> and forgets to honor the bitfield description?

Hmmm, that might be the case actually, yes, I'll check into this and let
you know. Thanks!

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH RESEND] video: mxsfb: Fix colors display on lower color depth

[Marek Vasut]

Dear Maxime Ripard,

> The current code always registers as a 32 bits display, and uses the
> hardware to drop the MSB of each color to abjust to the interface width
> used by the panel.
> 
> This results on 18 bits (and probably 16 bits display as well) in colors
> being displayed poorly, because the MSB are obviously the most important
> bits for each color definition.
> 
> The default controller behaviour when using an interface width smaller
> than the color depth is to drop the LSBs of each color, which makes more
> sense because you lose the least important part of the color definition.
> 
> So, to fix the colors display, just get back to the default controller
> behaviour.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Did you receive my latest email? Check M28EVK (imx28-m28evk.dts), it uses 18bit 
LCD and works without this patch I think.

Best regards,
Marek Vasut

Re: [PATCH RESEND] video: mxsfb: Fix colors display on lower color depth

[Marek Vasut]

Dear Maxime Ripard,

> The current code always registers as a 32 bits display, and uses the
> hardware to drop the MSB of each color to abjust to the interface width
> used by the panel.
> 
> This results on 18 bits (and probably 16 bits display as well) in colors
> being displayed poorly, because the MSB are obviously the most important
> bits for each color definition.
> 
> The default controller behaviour when using an interface width smaller
> than the color depth is to drop the LSBs of each color, which makes more
> sense because you lose the least important part of the color definition.
> 
> So, to fix the colors display, just get back to the default controller
> behaviour.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Did you receive my latest email? Check M28EVK (imx28-m28evk.dts), it uses 18bit 
LCD and works without this patch I think.

Best regards,
Marek Vasut

[PATCH RESEND] video: mxsfb: Fix colors display on lower color depth

[Marek Vasut]

Dear Maxime Ripard,

> The current code always registers as a 32 bits display, and uses the
> hardware to drop the MSB of each color to abjust to the interface width
> used by the panel.
> 
> This results on 18 bits (and probably 16 bits display as well) in colors
> being displayed poorly, because the MSB are obviously the most important
> bits for each color definition.
> 
> The default controller behaviour when using an interface width smaller
> than the color depth is to drop the LSBs of each color, which makes more
> sense because you lose the least important part of the color definition.
> 
> So, to fix the colors display, just get back to the default controller
> behaviour.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Did you receive my latest email? Check M28EVK (imx28-m28evk.dts), it uses 18bit 
LCD and works without this patch I think.

Best regards,
Marek Vasut

Re: [PATCH RESEND] video: mxsfb: Fix colors display on lower color depth

[Maxime Ripard]

Hi Marek,

Le 22/04/2013 11:16, Marek Vasut a écrit :
> Dear Maxime Ripard,
> 
>> The current code always registers as a 32 bits display, and uses the
>> hardware to drop the MSB of each color to abjust to the interface width
>> used by the panel.
>>
>> This results on 18 bits (and probably 16 bits display as well) in colors
>> being displayed poorly, because the MSB are obviously the most important
>> bits for each color definition.
>>
>> The default controller behaviour when using an interface width smaller
>> than the color depth is to drop the LSBs of each color, which makes more
>> sense because you lose the least important part of the color definition.
>>
>> So, to fix the colors display, just get back to the default controller
>> behaviour.
>>
>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> 
> Did you receive my latest email? Check M28EVK (imx28-m28evk.dts), it uses 18bit 
> LCD and works without this patch I think.

Thanks for the pointer. You mentionned in your other mail that it was
wired on 24bits but that the screen is actually 18 bits. I went to
search for the schematics to look at the wirings to see what could
differ, but I couldn't find any for the M28EVK. Are they publicly available?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH RESEND] video: mxsfb: Fix colors display on lower color depth

[Maxime Ripard]

Hi Marek,

Le 22/04/2013 11:16, Marek Vasut a écrit :
> Dear Maxime Ripard,
> 
>> The current code always registers as a 32 bits display, and uses the
>> hardware to drop the MSB of each color to abjust to the interface width
>> used by the panel.
>>
>> This results on 18 bits (and probably 16 bits display as well) in colors
>> being displayed poorly, because the MSB are obviously the most important
>> bits for each color definition.
>>
>> The default controller behaviour when using an interface width smaller
>> than the color depth is to drop the LSBs of each color, which makes more
>> sense because you lose the least important part of the color definition.
>>
>> So, to fix the colors display, just get back to the default controller
>> behaviour.
>>
>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> 
> Did you receive my latest email? Check M28EVK (imx28-m28evk.dts), it uses 18bit 
> LCD and works without this patch I think.

Thanks for the pointer. You mentionned in your other mail that it was
wired on 24bits but that the screen is actually 18 bits. I went to
search for the schematics to look at the wirings to see what could
differ, but I couldn't find any for the M28EVK. Are they publicly available?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH RESEND] video: mxsfb: Fix colors display on lower color depth

[Maxime Ripard]

Hi Marek,

Le 22/04/2013 11:16, Marek Vasut a ?crit :
> Dear Maxime Ripard,
> 
>> The current code always registers as a 32 bits display, and uses the
>> hardware to drop the MSB of each color to abjust to the interface width
>> used by the panel.
>>
>> This results on 18 bits (and probably 16 bits display as well) in colors
>> being displayed poorly, because the MSB are obviously the most important
>> bits for each color definition.
>>
>> The default controller behaviour when using an interface width smaller
>> than the color depth is to drop the LSBs of each color, which makes more
>> sense because you lose the least important part of the color definition.
>>
>> So, to fix the colors display, just get back to the default controller
>> behaviour.
>>
>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> 
> Did you receive my latest email? Check M28EVK (imx28-m28evk.dts), it uses 18bit 
> LCD and works without this patch I think.

Thanks for the pointer. You mentionned in your other mail that it was
wired on 24bits but that the screen is actually 18 bits. I went to
search for the schematics to look at the wirings to see what could
differ, but I couldn't find any for the M28EVK. Are they publicly available?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH RESEND] video: mxsfb: Fix colors display on lower color depth

[Marek Vasut]

Dear Maxime Ripard,

> Hi Marek,
> 
> Le 22/04/2013 11:16, Marek Vasut a écrit :
> > Dear Maxime Ripard,
> > 
> >> The current code always registers as a 32 bits display, and uses the
> >> hardware to drop the MSB of each color to abjust to the interface width
> >> used by the panel.
> >> 
> >> This results on 18 bits (and probably 16 bits display as well) in colors
> >> being displayed poorly, because the MSB are obviously the most important
> >> bits for each color definition.
> >> 
> >> The default controller behaviour when using an interface width smaller
> >> than the color depth is to drop the LSBs of each color, which makes more
> >> sense because you lose the least important part of the color definition.
> >> 
> >> So, to fix the colors display, just get back to the default controller
> >> behaviour.
> >> 
> >> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > 
> > Did you receive my latest email? Check M28EVK (imx28-m28evk.dts), it uses
> > 18bit LCD and works without this patch I think.
> 
> Thanks for the pointer. You mentionned in your other mail that it was
> wired on 24bits but that the screen is actually 18 bits. I went to
> search for the schematics to look at the wirings to see what could
> differ, but I couldn't find any for the M28EVK. Are they publicly
> available?

Yes, [1] page 6 .

[1] http://www.denx-cs.de/doku/sites/default/files/M28EVK_V20_0.pdf

Best regards,
Marek Vasut

Re: [PATCH RESEND] video: mxsfb: Fix colors display on lower color depth

[Marek Vasut]

Dear Maxime Ripard,

> Hi Marek,
> 
> Le 22/04/2013 11:16, Marek Vasut a écrit :
> > Dear Maxime Ripard,
> > 
> >> The current code always registers as a 32 bits display, and uses the
> >> hardware to drop the MSB of each color to abjust to the interface width
> >> used by the panel.
> >> 
> >> This results on 18 bits (and probably 16 bits display as well) in colors
> >> being displayed poorly, because the MSB are obviously the most important
> >> bits for each color definition.
> >> 
> >> The default controller behaviour when using an interface width smaller
> >> than the color depth is to drop the LSBs of each color, which makes more
> >> sense because you lose the least important part of the color definition.
> >> 
> >> So, to fix the colors display, just get back to the default controller
> >> behaviour.
> >> 
> >> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > 
> > Did you receive my latest email? Check M28EVK (imx28-m28evk.dts), it uses
> > 18bit LCD and works without this patch I think.
> 
> Thanks for the pointer. You mentionned in your other mail that it was
> wired on 24bits but that the screen is actually 18 bits. I went to
> search for the schematics to look at the wirings to see what could
> differ, but I couldn't find any for the M28EVK. Are they publicly
> available?

Yes, [1] page 6 .

[1] http://www.denx-cs.de/doku/sites/default/files/M28EVK_V20_0.pdf

Best regards,
Marek Vasut

[PATCH RESEND] video: mxsfb: Fix colors display on lower color depth

[Marek Vasut]

Dear Maxime Ripard,

> Hi Marek,
> 
> Le 22/04/2013 11:16, Marek Vasut a ?crit :
> > Dear Maxime Ripard,
> > 
> >> The current code always registers as a 32 bits display, and uses the
> >> hardware to drop the MSB of each color to abjust to the interface width
> >> used by the panel.
> >> 
> >> This results on 18 bits (and probably 16 bits display as well) in colors
> >> being displayed poorly, because the MSB are obviously the most important
> >> bits for each color definition.
> >> 
> >> The default controller behaviour when using an interface width smaller
> >> than the color depth is to drop the LSBs of each color, which makes more
> >> sense because you lose the least important part of the color definition.
> >> 
> >> So, to fix the colors display, just get back to the default controller
> >> behaviour.
> >> 
> >> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > 
> > Did you receive my latest email? Check M28EVK (imx28-m28evk.dts), it uses
> > 18bit LCD and works without this patch I think.
> 
> Thanks for the pointer. You mentionned in your other mail that it was
> wired on 24bits but that the screen is actually 18 bits. I went to
> search for the schematics to look at the wirings to see what could
> differ, but I couldn't find any for the M28EVK. Are they publicly
> available?

Yes, [1] page 6 .

[1] http://www.denx-cs.de/doku/sites/default/files/M28EVK_V20_0.pdf

Best regards,
Marek Vasut