Hi Hans,
On Fri, Jul 30, 2021 at 10:39:59AM +0200, Hans Verkuil wrote:
> On 29/07/2021 16:26, Laurent Pinchart wrote:
> > 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,
>
> This is the new commit message (not going to repost the patch, I'll just
> update the PR):
Did you forget to include it in a pull request by any chance ? I don't
see this in v5.14. Any chance it could go in v5.15 as a fix ?
> If a vb2_queue sets q->min_buffers_needed then when the number of
> queued buffers reaches q->min_buffers_needed, vb2_core_qbuf() will call
> the start_streaming() callback. If start_streaming() returns an error,
> then that error was just returned by vb2_core_qbuf(), but the buffer
> was still queued. However, userspace expects that if VIDIOC_QBUF fails,
> the buffer is returned dequeued.
>
> So if start_streaming() fails, then remove the buffer from the queue,
> thus avoiding this unwanted side-effect.
>
> > 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
