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

[PATCH v6 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 v6 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..33a5d93 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 the future,\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 v6 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..329c2a4 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 way to indicate end-of-stream for coda is to set the
+	 * bytesused == 0. However by default videobuf2 handles 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);
 }
-- 
1.7.9.5

[PATCH v6 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..9fe4d90 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 way to indicate end-of-stream for MFC is to set the
+	 * bytesused == 0. However by default videobuf2 handles 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.
+	 */
+	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 v6 2/4] vb2: add allow_zero_bytesused flag to the vb2_queue struct

[Hans Verkuil]

On 02/23/2015 01:26 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>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Thanks!

	Hans

> ---
>  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..33a5d93 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 the future,\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;
> 

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

[Laurent Pinchart]

Hello,

While working on fixing occurrences of

[   54.375534] use of bytesused == 0 is deprecated and will be removed in the future,
[   54.388026] use the actual size instead.

in libcamera, I realized that the patch that initially introduced the
warning and deprecated setting bytesused to 0 didn't change the V4L2 API
specification, which still documents bytesused as

    [...] If the application sets this to 0 for an output stream, then
    bytesused will be set to the size of the buffer (see the length
    field of this struct) by the driver. [...]

for both v4l2_buffer and v4l2_plane.

This deprecated behaviour has been present in the kernel for 7 years and
a half now. I'm wondering if it's really deprecated, in which case we
should update the API specification, or if it should be considered
supported, in which case the warning should be dropped.

On Mon, Feb 23, 2015 at 01:26:17PM +0100, 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..33a5d93 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 the future,\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;

-- 
Regards,

Laurent Pinchart

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

[Hans Verkuil]

On 10/2/22 01:48, Laurent Pinchart wrote:
> Hello,
> 
> While working on fixing occurrences of
> 
> [   54.375534] use of bytesused == 0 is deprecated and will be removed in the future,
> [   54.388026] use the actual size instead.
> 
> in libcamera, I realized that the patch that initially introduced the
> warning and deprecated setting bytesused to 0 didn't change the V4L2 API
> specification, which still documents bytesused as
> 
>     [...] If the application sets this to 0 for an output stream, then
>     bytesused will be set to the size of the buffer (see the length
>     field of this struct) by the driver. [...]
> 
> for both v4l2_buffer and v4l2_plane.
> 
> This deprecated behaviour has been present in the kernel for 7 years and
> a half now. I'm wondering if it's really deprecated, in which case we
> should update the API specification, or if it should be considered
> supported, in which case the warning should be dropped.

It's a good question.

I do believe it should be removed from the spec. It is IMHO a bad idea to
just leave bytesused at 0 for an output buffer. Applications should be explicit.

But on the other hand, I think we need to keep the current behavior in the
kernel of replacing bytesused == 0 with the length of the buffer. I don't
think we can return an error in that case, it would almost certainly break
userspace.

Regarding the warning: I think we need to keep it, to warn applications that
what they are doing is a bad idea, but that the text should change from:

"use of bytesused == 0 is deprecated and will be removed in the future"

to:

"use of bytesused == 0 is not recommended"

Regards,

	Hans

> 
> On Mon, Feb 23, 2015 at 01:26:17PM +0100, 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..33a5d93 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 the future,\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;
> 

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

[Sakari Ailus]

Hi Hans, Laurent,

On Tue, Oct 04, 2022 at 11:12:45AM +0200, Hans Verkuil wrote:
> 
> 
> On 10/2/22 01:48, Laurent Pinchart wrote:
> > Hello,
> > 
> > While working on fixing occurrences of
> > 
> > [   54.375534] use of bytesused == 0 is deprecated and will be removed in the future,
> > [   54.388026] use the actual size instead.
> > 
> > in libcamera, I realized that the patch that initially introduced the
> > warning and deprecated setting bytesused to 0 didn't change the V4L2 API
> > specification, which still documents bytesused as
> > 
> >     [...] If the application sets this to 0 for an output stream, then
> >     bytesused will be set to the size of the buffer (see the length
> >     field of this struct) by the driver. [...]
> > 
> > for both v4l2_buffer and v4l2_plane.
> > 
> > This deprecated behaviour has been present in the kernel for 7 years and
> > a half now. I'm wondering if it's really deprecated, in which case we
> > should update the API specification, or if it should be considered
> > supported, in which case the warning should be dropped.
> 
> It's a good question.
> 
> I do believe it should be removed from the spec. It is IMHO a bad idea to
> just leave bytesused at 0 for an output buffer. Applications should be explicit.
> 
> But on the other hand, I think we need to keep the current behavior in the
> kernel of replacing bytesused == 0 with the length of the buffer. I don't
> think we can return an error in that case, it would almost certainly break
> userspace.
> 
> Regarding the warning: I think we need to keep it, to warn applications that
> what they are doing is a bad idea, but that the text should change from:
> 
> "use of bytesused == 0 is deprecated and will be removed in the future"
> 
> to:
> 
> "use of bytesused == 0 is not recommended"

