Re: [PATCH] vb2: fix bytesused == 0 handling

[Marek Szyprowski <m.szyprowski@samsung.com>]

Hello,

On 2014-07-17 11:53, Hans Verkuil wrote:
> The original report from Nikhil was that if data_offset > 0 and bytesused == 0,
> then the check in __verify_length() would fail, even though the spec says that
> if bytes_used == 0, then it will be replaced by the actual length of the
> buffer.
>
> After digging into it a bit more I realized that there were several other
> things wrong:
>
> - in __verify_length() it would use the application-provided length value
>    for USERPTR and the vb2 core length for other memory models, but it
>    should have used the application-provided length as well for DMABUF.
>
> - in __fill_vb2_buffer() on the other hand it would replace bytesused == 0
>    by the application-provided length, even for MMAP buffers where the
>    length is determined by the vb2 core.
>
> - in __fill_vb2_buffer() it tries to figure out if all the planes have
>    bytesused == 0 before it will decide to replace bytesused by length.
>    However, the spec makes no such provision, and it makes for convoluted
>    code. So just replace any bytesused == 0 by the proper length.
>    The idea behind this was that you could use bytesused to signal empty
>    planes, something that is currently not supported. But that is better
>    done in the future by using one of the reserved fields in strucy v4l2_plane.
>
> This patch fixes all these issues.
>
> Regards,
>
> 	Hans
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Reported-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> Cc: Nikhil Devshatwar <nikhil.nd@ti.com>

Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---
>   drivers/media/v4l2-core/videobuf2-core.c | 78 ++++++++++++++++----------------
>   1 file changed, 38 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 7c4489c..f255c14 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -576,6 +576,7 @@ static int __verify_planes_array(struct vb2_buffer *vb, const struct v4l2_buffer
>   static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>   {
>   	unsigned int length;
> +	unsigned int bytesused;
>   	unsigned int plane;
>   
>   	if (!V4L2_TYPE_IS_OUTPUT(b->type))
> @@ -583,21 +584,24 @@ static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>   
>   	if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
>   		for (plane = 0; plane < vb->num_planes; ++plane) {
> -			length = (b->memory == V4L2_MEMORY_USERPTR)
> +			length = (b->memory == V4L2_MEMORY_USERPTR ||
> +				  b->memory == V4L2_MEMORY_DMABUF)
>   			       ? b->m.planes[plane].length
>   			       : vb->v4l2_planes[plane].length;
> +			bytesused = b->m.planes[plane].bytesused
> +				  ? b->m.planes[plane].bytesused : length;
>   
>   			if (b->m.planes[plane].bytesused > length)
>   				return -EINVAL;
>   
>   			if (b->m.planes[plane].data_offset > 0 &&
> -			    b->m.planes[plane].data_offset >=
> -			    b->m.planes[plane].bytesused)
> +			    b->m.planes[plane].data_offset >= bytesused)
>   				return -EINVAL;
>   		}
>   	} else {
>   		length = (b->memory == V4L2_MEMORY_USERPTR)
>   		       ? b->length : vb->v4l2_planes[0].length;
> +		bytesused = b->bytesused ? b->bytesused : length;
>   
>   		if (b->bytesused > length)
>   			return -EINVAL;
> @@ -1234,35 +1238,6 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
>   	unsigned int plane;
>   
>   	if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
> -		/* Fill in driver-provided information for OUTPUT types */
> -		if (V4L2_TYPE_IS_OUTPUT(b->type)) {
> -			bool bytesused_is_used;
> -
> -			/* Check if bytesused == 0 for all planes */
> -			for (plane = 0; plane < vb->num_planes; ++plane)
> -				if (b->m.planes[plane].bytesused)
> -					break;
> -			bytesused_is_used = plane < vb->num_planes;
> -
> -			/*
> -			 * Will have to go up to b->length when API starts
> -			 * accepting variable number of planes.
> -			 *
> -			 * If bytesused_is_used is false, then fall back to the
> -			 * full buffer size. In that case userspace clearly
> -			 * never bothered to set it and it's a safe assumption
> -			 * that they really meant to use the full plane sizes.
> -			 */
> -			for (plane = 0; plane < vb->num_planes; ++plane) {
> -				struct v4l2_plane *pdst = &v4l2_planes[plane];
> -				struct v4l2_plane *psrc = &b->m.planes[plane];
> -
> -				pdst->bytesused = bytesused_is_used ?
> -					psrc->bytesused : psrc->length;
> -				pdst->data_offset = psrc->data_offset;
> -			}
> -		}
> -
>   		if (b->memory == V4L2_MEMORY_USERPTR) {
>   			for (plane = 0; plane < vb->num_planes; ++plane) {
>   				v4l2_planes[plane].m.userptr =
> @@ -1279,6 +1254,28 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
>   					b->m.planes[plane].length;
>   			}
>   		}
> +
> +		/* Fill in driver-provided information for OUTPUT types */
> +		if (V4L2_TYPE_IS_OUTPUT(b->type)) {
> +			/*
> +			 * Will have to go up to b->length when API starts
> +			 * accepting variable number of planes.
> +			 *
> +			 * If bytesused == 0 for the output buffer, then fall
> +			 * back to the full buffer size. In that case
> +			 * userspace clearly never bothered to set it and
> +			 * it's a safe assumption that they really meant to
> +			 * use the full plane sizes.
> +			 */
> +			for (plane = 0; plane < vb->num_planes; ++plane) {
> +				struct v4l2_plane *pdst = &v4l2_planes[plane];
> +				struct v4l2_plane *psrc = &b->m.planes[plane];
> +
> +				pdst->bytesused = psrc->bytesused ?
> +					psrc->bytesused : pdst->length;
> +				pdst->data_offset = psrc->data_offset;
> +			}
> +		}
>   	} else {
>   		/*
>   		 * Single-planar buffers do not use planes array,
> @@ -1286,15 +1283,9 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
>   		 * In videobuf we use our internal V4l2_planes struct for
>   		 * single-planar buffers as well, for simplicity.
>   		 *
> -		 * If bytesused == 0, then fall back to the full buffer size
> -		 * as that's a sensible default.
> +		 * If bytesused == 0 for the output buffer, then fall back
> +		 * to the full buffer size as that's a sensible default.
>   		 */
> -		if (V4L2_TYPE_IS_OUTPUT(b->type))
> -			v4l2_planes[0].bytesused =
> -				b->bytesused ? b->bytesused : b->length;
> -		else
> -			v4l2_planes[0].bytesused = 0;
> -
>   		if (b->memory == V4L2_MEMORY_USERPTR) {
>   			v4l2_planes[0].m.userptr = b->m.userptr;
>   			v4l2_planes[0].length = b->length;
> @@ -1304,6 +1295,13 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
>   			v4l2_planes[0].m.fd = b->m.fd;
>   			v4l2_planes[0].length = b->length;
>   		}
> +
> +		if (V4L2_TYPE_IS_OUTPUT(b->type))
> +			v4l2_planes[0].bytesused = b->bytesused ?
> +				b->bytesused : v4l2_planes[0].length;
> +		else
> +			v4l2_planes[0].bytesused = 0;
> +
>   	}
>   
>   	/* Zero flags that the vb2 core handles */

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland