Re: [PATCH v2, 04/10] media: mtk-vcodec: Enable venc dual core usage

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxx>
---
   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





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux