[PATCH 0/4] OMAP3-ISP lane shifter support

[PATCH 0/4] OMAP3-ISP lane shifter support

[Michael Jones]

Add support for the ISP's lane shifter.  To use the shifter, set different
pixel formats at each end of the link at the CCDC input.

This has only been tested shifting Y12 and SBGGR12 from a parallel sensor to Y8
and SBGGR12 (respectively) at the CCDC input.  Support has also been added for
other formats and other shifting values, but is untested.  Shifting data coming
from one of the serial sensor interfaces (CSI2a, etc) is also untested.

As before, ccdc_try_format() does not check that the format at its input is
compatible with the format coming from the sensor interface. This consistency
check is first done when activating the pipeline.

These patches apply to Laurent's media-0005-omap3isp branch, based on 2.6.38-rc5

Michael Jones (4):
  v4l: add V4L2_PIX_FMT_Y12 format
  media: add 8-bit bayer formats and Y12
  omap3isp: ccdc: support Y10, Y12, SGRBG8, SBGGR8
  omap3isp: lane shifter support

 drivers/media/video/omap3-isp/isp.c      |   82 +++++++++++++++++++++++++++++-
 drivers/media/video/omap3-isp/isp.h      |    4 +-
 drivers/media/video/omap3-isp/ispccdc.c  |    6 ++-
 drivers/media/video/omap3-isp/ispvideo.c |   65 ++++++++++++++++++-----
 drivers/media/video/omap3-isp/ispvideo.h |    3 +
 include/linux/v4l2-mediabus.h            |    7 ++-
 include/linux/videodev2.h                |    1 +
 7 files changed, 148 insertions(+), 20 deletions(-)

-- 
1.7.4.1


MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

[PATCH 1/4] v4l: add V4L2_PIX_FMT_Y12 format

