[PATCH v12 0/3] drm/mediatek: Add support for OF graphs

[PATCH v12 0/3] drm/mediatek: Add support for OF graphs

[AngeloGioacchino Del Regno]

Changes in v12:
 - Added comment to describe graph for OVL_ADAPTOR in patch [3/3]
   as suggested by CK Hu.

Changes in v11:
 - Added OVL_ADAPTOR_MDP_RDMA to OVL Adaptor exclusive components list
   to avoid failures in graphs with MDP_RDMA inside
 - Rebased on next-20241004

Changes in v10:
 - Removed erroneously added *.orig/*.rej files

Changes in v9:
 - Rebased on next-20240910
 - Removed redundant assignment and changed a print to dev_err()
 - Dropped if branch to switch conversion as requested; this will
   be sent as a separate commit out of this series.

Changes in v8:
 - Rebased on next-20240617
 - Changed to allow probing a VDO with no available display outputs

Changes in v7:
 - Fix typo in patch 3/3

Changes in v6:
 - Added EPROBE_DEFER check to fix dsi/dpi false positive DT fallback case
 - Dropped refcount of ep_out in mtk_drm_of_get_ddp_ep_cid()
 - Fixed double refcount drop during path building
 - Removed failure upon finding a DT-disabled path as requested
 - Tested again on MT8195, MT8395 boards

Changes in v5:
 - Fixed commit [2/3], changed allOf -> anyOf to get the
   intended allowance in the binding

Changes in v4:
 - Fixed a typo that caused pure OF graphs pipelines multiple
   concurrent outputs to not get correctly parsed (port->id); 
 - Added OVL_ADAPTOR support for OF graph specified pipelines;
 - Now tested with fully OF Graph specified pipelines on MT8195
   Chromebooks and MT8395 boards;
 - Rebased on next-20240516

Changes in v3:
 - Rebased on next-20240502 because of renames in mediatek-drm

Changes in v2:
 - Fixed wrong `required` block indentation in commit [2/3]


The display IPs in MediaTek SoCs are *VERY* flexible and those support
being interconnected with different instances of DDP IPs (for example,
merge0 or merge1) and/or with different DDP IPs (for example, rdma can
be connected with either color, dpi, dsi, merge, etc), forming a full
Display Data Path that ends with an actual display.

This series was born because of an issue that I've found while enabling
support for MT8195/MT8395 boards with DSI output as main display: the
current mtk_drm_route variations would not work as currently, the driver
hardcodes a display path for Chromebooks, which have a DisplayPort panel
with DSC support, instead of a DSI panel without DSC support.

There are other reasons for which I wrote this series, and I find that
hardcoding those paths - when a HW path is clearly board-specific - is
highly suboptimal. Also, let's not forget about keeping this driver from
becoming a huge list of paths for each combination of SoC->board->disp
and... this and that.

For more information, please look at the commit description for each of
the commits included in this series.

This series is essential to enable support for the MT8195/MT8395 EVK,
Kontron i1200, Radxa NIO-12L and, mainly, for non-Chromebook boards
and Chromebooks to co-exist without conflicts.

Besides, this is also a valid option for MT8188 Chromebooks which might
have different DSI-or-eDP displays depending on the model (as far as I
can see from the mtk_drm_route attempt for this SoC that is already
present in this driver).

This series was tested on MT8195 Cherry Tomato and on MT8395 Radxa
NIO-12L with both hardcoded paths, OF graph support and partially
hardcoded paths, and pure OF graph support including pipelines that
require OVL_ADAPTOR support.


AngeloGioacchino Del Regno (3):
  dt-bindings: display: mediatek: Add OF graph support for board path
  dt-bindings: arm: mediatek: mmsys: Add OF graph support for board path
  drm/mediatek: Implement OF graphs support for display paths

 .../bindings/arm/mediatek/mediatek,mmsys.yaml |  28 ++
 .../display/mediatek/mediatek,aal.yaml        |  40 +++
 .../display/mediatek/mediatek,ccorr.yaml      |  21 ++
 .../display/mediatek/mediatek,color.yaml      |  22 ++
 .../display/mediatek/mediatek,dither.yaml     |  22 ++
 .../display/mediatek/mediatek,dpi.yaml        |  25 +-
 .../display/mediatek/mediatek,dsc.yaml        |  24 ++
 .../display/mediatek/mediatek,dsi.yaml        |  27 +-
 .../display/mediatek/mediatek,ethdr.yaml      |  22 ++
 .../display/mediatek/mediatek,gamma.yaml      |  19 ++
 .../display/mediatek/mediatek,merge.yaml      |  23 ++
 .../display/mediatek/mediatek,od.yaml         |  22 ++
 .../display/mediatek/mediatek,ovl-2l.yaml     |  22 ++
 .../display/mediatek/mediatek,ovl.yaml        |  22 ++
 .../display/mediatek/mediatek,postmask.yaml   |  21 ++
 .../display/mediatek/mediatek,rdma.yaml       |  22 ++
 .../display/mediatek/mediatek,ufoe.yaml       |  21 ++
 drivers/gpu/drm/mediatek/mtk_disp_drv.h       |   1 +
 .../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c   |  43 ++-
 drivers/gpu/drm/mediatek/mtk_dpi.c            |  21 +-
 drivers/gpu/drm/mediatek/mtk_drm_drv.c        | 253 +++++++++++++++++-
 drivers/gpu/drm/mediatek/mtk_drm_drv.h        |   2 +-
 drivers/gpu/drm/mediatek/mtk_dsi.c            |  14 +-
 23 files changed, 712 insertions(+), 25 deletions(-)

-- 
2.46.1

[PATCH v12 1/3] dt-bindings: display: mediatek: Add OF graph support for board path

[AngeloGioacchino Del Regno]

The display IPs in MediaTek SoCs support being interconnected with
different instances of DDP IPs (for example, merge0 or merge1) and/or
with different DDP IPs (for example, rdma can be connected with either
color, dpi, dsi, merge, etc), forming a full Display Data Path that
ends with an actual display.

The final display pipeline is effectively board specific, as it does
depend on the display that is attached to it, and eventually on the
sensors supported by the board (for example, Adaptive Ambient Light
would need an Ambient Light Sensor, otherwise it's pointless!), other
than the output type.

Add support for OF graphs to most of the MediaTek DDP (display) bindings
to add flexibility to build custom hardware paths, hence enabling board
specific configuration of the display pipeline and allowing to finally
migrate away from using hardcoded paths.

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
Tested-by: Alexandre Mergnat <amergnat@baylibre.com>
Reviewed-by: CK Hu <ck.hu@mediatek.com>
Tested-by: Michael Walle <mwalle@kernel.org> # on kontron-sbc-i1200
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 .../display/mediatek/mediatek,aal.yaml        | 40 +++++++++++++++++++
 .../display/mediatek/mediatek,ccorr.yaml      | 21 ++++++++++
 .../display/mediatek/mediatek,color.yaml      | 22 ++++++++++
 .../display/mediatek/mediatek,dither.yaml     | 22 ++++++++++
 .../display/mediatek/mediatek,dpi.yaml        | 25 +++++++++++-
 .../display/mediatek/mediatek,dsc.yaml        | 24 +++++++++++
 .../display/mediatek/mediatek,dsi.yaml        | 27 ++++++++++++-
 .../display/mediatek/mediatek,ethdr.yaml      | 22 ++++++++++
 .../display/mediatek/mediatek,gamma.yaml      | 19 +++++++++
 .../display/mediatek/mediatek,merge.yaml      | 23 +++++++++++
 .../display/mediatek/mediatek,od.yaml         | 22 ++++++++++
 .../display/mediatek/mediatek,ovl-2l.yaml     | 22 ++++++++++
 .../display/mediatek/mediatek,ovl.yaml        | 22 ++++++++++
 .../display/mediatek/mediatek,postmask.yaml   | 21 ++++++++++
 .../display/mediatek/mediatek,rdma.yaml       | 22 ++++++++++
 .../display/mediatek/mediatek,ufoe.yaml       | 21 ++++++++++
 16 files changed, 372 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
index cf24434854ff..47ddba5c41af 100644
--- a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
@@ -62,6 +62,27 @@ properties:
     $ref: /schemas/types.yaml#/definitions/phandle-array
     maxItems: 1
 
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+    description:
+      Input and output ports can have multiple endpoints, each of those
+      connects to either the primary, secondary, etc, display pipeline.
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: AAL input port
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          AAL output to the next component's input, for example could be one
+          of many gamma, overdrive or other blocks.
+
+    required:
+      - port@0
+      - port@1
+
 required:
   - compatible
   - reg
@@ -89,5 +110,24 @@ examples:
            power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
            clocks = <&mmsys CLK_MM_DISP_AAL>;
            mediatek,gce-client-reg = <&gce SUBSYS_1401XXXX 0x5000 0x1000>;
+
+           ports {
+               #address-cells = <1>;
+               #size-cells = <0>;
+
+               port@0 {
+                   reg = <0>;
+                   aal0_in: endpoint {
+                       remote-endpoint = <&ccorr0_out>;
+                   };
+               };
+
+               port@1 {
+                   reg = <1>;
+                   aal0_out: endpoint {
+                       remote-endpoint = <&gamma0_in>;
+                   };
+               };
+           };
        };
     };
diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
index 9f8366763831..fca8e7bb0cbc 100644
--- a/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
@@ -57,6 +57,27 @@ properties:
     $ref: /schemas/types.yaml#/definitions/phandle-array
     maxItems: 1
 
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+    description:
+      Input and output ports can have multiple endpoints, each of those
+      connects to either the primary, secondary, etc, display pipeline.
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: CCORR input port
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          CCORR output to the input of the next desired component in the
+          display pipeline, usually only one of the available AAL blocks.
+
+    required:
+      - port@0
+      - port@1
+
 required:
   - compatible
   - reg
diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
index 7df786bbad20..6160439ce4d7 100644
--- a/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
@@ -65,6 +65,28 @@ properties:
     $ref: /schemas/types.yaml#/definitions/phandle-array
     maxItems: 1
 
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+    description:
+      Input and output ports can have multiple endpoints, each of those
+      connects to either the primary, secondary, etc, display pipeline.
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: COLOR input port
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          COLOR output to the input of the next desired component in the
+          display pipeline, for example one of the available CCORR or AAL
+          blocks.
+
+    required:
+      - port@0
+      - port@1
+
 required:
   - compatible
   - reg
diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
index 6fceb1f95d2a..abaf27916d13 100644
--- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
@@ -56,6 +56,28 @@ properties:
     $ref: /schemas/types.yaml#/definitions/phandle-array
     maxItems: 1
 
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+    description:
+      Input and output ports can have multiple endpoints, each of those
+      connects to either the primary, secondary, etc, display pipeline.
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: DITHER input, usually from a POSTMASK or GAMMA block.
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          DITHER output to the input of the next desired component in the
+          display pipeline, for example one of the available DSC compressors,
+          DP_INTF, DSI, LVDS or others.
+
+    required:
+      - port@0
+      - port@1
+
 required:
   - compatible
   - reg
diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
index 3a82aec9021c..b567e3d58aa1 100644
--- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
@@ -71,13 +71,34 @@ properties:
       Output port node. This port should be connected to the input port of an
       attached HDMI, LVDS or DisplayPort encoder chip.
 
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: DPI input port
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: DPI output to an HDMI, LVDS or DisplayPort encoder input
+
+    required:
+      - port@0
+      - port@1
+
 required:
   - compatible
   - reg
   - interrupts
   - clocks
   - clock-names
-  - port
+
+oneOf:
+  - required:
+      - port
+  - required:
+      - ports
 
 allOf:
   - if:
@@ -100,7 +121,7 @@ examples:
     #include <dt-bindings/interrupt-controller/arm-gic.h>
     #include <dt-bindings/clock/mt8173-clk.h>
 
-    dpi0: dpi@1401d000 {
+    dpi: dpi@1401d000 {
         compatible = "mediatek,mt8173-dpi";
         reg = <0x1401d000 0x1000>;
         interrupts = <GIC_SPI 194 IRQ_TYPE_LEVEL_LOW>;
diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dsc.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dsc.yaml
index 2cbdd9ee449d..846de6c17d93 100644
--- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dsc.yaml
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dsc.yaml
@@ -49,6 +49,30 @@ properties:
     $ref: /schemas/types.yaml#/definitions/phandle-array
     maxItems: 1
 
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+    description:
+      Input and output ports can have multiple endpoints, each of those
+      connects to either the primary, secondary, etc, display pipeline.
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          Display Stream Compression input, usually from one of the DITHER
+          or MERGE blocks.
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          Display Stream Compression output to the input of the next desired
+          component in the display pipeline, for example to MERGE, DP_INTF,
+          DPI or DSI.
+
+    required:
+      - port@0
+      - port@1
+
 required:
   - compatible
   - reg
diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dsi.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dsi.yaml
index a7aa8fcb0dd1..27ffbccc2a08 100644
--- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dsi.yaml
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dsi.yaml
@@ -77,6 +77,26 @@ properties:
       Output port node. This port should be connected to the input
       port of an attached DSI panel or DSI-to-eDP encoder chip.
 
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+    description:
+      Input ports can have multiple endpoints, each of those connects
+      to either the primary, secondary, etc, display pipeline.
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: DSI input port, usually from DITHER, DSC or MERGE
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          DSI output to an attached DSI panel, or a DSI-to-X encoder chip
+
+    required:
+      - port@0
+      - port@1
+
 required:
   - compatible
   - reg
@@ -86,7 +106,12 @@ required:
   - clock-names
   - phys
   - phy-names
-  - port
+
+oneOf:
+  - required:
+      - port
+  - required:
+      - ports
 
 unevaluatedProperties: false
 
diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,ethdr.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,ethdr.yaml
index 677882348ede..98db47894eeb 100644
--- a/Documentation/devicetree/bindings/display/mediatek/mediatek,ethdr.yaml
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,ethdr.yaml
@@ -110,6 +110,28 @@ properties:
       include/dt-bindings/gce/<chip>-gce.h, mapping to the register of display
       function block.
 
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+    description:
+      Input and output ports can have multiple endpoints, each of those
+      connects to either the primary, secondary, etc, display pipeline.
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: ETHDR input, usually from one of the MERGE blocks.
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          ETHDR output to the input of the next desired component in the
+          display pipeline, for example one of the available MERGE blocks,
+          or others.
+
+    required:
+      - port@0
+      - port@1
+
 required:
   - compatible
   - reg
diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma.yaml
index 6823d3ce5049..48542dc7e784 100644
--- a/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma.yaml
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma.yaml
@@ -65,6 +65,25 @@ properties:
     $ref: /schemas/types.yaml#/definitions/phandle-array
     maxItems: 1
 
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: GAMMA input, usually from one of the AAL blocks.
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          GAMMA output to the input of the next desired component in the
+          display pipeline, for example one of the available DITHER or
+          POSTMASK blocks.
+
+    required:
+      - port@0
+      - port@1
+
 required:
   - compatible
   - reg
diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,merge.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,merge.yaml
index dae839279950..0de9f64f3f84 100644
--- a/Documentation/devicetree/bindings/display/mediatek/mediatek,merge.yaml
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,merge.yaml
@@ -77,6 +77,29 @@ properties:
     $ref: /schemas/types.yaml#/definitions/phandle-array
     maxItems: 1
 
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+    description:
+      Input and output ports can have multiple endpoints, each of those
+      connects to either the primary, secondary, etc, display pipeline.
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          MERGE input port, usually from DITHER, DPI, DSC, DSI, MDP_RDMA,
+          ETHDR or even from a different MERGE block
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          MERGE output to a DSC, DPI, DP_INTF, DSI, ETHDR, Write DMA, or
+          a different MERGE block, or others.
+
+    required:
+      - port@0
+      - port@1
+
   resets:
     description: reset controller
       See Documentation/devicetree/bindings/reset/reset.txt for details.
diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,od.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,od.yaml
index 831c653caffd..71534febd49c 100644
--- a/Documentation/devicetree/bindings/display/mediatek/mediatek,od.yaml
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,od.yaml
@@ -38,6 +38,28 @@ properties:
     items:
       - description: OD Clock
 
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+    description:
+      Input and output ports can have multiple endpoints, each of those
+      connects to either the primary, secondary, etc, display pipeline.
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: OD input port, usually from an AAL block
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          OD output to the input of the next desired component in the
+          display pipeline, for example one of the available RDMA or
+          other blocks.
+
+    required:
+      - port@0
+      - port@1
+
 required:
   - compatible
   - reg
diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,ovl-2l.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,ovl-2l.yaml
index c7dd0ef02dcf..bacdfe7d08a6 100644
--- a/Documentation/devicetree/bindings/display/mediatek/mediatek,ovl-2l.yaml
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,ovl-2l.yaml
@@ -57,6 +57,28 @@ properties:
     $ref: /schemas/types.yaml#/definitions/phandle-array
     maxItems: 1
 
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+    description:
+      Input and output ports can have multiple endpoints, each of those
+      connects to either the primary, secondary, etc, display pipeline.
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: OVL input port from MMSYS, VDOSYS or other OVLs
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          OVL output to the input of the next desired component in the
+          display pipeline, for example one of the available COLOR, RDMA
+          or WDMA blocks.
+
+    required:
+      - port@0
+      - port@1
+
 required:
   - compatible
   - reg
diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,ovl.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,ovl.yaml
index d55611c7ce5e..9ea796a033b2 100644
--- a/Documentation/devicetree/bindings/display/mediatek/mediatek,ovl.yaml
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,ovl.yaml
@@ -75,6 +75,28 @@ properties:
     $ref: /schemas/types.yaml#/definitions/phandle-array
     maxItems: 1
 
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+    description:
+      Input and output ports can have multiple endpoints, each of those
+      connects to either the primary, secondary, etc, display pipeline.
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: OVL input port from MMSYS or one of multiple VDOSYS
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          OVL output to the input of the next desired component in the
+          display pipeline, for example one of the available COLOR, RDMA
+          or WDMA blocks.
+
+    required:
+      - port@0
+      - port@1
+
 required:
   - compatible
   - reg
diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,postmask.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,postmask.yaml
index 11fe32e50a59..fb6fe4742624 100644
--- a/Documentation/devicetree/bindings/display/mediatek/mediatek,postmask.yaml
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,postmask.yaml
@@ -52,6 +52,27 @@ properties:
     $ref: /schemas/types.yaml#/definitions/phandle-array
     maxItems: 1
 
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+    description:
+      Input and output ports can have multiple endpoints, each of those
+      connects to either the primary, secondary, etc, display pipeline.
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: POSTMASK input port, usually from GAMMA
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          POSTMASK output to the input of the next desired component in the
+          display pipeline, for example one of the available DITHER blocks.
+
+    required:
+      - port@0
+      - port@1
+
 required:
   - compatible
   - reg
diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,rdma.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,rdma.yaml
index 4cadb245d028..878f676b581f 100644
--- a/Documentation/devicetree/bindings/display/mediatek/mediatek,rdma.yaml
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,rdma.yaml
@@ -87,6 +87,28 @@ properties:
     $ref: /schemas/types.yaml#/definitions/phandle-array
     maxItems: 1
 
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+    description:
+      Input and output ports can have multiple endpoints, each of those
+      connects to either the primary, secondary, etc, display pipeline.
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: RDMA input port, usually from MMSYS, OD or OVL
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          RDMA output to the input of the next desired component in the
+          display pipeline, for example one of the available COLOR, DPI,
+          DSI, MERGE or UFOE blocks.
+
+    required:
+      - port@0
+      - port@1
+
 required:
   - compatible
   - reg
diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,ufoe.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,ufoe.yaml
index 39e3e2d4a0db..61a5e22effbf 100644
--- a/Documentation/devicetree/bindings/display/mediatek/mediatek,ufoe.yaml
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,ufoe.yaml
@@ -43,6 +43,27 @@ properties:
     items:
       - description: UFOe Clock
 
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+    description:
+      Input and output ports can have multiple endpoints, each of those
+      connects to either the primary, secondary, etc, display pipeline.
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: UFOE input, usually from one of the RDMA blocks.
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          UFOE output to the input of the next desired component in the
+          display pipeline, usually one of the available DSI blocks.
+
+    required:
+      - port@0
+      - port@1
+
 required:
   - compatible
   - reg
-- 
2.46.1

[PATCH v12 2/3] dt-bindings: arm: mediatek: mmsys: Add OF graph support for board path

[AngeloGioacchino Del Regno]

Document OF graph on MMSYS/VDOSYS: this supports up to three DDP paths
per HW instance (so potentially up to six displays for multi-vdo SoCs).

The MMSYS or VDOSYS is always the first component in the DDP pipeline,
so it only supports an output port with multiple endpoints - where each
endpoint defines the starting point for one of the (currently three)
possible hardware paths.

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
Tested-by: Alexandre Mergnat <amergnat@baylibre.com>
Reviewed-by: CK Hu <ck.hu@mediatek.com>
Tested-by: Michael Walle <mwalle@kernel.org> # on kontron-sbc-i1200
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 .../bindings/arm/mediatek/mediatek,mmsys.yaml | 28 +++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
index b3c6888c1457..3f4262e93c78 100644
--- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
+++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
@@ -93,6 +93,34 @@ properties:
   '#reset-cells':
     const: 1
 
+  port:
+    $ref: /schemas/graph.yaml#/properties/port
+    description:
+      Output port node. This port connects the MMSYS/VDOSYS output to
+      the first component of one display pipeline, for example one of
+      the available OVL or RDMA blocks.
+      Some MediaTek SoCs support multiple display outputs per MMSYS.
+    properties:
+      endpoint@0:
+        $ref: /schemas/graph.yaml#/properties/endpoint
+        description: Output to the primary display pipeline
+
+      endpoint@1:
+        $ref: /schemas/graph.yaml#/properties/endpoint
+        description: Output to the secondary display pipeline
+
+      endpoint@2:
+        $ref: /schemas/graph.yaml#/properties/endpoint
+        description: Output to the tertiary display pipeline
+
+    anyOf:
+      - required:
+          - endpoint@0
+      - required:
+          - endpoint@1
+      - required:
+          - endpoint@2
+
 required:
   - compatible
   - reg
-- 
2.46.1

[PATCH v12 3/3] drm/mediatek: Implement OF graphs support for display paths

[AngeloGioacchino Del Regno]

It is impossible to add each and every possible DDP path combination
for each and every possible combination of SoC and board: right now,
this driver hardcodes configuration for 10 SoCs and this is going to
grow larger and larger, and with new hacks like the introduction of
mtk_drm_route which is anyway not enough for all final routes as the
DSI cannot be connected to MERGE if it's not a dual-DSI, or enabling
DSC preventively doesn't work if the display doesn't support it, or
others.

Since practically all display IPs in MediaTek SoCs support being
interconnected with different instances of other, or the same, IPs
or with different IPs and in different combinations, the final DDP
pipeline is effectively a board specific configuration.

Implement OF graphs support to the mediatek-drm drivers, allowing to
stop hardcoding the paths, and preventing this driver to get a huge
amount of arrays for each board and SoC combination, also paving the
way to share the same mtk_mmsys_driver_data between multiple SoCs,
making it more straightforward to add support for new chips.

Note that the OVL_ADAPTOR software component driver needs relatively
big changes in order to fully support OF Graphs (and more SoCs anyway)
and such changes will come at a later time.
As of now, the mtk_disp_ovl_adaptor driver takes the MERGE components
(for example, on mt8195, merge 1 to 4) dynamically so, even though
later updates to the ovl-adaptor driver will *not* require bindings
changes, the merge1-4 will be temporarily omitted in the graph for the
MT8195 SoC.

This means that an example graph for this SoC looks like:

mdp_rdma (0 ~ 7) -> padding (0 ~ 7) -> ethdr -> merge5

and the resulting path in this driver will be `ovl_adaptor -> merge5`

Later updates to the ovl adaptor will expand it to support more SoCs
and, in turn, to also fully support graphs.

Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
Tested-by: Alexandre Mergnat <amergnat@baylibre.com>
Acked-by: Sui Jingfeng <sui.jingfeng@linux.dev>
Tested-by: Michael Walle <mwalle@kernel.org> # on kontron-sbc-i1200
Reviewed-by: CK Hu <ck.hu@mediatek.com>
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_disp_drv.h       |   1 +
 .../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c   |  43 ++-
 drivers/gpu/drm/mediatek/mtk_dpi.c            |  21 +-
 drivers/gpu/drm/mediatek/mtk_drm_drv.c        | 253 +++++++++++++++++-
 drivers/gpu/drm/mediatek/mtk_drm_drv.h        |   2 +-
 drivers/gpu/drm/mediatek/mtk_dsi.c            |  14 +-
 6 files changed, 312 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
index 082ac18fe04a..94843974851f 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
+++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
@@ -108,6 +108,7 @@ size_t mtk_ovl_get_num_formats(struct device *dev);
 
 void mtk_ovl_adaptor_add_comp(struct device *dev, struct mtk_mutex *mutex);
 void mtk_ovl_adaptor_remove_comp(struct device *dev, struct mtk_mutex *mutex);
+bool mtk_ovl_adaptor_is_comp_present(struct device_node *node);
 void mtk_ovl_adaptor_connect(struct device *dev, struct device *mmsys_dev,
 			     unsigned int next);
 void mtk_ovl_adaptor_disconnect(struct device *dev, struct device *mmsys_dev,
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
index c6768210b08b..4e064d3c97cc 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
@@ -490,6 +490,41 @@ static int compare_of(struct device *dev, void *data)
 	return dev->of_node == data;
 }
 
+static int ovl_adaptor_of_get_ddp_comp_type(struct device_node *node,
+					    enum mtk_ovl_adaptor_comp_type *ctype)
+{
+	const struct of_device_id *of_id = of_match_node(mtk_ovl_adaptor_comp_dt_ids, node);
+
+	if (!of_id)
+		return -EINVAL;
+
+	*ctype = (enum mtk_ovl_adaptor_comp_type)((uintptr_t)of_id->data);
+
+	return 0;
+}
+
+bool mtk_ovl_adaptor_is_comp_present(struct device_node *node)
+{
+	enum mtk_ovl_adaptor_comp_type type;
+	int ret;
+
+	ret = ovl_adaptor_of_get_ddp_comp_type(node, &type);
+	if (ret)
+		return false;
+
+	if (type >= OVL_ADAPTOR_TYPE_NUM)
+		return false;
+
+	/*
+	 * In the context of mediatek-drm, ETHDR, MDP_RDMA and Padding are
+	 * used exclusively by OVL Adaptor: if this component is not one of
+	 * those, it's likely not an OVL Adaptor path.
+	 */
+	return type == OVL_ADAPTOR_TYPE_ETHDR ||
+	       type == OVL_ADAPTOR_TYPE_MDP_RDMA ||
+	       type == OVL_ADAPTOR_TYPE_PADDING;
+}
+
 static int ovl_adaptor_comp_init(struct device *dev, struct component_match **match)
 {
 	struct mtk_disp_ovl_adaptor *priv = dev_get_drvdata(dev);
@@ -499,12 +534,11 @@ static int ovl_adaptor_comp_init(struct device *dev, struct component_match **ma
 	parent = dev->parent->parent->of_node->parent;
 
 	for_each_child_of_node_scoped(parent, node) {
-		const struct of_device_id *of_id;
 		enum mtk_ovl_adaptor_comp_type type;
-		int id;
+		int id, ret;
 
-		of_id = of_match_node(mtk_ovl_adaptor_comp_dt_ids, node);
-		if (!of_id)
+		ret = ovl_adaptor_of_get_ddp_comp_type(node, &type);
+		if (ret)
 			continue;
 
 		if (!of_device_is_available(node)) {
@@ -513,7 +547,6 @@ static int ovl_adaptor_comp_init(struct device *dev, struct component_match **ma
 			continue;
 		}
 
-		type = (enum mtk_ovl_adaptor_comp_type)(uintptr_t)of_id->data;
 		id = ovl_adaptor_comp_get_id(dev, node, type);
 		if (id < 0) {
 			dev_warn(dev, "Skipping unknown component %pOF\n",
diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c
index e4da5cc75e60..ad37b0ecafb9 100644
--- a/drivers/gpu/drm/mediatek/mtk_dpi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
@@ -755,6 +755,20 @@ static int mtk_dpi_bridge_attach(struct drm_bridge *bridge,
 				 enum drm_bridge_attach_flags flags)
 {
 	struct mtk_dpi *dpi = bridge_to_dpi(bridge);
+	int ret;
+
+	dpi->next_bridge = devm_drm_of_get_bridge(dpi->dev, dpi->dev->of_node, 1, -1);
+	if (IS_ERR(dpi->next_bridge)) {
+		ret = PTR_ERR(dpi->next_bridge);
+		if (ret == -EPROBE_DEFER)
+			return ret;
+
+		/* Old devicetree has only one endpoint */
+		dpi->next_bridge = devm_drm_of_get_bridge(dpi->dev, dpi->dev->of_node, 0, 0);
+		if (IS_ERR(dpi->next_bridge))
+			return dev_err_probe(dpi->dev, PTR_ERR(dpi->next_bridge),
+					     "Failed to get bridge\n");
+	}
 
 	return drm_bridge_attach(bridge->encoder, dpi->next_bridge,
 				 &dpi->bridge, flags);
@@ -1162,13 +1176,6 @@ static int mtk_dpi_probe(struct platform_device *pdev)
 	if (dpi->irq < 0)
 		return dpi->irq;
 
-	dpi->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0);
-	if (IS_ERR(dpi->next_bridge))
-		return dev_err_probe(dev, PTR_ERR(dpi->next_bridge),
-				     "Failed to get bridge\n");
-
-	dev_info(dev, "Found bridge node: %pOF\n", dpi->next_bridge->of_node);
-
 	platform_set_drvdata(pdev, dpi);
 
 	dpi->bridge.funcs = &mtk_dpi_bridge_funcs;
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index a4594f8873d5..85be035a209a 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -27,6 +27,7 @@
 
 #include "mtk_crtc.h"
 #include "mtk_ddp_comp.h"
+#include "mtk_disp_drv.h"
 #include "mtk_drm_drv.h"
 #include "mtk_gem.h"
 
@@ -820,12 +821,235 @@ static const struct of_device_id mtk_ddp_comp_dt_ids[] = {
 	{ }
 };
 
+static int mtk_drm_of_get_ddp_comp_type(struct device_node *node, enum mtk_ddp_comp_type *ctype)
+{
+	const struct of_device_id *of_id = of_match_node(mtk_ddp_comp_dt_ids, node);
+
+	if (!of_id)
+		return -EINVAL;
+
+	*ctype = (enum mtk_ddp_comp_type)((uintptr_t)of_id->data);
+
+	return 0;
+}
+
+static int mtk_drm_of_get_ddp_ep_cid(struct device_node *node,
+				     int output_port, enum mtk_crtc_path crtc_path,
+				     struct device_node **next, unsigned int *cid)
+{
+	struct device_node *ep_dev_node, *ep_out;
+	enum mtk_ddp_comp_type comp_type;
+	int ret;
+
+	ep_out = of_graph_get_endpoint_by_regs(node, output_port, crtc_path);
+	if (!ep_out)
+		return -ENOENT;
+
+	ep_dev_node = of_graph_get_remote_port_parent(ep_out);
+	of_node_put(ep_out);
+	if (!ep_dev_node)
+		return -EINVAL;
+
+	/*
+	 * Pass the next node pointer regardless of failures in the later code
+	 * so that if this function is called in a loop it will walk through all
+	 * of the subsequent endpoints anyway.
+	 */
+	*next = ep_dev_node;
+
+	if (!of_device_is_available(ep_dev_node))
+		return -ENODEV;
+
+	ret = mtk_drm_of_get_ddp_comp_type(ep_dev_node, &comp_type);
+	if (ret) {
+		if (mtk_ovl_adaptor_is_comp_present(ep_dev_node)) {
+			*cid = (unsigned int)DDP_COMPONENT_DRM_OVL_ADAPTOR;
+			return 0;
+		}
+		return ret;
+	}
+
+	ret = mtk_ddp_comp_get_id(ep_dev_node, comp_type);
+	if (ret < 0)
+		return ret;
+
+	/* All ok! Pass the Component ID to the caller. */
+	*cid = (unsigned int)ret;
+
+	return 0;
+}
+
+/**
+ * mtk_drm_of_ddp_path_build_one - Build a Display HW Pipeline for a CRTC Path
+ * @dev:          The mediatek-drm device
+ * @cpath:        CRTC Path relative to a VDO or MMSYS
+ * @out_path:     Pointer to an array that will contain the new pipeline
+ * @out_path_len: Number of entries in the pipeline array
+ *
+ * MediaTek SoCs can use different DDP hardware pipelines (or paths) depending
+ * on the board-specific desired display configuration; this function walks
+ * through all of the output endpoints starting from a VDO or MMSYS hardware
+ * instance and builds the right pipeline as specified in device trees.
+ *
+ * Return:
+ * * %0       - Display HW Pipeline successfully built and validated
+ * * %-ENOENT - Display pipeline was not specified in device tree
+ * * %-EINVAL - Display pipeline built but validation failed
+ * * %-ENOMEM - Failure to allocate pipeline array to pass to the caller
+ */
+static int mtk_drm_of_ddp_path_build_one(struct device *dev, enum mtk_crtc_path cpath,
+					 const unsigned int **out_path,
+					 unsigned int *out_path_len)
+{
+	struct device_node *next, *prev, *vdo = dev->parent->of_node;
+	unsigned int temp_path[DDP_COMPONENT_DRM_ID_MAX] = { 0 };
+	unsigned int *final_ddp_path;
+	unsigned short int idx = 0;
+	bool ovl_adaptor_comp_added = false;
+	int ret;
+
+	/* Get the first entry for the temp_path array */
+	ret = mtk_drm_of_get_ddp_ep_cid(vdo, 0, cpath, &next, &temp_path[idx]);
+	if (ret) {
+		if (next && temp_path[idx] == DDP_COMPONENT_DRM_OVL_ADAPTOR) {
+			dev_dbg(dev, "Adding OVL Adaptor for %pOF\n", next);
+			ovl_adaptor_comp_added = true;
+		} else {
+			if (next)
+				dev_err(dev, "Invalid component %pOF\n", next);
+			else
+				dev_err(dev, "Cannot find first endpoint for path %d\n", cpath);
+
+			return ret;
+		}
+	}
+	idx++;
+
+	/*
+	 * Walk through port outputs until we reach the last valid mediatek-drm component.
+	 * To be valid, this must end with an "invalid" component that is a display node.
+	 */
+	do {
+		prev = next;
+		ret = mtk_drm_of_get_ddp_ep_cid(next, 1, cpath, &next, &temp_path[idx]);
+		of_node_put(prev);
+		if (ret) {
+			of_node_put(next);
+			break;
+		}
+
+		/*
+		 * If this is an OVL adaptor exclusive component and one of those
+		 * was already added, don't add another instance of the generic
+		 * DDP_COMPONENT_OVL_ADAPTOR, as this is used only to decide whether
+		 * to probe that component master driver of which only one instance
+		 * is needed and possible.
+		 */
+		if (temp_path[idx] == DDP_COMPONENT_DRM_OVL_ADAPTOR) {
+			if (!ovl_adaptor_comp_added)
+				ovl_adaptor_comp_added = true;
+			else
+				idx--;
+		}
+	} while (++idx < DDP_COMPONENT_DRM_ID_MAX);
+
+	/*
+	 * The device component might not be enabled: in that case, don't
+	 * check the last entry and just report that the device is missing.
+	 */
+	if (ret == -ENODEV)
+		return ret;
+
+	/* If the last entry is not a final display output, the configuration is wrong */
+	switch (temp_path[idx - 1]) {
+	case DDP_COMPONENT_DP_INTF0:
+	case DDP_COMPONENT_DP_INTF1:
+	case DDP_COMPONENT_DPI0:
+	case DDP_COMPONENT_DPI1:
+	case DDP_COMPONENT_DSI0:
+	case DDP_COMPONENT_DSI1:
+	case DDP_COMPONENT_DSI2:
+	case DDP_COMPONENT_DSI3:
+		break;
+	default:
+		dev_err(dev, "Invalid display hw pipeline. Last component: %d (ret=%d)\n",
+			temp_path[idx - 1], ret);
+		return -EINVAL;
+	}
+
+	final_ddp_path = devm_kmemdup(dev, temp_path, idx * sizeof(temp_path[0]), GFP_KERNEL);
+	if (!final_ddp_path)
+		return -ENOMEM;
+
+	dev_dbg(dev, "Display HW Pipeline built with %d components.\n", idx);
+
+	/* Pipeline built! */
+	*out_path = final_ddp_path;
+	*out_path_len = idx;
+
+	return 0;
+}
+
+static int mtk_drm_of_ddp_path_build(struct device *dev, struct device_node *node,
+				     struct mtk_mmsys_driver_data *data)
+{
+	struct device_node *ep_node;
+	struct of_endpoint of_ep;
+	bool output_present[MAX_CRTC] = { false };
+	int ret;
+
+	for_each_endpoint_of_node(node, ep_node) {
+		ret = of_graph_parse_endpoint(ep_node, &of_ep);
+		if (ret) {
+			dev_err_probe(dev, ret, "Cannot parse endpoint\n");
+			break;
+		}
+
+		if (of_ep.id >= MAX_CRTC) {
+			ret = dev_err_probe(dev, -EINVAL,
+					    "Invalid endpoint%u number\n", of_ep.port);
+			break;
+		}
+
+		output_present[of_ep.id] = true;
+	}
+
+	if (ret) {
+		of_node_put(ep_node);
+		return ret;
+	}
+
+	if (output_present[CRTC_MAIN]) {
+		ret = mtk_drm_of_ddp_path_build_one(dev, CRTC_MAIN,
+						    &data->main_path, &data->main_len);
+		if (ret && ret != -ENODEV)
+			return ret;
+	}
+
+	if (output_present[CRTC_EXT]) {
+		ret = mtk_drm_of_ddp_path_build_one(dev, CRTC_EXT,
+						    &data->ext_path, &data->ext_len);
+		if (ret && ret != -ENODEV)
+			return ret;
+	}
+
+	if (output_present[CRTC_THIRD]) {
+		ret = mtk_drm_of_ddp_path_build_one(dev, CRTC_THIRD,
+						    &data->third_path, &data->third_len);
+		if (ret && ret != -ENODEV)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int mtk_drm_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *phandle = dev->parent->of_node;
 	const struct of_device_id *of_id;
 	struct mtk_drm_private *private;
+	struct mtk_mmsys_driver_data *mtk_drm_data;
 	struct device_node *node;
 	struct component_match *match = NULL;
 	struct platform_device *ovl_adaptor;
@@ -846,7 +1070,27 @@ static int mtk_drm_probe(struct platform_device *pdev)
 	if (!of_id)
 		return -ENODEV;
 
-	private->data = of_id->data;
+	mtk_drm_data = (struct mtk_mmsys_driver_data *)of_id->data;
+	if (!mtk_drm_data)
+		return -EINVAL;
+
+	/* Try to build the display pipeline from devicetree graphs */
+	if (of_graph_is_present(phandle)) {
+		dev_dbg(dev, "Building display pipeline for MMSYS %u\n",
+			mtk_drm_data->mmsys_id);
+		private->data = devm_kmemdup(dev, mtk_drm_data,
+					     sizeof(*mtk_drm_data), GFP_KERNEL);
+		if (!private->data)
+			return -ENOMEM;
+
+		ret = mtk_drm_of_ddp_path_build(dev, phandle, private->data);
+		if (ret)
+			return ret;
+	} else {
+		/* No devicetree graphs support: go with hardcoded paths if present */
+		dev_dbg(dev, "Using hardcoded paths for MMSYS %u\n", mtk_drm_data->mmsys_id);
+		private->data = mtk_drm_data;
+	};
 
 	private->all_drm_private = devm_kmalloc_array(dev, private->data->mmsys_dev_num,
 						      sizeof(*private->all_drm_private),
@@ -868,12 +1112,11 @@ static int mtk_drm_probe(struct platform_device *pdev)
 
 	/* Iterate over sibling DISP function blocks */
 	for_each_child_of_node(phandle->parent, node) {
-		const struct of_device_id *of_id;
 		enum mtk_ddp_comp_type comp_type;
 		int comp_id;
 
-		of_id = of_match_node(mtk_ddp_comp_dt_ids, node);
-		if (!of_id)
+		ret = mtk_drm_of_get_ddp_comp_type(node, &comp_type);
+		if (ret)
 			continue;
 
 		if (!of_device_is_available(node)) {
@@ -882,8 +1125,6 @@ static int mtk_drm_probe(struct platform_device *pdev)
 			continue;
 		}
 
-		comp_type = (enum mtk_ddp_comp_type)(uintptr_t)of_id->data;
-
 		if (comp_type == MTK_DISP_MUTEX) {
 			int id;
 
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
index ce897984de51..675cdc90a440 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
@@ -63,7 +63,7 @@ struct mtk_drm_private {
 	struct device *mmsys_dev;
 	struct device_node *comp_node[DDP_COMPONENT_DRM_ID_MAX];
 	struct mtk_ddp_comp ddp_comp[DDP_COMPONENT_DRM_ID_MAX];
-	const struct mtk_mmsys_driver_data *data;
+	struct mtk_mmsys_driver_data *data;
 	struct drm_atomic_state *suspend_state;
 	unsigned int mbox_index;
 	struct mtk_drm_private **all_drm_private;
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index eeec641cab60..33ceeb8d6925 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -988,9 +988,17 @@ static int mtk_dsi_host_attach(struct mipi_dsi_host *host,
 	dsi->lanes = device->lanes;
 	dsi->format = device->format;
 	dsi->mode_flags = device->mode_flags;
-	dsi->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0);
-	if (IS_ERR(dsi->next_bridge))
-		return PTR_ERR(dsi->next_bridge);
+	dsi->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
+	if (IS_ERR(dsi->next_bridge)) {
+		ret = PTR_ERR(dsi->next_bridge);
+		if (ret == -EPROBE_DEFER)
+			return ret;
+
+		/* Old devicetree has only one endpoint */
+		dsi->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0);
+		if (IS_ERR(dsi->next_bridge))
+			return PTR_ERR(dsi->next_bridge);
+	}
 
 	drm_bridge_add(&dsi->bridge);
 
-- 
2.46.1

Re: [PATCH v12 1/3] dt-bindings: display: mediatek: Add OF graph support for board path

[Rob Herring]

On Mon, Oct 14, 2024 at 3:51 AM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> The display IPs in MediaTek SoCs support being interconnected with
> different instances of DDP IPs (for example, merge0 or merge1) and/or
> with different DDP IPs (for example, rdma can be connected with either
> color, dpi, dsi, merge, etc), forming a full Display Data Path that
> ends with an actual display.
>
> The final display pipeline is effectively board specific, as it does
> depend on the display that is attached to it, and eventually on the
> sensors supported by the board (for example, Adaptive Ambient Light
> would need an Ambient Light Sensor, otherwise it's pointless!), other
> than the output type.
>
> Add support for OF graphs to most of the MediaTek DDP (display) bindings
> to add flexibility to build custom hardware paths, hence enabling board
> specific configuration of the display pipeline and allowing to finally
> migrate away from using hardcoded paths.
>
> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
> Tested-by: Alexandre Mergnat <amergnat@baylibre.com>
> Reviewed-by: CK Hu <ck.hu@mediatek.com>
> Tested-by: Michael Walle <mwalle@kernel.org> # on kontron-sbc-i1200
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  .../display/mediatek/mediatek,aal.yaml        | 40 +++++++++++++++++++
>  .../display/mediatek/mediatek,ccorr.yaml      | 21 ++++++++++
>  .../display/mediatek/mediatek,color.yaml      | 22 ++++++++++
>  .../display/mediatek/mediatek,dither.yaml     | 22 ++++++++++
>  .../display/mediatek/mediatek,dpi.yaml        | 25 +++++++++++-
>  .../display/mediatek/mediatek,dsc.yaml        | 24 +++++++++++
>  .../display/mediatek/mediatek,dsi.yaml        | 27 ++++++++++++-
>  .../display/mediatek/mediatek,ethdr.yaml      | 22 ++++++++++
>  .../display/mediatek/mediatek,gamma.yaml      | 19 +++++++++
>  .../display/mediatek/mediatek,merge.yaml      | 23 +++++++++++
>  .../display/mediatek/mediatek,od.yaml         | 22 ++++++++++
>  .../display/mediatek/mediatek,ovl-2l.yaml     | 22 ++++++++++
>  .../display/mediatek/mediatek,ovl.yaml        | 22 ++++++++++
>  .../display/mediatek/mediatek,postmask.yaml   | 21 ++++++++++
>  .../display/mediatek/mediatek,rdma.yaml       | 22 ++++++++++
>  .../display/mediatek/mediatek,ufoe.yaml       | 21 ++++++++++
>  16 files changed, 372 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
> index cf24434854ff..47ddba5c41af 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
> @@ -62,6 +62,27 @@ properties:
>      $ref: /schemas/types.yaml#/definitions/phandle-array
>      maxItems: 1
>
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +    description:
> +      Input and output ports can have multiple endpoints, each of those
> +      connects to either the primary, secondary, etc, display pipeline.
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: AAL input port
> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description:
> +          AAL output to the next component's input, for example could be one
> +          of many gamma, overdrive or other blocks.
> +
> +    required:
> +      - port@0
> +      - port@1
> +
>  required:
>    - compatible
>    - reg
> @@ -89,5 +110,24 @@ examples:
>             power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
>             clocks = <&mmsys CLK_MM_DISP_AAL>;
>             mediatek,gce-client-reg = <&gce SUBSYS_1401XXXX 0x5000 0x1000>;
> +
> +           ports {
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +
> +               port@0 {
> +                   reg = <0>;
> +                   aal0_in: endpoint {
> +                       remote-endpoint = <&ccorr0_out>;
> +                   };
> +               };
> +
> +               port@1 {
> +                   reg = <1>;
> +                   aal0_out: endpoint {
> +                       remote-endpoint = <&gamma0_in>;
> +                   };
> +               };
> +           };
>         };
>      };
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
> index 9f8366763831..fca8e7bb0cbc 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
> @@ -57,6 +57,27 @@ properties:
>      $ref: /schemas/types.yaml#/definitions/phandle-array
>      maxItems: 1
>
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +    description:
> +      Input and output ports can have multiple endpoints, each of those
> +      connects to either the primary, secondary, etc, display pipeline.
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: CCORR input port
> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description:
> +          CCORR output to the input of the next desired component in the
> +          display pipeline, usually only one of the available AAL blocks.
> +
> +    required:
> +      - port@0
> +      - port@1
> +
>  required:
>    - compatible
>    - reg
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
> index 7df786bbad20..6160439ce4d7 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
> @@ -65,6 +65,28 @@ properties:
>      $ref: /schemas/types.yaml#/definitions/phandle-array
>      maxItems: 1
>
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +    description:
> +      Input and output ports can have multiple endpoints, each of those
> +      connects to either the primary, secondary, etc, display pipeline.
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: COLOR input port
> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description:
> +          COLOR output to the input of the next desired component in the
> +          display pipeline, for example one of the available CCORR or AAL
> +          blocks.
> +
> +    required:
> +      - port@0
> +      - port@1
> +
>  required:
>    - compatible
>    - reg
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
> index 6fceb1f95d2a..abaf27916d13 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
> @@ -56,6 +56,28 @@ properties:
>      $ref: /schemas/types.yaml#/definitions/phandle-array
>      maxItems: 1
>
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +    description:
> +      Input and output ports can have multiple endpoints, each of those
> +      connects to either the primary, secondary, etc, display pipeline.
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: DITHER input, usually from a POSTMASK or GAMMA block.
> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description:
> +          DITHER output to the input of the next desired component in the
> +          display pipeline, for example one of the available DSC compressors,
> +          DP_INTF, DSI, LVDS or others.
> +
> +    required:
> +      - port@0
> +      - port@1
> +
>  required:
>    - compatible
>    - reg
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
> index 3a82aec9021c..b567e3d58aa1 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
> @@ -71,13 +71,34 @@ properties:
>        Output port node. This port should be connected to the input port of an
>        attached HDMI, LVDS or DisplayPort encoder chip.
>
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: DPI input port
> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: DPI output to an HDMI, LVDS or DisplayPort encoder input

This is wrong. The existing 'port' is the output. 'port' and 'port@0'
are treated as the same thing. Since you are adding an input port, the
new port has to be 'port@1' (or any number but 0).

I haven't looked at the driver code, but it should request port 0 and
always get the output port. And requesting port 1 will return an error
or the input port.

Rob

Re: [PATCH v12 1/3] dt-bindings: display: mediatek: Add OF graph support for board path

[AngeloGioacchino Del Regno]

Il 14/10/24 19:36, Rob Herring ha scritto:
> On Mon, Oct 14, 2024 at 3:51 AM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> The display IPs in MediaTek SoCs support being interconnected with
>> different instances of DDP IPs (for example, merge0 or merge1) and/or
>> with different DDP IPs (for example, rdma can be connected with either
>> color, dpi, dsi, merge, etc), forming a full Display Data Path that
>> ends with an actual display.
>>
>> The final display pipeline is effectively board specific, as it does
>> depend on the display that is attached to it, and eventually on the
>> sensors supported by the board (for example, Adaptive Ambient Light
>> would need an Ambient Light Sensor, otherwise it's pointless!), other
>> than the output type.
>>
>> Add support for OF graphs to most of the MediaTek DDP (display) bindings
>> to add flexibility to build custom hardware paths, hence enabling board
>> specific configuration of the display pipeline and allowing to finally
>> migrate away from using hardcoded paths.
>>
>> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
>> Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
>> Tested-by: Alexandre Mergnat <amergnat@baylibre.com>
>> Reviewed-by: CK Hu <ck.hu@mediatek.com>
>> Tested-by: Michael Walle <mwalle@kernel.org> # on kontron-sbc-i1200
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   .../display/mediatek/mediatek,aal.yaml        | 40 +++++++++++++++++++
>>   .../display/mediatek/mediatek,ccorr.yaml      | 21 ++++++++++
>>   .../display/mediatek/mediatek,color.yaml      | 22 ++++++++++
>>   .../display/mediatek/mediatek,dither.yaml     | 22 ++++++++++
>>   .../display/mediatek/mediatek,dpi.yaml        | 25 +++++++++++-
>>   .../display/mediatek/mediatek,dsc.yaml        | 24 +++++++++++
>>   .../display/mediatek/mediatek,dsi.yaml        | 27 ++++++++++++-
>>   .../display/mediatek/mediatek,ethdr.yaml      | 22 ++++++++++
>>   .../display/mediatek/mediatek,gamma.yaml      | 19 +++++++++
>>   .../display/mediatek/mediatek,merge.yaml      | 23 +++++++++++
>>   .../display/mediatek/mediatek,od.yaml         | 22 ++++++++++
>>   .../display/mediatek/mediatek,ovl-2l.yaml     | 22 ++++++++++
>>   .../display/mediatek/mediatek,ovl.yaml        | 22 ++++++++++
>>   .../display/mediatek/mediatek,postmask.yaml   | 21 ++++++++++
>>   .../display/mediatek/mediatek,rdma.yaml       | 22 ++++++++++
>>   .../display/mediatek/mediatek,ufoe.yaml       | 21 ++++++++++
>>   16 files changed, 372 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
>> index cf24434854ff..47ddba5c41af 100644
>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
>> @@ -62,6 +62,27 @@ properties:
>>       $ref: /schemas/types.yaml#/definitions/phandle-array
>>       maxItems: 1
>>
>> +  ports:
>> +    $ref: /schemas/graph.yaml#/properties/ports
>> +    description:
>> +      Input and output ports can have multiple endpoints, each of those
>> +      connects to either the primary, secondary, etc, display pipeline.
>> +
>> +    properties:
>> +      port@0:
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +        description: AAL input port
>> +
>> +      port@1:
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +        description:
>> +          AAL output to the next component's input, for example could be one
>> +          of many gamma, overdrive or other blocks.
>> +
>> +    required:
>> +      - port@0
>> +      - port@1
>> +
>>   required:
>>     - compatible
>>     - reg
>> @@ -89,5 +110,24 @@ examples:
>>              power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
>>              clocks = <&mmsys CLK_MM_DISP_AAL>;
>>              mediatek,gce-client-reg = <&gce SUBSYS_1401XXXX 0x5000 0x1000>;
>> +
>> +           ports {
>> +               #address-cells = <1>;
>> +               #size-cells = <0>;
>> +
>> +               port@0 {
>> +                   reg = <0>;
>> +                   aal0_in: endpoint {
>> +                       remote-endpoint = <&ccorr0_out>;
>> +                   };
>> +               };
>> +
>> +               port@1 {
>> +                   reg = <1>;
>> +                   aal0_out: endpoint {
>> +                       remote-endpoint = <&gamma0_in>;
>> +                   };
>> +               };
>> +           };
>>          };
>>       };
>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
>> index 9f8366763831..fca8e7bb0cbc 100644
>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
>> @@ -57,6 +57,27 @@ properties:
>>       $ref: /schemas/types.yaml#/definitions/phandle-array
>>       maxItems: 1
>>
>> +  ports:
>> +    $ref: /schemas/graph.yaml#/properties/ports
>> +    description:
>> +      Input and output ports can have multiple endpoints, each of those
>> +      connects to either the primary, secondary, etc, display pipeline.
>> +
>> +    properties:
>> +      port@0:
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +        description: CCORR input port
>> +
>> +      port@1:
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +        description:
>> +          CCORR output to the input of the next desired component in the
>> +          display pipeline, usually only one of the available AAL blocks.
>> +
>> +    required:
>> +      - port@0
>> +      - port@1
>> +
>>   required:
>>     - compatible
>>     - reg
>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
>> index 7df786bbad20..6160439ce4d7 100644
>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
>> @@ -65,6 +65,28 @@ properties:
>>       $ref: /schemas/types.yaml#/definitions/phandle-array
>>       maxItems: 1
>>
>> +  ports:
>> +    $ref: /schemas/graph.yaml#/properties/ports
>> +    description:
>> +      Input and output ports can have multiple endpoints, each of those
>> +      connects to either the primary, secondary, etc, display pipeline.
>> +
>> +    properties:
>> +      port@0:
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +        description: COLOR input port
>> +
>> +      port@1:
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +        description:
>> +          COLOR output to the input of the next desired component in the
>> +          display pipeline, for example one of the available CCORR or AAL
>> +          blocks.
>> +
>> +    required:
>> +      - port@0
>> +      - port@1
>> +
>>   required:
>>     - compatible
>>     - reg
>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
>> index 6fceb1f95d2a..abaf27916d13 100644
>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
>> @@ -56,6 +56,28 @@ properties:
>>       $ref: /schemas/types.yaml#/definitions/phandle-array
>>       maxItems: 1
>>
>> +  ports:
>> +    $ref: /schemas/graph.yaml#/properties/ports
>> +    description:
>> +      Input and output ports can have multiple endpoints, each of those
>> +      connects to either the primary, secondary, etc, display pipeline.
>> +
>> +    properties:
>> +      port@0:
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +        description: DITHER input, usually from a POSTMASK or GAMMA block.
>> +
>> +      port@1:
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +        description:
>> +          DITHER output to the input of the next desired component in the
>> +          display pipeline, for example one of the available DSC compressors,
>> +          DP_INTF, DSI, LVDS or others.
>> +
>> +    required:
>> +      - port@0
>> +      - port@1
>> +
>>   required:
>>     - compatible
>>     - reg
>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
>> index 3a82aec9021c..b567e3d58aa1 100644
>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
>> @@ -71,13 +71,34 @@ properties:
>>         Output port node. This port should be connected to the input port of an
>>         attached HDMI, LVDS or DisplayPort encoder chip.
>>
>> +  ports:
>> +    $ref: /schemas/graph.yaml#/properties/ports
>> +
>> +    properties:
>> +      port@0:
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +        description: DPI input port
>> +
>> +      port@1:
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +        description: DPI output to an HDMI, LVDS or DisplayPort encoder input
> 
> This is wrong. The existing 'port' is the output. 'port' and 'port@0'
> are treated as the same thing. Since you are adding an input port, the
> new port has to be 'port@1' (or any number but 0).
> 
> I haven't looked at the driver code, but it should request port 0 and
> always get the output port. And requesting port 1 will return an error
> or the input port.

Hello Rob,

I want to remind you that in v2 of this series you said that it'd be wrong for
port@0 to be an output, I replied that you misread that as I had modeled it indeed
as an input, and then you gave me your Reviewed-by tag.

Anyway - I get your concern about the previous behavior of `port`, but I chose to
model this that way purely for consistency.

First of all - the driver(s) will check if we're feeding a full graph, as it will
indeed first check if port@1 is present: if it is, then it follows this scheme with
port@0 as INPUT and port@1 as OUTPUT.
If the component in port@0 is an OUTPUT, the bridge attach will fail.

Getting to bindings themselves, then... it would be a mistake to model port@0 as an
output and port@1 as an input, because that would be not only inconsistent with the
DRM Bridge bindings, but would be highly confusing when reading the devicetree.

Please note that the bridge bindings are always declaring port@0 as an INPUT and
other ports as OUTPUT(s).

As an example, you can check display/bridge/analogix,anx7625.yaml or
display/bridge/samsung,mipi-dsim.yaml (and others) for bridges, otherwise
display/st,stm32mp25-lvds.yaml or display/allwinner,sun4i-a10-display-frontend.yaml
(and others) for display controllers, which do all conform to this logic, where
the input is always @0, and the output is @1.

Of course, doing this required me to do extra changes to the MTK DRM drivers to
actually be retro-compatible with the old devicetrees as I explained before.

Just for clarity, if I were to model this with port@0 OUTPUT and @1 INPUT, we would
see in devicetree something like:

dpi-node@somewhere {
	ports {
		some_output_1: port@0 {
			remote-endpoint = <&some_input_2>;
		};
		some_input_1: port@1 {
			remote-endpoint = <&some_output_0>;
		};
};

/* already existing bridge binding, not touched by this commit */
bridge@somewhere-else {
	ports {
		some_input_2: port@0 {
			remote-endpoint = <&some_output_1>;
		};
		some_output_2: port@1 {
			remote-endpoint = <&to_display_input>;
		};
};

...and I think that you agree with me that this would be at least confusing for
whoever reads the DT (and again, IMO, inconsistent and simply wrong).

Instead, with the model proposed in this commit, we will have consistency:

dpi-node@somewhere {
	ports {
		some_input_1: port@1 {
			remote-endpoint = <&some_output_0>;
		};
		some_output_1: port@0 {
			remote-endpoint = <&some_input_2>;
		};
};

/* already existing bridge binding, not touched by this commit */
bridge@somewhere-else {
	ports {
		some_input_2: port@0 {
			remote-endpoint = <&some_output_1>;
		};
		some_output_2: port@1 {
			remote-endpoint = <&to_display_input>;
		};
};

...still, again, all this while still supporting the old device trees (which I plan
to update as soon as possible anyway, so that they're all using the full graph
instead of hardcoding board specific paths in the drivers).

Does this clarify to you the reasons why this was done like that?
If you have any other questions, I will be happy to clarify.

Cheers,
Angelo

Re: [PATCH v12 3/3] drm/mediatek: Implement OF graphs support for display paths

[Rob Herring]

On Mon, Oct 14, 2024 at 10:51:48AM +0200, AngeloGioacchino Del Regno wrote:
> It is impossible to add each and every possible DDP path combination
> for each and every possible combination of SoC and board: right now,
> this driver hardcodes configuration for 10 SoCs and this is going to
> grow larger and larger, and with new hacks like the introduction of
> mtk_drm_route which is anyway not enough for all final routes as the
> DSI cannot be connected to MERGE if it's not a dual-DSI, or enabling
> DSC preventively doesn't work if the display doesn't support it, or
> others.
> 
> Since practically all display IPs in MediaTek SoCs support being
> interconnected with different instances of other, or the same, IPs
> or with different IPs and in different combinations, the final DDP
> pipeline is effectively a board specific configuration.
> 
> Implement OF graphs support to the mediatek-drm drivers, allowing to
> stop hardcoding the paths, and preventing this driver to get a huge
> amount of arrays for each board and SoC combination, also paving the
> way to share the same mtk_mmsys_driver_data between multiple SoCs,
> making it more straightforward to add support for new chips.
> 
> Note that the OVL_ADAPTOR software component driver needs relatively
> big changes in order to fully support OF Graphs (and more SoCs anyway)
> and such changes will come at a later time.
> As of now, the mtk_disp_ovl_adaptor driver takes the MERGE components
> (for example, on mt8195, merge 1 to 4) dynamically so, even though
> later updates to the ovl-adaptor driver will *not* require bindings
> changes, the merge1-4 will be temporarily omitted in the graph for the
> MT8195 SoC.
> 
> This means that an example graph for this SoC looks like:
> 
> mdp_rdma (0 ~ 7) -> padding (0 ~ 7) -> ethdr -> merge5
> 
> and the resulting path in this driver will be `ovl_adaptor -> merge5`
> 
> Later updates to the ovl adaptor will expand it to support more SoCs
> and, in turn, to also fully support graphs.
> 
> Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
> Tested-by: Alexandre Mergnat <amergnat@baylibre.com>
> Acked-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> Tested-by: Michael Walle <mwalle@kernel.org> # on kontron-sbc-i1200
> Reviewed-by: CK Hu <ck.hu@mediatek.com>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_disp_drv.h       |   1 +
>  .../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c   |  43 ++-
>  drivers/gpu/drm/mediatek/mtk_dpi.c            |  21 +-
>  drivers/gpu/drm/mediatek/mtk_drm_drv.c        | 253 +++++++++++++++++-
>  drivers/gpu/drm/mediatek/mtk_drm_drv.h        |   2 +-
>  drivers/gpu/drm/mediatek/mtk_dsi.c            |  14 +-
>  6 files changed, 312 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> index 082ac18fe04a..94843974851f 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> @@ -108,6 +108,7 @@ size_t mtk_ovl_get_num_formats(struct device *dev);
>  
>  void mtk_ovl_adaptor_add_comp(struct device *dev, struct mtk_mutex *mutex);
>  void mtk_ovl_adaptor_remove_comp(struct device *dev, struct mtk_mutex *mutex);
> +bool mtk_ovl_adaptor_is_comp_present(struct device_node *node);
>  void mtk_ovl_adaptor_connect(struct device *dev, struct device *mmsys_dev,
>  			     unsigned int next);
>  void mtk_ovl_adaptor_disconnect(struct device *dev, struct device *mmsys_dev,
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> index c6768210b08b..4e064d3c97cc 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> @@ -490,6 +490,41 @@ static int compare_of(struct device *dev, void *data)
>  	return dev->of_node == data;
>  }
>  
> +static int ovl_adaptor_of_get_ddp_comp_type(struct device_node *node,
> +					    enum mtk_ovl_adaptor_comp_type *ctype)
> +{
> +	const struct of_device_id *of_id = of_match_node(mtk_ovl_adaptor_comp_dt_ids, node);
> +
> +	if (!of_id)
> +		return -EINVAL;
> +
> +	*ctype = (enum mtk_ovl_adaptor_comp_type)((uintptr_t)of_id->data);
> +
> +	return 0;
> +}
> +
> +bool mtk_ovl_adaptor_is_comp_present(struct device_node *node)
> +{
> +	enum mtk_ovl_adaptor_comp_type type;
> +	int ret;
> +
> +	ret = ovl_adaptor_of_get_ddp_comp_type(node, &type);
> +	if (ret)
> +		return false;
> +
> +	if (type >= OVL_ADAPTOR_TYPE_NUM)
> +		return false;
> +
> +	/*
> +	 * In the context of mediatek-drm, ETHDR, MDP_RDMA and Padding are
> +	 * used exclusively by OVL Adaptor: if this component is not one of
> +	 * those, it's likely not an OVL Adaptor path.
> +	 */
> +	return type == OVL_ADAPTOR_TYPE_ETHDR ||
> +	       type == OVL_ADAPTOR_TYPE_MDP_RDMA ||
> +	       type == OVL_ADAPTOR_TYPE_PADDING;
> +}
> +
>  static int ovl_adaptor_comp_init(struct device *dev, struct component_match **match)
>  {
>  	struct mtk_disp_ovl_adaptor *priv = dev_get_drvdata(dev);
> @@ -499,12 +534,11 @@ static int ovl_adaptor_comp_init(struct device *dev, struct component_match **ma
>  	parent = dev->parent->parent->of_node->parent;
>  
>  	for_each_child_of_node_scoped(parent, node) {
> -		const struct of_device_id *of_id;
>  		enum mtk_ovl_adaptor_comp_type type;
> -		int id;
> +		int id, ret;
>  
> -		of_id = of_match_node(mtk_ovl_adaptor_comp_dt_ids, node);
> -		if (!of_id)
> +		ret = ovl_adaptor_of_get_ddp_comp_type(node, &type);
> +		if (ret)
>  			continue;
>  
>  		if (!of_device_is_available(node)) {
> @@ -513,7 +547,6 @@ static int ovl_adaptor_comp_init(struct device *dev, struct component_match **ma
>  			continue;
>  		}
>  
> -		type = (enum mtk_ovl_adaptor_comp_type)(uintptr_t)of_id->data;
>  		id = ovl_adaptor_comp_get_id(dev, node, type);
>  		if (id < 0) {
>  			dev_warn(dev, "Skipping unknown component %pOF\n",
> diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c
> index e4da5cc75e60..ad37b0ecafb9 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dpi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
> @@ -755,6 +755,20 @@ static int mtk_dpi_bridge_attach(struct drm_bridge *bridge,
>  				 enum drm_bridge_attach_flags flags)
>  {
>  	struct mtk_dpi *dpi = bridge_to_dpi(bridge);
> +	int ret;
> +
> +	dpi->next_bridge = devm_drm_of_get_bridge(dpi->dev, dpi->dev->of_node, 1, -1);
> +	if (IS_ERR(dpi->next_bridge)) {
> +		ret = PTR_ERR(dpi->next_bridge);
> +		if (ret == -EPROBE_DEFER)
> +			return ret;
> +
> +		/* Old devicetree has only one endpoint */
> +		dpi->next_bridge = devm_drm_of_get_bridge(dpi->dev, dpi->dev->of_node, 0, 0);
> +		if (IS_ERR(dpi->next_bridge))
> +			return dev_err_probe(dpi->dev, PTR_ERR(dpi->next_bridge),
> +					     "Failed to get bridge\n");
> +	}
>  
>  	return drm_bridge_attach(bridge->encoder, dpi->next_bridge,
>  				 &dpi->bridge, flags);
> @@ -1162,13 +1176,6 @@ static int mtk_dpi_probe(struct platform_device *pdev)
>  	if (dpi->irq < 0)
>  		return dpi->irq;
>  
> -	dpi->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0);
> -	if (IS_ERR(dpi->next_bridge))
> -		return dev_err_probe(dev, PTR_ERR(dpi->next_bridge),
> -				     "Failed to get bridge\n");
> -
> -	dev_info(dev, "Found bridge node: %pOF\n", dpi->next_bridge->of_node);
> -
>  	platform_set_drvdata(pdev, dpi);
>  
>  	dpi->bridge.funcs = &mtk_dpi_bridge_funcs;
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> index a4594f8873d5..85be035a209a 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> @@ -27,6 +27,7 @@
>  
>  #include "mtk_crtc.h"
>  #include "mtk_ddp_comp.h"
> +#include "mtk_disp_drv.h"
>  #include "mtk_drm_drv.h"
>  #include "mtk_gem.h"
>  
> @@ -820,12 +821,235 @@ static const struct of_device_id mtk_ddp_comp_dt_ids[] = {
>  	{ }
>  };
>  
> +static int mtk_drm_of_get_ddp_comp_type(struct device_node *node, enum mtk_ddp_comp_type *ctype)
> +{
> +	const struct of_device_id *of_id = of_match_node(mtk_ddp_comp_dt_ids, node);
> +
> +	if (!of_id)
> +		return -EINVAL;
> +
> +	*ctype = (enum mtk_ddp_comp_type)((uintptr_t)of_id->data);
> +
> +	return 0;
> +}
> +
> +static int mtk_drm_of_get_ddp_ep_cid(struct device_node *node,
> +				     int output_port, enum mtk_crtc_path crtc_path,
> +				     struct device_node **next, unsigned int *cid)
> +{
> +	struct device_node *ep_dev_node, *ep_out;
> +	enum mtk_ddp_comp_type comp_type;
> +	int ret;
> +
> +	ep_out = of_graph_get_endpoint_by_regs(node, output_port, crtc_path);
> +	if (!ep_out)
> +		return -ENOENT;
> +
> +	ep_dev_node = of_graph_get_remote_port_parent(ep_out);
> +	of_node_put(ep_out);
> +	if (!ep_dev_node)
> +		return -EINVAL;
> +
> +	/*
> +	 * Pass the next node pointer regardless of failures in the later code
> +	 * so that if this function is called in a loop it will walk through all
> +	 * of the subsequent endpoints anyway.
> +	 */
> +	*next = ep_dev_node;
> +
> +	if (!of_device_is_available(ep_dev_node))
> +		return -ENODEV;
> +
> +	ret = mtk_drm_of_get_ddp_comp_type(ep_dev_node, &comp_type);
> +	if (ret) {
> +		if (mtk_ovl_adaptor_is_comp_present(ep_dev_node)) {
> +			*cid = (unsigned int)DDP_COMPONENT_DRM_OVL_ADAPTOR;
> +			return 0;
> +		}
> +		return ret;
> +	}
> +
> +	ret = mtk_ddp_comp_get_id(ep_dev_node, comp_type);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* All ok! Pass the Component ID to the caller. */
> +	*cid = (unsigned int)ret;
> +
> +	return 0;
> +}
> +
> +/**
> + * mtk_drm_of_ddp_path_build_one - Build a Display HW Pipeline for a CRTC Path
> + * @dev:          The mediatek-drm device
> + * @cpath:        CRTC Path relative to a VDO or MMSYS
> + * @out_path:     Pointer to an array that will contain the new pipeline
> + * @out_path_len: Number of entries in the pipeline array
> + *
> + * MediaTek SoCs can use different DDP hardware pipelines (or paths) depending
> + * on the board-specific desired display configuration; this function walks
> + * through all of the output endpoints starting from a VDO or MMSYS hardware
> + * instance and builds the right pipeline as specified in device trees.
> + *
> + * Return:
> + * * %0       - Display HW Pipeline successfully built and validated
> + * * %-ENOENT - Display pipeline was not specified in device tree
> + * * %-EINVAL - Display pipeline built but validation failed
> + * * %-ENOMEM - Failure to allocate pipeline array to pass to the caller
> + */
> +static int mtk_drm_of_ddp_path_build_one(struct device *dev, enum mtk_crtc_path cpath,
> +					 const unsigned int **out_path,
> +					 unsigned int *out_path_len)
> +{
> +	struct device_node *next, *prev, *vdo = dev->parent->of_node;
> +	unsigned int temp_path[DDP_COMPONENT_DRM_ID_MAX] = { 0 };
> +	unsigned int *final_ddp_path;
> +	unsigned short int idx = 0;
> +	bool ovl_adaptor_comp_added = false;
> +	int ret;
> +
> +	/* Get the first entry for the temp_path array */
> +	ret = mtk_drm_of_get_ddp_ep_cid(vdo, 0, cpath, &next, &temp_path[idx]);
> +	if (ret) {
> +		if (next && temp_path[idx] == DDP_COMPONENT_DRM_OVL_ADAPTOR) {
> +			dev_dbg(dev, "Adding OVL Adaptor for %pOF\n", next);
> +			ovl_adaptor_comp_added = true;
> +		} else {
> +			if (next)
> +				dev_err(dev, "Invalid component %pOF\n", next);
> +			else
> +				dev_err(dev, "Cannot find first endpoint for path %d\n", cpath);
> +
> +			return ret;
> +		}
> +	}
> +	idx++;
> +
> +	/*
> +	 * Walk through port outputs until we reach the last valid mediatek-drm component.
> +	 * To be valid, this must end with an "invalid" component that is a display node.
> +	 */
> +	do {
> +		prev = next;
> +		ret = mtk_drm_of_get_ddp_ep_cid(next, 1, cpath, &next, &temp_path[idx]);
> +		of_node_put(prev);
> +		if (ret) {
> +			of_node_put(next);
> +			break;
> +		}
> +
> +		/*
> +		 * If this is an OVL adaptor exclusive component and one of those
> +		 * was already added, don't add another instance of the generic
> +		 * DDP_COMPONENT_OVL_ADAPTOR, as this is used only to decide whether
> +		 * to probe that component master driver of which only one instance
> +		 * is needed and possible.
> +		 */
> +		if (temp_path[idx] == DDP_COMPONENT_DRM_OVL_ADAPTOR) {
> +			if (!ovl_adaptor_comp_added)
> +				ovl_adaptor_comp_added = true;
> +			else
> +				idx--;
> +		}
> +	} while (++idx < DDP_COMPONENT_DRM_ID_MAX);
> +
> +	/*
> +	 * The device component might not be enabled: in that case, don't
> +	 * check the last entry and just report that the device is missing.
> +	 */
> +	if (ret == -ENODEV)
> +		return ret;
> +
> +	/* If the last entry is not a final display output, the configuration is wrong */
> +	switch (temp_path[idx - 1]) {
> +	case DDP_COMPONENT_DP_INTF0:
> +	case DDP_COMPONENT_DP_INTF1:
> +	case DDP_COMPONENT_DPI0:
> +	case DDP_COMPONENT_DPI1:
> +	case DDP_COMPONENT_DSI0:
> +	case DDP_COMPONENT_DSI1:
> +	case DDP_COMPONENT_DSI2:
> +	case DDP_COMPONENT_DSI3:
> +		break;
> +	default:
> +		dev_err(dev, "Invalid display hw pipeline. Last component: %d (ret=%d)\n",
> +			temp_path[idx - 1], ret);
> +		return -EINVAL;
> +	}
> +
> +	final_ddp_path = devm_kmemdup(dev, temp_path, idx * sizeof(temp_path[0]), GFP_KERNEL);
> +	if (!final_ddp_path)
> +		return -ENOMEM;
> +
> +	dev_dbg(dev, "Display HW Pipeline built with %d components.\n", idx);
> +
> +	/* Pipeline built! */
> +	*out_path = final_ddp_path;
> +	*out_path_len = idx;
> +
> +	return 0;
> +}
> +
> +static int mtk_drm_of_ddp_path_build(struct device *dev, struct device_node *node,
> +				     struct mtk_mmsys_driver_data *data)
> +{
> +	struct device_node *ep_node;
> +	struct of_endpoint of_ep;
> +	bool output_present[MAX_CRTC] = { false };
> +	int ret;
> +
> +	for_each_endpoint_of_node(node, ep_node) {
> +		ret = of_graph_parse_endpoint(ep_node, &of_ep);
> +		if (ret) {
> +			dev_err_probe(dev, ret, "Cannot parse endpoint\n");
> +			break;
> +		}
> +
> +		if (of_ep.id >= MAX_CRTC) {
> +			ret = dev_err_probe(dev, -EINVAL,
> +					    "Invalid endpoint%u number\n", of_ep.port);
> +			break;
> +		}
> +
> +		output_present[of_ep.id] = true;
> +	}
> +
> +	if (ret) {
> +		of_node_put(ep_node);
> +		return ret;
> +	}
> +
> +	if (output_present[CRTC_MAIN]) {
> +		ret = mtk_drm_of_ddp_path_build_one(dev, CRTC_MAIN,
> +						    &data->main_path, &data->main_len);
> +		if (ret && ret != -ENODEV)
> +			return ret;
> +	}
> +
> +	if (output_present[CRTC_EXT]) {
> +		ret = mtk_drm_of_ddp_path_build_one(dev, CRTC_EXT,
> +						    &data->ext_path, &data->ext_len);
> +		if (ret && ret != -ENODEV)
> +			return ret;
> +	}
> +
> +	if (output_present[CRTC_THIRD]) {
> +		ret = mtk_drm_of_ddp_path_build_one(dev, CRTC_THIRD,
> +						    &data->third_path, &data->third_len);
> +		if (ret && ret != -ENODEV)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int mtk_drm_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct device_node *phandle = dev->parent->of_node;
>  	const struct of_device_id *of_id;
>  	struct mtk_drm_private *private;
> +	struct mtk_mmsys_driver_data *mtk_drm_data;
>  	struct device_node *node;
>  	struct component_match *match = NULL;
>  	struct platform_device *ovl_adaptor;
> @@ -846,7 +1070,27 @@ static int mtk_drm_probe(struct platform_device *pdev)
>  	if (!of_id)
>  		return -ENODEV;
>  
> -	private->data = of_id->data;
> +	mtk_drm_data = (struct mtk_mmsys_driver_data *)of_id->data;
> +	if (!mtk_drm_data)
> +		return -EINVAL;
> +
> +	/* Try to build the display pipeline from devicetree graphs */
> +	if (of_graph_is_present(phandle)) {
> +		dev_dbg(dev, "Building display pipeline for MMSYS %u\n",
> +			mtk_drm_data->mmsys_id);
> +		private->data = devm_kmemdup(dev, mtk_drm_data,
> +					     sizeof(*mtk_drm_data), GFP_KERNEL);
> +		if (!private->data)
> +			return -ENOMEM;
> +
> +		ret = mtk_drm_of_ddp_path_build(dev, phandle, private->data);
> +		if (ret)
> +			return ret;
> +	} else {
> +		/* No devicetree graphs support: go with hardcoded paths if present */
> +		dev_dbg(dev, "Using hardcoded paths for MMSYS %u\n", mtk_drm_data->mmsys_id);
> +		private->data = mtk_drm_data;
> +	};
>  
>  	private->all_drm_private = devm_kmalloc_array(dev, private->data->mmsys_dev_num,
>  						      sizeof(*private->all_drm_private),
> @@ -868,12 +1112,11 @@ static int mtk_drm_probe(struct platform_device *pdev)
>  
>  	/* Iterate over sibling DISP function blocks */
>  	for_each_child_of_node(phandle->parent, node) {
> -		const struct of_device_id *of_id;
>  		enum mtk_ddp_comp_type comp_type;
>  		int comp_id;
>  
> -		of_id = of_match_node(mtk_ddp_comp_dt_ids, node);
> -		if (!of_id)
> +		ret = mtk_drm_of_get_ddp_comp_type(node, &comp_type);
> +		if (ret)
>  			continue;
>  
>  		if (!of_device_is_available(node)) {
> @@ -882,8 +1125,6 @@ static int mtk_drm_probe(struct platform_device *pdev)
>  			continue;
>  		}
>  
> -		comp_type = (enum mtk_ddp_comp_type)(uintptr_t)of_id->data;
> -
>  		if (comp_type == MTK_DISP_MUTEX) {
>  			int id;
>  
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> index ce897984de51..675cdc90a440 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> @@ -63,7 +63,7 @@ struct mtk_drm_private {
>  	struct device *mmsys_dev;
>  	struct device_node *comp_node[DDP_COMPONENT_DRM_ID_MAX];
>  	struct mtk_ddp_comp ddp_comp[DDP_COMPONENT_DRM_ID_MAX];
> -	const struct mtk_mmsys_driver_data *data;
> +	struct mtk_mmsys_driver_data *data;
>  	struct drm_atomic_state *suspend_state;
>  	unsigned int mbox_index;
>  	struct mtk_drm_private **all_drm_private;
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index eeec641cab60..33ceeb8d6925 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -988,9 +988,17 @@ static int mtk_dsi_host_attach(struct mipi_dsi_host *host,
>  	dsi->lanes = device->lanes;
>  	dsi->format = device->format;
>  	dsi->mode_flags = device->mode_flags;
> -	dsi->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0);
> -	if (IS_ERR(dsi->next_bridge))
> -		return PTR_ERR(dsi->next_bridge);
> +	dsi->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
> +	if (IS_ERR(dsi->next_bridge)) {
> +		ret = PTR_ERR(dsi->next_bridge);
> +		if (ret == -EPROBE_DEFER)
> +			return ret;
> +
> +		/* Old devicetree has only one endpoint */
> +		dsi->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0);

Here's the problem with your binding change.

next_bridge was port 0, and now it is port 1. Yes, you have a fallback 
for an old DT, but what happens with a new DT passed to a kernel without 
the change here? 

Plus now we've got to carry this fallback code path when it's completely 
avoidable.

Rob

Re: [PATCH v12 1/3] dt-bindings: display: mediatek: Add OF graph support for board path

[Rob Herring]

On Tue, Oct 15, 2024 at 10:32:22AM +0200, AngeloGioacchino Del Regno wrote:
> Il 14/10/24 19:36, Rob Herring ha scritto:
> > On Mon, Oct 14, 2024 at 3:51 AM AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@collabora.com> wrote:
> > > 
> > > The display IPs in MediaTek SoCs support being interconnected with
> > > different instances of DDP IPs (for example, merge0 or merge1) and/or
> > > with different DDP IPs (for example, rdma can be connected with either
> > > color, dpi, dsi, merge, etc), forming a full Display Data Path that
> > > ends with an actual display.
> > > 
> > > The final display pipeline is effectively board specific, as it does
> > > depend on the display that is attached to it, and eventually on the
> > > sensors supported by the board (for example, Adaptive Ambient Light
> > > would need an Ambient Light Sensor, otherwise it's pointless!), other
> > > than the output type.
> > > 
> > > Add support for OF graphs to most of the MediaTek DDP (display) bindings
> > > to add flexibility to build custom hardware paths, hence enabling board
> > > specific configuration of the display pipeline and allowing to finally
> > > migrate away from using hardcoded paths.
> > > 
> > > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > > Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
> > > Tested-by: Alexandre Mergnat <amergnat@baylibre.com>
> > > Reviewed-by: CK Hu <ck.hu@mediatek.com>
> > > Tested-by: Michael Walle <mwalle@kernel.org> # on kontron-sbc-i1200
> > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > > ---
> > >   .../display/mediatek/mediatek,aal.yaml        | 40 +++++++++++++++++++
> > >   .../display/mediatek/mediatek,ccorr.yaml      | 21 ++++++++++
> > >   .../display/mediatek/mediatek,color.yaml      | 22 ++++++++++
> > >   .../display/mediatek/mediatek,dither.yaml     | 22 ++++++++++
> > >   .../display/mediatek/mediatek,dpi.yaml        | 25 +++++++++++-
> > >   .../display/mediatek/mediatek,dsc.yaml        | 24 +++++++++++
> > >   .../display/mediatek/mediatek,dsi.yaml        | 27 ++++++++++++-
> > >   .../display/mediatek/mediatek,ethdr.yaml      | 22 ++++++++++
> > >   .../display/mediatek/mediatek,gamma.yaml      | 19 +++++++++
> > >   .../display/mediatek/mediatek,merge.yaml      | 23 +++++++++++
> > >   .../display/mediatek/mediatek,od.yaml         | 22 ++++++++++
> > >   .../display/mediatek/mediatek,ovl-2l.yaml     | 22 ++++++++++
> > >   .../display/mediatek/mediatek,ovl.yaml        | 22 ++++++++++
> > >   .../display/mediatek/mediatek,postmask.yaml   | 21 ++++++++++
> > >   .../display/mediatek/mediatek,rdma.yaml       | 22 ++++++++++
> > >   .../display/mediatek/mediatek,ufoe.yaml       | 21 ++++++++++
> > >   16 files changed, 372 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
> > > index cf24434854ff..47ddba5c41af 100644
> > > --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
> > > +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
> > > @@ -62,6 +62,27 @@ properties:
> > >       $ref: /schemas/types.yaml#/definitions/phandle-array
> > >       maxItems: 1
> > > 
> > > +  ports:
> > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > +    description:
> > > +      Input and output ports can have multiple endpoints, each of those
> > > +      connects to either the primary, secondary, etc, display pipeline.
> > > +
> > > +    properties:
> > > +      port@0:
> > > +        $ref: /schemas/graph.yaml#/properties/port
> > > +        description: AAL input port
> > > +
> > > +      port@1:
> > > +        $ref: /schemas/graph.yaml#/properties/port
> > > +        description:
> > > +          AAL output to the next component's input, for example could be one
> > > +          of many gamma, overdrive or other blocks.
> > > +
> > > +    required:
> > > +      - port@0
> > > +      - port@1
> > > +
> > >   required:
> > >     - compatible
> > >     - reg
> > > @@ -89,5 +110,24 @@ examples:
> > >              power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
> > >              clocks = <&mmsys CLK_MM_DISP_AAL>;
> > >              mediatek,gce-client-reg = <&gce SUBSYS_1401XXXX 0x5000 0x1000>;
> > > +
> > > +           ports {
> > > +               #address-cells = <1>;
> > > +               #size-cells = <0>;
> > > +
> > > +               port@0 {
> > > +                   reg = <0>;
> > > +                   aal0_in: endpoint {
> > > +                       remote-endpoint = <&ccorr0_out>;
> > > +                   };
> > > +               };
> > > +
> > > +               port@1 {
> > > +                   reg = <1>;
> > > +                   aal0_out: endpoint {
> > > +                       remote-endpoint = <&gamma0_in>;
> > > +                   };
> > > +               };
> > > +           };
> > >          };
> > >       };
> > > diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
> > > index 9f8366763831..fca8e7bb0cbc 100644
> > > --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
> > > +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
> > > @@ -57,6 +57,27 @@ properties:
> > >       $ref: /schemas/types.yaml#/definitions/phandle-array
> > >       maxItems: 1
> > > 
> > > +  ports:
> > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > +    description:
> > > +      Input and output ports can have multiple endpoints, each of those
> > > +      connects to either the primary, secondary, etc, display pipeline.
> > > +
> > > +    properties:
> > > +      port@0:
> > > +        $ref: /schemas/graph.yaml#/properties/port
> > > +        description: CCORR input port
> > > +
> > > +      port@1:
> > > +        $ref: /schemas/graph.yaml#/properties/port
> > > +        description:
> > > +          CCORR output to the input of the next desired component in the
> > > +          display pipeline, usually only one of the available AAL blocks.
> > > +
> > > +    required:
> > > +      - port@0
> > > +      - port@1
> > > +
> > >   required:
> > >     - compatible
> > >     - reg
> > > diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
> > > index 7df786bbad20..6160439ce4d7 100644
> > > --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
> > > +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
> > > @@ -65,6 +65,28 @@ properties:
> > >       $ref: /schemas/types.yaml#/definitions/phandle-array
> > >       maxItems: 1
> > > 
> > > +  ports:
> > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > +    description:
> > > +      Input and output ports can have multiple endpoints, each of those
> > > +      connects to either the primary, secondary, etc, display pipeline.
> > > +
> > > +    properties:
> > > +      port@0:
> > > +        $ref: /schemas/graph.yaml#/properties/port
> > > +        description: COLOR input port
> > > +
> > > +      port@1:
> > > +        $ref: /schemas/graph.yaml#/properties/port
> > > +        description:
> > > +          COLOR output to the input of the next desired component in the
> > > +          display pipeline, for example one of the available CCORR or AAL
> > > +          blocks.
> > > +
> > > +    required:
> > > +      - port@0
> > > +      - port@1
> > > +
> > >   required:
> > >     - compatible
> > >     - reg
> > > diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
> > > index 6fceb1f95d2a..abaf27916d13 100644
> > > --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
> > > +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
> > > @@ -56,6 +56,28 @@ properties:
> > >       $ref: /schemas/types.yaml#/definitions/phandle-array
> > >       maxItems: 1
> > > 
> > > +  ports:
> > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > +    description:
> > > +      Input and output ports can have multiple endpoints, each of those
> > > +      connects to either the primary, secondary, etc, display pipeline.
> > > +
> > > +    properties:
> > > +      port@0:
> > > +        $ref: /schemas/graph.yaml#/properties/port
> > > +        description: DITHER input, usually from a POSTMASK or GAMMA block.
> > > +
> > > +      port@1:
> > > +        $ref: /schemas/graph.yaml#/properties/port
> > > +        description:
> > > +          DITHER output to the input of the next desired component in the
> > > +          display pipeline, for example one of the available DSC compressors,
> > > +          DP_INTF, DSI, LVDS or others.
> > > +
> > > +    required:
> > > +      - port@0
> > > +      - port@1
> > > +
> > >   required:
> > >     - compatible
> > >     - reg
> > > diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
> > > index 3a82aec9021c..b567e3d58aa1 100644
> > > --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
> > > +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
> > > @@ -71,13 +71,34 @@ properties:
> > >         Output port node. This port should be connected to the input port of an
> > >         attached HDMI, LVDS or DisplayPort encoder chip.
> > > 
> > > +  ports:
> > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > +
> > > +    properties:
> > > +      port@0:
> > > +        $ref: /schemas/graph.yaml#/properties/port
> > > +        description: DPI input port
> > > +
> > > +      port@1:
> > > +        $ref: /schemas/graph.yaml#/properties/port
> > > +        description: DPI output to an HDMI, LVDS or DisplayPort encoder input
> > 
> > This is wrong. The existing 'port' is the output. 'port' and 'port@0'
> > are treated as the same thing. Since you are adding an input port, the
> > new port has to be 'port@1' (or any number but 0).
> > 
> > I haven't looked at the driver code, but it should request port 0 and
> > always get the output port. And requesting port 1 will return an error
> > or the input port.
> 
> Hello Rob,
> 
> I want to remind you that in v2 of this series you said that it'd be wrong for
> port@0 to be an output, I replied that you misread that as I had modeled it indeed
> as an input, and then you gave me your Reviewed-by tag.

I have not misread it. Then I guess I forgot about it and missed it the 
next time on v3.

> Anyway - I get your concern about the previous behavior of `port`, but I chose to
> model this that way purely for consistency.
> 
> First of all - the driver(s) will check if we're feeding a full graph, as it will
> indeed first check if port@1 is present: if it is, then it follows this scheme with
> port@0 as INPUT and port@1 as OUTPUT.
> If the component in port@0 is an OUTPUT, the bridge attach will fail.
> 
> Getting to bindings themselves, then... it would be a mistake to model port@0 as an
> output and port@1 as an input, because that would be not only inconsistent with the
> DRM Bridge bindings, but would be highly confusing when reading the devicetree.

Somewhat confusing, yes. Highly, no. Put a comment in the DT.

> Please note that the bridge bindings are always declaring port@0 as an INPUT and
> other ports as OUTPUT(s).

There is no guarantee on that. Port numbering is local to the bridge and 
opaque to anything outside that bridge. That is why you have to document 
what each port represents.

Would we have followed that convention if all the ports were defined 
from the start? Certainly. But that didn't happen and you are stuck with 
the existing binding and ABI.

> As an example, you can check display/bridge/analogix,anx7625.yaml or
> display/bridge/samsung,mipi-dsim.yaml (and others) for bridges, otherwise
> display/st,stm32mp25-lvds.yaml or display/allwinner,sun4i-a10-display-frontend.yaml
> (and others) for display controllers, which do all conform to this logic, where
> the input is always @0, and the output is @1.
> 
> Of course, doing this required me to do extra changes to the MTK DRM drivers to
> actually be retro-compatible with the old devicetrees as I explained before.

You can't fix existing software...

If you want to break the ABI, then that's ultimately up to you and 
Mediatek maintainers to decide0. This case is easy to avoid, why would 
you knowingly break the ABI here. OTOH, this seems like a big enough 
change I would imagine it is a matter of time before supporting a 
missing OF graph for the components will be a problem.

Rob

Re: [PATCH v12 3/3] drm/mediatek: Implement OF graphs support for display paths

[AngeloGioacchino Del Regno]

Il 15/10/24 15:28, Rob Herring ha scritto:
> On Mon, Oct 14, 2024 at 10:51:48AM +0200, AngeloGioacchino Del Regno wrote:
>> It is impossible to add each and every possible DDP path combination
>> for each and every possible combination of SoC and board: right now,
>> this driver hardcodes configuration for 10 SoCs and this is going to
>> grow larger and larger, and with new hacks like the introduction of
>> mtk_drm_route which is anyway not enough for all final routes as the
>> DSI cannot be connected to MERGE if it's not a dual-DSI, or enabling
>> DSC preventively doesn't work if the display doesn't support it, or
>> others.
>>
>> Since practically all display IPs in MediaTek SoCs support being
>> interconnected with different instances of other, or the same, IPs
>> or with different IPs and in different combinations, the final DDP
>> pipeline is effectively a board specific configuration.
>>
>> Implement OF graphs support to the mediatek-drm drivers, allowing to
>> stop hardcoding the paths, and preventing this driver to get a huge
>> amount of arrays for each board and SoC combination, also paving the
>> way to share the same mtk_mmsys_driver_data between multiple SoCs,
>> making it more straightforward to add support for new chips.
>>
>> Note that the OVL_ADAPTOR software component driver needs relatively
>> big changes in order to fully support OF Graphs (and more SoCs anyway)
>> and such changes will come at a later time.
>> As of now, the mtk_disp_ovl_adaptor driver takes the MERGE components
>> (for example, on mt8195, merge 1 to 4) dynamically so, even though
>> later updates to the ovl-adaptor driver will *not* require bindings
>> changes, the merge1-4 will be temporarily omitted in the graph for the
>> MT8195 SoC.
>>
>> This means that an example graph for this SoC looks like:
>>
>> mdp_rdma (0 ~ 7) -> padding (0 ~ 7) -> ethdr -> merge5
>>
>> and the resulting path in this driver will be `ovl_adaptor -> merge5`
>>
>> Later updates to the ovl adaptor will expand it to support more SoCs
>> and, in turn, to also fully support graphs.
>>
>> Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
>> Tested-by: Alexandre Mergnat <amergnat@baylibre.com>
>> Acked-by: Sui Jingfeng <sui.jingfeng@linux.dev>
>> Tested-by: Michael Walle <mwalle@kernel.org> # on kontron-sbc-i1200
>> Reviewed-by: CK Hu <ck.hu@mediatek.com>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/gpu/drm/mediatek/mtk_disp_drv.h       |   1 +
>>   .../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c   |  43 ++-
>>   drivers/gpu/drm/mediatek/mtk_dpi.c            |  21 +-
>>   drivers/gpu/drm/mediatek/mtk_drm_drv.c        | 253 +++++++++++++++++-
>>   drivers/gpu/drm/mediatek/mtk_drm_drv.h        |   2 +-
>>   drivers/gpu/drm/mediatek/mtk_dsi.c            |  14 +-
>>   6 files changed, 312 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
>> index 082ac18fe04a..94843974851f 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
>> +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
>> @@ -108,6 +108,7 @@ size_t mtk_ovl_get_num_formats(struct device *dev);
>>   
>>   void mtk_ovl_adaptor_add_comp(struct device *dev, struct mtk_mutex *mutex);
>>   void mtk_ovl_adaptor_remove_comp(struct device *dev, struct mtk_mutex *mutex);
>> +bool mtk_ovl_adaptor_is_comp_present(struct device_node *node);
>>   void mtk_ovl_adaptor_connect(struct device *dev, struct device *mmsys_dev,
>>   			     unsigned int next);
>>   void mtk_ovl_adaptor_disconnect(struct device *dev, struct device *mmsys_dev,
>> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
>> index c6768210b08b..4e064d3c97cc 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
>> @@ -490,6 +490,41 @@ static int compare_of(struct device *dev, void *data)
>>   	return dev->of_node == data;
>>   }
>>   
>> +static int ovl_adaptor_of_get_ddp_comp_type(struct device_node *node,
>> +					    enum mtk_ovl_adaptor_comp_type *ctype)
>> +{
>> +	const struct of_device_id *of_id = of_match_node(mtk_ovl_adaptor_comp_dt_ids, node);
>> +
>> +	if (!of_id)
>> +		return -EINVAL;
>> +
>> +	*ctype = (enum mtk_ovl_adaptor_comp_type)((uintptr_t)of_id->data);
>> +
>> +	return 0;
>> +}
>> +
>> +bool mtk_ovl_adaptor_is_comp_present(struct device_node *node)
>> +{
>> +	enum mtk_ovl_adaptor_comp_type type;
>> +	int ret;
>> +
>> +	ret = ovl_adaptor_of_get_ddp_comp_type(node, &type);
>> +	if (ret)
>> +		return false;
>> +
>> +	if (type >= OVL_ADAPTOR_TYPE_NUM)
>> +		return false;
>> +
>> +	/*
>> +	 * In the context of mediatek-drm, ETHDR, MDP_RDMA and Padding are
>> +	 * used exclusively by OVL Adaptor: if this component is not one of
>> +	 * those, it's likely not an OVL Adaptor path.
>> +	 */
>> +	return type == OVL_ADAPTOR_TYPE_ETHDR ||
>> +	       type == OVL_ADAPTOR_TYPE_MDP_RDMA ||
>> +	       type == OVL_ADAPTOR_TYPE_PADDING;
>> +}
>> +
>>   static int ovl_adaptor_comp_init(struct device *dev, struct component_match **match)
>>   {
>>   	struct mtk_disp_ovl_adaptor *priv = dev_get_drvdata(dev);
>> @@ -499,12 +534,11 @@ static int ovl_adaptor_comp_init(struct device *dev, struct component_match **ma
>>   	parent = dev->parent->parent->of_node->parent;
>>   
>>   	for_each_child_of_node_scoped(parent, node) {
>> -		const struct of_device_id *of_id;
>>   		enum mtk_ovl_adaptor_comp_type type;
>> -		int id;
>> +		int id, ret;
>>   
>> -		of_id = of_match_node(mtk_ovl_adaptor_comp_dt_ids, node);
>> -		if (!of_id)
>> +		ret = ovl_adaptor_of_get_ddp_comp_type(node, &type);
>> +		if (ret)
>>   			continue;
>>   
>>   		if (!of_device_is_available(node)) {
>> @@ -513,7 +547,6 @@ static int ovl_adaptor_comp_init(struct device *dev, struct component_match **ma
>>   			continue;
>>   		}
>>   
>> -		type = (enum mtk_ovl_adaptor_comp_type)(uintptr_t)of_id->data;
>>   		id = ovl_adaptor_comp_get_id(dev, node, type);
>>   		if (id < 0) {
>>   			dev_warn(dev, "Skipping unknown component %pOF\n",
>> diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c
>> index e4da5cc75e60..ad37b0ecafb9 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_dpi.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
>> @@ -755,6 +755,20 @@ static int mtk_dpi_bridge_attach(struct drm_bridge *bridge,
>>   				 enum drm_bridge_attach_flags flags)
>>   {
>>   	struct mtk_dpi *dpi = bridge_to_dpi(bridge);
>> +	int ret;
>> +
>> +	dpi->next_bridge = devm_drm_of_get_bridge(dpi->dev, dpi->dev->of_node, 1, -1);
>> +	if (IS_ERR(dpi->next_bridge)) {
>> +		ret = PTR_ERR(dpi->next_bridge);
>> +		if (ret == -EPROBE_DEFER)
>> +			return ret;
>> +
>> +		/* Old devicetree has only one endpoint */
>> +		dpi->next_bridge = devm_drm_of_get_bridge(dpi->dev, dpi->dev->of_node, 0, 0);
>> +		if (IS_ERR(dpi->next_bridge))
>> +			return dev_err_probe(dpi->dev, PTR_ERR(dpi->next_bridge),
>> +					     "Failed to get bridge\n");
>> +	}
>>   
>>   	return drm_bridge_attach(bridge->encoder, dpi->next_bridge,
>>   				 &dpi->bridge, flags);
>> @@ -1162,13 +1176,6 @@ static int mtk_dpi_probe(struct platform_device *pdev)
>>   	if (dpi->irq < 0)
>>   		return dpi->irq;
>>   
>> -	dpi->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0);
>> -	if (IS_ERR(dpi->next_bridge))
>> -		return dev_err_probe(dev, PTR_ERR(dpi->next_bridge),
>> -				     "Failed to get bridge\n");
>> -
>> -	dev_info(dev, "Found bridge node: %pOF\n", dpi->next_bridge->of_node);
>> -
>>   	platform_set_drvdata(pdev, dpi);
>>   
>>   	dpi->bridge.funcs = &mtk_dpi_bridge_funcs;
>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
>> index a4594f8873d5..85be035a209a 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
>> @@ -27,6 +27,7 @@
>>   
>>   #include "mtk_crtc.h"
>>   #include "mtk_ddp_comp.h"
>> +#include "mtk_disp_drv.h"
>>   #include "mtk_drm_drv.h"
>>   #include "mtk_gem.h"
>>   
>> @@ -820,12 +821,235 @@ static const struct of_device_id mtk_ddp_comp_dt_ids[] = {
>>   	{ }
>>   };
>>   
>> +static int mtk_drm_of_get_ddp_comp_type(struct device_node *node, enum mtk_ddp_comp_type *ctype)
>> +{
>> +	const struct of_device_id *of_id = of_match_node(mtk_ddp_comp_dt_ids, node);
>> +
>> +	if (!of_id)
>> +		return -EINVAL;
>> +
>> +	*ctype = (enum mtk_ddp_comp_type)((uintptr_t)of_id->data);
>> +
>> +	return 0;
>> +}
>> +
>> +static int mtk_drm_of_get_ddp_ep_cid(struct device_node *node,
>> +				     int output_port, enum mtk_crtc_path crtc_path,
>> +				     struct device_node **next, unsigned int *cid)
>> +{
>> +	struct device_node *ep_dev_node, *ep_out;
>> +	enum mtk_ddp_comp_type comp_type;
>> +	int ret;
>> +
>> +	ep_out = of_graph_get_endpoint_by_regs(node, output_port, crtc_path);
>> +	if (!ep_out)
>> +		return -ENOENT;
>> +
>> +	ep_dev_node = of_graph_get_remote_port_parent(ep_out);
>> +	of_node_put(ep_out);
>> +	if (!ep_dev_node)
>> +		return -EINVAL;
>> +
>> +	/*
>> +	 * Pass the next node pointer regardless of failures in the later code
>> +	 * so that if this function is called in a loop it will walk through all
>> +	 * of the subsequent endpoints anyway.
>> +	 */
>> +	*next = ep_dev_node;
>> +
>> +	if (!of_device_is_available(ep_dev_node))
>> +		return -ENODEV;
>> +
>> +	ret = mtk_drm_of_get_ddp_comp_type(ep_dev_node, &comp_type);
>> +	if (ret) {
>> +		if (mtk_ovl_adaptor_is_comp_present(ep_dev_node)) {
>> +			*cid = (unsigned int)DDP_COMPONENT_DRM_OVL_ADAPTOR;
>> +			return 0;
>> +		}
>> +		return ret;
>> +	}
>> +
>> +	ret = mtk_ddp_comp_get_id(ep_dev_node, comp_type);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* All ok! Pass the Component ID to the caller. */
>> +	*cid = (unsigned int)ret;
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * mtk_drm_of_ddp_path_build_one - Build a Display HW Pipeline for a CRTC Path
>> + * @dev:          The mediatek-drm device
>> + * @cpath:        CRTC Path relative to a VDO or MMSYS
>> + * @out_path:     Pointer to an array that will contain the new pipeline
>> + * @out_path_len: Number of entries in the pipeline array
>> + *
>> + * MediaTek SoCs can use different DDP hardware pipelines (or paths) depending
>> + * on the board-specific desired display configuration; this function walks
>> + * through all of the output endpoints starting from a VDO or MMSYS hardware
>> + * instance and builds the right pipeline as specified in device trees.
>> + *
>> + * Return:
>> + * * %0       - Display HW Pipeline successfully built and validated
>> + * * %-ENOENT - Display pipeline was not specified in device tree
>> + * * %-EINVAL - Display pipeline built but validation failed
>> + * * %-ENOMEM - Failure to allocate pipeline array to pass to the caller
>> + */
>> +static int mtk_drm_of_ddp_path_build_one(struct device *dev, enum mtk_crtc_path cpath,
>> +					 const unsigned int **out_path,
>> +					 unsigned int *out_path_len)
>> +{
>> +	struct device_node *next, *prev, *vdo = dev->parent->of_node;
>> +	unsigned int temp_path[DDP_COMPONENT_DRM_ID_MAX] = { 0 };
>> +	unsigned int *final_ddp_path;
>> +	unsigned short int idx = 0;
>> +	bool ovl_adaptor_comp_added = false;
>> +	int ret;
>> +
>> +	/* Get the first entry for the temp_path array */
>> +	ret = mtk_drm_of_get_ddp_ep_cid(vdo, 0, cpath, &next, &temp_path[idx]);
>> +	if (ret) {
>> +		if (next && temp_path[idx] == DDP_COMPONENT_DRM_OVL_ADAPTOR) {
>> +			dev_dbg(dev, "Adding OVL Adaptor for %pOF\n", next);
>> +			ovl_adaptor_comp_added = true;
>> +		} else {
>> +			if (next)
>> +				dev_err(dev, "Invalid component %pOF\n", next);
>> +			else
>> +				dev_err(dev, "Cannot find first endpoint for path %d\n", cpath);
>> +
>> +			return ret;
>> +		}
>> +	}
>> +	idx++;
>> +
>> +	/*
>> +	 * Walk through port outputs until we reach the last valid mediatek-drm component.
>> +	 * To be valid, this must end with an "invalid" component that is a display node.
>> +	 */
>> +	do {
>> +		prev = next;
>> +		ret = mtk_drm_of_get_ddp_ep_cid(next, 1, cpath, &next, &temp_path[idx]);
>> +		of_node_put(prev);
>> +		if (ret) {
>> +			of_node_put(next);
>> +			break;
>> +		}
>> +
>> +		/*
>> +		 * If this is an OVL adaptor exclusive component and one of those
>> +		 * was already added, don't add another instance of the generic
>> +		 * DDP_COMPONENT_OVL_ADAPTOR, as this is used only to decide whether
>> +		 * to probe that component master driver of which only one instance
>> +		 * is needed and possible.
>> +		 */
>> +		if (temp_path[idx] == DDP_COMPONENT_DRM_OVL_ADAPTOR) {
>> +			if (!ovl_adaptor_comp_added)
>> +				ovl_adaptor_comp_added = true;
>> +			else
>> +				idx--;
>> +		}
>> +	} while (++idx < DDP_COMPONENT_DRM_ID_MAX);
>> +
>> +	/*
>> +	 * The device component might not be enabled: in that case, don't
>> +	 * check the last entry and just report that the device is missing.
>> +	 */
>> +	if (ret == -ENODEV)
>> +		return ret;
>> +
>> +	/* If the last entry is not a final display output, the configuration is wrong */
>> +	switch (temp_path[idx - 1]) {
>> +	case DDP_COMPONENT_DP_INTF0:
>> +	case DDP_COMPONENT_DP_INTF1:
>> +	case DDP_COMPONENT_DPI0:
>> +	case DDP_COMPONENT_DPI1:
>> +	case DDP_COMPONENT_DSI0:
>> +	case DDP_COMPONENT_DSI1:
>> +	case DDP_COMPONENT_DSI2:
>> +	case DDP_COMPONENT_DSI3:
>> +		break;
>> +	default:
>> +		dev_err(dev, "Invalid display hw pipeline. Last component: %d (ret=%d)\n",
>> +			temp_path[idx - 1], ret);
>> +		return -EINVAL;
>> +	}
>> +
>> +	final_ddp_path = devm_kmemdup(dev, temp_path, idx * sizeof(temp_path[0]), GFP_KERNEL);
>> +	if (!final_ddp_path)
>> +		return -ENOMEM;
>> +
>> +	dev_dbg(dev, "Display HW Pipeline built with %d components.\n", idx);
>> +
>> +	/* Pipeline built! */
>> +	*out_path = final_ddp_path;
>> +	*out_path_len = idx;
>> +
>> +	return 0;
>> +}
>> +
>> +static int mtk_drm_of_ddp_path_build(struct device *dev, struct device_node *node,
>> +				     struct mtk_mmsys_driver_data *data)
>> +{
>> +	struct device_node *ep_node;
>> +	struct of_endpoint of_ep;
>> +	bool output_present[MAX_CRTC] = { false };
>> +	int ret;
>> +
>> +	for_each_endpoint_of_node(node, ep_node) {
>> +		ret = of_graph_parse_endpoint(ep_node, &of_ep);
>> +		if (ret) {
>> +			dev_err_probe(dev, ret, "Cannot parse endpoint\n");
>> +			break;
>> +		}
>> +
>> +		if (of_ep.id >= MAX_CRTC) {
>> +			ret = dev_err_probe(dev, -EINVAL,
>> +					    "Invalid endpoint%u number\n", of_ep.port);
>> +			break;
>> +		}
>> +
>> +		output_present[of_ep.id] = true;
>> +	}
>> +
>> +	if (ret) {
>> +		of_node_put(ep_node);
>> +		return ret;
>> +	}
>> +
>> +	if (output_present[CRTC_MAIN]) {
>> +		ret = mtk_drm_of_ddp_path_build_one(dev, CRTC_MAIN,
>> +						    &data->main_path, &data->main_len);
>> +		if (ret && ret != -ENODEV)
>> +			return ret;
>> +	}
>> +
>> +	if (output_present[CRTC_EXT]) {
>> +		ret = mtk_drm_of_ddp_path_build_one(dev, CRTC_EXT,
>> +						    &data->ext_path, &data->ext_len);
>> +		if (ret && ret != -ENODEV)
>> +			return ret;
>> +	}
>> +
>> +	if (output_present[CRTC_THIRD]) {
>> +		ret = mtk_drm_of_ddp_path_build_one(dev, CRTC_THIRD,
>> +						    &data->third_path, &data->third_len);
>> +		if (ret && ret != -ENODEV)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static int mtk_drm_probe(struct platform_device *pdev)
>>   {
>>   	struct device *dev = &pdev->dev;
>>   	struct device_node *phandle = dev->parent->of_node;
>>   	const struct of_device_id *of_id;
>>   	struct mtk_drm_private *private;
>> +	struct mtk_mmsys_driver_data *mtk_drm_data;
>>   	struct device_node *node;
>>   	struct component_match *match = NULL;
>>   	struct platform_device *ovl_adaptor;
>> @@ -846,7 +1070,27 @@ static int mtk_drm_probe(struct platform_device *pdev)
>>   	if (!of_id)
>>   		return -ENODEV;
>>   
>> -	private->data = of_id->data;
>> +	mtk_drm_data = (struct mtk_mmsys_driver_data *)of_id->data;
>> +	if (!mtk_drm_data)
>> +		return -EINVAL;
>> +
>> +	/* Try to build the display pipeline from devicetree graphs */
>> +	if (of_graph_is_present(phandle)) {
>> +		dev_dbg(dev, "Building display pipeline for MMSYS %u\n",
>> +			mtk_drm_data->mmsys_id);
>> +		private->data = devm_kmemdup(dev, mtk_drm_data,
>> +					     sizeof(*mtk_drm_data), GFP_KERNEL);
>> +		if (!private->data)
>> +			return -ENOMEM;
>> +
>> +		ret = mtk_drm_of_ddp_path_build(dev, phandle, private->data);
>> +		if (ret)
>> +			return ret;
>> +	} else {
>> +		/* No devicetree graphs support: go with hardcoded paths if present */
>> +		dev_dbg(dev, "Using hardcoded paths for MMSYS %u\n", mtk_drm_data->mmsys_id);
>> +		private->data = mtk_drm_data;
>> +	};
>>   
>>   	private->all_drm_private = devm_kmalloc_array(dev, private->data->mmsys_dev_num,
>>   						      sizeof(*private->all_drm_private),
>> @@ -868,12 +1112,11 @@ static int mtk_drm_probe(struct platform_device *pdev)
>>   
>>   	/* Iterate over sibling DISP function blocks */
>>   	for_each_child_of_node(phandle->parent, node) {
>> -		const struct of_device_id *of_id;
>>   		enum mtk_ddp_comp_type comp_type;
>>   		int comp_id;
>>   
>> -		of_id = of_match_node(mtk_ddp_comp_dt_ids, node);
>> -		if (!of_id)
>> +		ret = mtk_drm_of_get_ddp_comp_type(node, &comp_type);
>> +		if (ret)
>>   			continue;
>>   
>>   		if (!of_device_is_available(node)) {
>> @@ -882,8 +1125,6 @@ static int mtk_drm_probe(struct platform_device *pdev)
>>   			continue;
>>   		}
>>   
>> -		comp_type = (enum mtk_ddp_comp_type)(uintptr_t)of_id->data;
>> -
>>   		if (comp_type == MTK_DISP_MUTEX) {
>>   			int id;
>>   
>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
>> index ce897984de51..675cdc90a440 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h
>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
>> @@ -63,7 +63,7 @@ struct mtk_drm_private {
>>   	struct device *mmsys_dev;
>>   	struct device_node *comp_node[DDP_COMPONENT_DRM_ID_MAX];
>>   	struct mtk_ddp_comp ddp_comp[DDP_COMPONENT_DRM_ID_MAX];
>> -	const struct mtk_mmsys_driver_data *data;
>> +	struct mtk_mmsys_driver_data *data;
>>   	struct drm_atomic_state *suspend_state;
>>   	unsigned int mbox_index;
>>   	struct mtk_drm_private **all_drm_private;
>> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
>> index eeec641cab60..33ceeb8d6925 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
>> @@ -988,9 +988,17 @@ static int mtk_dsi_host_attach(struct mipi_dsi_host *host,
>>   	dsi->lanes = device->lanes;
>>   	dsi->format = device->format;
>>   	dsi->mode_flags = device->mode_flags;
>> -	dsi->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0);
>> -	if (IS_ERR(dsi->next_bridge))
>> -		return PTR_ERR(dsi->next_bridge);
>> +	dsi->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
>> +	if (IS_ERR(dsi->next_bridge)) {
>> +		ret = PTR_ERR(dsi->next_bridge);
>> +		if (ret == -EPROBE_DEFER)
>> +			return ret;
>> +
>> +		/* Old devicetree has only one endpoint */
>> +		dsi->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0);
> 
> Here's the problem with your binding change.
> 
> next_bridge was port 0, and now it is port 1. Yes, you have a fallback
> for an old DT, but what happens with a new DT passed to a kernel without
> the change here?
> 

Rob, sorry but, didn't we have to ensure retro-compatibility and not really
forward-compatibility?
That's what I'm doing here.

Besides, it's not granted, yes, but it's a somewhat estabilished practice for
people to either upgrade both kernel and devicetree or just the kernel without
the devicetree.
Using the devicetree from a newer kernel version that is not a linux-stable version
is something that I've practically never seen (it's just me, and I'm not saying
that this is something that *never* happens at all), so I am guessing that this is
"more unique than rare", hence something that we shouldn't really worry about.

> Plus now we've got to carry this fallback code path when it's completely
> avoidable.

As I said in the reply of [1/3], the problem here is about consistency of the input
and output port number(s), and that's why the fallback is here.

Is it avoidable? Sure it is, but we lose that consistency.

My opinion is that this is not really avoidable if we want consistent devicetrees
on all MediaTek SoCs (counting arm64 only, more than 10 and growing, adding up the
arm ones they're many more).


Cheers,
Angelo

Re: [PATCH v12 1/3] dt-bindings: display: mediatek: Add OF graph support for board path

[AngeloGioacchino Del Regno]

Il 15/10/24 15:48, Rob Herring ha scritto:
> On Tue, Oct 15, 2024 at 10:32:22AM +0200, AngeloGioacchino Del Regno wrote:
>> Il 14/10/24 19:36, Rob Herring ha scritto:
>>> On Mon, Oct 14, 2024 at 3:51 AM AngeloGioacchino Del Regno
>>> <angelogioacchino.delregno@collabora.com> wrote:
>>>>
>>>> The display IPs in MediaTek SoCs support being interconnected with
>>>> different instances of DDP IPs (for example, merge0 or merge1) and/or
>>>> with different DDP IPs (for example, rdma can be connected with either
>>>> color, dpi, dsi, merge, etc), forming a full Display Data Path that
>>>> ends with an actual display.
>>>>
>>>> The final display pipeline is effectively board specific, as it does
>>>> depend on the display that is attached to it, and eventually on the
>>>> sensors supported by the board (for example, Adaptive Ambient Light
>>>> would need an Ambient Light Sensor, otherwise it's pointless!), other
>>>> than the output type.
>>>>
>>>> Add support for OF graphs to most of the MediaTek DDP (display) bindings
>>>> to add flexibility to build custom hardware paths, hence enabling board
>>>> specific configuration of the display pipeline and allowing to finally
>>>> migrate away from using hardcoded paths.
>>>>
>>>> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
>>>> Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
>>>> Tested-by: Alexandre Mergnat <amergnat@baylibre.com>
>>>> Reviewed-by: CK Hu <ck.hu@mediatek.com>
>>>> Tested-by: Michael Walle <mwalle@kernel.org> # on kontron-sbc-i1200
>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>>> ---
>>>>    .../display/mediatek/mediatek,aal.yaml        | 40 +++++++++++++++++++
>>>>    .../display/mediatek/mediatek,ccorr.yaml      | 21 ++++++++++
>>>>    .../display/mediatek/mediatek,color.yaml      | 22 ++++++++++
>>>>    .../display/mediatek/mediatek,dither.yaml     | 22 ++++++++++
>>>>    .../display/mediatek/mediatek,dpi.yaml        | 25 +++++++++++-
>>>>    .../display/mediatek/mediatek,dsc.yaml        | 24 +++++++++++
>>>>    .../display/mediatek/mediatek,dsi.yaml        | 27 ++++++++++++-
>>>>    .../display/mediatek/mediatek,ethdr.yaml      | 22 ++++++++++
>>>>    .../display/mediatek/mediatek,gamma.yaml      | 19 +++++++++
>>>>    .../display/mediatek/mediatek,merge.yaml      | 23 +++++++++++
>>>>    .../display/mediatek/mediatek,od.yaml         | 22 ++++++++++
>>>>    .../display/mediatek/mediatek,ovl-2l.yaml     | 22 ++++++++++
>>>>    .../display/mediatek/mediatek,ovl.yaml        | 22 ++++++++++
>>>>    .../display/mediatek/mediatek,postmask.yaml   | 21 ++++++++++
>>>>    .../display/mediatek/mediatek,rdma.yaml       | 22 ++++++++++
>>>>    .../display/mediatek/mediatek,ufoe.yaml       | 21 ++++++++++
>>>>    16 files changed, 372 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
>>>> index cf24434854ff..47ddba5c41af 100644
>>>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
>>>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
>>>> @@ -62,6 +62,27 @@ properties:
>>>>        $ref: /schemas/types.yaml#/definitions/phandle-array
>>>>        maxItems: 1
>>>>
>>>> +  ports:
>>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>>> +    description:
>>>> +      Input and output ports can have multiple endpoints, each of those
>>>> +      connects to either the primary, secondary, etc, display pipeline.
>>>> +
>>>> +    properties:
>>>> +      port@0:
>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>> +        description: AAL input port
>>>> +
>>>> +      port@1:
>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>> +        description:
>>>> +          AAL output to the next component's input, for example could be one
>>>> +          of many gamma, overdrive or other blocks.
>>>> +
>>>> +    required:
>>>> +      - port@0
>>>> +      - port@1
>>>> +
>>>>    required:
>>>>      - compatible
>>>>      - reg
>>>> @@ -89,5 +110,24 @@ examples:
>>>>               power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
>>>>               clocks = <&mmsys CLK_MM_DISP_AAL>;
>>>>               mediatek,gce-client-reg = <&gce SUBSYS_1401XXXX 0x5000 0x1000>;
>>>> +
>>>> +           ports {
>>>> +               #address-cells = <1>;
>>>> +               #size-cells = <0>;
>>>> +
>>>> +               port@0 {
>>>> +                   reg = <0>;
>>>> +                   aal0_in: endpoint {
>>>> +                       remote-endpoint = <&ccorr0_out>;
>>>> +                   };
>>>> +               };
>>>> +
>>>> +               port@1 {
>>>> +                   reg = <1>;
>>>> +                   aal0_out: endpoint {
>>>> +                       remote-endpoint = <&gamma0_in>;
>>>> +                   };
>>>> +               };
>>>> +           };
>>>>           };
>>>>        };
>>>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
>>>> index 9f8366763831..fca8e7bb0cbc 100644
>>>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
>>>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
>>>> @@ -57,6 +57,27 @@ properties:
>>>>        $ref: /schemas/types.yaml#/definitions/phandle-array
>>>>        maxItems: 1
>>>>
>>>> +  ports:
>>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>>> +    description:
>>>> +      Input and output ports can have multiple endpoints, each of those
>>>> +      connects to either the primary, secondary, etc, display pipeline.
>>>> +
>>>> +    properties:
>>>> +      port@0:
>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>> +        description: CCORR input port
>>>> +
>>>> +      port@1:
>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>> +        description:
>>>> +          CCORR output to the input of the next desired component in the
>>>> +          display pipeline, usually only one of the available AAL blocks.
>>>> +
>>>> +    required:
>>>> +      - port@0
>>>> +      - port@1
>>>> +
>>>>    required:
>>>>      - compatible
>>>>      - reg
>>>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
>>>> index 7df786bbad20..6160439ce4d7 100644
>>>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
>>>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
>>>> @@ -65,6 +65,28 @@ properties:
>>>>        $ref: /schemas/types.yaml#/definitions/phandle-array
>>>>        maxItems: 1
>>>>
>>>> +  ports:
>>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>>> +    description:
>>>> +      Input and output ports can have multiple endpoints, each of those
>>>> +      connects to either the primary, secondary, etc, display pipeline.
>>>> +
>>>> +    properties:
>>>> +      port@0:
>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>> +        description: COLOR input port
>>>> +
>>>> +      port@1:
>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>> +        description:
>>>> +          COLOR output to the input of the next desired component in the
>>>> +          display pipeline, for example one of the available CCORR or AAL
>>>> +          blocks.
>>>> +
>>>> +    required:
>>>> +      - port@0
>>>> +      - port@1
>>>> +
>>>>    required:
>>>>      - compatible
>>>>      - reg
>>>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
>>>> index 6fceb1f95d2a..abaf27916d13 100644
>>>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
>>>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
>>>> @@ -56,6 +56,28 @@ properties:
>>>>        $ref: /schemas/types.yaml#/definitions/phandle-array
>>>>        maxItems: 1
>>>>
>>>> +  ports:
>>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>>> +    description:
>>>> +      Input and output ports can have multiple endpoints, each of those
>>>> +      connects to either the primary, secondary, etc, display pipeline.
>>>> +
>>>> +    properties:
>>>> +      port@0:
>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>> +        description: DITHER input, usually from a POSTMASK or GAMMA block.
>>>> +
>>>> +      port@1:
>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>> +        description:
>>>> +          DITHER output to the input of the next desired component in the
>>>> +          display pipeline, for example one of the available DSC compressors,
>>>> +          DP_INTF, DSI, LVDS or others.
>>>> +
>>>> +    required:
>>>> +      - port@0
>>>> +      - port@1
>>>> +
>>>>    required:
>>>>      - compatible
>>>>      - reg
>>>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
>>>> index 3a82aec9021c..b567e3d58aa1 100644
>>>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
>>>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
>>>> @@ -71,13 +71,34 @@ properties:
>>>>          Output port node. This port should be connected to the input port of an
>>>>          attached HDMI, LVDS or DisplayPort encoder chip.
>>>>
>>>> +  ports:
>>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>>> +
>>>> +    properties:
>>>> +      port@0:
>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>> +        description: DPI input port
>>>> +
>>>> +      port@1:
>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>> +        description: DPI output to an HDMI, LVDS or DisplayPort encoder input
>>>
>>> This is wrong. The existing 'port' is the output. 'port' and 'port@0'
>>> are treated as the same thing. Since you are adding an input port, the
>>> new port has to be 'port@1' (or any number but 0).
>>>
>>> I haven't looked at the driver code, but it should request port 0 and
>>> always get the output port. And requesting port 1 will return an error
>>> or the input port.
>>
>> Hello Rob,
>>
>> I want to remind you that in v2 of this series you said that it'd be wrong for
>> port@0 to be an output, I replied that you misread that as I had modeled it indeed
>> as an input, and then you gave me your Reviewed-by tag.
> 
> I have not misread it. Then I guess I forgot about it and missed it the
> next time on v3.
> 

Okay, that was some misunderstanding then - it's fine, no problem.

>> Anyway - I get your concern about the previous behavior of `port`, but I chose to
>> model this that way purely for consistency.
>>
>> First of all - the driver(s) will check if we're feeding a full graph, as it will
>> indeed first check if port@1 is present: if it is, then it follows this scheme with
>> port@0 as INPUT and port@1 as OUTPUT.
>> If the component in port@0 is an OUTPUT, the bridge attach will fail.
>>
>> Getting to bindings themselves, then... it would be a mistake to model port@0 as an
>> output and port@1 as an input, because that would be not only inconsistent with the
>> DRM Bridge bindings, but would be highly confusing when reading the devicetree.
> 
> Somewhat confusing, yes. Highly, no. Put a comment in the DT.
> 

"Somewhat or highly" boils down to personal opinion, so I still go for "highly"
but won't argue about that as wouldn't be productive and would bring us nowhere
anyway, so, whatever :-)

Putting a comment in DT is an option, yes, but that comment would need to be put
on all of the MediaTek SoC device trees - current and future - and IMO would scream
"inconsistency warning" (in different words, of course) all over, which honestly
doesn't really look clean... at least to my eyes...

>> Please note that the bridge bindings are always declaring port@0 as an INPUT and
>> other ports as OUTPUT(s).
> 
> There is no guarantee on that. Port numbering is local to the bridge and
> opaque to anything outside that bridge. That is why you have to document
> what each port represents.
> 

I know and I agree that there's no guarantee on the numbering. I can see that in
other non-drm-bridge bindings, and that's fine.

> Would we have followed that convention if all the ports were defined
> from the start? Certainly. But that didn't happen and you are stuck with
> the existing binding and ABI.
> 

I thought about adding a new compatible for the new port scheme, but that looked
even worse to me as, after doing that (yeah, I actually went for it in the first
version that I have never sent upstream) during my own proof-read I started
screaming "HACK! HACK! NOOO!" all over, and rewritten the entire thing.

>> As an example, you can check display/bridge/analogix,anx7625.yaml or
>> display/bridge/samsung,mipi-dsim.yaml (and others) for bridges, otherwise
>> display/st,stm32mp25-lvds.yaml or display/allwinner,sun4i-a10-display-frontend.yaml
>> (and others) for display controllers, which do all conform to this logic, where
>> the input is always @0, and the output is @1.
>>
>> Of course, doing this required me to do extra changes to the MTK DRM drivers to
>> actually be retro-compatible with the old devicetrees as I explained before.
> 
> You can't fix existing software...
> 

That's true, but I don't see that as an "excuse" (grant me this term please) to
"carelessly" keep it in a suboptimal state.

This driver has been growing almost uncontrollably with (wrong, anyway!)
board-specific component vectors - and writing a new one would just add up
more code duplication to the mix and/or worsen the maintainability of older
MediaTek SoCs (as the "old" driver will get forgotten and never updated anymore).

> If you want to break the ABI, then that's ultimately up to you and
> Mediatek maintainers to decide0. This case is easy to avoid, why would
> you knowingly break the ABI here.

Because if we don't do this, we condemn (forever) this driver, or part of it
to have an inverted port scheme compared to either:
  A. The other drm/mediatek components; or
  B. The other bridge drivers (of which, some are used with MTK as well)

...and we also condemn (forever, again) all MediaTek device trees to scream
"port inconsistency warning: A for drm/mediatek components, B for every other
thing", which would scream "drm/mediatek is somewhat broken", which can be
avoided.

> OTOH, this seems like a big enough
> change I would imagine it is a matter of time before supporting a
> missing OF graph for the components will be a problem.
> 

Sorry I didn't understand this part, can you please-please-please reword?

Cheers,
Angelo

Re: [PATCH v12 1/3] dt-bindings: display: mediatek: Add OF graph support for board path

[Rob Herring]

On Wed, Oct 16, 2024 at 4:23 AM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 15/10/24 15:48, Rob Herring ha scritto:
> > On Tue, Oct 15, 2024 at 10:32:22AM +0200, AngeloGioacchino Del Regno wrote:
> >> Il 14/10/24 19:36, Rob Herring ha scritto:
> >>> On Mon, Oct 14, 2024 at 3:51 AM AngeloGioacchino Del Regno
> >>> <angelogioacchino.delregno@collabora.com> wrote:
> >>>>
> >>>> The display IPs in MediaTek SoCs support being interconnected with
> >>>> different instances of DDP IPs (for example, merge0 or merge1) and/or
> >>>> with different DDP IPs (for example, rdma can be connected with either
> >>>> color, dpi, dsi, merge, etc), forming a full Display Data Path that
> >>>> ends with an actual display.
> >>>>
> >>>> The final display pipeline is effectively board specific, as it does
> >>>> depend on the display that is attached to it, and eventually on the
> >>>> sensors supported by the board (for example, Adaptive Ambient Light
> >>>> would need an Ambient Light Sensor, otherwise it's pointless!), other
> >>>> than the output type.
> >>>>
> >>>> Add support for OF graphs to most of the MediaTek DDP (display) bindings
> >>>> to add flexibility to build custom hardware paths, hence enabling board
> >>>> specific configuration of the display pipeline and allowing to finally
> >>>> migrate away from using hardcoded paths.
> >>>>
> >>>> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> >>>> Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
> >>>> Tested-by: Alexandre Mergnat <amergnat@baylibre.com>
> >>>> Reviewed-by: CK Hu <ck.hu@mediatek.com>
> >>>> Tested-by: Michael Walle <mwalle@kernel.org> # on kontron-sbc-i1200
> >>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> >>>> ---
> >>>>    .../display/mediatek/mediatek,aal.yaml        | 40 +++++++++++++++++++
> >>>>    .../display/mediatek/mediatek,ccorr.yaml      | 21 ++++++++++
> >>>>    .../display/mediatek/mediatek,color.yaml      | 22 ++++++++++
> >>>>    .../display/mediatek/mediatek,dither.yaml     | 22 ++++++++++
> >>>>    .../display/mediatek/mediatek,dpi.yaml        | 25 +++++++++++-
> >>>>    .../display/mediatek/mediatek,dsc.yaml        | 24 +++++++++++
> >>>>    .../display/mediatek/mediatek,dsi.yaml        | 27 ++++++++++++-
> >>>>    .../display/mediatek/mediatek,ethdr.yaml      | 22 ++++++++++
> >>>>    .../display/mediatek/mediatek,gamma.yaml      | 19 +++++++++
> >>>>    .../display/mediatek/mediatek,merge.yaml      | 23 +++++++++++
> >>>>    .../display/mediatek/mediatek,od.yaml         | 22 ++++++++++
> >>>>    .../display/mediatek/mediatek,ovl-2l.yaml     | 22 ++++++++++
> >>>>    .../display/mediatek/mediatek,ovl.yaml        | 22 ++++++++++
> >>>>    .../display/mediatek/mediatek,postmask.yaml   | 21 ++++++++++
> >>>>    .../display/mediatek/mediatek,rdma.yaml       | 22 ++++++++++
> >>>>    .../display/mediatek/mediatek,ufoe.yaml       | 21 ++++++++++
> >>>>    16 files changed, 372 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
> >>>> index cf24434854ff..47ddba5c41af 100644
> >>>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
> >>>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
> >>>> @@ -62,6 +62,27 @@ properties:
> >>>>        $ref: /schemas/types.yaml#/definitions/phandle-array
> >>>>        maxItems: 1
> >>>>
> >>>> +  ports:
> >>>> +    $ref: /schemas/graph.yaml#/properties/ports
> >>>> +    description:
> >>>> +      Input and output ports can have multiple endpoints, each of those
> >>>> +      connects to either the primary, secondary, etc, display pipeline.
> >>>> +
> >>>> +    properties:
> >>>> +      port@0:
> >>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>> +        description: AAL input port
> >>>> +
> >>>> +      port@1:
> >>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>> +        description:
> >>>> +          AAL output to the next component's input, for example could be one
> >>>> +          of many gamma, overdrive or other blocks.
> >>>> +
> >>>> +    required:
> >>>> +      - port@0
> >>>> +      - port@1
> >>>> +
> >>>>    required:
> >>>>      - compatible
> >>>>      - reg
> >>>> @@ -89,5 +110,24 @@ examples:
> >>>>               power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
> >>>>               clocks = <&mmsys CLK_MM_DISP_AAL>;
> >>>>               mediatek,gce-client-reg = <&gce SUBSYS_1401XXXX 0x5000 0x1000>;
> >>>> +
> >>>> +           ports {
> >>>> +               #address-cells = <1>;
> >>>> +               #size-cells = <0>;
> >>>> +
> >>>> +               port@0 {
> >>>> +                   reg = <0>;
> >>>> +                   aal0_in: endpoint {
> >>>> +                       remote-endpoint = <&ccorr0_out>;
> >>>> +                   };
> >>>> +               };
> >>>> +
> >>>> +               port@1 {
> >>>> +                   reg = <1>;
> >>>> +                   aal0_out: endpoint {
> >>>> +                       remote-endpoint = <&gamma0_in>;
> >>>> +                   };
> >>>> +               };
> >>>> +           };
> >>>>           };
> >>>>        };
> >>>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
> >>>> index 9f8366763831..fca8e7bb0cbc 100644
> >>>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
> >>>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
> >>>> @@ -57,6 +57,27 @@ properties:
> >>>>        $ref: /schemas/types.yaml#/definitions/phandle-array
> >>>>        maxItems: 1
> >>>>
> >>>> +  ports:
> >>>> +    $ref: /schemas/graph.yaml#/properties/ports
> >>>> +    description:
> >>>> +      Input and output ports can have multiple endpoints, each of those
> >>>> +      connects to either the primary, secondary, etc, display pipeline.
> >>>> +
> >>>> +    properties:
> >>>> +      port@0:
> >>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>> +        description: CCORR input port
> >>>> +
> >>>> +      port@1:
> >>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>> +        description:
> >>>> +          CCORR output to the input of the next desired component in the
> >>>> +          display pipeline, usually only one of the available AAL blocks.
> >>>> +
> >>>> +    required:
> >>>> +      - port@0
> >>>> +      - port@1
> >>>> +
> >>>>    required:
> >>>>      - compatible
> >>>>      - reg
> >>>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
> >>>> index 7df786bbad20..6160439ce4d7 100644
> >>>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
> >>>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
> >>>> @@ -65,6 +65,28 @@ properties:
> >>>>        $ref: /schemas/types.yaml#/definitions/phandle-array
> >>>>        maxItems: 1
> >>>>
> >>>> +  ports:
> >>>> +    $ref: /schemas/graph.yaml#/properties/ports
> >>>> +    description:
> >>>> +      Input and output ports can have multiple endpoints, each of those
> >>>> +      connects to either the primary, secondary, etc, display pipeline.
> >>>> +
> >>>> +    properties:
> >>>> +      port@0:
> >>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>> +        description: COLOR input port
> >>>> +
> >>>> +      port@1:
> >>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>> +        description:
> >>>> +          COLOR output to the input of the next desired component in the
> >>>> +          display pipeline, for example one of the available CCORR or AAL
> >>>> +          blocks.
> >>>> +
> >>>> +    required:
> >>>> +      - port@0
> >>>> +      - port@1
> >>>> +
> >>>>    required:
> >>>>      - compatible
> >>>>      - reg
> >>>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
> >>>> index 6fceb1f95d2a..abaf27916d13 100644
> >>>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
> >>>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
> >>>> @@ -56,6 +56,28 @@ properties:
> >>>>        $ref: /schemas/types.yaml#/definitions/phandle-array
> >>>>        maxItems: 1
> >>>>
> >>>> +  ports:
> >>>> +    $ref: /schemas/graph.yaml#/properties/ports
> >>>> +    description:
> >>>> +      Input and output ports can have multiple endpoints, each of those
> >>>> +      connects to either the primary, secondary, etc, display pipeline.
> >>>> +
> >>>> +    properties:
> >>>> +      port@0:
> >>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>> +        description: DITHER input, usually from a POSTMASK or GAMMA block.
> >>>> +
> >>>> +      port@1:
> >>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>> +        description:
> >>>> +          DITHER output to the input of the next desired component in the
> >>>> +          display pipeline, for example one of the available DSC compressors,
> >>>> +          DP_INTF, DSI, LVDS or others.
> >>>> +
> >>>> +    required:
> >>>> +      - port@0
> >>>> +      - port@1
> >>>> +
> >>>>    required:
> >>>>      - compatible
> >>>>      - reg
> >>>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
> >>>> index 3a82aec9021c..b567e3d58aa1 100644
> >>>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
> >>>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
> >>>> @@ -71,13 +71,34 @@ properties:
> >>>>          Output port node. This port should be connected to the input port of an
> >>>>          attached HDMI, LVDS or DisplayPort encoder chip.
> >>>>
> >>>> +  ports:
> >>>> +    $ref: /schemas/graph.yaml#/properties/ports
> >>>> +
> >>>> +    properties:
> >>>> +      port@0:
> >>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>> +        description: DPI input port
> >>>> +
> >>>> +      port@1:
> >>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>> +        description: DPI output to an HDMI, LVDS or DisplayPort encoder input
> >>>
> >>> This is wrong. The existing 'port' is the output. 'port' and 'port@0'
> >>> are treated as the same thing. Since you are adding an input port, the
> >>> new port has to be 'port@1' (or any number but 0).
> >>>
> >>> I haven't looked at the driver code, but it should request port 0 and
> >>> always get the output port. And requesting port 1 will return an error
> >>> or the input port.
> >>
> >> Hello Rob,
> >>
> >> I want to remind you that in v2 of this series you said that it'd be wrong for
> >> port@0 to be an output, I replied that you misread that as I had modeled it indeed
> >> as an input, and then you gave me your Reviewed-by tag.
> >
> > I have not misread it. Then I guess I forgot about it and missed it the
> > next time on v3.
> >
>
> Okay, that was some misunderstanding then - it's fine, no problem.
>
> >> Anyway - I get your concern about the previous behavior of `port`, but I chose to
> >> model this that way purely for consistency.
> >>
> >> First of all - the driver(s) will check if we're feeding a full graph, as it will
> >> indeed first check if port@1 is present: if it is, then it follows this scheme with
> >> port@0 as INPUT and port@1 as OUTPUT.
> >> If the component in port@0 is an OUTPUT, the bridge attach will fail.
> >>
> >> Getting to bindings themselves, then... it would be a mistake to model port@0 as an
> >> output and port@1 as an input, because that would be not only inconsistent with the
> >> DRM Bridge bindings, but would be highly confusing when reading the devicetree.
> >
> > Somewhat confusing, yes. Highly, no. Put a comment in the DT.
> >
>
> "Somewhat or highly" boils down to personal opinion, so I still go for "highly"
> but won't argue about that as wouldn't be productive and would bring us nowhere
> anyway, so, whatever :-)
>
> Putting a comment in DT is an option, yes, but that comment would need to be put
> on all of the MediaTek SoC device trees - current and future - and IMO would scream
> "inconsistency warning" (in different words, of course) all over, which honestly
> doesn't really look clean... at least to my eyes...

What I find more confusing is my updated DT doesn't work with my
existing kernel...

> >> Please note that the bridge bindings are always declaring port@0 as an INPUT and
> >> other ports as OUTPUT(s).
> >
> > There is no guarantee on that. Port numbering is local to the bridge and
> > opaque to anything outside that bridge. That is why you have to document
> > what each port represents.
> >
>
> I know and I agree that there's no guarantee on the numbering. I can see that in
> other non-drm-bridge bindings, and that's fine.
>
> > Would we have followed that convention if all the ports were defined
> > from the start? Certainly. But that didn't happen and you are stuck with
> > the existing binding and ABI.
> >
>
> I thought about adding a new compatible for the new port scheme, but that looked
> even worse to me as, after doing that (yeah, I actually went for it in the first
> version that I have never sent upstream) during my own proof-read I started
> screaming "HACK! HACK! NOOO!" all over, and rewritten the entire thing.
>
> >> As an example, you can check display/bridge/analogix,anx7625.yaml or
> >> display/bridge/samsung,mipi-dsim.yaml (and others) for bridges, otherwise
> >> display/st,stm32mp25-lvds.yaml or display/allwinner,sun4i-a10-display-frontend.yaml
> >> (and others) for display controllers, which do all conform to this logic, where
> >> the input is always @0, and the output is @1.
> >>
> >> Of course, doing this required me to do extra changes to the MTK DRM drivers to
> >> actually be retro-compatible with the old devicetrees as I explained before.
> >
> > You can't fix existing software...
> >
>
> That's true, but I don't see that as an "excuse" (grant me this term please) to
> "carelessly" keep it in a suboptimal state.
>
> This driver has been growing almost uncontrollably with (wrong, anyway!)
> board-specific component vectors - and writing a new one would just add up
> more code duplication to the mix and/or worsen the maintainability of older
> MediaTek SoCs (as the "old" driver will get forgotten and never updated anymore).
>
> > If you want to break the ABI, then that's ultimately up to you and
> > Mediatek maintainers to decide0. This case is easy to avoid, why would
> > you knowingly break the ABI here.
>
> Because if we don't do this, we condemn (forever) this driver, or part of it
> to have an inverted port scheme compared to either:
>   A. The other drm/mediatek components; or
>   B. The other bridge drivers (of which, some are used with MTK as well)
>
> ...and we also condemn (forever, again) all MediaTek device trees to scream
> "port inconsistency warning: A for drm/mediatek components, B for every other
> thing", which would scream "drm/mediatek is somewhat broken", which can be
> avoided.

Sure. The cost is just an ABI break to do that.

> > OTOH, this seems like a big enough
> > change I would imagine it is a matter of time before supporting a
> > missing OF graph for the components will be a problem.
> >
>
> Sorry I didn't understand this part, can you please-please-please reword?

I think keeping the kernel working with the old and new binding will
be a challenge. Partly because testing with the old binding won't
happen, but also if the binding and drivers continue to evolve. So
while maintaining old ABI might be possible with this change, it will
continue to be an issue with each change. BTW, did you actually test
backwards compatibility with this? I can see you fallback to the old
binding, but there's a lot of other changes in there I can't really
tell by looking at it.

What I haven't heard from you is "yes, we need to maintain the ABI" or
"no, we can break it". Decide that, then the path here is simple.

Rob

Re: [PATCH v12 1/3] dt-bindings: display: mediatek: Add OF graph support for board path

[AngeloGioacchino Del Regno]

Il 16/10/24 16:00, Rob Herring ha scritto:
> On Wed, Oct 16, 2024 at 4:23 AM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Il 15/10/24 15:48, Rob Herring ha scritto:
>>> On Tue, Oct 15, 2024 at 10:32:22AM +0200, AngeloGioacchino Del Regno wrote:
>>>> Il 14/10/24 19:36, Rob Herring ha scritto:
>>>>> On Mon, Oct 14, 2024 at 3:51 AM AngeloGioacchino Del Regno
>>>>> <angelogioacchino.delregno@collabora.com> wrote:
>>>>>>
>>>>>> The display IPs in MediaTek SoCs support being interconnected with
>>>>>> different instances of DDP IPs (for example, merge0 or merge1) and/or
>>>>>> with different DDP IPs (for example, rdma can be connected with either
>>>>>> color, dpi, dsi, merge, etc), forming a full Display Data Path that
>>>>>> ends with an actual display.
>>>>>>
>>>>>> The final display pipeline is effectively board specific, as it does
>>>>>> depend on the display that is attached to it, and eventually on the
>>>>>> sensors supported by the board (for example, Adaptive Ambient Light
>>>>>> would need an Ambient Light Sensor, otherwise it's pointless!), other
>>>>>> than the output type.
>>>>>>
>>>>>> Add support for OF graphs to most of the MediaTek DDP (display) bindings
>>>>>> to add flexibility to build custom hardware paths, hence enabling board
>>>>>> specific configuration of the display pipeline and allowing to finally
>>>>>> migrate away from using hardcoded paths.
>>>>>>
>>>>>> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
>>>>>> Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
>>>>>> Tested-by: Alexandre Mergnat <amergnat@baylibre.com>
>>>>>> Reviewed-by: CK Hu <ck.hu@mediatek.com>
>>>>>> Tested-by: Michael Walle <mwalle@kernel.org> # on kontron-sbc-i1200
>>>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>>>>> ---
>>>>>>     .../display/mediatek/mediatek,aal.yaml        | 40 +++++++++++++++++++
>>>>>>     .../display/mediatek/mediatek,ccorr.yaml      | 21 ++++++++++
>>>>>>     .../display/mediatek/mediatek,color.yaml      | 22 ++++++++++
>>>>>>     .../display/mediatek/mediatek,dither.yaml     | 22 ++++++++++
>>>>>>     .../display/mediatek/mediatek,dpi.yaml        | 25 +++++++++++-
>>>>>>     .../display/mediatek/mediatek,dsc.yaml        | 24 +++++++++++
>>>>>>     .../display/mediatek/mediatek,dsi.yaml        | 27 ++++++++++++-
>>>>>>     .../display/mediatek/mediatek,ethdr.yaml      | 22 ++++++++++
>>>>>>     .../display/mediatek/mediatek,gamma.yaml      | 19 +++++++++
>>>>>>     .../display/mediatek/mediatek,merge.yaml      | 23 +++++++++++
>>>>>>     .../display/mediatek/mediatek,od.yaml         | 22 ++++++++++
>>>>>>     .../display/mediatek/mediatek,ovl-2l.yaml     | 22 ++++++++++
>>>>>>     .../display/mediatek/mediatek,ovl.yaml        | 22 ++++++++++
>>>>>>     .../display/mediatek/mediatek,postmask.yaml   | 21 ++++++++++
>>>>>>     .../display/mediatek/mediatek,rdma.yaml       | 22 ++++++++++
>>>>>>     .../display/mediatek/mediatek,ufoe.yaml       | 21 ++++++++++
>>>>>>     16 files changed, 372 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
>>>>>> index cf24434854ff..47ddba5c41af 100644
>>>>>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
>>>>>> @@ -62,6 +62,27 @@ properties:
>>>>>>         $ref: /schemas/types.yaml#/definitions/phandle-array
>>>>>>         maxItems: 1
>>>>>>
>>>>>> +  ports:
>>>>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>>>>> +    description:
>>>>>> +      Input and output ports can have multiple endpoints, each of those
>>>>>> +      connects to either the primary, secondary, etc, display pipeline.
>>>>>> +
>>>>>> +    properties:
>>>>>> +      port@0:
>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>>> +        description: AAL input port
>>>>>> +
>>>>>> +      port@1:
>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>>> +        description:
>>>>>> +          AAL output to the next component's input, for example could be one
>>>>>> +          of many gamma, overdrive or other blocks.
>>>>>> +
>>>>>> +    required:
>>>>>> +      - port@0
>>>>>> +      - port@1
>>>>>> +
>>>>>>     required:
>>>>>>       - compatible
>>>>>>       - reg
>>>>>> @@ -89,5 +110,24 @@ examples:
>>>>>>                power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
>>>>>>                clocks = <&mmsys CLK_MM_DISP_AAL>;
>>>>>>                mediatek,gce-client-reg = <&gce SUBSYS_1401XXXX 0x5000 0x1000>;
>>>>>> +
>>>>>> +           ports {
>>>>>> +               #address-cells = <1>;
>>>>>> +               #size-cells = <0>;
>>>>>> +
>>>>>> +               port@0 {
>>>>>> +                   reg = <0>;
>>>>>> +                   aal0_in: endpoint {
>>>>>> +                       remote-endpoint = <&ccorr0_out>;
>>>>>> +                   };
>>>>>> +               };
>>>>>> +
>>>>>> +               port@1 {
>>>>>> +                   reg = <1>;
>>>>>> +                   aal0_out: endpoint {
>>>>>> +                       remote-endpoint = <&gamma0_in>;
>>>>>> +                   };
>>>>>> +               };
>>>>>> +           };
>>>>>>            };
>>>>>>         };
>>>>>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
>>>>>> index 9f8366763831..fca8e7bb0cbc 100644
>>>>>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
>>>>>> @@ -57,6 +57,27 @@ properties:
>>>>>>         $ref: /schemas/types.yaml#/definitions/phandle-array
>>>>>>         maxItems: 1
>>>>>>
>>>>>> +  ports:
>>>>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>>>>> +    description:
>>>>>> +      Input and output ports can have multiple endpoints, each of those
>>>>>> +      connects to either the primary, secondary, etc, display pipeline.
>>>>>> +
>>>>>> +    properties:
>>>>>> +      port@0:
>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>>> +        description: CCORR input port
>>>>>> +
>>>>>> +      port@1:
>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>>> +        description:
>>>>>> +          CCORR output to the input of the next desired component in the
>>>>>> +          display pipeline, usually only one of the available AAL blocks.
>>>>>> +
>>>>>> +    required:
>>>>>> +      - port@0
>>>>>> +      - port@1
>>>>>> +
>>>>>>     required:
>>>>>>       - compatible
>>>>>>       - reg
>>>>>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
>>>>>> index 7df786bbad20..6160439ce4d7 100644
>>>>>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
>>>>>> @@ -65,6 +65,28 @@ properties:
>>>>>>         $ref: /schemas/types.yaml#/definitions/phandle-array
>>>>>>         maxItems: 1
>>>>>>
>>>>>> +  ports:
>>>>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>>>>> +    description:
>>>>>> +      Input and output ports can have multiple endpoints, each of those
>>>>>> +      connects to either the primary, secondary, etc, display pipeline.
>>>>>> +
>>>>>> +    properties:
>>>>>> +      port@0:
>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>>> +        description: COLOR input port
>>>>>> +
>>>>>> +      port@1:
>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>>> +        description:
>>>>>> +          COLOR output to the input of the next desired component in the
>>>>>> +          display pipeline, for example one of the available CCORR or AAL
>>>>>> +          blocks.
>>>>>> +
>>>>>> +    required:
>>>>>> +      - port@0
>>>>>> +      - port@1
>>>>>> +
>>>>>>     required:
>>>>>>       - compatible
>>>>>>       - reg
>>>>>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
>>>>>> index 6fceb1f95d2a..abaf27916d13 100644
>>>>>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
>>>>>> @@ -56,6 +56,28 @@ properties:
>>>>>>         $ref: /schemas/types.yaml#/definitions/phandle-array
>>>>>>         maxItems: 1
>>>>>>
>>>>>> +  ports:
>>>>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>>>>> +    description:
>>>>>> +      Input and output ports can have multiple endpoints, each of those
>>>>>> +      connects to either the primary, secondary, etc, display pipeline.
>>>>>> +
>>>>>> +    properties:
>>>>>> +      port@0:
>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>>> +        description: DITHER input, usually from a POSTMASK or GAMMA block.
>>>>>> +
>>>>>> +      port@1:
>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>>> +        description:
>>>>>> +          DITHER output to the input of the next desired component in the
>>>>>> +          display pipeline, for example one of the available DSC compressors,
>>>>>> +          DP_INTF, DSI, LVDS or others.
>>>>>> +
>>>>>> +    required:
>>>>>> +      - port@0
>>>>>> +      - port@1
>>>>>> +
>>>>>>     required:
>>>>>>       - compatible
>>>>>>       - reg
>>>>>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
>>>>>> index 3a82aec9021c..b567e3d58aa1 100644
>>>>>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
>>>>>> @@ -71,13 +71,34 @@ properties:
>>>>>>           Output port node. This port should be connected to the input port of an
>>>>>>           attached HDMI, LVDS or DisplayPort encoder chip.
>>>>>>
>>>>>> +  ports:
>>>>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>>>>> +
>>>>>> +    properties:
>>>>>> +      port@0:
>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>>> +        description: DPI input port
>>>>>> +
>>>>>> +      port@1:
>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>>> +        description: DPI output to an HDMI, LVDS or DisplayPort encoder input
>>>>>
>>>>> This is wrong. The existing 'port' is the output. 'port' and 'port@0'
>>>>> are treated as the same thing. Since you are adding an input port, the
>>>>> new port has to be 'port@1' (or any number but 0).
>>>>>
>>>>> I haven't looked at the driver code, but it should request port 0 and
>>>>> always get the output port. And requesting port 1 will return an error
>>>>> or the input port.
>>>>
>>>> Hello Rob,
>>>>
>>>> I want to remind you that in v2 of this series you said that it'd be wrong for
>>>> port@0 to be an output, I replied that you misread that as I had modeled it indeed
>>>> as an input, and then you gave me your Reviewed-by tag.
>>>
>>> I have not misread it. Then I guess I forgot about it and missed it the
>>> next time on v3.
>>>
>>
>> Okay, that was some misunderstanding then - it's fine, no problem.
>>
>>>> Anyway - I get your concern about the previous behavior of `port`, but I chose to
>>>> model this that way purely for consistency.
>>>>
>>>> First of all - the driver(s) will check if we're feeding a full graph, as it will
>>>> indeed first check if port@1 is present: if it is, then it follows this scheme with
>>>> port@0 as INPUT and port@1 as OUTPUT.
>>>> If the component in port@0 is an OUTPUT, the bridge attach will fail.
>>>>
>>>> Getting to bindings themselves, then... it would be a mistake to model port@0 as an
>>>> output and port@1 as an input, because that would be not only inconsistent with the
>>>> DRM Bridge bindings, but would be highly confusing when reading the devicetree.
>>>
>>> Somewhat confusing, yes. Highly, no. Put a comment in the DT.
>>>
>>
>> "Somewhat or highly" boils down to personal opinion, so I still go for "highly"
>> but won't argue about that as wouldn't be productive and would bring us nowhere
>> anyway, so, whatever :-)
>>
>> Putting a comment in DT is an option, yes, but that comment would need to be put
>> on all of the MediaTek SoC device trees - current and future - and IMO would scream
>> "inconsistency warning" (in different words, of course) all over, which honestly
>> doesn't really look clean... at least to my eyes...
> 
> What I find more confusing is my updated DT doesn't work with my
> existing kernel...
> 
>>>> Please note that the bridge bindings are always declaring port@0 as an INPUT and
>>>> other ports as OUTPUT(s).
>>>
>>> There is no guarantee on that. Port numbering is local to the bridge and
>>> opaque to anything outside that bridge. That is why you have to document
>>> what each port represents.
>>>
>>
>> I know and I agree that there's no guarantee on the numbering. I can see that in
>> other non-drm-bridge bindings, and that's fine.
>>
>>> Would we have followed that convention if all the ports were defined
>>> from the start? Certainly. But that didn't happen and you are stuck with
>>> the existing binding and ABI.
>>>
>>
>> I thought about adding a new compatible for the new port scheme, but that looked
>> even worse to me as, after doing that (yeah, I actually went for it in the first
>> version that I have never sent upstream) during my own proof-read I started
>> screaming "HACK! HACK! NOOO!" all over, and rewritten the entire thing.
>>
>>>> As an example, you can check display/bridge/analogix,anx7625.yaml or
>>>> display/bridge/samsung,mipi-dsim.yaml (and others) for bridges, otherwise
>>>> display/st,stm32mp25-lvds.yaml or display/allwinner,sun4i-a10-display-frontend.yaml
>>>> (and others) for display controllers, which do all conform to this logic, where
>>>> the input is always @0, and the output is @1.
>>>>
>>>> Of course, doing this required me to do extra changes to the MTK DRM drivers to
>>>> actually be retro-compatible with the old devicetrees as I explained before.
>>>
>>> You can't fix existing software...
>>>
>>
>> That's true, but I don't see that as an "excuse" (grant me this term please) to
>> "carelessly" keep it in a suboptimal state.
>>
>> This driver has been growing almost uncontrollably with (wrong, anyway!)
>> board-specific component vectors - and writing a new one would just add up
>> more code duplication to the mix and/or worsen the maintainability of older
>> MediaTek SoCs (as the "old" driver will get forgotten and never updated anymore).
>>
>>> If you want to break the ABI, then that's ultimately up to you and
>>> Mediatek maintainers to decide0. This case is easy to avoid, why would
>>> you knowingly break the ABI here.
>>
>> Because if we don't do this, we condemn (forever) this driver, or part of it
>> to have an inverted port scheme compared to either:
>>    A. The other drm/mediatek components; or
>>    B. The other bridge drivers (of which, some are used with MTK as well)
>>
>> ...and we also condemn (forever, again) all MediaTek device trees to scream
>> "port inconsistency warning: A for drm/mediatek components, B for every other
>> thing", which would scream "drm/mediatek is somewhat broken", which can be
>> avoided.
> 
> Sure. The cost is just an ABI break to do that.
> 
>>> OTOH, this seems like a big enough
>>> change I would imagine it is a matter of time before supporting a
>>> missing OF graph for the components will be a problem.
>>>
>>
>> Sorry I didn't understand this part, can you please-please-please reword?
> 
> I think keeping the kernel working with the old and new binding will
> be a challenge. Partly because testing with the old binding won't
> happen, but also if the binding and drivers continue to evolve. So
> while maintaining old ABI might be possible with this change, it will
> continue to be an issue with each change.

That's... exactly my point, so I am happy that we agree on that.

> BTW, did you actually test
> backwards compatibility with this? I can see you fallback to the old
> binding, but there's a lot of other changes in there I can't really
> tell by looking at it.


Short answer: Yes, largely

Long answer:

Yes, with this series applied, I have tested both *old* and *new* devicetrees on
7 boards with 4 different SoCs and different display pipelines (hence, graphs):
one smartphone (MT6795), a "bunch of" Chromebooks (MT8173, MT8186, MT8195), and
a SBC (MT8195).

Of course those had DSI or eDP displays, with or without DisplayPort external
display support.

> 
> What I haven't heard from you is "yes, we need to maintain the ABI" or
> "no, we can break it". Decide that, then the path here is simple.

No, we have to break it.

Cheers,
Angelo

Re: [PATCH v12 1/3] dt-bindings: display: mediatek: Add OF graph support for board path

[Rob Herring]

On Wed, Oct 16, 2024 at 10:26 AM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 16/10/24 16:00, Rob Herring ha scritto:
> > On Wed, Oct 16, 2024 at 4:23 AM AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@collabora.com> wrote:
> >>
> >> Il 15/10/24 15:48, Rob Herring ha scritto:
> >>> On Tue, Oct 15, 2024 at 10:32:22AM +0200, AngeloGioacchino Del Regno wrote:
> >>>> Il 14/10/24 19:36, Rob Herring ha scritto:
> >>>>> On Mon, Oct 14, 2024 at 3:51 AM AngeloGioacchino Del Regno
> >>>>> <angelogioacchino.delregno@collabora.com> wrote:
> >>>>>>
> >>>>>> The display IPs in MediaTek SoCs support being interconnected with
> >>>>>> different instances of DDP IPs (for example, merge0 or merge1) and/or
> >>>>>> with different DDP IPs (for example, rdma can be connected with either
> >>>>>> color, dpi, dsi, merge, etc), forming a full Display Data Path that
> >>>>>> ends with an actual display.
> >>>>>>
> >>>>>> The final display pipeline is effectively board specific, as it does
> >>>>>> depend on the display that is attached to it, and eventually on the
> >>>>>> sensors supported by the board (for example, Adaptive Ambient Light
> >>>>>> would need an Ambient Light Sensor, otherwise it's pointless!), other
> >>>>>> than the output type.
> >>>>>>
> >>>>>> Add support for OF graphs to most of the MediaTek DDP (display) bindings
> >>>>>> to add flexibility to build custom hardware paths, hence enabling board
> >>>>>> specific configuration of the display pipeline and allowing to finally
> >>>>>> migrate away from using hardcoded paths.
> >>>>>>
> >>>>>> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> >>>>>> Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
> >>>>>> Tested-by: Alexandre Mergnat <amergnat@baylibre.com>
> >>>>>> Reviewed-by: CK Hu <ck.hu@mediatek.com>
> >>>>>> Tested-by: Michael Walle <mwalle@kernel.org> # on kontron-sbc-i1200
> >>>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> >>>>>> ---
> >>>>>>     .../display/mediatek/mediatek,aal.yaml        | 40 +++++++++++++++++++
> >>>>>>     .../display/mediatek/mediatek,ccorr.yaml      | 21 ++++++++++
> >>>>>>     .../display/mediatek/mediatek,color.yaml      | 22 ++++++++++
> >>>>>>     .../display/mediatek/mediatek,dither.yaml     | 22 ++++++++++
> >>>>>>     .../display/mediatek/mediatek,dpi.yaml        | 25 +++++++++++-
> >>>>>>     .../display/mediatek/mediatek,dsc.yaml        | 24 +++++++++++
> >>>>>>     .../display/mediatek/mediatek,dsi.yaml        | 27 ++++++++++++-
> >>>>>>     .../display/mediatek/mediatek,ethdr.yaml      | 22 ++++++++++
> >>>>>>     .../display/mediatek/mediatek,gamma.yaml      | 19 +++++++++
> >>>>>>     .../display/mediatek/mediatek,merge.yaml      | 23 +++++++++++
> >>>>>>     .../display/mediatek/mediatek,od.yaml         | 22 ++++++++++
> >>>>>>     .../display/mediatek/mediatek,ovl-2l.yaml     | 22 ++++++++++
> >>>>>>     .../display/mediatek/mediatek,ovl.yaml        | 22 ++++++++++
> >>>>>>     .../display/mediatek/mediatek,postmask.yaml   | 21 ++++++++++
> >>>>>>     .../display/mediatek/mediatek,rdma.yaml       | 22 ++++++++++
> >>>>>>     .../display/mediatek/mediatek,ufoe.yaml       | 21 ++++++++++
> >>>>>>     16 files changed, 372 insertions(+), 3 deletions(-)
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
> >>>>>> index cf24434854ff..47ddba5c41af 100644
> >>>>>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
> >>>>>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
> >>>>>> @@ -62,6 +62,27 @@ properties:
> >>>>>>         $ref: /schemas/types.yaml#/definitions/phandle-array
> >>>>>>         maxItems: 1
> >>>>>>
> >>>>>> +  ports:
> >>>>>> +    $ref: /schemas/graph.yaml#/properties/ports
> >>>>>> +    description:
> >>>>>> +      Input and output ports can have multiple endpoints, each of those
> >>>>>> +      connects to either the primary, secondary, etc, display pipeline.
> >>>>>> +
> >>>>>> +    properties:
> >>>>>> +      port@0:
> >>>>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>>>> +        description: AAL input port
> >>>>>> +
> >>>>>> +      port@1:
> >>>>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>>>> +        description:
> >>>>>> +          AAL output to the next component's input, for example could be one
> >>>>>> +          of many gamma, overdrive or other blocks.
> >>>>>> +
> >>>>>> +    required:
> >>>>>> +      - port@0
> >>>>>> +      - port@1
> >>>>>> +
> >>>>>>     required:
> >>>>>>       - compatible
> >>>>>>       - reg
> >>>>>> @@ -89,5 +110,24 @@ examples:
> >>>>>>                power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
> >>>>>>                clocks = <&mmsys CLK_MM_DISP_AAL>;
> >>>>>>                mediatek,gce-client-reg = <&gce SUBSYS_1401XXXX 0x5000 0x1000>;
> >>>>>> +
> >>>>>> +           ports {
> >>>>>> +               #address-cells = <1>;
> >>>>>> +               #size-cells = <0>;
> >>>>>> +
> >>>>>> +               port@0 {
> >>>>>> +                   reg = <0>;
> >>>>>> +                   aal0_in: endpoint {
> >>>>>> +                       remote-endpoint = <&ccorr0_out>;
> >>>>>> +                   };
> >>>>>> +               };
> >>>>>> +
> >>>>>> +               port@1 {
> >>>>>> +                   reg = <1>;
> >>>>>> +                   aal0_out: endpoint {
> >>>>>> +                       remote-endpoint = <&gamma0_in>;
> >>>>>> +                   };
> >>>>>> +               };
> >>>>>> +           };
> >>>>>>            };
> >>>>>>         };
> >>>>>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
> >>>>>> index 9f8366763831..fca8e7bb0cbc 100644
> >>>>>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
> >>>>>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
> >>>>>> @@ -57,6 +57,27 @@ properties:
> >>>>>>         $ref: /schemas/types.yaml#/definitions/phandle-array
> >>>>>>         maxItems: 1
> >>>>>>
> >>>>>> +  ports:
> >>>>>> +    $ref: /schemas/graph.yaml#/properties/ports
> >>>>>> +    description:
> >>>>>> +      Input and output ports can have multiple endpoints, each of those
> >>>>>> +      connects to either the primary, secondary, etc, display pipeline.
> >>>>>> +
> >>>>>> +    properties:
> >>>>>> +      port@0:
> >>>>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>>>> +        description: CCORR input port
> >>>>>> +
> >>>>>> +      port@1:
> >>>>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>>>> +        description:
> >>>>>> +          CCORR output to the input of the next desired component in the
> >>>>>> +          display pipeline, usually only one of the available AAL blocks.
> >>>>>> +
> >>>>>> +    required:
> >>>>>> +      - port@0
> >>>>>> +      - port@1
> >>>>>> +
> >>>>>>     required:
> >>>>>>       - compatible
> >>>>>>       - reg
> >>>>>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
> >>>>>> index 7df786bbad20..6160439ce4d7 100644
> >>>>>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
> >>>>>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
> >>>>>> @@ -65,6 +65,28 @@ properties:
> >>>>>>         $ref: /schemas/types.yaml#/definitions/phandle-array
> >>>>>>         maxItems: 1
> >>>>>>
> >>>>>> +  ports:
> >>>>>> +    $ref: /schemas/graph.yaml#/properties/ports
> >>>>>> +    description:
> >>>>>> +      Input and output ports can have multiple endpoints, each of those
> >>>>>> +      connects to either the primary, secondary, etc, display pipeline.
> >>>>>> +
> >>>>>> +    properties:
> >>>>>> +      port@0:
> >>>>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>>>> +        description: COLOR input port
> >>>>>> +
> >>>>>> +      port@1:
> >>>>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>>>> +        description:
> >>>>>> +          COLOR output to the input of the next desired component in the
> >>>>>> +          display pipeline, for example one of the available CCORR or AAL
> >>>>>> +          blocks.
> >>>>>> +
> >>>>>> +    required:
> >>>>>> +      - port@0
> >>>>>> +      - port@1
> >>>>>> +
> >>>>>>     required:
> >>>>>>       - compatible
> >>>>>>       - reg
> >>>>>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
> >>>>>> index 6fceb1f95d2a..abaf27916d13 100644
> >>>>>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
> >>>>>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
> >>>>>> @@ -56,6 +56,28 @@ properties:
> >>>>>>         $ref: /schemas/types.yaml#/definitions/phandle-array
> >>>>>>         maxItems: 1
> >>>>>>
> >>>>>> +  ports:
> >>>>>> +    $ref: /schemas/graph.yaml#/properties/ports
> >>>>>> +    description:
> >>>>>> +      Input and output ports can have multiple endpoints, each of those
> >>>>>> +      connects to either the primary, secondary, etc, display pipeline.
> >>>>>> +
> >>>>>> +    properties:
> >>>>>> +      port@0:
> >>>>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>>>> +        description: DITHER input, usually from a POSTMASK or GAMMA block.
> >>>>>> +
> >>>>>> +      port@1:
> >>>>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>>>> +        description:
> >>>>>> +          DITHER output to the input of the next desired component in the
> >>>>>> +          display pipeline, for example one of the available DSC compressors,
> >>>>>> +          DP_INTF, DSI, LVDS or others.
> >>>>>> +
> >>>>>> +    required:
> >>>>>> +      - port@0
> >>>>>> +      - port@1
> >>>>>> +
> >>>>>>     required:
> >>>>>>       - compatible
> >>>>>>       - reg
> >>>>>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
> >>>>>> index 3a82aec9021c..b567e3d58aa1 100644
> >>>>>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
> >>>>>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
> >>>>>> @@ -71,13 +71,34 @@ properties:
> >>>>>>           Output port node. This port should be connected to the input port of an
> >>>>>>           attached HDMI, LVDS or DisplayPort encoder chip.
> >>>>>>
> >>>>>> +  ports:
> >>>>>> +    $ref: /schemas/graph.yaml#/properties/ports
> >>>>>> +
> >>>>>> +    properties:
> >>>>>> +      port@0:
> >>>>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>>>> +        description: DPI input port
> >>>>>> +
> >>>>>> +      port@1:
> >>>>>> +        $ref: /schemas/graph.yaml#/properties/port
> >>>>>> +        description: DPI output to an HDMI, LVDS or DisplayPort encoder input
> >>>>>
> >>>>> This is wrong. The existing 'port' is the output. 'port' and 'port@0'
> >>>>> are treated as the same thing. Since you are adding an input port, the
> >>>>> new port has to be 'port@1' (or any number but 0).
> >>>>>
> >>>>> I haven't looked at the driver code, but it should request port 0 and
> >>>>> always get the output port. And requesting port 1 will return an error
> >>>>> or the input port.
> >>>>
> >>>> Hello Rob,
> >>>>
> >>>> I want to remind you that in v2 of this series you said that it'd be wrong for
> >>>> port@0 to be an output, I replied that you misread that as I had modeled it indeed
> >>>> as an input, and then you gave me your Reviewed-by tag.
> >>>
> >>> I have not misread it. Then I guess I forgot about it and missed it the
> >>> next time on v3.
> >>>
> >>
> >> Okay, that was some misunderstanding then - it's fine, no problem.
> >>
> >>>> Anyway - I get your concern about the previous behavior of `port`, but I chose to
> >>>> model this that way purely for consistency.
> >>>>
> >>>> First of all - the driver(s) will check if we're feeding a full graph, as it will
> >>>> indeed first check if port@1 is present: if it is, then it follows this scheme with
> >>>> port@0 as INPUT and port@1 as OUTPUT.
> >>>> If the component in port@0 is an OUTPUT, the bridge attach will fail.
> >>>>
> >>>> Getting to bindings themselves, then... it would be a mistake to model port@0 as an
> >>>> output and port@1 as an input, because that would be not only inconsistent with the
> >>>> DRM Bridge bindings, but would be highly confusing when reading the devicetree.
> >>>
> >>> Somewhat confusing, yes. Highly, no. Put a comment in the DT.
> >>>
> >>
> >> "Somewhat or highly" boils down to personal opinion, so I still go for "highly"
> >> but won't argue about that as wouldn't be productive and would bring us nowhere
> >> anyway, so, whatever :-)
> >>
> >> Putting a comment in DT is an option, yes, but that comment would need to be put
> >> on all of the MediaTek SoC device trees - current and future - and IMO would scream
> >> "inconsistency warning" (in different words, of course) all over, which honestly
> >> doesn't really look clean... at least to my eyes...
> >
> > What I find more confusing is my updated DT doesn't work with my
> > existing kernel...
> >
> >>>> Please note that the bridge bindings are always declaring port@0 as an INPUT and
> >>>> other ports as OUTPUT(s).
> >>>
> >>> There is no guarantee on that. Port numbering is local to the bridge and
> >>> opaque to anything outside that bridge. That is why you have to document
> >>> what each port represents.
> >>>
> >>
> >> I know and I agree that there's no guarantee on the numbering. I can see that in
> >> other non-drm-bridge bindings, and that's fine.
> >>
> >>> Would we have followed that convention if all the ports were defined
> >>> from the start? Certainly. But that didn't happen and you are stuck with
> >>> the existing binding and ABI.
> >>>
> >>
> >> I thought about adding a new compatible for the new port scheme, but that looked
> >> even worse to me as, after doing that (yeah, I actually went for it in the first
> >> version that I have never sent upstream) during my own proof-read I started
> >> screaming "HACK! HACK! NOOO!" all over, and rewritten the entire thing.
> >>
> >>>> As an example, you can check display/bridge/analogix,anx7625.yaml or
> >>>> display/bridge/samsung,mipi-dsim.yaml (and others) for bridges, otherwise
> >>>> display/st,stm32mp25-lvds.yaml or display/allwinner,sun4i-a10-display-frontend.yaml
> >>>> (and others) for display controllers, which do all conform to this logic, where
> >>>> the input is always @0, and the output is @1.
> >>>>
> >>>> Of course, doing this required me to do extra changes to the MTK DRM drivers to
> >>>> actually be retro-compatible with the old devicetrees as I explained before.
> >>>
> >>> You can't fix existing software...
> >>>
> >>
> >> That's true, but I don't see that as an "excuse" (grant me this term please) to
> >> "carelessly" keep it in a suboptimal state.
> >>
> >> This driver has been growing almost uncontrollably with (wrong, anyway!)
> >> board-specific component vectors - and writing a new one would just add up
> >> more code duplication to the mix and/or worsen the maintainability of older
> >> MediaTek SoCs (as the "old" driver will get forgotten and never updated anymore).
> >>
> >>> If you want to break the ABI, then that's ultimately up to you and
> >>> Mediatek maintainers to decide0. This case is easy to avoid, why would
> >>> you knowingly break the ABI here.
> >>
> >> Because if we don't do this, we condemn (forever) this driver, or part of it
> >> to have an inverted port scheme compared to either:
> >>    A. The other drm/mediatek components; or
> >>    B. The other bridge drivers (of which, some are used with MTK as well)
> >>
> >> ...and we also condemn (forever, again) all MediaTek device trees to scream
> >> "port inconsistency warning: A for drm/mediatek components, B for every other
> >> thing", which would scream "drm/mediatek is somewhat broken", which can be
> >> avoided.
> >
> > Sure. The cost is just an ABI break to do that.
> >
> >>> OTOH, this seems like a big enough
> >>> change I would imagine it is a matter of time before supporting a
> >>> missing OF graph for the components will be a problem.
> >>>
> >>
> >> Sorry I didn't understand this part, can you please-please-please reword?
> >
> > I think keeping the kernel working with the old and new binding will
> > be a challenge. Partly because testing with the old binding won't
> > happen, but also if the binding and drivers continue to evolve. So
> > while maintaining old ABI might be possible with this change, it will
> > continue to be an issue with each change.
>
> That's... exactly my point, so I am happy that we agree on that.
>
> > BTW, did you actually test
> > backwards compatibility with this? I can see you fallback to the old
> > binding, but there's a lot of other changes in there I can't really
> > tell by looking at it.
>
>
> Short answer: Yes, largely
>
> Long answer:
>
> Yes, with this series applied, I have tested both *old* and *new* devicetrees on
> 7 boards with 4 different SoCs and different display pipelines (hence, graphs):
> one smartphone (MT6795), a "bunch of" Chromebooks (MT8173, MT8186, MT8195), and
> a SBC (MT8195).
>
> Of course those had DSI or eDP displays, with or without DisplayPort external
> display support.
>
> >
> > What I haven't heard from you is "yes, we need to maintain the ABI" or
> > "no, we can break it". Decide that, then the path here is simple.
>
> No, we have to break it.

Okay, then my reviewed-by stands. But please make it clear in the
binding and dts commit msgs that it is an ABI break.

Rob

Re: [PATCH v12 1/3] dt-bindings: display: mediatek: Add OF graph support for board path

[AngeloGioacchino Del Regno]

Il 16/10/24 18:09, Rob Herring ha scritto:
> On Wed, Oct 16, 2024 at 10:26 AM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Il 16/10/24 16:00, Rob Herring ha scritto:
>>> On Wed, Oct 16, 2024 at 4:23 AM AngeloGioacchino Del Regno
>>> <angelogioacchino.delregno@collabora.com> wrote:
>>>>
>>>> Il 15/10/24 15:48, Rob Herring ha scritto:
>>>>> On Tue, Oct 15, 2024 at 10:32:22AM +0200, AngeloGioacchino Del Regno wrote:
>>>>>> Il 14/10/24 19:36, Rob Herring ha scritto:
>>>>>>> On Mon, Oct 14, 2024 at 3:51 AM AngeloGioacchino Del Regno
>>>>>>> <angelogioacchino.delregno@collabora.com> wrote:
>>>>>>>>
>>>>>>>> The display IPs in MediaTek SoCs support being interconnected with
>>>>>>>> different instances of DDP IPs (for example, merge0 or merge1) and/or
>>>>>>>> with different DDP IPs (for example, rdma can be connected with either
>>>>>>>> color, dpi, dsi, merge, etc), forming a full Display Data Path that
>>>>>>>> ends with an actual display.
>>>>>>>>
>>>>>>>> The final display pipeline is effectively board specific, as it does
>>>>>>>> depend on the display that is attached to it, and eventually on the
>>>>>>>> sensors supported by the board (for example, Adaptive Ambient Light
>>>>>>>> would need an Ambient Light Sensor, otherwise it's pointless!), other
>>>>>>>> than the output type.
>>>>>>>>
>>>>>>>> Add support for OF graphs to most of the MediaTek DDP (display) bindings
>>>>>>>> to add flexibility to build custom hardware paths, hence enabling board
>>>>>>>> specific configuration of the display pipeline and allowing to finally
>>>>>>>> migrate away from using hardcoded paths.
>>>>>>>>
>>>>>>>> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
>>>>>>>> Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
>>>>>>>> Tested-by: Alexandre Mergnat <amergnat@baylibre.com>
>>>>>>>> Reviewed-by: CK Hu <ck.hu@mediatek.com>
>>>>>>>> Tested-by: Michael Walle <mwalle@kernel.org> # on kontron-sbc-i1200
>>>>>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>>>>>>> ---
>>>>>>>>      .../display/mediatek/mediatek,aal.yaml        | 40 +++++++++++++++++++
>>>>>>>>      .../display/mediatek/mediatek,ccorr.yaml      | 21 ++++++++++
>>>>>>>>      .../display/mediatek/mediatek,color.yaml      | 22 ++++++++++
>>>>>>>>      .../display/mediatek/mediatek,dither.yaml     | 22 ++++++++++
>>>>>>>>      .../display/mediatek/mediatek,dpi.yaml        | 25 +++++++++++-
>>>>>>>>      .../display/mediatek/mediatek,dsc.yaml        | 24 +++++++++++
>>>>>>>>      .../display/mediatek/mediatek,dsi.yaml        | 27 ++++++++++++-
>>>>>>>>      .../display/mediatek/mediatek,ethdr.yaml      | 22 ++++++++++
>>>>>>>>      .../display/mediatek/mediatek,gamma.yaml      | 19 +++++++++
>>>>>>>>      .../display/mediatek/mediatek,merge.yaml      | 23 +++++++++++
>>>>>>>>      .../display/mediatek/mediatek,od.yaml         | 22 ++++++++++
>>>>>>>>      .../display/mediatek/mediatek,ovl-2l.yaml     | 22 ++++++++++
>>>>>>>>      .../display/mediatek/mediatek,ovl.yaml        | 22 ++++++++++
>>>>>>>>      .../display/mediatek/mediatek,postmask.yaml   | 21 ++++++++++
>>>>>>>>      .../display/mediatek/mediatek,rdma.yaml       | 22 ++++++++++
>>>>>>>>      .../display/mediatek/mediatek,ufoe.yaml       | 21 ++++++++++
>>>>>>>>      16 files changed, 372 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
>>>>>>>> index cf24434854ff..47ddba5c41af 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
>>>>>>>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
>>>>>>>> @@ -62,6 +62,27 @@ properties:
>>>>>>>>          $ref: /schemas/types.yaml#/definitions/phandle-array
>>>>>>>>          maxItems: 1
>>>>>>>>
>>>>>>>> +  ports:
>>>>>>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>>>>>>> +    description:
>>>>>>>> +      Input and output ports can have multiple endpoints, each of those
>>>>>>>> +      connects to either the primary, secondary, etc, display pipeline.
>>>>>>>> +
>>>>>>>> +    properties:
>>>>>>>> +      port@0:
>>>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>>>>> +        description: AAL input port
>>>>>>>> +
>>>>>>>> +      port@1:
>>>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>>>>> +        description:
>>>>>>>> +          AAL output to the next component's input, for example could be one
>>>>>>>> +          of many gamma, overdrive or other blocks.
>>>>>>>> +
>>>>>>>> +    required:
>>>>>>>> +      - port@0
>>>>>>>> +      - port@1
>>>>>>>> +
>>>>>>>>      required:
>>>>>>>>        - compatible
>>>>>>>>        - reg
>>>>>>>> @@ -89,5 +110,24 @@ examples:
>>>>>>>>                 power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
>>>>>>>>                 clocks = <&mmsys CLK_MM_DISP_AAL>;
>>>>>>>>                 mediatek,gce-client-reg = <&gce SUBSYS_1401XXXX 0x5000 0x1000>;
>>>>>>>> +
>>>>>>>> +           ports {
>>>>>>>> +               #address-cells = <1>;
>>>>>>>> +               #size-cells = <0>;
>>>>>>>> +
>>>>>>>> +               port@0 {
>>>>>>>> +                   reg = <0>;
>>>>>>>> +                   aal0_in: endpoint {
>>>>>>>> +                       remote-endpoint = <&ccorr0_out>;
>>>>>>>> +                   };
>>>>>>>> +               };
>>>>>>>> +
>>>>>>>> +               port@1 {
>>>>>>>> +                   reg = <1>;
>>>>>>>> +                   aal0_out: endpoint {
>>>>>>>> +                       remote-endpoint = <&gamma0_in>;
>>>>>>>> +                   };
>>>>>>>> +               };
>>>>>>>> +           };
>>>>>>>>             };
>>>>>>>>          };
>>>>>>>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
>>>>>>>> index 9f8366763831..fca8e7bb0cbc 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
>>>>>>>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
>>>>>>>> @@ -57,6 +57,27 @@ properties:
>>>>>>>>          $ref: /schemas/types.yaml#/definitions/phandle-array
>>>>>>>>          maxItems: 1
>>>>>>>>
>>>>>>>> +  ports:
>>>>>>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>>>>>>> +    description:
>>>>>>>> +      Input and output ports can have multiple endpoints, each of those
>>>>>>>> +      connects to either the primary, secondary, etc, display pipeline.
>>>>>>>> +
>>>>>>>> +    properties:
>>>>>>>> +      port@0:
>>>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>>>>> +        description: CCORR input port
>>>>>>>> +
>>>>>>>> +      port@1:
>>>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>>>>> +        description:
>>>>>>>> +          CCORR output to the input of the next desired component in the
>>>>>>>> +          display pipeline, usually only one of the available AAL blocks.
>>>>>>>> +
>>>>>>>> +    required:
>>>>>>>> +      - port@0
>>>>>>>> +      - port@1
>>>>>>>> +
>>>>>>>>      required:
>>>>>>>>        - compatible
>>>>>>>>        - reg
>>>>>>>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
>>>>>>>> index 7df786bbad20..6160439ce4d7 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
>>>>>>>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
>>>>>>>> @@ -65,6 +65,28 @@ properties:
>>>>>>>>          $ref: /schemas/types.yaml#/definitions/phandle-array
>>>>>>>>          maxItems: 1
>>>>>>>>
>>>>>>>> +  ports:
>>>>>>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>>>>>>> +    description:
>>>>>>>> +      Input and output ports can have multiple endpoints, each of those
>>>>>>>> +      connects to either the primary, secondary, etc, display pipeline.
>>>>>>>> +
>>>>>>>> +    properties:
>>>>>>>> +      port@0:
>>>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>>>>> +        description: COLOR input port
>>>>>>>> +
>>>>>>>> +      port@1:
>>>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>>>>> +        description:
>>>>>>>> +          COLOR output to the input of the next desired component in the
>>>>>>>> +          display pipeline, for example one of the available CCORR or AAL
>>>>>>>> +          blocks.
>>>>>>>> +
>>>>>>>> +    required:
>>>>>>>> +      - port@0
>>>>>>>> +      - port@1
>>>>>>>> +
>>>>>>>>      required:
>>>>>>>>        - compatible
>>>>>>>>        - reg
>>>>>>>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
>>>>>>>> index 6fceb1f95d2a..abaf27916d13 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
>>>>>>>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
>>>>>>>> @@ -56,6 +56,28 @@ properties:
>>>>>>>>          $ref: /schemas/types.yaml#/definitions/phandle-array
>>>>>>>>          maxItems: 1
>>>>>>>>
>>>>>>>> +  ports:
>>>>>>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>>>>>>> +    description:
>>>>>>>> +      Input and output ports can have multiple endpoints, each of those
>>>>>>>> +      connects to either the primary, secondary, etc, display pipeline.
>>>>>>>> +
>>>>>>>> +    properties:
>>>>>>>> +      port@0:
>>>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>>>>> +        description: DITHER input, usually from a POSTMASK or GAMMA block.
>>>>>>>> +
>>>>>>>> +      port@1:
>>>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>>>>> +        description:
>>>>>>>> +          DITHER output to the input of the next desired component in the
>>>>>>>> +          display pipeline, for example one of the available DSC compressors,
>>>>>>>> +          DP_INTF, DSI, LVDS or others.
>>>>>>>> +
>>>>>>>> +    required:
>>>>>>>> +      - port@0
>>>>>>>> +      - port@1
>>>>>>>> +
>>>>>>>>      required:
>>>>>>>>        - compatible
>>>>>>>>        - reg
>>>>>>>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
>>>>>>>> index 3a82aec9021c..b567e3d58aa1 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
>>>>>>>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
>>>>>>>> @@ -71,13 +71,34 @@ properties:
>>>>>>>>            Output port node. This port should be connected to the input port of an
>>>>>>>>            attached HDMI, LVDS or DisplayPort encoder chip.
>>>>>>>>
>>>>>>>> +  ports:
>>>>>>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>>>>>>> +
>>>>>>>> +    properties:
>>>>>>>> +      port@0:
>>>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>>>>> +        description: DPI input port
>>>>>>>> +
>>>>>>>> +      port@1:
>>>>>>>> +        $ref: /schemas/graph.yaml#/properties/port
>>>>>>>> +        description: DPI output to an HDMI, LVDS or DisplayPort encoder input
>>>>>>>
>>>>>>> This is wrong. The existing 'port' is the output. 'port' and 'port@0'
>>>>>>> are treated as the same thing. Since you are adding an input port, the
>>>>>>> new port has to be 'port@1' (or any number but 0).
>>>>>>>
>>>>>>> I haven't looked at the driver code, but it should request port 0 and
>>>>>>> always get the output port. And requesting port 1 will return an error
>>>>>>> or the input port.
>>>>>>
>>>>>> Hello Rob,
>>>>>>
>>>>>> I want to remind you that in v2 of this series you said that it'd be wrong for
>>>>>> port@0 to be an output, I replied that you misread that as I had modeled it indeed
>>>>>> as an input, and then you gave me your Reviewed-by tag.
>>>>>
>>>>> I have not misread it. Then I guess I forgot about it and missed it the
>>>>> next time on v3.
>>>>>
>>>>
>>>> Okay, that was some misunderstanding then - it's fine, no problem.
>>>>
>>>>>> Anyway - I get your concern about the previous behavior of `port`, but I chose to
>>>>>> model this that way purely for consistency.
>>>>>>
>>>>>> First of all - the driver(s) will check if we're feeding a full graph, as it will
>>>>>> indeed first check if port@1 is present: if it is, then it follows this scheme with
>>>>>> port@0 as INPUT and port@1 as OUTPUT.
>>>>>> If the component in port@0 is an OUTPUT, the bridge attach will fail.
>>>>>>
>>>>>> Getting to bindings themselves, then... it would be a mistake to model port@0 as an
>>>>>> output and port@1 as an input, because that would be not only inconsistent with the
>>>>>> DRM Bridge bindings, but would be highly confusing when reading the devicetree.
>>>>>
>>>>> Somewhat confusing, yes. Highly, no. Put a comment in the DT.
>>>>>
>>>>
>>>> "Somewhat or highly" boils down to personal opinion, so I still go for "highly"
>>>> but won't argue about that as wouldn't be productive and would bring us nowhere
>>>> anyway, so, whatever :-)
>>>>
>>>> Putting a comment in DT is an option, yes, but that comment would need to be put
>>>> on all of the MediaTek SoC device trees - current and future - and IMO would scream
>>>> "inconsistency warning" (in different words, of course) all over, which honestly
>>>> doesn't really look clean... at least to my eyes...
>>>
>>> What I find more confusing is my updated DT doesn't work with my
>>> existing kernel...
>>>
>>>>>> Please note that the bridge bindings are always declaring port@0 as an INPUT and
>>>>>> other ports as OUTPUT(s).
>>>>>
>>>>> There is no guarantee on that. Port numbering is local to the bridge and
>>>>> opaque to anything outside that bridge. That is why you have to document
>>>>> what each port represents.
>>>>>
>>>>
>>>> I know and I agree that there's no guarantee on the numbering. I can see that in
>>>> other non-drm-bridge bindings, and that's fine.
>>>>
>>>>> Would we have followed that convention if all the ports were defined
>>>>> from the start? Certainly. But that didn't happen and you are stuck with
>>>>> the existing binding and ABI.
>>>>>
>>>>
>>>> I thought about adding a new compatible for the new port scheme, but that looked
>>>> even worse to me as, after doing that (yeah, I actually went for it in the first
>>>> version that I have never sent upstream) during my own proof-read I started
>>>> screaming "HACK! HACK! NOOO!" all over, and rewritten the entire thing.
>>>>
>>>>>> As an example, you can check display/bridge/analogix,anx7625.yaml or
>>>>>> display/bridge/samsung,mipi-dsim.yaml (and others) for bridges, otherwise
>>>>>> display/st,stm32mp25-lvds.yaml or display/allwinner,sun4i-a10-display-frontend.yaml
>>>>>> (and others) for display controllers, which do all conform to this logic, where
>>>>>> the input is always @0, and the output is @1.
>>>>>>
>>>>>> Of course, doing this required me to do extra changes to the MTK DRM drivers to
>>>>>> actually be retro-compatible with the old devicetrees as I explained before.
>>>>>
>>>>> You can't fix existing software...
>>>>>
>>>>
>>>> That's true, but I don't see that as an "excuse" (grant me this term please) to
>>>> "carelessly" keep it in a suboptimal state.
>>>>
>>>> This driver has been growing almost uncontrollably with (wrong, anyway!)
>>>> board-specific component vectors - and writing a new one would just add up
>>>> more code duplication to the mix and/or worsen the maintainability of older
>>>> MediaTek SoCs (as the "old" driver will get forgotten and never updated anymore).
>>>>
>>>>> If you want to break the ABI, then that's ultimately up to you and
>>>>> Mediatek maintainers to decide0. This case is easy to avoid, why would
>>>>> you knowingly break the ABI here.
>>>>
>>>> Because if we don't do this, we condemn (forever) this driver, or part of it
>>>> to have an inverted port scheme compared to either:
>>>>     A. The other drm/mediatek components; or
>>>>     B. The other bridge drivers (of which, some are used with MTK as well)
>>>>
>>>> ...and we also condemn (forever, again) all MediaTek device trees to scream
>>>> "port inconsistency warning: A for drm/mediatek components, B for every other
>>>> thing", which would scream "drm/mediatek is somewhat broken", which can be
>>>> avoided.
>>>
>>> Sure. The cost is just an ABI break to do that.
>>>
>>>>> OTOH, this seems like a big enough
>>>>> change I would imagine it is a matter of time before supporting a
>>>>> missing OF graph for the components will be a problem.
>>>>>
>>>>
>>>> Sorry I didn't understand this part, can you please-please-please reword?
>>>
>>> I think keeping the kernel working with the old and new binding will
>>> be a challenge. Partly because testing with the old binding won't
>>> happen, but also if the binding and drivers continue to evolve. So
>>> while maintaining old ABI might be possible with this change, it will
>>> continue to be an issue with each change.
>>
>> That's... exactly my point, so I am happy that we agree on that.
>>
>>> BTW, did you actually test
>>> backwards compatibility with this? I can see you fallback to the old
>>> binding, but there's a lot of other changes in there I can't really
>>> tell by looking at it.
>>
>>
>> Short answer: Yes, largely
>>
>> Long answer:
>>
>> Yes, with this series applied, I have tested both *old* and *new* devicetrees on
>> 7 boards with 4 different SoCs and different display pipelines (hence, graphs):
>> one smartphone (MT6795), a "bunch of" Chromebooks (MT8173, MT8186, MT8195), and
>> a SBC (MT8195).
>>
>> Of course those had DSI or eDP displays, with or without DisplayPort external
>> display support.
>>
>>>
>>> What I haven't heard from you is "yes, we need to maintain the ABI" or
>>> "no, we can break it". Decide that, then the path here is simple.
>>
>> No, we have to break it.
> 
> Okay, then my reviewed-by stands. But please make it clear in the
> binding and dts commit msgs that it is an ABI break.
> 
Thanks for that.

I'll send a v13 that adds a snippet of text in this commit's description stating
that it breaks the ABI, and I will keep your R-b tag.

Cheers,
Angelo