diff mbox

[1/2] omap3: ISP: Fix the failure of CCDC capture during suspend/resume

Message ID 1312984992-19315-1-git-send-email-deepthy.ravi@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Deepthy Ravi Aug. 10, 2011, 2:03 p.m. UTC
From: Abhilash K V <abhilash.kv@ti.com>

While resuming from the "suspended to memory" state,
occasionally CCDC fails to get enabled and thus fails
to capture frames any more till the next suspend/resume
is issued.
This is a race condition which happens only when a CCDC
frame-completion ISR is pending even as ISP device's
isp_pm_prepare() is getting called and only one buffer
is left on the DMA queue.
The DMA queue buffers are thus depleted which results in
its underrun.So when ISP resumes there are no buffers on
the queue (as the application which can issue buffers is
yet to resume) to start video capture.
This fix addresses this issue by dequeuing and enqueing
the last buffer in isp_pm_prepare() after its DMA gets
completed. Thus,when ISP resumes it always finds atleast
one buffer on the DMA queue - this is true if application
uses only 3 buffers.

Reviewed-by: Vaibhav Hiremath <hvaibhav@ti.com>
Signed-off-by: Abhilash K V <abhilash.kv@ti.com>
Signed-off-by: Deepthy Ravi <deepthy.ravi@ti.com>
---
 drivers/media/video/omap3isp/isp.c      |   20 +++++++++++
 drivers/media/video/omap3isp/isp.h      |    4 ++
 drivers/media/video/omap3isp/ispvideo.c |   56 ++++++++++++++++++++++++++++++-
 drivers/media/video/omap3isp/ispvideo.h |    2 +
 4 files changed, 81 insertions(+), 1 deletions(-)

Comments

Laurent Pinchart Aug. 30, 2011, 9:07 p.m. UTC | #1
Hi,

On Wednesday 10 August 2011 16:03:12 Deepthy Ravi wrote:
> From: Abhilash K V <abhilash.kv@ti.com>
> 
> While resuming from the "suspended to memory" state,
> occasionally CCDC fails to get enabled and thus fails
> to capture frames any more till the next suspend/resume
> is issued.
> This is a race condition which happens only when a CCDC
> frame-completion ISR is pending even as ISP device's
> isp_pm_prepare() is getting called and only one buffer
> is left on the DMA queue.
> The DMA queue buffers are thus depleted which results in
> its underrun.So when ISP resumes there are no buffers on
> the queue (as the application which can issue buffers is
> yet to resume) to start video capture.
> This fix addresses this issue by dequeuing and enqueing
> the last buffer in isp_pm_prepare() after its DMA gets
> completed. Thus,when ISP resumes it always finds atleast
> one buffer on the DMA queue - this is true if application
> uses only 3 buffers.

How is that problem specific to the CCDC ? Can't it be reproduce at the 
preview engine or resizer output as well ?
Koyamangalath, Abhilash Sept. 7, 2011, 10:10 a.m. UTC | #2
On Tue, Aug 30, 2011  Laurent Pinchart wrote:
> Hi,
>
> On Wednesday 10 August 2011 16:03:12 Deepthy Ravi wrote:
>> From: Abhilash K V <abhilash.kv@ti.com>
>>
>> While resuming from the "suspended to memory" state,
>> occasionally CCDC fails to get enabled and thus fails
>> to capture frames any more till the next suspend/resume
>> is issued.
>> This is a race condition which happens only when a CCDC
>> frame-completion ISR is pending even as ISP device's
>> isp_pm_prepare() is getting called and only one buffer
>> is left on the DMA queue.
>> The DMA queue buffers are thus depleted which results in
>> its underrun.So when ISP resumes there are no buffers on
>> the queue (as the application which can issue buffers is
>> yet to resume) to start video capture.
>> This fix addresses this issue by dequeuing and enqueing
>> the last buffer in isp_pm_prepare() after its DMA gets
>> completed. Thus,when ISP resumes it always finds atleast
>> one buffer on the DMA queue - this is true if application
>> uses only 3 buffers.
>
> How is that problem specific to the CCDC ? Can't it be reproduce at the
> preview engine or resizer output as well ?
[Abhilash K V]Yes,  I believe this issue would crop with preview and resizer too though I have
not been able to try these out.
>
> --
> Regards,
>
> Laurent Pinchart
>

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laurent Pinchart Sept. 7, 2011, 10:36 a.m. UTC | #3
Hi,

