Re: [PATCH] media: mediatek: vcodec: using encoder device to alloc/free encoder memory

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 08/10/23 05:19, Yunfei Dong ha scritto:
> Need to use encoder device to allocate/free encoder memory when calling
> mtk_vcodec_mem_alloc/mtk_vcodec_mem_free, or leading to below crash log
> when test encoder with decoder device.
> 
> pc : dma_alloc_attrs+0x44/0xf4
> lr : mtk_vcodec_mem_alloc+0x50/0xa4 [mtk_vcodec_common]
> sp : ffffffc0209f3990
> x29: ffffffc0209f39a0 x28: ffffff8024102a18 x27: 0000000000000000
> x26: 0000000000000000 x25: ffffffc00c06e2d8 x24: 0000000000000001
> x23: 0000000000000cc0 x22: 0000000000000010 x21: 0000000000000800
> x20: ffffff8024102a18 x19: 0000000000000000 x18: 0000000000000000
> x17: 0000000000000009 x16: ffffffe389736a98 x15: 0000000000000078
> x14: ffffffe389704434 x13: 0000000000000007 x12: ffffffe38a2b2560
> x11: 0000000000000800 x10: 0000000000000004 x9 : ffffffe331f07484
> x8 : 5400e9aef2395000 x7 : 0000000000000000 x6 : 000000000000003f
> x5 : 0000000000000001 x4 : 0000000000000000 x3 : 0000000000000cc0
> x2 : ffffff8024102a18 x1 : 0000000000000800 x0 : 0000000000000010
> Call trace:
>   dma_alloc_attrs+0x44/0xf4
>   mtk_vcodec_mem_alloc+0x50/0xa4 [mtk_vcodec_common 2819d3d601f3cd06c1f2213ac1b9995134441421]
>   h264_enc_set_param+0x27c/0x378 [mtk_vcodec_enc 772cc3d26c254e8cf54079451ef8d930d2eb4404]
>   venc_if_set_param+0x4c/0x7c [mtk_vcodec_enc 772cc3d26c254e8cf54079451ef8d930d2eb4404]
>   vb2ops_venc_start_streaming+0x1bc/0x328 [mtk_vcodec_enc 772cc3d26c254e8cf54079451ef8d930d2eb4404]
>   vb2_start_streaming+0x64/0x12c
>   vb2_core_streamon+0x114/0x158
>   vb2_streamon+0x38/0x60
>   v4l2_m2m_streamon+0x48/0x88
>   v4l2_m2m_ioctl_streamon+0x20/0x2c
>   v4l_streamon+0x2c/0x38
>   __video_do_ioctl+0x2c4/0x3dc
>   video_usercopy+0x404/0x934
>   video_ioctl2+0x20/0x2c
>   v4l2_ioctl+0x54/0x64
>   v4l2_compat_ioctl32+0x90/0xa34
>   __arm64_compat_sys_ioctl+0x128/0x13c
>   invoke_syscall+0x4c/0x108
>   el0_svc_common+0x98/0x104
>   do_el0_svc_compat+0x28/0x34
>   el0_svc_compat+0x2c/0x74
>   el0t_32_sync_handler+0xa8/0xcc
>   el0t_32_sync+0x194/0x198
> Code: aa0003f6 aa0203f4 aa0103f5 f900
> 
> 'Fixes: 01abf5fbb081c ("media: mediatek: vcodec: separate struct 'mtk_vcodec_ctx'")'
> Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
> ---
>   .../mediatek/vcodec/common/mtk_vcodec_util.c  | 66 ++++++++++++++++++-
>   1 file changed, 64 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.c b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.c
> index 908602031fd0..62bb7290c56d 100644
> --- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.c
> +++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.c
> @@ -45,7 +45,7 @@ int mtk_vcodec_write_vdecsys(struct mtk_vcodec_dec_ctx *ctx, unsigned int reg,
>   }
>   EXPORT_SYMBOL(mtk_vcodec_write_vdecsys);
>   
> -int mtk_vcodec_mem_alloc(void *priv, struct mtk_vcodec_mem *mem)
> +static int mtk_vcodec_mem_dec_alloc(void *priv, struct mtk_vcodec_mem *mem)
>   {
>   	unsigned long size = mem->size;
>   	struct mtk_vcodec_dec_ctx *ctx = priv;
> @@ -64,9 +64,39 @@ int mtk_vcodec_mem_alloc(void *priv, struct mtk_vcodec_mem *mem)
>   
>   	return 0;
>   }
> +
> +static int mtk_vcodec_mem_enc_alloc(void *priv, struct mtk_vcodec_mem *mem)
> +{
> +	unsigned long size = mem->size;
> +	struct mtk_vcodec_enc_ctx *ctx = priv;
> +	struct device *dev = &ctx->dev->plat_dev->dev;
> +
> +	mem->va = dma_alloc_coherent(dev, size, &mem->dma_addr, GFP_KERNEL);
> +	if (!mem->va) {
> +		mtk_v4l2_venc_err(ctx, "%s dma_alloc size=%ld failed!", dev_name(dev), size);
> +		return -ENOMEM;
> +	}
> +
> +	mtk_v4l2_venc_dbg(3, ctx, "[%d]  - va      = %p", ctx->id, mem->va);
> +	mtk_v4l2_venc_dbg(3, ctx, "[%d]  - dma     = 0x%lx", ctx->id,
> +			  (unsigned long)mem->dma_addr);
> +	mtk_v4l2_venc_dbg(3, ctx, "[%d]    size = 0x%lx", ctx->id, size);
> +
> +	return 0;
> +}
> +
> +int mtk_vcodec_mem_alloc(void *priv, struct mtk_vcodec_mem *mem)
> +{
> +	enum mtk_instance_type inst_type = *((unsigned int *)priv);

I agree in that the first member of both mtk_vcodec_{enc,dec}_ctx is this
enum mtk_instance_type, but no, you're not passing that as priv: you're actually
passing one of the two structures, and this would add up complexity in human
readability, and would also open the road for mistakes.

You should at this point pass the instance type to the function.

int mtk_vcodec_mem_alloc(void *priv, enum mtk_instance_type /* or int, but I prefer 
enum */ inst_type, struct mtk_vcodec_mem *mem)

...and then, since the only difference between the two `alloc` function is just
only about debugging logs, you could commonize the allocation part... an idea:

struct platform_device *pdev;
unsigned long size;
int id;


if (inst_type == MTK_INST_DECODER) {
	struct mtk_vcodec_enc_ctx *enc_ctx = priv;
	pdev = enc_ctx->dev->plat_dev;
	id = enc_ctx->id;
} else {
	struct mtk_vcodec_dec_ctx *dec_ctx = priv;
	pdev = dec_ctx->dev->plat_dev;
	id = dec_ctx->id;
}

mem->va = dma_alloc_coherent(&pdev->dev, etc.....)
if (!mem->va) {
	mtk_v4l2_err(pdev, .....);
}

mtk_v4l2_debug(&pdev->dev, 3, "[%d]  - va      = %p", id, mem->va);

...you wouldn't even need one function for enc_alloc and one for dec_alloc, as
the logic would be pretty much commonized like this.

> +
> +	if (inst_type == MTK_INST_ENCODER)
> +		return mtk_vcodec_mem_enc_alloc(priv, mem);
> +	else
> +		return mtk_vcodec_mem_dec_alloc(priv, mem);
> +}
>   EXPORT_SYMBOL(mtk_vcodec_mem_alloc);
>   
> -void mtk_vcodec_mem_free(void *priv, struct mtk_vcodec_mem *mem)
> +static void mtk_vcodec_mem_dec_free(void *priv, struct mtk_vcodec_mem *mem)
>   {
>   	unsigned long size = mem->size;
>   	struct mtk_vcodec_dec_ctx *ctx = priv;
> @@ -87,6 +117,38 @@ void mtk_vcodec_mem_free(void *priv, struct mtk_vcodec_mem *mem)
>   	mem->dma_addr = 0;
>   	mem->size = 0;
>   }
> +
> +static void mtk_vcodec_mem_enc_free(void *priv, struct mtk_vcodec_mem *mem)
> +{
> +	unsigned long size = mem->size;
> +	struct mtk_vcodec_enc_ctx *ctx = priv;
> +	struct device *dev = &ctx->dev->plat_dev->dev;
> +
> +	if (!mem->va) {
> +		mtk_v4l2_venc_err(ctx, "%s dma_free size=%ld failed!", dev_name(dev), size);
> +		return;
> +	}
> +
> +	mtk_v4l2_venc_dbg(3, ctx, "[%d]  - va      = %p", ctx->id, mem->va);
> +	mtk_v4l2_venc_dbg(3, ctx, "[%d]  - dma     = 0x%lx", ctx->id,
> +			  (unsigned long)mem->dma_addr);
> +	mtk_v4l2_venc_dbg(3, ctx, "[%d]    size = 0x%lx", ctx->id, size);
> +
> +	dma_free_coherent(dev, size, mem->va, mem->dma_addr);
> +	mem->va = NULL;
> +	mem->dma_addr = 0;
> +	mem->size = 0;
> +}
> +
> +void mtk_vcodec_mem_free(void *priv, struct mtk_vcodec_mem *mem)
> +{
> +	enum mtk_instance_type inst_type = *((unsigned int *)priv);
> +
> +	if (inst_type == MTK_INST_ENCODER)
> +		mtk_vcodec_mem_enc_free(priv, mem);
> +	else
> +		mtk_vcodec_mem_dec_free(priv, mem);

same comments apply to the xxxx_free() function as well.
You're in the "common" scope here, so you wouldn't be forced to use the enc/dec
specific logging functions, as you can use the more generic mtk_v4l2_{debug,err}()
and mtk_vcodec_{debug,err}() functions here, which are *perfect* for the common
code.

Cheers,
Angelo