MXS framebuffer driver and 18bits display

MXS framebuffer driver and 18bits display

[Maxime Ripard]

Hi,

We noticed a strange bug with the 18 bits display found in the
iMX28 CFA-10049, but it should not be specific to it.

Like I said, we have a small 18-bits LCD panel. When further testing
it, it displayed horrible colors with standard jpg images used together
with tools such as fbv, applications written in Qt, etc.

Digging a bit into it, it looks like the mxsfb driver accepts devices
with 18bits interfaces.

However, in such case, the driver registers as a 32bits framebuffer
device, sets up the controller as a 24 bits display, and is actually
asking the controller to drop the 2 upper bits of each color by setting
the DATA_FORMAT_24_BIT bit in the HW_LCDIF_CTRL register.

So basically, applications feed to the framebuffer something like:

+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
| R7| R6| R5| R4| R3| R2| R1| R0| G7| G6| G5| G4| G3| G2| G1| G0| B7| B6| B5| B4| B3| B2| B1| B0|
+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+

And what is being sent to the LCD is:

+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
| R5| R4| R3| R2| R1| R0| G5| G4| G3| G2| G1| G0| B5| B4| B3| B2| B1| B0|
+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+

And thus, results in colors being horrible because the MSB are dropped.

What I believe the controller should send in theory should be :

+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
| R7| R6| R5| R4| R3| R2| G7| G6| G5| G4| G3| G2| B7| B6| B5| B4| B3| B2|
+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+

Thus dropping the 2 LSB for each color. However, it doesn't seem that
the controller can be setup that way.

We thought of several solutions for this:
  - Switching to "real" 18 bits color depth
    * It is definitely not a good solution, since it requires the
      applications to handle the 18 bits color depth case, which only a
      few of them do.
  - Using the SHIFT_NUM_BITS bits in the HW_LCDIF_CTRL register
    * It doesn't look like a good solution either, since it seems to be
      shifting the whole 24 bits, and not each color independently, so
      the result will only be worse.
  - Use a 16 bits color depth
    * It doesn't seem possible either, since the 16 bits will be packed
      on two bytes, while the controller expects the 18 bits not to be
      packed.

Do you see a solution for this?

Thanks,
Maxime

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

[PATCH] 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>
---
 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] 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>

Isn't this what M28EVK is doing already? This was last tested on 3.9-rc3 I 
think:

bits-per-pixel = <16>;
bus-width = <18>;

But it's also true we have all 24 data lines routed to the LCD.

Best regards,
Marek Vasut

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

[Shawn Guo]

Copy Sascha.

Shawn

On Thu, Apr 18, 2013 at 11:23:52AM +0200, 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.
> 
> 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>
> ---
>  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] video: mxsfb: Fix colors display on lower color depth

[Maxime Ripard]

Hi Shawn,

Le 18/04/2013 16:48, Shawn Guo a ?crit :
> Copy Sascha.

Ah, yes, sorry, I somehow got confused and thought you were the one that
wrote the driver.

I'll resend the patch.

Maxime

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

MXS framebuffer driver and 18bits display

[Shawn Guo]

Copy Sascha who is the author of the driver to see he has any comment on
that.

Shawn

On Wed, Apr 17, 2013 at 05:15:48PM +0200, Maxime Ripard wrote:
> Hi,
> 
> We noticed a strange bug with the 18 bits display found in the
> iMX28 CFA-10049, but it should not be specific to it.
> 
> Like I said, we have a small 18-bits LCD panel. When further testing
> it, it displayed horrible colors with standard jpg images used together
> with tools such as fbv, applications written in Qt, etc.
> 
> Digging a bit into it, it looks like the mxsfb driver accepts devices
> with 18bits interfaces.
> 
> However, in such case, the driver registers as a 32bits framebuffer
> device, sets up the controller as a 24 bits display, and is actually
> asking the controller to drop the 2 upper bits of each color by setting
> the DATA_FORMAT_24_BIT bit in the HW_LCDIF_CTRL register.
> 
> So basically, applications feed to the framebuffer something like:
> 
> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
> | R7| R6| R5| R4| R3| R2| R1| R0| G7| G6| G5| G4| G3| G2| G1| G0| B7| B6| B5| B4| B3| B2| B1| B0|
> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
> 
> And what is being sent to the LCD is:
> 
> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
> | R5| R4| R3| R2| R1| R0| G5| G4| G3| G2| G1| G0| B5| B4| B3| B2| B1| B0|
> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
> 
> And thus, results in colors being horrible because the MSB are dropped.
> 
> What I believe the controller should send in theory should be :
> 
> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
> | R7| R6| R5| R4| R3| R2| G7| G6| G5| G4| G3| G2| B7| B6| B5| B4| B3| B2|
> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
> 
> Thus dropping the 2 LSB for each color. However, it doesn't seem that
> the controller can be setup that way.
> 
> We thought of several solutions for this:
>   - Switching to "real" 18 bits color depth
>     * It is definitely not a good solution, since it requires the
>       applications to handle the 18 bits color depth case, which only a
>       few of them do.
>   - Using the SHIFT_NUM_BITS bits in the HW_LCDIF_CTRL register
>     * It doesn't look like a good solution either, since it seems to be
>       shifting the whole 24 bits, and not each color independently, so
>       the result will only be worse.
>   - Use a 16 bits color depth
>     * It doesn't seem possible either, since the 16 bits will be packed
>       on two bytes, while the controller expects the 18 bits not to be
>       packed.
> 
> Do you see a solution for this?
> 
> Thanks,
> Maxime
> 
> -- 
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com