[PATCH 0/5] drm/bridge: simple-bridge: Add DPI color encoder support

[PATCH 0/5] drm/bridge: simple-bridge: Add DPI color encoder support

[Liu Ying]

Hi,

This patch series aims to add DPI color encoder support as a simple DRM
bridge.  A DPI color encoder simply converts input DPI color coding to
output DPI color coding, like Adafruit Kippah DPI hat[1] which converts
input 18-bit pixel data to 24-bit pixel data(with 2 low padding bits in
every color component though).  A real use case is that NXP i.MX93 11x11
EVK[2] and i.MX93 9x9 QSB[3] boards may connect a 24-bit DPI panel through
the Adafruit Kippah DPI hat.  The display pipeline is

i.MX93 LCDIF display controller(RGB888) ->
i.MX93 parallel display format configuration(RGB666) ->
on-board Raspiberry Pi compatible interface(RPi)(RGB666) ->
Adafruit Kippah DPI hat(RGB888 with 2 low padding bits in color components) ->
24-bit "ontat,kd50g21-40nt-a1" DPI panel

[1] https://learn.adafruit.com/adafruit-dpi-display-kippah-ttl-tft/downloads
[2] https://www.nxp.com/design/design-center/development-boards-and-designs/i.MX93EVK
[3] https://www.nxp.com/design/design-center/development-boards-and-designs/IMX93QSB

Liu Ying (5):
  dt-bindings: display: Document DPI color codings
  drm/of: Add drm_of_dpi_get_color_coding()
  dt-bindings: display: simple-bridge: Document DPI color encoder
  drm/bridge: simple-bridge: Add DPI color encoder support
  drm/bridge: simple-bridge: Add next panel support

 .../display/bridge/simple-bridge.yaml         |  89 +++++++++++-
 .../bindings/display/dpi-color-coding.yaml    |  90 ++++++++++++
 drivers/gpu/drm/bridge/Kconfig                |   1 +
 drivers/gpu/drm/bridge/simple-bridge.c        | 132 ++++++++++++++++--
 drivers/gpu/drm/drm_of.c                      |  43 ++++++
 include/drm/drm_of.h                          |   7 +
 6 files changed, 348 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/dpi-color-coding.yaml

-- 
2.34.1

[PATCH 1/5] dt-bindings: display: Document DPI color codings

[Liu Ying]

Document DPI color codings according to MIPI Alliance Standard for
Display Pixel Interface(DPI-2) Version 2.00(15 September 2005).

Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
 .../bindings/display/dpi-color-coding.yaml    | 90 +++++++++++++++++++
 1 file changed, 90 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/dpi-color-coding.yaml

diff --git a/Documentation/devicetree/bindings/display/dpi-color-coding.yaml b/Documentation/devicetree/bindings/display/dpi-color-coding.yaml
new file mode 100644
index 000000000000..6430d6f1ddd1
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/dpi-color-coding.yaml
@@ -0,0 +1,90 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/dpi-color-coding.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MIPI DPI Interface Color Coding
+
+maintainers:
+  - Liu Ying <victor.liu@nxp.com>
+
+description:
+  MIPI Alliance Standard for Display Pixel Interface(DPI-2) Version 2.00(15
+  September 2005) specifies color codings at the DPI interface.
+
+properties:
+  dpi-color-coding:
+    enum:
+      - 16bit-configuration1
+      - 16bit-configuration2
+      - 16bit-configuration3
+      - 18bit-configuration1
+      - 18bit-configuration2
+      - 24bit
+    description: |
+      This table specifies the mapping of data bits, as components of primary
+      pixel color values red(R), green(G) and blue(B), to signal lines at the
+      interface.
+
+      +--------+--------------------------+-----------------+--------+
+      | Signal |          16-bit          |      18-bit     |        |
+      |        +--------+--------+--------+--------+--------+ 24-bit |
+      | Line   |  cfg1  |  cfg2  |  cfg3  |  cfg1  |  cfg2  |        |
+      +========+========+========+========+========+========+========+
+      |   D23  | unused | unused | unused | unused | unused |   R7   |
+      +--------+--------+--------+--------+--------+--------+--------+
+      |   D22  | unused | unused | unused | unused | unused |   R6   |
+      +--------+--------+--------+--------+--------+--------+--------+
+      |   D21  | unused | unused |   R4   | unused |   R5   |   R5   |
+      +--------+--------+--------+--------+--------+--------+--------+
+      |   D20  | unused |   R4   |   R3   | unused |   R4   |   R4   |
+      +--------+--------+--------+--------+--------+--------+--------+
+      |   D19  | unused |   R3   |   R2   | unused |   R3   |   R3   |
+      +--------+--------+--------+--------+--------+--------+--------+
+      |   D18  | unused |   R2   |   R1   | unused |   R2   |   R2   |
+      +--------+--------+--------+--------+--------+--------+--------+
+      |   D17  | unused |   R1   |   R0   |   R5   |   R1   |   R1   |
+      +--------+--------+--------+--------+--------+--------+--------+
+      |   D16  | unused |   R0   | unused |   R4   |   R0   |   R0   |
+      +--------+--------+--------+--------+--------+--------+--------+
+      |   D15  |   R4   | unused | unused |   R3   | unused |   G7   |
+      +--------+--------+--------+--------+--------+--------+--------+
+      |   D14  |   R3   | unused | unused |   R2   | unused |   G6   |
+      +--------+--------+--------+--------+--------+--------+--------+
+      |   D13  |   R2   |   G5   |   G5   |   R1   |   G5   |   G5   |
+      +--------+--------+--------+--------+--------+--------+--------+
+      |   D12  |   R1   |   G4   |   G4   |   R0   |   G4   |   G4   |
+      +--------+--------+--------+--------+--------+--------+--------+
+      |   D11  |   R0   |   G3   |   G3   |   G5   |   G3   |   G3   |
+      +--------+--------+--------+--------+--------+--------+--------+
+      |   D10  |   G5   |   G2   |   G2   |   G4   |   G2   |   G2   |
+      +--------+--------+--------+--------+--------+--------+--------+
+      |   D9   |   G4   |   G1   |   G1   |   G3   |   G1   |   G1   |
+      +--------+--------+--------+--------+--------+--------+--------+
+      |   D8   |   G3   |   G0   |   G0   |   G2   |   G0   |   G0   |
+      +--------+--------+--------+--------+--------+--------+--------+
+      |   D7   |   G2   | unused | unused |   G1   | unused |   B7   |
+      +--------+--------+--------+--------+--------+--------+--------+
+      |   D6   |   G1   | unused | unused |   G0   | unused |   B6   |
+      +--------+--------+--------+--------+--------+--------+--------+
+      |   D5   |   G0   | unused |   B4   |   B5   |   B5   |   B5   |
+      +--------+--------+--------+--------+--------+--------+--------+
+      |   D4   |   B4   |   B4   |   B3   |   B4   |   B4   |   B4   |
+      +--------+--------+--------+--------+--------+--------+--------+
+      |   D3   |   B3   |   B3   |   B2   |   B3   |   B3   |   B3   |
+      +--------+--------+--------+--------+--------+--------+--------+
+      |   D2   |   B2   |   B2   |   B1   |   B2   |   B2   |   B2   |
+      +--------+--------+--------+--------+--------+--------+--------+
+      |   D1   |   B1   |   B1   |   B0   |   B1   |   B1   |   B1   |
+      +--------+--------+--------+--------+--------+--------+--------+
+      |   D0   |   B0   |   B0   | unused |   B0   |   B0   |   B0   |
+      +--------+--------+--------+--------+--------+--------+--------+
+
+      There are three mappings for 16-bit pixels to data signals, two mappings
+      for 18-bit pixels to data signals, and one mapping for 24-bit pixels to
+      data signals.
+
+additionalProperties: true
+
+...
-- 
2.34.1

[PATCH 2/5] drm/of: Add drm_of_dpi_get_color_coding()

[Liu Ying]

Add helper function drm_of_dpi_get_color_coding() to get media bus format
value from MIPI DPI color coding string.

Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
 drivers/gpu/drm/drm_of.c | 43 ++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_of.h     |  7 +++++++
 2 files changed, 50 insertions(+)

diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index d0183dea7703..6e2e19275b99 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -285,6 +285,49 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
 }
 EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
 
