* [PATCH for v5.14] videobuf2-core: dequeue if start_streaming fails
@ 2021-07-29 8:35 Hans Verkuil
2021-07-29 14:26 ` Laurent Pinchart
0 siblings, 1 reply; 5+ messages in thread
From: Hans Verkuil @ 2021-07-29 8:35 UTC (permalink / raw)
To: Linux Media Mailing List; +Cc: Laurent Pinchart, Kieran Bingham
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
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")
---
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);
--
2.30.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH for v5.14] videobuf2-core: dequeue if start_streaming fails
2021-07-29 8:35 [PATCH for v5.14] videobuf2-core: dequeue if start_streaming fails Hans Verkuil
@ 2021-07-29 14:26 ` Laurent Pinchart
2021-07-30 8:39 ` Hans Verkuil
0 siblings, 1 reply; 5+ messages in thread
From: Laurent Pinchart @ 2021-07-29 14:26 UTC (permalink / raw)
To: Hans Verkuil; +Cc: Linux Media Mailing List, Kieran Bingham
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);
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH for v5.14] videobuf2-core: dequeue if start_streaming fails
2021-07-29 14:26 ` Laurent Pinchart
@ 2021-07-30 8:39 ` Hans Verkuil
2021-09-03 17:11 ` Laurent Pinchart
0 siblings, 1 reply; 5+ messages in thread
From: Hans Verkuil @ 2021-07-30 8:39 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Linux Media Mailing List, Kieran Bingham
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);
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH for v5.14] videobuf2-core: dequeue if start_streaming fails
2021-07-30 8:39 ` Hans Verkuil
@ 2021-09-03 17:11 ` Laurent Pinchart
2021-09-03 19:00 ` Laurent Pinchart
0 siblings, 1 reply; 5+ messages in thread
From: Laurent Pinchart @ 2021-09-03 17:11 UTC (permalink / raw)
To: Hans Verkuil; +Cc: Linux Media Mailing List, Kieran Bingham, Jacopo Mondi
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH for v5.14] videobuf2-core: dequeue if start_streaming fails
2021-09-03 17:11 ` Laurent Pinchart
@ 2021-09-03 19:00 ` Laurent Pinchart
0 siblings, 0 replies; 5+ messages in thread
From: Laurent Pinchart @ 2021-09-03 19:00 UTC (permalink / raw)
To: Hans Verkuil; +Cc: Linux Media Mailing List, Kieran Bingham, Jacopo Mondi
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-09-03 19:00 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-29 8:35 [PATCH for v5.14] videobuf2-core: dequeue if start_streaming fails Hans Verkuil
2021-07-29 14:26 ` Laurent Pinchart
2021-07-30 8:39 ` Hans Verkuil
2021-09-03 17:11 ` Laurent Pinchart
2021-09-03 19:00 ` Laurent Pinchart
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.