[Michael Jones]

Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
---
 include/linux/videodev2.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index 02da9e7..6fac463 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -288,6 +288,7 @@ struct v4l2_pix_format {
 #define V4L2_PIX_FMT_Y4      v4l2_fourcc('Y', '0', '4', ' ') /*  4  Greyscale     */
 #define V4L2_PIX_FMT_Y6      v4l2_fourcc('Y', '0', '6', ' ') /*  6  Greyscale     */
 #define V4L2_PIX_FMT_Y10     v4l2_fourcc('Y', '1', '0', ' ') /* 10  Greyscale     */
+#define V4L2_PIX_FMT_Y12     v4l2_fourcc('Y', '1', '2', ' ') /* 12  Greyscale     */
 #define V4L2_PIX_FMT_Y16     v4l2_fourcc('Y', '1', '6', ' ') /* 16  Greyscale     */
 
 /* Palette formats */
-- 
1.7.4.1


MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

Re: [PATCH 1/4] v4l: add V4L2_PIX_FMT_Y12 format

[Laurent Pinchart]

Hi Michael,

Thanks for the patch.

On Friday 04 March 2011 09:58:01 Michael Jones wrote:
> Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  include/linux/videodev2.h |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index 02da9e7..6fac463 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -288,6 +288,7 @@ struct v4l2_pix_format {
>  #define V4L2_PIX_FMT_Y4      v4l2_fourcc('Y', '0', '4', ' ') /*  4 
> Greyscale     */ #define V4L2_PIX_FMT_Y6      v4l2_fourcc('Y', '0', '6', '
> ') /*  6  Greyscale     */ #define V4L2_PIX_FMT_Y10     v4l2_fourcc('Y',
> '1', '0', ' ') /* 10  Greyscale     */ +#define V4L2_PIX_FMT_Y12    
> v4l2_fourcc('Y', '1', '2', ' ') /* 12  Greyscale     */ #define
> V4L2_PIX_FMT_Y16     v4l2_fourcc('Y', '1', '6', ' ') /* 16  Greyscale    
> */
> 
>  /* Palette formats */

-- 
Regards,

Laurent Pinchart

[PATCH 2/4] media: add 8-bit bayer formats and Y12

[Michael Jones]

Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
---
 include/linux/v4l2-mediabus.h |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/linux/v4l2-mediabus.h b/include/linux/v4l2-mediabus.h
index 7054a7a..46caecd 100644
--- a/include/linux/v4l2-mediabus.h
+++ b/include/linux/v4l2-mediabus.h
@@ -47,8 +47,9 @@ enum v4l2_mbus_pixelcode {
 	V4L2_MBUS_FMT_RGB565_2X8_BE = 0x1007,
 	V4L2_MBUS_FMT_RGB565_2X8_LE = 0x1008,
 
-	/* YUV (including grey) - next is 0x2013 */
+	/* YUV (including grey) - next is 0x2014 */
 	V4L2_MBUS_FMT_Y8_1X8 = 0x2001,
+	V4L2_MBUS_FMT_Y12_1X12 = 0x2013,
 	V4L2_MBUS_FMT_UYVY8_1_5X8 = 0x2002,
 	V4L2_MBUS_FMT_VYUY8_1_5X8 = 0x2003,
 	V4L2_MBUS_FMT_YUYV8_1_5X8 = 0x2004,
@@ -67,9 +68,11 @@ enum v4l2_mbus_pixelcode {
 	V4L2_MBUS_FMT_YUYV10_1X20 = 0x200d,
 	V4L2_MBUS_FMT_YVYU10_1X20 = 0x200e,
 
-	/* Bayer - next is 0x3013 */
+	/* Bayer - next is 0x3015 */
 	V4L2_MBUS_FMT_SBGGR8_1X8 = 0x3001,
+	V4L2_MBUS_FMT_SGBRG8_1X8 = 0x3013,
 	V4L2_MBUS_FMT_SGRBG8_1X8 = 0x3002,
+	V4L2_MBUS_FMT_SRGGB8_1X8 = 0x3014,
 	V4L2_MBUS_FMT_SBGGR10_DPCM8_1X8 = 0x300b,
 	V4L2_MBUS_FMT_SGBRG10_DPCM8_1X8 = 0x300c,
 	V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8 = 0x3009,
-- 
1.7.4.1


MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

Re: [PATCH 2/4] media: add 8-bit bayer formats and Y12

[Laurent Pinchart]

Hi Michael,

Thanks for the patch.

On Friday 04 March 2011 09:58:02 Michael Jones wrote:
> Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  include/linux/v4l2-mediabus.h |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/v4l2-mediabus.h b/include/linux/v4l2-mediabus.h
> index 7054a7a..46caecd 100644
> --- a/include/linux/v4l2-mediabus.h
> +++ b/include/linux/v4l2-mediabus.h
> @@ -47,8 +47,9 @@ enum v4l2_mbus_pixelcode {
>  	V4L2_MBUS_FMT_RGB565_2X8_BE = 0x1007,
>  	V4L2_MBUS_FMT_RGB565_2X8_LE = 0x1008,
> 
> -	/* YUV (including grey) - next is 0x2013 */
> +	/* YUV (including grey) - next is 0x2014 */
>  	V4L2_MBUS_FMT_Y8_1X8 = 0x2001,
> +	V4L2_MBUS_FMT_Y12_1X12 = 0x2013,
>  	V4L2_MBUS_FMT_UYVY8_1_5X8 = 0x2002,
>  	V4L2_MBUS_FMT_VYUY8_1_5X8 = 0x2003,
>  	V4L2_MBUS_FMT_YUYV8_1_5X8 = 0x2004,
> @@ -67,9 +68,11 @@ enum v4l2_mbus_pixelcode {
>  	V4L2_MBUS_FMT_YUYV10_1X20 = 0x200d,
>  	V4L2_MBUS_FMT_YVYU10_1X20 = 0x200e,
> 
> -	/* Bayer - next is 0x3013 */
> +	/* Bayer - next is 0x3015 */
>  	V4L2_MBUS_FMT_SBGGR8_1X8 = 0x3001,
> +	V4L2_MBUS_FMT_SGBRG8_1X8 = 0x3013,
>  	V4L2_MBUS_FMT_SGRBG8_1X8 = 0x3002,
> +	V4L2_MBUS_FMT_SRGGB8_1X8 = 0x3014,
>  	V4L2_MBUS_FMT_SBGGR10_DPCM8_1X8 = 0x300b,
>  	V4L2_MBUS_FMT_SGBRG10_DPCM8_1X8 = 0x300c,
>  	V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8 = 0x3009,

-- 
Regards,

Laurent Pinchart

[PATCH 3/4] omap3isp: ccdc: support Y10, Y12, SGRBG8, SBGGR8

[Michael Jones]

Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
---
 drivers/media/video/omap3-isp/ispccdc.c  |    4 ++++
 drivers/media/video/omap3-isp/ispvideo.c |    8 ++++++++
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/omap3-isp/ispccdc.c b/drivers/media/video/omap3-isp/ispccdc.c
index e4d04ce..166115d 100644
--- a/drivers/media/video/omap3-isp/ispccdc.c
+++ b/drivers/media/video/omap3-isp/ispccdc.c
@@ -43,6 +43,10 @@ __ccdc_get_format(struct isp_ccdc_device *ccdc, struct v4l2_subdev_fh *fh,
 
 static const unsigned int ccdc_fmts[] = {
 	V4L2_MBUS_FMT_Y8_1X8,
+	V4L2_MBUS_FMT_Y10_1X10,
+	V4L2_MBUS_FMT_Y12_1X12,
+	V4L2_MBUS_FMT_SGRBG8_1X8,
+	V4L2_MBUS_FMT_SBGGR8_1X8,
 	V4L2_MBUS_FMT_SGRBG10_1X10,
 	V4L2_MBUS_FMT_SRGGB10_1X10,
 	V4L2_MBUS_FMT_SBGGR10_1X10,
diff --git a/drivers/media/video/omap3-isp/ispvideo.c b/drivers/media/video/omap3-isp/ispvideo.c
index f16d787..c406043 100644
--- a/drivers/media/video/omap3-isp/ispvideo.c
+++ b/drivers/media/video/omap3-isp/ispvideo.c
@@ -48,6 +48,14 @@
 static struct isp_format_info formats[] = {
 	{ V4L2_MBUS_FMT_Y8_1X8, V4L2_MBUS_FMT_Y8_1X8,
 	  V4L2_MBUS_FMT_Y8_1X8, V4L2_PIX_FMT_GREY, 8, },
+	{ V4L2_MBUS_FMT_Y10_1X10, V4L2_MBUS_FMT_Y10_1X10,
+	  V4L2_MBUS_FMT_Y10_1X10, V4L2_PIX_FMT_Y10, 10, },
+	{ V4L2_MBUS_FMT_Y12_1X12, V4L2_MBUS_FMT_Y10_1X10,
+	  V4L2_MBUS_FMT_Y12_1X12, V4L2_PIX_FMT_Y12, 12, },
+	{ V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_MBUS_FMT_SBGGR8_1X8,
+	  V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_PIX_FMT_SBGGR8, 8, },
+	{ V4L2_MBUS_FMT_SGRBG8_1X8, V4L2_MBUS_FMT_SGRBG8_1X8,
+	  V4L2_MBUS_FMT_SGRBG8_1X8, V4L2_PIX_FMT_SGRBG8, 8, },
 	{ V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8, V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8,
 	  V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10DPCM8, 8, },
 	{ V4L2_MBUS_FMT_SBGGR10_1X10, V4L2_MBUS_FMT_SBGGR10_1X10,
-- 
1.7.4.1


MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

Re: [PATCH 3/4] omap3isp: ccdc: support Y10, Y12, SGRBG8, SBGGR8

[Laurent Pinchart]

Hi Michael,

Thanks for the patch.

On Friday 04 March 2011 09:58:03 Michael Jones wrote:
> Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
> ---
>  drivers/media/video/omap3-isp/ispccdc.c  |    4 ++++
>  drivers/media/video/omap3-isp/ispvideo.c |    8 ++++++++
>  2 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/omap3-isp/ispccdc.c
> b/drivers/media/video/omap3-isp/ispccdc.c index e4d04ce..166115d 100644
> --- a/drivers/media/video/omap3-isp/ispccdc.c
> +++ b/drivers/media/video/omap3-isp/ispccdc.c
> @@ -43,6 +43,10 @@ __ccdc_get_format(struct isp_ccdc_device *ccdc, struct
> v4l2_subdev_fh *fh,
> 
>  static const unsigned int ccdc_fmts[] = {
>  	V4L2_MBUS_FMT_Y8_1X8,
> +	V4L2_MBUS_FMT_Y10_1X10,
> +	V4L2_MBUS_FMT_Y12_1X12,
> +	V4L2_MBUS_FMT_SGRBG8_1X8,
> +	V4L2_MBUS_FMT_SBGGR8_1X8,

While you're at it, what about SGBRG8_1X8 and SRGGB8_1X8 (here and in 
ispvideo.c) ?

>  	V4L2_MBUS_FMT_SGRBG10_1X10,
>  	V4L2_MBUS_FMT_SRGGB10_1X10,
>  	V4L2_MBUS_FMT_SBGGR10_1X10,
> diff --git a/drivers/media/video/omap3-isp/ispvideo.c
> b/drivers/media/video/omap3-isp/ispvideo.c index f16d787..c406043 100644
> --- a/drivers/media/video/omap3-isp/ispvideo.c
> +++ b/drivers/media/video/omap3-isp/ispvideo.c
> @@ -48,6 +48,14 @@
>  static struct isp_format_info formats[] = {
>  	{ V4L2_MBUS_FMT_Y8_1X8, V4L2_MBUS_FMT_Y8_1X8,
>  	  V4L2_MBUS_FMT_Y8_1X8, V4L2_PIX_FMT_GREY, 8, },
> +	{ V4L2_MBUS_FMT_Y10_1X10, V4L2_MBUS_FMT_Y10_1X10,
> +	  V4L2_MBUS_FMT_Y10_1X10, V4L2_PIX_FMT_Y10, 10, },
> +	{ V4L2_MBUS_FMT_Y12_1X12, V4L2_MBUS_FMT_Y10_1X10,
> +	  V4L2_MBUS_FMT_Y12_1X12, V4L2_PIX_FMT_Y12, 12, },
> +	{ V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_MBUS_FMT_SBGGR8_1X8,
> +	  V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_PIX_FMT_SBGGR8, 8, },
> +	{ V4L2_MBUS_FMT_SGRBG8_1X8, V4L2_MBUS_FMT_SGRBG8_1X8,
> +	  V4L2_MBUS_FMT_SGRBG8_1X8, V4L2_PIX_FMT_SGRBG8, 8, },
>  	{ V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8, V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8,
>  	  V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10DPCM8, 8, },
>  	{ V4L2_MBUS_FMT_SBGGR10_1X10, V4L2_MBUS_FMT_SBGGR10_1X10,

-- 
Regards,

Laurent Pinchart

[PATCH 4/4] omap3isp: lane shifter support

[Michael Jones]

Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
---
 drivers/media/video/omap3-isp/isp.c      |   82 +++++++++++++++++++++++++++++-
 drivers/media/video/omap3-isp/isp.h      |    4 +-
 drivers/media/video/omap3-isp/ispccdc.c  |    2 +-
 drivers/media/video/omap3-isp/ispvideo.c |   65 +++++++++++++++++-------
 drivers/media/video/omap3-isp/ispvideo.h |    3 +
 5 files changed, 134 insertions(+), 22 deletions(-)

diff --git a/drivers/media/video/omap3-isp/isp.c b/drivers/media/video/omap3-isp/isp.c
index 08d90fe..20e6daa 100644
--- a/drivers/media/video/omap3-isp/isp.c
+++ b/drivers/media/video/omap3-isp/isp.c
@@ -273,6 +273,44 @@ static void isp_power_settings(struct isp_device *isp, int idle)
 }
 
 /*
+ * Decide whether desired output pixel code can be obtained with
+ * the lane shifter by shifting the input pixel code.
+ * return 1 if the combination is possible
+ * return 0 otherwise
+ */
+int omap3isp_is_shiftable(enum v4l2_mbus_pixelcode in,
+		enum v4l2_mbus_pixelcode out)
+{
+	const struct isp_format_info *in_info, *out_info;
+	int shiftable = 0;
+	if ((in == 0) || (out == 0))
+		return 0;
+
+	if (in == out)
+		return 1;
+
+	in_info = omap3isp_video_format_info(in);
+	out_info = omap3isp_video_format_info(out);
+	if ((!in_info) || (!out_info))
+		return 0;
+
+	if (in_info->flavor != out_info->flavor)
+		return 0;
+
+	switch (in_info->bpp - out_info->bpp) {
+	case 2:
+	case 4:
+	case 6:
+		shiftable = 1;
+		break;
+	default:
+		shiftable = 0;
+	}
+
+	return shiftable;
+}
+
+/*
  * Configure the bridge and lane shifter. Valid inputs are
  *
  * CCDC_INPUT_PARALLEL: Parallel interface
@@ -288,6 +326,10 @@ void omap3isp_configure_bridge(struct isp_device *isp,
 			       const struct isp_parallel_platform_data *pdata)
 {
 	u32 ispctrl_val;
+	u32 depth_in = 0, depth_out = 0;
+	u32 shift;
+	const struct isp_format_info *fmt_info;
+	struct media_pad *srcpad;
 
 	ispctrl_val  = isp_reg_readl(isp, OMAP3_ISP_IOMEM_MAIN, ISP_CTRL);
 	ispctrl_val &= ~ISPCTRL_SHIFT_MASK;
@@ -298,7 +340,6 @@ void omap3isp_configure_bridge(struct isp_device *isp,
 	switch (input) {
 	case CCDC_INPUT_PARALLEL:
 		ispctrl_val |= ISPCTRL_PAR_SER_CLK_SEL_PARALLEL;
-		ispctrl_val |= pdata->data_lane_shift << ISPCTRL_SHIFT_SHIFT;
 		ispctrl_val |= pdata->clk_pol << ISPCTRL_PAR_CLK_POL_SHIFT;
 		ispctrl_val |= pdata->bridge << ISPCTRL_PAR_BRIDGE_SHIFT;
 		break;
@@ -319,6 +360,45 @@ void omap3isp_configure_bridge(struct isp_device *isp,
 		return;
 	}
 
+	/* find output format from the remote end of the link connected to
+	 * CCDC sink pad
+	 */
+	srcpad = media_entity_remote_source(&isp->isp_ccdc.pads[CCDC_PAD_SINK]);
+	if (srcpad == NULL)
+		dev_err(isp->dev, "No active input to CCDC.\n");
+
+	if (media_entity_type(srcpad->entity) == MEDIA_ENT_T_V4L2_SUBDEV) {
+		struct v4l2_subdev *subdev =
+		   media_entity_to_v4l2_subdev(srcpad->entity);
+		struct v4l2_subdev_format fmt_src;
+		fmt_src.pad = srcpad->index;
+		fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+		if (!v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt_src)) {
+			fmt_info =
+			   omap3isp_video_format_info(fmt_src.format.code);
+			depth_in = fmt_info ? fmt_info->bpp : 0;
+		}
+	}
+
+	/* find CCDC input format */
+	fmt_info = omap3isp_video_format_info
+		(isp->isp_ccdc.formats[CCDC_PAD_SINK].code);
+	depth_out = fmt_info ? fmt_info->bpp : 0;
+
+	isp->isp_ccdc.syncif.datsz = depth_out;
+
+	/* determine necessary shifting */
+	if (depth_in == depth_out + 6)
+		shift = 3;
+	else if (depth_in == depth_out + 4)
+		shift = 2;
+	else if (depth_in == depth_out + 2)
+		shift = 1;
+	else
+		shift = 0;
+
+	ispctrl_val |= shift << ISPCTRL_SHIFT_SHIFT;
+
 	ispctrl_val &= ~ISPCTRL_SYNC_DETECT_MASK;
 	ispctrl_val |= ISPCTRL_SYNC_DETECT_VSRISE;
 
diff --git a/drivers/media/video/omap3-isp/isp.h b/drivers/media/video/omap3-isp/isp.h
index 21fa88b..c84d349 100644
--- a/drivers/media/video/omap3-isp/isp.h
+++ b/drivers/media/video/omap3-isp/isp.h
@@ -144,8 +144,6 @@ struct isp_reg {
  *		ISPCTRL_PAR_BRIDGE_BENDIAN - Big endian
  */
 struct isp_parallel_platform_data {
-	unsigned int width;
-	unsigned int data_lane_shift:2;
 	unsigned int clk_pol:1;
 	unsigned int bridge:4;
 };
@@ -320,6 +318,8 @@ int omap3isp_module_sync_is_stopping(wait_queue_head_t *wait,
 
 int omap3isp_pipeline_set_stream(struct isp_pipeline *pipe,
 				 enum isp_pipeline_stream_state state);
+int omap3isp_is_shiftable(enum v4l2_mbus_pixelcode in,
+		enum v4l2_mbus_pixelcode out);
 void omap3isp_configure_bridge(struct isp_device *isp,
 			       enum ccdc_input_entity input,
 			       const struct isp_parallel_platform_data *pdata);
diff --git a/drivers/media/video/omap3-isp/ispccdc.c b/drivers/media/video/omap3-isp/ispccdc.c
index 166115d..efaf317 100644
--- a/drivers/media/video/omap3-isp/ispccdc.c
+++ b/drivers/media/video/omap3-isp/ispccdc.c
@@ -1132,7 +1132,7 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
 
 	omap3isp_configure_bridge(isp, ccdc->input, pdata);
 
-	ccdc->syncif.datsz = pdata ? pdata->width : 10;
+	/* syncif.datsz was set in isp_configure_bridge() */
 	ccdc_config_sync_if(ccdc, &ccdc->syncif);
 
 	/* CCDC_PAD_SINK */
diff --git a/drivers/media/video/omap3-isp/ispvideo.c b/drivers/media/video/omap3-isp/ispvideo.c
index c406043..4602d20 100644
--- a/drivers/media/video/omap3-isp/ispvideo.c
+++ b/drivers/media/video/omap3-isp/ispvideo.c
@@ -47,37 +47,53 @@
 
 static struct isp_format_info formats[] = {
 	{ V4L2_MBUS_FMT_Y8_1X8, V4L2_MBUS_FMT_Y8_1X8,
-	  V4L2_MBUS_FMT_Y8_1X8, V4L2_PIX_FMT_GREY, 8, },
+	  V4L2_MBUS_FMT_Y8_1X8, V4L2_MBUS_FMT_Y8_1X8,
+	  V4L2_PIX_FMT_GREY, 8, },
 	{ V4L2_MBUS_FMT_Y10_1X10, V4L2_MBUS_FMT_Y10_1X10,
-	  V4L2_MBUS_FMT_Y10_1X10, V4L2_PIX_FMT_Y10, 10, },
+	  V4L2_MBUS_FMT_Y10_1X10, V4L2_MBUS_FMT_Y8_1X8,
+	  V4L2_PIX_FMT_Y10, 10, },
 	{ V4L2_MBUS_FMT_Y12_1X12, V4L2_MBUS_FMT_Y10_1X10,
-	  V4L2_MBUS_FMT_Y12_1X12, V4L2_PIX_FMT_Y12, 12, },
+	  V4L2_MBUS_FMT_Y12_1X12, V4L2_MBUS_FMT_Y8_1X8,
+	  V4L2_PIX_FMT_Y12, 12, },
 	{ V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_MBUS_FMT_SBGGR8_1X8,
-	  V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_PIX_FMT_SBGGR8, 8, },
+	  V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_MBUS_FMT_SBGGR8_1X8,
+	  V4L2_PIX_FMT_SBGGR8, 8, },
 	{ V4L2_MBUS_FMT_SGRBG8_1X8, V4L2_MBUS_FMT_SGRBG8_1X8,
-	  V4L2_MBUS_FMT_SGRBG8_1X8, V4L2_PIX_FMT_SGRBG8, 8, },
+	  V4L2_MBUS_FMT_SGRBG8_1X8, V4L2_MBUS_FMT_SGRBG8_1X8,
+	  V4L2_PIX_FMT_SGRBG8, 8, },
 	{ V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8, V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8,
-	  V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10DPCM8, 8, },
+	  V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8,
+	  V4L2_PIX_FMT_SGRBG10DPCM8, 8, },
 	{ V4L2_MBUS_FMT_SBGGR10_1X10, V4L2_MBUS_FMT_SBGGR10_1X10,
-	  V4L2_MBUS_FMT_SBGGR10_1X10, V4L2_PIX_FMT_SBGGR10, 10, },
+	  V4L2_MBUS_FMT_SBGGR10_1X10, V4L2_MBUS_FMT_SBGGR8_1X8,
+	  V4L2_PIX_FMT_SBGGR10, 10, },
 	{ V4L2_MBUS_FMT_SGBRG10_1X10, V4L2_MBUS_FMT_SGBRG10_1X10,
-	  V4L2_MBUS_FMT_SGBRG10_1X10, V4L2_PIX_FMT_SGBRG10, 10, },
+	  V4L2_MBUS_FMT_SGBRG10_1X10, V4L2_MBUS_FMT_SGBRG8_1X8,
+	  V4L2_PIX_FMT_SGBRG10, 10, },
 	{ V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_MBUS_FMT_SGRBG10_1X10,
-	  V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10, 10, },
+	  V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_MBUS_FMT_SGRBG8_1X8,
+	  V4L2_PIX_FMT_SGRBG10, 10, },
 	{ V4L2_MBUS_FMT_SRGGB10_1X10, V4L2_MBUS_FMT_SRGGB10_1X10,
-	  V4L2_MBUS_FMT_SRGGB10_1X10, V4L2_PIX_FMT_SRGGB10, 10, },
+	  V4L2_MBUS_FMT_SRGGB10_1X10, V4L2_MBUS_FMT_SRGGB8_1X8,
+	  V4L2_PIX_FMT_SRGGB10, 10, },
 	{ V4L2_MBUS_FMT_SBGGR12_1X12, V4L2_MBUS_FMT_SBGGR10_1X10,
-	  V4L2_MBUS_FMT_SBGGR12_1X12, V4L2_PIX_FMT_SBGGR12, 12, },
+	  V4L2_MBUS_FMT_SBGGR12_1X12, V4L2_MBUS_FMT_SBGGR8_1X8,
+	  V4L2_PIX_FMT_SBGGR12, 12, },
 	{ V4L2_MBUS_FMT_SGBRG12_1X12, V4L2_MBUS_FMT_SGBRG10_1X10,
-	  V4L2_MBUS_FMT_SGBRG12_1X12, V4L2_PIX_FMT_SGBRG12, 12, },
+	  V4L2_MBUS_FMT_SGBRG12_1X12, V4L2_MBUS_FMT_SGBRG8_1X8,
+	  V4L2_PIX_FMT_SGBRG12, 12, },
 	{ V4L2_MBUS_FMT_SGRBG12_1X12, V4L2_MBUS_FMT_SGRBG10_1X10,
-	  V4L2_MBUS_FMT_SGRBG12_1X12, V4L2_PIX_FMT_SGRBG12, 12, },
+	  V4L2_MBUS_FMT_SGRBG12_1X12, V4L2_MBUS_FMT_SGRBG8_1X8,
+	  V4L2_PIX_FMT_SGRBG12, 12, },
 	{ V4L2_MBUS_FMT_SRGGB12_1X12, V4L2_MBUS_FMT_SRGGB10_1X10,
-	  V4L2_MBUS_FMT_SRGGB12_1X12, V4L2_PIX_FMT_SRGGB12, 12, },
+	  V4L2_MBUS_FMT_SRGGB12_1X12, V4L2_MBUS_FMT_SRGGB8_1X8,
+	  V4L2_PIX_FMT_SRGGB12, 12, },
 	{ V4L2_MBUS_FMT_UYVY8_1X16, V4L2_MBUS_FMT_UYVY8_1X16,
-	  V4L2_MBUS_FMT_UYVY8_1X16, V4L2_PIX_FMT_UYVY, 16, },
+	  V4L2_MBUS_FMT_UYVY8_1X16, V4L2_MBUS_FMT_UYVY8_2X8,
+	  V4L2_PIX_FMT_UYVY, 16, },
 	{ V4L2_MBUS_FMT_YUYV8_1X16, V4L2_MBUS_FMT_YUYV8_1X16,
-	  V4L2_MBUS_FMT_YUYV8_1X16, V4L2_PIX_FMT_YUYV, 16, },
+	  V4L2_MBUS_FMT_YUYV8_1X16, V4L2_MBUS_FMT_YUYV8_2X8,
+	  V4L2_PIX_FMT_YUYV, 16, },
 };
 
 const struct isp_format_info *
