diff mbox series

[for,v6.2] media: videobuf2: set q->streaming later

Message ID c12cfcc5-1d46-7b5f-4a27-f4cd52a1b885@xs4all.nl (mailing list archive)
State New, archived
Headers show
Series [for,v6.2] media: videobuf2: set q->streaming later | expand

Commit Message

Hans Verkuil Jan. 23, 2023, 8:45 a.m. UTC 
Commit a10b21532574 ("media: vb2: add (un)prepare_streaming queue ops") moved
up the q->streaming = 1 assignment to before the call to vb2_start_streaming().

This does make sense since q->streaming indicates that VIDIOC_STREAMON is called,
and the call to start_streaming happens either at that time or later if
q->min_buffers_needed > 0. So q->streaming should be 1 before start_streaming
is called.

However, it turned out that some drivers use vb2_is_streaming() in buf_queue,
and if q->min_buffers_needed == 0, then that will now return true instead of
false.

So for the time being revert to the original behavior.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Fixes: a10b21532574 ("media: vb2: add (un)prepare_streaming queue ops")
---

Comments

Laurent Pinchart Jan. 23, 2023, 10:43 a.m. UTC | #1 
Hi Hans,

Thank you for the patch.

On Mon, Jan 23, 2023 at 09:45:49AM +0100, Hans Verkuil wrote:
> Commit a10b21532574 ("media: vb2: add (un)prepare_streaming queue ops") moved
> up the q->streaming = 1 assignment to before the call to vb2_start_streaming().
> 
> This does make sense since q->streaming indicates that VIDIOC_STREAMON is called,
> and the call to start_streaming happens either at that time or later if
> q->min_buffers_needed > 0. So q->streaming should be 1 before start_streaming
> is called.
> 
> However, it turned out that some drivers use vb2_is_streaming() in buf_queue,
> and if q->min_buffers_needed == 0, then that will now return true instead of
> false.
> 
> So for the time being revert to the original behavior.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Fixes: a10b21532574 ("media: vb2: add (un)prepare_streaming queue ops")

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

I'll test it shortly to see if it fixes the VSP1 breakage.

> ---
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index fc3758a5bc1c..53e495223ea0 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -2149,8 +2149,6 @@ int vb2_core_streamon(struct vb2_queue *q, unsigned int type)
>  	if (ret)
>  		return ret;
> 
> -	q->streaming = 1;
> -
>  	/*
>  	 * Tell driver to start streaming provided sufficient buffers
>  	 * are available.
> @@ -2161,12 +2159,13 @@ int vb2_core_streamon(struct vb2_queue *q, unsigned int type)
>  			goto unprepare;
>  	}
> 
> +	q->streaming = 1;
> +
>  	dprintk(q, 3, "successful\n");
>  	return 0;
> 
>  unprepare:
>  	call_void_qop(q, unprepare_streaming, q);
> -	q->streaming = 0;
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(vb2_core_streamon);
Laurent Pinchart Jan. 23, 2023, 10:23 p.m. UTC | #2 
On Mon, Jan 23, 2023 at 12:43:46PM +0200, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Mon, Jan 23, 2023 at 09:45:49AM +0100, Hans Verkuil wrote:
> > Commit a10b21532574 ("media: vb2: add (un)prepare_streaming queue ops") moved
> > up the q->streaming = 1 assignment to before the call to vb2_start_streaming().
> > 
> > This does make sense since q->streaming indicates that VIDIOC_STREAMON is called,
> > and the call to start_streaming happens either at that time or later if
> > q->min_buffers_needed > 0. So q->streaming should be 1 before start_streaming
> > is called.
> > 
> > However, it turned out that some drivers use vb2_is_streaming() in buf_queue,
> > and if q->min_buffers_needed == 0, then that will now return true instead of
> > false.
> > 
> > So for the time being revert to the original behavior.
> > 
> > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > Fixes: a10b21532574 ("media: vb2: add (un)prepare_streaming queue ops")
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> I'll test it shortly to see if it fixes the VSP1 breakage.

Tested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> > ---
> > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> > index fc3758a5bc1c..53e495223ea0 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-core.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> > @@ -2149,8 +2149,6 @@ int vb2_core_streamon(struct vb2_queue *q, unsigned int type)
> >  	if (ret)
> >  		return ret;
> > 
> > -	q->streaming = 1;
> > -
> >  	/*
> >  	 * Tell driver to start streaming provided sufficient buffers
> >  	 * are available.
> > @@ -2161,12 +2159,13 @@ int vb2_core_streamon(struct vb2_queue *q, unsigned int type)
> >  			goto unprepare;
> >  	}
> > 
> > +	q->streaming = 1;
> > +
> >  	dprintk(q, 3, "successful\n");
> >  	return 0;
> > 
> >  unprepare:
> >  	call_void_qop(q, unprepare_streaming, q);
> > -	q->streaming = 0;
> >  	return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(vb2_core_streamon);
diff mbox series

Patch

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index fc3758a5bc1c..53e495223ea0 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -2149,8 +2149,6 @@  int vb2_core_streamon(struct vb2_queue *q, unsigned int type)
 	if (ret)
 		return ret;

-	q->streaming = 1;
-
 	/*
 	 * Tell driver to start streaming provided sufficient buffers
 	 * are available.
@@ -2161,12 +2159,13 @@  int vb2_core_streamon(struct vb2_queue *q, unsigned int type)
 			goto unprepare;
 	}

+	q->streaming = 1;
+
 	dprintk(q, 3, "successful\n");
 	return 0;

 unprepare:
 	call_void_qop(q, unprepare_streaming, q);
-	q->streaming = 0;
 	return ret;
 }
 EXPORT_SYMBOL_GPL(vb2_core_streamon);