[PATCH 0/3] *** Updates against OMAP3ISP and BT.656

[PATCH 0/3] *** Updates against OMAP3ISP and BT.656

[Tim Nordell]

I've been doing some testing for a client's system that has a ADV7180 
attached to the omap3isp and integrating it with kernel v3.19 on a 
DM3730 platform.  I had some stability problems with the driver (it 
would crash sometimes upon stream startup or shutdown) as well as the 
ISR causing the system to lockup.  Additionally, for the system I've 
described everything with device tree (except for the omap3isp of course 
since those bindings aren't available yet), and I discovered that the 
omap3isp was starting before I2C in this case and it needed to support 
the deferral of probing the I2C client.

I also encountered the ISP getting in a state where the interrupts were 
enabled and firing, but it wasn't actually processing it since the 
pipeline state wasn't correct yet.  I added mitigation to this by 
modifying when the VD0 and VD1 interrupt trigger levels are setup, and 
causing these trigger levels to be high enough not to occur when the 
pipeline is disabled.

The other issues I encountered I believe are due to the interaction of 
the ISP on the OMAP3 and BT.656 in part.  It appears that the timing is 
critical for the ISR when entering since the current design busywaits in 
the ISR waiting for the ISP to no longer be busy, and it appears that it 
can end up missing its opportunity.  Thus I added some code to have a 
delayed buffering mode for BT.656 that causes it to hold onto buffers a 
bit longer than it otherwise would have and rely on the VSYNC latching 
for the buffers in the CCDC.

Tim Nordell (3):
  omap3isp: Defer probing when subdev isn't available
  omap3isp: Disable CCDC's VD0 and VD1 interrupts when stream is not
    enabled
  omap3isp: Add a delayed buffers for frame mode

 drivers/media/platform/omap3isp/isp.c      |   6 +-
 drivers/media/platform/omap3isp/ispccdc.c  |  43 ++++++--
 drivers/media/platform/omap3isp/ispvideo.c | 163 ++++++++++++++++++++++++-----
 drivers/media/platform/omap3isp/ispvideo.h |   3 +
 4 files changed, 178 insertions(+), 37 deletions(-)

-- 
2.0.4

[PATCH 1/3] omap3isp: Defer probing when subdev isn't available

[Tim Nordell]

If the subdev isn't available just yet, defer probing of
the system.  This is useful if the omap3isp comes up before
the I2C subsystem does.

Signed-off-by: Tim Nordell <tim.nordell@logicpd.com>
---
 drivers/media/platform/omap3isp/isp.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 51c2129..a361c40 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -1811,7 +1811,7 @@ isp_register_subdev_group(struct isp_device *isp,
 				"device %s\n", __func__,
 				board_info->i2c_adapter_id,
 				board_info->board_info->type);
-			continue;
+			return ERR_PTR(-EPROBE_DEFER);
 		}
 
 		subdev = v4l2_i2c_new_subdev_board(&isp->v4l2_dev, adapter,
@@ -1898,6 +1898,10 @@ static int isp_register_entities(struct isp_device *isp)
 		unsigned int i;
 
 		sensor = isp_register_subdev_group(isp, subdevs->subdevs);
+		if (IS_ERR(sensor)) {
+			ret = PTR_ERR(sensor);
+			goto done;
+		}
 		if (sensor == NULL)
 			continue;
 
-- 
2.0.4

[PATCH 2/3] omap3isp: Disable CCDC's VD0 and VD1 interrupts when stream is not enabled

[Tim Nordell]

During testing there appeared to be a race condition where the IRQs
for VD0 and VD1 could be triggered while enabling the CCDC module
before the pipeline status was updated.  Simply modify the trigger
conditions for VD0 and VD1 so they won't occur when the CCDC module
is not enabled.

(When this occurred during testing, the VD0 interrupt was occurring
over and over again starving the rest of the system.)

Signed-off-by: Tim Nordell <tim.nordell@logicpd.com>
---
 drivers/media/platform/omap3isp/ispccdc.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispccdc.c b/drivers/media/platform/omap3isp/ispccdc.c
index 587489a..d5de843 100644
--- a/drivers/media/platform/omap3isp/ispccdc.c
+++ b/drivers/media/platform/omap3isp/ispccdc.c
@@ -1218,13 +1218,6 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
 	}
 	ccdc_config_imgattr(ccdc, ccdc_pattern);
 
