[PATCH v5 1/4] vb2: split the io_flags member of vb2_queue into a bit field

[PATCH v5 1/4] vb2: split the io_flags member of vb2_queue into a bit field

[Kamil Debski]

This patch splits the io_flags member of vb2_queue into a bit field.
Instead of an enum with flags separate bit fields were introduced.

Signed-off-by: Kamil Debski <k.debski@samsung.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/videobuf2-core.c |   17 +++++++++--------
 include/media/videobuf2-core.h           |   18 +++++-------------
 2 files changed, 14 insertions(+), 21 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index bc08a82..5cd60bf 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -2760,7 +2760,8 @@ struct vb2_fileio_data {
 	unsigned int initial_index;
 	unsigned int q_count;
 	unsigned int dq_count;
-	unsigned int flags;
+	unsigned read_once:1;
+	unsigned write_immediately:1;
 };
 
 /**
@@ -2798,14 +2799,16 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
 	 */
 	count = 1;
 
-	dprintk(3, "setting up file io: mode %s, count %d, flags %08x\n",
-		(read) ? "read" : "write", count, q->io_flags);
+	dprintk(3, "setting up file io: mode %s, count %d, read_once %d, write_immediately %d\n",
+		(read) ? "read" : "write", count, q->fileio_read_once,
+		q->fileio_write_immediately);
 
 	fileio = kzalloc(sizeof(struct vb2_fileio_data), GFP_KERNEL);
 	if (fileio == NULL)
 		return -ENOMEM;
 
-	fileio->flags = q->io_flags;
+	fileio->read_once = q->fileio_read_once;
+	fileio->write_immediately = q->fileio_write_immediately;
 
 	/*
 	 * Request buffers and use MMAP type to force driver
@@ -3028,13 +3031,11 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
 	/*
 	 * Queue next buffer if required.
 	 */
