Hi Hans,
On Fri, Sep 03, 2021 at 08:11:32PM +0300, Laurent Pinchart wrote:
> 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 ?
I wasn't looking in the right place. Sorry for the noise, the fix is in
mainline now.
> > 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
