[PATCH 0/8] OMAP3 ISP CCDC fixes

[PATCH 0/8] OMAP3 ISP CCDC fixes

[Laurent Pinchart]

Hello,

This patch series fixes several stability issues related to the CCDC,
especially (but not exclusively) in BT.656 mode.

The patches apply on top of the OMAP3 ISP CCDC BT.656 mode support series
previously posted. You can find both series at

	git://linuxtv.org/pinchartl/media.git omap3isp/bt656

I'm not sure to be completely happy with the last three patches. The CCDC
state machine is getting too complex for my tastes, race conditions becoming
too hard to spot. This doesn't mean the code is wrong, but a rewrite of the
state machine will probably needed sooner than later.

Laurent Pinchart (8):
  omap3isp: ccdc: Disable the video port when unused
  omap3isp: ccdc: Only complete buffer when all fields are captured
  omap3isp: ccdc: Rename __ccdc_handle_stopping to ccdc_handle_stopping
  omap3isp: ccdc: Simplify ccdc_lsc_is_configured()
  omap3isp: ccdc: Increment the frame number at VD0 time for BT.656
  omap3isp: ccdc: Fix freeze when a short frame is received
  omap3isp: ccdc: Don't timeout on stream off when the CCDC is stopped
  omap3isp: ccdc: Restart the CCDC immediately after an underrun in
    BT.656

 drivers/media/platform/omap3isp/ispccdc.c | 233 +++++++++++++++++++-----------
 drivers/media/platform/omap3isp/ispccdc.h |   9 ++
 2 files changed, 154 insertions(+), 88 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH 1/8] omap3isp: ccdc: Disable the video port when unused

[Laurent Pinchart]

The video port doesn't support YUV formats. Disable it when the CCDC
sink pad format is set to YUV instead of leaving it enabled and relying
on downstream modules not to process data they receive from the video
port.

Experiments showed that this fixes some of the CCDC failures to stop,
especially in BT.656 mode.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/omap3isp/ispccdc.c | 70 +++++++++++++------------------
 1 file changed, 29 insertions(+), 41 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispccdc.c b/drivers/media/platform/omap3isp/ispccdc.c
index 150bbf0..ecc37f2 100644
--- a/drivers/media/platform/omap3isp/ispccdc.c
+++ b/drivers/media/platform/omap3isp/ispccdc.c
@@ -808,29 +808,48 @@ static void ccdc_config_vp(struct isp_ccdc_device *ccdc)
 	struct isp_pipeline *pipe = to_isp_pipeline(&ccdc->subdev.entity);
 	struct isp_device *isp = to_isp_device(ccdc);
 	const struct isp_format_info *info;
+	struct v4l2_mbus_framefmt *format;
 	unsigned long l3_ick = pipe->l3_ick;
 	unsigned int max_div = isp->revision == ISP_REVISION_15_0 ? 64 : 8;
 	unsigned int div = 0;
-	u32 fmtcfg_vp;
+	u32 fmtcfg = ISPCCDC_FMTCFG_VPEN;
+
+	format = &ccdc->formats[CCDC_PAD_SOURCE_VP];
+
+	if (!format->code) {
+		/* Disable the video port when the input format isn't supported.
+		 * This is indicated by a pixel code set to 0.
+		 */
+		isp_reg_writel(isp, 0, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_FMTCFG);
+		return;
+	}
+
+	isp_reg_writel(isp, (0 << ISPCCDC_FMT_HORZ_FMTSPH_SHIFT) |
+		       (format->width << ISPCCDC_FMT_HORZ_FMTLNH_SHIFT),
+		       OMAP3_ISP_IOMEM_CCDC, ISPCCDC_FMT_HORZ);
+	isp_reg_writel(isp, (0 << ISPCCDC_FMT_VERT_FMTSLV_SHIFT) |
+		       ((format->height + 1) << ISPCCDC_FMT_VERT_FMTLNV_SHIFT),
+		       OMAP3_ISP_IOMEM_CCDC, ISPCCDC_FMT_VERT);
 
