[PATCH 0/2] s5p-mfc: added encoder support for end of stream handling

[PATCH 0/2] s5p-mfc: added encoder support for end of stream handling

[Andrzej Hajda]

Those patches add end of stream handling for s5p-mfc encoder.

The first patch was sent already to the list as RFC, but the discussion ended
without any decision.
This patch adds new v4l2_buffer flag V4L2_BUF_FLAG_EOS. Below short
description of this change.

s5p_mfc is a mem-to-mem MPEG/H263/H264 encoder and it requires that the last
incoming frame must be processed differently, it means the information about
the end of the stream driver should receive NOT LATER than the last
V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE buffer. Common practice
of sending empty buffer to indicate end-of-stream do not work in such case.
Setting V4L2_BUF_FLAG_EOS flag for the last V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
buffer seems to be the most straightforward solution here.

V4L2_BUF_FLAG_EOS flag should be used by application if driver requires it
and it should be set only on V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE buffers.

The second patch implements end-of-stream handling in s5p-mfc.

Comments are welcome
Andrzej Hajda

[PATCH 1/2] v4l: added V4L2_BUF_FLAG_EOS flag indicating the last frame in the stream

[Andrzej Hajda]

Some devices requires indicator if the buffer is the last one in the stream.
Applications and drivers can use this flag in such case.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 Documentation/DocBook/media/v4l/io.xml          |    7 +++++++
 Documentation/DocBook/media/v4l/vidioc-qbuf.xml |    2 ++
 include/linux/videodev2.h                       |    1 +
 3 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
index fd6aca2..dcbf1e0 100644
--- a/Documentation/DocBook/media/v4l/io.xml
+++ b/Documentation/DocBook/media/v4l/io.xml
@@ -956,6 +956,13 @@ Typically applications shall use this flag for output buffers if the data
 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_EOS</constant></entry>
+	    <entry>0x2000</entry>
+	    <entry>Application should set this flag in the output buffer
+in order to inform the driver about the last frame of the stream. Some
+drivers may require it to properly finish processing the stream.</entry>
+	  </row>
 	</tbody>
       </tgroup>
     </table>
diff --git a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
index 9caa49a..ad49f7d 100644
--- a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
@@ -76,6 +76,8 @@ supports capturing from specific video inputs and you want to specify a video
 input, then <structfield>flags</structfield> should be set to
 <constant>V4L2_BUF_FLAG_INPUT</constant> and the field
 <structfield>input</structfield> must be initialized to the desired input.
+Some drivers expects applications set <constant>V4L2_BUF_FLAG_EOS</constant>
+flag on the last buffer of the stream.
 The <structfield>reserved</structfield> field must be set to 0. When using
 the <link linkend="planar-apis">multi-planar API</link>, the
 <structfield>m.planes</structfield> field must contain a userspace pointer
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index 370d111..e44a7cd 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -676,6 +676,7 @@ struct v4l2_buffer {
 /* Cache handling flags */
 #define V4L2_BUF_FLAG_NO_CACHE_INVALIDATE	0x0800
 #define V4L2_BUF_FLAG_NO_CACHE_CLEAN		0x1000
+#define V4L2_BUF_FLAG_EOS	0x2000	/* The last buffer in the stream */
 
 /*
  *	O V E R L A Y   P R E V I E W
-- 
1.7.0.4

[PATCH 2/2] s5p-mfc: added encoder support for end of stream handling

[Andrzej Hajda]

s5p-mfc encoder after receiving buffer with flag V4L2_BUF_FLAG_EOS
will put all buffers cached in device into capture queue.
It will indicate end of encoded stream by providing empty buffer.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/s5p-mfc/s5p_mfc.c     |   44 +++++++++++++++++++++++++++++
 drivers/media/video/s5p-mfc/s5p_mfc_enc.c |   13 ++------
 drivers/media/video/s5p-mfc/s5p_mfc_opr.c |   33 ++++++++++++++++-----
 3 files changed, 72 insertions(+), 18 deletions(-)

diff --git a/drivers/media/video/s5p-mfc/s5p_mfc.c b/drivers/media/video/s5p-mfc/s5p_mfc.c
index 9bb68e7..fed8f55 100644
--- a/drivers/media/video/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/video/s5p-mfc/s5p_mfc.c
@@ -539,6 +539,45 @@ static void s5p_mfc_handle_init_buffers(struct s5p_mfc_ctx *ctx,
 	}
 }
 
+static void s5p_mfc_handle_complete(struct s5p_mfc_ctx *ctx,
+				 unsigned int reason, unsigned int err)
+{
+	struct s5p_mfc_dev *dev = ctx->dev;
+	struct s5p_mfc_buf *mb_entry;
+	unsigned long flags;
+
+	mfc_debug(2, "Stream completed");
+
+	s5p_mfc_clear_int_flags(dev);
+	ctx->int_type = reason;
+	ctx->int_err = err;
+
+	spin_lock_irqsave(&dev->irqlock, flags);
+	ctx->state = MFCINST_FINISHED;
+
+	if (!list_empty(&ctx->dst_queue)) {
+		mb_entry = list_entry(ctx->dst_queue.next, struct s5p_mfc_buf,
+									list);
+		list_del(&mb_entry->list);
+		ctx->dst_queue_cnt--;
+		vb2_set_plane_payload(mb_entry->b, 0, 0);
+		vb2_buffer_done(mb_entry->b, VB2_BUF_STATE_DONE);
+	}
+
+	spin_unlock_irqrestore(&dev->irqlock, flags);
+	if (ctx->dst_queue_cnt == 0) {
+		spin_lock(&dev->condlock);
+		clear_bit(ctx->num, &dev->ctx_work_bits);
+		spin_unlock(&dev->condlock);
+	}
+
+	if (test_and_clear_bit(0, &dev->hw_lock) == 0)
+		BUG();
+
+	s5p_mfc_clock_off();
+	wake_up(&ctx->queue);
+}
+
 /* Interrupt processing */
 static irqreturn_t s5p_mfc_irq(int irq, void *priv)
 {
@@ -614,6 +653,11 @@ static irqreturn_t s5p_mfc_irq(int irq, void *priv)
 	case S5P_FIMV_R2H_CMD_INIT_BUFFERS_RET:
 		s5p_mfc_handle_init_buffers(ctx, reason, err);
 		break;
+
+	case S5P_FIMV_R2H_CMD_ENC_COMPLETE_RET:
+		s5p_mfc_handle_complete(ctx, reason, err);
+		break;
+
 	default:
 		mfc_debug(2, "Unknown int reason\n");
 		s5p_mfc_clear_int_flags(dev);
diff --git a/drivers/media/video/s5p-mfc/s5p_mfc_enc.c b/drivers/media/video/s5p-mfc/s5p_mfc_enc.c
index acedb20..8dec186 100644
--- a/drivers/media/video/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/video/s5p-mfc/s5p_mfc_enc.c
@@ -584,7 +584,7 @@ static int s5p_mfc_ctx_ready(struct s5p_mfc_ctx *ctx)
 		return 1;
 	/* context is ready to encode remain frames */
 	if (ctx->state == MFCINST_FINISHING &&
-		ctx->src_queue_cnt >= 1 && ctx->dst_queue_cnt >= 1)
+		ctx->dst_queue_cnt >= 1)
 		return 1;
 	mfc_debug(2, "ctx is not ready\n");
 	return 0;
@@ -1715,15 +1715,8 @@ static void s5p_mfc_buf_queue(struct vb2_buffer *vb)
 		mfc_buf = &ctx->src_bufs[vb->v4l2_buf.index];
 		mfc_buf->used = 0;
 		spin_lock_irqsave(&dev->irqlock, flags);
-		if (vb->v4l2_planes[0].bytesused == 0) {
-			mfc_debug(1, "change state to FINISHING\n");
-			ctx->state = MFCINST_FINISHING;
-			vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
-			cleanup_ref_queue(ctx);
-		} else {
-			list_add_tail(&mfc_buf->list, &ctx->src_queue);
-			ctx->src_queue_cnt++;
-		}
+		list_add_tail(&mfc_buf->list, &ctx->src_queue);
+		ctx->src_queue_cnt++;
 		spin_unlock_irqrestore(&dev->irqlock, flags);
 	} else {
 		mfc_err("unsupported buffer type (%d)\n", vq->type);
diff --git a/drivers/media/video/s5p-mfc/s5p_mfc_opr.c b/drivers/media/video/s5p-mfc/s5p_mfc_opr.c
index e6217cb..4bcf93f 100644
--- a/drivers/media/video/s5p-mfc/s5p_mfc_opr.c
+++ b/drivers/media/video/s5p-mfc/s5p_mfc_opr.c
@@ -1081,8 +1081,14 @@ int s5p_mfc_encode_one_frame(struct s5p_mfc_ctx *ctx)
 	else if (ctx->src_fmt->fourcc == V4L2_PIX_FMT_NV12MT)
 		mfc_write(dev, 3, S5P_FIMV_ENC_MAP_FOR_CUR);
 	s5p_mfc_set_shared_buffer(ctx);
-	mfc_write(dev, (S5P_FIMV_CH_FRAME_START << 16 & 0x70000) |
-		(ctx->inst_no), S5P_FIMV_SI_CH0_INST_ID);
+
+	if (ctx->state == MFCINST_FINISHING)
+		mfc_write(dev, ((S5P_FIMV_CH_LAST_FRAME & S5P_FIMV_CH_MASK) <<
+		S5P_FIMV_CH_SHIFT) | (ctx->inst_no), S5P_FIMV_SI_CH0_INST_ID);
+	else
+		mfc_write(dev, ((S5P_FIMV_CH_FRAME_START & S5P_FIMV_CH_MASK) <<
+		S5P_FIMV_CH_SHIFT) | (ctx->inst_no), S5P_FIMV_SI_CH0_INST_ID);
+
 	return 0;
 }
 
@@ -1160,7 +1166,7 @@ static int s5p_mfc_run_enc_frame(struct s5p_mfc_ctx *ctx)
 	unsigned int dst_size;
 
 	spin_lock_irqsave(&dev->irqlock, flags);
