[PATCH 0/2] vicodec: a couple fixes towards spec compliancy

[PATCH 0/2] vicodec: a couple fixes towards spec compliancy

[Ezequiel Garcia]

Given the stateful codec specification is still a moving target,
it doesn't make any sense to try to comply fully with it.

On the other side, we can still comply with some basic userspace
expectations, with just a couple small changes.

This series implements proper resolution changes propagation,
and fixes the CMD_STOP so it actually works.

The intention of this series is to be able to test this driver
using already existing userspace, gstreamer in particular.
With this changes, it's possible to construct variations of
this pipeline:

  gst-launch-1.0 videotestsrc ! v4l2fwhtenc ! v4l2fwhtdec ! fakevideosink

Ezequiel Garcia (2):
  vicodec: Have decoder propagate changes to the CAPTURE queue
  vicodec: Implement spec-compliant stop command

 drivers/media/platform/vicodec/vicodec-core.c | 95 ++++++++++++-------
 1 file changed, 59 insertions(+), 36 deletions(-)

-- 
2.19.1

[PATCH 1/2] vicodec: Have decoder propagate changes to the CAPTURE queue

[Ezequiel Garcia]

The decoder interface (not yet merged) specifies that
width and height values set on the OUTPUT queue, must
be propagated to the CAPTURE queue.

This is not enough to comply with the specification,
which would require to properly support stream resolution
changes detection and notification.

However, it's a relatively small change, which fixes behavior
required by some applications such as gstreamer.

With this change, it's possible to run a simple T(T⁻¹) pipeline:

gst-launch-1.0 videotestsrc ! v4l2fwhtenc ! v4l2fwhtdec ! fakevideosink

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/platform/vicodec/vicodec-core.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/media/platform/vicodec/vicodec-core.c b/drivers/media/platform/vicodec/vicodec-core.c
index 1eb9132bfc85..a2c487b4b80d 100644
--- a/drivers/media/platform/vicodec/vicodec-core.c
+++ b/drivers/media/platform/vicodec/vicodec-core.c
@@ -673,6 +673,13 @@ static int vidioc_s_fmt(struct vicodec_ctx *ctx, struct v4l2_format *f)
 		q_data->width = pix->width;
 		q_data->height = pix->height;
 		q_data->sizeimage = pix->sizeimage;
+
+		/* Propagate changes to CAPTURE queue */
+		if (!ctx->is_enc && V4L2_TYPE_IS_OUTPUT(f->type)) {
+			ctx->q_data[V4L2_M2M_DST].width = pix->width;
+			ctx->q_data[V4L2_M2M_DST].height = pix->height;
+			ctx->q_data[V4L2_M2M_DST].sizeimage = pix->sizeimage;
+		}
 		break;
 	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
 	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
@@ -693,6 +700,14 @@ static int vidioc_s_fmt(struct vicodec_ctx *ctx, struct v4l2_format *f)
 		q_data->width = pix_mp->width;
 		q_data->height = pix_mp->height;
 		q_data->sizeimage = pix_mp->plane_fmt[0].sizeimage;
+
+		/* Propagate changes to CAPTURE queue */
+		if (!ctx->is_enc && V4L2_TYPE_IS_OUTPUT(f->type)) {
+			ctx->q_data[V4L2_M2M_DST].width = pix_mp->width;
+			ctx->q_data[V4L2_M2M_DST].height = pix_mp->height;
+			ctx->q_data[V4L2_M2M_DST].sizeimage =
+				pix_mp->plane_fmt[0].sizeimage;
+		}
 		break;
 	default:
 		return -EINVAL;
-- 
2.19.1

[PATCH 2/2] vicodec: Implement spec-compliant stop command

[Ezequiel Garcia]

Set up a statically-allocated, dummy buffer to
be used as flush buffer, which signals
a encoding (or decoding) stop.

When the flush buffer is queued to the OUTPUT queue,
the driver will send an V4L2_EVENT_EOS event, and
mark the CAPTURE buffer with V4L2_BUF_FLAG_LAST.

With this change, it's possible to run a pipeline to completion:

gst-launch-1.0 videotestsrc num-buffers=10 ! v4l2fwhtenc ! v4l2fwhtdec ! fakevideosink

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/platform/vicodec/vicodec-core.c | 80 ++++++++++---------
 1 file changed, 44 insertions(+), 36 deletions(-)

diff --git a/drivers/media/platform/vicodec/vicodec-core.c b/drivers/media/platform/vicodec/vicodec-core.c
index a2c487b4b80d..4ed4dae10e30 100644
--- a/drivers/media/platform/vicodec/vicodec-core.c
+++ b/drivers/media/platform/vicodec/vicodec-core.c
@@ -113,7 +113,7 @@ struct vicodec_ctx {
 	struct v4l2_ctrl_handler hdl;
 
 	struct vb2_v4l2_buffer *last_src_buf;
-	struct vb2_v4l2_buffer *last_dst_buf;
+	struct vb2_v4l2_buffer  flush_buf;
 
 	/* Source and destination queue data */
 	struct vicodec_q_data   q_data[2];
@@ -220,6 +220,7 @@ static void device_run(void *priv)
 	struct vicodec_dev *dev = ctx->dev;
 	struct vb2_v4l2_buffer *src_buf, *dst_buf;
 	struct vicodec_q_data *q_out;
+	bool flushing;
 	u32 state;
 
 	src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
@@ -227,26 +228,36 @@ static void device_run(void *priv)
 	q_out = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT);
 
 	state = VB2_BUF_STATE_DONE;
-	if (device_process(ctx, src_buf, dst_buf))
+
+	flushing = (src_buf == &ctx->flush_buf);
+	if (!flushing && device_process(ctx, src_buf, dst_buf))
 		state = VB2_BUF_STATE_ERROR;
-	ctx->last_dst_buf = dst_buf;
 
 	spin_lock(ctx->lock);
