Re: [PATCH 16/16] media: rockchip: rga: add rga3 support

[Nicolas Dufresne <nicolas@ndufresne.ca>]

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

Hi,

Le mercredi 03 décembre 2025 à 10:49 +0100, Sven Püschel a écrit :
> Hi Nicolas,
> 
> On 10/7/25 9:41 PM, Nicolas Dufresne wrote:
> > Hi,
> > 
> > Le mardi 07 octobre 2025 à 10:32 +0200, Sven Püschel a écrit :
> > 
> > > +}
> > > +
> > > +static void rga3_cmd_enable_win0(struct rga_ctx *ctx)
> > > +{
> > > +	u32 *cmd = ctx->rga->cmdbuf_virt;
> > > +	unsigned int reg;
> > > +
> > > +	reg = RGA3_WIN0_RD_CTRL - RGA3_FIRST_CMD_REG;
> > > +	cmd[reg >> 2] |= FIELD_PREP(RGA3_WIN_ENABLE, 1);
> > > +}
> > > +
> > > +static void rga3_cmd_set_wr_format(struct rga_ctx *ctx)
> > > +{
> > > +	u32 *cmd = ctx->rga->cmdbuf_virt;
> > > +	const struct rga3_fmt *out = ctx->out.fmt;
> > > +	u8 wr_format;
> > > +	unsigned int reg;
> > > +
> > > +	if (out->semi_planar)
> > > +		wr_format = RGA3_RDWR_FORMAT_SEMI_PLANAR;
> > > +	else
> > > +		wr_format = RGA3_RDWR_FORMAT_INTERLEAVED;
> > > +
> > > +	reg = RGA3_WR_CTRL - RGA3_FIRST_CMD_REG;
> > > +	cmd[reg >> 2] |= FIELD_PREP(RGA3_WR_PIC_FORMAT, out->hw_format)
> > > +		      |  FIELD_PREP(RGA3_WR_YC_SWAP, out->yc_swap)
> > > +		      |  FIELD_PREP(RGA3_WR_RBUV_SWAP, out->rbuv_swap)
> > > +		      |  FIELD_PREP(RGA3_WR_FORMAT, wr_format);
> > > +}
> > > +
> > > +static void rga3_cmd_disable_wr_limitation(struct rga_ctx *ctx)
> > > +{
> > > +	u32 *cmd = ctx->rga->cmdbuf_virt;
> > > +	unsigned int reg;
> > > +
> > > +	/* Use the max value to avoid limiting the write speed */
> > > +	reg = RGA3_WR_CTRL - RGA3_FIRST_CMD_REG;
> > > +	cmd[reg >> 2] |= FIELD_PREP(RGA3_WR_SW_OUTSTANDING_MAX, 63);
> > No issue with the code, but quite an interesting feature. We did discussed in
> > pas about using the target framerate (well frame duration in v4l2) to avoid
> > bursting the memory.
> 
> Do you have any links to these discussions or some actual implementation?

That likely happened during a media summit. Philippe Zabel might remember more.
I know for stateful encoders, you can already have two timing information, but
its all vague in my memory.

> 
>  From the API perspective I didn't really find the desired API to 
> support this feature. While VIDIOC_S_PARM would allow setting an frame 
> interval, the documentation doesn't really encompass this use case (as 
> we won't configure the RGA3 depending on this nor drop frames if the 
> RGA3 is too slow).
> 
> Also limiting the potential burst size only seems practical, when it 
> causes latency issues. Otherwise it should be more efficient to have big 
> bursts (bandwidth and potentially even power consumption wise).

I have no issue with letting it to its maximum. The comment here simply
highlights it, but pretty much all other drivers will hardcore the maximum
working size and move on. In a second thought, if you multi-plex the RGA3 a lot,
you don't want any slower use of it, since it may hang up on the core for
longer. So I'd say, unless we have problems, this is fine as it is.

> 
> Sincerely
>      Sven

[...]

cheers,
Nicolas

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