Hi Michael,
Thanks for the patch.
On Wednesday 09 March 2011 17:07:43 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@xxxxxxxxxxxxxxxx>
[snip]
> diff --git a/drivers/media/video/omap3-isp/isp.h
> b/drivers/media/video/omap3-isp/isp.h index 21fa88b..3d13f8b 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;
I'm afraid you can't remove the data_lane_shift field completely. Board can
wire a 8 bits sensor to DATA[9:2] :-/ That needs to be taken into account as
well when computing the total shift value.
Hardware configuration can be done by adding the requested shift value to
data_lane_shift for parallel sensors in omap3isp_configure_bridge(), but we
also need to take it into account when validating the pipeline.
I'm not aware of any board requiring data_lane_shift at the moment though, so
we could just drop it now and add it back later when needed. This will avoid
making this patch more complex.
> unsigned int clk_pol:1;
> unsigned int bridge:4;
> };
[snip]
> diff --git a/drivers/media/video/omap3-isp/ispccdc.c
> b/drivers/media/video/omap3-isp/ispccdc.c index 23000b6..923a08f 100644
> --- a/drivers/media/video/omap3-isp/ispccdc.c
> +++ b/drivers/media/video/omap3-isp/ispccdc.c
> @@ -1120,21 +1120,39 @@ 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;
> + int depth_in = 0, depth_out = 0;
Linux discourages the declaration of multiple variables on a single line.
Could you split this ? The depths should also be unsigned int's (as well as
the shift value below).
> + int shift;
> + const struct isp_format_info *fmt_info;
> + struct v4l2_subdev_format fmt_src;
> struct media_pad *pad;
> unsigned long flags;
> u32 syn_mode;
> u32 ccdc_pattern;
Could you keep variable declaration lines (mostly) sorted by line length ?
>
> + pad = media_entity_remote_source(&ccdc->pads[CCDC_PAD_SINK]);
> + sensor = media_entity_to_v4l2_subdev(pad->entity);
> if (ccdc->input == CCDC_INPUT_PARALLEL) {
> - pad = media_entity_remote_source(&ccdc->pads[CCDC_PAD_SINK]);
> - sensor = media_entity_to_v4l2_subdev(pad->entity);
> pdata = &((struct isp_v4l2_subdevs_group *)sensor->host_priv)
> ->bus.parallel;
> }
You can remove the curly braces as the 'if' body now contains a single
statement.
>
> - omap3isp_configure_bridge(isp, ccdc->input, pdata);
> + /* set syncif.datsz */
The following lines don't set syncif.datsz, so the comment is a bit
misleading. I think you can replace it with
/* Compute the lane shifter shift value to configure the bridge. */
or something similar, and remove the "find CCDC input format" comment below.
> + 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 ? fmt_info->bpp : 0;
omap3isp_video_format_info() won't return NULL, as it supports all formats
supported by the CCDC. You can thus skip the NULL check.
> + }
> +
> + /* 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;
And here too.
> +
> + 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..decc744 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[] = {
[snip]
> { 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,
Should this be
V4L2_MBUS_FMT_SGRBG10_1X10, 0,
instead ? DPCM formats are not shiftable. It won't make any difference in
practice, as the format is already 8-bit wide, but you state in the flavor
field documentation that "=0 if format is not shiftable".
> + V4L2_PIX_FMT_SGRBG10DPCM8, 8, },
[snip]
> @@ -98,6 +116,32 @@ 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.
> + * return 1 if the combination is possible
> + * return 0 otherwise
Return true if ... or false otherwise.
> + */
> +static bool omap3isp_is_shiftable(enum v4l2_mbus_pixelcode in,
> + enum v4l2_mbus_pixelcode out)
> +{
> + const struct isp_format_info *in_info, *out_info;
> +
> + if (in == out)
> + return 1;
return true;
(and false below).
> +
> + in_info = omap3isp_video_format_info(in);
> + out_info = omap3isp_video_format_info(out);
> +
> + if ((in_info->flavor == 0) || (out_info->flavor == 0))
> + return 0;
> +
> + if (in_info->flavor != out_info->flavor)
> + return 0;
> +
> + return (in_info->bpp - out_info->bpp <= 6);
No need for brackets.
> +}
> +
> +/*
> * 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 +291,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))
> @@ -275,6 +320,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. */
As you'll need to resubmit the patch, please capitalize the 'If'.
> + link_has_shifter = (subdev == &isp->isp_ccdc.subdev);
And there's no need for brackets.
> +
> /* Retrieve the source format */
> pad = media_entity_remote_source(pad);
> if (pad == NULL ||
> @@ -290,10 +339,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__);
Do we need the pr_debug call ?
> + return -EPIPE;
> + }
> + } else if (fmt_source.format.code != fmt_sink.format.code)
> + return -EPIPE;
> }
>
> return 0;
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
