[RFC PATCH 0/2] Signalling last decoded frame by V4L2_BUF_FLAG_LAST and -EPIPE

[RFC PATCH 0/2] Signalling last decoded frame by V4L2_BUF_FLAG_LAST and -EPIPE

[Philipp Zabel]

At the V4L2 codec API session during ELC-E 2014, we agreed that for the decoder
draining flow, after a V4L2_DEC_CMD_STOP decoder command was issued, the last
decoded buffer should get dequeued with a V4L2_BUF_FLAG_LAST set. After that,
poll should immediately return and all following VIDIOC_DQBUF should return
-EPIPE until the stream is stopped or decoding continued via V4L2_DEC_CMD_START.
(or STREAMOFF/STREAMON).

regards
Philipp

Peter Seiderer (1):
  [media] videodev2: Add V4L2_BUF_FLAG_LAST

Philipp Zabel (1):
  [media] videobuf2: return -EPIPE from DQBUF after the last buffer

 drivers/media/v4l2-core/v4l2-mem2mem.c   | 10 +++++++++-
 drivers/media/v4l2-core/videobuf2-core.c | 18 +++++++++++++++++-
 include/media/videobuf2-core.h           |  1 +
 include/uapi/linux/videodev2.h           |  2 ++
 4 files changed, 29 insertions(+), 2 deletions(-)

-- 
2.1.4

[RFC PATCH 1/2] [media] videodev2: Add V4L2_BUF_FLAG_LAST

[Philipp Zabel]

From: Peter Seiderer <ps.report@gmx.net>

This v4l2_buffer flag can be used by drivers to mark a capture buffer
as the last generated buffer, for example after a V4L2_DEC_CMD_STOP
command was issued.

Signed-off-by: Peter Seiderer <ps.report@gmx.net>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 include/uapi/linux/videodev2.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index fbdc360..c642c10 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -809,6 +809,8 @@ struct v4l2_buffer {
 #define V4L2_BUF_FLAG_TSTAMP_SRC_MASK		0x00070000
 #define V4L2_BUF_FLAG_TSTAMP_SRC_EOF		0x00000000
 #define V4L2_BUF_FLAG_TSTAMP_SRC_SOE		0x00010000
+/* mem2mem encoder/decoder */
+#define V4L2_BUF_FLAG_LAST			0x00100000
 
 /**
  * struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor
-- 
2.1.4

Re: [RFC PATCH 1/2] [media] videodev2: Add V4L2_BUF_FLAG_LAST

[Hans Verkuil]

On 01/22/2015 12:28 PM, Philipp Zabel wrote:
> From: Peter Seiderer <ps.report@gmx.net>
> 
> This v4l2_buffer flag can be used by drivers to mark a capture buffer
> as the last generated buffer, for example after a V4L2_DEC_CMD_STOP
> command was issued.
> 
> Signed-off-by: Peter Seiderer <ps.report@gmx.net>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  include/uapi/linux/videodev2.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index fbdc360..c642c10 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -809,6 +809,8 @@ struct v4l2_buffer {
>  #define V4L2_BUF_FLAG_TSTAMP_SRC_MASK		0x00070000
>  #define V4L2_BUF_FLAG_TSTAMP_SRC_EOF		0x00000000
>  #define V4L2_BUF_FLAG_TSTAMP_SRC_SOE		0x00010000
> +/* mem2mem encoder/decoder */
> +#define V4L2_BUF_FLAG_LAST			0x00100000
>  
>  /**
>   * struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor
> 

You probably knew this, but this should of course be documented in the V4L2
spec. In particular the spec should be clear about *when* the flag is set.

Also, any drivers that need this should be updated as well. Otherwise applications
cannot rely on it.

Regards,

	Hans

[RFC PATCH 2/2] [media] videobuf2: return -EPIPE from DQBUF after the last buffer

[Philipp Zabel]

If the last buffer was dequeued from a capture queue, let poll return
immediately and let DQBUF return -EPIPE to signal there will no more
buffers to dequeue until STREAMOFF.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
TODO: (How) should the last_buffer_dequeud flag be cleared in reaction to
V4L2_DEC_CMD_START?
---
 drivers/media/v4l2-core/v4l2-mem2mem.c   | 10 +++++++++-
 drivers/media/v4l2-core/videobuf2-core.c | 18 +++++++++++++++++-
 include/media/videobuf2-core.h           |  1 +
 3 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 80c588f..1b5b432 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -564,8 +564,16 @@ unsigned int v4l2_m2m_poll(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
 
 	if (list_empty(&src_q->done_list))
 		poll_wait(file, &src_q->done_wq, wait);
-	if (list_empty(&dst_q->done_list))
+	if (list_empty(&dst_q->done_list)) {
+		/*
+		 * If the last buffer was dequeued from the capture queue,
+		 * return immediately. DQBUF will return -EPIPE.
+		 */
+		if (dst_q->last_buffer_dequeued)
+			return rc | POLLIN | POLLRDNORM;
+
 		poll_wait(file, &dst_q->done_wq, wait);
+	}
 
 	if (m2m_ctx->m2m_dev->m2m_ops->lock)
 		m2m_ctx->m2m_dev->m2m_ops->lock(m2m_ctx->priv);
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index d09a891..c2c2eac 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -2046,6 +2046,10 @@ static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool n
 	struct vb2_buffer *vb = NULL;
 	int ret;
 
