diff mbox

[RFCv1,3/6] videobuf2-core: move plane verification out of __fill_v4l2_buffer.

Message ID bf34157b75c930ab456dc977ebafbe895c7a3e8a.1348064901.git.hans.verkuil@cisco.com (mailing list archive)
State New, archived
Headers show

Commit Message

Hans Verkuil Sept. 19, 2012, 2:37 p.m. UTC 
From: Hans Verkuil <hans.verkuil@cisco.com>

The plane verification should be done before actually queuing or
dequeuing buffers, so move it out of __fill_v4l2_buffer and call it
as a separate step.

The also makes it possible to change the return type of __fill_v4l2_buffer
to void.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/videobuf2-core.c |   29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

Comments

Sylwester Nawrocki/Kernel \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics Sept. 19, 2012, 4:55 p.m. UTC | #1 
On 09/19/2012 04:37 PM, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> The plane verification should be done before actually queuing or
> dequeuing buffers, so move it out of __fill_v4l2_buffer and call it
> as a separate step.
> 
> The also makes it possible to change the return type of __fill_v4l2_buffer
> to void.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Tested-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

There are just two small comment below...

> ---
>  drivers/media/v4l2-core/videobuf2-core.c |   29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 2e26e58..929cc99 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -276,6 +276,9 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
>   */
>  static int __verify_planes_array(struct vb2_queue *q, const struct v4l2_buffer *b)
>  {
> +	if (!V4L2_TYPE_IS_MULTIPLANAR(b->type))
> +		return 0;
> +
>  	/* Is memory for copying plane information present? */
>  	if (NULL == b->m.planes) {
>  		dprintk(1, "Multi-planar buffer passed but "
> @@ -331,10 +334,9 @@ static bool __buffers_in_use(struct vb2_queue *q)
>   * __fill_v4l2_buffer() - fill in a struct v4l2_buffer with information to be
>   * returned to userspace
>   */
> -static int __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
> +static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
>  {
>  	struct vb2_queue *q = vb->vb2_queue;
> -	int ret;
>  
>  	/* Copy back data such as timestamp, flags, etc. */
>  	memcpy(b, &vb->v4l2_buf, offsetof(struct v4l2_buffer, m));
> @@ -342,10 +344,6 @@ static int __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
>  	b->reserved = vb->v4l2_buf.reserved;
>  
>  	if (V4L2_TYPE_IS_MULTIPLANAR(q->type)) {
> -		ret = __verify_planes_array(q, b);
> -		if (ret)
> -			return ret;
> -
>  		/*
>  		 * Fill in plane-related data if userspace provided an array
>  		 * for it. The memory and size is verified above.

This comment should be updated, since __verify_planes_array() is now removed.

> @@ -391,8 +389,6 @@ static int __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
>  
>  	if (__buffer_in_use(q, vb))
>  		b->flags |= V4L2_BUF_FLAG_MAPPED;
> -
> -	return 0;
>  }
>  
>  /**
> @@ -411,6 +407,7 @@ static int __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
>  int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
>  {
>  	struct vb2_buffer *vb;
> +	int ret;
>  
>  	if (b->type != q->type) {
>  		dprintk(1, "querybuf: wrong buffer type\n");
> @@ -422,8 +419,10 @@ int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
>  		return -EINVAL;
>  	}
>  	vb = q->bufs[b->index];
> -
> -	return __fill_v4l2_buffer(vb, b);
> +	ret = __verify_planes_array(q, b);
> +	if (!ret)
> +		__fill_v4l2_buffer(vb, b);
> +	return ret;
>  }
>  EXPORT_SYMBOL(vb2_querybuf);
>  
> @@ -1061,8 +1060,8 @@ int vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b)
>  		dprintk(1, "%s(): invalid buffer state %d\n", __func__, vb->state);
>  		return -EINVAL;
>  	}
> -
> -	ret = __buf_prepare(vb, b);
> +	ret = __verify_planes_array(q, b);
> +	ret = ret ? ret : __buf_prepare(vb, b);

Could we just make it:

	ret = __verify_planes_array(q, b);
  	if (ret < 0)
  		return ret;

	ret = __buf_prepare(vb, b);
  	if (ret < 0)
  		return ret;

?
>  	if (ret < 0)
>  		return ret;
>  
> @@ -1149,6 +1148,9 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>  		ret = -EINVAL;
>  		goto unlock;
>  	}
> +	ret = __verify_planes_array(q, b);
> +	if (ret)
> +		return ret;
>  
>  	switch (vb->state) {
>  	case VB2_BUF_STATE_DEQUEUED:
> @@ -1337,6 +1339,9 @@ int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking)
>  		dprintk(1, "dqbuf: invalid buffer type\n");
>  		return -EINVAL;
>  	}
> +	ret = __verify_planes_array(q, b);
> +	if (ret)
> +		return ret;
>  
>  	ret = __vb2_get_done_vb(q, &vb, nonblocking);
>  	if (ret < 0) {

--

Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans Verkuil Sept. 20, 2012, 6:28 a.m. UTC | #2 
On Wed September 19 2012 18:55:25 Sylwester Nawrocki wrote:
> On 09/19/2012 04:37 PM, Hans Verkuil wrote:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> > 
> > The plane verification should be done before actually queuing or
> > dequeuing buffers, so move it out of __fill_v4l2_buffer and call it
> > as a separate step.
> > 
> > The also makes it possible to change the return type of __fill_v4l2_buffer
> > to void.
> > 
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Tested-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> 
> There are just two small comment below...
> 
> > ---
> >  drivers/media/v4l2-core/videobuf2-core.c |   29 +++++++++++++++++------------
> >  1 file changed, 17 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> > index 2e26e58..929cc99 100644
> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -276,6 +276,9 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
> >   */
> >  static int __verify_planes_array(struct vb2_queue *q, const struct v4l2_buffer *b)
> >  {
> > +	if (!V4L2_TYPE_IS_MULTIPLANAR(b->type))
> > +		return 0;
> > +
> >  	/* Is memory for copying plane information present? */
> >  	if (NULL == b->m.planes) {
> >  		dprintk(1, "Multi-planar buffer passed but "
> > @@ -331,10 +334,9 @@ static bool __buffers_in_use(struct vb2_queue *q)
> >   * __fill_v4l2_buffer() - fill in a struct v4l2_buffer with information to be
> >   * returned to userspace
> >   */
> > -static int __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
> > +static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
> >  {
> >  	struct vb2_queue *q = vb->vb2_queue;
> > -	int ret;
> >  
> >  	/* Copy back data such as timestamp, flags, etc. */
> >  	memcpy(b, &vb->v4l2_buf, offsetof(struct v4l2_buffer, m));
> > @@ -342,10 +344,6 @@ static int __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
> >  	b->reserved = vb->v4l2_buf.reserved;
> >  
> >  	if (V4L2_TYPE_IS_MULTIPLANAR(q->type)) {
> > -		ret = __verify_planes_array(q, b);
> > -		if (ret)
> > -			return ret;
> > -
> >  		/*
> >  		 * Fill in plane-related data if userspace provided an array
> >  		 * for it. The memory and size is verified above.
> 
> This comment should be updated, since __verify_planes_array() is now removed.

Will do.

> > @@ -391,8 +389,6 @@ static int __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
> >  
> >  	if (__buffer_in_use(q, vb))
> >  		b->flags |= V4L2_BUF_FLAG_MAPPED;
> > -
> > -	return 0;
> >  }
> >  
> >  /**
> > @@ -411,6 +407,7 @@ static int __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
> >  int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
> >  {
> >  	struct vb2_buffer *vb;
> > +	int ret;
> >  
> >  	if (b->type != q->type) {
> >  		dprintk(1, "querybuf: wrong buffer type\n");
> > @@ -422,8 +419,10 @@ int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
> >  		return -EINVAL;
> >  	}
> >  	vb = q->bufs[b->index];
> > -
> > -	return __fill_v4l2_buffer(vb, b);
> > +	ret = __verify_planes_array(q, b);
> > +	if (!ret)
> > +		__fill_v4l2_buffer(vb, b);
> > +	return ret;
> >  }
> >  EXPORT_SYMBOL(vb2_querybuf);
> >  
> > @@ -1061,8 +1060,8 @@ int vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b)
> >  		dprintk(1, "%s(): invalid buffer state %d\n", __func__, vb->state);
> >  		return -EINVAL;
> >  	}
> > -
> > -	ret = __buf_prepare(vb, b);
> > +	ret = __verify_planes_array(q, b);
> > +	ret = ret ? ret : __buf_prepare(vb, b);
> 
> Could we just make it:
> 
> 	ret = __verify_planes_array(q, b);
>   	if (ret < 0)
>   		return ret;
> 
> 	ret = __buf_prepare(vb, b);
>   	if (ret < 0)
>   		return ret;
> 
> ?

OK.

Regards,

	Hans

> >  	if (ret < 0)
> >  		return ret;
> >  
> > @@ -1149,6 +1148,9 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> >  		ret = -EINVAL;
> >  		goto unlock;
> >  	}
> > +	ret = __verify_planes_array(q, b);
> > +	if (ret)
> > +		return ret;
> >  
> >  	switch (vb->state) {
> >  	case VB2_BUF_STATE_DEQUEUED:
> > @@ -1337,6 +1339,9 @@ int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking)
> >  		dprintk(1, "dqbuf: invalid buffer type\n");
> >  		return -EINVAL;
> >  	}
> > +	ret = __verify_planes_array(q, b);
> > +	if (ret)
> > +		return ret;
> >  
> >  	ret = __vb2_get_done_vb(q, &vb, nonblocking);
> >  	if (ret < 0) {
> 
> --
> 
> Regards,
> Sylwester
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 2e26e58..929cc99 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -276,6 +276,9 @@  static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
  */
 static int __verify_planes_array(struct vb2_queue *q, const struct v4l2_buffer *b)
 {
+	if (!V4L2_TYPE_IS_MULTIPLANAR(b->type))
+		return 0;
+
 	/* Is memory for copying plane information present? */
 	if (NULL == b->m.planes) {
 		dprintk(1, "Multi-planar buffer passed but "
@@ -331,10 +334,9 @@  static bool __buffers_in_use(struct vb2_queue *q)
  * __fill_v4l2_buffer() - fill in a struct v4l2_buffer with information to be
  * returned to userspace
  */
-static int __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
+static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
 {
 	struct vb2_queue *q = vb->vb2_queue;
-	int ret;
 
 	/* Copy back data such as timestamp, flags, etc. */
 	memcpy(b, &vb->v4l2_buf, offsetof(struct v4l2_buffer, m));
@@ -342,10 +344,6 @@  static int __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
 	b->reserved = vb->v4l2_buf.reserved;
 
 	if (V4L2_TYPE_IS_MULTIPLANAR(q->type)) {
-		ret = __verify_planes_array(q, b);
-		if (ret)
-			return ret;
-
 		/*
 		 * Fill in plane-related data if userspace provided an array
 		 * for it. The memory and size is verified above.
@@ -391,8 +389,6 @@  static int __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
 
 	if (__buffer_in_use(q, vb))
 		b->flags |= V4L2_BUF_FLAG_MAPPED;
-
-	return 0;
 }
 
 /**
@@ -411,6 +407,7 @@  static int __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
 int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
 {
 	struct vb2_buffer *vb;
+	int ret;
 
 	if (b->type != q->type) {
 		dprintk(1, "querybuf: wrong buffer type\n");
@@ -422,8 +419,10 @@  int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
 		return -EINVAL;
 	}
 	vb = q->bufs[b->index];
-
-	return __fill_v4l2_buffer(vb, b);
+	ret = __verify_planes_array(q, b);
+	if (!ret)
+		__fill_v4l2_buffer(vb, b);
+	return ret;
 }
 EXPORT_SYMBOL(vb2_querybuf);
 
@@ -1061,8 +1060,8 @@  int vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b)
 		dprintk(1, "%s(): invalid buffer state %d\n", __func__, vb->state);
 		return -EINVAL;
 	}
-
-	ret = __buf_prepare(vb, b);
+	ret = __verify_planes_array(q, b);
+	ret = ret ? ret : __buf_prepare(vb, b);
 	if (ret < 0)
 		return ret;
 
@@ -1149,6 +1148,9 @@  int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
 		ret = -EINVAL;
 		goto unlock;
 	}
+	ret = __verify_planes_array(q, b);
+	if (ret)
+		return ret;
 
 	switch (vb->state) {
 	case VB2_BUF_STATE_DEQUEUED:
@@ -1337,6 +1339,9 @@  int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking)
 		dprintk(1, "dqbuf: invalid buffer type\n");
 		return -EINVAL;
 	}
+	ret = __verify_planes_array(q, b);
+	if (ret)
+		return ret;
 
 	ret = __vb2_get_done_vb(q, &vb, nonblocking);
 	if (ret < 0) {