[PATCHv4][ 3/7] staging: imx-drm: Add RGB666 support for parallel display.

[Laurent Pinchart]

Hi Russell,

On Wednesday 13 November 2013 19:12:30 Russell King - ARM Linux wrote:
> On Wed, Nov 13, 2013 at 11:43:44AM -0700, Troy Kisky wrote:
> > On 11/13/2013 2:23 AM, Denis Carikli wrote:
> >>   +/* rgb666 */
> >> 
> >> +  ipu_dc_map_clear(priv, IPU_DC_MAP_RGB666);
> >> +  ipu_dc_map_config(priv, IPU_DC_MAP_RGB666, 2, 17, 0xfc); /* red */
> >> +  ipu_dc_map_config(priv, IPU_DC_MAP_RGB666, 1, 11, 0xfc); /* green */
> >> +  ipu_dc_map_config(priv, IPU_DC_MAP_RGB666, 0, 5, 0xfc); /* blue */
> >> +
> >>return 0;
> >>   }
> > 
> > Since,  rgb24 and bgr24 reverse the byte numbers
> > /* rgb24 */
> > ipu_dc_map_clear(priv, IPU_DC_MAP_RGB24);
> > ipu_dc_map_config(priv, IPU_DC_MAP_RGB24, 0, 7, 0xff); /* blue */
> > ipu_dc_map_config(priv, IPU_DC_MAP_RGB24, 1, 15, 0xff); /* green
> > */
> > ipu_dc_map_config(priv, IPU_DC_MAP_RGB24, 2, 23, 0xff); /* red */
> > 
> > /* bgr24 */
> > ipu_dc_map_clear(priv, IPU_DC_MAP_BGR24);
> > ipu_dc_map_config(priv, IPU_DC_MAP_BGR24, 2, 7, 0xff); /* red */
> > ipu_dc_map_config(priv, IPU_DC_MAP_BGR24, 1, 15, 0xff); /* green
> > */
> > ipu_dc_map_config(priv, IPU_DC_MAP_BGR24, 0, 23, 0xff); /* blue */
> > 
> > Shouldn't rgb666 and bgr666 do the same?
> > Currently we have,
> > 
> > /* bgr666 */
> > ipu_dc_map_clear(priv, IPU_DC_MAP_BGR666);
> > ipu_dc_map_config(priv, IPU_DC_MAP_BGR666, 0, 5, 0xfc); /* blue */
> > ipu_dc_map_config(priv, IPU_DC_MAP_BGR666, 1, 11, 0xfc); /*
> > green */
> > ipu_dc_map_config(priv, IPU_DC_MAP_BGR666, 2, 17, 0xfc); /* red */
> 
> Yes, I concur - this doesn't make sense to me.  BGR666 would mean in
> memory:
> 
> 111
> 721650
> BBGGRR
> 
> which reflects the same order for "RGB24" above.

Beside component order and number of bits per component, an in-memory RGB 
format also defines the memory endianness and, for formats that don't span an 
interger number of bytes, the left or right alignment.

BGR666 is currently defined in V4L2 as

Byte 0 12
Bit  7  6  5  4  3  2  1  07  6  5  4  3  2  1  0   7  6  5  4  3  2  1  0

 b5 b4 b3 b2 b1 b0 g5 g4  g3 g2 g1 g0 r5 r4 r3 r2  r1 r0  -  -  -  -  -  -