-	/* Generate VD0 on the last line of the image and VD1 on the
-	 * 2/3 height line.
-	 */
-	isp_reg_writel(isp, ((format->height - 2) << ISPCCDC_VDINT_0_SHIFT) |
-		       ((format->height * 2 / 3) << ISPCCDC_VDINT_1_SHIFT),
-		       OMAP3_ISP_IOMEM_CCDC, ISPCCDC_VDINT);
-
 	/* CCDC_PAD_SOURCE_OF */
 	format = &ccdc->formats[CCDC_PAD_SOURCE_OF];
 	crop = &ccdc->crop;
@@ -1316,11 +1309,29 @@ unlock:
 
 static void __ccdc_enable(struct isp_ccdc_device *ccdc, int enable)
 {
+	struct v4l2_mbus_framefmt *format = &ccdc->formats[CCDC_PAD_SINK];
 	struct isp_device *isp = to_isp_device(ccdc);
+	int vd0, vd1;
 
 	isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_PCR,
 			ISPCCDC_PCR_EN, enable ? ISPCCDC_PCR_EN : 0);
 
+	/* Generate VD0 on the last line of the image and VD1 on the
+	* 2/3 height line when enabled.  Otherwise, set VD0 and VD1
+	* interrupts high enough that they won't be generated.
+	*/
+	if (enable) {
+		vd0 = format->height - 2;
+		vd1 = format->height * 2 / 3;
+	} else {
+		vd0 = 0xffff;
+		vd1 = 0xffff;
+	}
+
+	isp_reg_writel(isp, (vd0 << ISPCCDC_VDINT_0_SHIFT) |
+		(vd1 << ISPCCDC_VDINT_1_SHIFT),
+		OMAP3_ISP_IOMEM_CCDC, ISPCCDC_VDINT);
+
 	ccdc->running = enable;
 }
 
-- 
2.0.4

[PATCH 3/3] omap3isp: Add a delayed buffers for frame mode

[Tim Nordell]

When using an external decoder such as a NTSC decoder chip,
the decoder is sending frame data most of the time making
it very time sensitive to latch onto the CCDC not busy bit
from one frame to the next.  This is different than most
parallel cameras that may be attached to the system as
these send frames in more of a bursty way.

This exhibits itself as a problem in the VD0 interrupt, at
least when attached to a ADV7180 using BT.656.  In this case
the ISR sometimes misses the small amount of time that the
CCDC is not busy.  The ISR attempts to busywait for up to
1ms inside the ISR waiting for the CCDC to stop being busy
and if it misses it it will kill the stream.  In testing,
I set this delay up to 10ms with the ADV7180 and often saw
delays of ~6-7ms with this hardware configuration.

To avoid having to adjust this delay, the CCDC hardware
actually does latch the buffer address at each vertical sync
so the driver could modify the buffer address at any point
during a frame to take effect during the next frame.  In
this patch, the buffering subsystem has been modified for
BT.656 only so that the timing looks more like this near
the end of each frame:

  1. Frame N-2 is released back to userspace
  2. Frame N-1 is being filled by the hardware
  3. Frame N is loaded into buffer address

This introduces additional latency into the video pipeline and
it requires more buffers to be used in the pipeline, but it
removes the busy waiting in the ISR when it's attempting to
find the time the CCDC is not busy.

Rather than moving the buffers out of the dmaqueue, this patch
leaves the buffers in the dmaqueue so that the rest of the
cleanup code for the system isn't affected.  Peeking forward
from the front of the list doesn't take very many cycles to
complete and simplifies this patch's integration with the
rest of the system.  Additionally, this patch is set to only
occur when BT.656 is enabled in the system.

