[PATCH 0/3] vb2 and omap3isp driver fixes

[PATCH 0/3] vb2 and omap3isp driver fixes

[Sakari Ailus]

Hi Hans, Laurent and others,

This small set fixes a videobuf2 issue related to returning queued buffers
back to the driver. I found it after hopefully fixing a related issue (two
later patches) in the omap3isp driver.

The patchset has been tested up to streamon, but no buffers have been
successfully dequeued. That's exactly the remaining unresolved technical
problem from the N9 DT camera support patchset: I get no ISP interrupts at
all.

-- 
Kind regards,
Sakari

[PATCH 1/3] vb2: Buffers returned to videobuf2 from start_streaming in QUEUED state

[Sakari Ailus]

Patch "[media] v4l: vb2: Fix stream start and buffer completion race" has a
sets q->start_streaming_called before calling queue op start_streaming() in
order to fix a bug. This has the side effect that buffers returned to
videobuf2 in VB2_BUF_STATE_QUEUED will cause a WARN_ON() to be called.

Add a new field called done_buffers_queued_state to struct vb2_queue, which
must be set if the new state of buffers returned to videobuf2 must be
VB2_BUF_STATE_QUEUED, i.e. buffers returned in start_streaming op.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
Cc: Hans Verkuil <hverkuil@xs4all.nl>
---
 drivers/media/v4l2-core/videobuf2-core.c |    5 +++--
 include/media/videobuf2-core.h           |    4 ++++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 7e6aff6..202e2a5 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1174,7 +1174,7 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
 	if (WARN_ON(vb->state != VB2_BUF_STATE_ACTIVE))
 		return;
 
-	if (!q->start_streaming_called) {
+	if (q->done_buffers_queued_state) {
 		if (WARN_ON(state != VB2_BUF_STATE_QUEUED))
 			state = VB2_BUF_STATE_QUEUED;
 	} else if (WARN_ON(state != VB2_BUF_STATE_DONE &&
@@ -1742,9 +1742,10 @@ static int vb2_start_streaming(struct vb2_queue *q)
 		__enqueue_in_driver(vb);
 
 	/* Tell the driver to start streaming */
-	q->start_streaming_called = 1;
+	q->done_buffers_queued_state = q->start_streaming_called = 1;
 	ret = call_qop(q, start_streaming, q,
 		       atomic_read(&q->owned_by_drv_count));
+	q->done_buffers_queued_state = 0;
 	if (!ret)
 		return 0;
 
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 5a10d8d..7c0dac6 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -380,6 +380,9 @@ struct v4l2_fh;
  * @streaming:	current streaming state
  * @start_streaming_called: start_streaming() was called successfully and we
  *		started streaming.
+ * @done_buffers_queued_state: buffers returned to videobuf2 must go
+ *		to VB2_BUF_STATE_QUEUED state. This is the case whilst
+ *		the driver's start_streaming op is called.
  * @error:	a fatal error occurred on the queue
  * @fileio:	file io emulator internal data, used only if emulator is active
  * @threadio:	thread io internal data, used only if thread is active
@@ -418,6 +421,7 @@ struct vb2_queue {
 
 	unsigned int			streaming:1;
 	unsigned int			start_streaming_called:1;
+	unsigned int			done_buffers_queued_state:1;
 	unsigned int			error:1;
 
 	struct vb2_fileio_data		*fileio;
-- 
1.7.10.4

Re: [PATCH 1/3] vb2: Buffers returned to videobuf2 from start_streaming in QUEUED state

[Sakari Ailus]

On Fri, Sep 19, 2014 at 12:57:47AM +0300, Sakari Ailus wrote:
> @@ -1174,7 +1174,7 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
>  	if (WARN_ON(vb->state != VB2_BUF_STATE_ACTIVE))
>  		return;
>  
> -	if (!q->start_streaming_called) {
> +	if (q->done_buffers_queued_state) {
>  		if (WARN_ON(state != VB2_BUF_STATE_QUEUED))
>  			state = VB2_BUF_STATE_QUEUED;
>  	} else if (WARN_ON(state != VB2_BUF_STATE_DONE &&

This condition needs to be changed, too. I'll resend a corrected version.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 1/3] vb2: Buffers returned to videobuf2 from start_streaming in QUEUED state

[Hans Verkuil]

On 09/18/2014 11:57 PM, Sakari Ailus wrote:
> Patch "[media] v4l: vb2: Fix stream start and buffer completion race" has a
> sets q->start_streaming_called before calling queue op start_streaming() in
> order to fix a bug. This has the side effect that buffers returned to
> videobuf2 in VB2_BUF_STATE_QUEUED will cause a WARN_ON() to be called.
> 
> Add a new field called done_buffers_queued_state to struct vb2_queue, which
> must be set if the new state of buffers returned to videobuf2 must be
> VB2_BUF_STATE_QUEUED, i.e. buffers returned in start_streaming op.

I posted a fix for this over a month ago:

https://www.mail-archive.com/linux-media@vger.kernel.org/msg77871.html

Unfortunately, the pull request with that patch (https://patchwork.linuxtv.org/patch/25162/)
fell through the cracks as I discovered yesterday. Hopefully Mauro will pick
up that pull request quickly.

I prefer my patch since that avoids introducing yet another state variable.

Regards,

	Hans

> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> Cc: Hans Verkuil <hverkuil@xs4all.nl>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c |    5 +++--
>  include/media/videobuf2-core.h           |    4 ++++
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 7e6aff6..202e2a5 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1174,7 +1174,7 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
>  	if (WARN_ON(vb->state != VB2_BUF_STATE_ACTIVE))
>  		return;
>  
> -	if (!q->start_streaming_called) {
> +	if (q->done_buffers_queued_state) {
>  		if (WARN_ON(state != VB2_BUF_STATE_QUEUED))
>  			state = VB2_BUF_STATE_QUEUED;
>  	} else if (WARN_ON(state != VB2_BUF_STATE_DONE &&
> @@ -1742,9 +1742,10 @@ static int vb2_start_streaming(struct vb2_queue *q)
>  		__enqueue_in_driver(vb);
>  
>  	/* Tell the driver to start streaming */
> -	q->start_streaming_called = 1;
> +	q->done_buffers_queued_state = q->start_streaming_called = 1;
>  	ret = call_qop(q, start_streaming, q,
>  		       atomic_read(&q->owned_by_drv_count));
> +	q->done_buffers_queued_state = 0;
>  	if (!ret)
>  		return 0;
>  
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 5a10d8d..7c0dac6 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -380,6 +380,9 @@ struct v4l2_fh;
>   * @streaming:	current streaming state
>   * @start_streaming_called: start_streaming() was called successfully and we
>   *		started streaming.
> + * @done_buffers_queued_state: buffers returned to videobuf2 must go
> + *		to VB2_BUF_STATE_QUEUED state. This is the case whilst
> + *		the driver's start_streaming op is called.
>   * @error:	a fatal error occurred on the queue
>   * @fileio:	file io emulator internal data, used only if emulator is active
>   * @threadio:	thread io internal data, used only if thread is active
> @@ -418,6 +421,7 @@ struct vb2_queue {
>  
>  	unsigned int			streaming:1;
>  	unsigned int			start_streaming_called:1;
> +	unsigned int			done_buffers_queued_state:1;
>  	unsigned int			error:1;
>  
>  	struct vb2_fileio_data		*fileio;
> 

Re: [PATCH 1/3] vb2: Buffers returned to videobuf2 from start_streaming in QUEUED state

[Sakari Ailus]

Hi Hans,

On Fri, Sep 19, 2014 at 10:13:57AM +0200, Hans Verkuil wrote:
> On 09/18/2014 11:57 PM, Sakari Ailus wrote:
> > Patch "[media] v4l: vb2: Fix stream start and buffer completion race" has a
> > sets q->start_streaming_called before calling queue op start_streaming() in
> > order to fix a bug. This has the side effect that buffers returned to
> > videobuf2 in VB2_BUF_STATE_QUEUED will cause a WARN_ON() to be called.
> > 
> > Add a new field called done_buffers_queued_state to struct vb2_queue, which
> > must be set if the new state of buffers returned to videobuf2 must be
> > VB2_BUF_STATE_QUEUED, i.e. buffers returned in start_streaming op.
> 
> I posted a fix for this over a month ago:
> 
> https://www.mail-archive.com/linux-media@vger.kernel.org/msg77871.html
> 
> Unfortunately, the pull request with that patch (https://patchwork.linuxtv.org/patch/25162/)
> fell through the cracks as I discovered yesterday. Hopefully Mauro will pick
> up that pull request quickly.
> 
> I prefer my patch since that avoids introducing yet another state variable.

Using less state variables is good, but with your patch returning buffers
back to videobuf2 to state VB2_BUF_STATE_QUEUED is possible also after
start_stream() has finished. That's probably a lesser problem, though.

I'm fine with your patch as well.

-- 
Cheers,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[PATCH 2/3] omap3isp: Move starting the sensor from streamon IOCTL handler to VB2 QOP

[Sakari Ailus]

Move the starting of the sensor from the VIDIOC_STREAMON handler to the
videobuf2 queue op start_streaming. This avoids failing starting the stream
after vb2_streamon() has already finished.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/media/platform/omap3isp/ispvideo.c |   49 +++++++++++++++++-----------
 1 file changed, 30 insertions(+), 19 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index bc38c88..b233c8e 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -425,10 +425,40 @@ static void isp_video_buffer_queue(struct vb2_buffer *buf)
 	}
 }
 
+static int isp_video_start_streaming(struct vb2_queue *queue,
+				     unsigned int count)
+{
+	struct isp_video_fh *vfh = vb2_get_drv_priv(queue);
+	struct isp_video *video = vfh->video;
+	struct isp_pipeline *pipe = to_isp_pipeline(&video->video.entity);
+	unsigned long flags;
+	int ret;
+
+	/* In sensor-to-memory mode, the stream can be started synchronously
+	 * to the stream on command. In memory-to-memory mode, it will be
+	 * started when buffers are queued on both the input and output.
+	 */
+	if (pipe->input)
+		return 0;
+
+	ret = omap3isp_pipeline_set_stream(pipe,
+					   ISP_PIPELINE_STREAM_CONTINUOUS);
+	if (ret < 0)
+		return ret;
+
+	spin_lock_irqsave(&video->irqlock, flags);
+	if (list_empty(&video->dmaqueue))
+		video->dmaqueue_flags |= ISP_VIDEO_DMAQUEUE_UNDERRUN;
+	spin_unlock_irqrestore(&video->irqlock, flags);
+
+	return 0;
+}
+
 static const struct vb2_ops isp_video_queue_ops = {
 	.queue_setup = isp_video_queue_setup,
 	.buf_prepare = isp_video_buffer_prepare,
 	.buf_queue = isp_video_buffer_queue,
+	.start_streaming = isp_video_start_streaming,
 };
 
 /*
@@ -1077,28 +1107,9 @@ isp_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 	if (ret < 0)
 		goto err_check_format;
 
-	/* In sensor-to-memory mode, the stream can be started synchronously
-	 * to the stream on command. In memory-to-memory mode, it will be
-	 * started when buffers are queued on both the input and output.
-	 */
-	if (pipe->input == NULL) {
-		ret = omap3isp_pipeline_set_stream(pipe,
-					      ISP_PIPELINE_STREAM_CONTINUOUS);
-		if (ret < 0)
-			goto err_set_stream;
-		spin_lock_irqsave(&video->irqlock, flags);
-		if (list_empty(&video->dmaqueue))
-			video->dmaqueue_flags |= ISP_VIDEO_DMAQUEUE_UNDERRUN;
-		spin_unlock_irqrestore(&video->irqlock, flags);
-	}
-
 	mutex_unlock(&video->stream_lock);
 	return 0;
 
-err_set_stream:
-	mutex_lock(&video->queue_lock);
-	vb2_streamoff(&vfh->queue, type);
-	mutex_unlock(&video->queue_lock);
 err_check_format:
 	media_entity_pipeline_stop(&video->video.entity);
 err_pipeline_start:
-- 
1.7.10.4

Re: [PATCH 2/3] omap3isp: Move starting the sensor from streamon IOCTL handler to VB2 QOP

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Friday 19 September 2014 00:57:48 Sakari Ailus wrote:
> Move the starting of the sensor from the VIDIOC_STREAMON handler to the
> videobuf2 queue op start_streaming. This avoids failing starting the stream
> after vb2_streamon() has already finished.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

and applied to my tree.

> ---
>  drivers/media/platform/omap3isp/ispvideo.c |   49 +++++++++++++++---------
>  1 file changed, 30 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/ispvideo.c
> b/drivers/media/platform/omap3isp/ispvideo.c index bc38c88..b233c8e 100644
> --- a/drivers/media/platform/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/omap3isp/ispvideo.c
> @@ -425,10 +425,40 @@ static void isp_video_buffer_queue(struct vb2_buffer
> *buf) }
>  }
> 
> +static int isp_video_start_streaming(struct vb2_queue *queue,
> +				     unsigned int count)
> +{
> +	struct isp_video_fh *vfh = vb2_get_drv_priv(queue);
> +	struct isp_video *video = vfh->video;
> +	struct isp_pipeline *pipe = to_isp_pipeline(&video->video.entity);
> +	unsigned long flags;
> +	int ret;
> +
> +	/* In sensor-to-memory mode, the stream can be started synchronously
> +	 * to the stream on command. In memory-to-memory mode, it will be
> +	 * started when buffers are queued on both the input and output.
> +	 */
> +	if (pipe->input)
> +		return 0;
> +
> +	ret = omap3isp_pipeline_set_stream(pipe,
> +					   ISP_PIPELINE_STREAM_CONTINUOUS);
> +	if (ret < 0)
> +		return ret;
> +
> +	spin_lock_irqsave(&video->irqlock, flags);
> +	if (list_empty(&video->dmaqueue))
> +		video->dmaqueue_flags |= ISP_VIDEO_DMAQUEUE_UNDERRUN;
> +	spin_unlock_irqrestore(&video->irqlock, flags);
> +
> +	return 0;
> +}
> +
>  static const struct vb2_ops isp_video_queue_ops = {
>  	.queue_setup = isp_video_queue_setup,
>  	.buf_prepare = isp_video_buffer_prepare,
>  	.buf_queue = isp_video_buffer_queue,
> +	.start_streaming = isp_video_start_streaming,
>  };
> 
>  /*
> @@ -1077,28 +1107,9 @@ isp_video_streamon(struct file *file, void *fh, enum
> v4l2_buf_type type) if (ret < 0)
>  		goto err_check_format;
> 
> -	/* In sensor-to-memory mode, the stream can be started synchronously
> -	 * to the stream on command. In memory-to-memory mode, it will be
> -	 * started when buffers are queued on both the input and output.
> -	 */
> -	if (pipe->input == NULL) {
> -		ret = omap3isp_pipeline_set_stream(pipe,
> -					      ISP_PIPELINE_STREAM_CONTINUOUS);
> -		if (ret < 0)
> -			goto err_set_stream;
> -		spin_lock_irqsave(&video->irqlock, flags);
> -		if (list_empty(&video->dmaqueue))
> -			video->dmaqueue_flags |= ISP_VIDEO_DMAQUEUE_UNDERRUN;
> -		spin_unlock_irqrestore(&video->irqlock, flags);
> -	}
> -
>  	mutex_unlock(&video->stream_lock);
>  	return 0;
> 
> -err_set_stream:
> -	mutex_lock(&video->queue_lock);
> -	vb2_streamoff(&vfh->queue, type);
> -	mutex_unlock(&video->queue_lock);
>  err_check_format:
>  	media_entity_pipeline_stop(&video->video.entity);
>  err_pipeline_start:

-- 
Regards,

Laurent Pinchart

[PATCH 3/3] omap3isp: Return buffers back to videobuf2 if pipeline streamon fails

[Sakari Ailus]

When the video buffer queue was stopped before the stream source was started
in omap3isp_streamon(), the buffers were not returned back to videobuf2.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/media/platform/omap3isp/isp.c      |    4 ++--
 drivers/media/platform/omap3isp/ispvideo.c |   16 ++++++++++------
 drivers/media/platform/omap3isp/ispvideo.h |    3 ++-
 3 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 72265e5..2aa0a8e 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -1062,9 +1062,9 @@ int omap3isp_pipeline_set_stream(struct isp_pipeline *pipe,
 void omap3isp_pipeline_cancel_stream(struct isp_pipeline *pipe)
 {
 	if (pipe->input)
-		omap3isp_video_cancel_stream(pipe->input);
+		omap3isp_video_cancel_stream(pipe->input, VB2_BUF_STATE_ERROR);
 	if (pipe->output)
-		omap3isp_video_cancel_stream(pipe->output);
+		omap3isp_video_cancel_stream(pipe->output, VB2_BUF_STATE_ERROR);
 }
 
 /*
diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index b233c8e..73c0194 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -443,8 +443,10 @@ static int isp_video_start_streaming(struct vb2_queue *queue,
 
 	ret = omap3isp_pipeline_set_stream(pipe,
 					   ISP_PIPELINE_STREAM_CONTINUOUS);
-	if (ret < 0)
+	if (ret < 0) {
+		omap3isp_video_cancel_stream(video, VB2_BUF_STATE_QUEUED);
 		return ret;
+	}
 
 	spin_lock_irqsave(&video->irqlock, flags);
 	if (list_empty(&video->dmaqueue))
@@ -566,10 +568,12 @@ struct isp_buffer *omap3isp_video_buffer_next(struct isp_video *video)
  * omap3isp_video_cancel_stream - Cancel stream on a video node
  * @video: ISP video object
  *
- * Cancelling a stream mark all buffers on the video node as erroneous and makes
- * sure no new buffer can be queued.
+ * Cancelling a stream mark all buffers on the video node as erroneous
+ * and makes sure no new buffer can be queued. Buffers are returned
+ * back to videobuf2 in the given state.
  */
-void omap3isp_video_cancel_stream(struct isp_video *video)
+void omap3isp_video_cancel_stream(struct isp_video *video,
+				  enum vb2_buffer_state state)
 {
 	unsigned long flags;
 
@@ -581,7 +585,7 @@ void omap3isp_video_cancel_stream(struct isp_video *video)
 		buf = list_first_entry(&video->dmaqueue,
 				       struct isp_buffer, irqlist);
 		list_del(&buf->irqlist);
-		vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
+		vb2_buffer_done(&buf->vb, state);
 	}
 
 	video->error = true;
@@ -1166,7 +1170,7 @@ isp_video_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
 
 	/* Stop the stream. */
 	omap3isp_pipeline_set_stream(pipe, ISP_PIPELINE_STREAM_STOPPED);
-	omap3isp_video_cancel_stream(video);
+	omap3isp_video_cancel_stream(video, VB2_BUF_STATE_ERROR);
 
 	mutex_lock(&video->queue_lock);
 	vb2_streamoff(&vfh->queue, type);
diff --git a/drivers/media/platform/omap3isp/ispvideo.h b/drivers/media/platform/omap3isp/ispvideo.h
index 0b7efed..7e4732a 100644
--- a/drivers/media/platform/omap3isp/ispvideo.h
+++ b/drivers/media/platform/omap3isp/ispvideo.h
@@ -201,7 +201,8 @@ int omap3isp_video_register(struct isp_video *video,
 			    struct v4l2_device *vdev);
 void omap3isp_video_unregister(struct isp_video *video);
 struct isp_buffer *omap3isp_video_buffer_next(struct isp_video *video);
-void omap3isp_video_cancel_stream(struct isp_video *video);
+void omap3isp_video_cancel_stream(struct isp_video *video,
+				  enum vb2_buffer_state state);
 void omap3isp_video_resume(struct isp_video *video, int continuous);
 struct media_pad *omap3isp_video_remote_pad(struct isp_video *video);
 
-- 
1.7.10.4

Re: [PATCH 3/3] omap3isp: Return buffers back to videobuf2 if pipeline streamon fails

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Friday 19 September 2014 00:57:49 Sakari Ailus wrote:
> When the video buffer queue was stopped before the stream source was started
> in omap3isp_streamon(), the buffers were not returned back to videobuf2.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  drivers/media/platform/omap3isp/isp.c      |    4 ++--
>  drivers/media/platform/omap3isp/ispvideo.c |   16 ++++++++++------
>  drivers/media/platform/omap3isp/ispvideo.h |    3 ++-
>  3 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index 72265e5..2aa0a8e 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -1062,9 +1062,9 @@ int omap3isp_pipeline_set_stream(struct isp_pipeline
> *pipe, void omap3isp_pipeline_cancel_stream(struct isp_pipeline *pipe)
>  {
>  	if (pipe->input)
> -		omap3isp_video_cancel_stream(pipe->input);
> +		omap3isp_video_cancel_stream(pipe->input, VB2_BUF_STATE_ERROR);
>  	if (pipe->output)
> -		omap3isp_video_cancel_stream(pipe->output);
> +		omap3isp_video_cancel_stream(pipe->output, VB2_BUF_STATE_ERROR);
>  }
> 
>  /*
> diff --git a/drivers/media/platform/omap3isp/ispvideo.c
> b/drivers/media/platform/omap3isp/ispvideo.c index b233c8e..73c0194 100644
> --- a/drivers/media/platform/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/omap3isp/ispvideo.c
> @@ -443,8 +443,10 @@ static int isp_video_start_streaming(struct vb2_queue
> *queue,
> 
>  	ret = omap3isp_pipeline_set_stream(pipe,
>  					   ISP_PIPELINE_STREAM_CONTINUOUS);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		omap3isp_video_cancel_stream(video, VB2_BUF_STATE_QUEUED);

This will set video->error to true. As the flag is only reset to false when 
stopping the stream in isp_video_streamoff we'll have a problem here.

One option would be to split the buffer handling part of 
omap3isp_video_cancel_stream() to a separate function 
omap3isp_video_return_buffers() and call that one when stream start fails.

>  		return ret;
> +	}
> 
>  	spin_lock_irqsave(&video->irqlock, flags);
>  	if (list_empty(&video->dmaqueue))
> @@ -566,10 +568,12 @@ struct isp_buffer *omap3isp_video_buffer_next(struct
> isp_video *video) * omap3isp_video_cancel_stream - Cancel stream on a video
> node
>   * @video: ISP video object

You're missing documentation of the state parameter here.

>   *
> - * Cancelling a stream mark all buffers on the video node as erroneous and
> makes
> - * sure no new buffer can be queued.
> + * Cancelling a stream mark all buffers on the video node as erroneous
> + * and makes sure no new buffer can be queued. Buffers are returned
> + * back to videobuf2 in the given state.

The buffer isn't marked as erroneous when the state is set to 
VB2_BUF_STATE_QUEUED. How about

"Cancelling a stream returns all buffers queued on the video node to videobuf2
in the given state and makes sure no new buffer can be queued. The buffer
state should be VB2_BUF_STATE_QUEUED when the stream is cancelled due to an
error when starting the stream, or VB2_BUF_STATE_ERROR otherwise."

I've pushed a new version of the patch that fixes all this to

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

(http://git.linuxtv.org/cgit.cgi/pinchartl/media.git/commit/?h=omap3isp/next&id=da80a526014da7a2c07af9978aaafc47d816e103)

If you like it there's no need to resubmit.

>   */
> -void omap3isp_video_cancel_stream(struct isp_video *video)
> +void omap3isp_video_cancel_stream(struct isp_video *video,
> +				  enum vb2_buffer_state state)
>  {
>  	unsigned long flags;
> 
> @@ -581,7 +585,7 @@ void omap3isp_video_cancel_stream(struct isp_video
> *video) buf = list_first_entry(&video->dmaqueue,
>  				       struct isp_buffer, irqlist);
>  		list_del(&buf->irqlist);
> -		vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
> +		vb2_buffer_done(&buf->vb, state);
>  	}
> 
>  	video->error = true;
> @@ -1166,7 +1170,7 @@ isp_video_streamoff(struct file *file, void *fh, enum
> v4l2_buf_type type)
> 
>  	/* Stop the stream. */
>  	omap3isp_pipeline_set_stream(pipe, ISP_PIPELINE_STREAM_STOPPED);
> -	omap3isp_video_cancel_stream(video);
> +	omap3isp_video_cancel_stream(video, VB2_BUF_STATE_ERROR);
> 
>  	mutex_lock(&video->queue_lock);
>  	vb2_streamoff(&vfh->queue, type);
> diff --git a/drivers/media/platform/omap3isp/ispvideo.h
> b/drivers/media/platform/omap3isp/ispvideo.h index 0b7efed..7e4732a 100644
> --- a/drivers/media/platform/omap3isp/ispvideo.h
> +++ b/drivers/media/platform/omap3isp/ispvideo.h
> @@ -201,7 +201,8 @@ int omap3isp_video_register(struct isp_video *video,
>  			    struct v4l2_device *vdev);
>  void omap3isp_video_unregister(struct isp_video *video);
>  struct isp_buffer *omap3isp_video_buffer_next(struct isp_video *video);
> -void omap3isp_video_cancel_stream(struct isp_video *video);
> +void omap3isp_video_cancel_stream(struct isp_video *video,
> +				  enum vb2_buffer_state state);
>  void omap3isp_video_resume(struct isp_video *video, int continuous);
>  struct media_pad *omap3isp_video_remote_pad(struct isp_video *video);

-- 
Regards,

Laurent Pinchart