[PATCH v4 0/4] Signalling last decoded frame by V4L2_BUF_FLAG_LAST and -EPIPE

[PATCH v4 0/4] 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).

Changes since v3:
 - Moved documentation changes into patches 1 and 2 and added a comment about
   DQBUF returning -EPIPE to the encoder/decoder stop command documentation.

regards
Philipp

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

Philipp Zabel (3):
  [media] videobuf2: return -EPIPE from DQBUF after the last buffer
  [media] coda: Set last buffer flag and fix EOS event
  [media] s5p-mfc: Set last buffer flag

 Documentation/DocBook/media/v4l/io.xml             | 10 ++++++++
 .../DocBook/media/v4l/vidioc-decoder-cmd.xml       |  8 ++++++-
 .../DocBook/media/v4l/vidioc-encoder-cmd.xml       |  7 +++++-
 Documentation/DocBook/media/v4l/vidioc-qbuf.xml    |  8 +++++++
 drivers/media/platform/coda/coda-bit.c             |  4 ++--
 drivers/media/platform/coda/coda-common.c          | 27 +++++++++-------------
 drivers/media/platform/coda/coda.h                 |  3 +++
 drivers/media/platform/s5p-mfc/s5p_mfc.c           |  1 +
 drivers/media/v4l2-core/v4l2-mem2mem.c             | 10 +++++++-
 drivers/media/v4l2-core/videobuf2-core.c           | 18 ++++++++++++++-
 include/media/videobuf2-core.h                     | 10 ++++++++
 include/trace/events/v4l2.h                        |  3 ++-
 include/uapi/linux/videodev2.h                     |  2 ++
 13 files changed, 88 insertions(+), 23 deletions(-)

-- 
2.1.4

