[v3,4/4] omap3isp: lane shifter support

  To use the lane shifter, set different pixel formats at each end of
the link at the CCDC input.

Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
---
 drivers/media/video/omap3-isp/isp.c      |    7 ++-
 drivers/media/video/omap3-isp/isp.h      |    5 +-
 drivers/media/video/omap3-isp/ispccdc.c  |   27 ++++++--
 drivers/media/video/omap3-isp/ispvideo.c |  108 ++++++++++++++++++++++++------
 drivers/media/video/omap3-isp/ispvideo.h |    3 +
 5 files changed, 120 insertions(+), 30 deletions(-)
  

Hi Michael,

Thanks for the patch. I have some comments below.

Michael Jones wrote:
> To use the lane shifter, set different pixel formats at each end of
> the link at the CCDC input.
> 
> Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
> ---
>  drivers/media/video/omap3-isp/isp.c      |    7 ++-
>  drivers/media/video/omap3-isp/isp.h      |    5 +-
>  drivers/media/video/omap3-isp/ispccdc.c  |   27 ++++++--
>  drivers/media/video/omap3-isp/ispvideo.c |  108 ++++++++++++++++++++++++------
>  drivers/media/video/omap3-isp/ispvideo.h |    3 +
>  5 files changed, 120 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/media/video/omap3-isp/isp.c b/drivers/media/video/omap3-isp/isp.c
> index 08d90fe..866ce09 100644
> --- a/drivers/media/video/omap3-isp/isp.c
> +++ b/drivers/media/video/omap3-isp/isp.c
> @@ -285,7 +285,8 @@ static void isp_power_settings(struct isp_device *isp, int idle)
>   */
>  void omap3isp_configure_bridge(struct isp_device *isp,
>  			       enum ccdc_input_entity input,
> -			       const struct isp_parallel_platform_data *pdata)
> +			       const struct isp_parallel_platform_data *pdata,
> +			       int shift)

This goes more or less directly to register, so what about u32?
Definitely unsigned at least.

