[PATCH 1/7] media: coda: set output buffer bytesused to appease v4l2-compliance

[PATCH 1/7] media: coda: set output buffer bytesused to appease v4l2-compliance

[Philipp Zabel]

The V4L2 specification states:

 "If the application sets this to 0 for an output stream, then bytesused
  will be set to the size of the buffer (see the length field of this
  struct) by the driver."

Since we set allow_zero_bytesused, we have to handle this ourselves.

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

diff --git a/drivers/media/platform/chips-media/coda-bit.c b/drivers/media/platform/chips-media/coda-bit.c
index c484c008ab02..705a179ea8f0 100644
--- a/drivers/media/platform/chips-media/coda-bit.c
+++ b/drivers/media/platform/chips-media/coda-bit.c
@@ -381,6 +381,9 @@ void coda_fill_bitstream(struct coda_ctx *ctx, struct list_head *buffer_list)
 		/* Dump empty buffers */
 		if (!vb2_get_plane_payload(&src_buf->vb2_buf, 0)) {
 			src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
+			vb2_set_plane_payload(&src_buf->vb2_buf, 0,
+					      vb2_plane_size(&src_buf->vb2_buf,
+							     0));
 			v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_DONE);
 			continue;
 		}
-- 
2.30.2

[PATCH 2/7] media: coda: disable encoder cmd ioctl on decoder and vice versa

[Philipp Zabel]

Use v4l2_disable_ioctl() to disable the VIDIOC_TRY_ENCODER_CMD and
VIDIOC_ENCODER_CMD ioctls on decoder video devices and the
VIDIOC_TRY_DECODER_CMD and VIDIOC_DECODER_CMD ioctls on encoder
video devices.

This allows to drop the coda_try_encoder/decoder_cmd() functions
and to use v4l2_m2m_ioctl_try_encoder/decoder_cmd() directly.

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

diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c
index 6d2989504b33..dc75133b0ead 100644
--- a/drivers/media/platform/chips-media/coda-common.c
+++ b/drivers/media/platform/chips-media/coda-common.c
@@ -1091,17 +1091,6 @@ 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;
-
-	return v4l2_m2m_ioctl_try_encoder_cmd(file, fh, ec);
-}
-
 static void coda_wake_up_capture_queue(struct coda_ctx *ctx)
 {
 	struct vb2_queue *dst_vq;
@@ -1120,7 +1109,7 @@ static int coda_encoder_cmd(struct file *file, void *fh,
 	struct vb2_v4l2_buffer *buf;
 	int ret;
 
-	ret = coda_try_encoder_cmd(file, fh, ec);
+	ret = v4l2_m2m_ioctl_try_encoder_cmd(file, fh, ec);
 	if (ret < 0)
 		return ret;
 
@@ -1149,17 +1138,6 @@ static int coda_encoder_cmd(struct file *file, void *fh,
 	return 0;
 }
 
-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;
-
-	return v4l2_m2m_ioctl_try_decoder_cmd(file, fh, dc);
-}
-
 static bool coda_mark_last_meta(struct coda_ctx *ctx)
 {
 	struct coda_buffer_meta *meta;
@@ -1216,7 +1194,7 @@ static int coda_decoder_cmd(struct file *file, void *fh,
 	bool wakeup;
 	int ret;
 
-	ret = coda_try_decoder_cmd(file, fh, dc);
+	ret = v4l2_m2m_ioctl_try_decoder_cmd(file, fh, dc);
 	if (ret < 0)
 		return ret;
 
@@ -1498,9 +1476,9 @@ static const struct v4l2_ioctl_ops coda_ioctl_ops = {
 	.vidioc_g_selection	= coda_g_selection,
 	.vidioc_s_selection	= coda_s_selection,
 
-	.vidioc_try_encoder_cmd	= coda_try_encoder_cmd,
+	.vidioc_try_encoder_cmd	= v4l2_m2m_ioctl_try_encoder_cmd,
 	.vidioc_encoder_cmd	= coda_encoder_cmd,
-	.vidioc_try_decoder_cmd	= coda_try_decoder_cmd,
+	.vidioc_try_decoder_cmd	= v4l2_m2m_ioctl_try_decoder_cmd,
 	.vidioc_decoder_cmd	= coda_decoder_cmd,
 
 	.vidioc_g_parm		= coda_g_parm,
@@ -2904,6 +2882,14 @@ static int coda_register_device(struct coda_dev *dev, int i)
 	v4l2_disable_ioctl(vfd, VIDIOC_G_CROP);
 	v4l2_disable_ioctl(vfd, VIDIOC_S_CROP);
 
+	if (dev->devtype->vdevs[i]->type == CODA_INST_ENCODER) {
+		v4l2_disable_ioctl(vfd, VIDIOC_DECODER_CMD);
+		v4l2_disable_ioctl(vfd, VIDIOC_TRY_DECODER_CMD);
+	} else {
+		v4l2_disable_ioctl(vfd, VIDIOC_ENCODER_CMD);
+		v4l2_disable_ioctl(vfd, VIDIOC_TRY_ENCODER_CMD);
+	}
+
 	ret = video_register_device(vfd, VFL_TYPE_VIDEO, 0);
 	if (!ret)
 		v4l2_info(&dev->v4l2_dev, "%s registered as %s\n",
-- 
2.30.2

Re: [PATCH 2/7] media: coda: disable encoder cmd ioctl on decoder and vice versa

[Nicolas Dufresne]

Le lundi 04 avril 2022 à 18:35 +0200, Philipp Zabel a écrit :
> Use v4l2_disable_ioctl() to disable the VIDIOC_TRY_ENCODER_CMD and
> VIDIOC_ENCODER_CMD ioctls on decoder video devices and the
> VIDIOC_TRY_DECODER_CMD and VIDIOC_DECODER_CMD ioctls on encoder
> video devices.
> 
> This allows to drop the coda_try_encoder/decoder_cmd() functions
> and to use v4l2_m2m_ioctl_try_encoder/decoder_cmd() directly.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Nice cleanup.

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> ---
>  .../media/platform/chips-media/coda-common.c  | 38 ++++++-------------
>  1 file changed, 12 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c
> index 6d2989504b33..dc75133b0ead 100644
> --- a/drivers/media/platform/chips-media/coda-common.c
> +++ b/drivers/media/platform/chips-media/coda-common.c
> @@ -1091,17 +1091,6 @@ 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;
> -
> -	return v4l2_m2m_ioctl_try_encoder_cmd(file, fh, ec);
> -}
> -
>  static void coda_wake_up_capture_queue(struct coda_ctx *ctx)
>  {
>  	struct vb2_queue *dst_vq;
> @@ -1120,7 +1109,7 @@ static int coda_encoder_cmd(struct file *file, void *fh,
>  	struct vb2_v4l2_buffer *buf;
>  	int ret;
>  
> -	ret = coda_try_encoder_cmd(file, fh, ec);
> +	ret = v4l2_m2m_ioctl_try_encoder_cmd(file, fh, ec);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -1149,17 +1138,6 @@ static int coda_encoder_cmd(struct file *file, void *fh,
>  	return 0;
>  }
>  
> -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;
> -
> -	return v4l2_m2m_ioctl_try_decoder_cmd(file, fh, dc);
> -}
> -
>  static bool coda_mark_last_meta(struct coda_ctx *ctx)
>  {
>  	struct coda_buffer_meta *meta;
> @@ -1216,7 +1194,7 @@ static int coda_decoder_cmd(struct file *file, void *fh,
>  	bool wakeup;
>  	int ret;
>  
> -	ret = coda_try_decoder_cmd(file, fh, dc);
> +	ret = v4l2_m2m_ioctl_try_decoder_cmd(file, fh, dc);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -1498,9 +1476,9 @@ static const struct v4l2_ioctl_ops coda_ioctl_ops = {
>  	.vidioc_g_selection	= coda_g_selection,
>  	.vidioc_s_selection	= coda_s_selection,
>  
> -	.vidioc_try_encoder_cmd	= coda_try_encoder_cmd,
> +	.vidioc_try_encoder_cmd	= v4l2_m2m_ioctl_try_encoder_cmd,
>  	.vidioc_encoder_cmd	= coda_encoder_cmd,
> -	.vidioc_try_decoder_cmd	= coda_try_decoder_cmd,
> +	.vidioc_try_decoder_cmd	= v4l2_m2m_ioctl_try_decoder_cmd,
>  	.vidioc_decoder_cmd	= coda_decoder_cmd,
>  
>  	.vidioc_g_parm		= coda_g_parm,
> @@ -2904,6 +2882,14 @@ static int coda_register_device(struct coda_dev *dev, int i)
>  	v4l2_disable_ioctl(vfd, VIDIOC_G_CROP);
>  	v4l2_disable_ioctl(vfd, VIDIOC_S_CROP);
>  
> +	if (dev->devtype->vdevs[i]->type == CODA_INST_ENCODER) {
> +		v4l2_disable_ioctl(vfd, VIDIOC_DECODER_CMD);
> +		v4l2_disable_ioctl(vfd, VIDIOC_TRY_DECODER_CMD);
> +	} else {
> +		v4l2_disable_ioctl(vfd, VIDIOC_ENCODER_CMD);
> +		v4l2_disable_ioctl(vfd, VIDIOC_TRY_ENCODER_CMD);
> +	}
> +
>  	ret = video_register_device(vfd, VFL_TYPE_VIDEO, 0);
>  	if (!ret)
>  		v4l2_info(&dev->v4l2_dev, "%s registered as %s\n",

[PATCH 3/7] media: coda: disable encoder ioctls for decoder devices

[Philipp Zabel]

Use v4l2_disable_ioctl() to disable the encoder ioctls
VIDIOC_ENUM_FRAMESIZES, VIDIOC_ENUM_FRAMEINTERVALS, VIDIOC_G_PARM, and
VIDIOC_S_PARM, to fix this v4l2-compliance test failure:

		fail: v4l2-test-formats.cpp(1363): node->is_m2m && !is_stateful_enc
	test VIDIOC_G/S_PARM: FAIL

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

diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c
index dc75133b0ead..c60473b18b6b 100644
--- a/drivers/media/platform/chips-media/coda-common.c
+++ b/drivers/media/platform/chips-media/coda-common.c
@@ -1269,9 +1269,6 @@ static int coda_enum_framesizes(struct file *file, void *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;
 
@@ -2888,6 +2885,10 @@ static int coda_register_device(struct coda_dev *dev, int i)
 	} else {
 		v4l2_disable_ioctl(vfd, VIDIOC_ENCODER_CMD);
 		v4l2_disable_ioctl(vfd, VIDIOC_TRY_ENCODER_CMD);
+		v4l2_disable_ioctl(vfd, VIDIOC_ENUM_FRAMESIZES);
+		v4l2_disable_ioctl(vfd, VIDIOC_ENUM_FRAMEINTERVALS);
+		v4l2_disable_ioctl(vfd, VIDIOC_G_PARM);
+		v4l2_disable_ioctl(vfd, VIDIOC_S_PARM);
 	}
 
 	ret = video_register_device(vfd, VFL_TYPE_VIDEO, 0);
-- 
2.30.2

Re: [PATCH 3/7] media: coda: disable encoder ioctls for decoder devices

[Nicolas Dufresne]

Le lundi 04 avril 2022 à 18:35 +0200, Philipp Zabel a écrit :
> Use v4l2_disable_ioctl() to disable the encoder ioctls
> VIDIOC_ENUM_FRAMESIZES, VIDIOC_ENUM_FRAMEINTERVALS, VIDIOC_G_PARM, and
> VIDIOC_S_PARM, to fix this v4l2-compliance test failure:
> 
> 		fail: v4l2-test-formats.cpp(1363): node->is_m2m && !is_stateful_enc
> 	test VIDIOC_G/S_PARM: FAIL

nit: Perhaps this comment can be improved. VIDIOC_ENUM_FRAMESIZES (even though
it could arguably be nicer to implement it for decoders) was returning ENOTTY
already, so I think its unlikely it was causing a test failures. For other it
G/S_PARM that looks logical, we usually don't use these for decoding unless the
decoders can report the rate from the VUI or something, though this is not yet
specified.

For the change itself, which looks all right.

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/media/platform/chips-media/coda-common.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c
> index dc75133b0ead..c60473b18b6b 100644
> --- a/drivers/media/platform/chips-media/coda-common.c
> +++ b/drivers/media/platform/chips-media/coda-common.c
> @@ -1269,9 +1269,6 @@ static int coda_enum_framesizes(struct file *file, void *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;
>  
> @@ -2888,6 +2885,10 @@ static int coda_register_device(struct coda_dev *dev, int i)
>  	} else {
>  		v4l2_disable_ioctl(vfd, VIDIOC_ENCODER_CMD);
>  		v4l2_disable_ioctl(vfd, VIDIOC_TRY_ENCODER_CMD);
> +		v4l2_disable_ioctl(vfd, VIDIOC_ENUM_FRAMESIZES);
> +		v4l2_disable_ioctl(vfd, VIDIOC_ENUM_FRAMEINTERVALS);
> +		v4l2_disable_ioctl(vfd, VIDIOC_G_PARM);
> +		v4l2_disable_ioctl(vfd, VIDIOC_S_PARM);
>  	}
>  
>  	ret = video_register_device(vfd, VFL_TYPE_VIDEO, 0);

[PATCH 4/7] media: coda: disable stateful encoder ioctls for jpeg encoder

[Philipp Zabel]

Use v4l2_disable_ioctl() to disable the stateful encoder ioctls
VIDIOC_ENUM_FRAMEINTERVALS, VIDIOC_G_PARM, and VIDIOC_S_PARM for
the jpeg encoder device, to fix this v4l2-compliance test failure:

		fail: v4l2-test-formats.cpp(68): node->is_m2m && !(node->codec_mask & STATEFUL_ENCODER)
	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: FAIL
		fail: v4l2-test-formats.cpp(1363): node->is_m2m && !is_stateful_enc
	test VIDIOC_G/S_PARM: FAIL

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

diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c
index c60473b18b6b..4a7346ed771e 100644
--- a/drivers/media/platform/chips-media/coda-common.c
+++ b/drivers/media/platform/chips-media/coda-common.c
@@ -2882,6 +2882,11 @@ static int coda_register_device(struct coda_dev *dev, int i)
 	if (dev->devtype->vdevs[i]->type == CODA_INST_ENCODER) {
 		v4l2_disable_ioctl(vfd, VIDIOC_DECODER_CMD);
 		v4l2_disable_ioctl(vfd, VIDIOC_TRY_DECODER_CMD);
+		if (dev->devtype->vdevs[i]->dst_formats[0] == V4L2_PIX_FMT_JPEG) {
+			v4l2_disable_ioctl(vfd, VIDIOC_ENUM_FRAMEINTERVALS);
+			v4l2_disable_ioctl(vfd, VIDIOC_G_PARM);
+			v4l2_disable_ioctl(vfd, VIDIOC_S_PARM);
+		}
 	} else {
 		v4l2_disable_ioctl(vfd, VIDIOC_ENCODER_CMD);
 		v4l2_disable_ioctl(vfd, VIDIOC_TRY_ENCODER_CMD);
-- 
2.30.2

Re: [PATCH 4/7] media: coda: disable stateful encoder ioctls for jpeg encoder

[Nicolas Dufresne]

Le lundi 04 avril 2022 à 18:35 +0200, Philipp Zabel a écrit :
> Use v4l2_disable_ioctl() to disable the stateful encoder ioctls
> VIDIOC_ENUM_FRAMEINTERVALS, VIDIOC_G_PARM, and VIDIOC_S_PARM for
> the jpeg encoder device, to fix this v4l2-compliance test failure:
> 
> 		fail: v4l2-test-formats.cpp(68): node->is_m2m && !(node->codec_mask & STATEFUL_ENCODER)
> 	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: FAIL
> 		fail: v4l2-test-formats.cpp(1363): node->is_m2m && !is_stateful_enc
> 	test VIDIOC_G/S_PARM: FAIL
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> ---
>  drivers/media/platform/chips-media/coda-common.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c
> index c60473b18b6b..4a7346ed771e 100644
> --- a/drivers/media/platform/chips-media/coda-common.c
> +++ b/drivers/media/platform/chips-media/coda-common.c
> @@ -2882,6 +2882,11 @@ static int coda_register_device(struct coda_dev *dev, int i)
>  	if (dev->devtype->vdevs[i]->type == CODA_INST_ENCODER) {
>  		v4l2_disable_ioctl(vfd, VIDIOC_DECODER_CMD);
>  		v4l2_disable_ioctl(vfd, VIDIOC_TRY_DECODER_CMD);
> +		if (dev->devtype->vdevs[i]->dst_formats[0] == V4L2_PIX_FMT_JPEG) {
> +			v4l2_disable_ioctl(vfd, VIDIOC_ENUM_FRAMEINTERVALS);
> +			v4l2_disable_ioctl(vfd, VIDIOC_G_PARM);
> +			v4l2_disable_ioctl(vfd, VIDIOC_S_PARM);
> +		}
>  	} else {
>  		v4l2_disable_ioctl(vfd, VIDIOC_ENCODER_CMD);
>  		v4l2_disable_ioctl(vfd, VIDIOC_TRY_ENCODER_CMD);

[PATCH 5/7] media: coda: fix default JPEG colorimetry

[Philipp Zabel]

Set default colorspace to SRGB for JPEG encoder and decoder devices,
to fix the following v4l2-compliance test failure:

	test VIDIOC_TRY_FMT: OK
		fail: v4l2-test-formats.cpp(818): fmt_raw.g_colorspace() != V4L2_COLORSPACE_SRGB

Also explicitly set transfer function, YCbCr encoding and quantization
range, as required by v4l2-compliance for the JPEG encoded side.

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

diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c
index 4a7346ed771e..c068c16d1eb4 100644
--- a/drivers/media/platform/chips-media/coda-common.c
+++ b/drivers/media/platform/chips-media/coda-common.c
@@ -732,13 +732,22 @@ static int coda_try_fmt_vid_cap(struct file *file, void *priv,
 	return 0;
 }
 
-static void coda_set_default_colorspace(struct v4l2_pix_format *fmt)
+static void coda_set_default_colorspace(struct coda_ctx *ctx,
+					struct v4l2_pix_format *fmt)
 {
 	enum v4l2_colorspace colorspace;
 
-	if (fmt->pixelformat == V4L2_PIX_FMT_JPEG)
-		colorspace = V4L2_COLORSPACE_JPEG;
-	else if (fmt->width <= 720 && fmt->height <= 576)
+	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG ||
+	    ctx->cvd->dst_formats[0] == V4L2_PIX_FMT_JPEG ||
+	    fmt->pixelformat == V4L2_PIX_FMT_JPEG) {
+		fmt->colorspace = V4L2_COLORSPACE_SRGB;
+		fmt->xfer_func = V4L2_XFER_FUNC_SRGB;
+		fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
+		fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+		return;
+	}
+
+	if (fmt->width <= 720 && fmt->height <= 576)
 		colorspace = V4L2_COLORSPACE_SMPTE170M;
 	else
 		colorspace = V4L2_COLORSPACE_REC709;
@@ -763,7 +772,7 @@ static int coda_try_fmt_vid_out(struct file *file, void *priv,
 		return ret;
 
 	if (f->fmt.pix.colorspace == V4L2_COLORSPACE_DEFAULT)
-		coda_set_default_colorspace(&f->fmt.pix);
+		coda_set_default_colorspace(ctx, &f->fmt.pix);
 
 	q_data_dst = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
 	codec = coda_find_codec(dev, f->fmt.pix.pixelformat, q_data_dst->fourcc);
@@ -1640,13 +1649,18 @@ static void set_default_params(struct coda_ctx *ctx)
 	csize = coda_estimate_sizeimage(ctx, usize, max_w, max_h);
 
 	ctx->params.codec_mode = ctx->codec->mode;
-	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG)
-		ctx->colorspace = V4L2_COLORSPACE_JPEG;
-	else
+	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG ||
+	    ctx->cvd->dst_formats[0] == V4L2_PIX_FMT_JPEG) {
+		ctx->colorspace = V4L2_COLORSPACE_SRGB;
+		ctx->xfer_func = V4L2_XFER_FUNC_SRGB;
+		ctx->ycbcr_enc = V4L2_YCBCR_ENC_601;
+		ctx->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+	} else {
 		ctx->colorspace = V4L2_COLORSPACE_REC709;
-	ctx->xfer_func = V4L2_XFER_FUNC_DEFAULT;
-	ctx->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
-	ctx->quantization = V4L2_QUANTIZATION_DEFAULT;
+		ctx->xfer_func = V4L2_XFER_FUNC_DEFAULT;
+		ctx->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
+		ctx->quantization = V4L2_QUANTIZATION_DEFAULT;
+	}
 	ctx->params.framerate = 30;
 
 	/* Default formats for output and input queues */
-- 
2.30.2

Re: [PATCH 5/7] media: coda: fix default JPEG colorimetry

[Nicolas Dufresne]

Le lundi 04 avril 2022 à 18:35 +0200, Philipp Zabel a écrit :
> Set default colorspace to SRGB for JPEG encoder and decoder devices,
> to fix the following v4l2-compliance test failure:
> 
> 	test VIDIOC_TRY_FMT: OK
> 		fail: v4l2-test-formats.cpp(818): fmt_raw.g_colorspace() != V4L2_COLORSPACE_SRGB
> 
> Also explicitly set transfer function, YCbCr encoding and quantization
> range, as required by v4l2-compliance for the JPEG encoded side.

Note that this will perhaps hit some GStreamer bugs that are being discussed.
Documenting for the users:

https://gitlab.freedesktop.org/gstreamer/gstreamer/-/issues/1118
https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1406

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  .../media/platform/chips-media/coda-common.c  | 36 +++++++++++++------
>  1 file changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c
> index 4a7346ed771e..c068c16d1eb4 100644
> --- a/drivers/media/platform/chips-media/coda-common.c
> +++ b/drivers/media/platform/chips-media/coda-common.c
> @@ -732,13 +732,22 @@ static int coda_try_fmt_vid_cap(struct file *file, void *priv,
>  	return 0;
>  }
>  
> -static void coda_set_default_colorspace(struct v4l2_pix_format *fmt)
> +static void coda_set_default_colorspace(struct coda_ctx *ctx,
> +					struct v4l2_pix_format *fmt)
>  {
>  	enum v4l2_colorspace colorspace;
>  
> -	if (fmt->pixelformat == V4L2_PIX_FMT_JPEG)
> -		colorspace = V4L2_COLORSPACE_JPEG;
> -	else if (fmt->width <= 720 && fmt->height <= 576)
> +	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG ||
> +	    ctx->cvd->dst_formats[0] == V4L2_PIX_FMT_JPEG ||
> +	    fmt->pixelformat == V4L2_PIX_FMT_JPEG) {
> +		fmt->colorspace = V4L2_COLORSPACE_SRGB;
> +		fmt->xfer_func = V4L2_XFER_FUNC_SRGB;
> +		fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
> +		fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +		return;
> +	}
> +
> +	if (fmt->width <= 720 && fmt->height <= 576)
>  		colorspace = V4L2_COLORSPACE_SMPTE170M;
>  	else
>  		colorspace = V4L2_COLORSPACE_REC709;
> @@ -763,7 +772,7 @@ static int coda_try_fmt_vid_out(struct file *file, void *priv,
>  		return ret;
>  
>  	if (f->fmt.pix.colorspace == V4L2_COLORSPACE_DEFAULT)
> -		coda_set_default_colorspace(&f->fmt.pix);
> +		coda_set_default_colorspace(ctx, &f->fmt.pix);
>  
>  	q_data_dst = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
>  	codec = coda_find_codec(dev, f->fmt.pix.pixelformat, q_data_dst->fourcc);
> @@ -1640,13 +1649,18 @@ static void set_default_params(struct coda_ctx *ctx)
>  	csize = coda_estimate_sizeimage(ctx, usize, max_w, max_h);
>  
>  	ctx->params.codec_mode = ctx->codec->mode;
> -	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG)
> -		ctx->colorspace = V4L2_COLORSPACE_JPEG;
> -	else
> +	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG ||
> +	    ctx->cvd->dst_formats[0] == V4L2_PIX_FMT_JPEG) {
> +		ctx->colorspace = V4L2_COLORSPACE_SRGB;
> +		ctx->xfer_func = V4L2_XFER_FUNC_SRGB;
> +		ctx->ycbcr_enc = V4L2_YCBCR_ENC_601;
> +		ctx->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +	} else {
>  		ctx->colorspace = V4L2_COLORSPACE_REC709;
> -	ctx->xfer_func = V4L2_XFER_FUNC_DEFAULT;
> -	ctx->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> -	ctx->quantization = V4L2_QUANTIZATION_DEFAULT;
> +		ctx->xfer_func = V4L2_XFER_FUNC_DEFAULT;
> +		ctx->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> +		ctx->quantization = V4L2_QUANTIZATION_DEFAULT;
> +	}
>  	ctx->params.framerate = 30;
>  
>  	/* Default formats for output and input queues */

Re: [PATCH 5/7] media: coda: fix default JPEG colorimetry

[Hans Verkuil]

Hi Philipp,

On 04/04/2022 18:35, Philipp Zabel wrote:
> Set default colorspace to SRGB for JPEG encoder and decoder devices,
> to fix the following v4l2-compliance test failure:
> 
> 	test VIDIOC_TRY_FMT: OK
> 		fail: v4l2-test-formats.cpp(818): fmt_raw.g_colorspace() != V4L2_COLORSPACE_SRGB
> 
> Also explicitly set transfer function, YCbCr encoding and quantization
> range, as required by v4l2-compliance for the JPEG encoded side.

I'm not quite sure if this patch addresses the correct issue.

> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  .../media/platform/chips-media/coda-common.c  | 36 +++++++++++++------
>  1 file changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c
> index 4a7346ed771e..c068c16d1eb4 100644
> --- a/drivers/media/platform/chips-media/coda-common.c
> +++ b/drivers/media/platform/chips-media/coda-common.c
> @@ -732,13 +732,22 @@ static int coda_try_fmt_vid_cap(struct file *file, void *priv,
>  	return 0;
>  }
>  
> -static void coda_set_default_colorspace(struct v4l2_pix_format *fmt)
> +static void coda_set_default_colorspace(struct coda_ctx *ctx,
> +					struct v4l2_pix_format *fmt)
>  {
>  	enum v4l2_colorspace colorspace;
>  
> -	if (fmt->pixelformat == V4L2_PIX_FMT_JPEG)
> -		colorspace = V4L2_COLORSPACE_JPEG;

It's perfectly fine to keep this, the problem occurs with the raw image side
(capture for the decoder, output for the encoder).

There the colorspace must be SRGB, the xfer_func may be 0 or SRGB, and the
ycbcr_enc is 0 (if not a YUV pixelformat) or ENC_601 (if it is a YUV format).
Actually, if the hardware can convert from other YUV encodings such as 709,
then other YUV encodings are valid, but I assume that's not the case.

> -	else if (fmt->width <= 720 && fmt->height <= 576)
> +	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG ||
> +	    ctx->cvd->dst_formats[0] == V4L2_PIX_FMT_JPEG ||
> +	    fmt->pixelformat == V4L2_PIX_FMT_JPEG) {
> +		fmt->colorspace = V4L2_COLORSPACE_SRGB;
> +		fmt->xfer_func = V4L2_XFER_FUNC_SRGB;
> +		fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
> +		fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +		return;
> +	}
> +
> +	if (fmt->width <= 720 && fmt->height <= 576)
>  		colorspace = V4L2_COLORSPACE_SMPTE170M;
>  	else
>  		colorspace = V4L2_COLORSPACE_REC709;
> @@ -763,7 +772,7 @@ static int coda_try_fmt_vid_out(struct file *file, void *priv,
>  		return ret;
>  
>  	if (f->fmt.pix.colorspace == V4L2_COLORSPACE_DEFAULT)
> -		coda_set_default_colorspace(&f->fmt.pix);
> +		coda_set_default_colorspace(ctx, &f->fmt.pix);
>  
>  	q_data_dst = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
>  	codec = coda_find_codec(dev, f->fmt.pix.pixelformat, q_data_dst->fourcc);
> @@ -1640,13 +1649,18 @@ static void set_default_params(struct coda_ctx *ctx)
>  	csize = coda_estimate_sizeimage(ctx, usize, max_w, max_h);
>  
>  	ctx->params.codec_mode = ctx->codec->mode;
> -	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG)
> -		ctx->colorspace = V4L2_COLORSPACE_JPEG;
> -	else
> +	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG ||
> +	    ctx->cvd->dst_formats[0] == V4L2_PIX_FMT_JPEG) {
> +		ctx->colorspace = V4L2_COLORSPACE_SRGB;
> +		ctx->xfer_func = V4L2_XFER_FUNC_SRGB;
> +		ctx->ycbcr_enc = V4L2_YCBCR_ENC_601;
> +		ctx->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +	} else {
>  		ctx->colorspace = V4L2_COLORSPACE_REC709;

My guess is that the raw format colorspace is set to REC709, which is definitely
wrong for a JPEG codec. For a JPEG codec that must be set to SRGB.

I suspect that's the real bug here.

I'm skipping this patch for now.

Regards,

	Hans

> -	ctx->xfer_func = V4L2_XFER_FUNC_DEFAULT;
> -	ctx->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> -	ctx->quantization = V4L2_QUANTIZATION_DEFAULT;
> +		ctx->xfer_func = V4L2_XFER_FUNC_DEFAULT;
> +		ctx->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> +		ctx->quantization = V4L2_QUANTIZATION_DEFAULT;
> +	}
>  	ctx->params.framerate = 30;
>  
>  	/* Default formats for output and input queues */

Re: [PATCH 5/7] media: coda: fix default JPEG colorimetry

[Philipp Zabel]

On Do, 2022-04-21 at 12:02 +0200, Hans Verkuil wrote:
> Hi Philipp,
> 
> On 04/04/2022 18:35, Philipp Zabel wrote:
> > Set default colorspace to SRGB for JPEG encoder and decoder devices,
> > to fix the following v4l2-compliance test failure:
> > 
> > 	test VIDIOC_TRY_FMT: OK
> > 		fail: v4l2-test-formats.cpp(818): fmt_raw.g_colorspace() != V4L2_COLORSPACE_SRGB
> > 
> > Also explicitly set transfer function, YCbCr encoding and quantization
> > range, as required by v4l2-compliance for the JPEG encoded side.
> 
> I'm not quite sure if this patch addresses the correct issue.
> 
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  .../media/platform/chips-media/coda-common.c  | 36 +++++++++++++------
> >  1 file changed, 25 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c
> > index 4a7346ed771e..c068c16d1eb4 100644
> > --- a/drivers/media/platform/chips-media/coda-common.c
> > +++ b/drivers/media/platform/chips-media/coda-common.c
> > @@ -732,13 +732,22 @@ static int coda_try_fmt_vid_cap(struct file *file, void *priv,
> >  	return 0;
> >  }
> >  
> > 
> > 
> > 
> > -static void coda_set_default_colorspace(struct v4l2_pix_format *fmt)
> > +static void coda_set_default_colorspace(struct coda_ctx *ctx,
> > +					struct v4l2_pix_format *fmt)
> >  {
> >  	enum v4l2_colorspace colorspace;
> >  
> > 
> > 
> > 
> > -	if (fmt->pixelformat == V4L2_PIX_FMT_JPEG)
> > -		colorspace = V4L2_COLORSPACE_JPEG;
> 
> It's perfectly fine to keep this, the problem occurs with the raw image side
> (capture for the decoder, output for the encoder).
> 
> There the colorspace must be SRGB, the xfer_func may be 0 or SRGB, and the
> ycbcr_enc is 0 (if not a YUV pixelformat) or ENC_601 (if it is a YUV format).
> Actually, if the hardware can convert from other YUV encodings such as 709,
> then other YUV encodings are valid, but I assume that's not the case.

So the driver has to support different colorspace on output and capture
queues?

> > -	else if (fmt->width <= 720 && fmt->height <= 576)
> > +	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG ||
> > +	    ctx->cvd->dst_formats[0] == V4L2_PIX_FMT_JPEG ||
> > +	    fmt->pixelformat == V4L2_PIX_FMT_JPEG) {
> > +		fmt->colorspace = V4L2_COLORSPACE_SRGB;
> > +		fmt->xfer_func = V4L2_XFER_FUNC_SRGB;
> > +		fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
> > +		fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > +		return;
> > +	}
> > +
> > +	if (fmt->width <= 720 && fmt->height <= 576)
> >  		colorspace = V4L2_COLORSPACE_SMPTE170M;
> >  	else
> >  		colorspace = V4L2_COLORSPACE_REC709;
> > @@ -763,7 +772,7 @@ static int coda_try_fmt_vid_out(struct file *file, void *priv,
> >  		return ret;
> >  
> > 
> > 
> > 
> >  	if (f->fmt.pix.colorspace == V4L2_COLORSPACE_DEFAULT)
> > -		coda_set_default_colorspace(&f->fmt.pix);
> > +		coda_set_default_colorspace(ctx, &f->fmt.pix);
> >  
> > 
> > 
> > 
> >  	q_data_dst = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
> >  	codec = coda_find_codec(dev, f->fmt.pix.pixelformat, q_data_dst->fourcc);
> > @@ -1640,13 +1649,18 @@ static void set_default_params(struct coda_ctx *ctx)
> >  	csize = coda_estimate_sizeimage(ctx, usize, max_w, max_h);
> >  
> > 
> > 
> > 
> >  	ctx->params.codec_mode = ctx->codec->mode;
> > -	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG)
> > -		ctx->colorspace = V4L2_COLORSPACE_JPEG;
> > -	else
> > +	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG ||
> > +	    ctx->cvd->dst_formats[0] == V4L2_PIX_FMT_JPEG) {
> > +		ctx->colorspace = V4L2_COLORSPACE_SRGB;
> > +		ctx->xfer_func = V4L2_XFER_FUNC_SRGB;
> > +		ctx->ycbcr_enc = V4L2_YCBCR_ENC_601;
> > +		ctx->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > +	} else {
> >  		ctx->colorspace = V4L2_COLORSPACE_REC709;
> 
> My guess is that the raw format colorspace is set to REC709, which is definitely
> wrong for a JPEG codec. For a JPEG codec that must be set to SRGB.
> 
> I suspect that's the real bug here.
> 
> I'm skipping this patch for now.

Thank you, I think at least for the decoder the issue was that the
driver defaulted to V4L2_COLORSPACE_JPEG, but since ctx->colorspace is
used for both sides, that would also be used as colorspace for the raw
image side. For the encoder it looks like you are right.

I'll double check.

regards
Philipp

Re: [PATCH 5/7] media: coda: fix default JPEG colorimetry

[Hans Verkuil]

On 21/04/2022 12:38, Philipp Zabel wrote:
> On Do, 2022-04-21 at 12:02 +0200, Hans Verkuil wrote:
>> Hi Philipp,
>>
>> On 04/04/2022 18:35, Philipp Zabel wrote:
>>> Set default colorspace to SRGB for JPEG encoder and decoder devices,
>>> to fix the following v4l2-compliance test failure:
>>>
>>> 	test VIDIOC_TRY_FMT: OK
>>> 		fail: v4l2-test-formats.cpp(818): fmt_raw.g_colorspace() != V4L2_COLORSPACE_SRGB
>>>
>>> Also explicitly set transfer function, YCbCr encoding and quantization
>>> range, as required by v4l2-compliance for the JPEG encoded side.
>>
>> I'm not quite sure if this patch addresses the correct issue.
>>
>>>
>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>>> ---
>>>  .../media/platform/chips-media/coda-common.c  | 36 +++++++++++++------
>>>  1 file changed, 25 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c
>>> index 4a7346ed771e..c068c16d1eb4 100644
>>> --- a/drivers/media/platform/chips-media/coda-common.c
>>> +++ b/drivers/media/platform/chips-media/coda-common.c
>>> @@ -732,13 +732,22 @@ static int coda_try_fmt_vid_cap(struct file *file, void *priv,
>>>  	return 0;
>>>  }
>>>  
>>>
>>>
>>>
>>> -static void coda_set_default_colorspace(struct v4l2_pix_format *fmt)
>>> +static void coda_set_default_colorspace(struct coda_ctx *ctx,
>>> +					struct v4l2_pix_format *fmt)
>>>  {
>>>  	enum v4l2_colorspace colorspace;
>>>  
>>>
>>>
>>>
>>> -	if (fmt->pixelformat == V4L2_PIX_FMT_JPEG)
>>> -		colorspace = V4L2_COLORSPACE_JPEG;
>>
>> It's perfectly fine to keep this, the problem occurs with the raw image side
>> (capture for the decoder, output for the encoder).
>>
>> There the colorspace must be SRGB, the xfer_func may be 0 or SRGB, and the
>> ycbcr_enc is 0 (if not a YUV pixelformat) or ENC_601 (if it is a YUV format).
>> Actually, if the hardware can convert from other YUV encodings such as 709,
>> then other YUV encodings are valid, but I assume that's not the case.
> 
> So the driver has to support different colorspace on output and capture
> queues?

Correct. Note that it is OK to replace COLORSPACE_JPEG by explicit colorspace,
xfer_func, ycbcr_enc and quantization values, but in reality (almost?) all drivers
use COLORSPACE_JPEG, and that won't go away. Keeping it will certainly reduce
the patch size.

Regards,

	Hans

> 
>>> -	else if (fmt->width <= 720 && fmt->height <= 576)
>>> +	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG ||
>>> +	    ctx->cvd->dst_formats[0] == V4L2_PIX_FMT_JPEG ||
>>> +	    fmt->pixelformat == V4L2_PIX_FMT_JPEG) {
>>> +		fmt->colorspace = V4L2_COLORSPACE_SRGB;
>>> +		fmt->xfer_func = V4L2_XFER_FUNC_SRGB;
>>> +		fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
>>> +		fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
>>> +		return;
>>> +	}
>>> +
>>> +	if (fmt->width <= 720 && fmt->height <= 576)
>>>  		colorspace = V4L2_COLORSPACE_SMPTE170M;
>>>  	else
>>>  		colorspace = V4L2_COLORSPACE_REC709;
>>> @@ -763,7 +772,7 @@ static int coda_try_fmt_vid_out(struct file *file, void *priv,
>>>  		return ret;
>>>  
>>>
>>>
>>>
>>>  	if (f->fmt.pix.colorspace == V4L2_COLORSPACE_DEFAULT)
>>> -		coda_set_default_colorspace(&f->fmt.pix);
>>> +		coda_set_default_colorspace(ctx, &f->fmt.pix);
>>>  
>>>
>>>
>>>
>>>  	q_data_dst = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
>>>  	codec = coda_find_codec(dev, f->fmt.pix.pixelformat, q_data_dst->fourcc);
>>> @@ -1640,13 +1649,18 @@ static void set_default_params(struct coda_ctx *ctx)
>>>  	csize = coda_estimate_sizeimage(ctx, usize, max_w, max_h);
>>>  
>>>
>>>
>>>
>>>  	ctx->params.codec_mode = ctx->codec->mode;
>>> -	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG)
>>> -		ctx->colorspace = V4L2_COLORSPACE_JPEG;
>>> -	else
>>> +	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG ||
>>> +	    ctx->cvd->dst_formats[0] == V4L2_PIX_FMT_JPEG) {
>>> +		ctx->colorspace = V4L2_COLORSPACE_SRGB;
>>> +		ctx->xfer_func = V4L2_XFER_FUNC_SRGB;
>>> +		ctx->ycbcr_enc = V4L2_YCBCR_ENC_601;
>>> +		ctx->quantization = V4L2_QUANTIZATION_FULL_RANGE;
>>> +	} else {
>>>  		ctx->colorspace = V4L2_COLORSPACE_REC709;
>>
>> My guess is that the raw format colorspace is set to REC709, which is definitely
>> wrong for a JPEG codec. For a JPEG codec that must be set to SRGB.
>>
>> I suspect that's the real bug here.
>>
>> I'm skipping this patch for now.
> 
> Thank you, I think at least for the decoder the issue was that the
> driver defaulted to V4L2_COLORSPACE_JPEG, but since ctx->colorspace is
> used for both sides, that would also be used as colorspace for the raw
> image side. For the encoder it looks like you are right.
> 
> I'll double check.
> 
> regards
> Philipp

Re: [PATCH 5/7] media: coda: fix default JPEG colorimetry

[Philipp Zabel]

Hi Hans,

On Do, 2022-04-21 at 13:06 +0200, Hans Verkuil wrote:
> On 21/04/2022 12:38, Philipp Zabel wrote:
> > On Do, 2022-04-21 at 12:02 +0200, Hans Verkuil wrote:
> > > Hi Philipp,
> > > 
> > > On 04/04/2022 18:35, Philipp Zabel wrote:
> > > > Set default colorspace to SRGB for JPEG encoder and decoder devices,
> > > > to fix the following v4l2-compliance test failure:
> > > > 
> > > > 	test VIDIOC_TRY_FMT: OK
> > > > 		fail: v4l2-test-formats.cpp(818): fmt_raw.g_colorspace() != V4L2_COLORSPACE_SRGB
> > > > 
> > > > Also explicitly set transfer function, YCbCr encoding and quantization
> > > > range, as required by v4l2-compliance for the JPEG encoded side.
> > > 
> > > I'm not quite sure if this patch addresses the correct issue.
> > > 
> > > > 
> > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > > ---
> > > >  .../media/platform/chips-media/coda-common.c  | 36 +++++++++++++------
> > > >  1 file changed, 25 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c
> > > > index 4a7346ed771e..c068c16d1eb4 100644
> > > > --- a/drivers/media/platform/chips-media/coda-common.c
> > > > +++ b/drivers/media/platform/chips-media/coda-common.c
> > > > @@ -732,13 +732,22 @@ static int coda_try_fmt_vid_cap(struct file *file, void *priv,
> > > >  	return 0;
> > > >  }
> > > >  
> > > > 
> > > > 
> > > > 
> > > > -static void coda_set_default_colorspace(struct v4l2_pix_format *fmt)
> > > > +static void coda_set_default_colorspace(struct coda_ctx *ctx,
> > > > +					struct v4l2_pix_format *fmt)
> > > >  {
> > > >  	enum v4l2_colorspace colorspace;
> > > >  
> > > > 
> > > > 
> > > > 
> > > > -	if (fmt->pixelformat == V4L2_PIX_FMT_JPEG)
> > > > -		colorspace = V4L2_COLORSPACE_JPEG;
> > > 
> > > It's perfectly fine to keep this, the problem occurs with the raw image side
> > > (capture for the decoder, output for the encoder).
> > > 
> > > There the colorspace must be SRGB, the xfer_func may be 0 or SRGB, and the
> > > ycbcr_enc is 0 (if not a YUV pixelformat) or ENC_601 (if it is a YUV format).
> > > Actually, if the hardware can convert from other YUV encodings such as 709,
> > > then other YUV encodings are valid, but I assume that's not the case.
> > 
> > So the driver has to support different colorspace on output and capture
> > queues?
> 
> Correct. Note that it is OK to replace COLORSPACE_JPEG by explicit colorspace,
> xfer_func, ycbcr_enc and quantization values, but in reality (almost?) all drivers
> use COLORSPACE_JPEG, and that won't go away. Keeping it will certainly reduce
> the patch size.
> 
> Regards,
> 
> 	Hans
> 
> > 
> > > > -	else if (fmt->width <= 720 && fmt->height <= 576)
> > > > +	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG ||
> > > > +	    ctx->cvd->dst_formats[0] == V4L2_PIX_FMT_JPEG ||
> > > > +	    fmt->pixelformat == V4L2_PIX_FMT_JPEG) {
> > > > +		fmt->colorspace = V4L2_COLORSPACE_SRGB;
> > > > +		fmt->xfer_func = V4L2_XFER_FUNC_SRGB;
> > > > +		fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
> > > > +		fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	if (fmt->width <= 720 && fmt->height <= 576)
> > > >  		colorspace = V4L2_COLORSPACE_SMPTE170M;
> > > >  	else
> > > >  		colorspace = V4L2_COLORSPACE_REC709;

coda_set_default_colorspace() is only called when userspace calls S_FMT
with colorspace = V4L2_COLORSPACE_DEFAULT.

Since v4l2-compliance doesn't care about this, I've dropped this part.

> > > > @@ -763,7 +772,7 @@ static int coda_try_fmt_vid_out(struct file *file, void *priv,
> > > >  		return ret;
> > > >  
> > > > 
> > > > 
> > > > 
> > > >  	if (f->fmt.pix.colorspace == V4L2_COLORSPACE_DEFAULT)
> > > > -		coda_set_default_colorspace(&f->fmt.pix);
> > > > +		coda_set_default_colorspace(ctx, &f->fmt.pix);

And this.

> > > >  
> > > > 
> > > > 
> > > > 
> > > >  	q_data_dst = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
> > > >  	codec = coda_find_codec(dev, f->fmt.pix.pixelformat, q_data_dst->fourcc);
> > > > @@ -1640,13 +1649,18 @@ static void set_default_params(struct coda_ctx *ctx)
> > > >  	csize = coda_estimate_sizeimage(ctx, usize, max_w, max_h);
> > > >  
> > > > 
> > > > 
> > > > 
> > > >  	ctx->params.codec_mode = ctx->codec->mode;
> > > > -	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG)
> > > > -		ctx->colorspace = V4L2_COLORSPACE_JPEG;
> > > > -	else
> > > > +	if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG ||
> > > > +	    ctx->cvd->dst_formats[0] == V4L2_PIX_FMT_JPEG) {
> > > > +		ctx->colorspace = V4L2_COLORSPACE_SRGB;
> > > > +		ctx->xfer_func = V4L2_XFER_FUNC_SRGB;
> > > > +		ctx->ycbcr_enc = V4L2_YCBCR_ENC_601;
> > > > +		ctx->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > > > +	} else {
> > > >  		ctx->colorspace = V4L2_COLORSPACE_REC709;
> > > 
> > > My guess is that the raw format colorspace is set to REC709, which is definitely
> > > wrong for a JPEG codec. For a JPEG codec that must be set to SRGB.
> > > 
> > > I suspect that's the real bug here.

Right, this part is enough to make v4l2-compliance happy. I've sent a
v2 reduced to this.

The driver still only supports identical colorimetry on both queues, so
when userspace sets colorspace = V4L2_COLORSPACE_JPEG on the output
queue, the capture queue will be set to the same.

regards
Philipp

Re: [PATCH 5/7] media: coda: fix default JPEG colorimetry

[Philipp Zabel]

On Do, 2022-04-21 at 12:02 +0200, Hans Verkuil wrote:
[...]
> > -	if (fmt->pixelformat == V4L2_PIX_FMT_JPEG)
> > -		colorspace = V4L2_COLORSPACE_JPEG;
> 
> It's perfectly fine to keep this, the problem occurs with the raw image side
> (capture for the decoder, output for the encoder).
> 
> There the colorspace must be SRGB, the xfer_func may be 0 or SRGB,

v4l2-compliance doesn't allow xfer_func 0:

		fail: v4l2-test-formats.cpp(835): fmt_jpeg.g_xfer_func() != V4L2_XFER_FUNC_SRGB
	test VIDIOC_S_FMT: FAIL

Is this too strict?

> and the
> ycbcr_enc is 0 (if not a YUV pixelformat) or ENC_601 (if it is a YUV format).

Why does it have to be ENC_601 for YUV formats? Both
V4L2_MAP_YCBCR_ENC_DEFAULT(V4L2_COLORSPACE_JPEG) and
V4L2_MAP_YCBCR_ENC_DEFAULT(V4L2_COLORSPACE_SRGB) map to
V4L2_YCBCR_ENC_601 regardless of the pixel format, so there should be
no difference if 0 was allowed as well.

On the other hand, quantization should be set to
V4L2_QUANTIZATION_FULL_RANGE for YUV formats on the raw image side, as
that defaults to LIM_RANGE for raw YUV images with SRGB colorspace.

Basically, I wonder whether or not it would be a good idea to apply the
following change to v4l2-compliance:

diff --git a/utils/v4l2-compliance/v4l2-test-formats.cpp b/utils/v4l2-compliance/v4l2-test-formats.cpp
index 269a3832fcdf..902c66367ff7 100644
--- a/utils/v4l2-compliance/v4l2-test-formats.cpp
+++ b/utils/v4l2-compliance/v4l2-test-formats.cpp
@@ -832,8 +832,10 @@ static int testJPEGColorspace(const cv4l_fmt &fmt_raw, const cv4l_fmt &fmt_jpeg,
        }
        /* V4L2_COLORSPACE_JPEG is shorthand for these values: */
        fail_on_test(fmt_jpeg.g_colorspace() != V4L2_COLORSPACE_SRGB);
-       fail_on_test(fmt_jpeg.g_xfer_func() != V4L2_XFER_FUNC_SRGB);
-       fail_on_test(fmt_jpeg.g_ycbcr_enc() != V4L2_YCBCR_ENC_601);
+       fail_on_test(fmt_jpeg.g_xfer_func() != V4L2_XFER_FUNC_DEFAULT &&
+                    fmt_jpeg.g_xfer_func() != V4L2_XFER_FUNC_SRGB);
+       fail_on_test(fmt_jpeg.g_ycbcr_enc() != V4L2_YCBCR_ENC_DEFAULT &&
+                    fmt_jpeg.g_ycbcr_enc() != V4L2_YCBCR_ENC_601);
        fail_on_test(fmt_jpeg.g_quantization() != V4L2_QUANTIZATION_FULL_RANGE);
        return 0;
 }

Actually, if the hardware can convert from other YUV encodings such as 709,
then other YUV encodings are valid, but I assume that's not the case.

True, the hardware is completely oblivious to colorimetry.

regards
Philipp

Re: [PATCH 5/7] media: coda: fix default JPEG colorimetry

[Philipp Zabel]

On Do, 2022-04-21 at 16:54 +0200, Philipp Zabel wrote:
> On Do, 2022-04-21 at 12:02 +0200, Hans Verkuil wrote:
> [...]
> > > -	if (fmt->pixelformat == V4L2_PIX_FMT_JPEG)
> > > -		colorspace = V4L2_COLORSPACE_JPEG;
> > 
> > It's perfectly fine to keep this, the problem occurs with the raw image side
> > (capture for the decoder, output for the encoder).
> > 
> > There the colorspace must be SRGB, the xfer_func may be 0 or SRGB,
> 
> v4l2-compliance doesn't allow xfer_func 0:
> 
> 		fail: v4l2-test-formats.cpp(835): fmt_jpeg.g_xfer_func() != V4L2_XFER_FUNC_SRGB
> 	test VIDIOC_S_FMT: FAIL
> 
> Is this too strict?
> 
> > and the
> > ycbcr_enc is 0 (if not a YUV pixelformat) or ENC_601 (if it is a YUV format).
> 
> Why does it have to be ENC_601 for YUV formats? Both
> V4L2_MAP_YCBCR_ENC_DEFAULT(V4L2_COLORSPACE_JPEG) and
> V4L2_MAP_YCBCR_ENC_DEFAULT(V4L2_COLORSPACE_SRGB) map to
> V4L2_YCBCR_ENC_601 regardless of the pixel format, so there should be
> no difference if 0 was allowed as well.
> 
> On the other hand, quantization should be set to
> V4L2_QUANTIZATION_FULL_RANGE for YUV formats on the raw image side, as
> that defaults to LIM_RANGE for raw YUV images with SRGB colorspace.
> 
> Basically, I wonder whether or not it would be a good idea to apply the
> following change to v4l2-compliance:
> 
> diff --git a/utils/v4l2-compliance/v4l2-test-formats.cpp b/utils/v4l2-compliance/v4l2-test-formats.cpp
> index 269a3832fcdf..902c66367ff7 100644
> --- a/utils/v4l2-compliance/v4l2-test-formats.cpp
> +++ b/utils/v4l2-compliance/v4l2-test-formats.cpp
> @@ -832,8 +832,10 @@ static int testJPEGColorspace(const cv4l_fmt &fmt_raw, const cv4l_fmt &fmt_jpeg,
>         }
>         /* V4L2_COLORSPACE_JPEG is shorthand for these values: */
>         fail_on_test(fmt_jpeg.g_colorspace() != V4L2_COLORSPACE_SRGB);
> -       fail_on_test(fmt_jpeg.g_xfer_func() != V4L2_XFER_FUNC_SRGB);
> -       fail_on_test(fmt_jpeg.g_ycbcr_enc() != V4L2_YCBCR_ENC_601);
> +       fail_on_test(fmt_jpeg.g_xfer_func() != V4L2_XFER_FUNC_DEFAULT &&
> +                    fmt_jpeg.g_xfer_func() != V4L2_XFER_FUNC_SRGB);
> +       fail_on_test(fmt_jpeg.g_ycbcr_enc() != V4L2_YCBCR_ENC_DEFAULT &&
> +                    fmt_jpeg.g_ycbcr_enc() != V4L2_YCBCR_ENC_601);
>         fail_on_test(fmt_jpeg.g_quantization() != V4L2_QUANTIZATION_FULL_RANGE);
>         return 0;
>  }

Please disregard, I overlooked that this part is checking the encoded
image side. The raw image side is allowed 0 ycbcr_enc and xfer_func
already, and raw image quantization is not checked at all.

regards
Philipp

[PATCH 6/7] media: coda: enable capture G_PARM for stateful encoder

[Philipp Zabel]

Allow to call G_PARM with type == V4L2_BUF_TYPE_VIDEO_CAPTURE,
to fix the following v4l2-compliance test failure:

		fail: v4l2-test-formats.cpp(1344): ret && node->has_frmintervals
	test VIDIOC_G/S_PARM: FAIL

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

diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c
index c068c16d1eb4..33fcd8c7d72b 100644
--- a/drivers/media/platform/chips-media/coda-common.c
+++ b/drivers/media/platform/chips-media/coda-common.c
@@ -1341,9 +1341,6 @@ static int coda_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
 	struct coda_ctx *ctx = fh_to_ctx(fh);
 	struct v4l2_fract *tpf;
 
-	if (a->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
-		return -EINVAL;
-
 	a->parm.output.capability = V4L2_CAP_TIMEPERFRAME;
 	tpf = &a->parm.output.timeperframe;
 	tpf->denominator = ctx->params.framerate & CODA_FRATE_RES_MASK;
-- 
2.30.2

Re: [PATCH 6/7] media: coda: enable capture G_PARM for stateful encoder

[Nicolas Dufresne]

Le lundi 04 avril 2022 à 18:35 +0200, Philipp Zabel a écrit :
> Allow to call G_PARM with type == V4L2_BUF_TYPE_VIDEO_CAPTURE,
> to fix the following v4l2-compliance test failure:
> 
> 		fail: v4l2-test-formats.cpp(1344): ret && node->has_frmintervals
> 	test VIDIOC_G/S_PARM: FAIL

So basically the rate written in the bitstream (if any) will be the same as the
target real-time rate, which matches my reading of the new spec as what default
behaviour we should have.

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/media/platform/chips-media/coda-common.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c
> index c068c16d1eb4..33fcd8c7d72b 100644
> --- a/drivers/media/platform/chips-media/coda-common.c
> +++ b/drivers/media/platform/chips-media/coda-common.c
> @@ -1341,9 +1341,6 @@ static int coda_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
>  	struct coda_ctx *ctx = fh_to_ctx(fh);
>  	struct v4l2_fract *tpf;
>  
> -	if (a->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
> -		return -EINVAL;
> -
>  	a->parm.output.capability = V4L2_CAP_TIMEPERFRAME;
>  	tpf = &a->parm.output.timeperframe;
>  	tpf->denominator = ctx->params.framerate & CODA_FRATE_RES_MASK;

Re: [PATCH 6/7] media: coda: enable capture G_PARM for stateful encoder

[Hans Verkuil]

Hi Philipp,

On 04/04/2022 18:35, Philipp Zabel wrote:
> Allow to call G_PARM with type == V4L2_BUF_TYPE_VIDEO_CAPTURE,
> to fix the following v4l2-compliance test failure:
> 
> 		fail: v4l2-test-formats.cpp(1344): ret && node->has_frmintervals
> 	test VIDIOC_G/S_PARM: FAIL
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/media/platform/chips-media/coda-common.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c
> index c068c16d1eb4..33fcd8c7d72b 100644
> --- a/drivers/media/platform/chips-media/coda-common.c
> +++ b/drivers/media/platform/chips-media/coda-common.c
> @@ -1341,9 +1341,6 @@ static int coda_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
>  	struct coda_ctx *ctx = fh_to_ctx(fh);
>  	struct v4l2_fract *tpf;
>  
> -	if (a->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
> -		return -EINVAL;
> -
>  	a->parm.output.capability = V4L2_CAP_TIMEPERFRAME;
>  	tpf = &a->parm.output.timeperframe;
>  	tpf->denominator = ctx->params.framerate & CODA_FRATE_RES_MASK;

I think this is actually a v4l2-compliance bug, not a driver bug.

G/S_PARM doesn't make sense for the capture queue of a stateful encoder, unless
V4L2_FMT_FLAG_ENC_CAP_FRAME_INTERVAL is set to reserve HW resources.

See https://hverkuil.home.xs4all.nl/spec/userspace-api/v4l/vidioc-enum-fmt.html#fmtdesc-flags

That flags isn't used, so v4l2-compliance shouldn't complain.

Try this v4l2-compliance patch to see if it resolved the fails for this patch
and the next patch (7/7).

v4l2-compliance patch:

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
diff --git a/utils/v4l2-compliance/v4l2-test-formats.cpp b/utils/v4l2-compliance/v4l2-test-formats.cpp
index 3761b1fa..269a3832 100644
--- a/utils/v4l2-compliance/v4l2-test-formats.cpp
+++ b/utils/v4l2-compliance/v4l2-test-formats.cpp
@@ -1341,8 +1341,16 @@ static int testParmType(struct node *node, unsigned type)
 	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
 	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
 		if (node->g_caps() & buftype2cap[type]) {
-			fail_on_test(ret && node->has_frmintervals);
-			fail_on_test(ret && node->has_enc_cap_frame_interval);
+			if (is_stateful_enc) {
+				if (V4L2_TYPE_IS_OUTPUT(type))
+					fail_on_test(ret && node->has_frmintervals);
+				else if (node->has_enc_cap_frame_interval)
+					fail_on_test(ret);
+				else
+					fail_on_test(!ret);
+			} else {
+				fail_on_test(ret && node->has_frmintervals);
+			}
 		}
 		break;
 	default:

Re: [PATCH 6/7] media: coda: enable capture G_PARM for stateful encoder

[Philipp Zabel]

On Do, 2022-04-21 at 12:30 +0200, Hans Verkuil wrote:
[...]
> I think this is actually a v4l2-compliance bug, not a driver bug.
> 
> G/S_PARM doesn't make sense for the capture queue of a stateful encoder, unless
> V4L2_FMT_FLAG_ENC_CAP_FRAME_INTERVAL is set to reserve HW resources.
> 
> See https://hverkuil.home.xs4all.nl/spec/userspace-api/v4l/vidioc-enum-fmt.html#fmtdesc-flags
> 
> That flags isn't used, so v4l2-compliance shouldn't complain.
> 
> Try this v4l2-compliance patch to see if it resolved the fails for this patch
> and the next patch (7/7).
> 
> v4l2-compliance patch:
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
> diff --git a/utils/v4l2-compliance/v4l2-test-formats.cpp b/utils/v4l2-compliance/v4l2-test-formats.cpp
> index 3761b1fa..269a3832 100644
> --- a/utils/v4l2-compliance/v4l2-test-formats.cpp
> +++ b/utils/v4l2-compliance/v4l2-test-formats.cpp
> @@ -1341,8 +1341,16 @@ static int testParmType(struct node *node, unsigned type)
>  	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
>  	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>  		if (node->g_caps() & buftype2cap[type]) {
> -			fail_on_test(ret && node->has_frmintervals);
> -			fail_on_test(ret && node->has_enc_cap_frame_interval);
> +			if (is_stateful_enc) {
> +				if (V4L2_TYPE_IS_OUTPUT(type))
> +					fail_on_test(ret && node->has_frmintervals);
> +				else if (node->has_enc_cap_frame_interval)
> +					fail_on_test(ret);
> +				else
> +					fail_on_test(!ret);
> +			} else {
> +				fail_on_test(ret && node->has_frmintervals);
> +			}
>  		}
>  		break;
>  	default:

You are right, this patch resolves the compliance failures addressed by
patches 6 and 7.

regards
Philipp

Re: [PATCH 6/7] media: coda: enable capture G_PARM for stateful encoder

[Hans Verkuil]

On 21/04/2022 16:58, Philipp Zabel wrote:
> On Do, 2022-04-21 at 12:30 +0200, Hans Verkuil wrote:
> [...]
>> I think this is actually a v4l2-compliance bug, not a driver bug.
>>
>> G/S_PARM doesn't make sense for the capture queue of a stateful encoder, unless
>> V4L2_FMT_FLAG_ENC_CAP_FRAME_INTERVAL is set to reserve HW resources.
>>
>> See https://hverkuil.home.xs4all.nl/spec/userspace-api/v4l/vidioc-enum-fmt.html#fmtdesc-flags
>>
>> That flags isn't used, so v4l2-compliance shouldn't complain.
>>
>> Try this v4l2-compliance patch to see if it resolved the fails for this patch
>> and the next patch (7/7).
>>
>> v4l2-compliance patch:
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>> diff --git a/utils/v4l2-compliance/v4l2-test-formats.cpp b/utils/v4l2-compliance/v4l2-test-formats.cpp
>> index 3761b1fa..269a3832 100644
>> --- a/utils/v4l2-compliance/v4l2-test-formats.cpp
>> +++ b/utils/v4l2-compliance/v4l2-test-formats.cpp
>> @@ -1341,8 +1341,16 @@ static int testParmType(struct node *node, unsigned type)
>>  	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
>>  	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>>  		if (node->g_caps() & buftype2cap[type]) {
>> -			fail_on_test(ret && node->has_frmintervals);
>> -			fail_on_test(ret && node->has_enc_cap_frame_interval);
>> +			if (is_stateful_enc) {
>> +				if (V4L2_TYPE_IS_OUTPUT(type))
>> +					fail_on_test(ret && node->has_frmintervals);
>> +				else if (node->has_enc_cap_frame_interval)
>> +					fail_on_test(ret);
>> +				else
>> +					fail_on_test(!ret);
>> +			} else {
>> +				fail_on_test(ret && node->has_frmintervals);
>> +			}
>>  		}
>>  		break;
>>  	default:
> 
> You are right, this patch resolves the compliance failures addressed by
> patches 6 and 7.

Great! I've committed this change to v4l-utils.

Regards,

	Hans

[PATCH 7/7] media: coda: enable capture S_PARM for stateful encoder

[Philipp Zabel]

Allow to call S_PARM with type == V4L2_BUF_TYPE_VIDEO_OUTPUT,
to fix the following v4l2-compliance test failure:

		fail: v4l2-test-formats.cpp(1413): got error 22 when setting parms for buftype 1
	test VIDIOC_G/S_PARM: FAIL

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

diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c
index 33fcd8c7d72b..cd9ff2fa4147 100644
--- a/drivers/media/platform/chips-media/coda-common.c
+++ b/drivers/media/platform/chips-media/coda-common.c
@@ -1421,9 +1421,6 @@ static int coda_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
 	struct coda_ctx *ctx = fh_to_ctx(fh);
 	struct v4l2_fract *tpf;
 
-	if (a->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
-		return -EINVAL;
-
 	a->parm.output.capability = V4L2_CAP_TIMEPERFRAME;
 	tpf = &a->parm.output.timeperframe;
 	coda_approximate_timeperframe(tpf);
-- 
2.30.2

Re: [PATCH 7/7] media: coda: enable capture S_PARM for stateful encoder

[Nicolas Dufresne]

Le lundi 04 avril 2022 à 18:35 +0200, Philipp Zabel a écrit :
> Allow to call S_PARM with type == V4L2_BUF_TYPE_VIDEO_OUTPUT,
> to fix the following v4l2-compliance test failure:
> 
> 		fail: v4l2-test-formats.cpp(1413): got error 22 when setting parms for buftype 1
> 	test VIDIOC_G/S_PARM: FAIL

That one may be missing something though. As you don't implement performance
target, you need to override the value somehow with the value you wrote into the
bitstream no ? Otherwise we just ignore what userland sets silently ? I might
not have got exactly how this case is supposed to be handled. Looking for
feedback on what is proper behaviour for drivers that do not implement
performance targets (resource reservation).

> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/media/platform/chips-media/coda-common.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c
> index 33fcd8c7d72b..cd9ff2fa4147 100644
> --- a/drivers/media/platform/chips-media/coda-common.c
> +++ b/drivers/media/platform/chips-media/coda-common.c
> @@ -1421,9 +1421,6 @@ static int coda_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
>  	struct coda_ctx *ctx = fh_to_ctx(fh);
>  	struct v4l2_fract *tpf;
>  
> -	if (a->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
> -		return -EINVAL;
> -
>  	a->parm.output.capability = V4L2_CAP_TIMEPERFRAME;
>  	tpf = &a->parm.output.timeperframe;
>  	coda_approximate_timeperframe(tpf);

Re: [PATCH 7/7] media: coda: enable capture S_PARM for stateful encoder

[Philipp Zabel]

Hi Nicolas,

thank you for the review. You bring up a good point here, I think this
part of the spec is not very easy to understand.

On Di, 2022-04-05 at 10:22 -0400, Nicolas Dufresne wrote:
> Le lundi 04 avril 2022 à 18:35 +0200, Philipp Zabel a écrit :
> > Allow to call S_PARM with type == V4L2_BUF_TYPE_VIDEO_OUTPUT,
> > to fix the following v4l2-compliance test failure:
> > 
> >                 fail: v4l2-test-formats.cpp(1413): got error 22
> > when setting parms for buftype 1
> >         test VIDIOC_G/S_PARM: FAIL
> 
> That one may be missing something though. As you don't implement performance
> target, you need to override the value somehow with the value you wrote into the
> bitstream no ? Otherwise we just ignore what userland sets silently ?

You are right that we don't implement any performance targets.
But the spec also says [1]:

  Changing the OUTPUT frame interval also sets the framerate that the
  encoder uses to encode the video. So setting the frame interval to
  1/24 (or 24 frames per second) will produce a coded video stream that
  can be played back at that speed. The frame interval for the OUTPUT
  queue is just a hint, the application may provide raw frames at a
  different rate. It can be used by the driver to help schedule
  multiple encoders running in parallel.

  In the next step the CAPTURE frame interval can optionally be changed
  to a different value. This is useful for off-line encoding were the
  coded frame interval can be different from the rate at which raw
  frames are supplied.

And

  Changing the CAPTURE frame interval sets the framerate for the coded
  video. It does not set the rate at which buffers arrive on the
  CAPTURE queue, that depends on how fast the encoder is and how fast
  raw frames are queued on the OUTPUT queue.

[1] https://docs.kernel.org/userspace-api/media/v4l/dev-encoder.html?highlight=stateful%20encoder

So far I'm only implementing the OUTPUT->CAPTURE rate passthrough part
to make v4l2-compliance happy.

Since the "frame interval for the OUTPUT queue is just a hint" and the
spec only says that "the encoder may adjust it to match hardware
requirements", I felt free to just ignore the raw frame interval part
for now.
My understanding is that the driver may limit this value to the
achievable real time encoding speed (if it even knows).

One thing I'm not doing according to spec right now is that calling
S_PARM on CAPTURE will also change the OUTPUT interval, as the driver
just stores them in the same variable.
Also the driver does not set the V4L2_FMT_FLAG_ENC_CAP_FRAME_INTERVAL
to signal it supports S_PARM on CAPTURE.
My understanding is that the CAPTURE frame interval is the value that
should be written to the bitstream and that should be used for the
bitrate control algorithm.

> I might not have got exactly how this case is supposed to be handled.
> Looking for feedback on what is proper behaviour for drivers that do
> not implement performance targets (resource reservation).

Same here. I'd love to learn whether my understanding of the spec is
correct or not.

regards
Philipp

Re: [PATCH 7/7] media: coda: enable capture S_PARM for stateful encoder

[Nicolas Dufresne]

Le mardi 05 avril 2022 à 17:03 +0200, Philipp Zabel a écrit :
> Hi Nicolas,
> 
> thank you for the review. You bring up a good point here, I think this
> part of the spec is not very easy to understand.
> 
> On Di, 2022-04-05 at 10:22 -0400, Nicolas Dufresne wrote:
> > Le lundi 04 avril 2022 à 18:35 +0200, Philipp Zabel a écrit :
> > > Allow to call S_PARM with type == V4L2_BUF_TYPE_VIDEO_OUTPUT,
> > > to fix the following v4l2-compliance test failure:
> > > 
> > >                 fail: v4l2-test-formats.cpp(1413): got error 22
> > > when setting parms for buftype 1
> > >         test VIDIOC_G/S_PARM: FAIL
> > 
> > That one may be missing something though. As you don't implement performance
> > target, you need to override the value somehow with the value you wrote into the
> > bitstream no ? Otherwise we just ignore what userland sets silently ?
> 
> You are right that we don't implement any performance targets.
> But the spec also says [1]:
> 
>   Changing the OUTPUT frame interval also sets the framerate that the
>   encoder uses to encode the video. So setting the frame interval to
>   1/24 (or 24 frames per second) will produce a coded video stream that
>   can be played back at that speed. The frame interval for the OUTPUT
>   queue is just a hint, the application may provide raw frames at a
>   different rate. It can be used by the driver to help schedule
>   multiple encoders running in parallel.
> 
>   In the next step the CAPTURE frame interval can optionally be changed
>   to a different value. This is useful for off-line encoding were the
>   coded frame interval can be different from the rate at which raw
>   frames are supplied.
> 
> And
> 
>   Changing the CAPTURE frame interval sets the framerate for the coded
>   video. It does not set the rate at which buffers arrive on the
>   CAPTURE queue, that depends on how fast the encoder is and how fast
>   raw frames are queued on the OUTPUT queue.
> 
> [1] https://docs.kernel.org/userspace-api/media/v4l/dev-encoder.html?highlight=stateful%20encoder
> 
> So far I'm only implementing the OUTPUT->CAPTURE rate passthrough part
> to make v4l2-compliance happy.
> 
> Since the "frame interval for the OUTPUT queue is just a hint" and the
> spec only says that "the encoder may adjust it to match hardware
> requirements", I felt free to just ignore the raw frame interval part
> for now.
> My understanding is that the driver may limit this value to the
> achievable real time encoding speed (if it even knows).
> 
> One thing I'm not doing according to spec right now is that calling
> S_PARM on CAPTURE will also change the OUTPUT interval, as the driver
> just stores them in the same variable.
> Also the driver does not set the V4L2_FMT_FLAG_ENC_CAP_FRAME_INTERVAL
> to signal it supports S_PARM on CAPTURE.
> My understanding is that the CAPTURE frame interval is the value that
> should be written to the bitstream and that should be used for the
> bitrate control algorithm.

This is another very good point, if a driver does not set
V4L2_FMT_FLAG_ENC_CAP_FRAME_INTERVAL, why can't it return ENOTTY for
S_PARAM(CAPTURE) ? Is the test even correct?
> 
> > I might not have got exactly how this case is supposed to be handled.
> > Looking for feedback on what is proper behaviour for drivers that do
> > not implement performance targets (resource reservation).
> 
> Same here. I'd love to learn whether my understanding of the spec is
> correct or not.
> 
> regards
> Philipp

Re: [PATCH 1/7] media: coda: set output buffer bytesused to appease v4l2-compliance

[Nicolas Dufresne]

Le lundi 04 avril 2022 à 18:35 +0200, Philipp Zabel a écrit :
> The V4L2 specification states:
> 
>  "If the application sets this to 0 for an output stream, then bytesused
>   will be set to the size of the buffer (see the length field of this
>   struct) by the driver."
> 
> Since we set allow_zero_bytesused, we have to handle this ourselves.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> ---
>  drivers/media/platform/chips-media/coda-bit.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/platform/chips-media/coda-bit.c b/drivers/media/platform/chips-media/coda-bit.c
> index c484c008ab02..705a179ea8f0 100644
> --- a/drivers/media/platform/chips-media/coda-bit.c
> +++ b/drivers/media/platform/chips-media/coda-bit.c
> @@ -381,6 +381,9 @@ void coda_fill_bitstream(struct coda_ctx *ctx, struct list_head *buffer_list)
>  		/* Dump empty buffers */
>  		if (!vb2_get_plane_payload(&src_buf->vb2_buf, 0)) {
>  			src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> +			vb2_set_plane_payload(&src_buf->vb2_buf, 0,
> +					      vb2_plane_size(&src_buf->vb2_buf,
> +							     0));
>  			v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_DONE);
>  			continue;
>  		}

Re: [PATCH 1/7] media: coda: set output buffer bytesused to appease v4l2-compliance

[Hans Verkuil]

On 04/04/2022 18:35, Philipp Zabel wrote:
> The V4L2 specification states:
> 
>  "If the application sets this to 0 for an output stream, then bytesused
>   will be set to the size of the buffer (see the length field of this
>   struct) by the driver."
> 
> Since we set allow_zero_bytesused, we have to handle this ourselves.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/media/platform/chips-media/coda-bit.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/platform/chips-media/coda-bit.c b/drivers/media/platform/chips-media/coda-bit.c
> index c484c008ab02..705a179ea8f0 100644
> --- a/drivers/media/platform/chips-media/coda-bit.c
> +++ b/drivers/media/platform/chips-media/coda-bit.c
> @@ -381,6 +381,9 @@ void coda_fill_bitstream(struct coda_ctx *ctx, struct list_head *buffer_list)
>  		/* Dump empty buffers */
>  		if (!vb2_get_plane_payload(&src_buf->vb2_buf, 0)) {
>  			src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> +			vb2_set_plane_payload(&src_buf->vb2_buf, 0,
> +					      vb2_plane_size(&src_buf->vb2_buf,
> +							     0));

Would it be possible to stop using allow_zero_bytesused altogether?

Are there still applications that rely on zero-sized output buffers to stop the
decoder?

I'm not actually sure that I want this in the driver, perhaps v4l2-compliance
can be modified to turn a fail into a warn if the driver is the coda driver.

Patching the driver is hiding the fact that the coda driver does something
non-standard for legacy reasons. It doesn't make sense either to change
bytesused to the buffer size since there really is nothing in the buffer.

v4l2-compliance already has checks for two drivers, search for is_vivid and
is_uvcvideo.

I'm skipping this patch for now.

Regards,

	Hans

>  			v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_DONE);
>  			continue;
>  		}

Re: [PATCH 1/7] media: coda: set output buffer bytesused to appease v4l2-compliance

[Philipp Zabel]

Hi Hans,

On Do, 2022-04-21 at 11:44 +0200, Hans Verkuil wrote:
> On 04/04/2022 18:35, Philipp Zabel wrote:
> > The V4L2 specification states:
> > 
> >  "If the application sets this to 0 for an output stream, then bytesused
> >   will be set to the size of the buffer (see the length field of this
> >   struct) by the driver."
> > 
> > Since we set allow_zero_bytesused, we have to handle this ourselves.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  drivers/media/platform/chips-media/coda-bit.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/media/platform/chips-media/coda-bit.c b/drivers/media/platform/chips-media/coda-bit.c
> > index c484c008ab02..705a179ea8f0 100644
> > --- a/drivers/media/platform/chips-media/coda-bit.c
> > +++ b/drivers/media/platform/chips-media/coda-bit.c
> > @@ -381,6 +381,9 @@ void coda_fill_bitstream(struct coda_ctx *ctx, struct list_head *buffer_list)
> >  		/* Dump empty buffers */
> >  		if (!vb2_get_plane_payload(&src_buf->vb2_buf, 0)) {
> >  			src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> > +			vb2_set_plane_payload(&src_buf->vb2_buf, 0,
> > +					      vb2_plane_size(&src_buf->vb2_buf,
> > +							     0));
> 
> Would it be possible to stop using allow_zero_bytesused altogether?
> 
> Are there still applications that rely on zero-sized output buffers to stop the
> decoder?

This was used by GStreamer 1.8. The code is still left in current
versions, but is never executed unless the decoder stop command fails:

https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-good/sys/v4l2/gstv4l2videodec.c#L446-454

Whether there are still any applications using GStreamer 1.8 for V4L2
video decoding on devices that get kernel updates, I don't know.

> I'm not actually sure that I want this in the driver, perhaps v4l2-compliance
> can be modified to turn a fail into a warn if the driver is the coda driver.

Same for nxp/imx-jpeg, qcom/venus and samsung/s5p-mfc?

> Patching the driver is hiding the fact that the coda driver does something
> non-standard for legacy reasons. It doesn't make sense either to change
> bytesused to the buffer size since there really is nothing in the buffer.
>
> v4l2-compliance already has checks for two drivers, search for is_vivid and
> is_uvcvideo.

Ok.

> I'm skipping this patch for now.

regards
Philipp

Re: [PATCH 1/7] media: coda: set output buffer bytesused to appease v4l2-compliance

[Hans Verkuil]

On 21/04/2022 12:24, Philipp Zabel wrote:
> Hi Hans,
> 
> On Do, 2022-04-21 at 11:44 +0200, Hans Verkuil wrote:
>> On 04/04/2022 18:35, Philipp Zabel wrote:
>>> The V4L2 specification states:
>>>
>>>  "If the application sets this to 0 for an output stream, then bytesused
>>>   will be set to the size of the buffer (see the length field of this
>>>   struct) by the driver."
>>>
>>> Since we set allow_zero_bytesused, we have to handle this ourselves.
>>>
>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>>> ---
>>>  drivers/media/platform/chips-media/coda-bit.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/media/platform/chips-media/coda-bit.c b/drivers/media/platform/chips-media/coda-bit.c
>>> index c484c008ab02..705a179ea8f0 100644
>>> --- a/drivers/media/platform/chips-media/coda-bit.c
>>> +++ b/drivers/media/platform/chips-media/coda-bit.c
>>> @@ -381,6 +381,9 @@ void coda_fill_bitstream(struct coda_ctx *ctx, struct list_head *buffer_list)
>>>  		/* Dump empty buffers */
>>>  		if (!vb2_get_plane_payload(&src_buf->vb2_buf, 0)) {
>>>  			src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
>>> +			vb2_set_plane_payload(&src_buf->vb2_buf, 0,
>>> +					      vb2_plane_size(&src_buf->vb2_buf,
>>> +							     0));
>>
>> Would it be possible to stop using allow_zero_bytesused altogether?
>>
>> Are there still applications that rely on zero-sized output buffers to stop the
>> decoder?
> 
> This was used by GStreamer 1.8. The code is still left in current
> versions, but is never executed unless the decoder stop command fails:
> 
> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-good/sys/v4l2/gstv4l2videodec.c#L446-454
> 
> Whether there are still any applications using GStreamer 1.8 for V4L2
> video decoding on devices that get kernel updates, I don't know.
> 
>> I'm not actually sure that I want this in the driver, perhaps v4l2-compliance
>> can be modified to turn a fail into a warn if the driver is the coda driver.
> 
> Same for nxp/imx-jpeg, qcom/venus and samsung/s5p-mfc?

Yes for venus and s5p, but why would imx-jpeg use this? It makes no sense
for a jpeg codec. I think it should just be removed for imx-jpeg.

IMHO, once a decoder supports the STOP command, it should no longer set
allow_zero_bytesused to true. But that decision is up to you for the coda
driver.

Regards,

	Hans

> 
>> Patching the driver is hiding the fact that the coda driver does something
>> non-standard for legacy reasons. It doesn't make sense either to change
>> bytesused to the buffer size since there really is nothing in the buffer.
>>
>> v4l2-compliance already has checks for two drivers, search for is_vivid and
>> is_uvcvideo.
> 
> Ok.
> 
>> I'm skipping this patch for now.
> 
> regards
> Philipp

Re: [PATCH 1/7] media: coda: set output buffer bytesused to appease v4l2-compliance

[Philipp Zabel]

On Di, 2022-04-26 at 07:52 +0200, Hans Verkuil wrote:
[...]
> > > Are there still applications that rely on zero-sized output buffers to stop the
> > > decoder?
> > 
> > This was used by GStreamer 1.8. The code is still left in current
> > versions, but is never executed unless the decoder stop command fails:
> > 
> > https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-good/sys/v4l2/gstv4l2videodec.c#L446-454
> > 
> > Whether there are still any applications using GStreamer 1.8 for V4L2
> > video decoding on devices that get kernel updates, I don't know.
> > 
> > > I'm not actually sure that I want this in the driver, perhaps v4l2-compliance
> > > can be modified to turn a fail into a warn if the driver is the coda driver.
> > 
> > Same for nxp/imx-jpeg, qcom/venus and samsung/s5p-mfc?
> 
> Yes for venus and s5p, but why would imx-jpeg use this?
>
> It makes no sense for a jpeg codec. I think it should just be removed for imx-jpeg.
>
> IMHO, once a decoder supports the STOP command, it should no longer set
> allow_zero_bytesused to true. But that decision is up to you for the coda
> driver.

Turns out there is no associated v4l2-compliance failure at all.
I'd just drop this patch for now and keep the allow_zero_bytesused flag
as-is.

regards
Philipp