diff mbox series

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

Message ID 542319b4-387b-6e29-db72-998de02ae4b4@xs4all.nl (mailing list archive)
State New, archived
Headers show
Series [for,v5.14] videobuf2-core: dequeue if start_streaming fails | expand

Commit Message

Hans Verkuil July 29, 2021, 8:35 a.m. UTC 
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(-)

Comments

Laurent Pinchart July 29, 2021, 2:26 p.m. UTC | #1 
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);
Hans Verkuil July 30, 2021, 8:39 a.m. UTC | #2 
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);
>
Laurent Pinchart Sept. 3, 2021, 5:11 p.m. UTC | #3 
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);
Laurent Pinchart Sept. 3, 2021, 7 p.m. UTC | #4 
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);
diff mbox series

Patch

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);