>  {
>  	u32 ispctrl_val;
>  
> @@ -298,9 +299,9 @@ 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;
> +		shift += pdata->data_lane_shift*2;
>  		break;
>  
>  	case CCDC_INPUT_CSI2A:
> @@ -319,6 +320,8 @@ void omap3isp_configure_bridge(struct isp_device *isp,
>  		return;
>  	}
>  
> +	ispctrl_val |= ((shift/2) << ISPCTRL_SHIFT_SHIFT) & ISPCTRL_SHIFT_MASK;
> +
>  	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..6f0ff1a 100644
> --- a/drivers/media/video/omap3-isp/isp.h
> +++ b/drivers/media/video/omap3-isp/isp.h
> @@ -130,7 +130,6 @@ struct isp_reg {
>  
>  /**
>   * struct isp_parallel_platform_data - Parallel interface platform data
> - * @width: Parallel bus width in bits (8, 10, 11 or 12)
>   * @data_lane_shift: Data lane shifter
>   *		0 - CAMEXT[13:0] -> CAM[13:0]
>   *		1 - CAMEXT[13:2] -> CAM[11:0]
> @@ -144,7 +143,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;
> @@ -322,7 +320,8 @@ int omap3isp_pipeline_set_stream(struct isp_pipeline *pipe,
>  				 enum isp_pipeline_stream_state state);
>  void omap3isp_configure_bridge(struct isp_device *isp,
>  			       enum ccdc_input_entity input,
> -			       const struct isp_parallel_platform_data *pdata);
> +			       const struct isp_parallel_platform_data *pdata,
> +			       int shift);
>  
>  #define ISP_XCLK_NONE			-1
>  #define ISP_XCLK_A			0
> diff --git a/drivers/media/video/omap3-isp/ispccdc.c b/drivers/media/video/omap3-isp/ispccdc.c
> index 23000b6..bbcf619 100644
> --- a/drivers/media/video/omap3-isp/ispccdc.c
> +++ b/drivers/media/video/omap3-isp/ispccdc.c
> @@ -1120,21 +1120,38 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
>  	struct isp_parallel_platform_data *pdata = NULL;
>  	struct v4l2_subdev *sensor;
>  	struct v4l2_mbus_framefmt *format;
> +	const struct isp_format_info *fmt_info;
> +	struct v4l2_subdev_format fmt_src;
> +	unsigned int depth_out = 0;
> +	unsigned int depth_in = 0;
>  	struct media_pad *pad;
>  	unsigned long flags;
> +	unsigned int shift;
>  	u32 syn_mode;
>  	u32 ccdc_pattern;
>  
> -	if (ccdc->input == CCDC_INPUT_PARALLEL) {
> -		pad = media_entity_remote_source(&ccdc->pads[CCDC_PAD_SINK]);
> -		sensor = media_entity_to_v4l2_subdev(pad->entity);
> +	pad = media_entity_remote_source(&ccdc->pads[CCDC_PAD_SINK]);
> +	sensor = media_entity_to_v4l2_subdev(pad->entity);
> +	if (ccdc->input == CCDC_INPUT_PARALLEL)
>  		pdata = &((struct isp_v4l2_subdevs_group *)sensor->host_priv)
>  			->bus.parallel;
> +
> +	/* Compute shift value for lane shifter to configure the bridge. */
> +	fmt_src.pad = pad->index;
> +	fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> +	if (!v4l2_subdev_call(sensor, pad, get_fmt, NULL, &fmt_src)) {
> +		fmt_info = omap3isp_video_format_info(fmt_src.format.code);
> +		depth_in = fmt_info->bpp;
>  	}
>  
> -	omap3isp_configure_bridge(isp, ccdc->input, pdata);
> +	fmt_info = omap3isp_video_format_info
> +		(isp->isp_ccdc.formats[CCDC_PAD_SINK].code);
> +	depth_out = fmt_info->bpp;
> +
> +	shift = depth_in - depth_out;
> +	omap3isp_configure_bridge(isp, ccdc->input, pdata, shift);
>  
> -	ccdc->syncif.datsz = pdata ? pdata->width : 10;
> +	ccdc->syncif.datsz = depth_out;
>  	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 3c3b3c4..c4b9fe1 100644
> --- a/drivers/media/video/omap3-isp/ispvideo.c
> +++ b/drivers/media/video/omap3-isp/ispvideo.c
> @@ -47,41 +47,59 @@
>  
>  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_SGBRG8_1X8, V4L2_MBUS_FMT_SGBRG8_1X8,
> -	  V4L2_MBUS_FMT_SGBRG8_1X8, V4L2_PIX_FMT_SGBRG8, 8, },
> +	  V4L2_MBUS_FMT_SGBRG8_1X8, V4L2_MBUS_FMT_SGBRG8_1X8,
> +	  V4L2_PIX_FMT_SGBRG8, 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_SRGGB8_1X8, V4L2_MBUS_FMT_SRGGB8_1X8,
> -	  V4L2_MBUS_FMT_SRGGB8_1X8, V4L2_PIX_FMT_SRGGB8, 8, },
> +	  V4L2_MBUS_FMT_SRGGB8_1X8, V4L2_MBUS_FMT_SRGGB8_1X8,
> +	  V4L2_PIX_FMT_SRGGB8, 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, 0,
> +	  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, 0,
> +	  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, 0,
> +	  V4L2_PIX_FMT_YUYV, 16, },
>  };
>  
>  const struct isp_format_info *
> @@ -98,6 +116,37 @@ omap3isp_video_format_info(enum v4l2_mbus_pixelcode code)
>  }
>  
>  /*
> + * Decide whether desired output pixel code can be obtained with
> + * the lane shifter by shifting the input pixel code.
> + * @in: input pixelcode to shifter
> + * @out: output pixelcode from shifter
> + * @additional_shift: # of bits the sensor's LSB is offset from CAMEXT[0]
> + *
> + * return true if the combination is possible
> + * return false otherwise
> + */
> +static bool isp_video_is_shiftable(enum v4l2_mbus_pixelcode in,
> +		enum v4l2_mbus_pixelcode out,
> +		unsigned int additional_shift)
> +{
> +	const struct isp_format_info *in_info, *out_info;
> +
> +	if (in == out)
> +		return true;
> +
> +	in_info = omap3isp_video_format_info(in);
> +	out_info = omap3isp_video_format_info(out);
> +
> +	if ((in_info->flavor == 0) || (out_info->flavor == 0))
> +		return false;
> +
> +	if (in_info->flavor != out_info->flavor)
> +		return false;
> +
> +	return in_info->bpp - out_info->bpp + additional_shift <= 6;

Currently there are no formats that would behave badly in this check?
Perhaps it'd be good idea to take that into consideration. The shift
that can be done is even.

> +}
> +
> +/*
>   * isp_video_mbus_to_pix - Convert v4l2_mbus_framefmt to v4l2_pix_format
>   * @video: ISP video instance
>   * @mbus: v4l2_mbus_framefmt format (input)
> @@ -247,6 +296,7 @@ static int isp_video_validate_pipeline(struct isp_pipeline *pipe)
>  		return -EPIPE;
>  
>  	while (1) {
> +		unsigned int link_has_shifter;

link_has_shifter is only used in one place. Would it be cleaner to test
below if it's the CCDC? A comment there could be nice, too.

>  		/* Retrieve the sink format */
>  		pad = &subdev->entity.pads[0];
>  		if (!(pad->flags & MEDIA_PAD_FL_SINK))
> @@ -275,6 +325,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 ||
> @@ -290,10 +344,24 @@ 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) {
> +			unsigned int parallel_shift = 0;
> +			if (isp->isp_ccdc.input == CCDC_INPUT_PARALLEL) {
> +				struct isp_parallel_platform_data *pdata =
> +					&((struct isp_v4l2_subdevs_group *)
> +					      subdev->host_priv)->bus.parallel;
> +				parallel_shift = pdata->data_lane_shift * 2;
> +			}
> +			if (!isp_video_is_shiftable(fmt_source.format.code,
> +						fmt_sink.format.code,
> +						parallel_shift))
> +				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..911bea6 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. =0 if format is not shiftable.
>   * @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;
>  };

Best regards,
  

Hi Sakari,

Thanks for the comments.

On Wednesday 16 March 2011 16:44:49 Sakari Ailus wrote:
> Hi Michael,
> 
> Thanks for the patch. I have some comments below.
> 
> Michael Jones wrote:
> > To use the lane shifter, set different pixel formats at each end of
> > the link at the CCDC input.
> > 
> > Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
> > ---
> > 
> >  drivers/media/video/omap3-isp/isp.c      |    7 ++-
> >  drivers/media/video/omap3-isp/isp.h      |    5 +-
> >  drivers/media/video/omap3-isp/ispccdc.c  |   27 ++++++--
> >  drivers/media/video/omap3-isp/ispvideo.c |  108
> >  ++++++++++++++++++++++++------ drivers/media/video/omap3-isp/ispvideo.h
> >  |    3 +
> >  5 files changed, 120 insertions(+), 30 deletions(-)
> > 
> > diff --git a/drivers/media/video/omap3-isp/isp.c
> > b/drivers/media/video/omap3-isp/isp.c index 08d90fe..866ce09 100644
> > --- a/drivers/media/video/omap3-isp/isp.c
> > +++ b/drivers/media/video/omap3-isp/isp.c
> > @@ -285,7 +285,8 @@ static void isp_power_settings(struct isp_device
> > *isp, int idle)
> > 
> >   */
> >  
> >  void omap3isp_configure_bridge(struct isp_device *isp,
> >  
> >  			       enum ccdc_input_entity input,
> > 
> > -			       const struct isp_parallel_platform_data *pdata)
> > +			       const struct isp_parallel_platform_data *pdata,
> > +			       int shift)
> 
> This goes more or less directly to register, so what about u32?
> Definitely unsigned at least.

Agreed.

[snip]