Signed-off-by: Tim Nordell <tim.nordell@logicpd.com>
---
 drivers/media/platform/omap3isp/ispccdc.c  |  22 +++-
 drivers/media/platform/omap3isp/ispvideo.c | 163 ++++++++++++++++++++++++-----
 drivers/media/platform/omap3isp/ispvideo.h |   3 +
 3 files changed, 157 insertions(+), 31 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispccdc.c b/drivers/media/platform/omap3isp/ispccdc.c
index d5de843..882ebde 100644
--- a/drivers/media/platform/omap3isp/ispccdc.c
+++ b/drivers/media/platform/omap3isp/ispccdc.c
@@ -1147,6 +1147,8 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
 
 		pdata = &((struct isp_v4l2_subdevs_group *)sensor->host_priv)
 			->bus.parallel;
+		if (ccdc->bt656)
+			ccdc->video_out.pipe.do_delayed_frames = true;
 	}
 
 	/* CCDC_PAD_SINK */
@@ -1321,9 +1323,23 @@ static void __ccdc_enable(struct isp_ccdc_device *ccdc, int enable)
 	* interrupts high enough that they won't be generated.
 	*/
 	if (enable) {
-		vd0 = format->height - 2;
-		vd1 = format->height * 2 / 3;
+		if (ccdc->bt656) {
+			/* Generate VD0 around the first line of the image
+			* and VD1 high enough to not be encountered for BT.656.
+			*/
+			vd0 = 1;
+			vd1 = 0xffff;
+		} else {
+			/* Generate VD0 on the last line of the image and VD1
+			* on the 2/3 height line.
+			*/
+			vd0 = format->height - 2;
+			vd1 = format->height * 2 / 3;
+		}
 	} else {
+		/* Set VD0 and VD1 interrupts high enough that they won't
+		* be generated.
+		*/
 		vd0 = 0xffff;
 		vd1 = 0xffff;
 	}
@@ -1617,7 +1633,7 @@ static int ccdc_isr_buffer(struct isp_ccdc_device *ccdc)
 	}
 
 	/* Wait for the CCDC to become idle. */
