[PATCH 1/2] vb2: Add VB2_FILEIO_ALLOW_ZERO_BYTESUSED flag to vb2_fileio_flags

[PATCH 1/2] vb2: Add VB2_FILEIO_ALLOW_ZERO_BYTESUSED flag to vb2_fileio_flags

[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 - VB2_FILEIO_ALLOW_ZERO_BYTESUSED. If the flag is
set upon initialization of the queue, the videobuf2 keeps the value of
bytesused intact 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 |   33 ++++++++++++++++++++++++------
 include/media/videobuf2-core.h           |    3 +++
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index d09a891..1068dbb 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1276,13 +1276,23 @@ 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 following
+			 * io_flag VB2_FILEIO_ALLOW_ZERO_BYTESUSED 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->io_flags &
+					VB2_FILEIO_ALLOW_ZERO_BYTESUSED)
+					pdst->bytesused = psrc->bytesused;
+				else
+					pdst->bytesused = psrc->bytesused ?
+						psrc->bytesused : pdst->length;
 				pdst->data_offset = psrc->data_offset;
 			}
 		}
@@ -1295,6 +1305,12 @@ 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 following io_flag
+		 * VB2_FILEIO_ALLOW_ZERO_BYTESUSED to keep old userspace
+		 * applications working.
 		 */
 		if (b->memory == V4L2_MEMORY_USERPTR) {
 			v4l2_planes[0].m.userptr = b->m.userptr;
@@ -1306,11 +1322,16 @@ 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->io_flags &
+				VB2_FILEIO_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 bd2cec2..0540bc3 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -138,10 +138,13 @@ enum vb2_io_modes {
  * 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
+ * @VB2_FILEIO_ALLOW_ZERO_BYTESUSED:	the driver setting this flag will handle
+ *					bytesused == 0 as a special case
  */
 enum vb2_fileio_flags {
 	VB2_FILEIO_READ_ONCE		= (1 << 0),
 	VB2_FILEIO_WRITE_IMMEDIATELY	= (1 << 1),
+	VB2_FILEIO_ALLOW_ZERO_BYTESUSED	= (1 << 2),
 };
 
 /**
-- 
1.7.9.5

[PATCH 2/2] s5p-mfc: use VB2_FILEIO_ALLOW_ZERO_BYTESUSED flag

[Kamil Debski]

The 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 VB2_FILEIO_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 |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index 03204fd..89c148b 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -820,6 +820,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 VB2_FILEIO_ALLOW_ZERO_BYTESUSED flag so
+	 * that videobuf2 will keep the value of bytesused intact.
+	 */
+	q->io_flags = VB2_FILEIO_ALLOW_ZERO_BYTESUSED;
 	q->mem_ops = &vb2_dma_contig_memops;
 	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
 	ret = vb2_queue_init(q);
@@ -842,6 +849,10 @@ static int s5p_mfc_open(struct file *file)
 		ret = -ENOENT;
 		goto err_queue_init;
 	}
+	/* Set the VB2_FILEIO_ALLOW_ZERO_BYTESUSED flag, for more information
+	 * please see the comment above.
+	 */
+	q->io_flags = VB2_FILEIO_ALLOW_ZERO_BYTESUSED;
 	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 1/2] vb2: Add VB2_FILEIO_ALLOW_ZERO_BYTESUSED flag to vb2_fileio_flags

[Jean-Michel Hautbois]

Hi Kamil,

2014-12-16 12:36 GMT+01:00 Kamil Debski <k.debski@samsung.com>:
> 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 - VB2_FILEIO_ALLOW_ZERO_BYTESUSED. If the flag is
> set upon initialization of the queue, the videobuf2 keeps the value of
> bytesused intact and passes it to the driver.

Nice, this is something we were planning to do :).
But I would split this patch and the second which is specific to
s5p-mfc as this is core specific and should be independant.


> Reported-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Signed-off-by: Kamil Debski <k.debski@samsung.com>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c |   33 ++++++++++++++++++++++++------
>  include/media/videobuf2-core.h           |    3 +++
>  2 files changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index d09a891..1068dbb 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1276,13 +1276,23 @@ 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 following
> +                        * io_flag VB2_FILEIO_ALLOW_ZERO_BYTESUSED to keep old
> +                        * userspace applications working.