> > @@ -98,6 +116,37 @@ omap3isp_video_format_info(enum v4l2_mbus_pixelcode
> > code)
> > 
> >  }
> >  
> >  /*
> > 
> > + * Decide whether desired output pixel code can be obtained with
> > + * the lane shifter by shifting the input pixel code.
> > + * @in: input pixelcode to shifter
> > + * @out: output pixelcode from shifter
> > + * @additional_shift: # of bits the sensor's LSB is offset from
> > CAMEXT[0] + *
> > + * return true if the combination is possible
> > + * return false otherwise
> > + */
> > +static bool isp_video_is_shiftable(enum v4l2_mbus_pixelcode in,
> > +		enum v4l2_mbus_pixelcode out,
> > +		unsigned int additional_shift)
> > +{
> > +	const struct isp_format_info *in_info, *out_info;
> > +
> > +	if (in == out)
> > +		return true;
> > +
> > +	in_info = omap3isp_video_format_info(in);
> > +	out_info = omap3isp_video_format_info(out);
> > +
> > +	if ((in_info->flavor == 0) || (out_info->flavor == 0))
> > +		return false;
> > +
> > +	if (in_info->flavor != out_info->flavor)
> > +		return false;
> > +
> > +	return in_info->bpp - out_info->bpp + additional_shift <= 6;
> 
> Currently there are no formats that would behave badly in this check?
> Perhaps it'd be good idea to take that into consideration. The shift
> that can be done is even.

I've asked Michael to remove the check because we have no misbehaving formats 
:-) Do you think we need to add a check back ?

> > +}
> > +
> > +/*
> > 
> >   * isp_video_mbus_to_pix - Convert v4l2_mbus_framefmt to v4l2_pix_format
> >   * @video: ISP video instance
> >   * @mbus: v4l2_mbus_framefmt format (input)
> > 
> > @@ -247,6 +296,7 @@ static int isp_video_validate_pipeline(struct
> > isp_pipeline *pipe)
> > 
> >  		return -EPIPE;
> >  	
> >  	while (1) {
> > 
> > +		unsigned int link_has_shifter;
> 
> link_has_shifter is only used in one place. Would it be cleaner to test
> below if it's the CCDC? A comment there could be nice, too.

I would like that better as well, but between the line where link_has_shifter 
is set and the line where it is checked, the subdev variable changes so we 
can't just check subdev == &isp->isp_ccdc.subdev there.

> >  		/* Retrieve the sink format */
> >  		pad = &subdev->entity.pads[0];
> >  		if (!(pad->flags & MEDIA_PAD_FL_SINK))
> > 
> > @@ -275,6 +325,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 ||
> > 
> > @@ -290,10 +344,24 @@ 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) {
> > +			unsigned int parallel_shift = 0;
> > +			if (isp->isp_ccdc.input == CCDC_INPUT_PARALLEL) {
> > +				struct isp_parallel_platform_data *pdata =
> > +					&((struct isp_v4l2_subdevs_group *)
> > +					      subdev->host_priv)->bus.parallel;
> > +				parallel_shift = pdata->data_lane_shift * 2;
> > +			}
> > +			if (!isp_video_is_shiftable(fmt_source.format.code,
> > +						fmt_sink.format.code,
> > +						parallel_shift))
> > +				return -EPIPE;
> > +		} else if (fmt_source.format.code != fmt_sink.format.code)
> > +			return -EPIPE;
> > 
> >  	}
> >  	
> >  	return 0;
> >
  

Laurent Pinchart wrote:
> Hi Sakari,

Hi Laurent and Michael!

...
>>> +	return in_info->bpp - out_info->bpp + additional_shift <= 6;
>>
>> Currently there are no formats that would behave badly in this check?
>> Perhaps it'd be good idea to take that into consideration. The shift
>> that can be done is even.
> 
> I've asked Michael to remove the check because we have no misbehaving formats 
> :-) Do you think we need to add a check back ?

I think it would be helpful in debugging if someone decides to attach a
sensor which supports a shift of non-even bits (8 and 9 bits, for
example). In any case an invalid configuration is possible in such case,
and I don't think that should be allowed, should it?

