Dear Angelo,
Thanks for the comments.
On Fri, 2022-06-24 at 11:00 +0200, AngeloGioacchino Del Regno wrote:
> Il 24/06/22 10:23, Irui Wang ha scritto:
> > Adds new property to indicate whether the encoder has multiple
> > cores.
> > Use of_platform_populate to probe each venc cores, the core device
> > can
> > use the init_clk/request_irq helper to initialize their own
> > power/clk/irq.
> >
> > Signed-off-by: Irui Wang <irui.wang@xxxxxxxxxxxx>
> > ---
> > .../media/platform/mediatek/vcodec/Makefile | 4 +-
> > .../platform/mediatek/vcodec/mtk_vcodec_drv.h | 12 ++
> > .../mediatek/vcodec/mtk_vcodec_enc_drv.c | 10 ++
> > .../mediatek/vcodec/mtk_vcodec_enc_hw.c | 139
> > ++++++++++++++++++
> > .../mediatek/vcodec/mtk_vcodec_enc_hw.h | 36 +++++
> > 5 files changed, 200 insertions(+), 1 deletion(-)
> > create mode 100644
> > drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc_hw.c
> > create mode 100644
> > drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc_hw.h
> >
> > diff --git a/drivers/media/platform/mediatek/vcodec/Makefile
> > b/drivers/media/platform/mediatek/vcodec/Makefile
> > index 93e7a343b5b0..ac068d88af29 100644
> > --- a/drivers/media/platform/mediatek/vcodec/Makefile
> > +++ b/drivers/media/platform/mediatek/vcodec/Makefile
> > @@ -3,7 +3,8 @@
> > obj-$(CONFIG_VIDEO_MEDIATEK_VCODEC) += mtk-vcodec-dec.o \
> > mtk-vcodec-enc.o \
> > mtk-vcodec-common.o \
> > - mtk-vcodec-dec-hw.o
> > + mtk-vcodec-dec-hw.o \
> > + mtk_vcodec_enc_hw.o
> >
> > mtk-vcodec-dec-y := vdec/vdec_h264_if.o \
> > vdec/vdec_vp8_if.o \
> > @@ -32,6 +33,7 @@ mtk-vcodec-enc-y := venc/venc_vp8_if.o \
> > venc_drv_if.o \
> > venc_vpu_if.o \
> >
> > +mtk-vcodec-enc-hw-y := mtk_vcodec_enc_hw.o
> >
> > mtk-vcodec-common-y := mtk_vcodec_intr.o \
> > mtk_vcodec_util.o \
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
> > b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
> > index dc6aada882d9..8919bdf2eef5 100644
> > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
> > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
> > @@ -97,6 +97,15 @@ enum mtk_fmt_type {
> > MTK_FMT_FRAME = 2,
> > };
> >
> > +/*
> > + * enum mtk_venc_hw_id -- encoder hardware id
> > + */
> > +enum mtk_venc_hw_id {
> > + MTK_VENC_CORE_0 = 0,
> > + MTK_VENC_CORE_1,
> > + MTK_VENC_HW_MAX,
> > +};
> > +
> > /*
> > * enum mtk_vdec_hw_id - Hardware index used to separate
> > * different hardware
> > @@ -484,6 +493,7 @@ struct mtk_vcodec_enc_pdata {
> > * @dec_active_cnt: used to mark whether need to record register
> > value
> > * @vdec_racing_info: record register value
> > * @dec_racing_info_mutex: mutex lock used for inner racing mode
> > + * @enc_hw_dev: used to store venc core device
> > */
> > struct mtk_vcodec_dev {
> > struct v4l2_device v4l2_dev;
> > @@ -534,6 +544,8 @@ struct mtk_vcodec_dev {
> > u32 vdec_racing_info[132];
> > /* Protects access to vdec_racing_info data */
> > struct mutex dec_racing_info_mutex;
> > +
> > + void *enc_hw_dev[MTK_VENC_HW_MAX];
> > };
> >
> > static inline struct mtk_vcodec_ctx *fh_to_ctx(struct v4l2_fh
> > *fh)
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc_drv.c
> > b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc_drv.c
> > index 95e8c29ccc65..65a8251a5a68 100644
> > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc_drv.c
> > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc_drv.c
> > @@ -263,6 +263,16 @@ static int mtk_vcodec_probe(struct
> > platform_device *pdev)
> > goto err_enc_pm;
> > }
> >
> > + if (of_property_read_bool(pdev->dev.of_node,
> > + "mediatek,venc-multi-core")) {
>
> You don't need this property here: just call of_platform_populate()
> unconditionally. If there's no child node, this function will do
> nothing
> so this conditional is useless and can be avoided.
I will update YAML and fix it in next version.
>
> > + ret = of_platform_populate(pdev->dev.of_node,
> > + NULL, NULL, &pdev->dev);
> > + if (ret) {
> > + mtk_v4l2_err("Venc core device populate
> > failed");
>
> What about "Failed to populate children devices" ?
fix it in next version.
>
> > + goto err_enc_pm;
> > + }
> > + }
> > +
> > pm_runtime_enable(&pdev->dev);
> >
> > dev->reg_base[dev->venc_pdata->core_id] =
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc_hw.c
> > b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc_hw.c
> > new file mode 100644
> > index 000000000000..02582cce4863
> > --- /dev/null
> > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc_hw.c
> > @@ -0,0 +1,139 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2021 MediaTek Inc.
> > + */
> > +
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/slab.h>
> > +
> > +#include "mtk_vcodec_drv.h"
> > +#include "mtk_vcodec_enc.h"
> > +#include "mtk_vcodec_enc_hw.h"
> > +#include "mtk_vcodec_intr.h"
> > +
> > +static const struct of_device_id mtk_venc_hw_ids[] = {
> > + {
> > + .compatible = "mediatek,mtk-venc-hw",
> > + },
>
> Please compress this in one line.
>
> > + {},
>
> Usually, we say that this is a sentinel.
>
> { .compatible = "mediatek,mtk-venc-hw" },
> { /* sentinel */ },
fix it in next version.
>
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_venc_hw_ids);
> > +
>
> ..snip..
>
> > +
> > +static int mtk_venc_hw_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct mtk_venc_hw_dev *sub_core;
> > + struct mtk_vcodec_dev *main_dev;
> > + int ret;
> > +
> > + if (!dev->parent)
> > + return dev_err_probe(dev, -ENODEV,
> > + "No parent for venc core
> > device\n");
> > +
> > + main_dev = dev_get_drvdata(dev->parent);
> > + if (!main_dev)
> > + return dev_err_probe(dev, -EINVAL,
> > + "Failed to get parent driver
> > data\n");
> > +
> > + sub_core = devm_kzalloc(&pdev->dev, sizeof(*sub_core),
> > GFP_KERNEL);
> > + if (!sub_core)
> > + return dev_err_probe(dev, -ENOMEM,
> > + "Failed to get alloc core
> > data\n");
> > +
> > + sub_core->plat_dev = pdev;
> > +
> > + platform_set_drvdata(pdev, sub_core);
> > +
> > + sub_core->reg_base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(sub_core->reg_base))
> > + return dev_err_probe(dev, PTR_ERR(sub_core->reg_base),
> > + "Failed to get reg base\n");
> > +
> > + sub_core->enc_irq = platform_get_irq(pdev, 0);
> > + if (sub_core->enc_irq < 0)
> > + return dev_err_probe(dev, -EINVAL,
> > + "Failed to get irq resource\n");
> > +
> > + ret = devm_request_irq(dev, sub_core->enc_irq,
> > + mtk_enc_hw_irq_handler, 0,
> > + pdev->name, sub_core);
> > + if (ret)
> > + return dev_err_probe(dev, -EINVAL,
> > + "Failed to install sub_core-
> > >enc_irq %d\n",
> > + sub_core->enc_irq);
> > +
> > + of_property_read_u32(dev->of_node, "mediatek,hw-id",
> > + &sub_core->hw_id);
> > +
>
> I'd do it like this, instead:
>
> ret = of_property_read_u32(dev->of_node, "mediatek,hw-id",
> &sub_core->hw_id);
> if (ret || sub_core->hw_id >= MTK_VENC_HW_MAX)
> return dev_err_probe(dev, (ret ? ret : -EINVAL),
> "Cannot parse hardware id");
>
> P.S.: you're reading an unsigned value from devicetree, this cannot
> ever be less
> than zero!
fix it in next version.
>
> > + if (sub_core->hw_id < 0 || sub_core->hw_id >= MTK_VENC_HW_MAX)
> > + return dev_err_probe(dev, -EINVAL,
> > + "Invalid hardware id %d\n",
> > + sub_core->hw_id);
> > +
> > + main_dev->enc_hw_dev[sub_core->hw_id] = sub_core;
> > + sub_core->main_dev = main_dev;
> > +
> > + dev_dbg(dev, "Venc core :%d probe done\n", sub_core->hw_id);
> > +
> > + return 0;
> > +}
> > +
> > +static struct platform_driver mtk_venc_core_driver = {
> > + .probe = mtk_venc_hw_probe,
> > + .driver = {
> > + .name = "mtk-venc-core",
> > + .of_match_table = mtk_venc_hw_ids,
> > + },
> > +};
> > +module_platform_driver(mtk_venc_core_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("MediaTek video encoder core driver");
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc_hw.h
> > b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc_hw.h
> > new file mode 100644
> > index 000000000000..0ff544c20eb9
> > --- /dev/null
> > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc_hw.h
> > @@ -0,0 +1,36 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2021 MediaTek Inc.
> > + */
> > +
> > +#ifndef _MTK_VCODEC_ENC_HW_H_
> > +#define _MTK_VCODEC_ENC_HW_H_
> > +
> > +#include <linux/platform_device.h>
> > +#include "mtk_vcodec_drv.h"
> > +
> > +/**
> > + * struct mtk_venc_hw_dev - driver data
> > + * @plat_dev: platform_device
> > + * @main_dev: main device
> > + * @pm: power management data
> > + * @curr_ctx: the context that is waiting for venc hardware
> > + * @reg_base: mapped address of venc registers
> > + * @irq_status: venc hardware irq status
> > + * @enc_irq: venc device irq
> > + * @hw_id: for venc hardware id: core#0, core#1...
> > + */
> > +struct mtk_venc_hw_dev {
> > + struct platform_device *plat_dev;
> > + struct mtk_vcodec_dev *main_dev;
> > +
> > + struct mtk_vcodec_pm pm;
> > + struct mtk_vcodec_ctx *curr_ctx;
> > +
> > + void __iomem *reg_base;
> > + unsigned int irq_status;
> > + int enc_irq;
> > + int hw_id;
>
> For consistency, this should be `enum mtk_venc_hw_id hw_id;`
fix it in next version.
>
> > +};
> > +
> > +#endif /* _MTK_VCODEC_ENC_HW_H_ */
>
> Cheers,
> Angelo
Thanks
Best Regards