Not sure if this comment is necessary, as this is already in the commit ?

>                          */
>                         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->io_flags &
> +                                       VB2_FILEIO_ALLOW_ZERO_BYTESUSED)
> +                                       pdst->bytesused = psrc->bytesused;
> +                               else
> +                                       pdst->bytesused = psrc->bytesused ?
> +                                               psrc->bytesused : pdst->length;
>                                 pdst->data_offset = psrc->data_offset;
>                         }
>                 }
> @@ -1295,6 +1305,12 @@ 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 following io_flag
> +                * VB2_FILEIO_ALLOW_ZERO_BYTESUSED to keep old userspace
> +                * applications working.

Again, not sure this is useful.

>                  */
>                 if (b->memory == V4L2_MEMORY_USERPTR) {
>                         v4l2_planes[0].m.userptr = b->m.userptr;
> @@ -1306,11 +1322,16 @@ 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->io_flags &
> +                               VB2_FILEIO_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 bd2cec2..0540bc3 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -138,10 +138,13 @@ enum vb2_io_modes {
>   * 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
> + * @VB2_FILEIO_ALLOW_ZERO_BYTESUSED:   the driver setting this flag will handle
> + *                                     bytesused == 0 as a special case
>   */
>  enum vb2_fileio_flags {
>         VB2_FILEIO_READ_ONCE            = (1 << 0),
>         VB2_FILEIO_WRITE_IMMEDIATELY    = (1 << 1),
> +       VB2_FILEIO_ALLOW_ZERO_BYTESUSED = (1 << 2),
>  };
>
>  /**
> --
> 1.7.9.5
>
> --
> 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

I tested it with your coda patch too on i.MX6, thank you, this was annoying :).
JM

RE: [PATCH 1/2] vb2: Add VB2_FILEIO_ALLOW_ZERO_BYTESUSED flag to vb2_fileio_flags

[Kamil Debski]

Hi Jean,

> From: Jean-Michel Hautbois [mailto:jhautbois@gmail.com]
> Sent: Friday, December 19, 2014 3:36 PM
> To: Kamil Debski
> Cc: Linux Media Mailing List; m.szyprowski@samsung.com; Hans Verkuil;
> Nicolas Dufresne
> Subject: Re: [PATCH 1/2] vb2: Add VB2_FILEIO_ALLOW_ZERO_BYTESUSED flag
> to vb2_fileio_flags
> 
> Hi Kamil,
> 
> 2014-12-16 12:36 GMT+01:00 Kamil Debski <k.debski@samsung.com>:
> > 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 - VB2_FILEIO_ALLOW_ZERO_BYTESUSED. If the
> flag
> > is set upon initialization of the queue, the videobuf2 keeps the
> value
> > of bytesused intact and passes it to the driver.
> 
> Nice, this is something we were planning to do :).
> But I would split this patch and the second which is specific to s5p-
> mfc as this is core specific and should be independant.

This patch contains only changes in the videobuf2-core.c file. The next
patch in the series contains changes in the s5p-mfc driver. There is 
another patch sent today that adds this flag to coda. 

These are all separate patches, two of them are in a single patchset.
Actually, I would send all of them in one patchset, but initially I missed
that the coda driver should also have this change applied. (Nicolas, thank
you for reminding me to do this on IRC).

> 
> 
> > Reported-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > Signed-off-by: Kamil Debski <k.debski@samsung.com>
> > ---
> >  drivers/media/v4l2-core/videobuf2-core.c |   33
> ++++++++++++++++++++++++------
> >  include/media/videobuf2-core.h           |    3 +++
> >  2 files changed, 30 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> > b/drivers/media/v4l2-core/videobuf2-core.c
> > index d09a891..1068dbb 100644
> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -1276,13 +1276,23 @@ 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
> following
> > +                        * io_flag VB2_FILEIO_ALLOW_ZERO_BYTESUSED to
> keep old
> > +                        * userspace applications working.
> 
> Not sure if this comment is necessary, as this is already in the
> commit ?

The comment were present in the file already, I expanded them to cover the
exception when the VB2_FILEIO_ALLOW_ZERO_BYTESUSED flag is set.
It is also explained in the commit, I agree, but in the end one usually
looks in the source code. 

> 
> >                          */
> >                         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->io_flags &
> > +
> VB2_FILEIO_ALLOW_ZERO_BYTESUSED)
> > +                                       pdst->bytesused = psrc-
> >bytesused;
> > +                               else
> > +                                       pdst->bytesused = psrc-
> >bytesused ?
> > +                                               psrc->bytesused :
> > + pdst->length;
> >                                 pdst->data_offset = psrc->data_offset;
> >                         }
> >                 }
> > @@ -1295,6 +1305,12 @@ 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 following io_flag
> > +                * VB2_FILEIO_ALLOW_ZERO_BYTESUSED to keep old
> userspace
> > +                * applications working.
> 
> Again, not sure this is useful.

