Re: [PATCH v4 4/8] media: mediatek: vcodec: Add core-only VP9 decoding support for MT8189

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 3664 bytes --]

Hi,

Le jeudi 16 octobre 2025 à 14:07 +0800, Kyrie Wu a écrit :
> Implemented core-only VP9 decoding functions for MT8189.

What does "core-only" means ? Did you mean single core ?

> 
> Signed-off-by: Kyrie Wu <kyrie.wu@mediatek.com>
> ---
>  .../vcodec/decoder/vdec/vdec_vp9_req_lat_if.c | 27 +++++++++++--------
>  1 file changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git
> a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c
> b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c
> index fa0f406f7726..04197164fb82 100644
> ---
> a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c
> +++
> b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c
> @@ -23,6 +23,7 @@
>  
>  #define VP9_TILE_BUF_SIZE 4096
>  #define VP9_PROB_BUF_SIZE 2560
> +#define VP9_PROB_BUF_4K_SIZE 3840
>  #define VP9_COUNTS_BUF_SIZE 16384
>  
>  #define HDR_FLAG(x) (!!((hdr)->flags & V4L2_VP9_FRAME_FLAG_##x))
> @@ -616,7 +617,10 @@ static int vdec_vp9_slice_alloc_working_buffer(struct
> vdec_vp9_slice_instance *i
>  	}
>  
>  	if (!instance->prob.va) {
> -		instance->prob.size = VP9_PROB_BUF_SIZE;
> +		instance->prob.size = ((ctx->dev->chip_name ==
> MTK_VDEC_MT8196) ||
> +				       (ctx->dev->chip_name ==
> MTK_VDEC_MT8189)) ?
> +					VP9_PROB_BUF_4K_SIZE :
> VP9_PROB_BUF_SIZE;

I feel like this will keep growing, then you'll move to 8K and it will continue.
You already match every SoC in the driver, you should come up with SoC
configuration data structure so you don't have to add doc check conditions all
over the place. This change is also not reflected in the commit message.

> +
>  		if (mtk_vcodec_mem_alloc(ctx, &instance->prob))
>  			goto err;
>  	}
> @@ -696,21 +700,22 @@ static int vdec_vp9_slice_tile_offset(int idx, int
> mi_num, int tile_log2)
>  	return min(offset, mi_num);
>  }
>  
> -static
> -int vdec_vp9_slice_setup_single_from_src_to_dst(struct
> vdec_vp9_slice_instance *instance)
> +static int vdec_vp9_slice_setup_single_from_src_to_dst(struct
> vdec_vp9_slice_instance *instance,
> +						       struct mtk_vcodec_mem
> *bs,
> +						       struct vdec_fb *fb)
>  {
> -	struct vb2_v4l2_buffer *src;
> -	struct vb2_v4l2_buffer *dst;
> +	struct mtk_video_dec_buf *src_buf_info;
> +	struct mtk_video_dec_buf *dst_buf_info;
>  
> -	src = v4l2_m2m_next_src_buf(instance->ctx->m2m_ctx);
> -	if (!src)
> +	src_buf_info = container_of(bs, struct mtk_video_dec_buf, bs_buffer);
> +	if (!src_buf_info)
>  		return -EINVAL;
>  
> -	dst = v4l2_m2m_next_dst_buf(instance->ctx->m2m_ctx);
> -	if (!dst)
> +	dst_buf_info = container_of(fb, struct mtk_video_dec_buf,
> frame_buffer);
> +	if (!dst_buf_info)
>  		return -EINVAL;
>  
> -	v4l2_m2m_buf_copy_metadata(src, dst, true);
> +	v4l2_m2m_buf_copy_metadata(&src_buf_info->m2m_buf.vb, &dst_buf_info-
> >m2m_buf.vb, true);
>  
>  	return 0;
>  }
> @@ -1800,7 +1805,7 @@ static int vdec_vp9_slice_setup_single(struct
> vdec_vp9_slice_instance *instance,
>  	struct vdec_vp9_slice_vsi *vsi = &pfc->vsi;
>  	int ret;
>  
> -	ret = vdec_vp9_slice_setup_single_from_src_to_dst(instance);
> +	ret = vdec_vp9_slice_setup_single_from_src_to_dst(instance, bs, fb);

This entire change is not explained in the commit message at all. Explain why
this is needed, what difference it makes. There is no clear indication we are in
an MT8189 code path, so this change could have a incidence on all single core
SoC (if any).

Nicolas

>  	if (ret)
>  		goto err;
>  

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