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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4545bc3df2592965388aee3f170cce577d8cf538.camel@collabora.com>
Date: Mon, 08 Dec 2025 14:45:02 -0500
From: Nicolas Dufresne <nicolas.dufresne@...labora.com>
To: Benjamin Gaignard <benjamin.gaignard@...labora.com>, 
	p.zabel@...gutronix.de, mchehab@...nel.org, heiko@...ech.de,
 hverkuil@...nel.org
Cc: linux-media@...r.kernel.org, linux-rockchip@...ts.infradead.org, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	kernel@...labora.com, amazingfate <liujianfeng1994@...il.com>
Subject: Re: [PATCH] media: verisilicon: AV1: Fix enable cdef computation

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@...labora.com>
> Fixes: 727a400686a2c ("media: verisilicon: Add Rockchip AV1 decoder")

This is missing:

Reported-by: Jianfeng Liu <liujianfeng1994@...il.com>
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@...labora.com>

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));

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)