+/**
+ * drm_of_dpi_get_color_coding - Get DPI color coding
+ * @endpoint: DT endpoint node of the DPI source or sink
+ *
+ * Convert DT "dpi-color-coding" property string value into media bus format
+ * value.
+ *
+ * Return:
+ * * MEDIA_BUS_FMT_RGB565_1X16 - dpi-color-coding is "16bit-configuration1"
+ * * MEDIA_BUS_FMT_RGB565_1X24_CPADHI - dpi-color-coding is
+ *                                      "16bit-configuration2"
+ * * MEDIA_BUS_FMT_RGB666_1X18 - dpi-color-coding is "18bit-configuration1"
+ * * MEDIA_BUS_FMT_BGR666_1X24_CPADHI - dpi-color-coding is
+ *                                      "18bit-configuration2"
+ * * MEDIA_BUS_FMT_RGB888_1X24 - dpi-color-coding is "24bit"
+ * * -EINVAL - the "dpi-color-coding" property is unsupported
+ * * -ENODEV - the "dpi-color-coding" property is missing
+ */
+int drm_of_dpi_get_color_coding(const struct device_node *endpoint)
+{
+	const char *coding;
+	int ret;
+
+	ret = of_property_read_string(endpoint, "dpi-color-coding", &coding);
+	if (ret < 0)
+		return -ENODEV;
+
+	/* TODO: Add 16bit-configuration3 support. */
+	if (!strcmp(coding, "16bit-configuration1"))
+		return MEDIA_BUS_FMT_RGB565_1X16;
+	if (!strcmp(coding, "16bit-configuration2"))
+		return MEDIA_BUS_FMT_RGB565_1X24_CPADHI;
+	if (!strcmp(coding, "18bit-configuration1"))
+		return MEDIA_BUS_FMT_RGB666_1X18;
+	if (!strcmp(coding, "18bit-configuration2"))
+		return MEDIA_BUS_FMT_BGR666_1X24_CPADHI;
+	if (!strcmp(coding, "24bit"))
+		return MEDIA_BUS_FMT_RGB888_1X24;
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(drm_of_dpi_get_color_coding);
+
 enum drm_of_lvds_pixels {
 	DRM_OF_LVDS_EVEN = BIT(0),
 	DRM_OF_LVDS_ODD = BIT(1),
diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
index 7f0256dae3f1..0a827f3b3a55 100644
--- a/include/drm/drm_of.h
+++ b/include/drm/drm_of.h
@@ -50,6 +50,7 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
 				int port, int endpoint,
 				struct drm_panel **panel,
 				struct drm_bridge **bridge);
+int drm_of_dpi_get_color_coding(const struct device_node *endpoint);
 int drm_of_lvds_get_dual_link_pixel_order(const struct device_node *port1,
 					  const struct device_node *port2);
 int drm_of_lvds_get_dual_link_pixel_order_sink(struct device_node *port1,
@@ -104,6 +105,12 @@ static inline int drm_of_find_panel_or_bridge(const struct device_node *np,
 	return -EINVAL;
 }
 
+static inline int
+drm_of_dpi_get_color_coding(const struct device_node *endpoint)
+{
+	return -EINVAL;
+}
+
 static inline int
 drm_of_lvds_get_dual_link_pixel_order(const struct device_node *port1,
 				      const struct device_node *port2)
-- 
2.34.1

[PATCH 3/5] dt-bindings: display: simple-bridge: Document DPI color encoder

[Liu Ying]

A DPI color encoder, as a simple display bridge, converts input DPI color
coding to output DPI color coding, like Adafruit Kippah DPI hat[1] which
converts input 18-bit pixel data to 24-bit pixel data(with 2 low padding
bits in every color component though). Document the DPI color encoder.

[1] https://learn.adafruit.com/adafruit-dpi-display-kippah-ttl-tft/downloads

Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
 .../display/bridge/simple-bridge.yaml         | 89 ++++++++++++++++++-
 1 file changed, 87 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml b/Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml
index 43cf4df9811a..c1747c033040 100644
--- a/Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml
@@ -27,6 +27,7 @@ properties:
           - const: adi,adv7123
       - enum:
           - adi,adv7123
+          - dpi-color-encoder
           - dumb-vga-dac
           - ti,opa362
           - ti,ths8134
@@ -37,13 +38,31 @@ properties:
 
     properties:
       port@0:
-        $ref: /schemas/graph.yaml#/properties/port
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
         description: The bridge input
 
+        properties:
+          endpoint:
+            $ref: /schemas/media/video-interfaces.yaml#
+            unevaluatedProperties: false
+
+            properties:
+              dpi-color-coding: true
+
       port@1:
-        $ref: /schemas/graph.yaml#/properties/port
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
         description: The bridge output
 
+        properties:
+          endpoint:
+            $ref: /schemas/media/video-interfaces.yaml#
+            unevaluatedProperties: false
+
+            properties:
+              dpi-color-coding: true
+
     required:
       - port@0
       - port@1
@@ -61,6 +80,44 @@ required:
 
 additionalProperties: false
 
+allOf:
+  - $ref: /schemas/display/dpi-color-coding.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: dpi-color-encoder
+    then:
+      properties:
+        ports:
+          properties:
+            port@0:
+              properties:
+                endpoint:
+                  required:
+                    - dpi-color-coding
+
+            port@1:
+              properties:
+                endpoint:
+                  required:
+                    - dpi-color-coding
+    else:
+      properties:
+        ports:
+          properties:
+            port@0:
+              properties:
+                endpoint:
+                  properties:
+                    dpi-color-coding: false
+
+            port@1:
+              properties:
+                endpoint:
+                  properties:
+                    dpi-color-coding: false
+
 examples:
   - |
     bridge {
@@ -88,4 +145,32 @@ examples:
         };
     };
 
+  - |
+    bridge {
+        compatible = "dpi-color-enoder";
+
+        ports {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            port@0 {
+                reg = <0>;
+
+                dpi_in: endpoint {
+                    remote-endpoint = <&dc_out>;
+                    dpi-color-coding = "18bit-configuration1";
+                };
+            };
+
+            port@1 {
+                reg = <1>;
+
+                dpi_out: endpoint {
+                    remote-endpoint = <&panel_in>;
+                    dpi-color-coding = "24bit";
+                };
+            };
+        };
+    };
+
 ...
-- 
2.34.1

[PATCH 4/5] drm/bridge: simple-bridge: Add DPI color encoder support

[Liu Ying]

A DPI color encoder, as a simple display bridge, converts input DPI color
coding to output DPI color coding, like Adafruit Kippah DPI hat[1] which
converts input 18-bit pixel data to 24-bit pixel data(with 2 low padding
bits in every color component though). Add the DPI color encoder support
in the simple bridge driver.

[1] https://learn.adafruit.com/adafruit-dpi-display-kippah-ttl-tft/downloads

Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
 drivers/gpu/drm/bridge/simple-bridge.c | 104 ++++++++++++++++++++++++-
 1 file changed, 102 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c
index ab0b0e36e97a..c0445bd20e07 100644
--- a/drivers/gpu/drm/bridge/simple-bridge.c
+++ b/drivers/gpu/drm/bridge/simple-bridge.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/gpio/consumer.h>
+#include <linux/media-bus-format.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_graph.h>
@@ -17,12 +18,14 @@
 #include <drm/drm_bridge.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_edid.h>
+#include <drm/drm_of.h>
 #include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
 
 struct simple_bridge_info {
 	const struct drm_bridge_timings *timings;
 	unsigned int connector_type;
+	const struct drm_bridge_funcs *bridge_funcs;
 };
 
 struct simple_bridge {
@@ -34,6 +37,9 @@ struct simple_bridge {
 	struct drm_bridge	*next_bridge;
 	struct regulator	*vdd;
 	struct gpio_desc	*enable;
+
+	int			dpi_color_coding_input;
+	int			dpi_color_coding_output;
 };
 
 static inline struct simple_bridge *
@@ -156,16 +162,93 @@ static void simple_bridge_disable(struct drm_bridge *bridge)
 		regulator_disable(sbridge->vdd);
 }
 
-static const struct drm_bridge_funcs simple_bridge_bridge_funcs = {
+static const struct drm_bridge_funcs default_simple_bridge_bridge_funcs = {
 	.attach		= simple_bridge_attach,
 	.enable		= simple_bridge_enable,
 	.disable	= simple_bridge_disable,
 };
 
+static u32 *
+dpi_color_encoder_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
+					    struct drm_bridge_state *bridge_state,
+					    struct drm_crtc_state *crtc_state,
+					    struct drm_connector_state *conn_state,
+					    u32 output_fmt,
+					    unsigned int *num_input_fmts)
+{
+	struct simple_bridge *sbridge = drm_bridge_to_simple_bridge(bridge);
+	u32 *input_fmts;
+
+	*num_input_fmts = 0;
+
+	if (sbridge->dpi_color_coding_output != output_fmt)
+		return NULL;
+
+	input_fmts = kzalloc(sizeof(*input_fmts), GFP_KERNEL);
+	if (!input_fmts)
+		return NULL;
+
+	*num_input_fmts = 1;
+	input_fmts[0] = sbridge->dpi_color_coding_input;
+	return input_fmts;
+}
+
+static const struct drm_bridge_funcs dpi_color_encoder_bridge_funcs = {
+	.attach				= simple_bridge_attach,
+	.enable				= simple_bridge_enable,
+	.disable			= simple_bridge_disable,
+	.atomic_reset			= drm_atomic_helper_bridge_reset,
+	.atomic_duplicate_state		= drm_atomic_helper_bridge_duplicate_state,
+	.atomic_destroy_state		= drm_atomic_helper_bridge_destroy_state,
+	.atomic_get_input_bus_fmts	= dpi_color_encoder_atomic_get_input_bus_fmts,
+};
+
+static int simple_bridge_get_dpi_color_coding(struct simple_bridge *sbridge,
+					      struct device *dev)
+{
+	struct device_node *ep0, *ep1 = NULL;
+	int ret = 0;
+
+	ep0 = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
+	if (!ep0) {
+		dev_err(dev, "failed to get port@0 endpoint\n");
+		ret = -ENODEV;
+		goto out;
+	}
+
+	ep1 = of_graph_get_endpoint_by_regs(dev->of_node, 1, 0);
+	if (!ep1) {
+		dev_err(dev, "failed to get port@1 endpoint\n");
+		ret = -ENODEV;
+		goto out;
+	}
+
+	sbridge->dpi_color_coding_input = drm_of_dpi_get_color_coding(ep0);
+	if (sbridge->dpi_color_coding_input < 0) {
+		dev_err(dev, "failed to get DPI input media bus format\n");
+		ret = sbridge->dpi_color_coding_input;
+		goto out;
+	}
+
+	sbridge->dpi_color_coding_output = drm_of_dpi_get_color_coding(ep1);
+	if (sbridge->dpi_color_coding_output < 0) {
+		dev_err(dev, "failed to get DPI output media bus format\n");
+		ret = sbridge->dpi_color_coding_output;
+		goto out;
+	}
+
+out:
+	of_node_put(ep1);
+	of_node_put(ep0);
+
+	return ret;
+}
+
 static int simple_bridge_probe(struct platform_device *pdev)
 {
 	struct simple_bridge *sbridge;
 	struct device_node *remote;
+	int ret;
 
 	sbridge = devm_kzalloc(&pdev->dev, sizeof(*sbridge), GFP_KERNEL);
 	if (!sbridge)
@@ -202,8 +285,14 @@ static int simple_bridge_probe(struct platform_device *pdev)
 		return dev_err_probe(&pdev->dev, PTR_ERR(sbridge->enable),
 				     "Unable to retrieve enable GPIO\n");
 
+	if (of_device_is_compatible(pdev->dev.of_node, "dpi-color-encoder")) {
+		ret = simple_bridge_get_dpi_color_coding(sbridge, &pdev->dev);
+		if (ret)
+			return ret;
+	}
+
 	/* Register the bridge. */
-	sbridge->bridge.funcs = &simple_bridge_bridge_funcs;
+	sbridge->bridge.funcs = sbridge->info->bridge_funcs;
 	sbridge->bridge.of_node = pdev->dev.of_node;
 	sbridge->bridge.timings = sbridge->info->timings;
 
@@ -253,29 +342,40 @@ static const struct of_device_id simple_bridge_match[] = {
 		.compatible = "dumb-vga-dac",
 		.data = &(const struct simple_bridge_info) {
 			.connector_type = DRM_MODE_CONNECTOR_VGA,
+			.bridge_funcs = &default_simple_bridge_bridge_funcs,
 		},
 	}, {
 		.compatible = "adi,adv7123",
 		.data = &(const struct simple_bridge_info) {
 			.timings = &default_bridge_timings,
 			.connector_type = DRM_MODE_CONNECTOR_VGA,
+			.bridge_funcs = &default_simple_bridge_bridge_funcs,
+		},
+	}, {
+		.compatible = "dpi-color-encoder",
+		.data = &(const struct simple_bridge_info) {
+			.connector_type = DRM_MODE_CONNECTOR_DPI,
+			.bridge_funcs = &dpi_color_encoder_bridge_funcs,
 		},
 	}, {
 		.compatible = "ti,opa362",
 		.data = &(const struct simple_bridge_info) {
 			.connector_type = DRM_MODE_CONNECTOR_Composite,
+			.bridge_funcs = &default_simple_bridge_bridge_funcs,
 		},
 	}, {
 		.compatible = "ti,ths8135",
 		.data = &(const struct simple_bridge_info) {
 			.timings = &ti_ths8135_bridge_timings,
 			.connector_type = DRM_MODE_CONNECTOR_VGA,
+			.bridge_funcs = &default_simple_bridge_bridge_funcs,
 		},
 	}, {
 		.compatible = "ti,ths8134",
 		.data = &(const struct simple_bridge_info) {
 			.timings = &ti_ths8134_bridge_timings,
 			.connector_type = DRM_MODE_CONNECTOR_VGA,
+			.bridge_funcs = &default_simple_bridge_bridge_funcs,
 		},
 	},
 	{},
-- 
2.34.1

[PATCH 5/5] drm/bridge: simple-bridge: Add next panel support

[Liu Ying]

The next bridge connected to a simple bridge could be a panel, e.g.,
a DPI panel connected to a DPI color encoder. Add the next panel support,
instead of supporting non-panel next bridge only.

Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
 drivers/gpu/drm/bridge/Kconfig         |  1 +
 drivers/gpu/drm/bridge/simple-bridge.c | 32 ++++++++++++++++----------
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index d20f1646dac2..92187dbdd32b 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -310,6 +310,7 @@ config DRM_SIMPLE_BRIDGE
 	tristate "Simple DRM bridge support"
 	depends on OF
 	select DRM_KMS_HELPER
+	select DRM_PANEL_BRIDGE
 	help
 	  Support for non-programmable DRM bridges, such as ADI ADV7123, TI
 	  THS8134 and THS8135 or passive resistor ladder DACs.
diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c
index c0445bd20e07..4c585e5583ca 100644
--- a/drivers/gpu/drm/bridge/simple-bridge.c
+++ b/drivers/gpu/drm/bridge/simple-bridge.c
@@ -19,6 +19,7 @@
 #include <drm/drm_crtc.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_of.h>
+#include <drm/drm_panel.h>
 #include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
 
@@ -35,6 +36,7 @@ struct simple_bridge {
 	const struct simple_bridge_info *info;
 
 	struct drm_bridge	*next_bridge;
+	struct drm_panel	*next_panel;
 	struct regulator	*vdd;
 	struct gpio_desc	*enable;
 
@@ -114,6 +116,10 @@ static int simple_bridge_attach(struct drm_bridge *bridge,
 	struct simple_bridge *sbridge = drm_bridge_to_simple_bridge(bridge);
 	int ret;
 
+	if (sbridge->next_panel)
+		return drm_bridge_attach(bridge->encoder, sbridge->next_bridge,
+					 bridge, flags);
+
 	ret = drm_bridge_attach(bridge->encoder, sbridge->next_bridge, bridge,
 				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
 	if (ret < 0)
@@ -247,7 +253,6 @@ static int simple_bridge_get_dpi_color_coding(struct simple_bridge *sbridge,
 static int simple_bridge_probe(struct platform_device *pdev)
 {
 	struct simple_bridge *sbridge;
-	struct device_node *remote;
 	int ret;
 
 	sbridge = devm_kzalloc(&pdev->dev, sizeof(*sbridge), GFP_KERNEL);
@@ -257,17 +262,20 @@ static int simple_bridge_probe(struct platform_device *pdev)
 	sbridge->info = of_device_get_match_data(&pdev->dev);
 
 	/* Get the next bridge in the pipeline. */
-	remote = of_graph_get_remote_node(pdev->dev.of_node, 1, -1);
-	if (!remote)
-		return -EINVAL;
-
-	sbridge->next_bridge = of_drm_find_bridge(remote);
-	of_node_put(remote);
-
-	if (!sbridge->next_bridge) {
-		dev_dbg(&pdev->dev, "Next bridge not found, deferring probe\n");
-		return -EPROBE_DEFER;
-	}
+	ret = drm_of_find_panel_or_bridge(pdev->dev.of_node, 1, -1,
+					  &sbridge->next_panel,
+					  &sbridge->next_bridge);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "Next panel or bridge not found\n");
+
+	if (sbridge->next_panel)
+		sbridge->next_bridge = devm_drm_panel_bridge_add(&pdev->dev,
+								 sbridge->next_panel);
+
+	if (IS_ERR(sbridge->next_bridge))
+		return dev_err_probe(&pdev->dev, PTR_ERR(sbridge->next_bridge),
+				     "Next bridge not found\n");
 
 	/* Get the regulator and GPIO resources. */
 	sbridge->vdd = devm_regulator_get_optional(&pdev->dev, "vdd");
-- 
2.34.1

Re: [PATCH 1/5] dt-bindings: display: Document DPI color codings

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1607 bytes --]

On Tue, Mar 04, 2025 at 06:15:26PM +0800, Liu Ying wrote:
> Document DPI color codings according to MIPI Alliance Standard for
> Display Pixel Interface(DPI-2) Version 2.00(15 September 2005).
> 
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> ---
>  .../bindings/display/dpi-color-coding.yaml    | 90 +++++++++++++++++++
>  1 file changed, 90 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/dpi-color-coding.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/dpi-color-coding.yaml b/Documentation/devicetree/bindings/display/dpi-color-coding.yaml
> new file mode 100644
> index 000000000000..6430d6f1ddd1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/dpi-color-coding.yaml
> @@ -0,0 +1,90 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/dpi-color-coding.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MIPI DPI Interface Color Coding
> +
> +maintainers:
> +  - Liu Ying <victor.liu@nxp.com>
> +
> +description:
> +  MIPI Alliance Standard for Display Pixel Interface(DPI-2) Version 2.00(15
> +  September 2005) specifies color codings at the DPI interface.
> +
> +properties:
> +  dpi-color-coding:
> +    enum:
> +      - 16bit-configuration1
> +      - 16bit-configuration2
> +      - 16bit-configuration3
> +      - 18bit-configuration1
> +      - 18bit-configuration2
> +      - 24bit

Do we really needs strings there? It would be much better to use an int
plus a header

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 3/5] dt-bindings: display: simple-bridge: Document DPI color encoder

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3912 bytes --]

On Tue, Mar 04, 2025 at 06:15:28PM +0800, Liu Ying wrote:
> A DPI color encoder, as a simple display bridge, converts input DPI color
> coding to output DPI color coding, like Adafruit Kippah DPI hat[1] which
> converts input 18-bit pixel data to 24-bit pixel data(with 2 low padding
> bits in every color component though). Document the DPI color encoder.
> 
> [1] https://learn.adafruit.com/adafruit-dpi-display-kippah-ttl-tft/downloads
> 
> Signed-off-by: Liu Ying <victor.liu@nxp.com>

So it's just a bunch of signals that aren't wired / set to the ground? I
assume to free a few GPIOs?

I guess that makes sense.

> ---
>  .../display/bridge/simple-bridge.yaml         | 89 ++++++++++++++++++-
>  1 file changed, 87 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml b/Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml
> index 43cf4df9811a..c1747c033040 100644
> --- a/Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml
> @@ -27,6 +27,7 @@ properties:
>            - const: adi,adv7123
>        - enum:
>            - adi,adv7123
> +          - dpi-color-encoder

I don't think we can claim that there's a generic DPI color encoder.
Maybe we can add the prefix dumb or transparent?

>            - dumb-vga-dac
>            - ti,opa362
>            - ti,ths8134
> @@ -37,13 +38,31 @@ properties:
>  
>      properties:
>        port@0:
> -        $ref: /schemas/graph.yaml#/properties/port
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
>          description: The bridge input
>  
> +        properties:
> +          endpoint:
> +            $ref: /schemas/media/video-interfaces.yaml#
> +            unevaluatedProperties: false
> +
> +            properties:
> +              dpi-color-coding: true
> +
>        port@1:
> -        $ref: /schemas/graph.yaml#/properties/port
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
>          description: The bridge output
>  
> +        properties:
> +          endpoint:
> +            $ref: /schemas/media/video-interfaces.yaml#
> +            unevaluatedProperties: false
> +
> +            properties:
> +              dpi-color-coding: true
> +
>      required:
>        - port@0
>        - port@1
> @@ -61,6 +80,44 @@ required:
>  
>  additionalProperties: false
>  
> +allOf:
> +  - $ref: /schemas/display/dpi-color-coding.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: dpi-color-encoder
> +    then:
> +      properties:
> +        ports:
> +          properties:
> +            port@0:
> +              properties:
> +                endpoint:
> +                  required:
> +                    - dpi-color-coding
> +
> +            port@1:
> +              properties:
> +                endpoint:
> +                  required:
> +                    - dpi-color-coding
> +    else:
> +      properties:
> +        ports:
> +          properties:
> +            port@0:
> +              properties:
> +                endpoint:
> +                  properties:
> +                    dpi-color-coding: false
> +
> +            port@1:
> +              properties:
> +                endpoint:
> +                  properties:
> +                    dpi-color-coding: false
> +

Also it complicates the binding enough to warrant for another binding for
that specific "component".

>  examples:
>    - |
>      bridge {
> @@ -88,4 +145,32 @@ examples:
>          };
>      };
>  
> +  - |
> +    bridge {
> +        compatible = "dpi-color-enoder";

You have a typo in the compatible.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 4/5] drm/bridge: simple-bridge: Add DPI color encoder support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 785 bytes --]

On Tue, Mar 04, 2025 at 06:15:29PM +0800, Liu Ying wrote:
> A DPI color encoder, as a simple display bridge, converts input DPI color
> coding to output DPI color coding, like Adafruit Kippah DPI hat[1] which
> converts input 18-bit pixel data to 24-bit pixel data(with 2 low padding
> bits in every color component though). Add the DPI color encoder support
> in the simple bridge driver.
> 
> [1] https://learn.adafruit.com/adafruit-dpi-display-kippah-ttl-tft/downloads
> 
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> ---
>  drivers/gpu/drm/bridge/simple-bridge.c | 104 ++++++++++++++++++++++++-
>  1 file changed, 102 insertions(+), 2 deletions(-)

Same thing, I think it's easy enough to write a transparent bridge to
just put it into another driver.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 5/5] drm/bridge: simple-bridge: Add next panel support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3653 bytes --]