-	if (ccdc_sbl_wait_idle(ccdc, 1000)) {
+	if (!pipe->do_delayed_frames && ccdc_sbl_wait_idle(ccdc, 1000)) {
 		dev_info(isp->dev, "CCDC won't become idle!\n");
 		isp->crashed |= 1U << ccdc->subdev.entity.id;
 		omap3isp_pipeline_cancel_stream(pipe);
diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index 3fe9047..2764463 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -370,6 +370,34 @@ static int isp_video_buffer_prepare(struct vb2_buffer *buf)
 }
 
 /*
+ * isp_video_buffer_queue_ready - Returns true if dmaqueue is in a state that
+ * warrants starting the stream.
+ * @video: Video stream
+ *
+ * This function checks to see if isp_video_buffer_queue should start streaming
+ * of the queue or not.  If we're not doing delayed frames, we should kickstart
+ * on the basis of 0 frames currently in the queue.  If we are doing delayed
+ * frames, it should be on the basis of 2 frames currently in the queue.
+ */
+static bool isp_video_buffer_queue_ready(struct isp_video *video)
+{
+	int cnt = 0;
+	struct isp_buffer *pos;
+	struct isp_pipeline *pipe = to_isp_pipeline(&video->video.entity);
+
+	if (!pipe->do_delayed_frames)
+		return list_empty(&video->dmaqueue);
+
+	list_for_each_entry(pos, &video->dmaqueue, irqlist)
+		if (++cnt > 2)
+			return false;
+
+	if (cnt == 2)
+		return true;
+	return false;
+}
+
+/*
  * isp_video_buffer_queue - Add buffer to streaming queue
  * @buf: Video buffer
  *
@@ -389,6 +417,8 @@ static void isp_video_buffer_queue(struct vb2_buffer *buf)
 	unsigned int empty;
 	unsigned int start;
 
+	buffer->delayed_state = 0;
+
 	spin_lock_irqsave(&video->irqlock, flags);
 
 	if (unlikely(video->error)) {
@@ -397,9 +427,17 @@ static void isp_video_buffer_queue(struct vb2_buffer *buf)
 		return;
 	}
 
-	empty = list_empty(&video->dmaqueue);
+	empty = isp_video_buffer_queue_ready(video);
+
 	list_add_tail(&buffer->irqlist, &video->dmaqueue);
 
+	/* If we're doing delayed frames, we always want to load the _first_
+	 * frame in the queue.
+	 */
+	if (pipe->do_delayed_frames)
+		buffer = list_first_entry(&video->dmaqueue, struct isp_buffer,
+					  irqlist);
+
 	spin_unlock_irqrestore(&video->irqlock, flags);
 
 	if (empty) {
@@ -450,10 +488,15 @@ struct isp_buffer *omap3isp_video_buffer_next(struct isp_video *video)
 {
 	struct isp_pipeline *pipe = to_isp_pipeline(&video->video.entity);
 	enum isp_pipeline_state state;
-	struct isp_buffer *buf;
+	struct isp_buffer *buf, *tmp;
+	struct isp_buffer *buf_to_complete = NULL;
+	bool end_video_capture = false;
+
 	unsigned long flags;
 	struct timespec ts;
 
+	ktime_get_ts(&ts);
+
 	spin_lock_irqsave(&video->irqlock, flags);
 	if (WARN_ON(list_empty(&video->dmaqueue))) {
 		spin_unlock_irqrestore(&video->irqlock, flags);
@@ -462,43 +505,104 @@ struct isp_buffer *omap3isp_video_buffer_next(struct isp_video *video)
 
 	buf = list_first_entry(&video->dmaqueue, struct isp_buffer,
 			       irqlist);
-	list_del(&buf->irqlist);
-	spin_unlock_irqrestore(&video->irqlock, flags);
 
-	ktime_get_ts(&ts);
-	buf->vb.v4l2_buf.timestamp.tv_sec = ts.tv_sec;
-	buf->vb.v4l2_buf.timestamp.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
-
-	/* Do frame number propagation only if this is the output video node.
-	 * Frame number either comes from the CSI receivers or it gets
-	 * incremented here if H3A is not active.
-	 * Note: There is no guarantee that the output buffer will finish
-	 * first, so the input number might lag behind by 1 in some cases.
-	 */
-	if (video == pipe->output && !pipe->do_propagation)
-		buf->vb.v4l2_buf.sequence =
-			atomic_inc_return(&pipe->frame_number);
-	else
-		buf->vb.v4l2_buf.sequence = atomic_read(&pipe->frame_number);
+	if (pipe->do_delayed_frames) {
+		/* If the leading buffer is delayed, there could be others */
+		switch (buf->delayed_state) {
+		case 2: /* buf[N-2] - this buffer is ready to be completed */
+			buf_to_complete = buf;
+
+			/* Find the next buffer in the queue */
+			tmp = list_next_entry(buf, irqlist);
+
+			if (tmp == buf) {
+				buf = NULL;
+				break;
+			}
+			buf = tmp;
+
+			/* Fall through */
+		case 1: /* buf[N-1] - this buffer will be ready next time */
+			buf->delayed_state++;
+
+			/* Find the next buffer in the queue */
+			tmp = list_next_entry(buf, irqlist);
+
+			if (tmp == buf) {
+				buf = NULL;
+				break;
+			}
+			buf = tmp;
+
+			/* Fall through */
+		case 0: /* buf[N] - this buffer will be queued this time */
+			buf->delayed_state++;
+			break;
+		}
 
-	if (pipe->field != V4L2_FIELD_NONE)
-		buf->vb.v4l2_buf.sequence /= 2;
+		if (buf_to_complete != NULL)
+			list_del(&buf_to_complete->irqlist);
+	} else {
+		list_del(&buf->irqlist);
+	}
+
+	/* Buf is potentially NULL in the delayed_frames case only */
+	if (buf != NULL) {
+		buf->vb.v4l2_buf.timestamp.tv_sec = ts.tv_sec;
+		buf->vb.v4l2_buf.timestamp.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
+
+		/* Do frame number propagation only if this is the output
+		* video node.  Frame number either comes from the CSI
+		* receivers or it gets incremented here if H3A is not active.
+		*
+		* Note: There is no guarantee that the output buffer will
+		* finish first, so the input number might lag behind by 1 in
+		* some cases.
+		*/
+		if (video == pipe->output && !pipe->do_propagation)
+			buf->vb.v4l2_buf.sequence =
+				atomic_inc_return(&pipe->frame_number);
+		else
+			buf->vb.v4l2_buf.sequence =
+				atomic_read(&pipe->frame_number);
 
-	buf->vb.v4l2_buf.field = pipe->field;
+		if (pipe->field != V4L2_FIELD_NONE)
+			buf->vb.v4l2_buf.sequence /= 2;
+
+		buf->vb.v4l2_buf.field = pipe->field;
+	}
 
 	/* Report pipeline errors to userspace on the capture device side. */
 	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE && pipe->error) {
-		state = VB2_BUF_STATE_ERROR;
+		buf->state = VB2_BUF_STATE_ERROR;
 		pipe->error = false;
 	} else {
-		state = VB2_BUF_STATE_DONE;
+		buf->state = VB2_BUF_STATE_DONE;
 	}
 
-	vb2_buffer_done(&buf->vb, state);
+	spin_unlock_irqrestore(&video->irqlock, flags);
+
+	if (!pipe->do_delayed_frames)
+		vb2_buffer_done(&buf->vb, buf->state);
+	else if (buf_to_complete != NULL)
+		vb2_buffer_done(&buf_to_complete->vb, buf_to_complete->state);
 
 	spin_lock_irqsave(&video->irqlock, flags);
 
-	if (list_empty(&video->dmaqueue)) {
+	if (pipe->do_delayed_frames) {
+		/* Find the first buffer available that hasn't been started.  If
+		 * there isn't one, we need to end video capture.
+		 */
+		end_video_capture = true;
+		list_for_each_entry(buf, &video->dmaqueue, irqlist)
+			if (buf->delayed_state == 0) {
+				end_video_capture = false;
+				break;
+			}
+	} else if (list_empty(&video->dmaqueue))
+		end_video_capture = true;
+
+	if (end_video_capture) {
 		spin_unlock_irqrestore(&video->irqlock, flags);
 
 		if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
@@ -522,8 +626,11 @@ struct isp_buffer *omap3isp_video_buffer_next(struct isp_video *video)
 		spin_unlock(&pipe->lock);
 	}
 
-	buf = list_first_entry(&video->dmaqueue, struct isp_buffer,
-			       irqlist);
+	/* With delayed frames, we've already found our buffer above. */
+	if (!pipe->do_delayed_frames)
+		buf = list_first_entry(&video->dmaqueue, struct isp_buffer,
+				       irqlist);
+
 	buf->vb.state = VB2_BUF_STATE_ACTIVE;
 
 	spin_unlock_irqrestore(&video->irqlock, flags);
diff --git a/drivers/media/platform/omap3isp/ispvideo.h b/drivers/media/platform/omap3isp/ispvideo.h
index 4071dd7..5857ed9 100644
--- a/drivers/media/platform/omap3isp/ispvideo.h
+++ b/drivers/media/platform/omap3isp/ispvideo.h
@@ -100,6 +100,7 @@ struct isp_pipeline {
 	struct v4l2_subdev *external;
 	unsigned int external_rate;
 	unsigned int external_width;
+	bool do_delayed_frames;
 };
 
 #define to_isp_pipeline(__e) \
@@ -125,6 +126,8 @@ struct isp_buffer {
 	struct vb2_buffer vb;
 	struct list_head irqlist;
 	dma_addr_t dma;
+	int delayed_state;
+	enum isp_pipeline_state state;
 };
 
 #define to_isp_buffer(buf)	container_of(buf, struct isp_buffer, vb)
-- 
2.0.4

Re: [PATCH 1/3] omap3isp: Defer probing when subdev isn't available

[Laurent Pinchart]

Hi Tim,

Thank you for the patch.

The OMAP3 ISP driver is moving to DT, hopefully in time for v4.1. See "[PATCH 
00/15] omap3isp driver DT support" posted to the list on Monday. I'd rather go 
for proper DT support instead of custom deferred probing.

On Tuesday 10 March 2015 14:24:52 Tim Nordell wrote:
> If the subdev isn't available just yet, defer probing of
> the system.  This is useful if the omap3isp comes up before
> the I2C subsystem does.
> 
> Signed-off-by: Tim Nordell <tim.nordell@logicpd.com>
> ---
>  drivers/media/platform/omap3isp/isp.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index 51c2129..a361c40 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -1811,7 +1811,7 @@ isp_register_subdev_group(struct isp_device *isp,
>  				"device %s\n", __func__,
>  				board_info->i2c_adapter_id,
>  				board_info->board_info->type);
> -			continue;
> +			return ERR_PTR(-EPROBE_DEFER);
>  		}
> 
>  		subdev = v4l2_i2c_new_subdev_board(&isp->v4l2_dev, adapter,
> @@ -1898,6 +1898,10 @@ static int isp_register_entities(struct isp_device
> *isp) unsigned int i;
> 
>  		sensor = isp_register_subdev_group(isp, subdevs->subdevs);
> +		if (IS_ERR(sensor)) {
> +			ret = PTR_ERR(sensor);
> +			goto done;
> +		}
>  		if (sensor == NULL)
>  			continue;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] omap3isp: Defer probing when subdev isn't available

[Tim Nordell]

Laurent -

Agreed.  This is a stop gap for this, but I guess by the time this patch 
could possibly get incorporated we'd be off to device tree anyways.

- Tim

(Sorry for the repeat - my e-mail client sent out an HTML message so it 
didn't get through to the mailing list.)

On 03/18/15 10:15, Laurent Pinchart wrote:
> Hi Tim,
>
> Thank you for the patch.
>
> The OMAP3 ISP driver is moving to DT, hopefully in time for v4.1. See "[PATCH
> 00/15] omap3isp driver DT support" posted to the list on Monday. I'd rather go
> for proper DT support instead of custom deferred probing.
>
> On Tuesday 10 March 2015 14:24:52 Tim Nordell wrote:
>> If the subdev isn't available just yet, defer probing of
>> the system.  This is useful if the omap3isp comes up before
>> the I2C subsystem does.
>>
>> Signed-off-by: Tim Nordell <tim.nordell@logicpd.com>
>> ---
>>   drivers/media/platform/omap3isp/isp.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/omap3isp/isp.c
>> b/drivers/media/platform/omap3isp/isp.c index 51c2129..a361c40 100644
>> --- a/drivers/media/platform/omap3isp/isp.c
>> +++ b/drivers/media/platform/omap3isp/isp.c
>> @@ -1811,7 +1811,7 @@ isp_register_subdev_group(struct isp_device *isp,
>>   				"device %s\n", __func__,
>>   				board_info->i2c_adapter_id,
>>   				board_info->board_info->type);
>> -			continue;
>> +			return ERR_PTR(-EPROBE_DEFER);
>>   		}
>>
>>   		subdev = v4l2_i2c_new_subdev_board(&isp->v4l2_dev, adapter,
>> @@ -1898,6 +1898,10 @@ static int isp_register_entities(struct isp_device
>> *isp) unsigned int i;
>>
>>   		sensor = isp_register_subdev_group(isp, subdevs->subdevs);
>> +		if (IS_ERR(sensor)) {
>> +			ret = PTR_ERR(sensor);
>> +			goto done;
>> +		}
>>   		if (sensor == NULL)
>>   			continue;

Re: [PATCH 2/3] omap3isp: Disable CCDC's VD0 and VD1 interrupts when stream is not enabled

[Laurent Pinchart]

Hi Tim,

Thank you for the patch.

On Tuesday 10 March 2015 14:24:53 Tim Nordell wrote:
> During testing there appeared to be a race condition where the IRQs
> for VD0 and VD1 could be triggered while enabling the CCDC module
> before the pipeline status was updated.  Simply modify the trigger
> conditions for VD0 and VD1 so they won't occur when the CCDC module
> is not enabled.
> 
> (When this occurred during testing, the VD0 interrupt was occurring
> over and over again starving the rest of the system.)

I'm curious, might this be caused by the input (adv7180 in your case) being 
enabled before the ISP ? The CCDC is very sensitive to any glitch in its input 
signals, you need to make sure that the source is disabled before its subdev 
s_stream operation is called. Given that the adv7180 driver doesn't implement 
s_stream, I expect it to be free-running, which is definitely a problem.

> Signed-off-by: Tim Nordell <tim.nordell@logicpd.com>
> ---
>  drivers/media/platform/omap3isp/ispccdc.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/ispccdc.c
> b/drivers/media/platform/omap3isp/ispccdc.c index 587489a..d5de843 100644
> --- a/drivers/media/platform/omap3isp/ispccdc.c
> +++ b/drivers/media/platform/omap3isp/ispccdc.c
> @@ -1218,13 +1218,6 @@ static void ccdc_configure(struct isp_ccdc_device
> *ccdc) }
>  	ccdc_config_imgattr(ccdc, ccdc_pattern);
> 
> -	/* Generate VD0 on the last line of the image and VD1 on the
> -	 * 2/3 height line.
> -	 */
> -	isp_reg_writel(isp, ((format->height - 2) << ISPCCDC_VDINT_0_SHIFT) |
> -		       ((format->height * 2 / 3) << ISPCCDC_VDINT_1_SHIFT),
> -		       OMAP3_ISP_IOMEM_CCDC, ISPCCDC_VDINT);
> -
>  	/* CCDC_PAD_SOURCE_OF */
>  	format = &ccdc->formats[CCDC_PAD_SOURCE_OF];
>  	crop = &ccdc->crop;
> @@ -1316,11 +1309,29 @@ unlock:
> 
>  static void __ccdc_enable(struct isp_ccdc_device *ccdc, int enable)
>  {
> +	struct v4l2_mbus_framefmt *format = &ccdc->formats[CCDC_PAD_SINK];
>  	struct isp_device *isp = to_isp_device(ccdc);
> +	int vd0, vd1;
> 
>  	isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_PCR,
>  			ISPCCDC_PCR_EN, enable ? ISPCCDC_PCR_EN : 0);
> 
> +	/* Generate VD0 on the last line of the image and VD1 on the
> +	* 2/3 height line when enabled.  Otherwise, set VD0 and VD1
> +	* interrupts high enough that they won't be generated.
> +	*/
> +	if (enable) {
> +		vd0 = format->height - 2;
> +		vd1 = format->height * 2 / 3;
> +	} else {
> +		vd0 = 0xffff;
> +		vd1 = 0xffff;
> +	}
> +
> +	isp_reg_writel(isp, (vd0 << ISPCCDC_VDINT_0_SHIFT) |
> +		(vd1 << ISPCCDC_VDINT_1_SHIFT),
> +		OMAP3_ISP_IOMEM_CCDC, ISPCCDC_VDINT);
> +
>  	ccdc->running = enable;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] omap3isp: Disable CCDC's VD0 and VD1 interrupts when stream is not enabled