>>> @@ -247,6 +296,7 @@ static int isp_video_validate_pipeline(struct
>>> isp_pipeline *pipe)
>>>
>>>  		return -EPIPE;
>>>  	
>>>  	while (1) {
>>>
>>> +		unsigned int link_has_shifter;
>>
>> link_has_shifter is only used in one place. Would it be cleaner to test
>> below if it's the CCDC? A comment there could be nice, too.
> 
> I would like that better as well, but between the line where link_has_shifter 
> is set and the line where it is checked, the subdev variable changes so we 
> can't just check subdev == &isp->isp_ccdc.subdev there.

That's definitely valid. I take my comment back. The variable could be
called is_ccdc, though, since only the CCDC has that feature. No need to
generalise. :-)

Cheers,
  

Hi Sakari,

On Wednesday 16 March 2011 18:08:04 Sakari Ailus wrote:
> Laurent Pinchart wrote:
> > Hi Sakari,
> >>> +	return in_info->bpp - out_info->bpp + additional_shift <= 6;
> >> 
> >> Currently there are no formats that would behave badly in this check?
> >> Perhaps it'd be good idea to take that into consideration. The shift
> >> that can be done is even.
> > 
> > I've asked Michael to remove the check because we have no misbehaving
> > formats
> > 
> > :-) Do you think we need to add a check back ?
> 
> I think it would be helpful in debugging if someone decides to attach a
> sensor which supports a shift of non-even bits (8 and 9 bits, for
> example). In any case an invalid configuration is possible in such case,
> and I don't think that should be allowed, should it?

I agree it shouldn't be allowed, but the ISP driver doesn't support non-even 
widths at the moment, so there's no big risk. There could be an issue when a 
non-even width is added to the driver if the developer forgets to update the 
shift code. Maybe a comment in ispvideo.c above the big formats array would 
help making sure this is not forgotten ?

> >>> @@ -247,6 +296,7 @@ static int isp_video_validate_pipeline(struct
> >>> isp_pipeline *pipe)
> >>> 
> >>>  		return -EPIPE;
> >>>  	
> >>>  	while (1) {
> >>> 
> >>> +		unsigned int link_has_shifter;
> >> 
> >> link_has_shifter is only used in one place. Would it be cleaner to test
> >> below if it's the CCDC? A comment there could be nice, too.
> > 
> > I would like that better as well, but between the line where
> > link_has_shifter is set and the line where it is checked, the subdev
> > variable changes so we can't just check subdev == &isp->isp_ccdc.subdev
> > there.
> 
> That's definitely valid. I take my comment back. The variable could be
> called is_ccdc, though, since only the CCDC has that feature. No need to
> generalise. :-)
  

Hi Sakari,

Thanks for the review.

On 03/16/2011 06:46 PM, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Wednesday 16 March 2011 18:08:04 Sakari Ailus wrote:
>> Laurent Pinchart wrote:
>>> Hi Sakari,
>>>>> +	return in_info->bpp - out_info->bpp + additional_shift <= 6;
>>>>
>>>> Currently there are no formats that would behave badly in this check?
>>>> Perhaps it'd be good idea to take that into consideration. The shift
>>>> that can be done is even.
>>>
>>> I've asked Michael to remove the check because we have no misbehaving
>>> formats
>>>
>>> :-) Do you think we need to add a check back ?
>>
>> I think it would be helpful in debugging if someone decides to attach a
>> sensor which supports a shift of non-even bits (8 and 9 bits, for
>> example). In any case an invalid configuration is possible in such case,
>> and I don't think that should be allowed, should it?
> 
> I agree it shouldn't be allowed, but the ISP driver doesn't support non-even 
> widths at the moment, so there's no big risk. There could be an issue when a 
> non-even width is added to the driver if the developer forgets to update the 
> shift code. Maybe a comment in ispvideo.c above the big formats array would 
> help making sure this is not forgotten ?