@@ -243,6 +259,7 @@ static int isp_video_validate_pipeline(struct isp_pipeline *pipe)
 		return -EPIPE;
 
 	while (1) {
+		unsigned int link_has_shifter;
 		/* Retrieve the sink format */
 		pad = &subdev->entity.pads[0];
 		if (!(pad->flags & MEDIA_PAD_FL_SINK))
@@ -271,6 +288,10 @@ static int isp_video_validate_pipeline(struct isp_pipeline *pipe)
 				return -ENOSPC;
 		}
 
+		/* if sink pad is on CCDC, the link has the lane shifter
+		 * in the middle of it. */
+		link_has_shifter = (subdev == &isp->isp_ccdc.subdev);
+
 		/* Retrieve the source format */
 		pad = media_entity_remote_source(pad);
 		if (pad == NULL ||
@@ -286,10 +307,18 @@ static int isp_video_validate_pipeline(struct isp_pipeline *pipe)
 			return -EPIPE;
 
 		/* Check if the two ends match */
-		if (fmt_source.format.code != fmt_sink.format.code ||
-		    fmt_source.format.width != fmt_sink.format.width ||
+		if (fmt_source.format.width != fmt_sink.format.width ||
 		    fmt_source.format.height != fmt_sink.format.height)
 			return -EPIPE;
+
+		if (link_has_shifter) {
+			if (!omap3isp_is_shiftable(fmt_source.format.code,
+						fmt_sink.format.code)) {
+				pr_debug("%s not shiftable.\n", __func__);
+				return -EPIPE;
+			}
+		} else if (fmt_source.format.code != fmt_sink.format.code)
+			return -EPIPE;
 	}
 
 	return 0;
diff --git a/drivers/media/video/omap3-isp/ispvideo.h b/drivers/media/video/omap3-isp/ispvideo.h
index 524a1ac..7794cb4 100644
--- a/drivers/media/video/omap3-isp/ispvideo.h
+++ b/drivers/media/video/omap3-isp/ispvideo.h
@@ -49,6 +49,8 @@ struct v4l2_pix_format;
  *	bits. Identical to @code if the format is 10 bits wide or less.
  * @uncompressed: V4L2 media bus format code for the corresponding uncompressed
  *	format. Identical to @code if the format is not DPCM compressed.
+ *	@flavor: V4L2 media bus format code for the same pixel layout but
+ *	shifted to be 8 bits per pixel.
  * @pixelformat: V4L2 pixel format FCC identifier
  * @bpp: Bits per pixel
  */
@@ -56,6 +58,7 @@ struct isp_format_info {
 	enum v4l2_mbus_pixelcode code;
 	enum v4l2_mbus_pixelcode truncated;
 	enum v4l2_mbus_pixelcode uncompressed;
+	enum v4l2_mbus_pixelcode flavor;
 	u32 pixelformat;
 	unsigned int bpp;
 };
-- 
1.7.4.1


MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

Re: [PATCH 4/4] omap3isp: lane shifter support

[Laurent Pinchart]

Hi sMichal,

Thanks for the patch.

On Friday 04 March 2011 09:58:04 Michael Jones wrote:
> Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
> ---
>  drivers/media/video/omap3-isp/isp.c      |   82
> +++++++++++++++++++++++++++++- drivers/media/video/omap3-isp/isp.h      | 
>   4 +-
>  drivers/media/video/omap3-isp/ispccdc.c  |    2 +-
>  drivers/media/video/omap3-isp/ispvideo.c |   65 +++++++++++++++++-------
>  drivers/media/video/omap3-isp/ispvideo.h |    3 +
>  5 files changed, 134 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/video/omap3-isp/isp.c
> b/drivers/media/video/omap3-isp/isp.c index 08d90fe..20e6daa 100644
> --- a/drivers/media/video/omap3-isp/isp.c
> +++ b/drivers/media/video/omap3-isp/isp.c
> @@ -273,6 +273,44 @@ static void isp_power_settings(struct isp_device *isp,
> int idle) }
> 
>  /*
> + * Decide whether desired output pixel code can be obtained with
> + * the lane shifter by shifting the input pixel code.
> + * return 1 if the combination is possible
> + * return 0 otherwise
> + */
> +int omap3isp_is_shiftable(enum v4l2_mbus_pixelcode in,
> +		enum v4l2_mbus_pixelcode out)