(see the *second* table in 
http://linuxtv.org/downloads/v4l-dvb-apis/packed-rgb.html)

I would thus expect RGB666 to be

Byte 0 12
Bit  7  6  5  4  3  2  1  07  6  5  4  3  2  1  0   7  6  5  4  3  2  1  0

 r5 r4 r3 r2 r1 r0 g5 g4  g3 g2 g1 g0 b5 b4 b3 b2  b1 b0  -  -  -  -  -  -

We can also define right-aligned formats if needed.

> > Where I'd expect to see
> > /* bgr666 */
> > 
> > ipu_dc_map_clear(priv, IPU_DC_MAP_BGR666);
> > ipu_dc_map_config(priv, IPU_DC_MAP_BGR666, 0, 17, 0xfc); /* blue
> > */
> > ipu_dc_map_config(priv, IPU_DC_MAP_BGR666, 1, 11, 0xfc); /*
> > green */
> > ipu_dc_map_config(priv, IPU_DC_MAP_BGR666, 2, 5, 0xfc); /* red */
> 
> So this makes sense to me.

-- 
Regards,

Laurent Pinchart

[PATCHv4][ 3/7] staging: imx-drm: Add RGB666 support for parallel display.

[Russell King - ARM Linux]

On Wed, Nov 13, 2013 at 11:43:44AM -0700, Troy Kisky wrote:
> On 11/13/2013 2:23 AM, Denis Carikli wrote:
>>   +  /* rgb666 */
>> +ipu_dc_map_clear(priv, IPU_DC_MAP_RGB666);
>> +ipu_dc_map_config(priv, IPU_DC_MAP_RGB666, 2, 17, 0xfc); /* red */
>> +ipu_dc_map_config(priv, IPU_DC_MAP_RGB666, 1, 11, 0xfc); /* green */
>> +ipu_dc_map_config(priv, IPU_DC_MAP_RGB666, 0, 5, 0xfc); /* blue */
>> +
>>  return 0;
>>   }
>>   
>>
>
> Since,  rgb24 and bgr24 reverse the byte numbers
> /* rgb24 */
> ipu_dc_map_clear(priv, IPU_DC_MAP_RGB24);
> ipu_dc_map_config(priv, IPU_DC_MAP_RGB24, 0, 7, 0xff); /* blue */
> ipu_dc_map_config(priv, IPU_DC_MAP_RGB24, 1, 15, 0xff); /* green */
> ipu_dc_map_config(priv, IPU_DC_MAP_RGB24, 2, 23, 0xff); /* red */
>
> /* bgr24 */
> ipu_dc_map_clear(priv, IPU_DC_MAP_BGR24);
> ipu_dc_map_config(priv, IPU_DC_MAP_BGR24, 2, 7, 0xff); /* red */
> ipu_dc_map_config(priv, IPU_DC_MAP_BGR24, 1, 15, 0xff); /* green */
> ipu_dc_map_config(priv, IPU_DC_MAP_BGR24, 0, 23, 0xff); /* blue */


>
>
> Shouldn't rgb666 and bgr666 do the same?
> Currently we have,
>
> /* bgr666 */
> ipu_dc_map_clear(priv, IPU_DC_MAP_BGR666);
> ipu_dc_map_config(priv, IPU_DC_MAP_BGR666, 0, 5, 0xfc); /* blue */
> ipu_dc_map_config(priv, IPU_DC_MAP_BGR666, 1, 11, 0xfc); /*  
> green */
> ipu_dc_map_config(priv, IPU_DC_MAP_BGR666, 2, 17, 0xfc); /* red */

Yes, I concur - this doesn't make sense to me.  BGR666 would mean in
memory:

111
721650
BBGGRR

which reflects the same order for "RGB24" above.

> Where I'd expect to see
> /* bgr666 */
> ipu_dc_map_clear(priv, IPU_DC_MAP_BGR666);
> ipu_dc_map_config(priv, IPU_DC_MAP_BGR666, 0, 17, 0xfc); /* blue */
> ipu_dc_map_config(priv, IPU_DC_MAP_BGR666, 1, 11, 0xfc); /*  
> green */
> ipu_dc_map_config(priv, IPU_DC_MAP_BGR666, 2, 5, 0xfc); /* red */

So this makes sense to me.

[PATCHv4][ 3/7] staging: imx-drm: Add RGB666 support for parallel display.

[Troy Kisky]

On 11/13/2013 2:23 AM, Denis Carikli wrote:
>   
> + /* rgb666 */
> + ipu_dc_map_clear(priv, IPU_DC_MAP_RGB666);
> + ipu_dc_map_config(priv, IPU_DC_MAP_RGB666, 2, 17, 0xfc); /* red */
> + ipu_dc_map_config(priv, IPU_DC_MAP_RGB666, 1, 11, 0xfc); /* green */
> + ipu_dc_map_config(priv, IPU_DC_MAP_RGB666, 0, 5, 0xfc); /* blue */
> +
>   return 0;
>   }
>   
>

