diff mbox

[02/12,media] vb2: split out queueing from vb_core_qbuf()

Message ID 20170616073915.5027-3-gustavo@padovan.org (mailing list archive)
State Changes Requested, archived
Headers

Commit Message

Gustavo F. Padovan June 16, 2017, 7:39 a.m. UTC 
  From: Gustavo Padovan <gustavo.padovan@collabora.com>

In order to support explicit synchronization we need to divide
vb2_core_qbuf() in two parts, one to be executed before the fence
signals and another one to do the actual queueing of the buffer.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 51 ++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 22 deletions(-)

Comments

Mauro Carvalho Chehab June 30, 2017, 11:15 a.m. UTC | #1 
Em Fri, 16 Jun 2017 16:39:05 +0900
Gustavo Padovan <gustavo@padovan.org> escreveu:

> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> 
> In order to support explicit synchronization we need to divide
> vb2_core_qbuf() in two parts, one to be executed before the fence
> signals and another one to do the actual queueing of the buffer.

Looks good to me.


Acked-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>

> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 51 ++++++++++++++++++--------------
>  1 file changed, 29 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 3107e21..ea83126 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1367,6 +1367,34 @@ static int vb2_start_streaming(struct vb2_queue *q)
>  	return ret;
>  }
>  
> +static int __vb2_core_qbuf(struct vb2_buffer *vb, struct vb2_queue *q)
> +{
> +	int ret;
> +
> +	/*
> +	 * If already streaming, give the buffer to driver for processing.
> +	 * If not, the buffer will be given to driver on next streamon.
> +	 */
> +	if (q->start_streaming_called)
> +		__enqueue_in_driver(vb);
> +
> +	/*
> +	 * If streamon has been called, and we haven't yet called
> +	 * start_streaming() since not enough buffers were queued, and
> +	 * we now have reached the minimum number of queued buffers,
> +	 * then we can finally call start_streaming().
> +	 */
> +	if (q->streaming && !q->start_streaming_called &&
> +	    q->queued_count >= q->min_buffers_needed) {
> +		ret = vb2_start_streaming(q);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	dprintk(1, "qbuf of buffer %d succeeded\n", vb->index);
> +	return 0;
> +}
> +
>  int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
>  {
>  	struct vb2_buffer *vb;
> @@ -1404,32 +1432,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
>  
>  	trace_vb2_qbuf(q, vb);
>  
> -	/*
> -	 * If already streaming, give the buffer to driver for processing.
> -	 * If not, the buffer will be given to driver on next streamon.
> -	 */
> -	if (q->start_streaming_called)
> -		__enqueue_in_driver(vb);
> -
>  	/* Fill buffer information for the userspace */
>  	if (pb)
>  		call_void_bufop(q, fill_user_buffer, vb, pb);
>  
> -	/*
> -	 * If streamon has been called, and we haven't yet called
> -	 * start_streaming() since not enough buffers were queued, and
> -	 * we now have reached the minimum number of queued buffers,
> -	 * then we can finally call start_streaming().
> -	 */
> -	if (q->streaming && !q->start_streaming_called &&
> -	    q->queued_count >= q->min_buffers_needed) {
> -		ret = vb2_start_streaming(q);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	dprintk(1, "qbuf of buffer %d succeeded\n", vb->index);
> -	return 0;
> +	return __vb2_core_qbuf(vb, q);
>  }
>  EXPORT_SYMBOL_GPL(vb2_core_qbuf);
>
Hans Verkuil July 6, 2017, 7:46 a.m. UTC | #2 
On 06/16/17 09:39, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> 
> In order to support explicit synchronization we need to divide
> vb2_core_qbuf() in two parts, one to be executed before the fence
> signals and another one to do the actual queueing of the buffer.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 51 ++++++++++++++++++--------------
>  1 file changed, 29 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 3107e21..ea83126 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1367,6 +1367,34 @@ static int vb2_start_streaming(struct vb2_queue *q)
>  	return ret;
>  }
>  
> +static int __vb2_core_qbuf(struct vb2_buffer *vb, struct vb2_queue *q)
> +{
> +	int ret;
> +
> +	/*
> +	 * If already streaming, give the buffer to driver for processing.
> +	 * If not, the buffer will be given to driver on next streamon.
> +	 */
> +	if (q->start_streaming_called)
> +		__enqueue_in_driver(vb);
> +
> +	/*
> +	 * If streamon has been called, and we haven't yet called
> +	 * start_streaming() since not enough buffers were queued, and
> +	 * we now have reached the minimum number of queued buffers,
> +	 * then we can finally call start_streaming().
> +	 */
> +	if (q->streaming && !q->start_streaming_called &&
> +	    q->queued_count >= q->min_buffers_needed) {
> +		ret = vb2_start_streaming(q);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	dprintk(1, "qbuf of buffer %d succeeded\n", vb->index);
> +	return 0;
> +}
> +
>  int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
>  {
>  	struct vb2_buffer *vb;
> @@ -1404,32 +1432,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
>  
>  	trace_vb2_qbuf(q, vb);
>  
> -	/*
> -	 * If already streaming, give the buffer to driver for processing.
> -	 * If not, the buffer will be given to driver on next streamon.
> -	 */
> -	if (q->start_streaming_called)
> -		__enqueue_in_driver(vb);
> -
>  	/* Fill buffer information for the userspace */
>  	if (pb)
>  		call_void_bufop(q, fill_user_buffer, vb, pb);

This should be called *after* the __vb2_core_qbuf call. That call changes
vb->state which is used by buffer to fill in v4l2_buffer. So the order
should be swapped here to ensure we return the latest state of the buffer.

>  
> -	/*
> -	 * If streamon has been called, and we haven't yet called
> -	 * start_streaming() since not enough buffers were queued, and
> -	 * we now have reached the minimum number of queued buffers,
> -	 * then we can finally call start_streaming().
> -	 */
> -	if (q->streaming && !q->start_streaming_called &&
> -	    q->queued_count >= q->min_buffers_needed) {
> -		ret = vb2_start_streaming(q);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	dprintk(1, "qbuf of buffer %d succeeded\n", vb->index);
> -	return 0;
> +	return __vb2_core_qbuf(vb, q);
>  }
>  EXPORT_SYMBOL_GPL(vb2_core_qbuf);
>  
> 

Regards,

	Hans
Gustavo F. Padovan July 7, 2017, 1:04 a.m. UTC | #3 
2017-07-06 Hans Verkuil <hverkuil@xs4all.nl>:

> On 06/16/17 09:39, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.com>
> > 
> > In order to support explicit synchronization we need to divide
> > vb2_core_qbuf() in two parts, one to be executed before the fence
> > signals and another one to do the actual queueing of the buffer.
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> > ---
> >  drivers/media/v4l2-core/videobuf2-core.c | 51 ++++++++++++++++++--------------
> >  1 file changed, 29 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> > index 3107e21..ea83126 100644
> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -1367,6 +1367,34 @@ static int vb2_start_streaming(struct vb2_queue *q)
> >  	return ret;
> >  }
> >  
> > +static int __vb2_core_qbuf(struct vb2_buffer *vb, struct vb2_queue *q)
> > +{
> > +	int ret;
> > +
> > +	/*
> > +	 * If already streaming, give the buffer to driver for processing.
> > +	 * If not, the buffer will be given to driver on next streamon.
> > +	 */
> > +	if (q->start_streaming_called)
> > +		__enqueue_in_driver(vb);
> > +
> > +	/*
> > +	 * If streamon has been called, and we haven't yet called
> > +	 * start_streaming() since not enough buffers were queued, and
> > +	 * we now have reached the minimum number of queued buffers,
> > +	 * then we can finally call start_streaming().
> > +	 */
> > +	if (q->streaming && !q->start_streaming_called &&
> > +	    q->queued_count >= q->min_buffers_needed) {
> > +		ret = vb2_start_streaming(q);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	dprintk(1, "qbuf of buffer %d succeeded\n", vb->index);
> > +	return 0;
> > +}
> > +
> >  int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
> >  {
> >  	struct vb2_buffer *vb;
> > @@ -1404,32 +1432,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
> >  
> >  	trace_vb2_qbuf(q, vb);
> >  
> > -	/*
> > -	 * If already streaming, give the buffer to driver for processing.
> > -	 * If not, the buffer will be given to driver on next streamon.
> > -	 */
> > -	if (q->start_streaming_called)
> > -		__enqueue_in_driver(vb);
> > -
> >  	/* Fill buffer information for the userspace */
> >  	if (pb)
> >  		call_void_bufop(q, fill_user_buffer, vb, pb);
> 
> This should be called *after* the __vb2_core_qbuf call. That call changes
> vb->state which is used by buffer to fill in v4l2_buffer. So the order
> should be swapped here to ensure we return the latest state of the buffer.

I didn't pay attention to that detail. The reason why I put it here is
that later when in-fences are introduced we will only call
__vb2_core_qbuf() after the fence signals and that may happen after we
returned to userspace. I'll look into possible ways to solve this
problem.

Gustavo
diff mbox

Patch

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 3107e21..ea83126 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1367,6 +1367,34 @@  static int vb2_start_streaming(struct vb2_queue *q)
 	return ret;
 }
 
+static int __vb2_core_qbuf(struct vb2_buffer *vb, struct vb2_queue *q)
+{
+	int ret;
+
+	/*
+	 * If already streaming, give the buffer to driver for processing.
+	 * If not, the buffer will be given to driver on next streamon.
+	 */
+	if (q->start_streaming_called)
+		__enqueue_in_driver(vb);
+
+	/*
+	 * If streamon has been called, and we haven't yet called
+	 * start_streaming() since not enough buffers were queued, and
+	 * we now have reached the minimum number of queued buffers,
+	 * then we can finally call start_streaming().
+	 */
+	if (q->streaming && !q->start_streaming_called &&
+	    q->queued_count >= q->min_buffers_needed) {
+		ret = vb2_start_streaming(q);
+		if (ret)
+			return ret;
+	}
+
+	dprintk(1, "qbuf of buffer %d succeeded\n", vb->index);
+	return 0;
+}
+
 int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
 {
 	struct vb2_buffer *vb;
@@ -1404,32 +1432,11 @@  int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
 
 	trace_vb2_qbuf(q, vb);
 
-	/*
-	 * If already streaming, give the buffer to driver for processing.
-	 * If not, the buffer will be given to driver on next streamon.
-	 */
-	if (q->start_streaming_called)
-		__enqueue_in_driver(vb);
-
 	/* Fill buffer information for the userspace */
 	if (pb)
 		call_void_bufop(q, fill_user_buffer, vb, pb);
 
-	/*
-	 * If streamon has been called, and we haven't yet called
-	 * start_streaming() since not enough buffers were queued, and
-	 * we now have reached the minimum number of queued buffers,
-	 * then we can finally call start_streaming().
-	 */
-	if (q->streaming && !q->start_streaming_called &&
-	    q->queued_count >= q->min_buffers_needed) {
-		ret = vb2_start_streaming(q);
-		if (ret)
-			return ret;
-	}
-
-	dprintk(1, "qbuf of buffer %d succeeded\n", vb->index);
-	return 0;
+	return __vb2_core_qbuf(vb, q);
 }
 EXPORT_SYMBOL_GPL(vb2_core_qbuf);