Adds new venc core mode to indicate different venc hardware mode:
VENC_SINGLE_CORE_MODE means only one core, the device has its own
power/clk/irq, init_clk/request_irq helper can be used.
VENC_DUAL_CORE_MODE means more than one core inside, the core device
can use the init_clk/request_irq helper to initialize their own
power/clk/irq. And the main device doesn't need use these helper anymore.
MT8195 has two H264 venc cores, enable dual_core_mode for it.
Signed-off-by: Irui Wang <irui.wang@mediatek.com>
---
drivers/media/platform/mtk-vcodec/Makefile | 4 +-
.../platform/mtk-vcodec/mtk_vcodec_drv.h | 22 +++
.../platform/mtk-vcodec/mtk_vcodec_enc_core.c | 153 ++++++++++++++++++
.../platform/mtk-vcodec/mtk_vcodec_enc_core.h | 36 +++++
.../platform/mtk-vcodec/mtk_vcodec_enc_drv.c | 88 +++++-----
5 files changed, 266 insertions(+), 37 deletions(-)
create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_core.c
create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_core.h
diff --git a/drivers/media/platform/mtk-vcodec/Makefile b/drivers/media/platform/mtk-vcodec/Makefile
index 93e7a343b5b0..c472b221bd6b 100644
--- a/drivers/media/platform/mtk-vcodec/Makefile
+++ b/drivers/media/platform/mtk-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-core.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-core-y := mtk_vcodec_enc_core.o
mtk-vcodec-common-y := mtk_vcodec_intr.o \
mtk_vcodec_util.o \
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
index f78463ff4551..9e4e4290a69a 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
@@ -117,6 +117,23 @@ enum mtk_vdec_hw_count {
MTK_VDEC_MAX_HW_COUNT,
};
+/*
+ * enum mtk_venc_core_id -- encoder core id
+ */
+enum mtk_venc_core_id {
+ MTK_VENC_CORE0 = 0,
+ MTK_VENC_CORE1 = 1,
+ MTK_VENC_CORE_MAX,
+};
+
+/**
+ * enmu mtk_venc_core_mode - Used to indicate different encode mode
+ */
+enum mtk_venc_core_mode {
+ VENC_SINGLE_CORE_MODE = 0,
+ VENC_DUAL_CORE_MODE = 1,
+};
+
/*
* struct mtk_video_fmt - Structure used to store information about pixelformats
*/
@@ -420,6 +437,7 @@ struct mtk_vcodec_dec_pdata {
* @output_formats: array of supported output formats
* @num_output_formats: number of entries in output_formats
* @core_type: stand for h264 or vp8 encode
+ * @core_mode: indicate encode core mode
*/
struct mtk_vcodec_enc_pdata {
bool uses_ext;
@@ -430,6 +448,7 @@ struct mtk_vcodec_enc_pdata {
const struct mtk_video_fmt *output_formats;
size_t num_output_formats;
int core_type;
+ enum mtk_venc_core_mode core_mode;
};
#define MTK_ENC_CTX_IS_EXT(ctx) ((ctx)->dev->venc_pdata->uses_ext)
@@ -479,6 +498,7 @@ struct mtk_vcodec_enc_pdata {
* @subdev_dev: subdev hardware device
* @subdev_prob_done: check whether all used hw device is prob done
* @subdev_bitmap: used to record hardware is ready or not
+ * @enc_core_dev: used to store venc core device
*/
struct mtk_vcodec_dev {
struct v4l2_device v4l2_dev;
@@ -524,6 +544,8 @@ struct mtk_vcodec_dev {
void *subdev_dev[MTK_VDEC_HW_MAX];
int (*subdev_prob_done)(struct mtk_vcodec_dev *vdec_dev);
DECLARE_BITMAP(subdev_bitmap, MTK_VDEC_HW_MAX);
+
+ void *enc_core_dev[MTK_VENC_CORE_MAX];
};
static inline struct mtk_vcodec_ctx *fh_to_ctx(struct v4l2_fh *fh)
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_core.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_core.c
new file mode 100644
index 000000000000..d84914f615a5
--- /dev/null
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_core.c
@@ -0,0 +1,153 @@
+// 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_core.h"
+
+static const struct of_device_id mtk_venc_core_ids[] = {
+ {
+ .compatible = "mediatek,mtk-venc-core0",
+ .data = (void *)MTK_VENC_CORE0,
+ },
+ {
+ .compatible = "mediatek,mtk-venc-core1",
+ .data = (void *)MTK_VENC_CORE1,
+ },
+ {},
+};
+MODULE_DEVICE_TABLE(of, mtk_venc_core_ids);
+
+static void clean_irq_status(unsigned int irq_status, void __iomem *addr)
+{
+ if (irq_status & MTK_VENC_IRQ_STATUS_PAUSE)
+ writel(MTK_VENC_IRQ_STATUS_PAUSE, addr);
+
+ if (irq_status & MTK_VENC_IRQ_STATUS_SWITCH)
+ writel(MTK_VENC_IRQ_STATUS_SWITCH, addr);
+
+ if (irq_status & MTK_VENC_IRQ_STATUS_DRAM)
+ writel(MTK_VENC_IRQ_STATUS_DRAM, addr);
+
+ if (irq_status & MTK_VENC_IRQ_STATUS_SPS)
+ writel(MTK_VENC_IRQ_STATUS_SPS, addr);
+
+ if (irq_status & MTK_VENC_IRQ_STATUS_PPS)
+ writel(MTK_VENC_IRQ_STATUS_PPS, addr);
+
+ if (irq_status & MTK_VENC_IRQ_STATUS_FRM)
+ writel(MTK_VENC_IRQ_STATUS_FRM, addr);
+}
+
+static irqreturn_t mtk_enc_core_irq_handler(int irq, void *priv)
+{
+ struct mtk_venc_core_dev *core = priv;
+ struct mtk_vcodec_ctx *ctx;
+ unsigned long flags;
+ void __iomem *addr;
+
+ spin_lock_irqsave(&core->main_dev->irqlock, flags);
+ ctx = core->curr_ctx;
+ spin_unlock_irqrestore(&core->main_dev->irqlock, flags);
+ if (!ctx)
+ return IRQ_HANDLED;
+
+ mtk_v4l2_debug(1, "id=%d core :%d", ctx->id, core->core_id);
+
+ addr = core->reg_base + MTK_VENC_IRQ_ACK_OFFSET;
+ ctx->irq_status = readl(core->reg_base + MTK_VENC_IRQ_STATUS_OFFSET);
+ clean_irq_status(ctx->irq_status, addr);
+
+ wake_up_ctx(ctx, MTK_INST_IRQ_RECEIVED, 0);
+ return IRQ_HANDLED;
+}
+
+static int mtk_venc_core_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct mtk_venc_core_dev *core;
+ struct mtk_vcodec_dev *main_dev;
+ int ret;
+
+ if (!dev->parent) {
+ dev_err(dev, "No parent for venc core device\n");
+ return -ENODEV;
+ }
+
+ main_dev = dev_get_drvdata(dev->parent);
+ if (!main_dev) {
+ dev_err(dev, "Failed to get parent driver data");
+ return -EINVAL;
+ }
+
+ core = devm_kzalloc(&pdev->dev, sizeof(*core), GFP_KERNEL);
+ if (!core)
+ return -ENOMEM;
+
+ core->plat_dev = pdev;
+
+ core->reg_base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(core->reg_base)) {
+ dev_err(&pdev->dev, "Failed to get reg base");
+ ret = PTR_ERR(core->reg_base);
+ goto err;
+ }
+
+ core->enc_irq = platform_get_irq(pdev, 0);
+ if (core->enc_irq < 0) {
+ dev_err(&pdev->dev, "Failed to get irq resource");
+ ret = core->enc_irq;
+ goto err;
+ }
+
+ ret = devm_request_irq(&pdev->dev, core->enc_irq,
+ mtk_enc_core_irq_handler, 0,
+ pdev->name, core);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "Failed to install core->enc_irq %d (%d)",
+ core->enc_irq, ret);
+ ret = -EINVAL;
+ goto err;
+ }
+
+ core->core_id =
+ (enum mtk_venc_core_id)of_device_get_match_data(&pdev->dev);
+ if (core->core_id < 0 || core->core_id >= MTK_VENC_CORE_MAX) {
+ ret = -EINVAL;
+ goto err;
+ }
+
+ main_dev->enc_core_dev[core->core_id] = core;
+ core->main_dev = main_dev;
+
+ platform_set_drvdata(pdev, core);
+
+ dev_info(dev, "Venc core :%d probe done\n", core->core_id);
+
+ return 0;
+
+err:
+ return ret;
+}
+
+static struct platform_driver mtk_venc_core_driver = {
+ .probe = mtk_venc_core_probe,
+ .driver = {
+ .name = "mtk-venc-core",
+ .of_match_table = mtk_venc_core_ids,
+ },
+};
+module_platform_driver(mtk_venc_core_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Mediatek video encoder core driver");
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_core.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_core.h
new file mode 100644
index 000000000000..856681989869
--- /dev/null
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_core.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2021 MediaTek Inc.
+ */
+
+#ifndef _MTK_VCODEC_ENC_CORE_H_
+#define _MTK_VCODEC_ENC_CORE_H_
+
+#include <linux/platform_device.h>
+#include "mtk_vcodec_drv.h"
+
+/*
+ * struct mtk_venc_core_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 core irq status
+ * @enc_irq: venc device irq
+ * @core id: for venc core id: core#0, core#1...
+ */
+struct mtk_venc_core_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 core_id;
+};
+
+#endif /* _MTK_VCODEC_ENC_CORE_H_ */
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c
index 1f950310a3fb..9d48c604bc54 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c
@@ -231,10 +231,9 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
{
struct mtk_vcodec_dev *dev;
struct video_device *vfd_enc;
- struct resource *res;
phandle rproc_phandle;
enum mtk_vcodec_fw_type fw_type;
- int ret;
+ int ret, core_type;
dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
if (!dev)
@@ -260,39 +259,37 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
return PTR_ERR(dev->fw_handler);
dev->venc_pdata = of_device_get_match_data(&pdev->dev);
- ret = mtk_vcodec_init_enc_clk(dev);
- if (ret < 0) {
- dev_err(&pdev->dev, "Failed to get mtk vcodec clock source!");
- goto err_enc_pm;
- }
+ core_type = dev->venc_pdata->core_type;
- pm_runtime_enable(&pdev->dev);
+ if (dev->venc_pdata->core_mode == VENC_SINGLE_CORE_MODE) {
+ ret = mtk_vcodec_init_enc_clk(dev);
+ if (ret < 0) {
+ dev_err(&pdev->dev,
+ "Failed to get mtk vcodec clock source!");
+ goto err_enc_pm;
+ }
- dev->reg_base[dev->venc_pdata->core_type] =
- devm_platform_ioremap_resource(pdev, 0);
- if (IS_ERR(dev->reg_base[dev->venc_pdata->core_type])) {
- ret = PTR_ERR(dev->reg_base[dev->venc_pdata->core_type]);
- goto err_res;
- }
+ pm_runtime_enable(&pdev->dev);
- res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
- if (res == NULL) {
- dev_err(&pdev->dev, "failed to get irq resource");
- ret = -ENOENT;
- goto err_res;
- }
+ dev->reg_base[core_type] =
+ devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(dev->reg_base[core_type])) {
+ ret = PTR_ERR(dev->reg_base[core_type]);
+ goto err_res;
+ }
- dev->enc_irq = platform_get_irq(pdev, 0);
- irq_set_status_flags(dev->enc_irq, IRQ_NOAUTOEN);
- ret = devm_request_irq(&pdev->dev, dev->enc_irq,
- mtk_vcodec_enc_irq_handler,
- 0, pdev->name, dev);
- if (ret) {
- dev_err(&pdev->dev,
- "Failed to install dev->enc_irq %d (%d) core_type (%d)",
- dev->enc_irq, ret, dev->venc_pdata->core_type);
- ret = -EINVAL;
- goto err_res;
+ dev->enc_irq = platform_get_irq(pdev, 0);
+ irq_set_status_flags(dev->enc_irq, IRQ_NOAUTOEN);
+ ret = devm_request_irq(&pdev->dev, dev->enc_irq,
+ mtk_vcodec_enc_irq_handler,
+ 0, pdev->name, dev);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "Failed to install irq %d (%d) core_type (%d)",
+ dev->enc_irq, ret, core_type);
+ ret = -EINVAL;
+ goto err_res;
+ }
}
mutex_init(&dev->enc_mutex);
@@ -358,8 +355,17 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
goto err_enc_reg;
}
+ if (dev->venc_pdata->core_mode == VENC_DUAL_CORE_MODE) {
+ ret = of_platform_populate(pdev->dev.of_node,
+ NULL, NULL, &pdev->dev);
+ if (ret) {
+ mtk_v4l2_err("Venc main device populate failed");
+ goto err_enc_reg;
+ }
+ }
+
mtk_v4l2_debug(0, "encoder %d registered as /dev/video%d",
- dev->venc_pdata->core_type, vfd_enc->num);
+ core_type, vfd_enc->num);
return 0;
@@ -372,8 +378,10 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
err_enc_alloc:
v4l2_device_unregister(&dev->v4l2_dev);
err_res:
- pm_runtime_disable(dev->pm.dev);
- put_device(dev->pm.larbvenc);
+ if (dev->venc_pdata->core_mode == VENC_SINGLE_CORE_MODE) {
+ pm_runtime_disable(dev->pm.dev);
+ put_device(dev->pm.larbvenc);
+ }
err_enc_pm:
mtk_vcodec_fw_release(dev->fw_handler);
return ret;
@@ -387,6 +395,7 @@ static const struct mtk_vcodec_enc_pdata mt8173_avc_pdata = {
.min_bitrate = 64,
.max_bitrate = 60000000,
.core_type = VENC_SYS,
+ .core_mode = VENC_SINGLE_CORE_MODE,
};
static const struct mtk_vcodec_enc_pdata mt8173_vp8_pdata = {
@@ -397,6 +406,7 @@ static const struct mtk_vcodec_enc_pdata mt8173_vp8_pdata = {
.min_bitrate = 64,
.max_bitrate = 9000000,
.core_type = VENC_LT_SYS,
+ .core_mode = VENC_SINGLE_CORE_MODE,
};
static const struct mtk_vcodec_enc_pdata mt8183_pdata = {
@@ -408,6 +418,7 @@ static const struct mtk_vcodec_enc_pdata mt8183_pdata = {
.min_bitrate = 64,
.max_bitrate = 40000000,
.core_type = VENC_SYS,
+ .core_mode = VENC_SINGLE_CORE_MODE,
};
static const struct mtk_vcodec_enc_pdata mt8192_pdata = {
@@ -419,6 +430,7 @@ static const struct mtk_vcodec_enc_pdata mt8192_pdata = {
.min_bitrate = 64,
.max_bitrate = 100000000,
.core_type = VENC_SYS,
+ .core_mode = VENC_SINGLE_CORE_MODE,
};
static const struct mtk_vcodec_enc_pdata mt8195_pdata = {
@@ -430,6 +442,7 @@ static const struct mtk_vcodec_enc_pdata mt8195_pdata = {
.min_bitrate = 64,
.max_bitrate = 100000000,
.core_type = VENC_SYS,
+ .core_mode = VENC_DUAL_CORE_MODE,
};
static const struct of_device_id mtk_vcodec_enc_match[] = {
@@ -457,8 +470,11 @@ static int mtk_vcodec_enc_remove(struct platform_device *pdev)
video_unregister_device(dev->vfd_enc);
v4l2_device_unregister(&dev->v4l2_dev);
- pm_runtime_disable(dev->pm.dev);
- put_device(dev->pm.larbvenc);
+
+ if (dev->venc_pdata->core_mode == VENC_SINGLE_CORE_MODE) {
+ pm_runtime_disable(dev->pm.dev);
+ put_device(dev->pm.larbvenc);
+ }
mtk_vcodec_fw_release(dev->fw_handler);
return 0;
}
--
2.18.0
Il 17/01/22 13:06, Irui Wang ha scritto: > Adds new venc core mode to indicate different venc hardware mode: > VENC_SINGLE_CORE_MODE means only one core, the device has its own > power/clk/irq, init_clk/request_irq helper can be used. > > VENC_DUAL_CORE_MODE means more than one core inside, the core device > can use the init_clk/request_irq helper to initialize their own > power/clk/irq. And the main device doesn't need use these helper anymore. > > MT8195 has two H264 venc cores, enable dual_core_mode for it. > > Signed-off-by: Irui Wang <irui.wang@mediatek.com> > --- > drivers/media/platform/mtk-vcodec/Makefile | 4 +- > .../platform/mtk-vcodec/mtk_vcodec_drv.h | 22 +++ > .../platform/mtk-vcodec/mtk_vcodec_enc_core.c | 153 ++++++++++++++++++ > .../platform/mtk-vcodec/mtk_vcodec_enc_core.h | 36 +++++ > .../platform/mtk-vcodec/mtk_vcodec_enc_drv.c | 88 +++++----- > 5 files changed, 266 insertions(+), 37 deletions(-) > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_core.c > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_core.h > > diff --git a/drivers/media/platform/mtk-vcodec/Makefile b/drivers/media/platform/mtk-vcodec/Makefile > index 93e7a343b5b0..c472b221bd6b 100644 > --- a/drivers/media/platform/mtk-vcodec/Makefile > +++ b/drivers/media/platform/mtk-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-core.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-core-y := mtk_vcodec_enc_core.o > > mtk-vcodec-common-y := mtk_vcodec_intr.o \ > mtk_vcodec_util.o \ > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > index f78463ff4551..9e4e4290a69a 100644 > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > @@ -117,6 +117,23 @@ enum mtk_vdec_hw_count { > MTK_VDEC_MAX_HW_COUNT, > }; > > +/* > + * enum mtk_venc_core_id -- encoder core id > + */ > +enum mtk_venc_core_id { > + MTK_VENC_CORE0 = 0, > + MTK_VENC_CORE1 = 1, You don't have to say "= 1" for core1, just... MTK_VENC_CORE0 = 0, MTK_VENC_CORE1, ...is fine, and better. > + MTK_VENC_CORE_MAX, > +}; > + > +/** > + * enmu mtk_venc_core_mode - Used to indicate different encode mode > + */ > +enum mtk_venc_core_mode { > + VENC_SINGLE_CORE_MODE = 0, > + VENC_DUAL_CORE_MODE = 1, > +}; > + > /* > * struct mtk_video_fmt - Structure used to store information about pixelformats > */ > @@ -420,6 +437,7 @@ struct mtk_vcodec_dec_pdata { > * @output_formats: array of supported output formats > * @num_output_formats: number of entries in output_formats > * @core_type: stand for h264 or vp8 encode > + * @core_mode: indicate encode core mode > */ > struct mtk_vcodec_enc_pdata { > bool uses_ext; > @@ -430,6 +448,7 @@ struct mtk_vcodec_enc_pdata { > const struct mtk_video_fmt *output_formats; > size_t num_output_formats; > int core_type; > + enum mtk_venc_core_mode core_mode; > }; > > #define MTK_ENC_CTX_IS_EXT(ctx) ((ctx)->dev->venc_pdata->uses_ext) > @@ -479,6 +498,7 @@ struct mtk_vcodec_enc_pdata { > * @subdev_dev: subdev hardware device > * @subdev_prob_done: check whether all used hw device is prob done > * @subdev_bitmap: used to record hardware is ready or not > + * @enc_core_dev: used to store venc core device > */ > struct mtk_vcodec_dev { > struct v4l2_device v4l2_dev; > @@ -524,6 +544,8 @@ struct mtk_vcodec_dev { > void *subdev_dev[MTK_VDEC_HW_MAX]; > int (*subdev_prob_done)(struct mtk_vcodec_dev *vdec_dev); > DECLARE_BITMAP(subdev_bitmap, MTK_VDEC_HW_MAX); > + > + void *enc_core_dev[MTK_VENC_CORE_MAX]; > }; > > static inline struct mtk_vcodec_ctx *fh_to_ctx(struct v4l2_fh *fh) > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_core.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_core.c > new file mode 100644 > index 000000000000..d84914f615a5 > --- /dev/null > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_core.c > @@ -0,0 +1,153 @@ > +// 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_core.h" > + > +static const struct of_device_id mtk_venc_core_ids[] = { > + { > + .compatible = "mediatek,mtk-venc-core0", > + .data = (void *)MTK_VENC_CORE0, > + }, > + { > + .compatible = "mediatek,mtk-venc-core1", > + .data = (void *)MTK_VENC_CORE1, > + }, > + {}, > +}; Hello Irui, You don't need a different compatible for the different cores, as in the declaration, there's nothing special that differentiates them that much. I understand that there may be a need to differentiate the core number, as in, CORE0 always has to be the leader, while CORE1 would be the follower, but this is not a good reason to give them a different compatible string. I want to make you aware that Kyrie Wu did the same thing as you did here and in my review on his patch I was able to give an extensive example of how this should look; the exactly same logic would apply to this patch. Please have a look here: https://patchwork.kernel.org/comment/24726607/ P.S.: In short, you should have only one "mediatek,mtk-venc-hw" compatible used for probing both cores. > +MODULE_DEVICE_TABLE(of, mtk_venc_core_ids); > + > +static void clean_irq_status(unsigned int irq_status, void __iomem *addr) > +{ > + if (irq_status & MTK_VENC_IRQ_STATUS_PAUSE) > + writel(MTK_VENC_IRQ_STATUS_PAUSE, addr); > + > + if (irq_status & MTK_VENC_IRQ_STATUS_SWITCH) > + writel(MTK_VENC_IRQ_STATUS_SWITCH, addr); > + > + if (irq_status & MTK_VENC_IRQ_STATUS_DRAM) > + writel(MTK_VENC_IRQ_STATUS_DRAM, addr); > + > + if (irq_status & MTK_VENC_IRQ_STATUS_SPS) > + writel(MTK_VENC_IRQ_STATUS_SPS, addr); > + > + if (irq_status & MTK_VENC_IRQ_STATUS_PPS) > + writel(MTK_VENC_IRQ_STATUS_PPS, addr); > + > + if (irq_status & MTK_VENC_IRQ_STATUS_FRM) > + writel(MTK_VENC_IRQ_STATUS_FRM, addr); > +} > + > +static irqreturn_t mtk_enc_core_irq_handler(int irq, void *priv) > +{ > + struct mtk_venc_core_dev *core = priv; > + struct mtk_vcodec_ctx *ctx; > + unsigned long flags; > + void __iomem *addr; > + > + spin_lock_irqsave(&core->main_dev->irqlock, flags); > + ctx = core->curr_ctx; > + spin_unlock_irqrestore(&core->main_dev->irqlock, flags); > + if (!ctx) > + return IRQ_HANDLED; > + > + mtk_v4l2_debug(1, "id=%d core :%d", ctx->id, core->core_id); > + > + addr = core->reg_base + MTK_VENC_IRQ_ACK_OFFSET; > + ctx->irq_status = readl(core->reg_base + MTK_VENC_IRQ_STATUS_OFFSET); > + clean_irq_status(ctx->irq_status, addr); > + > + wake_up_ctx(ctx, MTK_INST_IRQ_RECEIVED, 0); > + return IRQ_HANDLED; > +} > + > +static int mtk_venc_core_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct mtk_venc_core_dev *core; > + struct mtk_vcodec_dev *main_dev; > + int ret; > + > + if (!dev->parent) { > + dev_err(dev, "No parent for venc core device\n"); > + return -ENODEV; > + } Please, use dev_err_probe(), here and everywhere else. > + > + main_dev = dev_get_drvdata(dev->parent); > + if (!main_dev) { > + dev_err(dev, "Failed to get parent driver data"); > + return -EINVAL; > + } > + > + core = devm_kzalloc(&pdev->dev, sizeof(*core), GFP_KERNEL); > + if (!core) > + return -ENOMEM; > + > + core->plat_dev = pdev; > + > + core->reg_base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(core->reg_base)) { > + dev_err(&pdev->dev, "Failed to get reg base"); > + ret = PTR_ERR(core->reg_base); > + goto err; You don't need the "err" label. Also, you can return dev_err_probe here too. Example: return dev_err_probe(&pdev->dev, PTR_ERR(core->reg_base), "Failed to get reg base\n"); > + } > + > + core->enc_irq = platform_get_irq(pdev, 0); > + if (core->enc_irq < 0) { > + dev_err(&pdev->dev, "Failed to get irq resource"); > + ret = core->enc_irq; > + goto err; > + } > + > + ret = devm_request_irq(&pdev->dev, core->enc_irq, > + mtk_enc_core_irq_handler, 0, > + pdev->name, core); > + if (ret) { > + dev_err(&pdev->dev, > + "Failed to install core->enc_irq %d (%d)", > + core->enc_irq, ret); > + ret = -EINVAL; > + goto err; > + } > + > + core->core_id = core_id would be 0 if "mediatek,hw-leader", N if it's not a mediatek,hw-leader. > + (enum mtk_venc_core_id)of_device_get_match_data(&pdev->dev); > + if (core->core_id < 0 || core->core_id >= MTK_VENC_CORE_MAX) { > + ret = -EINVAL; > + goto err; > + } > + > + main_dev->enc_core_dev[core->core_id] = core; > + core->main_dev = main_dev; > + > + platform_set_drvdata(pdev, core); > + > + dev_info(dev, "Venc core :%d probe done\n", core->core_id); Please change this to dev_dbg(). > + > + return 0; > + > +err: > + return ret; > +} > + > +static struct platform_driver mtk_venc_core_driver = { > + .probe = mtk_venc_core_probe, > + .driver = { > + .name = "mtk-venc-core", > + .of_match_table = mtk_venc_core_ids, > + }, > +}; > +module_platform_driver(mtk_venc_core_driver); > + > +MODULE_LICENSE("GPL v2"); > +MODULE_DESCRIPTION("Mediatek video encoder core driver"); > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_core.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_core.h > new file mode 100644 > index 000000000000..856681989869 > --- /dev/null > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_core.h > @@ -0,0 +1,36 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (c) 2021 MediaTek Inc. > + */ > + > +#ifndef _MTK_VCODEC_ENC_CORE_H_ > +#define _MTK_VCODEC_ENC_CORE_H_ > + > +#include <linux/platform_device.h> > +#include "mtk_vcodec_drv.h" > + > +/* I love when developers try to write kerneldoc, this is something that should really be done *always*, so thank you for this!!!! ....but you missed something here: for this to be parsed by kerneldoc, you need to do: /** ^^^ this one :)) * struct mtk_venc_core_dev - driver data P.S.: https://docs.kernel.org/doc-guide/kernel-doc.html > + * struct mtk_venc_core_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 core irq status > + * @enc_irq: venc device irq > + * @core id: for venc core id: core#0, core#1... > + */ > +struct mtk_venc_core_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 core_id; > +}; > + > +#endif /* _MTK_VCODEC_ENC_CORE_H_ */ Thanks, Angelo
Hello, Angelo, Many thanks for your review. On Thu, 2022-03-03 at 15:27 +0100, AngeloGioacchino Del Regno wrote: > Il 17/01/22 13:06, Irui Wang ha scritto: > > Adds new venc core mode to indicate different venc hardware mode: > > VENC_SINGLE_CORE_MODE means only one core, the device has its own > > power/clk/irq, init_clk/request_irq helper can be used. > > > > VENC_DUAL_CORE_MODE means more than one core inside, the core > > device > > can use the init_clk/request_irq helper to initialize their own > > power/clk/irq. And the main device doesn't need use these helper > > anymore. > > > > MT8195 has two H264 venc cores, enable dual_core_mode for it. > > > > Signed-off-by: Irui Wang <irui.wang@mediatek.com> > > --- > > drivers/media/platform/mtk-vcodec/Makefile | 4 +- > > .../platform/mtk-vcodec/mtk_vcodec_drv.h | 22 +++ > > .../platform/mtk-vcodec/mtk_vcodec_enc_core.c | 153 > > ++++++++++++++++++ > > .../platform/mtk-vcodec/mtk_vcodec_enc_core.h | 36 +++++ > > .../platform/mtk-vcodec/mtk_vcodec_enc_drv.c | 88 +++++----- > > 5 files changed, 266 insertions(+), 37 deletions(-) > > create mode 100644 drivers/media/platform/mtk- > > vcodec/mtk_vcodec_enc_core.c > > create mode 100644 drivers/media/platform/mtk- > > vcodec/mtk_vcodec_enc_core.h > > > > diff --git a/drivers/media/platform/mtk-vcodec/Makefile > > b/drivers/media/platform/mtk-vcodec/Makefile > > index 93e7a343b5b0..c472b221bd6b 100644 > > --- a/drivers/media/platform/mtk-vcodec/Makefile > > +++ b/drivers/media/platform/mtk-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-core.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-core-y := mtk_vcodec_enc_core.o > > > > mtk-vcodec-common-y := mtk_vcodec_intr.o \ > > mtk_vcodec_util.o \ > > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > > index f78463ff4551..9e4e4290a69a 100644 > > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > > @@ -117,6 +117,23 @@ enum mtk_vdec_hw_count { > > MTK_VDEC_MAX_HW_COUNT, > > }; > > > > +/* > > + * enum mtk_venc_core_id -- encoder core id > > + */ > > +enum mtk_venc_core_id { > > + MTK_VENC_CORE0 = 0, > > + MTK_VENC_CORE1 = 1, > > You don't have to say "= 1" for core1, just... > > MTK_VENC_CORE0 = 0, > MTK_VENC_CORE1, > > ...is fine, and better. I will fix it. > > > + MTK_VENC_CORE_MAX, > > +}; > > + > > +/** > > + * enmu mtk_venc_core_mode - Used to indicate different encode > > mode > > + */ > > +enum mtk_venc_core_mode { > > + VENC_SINGLE_CORE_MODE = 0, > > + VENC_DUAL_CORE_MODE = 1, > > +}; > > + > > /* > > * struct mtk_video_fmt - Structure used to store information > > about pixelformats > > */ > > @@ -420,6 +437,7 @@ struct mtk_vcodec_dec_pdata { > > * @output_formats: array of supported output formats > > * @num_output_formats: number of entries in output_formats > > * @core_type: stand for h264 or vp8 encode > > + * @core_mode: indicate encode core mode > > */ > > struct mtk_vcodec_enc_pdata { > > bool uses_ext; > > @@ -430,6 +448,7 @@ struct mtk_vcodec_enc_pdata { > > const struct mtk_video_fmt *output_formats; > > size_t num_output_formats; > > int core_type; > > + enum mtk_venc_core_mode core_mode; > > }; > > > > #define MTK_ENC_CTX_IS_EXT(ctx) ((ctx)->dev->venc_pdata- > > >uses_ext) > > @@ -479,6 +498,7 @@ struct mtk_vcodec_enc_pdata { > > * @subdev_dev: subdev hardware device > > * @subdev_prob_done: check whether all used hw device is prob > > done > > * @subdev_bitmap: used to record hardware is ready or not > > + * @enc_core_dev: used to store venc core device > > */ > > struct mtk_vcodec_dev { > > struct v4l2_device v4l2_dev; > > @@ -524,6 +544,8 @@ struct mtk_vcodec_dev { > > void *subdev_dev[MTK_VDEC_HW_MAX]; > > int (*subdev_prob_done)(struct mtk_vcodec_dev *vdec_dev); > > DECLARE_BITMAP(subdev_bitmap, MTK_VDEC_HW_MAX); > > + > > + void *enc_core_dev[MTK_VENC_CORE_MAX]; > > }; > > > > static inline struct mtk_vcodec_ctx *fh_to_ctx(struct v4l2_fh > > *fh) > > diff --git a/drivers/media/platform/mtk- > > vcodec/mtk_vcodec_enc_core.c b/drivers/media/platform/mtk- > > vcodec/mtk_vcodec_enc_core.c > > new file mode 100644 > > index 000000000000..d84914f615a5 > > --- /dev/null > > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_core.c > > @@ -0,0 +1,153 @@ > > +// 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_core.h" > > + > > +static const struct of_device_id mtk_venc_core_ids[] = { > > + { > > + .compatible = "mediatek,mtk-venc-core0", > > + .data = (void *)MTK_VENC_CORE0, > > + }, > > + { > > + .compatible = "mediatek,mtk-venc-core1", > > + .data = (void *)MTK_VENC_CORE1, > > + }, > > + {}, > > +}; > > Hello Irui, > > You don't need a different compatible for the different cores, as in > the > declaration, there's nothing special that differentiates them that > much. > > I understand that there may be a need to differentiate the core > number, as > in, CORE0 always has to be the leader, while CORE1 would be the > follower, > but this is not a good reason to give them a different compatible > string. > > I want to make you aware that Kyrie Wu did the same thing as you did > here > and in my review on his patch I was able to give an extensive example > of > how this should look; the exactly same logic would apply to this > patch. > > Please have a look here: > https://patchwork.kernel.org/comment/24726607/ > > P.S.: In short, you should have only one "mediatek,mtk-venc-hw" > compatible > used for probing both cores. thanks for your suggestions, with your example, venc can be rewritten like this: venc { compatible = "mediatek,mt8195-vcodec-enc"; ..... other properties ..... venc_core0 { compatible = "mediatek,mtk-venc-hw"; mediatek,hw-leader;//mediatek,venc-core0; ..... other properties ..... }; venc_core1 { compatible = "mediatek,mtk-venc-hw"; //mediatek,venc-core1; ..... other properties ..... }; }; I will rewrite this code if it matches your suggestions. > > > +MODULE_DEVICE_TABLE(of, mtk_venc_core_ids); > > + > > +static void clean_irq_status(unsigned int irq_status, void __iomem > > *addr) > > +{ > > + if (irq_status & MTK_VENC_IRQ_STATUS_PAUSE) > > + writel(MTK_VENC_IRQ_STATUS_PAUSE, addr); > > + > > + if (irq_status & MTK_VENC_IRQ_STATUS_SWITCH) > > + writel(MTK_VENC_IRQ_STATUS_SWITCH, addr); > > + > > + if (irq_status & MTK_VENC_IRQ_STATUS_DRAM) > > + writel(MTK_VENC_IRQ_STATUS_DRAM, addr); > > + > > + if (irq_status & MTK_VENC_IRQ_STATUS_SPS) > > + writel(MTK_VENC_IRQ_STATUS_SPS, addr); > > + > > + if (irq_status & MTK_VENC_IRQ_STATUS_PPS) > > + writel(MTK_VENC_IRQ_STATUS_PPS, addr); > > + > > + if (irq_status & MTK_VENC_IRQ_STATUS_FRM) > > + writel(MTK_VENC_IRQ_STATUS_FRM, addr); > > +} > > + > > +static irqreturn_t mtk_enc_core_irq_handler(int irq, void *priv) > > +{ > > + struct mtk_venc_core_dev *core = priv; > > + struct mtk_vcodec_ctx *ctx; > > + unsigned long flags; > > + void __iomem *addr; > > + > > + spin_lock_irqsave(&core->main_dev->irqlock, flags); > > + ctx = core->curr_ctx; > > + spin_unlock_irqrestore(&core->main_dev->irqlock, flags); > > + if (!ctx) > > + return IRQ_HANDLED; > > + > > + mtk_v4l2_debug(1, "id=%d core :%d", ctx->id, core->core_id); > > + > > + addr = core->reg_base + MTK_VENC_IRQ_ACK_OFFSET; > > + ctx->irq_status = readl(core->reg_base + > > MTK_VENC_IRQ_STATUS_OFFSET); > > + clean_irq_status(ctx->irq_status, addr); > > + > > + wake_up_ctx(ctx, MTK_INST_IRQ_RECEIVED, 0); > > + return IRQ_HANDLED; > > +} > > + > > +static int mtk_venc_core_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct mtk_venc_core_dev *core; > > + struct mtk_vcodec_dev *main_dev; > > + int ret; > > + > > + if (!dev->parent) { > > + dev_err(dev, "No parent for venc core device\n"); > > + return -ENODEV; > > + } > > Please, use dev_err_probe(), here and everywhere else. I will fix it > > > + > > + main_dev = dev_get_drvdata(dev->parent); > > + if (!main_dev) { > > + dev_err(dev, "Failed to get parent driver data"); > > + return -EINVAL; > > + } > > + > > + core = devm_kzalloc(&pdev->dev, sizeof(*core), GFP_KERNEL); > > + if (!core) > > + return -ENOMEM; > > + > > + core->plat_dev = pdev; > > + > > + core->reg_base = devm_platform_ioremap_resource(pdev, 0); > > + if (IS_ERR(core->reg_base)) { > > + dev_err(&pdev->dev, "Failed to get reg base"); > > + ret = PTR_ERR(core->reg_base); > > + goto err; > > You don't need the "err" label. Also, you can return dev_err_probe > here too. > Example: > > return dev_err_probe(&pdev->dev, PTR_ERR(core- > >reg_base), > "Failed to get reg base\n"); I will fix it > > > + } > > + > > + core->enc_irq = platform_get_irq(pdev, 0); > > + if (core->enc_irq < 0) { > > + dev_err(&pdev->dev, "Failed to get irq resource"); > > + ret = core->enc_irq; > > + goto err; > > + } > > + > > + ret = devm_request_irq(&pdev->dev, core->enc_irq, > > + mtk_enc_core_irq_handler, 0, > > + pdev->name, core); > > + if (ret) { > > + dev_err(&pdev->dev, > > + "Failed to install core->enc_irq %d (%d)", > > + core->enc_irq, ret); > > + ret = -EINVAL; > > + goto err; > > + } > > + > > + core->core_id = > > core_id would be 0 if "mediatek,hw-leader", N if it's not a > mediatek,hw-leader. > > > + (enum mtk_venc_core_id)of_device_get_match_data(&pdev- > > >dev); > > + if (core->core_id < 0 || core->core_id >= MTK_VENC_CORE_MAX) { > > + ret = -EINVAL; > > + goto err; > > + } > > + > > + main_dev->enc_core_dev[core->core_id] = core; > > + core->main_dev = main_dev; > > + > > + platform_set_drvdata(pdev, core); > > + > > + dev_info(dev, "Venc core :%d probe done\n", core->core_id); > > Please change this to dev_dbg(). I will fix it > > > + > > + return 0; > > + > > +err: > > + return ret; > > +} > > + > > +static struct platform_driver mtk_venc_core_driver = { > > + .probe = mtk_venc_core_probe, > > + .driver = { > > + .name = "mtk-venc-core", > > + .of_match_table = mtk_venc_core_ids, > > + }, > > +}; > > +module_platform_driver(mtk_venc_core_driver); > > + > > +MODULE_LICENSE("GPL v2"); > > +MODULE_DESCRIPTION("Mediatek video encoder core driver"); > > diff --git a/drivers/media/platform/mtk- > > vcodec/mtk_vcodec_enc_core.h b/drivers/media/platform/mtk- > > vcodec/mtk_vcodec_enc_core.h > > new file mode 100644 > > index 000000000000..856681989869 > > --- /dev/null > > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_core.h > > @@ -0,0 +1,36 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Copyright (c) 2021 MediaTek Inc. > > + */ > > + > > +#ifndef _MTK_VCODEC_ENC_CORE_H_ > > +#define _MTK_VCODEC_ENC_CORE_H_ > > + > > +#include <linux/platform_device.h> > > +#include "mtk_vcodec_drv.h" > > + > > +/* > > I love when developers try to write kerneldoc, this is something that > should really be done *always*, so thank you for this!!!! > > ....but you missed something here: for this to be parsed by > kerneldoc, > you need to do: > > /** > ^^^ this one :)) > > * struct mtk_venc_core_dev - driver data > > P.S.: https://docs.kernel.org/doc-guide/kernel-doc.html I will fix it > > > + * struct mtk_venc_core_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 core irq status > > + * @enc_irq: venc device irq > > + * @core id: for venc core id: core#0, core#1... > > + */ > > +struct mtk_venc_core_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 core_id; > > +}; > > + > > +#endif /* _MTK_VCODEC_ENC_CORE_H_ */ > > > Thanks, > Angelo Thanks Best Regards
Il 04/03/22 03:12, Irui Wang ha scritto: > Hello, Angelo, > > Many thanks for your review. > > On Thu, 2022-03-03 at 15:27 +0100, AngeloGioacchino Del Regno wrote: >> Il 17/01/22 13:06, Irui Wang ha scritto: >>> Adds new venc core mode to indicate different venc hardware mode: >>> VENC_SINGLE_CORE_MODE means only one core, the device has its own >>> power/clk/irq, init_clk/request_irq helper can be used. >>> >>> VENC_DUAL_CORE_MODE means more than one core inside, the core >>> device >>> can use the init_clk/request_irq helper to initialize their own >>> power/clk/irq. And the main device doesn't need use these helper >>> anymore. >>> >>> MT8195 has two H264 venc cores, enable dual_core_mode for it. >>> >>> Signed-off-by: Irui Wang <irui.wang@mediatek.com> >>> --- >>> drivers/media/platform/mtk-vcodec/Makefile | 4 +- >>> .../platform/mtk-vcodec/mtk_vcodec_drv.h | 22 +++ >>> .../platform/mtk-vcodec/mtk_vcodec_enc_core.c | 153 >>> ++++++++++++++++++ >>> .../platform/mtk-vcodec/mtk_vcodec_enc_core.h | 36 +++++ >>> .../platform/mtk-vcodec/mtk_vcodec_enc_drv.c | 88 +++++----- >>> 5 files changed, 266 insertions(+), 37 deletions(-) >>> create mode 100644 drivers/media/platform/mtk- >>> vcodec/mtk_vcodec_enc_core.c >>> create mode 100644 drivers/media/platform/mtk- >>> vcodec/mtk_vcodec_enc_core.h >>> >>> diff --git a/drivers/media/platform/mtk-vcodec/Makefile >>> b/drivers/media/platform/mtk-vcodec/Makefile >>> index 93e7a343b5b0..c472b221bd6b 100644 >>> --- a/drivers/media/platform/mtk-vcodec/Makefile >>> +++ b/drivers/media/platform/mtk-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-core.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-core-y := mtk_vcodec_enc_core.o >>> >>> mtk-vcodec-common-y := mtk_vcodec_intr.o \ >>> mtk_vcodec_util.o \ >>> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h >>> b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h >>> index f78463ff4551..9e4e4290a69a 100644 >>> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h >>> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h >>> @@ -117,6 +117,23 @@ enum mtk_vdec_hw_count { >>> MTK_VDEC_MAX_HW_COUNT, >>> }; >>> >>> +/* >>> + * enum mtk_venc_core_id -- encoder core id >>> + */ >>> +enum mtk_venc_core_id { >>> + MTK_VENC_CORE0 = 0, >>> + MTK_VENC_CORE1 = 1, >> >> You don't have to say "= 1" for core1, just... >> >> MTK_VENC_CORE0 = 0, >> MTK_VENC_CORE1, >> >> ...is fine, and better. > > I will fix it. > >> >>> + MTK_VENC_CORE_MAX, >>> +}; >>> + >>> +/** >>> + * enmu mtk_venc_core_mode - Used to indicate different encode >>> mode >>> + */ >>> +enum mtk_venc_core_mode { >>> + VENC_SINGLE_CORE_MODE = 0, >>> + VENC_DUAL_CORE_MODE = 1, >>> +}; >>> + >>> /* >>> * struct mtk_video_fmt - Structure used to store information >>> about pixelformats >>> */ >>> @@ -420,6 +437,7 @@ struct mtk_vcodec_dec_pdata { >>> * @output_formats: array of supported output formats >>> * @num_output_formats: number of entries in output_formats >>> * @core_type: stand for h264 or vp8 encode >>> + * @core_mode: indicate encode core mode >>> */ >>> struct mtk_vcodec_enc_pdata { >>> bool uses_ext; >>> @@ -430,6 +448,7 @@ struct mtk_vcodec_enc_pdata { >>> const struct mtk_video_fmt *output_formats; >>> size_t num_output_formats; >>> int core_type; >>> + enum mtk_venc_core_mode core_mode; >>> }; >>> >>> #define MTK_ENC_CTX_IS_EXT(ctx) ((ctx)->dev->venc_pdata- >>>> uses_ext) >>> @@ -479,6 +498,7 @@ struct mtk_vcodec_enc_pdata { >>> * @subdev_dev: subdev hardware device >>> * @subdev_prob_done: check whether all used hw device is prob >>> done >>> * @subdev_bitmap: used to record hardware is ready or not >>> + * @enc_core_dev: used to store venc core device >>> */ >>> struct mtk_vcodec_dev { >>> struct v4l2_device v4l2_dev; >>> @@ -524,6 +544,8 @@ struct mtk_vcodec_dev { >>> void *subdev_dev[MTK_VDEC_HW_MAX]; >>> int (*subdev_prob_done)(struct mtk_vcodec_dev *vdec_dev); >>> DECLARE_BITMAP(subdev_bitmap, MTK_VDEC_HW_MAX); >>> + >>> + void *enc_core_dev[MTK_VENC_CORE_MAX]; >>> }; >>> >>> static inline struct mtk_vcodec_ctx *fh_to_ctx(struct v4l2_fh >>> *fh) >>> diff --git a/drivers/media/platform/mtk- >>> vcodec/mtk_vcodec_enc_core.c b/drivers/media/platform/mtk- >>> vcodec/mtk_vcodec_enc_core.c >>> new file mode 100644 >>> index 000000000000..d84914f615a5 >>> --- /dev/null >>> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_core.c >>> @@ -0,0 +1,153 @@ >>> +// 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_core.h" >>> + >>> +static const struct of_device_id mtk_venc_core_ids[] = { >>> + { >>> + .compatible = "mediatek,mtk-venc-core0", >>> + .data = (void *)MTK_VENC_CORE0, >>> + }, >>> + { >>> + .compatible = "mediatek,mtk-venc-core1", >>> + .data = (void *)MTK_VENC_CORE1, >>> + }, >>> + {}, >>> +}; >> >> Hello Irui, >> >> You don't need a different compatible for the different cores, as in >> the >> declaration, there's nothing special that differentiates them that >> much. >> >> I understand that there may be a need to differentiate the core >> number, as >> in, CORE0 always has to be the leader, while CORE1 would be the >> follower, >> but this is not a good reason to give them a different compatible >> string. >> >> I want to make you aware that Kyrie Wu did the same thing as you did >> here >> and in my review on his patch I was able to give an extensive example >> of >> how this should look; the exactly same logic would apply to this >> patch. >> >> Please have a look here: >> https://patchwork.kernel.org/comment/24726607/ >> >> P.S.: In short, you should have only one "mediatek,mtk-venc-hw" >> compatible >> used for probing both cores. > > thanks for your suggestions, with your example, venc can be rewritten > like this: > venc { > compatible = "mediatek,mt8195-vcodec-enc"; > ..... other properties ..... > > venc_core0 { > compatible = "mediatek,mtk-venc-hw"; > mediatek,hw-leader;//mediatek,venc-core0; > ..... other properties ..... > }; > > venc_core1 { > compatible = "mediatek,mtk-venc-hw"; > //mediatek,venc-core1; > ..... other properties ..... > }; > }; > I will rewrite this code if it matches your suggestions. Yes, exactly. Just one nit, please don't use underscores. venc_core0: venc-hw@(addr) this is fine ^ venc_core0: venc_hw@(addr) this is NOT ok ^^ By the way, one (or more than one) of the commits in this series is not working correctly, giving a kernel panic on dma mem alloc. Looking forward to see the new version! Regards, Angelo
Hello, Angelo, thanks for your review comments. I will update my new device tree, example inline: On Fri, 2022-03-04 at 10:07 +0100, AngeloGioacchino Del Regno wrote: > Il 04/03/22 03:12, Irui Wang ha scritto: > > Hello, Angelo, > > > > Many thanks for your review. > > > > On Thu, 2022-03-03 at 15:27 +0100, AngeloGioacchino Del Regno > > wrote: > > > Il 17/01/22 13:06, Irui Wang ha scritto: > > > > Adds new venc core mode to indicate different venc hardware > > > > mode: > > > > VENC_SINGLE_CORE_MODE means only one core, the device has its > > > > own > > > > power/clk/irq, init_clk/request_irq helper can be used. > > > > > > > > VENC_DUAL_CORE_MODE means more than one core inside, the core > > > > device > > > > can use the init_clk/request_irq helper to initialize their own > > > > power/clk/irq. And the main device doesn't need use these > > > > helper > > > > anymore. > > > > > > > > MT8195 has two H264 venc cores, enable dual_core_mode for it. > > > > > > > > Signed-off-by: Irui Wang <irui.wang@mediatek.com> > > > > --- > > > > drivers/media/platform/mtk-vcodec/Makefile | 4 +- > > > > .../platform/mtk-vcodec/mtk_vcodec_drv.h | 22 +++ > > > > .../platform/mtk-vcodec/mtk_vcodec_enc_core.c | 153 > > > > ++++++++++++++++++ > > > > .../platform/mtk-vcodec/mtk_vcodec_enc_core.h | 36 +++++ > > > > .../platform/mtk-vcodec/mtk_vcodec_enc_drv.c | 88 +++++--- > > > > -- > > > > 5 files changed, 266 insertions(+), 37 deletions(-) > > > > create mode 100644 drivers/media/platform/mtk- > > > > vcodec/mtk_vcodec_enc_core.c > > > > create mode 100644 drivers/media/platform/mtk- > > > > vcodec/mtk_vcodec_enc_core.h > > > > > > > > diff --git a/drivers/media/platform/mtk-vcodec/Makefile > > > > b/drivers/media/platform/mtk-vcodec/Makefile > > > > index 93e7a343b5b0..c472b221bd6b 100644 > > > > --- a/drivers/media/platform/mtk-vcodec/Makefile > > > > +++ b/drivers/media/platform/mtk-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-core.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-core-y := mtk_vcodec_enc_core.o > > > > > > > > mtk-vcodec-common-y := mtk_vcodec_intr.o \ > > > > mtk_vcodec_util.o \ > > > > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > > > > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > > > > index f78463ff4551..9e4e4290a69a 100644 > > > > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > > > > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > > > > @@ -117,6 +117,23 @@ enum mtk_vdec_hw_count { > > > > MTK_VDEC_MAX_HW_COUNT, > > > > }; > > > > > > > > +/* > > > > + * enum mtk_venc_core_id -- encoder core id > > > > + */ > > > > +enum mtk_venc_core_id { > > > > + MTK_VENC_CORE0 = 0, > > > > + MTK_VENC_CORE1 = 1, > > > > > > You don't have to say "= 1" for core1, just... > > > > > > MTK_VENC_CORE0 = 0, > > > MTK_VENC_CORE1, > > > > > > ...is fine, and better. > > > > I will fix it. > > > > > > > > > + MTK_VENC_CORE_MAX, > > > > +}; > > > > + > > > > +/** > > > > + * enmu mtk_venc_core_mode - Used to indicate different encode > > > > mode > > > > + */ > > > > +enum mtk_venc_core_mode { > > > > + VENC_SINGLE_CORE_MODE = 0, > > > > + VENC_DUAL_CORE_MODE = 1, > > > > +}; > > > > + > > > > /* > > > > * struct mtk_video_fmt - Structure used to store > > > > information > > > > about pixelformats > > > > */ > > > > @@ -420,6 +437,7 @@ struct mtk_vcodec_dec_pdata { > > > > * @output_formats: array of supported output formats > > > > * @num_output_formats: number of entries in output_formats > > > > * @core_type: stand for h264 or vp8 encode > > > > + * @core_mode: indicate encode core mode > > > > */ > > > > struct mtk_vcodec_enc_pdata { > > > > bool uses_ext; > > > > @@ -430,6 +448,7 @@ struct mtk_vcodec_enc_pdata { > > > > const struct mtk_video_fmt *output_formats; > > > > size_t num_output_formats; > > > > int core_type; > > > > + enum mtk_venc_core_mode core_mode; > > > > }; > > > > > > > > #define MTK_ENC_CTX_IS_EXT(ctx) ((ctx)->dev->venc_pdata- > > > > > uses_ext) > > > > > > > > @@ -479,6 +498,7 @@ struct mtk_vcodec_enc_pdata { > > > > * @subdev_dev: subdev hardware device > > > > * @subdev_prob_done: check whether all used hw device is > > > > prob > > > > done > > > > * @subdev_bitmap: used to record hardware is ready or not > > > > + * @enc_core_dev: used to store venc core device > > > > */ > > > > struct mtk_vcodec_dev { > > > > struct v4l2_device v4l2_dev; > > > > @@ -524,6 +544,8 @@ struct mtk_vcodec_dev { > > > > void *subdev_dev[MTK_VDEC_HW_MAX]; > > > > int (*subdev_prob_done)(struct mtk_vcodec_dev > > > > *vdec_dev); > > > > DECLARE_BITMAP(subdev_bitmap, MTK_VDEC_HW_MAX); > > > > + > > > > + void *enc_core_dev[MTK_VENC_CORE_MAX]; > > > > }; > > > > > > > > static inline struct mtk_vcodec_ctx *fh_to_ctx(struct > > > > v4l2_fh > > > > *fh) > > > > diff --git a/drivers/media/platform/mtk- > > > > vcodec/mtk_vcodec_enc_core.c b/drivers/media/platform/mtk- > > > > vcodec/mtk_vcodec_enc_core.c > > > > new file mode 100644 > > > > index 000000000000..d84914f615a5 > > > > --- /dev/null > > > > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_core.c > > > > @@ -0,0 +1,153 @@ > > > > +// 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_core.h" > > > > + > > > > +static const struct of_device_id mtk_venc_core_ids[] = { > > > > + { > > > > + .compatible = "mediatek,mtk-venc-core0", > > > > + .data = (void *)MTK_VENC_CORE0, > > > > + }, > > > > + { > > > > + .compatible = "mediatek,mtk-venc-core1", > > > > + .data = (void *)MTK_VENC_CORE1, > > > > + }, > > > > + {}, > > > > +}; > > > > > > Hello Irui, > > > > > > You don't need a different compatible for the different cores, as > > > in > > > the > > > declaration, there's nothing special that differentiates them > > > that > > > much. > > > > > > I understand that there may be a need to differentiate the core > > > number, as > > > in, CORE0 always has to be the leader, while CORE1 would be the > > > follower, > > > but this is not a good reason to give them a different compatible > > > string. > > > > > > I want to make you aware that Kyrie Wu did the same thing as you > > > did > > > here > > > and in my review on his patch I was able to give an extensive > > > example > > > of > > > how this should look; the exactly same logic would apply to this > > > patch. > > > > > > Please have a look here: > > > https://patchwork.kernel.org/comment/24726607/ > > > > > > P.S.: In short, you should have only one "mediatek,mtk-venc-hw" > > > compatible > > > used for probing both cores. > > > > thanks for your suggestions, with your example, venc can be > > rewritten > > like this: > > venc { > > compatible = "mediatek,mt8195-vcodec-enc"; > > ..... other properties ..... > > > > venc_core0 { > > compatible = "mediatek,mtk-venc-hw"; > > mediatek,hw-leader;//mediatek,venc-core0; > > ..... other properties ..... > > }; > > > > venc_core1 { > > compatible = "mediatek,mtk-venc-hw"; > > //mediatek,venc-core1; > > ..... other properties ..... > > }; > > }; > > I will rewrite this code if it matches your suggestions. > > Yes, exactly. Just one nit, please don't use underscores. > > venc_core0: venc-hw@(addr) > this is fine ^ > > venc_core0: venc_hw@(addr) > this is NOT ok ^^ > > By the way, one (or more than one) of the commits in this series > is not working correctly, giving a kernel panic on dma mem alloc. > > Looking forward to see the new version! > > Regards, > Angelo We add a new property "mediatek,core-id" to indicite current encoder core id. if we enable the two encoder cores, it will need more dma mem buffers for encoding, in patch-08 add a new venc-vsi to store these encoding working buffers, both kernel and user side need update the venc-vsi, otherwise, we will get a panic on mem alloc. There are still some encoding issues after apply the device tree, we still working on it, and we will send a new version patches once wesolve them. Thanks Best Regards example dtsi soc { #address-cells = <2>; #size-cells = <2>; venc { compatible = "mediatek,mt8195-vcodec-enc"; mediatek,scp = <&scp>; iommus = <&iommu_vdo M4U_PORT_L19_VENC_RCPU>; #address-cells = <2>; #size-cells = <2>; ranges; dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; venc-core@1a020000 { compatible = "mediatek,mtk-venc-core"; reg = <0 0x1a020000 0 0x10000>; mediatek,core-id = <0>; iommus = <&iommu_vdo M4U_PORT_L19_VENC_RCPU>, <&iommu_vdo M4U_PORT_L19_VENC_REC>, <&iommu_vdo M4U_PORT_L19_VENC_BSDMA>, <&iommu_vdo M4U_PORT_L19_VENC_SV_COMV>, <&iommu_vdo M4U_PORT_L19_VENC_RD_COMV>, <&iommu_vdo M4U_PORT_L19_VENC_CUR_LUMA>, <&iommu_vdo M4U_PORT_L19_VENC_CUR_CHROMA>, <&iommu_vdo M4U_PORT_L19_VENC_REF_LUMA>, <&iommu_vdo M4U_PORT_L19_VENC_REF_CHROMA>; interrupts = <GIC_SPI 341 IRQ_TYPE_LEVEL_HIGH 0>; clocks = <&vencsys CLK_VENC_VENC>; clock-names = "clk_venc"; assigned-clocks = <&topckgen CLK_TOP_VENC>; assigned-clock-parents = <&topckgen CLK_TOP_UNIVPLL_D4>; power-domains = <&spm MT8195_POWER_DOMAIN_VENC>; }; venc-core@1b020000 { compatible = "mediatek,mtk-venc-core"; reg = <0 0x1b020000 0 0x10000>; mediatek,core-id = <1>; iommus = <&iommu_vpp M4U_PORT_L20_VENC_RCPU>, <&iommu_vpp M4U_PORT_L20_VENC_REC>, <&iommu_vpp M4U_PORT_L20_VENC_BSDMA>, <&iommu_vpp M4U_PORT_L20_VENC_SV_COMV>, <&iommu_vpp M4U_PORT_L20_VENC_RD_COMV>, <&iommu_vpp M4U_PORT_L20_VENC_CUR_LUMA>, <&iommu_vpp M4U_PORT_L20_VENC_CUR_CHROMA>, <&iommu_vpp M4U_PORT_L20_VENC_REF_LUMA>, <&iommu_vpp M4U_PORT_L20_VENC_REF_CHROMA>; interrupts = <GIC_SPI 346 IRQ_TYPE_LEVEL_HIGH 0>; clocks = <&vencsys_core1 CLK_VENC_CORE1_VENC>; clock-names = "clk_venc_core1"; assigned-clocks = <&topckgen CLK_TOP_VENC>; assigned-clock-parents = <&topckgen CLK_TOP_UNIVPLL_D4>; power-domains = <&spm MT8195_POWER_DOMAIN_VENC_CORE1>; }; }; };
© 2016 - 2024 Red Hat, Inc.