[PATCH 01/10] media: coda: set codec earlier

[PATCH 01/10] media: coda: set codec earlier

[Philipp Zabel]

The chosen codec depends on the coded format, which is known as soon as
the S_FMT call on the coded queue. This allows to use the codec in
callbacks that may be called before start_streaming, such as buf_queue.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/media/platform/coda/coda-common.c | 28 ++++++++++++++++-------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index fa0b22fb7991..a403bc2995b4 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -764,6 +764,7 @@ static int coda_s_fmt_vid_cap(struct file *file, void *priv,
 {
 	struct coda_ctx *ctx = fh_to_ctx(priv);
 	struct coda_q_data *q_data_src;
+	const struct coda_codec *codec;
 	struct v4l2_rect r;
 	int ret;
 
@@ -784,6 +785,15 @@ static int coda_s_fmt_vid_cap(struct file *file, void *priv,
 	if (ctx->inst_type != CODA_INST_ENCODER)
 		return 0;
 
+	/* Setting the coded format determines the selected codec */
+	codec = coda_find_codec(ctx->dev, q_data_src->fourcc,
+				f->fmt.pix.pixelformat);
+	if (!codec) {
+		v4l2_err(&ctx->dev->v4l2_dev, "failed to determine codec\n");
+		return -EINVAL;
+	}
+	ctx->codec = codec;
+
 	ctx->colorspace = f->fmt.pix.colorspace;
 	ctx->xfer_func = f->fmt.pix.xfer_func;
 	ctx->ycbcr_enc = f->fmt.pix.ycbcr_enc;
@@ -796,6 +806,7 @@ static int coda_s_fmt_vid_out(struct file *file, void *priv,
 			      struct v4l2_format *f)
 {
 	struct coda_ctx *ctx = fh_to_ctx(priv);
+	const struct coda_codec *codec;
 	struct v4l2_format f_cap;
 	struct vb2_queue *dst_vq;
 	int ret;
@@ -811,6 +822,15 @@ static int coda_s_fmt_vid_out(struct file *file, void *priv,
 	if (ctx->inst_type != CODA_INST_DECODER)
 		return 0;
 
+	/* Setting the coded format determines the selected codec */
+	codec = coda_find_codec(ctx->dev, f->fmt.pix.pixelformat,
+				V4L2_PIX_FMT_YUV420);
+	if (!codec) {
+		v4l2_err(&ctx->dev->v4l2_dev, "failed to determine codec\n");
+		return -EINVAL;
+	}
+	ctx->codec = codec;
+
 	ctx->colorspace = f->fmt.pix.colorspace;
 	ctx->xfer_func = f->fmt.pix.xfer_func;
 	ctx->ycbcr_enc = f->fmt.pix.ycbcr_enc;
@@ -1680,14 +1700,6 @@ static int coda_start_streaming(struct vb2_queue *q, unsigned int count)
 
 	ctx->gopcounter = ctx->params.gop_size - 1;
 
-	ctx->codec = coda_find_codec(ctx->dev, q_data_src->fourcc,
-				     q_data_dst->fourcc);
-	if (!ctx->codec) {
-		v4l2_err(v4l2_dev, "couldn't tell instance type.\n");
-		ret = -EINVAL;
-		goto err;
-	}
-
 	if (q_data_dst->fourcc == V4L2_PIX_FMT_JPEG)
 		ctx->params.gop_size = 1;
 	ctx->gopcounter = ctx->params.gop_size - 1;
-- 
2.20.1

[PATCH 02/10] media: coda: remove mask from decoder h.264 level control

[Philipp Zabel]

Since levels are specified in terms of maximum values, there is no
reason to filter out lower levels than the supported maximum.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/media/platform/coda/coda-common.c | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index a403bc2995b4..de608de62908 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -2027,7 +2027,6 @@ static void coda_jpeg_encode_ctrls(struct coda_ctx *ctx)
 
 static void coda_decode_ctrls(struct coda_ctx *ctx)
 {
-	u64 mask;
 	u8 max;
 
 	ctx->h264_profile_ctrl = v4l2_ctrl_new_std_menu(&ctx->ctrls,
@@ -2041,27 +2040,14 @@ static void coda_decode_ctrls(struct coda_ctx *ctx)
 		ctx->h264_profile_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
 	if (ctx->dev->devtype->product == CODA_HX4 ||
-	    ctx->dev->devtype->product == CODA_7541) {
+	    ctx->dev->devtype->product == CODA_7541)
 		max = V4L2_MPEG_VIDEO_H264_LEVEL_4_0;
-		mask = ~((1 << V4L2_MPEG_VIDEO_H264_LEVEL_2_0) |
-			 (1 << V4L2_MPEG_VIDEO_H264_LEVEL_3_0) |
-			 (1 << V4L2_MPEG_VIDEO_H264_LEVEL_3_1) |
-			 (1 << V4L2_MPEG_VIDEO_H264_LEVEL_3_2) |
-			 (1 << V4L2_MPEG_VIDEO_H264_LEVEL_4_0));
-	} else if (ctx->dev->devtype->product == CODA_960) {
+	else if (ctx->dev->devtype->product == CODA_960)
 		max = V4L2_MPEG_VIDEO_H264_LEVEL_4_1;
-		mask = ~((1 << V4L2_MPEG_VIDEO_H264_LEVEL_2_0) |
-			 (1 << V4L2_MPEG_VIDEO_H264_LEVEL_3_0) |
-			 (1 << V4L2_MPEG_VIDEO_H264_LEVEL_3_1) |
-			 (1 << V4L2_MPEG_VIDEO_H264_LEVEL_3_2) |
-			 (1 << V4L2_MPEG_VIDEO_H264_LEVEL_4_0) |
-			 (1 << V4L2_MPEG_VIDEO_H264_LEVEL_4_1));
-	} else {
+	else
 		return;
-	}
 	ctx->h264_level_ctrl = v4l2_ctrl_new_std_menu(&ctx->ctrls,
-		&coda_ctrl_ops, V4L2_CID_MPEG_VIDEO_H264_LEVEL, max, mask,
-		max);
+		&coda_ctrl_ops, V4L2_CID_MPEG_VIDEO_H264_LEVEL, max, 0, max);
 	if (ctx->h264_level_ctrl)
 		ctx->h264_level_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 }
-- 
2.20.1

[PATCH 03/10] media: coda: clear error return value before picture run

[Philipp Zabel]

The error return value is not written by some firmware codecs, such as
MPEG-2 decode on CodaHx4. Clear the error return value before starting
the picture run to avoid misinterpreting unrelated values returned by
sequence initialization as error return value.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/media/platform/coda/coda-bit.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/coda/coda-bit.c b/drivers/media/platform/coda/coda-bit.c
index b4f396c2e72c..eaa86737fa04 100644
--- a/drivers/media/platform/coda/coda-bit.c
+++ b/drivers/media/platform/coda/coda-bit.c
@@ -2010,6 +2010,9 @@ static int coda_prepare_decode(struct coda_ctx *ctx)
 	/* Clear decode success flag */
 	coda_write(dev, 0, CODA_RET_DEC_PIC_SUCCESS);
 
+	/* Clear error return value */
+	coda_write(dev, 0, CODA_RET_DEC_PIC_ERR_MB);
+
 	trace_coda_dec_pic_run(ctx, meta);
 
 	coda_command_async(ctx, CODA_COMMAND_PIC_RUN);
-- 
2.20.1

[PATCH 04/10] media: coda: add min number of buffers controls

[Philipp Zabel]

Add min number of buffers for capture (decoder) and output (encoder)
controls, which are required by the stateful video decoder / encoder
interface specification.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/media/platform/coda/coda-common.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index de608de62908..f1d3fb17784a 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -2061,11 +2061,17 @@ static int coda_ctrls_setup(struct coda_ctx *ctx)
 	v4l2_ctrl_new_std(&ctx->ctrls, &coda_ctrl_ops,
 		V4L2_CID_VFLIP, 0, 1, 1, 0);
 	if (ctx->inst_type == CODA_INST_ENCODER) {
+		v4l2_ctrl_new_std(&ctx->ctrls, &coda_ctrl_ops,
+				  V4L2_CID_MIN_BUFFERS_FOR_OUTPUT,
+				  1, 1, 1, 1);
 		if (ctx->cvd->dst_formats[0] == V4L2_PIX_FMT_JPEG)
 			coda_jpeg_encode_ctrls(ctx);
 		else
 			coda_encode_ctrls(ctx);
 	} else {
+		v4l2_ctrl_new_std(&ctx->ctrls, &coda_ctrl_ops,
+				  V4L2_CID_MIN_BUFFERS_FOR_CAPTURE,
+				  1, 1, 1, 1);
 		if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_H264)
 			coda_decode_ctrls(ctx);
 	}
-- 
2.20.1

[PATCH 05/10] media: coda: disable encoder command on decoder and vice versa

[Philipp Zabel]

Return -ENOTTY when userspace tries to call VIDIOC_(TRY_)ENCODER_CMD on
a decoder instance or VIDIOC_(TRY_)DECODER_CMD on an encoder instance.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/media/platform/coda/coda-common.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index f1d3fb17784a..c0421f06ca48 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -1000,6 +1000,11 @@ static int coda_s_selection(struct file *file, void *fh,
 static int coda_try_encoder_cmd(struct file *file, void *fh,
 				struct v4l2_encoder_cmd *ec)
 {
+	struct coda_ctx *ctx = fh_to_ctx(fh);
+
+	if (ctx->inst_type != CODA_INST_ENCODER)
+		return -ENOTTY;
+
 	if (ec->cmd != V4L2_ENC_CMD_STOP)
 		return -EINVAL;
 
@@ -1020,10 +1025,6 @@ static int coda_encoder_cmd(struct file *file, void *fh,
 	if (ret < 0)
 		return ret;
 
-	/* Ignore encoder stop command silently in decoder context */
-	if (ctx->inst_type != CODA_INST_ENCODER)
-		return 0;
-
 	/* Set the stream-end flag on this context */
 	ctx->bit_stream_param |= CODA_BIT_STREAM_END_FLAG;
 
@@ -1041,6 +1042,11 @@ static int coda_encoder_cmd(struct file *file, void *fh,
 static int coda_try_decoder_cmd(struct file *file, void *fh,
 				struct v4l2_decoder_cmd *dc)
 {
+	struct coda_ctx *ctx = fh_to_ctx(fh);
+
+	if (ctx->inst_type != CODA_INST_DECODER)
+		return -ENOTTY;
+
 	if (dc->cmd != V4L2_DEC_CMD_STOP)
 		return -EINVAL;
 
@@ -1063,10 +1069,6 @@ static int coda_decoder_cmd(struct file *file, void *fh,
 	if (ret < 0)
 		return ret;
 
-	/* Ignore decoder stop command silently in encoder context */
-	if (ctx->inst_type != CODA_INST_DECODER)
-		return 0;
-
 	/* Set the stream-end flag on this context */
 	coda_bit_stream_end_flag(ctx);
 	ctx->hold = false;
-- 
2.20.1

[PATCH 06/10] media: coda: implement encoder frame size enumeration

[Philipp Zabel]

The stateful encoder API requires VIDIOC_ENUM_FRAMESIZES to be
implemented.
Allow enumeration of supported frame sizes for encoding.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/media/platform/coda/coda-common.c | 37 +++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index c0421f06ca48..943f003c26c4 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -1077,6 +1077,42 @@ static int coda_decoder_cmd(struct file *file, void *fh,
 	return 0;
 }
 
+static int coda_enum_framesizes(struct file *file, void *fh,
+				struct v4l2_frmsizeenum *fsize)
+{
+	struct coda_ctx *ctx = fh_to_ctx(fh);
+	struct coda_q_data *q_data_dst;
+	const struct coda_codec *codec;
+
+	if (ctx->inst_type != CODA_INST_ENCODER)
+		return -ENOTTY;
+
+	if (fsize->index)
+		return -EINVAL;
+
+	if (coda_format_normalize_yuv(fsize->pixel_format) ==
+	    V4L2_PIX_FMT_YUV420) {
+		q_data_dst = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
+		codec = coda_find_codec(ctx->dev, fsize->pixel_format,
+					q_data_dst->fourcc);
+	} else {
+		codec = coda_find_codec(ctx->dev, V4L2_PIX_FMT_YUV420,
+					fsize->pixel_format);
+	}
+	if (!codec)
+		return -EINVAL;
+
+	fsize->type = V4L2_FRMSIZE_TYPE_CONTINUOUS;
+	fsize->stepwise.min_width = MIN_W;
+	fsize->stepwise.max_width = codec->max_w;
+	fsize->stepwise.step_width = 1;
+	fsize->stepwise.min_height = MIN_H;
+	fsize->stepwise.max_height = codec->max_h;
+	fsize->stepwise.step_height = 1;
+
+	return 0;
+}
+
 static int coda_enum_frameintervals(struct file *file, void *fh,
 				    struct v4l2_frmivalenum *f)
 {
@@ -1255,6 +1291,7 @@ static const struct v4l2_ioctl_ops coda_ioctl_ops = {
 	.vidioc_g_parm		= coda_g_parm,
 	.vidioc_s_parm		= coda_s_parm,
 
+	.vidioc_enum_framesizes	= coda_enum_framesizes,
 	.vidioc_enum_frameintervals = coda_enum_frameintervals,
 
 	.vidioc_subscribe_event = coda_subscribe_event,
-- 
2.20.1

[PATCH 07/10] media: coda: limit frame interval enumeration to supported frame sizes

[Philipp Zabel]

Let VIDIOC_ENUM_FRAMEINTERVALS return -EINVAL if userspace queries
frame intervals for unsupported frame sizes.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/media/platform/coda/coda-common.c | 33 ++++++++++++++++++-----
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index 943f003c26c4..2966eb1c4d2d 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -1117,7 +1117,8 @@ static int coda_enum_frameintervals(struct file *file, void *fh,
 				    struct v4l2_frmivalenum *f)
 {
 	struct coda_ctx *ctx = fh_to_ctx(fh);
-	int i;
+	struct coda_q_data *q_data;
+	const struct coda_codec *codec;
 
 	if (f->index)
 		return -EINVAL;
@@ -1126,12 +1127,32 @@ static int coda_enum_frameintervals(struct file *file, void *fh,
 	if (!ctx->vdoa && f->pixel_format == V4L2_PIX_FMT_YUYV)
 		return -EINVAL;
 
-	for (i = 0; i < CODA_MAX_FORMATS; i++) {
-		if (f->pixel_format == ctx->cvd->src_formats[i] ||
-		    f->pixel_format == ctx->cvd->dst_formats[i])
-			break;
+	if (ctx->inst_type == CODA_INST_ENCODER) {
+		if (coda_format_normalize_yuv(f->pixel_format) ==
+		    V4L2_PIX_FMT_YUV420) {
+			q_data = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
+			codec = coda_find_codec(ctx->dev, f->pixel_format,
+						q_data->fourcc);
+		} else {
+			codec = coda_find_codec(ctx->dev, V4L2_PIX_FMT_YUV420,
+						f->pixel_format);
+		}
+	} else {
+		if (coda_format_normalize_yuv(f->pixel_format) ==
+		    V4L2_PIX_FMT_YUV420) {
+			q_data = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT);
+			codec = coda_find_codec(ctx->dev, q_data->fourcc,
+						f->pixel_format);
+		} else {
+			codec = coda_find_codec(ctx->dev, f->pixel_format,
+						V4L2_PIX_FMT_YUV420);
+		}
 	}
-	if (i == CODA_MAX_FORMATS)
+	if (!codec)
+		return -EINVAL;
+
+	if (f->width < MIN_W || f->width > codec->max_w ||
+	    f->height < MIN_H || f->height > codec->max_h)
 		return -EINVAL;
 
 	f->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
-- 
2.20.1

[PATCH 08/10] media: coda: allow encoder to set colorimetry on the output queue

[Philipp Zabel]

v4l2-compliance sets colorimetry on the output queue and then verifies
that querying colorimetry on the capture queue returns the same
configuration. For this to work, the encoder must allow setting context
colorimetry parameters on the output queue.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/media/platform/coda/coda-common.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index 2966eb1c4d2d..7b89dbae938d 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -819,6 +819,11 @@ static int coda_s_fmt_vid_out(struct file *file, void *priv,
 	if (ret)
 		return ret;
 
+	ctx->colorspace = f->fmt.pix.colorspace;
+	ctx->xfer_func = f->fmt.pix.xfer_func;
+	ctx->ycbcr_enc = f->fmt.pix.ycbcr_enc;
+	ctx->quantization = f->fmt.pix.quantization;
+
 	if (ctx->inst_type != CODA_INST_DECODER)
 		return 0;
 
@@ -831,11 +836,6 @@ static int coda_s_fmt_vid_out(struct file *file, void *priv,
 	}
 	ctx->codec = codec;
 
-	ctx->colorspace = f->fmt.pix.colorspace;
-	ctx->xfer_func = f->fmt.pix.xfer_func;
-	ctx->ycbcr_enc = f->fmt.pix.ycbcr_enc;
-	ctx->quantization = f->fmt.pix.quantization;
-
 	dst_vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
 	if (!dst_vq)
 		return -EINVAL;
-- 
2.20.1

[PATCH 09/10] media: coda: throw error on create_bufs with too small size

[Philipp Zabel]

If VIDIOC_CREATE_BUFS is called with a sizeimage smaller than the
queue sizeimage, fail with -EINVAL instead of correcting the size
and continuing without error. This is required by v4l2-compliance.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/media/platform/coda/coda-common.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index 7b89dbae938d..318f0be103bb 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -1522,6 +1522,9 @@ static int coda_queue_setup(struct vb2_queue *vq,
 	q_data = get_q_data(ctx, vq->type);
 	size = q_data->sizeimage;
 
+	if (*nplanes)
+		return sizes[0] < size ? -EINVAL : 0;
+
 	*nplanes = 1;
 	sizes[0] = size;
 
-- 
2.20.1

[PATCH 10/10] media: coda: require all decoder command flags to be cleared

[Philipp Zabel]

The memory-to-memory stateful video decoder interface documentation
requires the decoder stop command initiating the drain sequence to have
flags set to zero.
Stop to black makes no sense as stopped memory-to-memory decoders do not
produce any frames, and stopping immediately can be achieved by stopping
the output video queue with VIDIOC_STREAMOFF.

The mute audio start command flag serves no purpose as the coda driver
does not handle any audio formats, and does not support playback at
non-standard speeds.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/media/platform/coda/coda-common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index 318f0be103bb..96798f98734a 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -1050,10 +1050,10 @@ static int coda_try_decoder_cmd(struct file *file, void *fh,
 	if (dc->cmd != V4L2_DEC_CMD_STOP)
 		return -EINVAL;
 
-	if (dc->flags & V4L2_DEC_CMD_STOP_TO_BLACK)
+	if (dc->stop.pts != 0)
 		return -EINVAL;
 
-	if (!(dc->flags & V4L2_DEC_CMD_STOP_IMMEDIATELY) && (dc->stop.pts != 0))
+	if (dc->flags != 0)
 		return -EINVAL;
 
 	return 0;
-- 
2.20.1

Re: [PATCH 10/10] media: coda: require all decoder command flags to be cleared

[Philipp Zabel]

On Mon, 2019-04-08 at 14:32 +0200, Philipp Zabel wrote:
> The memory-to-memory stateful video decoder interface documentation
> requires the decoder stop command initiating the drain sequence to have
> flags set to zero.
> Stop to black makes no sense as stopped memory-to-memory decoders do not
> produce any frames, and stopping immediately can be achieved by stopping
> the output video queue with VIDIOC_STREAMOFF.
> 
> The mute audio start command flag serves no purpose as the coda driver
> does not handle any audio formats, and does not support playback at
> non-standard speeds.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/media/platform/coda/coda-common.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
> index 318f0be103bb..96798f98734a 100644
> --- a/drivers/media/platform/coda/coda-common.c
> +++ b/drivers/media/platform/coda/coda-common.c
> @@ -1050,10 +1050,10 @@ static int coda_try_decoder_cmd(struct file *file, void *fh,
>  	if (dc->cmd != V4L2_DEC_CMD_STOP)
>  		return -EINVAL;
>  
> -	if (dc->flags & V4L2_DEC_CMD_STOP_TO_BLACK)
> +	if (dc->stop.pts != 0)
>  		return -EINVAL;
>  
> -	if (!(dc->flags & V4L2_DEC_CMD_STOP_IMMEDIATELY) && (dc->stop.pts != 0))
> +	if (dc->flags != 0)
>  		return -EINVAL;

This change currently causes a v4l2-compliance failure

                fail: v4l2-test-codecs.cpp(104): ret != 0
        test VIDIOC_(TRY_)DECODER_CMD: FAIL

because it still expects V4L2_DEC_CMD_STOP_IMMEDIATELY to be supported.

regards
Philipp

Re: [PATCH 07/10] media: coda: limit frame interval enumeration to supported frame sizes

[Hans Verkuil]

On 4/8/19 2:32 PM, Philipp Zabel wrote:
> Let VIDIOC_ENUM_FRAMEINTERVALS return -EINVAL if userspace queries
> frame intervals for unsupported frame sizes.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/media/platform/coda/coda-common.c | 33 ++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
> index 943f003c26c4..2966eb1c4d2d 100644
> --- a/drivers/media/platform/coda/coda-common.c
> +++ b/drivers/media/platform/coda/coda-common.c
> @@ -1117,7 +1117,8 @@ static int coda_enum_frameintervals(struct file *file, void *fh,
>  				    struct v4l2_frmivalenum *f)
>  {
>  	struct coda_ctx *ctx = fh_to_ctx(fh);
> -	int i;
> +	struct coda_q_data *q_data;
> +	const struct coda_codec *codec;
>  
>  	if (f->index)
>  		return -EINVAL;
> @@ -1126,12 +1127,32 @@ static int coda_enum_frameintervals(struct file *file, void *fh,
>  	if (!ctx->vdoa && f->pixel_format == V4L2_PIX_FMT_YUYV)
>  		return -EINVAL;
>  
> -	for (i = 0; i < CODA_MAX_FORMATS; i++) {
> -		if (f->pixel_format == ctx->cvd->src_formats[i] ||
> -		    f->pixel_format == ctx->cvd->dst_formats[i])
> -			break;
> +	if (ctx->inst_type == CODA_INST_ENCODER) {
> +		if (coda_format_normalize_yuv(f->pixel_format) ==
> +		    V4L2_PIX_FMT_YUV420) {
> +			q_data = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
> +			codec = coda_find_codec(ctx->dev, f->pixel_format,
> +						q_data->fourcc);
> +		} else {
> +			codec = coda_find_codec(ctx->dev, V4L2_PIX_FMT_YUV420,
> +						f->pixel_format);
> +		}
> +	} else {
> +		if (coda_format_normalize_yuv(f->pixel_format) ==
> +		    V4L2_PIX_FMT_YUV420) {
> +			q_data = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT);
> +			codec = coda_find_codec(ctx->dev, q_data->fourcc,
> +						f->pixel_format);
> +		} else {
> +			codec = coda_find_codec(ctx->dev, f->pixel_format,
> +						V4L2_PIX_FMT_YUV420);
> +		}
>  	}
> -	if (i == CODA_MAX_FORMATS)
> +	if (!codec)
> +		return -EINVAL;
> +
> +	if (f->width < MIN_W || f->width > codec->max_w ||
> +	    f->height < MIN_H || f->height > codec->max_h)
>  		return -EINVAL;
>  
>  	f->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
> 

Why support VIDIOC_ENUM_FRAMEINTERVALS at all? It makes no sense for a codec.
I'd remove it altogether.

Regards,

	Hans

Re: [PATCH 08/10] media: coda: allow encoder to set colorimetry on the output queue

[Hans Verkuil]

On 4/8/19 2:32 PM, Philipp Zabel wrote:
> v4l2-compliance sets colorimetry on the output queue and then verifies
> that querying colorimetry on the capture queue returns the same
> configuration. For this to work, the encoder must allow setting context
> colorimetry parameters on the output queue.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/media/platform/coda/coda-common.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
> index 2966eb1c4d2d..7b89dbae938d 100644
> --- a/drivers/media/platform/coda/coda-common.c
> +++ b/drivers/media/platform/coda/coda-common.c
> @@ -819,6 +819,11 @@ static int coda_s_fmt_vid_out(struct file *file, void *priv,
>  	if (ret)
>  		return ret;
>  
> +	ctx->colorspace = f->fmt.pix.colorspace;
> +	ctx->xfer_func = f->fmt.pix.xfer_func;
> +	ctx->ycbcr_enc = f->fmt.pix.ycbcr_enc;
> +	ctx->quantization = f->fmt.pix.quantization;
> +
>  	if (ctx->inst_type != CODA_INST_DECODER)
>  		return 0;
>  
> @@ -831,11 +836,6 @@ static int coda_s_fmt_vid_out(struct file *file, void *priv,
>  	}
>  	ctx->codec = codec;
>  
> -	ctx->colorspace = f->fmt.pix.colorspace;
> -	ctx->xfer_func = f->fmt.pix.xfer_func;
> -	ctx->ycbcr_enc = f->fmt.pix.ycbcr_enc;
> -	ctx->quantization = f->fmt.pix.quantization;
> -
>  	dst_vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
>  	if (!dst_vq)
>  		return -EINVAL;
> 

Isn't the colorimetry information encoded in the stream's metadata? So should
it be set in an encoder register so it ends up in the right place in the bitstream?

And for decoders it should then be read back from a register.

Just curious how this would work for this codec and if it is even possible.

For now this patch is OK, but I think more work is needed.

Regards,

	Hans

Re: [PATCH 10/10] media: coda: require all decoder command flags to be cleared

[Hans Verkuil]

On 4/9/19 6:57 PM, Philipp Zabel wrote:
> On Mon, 2019-04-08 at 14:32 +0200, Philipp Zabel wrote:
>> The memory-to-memory stateful video decoder interface documentation
>> requires the decoder stop command initiating the drain sequence to have
>> flags set to zero.
>> Stop to black makes no sense as stopped memory-to-memory decoders do not
>> produce any frames, and stopping immediately can be achieved by stopping
>> the output video queue with VIDIOC_STREAMOFF.
>>
>> The mute audio start command flag serves no purpose as the coda driver
>> does not handle any audio formats, and does not support playback at
>> non-standard speeds.
>>
>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>> ---
>>  drivers/media/platform/coda/coda-common.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
>> index 318f0be103bb..96798f98734a 100644
>> --- a/drivers/media/platform/coda/coda-common.c
>> +++ b/drivers/media/platform/coda/coda-common.c
>> @@ -1050,10 +1050,10 @@ static int coda_try_decoder_cmd(struct file *file, void *fh,
>>  	if (dc->cmd != V4L2_DEC_CMD_STOP)
>>  		return -EINVAL;
>>  
>> -	if (dc->flags & V4L2_DEC_CMD_STOP_TO_BLACK)
>> +	if (dc->stop.pts != 0)
>>  		return -EINVAL;
>>  
>> -	if (!(dc->flags & V4L2_DEC_CMD_STOP_IMMEDIATELY) && (dc->stop.pts != 0))
>> +	if (dc->flags != 0)
>>  		return -EINVAL;
> 
> This change currently causes a v4l2-compliance failure
> 
>                 fail: v4l2-test-codecs.cpp(104): ret != 0
>         test VIDIOC_(TRY_)DECODER_CMD: FAIL
> 
> because it still expects V4L2_DEC_CMD_STOP_IMMEDIATELY to be supported.

I've fixed this in my v4l-utils work-in-progress branch:

https://git.linuxtv.org/hverkuil/v4l-utils.git/log/?h=vicodec

But I want to hold off on this patch a little bit.

I think we need to add helpers for the try_en/decoder_cmd ioctls
to v4l2-mem2mem.c since it will be the same for all codecs.

I also had to fix vicodec since the checks it did weren't complete:

https://git.linuxtv.org/hverkuil/media_tree.git/commit/?h=vicodec&id=4e95bff9a15b63179fab20ac2113c238dc20665b

Regards,

	Hans

Re: [PATCH 07/10] media: coda: limit frame interval enumeration to supported frame sizes

[Philipp Zabel]

On Wed, 2019-04-10 at 15:43 +0200, Hans Verkuil wrote:
[...]
> > @@ -1126,12 +1127,32 @@ static int coda_enum_frameintervals(struct file *file, void *fh,
[...]
> 
> Why support VIDIOC_ENUM_FRAMEINTERVALS at all? It makes no sense for a codec.
> I'd remove it altogether.

It returns the range supported by the frame rate registers that can be
set for constant bitrate encoding. I think the idea was to let the
GStreamer v4l2 elements know about possible frame rate range.
I think I should be able to remove it without any negative effects.

regards
Philipp

Re: [PATCH 08/10] media: coda: allow encoder to set colorimetry on the output queue

[Philipp Zabel]

On Wed, 2019-04-10 at 15:48 +0200, Hans Verkuil wrote:
[...]
> Isn't the colorimetry information encoded in the stream's metadata?

That depends on the codec. Colorimetry information can be stored in
optional headers (for example Sequence Display Extension for MPEG-2,
Video Usability Information for H.264) but I don't know that the CODA
firmware can parse or generate any of them.

> So should it be set i an encoder register so it ends up in the right
> place in the bitstream?

I could produce the necessary headers manually and inject them into the
bitstream in the driver.

But for example for JPEG it is not clear to me if there even is a
correct way to do that. Can V4L2 colorimetry settings be translated into
ICC profiles?

> And for decoders it should then be read back from a register.

Same as above, I'd have to parse the headers in the driver.

> Just curious how this would work for this codec and if it is even possible.
> 
> For now this patch is OK, but I think more work is needed.

Agreed.

regards
Philipp

Re: [PATCH 07/10] media: coda: limit frame interval enumeration to supported frame sizes

[Nicolas Dufresne]

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

Le mercredi 10 avril 2019 à 16:22 +0200, Philipp Zabel a écrit :
> On Wed, 2019-04-10 at 15:43 +0200, Hans Verkuil wrote:
> [...]
> > > @@ -1126,12 +1127,32 @@ static int coda_enum_frameintervals(struct file *file, void *fh,
> [...]
> > Why support VIDIOC_ENUM_FRAMEINTERVALS at all? It makes no sense for a codec.
> > I'd remove it altogether.

It does make sense, since framerate is the only information that can be
used to produce a specific bitrate. If you don't enumerate the rates,
then you may endup with a miss-match of what userspace wants, which
will result in a different rate then what the user-space anticipated.
That being said, I expect these intervals to be really wide. Venus HW
uses a Q16 internally, which is precise enough that we could just
ignore the interval.

> 
> It returns the range supported by the frame rate registers that can be
> set for constant bitrate encoding. I think the idea was to let the
> GStreamer v4l2 elements know about possible frame rate range.
> I think I should be able to remove it without any negative effects.

As long as we can still set the framerate.

> 
> regards
> Philipp

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

Re: [PATCH 07/10] media: coda: limit frame interval enumeration to supported frame sizes

[Hans Verkuil]

On 4/10/19 6:11 PM, Nicolas Dufresne wrote:
> Le mercredi 10 avril 2019 à 16:22 +0200, Philipp Zabel a écrit :
>> On Wed, 2019-04-10 at 15:43 +0200, Hans Verkuil wrote:
>> [...]
>>>> @@ -1126,12 +1127,32 @@ static int coda_enum_frameintervals(struct file *file, void *fh,
>> [...]
>>> Why support VIDIOC_ENUM_FRAMEINTERVALS at all? It makes no sense for a codec.
>>> I'd remove it altogether.
> 
> It does make sense, since framerate is the only information that can be
> used to produce a specific bitrate. If you don't enumerate the rates,
> then you may endup with a miss-match of what userspace wants, which
> will result in a different rate then what the user-space anticipated.
> That being said, I expect these intervals to be really wide. Venus HW
> uses a Q16 internally, which is precise enough that we could just
> ignore the interval.

So the problem is that for an encoder where you desires a specific
constant bitrate, the encoder also needs to know the framerate.

And the driver then would have to support ENUM_FRAMEINTERVALS and the
S_PARM ioctl so userspace can set the framerate.

For decoders this would not make any sense AFAIKS, so this is encoder
specific.

Do I understand this correctly?

What should the default framerate be? 24 or 29.97 fps?

This should be documented in the stateful encoder spec. I never realized
that this would be needed...

Philipp, based on this information I would say that ENUM_FRAMEINTERVALS
can stay in the coda drivers, but for encoders only.

Regards,

	Hans

> 
>>
>> It returns the range supported by the frame rate registers that can be
>> set for constant bitrate encoding. I think the idea was to let the
>> GStreamer v4l2 elements know about possible frame rate range.
>> I think I should be able to remove it without any negative effects.
> 
> As long as we can still set the framerate.
> 
>>
>> regards
>> Philipp

Re: [PATCH 07/10] media: coda: limit frame interval enumeration to supported frame sizes

[Philipp Zabel]

On Wed, 2019-04-10 at 18:24 +0200, Hans Verkuil wrote:
> On 4/10/19 6:11 PM, Nicolas Dufresne wrote:
> > Le mercredi 10 avril 2019 à 16:22 +0200, Philipp Zabel a écrit :
> > > On Wed, 2019-04-10 at 15:43 +0200, Hans Verkuil wrote:
> > > [...]
> > > > > @@ -1126,12 +1127,32 @@ static int coda_enum_frameintervals(struct file *file, void *fh,
> > > 
> > > [...]
> > > > Why support VIDIOC_ENUM_FRAMEINTERVALS at all? It makes no sense for a codec.
> > > > I'd remove it altogether.
> > 
> > It does make sense, since framerate is the only information that can be
> > used to produce a specific bitrate. If you don't enumerate the rates,
> > then you may endup with a miss-match of what userspace wants, which
> > will result in a different rate then what the user-space anticipated.
> > That being said, I expect these intervals to be really wide. Venus HW
> > uses a Q16 internally, which is precise enough that we could just
> > ignore the interval.
> 
> So the problem is that for an encoder where you desires a specific
> constant bitrate, the encoder also needs to know the framerate.
> 
> And the driver then would have to support ENUM_FRAMEINTERVALS and the
> S_PARM ioctl so userspace can set the framerate.

Ah right, that was it. Encoding works fine without ENUM_FRAMEINTERVALS,
as coda has no meaningful frame rate limitations. I had implemented
S_PARM since it is required for CBR encoding, and v4l2-compliance then
complained about ENUM_FRAMEINTERVALS missing:

  cde29ef313de ("[media] coda: Use S_PARM to set nominal framerate for h.264 encoder")
  07b6080d4e6d ("media: coda: implement ENUM_FRAMEINTERVALS")

> For decoders this would not make any sense AFAIKS, so this is encoder
> specific.

Hmm, not necesarily. I hadn't thought about that before, but if the
decoder supports frame skipping this could be used to advertise
available reductions in frame rate.

> Do I understand this correctly?
> 
> What should the default framerate be? 24 or 29.97 fps?

Driver specific? Max nominal real time capable frame rate? 30 fps?

> This should be documented in the stateful encoder spec. I never realized
> that this would be needed...
> 
> Philipp, based on this information I would say that ENUM_FRAMEINTERVALS
> can stay in the coda drivers, but for encoders only.

Ok.

regards
Philipp

Re: [PATCH 07/10] media: coda: limit frame interval enumeration to supported frame sizes

[Hans Verkuil]

On 4/11/19 10:22 AM, Philipp Zabel wrote:
> On Wed, 2019-04-10 at 18:24 +0200, Hans Verkuil wrote:
>> On 4/10/19 6:11 PM, Nicolas Dufresne wrote:
>>> Le mercredi 10 avril 2019 à 16:22 +0200, Philipp Zabel a écrit :
>>>> On Wed, 2019-04-10 at 15:43 +0200, Hans Verkuil wrote:
>>>> [...]
>>>>>> @@ -1126,12 +1127,32 @@ static int coda_enum_frameintervals(struct file *file, void *fh,
>>>>
>>>> [...]
>>>>> Why support VIDIOC_ENUM_FRAMEINTERVALS at all? It makes no sense for a codec.
>>>>> I'd remove it altogether.
>>>
>>> It does make sense, since framerate is the only information that can be
>>> used to produce a specific bitrate. If you don't enumerate the rates,
>>> then you may endup with a miss-match of what userspace wants, which
>>> will result in a different rate then what the user-space anticipated.
>>> That being said, I expect these intervals to be really wide. Venus HW
>>> uses a Q16 internally, which is precise enough that we could just
>>> ignore the interval.
>>
>> So the problem is that for an encoder where you desires a specific
>> constant bitrate, the encoder also needs to know the framerate.
>>
>> And the driver then would have to support ENUM_FRAMEINTERVALS and the
>> S_PARM ioctl so userspace can set the framerate.
> 
> Ah right, that was it. Encoding works fine without ENUM_FRAMEINTERVALS,
> as coda has no meaningful frame rate limitations. I had implemented
> S_PARM since it is required for CBR encoding, and v4l2-compliance then
> complained about ENUM_FRAMEINTERVALS missing:
> 
>   cde29ef313de ("[media] coda: Use S_PARM to set nominal framerate for h.264 encoder")
>   07b6080d4e6d ("media: coda: implement ENUM_FRAMEINTERVALS")

And I think that's right. If you advertise framerate support, then
userspace needs to know about the capabilities.

I do think ENUM_FRAMEINTERVALS should return some sane values. Right now
coda advertises 1/65535 fps to 65535 fps (or thereabout).

I would probably return 1 fps to 200 fps (200 Hz is the highest framerate
defined by CTA-861-G).

This can also eventually be a helper function in v4l2-mem2mem.c since I expect
this to be the same for all (?) encoders.

> 
>> For decoders this would not make any sense AFAIKS, so this is encoder
>> specific.
> 
> Hmm, not necesarily. I hadn't thought about that before, but if the
> decoder supports frame skipping this could be used to advertise
> available reductions in frame rate.

That assumes that you want to use S_PARM for that as well. And does the decoder
provide the framerate encoded in the bitstream to the driver? Using S_PARM for
this only works if the driver can know the encoded framerate.

Anyway, this is probably something to discuss once a driver supports frame
skipping for the decoder.

> 
>> Do I understand this correctly?
>>
>> What should the default framerate be? 24 or 29.97 fps?
> 
> Driver specific? Max nominal real time capable frame rate? 30 fps?
> 
>> This should be documented in the stateful encoder spec. I never realized
>> that this would be needed...
>>
>> Philipp, based on this information I would say that ENUM_FRAMEINTERVALS
>> can stay in the coda drivers, but for encoders only.
> 
> Ok.
> 
> regards
> Philipp
> 

Regards,

	Hans

Re: [PATCH 07/10] media: coda: limit frame interval enumeration to supported frame sizes

[Ian Arkver]

Hi,

On 11/04/2019 11:18, Hans Verkuil wrote:
> On 4/11/19 10:22 AM, Philipp Zabel wrote:
>> On Wed, 2019-04-10 at 18:24 +0200, Hans Verkuil wrote:
>>> On 4/10/19 6:11 PM, Nicolas Dufresne wrote:
>>>> Le mercredi 10 avril 2019 à 16:22 +0200, Philipp Zabel a écrit :
>>>>> On Wed, 2019-04-10 at 15:43 +0200, Hans Verkuil wrote:
>>>>> [...]
>>>>>>> @@ -1126,12 +1127,32 @@ static int coda_enum_frameintervals(struct file *file, void *fh,
>>>>>
>>>>> [...]
>>>>>> Why support VIDIOC_ENUM_FRAMEINTERVALS at all? It makes no sense for a codec.
>>>>>> I'd remove it altogether.
>>>>
>>>> It does make sense, since framerate is the only information that can be
>>>> used to produce a specific bitrate. If you don't enumerate the rates,
>>>> then you may endup with a miss-match of what userspace wants, which
>>>> will result in a different rate then what the user-space anticipated.
>>>> That being said, I expect these intervals to be really wide. Venus HW
>>>> uses a Q16 internally, which is precise enough that we could just
>>>> ignore the interval.
>>>
>>> So the problem is that for an encoder where you desires a specific
>>> constant bitrate, the encoder also needs to know the framerate.
>>>
>>> And the driver then would have to support ENUM_FRAMEINTERVALS and the
>>> S_PARM ioctl so userspace can set the framerate.
>>
>> Ah right, that was it. Encoding works fine without ENUM_FRAMEINTERVALS,
>> as coda has no meaningful frame rate limitations. I had implemented
>> S_PARM since it is required for CBR encoding, and v4l2-compliance then
>> complained about ENUM_FRAMEINTERVALS missing:
>>
>>    cde29ef313de ("[media] coda: Use S_PARM to set nominal framerate for h.264 encoder")
>>    07b6080d4e6d ("media: coda: implement ENUM_FRAMEINTERVALS")
> 
> And I think that's right. If you advertise framerate support, then
> userspace needs to know about the capabilities.
> 
> I do think ENUM_FRAMEINTERVALS should return some sane values. Right now
> coda advertises 1/65535 fps to 65535 fps (or thereabout).
> 
> I would probably return 1 fps to 200 fps (200 Hz is the highest framerate
> defined by CTA-861-G).
> 
> This can also eventually be a helper function in v4l2-mem2mem.c since I expect
> this to be the same for all (?) encoders.
> 

While an i.MX wouldn't be my 1st choice for encoding a high framerate 
video, it could probably be done. This is a value for the bitrate 
control algo, afaics, not a statement of real time performance.

If an arbitrary limit is imposed then userspace would just need to work 
around it by adjusting the target bitrate for corner cases like this.

Regards,
Ian

>>
>>> For decoders this would not make any sense AFAIKS, so this is encoder
>>> specific.
>>
>> Hmm, not necesarily. I hadn't thought about that before, but if the
>> decoder supports frame skipping this could be used to advertise
>> available reductions in frame rate.
> 
> That assumes that you want to use S_PARM for that as well. And does the decoder
> provide the framerate encoded in the bitstream to the driver? Using S_PARM for
> this only works if the driver can know the encoded framerate.
> 
> Anyway, this is probably something to discuss once a driver supports frame
> skipping for the decoder.
> 
>>
>>> Do I understand this correctly?
>>>
>>> What should the default framerate be? 24 or 29.97 fps?
>>
>> Driver specific? Max nominal real time capable frame rate? 30 fps?
>>
>>> This should be documented in the stateful encoder spec. I never realized
>>> that this would be needed...
>>>
>>> Philipp, based on this information I would say that ENUM_FRAMEINTERVALS
>>> can stay in the coda drivers, but for encoders only.
>>
>> Ok.
>>
>> regards
>> Philipp
>>
> 
> Regards,
> 
> 	Hans
> 

Re: [PATCH 07/10] media: coda: limit frame interval enumeration to supported frame sizes

[Philipp Zabel]

On Thu, 2019-04-11 at 12:18 +0200, Hans Verkuil wrote:
> On 4/11/19 10:22 AM, Philipp Zabel wrote:
> > On Wed, 2019-04-10 at 18:24 +0200, Hans Verkuil wrote:
> > > On 4/10/19 6:11 PM, Nicolas Dufresne wrote:
> > > > Le mercredi 10 avril 2019 à 16:22 +0200, Philipp Zabel a écrit :
> > > > > On Wed, 2019-04-10 at 15:43 +0200, Hans Verkuil wrote:
> > > > > [...]
> > > > > > > @@ -1126,12 +1127,32 @@ static int coda_enum_frameintervals(struct file *file, void *fh,
> > > > > 
> > > > > [...]
> > > > > > Why support VIDIOC_ENUM_FRAMEINTERVALS at all? It makes no sense for a codec.
> > > > > > I'd remove it altogether.
> > > > 
> > > > It does make sense, since framerate is the only information that can be
> > > > used to produce a specific bitrate. If you don't enumerate the rates,
> > > > then you may endup with a miss-match of what userspace wants, which
> > > > will result in a different rate then what the user-space anticipated.
> > > > That being said, I expect these intervals to be really wide. Venus HW
> > > > uses a Q16 internally, which is precise enough that we could just
> > > > ignore the interval.
> > > 
> > > So the problem is that for an encoder where you desires a specific
> > > constant bitrate, the encoder also needs to know the framerate.
> > > 
> > > And the driver then would have to support ENUM_FRAMEINTERVALS and the
> > > S_PARM ioctl so userspace can set the framerate.
> > 
> > Ah right, that was it. Encoding works fine without ENUM_FRAMEINTERVALS,
> > as coda has no meaningful frame rate limitations. I had implemented
> > S_PARM since it is required for CBR encoding, and v4l2-compliance then
> > complained about ENUM_FRAMEINTERVALS missing:
> > 
> >   cde29ef313de ("[media] coda: Use S_PARM to set nominal framerate for h.264 encoder")
> >   07b6080d4e6d ("media: coda: implement ENUM_FRAMEINTERVALS")
> 
> And I think that's right. If you advertise framerate support, then
> userspace needs to know about the capabilities.
> 
> I do think ENUM_FRAMEINTERVALS should return some sane values. Right now
> coda advertises 1/65535 fps to 65535 fps (or thereabout).

Those are the limits imposed by the frame rate registers.

> I would probably return 1 fps to 200 fps (200 Hz is the highest framerate
> defined by CTA-861-G).

Why? We can certainly encode more than 200 fps in real time if only the
frame size is small enough, and for offline transcoding the nominal
frame rate can be anything the coded format supports.
Smaller than 1 fps might be useful for time lapse videos as well.

> This can also eventually be a helper function in v4l2-mem2mem.c since I expect
> this to be the same for all (?) encoders.

For example h.264 has an optional 32-bit numerator and denominator
(num_units_in_tick and time_scale in the Video Usability Information).
I don't see any reason to limit this artificially in the driver.

> > Hmm, not necesarily. I hadn't thought about that before, but if the
> > decoder supports frame skipping this could be used to advertise
> > available reductions in frame rate.
> 
> That assumes that you want to use S_PARM for that as well. And does the decoder
> provide the framerate encoded in the bitstream to the driver?

That is a difficult question. Yes, depending on CODA hardware variant,
codec, and actual bitstream (where this information is optional), the
firmware may or may not report the frame rate. Maybe it is fundamentally
not a good idea to try to control frame skipping via the nominal frame
rate instead of directly.

> Using S_PARM for this only works if the driver can know the encoded framerate.
> 
> Anyway, this is probably something to discuss once a driver supports frame
> skipping for the decoder.

Right. Since the coda driver does not support frame skipping yet, I'll
return -ENOTTY for decoder ENUM_FRAMEINTERVALS.

regards
Philipp

Re: [PATCH 07/10] media: coda: limit frame interval enumeration to supported frame sizes

[Nicolas Dufresne]

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

Le jeudi 11 avril 2019 à 14:00 +0200, Philipp Zabel a écrit :
> On Thu, 2019-04-11 at 12:18 +0200, Hans Verkuil wrote:
> > On 4/11/19 10:22 AM, Philipp Zabel wrote:
> > > On Wed, 2019-04-10 at 18:24 +0200, Hans Verkuil wrote:
> > > > On 4/10/19 6:11 PM, Nicolas Dufresne wrote:
> > > > > Le mercredi 10 avril 2019 à 16:22 +0200, Philipp Zabel a écrit :
> > > > > > On Wed, 2019-04-10 at 15:43 +0200, Hans Verkuil wrote:
> > > > > > [...]
> > > > > > > > @@ -1126,12 +1127,32 @@ static int coda_enum_frameintervals(struct file *file, void *fh,
> > > > > > 
> > > > > > [...]
> > > > > > > Why support VIDIOC_ENUM_FRAMEINTERVALS at all? It makes no sense for a codec.
> > > > > > > I'd remove it altogether.
> > > > > 
> > > > > It does make sense, since framerate is the only information that can be
> > > > > used to produce a specific bitrate. If you don't enumerate the rates,
> > > > > then you may endup with a miss-match of what userspace wants, which
> > > > > will result in a different rate then what the user-space anticipated.
> > > > > That being said, I expect these intervals to be really wide. Venus HW
> > > > > uses a Q16 internally, which is precise enough that we could just
> > > > > ignore the interval.
> > > > 
> > > > So the problem is that for an encoder where you desires a specific
> > > > constant bitrate, the encoder also needs to know the framerate.
> > > > 
> > > > And the driver then would have to support ENUM_FRAMEINTERVALS and the
> > > > S_PARM ioctl so userspace can set the framerate.
> > > 
> > > Ah right, that was it. Encoding works fine without ENUM_FRAMEINTERVALS,
> > > as coda has no meaningful frame rate limitations. I had implemented
> > > S_PARM since it is required for CBR encoding, and v4l2-compliance then
> > > complained about ENUM_FRAMEINTERVALS missing:
> > > 
> > >   cde29ef313de ("[media] coda: Use S_PARM to set nominal framerate for h.264 encoder")
> > >   07b6080d4e6d ("media: coda: implement ENUM_FRAMEINTERVALS")
> > 
> > And I think that's right. If you advertise framerate support, then
> > userspace needs to know about the capabilities.
> > 
> > I do think ENUM_FRAMEINTERVALS should return some sane values. Right now
> > coda advertises 1/65535 fps to 65535 fps (or thereabout).
> 
> Those are the limits imposed by the frame rate registers.
> 
> > I would probably return 1 fps to 200 fps (200 Hz is the highest framerate
> > defined by CTA-861-G).
> 
> Why? We can certainly encode more than 200 fps in real time if only the
> frame size is small enough, and for offline transcoding the nominal
> frame rate can be anything the coded format supports.
> Smaller than 1 fps might be useful for time lapse videos as well.
> 
> > This can also eventually be a helper function in v4l2-mem2mem.c since I expect
> > this to be the same for all (?) encoders.
> 
> For example h.264 has an optional 32-bit numerator and denominator
> (num_units_in_tick and time_scale in the Video Usability Information).
> I don't see any reason to limit this artificially in the driver.
> 
> > > Hmm, not necesarily. I hadn't thought about that before, but if the
> > > decoder supports frame skipping this could be used to advertise
> > > available reductions in frame rate.
> > 
> > That assumes that you want to use S_PARM for that as well. And does the decoder
> > provide the framerate encoded in the bitstream to the driver?
> 
> That is a difficult question. Yes, depending on CODA hardware variant,
> codec, and actual bitstream (where this information is optional), the
> firmware may or may not report the frame rate. Maybe it is fundamentally
> not a good idea to try to control frame skipping via the nominal frame
> rate instead of directly.

The Xilinx VCU decoder will allocate a different amount of cores per
session depending on the framerate we give it. This is how they manage
to handle more stream for lower rate/resolution etc. And it makes the
S_PARM call mandatory, since faking one will always lead to some
streams not to work properly, or less streams being handled in
parallel.

I personally don't see why we should in anyway be restrictive to the
information we are allowed to pass to a driver. Specially that is does
not matter if the driver uses it or not. And because there is no way we
will be able to think about every possible use cases popping in the
future, and it does not make backward compatibility to be a little more
open here.

> 
> > Using S_PARM for this only works if the driver can know the encoded framerate.
> > 
> > Anyway, this is probably something to discuss once a driver supports frame
> > skipping for the decoder.
> 
> Right. Since the coda driver does not support frame skipping yet, I'll
> return -ENOTTY for decoder ENUM_FRAMEINTERVALS.
> 
> regards
> Philipp

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

Re: [PATCH 07/10] media: coda: limit frame interval enumeration to supported frame sizes

[Tomasz Figa]

On Fri, Apr 12, 2019 at 12:53 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>
> Le jeudi 11 avril 2019 à 14:00 +0200, Philipp Zabel a écrit :
> > On Thu, 2019-04-11 at 12:18 +0200, Hans Verkuil wrote:
> > > On 4/11/19 10:22 AM, Philipp Zabel wrote:
> > > > On Wed, 2019-04-10 at 18:24 +0200, Hans Verkuil wrote:
> > > > > On 4/10/19 6:11 PM, Nicolas Dufresne wrote:
> > > > > > Le mercredi 10 avril 2019 à 16:22 +0200, Philipp Zabel a écrit :
> > > > > > > On Wed, 2019-04-10 at 15:43 +0200, Hans Verkuil wrote:
> > > > > > > [...]
> > > > > > > > > @@ -1126,12 +1127,32 @@ static int coda_enum_frameintervals(struct file *file, void *fh,
> > > > > > >
> > > > > > > [...]
> > > > > > > > Why support VIDIOC_ENUM_FRAMEINTERVALS at all? It makes no sense for a codec.
> > > > > > > > I'd remove it altogether.
> > > > > >
> > > > > > It does make sense, since framerate is the only information that can be
> > > > > > used to produce a specific bitrate. If you don't enumerate the rates,
> > > > > > then you may endup with a miss-match of what userspace wants, which
> > > > > > will result in a different rate then what the user-space anticipated.
> > > > > > That being said, I expect these intervals to be really wide. Venus HW
> > > > > > uses a Q16 internally, which is precise enough that we could just
> > > > > > ignore the interval.
> > > > >
> > > > > So the problem is that for an encoder where you desires a specific
> > > > > constant bitrate, the encoder also needs to know the framerate.
> > > > >
> > > > > And the driver then would have to support ENUM_FRAMEINTERVALS and the
> > > > > S_PARM ioctl so userspace can set the framerate.
> > > >
> > > > Ah right, that was it. Encoding works fine without ENUM_FRAMEINTERVALS,
> > > > as coda has no meaningful frame rate limitations. I had implemented
> > > > S_PARM since it is required for CBR encoding, and v4l2-compliance then
> > > > complained about ENUM_FRAMEINTERVALS missing:
> > > >
> > > >   cde29ef313de ("[media] coda: Use S_PARM to set nominal framerate for h.264 encoder")
> > > >   07b6080d4e6d ("media: coda: implement ENUM_FRAMEINTERVALS")
> > >
> > > And I think that's right. If you advertise framerate support, then
> > > userspace needs to know about the capabilities.
> > >
> > > I do think ENUM_FRAMEINTERVALS should return some sane values. Right now
> > > coda advertises 1/65535 fps to 65535 fps (or thereabout).
> >
> > Those are the limits imposed by the frame rate registers.
> >
> > > I would probably return 1 fps to 200 fps (200 Hz is the highest framerate
> > > defined by CTA-861-G).
> >
> > Why? We can certainly encode more than 200 fps in real time if only the
> > frame size is small enough, and for offline transcoding the nominal
> > frame rate can be anything the coded format supports.
> > Smaller than 1 fps might be useful for time lapse videos as well.
> >
> > > This can also eventually be a helper function in v4l2-mem2mem.c since I expect
> > > this to be the same for all (?) encoders.
> >
> > For example h.264 has an optional 32-bit numerator and denominator
> > (num_units_in_tick and time_scale in the Video Usability Information).
> > I don't see any reason to limit this artificially in the driver.
> >
> > > > Hmm, not necesarily. I hadn't thought about that before, but if the
> > > > decoder supports frame skipping this could be used to advertise
> > > > available reductions in frame rate.
> > >
> > > That assumes that you want to use S_PARM for that as well. And does the decoder
> > > provide the framerate encoded in the bitstream to the driver?
> >
> > That is a difficult question. Yes, depending on CODA hardware variant,
> > codec, and actual bitstream (where this information is optional), the
> > firmware may or may not report the frame rate. Maybe it is fundamentally
> > not a good idea to try to control frame skipping via the nominal frame
> > rate instead of directly.
>
> The Xilinx VCU decoder will allocate a different amount of cores per
> session depending on the framerate we give it. This is how they manage
> to handle more stream for lower rate/resolution etc. And it makes the
> S_PARM call mandatory, since faking one will always lead to some
> streams not to work properly, or less streams being handled in
> parallel.
>
> I personally don't see why we should in anyway be restrictive to the
> information we are allowed to pass to a driver. Specially that is does
> not matter if the driver uses it or not. And because there is no way we
> will be able to think about every possible use cases popping in the
> future, and it does not make backward compatibility to be a little more
> open here.

I think it just boils down to defining the meaning properly,
especially thinking about what ENUM_FRAME_INTERVALS would return.

In particular, I don't think there is any sensible way to guarantee
any performance aspects related to the frame rate conveyed via this
channel, so even if the application sees 200 fps here, it should just
mean that the bit-rate control algorithm can handle 200 fps streams,
but it can actually encode 60 fps (and so not real-time), due to the
hardware performance.

Best regards,
Tomasz