On Wednesday 07 September 2011 12:10:23 Koyamangalath, Abhilash wrote:
> On Tue, Aug 30, 2011  Laurent Pinchart wrote:
> > On Wednesday 10 August 2011 16:03:12 Deepthy Ravi wrote:
> >> From: Abhilash K V <abhilash.kv@ti.com>
> >> 
> >> While resuming from the "suspended to memory" state,
> >> occasionally CCDC fails to get enabled and thus fails
> >> to capture frames any more till the next suspend/resume
> >> is issued.
> >> This is a race condition which happens only when a CCDC
> >> frame-completion ISR is pending even as ISP device's
> >> isp_pm_prepare() is getting called and only one buffer
> >> is left on the DMA queue.
> >> The DMA queue buffers are thus depleted which results in
> >> its underrun.So when ISP resumes there are no buffers on
> >> the queue (as the application which can issue buffers is
> >> yet to resume) to start video capture.
> >> This fix addresses this issue by dequeuing and enqueing
> >> the last buffer in isp_pm_prepare() after its DMA gets
> >> completed. Thus,when ISP resumes it always finds atleast
> >> one buffer on the DMA queue - this is true if application
> >> uses only 3 buffers.
> > 
> > How is that problem specific to the CCDC ? Can't it be reproduce at the
> > preview engine or resizer output as well ?
> 
> Yes, I believe this issue would crop with preview and resizer too though I
> have not been able to try these out.

That's my belief as well. In that case the fix should be generic, not CCDC-
specific.
diff mbox

Patch

