1. Patchew
  2. linux
  3. View series

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

Benjamin Gaignard posted 1 patch 1 week, 3 days ago
There is a newer version of this series
.../platform/verisilicon/rockchip_vpu981_hw_av1_dec.c  | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
[PATCH] media: verisilicon: AV1: Fix enable cdef computation
Posted by Benjamin Gaignard 1 week, 3 days ago 
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
Re: [PATCH] media: verisilicon: AV1: Fix enable cdef computation
Posted by Nicolas Dufresne 1 week, 3 days ago 
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));
[PATCH] media: verisilicon: AV1: Fix tx mode bit setting
Posted by Benjamin Gaignard 1 week, 3 days ago 
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
Re: [PATCH] media: verisilicon: AV1: Fix tx mode bit setting
Posted by Nicolas Dufresne 1 week, 3 days ago 
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);

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.