As you only use this function in ispvideo.c, I would move it there. You could 
also make the function return a bool.

> +{
> +	const struct isp_format_info *in_info, *out_info;
> +	int shiftable = 0;
> +	if ((in == 0) || (out == 0))
> +		return 0;

Can this happen ?

> +
> +	if (in == out)
> +		return 1;
> +
> +	in_info = omap3isp_video_format_info(in);
> +	out_info = omap3isp_video_format_info(out);
> +	if ((!in_info) || (!out_info))
> +		return 0;

Can this happen ?

> +
> +	if (in_info->flavor != out_info->flavor)
> +		return 0;
> +
> +	switch (in_info->bpp - out_info->bpp) {
> +	case 2:
> +	case 4:
> +	case 6:
> +		shiftable = 1;
> +		break;
> +	default:
> +		shiftable = 0;
> +	}

What about

return in_info->bpp - out_info->bpp <= 6;

> +
> +	return shiftable;
> +}
> +
> +/*
>   * Configure the bridge and lane shifter. Valid inputs are
>   *
>   * CCDC_INPUT_PARALLEL: Parallel interface
> @@ -288,6 +326,10 @@ void omap3isp_configure_bridge(struct isp_device *isp,
>  			       const struct isp_parallel_platform_data *pdata)
>  {
>  	u32 ispctrl_val;
> +	u32 depth_in = 0, depth_out = 0;
> +	u32 shift;
> +	const struct isp_format_info *fmt_info;
> +	struct media_pad *srcpad;
> 
>  	ispctrl_val  = isp_reg_readl(isp, OMAP3_ISP_IOMEM_MAIN, ISP_CTRL);
>  	ispctrl_val &= ~ISPCTRL_SHIFT_MASK;
> @@ -298,7 +340,6 @@ void omap3isp_configure_bridge(struct isp_device *isp,
>  	switch (input) {
>  	case CCDC_INPUT_PARALLEL:
>  		ispctrl_val |= ISPCTRL_PAR_SER_CLK_SEL_PARALLEL;
> -		ispctrl_val |= pdata->data_lane_shift << ISPCTRL_SHIFT_SHIFT;
>  		ispctrl_val |= pdata->clk_pol << ISPCTRL_PAR_CLK_POL_SHIFT;
>  		ispctrl_val |= pdata->bridge << ISPCTRL_PAR_BRIDGE_SHIFT;
>  		break;
> @@ -319,6 +360,45 @@ void omap3isp_configure_bridge(struct isp_device *isp,
>  		return;
>  	}
> 
> +	/* find output format from the remote end of the link connected to
> +	 * CCDC sink pad
> +	 */
> +	srcpad = media_entity_remote_source(&isp->isp_ccdc.pads[CCDC_PAD_SINK]);
> +	if (srcpad == NULL)
> +		dev_err(isp->dev, "No active input to CCDC.\n");