[PATCH v4 1/4] [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.
The DocBook is updated to mention mem2mem codecs and the mem2mem draining flow
signals in the VIDIOC_DECODER_CMD V4L2_DEC_CMD_STOP and VIDIOC_ENCODER_CMD
V4L2_ENC_CMD_STOP documentation.

Signed-off-by: Peter Seiderer <ps.report@gmx.net>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v3:
 - Added DocBook update mentioning V4L2_BUF_FLAG_LAST in the encoder/decoder
   stop command documentation.
---
 Documentation/DocBook/media/v4l/io.xml                 | 10 ++++++++++
 Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml |  6 +++++-
 Documentation/DocBook/media/v4l/vidioc-encoder-cmd.xml |  5 ++++-
 include/trace/events/v4l2.h                            |  3 ++-
 include/uapi/linux/videodev2.h                         |  2 ++
 5 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
index 1c17f80..f3b8bc0 100644
--- a/Documentation/DocBook/media/v4l/io.xml
+++ b/Documentation/DocBook/media/v4l/io.xml
@@ -1129,6 +1129,16 @@ in this buffer has not been created by the CPU but by some DMA-capable unit,
 in which case caches have not been used.</entry>
 	  </row>
 	  <row>
+	    <entry><constant>V4L2_BUF_FLAG_LAST</constant></entry>
+	    <entry>0x00100000</entry>
+	    <entry>Last buffer produced by the hardware. mem2mem codec drivers
+set this flag on the capture queue for the last buffer when the
+<link linkend="vidioc-querybuf">VIDIOC_QUERYBUF</link> or
+<link linkend="vidioc-qbuf">VIDIOC_DQBUF</link> ioctl is called. After the
+queue is drained, the <link linkend="vidioc-qbuf">VIDIOC_DQBUF</link> ioctl will
+not block anymore, but return an &EPIPE;.</entry>
+	  </row>
+	  <row>
 	    <entry><constant>V4L2_BUF_FLAG_TIMESTAMP_MASK</constant></entry>
 	    <entry>0x0000e000</entry>
 	    <entry>Mask for timestamp types below. To test the
diff --git a/Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml b/Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml
index 9215627..cbb7135 100644
--- a/Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml
@@ -197,7 +197,11 @@ be muted when playing back at a non-standard speed.
 this command does nothing. This command has two flags:
 if <constant>V4L2_DEC_CMD_STOP_TO_BLACK</constant> is set, then the decoder will
 set the picture to black after it stopped decoding. Otherwise the last image will
-repeat. If <constant>V4L2_DEC_CMD_STOP_IMMEDIATELY</constant> is set, then the decoder
+repeat. mem2mem decoders will stop producing new frames altogether. They will send
+a <constant>V4L2_EVENT_EOS</constant> event after the last frame was decoded and
+will set the <constant>V4L2_BUF_FLAG_LAST</constant> buffer flag when there will
+be no new buffers produced to dequeue.
+If <constant>V4L2_DEC_CMD_STOP_IMMEDIATELY</constant> is set, then the decoder
 stops immediately (ignoring the <structfield>pts</structfield> value), otherwise it
 will keep decoding until timestamp >= pts or until the last of the pending data from
 its internal buffers was decoded.
diff --git a/Documentation/DocBook/media/v4l/vidioc-encoder-cmd.xml b/Documentation/DocBook/media/v4l/vidioc-encoder-cmd.xml
index 0619ca5..e9cf601 100644
--- a/Documentation/DocBook/media/v4l/vidioc-encoder-cmd.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-encoder-cmd.xml
@@ -129,7 +129,10 @@ this command.</entry>
 encoding will continue until the end of the current <wordasword>Group
 Of Pictures</wordasword>, otherwise encoding will stop immediately.
 When the encoder is already stopped, this command does
-nothing.</entry>
+nothing. mem2mem encoders will send a <constant>V4L2_EVENT_EOS</constant> event
+after the last frame was encoded and will set the
+<constant>V4L2_BUF_FLAG_LAST</constant> buffer flag on the capture queue when
+there will be no new buffers produced to dequeue</entry>
 	  </row>
 	  <row>
 	    <entry><constant>V4L2_ENC_CMD_PAUSE</constant></entry>
diff --git a/include/trace/events/v4l2.h b/include/trace/events/v4l2.h
index b9bb1f2..32c33aa 100644
--- a/include/trace/events/v4l2.h
+++ b/include/trace/events/v4l2.h
@@ -58,7 +58,8 @@
 		{ V4L2_BUF_FLAG_TIMESTAMP_MASK,	     "TIMESTAMP_MASK" },      \
 		{ V4L2_BUF_FLAG_TIMESTAMP_UNKNOWN,   "TIMESTAMP_UNKNOWN" },   \
 		{ V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC, "TIMESTAMP_MONOTONIC" }, \
-		{ V4L2_BUF_FLAG_TIMESTAMP_COPY,	     "TIMESTAMP_COPY" })
+		{ V4L2_BUF_FLAG_TIMESTAMP_COPY,	     "TIMESTAMP_COPY" },      \
+		{ V4L2_BUF_FLAG_LAST,                "LAST" })
 
 #define show_timecode_flags(flags)					  \
 	__print_flags(flags, "|",					  \
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

[PATCH v4 2/4] [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.
The driver signals the last buffer by setting the V4L2_BUF_FLAG_LAST.
To reenable dequeuing on the capture queue, the driver must explicitly
call vb2_clear_last_buffer_queued. The last buffer queued flag is
cleared automatically during STREAMOFF.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v3:
 - Added DocBook update mentioning DQBUF returning -EPIPE in the encoder/decoder
   stop command documentation.
---
 Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml |  4 +++-
 Documentation/DocBook/media/v4l/vidioc-encoder-cmd.xml |  4 +++-
 Documentation/DocBook/media/v4l/vidioc-qbuf.xml        |  8 ++++++++
 drivers/media/v4l2-core/v4l2-mem2mem.c                 | 10 +++++++++-
 drivers/media/v4l2-core/videobuf2-core.c               | 18 +++++++++++++++++-
 include/media/videobuf2-core.h                         | 10 ++++++++++
 6 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml b/Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml
index cbb7135..2601c37 100644
--- a/Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml
@@ -200,7 +200,9 @@ set the picture to black after it stopped decoding. Otherwise the last image wil
 repeat. mem2mem decoders will stop producing new frames altogether. They will send
 a <constant>V4L2_EVENT_EOS</constant> event after the last frame was decoded and
 will set the <constant>V4L2_BUF_FLAG_LAST</constant> buffer flag when there will
-be no new buffers produced to dequeue.
+be no new buffers produced to dequeue. Once this flag was set, the
+<link linkend="vidioc-qbuf">VIDIOC_DQBUF</link> ioctl will not block anymore,
+but return an &EPIPE;.
 If <constant>V4L2_DEC_CMD_STOP_IMMEDIATELY</constant> is set, then the decoder
 stops immediately (ignoring the <structfield>pts</structfield> value), otherwise it
 will keep decoding until timestamp >= pts or until the last of the pending data from
diff --git a/Documentation/DocBook/media/v4l/vidioc-encoder-cmd.xml b/Documentation/DocBook/media/v4l/vidioc-encoder-cmd.xml
index e9cf601..def688f 100644
--- a/Documentation/DocBook/media/v4l/vidioc-encoder-cmd.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-encoder-cmd.xml
@@ -132,7 +132,9 @@ When the encoder is already stopped, this command does
 nothing. mem2mem encoders will send a <constant>V4L2_EVENT_EOS</constant> event
 after the last frame was encoded and will set the
 <constant>V4L2_BUF_FLAG_LAST</constant> buffer flag on the capture queue when
-there will be no new buffers produced to dequeue</entry>
+there will be no new buffers produced to dequeue. Once this flag was set, the
+<link linkend="vidioc-qbuf">VIDIOC_DQBUF</link> ioctl will not block anymore,
+but return an &EPIPE;.</entry>
 	  </row>
 	  <row>
 	    <entry><constant>V4L2_ENC_CMD_PAUSE</constant></entry>
diff --git a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
index 3504a7f..6cfc53b 100644
--- a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
@@ -186,6 +186,14 @@ In that case the application should be able to safely reuse the buffer and
 continue streaming.
 	</para>
 	</listitem>
+	<term><errorcode>EPIPE</errorcode></term>
+	<listitem>
+	  <para><constant>VIDIOC_DQBUF</constant> returns this on an empty
+capture queue for mem2mem codecs if a buffer with the
+<constant>V4L2_BUF_FLAG_LAST</constant> was already dequeued and no new buffers
+are expected to become available.
+	</para>
+	</listitem>
       </varlistentry>
     </variablelist>
   </refsect1>
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 bc08a82..a0b9946 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..863a8bb 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;
@@ -609,6 +610,15 @@ static inline bool vb2_start_streaming_called(struct vb2_queue *q)
 	return q->start_streaming_called;
 }
 
+/**
+ * vb2_clear_last_buffer_dequeued() - clear last buffer dequeued flag of queue
+ * @q:		videobuf queue
+ */
+static inline void vb2_clear_last_buffer_dequeued(struct vb2_queue *q)
+{
+	q->last_buffer_dequeued = false;
+}
+
 /*
  * The following functions are not part of the vb2 core API, but are simple
  * helper functions that you can use in your struct v4l2_file_operations,
-- 
2.1.4

[PATCH v4 3/4] [media] coda: Set last buffer flag and fix EOS event

[Philipp Zabel]

Setting the last buffer flag causes the videobuf2 core to return -EPIPE from
DQBUF calls on the capture queue after the last buffer is dequeued.
This patch also fixes the EOS event to conform to the specification. It now is
sent right after the last buffer has been decoded instead of when the last
buffer is dequeued.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/media/platform/coda/coda-bit.c    |  4 ++--
 drivers/media/platform/coda/coda-common.c | 27 +++++++++++----------------
 drivers/media/platform/coda/coda.h        |  3 +++
 3 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/drivers/media/platform/coda/coda-bit.c b/drivers/media/platform/coda/coda-bit.c
index 856b542..9ae0bfa 100644
--- a/drivers/media/platform/coda/coda-bit.c
+++ b/drivers/media/platform/coda/coda-bit.c
@@ -1278,7 +1278,7 @@ static void coda_finish_encode(struct coda_ctx *ctx)
 	v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_DONE);
 
 	dst_buf = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
-	v4l2_m2m_buf_done(dst_buf, VB2_BUF_STATE_DONE);
+	coda_m2m_buf_done(ctx, dst_buf, VB2_BUF_STATE_DONE);
 
 	ctx->gopcounter--;
 	if (ctx->gopcounter < 0)
@@ -1887,7 +1887,7 @@ static void coda_finish_decode(struct coda_ctx *ctx)
 		}
 		vb2_set_plane_payload(dst_buf, 0, payload);
 
-		v4l2_m2m_buf_done(dst_buf, ctx->frame_errors[display_idx] ?
+		coda_m2m_buf_done(ctx, dst_buf, ctx->frame_errors[display_idx] ?
 				  VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE);
 
 		v4l2_dbg(1, coda_debug, &dev->v4l2_dev,
diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index 6f32e6d..f178ad3 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -705,35 +705,30 @@ static int coda_qbuf(struct file *file, void *priv,
 }
 
 static bool coda_buf_is_end_of_stream(struct coda_ctx *ctx,
-				      struct v4l2_buffer *buf)
+				      struct vb2_buffer *buf)
 {
 	struct vb2_queue *src_vq;
 
 	src_vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT);
 
 	return ((ctx->bit_stream_param & CODA_BIT_STREAM_END_FLAG) &&
-		(buf->sequence == (ctx->qsequence - 1)));
+		(buf->v4l2_buf.sequence == (ctx->qsequence - 1)));
 }
 
-static int coda_dqbuf(struct file *file, void *priv,
-		      struct v4l2_buffer *buf)
+void coda_m2m_buf_done(struct coda_ctx *ctx, struct vb2_buffer *buf,
+		       enum vb2_buffer_state state)
 {
-	struct coda_ctx *ctx = fh_to_ctx(priv);
-	int ret;
+	const struct v4l2_event eos_event = {
+		.type = V4L2_EVENT_EOS
+	};
 
-	ret = v4l2_m2m_dqbuf(file, ctx->fh.m2m_ctx, buf);
-
-	/* If this is the last capture buffer, emit an end-of-stream event */
-	if (buf->type == V4L2_BUF_TYPE_VIDEO_CAPTURE &&
-	    coda_buf_is_end_of_stream(ctx, buf)) {
-		const struct v4l2_event eos_event = {
-			.type = V4L2_EVENT_EOS
-		};
+	if (coda_buf_is_end_of_stream(ctx, buf)) {
+		buf->v4l2_buf.flags |= V4L2_BUF_FLAG_LAST;
 
 		v4l2_event_queue_fh(&ctx->fh, &eos_event);
 	}
 
-	return ret;
+	v4l2_m2m_buf_done(buf, state);
 }
 
 static int coda_g_selection(struct file *file, void *fh,
@@ -846,7 +841,7 @@ static const struct v4l2_ioctl_ops coda_ioctl_ops = {
 
 	.vidioc_qbuf		= coda_qbuf,
 	.vidioc_expbuf		= v4l2_m2m_ioctl_expbuf,
-	.vidioc_dqbuf		= coda_dqbuf,
+	.vidioc_dqbuf		= v4l2_m2m_ioctl_dqbuf,
 	.vidioc_create_bufs	= v4l2_m2m_ioctl_create_bufs,
 
 	.vidioc_streamon	= v4l2_m2m_ioctl_streamon,
diff --git a/drivers/media/platform/coda/coda.h b/drivers/media/platform/coda/coda.h
index 0c35cd5..420de18 100644
--- a/drivers/media/platform/coda/coda.h
+++ b/drivers/media/platform/coda/coda.h
@@ -291,6 +291,9 @@ static inline int coda_get_bitstream_payload(struct coda_ctx *ctx)
 
 void coda_bit_stream_end_flag(struct coda_ctx *ctx);
 
+void coda_m2m_buf_done(struct coda_ctx *ctx, struct vb2_buffer *buf,
+		       enum vb2_buffer_state state);
+
 int coda_h264_padding(int size, char *p);
 
 bool coda_jpeg_check_buffer(struct coda_ctx *ctx, struct vb2_buffer *vb);
-- 
2.1.4

[PATCH v4 4/4] [media] s5p-mfc: Set last buffer flag

[Philipp Zabel]

Setting the last buffer flag causes the videobuf2 core to return -EPIPE from
DQBUF calls on the capture queue after the last buffer is dequeued.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/media/platform/s5p-mfc/s5p_mfc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index 8e44a59..f08d639 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -211,6 +211,7 @@ static void s5p_mfc_handle_frame_all_extracted(struct s5p_mfc_ctx *ctx)
 			dst_buf->b->v4l2_buf.field = V4L2_FIELD_NONE;
 		else
 			dst_buf->b->v4l2_buf.field = V4L2_FIELD_INTERLACED;
+		dst_buf->b->v4l2_buf.flags |= V4L2_BUF_FLAG_LAST;
 
 		ctx->dec_dst_flag &= ~(1 << dst_buf->b->v4l2_buf.index);
 		vb2_buffer_done(dst_buf->b, VB2_BUF_STATE_DONE);
-- 
2.1.4

Re: [PATCH v4 1/4] [media] videodev2: Add V4L2_BUF_FLAG_LAST

[Pawel Osciak]

Hi,
Thanks for working on this!

On Wed, Mar 25, 2015 at 2:46 AM, Philipp Zabel <p.zabel@pengutronix.de> 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.
> The DocBook is updated to mention mem2mem codecs and the mem2mem draining flow
> signals in the VIDIOC_DECODER_CMD V4L2_DEC_CMD_STOP and VIDIOC_ENCODER_CMD
> V4L2_ENC_CMD_STOP documentation.
>
> Signed-off-by: Peter Seiderer <ps.report@gmx.net>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Changes since v3:
>  - Added DocBook update mentioning V4L2_BUF_FLAG_LAST in the encoder/decoder
>    stop command documentation.
> ---
>  Documentation/DocBook/media/v4l/io.xml                 | 10 ++++++++++
>  Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml |  6 +++++-
>  Documentation/DocBook/media/v4l/vidioc-encoder-cmd.xml |  5 ++++-
>  include/trace/events/v4l2.h                            |  3 ++-
>  include/uapi/linux/videodev2.h                         |  2 ++
>  5 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
> index 1c17f80..f3b8bc0 100644
> --- a/Documentation/DocBook/media/v4l/io.xml
> +++ b/Documentation/DocBook/media/v4l/io.xml
> @@ -1129,6 +1129,16 @@ in this buffer has not been created by the CPU but by some DMA-capable unit,
>  in which case caches have not been used.</entry>
>           </row>
>           <row>
> +           <entry><constant>V4L2_BUF_FLAG_LAST</constant></entry>
> +           <entry>0x00100000</entry>
> +           <entry>Last buffer produced by the hardware. mem2mem codec drivers
> +set this flag on the capture queue for the last buffer when the
> +<link linkend="vidioc-querybuf">VIDIOC_QUERYBUF</link> or
> +<link linkend="vidioc-qbuf">VIDIOC_DQBUF</link> ioctl is called. After the
> +queue is drained, the <link linkend="vidioc-qbuf">VIDIOC_DQBUF</link> ioctl will

Perhaps just s/After the queue is drained, the/Any subsequent/ ? This
would make it more clear I feel.
DQBUF of LAST is the end of draining.

> +not block anymore, but return an &EPIPE;.</entry>
> +         </row>
> +         <row>
>             <entry><constant>V4L2_BUF_FLAG_TIMESTAMP_MASK</constant></entry>
>             <entry>0x0000e000</entry>
>             <entry>Mask for timestamp types below. To test the
> diff --git a/Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml b/Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml
> index 9215627..cbb7135 100644
> --- a/Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml
> +++ b/Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml
> @@ -197,7 +197,11 @@ be muted when playing back at a non-standard speed.
>  this command does nothing. This command has two flags:
>  if <constant>V4L2_DEC_CMD_STOP_TO_BLACK</constant> is set, then the decoder will
>  set the picture to black after it stopped decoding. Otherwise the last image will
> -repeat. If <constant>V4L2_DEC_CMD_STOP_IMMEDIATELY</constant> is set, then the decoder
> +repeat. mem2mem decoders will stop producing new frames altogether. They will send
> +a <constant>V4L2_EVENT_EOS</constant> event after the last frame was decoded and

s/was decoded/has been decoded and all frames are ready to be dequeued/

> +will set the <constant>V4L2_BUF_FLAG_LAST</constant> buffer flag when there will
> +be no new buffers produced to dequeue.

To make the timing description more explicit, s/when there will be no
new buffers produced to dequeue./on the final buffer being dequeued/
perhaps?
EOS indicates "no more buffers will be produced and all are ready to
be dequeued", while LAST indicates "final buffer being dequeued".

> +If <constant>V4L2_DEC_CMD_STOP_IMMEDIATELY</constant> is set, then the decoder
>  stops immediately (ignoring the <structfield>pts</structfield> value), otherwise it
>  will keep decoding until timestamp >= pts or until the last of the pending data from
>  its internal buffers was decoded.
> diff --git a/Documentation/DocBook/media/v4l/vidioc-encoder-cmd.xml b/Documentation/DocBook/media/v4l/vidioc-encoder-cmd.xml
> index 0619ca5..e9cf601 100644
> --- a/Documentation/DocBook/media/v4l/vidioc-encoder-cmd.xml
> +++ b/Documentation/DocBook/media/v4l/vidioc-encoder-cmd.xml
> @@ -129,7 +129,10 @@ this command.</entry>
>  encoding will continue until the end of the current <wordasword>Group
>  Of Pictures</wordasword>, otherwise encoding will stop immediately.
>  When the encoder is already stopped, this command does
> -nothing.</entry>
> +nothing. mem2mem encoders will send a <constant>V4L2_EVENT_EOS</constant> event
> +after the last frame was encoded and will set the
> +<constant>V4L2_BUF_FLAG_LAST</constant> buffer flag on the capture queue when
> +there will be no new buffers produced to dequeue</entry>

I'd propose the same here.

-- 
Thanks,
Pawel

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

[Pawel Osciak]

Hi,

On Wed, Mar 25, 2015 at 2:46 AM, Philipp Zabel <p.zabel@pengutronix.de> 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.
> The driver signals the last buffer by setting the V4L2_BUF_FLAG_LAST.
> To reenable dequeuing on the capture queue, the driver must explicitly
> call vb2_clear_last_buffer_queued. The last buffer queued flag is
> cleared automatically during STREAMOFF.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Changes since v3:
>  - Added DocBook update mentioning DQBUF returning -EPIPE in the encoder/decoder
>    stop command documentation.
> ---
>  Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml |  4 +++-
>  Documentation/DocBook/media/v4l/vidioc-encoder-cmd.xml |  4 +++-
>  Documentation/DocBook/media/v4l/vidioc-qbuf.xml        |  8 ++++++++
>  drivers/media/v4l2-core/v4l2-mem2mem.c                 | 10 +++++++++-
>  drivers/media/v4l2-core/videobuf2-core.c               | 18 +++++++++++++++++-
>  include/media/videobuf2-core.h                         | 10 ++++++++++
>  6 files changed, 50 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml b/Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml

Would it make sense to perhaps split this patch into docbook and vb2
changes please?

> 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;

These indicate there is data to be read. Is there something else we
could return? Maybe POLLHUP?

> +
>                 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 bc08a82..a0b9946 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;
> +       }

This should go after the check for queue type at least. However, best
probably to __vb2_wait_for_done_vb(), where we already have the checks
for q->streaming and q->error.

>         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)

Do we need to check !V4L2_TYPE_IS_OUTPUT(q->type) here? We wouldn't
have set last_buffer_dequeued to true if it wasn't, so we could drop
this check?

> +                       return res | POLLIN | POLLRDNORM;

Same comment as above.

> +
>                 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..863a8bb 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;

Please add documentation above.

>
>         struct vb2_fileio_data          *fileio;
>         struct vb2_threadio_data        *threadio;
> @@ -609,6 +610,15 @@ static inline bool vb2_start_streaming_called(struct vb2_queue *q)
>         return q->start_streaming_called;
>  }
>
> +/**
> + * vb2_clear_last_buffer_dequeued() - clear last buffer dequeued flag of queue
> + * @q:         videobuf queue
> + */
> +static inline void vb2_clear_last_buffer_dequeued(struct vb2_queue *q)
> +{
> +       q->last_buffer_dequeued = false;
> +}
> +