-	if (!ctx->comp_has_next_frame && src_buf == ctx->last_src_buf) {
+	if (!flushing) {
+		if (!ctx->comp_has_next_frame && src_buf == ctx->last_src_buf) {
+			dst_buf->flags |= V4L2_BUF_FLAG_LAST;
+			v4l2_event_queue_fh(&ctx->fh, &eos_event);
+		}
+
+		if (ctx->is_enc) {
+			src_buf->sequence = q_out->sequence++;
+			src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
+			v4l2_m2m_buf_done(src_buf, state);
+		} else if (vb2_get_plane_payload(&src_buf->vb2_buf, 0)
+				== ctx->cur_buf_offset) {
+			src_buf->sequence = q_out->sequence++;
+			src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
+			v4l2_m2m_buf_done(src_buf, state);
+			ctx->cur_buf_offset = 0;
+			ctx->comp_has_next_frame = false;
+		}
+	} else {
+		src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
+		vb2_set_plane_payload(&dst_buf->vb2_buf, 0, 0);
 		dst_buf->flags |= V4L2_BUF_FLAG_LAST;
 		v4l2_event_queue_fh(&ctx->fh, &eos_event);
 	}
-	if (ctx->is_enc) {
-		src_buf->sequence = q_out->sequence++;
-		src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
-		v4l2_m2m_buf_done(src_buf, state);
-	} else if (vb2_get_plane_payload(&src_buf->vb2_buf, 0) == ctx->cur_buf_offset) {
-		src_buf->sequence = q_out->sequence++;
-		src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
-		v4l2_m2m_buf_done(src_buf, state);
-		ctx->cur_buf_offset = 0;
-		ctx->comp_has_next_frame = false;
-	}
 	v4l2_m2m_buf_done(dst_buf, state);
 	ctx->comp_size = 0;
 	ctx->comp_magic_cnt = 0;
@@ -293,6 +304,8 @@ static int job_ready(void *priv)
 	src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
 	if (!src_buf)
 		return 0;
+	if (src_buf == &ctx->flush_buf)
+		return 1;
 	p_out = vb2_plane_vaddr(&src_buf->vb2_buf, 0);
 	sz = vb2_get_plane_payload(&src_buf->vb2_buf, 0);
 	p = p_out + ctx->cur_buf_offset;
@@ -770,21 +783,6 @@ static int vidioc_s_fmt_vid_out(struct file *file, void *priv,
 	return ret;
 }
 
-static void vicodec_mark_last_buf(struct vicodec_ctx *ctx)
-{
-	static const struct v4l2_event eos_event = {
-		.type = V4L2_EVENT_EOS
-	};
-
-	spin_lock(ctx->lock);
-	ctx->last_src_buf = v4l2_m2m_last_src_buf(ctx->fh.m2m_ctx);
-	if (!ctx->last_src_buf && ctx->last_dst_buf) {
-		ctx->last_dst_buf->flags |= V4L2_BUF_FLAG_LAST;
-		v4l2_event_queue_fh(&ctx->fh, &eos_event);
-	}
-	spin_unlock(ctx->lock);
-}
-
 static int vicodec_try_encoder_cmd(struct file *file, void *fh,
 				struct v4l2_encoder_cmd *ec)
 {
@@ -806,8 +804,8 @@ static int vicodec_encoder_cmd(struct file *file, void *fh,
 	ret = vicodec_try_encoder_cmd(file, fh, ec);
 	if (ret < 0)
 		return ret;
-
-	vicodec_mark_last_buf(ctx);
+	v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, &ctx->flush_buf);
+	v4l2_m2m_try_schedule(ctx->fh.m2m_ctx);
 	return 0;
 }
 
@@ -835,8 +833,8 @@ static int vicodec_decoder_cmd(struct file *file, void *fh,
 	ret = vicodec_try_decoder_cmd(file, fh, dc);
 	if (ret < 0)
 		return ret;
-
-	vicodec_mark_last_buf(ctx);
+	v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, &ctx->flush_buf);
+	v4l2_m2m_try_schedule(ctx->fh.m2m_ctx);
 	return 0;
 }
 
@@ -991,7 +989,7 @@ static void vicodec_return_bufs(struct vb2_queue *q, u32 state)
 			vbuf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
 		else
 			vbuf = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
-		if (vbuf == NULL)
+		if (!vbuf || vbuf == &ctx->flush_buf)
 			return;
 		spin_lock(ctx->lock);
 		v4l2_m2m_buf_done(vbuf, state);
@@ -1031,7 +1029,6 @@ static int vicodec_start_streaming(struct vb2_queue *q,
 	state->ref_frame.cb = state->ref_frame.luma + size;
 	state->ref_frame.cr = state->ref_frame.cb + size / chroma_div;
 	ctx->last_src_buf = NULL;
-	ctx->last_dst_buf = NULL;
 	state->gop_cnt = 0;
 	ctx->cur_buf_offset = 0;
 	ctx->comp_size = 0;
@@ -1158,6 +1155,7 @@ static int vicodec_open(struct file *file)
 	struct vicodec_dev *dev = video_drvdata(file);
 	struct vicodec_ctx *ctx = NULL;
 	struct v4l2_ctrl_handler *hdl;
+	struct vb2_queue *vq;
 	unsigned int size;
 	int rc = 0;
 
@@ -1234,6 +1232,16 @@ static int vicodec_open(struct file *file)
 
 	v4l2_fh_add(&ctx->fh);
 
+	/* Setup a dummy flush buffer, used to signal
+	 * encoding/decoding stop operation. When this buffer
+	 * is queued to the OUTPUT queue, the driver will send
+	 * V4L2_EVENT_EOS and send the last buffer to userspace.
+	 */
+	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, multiplanar ?
+			     V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE :
+			     V4L2_BUF_TYPE_VIDEO_OUTPUT);
+	ctx->flush_buf.vb2_buf.vb2_queue = vq;
+
 open_unlock:
 	mutex_unlock(vfd->lock);
 	return rc;
-- 
2.19.1

Re: [PATCH 1/2] vicodec: Have decoder propagate changes to the CAPTURE queue

[Hans Verkuil]

On 10/18/2018 06:08 PM, Ezequiel Garcia wrote:
> The decoder interface (not yet merged) specifies that
> width and height values set on the OUTPUT queue, must
> be propagated to the CAPTURE queue.
> 
> This is not enough to comply with the specification,
> which would require to properly support stream resolution
> changes detection and notification.
> 
> However, it's a relatively small change, which fixes behavior
> required by some applications such as gstreamer.
> 
> With this change, it's possible to run a simple T(T⁻¹) pipeline:
> 
> gst-launch-1.0 videotestsrc ! v4l2fwhtenc ! v4l2fwhtdec ! fakevideosink
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/media/platform/vicodec/vicodec-core.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/media/platform/vicodec/vicodec-core.c b/drivers/media/platform/vicodec/vicodec-core.c
> index 1eb9132bfc85..a2c487b4b80d 100644
> --- a/drivers/media/platform/vicodec/vicodec-core.c
> +++ b/drivers/media/platform/vicodec/vicodec-core.c
> @@ -673,6 +673,13 @@ static int vidioc_s_fmt(struct vicodec_ctx *ctx, struct v4l2_format *f)
>  		q_data->width = pix->width;
>  		q_data->height = pix->height;
>  		q_data->sizeimage = pix->sizeimage;
> +
> +		/* Propagate changes to CAPTURE queue */
> +		if (!ctx->is_enc && V4L2_TYPE_IS_OUTPUT(f->type)) {

Do we need !ctx->is_enc? Isn't this the same for both decoder and encoder?

> +			ctx->q_data[V4L2_M2M_DST].width = pix->width;
> +			ctx->q_data[V4L2_M2M_DST].height = pix->height;
> +			ctx->q_data[V4L2_M2M_DST].sizeimage = pix->sizeimage;

This is wrong: you are copying the sizeimage for the compressed format as the
sizeimage for the raw format, which is quite different.

I think you need to make a little helper function that can update the width/height
of a particular queue and that can calculate the sizeimage correctly.

Regards,

	Hans

> +		}
>  		break;
>  	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
>  	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> @@ -693,6 +700,14 @@ static int vidioc_s_fmt(struct vicodec_ctx *ctx, struct v4l2_format *f)
>  		q_data->width = pix_mp->width;
>  		q_data->height = pix_mp->height;
>  		q_data->sizeimage = pix_mp->plane_fmt[0].sizeimage;
> +
> +		/* Propagate changes to CAPTURE queue */
> +		if (!ctx->is_enc && V4L2_TYPE_IS_OUTPUT(f->type)) {
> +			ctx->q_data[V4L2_M2M_DST].width = pix_mp->width;
> +			ctx->q_data[V4L2_M2M_DST].height = pix_mp->height;
> +			ctx->q_data[V4L2_M2M_DST].sizeimage =
> +				pix_mp->plane_fmt[0].sizeimage;
> +		}
>  		break;
>  	default:
>  		return -EINVAL;
> 