On Tue, Mar 04, 2025 at 06:15:30PM +0800, Liu Ying wrote:
> The next bridge connected to a simple bridge could be a panel, e.g.,
> a DPI panel connected to a DPI color encoder. Add the next panel support,
> instead of supporting non-panel next bridge only.
> 
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> ---
>  drivers/gpu/drm/bridge/Kconfig         |  1 +
>  drivers/gpu/drm/bridge/simple-bridge.c | 32 ++++++++++++++++----------
>  2 files changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index d20f1646dac2..92187dbdd32b 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -310,6 +310,7 @@ config DRM_SIMPLE_BRIDGE
>  	tristate "Simple DRM bridge support"
>  	depends on OF
>  	select DRM_KMS_HELPER
> +	select DRM_PANEL_BRIDGE
>  	help
>  	  Support for non-programmable DRM bridges, such as ADI ADV7123, TI
>  	  THS8134 and THS8135 or passive resistor ladder DACs.
> diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c
> index c0445bd20e07..4c585e5583ca 100644
> --- a/drivers/gpu/drm/bridge/simple-bridge.c
> +++ b/drivers/gpu/drm/bridge/simple-bridge.c
> @@ -19,6 +19,7 @@
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_edid.h>
>  #include <drm/drm_of.h>
> +#include <drm/drm_panel.h>
>  #include <drm/drm_print.h>
>  #include <drm/drm_probe_helper.h>
>  
> @@ -35,6 +36,7 @@ struct simple_bridge {
>  	const struct simple_bridge_info *info;
>  
>  	struct drm_bridge	*next_bridge;
> +	struct drm_panel	*next_panel;
>  	struct regulator	*vdd;
>  	struct gpio_desc	*enable;
>  
> @@ -114,6 +116,10 @@ static int simple_bridge_attach(struct drm_bridge *bridge,
>  	struct simple_bridge *sbridge = drm_bridge_to_simple_bridge(bridge);
>  	int ret;
>  
> +	if (sbridge->next_panel)
> +		return drm_bridge_attach(bridge->encoder, sbridge->next_bridge,
> +					 bridge, flags);
> +
>  	ret = drm_bridge_attach(bridge->encoder, sbridge->next_bridge, bridge,
>  				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>  	if (ret < 0)
> @@ -247,7 +253,6 @@ static int simple_bridge_get_dpi_color_coding(struct simple_bridge *sbridge,
>  static int simple_bridge_probe(struct platform_device *pdev)
>  {
>  	struct simple_bridge *sbridge;
> -	struct device_node *remote;
>  	int ret;
>  
>  	sbridge = devm_kzalloc(&pdev->dev, sizeof(*sbridge), GFP_KERNEL);
> @@ -257,17 +262,20 @@ static int simple_bridge_probe(struct platform_device *pdev)
>  	sbridge->info = of_device_get_match_data(&pdev->dev);
>  
>  	/* Get the next bridge in the pipeline. */
> -	remote = of_graph_get_remote_node(pdev->dev.of_node, 1, -1);
> -	if (!remote)
> -		return -EINVAL;
> -
> -	sbridge->next_bridge = of_drm_find_bridge(remote);
> -	of_node_put(remote);
> -
> -	if (!sbridge->next_bridge) {
> -		dev_dbg(&pdev->dev, "Next bridge not found, deferring probe\n");
> -		return -EPROBE_DEFER;
> -	}
> +	ret = drm_of_find_panel_or_bridge(pdev->dev.of_node, 1, -1,
> +					  &sbridge->next_panel,
> +					  &sbridge->next_bridge);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "Next panel or bridge not found\n");
> +
> +	if (sbridge->next_panel)
> +		sbridge->next_bridge = devm_drm_panel_bridge_add(&pdev->dev,
> +								 sbridge->next_panel);
> +
> +	if (IS_ERR(sbridge->next_bridge))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(sbridge->next_bridge),
> +				     "Next bridge not found\n");

This makes sense in general, but I think a better approach would be to
use devm/drmm_of_get_bridge here.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 3/5] dt-bindings: display: simple-bridge: Document DPI color encoder

[Rob Herring (Arm)]

On Tue, 04 Mar 2025 18:15:28 +0800, Liu Ying wrote:
> A DPI color encoder, as a simple display bridge, converts input DPI color
> coding to output DPI color coding, like Adafruit Kippah DPI hat[1] which
> converts input 18-bit pixel data to 24-bit pixel data(with 2 low padding
> bits in every color component though). Document the DPI color encoder.
> 
> [1] https://learn.adafruit.com/adafruit-dpi-display-kippah-ttl-tft/downloads
> 
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> ---
>  .../display/bridge/simple-bridge.yaml         | 89 ++++++++++++++++++-
>  1 file changed, 87 insertions(+), 2 deletions(-)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/display/bridge/simple-bridge.example.dtb: /example-1/bridge: failed to match any schema with compatible: ['dpi-color-enoder']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250304101530.969920-4-victor.liu@nxp.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 0/5] drm/bridge: simple-bridge: Add DPI color encoder support

[Alexander Stein]

Hi,

Am Dienstag, 4. März 2025, 11:15:25 CET schrieb Liu Ying:
> Hi,
> 
> This patch series aims to add DPI color encoder support as a simple DRM
> bridge.  A DPI color encoder simply converts input DPI color coding to
> output DPI color coding, like Adafruit Kippah DPI hat[1] which converts
> input 18-bit pixel data to 24-bit pixel data(with 2 low padding bits in
> every color component though).  A real use case is that NXP i.MX93 11x11
> EVK[2] and i.MX93 9x9 QSB[3] boards may connect a 24-bit DPI panel through
> the Adafruit Kippah DPI hat.  The display pipeline is
> 
> i.MX93 LCDIF display controller(RGB888) ->
> i.MX93 parallel display format configuration(RGB666) ->
> on-board Raspiberry Pi compatible interface(RPi)(RGB666) ->
> Adafruit Kippah DPI hat(RGB888 with 2 low padding bits in color components) ->
> 24-bit "ontat,kd50g21-40nt-a1" DPI panel
> 
> [1] https://learn.adafruit.com/adafruit-dpi-display-kippah-ttl-tft/downloads
> [2] https://www.nxp.com/design/design-center/development-boards-and-designs/i.MX93EVK
> [3] https://www.nxp.com/design/design-center/development-boards-and-designs/IMX93QSB

Thanks for this series.
Actually I was about to create a similar (dumb) bridge. My use case is wrong
wiring on hardware for DPI displays. The current workaround was to use a
"new" display compatible with bus_format changes from
MEDIA_BUS_FMT_RGB666_1X18 -> MEDIA_BUS_FMT_RGB888_1X24.

I added this new bridge and changed my DT and it works flawlessly.

Best regards
Alexander

> Liu Ying (5):
>   dt-bindings: display: Document DPI color codings
>   drm/of: Add drm_of_dpi_get_color_coding()
>   dt-bindings: display: simple-bridge: Document DPI color encoder
>   drm/bridge: simple-bridge: Add DPI color encoder support
>   drm/bridge: simple-bridge: Add next panel support
> 
>  .../display/bridge/simple-bridge.yaml         |  89 +++++++++++-
>  .../bindings/display/dpi-color-coding.yaml    |  90 ++++++++++++
>  drivers/gpu/drm/bridge/Kconfig                |   1 +
>  drivers/gpu/drm/bridge/simple-bridge.c        | 132 ++++++++++++++++--
>  drivers/gpu/drm/drm_of.c                      |  43 ++++++
>  include/drm/drm_of.h                          |   7 +
>  6 files changed, 348 insertions(+), 14 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/display/dpi-color-coding.yaml
> 
> 


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH 3/5] dt-bindings: display: simple-bridge: Document DPI color encoder

[Rob Herring]