-	fmtcfg_vp = isp_reg_readl(isp, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_FMTCFG)
-		  & ~(ISPCCDC_FMTCFG_VPIN_MASK | ISPCCDC_FMTCFG_VPIF_FRQ_MASK);
+	isp_reg_writel(isp, (format->width << ISPCCDC_VP_OUT_HORZ_NUM_SHIFT) |
+		       (format->height << ISPCCDC_VP_OUT_VERT_NUM_SHIFT),
+		       OMAP3_ISP_IOMEM_CCDC, ISPCCDC_VP_OUT);
 
 	info = omap3isp_video_format_info(ccdc->formats[CCDC_PAD_SINK].code);
 
 	switch (info->width) {
 	case 8:
 	case 10:
-		fmtcfg_vp |= ISPCCDC_FMTCFG_VPIN_9_0;
+		fmtcfg |= ISPCCDC_FMTCFG_VPIN_9_0;
 		break;
 	case 11:
-		fmtcfg_vp |= ISPCCDC_FMTCFG_VPIN_10_1;
+		fmtcfg |= ISPCCDC_FMTCFG_VPIN_10_1;
 		break;
 	case 12:
-		fmtcfg_vp |= ISPCCDC_FMTCFG_VPIN_11_2;
+		fmtcfg |= ISPCCDC_FMTCFG_VPIN_11_2;
 		break;
 	case 13:
-		fmtcfg_vp |= ISPCCDC_FMTCFG_VPIN_12_3;
+		fmtcfg |= ISPCCDC_FMTCFG_VPIN_12_3;
 		break;
 	}
 
@@ -840,24 +859,9 @@ static void ccdc_config_vp(struct isp_ccdc_device *ccdc)
 		div = l3_ick / pipe->external_rate;
 
 	div = clamp(div, 2U, max_div);
-	fmtcfg_vp |= (div - 2) << ISPCCDC_FMTCFG_VPIF_FRQ_SHIFT;
+	fmtcfg |= (div - 2) << ISPCCDC_FMTCFG_VPIF_FRQ_SHIFT;
 
-	isp_reg_writel(isp, fmtcfg_vp, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_FMTCFG);
-}
-
-/*
- * ccdc_enable_vp - Enable Video Port.
- * @ccdc: Pointer to ISP CCDC device.
- * @enable: 0 Disables VP, 1 Enables VP
- *
- * This is needed for outputting image to Preview, H3A and HIST ISP submodules.
- */
-static void ccdc_enable_vp(struct isp_ccdc_device *ccdc, u8 enable)
-{
-	struct isp_device *isp = to_isp_device(ccdc);
-
-	isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_FMTCFG,
-			ISPCCDC_FMTCFG_VPEN, enable ? ISPCCDC_FMTCFG_VPEN : 0);
+	isp_reg_writel(isp, fmtcfg, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_FMTCFG);
 }
 
 /*
@@ -1282,18 +1286,7 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
 	isp_reg_writel(isp, syn_mode, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SYN_MODE);
 
 	/* CCDC_PAD_SOURCE_VP */
-	format = &ccdc->formats[CCDC_PAD_SOURCE_VP];
-
-	isp_reg_writel(isp, (0 << ISPCCDC_FMT_HORZ_FMTSPH_SHIFT) |
-		       (format->width << ISPCCDC_FMT_HORZ_FMTLNH_SHIFT),
-		       OMAP3_ISP_IOMEM_CCDC, ISPCCDC_FMT_HORZ);
-	isp_reg_writel(isp, (0 << ISPCCDC_FMT_VERT_FMTSLV_SHIFT) |
-		       ((format->height + 1) << ISPCCDC_FMT_VERT_FMTLNV_SHIFT),
-		       OMAP3_ISP_IOMEM_CCDC, ISPCCDC_FMT_VERT);
-
-	isp_reg_writel(isp, (format->width << ISPCCDC_VP_OUT_HORZ_NUM_SHIFT) |
-		       (format->height << ISPCCDC_VP_OUT_VERT_NUM_SHIFT),
-		       OMAP3_ISP_IOMEM_CCDC, ISPCCDC_VP_OUT);
+	ccdc_config_vp(ccdc);
 
 	/* Lens shading correction. */
 	spin_lock_irqsave(&ccdc->lsc.req_lock, flags);