There are some timing issues here to consider. How does the driver
know when it's ok to call this function, i.e. that the userspace has
already dequeued all the buffers, so that it doesn't call this too
early?

But, in general, in what kind of scenario would the driver want to
call this function, as opposed to vb2 clearing this flag by itself on
STREAMOFF?

-- 
Thanks,
Pawel

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

[Philipp Zabel]

Hi Pawel,

thanks for the review!

Am Montag, den 13.04.2015, 15:30 +0900 schrieb Pawel Osciak:
> Hi,
> 
> On Wed, Mar 25, 2015 at 2:46 AM, Philipp Zabel <p.zabel@pengutronix.de> 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.
> > The driver signals the last buffer by setting the V4L2_BUF_FLAG_LAST.
> > To reenable dequeuing on the capture queue, the driver must explicitly
> > call vb2_clear_last_buffer_queued. The last buffer queued flag is
> > cleared automatically during STREAMOFF.
> >
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > Changes since v3:
> >  - Added DocBook update mentioning DQBUF returning -EPIPE in the encoder/decoder
> >    stop command documentation.
> > ---
> >  Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml |  4 +++-
> >  Documentation/DocBook/media/v4l/vidioc-encoder-cmd.xml |  4 +++-
> >  Documentation/DocBook/media/v4l/vidioc-qbuf.xml        |  8 ++++++++
> >  drivers/media/v4l2-core/v4l2-mem2mem.c                 | 10 +++++++++-
> >  drivers/media/v4l2-core/videobuf2-core.c               | 18 +++++++++++++++++-
> >  include/media/videobuf2-core.h                         | 10 ++++++++++
> >  6 files changed, 50 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml b/Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml
> 
> Would it make sense to perhaps split this patch into docbook and vb2
> changes please?