-	if (list_empty(&ctx->src_queue)) {
+	if (list_empty(&ctx->src_queue) && ctx->state != MFCINST_FINISHING) {
 		mfc_debug(2, "no src buffers\n");
 		spin_unlock_irqrestore(&dev->irqlock, flags);
 		return -EAGAIN;
@@ -1170,11 +1176,18 @@ static int s5p_mfc_run_enc_frame(struct s5p_mfc_ctx *ctx)
 		spin_unlock_irqrestore(&dev->irqlock, flags);
 		return -EAGAIN;
 	}
-	src_mb = list_entry(ctx->src_queue.next, struct s5p_mfc_buf, list);
-	src_mb->used = 1;
-	src_y_addr = vb2_dma_contig_plane_dma_addr(src_mb->b, 0);
-	src_c_addr = vb2_dma_contig_plane_dma_addr(src_mb->b, 1);
-	s5p_mfc_set_enc_frame_buffer(ctx, src_y_addr, src_c_addr);
+	if (list_empty(&ctx->src_queue)) {
+		s5p_mfc_set_enc_frame_buffer(ctx, 0, 0);
+	} else {
+		src_mb = list_entry(ctx->src_queue.next, struct s5p_mfc_buf,
+									list);
+		src_mb->used = 1;
+		src_y_addr = vb2_dma_contig_plane_dma_addr(src_mb->b, 0);
+		src_c_addr = vb2_dma_contig_plane_dma_addr(src_mb->b, 1);
+		s5p_mfc_set_enc_frame_buffer(ctx, src_y_addr, src_c_addr);
+		if (src_mb->b->v4l2_buf.flags & V4L2_BUF_FLAG_EOS)
+			ctx->state = MFCINST_FINISHING;
+	}
 	dst_mb = list_entry(ctx->dst_queue.next, struct s5p_mfc_buf, list);
 	dst_mb->used = 1;
 	dst_addr = vb2_dma_contig_plane_dma_addr(dst_mb->b, 0);
@@ -1183,6 +1196,8 @@ static int s5p_mfc_run_enc_frame(struct s5p_mfc_ctx *ctx)
 	spin_unlock_irqrestore(&dev->irqlock, flags);
 	dev->curr_ctx = ctx->num;
 	s5p_mfc_clean_ctx_int_flags(ctx);
+	mfc_debug(2, "encoding %s frame state=%d",
+		list_empty(&ctx->src_queue) ? "null" : "normal", ctx->state);
 	s5p_mfc_encode_one_frame(ctx);
 	return 0;
 }