-	if (buf->pos == buf->size ||
-	   (!read && (fileio->flags & VB2_FILEIO_WRITE_IMMEDIATELY))) {
+	if (buf->pos == buf->size || (!read && fileio->write_immediately)) {
 		/*
 		 * Check if this is the last buffer to read.
 		 */
-		if (read && (fileio->flags & VB2_FILEIO_READ_ONCE) &&
-		    fileio->dq_count == 1) {
+		if (read && fileio->read_once && fileio->dq_count == 1) {
 			dprintk(3, "read limit reached\n");
 			return __vb2_cleanup_fileio(q);
 		}
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index bd2cec2..e49dc6b 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -134,17 +134,6 @@ enum vb2_io_modes {
 };
 
 /**
- * enum vb2_fileio_flags - flags for selecting a mode of the file io emulator,
- * by default the 'streaming' style is used by the file io emulator
- * @VB2_FILEIO_READ_ONCE:	report EOF after reading the first buffer
- * @VB2_FILEIO_WRITE_IMMEDIATELY:	queue buffer after each write() call
- */
-enum vb2_fileio_flags {
-	VB2_FILEIO_READ_ONCE		= (1 << 0),
-	VB2_FILEIO_WRITE_IMMEDIATELY	= (1 << 1),
-};
-
-/**
  * enum vb2_buffer_state - current video buffer state
  * @VB2_BUF_STATE_DEQUEUED:	buffer under userspace control
  * @VB2_BUF_STATE_PREPARING:	buffer is being prepared in videobuf
@@ -346,7 +335,8 @@ struct v4l2_fh;
  *
  * @type:	queue type (see V4L2_BUF_TYPE_* in linux/videodev2.h
  * @io_modes:	supported io methods (see vb2_io_modes enum)
- * @io_flags:	additional io flags (see vb2_fileio_flags enum)
+ * @fileio_read_once:		report EOF after reading the first buffer
+ * @fileio_write_immediately:	queue buffer after each write() call
  * @lock:	pointer to a mutex that protects the vb2_queue struct. The
  *		driver can set this to a mutex to let the v4l2 core serialize
  *		the queuing ioctls. If the driver wants to handle locking
@@ -396,7 +386,9 @@ struct v4l2_fh;
 struct vb2_queue {
 	enum v4l2_buf_type		type;
 	unsigned int			io_modes;
-	unsigned int			io_flags;
+	unsigned			fileio_read_once:1;
+	unsigned			fileio_write_immediately:1;
+
 	struct mutex			*lock;
 	struct v4l2_fh			*owner;
 
-- 
1.7.9.5

[PATCH v5 2/4] vb2: add allow_zero_bytesused flag to the vb2_queue struct

[Kamil Debski]

The vb2: fix bytesused == 0 handling (8a75ffb) patch changed the behavior
of __fill_vb2_buffer function, so that if bytesused is 0 it is set to the
size of the buffer. However, bytesused set to 0 is used by older codec
drivers as as indication used to mark the end of stream.

To keep backward compatibility, this patch adds a flag passed to the
vb2_queue_init function - allow_zero_bytesused. If the flag is set upon
initialization of the queue, the videobuf2 keeps the value of bytesused
intact in the OUTPUT queue and passes it to the driver.

Reported-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Signed-off-by: Kamil Debski <k.debski@samsung.com>
---
 drivers/media/v4l2-core/videobuf2-core.c |   39 +++++++++++++++++++++++++-----
 include/media/videobuf2-core.h           |    2 ++
 2 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 5cd60bf..5d77670 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1247,6 +1247,16 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
 {
 	unsigned int plane;
 
+	if (V4L2_TYPE_IS_OUTPUT(b->type)) {
+		if (WARN_ON_ONCE(b->bytesused == 0)) {
+			pr_warn_once("use of bytesused == 0 is deprecated and will be removed in 2017,\n");
+			if (vb->vb2_queue->allow_zero_bytesused)
+				pr_warn_once("use VIDIOC_DECODER_CMD(V4L2_DEC_CMD_STOP) instead.\n");
+			else
+				pr_warn_once("use the actual size instead.\n");
+		}
+	}
+
 	if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
 		if (b->memory == V4L2_MEMORY_USERPTR) {
 			for (plane = 0; plane < vb->num_planes; ++plane) {
@@ -1276,13 +1286,22 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
 			 * userspace clearly never bothered to set it and
 			 * it's a safe assumption that they really meant to
 			 * use the full plane sizes.
+			 *
+			 * Some drivers, e.g. old codec drivers, use bytesused
+			 * == 0 as a way to indicate that streaming is finished.
+			 * In that case, the driver should use the
+			 * allow_zero_bytesused flag to keep old userspace
+			 * applications working.
 			 */
 			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;
+				if (vb->vb2_queue->allow_zero_bytesused)
+					pdst->bytesused = psrc->bytesused;
+				else
+					pdst->bytesused = psrc->bytesused ?
+						psrc->bytesused : pdst->length;
 				pdst->data_offset = psrc->data_offset;
 			}
 		}
@@ -1295,6 +1314,11 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
 		 *
 		 * If bytesused == 0 for the output buffer, then fall back
 		 * to the full buffer size as that's a sensible default.
+		 *
+		 * Some drivers, e.g. old codec drivers, use bytesused * == 0 as
+		 * a way to indicate that streaming is finished. In that case,
+		 * the driver should use the allow_zero_bytesused flag to keep
+		 * old userspace applications working.
 		 */
 		if (b->memory == V4L2_MEMORY_USERPTR) {
 			v4l2_planes[0].m.userptr = b->m.userptr;
@@ -1306,10 +1330,13 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
 			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
+		if (V4L2_TYPE_IS_OUTPUT(b->type)) {
+			if (vb->vb2_queue->allow_zero_bytesused)
+				v4l2_planes[0].bytesused = b->bytesused;
+			else
+				v4l2_planes[0].bytesused = b->bytesused ?
+					b->bytesused : v4l2_planes[0].length;
+		} else
 			v4l2_planes[0].bytesused = 0;
 
 	}
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index e49dc6b..a5790fd 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -337,6 +337,7 @@ struct v4l2_fh;
  * @io_modes:	supported io methods (see vb2_io_modes enum)
  * @fileio_read_once:		report EOF after reading the first buffer
  * @fileio_write_immediately:	queue buffer after each write() call
+ * @allow_zero_bytesused:	allow bytesused == 0 to be passed to the driver
  * @lock:	pointer to a mutex that protects the vb2_queue struct. The
  *		driver can set this to a mutex to let the v4l2 core serialize
  *		the queuing ioctls. If the driver wants to handle locking
@@ -388,6 +389,7 @@ struct vb2_queue {
 	unsigned int			io_modes;
 	unsigned			fileio_read_once:1;
 	unsigned			fileio_write_immediately:1;
+	unsigned			allow_zero_bytesused:1;
 
 	struct mutex			*lock;
 	struct v4l2_fh			*owner;
-- 
1.7.9.5

[PATCH v5 3/4] coda: set allow_zero_bytesused flag for vb2_queue_init

[Kamil Debski]

The coda driver interprets a buffer with bytesused equal to 0 as a special
case indicating end-of-stream. After vb2: fix bytesused == 0 handling
(8a75ffb) patch videobuf2 modified the value of bytesused if it was 0.
The allow_zero_bytesused flag was added to videobuf2 to keep
backward compatibility.

Signed-off-by: Kamil Debski <k.debski@samsung.com>
---
 drivers/media/platform/coda/coda-common.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index 6f32e6d..2d23f9a 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -1541,6 +1541,13 @@ static int coda_queue_init(struct coda_ctx *ctx, struct vb2_queue *vq)
 	vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
 	vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
 	vq->lock = &ctx->dev->dev_mutex;
+	/* One of means to indicate end-of-stream for coda is to set the
+	 * bytesused == 0. However by default videobuf2 handles videobuf
+	 * equal to 0 as a special case and changes its value to the size
+	 * of the buffer. Set the allow_zero_bytesused flag, so
+	 * that videobuf2 will keep the value of bytesused intact.
+	 */
+	vq->allow_zero_bytesused = 1;
 
 	return vb2_queue_init(vq);
 }
-- 
1.7.9.5

[PATCH v5 4/4] s5p-mfc: set allow_zero_bytesused flag for vb2_queue_init

[Kamil Debski]

The s5p-mfc driver interprets a buffer with bytesused equal to 0 as a
special case indicating end-of-stream. After vb2: fix bytesused == 0
handling (8a75ffb) patch videobuf2 modified the value of bytesused if it
was 0. The allow_zero_bytesused flag was added to videobuf2 to keep
backward compatibility.

Signed-off-by: Kamil Debski <k.debski@samsung.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index 8e44a59..6b08488 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -843,6 +843,13 @@ static int s5p_mfc_open(struct file *file)
 		ret = -ENOENT;
 		goto err_queue_init;
 	}