Re: [PATCH 2/2] vicodec: Implement spec-compliant stop command

[Hans Verkuil]

On 10/18/2018 06:08 PM, Ezequiel Garcia wrote:
> Set up a statically-allocated, dummy buffer to
> be used as flush buffer, which signals
> a encoding (or decoding) stop.
> 
> When the flush buffer is queued to the OUTPUT queue,
> the driver will send an V4L2_EVENT_EOS event, and
> mark the CAPTURE buffer with V4L2_BUF_FLAG_LAST.

I'm confused. What is the current driver doing wrong? It is already
setting the LAST flag AFAIK. I don't see why a dummy buffer is needed.

Regards,

	Hans

> 
> With this change, it's possible to run a pipeline to completion:
> 
> gst-launch-1.0 videotestsrc num-buffers=10 ! v4l2fwhtenc ! v4l2fwhtdec ! fakevideosink
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/media/platform/vicodec/vicodec-core.c | 80 ++++++++++---------
>  1 file changed, 44 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/media/platform/vicodec/vicodec-core.c b/drivers/media/platform/vicodec/vicodec-core.c
> index a2c487b4b80d..4ed4dae10e30 100644
> --- a/drivers/media/platform/vicodec/vicodec-core.c
> +++ b/drivers/media/platform/vicodec/vicodec-core.c
> @@ -113,7 +113,7 @@ struct vicodec_ctx {
>  	struct v4l2_ctrl_handler hdl;
>  
>  	struct vb2_v4l2_buffer *last_src_buf;
> -	struct vb2_v4l2_buffer *last_dst_buf;
> +	struct vb2_v4l2_buffer  flush_buf;
>  
>  	/* Source and destination queue data */
>  	struct vicodec_q_data   q_data[2];
> @@ -220,6 +220,7 @@ static void device_run(void *priv)
>  	struct vicodec_dev *dev = ctx->dev;
>  	struct vb2_v4l2_buffer *src_buf, *dst_buf;
>  	struct vicodec_q_data *q_out;
> +	bool flushing;
>  	u32 state;
>  
>  	src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> @@ -227,26 +228,36 @@ static void device_run(void *priv)
>  	q_out = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT);
>  
>  	state = VB2_BUF_STATE_DONE;
> -	if (device_process(ctx, src_buf, dst_buf))
> +
> +	flushing = (src_buf == &ctx->flush_buf);
> +	if (!flushing && device_process(ctx, src_buf, dst_buf))
>  		state = VB2_BUF_STATE_ERROR;
> -	ctx->last_dst_buf = dst_buf;
>  
>  	spin_lock(ctx->lock);
> -	if (!ctx->comp_has_next_frame && src_buf == ctx->last_src_buf) {
> +	if (!flushing) {
> +		if (!ctx->comp_has_next_frame && src_buf == ctx->last_src_buf) {
> +			dst_buf->flags |= V4L2_BUF_FLAG_LAST;
> +			v4l2_event_queue_fh(&ctx->fh, &eos_event);
> +		}
> +
> +		if (ctx->is_enc) {
> +			src_buf->sequence = q_out->sequence++;
> +			src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> +			v4l2_m2m_buf_done(src_buf, state);
> +		} else if (vb2_get_plane_payload(&src_buf->vb2_buf, 0)
> +				== ctx->cur_buf_offset) {
> +			src_buf->sequence = q_out->sequence++;
> +			src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> +			v4l2_m2m_buf_done(src_buf, state);
> +			ctx->cur_buf_offset = 0;
> +			ctx->comp_has_next_frame = false;
> +		}
> +	} else {
> +		src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> +		vb2_set_plane_payload(&dst_buf->vb2_buf, 0, 0);
>  		dst_buf->flags |= V4L2_BUF_FLAG_LAST;
>  		v4l2_event_queue_fh(&ctx->fh, &eos_event);
>  	}
> -	if (ctx->is_enc) {
> -		src_buf->sequence = q_out->sequence++;
> -		src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> -		v4l2_m2m_buf_done(src_buf, state);
> -	} else if (vb2_get_plane_payload(&src_buf->vb2_buf, 0) == ctx->cur_buf_offset) {
> -		src_buf->sequence = q_out->sequence++;
> -		src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> -		v4l2_m2m_buf_done(src_buf, state);
> -		ctx->cur_buf_offset = 0;
> -		ctx->comp_has_next_frame = false;
> -	}
>  	v4l2_m2m_buf_done(dst_buf, state);
>  	ctx->comp_size = 0;
>  	ctx->comp_magic_cnt = 0;
> @@ -293,6 +304,8 @@ static int job_ready(void *priv)
>  	src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
>  	if (!src_buf)
>  		return 0;
> +	if (src_buf == &ctx->flush_buf)
> +		return 1;
>  	p_out = vb2_plane_vaddr(&src_buf->vb2_buf, 0);
>  	sz = vb2_get_plane_payload(&src_buf->vb2_buf, 0);
>  	p = p_out + ctx->cur_buf_offset;
> @@ -770,21 +783,6 @@ static int vidioc_s_fmt_vid_out(struct file *file, void *priv,
>  	return ret;
>  }
>  
> -static void vicodec_mark_last_buf(struct vicodec_ctx *ctx)
> -{
> -	static const struct v4l2_event eos_event = {
> -		.type = V4L2_EVENT_EOS
> -	};
> -
> -	spin_lock(ctx->lock);
> -	ctx->last_src_buf = v4l2_m2m_last_src_buf(ctx->fh.m2m_ctx);
> -	if (!ctx->last_src_buf && ctx->last_dst_buf) {
> -		ctx->last_dst_buf->flags |= V4L2_BUF_FLAG_LAST;
> -		v4l2_event_queue_fh(&ctx->fh, &eos_event);
> -	}
> -	spin_unlock(ctx->lock);
> -}
> -
>  static int vicodec_try_encoder_cmd(struct file *file, void *fh,
>  				struct v4l2_encoder_cmd *ec)
>  {
> @@ -806,8 +804,8 @@ static int vicodec_encoder_cmd(struct file *file, void *fh,
>  	ret = vicodec_try_encoder_cmd(file, fh, ec);
>  	if (ret < 0)
>  		return ret;
> -
> -	vicodec_mark_last_buf(ctx);
> +	v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, &ctx->flush_buf);
> +	v4l2_m2m_try_schedule(ctx->fh.m2m_ctx);
>  	return 0;
>  }
>  
> @@ -835,8 +833,8 @@ static int vicodec_decoder_cmd(struct file *file, void *fh,
>  	ret = vicodec_try_decoder_cmd(file, fh, dc);
>  	if (ret < 0)
>  		return ret;
> -
> -	vicodec_mark_last_buf(ctx);
> +	v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, &ctx->flush_buf);
> +	v4l2_m2m_try_schedule(ctx->fh.m2m_ctx);
>  	return 0;
>  }
>  
> @@ -991,7 +989,7 @@ static void vicodec_return_bufs(struct vb2_queue *q, u32 state)
>  			vbuf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
>  		else
>  			vbuf = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> -		if (vbuf == NULL)
> +		if (!vbuf || vbuf == &ctx->flush_buf)
>  			return;
>  		spin_lock(ctx->lock);
>  		v4l2_m2m_buf_done(vbuf, state);
> @@ -1031,7 +1029,6 @@ static int vicodec_start_streaming(struct vb2_queue *q,
>  	state->ref_frame.cb = state->ref_frame.luma + size;
>  	state->ref_frame.cr = state->ref_frame.cb + size / chroma_div;
>  	ctx->last_src_buf = NULL;
> -	ctx->last_dst_buf = NULL;
>  	state->gop_cnt = 0;
>  	ctx->cur_buf_offset = 0;
>  	ctx->comp_size = 0;
> @@ -1158,6 +1155,7 @@ static int vicodec_open(struct file *file)
>  	struct vicodec_dev *dev = video_drvdata(file);
>  	struct vicodec_ctx *ctx = NULL;
>  	struct v4l2_ctrl_handler *hdl;
> +	struct vb2_queue *vq;
>  	unsigned int size;
>  	int rc = 0;
>  
> @@ -1234,6 +1232,16 @@ static int vicodec_open(struct file *file)
>  
>  	v4l2_fh_add(&ctx->fh);
>  
> +	/* Setup a dummy flush buffer, used to signal
> +	 * encoding/decoding stop operation. When this buffer
> +	 * is queued to the OUTPUT queue, the driver will send
> +	 * V4L2_EVENT_EOS and send the last buffer to userspace.
> +	 */
> +	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, multiplanar ?
> +			     V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE :
> +			     V4L2_BUF_TYPE_VIDEO_OUTPUT);
> +	ctx->flush_buf.vb2_buf.vb2_queue = vq;
> +
>  open_unlock:
>  	mutex_unlock(vfd->lock);
>  	return rc;
> 