On Tue, Mar 04, 2025 at 06:15:28PM +0800, Liu Ying wrote:
> A DPI color encoder, as a simple display bridge, converts input DPI color
> coding to output DPI color coding, like Adafruit Kippah DPI hat[1] which
> converts input 18-bit pixel data to 24-bit pixel data(with 2 low padding
> bits in every color component though). Document the DPI color encoder.

Why do we need a node for this? Isn't this just wired how it is wired 
and there's nothing for s/w to see or do? I suppose if you are trying to 
resolve the mode with 24-bit on one end and 18-bit on the other end, you 
need to allow that and not require an exact match. You still might need 
to figure out which pins the 18-bit data comes out on, but you have that 
problem with an 18-bit panel too. IOW, how is this any different if you 
have an 18-bit panel versus 24-bit panel?

Rob

Re: [PATCH 1/5] dt-bindings: display: Document DPI color codings

[Krzysztof Kozlowski]

On Tue, Mar 04, 2025 at 11:33:44AM +0100, Maxime Ripard wrote:
> > +properties:
> > +  dpi-color-coding:
> > +    enum:
> > +      - 16bit-configuration1
> > +      - 16bit-configuration2
> > +      - 16bit-configuration3
> > +      - 18bit-configuration1
> > +      - 18bit-configuration2
> > +      - 24bit
> 
> Do we really needs strings there? It would be much better to use an int
> plus a header

So DTS would sill have a name, just being a define? Then what is the
benefit comparing to strings above in DTS readability?

Best regards,
Krzysztof

Re: [PATCH 1/5] dt-bindings: display: Document DPI color codings

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 856 bytes --]

On Wed, Mar 05, 2025 at 08:51:35AM +0100, Krzysztof Kozlowski wrote:
> On Tue, Mar 04, 2025 at 11:33:44AM +0100, Maxime Ripard wrote:
> > > +properties:
> > > +  dpi-color-coding:
> > > +    enum:
> > > +      - 16bit-configuration1
> > > +      - 16bit-configuration2
> > > +      - 16bit-configuration3
> > > +      - 18bit-configuration1
> > > +      - 18bit-configuration2
> > > +      - 24bit
> > 
> > Do we really needs strings there? It would be much better to use an int
> > plus a header
> 
> So DTS would sill have a name, just being a define? Then what is the
> benefit comparing to strings above in DTS readability?

There's no benefits and no downside when it comes to readability.

However, it's not the only criteria, and not having to manipulate
strings but instead just doing int comparison is a huge plus.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 3/5] dt-bindings: display: simple-bridge: Document DPI color encoder

[Alexander Stein]

Hi,

Am Dienstag, 4. März 2025, 16:23:20 CET schrieb Rob Herring:
> On Tue, Mar 04, 2025 at 06:15:28PM +0800, Liu Ying wrote:
> > A DPI color encoder, as a simple display bridge, converts input DPI color
> > coding to output DPI color coding, like Adafruit Kippah DPI hat[1] which
> > converts input 18-bit pixel data to 24-bit pixel data(with 2 low padding
> > bits in every color component though). Document the DPI color encoder.
> 
> Why do we need a node for this? Isn't this just wired how it is wired 
> and there's nothing for s/w to see or do? I suppose if you are trying to 
> resolve the mode with 24-bit on one end and 18-bit on the other end, you 
> need to allow that and not require an exact match. You still might need 
> to figure out which pins the 18-bit data comes out on, but you have that 
> problem with an 18-bit panel too. IOW, how is this any different if you 
> have an 18-bit panel versus 24-bit panel?

Especially panel-simple.c has a fixed configuration for each display, such as:
> .bus_format = MEDIA_BUS_FMT_RGB666_1X18

How would you allow or even know it should be addressed as
MEDIA_BUS_FMT_RGB888_1X24 instead? I see different ways:
1. Create a new display setting/compatible
2. Add an overwrite property to the displays
3. Use a (transparent) bridge (this series)

Number 1 is IMHO out of question. I personally don't like number 2 as this
feels like adding quirks to displays, which they don't have.
Number 3 actually describe the hardware connection. The only impact for
software is to know which bus format it should use.

Best regards
Alexander
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH 1/5] dt-bindings: display: Document DPI color codings

[Krzysztof Kozlowski]

On 05/03/2025 09:26, Maxime Ripard wrote:
> On Wed, Mar 05, 2025 at 08:51:35AM +0100, Krzysztof Kozlowski wrote:
>> On Tue, Mar 04, 2025 at 11:33:44AM +0100, Maxime Ripard wrote:
>>>> +properties:
>>>> +  dpi-color-coding:
>>>> +    enum:
>>>> +      - 16bit-configuration1
>>>> +      - 16bit-configuration2
>>>> +      - 16bit-configuration3
>>>> +      - 18bit-configuration1
>>>> +      - 18bit-configuration2
>>>> +      - 24bit
>>>
>>> Do we really needs strings there? It would be much better to use an int
>>> plus a header
>>
>> So DTS would sill have a name, just being a define? Then what is the
>> benefit comparing to strings above in DTS readability?
> 
> There's no benefits and no downside when it comes to readability.
> 
> However, it's not the only criteria, and not having to manipulate
> strings but instead just doing int comparison is a huge plus.

Sure, defines work as well. BTW, it has a minor drawback on bindings as
it means you might need to update both binding and the header when
adding new entry, but I understand that it makes implementation life
easier or faster.

Best regards,
Krzysztof

Re: [PATCH 3/5] dt-bindings: display: simple-bridge: Document DPI color encoder

[Rob Herring]

On Wed, Mar 05, 2025 at 10:35:26AM +0100, Alexander Stein wrote:
> Hi,
> 
> Am Dienstag, 4. März 2025, 16:23:20 CET schrieb Rob Herring:
> > On Tue, Mar 04, 2025 at 06:15:28PM +0800, Liu Ying wrote:
> > > A DPI color encoder, as a simple display bridge, converts input DPI color
> > > coding to output DPI color coding, like Adafruit Kippah DPI hat[1] which
> > > converts input 18-bit pixel data to 24-bit pixel data(with 2 low padding
> > > bits in every color component though). Document the DPI color encoder.
> > 
> > Why do we need a node for this? Isn't this just wired how it is wired 
> > and there's nothing for s/w to see or do? I suppose if you are trying to 
> > resolve the mode with 24-bit on one end and 18-bit on the other end, you 
> > need to allow that and not require an exact match. You still might need 
> > to figure out which pins the 18-bit data comes out on, but you have that 
> > problem with an 18-bit panel too. IOW, how is this any different if you 
> > have an 18-bit panel versus 24-bit panel?
> 
> Especially panel-simple.c has a fixed configuration for each display, such as:
> > .bus_format = MEDIA_BUS_FMT_RGB666_1X18
> 
> How would you allow or even know it should be addressed as
> MEDIA_BUS_FMT_RGB888_1X24 instead? I see different ways:
> 1. Create a new display setting/compatible
> 2. Add an overwrite property to the displays
> 3. Use a (transparent) bridge (this series)
> 
> Number 1 is IMHO out of question. 

Agreed.

> I personally don't like number 2 as this
> feels like adding quirks to displays, which they don't have.

This is what I would do except apply it to the controller side. We know 
the panel side already. This is a board variation, so a property makes 
sense. I don't think you need any more than knowing what's on each end. 

> Number 3 actually describe the hardware connection. The only impact for
> software is to know which bus format it should use.

I'm not opposed to this, but only if it provides *something* that option 
2 does not. I'm not seeing what that is.

Node or not, either case needs a format property. We already have a 
variety of bus/pixel format related properties. I've rejected new ones 
because we need something common here that's flexible enough to handle 
any situation. That's either something that can describe any bit layout 
or something enumerating the formats (as MEDIA_BUS_FMT_* does). The 
former is hard to get right and there's always something else you can't 
handle. I'm not opposed to just reusing MEDIA_BUS_FMT_ if that works.

Rob

Re: [PATCH 3/5] dt-bindings: display: simple-bridge: Document DPI color encoder

[Liu Ying]

On 03/04/2025, Maxime Ripard wrote:
> On Tue, Mar 04, 2025 at 06:15:28PM +0800, Liu Ying wrote:
>> A DPI color encoder, as a simple display bridge, converts input DPI color
>> coding to output DPI color coding, like Adafruit Kippah DPI hat[1] which
>> converts input 18-bit pixel data to 24-bit pixel data(with 2 low padding
>> bits in every color component though). Document the DPI color encoder.
>>
>> [1] https://learn.adafruit.com/adafruit-dpi-display-kippah-ttl-tft/downloads
>>
>> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> 
> So it's just a bunch of signals that aren't wired / set to the ground? I
> assume to free a few GPIOs?

Yes. Freed pins could work as GPIOs or other functions.

> 
> I guess that makes sense.
> 
>> ---
>>  .../display/bridge/simple-bridge.yaml         | 89 ++++++++++++++++++-
>>  1 file changed, 87 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml b/Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml
>> index 43cf4df9811a..c1747c033040 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml
>> +++ b/Documentation/devicetree/bindings/display/bridge/simple-bridge.yaml
>> @@ -27,6 +27,7 @@ properties:
>>            - const: adi,adv7123
>>        - enum:
>>            - adi,adv7123
>> +          - dpi-color-encoder
> 
> I don't think we can claim that there's a generic DPI color encoder.
> Maybe we can add the prefix dumb or transparent?

TBH, I don't have a best compatible string and open to suggestions.
"dpi-color-encoder" came up to my mind just because the MIPI DPI standard
documentation mentions *color coding* and this bridge changes the coding,
hence *encoder*.

If folks think "dumb-dpi-color-encoder" or "transparent-dpi-color-encoder"
are good enough, I'd use it.  Just let me know which one to use.

> 
>>            - dumb-vga-dac
>>            - ti,opa362
>>            - ti,ths8134
>> @@ -37,13 +38,31 @@ properties:
>>  
>>      properties:
>>        port@0:
>> -        $ref: /schemas/graph.yaml#/properties/port
>> +        $ref: /schemas/graph.yaml#/$defs/port-base
>> +        unevaluatedProperties: false
>>          description: The bridge input
>>  
>> +        properties:
>> +          endpoint:
>> +            $ref: /schemas/media/video-interfaces.yaml#
>> +            unevaluatedProperties: false
>> +
>> +            properties:
>> +              dpi-color-coding: true
>> +
>>        port@1:
>> -        $ref: /schemas/graph.yaml#/properties/port
>> +        $ref: /schemas/graph.yaml#/$defs/port-base
>> +        unevaluatedProperties: false
>>          description: The bridge output
>>  
>> +        properties:
>> +          endpoint:
>> +            $ref: /schemas/media/video-interfaces.yaml#
>> +            unevaluatedProperties: false
>> +
>> +            properties:
>> +              dpi-color-coding: true
>> +
>>      required:
>>        - port@0
>>        - port@1
>> @@ -61,6 +80,44 @@ required:
>>  
>>  additionalProperties: false
>>  
>> +allOf:
>> +  - $ref: /schemas/display/dpi-color-coding.yaml#
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: dpi-color-encoder
>> +    then:
>> +      properties:
>> +        ports:
>> +          properties:
>> +            port@0:
>> +              properties:
>> +                endpoint:
>> +                  required:
>> +                    - dpi-color-coding
>> +
>> +            port@1:
>> +              properties:
>> +                endpoint:
>> +                  required:
>> +                    - dpi-color-coding
>> +    else:
>> +      properties:
>> +        ports:
>> +          properties:
>> +            port@0:
>> +              properties:
>> +                endpoint:
>> +                  properties:
>> +                    dpi-color-coding: false
>> +
>> +            port@1:
>> +              properties:
>> +                endpoint:
>> +                  properties:
>> +                    dpi-color-coding: false
>> +
> 
> Also it complicates the binding enough to warrant for another binding for
> that specific "component".

Do you suggest to write a separate/new DT binding for this bridge?

