Message-ID: <f05896551f8545af3c7352a6bd38248e038b61d2.camel@collabora.com>
Date:   Fri, 15 Jul 2022 11:36:01 -0400
From:   Nicolas Dufresne <nicolas.dufresne@...labora.com>
To:     Robin Murphy <robin.murphy@....com>,
        Sebastian Fricke <sebastian.fricke@...labora.com>,
        linux-media@...r.kernel.org
Cc:     jernej.skrabec@...il.com, 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 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/1079)
> > 
> > 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.

Nicolas

> 
> Thanks,
> Robin.
> 
> [1] 
> https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst#bit-fields
> 
> > - 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