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

[Nicolas Dufresne]

Le vendredi 05 avril 2024 à 16:21 +0200, Link Mauve a écrit :
> 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.

As the behaviour we want is that these cores becomes a group and get schedule
together, its certainly a good time to slow down and evaluate if that part needs
to be improve in the DT too.

Hantro G1/H1 and VEPU/VDPU121 combos originally shared the same sram region Its
not clear if any of these cores have this limitation and if this should be
expressed in the DT / driver.

> 
> > 
> > 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.

Might be on the gray side, but there is good chances software written for your
specific board can stop working after te grouping is done.

> 
> > 
> > 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?

I don't know for JPEG, the MTK vcoder do support cascading cores. This is
different from concurrent cores. In MTK architecture, for some of the codec,
there is LAT (entropy decoder) and CORE (the reconstruction block) that are
split.

Nicolas