There's no need to test for NULL, as this function will only be called on 
streamon, so the pipeline will be valid.

> +	if (media_entity_type(srcpad->entity) == MEDIA_ENT_T_V4L2_SUBDEV) {

The CCDC has no memory input, so this condition will always be true.

> +		struct v4l2_subdev *subdev =
> +		   media_entity_to_v4l2_subdev(srcpad->entity);
> +		struct v4l2_subdev_format fmt_src;
> +		fmt_src.pad = srcpad->index;
> +		fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> +		if (!v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt_src)) {
> +			fmt_info =
> +			   omap3isp_video_format_info(fmt_src.format.code);
> +			depth_in = fmt_info ? fmt_info->bpp : 0;
> +		}
> +	}
> +
> +	/* find CCDC input format */
> +	fmt_info = omap3isp_video_format_info
> +		(isp->isp_ccdc.formats[CCDC_PAD_SINK].code);
> +	depth_out = fmt_info ? fmt_info->bpp : 0;
> +
> +	isp->isp_ccdc.syncif.datsz = depth_out;
> +
> +	/* determine necessary shifting */
> +	if (depth_in == depth_out + 6)
> +		shift = 3;
> +	else if (depth_in == depth_out + 4)
> +		shift = 2;
> +	else if (depth_in == depth_out + 2)
> +		shift = 1;
> +	else
> +		shift = 0;

Maybe shift = (depth_out - depth_in) / 2; ?

> +
> +	ispctrl_val |= shift << ISPCTRL_SHIFT_SHIFT;
> +
>  	ispctrl_val &= ~ISPCTRL_SYNC_DETECT_MASK;
>  	ispctrl_val |= ISPCTRL_SYNC_DETECT_VSRISE;
> 

[snip]

> diff --git a/drivers/media/video/omap3-isp/ispccdc.c
> b/drivers/media/video/omap3-isp/ispccdc.c index 166115d..efaf317 100644
> --- a/drivers/media/video/omap3-isp/ispccdc.c
> +++ b/drivers/media/video/omap3-isp/ispccdc.c
> @@ -1132,7 +1132,7 @@ static void ccdc_configure(struct isp_ccdc_device
> *ccdc)
> 
>  	omap3isp_configure_bridge(isp, ccdc->input, pdata);
> 
> -	ccdc->syncif.datsz = pdata ? pdata->width : 10;
> +	/* syncif.datsz was set in isp_configure_bridge() */

I'd rather set syncif.datsz in ispccdc.c than in isp.c, as all other syncif 
fields are set there. It might even make sense to compute the shift value here 
and pass it to omap3isp_configure_bridge, especially with the code a couple of 
lines above to retrieve the connected subdev (which would need to be moved out 
of the if(), except for the pdata line).