I think now that additional_shift is also being considered which comes
from the board file, it makes sense to reintroduce the check for an even
shift.  As Sakari points out, this would be helpful for debugging if
someone tries using .data_lane_shift which is odd.

> 
>>>>> @@ -247,6 +296,7 @@ static int isp_video_validate_pipeline(struct
>>>>> isp_pipeline *pipe)
>>>>>
>>>>>  		return -EPIPE;
>>>>>  	
>>>>>  	while (1) {
>>>>>
>>>>> +		unsigned int link_has_shifter;
>>>>
>>>> link_has_shifter is only used in one place. Would it be cleaner to test
>>>> below if it's the CCDC? A comment there could be nice, too.
>>>
>>> I would like that better as well, but between the line where
>>> link_has_shifter is set and the line where it is checked, the subdev
>>> variable changes so we can't just check subdev == &isp->isp_ccdc.subdev
>>> there.
>>
>> That's definitely valid. I take my comment back. The variable could be
>> called is_ccdc, though, since only the CCDC has that feature. No need to
>> generalise. :-)
> 

But this is not a feature of the CCDC, the lane shifter is outside of
the CCDC.  Each 'while (1)' iteration handles 2 subdevs on each side of
one link, so I think it makes sense for a particular iteration to say
"this link has", especially when the subdev ptr changes values between
the assignment of this var and its usage.  "is_ccdc" is vague as to
which side of the CCDC we're on.  'link_has_shifter' wasn't intended to
be general, it was supposed to mean 'this_is_the_link_with_the_shifter'.
 If you want to be more specific where that is in the pipeline, maybe
'ccdc_sink_link'?  If you just want it to sound less like "this is one
of the links with a shifter" and more like "We've found _the_ link with
_the_ shifter", it could just be 'shifter_link'.

After we iron these two things out, are you guys ready to see v4?

-Michael

MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Thursday 17 March 2011 11:07:40 Michael Jones wrote:
> On 03/16/2011 06:46 PM, Laurent Pinchart wrote:
> > On Wednesday 16 March 2011 18:08:04 Sakari Ailus wrote:
> >> Laurent Pinchart wrote:
> >>> Hi Sakari,
> >>> 
> >>>>> +	return in_info->bpp - out_info->bpp + additional_shift <= 6;
> >>>> 
> >>>> Currently there are no formats that would behave badly in this check?
> >>>> Perhaps it'd be good idea to take that into consideration. The shift
> >>>> that can be done is even.
> >>> 
> >>> I've asked Michael to remove the check because we have no misbehaving
> >>> formats
> >>> 
> >>> :-) Do you think we need to add a check back ?
> >> 
> >> I think it would be helpful in debugging if someone decides to attach a
> >> sensor which supports a shift of non-even bits (8 and 9 bits, for
> >> example). In any case an invalid configuration is possible in such case,
> >> and I don't think that should be allowed, should it?
> > 
> > I agree it shouldn't be allowed, but the ISP driver doesn't support
> > non-even widths at the moment, so there's no big risk. There could be an
> > issue when a non-even width is added to the driver if the developer
> > forgets to update the shift code. Maybe a comment in ispvideo.c above
> > the big formats array would help making sure this is not forgotten ?
> 
> I think now that additional_shift is also being considered which comes
> from the board file, it makes sense to reintroduce the check for an even
> shift.  As Sakari points out, this would be helpful for debugging if
> someone tries using .data_lane_shift which is odd.

How should we handle such a broken .data_lane_shift value ? Always refuse to 
start streaming (maybe with a kernel log message) ? Or should we catch it in 
isp_register_entities() instead ?