@@ -1345,6 +1360,8 @@ void s5p_mfc_try_run(struct s5p_mfc_dev *dev)
 	} else if (ctx->type == MFCINST_ENCODER) {
 		switch (ctx->state) {
 		case MFCINST_FINISHING:
+			s5p_mfc_run_enc_frame(ctx);
+			break;
 		case MFCINST_RUNNING:
 			ret = s5p_mfc_run_enc_frame(ctx);
 			break;
-- 
1.7.0.4

Re: [PATCH 2/2] s5p-mfc: added encoder support for end of stream handling

[Sylwester Nawrocki]

Hi Andrzej,

just a few nit picks below...

On 05/22/2012 05:33 PM, Andrzej Hajda wrote:
> s5p-mfc encoder after receiving buffer with flag V4L2_BUF_FLAG_EOS
> will put all buffers cached in device into capture queue.
> It will indicate end of encoded stream by providing empty buffer.
> 
> Signed-off-by: Andrzej Hajda<a.hajda@samsung.com>
> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> ---
...
> +static void s5p_mfc_handle_complete(struct s5p_mfc_ctx *ctx,
> +				 unsigned int reason, unsigned int err)
> +{
> +	struct s5p_mfc_dev *dev = ctx->dev;
> +	struct s5p_mfc_buf *mb_entry;
> +	unsigned long flags;
> +
> +	mfc_debug(2, "Stream completed");
> +
> +	s5p_mfc_clear_int_flags(dev);
> +	ctx->int_type = reason;
> +	ctx->int_err = err;
> +
> +	spin_lock_irqsave(&dev->irqlock, flags);
> +	ctx->state = MFCINST_FINISHED;
> +
> +	if (!list_empty(&ctx->dst_queue)) {
> +		mb_entry = list_entry(ctx->dst_queue.next, struct s5p_mfc_buf,
> +									list);
> +		list_del(&mb_entry->list);
> +		ctx->dst_queue_cnt--;
> +		vb2_set_plane_payload(mb_entry->b, 0, 0);
> +		vb2_buffer_done(mb_entry->b, VB2_BUF_STATE_DONE);
> +	}
> +
> +	spin_unlock_irqrestore(&dev->irqlock, flags);
> +	if (ctx->dst_queue_cnt == 0) {
> +		spin_lock(&dev->condlock);
> +		clear_bit(ctx->num,&dev->ctx_work_bits);
> +		spin_unlock(&dev->condlock);

This looks a bit odd, since clear_bit is atomic and yet we protect it 
with a spinlock.

Perhaps it's worth to add a set_work_bit() function as a pair to existing 
clear_work_bit(),

static void clear_work_bit(struct s5p_mfc_dev *dev, unsigned int num)
{
	int flags;

	spin_lock_irqsave(&dev->condlock, flags);
	dev->ctx_work_bits &= ~BIT(num);
	spin_unlock_irqrestore(&dev->condlock, flags);
}

static void set_work_bit(struct s5p_mfc_dev *dev, unsigned int num)
{
	int flags;

	spin_lock_irqsave(&dev->condlock, flags);
	dev->ctx_work_bits |= BIT(num);
	spin_unlock_irqrestore(&dev->condlock, flags);
}

and replace all occurrences of spin_lock/set_bit/spin_unlock with it?
It's just a tiny optimization though.

> +	}
> +
> +	if (test_and_clear_bit(0,&dev->hw_lock) == 0)
> +		BUG();

Is it really needed, is it unrecoverable system error ? Wouldn't just
WARN_ON() do ? BUG(); seems an to me overkill here.

> +
> +	s5p_mfc_clock_off();
> +	wake_up(&ctx->queue);
> +}
> +
>   /* Interrupt processing */
>   static irqreturn_t s5p_mfc_irq(int irq, void *priv)
>   {
> @@ -614,6 +653,11 @@ static irqreturn_t s5p_mfc_irq(int irq, void *priv)
>   	case S5P_FIMV_R2H_CMD_INIT_BUFFERS_RET:
>   		s5p_mfc_handle_init_buffers(ctx, reason, err);
>   		break;
> +
> +	case S5P_FIMV_R2H_CMD_ENC_COMPLETE_RET:
> +		s5p_mfc_handle_complete(ctx, reason, err);
> +		break;
> +
>   	default:
>   		mfc_debug(2, "Unknown int reason\n");
>   		s5p_mfc_clear_int_flags(dev);
> diff --git a/drivers/media/video/s5p-mfc/s5p_mfc_enc.c b/drivers/media/video/s5p-mfc/s5p_mfc_enc.c
> index acedb20..8dec186 100644
> --- a/drivers/media/video/s5p-mfc/s5p_mfc_enc.c
> +++ b/drivers/media/video/s5p-mfc/s5p_mfc_enc.c
> @@ -584,7 +584,7 @@ static int s5p_mfc_ctx_ready(struct s5p_mfc_ctx *ctx)
>   		return 1;
>   	/* context is ready to encode remain frames */
>   	if (ctx->state == MFCINST_FINISHING&&
> -		ctx->src_queue_cnt>= 1&&  ctx->dst_queue_cnt>= 1)
> +		ctx->dst_queue_cnt>= 1)
>   		return 1;
>   	mfc_debug(2, "ctx is not ready\n");
>   	return 0;
> @@ -1715,15 +1715,8 @@ static void s5p_mfc_buf_queue(struct vb2_buffer *vb)
>   		mfc_buf =&ctx->src_bufs[vb->v4l2_buf.index];
>   		mfc_buf->used = 0;
>   		spin_lock_irqsave(&dev->irqlock, flags);
> -		if (vb->v4l2_planes[0].bytesused == 0) {
> -			mfc_debug(1, "change state to FINISHING\n");
> -			ctx->state = MFCINST_FINISHING;
> -			vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
> -			cleanup_ref_queue(ctx);
> -		} else {
> -			list_add_tail(&mfc_buf->list,&ctx->src_queue);
> -			ctx->src_queue_cnt++;
> -		}
> +		list_add_tail(&mfc_buf->list,&ctx->src_queue);
> +		ctx->src_queue_cnt++;
>   		spin_unlock_irqrestore(&dev->irqlock, flags);
>   	} else {
>   		mfc_err("unsupported buffer type (%d)\n", vq->type);
> diff --git a/drivers/media/video/s5p-mfc/s5p_mfc_opr.c b/drivers/media/video/s5p-mfc/s5p_mfc_opr.c
> index e6217cb..4bcf93f 100644
> --- a/drivers/media/video/s5p-mfc/s5p_mfc_opr.c
> +++ b/drivers/media/video/s5p-mfc/s5p_mfc_opr.c
> @@ -1081,8 +1081,14 @@ int s5p_mfc_encode_one_frame(struct s5p_mfc_ctx *ctx)
>   	else if (ctx->src_fmt->fourcc == V4L2_PIX_FMT_NV12MT)
>   		mfc_write(dev, 3, S5P_FIMV_ENC_MAP_FOR_CUR);
>   	s5p_mfc_set_shared_buffer(ctx);
> -	mfc_write(dev, (S5P_FIMV_CH_FRAME_START<<  16&  0x70000) |
> -		(ctx->inst_no), S5P_FIMV_SI_CH0_INST_ID);
> +
> +	if (ctx->state == MFCINST_FINISHING)
> +		mfc_write(dev, ((S5P_FIMV_CH_LAST_FRAME&  S5P_FIMV_CH_MASK)<<
> +		S5P_FIMV_CH_SHIFT) | (ctx->inst_no), S5P_FIMV_SI_CH0_INST_ID);

Indentation is a bit misleading here.

> +	else
> +		mfc_write(dev, ((S5P_FIMV_CH_FRAME_START&  S5P_FIMV_CH_MASK)<<
> +		S5P_FIMV_CH_SHIFT) | (ctx->inst_no), S5P_FIMV_SI_CH0_INST_ID);
> +
>   	return 0;
>   }

--
Regards,
Sylwester

Re: [PATCH 0/2] s5p-mfc: added encoder support for end of stream handling

[Hans Verkuil]

Hi Andrzej!

Thanks for the patch, but I do have two questions:

On Tue 22 May 2012 17:33:53 Andrzej Hajda wrote:
> Those patches add end of stream handling for s5p-mfc encoder.
> 
> The first patch was sent already to the list as RFC, but the discussion ended
> without any decision.
> This patch adds new v4l2_buffer flag V4L2_BUF_FLAG_EOS. Below short
> description of this change.
> 
> s5p_mfc is a mem-to-mem MPEG/H263/H264 encoder and it requires that the last
> incoming frame must be processed differently, it means the information about
> the end of the stream driver should receive NOT LATER than the last
> V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE buffer. Common practice
> of sending empty buffer to indicate end-of-stream do not work in such case.
> Setting V4L2_BUF_FLAG_EOS flag for the last V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
> buffer seems to be the most straightforward solution here.
> 
> V4L2_BUF_FLAG_EOS flag should be used by application if driver requires it

How will the application know that?

> and it should be set only on V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE buffers.

Why only for this type?

Regards,

	Hans

> 
> The second patch implements end-of-stream handling in s5p-mfc.
> 
> Comments are welcome
> Andrzej Hajda
> 

Re: [PATCH 0/2] s5p-mfc: added encoder support for end of stream handling

[Andrzej Hajda]

On Wed, 2012-05-23 at 09:43 +0200, Hans Verkuil wrote:
> Hi Andrzej!
> 
> Thanks for the patch, but I do have two questions:
> 
> On Tue 22 May 2012 17:33:53 Andrzej Hajda wrote:
> > Those patches add end of stream handling for s5p-mfc encoder.
> > 
> > The first patch was sent already to the list as RFC, but the discussion ended
> > without any decision.
> > This patch adds new v4l2_buffer flag V4L2_BUF_FLAG_EOS. Below short
> > description of this change.
> > 
> > s5p_mfc is a mem-to-mem MPEG/H263/H264 encoder and it requires that the last
> > incoming frame must be processed differently, it means the information about
> > the end of the stream driver should receive NOT LATER than the last
> > V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE buffer. Common practice
> > of sending empty buffer to indicate end-of-stream do not work in such case.
> > Setting V4L2_BUF_FLAG_EOS flag for the last V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
> > buffer seems to be the most straightforward solution here.
> > 
> > V4L2_BUF_FLAG_EOS flag should be used by application if driver requires it
> 
> How will the application know that?

Application can always set this flag, it will be ignored by drivers not
requiring it.

I see some drawback of this solution - application should know if the
frame enqueued to the driver is the last one. If the application
receives frames to encode form an external source (for example via pipe)
it often does not know if the frame it received is the last one. So to
be able to properly queue frame to the driver it should wait with frame
queuing until it knows there is next frame or end-of-stream is reached,
in such situation it will properly set flag before queuing.

Alternative to "V4L2_BUF_FLAG_EOS" solution is to implement "wait for
next frame" logic directly into the driver. In such case application can
use empty buffer to signal the end of the stream. Driver waits with
frame processing if there are at least two buffers in output queue. Then
it checks if the second buffer is empty if not it process the first
buffer as a normal frame and repeats procedure, if yes it process the
first buffer as the last frame and releases second buffer.

The drawback of this solution is that it wastes resources/space
(additional buffer) and time (delayed encoding).

I am still hesitating which solution is better, any advices?


> > and it should be set only on V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE buffers.
> 
> Why only for this type?

I wanted to say only for output buffers not just output multi-plane. And
why not capture? Explanation below.
Capture buffers are filled by driver, so only drivers could set this
flag. Some devices provides information about the end of the stream
together with the last frame, but some devices provides this info later
(for example s5p-mfc :) ). In the latter case to properly flag the
capture buffer driver should wait for next available frame. Simpler
solution is to use current solution with sending empty buffer to signal
the end of the stream.

