Re: [PATCH v2 0/2] Enable JPEG encoding on rk3588

[Link Mauve <linkmauve@linkmauve.fr>]

On Thu, Apr 04, 2024 at 01:41:15PM -0400, Nicolas Dufresne wrote:
> Hi,

Hi,

> 
> Le mercredi 27 mars 2024 à 14:41 +0100, Emmanuel Gil Peyrot a écrit :
> > Only the JPEG encoder is available for now, although there are patches
> > for the undocumented VP8 encoder floating around[0].
> 
> [0] seems like a broken link. The VP8 encoder RFC is for RK3399 (and Hantro H1
> posted by ST more recently). The TRM says "VEPU121(JPEG encoder only)", which
> suggest that the H.264 and VP8 encoders usually found on the VEPU121 are
> removed. As Rockchip have remove the synthesize register while modifying the H1
> IP, it is difficult to verify. Confusingly the H.264 specific registers are
> documented in the TRM around VEPU121.

Ah, the link became, and was indeed ST’s series:
https://patchwork.kernel.org/project/linux-rockchip/list/?series=789885&archive=both

But the TRM part 1 says the VEPU121 supports H.264 encoding (page 367),
and it’s likely they didn’t remove just VP8 support since the codec
features are pretty close to H.264’s.

> 
> > 
> > This has been tested on a rock-5b, resulting in four /dev/video*
> > encoders.  The userspace program I’ve been using to test them is
> > Onix[1], using the jpeg-encoder example, it will pick one of these four
> > at random (but displays the one it picked):
> > % ffmpeg -i <input image> -pix_fmt yuvj420p temp.yuv
> > % jpeg-encoder temp.yuv <width> <height> NV12 <quality> output.jpeg
> 
> I don't like that we exposing each identical cores a separate video nodes. I
> think we should aim for 1 device, and then multi-plex and schedule de cores from
> inside the Linux kernel.

I agree, but this should be handled in the driver not in the device
tree, and it can be done later.

> 
> Not doing this now means we'll never have an optimal hardware usage
> distribution. Just consider two userspace software wanting to do jpeg encoding.
> If they both take a guess, they may endup using a single core. Where with proper
> scheduling in V4L2, the kernel will be able to properly distribute the load. I
> insist on this, since if we merge you changes it becomes an ABI and we can't
> change it anymore.

Will it really become ABI just like that?  Userspace should always
discover the video nodes and their capabilities and not hardcode e.g. a
specific /dev/videoN file for a specific codec.  I would argue that this
series would let userspace do JPEG encoding right away, even if in a
less optimal way than if the driver would round-robin them through a
single video node, but that can always be added in a future version.

> 
> I understand that this impose a rework of the mem2mem framework so that we can
> run multiple jobs, but this will be needed anyway on RK3588, since the rkvdec2,
> which we don't have a driver yet is also multi-core, but you need to use 2 cores
> when the resolution is close to 8K.

I think the mediatek JPEG driver already supports that, would it be ok
to do it the same way?

> 
> Nicolas
> 
> > 
> > [0] https://patchwork.kernel.org/project/linux-rockchip/list/?series=789885
> > [1] https://crates.io/crates/onix
> > 
> > Changes since v1:
> > - Dropped patches 1 and 4.
> > - Use the proper compatible form, since this device should be fully
> >   compatible with the VEPU of rk356x.
> > - Describe where the VEPU121 name comes from, and list other encoders
> >   and decoders present in this SoC.
> > - Properly test the device tree changes, I previously couldn’t since I
> >   was using a too recent version of python-jsonschema…
> > 
> > Emmanuel Gil Peyrot (2):
> >   media: dt-binding: media: Document rk3588’s VEPU121
> >   arm64: dts: rockchip: Add VEPU121 to rk3588
> > 
> >  .../bindings/media/rockchip,rk3568-vepu.yaml  |  8 +-
> >  arch/arm64/boot/dts/rockchip/rk3588s.dtsi     | 80 +++++++++++++++++++
> >  2 files changed, 86 insertions(+), 2 deletions(-)
> > 
> 

-- 
Link Mauve