[Tim Nordell]

Laurent -

On 03/18/15 10:19, Laurent Pinchart wrote:
> Hi Tim,
>
> Thank you for the patch.
>
> On Tuesday 10 March 2015 14:24:53 Tim Nordell wrote:
>> During testing there appeared to be a race condition where the IRQs
>> for VD0 and VD1 could be triggered while enabling the CCDC module
>> before the pipeline status was updated.  Simply modify the trigger
>> conditions for VD0 and VD1 so they won't occur when the CCDC module
>> is not enabled.
>>
>> (When this occurred during testing, the VD0 interrupt was occurring
>> over and over again starving the rest of the system.)
> I'm curious, might this be caused by the input (adv7180 in your case) being
> enabled before the ISP ? The CCDC is very sensitive to any glitch in its input
> signals, you need to make sure that the source is disabled before its subdev
> s_stream operation is called. Given that the adv7180 driver doesn't implement
> s_stream, I expect it to be free-running, which is definitely a problem.
>
I'll give that a shot and try add code into the adv7180 driver to turn 
on and off its output signals.  However, it seems like if the driver can 
avoid a problem presented by external hardware (or other drivers), that 
it should.  Something like either turning off the VD0 and VD1 interrupts 
when not in use, or by simply moving the trigger points for those 
interrupts (as I did here) to avoid problems by presented by signals to 
the system is probably a good thing for robustness.

