OMAP3 Bridge Problems

OMAP3 Bridge Problems

[Lane Brooks]

Laurent and team,

I am using the OMAP3 ISP code from the devel branch on gitorious that I 
back ported to a 2.6.31 kernel. Raw bayer streaming to the CCDC output 
works fine. I am using parallel input with the bridge disabled in that mode.

I am having a problem when I switch the sensor to output YUV422 data. 
The YUV422 stream is a 8x2. If I only switch the sensor to YUV422 mode, 
then I can get the YUV422 data at the CCDC output, but the CCDC pads an 
extra zero byte in there and I only get half the image. So that works as 
expected. I was then hoping all I would have to do is enable the bridge 
to get the YUV422_8x2 data packed into the YUV422_16x1 automatically, 
but instead I get select timeouts.

My question:

- Are there other things I need to when I enable the parallel bridge? 
For example, do I need to change a clock rate somewhere? From the TRM, 
it seems like it should just work without any changes, but maybe I am 
missing something.

Thanks,
Lane

Re: OMAP3 Bridge Problems

[Laurent Pinchart]

Hi Lane,

On Tuesday 03 August 2010 17:26:48 Lane Brooks wrote:
> Laurent and team,

Could you please CC me when sending a mail that requires my attention ? I 
follow the linux-media mailing list, but sometimes mails can slip by.

> I am using the OMAP3 ISP code from the devel branch on gitorious that I
> back ported to a 2.6.31 kernel. Raw bayer streaming to the CCDC output
> works fine. I am using parallel input with the bridge disabled in that
> mode.
> 
> I am having a problem when I switch the sensor to output YUV422 data.
> The YUV422 stream is a 8x2. If I only switch the sensor to YUV422 mode,
> then I can get the YUV422 data at the CCDC output, but the CCDC pads an
> extra zero byte in there and I only get half the image. So that works as
> expected. I was then hoping all I would have to do is enable the bridge
> to get the YUV422_8x2 data packed into the YUV422_16x1 automatically,
> but instead I get select timeouts.
> 
> My question:
> 
> - Are there other things I need to when I enable the parallel bridge?
> For example, do I need to change a clock rate somewhere? From the TRM,
> it seems like it should just work without any changes, but maybe I am
> missing something.

Good question. ISP bridge and YUV modes support are not implemented in the 
driver, but you're probably already aware of that.

I unfortunately have no straightforward answer. Try tracing the ISP interrupts 
and monitoring the CCDC SBL busy bits to see if the CCDC writes images to 
memory correctly.

-- 
Regards,

Laurent Pinchart

Re: OMAP3 Bridge Problems

[Lane Brooks]

  On 08/04/2010 02:57 PM, Laurent Pinchart wrote:
> Hi Lane,
>
> On Tuesday 03 August 2010 17:26:48 Lane Brooks wrote:
[snip]
>
>> My question:
>>
>> - Are there other things I need to when I enable the parallel bridge?
>> For example, do I need to change a clock rate somewhere? From the TRM,
>> it seems like it should just work without any changes, but maybe I am
>> missing something.
> Good question. ISP bridge and YUV modes support are not implemented in the
> driver, but you're probably already aware of that.
>
> I unfortunately have no straightforward answer. Try tracing the ISP interrupts
> and monitoring the CCDC SBL busy bits to see if the CCDC writes images to
> memory correctly.
I found at least some of the problem. In my platform data I was enabling 
the bridge using the #defines in ispreg.h as in


static struct isp_platform_data bmi_isp_platform_data = {
     .parallel = {
         .data_lane_shift    = 3,
         .clk_pol            = 0,
         .bridge             = ISPCTRL_PAR_BRIDGE_LENDIAN,
     },
     .subdevs = bmi_camera_subdevs,
};

The bridge related #defines in ispreg.h, however, have a shift of 2 
applied to them. The problem is that the shift is applied again in isp.c 
when the options are actually applied. In other words, the bridge 
parameters are being shifted up twice, which is causing corruption to 
the control register and causing my hanging problems when I try to 
enable the bridge. It seems there are several other such cases in the 
ispreg.h where double shifting might occur if the user tries to use them 
in the platform data.

My question:
Is this an oversight or is it this way on purpose? Am I not supposed to 
be using these defines in my platform definitions? It seems that *some* 
of the parameters in ispreg.h should not be shifted up (like the bridge 
options).

Thanks,
Lane

Re: OMAP3 Bridge Problems

[Lane Brooks]

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

On 08/05/2010 10:06 AM, Lane Brooks wrote:
> On 08/04/2010 02:57 PM, Laurent Pinchart wrote:
>> Hi Lane,
>>
>> On Tuesday 03 August 2010 17:26:48 Lane Brooks wrote:
> [snip]
>>
>>> My question:
>>>
>>> - Are there other things I need to when I enable the parallel bridge?
>>> For example, do I need to change a clock rate somewhere? From the TRM,
>>> it seems like it should just work without any changes, but maybe I am
>>> missing something.
>> Good question. ISP bridge and YUV modes support are not implemented in
>> the
>> driver, but you're probably already aware of that.
>>
>> I unfortunately have no straightforward answer. Try tracing the ISP
>> interrupts
>> and monitoring the CCDC SBL busy bits to see if the CCDC writes images to
>> memory correctly.
[snip]

Laurent,

I was able to get YUV CCDC capture mode working with rather minimal 
effort. Attached is a patch with the initial effort. Can you comment?

Thanks,
Lane

