Re: [PATCH v2 1/7] media: rkvdec: Add HEVC backend

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 4612 bytes --]

Le lundi 11 août 2025 à 21:46 +0200, Jonas Karlman a écrit :
> Hi Nicolas,
> 
> On 8/11/2025 9:12 PM, Nicolas Dufresne wrote:
> > Le dimanche 10 août 2025 à 21:24 +0000, Jonas Karlman a écrit :
> > > The Rockchip VDEC supports the HEVC codec with the Main and Main10
> > > Profile up to Level 5.1 High tier: 4096x2304@60 fps.
> > > 
> > > Add the backend for HEVC format to the decoder.
> > > 
> > > Signed-off-by: Alex Bee <knaerzche@gmail.com>
> > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > > ---
> > > Changes in v2:
> > > - Use new_value in transpose_and_flatten_matrices()
> > > - Add NULL check for ctrl->new_elems in rkvdec_hevc_run_preamble()
> > > - Set RKVDEC_WR_DDR_ALIGN_EN for RK3328
> > > ---
> > >  .../media/platform/rockchip/rkvdec/Makefile   |    2 +-
> > >  .../rockchip/rkvdec/rkvdec-hevc-data.c        | 1848 +++++++++++++++++
> > >  .../platform/rockchip/rkvdec/rkvdec-hevc.c    |  817 ++++++++
> > >  .../platform/rockchip/rkvdec/rkvdec-regs.h    |    2 +
> > >  .../media/platform/rockchip/rkvdec/rkvdec.c   |   76 +
> > >  .../media/platform/rockchip/rkvdec/rkvdec.h   |    1 +
> > >  6 files changed, 2745 insertions(+), 1 deletion(-)
> > >  create mode 100644 drivers/media/platform/rockchip/rkvdec/rkvdec-hevc-
> > > data.c
> > >  create mode 100644 drivers/media/platform/rockchip/rkvdec/rkvdec-hevc.c
> > > 
> > > diff --git a/drivers/media/platform/rockchip/rkvdec/Makefile
> > > b/drivers/media/platform/rockchip/rkvdec/Makefile
> > > index cb86b429cfaa..a77122641d14 100644
> > > --- a/drivers/media/platform/rockchip/rkvdec/Makefile
> > > +++ b/drivers/media/platform/rockchip/rkvdec/Makefile
> > > @@ -1,3 +1,3 @@
> > >  obj-$(CONFIG_VIDEO_ROCKCHIP_VDEC) += rockchip-vdec.o
> > >  
> > > -rockchip-vdec-y += rkvdec.o rkvdec-h264.o rkvdec-vp9.o
> > > +rockchip-vdec-y += rkvdec.o rkvdec-h264.o rkvdec-hevc.o rkvdec-vp9.o
> > > diff --git a/drivers/media/platform/rockchip/rkvdec/rkvdec-hevc-data.c
> > > b/drivers/media/platform/rockchip/rkvdec/rkvdec-hevc-data.c
> > > new file mode 100644
> > > index 000000000000..eac4ea604949
> > > --- /dev/null
> > > +++ b/drivers/media/platform/rockchip/rkvdec/rkvdec-hevc-data.c
> > > @@ -0,0 +1,1848 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Rockchip Video Decoder driver
> > > + *
> > > + * Copyright (C) 2023 Collabora, Ltd.
> > > + *	Sebastian Fricke <sebastian.fricke@collabora.com>
> > > + */
> > > +
> > > +#include <linux/types.h>
> > > +
> > > +#define RKV_CABAC_TABLE_SIZE		27456
> > > +
> > > +/*
> > > + * This file is #include from rkvdec-hevc.c and not compiled.
> > > + */
> > > +static const u8 rkvdec_hevc_cabac_table[RKV_CABAC_TABLE_SIZE] = {
> > > +	0x07, 0x0f, 0x48, 0x58, 0x58, 0x40, 0x40, 0x40, 0x40, 0x40, 0x0f,
> > > 0x40, 0x40, 0x40, 0x0f,
> > 
> > Nit, in H.264 case, we managed to fill the CABAC based on the spect with
> > macros,
> > didn't we figure-out this one ? I didn't check in Detlev tree, I'm just
> > asking
> > here.
> 
> As hinted at in the cover letter: in the initial implementation used for
> LibreELEC I just shamelessly copied the cabac data 1:1 from the Rockchip
> mpp library, for this series it was replaced with the cabac table from
> Sebastian Fricke prior series to add a HEVC backend [1]. Sebastian
> mentioned following regarding the cabac table:
> 
> """
> Notable design decisions:
> - 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 (No clear pattern
>     found for the calculation using the static array and the formulas from the
>     specification)
>   - Supply them via firmware (Adding firmware makes the whole software
>     way more complicated and the usage of the driver less obvious)
> """
> 
> I have not explored any other way to handle the cabac table based on
> these design decisions.
> 
> [1]
> https://lore.kernel.org/linux-media/20230101-patch-series-v2-6-2-rc1-v2-0-fa1897efac14@collabora.com/

I see, I guess if we don't know how it was generated, there isn't much we can do
about it.

cheers,
Nicolas

> 
> Regards,
> Jonas
> 
> > 
> > Nicolas
> 
> [snip]

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]