Alright, I'll split DocBook from vb2 changes, with the documentation
patches first.

> > 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;
> 
> These indicate there is data to be read. Is there something else we
> could return? Maybe POLLHUP?

This is a good point, but I'm not sure. POLLHUP seems to mean a similar
thing, yet not quite the same. The documentation explicitly mentions
disconnected devices or FIFOs without writers, neither of which applies.
Also a FIFO signals POLLHUP when the last writer leaves, so we would
probably have to signal POLLHUP|POLLIN after the last buffer was decoded
for consistency, and keep that until the last buffer is dequeued. Then
we could signal POLLHUP alone here.

I didn't want to risk redefining/misinterpreting any poll(2) behavior,
so my interpretation was that POLLIN signals userspace to try a DQBUF,
as usual.

> > +
> >                 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 bc08a82..a0b9946 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;
> > +       }
> 
> This should go after the check for queue type at least. However, best
> probably to __vb2_wait_for_done_vb(), where we already have the checks
> for q->streaming and q->error.

Yes, that'll be better.

> >         if (b->type != q->type) {
> >                 dprintk(1, "invalid buffer type\n");
> >                 return -EINVAL;
[...]
> > @@ -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)
> 
> Do we need to check !V4L2_TYPE_IS_OUTPUT(q->type) here? We wouldn't
> have set last_buffer_dequeued to true if it wasn't, so we could drop
> this check?

