Re: [PATCH] media: verisilicon: AV1: Fix enable cdef computation

[Nicolas Dufresne]

Hi,

Le lundi 08 décembre 2025 à 10:52 +0100, Benjamin Gaignard a écrit :
> Testing V4L2_AV1_SEQUENCE_FLAG_ENABLE_CDEF flag isn't enough
> to know if cdef bit has to be set.
> If any of the used cdef fields isn't zero then we must enable
> cdef feature on the hardware.

I think the problem goes the other way around.

   If all the fields of the CDEF parameters are zero (which is the default), then
   av1_enable_cdef register needs to be unset (despite the
   V4L2_AV1_SEQUENCE_FLAG_ENABLE_CDEF possibly being set).

Its interesting to note that the other AV1 decoder also ignores this flag.
Though, I don't have enough data to add something to the doc to try and convince
future driver writers to not use it.

> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxxxxx>
> Fixes: 727a400686a2c ("media: verisilicon: Add Rockchip AV1 decoder")

This is missing:

Reported-by: Jianfeng Liu <liujianfeng1994@xxxxxxxxx>
Link: https://gitlab.freedesktop.org/gstreamer/gstreamer/-/issues/4786

Please also include my Rb in v2 with correct commit message.

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@xxxxxxxxxxxxx>

Nicolas

> ---
>  .../platform/verisilicon/rockchip_vpu981_hw_av1_dec.c  | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c b/drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c
> index e4703bb6be7c..f4f7cb45b1f1 100644
> --- a/drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c
> +++ b/drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c
> @@ -1396,8 +1396,16 @@ static void rockchip_vpu981_av1_dec_set_cdef(struct hantro_ctx *ctx)
>  	u16 luma_sec_strength = 0;
>  	u32 chroma_pri_strength = 0;
>  	u16 chroma_sec_strength = 0;
> +	bool enable_cdef;
>  	int i;
>  
> +	enable_cdef = !(cdef->bits == 0 &&
> +			cdef->damping_minus_3 == 0 &&
> +			cdef->y_pri_strength[0] == 0 &&
> +			cdef->y_sec_strength[0] == 0 &&
> +			cdef->uv_pri_strength[0] == 0 &&
> +			cdef->uv_sec_strength[0] == 0);
> +	hantro_reg_write(vpu, &av1_enable_cdef, enable_cdef);
>  	hantro_reg_write(vpu, &av1_cdef_bits, cdef->bits);
>  	hantro_reg_write(vpu, &av1_cdef_damping, cdef->damping_minus_3);
>  
> @@ -1953,8 +1961,6 @@ static void rockchip_vpu981_av1_dec_set_parameters(struct hantro_ctx *ctx)
>  			 !!(ctrls->frame->flags & V4L2_AV1_FRAME_FLAG_SHOW_FRAME));
>  	hantro_reg_write(vpu, &av1_switchable_motion_mode,
>  			 !!(ctrls->frame->flags & V4L2_AV1_FRAME_FLAG_IS_MOTION_MODE_SWITCHABLE));
> -	hantro_reg_write(vpu, &av1_enable_cdef,
> -			 !!(ctrls->sequence->flags & V4L2_AV1_SEQUENCE_FLAG_ENABLE_CDEF));
>  	hantro_reg_write(vpu, &av1_allow_masked_compound,
>  			 !!(ctrls->sequence->flags
>  			    & V4L2_AV1_SEQUENCE_FLAG_ENABLE_MASKED_COMPOUND));

Attachment: signature.asc
Description: This is a digitally signed message part