Re: [PATCH 2/2] vicodec: Implement spec-compliant stop command

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 7755 bytes --]

Le vendredi 19 octobre 2018 à 09:28 +0200, Hans Verkuil a écrit :
> On 10/18/2018 06:08 PM, Ezequiel Garcia wrote:
> > Set up a statically-allocated, dummy buffer to
> > be used as flush buffer, which signals
> > a encoding (or decoding) stop.
> > 
> > When the flush buffer is queued to the OUTPUT queue,
> > the driver will send an V4L2_EVENT_EOS event, and
> > mark the CAPTURE buffer with V4L2_BUF_FLAG_LAST.
> 
> I'm confused. What is the current driver doing wrong? It is already
> setting the LAST flag AFAIK. I don't see why a dummy buffer is
> needed.

I'm not sure of this patch either. It seems to trigger the legacy
"empty payload" buffer case. Driver should mark the last buffer, and
then following poll should return EPIPE. Maybe it's the later that
isn't respected ?

> 
> Regards,
> 
> 	Hans
> 
> > 
> > With this change, it's possible to run a pipeline to completion:
> > 
> > gst-launch-1.0 videotestsrc num-buffers=10 ! v4l2fwhtenc !
> > v4l2fwhtdec ! fakevideosink
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  drivers/media/platform/vicodec/vicodec-core.c | 80 ++++++++++-----
> > ----
> >  1 file changed, 44 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/media/platform/vicodec/vicodec-core.c
> > b/drivers/media/platform/vicodec/vicodec-core.c
> > index a2c487b4b80d..4ed4dae10e30 100644
> > --- a/drivers/media/platform/vicodec/vicodec-core.c
> > +++ b/drivers/media/platform/vicodec/vicodec-core.c
> > @@ -113,7 +113,7 @@ struct vicodec_ctx {
> >  	struct v4l2_ctrl_handler hdl;
> >  
> >  	struct vb2_v4l2_buffer *last_src_buf;
> > -	struct vb2_v4l2_buffer *last_dst_buf;
> > +	struct vb2_v4l2_buffer  flush_buf;
> >  
> >  	/* Source and destination queue data */
> >  	struct vicodec_q_data   q_data[2];
> > @@ -220,6 +220,7 @@ static void device_run(void *priv)
> >  	struct vicodec_dev *dev = ctx->dev;
> >  	struct vb2_v4l2_buffer *src_buf, *dst_buf;
> >  	struct vicodec_q_data *q_out;
> > +	bool flushing;
> >  	u32 state;
> >  
> >  	src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> > @@ -227,26 +228,36 @@ static void device_run(void *priv)
> >  	q_out = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT);
> >  
> >  	state = VB2_BUF_STATE_DONE;
> > -	if (device_process(ctx, src_buf, dst_buf))
> > +
> > +	flushing = (src_buf == &ctx->flush_buf);
> > +	if (!flushing && device_process(ctx, src_buf, dst_buf))
> >  		state = VB2_BUF_STATE_ERROR;
> > -	ctx->last_dst_buf = dst_buf;
> >  
> >  	spin_lock(ctx->lock);
> > -	if (!ctx->comp_has_next_frame && src_buf == ctx->last_src_buf)
> > {
> > +	if (!flushing) {
> > +		if (!ctx->comp_has_next_frame && src_buf == ctx-
> > >last_src_buf) {
> > +			dst_buf->flags |= V4L2_BUF_FLAG_LAST;
> > +			v4l2_event_queue_fh(&ctx->fh, &eos_event);
> > +		}
> > +
> > +		if (ctx->is_enc) {
> > +			src_buf->sequence = q_out->sequence++;
> > +			src_buf = v4l2_m2m_src_buf_remove(ctx-
> > >fh.m2m_ctx);
> > +			v4l2_m2m_buf_done(src_buf, state);
> > +		} else if (vb2_get_plane_payload(&src_buf->vb2_buf, 0)
> > +				== ctx->cur_buf_offset) {
> > +			src_buf->sequence = q_out->sequence++;
> > +			src_buf = v4l2_m2m_src_buf_remove(ctx-
> > >fh.m2m_ctx);
> > +			v4l2_m2m_buf_done(src_buf, state);
> > +			ctx->cur_buf_offset = 0;
> > +			ctx->comp_has_next_frame = false;
> > +		}
> > +	} else {
> > +		src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> > +		vb2_set_plane_payload(&dst_buf->vb2_buf, 0, 0);
> >  		dst_buf->flags |= V4L2_BUF_FLAG_LAST;
> >  		v4l2_event_queue_fh(&ctx->fh, &eos_event);
> >  	}
> > -	if (ctx->is_enc) {
> > -		src_buf->sequence = q_out->sequence++;
> > -		src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> > -		v4l2_m2m_buf_done(src_buf, state);
> > -	} else if (vb2_get_plane_payload(&src_buf->vb2_buf, 0) == ctx-
> > >cur_buf_offset) {
> > -		src_buf->sequence = q_out->sequence++;
> > -		src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> > -		v4l2_m2m_buf_done(src_buf, state);
> > -		ctx->cur_buf_offset = 0;
> > -		ctx->comp_has_next_frame = false;
> > -	}
> >  	v4l2_m2m_buf_done(dst_buf, state);
> >  	ctx->comp_size = 0;
> >  	ctx->comp_magic_cnt = 0;
> > @@ -293,6 +304,8 @@ static int job_ready(void *priv)
> >  	src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> >  	if (!src_buf)
> >  		return 0;
> > +	if (src_buf == &ctx->flush_buf)
> > +		return 1;
> >  	p_out = vb2_plane_vaddr(&src_buf->vb2_buf, 0);
> >  	sz = vb2_get_plane_payload(&src_buf->vb2_buf, 0);
> >  	p = p_out + ctx->cur_buf_offset;
> > @@ -770,21 +783,6 @@ static int vidioc_s_fmt_vid_out(struct file
> > *file, void *priv,
> >  	return ret;
> >  }
> >  
> > -static void vicodec_mark_last_buf(struct vicodec_ctx *ctx)
> > -{
> > -	static const struct v4l2_event eos_event = {
> > -		.type = V4L2_EVENT_EOS
> > -	};
> > -
> > -	spin_lock(ctx->lock);
> > -	ctx->last_src_buf = v4l2_m2m_last_src_buf(ctx->fh.m2m_ctx);
> > -	if (!ctx->last_src_buf && ctx->last_dst_buf) {
> > -		ctx->last_dst_buf->flags |= V4L2_BUF_FLAG_LAST;
> > -		v4l2_event_queue_fh(&ctx->fh, &eos_event);
> > -	}
> > -	spin_unlock(ctx->lock);
> > -}
> > -
> >  static int vicodec_try_encoder_cmd(struct file *file, void *fh,
> >  				struct v4l2_encoder_cmd *ec)
> >  {
> > @@ -806,8 +804,8 @@ static int vicodec_encoder_cmd(struct file
> > *file, void *fh,
> >  	ret = vicodec_try_encoder_cmd(file, fh, ec);
> >  	if (ret < 0)
> >  		return ret;
> > -
> > -	vicodec_mark_last_buf(ctx);
> > +	v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, &ctx->flush_buf);
> > +	v4l2_m2m_try_schedule(ctx->fh.m2m_ctx);
> >  	return 0;
> >  }
> >  
> > @@ -835,8 +833,8 @@ static int vicodec_decoder_cmd(struct file
> > *file, void *fh,
> >  	ret = vicodec_try_decoder_cmd(file, fh, dc);
> >  	if (ret < 0)
> >  		return ret;
> > -
> > -	vicodec_mark_last_buf(ctx);
> > +	v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, &ctx->flush_buf);
> > +	v4l2_m2m_try_schedule(ctx->fh.m2m_ctx);
> >  	return 0;
> >  }
> >  
> > @@ -991,7 +989,7 @@ static void vicodec_return_bufs(struct
> > vb2_queue *q, u32 state)
> >  			vbuf = v4l2_m2m_src_buf_remove(ctx-
> > >fh.m2m_ctx);
> >  		else
> >  			vbuf = v4l2_m2m_dst_buf_remove(ctx-
> > >fh.m2m_ctx);
> > -		if (vbuf == NULL)
> > +		if (!vbuf || vbuf == &ctx->flush_buf)
> >  			return;
> >  		spin_lock(ctx->lock);
> >  		v4l2_m2m_buf_done(vbuf, state);
> > @@ -1031,7 +1029,6 @@ static int vicodec_start_streaming(struct
> > vb2_queue *q,
> >  	state->ref_frame.cb = state->ref_frame.luma + size;
> >  	state->ref_frame.cr = state->ref_frame.cb + size / chroma_div;
> >  	ctx->last_src_buf = NULL;
> > -	ctx->last_dst_buf = NULL;
> >  	state->gop_cnt = 0;
> >  	ctx->cur_buf_offset = 0;
> >  	ctx->comp_size = 0;
> > @@ -1158,6 +1155,7 @@ static int vicodec_open(struct file *file)
> >  	struct vicodec_dev *dev = video_drvdata(file);
> >  	struct vicodec_ctx *ctx = NULL;
> >  	struct v4l2_ctrl_handler *hdl;
> > +	struct vb2_queue *vq;
> >  	unsigned int size;
> >  	int rc = 0;
> >  
> > @@ -1234,6 +1232,16 @@ static int vicodec_open(struct file *file)
> >  
> >  	v4l2_fh_add(&ctx->fh);
> >  
> > +	/* Setup a dummy flush buffer, used to signal
> > +	 * encoding/decoding stop operation. When this buffer
> > +	 * is queued to the OUTPUT queue, the driver will send
> > +	 * V4L2_EVENT_EOS and send the last buffer to userspace.
> > +	 */
> > +	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, multiplanar ?
> > +			     V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE :
> > +			     V4L2_BUF_TYPE_VIDEO_OUTPUT);
> > +	ctx->flush_buf.vb2_buf.vb2_queue = vq;
> > +
> >  open_unlock:
> >  	mutex_unlock(vfd->lock);
> >  	return rc;
> > 
> 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 2/2] vicodec: Implement spec-compliant stop command

