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

["Kyrie Wu (吴晗)" <Kyrie.Wu@mediatek.com>]

On Thu, 2025-10-16 at 11:19 -0400, Nicolas Dufresne wrote:
> 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 ?
Dear Nicolas,

Yes, it's a right thinking. I will change to "single core" to remove
the missing understanding.

Thanks.
> 
> > 
> > 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.

The prob size is independent with resolution. Based on feedback from
hardware designer, this size won't increase. Different ICs will only
choose between 4096 and 2560. However, as more ICs are added, redundant
code will become increasingly common. I'm considering adding a function
to handle this, as shown below:

static int mtk_vcodec_get_vp9_prob_size(int chip_name)
{
	switch (chip_name) {
		case MTK_VDEC_MT8189:
		case MTK_VDEC_MT8189:
			return VP9_PROB_BUF_4K_SIZE;
		default:
			return VP9_PROB_BUF_SIZE;
	}
}

the prob size is set like that:
instance->prob.size =
mtk_vcodec_get_vp9_prob_size(...)

Could you please give further comments for above solution?

Thanks.

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

In the decoding software flow, the app queue src or dst buffer to the
driver will try to schedule the decoding software flow by queuing the
work queue. At this time, only one of the src or dst buffer can be
obtained. However, in the original software flow, calling the
vdec_vp9_slice_setup_single_from_src_to_dst function using
v4l2_m2m_next_src_buf to get the src and dst buffers will return
-EINVAL, interrupting the decoding pipeline. Therefore, this interface
needs to be modified to set both src and dst buffer to set metadata.

Thanks.

Regards,
Kyrie.
> 
> >  	if (ret)
> >  		goto err;
> >