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

From: Irui Wang
Date: Sat Mar 12 2022 - 04:05:19 EST

Next message: Ard Biesheuvel: "Re: [PATCH 1/2] ARM: fix building NOMMU ARMv4/v5 kernels"
Previous message: Arnd Bergmann: "Re: [PATCH V7 13/20] riscv: compat: process: Add UXL_32 support in start_thread"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

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>;
};
};
};

Next message: Ard Biesheuvel: "Re: [PATCH 1/2] ARM: fix building NOMMU ARMv4/v5 kernels"
Previous message: Arnd Bergmann: "Re: [PATCH V7 13/20] riscv: compat: process: Add UXL_32 support in start_thread"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]