Re: [PATCH v4 03/16] dt-binding: mediatek: add MediaTek mt8195 MDP3 components

["Moudy Ho (何宗原)" <Moudy.Ho@mediatek.com>]

Hi Rob,

Thanks for taking the time to review those bindings.

On Thu, 2023-02-09 at 12:01 -0600, Rob Herring wrote:
> On Wed, Feb 08, 2023 at 05:21:56PM +0800, Moudy Ho wrote:
> > Adds support for MT8195 MDP3 RDMA, and introduce more MDP3
> > components
> > present in MT8195.
> 
> I'm sure I asked this before, but how are these blocks different
> from 
> what we already have in bindings/display/mediatek/. It's all the
> same 
> block names. If they are the same/similar h/w, then it should be 1 
> binding even if you have different consumers in the kernel.
> 
> If my questions aren't answered in the patches, then I'll just keep 
> asking because I won't remember...
> 

As you mentioned, some blocks are indeed the same as those under path
"bindings/display/mediatek", the difference is only in the way of
operation.

1. By remove the required property "interrupts", 3 blocks can be
merged:
   mediatek,aal.yaml
   mediatek,color.yaml
   mediatek,ovl.yaml

2. By adding optional "mediatek,gce-client-reg" property, the following
two can also be merged:
   mediatek,merge.yaml
   mediatek,split.yaml

3. Besides, there is a binding "mediatek,mdp-rdma.yaml" that needs to
be discussed, which is newly added after the RDMA of DISP/MDP3.
   It presents in mt8195 VDO1 and upon inspection it has the same h/w
as the MDP3 RDMA.
   May I remove this file and list it in "media/mediatek,mdp3-rdma.yaml 
" with an enum?

> > 
> > Signed-off-by: Moudy Ho <moudy.ho@mediatek.com>
> > ---
> >  .../bindings/media/mediatek,mdp3-rdma.yaml    |  30 +--
> >  .../bindings/media/mediatek,mdp3-rsz.yaml     |   5 +-
> >  .../bindings/media/mediatek,mt8195-mdp3.yaml  | 174
> > ++++++++++++++++++
> >  3 files changed, 197 insertions(+), 12 deletions(-)
> >  create mode 100644
> > Documentation/devicetree/bindings/media/mediatek,mt8195-mdp3.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/media/mediatek,mdp3-
> > rdma.yaml b/Documentation/devicetree/bindings/media/mediatek,mdp3-
> > rdma.yaml
> > index 46730687c662..abc3284b21d0 100644
> > --- a/Documentation/devicetree/bindings/media/mediatek,mdp3-
> > rdma.yaml
> > +++ b/Documentation/devicetree/bindings/media/mediatek,mdp3-
> > rdma.yaml
> > @@ -20,8 +20,9 @@ description: |
> >  
> >  properties:
> >    compatible:
> > -    items:
> > -      - const: mediatek,mt8183-mdp3-rdma
> > +    enum:
> > +      - mediatek,mt8183-mdp3-rdma
> > +      - mediatek,mt8195-mdp3-rdma
> >  
> >    reg:
> >      maxItems: 1
> > @@ -46,20 +47,28 @@ properties:
> >      $ref: /schemas/types.yaml#/definitions/uint32-array
> >  
> >    power-domains:
> > -    maxItems: 1
> > +    oneOf:
> > +      - items:
> > +          - description: for RDMA
> > +      - items:
> > +          - description: for vppsys 0
> > +          - description: for vppsys 1
> >  
> >    clocks:
> > -    items:
> > -      - description: RDMA clock
> > -      - description: RSZ clock
> > +    minItems: 2
> > +    maxItems: 19
> >  
> >    iommus:
> > -    maxItems: 1
> > +    oneOf:
> > +      - items:
> > +          - description: RDMA port
> > +      - items:
> > +          - description: RDMA port
> > +          - description: RDMA to WROT DL port
> >  
> >    mboxes:
> > -    items:
> > -      - description: used for 1st data pipe from RDMA
> > -      - description: used for 2nd data pipe from RDMA
> > +    minItems: 1
> > +    maxItems: 5
> >  
> >    '#dma-cells':
> >      const: 1
> > @@ -72,7 +81,6 @@ required:
> >    - power-domains
> >    - clocks
> >    - iommus
> > -  - mboxes
> >    - '#dma-cells'
> >  
> >  additionalProperties: false
> > diff --git a/Documentation/devicetree/bindings/media/mediatek,mdp3-
> > rsz.yaml b/Documentation/devicetree/bindings/media/mediatek,mdp3-
> > rsz.yaml
> > index 78f9de6192ef..4bc5ac112d2a 100644
> > --- a/Documentation/devicetree/bindings/media/mediatek,mdp3-
> > rsz.yaml
> > +++ b/Documentation/devicetree/bindings/media/mediatek,mdp3-
> > rsz.yaml
> > @@ -43,12 +43,15 @@ properties:
> >  
> >    clocks:
> >      minItems: 1
> > +    maxItems: 2
> > +
> > +  power-domains:
> > +    maxItems: 1
> >  
> >  required:
> >    - compatible
> >    - reg
> >    - mediatek,gce-client-reg
> > -  - mediatek,gce-events
> >    - clocks
> >  
> >  additionalProperties: false
> > diff --git
> > a/Documentation/devicetree/bindings/media/mediatek,mt8195-mdp3.yaml 
> > b/Documentation/devicetree/bindings/media/mediatek,mt8195-mdp3.yaml
> > new file mode 100644
> > index 000000000000..d2b01456c495
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/mediatek,mt8195-
> > mdp3.yaml
> > @@ -0,0 +1,174 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: 
> > https://urldefense.com/v3/__http://devicetree.org/schemas/media/mediatek,mt8195-mdp3.yaml*__;Iw!!CTRNKA9wMg0ARbw!mODe8g9MAxc3lRYniX1pJA1MrkgZ7WMEqLHFU-KneHoDKs0gsAGTOzzmF3L0Soq___rPGrQfbuVfnw$ 
> >  
> > +$schema: 
> > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!mODe8g9MAxc3lRYniX1pJA1MrkgZ7WMEqLHFU-KneHoDKs0gsAGTOzzmF3L0Soq___rPGrQokjKe2Q$ 
> >  
> > +
> > +title: MediaTek Media Data Path 3 display components
> > +
> > +maintainers:
> > +  - Matthias Brugger <matthias.bgg@gmail.com>
> > +  - Moudy Ho <moudy.ho@mediatek.com>
> > +
> > +description:
> > +  A group of display pipeline components for image quality
> > adjustment,
> > +  color format conversion and data flow control, and the
> > abbreviations
> > +  are explained below.
> > +  AAL    - Ambient-light Adaptive Luma.
> > +  Color  - Enhance or reduce color in Y/S/H channel.
> > +  FG     - Fime Grain for AV1 spec.
> > +  HDR    - Perform HDR to SDR.
> > +  MERGE  - Used to merge two slice-per-line into one side-by-side.
> > +  OVL    - Perform alpha blending.
> > +  PAD    - Predefined alpha or color value insertion.
> > +  SPLIT  - Split a HDMI stream into two ouptut.
> > +  STITCH - Combine multiple video frame with overlapping fields of
> > view.
> > +  TCC    - HDR gamma curve conversion support.
> > +  TDSHP  - Sharpness and contrast improvement.
> 
> Each block likely needs its own schema.
> 

After deducting the bindings that can be merged before, the remaining
ones will have exactly the same attributes. Can those be simplified
into one bindings as discussed in the 3rd version?

https://patchwork.kernel.org/project/linux-media/patch/20230116032147.23607-2-moudy.ho@mediatek.com/

Sincerely,
Moudy

> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - mediatek,mt8195-mdp3-aal
> > +      - mediatek,mt8195-mdp3-color
> > +      - mediatek,mt8195-mdp3-fg
> > +      - mediatek,mt8195-mdp3-hdr
> > +      - mediatek,mt8195-mdp3-merge
> > +      - mediatek,mt8195-mdp3-ovl
> > +      - mediatek,mt8195-mdp3-pad
> > +      - mediatek,mt8195-mdp3-split
> > +      - mediatek,mt8195-mdp3-stitch
> > +      - mediatek,mt8195-mdp3-tcc
> > +      - mediatek,mt8195-mdp3-tdshp
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  mediatek,gce-client-reg:
> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +    items:
> > +      items:
> > +        - description: phandle of GCE
> > +        - description: GCE subsys id
> > +        - description: register offset
> > +        - description: register size
> 
> Given these match up to reg values, I'm really wondering why we have 
> this.
> 
> > +    description:
> > +      Each GCE subsys id is mapping to a base address of display
> > function blocks
> > +      register which is defined in <include/dt-
> > bindings/gce/mt8195-gce.h>.
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    minItems: 1
> > +    maxItems: 7
> 
> You have to define what each clock is which probably depends on each 
> block.
> 
> Rob