[PATCH v4 0/6] drm: exynos: dsi: Convert drm bridge

[PATCH v4 0/6] drm: exynos: dsi: Convert drm bridge

[Jagan Teki]

Updated series about drm bridge conversion of exynos dsi.
Previous version can be accessible, here [1].

Patch 1: connector reset

Patch 2: panel_bridge API

Patch 3: Bridge conversion

Patch 4: Atomic functions

Patch 5: atomic_set

Patch 6: DSI init in enable

Apply below patches to test on Exynos DSI:
- https://patchwork.amarulasolutions.com/patch/1825/
- https://patchwork.amarulasolutions.com/patch/1823/

[1] https://patchwork.kernel.org/project/dri-devel/cover/20211212181416.3312656-1-jagan@amarulasolutions.com/

Any inputs?
Jagan.

Jagan Teki (6):
  drm: bridge: panel: Reset the connector state pointer
  drm: exynos: dsi: Use drm panel_bridge API
  drm: exynos: dsi: Convert to bridge driver
  drm: exynos: dsi: Switch to atomic funcs
  drm: exynos: dsi: Get the mode from bridge
  drm: exynos: dsi: Move DSI init in bridge enable

 drivers/gpu/drm/bridge/panel.c          |   3 +
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 244 ++++++++----------------
 2 files changed, 80 insertions(+), 167 deletions(-)

-- 
2.25.1

[PATCH v4 1/6] drm: bridge: panel: Reset the connector state pointer

[Jagan Teki]

Trigger hotplug event with drm_kms_helper_hotplug_event might fail if the
connector state pointer is NULL.

BUG observed in exynos dsi driver where drm_bridge_attach is trying to register
a connector in panel_bridge before the hotplug event is triggered,

WARNING: CPU: 1 PID: 1 at drivers/gpu/drm/drm_atomic_state_helper.c:494 drm_atomic_helper_connector_duplicate_state+0x94/0x9c
Modules linked in:
CPU: 1 PID: 1 Comm: swapper/0 Tainted: G W 5.16.0-rc1-00009-g704b1dbfa4c2 #11058
Hardware name: Samsung Exynos (Flattened Device Tree)
[<c0110b30>] (unwind_backtrace) from [<c010c618>] (show_stack+0x10/0x14)
[<c010c618>] (show_stack) from [<c0b657d4>] (dump_stack_lvl+0x58/0x70)
[<c0b657d4>] (dump_stack_lvl) from [<c01261dc>] (__warn+0xd0/0x134)
[<c01261dc>] (__warn) from [<c0b5f628>] (warn_slowpath_fmt+0x5c/0xb4)
[<c0b5f628>] (warn_slowpath_fmt) from [<c064bce4>] (drm_atomic_helper_connector_duplicate_state+0x94/0x9c)
[<c064bce4>] (drm_atomic_helper_connector_duplicate_state) from [<c0666b64>] (drm_atomic_get_connector_state+0xd4/0x190)
[<c0666b64>] (drm_atomic_get_connector_state) from [<c0667928>] (__drm_atomic_helper_set_config+0x314/0x368)
[<c0667928>] (__drm_atomic_helper_set_config) from [<c067e628>] (drm_client_modeset_commit_atomic+0x170/0x278)
[<c067e628>] (drm_client_modeset_commit_atomic) from [<c067e800>] (drm_client_modeset_commit_locked+0x60/0x1c8)
[<c067e800>] (drm_client_modeset_commit_locked) from [<c067e98c>] (drm_client_modeset_commit+0x24/0x40)
[<c067e98c>] (drm_client_modeset_commit) from [<c06509c0>] (drm_fb_helper_set_par+0xb8/0xf8)
[<c06509c0>] (drm_fb_helper_set_par) from [<c05b86d0>] (fbcon_init+0x2c0/0x518)
[<c05b86d0>] (fbcon_init) from [<c060636c>] (visual_init+0xc0/0x108)
[<c060636c>] (visual_init) from [<c06085e4>] (do_bind_con_driver+0x1b8/0x3a4)
[<c06085e4>] (do_bind_con_driver) from [<c0608b40>] (do_take_over_console+0x13c/0x1e8)
[<c0608b40>] (do_take_over_console) from [<c05b6854>] (do_fbcon_takeover+0x78/0xd8)
[<c05b6854>] (do_fbcon_takeover) from [<c05b1154>] (register_framebuffer+0x208/0x2e0)
[<c05b1154>] (register_framebuffer) from [<c064ead0>] (__drm_fb_helper_initial_config_and_unlock+0x400/0x63c)
[<c064ead0>] (__drm_fb_helper_initial_config_and_unlock) from [<c063a718>] (drm_kms_helper_hotplug_event+0x24/0x30)
[<c063a718>] (drm_kms_helper_hotplug_event) from [<c068f668>] (exynos_dsi_host_attach+0x174/0x1fc)
[<c068f668>] (exynos_dsi_host_attach) from [<c0699354>] (s6e8aa0_probe+0x1b4/0x218)

So reset the atomic state for a given connector by freeing the state pointer
and allocate a new empty state object. This can be done using connector
funcs->reset helper and has to be done before the hotplug even calls.

This patch calls the connector->funcs->reset in panel_bridge_attach.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v4:
- new patch

 drivers/gpu/drm/bridge/panel.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index b32295abd9e7..f6eea194482a 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -83,6 +83,9 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
 	drm_connector_attach_encoder(&panel_bridge->connector,
 					  bridge->encoder);
 
+	if (connector->funcs->reset)
+		connector->funcs->reset(connector);
+
 	return 0;
 }
 
-- 
2.25.1

[PATCH v4 2/6] drm: exynos: dsi: Use drm panel_bridge API

[Jagan Teki]

Replace the manual panel handling code by a drm panel_bridge via
devm_drm_of_get_bridge().

Adding panel_bridge handling,

- Drops drm_connector and related operations as drm_bridge_attach
  creates connector during attachment.

- Drops panel pointer and iterate the bridge, so-that it can operate
  the normal bridge and panel_bridge in constitutive callbacks.

This simplifies the driver and allows all components in the display
pipeline to be treated as bridges.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v4:
- drop unneeded headers
Changes for v3:
- fix port number
- add print for attached device
Changes for v2:
- new patch

 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 167 ++++--------------------
 1 file changed, 28 insertions(+), 139 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 8d137857818c..14e16f2d594b 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -25,9 +25,7 @@
 
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
-#include <drm/drm_fb_helper.h>
 #include <drm/drm_mipi_dsi.h>
-#include <drm/drm_panel.h>
 #include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_simple_kms_helper.h>
@@ -221,6 +219,11 @@ enum exynos_dsi_transfer_type {
 	EXYNOS_DSI_RX,
 };
 