+	/* One of means to indicate end-of-stream for MFC is to set the
+	 * bytesused == 0. However by default videobuf2 handles videobuf
+	 * equal to 0 as a special case and changes its value to the size
+	 * of the buffer. Set the allow_zero_bytesused flag so that videobuf2
+	 * will keep the value of bytesused intact.
+	 */
+	q->allow_zero_bytesused = 1;
 	q->mem_ops = &vb2_dma_contig_memops;
 	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
 	ret = vb2_queue_init(q);
-- 
1.7.9.5

Re: [PATCH v5 2/4] vb2: add allow_zero_bytesused flag to the vb2_queue struct

[Hans Verkuil]

Hi Kamil,

One question and one typo below...

On 02/20/2015 05:38 PM, Kamil Debski wrote:
> The vb2: fix bytesused == 0 handling (8a75ffb) patch changed the behavior
> of __fill_vb2_buffer function, so that if bytesused is 0 it is set to the
> size of the buffer. However, bytesused set to 0 is used by older codec
> drivers as as indication used to mark the end of stream.
> 
> To keep backward compatibility, this patch adds a flag passed to the
> vb2_queue_init function - allow_zero_bytesused. If the flag is set upon
> initialization of the queue, the videobuf2 keeps the value of bytesused
> intact in the OUTPUT queue and passes it to the driver.
> 
> Reported-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Signed-off-by: Kamil Debski <k.debski@samsung.com>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c |   39 +++++++++++++++++++++++++-----
>  include/media/videobuf2-core.h           |    2 ++
>  2 files changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 5cd60bf..5d77670 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1247,6 +1247,16 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
>  {
>  	unsigned int plane;
>  
> +	if (V4L2_TYPE_IS_OUTPUT(b->type)) {
> +		if (WARN_ON_ONCE(b->bytesused == 0)) {
> +			pr_warn_once("use of bytesused == 0 is deprecated and will be removed in 2017,\n");

I wonder if we should give a specific year, or just say 'in the future'.

What do you think?

> +			if (vb->vb2_queue->allow_zero_bytesused)
> +				pr_warn_once("use VIDIOC_DECODER_CMD(V4L2_DEC_CMD_STOP) instead.\n");
> +			else
> +				pr_warn_once("use the actual size instead.\n");
> +		}
> +	}
> +
>  	if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
>  		if (b->memory == V4L2_MEMORY_USERPTR) {
>  			for (plane = 0; plane < vb->num_planes; ++plane) {
> @@ -1276,13 +1286,22 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
>  			 * userspace clearly never bothered to set it and
>  			 * it's a safe assumption that they really meant to
>  			 * use the full plane sizes.
> +			 *
> +			 * Some drivers, e.g. old codec drivers, use bytesused
> +			 * == 0 as a way to indicate that streaming is finished.
> +			 * In that case, the driver should use the
> +			 * allow_zero_bytesused flag to keep old userspace
> +			 * applications working.
>  			 */
>  			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;
> +				if (vb->vb2_queue->allow_zero_bytesused)
> +					pdst->bytesused = psrc->bytesused;
> +				else
> +					pdst->bytesused = psrc->bytesused ?
> +						psrc->bytesused : pdst->length;
>  				pdst->data_offset = psrc->data_offset;
>  			}
>  		}
> @@ -1295,6 +1314,11 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
>  		 *
>  		 * If bytesused == 0 for the output buffer, then fall back
>  		 * to the full buffer size as that's a sensible default.
> +		 *
> +		 * Some drivers, e.g. old codec drivers, use bytesused * == 0 as

Small typo:

s/bytesused * == 0/bytesused == 0/

> +		 * a way to indicate that streaming is finished. In that case,
> +		 * the driver should use the allow_zero_bytesused flag to keep
> +		 * old userspace applications working.
>  		 */
>  		if (b->memory == V4L2_MEMORY_USERPTR) {
>  			v4l2_planes[0].m.userptr = b->m.userptr;
> @@ -1306,10 +1330,13 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
>  			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
> +		if (V4L2_TYPE_IS_OUTPUT(b->type)) {
> +			if (vb->vb2_queue->allow_zero_bytesused)
> +				v4l2_planes[0].bytesused = b->bytesused;
> +			else
> +				v4l2_planes[0].bytesused = b->bytesused ?
> +					b->bytesused : v4l2_planes[0].length;
> +		} else
>  			v4l2_planes[0].bytesused = 0;
>  
>  	}
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index e49dc6b..a5790fd 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -337,6 +337,7 @@ struct v4l2_fh;
>   * @io_modes:	supported io methods (see vb2_io_modes enum)
>   * @fileio_read_once:		report EOF after reading the first buffer
>   * @fileio_write_immediately:	queue buffer after each write() call
> + * @allow_zero_bytesused:	allow bytesused == 0 to be passed to the driver
>   * @lock:	pointer to a mutex that protects the vb2_queue struct. The
>   *		driver can set this to a mutex to let the v4l2 core serialize
>   *		the queuing ioctls. If the driver wants to handle locking
> @@ -388,6 +389,7 @@ struct vb2_queue {
>  	unsigned int			io_modes;
>  	unsigned			fileio_read_once:1;
>  	unsigned			fileio_write_immediately:1;
> +	unsigned			allow_zero_bytesused:1;
>  
>  	struct mutex			*lock;
>  	struct v4l2_fh			*owner;
> 

Regards,

	Hans

Re: [PATCH v5 3/4] coda: set allow_zero_bytesused flag for vb2_queue_init

[Hans Verkuil]

On 02/20/2015 05:38 PM, Kamil Debski wrote:
> The coda driver interprets a buffer with bytesused equal to 0 as a special
> case indicating end-of-stream. After vb2: fix bytesused == 0 handling
> (8a75ffb) patch videobuf2 modified the value of bytesused if it was 0.
> The allow_zero_bytesused flag was added to videobuf2 to keep
> backward compatibility.
> 
> Signed-off-by: Kamil Debski <k.debski@samsung.com>
> ---
>  drivers/media/platform/coda/coda-common.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
> index 6f32e6d..2d23f9a 100644
> --- a/drivers/media/platform/coda/coda-common.c
> +++ b/drivers/media/platform/coda/coda-common.c
> @@ -1541,6 +1541,13 @@ static int coda_queue_init(struct coda_ctx *ctx, struct vb2_queue *vq)
>  	vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
>  	vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
>  	vq->lock = &ctx->dev->dev_mutex;
> +	/* One of means to indicate end-of-stream for coda is to set the

s/One of means/One way/

> +	 * bytesused == 0. However by default videobuf2 handles videobuf

s/videobuf/bytesused/

> +	 * equal to 0 as a special case and changes its value to the size
> +	 * of the buffer. Set the allow_zero_bytesused flag, so
> +	 * that videobuf2 will keep the value of bytesused intact.
> +	 */
> +	vq->allow_zero_bytesused = 1;
>  
>  	return vb2_queue_init(vq);
>  }
> 

Regards,

	Hans

Re: [PATCH v5 4/4] s5p-mfc: set allow_zero_bytesused flag for vb2_queue_init

[Hans Verkuil]

On 02/20/2015 05:38 PM, Kamil Debski wrote:
> The s5p-mfc driver interprets a buffer with bytesused equal to 0 as a
> special case indicating end-of-stream. After vb2: fix bytesused == 0
> handling (8a75ffb) patch videobuf2 modified the value of bytesused if it
> was 0. The allow_zero_bytesused flag was added to videobuf2 to keep
> backward compatibility.
> 
> Signed-off-by: Kamil Debski <k.debski@samsung.com>
> ---
>  drivers/media/platform/s5p-mfc/s5p_mfc.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> index 8e44a59..6b08488 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> @@ -843,6 +843,13 @@ static int s5p_mfc_open(struct file *file)
>  		ret = -ENOENT;
>  		goto err_queue_init;
>  	}
> +	/* One of means to indicate end-of-stream for MFC is to set the
> +	 * bytesused == 0. However by default videobuf2 handles videobuf

Same typos as in patch 3/4.

Regards,

	Hans

> +	 * equal to 0 as a special case and changes its value to the size
> +	 * of the buffer. Set the allow_zero_bytesused flag so that videobuf2
> +	 * will keep the value of bytesused intact.
> +	 */
> +	q->allow_zero_bytesused = 1;
>  	q->mem_ops = &vb2_dma_contig_memops;
>  	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
>  	ret = vb2_queue_init(q);
> 

