[PATCH 0/2] media: imx: add capture support for RGB565_2X8 on parallel bus

[PATCH 0/2] media: imx: add capture support for RGB565_2X8 on parallel bus

[Jan Luebbe]

The IPU can only capture RGB565 with two 8-bit cycles in bayer/generic
mode on the parallel bus, compared to a specific mode on MIPI CSI-2.
To handle this, we extend imx_media_pixfmt with a cycles per pixel
field, which is used for generic formats on the parallel bus.

Before actually adding RGB565_2X8 support for the parallel bus, this
series simplifies handing of the the different configurations for RGB565
between parallel and MIPI CSI-2 in imx-media-capture. This avoids having
to explicitly pass on the format in the second patch.

Jan Luebbe (2):
  media: imx: capture: refactor enum_/try_fmt
  media: imx: add support for RGB565_2X8 on parallel bus

 drivers/staging/media/imx/imx-media-capture.c | 38 +++++++--------
 drivers/staging/media/imx/imx-media-csi.c     | 47 +++++++++++++++++--
 drivers/staging/media/imx/imx-media-utils.c   |  1 +
 drivers/staging/media/imx/imx-media.h         |  2 +
 4 files changed, 63 insertions(+), 25 deletions(-)

-- 
2.17.0

[PATCH 1/2] media: imx: capture: refactor enum_/try_fmt

[Jan Luebbe]

By checking and handling the internal IPU formats (ARGB or AYUV) first,
we don't need to check whether it's a bayer format, as we can default to
passing the input format on in all other cases.

This simplifies handling the different configurations for RGB565 between
parallel and MIPI CSI-2, as we don't need to check the details of the
format anymore.

Signed-off-by: Jan Luebbe <jlu@pengutronix.de>
---
 drivers/staging/media/imx/imx-media-capture.c | 38 +++++++++----------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
index 0ccabe04b0e1..64c23ef92931 100644
--- a/drivers/staging/media/imx/imx-media-capture.c
+++ b/drivers/staging/media/imx/imx-media-capture.c
@@ -170,23 +170,22 @@ static int capture_enum_fmt_vid_cap(struct file *file, void *fh,
 	}
 
 	cc_src = imx_media_find_ipu_format(fmt_src.format.code, CS_SEL_ANY);
