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

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

[Benjamin Gaignard]

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.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Fixes: 727a400686a2c ("media: verisilicon: Add Rockchip AV1 decoder")
---
 .../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));
-- 
2.43.0


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

[PATCH] media: verisilicon: AV1: Fix tx mode bit setting

[Benjamin Gaignard]

If tx_mode field is non-zero then write it value + 2 else
write 0.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Fixes: 727a400686a2c ("media: verisilicon: Add Rockchip AV1 decoder")
---
 drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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 f4f7cb45b1f1..fccdece51b1b 100644
--- a/drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c
+++ b/drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c
@@ -2005,7 +2005,7 @@ static void rockchip_vpu981_av1_dec_set_parameters(struct hantro_ctx *ctx)
 			 !!(ctrls->frame->flags & V4L2_AV1_FRAME_FLAG_ALLOW_HIGH_PRECISION_MV));
 	hantro_reg_write(vpu, &av1_comp_pred_mode,
 			 (ctrls->frame->flags & V4L2_AV1_FRAME_FLAG_REFERENCE_SELECT) ? 2 : 0);
-	hantro_reg_write(vpu, &av1_transform_mode, (ctrls->frame->tx_mode == 1) ? 3 : 4);
+	hantro_reg_write(vpu, &av1_transform_mode, ctrls->frame->tx_mode ? ctrls->frame->tx_mode + 2 : 0);
 	hantro_reg_write(vpu, &av1_max_cb_size,
 			 (ctrls->sequence->flags
 			  & V4L2_AV1_SEQUENCE_FLAG_USE_128X128_SUPERBLOCK) ? 7 : 6);
-- 
2.43.0


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

Re: [PATCH] media: verisilicon: AV1: Fix tx mode bit setting

[Nicolas Dufresne]

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

Hi,

Le lundi 08 décembre 2025 à 10:52 +0100, Benjamin Gaignard a écrit :
> If tx_mode field is non-zero then write it value + 2 else

it -> its



> write 0.


Please don't sent a an In-reply-to: email for a patch that isn't part of a
series.


> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> Fixes: 727a400686a2c ("media: verisilicon: Add Rockchip AV1 decoder")
> ---
>  drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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 f4f7cb45b1f1..fccdece51b1b 100644
> --- a/drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c
> +++ b/drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c
> @@ -2005,7 +2005,7 @@ static void
> rockchip_vpu981_av1_dec_set_parameters(struct hantro_ctx *ctx)
>  			 !!(ctrls->frame->flags &
> V4L2_AV1_FRAME_FLAG_ALLOW_HIGH_PRECISION_MV));
>  	hantro_reg_write(vpu, &av1_comp_pred_mode,
>  			 (ctrls->frame->flags &
> V4L2_AV1_FRAME_FLAG_REFERENCE_SELECT) ? 2 : 0);
> -	hantro_reg_write(vpu, &av1_transform_mode, (ctrls->frame->tx_mode ==
> 1) ? 3 : 4);
> +	hantro_reg_write(vpu, &av1_transform_mode, ctrls->frame->tx_mode ?
> ctrls->frame->tx_mode + 2 : 0);

That all seem very hacky. Let's step back, and use that as inspiration for a
useful commit message. From bitstream we have:

tx_mode:
  0 == 4x4 only
  1 == SELECT
  2 == LARGEST

And the HW have:
  0 == 4x4 only
  1 == 8x8 and less
  2 == 16x16 and less
  3 == 32x32 and less
  4 == Select

Since the two enums have no relation (except for value 0), I'd suggest to
translate this in a switch (with nice enums so its readable).

  0 -> 0
  1 -> 4
  2 -> 3

Would be good to check if we can refine with some other params to reach HW value
1 and 2, but otherwise, that mapping is sufficient. Tha hacky +2 is really
obscure to me and I'd rather not do that upstream.

Nicolas

>  	hantro_reg_write(vpu, &av1_max_cb_size,
>  			 (ctrls->sequence->flags
>  			  & V4L2_AV1_SEQUENCE_FLAG_USE_128X128_SUPERBLOCK) ?
> 7 : 6);

[-- 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

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

[Nicolas Dufresne]

[-- 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