Message-ID: <c43bd8cf385cc4c90e549ae28174b3d406fae1ce.camel@collabora.com>
Date:   Mon, 18 Jul 2022 10:54:25 -0400
From:   Nicolas Dufresne <nicolas.dufresne@...labora.com>
To:     Jernej Škrabec <jernej.skrabec@...il.com>,
        Robin Murphy <robin.murphy@....com>,
        Sebastian Fricke <sebastian.fricke@...labora.com>,
        linux-media@...r.kernel.org
Cc:     knaerzche@...il.com, kernel@...labora.com,
        bob.beckett@...labora.com, ezequiel@...guardiasur.com.ar,
        mchehab@...nel.org, gregkh@...uxfoundation.org,
        linux-kernel@...r.kernel.org, linux-rockchip@...ts.infradead.org,
        linux-staging@...ts.linux.dev, Yury Norov <yury.norov@...il.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>
Subject: Re: [PATCH 0/6] RkVDEC HEVC driver

Le samedi 16 juillet 2022 à 08:45 +0200, Jernej Škrabec a écrit :
> Dne petek, 15. julij 2022 ob 17:36:01 CEST je Nicolas Dufresne napisal(a):
> > Le vendredi 15 juillet 2022 à 12:04 +0100, Robin Murphy a écrit :
> > > On 2022-07-13 17:24, Sebastian Fricke wrote:
> > > > Implement the HEVC codec variation for the RkVDEC driver. Currently only
> > > > the RK3399 is supported, but it is possible to enable the RK3288 as it
> > > > also supports this codec.
> > > > 
> > > > Based on top of the media tree @ef7fcbbb9eabbe86d2287484bf366dd1821cc6b8
> > > > and the HEVC uABI MR by Benjamin Gaignard.
> > > > (https://patchwork.linuxtv.org/project/linux-media/list/?series=8360)
> > > > 
> > > > Tested with the GStreamer V4L2 HEVC plugin:
> > > > (https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/107
> > > > 9)
> > > > 
> > > > Current Fluster score:
> > > > `Ran 131/147 tests successfully               in 278.568 secs`
> > > > with
> > > > `python3 fluster.py run -d GStreamer-H.265-V4L2SL-Gst1.0 -ts
> > > > JCT-VC-HEVC_V1 -j1`
> > > > 
> > > > failed conformance tests:
> > > > - DBLK_D_VIXS_2 (Success on Hantro G2)
> > > > - DSLICE_A_HHI_5 (Success on Hantro G2)
> > > > - EXT_A_ericsson_4 (Success on Hantro G2)
> > > > - PICSIZE_A_Bossen_1 (Hardware limitation)
> > > > - PICSIZE_B_Bossen_1 (Hardware limitation)
> > > > - PICSIZE_C_Bossen_1 (Hardware limitation)
> > > > - PICSIZE_D_Bossen_1 (Hardware limitation)
> > > > - PPS_A_qualcomm_7 (Success on Hantro G2)
> > > > - SAODBLK_A_MainConcept_4 (Success on Hantro G2)
> > > > - SAODBLK_B_MainConcept_4 (Success on Hantro G2)
> > > > - SLIST_B_Sony_9 (Success on Hantro G2)
> > > > - SLIST_D_Sony_9 (Success on Hantro G2)
> > > > - TSUNEQBD_A_MAIN10_Technicolor_2 (Success on Hantro G2)
> > > > - VPSSPSPPS_A_MainConcept_1 (Success on Hantro G2)
> > > > - WPP_D_ericsson_MAIN10_2 (Fail on Hantro G2)
> > > > - WPP_D_ericsson_MAIN_2 (Fail on Hantro G2)
> > > > 
> > > > Not tested with FFMpeg so far.
> > > > 
> > > > Known issues:
> > > > - Unable to reliably decode multiple videos concurrently
> > > > - The SAODBLK_* tests timeout if the timeout time in fluster is lower
> > > > than 120 - Currently the uv_virstride is calculated in a manner that is
> > > > hardcoded for the two available formats NV12 and NV15.
> > > > (@config_registers)
> > > > 
> > > > Notable design decisions:
> > > > - I opted for a bitfield to represent the PPS memory blob as it is the
> > > > perfect tool for that job. It describes the memory layout with any
> > > > additional required documentation, is easy to read and a native language
> > > > tool for that job
> > > 
> > > Can I point out how terrible an idea this is? The C language gives
> > > virtually zero guarantee about how bitfields are actually represented in
> > > memory. Platform ABIs (e.g. [1]) might nail things down a bit more, but
> > > different platforms are free to make completely different choices so
> > > portability still goes out the window. Even for a single platform,
> > > different compilers (or at worst even different version of one compiler)
> > > can still make incompatible choices e.g. WRT alignment of packed
> > > members. Even if you narrow the scope as far as a specific version of
> > > AArch64 GCC, I think this is still totally broken for big-endian.
> > > 
> > > The fact that you've had to use nonsensical types to trick a compiler
> > > into meeting your expectations should already be a clue to how fragile
> > > this is in general.
> > > 
> > > > - The RPS memory blob is created using a bitmap implementation, which
> > > > uses a common Kernel API to avoid reinventing the wheel and to keep the
> > > > code clean.
> > > 
> > > Similarly, Linux bitmaps are designed for use as, well, bitmaps. Abusing
> > > them as a data interchange format for bit-aligned numerical values is
> > > far from "clean" semantically. And I'm pretty sure it's also broken for
> > > big-endian.
> > > 
> > > This kind of stuff may be standard practice in embedded development
> > > where you're targeting a specific MCU with a specific toolchain, but I
> > > don't believe it's suitable for upstream Linux. It would take pretty
> > > much the same number of lines to use GENMASK definitions and bitfield.h
> > > helpers to pack values into words which can then be written to memory in
> > > a guaranteed format and endianness (certainly for the PPS; for the RPS
> > > it may well end up a bit longer, but would be self-documenting and
> > > certainly more readable than those loops). It mostly just means that for
> > > any field which crosses a word boundary you'll end up with 2 definitions
> > > and 2 assignments, which is hardly a problem (and in some ways more
> > > honest about what's actually going on).
> > 
> > Thanks for the feedback, in multimedia (unlike register programming), we
> > don't really consider bitstreams as bitmap or bitfield. What we do really
> > expect is to use bit writer helpers (and sometimes a bit reader though we
> > try and avoid the second one in the  kernel). Its more of less a cursor (a
> > bit position) into a memory that advance while writing. A bit writer should
> > help protect against overflow too.
> > 
> > When writing lets say a chain of 8 bits from a char, a proper helper is
> > expected to be very explicit on the ordering (write_u8_le/be or something
> > better worded). I would rather like to see all these blobs written this way
> > personally then having a cleared buffer and writing using bit offsets.
> > 
> > Perhaps I may suggest to start with implementing just that inside this
> > driver? It isn't very hard, and then the implementation can be reduced
> > later and shared later, with whatever exists without deviating from the
> > intent of the existing API ? I do believe that having this in linux-media
> > can be useful in the future. We will notably need to extend such a helper
> > with multimedia specific coding technique (golomb, boolean coding, etc.)
> > for use in stateless encoder drivers.
> 
> I don't know RKVDEC, but at least Cedar has integrated bitstream parsing 
> engine. Is there something similar in RKVDEC? That way HW could be used 
> instead of SW implementation.

This is unrelated, since the code here generates a bitstream. Some of the
parameters you'd pass with registers with other drivers, are passed with memory
chunk in rkvdec. Not all these blob have a byte aligned memory layout, they are
instead bitstream without any consideration for byte alignment. So we need a
tool to create such a bitstream. Similar tool will be needed for adapting
encoders.

> 
> Best regards,
> Jernej
> 
> > 
> > Nicolas
> > 
> > > Thanks,
> > > Robin.
> > > 
> > > [1]
> > > https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst#bit-f
> > > ields> 
> > > > - I deliberatly opted against the macro solution used in H264, which
> > > > declares Macros in mid function and declares the fields of the memory
> > > > blob as macros as well. And I would be glad to refactor the H264 code if
> > > > desired by the maintainer to use common Kernel APIs and native language
> > > > elements.
> > > > - The giant static array of cabac values is moved to a separate c file,
> > > > I did so because a separate .h file would be incorrect as it doesn't
> > > > expose anything of any value for any other file than the rkvdec-hevc.c
> > > > 
> > > > file. Other options were:
> > > >    - Calculating the values instead of storing the results (doesn't seem
> > > >    to be worth it)
> > > >    - Supply them via firmware (Adding firmware makes the whole software
> > > >    way more complicated and the usage of the driver less obvious)
> > > > 
> > > > Ignored Checkpatch warnings (as it fits to the current style of the
> > > > file):
> > > > ```
> > > > WARNING: line length of 162 exceeds 100 columns
> > > > #115: FILE: drivers/media/v4l2-core/v4l2-common.c:265:
> > > > +               { .format = V4L2_PIX_FMT_NV15,    .pixel_enc =
> > > > V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 5,
> > > > 0, 0 }, .hdiv = 2, .vdiv = 2,
> > > > 
> > > > ERROR: trailing statements should be on next line
> > > > #128: FILE: drivers/media/v4l2-core/v4l2-ioctl.c:1305:
> > > > +       case V4L2_PIX_FMT_NV15:         descr = "10-bit Y/CbCr 4:2:0
> > > > (Packed)"; break; ```
> > > > 
> > > > v4l2-compliance test:
> > > > ```
> > > > Total for rkvdec device /dev/video3: 46, Succeeded: 46, Failed: 0,
> > > > Warnings: 0 ```
> > > > 
> > > > kselftest module run for the bitmap changes:
> > > > ```
> > > > $ sudo insmod
> > > > /usr/lib/modules/5.19.0-rc3-finalseries/kernel/lib/test_bitmap.ko [  
> > > > 71.751716] test_bitmap: parselist: 14: input is '0-2047:128/256' OK,
> > > > Time: 1750 [   71.751787] test_bitmap: bitmap_print_to_pagebuf: input
> > > > is '0-32767 [   71.751787] ', Time: 6708
> > > > [   71.760373] test_bitmap: set_value: 6/6 tests correct
> > > > ```
> > > > 
> > > > Jonas Karlman (2):
> > > >    media: v4l2: Add NV15 pixel format
> > > >    media: v4l2-common: Add helpers to calculate bytesperline and
> > > >    
> > > >      sizeimage
> > > > 
> > > > Sebastian Fricke (4):
> > > >    bitops: bitmap helper to set variable length values
> > > >    staging: media: rkvdec: Add valid pixel format check
> > > >    staging: media: rkvdec: Enable S_CTRL IOCTL
> > > >    staging: media: rkvdec: Add HEVC backend
> > > >   
> > > >   .../media/v4l/pixfmt-yuv-planar.rst           |   53 +
> > > >   drivers/media/v4l2-core/v4l2-common.c         |   79 +-
> > > >   drivers/media/v4l2-core/v4l2-ioctl.c          |    1 +
> > > >   drivers/staging/media/rkvdec/Makefile         |    2 +-
> > > >   drivers/staging/media/rkvdec/TODO             |   22 +-
> > > >   .../staging/media/rkvdec/rkvdec-hevc-data.c   | 1844 +++++++++++++++++
> > > >   drivers/staging/media/rkvdec/rkvdec-hevc.c    |  859 ++++++++
> > > >   drivers/staging/media/rkvdec/rkvdec-regs.h    |    1 +
> > > >   drivers/staging/media/rkvdec/rkvdec.c         |  182 +-
> > > >   drivers/staging/media/rkvdec/rkvdec.h         |    3 +
> > > >   include/linux/bitmap.h                        |   39 +
> > > >   include/uapi/linux/videodev2.h                |    1 +
> > > >   lib/test_bitmap.c                             |   47 +
> > > >   13 files changed, 3066 insertions(+), 67 deletions(-)
> > > >   create mode 100644 drivers/staging/media/rkvdec/rkvdec-hevc-data.c
> > > >   create mode 100644 drivers/staging/media/rkvdec/rkvdec-hevc.c
> 
> 
> 
> 

Powered by blists - more mailing lists