Re: [PATCH v5 12/29] media: rockchip: rga: avoid odd frame sizes for YUV formats

[Nicolas Dufresne <nicolas@ndufresne.ca>]

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

Le mardi 28 avril 2026 à 11:00 +0200, Sven Püschel a écrit :
> Avoid odd frame sizes for YUV formats, as they may cause undefined
> behavior. This is done in preparation for the RGA3, which hangs when the
> output format is set to 129x129 pixel YUV420 SP (NV12).
> 
> This requirement is documented explicitly for the RGA3 in  section 5.6.3
> of the RK3588 TRM Part 2. For the RGA2 the RK3588 TRM Part 2
> (section 6.1.2) and RK3568 TRM Part 2 (section 14.2) only mentions the
> x/y offsets and stride aligning requirements. But the vendor driver for
> the RGA2 also contains checks for the width and height to be aligned to
> 2 bytes.
> 
> Signed-off-by: Sven Püschel <s.pueschel@pengutronix.de>
> ---
>  drivers/media/platform/rockchip/rga/rga.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
> index f599c992829dd..77b8c7ab74274 100644
> --- a/drivers/media/platform/rockchip/rga/rga.c
> +++ b/drivers/media/platform/rockchip/rga/rga.c
> @@ -337,6 +337,19 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  	struct rga_ctx *ctx = file_to_rga_ctx(file);
>  	const struct rga_hw *hw = ctx->rga->hw;
>  	struct rga_fmt *fmt;
> +	struct v4l2_frmsize_stepwise frmsize = {
> +		.min_width = hw->min_width,
> +		.max_width = hw->max_width,
> +		.min_height = hw->min_height,
> +		.max_height = hw->max_height,
> +		.step_width = 1,
> +		.step_height = 1,
> +	};
> +
> +	if (v4l2_is_format_yuv(v4l2_format_info(pix_fmt->pixelformat))) {
> +		frmsize.step_width = 2;
> +		frmsize.step_height = 2;

I think its fine like this, so let's start with:

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

But it does not feel like a hardware alignment to me. When we process in
software these things, the minimum alignment is bound to the subsampling, since
there is no way to store half or quarter pixels, the padded width/height
requires a step that follow the subsampling, something like:

		frmsize.step_width = finfo->hdiv;
		frmsize.step_height = finfo->vdiv;

Nicolas


> +	}
>  
>  	if (V4L2_TYPE_IS_CAPTURE(f->type)) {
>  		const struct rga_frame *frm;
> @@ -358,11 +371,7 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  	if (!fmt)
>  		fmt = &hw->formats[0];
>  
> -	pix_fmt->width = clamp(pix_fmt->width,
> -			       hw->min_width, hw->max_width);
> -	pix_fmt->height = clamp(pix_fmt->height,
> -				hw->min_height, hw->max_height);
> -
> +	v4l2_apply_frmsize_constraints(&pix_fmt->width, &pix_fmt->height, &frmsize);
>  	v4l2_fill_pixfmt_mp(pix_fmt, fmt->fourcc, pix_fmt->width, pix_fmt->height);
>  	pix_fmt->field = V4L2_FIELD_NONE;
>  

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