RE: [PATCH v5 2/4] vb2: add allow_zero_bytesused flag to the vb2_queue struct

[Kamil Debski]

Hi,

Thank you for the review :)

> From: Hans Verkuil [mailto:hverkuil@xs4all.nl]
> Sent: Friday, February 20, 2015 5:52 PM
> 
> Hi Kamil,
> 
> One question and one typo below...
> 
> On 02/20/2015 05:38 PM, Kamil Debski wrote:
> > The vb2: fix bytesused == 0 handling (8a75ffb) patch changed the
> > behavior of __fill_vb2_buffer function, so that if bytesused is 0 it
> > is set to the size of the buffer. However, bytesused set to 0 is used
> > by older codec drivers as as indication used to mark the end of
> stream.
> >
> > To keep backward compatibility, this patch adds a flag passed to the
> > vb2_queue_init function - allow_zero_bytesused. If the flag is set
> > upon initialization of the queue, the videobuf2 keeps the value of
> > bytesused intact in the OUTPUT queue and passes it to the driver.
> >
> > Reported-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > Signed-off-by: Kamil Debski <k.debski@samsung.com>
> > ---
> >  drivers/media/v4l2-core/videobuf2-core.c |   39
> +++++++++++++++++++++++++-----
> >  include/media/videobuf2-core.h           |    2 ++
> >  2 files changed, 35 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> > b/drivers/media/v4l2-core/videobuf2-core.c
> > index 5cd60bf..5d77670 100644
> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -1247,6 +1247,16 @@ static void __fill_vb2_buffer(struct
> vb2_buffer
> > *vb, const struct v4l2_buffer *b  {
> >  	unsigned int plane;
> >
> > +	if (V4L2_TYPE_IS_OUTPUT(b->type)) {
> > +		if (WARN_ON_ONCE(b->bytesused == 0)) {
> > +			pr_warn_once("use of bytesused == 0 is deprecated
and
> will be
> > +removed in 2017,\n");
> 
> I wonder if we should give a specific year, or just say 'in the
> future'.
> 
> What do you think?

I think I would prefer to use the phrase "in the future". 
If you are ok with that I will post an updated patch set soon.

> 
> > +			if (vb->vb2_queue->allow_zero_bytesused)
> > +				pr_warn_once("use
> VIDIOC_DECODER_CMD(V4L2_DEC_CMD_STOP) instead.\n");
> > +			else
> > +				pr_warn_once("use the actual size
instead.\n");
> > +		}
> > +	}
> > +
> >  	if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
> >  		if (b->memory == V4L2_MEMORY_USERPTR) {
> >  			for (plane = 0; plane < vb->num_planes; ++plane) {
@@
> -1276,13
> > +1286,22 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb,
> const struct v4l2_buffer *b
> >  			 * userspace clearly never bothered to set it and
> >  			 * it's a safe assumption that they really meant to
> >  			 * use the full plane sizes.
> > +			 *
> > +			 * Some drivers, e.g. old codec drivers, use
> bytesused
> > +			 * == 0 as a way to indicate that streaming is
> finished.
> > +			 * In that case, the driver should use the
> > +			 * allow_zero_bytesused flag to keep old userspace
> > +			 * applications working.
> >  			 */
> >  			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;
> > +				if (vb->vb2_queue->allow_zero_bytesused)
> > +					pdst->bytesused = psrc->bytesused;
> > +				else
> > +					pdst->bytesused = psrc->bytesused ?
> > +						psrc->bytesused :
pdst->length;
> >  				pdst->data_offset = psrc->data_offset;
> >  			}
> >  		}
> > @@ -1295,6 +1314,11 @@ static void __fill_vb2_buffer(struct
> vb2_buffer *vb, const struct v4l2_buffer *b
> >  		 *
> >  		 * If bytesused == 0 for the output buffer, then fall back
> >  		 * to the full buffer size as that's a sensible default.
> > +		 *
> > +		 * Some drivers, e.g. old codec drivers, use bytesused * ==
> 0 as
> 
> Small typo:
> 
> s/bytesused * == 0/bytesused == 0/
> 
> > +		 * a way to indicate that streaming is finished. In that
> case,
> > +		 * the driver should use the allow_zero_bytesused flag to
> keep
> > +		 * old userspace applications working.
> >  		 */
> >  		if (b->memory == V4L2_MEMORY_USERPTR) {
> >  			v4l2_planes[0].m.userptr = b->m.userptr; @@ -1306,10
> +1330,13 @@
> > static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct
> v4l2_buffer *b
> >  			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
> > +		if (V4L2_TYPE_IS_OUTPUT(b->type)) {
> > +			if (vb->vb2_queue->allow_zero_bytesused)
> > +				v4l2_planes[0].bytesused = b->bytesused;
> > +			else
> > +				v4l2_planes[0].bytesused = b->bytesused ?
> > +					b->bytesused :
v4l2_planes[0].length;
> > +		} else
> >  			v4l2_planes[0].bytesused = 0;
> >
> >  	}
> > diff --git a/include/media/videobuf2-core.h
> > b/include/media/videobuf2-core.h index e49dc6b..a5790fd 100644
> > --- a/include/media/videobuf2-core.h
> > +++ b/include/media/videobuf2-core.h
> > @@ -337,6 +337,7 @@ struct v4l2_fh;
> >   * @io_modes:	supported io methods (see vb2_io_modes enum)
> >   * @fileio_read_once:		report EOF after reading the first
> buffer
> >   * @fileio_write_immediately:	queue buffer after each write() call
> > + * @allow_zero_bytesused:	allow bytesused == 0 to be passed to the
> driver
> >   * @lock:	pointer to a mutex that protects the vb2_queue
> struct. The
> >   *		driver can set this to a mutex to let the v4l2 core
> serialize
> >   *		the queuing ioctls. If the driver wants to handle locking
> > @@ -388,6 +389,7 @@ struct vb2_queue {
> >  	unsigned int			io_modes;
> >  	unsigned			fileio_read_once:1;
> >  	unsigned			fileio_write_immediately:1;
> > +	unsigned			allow_zero_bytesused:1;
> >
> >  	struct mutex			*lock;
> >  	struct v4l2_fh			*owner;
> >
> 
> Regards,
> 
> 	Hans

Best wishes,
-- 
Kamil Debski
Samsung R&D Institute Poland

Re: [PATCH v5 2/4] vb2: add allow_zero_bytesused flag to the vb2_queue struct

[Hans Verkuil]

On 02/23/2015 12:58 PM, Kamil Debski wrote:
> Hi,
> 
> Thank you for the review :)
> 
>> From: Hans Verkuil [mailto:hverkuil@xs4all.nl]
>> Sent: Friday, February 20, 2015 5:52 PM
>>
>> Hi Kamil,
>>
>> One question and one typo below...
>>
>> On 02/20/2015 05:38 PM, Kamil Debski wrote:
>>> The vb2: fix bytesused == 0 handling (8a75ffb) patch changed the
>>> behavior of __fill_vb2_buffer function, so that if bytesused is 0 it
>>> is set to the size of the buffer. However, bytesused set to 0 is used
>>> by older codec drivers as as indication used to mark the end of
>> stream.
>>>
>>> To keep backward compatibility, this patch adds a flag passed to the
>>> vb2_queue_init function - allow_zero_bytesused. If the flag is set
>>> upon initialization of the queue, the videobuf2 keeps the value of
>>> bytesused intact in the OUTPUT queue and passes it to the driver.
>>>
>>> Reported-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>> Signed-off-by: Kamil Debski <k.debski@samsung.com>
>>> ---
>>>  drivers/media/v4l2-core/videobuf2-core.c |   39
>> +++++++++++++++++++++++++-----
>>>  include/media/videobuf2-core.h           |    2 ++
>>>  2 files changed, 35 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
>>> b/drivers/media/v4l2-core/videobuf2-core.c
>>> index 5cd60bf..5d77670 100644
>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>>> @@ -1247,6 +1247,16 @@ static void __fill_vb2_buffer(struct
>> vb2_buffer
>>> *vb, const struct v4l2_buffer *b  {
>>>  	unsigned int plane;
>>>
>>> +	if (V4L2_TYPE_IS_OUTPUT(b->type)) {
>>> +		if (WARN_ON_ONCE(b->bytesused == 0)) {
>>> +			pr_warn_once("use of bytesused == 0 is deprecated
> and
>> will be
>>> +removed in 2017,\n");
>>
>> I wonder if we should give a specific year, or just say 'in the
>> future'.
>>
>> What do you think?
> 
> I think I would prefer to use the phrase "in the future". 
> If you are ok with that I will post an updated patch set soon.

I agree with that.

Regards,

	Hans

> 
>>
>>> +			if (vb->vb2_queue->allow_zero_bytesused)
>>> +				pr_warn_once("use
>> VIDIOC_DECODER_CMD(V4L2_DEC_CMD_STOP) instead.\n");
>>> +			else
>>> +				pr_warn_once("use the actual size
> instead.\n");
>>> +		}
>>> +	}
>>> +
>>>  	if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
>>>  		if (b->memory == V4L2_MEMORY_USERPTR) {
>>>  			for (plane = 0; plane < vb->num_planes; ++plane) {
> @@
>> -1276,13
>>> +1286,22 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb,
>> const struct v4l2_buffer *b
>>>  			 * userspace clearly never bothered to set it and
>>>  			 * it's a safe assumption that they really meant to
>>>  			 * use the full plane sizes.
>>> +			 *
>>> +			 * Some drivers, e.g. old codec drivers, use
>> bytesused
>>> +			 * == 0 as a way to indicate that streaming is
>> finished.
>>> +			 * In that case, the driver should use the
>>> +			 * allow_zero_bytesused flag to keep old userspace
>>> +			 * applications working.
>>>  			 */
>>>  			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;
>>> +				if (vb->vb2_queue->allow_zero_bytesused)
>>> +					pdst->bytesused = psrc->bytesused;
>>> +				else
>>> +					pdst->bytesused = psrc->bytesused ?
>>> +						psrc->bytesused :
> pdst->length;
>>>  				pdst->data_offset = psrc->data_offset;
>>>  			}
>>>  		}
>>> @@ -1295,6 +1314,11 @@ static void __fill_vb2_buffer(struct
>> vb2_buffer *vb, const struct v4l2_buffer *b
>>>  		 *
>>>  		 * If bytesused == 0 for the output buffer, then fall back
>>>  		 * to the full buffer size as that's a sensible default.
>>> +		 *
>>> +		 * Some drivers, e.g. old codec drivers, use bytesused * ==
>> 0 as
>>
>> Small typo:
>>
>> s/bytesused * == 0/bytesused == 0/
>>
>>> +		 * a way to indicate that streaming is finished. In that
>> case,
>>> +		 * the driver should use the allow_zero_bytesused flag to
>> keep
>>> +		 * old userspace applications working.
>>>  		 */
>>>  		if (b->memory == V4L2_MEMORY_USERPTR) {
>>>  			v4l2_planes[0].m.userptr = b->m.userptr; @@ -1306,10
>> +1330,13 @@
>>> static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct
>> v4l2_buffer *b
>>>  			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
>>> +		if (V4L2_TYPE_IS_OUTPUT(b->type)) {
>>> +			if (vb->vb2_queue->allow_zero_bytesused)
>>> +				v4l2_planes[0].bytesused = b->bytesused;
>>> +			else
>>> +				v4l2_planes[0].bytesused = b->bytesused ?
>>> +					b->bytesused :
> v4l2_planes[0].length;
>>> +		} else
>>>  			v4l2_planes[0].bytesused = 0;
>>>
>>>  	}
>>> diff --git a/include/media/videobuf2-core.h
>>> b/include/media/videobuf2-core.h index e49dc6b..a5790fd 100644
>>> --- a/include/media/videobuf2-core.h
>>> +++ b/include/media/videobuf2-core.h
>>> @@ -337,6 +337,7 @@ struct v4l2_fh;
>>>   * @io_modes:	supported io methods (see vb2_io_modes enum)
>>>   * @fileio_read_once:		report EOF after reading the first
>> buffer
>>>   * @fileio_write_immediately:	queue buffer after each write() call
>>> + * @allow_zero_bytesused:	allow bytesused == 0 to be passed to the
>> driver
>>>   * @lock:	pointer to a mutex that protects the vb2_queue
>> struct. The
>>>   *		driver can set this to a mutex to let the v4l2 core
>> serialize
>>>   *		the queuing ioctls. If the driver wants to handle locking
>>> @@ -388,6 +389,7 @@ struct vb2_queue {
>>>  	unsigned int			io_modes;
>>>  	unsigned			fileio_read_once:1;
>>>  	unsigned			fileio_write_immediately:1;
>>> +	unsigned			allow_zero_bytesused:1;
>>>
>>>  	struct mutex			*lock;
>>>  	struct v4l2_fh			*owner;
>>>
>>
>> Regards,
>>
>> 	Hans
> 
> Best wishes,
>