@@ -1837,11 +1830,6 @@ static int ccdc_set_stream(struct v4l2_subdev *sd, int enable)
 
 		ccdc_configure(ccdc);
 
-		/* TODO: Don't configure the video port if all of its output
-		 * links are inactive.
-		 */
-		ccdc_config_vp(ccdc);
-		ccdc_enable_vp(ccdc, 1);
 		ccdc_print_status(ccdc);
 	}
 
-- 
1.8.5.5

[PATCH 2/8] omap3isp: ccdc: Only complete buffer when all fields are captured

[Laurent Pinchart]

Checking that the captured field corresponds to the last required field
depending on the requested field order before completing the buffer
isn't enough. When the first field at stream start corresponds to the
last required field, this would result in returning an interlaced buffer
containing a single field.

Fix this by keeping track of the fields captured in the buffer, and make
sure that both fields are present for alternate field orders.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/omap3isp/ispccdc.c | 78 ++++++++++++++++++++-----------
 drivers/media/platform/omap3isp/ispccdc.h |  7 +++
 2 files changed, 58 insertions(+), 27 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispccdc.c b/drivers/media/platform/omap3isp/ispccdc.c
index ecc37f2..56c3129 100644
--- a/drivers/media/platform/omap3isp/ispccdc.c
+++ b/drivers/media/platform/omap3isp/ispccdc.c
@@ -1134,6 +1134,7 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
 	u32 ccdc_pattern;
 
 	ccdc->bt656 = false;
+	ccdc->fields = 0;
 
 	pad = media_entity_remote_pad(&ccdc->pads[CCDC_PAD_SINK]);
 	sensor = media_entity_to_v4l2_subdev(pad->entity);
@@ -1529,12 +1530,59 @@ done:
 	spin_unlock_irqrestore(&ccdc->lsc.req_lock, flags);
 }
 