Since,  rgb24 and bgr24 reverse the byte numbers
/* rgb24 */
 ipu_dc_map_clear(priv, IPU_DC_MAP_RGB24);
 ipu_dc_map_config(priv, IPU_DC_MAP_RGB24, 0, 7, 0xff); /* blue */
 ipu_dc_map_config(priv, IPU_DC_MAP_RGB24, 1, 15, 0xff); /* green */
 ipu_dc_map_config(priv, IPU_DC_MAP_RGB24, 2, 23, 0xff); /* red */

/* bgr24 */
 ipu_dc_map_clear(priv, IPU_DC_MAP_BGR24);
 ipu_dc_map_config(priv, IPU_DC_MAP_BGR24, 2, 7, 0xff); /* red */
 ipu_dc_map_config(priv, IPU_DC_MAP_BGR24, 1, 15, 0xff); /* green */
 ipu_dc_map_config(priv, IPU_DC_MAP_BGR24, 0, 23, 0xff); /* blue */


Shouldn't rgb666 and bgr666 do the same?
Currently we have,

/* bgr666 */
 ipu_dc_map_clear(priv, IPU_DC_MAP_BGR666);
 ipu_dc_map_config(priv, IPU_DC_MAP_BGR666, 0, 5, 0xfc); /* blue */
 ipu_dc_map_config(priv, IPU_DC_MAP_BGR666, 1, 11, 0xfc); /* 
green */
 ipu_dc_map_config(priv, IPU_DC_MAP_BGR666, 2, 17, 0xfc); /* red */

Where I'd expect to see
/* bgr666 */
 ipu_dc_map_clear(priv, IPU_DC_MAP_BGR666);
 ipu_dc_map_config(priv, IPU_DC_MAP_BGR666, 0, 17, 0xfc); /* blue */
 ipu_dc_map_config(priv, IPU_DC_MAP_BGR666, 1, 11, 0xfc); /* 
green */
 ipu_dc_map_config(priv, IPU_DC_MAP_BGR666, 2, 5, 0xfc); /* red */


So, it looks like you are adding a duplicate of bgr666 because bgr666 is 
wrong.
Also, I'd prefer to keep the entries in 0,1,2 byte number order(blue, 
green, red,
assuming byte 0 is always blue) so that duplicates are easier to spot.

Not related to this patch, but the comments on gbr24 appear wrong as well.

/* gbr24 */
 ipu_dc_map_clear(priv, IPU_DC_MAP_GBR24);
 ipu_dc_map_config(priv, IPU_DC_MAP_GBR24, 2, 15, 0xff); /* green */
 ipu_dc_map_config(priv, IPU_DC_MAP_GBR24, 1, 7, 0xff); /* blue */
 ipu_dc_map_config(priv, IPU_DC_MAP_GBR24, 0, 23, 0xff); /* red */

Should be
/* brg24 */
 ipu_dc_map_clear(priv, IPU_DC_MAP_BRG24);
 ipu_dc_map_config(priv, IPU_DC_MAP_BRG24, 0, 23, 0xff); /* blue*/
 ipu_dc_map_config(priv, IPU_DC_MAP_BRG24, 1, 7, 0xff); /* green */
 ipu_dc_map_config(priv, IPU_DC_MAP_BRG24, 2, 15, 0xff); /* red */

Of course, my understanding may be totally wrong. If so, please show me 
the light!


Troy

[PATCHv4][ 3/7] staging: imx-drm: Add RGB666 support for parallel display.

[Denis Carikli]

Cc: Rob Herring 
Cc: Pawel Moll 
Cc: Mark Rutland 
Cc: Stephen Warren 
Cc: Ian Campbell 
Cc: devicetree at vger.kernel.org
Cc: Greg Kroah-Hartman 
Cc: driverdev-devel at linuxdriverproject.org
Cc: David Airlie 
Cc: dri-devel at lists.freedesktop.org
Cc: Mauro Carvalho Chehab 
Cc: Laurent Pinchart 
Cc: linux-media at vger.kernel.org
Cc: Sascha Hauer 
Cc: Shawn Guo 
Cc: linux-arm-kernel at lists.infradead.org
Cc: Eric B?nard 
Signed-off-by: Denis Carikli 
---
ChangeLog v2->v3:
- Added some interested people in the Cc list.
- Removed the commit message long desciption that was just a copy of the short
  description.