[Hans Verkuil]

On 10/19/18 13:35, Nicolas Dufresne wrote:
> Le vendredi 19 octobre 2018 à 09:28 +0200, Hans Verkuil a écrit :
>> On 10/18/2018 06:08 PM, Ezequiel Garcia wrote:
>>> Set up a statically-allocated, dummy buffer to
>>> be used as flush buffer, which signals
>>> a encoding (or decoding) stop.
>>>
>>> When the flush buffer is queued to the OUTPUT queue,
>>> the driver will send an V4L2_EVENT_EOS event, and
>>> mark the CAPTURE buffer with V4L2_BUF_FLAG_LAST.
>>
>> I'm confused. What is the current driver doing wrong? It is already
>> setting the LAST flag AFAIK. I don't see why a dummy buffer is
>> needed.
> 
> I'm not sure of this patch either. It seems to trigger the legacy
> "empty payload" buffer case. Driver should mark the last buffer, and
> then following poll should return EPIPE. Maybe it's the later that
> isn't respected ?

That would make more sense: vicodec doesn't set last_buffer_dequeued
to true in the vb2_queue, so you won't get EPIPE.

But that should be much easier to add.

Regards,

	Hans

Re: [PATCH 2/2] vicodec: Implement spec-compliant stop command

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 8894 bytes --]