> 
>>  examples:
>>    - |
>>      bridge {
>> @@ -88,4 +145,32 @@ examples:
>>          };
>>      };
>>  
>> +  - |
>> +    bridge {
>> +        compatible = "dpi-color-enoder";
> 
> You have a typo in the compatible.

Hmm, good catch!  I did upgrade dtschema to version 2025.2 with pip3 and
didn't see any warnings/errors reported by dt_binding_check, weird...

> 
> Maxime

-- 
Regards,
Liu Ying

Re: [PATCH 4/5] drm/bridge: simple-bridge: Add DPI color encoder support

[Liu Ying]

On 03/04/2025, Maxime Ripard wrote:
> On Tue, Mar 04, 2025 at 06:15:29PM +0800, Liu Ying wrote:
>> A DPI color encoder, as a simple display bridge, converts input DPI color
>> coding to output DPI color coding, like Adafruit Kippah DPI hat[1] which
>> converts input 18-bit pixel data to 24-bit pixel data(with 2 low padding
>> bits in every color component though). Add the DPI color encoder support
>> in the simple bridge driver.
>>
>> [1] https://learn.adafruit.com/adafruit-dpi-display-kippah-ttl-tft/downloads
>>
>> Signed-off-by: Liu Ying <victor.liu@nxp.com>
>> ---
>>  drivers/gpu/drm/bridge/simple-bridge.c | 104 ++++++++++++++++++++++++-
>>  1 file changed, 102 insertions(+), 2 deletions(-)
> 
> Same thing, I think it's easy enough to write a transparent bridge to
> just put it into another driver.

I chose to touch this driver just because it seems that this DPI color
encoder fits the description of simple-bridge.yaml:

-8<-
description: |
  This binding supports transparent non-programmable bridges that don't
  require any configuration, with a single input and a single output.
-8<-

Anyway, if you think another driver is really needed and DT maintainers
accept to document this DPI color encoder, I may try to write one.

> 
> Maxime

-- 
Regards,
Liu Ying

Re: [PATCH 5/5] drm/bridge: simple-bridge: Add next panel support

[Liu Ying]

On 03/04/2025, Maxime Ripard wrote:
> On Tue, Mar 04, 2025 at 06:15:30PM +0800, Liu Ying wrote:
>> The next bridge connected to a simple bridge could be a panel, e.g.,
>> a DPI panel connected to a DPI color encoder. Add the next panel support,
>> instead of supporting non-panel next bridge only.
>>
>> Signed-off-by: Liu Ying <victor.liu@nxp.com>
>> ---
>>  drivers/gpu/drm/bridge/Kconfig         |  1 +
>>  drivers/gpu/drm/bridge/simple-bridge.c | 32 ++++++++++++++++----------
>>  2 files changed, 21 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
>> index d20f1646dac2..92187dbdd32b 100644
>> --- a/drivers/gpu/drm/bridge/Kconfig
>> +++ b/drivers/gpu/drm/bridge/Kconfig
>> @@ -310,6 +310,7 @@ config DRM_SIMPLE_BRIDGE
>>  	tristate "Simple DRM bridge support"
>>  	depends on OF
>>  	select DRM_KMS_HELPER
>> +	select DRM_PANEL_BRIDGE
>>  	help
>>  	  Support for non-programmable DRM bridges, such as ADI ADV7123, TI
>>  	  THS8134 and THS8135 or passive resistor ladder DACs.
>> diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c
>> index c0445bd20e07..4c585e5583ca 100644
>> --- a/drivers/gpu/drm/bridge/simple-bridge.c
>> +++ b/drivers/gpu/drm/bridge/simple-bridge.c
>> @@ -19,6 +19,7 @@
>>  #include <drm/drm_crtc.h>
>>  #include <drm/drm_edid.h>
>>  #include <drm/drm_of.h>
>> +#include <drm/drm_panel.h>
>>  #include <drm/drm_print.h>
>>  #include <drm/drm_probe_helper.h>
>>  
>> @@ -35,6 +36,7 @@ struct simple_bridge {
>>  	const struct simple_bridge_info *info;
>>  
>>  	struct drm_bridge	*next_bridge;
>> +	struct drm_panel	*next_panel;
>>  	struct regulator	*vdd;
>>  	struct gpio_desc	*enable;
>>  
>> @@ -114,6 +116,10 @@ static int simple_bridge_attach(struct drm_bridge *bridge,
>>  	struct simple_bridge *sbridge = drm_bridge_to_simple_bridge(bridge);
>>  	int ret;
>>  
>> +	if (sbridge->next_panel)
>> +		return drm_bridge_attach(bridge->encoder, sbridge->next_bridge,
>> +					 bridge, flags);
>> +
>>  	ret = drm_bridge_attach(bridge->encoder, sbridge->next_bridge, bridge,
>>  				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>>  	if (ret < 0)
>> @@ -247,7 +253,6 @@ static int simple_bridge_get_dpi_color_coding(struct simple_bridge *sbridge,
>>  static int simple_bridge_probe(struct platform_device *pdev)
>>  {
>>  	struct simple_bridge *sbridge;
>> -	struct device_node *remote;
>>  	int ret;
>>  
>>  	sbridge = devm_kzalloc(&pdev->dev, sizeof(*sbridge), GFP_KERNEL);
>> @@ -257,17 +262,20 @@ static int simple_bridge_probe(struct platform_device *pdev)
>>  	sbridge->info = of_device_get_match_data(&pdev->dev);
>>  
>>  	/* Get the next bridge in the pipeline. */
>> -	remote = of_graph_get_remote_node(pdev->dev.of_node, 1, -1);
>> -	if (!remote)
>> -		return -EINVAL;
>> -
>> -	sbridge->next_bridge = of_drm_find_bridge(remote);
>> -	of_node_put(remote);
>> -
>> -	if (!sbridge->next_bridge) {
>> -		dev_dbg(&pdev->dev, "Next bridge not found, deferring probe\n");
>> -		return -EPROBE_DEFER;
>> -	}
>> +	ret = drm_of_find_panel_or_bridge(pdev->dev.of_node, 1, -1,
>> +					  &sbridge->next_panel,
>> +					  &sbridge->next_bridge);
>> +	if (ret)
>> +		return dev_err_probe(&pdev->dev, ret,
>> +				     "Next panel or bridge not found\n");
>> +
>> +	if (sbridge->next_panel)
>> +		sbridge->next_bridge = devm_drm_panel_bridge_add(&pdev->dev,
>> +								 sbridge->next_panel);
>> +
>> +	if (IS_ERR(sbridge->next_bridge))
>> +		return dev_err_probe(&pdev->dev, PTR_ERR(sbridge->next_bridge),
>> +				     "Next bridge not found\n");
> 
> This makes sense in general, but I think a better approach would be to
> use devm/drmm_of_get_bridge here.

I chose to open-code devm_of_get_bridge() because sbridge->next_panel can
be grabbed and used to determine the logics in simple_bridge_attach() and
the flags handled over to drm_bridge_attach().

However, if a separate driver is needed for the DPI color encoder, I won't
touch this driver.

> 
> Maxime

-- 
Regards,
Liu Ying

Re: [PATCH 3/5] dt-bindings: display: simple-bridge: Document DPI color encoder

[Liu Ying]

On 03/06/2025, Rob Herring wrote:
> On Wed, Mar 05, 2025 at 10:35:26AM +0100, Alexander Stein wrote:
>> Hi,
>>
>> Am Dienstag, 4. März 2025, 16:23:20 CET schrieb Rob Herring:
>>> On Tue, Mar 04, 2025 at 06:15:28PM +0800, Liu Ying wrote:
>>>> A DPI color encoder, as a simple display bridge, converts input DPI color
>>>> coding to output DPI color coding, like Adafruit Kippah DPI hat[1] which
>>>> converts input 18-bit pixel data to 24-bit pixel data(with 2 low padding
>>>> bits in every color component though). Document the DPI color encoder.
>>>
>>> Why do we need a node for this? Isn't this just wired how it is wired 
>>> and there's nothing for s/w to see or do? I suppose if you are trying to 
>>> resolve the mode with 24-bit on one end and 18-bit on the other end, you 
>>> need to allow that and not require an exact match. You still might need 
>>> to figure out which pins the 18-bit data comes out on, but you have that 
>>> problem with an 18-bit panel too. IOW, how is this any different if you 
>>> have an 18-bit panel versus 24-bit panel?
>>
>> Especially panel-simple.c has a fixed configuration for each display, such as:
>>> .bus_format = MEDIA_BUS_FMT_RGB666_1X18
>>
>> How would you allow or even know it should be addressed as
>> MEDIA_BUS_FMT_RGB888_1X24 instead? I see different ways:
>> 1. Create a new display setting/compatible
>> 2. Add an overwrite property to the displays
>> 3. Use a (transparent) bridge (this series)
>>
>> Number 1 is IMHO out of question. 
> 
> Agreed.
> 
>> I personally don't like number 2 as this
>> feels like adding quirks to displays, which they don't have.
> 
> This is what I would do except apply it to the controller side. We know 
> the panel side already. This is a board variation, so a property makes 
> sense. I don't think you need any more than knowing what's on each end. 

With option 2, no matter putting a property in source side or sink side,
impacted display drivers and DT bindings need to be changed, once a board
manipulates the DPI color coding.  This adds burdens and introduces new
versions of those DT bindings.  Is this what we want?

> 
>> Number 3 actually describe the hardware connection. The only impact for
>> software is to know which bus format it should use.
> 
> I'm not opposed to this, but only if it provides *something* that option 
> 2 does not. I'm not seeing what that is.

Option 3 provides an intermediate bridge for both DT and OSes.  It makes
the DPI color coding manipulation transparent to source side and sink side.

> 
> Node or not, either case needs a format property. We already have a 
> variety of bus/pixel format related properties. I've rejected new ones 
> because we need something common here that's flexible enough to handle 
> any situation. That's either something that can describe any bit layout 
> or something enumerating the formats (as MEDIA_BUS_FMT_* does). The 
> former is hard to get right and there's always something else you can't 
> handle. I'm not opposed to just reusing MEDIA_BUS_FMT_ if that works.

Agree that the former is hard to get right.

Since this is all about DPI and we have that MIPI DPI standard document
which specifies color codings, maybe it's fine to start with those limited
color codings.

BTW, I'd admit that the artificial paddings are not described by this patch
set, though maybe no one cares about that.  The paddings could be floating/
low/high/other component bits.

> 
> Rob

-- 
Regards,
Liu Ying

Re: [PATCH 1/5] dt-bindings: display: Document DPI color codings

[Liu Ying]

On 03/05/2025, Krzysztof Kozlowski wrote:
> On 05/03/2025 09:26, Maxime Ripard wrote:
>> On Wed, Mar 05, 2025 at 08:51:35AM +0100, Krzysztof Kozlowski wrote:
>>> On Tue, Mar 04, 2025 at 11:33:44AM +0100, Maxime Ripard wrote:
>>>>> +properties:
>>>>> +  dpi-color-coding:
>>>>> +    enum:
>>>>> +      - 16bit-configuration1
>>>>> +      - 16bit-configuration2
>>>>> +      - 16bit-configuration3
>>>>> +      - 18bit-configuration1
>>>>> +      - 18bit-configuration2
>>>>> +      - 24bit
>>>>
>>>> Do we really needs strings there? It would be much better to use an int
>>>> plus a header

I thought it was fine to align to lvds-data-mapping.yaml where data-mapping
is defined as strings, like "vesa-24".

>>>
>>> So DTS would sill have a name, just being a define? Then what is the
>>> benefit comparing to strings above in DTS readability?
>>
>> There's no benefits and no downside when it comes to readability.
>>
>> However, it's not the only criteria, and not having to manipulate
>> strings but instead just doing int comparison is a huge plus.
> 
> Sure, defines work as well. BTW, it has a minor drawback on bindings as
> it means you might need to update both binding and the header when
> adding new entry, but I understand that it makes implementation life
> easier or faster.

If no objections, I'd use integer color codings.

> 
> Best regards,
> Krzysztof

-- 
Regards,
Liu Ying

Re: [PATCH 3/5] dt-bindings: display: simple-bridge: Document DPI color encoder

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2550 bytes --]

On Thu, Mar 06, 2025 at 03:02:41PM +0800, Liu Ying wrote:
> On 03/06/2025, Rob Herring wrote:
> > On Wed, Mar 05, 2025 at 10:35:26AM +0100, Alexander Stein wrote:
> >> Hi,
> >>
> >> Am Dienstag, 4. März 2025, 16:23:20 CET schrieb Rob Herring:
> >>> On Tue, Mar 04, 2025 at 06:15:28PM +0800, Liu Ying wrote:
> >>>> A DPI color encoder, as a simple display bridge, converts input DPI color
> >>>> coding to output DPI color coding, like Adafruit Kippah DPI hat[1] which
> >>>> converts input 18-bit pixel data to 24-bit pixel data(with 2 low padding
> >>>> bits in every color component though). Document the DPI color encoder.
> >>>
> >>> Why do we need a node for this? Isn't this just wired how it is wired 
> >>> and there's nothing for s/w to see or do? I suppose if you are trying to 
> >>> resolve the mode with 24-bit on one end and 18-bit on the other end, you 
> >>> need to allow that and not require an exact match. You still might need 
> >>> to figure out which pins the 18-bit data comes out on, but you have that 
> >>> problem with an 18-bit panel too. IOW, how is this any different if you 
> >>> have an 18-bit panel versus 24-bit panel?
> >>
> >> Especially panel-simple.c has a fixed configuration for each display, such as:
> >>> .bus_format = MEDIA_BUS_FMT_RGB666_1X18
> >>
> >> How would you allow or even know it should be addressed as
> >> MEDIA_BUS_FMT_RGB888_1X24 instead? I see different ways:
> >> 1. Create a new display setting/compatible
> >> 2. Add an overwrite property to the displays
> >> 3. Use a (transparent) bridge (this series)
> >>
> >> Number 1 is IMHO out of question. 
> > 
> > Agreed.
> > 
> >> I personally don't like number 2 as this
> >> feels like adding quirks to displays, which they don't have.
> > 
> > This is what I would do except apply it to the controller side. We know 
> > the panel side already. This is a board variation, so a property makes 
> > sense. I don't think you need any more than knowing what's on each end. 
> 
> With option 2, no matter putting a property in source side or sink side,
> impacted display drivers and DT bindings need to be changed, once a board
> manipulates the DPI color coding.  This adds burdens and introduces new
> versions of those DT bindings.  Is this what we want?

There's an option 4: make it a property of the OF graph endpoints. In
essence, it's similar to properties that are already there like
lane-mapping, and it wouldn't affect the panel drivers, or create an
intermediate bridge.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 3/5] dt-bindings: display: simple-bridge: Document DPI color encoder

[Rob Herring]

On Thu, Mar 06, 2025 at 12:35:49PM +0100, Maxime Ripard wrote:
> On Thu, Mar 06, 2025 at 03:02:41PM +0800, Liu Ying wrote:
> > On 03/06/2025, Rob Herring wrote:
> > > On Wed, Mar 05, 2025 at 10:35:26AM +0100, Alexander Stein wrote:
> > >> Hi,
> > >>
> > >> Am Dienstag, 4. März 2025, 16:23:20 CET schrieb Rob Herring:
> > >>> On Tue, Mar 04, 2025 at 06:15:28PM +0800, Liu Ying wrote:
> > >>>> A DPI color encoder, as a simple display bridge, converts input DPI color
> > >>>> coding to output DPI color coding, like Adafruit Kippah DPI hat[1] which
> > >>>> converts input 18-bit pixel data to 24-bit pixel data(with 2 low padding
> > >>>> bits in every color component though). Document the DPI color encoder.
> > >>>
> > >>> Why do we need a node for this? Isn't this just wired how it is wired 
> > >>> and there's nothing for s/w to see or do? I suppose if you are trying to 
> > >>> resolve the mode with 24-bit on one end and 18-bit on the other end, you 
> > >>> need to allow that and not require an exact match. You still might need 
> > >>> to figure out which pins the 18-bit data comes out on, but you have that 
> > >>> problem with an 18-bit panel too. IOW, how is this any different if you 
> > >>> have an 18-bit panel versus 24-bit panel?
> > >>
> > >> Especially panel-simple.c has a fixed configuration for each display, such as:
> > >>> .bus_format = MEDIA_BUS_FMT_RGB666_1X18
> > >>
> > >> How would you allow or even know it should be addressed as
> > >> MEDIA_BUS_FMT_RGB888_1X24 instead? I see different ways:
> > >> 1. Create a new display setting/compatible
> > >> 2. Add an overwrite property to the displays
> > >> 3. Use a (transparent) bridge (this series)
> > >>
> > >> Number 1 is IMHO out of question. 
> > > 
> > > Agreed.
> > > 
> > >> I personally don't like number 2 as this
> > >> feels like adding quirks to displays, which they don't have.
> > > 
> > > This is what I would do except apply it to the controller side. We know 
> > > the panel side already. This is a board variation, so a property makes 
> > > sense. I don't think you need any more than knowing what's on each end. 
> > 
> > With option 2, no matter putting a property in source side or sink side,
> > impacted display drivers and DT bindings need to be changed, once a board
> > manipulates the DPI color coding.  This adds burdens and introduces new
> > versions of those DT bindings.  Is this what we want?
> 
> There's an option 4: make it a property of the OF graph endpoints. In
> essence, it's similar to properties that are already there like
> lane-mapping, and it wouldn't affect the panel drivers, or create an
> intermediate bridge.

Yes, that's actually where I meant to put the property(ies).

Rob

Re: [PATCH 3/5] dt-bindings: display: simple-bridge: Document DPI color encoder

[Liu Ying]

On 03/06/2025, Maxime Ripard wrote:
> On Thu, Mar 06, 2025 at 03:02:41PM +0800, Liu Ying wrote:
>> On 03/06/2025, Rob Herring wrote:
>>> On Wed, Mar 05, 2025 at 10:35:26AM +0100, Alexander Stein wrote:
>>>> Hi,
>>>>
>>>> Am Dienstag, 4. März 2025, 16:23:20 CET schrieb Rob Herring:
>>>>> On Tue, Mar 04, 2025 at 06:15:28PM +0800, Liu Ying wrote:
>>>>>> A DPI color encoder, as a simple display bridge, converts input DPI color
>>>>>> coding to output DPI color coding, like Adafruit Kippah DPI hat[1] which
>>>>>> converts input 18-bit pixel data to 24-bit pixel data(with 2 low padding
>>>>>> bits in every color component though). Document the DPI color encoder.
>>>>>
>>>>> Why do we need a node for this? Isn't this just wired how it is wired 
>>>>> and there's nothing for s/w to see or do? I suppose if you are trying to 
>>>>> resolve the mode with 24-bit on one end and 18-bit on the other end, you 
>>>>> need to allow that and not require an exact match. You still might need 
>>>>> to figure out which pins the 18-bit data comes out on, but you have that 
>>>>> problem with an 18-bit panel too. IOW, how is this any different if you 
>>>>> have an 18-bit panel versus 24-bit panel?
>>>>
>>>> Especially panel-simple.c has a fixed configuration for each display, such as:
>>>>> .bus_format = MEDIA_BUS_FMT_RGB666_1X18
>>>>
>>>> How would you allow or even know it should be addressed as
>>>> MEDIA_BUS_FMT_RGB888_1X24 instead? I see different ways:
>>>> 1. Create a new display setting/compatible
>>>> 2. Add an overwrite property to the displays
>>>> 3. Use a (transparent) bridge (this series)
>>>>
>>>> Number 1 is IMHO out of question. 
>>>
>>> Agreed.
>>>
>>>> I personally don't like number 2 as this
>>>> feels like adding quirks to displays, which they don't have.
>>>
>>> This is what I would do except apply it to the controller side. We know 
>>> the panel side already. This is a board variation, so a property makes 
>>> sense. I don't think you need any more than knowing what's on each end. 
>>
>> With option 2, no matter putting a property in source side or sink side,
>> impacted display drivers and DT bindings need to be changed, once a board
>> manipulates the DPI color coding.  This adds burdens and introduces new
>> versions of those DT bindings.  Is this what we want?
> 
> There's an option 4: make it a property of the OF graph endpoints. In
> essence, it's similar to properties that are already there like
> lane-mapping, and it wouldn't affect the panel drivers, or create an
> intermediate bridge.

I don't see lane-mapping anywhere. Do you mean data-mapping instead?
data-mapping is not defined in dtschema. Only lvds-codec.yaml defines
data-mapping in endpoint.

With option 4, I guess you meant display sink drivers, i.e., panel and
bridge drivers, wouldn't be affected. Then, display source drivers, i.e.,
display controller and bridge drivers, would be affected. This adds
burdens for driver developers/maintainers(though almost no effort from
DT's PoV), doesn't it?

Moreover, why it has to be the display sink drivers which are not affected?
DT writers might choose to set the format at the sink endpoint, e.g., setting
RGB666 at the sink endpoint of a RGB888 DPI panel or bridge.

> 
> Maxime

-- 
Regards,
Liu Ying

Re: [PATCH 3/5] dt-bindings: display: simple-bridge: Document DPI color encoder

[Liu Ying]

On 03/07/2025, Rob Herring wrote:
> On Thu, Mar 06, 2025 at 12:35:49PM +0100, Maxime Ripard wrote:
>> On Thu, Mar 06, 2025 at 03:02:41PM +0800, Liu Ying wrote:
>>> On 03/06/2025, Rob Herring wrote:
>>>> On Wed, Mar 05, 2025 at 10:35:26AM +0100, Alexander Stein wrote:
>>>>> Hi,
>>>>>
>>>>> Am Dienstag, 4. März 2025, 16:23:20 CET schrieb Rob Herring:
>>>>>> On Tue, Mar 04, 2025 at 06:15:28PM +0800, Liu Ying wrote:
>>>>>>> A DPI color encoder, as a simple display bridge, converts input DPI color
>>>>>>> coding to output DPI color coding, like Adafruit Kippah DPI hat[1] which
>>>>>>> converts input 18-bit pixel data to 24-bit pixel data(with 2 low padding
>>>>>>> bits in every color component though). Document the DPI color encoder.
>>>>>>
>>>>>> Why do we need a node for this? Isn't this just wired how it is wired 
>>>>>> and there's nothing for s/w to see or do? I suppose if you are trying to 
>>>>>> resolve the mode with 24-bit on one end and 18-bit on the other end, you 
>>>>>> need to allow that and not require an exact match. You still might need 
>>>>>> to figure out which pins the 18-bit data comes out on, but you have that 
>>>>>> problem with an 18-bit panel too. IOW, how is this any different if you 
>>>>>> have an 18-bit panel versus 24-bit panel?
>>>>>
>>>>> Especially panel-simple.c has a fixed configuration for each display, such as:
>>>>>> .bus_format = MEDIA_BUS_FMT_RGB666_1X18
>>>>>
>>>>> How would you allow or even know it should be addressed as
>>>>> MEDIA_BUS_FMT_RGB888_1X24 instead? I see different ways:
>>>>> 1. Create a new display setting/compatible
>>>>> 2. Add an overwrite property to the displays
>>>>> 3. Use a (transparent) bridge (this series)
>>>>>
>>>>> Number 1 is IMHO out of question. 
>>>>
>>>> Agreed.
>>>>
>>>>> I personally don't like number 2 as this
>>>>> feels like adding quirks to displays, which they don't have.
>>>>
>>>> This is what I would do except apply it to the controller side. We know 
>>>> the panel side already. This is a board variation, so a property makes 
>>>> sense. I don't think you need any more than knowing what's on each end. 
>>>
>>> With option 2, no matter putting a property in source side or sink side,
>>> impacted display drivers and DT bindings need to be changed, once a board
>>> manipulates the DPI color coding.  This adds burdens and introduces new
>>> versions of those DT bindings.  Is this what we want?
>>
>> There's an option 4: make it a property of the OF graph endpoints. In
>> essence, it's similar to properties that are already there like
>> lane-mapping, and it wouldn't affect the panel drivers, or create an
>> intermediate bridge.
> 
> Yes, that's actually where I meant to put the property(ies).

Put optional dpi-color-coding or something else in endpoint-base?
Assuming it's optional, then it implies that it will overwrite OS's setting,
which sounds kinda awkward, because it is supposed to be required to describe
the actual color coding. If it's required, then we would have to create
something like dpi-endpoint-base, but existing display device DT nodes are
based on endpoint-base.

> 
> Rob

-- 
Regards,
Liu Ying

Re: [PATCH 3/5] dt-bindings: display: simple-bridge: Document DPI color encoder

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3839 bytes --]

On Fri, Mar 07, 2025 at 11:10:00AM +0800, Liu Ying wrote:
> On 03/06/2025, Maxime Ripard wrote:
> > On Thu, Mar 06, 2025 at 03:02:41PM +0800, Liu Ying wrote:
> >> On 03/06/2025, Rob Herring wrote:
> >>> On Wed, Mar 05, 2025 at 10:35:26AM +0100, Alexander Stein wrote:
> >>>> Hi,
> >>>>
> >>>> Am Dienstag, 4. März 2025, 16:23:20 CET schrieb Rob Herring:
> >>>>> On Tue, Mar 04, 2025 at 06:15:28PM +0800, Liu Ying wrote:
> >>>>>> A DPI color encoder, as a simple display bridge, converts input DPI color
> >>>>>> coding to output DPI color coding, like Adafruit Kippah DPI hat[1] which
> >>>>>> converts input 18-bit pixel data to 24-bit pixel data(with 2 low padding
> >>>>>> bits in every color component though). Document the DPI color encoder.
> >>>>>
> >>>>> Why do we need a node for this? Isn't this just wired how it is wired 
> >>>>> and there's nothing for s/w to see or do? I suppose if you are trying to 
> >>>>> resolve the mode with 24-bit on one end and 18-bit on the other end, you 
> >>>>> need to allow that and not require an exact match. You still might need 
> >>>>> to figure out which pins the 18-bit data comes out on, but you have that 
> >>>>> problem with an 18-bit panel too. IOW, how is this any different if you 
> >>>>> have an 18-bit panel versus 24-bit panel?
> >>>>
> >>>> Especially panel-simple.c has a fixed configuration for each display, such as:
> >>>>> .bus_format = MEDIA_BUS_FMT_RGB666_1X18
> >>>>
> >>>> How would you allow or even know it should be addressed as
> >>>> MEDIA_BUS_FMT_RGB888_1X24 instead? I see different ways:
> >>>> 1. Create a new display setting/compatible
> >>>> 2. Add an overwrite property to the displays
> >>>> 3. Use a (transparent) bridge (this series)
> >>>>
> >>>> Number 1 is IMHO out of question. 
> >>>
> >>> Agreed.
> >>>
> >>>> I personally don't like number 2 as this
> >>>> feels like adding quirks to displays, which they don't have.
> >>>
> >>> This is what I would do except apply it to the controller side. We know 
> >>> the panel side already. This is a board variation, so a property makes 
> >>> sense. I don't think you need any more than knowing what's on each end. 
> >>
> >> With option 2, no matter putting a property in source side or sink side,
> >> impacted display drivers and DT bindings need to be changed, once a board
> >> manipulates the DPI color coding.  This adds burdens and introduces new
> >> versions of those DT bindings.  Is this what we want?
> > 
> > There's an option 4: make it a property of the OF graph endpoints. In
> > essence, it's similar to properties that are already there like
> > lane-mapping, and it wouldn't affect the panel drivers, or create an
> > intermediate bridge.
> 
> I don't see lane-mapping anywhere. Do you mean data-mapping instead?
> data-mapping is not defined in dtschema. Only lvds-codec.yaml defines
> data-mapping in endpoint.

I meant as a general concept. The properties are data-lanes and
clock-lanes in
Documentation/devicetree/bindings/media/video-interfaces.yaml

> With option 4, I guess you meant display sink drivers, i.e., panel and
> bridge drivers, wouldn't be affected. Then, display source drivers, i.e.,
> display controller and bridge drivers, would be affected. This adds
> burdens for driver developers/maintainers(though almost no effort from
> DT's PoV), doesn't it?

Not necessarily, panels have a phandle to the parent endpoint too so
they can do that walk and configure their format if it's any easier.

> Moreover, why it has to be the display sink drivers which are not affected?
> DT writers might choose to set the format at the sink endpoint, e.g., setting
> RGB666 at the sink endpoint of a RGB888 DPI panel or bridge.

Why wouldn't you run the panel at the highest bpc possible?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 3/5] dt-bindings: display: simple-bridge: Document DPI color encoder

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3708 bytes --]

On Fri, Mar 07, 2025 at 11:25:40AM +0800, Liu Ying wrote:
> On 03/07/2025, Rob Herring wrote:
> > On Thu, Mar 06, 2025 at 12:35:49PM +0100, Maxime Ripard wrote:
> >> On Thu, Mar 06, 2025 at 03:02:41PM +0800, Liu Ying wrote:
> >>> On 03/06/2025, Rob Herring wrote:
> >>>> On Wed, Mar 05, 2025 at 10:35:26AM +0100, Alexander Stein wrote:
> >>>>> Hi,
> >>>>>
> >>>>> Am Dienstag, 4. März 2025, 16:23:20 CET schrieb Rob Herring:
> >>>>>> On Tue, Mar 04, 2025 at 06:15:28PM +0800, Liu Ying wrote:
> >>>>>>> A DPI color encoder, as a simple display bridge, converts input DPI color
> >>>>>>> coding to output DPI color coding, like Adafruit Kippah DPI hat[1] which
> >>>>>>> converts input 18-bit pixel data to 24-bit pixel data(with 2 low padding
> >>>>>>> bits in every color component though). Document the DPI color encoder.
> >>>>>>
> >>>>>> Why do we need a node for this? Isn't this just wired how it is wired 
> >>>>>> and there's nothing for s/w to see or do? I suppose if you are trying to 
> >>>>>> resolve the mode with 24-bit on one end and 18-bit on the other end, you 
> >>>>>> need to allow that and not require an exact match. You still might need 
> >>>>>> to figure out which pins the 18-bit data comes out on, but you have that 
> >>>>>> problem with an 18-bit panel too. IOW, how is this any different if you 
> >>>>>> have an 18-bit panel versus 24-bit panel?
> >>>>>
> >>>>> Especially panel-simple.c has a fixed configuration for each display, such as:
> >>>>>> .bus_format = MEDIA_BUS_FMT_RGB666_1X18
> >>>>>
> >>>>> How would you allow or even know it should be addressed as
> >>>>> MEDIA_BUS_FMT_RGB888_1X24 instead? I see different ways:
> >>>>> 1. Create a new display setting/compatible
> >>>>> 2. Add an overwrite property to the displays
> >>>>> 3. Use a (transparent) bridge (this series)
> >>>>>
> >>>>> Number 1 is IMHO out of question. 
> >>>>
> >>>> Agreed.
> >>>>
> >>>>> I personally don't like number 2 as this
> >>>>> feels like adding quirks to displays, which they don't have.
> >>>>
> >>>> This is what I would do except apply it to the controller side. We know 
> >>>> the panel side already. This is a board variation, so a property makes 
> >>>> sense. I don't think you need any more than knowing what's on each end. 
> >>>
> >>> With option 2, no matter putting a property in source side or sink side,
> >>> impacted display drivers and DT bindings need to be changed, once a board
> >>> manipulates the DPI color coding.  This adds burdens and introduces new
> >>> versions of those DT bindings.  Is this what we want?
> >>
> >> There's an option 4: make it a property of the OF graph endpoints. In
> >> essence, it's similar to properties that are already there like
> >> lane-mapping, and it wouldn't affect the panel drivers, or create an
> >> intermediate bridge.
> > 
> > Yes, that's actually where I meant to put the property(ies).
> 
> Put optional dpi-color-coding or something else in endpoint-base?

I'm not sure what you mean by endpoint base, but it would be just like
data-lanes, on the endpoint itself, right next to remote-endpoint. Given
the nomenclature we have, something like "color-format" or
"color-encoding", and taking the media format bus as value.

> Assuming it's optional, then it implies that it will overwrite OS's
> setting, which sounds kinda awkward, because it is supposed to be
> required to describe the actual color coding.

I'm sorry, I don't understand what you mean here. Your bridge would have
been optional as well, right?

Worst case scenario, your driver could make that property mandatory on
its endpoints. Plenty of drivers are doing it.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 3/5] dt-bindings: display: simple-bridge: Document DPI color encoder

[Liu Ying]

On 03/10/2025, Maxime Ripard wrote:
> On Fri, Mar 07, 2025 at 11:25:40AM +0800, Liu Ying wrote:
>> On 03/07/2025, Rob Herring wrote:
>>> On Thu, Mar 06, 2025 at 12:35:49PM +0100, Maxime Ripard wrote:
>>>> On Thu, Mar 06, 2025 at 03:02:41PM +0800, Liu Ying wrote:
>>>>> On 03/06/2025, Rob Herring wrote:
>>>>>> On Wed, Mar 05, 2025 at 10:35:26AM +0100, Alexander Stein wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Am Dienstag, 4. März 2025, 16:23:20 CET schrieb Rob Herring:
>>>>>>>> On Tue, Mar 04, 2025 at 06:15:28PM +0800, Liu Ying wrote:
>>>>>>>>> A DPI color encoder, as a simple display bridge, converts input DPI color
>>>>>>>>> coding to output DPI color coding, like Adafruit Kippah DPI hat[1] which
>>>>>>>>> converts input 18-bit pixel data to 24-bit pixel data(with 2 low padding
>>>>>>>>> bits in every color component though). Document the DPI color encoder.
>>>>>>>>
>>>>>>>> Why do we need a node for this? Isn't this just wired how it is wired 
>>>>>>>> and there's nothing for s/w to see or do? I suppose if you are trying to 
>>>>>>>> resolve the mode with 24-bit on one end and 18-bit on the other end, you 
>>>>>>>> need to allow that and not require an exact match. You still might need 
>>>>>>>> to figure out which pins the 18-bit data comes out on, but you have that 
>>>>>>>> problem with an 18-bit panel too. IOW, how is this any different if you 
>>>>>>>> have an 18-bit panel versus 24-bit panel?
>>>>>>>
>>>>>>> Especially panel-simple.c has a fixed configuration for each display, such as:
>>>>>>>> .bus_format = MEDIA_BUS_FMT_RGB666_1X18
>>>>>>>
>>>>>>> How would you allow or even know it should be addressed as
>>>>>>> MEDIA_BUS_FMT_RGB888_1X24 instead? I see different ways:
>>>>>>> 1. Create a new display setting/compatible
>>>>>>> 2. Add an overwrite property to the displays
>>>>>>> 3. Use a (transparent) bridge (this series)
>>>>>>>
>>>>>>> Number 1 is IMHO out of question. 
>>>>>>
>>>>>> Agreed.
>>>>>>
>>>>>>> I personally don't like number 2 as this
>>>>>>> feels like adding quirks to displays, which they don't have.
>>>>>>
>>>>>> This is what I would do except apply it to the controller side. We know 
>>>>>> the panel side already. This is a board variation, so a property makes 
>>>>>> sense. I don't think you need any more than knowing what's on each end. 
>>>>>
>>>>> With option 2, no matter putting a property in source side or sink side,
>>>>> impacted display drivers and DT bindings need to be changed, once a board
>>>>> manipulates the DPI color coding.  This adds burdens and introduces new
>>>>> versions of those DT bindings.  Is this what we want?
>>>>
>>>> There's an option 4: make it a property of the OF graph endpoints. In
>>>> essence, it's similar to properties that are already there like
>>>> lane-mapping, and it wouldn't affect the panel drivers, or create an
>>>> intermediate bridge.
>>>
>>> Yes, that's actually where I meant to put the property(ies).
>>
>> Put optional dpi-color-coding or something else in endpoint-base?
> 
> I'm not sure what you mean by endpoint base, but it would be just like

I meant the endpoint-base definition in graph.yaml.

https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph.yaml#L37

If optional dpi-color-coding property or something else is put there, then
any existing DT binding doc which references it starts to contain the
additional property automatically, which means those existing DT binding docs
don't need any change.  I'm not saying that this is ok to do(due to burdens
added by driver modification and maintainence) - I still think option 3 is the
right thing to do.

> data-lanes, on the endpoint itself, right next to remote-endpoint. Given
> the nomenclature we have, something like "color-format" or
> "color-encoding", and taking the media format bus as value.

This requires to change drivers and maintain the change, which adds burdens.

> 
>> Assuming it's optional, then it implies that it will overwrite OS's
>> setting, which sounds kinda awkward, because it is supposed to be
>> required to describe the actual color coding.
> 
> I'm sorry, I don't understand what you mean here. Your bridge would have

I meant an optional new property is not that welcomed and it is supposed
to be required.

> been optional as well, right?

No, if _no_ additional property is added to endpoint-base in graph.yaml or
to video-interfaces.yaml, then my bridge would be required, not optional,
because that would be the only way to support DPI color encoding.

> 
> Worst case scenario, your driver could make that property mandatory on
> its endpoints. Plenty of drivers are doing it.

This adds a new property to existing DT binding docs(yet another new version
of DT binding doc) and requires modifications on existing drivers, which again
adds burdens.  It's also a burden for developers to support that property in
new DT binding docs and new drivers.  For existing DT binding docs and drivers
which are using that property, that's fine and good to them.

> 
> Maxime

-- 
Regards,
Liu Ying

Re: [PATCH 3/5] dt-bindings: display: simple-bridge: Document DPI color encoder

[Liu Ying]

On 03/10/2025, Maxime Ripard wrote:
> On Fri, Mar 07, 2025 at 11:10:00AM +0800, Liu Ying wrote:
>> On 03/06/2025, Maxime Ripard wrote:
>>> On Thu, Mar 06, 2025 at 03:02:41PM +0800, Liu Ying wrote:
>>>> On 03/06/2025, Rob Herring wrote:
>>>>> On Wed, Mar 05, 2025 at 10:35:26AM +0100, Alexander Stein wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Am Dienstag, 4. März 2025, 16:23:20 CET schrieb Rob Herring:
>>>>>>> On Tue, Mar 04, 2025 at 06:15:28PM +0800, Liu Ying wrote:
>>>>>>>> A DPI color encoder, as a simple display bridge, converts input DPI color
>>>>>>>> coding to output DPI color coding, like Adafruit Kippah DPI hat[1] which
>>>>>>>> converts input 18-bit pixel data to 24-bit pixel data(with 2 low padding
>>>>>>>> bits in every color component though). Document the DPI color encoder.
>>>>>>>
>>>>>>> Why do we need a node for this? Isn't this just wired how it is wired 
>>>>>>> and there's nothing for s/w to see or do? I suppose if you are trying to 
>>>>>>> resolve the mode with 24-bit on one end and 18-bit on the other end, you 
>>>>>>> need to allow that and not require an exact match. You still might need 
>>>>>>> to figure out which pins the 18-bit data comes out on, but you have that 
>>>>>>> problem with an 18-bit panel too. IOW, how is this any different if you 
>>>>>>> have an 18-bit panel versus 24-bit panel?
>>>>>>
>>>>>> Especially panel-simple.c has a fixed configuration for each display, such as:
>>>>>>> .bus_format = MEDIA_BUS_FMT_RGB666_1X18
>>>>>>
>>>>>> How would you allow or even know it should be addressed as
>>>>>> MEDIA_BUS_FMT_RGB888_1X24 instead? I see different ways:
>>>>>> 1. Create a new display setting/compatible
>>>>>> 2. Add an overwrite property to the displays
>>>>>> 3. Use a (transparent) bridge (this series)
>>>>>>
>>>>>> Number 1 is IMHO out of question. 
>>>>>
>>>>> Agreed.
>>>>>
>>>>>> I personally don't like number 2 as this
>>>>>> feels like adding quirks to displays, which they don't have.
>>>>>
>>>>> This is what I would do except apply it to the controller side. We know 
>>>>> the panel side already. This is a board variation, so a property makes 
>>>>> sense. I don't think you need any more than knowing what's on each end. 
>>>>
>>>> With option 2, no matter putting a property in source side or sink side,
>>>> impacted display drivers and DT bindings need to be changed, once a board
>>>> manipulates the DPI color coding.  This adds burdens and introduces new
>>>> versions of those DT bindings.  Is this what we want?
>>>
>>> There's an option 4: make it a property of the OF graph endpoints. In
>>> essence, it's similar to properties that are already there like
>>> lane-mapping, and it wouldn't affect the panel drivers, or create an
>>> intermediate bridge.
>>
>> I don't see lane-mapping anywhere. Do you mean data-mapping instead?
>> data-mapping is not defined in dtschema. Only lvds-codec.yaml defines
>> data-mapping in endpoint.
> 
> I meant as a general concept. The properties are data-lanes and
> clock-lanes in
> Documentation/devicetree/bindings/media/video-interfaces.yaml

This requires referenceing video-interfaces.yaml in existing DT binding docs
and driver modifictions, which adds burdens.

> 
>> With option 4, I guess you meant display sink drivers, i.e., panel and
>> bridge drivers, wouldn't be affected. Then, display source drivers, i.e.,
>> display controller and bridge drivers, would be affected. This adds
>> burdens for driver developers/maintainers(though almost no effort from
>> DT's PoV), doesn't it?
> 
> Not necessarily, panels have a phandle to the parent endpoint too so
> they can do that walk and configure their format if it's any easier.

I'm sorry, I don't get your meaning here.  I have no idea how to support
this new property in endpoint-base(graph.yaml) or video-interfaces.yaml
_without_ changing existing display source drivers.

> 
>> Moreover, why it has to be the display sink drivers which are not affected?
>> DT writers might choose to set the format at the sink endpoint, e.g., setting
>> RGB666 at the sink endpoint of a RGB888 DPI panel or bridge.
> 
> Why wouldn't you run the panel at the highest bpc possible?

Because hardware designers route less data signals than regular ones to the
DPI panel or bridge.

> 
> Maxime

-- 
Regards,
Liu Ying

Re: [PATCH 3/5] dt-bindings: display: simple-bridge: Document DPI color encoder

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 6147 bytes --]

Hi,

On Tue, Mar 11, 2025 at 10:29:28AM +0800, Liu Ying wrote:
> On 03/10/2025, Maxime Ripard wrote:
> > On Fri, Mar 07, 2025 at 11:25:40AM +0800, Liu Ying wrote:
> >> On 03/07/2025, Rob Herring wrote:
> >>> On Thu, Mar 06, 2025 at 12:35:49PM +0100, Maxime Ripard wrote:
> >>>> On Thu, Mar 06, 2025 at 03:02:41PM +0800, Liu Ying wrote:
> >>>>> On 03/06/2025, Rob Herring wrote:
> >>>>>> On Wed, Mar 05, 2025 at 10:35:26AM +0100, Alexander Stein wrote:
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> Am Dienstag, 4. März 2025, 16:23:20 CET schrieb Rob Herring:
> >>>>>>>> On Tue, Mar 04, 2025 at 06:15:28PM +0800, Liu Ying wrote:
> >>>>>>>>> A DPI color encoder, as a simple display bridge, converts input DPI color
> >>>>>>>>> coding to output DPI color coding, like Adafruit Kippah DPI hat[1] which
> >>>>>>>>> converts input 18-bit pixel data to 24-bit pixel data(with 2 low padding
> >>>>>>>>> bits in every color component though). Document the DPI color encoder.
> >>>>>>>>
> >>>>>>>> Why do we need a node for this? Isn't this just wired how it is wired 
> >>>>>>>> and there's nothing for s/w to see or do? I suppose if you are trying to 
> >>>>>>>> resolve the mode with 24-bit on one end and 18-bit on the other end, you 
> >>>>>>>> need to allow that and not require an exact match. You still might need 
> >>>>>>>> to figure out which pins the 18-bit data comes out on, but you have that 
> >>>>>>>> problem with an 18-bit panel too. IOW, how is this any different if you 
> >>>>>>>> have an 18-bit panel versus 24-bit panel?
> >>>>>>>
> >>>>>>> Especially panel-simple.c has a fixed configuration for each display, such as:
> >>>>>>>> .bus_format = MEDIA_BUS_FMT_RGB666_1X18
> >>>>>>>
> >>>>>>> How would you allow or even know it should be addressed as
> >>>>>>> MEDIA_BUS_FMT_RGB888_1X24 instead? I see different ways:
> >>>>>>> 1. Create a new display setting/compatible
> >>>>>>> 2. Add an overwrite property to the displays
> >>>>>>> 3. Use a (transparent) bridge (this series)
> >>>>>>>
> >>>>>>> Number 1 is IMHO out of question. 
> >>>>>>
> >>>>>> Agreed.
> >>>>>>
> >>>>>>> I personally don't like number 2 as this
> >>>>>>> feels like adding quirks to displays, which they don't have.
> >>>>>>
> >>>>>> This is what I would do except apply it to the controller side. We know 
> >>>>>> the panel side already. This is a board variation, so a property makes 
> >>>>>> sense. I don't think you need any more than knowing what's on each end. 
> >>>>>
> >>>>> With option 2, no matter putting a property in source side or sink side,
> >>>>> impacted display drivers and DT bindings need to be changed, once a board
> >>>>> manipulates the DPI color coding.  This adds burdens and introduces new
> >>>>> versions of those DT bindings.  Is this what we want?
> >>>>
> >>>> There's an option 4: make it a property of the OF graph endpoints. In
> >>>> essence, it's similar to properties that are already there like
> >>>> lane-mapping, and it wouldn't affect the panel drivers, or create an
> >>>> intermediate bridge.
> >>>
> >>> Yes, that's actually where I meant to put the property(ies).
> >>
> >> Put optional dpi-color-coding or something else in endpoint-base?
> > 
> > I'm not sure what you mean by endpoint base, but it would be just like
> 
> I meant the endpoint-base definition in graph.yaml.
> 
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph.yaml#L37

I don't think it should be put in the graph binding document, but either
in video-interfaces, or, since it's mostly used by v4l2, in a similar
but separate document for DRM.

> If optional dpi-color-coding property or something else is put there, then
> any existing DT binding doc which references it starts to contain the
> additional property automatically, which means those existing DT binding docs
> don't need any change.  I'm not saying that this is ok to do(due to burdens
> added by driver modification and maintainence) - I still think option 3 is the
> right thing to do.

If the property is generic, and the support for it done well enough,
it's probably going to be a single function call in drivers. I wouldn't
call that a burden.

> > data-lanes, on the endpoint itself, right next to remote-endpoint. Given
> > the nomenclature we have, something like "color-format" or
> > "color-encoding", and taking the media format bus as value.
> 
> This requires to change drivers and maintain the change, which adds burdens.
> 
> > 
> >> Assuming it's optional, then it implies that it will overwrite OS's
> >> setting, which sounds kinda awkward, because it is supposed to be
> >> required to describe the actual color coding.
> > 
> > I'm sorry, I don't understand what you mean here. Your bridge would have
> 
> I meant an optional new property is not that welcomed

Not that welcomed by whom?

> and it is supposed to be required.

Not supposed to be required by whom?

> > been optional as well, right?
> 
> No, if _no_ additional property is added to endpoint-base in graph.yaml or
> to video-interfaces.yaml, then my bridge would be required, not optional,
> because that would be the only way to support DPI color encoding.

I mean, you can turn that any way you like, but we've supported the
setups your bridge needs to cover fine so far. Your work would
definitely improve the situation, but it's definitely not going to be
mandatory. Just like the color encoding property wouldn't be.

> > Worst case scenario, your driver could make that property mandatory on
> > its endpoints. Plenty of drivers are doing it.
> 
> This adds a new property to existing DT binding docs(yet another new version
> of DT binding doc) and requires modifications on existing drivers, which again
> adds burdens.  It's also a burden for developers to support that property in
> new DT binding docs and new drivers.  For existing DT binding docs and drivers
> which are using that property, that's fine and good to them.

Both Rob and I are maintainers of the affected code, let us worry about
the maintenance burden.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 3/5] dt-bindings: display: simple-bridge: Document DPI color encoder

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 5226 bytes --]

On Tue, Mar 11, 2025 at 10:38:37AM +0800, Liu Ying wrote:
> On 03/10/2025, Maxime Ripard wrote:
> > On Fri, Mar 07, 2025 at 11:10:00AM +0800, Liu Ying wrote:
> >> On 03/06/2025, Maxime Ripard wrote:
> >>> On Thu, Mar 06, 2025 at 03:02:41PM +0800, Liu Ying wrote:
> >>>> On 03/06/2025, Rob Herring wrote:
> >>>>> On Wed, Mar 05, 2025 at 10:35:26AM +0100, Alexander Stein wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> Am Dienstag, 4. März 2025, 16:23:20 CET schrieb Rob Herring:
> >>>>>>> On Tue, Mar 04, 2025 at 06:15:28PM +0800, Liu Ying wrote:
> >>>>>>>> A DPI color encoder, as a simple display bridge, converts input DPI color
> >>>>>>>> coding to output DPI color coding, like Adafruit Kippah DPI hat[1] which
> >>>>>>>> converts input 18-bit pixel data to 24-bit pixel data(with 2 low padding
> >>>>>>>> bits in every color component though). Document the DPI color encoder.
> >>>>>>>
> >>>>>>> Why do we need a node for this? Isn't this just wired how it is wired 
> >>>>>>> and there's nothing for s/w to see or do? I suppose if you are trying to 
> >>>>>>> resolve the mode with 24-bit on one end and 18-bit on the other end, you 
> >>>>>>> need to allow that and not require an exact match. You still might need 
> >>>>>>> to figure out which pins the 18-bit data comes out on, but you have that 
> >>>>>>> problem with an 18-bit panel too. IOW, how is this any different if you 
> >>>>>>> have an 18-bit panel versus 24-bit panel?
> >>>>>>
> >>>>>> Especially panel-simple.c has a fixed configuration for each display, such as:
> >>>>>>> .bus_format = MEDIA_BUS_FMT_RGB666_1X18
> >>>>>>
> >>>>>> How would you allow or even know it should be addressed as
> >>>>>> MEDIA_BUS_FMT_RGB888_1X24 instead? I see different ways:
> >>>>>> 1. Create a new display setting/compatible
> >>>>>> 2. Add an overwrite property to the displays
> >>>>>> 3. Use a (transparent) bridge (this series)
> >>>>>>
> >>>>>> Number 1 is IMHO out of question. 
> >>>>>
> >>>>> Agreed.
> >>>>>
> >>>>>> I personally don't like number 2 as this
> >>>>>> feels like adding quirks to displays, which they don't have.
> >>>>>
> >>>>> This is what I would do except apply it to the controller side. We know 
> >>>>> the panel side already. This is a board variation, so a property makes 
> >>>>> sense. I don't think you need any more than knowing what's on each end. 
> >>>>
> >>>> With option 2, no matter putting a property in source side or sink side,
> >>>> impacted display drivers and DT bindings need to be changed, once a board
> >>>> manipulates the DPI color coding.  This adds burdens and introduces new
> >>>> versions of those DT bindings.  Is this what we want?
> >>>
> >>> There's an option 4: make it a property of the OF graph endpoints. In
> >>> essence, it's similar to properties that are already there like
> >>> lane-mapping, and it wouldn't affect the panel drivers, or create an
> >>> intermediate bridge.
> >>
> >> I don't see lane-mapping anywhere. Do you mean data-mapping instead?
> >> data-mapping is not defined in dtschema. Only lvds-codec.yaml defines
> >> data-mapping in endpoint.
> > 
> > I meant as a general concept. The properties are data-lanes and
> > clock-lanes in
> > Documentation/devicetree/bindings/media/video-interfaces.yaml
> 
> This requires referenceing video-interfaces.yaml in existing DT binding docs
> and driver modifictions, which adds burdens.

If burden is an issue, any new driver also adds burden, thus we
shouldn't merge anything anymore and we can stop there?

It's not black and white, it should be managed, and here it's
manageable and / or mitigated.

> >> With option 4, I guess you meant display sink drivers, i.e., panel and
> >> bridge drivers, wouldn't be affected. Then, display source drivers, i.e.,
> >> display controller and bridge drivers, would be affected. This adds
> >> burdens for driver developers/maintainers(though almost no effort from
> >> DT's PoV), doesn't it?
> > 
> > Not necessarily, panels have a phandle to the parent endpoint too so
> > they can do that walk and configure their format if it's any easier.
> 
> I'm sorry, I don't get your meaning here.  I have no idea how to support
> this new property in endpoint-base(graph.yaml) or video-interfaces.yaml
> _without_ changing existing display source drivers.

Make a helper, and drivers can always call that helper. It will be a
single function call, and panel-simple will cover most of the existing
user-base already.

Otherwise, we have the option to use coccinelle if you want to mass
convert it.

> >> Moreover, why it has to be the display sink drivers which are not affected?
> >> DT writers might choose to set the format at the sink endpoint, e.g., setting
> >> RGB666 at the sink endpoint of a RGB888 DPI panel or bridge.
> > 
> > Why wouldn't you run the panel at the highest bpc possible?
> 
> Because hardware designers route less data signals than regular ones to the
> DPI panel or bridge.

Sure, and that's a hardware property, that's what your bridge or that
property would cover. It wouldn't cover why someone would want to change
*that*, or I didn't underdstand what you meant?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]