- Rebased the patch.
- Fixed a copy-paste error in the ipu_dc_map_clear parameter.
---
 .../bindings/staging/imx-drm/fsl-imx-drm.txt   |2 +-
 drivers/staging/imx-drm/ipu-v3/ipu-dc.c|9 +
 drivers/staging/imx-drm/parallel-display.c |2 ++
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt 
b/Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt
index b876d49..2d24425 100644
--- a/Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt
+++ b/Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt
@@ -29,7 +29,7 @@ Required properties:
 - crtc: the crtc this display is connected to, see below
 Optional properties:
 - interface_pix_fmt: How this display is connected to the
-  crtc. Currently supported types: "rgb24", "rgb565", "bgr666"
+  crtc. Currently supported types: "rgb24", "rgb565", "bgr666", "rgb666"
 - edid: verbatim EDID data block describing attached display.
 - ddc: phandle describing the i2c bus handling the display data
   channel
diff --git a/drivers/staging/imx-drm/ipu-v3/ipu-dc.c 
b/drivers/staging/imx-drm/ipu-v3/ipu-dc.c
index d0e3bc3..bcc7680 100644
--- a/drivers/staging/imx-drm/ipu-v3/ipu-dc.c
+++ b/drivers/staging/imx-drm/ipu-v3/ipu-dc.c
@@ -92,6 +92,7 @@ enum ipu_dc_map {
IPU_DC_MAP_GBR24, /* TVEv2 */
IPU_DC_MAP_BGR666,
IPU_DC_MAP_BGR24,
+   IPU_DC_MAP_RGB666,
 };

 struct ipu_dc {
@@ -155,6 +156,8 @@ static int ipu_pixfmt_to_map(u32 fmt)
return IPU_DC_MAP_BGR666;
case V4L2_PIX_FMT_BGR24:
return IPU_DC_MAP_BGR24;
+   case V4L2_PIX_FMT_RGB666:
+   return IPU_DC_MAP_RGB666;
default:
return -EINVAL;
}
@@ -404,6 +407,12 @@ int ipu_dc_init(struct ipu_soc *ipu, struct device *dev,
ipu_dc_map_config(priv, IPU_DC_MAP_BGR24, 1, 15, 0xff); /* green */
ipu_dc_map_config(priv, IPU_DC_MAP_BGR24, 0, 23, 0xff); /* blue */

+   /* rgb666 */
+   ipu_dc_map_clear(priv, IPU_DC_MAP_RGB666);
+   ipu_dc_map_config(priv, IPU_DC_MAP_RGB666, 2, 17, 0xfc); /* red */
+   ipu_dc_map_config(priv, IPU_DC_MAP_RGB666, 1, 11, 0xfc); /* green */
+   ipu_dc_map_config(priv, IPU_DC_MAP_RGB666, 0, 5, 0xfc); /* blue */
+
return 0;
 }

diff --git a/drivers/staging/imx-drm/parallel-display.c 
b/drivers/staging/imx-drm/parallel-display.c
index b34f3a3..46b6fce 100644
--- a/drivers/staging/imx-drm/parallel-display.c
+++ b/drivers/staging/imx-drm/parallel-display.c
@@ -248,6 +248,8 @@ static int imx_pd_probe(struct platform_device *pdev)
imxpd->interface_pix_fmt = V4L2_PIX_FMT_RGB565;
else if (!strcmp(fmt, "bgr666"))
imxpd->interface_pix_fmt = V4L2_PIX_FMT_BGR666;
+   else if (!strcmp(fmt, "rgb666"))
+   imxpd->interface_pix_fmt = V4L2_PIX_FMT_RGB666;
}

imxpd->dev = >dev;
-- 
1.7.9.5