Indeed we don't. I'll drop the check.

> > +                       return res | POLLIN | POLLRDNORM;
> 
> Same comment as above.

Same concern as above.

> > +
> >                 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..863a8bb 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;
> 
> Please add documentation above.

Will do.

> >
> >         struct vb2_fileio_data          *fileio;
> >         struct vb2_threadio_data        *threadio;
> > @@ -609,6 +610,15 @@ static inline bool vb2_start_streaming_called(struct vb2_queue *q)
> >         return q->start_streaming_called;
> >  }
> >
> > +/**
> > + * vb2_clear_last_buffer_dequeued() - clear last buffer dequeued flag of queue
> > + * @q:         videobuf queue
> > + */
> > +static inline void vb2_clear_last_buffer_dequeued(struct vb2_queue *q)
> > +{
> > +       q->last_buffer_dequeued = false;
> > +}
> > +
> 
> There are some timing issues here to consider. How does the driver
> know when it's ok to call this function, i.e. that the userspace has
> already dequeued all the buffers, so that it doesn't call this too
> early?

This flag is only set when userspace dequeues the last buffer. I'd
expect the driver to clear the flag after restarting the machinery that
can actually produce decoded frames.

> But, in general, in what kind of scenario would the driver want to
> call this function, as opposed to vb2 clearing this flag by itself on
> STREAMOFF?

There is VIDIOC_DECODER_CMD / V4L2_DEC_CMD_START.
I'd expect this timeline for decoder draining and restart:

- userspace calls VIDIOC_DECODER_CMD, cmd=V4L2_DEC_CMD_STOP
  after queueing the last output buffer to be decoded
- the driver processes remaining output buffers into capture buffers
  and sets the V4L2_BUF_FLAG_LAST set on the last capture buffer
- when the last capture buffer is put on the queue, the driver
  sends V4L2_EVENT_EOS
- userspace dequeues remaining buffers (either in reaction to the
  V4L2_EVENT_EOS signal or because it knows it just sent CMD_STOP)
  until the returned buffer has V4L2_BUF_FLAG_LAST set, or until the
  driver sets the last_buffer_dequeued flag and VIDIOC_DQBUF returns
  -EPIPE

- userspace calls VIDIOC_DECODER_CMD, cmd=V4L2_DEC_CMD_START
  after queueing new output buffers to be decoded
- the driver restarts the decoding process and clears the
  last_buffer_dequeued flag before returning from VIDIOC_DECODER_CMD
- userspace can poll on the capture queue again

regards
Philipp

Re: [PATCH v4 1/4] [media] videodev2: Add V4L2_BUF_FLAG_LAST

[Philipp Zabel]

Am Montag, den 13.04.2015, 14:42 +0900 schrieb Pawel Osciak:
> Hi,
> Thanks for working on this!
> 
> On Wed, Mar 25, 2015 at 2:46 AM, Philipp Zabel <p.zabel@pengutronix.de> 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.
> > The DocBook is updated to mention mem2mem codecs and the mem2mem draining flow
> > signals in the VIDIOC_DECODER_CMD V4L2_DEC_CMD_STOP and VIDIOC_ENCODER_CMD
> > V4L2_ENC_CMD_STOP documentation.
> >
> > Signed-off-by: Peter Seiderer <ps.report@gmx.net>
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > Changes since v3:
> >  - Added DocBook update mentioning V4L2_BUF_FLAG_LAST in the encoder/decoder
> >    stop command documentation.
> > ---
> >  Documentation/DocBook/media/v4l/io.xml                 | 10 ++++++++++
> >  Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml |  6 +++++-
> >  Documentation/DocBook/media/v4l/vidioc-encoder-cmd.xml |  5 ++++-
> >  include/trace/events/v4l2.h                            |  3 ++-
> >  include/uapi/linux/videodev2.h                         |  2 ++
> >  5 files changed, 23 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
> > index 1c17f80..f3b8bc0 100644
> > --- a/Documentation/DocBook/media/v4l/io.xml
> > +++ b/Documentation/DocBook/media/v4l/io.xml
> > @@ -1129,6 +1129,16 @@ in this buffer has not been created by the CPU but by some DMA-capable unit,
> >  in which case caches have not been used.</entry>
> >           </row>
> >           <row>
> > +           <entry><constant>V4L2_BUF_FLAG_LAST</constant></entry>
> > +           <entry>0x00100000</entry>
> > +           <entry>Last buffer produced by the hardware. mem2mem codec drivers
> > +set this flag on the capture queue for the last buffer when the
> > +<link linkend="vidioc-querybuf">VIDIOC_QUERYBUF</link> or
> > +<link linkend="vidioc-qbuf">VIDIOC_DQBUF</link> ioctl is called. After the
> > +queue is drained, the <link linkend="vidioc-qbuf">VIDIOC_DQBUF</link> ioctl will
> 
> Perhaps just s/After the queue is drained, the/Any subsequent/ ? This
> would make it more clear I feel.
> DQBUF of LAST is the end of draining.

Concise, I like it.

> > +not block anymore, but return an &EPIPE;.</entry>
> > +         </row>
> > +         <row>
> >             <entry><constant>V4L2_BUF_FLAG_TIMESTAMP_MASK</constant></entry>
> >             <entry>0x0000e000</entry>
> >             <entry>Mask for timestamp types below. To test the
> > diff --git a/Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml b/Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml
> > index 9215627..cbb7135 100644
> > --- a/Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml
> > +++ b/Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml
> > @@ -197,7 +197,11 @@ be muted when playing back at a non-standard speed.
> >  this command does nothing. This command has two flags:
> >  if <constant>V4L2_DEC_CMD_STOP_TO_BLACK</constant> is set, then the decoder will
> >  set the picture to black after it stopped decoding. Otherwise the last image will
> > -repeat. If <constant>V4L2_DEC_CMD_STOP_IMMEDIATELY</constant> is set, then the decoder
> > +repeat. mem2mem decoders will stop producing new frames altogether. They will send
> > +a <constant>V4L2_EVENT_EOS</constant> event after the last frame was decoded and
> 
> s/was decoded/has been decoded and all frames are ready to be dequeued/

Yes.

> > +will set the <constant>V4L2_BUF_FLAG_LAST</constant> buffer flag when there will
> > +be no new buffers produced to dequeue.
> 
> To make the timing description more explicit, s/when there will be no
> new buffers produced to dequeue./on the final buffer being dequeued/
> perhaps?
> EOS indicates "no more buffers will be produced and all are ready to
> be dequeued", while LAST indicates "final buffer being dequeued".

Yes.

> > +If <constant>V4L2_DEC_CMD_STOP_IMMEDIATELY</constant> is set, then the decoder
> >  stops immediately (ignoring the <structfield>pts</structfield> value), otherwise it
> >  will keep decoding until timestamp >= pts or until the last of the pending data from
> >  its internal buffers was decoded.
> > diff --git a/Documentation/DocBook/media/v4l/vidioc-encoder-cmd.xml b/Documentation/DocBook/media/v4l/vidioc-encoder-cmd.xml
> > index 0619ca5..e9cf601 100644
> > --- a/Documentation/DocBook/media/v4l/vidioc-encoder-cmd.xml
> > +++ b/Documentation/DocBook/media/v4l/vidioc-encoder-cmd.xml
> > @@ -129,7 +129,10 @@ this command.</entry>
> >  encoding will continue until the end of the current <wordasword>Group
> >  Of Pictures</wordasword>, otherwise encoding will stop immediately.
> >  When the encoder is already stopped, this command does
> > -nothing.</entry>
> > +nothing. mem2mem encoders will send a <constant>V4L2_EVENT_EOS</constant> event
> > +after the last frame was encoded and will set the
> > +<constant>V4L2_BUF_FLAG_LAST</constant> buffer flag on the capture queue when
> > +there will be no new buffers produced to dequeue</entry>
> 
> I'd propose the same here.

