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.jo...@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,
--
Sakari Ailus
sakari.ai...@maxwell.research.nokia.com
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
