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

[Hans Verkuil <hverkuil+cisco@kernel.org>]

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.

> +
> +	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?

> +
> +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.

Also consider using #defines for the array sizes.

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