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

From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
To: Benjamin Gaignard <benjamin.gaignard@collabora.com>,
	 p.zabel@pengutronix.de, mchehab@kernel.org, heiko@sntech.de,
	hverkuil@kernel.org
Cc: linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org,
	 linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,  kernel@collabora.com,
	amazingfate <liujianfeng1994@gmail.com>
Subject: Re: [PATCH] media: verisilicon: AV1: Fix enable cdef computation
Date: Mon, 08 Dec 2025 14:45:02 -0500	[thread overview]
Message-ID: <4545bc3df2592965388aee3f170cce577d8cf538.camel@collabora.com> (raw)
In-Reply-To: <20251208095227.19720-1-benjamin.gaignard@collabora.com>

[-- Attachment #1.1: Type: text/plain, Size: 2902 bytes --]

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

This is missing:

Reported-by: Jianfeng Liu <liujianfeng1994@gmail.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@collabora.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));

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip