| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <0b4f1ab2753d4fdb4934e8f76df6e584bba2d628.camel@mediatek.com>
Date: Wed, 13 Sep 2023 08:39:14 +0000
From: Moudy Ho (何宗原) <Moudy.Ho@...iatek.com>
To: "robh+dt@...nel.org" <robh+dt@...nel.org>,
"chunkuang.hu@...nel.org" <chunkuang.hu@...nel.org>,
"mchehab@...nel.org" <mchehab@...nel.org>,
"krzysztof.kozlowski@...aro.org" <krzysztof.kozlowski@...aro.org>,
"daniel@...ll.ch" <daniel@...ll.ch>,
"p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
"conor+dt@...nel.org" <conor+dt@...nel.org>,
"hverkuil-cisco@...all.nl" <hverkuil-cisco@...all.nl>,
"airlied@...il.com" <airlied@...il.com>,
"krzysztof.kozlowski+dt@...aro.org"
<krzysztof.kozlowski+dt@...aro.org>,
"matthias.bgg@...il.com" <matthias.bgg@...il.com>
CC: "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-mediatek@...ts.infradead.org"
<linux-mediatek@...ts.infradead.org>,
"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
"angelogioacchino.delregno@...labora.com"
<angelogioacchino.delregno@...labora.com>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v5 3/3] dt-binding: mediatek: add MediaTek mt8195 MDP3
components
Hi Krzysztof,
On Tue, 2023-09-12 at 10:19 +0200, Krzysztof Kozlowski wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> On 12/09/2023 09:56, Moudy Ho wrote:
> > Introduce more MDP3 components present in MT8195.
>
> Please use subject prefixes matching the subsystem. You can get them
> for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the
> directory
> your patch is touching.
>
> >
> > Signed-off-by: Moudy Ho <moudy.ho@...iatek.com>
> > ---
> > .../display/mediatek/mediatek,aal.yaml | 2 +-
> > .../display/mediatek/mediatek,color.yaml | 2 +-
> > .../display/mediatek/mediatek,merge.yaml | 1 +
> > .../display/mediatek/mediatek,ovl.yaml | 2 +-
> > .../display/mediatek/mediatek,split.yaml | 1 +
> > .../bindings/media/mediatek,mdp3-fg.yaml | 61
> +++++++++++++++++++
> > .../bindings/media/mediatek,mdp3-hdr.yaml | 60
> ++++++++++++++++++
> > .../bindings/media/mediatek,mdp3-pad.yaml | 61
> +++++++++++++++++++
> > .../bindings/media/mediatek,mdp3-rdma.yaml | 16 ++---
> > .../bindings/media/mediatek,mdp3-stitch.yaml | 61
> +++++++++++++++++++
> > .../bindings/media/mediatek,mdp3-tcc.yaml | 60
> ++++++++++++++++++
> > .../bindings/media/mediatek,mdp3-tdshp.yaml | 61
> +++++++++++++++++++
> > 12 files changed, 378 insertions(+), 10 deletions(-)
> > create mode 100644
> Documentation/devicetree/bindings/media/mediatek,mdp3-fg.yaml
> > create mode 100644
> Documentation/devicetree/bindings/media/mediatek,mdp3-hdr.yaml
> > create mode 100644
> Documentation/devicetree/bindings/media/mediatek,mdp3-pad.yaml
> > create mode 100644
> Documentation/devicetree/bindings/media/mediatek,mdp3-stitch.yaml
> > create mode 100644
> Documentation/devicetree/bindings/media/mediatek,mdp3-tcc.yaml
> > create mode 100644
> Documentation/devicetree/bindings/media/mediatek,mdp3-tdshp.yaml
> >
> > diff --git
> a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yam
> l
> b/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yam
> l
> > index 7fd42c8fdc32..04b1314d00f2 100644
> > ---
> a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yam
> l
> > +++
> b/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yam
> l
> > @@ -24,6 +24,7 @@ properties:
> > - enum:
> > - mediatek,mt8173-disp-aal
> > - mediatek,mt8183-disp-aal
> > + - mediatek,mt8195-mdp3-aal
> > - items:
> > - enum:
> > - mediatek,mt2712-disp-aal
> > @@ -63,7 +64,6 @@ properties:
> > required:
> > - compatible
> > - reg
> > - - interrupts
>
> Why? commit msg tells nothing about it. Why interrupt is not erquired
> in
> mt8173? How dropping such requirement is anyhow related to mt8195?
>
>
The signals of the MDP engines are completely controlled by MTK's MUTEX
for starting and stopping frame processing, eliminating the need for
additional interrupts.
Considering the discussion in the previous version, it is advisable to
merge it into the existing display binding files.
Therefore, I tried removing the required conditions to facilitate file
merging.
However, for file integrity purposes, I should revert these changes and
set the corresponding settings in DTS(even if they are not used).
The other YAML files - color, merge and ovl - mentioned below will also
be rectified in the next version.
> > - power-domains
> > - clocks
> >
> > diff --git
> a/Documentation/devicetree/bindings/display/mediatek/mediatek,color.y
> aml
> b/Documentation/devicetree/bindings/display/mediatek/mediatek,color.y
> aml
> > index f21e44092043..8e97b0a6a7b3 100644
> > ---
> a/Documentation/devicetree/bindings/display/mediatek/mediatek,color.y
> aml
> > +++
> b/Documentation/devicetree/bindings/display/mediatek/mediatek,color.y
> aml
> > @@ -26,6 +26,7 @@ properties:
> > - mediatek,mt2701-disp-color
> > - mediatek,mt8167-disp-color
> > - mediatek,mt8173-disp-color
> > + - mediatek,mt8195-mdp3-color
> > - items:
> > - enum:
> > - mediatek,mt7623-disp-color
> > @@ -66,7 +67,6 @@ properties:
> > required:
> > - compatible
> > - reg
> > - - interrupts
>
> Why?
>
> > - power-domains
> > - clocks
> >
> > diff --git
> a/Documentation/devicetree/bindings/display/mediatek/mediatek,merge.y
> aml
> b/Documentation/devicetree/bindings/display/mediatek/mediatek,merge.y
> aml
> > index eead5cb8636e..401498523404 100644
> > ---
> a/Documentation/devicetree/bindings/display/mediatek/mediatek,merge.y
> aml
> > +++
> b/Documentation/devicetree/bindings/display/mediatek/mediatek,merge.y
> aml
> > @@ -24,6 +24,7 @@ properties:
> > - enum:
> > - mediatek,mt8173-disp-merge
> > - mediatek,mt8195-disp-merge
> > + - mediatek,mt8195-mdp3-merge
> > - items:
> > - const: mediatek,mt6795-disp-merge
> > - const: mediatek,mt8173-disp-merge
> > diff --git
> a/Documentation/devicetree/bindings/display/mediatek/mediatek,ovl.yam
> l
> b/Documentation/devicetree/bindings/display/mediatek/mediatek,ovl.yam
> l
> > index 3e1069b00b56..10d4d4f64e09 100644
> > ---
> a/Documentation/devicetree/bindings/display/mediatek/mediatek,ovl.yam
> l
> > +++
> b/Documentation/devicetree/bindings/display/mediatek/mediatek,ovl.yam
> l
> > @@ -26,6 +26,7 @@ properties:
> > - mediatek,mt8173-disp-ovl
> > - mediatek,mt8183-disp-ovl
> > - mediatek,mt8192-disp-ovl
> > + - mediatek,mt8195-mdp3-ovl
> > - items:
> > - enum:
> > - mediatek,mt7623-disp-ovl
> > @@ -76,7 +77,6 @@ properties:
> > required:
> > - compatible
> > - reg
> > - - interrupts
>
> Why?
>
> > - power-domains
> > - clocks
> > - iommus
> > diff --git
> a/Documentation/devicetree/bindings/display/mediatek/mediatek,split.y
> aml
> b/Documentation/devicetree/bindings/display/mediatek/mediatek,split.y
> aml
> > index a8a5c9608598..a96b271e3240 100644
> > ---
> a/Documentation/devicetree/bindings/display/mediatek/mediatek,split.y
> aml
> > +++
> b/Documentation/devicetree/bindings/display/mediatek/mediatek,split.y
> aml
> > @@ -23,6 +23,7 @@ properties:
> > oneOf:
> > - enum:
> > - mediatek,mt8173-disp-split
> > + - mediatek,mt8195-mdp3-split
> > - items:
> > - const: mediatek,mt6795-disp-split
> > - const: mediatek,mt8173-disp-split
> > diff --git a/Documentation/devicetree/bindings/media/mediatek,mdp3-
> fg.yaml b/Documentation/devicetree/bindings/media/mediatek,mdp3-
> fg.yaml
> > new file mode 100644
> > index 000000000000..71fd449de8b4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/mediatek,mdp3-fg.yaml
> > @@ -0,0 +1,61 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/mediatek,mdp3-fg.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: MediaTek Media Data Path 3 FG
> > +
> > +maintainers:
> > + - Matthias Brugger <matthias.bgg@...il.com>
> > + - Moudy Ho <moudy.ho@...iatek.com>
> > +
> > +description:
> > + One of Media Data Path 3 (MDP3) components used to add film
> grain
> > + according to AV1 spec.
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - mediatek,mt8195-mdp3-fg
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + mediatek,gce-client-reg:
> > + description:
> > + The register of display function block to be set by gce.
> There are 4 arguments,
> > + such as gce node, subsys id, offset and register size. The
> subsys id that is
> > + mapping to the register of display function blocks is
> defined in the gce header
> > + include/dt-bindings/gce/<chip>-gce.h of each chips.
> > + $ref: /schemas/types.yaml#/definitions/phandle-array
> > + items:
> > + items:
> > + - description: phandle of GCE
> > + - description: GCE subsys id
> > + - description: register offset
> > + - description: register size
> > + maxItems: 1
> > +
> > + clocks:
> > + minItems: 1
>
> This must be maxItems. Use existing code as an example, do not re-
> invent it.
>
Thanks for the advice, it will be fixed in the next version.
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - mediatek,gce-client-reg
> > + - clocks
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/clock/mt8195-clk.h>
> > + #include <dt-bindings/gce/mt8195-gce.h>
> > +
> > + display@...02000 {
> > + compatible = "mediatek,mt8195-mdp3-fg";
> > + reg = <0x14002000 0x1000>;
> > + mediatek,gce-client-reg = <&gce1 SUBSYS_1400XXXX 0x2000
> 0x1000>;
> > + clocks = <&vppsys0 CLK_VPP0_MDP_FG>;
> > + };
> > diff --git a/Documentation/devicetree/bindings/media/mediatek,mdp3-
> hdr.yaml b/Documentation/devicetree/bindings/media/mediatek,mdp3-
> hdr.yaml
> > new file mode 100644
> > index 000000000000..fb1bb5a9e57f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/mediatek,mdp3-
> hdr.yaml
> > @@ -0,0 +1,60 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/mediatek,mdp3-hdr.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: MediaTek Media Data Path 3 HDR
> > +
> > +maintainers:
> > + - Matthias Brugger <matthias.bgg@...il.com>
> > + - Moudy Ho <moudy.ho@...iatek.com>
> > +
> > +description:
> > + One of Media Data Path 3 (MDP3) components used to perform HDR
> to SDR
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - mediatek,mt8195-mdp3-hdr
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + mediatek,gce-client-reg:
> > + description:
> > + The register of display function block to be set by gce.
> There are 4 arguments,
> > + such as gce node, subsys id, offset and register size. The
> subsys id that is
> > + mapping to the register of display function blocks is
> defined in the gce header
> > + include/dt-bindings/gce/<chip>-gce.h of each chips.
> > + $ref: /schemas/types.yaml#/definitions/phandle-array
> > + items:
> > + items:
> > + - description: phandle of GCE
> > + - description: GCE subsys id
> > + - description: register offset
> > + - description: register size
> > + maxItems: 1
> > +
> > + clocks:
> > + minItems: 1
>
> Here as well. Why is this minitems?
>
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - mediatek,gce-client-reg
> > + - clocks
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/clock/mt8195-clk.h>
> > + #include <dt-bindings/gce/mt8195-gce.h>
> > +
> > + display@...04000 {
> > + compatible = "mediatek,mt8195-mdp3-hdr";
> > + reg = <0x14004000 0x1000>;
> > + mediatek,gce-client-reg = <&gce1 SUBSYS_1400XXXX 0x4000
> 0x1000>;
> > + clocks = <&vppsys0 CLK_VPP0_MDP_HDR>;
> > + };
> > diff --git a/Documentation/devicetree/bindings/media/mediatek,mdp3-
> pad.yaml b/Documentation/devicetree/bindings/media/mediatek,mdp3-
> pad.yaml
> > new file mode 100644
> > index 000000000000..13b66c5985fe
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/mediatek,mdp3-
> pad.yaml
> > @@ -0,0 +1,61 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/mediatek,mdp3-pad.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: MediaTek Media Data Path 3 PADDING
> > +
> > +maintainers:
> > + - Matthias Brugger <matthias.bgg@...il.com>
> > + - Moudy Ho <moudy.ho@...iatek.com>
> > +
> > +description:
> > + One of Media Data Path 3 (MDP3) components used to insert
> > + pre-defined color or alpha value to arbitrary side of image.
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - mediatek,mt8195-mdp3-pad
>
> And you cannot add it to existing padding because?
>
Apologies for the oversight. I will integrate padding into the same
files, and include a link to the series below:
Message ID =
20230911074233.31556-5-shawn.sung@...iatek.com
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + mediatek,gce-client-reg:
> > + description:
> > + The register of display function block to be set by gce.
> There are 4 arguments,
> > + such as gce node, subsys id, offset and register size. The
> subsys id that is
> > + mapping to the register of display function blocks is
> defined in the gce header
> > + include/dt-bindings/gce/<chip>-gce.h of each chips.
> > + $ref: /schemas/types.yaml#/definitions/phandle-array
> > + items:
> > + items:
> > + - description: phandle of GCE
> > + - description: GCE subsys id
> > + - description: register offset
> > + - description: register size
> > + maxItems: 1
> > +
> > + clocks:
> > + minItems: 1
>
> Nope
>
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - mediatek,gce-client-reg
> > + - clocks
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/clock/mt8195-clk.h>
> > + #include <dt-bindings/gce/mt8195-gce.h>
> > +
> > + display@...0a000 {
> > + compatible = "mediatek,mt8195-mdp3-pad";
> > + reg = <0x1400a000 0x1000>;
> > + mediatek,gce-client-reg = <&gce1 SUBSYS_1400XXXX 0xa000
> 0x1000>;
> > + clocks = <&vppsys0 CLK_VPP0_PADDING>;
> > + };
> > diff --git a/Documentation/devicetree/bindings/media/mediatek,mdp3-
> rdma.yaml b/Documentation/devicetree/bindings/media/mediatek,mdp3-
> rdma.yaml
> > index 0c22571d8c22..17cd5b587e23 100644
> > --- a/Documentation/devicetree/bindings/media/mediatek,mdp3-
> rdma.yaml
> > +++ b/Documentation/devicetree/bindings/media/mediatek,mdp3-
> rdma.yaml
> > @@ -23,6 +23,7 @@ properties:
> > enum:
> > - mediatek,mt8183-mdp3-rdma
> > - mediatek,mt8195-vdo1-rdma
> > + - mediatek,mt8195-mdp3-rdma
>
> m is before v
>
Thanks for pointing that out. It will be addressed and fixed.
> >
> > reg:
> > maxItems: 1
> > @@ -50,17 +51,19 @@ properties:
> > maxItems: 1
> >
> > clocks:
> > - items:
> > - - description: RDMA clock
> > - - description: RSZ clock
> > + oneOf:
> > + - items:
> > + - description: RDMA clock
> > + - description: SRAM shared component clock
> > + - items:
> > + - description: RDMA clock
>
> Why now mt8183 can have SRAM clock optional? How changing mt8183 is
> related to this patch?
>
> I'll finish the review, sorry fix basics here.
>
> Best regards,
> Krzysztof
>
The RDMA of only the 8183 needed to share SRMA with other component due
to the old desgin.
I attempted to describe both the situation of the 8183 and new designs
like the 8195, but it appears that this writing style may lead to
misunderstandings.
I am unsure if there are any ways to enhance it.
Sincerely,
Moudy
Powered by blists - more mailing lists