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: 4650 bytes --]

Le mardi 12 mai 2026 à 16:56 +0200, Sven Püschel a écrit :
> Hi Nicolas,
> 
> On 5/8/26 11:18 PM, Nicolas Dufresne wrote:
> > 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;
> 
> I agree that this looks better. My main intention is to be more 
> conservative, as the Rockchip related code/docs seem to always ensure a 
> 2 pixel alignment in both directions even for formats like YUV422, where 
> we shouldn't have an alignment requirement in the height. Besides the 
> vendor driver and TRM mentioned in the datasheet, the librga and it's 
> docs also mention an alignment of 2 (pixels?!) for all YUV formats (see 
> format alignment list in [1] and Q2.5 in [2]).
> 
> Given that the driver currently doesn't have any way to get the cores 
> unstuck/reset in case of a hang, I'd like to play it more safely by 
> adhering to what the vendor does instead of trying to do more and 
> therefore allowing some potential breaking format. E.g. I've also 
> experimented with sizes of 68x2, which the librga allows as input/output 
> of the RGA3 (whereas the TRM specifies a min size of 128x128), but 
> quickly dropped it as it produced some interesting broken outputs.

Agreed, simply preserve my Rb in next version.

cheers,
Nicolas

> 
> Sincerely
>      Sven
> 
> 
> [1] 
> https://codeberg.org/airockchip/librga/src/branch/main/docs/Rockchip_Developer_Guide_RGA_EN.md#image-format-alignment-instructions
> 
> [2] 
> https://codeberg.org/airockchip/librga/src/branch/main/docs/Rockchip_FAQ_RGA_EN.md
> 
> > 
> > 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 --]