| Message ID | 1384334603-14208-3-git-send-email-denis@eukrea.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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: 1 11 7 21 65 0 BBBBBBGGGGGGRRRRRR 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.
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: > > 1 11 > 7 21 65 0 > BBBBBBGGGGGGRRRRRR > > 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 1 2 Bit 7 6 5 4 3 2 1 0 7 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 1 2 Bit 7 6 5 4 3 2 1 0 7 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.
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 = &pdev->dev;
Cc: Rob Herring <rob.herring@calxeda.com> Cc: Pawel Moll <pawel.moll@arm.com> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Stephen Warren <swarren@wwwdotorg.org> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk> Cc: devicetree@vger.kernel.org Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: driverdev-devel@linuxdriverproject.org Cc: David Airlie <airlied@linux.ie> Cc: dri-devel@lists.freedesktop.org Cc: Mauro Carvalho Chehab <m.chehab@samsung.com> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: linux-media@vger.kernel.org Cc: Sascha Hauer <kernel@pengutronix.de> Cc: Shawn Guo <shawn.guo@linaro.org> Cc: linux-arm-kernel@lists.infradead.org Cc: Eric Bénard <eric@eukrea.com> Signed-off-by: Denis Carikli <denis@eukrea.com> --- 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(-)