If we ever intend to drop this support, we should warn we're going to do
so. Otherwise there's little point in recommending against using it. The
spec should document it as deprecated and to be removed in the future as
well. (Or alternatively, the warning should be removed altogether.)

-- 
Kind regards,

Sakari Ailus

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

[Laurent Pinchart]

Hello,

On Tue, Oct 04, 2022 at 12:23:24PM +0300, Sakari Ailus wrote:
> On Tue, Oct 04, 2022 at 11:12:45AM +0200, Hans Verkuil wrote:
> > On 10/2/22 01:48, Laurent Pinchart wrote:
> > > Hello,
> > > 
> > > While working on fixing occurrences of
> > > 
> > > [   54.375534] use of bytesused == 0 is deprecated and will be removed in the future,
> > > [   54.388026] use the actual size instead.
> > > 
> > > in libcamera, I realized that the patch that initially introduced the
> > > warning and deprecated setting bytesused to 0 didn't change the V4L2 API
> > > specification, which still documents bytesused as
> > > 
> > >     [...] If the application sets this to 0 for an output stream, then
> > >     bytesused will be set to the size of the buffer (see the length
> > >     field of this struct) by the driver. [...]
> > > 
> > > for both v4l2_buffer and v4l2_plane.
> > > 
> > > This deprecated behaviour has been present in the kernel for 7 years and
> > > a half now. I'm wondering if it's really deprecated, in which case we
> > > should update the API specification, or if it should be considered
> > > supported, in which case the warning should be dropped.
> > 
> > It's a good question.
> > 
> > I do believe it should be removed from the spec. It is IMHO a bad idea to
> > just leave bytesused at 0 for an output buffer. Applications should be explicit.

OK. I'll write a patch.

> > But on the other hand, I think we need to keep the current behavior in the
> > kernel of replacing bytesused == 0 with the length of the buffer. I don't
> > think we can return an error in that case, it would almost certainly break
> > userspace.

Yes, I don't think we can change the implementation for the time being,
the risk of breakages is just too high (I'm fixing the libcamera side
though).

> > Regarding the warning: I think we need to keep it, to warn applications that
> > what they are doing is a bad idea, but that the text should change from:
> > 
> > "use of bytesused == 0 is deprecated and will be removed in the future"
> > 
> > to:
> > 
> > "use of bytesused == 0 is not recommended"
> 
> If we ever intend to drop this support, we should warn we're going to do
> so. Otherwise there's little point in recommending against using it.

I agree. Just saying it's not recommended is pointless. Either we want
to deprecate this behaviour, which means that it may get removed in the
future (one option could be to WARN_ONCE() for a few years, although
even that may not be enough), or we conclude that removing it will never
be possible, in which case I'd drop the message.

> The
> spec should document it as deprecated and to be removed in the future as
> well. (Or alternatively, the warning should be removed altogether.)

I wouldn't document it at all, if it's deprecated it doesn't deserve a
mention in the spec. It's hard enough for people to understand how to do
the right thing when reading documentation without being told about the
things that work but shouldn't be done :-)

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

Hi Laurent,

On Tue, Oct 04, 2022 at 05:29:46PM +0300, Laurent Pinchart wrote:
> > If we ever intend to drop this support, we should warn we're going to do
> > so. Otherwise there's little point in recommending against using it.
> 
> I agree. Just saying it's not recommended is pointless. Either we want
> to deprecate this behaviour, which means that it may get removed in the
> future (one option could be to WARN_ONCE() for a few years, although
> even that may not be enough), or we conclude that removing it will never
> be possible, in which case I'd drop the message.

