Re: [PATCH v7 10/28] media: rockchip: rga: announce and sync colorimetry

[Michael Tretter <m.tretter@pengutronix.de>]

"announce colorimetry" in the subject is a bit strange. Maybe rephrase
the subject to

	media: rockchip: rga: announce CSC and sync colorimetry

On Thu, 21 May 2026 00:44:15 +0200, Sven Püschel wrote:
> Announce the capability to adjust the quantization and ycbcr_enc on the
> capture side and check if the SET_CSC flag is set when the colorimetry
> is changed. Furthermore copy the colorimetry from the output to the
> capture side to fix the currently failing v4l2-compliance tests, which
> expect exactly this behavior.
> 
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Signed-off-by: Sven Püschel <s.pueschel@pengutronix.de>
> ---
>  drivers/media/platform/rockchip/rga/rga.c | 37 +++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
> index ca8d8a53dc251..8c34f73d69764 100644
> --- a/drivers/media/platform/rockchip/rga/rga.c
> +++ b/drivers/media/platform/rockchip/rga/rga.c
> @@ -437,6 +437,15 @@ static int vidioc_enum_fmt(struct file *file, void *priv, struct v4l2_fmtdesc *f
>  	fmt = &formats[f->index];
>  	f->pixelformat = fmt->fourcc;
>  
> +	if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE &&
> +	    f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)

Is there a reason for not using V4L2_TYPE_IS_CAPTURE(f->type)? I'd also
invert the condition, set the flags in the branch and have a single exit
point of this function.

> +		return 0;
> +
> +	/* allow changing the quantization and xfer func for YUV formats */
> +	if (v4l2_is_format_yuv(v4l2_format_info(f->pixelformat)))
> +		f->flags |= V4L2_FMT_FLAG_CSC_QUANTIZATION |
> +			    V4L2_FMT_FLAG_CSC_YCBCR_ENC;
> +
>  	return 0;
>  }
>  
> @@ -459,8 +468,25 @@ static int vidioc_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  {
>  	struct v4l2_pix_format_mplane *pix_fmt = &f->fmt.pix_mp;
> +	struct rga_ctx *ctx = file_to_rga_ctx(file);
>  	struct rga_fmt *fmt;
>  
> +	if (V4L2_TYPE_IS_CAPTURE(f->type)) {
> +		const struct rga_frame *frm;
> +
> +		frm = rga_get_frame(ctx, f->type);
> +		if (IS_ERR(frm))
> +			return PTR_ERR(frm);
> +
> +		if (!(pix_fmt->flags & V4L2_PIX_FMT_FLAG_SET_CSC)) {
> +			pix_fmt->quantization = frm->pix.quantization;
> +			pix_fmt->ycbcr_enc = frm->pix.ycbcr_enc;
> +		}

Are there any limits on the colorspace conversion that the RGA can do?
If I understand correctly, user space may set an arbitrary
v4l2_ycbcr_encoding (for example V4L2_YCBCR_ENC_BT2020) and the driver
will happily accept it.

> +		/* disallow values not announced in vidioc_enum_fmt */

"disallow values" sounds strange. Maybe:

		/* The RGA cannot convert colorspace and xfer_func */

> +		pix_fmt->colorspace = frm->pix.colorspace;
> +		pix_fmt->xfer_func = frm->pix.xfer_func;
> +	}
> +
>  	fmt = rga_fmt_find(pix_fmt->pixelformat);
>  	if (!fmt)
>  		fmt = &formats[0];
> @@ -506,6 +532,17 @@ static int vidioc_s_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  	frm->fmt = rga_fmt_find(pix_fmt->pixelformat);
>  	frm->stride = pix_fmt->plane_fmt[0].bytesperline;
>  
> +	/*
> +	 * Copy colorimetry from output to capture as required by the
> +	 * v4l2-compliance tests
> +	 */
> +	if (V4L2_TYPE_IS_OUTPUT(f->type)) {
> +		ctx->out.pix.colorspace = pix_fmt->colorspace;
> +		ctx->out.pix.ycbcr_enc = pix_fmt->ycbcr_enc;
> +		ctx->out.pix.quantization = pix_fmt->quantization;
> +		ctx->out.pix.xfer_func = pix_fmt->xfer_func;

I was very confused, because ctx->out is actually the format for
CAPTURE. The comment kind of helps to mark this trap. Not sure, if there
is anything that can be done about this.

Michael

> +	}
> +
>  	/* Reset crop settings */
>  	frm->crop.left = 0;
>  	frm->crop.top = 0;
> 
> -- 
> 2.54.0
> 
>