+/*
+ * Check whether the CCDC has captured all fields necessary to complete the
+ * buffer.
+ */
+static bool ccdc_has_all_fields(struct isp_ccdc_device *ccdc)
+{
+	struct isp_pipeline *pipe = to_isp_pipeline(&ccdc->subdev.entity);
+	struct isp_device *isp = to_isp_device(ccdc);
+	enum v4l2_field of_field = ccdc->formats[CCDC_PAD_SOURCE_OF].field;
+	enum v4l2_field field;
+
+	/* When the input is progressive fields don't matter. */
+	if (of_field == V4L2_FIELD_NONE)
+		return true;
+
+	/* Read the current field identifier. */
+	field = isp_reg_readl(isp, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SYN_MODE)
+	      & ISPCCDC_SYN_MODE_FLDSTAT
+	      ? V4L2_FIELD_BOTTOM : V4L2_FIELD_TOP;
+
+	/* When capturing fields in alternate order just store the current field
+	 * identifier in the pipeline.
+	 */
+	if (of_field == V4L2_FIELD_ALTERNATE) {
+		pipe->field = field;
+		return true;
+	}
+
+	/* The format is interlaced. Make sure we've captured both fields. */
+	ccdc->fields |= field == V4L2_FIELD_BOTTOM
+		      ? CCDC_FIELD_BOTTOM : CCDC_FIELD_TOP;
+
+	if (ccdc->fields != CCDC_FIELD_BOTH)
+		return false;
+
+	/* Verify that the field just captured corresponds to the last field
+	 * needed based on the desired field order.
+	 */
+	if ((of_field == V4L2_FIELD_INTERLACED_TB && field == V4L2_FIELD_TOP) ||
+	    (of_field == V4L2_FIELD_INTERLACED_BT && field == V4L2_FIELD_BOTTOM))
+		return false;
+
+	/* The buffer can be completed, reset the fields for the next buffer. */
+	ccdc->fields = 0;
+
+	return true;
+}
+
 static int ccdc_isr_buffer(struct isp_ccdc_device *ccdc)
 {
 	struct isp_pipeline *pipe = to_isp_pipeline(&ccdc->subdev.entity);
 	struct isp_device *isp = to_isp_device(ccdc);
 	struct isp_buffer *buffer;
-	enum v4l2_field field;
 
 	/* The CCDC generates VD0 interrupts even when disabled (the datasheet
 	 * doesn't explicitly state if that's supposed to happen or not, so it
@@ -1554,11 +1602,6 @@ static int ccdc_isr_buffer(struct isp_ccdc_device *ccdc)
 		return 1;
 	}
 
-	/* Read the current field identifier. */
-	field = isp_reg_readl(isp, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SYN_MODE)
-	      & ISPCCDC_SYN_MODE_FLDSTAT
-	      ? V4L2_FIELD_BOTTOM : V4L2_FIELD_TOP;
-
 	/* Wait for the CCDC to become idle. */
 	if (ccdc_sbl_wait_idle(ccdc, 1000)) {
 		dev_info(isp->dev, "CCDC won't become idle!\n");
@@ -1567,27 +1610,8 @@ static int ccdc_isr_buffer(struct isp_ccdc_device *ccdc)
 		return 0;
 	}
 
-	switch (ccdc->formats[CCDC_PAD_SOURCE_OF].field) {
-	case V4L2_FIELD_ALTERNATE:
-		/* When capturing fields in alternate order store the current
-		 * field identifier in the pipeline.
-		 */
-		pipe->field = field;
-		break;
-
-	case V4L2_FIELD_INTERLACED_TB:
-		/* When interleaving fields only complete the buffer after
-		 * capturing the second field.
-		 */
-		if (field == V4L2_FIELD_TOP)
-			return 1;
-		break;
-
-	case V4L2_FIELD_INTERLACED_BT:
-		if (field == V4L2_FIELD_BOTTOM)
-			return 1;
-		break;
-	}
+	if (!ccdc_has_all_fields(ccdc))
+		return 1;
 
 	buffer = omap3isp_video_buffer_next(&ccdc->video_out);
 	if (buffer != NULL)
diff --git a/drivers/media/platform/omap3isp/ispccdc.h b/drivers/media/platform/omap3isp/ispccdc.h
index c325b89..731ecc7 100644
--- a/drivers/media/platform/omap3isp/ispccdc.h
+++ b/drivers/media/platform/omap3isp/ispccdc.h
@@ -93,6 +93,10 @@ struct ispccdc_lsc {
 #define CCDC_PAD_SOURCE_VP		2
 #define CCDC_PADS_NUM			3
 
+#define CCDC_FIELD_TOP			1
+#define CCDC_FIELD_BOTTOM		2
+#define CCDC_FIELD_BOTH			3
+
 /*
  * struct isp_ccdc_device - Structure for the CCDC module to store its own
  *			    information
@@ -114,6 +118,7 @@ struct ispccdc_lsc {
  * @update: Bitmask of controls to update during the next interrupt
  * @shadow_update: Controls update in progress by userspace
  * @bt656: Whether the input interface uses BT.656 synchronization
+ * @fields: The fields (CCDC_FIELD_*) stored in the current buffer
  * @underrun: A buffer underrun occurred and a new buffer has been queued
  * @state: Streaming state
  * @lock: Serializes shadow_update with interrupt handler
@@ -143,6 +148,8 @@ struct isp_ccdc_device {
 	unsigned int shadow_update;
 
 	bool bt656;
+	unsigned int fields;
+
 	unsigned int underrun:1;
 	enum isp_pipeline_stream_state state;
 	spinlock_t lock;
-- 
1.8.5.5

Re: [PATCH 2/8] omap3isp: ccdc: Only complete buffer when all fields are captured

[Laurent Pinchart]

On Friday 01 August 2014 07:08:11 Raymond Jender wrote:
> Get me the f u c k  off this list!!

Swearing won't get you anywhere. There's (to my knowledge) nobody registered 
to this list who has administrative permissions needed to modify the 
subscribers list.

Unsubscription information is available at the end of every e-mail you get 
from the list. If you can't understand them, you could ask politely. If you're 
not successful following the procedure you could report the problem politely 
with enough *details* to allow someone to help you. If you still can't 
unsubscribe the footer also has a link to the mailing list manager website, 
which includes a contact e-mail address. If you can't unsubscribe despite all 
that information maybe you should stop using e-mail.

-- 
Regards,

Laurent Pinchart

[PATCH 3/8] omap3isp: ccdc: Rename __ccdc_handle_stopping to ccdc_handle_stopping

[Laurent Pinchart]

There's no need for a double underscore in the function name, remove it.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/omap3isp/ispccdc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispccdc.c b/drivers/media/platform/omap3isp/ispccdc.c
index 56c3129..cd62d29 100644
--- a/drivers/media/platform/omap3isp/ispccdc.c
+++ b/drivers/media/platform/omap3isp/ispccdc.c
@@ -1413,14 +1413,14 @@ static int ccdc_sbl_wait_idle(struct isp_ccdc_device *ccdc,
 	return -EBUSY;
 }
 
-/* __ccdc_handle_stopping - Handle CCDC and/or LSC stopping sequence
+/* ccdc_handle_stopping - Handle CCDC and/or LSC stopping sequence
  * @ccdc: Pointer to ISP CCDC device.
  * @event: Pointing which event trigger handler
  *
  * Return 1 when the event and stopping request combination is satisfied,
  * zero otherwise.
  */
-static int __ccdc_handle_stopping(struct isp_ccdc_device *ccdc, u32 event)
+static int ccdc_handle_stopping(struct isp_ccdc_device *ccdc, u32 event)
 {
 	int rval = 0;
 
@@ -1502,7 +1502,7 @@ static void ccdc_lsc_isr(struct isp_ccdc_device *ccdc, u32 events)
 	if (ccdc->lsc.state == LSC_STATE_STOPPING)
 		ccdc->lsc.state = LSC_STATE_STOPPED;
 
-	if (__ccdc_handle_stopping(ccdc, CCDC_EVENT_LSC_DONE))
+	if (ccdc_handle_stopping(ccdc, CCDC_EVENT_LSC_DONE))
 		goto done;
 
 	if (ccdc->lsc.state != LSC_STATE_RECONFIG)
@@ -1642,7 +1642,7 @@ static void ccdc_vd0_isr(struct isp_ccdc_device *ccdc)
 		restart = ccdc_isr_buffer(ccdc);
 
 	spin_lock_irqsave(&ccdc->lock, flags);
-	if (__ccdc_handle_stopping(ccdc, CCDC_EVENT_VD0)) {
+	if (ccdc_handle_stopping(ccdc, CCDC_EVENT_VD0)) {
 		spin_unlock_irqrestore(&ccdc->lock, flags);
 		return;
 	}
@@ -1702,7 +1702,7 @@ static void ccdc_vd1_isr(struct isp_ccdc_device *ccdc)
 		break;
 	}
 
-	if (__ccdc_handle_stopping(ccdc, CCDC_EVENT_VD1))
+	if (ccdc_handle_stopping(ccdc, CCDC_EVENT_VD1))
 		goto done;
 
 	if (ccdc->lsc.request == NULL)
-- 
1.8.5.5

[PATCH 4/8] omap3isp: ccdc: Simplify ccdc_lsc_is_configured()

[Laurent Pinchart]

Use a local variable to avoid the duplicate spin_unlock_irqrestore()
call.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/omap3isp/ispccdc.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispccdc.c b/drivers/media/platform/omap3isp/ispccdc.c
index cd62d29..6a62cb7 100644
--- a/drivers/media/platform/omap3isp/ispccdc.c
+++ b/drivers/media/platform/omap3isp/ispccdc.c
@@ -481,14 +481,13 @@ done:
 static inline int ccdc_lsc_is_configured(struct isp_ccdc_device *ccdc)
 {
 	unsigned long flags;
+	int ret;
 
 	spin_lock_irqsave(&ccdc->lsc.req_lock, flags);
-	if (ccdc->lsc.active) {
-		spin_unlock_irqrestore(&ccdc->lsc.req_lock, flags);
-		return 1;
-	}
+	ret = ccdc->lsc.active != NULL;
 	spin_unlock_irqrestore(&ccdc->lsc.req_lock, flags);
-	return 0;
+
+	return ret;
 }
 
 static int ccdc_lsc_enable(struct isp_ccdc_device *ccdc)
-- 
1.8.5.5

[PATCH 5/8] omap3isp: ccdc: Increment the frame number at VD0 time for BT.656

[Laurent Pinchart]

We will stop using VD1 in BT.656 mode, move frame number increment to
the VD0 interrupt handler.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/omap3isp/ispccdc.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispccdc.c b/drivers/media/platform/omap3isp/ispccdc.c
index 6a62cb7..112bced 100644
--- a/drivers/media/platform/omap3isp/ispccdc.c
+++ b/drivers/media/platform/omap3isp/ispccdc.c
@@ -1637,6 +1637,16 @@ static void ccdc_vd0_isr(struct isp_ccdc_device *ccdc)
 	unsigned long flags;
 	int restart = 0;
 
+	/* In BT.656 mode the CCDC doesn't generate an HS/VS interrupt. We thus
+	 * need to increment the frame counter here.
+	 */
+	if (ccdc->bt656) {
+		struct isp_pipeline *pipe =
+			to_isp_pipeline(&ccdc->subdev.entity);
+
+		atomic_inc(&pipe->frame_number);
+	}
+
 	if (ccdc->output & CCDC_OUTPUT_MEMORY)
 		restart = ccdc_isr_buffer(ccdc);
 
@@ -1662,16 +1672,6 @@ static void ccdc_vd1_isr(struct isp_ccdc_device *ccdc)
 {
 	unsigned long flags;
 
-	/* In BT.656 mode the CCDC doesn't generate an HS/VS interrupt. We thus
-	 * need to increment the frame counter here.
-	 */
-	if (ccdc->bt656) {
-		struct isp_pipeline *pipe =
-			to_isp_pipeline(&ccdc->subdev.entity);
-
-		atomic_inc(&pipe->frame_number);
-	}
-
 	spin_lock_irqsave(&ccdc->lsc.req_lock, flags);
 
 	/*
-- 
1.8.5.5

[PATCH 6/8] omap3isp: ccdc: Fix freeze when a short frame is received

[Laurent Pinchart]

In BT.656 mode the synchronization signals are generated by the CCDC
from the embedded sync codes. The VD0 and VD1 interrupts are thus only
triggered when the CCDC is enabled, unlike external sync mode where the
line counter runs even when the CCDC is stopped. We can't disable the
CCDC at VD1 time, as no VD0 interrupt would be generated for a short
frame, which would result in the CCDC being stopped and no VD interrupt
generated anymore. The CCDC is stopped from the VD0 interrupt handler
instead for BT.656.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/omap3isp/ispccdc.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/media/platform/omap3isp/ispccdc.c b/drivers/media/platform/omap3isp/ispccdc.c
index 112bced..ff2ea2b 100644
--- a/drivers/media/platform/omap3isp/ispccdc.c
+++ b/drivers/media/platform/omap3isp/ispccdc.c
@@ -1647,10 +1647,27 @@ static void ccdc_vd0_isr(struct isp_ccdc_device *ccdc)
 		atomic_inc(&pipe->frame_number);
 	}
 
+	/* Emulate a VD1 interrupt for BT.656 mode, as we can't stop the CCDC in
+	 * the VD1 interrupt handler in that mode without risking a CCDC stall
+	 * if a short frame is received.
+	 */
+	if (ccdc->bt656) {
+		spin_lock_irqsave(&ccdc->lock, flags);
+		if (ccdc->state == ISP_PIPELINE_STREAM_CONTINUOUS &&
+		    ccdc->output & CCDC_OUTPUT_MEMORY) {
+			if (ccdc->lsc.state != LSC_STATE_STOPPED)
+				__ccdc_lsc_enable(ccdc, 0);
+			__ccdc_enable(ccdc, 0);
+		}
+		ccdc_handle_stopping(ccdc, CCDC_EVENT_VD1);
+		spin_unlock_irqrestore(&ccdc->lock, flags);
+	}
+
 	if (ccdc->output & CCDC_OUTPUT_MEMORY)
 		restart = ccdc_isr_buffer(ccdc);
 
 	spin_lock_irqsave(&ccdc->lock, flags);
+
 	if (ccdc_handle_stopping(ccdc, CCDC_EVENT_VD0)) {
 		spin_unlock_irqrestore(&ccdc->lock, flags);
 		return;
@@ -1672,6 +1689,18 @@ static void ccdc_vd1_isr(struct isp_ccdc_device *ccdc)
 {
 	unsigned long flags;
 
+	/* In BT.656 mode the synchronization signals are generated by the CCDC
+	 * from the embedded sync codes. The VD0 and VD1 interrupts are thus
+	 * only triggered when the CCDC is enabled, unlike external sync mode
+	 * where the line counter runs even when the CCDC is stopped. We can't
+	 * disable the CCDC at VD1 time, as no VD0 interrupt would be generated
+	 * for a short frame, which would result in the CCDC being stopped and
+	 * no VD interrupt generated anymore. The CCDC is stopped from the VD0
+	 * interrupt handler instead for BT.656.
+	 */
+	if (ccdc->bt656)
+		return;
+
 	spin_lock_irqsave(&ccdc->lsc.req_lock, flags);
 
 	/*
-- 
1.8.5.5

[PATCH 7/8] omap3isp: ccdc: Don't timeout on stream off when the CCDC is stopped

[Laurent Pinchart]

When the CCDC is already stopped due to a buffer underrun, the stop
state machine won't advance in BT.656 mode as no interrupt are generated
by the stopped CCDC in that mode. Handle this case explicitly in the
ccdc_disable() function.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/omap3isp/ispccdc.c | 4 ++++
 drivers/media/platform/omap3isp/ispccdc.h | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/drivers/media/platform/omap3isp/ispccdc.c b/drivers/media/platform/omap3isp/ispccdc.c
index ff2ea2b..ec0a0e8 100644
--- a/drivers/media/platform/omap3isp/ispccdc.c
+++ b/drivers/media/platform/omap3isp/ispccdc.c
@@ -1320,6 +1320,8 @@ static void __ccdc_enable(struct isp_ccdc_device *ccdc, int enable)
 
 	isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_PCR,
 			ISPCCDC_PCR_EN, enable ? ISPCCDC_PCR_EN : 0);
+
+	ccdc->running = enable;
 }
 
 static int ccdc_disable(struct isp_ccdc_device *ccdc)
@@ -1330,6 +1332,8 @@ static int ccdc_disable(struct isp_ccdc_device *ccdc)
 	spin_lock_irqsave(&ccdc->lock, flags);
 	if (ccdc->state == ISP_PIPELINE_STREAM_CONTINUOUS)
 		ccdc->stopping = CCDC_STOP_REQUEST;
+	if (!ccdc->running)
+		ccdc->stopping = CCDC_STOP_FINISHED;
 	spin_unlock_irqrestore(&ccdc->lock, flags);
 
 	ret = wait_event_timeout(ccdc->wait,
diff --git a/drivers/media/platform/omap3isp/ispccdc.h b/drivers/media/platform/omap3isp/ispccdc.h
index 731ecc7..3440a70 100644
--- a/drivers/media/platform/omap3isp/ispccdc.h
+++ b/drivers/media/platform/omap3isp/ispccdc.h
@@ -124,6 +124,7 @@ struct ispccdc_lsc {
  * @lock: Serializes shadow_update with interrupt handler
  * @wait: Wait queue used to stop the module
  * @stopping: Stopping state
+ * @running: Is the CCDC hardware running
  * @ioctl_lock: Serializes ioctl calls and LSC requests freeing
  */
 struct isp_ccdc_device {
@@ -155,6 +156,7 @@ struct isp_ccdc_device {
 	spinlock_t lock;
 	wait_queue_head_t wait;
 	unsigned int stopping;
+	bool running;
 	struct mutex ioctl_lock;
 };
 
-- 
1.8.5.5

[PATCH 8/8] omap3isp: ccdc: Restart the CCDC immediately after an underrun in BT.656

[Laurent Pinchart]

As the CCDC doesn't generate interrupts when stopped in BT.656 mode,
restart it immediately when the next buffer after an underrun is queued
instead of relying on the interrupt handler to restart the CCDC.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/omap3isp/ispccdc.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispccdc.c b/drivers/media/platform/omap3isp/ispccdc.c
index ec0a0e8..cabf46b 100644
--- a/drivers/media/platform/omap3isp/ispccdc.c
+++ b/drivers/media/platform/omap3isp/ispccdc.c
@@ -1789,6 +1789,8 @@ int omap3isp_ccdc_isr(struct isp_ccdc_device *ccdc, u32 events)
 static int ccdc_video_queue(struct isp_video *video, struct isp_buffer *buffer)
 {
 	struct isp_ccdc_device *ccdc = &video->isp->isp_ccdc;
+	unsigned long flags;
+	bool restart = false;
 
 	if (!(ccdc->output & CCDC_OUTPUT_MEMORY))
 		return -ENODEV;
@@ -1797,9 +1799,20 @@ static int ccdc_video_queue(struct isp_video *video, struct isp_buffer *buffer)
 
 	/* We now have a buffer queued on the output, restart the pipeline
 	 * on the next CCDC interrupt if running in continuous mode (or when
-	 * starting the stream).
+	 * starting the stream) in external sync mode, or immediately in BT.656
+	 * sync mode as no CCDC interrupt is generated when the CCDC is stopped
+	 * in that case.
 	 */
-	ccdc->underrun = 1;
+	spin_lock_irqsave(&ccdc->lock, flags);
+	if (ccdc->state == ISP_PIPELINE_STREAM_CONTINUOUS && !ccdc->running &&
+	    ccdc->bt656)
+		restart = 1;
+	else
+		ccdc->underrun = 1;
+	spin_unlock_irqrestore(&ccdc->lock, flags);
+
+	if (restart)
+		ccdc_enable(ccdc);
 
 	return 0;
 }
-- 
1.8.5.5

Re: [PATCH 0/8] OMAP3 ISP CCDC fixes

[Enrico]

On Fri, Aug 1, 2014 at 3:46 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hello,
>
> This patch series fixes several stability issues related to the CCDC,
> especially (but not exclusively) in BT.656 mode.
>
> The patches apply on top of the OMAP3 ISP CCDC BT.656 mode support series
> previously posted. You can find both series at
>
>         git://linuxtv.org/pinchartl/media.git omap3isp/bt656
>
> I'm not sure to be completely happy with the last three patches. The CCDC
> state machine is getting too complex for my tastes, race conditions becoming
> too hard to spot. This doesn't mean the code is wrong, but a rewrite of the
> state machine will probably needed sooner than later.

I tested this patch series on an igep proton (omap3530) with tvp5150,
kernel 3.16, bt656 mode.
All issues i had with the first series are fixed, so:

Tested-by: Enrico Butera <ebutera@users.sourceforge.net>