Re: [PATCH v6 4/6] media: mediatek: encoder: Add support for VCP encode process

["Irui Wang (王瑞)" <Irui.Wang@mediatek.com>]

Dear Nicolas,

Thank you greatly for taking the time to help review the patch.

On Wed, 2026-04-29 at 10:56 -0400, Nicolas Dufresne wrote:
> Le jeudi 23 avril 2026 à 15:33 +0800, Irui Wang a écrit :
> > Adapt the encoder driver to support VCP firmware interface.
> > 
> > Set the encoder driver firmware type to 'VCP'.
> > Allocate RC buffers using the VCP device.
> > Send the shared memory address to VCP and map the encoder VSI
> > address
> > to the CPU address space using the VCP shared memory address.
> > 
> > Signed-off-by: Irui Wang <irui.wang@mediatek.com>
> > ---
> >  .../mediatek/vcodec/common/mtk_vcodec_fw.c    |  6 +++++
> >  .../mediatek/vcodec/common/mtk_vcodec_fw.h    |  1 +
> >  .../vcodec/common/mtk_vcodec_fw_priv.h        |  1 +
> >  .../vcodec/common/mtk_vcodec_fw_vcp.c         |  6 +++++
> >  .../vcodec/encoder/mtk_vcodec_enc_drv.c       |  3 +++
> >  .../vcodec/encoder/venc/venc_common_if.c      | 23 ++++++++++++++-
> > ----
> >  .../mediatek/vcodec/encoder/venc_vpu_if.c     | 14 ++++++++++-
> >  7 files changed, 47 insertions(+), 7 deletions(-)
> > 
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw.c
> > b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw.c
> > index 0381acceda25..7a504f093bd8 100644
> > --- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw.c
> > +++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw.c
> > @@ -105,3 +105,9 @@ int mtk_vcodec_fw_get_type(struct mtk_vcodec_fw
> > *fw)
> >  	return fw->type;
> >  }
> >  EXPORT_SYMBOL_GPL(mtk_vcodec_fw_get_type);
> > +
> > +struct device *mtk_vcodec_fw_get_dev(struct mtk_vcodec_fw *fw)
> > +{
> > +	return fw->ops->get_fw_dev(fw);
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_vcodec_fw_get_dev);
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw.h
> > b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw.h
> > index e7304a7dd3e0..56c26b91651e 100644
> > --- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw.h
> > +++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw.h
> > @@ -43,5 +43,6 @@ int mtk_vcodec_fw_ipi_send(struct mtk_vcodec_fw
> > *fw, int id,
> >  int mtk_vcodec_fw_get_type(struct mtk_vcodec_fw *fw);
> >  int mtk_vcodec_fw_get_ipi(enum mtk_vcodec_fw_type type, int
> > hw_id);
> >  int mtk_vcodec_fw_get_venc_ipi(enum mtk_vcodec_fw_type type);
> > +struct device *mtk_vcodec_fw_get_dev(struct mtk_vcodec_fw *fw);
> >  
> >  #endif /* _MTK_VCODEC_FW_H_ */
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_priv.
> > h
> > b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_priv.
> > h
> > index 0a2a9b010244..710c83c871f4 100644
> > ---
> > a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_priv.
> > h
> > +++
> > b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_priv.
> > h
> > @@ -29,6 +29,7 @@ struct mtk_vcodec_fw_ops {
> >  	int (*ipi_send)(struct mtk_vcodec_fw *fw, int id, void *buf,
> >  			unsigned int len, unsigned int wait);
> >  	void (*release)(struct mtk_vcodec_fw *fw);
> > +	struct device *(*get_fw_dev)(struct mtk_vcodec_fw *fw);
> >  };
> >  
> >  #if IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VCODEC_VPU)
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vcp.c
> > b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vcp.c
> > index 6b69ce44d4bb..2859fe78f67d 100644
> > ---
> > a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vcp.c
> > +++
> > b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vcp.c
> > @@ -500,6 +500,11 @@ static void mtk_vcodec_vcp_release(struct
> > mtk_vcodec_fw *fw)
> >  
> >  }
> >  
> > +static struct device *mtk_vcodec_vcp_get_fw_dev(struct
> > mtk_vcodec_fw *fw)
> > +{
> > +	return fw->vcp->vcp_device->dev;
> > +}
> > +
> >  static const struct mtk_vcodec_fw_ops mtk_vcodec_vcp_msg = {
> >  	.load_firmware = mtk_vcodec_vcp_load_firmware,
> >  	.get_vdec_capa = mtk_vcodec_vcp_get_vdec_capa,
> > @@ -508,6 +513,7 @@ static const struct mtk_vcodec_fw_ops
> > mtk_vcodec_vcp_msg = {
> >  	.ipi_register = mtk_vcodec_vcp_set_ipi_register,
> >  	.ipi_send = mtk_vcodec_vcp_ipi_send,
> >  	.release = mtk_vcodec_vcp_release,
> > +	.get_fw_dev = mtk_vcodec_vcp_get_fw_dev,
> >  };
> >  
> >  struct mtk_vcodec_fw *mtk_vcodec_fw_vcp_init(void *priv, enum
> > mtk_vcodec_fw_use fw_use)
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_drv
> > .c
> > b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_drv
> > .c
> > index 4e4541b2fc8e..2f6ee0cd15e3 100644
> > ---
> > a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_drv
> > .c
> > +++
> > b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_drv
> > .c
> > @@ -262,6 +262,9 @@ static int mtk_vcodec_probe(struct
> > platform_device *pdev)
> >  	} else if (!of_property_read_u32(pdev->dev.of_node,
> > "mediatek,scp",
> >  					 &rproc_phandle)) {
> >  		fw_type = SCP;
> > +	} else if (!of_property_read_u32(pdev->dev.of_node,
> > "mediatek,vcp",
> > +					 &rproc_phandle)) {
> > +		fw_type = VCP;
> 
> Similar comment on the patchset you depend on, its third firmware
> type, and time
> to cleanup. The of_device_get_match_data() should be sufficient for
> this simple
> case, there is not reason for custom DT parsing here. PLace the
> fw_type in the
> pdata, and move of_device_get_match_data() call earlier.
> 
> Please apply the same logic for the entire patchset. Every bit of
> information
> that is purely static to your device is better placed in the pdata
> then through
> addition of if/else conditions all over the place. The pdata can also
> have ops
> in it, letting easily firmware specific code without having to do
> if/else
> everywhere.
> 
> Nicolas

I will clean up this if/else in next patch, and with the logic, I will
review and modify the entire patch set if needed.

Thanks
Best Regards

> 
> >  	} else {
> >  		dev_err(&pdev->dev, "[MTK VCODEC] Could not get venc
> > IPI device");
> >  		return -ENODEV;
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_common_i
> > f.c
> > b/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_common_i
> > f.c
> > index 050b827f0fd0..d981155aeb8c 100644
> > ---
> > a/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_common_i
> > f.c
> > +++
> > b/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_common_i
> > f.c
> > @@ -480,8 +480,13 @@ static void venc_free_rc_buf(struct venc_inst
> > *inst,
> >  {
> >  	int i;
> >  	struct device *dev;
> > +	struct mtk_vcodec_fw *fw = inst->ctx->dev->fw_handler;
> > +
> > +	if (mtk_vcodec_fw_get_type(fw) == VCP)
> > +		dev = mtk_vcodec_fw_get_dev(fw);
> > +	else
> > +		dev = &inst->ctx->dev->plat_dev->dev;
> >  
> > -	dev = &inst->ctx->dev->plat_dev->dev;
> >  	mtk_venc_mem_free(inst, dev, &bufs->rc_code);
> >  
> >  	for (i = 0; i < core_num; i++)
> > @@ -530,12 +535,18 @@ static int venc_alloc_rc_buf(struct venc_inst
> > *inst,
> >  	struct device *dev;
> >  	void *tmp_va;
> >  
> > -	dev = &inst->ctx->dev->plat_dev->dev;
> > -	if (mtk_venc_mem_alloc(inst, dev, &bufs->rc_code))
> > -		return -ENOMEM;
> > +	if (mtk_vcodec_fw_get_type(fw) == VCP) {
> > +		dev = mtk_vcodec_fw_get_dev(fw);
> > +		if (mtk_venc_mem_alloc(inst, dev, &bufs->rc_code))
> > +			return -ENOMEM;
> > +	} else {
> > +		dev = &inst->ctx->dev->plat_dev->dev;
> > +		if (mtk_venc_mem_alloc(inst, dev, &bufs->rc_code))
> > +			return -ENOMEM;
> >  
> > -	tmp_va = mtk_vcodec_fw_map_dm_addr(fw, bufs->rc_code.pa);
> > -	memcpy(bufs->rc_code.va, tmp_va, bufs->rc_code.size);
> > +		tmp_va = mtk_vcodec_fw_map_dm_addr(fw, bufs-
> > >rc_code.pa);
> > +		memcpy(bufs->rc_code.va, tmp_va, bufs->rc_code.size);
> > +	}
> >  
> >  	for (i = 0; i < core_num; i++) {
> >  		if (mtk_venc_mem_alloc(inst, dev, &bufs->rc_info[i]))
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
> > b/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
> > index 7772b8442ebc..0f4693e04a9f 100644
> > --- a/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
> > +++ b/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
> > @@ -8,13 +8,23 @@
> >  #include "venc_ipi_msg.h"
> >  #include "venc_vpu_if.h"
> >  
> > +#define VSI_OFFSET_MASK 0x0FFFFFFF
> > +
> >  static void handle_enc_init_msg(struct venc_vpu_inst *vpu, const
> > void *data)
> >  {
> >  	const struct venc_vpu_ipi_msg_init_comm *msg = data;
> >  	struct mtk_vcodec_fw *fw = vpu->ctx->dev->fw_handler;
> > +	u64 pa_start, vsi_offset;
> >  
> >  	vpu->inst_addr = msg->init_ack.vpu_inst_addr;
> > -	vpu->vsi = mtk_vcodec_fw_map_dm_addr(fw, vpu->inst_addr);
> > +
> > +	if (mtk_vcodec_fw_get_type(fw) == VCP) {
> > +		pa_start = (u64)fw->vcp->iova_addr;
> > +		vsi_offset = (msg->vpu_vsi_addr & VSI_OFFSET_MASK) -
> > (pa_start & VSI_OFFSET_MASK);
> > +		vpu->vsi = mtk_vcodec_fw_map_dm_addr(fw, ENCODER_MEM) +
> > vsi_offset;
> > +	} else {
> > +		vpu->vsi = mtk_vcodec_fw_map_dm_addr(fw, msg-
> > >vpu_vsi_addr);
> > +	}
> >  
> >  	/* Firmware version field value is unspecified on MT8173. */
> >  	if (mtk_vcodec_fw_get_type(fw) == VPU)
> > @@ -155,6 +165,8 @@ int vpu_enc_init(struct venc_vpu_inst *vpu)
> >  	out.base.venc_inst = (unsigned long)vpu;
> >  	if (MTK_ENC_DRV_IS_COMM(vpu->ctx)) {
> >  		out.codec_type = vpu->ctx->q_data[MTK_Q_DATA_DST].fmt-
> > >fourcc;
> > +		if (mtk_vcodec_fw_get_type(vpu->ctx->dev->fw_handler)
> > == VCP)
> > +			out.shared_iova = vpu->ctx->dev->fw_handler-
> > >vcp->iova_addr;
> >  		msg_size = sizeof(struct venc_ap_ipi_msg_init_comm);
> >  	} else {
> >  		msg_size = sizeof(struct venc_ap_ipi_msg_init);