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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:

New issues:
- [Medium] Hardware Mismatch / Fake API Implementation
- [Medium] State Corruption / Race Condition in vidioc_s_fmt
- [Medium] Uninitialized State Leak overriding valid TRY_FMT colorspace
- [Low] Missing Validation / API Violation for SET_CSC
- [Low] Code Quality / Documentation Mismatch in vidioc_enum_fmt

Pre-existing issues:
- [High] UAF / NULL Pointer Dereference due to unsynchronized stop_streaming
- [High] NULL Pointer Dereference in ISR when ctx is NULL
--

commit 1fbef10f8fc2594899895cefaffc9c924524b76e
Author: Sven Püschel <s.pueschel@pengutronix.de>
Subject: media: rockchip: rga: announce and sync colorimetry

This patch announces the capability to adjust quantization and ycbcr_enc for
capture formats in the Rockchip RGA driver. It checks the SET_CSC flag
when colorimetry changes and synchronizes colorimetry from the output queue
to the capture queue to satisfy v4l2-compliance tests.

> 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 */

[Severity: Low]
Does the comment mismatch the code below? It mentions the xfer func, but the
flags set are for YCBCR_ENC.

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

[Severity: Medium]
Is it safe to advertise these CSC flags if the hardware doesn't enforce them?

It appears the hardware programming logic ignores the quantization and
ycbcr_enc fields, determining the CSC mode based solely on the colorspace.

Falsely advertising capabilities to pass compliance tools without actual
hardware enforcement could lead to incorrect colorimetry outputs.

> +
>  	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)) {

[Severity: Low]
Could this accept the SET_CSC flag for any format? The code in enum_fmt
restricts the CSC capability flags to YUV formats, but there doesn't seem
to be a check here to validate if the requested format is actually YUV.

Additionally, should the driver clear the SET_CSC flag from pix_fmt->flags
if the CSC settings are not supported for the requested format?

> +			pix_fmt->quantization = frm->pix.quantization;
> +			pix_fmt->ycbcr_enc = frm->pix.ycbcr_enc;
> +		}
> +		/* disallow values not announced in vidioc_enum_fmt */
> +		pix_fmt->colorspace = frm->pix.colorspace;

[Severity: Medium]
Will this overwrite a valid requested colorspace with an uninitialized value?

During context initialization, ctx->out.pix.colorspace defaults to 0
(V4L2_COLORSPACE_DEFAULT).

If an application calls TRY_FMT on the capture queue before configuring
the output queue, this logic forcibly replaces the user-provided
colorspace with 0. Returning V4L2_COLORSPACE_DEFAULT from TRY_FMT is
generally disallowed by the API.

> +		pix_fmt->xfer_func = frm->pix.xfer_func;
> +	}
> +
>  	fmt = rga_fmt_find(pix_fmt->pixelformat);
>  	if (!fmt)

[ ... ]

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

[Severity: Medium]
Can this create a race condition if the CAPTURE queue is currently streaming?

The driver correctly checks if the OUTPUT queue is busy earlier in this
function, but it doesn't verify if the CAPTURE queue is active.

Mutating ctx->out.pix while the CAPTURE queue is active could cause
hardware configuration routines to asynchronously read corrupted or
intermediate state without appropriate queue locks.

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

[Severity: High]
This is a pre-existing issue, but does rga_buf_stop_streaming() leave
hardware jobs running when clearing the queues?

When rga_buf_stop_streaming() calls rga_buf_return_buffers() with
VB2_BUF_STATE_ERROR, it removes all buffers from the M2M context queues.
However, it does not appear to instruct the hardware to stop processing
the current frame or wait for completion.

When the active hardware job completes, it triggers an interrupt, and
the ISR attempts to fetch processed buffers via v4l2_m2m_src_buf_remove().
Since the queue was emptied, this returns NULL, which then gets blindly
dereferenced in v4l2_m2m_buf_copy_metadata().

[Severity: High]
This is a pre-existing issue, but is it possible for rga_isr() to
dereference a NULL context pointer?

In rga_isr(), the driver assigns ctx = rga->curr. If ctx is NULL due to
a spurious interrupt or delayed state update, the driver triggers a
warning via WARN_ON(!ctx).

However, execution continues and the driver immediately dereferences the
NULL ctx pointer in the call to v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx),
leading to a crash.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-spu-rga3-v7-0-3f33e8c7145f@pengutronix.de?part=10