>  	ccdc_config_sync_if(ccdc, &ccdc->syncif);
> 
>  	/* CCDC_PAD_SINK */
> diff --git a/drivers/media/video/omap3-isp/ispvideo.c
> b/drivers/media/video/omap3-isp/ispvideo.c index c406043..4602d20 100644
> --- a/drivers/media/video/omap3-isp/ispvideo.c
> +++ b/drivers/media/video/omap3-isp/ispvideo.c
> @@ -47,37 +47,53 @@
> 
>  static struct isp_format_info formats[] = {
>  	{ V4L2_MBUS_FMT_Y8_1X8, V4L2_MBUS_FMT_Y8_1X8,
> -	  V4L2_MBUS_FMT_Y8_1X8, V4L2_PIX_FMT_GREY, 8, },
> +	  V4L2_MBUS_FMT_Y8_1X8, V4L2_MBUS_FMT_Y8_1X8,
> +	  V4L2_PIX_FMT_GREY, 8, },
>  	{ V4L2_MBUS_FMT_Y10_1X10, V4L2_MBUS_FMT_Y10_1X10,
> -	  V4L2_MBUS_FMT_Y10_1X10, V4L2_PIX_FMT_Y10, 10, },
> +	  V4L2_MBUS_FMT_Y10_1X10, V4L2_MBUS_FMT_Y8_1X8,
> +	  V4L2_PIX_FMT_Y10, 10, },
>  	{ V4L2_MBUS_FMT_Y12_1X12, V4L2_MBUS_FMT_Y10_1X10,
> -	  V4L2_MBUS_FMT_Y12_1X12, V4L2_PIX_FMT_Y12, 12, },
> +	  V4L2_MBUS_FMT_Y12_1X12, V4L2_MBUS_FMT_Y8_1X8,
> +	  V4L2_PIX_FMT_Y12, 12, },
>  	{ V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_MBUS_FMT_SBGGR8_1X8,
> -	  V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_PIX_FMT_SBGGR8, 8, },
> +	  V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_MBUS_FMT_SBGGR8_1X8,
> +	  V4L2_PIX_FMT_SBGGR8, 8, },
>  	{ V4L2_MBUS_FMT_SGRBG8_1X8, V4L2_MBUS_FMT_SGRBG8_1X8,
> -	  V4L2_MBUS_FMT_SGRBG8_1X8, V4L2_PIX_FMT_SGRBG8, 8, },
> +	  V4L2_MBUS_FMT_SGRBG8_1X8, V4L2_MBUS_FMT_SGRBG8_1X8,
> +	  V4L2_PIX_FMT_SGRBG8, 8, },
>  	{ V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8, V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8,
> -	  V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10DPCM8, 8, },
> +	  V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8,
> +	  V4L2_PIX_FMT_SGRBG10DPCM8, 8, },
>  	{ V4L2_MBUS_FMT_SBGGR10_1X10, V4L2_MBUS_FMT_SBGGR10_1X10,
> -	  V4L2_MBUS_FMT_SBGGR10_1X10, V4L2_PIX_FMT_SBGGR10, 10, },
> +	  V4L2_MBUS_FMT_SBGGR10_1X10, V4L2_MBUS_FMT_SBGGR8_1X8,
> +	  V4L2_PIX_FMT_SBGGR10, 10, },
>  	{ V4L2_MBUS_FMT_SGBRG10_1X10, V4L2_MBUS_FMT_SGBRG10_1X10,
> -	  V4L2_MBUS_FMT_SGBRG10_1X10, V4L2_PIX_FMT_SGBRG10, 10, },
> +	  V4L2_MBUS_FMT_SGBRG10_1X10, V4L2_MBUS_FMT_SGBRG8_1X8,
> +	  V4L2_PIX_FMT_SGBRG10, 10, },
>  	{ V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_MBUS_FMT_SGRBG10_1X10,
> -	  V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10, 10, },
> +	  V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_MBUS_FMT_SGRBG8_1X8,
> +	  V4L2_PIX_FMT_SGRBG10, 10, },
>  	{ V4L2_MBUS_FMT_SRGGB10_1X10, V4L2_MBUS_FMT_SRGGB10_1X10,
> -	  V4L2_MBUS_FMT_SRGGB10_1X10, V4L2_PIX_FMT_SRGGB10, 10, },
> +	  V4L2_MBUS_FMT_SRGGB10_1X10, V4L2_MBUS_FMT_SRGGB8_1X8,
> +	  V4L2_PIX_FMT_SRGGB10, 10, },
>  	{ V4L2_MBUS_FMT_SBGGR12_1X12, V4L2_MBUS_FMT_SBGGR10_1X10,
> -	  V4L2_MBUS_FMT_SBGGR12_1X12, V4L2_PIX_FMT_SBGGR12, 12, },
> +	  V4L2_MBUS_FMT_SBGGR12_1X12, V4L2_MBUS_FMT_SBGGR8_1X8,
> +	  V4L2_PIX_FMT_SBGGR12, 12, },
>  	{ V4L2_MBUS_FMT_SGBRG12_1X12, V4L2_MBUS_FMT_SGBRG10_1X10,
> -	  V4L2_MBUS_FMT_SGBRG12_1X12, V4L2_PIX_FMT_SGBRG12, 12, },
> +	  V4L2_MBUS_FMT_SGBRG12_1X12, V4L2_MBUS_FMT_SGBRG8_1X8,
> +	  V4L2_PIX_FMT_SGBRG12, 12, },
>  	{ V4L2_MBUS_FMT_SGRBG12_1X12, V4L2_MBUS_FMT_SGRBG10_1X10,
> -	  V4L2_MBUS_FMT_SGRBG12_1X12, V4L2_PIX_FMT_SGRBG12, 12, },
> +	  V4L2_MBUS_FMT_SGRBG12_1X12, V4L2_MBUS_FMT_SGRBG8_1X8,
> +	  V4L2_PIX_FMT_SGRBG12, 12, },
>  	{ V4L2_MBUS_FMT_SRGGB12_1X12, V4L2_MBUS_FMT_SRGGB10_1X10,
> -	  V4L2_MBUS_FMT_SRGGB12_1X12, V4L2_PIX_FMT_SRGGB12, 12, },
> +	  V4L2_MBUS_FMT_SRGGB12_1X12, V4L2_MBUS_FMT_SRGGB8_1X8,
> +	  V4L2_PIX_FMT_SRGGB12, 12, },
>  	{ V4L2_MBUS_FMT_UYVY8_1X16, V4L2_MBUS_FMT_UYVY8_1X16,
> -	  V4L2_MBUS_FMT_UYVY8_1X16, V4L2_PIX_FMT_UYVY, 16, },
> +	  V4L2_MBUS_FMT_UYVY8_1X16, V4L2_MBUS_FMT_UYVY8_2X8,
> +	  V4L2_PIX_FMT_UYVY, 16, },
>  	{ V4L2_MBUS_FMT_YUYV8_1X16, V4L2_MBUS_FMT_YUYV8_1X16,
> -	  V4L2_MBUS_FMT_YUYV8_1X16, V4L2_PIX_FMT_YUYV, 16, },
> +	  V4L2_MBUS_FMT_YUYV8_1X16, V4L2_MBUS_FMT_YUYV8_2X8,

I don't think these two are correct. YUYV8_1X16 doesn't magically become 
YUYV8_2X8 when shifted. As those formats can only be used by the preview 
engine and the resizer, they're pretty much unshiftable.

> +	  V4L2_PIX_FMT_YUYV, 16, },
>  };
> 
>  const struct isp_format_info *
> @@ -243,6 +259,7 @@ static int isp_video_validate_pipeline(struct
> isp_pipeline *pipe) return -EPIPE;
> 
>  	while (1) {
> +		unsigned int link_has_shifter;
>  		/* Retrieve the sink format */
>  		pad = &subdev->entity.pads[0];
>  		if (!(pad->flags & MEDIA_PAD_FL_SINK))
> @@ -271,6 +288,10 @@ static int isp_video_validate_pipeline(struct
> isp_pipeline *pipe) return -ENOSPC;
>  		}
> 
> +		/* if sink pad is on CCDC, the link has the lane shifter
> +		 * in the middle of it. */
> +		link_has_shifter = (subdev == &isp->isp_ccdc.subdev);
> +
>  		/* Retrieve the source format */
>  		pad = media_entity_remote_source(pad);
>  		if (pad == NULL ||
> @@ -286,10 +307,18 @@ static int isp_video_validate_pipeline(struct
> isp_pipeline *pipe) return -EPIPE;
> 
>  		/* Check if the two ends match */
> -		if (fmt_source.format.code != fmt_sink.format.code ||
> -		    fmt_source.format.width != fmt_sink.format.width ||
> +		if (fmt_source.format.width != fmt_sink.format.width ||
>  		    fmt_source.format.height != fmt_sink.format.height)
>  			return -EPIPE;
> +
> +		if (link_has_shifter) {
> +			if (!omap3isp_is_shiftable(fmt_source.format.code,
> +						fmt_sink.format.code)) {
> +				pr_debug("%s not shiftable.\n", __func__);
> +				return -EPIPE;
> +			}
> +		} else if (fmt_source.format.code != fmt_sink.format.code)
> +			return -EPIPE;
>  	}
> 
>  	return 0;
> diff --git a/drivers/media/video/omap3-isp/ispvideo.h
> b/drivers/media/video/omap3-isp/ispvideo.h index 524a1ac..7794cb4 100644
> --- a/drivers/media/video/omap3-isp/ispvideo.h
> +++ b/drivers/media/video/omap3-isp/ispvideo.h
> @@ -49,6 +49,8 @@ struct v4l2_pix_format;
>   *	bits. Identical to @code if the format is 10 bits wide or less.
>   * @uncompressed: V4L2 media bus format code for the corresponding
> uncompressed
>   *	format. Identical to @code if the format is not DPCM compressed.
> + *	@flavor: V4L2 media bus format code for the same pixel layout but

@flavor should be aligned left on @uncompressed and @pixelformat.

> + *	shifted to be 8 bits per pixel.
>   * @pixelformat: V4L2 pixel format FCC identifier
>   * @bpp: Bits per pixel
>   */
> @@ -56,6 +58,7 @@ struct isp_format_info {
>  	enum v4l2_mbus_pixelcode code;
>  	enum v4l2_mbus_pixelcode truncated;
>  	enum v4l2_mbus_pixelcode uncompressed;
> +	enum v4l2_mbus_pixelcode flavor;
>  	u32 pixelformat;
>  	unsigned int bpp;
>  };

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/4] omap3isp: lane shifter support