+enum {
+	DSI_PORT_IN,
+	DSI_PORT_OUT
+};
+
 struct exynos_dsi_transfer {
 	struct list_head list;
 	struct completion completed;
@@ -254,8 +257,6 @@ struct exynos_dsi_driver_data {
 struct exynos_dsi {
 	struct drm_encoder encoder;
 	struct mipi_dsi_host dsi_host;
-	struct drm_connector connector;
-	struct drm_panel *panel;
 	struct list_head bridge_chain;
 	struct drm_bridge *out_bridge;
 	struct device *dev;
@@ -285,7 +286,6 @@ struct exynos_dsi {
 };
 
 #define host_to_dsi(host) container_of(host, struct exynos_dsi, dsi_host)
-#define connector_to_dsi(c) container_of(c, struct exynos_dsi, connector)
 
 static inline struct exynos_dsi *encoder_to_dsi(struct drm_encoder *e)
 {
@@ -1391,42 +1391,21 @@ static void exynos_dsi_enable(struct drm_encoder *encoder)
 
 	dsi->state |= DSIM_STATE_ENABLED;
 
-	if (dsi->panel) {
-		ret = drm_panel_prepare(dsi->panel);
-		if (ret < 0)
-			goto err_put_sync;
-	} else {
-		list_for_each_entry_reverse(iter, &dsi->bridge_chain,
-					    chain_node) {
-			if (iter->funcs->pre_enable)
-				iter->funcs->pre_enable(iter);
-		}
+	list_for_each_entry_reverse(iter, &dsi->bridge_chain, chain_node) {
+		if (iter->funcs->pre_enable)
+			iter->funcs->pre_enable(iter);
 	}
 
 	exynos_dsi_set_display_mode(dsi);
 	exynos_dsi_set_display_enable(dsi, true);
 
-	if (dsi->panel) {
-		ret = drm_panel_enable(dsi->panel);
-		if (ret < 0)
-			goto err_display_disable;
-	} else {
-		list_for_each_entry(iter, &dsi->bridge_chain, chain_node) {
-			if (iter->funcs->enable)
-				iter->funcs->enable(iter);
-		}
+	list_for_each_entry(iter, &dsi->bridge_chain, chain_node) {
+		if (iter->funcs->enable)
+			iter->funcs->enable(iter);
 	}
 
 	dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
 	return;
-
-err_display_disable:
-	exynos_dsi_set_display_enable(dsi, false);
-	drm_panel_unprepare(dsi->panel);
-
-err_put_sync:
-	dsi->state &= ~DSIM_STATE_ENABLED;
-	pm_runtime_put(dsi->dev);
 }
 
 static void exynos_dsi_disable(struct drm_encoder *encoder)
@@ -1439,15 +1418,12 @@ static void exynos_dsi_disable(struct drm_encoder *encoder)
 
 	dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
 
-	drm_panel_disable(dsi->panel);
-
 	list_for_each_entry_reverse(iter, &dsi->bridge_chain, chain_node) {
 		if (iter->funcs->disable)
 			iter->funcs->disable(iter);
 	}
 
 	exynos_dsi_set_display_enable(dsi, false);
-	drm_panel_unprepare(dsi->panel);
 
 	list_for_each_entry(iter, &dsi->bridge_chain, chain_node) {
 		if (iter->funcs->post_disable)
@@ -1458,70 +1434,6 @@ static void exynos_dsi_disable(struct drm_encoder *encoder)
 	pm_runtime_put_sync(dsi->dev);
 }
 
-static enum drm_connector_status
-exynos_dsi_detect(struct drm_connector *connector, bool force)
-{
-	return connector->status;
-}
-
-static void exynos_dsi_connector_destroy(struct drm_connector *connector)
-{
-	drm_connector_unregister(connector);
-	drm_connector_cleanup(connector);
-	connector->dev = NULL;
-}
-
-static const struct drm_connector_funcs exynos_dsi_connector_funcs = {
-	.detect = exynos_dsi_detect,
-	.fill_modes = drm_helper_probe_single_connector_modes,
-	.destroy = exynos_dsi_connector_destroy,
-	.reset = drm_atomic_helper_connector_reset,
-	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-};
-
-static int exynos_dsi_get_modes(struct drm_connector *connector)
-{
-	struct exynos_dsi *dsi = connector_to_dsi(connector);
-
-	if (dsi->panel)
-		return drm_panel_get_modes(dsi->panel, connector);
-
-	return 0;
-}
-
-static const struct drm_connector_helper_funcs exynos_dsi_connector_helper_funcs = {
-	.get_modes = exynos_dsi_get_modes,
-};
-
-static int exynos_dsi_create_connector(struct drm_encoder *encoder)
-{
-	struct exynos_dsi *dsi = encoder_to_dsi(encoder);
-	struct drm_connector *connector = &dsi->connector;
-	struct drm_device *drm = encoder->dev;
-	int ret;
-
-	connector->polled = DRM_CONNECTOR_POLL_HPD;
-
-	ret = drm_connector_init(drm, connector, &exynos_dsi_connector_funcs,
-				 DRM_MODE_CONNECTOR_DSI);
-	if (ret) {
-		DRM_DEV_ERROR(dsi->dev,
-			      "Failed to initialize connector with drm\n");
-		return ret;
-	}
-
-	connector->status = connector_status_disconnected;
-	drm_connector_helper_add(connector, &exynos_dsi_connector_helper_funcs);
-	drm_connector_attach_encoder(connector, encoder);
-	if (!drm->registered)
-		return 0;
-
-	connector->funcs->reset(connector);
-	drm_connector_register(connector);
-	return 0;
-}
-
 static const struct drm_encoder_helper_funcs exynos_dsi_encoder_helper_funcs = {
 	.enable = exynos_dsi_enable,
 	.disable = exynos_dsi_disable,
@@ -1533,33 +1445,23 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
 				  struct mipi_dsi_device *device)
 {
 	struct exynos_dsi *dsi = host_to_dsi(host);
+	struct device *dev = dsi->dev;
 	struct drm_encoder *encoder = &dsi->encoder;
 	struct drm_device *drm = encoder->dev;
-	struct drm_bridge *out_bridge;
-
-	out_bridge  = of_drm_find_bridge(device->dev.of_node);
-	if (out_bridge) {
-		drm_bridge_attach(encoder, out_bridge, NULL, 0);
-		dsi->out_bridge = out_bridge;
-		list_splice_init(&encoder->bridge_chain, &dsi->bridge_chain);
-	} else {
-		int ret = exynos_dsi_create_connector(encoder);
-
-		if (ret) {
-			DRM_DEV_ERROR(dsi->dev,
-				      "failed to create connector ret = %d\n",
-				      ret);
-			drm_encoder_cleanup(encoder);
-			return ret;
-		}
+	int ret;
 
-		dsi->panel = of_drm_find_panel(device->dev.of_node);
-		if (IS_ERR(dsi->panel))
-			dsi->panel = NULL;
-		else
-			dsi->connector.status = connector_status_connected;
+	dsi->out_bridge = devm_drm_of_get_bridge(dev, dev->of_node, DSI_PORT_OUT, 0);
+	if (IS_ERR(dsi->out_bridge)) {
+		ret = PTR_ERR(dsi->out_bridge);
+		DRM_DEV_ERROR(dev, "failed to find the bridge: %d\n", ret);
+		return ret;
 	}
 
+	DRM_DEV_INFO(dev, "Attached %s device\n", device->name);
+
+	drm_bridge_attach(encoder, dsi->out_bridge, NULL, 0);
+	list_splice_init(&encoder->bridge_chain, &dsi->bridge_chain);
+
 	/*
 	 * This is a temporary solution and should be made by more generic way.
 	 *
@@ -1567,7 +1469,7 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
 	 * TE interrupt handler.
 	 */
 	if (!(device->mode_flags & MIPI_DSI_MODE_VIDEO)) {
-		int ret = exynos_dsi_register_te_irq(dsi, &device->dev);
+		ret = exynos_dsi_register_te_irq(dsi, &device->dev);
 		if (ret)
 			return ret;
 	}
@@ -1594,18 +1496,10 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
 	struct exynos_dsi *dsi = host_to_dsi(host);
 	struct drm_device *drm = dsi->encoder.dev;
 
-	if (dsi->panel) {
-		mutex_lock(&drm->mode_config.mutex);
-		exynos_dsi_disable(&dsi->encoder);
-		dsi->panel = NULL;
-		dsi->connector.status = connector_status_disconnected;
-		mutex_unlock(&drm->mode_config.mutex);
-	} else {
-		if (dsi->out_bridge->funcs->detach)
-			dsi->out_bridge->funcs->detach(dsi->out_bridge);
-		dsi->out_bridge = NULL;
-		INIT_LIST_HEAD(&dsi->bridge_chain);
-	}
+	if (dsi->out_bridge->funcs->detach)
+		dsi->out_bridge->funcs->detach(dsi->out_bridge);
+	dsi->out_bridge = NULL;
+	INIT_LIST_HEAD(&dsi->bridge_chain);
 
 	if (drm->mode_config.poll_enabled)
 		drm_kms_helper_hotplug_event(drm);
@@ -1661,11 +1555,6 @@ static int exynos_dsi_of_read_u32(const struct device_node *np,
 	return ret;
 }
 
-enum {
-	DSI_PORT_IN,
-	DSI_PORT_OUT
-};
-
 static int exynos_dsi_parse_dt(struct exynos_dsi *dsi)
 {
 	struct device *dev = dsi->dev;
-- 
2.25.1

[PATCH v4 3/6] drm: exynos: dsi: Convert to bridge driver

[Jagan Teki]

Convert the encoders to bridge drivers in order to standardize on
a single API with built-in dumb encoder support for compatibility
with existing component drivers.

Driver bridge conversion will help to reuse the same bridge on
different platforms as exynos dsi driver can be used as a Samsung
DSIM and use it for i.MX8MM platform.

Bridge conversion,

- Drops drm_encoder_helper_funcs.

- Adds drm_bridge_funcs and register a drm bridge.

- Drops bridge_chain.

- Separate pre_enable from enable function.

- Separate post_disable from disable function.

Convert it.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v4:
- add pre_enable function
- add post_disable function
Changes for v3:
- move bridge add in host_attach
- move bridge remove in host_detach
- use flags, bridge in drm_bridge_attach in attch 
Changes for v2:
- drop bridge_chain

 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 85 +++++++++++++------------
 1 file changed, 44 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 14e16f2d594b..29c68379e6be 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -257,7 +257,7 @@ struct exynos_dsi_driver_data {
 struct exynos_dsi {
 	struct drm_encoder encoder;
 	struct mipi_dsi_host dsi_host;
-	struct list_head bridge_chain;
+	struct drm_bridge bridge;
 	struct drm_bridge *out_bridge;
 	struct device *dev;
 
@@ -287,9 +287,9 @@ struct exynos_dsi {
 
 #define host_to_dsi(host) container_of(host, struct exynos_dsi, dsi_host)
 
-static inline struct exynos_dsi *encoder_to_dsi(struct drm_encoder *e)
+static inline struct exynos_dsi *bridge_to_dsi(struct drm_bridge *b)
 {
-	return container_of(e, struct exynos_dsi, encoder);
+	return container_of(b, struct exynos_dsi, bridge);
 }
 
 enum reg_idx {
@@ -880,9 +880,10 @@ static int exynos_dsi_init_link(struct exynos_dsi *dsi)
 	return 0;
 }
 
-static void exynos_dsi_set_display_mode(struct exynos_dsi *dsi)
+static void exynos_dsi_set_display_mode(struct drm_bridge *bridge)
 {
-	struct drm_display_mode *m = &dsi->encoder.crtc->state->adjusted_mode;
+	struct exynos_dsi *dsi = bridge_to_dsi(bridge);
+	struct drm_display_mode *m = &bridge->encoder->crtc->state->adjusted_mode;
 	unsigned int num_bits_resol = dsi->driver_data->num_bits_resol;
 	u32 reg;
 
@@ -1374,10 +1375,9 @@ static void exynos_dsi_unregister_te_irq(struct exynos_dsi *dsi)
 	}
 }
 
-static void exynos_dsi_enable(struct drm_encoder *encoder)
+static void exynos_dsi_pre_enable(struct drm_bridge *bridge)
 {
-	struct exynos_dsi *dsi = encoder_to_dsi(encoder);
-	struct drm_bridge *iter;
+	struct exynos_dsi *dsi = bridge_to_dsi(bridge);
 	int ret;
 
 	if (dsi->state & DSIM_STATE_ENABLED)
@@ -1390,53 +1390,54 @@ static void exynos_dsi_enable(struct drm_encoder *encoder)
 	}
 
 	dsi->state |= DSIM_STATE_ENABLED;
+}
 
-	list_for_each_entry_reverse(iter, &dsi->bridge_chain, chain_node) {
-		if (iter->funcs->pre_enable)
-			iter->funcs->pre_enable(iter);
-	}
+static void exynos_dsi_enable(struct drm_bridge *bridge)
+{
+	struct exynos_dsi *dsi = bridge_to_dsi(bridge);
 
-	exynos_dsi_set_display_mode(dsi);
+	exynos_dsi_set_display_mode(bridge);
 	exynos_dsi_set_display_enable(dsi, true);
 
-	list_for_each_entry(iter, &dsi->bridge_chain, chain_node) {
-		if (iter->funcs->enable)
-			iter->funcs->enable(iter);
-	}
-
 	dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
+
 	return;
 }
 
-static void exynos_dsi_disable(struct drm_encoder *encoder)
+static void exynos_dsi_disable(struct drm_bridge *bridge)
 {
-	struct exynos_dsi *dsi = encoder_to_dsi(encoder);
-	struct drm_bridge *iter;
+	struct exynos_dsi *dsi = bridge_to_dsi(bridge);
 
 	if (!(dsi->state & DSIM_STATE_ENABLED))
 		return;
 
 	dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
+}
 
-	list_for_each_entry_reverse(iter, &dsi->bridge_chain, chain_node) {
-		if (iter->funcs->disable)
-			iter->funcs->disable(iter);
-	}
+static void exynos_dsi_post_disable(struct drm_bridge *bridge)
+{
+	struct exynos_dsi *dsi = bridge_to_dsi(bridge);
 
 	exynos_dsi_set_display_enable(dsi, false);
 
-	list_for_each_entry(iter, &dsi->bridge_chain, chain_node) {
-		if (iter->funcs->post_disable)
-			iter->funcs->post_disable(iter);
-	}
-
 	dsi->state &= ~DSIM_STATE_ENABLED;
 	pm_runtime_put_sync(dsi->dev);
 }
 
-static const struct drm_encoder_helper_funcs exynos_dsi_encoder_helper_funcs = {
-	.enable = exynos_dsi_enable,
-	.disable = exynos_dsi_disable,
+static int exynos_dsi_attach(struct drm_bridge *bridge,
+			     enum drm_bridge_attach_flags flags)
+{
+	struct exynos_dsi *dsi = bridge_to_dsi(bridge);
+
+	return drm_bridge_attach(bridge->encoder, dsi->out_bridge, NULL, flags);
+}
+
+static const struct drm_bridge_funcs exynos_dsi_bridge_funcs = {
+	.pre_enable			= exynos_dsi_pre_enable,
+	.enable				= exynos_dsi_enable,
+	.disable			= exynos_dsi_disable,
+	.post_disable			= exynos_dsi_post_disable,
+	.attach				= exynos_dsi_attach,
 };
 
 MODULE_DEVICE_TABLE(of, exynos_dsi_of_match);
@@ -1459,8 +1460,9 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
 
 	DRM_DEV_INFO(dev, "Attached %s device\n", device->name);
 
-	drm_bridge_attach(encoder, dsi->out_bridge, NULL, 0);
-	list_splice_init(&encoder->bridge_chain, &dsi->bridge_chain);
+	drm_bridge_add(&dsi->bridge);
+
+	drm_bridge_attach(encoder, &dsi->bridge, NULL, 0);
 
 	/*
 	 * This is a temporary solution and should be made by more generic way.
@@ -1499,13 +1501,14 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
 	if (dsi->out_bridge->funcs->detach)
 		dsi->out_bridge->funcs->detach(dsi->out_bridge);
 	dsi->out_bridge = NULL;
-	INIT_LIST_HEAD(&dsi->bridge_chain);
 
 	if (drm->mode_config.poll_enabled)
 		drm_kms_helper_hotplug_event(drm);
 
 	exynos_dsi_unregister_te_irq(dsi);
 
+	drm_bridge_remove(&dsi->bridge);
+
 	return 0;
 }
 
@@ -1591,8 +1594,6 @@ static int exynos_dsi_bind(struct device *dev, struct device *master,
 
 	drm_simple_encoder_init(drm_dev, encoder, DRM_MODE_ENCODER_TMDS);
 
-	drm_encoder_helper_add(encoder, &exynos_dsi_encoder_helper_funcs);
-
 	ret = exynos_drm_set_possible_crtcs(encoder, EXYNOS_DISPLAY_TYPE_LCD);
 	if (ret < 0)
 		return ret;
@@ -1612,9 +1613,8 @@ static void exynos_dsi_unbind(struct device *dev, struct device *master,
 				void *data)
 {
 	struct exynos_dsi *dsi = dev_get_drvdata(dev);
-	struct drm_encoder *encoder = &dsi->encoder;
 
-	exynos_dsi_disable(encoder);
+	exynos_dsi_disable(&dsi->bridge);
 
 	mipi_dsi_host_unregister(&dsi->dsi_host);
 }
@@ -1640,7 +1640,6 @@ static int exynos_dsi_probe(struct platform_device *pdev)
 	init_completion(&dsi->completed);
 	spin_lock_init(&dsi->transfer_lock);
 	INIT_LIST_HEAD(&dsi->transfer_list);
-	INIT_LIST_HEAD(&dsi->bridge_chain);
 
 	dsi->dsi_host.ops = &exynos_dsi_ops;
 	dsi->dsi_host.dev = dev;
@@ -1708,6 +1707,10 @@ static int exynos_dsi_probe(struct platform_device *pdev)
 
 	pm_runtime_enable(dev);
 
+	dsi->bridge.funcs = &exynos_dsi_bridge_funcs;
+	dsi->bridge.of_node = dev->of_node;
+	dsi->bridge.type = DRM_MODE_CONNECTOR_DSI;
+
 	ret = component_add(dev, &exynos_dsi_component_ops);
 	if (ret)
 		goto err_disable_runtime;
-- 
2.25.1

[PATCH v4 4/6] drm: exynos: dsi: Switch to atomic funcs

[Jagan Teki]

The new support drm bridges are moving towards atomic functions.

Replace atomic version of functions to continue the transition
to the atomic API.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v4, v3:
- none

 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 29c68379e6be..983e7cb65e2a 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -1375,7 +1375,8 @@ static void exynos_dsi_unregister_te_irq(struct exynos_dsi *dsi)
 	}
 }
 
-static void exynos_dsi_pre_enable(struct drm_bridge *bridge)
+static void exynos_dsi_atomic_pre_enable(struct drm_bridge *bridge,
+					 struct drm_bridge_state *old_bridge_state)
 {
 	struct exynos_dsi *dsi = bridge_to_dsi(bridge);
 	int ret;
@@ -1392,7 +1393,8 @@ static void exynos_dsi_pre_enable(struct drm_bridge *bridge)
 	dsi->state |= DSIM_STATE_ENABLED;
 }
 
-static void exynos_dsi_enable(struct drm_bridge *bridge)
+static void exynos_dsi_atomic_enable(struct drm_bridge *bridge,
+				    struct drm_bridge_state *old_bridge_state)
 {
 	struct exynos_dsi *dsi = bridge_to_dsi(bridge);
 
@@ -1404,7 +1406,8 @@ static void exynos_dsi_enable(struct drm_bridge *bridge)
 	return;
 }
 
-static void exynos_dsi_disable(struct drm_bridge *bridge)
+static void exynos_dsi_atomic_disable(struct drm_bridge *bridge,
+				      struct drm_bridge_state *old_bridge_state)
 {
 	struct exynos_dsi *dsi = bridge_to_dsi(bridge);
 
@@ -1414,7 +1417,8 @@ static void exynos_dsi_disable(struct drm_bridge *bridge)
 	dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
 }
 
-static void exynos_dsi_post_disable(struct drm_bridge *bridge)
+static void exynos_dsi_atomic_post_disable(struct drm_bridge *bridge,
+					   struct drm_bridge_state *old_bridge_state)
 {
 	struct exynos_dsi *dsi = bridge_to_dsi(bridge);
 
@@ -1433,10 +1437,13 @@ static int exynos_dsi_attach(struct drm_bridge *bridge,
 }
 
 static const struct drm_bridge_funcs exynos_dsi_bridge_funcs = {
-	.pre_enable			= exynos_dsi_pre_enable,
-	.enable				= exynos_dsi_enable,
-	.disable			= exynos_dsi_disable,
-	.post_disable			= exynos_dsi_post_disable,
+	.atomic_duplicate_state		= drm_atomic_helper_bridge_duplicate_state,
+	.atomic_destroy_state		= drm_atomic_helper_bridge_destroy_state,
+	.atomic_reset			= drm_atomic_helper_bridge_reset,
+	.atomic_pre_enable		= exynos_dsi_atomic_pre_enable,
+	.atomic_enable			= exynos_dsi_atomic_enable,
+	.atomic_disable			= exynos_dsi_atomic_disable,
+	.atomic_post_disable		= exynos_dsi_atomic_post_disable,
 	.attach				= exynos_dsi_attach,
 };
 
@@ -1614,7 +1621,7 @@ static void exynos_dsi_unbind(struct device *dev, struct device *master,
 {
 	struct exynos_dsi *dsi = dev_get_drvdata(dev);
 
-	exynos_dsi_disable(&dsi->bridge);
+	exynos_dsi_atomic_disable(&dsi->bridge, NULL);
 
 	mipi_dsi_host_unregister(&dsi->dsi_host);
 }
-- 
2.25.1

[PATCH v4 5/6] drm: exynos: dsi: Get the mode from bridge

[Jagan Teki]

Now the exynos dsi driver is fully aware of bridge handling,
so get the display mode from bridge, mode_set API instead of
legacy encoder crtc.

This makes bridge usage more efficient instead of relying on
encoder stack.

Add mode_set in drm_bridge_funcs.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v4, v3:
- none

 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 983e7cb65e2a..774ca265ed3b 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -276,6 +276,7 @@ struct exynos_dsi {
 	u32 format;
 
 	int state;
+	struct drm_display_mode mode;
 	struct drm_property *brightness;
 	struct completion completed;
 
@@ -880,10 +881,9 @@ static int exynos_dsi_init_link(struct exynos_dsi *dsi)
 	return 0;
 }
 
-static void exynos_dsi_set_display_mode(struct drm_bridge *bridge)
+static void exynos_dsi_set_display_mode(struct exynos_dsi *dsi)
 {
-	struct exynos_dsi *dsi = bridge_to_dsi(bridge);
-	struct drm_display_mode *m = &bridge->encoder->crtc->state->adjusted_mode;
+	struct drm_display_mode *m = &dsi->mode;
 	unsigned int num_bits_resol = dsi->driver_data->num_bits_resol;
 	u32 reg;
 
@@ -1398,7 +1398,7 @@ static void exynos_dsi_atomic_enable(struct drm_bridge *bridge,
 {
 	struct exynos_dsi *dsi = bridge_to_dsi(bridge);
 
-	exynos_dsi_set_display_mode(bridge);
+	exynos_dsi_set_display_mode(dsi);
 	exynos_dsi_set_display_enable(dsi, true);
 
 	dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
@@ -1428,6 +1428,15 @@ static void exynos_dsi_atomic_post_disable(struct drm_bridge *bridge,
 	pm_runtime_put_sync(dsi->dev);
 }
 
+static void exynos_dsi_mode_set(struct drm_bridge *bridge,
+				const struct drm_display_mode *mode,
+				const struct drm_display_mode *adjusted_mode)
+{
+	struct exynos_dsi *dsi = bridge_to_dsi(bridge);
+
+	drm_mode_copy(&dsi->mode, adjusted_mode);
+}
+
 static int exynos_dsi_attach(struct drm_bridge *bridge,
 			     enum drm_bridge_attach_flags flags)
 {
@@ -1444,6 +1453,7 @@ static const struct drm_bridge_funcs exynos_dsi_bridge_funcs = {
 	.atomic_enable			= exynos_dsi_atomic_enable,
 	.atomic_disable			= exynos_dsi_atomic_disable,
 	.atomic_post_disable		= exynos_dsi_atomic_post_disable,
+	.mode_set			= exynos_dsi_mode_set,
 	.attach				= exynos_dsi_attach,
 };
 
-- 
2.25.1

[PATCH v4 6/6] drm: exynos: dsi: Move DSI init in bridge enable

[Jagan Teki]

Host transfer in DSI master will invoke only when the DSI commands
sent from DSI devices like DSI Panel or DSI bridges and this host
transfer wouldn't invoke I2C based DSI bridge drivers.

Handling DSI host initialization in transfer calls might miss the
controller setup for I2C configured DSI bridges.

So, move the DSI initialization from transfer to bridge enable as
the bridge enable API as it is common across all classes of DSI
device drivers.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v4:
- none
Changes for v3:
- new patch

 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 774ca265ed3b..d853dd8d1271 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -1397,6 +1397,14 @@ static void exynos_dsi_atomic_enable(struct drm_bridge *bridge,
 				    struct drm_bridge_state *old_bridge_state)
 {
 	struct exynos_dsi *dsi = bridge_to_dsi(bridge);
+	int ret;
+
+	if (!(dsi->state & DSIM_STATE_INITIALIZED)) {
+		ret = exynos_dsi_init(dsi);
+		if (ret)
+			return;
+		dsi->state |= DSIM_STATE_INITIALIZED;
+	}
 
 	exynos_dsi_set_display_mode(dsi);
 	exynos_dsi_set_display_enable(dsi, true);
@@ -1539,13 +1547,6 @@ static ssize_t exynos_dsi_host_transfer(struct mipi_dsi_host *host,
 	if (!(dsi->state & DSIM_STATE_ENABLED))
 		return -EINVAL;
 
-	if (!(dsi->state & DSIM_STATE_INITIALIZED)) {
-		ret = exynos_dsi_init(dsi);
-		if (ret)
-			return ret;
-		dsi->state |= DSIM_STATE_INITIALIZED;
-	}
-
 	ret = mipi_dsi_create_packet(&xfer.packet, msg);
 	if (ret < 0)
 		return ret;
-- 
2.25.1

Re: [PATCH v4 0/6] drm: exynos: dsi: Convert drm bridge

[Marek Szyprowski]

Hi Jagan,

On 15.12.2021 11:15, Jagan Teki wrote:
> Updated series about drm bridge conversion of exynos dsi.
> Previous version can be accessible, here [1].
>
> Patch 1: connector reset
>
> Patch 2: panel_bridge API
>
> Patch 3: Bridge conversion
>
> Patch 4: Atomic functions
>
> Patch 5: atomic_set
>
> Patch 6: DSI init in enable

There is a little progress! :)

Devices with a simple display pipeline (only a DSI panel, like 
Trats/Trats2) works till the last patch. Then, after applying ("[PATCH 
v4 6/6] drm: exynos: dsi: Move DSI init in bridge enable"), I get no 
display at all.

A TM2e board with in-bridge (Exynos MIC) stops displaying anything after 
applying patch "[PATCH v4 2/6] drm: exynos: dsi: Use drm panel_bridge API".

In case of the Arndale board with tc358764 bridge, no much progress. The 
display is broken just after applying the "[PATCH v2] drm: bridge: 
tc358764: Use drm panel_bridge API" patch on top of linux-next.

In all cases the I had "drm: of: Lookup if child node has panel or 
bridge" patch applied.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v4 0/6] drm: exynos: dsi: Convert drm bridge

[Jagan Teki]

Hi Marek,

On Wed, Dec 15, 2021 at 5:31 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi Jagan,
>
> On 15.12.2021 11:15, Jagan Teki wrote:
> > Updated series about drm bridge conversion of exynos dsi.
> > Previous version can be accessible, here [1].
> >
> > Patch 1: connector reset
> >
> > Patch 2: panel_bridge API
> >
> > Patch 3: Bridge conversion
> >
> > Patch 4: Atomic functions
> >
> > Patch 5: atomic_set
> >
> > Patch 6: DSI init in enable
>
> There is a little progress! :)
>
> Devices with a simple display pipeline (only a DSI panel, like
> Trats/Trats2) works till the last patch. Then, after applying ("[PATCH
> v4 6/6] drm: exynos: dsi: Move DSI init in bridge enable"), I get no
> display at all.
>
> A TM2e board with in-bridge (Exynos MIC) stops displaying anything after
> applying patch "[PATCH v4 2/6] drm: exynos: dsi: Use drm panel_bridge API".
>
> In case of the Arndale board with tc358764 bridge, no much progress. The
> display is broken just after applying the "[PATCH v2] drm: bridge:
> tc358764: Use drm panel_bridge API" patch on top of linux-next.
>
> In all cases the I had "drm: of: Lookup if child node has panel or
> bridge" patch applied.

Just skip the 6/6 for now.

Apply
- https://patchwork.amarulasolutions.com/patch/1825/
- https://patchwork.amarulasolutions.com/patch/1823/

Then apply 1/6 to 5/6.  and update the status?

Thanks,
Jagan.

Re: [PATCH v4 0/6] drm: exynos: dsi: Convert drm bridge

[Marek Szyprowski]

Hi Jagan,

On 15.12.2021 13:57, Jagan Teki wrote:
> On Wed, Dec 15, 2021 at 5:31 PM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> On 15.12.2021 11:15, Jagan Teki wrote:
>>> Updated series about drm bridge conversion of exynos dsi.
>>> Previous version can be accessible, here [1].
>>>
>>> Patch 1: connector reset
>>>
>>> Patch 2: panel_bridge API
>>>
>>> Patch 3: Bridge conversion
>>>
>>> Patch 4: Atomic functions
>>>
>>> Patch 5: atomic_set
>>>
>>> Patch 6: DSI init in enable
>> There is a little progress! :)
>>
>> Devices with a simple display pipeline (only a DSI panel, like
>> Trats/Trats2) works till the last patch. Then, after applying ("[PATCH
>> v4 6/6] drm: exynos: dsi: Move DSI init in bridge enable"), I get no
>> display at all.
>>
>> A TM2e board with in-bridge (Exynos MIC) stops displaying anything after
>> applying patch "[PATCH v4 2/6] drm: exynos: dsi: Use drm panel_bridge API".
>>
>> In case of the Arndale board with tc358764 bridge, no much progress. The
>> display is broken just after applying the "[PATCH v2] drm: bridge:
>> tc358764: Use drm panel_bridge API" patch on top of linux-next.
>>
>> In all cases the I had "drm: of: Lookup if child node has panel or
>> bridge" patch applied.
> Just skip the 6/6 for now.
>
> Apply
> - https://protect2.fireeye.com/v1/url?k=a24f3f76-fdd40659-a24eb439-0cc47a31cdf8-97ea12b4c5258d11&q=1&e=37a169bf-7ca5-4362-aad7-486018c7a708&u=https%3A%2F%2Fpatchwork.amarulasolutions.com%2Fpatch%2F1825%2F
> - https://protect2.fireeye.com/v1/url?k=a226360f-fdbd0f20-a227bd40-0cc47a31cdf8-ebd66aebee1058d7&q=1&e=37a169bf-7ca5-4362-aad7-486018c7a708&u=https%3A%2F%2Fpatchwork.amarulasolutions.com%2Fpatch%2F1823%2F
>
> Then apply 1/6 to 5/6.  and update the status?

Okay, my fault, I didn't check that case on Arndale.

I've checked and indeed, Trats/Trats2 and Arndale works after the above 
2 patches AND patches 1-5.

The only problem is now on TM2e, which uses Exynos MIC as in-bridge for 
Exynos DSI:

[    4.068866] [drm] Exynos DRM: using 13800000.decon device for DMA 
mapping operations
[    4.069183] exynos-drm exynos-drm: bound 13800000.decon (ops 
decon_component_ops)
[    4.128983] exynos-drm exynos-drm: bound 13880000.decon (ops 
decon_component_ops)
[    4.129261] exynos-drm exynos-drm: bound 13930000.mic (ops 
exynos_mic_component_ops)
[    4.133508] exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach] 
*ERROR* failed to find the bridge: -19
[    4.136392] exynos-drm exynos-drm: bound 13900000.dsi (ops 
exynos_dsi_component_ops)
[    4.145499] rc_core: Couldn't load IR keymap rc-cec
[    4.145666] Registered IR keymap rc-empty
[    4.148402] rc rc0: sii8620 as /devices/virtual/rc/rc0
[    4.156051] input: sii8620 as /devices/virtual/rc/rc0/input1
[    4.160647] exynos-drm exynos-drm: bound 13970000.hdmi (ops 
hdmi_component_ops)
[    4.169923] exynos-drm exynos-drm: [drm] Cannot find any crtc or sizes
[    4.173958] exynos-drm exynos-drm: [drm] Cannot find any crtc or sizes
[    4.182304] [drm] Initialized exynos 1.1.0 20180330 for exynos-drm on 
minor 0

The display pipeline for TM2e is:

Exynos5433 Decon -> Exynos MIC -> Exynos DSI -> s6e3ha2 DSI panel


Best regards

-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v4 0/6] drm: exynos: dsi: Convert drm bridge

[Jagan Teki]

Hi Marek,

On Wed, Dec 15, 2021 at 7:49 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi Jagan,
>
> On 15.12.2021 13:57, Jagan Teki wrote:
> > On Wed, Dec 15, 2021 at 5:31 PM Marek Szyprowski
> > <m.szyprowski@samsung.com> wrote:
> >> On 15.12.2021 11:15, Jagan Teki wrote:
> >>> Updated series about drm bridge conversion of exynos dsi.
> >>> Previous version can be accessible, here [1].
> >>>
> >>> Patch 1: connector reset
> >>>
> >>> Patch 2: panel_bridge API
> >>>
> >>> Patch 3: Bridge conversion
> >>>
> >>> Patch 4: Atomic functions
> >>>
> >>> Patch 5: atomic_set
> >>>
> >>> Patch 6: DSI init in enable
> >> There is a little progress! :)
> >>
> >> Devices with a simple display pipeline (only a DSI panel, like
> >> Trats/Trats2) works till the last patch. Then, after applying ("[PATCH
> >> v4 6/6] drm: exynos: dsi: Move DSI init in bridge enable"), I get no
> >> display at all.
> >>
> >> A TM2e board with in-bridge (Exynos MIC) stops displaying anything after
> >> applying patch "[PATCH v4 2/6] drm: exynos: dsi: Use drm panel_bridge API".
> >>
> >> In case of the Arndale board with tc358764 bridge, no much progress. The
> >> display is broken just after applying the "[PATCH v2] drm: bridge:
> >> tc358764: Use drm panel_bridge API" patch on top of linux-next.
> >>
> >> In all cases the I had "drm: of: Lookup if child node has panel or
> >> bridge" patch applied.
> > Just skip the 6/6 for now.
> >
> > Apply
> > - https://protect2.fireeye.com/v1/url?k=a24f3f76-fdd40659-a24eb439-0cc47a31cdf8-97ea12b4c5258d11&q=1&e=37a169bf-7ca5-4362-aad7-486018c7a708&u=https%3A%2F%2Fpatchwork.amarulasolutions.com%2Fpatch%2F1825%2F
> > - https://protect2.fireeye.com/v1/url?k=a226360f-fdbd0f20-a227bd40-0cc47a31cdf8-ebd66aebee1058d7&q=1&e=37a169bf-7ca5-4362-aad7-486018c7a708&u=https%3A%2F%2Fpatchwork.amarulasolutions.com%2Fpatch%2F1823%2F
> >
> > Then apply 1/6 to 5/6.  and update the status?
>
> Okay, my fault, I didn't check that case on Arndale.
>
> I've checked and indeed, Trats/Trats2 and Arndale works after the above
> 2 patches AND patches 1-5.
>
> The only problem is now on TM2e, which uses Exynos MIC as in-bridge for
> Exynos DSI:
>
> [    4.068866] [drm] Exynos DRM: using 13800000.decon device for DMA
> mapping operations
> [    4.069183] exynos-drm exynos-drm: bound 13800000.decon (ops
> decon_component_ops)
> [    4.128983] exynos-drm exynos-drm: bound 13880000.decon (ops
> decon_component_ops)
> [    4.129261] exynos-drm exynos-drm: bound 13930000.mic (ops
> exynos_mic_component_ops)
> [    4.133508] exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach]
> *ERROR* failed to find the bridge: -19
> [    4.136392] exynos-drm exynos-drm: bound 13900000.dsi (ops
> exynos_dsi_component_ops)
> [    4.145499] rc_core: Couldn't load IR keymap rc-cec
> [    4.145666] Registered IR keymap rc-empty
> [    4.148402] rc rc0: sii8620 as /devices/virtual/rc/rc0
> [    4.156051] input: sii8620 as /devices/virtual/rc/rc0/input1
> [    4.160647] exynos-drm exynos-drm: bound 13970000.hdmi (ops
> hdmi_component_ops)
> [    4.169923] exynos-drm exynos-drm: [drm] Cannot find any crtc or sizes
> [    4.173958] exynos-drm exynos-drm: [drm] Cannot find any crtc or sizes
> [    4.182304] [drm] Initialized exynos 1.1.0 20180330 for exynos-drm on
> minor 0
>
> The display pipeline for TM2e is:
>
> Exynos5433 Decon -> Exynos MIC -> Exynos DSI -> s6e3ha2 DSI panel

If Trats/Trats2 is working then it has to work. I don't see any
difference in output pipeline. Can you please share the full log, I
cannot see host_attach print saying "Attached.."

Jagan.

Re: [PATCH v4 0/6] drm: exynos: dsi: Convert drm bridge

[Marek Szyprowski]

Hi Jagan,

On 15.12.2021 15:56, Jagan Teki wrote:
> On Wed, Dec 15, 2021 at 7:49 PM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> On 15.12.2021 13:57, Jagan Teki wrote:
>>> On Wed, Dec 15, 2021 at 5:31 PM Marek Szyprowski
>>> <m.szyprowski@samsung.com> wrote:
>>>> On 15.12.2021 11:15, Jagan Teki wrote:
>>>>> Updated series about drm bridge conversion of exynos dsi.
>>>>> Previous version can be accessible, here [1].
>>>>>
>>>>> Patch 1: connector reset
>>>>>
>>>>> Patch 2: panel_bridge API
>>>>>
>>>>> Patch 3: Bridge conversion
>>>>>
>>>>> Patch 4: Atomic functions
>>>>>
>>>>> Patch 5: atomic_set
>>>>>
>>>>> Patch 6: DSI init in enable
>>>> There is a little progress! :)
>>>>
>>>> Devices with a simple display pipeline (only a DSI panel, like
>>>> Trats/Trats2) works till the last patch. Then, after applying ("[PATCH
>>>> v4 6/6] drm: exynos: dsi: Move DSI init in bridge enable"), I get no
>>>> display at all.
>>>>
>>>> A TM2e board with in-bridge (Exynos MIC) stops displaying anything after
>>>> applying patch "[PATCH v4 2/6] drm: exynos: dsi: Use drm panel_bridge API".
>>>>
>>>> In case of the Arndale board with tc358764 bridge, no much progress. The
>>>> display is broken just after applying the "[PATCH v2] drm: bridge:
>>>> tc358764: Use drm panel_bridge API" patch on top of linux-next.
>>>>
>>>> In all cases the I had "drm: of: Lookup if child node has panel or
>>>> bridge" patch applied.
>>> Just skip the 6/6 for now.
>>>
>>> Apply
>>> - https://protect2.fireeye.com/v1/url?k=a24f3f76-fdd40659-a24eb439-0cc47a31cdf8-97ea12b4c5258d11&q=1&e=37a169bf-7ca5-4362-aad7-486018c7a708&u=https%3A%2F%2Fpatchwork.amarulasolutions.com%2Fpatch%2F1825%2F
>>> - https://protect2.fireeye.com/v1/url?k=a226360f-fdbd0f20-a227bd40-0cc47a31cdf8-ebd66aebee1058d7&q=1&e=37a169bf-7ca5-4362-aad7-486018c7a708&u=https%3A%2F%2Fpatchwork.amarulasolutions.com%2Fpatch%2F1823%2F
>>>
>>> Then apply 1/6 to 5/6.  and update the status?
>> Okay, my fault, I didn't check that case on Arndale.
>>
>> I've checked and indeed, Trats/Trats2 and Arndale works after the above
>> 2 patches AND patches 1-5.
>>
>> The only problem is now on TM2e, which uses Exynos MIC as in-bridge for
>> Exynos DSI:
>>
>> [    4.068866] [drm] Exynos DRM: using 13800000.decon device for DMA
>> mapping operations
>> [    4.069183] exynos-drm exynos-drm: bound 13800000.decon (ops
>> decon_component_ops)
>> [    4.128983] exynos-drm exynos-drm: bound 13880000.decon (ops
>> decon_component_ops)
>> [    4.129261] exynos-drm exynos-drm: bound 13930000.mic (ops
>> exynos_mic_component_ops)
>> [    4.133508] exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach]
>> *ERROR* failed to find the bridge: -19
>> [    4.136392] exynos-drm exynos-drm: bound 13900000.dsi (ops
>> exynos_dsi_component_ops)
>> [    4.145499] rc_core: Couldn't load IR keymap rc-cec
>> [    4.145666] Registered IR keymap rc-empty
>> [    4.148402] rc rc0: sii8620 as /devices/virtual/rc/rc0
>> [    4.156051] input: sii8620 as /devices/virtual/rc/rc0/input1
>> [    4.160647] exynos-drm exynos-drm: bound 13970000.hdmi (ops
>> hdmi_component_ops)
>> [    4.169923] exynos-drm exynos-drm: [drm] Cannot find any crtc or sizes
>> [    4.173958] exynos-drm exynos-drm: [drm] Cannot find any crtc or sizes
>> [    4.182304] [drm] Initialized exynos 1.1.0 20180330 for exynos-drm on
>> minor 0
>>
>> The display pipeline for TM2e is:
>>
>> Exynos5433 Decon -> Exynos MIC -> Exynos DSI -> s6e3ha2 DSI panel
> If Trats/Trats2 is working then it has to work. I don't see any
> difference in output pipeline. Can you please share the full log, I
> cannot see host_attach print saying "Attached.."

Well, there is a failure message about the panel:

exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach] *ERROR* failed to 
find the bridge: -19

however it looks that something might be broken in dts. The in-bridge 
(Exynos MIC) is on port 0 and the panel is @0, what imho might cause the 
issue.

I've tried to change in in-bridge ('mic_to_dsi') port to 1 in 
exynos5433.dtsi. Then the panel has been attached:

exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach] Attached s6e3hf2 
device

but the display is still not working, probably due to lack of proper 
Exynos MIC handling. I will investigate it later and let You know.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v4 0/6] drm: exynos: dsi: Convert drm bridge

[Marek Szyprowski]

Hi Jagan,

On 18.12.2021 00:16, Marek Szyprowski wrote:
> On 15.12.2021 15:56, Jagan Teki wrote:
>> On Wed, Dec 15, 2021 at 7:49 PM Marek Szyprowski
>> <m.szyprowski@samsung.com> wrote:
>>> On 15.12.2021 13:57, Jagan Teki wrote:
>>>> On Wed, Dec 15, 2021 at 5:31 PM Marek Szyprowski
>>>> <m.szyprowski@samsung.com> wrote:
>>>>> On 15.12.2021 11:15, Jagan Teki wrote:
>>>>>> Updated series about drm bridge conversion of exynos dsi.
>>>>>> Previous version can be accessible, here [1].
>>>>>>
>>>>>> Patch 1: connector reset
>>>>>>
>>>>>> Patch 2: panel_bridge API
>>>>>>
>>>>>> Patch 3: Bridge conversion
>>>>>>
>>>>>> Patch 4: Atomic functions
>>>>>>
>>>>>> Patch 5: atomic_set
>>>>>>
>>>>>> Patch 6: DSI init in enable
>>>>> There is a little progress! :)
>>>>>
>>>>> Devices with a simple display pipeline (only a DSI panel, like
>>>>> Trats/Trats2) works till the last patch. Then, after applying 
>>>>> ("[PATCH
>>>>> v4 6/6] drm: exynos: dsi: Move DSI init in bridge enable"), I get no
>>>>> display at all.
>>>>>
>>>>> A TM2e board with in-bridge (Exynos MIC) stops displaying anything 
>>>>> after
>>>>> applying patch "[PATCH v4 2/6] drm: exynos: dsi: Use drm 
>>>>> panel_bridge API".
>>>>>
>>>>> In case of the Arndale board with tc358764 bridge, no much 
>>>>> progress. The
>>>>> display is broken just after applying the "[PATCH v2] drm: bridge:
>>>>> tc358764: Use drm panel_bridge API" patch on top of linux-next.
>>>>>
>>>>> In all cases the I had "drm: of: Lookup if child node has panel or
>>>>> bridge" patch applied.
>>>> Just skip the 6/6 for now.
>>>>
>>>> Apply
>>>> - 
>>>> https://protect2.fireeye.com/v1/url?k=a24f3f76-fdd40659-a24eb439-0cc47a31cdf8-97ea12b4c5258d11&q=1&e=37a169bf-7ca5-4362-aad7-486018c7a708&u=https%3A%2F%2Fpatchwork.amarulasolutions.com%2Fpatch%2F1825%2F
>>>> - 
>>>> https://protect2.fireeye.com/v1/url?k=a226360f-fdbd0f20-a227bd40-0cc47a31cdf8-ebd66aebee1058d7&q=1&e=37a169bf-7ca5-4362-aad7-486018c7a708&u=https%3A%2F%2Fpatchwork.amarulasolutions.com%2Fpatch%2F1823%2F
>>>>
>>>> Then apply 1/6 to 5/6.  and update the status?
>>> Okay, my fault, I didn't check that case on Arndale.
>>>
>>> I've checked and indeed, Trats/Trats2 and Arndale works after the above
>>> 2 patches AND patches 1-5.
>>>
>>> The only problem is now on TM2e, which uses Exynos MIC as in-bridge for
>>> Exynos DSI:
>>>
>>> [    4.068866] [drm] Exynos DRM: using 13800000.decon device for DMA
>>> mapping operations
>>> [    4.069183] exynos-drm exynos-drm: bound 13800000.decon (ops
>>> decon_component_ops)
>>> [    4.128983] exynos-drm exynos-drm: bound 13880000.decon (ops
>>> decon_component_ops)
>>> [    4.129261] exynos-drm exynos-drm: bound 13930000.mic (ops
>>> exynos_mic_component_ops)
>>> [    4.133508] exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach]
>>> *ERROR* failed to find the bridge: -19
>>> [    4.136392] exynos-drm exynos-drm: bound 13900000.dsi (ops
>>> exynos_dsi_component_ops)
>>> [    4.145499] rc_core: Couldn't load IR keymap rc-cec
>>> [    4.145666] Registered IR keymap rc-empty
>>> [    4.148402] rc rc0: sii8620 as /devices/virtual/rc/rc0
>>> [    4.156051] input: sii8620 as /devices/virtual/rc/rc0/input1
>>> [    4.160647] exynos-drm exynos-drm: bound 13970000.hdmi (ops
>>> hdmi_component_ops)
>>> [    4.169923] exynos-drm exynos-drm: [drm] Cannot find any crtc or 
>>> sizes
>>> [    4.173958] exynos-drm exynos-drm: [drm] Cannot find any crtc or 
>>> sizes
>>> [    4.182304] [drm] Initialized exynos 1.1.0 20180330 for 
>>> exynos-drm on
>>> minor 0
>>>
>>> The display pipeline for TM2e is:
>>>
>>> Exynos5433 Decon -> Exynos MIC -> Exynos DSI -> s6e3ha2 DSI panel
>> If Trats/Trats2 is working then it has to work. I don't see any
>> difference in output pipeline. Can you please share the full log, I
>> cannot see host_attach print saying "Attached.."
>
> Well, there is a failure message about the panel:
>
> exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach] *ERROR* failed 
> to find the bridge: -19
>
> however it looks that something might be broken in dts. The in-bridge 
> (Exynos MIC) is on port 0 and the panel is @0, what imho might cause 
> the issue.
>
> I've tried to change in in-bridge ('mic_to_dsi') port to 1 in 
> exynos5433.dtsi. Then the panel has been attached:
>
> exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach] Attached s6e3hf2 
> device
>
> but the display is still not working, probably due to lack of proper 
> Exynos MIC handling. I will investigate it later and let You know.


I've played a bit with the Exynos DRM code and finally I made it working 
on TM2(e). There are basically 3 different issues that need to be fixed 
to get it working with the $subject patchset:

1. Port numbers in exynos5433 dsi/dts are broken. For all pre-Exynos5433 
boards the panel was defined as a DSI node child (at 'reg 0'), what 
means it used port 0. Then, Exynos5433 introduced so called RGB-in at 
port 0 and panel at port 1 (as described in the dt bindings). However 
the committed Exynos5433 dtsi and TM2(e) dts still defined panel as a 
DSI child (with reg=0, so port 0) and Exynos MIC as of-graph at port 0. 
The Exynos DSI code however always searched for a panel as a DSI child 
node, so it worked fine, even though the panel and exynos mic used in 
fact the 'port 0'. IMHO this can be fixed by the following patch:

diff --git a/arch/arm64/boot/dts/exynos/exynos5433.dtsi 
b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
index bfe4ed8a23d6..2718c752d916 100644
--- a/arch/arm64/boot/dts/exynos/exynos5433.dtsi
+++ b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
@@ -1046,8 +1046,8 @@
                                 #address-cells = <1>;
                                 #size-cells = <0>;

-                               port@0 {
-                                       reg = <0>;
+                               port@1 {
+                                       reg = <1>;
                                         dsi_to_mic: endpoint {
                                                 remote-endpoint = 
<&mic_to_dsi>;
                                         };
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c 
b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index d2933a70c01f..e8e2df339c5f 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -220,8 +220,8 @@ enum exynos_dsi_transfer_type {
  };

  enum {
-       DSI_PORT_IN,
-       DSI_PORT_OUT
+       DSI_PORT_OUT,
+       DSI_PORT_IN
  };

  struct exynos_dsi_transfer {
--

2. (devm_)drm_of_get_bridge() ignores panel's 'reg' property and it is 
unable to find a panel if there is a 'ports' child node (even if it 
describes completely different port than the one requested by the 
caller). I don't have time now to fix this, a simple and ugly workaround 
for that is to define the complete of-graph for the DSI child panel:

diff --git a/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts 
b/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts
index 22d26460f3dd..4726c325bd2c 100644
--- a/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts
+++ b/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts
@@ -58,6 +58,27 @@
                 vci-supply = <&ldo28_reg>;
                 reset-gpios = <&gpg0 0 GPIO_ACTIVE_LOW>;
                 enable-gpios = <&gpf1 5 GPIO_ACTIVE_HIGH>;
+
+               ports {
+                       #address-cells = <1>;
+                       #size-cells = <0>;
+
+                       port@0 {
+                               reg = <0>;
+                               panel_to_dsi: endpoint {
+                                       remote-endpoint = <&dsi_to_panel>;
+                               };
+                       };
+               };
+       };
+
+       ports {
+               port@0 {
+                       reg = <0>;
+                       dsi_to_panel: endpoint {
+                               remote-endpoint = <&panel_to_dsi>;
+                       };
+               };
         };
  };

--

3. Lack of proper handling of the 'in-bridge' in the Exynos DSI after 
Your conversion. Initially I thought that this could be fixed with the 
following simple patch:

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c 
b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index e8e2df339c5f..2fe9c995549f 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -259,6 +259,7 @@ struct exynos_dsi {
         struct mipi_dsi_host dsi_host;
         struct drm_bridge bridge;
         struct drm_bridge *out_bridge;
+       struct drm_bridge *in_bridge;
         struct device *dev;

         void __iomem *reg_base;
@@ -1535,7 +1536,7 @@ static int exynos_dsi_host_attach(struct 
mipi_dsi_host *host,

         drm_bridge_add(&dsi->bridge);

-       drm_bridge_attach(encoder, &dsi->bridge, NULL, 0);
+       drm_bridge_attach(encoder, &dsi->bridge, dsi->in_bridge, 0);

         /*
          * This is a temporary solution and should be made by more 
generic way.
@@ -1674,7 +1675,6 @@ static int exynos_dsi_bind(struct device *dev, 
struct device *master,
         struct drm_encoder *encoder = &dsi->encoder;
         struct drm_device *drm_dev = data;
         struct device_node *in_bridge_node;
-       struct drm_bridge *in_bridge;
         int ret;

         printk("drm: %s enter\n", __func__);
@@ -1688,9 +1688,10 @@ static int exynos_dsi_bind(struct device *dev, 
struct device *master,
         }
         in_bridge_node = of_graph_get_remote_node(dev->of_node, 
DSI_PORT_IN, 0);
         if (in_bridge_node) {
-               in_bridge = of_drm_find_bridge(in_bridge_node);
-               if (in_bridge)
-                       drm_bridge_attach(encoder, in_bridge, NULL, 0);
+               dsi->in_bridge = of_drm_find_bridge(in_bridge_node);
+               if (dsi->in_bridge) {
+                       drm_bridge_attach(encoder, dsi->in_bridge, NULL, 0);
+               }
                 of_node_put(in_bridge_node);
         }

--

but it turned out that in such case the order of the bridge 'enable' 
calls is not correct for display pipeline operation. The one that 
actually works is (sorry for the hacky code):

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c 
b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index e8e2df339c5f..51b568556fce 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -259,6 +259,7 @@ struct exynos_dsi {
         struct mipi_dsi_host dsi_host;
         struct drm_bridge bridge;
         struct drm_bridge *out_bridge;
+       struct drm_bridge *in_bridge;
         struct device *dev;

         void __iomem *reg_base;
@@ -1424,6 +1425,9 @@ static void exynos_dsi_atomic_pre_enable(struct 
drm_bridge *bridge,

         dsi->state |= DSIM_STATE_ENABLED;
         printk("drm: %s finish: %d\n", __func__, 0);
+
+       if (dsi->in_bridge)
+ dsi->in_bridge->funcs->pre_enable(dsi->in_bridge);
  }

  static void exynos_dsi_atomic_enable(struct drm_bridge *bridge,
@@ -1439,6 +1443,9 @@ static void exynos_dsi_atomic_enable(struct 
drm_bridge *bridge,

         printk("drm: %s finish: %d\n", __func__, 0);

+       if (dsi->in_bridge)
+ dsi->in_bridge->funcs->enable(dsi->in_bridge);
+
         return;
  }

@@ -1455,6 +1462,8 @@ static void exynos_dsi_atomic_disable(struct 
drm_bridge *bridge,
         dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
         printk("drm: %s finish: %d\n", __func__, 0);

+       if (dsi->in_bridge)
+ dsi->in_bridge->funcs->disable(dsi->in_bridge);
  }

  static void exynos_dsi_atomic_post_disable(struct drm_bridge *bridge,
@@ -1470,6 +1479,8 @@ static void exynos_dsi_atomic_post_disable(struct 
drm_bridge *bridge,

         printk("drm: %s finish: %d\n", __func__, 0);

+       if (dsi->in_bridge)
+ dsi->in_bridge->funcs->post_disable(dsi->in_bridge);
  }

  static void exynos_dsi_mode_set(struct drm_bridge *bridge,
@@ -1482,6 +1493,10 @@ static void exynos_dsi_mode_set(struct drm_bridge 
*bridge,

         drm_mode_copy(&dsi->mode, adjusted_mode);
         printk("drm: %s finish: %d\n", __func__, ret);
+
+       if (dsi->in_bridge)
+ dsi->in_bridge->funcs->mode_set(dsi->in_bridge, mode,
+                                               adjusted_mode);
  }

  static int exynos_dsi_attach(struct drm_bridge *bridge,
  @@ -1674,7 +1689,6 @@ static int exynos_dsi_bind(struct device *dev, 
struct device *master,
         struct drm_encoder *encoder = &dsi->encoder;
         struct drm_device *drm_dev = data;
         struct device_node *in_bridge_node;
-       struct drm_bridge *in_bridge;
         int ret;

         printk("drm: %s enter\n", __func__);
@@ -1688,9 +1702,11 @@ static int exynos_dsi_bind(struct device *dev, 
struct device *master,
         }
         in_bridge_node = of_graph_get_remote_node(dev->of_node, 
DSI_PORT_IN, 0);
         if (in_bridge_node) {
-               in_bridge = of_drm_find_bridge(in_bridge_node);
-               if (in_bridge)
-                       drm_bridge_attach(encoder, in_bridge, NULL, 0);
+               dsi->in_bridge = of_drm_find_bridge(in_bridge_node);
+               if (dsi->in_bridge) {
+                       drm_bridge_attach(encoder, dsi->in_bridge, NULL, 0);
+ INIT_LIST_HEAD(&encoder->bridge_chain);
+               }
                 of_node_put(in_bridge_node);
         }

--

I hope the above findings helps somehow in finishing this patchset. I'm 
not deep in the DRM bridge development, but it looks that there is 
something seriously broken in the design or Exynos MIC and DSI should 
not be modeled as bridges.

I will be on holidays till 10th Jan 2022.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v4 0/6] drm: exynos: dsi: Convert drm bridge

[Andrzej Hajda]

Hi Marek,

On 23.12.2021 10:15, Marek Szyprowski wrote:
> Hi Jagan,
>
> On 18.12.2021 00:16, Marek Szyprowski wrote:
>> On 15.12.2021 15:56, Jagan Teki wrote:
>>> On Wed, Dec 15, 2021 at 7:49 PM Marek Szyprowski
>>> <m.szyprowski@samsung.com> wrote:
>>>> On 15.12.2021 13:57, Jagan Teki wrote:
>>>>> On Wed, Dec 15, 2021 at 5:31 PM Marek Szyprowski
>>>>> <m.szyprowski@samsung.com> wrote:
>>>>>> On 15.12.2021 11:15, Jagan Teki wrote:
>>>>>>> Updated series about drm bridge conversion of exynos dsi.
>>>>>>> Previous version can be accessible, here [1].
>>>>>>>
>>>>>>> Patch 1: connector reset
>>>>>>>
>>>>>>> Patch 2: panel_bridge API
>>>>>>>
>>>>>>> Patch 3: Bridge conversion
>>>>>>>
>>>>>>> Patch 4: Atomic functions
>>>>>>>
>>>>>>> Patch 5: atomic_set
>>>>>>>
>>>>>>> Patch 6: DSI init in enable
>>>>>> There is a little progress! :)
>>>>>>
>>>>>> Devices with a simple display pipeline (only a DSI panel, like
>>>>>> Trats/Trats2) works till the last patch. Then, after applying
>>>>>> ("[PATCH
>>>>>> v4 6/6] drm: exynos: dsi: Move DSI init in bridge enable"), I get no
>>>>>> display at all.
>>>>>>
>>>>>> A TM2e board with in-bridge (Exynos MIC) stops displaying anything
>>>>>> after
>>>>>> applying patch "[PATCH v4 2/6] drm: exynos: dsi: Use drm
>>>>>> panel_bridge API".
>>>>>>
>>>>>> In case of the Arndale board with tc358764 bridge, no much
>>>>>> progress. The
>>>>>> display is broken just after applying the "[PATCH v2] drm: bridge:
>>>>>> tc358764: Use drm panel_bridge API" patch on top of linux-next.
>>>>>>
>>>>>> In all cases the I had "drm: of: Lookup if child node has panel or
>>>>>> bridge" patch applied.
>>>>> Just skip the 6/6 for now.
>>>>>
>>>>> Apply
>>>>> -
>>>>> https://protect2.fireeye.com/v1/url?k=a24f3f76-fdd40659-a24eb439-0cc47a31cdf8-97ea12b4c5258d11&q=1&e=37a169bf-7ca5-4362-aad7-486018c7a708&u=https%3A%2F%2Fpatchwork.amarulasolutions.com%2Fpatch%2F1825%2F
>>>>> -
>>>>> https://protect2.fireeye.com/v1/url?k=a226360f-fdbd0f20-a227bd40-0cc47a31cdf8-ebd66aebee1058d7&q=1&e=37a169bf-7ca5-4362-aad7-486018c7a708&u=https%3A%2F%2Fpatchwork.amarulasolutions.com%2Fpatch%2F1823%2F
>>>>>
>>>>> Then apply 1/6 to 5/6.  and update the status?
>>>> Okay, my fault, I didn't check that case on Arndale.
>>>>
>>>> I've checked and indeed, Trats/Trats2 and Arndale works after the above
>>>> 2 patches AND patches 1-5.
>>>>
>>>> The only problem is now on TM2e, which uses Exynos MIC as in-bridge for
>>>> Exynos DSI:
>>>>
>>>> [    4.068866] [drm] Exynos DRM: using 13800000.decon device for DMA
>>>> mapping operations
>>>> [    4.069183] exynos-drm exynos-drm: bound 13800000.decon (ops
>>>> decon_component_ops)
>>>> [    4.128983] exynos-drm exynos-drm: bound 13880000.decon (ops
>>>> decon_component_ops)
>>>> [    4.129261] exynos-drm exynos-drm: bound 13930000.mic (ops
>>>> exynos_mic_component_ops)
>>>> [    4.133508] exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach]
>>>> *ERROR* failed to find the bridge: -19
>>>> [    4.136392] exynos-drm exynos-drm: bound 13900000.dsi (ops
>>>> exynos_dsi_component_ops)
>>>> [    4.145499] rc_core: Couldn't load IR keymap rc-cec
>>>> [    4.145666] Registered IR keymap rc-empty
>>>> [    4.148402] rc rc0: sii8620 as /devices/virtual/rc/rc0
>>>> [    4.156051] input: sii8620 as /devices/virtual/rc/rc0/input1
>>>> [    4.160647] exynos-drm exynos-drm: bound 13970000.hdmi (ops
>>>> hdmi_component_ops)
>>>> [    4.169923] exynos-drm exynos-drm: [drm] Cannot find any crtc or
>>>> sizes
>>>> [    4.173958] exynos-drm exynos-drm: [drm] Cannot find any crtc or
>>>> sizes
>>>> [    4.182304] [drm] Initialized exynos 1.1.0 20180330 for
>>>> exynos-drm on
>>>> minor 0
>>>>
>>>> The display pipeline for TM2e is:
>>>>
>>>> Exynos5433 Decon -> Exynos MIC -> Exynos DSI -> s6e3ha2 DSI panel
>>> If Trats/Trats2 is working then it has to work. I don't see any
>>> difference in output pipeline. Can you please share the full log, I
>>> cannot see host_attach print saying "Attached.."
>> Well, there is a failure message about the panel:
>>
>> exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach] *ERROR* failed
>> to find the bridge: -19
>>
>> however it looks that something might be broken in dts. The in-bridge
>> (Exynos MIC) is on port 0 and the panel is @0, what imho might cause
>> the issue.
>>
>> I've tried to change in in-bridge ('mic_to_dsi') port to 1 in
>> exynos5433.dtsi. Then the panel has been attached:
>>
>> exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach] Attached s6e3hf2
>> device
>>
>> but the display is still not working, probably due to lack of proper
>> Exynos MIC handling. I will investigate it later and let You know.
>
> I've played a bit with the Exynos DRM code and finally I made it working
> on TM2(e). There are basically 3 different issues that need to be fixed
> to get it working with the $subject patchset:
>
> 1. Port numbers in exynos5433 dsi/dts are broken. For all pre-Exynos5433
> boards the panel was defined as a DSI node child (at 'reg 0'),


True, emphasis that it is reg 0 of DSI bus.


>   what
> means it used port 0.


And this does not seems true to me. Established practice is (unless I 
have not noticed change in bindings :) ) that in case of data bus shared 
with control bus the port node is optional. In such case host knows 
already who is connected to its data bus, it does not need port node. 
And if there is no port node there is no port number as well.

As I quickly looked at the exynos bindings they seems generally OK to 
me, if there is something wrong/suspicious let me know.


>   Then, Exynos5433 introduced so called RGB-in at
> port 0 and panel at port 1 (as described in the dt bindings).However
> the committed Exynos5433 dtsi and TM2(e) dts still defined panel as a
> DSI child (with reg=0, so port 0) and Exynos MIC as of-graph at port 0.
> The Exynos DSI code however always searched for a panel as a DSI child
> node, so it worked fine, even though the panel and exynos mic used in
> fact the 'port 0'. IMHO this can be fixed by the following patch:
>
> diff --git a/arch/arm64/boot/dts/exynos/exynos5433.dtsi
> b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
> index bfe4ed8a23d6..2718c752d916 100644
> --- a/arch/arm64/boot/dts/exynos/exynos5433.dtsi
> +++ b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
> @@ -1046,8 +1046,8 @@
>                                   #address-cells = <1>;
>                                   #size-cells = <0>;
>
> -                               port@0 {
> -                                       reg = <0>;
> +                               port@1 {
> +                                       reg = <1>;
>                                           dsi_to_mic: endpoint {
>                                                   remote-endpoint =
> <&mic_to_dsi>;
>                                           };
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index d2933a70c01f..e8e2df339c5f 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -220,8 +220,8 @@ enum exynos_dsi_transfer_type {
>    };
>
>    enum {
> -       DSI_PORT_IN,
> -       DSI_PORT_OUT
> +       DSI_PORT_OUT,
> +       DSI_PORT_IN
>    };
>
>    struct exynos_dsi_transfer {
> --
>
> 2. (devm_)drm_of_get_bridge() ignores panel's 'reg' property and it is


I guess drm_of_get_bridge should not be used in exynos_dsi_host_attach 
at all - there are no ports here, only of_node of the sink.

Since there is no helper to workaround the dualism panel/bridge you 
should still use of_drm_find_bridge and of_drm_find_panel pair.


> unable to find a panel if there is a 'ports' child node (even if it
> describes completely different port than the one requested by the
> caller). I don't have time now to fix this, a simple and ugly workaround
> for that is to define the complete of-graph for the DSI child panel:
>
> diff --git a/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts
> b/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts
> index 22d26460f3dd..4726c325bd2c 100644
> --- a/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts
> +++ b/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts
> @@ -58,6 +58,27 @@
>                   vci-supply = <&ldo28_reg>;
>                   reset-gpios = <&gpg0 0 GPIO_ACTIVE_LOW>;
>                   enable-gpios = <&gpf1 5 GPIO_ACTIVE_HIGH>;
> +
> +               ports {
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +
> +                       port@0 {
> +                               reg = <0>;
> +                               panel_to_dsi: endpoint {
> +                                       remote-endpoint = <&dsi_to_panel>;
> +                               };
> +                       };
> +               };
> +       };
> +
> +       ports {
> +               port@0 {
> +                       reg = <0>;
> +                       dsi_to_panel: endpoint {
> +                               remote-endpoint = <&panel_to_dsi>;
> +                       };
> +               };
>           };
>    };
>
> --
>
> 3. Lack of proper handling of the 'in-bridge' in the Exynos DSI after
> Your conversion. Initially I thought that this could be fixed with the
> following simple patch:
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index e8e2df339c5f..2fe9c995549f 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -259,6 +259,7 @@ struct exynos_dsi {
>           struct mipi_dsi_host dsi_host;
>           struct drm_bridge bridge;
>           struct drm_bridge *out_bridge;
> +       struct drm_bridge *in_bridge;
>           struct device *dev;
>
>           void __iomem *reg_base;
> @@ -1535,7 +1536,7 @@ static int exynos_dsi_host_attach(struct
> mipi_dsi_host *host,
>
>           drm_bridge_add(&dsi->bridge);
>
> -       drm_bridge_attach(encoder, &dsi->bridge, NULL, 0);
> +       drm_bridge_attach(encoder, &dsi->bridge, dsi->in_bridge, 0);
>
>           /*
>            * This is a temporary solution and should be made by more
> generic way.
> @@ -1674,7 +1675,6 @@ static int exynos_dsi_bind(struct device *dev,
> struct device *master,
>           struct drm_encoder *encoder = &dsi->encoder;
>           struct drm_device *drm_dev = data;
>           struct device_node *in_bridge_node;
> -       struct drm_bridge *in_bridge;
>           int ret;
>
>           printk("drm: %s enter\n", __func__);
> @@ -1688,9 +1688,10 @@ static int exynos_dsi_bind(struct device *dev,
> struct device *master,
>           }
>           in_bridge_node = of_graph_get_remote_node(dev->of_node,
> DSI_PORT_IN, 0);
>           if (in_bridge_node) {
> -               in_bridge = of_drm_find_bridge(in_bridge_node);
> -               if (in_bridge)
> -                       drm_bridge_attach(encoder, in_bridge, NULL, 0);
> +               dsi->in_bridge = of_drm_find_bridge(in_bridge_node);
> +               if (dsi->in_bridge) {
> +                       drm_bridge_attach(encoder, dsi->in_bridge, NULL, 0);
> +               }
>                   of_node_put(in_bridge_node);
>           }
>
> --
>
> but it turned out that in such case the order of the bridge 'enable'
> calls is not correct for display pipeline operation. The one that
> actually works is (sorry for the hacky code):
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index e8e2df339c5f..51b568556fce 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -259,6 +259,7 @@ struct exynos_dsi {
>           struct mipi_dsi_host dsi_host;
>           struct drm_bridge bridge;
>           struct drm_bridge *out_bridge;
> +       struct drm_bridge *in_bridge;
>           struct device *dev;
>
>           void __iomem *reg_base;
> @@ -1424,6 +1425,9 @@ static void exynos_dsi_atomic_pre_enable(struct
> drm_bridge *bridge,
>
>           dsi->state |= DSIM_STATE_ENABLED;
>           printk("drm: %s finish: %d\n", __func__, 0);
> +
> +       if (dsi->in_bridge)
> + dsi->in_bridge->funcs->pre_enable(dsi->in_bridge);
>    }
>
>    static void exynos_dsi_atomic_enable(struct drm_bridge *bridge,
> @@ -1439,6 +1443,9 @@ static void exynos_dsi_atomic_enable(struct
> drm_bridge *bridge,
>
>           printk("drm: %s finish: %d\n", __func__, 0);
>
> +       if (dsi->in_bridge)
> + dsi->in_bridge->funcs->enable(dsi->in_bridge);
> +
>           return;
>    }
>
> @@ -1455,6 +1462,8 @@ static void exynos_dsi_atomic_disable(struct
> drm_bridge *bridge,
>           dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
>           printk("drm: %s finish: %d\n", __func__, 0);
>
> +       if (dsi->in_bridge)
> + dsi->in_bridge->funcs->disable(dsi->in_bridge);
>    }
>
>    static void exynos_dsi_atomic_post_disable(struct drm_bridge *bridge,
> @@ -1470,6 +1479,8 @@ static void exynos_dsi_atomic_post_disable(struct
> drm_bridge *bridge,
>
>           printk("drm: %s finish: %d\n", __func__, 0);
>
> +       if (dsi->in_bridge)
> + dsi->in_bridge->funcs->post_disable(dsi->in_bridge);
>    }
>
>    static void exynos_dsi_mode_set(struct drm_bridge *bridge,
> @@ -1482,6 +1493,10 @@ static void exynos_dsi_mode_set(struct drm_bridge
> *bridge,
>
>           drm_mode_copy(&dsi->mode, adjusted_mode);
>           printk("drm: %s finish: %d\n", __func__, ret);
> +
> +       if (dsi->in_bridge)
> + dsi->in_bridge->funcs->mode_set(dsi->in_bridge, mode,
> +                                               adjusted_mode);
>    }
>
>    static int exynos_dsi_attach(struct drm_bridge *bridge,
>    @@ -1674,7 +1689,6 @@ static int exynos_dsi_bind(struct device *dev,
> struct device *master,
>           struct drm_encoder *encoder = &dsi->encoder;
>           struct drm_device *drm_dev = data;
>           struct device_node *in_bridge_node;
> -       struct drm_bridge *in_bridge;
>           int ret;
>
>           printk("drm: %s enter\n", __func__);
> @@ -1688,9 +1702,11 @@ static int exynos_dsi_bind(struct device *dev,
> struct device *master,
>           }
>           in_bridge_node = of_graph_get_remote_node(dev->of_node,
> DSI_PORT_IN, 0);
>           if (in_bridge_node) {
> -               in_bridge = of_drm_find_bridge(in_bridge_node);
> -               if (in_bridge)
> -                       drm_bridge_attach(encoder, in_bridge, NULL, 0);
> +               dsi->in_bridge = of_drm_find_bridge(in_bridge_node);
> +               if (dsi->in_bridge) {
> +                       drm_bridge_attach(encoder, dsi->in_bridge, NULL, 0);
> + INIT_LIST_HEAD(&encoder->bridge_chain);
> +               }
>                   of_node_put(in_bridge_node);
>           }
>
> --
>
> I hope the above findings helps somehow in finishing this patchset. I'm
> not deep in the DRM bridge development, but it looks that there is
> something seriously broken in the design or Exynos MIC and DSI should
> not be modeled as bridges.


I guess the most 'correct' way of handling mic would be dropping 
completely in_bridge from exynos_dsi, instead use bridge chain.

This would require touching decon driver and re-think the role of almost 
dummy 'encoder' code.

I suppose original author of mic bridge did this way because bridge 
framework did not supported attaching bridge between crtc and encoder, 
but now with exynos_dsi as a bridge it should be possible.

But this could be next step.


Regards

Andrzej



>
> I will be on holidays till 10th Jan 2022.
>
> Best regards

Re: [PATCH v4 0/6] drm: exynos: dsi: Convert drm bridge

[Jagan Teki]

Hi Marek,

On Thu, Dec 23, 2021 at 2:45 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi Jagan,
>
> On 18.12.2021 00:16, Marek Szyprowski wrote:
> > On 15.12.2021 15:56, Jagan Teki wrote:
> >> On Wed, Dec 15, 2021 at 7:49 PM Marek Szyprowski
> >> <m.szyprowski@samsung.com> wrote:
> >>> On 15.12.2021 13:57, Jagan Teki wrote:
> >>>> On Wed, Dec 15, 2021 at 5:31 PM Marek Szyprowski
> >>>> <m.szyprowski@samsung.com> wrote:
> >>>>> On 15.12.2021 11:15, Jagan Teki wrote:
> >>>>>> Updated series about drm bridge conversion of exynos dsi.
> >>>>>> Previous version can be accessible, here [1].
> >>>>>>
> >>>>>> Patch 1: connector reset
> >>>>>>
> >>>>>> Patch 2: panel_bridge API
> >>>>>>
> >>>>>> Patch 3: Bridge conversion
> >>>>>>
> >>>>>> Patch 4: Atomic functions
> >>>>>>
> >>>>>> Patch 5: atomic_set
> >>>>>>
> >>>>>> Patch 6: DSI init in enable
> >>>>> There is a little progress! :)
> >>>>>
> >>>>> Devices with a simple display pipeline (only a DSI panel, like
> >>>>> Trats/Trats2) works till the last patch. Then, after applying
> >>>>> ("[PATCH
> >>>>> v4 6/6] drm: exynos: dsi: Move DSI init in bridge enable"), I get no
> >>>>> display at all.
> >>>>>
> >>>>> A TM2e board with in-bridge (Exynos MIC) stops displaying anything
> >>>>> after
> >>>>> applying patch "[PATCH v4 2/6] drm: exynos: dsi: Use drm
> >>>>> panel_bridge API".
> >>>>>
> >>>>> In case of the Arndale board with tc358764 bridge, no much
> >>>>> progress. The
> >>>>> display is broken just after applying the "[PATCH v2] drm: bridge:
> >>>>> tc358764: Use drm panel_bridge API" patch on top of linux-next.
> >>>>>
> >>>>> In all cases the I had "drm: of: Lookup if child node has panel or
> >>>>> bridge" patch applied.
> >>>> Just skip the 6/6 for now.
> >>>>
> >>>> Apply
> >>>> -
> >>>> https://protect2.fireeye.com/v1/url?k=a24f3f76-fdd40659-a24eb439-0cc47a31cdf8-97ea12b4c5258d11&q=1&e=37a169bf-7ca5-4362-aad7-486018c7a708&u=https%3A%2F%2Fpatchwork.amarulasolutions.com%2Fpatch%2F1825%2F
> >>>> -
> >>>> https://protect2.fireeye.com/v1/url?k=a226360f-fdbd0f20-a227bd40-0cc47a31cdf8-ebd66aebee1058d7&q=1&e=37a169bf-7ca5-4362-aad7-486018c7a708&u=https%3A%2F%2Fpatchwork.amarulasolutions.com%2Fpatch%2F1823%2F
> >>>>
> >>>> Then apply 1/6 to 5/6.  and update the status?
> >>> Okay, my fault, I didn't check that case on Arndale.
> >>>
> >>> I've checked and indeed, Trats/Trats2 and Arndale works after the above
> >>> 2 patches AND patches 1-5.
> >>>
> >>> The only problem is now on TM2e, which uses Exynos MIC as in-bridge for
> >>> Exynos DSI:
> >>>
> >>> [    4.068866] [drm] Exynos DRM: using 13800000.decon device for DMA
> >>> mapping operations
> >>> [    4.069183] exynos-drm exynos-drm: bound 13800000.decon (ops
> >>> decon_component_ops)
> >>> [    4.128983] exynos-drm exynos-drm: bound 13880000.decon (ops
> >>> decon_component_ops)
> >>> [    4.129261] exynos-drm exynos-drm: bound 13930000.mic (ops
> >>> exynos_mic_component_ops)
> >>> [    4.133508] exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach]
> >>> *ERROR* failed to find the bridge: -19
> >>> [    4.136392] exynos-drm exynos-drm: bound 13900000.dsi (ops
> >>> exynos_dsi_component_ops)
> >>> [    4.145499] rc_core: Couldn't load IR keymap rc-cec
> >>> [    4.145666] Registered IR keymap rc-empty
> >>> [    4.148402] rc rc0: sii8620 as /devices/virtual/rc/rc0
> >>> [    4.156051] input: sii8620 as /devices/virtual/rc/rc0/input1
> >>> [    4.160647] exynos-drm exynos-drm: bound 13970000.hdmi (ops
> >>> hdmi_component_ops)
> >>> [    4.169923] exynos-drm exynos-drm: [drm] Cannot find any crtc or
> >>> sizes
> >>> [    4.173958] exynos-drm exynos-drm: [drm] Cannot find any crtc or
> >>> sizes
> >>> [    4.182304] [drm] Initialized exynos 1.1.0 20180330 for
> >>> exynos-drm on
> >>> minor 0
> >>>
> >>> The display pipeline for TM2e is:
> >>>
> >>> Exynos5433 Decon -> Exynos MIC -> Exynos DSI -> s6e3ha2 DSI panel
> >> If Trats/Trats2 is working then it has to work. I don't see any
> >> difference in output pipeline. Can you please share the full log, I
> >> cannot see host_attach print saying "Attached.."
> >
> > Well, there is a failure message about the panel:
> >
> > exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach] *ERROR* failed
> > to find the bridge: -19
> >
> > however it looks that something might be broken in dts. The in-bridge
> > (Exynos MIC) is on port 0 and the panel is @0, what imho might cause
> > the issue.
> >
> > I've tried to change in in-bridge ('mic_to_dsi') port to 1 in
> > exynos5433.dtsi. Then the panel has been attached:
> >
> > exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach] Attached s6e3hf2
> > device
> >
> > but the display is still not working, probably due to lack of proper
> > Exynos MIC handling. I will investigate it later and let You know.
>
>
> I've played a bit with the Exynos DRM code and finally I made it working
> on TM2(e). There are basically 3 different issues that need to be fixed
> to get it working with the $subject patchset:
>
> 1. Port numbers in exynos5433 dsi/dts are broken. For all pre-Exynos5433
> boards the panel was defined as a DSI node child (at 'reg 0'), what
> means it used port 0. Then, Exynos5433 introduced so called RGB-in at
> port 0 and panel at port 1 (as described in the dt bindings). However
> the committed Exynos5433 dtsi and TM2(e) dts still defined panel as a
> DSI child (with reg=0, so port 0) and Exynos MIC as of-graph at port 0.
> The Exynos DSI code however always searched for a panel as a DSI child
> node, so it worked fine, even though the panel and exynos mic used in
> fact the 'port 0'. IMHO this can be fixed by the following patch:
>
> diff --git a/arch/arm64/boot/dts/exynos/exynos5433.dtsi
> b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
> index bfe4ed8a23d6..2718c752d916 100644
> --- a/arch/arm64/boot/dts/exynos/exynos5433.dtsi
> +++ b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
> @@ -1046,8 +1046,8 @@
>                                  #address-cells = <1>;
>                                  #size-cells = <0>;
>
> -                               port@0 {
> -                                       reg = <0>;
> +                               port@1 {
> +                                       reg = <1>;
>                                          dsi_to_mic: endpoint {
>                                                  remote-endpoint =
> <&mic_to_dsi>;
>                                          };
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index d2933a70c01f..e8e2df339c5f 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -220,8 +220,8 @@ enum exynos_dsi_transfer_type {
>   };
>
>   enum {
> -       DSI_PORT_IN,
> -       DSI_PORT_OUT
> +       DSI_PORT_OUT,
> +       DSI_PORT_IN
>   };
>
>   struct exynos_dsi_transfer {
> --
>

port@1 of mic is invalid, it is input to DSI so it has to be port@0
with proper pipeline on dts file can fix the things.

--- a/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts
+++ b/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts
@@ -51,6 +51,16 @@ &cmu_disp {
 };

 &dsi {
+       ports {
+               port@1 {
+                       reg = <1>;
+
+                       dsi_out_panel: endpoint {
+                               remote-endpoint = <&dsi_in_panel>;
+                       };
+               };
+       };
+
        panel@0 {
                compatible = "samsung,s6e3hf2";
                reg = <0>;
@@ -58,6 +68,12 @@ panel@0 {
                vci-supply = <&ldo28_reg>;
                reset-gpios = <&gpg0 0 GPIO_ACTIVE_LOW>;
                enable-gpios = <&gpf1 5 GPIO_ACTIVE_HIGH>;
+
+               port {
+                       dsi_in_panel: endpoint {
+                               remote-endpoint = <&dsi_out_panel>;
+                       };
+               };
        };
 };

> 2. (devm_)drm_of_get_bridge() ignores panel's 'reg' property and it is
> unable to find a panel if there is a 'ports' child node (even if it
> describes completely different port than the one requested by the
> caller). I don't have time now to fix this, a simple and ugly workaround
> for that is to define the complete of-graph for the DSI child panel:
>
> diff --git a/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts
> b/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts
> index 22d26460f3dd..4726c325bd2c 100644
> --- a/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts
> +++ b/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts
> @@ -58,6 +58,27 @@
>                  vci-supply = <&ldo28_reg>;
>                  reset-gpios = <&gpg0 0 GPIO_ACTIVE_LOW>;
>                  enable-gpios = <&gpf1 5 GPIO_ACTIVE_HIGH>;
> +
> +               ports {
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +
> +                       port@0 {
> +                               reg = <0>;
> +                               panel_to_dsi: endpoint {
> +                                       remote-endpoint = <&dsi_to_panel>;
> +                               };
> +                       };
> +               };
> +       };
> +
> +       ports {
> +               port@0 {
> +                       reg = <0>;
> +                       dsi_to_panel: endpoint {
> +                               remote-endpoint = <&panel_to_dsi>;
> +                       };
> +               };
>          };
>   };
>
> --
>
> 3. Lack of proper handling of the 'in-bridge' in the Exynos DSI after
> Your conversion. Initially I thought that this could be fixed with the
> following simple patch:
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index e8e2df339c5f..2fe9c995549f 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -259,6 +259,7 @@ struct exynos_dsi {
>          struct mipi_dsi_host dsi_host;
>          struct drm_bridge bridge;
>          struct drm_bridge *out_bridge;
> +       struct drm_bridge *in_bridge;
>          struct device *dev;
>
>          void __iomem *reg_base;
> @@ -1535,7 +1536,7 @@ static int exynos_dsi_host_attach(struct
> mipi_dsi_host *host,
>
>          drm_bridge_add(&dsi->bridge);
>
> -       drm_bridge_attach(encoder, &dsi->bridge, NULL, 0);
> +       drm_bridge_attach(encoder, &dsi->bridge, dsi->in_bridge, 0);
>
>          /*
>           * This is a temporary solution and should be made by more
> generic way.
> @@ -1674,7 +1675,6 @@ static int exynos_dsi_bind(struct device *dev,
> struct device *master,
>          struct drm_encoder *encoder = &dsi->encoder;
>          struct drm_device *drm_dev = data;
>          struct device_node *in_bridge_node;
> -       struct drm_bridge *in_bridge;
>          int ret;
>
>          printk("drm: %s enter\n", __func__);
> @@ -1688,9 +1688,10 @@ static int exynos_dsi_bind(struct device *dev,
> struct device *master,
>          }
>          in_bridge_node = of_graph_get_remote_node(dev->of_node,
> DSI_PORT_IN, 0);
>          if (in_bridge_node) {
> -               in_bridge = of_drm_find_bridge(in_bridge_node);
> -               if (in_bridge)
> -                       drm_bridge_attach(encoder, in_bridge, NULL, 0);
> +               dsi->in_bridge = of_drm_find_bridge(in_bridge_node);
> +               if (dsi->in_bridge) {
> +                       drm_bridge_attach(encoder, dsi->in_bridge, NULL, 0);
> +               }
>                  of_node_put(in_bridge_node);
>          }
>
> --
>
> but it turned out that in such case the order of the bridge 'enable'
> calls is not correct for display pipeline operation. The one that
> actually works is (sorry for the hacky code):
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index e8e2df339c5f..51b568556fce 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -259,6 +259,7 @@ struct exynos_dsi {
>          struct mipi_dsi_host dsi_host;
>          struct drm_bridge bridge;
>          struct drm_bridge *out_bridge;
> +       struct drm_bridge *in_bridge;
>          struct device *dev;
>
>          void __iomem *reg_base;
> @@ -1424,6 +1425,9 @@ static void exynos_dsi_atomic_pre_enable(struct
> drm_bridge *bridge,
>
>          dsi->state |= DSIM_STATE_ENABLED;
>          printk("drm: %s finish: %d\n", __func__, 0);
> +
> +       if (dsi->in_bridge)
> + dsi->in_bridge->funcs->pre_enable(dsi->in_bridge);
>   }
>
>   static void exynos_dsi_atomic_enable(struct drm_bridge *bridge,
> @@ -1439,6 +1443,9 @@ static void exynos_dsi_atomic_enable(struct
> drm_bridge *bridge,
>
>          printk("drm: %s finish: %d\n", __func__, 0);
>
> +       if (dsi->in_bridge)
> + dsi->in_bridge->funcs->enable(dsi->in_bridge);
> +
>          return;
>   }
>
> @@ -1455,6 +1462,8 @@ static void exynos_dsi_atomic_disable(struct
> drm_bridge *bridge,
>          dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
>          printk("drm: %s finish: %d\n", __func__, 0);
>
> +       if (dsi->in_bridge)
> + dsi->in_bridge->funcs->disable(dsi->in_bridge);
>   }
>
>   static void exynos_dsi_atomic_post_disable(struct drm_bridge *bridge,
> @@ -1470,6 +1479,8 @@ static void exynos_dsi_atomic_post_disable(struct
> drm_bridge *bridge,
>
>          printk("drm: %s finish: %d\n", __func__, 0);
>
> +       if (dsi->in_bridge)
> + dsi->in_bridge->funcs->post_disable(dsi->in_bridge);
>   }
>
>   static void exynos_dsi_mode_set(struct drm_bridge *bridge,
> @@ -1482,6 +1493,10 @@ static void exynos_dsi_mode_set(struct drm_bridge
> *bridge,
>
>          drm_mode_copy(&dsi->mode, adjusted_mode);
>          printk("drm: %s finish: %d\n", __func__, ret);
> +
> +       if (dsi->in_bridge)
> + dsi->in_bridge->funcs->mode_set(dsi->in_bridge, mode,
> +                                               adjusted_mode);
>   }
>
>   static int exynos_dsi_attach(struct drm_bridge *bridge,
>   @@ -1674,7 +1689,6 @@ static int exynos_dsi_bind(struct device *dev,
> struct device *master,
>          struct drm_encoder *encoder = &dsi->encoder;
>          struct drm_device *drm_dev = data;
>          struct device_node *in_bridge_node;
> -       struct drm_bridge *in_bridge;
>          int ret;
>
>          printk("drm: %s enter\n", __func__);
> @@ -1688,9 +1702,11 @@ static int exynos_dsi_bind(struct device *dev,
> struct device *master,
>          }
>          in_bridge_node = of_graph_get_remote_node(dev->of_node,
> DSI_PORT_IN, 0);
>          if (in_bridge_node) {
> -               in_bridge = of_drm_find_bridge(in_bridge_node);
> -               if (in_bridge)
> -                       drm_bridge_attach(encoder, in_bridge, NULL, 0);
> +               dsi->in_bridge = of_drm_find_bridge(in_bridge_node);
> +               if (dsi->in_bridge) {
> +                       drm_bridge_attach(encoder, dsi->in_bridge, NULL, 0);
> + INIT_LIST_HEAD(&encoder->bridge_chain);
> +               }
>                  of_node_put(in_bridge_node);
>          }
>
> --

I'm not quite sure of having in_bridge require explicit chain
handling. It is the job of drm code as it finds the chain and invoke
it. On the other hand, if it MIC is registered as bridge then the
respective bridge calls has to be in their driver.

Let me fix the exynos5433 and send apart of the same series. then we
can continue to see the trouble necks.

will that be fine?

Thanks,
Jagan.

Re: [PATCH v4 0/6] drm: exynos: dsi: Convert drm bridge

[Jagan Teki]

Hi Andrzej,

On Tue, Dec 28, 2021 at 4:18 PM Andrzej Hajda <andrzej.hajda@intel.com> wrote:
>
> Hi Marek,
>
> On 23.12.2021 10:15, Marek Szyprowski wrote:
> > Hi Jagan,
> >
> > On 18.12.2021 00:16, Marek Szyprowski wrote:
> >> On 15.12.2021 15:56, Jagan Teki wrote:
> >>> On Wed, Dec 15, 2021 at 7:49 PM Marek Szyprowski
> >>> <m.szyprowski@samsung.com> wrote:
> >>>> On 15.12.2021 13:57, Jagan Teki wrote:
> >>>>> On Wed, Dec 15, 2021 at 5:31 PM Marek Szyprowski
> >>>>> <m.szyprowski@samsung.com> wrote:
> >>>>>> On 15.12.2021 11:15, Jagan Teki wrote:
> >>>>>>> Updated series about drm bridge conversion of exynos dsi.
> >>>>>>> Previous version can be accessible, here [1].
> >>>>>>>
> >>>>>>> Patch 1: connector reset
> >>>>>>>
> >>>>>>> Patch 2: panel_bridge API
> >>>>>>>
> >>>>>>> Patch 3: Bridge conversion
> >>>>>>>
> >>>>>>> Patch 4: Atomic functions
> >>>>>>>
> >>>>>>> Patch 5: atomic_set
> >>>>>>>
> >>>>>>> Patch 6: DSI init in enable
> >>>>>> There is a little progress! :)
> >>>>>>
> >>>>>> Devices with a simple display pipeline (only a DSI panel, like
> >>>>>> Trats/Trats2) works till the last patch. Then, after applying
> >>>>>> ("[PATCH
> >>>>>> v4 6/6] drm: exynos: dsi: Move DSI init in bridge enable"), I get no
> >>>>>> display at all.
> >>>>>>
> >>>>>> A TM2e board with in-bridge (Exynos MIC) stops displaying anything
> >>>>>> after
> >>>>>> applying patch "[PATCH v4 2/6] drm: exynos: dsi: Use drm
> >>>>>> panel_bridge API".
> >>>>>>
> >>>>>> In case of the Arndale board with tc358764 bridge, no much
> >>>>>> progress. The
> >>>>>> display is broken just after applying the "[PATCH v2] drm: bridge:
> >>>>>> tc358764: Use drm panel_bridge API" patch on top of linux-next.
> >>>>>>
> >>>>>> In all cases the I had "drm: of: Lookup if child node has panel or
> >>>>>> bridge" patch applied.
> >>>>> Just skip the 6/6 for now.
> >>>>>
> >>>>> Apply
> >>>>> -
> >>>>> https://protect2.fireeye.com/v1/url?k=a24f3f76-fdd40659-a24eb439-0cc47a31cdf8-97ea12b4c5258d11&q=1&e=37a169bf-7ca5-4362-aad7-486018c7a708&u=https%3A%2F%2Fpatchwork.amarulasolutions.com%2Fpatch%2F1825%2F
> >>>>> -
> >>>>> https://protect2.fireeye.com/v1/url?k=a226360f-fdbd0f20-a227bd40-0cc47a31cdf8-ebd66aebee1058d7&q=1&e=37a169bf-7ca5-4362-aad7-486018c7a708&u=https%3A%2F%2Fpatchwork.amarulasolutions.com%2Fpatch%2F1823%2F
> >>>>>
> >>>>> Then apply 1/6 to 5/6.  and update the status?
> >>>> Okay, my fault, I didn't check that case on Arndale.
> >>>>
> >>>> I've checked and indeed, Trats/Trats2 and Arndale works after the above
> >>>> 2 patches AND patches 1-5.
> >>>>
> >>>> The only problem is now on TM2e, which uses Exynos MIC as in-bridge for
> >>>> Exynos DSI:
> >>>>
> >>>> [    4.068866] [drm] Exynos DRM: using 13800000.decon device for DMA
> >>>> mapping operations
> >>>> [    4.069183] exynos-drm exynos-drm: bound 13800000.decon (ops
> >>>> decon_component_ops)
> >>>> [    4.128983] exynos-drm exynos-drm: bound 13880000.decon (ops
> >>>> decon_component_ops)
> >>>> [    4.129261] exynos-drm exynos-drm: bound 13930000.mic (ops
> >>>> exynos_mic_component_ops)
> >>>> [    4.133508] exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach]
> >>>> *ERROR* failed to find the bridge: -19
> >>>> [    4.136392] exynos-drm exynos-drm: bound 13900000.dsi (ops
> >>>> exynos_dsi_component_ops)
> >>>> [    4.145499] rc_core: Couldn't load IR keymap rc-cec
> >>>> [    4.145666] Registered IR keymap rc-empty
> >>>> [    4.148402] rc rc0: sii8620 as /devices/virtual/rc/rc0
> >>>> [    4.156051] input: sii8620 as /devices/virtual/rc/rc0/input1
> >>>> [    4.160647] exynos-drm exynos-drm: bound 13970000.hdmi (ops
> >>>> hdmi_component_ops)
> >>>> [    4.169923] exynos-drm exynos-drm: [drm] Cannot find any crtc or
> >>>> sizes
> >>>> [    4.173958] exynos-drm exynos-drm: [drm] Cannot find any crtc or
> >>>> sizes
> >>>> [    4.182304] [drm] Initialized exynos 1.1.0 20180330 for
> >>>> exynos-drm on
> >>>> minor 0
> >>>>
> >>>> The display pipeline for TM2e is:
> >>>>
> >>>> Exynos5433 Decon -> Exynos MIC -> Exynos DSI -> s6e3ha2 DSI panel
> >>> If Trats/Trats2 is working then it has to work. I don't see any
> >>> difference in output pipeline. Can you please share the full log, I
> >>> cannot see host_attach print saying "Attached.."
> >> Well, there is a failure message about the panel:
> >>
> >> exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach] *ERROR* failed
> >> to find the bridge: -19
> >>
> >> however it looks that something might be broken in dts. The in-bridge
> >> (Exynos MIC) is on port 0 and the panel is @0, what imho might cause
> >> the issue.
> >>
> >> I've tried to change in in-bridge ('mic_to_dsi') port to 1 in
> >> exynos5433.dtsi. Then the panel has been attached:
> >>
> >> exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach] Attached s6e3hf2
> >> device
> >>
> >> but the display is still not working, probably due to lack of proper
> >> Exynos MIC handling. I will investigate it later and let You know.
> >
> > I've played a bit with the Exynos DRM code and finally I made it working
> > on TM2(e). There are basically 3 different issues that need to be fixed
> > to get it working with the $subject patchset:
> >
> > 1. Port numbers in exynos5433 dsi/dts are broken. For all pre-Exynos5433
> > boards the panel was defined as a DSI node child (at 'reg 0'),
>
>
> True, emphasis that it is reg 0 of DSI bus.
>
>
> >   what
> > means it used port 0.
>
>
> And this does not seems true to me. Established practice is (unless I
> have not noticed change in bindings :) ) that in case of data bus shared
> with control bus the port node is optional. In such case host knows
> already who is connected to its data bus, it does not need port node.
> And if there is no port node there is no port number as well.
>
> As I quickly looked at the exynos bindings they seems generally OK to
> me, if there is something wrong/suspicious let me know.
>
>
> >   Then, Exynos5433 introduced so called RGB-in at
> > port 0 and panel at port 1 (as described in the dt bindings).However
> > the committed Exynos5433 dtsi and TM2(e) dts still defined panel as a
> > DSI child (with reg=0, so port 0) and Exynos MIC as of-graph at port 0.
> > The Exynos DSI code however always searched for a panel as a DSI child
> > node, so it worked fine, even though the panel and exynos mic used in
> > fact the 'port 0'. IMHO this can be fixed by the following patch:
> >
> > diff --git a/arch/arm64/boot/dts/exynos/exynos5433.dtsi
> > b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
> > index bfe4ed8a23d6..2718c752d916 100644
> > --- a/arch/arm64/boot/dts/exynos/exynos5433.dtsi
> > +++ b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
> > @@ -1046,8 +1046,8 @@
> >                                   #address-cells = <1>;
> >                                   #size-cells = <0>;
> >
> > -                               port@0 {
> > -                                       reg = <0>;
> > +                               port@1 {
> > +                                       reg = <1>;
> >                                           dsi_to_mic: endpoint {
> >                                                   remote-endpoint =
> > <&mic_to_dsi>;
> >                                           };
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > index d2933a70c01f..e8e2df339c5f 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > @@ -220,8 +220,8 @@ enum exynos_dsi_transfer_type {
> >    };
> >
> >    enum {
> > -       DSI_PORT_IN,
> > -       DSI_PORT_OUT
> > +       DSI_PORT_OUT,
> > +       DSI_PORT_IN
> >    };
> >
> >    struct exynos_dsi_transfer {
> > --
> >
> > 2. (devm_)drm_of_get_bridge() ignores panel's 'reg' property and it is
>
>
> I guess drm_of_get_bridge should not be used in exynos_dsi_host_attach
> at all - there are no ports here, only of_node of the sink.
>
> Since there is no helper to workaround the dualism panel/bridge you
> should still use of_drm_find_bridge and of_drm_find_panel pair.

We have 2 use cases so far for adding input and outputs for a given host node.

1. with ports

    dsi {
            compatible = "samsung,exynos5433-mipi-dsi";
            #address-cells = <1>;
            #size-cells = <0>;

             ports {
                                #address-cells = <1>;
                                #size-cells = <0>;

                                port@0 {
                                        reg = <0>;
                                        dsi_to_mic: endpoint {
                                                remote-endpoint = <&mic_to_dsi>;
                                        };
                                };

                           port@1 {
                                       reg = <1>;

                                      dsi_out_panel: endpoint {
                                                remote-endpoint =
<&dsi_in_panel>;
                                      };
                             };
            };

            panel@0 {
                    compatible = "samsung,s6e3hf2";
                    reg = <0>;
                    vdd3-supply = <&ldo27_reg>;
                    vci-supply = <&ldo28_reg>;
                    reset-gpios = <&gpg0 0 GPIO_ACTIVE_LOW>;
                    enable-gpios = <&gpf1 5 GPIO_ACTIVE_HIGH>;

                    port {
                                    dsi_in_panel: endpoint {
                                           remote-endpoint = <&dsi_out_panel>;
                                     };
                    };
            };
    };


2. with port

    dsi {
            compatible = "samsung,exynos5433-mipi-dsi";
            #address-cells = <1>;
            #size-cells = <0>;

            port {
                    dsi_to_mic: endpoint {
                            remote-endpoint = <&mic_to_dsi>;
                    };
            };

            panel@0 {
                    compatible = "samsung,s6e3hf2";
                    reg = <0>;
                    vdd3-supply = <&ldo27_reg>;
                    vci-supply = <&ldo28_reg>;
                    reset-gpios = <&gpg0 0 GPIO_ACTIVE_LOW>;
                    enable-gpios = <&gpf1 5 GPIO_ACTIVE_HIGH>;
            };
    };

We have a patch which do find the panel/bridge as a child node[1] via
devm_drm_of_get_bridge. However that based on the above use cases
where child panel/bridge added as per 2 use case and there is no
possibility of child node in 1 use case as it has a feasibility to add
outputs via 'ports'.

Since exynos5433 has 'ports' in host node, this patches [2] added
panel via port@1.

[1] https://patchwork.amarulasolutions.com/patch/1823/
[2] https://patchwork.amarulasolutions.com/patch/1836/

Thanks,
Jagan.

Re: [PATCH v4 0/6] drm: exynos: dsi: Convert drm bridge

[Andrzej Hajda]

Hi Jagan,

On 11.01.2022 10:32, Jagan Teki wrote:
> Hi Andrzej,
>
> On Tue, Dec 28, 2021 at 4:18 PM Andrzej Hajda <andrzej.hajda@intel.com> wrote:
>> Hi Marek,
>>
>> On 23.12.2021 10:15, Marek Szyprowski wrote:
>>> Hi Jagan,
>>>
>>> On 18.12.2021 00:16, Marek Szyprowski wrote:
>>>> On 15.12.2021 15:56, Jagan Teki wrote:
>>>>> On Wed, Dec 15, 2021 at 7:49 PM Marek Szyprowski
>>>>> <m.szyprowski@samsung.com> wrote:
>>>>>> On 15.12.2021 13:57, Jagan Teki wrote:
>>>>>>> On Wed, Dec 15, 2021 at 5:31 PM Marek Szyprowski
>>>>>>> <m.szyprowski@samsung.com> wrote:
>>>>>>>> On 15.12.2021 11:15, Jagan Teki wrote:
>>>>>>>>> Updated series about drm bridge conversion of exynos dsi.
>>>>>>>>> Previous version can be accessible, here [1].
>>>>>>>>>
>>>>>>>>> Patch 1: connector reset
>>>>>>>>>
>>>>>>>>> Patch 2: panel_bridge API
>>>>>>>>>
>>>>>>>>> Patch 3: Bridge conversion
>>>>>>>>>
>>>>>>>>> Patch 4: Atomic functions
>>>>>>>>>
>>>>>>>>> Patch 5: atomic_set
>>>>>>>>>
>>>>>>>>> Patch 6: DSI init in enable
>>>>>>>> There is a little progress! :)
>>>>>>>>
>>>>>>>> Devices with a simple display pipeline (only a DSI panel, like
>>>>>>>> Trats/Trats2) works till the last patch. Then, after applying
>>>>>>>> ("[PATCH
>>>>>>>> v4 6/6] drm: exynos: dsi: Move DSI init in bridge enable"), I get no
>>>>>>>> display at all.
>>>>>>>>
>>>>>>>> A TM2e board with in-bridge (Exynos MIC) stops displaying anything
>>>>>>>> after
>>>>>>>> applying patch "[PATCH v4 2/6] drm: exynos: dsi: Use drm
>>>>>>>> panel_bridge API".
>>>>>>>>
>>>>>>>> In case of the Arndale board with tc358764 bridge, no much
>>>>>>>> progress. The
>>>>>>>> display is broken just after applying the "[PATCH v2] drm: bridge:
>>>>>>>> tc358764: Use drm panel_bridge API" patch on top of linux-next.
>>>>>>>>
>>>>>>>> In all cases the I had "drm: of: Lookup if child node has panel or
>>>>>>>> bridge" patch applied.
>>>>>>> Just skip the 6/6 for now.
>>>>>>>
>>>>>>> Apply
>>>>>>> -
>>>>>>> https://protect2.fireeye.com/v1/url?k=a24f3f76-fdd40659-a24eb439-0cc47a31cdf8-97ea12b4c5258d11&q=1&e=37a169bf-7ca5-4362-aad7-486018c7a708&u=https%3A%2F%2Fpatchwork.amarulasolutions.com%2Fpatch%2F1825%2F
>>>>>>> -
>>>>>>> https://protect2.fireeye.com/v1/url?k=a226360f-fdbd0f20-a227bd40-0cc47a31cdf8-ebd66aebee1058d7&q=1&e=37a169bf-7ca5-4362-aad7-486018c7a708&u=https%3A%2F%2Fpatchwork.amarulasolutions.com%2Fpatch%2F1823%2F
>>>>>>>
>>>>>>> Then apply 1/6 to 5/6.  and update the status?
>>>>>> Okay, my fault, I didn't check that case on Arndale.
>>>>>>
>>>>>> I've checked and indeed, Trats/Trats2 and Arndale works after the above
>>>>>> 2 patches AND patches 1-5.
>>>>>>
>>>>>> The only problem is now on TM2e, which uses Exynos MIC as in-bridge for
>>>>>> Exynos DSI:
>>>>>>
>>>>>> [    4.068866] [drm] Exynos DRM: using 13800000.decon device for DMA
>>>>>> mapping operations
>>>>>> [    4.069183] exynos-drm exynos-drm: bound 13800000.decon (ops
>>>>>> decon_component_ops)
>>>>>> [    4.128983] exynos-drm exynos-drm: bound 13880000.decon (ops
>>>>>> decon_component_ops)
>>>>>> [    4.129261] exynos-drm exynos-drm: bound 13930000.mic (ops
>>>>>> exynos_mic_component_ops)
>>>>>> [    4.133508] exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach]
>>>>>> *ERROR* failed to find the bridge: -19
>>>>>> [    4.136392] exynos-drm exynos-drm: bound 13900000.dsi (ops
>>>>>> exynos_dsi_component_ops)
>>>>>> [    4.145499] rc_core: Couldn't load IR keymap rc-cec
>>>>>> [    4.145666] Registered IR keymap rc-empty
>>>>>> [    4.148402] rc rc0: sii8620 as /devices/virtual/rc/rc0
>>>>>> [    4.156051] input: sii8620 as /devices/virtual/rc/rc0/input1
>>>>>> [    4.160647] exynos-drm exynos-drm: bound 13970000.hdmi (ops
>>>>>> hdmi_component_ops)
>>>>>> [    4.169923] exynos-drm exynos-drm: [drm] Cannot find any crtc or
>>>>>> sizes
>>>>>> [    4.173958] exynos-drm exynos-drm: [drm] Cannot find any crtc or
>>>>>> sizes
>>>>>> [    4.182304] [drm] Initialized exynos 1.1.0 20180330 for
>>>>>> exynos-drm on
>>>>>> minor 0
>>>>>>
>>>>>> The display pipeline for TM2e is:
>>>>>>
>>>>>> Exynos5433 Decon -> Exynos MIC -> Exynos DSI -> s6e3ha2 DSI panel
>>>>> If Trats/Trats2 is working then it has to work. I don't see any
>>>>> difference in output pipeline. Can you please share the full log, I
>>>>> cannot see host_attach print saying "Attached.."
>>>> Well, there is a failure message about the panel:
>>>>
>>>> exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach] *ERROR* failed
>>>> to find the bridge: -19
>>>>
>>>> however it looks that something might be broken in dts. The in-bridge
>>>> (Exynos MIC) is on port 0 and the panel is @0, what imho might cause
>>>> the issue.
>>>>
>>>> I've tried to change in in-bridge ('mic_to_dsi') port to 1 in
>>>> exynos5433.dtsi. Then the panel has been attached:
>>>>
>>>> exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach] Attached s6e3hf2
>>>> device
>>>>
>>>> but the display is still not working, probably due to lack of proper
>>>> Exynos MIC handling. I will investigate it later and let You know.
>>> I've played a bit with the Exynos DRM code and finally I made it working
>>> on TM2(e). There are basically 3 different issues that need to be fixed
>>> to get it working with the $subject patchset:
>>>
>>> 1. Port numbers in exynos5433 dsi/dts are broken. For all pre-Exynos5433
>>> boards the panel was defined as a DSI node child (at 'reg 0'),
>>
>> True, emphasis that it is reg 0 of DSI bus.
>>
>>
>>>    what
>>> means it used port 0.
>>
>> And this does not seems true to me. Established practice is (unless I
>> have not noticed change in bindings :) ) that in case of data bus shared
>> with control bus the port node is optional. In such case host knows
>> already who is connected to its data bus, it does not need port node.
>> And if there is no port node there is no port number as well.
>>
>> As I quickly looked at the exynos bindings they seems generally OK to
>> me, if there is something wrong/suspicious let me know.
>>
>>
>>>    Then, Exynos5433 introduced so called RGB-in at
>>> port 0 and panel at port 1 (as described in the dt bindings).However
>>> the committed Exynos5433 dtsi and TM2(e) dts still defined panel as a
>>> DSI child (with reg=0, so port 0) and Exynos MIC as of-graph at port 0.
>>> The Exynos DSI code however always searched for a panel as a DSI child
>>> node, so it worked fine, even though the panel and exynos mic used in
>>> fact the 'port 0'. IMHO this can be fixed by the following patch:
>>>
>>> diff --git a/arch/arm64/boot/dts/exynos/exynos5433.dtsi
>>> b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
>>> index bfe4ed8a23d6..2718c752d916 100644
>>> --- a/arch/arm64/boot/dts/exynos/exynos5433.dtsi
>>> +++ b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
>>> @@ -1046,8 +1046,8 @@
>>>                                    #address-cells = <1>;
>>>                                    #size-cells = <0>;
>>>
>>> -                               port@0 {
>>> -                                       reg = <0>;
>>> +                               port@1 {
>>> +                                       reg = <1>;
>>>                                            dsi_to_mic: endpoint {
>>>                                                    remote-endpoint =
>>> <&mic_to_dsi>;
>>>                                            };
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> index d2933a70c01f..e8e2df339c5f 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> @@ -220,8 +220,8 @@ enum exynos_dsi_transfer_type {
>>>     };
>>>
>>>     enum {
>>> -       DSI_PORT_IN,
>>> -       DSI_PORT_OUT
>>> +       DSI_PORT_OUT,
>>> +       DSI_PORT_IN
>>>     };
>>>
>>>     struct exynos_dsi_transfer {
>>> --
>>>
>>> 2. (devm_)drm_of_get_bridge() ignores panel's 'reg' property and it is
>>
>> I guess drm_of_get_bridge should not be used in exynos_dsi_host_attach
>> at all - there are no ports here, only of_node of the sink.
>>
>> Since there is no helper to workaround the dualism panel/bridge you
>> should still use of_drm_find_bridge and of_drm_find_panel pair.
> We have 2 use cases so far for adding input and outputs for a given host node.
>
> 1. with ports
>
>      dsi {
>              compatible = "samsung,exynos5433-mipi-dsi";
>              #address-cells = <1>;
>              #size-cells = <0>;
>
>               ports {
>                                  #address-cells = <1>;
>                                  #size-cells = <0>;
>
>                                  port@0 {
>                                          reg = <0>;
>                                          dsi_to_mic: endpoint {
>                                                  remote-endpoint = <&mic_to_dsi>;
>                                          };
>                                  };
>
>                             port@1 {
>                                         reg = <1>;
>
>                                        dsi_out_panel: endpoint {
>                                                  remote-endpoint =
> <&dsi_in_panel>;
>                                        };
>                               };
>              };
>
>              panel@0 {
>                      compatible = "samsung,s6e3hf2";
>                      reg = <0>;
>                      vdd3-supply = <&ldo27_reg>;
>                      vci-supply = <&ldo28_reg>;
>                      reset-gpios = <&gpg0 0 GPIO_ACTIVE_LOW>;
>                      enable-gpios = <&gpf1 5 GPIO_ACTIVE_HIGH>;
>
>                      port {
>                                      dsi_in_panel: endpoint {
>                                             remote-endpoint = <&dsi_out_panel>;
>                                       };
>                      };
>              };
>      };
>
>
> 2. with port
>
>      dsi {
>              compatible = "samsung,exynos5433-mipi-dsi";
>              #address-cells = <1>;
>              #size-cells = <0>;
>
>              port {
>                      dsi_to_mic: endpoint {
>                              remote-endpoint = <&mic_to_dsi>;
>                      };
>              };
>
>              panel@0 {
>                      compatible = "samsung,s6e3hf2";
>                      reg = <0>;
>                      vdd3-supply = <&ldo27_reg>;
>                      vci-supply = <&ldo28_reg>;
>                      reset-gpios = <&gpg0 0 GPIO_ACTIVE_LOW>;
>                      enable-gpios = <&gpf1 5 GPIO_ACTIVE_HIGH>;
>              };
>      };
>
> We have a patch which do find the panel/bridge as a child node[1] via
> devm_drm_of_get_bridge. However that based on the above use cases
> where child panel/bridge added as per 2 use case and there is no
> possibility of child node in 1 use case as it has a feasibility to add
> outputs via 'ports'.
>
> Since exynos5433 has 'ports' in host node, this patches [2] added
> panel via port@1.
>
> [1] https://patchwork.amarulasolutions.com/patch/1823/
> [2] https://patchwork.amarulasolutions.com/patch/1836/


Maybe I am missing something, but you do not have to 'find' 
panel/bridge, you have it already - it is 'device' argument of 
exynos_dsi_host_attach.

For me it looks odd when panel calls 'hey I am here, ready to proceed' 
(exynos_dsi_host_attach callback), and then dsi host ignores this call, 
instead it look for panel using different mechanism.


Regards

Andrzej


>
> Thanks,
> Jagan.

Re: [PATCH v4 0/6] drm: exynos: dsi: Convert drm bridge

[Jagan Teki]

On Tue, Jan 11, 2022 at 5:20 PM Andrzej Hajda <andrzej.hajda@intel.com> wrote:
>
> Hi Jagan,
>
> On 11.01.2022 10:32, Jagan Teki wrote:
> > Hi Andrzej,
> >
> > On Tue, Dec 28, 2021 at 4:18 PM Andrzej Hajda <andrzej.hajda@intel.com> wrote:
> >> Hi Marek,
> >>
> >> On 23.12.2021 10:15, Marek Szyprowski wrote:
> >>> Hi Jagan,
> >>>
> >>> On 18.12.2021 00:16, Marek Szyprowski wrote:
> >>>> On 15.12.2021 15:56, Jagan Teki wrote:
> >>>>> On Wed, Dec 15, 2021 at 7:49 PM Marek Szyprowski
> >>>>> <m.szyprowski@samsung.com> wrote:
> >>>>>> On 15.12.2021 13:57, Jagan Teki wrote:
> >>>>>>> On Wed, Dec 15, 2021 at 5:31 PM Marek Szyprowski
> >>>>>>> <m.szyprowski@samsung.com> wrote:
> >>>>>>>> On 15.12.2021 11:15, Jagan Teki wrote:
> >>>>>>>>> Updated series about drm bridge conversion of exynos dsi.
> >>>>>>>>> Previous version can be accessible, here [1].
> >>>>>>>>>
> >>>>>>>>> Patch 1: connector reset
> >>>>>>>>>
> >>>>>>>>> Patch 2: panel_bridge API
> >>>>>>>>>
> >>>>>>>>> Patch 3: Bridge conversion
> >>>>>>>>>
> >>>>>>>>> Patch 4: Atomic functions
> >>>>>>>>>
> >>>>>>>>> Patch 5: atomic_set
> >>>>>>>>>
> >>>>>>>>> Patch 6: DSI init in enable
> >>>>>>>> There is a little progress! :)
> >>>>>>>>
> >>>>>>>> Devices with a simple display pipeline (only a DSI panel, like
> >>>>>>>> Trats/Trats2) works till the last patch. Then, after applying
> >>>>>>>> ("[PATCH
> >>>>>>>> v4 6/6] drm: exynos: dsi: Move DSI init in bridge enable"), I get no
> >>>>>>>> display at all.
> >>>>>>>>
> >>>>>>>> A TM2e board with in-bridge (Exynos MIC) stops displaying anything
> >>>>>>>> after
> >>>>>>>> applying patch "[PATCH v4 2/6] drm: exynos: dsi: Use drm
> >>>>>>>> panel_bridge API".
> >>>>>>>>
> >>>>>>>> In case of the Arndale board with tc358764 bridge, no much
> >>>>>>>> progress. The
> >>>>>>>> display is broken just after applying the "[PATCH v2] drm: bridge:
> >>>>>>>> tc358764: Use drm panel_bridge API" patch on top of linux-next.
> >>>>>>>>
> >>>>>>>> In all cases the I had "drm: of: Lookup if child node has panel or
> >>>>>>>> bridge" patch applied.
> >>>>>>> Just skip the 6/6 for now.
> >>>>>>>
> >>>>>>> Apply
> >>>>>>> -
> >>>>>>> https://protect2.fireeye.com/v1/url?k=a24f3f76-fdd40659-a24eb439-0cc47a31cdf8-97ea12b4c5258d11&q=1&e=37a169bf-7ca5-4362-aad7-486018c7a708&u=https%3A%2F%2Fpatchwork.amarulasolutions.com%2Fpatch%2F1825%2F
> >>>>>>> -
> >>>>>>> https://protect2.fireeye.com/v1/url?k=a226360f-fdbd0f20-a227bd40-0cc47a31cdf8-ebd66aebee1058d7&q=1&e=37a169bf-7ca5-4362-aad7-486018c7a708&u=https%3A%2F%2Fpatchwork.amarulasolutions.com%2Fpatch%2F1823%2F
> >>>>>>>
> >>>>>>> Then apply 1/6 to 5/6.  and update the status?
> >>>>>> Okay, my fault, I didn't check that case on Arndale.
> >>>>>>
> >>>>>> I've checked and indeed, Trats/Trats2 and Arndale works after the above
> >>>>>> 2 patches AND patches 1-5.
> >>>>>>
> >>>>>> The only problem is now on TM2e, which uses Exynos MIC as in-bridge for
> >>>>>> Exynos DSI:
> >>>>>>
> >>>>>> [    4.068866] [drm] Exynos DRM: using 13800000.decon device for DMA
> >>>>>> mapping operations
> >>>>>> [    4.069183] exynos-drm exynos-drm: bound 13800000.decon (ops
> >>>>>> decon_component_ops)
> >>>>>> [    4.128983] exynos-drm exynos-drm: bound 13880000.decon (ops
> >>>>>> decon_component_ops)
> >>>>>> [    4.129261] exynos-drm exynos-drm: bound 13930000.mic (ops
> >>>>>> exynos_mic_component_ops)
> >>>>>> [    4.133508] exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach]
> >>>>>> *ERROR* failed to find the bridge: -19
> >>>>>> [    4.136392] exynos-drm exynos-drm: bound 13900000.dsi (ops
> >>>>>> exynos_dsi_component_ops)
> >>>>>> [    4.145499] rc_core: Couldn't load IR keymap rc-cec
> >>>>>> [    4.145666] Registered IR keymap rc-empty
> >>>>>> [    4.148402] rc rc0: sii8620 as /devices/virtual/rc/rc0
> >>>>>> [    4.156051] input: sii8620 as /devices/virtual/rc/rc0/input1
> >>>>>> [    4.160647] exynos-drm exynos-drm: bound 13970000.hdmi (ops
> >>>>>> hdmi_component_ops)
> >>>>>> [    4.169923] exynos-drm exynos-drm: [drm] Cannot find any crtc or
> >>>>>> sizes
> >>>>>> [    4.173958] exynos-drm exynos-drm: [drm] Cannot find any crtc or
> >>>>>> sizes
> >>>>>> [    4.182304] [drm] Initialized exynos 1.1.0 20180330 for
> >>>>>> exynos-drm on
> >>>>>> minor 0
> >>>>>>
> >>>>>> The display pipeline for TM2e is:
> >>>>>>
> >>>>>> Exynos5433 Decon -> Exynos MIC -> Exynos DSI -> s6e3ha2 DSI panel
> >>>>> If Trats/Trats2 is working then it has to work. I don't see any
> >>>>> difference in output pipeline. Can you please share the full log, I
> >>>>> cannot see host_attach print saying "Attached.."
> >>>> Well, there is a failure message about the panel:
> >>>>
> >>>> exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach] *ERROR* failed
> >>>> to find the bridge: -19
> >>>>
> >>>> however it looks that something might be broken in dts. The in-bridge
> >>>> (Exynos MIC) is on port 0 and the panel is @0, what imho might cause
> >>>> the issue.
> >>>>
> >>>> I've tried to change in in-bridge ('mic_to_dsi') port to 1 in
> >>>> exynos5433.dtsi. Then the panel has been attached:
> >>>>
> >>>> exynos-dsi 13900000.dsi: [drm:exynos_dsi_host_attach] Attached s6e3hf2
> >>>> device
> >>>>
> >>>> but the display is still not working, probably due to lack of proper
> >>>> Exynos MIC handling. I will investigate it later and let You know.
> >>> I've played a bit with the Exynos DRM code and finally I made it working
> >>> on TM2(e). There are basically 3 different issues that need to be fixed
> >>> to get it working with the $subject patchset:
> >>>
> >>> 1. Port numbers in exynos5433 dsi/dts are broken. For all pre-Exynos5433
> >>> boards the panel was defined as a DSI node child (at 'reg 0'),
> >>
> >> True, emphasis that it is reg 0 of DSI bus.
> >>
> >>
> >>>    what
> >>> means it used port 0.
> >>
> >> And this does not seems true to me. Established practice is (unless I
> >> have not noticed change in bindings :) ) that in case of data bus shared
> >> with control bus the port node is optional. In such case host knows
> >> already who is connected to its data bus, it does not need port node.
> >> And if there is no port node there is no port number as well.
> >>
> >> As I quickly looked at the exynos bindings they seems generally OK to
> >> me, if there is something wrong/suspicious let me know.
> >>
> >>
> >>>    Then, Exynos5433 introduced so called RGB-in at
> >>> port 0 and panel at port 1 (as described in the dt bindings).However
> >>> the committed Exynos5433 dtsi and TM2(e) dts still defined panel as a
> >>> DSI child (with reg=0, so port 0) and Exynos MIC as of-graph at port 0.
> >>> The Exynos DSI code however always searched for a panel as a DSI child
> >>> node, so it worked fine, even though the panel and exynos mic used in
> >>> fact the 'port 0'. IMHO this can be fixed by the following patch:
> >>>
> >>> diff --git a/arch/arm64/boot/dts/exynos/exynos5433.dtsi
> >>> b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
> >>> index bfe4ed8a23d6..2718c752d916 100644
> >>> --- a/arch/arm64/boot/dts/exynos/exynos5433.dtsi
> >>> +++ b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
> >>> @@ -1046,8 +1046,8 @@
> >>>                                    #address-cells = <1>;
> >>>                                    #size-cells = <0>;
> >>>
> >>> -                               port@0 {
> >>> -                                       reg = <0>;
> >>> +                               port@1 {
> >>> +                                       reg = <1>;
> >>>                                            dsi_to_mic: endpoint {
> >>>                                                    remote-endpoint =
> >>> <&mic_to_dsi>;
> >>>                                            };
> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> >>> b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> >>> index d2933a70c01f..e8e2df339c5f 100644
> >>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> >>> @@ -220,8 +220,8 @@ enum exynos_dsi_transfer_type {
> >>>     };
> >>>
> >>>     enum {
> >>> -       DSI_PORT_IN,
> >>> -       DSI_PORT_OUT
> >>> +       DSI_PORT_OUT,
> >>> +       DSI_PORT_IN
> >>>     };
> >>>
> >>>     struct exynos_dsi_transfer {
> >>> --
> >>>
> >>> 2. (devm_)drm_of_get_bridge() ignores panel's 'reg' property and it is
> >>
> >> I guess drm_of_get_bridge should not be used in exynos_dsi_host_attach
> >> at all - there are no ports here, only of_node of the sink.
> >>
> >> Since there is no helper to workaround the dualism panel/bridge you
> >> should still use of_drm_find_bridge and of_drm_find_panel pair.
> > We have 2 use cases so far for adding input and outputs for a given host node.
> >
> > 1. with ports
> >
> >      dsi {
> >              compatible = "samsung,exynos5433-mipi-dsi";
> >              #address-cells = <1>;
> >              #size-cells = <0>;
> >
> >               ports {
> >                                  #address-cells = <1>;
> >                                  #size-cells = <0>;
> >
> >                                  port@0 {
> >                                          reg = <0>;
> >                                          dsi_to_mic: endpoint {
> >                                                  remote-endpoint = <&mic_to_dsi>;
> >                                          };
> >                                  };
> >
> >                             port@1 {
> >                                         reg = <1>;
> >
> >                                        dsi_out_panel: endpoint {
> >                                                  remote-endpoint =
> > <&dsi_in_panel>;
> >                                        };
> >                               };
> >              };
> >
> >              panel@0 {
> >                      compatible = "samsung,s6e3hf2";
> >                      reg = <0>;
> >                      vdd3-supply = <&ldo27_reg>;
> >                      vci-supply = <&ldo28_reg>;
> >                      reset-gpios = <&gpg0 0 GPIO_ACTIVE_LOW>;
> >                      enable-gpios = <&gpf1 5 GPIO_ACTIVE_HIGH>;
> >
> >                      port {
> >                                      dsi_in_panel: endpoint {
> >                                             remote-endpoint = <&dsi_out_panel>;
> >                                       };
> >                      };
> >              };
> >      };
> >
> >
> > 2. with port
> >
> >      dsi {
> >              compatible = "samsung,exynos5433-mipi-dsi";
> >              #address-cells = <1>;
> >              #size-cells = <0>;
> >
> >              port {
> >                      dsi_to_mic: endpoint {
> >                              remote-endpoint = <&mic_to_dsi>;
> >                      };
> >              };
> >
> >              panel@0 {
> >                      compatible = "samsung,s6e3hf2";
> >                      reg = <0>;
> >                      vdd3-supply = <&ldo27_reg>;
> >                      vci-supply = <&ldo28_reg>;
> >                      reset-gpios = <&gpg0 0 GPIO_ACTIVE_LOW>;
> >                      enable-gpios = <&gpf1 5 GPIO_ACTIVE_HIGH>;
> >              };
> >      };
> >
> > We have a patch which do find the panel/bridge as a child node[1] via
> > devm_drm_of_get_bridge. However that based on the above use cases
> > where child panel/bridge added as per 2 use case and there is no
> > possibility of child node in 1 use case as it has a feasibility to add
> > outputs via 'ports'.
> >
> > Since exynos5433 has 'ports' in host node, this patches [2] added
> > panel via port@1.
> >
> > [1] https://patchwork.amarulasolutions.com/patch/1823/
> > [2] https://patchwork.amarulasolutions.com/patch/1836/
>
>
> Maybe I am missing something, but you do not have to 'find'
> panel/bridge, you have it already - it is 'device' argument of
> exynos_dsi_host_attach.
>
> For me it looks odd when panel calls 'hey I am here, ready to proceed'
> (exynos_dsi_host_attach callback), and then dsi host ignores this call,
> instead it look for panel using different mechanism.

From the DSI we didn't know the given 'device' is panel/bridge right?
so instead of doing it via of_drm_find_panel/bridge we are trying to
call the common devm_drm_of_get_bridge which would compatible to other
ports based (non-child) OF graphs example i.MX8MM.

Idea is to have common finding call to make compatible between platforms.

Thanks,
Jagan.