> >>>>> @@ -247,6 +296,7 @@ static int isp_video_validate_pipeline(struct
> >>>>> isp_pipeline *pipe)
> >>>>> 
> >>>>>  		return -EPIPE;
> >>>>>  	
> >>>>>  	while (1) {
> >>>>> 
> >>>>> +		unsigned int link_has_shifter;
> >>>> 
> >>>> link_has_shifter is only used in one place. Would it be cleaner to
> >>>> test below if it's the CCDC? A comment there could be nice, too.
> >>> 
> >>> I would like that better as well, but between the line where
> >>> link_has_shifter is set and the line where it is checked, the subdev
> >>> variable changes so we can't just check subdev == &isp->isp_ccdc.subdev
> >>> there.
> >> 
> >> That's definitely valid. I take my comment back. The variable could be
> >> called is_ccdc, though, since only the CCDC has that feature. No need to
> >> generalise. :-)
> 
> But this is not a feature of the CCDC, the lane shifter is outside of
> the CCDC.  Each 'while (1)' iteration handles 2 subdevs on each side of
> one link, so I think it makes sense for a particular iteration to say
> "this link has", especially when the subdev ptr changes values between
> the assignment of this var and its usage.  "is_ccdc" is vague as to
> which side of the CCDC we're on.  'link_has_shifter' wasn't intended to
> be general, it was supposed to mean 'this_is_the_link_with_the_shifter'.
>  If you want to be more specific where that is in the pipeline, maybe
> 'ccdc_sink_link'?  If you just want it to sound less like "this is one
> of the links with a shifter" and more like "We've found _the_ link with
> _the_ shifter", it could just be 'shifter_link'.

shifter_link sounds good to me.

> After we iron these two things out, are you guys ready to see v4?

That's fine with me.
  

Hi Laurent and Michael,

Laurent Pinchart wrote:
> On Thursday 17 March 2011 11:07:40 Michael Jones wrote:
>> On 03/16/2011 06:46 PM, Laurent Pinchart wrote:
>>> On Wednesday 16 March 2011 18:08:04 Sakari Ailus wrote:
>>>> Laurent Pinchart wrote:
>>>>> Hi Sakari,
>>>>>
>>>>>>> +	return in_info->bpp - out_info->bpp + additional_shift <= 6;
>>>>>>
>>>>>> Currently there are no formats that would behave badly in this check?
>>>>>> Perhaps it'd be good idea to take that into consideration. The shift
>>>>>> that can be done is even.
>>>>>
>>>>> I've asked Michael to remove the check because we have no misbehaving
>>>>> formats
>>>>>
>>>>> :-) Do you think we need to add a check back ?
>>>>
>>>> I think it would be helpful in debugging if someone decides to attach a
>>>> sensor which supports a shift of non-even bits (8 and 9 bits, for
>>>> example). In any case an invalid configuration is possible in such case,
>>>> and I don't think that should be allowed, should it?
>>>
>>> I agree it shouldn't be allowed, but the ISP driver doesn't support
>>> non-even widths at the moment, so there's no big risk. There could be an
>>> issue when a non-even width is added to the driver if the developer
>>> forgets to update the shift code. Maybe a comment in ispvideo.c above
>>> the big formats array would help making sure this is not forgotten ?
>>
>> I think now that additional_shift is also being considered which comes
>> from the board file, it makes sense to reintroduce the check for an even
>> shift.  As Sakari points out, this would be helpful for debugging if
>> someone tries using .data_lane_shift which is odd.
> 
> How should we handle such a broken .data_lane_shift value ? Always refuse to 
> start streaming (maybe with a kernel log message) ? Or should we catch it in 
> isp_register_entities() instead ?

If I understand correctly it's not possible to shift odd bits in any
case. It's a hardware limitation.

I'd perhaps have just the appropriate register bits in the platform data
so that leaves no room for accidental misconfiguration, but this is
perhaps just too much work for not much gain.

