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@xxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
