mxsfb: DATA_FORMAT_24_BIT flag outputs invalid colours

mxsfb: DATA_FORMAT_24_BIT flag outputs invalid colours

[Hector Palacios]

Hello,

I'm using an i.MX28 based board with lcd connected with 18bits data bus.
My platform uses 32 bits per pixel:

	mxsfb_pdata.default_bpp = 32;
	mxsfb_pdata.ld_intf_width = STMLCDIF_18BIT;

With these settings the mxsfb.c driver sets flag DATA_FORMAT_24_BIT at HW_LCDIF_CTRL 
register in function mxsfb_set_par():

	case 32:
		dev_dbg(&host->pdev->dev, "Setting up RGB888/666 mode\n");
		ctrl |= CTRL_SET_WORD_LENGTH(3);
		switch (host->ld_intf_width) {
		case STMLCDIF_8BIT:
			dev_dbg(&host->pdev->dev,
					"Unsupported LCD bus width mapping\n");
			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;
		}

According to the manual, this flag does:
	0x0: ALL_24_BITS_VALID: Data input to the block is in 24 bpp format, such that all 
RGB 888 data is contained in 24 bits.
	0x1: DROP_UPPER_2_BITS_PER_BYTE — Data input to the block is actually RGB 18 bpp, but 
there is 1 colour per byte, hence the upper 2 bits in each byte do not contain any 
useful data, and should be dropped.

The setting of this flag is producing bad colours with true colour images (i.e. the 
Linux penguin is displayed ok, but QT applications or images displayed with fbv are not).
I believe the setting of this flag is not correct (after all, if my bpp is 32, then 
all 24bit colours are useful and dropping the upper 2 bits is a bad idea).
If I don't set it, then true colour images are displayed correctly. The only problem 
is that the Linux penguin is displayed much darker than usual (correct colours, but 
darker). Perhaps the 224 colour format of this image justifies it?

I noticed the cfa10049 platform also uses the same configuration (18 bits data bus and 
32bpp) and was wondering if true colour images are correctly displayed in this 
platform with this flag set (for example with fbv application [1]).

Regards,
-- 
Héctor Palacios

[1] http://freecode.com/projects/fbv

Re: mxsfb: DATA_FORMAT_24_BIT flag outputs invalid colours

[maxime.ripard]

Hi Hector,

On Thu, May 23, 2013 at 01:55:28PM +0200, Hector Palacios wrote:
> Hello,
> 
> I'm using an i.MX28 based board with lcd connected with 18bits data bus.
> My platform uses 32 bits per pixel:
> 
> 	mxsfb_pdata.default_bpp = 32;
> 	mxsfb_pdata.ld_intf_width = STMLCDIF_18BIT;
> 
> With these settings the mxsfb.c driver sets flag DATA_FORMAT_24_BIT
> at HW_LCDIF_CTRL register in function mxsfb_set_par():
> 
> 	case 32:
> 		dev_dbg(&host->pdev->dev, "Setting up RGB888/666 mode\n");
> 		ctrl |= CTRL_SET_WORD_LENGTH(3);
> 		switch (host->ld_intf_width) {
> 		case STMLCDIF_8BIT:
> 			dev_dbg(&host->pdev->dev,
> 					"Unsupported LCD bus width mapping\n");
> 			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;
> 		}
> 
> According to the manual, this flag does:
> 	0x0: ALL_24_BITS_VALID: Data input to the block is in 24 bpp
> format, such that all RGB 888 data is contained in 24 bits.
> 	0x1: DROP_UPPER_2_BITS_PER_BYTE — Data input to the block is
> actually RGB 18 bpp, but there is 1 colour per byte, hence the upper
> 2 bits in each byte do not contain any useful data, and should be
> dropped.
> 
> The setting of this flag is producing bad colours with true colour
> images (i.e. the Linux penguin is displayed ok, but QT applications
> or images displayed with fbv are not).
> I believe the setting of this flag is not correct (after all, if my
> bpp is 32, then all 24bit colours are useful and dropping the upper
> 2 bits is a bad idea).
> If I don't set it, then true colour images are displayed correctly.
> The only problem is that the Linux penguin is displayed much darker
> than usual (correct colours, but darker). Perhaps the 224 colour
> format of this image justifies it?
> 
> I noticed the cfa10049 platform also uses the same configuration (18
> bits data bus and 32bpp) and was wondering if true colour images are
> correctly displayed in this platform with this flag set (for example
> with fbv application [1]).

I had the exact same problem, and suggested the exact same solution a
few weeks back.

https://patchwork.kernel.org/patch/2470441/

The conclusion of that discussion what that the userspace applications
were not honouring the bitfield correctly set by the mxsfb driver, and
as such, it was not a bug in the driver.

While this is correct, I wonder, now that since we had that same problem
in a very short amount of time, if we couldn't set this behaviour
dependant of some (dt? kernel argument?) property so that one could
customise it anyway he want.

Maxime

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

Re: mxsfb: DATA_FORMAT_24_BIT flag outputs invalid colours

[Juergen Beisert]

Hi Maxime,

maxime.ripard@free-electrons.com wrote:
> On Thu, May 23, 2013 at 01:55:28PM +0200, Hector Palacios wrote:
> > I'm using an i.MX28 based board with lcd connected with 18bits data bus.
> > My platform uses 32 bits per pixel:
> >
> > 	mxsfb_pdata.default_bpp = 32;
> > 	mxsfb_pdata.ld_intf_width = STMLCDIF_18BIT;
> >
> > With these settings the mxsfb.c driver sets flag DATA_FORMAT_24_BIT
> > at HW_LCDIF_CTRL register in function mxsfb_set_par():
> >
> > 	case 32:
> > 		dev_dbg(&host->pdev->dev, "Setting up RGB888/666 mode\n");
> > 		ctrl |= CTRL_SET_WORD_LENGTH(3);
> > 		switch (host->ld_intf_width) {
> > 		case STMLCDIF_8BIT:
> > 			dev_dbg(&host->pdev->dev,
> > 					"Unsupported LCD bus width mapping\n");
> > 			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;
> > 		}
> >
> > According to the manual, this flag does:
> > 	0x0: ALL_24_BITS_VALID: Data input to the block is in 24 bpp
> > format, such that all RGB 888 data is contained in 24 bits.
> > 	0x1: DROP_UPPER_2_BITS_PER_BYTE — Data input to the block is
> > actually RGB 18 bpp, but there is 1 colour per byte, hence the upper
> > 2 bits in each byte do not contain any useful data, and should be
> > dropped.
> >
> > The setting of this flag is producing bad colours with true colour
> > images (i.e. the Linux penguin is displayed ok, but QT applications
> > or images displayed with fbv are not).
> > I believe the setting of this flag is not correct (after all, if my
> > bpp is 32, then all 24bit colours are useful and dropping the upper
> > 2 bits is a bad idea).
> > If I don't set it, then true colour images are displayed correctly.
> > The only problem is that the Linux penguin is displayed much darker
> > than usual (correct colours, but darker). Perhaps the 224 colour
> > format of this image justifies it?
> >
> > I noticed the cfa10049 platform also uses the same configuration (18
> > bits data bus and 32bpp) and was wondering if true colour images are
> > correctly displayed in this platform with this flag set (for example
> > with fbv application [1]).
>
> I had the exact same problem, and suggested the exact same solution a
> few weeks back.
>
> https://patchwork.kernel.org/patch/2470441/
>
> The conclusion of that discussion what that the userspace applications
> were not honouring the bitfield correctly set by the mxsfb driver, and
> as such, it was not a bug in the driver.
>
> While this is correct, I wonder, now that since we had that same problem
> in a very short amount of time, if we couldn't set this behaviour
> dependant of some (dt? kernel argument?) property so that one could
> customise it anyway he want.
>
> Maxime

i.MX2[3|8]    LCD1       LCD2       LCD3
              24bit      18bit      18bit
--------------------------------------------
LCD_D0         B0         B0         --
LCD_D1         B1         B1         --
LCD_D2         B2         B2         B0
LCD_D3         B3         B3         B1
LCD_D4         B4         B4         B2
LCD_D5         B5         B5         B3
LCD_D6         B6         G0         B4
LCD_D7         B7         G1         B5

LCD_D8         G0         G2         --
LCD_D9         G1         G3         --
LCD_D10        G2         G4         G0
LCD_D11        G3         G5         G1
LCD_D12        G4         R0         G2
LCD_D13        G5         R1         G3
LCD_D14        G6         R2         G4
LCD_D15        G7         R3         G5

LCD_D16        R0         R4         --
LCD_D17        R1         R5         --
LCD_D18        R2                    R0
LCD_D19        R3                    R1
LCD_D20        R4                    R2
LCD_D21        R5                    R3
LCD_D22        R6                    R4
LCD_D23        R7                    R5

Is your display connected like LCD2 or LCD3? LCD3 must still handled like a 24 
bit display shown in LCD1, while only the LCD2-case is the "24 bit to 18 bit 
mapping" case.

At least my current tests with an i.MX23 and a connection like LCD2 are 
working here with a Qt application. Qt honours the pixel bitfield 
description. And I'm using the "bits-per-pixel = <32>" and "bus-width = <18>" 
entries in the device tree.

Regards,
Juergen

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

Re: mxsfb: DATA_FORMAT_24_BIT flag outputs invalid colours

[Hector Palacios]

Hi Juergen,

