Re: [yocto-patches] [meta-rockchip PATCH 3/9] rk3399: enable gstreamer v4l2codecs support

[Trevor Woerner <twoerner@gmail.com>]

On Mon 2024-08-26 @ 05:35:14 PM, Quentin Schulz via lists.yoctoproject.org wrote:
> Hi Trevor,
> 
> On 8/23/24 3:55 PM, Trevor Woerner via lists.yoctoproject.org wrote:
> > On Thu 2024-08-22 @ 02:38:11 PM, Quentin Schulz via lists.yoctoproject.org wrote:
> > > Hi Trevor,
> > > 
> > > On 8/22/24 2:23 PM, Trevor Woerner via lists.yoctoproject.org wrote:
> > > > On Wed 2024-08-21 @ 10:17:52 AM, Quentin Schulz via lists.yoctoproject.org wrote:
> > > > > Hi Trevor,
> > > > > 
> > > > > On 8/21/24 6:42 AM, Trevor Woerner via lists.yoctoproject.org wrote:
> > > > > > On Tue 2024-08-20 @ 03:48:16 PM, Quentin Schulz via lists.yoctoproject.org wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > On 8/20/24 2:56 PM, Quentin Schulz wrote:
> > > > > > > > From: Quentin Schulz <quentin.schulz@cherry.de>
> > > > > > > > 
> > > > > > > > RK3399 has a Hantro VPU, so let's set HAS_HANTRO so that gstreamer
> > > > > > > > v4l2codecs plugin can be built.
> > > > > > > > 
> > > > > > > 
> > > > > > > RK3399 actually also has an RKVDEC VPU which is used for decoding H.264, VP9
> > > > > > > and H.265. Currently, rkvdec supports H.264 and VP9 decoding, while Hantro
> > > > > > > supports VP8 and MPEG2 decoding as well as JPEG encoding. Therefore, I'm not
> > > > > > > sure the naming of the variable is proper? Should we go for
> > > > > > > HAS_STATELESS_VPU instead?
> > > > > > 
> > > > > > What about 2 knobs:
> > > > > > 	1. HAS_HANTRO
> > > > > > 	2. HAS_RKVDEC
> > > > > > ?
> > > > > > 
> > > > > > Does gstreamer have knobs for both sets?
> > > > > > 
> > > > > 
> > > > > They are both stateless VPUs, supported by the same kernel API I believe. So
> > > > > the same meson option is used, v4l2codecs, so I don't think we need to have
> > > > > two separate knobs for those? At least, I used the same gstreamer plugin for
> > > > > decoding h264 on PX30 and RK3399 and PX30 uses Hantro while RK3399 uses
> > > > > RKVDEC for that. Similarly, RK3562, RK356x and RK3588(s) all seems to have
> > > > > an RKVDECv2, which likely is also stateless?
> > > > 
> > > > Is this something the user will definitely always want on (i.e. it won't
> > > > work without it) or should we allow the user the choose whether they want it
> > > > enabled or not?
> > > > 
> > > 
> > > Considering that the only other tool I'm aware of for decoding is ffmpeg and
> > > it does not currently support stateless VPUs
> > > (https://ffmpeg.org/pipermail/ffmpeg-devel/2024-August/332034.html), if one
> > > wants to do decoding on upstream kernel on Rockchip boards, they'll need the
> > > v4l2codecs PACKAGECONFIG option for gst-plugins-bad recipe selected.
> > > 
> > > They could also disable it for their board if they want to by redefining
> > > HAS_HANTRO variable in their own config file? Or in a distro, etc.
> > 
> > I just wanted to make sure the = in the patches wasn't going to prevent them
> > from doing that.
> > 
> 
> Well, it does. So thanks for bringing this up :)
> 
> > > 
> > > > Personally I would rather see one patch to add this one feature, rather than
> > > > 8.
> > > > 
> > > 
> > > Those were separate patches for multiple reasons:
> > > - I only tested on 2 vanilla upstream kernel (RK3399, PX30)
> > > - I only tested on 1 downstream upstream-based kernel (RK3588)
> > > - the rest is untested (not even built)
> > > - All patches except the one for RK3588(s) could be backported to other
> > > branches (e.g. scarthgap); this turned out to be incorrect because the
> > > :rockchip OVERRIDES isn't available in that branch (but we can fix this :)
> > > ).
> > > 
> > > I wanted to provide also the context for tests I've performed on those
> > > boards in the commit log. It could be quite long if I put all tests for all
> > > SoC in the same commit log (but that's fine with me).
> > 
> > Have you seen the size of some of my commit messages? ;-) I don't mind a large
> > commit message. I've build-tested your patchset against all rockchip MACHINEs
> > and it builds fine. When it comes time to backport, the patch can be adjusted
> > as necessary.
> > 
> 
> ACK, will do.
> 
> > > > Should we add a note in the README about this, or perhaps the gstreamer
> > > > bbappend?
> > > > 
> > > 
> > > What exactly would you like to see there?
> > 
> > Nobody starts by reading the README, but if a user is browsing the MACHINE
> > files, notices the HANTRO line and is looking for a quick understanding, they
> > might reach for the README file then? You know that HANTRO refers to something
> > to do with video decoding, does everybody? Even if all you do is duplicate the
> > commit message from 1/9 that would be great.
> > 
> 
> Fair point, the hardest part is now to come up with a proper name for the
> variable that is not misleading.
> 
> Something like:
> 
> UPSTREAM_STATELESS_VPU ?= "1"
> 
> maybe?

If one reads the text that scrolls past while booting, the word Hantro does
show up on devices that have it. Could the variable include the string
"hantro" in it somewhere?

> 
> I would still add a comment in the README as requested.
> 
> Cheers,
> Quentin
> 
> 
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#563): https://lists.yoctoproject.org/g/yocto-patches/message/563
> Mute This Topic: https://lists.yoctoproject.org/mt/107999800/900817
> Group Owner: yocto-patches+owner@lists.yoctoproject.org
> Unsubscribe: https://lists.yoctoproject.org/g/yocto-patches/leave/13168745/900817/63955952/xyzzy [twoerner@gmail.com]
> -=-=-=-=-=-=-=-=-=-=-=-
> 
>