Le vendredi 19 octobre 2018 à 07:35 -0400, Nicolas Dufresne a écrit :
> Le vendredi 19 octobre 2018 à 09:28 +0200, Hans Verkuil a écrit :
> > On 10/18/2018 06:08 PM, Ezequiel Garcia wrote:
> > > Set up a statically-allocated, dummy buffer to
> > > be used as flush buffer, which signals
> > > a encoding (or decoding) stop.
> > > 
> > > When the flush buffer is queued to the OUTPUT queue,
> > > the driver will send an V4L2_EVENT_EOS event, and
> > > mark the CAPTURE buffer with V4L2_BUF_FLAG_LAST.
> > 
> > I'm confused. What is the current driver doing wrong? It is already
> > setting the LAST flag AFAIK. I don't see why a dummy buffer is
> > needed.
> 
> I'm not sure of this patch either. It seems to trigger the legacy
> "empty payload" buffer case. Driver should mark the last buffer, and
> then following poll should return EPIPE. Maybe it's the later that
> isn't respected ?

Sorry, I've send this too fast. The following poll should not block,
and DQBUF should retunr EPIPE.

In GStreamer we currently ignore the LAST flag and wait for EPIPE. The
reason is that not all driver can set the LAST flag. Exynos firmware
tells you it's done later and we don't want to introduce any latency in
the driver. The last flag isn't that useful in fact, but it can be use
with RTP to set the marker bit.

In previous discussion, using a buffer with payload 0 was not liked.
There might be codec where an empty buffer is valid, who knows ?

> 
> > 
> > Regards,
> > 
> > 	Hans
> > 
> > > 
> > > With this change, it's possible to run a pipeline to completion:
> > > 
> > > gst-launch-1.0 videotestsrc num-buffers=10 ! v4l2fwhtenc !
> > > v4l2fwhtdec ! fakevideosink
> > > 
> > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > > ---
> > >  drivers/media/platform/vicodec/vicodec-core.c | 80 ++++++++++---
> > > --
> > > ----
> > >  1 file changed, 44 insertions(+), 36 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/vicodec/vicodec-core.c
> > > b/drivers/media/platform/vicodec/vicodec-core.c
> > > index a2c487b4b80d..4ed4dae10e30 100644
> > > --- a/drivers/media/platform/vicodec/vicodec-core.c
> > > +++ b/drivers/media/platform/vicodec/vicodec-core.c
> > > @@ -113,7 +113,7 @@ struct vicodec_ctx {
> > >  	struct v4l2_ctrl_handler hdl;
> > >  
> > >  	struct vb2_v4l2_buffer *last_src_buf;
> > > -	struct vb2_v4l2_buffer *last_dst_buf;
> > > +	struct vb2_v4l2_buffer  flush_buf;
> > >  
> > >  	/* Source and destination queue data */
> > >  	struct vicodec_q_data   q_data[2];
> > > @@ -220,6 +220,7 @@ static void device_run(void *priv)
> > >  	struct vicodec_dev *dev = ctx->dev;
> > >  	struct vb2_v4l2_buffer *src_buf, *dst_buf;
> > >  	struct vicodec_q_data *q_out;
> > > +	bool flushing;
> > >  	u32 state;
> > >  
> > >  	src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> > > @@ -227,26 +228,36 @@ static void device_run(void *priv)
> > >  	q_out = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT);
> > >  
> > >  	state = VB2_BUF_STATE_DONE;
> > > -	if (device_process(ctx, src_buf, dst_buf))
> > > +
> > > +	flushing = (src_buf == &ctx->flush_buf);
> > > +	if (!flushing && device_process(ctx, src_buf, dst_buf))
> > >  		state = VB2_BUF_STATE_ERROR;
> > > -	ctx->last_dst_buf = dst_buf;
> > >  
> > >  	spin_lock(ctx->lock);
> > > -	if (!ctx->comp_has_next_frame && src_buf == ctx->last_src_buf)
> > > {
> > > +	if (!flushing) {
> > > +		if (!ctx->comp_has_next_frame && src_buf == ctx-
> > > > last_src_buf) {
> > > 
> > > +			dst_buf->flags |= V4L2_BUF_FLAG_LAST;
> > > +			v4l2_event_queue_fh(&ctx->fh, &eos_event);
> > > +		}
> > > +
> > > +		if (ctx->is_enc) {
> > > +			src_buf->sequence = q_out->sequence++;
> > > +			src_buf = v4l2_m2m_src_buf_remove(ctx-
> > > > fh.m2m_ctx);
> > > 
> > > +			v4l2_m2m_buf_done(src_buf, state);
> > > +		} else if (vb2_get_plane_payload(&src_buf->vb2_buf, 0)
> > > +				== ctx->cur_buf_offset) {
> > > +			src_buf->sequence = q_out->sequence++;
> > > +			src_buf = v4l2_m2m_src_buf_remove(ctx-
> > > > fh.m2m_ctx);
> > > 
> > > +			v4l2_m2m_buf_done(src_buf, state);
> > > +			ctx->cur_buf_offset = 0;
> > > +			ctx->comp_has_next_frame = false;
> > > +		}
> > > +	} else {
> > > +		src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> > > +		vb2_set_plane_payload(&dst_buf->vb2_buf, 0, 0);
> > >  		dst_buf->flags |= V4L2_BUF_FLAG_LAST;
> > >  		v4l2_event_queue_fh(&ctx->fh, &eos_event);
> > >  	}
> > > -	if (ctx->is_enc) {
> > > -		src_buf->sequence = q_out->sequence++;
> > > -		src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> > > -		v4l2_m2m_buf_done(src_buf, state);
> > > -	} else if (vb2_get_plane_payload(&src_buf->vb2_buf, 0) == ctx-
> > > > cur_buf_offset) {
> > > 
> > > -		src_buf->sequence = q_out->sequence++;
> > > -		src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> > > -		v4l2_m2m_buf_done(src_buf, state);
> > > -		ctx->cur_buf_offset = 0;
> > > -		ctx->comp_has_next_frame = false;
> > > -	}
> > >  	v4l2_m2m_buf_done(dst_buf, state);
> > >  	ctx->comp_size = 0;
> > >  	ctx->comp_magic_cnt = 0;
> > > @@ -293,6 +304,8 @@ static int job_ready(void *priv)
> > >  	src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> > >  	if (!src_buf)
> > >  		return 0;
> > > +	if (src_buf == &ctx->flush_buf)
> > > +		return 1;
> > >  	p_out = vb2_plane_vaddr(&src_buf->vb2_buf, 0);
> > >  	sz = vb2_get_plane_payload(&src_buf->vb2_buf, 0);
> > >  	p = p_out + ctx->cur_buf_offset;
> > > @@ -770,21 +783,6 @@ static int vidioc_s_fmt_vid_out(struct file
> > > *file, void *priv,
> > >  	return ret;
> > >  }
> > >  
> > > -static void vicodec_mark_last_buf(struct vicodec_ctx *ctx)
> > > -{
> > > -	static const struct v4l2_event eos_event = {
> > > -		.type = V4L2_EVENT_EOS
> > > -	};
> > > -
> > > -	spin_lock(ctx->lock);
> > > -	ctx->last_src_buf = v4l2_m2m_last_src_buf(ctx->fh.m2m_ctx);
> > > -	if (!ctx->last_src_buf && ctx->last_dst_buf) {
> > > -		ctx->last_dst_buf->flags |= V4L2_BUF_FLAG_LAST;
> > > -		v4l2_event_queue_fh(&ctx->fh, &eos_event);
> > > -	}
> > > -	spin_unlock(ctx->lock);
> > > -}
> > > -
> > >  static int vicodec_try_encoder_cmd(struct file *file, void *fh,
> > >  				struct v4l2_encoder_cmd *ec)
> > >  {
> > > @@ -806,8 +804,8 @@ static int vicodec_encoder_cmd(struct file
> > > *file, void *fh,
> > >  	ret = vicodec_try_encoder_cmd(file, fh, ec);
> > >  	if (ret < 0)
> > >  		return ret;
> > > -
> > > -	vicodec_mark_last_buf(ctx);
> > > +	v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, &ctx->flush_buf);
> > > +	v4l2_m2m_try_schedule(ctx->fh.m2m_ctx);
> > >  	return 0;
> > >  }
> > >  
> > > @@ -835,8 +833,8 @@ static int vicodec_decoder_cmd(struct file
> > > *file, void *fh,
> > >  	ret = vicodec_try_decoder_cmd(file, fh, dc);
> > >  	if (ret < 0)
> > >  		return ret;
> > > -
> > > -	vicodec_mark_last_buf(ctx);
> > > +	v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, &ctx->flush_buf);
> > > +	v4l2_m2m_try_schedule(ctx->fh.m2m_ctx);
> > >  	return 0;
> > >  }
> > >  
> > > @@ -991,7 +989,7 @@ static void vicodec_return_bufs(struct
> > > vb2_queue *q, u32 state)
> > >  			vbuf = v4l2_m2m_src_buf_remove(ctx-
> > > > fh.m2m_ctx);
> > > 
> > >  		else
> > >  			vbuf = v4l2_m2m_dst_buf_remove(ctx-
> > > > fh.m2m_ctx);
> > > 
> > > -		if (vbuf == NULL)
> > > +		if (!vbuf || vbuf == &ctx->flush_buf)
> > >  			return;
> > >  		spin_lock(ctx->lock);
> > >  		v4l2_m2m_buf_done(vbuf, state);
> > > @@ -1031,7 +1029,6 @@ static int vicodec_start_streaming(struct
> > > vb2_queue *q,
> > >  	state->ref_frame.cb = state->ref_frame.luma + size;
> > >  	state->ref_frame.cr = state->ref_frame.cb + size / chroma_div;
> > >  	ctx->last_src_buf = NULL;
> > > -	ctx->last_dst_buf = NULL;
> > >  	state->gop_cnt = 0;
> > >  	ctx->cur_buf_offset = 0;
> > >  	ctx->comp_size = 0;
> > > @@ -1158,6 +1155,7 @@ static int vicodec_open(struct file *file)
> > >  	struct vicodec_dev *dev = video_drvdata(file);
> > >  	struct vicodec_ctx *ctx = NULL;
> > >  	struct v4l2_ctrl_handler *hdl;
> > > +	struct vb2_queue *vq;
> > >  	unsigned int size;
> > >  	int rc = 0;
> > >  
> > > @@ -1234,6 +1232,16 @@ static int vicodec_open(struct file *file)
> > >  
> > >  	v4l2_fh_add(&ctx->fh);
> > >  
> > > +	/* Setup a dummy flush buffer, used to signal
> > > +	 * encoding/decoding stop operation. When this buffer
> > > +	 * is queued to the OUTPUT queue, the driver will send
> > > +	 * V4L2_EVENT_EOS and send the last buffer to userspace.
> > > +	 */
> > > +	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, multiplanar ?
> > > +			     V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE :
> > > +			     V4L2_BUF_TYPE_VIDEO_OUTPUT);
> > > +	ctx->flush_buf.vb2_buf.vb2_queue = vq;
> > > +
> > >  open_unlock:
> > >  	mutex_unlock(vfd->lock);
> > >  	return rc;
> > > 
> > 
> > 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 1/2] vicodec: Have decoder propagate changes to the CAPTURE queue