Regards
Andrzej Hajda

Re: [PATCH 0/2] s5p-mfc: added encoder support for end of stream handling

[Hans Verkuil]

On Wed 23 May 2012 13:20:03 Andrzej Hajda wrote:
> On Wed, 2012-05-23 at 09:43 +0200, Hans Verkuil wrote:
> > Hi Andrzej!
> > 
> > Thanks for the patch, but I do have two questions:
> > 
> > On Tue 22 May 2012 17:33:53 Andrzej Hajda wrote:
> > > Those patches add end of stream handling for s5p-mfc encoder.
> > > 
> > > The first patch was sent already to the list as RFC, but the discussion ended
> > > without any decision.
> > > This patch adds new v4l2_buffer flag V4L2_BUF_FLAG_EOS. Below short
> > > description of this change.
> > > 
> > > s5p_mfc is a mem-to-mem MPEG/H263/H264 encoder and it requires that the last
> > > incoming frame must be processed differently, it means the information about
> > > the end of the stream driver should receive NOT LATER than the last
> > > V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE buffer. Common practice
> > > of sending empty buffer to indicate end-of-stream do not work in such case.
> > > Setting V4L2_BUF_FLAG_EOS flag for the last V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
> > > buffer seems to be the most straightforward solution here.
> > > 
> > > V4L2_BUF_FLAG_EOS flag should be used by application if driver requires it
> > 
> > How will the application know that?
> 
> Application can always set this flag, it will be ignored by drivers not
> requiring it.

That's going to make it very hard to write generic applications: people will
always forget to set that flag, unless they happen to using your hardware.

> I see some drawback of this solution - application should know if the
> frame enqueued to the driver is the last one. If the application
> receives frames to encode form an external source (for example via pipe)
> it often does not know if the frame it received is the last one. So to
> be able to properly queue frame to the driver it should wait with frame
> queuing until it knows there is next frame or end-of-stream is reached,
> in such situation it will properly set flag before queuing.
> 
> Alternative to "V4L2_BUF_FLAG_EOS" solution is to implement "wait for
> next frame" logic directly into the driver. In such case application can
> use empty buffer to signal the end of the stream. Driver waits with
> frame processing if there are at least two buffers in output queue. Then
> it checks if the second buffer is empty if not it process the first
> buffer as a normal frame and repeats procedure, if yes it process the
> first buffer as the last frame and releases second buffer.