[-- Attachment #2: ispyuv.patch --]
[-- Type: text/plain, Size: 4279 bytes --]

diff --git a/drivers/media/video/isp/ispccdc.c b/drivers/media/video/isp/ispccdc.c
index 90bcc6c..cc91fa1 100644
--- a/drivers/media/video/isp/ispccdc.c
+++ b/drivers/media/video/isp/ispccdc.c
@@ -1563,6 +1563,15 @@ __ccdc_get_format(struct isp_ccdc_device *ccdc, struct v4l2_subdev_fh *fh,
  * @pad: Pad number
  * @fmt: Format
  */
+
+static enum v4l2_mbus_pixelcode sink_fmts[] = {
+	V4L2_MBUS_FMT_SGRBG10_1X10,
+	V4L2_MBUS_FMT_YUYV16_1X16,
+	V4L2_MBUS_FMT_UYVY16_1X16,
+	V4L2_MBUS_FMT_YVYU16_1X16,
+	V4L2_MBUS_FMT_VYUY16_1X16,
+};
+
 static void
 ccdc_try_format(struct isp_ccdc_device *ccdc, struct v4l2_subdev_fh *fh,
 		unsigned int pad, struct v4l2_mbus_framefmt *fmt,
@@ -1571,14 +1580,22 @@ ccdc_try_format(struct isp_ccdc_device *ccdc, struct v4l2_subdev_fh *fh,
 	struct v4l2_mbus_framefmt *format;
 	unsigned int width = fmt->width;
 	unsigned int height = fmt->height;
-
+	int i;
 	switch (pad) {
 	case CCDC_PAD_SINK:
 		/* Check if the requested pixel format is supported.
 		 * TODO: If the CCDC output formatter pad is connected directly
 		 * to the resizer, only YUV formats can be used.
 		 */
-		fmt->code = V4L2_MBUS_FMT_SGRBG10_1X10;
+		for(i=0; i<ARRAY_SIZE(sink_fmts); ++i) {
+			if(sink_fmts[i] == fmt->code) 
+				break;
+		}
+		/* if the requested type is not in the list of
+		 * supported types, then change it to the first format
+		 * code in the list of supported. */
+		if(i==ARRAY_SIZE(sink_fmts)) 
+			fmt->code = sink_fmts[0];
 
 		/* Clamp the input size. */
 		fmt->width = clamp_t(u32, width, 32, 4096);
@@ -1630,10 +1647,10 @@ static int ccdc_enum_mbus_code(struct v4l2_subdev *sd,
 			       struct v4l2_subdev_fh *fh,
 			       struct v4l2_subdev_pad_mbus_code_enum *code)
 {
-	if (code->index != 0)
+	if (code->index >= ARRAY_SIZE(sink_fmts))
 		return -EINVAL;
 
-	code->code = V4L2_MBUS_FMT_SGRBG10_1X10;
+	code->code = sink_fmts[code->index];
 
 	return 0;
 }
@@ -1719,6 +1736,45 @@ static int ccdc_set_format(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
 
 	/* Propagate the format from sink to source */
 	if (pad == CCDC_PAD_SINK) {
+		u32 syn_mode, ispctrl_val;
+		struct isp_device *isp = to_isp_device(ccdc);
+		if (!isp_get(isp))
+			return -EBUSY;
+
+		syn_mode    = isp_reg_readl(isp, OMAP3_ISP_IOMEM_CCDC, 
+					    ISPCCDC_SYN_MODE);
+		ispctrl_val = isp_reg_readl(isp, OMAP3_ISP_IOMEM_MAIN, 
+					    ISP_CTRL);
+		syn_mode    &= ISPCCDC_SYN_MODE_INPMOD_MASK;
+		ispctrl_val &= ~(ISPCTRL_PAR_BRIDGE_MASK 
+				 << ISPCTRL_PAR_BRIDGE_SHIFT);
+		switch(format->code) {
+		case V4L2_MBUS_FMT_YUYV16_1X16:
+		case V4L2_MBUS_FMT_UYVY16_1X16:
+		case V4L2_MBUS_FMT_YVYU16_1X16:
+		case V4L2_MBUS_FMT_VYUY16_1X16:
+			syn_mode |= ISPCCDC_SYN_MODE_INPMOD_YCBCR16;
+
+			/* TODO: In YCBCR16 mode, the bridge has to be
+			 * enabled, so we enable it here and force it
+			 * big endian. Whether to do big or little endian
+			 * should somehow come from the platform data.*/
+			ispctrl_val |= ISPCTRL_PAR_BRIDGE_BENDIAN 
+				<< ISPCTRL_PAR_BRIDGE_SHIFT;
+			break;
+		default:
+			syn_mode |= ISPCCDC_SYN_MODE_INPMOD_RAW;
+			ispctrl_val |= isp->pdata->parallel.bridge
+				<< ISPCTRL_PAR_BRIDGE_SHIFT;
+			break;
+		}
+		isp_reg_writel(isp, syn_mode, OMAP3_ISP_IOMEM_CCDC, 
+			       ISPCCDC_SYN_MODE);
+		isp_reg_writel(isp, ispctrl_val, OMAP3_ISP_IOMEM_MAIN, 
+			       ISP_CTRL);
+		isp_put(isp);
+
+
 		format = __ccdc_get_format(ccdc, fh, CCDC_PAD_SOURCE_OF, which);
 		memcpy(format, fmt, sizeof(*format));
 		ccdc_try_format(ccdc, fh, CCDC_PAD_SOURCE_OF, format, which);
diff --git a/drivers/media/video/isp/ispreg.h b/drivers/media/video/isp/ispreg.h
index 7efcfaa..4c191af 100644
--- a/drivers/media/video/isp/ispreg.h
+++ b/drivers/media/video/isp/ispreg.h
@@ -732,10 +732,10 @@
 #define ISPCTRL_PAR_SER_CLK_SEL_MASK		0xFFFFFFFC
 
 #define ISPCTRL_PAR_BRIDGE_SHIFT		2
-#define ISPCTRL_PAR_BRIDGE_DISABLE		(0x0 << 2)
-#define ISPCTRL_PAR_BRIDGE_LENDIAN		(0x2 << 2)
-#define ISPCTRL_PAR_BRIDGE_BENDIAN		(0x3 << 2)
-#define ISPCTRL_PAR_BRIDGE_MASK			(0x3 << 2)
+#define ISPCTRL_PAR_BRIDGE_DISABLE		0x0
+#define ISPCTRL_PAR_BRIDGE_LENDIAN		0x2
+#define ISPCTRL_PAR_BRIDGE_BENDIAN		0x3
+#define ISPCTRL_PAR_BRIDGE_MASK			0x3
 
 #define ISPCTRL_PAR_CLK_POL_SHIFT		4
 #define ISPCTRL_PAR_CLK_POL_INV			(1 << 4)

Re: OMAP3 Bridge Problems

[Laurent Pinchart]

Hi Lane,

Thanks for the patch.

On Thursday 05 August 2010 20:53:50 Lane Brooks wrote:

[snip]

> I was able to get YUV CCDC capture mode working with rather minimal
> effort. Attached is a patch with the initial effort. Can you comment?

When sending patches for review, please send them inline instead of attaching 
them to the mail. It makes the review easier.

> diff --git a/drivers/media/video/isp/ispccdc.c 
b/drivers/media/video/isp/ispccdc.c
> index 90bcc6c..cc91fa1 100644
> --- a/drivers/media/video/isp/ispccdc.c
> +++ b/drivers/media/video/isp/ispccdc.c
> @@ -1563,6 +1563,15 @@ __ccdc_get_format(struct isp_ccdc_device *ccdc, 
struct v4l2_subdev_fh *fh,
>   * @pad: Pad number
>   * @fmt: Format
>   */
> +
> +static enum v4l2_mbus_pixelcode sink_fmts[] = {
> +	V4L2_MBUS_FMT_SGRBG10_1X10,
> +	V4L2_MBUS_FMT_YUYV16_1X16,
> +	V4L2_MBUS_FMT_UYVY16_1X16,
> +	V4L2_MBUS_FMT_YVYU16_1X16,
> +	V4L2_MBUS_FMT_VYUY16_1X16,
> +};
> +
>  static void
>  ccdc_try_format(struct isp_ccdc_device *ccdc, struct v4l2_subdev_fh *fh,
>  		unsigned int pad, struct v4l2_mbus_framefmt *fmt,

There's a very similar patch that is currently pending in my queue. It adds 
support for other Bayer patterns. Your overall approach is good, but the two 
patches will conflict.

Once the other one will get committed, your patch will become much simpler. I 
thus won't comment on the parts that will disappear then.

> @@ -1719,6 +1736,45 @@ static int ccdc_set_format(struct v4l2_subdev *sd, 
struct v4l2_subdev_fh *fh,
>  
>  	/* Propagate the format from sink to source */
>  	if (pad == CCDC_PAD_SINK) {
> +		u32 syn_mode, ispctrl_val;
> +		struct isp_device *isp = to_isp_device(ccdc);
> +		if (!isp_get(isp))
> +			return -EBUSY;
> +
> +		syn_mode    = isp_reg_readl(isp, OMAP3_ISP_IOMEM_CCDC, 
> +					    ISPCCDC_SYN_MODE);
> +		ispctrl_val = isp_reg_readl(isp, OMAP3_ISP_IOMEM_MAIN, 
> +					    ISP_CTRL);
> +		syn_mode    &= ISPCCDC_SYN_MODE_INPMOD_MASK;
> +		ispctrl_val &= ~(ISPCTRL_PAR_BRIDGE_MASK 
> +				 << ISPCTRL_PAR_BRIDGE_SHIFT);
> +		switch(format->code) {

Documentation/CodingStyle requires a space after the switch keyword. Please 
run scripts/checkpatch.pl before submitting patches.

> +		case V4L2_MBUS_FMT_YUYV16_1X16:
> +		case V4L2_MBUS_FMT_UYVY16_1X16:
> +		case V4L2_MBUS_FMT_YVYU16_1X16:
> +		case V4L2_MBUS_FMT_VYUY16_1X16:
> +			syn_mode |= ISPCCDC_SYN_MODE_INPMOD_YCBCR16;
> +
> +			/* TODO: In YCBCR16 mode, the bridge has to be
> +			 * enabled, so we enable it here and force it
> +			 * big endian. Whether to do big or little endian
> +			 * should somehow come from the platform data.*/
> +			ispctrl_val |= ISPCTRL_PAR_BRIDGE_BENDIAN 
> +				<< ISPCTRL_PAR_BRIDGE_SHIFT;
> +			break;
> +		default:
> +			syn_mode |= ISPCCDC_SYN_MODE_INPMOD_RAW;
> +			ispctrl_val |= isp->pdata->parallel.bridge
> +				<< ISPCTRL_PAR_BRIDGE_SHIFT;
> +			break;
> +		}
> +		isp_reg_writel(isp, syn_mode, OMAP3_ISP_IOMEM_CCDC, 
> +			       ISPCCDC_SYN_MODE);

Writing to the ISPCCDC_SYN_MODE register should be moved to ccdc_configure(). 
Just move the switch statement there right after the

	format = &ccdc->formats[CCDC_PAD_SINK];

line (without the ispctrl_val settings), it should be enough.

> +		isp_reg_writel(isp, ispctrl_val, OMAP3_ISP_IOMEM_MAIN, 
> +			       ISP_CTRL);

The ISP_CTRL register should be written in isp_select_bridge_input() only. As 
you correctly mention, whether the data is in little endian or big endian 
format should come from platform data, so I think it's fine to force board 
files to set the isp->pdata->parallel.bridge field to the correct value.

> +		isp_put(isp);
> +
> +
>  		format = __ccdc_get_format(ccdc, fh, CCDC_PAD_SOURCE_OF, which);
>  		memcpy(format, fmt, sizeof(*format));
>  		ccdc_try_format(ccdc, fh, CCDC_PAD_SOURCE_OF, format, which);
> diff --git a/drivers/media/video/isp/ispreg.h 
b/drivers/media/video/isp/ispreg.h
> index 7efcfaa..4c191af 100644
> --- a/drivers/media/video/isp/ispreg.h
> +++ b/drivers/media/video/isp/ispreg.h
> @@ -732,10 +732,10 @@
>  #define ISPCTRL_PAR_SER_CLK_SEL_MASK		0xFFFFFFFC
>  
>  #define ISPCTRL_PAR_BRIDGE_SHIFT		2
> -#define ISPCTRL_PAR_BRIDGE_DISABLE		(0x0 << 2)
> -#define ISPCTRL_PAR_BRIDGE_LENDIAN		(0x2 << 2)
> -#define ISPCTRL_PAR_BRIDGE_BENDIAN		(0x3 << 2)
> -#define ISPCTRL_PAR_BRIDGE_MASK			(0x3 << 2)
> +#define ISPCTRL_PAR_BRIDGE_DISABLE		0x0
> +#define ISPCTRL_PAR_BRIDGE_LENDIAN		0x2
> +#define ISPCTRL_PAR_BRIDGE_BENDIAN		0x3
> +#define ISPCTRL_PAR_BRIDGE_MASK			0x3

You should remove the shift in isp_select_bridge_input() instead. Could you 
please submit a patch that does just that ? Don't forget to sign it and 
include a meaningful commit message.

-- 
Regards,

Laurent Pinchart

Re: OMAP3 Bridge Problems

[Lane Brooks]

  On 08/08/2010 04:29 PM, Laurent Pinchart wrote:
> Hi Lane,
>
> Thanks for the patch.
>
> On Thursday 05 August 2010 20:53:50 Lane Brooks wrote:
>
> [snip]
>
>> I was able to get YUV CCDC capture mode working with rather minimal
>> effort. Attached is a patch with the initial effort. Can you comment?
>
> Writing to the ISPCCDC_SYN_MODE register should be moved to ccdc_configure().
> Just move the switch statement there right after the
>
> 	format =&ccdc->formats[CCDC_PAD_SINK];
>
> line (without the ispctrl_val settings), it should be enough.
>
>> +		isp_reg_writel(isp, ispctrl_val, OMAP3_ISP_IOMEM_MAIN,
>> +			       ISP_CTRL);
> The ISP_CTRL register should be written in isp_select_bridge_input() only. As
> you correctly mention, whether the data is in little endian or big endian
> format should come from platform data, so I think it's fine to force board
> files to set the isp->pdata->parallel.bridge field to the correct value.
Putting the bridge settings in the platform data is tricky because they 
need to change depending on the selected format. For example, for my 
board, when in SGRBG mode, the bridge needs disabled. When in YUV16 
mode, however, I need need to select BIG/LITTLE endian depending on 
whether it is YUYV or UYVY or ...  I am not quite sure how to capture 
that in the platform data without enumerating every supported format 
code in the platform data. The current patch knows (based on the OMAP 
TRM) that YUV16 mode requires the bridge to be enabled. So in the 
platform data I specify the bridge state for SGBRG mode and force the 
bridge to BIG endian in YUV16 mode. This leaves the sensor to switch the 
phasing based on YUYV, YVYU, etc. mode.  I am not sure who should be in 
charge of doing byte swapping in general, but if the input and output 
modes are the same, then big endian should be used to avoid a byte swap. 
In other words, the mode is completely determinable based on the 
formats, so perhaps I should implement it that way. If the input and 
output port require a byte swap, then go little endian, otherwise go big 
endian.

The reason why I put both writes to the ISPCTRL and SYN_MODE registers 
where I did. Moving them to other places will require querying the 
selected format code. Is that what you want as well?

Lane

Re: OMAP3 Bridge Problems

[Laurent Pinchart]

Hi Lane,

On Monday 09 August 2010 00:56:27 Lane Brooks wrote:
> On 08/08/2010 04:29 PM, Laurent Pinchart wrote:
> > Hi Lane,
> > 
> > Thanks for the patch.
> > 
> > On Thursday 05 August 2010 20:53:50 Lane Brooks wrote:
> > 
> > [snip]
> > 
> >> I was able to get YUV CCDC capture mode working with rather minimal
> >> effort. Attached is a patch with the initial effort. Can you comment?
> > 
> > Writing to the ISPCCDC_SYN_MODE register should be moved to
> > ccdc_configure(). Just move the switch statement there right after the
> > 
> > 	format =&ccdc->formats[CCDC_PAD_SINK];
> > 
> > line (without the ispctrl_val settings), it should be enough.
> > 
> >> +		isp_reg_writel(isp, ispctrl_val, OMAP3_ISP_IOMEM_MAIN,
> >> +			       ISP_CTRL);
> > 
> > The ISP_CTRL register should be written in isp_select_bridge_input()
> > only. As you correctly mention, whether the data is in little endian or
> > big endian format should come from platform data, so I think it's fine
> > to force board files to set the isp->pdata->parallel.bridge field to the
> > correct value.
> 
> Putting the bridge settings in the platform data is tricky because they
> need to change depending on the selected format. For example, for my
> board, when in SGRBG mode, the bridge needs disabled. When in YUV16
> mode, however, I need need to select BIG/LITTLE endian depending on
> whether it is YUYV or UYVY or ...

Ah right... So your sensor can output both Bayer and YUV data ? What sensor is 
that BTW ?

> I am not quite sure how to capture that in the platform data without
> enumerating every supported format code in the platform data. The current
> patch knows (based on the OMAP TRM) that YUV16 mode requires the bridge to
> be enabled. So in the platform data I specify the bridge state for SGBRG
> mode and force the bridge to BIG endian in YUV16 mode. This leaves the
> sensor to switch the phasing based on YUYV, YVYU, etc. mode.  I am not sure
> who should be in charge of doing byte swapping in general, but if the input
> and output modes are the same, then big endian should be used to avoid a
> byte swap. In other words, the mode is completely determinable based on the
> formats, so perhaps I should implement it that way. If the input and output
> port require a byte swap, then go little endian, otherwise go big endian.

OK I understand. The best solution (for now) would then be to modify 
isp_configure_bridge(). I wrote a few patches that modify how platform data is 
handled, but I can't commit them at the moment (they depend on other patches 
that I still need to clean up).

> The reason why I put both writes to the ISPCTRL and SYN_MODE registers
> where I did. Moving them to other places will require querying the
> selected format code. Is that what you want as well?

For SYN_MODE, definitely. For ISPCTRL, you can hack isp_configure_bridge() to 
retrieve the current CCDC input format, and we'll write a proper fix right 
after I commit my platform data restructuring patches.

-- 
Regards,

Laurent Pinchart

Re: OMAP3 Bridge Problems

[Lane Brooks]

  On 08/09/2010 03:25 AM, Laurent Pinchart wrote:
> Hi Lane,
>
> On Monday 09 August 2010 00:56:27 Lane Brooks wrote:
>> On 08/08/2010 04:29 PM, Laurent Pinchart wrote:
>>> Hi Lane,
>>>
>>> Thanks for the patch.
>>>
>>> On Thursday 05 August 2010 20:53:50 Lane Brooks wrote:
>>>
>>> [snip]
>>>
>>>> I was able to get YUV CCDC capture mode working with rather minimal
>>>> effort. Attached is a patch with the initial effort. Can you comment?
>>> Writing to the ISPCCDC_SYN_MODE register should be moved to
>>> ccdc_configure(). Just move the switch statement there right after the
>>>
>>> 	format =&ccdc->formats[CCDC_PAD_SINK];
>>>
>>> line (without the ispctrl_val settings), it should be enough.
>>>
>>>> +		isp_reg_writel(isp, ispctrl_val, OMAP3_ISP_IOMEM_MAIN,
>>>> +			       ISP_CTRL);
>>> The ISP_CTRL register should be written in isp_select_bridge_input()
>>> only. As you correctly mention, whether the data is in little endian or
>>> big endian format should come from platform data, so I think it's fine
>>> to force board files to set the isp->pdata->parallel.bridge field to the
>>> correct value.
>> Putting the bridge settings in the platform data is tricky because they
>> need to change depending on the selected format. For example, for my
>> board, when in SGRBG mode, the bridge needs disabled. When in YUV16
>> mode, however, I need need to select BIG/LITTLE endian depending on
>> whether it is YUYV or UYVY or ...
> Ah right... So your sensor can output both Bayer and YUV data ? What sensor is
> that BTW ?
>

Aptina MT9T111. It can even output JPEG.



>> I am not quite sure how to capture that in the platform data without
>> enumerating every supported format code in the platform data. The current
>> patch knows (based on the OMAP TRM) that YUV16 mode requires the bridge to
>> be enabled. So in the platform data I specify the bridge state for SGBRG
>> mode and force the bridge to BIG endian in YUV16 mode. This leaves the
>> sensor to switch the phasing based on YUYV, YVYU, etc. mode.  I am not sure
>> who should be in charge of doing byte swapping in general, but if the input
>> and output modes are the same, then big endian should be used to avoid a
>> byte swap. In other words, the mode is completely determinable based on the
>> formats, so perhaps I should implement it that way. If the input and output
>> port require a byte swap, then go little endian, otherwise go big endian.
> OK I understand. The best solution (for now) would then be to modify
> isp_configure_bridge(). I wrote a few patches that modify how platform data is
> handled, but I can't commit them at the moment (they depend on other patches
> that I still need to clean up).
>
>> The reason why I put both writes to the ISPCTRL and SYN_MODE registers
>> where I did. Moving them to other places will require querying the
>> selected format code. Is that what you want as well?
> For SYN_MODE, definitely. For ISPCTRL, you can hack isp_configure_bridge() to
> retrieve the current CCDC input format, and we'll write a proper fix right
> after I commit my platform data restructuring patches.
I'll wait for your patches then. Let me know.

Re: OMAP3 Bridge Problems

[Laurent Pinchart]

Hi Lane,

On Monday 09 August 2010 15:38:56 Lane Brooks wrote:
> On 08/09/2010 03:25 AM, Laurent Pinchart wrote:
> > On Monday 09 August 2010 00:56:27 Lane Brooks wrote:
> >> On 08/08/2010 04:29 PM, Laurent Pinchart wrote:
> >>> On Thursday 05 August 2010 20:53:50 Lane Brooks wrote:
> >>> 
> >>> [snip]
> >>> 
> >>>> I was able to get YUV CCDC capture mode working with rather minimal
> >>>> effort. Attached is a patch with the initial effort. Can you comment?
> >>> 
> >>> Writing to the ISPCCDC_SYN_MODE register should be moved to
> >>> ccdc_configure(). Just move the switch statement there right after the
> >>> 
> >>> 	format =&ccdc->formats[CCDC_PAD_SINK];
> >>> 
> >>> line (without the ispctrl_val settings), it should be enough.
> >>> 
> >>>> +		isp_reg_writel(isp, ispctrl_val, OMAP3_ISP_IOMEM_MAIN,
> >>>> +			       ISP_CTRL);
> >>> 
> >>> The ISP_CTRL register should be written in isp_select_bridge_input()
> >>> only. As you correctly mention, whether the data is in little endian or
> >>> big endian format should come from platform data, so I think it's fine
> >>> to force board files to set the isp->pdata->parallel.bridge field to
> >>> the correct value.
> >> 
> >> Putting the bridge settings in the platform data is tricky because they
> >> need to change depending on the selected format. For example, for my
> >> board, when in SGRBG mode, the bridge needs disabled. When in YUV16
> >> mode, however, I need need to select BIG/LITTLE endian depending on
> >> whether it is YUYV or UYVY or ...
> > 
> > Ah right... So your sensor can output both Bayer and YUV data ? What
> > sensor is that BTW ?
> 
> Aptina MT9T111. It can even output JPEG.
> 
> >> I am not quite sure how to capture that in the platform data without
> >> enumerating every supported format code in the platform data. The
> >> current patch knows (based on the OMAP TRM) that YUV16 mode requires
> >> the bridge to be enabled. So in the platform data I specify the bridge
> >> state for SGBRG mode and force the bridge to BIG endian in YUV16 mode.
> >> This leaves the sensor to switch the phasing based on YUYV, YVYU, etc.
> >> mode.  I am not sure who should be in charge of doing byte swapping in
> >> general, but if the input and output modes are the same, then big
> >> endian should be used to avoid a byte swap. In other words, the mode is
> >> completely determinable based on the formats, so perhaps I should
> >> implement it that way. If the input and output port require a byte
> >> swap, then go little endian, otherwise go big endian.
> > 
> > OK I understand. The best solution (for now) would then be to modify
> > isp_configure_bridge(). I wrote a few patches that modify how platform
> > data is handled, but I can't commit them at the moment (they depend on
> > other patches that I still need to clean up).
> > 
> >> The reason why I put both writes to the ISPCTRL and SYN_MODE registers
> >> where I did. Moving them to other places will require querying the
> >> selected format code. Is that what you want as well?
> > 
> > For SYN_MODE, definitely. For ISPCTRL, you can hack
> > isp_configure_bridge() to retrieve the current CCDC input format, and
> > we'll write a proper fix right after I commit my platform data
> > restructuring patches.
> 
> I'll wait for your patches then. Let me know.

The omap3camera repository is now deprecated. Please use

http://meego.gitorious.org/maemo-multimedia/omap3isp-rx51

where you will find the latest patches based on 2.6.35. Have fun :-)

I will be on holidays next week, so I won't be able to provide assistance 
before week 37.

-- 
Regards,

Laurent Pinchart

Re: OMAP3 Bridge Problems

[Lane Brooks]

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

  On 08/05/2010 10:06 AM, Lane Brooks wrote:
>  On 08/04/2010 02:57 PM, Laurent Pinchart wrote:
>> Hi Lane,
>>
>> On Tuesday 03 August 2010 17:26:48 Lane Brooks wrote:
> [snip]
>>
>>> My question:
>>>
>>> - Are there other things I need to when I enable the parallel bridge?
>>> For example, do I need to change a clock rate somewhere? From the TRM,
>>> it seems like it should just work without any changes, but maybe I am
>>> missing something.
>> Good question. ISP bridge and YUV modes support are not implemented 
>> in the
>> driver, but you're probably already aware of that.
>>
>> I unfortunately have no straightforward answer. Try tracing the ISP 
>> interrupts
>> and monitoring the CCDC SBL busy bits to see if the CCDC writes 
>> images to
>> memory correctly.
>
[snip]

Laurent,

I was able to get CCDC capture of YUV data working with rather minimal 
effort. Attached is a patch with the initial work. Can you comment?

Thanks,
Lane

[-- Attachment #2: ispyuv.patch --]
[-- Type: text/plain, Size: 4157 bytes --]

diff --git a/drivers/media/video/isp/ispccdc.c b/drivers/media/video/isp/ispccdc.c
index 90bcc6c..9226406 100644
--- a/drivers/media/video/isp/ispccdc.c
+++ b/drivers/media/video/isp/ispccdc.c
@@ -1563,6 +1563,15 @@ __ccdc_get_format(struct isp_ccdc_device *ccdc, struct v4l2_subdev_fh *fh,
  * @pad: Pad number
  * @fmt: Format
  */
+
+static enum v4l2_mbus_pixelcode sink_fmts[] = {
+	V4L2_MBUS_FMT_SGRBG10_1X10,
+	V4L2_MBUS_FMT_YUYV16_1X16,
+	V4L2_MBUS_FMT_UYVY16_1X16,
+	V4L2_MBUS_FMT_YVYU16_1X16,
+	V4L2_MBUS_FMT_VYUY16_1X16,
+};
+
 static void
 ccdc_try_format(struct isp_ccdc_device *ccdc, struct v4l2_subdev_fh *fh,
 		unsigned int pad, struct v4l2_mbus_framefmt *fmt,
@@ -1571,14 +1580,22 @@ ccdc_try_format(struct isp_ccdc_device *ccdc, struct v4l2_subdev_fh *fh,
 	struct v4l2_mbus_framefmt *format;
 	unsigned int width = fmt->width;
 	unsigned int height = fmt->height;
-
+	int i;
 	switch (pad) {
 	case CCDC_PAD_SINK:
 		/* Check if the requested pixel format is supported.
 		 * TODO: If the CCDC output formatter pad is connected directly
 		 * to the resizer, only YUV formats can be used.
 		 */
-		fmt->code = V4L2_MBUS_FMT_SGRBG10_1X10;
+		for(i=0; i<ARRAY_SIZE(sink_fmts); ++i) {
+			if(sink_fmts[i] == fmt->code) 
+				break;
+		}
+		/* if the requested type is not in the list of
+		 * supported types, then change it to the first format
+		 * code in the list of supported. */
+		if(i==ARRAY_SIZE(sink_fmts)) 
+			fmt->code = sink_fmts[0];
 
 		/* Clamp the input size. */
 		fmt->width = clamp_t(u32, width, 32, 4096);
@@ -1630,10 +1647,10 @@ static int ccdc_enum_mbus_code(struct v4l2_subdev *sd,
 			       struct v4l2_subdev_fh *fh,
 			       struct v4l2_subdev_pad_mbus_code_enum *code)
 {
-	if (code->index != 0)
+	if (code->index >= ARRAY_SIZE(sink_fmts))
 		return -EINVAL;
 
-	code->code = V4L2_MBUS_FMT_SGRBG10_1X10;
+	code->code = sink_fmts[code->index];
 
 	return 0;
 }
@@ -1719,6 +1736,44 @@ static int ccdc_set_format(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
 
 	/* Propagate the format from sink to source */
 	if (pad == CCDC_PAD_SINK) {
+		u32 syn_mode, ispctrl_val;
+		struct isp_device *isp = to_isp_device(ccdc);
+		if (!isp_get(isp))
+			return -EBUSY;
+
+		syn_mode    = isp_reg_readl(isp, OMAP3_ISP_IOMEM_CCDC, 
+					    ISPCCDC_SYN_MODE);
+		ispctrl_val = isp_reg_readl(isp, OMAP3_ISP_IOMEM_MAIN, 
+					    ISP_CTRL);
+		syn_mode    &= ISPCCDC_SYN_MODE_INPMOD_MASK;
+		ispctrl_val &= ~ISPCTRL_PAR_BRIDGE_MASK;
+		switch(format->code) {
+		case V4L2_MBUS_FMT_YUYV16_1X16:
+		case V4L2_MBUS_FMT_UYVY16_1X16:
+		case V4L2_MBUS_FMT_YVYU16_1X16:
+		case V4L2_MBUS_FMT_VYUY16_1X16:
+			syn_mode |= ISPCCDC_SYN_MODE_INPMOD_YCBCR16;
+
+			/* TODO: In YCBCR16 mode, the bridge has to be
+			 * enabled, so we enable it here and force it
+			 * big endian. Whether to do big or little endian
+			 * should somehow come from the platform data.*/
+			ispctrl_val |= ISPCTRL_PAR_BRIDGE_BENDIAN 
+				<< ISPCTRL_PAR_BRIDGE_SHIFT;
+			break;
+		default:
+			syn_mode |= ISPCCDC_SYN_MODE_INPMOD_RAW;
+			ispctrl_val |= isp->pdata->parallel.bridge
+				<< ISPCTRL_PAR_BRIDGE_SHIFT;
+			break;
+		}
+		isp_reg_writel(isp, syn_mode, OMAP3_ISP_IOMEM_CCDC, 
+			       ISPCCDC_SYN_MODE);
+		isp_reg_writel(isp, ispctrl_val, OMAP3_ISP_IOMEM_MAIN, 
+			       ISP_CTRL);
+		isp_put(isp);
+
+
 		format = __ccdc_get_format(ccdc, fh, CCDC_PAD_SOURCE_OF, which);
 		memcpy(format, fmt, sizeof(*format));
 		ccdc_try_format(ccdc, fh, CCDC_PAD_SOURCE_OF, format, which);
diff --git a/drivers/media/video/isp/ispreg.h b/drivers/media/video/isp/ispreg.h
index 7efcfaa..9bbeb9b 100644
--- a/drivers/media/video/isp/ispreg.h
+++ b/drivers/media/video/isp/ispreg.h
@@ -732,9 +732,9 @@
 #define ISPCTRL_PAR_SER_CLK_SEL_MASK		0xFFFFFFFC
 
 #define ISPCTRL_PAR_BRIDGE_SHIFT		2
-#define ISPCTRL_PAR_BRIDGE_DISABLE		(0x0 << 2)
-#define ISPCTRL_PAR_BRIDGE_LENDIAN		(0x2 << 2)
-#define ISPCTRL_PAR_BRIDGE_BENDIAN		(0x3 << 2)
+#define ISPCTRL_PAR_BRIDGE_DISABLE		0x0
+#define ISPCTRL_PAR_BRIDGE_LENDIAN		0x2
+#define ISPCTRL_PAR_BRIDGE_BENDIAN		0x3
 #define ISPCTRL_PAR_BRIDGE_MASK			(0x3 << 2)
 
 #define ISPCTRL_PAR_CLK_POL_SHIFT		4

Re: OMAP3 Bridge Problems

[Laurent Pinchart]

Hi Lane,

On Thursday 05 August 2010 18:06:51 Lane Brooks wrote:
>   On 08/04/2010 02:57 PM, Laurent Pinchart wrote:
> > On Tuesday 03 August 2010 17:26:48 Lane Brooks wrote:
> [snip]
> 
> >> My question:
> >> 
> >> - Are there other things I need to when I enable the parallel bridge?
> >> For example, do I need to change a clock rate somewhere? From the TRM,
> >> it seems like it should just work without any changes, but maybe I am
> >> missing something.
> > 
> > Good question. ISP bridge and YUV modes support are not implemented in
> > the driver, but you're probably already aware of that.
> > 
> > I unfortunately have no straightforward answer. Try tracing the ISP
> > interrupts and monitoring the CCDC SBL busy bits to see if the CCDC
> > writes images to memory correctly.
> 
> I found at least some of the problem. In my platform data I was enabling
> the bridge using the #defines in ispreg.h as in
> 
> 
> static struct isp_platform_data bmi_isp_platform_data = {
>      .parallel = {
>          .data_lane_shift    = 3,
>          .clk_pol            = 0,
>          .bridge             = ISPCTRL_PAR_BRIDGE_LENDIAN,
>      },
>      .subdevs = bmi_camera_subdevs,
> };
> 
> The bridge related #defines in ispreg.h, however, have a shift of 2
> applied to them. The problem is that the shift is applied again in isp.c
> when the options are actually applied. In other words, the bridge
> parameters are being shifted up twice, which is causing corruption to
> the control register and causing my hanging problems when I try to
> enable the bridge. It seems there are several other such cases in the
> ispreg.h where double shifting might occur if the user tries to use them
> in the platform data.
> 
> My question:
> Is this an oversight or is it this way on purpose? Am I not supposed to
> be using these defines in my platform definitions? It seems that *some*
> of the parameters in ispreg.h should not be shifted up (like the bridge
> options).

It's a bug, thanks for pointing it out. The value shouldn't be shifted again 
in isp_select_bridge_input(). Do you want to submit a patch ?

-- 
Regards,

Laurent Pinchart

Re: OMAP3 Bridge Problems

[Lane Brooks]

  On 08/08/2010 04:13 PM, Laurent Pinchart wrote:
> Hi Lane,
>
> On Thursday 05 August 2010 18:06:51 Lane Brooks wrote:
>>    On 08/04/2010 02:57 PM, Laurent Pinchart wrote:
>>> On Tuesday 03 August 2010 17:26:48 Lane Brooks wrote:
>> [snip]
>>
>>>> My question:
>>>>
>>>> - Are there other things I need to when I enable the parallel bridge?
>>>> For example, do I need to change a clock rate somewhere? From the TRM,
>>>> it seems like it should just work without any changes, but maybe I am
>>>> missing something.
>>> Good question. ISP bridge and YUV modes support are not implemented in
>>> the driver, but you're probably already aware of that.
>>>
>>> I unfortunately have no straightforward answer. Try tracing the ISP
>>> interrupts and monitoring the CCDC SBL busy bits to see if the CCDC
>>> writes images to memory correctly.
>> I found at least some of the problem. In my platform data I was enabling
>> the bridge using the #defines in ispreg.h as in
>>
>>
>> static struct isp_platform_data bmi_isp_platform_data = {
>>       .parallel = {
>>           .data_lane_shift    = 3,
>>           .clk_pol            = 0,
>>           .bridge             = ISPCTRL_PAR_BRIDGE_LENDIAN,
>>       },
>>       .subdevs = bmi_camera_subdevs,
>> };
>>
>> The bridge related #defines in ispreg.h, however, have a shift of 2
>> applied to them. The problem is that the shift is applied again in isp.c
>> when the options are actually applied. In other words, the bridge
>> parameters are being shifted up twice, which is causing corruption to
>> the control register and causing my hanging problems when I try to
>> enable the bridge. It seems there are several other such cases in the
>> ispreg.h where double shifting might occur if the user tries to use them
>> in the platform data.
>>
>> My question:
>> Is this an oversight or is it this way on purpose? Am I not supposed to
>> be using these defines in my platform definitions? It seems that *some*
>> of the parameters in ispreg.h should not be shifted up (like the bridge
>> options).
> It's a bug, thanks for pointing it out. The value shouldn't be shifted again
> in isp_select_bridge_input(). Do you want to submit a patch ?
>
The isp_parallel_platform_data struct specifies the bridge definition as 
2 bits, so if the shift is removed from isp_select_bridge instead of 
from the ispreg.h file, then the platform_data definition needs modified 
as well. Is that what you want?

Re: OMAP3 Bridge Problems

[Laurent Pinchart]

Hi Lane,

On Monday 09 August 2010 00:34:24 Lane Brooks wrote:
> On 08/08/2010 04:13 PM, Laurent Pinchart wrote:
> > On Thursday 05 August 2010 18:06:51 Lane Brooks wrote:
> >> On 08/04/2010 02:57 PM, Laurent Pinchart wrote:
> >>> On Tuesday 03 August 2010 17:26:48 Lane Brooks wrote:
> >> [snip]
> >> 
> >>>> My question:
> >>>> 
> >>>> - Are there other things I need to when I enable the parallel bridge?
> >>>> For example, do I need to change a clock rate somewhere? From the TRM,
> >>>> it seems like it should just work without any changes, but maybe I am
> >>>> missing something.
> >>> 
> >>> Good question. ISP bridge and YUV modes support are not implemented in
> >>> the driver, but you're probably already aware of that.
> >>> 
> >>> I unfortunately have no straightforward answer. Try tracing the ISP
> >>> interrupts and monitoring the CCDC SBL busy bits to see if the CCDC
> >>> writes images to memory correctly.
> >> 
> >> I found at least some of the problem. In my platform data I was enabling
> >> the bridge using the #defines in ispreg.h as in
> >> 
> >> 
> >> static struct isp_platform_data bmi_isp_platform_data = {
> >> 
> >>       .parallel = {
> >>       
> >>           .data_lane_shift    = 3,
> >>           .clk_pol            = 0,
> >>           .bridge             = ISPCTRL_PAR_BRIDGE_LENDIAN,
> >>       
> >>       },
> >>       .subdevs = bmi_camera_subdevs,
> >> 
> >> };
> >> 
> >> The bridge related #defines in ispreg.h, however, have a shift of 2
> >> applied to them. The problem is that the shift is applied again in isp.c
> >> when the options are actually applied. In other words, the bridge
> >> parameters are being shifted up twice, which is causing corruption to
> >> the control register and causing my hanging problems when I try to
> >> enable the bridge. It seems there are several other such cases in the
> >> ispreg.h where double shifting might occur if the user tries to use them
> >> in the platform data.
> >> 
> >> My question:
> >> Is this an oversight or is it this way on purpose? Am I not supposed to
> >> be using these defines in my platform definitions? It seems that *some*
> >> of the parameters in ispreg.h should not be shifted up (like the bridge
> >> options).
> > 
> > It's a bug, thanks for pointing it out. The value shouldn't be shifted
> > again in isp_select_bridge_input(). Do you want to submit a patch ?
> 
> The isp_parallel_platform_data struct specifies the bridge definition as
> 2 bits, so if the shift is removed from isp_select_bridge instead of
> from the ispreg.h file, then the platform_data definition needs modified
> as well. Is that what you want?

Good point. I think it's important to keep the registers definitions 
consistent (they all have - or should have - the shift applied), so please 
extend the bridge field to 4 bits.

-- 
Regards,

Laurent Pinchart