[Ezequiel Garcia]

On Fri, 2018-10-19 at 09:14 +0200, Hans Verkuil wrote:
> On 10/18/2018 06:08 PM, Ezequiel Garcia wrote:
> > The decoder interface (not yet merged) specifies that
> > width and height values set on the OUTPUT queue, must
> > be propagated to the CAPTURE queue.
> > 
> > This is not enough to comply with the specification,
> > which would require to properly support stream resolution
> > changes detection and notification.
> > 
> > However, it's a relatively small change, which fixes behavior
> > required by some applications such as gstreamer.
> > 
> > With this change, it's possible to run a simple T(T⁻¹) pipeline:
> > 
> > gst-launch-1.0 videotestsrc ! v4l2fwhtenc ! v4l2fwhtdec ! fakevideosink
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  drivers/media/platform/vicodec/vicodec-core.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/drivers/media/platform/vicodec/vicodec-core.c b/drivers/media/platform/vicodec/vicodec-core.c
> > index 1eb9132bfc85..a2c487b4b80d 100644
> > --- a/drivers/media/platform/vicodec/vicodec-core.c
> > +++ b/drivers/media/platform/vicodec/vicodec-core.c
> > @@ -673,6 +673,13 @@ static int vidioc_s_fmt(struct vicodec_ctx *ctx, struct v4l2_format *f)
> >  		q_data->width = pix->width;
> >  		q_data->height = pix->height;
> >  		q_data->sizeimage = pix->sizeimage;
> > +
> > +		/* Propagate changes to CAPTURE queue */
> > +		if (!ctx->is_enc && V4L2_TYPE_IS_OUTPUT(f->type)) {
> 
> Do we need !ctx->is_enc? Isn't this the same for both decoder and encoder?
> 

Well, I wasn't really sure about this. The decoder document clearly
says that changes has to be propagated to the capture queue, but that statement
is not in the encoder spec.

Since gstreamer didn't needs this, I decided not to add it.

Perhaps it's something to correct in the encoder spec?

> > +			ctx->q_data[V4L2_M2M_DST].width = pix->width;
> > +			ctx->q_data[V4L2_M2M_DST].height = pix->height;
> > +			ctx->q_data[V4L2_M2M_DST].sizeimage = pix->sizeimage;
> 
> This is wrong: you are copying the sizeimage for the compressed format as the
> sizeimage for the raw format, which is quite different.
> 

Doh, you are right.

> I think you need to make a little helper function that can update the width/height
> of a particular queue and that can calculate the sizeimage correctly.
> 

Sounds good.

Thanks for the review,
Ezequiel

Re: [PATCH 2/2] vicodec: Implement spec-compliant stop command

[Ezequiel Garcia]

On Fri, 2018-10-19 at 09:28 +0200, Hans Verkuil wrote:
> On 10/18/2018 06:08 PM, Ezequiel Garcia wrote:
> > Set up a statically-allocated, dummy buffer to
> > be used as flush buffer, which signals
> > a encoding (or decoding) stop.
> > 
> > When the flush buffer is queued to the OUTPUT queue,
> > the driver will send an V4L2_EVENT_EOS event, and
> > mark the CAPTURE buffer with V4L2_BUF_FLAG_LAST.
> 
> I'm confused. What is the current driver doing wrong? It is already
> setting the LAST flag AFAIK.

The driver seems to be setting V4L2_BUF_FLAG_LAST to the dst
buf, if there's no src buf.

IIRC, that alone is has no effects, because .device_run never
gets to run (after all, there is no src buf), and the dst
buf is never marked as done and dequeued...
 
> I don't see why a dummy buffer is needed.
> 

... and that is why I took the dummy buffer idea (from some other driver),
which seemed an easy way to artificially run device_run
to dequeue the dst buf.

What do you think?

Thanks,
Ezequiel

Re: [PATCH 2/2] vicodec: Implement spec-compliant stop command

[Ezequiel Garcia]

Hi Nicolas,

On Fri, 2018-10-19 at 07:41 -0400, Nicolas Dufresne wrote:
> Le vendredi 19 octobre 2018 à 07:35 -0400, Nicolas Dufresne a écrit :
> > Le vendredi 19 octobre 2018 à 09:28 +0200, Hans Verkuil a écrit :
> > > On 10/18/2018 06:08 PM, Ezequiel Garcia wrote:
> > > > Set up a statically-allocated, dummy buffer to
> > > > be used as flush buffer, which signals
> > > > a encoding (or decoding) stop.
> > > > 
> > > > When the flush buffer is queued to the OUTPUT queue,
> > > > the driver will send an V4L2_EVENT_EOS event, and
> > > > mark the CAPTURE buffer with V4L2_BUF_FLAG_LAST.
> > > 
> > > I'm confused. What is the current driver doing wrong? It is already
> > > setting the LAST flag AFAIK. I don't see why a dummy buffer is
> > > needed.
> > 
> > I'm not sure of this patch either. It seems to trigger the legacy
> > "empty payload" buffer case. Driver should mark the last buffer, and
> > then following poll should return EPIPE. Maybe it's the later that
> > isn't respected ?
> 
> Sorry, I've send this too fast. The following poll should not block,
> and DQBUF should retunr EPIPE.
> 
> In GStreamer we currently ignore the LAST flag and wait for EPIPE. The
> reason is that not all driver can set the LAST flag. Exynos firmware
> tells you it's done later and we don't want to introduce any latency in
> the driver. The last flag isn't that useful in fact, but it can be use
> with RTP to set the marker bit.
> 

Yeah, I know that gstreamer ignores the LAST flag.

> In previous discussion, using a buffer with payload 0 was not liked.
> There might be codec where an empty buffer is valid, who knows ?
> 

The goal of this patch is for the driver to mark the dst buf
as V4L2_BUF_FLAG_DONE and V4L2_BUF_FLAG_LAST, so videobuf2
core returns EPIPE on a DQBUF.

Sorry for not being clear in the commit log.

Thanks,
Ezequiel

Re: [PATCH 1/2] vicodec: Have decoder propagate changes to the CAPTURE queue

[Tomasz Figa]

On Fri, Oct 19, 2018 at 10:00 PM Ezequiel Garcia <ezequiel@collabora.com> wrote:
>
> On Fri, 2018-10-19 at 09:14 +0200, Hans Verkuil wrote:
> > On 10/18/2018 06:08 PM, Ezequiel Garcia wrote:
> > > The decoder interface (not yet merged) specifies that
> > > width and height values set on the OUTPUT queue, must
> > > be propagated to the CAPTURE queue.
> > >
> > > This is not enough to comply with the specification,
> > > which would require to properly support stream resolution
> > > changes detection and notification.
> > >
> > > However, it's a relatively small change, which fixes behavior
> > > required by some applications such as gstreamer.
> > >
> > > With this change, it's possible to run a simple T(T⁻¹) pipeline:
> > >
> > > gst-launch-1.0 videotestsrc ! v4l2fwhtenc ! v4l2fwhtdec ! fakevideosink
> > >
> > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > > ---
> > >  drivers/media/platform/vicodec/vicodec-core.c | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > >
> > > diff --git a/drivers/media/platform/vicodec/vicodec-core.c b/drivers/media/platform/vicodec/vicodec-core.c
> > > index 1eb9132bfc85..a2c487b4b80d 100644
> > > --- a/drivers/media/platform/vicodec/vicodec-core.c
> > > +++ b/drivers/media/platform/vicodec/vicodec-core.c
> > > @@ -673,6 +673,13 @@ static int vidioc_s_fmt(struct vicodec_ctx *ctx, struct v4l2_format *f)
> > >             q_data->width = pix->width;
> > >             q_data->height = pix->height;
> > >             q_data->sizeimage = pix->sizeimage;
> > > +
> > > +           /* Propagate changes to CAPTURE queue */
> > > +           if (!ctx->is_enc && V4L2_TYPE_IS_OUTPUT(f->type)) {
> >
> > Do we need !ctx->is_enc? Isn't this the same for both decoder and encoder?
> >
>
> Well, I wasn't really sure about this. The decoder document clearly
> says that changes has to be propagated to the capture queue, but that statement
> is not in the encoder spec.
>
> Since gstreamer didn't needs this, I decided not to add it.
>
> Perhaps it's something to correct in the encoder spec?

Hmm, in the v2 of the documentation I sent recently, the CAPTURE queue
of an encoder doesn't have width and height specified. For formats
that have the resolution embedded in bitstream metadata, this isn't
anything that the userspace should be concerned with. I forgot about
the formats that don't have the resolution in the metadata, so we
might need to bring them back. Then the propagation would have to be
there indeed.

> > > +                   ctx->q_data[V4L2_M2M_DST].width = pix->width;
> > > +                   ctx->q_data[V4L2_M2M_DST].height = pix->height;
> > > +                   ctx->q_data[V4L2_M2M_DST].sizeimage = pix->sizeimage;
> >
> > This is wrong: you are copying the sizeimage for the compressed format as the
> > sizeimage for the raw format, which is quite different.
> >
>
> Doh, you are right.
>
> > I think you need to make a little helper function that can update the width/height
> > of a particular queue and that can calculate the sizeimage correctly.
> >

I wish we had generic helpers to manage all the formats in one place,
rather than duplicating the handling in each driver. I found many
cases of drivers not reporting bytesperline correctly or not handling
some formats (other than default and so often not tested) correctly.
If we could just have the driver call
v4l2_fill_pixfmt_mp_for_format(&pixfmt_mp, pixelformat, width, height,
...), a lot of boilerplate and potential source of errors could be
removed. (Bonus points for helpers that can convert pixfmt_mp for a
non-M format, e.g. NV12, into a pixfmt_mp for the corresponding M
format, e.g. NV12M, so that all the drivers that can support M formats
can also handle non-M formats automatically.)

One thing to note, though, is that there might be driver specific
alignment constraints in the play, so care must be taken.

Best regards,
Tomasz