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

[Benjamin Gaignard <benjamin.gaignard@collabora.com>]

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@collabora.com>
>> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@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
>