[PATCH v5 16/29] media: rockchip: rga: split flip and rotate into separate function
Nicolas Dufresne
nicolas at ndufresne.ca
Tue May 12 07:15:30 PDT 2026
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 at 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);
> > >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-rockchip/attachments/20260512/1e60a6c4/attachment.sig>
More information about the Linux-rockchip
mailing list