Re: [PATCH v7 03/14] media: mediatek: vcodec: add driver to support vcp encoder

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing device tree parsing makes VCP encoder initialization dead code
- [High] Stub mtk_vcodec_vcp_dm_addr() returns NULL causing encoder init failure
- [High] IPI ID mismatch causes VCP encoder IPI registration and messaging to fail
--

commit 28bb4d159c923a5740008e975162078560f6eee9
Author: Kyrie Wu <kyrie.wu@mediatek.com>

media: mediatek: vcodec: add driver to support vcp encoder
    
Encoder also need to call vcp interface to communicate with vcp,
add driver to support encoder.

> 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 150d842c7d226..ece4e3e87e62d 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
> @@ -223,8 +225,13 @@ static int mtk_vcodec_vcp_msg_ipi_send(struct mtk_vcodec_fw *fw, int id, void *b
>  	}
>  
>  	mutex_lock(&vcp->ipi_mutex);
> -	feature_id = VDEC_FEATURE_ID;
> -	mailbox_id = IPI_OUT_VDEC_1;
> +	if (fw->fw_use == ENCODER) {
> +		feature_id = VENC_FEATURE_ID;
> +		mailbox_id = IPI_OUT_VENC_0;
> +	} else {
> +		feature_id = VDEC_FEATURE_ID;
> +		mailbox_id = IPI_OUT_VDEC_1;
> +	}

[Severity: High]
Does this code handle the IPI IDs used by the encoder correctly?

The VCP firmware abstraction requires the IPI id to be less than VCP_IPI_MAX
(which is 3). However, the encoder driver initializes its vpu->id to values
like SCP_IPI_VENC_H264 (4) or IPI_VENC_VP8 (5).

For example, in venc_h264_if.c:

drivers/media/platform/mediatek/vcodec/encoder/venc/venc_h264_if.c:venc_h264_init() {
    ...
    inst->vpu_inst.id = SCP_IPI_VENC_H264;
    ...
}

When this id is passed to mtk_vcodec_vcp_msg_ipi_send() and checked against
mtk_vcodec_vcp_ipi_id_valid(), won't validation fail and return -EINVAL?
This would prevent the VCP encoder from successfully sending messages.

Note that while patch 04 ('media: mediatek: vcodec: get different firmware
ipi id') fixes this mapping for decoders via mtk_vcodec_fw_get_ipi(), it
appears to completely omit the encoder implementations.

[ ... ]

> @@ -402,6 +414,26 @@ static int mtk_vcodec_vcp_load_firmware(struct mtk_vcodec_fw *fw)
>  		mutex_init(&fw->vcp->ipi_desc[i].lock);
>  	mutex_init(&fw->vcp->ipi_mutex);
>  
> +	if (fw->fw_use == ENCODER) {
> +		init_waitqueue_head(&fw->vcp->msg_wq[VCP_IPI_ENCODER]);
> +		fw->vcp->msg_thread =
> +			kthread_run(mtk_vcodec_vcp_msg_process_thread, fw, "vcp_enc_msq_thread");
> +		if (IS_ERR(fw->vcp->msg_thread)) {
> +			ret = PTR_ERR(fw->vcp->msg_thread);
> +			fw->vcp->msg_thread = NULL;
> +			goto err_deregister_feature;
> +		}
> +
> +		fw->vcp->vsi_addr = vcp_device->ops->get_mem_virt(vcp_device, mem_id);
> +		fw->vcp->vsi_size = vcp_device->ops->get_mem_size(vcp_device, mem_id);
> +		fw->vcp->iova_addr = vcp_device->ops->get_mem_iova(vcp_device, mem_id);
> +
> +		dev_dbg(&fw->pdev->dev, "enc vcp init done => va: %p size:0x%x iova:%pad.\n",
> +			fw->vcp->vsi_addr, fw->vcp->vsi_size, &fw->vcp->iova_addr);
> +
> +		return 0;
> +	}

[Severity: High]
Does the encoder receive a valid VSI pointer during initialization?

In mtk_vcodec_fw_vcp.c, mtk_vcodec_vcp_dm_addr() is currently a stub that
unconditionally returns NULL:

drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vcp.c:mtk_vcodec_vcp_dm_addr() {
    static void *mtk_vcodec_vcp_dm_addr(struct mtk_vcodec_fw *fw, u32 dtcm_dmem_addr)
    {
        return NULL;
    }
}

When the encoder receives VPU_IPIMSG_ENC_INIT_DONE, it calls
mtk_vcodec_fw_map_dm_addr(), which invokes this stub and causes vpu->vsi
to be NULL.

The core encoder initialization then explicitly rejects a NULL VSI address:

drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c:vpu_enc_init() {
    ...
    if (IS_ERR_OR_NULL(vpu->vsi)) {
        mtk_venc_err(vpu->ctx, "invalid venc vsi");
        return -EINVAL;
    }
    ...
}

Will this cause VCP encoder initialization to unconditionally fail?

Even though patch 05 ('media: mediatek: vcodec: get share memory address')
updates this function to switch on mem_type, won't the caller in venc_vpu_if.c
still pass a remote instance address (e.g., msg->vpu_inst_addr) rather than
the expected ENCODER_MEM enum value? This would hit the default case and
still return NULL at the end of the series.

[ ... ]

> @@ -530,7 +562,11 @@ struct mtk_vcodec_fw *mtk_vcodec_fw_vcp_init(void *priv, enum mtk_vcodec_fw_use
>  	struct mtk_vcodec_fw *fw;
>  	int i;
>  
> -	if (fw_use == DECODER) {
> +	if (fw_use == ENCODER) {
> +		struct mtk_vcodec_enc_dev *enc_dev = priv;
> +
> +		plat_dev = enc_dev->plat_dev;
> +	} else if (fw_use == DECODER) {
>  		struct mtk_vcodec_dec_dev *dec_dev = priv;
>  
>  		plat_dev = dec_dev->plat_dev;

[Severity: High]
Is the newly added VCP encoder initialization unreachable due to missing
device tree parsing?

While this patch adds VCP firmware support for the encoder, the encoder's
probe function only checks for mediatek,vpu and mediatek,scp:

drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_drv.c:mtk_vcodec_probe() {
    ...
    } else if (!of_property_read_u32(pdev->dev.of_node, "mediatek,scp",
                                     &rproc_phandle)) {
        fw_type = SCP;
        dev->fw_init = mtk_vcodec_fw_scp_init;
    } else {
        dev_err(&pdev->dev, "[MTK VCODEC] Could not get venc IPI device");
        return -ENODEV;
    }
    ...
}

Since mediatek,vcp is never checked, won't the driver return -ENODEV if an
encoder node specifies it? The assignment to mtk_vcodec_fw_vcp_init seems
to be missing from the encoder probe path.

This appears to remain unfixed through the end of the patch series.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923024258.257846-1-kyrie.wu@mediatek.com?part=3