Re: [PATCH v10, 15/19] dt-bindings: media: mtk-vcodec: Adds decoder dt-bindings for mt8192
From: yunfei.dong@xxxxxxxxxxxx
Date: Tue Nov 16 2021 - 07:07:23 EST
Hi Ezequiel,
Thanks for your suggestion.
On Sun, 2021-11-14 at 18:56 -0300, Ezequiel Garcia wrote:
> Yunfei,
>
> On Thu, 11 Nov 2021 at 01:15, Yunfei Dong <yunfei.dong@xxxxxxxxxxxx>
> wrote:
> >
> > Adds decoder dt-bindings for mt8192.
> >
> > Signed-off-by: Yunfei Dong <yunfei.dong@xxxxxxxxxxxx>
> > ---
> > .../media/mediatek,vcodec-subdev-decoder.yaml | 261
> > ++++++++++++++++++
> > 1 file changed, 261 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/media/mediatek,vcodec-subdev-
> > decoder.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/media/mediatek,vcodec-subdev-
> > decoder.yaml
> > b/Documentation/devicetree/bindings/media/mediatek,vcodec-subdev-
> > decoder.yaml
> > new file mode 100644
> > index 000000000000..1886fae6e39d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-
> > subdev-decoder.yaml
> > @@ -0,0 +1,261 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +
> > +%YAML 1.2
> > +---
> > +$id: "
> > http://devicetree.org/schemas/media/mediatek,vcodec-subdev-decoder.yaml#
> > "
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#";
> > +
> > +title: Mediatek Video Decode Accelerator With Multi Hardware
> > +
> > +maintainers:
> > + - Yunfei Dong <yunfei.dong@xxxxxxxxxxxx>
> > +
> > +description: |
> > + Mediatek Video Decode is the video decode hardware present in
> > Mediatek
> > + SoCs which supports high resolution decoding functionalities.
> > Required
> > + main and subdev device node.
> > +
> > + About the Decoder Hardware Block Diagram, please check below:
> > +
> > + +---------------------------------+---------------------------
> > ---------+
> > + | |
> > |
> > + | input -> lat HW -> lat buffer --|--> lat buffer -> core HW
> > -> output
>
> |
>
> To be completely honest, I can't really understand what is the
> meaning
> of the blocks
> with input -> lat hw -> lat buffer, and how this means lat- and core-
> are children of some parent.
>
I will add more detail information as below:
The block diagram is data flow, need two or more hardwares to decode
for each picture. input is input bitstream, lat hardware need input
bitstream and lat buffer to decode, will write temp information to lat
buffer when lat decode done. lat buffer is used to share lat buffer
information for lat hardware and core hardware. core hardware need
frame buffer and lat buffer to decode, will write decode result to
frame buffer.
> > + | || | ||
> > |
> > + +------------||-------------------+---------------------||--
> > -----------+
> > + || lat thread | core
> > thread || <parent>
> > + -------------||-----------------------------------------||--
> > --------------
> > + || ||
> > <child>
> > + \/ <----------------HW index-------------->\/
> > + +----------------------------------------------------
> > --+
> > + | enable/disable
> > |
> > + | clk power irq iommu
> > port |
> > + | (lat/lat
> > soc/core0/core1) |
> > + +----------------------------------------------------
> > --+
> > +
> > + As above, <parent> mean in main device, <child> mean in subdev
> > device. The information
> > + of each hardware will be stored in subdev device. There are two
> > workqueues in main device:
> > + lat and core. Enable/disable the lat clk/power/irq when lat need
> > to work through hardware
> > + index, core is the same.
> > +
> > + Normally the smi common may not the same for each hardware,
> > can't combine all hardware in
> > + one node, or leading to iommu fault when access dram data.
> > +
>
There are one parent node, regards it as main device, each child node
as subdevices, one child node mean one hardware.
There are two work queues in main device used for lat and core hardware
to decode.
The information of each hardware will be stored in subdevices.
Enable/disable the lat clk/power/irq when hardware need to work.
> To what extent the lat- and core- devices are really "children"
> or "subdevices" of the video-codec@16000000 device?
>
> I.e. what resources do they share? What are the details of
> their bus topology?
>
The hardware is really very simple, just enable it to work when needed.
But each hardware isn't independent, need lat and core to decode for
every picture. No complex bus topology.
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/interrupt-controller/arm-gic.h>
> > + #include <dt-bindings/memory/mt8192-larb-port.h>
> > + #include <dt-bindings/interrupt-controller/irq.h>
> > + #include <dt-bindings/clock/mt8192-clk.h>
> > + #include <dt-bindings/power/mt8192-power.h>
> > +
> > + video-codec@16000000 {
> > + compatible = "mediatek,mt8192-vcodec-dec";
> > + reg = <0x16000000 0x1000>; /* VDEC_SYS */
> > + mediatek,scp = <&scp>;
> > + iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>;
> > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>;
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + ranges;
> > + vcodec-lat@16010000 {
> > + compatible = "mediatek,mtk-vcodec-lat";
> > + reg = <0x16010000 0x800>;
> > + interrupts = <GIC_SPI 426 IRQ_TYPE_LEVEL_HIGH 0>;
> > + iommus = <&iommu0 M4U_PORT_L5_VDEC_LAT0_VLD_EXT>,
> > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_VLD2_EXT>,
> > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_AVC_MV_EXT>,
> > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_PRED_RD_EXT>,
> > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_TILE_EXT>,
> > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_WDMA_EXT>,
> > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_RG_CTRL_DMA_EXT>,
> > + <&iommu0 M4U_PORT_L5_VDEC_UFO_ENC_EXT>;
> > + clocks = <&topckgen CLK_TOP_VDEC_SEL>,
> > + <&vdecsys_soc CLK_VDEC_SOC_VDEC>,
> > + <&vdecsys_soc CLK_VDEC_SOC_LAT>,
> > + <&vdecsys_soc CLK_VDEC_SOC_LARB1>,
> > + <&topckgen CLK_TOP_MAINPLL_D4>;
> > + clock-names = "sel", "soc-vdec", "soc-lat", "vdec",
> > "top";
> > + assigned-clocks = <&topckgen CLK_TOP_VDEC_SEL>;
> > + assigned-clock-parents = <&topckgen
> > CLK_TOP_MAINPLL_D4>;
> > + power-domains = <&spm MT8192_POWER_DOMAIN_VDEC>;
> > + };
> > +
> > + vcodec-core@16025000 {
> > + compatible = "mediatek,mtk-vcodec-core";
> > + reg = <0x16025000 0x1000>;
>
> The children address space might need some thinking.
> In other words,
>
> video-codec@16000000 {
>
> vcodec-lat@16010000 {
> }
>
> vcodec-core@16025000 {
> }
> }
>
> Your proposal has vcodec-lat as children of video-codec, but its
> address space
> are really on the same level, instead of children being contained in
> the parent address space:
>
> video-codec@16000000 {
>
> vcodec-lat@10000 {
> }
>
> vcodec-core@25000 {
> }
> }
>
yes, I will try to fix the dtsi as: vcodec-lat@10000, if it looks much
more reasonable.
> I think this last tree makes more sense from a device-tree point of
> view.
> The ranges property allows you to put the right translation
> information,
> so the device driver itself will get the correct address 0x16000000 +
> 0x25000.
>
> You might find that the ranges property is tricky to understand at
> first.
>
> Thanks,
> Ezequiel
>
Hope to get your feedback.
Thanks,
Yunfei Dong
>
> > + interrupts = <GIC_SPI 425 IRQ_TYPE_LEVEL_HIGH 0>;
> > + iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>,
> > + <&iommu0 M4U_PORT_L4_VDEC_UFO_EXT>,
> > + <&iommu0 M4U_PORT_L4_VDEC_PP_EXT>,
> > + <&iommu0 M4U_PORT_L4_VDEC_PRED_RD_EXT>,
> > + <&iommu0 M4U_PORT_L4_VDEC_PRED_WR_EXT>,
> > + <&iommu0 M4U_PORT_L4_VDEC_PPWRAP_EXT>,
> > + <&iommu0 M4U_PORT_L4_VDEC_TILE_EXT>,
> > + <&iommu0 M4U_PORT_L4_VDEC_VLD_EXT>,
> > + <&iommu0 M4U_PORT_L4_VDEC_VLD2_EXT>,
> > + <&iommu0 M4U_PORT_L4_VDEC_AVC_MV_EXT>,
> > + <&iommu0 M4U_PORT_L4_VDEC_RG_CTRL_DMA_EXT>;
> > + clocks = <&topckgen CLK_TOP_VDEC_SEL>,
> > + <&vdecsys CLK_VDEC_VDEC>,
> > + <&vdecsys CLK_VDEC_LAT>,
> > + <&vdecsys CLK_VDEC_LARB1>,
> > + <&topckgen CLK_TOP_MAINPLL_D4>;
> > + clock-names = "sel", "soc-vdec", "soc-lat", "vdec",
> > "top";
> > + assigned-clocks = <&topckgen CLK_TOP_VDEC_SEL>;
> > + assigned-clock-parents = <&topckgen
> > CLK_TOP_MAINPLL_D4>;
> > + power-domains = <&spm MT8192_POWER_DOMAIN_VDEC2>;
> > + };
> > + };
> > --
> > 2.25.1
> >