| Message ID | 20230111165931.753763-1-benjamin.gaignard@collabora.com |
|---|---|
| Headers | show |
| Series | AV1 stateless decoder for RK3588 | expand |
Le 25/01/2023 à 10:54, Hans Verkuil a écrit : > On 1/11/23 17:59, Benjamin Gaignard wrote: >> This series implement AV1 stateless decoder for RK3588 SoC. >> The hardware support 8 and 10 bits bitstreams up to 7680x4320. >> AV1 feature like film grain or scaling are done by the postprocessor. >> The driver can produce NV12_4L4, NV12_10LE40_4L4, NV12 and P010 pixels formats. >> Even if Rockchip have named the hardware VPU981 it looks like a VC9000 but >> with a different registers mapping. >> >> It is based on Daniel's "[PATCH v4] media: Add AV1 uAPI" [1] patches. >> >> The full branch can be found here: >> https://gitlab.collabora.com/linux/for-upstream/-/commits/rk3588_av1_decoder_v3 >> >> Fluster score is: 200/239 while testing AV1-TEST-VECTORS with GStreamer-AV1-V4L2SL-Gst1.0. >> The failing tests are: >> - the 2 tests with 2 spatial layers: few errors in luma/chroma values >> - tests with resolution < hardware limit (64x64) >> - 10bits film grain test: bad macroblocks while decoding, the same 8bits >> test is working fine. >> >> Changes in v3: >> - Fix arrays loops limites. >> - Remove unused field. >> - Reset raw pixel formats list when bit depth or film grain feature >> values change. >> - Enable post-processor P010 support >> >> Changes in v2: >> - Remove useless +1 in sbs computation. >> - Describe NV12_10LE40_4L4 pixels format. >> - Post-processor could generate P010. >> - Fix comments done on v1. >> - The last patch make sure that only post-processed formats are used when film >> grain feature is enabled. >> >> Benjamin >> > I have a bunch of sparse errors: > > SPARSE:drivers/media/platform/verisilicon/hantro_drv.c:342:17: error: typename in expression > SPARSE:drivers/media/platform/verisilicon/hantro_drv.c:342:21: error: Expected ; at end of statement > SPARSE:drivers/media/platform/verisilicon/hantro_drv.c:342:21: error: got bit_depth > SPARSE:drivers/media/platform/verisilicon/hantro_drv.c:342:17: error: undefined identifier 'int' > SPARSE:drivers/media/platform/verisilicon/hantro_drv.c:346:47: error: undefined identifier 'bit_depth' > SPARSE:drivers/media/platform/verisilicon/hantro_drv.c:353:39: error: undefined identifier 'bit_depth' > SPARSE:drivers/media/platform/verisilicon/hantro_drv.c:354:42: error: undefined identifier 'bit_depth' > > Also some smatch errors: > > SMATCH:drivers/media/platform/verisilicon/hantro_drv.c:342:17: :error: typename in expression > SMATCH:drivers/media/platform/verisilicon/hantro_drv.c:342:21: :error: Expected ; at end of statement > SMATCH:drivers/media/platform/verisilicon/hantro_drv.c:342:21: :error: got bit_depth > drivers/media/platform/verisilicon/hantro_drv.c:342 hantro_av1_s_ctrl() warn: statement has no effect 3 > drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c:227:6: warning: no previous prototype for 'rockchip_vpu981_av1_dec_tiles_free' [-Wmissing-prototypes] > drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c:802:47: warning: variable 'chroma_addr' set but not used [-Wunused-but-set-variable] I have fix that in v4. I will send v4 once Hantro bit depth problem will be fixed for HEVC since the root cause are the same. Thanks, Benjamin > > Regards, > > Hans > >> [1] https://lore.kernel.org/linux-media/20230103154832.6982-1-daniel.almeida@collabora.com/T/#u >> >> Benjamin Gaignard (12): >> dt-bindings: media: rockchip-vpu: Add rk3588 vpu compatible >> media: Add NV12_10LE40_4L4 pixel format >> media: verisilicon: Get bit depth for V4L2_PIX_FMT_NV12_10LE40_4L4 >> media: verisilicon: Add AV1 decoder mode and controls >> media: verisilicon: Save bit depth for AV1 decoder >> media: verisilicon: Check AV1 bitstreams bit depth >> media: verisilicon: Compute motion vectors size for AV1 frames >> media: verisilicon: Add AV1 entropy helpers >> media: verisilicon: Add Rockchip AV1 decoder >> media: verisilicon: Add film grain feature to AV1 driver >> media: verisilicon: Enable AV1 decoder on rk3588 >> media: verisilicon: Conditionnaly ignore native formats >> >> Nicolas Dufresne (1): >> v4l2-common: Add support for fractional bpp >> >> .../bindings/media/rockchip-vpu.yaml | 1 + >> .../media/v4l/pixfmt-yuv-planar.rst | 4 + >> drivers/media/platform/verisilicon/Makefile | 3 + >> drivers/media/platform/verisilicon/hantro.h | 8 + >> .../media/platform/verisilicon/hantro_drv.c | 69 + >> .../media/platform/verisilicon/hantro_hw.h | 102 + >> .../platform/verisilicon/hantro_postproc.c | 7 + >> .../media/platform/verisilicon/hantro_v4l2.c | 22 +- >> .../media/platform/verisilicon/hantro_v4l2.h | 1 + >> .../verisilicon/rockchip_av1_entropymode.c | 4546 +++++++++++++++++ >> .../verisilicon/rockchip_av1_entropymode.h | 272 + >> .../verisilicon/rockchip_av1_filmgrain.c | 401 ++ >> .../verisilicon/rockchip_av1_filmgrain.h | 36 + >> .../verisilicon/rockchip_vpu981_hw_av1_dec.c | 2278 +++++++++ >> .../verisilicon/rockchip_vpu981_regs.h | 477 ++ >> .../platform/verisilicon/rockchip_vpu_hw.c | 134 + >> drivers/media/v4l2-core/v4l2-common.c | 149 +- >> drivers/media/v4l2-core/v4l2-ioctl.c | 1 + >> include/media/v4l2-common.h | 2 + >> include/uapi/linux/videodev2.h | 1 + >> 20 files changed, 8439 insertions(+), 75 deletions(-) >> create mode 100644 drivers/media/platform/verisilicon/rockchip_av1_entropymode.c >> create mode 100644 drivers/media/platform/verisilicon/rockchip_av1_entropymode.h >> create mode 100644 drivers/media/platform/verisilicon/rockchip_av1_filmgrain.c >> create mode 100644 drivers/media/platform/verisilicon/rockchip_av1_filmgrain.h >> create mode 100644 drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c >> create mode 100644 drivers/media/platform/verisilicon/rockchip_vpu981_regs.h >>
Le 25/01/2023 à 13:18, Hans Verkuil a écrit : > Some review comments: > > On 1/11/23 17:59, Benjamin Gaignard wrote: >> AV1 hardware decoder needs entropy parameters to decode frames. >> They are computed from various arrays defined in AV1 section >> "9.4. Default CDF tables". >> >> Add helpers functions to init, store and get these parameters. >> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >> --- >> drivers/media/platform/verisilicon/Makefile | 1 + >> drivers/media/platform/verisilicon/hantro.h | 2 + >> .../media/platform/verisilicon/hantro_hw.h | 20 + >> .../verisilicon/rockchip_av1_entropymode.c | 4546 +++++++++++++++++ >> .../verisilicon/rockchip_av1_entropymode.h | 272 + >> 5 files changed, 4841 insertions(+) >> create mode 100644 drivers/media/platform/verisilicon/rockchip_av1_entropymode.c >> create mode 100644 drivers/media/platform/verisilicon/rockchip_av1_entropymode.h >> >> diff --git a/drivers/media/platform/verisilicon/Makefile b/drivers/media/platform/verisilicon/Makefile >> index ebd5ede7bef7..d2b2679c00eb 100644 >> --- a/drivers/media/platform/verisilicon/Makefile >> +++ b/drivers/media/platform/verisilicon/Makefile >> @@ -18,6 +18,7 @@ hantro-vpu-y += \ >> rockchip_vpu2_hw_h264_dec.o \ >> rockchip_vpu2_hw_mpeg2_dec.o \ >> rockchip_vpu2_hw_vp8_dec.o \ >> + rockchip_av1_entropymode.o \ >> hantro_jpeg.o \ >> hantro_h264.o \ >> hantro_hevc.o \ >> diff --git a/drivers/media/platform/verisilicon/hantro.h b/drivers/media/platform/verisilicon/hantro.h >> index 61480825b856..a98cb40a8d3b 100644 >> --- a/drivers/media/platform/verisilicon/hantro.h >> +++ b/drivers/media/platform/verisilicon/hantro.h >> @@ -240,6 +240,7 @@ struct hantro_dev { >> * @vp8_dec: VP8-decoding context. >> * @hevc_dec: HEVC-decoding context. >> * @vp9_dec: VP9-decoding context. >> + * @av1_dec: AV1-decoding context. >> */ >> struct hantro_ctx { >> struct hantro_dev *dev; >> @@ -268,6 +269,7 @@ struct hantro_ctx { >> struct hantro_vp8_dec_hw_ctx vp8_dec; >> struct hantro_hevc_dec_hw_ctx hevc_dec; >> struct hantro_vp9_dec_hw_ctx vp9_dec; >> + struct hantro_av1_dec_hw_ctx av1_dec; >> }; >> }; >> >> diff --git a/drivers/media/platform/verisilicon/hantro_hw.h b/drivers/media/platform/verisilicon/hantro_hw.h >> index 201f8679ca11..c7438e197d85 100644 >> --- a/drivers/media/platform/verisilicon/hantro_hw.h >> +++ b/drivers/media/platform/verisilicon/hantro_hw.h >> @@ -15,6 +15,8 @@ >> #include <media/v4l2-vp9.h> >> #include <media/videobuf2-core.h> >> >> +#include "rockchip_av1_entropymode.h" >> + >> #define DEC_8190_ALIGN_MASK 0x07U >> >> #define MB_DIM 16 >> @@ -247,6 +249,24 @@ struct hantro_vp9_dec_hw_ctx { >> s16 feature_data[8][4]; >> }; >> >> +/** >> + * hantro_av1_dec_hw_ctx >> + * @cdfs: current probabilities structure >> + * @cdfs_ndvc: current mv probabilities structure >> + * @default_cdfs: default probabilities structure >> + * @default_cdfs_ndvc: default mv probabilties structure >> + * @cdfs_last: stored probabilities structures >> + * @cdfs_last_ndvc: stored mv probabilities structures >> + */ >> +struct hantro_av1_dec_hw_ctx { >> + struct av1cdfs *cdfs; >> + struct mvcdfs *cdfs_ndvc; >> + struct av1cdfs default_cdfs; >> + struct mvcdfs default_cdfs_ndvc; >> + struct av1cdfs cdfs_last[NUM_REF_FRAMES]; >> + struct mvcdfs cdfs_last_ndvc[NUM_REF_FRAMES]; >> +}; >> + >> /** >> * struct hantro_postproc_ctx >> * >> diff --git a/drivers/media/platform/verisilicon/rockchip_av1_entropymode.c b/drivers/media/platform/verisilicon/rockchip_av1_entropymode.c >> new file mode 100644 >> index 000000000000..2b5499a6d995 >> --- /dev/null >> +++ b/drivers/media/platform/verisilicon/rockchip_av1_entropymode.c >> @@ -0,0 +1,4546 @@ >> +// SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > Is 'GPL-2.0-only' correct? In their proprietary MPP code rockchip claims the copyright but Nicolas notices it originally comes from AOM. The correct license shoud be BSD-2-Clause, I will fix that in v4. > >> +/* >> + * Copyright (c) 2016, Alliance for Open Media. All rights reserved >> + * >> + * This source code is subject to the terms of the BSD 2 Clause License and >> + * the Alliance for Open Media Patent License 1.0. If the BSD 2 Clause License > There is no mention of GPL here. > >> + * was not distributed with this source code in the LICENSE file, you can >> + * obtain it at www.aomedia.org/license/software. If the Alliance for Open >> + * Media Patent License 1.0 was not distributed with this source code in the >> + * PATENTS file, you can obtain it at www.aomedia.org/license/patent. >> + */ >> + >> +#include "hantro.h" >> +#include "rockchip_av1_entropymode.h" >> + >> +#define AOM_ICDF ICDF >> +#define AOM_CDF2(a0) AOM_ICDF(a0) >> +#define AOM_CDF3(a0, a1) \ >> + AOM_ICDF(a0), AOM_ICDF(a1) >> +#define AOM_CDF4(a0, a1, a2) \ >> + AOM_ICDF(a0), AOM_ICDF(a1), AOM_ICDF(a2) >> +#define AOM_CDF5(a0, a1, a2, a3) \ >> + AOM_ICDF(a0), AOM_ICDF(a1), AOM_ICDF(a2), AOM_ICDF(a3) >> +#define AOM_CDF6(a0, a1, a2, a3, a4) \ >> + AOM_ICDF(a0), AOM_ICDF(a1), AOM_ICDF(a2), AOM_ICDF(a3), AOM_ICDF(a4) >> +#define AOM_CDF7(a0, a1, a2, a3, a4, a5) \ >> + AOM_ICDF(a0), AOM_ICDF(a1), AOM_ICDF(a2), \ >> + AOM_ICDF(a3), AOM_ICDF(a4), AOM_ICDF(a5) >> +#define AOM_CDF8(a0, a1, a2, a3, a4, a5, a6) \ >> + AOM_ICDF(a0), AOM_ICDF(a1), AOM_ICDF(a2), \ >> + AOM_ICDF(a3), AOM_ICDF(a4), AOM_ICDF(a5), AOM_ICDF(a6) >> +#define AOM_CDF9(a0, a1, a2, a3, a4, a5, a6, a7) \ >> + AOM_ICDF(a0), AOM_ICDF(a1), AOM_ICDF(a2), AOM_ICDF(a3), \ >> + AOM_ICDF(a4), AOM_ICDF(a5), AOM_ICDF(a6), AOM_ICDF(a7) >> +#define AOM_CDF10(a0, a1, a2, a3, a4, a5, a6, a7, a8) \ >> + AOM_ICDF(a0), AOM_ICDF(a1), AOM_ICDF(a2), AOM_ICDF(a3), \ >> + AOM_ICDF(a4), AOM_ICDF(a5), AOM_ICDF(a6), AOM_ICDF(a7), AOM_ICDF(a8) >> +#define AOM_CDF11(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9) \ >> + AOM_ICDF(a0), AOM_ICDF(a1), AOM_ICDF(a2), AOM_ICDF(a3), AOM_ICDF(a4), \ >> + AOM_ICDF(a5), AOM_ICDF(a6), AOM_ICDF(a7), AOM_ICDF(a8), AOM_ICDF(a9) >> +#define AOM_CDF12(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10) \ >> + AOM_ICDF(a0), AOM_ICDF(a1), AOM_ICDF(a2), AOM_ICDF(a3), AOM_ICDF(a4), AOM_ICDF(a5), \ >> + AOM_ICDF(a6), AOM_ICDF(a7), AOM_ICDF(a8), AOM_ICDF(a9), AOM_ICDF(a10) >> +#define AOM_CDF13(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11) \ >> + AOM_ICDF(a0), AOM_ICDF(a1), AOM_ICDF(a2), AOM_ICDF(a3), AOM_ICDF(a4), AOM_ICDF(a5), \ >> + AOM_ICDF(a6), AOM_ICDF(a7), AOM_ICDF(a8), AOM_ICDF(a9), AOM_ICDF(a10), AOM_ICDF(a11) >> +#define AOM_CDF14(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11, a12) \ >> + AOM_ICDF(a0), AOM_ICDF(a1), AOM_ICDF(a2), AOM_ICDF(a3), AOM_ICDF(a4), \ >> + AOM_ICDF(a5), AOM_ICDF(a6), AOM_ICDF(a7), AOM_ICDF(a8), AOM_ICDF(a9), \ >> + AOM_ICDF(a10), AOM_ICDF(a11), AOM_ICDF(a12) >> +#define AOM_CDF15(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11, a12, a13) \ >> + AOM_ICDF(a0), AOM_ICDF(a1), AOM_ICDF(a2), AOM_ICDF(a3), AOM_ICDF(a4), \ >> + AOM_ICDF(a5), AOM_ICDF(a6), AOM_ICDF(a7), AOM_ICDF(a8), AOM_ICDF(a9), \ >> + AOM_ICDF(a10), AOM_ICDF(a11), AOM_ICDF(a12), AOM_ICDF(a13) >> +#define AOM_CDF16(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11, a12, a13, a14) \ >> + AOM_ICDF(a0), AOM_ICDF(a1), AOM_ICDF(a2), AOM_ICDF(a3), AOM_ICDF(a4), \ >> + AOM_ICDF(a5), AOM_ICDF(a6), AOM_ICDF(a7), AOM_ICDF(a8), AOM_ICDF(a9), \ >> + AOM_ICDF(a10), AOM_ICDF(a11), AOM_ICDF(a12), AOM_ICDF(a13), AOM_ICDF(a14) >> + >> +const uint8_t av1_partition_probs[NUM_FRAME_TYPES][NUM_PARTITION_CONTEXTS][PARTITION_TYPES] = { > Weird, this doesn't seem to be used. I would expect this to be static as well. > >> + /* 1 byte padding */ >> + { >> + /* frame_type = keyframe */ >> + /* 8x8 -> 4x4 */ >> + { 158, 97, 94, 0} /* a/l both not split */, >> + { 93, 24, 99, 0} /* a split, l not split */, >> + { 85, 119, 44, 0} /* l split, a not split */, >> + { 62, 59, 67, 0} /* a/l both split */, >> + /* 16x16 -> 8x8 */ >> + { 149, 53, 53, 0} /* a/l both not split */, >> + { 94, 20, 48, 0} /* a split, l not split */, >> + { 83, 53, 24, 0} /* l split, a not split */, >> + { 52, 18, 18, 0} /* a/l both split */, >> + /* 32x32 -> 16x16 */ >> + { 150, 40, 39, 0} /* a/l both not split */, >> + { 78, 12, 26, 0} /* a split, l not split */, >> + { 67, 33, 11, 0} /* l split, a not split */, >> + { 24, 7, 5, 0} /* a/l both split */, >> + /* 64x64 -> 32x32 */ >> + { 174, 35, 49, 0} /* a/l both not split */, >> + { 68, 11, 27, 0} /* a split, l not split */, >> + { 57, 15, 9, 0} /* l split, a not split */, >> + { 12, 3, 3, 0} /* a/l both split */ >> + }, >> + { >> + /* frame_type = interframe */ >> + /* 8x8 -> 4x4 */ >> + { 199, 122, 141, 0} /* a/l both not split */, >> + { 147, 63, 159, 0} /* a split, l not split */, >> + { 148, 133, 118, 0} /* l split, a not split */, >> + { 121, 104, 114, 0} /* a/l both split */, >> + /* 16x16 -> 8x8 */ >> + { 174, 73, 87, 0} /* a/l both not split */, >> + { 92, 41, 83, 0} /* a split, l not split */, >> + { 82, 99, 50, 0} /* l split, a not split */, >> + { 53, 39, 39, 0} /* a/l both split */, >> + /* 32x32 -> 16x16 */ >> + { 177, 58, 59, 0} /* a/l both not split */, >> + { 68, 26, 63, 0} /* a split, l not split */, >> + { 52, 79, 25, 0} /* l split, a not split */, >> + { 17, 14, 12, 0} /* a/l both split */, >> + /* 64x64 -> 32x32 */ >> + { 222, 34, 30, 0} /* a/l both not split */, >> + { 72, 16, 44, 0} /* a split, l not split */, >> + { 58, 32, 12, 0} /* l split, a not split */, >> + { 10, 7, 6, 0} /* a/l both split */ >> + } >> +}; >> + >> +/* Array indices are identical to previously-existing INTRAMODECONTEXTNODES. */ >> +const int8_t av1hwd_intra_mode_tree[] = { > Ditto. > >> + -DC_PRED, >> + 2, /* 0 = DC_NODE */ >> + -TM_PRED_AV1, >> + 4, /* 1 = TM_NODE */ >> + -V_PRED, >> + 6, /* 2 = V_NODE */ >> + 8, >> + 12, /* 3 = COM_NODE */ >> + -H_PRED, >> + 10, /* 4 = H_NODE */ >> + -D135_PRED, >> + -D117_PRED, /* 5 = D135_NODE */ >> + -D45_PRED, >> + 14, /* 6 = D45_NODE */ >> + -D63_PRED, >> + 16, /* 7 = D63_NODE */ >> + -D153_PRED, >> + -D27_PRED /* 8 = D153_NODE */ >> +}; >> + >> +const int8_t av1_sb_mv_ref_tree[6] = { >> + -ZEROMV, 2, -NEARESTMV, >> + 4, -NEARMV, -NEWMV >> +}; >> + >> +const int8_t av1hwd_partition_tree[6] = { >> + -PARTITION_NONE, 2, -PARTITION_HORZ, >> + 4, -PARTITION_VERT, -PARTITION_SPLIT >> +}; >> + >> +const int8_t av1_segment_tree[2 * MAX_MB_SEGMENTS - 2] = { >> + 2, 4, 6, 8, 10, 12, 0, -1, -2, -3, -4, -5, -6, -7 >> +}; >> + >> +const uint8_t av1_default_tx_probs_32x32p[TX_SIZE_CONTEXTS][TX_SIZE_MAX_SB - 1] = { >> + { 3, 136, 37, }, >> + { 5, 52, 13, }, >> +}; >> + >> +const uint8_t av1_default_tx_probs_16x16p[TX_SIZE_CONTEXTS][TX_SIZE_MAX_SB - 2] = { >> + { 20, 152, }, >> + { 15, 101, }, >> +}; >> + >> +const uint8_t av1_default_tx_probs_8x8p[TX_SIZE_CONTEXTS][TX_SIZE_MAX_SB - 3] = { >> + { 100, }, >> + { 66, }, >> +}; >> + >> +const uint8_t av1_default_mbskip_probs[MBSKIP_CONTEXTS] = { 192, 128, 64}; >> + >> +const uint8_t av1_default_intra_ext_tx_prob[EXT_TX_SIZES][TX_TYPES][TX_TYPES - 1] = { >> + { { 240, 85, 128}, { 4, 1, 248}, { 4, 1, 8}, { 4, 248, 128} }, >> + { { 244, 85, 128}, { 8, 2, 248}, { 8, 2, 8}, { 8, 248, 128} }, >> + { { 248, 85, 128}, { 16, 4, 248}, { 16, 4, 8}, { 16, 248, 128} }, >> +}; >> + >> +const uint8_t av1_default_inter_ext_tx_prob[EXT_TX_SIZES][TX_TYPES - 1] = { >> + { 160, 85, 128 }, >> + { 176, 85, 128 }, >> + { 192, 85, 128 }, >> +}; >> + >> +const uint8_t av1_default_motion_mode_prob[MOTION_MODE_CONTEXTS][MOTION_MODE_COUNT - 1] = { >> + { 151 }, { 153 }, { 144 }, { 178 }, >> + { 165 }, { 160 }, { 207 }, { 195 }, >> + { 168 }, { 244 } >> +}; > Same for (I think) all these non-static arrays above. > > What's going on with that? I thought I have remove all unused arrays but obviously I have miss some. The original file contains all the arrays from AV1 specifications but the hardware/driver don't need all of them. That will be fix in v4. Regards, Benjamin > > Regards, > > Hans