In the current V4L2 API the last output frame is reached when:

1) the filehandle is closed
2) VIDIOC_STREAMOFF is called
3) VIDIOC_ENCODER_CMD is called with V4L2_ENC_CMD_STOP.

The latter is currently only used by MPEG encoders, but it might be an idea
to consider it for your hardware as well. Perhaps a flag like 'stop_after_next_frame'
is needed.

How are cases 1 and 2 handled today?

And what happens if the app sets the EOS flag, and then later queues another
buffer without that flag. Is that frame accepted/rejected/ignored?

I'm trying to understand how the current implementation behaves in corner cases
like those.

> The drawback of this solution is that it wastes resources/space
> (additional buffer) and time (delayed encoding).
> 
> I am still hesitating which solution is better, any advices?
> 
> 
> > > and it should be set only on V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE buffers.
> > 
> > Why only for this type?
> 
> I wanted to say only for output buffers not just output multi-plane. And
> why not capture? Explanation below.
> Capture buffers are filled by driver, so only drivers could set this
> flag. Some devices provides information about the end of the stream
> together with the last frame, but some devices provides this info later
> (for example s5p-mfc :) ). In the latter case to properly flag the
> capture buffer driver should wait for next available frame. Simpler
> solution is to use current solution with sending empty buffer to signal
> the end of the stream.

I don't believe this is documented anywhere. Wouldn't it be better to send
a V4L2_EVENT_EOS event? That's documented and is the way I would expect this
to work.

Regards,

	Hans

Re: [PATCH 0/2] s5p-mfc: added encoder support for end of stream handling

[Andrzej Hajda]

On Wed, 2012-05-23 at 14:28 +0200, Hans Verkuil wrote:
> On Wed 23 May 2012 13:20:03 Andrzej Hajda wrote:
> > On Wed, 2012-05-23 at 09:43 +0200, Hans Verkuil wrote:
> > > Hi Andrzej!
> > > 
> > > Thanks for the patch, but I do have two questions:
> > > 
> > > On Tue 22 May 2012 17:33:53 Andrzej Hajda wrote:
> > > > Those patches add end of stream handling for s5p-mfc encoder.
> > > > 
> > > > The first patch was sent already to the list as RFC, but the discussion ended
> > > > without any decision.
> > > > This patch adds new v4l2_buffer flag V4L2_BUF_FLAG_EOS. Below short
> > > > description of this change.
> > > > 
> > > > s5p_mfc is a mem-to-mem MPEG/H263/H264 encoder and it requires that the last
> > > > incoming frame must be processed differently, it means the information about
> > > > the end of the stream driver should receive NOT LATER than the last
> > > > V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE buffer. Common practice
> > > > of sending empty buffer to indicate end-of-stream do not work in such case.
> > > > Setting V4L2_BUF_FLAG_EOS flag for the last V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
> > > > buffer seems to be the most straightforward solution here.
> > > > 
> > > > V4L2_BUF_FLAG_EOS flag should be used by application if driver requires it
> > > 
> > > How will the application know that?
> > 
> > Application can always set this flag, it will be ignored by drivers not
> > requiring it.
> 
> That's going to make it very hard to write generic applications: people will
> always forget to set that flag, unless they happen to using your hardware.
> 
> > I see some drawback of this solution - application should know if the
> > frame enqueued to the driver is the last one. If the application
> > receives frames to encode form an external source (for example via pipe)
> > it often does not know if the frame it received is the last one. So to
> > be able to properly queue frame to the driver it should wait with frame
> > queuing until it knows there is next frame or end-of-stream is reached,
> > in such situation it will properly set flag before queuing.
> > 
> > Alternative to "V4L2_BUF_FLAG_EOS" solution is to implement "wait for
> > next frame" logic directly into the driver. In such case application can
> > use empty buffer to signal the end of the stream. Driver waits with
> > frame processing if there are at least two buffers in output queue. Then
> > it checks if the second buffer is empty if not it process the first
> > buffer as a normal frame and repeats procedure, if yes it process the
> > first buffer as the last frame and releases second buffer.
> 
> In the current V4L2 API the last output frame is reached when:
> 
> 1) the filehandle is closed
> 2) VIDIOC_STREAMOFF is called
> 3) VIDIOC_ENCODER_CMD is called with V4L2_ENC_CMD_STOP.
> 
> The latter is currently only used by MPEG encoders, but it might be an idea
> to consider it for your hardware as well. Perhaps a flag like 'stop_after_next_frame'
> is needed.

It seemed to me less straightforward - EOS is sent before the last frame
- but I can implement it this way of course.

> 
> How are cases 1 and 2 handled today?
> 

As I lurked into the driver's code it seems it behaves in standard way -
driver waits for device to finish current operation if there is any,
next it releases all buffers.

> And what happens if the app sets the EOS flag, and then later queues another
> buffer without that flag. Is that frame accepted/rejected/ignored?

I have not take care of this situation.

The simplest solution is to reject frames, application in that case
should reopen device to encode next stream if necessary.

Other solution I see is to allow queue output frames but do not process
them by device until device finish producing encoded frames, it would
require device reinitialization.

