Hi Hans,
Thank you for the patch.
On Thu, Jul 29, 2021 at 10:35:33AM +0200, Hans Verkuil wrote:
> If a vb2_queue sets q->min_buffers_needed then if the number of
> queued buffers reaches that number vb2_core_qbuf() will call
> the start_streaming() callback. If that returns an error, then that
> was just returned, but that left the buffer still queued. But userspace
The three "that" in the sentence are confusing. Do you mean "If that
function returns an error, the error code is just returned, but the
buffer is left still queued." ?
> expects that if VIDIOC_QBUF fails, the buffer wasn't queued.
>
> So if start_streaming() fails, then remove the buffer from the queue,
> thus avoiding this unwanted side-effect.
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> Tested-by: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
> Fixes: b3379c6201bb ("[media] vb2: only call start_streaming if sufficient buffers are queued")
Possibly with the commit message updated,
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
> drivers/media/common/videobuf2/videobuf2-core.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 02281d13505f..508ac295eb06 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -1573,6 +1573,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
> struct media_request *req)
> {
> struct vb2_buffer *vb;
> + enum vb2_buffer_state orig_state;
> int ret;
>
> if (q->error) {
> @@ -1673,6 +1674,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
> * Add to the queued buffers list, a buffer will stay on it until
> * dequeued in dqbuf.
> */
> + orig_state = vb->state;
> list_add_tail(&vb->queued_entry, &q->queued_list);
> q->queued_count++;
> q->waiting_for_buffers = false;
> @@ -1703,8 +1705,17 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
> if (q->streaming && !q->start_streaming_called &&
> q->queued_count >= q->min_buffers_needed) {
> ret = vb2_start_streaming(q);
> - if (ret)
> + if (ret) {
> + /*
> + * Since vb2_core_qbuf will return with an error,
> + * we should return it to state DEQUEUED since
> + * the error indicates that the buffer wasn't queued.
> + */
> + list_del(&vb->queued_entry);
> + q->queued_count--;
> + vb->state = orig_state;
> return ret;
> + }
> }
>
> dprintk(q, 2, "qbuf of buffer %d succeeded\n", vb->index);
--
Regards,
Laurent Pinchart
