by AngeloGioacchino Del Regno

[permalink] [raw]

Subject: Re: [PATCH v2, 03/10] dt-bindings: media: mtk-vcodec: Adds encoder cores dt-bindings for mt8195

Il 17/01/22 13:06, Irui Wang ha scritto:
> Adds encoder cores dt-bindings for mt8195
>
> Signed-off-by: Irui Wang <[email protected]>
> ---
> .../media/mediatek,vcodec-encoder-core.yaml | 214 ++++++++++++++++++
> 1 file changed, 214 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/mediatek,vcodec-encoder-core.yaml
>
> diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-encoder-core.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-encoder-core.yaml
> new file mode 100644
> index 000000000000..d1e7bfa50bce
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-encoder-core.yaml
> @@ -0,0 +1,214 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/media/mediatek,vcodec-encoder-core.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Mediatek Video Encoder Accelerator With Multi Core
> +
> +maintainers:
> + - Irui Wang <[email protected]>
> +
> +description: |
> + Mediatek Video Encode is the video encode hardware present in Mediatek
> + SoCs which supports high resolution encoding functionalities. Required
> + parent and child device node.
> +
> +properties:
> + compatible:
> + const: mediatek,mt8195-vcodec-enc
> +
> + mediatek,scp:
> + $ref: /schemas/types.yaml#/definitions/phandle
> + maxItems: 1
> + description: |
> + The node of system control processor (SCP), using
> + the remoteproc & rpmsg framework.
> +
> + iommus:
> + minItems: 1
> + maxItems: 32
> + description: |
> + List of the hardware port in respective IOMMU block for current Socs.
> + Refer to bindings/iommu/mediatek,iommu.yaml.
> +
> + dma-ranges:
> + maxItems: 1
> + description: |
> + Describes the physical address space of IOMMU maps to memory.
> +
> + "#address-cells":
> + const: 1
> +
> + "#size-cells":
> + const: 1
> +
> + ranges: true
> +
> +# Required child node:
> +patternProperties:
> + "venc_core0@1a020000":

This should be a pattern, venc-coreN where N is a number between 0 and 9,
and any iostart should be allowed, not just 1a020000; doing it that way
allows you to do a spec for all the core subnodes in one.

> + type: object
> +
> + properties:
> + compatible:
> + const: mediatek,mtk-venc-core0
> +

..snip..

> +
> +required:
> + - compatible
> + - mediatek,scp
> + - iommus
> + - dma-ranges
> + - ranges
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + #include <dt-bindings/memory/mt8195-memory-port.h>
> + #include <dt-bindings/interrupt-controller/irq.h>
> + #include <dt-bindings/clock/mt8195-clk.h>
> + #include <dt-bindings/power/mt8195-power.h>
> +
> + venc {
> + compatible = "mediatek,mt8195-vcodec-enc";
> + mediatek,scp = <&scp>;
> + iommus = <&iommu_vdo M4U_PORT_L19_VENC_RCPU>;
> + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;

What about doing instead...

#address-cells = <2>;
#size-cells = <2>;
(or fix your dma-ranges property)

and...

ranges = <0 0 0 0x1a020000 0 0x1020000>;
venc-core0@0 {
compatible = "mediatek,mtk-venc-hw";
reg = <0 0 0 0x10000>;
mediatek,hw-leader;
..... other properties .....
};

venc-core1@1000000 {
compatible = "mediatek,mtk-venc-hw";
reg = <0 0x1000000 0 0x10000>;
..... other properties .....
}

Regards,
Angelo

> +
> + venc_core0@1a020000 {
> + compatible = "mediatek,mtk-venc-core0";
> + reg = <0x1a020000 0x10000>;
> + 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 = "MT_CG_VENC0";
> + assigned-clocks = <&topckgen CLK_TOP_VENC>;
> + assigned-clock-parents = <&topckgen CLK_TOP_UNIVPLL_D4>;
> + power-domains = <&spm MT8195_POWER_DOMAIN_VENC>;
> + };
> +
> + venc_core1@1b020000 {
> + compatible = "mediatek,mtk-venc-core1";
> + reg = <0x1b020000 0x10000>;
> + 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 = "MT_CG_VENC1";
> + assigned-clocks = <&topckgen CLK_TOP_VENC>;
> + assigned-clock-parents = <&topckgen CLK_TOP_UNIVPLL_D4>;
> + power-domains = <&spm MT8195_POWER_DOMAIN_VENC_CORE1>;
> + };
> + };

2022-03-04 15:28:35

by AngeloGioacchino Del Regno

[permalink] [raw]

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

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 <[email protected]>
>>> ---
>>> 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