+	if (q->last_buffer_dequeued) {
+		dprintk(3, "last buffer dequeued already\n");
+		return -EPIPE;
+	}
 	if (b->type != q->type) {
 		dprintk(1, "invalid buffer type\n");
 		return -EINVAL;
@@ -2073,6 +2077,9 @@ static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool n
 	/* Remove from videobuf queue */
 	list_del(&vb->queued_entry);
 	q->queued_count--;
+	if (!V4L2_TYPE_IS_OUTPUT(q->type) &&
+	    vb->v4l2_buf.flags & V4L2_BUF_FLAG_LAST)
+		q->last_buffer_dequeued = true;
 	/* go back to dequeued state */
 	__vb2_dqbuf(vb);
 
@@ -2286,6 +2293,7 @@ static int vb2_internal_streamoff(struct vb2_queue *q, enum v4l2_buf_type type)
 	 */
 	__vb2_queue_cancel(q);
 	q->waiting_for_buffers = !V4L2_TYPE_IS_OUTPUT(q->type);
+	q->last_buffer_dequeued = false;
 
 	dprintk(3, "successful\n");
 	return 0;
@@ -2628,8 +2636,16 @@ unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
 	if (V4L2_TYPE_IS_OUTPUT(q->type) && q->queued_count < q->num_buffers)
 		return res | POLLOUT | POLLWRNORM;
 
-	if (list_empty(&q->done_list))
+	if (list_empty(&q->done_list)) {
+		/*
+		 * If the last buffer was dequeued from a capture queue,
+		 * return immediately. DQBUF will return -EPIPE.
+		 */
+		if (!V4L2_TYPE_IS_OUTPUT(q->type) && q->last_buffer_dequeued)
+			return res | POLLIN | POLLRDNORM;
+
 		poll_wait(file, &q->done_wq, wait);
+	}
 
 	/*
 	 * Take first buffer available for dequeuing.
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index bd2cec2..ca337bf 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -429,6 +429,7 @@ struct vb2_queue {
 	unsigned int			start_streaming_called:1;
 	unsigned int			error:1;
 	unsigned int			waiting_for_buffers:1;
+	unsigned int			last_buffer_dequeued:1;
 
 	struct vb2_fileio_data		*fileio;
 	struct vb2_threadio_data	*threadio;
-- 
2.1.4

Re: [RFC PATCH 2/2] [media] videobuf2: return -EPIPE from DQBUF after the last buffer

[Hans Verkuil]

On 01/22/2015 12:28 PM, Philipp Zabel wrote:
> If the last buffer was dequeued from a capture queue, let poll return
> immediately and let DQBUF return -EPIPE to signal there will no more
> buffers to dequeue until STREAMOFF.

This looks OK to me, although I would like to see comments from others as well.
Of course, this needs to be documented in the spec as well.

> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> TODO: (How) should the last_buffer_dequeud flag be cleared in reaction to
> V4L2_DEC_CMD_START?

I would suggest an inline function in videobuf2-core.h that clears the flag
and that drivers can call. I don't think the vb2 core can detect when it is
OK to clear the flag, it needs to be told by the driver (correct me if I am
wrong).

Regards,

	Hans

> ---
>  drivers/media/v4l2-core/v4l2-mem2mem.c   | 10 +++++++++-
>  drivers/media/v4l2-core/videobuf2-core.c | 18 +++++++++++++++++-
>  include/media/videobuf2-core.h           |  1 +
>  3 files changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index 80c588f..1b5b432 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> @@ -564,8 +564,16 @@ unsigned int v4l2_m2m_poll(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
>  
>  	if (list_empty(&src_q->done_list))
>  		poll_wait(file, &src_q->done_wq, wait);
> -	if (list_empty(&dst_q->done_list))
> +	if (list_empty(&dst_q->done_list)) {
> +		/*
> +		 * If the last buffer was dequeued from the capture queue,
> +		 * return immediately. DQBUF will return -EPIPE.
> +		 */
> +		if (dst_q->last_buffer_dequeued)
> +			return rc | POLLIN | POLLRDNORM;
> +
>  		poll_wait(file, &dst_q->done_wq, wait);
> +	}
>  
>  	if (m2m_ctx->m2m_dev->m2m_ops->lock)
>  		m2m_ctx->m2m_dev->m2m_ops->lock(m2m_ctx->priv);
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index d09a891..c2c2eac 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -2046,6 +2046,10 @@ static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool n
>  	struct vb2_buffer *vb = NULL;
>  	int ret;
>  
> +	if (q->last_buffer_dequeued) {
> +		dprintk(3, "last buffer dequeued already\n");
> +		return -EPIPE;
> +	}
>  	if (b->type != q->type) {
>  		dprintk(1, "invalid buffer type\n");
>  		return -EINVAL;
> @@ -2073,6 +2077,9 @@ static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool n
>  	/* Remove from videobuf queue */
>  	list_del(&vb->queued_entry);
>  	q->queued_count--;
> +	if (!V4L2_TYPE_IS_OUTPUT(q->type) &&
> +	    vb->v4l2_buf.flags & V4L2_BUF_FLAG_LAST)
> +		q->last_buffer_dequeued = true;
>  	/* go back to dequeued state */
>  	__vb2_dqbuf(vb);
>  
> @@ -2286,6 +2293,7 @@ static int vb2_internal_streamoff(struct vb2_queue *q, enum v4l2_buf_type type)
>  	 */
>  	__vb2_queue_cancel(q);
>  	q->waiting_for_buffers = !V4L2_TYPE_IS_OUTPUT(q->type);
> +	q->last_buffer_dequeued = false;
>  
>  	dprintk(3, "successful\n");
>  	return 0;
> @@ -2628,8 +2636,16 @@ unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
>  	if (V4L2_TYPE_IS_OUTPUT(q->type) && q->queued_count < q->num_buffers)
>  		return res | POLLOUT | POLLWRNORM;
>  
> -	if (list_empty(&q->done_list))
> +	if (list_empty(&q->done_list)) {
> +		/*
> +		 * If the last buffer was dequeued from a capture queue,
> +		 * return immediately. DQBUF will return -EPIPE.
> +		 */
> +		if (!V4L2_TYPE_IS_OUTPUT(q->type) && q->last_buffer_dequeued)
> +			return res | POLLIN | POLLRDNORM;
> +
>  		poll_wait(file, &q->done_wq, wait);
> +	}
>  
>  	/*
>  	 * Take first buffer available for dequeuing.
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index bd2cec2..ca337bf 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -429,6 +429,7 @@ struct vb2_queue {
>  	unsigned int			start_streaming_called:1;
>  	unsigned int			error:1;
>  	unsigned int			waiting_for_buffers:1;
> +	unsigned int			last_buffer_dequeued:1;
>  
>  	struct vb2_fileio_data		*fileio;
>  	struct vb2_threadio_data	*threadio;
> 

Re: [RFC PATCH 2/2] [media] videobuf2: return -EPIPE from DQBUF after the last buffer

[Philipp Zabel]

Am Montag, den 02.02.2015, 15:00 +0100 schrieb Hans Verkuil:
> On 01/22/2015 12:28 PM, Philipp Zabel wrote:
> > If the last buffer was dequeued from a capture queue, let poll return
> > immediately and let DQBUF return -EPIPE to signal there will no more
> > buffers to dequeue until STREAMOFF.
> 
> This looks OK to me, although I would like to see comments from others as well.
> Of course, this needs to be documented in the spec as well.

Thanks, I'll fix that in the next round.

> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > TODO: (How) should the last_buffer_dequeud flag be cleared in reaction to
> > V4L2_DEC_CMD_START?
> 
> I would suggest an inline function in videobuf2-core.h that clears the flag
> and that drivers can call. I don't think the vb2 core can detect when it is
> OK to clear the flag, it needs to be told by the driver (correct me if I am
> wrong).

No, I think you are right that this should be done explicitly. I'll add
an inline function next time.

regards
Philipp

Re: [RFC PATCH 2/2] [media] videobuf2: return -EPIPE from DQBUF after the last buffer

[Laurent Pinchart]

On Monday 02 February 2015 15:00:51 Hans Verkuil wrote:
> On 01/22/2015 12:28 PM, Philipp Zabel wrote:
> > If the last buffer was dequeued from a capture queue, let poll return
> > immediately and let DQBUF return -EPIPE to signal there will no more
> > buffers to dequeue until STREAMOFF.
> 
> This looks OK to me, although I would like to see comments from others as
> well. Of course, this needs to be documented in the spec as well.

I haven't followed the V4L2 codec API discussion during ELC-E. Could someone 
briefly expose the rationale for this and the codec draining flow ? The 
explanation should probably included in the documentation.

Existing applications will receive -EPIPE from DQBUF now. Have potential 
breakages been taken into account ?

> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > TODO: (How) should the last_buffer_dequeud flag be cleared in reaction to
> > V4L2_DEC_CMD_START?
> 
> I would suggest an inline function in videobuf2-core.h that clears the flag
> and that drivers can call. I don't think the vb2 core can detect when it is
> OK to clear the flag, it needs to be told by the driver (correct me if I am
> wrong).

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 2/2] [media] videobuf2: return -EPIPE from DQBUF after the last buffer

[Philipp Zabel]

Hi Laurent,

Am Dienstag, den 03.02.2015, 13:20 +0200 schrieb Laurent Pinchart:
> On Monday 02 February 2015 15:00:51 Hans Verkuil wrote:
> > On 01/22/2015 12:28 PM, Philipp Zabel wrote:
> > > If the last buffer was dequeued from a capture queue, let poll return
> > > immediately and let DQBUF return -EPIPE to signal there will no more
> > > buffers to dequeue until STREAMOFF.
> > 
> > This looks OK to me, although I would like to see comments from others as
> > well. Of course, this needs to be documented in the spec as well.
> 
> I haven't followed the V4L2 codec API discussion during ELC-E. Could someone 
> briefly expose the rationale for this and the codec draining flow ? The 
> explanation should probably included in the documentation.

This is the draining flow as written down in the notes:

  The decoder draining flow starts with the application making the
  driver aware of its intentions by calling VIDIOC_DECODER_CMD with
  V4L2_DEC_CMD_STOP.

  The driver processes the remaining buffers on the OUTPUT queue.
  Once all CAPTURE buffers produced from those are ready to dequeue, it 
  sends a V4L2_EVENT_EOS.

  The application then dequeues buffers from the CAPTURE queue until it
  encounters a buffer with V4L2_BUF_FLAG_LAST set. This buffer may
  already be empty due to hardware limitations.
  Alternatively, the application may simply continue to dequeue
  (possibly empty) buffers until the VIDIOC_DQBUF ioctl returns -EPIPE.

  To resume decoding, the application can call VIDIOC_DECODER_CMD with
  V4L2_CMD_START without the need to stop streaming.

This should work the same for encoding.
The rationale for using -EPIPE was IIRC that it allows for simpler code
(DQBUF loop until error). Also it works without the newest kernel
headers that #define V4L2_BUF_FLAG_LAST.

I don't remember if we talked about what should happen to new buffers
queued via QBUF on the OUTPUT queue while in draining mode, but I
suppose the driver should just hold them until V4L2_CMD_START is called.

> Existing applications will receive -EPIPE from DQBUF now. Have potential 
> breakages been taken into account ?

We can continue to produce empty frames this way, which are currently
used to signal the last frame in s5p-mfc. I am not aware of other
unspecified mechanisms in use.

regards
Philipp