And yes. Thanks, I'll make those changes.

regards
Philipp

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

[Kamil Debski]

Hi,

> From: Philipp Zabel [mailto:p.zabel@pengutronix.de]
> Sent: Monday, April 13, 2015 6:15 PM
> To: Pawel Osciak
> Cc: LMML; Hans Verkuil; Kamil Debski; Laurent Pinchart; Nicolas
> Dufresne; Sakari Ailus; kernel@pengutronix.de
> Subject: Re: [PATCH v4 2/4] [media] videobuf2: return -EPIPE from DQBUF
> after the last buffer
> 
> Hi Pawel,
> 
> thanks for the review!
> 
> Am Montag, den 13.04.2015, 15:30 +0900 schrieb Pawel Osciak:
> > Hi,
> >
> > On Wed, Mar 25, 2015 at 2:46 AM, Philipp Zabel
> <p.zabel@pengutronix.de> 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.
> > > The driver signals the last buffer by setting the
> V4L2_BUF_FLAG_LAST.
> > > To reenable dequeuing on the capture queue, the driver must
> > > explicitly call vb2_clear_last_buffer_queued. The last buffer
> queued
> > > flag is cleared automatically during STREAMOFF.
> > >
> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > ---
> > > Changes since v3:
> > >  - Added DocBook update mentioning DQBUF returning -EPIPE in the
> encoder/decoder
> > >    stop command documentation.
> > > ---
> > >  Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml |  4 +++-
> > > Documentation/DocBook/media/v4l/vidioc-encoder-cmd.xml |  4 +++-
> > >  Documentation/DocBook/media/v4l/vidioc-qbuf.xml        |  8
> ++++++++
> > >  drivers/media/v4l2-core/v4l2-mem2mem.c                 | 10
> +++++++++-
> > >  drivers/media/v4l2-core/videobuf2-core.c               | 18
> +++++++++++++++++-
> > >  include/media/videobuf2-core.h                         | 10
> ++++++++++
> > >  6 files changed, 50 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml
> > > b/Documentation/DocBook/media/v4l/vidioc-decoder-cmd.xml
> >
> > Would it make sense to perhaps split this patch into docbook and vb2
> > changes please?
> 
> Alright, I'll split DocBook from vb2 changes, with the documentation
> patches first.
> 
> > > 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;
> >
> > These indicate there is data to be read. Is there something else we
> > could return? Maybe POLLHUP?
> 
> This is a good point, but I'm not sure. POLLHUP seems to mean a similar
> thing, yet not quite the same. The documentation explicitly mentions
> disconnected devices or FIFOs without writers, neither of which applies.
> Also a FIFO signals POLLHUP when the last writer leaves, so we would
> probably have to signal POLLHUP|POLLIN after the last buffer was
> decoded for consistency, and keep that until the last buffer is
> dequeued. Then we could signal POLLHUP alone here.
> 
> I didn't want to risk redefining/misinterpreting any poll(2) behavior,
> so my interpretation was that POLLIN signals userspace to try a DQBUF,
> as usual.
> 
> > > +
> > >                 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 bc08a82..a0b9946 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;
> > > +       }
> >
> > This should go after the check for queue type at least. However, best
> > probably to __vb2_wait_for_done_vb(), where we already have the
> checks
> > for q->streaming and q->error.
> 
> Yes, that'll be better.
> 
> > >         if (b->type != q->type) {
> > >                 dprintk(1, "invalid buffer type\n");
> > >                 return -EINVAL;
> [...]
> > > @@ -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)
> >
> > Do we need to check !V4L2_TYPE_IS_OUTPUT(q->type) here? We wouldn't
> > have set last_buffer_dequeued to true if it wasn't, so we could drop
> > this check?
> 
> Indeed we don't. I'll drop the check.
> 
> > > +                       return res | POLLIN | POLLRDNORM;
> >
> > Same comment as above.
> 
> Same concern as above.
> 
> > > +
> > >                 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..863a8bb 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;
> >
> > Please add documentation above.
> 
> Will do.
> 
> > >
> > >         struct vb2_fileio_data          *fileio;
> > >         struct vb2_threadio_data        *threadio;
> > > @@ -609,6 +610,15 @@ static inline bool
> vb2_start_streaming_called(struct vb2_queue *q)
> > >         return q->start_streaming_called;  }
> > >
> > > +/**
> > > + * vb2_clear_last_buffer_dequeued() - clear last buffer dequeued
> flag of queue
> > > + * @q:         videobuf queue
> > > + */
> > > +static inline void vb2_clear_last_buffer_dequeued(struct vb2_queue
> > > +*q) {
> > > +       q->last_buffer_dequeued = false; }
> > > +
> >
> > There are some timing issues here to consider. How does the driver
> > know when it's ok to call this function, i.e. that the userspace has
> > already dequeued all the buffers, so that it doesn't call this too
> > early?
> 
> This flag is only set when userspace dequeues the last buffer. I'd
> expect the driver to clear the flag after restarting the machinery that
> can actually produce decoded frames.
> 
> > But, in general, in what kind of scenario would the driver want to
> > call this function, as opposed to vb2 clearing this flag by itself on
> > STREAMOFF?
> 
> There is VIDIOC_DECODER_CMD / V4L2_DEC_CMD_START.
> I'd expect this timeline for decoder draining and restart:
> 
> - userspace calls VIDIOC_DECODER_CMD, cmd=V4L2_DEC_CMD_STOP
>   after queueing the last output buffer to be decoded
> - the driver processes remaining output buffers into capture buffers
>   and sets the V4L2_BUF_FLAG_LAST set on the last capture Buffet