I think displaying a warning for a few seconds would do it. ;-) ;-)

> 
> > The
> > spec should document it as deprecated and to be removed in the future as
> > well. (Or alternatively, the warning should be removed altogether.)
> 
> I wouldn't document it at all, if it's deprecated it doesn't deserve a
> mention in the spec. It's hard enough for people to understand how to do
> the right thing when reading documentation without being told about the
> things that work but shouldn't be done :-)

I would document it as deprecated so that application developers reading it
could get the hint. Otherwise they won't (unless they look at the kernel
logs of course). Up to you, I don't have a strong opinion on this.

-- 
Kind regards,

Sakari Ailus

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

[Laurent Pinchart]

Hi Sakari,

On Tue, Oct 04, 2022 at 09:36:56PM +0300, Sakari Ailus wrote:
> On Tue, Oct 04, 2022 at 05:29:46PM +0300, Laurent Pinchart wrote:
> > > If we ever intend to drop this support, we should warn we're going to do
> > > so. Otherwise there's little point in recommending against using it.
> > 
> > I agree. Just saying it's not recommended is pointless. Either we want
> > to deprecate this behaviour, which means that it may get removed in the
> > future (one option could be to WARN_ONCE() for a few years, although
> > even that may not be enough), or we conclude that removing it will never
> > be possible, in which case I'd drop the message.
> 
> I think displaying a warning for a few seconds would do it. ;-) ;-)
> 
> > > The
> > > spec should document it as deprecated and to be removed in the future as
> > > well. (Or alternatively, the warning should be removed altogether.)
> > 
> > I wouldn't document it at all, if it's deprecated it doesn't deserve a
> > mention in the spec. It's hard enough for people to understand how to do
> > the right thing when reading documentation without being told about the
> > things that work but shouldn't be done :-)
> 
> I would document it as deprecated so that application developers reading it
> could get the hint. Otherwise they won't (unless they look at the kernel
> logs of course). Up to you, I don't have a strong opinion on this.

Why do you think that would be better than documenting the
non-deprecated behaviour only ? I doubt anyone would read the
documentation, realize that the feature is deprecated, and then go fix
an application. I may be wrong though, is that why you would like to see
a mention of the deprecated feature in the documentation ?

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

Hi Laurent,

On Tue, Oct 04, 2022 at 10:47:49PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Tue, Oct 04, 2022 at 09:36:56PM +0300, Sakari Ailus wrote:
> > On Tue, Oct 04, 2022 at 05:29:46PM +0300, Laurent Pinchart wrote:
> > > > If we ever intend to drop this support, we should warn we're going to do
> > > > so. Otherwise there's little point in recommending against using it.
> > > 
> > > I agree. Just saying it's not recommended is pointless. Either we want
> > > to deprecate this behaviour, which means that it may get removed in the
> > > future (one option could be to WARN_ONCE() for a few years, although
> > > even that may not be enough), or we conclude that removing it will never
> > > be possible, in which case I'd drop the message.
> > 
> > I think displaying a warning for a few seconds would do it. ;-) ;-)
> > 
> > > > The
> > > > spec should document it as deprecated and to be removed in the future as
> > > > well. (Or alternatively, the warning should be removed altogether.)
> > > 
> > > I wouldn't document it at all, if it's deprecated it doesn't deserve a
> > > mention in the spec. It's hard enough for people to understand how to do
> > > the right thing when reading documentation without being told about the
> > > things that work but shouldn't be done :-)
> > 
> > I would document it as deprecated so that application developers reading it
> > could get the hint. Otherwise they won't (unless they look at the kernel
> > logs of course). Up to you, I don't have a strong opinion on this.
> 
> Why do you think that would be better than documenting the
> non-deprecated behaviour only ? I doubt anyone would read the
> documentation, realize that the feature is deprecated, and then go fix
> an application. I may be wrong though, is that why you would like to see
> a mention of the deprecated feature in the documentation ?

Yes. I agree the likelihood of that happening is not very big presumably.
As noted, I don't have a strong opimion on this.

On the other hand, I'm certain the other option I proposed above would be
far more efficient in having applications fixed.

-- 
Regards,

Sakari Ailus