Hi Hans,
On Friday 22 November 2013 09:43:23 Hans Verkuil wrote:
> On 11/21/2013 08:09 PM, Laurent Pinchart wrote:
> > On Thursday 21 November 2013 16:22:02 Hans Verkuil wrote:
> >> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> >>
> >> If start_streaming returns -ENODATA, then it will be retried the next
> >> time a buffer is queued. This means applications no longer need to know
> >> how many buffers need to be queued before STREAMON can be called. This is
> >> particularly useful for output stream I/O.
> >>
> >> If a DMA engine needs at least X buffers before it can start streaming,
> >> then for applications to get a buffer out as soon as possible they need
> >> to know the minimum number of buffers to queue before STREAMON can be
> >> called. You can't just try STREAMON after every buffer since on failure
> >> STREAMON will dequeue all your buffers. (Is that a bug or a feature?
> >> Frankly, I'm not sure).
> >>
> >> This patch simplifies applications substantially: they can just call
> >> STREAMON at the beginning and then start queuing buffers and the DMA
> >> engine will kick in automagically once enough buffers are available.
> >>
> >> This also fixes using write() to stream video: the fileio implementation
> >> calls streamon without having any queued buffers, which will fail today
> >> for any driver that requires a minimum number of buffers.
> >>
> >> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> >> ---
> >>
> >> drivers/media/v4l2-core/videobuf2-core.c | 66 +++++++++++++++++++-------
> >> include/media/videobuf2-core.h | 15 ++++++--
> >> 2 files changed, 64 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> >> b/drivers/media/v4l2-core/videobuf2-core.c index 9ea3ae9..5bb91f7 100644
> >> --- a/drivers/media/v4l2-core/videobuf2-core.c
> >> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> >> @@ -1332,6 +1332,39 @@ int vb2_prepare_buf(struct vb2_queue *q, struct
> >> v4l2_buffer *b) }
> >>
> >> EXPORT_SYMBOL_GPL(vb2_prepare_buf);
> >>
> >> +/**
> >> + * vb2_start_streaming() - Attempt to start streaming.
> >> + * @q: videobuf2 queue
> >> + *
> >> + * If there are not enough buffers, then retry_start_streaming is set to
> >> + * true and 0 is returned. The next time a buffer is queued and
> >> + * retry_start_streaming is true, this function will be called again to
> >> + * retry starting the DMA engine.
> >> + */
> >> +static int vb2_start_streaming(struct vb2_queue *q)
> >> +{
> >> + int ret;
> >> +
> >> + /* Tell the driver to start streaming */
> >> + ret = call_qop(q, start_streaming, q, atomic_read(&q->queued_count));
> >> +
> >> + /*
> >> + * If there are not enough buffers queued to start streaming, then
> >> + * the start_streaming operation will return -ENODATA and you have to
> >> + * retry when the next buffer is queued.
> >> + */
> >> + if (ret == -ENODATA) {
> >> + dprintk(1, "qbuf: not enough buffers, retry when more buffers are
> >> queued.\n");
> >> + q->retry_start_streaming = true;
> >> + return 0;
> >> + }
> >> + if (ret)
> >> + dprintk(1, "qbuf: driver refused to start streaming\n");
> >> + else
> >> + q->retry_start_streaming = false;
> >> + return ret;
> >> +}
> >> +
> >> static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> >> {
> >> int ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
> >> @@ -1377,6 +1410,12 @@ static int vb2_internal_qbuf(struct vb2_queue *q,
> >> struct v4l2_buffer *b) /* Fill buffer information for the userspace */
> >>
> >> __fill_v4l2_buffer(vb, b);
> >>
> >> + if (q->retry_start_streaming) {
> >> + ret = vb2_start_streaming(q);
> >> + if (ret)
> >> + return ret;
> >> + }
> >> +
> >> dprintk(1, "%s() of buffer %d succeeded\n", __func__, vb-
> >> v4l2_buf.index);
> >> return 0;
> >> }
> >>
> >> @@ -1526,7 +1565,8 @@ int vb2_wait_for_all_buffers(struct vb2_queue *q)
> >> return -EINVAL;
> >> }
> >>
> >> - wait_event(q->done_wq, !atomic_read(&q->queued_count));
> >> + if (!q->retry_start_streaming)
> >> + wait_event(q->done_wq, !atomic_read(&q->queued_count));
> >> return 0;
> >>
> >> }
> >> EXPORT_SYMBOL_GPL(vb2_wait_for_all_buffers);
> >> @@ -1640,6 +1680,9 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
> >> {
> >> unsigned int i;
> >>
> >> + if (q->retry_start_streaming)
> >> + q->retry_start_streaming = q->streaming = 0;
> >> +
> >> /*
> >> * Tell driver to stop all transactions and release all queued
> >> * buffers.
> >> @@ -1689,12 +1732,9 @@ static int vb2_internal_streamon(struct vb2_queue
> >> *q, enum v4l2_buf_type type) list_for_each_entry(vb, &q->queued_list,
> >> queued_entry)
> >>
> >> __enqueue_in_driver(vb);
> >>
> >> - /*
> >> - * Let driver notice that streaming state has been enabled.
> >> - */
> >> - ret = call_qop(q, start_streaming, q, atomic_read(&q->queued_count));
> >> + /* Tell driver to start streaming. */
> >
> > Wouldn't it be better to reset q->retry_start_streaming to 0 here instead
> > of in the several other locations ?
>
> I don't follow. retry_start_streaming is set only in vb2_start_streaming or
> in queue_cancel. I'm not sure what vb2_internal_streamon has to do with
> this.
My point is that the code would be simpler and less error-prone if we reset
retry_start_streaming in a single location right before starting the stream
instead of in multiple locations when stopping the stream. There would be less
chances to introduce a bug when refactoring code in the future in that case.
> >> + ret = vb2_start_streaming(q);
> >> if (ret) {
> >> - dprintk(1, "streamon: driver refused to start streaming\n");
> >> __vb2_queue_cancel(q);
> >> return ret;
> >> }
> >> @@ -2264,15 +2304,15 @@ static int __vb2_init_fileio(struct vb2_queue *q,
> >> int read) goto err_reqbufs;
> >>
> >> fileio->bufs[i].queued = 1;
> >> }
> >> -
> >> - /*
> >> - * Start streaming.
> >> - */
> >> - ret = vb2_streamon(q, q->type);
> >> - if (ret)
> >> - goto err_reqbufs;
> >> }
> >>
> >> + /*
> >> + * Start streaming.
> >> + */
> >> + ret = vb2_streamon(q, q->type);
> >> + if (ret)
> >> + goto err_reqbufs;
> >> +
> >> q->fileio = fileio;
> >>
> >> return ret;
--
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