On 05/23/2013 03:31 PM, Juergen Beisert wrote:
> Hi Maxime,
>
> maxime.ripard@free-electrons.com wrote:
>> On Thu, May 23, 2013 at 01:55:28PM +0200, Hector Palacios wrote:
>>> I'm using an i.MX28 based board with lcd connected with 18bits data bus.
>>> My platform uses 32 bits per pixel:
>>>
>>> 	mxsfb_pdata.default_bpp = 32;
>>> 	mxsfb_pdata.ld_intf_width = STMLCDIF_18BIT;
>>>
>>> With these settings the mxsfb.c driver sets flag DATA_FORMAT_24_BIT
>>> at HW_LCDIF_CTRL register in function mxsfb_set_par():
>>>
>>> 	case 32:
>>> 		dev_dbg(&host->pdev->dev, "Setting up RGB888/666 mode\n");
>>> 		ctrl |= CTRL_SET_WORD_LENGTH(3);
>>> 		switch (host->ld_intf_width) {
>>> 		case STMLCDIF_8BIT:
>>> 			dev_dbg(&host->pdev->dev,
>>> 					"Unsupported LCD bus width mapping\n");
>>> 			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;
>>> 		}
>>>
>>> According to the manual, this flag does:
>>> 	0x0: ALL_24_BITS_VALID: Data input to the block is in 24 bpp
>>> format, such that all RGB 888 data is contained in 24 bits.
>>> 	0x1: DROP_UPPER_2_BITS_PER_BYTE — Data input to the block is
>>> actually RGB 18 bpp, but there is 1 colour per byte, hence the upper
>>> 2 bits in each byte do not contain any useful data, and should be
>>> dropped.
>>>
>>> The setting of this flag is producing bad colours with true colour
>>> images (i.e. the Linux penguin is displayed ok, but QT applications
>>> or images displayed with fbv are not).
>>> I believe the setting of this flag is not correct (after all, if my
>>> bpp is 32, then all 24bit colours are useful and dropping the upper
>>> 2 bits is a bad idea).
>>> If I don't set it, then true colour images are displayed correctly.
>>> The only problem is that the Linux penguin is displayed much darker
>>> than usual (correct colours, but darker). Perhaps the 224 colour
>>> format of this image justifies it?
>>>
>>> I noticed the cfa10049 platform also uses the same configuration (18
>>> bits data bus and 32bpp) and was wondering if true colour images are
>>> correctly displayed in this platform with this flag set (for example
>>> with fbv application [1]).
>>
>> I had the exact same problem, and suggested the exact same solution a
>> few weeks back.
>>
>> https://patchwork.kernel.org/patch/2470441/
>>
>> The conclusion of that discussion what that the userspace applications
>> were not honouring the bitfield correctly set by the mxsfb driver, and
>> as such, it was not a bug in the driver.
>>
>> While this is correct, I wonder, now that since we had that same problem
>> in a very short amount of time, if we couldn't set this behaviour
>> dependant of some (dt? kernel argument?) property so that one could
>> customise it anyway he want.
>>
>> Maxime
>
> i.MX2[3|8]    LCD1       LCD2       LCD3
>                24bit      18bit      18bit
> --------------------------------------------
> LCD_D0         B0         B0         --
> LCD_D1         B1         B1         --
> LCD_D2         B2         B2         B0
> LCD_D3         B3         B3         B1
> LCD_D4         B4         B4         B2
> LCD_D5         B5         B5         B3
> LCD_D6         B6         G0         B4
> LCD_D7         B7         G1         B5
>
> LCD_D8         G0         G2         --
> LCD_D9         G1         G3         --
> LCD_D10        G2         G4         G0
> LCD_D11        G3         G5         G1
> LCD_D12        G4         R0         G2
> LCD_D13        G5         R1         G3
> LCD_D14        G6         R2         G4
> LCD_D15        G7         R3         G5
>
> LCD_D16        R0         R4         --
> LCD_D17        R1         R5         --
> LCD_D18        R2                    R0
> LCD_D19        R3                    R1
> LCD_D20        R4                    R2
> LCD_D21        R5                    R3
> LCD_D22        R6                    R4
> LCD_D23        R7                    R5
>
> Is your display connected like LCD2 or LCD3? LCD3 must still handled like a 24
> bit display shown in LCD1, while only the LCD2-case is the "24 bit to 18 bit
> mapping" case.
>
> At least my current tests with an i.MX23 and a connection like LCD2 are
> working here with a Qt application. Qt honours the pixel bitfield
> description. And I'm using the "bits-per-pixel = <32>" and "bus-width = <18>"
> entries in the device tree.

I have a 24bit LCD display but my connection to it is done at 18bits data width.
Represented below as LCD4.
NOTE: In my LCD4 column, notation Rx/Gx/Bx represent the color bit in memory as well 
as the display data line.
Since we use 32bpp each channel has 8 bits (R7..R0, etc.).
I understand that you have an 18bit display and that your notation in LCD2 column 
represents the display data lines, not the color bit indexes in memory.

i.MX2[3|8]    LCD1       LCD2       LCD3	LCD4
               24bit      18bit      18bit	24bit connected at 18bit
-------------------------------------------------------
LCD_D0         B0         B0         --		B2
LCD_D1         B1         B1         --		B3
LCD_D2         B2         B2         B0		B4
LCD_D3         B3         B3         B1		B5
LCD_D4         B4         B4         B2		B6
LCD_D5         B5         B5         B3		B7
LCD_D6         B6         G0         B4		G2
LCD_D7         B7         G1         B5		G3

LCD_D8         G0         G2         --		G4
LCD_D9         G1         G3         --		G5
LCD_D10        G2         G4         G0		G6
LCD_D11        G3         G5         G1		G7
LCD_D12        G4         R0         G2		R2
LCD_D13        G5         R1         G3		R3
LCD_D14        G6         R2         G4		R4
LCD_D15        G7         R3         G5		R5

LCD_D16        R0         R4         --		R6
LCD_D17        R1         R5         --		R7
LCD_D18        R2                    R0
LCD_D19        R3                    R1
LCD_D20        R4                    R2
LCD_D21        R5                    R3
LCD_D22        R6                    R4
LCD_D23        R7                    R5

For 32bpp (RGB888) and 18bit data bus I would expect the LCD controller to take the 
six *most significant* bits [7..2] from each color byte out to the LCD data bus 
(LCD_D17..D0) in the order depicted in my LCD4 column.

I'm not sure what the DATA_FORMAT_24_BIT flag is doing, but dropping the two most 
significant bits of color in memory doesn't seem to be a good idea unless (maybe) 
color is in 18bpp. Previous kernels did not even touch this flag.

Does the following patch make sense?