> 
> I'm trying to understand how the current implementation behaves in corner cases
> like those.
> 
> > The drawback of this solution is that it wastes resources/space
> > (additional buffer) and time (delayed encoding).
> > 
> > I am still hesitating which solution is better, any advices?
> > 
> > 
> > > > and it should be set only on V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE buffers.
> > > 
> > > Why only for this type?
> > 
> > I wanted to say only for output buffers not just output multi-plane. And
> > why not capture? Explanation below.
> > Capture buffers are filled by driver, so only drivers could set this
> > flag. Some devices provides information about the end of the stream
> > together with the last frame, but some devices provides this info later
> > (for example s5p-mfc :) ). In the latter case to properly flag the
> > capture buffer driver should wait for next available frame. Simpler
> > solution is to use current solution with sending empty buffer to signal
> > the end of the stream.
> 

> I don't believe this is documented anywhere. Wouldn't it be better to send
> a V4L2_EVENT_EOS event? That's documented and is the way I would expect this
> to work.

OK, I will change the code accordingly.

Regards
Andrzej

Re: [PATCH 1/2] v4l: added V4L2_BUF_FLAG_EOS flag indicating the last frame in the stream

[Mauro Carvalho Chehab]

Em 22-05-2012 12:33, Andrzej Hajda escreveu:
> Some devices requires indicator if the buffer is the last one in the stream.
> Applications and drivers can use this flag in such case.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>   Documentation/DocBook/media/v4l/io.xml          |    7 +++++++
>   Documentation/DocBook/media/v4l/vidioc-qbuf.xml |    2 ++
>   include/linux/videodev2.h                       |    1 +
>   3 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
> index fd6aca2..dcbf1e0 100644
> --- a/Documentation/DocBook/media/v4l/io.xml
> +++ b/Documentation/DocBook/media/v4l/io.xml
> @@ -956,6 +956,13 @@ Typically applications shall use this flag for output buffers if the data
>   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_EOS</constant></entry>
> +	    <entry>0x2000</entry>
> +	    <entry>Application should set this flag in the output buffer
> +in order to inform the driver about the last frame of the stream. Some
> +drivers may require it to properly finish processing the stream.</entry>

This breaks backward compatibility, as applications written before this change
won't set this flag.

