| Message ID | 20240820-gst-hantro-v1-3-335c4eaf8e8b@cherry.de |
|---|---|
| State | New |
| Headers | show |
| Series | enable v4l2codecs gstreamer plugin for VPU decoding | expand |
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? Cheers, Quentin
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? > Cheers, > Quentin > > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#535): https://lists.yoctoproject.org/g/yocto-patches/message/535 > 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] > -=-=-=-=-=-=-=-=-=-=-=- > >
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? Cheers, Quentin
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? Personally I would rather see one patch to add this one feature, rather than 8. Should we add a note in the README about this, or perhaps the gstreamer bbappend? > > Cheers, > Quentin > > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#538): https://lists.yoctoproject.org/g/yocto-patches/message/538 > 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] > -=-=-=-=-=-=-=-=-=-=-=- > >
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. > 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). > Should we add a note in the README about this, or perhaps the gstreamer > bbappend? > What exactly would you like to see there? Cheers, Quentin
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. > > > 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. > > 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. > > Cheers, > Quentin > > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#550): https://lists.yoctoproject.org/g/yocto-patches/message/550 > 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] > -=-=-=-=-=-=-=-=-=-=-=- > >
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? I would still add a comment in the README as requested. Cheers, Quentin
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] > -=-=-=-=-=-=-=-=-=-=-=- > >
Hi Trevor, On 8/27/24 5:10 PM, Trevor Woerner via lists.yoctoproject.org wrote: > 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? > That;s the thing... not all Rockchip stateless VPUs are called Hantro :) Some are called rkvdec, some rkvdec2, some hantro. Some SoCs have multiple of those :) It;s a wonderful mess :) Cheers, Quentin
diff --git a/conf/machine/include/rk3399.inc b/conf/machine/include/rk3399.inc index 47f0560..8650d0a 100644 --- a/conf/machine/include/rk3399.inc +++ b/conf/machine/include/rk3399.inc @@ -20,3 +20,5 @@ TFA_BUILD_TARGET = "bl31" UBOOT_SUFFIX ?= "itb" UBOOT_ENTRYPOINT ?= "0x06000000" + +HAS_HANTRO = "1"