-	if (!cc_src)
-		cc_src = imx_media_find_mbus_format(fmt_src.format.code,
-						    CS_SEL_ANY, true);
-	if (!cc_src)
-		return -EINVAL;
-
-	if (cc_src->bayer) {
-		if (f->index != 0)
-			return -EINVAL;
-		fourcc = cc_src->fourcc;
-	} else {
+	if (cc_src) {
 		u32 cs_sel = (cc_src->cs == IPUV3_COLORSPACE_YUV) ?
 			CS_SEL_YUV : CS_SEL_RGB;
 
 		ret = imx_media_enum_format(&fourcc, f->index, cs_sel);
 		if (ret)
 			return ret;
+	} else {
+		cc_src = imx_media_find_mbus_format(fmt_src.format.code,
+						    CS_SEL_ANY, true);
+		if (WARN_ON(!cc_src))
+			return -EINVAL;
+
+		if (f->index != 0)
+			return -EINVAL;
+		fourcc = cc_src->fourcc;
 	}
 
 	f->pixelformat = fourcc;
@@ -219,15 +218,7 @@ static int capture_try_fmt_vid_cap(struct file *file, void *fh,
 		return ret;
 
 	cc_src = imx_media_find_ipu_format(fmt_src.format.code, CS_SEL_ANY);
-	if (!cc_src)
-		cc_src = imx_media_find_mbus_format(fmt_src.format.code,
-						    CS_SEL_ANY, true);
-	if (!cc_src)
-		return -EINVAL;
-
-	if (cc_src->bayer) {
-		cc = cc_src;
-	} else {
+	if (cc_src) {
 		u32 fourcc, cs_sel;
 
 		cs_sel = (cc_src->cs == IPUV3_COLORSPACE_YUV) ?
@@ -239,6 +230,13 @@ static int capture_try_fmt_vid_cap(struct file *file, void *fh,
 			imx_media_enum_format(&fourcc, 0, cs_sel);
 			cc = imx_media_find_format(fourcc, cs_sel, false);
 		}
+	} else {
+		cc_src = imx_media_find_mbus_format(fmt_src.format.code,
+						    CS_SEL_ANY, true);
+		if (WARN_ON(!cc_src))
+			return -EINVAL;
+
+		cc = cc_src;
 	}
 
 	imx_media_mbus_fmt_to_pix_fmt(&f->fmt.pix, &fmt_src.format, cc);
-- 
2.17.0

[PATCH 2/2] media: imx: add support for RGB565_2X8 on parallel bus

[Jan Luebbe]

The IPU can only capture RGB565 with two 8-bit cycles in bayer/generic
mode on the parallel bus, compared to a specific mode on MIPI CSI-2.
To handle this, we extend imx_media_pixfmt with a cycles per pixel
field, which is used for generic formats on the parallel bus.

Based on the selected format and bus, we then update the width to
account for the multiple cycles per pixel.

Signed-off-by: Jan Luebbe <jlu@pengutronix.de>
---
 drivers/staging/media/imx/imx-media-csi.c   | 47 ++++++++++++++++++---
 drivers/staging/media/imx/imx-media-utils.c |  1 +
 drivers/staging/media/imx/imx-media.h       |  2 +
 3 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index 08b636084286..aeb6801e5bd2 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -370,6 +370,7 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
 	struct v4l2_mbus_framefmt *infmt;
 	struct ipu_image image;
 	u32 passthrough_bits;
+	u32 passthrough_cycles;
 	dma_addr_t phys[2];
 	bool passthrough;
 	u32 burst_size;
@@ -395,6 +396,7 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
 	 * - raw bayer formats
 	 * - the CSI is receiving from a 16-bit parallel bus
 	 */
+	passthrough_cycles = 1;
 	switch (image.pix.pixelformat) {
 	case V4L2_PIX_FMT_SBGGR8:
 	case V4L2_PIX_FMT_SGBRG8:
@@ -431,6 +433,16 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
 		passthrough = is_parallel_16bit_bus(&priv->upstream_ep);
 		passthrough_bits = 16;
 		break;
+	case V4L2_PIX_FMT_RGB565:
+		/* without CSI2 we can only use passthrough mode */
+		if (priv->upstream_ep.bus_type != V4L2_MBUS_CSI2) {
+			burst_size = 16;
+			passthrough = true;
+			passthrough_bits = 8;
+			passthrough_cycles = 2;
+			break;
+		};
+		/* fallthrough */
 	default:
 		burst_size = (image.pix.width & 0xf) ? 8 : 16;
 		passthrough = is_parallel_16bit_bus(&priv->upstream_ep);
@@ -439,7 +451,8 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
 	}
 
 	if (passthrough) {
-		ipu_cpmem_set_resolution(priv->idmac_ch, image.rect.width,
+		ipu_cpmem_set_resolution(priv->idmac_ch,
+					 image.rect.width * passthrough_cycles,
 					 image.rect.height);
 		ipu_cpmem_set_stride(priv->idmac_ch, image.pix.bytesperline);
 		ipu_cpmem_set_buffer(priv->idmac_ch, 0, image.phys0);
@@ -632,9 +645,12 @@ static int csi_setup(struct csi_priv *priv)
 	struct v4l2_mbus_framefmt *infmt, *outfmt;
 	struct v4l2_mbus_config mbus_cfg;
 	struct v4l2_mbus_framefmt if_fmt;
+	struct imx_media_pixfmt *outcc;
+	struct v4l2_rect crop;
 
 	infmt = &priv->format_mbus[CSI_SINK_PAD];
 	outfmt = &priv->format_mbus[priv->active_output_pad];
+	outcc = priv->cc[priv->active_output_pad];
 
 	/* compose mbus_config from the upstream endpoint */
 	mbus_cfg.type = priv->upstream_ep.bus_type;
@@ -648,8 +664,18 @@ static int csi_setup(struct csi_priv *priv)
 	 */
 	if_fmt = *infmt;
 	if_fmt.field = outfmt->field;
+	crop = priv->crop;
+
+	/*
+	 * if cycles is set, we need to handle this over multiple cycles as
+	 * generic/bayer data
+	 */
+	if ((priv->upstream_ep.bus_type != V4L2_MBUS_CSI2) && outcc->cycles) {
+		if_fmt.width *= outcc->cycles;
+		crop.width *= outcc->cycles;
+	}
 
-	ipu_csi_set_window(priv->csi, &priv->crop);
+	ipu_csi_set_window(priv->csi, &crop);
 
 	ipu_csi_set_downsize(priv->csi,
 			     priv->crop.width == 2 * priv->compose.width,
@@ -1029,7 +1055,9 @@ static int csi_link_validate(struct v4l2_subdev *sd,
 	incc = priv->cc[CSI_SINK_PAD];
 
 	if (priv->dest != IPU_CSI_DEST_IDMAC &&
-	    (incc->bayer || is_parallel_16bit_bus(&upstream_ep))) {
+	    (incc->bayer ||
+	     is_parallel_16bit_bus(&upstream_ep) ||
+	     (!is_csi2 && incc->cycles))) {
 		v4l2_err(&priv->sd,
 			 "bayer/16-bit parallel buses must go to IDMAC pad\n");
 		ret = -EINVAL;
@@ -1131,6 +1159,7 @@ static int csi_enum_mbus_code(struct v4l2_subdev *sd,
 			      struct v4l2_subdev_mbus_code_enum *code)
 {
 	struct csi_priv *priv = v4l2_get_subdevdata(sd);
+	struct v4l2_fwnode_endpoint upstream_ep;
 	const struct imx_media_pixfmt *incc;
 	struct v4l2_mbus_framefmt *infmt;
 	int ret = 0;
@@ -1147,7 +1176,14 @@ static int csi_enum_mbus_code(struct v4l2_subdev *sd,
 		break;
 	case CSI_SRC_PAD_DIRECT:
 	case CSI_SRC_PAD_IDMAC:
-		if (incc->bayer) {
+		ret = csi_get_upstream_endpoint(priv, &upstream_ep);
+		if (ret) {
+			v4l2_err(&priv->sd, "failed to find upstream endpoint\n");
+			goto out;
+		}
+
+		if (incc->bayer ||
+		    (upstream_ep.bus_type != V4L2_MBUS_CSI2 && incc->cycles)) {
 			if (code->index != 0) {
 				ret = -EINVAL;
 				goto out;
@@ -1286,7 +1322,8 @@ static void csi_try_fmt(struct csi_priv *priv,
 		sdformat->format.width = compose->width;
 		sdformat->format.height = compose->height;
 
-		if (incc->bayer) {
+		if (incc->bayer ||
+		    (upstream_ep->bus_type != V4L2_MBUS_CSI2 && incc->cycles)) {
 			sdformat->format.code = infmt->code;
 			*cc = incc;
 		} else {
diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index 7ec2db84451c..8aa13403b09d 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -78,6 +78,7 @@ static const struct imx_media_pixfmt rgb_formats[] = {
 		.codes  = {MEDIA_BUS_FMT_RGB565_2X8_LE},
 		.cs     = IPUV3_COLORSPACE_RGB,
 		.bpp    = 16,
+		.cycles = 2,
 	}, {
 		.fourcc	= V4L2_PIX_FMT_RGB24,
 		.codes  = {
diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
index e945e0ed6dd6..57bd094cf765 100644
--- a/drivers/staging/media/imx/imx-media.h
+++ b/drivers/staging/media/imx/imx-media.h
@@ -62,6 +62,8 @@ struct imx_media_pixfmt {
 	u32     fourcc;
 	u32     codes[4];
 	int     bpp;     /* total bpp */
+	/* cycles per pixel for generic (bayer) formats for the parallel bus */
+	int	cycles;
 	enum ipu_color_space cs;
 	bool    planar;  /* is a planar format */
 	bool    bayer;   /* is a raw bayer format */
-- 
2.17.0

Re: [PATCH 2/2] media: imx: add support for RGB565_2X8 on parallel bus

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3269 bytes --]

Hi Jan,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.17-rc3 next-20180503]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Jan-Luebbe/media-imx-add-capture-support-for-RGB565_2X8-on-parallel-bus/20180504-120003
base:   git://linuxtv.org/media_tree.git master
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=xtensa 

All warnings (new ones prefixed by >>):

   drivers/staging/media/imx/imx-media-csi.c: In function 'csi_setup':
>> drivers/staging/media/imx/imx-media-csi.c:652:8: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     outcc = priv->cc[priv->active_output_pad];
           ^

vim +/const +652 drivers/staging/media/imx/imx-media-csi.c

   640	
   641	/* Update the CSI whole sensor and active windows */
   642	static int csi_setup(struct csi_priv *priv)
   643	{
   644		struct v4l2_mbus_framefmt *infmt, *outfmt;
   645		struct v4l2_mbus_config mbus_cfg;
   646		struct v4l2_mbus_framefmt if_fmt;
   647		struct imx_media_pixfmt *outcc;
   648		struct v4l2_rect crop;
   649	
   650		infmt = &priv->format_mbus[CSI_SINK_PAD];
   651		outfmt = &priv->format_mbus[priv->active_output_pad];
 > 652		outcc = priv->cc[priv->active_output_pad];
   653	
   654		/* compose mbus_config from the upstream endpoint */
   655		mbus_cfg.type = priv->upstream_ep.bus_type;
   656		mbus_cfg.flags = (priv->upstream_ep.bus_type == V4L2_MBUS_CSI2) ?
   657			priv->upstream_ep.bus.mipi_csi2.flags :
   658			priv->upstream_ep.bus.parallel.flags;
   659	
   660		/*
   661		 * we need to pass input frame to CSI interface, but
   662		 * with translated field type from output format
   663		 */
   664		if_fmt = *infmt;
   665		if_fmt.field = outfmt->field;
   666		crop = priv->crop;
   667	
   668		/*
   669		 * if cycles is set, we need to handle this over multiple cycles as
   670		 * generic/bayer data
   671		 */
   672		if ((priv->upstream_ep.bus_type != V4L2_MBUS_CSI2) && outcc->cycles) {
   673			if_fmt.width *= outcc->cycles;
   674			crop.width *= outcc->cycles;
   675		}
   676	
   677		ipu_csi_set_window(priv->csi, &crop);
   678	
   679		ipu_csi_set_downsize(priv->csi,
   680				     priv->crop.width == 2 * priv->compose.width,
   681				     priv->crop.height == 2 * priv->compose.height);
   682	
   683		ipu_csi_init_interface(priv->csi, &mbus_cfg, &if_fmt);
   684	
   685		ipu_csi_set_dest(priv->csi, priv->dest);
   686	
   687		if (priv->dest == IPU_CSI_DEST_IDMAC)
   688			ipu_csi_set_skip_smfc(priv->csi, priv->skip->skip_smfc,
   689					      priv->skip->max_ratio - 1, 0);
   690	
   691		ipu_csi_dump(priv->csi);
   692	
   693		return 0;
   694	}
   695	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 53302 bytes --]

Re: [PATCH 2/2] media: imx: add support for RGB565_2X8 on parallel bus

[Jan Lübbe]

On Fri, 2018-05-04 at 13:07 +0800, kbuild test robot wrote:
>    drivers/staging/media/imx/imx-media-csi.c: In function
> 'csi_setup':
> >> drivers/staging/media/imx/imx-media-csi.c:652:8: warning:
> assignment discards 'const' qualifier from pointer target type [-
> Wdiscarded-qualifiers]
>      outcc = priv->cc[priv->active_output_pad];
>            ^

I've fixed this and the unneeded semicolon for the next round.

Regards,
Jan
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] media: imx: fix semicolon.cocci warnings

[kbuild test robot]

From: Fengguang Wu <fengguang.wu@intel.com>

drivers/staging/media/imx/imx-media-csi.c:443:3-4: Unneeded semicolon


 Remove unneeded semicolon.

Generated by: scripts/coccinelle/misc/semicolon.cocci

Fixes: ad2cc62e77ef ("media: imx: add support for RGB565_2X8 on parallel bus")
CC: Jan Luebbe <jlu@pengutronix.de>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---

 imx-media-csi.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -440,7 +440,7 @@ static int csi_idmac_setup_channel(struc
 			passthrough_bits = 8;
 			passthrough_cycles = 2;
 			break;
-		};
+		}
 		/* fallthrough */
 	default:
 		burst_size = (image.pix.width & 0xf) ? 8 : 16;

Re: [PATCH 2/2] media: imx: add support for RGB565_2X8 on parallel bus

[kbuild test robot]

Hi Jan,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.17-rc3 next-20180503]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Jan-Luebbe/media-imx-add-capture-support-for-RGB565_2X8-on-parallel-bus/20180504-120003
base:   git://linuxtv.org/media_tree.git master


coccinelle warnings: (new ones prefixed by >>)

>> drivers/staging/media/imx/imx-media-csi.c:443:3-4: Unneeded semicolon

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH 2/2] media: imx: add support for RGB565_2X8 on parallel bus

[kbuild test robot]

Hi Jan,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.17-rc3 next-20180503]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Jan-Luebbe/media-imx-add-capture-support-for-RGB565_2X8-on-parallel-bus/20180504-120003
base:   git://linuxtv.org/media_tree.git master
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)


vim +652 drivers/staging/media/imx/imx-media-csi.c

   640	
   641	/* Update the CSI whole sensor and active windows */
   642	static int csi_setup(struct csi_priv *priv)
   643	{
   644		struct v4l2_mbus_framefmt *infmt, *outfmt;
   645		struct v4l2_mbus_config mbus_cfg;
   646		struct v4l2_mbus_framefmt if_fmt;
   647		struct imx_media_pixfmt *outcc;
   648		struct v4l2_rect crop;
   649	
   650		infmt = &priv->format_mbus[CSI_SINK_PAD];
   651		outfmt = &priv->format_mbus[priv->active_output_pad];
 > 652		outcc = priv->cc[priv->active_output_pad];
   653	
   654		/* compose mbus_config from the upstream endpoint */
   655		mbus_cfg.type = priv->upstream_ep.bus_type;
   656		mbus_cfg.flags = (priv->upstream_ep.bus_type == V4L2_MBUS_CSI2) ?
   657			priv->upstream_ep.bus.mipi_csi2.flags :
   658			priv->upstream_ep.bus.parallel.flags;
   659	
   660		/*
   661		 * we need to pass input frame to CSI interface, but
   662		 * with translated field type from output format
   663		 */
   664		if_fmt = *infmt;
   665		if_fmt.field = outfmt->field;
   666		crop = priv->crop;
   667	
   668		/*
   669		 * if cycles is set, we need to handle this over multiple cycles as
   670		 * generic/bayer data
   671		 */
   672		if ((priv->upstream_ep.bus_type != V4L2_MBUS_CSI2) && outcc->cycles) {
   673			if_fmt.width *= outcc->cycles;
   674			crop.width *= outcc->cycles;
   675		}
   676	
   677		ipu_csi_set_window(priv->csi, &crop);
   678	
   679		ipu_csi_set_downsize(priv->csi,
   680				     priv->crop.width == 2 * priv->compose.width,
   681				     priv->crop.height == 2 * priv->compose.height);
   682	
   683		ipu_csi_init_interface(priv->csi, &mbus_cfg, &if_fmt);
   684	
   685		ipu_csi_set_dest(priv->csi, priv->dest);
   686	
   687		if (priv->dest == IPU_CSI_DEST_IDMAC)
   688			ipu_csi_set_skip_smfc(priv->csi, priv->skip->skip_smfc,
   689					      priv->skip->max_ratio - 1, 0);
   690	
   691		ipu_csi_dump(priv->csi);
   692	
   693		return 0;
   694	}
   695	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH 0/2] media: imx: add capture support for RGB565_2X8 on parallel bus

[Steve Longerbeam]

Hi Jan, Philipp,

I reviewed this patch series, and while I don't have any
objections to the code-level changes, but my question
is more specifically about how the IPU/CSI deals with
receiving RGB565 over a parallel bus.

My understanding was that if the CSI receives RGB565
over a parallel 8-bit sensor bus, the CSI_SENS_DATA_FORMAT
register field is programmed to RGB565, and the CSI outputs
ARGB8888. Then IDMAC component packing can be setup to
write pixels to memory as RGB565. Does that not work?

Assuming that above does not work (and indeed parallel RGB565
must be handled as pass-through), then I think support for capturing
parallel RGB555 as pass-through should be added to this series as
well.

Also what about RGB565 over a 16-bit parallel sensor bus? The
reference manual hints that perhaps this could be treated as
non-passthrough ("on the fly processing"), e.g. could be passed
on to the IC pre-processor:

     16 bit RGB565
         This is the only mode that allows on the fly processing of 16 
bit data. In this mode the
         CSI is programmed to receive 16 bit generic data. In this mode 
the interface is
         restricted to be in "non-gated mode" and the CSI#_DATA_SOURCE 
bit has to be set
         If the external device is 24bit - the user can connect a 16 bit 
sample of it (RGB565
         format). The IPU has to be configured in the same way as the 
case of
         CSI#_SENS_DATA_FORMAT=RGB565


Thanks,
Steve


On 05/03/2018 09:41 AM, Jan Luebbe wrote:
> The IPU can only capture RGB565 with two 8-bit cycles in bayer/generic
> mode on the parallel bus, compared to a specific mode on MIPI CSI-2.
> To handle this, we extend imx_media_pixfmt with a cycles per pixel
> field, which is used for generic formats on the parallel bus.
>
> Before actually adding RGB565_2X8 support for the parallel bus, this
> series simplifies handing of the the different configurations for RGB565
> between parallel and MIPI CSI-2 in imx-media-capture. This avoids having
> to explicitly pass on the format in the second patch.
>
> Jan Luebbe (2):
>    media: imx: capture: refactor enum_/try_fmt
>    media: imx: add support for RGB565_2X8 on parallel bus
>
>   drivers/staging/media/imx/imx-media-capture.c | 38 +++++++--------
>   drivers/staging/media/imx/imx-media-csi.c     | 47 +++++++++++++++++--
>   drivers/staging/media/imx/imx-media-utils.c   |  1 +
>   drivers/staging/media/imx/imx-media.h         |  2 +
>   4 files changed, 63 insertions(+), 25 deletions(-)
>

Re: [PATCH 0/2] media: imx: add capture support for RGB565_2X8 on parallel bus

[Jan Lübbe]

Hi Steve,

thanks for reviewing!

On Sat, 2018-05-05 at 15:22 -0700, Steve Longerbeam wrote:
> I reviewed this patch series, and while I don't have any
> objections to the code-level changes, but my question
> is more specifically about how the IPU/CSI deals with
> receiving RGB565 over a parallel bus.
> 
> My understanding was that if the CSI receives RGB565
> over a parallel 8-bit sensor bus, the CSI_SENS_DATA_FORMAT
> register field is programmed to RGB565, and the CSI outputs
> ARGB8888. Then IDMAC component packing can be setup to
> write pixels to memory as RGB565. Does that not work?

This was our first thought too. As far as we can see in our
experiments, that mode doesn't actually work for the parallel bus.
Philipp's interpretation is that this mode is only intended for use
with the MIPI-CSI2 input.

> Assuming that above does not work (and indeed parallel RGB565
> must be handled as pass-through), then I think support for capturing
> parallel RGB555 as pass-through should be added to this series as
> well.

I don't have a sensor which produces RGB555, so it wouldn't be able to
test it.

> Also what about RGB565 over a 16-bit parallel sensor bus? The
> reference manual hints that perhaps this could be treated as
> non-passthrough ("on the fly processing"), e.g. could be passed
> on to the IC pre-processor:
> 
>      16 bit RGB565
>          This is the only mode that allows on the fly processing of 16 bit data. In this mode the
>          CSI is programmed to receive 16 bit generic data. In this mode the interface is
>          restricted to be in "non-gated mode" and the CSI#_DATA_SOURCE bit has to be set
>          If the external device is 24bit - the user can connect a 16 bit sample of it (RGB565
>          format). The IPU has to be configured in the same way as the case of
>          CSI#_SENS_DATA_FORMAT=RGB565

I've not looked at this case, as I don't have a sensor with that format
either. :/

Thanks,
Jan
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 0/2] media: imx: add capture support for RGB565_2X8 on parallel bus

[Steve Longerbeam]

On 05/07/2018 07:23 AM, Jan Lübbe wrote:
> Hi Steve,
>
> thanks for reviewing!
>
> On Sat, 2018-05-05 at 15:22 -0700, Steve Longerbeam wrote:
>> I reviewed this patch series, and while I don't have any
>> objections to the code-level changes, but my question
>> is more specifically about how the IPU/CSI deals with
>> receiving RGB565 over a parallel bus.
>>
>> My understanding was that if the CSI receives RGB565
>> over a parallel 8-bit sensor bus, the CSI_SENS_DATA_FORMAT
>> register field is programmed to RGB565, and the CSI outputs
>> ARGB8888. Then IDMAC component packing can be setup to
>> write pixels to memory as RGB565. Does that not work?
> This was our first thought too. As far as we can see in our
> experiments, that mode doesn't actually work for the parallel bus.
> Philipp's interpretation is that this mode is only intended for use
> with the MIPI-CSI2 input.

Ok, that was my suspicion on reading this as well. And it's likely
that the other sensor formats on parallel busses require pass-through,
e.g. anything other than UYVY_2x8 and YUYV_2x8 requires
pass-through.

In other words, if the sensor bus is parallel, only 8-bit bus UYVY_2x8
and YUYV_2x8 can be routed to the IC pad or component packed/unpacked
by the IDMAC pad. All other sensor formats on a parallel bus (8 or 16 
bit) must
be sent to IDMAC pad as pass-through.

I think the code can be simplified/made more readable because of this,
something like:

static inline bool is_parallel_bus(struct v4l2_fwnode_endpoint *ep)
{
         return ep->bus_type != V4L2_MBUS_CSI2;
}

static inline bool requires_pass_through(
     struct v4l2_fwnode_endpoint *ep,
     struct v4l2_mbus_framefmt *infmt,
     const struct imx_media_pixfmt *incc) {
         return incc->bayer || (is_parallel_bus(ep) && infmt->code != 
UYVY_2x8 && infmt->code != YUYV_2x8);
}


Then requires_pass_through() can be used everywhere we need to
determine the pass-though requirement.

Also, there's something wrong with the 'switch (image.pix.pixelformat) 
{...}'
block in csi_idmac_setup_channel(). Pass-though, burst size, pass-though 
bits,
should be determined by input media-bus code, not final capture V4L2 pix
format.

Steve

>> Assuming that above does not work (and indeed parallel RGB565
>> must be handled as pass-through), then I think support for capturing
>> parallel RGB555 as pass-through should be added to this series as
>> well.
> I don't have a sensor which produces RGB555, so it wouldn't be able to
> test it.

Understood, but for code readability and consistency I think the code
can be cleaned up as above.


>> Also what about RGB565 over a 16-bit parallel sensor bus? The
>> reference manual hints that perhaps this could be treated as
>> non-passthrough ("on the fly processing"), e.g. could be passed
>> on to the IC pre-processor:
>>
>>       16 bit RGB565
>>           This is the only mode that allows on the fly processing of 16 bit data. In this mode the
>>           CSI is programmed to receive 16 bit generic data. In this mode the interface is
>>           restricted to be in "non-gated mode" and the CSI#_DATA_SOURCE bit has to be set
>>           If the external device is 24bit - the user can connect a 16 bit sample of it (RGB565
>>           format). The IPU has to be configured in the same way as the case of
>>           CSI#_SENS_DATA_FORMAT=RGB565
> I've not looked at this case, as I don't have a sensor with that format
> either. :/

Ok, I was just curious if you knew anything more about this. Let's
ignore it :)

Steve

Re: [PATCH 0/2] media: imx: add capture support for RGB565_2X8 on parallel bus

[Jan Lübbe]

Hi,

On Mon, 2018-05-07 at 11:21 -0700, Steve Longerbeam wrote:
> On 05/07/2018 07:23 AM, Jan Lübbe wrote:
> > On Sat, 2018-05-05 at 15:22 -0700, Steve Longerbeam wrote:
> > > I reviewed this patch series, and while I don't have any
> > > objections to the code-level changes, but my question
> > > is more specifically about how the IPU/CSI deals with
> > > receiving RGB565 over a parallel bus.
> > > 
> > > My understanding was that if the CSI receives RGB565
> > > over a parallel 8-bit sensor bus, the CSI_SENS_DATA_FORMAT
> > > register field is programmed to RGB565, and the CSI outputs
> > > ARGB8888. Then IDMAC component packing can be setup to
> > > write pixels to memory as RGB565. Does that not work?
> > 
> > This was our first thought too. As far as we can see in our
> > experiments, that mode doesn't actually work for the parallel bus.
> > Philipp's interpretation is that this mode is only intended for use
> > with the MIPI-CSI2 input.
> 
> Ok, that was my suspicion on reading this as well. And it's likely
> that the other sensor formats on parallel busses require pass-through,
> e.g. anything other than UYVY_2x8 and YUYV_2x8 requires
> pass-through.

OK.

> In other words, if the sensor bus is parallel, only 8-bit bus UYVY_2x8
> and YUYV_2x8 can be routed to the IC pad or component packed/unpacked
> by the IDMAC pad. All other sensor formats on a parallel bus (8 or 16 
> bit) must be sent to IDMAC pad as pass-through.
> 
> I think the code can be simplified/made more readable because of this,
> something like:
> 
> static inline bool is_parallel_bus(struct v4l2_fwnode_endpoint *ep)
> {
>          return ep->bus_type != V4L2_MBUS_CSI2;
> }
> 
> static inline bool requires_pass_through(
>      struct v4l2_fwnode_endpoint *ep,
>      struct v4l2_mbus_framefmt *infmt,
>      const struct imx_media_pixfmt *incc) {
>          return incc->bayer || (is_parallel_bus(ep) && infmt->code != 
> UYVY_2x8 && infmt->code != YUYV_2x8);
> }
> 
> 
> Then requires_pass_through() can be used everywhere we need to
> determine the pass-though requirement.

OK, i've added these helper functions. In csi_link_validate() we don't
have the infmt handy, but as the downstream elements check if they have
a native format anyway, this check is redundant and so i've dropped it.

> Also, there's something wrong with the 'switch (image.pix.pixelformat) 
> {...}' block in csi_idmac_setup_channel(). Pass-though, burst size, pass-though 
> bits, should be determined by input media-bus code, not final capture V4L2 pix
> format.

I just followed the existing code there, which already configures all
of these.

> > > Assuming that above does not work (and indeed parallel RGB565
> > > must be handled as pass-through), then I think support for capturing
> > > parallel RGB555 as pass-through should be added to this series as
> > > well.
> > 
> > I don't have a sensor which produces RGB555, so it wouldn't be able to
> > test it.
> 
> Understood, but for code readability and consistency I think the code
> can be cleaned up as above.

Yes, i've changed that for v2.

> > > Also what about RGB565 over a 16-bit parallel sensor bus? The
> > > reference manual hints that perhaps this could be treated as
> > > non-passthrough ("on the fly processing"), e.g. could be passed
> > > on to the IC pre-processor:
> > > 
> > >       16 bit RGB565
> > >           This is the only mode that allows on the fly processing of 16 bit data. In this mode the
> > >           CSI is programmed to receive 16 bit generic data. In this mode the interface is
> > >           restricted to be in "non-gated mode" and the CSI#_DATA_SOURCE bit has to be set
> > >           If the external device is 24bit - the user can connect a 16 bit sample of it (RGB565
> > >           format). The IPU has to be configured in the same way as the case of
> > >           CSI#_SENS_DATA_FORMAT=RGB565
> > 
> > I've not looked at this case, as I don't have a sensor with that format
> > either. :/
> 
> Ok, I was just curious if you knew anything more about this. Let's
> ignore it :)

OK. :)

Regards,
Jan
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 0/2] media: imx: add capture support for RGB565_2X8 on parallel bus

[Steve Longerbeam]

Hi Jan,


On 05/08/2018 07:13 AM, Jan Lübbe wrote:
> Hi,
>
> On Mon, 2018-05-07 at 11:21 -0700, Steve Longerbeam wrote:
>> In other words, if the sensor bus is parallel, only 8-bit bus UYVY_2x8
>> and YUYV_2x8 can be routed to the IC pad or component packed/unpacked
>> by the IDMAC pad. All other sensor formats on a parallel bus (8 or 16
>> bit) must be sent to IDMAC pad as pass-through.
>>
>> I think the code can be simplified/made more readable because of this,
>> something like:
>>
>> static inline bool is_parallel_bus(struct v4l2_fwnode_endpoint *ep)
>> {
>>           return ep->bus_type != V4L2_MBUS_CSI2;
>> }
>>
>> static inline bool requires_pass_through(
>>       struct v4l2_fwnode_endpoint *ep,
>>       struct v4l2_mbus_framefmt *infmt,
>>       const struct imx_media_pixfmt *incc) {
>>           return incc->bayer || (is_parallel_bus(ep) && infmt->code !=
>> UYVY_2x8 && infmt->code != YUYV_2x8);
>> }
>>
>>
>> Then requires_pass_through() can be used everywhere we need to
>> determine the pass-though requirement.
> OK, i've added these helper functions. In csi_link_validate() we don't
> have the infmt handy, but as the downstream elements check if they have
> a native format anyway, this check is redundant and so i've dropped it.

Makes sense.

>
>> Also, there's something wrong with the 'switch (image.pix.pixelformat)
>> {...}' block in csi_idmac_setup_channel(). Pass-though, burst size, pass-though
>> bits, should be determined by input media-bus code, not final capture V4L2 pix
>> format.
> I just followed the existing code there, which already configures all
> of these.

Sorry never mind, I forgot that there is a need to check for planar formats
here.

>>>> Assuming that above does not work (and indeed parallel RGB565
>>>> must be handled as pass-through), then I think support for capturing
>>>> parallel RGB555 as pass-through should be added to this series as
>>>> well.
>>> I don't have a sensor which produces RGB555, so it wouldn't be able to
>>> test it.
>> Understood, but for code readability and consistency I think the code
>> can be cleaned up as above.
> Yes, i've changed that for v2.
>
>

The new macros can be used in more places. I will respond to v2 patch.

Steve