Re: [PATCH v5 16/29] media: rockchip: rga: split flip and rotate into separate function

[Nicolas Dufresne <nicolas@ndufresne.ca>]

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

Le mardi 12 mai 2026 à 16:08 +0200, Sven Püschel a écrit :
> Hi Nicolas,
> 
> On 5/9/26 12:11 AM, Nicolas Dufresne wrote:
> > Le mardi 28 avril 2026 à 11:00 +0200, Sven Püschel a écrit :
> > > Split the flip and rotate command configuration into a separate
> > > function in preparation of filling the command stream at streamon.
> > > As the userspace can change the flipping and rotation controls while
> > > streaming, we have to update them with each new frame to prevent the
> > > user being unable to change them while streaming.
> > > 
> > > Signed-off-by: Sven Püschel <s.pueschel@pengutronix.de>
> > For code point of view, everything seems fine, but the commit message leave me a
> > bit wondering. Any rotation that isn't 180 degree will cause the width and
> > height to be reversed, and a new stride is needed to present the buffer
> > correctly. Meaning the capture format can be affected by this change.
> 
> Sorry for missing to properly communicate my intention with this patch.
> 
> I've stumbled over the RGA pulling a spin lock on the controls, when 
> starting the next job [1]. This made me realize that the driver allows 
> changing the controls while streaming, which my change to move the 
> command buffer setup to streamon breaks (as a potential rotation isn't 
> updated until the next streamon).
> 
> This commit is the result of trying to not break the old behavior by 
> moving the relevant code parts to be run on every frame instead of being 
> only run at streamon. I didn't think about the 90 degree rotation 
> problems, which are also present in the current RGA state.
> 
> sashiko.dev also pointed out that my simple code move didn't work for 
> the mirroring case [2], as the relevant command buffer is ore'd with the 
> mirroring flags. Also I've noticed that the rotation mode affects the 
> scaling factor. To avoid these footguns, I've got the idea to set a flag 
> to raise when the controls change. Then I can fully re-initialize the 
> command buffer on the next frame and fully avoid these kind of problems.

Thanks for the update, thanks for looking into that.

> 
> 
> [1] 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/platform/rockchip/rga/rga.c?id=50897c955902c93ae71c38698abb910525ebdc89#n41
> 
> [2] 
> https://sashiko.dev/#/patchset/20260428-spu-rga3-v5-0-eb7f5d019d86%40pengutronix.de?part=16
> 
> > 
> > To stick with the spec, the capture format needs to be updated, and it needs to
> > happen in a way user can be able to read it back for the correct frame if
> > userspace make use of the queues. I see 3 options, let me know what you think,
> > or what is later implemented if you already thought about that.
> > 
> > 1. Synchronously update the capture format width/height, document in the
> > respective control this behaviour, leaving to userspace to remember which frames
> > the change will apply to.
> > 
> > This works nicely for this type of HW, but would be a bit complicated for a
> > deinterlacer, since the buffering might be HW specific. It also make usage of
> > queues harder, less independent.
> > 
> > 2. Force a drain/stop/start for any 90 degree rotation
> > 
> > This might impose a longer idle time for the converter core, and is kind of
> > opposite of your commit message. But requires no spec work.
> > 
> > 3. Emit SRC_CH, implement the drain procedure typical to decoder resolution
> > change.
> > 
> > Typically it means userspace can keep buffering on the OUTPUT queue, and once
> > the LAST buffer is met, it can simply read the new format (and new stride, since
> > due to alignment, this might be hardware specific) and toggle streamoff/on only
> > on capture queue to reactivate the processing.
> > 
> > The 3. is more complex for the driver, but its a proven race-free method for
> > decoders already. 2 would be statusquo to get this series in, and we could post-
> > poned more advance work for seamless 90degree rorations. 1., I don't really like
> > that solution, it not quite generic enough.
> 
> I'll go with option 2 for now to keep it simple and improve the status 
> quo a bit.

Works for me.

Nicolas

