Re: [PATCH v6,12/24] media: mediatek: vcodec: add interface to allocate/free secure memory

["Yunfei Dong (董云飞)" <Yunfei.Dong@mediatek.com>]

Hi Andrzej,

Thanks for your help to review this patch.
On Wed, 2024-05-22 at 14:25 +0200, Andrzej Pietrasiewicz wrote:
> Hi Yunfei,
> 
> W dniu 16.05.2024 o 14:20, Yunfei Dong pisze:
> > Need to call dma heap interface to allocate/free secure memory when
> > playing
> > secure video.
> > 
> > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
> > ---
> >   .../media/platform/mediatek/vcodec/Kconfig    |   1 +
> >   .../mediatek/vcodec/common/mtk_vcodec_util.c  | 122
> > +++++++++++++++++-
> >   .../mediatek/vcodec/common/mtk_vcodec_util.h  |   3 +
> >   3 files changed, 123 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/platform/mediatek/vcodec/Kconfig
> > b/drivers/media/platform/mediatek/vcodec/Kconfig
> > index bc8292232530..707865703e61 100644
> > --- a/drivers/media/platform/mediatek/vcodec/Kconfig
> > +++ b/drivers/media/platform/mediatek/vcodec/Kconfig
> > @@ -17,6 +17,7 @@ config VIDEO_MEDIATEK_VCODEC
> >   	depends on VIDEO_MEDIATEK_VPU || !VIDEO_MEDIATEK_VPU
> >   	depends on MTK_SCP || !MTK_SCP
> >   	depends on MTK_SMI || (COMPILE_TEST && MTK_SMI=n)
> > +	depends on DMABUF_HEAPS
> >   	select VIDEOBUF2_DMA_CONTIG
> >   	select V4L2_MEM2MEM_DEV
> >   	select VIDEO_MEDIATEK_VCODEC_VPU if VIDEO_MEDIATEK_VPU
> > 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 c60e4c193b25..5958dcd7965a 100644
> > ---
> > a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.c
> > +++
> > b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.c
> > @@ -5,9 +5,11 @@
> >   *	Tiffany Lin <tiffany.lin@mediatek.com>
> >   */
> >   
> > +#include <linux/dma-heap.h>
> >   #include <linux/module.h>
> >   #include <linux/of.h>
> >   #include <linux/regmap.h>
> > +#include <uapi/linux/dma-heap.h>
> >   
> >   #include "../decoder/mtk_vcodec_dec_drv.h"
> >   #include "../encoder/mtk_vcodec_enc_drv.h"
> > @@ -45,7 +47,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_alloc_nor(void *priv, struct
> > mtk_vcodec_mem *mem)
> >   {
> >   	enum mtk_instance_type inst_type = *((unsigned int *)priv);
> >   	struct platform_device *plat_dev;
> > @@ -75,9 +77,71 @@ int mtk_vcodec_mem_alloc(void *priv, struct
> > mtk_vcodec_mem *mem)
> >   
> >   	return 0;
> >   }
> > -EXPORT_SYMBOL(mtk_vcodec_mem_alloc);
> >   
> > -void mtk_vcodec_mem_free(void *priv, struct mtk_vcodec_mem *mem)
> > +static int mtk_vcodec_mem_alloc_sec(struct mtk_vcodec_dec_ctx
> > *ctx, struct mtk_vcodec_mem *mem)
> > +{
> > +	struct device *dev = &ctx->dev->plat_dev->dev;
> > +	struct dma_buf *dma_buffer;
> > +	struct dma_heap *vdec_heap;
> > +	struct dma_buf_attachment *attach;
> > +	struct sg_table *sgt;
> > +	unsigned long size = mem->size;
> > +	int ret = 0;
> > +
> > +	if (!size)
> > +		return -EINVAL;
> > +
> > +	vdec_heap = dma_heap_find("restricted_mtk_cma");
> > +	if (!vdec_heap) {
> > +		mtk_v4l2_vdec_err(ctx, "dma heap find failed!");
> > +		return -EPERM;
> > +	}
> > +
> > +	dma_buffer = dma_heap_buffer_alloc(vdec_heap, size,
> > DMA_HEAP_VALID_FD_FLAGS,
> > +					   DMA_HEAP_VALID_HEAP_FLAGS);
> > +	if (IS_ERR_OR_NULL(dma_buffer)) {
> > +		mtk_v4l2_vdec_err(ctx, "dma heap alloc size=0x%lx
> > failed!", size);
> > +		return PTR_ERR(dma_buffer);
> > +	}
> > +
> > +	attach = dma_buf_attach(dma_buffer, dev);
> > +	if (IS_ERR_OR_NULL(attach)) {
> > +		mtk_v4l2_vdec_err(ctx, "dma attach size=0x%lx failed!",
> > size);
> > +		ret = PTR_ERR(attach);
> > +		goto err_attach;
> > +	}
> > +
> > +	sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL);
> > +	if (IS_ERR_OR_NULL(sgt)) {
> > +		mtk_v4l2_vdec_err(ctx, "dma map attach size=0x%lx
> > failed!", size);
> > +		ret = PTR_ERR(sgt);
> > +		goto err_sgt;
> > +	}
> > +
> > +	mem->va = dma_buffer;
> > +	mem->dma_addr = (dma_addr_t)sg_dma_address((sgt)->sgl);
> > +
> > +	if (!mem->va || !mem->dma_addr) {
> > +		mtk_v4l2_vdec_err(ctx, "dma buffer size=0x%lx failed!",
> > size);
> > +		ret = -EPERM;
> > +		goto err_addr;
> > +	}
> > +
> > +	mem->attach = attach;
> > +	mem->sgt = sgt;
> > +
> > +	return 0;
> > +err_addr:
> > +	dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL);
> > +err_sgt:
> > +	dma_buf_detach(dma_buffer, attach);
> > +err_attach:
> > +	dma_buf_put(dma_buffer);
> > +
> > +	return ret;
> > +}
> > +
> > +static void mtk_vcodec_mem_free_nor(void *priv, struct
> > mtk_vcodec_mem *mem)
> >   {
> >   	enum mtk_instance_type inst_type = *((unsigned int *)priv);
> >   	struct platform_device *plat_dev;
> > @@ -110,6 +174,57 @@ void mtk_vcodec_mem_free(void *priv, struct
> > mtk_vcodec_mem *mem)
> >   	mem->dma_addr = 0;
> >   	mem->size = 0;
> >   }
> > +
> > +static void mtk_vcodec_mem_free_sec(struct mtk_vcodec_mem *mem)
> > +{
> > +	if (mem->sgt)
> > +		dma_buf_unmap_attachment(mem->attach, mem->sgt,
> > DMA_BIDIRECTIONAL);
> 
> is (!mem->sgt) possible at all here?
> 
> In mtk_vcodec_mem_alloc_sec() "if (IS_ERR_OR_NULL(sgt))" triggers an
> error recovery path and the allocation fails. Do you ever try to
> free_sec()
> a failed allocation?

Looks using 'if (!IS_ERR_OR_NULL(mem->sgt))' is much more reasonable.

Best Regards,
Yunfei Dong
> 
> > +	dma_buf_detach((struct dma_buf *)mem->va, mem->attach);
> > +	dma_buf_put((struct dma_buf *)mem->va);
> > +
> > +	mem->attach = NULL;
> > +	mem->sgt = NULL;
> > +	mem->va = NULL;
> > +	mem->dma_addr = 0;
> > +	mem->size = 0;
> > +}
> > +
> > +int mtk_vcodec_mem_alloc(void *priv, struct mtk_vcodec_mem *mem)
> > +{
> > +	enum mtk_instance_type inst_type = *((unsigned int *)priv);
> > +	int ret;
> > +
> > +	if (inst_type == MTK_INST_DECODER) {
> > +		struct mtk_vcodec_dec_ctx *dec_ctx = priv;
> > +
> > +		if (dec_ctx->is_secure_playback) {
> > +			ret = mtk_vcodec_mem_alloc_sec(dec_ctx, mem);
> > +			goto alloc_end;
> > +		}
> > +	}
> > +
> > +	ret = mtk_vcodec_mem_alloc_nor(priv, mem);
> > +alloc_end:
> > +
> 
> again maybe it's just my personal preference, but I'd have no goto
> (not because goto is prohibited, but because maybe it's not really
> justified here), fewer curly braces and no label:
> 
> int mtk_vcodec_mem_alloc(void *priv, struct mtk_vcodec_mem *mem)
> {
> 	struct mtk_vcodec_dec_ctx *dec_ctx = priv;
> 
> 	if (dec_ctx->inst_type == MTK_INST_DECODER && dec_ctx-
> >is_secure_playback)
> 		return mtk_vcodec_mem_alloc_sec(dec_ctx, mem);
> 
> 	return mtk_vcodec_mem_alloc_nor(priv, mem);
> }
> 
> To me it makes no sense to cast priv to inst_type _and_ to dec_ctx
> given that dec_ctx's first member _is_ inst_type.
> 
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL(mtk_vcodec_mem_alloc);
> > +
> > +void mtk_vcodec_mem_free(void *priv, struct mtk_vcodec_mem *mem)
> > +{
> > +	enum mtk_instance_type inst_type = *((unsigned int *)priv);
> 
> 
> ditto here.
> 
> Regards,
> 
> Andrzej
> 
> > +
> > +	if (inst_type == MTK_INST_DECODER) {
> > +		struct mtk_vcodec_dec_ctx *dec_ctx = priv;
> > +
> > +		if (dec_ctx->is_secure_playback) {
> > +			mtk_vcodec_mem_free_sec(mem);
> > +			return;
> > +		}
> > +	}
> > +
> > +	mtk_vcodec_mem_free_nor(priv, mem);
> > +}
> >   EXPORT_SYMBOL(mtk_vcodec_mem_free);
> >   
> >   void *mtk_vcodec_get_hw_dev(struct mtk_vcodec_dec_dev *dev, int
> > hw_idx)
> > @@ -171,3 +286,4 @@ EXPORT_SYMBOL(mtk_vcodec_get_curr_ctx);
> >   
> >   MODULE_LICENSE("GPL v2");
> >   MODULE_DESCRIPTION("Mediatek video codec driver");
> > +MODULE_IMPORT_NS(DMA_BUF);
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.h
> > b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.h
> > index 85f615cdd4d3..22078e757ed0 100644
> > ---
> > a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.h
> > +++
> > b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.h
> > @@ -18,6 +18,9 @@ struct mtk_vcodec_mem {
> >   	size_t size;
> >   	void *va;
> >   	dma_addr_t dma_addr;
> > +
> > +	struct dma_buf_attachment *attach;
> > +	struct sg_table *sgt;
> >   };
> >   
> >   struct mtk_vcodec_fb {
> 
>