- Tim

Re: [PATCH 2/3] omap3isp: Disable CCDC's VD0 and VD1 interrupts when stream is not enabled

[Laurent Pinchart]

Hi Tim,

On Wednesday 18 March 2015 10:25:34 Tim Nordell wrote:
> On 03/18/15 10:19, Laurent Pinchart wrote:
> > On Tuesday 10 March 2015 14:24:53 Tim Nordell wrote:
> >> During testing there appeared to be a race condition where the IRQs
> >> for VD0 and VD1 could be triggered while enabling the CCDC module
> >> before the pipeline status was updated.  Simply modify the trigger
> >> conditions for VD0 and VD1 so they won't occur when the CCDC module
> >> is not enabled.
> >> 
> >> (When this occurred during testing, the VD0 interrupt was occurring
> >> over and over again starving the rest of the system.)
> > 
> > I'm curious, might this be caused by the input (adv7180 in your case)
> > being enabled before the ISP ? The CCDC is very sensitive to any glitch in
> > its input signals, you need to make sure that the source is disabled
> > before its subdev s_stream operation is called. Given that the adv7180
> > driver doesn't implement s_stream, I expect it to be free-running, which
> > is definitely a problem.
>
> I'll give that a shot and try add code into the adv7180 driver to turn on
> and off its output signals.  However, it seems like if the driver can avoid
> a problem presented by external hardware (or other drivers), that it should. 
> Something like either turning off the VD0 and VD1 interrupts when not in
> use, or by simply moving the trigger points for those interrupts (as I did
> here) to avoid problems by presented by signals to the system is probably a
> good thing for robustness.

