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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Falsely advertising hardware capabilities for CSC quantization and YCbCr encoding.
- [High] Missing validation and clamping of colorimetry fields in `vidioc_try_fmt`.
- [Medium] `vidioc_try_fmt` uses the CAPTURE queue's state instead of the OUTPUT queue's state as the default colorimetry.
- [Medium] `vidioc_try_fmt` accepts user-provided colorimetry for non-YUV formats, bypassing intended restrictions.
- [Low] Comment in `vidioc_enum_fmt` contradicts the implemented code.
--

commit 0b2b6b238c69f747c597a881c5d1349d57e712fe
Author: Sven Püschel <s.pueschel@pengutronix.de>

media: rockchip: rga: announce and sync colorimetry
    
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.

> 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)
> +		return 0;
> +
> +	/* allow changing the quantization and xfer func for YUV formats */

This isn't a bug, but the comment mentions "xfer func" instead of
"ycbcr_enc", while the code sets V4L2_FMT_FLAG_CSC_YCBCR_ENC. Furthermore,
vidioc_try_fmt explicitly disallows modifying xfer_func.

> +	if (v4l2_is_format_yuv(v4l2_format_info(f->pixelformat)))
> +		f->flags |= V4L2_FMT_FLAG_CSC_QUANTIZATION |
> +			    V4L2_FMT_FLAG_CSC_YCBCR_ENC;

Does the hardware actually support applying custom quantization and YCbCr
encoding matrices?

Looking at rga_cmd_set_trans_info() in rga-hw.c, the hardware configuration
backend appears to ignore quantization and ycbcr_enc, hardcoding the CSC
mode to either RGA_SRC_CSC_MODE_BT709_R0 or RGA_SRC_CSC_MODE_BT601_R0 based
purely on colorspace.

If the capabilities are announced merely to pass v4l2-compliance without
applying them in hardware, could this cause the driver to silently ignore
user requests and produce visually incorrect output?

[ ... ]

> @@ -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);

Is it correct to use the CAPTURE queue's state here instead of the OUTPUT
queue's state as the default colorimetry?

For memory-to-memory devices, when V4L2_PIX_FMT_FLAG_SET_CSC is not set on
the CAPTURE queue, it usually falls back to the colorimetry of the OUTPUT
queue (ctx->in). Fetching the frame with f->type resolves to ctx->out for
the CAPTURE queue.

If a user previously used SET_CSC to override the CAPTURE queue's
colorimetry, wouldn't a subsequent call without SET_CSC inherit this
overridden state instead of resetting to the OUTPUT queue's defaults?

> +		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;
> +		}

When V4L2_PIX_FMT_FLAG_SET_CSC is set, does this blindly accept
pix_fmt->quantization and pix_fmt->ycbcr_enc without validating if they are
supported enum values?

Additionally, does this enforce the restriction that colorimetry can only
be changed for YUV formats? vidioc_enum_fmt restricts the CSC flags to YUV
formats, but this code seems to honor SET_CSC on the CAPTURE queue
regardless of the format type, allowing arbitrary colorimetry values even
for RGB formats.

Furthermore, for the OUTPUT queue, the colorimetry fields don't seem to be
validated or resolved. Does the driver need to resolve
V4L2_COLORSPACE_DEFAULT to concrete standards and clamp unsupported values
during try_fmt as required by the V4L2 specification?

> +		/* disallow values not announced in vidioc_enum_fmt */
> +		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];

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-spu-rga3-v6-0-e547152eb9c9@pengutronix.de?part=10