diff --git a/drivers/video/mxsfb.c b/drivers/video/mxsfb.c
index b1c1a80..bb0a4e1 100644
--- a/drivers/video/mxsfb.c
+++ b/drivers/video/mxsfb.c
@@ -298,9 +298,6 @@ static int mxsfb_check_var(struct fb_var_screeninfo *var,
                         break;
                 case STMLCDIF_16BIT:
                 case STMLCDIF_18BIT:
-                       /* 24 bit to 18 bit mapping */
-                       rgb = def_rgb666;
-                       break;
                 case STMLCDIF_24BIT:
                         /* real 24 bit */
                         rgb = def_rgb888;
@@ -424,11 +421,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;

The setting of def_rgb666 for a 32bpp color depth does not make sense to me because 
the color in memory is really rgb888.

With this patch, my true color images are displayed ok and so does the penguin logo.
I don't know however how other displays connections at 18bit will do.

Regards,
-- 
Héctor Palacios

Re: mxsfb: DATA_FORMAT_24_BIT flag outputs invalid colours

[Juergen Beisert]

Hector Palacios wrote:
> Hi Juergen,
>
> On 05/23/2013 03:31 PM, Juergen Beisert wrote:
> > Hi Maxime,
> >
> > maxime.ripard@free-electrons.com wrote:
> >> On Thu, May 23, 2013 at 01:55:28PM +0200, Hector Palacios wrote:
> >>> I'm using an i.MX28 based board with lcd connected with 18bits data
> >>> bus. My platform uses 32 bits per pixel:
> >>>
> >>> 	mxsfb_pdata.default_bpp = 32;
> >>> 	mxsfb_pdata.ld_intf_width = STMLCDIF_18BIT;
> >>>
> >>> With these settings the mxsfb.c driver sets flag DATA_FORMAT_24_BIT
> >>> at HW_LCDIF_CTRL register in function mxsfb_set_par():
> >>>
> >>> 	case 32:
> >>> 		dev_dbg(&host->pdev->dev, "Setting up RGB888/666 mode\n");
> >>> 		ctrl |= CTRL_SET_WORD_LENGTH(3);
> >>> 		switch (host->ld_intf_width) {
> >>> 		case STMLCDIF_8BIT:
> >>> 			dev_dbg(&host->pdev->dev,
> >>> 					"Unsupported LCD bus width mapping\n");
> >>> 			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;
> >>> 		}
> >>>
> >>> According to the manual, this flag does:
> >>> 	0x0: ALL_24_BITS_VALID: Data input to the block is in 24 bpp
> >>> format, such that all RGB 888 data is contained in 24 bits.
> >>> 	0x1: DROP_UPPER_2_BITS_PER_BYTE — Data input to the block is
> >>> actually RGB 18 bpp, but there is 1 colour per byte, hence the upper
> >>> 2 bits in each byte do not contain any useful data, and should be
> >>> dropped.
> >>>
> >>> The setting of this flag is producing bad colours with true colour
> >>> images (i.e. the Linux penguin is displayed ok, but QT applications
> >>> or images displayed with fbv are not).
> >>> I believe the setting of this flag is not correct (after all, if my
> >>> bpp is 32, then all 24bit colours are useful and dropping the upper
> >>> 2 bits is a bad idea).
> >>> If I don't set it, then true colour images are displayed correctly.
> >>> The only problem is that the Linux penguin is displayed much darker
> >>> than usual (correct colours, but darker). Perhaps the 224 colour
> >>> format of this image justifies it?
> >>>
> >>> I noticed the cfa10049 platform also uses the same configuration (18
> >>> bits data bus and 32bpp) and was wondering if true colour images are
> >>> correctly displayed in this platform with this flag set (for example
> >>> with fbv application [1]).
> >>
> >> I had the exact same problem, and suggested the exact same solution a
> >> few weeks back.
> >>
> >> https://patchwork.kernel.org/patch/2470441/
> >>
> >> The conclusion of that discussion what that the userspace applications
> >> were not honouring the bitfield correctly set by the mxsfb driver, and
> >> as such, it was not a bug in the driver.
> >>
> >> While this is correct, I wonder, now that since we had that same problem
> >> in a very short amount of time, if we couldn't set this behaviour
> >> dependant of some (dt? kernel argument?) property so that one could
> >> customise it anyway he want.
> >>
> >> Maxime
> >
> > i.MX2[3|8]    LCD1       LCD2       LCD3
> >                24bit      18bit      18bit
> > --------------------------------------------
> > LCD_D0         B0         B0         --
> > LCD_D1         B1         B1         --
> > LCD_D2         B2         B2         B0
> > LCD_D3         B3         B3         B1
> > LCD_D4         B4         B4         B2
> > LCD_D5         B5         B5         B3
> > LCD_D6         B6         G0         B4
> > LCD_D7         B7         G1         B5
> >
> > LCD_D8         G0         G2         --
> > LCD_D9         G1         G3         --
> > LCD_D10        G2         G4         G0
> > LCD_D11        G3         G5         G1
> > LCD_D12        G4         R0         G2
> > LCD_D13        G5         R1         G3
> > LCD_D14        G6         R2         G4
> > LCD_D15        G7         R3         G5
> >
> > LCD_D16        R0         R4         --
> > LCD_D17        R1         R5         --
> > LCD_D18        R2                    R0
> > LCD_D19        R3                    R1
> > LCD_D20        R4                    R2
> > LCD_D21        R5                    R3
> > LCD_D22        R6                    R4
> > LCD_D23        R7                    R5
> >
> > Is your display connected like LCD2 or LCD3? LCD3 must still handled like
> > a 24 bit display shown in LCD1, while only the LCD2-case is the "24 bit
> > to 18 bit mapping" case.
> >
> > At least my current tests with an i.MX23 and a connection like LCD2 are
> > working here with a Qt application. Qt honours the pixel bitfield
> > description. And I'm using the "bits-per-pixel = <32>" and "bus-width =
> > <18>" entries in the device tree.
>
> I have a 24bit LCD display but my connection to it is done at 18bits data
> width. Represented below as LCD4.
> NOTE: In my LCD4 column, notation Rx/Gx/Bx represent the color bit in
> memory as well as the display data line.
> Since we use 32bpp each channel has 8 bits (R7..R0, etc.).
> I understand that you have an 18bit display and that your notation in LCD2
> column represents the display data lines, not the color bit indexes in
> memory.
>
> i.MX2[3|8]    LCD1       LCD2       LCD3	LCD4
>               24bit      18bit      18bit	24bit connected at 18bit
> -------------------------------------------------------
> LCD_D0         B0         B0         --		B2
> LCD_D1         B1         B1         --		B3
> LCD_D2         B2         B2         B0		B4
> LCD_D3         B3         B3         B1		B5
> LCD_D4         B4         B4         B2		B6
> LCD_D5         B5         B5         B3		B7
> LCD_D6         B6         G0         B4		G2
> LCD_D7         B7         G1         B5		G3
>
> LCD_D8         G0         G2         --		G4
> LCD_D9         G1         G3         --		G5
> LCD_D10        G2         G4         G0		G6
> LCD_D11        G3         G5         G1		G7
> LCD_D12        G4         R0         G2		R2
> LCD_D13        G5         R1         G3		R3
> LCD_D14        G6         R2         G4		R4
> LCD_D15        G7         R3         G5		R5
>
> LCD_D16        R0         R4         --		R6
> LCD_D17        R1         R5         --		R7
> LCD_D18        R2                    R0
> LCD_D19        R3                    R1
> LCD_D20        R4                    R2
> LCD_D21        R5                    R3
> LCD_D22        R6                    R4
> LCD_D23        R7                    R5
>
> For 32bpp (RGB888) and 18bit data bus I would expect the LCD controller to
> take the six *most significant* bits [7..2] from each color byte out to the
> LCD data bus (LCD_D17..D0) in the order depicted in my LCD4 column.
>
> I'm not sure what the DATA_FORMAT_24_BIT flag is doing, but dropping the
> two most significant bits of color in memory doesn't seem to be a good idea
> unless (maybe) color is in 18bpp. Previous kernels did not even touch this
> flag.
>
> Does the following patch make sense?
>
> diff --git a/drivers/video/mxsfb.c b/drivers/video/mxsfb.c
> index b1c1a80..bb0a4e1 100644
> --- a/drivers/video/mxsfb.c
> +++ b/drivers/video/mxsfb.c
> @@ -298,9 +298,6 @@ static int mxsfb_check_var(struct fb_var_screeninfo
> *var, break;
>                  case STMLCDIF_16BIT:
>                  case STMLCDIF_18BIT:
> -                       /* 24 bit to 18 bit mapping */
> -                       rgb = def_rgb666;
> -                       break;
>                  case STMLCDIF_24BIT:
>                          /* real 24 bit */
>                          rgb = def_rgb888;
> @@ -424,11 +421,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;
>
> The setting of def_rgb666 for a 32bpp color depth does not make sense to me
> because the color in memory is really rgb888.
>
> With this patch, my true color images are displayed ok and so does the
> penguin logo. I don't know however how other displays connections at 18bit
> will do.

Your 24 bit display is connected like a regular 18 bit display. So they should 
work in the same way with the same settings. You *must* always skip two data 
bits from the memory RGB888 to form a RGB666 value.

I just did some measurement here with my i.MX23 based hardware.

I'm using 32 bits per pixel and the 18 bit display interface (LCD_D[0..17]).

And surprise, surprise: the i.MX23 *always* maps the 24 bit input data to the 
18 bit interface. And what does the DATA_FORMAT_24_BIT aka. CTRL_DF24 do???
Just simple: it seems its meaning changes with the interface width. The 
documentation says:

  0 = all 24 bits are valid
  1 = drop upper 2 bits per byte

This text seems valid for a real 24 bit display (but I cannot test it). From 
my measurement its meaning changes when used with an 18 bit display to:

  0 = drop lower 2 bits per byte
  1 = drop upper 2 bits per byte

When this bit is 0:

      red     green    blue
   10000001|00001111|10001111 (memory layout)

   100000..|000011..|100011.. (at the display)

When this bit is 1:

      red     green    blue
   10000001|00001111|10001111 (memory layout)

   ..000001|..001111|..001111 (at the display)

Hope it does help you.

Regards,
Juergen

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

Re: mxsfb: DATA_FORMAT_24_BIT flag outputs invalid colours

[Hector Palacios]

Hi Juergen,

On 05/24/2013 12:28 PM, Juergen Beisert wrote:
> Hector Palacios wrote:
>> Hi Juergen,
>>
>> On 05/23/2013 03:31 PM, Juergen Beisert wrote:
>>> Hi Maxime,
>>>
>>> maxime.ripard@free-electrons.com wrote:
>>>> On Thu, May 23, 2013 at 01:55:28PM +0200, Hector Palacios wrote:
>>>>> I'm using an i.MX28 based board with lcd connected with 18bits data
>>>>> bus. My platform uses 32 bits per pixel:
>>>>>
>>>>> 	mxsfb_pdata.default_bpp = 32;
>>>>> 	mxsfb_pdata.ld_intf_width = STMLCDIF_18BIT;
>>>>>
>>>>> With these settings the mxsfb.c driver sets flag DATA_FORMAT_24_BIT
>>>>> at HW_LCDIF_CTRL register in function mxsfb_set_par():
>>>>>
>>>>> 	case 32:
>>>>> 		dev_dbg(&host->pdev->dev, "Setting up RGB888/666 mode\n");
>>>>> 		ctrl |= CTRL_SET_WORD_LENGTH(3);
>>>>> 		switch (host->ld_intf_width) {
>>>>> 		case STMLCDIF_8BIT:
>>>>> 			dev_dbg(&host->pdev->dev,
>>>>> 					"Unsupported LCD bus width mapping\n");
>>>>> 			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;
>>>>> 		}
>>>>>
>>>>> According to the manual, this flag does:
>>>>> 	0x0: ALL_24_BITS_VALID: Data input to the block is in 24 bpp
>>>>> format, such that all RGB 888 data is contained in 24 bits.
>>>>> 	0x1: DROP_UPPER_2_BITS_PER_BYTE — Data input to the block is
>>>>> actually RGB 18 bpp, but there is 1 colour per byte, hence the upper
>>>>> 2 bits in each byte do not contain any useful data, and should be
>>>>> dropped.
>>>>>
>>>>> The setting of this flag is producing bad colours with true colour
>>>>> images (i.e. the Linux penguin is displayed ok, but QT applications
>>>>> or images displayed with fbv are not).
>>>>> I believe the setting of this flag is not correct (after all, if my
>>>>> bpp is 32, then all 24bit colours are useful and dropping the upper
>>>>> 2 bits is a bad idea).
>>>>> If I don't set it, then true colour images are displayed correctly.
>>>>> The only problem is that the Linux penguin is displayed much darker
>>>>> than usual (correct colours, but darker). Perhaps the 224 colour
>>>>> format of this image justifies it?
>>>>>
>>>>> I noticed the cfa10049 platform also uses the same configuration (18
>>>>> bits data bus and 32bpp) and was wondering if true colour images are
>>>>> correctly displayed in this platform with this flag set (for example
>>>>> with fbv application [1]).
>>>>
>>>> I had the exact same problem, and suggested the exact same solution a
>>>> few weeks back.
>>>>
>>>> https://patchwork.kernel.org/patch/2470441/
>>>>
>>>> The conclusion of that discussion what that the userspace applications
>>>> were not honouring the bitfield correctly set by the mxsfb driver, and
>>>> as such, it was not a bug in the driver.
>>>>
>>>> While this is correct, I wonder, now that since we had that same problem
>>>> in a very short amount of time, if we couldn't set this behaviour
>>>> dependant of some (dt? kernel argument?) property so that one could
>>>> customise it anyway he want.
>>>>
>>>> Maxime
>>>
>>> i.MX2[3|8]    LCD1       LCD2       LCD3
>>>                 24bit      18bit      18bit
>>> --------------------------------------------
>>> LCD_D0         B0         B0         --
>>> LCD_D1         B1         B1         --
>>> LCD_D2         B2         B2         B0
>>> LCD_D3         B3         B3         B1
>>> LCD_D4         B4         B4         B2
>>> LCD_D5         B5         B5         B3
>>> LCD_D6         B6         G0         B4
>>> LCD_D7         B7         G1         B5
>>>
>>> LCD_D8         G0         G2         --
>>> LCD_D9         G1         G3         --
>>> LCD_D10        G2         G4         G0
>>> LCD_D11        G3         G5         G1
>>> LCD_D12        G4         R0         G2
>>> LCD_D13        G5         R1         G3
>>> LCD_D14        G6         R2         G4
>>> LCD_D15        G7         R3         G5
>>>
>>> LCD_D16        R0         R4         --
>>> LCD_D17        R1         R5         --
>>> LCD_D18        R2                    R0
>>> LCD_D19        R3                    R1
>>> LCD_D20        R4                    R2
>>> LCD_D21        R5                    R3
>>> LCD_D22        R6                    R4
>>> LCD_D23        R7                    R5
>>>
>>> Is your display connected like LCD2 or LCD3? LCD3 must still handled like
>>> a 24 bit display shown in LCD1, while only the LCD2-case is the "24 bit
>>> to 18 bit mapping" case.
>>>
>>> At least my current tests with an i.MX23 and a connection like LCD2 are
>>> working here with a Qt application. Qt honours the pixel bitfield
>>> description. And I'm using the "bits-per-pixel = <32>" and "bus-width =
>>> <18>" entries in the device tree.
>>
>> I have a 24bit LCD display but my connection to it is done at 18bits data
>> width. Represented below as LCD4.
>> NOTE: In my LCD4 column, notation Rx/Gx/Bx represent the color bit in
>> memory as well as the display data line.
>> Since we use 32bpp each channel has 8 bits (R7..R0, etc.).
>> I understand that you have an 18bit display and that your notation in LCD2
>> column represents the display data lines, not the color bit indexes in
>> memory.
>>
>> i.MX2[3|8]    LCD1       LCD2       LCD3	LCD4
>>                24bit      18bit      18bit	24bit connected at 18bit
>> -------------------------------------------------------
>> LCD_D0         B0         B0         --		B2
>> LCD_D1         B1         B1         --		B3
>> LCD_D2         B2         B2         B0		B4
>> LCD_D3         B3         B3         B1		B5
>> LCD_D4         B4         B4         B2		B6
>> LCD_D5         B5         B5         B3		B7
>> LCD_D6         B6         G0         B4		G2
>> LCD_D7         B7         G1         B5		G3
>>
>> LCD_D8         G0         G2         --		G4
>> LCD_D9         G1         G3         --		G5
>> LCD_D10        G2         G4         G0		G6
>> LCD_D11        G3         G5         G1		G7
>> LCD_D12        G4         R0         G2		R2
>> LCD_D13        G5         R1         G3		R3
>> LCD_D14        G6         R2         G4		R4
>> LCD_D15        G7         R3         G5		R5
>>
>> LCD_D16        R0         R4         --		R6
>> LCD_D17        R1         R5         --		R7
>> LCD_D18        R2                    R0
>> LCD_D19        R3                    R1
>> LCD_D20        R4                    R2
>> LCD_D21        R5                    R3
>> LCD_D22        R6                    R4
>> LCD_D23        R7                    R5
>>
>> For 32bpp (RGB888) and 18bit data bus I would expect the LCD controller to
>> take the six *most significant* bits [7..2] from each color byte out to the
>> LCD data bus (LCD_D17..D0) in the order depicted in my LCD4 column.
>>
>> I'm not sure what the DATA_FORMAT_24_BIT flag is doing, but dropping the
>> two most significant bits of color in memory doesn't seem to be a good idea
>> unless (maybe) color is in 18bpp. Previous kernels did not even touch this
>> flag.
>>
>> Does the following patch make sense?
>>
>> diff --git a/drivers/video/mxsfb.c b/drivers/video/mxsfb.c
>> index b1c1a80..bb0a4e1 100644
>> --- a/drivers/video/mxsfb.c
>> +++ b/drivers/video/mxsfb.c
>> @@ -298,9 +298,6 @@ static int mxsfb_check_var(struct fb_var_screeninfo
>> *var, break;
>>                   case STMLCDIF_16BIT:
>>                   case STMLCDIF_18BIT:
>> -                       /* 24 bit to 18 bit mapping */
>> -                       rgb = def_rgb666;
>> -                       break;
>>                   case STMLCDIF_24BIT:
>>                           /* real 24 bit */
>>                           rgb = def_rgb888;
>> @@ -424,11 +421,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;
>>
>> The setting of def_rgb666 for a 32bpp color depth does not make sense to me
>> because the color in memory is really rgb888.
>>
>> With this patch, my true color images are displayed ok and so does the
>> penguin logo. I don't know however how other displays connections at 18bit
>> will do.
>
> Your 24 bit display is connected like a regular 18 bit display. So they should
> work in the same way with the same settings. You *must* always skip two data
> bits from the memory RGB888 to form a RGB666 value.
>
> I just did some measurement here with my i.MX23 based hardware.
>
> I'm using 32 bits per pixel and the 18 bit display interface (LCD_D[0..17]).
>
> And surprise, surprise: the i.MX23 *always* maps the 24 bit input data to the
> 18 bit interface. And what does the DATA_FORMAT_24_BIT aka. CTRL_DF24 do???
> Just simple: it seems its meaning changes with the interface width. The
> documentation says:
>
>    0 = all 24 bits are valid
>    1 = drop upper 2 bits per byte
>
> This text seems valid for a real 24 bit display (but I cannot test it). From
> my measurement its meaning changes when used with an 18 bit display to:
>
>    0 = drop lower 2 bits per byte
>    1 = drop upper 2 bits per byte
>
> When this bit is 0:
>
>        red     green    blue
>     10000001|00001111|10001111 (memory layout)
>
>     100000..|000011..|100011.. (at the display)
>
> When this bit is 1:
>
>        red     green    blue
>     10000001|00001111|10001111 (memory layout)
>
>     ..000001|..001111|..001111 (at the display)

And how can the setting of this flag be useful?
In the example above, you had a red component of 0x81 (half way through the scale), 
which is converted to a 0x01 (almost black) at the display. Isn't this wrong?

This only looks right to me if you set a 18bpp but even in that case masking the upper 
bits isn't really needed at all.

Regards,
-- 
Héctor Palacios

Re: mxsfb: DATA_FORMAT_24_BIT flag outputs invalid colours

[Juergen Beisert]

Hector Palacios wrote:
> Hi Juergen,
>
> On 05/24/2013 12:28 PM, Juergen Beisert wrote:
> > Hector Palacios wrote:
> >> Hi Juergen,
> >>
> >> On 05/23/2013 03:31 PM, Juergen Beisert wrote:
> >>> Hi Maxime,
> >>>
> >>> maxime.ripard@free-electrons.com wrote:
> >>>> On Thu, May 23, 2013 at 01:55:28PM +0200, Hector Palacios wrote:
> >>>>> I'm using an i.MX28 based board with lcd connected with 18bits data
> >>>>> bus. My platform uses 32 bits per pixel:
> >>>>>
> >>>>> 	mxsfb_pdata.default_bpp = 32;
> >>>>> 	mxsfb_pdata.ld_intf_width = STMLCDIF_18BIT;
> >>>>>
> >>>>> With these settings the mxsfb.c driver sets flag DATA_FORMAT_24_BIT
> >>>>> at HW_LCDIF_CTRL register in function mxsfb_set_par():
> >>>>>
> >>>>> 	case 32:
> >>>>> 		dev_dbg(&host->pdev->dev, "Setting up RGB888/666 mode\n");
> >>>>> 		ctrl |= CTRL_SET_WORD_LENGTH(3);
> >>>>> 		switch (host->ld_intf_width) {
> >>>>> 		case STMLCDIF_8BIT:
> >>>>> 			dev_dbg(&host->pdev->dev,
> >>>>> 					"Unsupported LCD bus width mapping\n");
> >>>>> 			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;
> >>>>> 		}
> >>>>>
> >>>>> According to the manual, this flag does:
> >>>>> 	0x0: ALL_24_BITS_VALID: Data input to the block is in 24 bpp
> >>>>> format, such that all RGB 888 data is contained in 24 bits.
> >>>>> 	0x1: DROP_UPPER_2_BITS_PER_BYTE — Data input to the block is
> >>>>> actually RGB 18 bpp, but there is 1 colour per byte, hence the upper
> >>>>> 2 bits in each byte do not contain any useful data, and should be
> >>>>> dropped.
> >>>>>
> >>>>> The setting of this flag is producing bad colours with true colour
> >>>>> images (i.e. the Linux penguin is displayed ok, but QT applications
> >>>>> or images displayed with fbv are not).
> >>>>> I believe the setting of this flag is not correct (after all, if my
> >>>>> bpp is 32, then all 24bit colours are useful and dropping the upper
> >>>>> 2 bits is a bad idea).
> >>>>> If I don't set it, then true colour images are displayed correctly.
> >>>>> The only problem is that the Linux penguin is displayed much darker
> >>>>> than usual (correct colours, but darker). Perhaps the 224 colour
> >>>>> format of this image justifies it?
> >>>>>
> >>>>> I noticed the cfa10049 platform also uses the same configuration (18
> >>>>> bits data bus and 32bpp) and was wondering if true colour images are
> >>>>> correctly displayed in this platform with this flag set (for example
> >>>>> with fbv application [1]).
> >>>>
> >>>> I had the exact same problem, and suggested the exact same solution a
> >>>> few weeks back.
> >>>>
> >>>> https://patchwork.kernel.org/patch/2470441/
> >>>>
> >>>> The conclusion of that discussion what that the userspace applications
> >>>> were not honouring the bitfield correctly set by the mxsfb driver, and
> >>>> as such, it was not a bug in the driver.
> >>>>
> >>>> While this is correct, I wonder, now that since we had that same
> >>>> problem in a very short amount of time, if we couldn't set this
> >>>> behaviour dependant of some (dt? kernel argument?) property so that
> >>>> one could customise it anyway he want.
> >>>>
> >>>> Maxime
> >>>
> >>> i.MX2[3|8]    LCD1       LCD2       LCD3
> >>>                 24bit      18bit      18bit
> >>> --------------------------------------------
> >>> LCD_D0         B0         B0         --
> >>> LCD_D1         B1         B1         --
> >>> LCD_D2         B2         B2         B0
> >>> LCD_D3         B3         B3         B1
> >>> LCD_D4         B4         B4         B2
> >>> LCD_D5         B5         B5         B3
> >>> LCD_D6         B6         G0         B4
> >>> LCD_D7         B7         G1         B5
> >>>
> >>> LCD_D8         G0         G2         --
> >>> LCD_D9         G1         G3         --
> >>> LCD_D10        G2         G4         G0
> >>> LCD_D11        G3         G5         G1
> >>> LCD_D12        G4         R0         G2
> >>> LCD_D13        G5         R1         G3
> >>> LCD_D14        G6         R2         G4
> >>> LCD_D15        G7         R3         G5
> >>>
> >>> LCD_D16        R0         R4         --
> >>> LCD_D17        R1         R5         --
> >>> LCD_D18        R2                    R0
> >>> LCD_D19        R3                    R1
> >>> LCD_D20        R4                    R2
> >>> LCD_D21        R5                    R3
> >>> LCD_D22        R6                    R4
> >>> LCD_D23        R7                    R5
> >>>
> >>> Is your display connected like LCD2 or LCD3? LCD3 must still handled
> >>> like a 24 bit display shown in LCD1, while only the LCD2-case is the
> >>> "24 bit to 18 bit mapping" case.
> >>>
> >>> At least my current tests with an i.MX23 and a connection like LCD2 are
> >>> working here with a Qt application. Qt honours the pixel bitfield
> >>> description. And I'm using the "bits-per-pixel = <32>" and "bus-width =
> >>> <18>" entries in the device tree.
> >>
> >> I have a 24bit LCD display but my connection to it is done at 18bits
> >> data width. Represented below as LCD4.
> >> NOTE: In my LCD4 column, notation Rx/Gx/Bx represent the color bit in
> >> memory as well as the display data line.
> >> Since we use 32bpp each channel has 8 bits (R7..R0, etc.).
> >> I understand that you have an 18bit display and that your notation in
> >> LCD2 column represents the display data lines, not the color bit indexes
> >> in memory.
> >>
> >> i.MX2[3|8]    LCD1       LCD2       LCD3	LCD4
> >>                24bit      18bit      18bit	24bit connected at 18bit
> >> -------------------------------------------------------
> >> LCD_D0         B0         B0         --		B2
> >> LCD_D1         B1         B1         --		B3
> >> LCD_D2         B2         B2         B0		B4
> >> LCD_D3         B3         B3         B1		B5
> >> LCD_D4         B4         B4         B2		B6
> >> LCD_D5         B5         B5         B3		B7
> >> LCD_D6         B6         G0         B4		G2
> >> LCD_D7         B7         G1         B5		G3
> >>
> >> LCD_D8         G0         G2         --		G4
> >> LCD_D9         G1         G3         --		G5
> >> LCD_D10        G2         G4         G0		G6
> >> LCD_D11        G3         G5         G1		G7
> >> LCD_D12        G4         R0         G2		R2
> >> LCD_D13        G5         R1         G3		R3
> >> LCD_D14        G6         R2         G4		R4
> >> LCD_D15        G7         R3         G5		R5
> >>
> >> LCD_D16        R0         R4         --		R6
> >> LCD_D17        R1         R5         --		R7
> >> LCD_D18        R2                    R0
> >> LCD_D19        R3                    R1
> >> LCD_D20        R4                    R2
> >> LCD_D21        R5                    R3
> >> LCD_D22        R6                    R4
> >> LCD_D23        R7                    R5
> >>
> >> For 32bpp (RGB888) and 18bit data bus I would expect the LCD controller
> >> to take the six *most significant* bits [7..2] from each color byte out
> >> to the LCD data bus (LCD_D17..D0) in the order depicted in my LCD4
> >> column.
> >>
> >> I'm not sure what the DATA_FORMAT_24_BIT flag is doing, but dropping the
> >> two most significant bits of color in memory doesn't seem to be a good
> >> idea unless (maybe) color is in 18bpp. Previous kernels did not even
> >> touch this flag.
> >>
> >> Does the following patch make sense?
> >>
> >> diff --git a/drivers/video/mxsfb.c b/drivers/video/mxsfb.c
> >> index b1c1a80..bb0a4e1 100644
> >> --- a/drivers/video/mxsfb.c
> >> +++ b/drivers/video/mxsfb.c
> >> @@ -298,9 +298,6 @@ static int mxsfb_check_var(struct fb_var_screeninfo
> >> *var, break;
> >>                   case STMLCDIF_16BIT:
> >>                   case STMLCDIF_18BIT:
> >> -                       /* 24 bit to 18 bit mapping */
> >> -                       rgb = def_rgb666;
> >> -                       break;
> >>                   case STMLCDIF_24BIT:
> >>                           /* real 24 bit */
> >>                           rgb = def_rgb888;
> >> @@ -424,11 +421,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;
> >>
> >> The setting of def_rgb666 for a 32bpp color depth does not make sense to
> >> me because the color in memory is really rgb888.
> >>
> >> With this patch, my true color images are displayed ok and so does the
> >> penguin logo. I don't know however how other displays connections at
> >> 18bit will do.
> >
> > Your 24 bit display is connected like a regular 18 bit display. So they
> > should work in the same way with the same settings. You *must* always
> > skip two data bits from the memory RGB888 to form a RGB666 value.
> >
> > I just did some measurement here with my i.MX23 based hardware.
> >
> > I'm using 32 bits per pixel and the 18 bit display interface
> > (LCD_D[0..17]).
> >
> > And surprise, surprise: the i.MX23 *always* maps the 24 bit input data to
> > the 18 bit interface. And what does the DATA_FORMAT_24_BIT aka. CTRL_DF24
> > do??? Just simple: it seems its meaning changes with the interface width.
> > The documentation says:
> >
> >    0 = all 24 bits are valid
> >    1 = drop upper 2 bits per byte
> >
> > This text seems valid for a real 24 bit display (but I cannot test it).
> > From my measurement its meaning changes when used with an 18 bit display
> > to:
> >
> >    0 = drop lower 2 bits per byte
> >    1 = drop upper 2 bits per byte
> >
> > When this bit is 0:
> >
> >        red     green    blue
> >     10000001|00001111|10001111 (memory layout)
> >
> >     100000..|000011..|100011.. (at the display)
> >
> > When this bit is 1:
> >
> >        red     green    blue
> >     10000001|00001111|10001111 (memory layout)
> >
> >     ..000001|..001111|..001111 (at the display)
>
> And how can the setting of this flag be useful?

Maybe with this measurement knowledge is isn't useful any more. When I wrote 
this code I just used the datasheet.

> In the example above, you had a red component of 0x81 (half way through the
> scale), which is converted to a 0x01 (almost black) at the display. Isn't
> this wrong?
>
> This only looks right to me if you set a 18bpp but even in that case
> masking the upper bits isn't really needed at all.

These bits should only show what is happen to the data. In both cases you must 
define a corresponding fb_bitfield description. Which means the currently 
existing def_rgb666 description is valid only when the DATA_FORMAT_24_BIT 
aka. CTRL_DF24 register bit is set.

And for the DATA_FORMAT_24_BIT aka. CTRL_DF24 register be cleared you *can* 
use the def_rgb888 or define a different def_rgb666_shift like this one:

static const struct fb_bitfield def_rgb666_shift[] = {
	[RED] = {
		.offset = 18,
		.length = 6,
	},
	[GREEN] = {
		.offset = 10,
		.length = 6,
	},
	[BLUE] = {
		.offset = 2,
		.length = 6,
	},
	[TRANSP] = {	/* no support for transparency */
		.length = 0,
	}
};

I'm not sure if it is of interest for a userland application to know if a 
display can handle 256 k or 16 M colours physically.
With the def_rgb888 in use *all* displays look like 16 M colour LCDs. With the 
def_rgb666 in use an 18 bit display really looks like an 18 bit display.

Regards,
Juergen

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

Re: mxsfb: DATA_FORMAT_24_BIT flag outputs invalid colours

[Juergen Beisert]

Hi Hector,

Juergen Beisert wrote:
> Hector Palacios wrote:
> > On 05/24/2013 12:28 PM, Juergen Beisert wrote:
> > > Hector Palacios wrote:
> > >> On 05/23/2013 03:31 PM, Juergen Beisert wrote:
> > >>> maxime.ripard@free-electrons.com wrote:
> > >>>> On Thu, May 23, 2013 at 01:55:28PM +0200, Hector Palacios wrote:
> > >>>>> I'm using an i.MX28 based board with lcd connected with 18bits data
> > >>>>> bus. My platform uses 32 bits per pixel:
> > >>>>>
> > >>>>> 	mxsfb_pdata.default_bpp = 32;
> > >>>>> 	mxsfb_pdata.ld_intf_width = STMLCDIF_18BIT;
> > >>>>>
> > >>>>> With these settings the mxsfb.c driver sets flag DATA_FORMAT_24_BIT
> > >>>>> at HW_LCDIF_CTRL register in function mxsfb_set_par():
> > >>>>>
> > >>>>> 	case 32:
> > >>>>> 		dev_dbg(&host->pdev->dev, "Setting up RGB888/666 mode\n");
> > >>>>> 		ctrl |= CTRL_SET_WORD_LENGTH(3);
> > >>>>> 		switch (host->ld_intf_width) {
> > >>>>> 		case STMLCDIF_8BIT:
> > >>>>> 			dev_dbg(&host->pdev->dev,
> > >>>>> 					"Unsupported LCD bus width mapping\n");
> > >>>>> 			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;
> > >>>>> 		}
> > >>>>>
> > >>>>> According to the manual, this flag does:
> > >>>>> 	0x0: ALL_24_BITS_VALID: Data input to the block is in 24 bpp
> > >>>>> format, such that all RGB 888 data is contained in 24 bits.
> > >>>>> 	0x1: DROP_UPPER_2_BITS_PER_BYTE — Data input to the block is
> > >>>>> actually RGB 18 bpp, but there is 1 colour per byte, hence the
> > >>>>> upper 2 bits in each byte do not contain any useful data, and
> > >>>>> should be dropped.
> > >>>>>
> > >>>>> The setting of this flag is producing bad colours with true colour
> > >>>>> images (i.e. the Linux penguin is displayed ok, but QT applications
> > >>>>> or images displayed with fbv are not).
> > >>>>> I believe the setting of this flag is not correct (after all, if my
> > >>>>> bpp is 32, then all 24bit colours are useful and dropping the upper
> > >>>>> 2 bits is a bad idea).
> > >>>>> If I don't set it, then true colour images are displayed correctly.
> > >>>>> The only problem is that the Linux penguin is displayed much darker
> > >>>>> than usual (correct colours, but darker). Perhaps the 224 colour
> > >>>>> format of this image justifies it?
> > >>>>>
> > >>>>> I noticed the cfa10049 platform also uses the same configuration
> > >>>>> (18 bits data bus and 32bpp) and was wondering if true colour
> > >>>>> images are correctly displayed in this platform with this flag set
> > >>>>> (for example with fbv application [1]).
> > >>>>
> > >>>> I had the exact same problem, and suggested the exact same solution
> > >>>> a few weeks back.
> > >>>>
> > >>>> https://patchwork.kernel.org/patch/2470441/
> > >>>>
> > >>>> The conclusion of that discussion what that the userspace
> > >>>> applications were not honouring the bitfield correctly set by the
> > >>>> mxsfb driver, and as such, it was not a bug in the driver.
> > >>>>
> > >>>> While this is correct, I wonder, now that since we had that same
> > >>>> problem in a very short amount of time, if we couldn't set this
> > >>>> behaviour dependant of some (dt? kernel argument?) property so that
> > >>>> one could customise it anyway he want.
> > >>>>
> > >>>> Maxime
> > >>>
> > >>> i.MX2[3|8]    LCD1       LCD2       LCD3
> > >>>                 24bit      18bit      18bit
> > >>> --------------------------------------------
> > >>> LCD_D0         B0         B0         --
> > >>> LCD_D1         B1         B1         --
> > >>> LCD_D2         B2         B2         B0
> > >>> LCD_D3         B3         B3         B1
> > >>> LCD_D4         B4         B4         B2
> > >>> LCD_D5         B5         B5         B3
> > >>> LCD_D6         B6         G0         B4
> > >>> LCD_D7         B7         G1         B5
> > >>>
> > >>> LCD_D8         G0         G2         --
> > >>> LCD_D9         G1         G3         --
> > >>> LCD_D10        G2         G4         G0
> > >>> LCD_D11        G3         G5         G1
> > >>> LCD_D12        G4         R0         G2
> > >>> LCD_D13        G5         R1         G3
> > >>> LCD_D14        G6         R2         G4
> > >>> LCD_D15        G7         R3         G5
> > >>>
> > >>> LCD_D16        R0         R4         --
> > >>> LCD_D17        R1         R5         --
> > >>> LCD_D18        R2                    R0
> > >>> LCD_D19        R3                    R1
> > >>> LCD_D20        R4                    R2
> > >>> LCD_D21        R5                    R3
> > >>> LCD_D22        R6                    R4
> > >>> LCD_D23        R7                    R5
> > >>>
> > >>> Is your display connected like LCD2 or LCD3? LCD3 must still handled
> > >>> like a 24 bit display shown in LCD1, while only the LCD2-case is the
> > >>> "24 bit to 18 bit mapping" case.
> > >>>
> > >>> At least my current tests with an i.MX23 and a connection like LCD2
> > >>> are working here with a Qt application. Qt honours the pixel bitfield
> > >>> description. And I'm using the "bits-per-pixel = <32>" and "bus-width
> > >>> = <18>" entries in the device tree.
> > >>
> > >> I have a 24bit LCD display but my connection to it is done at 18bits
> > >> data width. Represented below as LCD4.
> > >> NOTE: In my LCD4 column, notation Rx/Gx/Bx represent the color bit in
> > >> memory as well as the display data line.
> > >> Since we use 32bpp each channel has 8 bits (R7..R0, etc.).
> > >> I understand that you have an 18bit display and that your notation in
> > >> LCD2 column represents the display data lines, not the color bit
> > >> indexes in memory.
> > >>
> > >> i.MX2[3|8]    LCD1       LCD2       LCD3	LCD4
> > >>                24bit      18bit      18bit	24bit connected at 18bit
> > >> -------------------------------------------------------
> > >> LCD_D0         B0         B0         --		B2
> > >> LCD_D1         B1         B1         --		B3
> > >> LCD_D2         B2         B2         B0		B4
> > >> LCD_D3         B3         B3         B1		B5
> > >> LCD_D4         B4         B4         B2		B6
> > >> LCD_D5         B5         B5         B3		B7
> > >> LCD_D6         B6         G0         B4		G2
> > >> LCD_D7         B7         G1         B5		G3
> > >>
> > >> LCD_D8         G0         G2         --		G4
> > >> LCD_D9         G1         G3         --		G5
> > >> LCD_D10        G2         G4         G0		G6
> > >> LCD_D11        G3         G5         G1		G7
> > >> LCD_D12        G4         R0         G2		R2
> > >> LCD_D13        G5         R1         G3		R3
> > >> LCD_D14        G6         R2         G4		R4
> > >> LCD_D15        G7         R3         G5		R5
> > >>
> > >> LCD_D16        R0         R4         --		R6
> > >> LCD_D17        R1         R5         --		R7
> > >> LCD_D18        R2                    R0
> > >> LCD_D19        R3                    R1
> > >> LCD_D20        R4                    R2
> > >> LCD_D21        R5                    R3
> > >> LCD_D22        R6                    R4
> > >> LCD_D23        R7                    R5
> > >>
> > >> For 32bpp (RGB888) and 18bit data bus I would expect the LCD
> > >> controller to take the six *most significant* bits [7..2] from each
> > >> color byte out to the LCD data bus (LCD_D17..D0) in the order depicted
> > >> in my LCD4 column.
> > >>
> > >> I'm not sure what the DATA_FORMAT_24_BIT flag is doing, but dropping
> > >> the two most significant bits of color in memory doesn't seem to be a
> > >> good idea unless (maybe) color is in 18bpp. Previous kernels did not
> > >> even touch this flag.
> > >>
> > >> Does the following patch make sense?
> > >>
> > >> diff --git a/drivers/video/mxsfb.c b/drivers/video/mxsfb.c
> > >> index b1c1a80..bb0a4e1 100644
> > >> --- a/drivers/video/mxsfb.c
> > >> +++ b/drivers/video/mxsfb.c
> > >> @@ -298,9 +298,6 @@ static int mxsfb_check_var(struct
> > >> fb_var_screeninfo *var, break;
> > >>                   case STMLCDIF_16BIT:
> > >>                   case STMLCDIF_18BIT:
> > >> -                       /* 24 bit to 18 bit mapping */
> > >> -                       rgb = def_rgb666;
> > >> -                       break;
> > >>                   case STMLCDIF_24BIT:
> > >>                           /* real 24 bit */
> > >>                           rgb = def_rgb888;
> > >> @@ -424,11 +421,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;
> > >>
> > >> The setting of def_rgb666 for a 32bpp color depth does not make sense
> > >> to me because the color in memory is really rgb888.
> > >>
> > >> With this patch, my true color images are displayed ok and so does the
> > >> penguin logo. I don't know however how other displays connections at
> > >> 18bit will do.
> > >
> > > Your 24 bit display is connected like a regular 18 bit display. So they
> > > should work in the same way with the same settings. You *must* always
> > > skip two data bits from the memory RGB888 to form a RGB666 value.
> > >
> > > I just did some measurement here with my i.MX23 based hardware.
> > >
> > > I'm using 32 bits per pixel and the 18 bit display interface
> > > (LCD_D[0..17]).
> > >
> > > And surprise, surprise: the i.MX23 *always* maps the 24 bit input data
> > > to the 18 bit interface. And what does the DATA_FORMAT_24_BIT aka.
> > > CTRL_DF24 do??? Just simple: it seems its meaning changes with the
> > > interface width. The documentation says:
> > >
> > >    0 = all 24 bits are valid
> > >    1 = drop upper 2 bits per byte
> > >
> > > This text seems valid for a real 24 bit display (but I cannot test it).
> > > From my measurement its meaning changes when used with an 18 bit
> > > display to:
> > >
> > >    0 = drop lower 2 bits per byte
> > >    1 = drop upper 2 bits per byte
> > >
> > > When this bit is 0:
> > >
> > >        red     green    blue
> > >     10000001|00001111|10001111 (memory layout)
> > >
> > >     100000..|000011..|100011.. (at the display)
> > >
> > > When this bit is 1:
> > >
> > >        red     green    blue
> > >     10000001|00001111|10001111 (memory layout)
> > >
> > >     ..000001|..001111|..001111 (at the display)
> >
> > And how can the setting of this flag be useful?
>
> Maybe with this measurement knowledge is isn't useful any more. When I
> wrote this code I just used the datasheet.
>
> > In the example above, you had a red component of 0x81 (half way through
> > the scale), which is converted to a 0x01 (almost black) at the display.
> > Isn't this wrong?
> >
> > This only looks right to me if you set a 18bpp but even in that case
> > masking the upper bits isn't really needed at all.
>
> These bits should only show what is happen to the data. In both cases you
> must define a corresponding fb_bitfield description. Which means the
> currently existing def_rgb666 description is valid only when the
> DATA_FORMAT_24_BIT aka. CTRL_DF24 register bit is set.
>
> And for the DATA_FORMAT_24_BIT aka. CTRL_DF24 register be cleared you *can*
> use the def_rgb888 or define a different def_rgb666_shift like this one:
>
> static const struct fb_bitfield def_rgb666_shift[] = {
> 	[RED] = {
> 		.offset = 18,
> 		.length = 6,
> 	},
> 	[GREEN] = {
> 		.offset = 10,
> 		.length = 6,
> 	},
> 	[BLUE] = {
> 		.offset = 2,
> 		.length = 6,
> 	},
> 	[TRANSP] = {	/* no support for transparency */
> 		.length = 0,
> 	}
> };
>
> I'm not sure if it is of interest for a userland application to know if a
> display can handle 256 k or 16 M colours physically.
> With the def_rgb888 in use *all* displays look like 16 M colour LCDs. With
> the def_rgb666 in use an 18 bit display really looks like an 18 bit
> display.

Someone told me, Qt5 cannot handle this special RGB666 mode (even not the 
def_rgb666_shift memory layout mentioned above). My test are based on Qt4. 
Qt5 needs a regular RGB888 mode, which should silently be converted 
internally to RGB666 in the hardware.

So, your patch to always use the RGB888 memory layout seems to be the right 
way to go.

Regards,
Juergen

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

Re: mxsfb: DATA_FORMAT_24_BIT flag outputs invalid colours

[maxime.ripard]

Hi Hector,

On Fri, May 24, 2013 at 03:33:19PM +0200, Juergen Beisert wrote:
> Someone told me, Qt5 cannot handle this special RGB666 mode (even not the 
> def_rgb666_shift memory layout mentioned above). My test are based on Qt4. 
> Qt5 needs a regular RGB888 mode, which should silently be converted 
> internally to RGB666 in the hardware.
> 
> So, your patch to always use the RGB888 memory layout seems to be the right 
> way to go.

Do you plan on submitting this patch? (Or did you already submit it and
I overlooked it?)

Thanks,
Maxime

Re: mxsfb: DATA_FORMAT_24_BIT flag outputs invalid colours

[Hector Palacios]

Hi Maxime,

On 06/07/2013 09:21 AM, maxime.ripard@free-electrons.com wrote:
> Hi Hector,
>
> On Fri, May 24, 2013 at 03:33:19PM +0200, Juergen Beisert wrote:
>> Someone told me, Qt5 cannot handle this special RGB666 mode (even not the
>> def_rgb666_shift memory layout mentioned above). My test are based on Qt4.
>> Qt5 needs a regular RGB888 mode, which should silently be converted
>> internally to RGB666 in the hardware.
>>
>> So, your patch to always use the RGB888 memory layout seems to be the right
>> way to go.
>
> Do you plan on submitting this patch? (Or did you already submit it and
> I overlooked it?)

I wasn't sure that everybody involved agreed with the patch.
@Juergen, would the patch break your platform?
Additionally, the guys from Crystalfontz didn't comment on it, but their platform is 
also using a 18bit data bus width and 32bpp.

If no-one is against I'll be glad to submit it.

Best regards,
--
Hector Palacios

Re: mxsfb: DATA_FORMAT_24_BIT flag outputs invalid colours

[Juergen Beisert]

Hi Hector,

On 07.06.2013 09:28, Hector Palacios wrote:
> On 06/07/2013 09:21 AM, maxime.ripard@free-electrons.com wrote:
>> Hi Hector,
>>
>> On Fri, May 24, 2013 at 03:33:19PM +0200, Juergen Beisert wrote:
>>> Someone told me, Qt5 cannot handle this special RGB666 mode (even not the
>>> def_rgb666_shift memory layout mentioned above). My test are based on Qt4.
>>> Qt5 needs a regular RGB888 mode, which should silently be converted
>>> internally to RGB666 in the hardware.
>>>
>>> So, your patch to always use the RGB888 memory layout seems to be the right
>>> way to go.
>>
>> Do you plan on submitting this patch? (Or did you already submit it and
>> I overlooked it?)
> 
> I wasn't sure that everybody involved agreed with the patch.
> @Juergen, would the patch break your platform?

No, we need to switch to this data format here, too.

> Additionally, the guys from Crystalfontz didn't comment on it, but their platform is 
> also using a 18bit data bus width and 32bpp.
> 
> If no-one is against I'll be glad to submit it.

You can have my Acked-by: Juergen Beisert <jbe@pengutronix.de> for this patch.

Regards,
Juergen

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | Phone: +49-5121-206917-5128 |
Peiner Str. 6-8, 31137 Hildesheim, Germany    | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686              | http://www.pengutronix.de/  |

Re: mxsfb: DATA_FORMAT_24_BIT flag outputs invalid colours

[maxime.ripard]

On Fri, Jun 07, 2013 at 09:28:32AM +0200, Hector Palacios wrote:
> I wasn't sure that everybody involved agreed with the patch.
> @Juergen, would the patch break your platform?
> Additionally, the guys from Crystalfontz didn't comment on it, but
> their platform is also using a 18bit data bus width and 32bpp.

We (Free Electrons) are doing the bring up for the Crystalfontz boards,
so it's fine :)

Thanks!
Maxime

[PATCH] video: mxsfb: fix color settings for 18bit data bus and 32bpp

[Hector Palacios]

For a combination of 18bit LCD data bus width and a color
mode of 32bpp, the driver was setting the color mapping to
rgb666, which is wrong, as the color in memory realy has an
rgb888 layout.

This patch also removes the setting of flag CTRL_DF24 that
makes the driver dimiss the upper 2 bits when handling 32/24bpp
colors in a diplay with 18bit data bus width. This flag made
true color images display wrong in such configurations.

Finally, the color mapping rgb666 has also been removed as nobody
is using it and high level applications like Qt5 cannot work
with it either.

Reference: https://lkml.org/lkml/2013/5/23/220
Signed-off-by: Hector Palacios <hector.palacios@digi.com>
Acked-by: Juergen Beisert <jbe@pengutronix.de>
---
  drivers/video/mxsfb.c | 26 --------------------------
  1 file changed, 26 deletions(-)

diff --git a/drivers/video/mxsfb.c b/drivers/video/mxsfb.c
index 21223d4..d2c5105 100644
--- a/drivers/video/mxsfb.c
+++ b/drivers/video/mxsfb.c
@@ -239,24 +239,6 @@ static const struct fb_bitfield def_rgb565[] = {
  	}
  };

-static const struct fb_bitfield def_rgb666[] = {
-	[RED] = {
-		.offset = 16,
-		.length = 6,
-	},
-	[GREEN] = {
-		.offset = 8,
-		.length = 6,
-	},
-	[BLUE] = {
-		.offset = 0,
-		.length = 6,
-	},
-	[TRANSP] = {	/* no support for transparency */
-		.length = 0,
-	}
-};
-
  static const struct fb_bitfield def_rgb888[] = {
  	[RED] = {
  		.offset = 16,
@@ -309,9 +291,6 @@ static int mxsfb_check_var(struct fb_var_screeninfo *var,
  			break;
  		case STMLCDIF_16BIT:
  		case STMLCDIF_18BIT:
-			/* 24 bit to 18 bit mapping */
-			rgb = def_rgb666;
-			break;
  		case STMLCDIF_24BIT:
  			/* real 24 bit */
  			rgb = def_rgb888;
@@ -453,11 +432,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.8.3

Re: [PATCH] video: mxsfb: fix color settings for 18bit data bus and 32bpp

[maxime.ripard]

Hi Hector,

On Fri, Jun 07, 2013 at 10:10:39AM +0200, Hector Palacios wrote:
> For a combination of 18bit LCD data bus width and a color
> mode of 32bpp, the driver was setting the color mapping to
> rgb666, which is wrong, as the color in memory realy has an
> rgb888 layout.
> 
> This patch also removes the setting of flag CTRL_DF24 that
> makes the driver dimiss the upper 2 bits when handling 32/24bpp
> colors in a diplay with 18bit data bus width. This flag made
> true color images display wrong in such configurations.
> 
> Finally, the color mapping rgb666 has also been removed as nobody
> is using it and high level applications like Qt5 cannot work
> with it either.
> 
> Reference: https://lkml.org/lkml/2013/5/23/220
> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
> Acked-by: Juergen Beisert <jbe@pengutronix.de>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Please also note that fbdev is now maintained by Jean Christophe
Plagniol-Villard (plagnioj@jcrosoft.com, in CC), and that he is away
until the 10th of June, so maybe it should be safe to resend it to him
after this date.

Thanks!
Maxime

Re: [PATCH] video: mxsfb: fix color settings for 18bit data bus and 32bpp

[maxime.ripard]

Hi Hector,

On Fri, Jun 07, 2013 at 11:02:28AM +0200, maxime.ripard@free-electrons.com wrote:
> On Fri, Jun 07, 2013 at 10:10:39AM +0200, Hector Palacios wrote:
> > For a combination of 18bit LCD data bus width and a color
> > mode of 32bpp, the driver was setting the color mapping to
> > rgb666, which is wrong, as the color in memory realy has an
> > rgb888 layout.
> > 
> > This patch also removes the setting of flag CTRL_DF24 that
> > makes the driver dimiss the upper 2 bits when handling 32/24bpp
> > colors in a diplay with 18bit data bus width. This flag made
> > true color images display wrong in such configurations.
> > 
> > Finally, the color mapping rgb666 has also been removed as nobody
> > is using it and high level applications like Qt5 cannot work
> > with it either.
> > 
> > Reference: https://lkml.org/lkml/2013/5/23/220
> > Signed-off-by: Hector Palacios <hector.palacios@digi.com>
> > Acked-by: Juergen Beisert <jbe@pengutronix.de>
> 
> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> 
> Please also note that fbdev is now maintained by Jean Christophe
> Plagniol-Villard (plagnioj@jcrosoft.com, in CC), and that he is away
> until the 10th of June, so maybe it should be safe to resend it to him
> after this date.

It seems like Jean-Christophe is back online, maybe you should resend
him the patch now, it would be great to have it for 3.11.

Thanks,
Maxime

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

Re: [PATCH] video: mxsfb: fix color settings for 18bit data bus and 32bpp

[Hector Palacios]

On 06/18/2013 10:32 AM, maxime.ripard@free-electrons.com wrote:
> Hi Hector,
>
> On Fri, Jun 07, 2013 at 11:02:28AM +0200, maxime.ripard@free-electrons.com wrote:
>> On Fri, Jun 07, 2013 at 10:10:39AM +0200, Hector Palacios wrote:
>>> For a combination of 18bit LCD data bus width and a color
>>> mode of 32bpp, the driver was setting the color mapping to
>>> rgb666, which is wrong, as the color in memory realy has an
>>> rgb888 layout.
>>>
>>> This patch also removes the setting of flag CTRL_DF24 that
>>> makes the driver dimiss the upper 2 bits when handling 32/24bpp
>>> colors in a diplay with 18bit data bus width. This flag made
>>> true color images display wrong in such configurations.
>>>
>>> Finally, the color mapping rgb666 has also been removed as nobody
>>> is using it and high level applications like Qt5 cannot work
>>> with it either.
>>>
>>> Reference: https://lkml.org/lkml/2013/5/23/220
>>> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
>>> Acked-by: Juergen Beisert <jbe@pengutronix.de>
>>
>> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>>
>> Please also note that fbdev is now maintained by Jean Christophe
>> Plagniol-Villard (plagnioj@jcrosoft.com, in CC), and that he is away
>> until the 10th of June, so maybe it should be safe to resend it to him
>> after this date.
>
> It seems like Jean-Christophe is back online, maybe you should resend
> him the patch now, it would be great to have it for 3.11.

I just resent it. Thanks for the reminder.

-- 
Héctor Palacios

Best regards,
--
Hector Palacios

Re: mxsfb: DATA_FORMAT_24_BIT flag outputs invalid colours

[maxime.ripard]

Hi Juergen,

On Thu, May 23, 2013 at 03:31:31PM +0200, Juergen Beisert wrote:
> Hi Maxime,
> 
> maxime.ripard@free-electrons.com wrote:
> > On Thu, May 23, 2013 at 01:55:28PM +0200, Hector Palacios wrote:
> > > I'm using an i.MX28 based board with lcd connected with 18bits data bus.
> > > My platform uses 32 bits per pixel:
> > >
> > > 	mxsfb_pdata.default_bpp = 32;
> > > 	mxsfb_pdata.ld_intf_width = STMLCDIF_18BIT;
> > >
> > > With these settings the mxsfb.c driver sets flag DATA_FORMAT_24_BIT
> > > at HW_LCDIF_CTRL register in function mxsfb_set_par():
> > >
> > > 	case 32:
> > > 		dev_dbg(&host->pdev->dev, "Setting up RGB888/666 mode\n");
> > > 		ctrl |= CTRL_SET_WORD_LENGTH(3);
> > > 		switch (host->ld_intf_width) {
> > > 		case STMLCDIF_8BIT:
> > > 			dev_dbg(&host->pdev->dev,
> > > 					"Unsupported LCD bus width mapping\n");
> > > 			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;
> > > 		}
> > >
> > > According to the manual, this flag does:
> > > 	0x0: ALL_24_BITS_VALID: Data input to the block is in 24 bpp
> > > format, such that all RGB 888 data is contained in 24 bits.
> > > 	0x1: DROP_UPPER_2_BITS_PER_BYTE — Data input to the block is
> > > actually RGB 18 bpp, but there is 1 colour per byte, hence the upper
> > > 2 bits in each byte do not contain any useful data, and should be
> > > dropped.
> > >
> > > The setting of this flag is producing bad colours with true colour
> > > images (i.e. the Linux penguin is displayed ok, but QT applications
> > > or images displayed with fbv are not).
> > > I believe the setting of this flag is not correct (after all, if my
> > > bpp is 32, then all 24bit colours are useful and dropping the upper
> > > 2 bits is a bad idea).
> > > If I don't set it, then true colour images are displayed correctly.
> > > The only problem is that the Linux penguin is displayed much darker
> > > than usual (correct colours, but darker). Perhaps the 224 colour
> > > format of this image justifies it?
> > >
> > > I noticed the cfa10049 platform also uses the same configuration (18
> > > bits data bus and 32bpp) and was wondering if true colour images are
> > > correctly displayed in this platform with this flag set (for example
> > > with fbv application [1]).
> >
> > I had the exact same problem, and suggested the exact same solution a
> > few weeks back.
> >
> > https://patchwork.kernel.org/patch/2470441/
> >
> > The conclusion of that discussion what that the userspace applications
> > were not honouring the bitfield correctly set by the mxsfb driver, and
> > as such, it was not a bug in the driver.
> >
> > While this is correct, I wonder, now that since we had that same problem
> > in a very short amount of time, if we couldn't set this behaviour
> > dependant of some (dt? kernel argument?) property so that one could
> > customise it anyway he want.
> >
> > Maxime
> 
> i.MX2[3|8]    LCD1       LCD2       LCD3
>               24bit      18bit      18bit
> --------------------------------------------
> LCD_D0         B0         B0         --
> LCD_D1         B1         B1         --
> LCD_D2         B2         B2         B0
> LCD_D3         B3         B3         B1
> LCD_D4         B4         B4         B2
> LCD_D5         B5         B5         B3
> LCD_D6         B6         G0         B4
> LCD_D7         B7         G1         B5
> 
> LCD_D8         G0         G2         --
> LCD_D9         G1         G3         --
> LCD_D10        G2         G4         G0
> LCD_D11        G3         G5         G1
> LCD_D12        G4         R0         G2
> LCD_D13        G5         R1         G3
> LCD_D14        G6         R2         G4
> LCD_D15        G7         R3         G5
> 
> LCD_D16        R0         R4         --
> LCD_D17        R1         R5         --
> LCD_D18        R2                    R0
> LCD_D19        R3                    R1
> LCD_D20        R4                    R2
> LCD_D21        R5                    R3
> LCD_D22        R6                    R4
> LCD_D23        R7                    R5
> 
> Is your display connected like LCD2 or LCD3? LCD3 must still handled like a 24 
> bit display shown in LCD1, while only the LCD2-case is the "24 bit to 18 bit 
> mapping" case.
> 
> At least my current tests with an i.MX23 and a connection like LCD2 are 
> working here with a Qt application. Qt honours the pixel bitfield 
> description. And I'm using the "bits-per-pixel = <32>" and "bus-width = <18>" 
> entries in the device tree.

I'm in the second case, so with the same setup that you have it seems.

Don't get me wrong, I was not saying that the driver was flawed or had a
bug, I understood very well your arguments when I first submitted the
patch. The problem is more for all the userspace applications that don't
honour the bitfield and just look at the color depth used to feed data
to the framebuffer.

I must admit that the ideal case should be that these applications
should be fixed, but it's not realistic.

About QT, let's not use it as an example, the issues I had were probably
some miscompilation on my side, and if you say that it works fine, then
I trust you.

This is also why I suggested a kernel argument to enable a different
"compatibility" mode when you have to deal with these kind of
applications on your system. the pixel bitfield should probably be
changed as well so that applications honouring it are not confused.

Hector's case looks like a different story though.

Maxime

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