I don't disagree with that. I'll have to review the patch in details, as the 
CCDC code is quite sensitive. In order to do so, I'd like to know whether the 
problem in your case was caused by the adv7180 always being enabled. Any luck 
with adding a s_stream implementation in the adv7180 driver ? :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] omap3isp: Disable CCDC's VD0 and VD1 interrupts when stream is not enabled

[Tim Nordell]

Laurent -

On 04/21/15 12:58, Laurent Pinchart wrote:
> Hi Tim,
>
> On Wednesday 18 March 2015 10:25:34 Tim Nordell wrote:
>> I'll give that a shot and try add code into the adv7180 driver to turn on
>> and off its output signals.  However, it seems like if the driver can avoid
>> a problem presented by external hardware (or other drivers), that it should.
>> Something like either turning off the VD0 and VD1 interrupts when not in
>> use, or by simply moving the trigger points for those interrupts (as I did
>> here) to avoid problems by presented by signals to the system is probably a
>> good thing for robustness.
> I don't disagree with that. I'll have to review the patch in details, as the
> CCDC code is quite sensitive. In order to do so, I'd like to know whether the
> problem in your case was caused by the adv7180 always being enabled. Any luck
> with adding a s_stream implementation in the adv7180 driver ? :-)
>

I did add the stream on/off code, but it still seemed to have some 
difficulties.  The codebase has effectively been handed off to our 
client, however, at this point.  I still happen to have hardware (we're 
wrapping things up with the client), but likely I won't have the 
hardware in a week or so.

I still think that the driver should avoid having the interrupts enabled 
if it knows it shouldn't be receiving any at a given point. I personally 
like the approach of modifying the VD0/VD1 trigger points as it 
effectively silences those interrupts without touching the central 
interrupt register (less potential locking issues between the various 
components in the OMAP3 ISP), but it could be reworked of course to 
touch the central interrupt register too.

- Tim