| Message ID | 542319b4-387b-6e29-db72-998de02ae4b4@xs4all.nl |
|---|---|
| State | New |
| Headers | show |
| Series | [for,v5.14] videobuf2-core: dequeue if start_streaming fails | expand |
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@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, 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);
On 29/07/2021 16:26, Laurent Pinchart wrote: > 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@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): 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. Regards, Hans > > 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); >
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
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@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 ? 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@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
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);