>>>>>>> @@ -247,6 +296,7 @@ static int isp_video_validate_pipeline(struct
>>>>>>> isp_pipeline *pipe)
>>>>>>>
>>>>>>>  		return -EPIPE;
>>>>>>>  	
>>>>>>>  	while (1) {
>>>>>>>
>>>>>>> +		unsigned int link_has_shifter;
>>>>>>
>>>>>> link_has_shifter is only used in one place. Would it be cleaner to
>>>>>> test below if it's the CCDC? A comment there could be nice, too.
>>>>>
>>>>> I would like that better as well, but between the line where
>>>>> link_has_shifter is set and the line where it is checked, the subdev
>>>>> variable changes so we can't just check subdev == &isp->isp_ccdc.subdev
>>>>> there.
>>>>
>>>> That's definitely valid. I take my comment back. The variable could be
>>>> called is_ccdc, though, since only the CCDC has that feature. No need to
>>>> generalise. :-)
>>
>> But this is not a feature of the CCDC, the lane shifter is outside of
>> the CCDC.  Each 'while (1)' iteration handles 2 subdevs on each side of
>> one link, so I think it makes sense for a particular iteration to say
>> "this link has", especially when the subdev ptr changes values between
>> the assignment of this var and its usage.  "is_ccdc" is vague as to
>> which side of the CCDC we're on.  'link_has_shifter' wasn't intended to
>> be general, it was supposed to mean 'this_is_the_link_with_the_shifter'.
>>  If you want to be more specific where that is in the pipeline, maybe
>> 'ccdc_sink_link'?  If you just want it to sound less like "this is one
>> of the links with a shifter" and more like "We've found _the_ link with
>> _the_ shifter", it could just be 'shifter_link'.
> 
> shifter_link sounds good to me.

I agree.

>> After we iron these two things out, are you guys ready to see v4?
> 
> That's fine with me.

For me, too!

Cheers,
  

On 03/17/2011 04:40 PM, Sakari Ailus wrote:
> Hi Laurent and Michael,
> 
> Laurent Pinchart wrote:
>> On Thursday 17 March 2011 11:07:40 Michael Jones wrote:
>>> On 03/16/2011 06:46 PM, Laurent Pinchart wrote:
>>>> On Wednesday 16 March 2011 18:08:04 Sakari Ailus wrote:
>>>>> Laurent Pinchart wrote:
>>>>>> Hi Sakari,
>>>>>>
>>>>>>>> +	return in_info->bpp - out_info->bpp + additional_shift <= 6;
>>>>>>>
>>>>>>> Currently there are no formats that would behave badly in this check?
>>>>>>> Perhaps it'd be good idea to take that into consideration. The shift
>>>>>>> that can be done is even.
>>>>>>
>>>>>> I've asked Michael to remove the check because we have no misbehaving
>>>>>> formats
>>>>>>
>>>>>> :-) Do you think we need to add a check back ?
>>>>>
>>>>> I think it would be helpful in debugging if someone decides to attach a
>>>>> sensor which supports a shift of non-even bits (8 and 9 bits, for
>>>>> example). In any case an invalid configuration is possible in such case,
>>>>> and I don't think that should be allowed, should it?
>>>>
>>>> I agree it shouldn't be allowed, but the ISP driver doesn't support
>>>> non-even widths at the moment, so there's no big risk. There could be an
>>>> issue when a non-even width is added to the driver if the developer
>>>> forgets to update the shift code. Maybe a comment in ispvideo.c above
>>>> the big formats array would help making sure this is not forgotten ?
>>>
>>> I think now that additional_shift is also being considered which comes
>>> from the board file, it makes sense to reintroduce the check for an even
>>> shift.  As Sakari points out, this would be helpful for debugging if
>>> someone tries using .data_lane_shift which is odd.
>>
>> How should we handle such a broken .data_lane_shift value ? Always refuse to 
>> start streaming (maybe with a kernel log message) ? Or should we catch it in 
>> isp_register_entities() instead ?
> 
> If I understand correctly it's not possible to shift odd bits in any
> case. It's a hardware limitation.
> 
> I'd perhaps have just the appropriate register bits in the platform data
> so that leaves no room for accidental misconfiguration, but this is
> perhaps just too much work for not much gain.
> 

Actually, that's the way .data_lane_shift was originally defined
(0,1,2,3), and I left it that way to minimize confusion.  I was mistaken
above when I said that .data_lane_shift could sneak an odd shift to
isp_video_is_shiftable(), because .data_lane_shift is multiplied by 2
before getting passed there.  So I would like to leave this as is, and
it sounds like we have a consensus on this.

I'll submit v4 soon, then.

thanks,
Michael

MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html