> +	  </row>
>   	</tbody>
>         </tgroup>
>       </table>
> diff --git a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
> index 9caa49a..ad49f7d 100644
> --- a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
> +++ b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
> @@ -76,6 +76,8 @@ supports capturing from specific video inputs and you want to specify a video
>   input, then <structfield>flags</structfield> should be set to
>   <constant>V4L2_BUF_FLAG_INPUT</constant> and the field
>   <structfield>input</structfield> must be initialized to the desired input.
> +Some drivers expects applications set <constant>V4L2_BUF_FLAG_EOS</constant>
> +flag on the last buffer of the stream.
>   The <structfield>reserved</structfield> field must be set to 0. When using
>   the <link linkend="planar-apis">multi-planar API</link>, the
>   <structfield>m.planes</structfield> field must contain a userspace pointer
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index 370d111..e44a7cd 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -676,6 +676,7 @@ struct v4l2_buffer {
>   /* Cache handling flags */
>   #define V4L2_BUF_FLAG_NO_CACHE_INVALIDATE	0x0800
>   #define V4L2_BUF_FLAG_NO_CACHE_CLEAN		0x1000
> +#define V4L2_BUF_FLAG_EOS	0x2000	/* The last buffer in the stream */
>   
>   /*
>    *	O V E R L A Y   P R E V I E W
> 

Re: [PATCH 1/2] v4l: added V4L2_BUF_FLAG_EOS flag indicating the last frame in the stream

[Andrzej Hajda]

On Mon, 2012-06-18 at 08:24 -0300, Mauro Carvalho Chehab wrote:
> Em 22-05-2012 12:33, Andrzej Hajda escreveu:
> > Some devices requires indicator if the buffer is the last one in the stream.
> > Applications and drivers can use this flag in such case.
> > 
> > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> >   Documentation/DocBook/media/v4l/io.xml          |    7 +++++++
> >   Documentation/DocBook/media/v4l/vidioc-qbuf.xml |    2 ++
> >   include/linux/videodev2.h                       |    1 +
> >   3 files changed, 10 insertions(+), 0 deletions(-)
> > 
> > diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
> > index fd6aca2..dcbf1e0 100644
> > --- a/Documentation/DocBook/media/v4l/io.xml
> > +++ b/Documentation/DocBook/media/v4l/io.xml
> > @@ -956,6 +956,13 @@ Typically applications shall use this flag for output buffers if the data
> >   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_EOS</constant></entry>
> > +	    <entry>0x2000</entry>
> > +	    <entry>Application should set this flag in the output buffer
> > +in order to inform the driver about the last frame of the stream. Some
> > +drivers may require it to properly finish processing the stream.</entry>
> 
> This breaks backward compatibility, as applications written before this change
> won't set this flag.

I am preparing a new patch which will use VIDIOC_ENCODER_CMD with
command V4L2_ENC_CMD_STOP and a new flag
V4L2_ENC_CMD_STOP_AFTER_NEXT_FRAME, according to suggestions by Hans
Verkuil. Discussion is at thread started from the parent email (subject
"[PATCH 0/2] s5p-mfc: added encoder support for end of stream
handling").
> 
> > +	  </row>
> >   	</tbody>
> >         </tgroup>
> >       </table>
> > diff --git a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
> > index 9caa49a..ad49f7d 100644
> > --- a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
> > +++ b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
> > @@ -76,6 +76,8 @@ supports capturing from specific video inputs and you want to specify a video
> >   input, then <structfield>flags</structfield> should be set to
> >   <constant>V4L2_BUF_FLAG_INPUT</constant> and the field
> >   <structfield>input</structfield> must be initialized to the desired input.
> > +Some drivers expects applications set <constant>V4L2_BUF_FLAG_EOS</constant>
> > +flag on the last buffer of the stream.
> >   The <structfield>reserved</structfield> field must be set to 0. When using
> >   the <link linkend="planar-apis">multi-planar API</link>, the
> >   <structfield>m.planes</structfield> field must contain a userspace pointer
> > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> > index 370d111..e44a7cd 100644
> > --- a/include/linux/videodev2.h
> > +++ b/include/linux/videodev2.h
> > @@ -676,6 +676,7 @@ struct v4l2_buffer {
> >   /* Cache handling flags */
> >   #define V4L2_BUF_FLAG_NO_CACHE_INVALIDATE	0x0800
> >   #define V4L2_BUF_FLAG_NO_CACHE_CLEAN		0x1000
> > +#define V4L2_BUF_FLAG_EOS	0x2000	/* The last buffer in the stream */
> >   
> >   /*
> >    *	O V E R L A Y   P R E V I E W
> > 
> 
> 
Regards
Andrzej

Re: [PATCH 1/2] v4l: added V4L2_BUF_FLAG_EOS flag indicating the last frame in the stream

[Mauro Carvalho Chehab]

Em 18-06-2012 08:54, Andrzej Hajda escreveu:
> On Mon, 2012-06-18 at 08:24 -0300, Mauro Carvalho Chehab wrote:
>> Em 22-05-2012 12:33, Andrzej Hajda escreveu:
>>> Some devices requires indicator if the buffer is the last one in the stream.
>>> Applications and drivers can use this flag in such case.
>>>
>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>> ---
>>>    Documentation/DocBook/media/v4l/io.xml          |    7 +++++++
>>>    Documentation/DocBook/media/v4l/vidioc-qbuf.xml |    2 ++
>>>    include/linux/videodev2.h                       |    1 +
>>>    3 files changed, 10 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
>>> index fd6aca2..dcbf1e0 100644
>>> --- a/Documentation/DocBook/media/v4l/io.xml
>>> +++ b/Documentation/DocBook/media/v4l/io.xml
>>> @@ -956,6 +956,13 @@ Typically applications shall use this flag for output buffers if the data
>>>    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_EOS</constant></entry>
>>> +	    <entry>0x2000</entry>
>>> +	    <entry>Application should set this flag in the output buffer
>>> +in order to inform the driver about the last frame of the stream. Some
>>> +drivers may require it to properly finish processing the stream.</entry>
>>
>> This breaks backward compatibility, as applications written before this change
>> won't set this flag.
> 
> I am preparing a new patch which will use VIDIOC_ENCODER_CMD with
> command V4L2_ENC_CMD_STOP and a new flag
> V4L2_ENC_CMD_STOP_AFTER_NEXT_FRAME, according to suggestions by Hans
> Verkuil. Discussion is at thread started from the parent email (subject
> "[PATCH 0/2] s5p-mfc: added encoder support for end of stream
> handling").

So?

The point is: new changes should not break backward compatibility,
otherwise a regression is introduced, and such patch should be nacked.

So, newer functionality should carefully be introduced in a way that it
won't make older apps to break.

That means that you'll need to work on some design that an EOS flag will
be used only if the application is known to be compiled with EOS flag support.

Regards,
Mauro

>>
>>> +	  </row>
>>>    	</tbody>
>>>          </tgroup>
>>>        </table>
>>> diff --git a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
>>> index 9caa49a..ad49f7d 100644
>>> --- a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
>>> +++ b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
>>> @@ -76,6 +76,8 @@ supports capturing from specific video inputs and you want to specify a video
>>>    input, then <structfield>flags</structfield> should be set to
>>>    <constant>V4L2_BUF_FLAG_INPUT</constant> and the field
>>>    <structfield>input</structfield> must be initialized to the desired input.
>>> +Some drivers expects applications set <constant>V4L2_BUF_FLAG_EOS</constant>
>>> +flag on the last buffer of the stream.
>>>    The <structfield>reserved</structfield> field must be set to 0. When using
>>>    the <link linkend="planar-apis">multi-planar API</link>, the
>>>    <structfield>m.planes</structfield> field must contain a userspace pointer
>>> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
>>> index 370d111..e44a7cd 100644
>>> --- a/include/linux/videodev2.h
>>> +++ b/include/linux/videodev2.h
>>> @@ -676,6 +676,7 @@ struct v4l2_buffer {
>>>    /* Cache handling flags */
>>>    #define V4L2_BUF_FLAG_NO_CACHE_INVALIDATE	0x0800
>>>    #define V4L2_BUF_FLAG_NO_CACHE_CLEAN		0x1000
>>> +#define V4L2_BUF_FLAG_EOS	0x2000	/* The last buffer in the stream */
>>>    
>>>    /*
>>>     *	O V E R L A Y   P R E V I E W
>>>
>>
>>
> Regards
> Andrzej
> 
>