[Michael Jones]

Hi Laurent,

Thanks for the review.

On 03/04/2011 05:33 PM, Laurent Pinchart wrote:
> Hi Michael,
> 
> Thanks for the patch.
> 
> On Friday 04 March 2011 09:58:04 Michael Jones wrote:
>> Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
>> ---
>>  drivers/media/video/omap3-isp/isp.c      |   82
>> +++++++++++++++++++++++++++++- drivers/media/video/omap3-isp/isp.h      | 
>>   4 +-
>>  drivers/media/video/omap3-isp/ispccdc.c  |    2 +-
>>  drivers/media/video/omap3-isp/ispvideo.c |   65 +++++++++++++++++-------
>>  drivers/media/video/omap3-isp/ispvideo.h |    3 +
>>  5 files changed, 134 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/media/video/omap3-isp/isp.c
>> b/drivers/media/video/omap3-isp/isp.c index 08d90fe..20e6daa 100644
>> --- a/drivers/media/video/omap3-isp/isp.c
>> +++ b/drivers/media/video/omap3-isp/isp.c
>> @@ -273,6 +273,44 @@ static void isp_power_settings(struct isp_device *isp,
>> int idle) }
>>
>>  /*
>> + * Decide whether desired output pixel code can be obtained with
>> + * the lane shifter by shifting the input pixel code.
>> + * return 1 if the combination is possible
>> + * return 0 otherwise
>> + */
>> +int omap3isp_is_shiftable(enum v4l2_mbus_pixelcode in,
>> +		enum v4l2_mbus_pixelcode out)
> 
> As you only use this function in ispvideo.c, I would move it there. You could 
> also make the function return a bool.

I thought returning a bool was inconsistent with kernel style (e.g.
isp_pipeline_is_last, ccdc_lsc_is_configured return int).

> 
>> +{
>> +	const struct isp_format_info *in_info, *out_info;
>> +	int shiftable = 0;
>> +	if ((in == 0) || (out == 0))
>> +		return 0;
> 
> Can this happen ?
> 
>> +
>> +	if (in == out)
>> +		return 1;
>> +
>> +	in_info = omap3isp_video_format_info(in);
>> +	out_info = omap3isp_video_format_info(out);
>> +	if ((!in_info) || (!out_info))
>> +		return 0;
> 
> Can this happen ?
> 

These were all relics of previous versions when I was also calling
omap3isp_is_shiftable() from ccdc_try_format().  I will move it to
ispvideo.c and remove the two if statements.

>> +
>> +	if (in_info->flavor != out_info->flavor)
>> +		return 0;
>> +
>> +	switch (in_info->bpp - out_info->bpp) {
>> +	case 2:
>> +	case 4:
>> +	case 6:
>> +		shiftable = 1;
>> +		break;
>> +	default:
>> +		shiftable = 0;
>> +	}
> 
> What about
> 
> return in_info->bpp - out_info->bpp <= 6;

As long as there are never formats which are the same flavor but shifted
1, 3, or 5 bits, that's fine.  I suppose this is a safe assumption?

> 
>> +
>> +	return shiftable;
>> +}
>> +
>> +/*
>>   * Configure the bridge and lane shifter. Valid inputs are
>>   *
>>   * CCDC_INPUT_PARALLEL: Parallel interface
>> @@ -288,6 +326,10 @@ void omap3isp_configure_bridge(struct isp_device *isp,
>>  			       const struct isp_parallel_platform_data *pdata)
>>  {
>>  	u32 ispctrl_val;
>> +	u32 depth_in = 0, depth_out = 0;
>> +	u32 shift;
>> +	const struct isp_format_info *fmt_info;
>> +	struct media_pad *srcpad;
>>
>>  	ispctrl_val  = isp_reg_readl(isp, OMAP3_ISP_IOMEM_MAIN, ISP_CTRL);
>>  	ispctrl_val &= ~ISPCTRL_SHIFT_MASK;
>> @@ -298,7 +340,6 @@ void omap3isp_configure_bridge(struct isp_device *isp,
>>  	switch (input) {
>>  	case CCDC_INPUT_PARALLEL:
>>  		ispctrl_val |= ISPCTRL_PAR_SER_CLK_SEL_PARALLEL;
>> -		ispctrl_val |= pdata->data_lane_shift << ISPCTRL_SHIFT_SHIFT;
>>  		ispctrl_val |= pdata->clk_pol << ISPCTRL_PAR_CLK_POL_SHIFT;
>>  		ispctrl_val |= pdata->bridge << ISPCTRL_PAR_BRIDGE_SHIFT;
>>  		break;
>> @@ -319,6 +360,45 @@ void omap3isp_configure_bridge(struct isp_device *isp,
>>  		return;
>>  	}
>>
>> +	/* find output format from the remote end of the link connected to
>> +	 * CCDC sink pad
>> +	 */
>> +	srcpad = media_entity_remote_source(&isp->isp_ccdc.pads[CCDC_PAD_SINK]);
>> +	if (srcpad == NULL)
>> +		dev_err(isp->dev, "No active input to CCDC.\n");
> 
> There's no need to test for NULL, as this function will only be called on 
> streamon, so the pipeline will be valid.

OK.

> 
>> +	if (media_entity_type(srcpad->entity) == MEDIA_ENT_T_V4L2_SUBDEV) {
> 
> The CCDC has no memory input, so this condition will always be true.

OK.

> 
>> +		struct v4l2_subdev *subdev =
>> +		   media_entity_to_v4l2_subdev(srcpad->entity);
>> +		struct v4l2_subdev_format fmt_src;
>> +		fmt_src.pad = srcpad->index;
>> +		fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>> +		if (!v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt_src)) {
>> +			fmt_info =
>> +			   omap3isp_video_format_info(fmt_src.format.code);
>> +			depth_in = fmt_info ? fmt_info->bpp : 0;
>> +		}
>> +	}
>> +
>> +	/* find CCDC input format */
>> +	fmt_info = omap3isp_video_format_info
>> +		(isp->isp_ccdc.formats[CCDC_PAD_SINK].code);
>> +	depth_out = fmt_info ? fmt_info->bpp : 0;
>> +
>> +	isp->isp_ccdc.syncif.datsz = depth_out;
>> +
>> +	/* determine necessary shifting */
>> +	if (depth_in == depth_out + 6)
>> +		shift = 3;
>> +	else if (depth_in == depth_out + 4)
>> +		shift = 2;
>> +	else if (depth_in == depth_out + 2)
>> +		shift = 1;
>> +	else
>> +		shift = 0;
> 
> Maybe shift = (depth_out - depth_in) / 2; ?

First of all, the other way around: (depth_in - depth_out).  I suppose I
don't need to account for e.g. (depth_in - depth_out > 6) because then
the pipeline would've been invalid in the first place?  If I do this, I
would at least use ISPCTRL_SHIFT_MASK when writing 'shift' into
ispctrl_val as a final catch if something went wrong with shift.