I would like to confirm that it will work with MFC. Am I right that the
below will work? Did you take that into account?

So in MFC's case the V4L2_BUF_FLAG_LAST will be set on the one buffer after
the last one and the bytesused of that buffer would be set to 0. 

The problem of MFC is that it will signal that the last frame was decoded
after it was decoded. To particularize:
- a frame is decoded, an irq is sent by MFC - we have a new decoded picture
- next an irq is sent with an internal MFC flag that the buffer is empty
  (last picture was already decoded)

> - when the last capture buffer is put on the queue, the driver
>   sends V4L2_EVENT_EOS
> - userspace dequeues remaining buffers (either in reaction to the
>   V4L2_EVENT_EOS signal or because it knows it just sent CMD_STOP)
>   until the returned buffer has V4L2_BUF_FLAG_LAST set, or until the
>   driver sets the last_buffer_dequeued flag and VIDIOC_DQBUF returns
>   -EPIPE
> 
> - userspace calls VIDIOC_DECODER_CMD, cmd=V4L2_DEC_CMD_START
>   after queueing new output buffers to be decoded
> - the driver restarts the decoding process and clears the
>   last_buffer_dequeued flag before returning from VIDIOC_DECODER_CMD
> - userspace can poll on the capture queue again
> 
> regards
> Philipp

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

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

[Philipp Zabel]

Am Donnerstag, den 16.04.2015, 10:23 +0200 schrieb Kamil Debski:
[...]
> > > But, in general, in what kind of scenario would the driver want to
> > > call this function, as opposed to vb2 clearing this flag by itself on
> > > STREAMOFF?
> > 
> > There is VIDIOC_DECODER_CMD / V4L2_DEC_CMD_START.
> > I'd expect this timeline for decoder draining and restart:
> > 
> > - userspace calls VIDIOC_DECODER_CMD, cmd=V4L2_DEC_CMD_STOP
> >   after queueing the last output buffer to be decoded
> > - the driver processes remaining output buffers into capture buffers
> >   and sets the V4L2_BUF_FLAG_LAST set on the last capture Buffet
>
> I would like to confirm that it will work with MFC. Am I right that the
> below will work? Did you take that into account?

I see no reason why it wouldn't. The only difference is that userspace
has to be able to handle the empty frame.

> So in MFC's case the V4L2_BUF_FLAG_LAST will be set on the one buffer after
> the last one and the bytesused of that buffer would be set to 0. 
> 
> The problem of MFC is that it will signal that the last frame was decoded
> after it was decoded. To particularize:
> - a frame is decoded, an irq is sent by MFC - we have a new decoded picture
> - next an irq is sent with an internal MFC flag that the buffer is empty
>   (last picture was already decoded)

Doesn't MFC userspace currently stop on the empty frame? That empty
frame will still be dequeued as before, the only difference is the added
V4L2_BUF_FLAG_LAST on it, and that subsequent calls to DQBUF would
return -EPIPE.

regards
Philipp

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

[Kamil Debski]

Hi, 

> From: Philipp Zabel [mailto:p.zabel@pengutronix.de]
> Sent: Monday, April 20, 2015 10:28 AM
> To: Kamil Debski
> Cc: 'Pawel Osciak'; 'LMML'; 'Hans Verkuil'; 'Laurent Pinchart';
> 'Nicolas Dufresne'; 'Sakari Ailus'; kernel@pengutronix.de
> Subject: Re: [PATCH v4 2/4] [media] videobuf2: return -EPIPE from DQBUF
> after the last buffer
> 
> Am Donnerstag, den 16.04.2015, 10:23 +0200 schrieb Kamil Debski:
> [...]
> > > > But, in general, in what kind of scenario would the driver want
> to
> > > > call this function, as opposed to vb2 clearing this flag by
> itself on
> > > > STREAMOFF?
> > >
> > > There is VIDIOC_DECODER_CMD / V4L2_DEC_CMD_START.
> > > I'd expect this timeline for decoder draining and restart:
> > >
> > > - userspace calls VIDIOC_DECODER_CMD, cmd=V4L2_DEC_CMD_STOP
> > >   after queueing the last output buffer to be decoded
> > > - the driver processes remaining output buffers into capture
> buffers
> > >   and sets the V4L2_BUF_FLAG_LAST set on the last capture Buffet
> >
> > I would like to confirm that it will work with MFC. Am I right that
> the
> > below will work? Did you take that into account?
> 
> I see no reason why it wouldn't. The only difference is that userspace
> has to be able to handle the empty frame.

I just checked the notes from the codec meeting in Dusseldorf. When we
talked about the draining flow, we agreed to mention in the documentation
that a buffer with the V4L2_BUF_FLAG_LAST can be empty. Thus the userspace
applications should check the size of last frame.

Could you please add that the last buffer can be empty (zero size) in the
DocBook patch?

> 
> > So in MFC's case the V4L2_BUF_FLAG_LAST will be set on the one buffer
> after
> > the last one and the bytesused of that buffer would be set to 0.
> >
> > The problem of MFC is that it will signal that the last frame was
> decoded
> > after it was decoded. To particularize:
> > - a frame is decoded, an irq is sent by MFC - we have a new decoded
> picture
> > - next an irq is sent with an internal MFC flag that the buffer is
> empty
> >   (last picture was already decoded)
> 
> Doesn't MFC userspace currently stop on the empty frame? That empty
> frame will still be dequeued as before, the only difference is the
> added
> V4L2_BUF_FLAG_LAST on it, and that subsequent calls to DQBUF would
> return -EPIPE.

Ok, I see this.
 
> regards
> Philipp

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