[PATCH v3] media: verisilicon: Create AV1 helper library
Benjamin Gaignard
benjamin.gaignard at collabora.com
Mon May 4 03:25:54 PDT 2026
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 at collabora.com>
>> Reviewed-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
>> ---
>> 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
>
More information about the linux-arm-kernel
mailing list