Re: [PATCH for v5.14] videobuf2-core: dequeue if start_streaming fails

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@xs4all.nl>
> >> Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> 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@ideasonboard.com>
> > 
> >> ---
> >>  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