Same applies here, I expanded the comment to cover a new case. 

> 
> >                  */
> >                 if (b->memory == V4L2_MEMORY_USERPTR) {
> >                         v4l2_planes[0].m.userptr = b->m.userptr; @@
> > -1306,11 +1322,16 @@ 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->io_flags &
> > +                               VB2_FILEIO_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 bd2cec2..0540bc3 100644
> > --- a/include/media/videobuf2-core.h
> > +++ b/include/media/videobuf2-core.h
> > @@ -138,10 +138,13 @@ enum vb2_io_modes {
> >   * 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
> > + * @VB2_FILEIO_ALLOW_ZERO_BYTESUSED:   the driver setting this flag
> will handle
> > + *                                     bytesused == 0 as a special
> case
> >   */
> >  enum vb2_fileio_flags {
> >         VB2_FILEIO_READ_ONCE            = (1 << 0),
> >         VB2_FILEIO_WRITE_IMMEDIATELY    = (1 << 1),
> > +       VB2_FILEIO_ALLOW_ZERO_BYTESUSED = (1 << 2),
> >  };
> >
> >  /**
> > --
> > 1.7.9.5
> >
> > --
> > 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
> 
> I tested it with your coda patch too on i.MX6, thank you, this was
> annoying :).
> JM

You're welcome. Thanks for testing :) 

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

Re: [PATCH 1/2] vb2: Add VB2_FILEIO_ALLOW_ZERO_BYTESUSED flag to vb2_fileio_flags

[Jean-Michel Hautbois]

2014-12-19 17:03 GMT+01:00 Kamil Debski <k.debski@samsung.com>:
> Hi Jean,
>
>> From: Jean-Michel Hautbois [mailto:jhautbois@gmail.com]
>> Sent: Friday, December 19, 2014 3:36 PM
>> To: Kamil Debski
>> Cc: Linux Media Mailing List; m.szyprowski@samsung.com; Hans Verkuil;
>> Nicolas Dufresne
>> Subject: Re: [PATCH 1/2] vb2: Add VB2_FILEIO_ALLOW_ZERO_BYTESUSED flag
>> to vb2_fileio_flags
>>
>> Hi Kamil,
>>
>> 2014-12-16 12:36 GMT+01:00 Kamil Debski <k.debski@samsung.com>:
>> > 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 - VB2_FILEIO_ALLOW_ZERO_BYTESUSED. If the
>> flag
>> > is set upon initialization of the queue, the videobuf2 keeps the
>> value
>> > of bytesused intact and passes it to the driver.
>>
>> Nice, this is something we were planning to do :).
>> But I would split this patch and the second which is specific to s5p-
>> mfc as this is core specific and should be independant.
>
> This patch contains only changes in the videobuf2-core.c file. The next
> patch in the series contains changes in the s5p-mfc driver. There is
> another patch sent today that adds this flag to coda.
>
> These are all separate patches, two of them are in a single patchset.
> Actually, I would send all of them in one patchset, but initially I missed
> that the coda driver should also have this change applied. (Nicolas, thank
> you for reminding me to do this on IRC).

OK, This is why I have been confused. Well, I still think that core
modification should be a separate patch, and maybe s5f-mpc and coda be
in the same patchset. Not a problem.

>>
>>
>> > Reported-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>> > Signed-off-by: Kamil Debski <k.debski@samsung.com>
>> > ---
>> >  drivers/media/v4l2-core/videobuf2-core.c |   33
>> ++++++++++++++++++++++++------
>> >  include/media/videobuf2-core.h           |    3 +++
>> >  2 files changed, 30 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c
>> > b/drivers/media/v4l2-core/videobuf2-core.c
>> > index d09a891..1068dbb 100644
>> > --- a/drivers/media/v4l2-core/videobuf2-core.c
>> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> > @@ -1276,13 +1276,23 @@ 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
>> following
>> > +                        * io_flag VB2_FILEIO_ALLOW_ZERO_BYTESUSED to
>> keep old
>> > +                        * userspace applications working.
>>
>> Not sure if this comment is necessary, as this is already in the
>> commit ?
>
> The comment were present in the file already, I expanded them to cover the
> exception when the VB2_FILEIO_ALLOW_ZERO_BYTESUSED flag is set.
> It is also explained in the commit, I agree, but in the end one usually
> looks in the source code.

OK

>>
>> >                          */
>> >                         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->io_flags &
>> > +
>> VB2_FILEIO_ALLOW_ZERO_BYTESUSED)
>> > +                                       pdst->bytesused = psrc-
>> >bytesused;
>> > +                               else
>> > +                                       pdst->bytesused = psrc-
>> >bytesused ?
>> > +                                               psrc->bytesused :
>> > + pdst->length;
>> >                                 pdst->data_offset = psrc->data_offset;
>> >                         }
>> >                 }
>> > @@ -1295,6 +1305,12 @@ 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 following io_flag
>> > +                * VB2_FILEIO_ALLOW_ZERO_BYTESUSED to keep old
>> userspace
>> > +                * applications working.
>>
>> Again, not sure this is useful.
>
> Same applies here, I expanded the comment to cover a new case.
>
>>
>> >                  */
>> >                 if (b->memory == V4L2_MEMORY_USERPTR) {
>> >                         v4l2_planes[0].m.userptr = b->m.userptr; @@
>> > -1306,11 +1322,16 @@ 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->io_flags &
>> > +                               VB2_FILEIO_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 bd2cec2..0540bc3 100644
>> > --- a/include/media/videobuf2-core.h
>> > +++ b/include/media/videobuf2-core.h
>> > @@ -138,10 +138,13 @@ enum vb2_io_modes {
>> >   * 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
>> > + * @VB2_FILEIO_ALLOW_ZERO_BYTESUSED:   the driver setting this flag
>> will handle
>> > + *                                     bytesused == 0 as a special
>> case
>> >   */
>> >  enum vb2_fileio_flags {
>> >         VB2_FILEIO_READ_ONCE            = (1 << 0),
>> >         VB2_FILEIO_WRITE_IMMEDIATELY    = (1 << 1),
>> > +       VB2_FILEIO_ALLOW_ZERO_BYTESUSED = (1 << 2),
>> >  };
>> >
>> >  /**
>> > --
>> > 1.7.9.5
>> >
>> > --
>> > 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
>>
>> I tested it with your coda patch too on i.MX6, thank you, this was
>> annoying :).
>> JM
>
> You're welcome. Thanks for testing :)
>
> Best wishes,

Thanks, and best wishes too.
JM

Re: [PATCH 1/2] vb2: Add VB2_FILEIO_ALLOW_ZERO_BYTESUSED flag to vb2_fileio_flags

[Hans Verkuil]

Hi Kamil,

On 12/16/14 12:36, 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 - VB2_FILEIO_ALLOW_ZERO_BYTESUSED. If the flag is
> set upon initialization of the queue, the videobuf2 keeps the value of
> bytesused intact and passes it to the driver.

What is the status of this patch series?

Note that io_flags is really the wrong place for this flag, it should
be io_modes. This flag has nothing to do with file I/O.

Regards,

	Hans

> 
> Reported-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Signed-off-by: Kamil Debski <k.debski@samsung.com>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c |   33 ++++++++++++++++++++++++------
>  include/media/videobuf2-core.h           |    3 +++
>  2 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index d09a891..1068dbb 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1276,13 +1276,23 @@ 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 following
> +			 * io_flag VB2_FILEIO_ALLOW_ZERO_BYTESUSED 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->io_flags &
> +					VB2_FILEIO_ALLOW_ZERO_BYTESUSED)
> +					pdst->bytesused = psrc->bytesused;
> +				else
> +					pdst->bytesused = psrc->bytesused ?
> +						psrc->bytesused : pdst->length;
>  				pdst->data_offset = psrc->data_offset;
>  			}
>  		}
> @@ -1295,6 +1305,12 @@ 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 following io_flag
> +		 * VB2_FILEIO_ALLOW_ZERO_BYTESUSED to keep old userspace
> +		 * applications working.
>  		 */
>  		if (b->memory == V4L2_MEMORY_USERPTR) {
>  			v4l2_planes[0].m.userptr = b->m.userptr;
> @@ -1306,11 +1322,16 @@ 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->io_flags &
> +				VB2_FILEIO_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 bd2cec2..0540bc3 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -138,10 +138,13 @@ enum vb2_io_modes {
>   * 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
> + * @VB2_FILEIO_ALLOW_ZERO_BYTESUSED:	the driver setting this flag will handle
> + *					bytesused == 0 as a special case
>   */
>  enum vb2_fileio_flags {
>  	VB2_FILEIO_READ_ONCE		= (1 << 0),
>  	VB2_FILEIO_WRITE_IMMEDIATELY	= (1 << 1),
> +	VB2_FILEIO_ALLOW_ZERO_BYTESUSED	= (1 << 2),
>  };
>  
>  /**
> 

RE: [PATCH 1/2] vb2: Add VB2_FILEIO_ALLOW_ZERO_BYTESUSED flag to vb2_fileio_flags

[Kamil Debski]

Hi Hans,

> From: Hans Verkuil [mailto:hverkuil@xs4all.nl]
> Sent: Tuesday, February 17, 2015 10:11 AM
> 
> Hi Kamil,
> 
> On 12/16/14 12:36, 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 - VB2_FILEIO_ALLOW_ZERO_BYTESUSED. If the
> flag
> > is set upon initialization of the queue, the videobuf2 keeps the
> value
> > of bytesused intact and passes it to the driver.
> 
> What is the status of this patch series?

I have to admit that I had forgotten a bit about this patch, because of
other
important work. Thanks for reminding me :)
 
> Note that io_flags is really the wrong place for this flag, it should
> be io_modes. This flag has nothing to do with file I/O.

What do you think about adding a separate flags field into the vb2_queue
structure? This could be combined with changing io_flags to u8 or a bit
field
to save space.

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

> 
> Regards,
> 
> 	Hans
> 
> >
> > Reported-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > Signed-off-by: Kamil Debski <k.debski@samsung.com>
> > ---
> >  drivers/media/v4l2-core/videobuf2-core.c |   33
> ++++++++++++++++++++++++------
> >  include/media/videobuf2-core.h           |    3 +++
> >  2 files changed, 30 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> > b/drivers/media/v4l2-core/videobuf2-core.c
> > index d09a891..1068dbb 100644
> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -1276,13 +1276,23 @@ 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 following
> > +			 * io_flag VB2_FILEIO_ALLOW_ZERO_BYTESUSED 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->io_flags &
> > +					VB2_FILEIO_ALLOW_ZERO_BYTESUSED)
> > +					pdst->bytesused = psrc->bytesused;
> > +				else
> > +					pdst->bytesused = psrc->bytesused ?
> > +						psrc->bytesused :
pdst->length;
> >  				pdst->data_offset = psrc->data_offset;
> >  			}
> >  		}
> > @@ -1295,6 +1305,12 @@ 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 following io_flag
> > +		 * VB2_FILEIO_ALLOW_ZERO_BYTESUSED to keep old userspace
> > +		 * applications working.
> >  		 */
> >  		if (b->memory == V4L2_MEMORY_USERPTR) {
> >  			v4l2_planes[0].m.userptr = b->m.userptr; @@ -1306,11
> +1322,16 @@
> > 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->io_flags &
> > +				VB2_FILEIO_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 bd2cec2..0540bc3 100644
> > --- a/include/media/videobuf2-core.h
> > +++ b/include/media/videobuf2-core.h
> > @@ -138,10 +138,13 @@ enum vb2_io_modes {
> >   * 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
> > + * @VB2_FILEIO_ALLOW_ZERO_BYTESUSED:	the driver setting this flag
> will handle
> > + *					bytesused == 0 as a special case
> >   */
> >  enum vb2_fileio_flags {
> >  	VB2_FILEIO_READ_ONCE		= (1 << 0),
> >  	VB2_FILEIO_WRITE_IMMEDIATELY	= (1 << 1),
> > +	VB2_FILEIO_ALLOW_ZERO_BYTESUSED	= (1 << 2),
> >  };
> >
> >  /**
> >

Re: [PATCH 1/2] vb2: Add VB2_FILEIO_ALLOW_ZERO_BYTESUSED flag to vb2_fileio_flags

[Hans Verkuil]

On 02/18/15 10:42, Kamil Debski wrote:
> Hi Hans,
> 
>> From: Hans Verkuil [mailto:hverkuil@xs4all.nl]
>> Sent: Tuesday, February 17, 2015 10:11 AM
>>
>> Hi Kamil,
>>
>> On 12/16/14 12:36, 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 - VB2_FILEIO_ALLOW_ZERO_BYTESUSED. If the
>> flag
>>> is set upon initialization of the queue, the videobuf2 keeps the
>> value
>>> of bytesused intact and passes it to the driver.
>>
>> What is the status of this patch series?
> 
> I have to admit that I had forgotten a bit about this patch, because of
> other
> important work. Thanks for reminding me :)
>  
>> Note that io_flags is really the wrong place for this flag, it should
>> be io_modes. This flag has nothing to do with file I/O.
> 
> What do you think about adding a separate flags field into the vb2_queue
> structure? This could be combined with changing io_flags to u8 or a bit
> field
> to save space.

I think changing io_flags to a bitfield is a good idea.

	unsigned fileio_read_once:1;
	unsigned fileio_write_immediately:1;
	unsigned allow_zero_bytesused:1;

However, going back to allow_zero_bytesused: this has been broken for
quite some time without anyone complaining (other than you :-) ). Might
it not be better to just fix this properly by calling V4L2_DEC_CMD_STOP,
as done here: https://www.mail-archive.com/linux-media@vger.kernel.org/msg84916.html,
and drop the support for zero bytesused to mark EOS entirely?

I might be too optimistic here. Or perhaps at least add a pr_warn telling
users to switch to V4L2_DEC_CMD_STOP since this will be removed in 2017 or
whatever.

Regards,

	Hans

RE: [PATCH 1/2] vb2: Add VB2_FILEIO_ALLOW_ZERO_BYTESUSED flag to vb2_fileio_flags

[Kamil Debski]

Hi,

> From: Hans Verkuil [mailto:hverkuil@xs4all.nl]
> Sent: Wednesday, February 18, 2015 10:58 AM
> 
> On 02/18/15 10:42, Kamil Debski wrote:
> > Hi Hans,
> >
> >> From: Hans Verkuil [mailto:hverkuil@xs4all.nl]
> >> Sent: Tuesday, February 17, 2015 10:11 AM
> >>
> >> Hi Kamil,
> >>
> >> On 12/16/14 12:36, 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 - VB2_FILEIO_ALLOW_ZERO_BYTESUSED. If the
> >> flag
> >>> is set upon initialization of the queue, the videobuf2 keeps the
> >> value
> >>> of bytesused intact and passes it to the driver.
> >>
> >> What is the status of this patch series?
> >
> > I have to admit that I had forgotten a bit about this patch, because
> > of other important work. Thanks for reminding me :)
> >
> >> Note that io_flags is really the wrong place for this flag, it
> should
> >> be io_modes. This flag has nothing to do with file I/O.
> >
> > What do you think about adding a separate flags field into the
> > vb2_queue structure? This could be combined with changing io_flags to
> > u8 or a bit field to save space.
> 
> I think changing io_flags to a bitfield is a good idea.
> 
> 	unsigned fileio_read_once:1;
> 	unsigned fileio_write_immediately:1;
> 	unsigned allow_zero_bytesused:1;
> 
> However, going back to allow_zero_bytesused: this has been broken for
> quite some time without anyone complaining (other than you :-) ). 

If I remember correctly, it was Nicolas who reported to me the problem 
on the IRC.

> Might
> it not be better to just fix this properly by calling
> V4L2_DEC_CMD_STOP, as done here: https://www.mail-archive.com/linux-
> media@vger.kernel.org/msg84916.html,
> and drop the support for zero bytesused to mark EOS entirely?

I think it would be good to have the backward compatibility for some time.

> I might be too optimistic here. Or perhaps at least add a pr_warn
> telling users to switch to V4L2_DEC_CMD_STOP since this will be removed
> in 2017 or whatever.

Where do you see the pr_warn? I guess it would be good if it was only
displayed once and only when the app uses bytesused == 0 to signal the
EOS. Do you think alike?


> 
> Regards,
> 
> 	Hans

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

Re: [PATCH 1/2] vb2: Add VB2_FILEIO_ALLOW_ZERO_BYTESUSED flag to vb2_fileio_flags

[Hans Verkuil]

On 02/18/15 11:31, Kamil Debski wrote:
> Hi,
> 
>> From: Hans Verkuil [mailto:hverkuil@xs4all.nl]
>> Sent: Wednesday, February 18, 2015 10:58 AM
>>
>> On 02/18/15 10:42, Kamil Debski wrote:
>>> Hi Hans,
>>>
>>>> From: Hans Verkuil [mailto:hverkuil@xs4all.nl]
>>>> Sent: Tuesday, February 17, 2015 10:11 AM
>>>>
>>>> Hi Kamil,
>>>>
>>>> On 12/16/14 12:36, 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 - VB2_FILEIO_ALLOW_ZERO_BYTESUSED. If the
>>>> flag
>>>>> is set upon initialization of the queue, the videobuf2 keeps the
>>>> value
>>>>> of bytesused intact and passes it to the driver.
>>>>
>>>> What is the status of this patch series?
>>>
>>> I have to admit that I had forgotten a bit about this patch, because
>>> of other important work. Thanks for reminding me :)
>>>
>>>> Note that io_flags is really the wrong place for this flag, it
>> should
>>>> be io_modes. This flag has nothing to do with file I/O.
>>>
>>> What do you think about adding a separate flags field into the
>>> vb2_queue structure? This could be combined with changing io_flags to
>>> u8 or a bit field to save space.
>>
>> I think changing io_flags to a bitfield is a good idea.
>>
>> 	unsigned fileio_read_once:1;
>> 	unsigned fileio_write_immediately:1;
>> 	unsigned allow_zero_bytesused:1;
>>
>> However, going back to allow_zero_bytesused: this has been broken for
>> quite some time without anyone complaining (other than you :-) ). 
> 
> If I remember correctly, it was Nicolas who reported to me the problem 
> on the IRC.
> 
>> Might
>> it not be better to just fix this properly by calling
>> V4L2_DEC_CMD_STOP, as done here: https://www.mail-archive.com/linux-
>> media@vger.kernel.org/msg84916.html,
>> and drop the support for zero bytesused to mark EOS entirely?
> 
> I think it would be good to have the backward compatibility for some time.
> 
>> I might be too optimistic here. Or perhaps at least add a pr_warn
>> telling users to switch to V4L2_DEC_CMD_STOP since this will be removed
>> in 2017 or whatever.
> 
> Where do you see the pr_warn? I guess it would be good if it was only
> displayed once and only when the app uses bytesused == 0 to signal the
> EOS. Do you think alike?

Right, pr_warn_once() would do that.

As I discussed with Pawel on irc I also think we need to do a pr_warn_once
if the application doesn't set bytesused for output streams. Right now we
replace bytesused by length to cater for bad apps, but I really hate that.

I'd like to eventually require a proper setting for bytesused, but that
means apps need to be informed that they do something wrong. Unfortunately,
other than gstreamer I would expect that all those apps are closed source
running on embedded systems.

Nicolas, can you confirm that gstreamer is filling in bytesused for output
buffers correctly? (I.e., doesn't leave it to 0). I may have mentioned this
before, but have you run gstreamer with valgrind? valgrind 3.10 now supports
all v4l2 ioctls.

Regards,

	Hans

> 
> 
>>
>> Regards,
>>
>> 	Hans
> 
> Best wishes,
>