> 
>> +
>> +	ispctrl_val |= shift << ISPCTRL_SHIFT_SHIFT;
>> +
>>  	ispctrl_val &= ~ISPCTRL_SYNC_DETECT_MASK;
>>  	ispctrl_val |= ISPCTRL_SYNC_DETECT_VSRISE;
>>
> 
> [snip]
> 
>> diff --git a/drivers/media/video/omap3-isp/ispccdc.c
>> b/drivers/media/video/omap3-isp/ispccdc.c index 166115d..efaf317 100644
>> --- a/drivers/media/video/omap3-isp/ispccdc.c
>> +++ b/drivers/media/video/omap3-isp/ispccdc.c
>> @@ -1132,7 +1132,7 @@ static void ccdc_configure(struct isp_ccdc_device
>> *ccdc)
>>
>>  	omap3isp_configure_bridge(isp, ccdc->input, pdata);
>>
>> -	ccdc->syncif.datsz = pdata ? pdata->width : 10;
>> +	/* syncif.datsz was set in isp_configure_bridge() */
> 
> I'd rather set syncif.datsz in ispccdc.c than in isp.c, as all other syncif 
> fields are set there. It might even make sense to compute the shift value here 
> and pass it to omap3isp_configure_bridge, especially with the code a couple of 
> lines above to retrieve the connected subdev (which would need to be moved out 
> of the if(), except for the pdata line).

Agreed.  I will move the shift computation and datsz assignment from
isp_configure_bridge() into ccdc_configure(), then pass 'shift' as a new
arg to omap3isp_configure_bridge().

> 
>>  	ccdc_config_sync_if(ccdc, &ccdc->syncif);
>>
>>  	/* CCDC_PAD_SINK */
>> diff --git a/drivers/media/video/omap3-isp/ispvideo.c
>> b/drivers/media/video/omap3-isp/ispvideo.c index c406043..4602d20 100644
>> --- a/drivers/media/video/omap3-isp/ispvideo.c
>> +++ b/drivers/media/video/omap3-isp/ispvideo.c
>> @@ -47,37 +47,53 @@
>>
>>  static struct isp_format_info formats[] = {
[snip]
>>  	{ V4L2_MBUS_FMT_UYVY8_1X16, V4L2_MBUS_FMT_UYVY8_1X16,
>> -	  V4L2_MBUS_FMT_UYVY8_1X16, V4L2_PIX_FMT_UYVY, 16, },
>> +	  V4L2_MBUS_FMT_UYVY8_1X16, V4L2_MBUS_FMT_UYVY8_2X8,
>> +	  V4L2_PIX_FMT_UYVY, 16, },
>>  	{ V4L2_MBUS_FMT_YUYV8_1X16, V4L2_MBUS_FMT_YUYV8_1X16,
>> -	  V4L2_MBUS_FMT_YUYV8_1X16, V4L2_PIX_FMT_YUYV, 16, },
>> +	  V4L2_MBUS_FMT_YUYV8_1X16, V4L2_MBUS_FMT_YUYV8_2X8,
> 
> I don't think these two are correct. YUYV8_1X16 doesn't magically become 
> YUYV8_2X8 when shifted. As those formats can only be used by the preview 
> engine and the resizer, they're pretty much unshiftable.

OK, I'll set flavor = 0 for these and add a check to
omap3isp_is_shiftable that in and out flavors !=0.

> 
>> +	  V4L2_PIX_FMT_YUYV, 16, },
>>  };
>>
>>  const struct isp_format_info *
>> @@ -243,6 +259,7 @@ static int isp_video_validate_pipeline(struct
>> isp_pipeline *pipe) return -EPIPE;
>>
>>  	while (1) {
>> +		unsigned int link_has_shifter;
>>  		/* Retrieve the sink format */
>>  		pad = &subdev->entity.pads[0];
>>  		if (!(pad->flags & MEDIA_PAD_FL_SINK))
>> @@ -271,6 +288,10 @@ static int isp_video_validate_pipeline(struct
>> isp_pipeline *pipe) return -ENOSPC;
>>  		}
>>
>> +		/* if sink pad is on CCDC, the link has the lane shifter
>> +		 * in the middle of it. */
>> +		link_has_shifter = (subdev == &isp->isp_ccdc.subdev);
>> +
>>  		/* Retrieve the source format */
>>  		pad = media_entity_remote_source(pad);
>>  		if (pad == NULL ||
>> @@ -286,10 +307,18 @@ static int isp_video_validate_pipeline(struct
>> isp_pipeline *pipe) return -EPIPE;
>>
>>  		/* Check if the two ends match */
>> -		if (fmt_source.format.code != fmt_sink.format.code ||
>> -		    fmt_source.format.width != fmt_sink.format.width ||
>> +		if (fmt_source.format.width != fmt_sink.format.width ||
>>  		    fmt_source.format.height != fmt_sink.format.height)
>>  			return -EPIPE;
>> +
>> +		if (link_has_shifter) {
>> +			if (!omap3isp_is_shiftable(fmt_source.format.code,
>> +						fmt_sink.format.code)) {
>> +				pr_debug("%s not shiftable.\n", __func__);
>> +				return -EPIPE;
>> +			}
>> +		} else if (fmt_source.format.code != fmt_sink.format.code)
>> +			return -EPIPE;
>>  	}
>>
>>  	return 0;
>> diff --git a/drivers/media/video/omap3-isp/ispvideo.h
>> b/drivers/media/video/omap3-isp/ispvideo.h index 524a1ac..7794cb4 100644
>> --- a/drivers/media/video/omap3-isp/ispvideo.h
>> +++ b/drivers/media/video/omap3-isp/ispvideo.h
>> @@ -49,6 +49,8 @@ struct v4l2_pix_format;
>>   *	bits. Identical to @code if the format is 10 bits wide or less.
>>   * @uncompressed: V4L2 media bus format code for the corresponding
>> uncompressed
>>   *	format. Identical to @code if the format is not DPCM compressed.
>> + *	@flavor: V4L2 media bus format code for the same pixel layout but
> 
> @flavor should be aligned left on @uncompressed and @pixelformat.

oops.

-Michael

MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

Re: [PATCH 4/4] omap3isp: lane shifter support

[Laurent Pinchart]

Hi Michael,

On Monday 07 March 2011 11:53:26 Michael Jones wrote:
> On 03/04/2011 05:33 PM, Laurent Pinchart wrote:

[snip]

> >> +
> >> +	if (in_info->flavor != out_info->flavor)
> >> +		return 0;
> >> +
> >> +	switch (in_info->bpp - out_info->bpp) {
> >> +	case 2:
> >> +	case 4:
> >> +	case 6:
> >> +		shiftable = 1;
> >> +		break;
> >> +	default:
> >> +		shiftable = 0;
> >> +	}
> > 
> > What about
> > 
> > return in_info->bpp - out_info->bpp <= 6;
> 
> As long as there are never formats which are the same flavor but shifted
> 1, 3, or 5 bits, that's fine.  I suppose this is a safe assumption?

I think so. If we need to add support for those formats later we can revisit 
the code.

> >> +
> >> +	return shiftable;
> >> +}
> >> +
> >> +/*
> >> 
> >>   * Configure the bridge and lane shifter. Valid inputs are
> >>   *
> >>   * CCDC_INPUT_PARALLEL: Parallel interface
> >> 

[snip]

> >> +	/* find CCDC input format */
> >> +	fmt_info = omap3isp_video_format_info
> >> +		(isp->isp_ccdc.formats[CCDC_PAD_SINK].code);
> >> +	depth_out = fmt_info ? fmt_info->bpp : 0;
> >> +
> >> +	isp->isp_ccdc.syncif.datsz = depth_out;
> >> +
> >> +	/* determine necessary shifting */
> >> +	if (depth_in == depth_out + 6)
> >> +		shift = 3;
> >> +	else if (depth_in == depth_out + 4)
> >> +		shift = 2;
> >> +	else if (depth_in == depth_out + 2)
> >> +		shift = 1;
> >> +	else
> >> +		shift = 0;
> > 
> > Maybe shift = (depth_out - depth_in) / 2; ?
> 
> First of all, the other way around: (depth_in - depth_out).  I suppose I
> don't need to account for e.g. (depth_in - depth_out > 6) because then
> the pipeline would've been invalid in the first place?  If I do this, I
> would at least use ISPCTRL_SHIFT_MASK when writing 'shift' into
> ispctrl_val as a final catch if something went wrong with shift.

Sounds good to me.

-- 
Regards,

Laurent Pinchart