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@xxxxxxxxxxxx>
> > ---
> > .../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@xxxxxxxxxxxx>
> > */
> >
> > +#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 {
>
>
