Re: [PATCH v3] media: verisilicon: Create AV1 helper library

From: Benjamin Gaignard

Date: Mon May 04 2026 - 06:30:35 EST


Le 04/05/2026 à 09:20, Hans Verkuil a écrit :
Hi Benjamin,

I have a few comments about this:

On 15/04/2026 09:38, Benjamin Gaignard wrote:
Regroup all none hardware related AV1 functions into a helper library.
The goal is to avoid code duplication for futur AV1 codecs.
futur -> future

Tested on rock 5b board Fluster score remains the same 204/241.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxxxxx>
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@xxxxxxxxxxxxx>
---
changes in version 3:
- Remove useless wrapper functions.

drivers/media/platform/verisilicon/Makefile | 7 +-
.../media/platform/verisilicon/hantro_av1.c | 780 +++++++++++++++
.../media/platform/verisilicon/hantro_av1.h | 62 ++
...entropymode.c => hantro_av1_entropymode.c} | 18 +-
...entropymode.h => hantro_av1_entropymode.h} | 18 +-
...av1_filmgrain.c => hantro_av1_filmgrain.c} | 82 +-
.../verisilicon/hantro_av1_filmgrain.h | 44 +
.../media/platform/verisilicon/hantro_hw.h | 7 +-
.../verisilicon/rockchip_av1_filmgrain.h | 36 -
.../verisilicon/rockchip_vpu981_hw_av1_dec.c | 935 ++----------------
.../platform/verisilicon/rockchip_vpu_hw.c | 7 +-
11 files changed, 1041 insertions(+), 955 deletions(-)
create mode 100644 drivers/media/platform/verisilicon/hantro_av1.c
create mode 100644 drivers/media/platform/verisilicon/hantro_av1.h
rename drivers/media/platform/verisilicon/{rockchip_av1_entropymode.c => hantro_av1_entropymode.c} (99%)
rename drivers/media/platform/verisilicon/{rockchip_av1_entropymode.h => hantro_av1_entropymode.h} (95%)
rename drivers/media/platform/verisilicon/{rockchip_av1_filmgrain.c => hantro_av1_filmgrain.c} (92%)
create mode 100644 drivers/media/platform/verisilicon/hantro_av1_filmgrain.h
delete mode 100644 drivers/media/platform/verisilicon/rockchip_av1_filmgrain.h

<snip>

diff --git a/drivers/media/platform/verisilicon/hantro_av1.c b/drivers/media/platform/verisilicon/hantro_av1.c
new file mode 100644
index 000000000000..5a51ac877c9c
--- /dev/null
+++ b/drivers/media/platform/verisilicon/hantro_av1.c
@@ -0,0 +1,780 @@
<snip>

+
+int hantro_av1_tile_log2(int target)
+{
+ int k;
+
+ /*
+ * returns the smallest value for k such that 1 << k is greater
+ * than or equal to target
+ */
+ for (k = 0; (1 << k) < target; k++);
Checkpatch gives:

ERROR: trailing statements should be on next line
#637: FILE: drivers/media/platform/verisilicon/hantro_av1.c:568:
+ for (k = 0; (1 << k) < target; k++);

Just move the ';' to the next line.

OK I will do that.


+
+ return k;
+}
+
<snip>

diff --git a/drivers/media/platform/verisilicon/hantro_av1_filmgrain.h b/drivers/media/platform/verisilicon/hantro_av1_filmgrain.h
new file mode 100644
index 000000000000..5593e84114d0
--- /dev/null
+++ b/drivers/media/platform/verisilicon/hantro_av1_filmgrain.h
@@ -0,0 +1,44 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef _HANTRO_AV1_FILMGRAIN_H_
+#define _HANTRO_AV1_FILMGRAIN_H_
+
+#include <linux/types.h>
+
+struct hantro_av1_film_grain {
+ u8 scaling_lut_y[256];
+ u8 scaling_lut_cb[256];
+ u8 scaling_lut_cr[256];
+ s16 cropped_luma_grain_block[4096];
+ s16 cropped_chroma_grain_block[1024 * 2];
+};
This struct is not used in hantro_av1_filmgrain.c/h.

Does this belong here?

The same structure can used for future Verisilicon hardware block so I would prefer to
keep it here.


+
+void hantro_av1_generate_luma_grain_block(s32 (*luma_grain_block)[73][82],
+ s32 bitdepth,
+ u8 num_y_points,
+ s32 grain_scale_shift,
+ s32 ar_coeff_lag,
+ s32 (*ar_coeffs_y)[24],
+ s32 ar_coeff_shift,
+ s32 grain_min,
+ s32 grain_max,
+ u16 random_seed);
+
+void hantro_av1_generate_chroma_grain_block(s32 (*luma_grain_block)[73][82],
+ s32 (*cb_grain_block)[38][44],
+ s32 (*cr_grain_block)[38][44],
+ s32 bitdepth,
+ u8 num_y_points,
+ u8 num_cb_points,
+ u8 num_cr_points,
+ s32 grain_scale_shift,
+ s32 ar_coeff_lag,
+ s32 (*ar_coeffs_cb)[25],
+ s32 (*ar_coeffs_cr)[25],
+ s32 ar_coeff_shift,
+ s32 grain_min,
+ s32 grain_max,
+ u8 chroma_scaling_from_luma,
+ u16 random_seed);
+
I get a lot of checkpatch warnings of this type:

WARNING: function definition argument 's32' should also have an identifier name
#1205: FILE: drivers/media/platform/verisilicon/hantro_av1_filmgrain.h:16:
+void hantro_av1_generate_luma_grain_block(s32 (*luma_grain_block)[73][82],
Looking at how it is used in rockchip_vpu981_av1_dec_set_fgs() I think this
can be done a lot easier if you add a new struct here containing those
arrays. E.g.:

struct hantro_av1_coeffs_grain_block {
s32 ar_coeffs_y[24];
s32 ar_coeffs_cb[25];
s32 ar_coeffs_cr[25];
s32 luma_grain_block[73][82];
s32 cb_grain_block[38][44];
s32 cr_grain_block[38][44];
};

Then in rockchip_vpu981_av1_dec_set_fgs you can just kzalloc that struct
and pass it to these filmgrain functions.

I will do that.


Also consider using #defines for the array sizes.

Chapter "7.18.3.3. Generate grain process" of AV1 spec doesn't define/name
these values so keep them like this make the code more easy to review with
the spec.

https://aomediacodec.github.io/av1-spec/#generate-grain-process

That said I'm always for naming values so good suggestion are welcome.

Thanks,
Benjamin


If you decide not to use a struct, then you can at least change simply use
's32 luma_grain_block[73][82]' as arguments to these helper functions. There
is no need to complicate the function prototypes.

But I think creating a struct is a cleaner approach.

Regards,

Hans