> 
> Sincerely
>      Sven
> 
> > feedback welcome,
> > Nicolas
> > 
> > > ---
> > >   drivers/media/platform/rockchip/rga/rga-hw.c | 57 +++++++++++++++++-----------
> > >   1 file changed, 34 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/rockchip/rga/rga-hw.c b/drivers/media/platform/rockchip/rga/rga-hw.c
> > > index dac3cb6aa17d3..6c1956b04f6ba 100644
> > > --- a/drivers/media/platform/rockchip/rga/rga-hw.c
> > > +++ b/drivers/media/platform/rockchip/rga/rga-hw.c
> > > @@ -156,7 +156,38 @@ static void rga_cmd_set_dst_addr(struct rga_ctx *ctx, dma_addr_t dma_addr)
> > >   	dest[reg >> 2] |= 0x7 << 8;
> > >   }
> > >   
> > > -static void rga_cmd_set_trans_info(struct rga_ctx *ctx)
> > > +static void rga_cmd_set_flip_rotate_info(struct rga_ctx *ctx)
> > > +{
> > > +	u32 *dest = ctx->cmdbuf_virt;
> > > +	union rga_src_info src_info;
> > > +
> > > +	src_info.val = dest[(RGA_SRC_INFO - RGA_MODE_BASE_REG) >> 2];
> > > +
> > > +	if (ctx->vflip)
> > > +		src_info.data.mir_mode |= RGA_SRC_MIRR_MODE_X;
> > > +
> > > +	if (ctx->hflip)
> > > +		src_info.data.mir_mode |= RGA_SRC_MIRR_MODE_Y;
> > > +
> > > +	switch (ctx->rotate) {
> > > +	case 90:
> > > +		src_info.data.rot_mode = RGA_SRC_ROT_MODE_90_DEGREE;
> > > +		break;
> > > +	case 180:
> > > +		src_info.data.rot_mode = RGA_SRC_ROT_MODE_180_DEGREE;
> > > +		break;
> > > +	case 270:
> > > +		src_info.data.rot_mode = RGA_SRC_ROT_MODE_270_DEGREE;
> > > +		break;
> > > +	default:
> > > +		src_info.data.rot_mode = RGA_SRC_ROT_MODE_0_DEGREE;
> > > +		break;
> > > +	}
> > > +
> > > +	dest[(RGA_SRC_INFO - RGA_MODE_BASE_REG) >> 2] = src_info.val;
> > > +}
> > > +
> > > +static void rga_cmd_set_format_scale_info(struct rga_ctx *ctx)
> > >   {
> > >   	struct rockchip_rga *rga = ctx->rga;
> > >   	u32 *dest = ctx->cmdbuf_virt;
> > > @@ -219,27 +250,6 @@ static void rga_cmd_set_trans_info(struct rga_ctx *ctx)
> > >   		}
> > >   	}
> > >   
> > > -	if (ctx->vflip)
> > > -		src_info.data.mir_mode |= RGA_SRC_MIRR_MODE_X;
> > > -
> > > -	if (ctx->hflip)
> > > -		src_info.data.mir_mode |= RGA_SRC_MIRR_MODE_Y;
> > > -
> > > -	switch (ctx->rotate) {
> > > -	case 90:
> > > -		src_info.data.rot_mode = RGA_SRC_ROT_MODE_90_DEGREE;
> > > -		break;
> > > -	case 180:
> > > -		src_info.data.rot_mode = RGA_SRC_ROT_MODE_180_DEGREE;
> > > -		break;
> > > -	case 270:
> > > -		src_info.data.rot_mode = RGA_SRC_ROT_MODE_270_DEGREE;
> > > -		break;
> > > -	default:
> > > -		src_info.data.rot_mode = RGA_SRC_ROT_MODE_0_DEGREE;
> > > -		break;
> > > -	}
> > > -
> > >   	/*
> > >   	 * Calculate the up/down scaling mode/factor.
> > >   	 *
> > > @@ -431,7 +441,8 @@ static void rga_cmd_set(struct rga_ctx *ctx,
> > >   
> > >   	rga_cmd_set_src_info(ctx, &src->offset);
> > >   	rga_cmd_set_dst_info(ctx, &dst->offset);
> > > -	rga_cmd_set_trans_info(ctx);
> > > +	rga_cmd_set_format_scale_info(ctx);
> > > +	rga_cmd_set_flip_rotate_info(ctx);
> > >   
> > >   	rga_write(rga, RGA_CMD_BASE, ctx->cmdbuf_phy);
> > >   

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