diff --git a/drivers/media/video/omap3isp/isp.c b/drivers/media/video/omap3isp/isp.c
index 94b6ed8..6604fbd 100644
--- a/drivers/media/video/omap3isp/isp.c
+++ b/drivers/media/video/omap3isp/isp.c
@@ -1566,8 +1566,20 @@  static int isp_pm_prepare(struct device *dev)
 {
 	struct isp_device *isp = dev_get_drvdata(dev);
 	int reset;
+	int err = 0;
+	struct isp_ccdc_device *ccdc = &isp->isp_ccdc;
+	struct isp_video *video = &ccdc->video_out;
+	struct isp_pipeline *pipe = to_isp_pipeline(&video->video.entity);
+	unsigned long flags;
 
 	WARN_ON(mutex_is_locked(&isp->isp_mutex));
+	spin_lock_irqsave(&pipe->lock, flags);
+	pipe->state |= ISP_PIPELINE_PREPARE_SUSPEND;
+	spin_unlock_irqrestore(&pipe->lock, flags);
+
+	err = isp_video_handle_buffer_starvation(video);
+	if (err < 0)
+		return err;
 
 	if (isp->ref_count == 0)
 		return 0;
@@ -1596,6 +1608,14 @@  static int isp_pm_suspend(struct device *dev)
 static int isp_pm_resume(struct device *dev)
 {
 	struct isp_device *isp = dev_get_drvdata(dev);
+	struct isp_ccdc_device *ccdc = &isp->isp_ccdc;
+	struct isp_video *video = &ccdc->video_out;
+	struct isp_pipeline *pipe = to_isp_pipeline(&video->video.entity);
+	unsigned long flags;
+
+	spin_lock_irqsave(&pipe->lock, flags);
+	pipe->state &= ~ISP_PIPELINE_PREPARE_SUSPEND;
+	spin_unlock_irqrestore(&pipe->lock, flags);
 
 	if (isp->ref_count == 0)
 		return 0;
diff --git a/drivers/media/video/omap3isp/isp.h b/drivers/media/video/omap3isp/isp.h
index 841870f..e391974 100644
--- a/drivers/media/video/omap3isp/isp.h
+++ b/drivers/media/video/omap3isp/isp.h
@@ -349,6 +349,10 @@  int omap3isp_register_entities(struct platform_device *pdev,
 			       struct v4l2_device *v4l2_dev);
 void omap3isp_unregister_entities(struct platform_device *pdev);
 
+#ifdef CONFIG_PM
+int isp_video_handle_buffer_starvation(struct isp_video *video);
+#endif
+
 /*
  * isp_reg_readl - Read value of an OMAP3 ISP register
  * @dev: Device pointer specific to the OMAP3 ISP.
diff --git a/drivers/media/video/omap3isp/ispvideo.c b/drivers/media/video/omap3isp/ispvideo.c
index 5833a0e..bf149a7 100644
--- a/drivers/media/video/omap3isp/ispvideo.c
+++ b/drivers/media/video/omap3isp/ispvideo.c
@@ -523,6 +523,26 @@  static int isp_video_buffer_prepare(struct isp_video_buffer *buf)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int isp_video_deq_enq(struct isp_video_queue *queue)
+{
+	int err = 0;
+	struct v4l2_buffer vbuf;
+
+	vbuf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	/* blocking dequeue to ensure DMA is done */
+	err = omap3isp_video_queue_dqbuf(queue, &vbuf, 0);
+	if (err < 0)
+		return err;
+	else {
+		err = omap3isp_video_queue_qbuf(queue, &vbuf);
+		if (err < 0)
+			return err;
+	}
+	return err;
+}
+#endif
+
 /*
  * isp_video_buffer_queue - Add buffer to streaming queue
  * @buf: Video buffer
@@ -546,7 +566,7 @@  static void isp_video_buffer_queue(struct isp_video_buffer *buf)
 	empty = list_empty(&video->dmaqueue);
 	list_add_tail(&buffer->buffer.irqlist, &video->dmaqueue);
 
-	if (empty) {
+	if (empty && !(pipe->state & ISP_PIPELINE_PREPARE_SUSPEND)) {
 		if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
 			state = ISP_PIPELINE_QUEUE_OUTPUT;
 		else
@@ -687,6 +707,40 @@  void omap3isp_video_resume(struct isp_video *video, int continuous)
 	}
 }
 
+#ifdef CONFIG_PM
+
+/*
+ * isp_video_handle_buffer_starvation - Handles the case when there are only 1
+ * or less active buffers present on the dmaqueue while preparing for suspend.
+ *
+ * @video: ISP video object
+ *
+ * This function is intended to be used in suspend/resume scenario. While
+ * preparing for suspend, if the number of active buffers on dmaqueue is 0 or 1,
+ * the last buffer is dequeued after DMA completes and re-queued again. This
+ * prevents ISP_VIDEO_DMAQUEUE_UNDERRUN from occuring on issue of resume.
+ */
+int isp_video_handle_buffer_starvation(struct isp_video *video)
+{
+	int err = 0;
+	struct isp_video_queue *queue = video->queue;
+	struct isp_video_buffer *buf;
+	struct list_head *head = &video->dmaqueue;
+
+	if (list_empty(&video->dmaqueue)) {
+		err = isp_video_deq_enq(queue);
+	} else if (head->next->next == head) {
+		/* only one buffer is left on dmaqueue */
+		buf = list_first_entry(&video->dmaqueue,
+					struct isp_video_buffer,
+					irqlist);
+		if (buf->state == ISP_BUF_STATE_ACTIVE)
+			err = isp_video_deq_enq(queue);
+	}
+	return err;
+}
+#endif
+
 /* -----------------------------------------------------------------------------
  * V4L2 ioctls
  */
diff --git a/drivers/media/video/omap3isp/ispvideo.h b/drivers/media/video/omap3isp/ispvideo.h
index 6f203aa..cf209f3 100644
--- a/drivers/media/video/omap3isp/ispvideo.h
+++ b/drivers/media/video/omap3isp/ispvideo.h
@@ -84,6 +84,8 @@  enum isp_pipeline_state {
 	ISP_PIPELINE_IDLE_OUTPUT = 32,
 	/* The pipeline is currently streaming. */
 	ISP_PIPELINE_STREAM = 64,
+	/* The pipeline is currently preparing to suspend. */
+	ISP_PIPELINE_PREPARE_SUSPEND = 128,
 };
 
 struct isp_pipeline {