[PATCH V2 0/3] DSI host and peripheral initialisation ordering

[PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Dave Stevenson]

Hi All

Changes from v1:
- New patch to refactor drm_bridge_chain_post_disable and drm_bridge_chain_pre_enable
  to reuse drm_atomic_bridge_chain_post_disable / drm_atomic_bridge_chain_pre_enable
  but with a NULL state.
- New patch that adds a pre_enable_upstream_first to drm_panel.
- changed from an OPS flag to a bool "pre_enable_upstream_first" in drm_bridge.
- Followed Andrzej's suggestion of using continue in the main loop to avoid
  needing 2 additional loops (one forward to find the last bridge wanting
  upstream first, and the second backwards again).
- Actioned Laurent's review comments on docs patch.

Original cover letter: 

Hopefully I've cc'ed all those that have bashed this problem around previously,
or are otherwise linked to DRM bridges.

There have been numerous discussions around how DSI support is currently broken
as it doesn't support initialising the PHY to LP-11 and potentially the clock
lane to HS prior to configuring the DSI peripheral. There is no op where the
interface is initialised but HS video isn't also being sent.
Currently you have:
- peripheral pre_enable (host not initialised yet)
- host pre_enable
- encoder enable
- host enable
- peripheral enable (video already running)

vc4 and exynos currently implement the DSI host as an encoder, and split the
bridge_chain. This fails if you want to switch to being a bridge and/or use
atomic calls as the state of all the elements split off are not added by
drm_atomic_add_encoder_bridges.

dw-mipi-dsi[1] and now msm[2] use the mode_set hook to initialise the PHY, so
the bridge/panel pre_enable can send commands. In their post_disable they then
call the downstream bridge/panel post_disable op manually so that shutdown
commands can be sent before shutting down the PHY. Nothing handles that fact,
so the framework then continues down the bridge chain and calls the post_disable
again, so we get unbalanced panel prepare/unprepare calls being reported [3].

There have been patches[4] proposing reversing the entire direction of
pre_enable and post_disable, but that risks driving voltage into devices that
have yet to be powered up.
There have been discussions about adding either a pre_pre_enable, or adding a
DSI host_op to initialise the host[5]. Both require significant reworking to all
existing drivers in moving initialisation phases.
We have patches that look like they may well be addressing race conditions in
starting up a DSI peripheral[6].

This patch takes a hybrid of the two: an optional reversing of the order for
specific links within the bridge chain within pre_enable and post_disable done
within the drm_bridge framework.
I'm more than happy to move where the flag exists in structures (currently as
DRM_BRIDGE_OP_UPSTREAM_FIRST in drm_bridge_ops, but it isn't an op), but does
this solve the problem posed? If not, then can you describe the actual scenario
it doesn't cover?
A DSI peripheral can set the flag to get the DSI host initialised first, and
therefore it has a stable LP-11 state before pre_enable. Likewise the peripheral
can still send shutdown commands prior to the DSI host being shut down in
post_disable. It also handles the case where there are multiple devices in the
chain that all want their upstream bridge enabled first, so should there be a
DSI mux between host and peripheral, then it can still get the host to the
correct state.

An example tree is at [7] which is drm-misc-next with these patches and then a
conversion of vc4_dsi to use the atomic bridge functions (will be upstreamed
once we're over this hurdle). It is working happily with the Toshiba TC358762 on
a Raspberry Pi 7" panel.
The same approach but on our vendor 5.15 tree[8] has also been tested
successfully on a TI SN65DSI83 and LVDS panel.

Whilst here, I've also documented the expected behaviour of DSI hosts and
peripherals to aid those who come along after.

Thanks
  Dave

[1] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L940
[2] https://lists.freedesktop.org/archives/dri-devel/2022-January/337769.html
[3] https://lists.freedesktop.org/archives/dri-devel/2021-December/333908.html
[4] https://lists.freedesktop.org/archives/dri-devel/2021-October/328476.html
[5] https://lists.freedesktop.org/archives/dri-devel/2021-October/325853.html
[6] https://lists.freedesktop.org/archives/dri-devel/2022-February/341852.html
[7] https://github.com/6by9/linux/tree/drm-misc-next-vc4_dsi
[8] https://github.com/6by9/linux/tree/rpi-5.15.y-sn65dsi83

Dave Stevenson (4):
  drm/bridge: Remove duplication from drm_bridge and drm_atomic_bridge
    chains
  drm/bridge: Introduce pre_enable_upstream_first to alter bridge init
    order
  drm/panel: Add prepare_upstream_first flag to drm_panel
  drm/bridge: Document the expected behaviour of DSI host controllers

 Documentation/gpu/drm-kms-helpers.rst |   7 ++
 drivers/gpu/drm/bridge/panel.c        |   3 +
 drivers/gpu/drm/drm_bridge.c          | 181 ++++++++++++++++++++++++----------
 include/drm/drm_bridge.h              |   8 ++
 include/drm/drm_panel.h               |  10 ++
 5 files changed, 159 insertions(+), 50 deletions(-)

-- 
2.7.4

[PATCH V2 1/4] drm/bridge: Remove duplication from drm_bridge and drm_atomic_bridge chains

[Dave Stevenson]

drm_bridge_chain_pre_enable is a subset of
drm_atomic_bridge_chain_pre_enable, and drm_bridge_chain_post_disable
is a subset of drm_atomic_bridge_chain_post_disable.

Change drm_bridge_chain_pre_enable and drm_bridge_chain_post_disable to
call the atomic versions with a NULL state, and ensure that atomic
calls are not made if there is no state.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/gpu/drm/drm_bridge.c | 30 ++++--------------------------
 1 file changed, 4 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index c96847fc0ebc..198fd471a488 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -527,16 +527,7 @@ EXPORT_SYMBOL(drm_bridge_chain_disable);
  */
 void drm_bridge_chain_post_disable(struct drm_bridge *bridge)
 {
-	struct drm_encoder *encoder;
-
-	if (!bridge)
-		return;
-
-	encoder = bridge->encoder;
-	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
-		if (bridge->funcs->post_disable)
-			bridge->funcs->post_disable(bridge);
-	}
+	drm_atomic_bridge_chain_post_disable(bridge, NULL);
 }
 EXPORT_SYMBOL(drm_bridge_chain_post_disable);
 
@@ -582,20 +573,7 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set);
  */
 void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
 {
-	struct drm_encoder *encoder;
-	struct drm_bridge *iter;
-
-	if (!bridge)
-		return;
-
-	encoder = bridge->encoder;
-	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
-		if (iter->funcs->pre_enable)
-			iter->funcs->pre_enable(iter);
-
-		if (iter == bridge)
-			break;
-	}
+	drm_atomic_bridge_chain_pre_enable(bridge, NULL);
 }
 EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
 
@@ -690,7 +668,7 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
 
 	encoder = bridge->encoder;
 	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
-		if (bridge->funcs->atomic_post_disable) {
+		if (old_state && bridge->funcs->atomic_post_disable) {
 			struct drm_bridge_state *old_bridge_state;
 
 			old_bridge_state =
@@ -732,7 +710,7 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
 
 	encoder = bridge->encoder;
 	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
-		if (iter->funcs->atomic_pre_enable) {
+		if (old_state && iter->funcs->atomic_pre_enable) {
 			struct drm_bridge_state *old_bridge_state;
 
 			old_bridge_state =
-- 
2.7.4

[PATCH V2 2/4] drm/bridge: Introduce pre_enable_upstream_first to alter bridge init order

[Dave Stevenson]

DSI sink devices typically want the DSI host powered up and configured
before they are powered up. pre_enable is the place this would normally
happen, but they are called in reverse order from panel/connector towards
the encoder, which is the "wrong" order.

Add a new flag pre_enable_upstream_first that any bridge can set
to swap the order of pre_enable (and post_disable) for that and the
immediately upstream bridge.
Should the immediately upstream bridge also set the
pre_enable_upstream_first flag, the bridge upstream of that will be called
before either of those which requested pre_enable_upstream_first.

eg:
- Panel
- Bridge 1
- Bridge 2 pre_enable_upstream_first
- Bridge 3
- Bridge 4 pre_enable_upstream_first
- Bridge 5 pre_enable_upstream_first
- Bridge 6
- Encoder
Would result in pre_enable's being called as Panel, Bridge 1, Bridge 3,
Bridge 2, Bridge 6, Bridge 5, Bridge 4, Encoder.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/gpu/drm/drm_bridge.c | 116 +++++++++++++++++++++++++++++++++----------
 include/drm/drm_bridge.h     |   8 +++
 2 files changed, 98 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 198fd471a488..70b513f5ce0d 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -523,6 +523,10 @@ EXPORT_SYMBOL(drm_bridge_chain_disable);
  * encoder chain, starting from the first bridge to the last. These are called
  * after completing the encoder's prepare op.
  *
+ * If a bridge sets @pre_enable_upstream_first, then the @post_disable for that
+ * bridge will be called before the previous one to reverse the @pre_enable
+ * calling direction.
+ *
  * Note: the bridge passed should be the one closest to the encoder
  */
 void drm_bridge_chain_post_disable(struct drm_bridge *bridge)
@@ -569,6 +573,9 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set);
  * chain, starting from the last bridge to the first. These are called
  * before calling the encoder's commit op.
  *
+ * If a bridge sets @pre_enable_upstream_first, then the @pre_enable for the
+ * previous bridge will be called before @pre_enable of this bridge.
+ *
  * Note: the bridge passed should be the one closest to the encoder
  */
 void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
@@ -645,6 +652,25 @@ void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge,
 }
 EXPORT_SYMBOL(drm_atomic_bridge_chain_disable);
 
+static void drm_atomic_bridge_call_post_disable(struct drm_bridge *bridge,
+						struct drm_atomic_state *old_state)
+{
+	if (old_state && bridge->funcs->atomic_post_disable) {
+		struct drm_bridge_state *old_bridge_state;
+
+		old_bridge_state =
+			drm_atomic_get_old_bridge_state(old_state,
+							bridge);
+		if (WARN_ON(!old_bridge_state))
+			return;
+
+		bridge->funcs->atomic_post_disable(bridge,
+						   old_bridge_state);
+	} else if (bridge->funcs->post_disable) {
+		bridge->funcs->post_disable(bridge);
+	}
+}
+
 /**
  * drm_atomic_bridge_chain_post_disable - cleans up after disabling all bridges
  *					  in the encoder chain
@@ -655,6 +681,9 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_disable);
  * &drm_bridge_funcs.post_disable) op for all the bridges in the encoder chain,
  * starting from the first bridge to the last. These are called after completing
  * &drm_encoder_helper_funcs.atomic_disable
+ * If a bridge sets @pre_enable_upstream_first, then the @post_disable for that
+ * bridge will be called before the previous one to reverse the @pre_enable
+ * calling direction.
  *
  * Note: the bridge passed should be the one closest to the encoder
  */
@@ -662,30 +691,55 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
 					  struct drm_atomic_state *old_state)
 {
 	struct drm_encoder *encoder;
+	struct drm_bridge *prev, *tmp;
 
 	if (!bridge)
 		return;
 
 	encoder = bridge->encoder;
-	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
-		if (old_state && bridge->funcs->atomic_post_disable) {
-			struct drm_bridge_state *old_bridge_state;
 
-			old_bridge_state =
-				drm_atomic_get_old_bridge_state(old_state,
-								bridge);
-			if (WARN_ON(!old_bridge_state))
-				return;
+	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
+		if (!list_is_last(&bridge->chain_node, &encoder->bridge_chain) &&
+		    list_next_entry(bridge, chain_node)->pre_enable_upstream_first)
+			/* Skip bridges where the downstream bridge wanted
+			 * pre_enable after / post_disable before the upstream
+			 * bridge.
+			 */
+			continue;
 
-			bridge->funcs->atomic_post_disable(bridge,
-							   old_bridge_state);
-		} else if (bridge->funcs->post_disable) {
-			bridge->funcs->post_disable(bridge);
-		}
+		/* Call this bridge and any skipped ones in reverse order */
+		tmp = bridge;
+		do {
+			prev = tmp;
+			drm_atomic_bridge_call_post_disable(tmp, old_state);
+			if (tmp == list_first_entry(&encoder->bridge_chain,
+						    struct drm_bridge, chain_node))
+				tmp = NULL;
+			else
+				tmp = list_prev_entry(tmp, chain_node);
+		} while (tmp && prev->pre_enable_upstream_first);
 	}
 }
 EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
 
+static void drm_atomic_bridge_call_pre_enable(struct drm_bridge *bridge,
+					      struct drm_atomic_state *old_state)
+{
+	if (old_state && bridge->funcs->atomic_pre_enable) {
+		struct drm_bridge_state *old_bridge_state;
+
+		old_bridge_state =
+			drm_atomic_get_old_bridge_state(old_state,
+							bridge);
+		if (WARN_ON(!old_bridge_state))
+			return;
+
+		bridge->funcs->atomic_pre_enable(bridge, old_bridge_state);
+	} else if (bridge->funcs->pre_enable) {
+		bridge->funcs->pre_enable(bridge);
+	}
+}
+
 /**
  * drm_atomic_bridge_chain_pre_enable - prepares for enabling all bridges in
  *					the encoder chain
@@ -697,32 +751,42 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
  * starting from the last bridge to the first. These are called before calling
  * &drm_encoder_helper_funcs.atomic_enable
  *
+ * If a bridge sets @pre_enable_upstream_first, then the pre_enable for the
+ * upstream bridge will be called before pre_enable of this bridge.
+ *
  * Note: the bridge passed should be the one closest to the encoder
  */
 void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
 					struct drm_atomic_state *old_state)
 {
 	struct drm_encoder *encoder;
-	struct drm_bridge *iter;
+	struct drm_bridge *iter, *tmp;
 
 	if (!bridge)
 		return;
 
 	encoder = bridge->encoder;
-	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
-		if (old_state && iter->funcs->atomic_pre_enable) {
-			struct drm_bridge_state *old_bridge_state;
 
-			old_bridge_state =
-				drm_atomic_get_old_bridge_state(old_state,
-								iter);
-			if (WARN_ON(!old_bridge_state))
-				return;
+	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
+		if (iter->pre_enable_upstream_first && iter != bridge)
+			/* Skip bridges which want the upstream pre_enable
+			 * called before their pre_enable.
+			 */
+			continue;
 
-			iter->funcs->atomic_pre_enable(iter, old_bridge_state);
-		} else if (iter->funcs->pre_enable) {
-			iter->funcs->pre_enable(iter);
-		}
+		tmp = iter;
+		do {
+			/* Work forward through the current bridge, and any
+			 * that had been skipped.
+			 */
+			drm_atomic_bridge_call_pre_enable(tmp, old_state);
+			if (tmp == list_last_entry(&encoder->bridge_chain,
+						   struct drm_bridge,
+						   chain_node))
+				tmp = NULL;
+			else
+				tmp = list_next_entry(tmp, chain_node);
+		} while (tmp && tmp->pre_enable_upstream_first);
 
 		if (iter == bridge)
 			break;
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index f27b4060faa2..cf1fb3ad7054 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -769,6 +769,14 @@ struct drm_bridge {
 	 */
 	bool interlace_allowed;
 	/**
+	 * @pre_enable_upstream_first: The bridge requires that the upstream
+	 * bridge @pre_enable function is called before its @pre_enable,
+	 * and conversely for post_disable. This is most frequently a
+	 * requirement for DSI devices which need the host to be initialised
+	 * before the peripheral.
+	 */
+	bool pre_enable_upstream_first;
+	/**
 	 * @ddc: Associated I2C adapter for DDC access, if any.
 	 */
 	struct i2c_adapter *ddc;
-- 
2.7.4

[PATCH V2 3/4] drm/panel: Add prepare_upstream_first flag to drm_panel

[Dave Stevenson]

Mapping to the drm_bridge flag pre_enable_upstream_first,
add a new flag prepare_upstream_first to drm_panel to allow
the panel driver to request that the upstream bridge should
be pre_enabled before the panel prepare.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/gpu/drm/bridge/panel.c |  3 +++
 include/drm/drm_panel.h        | 10 ++++++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index 5be057575183..2ea08b3ba326 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -234,6 +234,9 @@ struct drm_bridge *drm_panel_bridge_add_typed(struct drm_panel *panel,
 	panel_bridge->bridge.ops = DRM_BRIDGE_OP_MODES;
 	panel_bridge->bridge.type = connector_type;
 
+	panel_bridge->bridge.pre_enable_upstream_first =
+						panel->prepare_upstream_first;
+
 	drm_bridge_add(&panel_bridge->bridge);
 
 	return &panel_bridge->bridge;
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index 1ba2d424a53f..c0f39dfbd071 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -179,6 +179,16 @@ struct drm_panel {
 	 * Panel entry in registry.
 	 */
 	struct list_head list;
+
+	/**
+	 * @prepare_upstream_first:
+	 *
+	 * The upstream controller should be prepared first, before the prepare
+	 * for the panel is called. This is largely required for DSI panels
+	 * where the DSI host controller should be initialised to LP-11 before
+	 * the panel is powered up.
+	 */
+	bool prepare_upstream_first;
 };
 
 void drm_panel_init(struct drm_panel *panel, struct device *dev,
-- 
2.7.4

[PATCH V2 4/4] drm/bridge: Document the expected behaviour of DSI host controllers

[Dave Stevenson]

The exact behaviour of DSI host controllers is not specified,
therefore define it.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 Documentation/gpu/drm-kms-helpers.rst |  7 +++++++
 drivers/gpu/drm/drm_bridge.c          | 39 +++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
index c3ce91eecbc1..362afdb867c6 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -185,6 +185,13 @@ Bridge Helper Reference
 .. kernel-doc:: drivers/gpu/drm/drm_bridge.c
    :export:
 
+MIPI-DSI bridge operation
+-------------------------
+
+.. kernel-doc:: drivers/gpu/drm/drm_bridge.c
+   :doc: dsi bridge operations
+
+
 Bridge Connector Helper Reference
 ---------------------------------
 
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 70b513f5ce0d..32def1be682a 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -152,6 +152,45 @@
  * situation when probing.
  */
 
+/**
+ * DOC: dsi bridge operations
+ *
+ * DSI host interfaces are expected to be implemented as bridges rather than
+ * encoders, however there are a few aspects of their operation that need to
+ * be defined in order to provide a consistent interface.
+ *
+ * A DSI host should keep the PHY powered down until the pre_enable operation is
+ * called. All lanes are in an undefined idle state up to this point, and it
+ * must not be assumed that it is LP-11.
+ * pre_enable should initialise the PHY, set the data lanes to LP-11, and the
+ * clock lane to either LP-11 or HS depending on the mode_flag
+ * %MIPI_DSI_CLOCK_NON_CONTINUOUS.
+ *
+ * Ordinarily the downstream bridge DSI peripheral pre_enable will have been
+ * called before the DSI host. If the DSI peripheral requires LP-11 and/or
+ * the clock lane to be in HS mode prior to pre_enable, then it can set the
+ * &pre_enable_upstream_first flag to request the pre_enable (and
+ * post_disable) order to be altered to enable the DSI host first.
+ *
+ * Either the CRTC being enabled, or the DSI host enable operation should switch
+ * the host to actively transmitting video on the data lanes.
+ *
+ * The reverse also applies. The DSI host disable operation or stopping the CRTC
+ * should stop transmitting video, and the data lanes should return to the LP-11
+ * state. The DSI host &post_disable operation should disable the PHY.
+ * If the &pre_enable_upstream_first flag is set, then the DSI peripheral's
+ * bridge &post_disable will be called before the DSI host's post_disable.
+ *
+ * Whilst it is valid to call &host_transfer prior to pre_enable or after
+ * post_disable, the exact state of the lanes is undefined at this point. The
+ * DSI host should initialise the interface, transmit the data, and then disable
+ * the interface again.
+ *
+ * Ultra Low Power State (ULPS) is not explicitly supported by DRM. If
+ * implemented, it therefore needs to be handled entirely within the DSI Host
+ * driver.
+ */
+
 static DEFINE_MUTEX(bridge_lock);
 static LIST_HEAD(bridge_list);
 
-- 
2.7.4

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Dave Stevenson]

On Fri, 4 Mar 2022 at 15:18, Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi All

A gentle ping on this series. Any comments on the approach?
Thanks.

> Changes from v1:
> - New patch to refactor drm_bridge_chain_post_disable and drm_bridge_chain_pre_enable
>   to reuse drm_atomic_bridge_chain_post_disable / drm_atomic_bridge_chain_pre_enable
>   but with a NULL state.
> - New patch that adds a pre_enable_upstream_first to drm_panel.
> - changed from an OPS flag to a bool "pre_enable_upstream_first" in drm_bridge.
> - Followed Andrzej's suggestion of using continue in the main loop to avoid
>   needing 2 additional loops (one forward to find the last bridge wanting
>   upstream first, and the second backwards again).
> - Actioned Laurent's review comments on docs patch.
>
> Original cover letter:
>
> Hopefully I've cc'ed all those that have bashed this problem around previously,
> or are otherwise linked to DRM bridges.
>
> There have been numerous discussions around how DSI support is currently broken
> as it doesn't support initialising the PHY to LP-11 and potentially the clock
> lane to HS prior to configuring the DSI peripheral. There is no op where the
> interface is initialised but HS video isn't also being sent.
> Currently you have:
> - peripheral pre_enable (host not initialised yet)
> - host pre_enable
> - encoder enable
> - host enable
> - peripheral enable (video already running)
>
> vc4 and exynos currently implement the DSI host as an encoder, and split the
> bridge_chain. This fails if you want to switch to being a bridge and/or use
> atomic calls as the state of all the elements split off are not added by
> drm_atomic_add_encoder_bridges.
>
> dw-mipi-dsi[1] and now msm[2] use the mode_set hook to initialise the PHY, so
> the bridge/panel pre_enable can send commands. In their post_disable they then
> call the downstream bridge/panel post_disable op manually so that shutdown
> commands can be sent before shutting down the PHY. Nothing handles that fact,
> so the framework then continues down the bridge chain and calls the post_disable
> again, so we get unbalanced panel prepare/unprepare calls being reported [3].
>
> There have been patches[4] proposing reversing the entire direction of
> pre_enable and post_disable, but that risks driving voltage into devices that
> have yet to be powered up.
> There have been discussions about adding either a pre_pre_enable, or adding a
> DSI host_op to initialise the host[5]. Both require significant reworking to all
> existing drivers in moving initialisation phases.
> We have patches that look like they may well be addressing race conditions in
> starting up a DSI peripheral[6].
>
> This patch takes a hybrid of the two: an optional reversing of the order for
> specific links within the bridge chain within pre_enable and post_disable done
> within the drm_bridge framework.
> I'm more than happy to move where the flag exists in structures (currently as
> DRM_BRIDGE_OP_UPSTREAM_FIRST in drm_bridge_ops, but it isn't an op), but does
> this solve the problem posed? If not, then can you describe the actual scenario
> it doesn't cover?
> A DSI peripheral can set the flag to get the DSI host initialised first, and
> therefore it has a stable LP-11 state before pre_enable. Likewise the peripheral
> can still send shutdown commands prior to the DSI host being shut down in
> post_disable. It also handles the case where there are multiple devices in the
> chain that all want their upstream bridge enabled first, so should there be a
> DSI mux between host and peripheral, then it can still get the host to the
> correct state.
>
> An example tree is at [7] which is drm-misc-next with these patches and then a
> conversion of vc4_dsi to use the atomic bridge functions (will be upstreamed
> once we're over this hurdle). It is working happily with the Toshiba TC358762 on
> a Raspberry Pi 7" panel.
> The same approach but on our vendor 5.15 tree[8] has also been tested
> successfully on a TI SN65DSI83 and LVDS panel.
>
> Whilst here, I've also documented the expected behaviour of DSI hosts and
> peripherals to aid those who come along after.
>
> Thanks
>   Dave
>
> [1] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L940
> [2] https://lists.freedesktop.org/archives/dri-devel/2022-January/337769.html
> [3] https://lists.freedesktop.org/archives/dri-devel/2021-December/333908.html
> [4] https://lists.freedesktop.org/archives/dri-devel/2021-October/328476.html
> [5] https://lists.freedesktop.org/archives/dri-devel/2021-October/325853.html
> [6] https://lists.freedesktop.org/archives/dri-devel/2022-February/341852.html
> [7] https://github.com/6by9/linux/tree/drm-misc-next-vc4_dsi
> [8] https://github.com/6by9/linux/tree/rpi-5.15.y-sn65dsi83
>
> Dave Stevenson (4):
>   drm/bridge: Remove duplication from drm_bridge and drm_atomic_bridge
>     chains
>   drm/bridge: Introduce pre_enable_upstream_first to alter bridge init
>     order
>   drm/panel: Add prepare_upstream_first flag to drm_panel
>   drm/bridge: Document the expected behaviour of DSI host controllers
>
>  Documentation/gpu/drm-kms-helpers.rst |   7 ++
>  drivers/gpu/drm/bridge/panel.c        |   3 +
>  drivers/gpu/drm/drm_bridge.c          | 181 ++++++++++++++++++++++++----------
>  include/drm/drm_bridge.h              |   8 ++
>  include/drm/drm_panel.h               |  10 ++
>  5 files changed, 159 insertions(+), 50 deletions(-)
>
> --
> 2.7.4
>

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Dave Stevenson]

On Fri, 18 Mar 2022 at 12:25, Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> On Fri, 4 Mar 2022 at 15:18, Dave Stevenson
> <dave.stevenson@raspberrypi.com> wrote:
> >
> > Hi All
>
> A gentle ping on this series. Any comments on the approach?
> Thanks.

I realise the merge window has just closed and therefore folks have
been busy, but no responses on this after a month?

Do I give up and submit a patch to document that DSI is broken and no one cares?

  Dave

> > Changes from v1:
> > - New patch to refactor drm_bridge_chain_post_disable and drm_bridge_chain_pre_enable
> >   to reuse drm_atomic_bridge_chain_post_disable / drm_atomic_bridge_chain_pre_enable
> >   but with a NULL state.
> > - New patch that adds a pre_enable_upstream_first to drm_panel.
> > - changed from an OPS flag to a bool "pre_enable_upstream_first" in drm_bridge.
> > - Followed Andrzej's suggestion of using continue in the main loop to avoid
> >   needing 2 additional loops (one forward to find the last bridge wanting
> >   upstream first, and the second backwards again).
> > - Actioned Laurent's review comments on docs patch.
> >
> > Original cover letter:
> >
> > Hopefully I've cc'ed all those that have bashed this problem around previously,
> > or are otherwise linked to DRM bridges.
> >
> > There have been numerous discussions around how DSI support is currently broken
> > as it doesn't support initialising the PHY to LP-11 and potentially the clock
> > lane to HS prior to configuring the DSI peripheral. There is no op where the
> > interface is initialised but HS video isn't also being sent.
> > Currently you have:
> > - peripheral pre_enable (host not initialised yet)
> > - host pre_enable
> > - encoder enable
> > - host enable
> > - peripheral enable (video already running)
> >
> > vc4 and exynos currently implement the DSI host as an encoder, and split the
> > bridge_chain. This fails if you want to switch to being a bridge and/or use
> > atomic calls as the state of all the elements split off are not added by
> > drm_atomic_add_encoder_bridges.
> >
> > dw-mipi-dsi[1] and now msm[2] use the mode_set hook to initialise the PHY, so
> > the bridge/panel pre_enable can send commands. In their post_disable they then
> > call the downstream bridge/panel post_disable op manually so that shutdown
> > commands can be sent before shutting down the PHY. Nothing handles that fact,
> > so the framework then continues down the bridge chain and calls the post_disable
> > again, so we get unbalanced panel prepare/unprepare calls being reported [3].
> >
> > There have been patches[4] proposing reversing the entire direction of
> > pre_enable and post_disable, but that risks driving voltage into devices that
> > have yet to be powered up.
> > There have been discussions about adding either a pre_pre_enable, or adding a
> > DSI host_op to initialise the host[5]. Both require significant reworking to all
> > existing drivers in moving initialisation phases.
> > We have patches that look like they may well be addressing race conditions in
> > starting up a DSI peripheral[6].
> >
> > This patch takes a hybrid of the two: an optional reversing of the order for
> > specific links within the bridge chain within pre_enable and post_disable done
> > within the drm_bridge framework.
> > I'm more than happy to move where the flag exists in structures (currently as
> > DRM_BRIDGE_OP_UPSTREAM_FIRST in drm_bridge_ops, but it isn't an op), but does
> > this solve the problem posed? If not, then can you describe the actual scenario
> > it doesn't cover?
> > A DSI peripheral can set the flag to get the DSI host initialised first, and
> > therefore it has a stable LP-11 state before pre_enable. Likewise the peripheral
> > can still send shutdown commands prior to the DSI host being shut down in
> > post_disable. It also handles the case where there are multiple devices in the
> > chain that all want their upstream bridge enabled first, so should there be a
> > DSI mux between host and peripheral, then it can still get the host to the
> > correct state.
> >
> > An example tree is at [7] which is drm-misc-next with these patches and then a
> > conversion of vc4_dsi to use the atomic bridge functions (will be upstreamed
> > once we're over this hurdle). It is working happily with the Toshiba TC358762 on
> > a Raspberry Pi 7" panel.
> > The same approach but on our vendor 5.15 tree[8] has also been tested
> > successfully on a TI SN65DSI83 and LVDS panel.
> >
> > Whilst here, I've also documented the expected behaviour of DSI hosts and
> > peripherals to aid those who come along after.
> >
> > Thanks
> >   Dave
> >
> > [1] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L940
> > [2] https://lists.freedesktop.org/archives/dri-devel/2022-January/337769.html
> > [3] https://lists.freedesktop.org/archives/dri-devel/2021-December/333908.html
> > [4] https://lists.freedesktop.org/archives/dri-devel/2021-October/328476.html
> > [5] https://lists.freedesktop.org/archives/dri-devel/2021-October/325853.html
> > [6] https://lists.freedesktop.org/archives/dri-devel/2022-February/341852.html
> > [7] https://github.com/6by9/linux/tree/drm-misc-next-vc4_dsi
> > [8] https://github.com/6by9/linux/tree/rpi-5.15.y-sn65dsi83
> >
> > Dave Stevenson (4):
> >   drm/bridge: Remove duplication from drm_bridge and drm_atomic_bridge
> >     chains
> >   drm/bridge: Introduce pre_enable_upstream_first to alter bridge init
> >     order
> >   drm/panel: Add prepare_upstream_first flag to drm_panel
> >   drm/bridge: Document the expected behaviour of DSI host controllers
> >
> >  Documentation/gpu/drm-kms-helpers.rst |   7 ++
> >  drivers/gpu/drm/bridge/panel.c        |   3 +
> >  drivers/gpu/drm/drm_bridge.c          | 181 ++++++++++++++++++++++++----------
> >  include/drm/drm_bridge.h              |   8 ++
> >  include/drm/drm_panel.h               |  10 ++
> >  5 files changed, 159 insertions(+), 50 deletions(-)
> >
> > --
> > 2.7.4
> >

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Marek Szyprowski]

Hi Dave,

On 05.04.2022 13:43, Dave Stevenson wrote:
> On Fri, 18 Mar 2022 at 12:25, Dave Stevenson
> <dave.stevenson@raspberrypi.com>  wrote:
>> On Fri, 4 Mar 2022 at 15:18, Dave Stevenson
>> <dave.stevenson@raspberrypi.com>  wrote:
>>> Hi All
>> A gentle ping on this series. Any comments on the approach?
>> Thanks.
> I realise the merge window has just closed and therefore folks have
> been busy, but no responses on this after a month?
>
> Do I give up and submit a patch to document that DSI is broken and no one cares?

Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI 
DSIM bridge' thread, otherwise I would miss it since I'm not involved 
much in the DRM development.

This resolves most of the issues in the Exynos DSI and its recent 
conversion to the drm bridge framework. I've added the needed 
prepare_upstream_first flags to the panels and everything works fine 
without the bridge chain order hacks.

Feel free to add:

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>


The only remaining thing to resolve is the moment of enabling DSI host. 
The proper sequence is:

1. host power on, 2. device power on, 3. host init, 4. device init, 5. 
video enable.

#1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so 
far done in the first host transfer call, which usually happens in 
panel's prepare, then the #4 happens. Then video enable is done in the 
enable callbacks.

Jagan wants to move it to the dsi host pre_enable() to let it work with 
DSI bridges controlled over different interfaces 
(https://lore.kernel.org/all/20220504114021.33265-6-jagan@amarulasolutions.com/ 
). This however fails on Exynos with DSI panels, because when dsi's 
pre_enable is called, the dsi device is not yet powered. I've discussed 
this with Andrzej Hajda and we came to the conclusion that this can be 
resolved by adding the init() callback to the struct mipi_dsi_host_ops. 
Then DSI client (next bridge or panel) would call it after powering self 
on, but before sending any DSI commands in its pre_enable/prepare functions.

I've prepared a prototype of such solution. This approach finally 
resolved all the initialization issues! The bridge chain finally matches 
the hardware, no hack are needed, and everything is controlled by the 
DRM core. This prototype also includes the Jagan's patches, which add 
IMX support to Samsung DSIM. If one is interested, here is my git repo 
with all the PoC patches:

https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework


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

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Marek Vasut]

On 5/11/22 16:58, Marek Szyprowski wrote:
> Hi Dave,
> 
> On 05.04.2022 13:43, Dave Stevenson wrote:
>> On Fri, 18 Mar 2022 at 12:25, Dave Stevenson
>> <dave.stevenson@raspberrypi.com>  wrote:
>>> On Fri, 4 Mar 2022 at 15:18, Dave Stevenson
>>> <dave.stevenson@raspberrypi.com>  wrote:
>>>> Hi All
>>> A gentle ping on this series. Any comments on the approach?
>>> Thanks.
>> I realise the merge window has just closed and therefore folks have
>> been busy, but no responses on this after a month?
>>
>> Do I give up and submit a patch to document that DSI is broken and no one cares?
> 
> Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI
> DSIM bridge' thread, otherwise I would miss it since I'm not involved
> much in the DRM development.
> 
> This resolves most of the issues in the Exynos DSI and its recent
> conversion to the drm bridge framework. I've added the needed
> prepare_upstream_first flags to the panels and everything works fine
> without the bridge chain order hacks.
> 
> Feel free to add:
> 
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> 
> 
> The only remaining thing to resolve is the moment of enabling DSI host.
> The proper sequence is:
> 
> 1. host power on, 2. device power on, 3. host init, 4. device init, 5.
> video enable.

+CC Raphael, STM32MP1 has similar topic.

> #1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so
> far done in the first host transfer call, which usually happens in
> panel's prepare, then the #4 happens. Then video enable is done in the
> enable callbacks.
> 
> Jagan wants to move it to the dsi host pre_enable() to let it work with
> DSI bridges controlled over different interfaces
> (https://lore.kernel.org/all/20220504114021.33265-6-jagan@amarulasolutions.com/
> ). This however fails on Exynos with DSI panels, because when dsi's
> pre_enable is called, the dsi device is not yet powered. I've discussed
> this with Andrzej Hajda and we came to the conclusion that this can be
> resolved by adding the init() callback to the struct mipi_dsi_host_ops.
> Then DSI client (next bridge or panel) would call it after powering self
> on, but before sending any DSI commands in its pre_enable/prepare functions.
> 
> I've prepared a prototype of such solution. This approach finally
> resolved all the initialization issues! The bridge chain finally matches
> the hardware, no hack are needed, and everything is controlled by the
> DRM core. This prototype also includes the Jagan's patches, which add
> IMX support to Samsung DSIM. If one is interested, here is my git repo
> with all the PoC patches:
> 
> https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework

Can you CC me on the iMX DSIM discussion/patches ? It seems I was left 
out of it all, even though I work with the iMX8M DRM stuff extensively.

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Dave Stevenson]

Hi Marek.

On Wed, 11 May 2022 at 15:58, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> Hi Dave,
>
> On 05.04.2022 13:43, Dave Stevenson wrote:
> > On Fri, 18 Mar 2022 at 12:25, Dave Stevenson
> > <dave.stevenson@raspberrypi.com>  wrote:
> >> On Fri, 4 Mar 2022 at 15:18, Dave Stevenson
> >> <dave.stevenson@raspberrypi.com>  wrote:
> >>> Hi All
> >> A gentle ping on this series. Any comments on the approach?
> >> Thanks.
> > I realise the merge window has just closed and therefore folks have
> > been busy, but no responses on this after a month?
> >
> > Do I give up and submit a patch to document that DSI is broken and no one cares?
>
> Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI
> DSIM bridge' thread, otherwise I would miss it since I'm not involved
> much in the DRM development.
>
> This resolves most of the issues in the Exynos DSI and its recent
> conversion to the drm bridge framework. I've added the needed
> prepare_upstream_first flags to the panels and everything works fine
> without the bridge chain order hacks.
>
> Feel free to add:
>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

Thanks for testing it. I was almost at the stage of abandoning the patch set.

> The only remaining thing to resolve is the moment of enabling DSI host.
> The proper sequence is:
>
> 1. host power on, 2. device power on, 3. host init, 4. device init, 5.
> video enable.
>
> #1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so
> far done in the first host transfer call, which usually happens in
> panel's prepare, then the #4 happens. Then video enable is done in the
> enable callbacks.

What's your definition of host power on and host init here? What state
are you defining the DSI interface to be in after each operation?

> Jagan wants to move it to the dsi host pre_enable() to let it work with
> DSI bridges controlled over different interfaces
> (https://lore.kernel.org/all/20220504114021.33265-6-jagan@amarulasolutions.com/
> ).

I think I'm in agreement with Jagan.
As documented in patch 4/4:
+ * A DSI host should keep the PHY powered down until the pre_enable
operation is
+ * called. All lanes are in an undefined idle state up to this point, and it
+ * must not be assumed that it is LP-11.
+ * pre_enable should initialise the PHY, set the data lanes to LP-11, and the
+ * clock lane to either LP-11 or HS depending on the mode_flag
+ * %MIPI_DSI_CLOCK_NON_CONTINUOUS.

> This however fails on Exynos with DSI panels, because when dsi's
> pre_enable is called, the dsi device is not yet powered.

Sorry, I'm not following what the issue is here? DSI lanes being at
LP-11 when the sink isn't powered, so potential for leakage through
the device?
In which case the device should NOT set pre_enable_upstream first, and
the host gets powered up and down with each host_transfer call the
device makes in pre_enable.

(Whilst I can't claim that I know of every single DSI device, most
datasheets I've encountered have required LP-11 on the lanes before
powering up the device).

> I've discussed
> this with Andrzej Hajda and we came to the conclusion that this can be
> resolved by adding the init() callback to the struct mipi_dsi_host_ops.
> Then DSI client (next bridge or panel) would call it after powering self
> on, but before sending any DSI commands in its pre_enable/prepare functions.

You may as well have a mipi_dsi_host_ops call to fully control the DSI
host state, and forget about changing the pre_enable/post_disable
order. However it involves even more changes to all the panel and
bridge drivers.

If you've added an init hook, don't you also need a deinint hook to
ensure that the host is restored to the "power on but not inited"
state before device power off? Currently it feels unbalanced, but
largely as I don't know exactly what you're defining that power on
state to be.

  Dave

> I've prepared a prototype of such solution. This approach finally
> resolved all the initialization issues! The bridge chain finally matches
> the hardware, no hack are needed, and everything is controlled by the
> DRM core. This prototype also includes the Jagan's patches, which add
> IMX support to Samsung DSIM. If one is interested, here is my git repo
> with all the PoC patches:
>
> https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Marek Szyprowski]

On 11.05.2022 17:47, Marek Vasut wrote:
> On 5/11/22 16:58, Marek Szyprowski wrote:
>> Hi Dave,
>>
>> On 05.04.2022 13:43, Dave Stevenson wrote:
>>> On Fri, 18 Mar 2022 at 12:25, Dave Stevenson
>>> <dave.stevenson@raspberrypi.com>  wrote:
>>>> On Fri, 4 Mar 2022 at 15:18, Dave Stevenson
>>>> <dave.stevenson@raspberrypi.com>  wrote:
>>>>> Hi All
>>>> A gentle ping on this series. Any comments on the approach?
>>>> Thanks.
>>> I realise the merge window has just closed and therefore folks have
>>> been busy, but no responses on this after a month?
>>>
>>> Do I give up and submit a patch to document that DSI is broken and 
>>> no one cares?
>>
>> Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI
>> DSIM bridge' thread, otherwise I would miss it since I'm not involved
>> much in the DRM development.
>>
>> This resolves most of the issues in the Exynos DSI and its recent
>> conversion to the drm bridge framework. I've added the needed
>> prepare_upstream_first flags to the panels and everything works fine
>> without the bridge chain order hacks.
>>
>> Feel free to add:
>>
>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>
>>
>> The only remaining thing to resolve is the moment of enabling DSI host.
>> The proper sequence is:
>>
>> 1. host power on, 2. device power on, 3. host init, 4. device init, 5.
>> video enable.
>
> +CC Raphael, STM32MP1 has similar topic.
>
>> #1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so
>> far done in the first host transfer call, which usually happens in
>> panel's prepare, then the #4 happens. Then video enable is done in the
>> enable callbacks.
>>
>> Jagan wants to move it to the dsi host pre_enable() to let it work with
>> DSI bridges controlled over different interfaces
>> (https://lore.kernel.org/all/20220504114021.33265-6-jagan@amarulasolutions.com/ 
>>
>> ). This however fails on Exynos with DSI panels, because when dsi's
>> pre_enable is called, the dsi device is not yet powered. I've discussed
>> this with Andrzej Hajda and we came to the conclusion that this can be
>> resolved by adding the init() callback to the struct mipi_dsi_host_ops.
>> Then DSI client (next bridge or panel) would call it after powering self
>> on, but before sending any DSI commands in its pre_enable/prepare 
>> functions.
>>
>> I've prepared a prototype of such solution. This approach finally
>> resolved all the initialization issues! The bridge chain finally matches
>> the hardware, no hack are needed, and everything is controlled by the
>> DRM core. This prototype also includes the Jagan's patches, which add
>> IMX support to Samsung DSIM. If one is interested, here is my git repo
>> with all the PoC patches:
>>
>> https://protect2.fireeye.com/v1/url?k=fc60b660-9deba379-fc613d2f-74fe485cbfec-6741e6fb26af486e&q=1&e=07baacdb-540b-46fa-be0f-f534635150ec&u=https%3A%2F%2Fgithub.com%2Fmszyprow%2Flinux%2Ftree%2Fv5.18-next-20220511-dsi-rework 
>>
>
> Can you CC me on the iMX DSIM discussion/patches ? It seems I was left 
> out of it all, even though I work with the iMX8M DRM stuff extensively.


https://lore.kernel.org/all/20220504114021.33265-1-jagan@amarulasolutions.com/

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

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Jagan Teki]

On Wed, May 11, 2022 at 8:28 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi Dave,
>
> On 05.04.2022 13:43, Dave Stevenson wrote:
> > On Fri, 18 Mar 2022 at 12:25, Dave Stevenson
> > <dave.stevenson@raspberrypi.com>  wrote:
> >> On Fri, 4 Mar 2022 at 15:18, Dave Stevenson
> >> <dave.stevenson@raspberrypi.com>  wrote:
> >>> Hi All
> >> A gentle ping on this series. Any comments on the approach?
> >> Thanks.
> > I realise the merge window has just closed and therefore folks have
> > been busy, but no responses on this after a month?
> >
> > Do I give up and submit a patch to document that DSI is broken and no one cares?
>
> Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI
> DSIM bridge' thread, otherwise I would miss it since I'm not involved
> much in the DRM development.
>
> This resolves most of the issues in the Exynos DSI and its recent
> conversion to the drm bridge framework. I've added the needed
> prepare_upstream_first flags to the panels and everything works fine
> without the bridge chain order hacks.
>
> Feel free to add:
>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>
>
> The only remaining thing to resolve is the moment of enabling DSI host.
> The proper sequence is:
>
> 1. host power on, 2. device power on, 3. host init, 4. device init, 5.
> video enable.
>
> #1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so
> far done in the first host transfer call, which usually happens in
> panel's prepare, then the #4 happens. Then video enable is done in the
> enable callbacks.
>
> Jagan wants to move it to the dsi host pre_enable() to let it work with
> DSI bridges controlled over different interfaces
> (https://lore.kernel.org/all/20220504114021.33265-6-jagan@amarulasolutions.com/
> ). This however fails on Exynos with DSI panels, because when dsi's
> pre_enable is called, the dsi device is not yet powered. I've discussed
> this with Andrzej Hajda and we came to the conclusion that this can be
> resolved by adding the init() callback to the struct mipi_dsi_host_ops.
> Then DSI client (next bridge or panel) would call it after powering self
> on, but before sending any DSI commands in its pre_enable/prepare functions.
>
> I've prepared a prototype of such solution. This approach finally
> resolved all the initialization issues! The bridge chain finally matches
> the hardware, no hack are needed, and everything is controlled by the
> DRM core. This prototype also includes the Jagan's patches, which add
> IMX support to Samsung DSIM. If one is interested, here is my git repo
> with all the PoC patches:
>
> https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework

This seems a bit confusing to me, how come a Host initialization
depends on the downstream bridge call flow?

Thanks,
Jagan.

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Jagan Teki]

On Wed, May 11, 2022 at 9:17 PM Marek Vasut <marex@denx.de> wrote:
>
> On 5/11/22 16:58, Marek Szyprowski wrote:
> > Hi Dave,
> >
> > On 05.04.2022 13:43, Dave Stevenson wrote:
> >> On Fri, 18 Mar 2022 at 12:25, Dave Stevenson
> >> <dave.stevenson@raspberrypi.com>  wrote:
> >>> On Fri, 4 Mar 2022 at 15:18, Dave Stevenson
> >>> <dave.stevenson@raspberrypi.com>  wrote:
> >>>> Hi All
> >>> A gentle ping on this series. Any comments on the approach?
> >>> Thanks.
> >> I realise the merge window has just closed and therefore folks have
> >> been busy, but no responses on this after a month?
> >>
> >> Do I give up and submit a patch to document that DSI is broken and no one cares?
> >
> > Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI
> > DSIM bridge' thread, otherwise I would miss it since I'm not involved
> > much in the DRM development.
> >
> > This resolves most of the issues in the Exynos DSI and its recent
> > conversion to the drm bridge framework. I've added the needed
> > prepare_upstream_first flags to the panels and everything works fine
> > without the bridge chain order hacks.
> >
> > Feel free to add:
> >
> > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >
> >
> > The only remaining thing to resolve is the moment of enabling DSI host.
> > The proper sequence is:
> >
> > 1. host power on, 2. device power on, 3. host init, 4. device init, 5.
> > video enable.
>
> +CC Raphael, STM32MP1 has similar topic.
>
> > #1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so
> > far done in the first host transfer call, which usually happens in
> > panel's prepare, then the #4 happens. Then video enable is done in the
> > enable callbacks.
> >
> > Jagan wants to move it to the dsi host pre_enable() to let it work with
> > DSI bridges controlled over different interfaces
> > (https://lore.kernel.org/all/20220504114021.33265-6-jagan@amarulasolutions.com/
> > ). This however fails on Exynos with DSI panels, because when dsi's
> > pre_enable is called, the dsi device is not yet powered. I've discussed
> > this with Andrzej Hajda and we came to the conclusion that this can be
> > resolved by adding the init() callback to the struct mipi_dsi_host_ops.
> > Then DSI client (next bridge or panel) would call it after powering self
> > on, but before sending any DSI commands in its pre_enable/prepare functions.
> >
> > I've prepared a prototype of such solution. This approach finally
> > resolved all the initialization issues! The bridge chain finally matches
> > the hardware, no hack are needed, and everything is controlled by the
> > DRM core. This prototype also includes the Jagan's patches, which add
> > IMX support to Samsung DSIM. If one is interested, here is my git repo
> > with all the PoC patches:
> >
> > https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework
>
> Can you CC me on the iMX DSIM discussion/patches ? It seems I was left
> out of it all, even though I work with the iMX8M DRM stuff extensively.

Sorry, this is not intentional. I added you and many others in RFC and
subsequently, I have added in the next versions whoever responds to
the previous. I will do it in the next version DSIM.

Thanks,
Jagan.

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Marek Szyprowski]

Hi Dave,

On 11.05.2022 17:47, Dave Stevenson wrote:
> On Wed, 11 May 2022 at 15:58, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>> On 05.04.2022 13:43, Dave Stevenson wrote:
>>> On Fri, 18 Mar 2022 at 12:25, Dave Stevenson
>>> <dave.stevenson@raspberrypi.com>  wrote:
>>>> On Fri, 4 Mar 2022 at 15:18, Dave Stevenson
>>>> <dave.stevenson@raspberrypi.com>  wrote:
>>>>> Hi All
>>>> A gentle ping on this series. Any comments on the approach?
>>>> Thanks.
>>> I realise the merge window has just closed and therefore folks have
>>> been busy, but no responses on this after a month?
>>>
>>> Do I give up and submit a patch to document that DSI is broken and no one cares?
>> Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI
>> DSIM bridge' thread, otherwise I would miss it since I'm not involved
>> much in the DRM development.
>>
>> This resolves most of the issues in the Exynos DSI and its recent
>> conversion to the drm bridge framework. I've added the needed
>> prepare_upstream_first flags to the panels and everything works fine
>> without the bridge chain order hacks.
>>
>> Feel free to add:
>>
>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Thanks for testing it. I was almost at the stage of abandoning the patch set.
>
>> The only remaining thing to resolve is the moment of enabling DSI host.
>> The proper sequence is:
>>
>> 1. host power on, 2. device power on, 3. host init, 4. device init, 5.
>> video enable.
>>
>> #1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so
>> far done in the first host transfer call, which usually happens in
>> panel's prepare, then the #4 happens. Then video enable is done in the
>> enable callbacks.
> What's your definition of host power on and host init here? What state
> are you defining the DSI interface to be in after each operation?

Well, lets start from the point that I'm not a DSI specialist nor I'm 
not the exynos-dsi author. I just played a bit with the code trying to 
restore proper driver operation on the various Exynos based boards I have.

By the host/device power on I mean enabling their power regulators. By 
host init I mean executing the samsung_dsim_init() function, which 
basically sets the lp-11 state if I understand it right.


>> Jagan wants to move it to the dsi host pre_enable() to let it work with
>> DSI bridges controlled over different interfaces
>> (https://lore.kernel.org/all/20220504114021.33265-6-jagan@amarulasolutions.com/
>> ).
> I think I'm in agreement with Jagan.
> As documented in patch 4/4:
> + * A DSI host should keep the PHY powered down until the pre_enable
> operation is
> + * called. All lanes are in an undefined idle state up to this point, and it
> + * must not be assumed that it is LP-11.
> + * pre_enable should initialise the PHY, set the data lanes to LP-11, and the
> + * clock lane to either LP-11 or HS depending on the mode_flag
> + * %MIPI_DSI_CLOCK_NON_CONTINUOUS.

Right, this theory makes sense.

However Exynos DSI for some reasons did the host initialization in the 
first call of the samsung_dsim_host_transfer(). If I moved the host 
initialization to pre_enable (before powering the panel on), executing 
DSI commands failed (timeout). This issue happens on all boards I have 
access (Trats, Trats2, Arndale, TM2e), so this must be an issue with 
Exynos DSI host itself not related to particular panel/bridge.

If I call samsung_dsim_init() once again, before issuing the first DSI 
command, then everything works fine. I've tried to check which part of 
that function is needed to be executed before transferring the commands, 
but it turned out that the complete host reset and (re)configuration is 
necessary. It looks that the initialization will need to be done twice, 
first time in the pre_enable to satisfy Jagan case, then on the first 
dsi transfer to make it work with real DSI panels.

Here is a git repo with such change: 
https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework-v2


>> This however fails on Exynos with DSI panels, because when dsi's
>> pre_enable is called, the dsi device is not yet powered.
> Sorry, I'm not following what the issue is here? DSI lanes being at
> LP-11 when the sink isn't powered, so potential for leakage through
> the device?

I also have no idea why sending DSI commands fails if host is 
initialized without device being powered up first. Maybe powering it 
later causes some glitches on the lines? However it looks doing the 
initialization again on the first transfer is enough to fix it.

> In which case the device should NOT set pre_enable_upstream first, and
> the host gets powered up and down with each host_transfer call the
> device makes in pre_enable.

Doing the initialization on each host_transfer also is not an option, 
because in such case the panel is not initialized properly. I get no 
errors, but also there is no valid display on the panel in such case.

> (Whilst I can't claim that I know of every single DSI device, most
> datasheets I've encountered have required LP-11 on the lanes before
> powering up the device).


>> I've discussed
>> this with Andrzej Hajda and we came to the conclusion that this can be
>> resolved by adding the init() callback to the struct mipi_dsi_host_ops.
>> Then DSI client (next bridge or panel) would call it after powering self
>> on, but before sending any DSI commands in its pre_enable/prepare functions.
> You may as well have a mipi_dsi_host_ops call to fully control the DSI
> host state, and forget about changing the pre_enable/post_disable
> order. However it involves even more changes to all the panel and
> bridge drivers.

True. Although setting prepare_upstream_first/pre_enable_upstream_first 
flags also requires to revisit all the dsi panels and bridges.


It looks that I've focused too much on finding a single place of the dsi 
initialization, what resulted in that host_init callback. I can live 
without it, doing the initialization twice.

> If you've added an init hook, don't you also need a deinint hook to
> ensure that the host is restored to the "power on but not inited"
> state before device power off? Currently it feels unbalanced, but
> largely as I don't know exactly what you're defining that power on
> state to be.

So far I had no use case for that deinit hook.


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

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Andrzej Hajda]

On 18.05.2022 16:05, Marek Szyprowski wrote:
> Hi Dave,
>
> On 11.05.2022 17:47, Dave Stevenson wrote:
>> On Wed, 11 May 2022 at 15:58, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>>> On 05.04.2022 13:43, Dave Stevenson wrote:
>>>> On Fri, 18 Mar 2022 at 12:25, Dave Stevenson
>>>> <dave.stevenson@raspberrypi.com>  wrote:
>>>>> On Fri, 4 Mar 2022 at 15:18, Dave Stevenson
>>>>> <dave.stevenson@raspberrypi.com>  wrote:
>>>>>> Hi All
>>>>> A gentle ping on this series. Any comments on the approach?
>>>>> Thanks.
>>>> I realise the merge window has just closed and therefore folks have
>>>> been busy, but no responses on this after a month?
>>>>
>>>> Do I give up and submit a patch to document that DSI is broken and no one cares?
>>> Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI
>>> DSIM bridge' thread, otherwise I would miss it since I'm not involved
>>> much in the DRM development.
>>>
>>> This resolves most of the issues in the Exynos DSI and its recent
>>> conversion to the drm bridge framework. I've added the needed
>>> prepare_upstream_first flags to the panels and everything works fine
>>> without the bridge chain order hacks.
>>>
>>> Feel free to add:
>>>
>>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> Thanks for testing it. I was almost at the stage of abandoning the patch set.
>>
>>> The only remaining thing to resolve is the moment of enabling DSI host.
>>> The proper sequence is:
>>>
>>> 1. host power on, 2. device power on, 3. host init, 4. device init, 5.
>>> video enable.
>>>
>>> #1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so
>>> far done in the first host transfer call, which usually happens in
>>> panel's prepare, then the #4 happens. Then video enable is done in the
>>> enable callbacks.
>> What's your definition of host power on and host init here? What state
>> are you defining the DSI interface to be in after each operation?
> Well, lets start from the point that I'm not a DSI specialist nor I'm
> not the exynos-dsi author. I just played a bit with the code trying to
> restore proper driver operation on the various Exynos based boards I have.
>
> By the host/device power on I mean enabling their power regulators. By
> host init I mean executing the samsung_dsim_init() function, which
> basically sets the lp-11 state if I understand it right.
>
>
>>> Jagan wants to move it to the dsi host pre_enable() to let it work with
>>> DSI bridges controlled over different interfaces
>>> (https://lore.kernel.org/all/20220504114021.33265-6-jagan@amarulasolutions.com/
>>> ).
>> I think I'm in agreement with Jagan.
>> As documented in patch 4/4:
>> + * A DSI host should keep the PHY powered down until the pre_enable
>> operation is
>> + * called. All lanes are in an undefined idle state up to this point, and it
>> + * must not be assumed that it is LP-11.
>> + * pre_enable should initialise the PHY, set the data lanes to LP-11, and the
>> + * clock lane to either LP-11 or HS depending on the mode_flag
>> + * %MIPI_DSI_CLOCK_NON_CONTINUOUS.
> Right, this theory makes sense.
>
> However Exynos DSI for some reasons did the host initialization in the
> first call of the samsung_dsim_host_transfer().

It was a way to fit exynos DSI order of initialization to the frameworks 
present at the time it was written:
exynos_dsi was an drm_encoder, and it was connected to drm_panel (DSI 
controlled panel).
1. In exynos_dsi_enable host was powered on then drm_panel_prepare was 
called.
2. drm_panel_prepare called .prepare callback of the panel, which 
powered on the panel and then requested dsi transfers to initialize panel.
3. 1st dsi transfer performed dsi host init (LP-11), after that all 
transfers started by panel performed panel initialization.
4. after that control goes back to exynos_dsi_enable.
5. exynos_dsi_enable starts video transfer and notify panel about it via 
drm_panel_enable.
6. .enable callback of the panel starts displaying frames (after some 
delay).

This construction allowed to keep proper order of hw initialization: 
host power on, panel power on, host init, panel init, start video 
transmission, start display frames.
Almost all elements of this sequence are enforced by DSI specification 
(at least for devices controlled via dsi).
The only thing which I am not sure is placement of "panel power on". It 
does not seem to be a part of DSI spec, but I am not 100% sure.
It could be specific to platform (mis)configuration (regulators, level 
shifters, ...), or just dsi host init sequence tries to do too much.
I wonder if dropping checking stop state in exynos_dsi wouldn't solve 
the mystery [1], or moving it to 1st transfer, maybe with(or w/o) 
subsequent code.
Does IMX have some 'vendor code' of DSI host to compare with?

[1]: 
https://elixir.bootlin.com/linux/v5.15.1/source/drivers/gpu/drm/exynos/exynos_drm_dsi.c#L860

Regards
Andrzej

> If I moved the host
> initialization to pre_enable (before powering the panel on), executing
> DSI commands failed (timeout). This issue happens on all boards I have
> access (Trats, Trats2, Arndale, TM2e), so this must be an issue with
> Exynos DSI host itself not related to particular panel/bridge.
>
> If I call samsung_dsim_init() once again, before issuing the first DSI
> command, then everything works fine. I've tried to check which part of
> that function is needed to be executed before transferring the commands,
> but it turned out that the complete host reset and (re)configuration is
> necessary. It looks that the initialization will need to be done twice,
> first time in the pre_enable to satisfy Jagan case, then on the first
> dsi transfer to make it work with real DSI panels.
>
> Here is a git repo with such change:
> https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework-v2
>
>
>>> This however fails on Exynos with DSI panels, because when dsi's
>>> pre_enable is called, the dsi device is not yet powered.
>> Sorry, I'm not following what the issue is here? DSI lanes being at
>> LP-11 when the sink isn't powered, so potential for leakage through
>> the device?
> I also have no idea why sending DSI commands fails if host is
> initialized without device being powered up first. Maybe powering it
> later causes some glitches on the lines? However it looks doing the
> initialization again on the first transfer is enough to fix it.
>
>> In which case the device should NOT set pre_enable_upstream first, and
>> the host gets powered up and down with each host_transfer call the
>> device makes in pre_enable.
> Doing the initialization on each host_transfer also is not an option,
> because in such case the panel is not initialized properly. I get no
> errors, but also there is no valid display on the panel in such case.
>
>> (Whilst I can't claim that I know of every single DSI device, most
>> datasheets I've encountered have required LP-11 on the lanes before
>> powering up the device).
>
>>> I've discussed
>>> this with Andrzej Hajda and we came to the conclusion that this can be
>>> resolved by adding the init() callback to the struct mipi_dsi_host_ops.
>>> Then DSI client (next bridge or panel) would call it after powering self
>>> on, but before sending any DSI commands in its pre_enable/prepare functions.
>> You may as well have a mipi_dsi_host_ops call to fully control the DSI
>> host state, and forget about changing the pre_enable/post_disable
>> order. However it involves even more changes to all the panel and
>> bridge drivers.
> True. Although setting prepare_upstream_first/pre_enable_upstream_first
> flags also requires to revisit all the dsi panels and bridges.
>
>
> It looks that I've focused too much on finding a single place of the dsi
> initialization, what resulted in that host_init callback. I can live
> without it, doing the initialization twice.
>
>> If you've added an init hook, don't you also need a deinint hook to
>> ensure that the host is restored to the "power on but not inited"
>> state before device power off? Currently it feels unbalanced, but
>> largely as I don't know exactly what you're defining that power on
>> state to be.
> So far I had no use case for that deinit hook.
>
>
> Best regards

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Dave Stevenson]

Hi Marek and Andrzej

On Wed, 18 May 2022 at 23:53, Andrzej Hajda <andrzej.hajda@intel.com> wrote:
>
>
>
> On 18.05.2022 16:05, Marek Szyprowski wrote:
> > Hi Dave,
> >
> > On 11.05.2022 17:47, Dave Stevenson wrote:
> >> On Wed, 11 May 2022 at 15:58, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> >>> On 05.04.2022 13:43, Dave Stevenson wrote:
> >>>> On Fri, 18 Mar 2022 at 12:25, Dave Stevenson
> >>>> <dave.stevenson@raspberrypi.com>  wrote:
> >>>>> On Fri, 4 Mar 2022 at 15:18, Dave Stevenson
> >>>>> <dave.stevenson@raspberrypi.com>  wrote:
> >>>>>> Hi All
> >>>>> A gentle ping on this series. Any comments on the approach?
> >>>>> Thanks.
> >>>> I realise the merge window has just closed and therefore folks have
> >>>> been busy, but no responses on this after a month?
> >>>>
> >>>> Do I give up and submit a patch to document that DSI is broken and no one cares?
> >>> Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI
> >>> DSIM bridge' thread, otherwise I would miss it since I'm not involved
> >>> much in the DRM development.
> >>>
> >>> This resolves most of the issues in the Exynos DSI and its recent
> >>> conversion to the drm bridge framework. I've added the needed
> >>> prepare_upstream_first flags to the panels and everything works fine
> >>> without the bridge chain order hacks.
> >>>
> >>> Feel free to add:
> >>>
> >>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >> Thanks for testing it. I was almost at the stage of abandoning the patch set.
> >>
> >>> The only remaining thing to resolve is the moment of enabling DSI host.
> >>> The proper sequence is:
> >>>
> >>> 1. host power on, 2. device power on, 3. host init, 4. device init, 5.
> >>> video enable.
> >>>
> >>> #1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so
> >>> far done in the first host transfer call, which usually happens in
> >>> panel's prepare, then the #4 happens. Then video enable is done in the
> >>> enable callbacks.
> >> What's your definition of host power on and host init here? What state
> >> are you defining the DSI interface to be in after each operation?
> > Well, lets start from the point that I'm not a DSI specialist nor I'm
> > not the exynos-dsi author. I just played a bit with the code trying to
> > restore proper driver operation on the various Exynos based boards I have.

Is there any such thing as a DSI specialist? It seems to have many
nasty corners that very few can really claim to fully understand (I
don't claim to be an expert in it!).

> > By the host/device power on I mean enabling their power regulators. By
> > host init I mean executing the samsung_dsim_init() function, which
> > basically sets the lp-11 state if I understand it right.
> >
> >
> >>> Jagan wants to move it to the dsi host pre_enable() to let it work with
> >>> DSI bridges controlled over different interfaces
> >>> (https://lore.kernel.org/all/20220504114021.33265-6-jagan@amarulasolutions.com/
> >>> ).
> >> I think I'm in agreement with Jagan.
> >> As documented in patch 4/4:
> >> + * A DSI host should keep the PHY powered down until the pre_enable
> >> operation is
> >> + * called. All lanes are in an undefined idle state up to this point, and it
> >> + * must not be assumed that it is LP-11.
> >> + * pre_enable should initialise the PHY, set the data lanes to LP-11, and the
> >> + * clock lane to either LP-11 or HS depending on the mode_flag
> >> + * %MIPI_DSI_CLOCK_NON_CONTINUOUS.
> > Right, this theory makes sense.
> >
> > However Exynos DSI for some reasons did the host initialization in the
> > first call of the samsung_dsim_host_transfer().
>
> It was a way to fit exynos DSI order of initialization to the frameworks
> present at the time it was written:
> exynos_dsi was an drm_encoder, and it was connected to drm_panel (DSI
> controlled panel).
> 1. In exynos_dsi_enable host was powered on then drm_panel_prepare was
> called.
> 2. drm_panel_prepare called .prepare callback of the panel, which
> powered on the panel and then requested dsi transfers to initialize panel.
> 3. 1st dsi transfer performed dsi host init (LP-11), after that all
> transfers started by panel performed panel initialization.
> 4. after that control goes back to exynos_dsi_enable.
> 5. exynos_dsi_enable starts video transfer and notify panel about it via
> drm_panel_enable.
> 6. .enable callback of the panel starts displaying frames (after some
> delay).
>
> This construction allowed to keep proper order of hw initialization:
> host power on, panel power on, host init, panel init, start video
> transmission, start display frames.
> Almost all elements of this sequence are enforced by DSI specification
> (at least for devices controlled via dsi).
> The only thing which I am not sure is placement of "panel power on". It
> does not seem to be a part of DSI spec, but I am not 100% sure.
> It could be specific to platform (mis)configuration (regulators, level
> shifters, ...), or just dsi host init sequence tries to do too much.
> I wonder if dropping checking stop state in exynos_dsi wouldn't solve
> the mystery [1], or moving it to 1st transfer, maybe with(or w/o)
> subsequent code.

I was thinking along similar lines when I read Marek's email.

If the panel is powered off then it may well be pulling the lanes
down, and so the host can not achieve LP-11. However at the pre_enable
stage there is no requirement to achieve LP-11 as there is no data
being passed such that bus contention needs to be detected.
Moving that test to host_transfer, where we are potentially reading
data and there is a definite need to check for contention, would
certainly be a useful test. I see that registers DSIM_ESCMODE_REG and
DSIM_TIMEOUT_REG that are only set after the lanes are viewed as being
in the "right" state, so presumably those are not being written with
things reordered. (The return value of exynos_dsi_init_link is not
checked, so that won't make any difference).
Potentially test the lane state at enable too, although one could
argue that detecting contention when sending video is of limited
benefit as the display may be able to receive the data even if the
host sees issues.

I've had a conversation with my hardware colleagues and they see no
reason for concern for having LP-11 driven from the host with the
slave powered down.

Checking the MIPI D-PHY spec v1.1 section 6.11 Initialization. State
"Slave off", Line Levels "Any", so the slave is meant to handle
anything when powered down.
Section 4.2 Mandatory Functionality, "All functionality that is
specified in this document and which is not explicitly stated in
Section 5.5 shall be implemented for all D-PHY configurations." So
that is not optional, but does rely on implementers having followed
the spec.

  Dave

> Does IMX have some 'vendor code' of DSI host to compare with?
>
> [1]:
> https://elixir.bootlin.com/linux/v5.15.1/source/drivers/gpu/drm/exynos/exynos_drm_dsi.c#L860
>
> Regards
> Andrzej
>
> > If I moved the host
> > initialization to pre_enable (before powering the panel on), executing
> > DSI commands failed (timeout). This issue happens on all boards I have
> > access (Trats, Trats2, Arndale, TM2e), so this must be an issue with
> > Exynos DSI host itself not related to particular panel/bridge.
> >
> > If I call samsung_dsim_init() once again, before issuing the first DSI
> > command, then everything works fine. I've tried to check which part of
> > that function is needed to be executed before transferring the commands,
> > but it turned out that the complete host reset and (re)configuration is
> > necessary. It looks that the initialization will need to be done twice,
> > first time in the pre_enable to satisfy Jagan case, then on the first
> > dsi transfer to make it work with real DSI panels.
> >
> > Here is a git repo with such change:
> > https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework-v2
> >
> >
> >>> This however fails on Exynos with DSI panels, because when dsi's
> >>> pre_enable is called, the dsi device is not yet powered.
> >> Sorry, I'm not following what the issue is here? DSI lanes being at
> >> LP-11 when the sink isn't powered, so potential for leakage through
> >> the device?
> > I also have no idea why sending DSI commands fails if host is
> > initialized without device being powered up first. Maybe powering it
> > later causes some glitches on the lines? However it looks doing the
> > initialization again on the first transfer is enough to fix it.
> >
> >> In which case the device should NOT set pre_enable_upstream first, and
> >> the host gets powered up and down with each host_transfer call the
> >> device makes in pre_enable.
> > Doing the initialization on each host_transfer also is not an option,
> > because in such case the panel is not initialized properly. I get no
> > errors, but also there is no valid display on the panel in such case.
> >
> >> (Whilst I can't claim that I know of every single DSI device, most
> >> datasheets I've encountered have required LP-11 on the lanes before
> >> powering up the device).
> >
> >>> I've discussed
> >>> this with Andrzej Hajda and we came to the conclusion that this can be
> >>> resolved by adding the init() callback to the struct mipi_dsi_host_ops.
> >>> Then DSI client (next bridge or panel) would call it after powering self
> >>> on, but before sending any DSI commands in its pre_enable/prepare functions.
> >> You may as well have a mipi_dsi_host_ops call to fully control the DSI
> >> host state, and forget about changing the pre_enable/post_disable
> >> order. However it involves even more changes to all the panel and
> >> bridge drivers.
> > True. Although setting prepare_upstream_first/pre_enable_upstream_first
> > flags also requires to revisit all the dsi panels and bridges.
> >
> >
> > It looks that I've focused too much on finding a single place of the dsi
> > initialization, what resulted in that host_init callback. I can live
> > without it, doing the initialization twice.
> >
> >> If you've added an init hook, don't you also need a deinint hook to
> >> ensure that the host is restored to the "power on but not inited"
> >> state before device power off? Currently it feels unbalanced, but
> >> largely as I don't know exactly what you're defining that power on
> >> state to be.
> > So far I had no use case for that deinit hook.
> >
> >
> > Best regards
>

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Jagan Teki]

Hi Marek,

On Wed, May 18, 2022 at 7:35 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi Dave,
>
> On 11.05.2022 17:47, Dave Stevenson wrote:
> > On Wed, 11 May 2022 at 15:58, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> >> On 05.04.2022 13:43, Dave Stevenson wrote:
> >>> On Fri, 18 Mar 2022 at 12:25, Dave Stevenson
> >>> <dave.stevenson@raspberrypi.com>  wrote:
> >>>> On Fri, 4 Mar 2022 at 15:18, Dave Stevenson
> >>>> <dave.stevenson@raspberrypi.com>  wrote:
> >>>>> Hi All
> >>>> A gentle ping on this series. Any comments on the approach?
> >>>> Thanks.
> >>> I realise the merge window has just closed and therefore folks have
> >>> been busy, but no responses on this after a month?
> >>>
> >>> Do I give up and submit a patch to document that DSI is broken and no one cares?
> >> Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI
> >> DSIM bridge' thread, otherwise I would miss it since I'm not involved
> >> much in the DRM development.
> >>
> >> This resolves most of the issues in the Exynos DSI and its recent
> >> conversion to the drm bridge framework. I've added the needed
> >> prepare_upstream_first flags to the panels and everything works fine
> >> without the bridge chain order hacks.
> >>
> >> Feel free to add:
> >>
> >> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > Thanks for testing it. I was almost at the stage of abandoning the patch set.
> >
> >> The only remaining thing to resolve is the moment of enabling DSI host.
> >> The proper sequence is:
> >>
> >> 1. host power on, 2. device power on, 3. host init, 4. device init, 5.
> >> video enable.
> >>
> >> #1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so
> >> far done in the first host transfer call, which usually happens in
> >> panel's prepare, then the #4 happens. Then video enable is done in the
> >> enable callbacks.
> > What's your definition of host power on and host init here? What state
> > are you defining the DSI interface to be in after each operation?
>
> Well, lets start from the point that I'm not a DSI specialist nor I'm
> not the exynos-dsi author. I just played a bit with the code trying to
> restore proper driver operation on the various Exynos based boards I have.
>
> By the host/device power on I mean enabling their power regulators. By
> host init I mean executing the samsung_dsim_init() function, which
> basically sets the lp-11 state if I understand it right.
>
>
> >> Jagan wants to move it to the dsi host pre_enable() to let it work with
> >> DSI bridges controlled over different interfaces
> >> (https://lore.kernel.org/all/20220504114021.33265-6-jagan@amarulasolutions.com/
> >> ).
> > I think I'm in agreement with Jagan.
> > As documented in patch 4/4:
> > + * A DSI host should keep the PHY powered down until the pre_enable
> > operation is
> > + * called. All lanes are in an undefined idle state up to this point, and it
> > + * must not be assumed that it is LP-11.
> > + * pre_enable should initialise the PHY, set the data lanes to LP-11, and the
> > + * clock lane to either LP-11 or HS depending on the mode_flag
> > + * %MIPI_DSI_CLOCK_NON_CONTINUOUS.
>
> Right, this theory makes sense.
>
> However Exynos DSI for some reasons did the host initialization in the
> first call of the samsung_dsim_host_transfer(). If I moved the host
> initialization to pre_enable (before powering the panel on), executing
> DSI commands failed (timeout). This issue happens on all boards I have
> access (Trats, Trats2, Arndale, TM2e), so this must be an issue with
> Exynos DSI host itself not related to particular panel/bridge.
>
> If I call samsung_dsim_init() once again, before issuing the first DSI
> command, then everything works fine. I've tried to check which part of
> that function is needed to be executed before transferring the commands,
> but it turned out that the complete host reset and (re)configuration is
> necessary. It looks that the initialization will need to be done twice,
> first time in the pre_enable to satisfy Jagan case, then on the first
> dsi transfer to make it work with real DSI panels.
>
> Here is a git repo with such change:
> https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework-v2

This is worthy check, I will test it on imx8mm and integrate it into
the next version - hope this is fine for you?

Jagan.

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Dave Stevenson]

Hi All.

This series has been kicking around since March now. Any chance of a
review by any of the bridge maintainers or other DSI maintainers?

Converting Exynos DSI to the bridge framework seems to have issues,
but that is a separate thing to this patch set.
(It looks to be nearly there, but the check of stop state at init is
not required by the DSI spec, and is likely to be tripping things up
as it then doesn't set some registers. Contention only needs to be
checked for before sending data).

Thanks
  Dave

On Thu, 19 May 2022 at 13:56, Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi Marek and Andrzej
>
> On Wed, 18 May 2022 at 23:53, Andrzej Hajda <andrzej.hajda@intel.com> wrote:
> >
> >
> >
> > On 18.05.2022 16:05, Marek Szyprowski wrote:
> > > Hi Dave,
> > >
> > > On 11.05.2022 17:47, Dave Stevenson wrote:
> > >> On Wed, 11 May 2022 at 15:58, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> > >>> On 05.04.2022 13:43, Dave Stevenson wrote:
> > >>>> On Fri, 18 Mar 2022 at 12:25, Dave Stevenson
> > >>>> <dave.stevenson@raspberrypi.com>  wrote:
> > >>>>> On Fri, 4 Mar 2022 at 15:18, Dave Stevenson
> > >>>>> <dave.stevenson@raspberrypi.com>  wrote:
> > >>>>>> Hi All
> > >>>>> A gentle ping on this series. Any comments on the approach?
> > >>>>> Thanks.
> > >>>> I realise the merge window has just closed and therefore folks have
> > >>>> been busy, but no responses on this after a month?
> > >>>>
> > >>>> Do I give up and submit a patch to document that DSI is broken and no one cares?
> > >>> Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI
> > >>> DSIM bridge' thread, otherwise I would miss it since I'm not involved
> > >>> much in the DRM development.
> > >>>
> > >>> This resolves most of the issues in the Exynos DSI and its recent
> > >>> conversion to the drm bridge framework. I've added the needed
> > >>> prepare_upstream_first flags to the panels and everything works fine
> > >>> without the bridge chain order hacks.
> > >>>
> > >>> Feel free to add:
> > >>>
> > >>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > >> Thanks for testing it. I was almost at the stage of abandoning the patch set.
> > >>
> > >>> The only remaining thing to resolve is the moment of enabling DSI host.
> > >>> The proper sequence is:
> > >>>
> > >>> 1. host power on, 2. device power on, 3. host init, 4. device init, 5.
> > >>> video enable.
> > >>>
> > >>> #1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so
> > >>> far done in the first host transfer call, which usually happens in
> > >>> panel's prepare, then the #4 happens. Then video enable is done in the
> > >>> enable callbacks.
> > >> What's your definition of host power on and host init here? What state
> > >> are you defining the DSI interface to be in after each operation?
> > > Well, lets start from the point that I'm not a DSI specialist nor I'm
> > > not the exynos-dsi author. I just played a bit with the code trying to
> > > restore proper driver operation on the various Exynos based boards I have.
>
> Is there any such thing as a DSI specialist? It seems to have many
> nasty corners that very few can really claim to fully understand (I
> don't claim to be an expert in it!).
>
> > > By the host/device power on I mean enabling their power regulators. By
> > > host init I mean executing the samsung_dsim_init() function, which
> > > basically sets the lp-11 state if I understand it right.
> > >
> > >
> > >>> Jagan wants to move it to the dsi host pre_enable() to let it work with
> > >>> DSI bridges controlled over different interfaces
> > >>> (https://lore.kernel.org/all/20220504114021.33265-6-jagan@amarulasolutions.com/
> > >>> ).
> > >> I think I'm in agreement with Jagan.
> > >> As documented in patch 4/4:
> > >> + * A DSI host should keep the PHY powered down until the pre_enable
> > >> operation is
> > >> + * called. All lanes are in an undefined idle state up to this point, and it
> > >> + * must not be assumed that it is LP-11.
> > >> + * pre_enable should initialise the PHY, set the data lanes to LP-11, and the
> > >> + * clock lane to either LP-11 or HS depending on the mode_flag
> > >> + * %MIPI_DSI_CLOCK_NON_CONTINUOUS.
> > > Right, this theory makes sense.
> > >
> > > However Exynos DSI for some reasons did the host initialization in the
> > > first call of the samsung_dsim_host_transfer().
> >
> > It was a way to fit exynos DSI order of initialization to the frameworks
> > present at the time it was written:
> > exynos_dsi was an drm_encoder, and it was connected to drm_panel (DSI
> > controlled panel).
> > 1. In exynos_dsi_enable host was powered on then drm_panel_prepare was
> > called.
> > 2. drm_panel_prepare called .prepare callback of the panel, which
> > powered on the panel and then requested dsi transfers to initialize panel.
> > 3. 1st dsi transfer performed dsi host init (LP-11), after that all
> > transfers started by panel performed panel initialization.
> > 4. after that control goes back to exynos_dsi_enable.
> > 5. exynos_dsi_enable starts video transfer and notify panel about it via
> > drm_panel_enable.
> > 6. .enable callback of the panel starts displaying frames (after some
> > delay).
> >
> > This construction allowed to keep proper order of hw initialization:
> > host power on, panel power on, host init, panel init, start video
> > transmission, start display frames.
> > Almost all elements of this sequence are enforced by DSI specification
> > (at least for devices controlled via dsi).
> > The only thing which I am not sure is placement of "panel power on". It
> > does not seem to be a part of DSI spec, but I am not 100% sure.
> > It could be specific to platform (mis)configuration (regulators, level
> > shifters, ...), or just dsi host init sequence tries to do too much.
> > I wonder if dropping checking stop state in exynos_dsi wouldn't solve
> > the mystery [1], or moving it to 1st transfer, maybe with(or w/o)
> > subsequent code.
>
> I was thinking along similar lines when I read Marek's email.
>
> If the panel is powered off then it may well be pulling the lanes
> down, and so the host can not achieve LP-11. However at the pre_enable
> stage there is no requirement to achieve LP-11 as there is no data
> being passed such that bus contention needs to be detected.
> Moving that test to host_transfer, where we are potentially reading
> data and there is a definite need to check for contention, would
> certainly be a useful test. I see that registers DSIM_ESCMODE_REG and
> DSIM_TIMEOUT_REG that are only set after the lanes are viewed as being
> in the "right" state, so presumably those are not being written with
> things reordered. (The return value of exynos_dsi_init_link is not
> checked, so that won't make any difference).
> Potentially test the lane state at enable too, although one could
> argue that detecting contention when sending video is of limited
> benefit as the display may be able to receive the data even if the
> host sees issues.
>
> I've had a conversation with my hardware colleagues and they see no
> reason for concern for having LP-11 driven from the host with the
> slave powered down.
>
> Checking the MIPI D-PHY spec v1.1 section 6.11 Initialization. State
> "Slave off", Line Levels "Any", so the slave is meant to handle
> anything when powered down.
> Section 4.2 Mandatory Functionality, "All functionality that is
> specified in this document and which is not explicitly stated in
> Section 5.5 shall be implemented for all D-PHY configurations." So
> that is not optional, but does rely on implementers having followed
> the spec.
>
>   Dave
>
> > Does IMX have some 'vendor code' of DSI host to compare with?
> >
> > [1]:
> > https://elixir.bootlin.com/linux/v5.15.1/source/drivers/gpu/drm/exynos/exynos_drm_dsi.c#L860
> >
> > Regards
> > Andrzej
> >
> > > If I moved the host
> > > initialization to pre_enable (before powering the panel on), executing
> > > DSI commands failed (timeout). This issue happens on all boards I have
> > > access (Trats, Trats2, Arndale, TM2e), so this must be an issue with
> > > Exynos DSI host itself not related to particular panel/bridge.
> > >
> > > If I call samsung_dsim_init() once again, before issuing the first DSI
> > > command, then everything works fine. I've tried to check which part of
> > > that function is needed to be executed before transferring the commands,
> > > but it turned out that the complete host reset and (re)configuration is
> > > necessary. It looks that the initialization will need to be done twice,
> > > first time in the pre_enable to satisfy Jagan case, then on the first
> > > dsi transfer to make it work with real DSI panels.
> > >
> > > Here is a git repo with such change:
> > > https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework-v2
> > >
> > >
> > >>> This however fails on Exynos with DSI panels, because when dsi's
> > >>> pre_enable is called, the dsi device is not yet powered.
> > >> Sorry, I'm not following what the issue is here? DSI lanes being at
> > >> LP-11 when the sink isn't powered, so potential for leakage through
> > >> the device?
> > > I also have no idea why sending DSI commands fails if host is
> > > initialized without device being powered up first. Maybe powering it
> > > later causes some glitches on the lines? However it looks doing the
> > > initialization again on the first transfer is enough to fix it.
> > >
> > >> In which case the device should NOT set pre_enable_upstream first, and
> > >> the host gets powered up and down with each host_transfer call the
> > >> device makes in pre_enable.
> > > Doing the initialization on each host_transfer also is not an option,
> > > because in such case the panel is not initialized properly. I get no
> > > errors, but also there is no valid display on the panel in such case.
> > >
> > >> (Whilst I can't claim that I know of every single DSI device, most
> > >> datasheets I've encountered have required LP-11 on the lanes before
> > >> powering up the device).
> > >
> > >>> I've discussed
> > >>> this with Andrzej Hajda and we came to the conclusion that this can be
> > >>> resolved by adding the init() callback to the struct mipi_dsi_host_ops.
> > >>> Then DSI client (next bridge or panel) would call it after powering self
> > >>> on, but before sending any DSI commands in its pre_enable/prepare functions.
> > >> You may as well have a mipi_dsi_host_ops call to fully control the DSI
> > >> host state, and forget about changing the pre_enable/post_disable
> > >> order. However it involves even more changes to all the panel and
> > >> bridge drivers.
> > > True. Although setting prepare_upstream_first/pre_enable_upstream_first
> > > flags also requires to revisit all the dsi panels and bridges.
> > >
> > >
> > > It looks that I've focused too much on finding a single place of the dsi
> > > initialization, what resulted in that host_init callback. I can live
> > > without it, doing the initialization twice.
> > >
> > >> If you've added an init hook, don't you also need a deinint hook to
> > >> ensure that the host is restored to the "power on but not inited"
> > >> state before device power off? Currently it feels unbalanced, but
> > >> largely as I don't know exactly what you're defining that power on
> > >> state to be.
> > > So far I had no use case for that deinit hook.
> > >
> > >
> > > Best regards
> >

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Jagan Teki]

On Wed, Jun 8, 2022 at 4:19 PM Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi All.
>
> This series has been kicking around since March now. Any chance of a
> review by any of the bridge maintainers or other DSI maintainers?
>
> Converting Exynos DSI to the bridge framework seems to have issues,
> but that is a separate thing to this patch set.

Correct.

> (It looks to be nearly there, but the check of stop state at init is
> not required by the DSI spec, and is likely to be tripping things up
> as it then doesn't set some registers. Contention only needs to be
> checked for before sending data).

I might take a look at this weekend and see if I can give some comments. thanks.

Jagan.

Re: [PATCH V2 1/4] drm/bridge: Remove duplication from drm_bridge and drm_atomic_bridge chains

[Dmitry Baryshkov]

On 04/03/2022 18:17, Dave Stevenson wrote:
> drm_bridge_chain_pre_enable is a subset of
> drm_atomic_bridge_chain_pre_enable, and drm_bridge_chain_post_disable
> is a subset of drm_atomic_bridge_chain_post_disable.
> 
> Change drm_bridge_chain_pre_enable and drm_bridge_chain_post_disable to
> call the atomic versions with a NULL state, and ensure that atomic
> calls are not made if there is no state.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

I think we should update parade-ps8640 to use drm_atomic_bridge_chain_() 
and drop drm_bridge_chain_* API completely.

> ---
>   drivers/gpu/drm/drm_bridge.c | 30 ++++--------------------------
>   1 file changed, 4 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index c96847fc0ebc..198fd471a488 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -527,16 +527,7 @@ EXPORT_SYMBOL(drm_bridge_chain_disable);
>    */
>   void drm_bridge_chain_post_disable(struct drm_bridge *bridge)
>   {
> -	struct drm_encoder *encoder;
> -
> -	if (!bridge)
> -		return;
> -
> -	encoder = bridge->encoder;
> -	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> -		if (bridge->funcs->post_disable)
> -			bridge->funcs->post_disable(bridge);
> -	}
> +	drm_atomic_bridge_chain_post_disable(bridge, NULL);
>   }
>   EXPORT_SYMBOL(drm_bridge_chain_post_disable);
>   
> @@ -582,20 +573,7 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set);
>    */
>   void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
>   {
> -	struct drm_encoder *encoder;
> -	struct drm_bridge *iter;
> -
> -	if (!bridge)
> -		return;
> -
> -	encoder = bridge->encoder;
> -	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> -		if (iter->funcs->pre_enable)
> -			iter->funcs->pre_enable(iter);
> -
> -		if (iter == bridge)
> -			break;
> -	}
> +	drm_atomic_bridge_chain_pre_enable(bridge, NULL);
>   }
>   EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
>   
> @@ -690,7 +668,7 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
>   
>   	encoder = bridge->encoder;
>   	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> -		if (bridge->funcs->atomic_post_disable) {
> +		if (old_state && bridge->funcs->atomic_post_disable) {
>   			struct drm_bridge_state *old_bridge_state;
>   
>   			old_bridge_state =
> @@ -732,7 +710,7 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
>   
>   	encoder = bridge->encoder;
>   	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> -		if (iter->funcs->atomic_pre_enable) {
> +		if (old_state && iter->funcs->atomic_pre_enable) {
>   			struct drm_bridge_state *old_bridge_state;
>   
>   			old_bridge_state =


-- 
With best wishes
Dmitry

Re: [PATCH V2 3/4] drm/panel: Add prepare_upstream_first flag to drm_panel

[Dmitry Baryshkov]

On 04/03/2022 18:17, Dave Stevenson wrote:
> Mapping to the drm_bridge flag pre_enable_upstream_first,
> add a new flag prepare_upstream_first to drm_panel to allow
> the panel driver to request that the upstream bridge should
> be pre_enabled before the panel prepare.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---
>   drivers/gpu/drm/bridge/panel.c |  3 +++
>   include/drm/drm_panel.h        | 10 ++++++++++
>   2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index 5be057575183..2ea08b3ba326 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -234,6 +234,9 @@ struct drm_bridge *drm_panel_bridge_add_typed(struct drm_panel *panel,
>   	panel_bridge->bridge.ops = DRM_BRIDGE_OP_MODES;
>   	panel_bridge->bridge.type = connector_type;
>   
> +	panel_bridge->bridge.pre_enable_upstream_first =
> +						panel->prepare_upstream_first;
> +
>   	drm_bridge_add(&panel_bridge->bridge);
>   
>   	return &panel_bridge->bridge;
> diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> index 1ba2d424a53f..c0f39dfbd071 100644
> --- a/include/drm/drm_panel.h
> +++ b/include/drm/drm_panel.h
> @@ -179,6 +179,16 @@ struct drm_panel {
>   	 * Panel entry in registry.
>   	 */
>   	struct list_head list;
> +
> +	/**
> +	 * @prepare_upstream_first:
> +	 *
> +	 * The upstream controller should be prepared first, before the prepare
> +	 * for the panel is called. This is largely required for DSI panels
> +	 * where the DSI host controller should be initialised to LP-11 before
> +	 * the panel is powered up.
> +	 */
> +	bool prepare_upstream_first;
>   };
>   
>   void drm_panel_init(struct drm_panel *panel, struct device *dev,


-- 
With best wishes
Dmitry

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Lucas Stach]

Hi,

Am Mittwoch, dem 11.05.2022 um 16:58 +0200 schrieb Marek Szyprowski:
> Hi Dave,
> 
> On 05.04.2022 13:43, Dave Stevenson wrote:
> > On Fri, 18 Mar 2022 at 12:25, Dave Stevenson
> > <dave.stevenson@raspberrypi.com>  wrote:
> > > On Fri, 4 Mar 2022 at 15:18, Dave Stevenson
> > > <dave.stevenson@raspberrypi.com>  wrote:
> > > > Hi All
> > > A gentle ping on this series. Any comments on the approach?
> > > Thanks.
> > I realise the merge window has just closed and therefore folks have
> > been busy, but no responses on this after a month?
> > 
> > Do I give up and submit a patch to document that DSI is broken and no one cares?
> 
> Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI 
> DSIM bridge' thread, otherwise I would miss it since I'm not involved 
> much in the DRM development.
> 
> This resolves most of the issues in the Exynos DSI and its recent 
> conversion to the drm bridge framework. I've added the needed 
> prepare_upstream_first flags to the panels and everything works fine 
> without the bridge chain order hacks.
> 
> Feel free to add:
> 
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> 
> 
> The only remaining thing to resolve is the moment of enabling DSI host. 
> The proper sequence is:
> 
> 1. host power on, 2. device power on, 3. host init, 4. device init, 5. 
> video enable.
> 
> #1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so 
> far done in the first host transfer call, which usually happens in 
> panel's prepare, then the #4 happens. Then video enable is done in the 
> enable callbacks.
> 
> Jagan wants to move it to the dsi host pre_enable() to let it work with 
> DSI bridges controlled over different interfaces 
> (https://lore.kernel.org/all/20220504114021.33265-6-jagan@amarulasolutions.com/ 
> ). This however fails on Exynos with DSI panels, because when dsi's 
> pre_enable is called, the dsi device is not yet powered. I've discussed 
> this with Andrzej Hajda and we came to the conclusion that this can be 
> resolved by adding the init() callback to the struct mipi_dsi_host_ops. 
> Then DSI client (next bridge or panel) would call it after powering self 
> on, but before sending any DSI commands in its pre_enable/prepare functions.
> 
> I've prepared a prototype of such solution. This approach finally 
> resolved all the initialization issues! The bridge chain finally matches 
> the hardware, no hack are needed, and everything is controlled by the 
> DRM core. This prototype also includes the Jagan's patches, which add 
> IMX support to Samsung DSIM. If one is interested, here is my git repo 
> with all the PoC patches:
> 
> https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework

While this needs rework on the bridge chip side, I fear that we need
something like that to allow the bridge to control the sequencing of
the DSI host init. While most bridges that aren't controlled via the
DSI channel might be fine with just initializing the host right before
a video signal is driven, there are some that need a different
sequencing.

The chip I'm currently looking at is a TC368767, where the datasheet
states that the DSI lanes must be in LP-11 before the reset is
released. While the datasheet doesn't specify what happens if that
sequence is violated, Marek Vasut found that the chip enters a test
mode if the lanes are not in LP-11 at that point and I can confirm this
observation.
Now with the TC358767 being a DSI to (e)DP converter, we need to
release the chip from reset pretty early to establish the DP AUX
connection to communicate with the display, in order to find out which
video modes we can drive. As the chip is controlled via i2c in my case,
initializing the DSI host on first DSI command transaction is out and
doing so before the bridge pre_enable is way too late.

What I would need for this chip to work properly is an explicit call,
like the mipi_dsi_host_init() added in the PoC above, to allow the
bridge driver to kick the DSI host initialization before releasing the
chip from reset state.

Regards,
Lucas

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Frieder Schrempf]

Am 10.06.22 um 09:52 schrieb Lucas Stach:
> Hi,
> 
> Am Mittwoch, dem 11.05.2022 um 16:58 +0200 schrieb Marek Szyprowski:
>> Hi Dave,
>>
>> On 05.04.2022 13:43, Dave Stevenson wrote:
>>> On Fri, 18 Mar 2022 at 12:25, Dave Stevenson
>>> <dave.stevenson@raspberrypi.com>  wrote:
>>>> On Fri, 4 Mar 2022 at 15:18, Dave Stevenson
>>>> <dave.stevenson@raspberrypi.com>  wrote:
>>>>> Hi All
>>>> A gentle ping on this series. Any comments on the approach?
>>>> Thanks.
>>> I realise the merge window has just closed and therefore folks have
>>> been busy, but no responses on this after a month?
>>>
>>> Do I give up and submit a patch to document that DSI is broken and no one cares?
>>
>> Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI 
>> DSIM bridge' thread, otherwise I would miss it since I'm not involved 
>> much in the DRM development.
>>
>> This resolves most of the issues in the Exynos DSI and its recent 
>> conversion to the drm bridge framework. I've added the needed 
>> prepare_upstream_first flags to the panels and everything works fine 
>> without the bridge chain order hacks.
>>
>> Feel free to add:
>>
>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>
>>
>> The only remaining thing to resolve is the moment of enabling DSI host. 
>> The proper sequence is:
>>
>> 1. host power on, 2. device power on, 3. host init, 4. device init, 5. 
>> video enable.
>>
>> #1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so 
>> far done in the first host transfer call, which usually happens in 
>> panel's prepare, then the #4 happens. Then video enable is done in the 
>> enable callbacks.
>>
>> Jagan wants to move it to the dsi host pre_enable() to let it work with 
>> DSI bridges controlled over different interfaces 
>> (https://lore.kernel.org/all/20220504114021.33265-6-jagan@amarulasolutions.com/ 
>> ). This however fails on Exynos with DSI panels, because when dsi's 
>> pre_enable is called, the dsi device is not yet powered. I've discussed 
>> this with Andrzej Hajda and we came to the conclusion that this can be 
>> resolved by adding the init() callback to the struct mipi_dsi_host_ops. 
>> Then DSI client (next bridge or panel) would call it after powering self 
>> on, but before sending any DSI commands in its pre_enable/prepare functions.
>>
>> I've prepared a prototype of such solution. This approach finally 
>> resolved all the initialization issues! The bridge chain finally matches 
>> the hardware, no hack are needed, and everything is controlled by the 
>> DRM core. This prototype also includes the Jagan's patches, which add 
>> IMX support to Samsung DSIM. If one is interested, here is my git repo 
>> with all the PoC patches:
>>
>> https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework
> 
> While this needs rework on the bridge chip side, I fear that we need
> something like that to allow the bridge to control the sequencing of
> the DSI host init. While most bridges that aren't controlled via the
> DSI channel might be fine with just initializing the host right before
> a video signal is driven, there are some that need a different
> sequencing.
> 
> The chip I'm currently looking at is a TC368767, where the datasheet
> states that the DSI lanes must be in LP-11 before the reset is
> released. While the datasheet doesn't specify what happens if that
> sequence is violated, Marek Vasut found that the chip enters a test
> mode if the lanes are not in LP-11 at that point and I can confirm this
> observation.

The SN65DSI84 also has this requirement according to the datasheet.

> Now with the TC358767 being a DSI to (e)DP converter, we need to
> release the chip from reset pretty early to establish the DP AUX
> connection to communicate with the display, in order to find out which
> video modes we can drive. As the chip is controlled via i2c in my case,
> initializing the DSI host on first DSI command transaction is out and
> doing so before the bridge pre_enable is way too late.
> 
> What I would need for this chip to work properly is an explicit call,
> like the mipi_dsi_host_init() added in the PoC above, to allow the
> bridge driver to kick the DSI host initialization before releasing the
> chip from reset state.

So to sum up on the missing parts:

1. This series needs more reviews, but is generally agreed on.
2. Jagan will integrate Marek's mipi_dsi_host_init() PoC when respinning
his series "drm: bridge: Add Samsung MIPI DSIM bridge".
3. Bridge drivers need to be adjusted to call mipi_dsi_host_init() if
required.

Did I get anything wrong here, or is there some point that I'm missing?
Jagan, do you have any plan to pick up the threads?

Thanks
Frieder

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Jagan Teki]

Hi Frieder,

On Wed, Jul 6, 2022 at 12:39 PM Frieder Schrempf
<frieder.schrempf@kontron.de> wrote:
>
> Am 10.06.22 um 09:52 schrieb Lucas Stach:
> > Hi,
> >
> > Am Mittwoch, dem 11.05.2022 um 16:58 +0200 schrieb Marek Szyprowski:
> >> Hi Dave,
> >>
> >> On 05.04.2022 13:43, Dave Stevenson wrote:
> >>> On Fri, 18 Mar 2022 at 12:25, Dave Stevenson
> >>> <dave.stevenson@raspberrypi.com>  wrote:
> >>>> On Fri, 4 Mar 2022 at 15:18, Dave Stevenson
> >>>> <dave.stevenson@raspberrypi.com>  wrote:
> >>>>> Hi All
> >>>> A gentle ping on this series. Any comments on the approach?
> >>>> Thanks.
> >>> I realise the merge window has just closed and therefore folks have
> >>> been busy, but no responses on this after a month?
> >>>
> >>> Do I give up and submit a patch to document that DSI is broken and no one cares?
> >>
> >> Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI
> >> DSIM bridge' thread, otherwise I would miss it since I'm not involved
> >> much in the DRM development.
> >>
> >> This resolves most of the issues in the Exynos DSI and its recent
> >> conversion to the drm bridge framework. I've added the needed
> >> prepare_upstream_first flags to the panels and everything works fine
> >> without the bridge chain order hacks.
> >>
> >> Feel free to add:
> >>
> >> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >>
> >>
> >> The only remaining thing to resolve is the moment of enabling DSI host.
> >> The proper sequence is:
> >>
> >> 1. host power on, 2. device power on, 3. host init, 4. device init, 5.
> >> video enable.
> >>
> >> #1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so
> >> far done in the first host transfer call, which usually happens in
> >> panel's prepare, then the #4 happens. Then video enable is done in the
> >> enable callbacks.
> >>
> >> Jagan wants to move it to the dsi host pre_enable() to let it work with
> >> DSI bridges controlled over different interfaces
> >> (https://lore.kernel.org/all/20220504114021.33265-6-jagan@amarulasolutions.com/
> >> ). This however fails on Exynos with DSI panels, because when dsi's
> >> pre_enable is called, the dsi device is not yet powered. I've discussed
> >> this with Andrzej Hajda and we came to the conclusion that this can be
> >> resolved by adding the init() callback to the struct mipi_dsi_host_ops.
> >> Then DSI client (next bridge or panel) would call it after powering self
> >> on, but before sending any DSI commands in its pre_enable/prepare functions.
> >>
> >> I've prepared a prototype of such solution. This approach finally
> >> resolved all the initialization issues! The bridge chain finally matches
> >> the hardware, no hack are needed, and everything is controlled by the
> >> DRM core. This prototype also includes the Jagan's patches, which add
> >> IMX support to Samsung DSIM. If one is interested, here is my git repo
> >> with all the PoC patches:
> >>
> >> https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework
> >
> > While this needs rework on the bridge chip side, I fear that we need
> > something like that to allow the bridge to control the sequencing of
> > the DSI host init. While most bridges that aren't controlled via the
> > DSI channel might be fine with just initializing the host right before
> > a video signal is driven, there are some that need a different
> > sequencing.
> >
> > The chip I'm currently looking at is a TC368767, where the datasheet
> > states that the DSI lanes must be in LP-11 before the reset is
> > released. While the datasheet doesn't specify what happens if that
> > sequence is violated, Marek Vasut found that the chip enters a test
> > mode if the lanes are not in LP-11 at that point and I can confirm this
> > observation.
>
> The SN65DSI84 also has this requirement according to the datasheet.
>
> > Now with the TC358767 being a DSI to (e)DP converter, we need to
> > release the chip from reset pretty early to establish the DP AUX
> > connection to communicate with the display, in order to find out which
> > video modes we can drive. As the chip is controlled via i2c in my case,
> > initializing the DSI host on first DSI command transaction is out and
> > doing so before the bridge pre_enable is way too late.
> >
> > What I would need for this chip to work properly is an explicit call,
> > like the mipi_dsi_host_init() added in the PoC above, to allow the
> > bridge driver to kick the DSI host initialization before releasing the
> > chip from reset state.
>
> So to sum up on the missing parts:
>
> 1. This series needs more reviews, but is generally agreed on.
> 2. Jagan will integrate Marek's mipi_dsi_host_init() PoC when respinning
> his series "drm: bridge: Add Samsung MIPI DSIM bridge".
> 3. Bridge drivers need to be adjusted to call mipi_dsi_host_init() if
> required.
>
> Did I get anything wrong here, or is there some point that I'm missing?
> Jagan, do you have any plan to pick up the threads?

I'm working on for next version series to pick a few changes from
Marek's comment on the DSIM series. Not sure about
mipi_dsi_host_init() but I will comment on it based on my new series
of tests.

Jagan.

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Dave Stevenson]

Hi Frieder.

Apologies Lucas - I missed your response.

On Wed, 6 Jul 2022 at 08:09, Frieder Schrempf
<frieder.schrempf@kontron.de> wrote:
>
> Am 10.06.22 um 09:52 schrieb Lucas Stach:
> > Hi,
> >
> > Am Mittwoch, dem 11.05.2022 um 16:58 +0200 schrieb Marek Szyprowski:
> >> Hi Dave,
> >>
> >> On 05.04.2022 13:43, Dave Stevenson wrote:
> >>> On Fri, 18 Mar 2022 at 12:25, Dave Stevenson
> >>> <dave.stevenson@raspberrypi.com>  wrote:
> >>>> On Fri, 4 Mar 2022 at 15:18, Dave Stevenson
> >>>> <dave.stevenson@raspberrypi.com>  wrote:
> >>>>> Hi All
> >>>> A gentle ping on this series. Any comments on the approach?
> >>>> Thanks.
> >>> I realise the merge window has just closed and therefore folks have
> >>> been busy, but no responses on this after a month?
> >>>
> >>> Do I give up and submit a patch to document that DSI is broken and no one cares?
> >>
> >> Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI
> >> DSIM bridge' thread, otherwise I would miss it since I'm not involved
> >> much in the DRM development.
> >>
> >> This resolves most of the issues in the Exynos DSI and its recent
> >> conversion to the drm bridge framework. I've added the needed
> >> prepare_upstream_first flags to the panels and everything works fine
> >> without the bridge chain order hacks.
> >>
> >> Feel free to add:
> >>
> >> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >>
> >>
> >> The only remaining thing to resolve is the moment of enabling DSI host.
> >> The proper sequence is:
> >>
> >> 1. host power on, 2. device power on, 3. host init, 4. device init, 5.
> >> video enable.
> >>
> >> #1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so
> >> far done in the first host transfer call, which usually happens in
> >> panel's prepare, then the #4 happens. Then video enable is done in the
> >> enable callbacks.
> >>
> >> Jagan wants to move it to the dsi host pre_enable() to let it work with
> >> DSI bridges controlled over different interfaces
> >> (https://lore.kernel.org/all/20220504114021.33265-6-jagan@amarulasolutions.com/
> >> ). This however fails on Exynos with DSI panels, because when dsi's
> >> pre_enable is called, the dsi device is not yet powered. I've discussed
> >> this with Andrzej Hajda and we came to the conclusion that this can be
> >> resolved by adding the init() callback to the struct mipi_dsi_host_ops.
> >> Then DSI client (next bridge or panel) would call it after powering self
> >> on, but before sending any DSI commands in its pre_enable/prepare functions.
> >>
> >> I've prepared a prototype of such solution. This approach finally
> >> resolved all the initialization issues! The bridge chain finally matches
> >> the hardware, no hack are needed, and everything is controlled by the
> >> DRM core. This prototype also includes the Jagan's patches, which add
> >> IMX support to Samsung DSIM. If one is interested, here is my git repo
> >> with all the PoC patches:
> >>
> >> https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework
> >
> > While this needs rework on the bridge chip side, I fear that we need
> > something like that to allow the bridge to control the sequencing of
> > the DSI host init. While most bridges that aren't controlled via the
> > DSI channel might be fine with just initializing the host right before
> > a video signal is driven, there are some that need a different
> > sequencing.
> >
> > The chip I'm currently looking at is a TC368767, where the datasheet
> > states that the DSI lanes must be in LP-11 before the reset is
> > released. While the datasheet doesn't specify what happens if that
> > sequence is violated, Marek Vasut found that the chip enters a test
> > mode if the lanes are not in LP-11 at that point and I can confirm this
> > observation.
>
> The SN65DSI84 also has this requirement according to the datasheet.

SN65DSI8[3|4|5] need LP-11 before the chip comes out of reset, but
that only happens as part of enabling the bridge chain. This patch set
allows it to request the DSI host to be initialised first in order to
meet that constraint, which is common with many DSI sink devices.

Lucas' comment with TC368767 is that it is doing other things for
display negotiation over the AUX channel prior to enabling the video
path, and that is needing the DSI interface to be enabled and in LP-11
much earlier (and potentially clock lane in HS if not using an
external clock).

> > Now with the TC358767 being a DSI to (e)DP converter, we need to
> > release the chip from reset pretty early to establish the DP AUX
> > connection to communicate with the display, in order to find out which
> > video modes we can drive. As the chip is controlled via i2c in my case,
> > initializing the DSI host on first DSI command transaction is out and
> > doing so before the bridge pre_enable is way too late.
> >
> > What I would need for this chip to work properly is an explicit call,
> > like the mipi_dsi_host_init() added in the PoC above, to allow the
> > bridge driver to kick the DSI host initialization before releasing the
> > chip from reset state.
>
> So to sum up on the missing parts:
>
> 1. This series needs more reviews, but is generally agreed on.
> 2. Jagan will integrate Marek's mipi_dsi_host_init() PoC when respinning
> his series "drm: bridge: Add Samsung MIPI DSIM bridge".

I'm still not clear as to whether Marek's mipi_dsi_host_init is needed
in the majority of cases.
Exynos appeared to be trying to check for no contention as part of the
initialisation to LP-11, and that isn't necessary. No one has come
back on that one.

Adding a mipi_dsi_host_init would potentially solve the additional
issue for TC358767.
However it leaves a number of open questions. The first I can think of
is that there are no defined mechanisms for specifying the link
frequency prior to having a video mode set. Yes struct mipi_dsi_device
has hs_rate, but that is defined as the maximum HS rate that the
device supports, and therefore open to variation in the
implementation. Exynos has various vendor specific DT parameters to
configure link frequencies, which ought to be standardised if that is
the preferred approach.

> 3. Bridge drivers need to be adjusted to call mipi_dsi_host_init() if
> required.

Which hopefully is only the weirder devices such as TC358767.
SN65DSI8[3|4|5] do not need this, but they will need
pre_enable_upstream_first if/when the enable logic is shifted from
atomic_enable to atomic_pre_enable.

Cheers.
  Dave

> Did I get anything wrong here, or is there some point that I'm missing?
> Jagan, do you have any plan to pick up the threads?
>
> Thanks
> Frieder

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Frieder Schrempf]

Hi Dave,

Am 06.07.22 um 12:27 schrieb Dave Stevenson:
> Hi Frieder.
> 
> Apologies Lucas - I missed your response.
> 
> On Wed, 6 Jul 2022 at 08:09, Frieder Schrempf
> <frieder.schrempf@kontron.de> wrote:
>>
>> Am 10.06.22 um 09:52 schrieb Lucas Stach:
>>> Hi,
>>>
>>> Am Mittwoch, dem 11.05.2022 um 16:58 +0200 schrieb Marek Szyprowski:
>>>> Hi Dave,
>>>>
>>>> On 05.04.2022 13:43, Dave Stevenson wrote:
>>>>> On Fri, 18 Mar 2022 at 12:25, Dave Stevenson
>>>>> <dave.stevenson@raspberrypi.com>  wrote:
>>>>>> On Fri, 4 Mar 2022 at 15:18, Dave Stevenson
>>>>>> <dave.stevenson@raspberrypi.com>  wrote:
>>>>>>> Hi All
>>>>>> A gentle ping on this series. Any comments on the approach?
>>>>>> Thanks.
>>>>> I realise the merge window has just closed and therefore folks have
>>>>> been busy, but no responses on this after a month?
>>>>>
>>>>> Do I give up and submit a patch to document that DSI is broken and no one cares?
>>>>
>>>> Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI
>>>> DSIM bridge' thread, otherwise I would miss it since I'm not involved
>>>> much in the DRM development.
>>>>
>>>> This resolves most of the issues in the Exynos DSI and its recent
>>>> conversion to the drm bridge framework. I've added the needed
>>>> prepare_upstream_first flags to the panels and everything works fine
>>>> without the bridge chain order hacks.
>>>>
>>>> Feel free to add:
>>>>
>>>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>>
>>>>
>>>> The only remaining thing to resolve is the moment of enabling DSI host.
>>>> The proper sequence is:
>>>>
>>>> 1. host power on, 2. device power on, 3. host init, 4. device init, 5.
>>>> video enable.
>>>>
>>>> #1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so
>>>> far done in the first host transfer call, which usually happens in
>>>> panel's prepare, then the #4 happens. Then video enable is done in the
>>>> enable callbacks.
>>>>
>>>> Jagan wants to move it to the dsi host pre_enable() to let it work with
>>>> DSI bridges controlled over different interfaces
>>>> (https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20220504114021.33265-6-jagan%40amarulasolutions.com%2F&amp;data=05%7C01%7Cfrieder.schrempf%40kontron.de%7Ca82478efb8434ac07dfb08da5f3a1b75%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C637927000416444951%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=vgnJ8L7fhloUV83p%2Be6ziUKHbL9apqcLWpDvMUcOOoY%3D&amp;reserved=0
>>>> ). This however fails on Exynos with DSI panels, because when dsi's
>>>> pre_enable is called, the dsi device is not yet powered. I've discussed
>>>> this with Andrzej Hajda and we came to the conclusion that this can be
>>>> resolved by adding the init() callback to the struct mipi_dsi_host_ops.
>>>> Then DSI client (next bridge or panel) would call it after powering self
>>>> on, but before sending any DSI commands in its pre_enable/prepare functions.
>>>>
>>>> I've prepared a prototype of such solution. This approach finally
>>>> resolved all the initialization issues! The bridge chain finally matches
>>>> the hardware, no hack are needed, and everything is controlled by the
>>>> DRM core. This prototype also includes the Jagan's patches, which add
>>>> IMX support to Samsung DSIM. If one is interested, here is my git repo
>>>> with all the PoC patches:
>>>>
>>>> https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmszyprow%2Flinux%2Ftree%2Fv5.18-next-20220511-dsi-rework&amp;data=05%7C01%7Cfrieder.schrempf%40kontron.de%7Ca82478efb8434ac07dfb08da5f3a1b75%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C637927000416444951%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=ka2wikbVhc4RxFGARjTh0ixGKiaNHloW9dVkejA5kEg%3D&amp;reserved=0
>>>
>>> While this needs rework on the bridge chip side, I fear that we need
>>> something like that to allow the bridge to control the sequencing of
>>> the DSI host init. While most bridges that aren't controlled via the
>>> DSI channel might be fine with just initializing the host right before
>>> a video signal is driven, there are some that need a different
>>> sequencing.
>>>
>>> The chip I'm currently looking at is a TC368767, where the datasheet
>>> states that the DSI lanes must be in LP-11 before the reset is
>>> released. While the datasheet doesn't specify what happens if that
>>> sequence is violated, Marek Vasut found that the chip enters a test
>>> mode if the lanes are not in LP-11 at that point and I can confirm this
>>> observation.
>>
>> The SN65DSI84 also has this requirement according to the datasheet.
> 
> SN65DSI8[3|4|5] need LP-11 before the chip comes out of reset, but
> that only happens as part of enabling the bridge chain. This patch set
> allows it to request the DSI host to be initialised first in order to
> meet that constraint, which is common with many DSI sink devices.
> 
> Lucas' comment with TC368767 is that it is doing other things for
> display negotiation over the AUX channel prior to enabling the video
> path, and that is needing the DSI interface to be enabled and in LP-11
> much earlier (and potentially clock lane in HS if not using an
> external clock).

Ok, got it know. I see this is a separate issue that is specific to
devices that need the link for video mode negotiation early. Thanks for
the explanation!

> 
>>> Now with the TC358767 being a DSI to (e)DP converter, we need to
>>> release the chip from reset pretty early to establish the DP AUX
>>> connection to communicate with the display, in order to find out which
>>> video modes we can drive. As the chip is controlled via i2c in my case,
>>> initializing the DSI host on first DSI command transaction is out and
>>> doing so before the bridge pre_enable is way too late.
>>>
>>> What I would need for this chip to work properly is an explicit call,
>>> like the mipi_dsi_host_init() added in the PoC above, to allow the
>>> bridge driver to kick the DSI host initialization before releasing the
>>> chip from reset state.
>>
>> So to sum up on the missing parts:
>>
>> 1. This series needs more reviews, but is generally agreed on.
>> 2. Jagan will integrate Marek's mipi_dsi_host_init() PoC when respinning
>> his series "drm: bridge: Add Samsung MIPI DSIM bridge".
> 
> I'm still not clear as to whether Marek's mipi_dsi_host_init is needed
> in the majority of cases.
> Exynos appeared to be trying to check for no contention as part of the
> initialisation to LP-11, and that isn't necessary. No one has come
> back on that one.
> 
> Adding a mipi_dsi_host_init would potentially solve the additional
> issue for TC358767.
> However it leaves a number of open questions. The first I can think of
> is that there are no defined mechanisms for specifying the link
> frequency prior to having a video mode set. Yes struct mipi_dsi_device
> has hs_rate, but that is defined as the maximum HS rate that the
> device supports, and therefore open to variation in the
> implementation. Exynos has various vendor specific DT parameters to
> configure link frequencies, which ought to be standardised if that is
> the preferred approach.

Having standardized DT properties to configure the early link parameters
could be a possible solution.

> 
>> 3. Bridge drivers need to be adjusted to call mipi_dsi_host_init() if
>> required.
> 
> Which hopefully is only the weirder devices such as TC358767.
> SN65DSI8[3|4|5] do not need this, but they will need
> pre_enable_upstream_first if/when the enable logic is shifted from
> atomic_enable to atomic_pre_enable.

Got it, thanks for clarifying!

Thanks
Frieder

Re: [PATCH V2 1/4] drm/bridge: Remove duplication from drm_bridge and drm_atomic_bridge chains

[Sam Ravnborg]

Hi Dmitry,

On Wed, Jun 08, 2022 at 02:00:57PM +0300, Dmitry Baryshkov wrote:
> On 04/03/2022 18:17, Dave Stevenson wrote:
> > drm_bridge_chain_pre_enable is a subset of
> > drm_atomic_bridge_chain_pre_enable, and drm_bridge_chain_post_disable
> > is a subset of drm_atomic_bridge_chain_post_disable.
> > 
> > Change drm_bridge_chain_pre_enable and drm_bridge_chain_post_disable to
> > call the atomic versions with a NULL state, and ensure that atomic
> > calls are not made if there is no state.
> > 
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> I think we should update parade-ps8640 to use drm_atomic_bridge_chain_() and
> drop drm_bridge_chain_* API completely.

This is done in a few of the patches you can find here:
https://lore.kernel.org/dri-devel/20220717174454.46616-1-sam@ravnborg.org/

Review/testing would be appreciated.

	Sam

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Sam Ravnborg]

Hi Dave,

a long overdue reply on this series.

On Fri, Mar 04, 2022 at 03:17:55PM +0000, Dave Stevenson wrote:
> Hi All
> 
> Changes from v1:
> - New patch to refactor drm_bridge_chain_post_disable and drm_bridge_chain_pre_enable
>   to reuse drm_atomic_bridge_chain_post_disable / drm_atomic_bridge_chain_pre_enable
>   but with a NULL state.
> - New patch that adds a pre_enable_upstream_first to drm_panel.
> - changed from an OPS flag to a bool "pre_enable_upstream_first" in drm_bridge.
> - Followed Andrzej's suggestion of using continue in the main loop to avoid
>   needing 2 additional loops (one forward to find the last bridge wanting
>   upstream first, and the second backwards again).
> - Actioned Laurent's review comments on docs patch.
> 
> Original cover letter: 
> 
> Hopefully I've cc'ed all those that have bashed this problem around previously,
> or are otherwise linked to DRM bridges.
> 
> There have been numerous discussions around how DSI support is currently broken
> as it doesn't support initialising the PHY to LP-11 and potentially the clock
> lane to HS prior to configuring the DSI peripheral. There is no op where the
> interface is initialised but HS video isn't also being sent.
> Currently you have:
> - peripheral pre_enable (host not initialised yet)
> - host pre_enable
> - encoder enable
> - host enable
> - peripheral enable (video already running)
> 
> vc4 and exynos currently implement the DSI host as an encoder, and split the
> bridge_chain. This fails if you want to switch to being a bridge and/or use
> atomic calls as the state of all the elements split off are not added by
> drm_atomic_add_encoder_bridges.

A typically chain looks like this:

CRTC => Encoder => Bridge A => Bridge B

We have in DRM bridges established what is the "next" bridge - indicated
with the direction of the arrows in the drawing.

This set of patches introduces the concept of "upstream" bridges.

pre_enable_prev_bridge_first would be easier to understand as it uses
the current terminology.
I get that "upstream" is used in the DSI specification - but we are
dealing with bridges that happens to support DSI and more, and mixing
the two terminologies is not good.

Note: Upstream is also used in a bridge doc section - here it should
      most likely be updated too.

The current approach set a flag that magically makes the core do something
else. Have you considered a much more explicit approach?

A few helpers like:

	drm_bridge_pre_enable_prev_bridge()
	drm_bridge_enable_prev_bridge()
	drm_bridge_disable_prev_bridge()
	drm_bridge_post_disable_prev_bridge()

And then update the core so the relevant function is only called once
for a bridge.
Then the need for DSI lanes in LP-11 can be archived by a call to

	drm_bridge_pre_enable_prev_bridge()

This is more explicit than a flag that triggers some magic behaviour.
It may even see uses we have not realised yet.

It is late here - so maybe the above is not a good idea tomorrow - but
right now I like the simplicity of it.

Other than the above I read that a mipi_dsi_host_init() is planned,
which is also explicit and simple - good.

Have we seen a new revision of some of these?
Chances are high that I have missed it then.

	Sam

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Dave Stevenson]

Hi Sam

On Mon, 18 Jul 2022 at 21:52, Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Dave,
>
> a long overdue reply on this series.
>
> On Fri, Mar 04, 2022 at 03:17:55PM +0000, Dave Stevenson wrote:
> > Hi All
> >
> > Changes from v1:
> > - New patch to refactor drm_bridge_chain_post_disable and drm_bridge_chain_pre_enable
> >   to reuse drm_atomic_bridge_chain_post_disable / drm_atomic_bridge_chain_pre_enable
> >   but with a NULL state.
> > - New patch that adds a pre_enable_upstream_first to drm_panel.
> > - changed from an OPS flag to a bool "pre_enable_upstream_first" in drm_bridge.
> > - Followed Andrzej's suggestion of using continue in the main loop to avoid
> >   needing 2 additional loops (one forward to find the last bridge wanting
> >   upstream first, and the second backwards again).
> > - Actioned Laurent's review comments on docs patch.
> >
> > Original cover letter:
> >
> > Hopefully I've cc'ed all those that have bashed this problem around previously,
> > or are otherwise linked to DRM bridges.
> >
> > There have been numerous discussions around how DSI support is currently broken
> > as it doesn't support initialising the PHY to LP-11 and potentially the clock
> > lane to HS prior to configuring the DSI peripheral. There is no op where the
> > interface is initialised but HS video isn't also being sent.
> > Currently you have:
> > - peripheral pre_enable (host not initialised yet)
> > - host pre_enable
> > - encoder enable
> > - host enable
> > - peripheral enable (video already running)
> >
> > vc4 and exynos currently implement the DSI host as an encoder, and split the
> > bridge_chain. This fails if you want to switch to being a bridge and/or use
> > atomic calls as the state of all the elements split off are not added by
> > drm_atomic_add_encoder_bridges.
>
> A typically chain looks like this:
>
> CRTC => Encoder => Bridge A => Bridge B
>
> We have in DRM bridges established what is the "next" bridge - indicated
> with the direction of the arrows in the drawing.
>
> This set of patches introduces the concept of "upstream" bridges.
>
> pre_enable_prev_bridge_first would be easier to understand as it uses
> the current terminology.
> I get that "upstream" is used in the DSI specification - but we are
> dealing with bridges that happens to support DSI and more, and mixing
> the two terminologies is not good.
>
> Note: Upstream is also used in a bridge doc section - here it should
>       most likely be updated too.

Sure, I have no issues with switching to prev/next from upstream/downstream.
To the outsider it can be confusing - in pre_enable and disable, the
next bridge to be called is the previous one. At least it is
documented.

> The current approach set a flag that magically makes the core do something
> else. Have you considered a much more explicit approach?
>
> A few helpers like:
>
>         drm_bridge_pre_enable_prev_bridge()
>         drm_bridge_enable_prev_bridge()
>         drm_bridge_disable_prev_bridge()
>         drm_bridge_post_disable_prev_bridge()

No point in drm_bridge_enable_prev_bridge() and
drm_bridge_post_disable_prev_bridge() as the call order down the chain
will mean that they have already been called.
drm_bridge_enable_next_bridge() and
drm_bridge_post_disable_next_bridge() possibly.

> And then update the core so the relevant function is only called once
> for a bridge.
> Then the need for DSI lanes in LP-11 can be archived by a call to
>
>         drm_bridge_pre_enable_prev_bridge()

Unfortunately it gets ugly with post_disable.
The DSI host controller post_disable will have been called before the
DSI peripheral's post_disable, and there are conditions where the
peripheral needs to send DSI commands in post_disable (eg
panel-asus-z00t-tm5p5-n35596 [1]). Changing all DSI hosts to call
drm_bridge_post_disable_next_bridge feels like the wrong thing to do.
There are currently hacks in dw-mipi-dsi that do call the next
panel/bridge post_disable [2] and it would be nice to get rid of them.
Currently the calls aren't tracked for state, so you end up with
post_disable being called twice, and panels having to track state (eg
jdi_lt070me050000 [3]).

[1] tm5p5_nt35596_unprepare() calls tm5p5_nt35596_off()
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c#L107
[2] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L889
[3] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c#L44

> This is more explicit than a flag that triggers some magic behaviour.
> It may even see uses we have not realised yet.

Personally it feels like more boilerplate in almost all DSI drivers,
and generally I see a push to remove boilerplate.

> It is late here - so maybe the above is not a good idea tomorrow - but
> right now I like the simplicity of it.
>
> Other than the above I read that a mipi_dsi_host_init() is planned,
> which is also explicit and simple - good.

It's been raised, but the justification for most use cases hasn't been
made. The Exynos conversion looks to be doing the wrong thing in
checking state, and that's why it is currently needing it.
Again it's also more boilerplate.

TC358767 is an odd one as it wants the DSI interface enabled very
early in order to have a clock for the DP aux channel well before
video is running. I had a thought on that, but It looks like I haven't
hit send on a reply to Lucas on that one - too many distractions.

> Have we seen a new revision of some of these?
> Chances are high that I have missed it then.

No, still on V2. Other than Dmitry's comment over updating
parade-ps8640 and dropping drm_bridge_chain_*, no real comments had
been made.

  Dave

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Dave Stevenson]

Hi Lucas

On Fri, 10 Jun 2022 at 08:52, Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Hi,
>
> Am Mittwoch, dem 11.05.2022 um 16:58 +0200 schrieb Marek Szyprowski:
> > Hi Dave,
> >
> > On 05.04.2022 13:43, Dave Stevenson wrote:
> > > On Fri, 18 Mar 2022 at 12:25, Dave Stevenson
> > > <dave.stevenson@raspberrypi.com>  wrote:
> > > > On Fri, 4 Mar 2022 at 15:18, Dave Stevenson
> > > > <dave.stevenson@raspberrypi.com>  wrote:
> > > > > Hi All
> > > > A gentle ping on this series. Any comments on the approach?
> > > > Thanks.
> > > I realise the merge window has just closed and therefore folks have
> > > been busy, but no responses on this after a month?
> > >
> > > Do I give up and submit a patch to document that DSI is broken and no one cares?
> >
> > Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI
> > DSIM bridge' thread, otherwise I would miss it since I'm not involved
> > much in the DRM development.
> >
> > This resolves most of the issues in the Exynos DSI and its recent
> > conversion to the drm bridge framework. I've added the needed
> > prepare_upstream_first flags to the panels and everything works fine
> > without the bridge chain order hacks.
> >
> > Feel free to add:
> >
> > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >
> >
> > The only remaining thing to resolve is the moment of enabling DSI host.
> > The proper sequence is:
> >
> > 1. host power on, 2. device power on, 3. host init, 4. device init, 5.
> > video enable.
> >
> > #1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so
> > far done in the first host transfer call, which usually happens in
> > panel's prepare, then the #4 happens. Then video enable is done in the
> > enable callbacks.
> >
> > Jagan wants to move it to the dsi host pre_enable() to let it work with
> > DSI bridges controlled over different interfaces
> > (https://lore.kernel.org/all/20220504114021.33265-6-jagan@amarulasolutions.com/
> > ). This however fails on Exynos with DSI panels, because when dsi's
> > pre_enable is called, the dsi device is not yet powered. I've discussed
> > this with Andrzej Hajda and we came to the conclusion that this can be
> > resolved by adding the init() callback to the struct mipi_dsi_host_ops.
> > Then DSI client (next bridge or panel) would call it after powering self
> > on, but before sending any DSI commands in its pre_enable/prepare functions.
> >
> > I've prepared a prototype of such solution. This approach finally
> > resolved all the initialization issues! The bridge chain finally matches
> > the hardware, no hack are needed, and everything is controlled by the
> > DRM core. This prototype also includes the Jagan's patches, which add
> > IMX support to Samsung DSIM. If one is interested, here is my git repo
> > with all the PoC patches:
> >
> > https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework
>
> While this needs rework on the bridge chip side, I fear that we need
> something like that to allow the bridge to control the sequencing of
> the DSI host init. While most bridges that aren't controlled via the
> DSI channel might be fine with just initializing the host right before
> a video signal is driven, there are some that need a different
> sequencing.
>
> The chip I'm currently looking at is a TC368767, where the datasheet
> states that the DSI lanes must be in LP-11 before the reset is
> released. While the datasheet doesn't specify what happens if that
> sequence is violated, Marek Vasut found that the chip enters a test
> mode if the lanes are not in LP-11 at that point and I can confirm this
> observation.
> Now with the TC358767 being a DSI to (e)DP converter, we need to
> release the chip from reset pretty early to establish the DP AUX
> connection to communicate with the display, in order to find out which
> video modes we can drive. As the chip is controlled via i2c in my case,
> initializing the DSI host on first DSI command transaction is out and
> doing so before the bridge pre_enable is way too late.
>
> What I would need for this chip to work properly is an explicit call,
> like the mipi_dsi_host_init() added in the PoC above, to allow the
> bridge driver to kick the DSI host initialization before releasing the
> chip from reset state.

This is going off on a slight tangent from the original patch set, but
a thought has just come to mind for this use case.

For this sort of bridge device where you want to power up early
(either just for LP-11 state, or for HS on the clock lane), is power
up at mipi_dsi_attach, and power down at mipi_dsi_detach sufficient?
We have mode_flags in struct mipi_dsi_device passed in
mipi_dsi_attach, so an extra flag in there (eg
MIPI_DSI_EARLY_POWER_ON) would be very easy and be a simple way to
signal an alternate DSI host behaviour.

It still leaves the configuration of link frequency as an open
question, but potentially solves the sequencing issue. Just a thought.
Perhaps dsi_attach() is still too late in the process.

  Dave

Re: [PATCH V2 2/4] drm/bridge: Introduce pre_enable_upstream_first to alter bridge init order

[Jagan Teki]

Hi Dave,

On Fri, Mar 4, 2022 at 8:48 PM Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> DSI sink devices typically want the DSI host powered up and configured
> before they are powered up. pre_enable is the place this would normally
> happen, but they are called in reverse order from panel/connector towards
> the encoder, which is the "wrong" order.
>
> Add a new flag pre_enable_upstream_first that any bridge can set
> to swap the order of pre_enable (and post_disable) for that and the
> immediately upstream bridge.
> Should the immediately upstream bridge also set the
> pre_enable_upstream_first flag, the bridge upstream of that will be called
> before either of those which requested pre_enable_upstream_first.
>
> eg:
> - Panel
> - Bridge 1
> - Bridge 2 pre_enable_upstream_first
> - Bridge 3
> - Bridge 4 pre_enable_upstream_first
> - Bridge 5 pre_enable_upstream_first
> - Bridge 6
> - Encoder
> Would result in pre_enable's being called as Panel, Bridge 1, Bridge 3,
> Bridge 2, Bridge 6, Bridge 5, Bridge 4, Encoder.

Look like this is something that I've reproduced before, isn't it?
https://patchwork.kernel.org/project/dri-devel/patch/20210214194102.126146-6-jagan@amarulasolutions.com/

Jagan.

Re: [PATCH V2 3/4] drm/panel: Add prepare_upstream_first flag to drm_panel

[Jagan Teki]

On Fri, Mar 4, 2022 at 8:48 PM Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Mapping to the drm_bridge flag pre_enable_upstream_first,
> add a new flag prepare_upstream_first to drm_panel to allow
> the panel driver to request that the upstream bridge should
> be pre_enabled before the panel prepare.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/gpu/drm/bridge/panel.c |  3 +++
>  include/drm/drm_panel.h        | 10 ++++++++++
>  2 files changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index 5be057575183..2ea08b3ba326 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -234,6 +234,9 @@ struct drm_bridge *drm_panel_bridge_add_typed(struct drm_panel *panel,
>         panel_bridge->bridge.ops = DRM_BRIDGE_OP_MODES;
>         panel_bridge->bridge.type = connector_type;
>
> +       panel_bridge->bridge.pre_enable_upstream_first =
> +                                               panel->prepare_upstream_first;
> +

panel_bridge is common for bridge users who used panel and those who
might not need upstream first, so better to handle per bridge user
whoever needs this.

Thanks,
Jagan.

Re: [PATCH V2 3/4] drm/panel: Add prepare_upstream_first flag to drm_panel

[Dave Stevenson]

Hi Jagan

On Thu, 6 Oct 2022 at 15:25, Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> On Fri, Mar 4, 2022 at 8:48 PM Dave Stevenson
> <dave.stevenson@raspberrypi.com> wrote:
> >
> > Mapping to the drm_bridge flag pre_enable_upstream_first,
> > add a new flag prepare_upstream_first to drm_panel to allow
> > the panel driver to request that the upstream bridge should
> > be pre_enabled before the panel prepare.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/gpu/drm/bridge/panel.c |  3 +++
> >  include/drm/drm_panel.h        | 10 ++++++++++
> >  2 files changed, 13 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> > index 5be057575183..2ea08b3ba326 100644
> > --- a/drivers/gpu/drm/bridge/panel.c
> > +++ b/drivers/gpu/drm/bridge/panel.c
> > @@ -234,6 +234,9 @@ struct drm_bridge *drm_panel_bridge_add_typed(struct drm_panel *panel,
> >         panel_bridge->bridge.ops = DRM_BRIDGE_OP_MODES;
> >         panel_bridge->bridge.type = connector_type;
> >
> > +       panel_bridge->bridge.pre_enable_upstream_first =
> > +                                               panel->prepare_upstream_first;
> > +
>
> panel_bridge is common for bridge users who used panel and those who
> might not need upstream first, so better to handle per bridge user
> whoever needs this.

Sorry, I don't follow you.

prepare_upstream_first is coming from a struct drm_panel, generally
for DSI panels are still panel drivers. An example would be Ilitek
ILI9881C.
It's a feature of the panel that would dictate that they want their
source initialised first.

The source bridge for the panel will call devm_drm_of_get_bridge,
which will call devm_drm_panel_bridge_add. Nothing outside of those
two functions have both the panel and bridge handles, so are you
proposing that the assignment should be done in devm_drm_of_get_bridge
[1]? That would leave the behaviour of drivers calling
drm_panel_bridge_add(_typed) directly as they were.

Thanks.
  Dave

[1] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/panel.c#L418

Re: [PATCH V2 3/4] drm/panel: Add prepare_upstream_first flag to drm_panel

[Jagan Teki]

Hi Dave,

On Fri, Oct 7, 2022 at 6:26 PM Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi Jagan
>
> On Thu, 6 Oct 2022 at 15:25, Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > On Fri, Mar 4, 2022 at 8:48 PM Dave Stevenson
> > <dave.stevenson@raspberrypi.com> wrote:
> > >
> > > Mapping to the drm_bridge flag pre_enable_upstream_first,
> > > add a new flag prepare_upstream_first to drm_panel to allow
> > > the panel driver to request that the upstream bridge should
> > > be pre_enabled before the panel prepare.
> > >
> > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > ---
> > >  drivers/gpu/drm/bridge/panel.c |  3 +++
> > >  include/drm/drm_panel.h        | 10 ++++++++++
> > >  2 files changed, 13 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> > > index 5be057575183..2ea08b3ba326 100644
> > > --- a/drivers/gpu/drm/bridge/panel.c
> > > +++ b/drivers/gpu/drm/bridge/panel.c
> > > @@ -234,6 +234,9 @@ struct drm_bridge *drm_panel_bridge_add_typed(struct drm_panel *panel,
> > >         panel_bridge->bridge.ops = DRM_BRIDGE_OP_MODES;
> > >         panel_bridge->bridge.type = connector_type;
> > >
> > > +       panel_bridge->bridge.pre_enable_upstream_first =
> > > +                                               panel->prepare_upstream_first;
> > > +
> >
> > panel_bridge is common for bridge users who used panel and those who
> > might not need upstream first, so better to handle per bridge user
> > whoever needs this.
>
> Sorry, I don't follow you.

panel_bridge driver is a common bridge for drm_panel_bridge_add
registered bridges. If we enable pre_enable_upstream_first globally in
panel_bridge driver then it affects panes that don't require
pre_enable first for that bridge chain. Hope you understand.

Thanks,
Jagan.

Re: [PATCH V2 3/4] drm/panel: Add prepare_upstream_first flag to drm_panel

[Dave Stevenson]

Hi Jagan

On Mon, 17 Oct 2022 at 03:44, Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> Hi Dave,
>
> On Fri, Oct 7, 2022 at 6:26 PM Dave Stevenson
> <dave.stevenson@raspberrypi.com> wrote:
> >
> > Hi Jagan
> >
> > On Thu, 6 Oct 2022 at 15:25, Jagan Teki <jagan@amarulasolutions.com> wrote:
> > >
> > > On Fri, Mar 4, 2022 at 8:48 PM Dave Stevenson
> > > <dave.stevenson@raspberrypi.com> wrote:
> > > >
> > > > Mapping to the drm_bridge flag pre_enable_upstream_first,
> > > > add a new flag prepare_upstream_first to drm_panel to allow
> > > > the panel driver to request that the upstream bridge should
> > > > be pre_enabled before the panel prepare.
> > > >
> > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > > ---
> > > >  drivers/gpu/drm/bridge/panel.c |  3 +++
> > > >  include/drm/drm_panel.h        | 10 ++++++++++
> > > >  2 files changed, 13 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> > > > index 5be057575183..2ea08b3ba326 100644
> > > > --- a/drivers/gpu/drm/bridge/panel.c
> > > > +++ b/drivers/gpu/drm/bridge/panel.c
> > > > @@ -234,6 +234,9 @@ struct drm_bridge *drm_panel_bridge_add_typed(struct drm_panel *panel,
> > > >         panel_bridge->bridge.ops = DRM_BRIDGE_OP_MODES;
> > > >         panel_bridge->bridge.type = connector_type;
> > > >
> > > > +       panel_bridge->bridge.pre_enable_upstream_first =
> > > > +                                               panel->prepare_upstream_first;
> > > > +
> > >
> > > panel_bridge is common for bridge users who used panel and those who
> > > might not need upstream first, so better to handle per bridge user
> > > whoever needs this.
> >
> > Sorry, I don't follow you.
>
> panel_bridge driver is a common bridge for drm_panel_bridge_add
> registered bridges. If we enable pre_enable_upstream_first globally in
> panel_bridge driver then it affects panes that don't require
> pre_enable first for that bridge chain. Hope you understand.

No, sorry, I'm still not getting your point.

It is not enabled globally.
If (and only if) the specific panel driver has set
prepare_upstream_first in the struct drm_panel passed to
drm_panel_add(), then that setting is replicated in the associated
struct drm_bridge pre_enable_upstream_first.

Can you give an example of where you see this being an issue?

You proposed handling this on a per bridge user basis? What exactly
are you calling a bridge user in that case? The DSI host (or
equivalent) source to the panel? Because the panel driver has no idea
it is being wrapped into a drm_bridge.
However that source device can't alter the bridge chain call order
(breaking the chain as Exynos and vc4 do does not work with the atomic
API in "entertaining" ways), and it has no knowledge of the behaviour
the attached panel wants, nor does it know that it's going through
panel_bridge.

As per my previous email, devm_drm_of_get_bridge is the only other
place in the callstack that has both the drm_panel and drm_bridge
handles. Does putting the assignment from drm_panel to drm_bridge in
there solve your concern?

Thanks.
  Dave

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Dmitry Baryshkov]

Hi Dave,

On 19/07/2022 16:45, Dave Stevenson wrote:
> Hi Sam
> 
> On Mon, 18 Jul 2022 at 21:52, Sam Ravnborg <sam@ravnborg.org> wrote:
>>
>> Hi Dave,
>>
>> a long overdue reply on this series.
>>
>> On Fri, Mar 04, 2022 at 03:17:55PM +0000, Dave Stevenson wrote:
>>> Hi All
>>>
>>> Changes from v1:
>>> - New patch to refactor drm_bridge_chain_post_disable and drm_bridge_chain_pre_enable
>>>    to reuse drm_atomic_bridge_chain_post_disable / drm_atomic_bridge_chain_pre_enable
>>>    but with a NULL state.
>>> - New patch that adds a pre_enable_upstream_first to drm_panel.
>>> - changed from an OPS flag to a bool "pre_enable_upstream_first" in drm_bridge.
>>> - Followed Andrzej's suggestion of using continue in the main loop to avoid
>>>    needing 2 additional loops (one forward to find the last bridge wanting
>>>    upstream first, and the second backwards again).
>>> - Actioned Laurent's review comments on docs patch.
>>>
>>> Original cover letter:
>>>
>>> Hopefully I've cc'ed all those that have bashed this problem around previously,
>>> or are otherwise linked to DRM bridges.
>>>
>>> There have been numerous discussions around how DSI support is currently broken
>>> as it doesn't support initialising the PHY to LP-11 and potentially the clock
>>> lane to HS prior to configuring the DSI peripheral. There is no op where the
>>> interface is initialised but HS video isn't also being sent.
>>> Currently you have:
>>> - peripheral pre_enable (host not initialised yet)
>>> - host pre_enable
>>> - encoder enable
>>> - host enable
>>> - peripheral enable (video already running)
>>>
>>> vc4 and exynos currently implement the DSI host as an encoder, and split the
>>> bridge_chain. This fails if you want to switch to being a bridge and/or use
>>> atomic calls as the state of all the elements split off are not added by
>>> drm_atomic_add_encoder_bridges.
>>
>> A typically chain looks like this:
>>
>> CRTC => Encoder => Bridge A => Bridge B
>>
>> We have in DRM bridges established what is the "next" bridge - indicated
>> with the direction of the arrows in the drawing.
>>
>> This set of patches introduces the concept of "upstream" bridges.
>>
>> pre_enable_prev_bridge_first would be easier to understand as it uses
>> the current terminology.
>> I get that "upstream" is used in the DSI specification - but we are
>> dealing with bridges that happens to support DSI and more, and mixing
>> the two terminologies is not good.
>>
>> Note: Upstream is also used in a bridge doc section - here it should
>>        most likely be updated too.
> 
> Sure, I have no issues with switching to prev/next from upstream/downstream.
> To the outsider it can be confusing - in pre_enable and disable, the
> next bridge to be called is the previous one. At least it is
> documented.
> 
>> The current approach set a flag that magically makes the core do something
>> else. Have you considered a much more explicit approach?
>>
>> A few helpers like:
>>
>>          drm_bridge_pre_enable_prev_bridge()
>>          drm_bridge_enable_prev_bridge()
>>          drm_bridge_disable_prev_bridge()
>>          drm_bridge_post_disable_prev_bridge()
> 
> No point in drm_bridge_enable_prev_bridge() and
> drm_bridge_post_disable_prev_bridge() as the call order down the chain
> will mean that they have already been called.
> drm_bridge_enable_next_bridge() and
> drm_bridge_post_disable_next_bridge() possibly.
> 
>> And then update the core so the relevant function is only called once
>> for a bridge.
>> Then the need for DSI lanes in LP-11 can be archived by a call to
>>
>>          drm_bridge_pre_enable_prev_bridge()
> 
> Unfortunately it gets ugly with post_disable.
> The DSI host controller post_disable will have been called before the
> DSI peripheral's post_disable, and there are conditions where the
> peripheral needs to send DSI commands in post_disable (eg
> panel-asus-z00t-tm5p5-n35596 [1]). Changing all DSI hosts to call
> drm_bridge_post_disable_next_bridge feels like the wrong thing to do.
> There are currently hacks in dw-mipi-dsi that do call the next
> panel/bridge post_disable [2] and it would be nice to get rid of them.
> Currently the calls aren't tracked for state, so you end up with
> post_disable being called twice, and panels having to track state (eg
> jdi_lt070me050000 [3]).
> 
> [1] tm5p5_nt35596_unprepare() calls tm5p5_nt35596_off()
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c#L107
> [2] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L889
> [3] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c#L44
> 
>> This is more explicit than a flag that triggers some magic behaviour.
>> It may even see uses we have not realised yet.
> 
> Personally it feels like more boilerplate in almost all DSI drivers,
> and generally I see a push to remove boilerplate.
> 
>> It is late here - so maybe the above is not a good idea tomorrow - but
>> right now I like the simplicity of it.
>>
>> Other than the above I read that a mipi_dsi_host_init() is planned,
>> which is also explicit and simple - good.
> 
> It's been raised, but the justification for most use cases hasn't been
> made. The Exynos conversion looks to be doing the wrong thing in
> checking state, and that's why it is currently needing it.
> Again it's also more boilerplate.
> 
> TC358767 is an odd one as it wants the DSI interface enabled very
> early in order to have a clock for the DP aux channel well before
> video is running. I had a thought on that, but It looks like I haven't
> hit send on a reply to Lucas on that one - too many distractions.
> 
>> Have we seen a new revision of some of these?
>> Chances are high that I have missed it then.
> 
> No, still on V2. Other than Dmitry's comment over updating
> parade-ps8640 and dropping drm_bridge_chain_*, no real comments had
> been made.

It's been a while now. Do you still plan to pursue this patchset?

[personal notice: I'd prefer something less strange, e.g. an explicit 
calls to mipi_dsi_host, but as this patchset seems to fix the issues, 
I'm fine with it].

> 
>    Dave

-- 
With best wishes
Dmitry

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Dave Stevenson]

Hi Dmitry

On Sun, 13 Nov 2022 at 13:06, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> Hi Dave,
>
> On 19/07/2022 16:45, Dave Stevenson wrote:
> > Hi Sam
> >
> > On Mon, 18 Jul 2022 at 21:52, Sam Ravnborg <sam@ravnborg.org> wrote:
> >>
> >> Hi Dave,
> >>
> >> a long overdue reply on this series.
> >>
> >> On Fri, Mar 04, 2022 at 03:17:55PM +0000, Dave Stevenson wrote:
> >>> Hi All
> >>>
> >>> Changes from v1:
> >>> - New patch to refactor drm_bridge_chain_post_disable and drm_bridge_chain_pre_enable
> >>>    to reuse drm_atomic_bridge_chain_post_disable / drm_atomic_bridge_chain_pre_enable
> >>>    but with a NULL state.
> >>> - New patch that adds a pre_enable_upstream_first to drm_panel.
> >>> - changed from an OPS flag to a bool "pre_enable_upstream_first" in drm_bridge.
> >>> - Followed Andrzej's suggestion of using continue in the main loop to avoid
> >>>    needing 2 additional loops (one forward to find the last bridge wanting
> >>>    upstream first, and the second backwards again).
> >>> - Actioned Laurent's review comments on docs patch.
> >>>
> >>> Original cover letter:
> >>>
> >>> Hopefully I've cc'ed all those that have bashed this problem around previously,
> >>> or are otherwise linked to DRM bridges.
> >>>
> >>> There have been numerous discussions around how DSI support is currently broken
> >>> as it doesn't support initialising the PHY to LP-11 and potentially the clock
> >>> lane to HS prior to configuring the DSI peripheral. There is no op where the
> >>> interface is initialised but HS video isn't also being sent.
> >>> Currently you have:
> >>> - peripheral pre_enable (host not initialised yet)
> >>> - host pre_enable
> >>> - encoder enable
> >>> - host enable
> >>> - peripheral enable (video already running)
> >>>
> >>> vc4 and exynos currently implement the DSI host as an encoder, and split the
> >>> bridge_chain. This fails if you want to switch to being a bridge and/or use
> >>> atomic calls as the state of all the elements split off are not added by
> >>> drm_atomic_add_encoder_bridges.
> >>
> >> A typically chain looks like this:
> >>
> >> CRTC => Encoder => Bridge A => Bridge B
> >>
> >> We have in DRM bridges established what is the "next" bridge - indicated
> >> with the direction of the arrows in the drawing.
> >>
> >> This set of patches introduces the concept of "upstream" bridges.
> >>
> >> pre_enable_prev_bridge_first would be easier to understand as it uses
> >> the current terminology.
> >> I get that "upstream" is used in the DSI specification - but we are
> >> dealing with bridges that happens to support DSI and more, and mixing
> >> the two terminologies is not good.
> >>
> >> Note: Upstream is also used in a bridge doc section - here it should
> >>        most likely be updated too.
> >
> > Sure, I have no issues with switching to prev/next from upstream/downstream.
> > To the outsider it can be confusing - in pre_enable and disable, the
> > next bridge to be called is the previous one. At least it is
> > documented.
> >
> >> The current approach set a flag that magically makes the core do something
> >> else. Have you considered a much more explicit approach?
> >>
> >> A few helpers like:
> >>
> >>          drm_bridge_pre_enable_prev_bridge()
> >>          drm_bridge_enable_prev_bridge()
> >>          drm_bridge_disable_prev_bridge()
> >>          drm_bridge_post_disable_prev_bridge()
> >
> > No point in drm_bridge_enable_prev_bridge() and
> > drm_bridge_post_disable_prev_bridge() as the call order down the chain
> > will mean that they have already been called.
> > drm_bridge_enable_next_bridge() and
> > drm_bridge_post_disable_next_bridge() possibly.
> >
> >> And then update the core so the relevant function is only called once
> >> for a bridge.
> >> Then the need for DSI lanes in LP-11 can be archived by a call to
> >>
> >>          drm_bridge_pre_enable_prev_bridge()
> >
> > Unfortunately it gets ugly with post_disable.
> > The DSI host controller post_disable will have been called before the
> > DSI peripheral's post_disable, and there are conditions where the
> > peripheral needs to send DSI commands in post_disable (eg
> > panel-asus-z00t-tm5p5-n35596 [1]). Changing all DSI hosts to call
> > drm_bridge_post_disable_next_bridge feels like the wrong thing to do.
> > There are currently hacks in dw-mipi-dsi that do call the next
> > panel/bridge post_disable [2] and it would be nice to get rid of them.
> > Currently the calls aren't tracked for state, so you end up with
> > post_disable being called twice, and panels having to track state (eg
> > jdi_lt070me050000 [3]).
> >
> > [1] tm5p5_nt35596_unprepare() calls tm5p5_nt35596_off()
> > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c#L107
> > [2] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L889
> > [3] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c#L44
> >
> >> This is more explicit than a flag that triggers some magic behaviour.
> >> It may even see uses we have not realised yet.
> >
> > Personally it feels like more boilerplate in almost all DSI drivers,
> > and generally I see a push to remove boilerplate.
> >
> >> It is late here - so maybe the above is not a good idea tomorrow - but
> >> right now I like the simplicity of it.
> >>
> >> Other than the above I read that a mipi_dsi_host_init() is planned,
> >> which is also explicit and simple - good.
> >
> > It's been raised, but the justification for most use cases hasn't been
> > made. The Exynos conversion looks to be doing the wrong thing in
> > checking state, and that's why it is currently needing it.
> > Again it's also more boilerplate.
> >
> > TC358767 is an odd one as it wants the DSI interface enabled very
> > early in order to have a clock for the DP aux channel well before
> > video is running. I had a thought on that, but It looks like I haven't
> > hit send on a reply to Lucas on that one - too many distractions.
> >
> >> Have we seen a new revision of some of these?
> >> Chances are high that I have missed it then.
> >
> > No, still on V2. Other than Dmitry's comment over updating
> > parade-ps8640 and dropping drm_bridge_chain_*, no real comments had
> > been made.
>
> It's been a while now. Do you still plan to pursue this patchset?

If there was anything that could actually be worked on, then I'm happy
to respin it, but if the approach is generally being rejected then I
don't want to waste the effort.

I'm not totally clear who the maintainers are that the final arbiters
and need to sign off on this.
drm_bridge.c falls to Maarten, Maxime, and Thomas for "DRM DRIVERS AND
MISC GPU PATCHES"
drm_panel.c falls to Thierry and Sam for "DRM PANEL DRIVERS", and then
Maarten, Maxime, and Thomas.
Only Sam has responded publicly. I have had discussions with Maxime,
but it's not directly his area of knowledge.

Looking at the patch series:
Patch 1: Your comment "update parade-ps8640 to use
drm_atomic_bridge_chain_". It looks like patchset [1] by Sam does
this, but the patchset went wrong and is missing patches 8-11 and
therefore hasn't been merged.
Patch 2: Comment from Jagan that it's like an old patch. It has
similarities, but isn't the same.
Patch 3: R-b by you (thank you), but concerns from Jagan which I still
don't understand. Without clarification on the issue and whether my
suggested alternative place for the hook solves the issue, IMHO it's
not worth respinning.
Patch 4: R-b Laurent.

This cover note got totally subverted with Exynos issues.
Sam did request use of prev / next instead of upstream / downstream,
which can be done and perhaps warrants a respin now.

> [personal notice: I'd prefer something less strange, e.g. an explicit
> calls to mipi_dsi_host, but as this patchset seems to fix the issues,
> I'm fine with it].

That can fix the power up sequence, but how do you propose telling the
DSI controller NOT to power down in post_disable before the DSI
peripheral post_disable has potentially sent DCS commands - i.e. the
case you were discussing on Friday in [2].

If a panel/bridge driver doesn't call mipi_dsi_host_init then the
expectation must be that it will be called by the DSI controller's
pre_enable, and deinit from post_disable. Likewise init & deinit would
be called if host_transfer is used when the host isn't initialised.

If the panel/bridge driver explicitly calls mipi_dsi_host_init, then
does that mandate that it must also call mipi_dsi_host_deinint. The
controller post_disable is then effectively a no-op. This can be
covered in documentation, but also leaves the potential for strange
behaviour if the requirement is not followed, and I can't think of a
nice place to drop a WARN to flag the issue in the driver.


TBH The lack of interest in solving the issues almost makes me want to
just document the total brokenness of it and throw in the towel.
Seeing as we as Raspberry Pi run a vendor kernel, we can run with
downstream patches until those who care finally make a decision for
mainline. I'd prefer to solve it properly, but it requires some
engagement from the community.

I'll do a respin now second guessing Jagan's comment, and then give it
a month before giving up.

Cheers
  Dave

[1] https://patchwork.freedesktop.org/series/106422/#rev5
[2] https://lists.freedesktop.org/archives/dri-devel/2022-November/379693.html


> >
> >    Dave
>
> --
> With best wishes
> Dmitry
>

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Dmitry Baryshkov]

On 15/11/2022 17:14, Dave Stevenson wrote:
> Hi Dmitry
> 
> On Sun, 13 Nov 2022 at 13:06, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
>>
>> Hi Dave,
>>
>> On 19/07/2022 16:45, Dave Stevenson wrote:
>>> Hi Sam
>>>
>>> On Mon, 18 Jul 2022 at 21:52, Sam Ravnborg <sam@ravnborg.org> wrote:
>>>>
>>>> Hi Dave,
>>>>
>>>> a long overdue reply on this series.
>>>>
>>>> On Fri, Mar 04, 2022 at 03:17:55PM +0000, Dave Stevenson wrote:
>>>>> Hi All
>>>>>
>>>>> Changes from v1:
>>>>> - New patch to refactor drm_bridge_chain_post_disable and drm_bridge_chain_pre_enable
>>>>>     to reuse drm_atomic_bridge_chain_post_disable / drm_atomic_bridge_chain_pre_enable
>>>>>     but with a NULL state.
>>>>> - New patch that adds a pre_enable_upstream_first to drm_panel.
>>>>> - changed from an OPS flag to a bool "pre_enable_upstream_first" in drm_bridge.
>>>>> - Followed Andrzej's suggestion of using continue in the main loop to avoid
>>>>>     needing 2 additional loops (one forward to find the last bridge wanting
>>>>>     upstream first, and the second backwards again).
>>>>> - Actioned Laurent's review comments on docs patch.
>>>>>
>>>>> Original cover letter:
>>>>>
>>>>> Hopefully I've cc'ed all those that have bashed this problem around previously,
>>>>> or are otherwise linked to DRM bridges.
>>>>>
>>>>> There have been numerous discussions around how DSI support is currently broken
>>>>> as it doesn't support initialising the PHY to LP-11 and potentially the clock
>>>>> lane to HS prior to configuring the DSI peripheral. There is no op where the
>>>>> interface is initialised but HS video isn't also being sent.
>>>>> Currently you have:
>>>>> - peripheral pre_enable (host not initialised yet)
>>>>> - host pre_enable
>>>>> - encoder enable
>>>>> - host enable
>>>>> - peripheral enable (video already running)
>>>>>
>>>>> vc4 and exynos currently implement the DSI host as an encoder, and split the
>>>>> bridge_chain. This fails if you want to switch to being a bridge and/or use
>>>>> atomic calls as the state of all the elements split off are not added by
>>>>> drm_atomic_add_encoder_bridges.
>>>>
>>>> A typically chain looks like this:
>>>>
>>>> CRTC => Encoder => Bridge A => Bridge B
>>>>
>>>> We have in DRM bridges established what is the "next" bridge - indicated
>>>> with the direction of the arrows in the drawing.
>>>>
>>>> This set of patches introduces the concept of "upstream" bridges.
>>>>
>>>> pre_enable_prev_bridge_first would be easier to understand as it uses
>>>> the current terminology.
>>>> I get that "upstream" is used in the DSI specification - but we are
>>>> dealing with bridges that happens to support DSI and more, and mixing
>>>> the two terminologies is not good.
>>>>
>>>> Note: Upstream is also used in a bridge doc section - here it should
>>>>         most likely be updated too.
>>>
>>> Sure, I have no issues with switching to prev/next from upstream/downstream.
>>> To the outsider it can be confusing - in pre_enable and disable, the
>>> next bridge to be called is the previous one. At least it is
>>> documented.
>>>
>>>> The current approach set a flag that magically makes the core do something
>>>> else. Have you considered a much more explicit approach?
>>>>
>>>> A few helpers like:
>>>>
>>>>           drm_bridge_pre_enable_prev_bridge()
>>>>           drm_bridge_enable_prev_bridge()
>>>>           drm_bridge_disable_prev_bridge()
>>>>           drm_bridge_post_disable_prev_bridge()
>>>
>>> No point in drm_bridge_enable_prev_bridge() and
>>> drm_bridge_post_disable_prev_bridge() as the call order down the chain
>>> will mean that they have already been called.
>>> drm_bridge_enable_next_bridge() and
>>> drm_bridge_post_disable_next_bridge() possibly.
>>>
>>>> And then update the core so the relevant function is only called once
>>>> for a bridge.
>>>> Then the need for DSI lanes in LP-11 can be archived by a call to
>>>>
>>>>           drm_bridge_pre_enable_prev_bridge()
>>>
>>> Unfortunately it gets ugly with post_disable.
>>> The DSI host controller post_disable will have been called before the
>>> DSI peripheral's post_disable, and there are conditions where the
>>> peripheral needs to send DSI commands in post_disable (eg
>>> panel-asus-z00t-tm5p5-n35596 [1]). Changing all DSI hosts to call
>>> drm_bridge_post_disable_next_bridge feels like the wrong thing to do.
>>> There are currently hacks in dw-mipi-dsi that do call the next
>>> panel/bridge post_disable [2] and it would be nice to get rid of them.
>>> Currently the calls aren't tracked for state, so you end up with
>>> post_disable being called twice, and panels having to track state (eg
>>> jdi_lt070me050000 [3]).
>>>
>>> [1] tm5p5_nt35596_unprepare() calls tm5p5_nt35596_off()
>>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c#L107
>>> [2] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L889
>>> [3] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c#L44
>>>
>>>> This is more explicit than a flag that triggers some magic behaviour.
>>>> It may even see uses we have not realised yet.
>>>
>>> Personally it feels like more boilerplate in almost all DSI drivers,
>>> and generally I see a push to remove boilerplate.
>>>
>>>> It is late here - so maybe the above is not a good idea tomorrow - but
>>>> right now I like the simplicity of it.
>>>>
>>>> Other than the above I read that a mipi_dsi_host_init() is planned,
>>>> which is also explicit and simple - good.
>>>
>>> It's been raised, but the justification for most use cases hasn't been
>>> made. The Exynos conversion looks to be doing the wrong thing in
>>> checking state, and that's why it is currently needing it.
>>> Again it's also more boilerplate.
>>>
>>> TC358767 is an odd one as it wants the DSI interface enabled very
>>> early in order to have a clock for the DP aux channel well before
>>> video is running. I had a thought on that, but It looks like I haven't
>>> hit send on a reply to Lucas on that one - too many distractions.
>>>
>>>> Have we seen a new revision of some of these?
>>>> Chances are high that I have missed it then.
>>>
>>> No, still on V2. Other than Dmitry's comment over updating
>>> parade-ps8640 and dropping drm_bridge_chain_*, no real comments had
>>> been made.
>>
>> It's been a while now. Do you still plan to pursue this patchset?
> 
> If there was anything that could actually be worked on, then I'm happy
> to respin it, but if the approach is generally being rejected then I
> don't want to waste the effort.
> 
> I'm not totally clear who the maintainers are that the final arbiters
> and need to sign off on this.
> drm_bridge.c falls to Maarten, Maxime, and Thomas for "DRM DRIVERS AND
> MISC GPU PATCHES"
> drm_panel.c falls to Thierry and Sam for "DRM PANEL DRIVERS", and then
> Maarten, Maxime, and Thomas.
> Only Sam has responded publicly. I have had discussions with Maxime,
> but it's not directly his area of knowledge.
> 
> Looking at the patch series:
> Patch 1: Your comment "update parade-ps8640 to use
> drm_atomic_bridge_chain_". It looks like patchset [1] by Sam does
> this, but the patchset went wrong and is missing patches 8-11 and
> therefore hasn't been merged.
> Patch 2: Comment from Jagan that it's like an old patch. It has
> similarities, but isn't the same.
> Patch 3: R-b by you (thank you), but concerns from Jagan which I still
> don't understand. Without clarification on the issue and whether my
> suggested alternative place for the hook solves the issue, IMHO it's
> not worth respinning.
> Patch 4: R-b Laurent.
> 
> This cover note got totally subverted with Exynos issues.
> Sam did request use of prev / next instead of upstream / downstream,
> which can be done and perhaps warrants a respin now.
> 
>> [personal notice: I'd prefer something less strange, e.g. an explicit
>> calls to mipi_dsi_host, but as this patchset seems to fix the issues,
>> I'm fine with it].
> 
> That can fix the power up sequence, but how do you propose telling the
> DSI controller NOT to power down in post_disable before the DSI
> peripheral post_disable has potentially sent DCS commands - i.e. the
> case you were discussing on Friday in [2].

I thought that the same 'call the parent beforehand' switch applied to 
the deinit paths, didn't it?

> If a panel/bridge driver doesn't call mipi_dsi_host_init then the
> expectation must be that it will be called by the DSI controller's
> pre_enable, and deinit from post_disable. Likewise init & deinit would
> be called if host_transfer is used when the host isn't initialised.
> 
> If the panel/bridge driver explicitly calls mipi_dsi_host_init, then
> does that mandate that it must also call mipi_dsi_host_deinint. The
> controller post_disable is then effectively a no-op. This can be
> covered in documentation, but also leaves the potential for strange
> behaviour if the requirement is not followed, and I can't think of a
> nice place to drop a WARN to flag the issue in the driver.
> 
> 
> TBH The lack of interest in solving the issues almost makes me want to
> just document the total brokenness of it and throw in the towel.
> Seeing as we as Raspberry Pi run a vendor kernel, we can run with
> downstream patches until those who care finally make a decision for
> mainline. I'd prefer to solve it properly, but it requires some
> engagement from the community.

I see. I can probably try spinning a patchset doing explicit mipi_dsi 
calls. Let's see if it gains more attention.

It seems something is broken with respect to reviewing of core drm 
patches touching strange areas. My patchset improving 
drm_bridge_connector HPD also didn't gain a lot of responses.


> I'll do a respin now second guessing Jagan's comment, and then give it
> a month before giving up
> 
> Cheers
>    Dave
> 
> [1] https://patchwork.freedesktop.org/series/106422/#rev5
> [2] https://lists.freedesktop.org/archives/dri-devel/2022-November/379693.html
> 
> 
>>>
>>>     Dave
>>
>> --
>> With best wishes
>> Dmitry
>>

-- 
With best wishes
Dmitry

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Dave Stevenson]

Hi Dmitry

On Tue, 15 Nov 2022 at 14:21, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 15/11/2022 17:14, Dave Stevenson wrote:
> > Hi Dmitry
> >
> > On Sun, 13 Nov 2022 at 13:06, Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> >>
> >> Hi Dave,
> >>
> >> On 19/07/2022 16:45, Dave Stevenson wrote:
> >>> Hi Sam
> >>>
> >>> On Mon, 18 Jul 2022 at 21:52, Sam Ravnborg <sam@ravnborg.org> wrote:
> >>>>
> >>>> Hi Dave,
> >>>>
> >>>> a long overdue reply on this series.
> >>>>
> >>>> On Fri, Mar 04, 2022 at 03:17:55PM +0000, Dave Stevenson wrote:
> >>>>> Hi All
> >>>>>
> >>>>> Changes from v1:
> >>>>> - New patch to refactor drm_bridge_chain_post_disable and drm_bridge_chain_pre_enable
> >>>>>     to reuse drm_atomic_bridge_chain_post_disable / drm_atomic_bridge_chain_pre_enable
> >>>>>     but with a NULL state.
> >>>>> - New patch that adds a pre_enable_upstream_first to drm_panel.
> >>>>> - changed from an OPS flag to a bool "pre_enable_upstream_first" in drm_bridge.
> >>>>> - Followed Andrzej's suggestion of using continue in the main loop to avoid
> >>>>>     needing 2 additional loops (one forward to find the last bridge wanting
> >>>>>     upstream first, and the second backwards again).
> >>>>> - Actioned Laurent's review comments on docs patch.
> >>>>>
> >>>>> Original cover letter:
> >>>>>
> >>>>> Hopefully I've cc'ed all those that have bashed this problem around previously,
> >>>>> or are otherwise linked to DRM bridges.
> >>>>>
> >>>>> There have been numerous discussions around how DSI support is currently broken
> >>>>> as it doesn't support initialising the PHY to LP-11 and potentially the clock
> >>>>> lane to HS prior to configuring the DSI peripheral. There is no op where the
> >>>>> interface is initialised but HS video isn't also being sent.
> >>>>> Currently you have:
> >>>>> - peripheral pre_enable (host not initialised yet)
> >>>>> - host pre_enable
> >>>>> - encoder enable
> >>>>> - host enable
> >>>>> - peripheral enable (video already running)
> >>>>>
> >>>>> vc4 and exynos currently implement the DSI host as an encoder, and split the
> >>>>> bridge_chain. This fails if you want to switch to being a bridge and/or use
> >>>>> atomic calls as the state of all the elements split off are not added by
> >>>>> drm_atomic_add_encoder_bridges.
> >>>>
> >>>> A typically chain looks like this:
> >>>>
> >>>> CRTC => Encoder => Bridge A => Bridge B
> >>>>
> >>>> We have in DRM bridges established what is the "next" bridge - indicated
> >>>> with the direction of the arrows in the drawing.
> >>>>
> >>>> This set of patches introduces the concept of "upstream" bridges.
> >>>>
> >>>> pre_enable_prev_bridge_first would be easier to understand as it uses
> >>>> the current terminology.
> >>>> I get that "upstream" is used in the DSI specification - but we are
> >>>> dealing with bridges that happens to support DSI and more, and mixing
> >>>> the two terminologies is not good.
> >>>>
> >>>> Note: Upstream is also used in a bridge doc section - here it should
> >>>>         most likely be updated too.
> >>>
> >>> Sure, I have no issues with switching to prev/next from upstream/downstream.
> >>> To the outsider it can be confusing - in pre_enable and disable, the
> >>> next bridge to be called is the previous one. At least it is
> >>> documented.
> >>>
> >>>> The current approach set a flag that magically makes the core do something
> >>>> else. Have you considered a much more explicit approach?
> >>>>
> >>>> A few helpers like:
> >>>>
> >>>>           drm_bridge_pre_enable_prev_bridge()
> >>>>           drm_bridge_enable_prev_bridge()
> >>>>           drm_bridge_disable_prev_bridge()
> >>>>           drm_bridge_post_disable_prev_bridge()
> >>>
> >>> No point in drm_bridge_enable_prev_bridge() and
> >>> drm_bridge_post_disable_prev_bridge() as the call order down the chain
> >>> will mean that they have already been called.
> >>> drm_bridge_enable_next_bridge() and
> >>> drm_bridge_post_disable_next_bridge() possibly.
> >>>
> >>>> And then update the core so the relevant function is only called once
> >>>> for a bridge.
> >>>> Then the need for DSI lanes in LP-11 can be archived by a call to
> >>>>
> >>>>           drm_bridge_pre_enable_prev_bridge()
> >>>
> >>> Unfortunately it gets ugly with post_disable.
> >>> The DSI host controller post_disable will have been called before the
> >>> DSI peripheral's post_disable, and there are conditions where the
> >>> peripheral needs to send DSI commands in post_disable (eg
> >>> panel-asus-z00t-tm5p5-n35596 [1]). Changing all DSI hosts to call
> >>> drm_bridge_post_disable_next_bridge feels like the wrong thing to do.
> >>> There are currently hacks in dw-mipi-dsi that do call the next
> >>> panel/bridge post_disable [2] and it would be nice to get rid of them.
> >>> Currently the calls aren't tracked for state, so you end up with
> >>> post_disable being called twice, and panels having to track state (eg
> >>> jdi_lt070me050000 [3]).
> >>>
> >>> [1] tm5p5_nt35596_unprepare() calls tm5p5_nt35596_off()
> >>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c#L107
> >>> [2] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L889
> >>> [3] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c#L44
> >>>
> >>>> This is more explicit than a flag that triggers some magic behaviour.
> >>>> It may even see uses we have not realised yet.
> >>>
> >>> Personally it feels like more boilerplate in almost all DSI drivers,
> >>> and generally I see a push to remove boilerplate.
> >>>
> >>>> It is late here - so maybe the above is not a good idea tomorrow - but
> >>>> right now I like the simplicity of it.
> >>>>
> >>>> Other than the above I read that a mipi_dsi_host_init() is planned,
> >>>> which is also explicit and simple - good.
> >>>
> >>> It's been raised, but the justification for most use cases hasn't been
> >>> made. The Exynos conversion looks to be doing the wrong thing in
> >>> checking state, and that's why it is currently needing it.
> >>> Again it's also more boilerplate.
> >>>
> >>> TC358767 is an odd one as it wants the DSI interface enabled very
> >>> early in order to have a clock for the DP aux channel well before
> >>> video is running. I had a thought on that, but It looks like I haven't
> >>> hit send on a reply to Lucas on that one - too many distractions.
> >>>
> >>>> Have we seen a new revision of some of these?
> >>>> Chances are high that I have missed it then.
> >>>
> >>> No, still on V2. Other than Dmitry's comment over updating
> >>> parade-ps8640 and dropping drm_bridge_chain_*, no real comments had
> >>> been made.
> >>
> >> It's been a while now. Do you still plan to pursue this patchset?
> >
> > If there was anything that could actually be worked on, then I'm happy
> > to respin it, but if the approach is generally being rejected then I
> > don't want to waste the effort.
> >
> > I'm not totally clear who the maintainers are that the final arbiters
> > and need to sign off on this.
> > drm_bridge.c falls to Maarten, Maxime, and Thomas for "DRM DRIVERS AND
> > MISC GPU PATCHES"
> > drm_panel.c falls to Thierry and Sam for "DRM PANEL DRIVERS", and then
> > Maarten, Maxime, and Thomas.
> > Only Sam has responded publicly. I have had discussions with Maxime,
> > but it's not directly his area of knowledge.
> >
> > Looking at the patch series:
> > Patch 1: Your comment "update parade-ps8640 to use
> > drm_atomic_bridge_chain_". It looks like patchset [1] by Sam does
> > this, but the patchset went wrong and is missing patches 8-11 and
> > therefore hasn't been merged.
> > Patch 2: Comment from Jagan that it's like an old patch. It has
> > similarities, but isn't the same.
> > Patch 3: R-b by you (thank you), but concerns from Jagan which I still
> > don't understand. Without clarification on the issue and whether my
> > suggested alternative place for the hook solves the issue, IMHO it's
> > not worth respinning.
> > Patch 4: R-b Laurent.
> >
> > This cover note got totally subverted with Exynos issues.
> > Sam did request use of prev / next instead of upstream / downstream,
> > which can be done and perhaps warrants a respin now.
> >
> >> [personal notice: I'd prefer something less strange, e.g. an explicit
> >> calls to mipi_dsi_host, but as this patchset seems to fix the issues,
> >> I'm fine with it].
> >
> > That can fix the power up sequence, but how do you propose telling the
> > DSI controller NOT to power down in post_disable before the DSI
> > peripheral post_disable has potentially sent DCS commands - i.e. the
> > case you were discussing on Friday in [2].
>
> I thought that the same 'call the parent beforehand' switch applied to
> the deinit paths, didn't it?

My proposed flag does indeed swap the order of post_disable as well.

Perhaps I was misunderstanding your personal preference.
I was taking it as an explicit mipi_dsi_host call to initialise the
DSI link, which then also needs an explicit mipi_dsi_host call to tear
it down as well. In that situation there is a need to rework the
bridge chain post_disable to allow for the panel post_disable to send
DCS commands before the DSI host is disabled.

> > If a panel/bridge driver doesn't call mipi_dsi_host_init then the
> > expectation must be that it will be called by the DSI controller's
> > pre_enable, and deinit from post_disable. Likewise init & deinit would
> > be called if host_transfer is used when the host isn't initialised.
> >
> > If the panel/bridge driver explicitly calls mipi_dsi_host_init, then
> > does that mandate that it must also call mipi_dsi_host_deinint. The
> > controller post_disable is then effectively a no-op. This can be
> > covered in documentation, but also leaves the potential for strange
> > behaviour if the requirement is not followed, and I can't think of a
> > nice place to drop a WARN to flag the issue in the driver.
> >
> >
> > TBH The lack of interest in solving the issues almost makes me want to
> > just document the total brokenness of it and throw in the towel.
> > Seeing as we as Raspberry Pi run a vendor kernel, we can run with
> > downstream patches until those who care finally make a decision for
> > mainline. I'd prefer to solve it properly, but it requires some
> > engagement from the community.
>
> I see. I can probably try spinning a patchset doing explicit mipi_dsi
> calls. Let's see if it gains more attention.

Is it better to have 2 competing patchsets floating around, or try and
get responses on one first? I'll try and get an updated set out today.

Whilst the main use case now is DSI devices that want the bus powered
up, is it only restricted to DSI or are there potentially other links
that have a similar requirement?

> It seems something is broken with respect to reviewing of core drm
> patches touching strange areas. My patchset improving
> drm_bridge_connector HPD also didn't gain a lot of responses.

I do appreciate that folks are busy, but there does seem to be a
tendency of "it might not be the optimum solution, however I haven't
time to think it through so let's just ignore the problem".
Particularly when it is purely an in-kernel API with no impact on
userspace, I do find that quite frustrating.

  Dave

> > I'll do a respin now second guessing Jagan's comment, and then give it
> > a month before giving up
> >
> > Cheers
> >    Dave
> >
> > [1] https://patchwork.freedesktop.org/series/106422/#rev5
> > [2] https://lists.freedesktop.org/archives/dri-devel/2022-November/379693.html
> >
> >
> >>>
> >>>     Dave
> >>
> >> --
> >> With best wishes
> >> Dmitry
> >>
>
> --
> With best wishes
> Dmitry
>

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Dmitry Baryshkov]

On 15/11/2022 17:38, Dave Stevenson wrote:
> Hi Dmitry
> 
> On Tue, 15 Nov 2022 at 14:21, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
>>
>> On 15/11/2022 17:14, Dave Stevenson wrote:
>>> Hi Dmitry
>>>
>>> On Sun, 13 Nov 2022 at 13:06, Dmitry Baryshkov
>>> <dmitry.baryshkov@linaro.org> wrote:
>>>>
>>>> Hi Dave,
>>>>
>>>> On 19/07/2022 16:45, Dave Stevenson wrote:
>>>>> Hi Sam
>>>>>
>>>>> On Mon, 18 Jul 2022 at 21:52, Sam Ravnborg <sam@ravnborg.org> wrote:
>>>>>>
>>>>>> Hi Dave,
>>>>>>
>>>>>> a long overdue reply on this series.
>>>>>>
>>>>>> On Fri, Mar 04, 2022 at 03:17:55PM +0000, Dave Stevenson wrote:
>>>>>>> Hi All
>>>>>>>
>>>>>>> Changes from v1:
>>>>>>> - New patch to refactor drm_bridge_chain_post_disable and drm_bridge_chain_pre_enable
>>>>>>>      to reuse drm_atomic_bridge_chain_post_disable / drm_atomic_bridge_chain_pre_enable
>>>>>>>      but with a NULL state.
>>>>>>> - New patch that adds a pre_enable_upstream_first to drm_panel.
>>>>>>> - changed from an OPS flag to a bool "pre_enable_upstream_first" in drm_bridge.
>>>>>>> - Followed Andrzej's suggestion of using continue in the main loop to avoid
>>>>>>>      needing 2 additional loops (one forward to find the last bridge wanting
>>>>>>>      upstream first, and the second backwards again).
>>>>>>> - Actioned Laurent's review comments on docs patch.
>>>>>>>
>>>>>>> Original cover letter:
>>>>>>>
>>>>>>> Hopefully I've cc'ed all those that have bashed this problem around previously,
>>>>>>> or are otherwise linked to DRM bridges.
>>>>>>>
>>>>>>> There have been numerous discussions around how DSI support is currently broken
>>>>>>> as it doesn't support initialising the PHY to LP-11 and potentially the clock
>>>>>>> lane to HS prior to configuring the DSI peripheral. There is no op where the
>>>>>>> interface is initialised but HS video isn't also being sent.
>>>>>>> Currently you have:
>>>>>>> - peripheral pre_enable (host not initialised yet)
>>>>>>> - host pre_enable
>>>>>>> - encoder enable
>>>>>>> - host enable
>>>>>>> - peripheral enable (video already running)
>>>>>>>
>>>>>>> vc4 and exynos currently implement the DSI host as an encoder, and split the
>>>>>>> bridge_chain. This fails if you want to switch to being a bridge and/or use
>>>>>>> atomic calls as the state of all the elements split off are not added by
>>>>>>> drm_atomic_add_encoder_bridges.
>>>>>>
>>>>>> A typically chain looks like this:
>>>>>>
>>>>>> CRTC => Encoder => Bridge A => Bridge B
>>>>>>
>>>>>> We have in DRM bridges established what is the "next" bridge - indicated
>>>>>> with the direction of the arrows in the drawing.
>>>>>>
>>>>>> This set of patches introduces the concept of "upstream" bridges.
>>>>>>
>>>>>> pre_enable_prev_bridge_first would be easier to understand as it uses
>>>>>> the current terminology.
>>>>>> I get that "upstream" is used in the DSI specification - but we are
>>>>>> dealing with bridges that happens to support DSI and more, and mixing
>>>>>> the two terminologies is not good.
>>>>>>
>>>>>> Note: Upstream is also used in a bridge doc section - here it should
>>>>>>          most likely be updated too.
>>>>>
>>>>> Sure, I have no issues with switching to prev/next from upstream/downstream.
>>>>> To the outsider it can be confusing - in pre_enable and disable, the
>>>>> next bridge to be called is the previous one. At least it is
>>>>> documented.
>>>>>
>>>>>> The current approach set a flag that magically makes the core do something
>>>>>> else. Have you considered a much more explicit approach?
>>>>>>
>>>>>> A few helpers like:
>>>>>>
>>>>>>            drm_bridge_pre_enable_prev_bridge()
>>>>>>            drm_bridge_enable_prev_bridge()
>>>>>>            drm_bridge_disable_prev_bridge()
>>>>>>            drm_bridge_post_disable_prev_bridge()
>>>>>
>>>>> No point in drm_bridge_enable_prev_bridge() and
>>>>> drm_bridge_post_disable_prev_bridge() as the call order down the chain
>>>>> will mean that they have already been called.
>>>>> drm_bridge_enable_next_bridge() and
>>>>> drm_bridge_post_disable_next_bridge() possibly.
>>>>>
>>>>>> And then update the core so the relevant function is only called once
>>>>>> for a bridge.
>>>>>> Then the need for DSI lanes in LP-11 can be archived by a call to
>>>>>>
>>>>>>            drm_bridge_pre_enable_prev_bridge()
>>>>>
>>>>> Unfortunately it gets ugly with post_disable.
>>>>> The DSI host controller post_disable will have been called before the
>>>>> DSI peripheral's post_disable, and there are conditions where the
>>>>> peripheral needs to send DSI commands in post_disable (eg
>>>>> panel-asus-z00t-tm5p5-n35596 [1]). Changing all DSI hosts to call
>>>>> drm_bridge_post_disable_next_bridge feels like the wrong thing to do.
>>>>> There are currently hacks in dw-mipi-dsi that do call the next
>>>>> panel/bridge post_disable [2] and it would be nice to get rid of them.
>>>>> Currently the calls aren't tracked for state, so you end up with
>>>>> post_disable being called twice, and panels having to track state (eg
>>>>> jdi_lt070me050000 [3]).
>>>>>
>>>>> [1] tm5p5_nt35596_unprepare() calls tm5p5_nt35596_off()
>>>>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c#L107
>>>>> [2] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L889
>>>>> [3] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c#L44
>>>>>
>>>>>> This is more explicit than a flag that triggers some magic behaviour.
>>>>>> It may even see uses we have not realised yet.
>>>>>
>>>>> Personally it feels like more boilerplate in almost all DSI drivers,
>>>>> and generally I see a push to remove boilerplate.
>>>>>
>>>>>> It is late here - so maybe the above is not a good idea tomorrow - but
>>>>>> right now I like the simplicity of it.
>>>>>>
>>>>>> Other than the above I read that a mipi_dsi_host_init() is planned,
>>>>>> which is also explicit and simple - good.
>>>>>
>>>>> It's been raised, but the justification for most use cases hasn't been
>>>>> made. The Exynos conversion looks to be doing the wrong thing in
>>>>> checking state, and that's why it is currently needing it.
>>>>> Again it's also more boilerplate.
>>>>>
>>>>> TC358767 is an odd one as it wants the DSI interface enabled very
>>>>> early in order to have a clock for the DP aux channel well before
>>>>> video is running. I had a thought on that, but It looks like I haven't
>>>>> hit send on a reply to Lucas on that one - too many distractions.
>>>>>
>>>>>> Have we seen a new revision of some of these?
>>>>>> Chances are high that I have missed it then.
>>>>>
>>>>> No, still on V2. Other than Dmitry's comment over updating
>>>>> parade-ps8640 and dropping drm_bridge_chain_*, no real comments had
>>>>> been made.
>>>>
>>>> It's been a while now. Do you still plan to pursue this patchset?
>>>
>>> If there was anything that could actually be worked on, then I'm happy
>>> to respin it, but if the approach is generally being rejected then I
>>> don't want to waste the effort.
>>>
>>> I'm not totally clear who the maintainers are that the final arbiters
>>> and need to sign off on this.
>>> drm_bridge.c falls to Maarten, Maxime, and Thomas for "DRM DRIVERS AND
>>> MISC GPU PATCHES"
>>> drm_panel.c falls to Thierry and Sam for "DRM PANEL DRIVERS", and then
>>> Maarten, Maxime, and Thomas.
>>> Only Sam has responded publicly. I have had discussions with Maxime,
>>> but it's not directly his area of knowledge.
>>>
>>> Looking at the patch series:
>>> Patch 1: Your comment "update parade-ps8640 to use
>>> drm_atomic_bridge_chain_". It looks like patchset [1] by Sam does
>>> this, but the patchset went wrong and is missing patches 8-11 and
>>> therefore hasn't been merged.
>>> Patch 2: Comment from Jagan that it's like an old patch. It has
>>> similarities, but isn't the same.
>>> Patch 3: R-b by you (thank you), but concerns from Jagan which I still
>>> don't understand. Without clarification on the issue and whether my
>>> suggested alternative place for the hook solves the issue, IMHO it's
>>> not worth respinning.
>>> Patch 4: R-b Laurent.
>>>
>>> This cover note got totally subverted with Exynos issues.
>>> Sam did request use of prev / next instead of upstream / downstream,
>>> which can be done and perhaps warrants a respin now.
>>>
>>>> [personal notice: I'd prefer something less strange, e.g. an explicit
>>>> calls to mipi_dsi_host, but as this patchset seems to fix the issues,
>>>> I'm fine with it].
>>>
>>> That can fix the power up sequence, but how do you propose telling the
>>> DSI controller NOT to power down in post_disable before the DSI
>>> peripheral post_disable has potentially sent DCS commands - i.e. the
>>> case you were discussing on Friday in [2].
>>
>> I thought that the same 'call the parent beforehand' switch applied to
>> the deinit paths, didn't it?
> 
> My proposed flag does indeed swap the order of post_disable as well.
> 
> Perhaps I was misunderstanding your personal preference.
> I was taking it as an explicit mipi_dsi_host call to initialise the
> DSI link, which then also needs an explicit mipi_dsi_host call to tear
> it down as well. In that situation there is a need to rework the
> bridge chain post_disable to allow for the panel post_disable to send
> DCS commands before the DSI host is disabled.
> 
>>> If a panel/bridge driver doesn't call mipi_dsi_host_init then the
>>> expectation must be that it will be called by the DSI controller's
>>> pre_enable, and deinit from post_disable. Likewise init & deinit would
>>> be called if host_transfer is used when the host isn't initialised.
>>>
>>> If the panel/bridge driver explicitly calls mipi_dsi_host_init, then
>>> does that mandate that it must also call mipi_dsi_host_deinint. The
>>> controller post_disable is then effectively a no-op. This can be
>>> covered in documentation, but also leaves the potential for strange
>>> behaviour if the requirement is not followed, and I can't think of a
>>> nice place to drop a WARN to flag the issue in the driver.
>>>
>>>
>>> TBH The lack of interest in solving the issues almost makes me want to
>>> just document the total brokenness of it and throw in the towel.
>>> Seeing as we as Raspberry Pi run a vendor kernel, we can run with
>>> downstream patches until those who care finally make a decision for
>>> mainline. I'd prefer to solve it properly, but it requires some
>>> engagement from the community.
>>
>> I see. I can probably try spinning a patchset doing explicit mipi_dsi
>> calls. Let's see if it gains more attention.
> 
> Is it better to have 2 competing patchsets floating around, or try and
> get responses on one first? I'll try and get an updated set out today.

I'm a bit in a tough position here. I can't say that I like this 
approach, but it seems to fix all the issues that we have with DSI 
hosts, so it's better than the current state.

> Whilst the main use case now is DSI devices that want the bus powered
> up, is it only restricted to DSI or are there potentially other links
> that have a similar requirement?

eDP has even more strange requirements: several panels have strict 
requirement to power it off completely before being able to power it up 
again (see all the patches from Douglas Anderson to get AUX bus to work 
properly). And powering up a bus requires waiting for the HPD pin.

In the eDP case powering up the bus was solved using autosuspend on the 
panel and on the host sides. However I don't remember whether eDP has 
anything comparable to DSI's LP state

> 
>> It seems something is broken with respect to reviewing of core drm
>> patches touching strange areas. My patchset improving
>> drm_bridge_connector HPD also didn't gain a lot of responses.
> 
> I do appreciate that folks are busy, but there does seem to be a
> tendency of "it might not be the optimum solution, however I haven't
> time to think it through so let's just ignore the problem".
> Particularly when it is purely an in-kernel API with no impact on
> userspace, I do find that quite frustrating.

Fully agree.

> 
>    Dave
> 
>>> I'll do a respin now second guessing Jagan's comment, and then give it
>>> a month before giving up
>>>
>>> Cheers
>>>     Dave
>>>
>>> [1] https://patchwork.freedesktop.org/series/106422/#rev5
>>> [2] https://lists.freedesktop.org/archives/dri-devel/2022-November/379693.html


-- 
With best wishes
Dmitry

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Maxime Ripard]

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

On Thu, Nov 17, 2022 at 03:24:07PM +0200, Dmitry Baryshkov wrote:
> On 15/11/2022 17:38, Dave Stevenson wrote:
> > Hi Dmitry
> > 
> > On Tue, 15 Nov 2022 at 14:21, Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > > 
> > > On 15/11/2022 17:14, Dave Stevenson wrote:
> > > > Hi Dmitry
> > > > 
> > > > On Sun, 13 Nov 2022 at 13:06, Dmitry Baryshkov
> > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > > 
> > > > > Hi Dave,
> > > > > 
> > > > > On 19/07/2022 16:45, Dave Stevenson wrote:
> > > > > > Hi Sam
> > > > > > 
> > > > > > On Mon, 18 Jul 2022 at 21:52, Sam Ravnborg <sam@ravnborg.org> wrote:
> > > > > > > 
> > > > > > > Hi Dave,
> > > > > > > 
> > > > > > > a long overdue reply on this series.
> > > > > > > 
> > > > > > > On Fri, Mar 04, 2022 at 03:17:55PM +0000, Dave Stevenson wrote:
> > > > > > > > Hi All
> > > > > > > > 
> > > > > > > > Changes from v1:
> > > > > > > > - New patch to refactor drm_bridge_chain_post_disable and drm_bridge_chain_pre_enable
> > > > > > > >      to reuse drm_atomic_bridge_chain_post_disable / drm_atomic_bridge_chain_pre_enable
> > > > > > > >      but with a NULL state.
> > > > > > > > - New patch that adds a pre_enable_upstream_first to drm_panel.
> > > > > > > > - changed from an OPS flag to a bool "pre_enable_upstream_first" in drm_bridge.
> > > > > > > > - Followed Andrzej's suggestion of using continue in the main loop to avoid
> > > > > > > >      needing 2 additional loops (one forward to find the last bridge wanting
> > > > > > > >      upstream first, and the second backwards again).
> > > > > > > > - Actioned Laurent's review comments on docs patch.
> > > > > > > > 
> > > > > > > > Original cover letter:
> > > > > > > > 
> > > > > > > > Hopefully I've cc'ed all those that have bashed this problem around previously,
> > > > > > > > or are otherwise linked to DRM bridges.
> > > > > > > > 
> > > > > > > > There have been numerous discussions around how DSI support is currently broken
> > > > > > > > as it doesn't support initialising the PHY to LP-11 and potentially the clock
> > > > > > > > lane to HS prior to configuring the DSI peripheral. There is no op where the
> > > > > > > > interface is initialised but HS video isn't also being sent.
> > > > > > > > Currently you have:
> > > > > > > > - peripheral pre_enable (host not initialised yet)
> > > > > > > > - host pre_enable
> > > > > > > > - encoder enable
> > > > > > > > - host enable
> > > > > > > > - peripheral enable (video already running)
> > > > > > > > 
> > > > > > > > vc4 and exynos currently implement the DSI host as an encoder, and split the
> > > > > > > > bridge_chain. This fails if you want to switch to being a bridge and/or use
> > > > > > > > atomic calls as the state of all the elements split off are not added by
> > > > > > > > drm_atomic_add_encoder_bridges.
> > > > > > > 
> > > > > > > A typically chain looks like this:
> > > > > > > 
> > > > > > > CRTC => Encoder => Bridge A => Bridge B
> > > > > > > 
> > > > > > > We have in DRM bridges established what is the "next" bridge - indicated
> > > > > > > with the direction of the arrows in the drawing.
> > > > > > > 
> > > > > > > This set of patches introduces the concept of "upstream" bridges.
> > > > > > > 
> > > > > > > pre_enable_prev_bridge_first would be easier to understand as it uses
> > > > > > > the current terminology.
> > > > > > > I get that "upstream" is used in the DSI specification - but we are
> > > > > > > dealing with bridges that happens to support DSI and more, and mixing
> > > > > > > the two terminologies is not good.
> > > > > > > 
> > > > > > > Note: Upstream is also used in a bridge doc section - here it should
> > > > > > >          most likely be updated too.
> > > > > > 
> > > > > > Sure, I have no issues with switching to prev/next from upstream/downstream.
> > > > > > To the outsider it can be confusing - in pre_enable and disable, the
> > > > > > next bridge to be called is the previous one. At least it is
> > > > > > documented.
> > > > > > 
> > > > > > > The current approach set a flag that magically makes the core do something
> > > > > > > else. Have you considered a much more explicit approach?
> > > > > > > 
> > > > > > > A few helpers like:
> > > > > > > 
> > > > > > >            drm_bridge_pre_enable_prev_bridge()
> > > > > > >            drm_bridge_enable_prev_bridge()
> > > > > > >            drm_bridge_disable_prev_bridge()
> > > > > > >            drm_bridge_post_disable_prev_bridge()
> > > > > > 
> > > > > > No point in drm_bridge_enable_prev_bridge() and
> > > > > > drm_bridge_post_disable_prev_bridge() as the call order down the chain
> > > > > > will mean that they have already been called.
> > > > > > drm_bridge_enable_next_bridge() and
> > > > > > drm_bridge_post_disable_next_bridge() possibly.
> > > > > > 
> > > > > > > And then update the core so the relevant function is only called once
> > > > > > > for a bridge.
> > > > > > > Then the need for DSI lanes in LP-11 can be archived by a call to
> > > > > > > 
> > > > > > >            drm_bridge_pre_enable_prev_bridge()
> > > > > > 
> > > > > > Unfortunately it gets ugly with post_disable.
> > > > > > The DSI host controller post_disable will have been called before the
> > > > > > DSI peripheral's post_disable, and there are conditions where the
> > > > > > peripheral needs to send DSI commands in post_disable (eg
> > > > > > panel-asus-z00t-tm5p5-n35596 [1]). Changing all DSI hosts to call
> > > > > > drm_bridge_post_disable_next_bridge feels like the wrong thing to do.
> > > > > > There are currently hacks in dw-mipi-dsi that do call the next
> > > > > > panel/bridge post_disable [2] and it would be nice to get rid of them.
> > > > > > Currently the calls aren't tracked for state, so you end up with
> > > > > > post_disable being called twice, and panels having to track state (eg
> > > > > > jdi_lt070me050000 [3]).
> > > > > > 
> > > > > > [1] tm5p5_nt35596_unprepare() calls tm5p5_nt35596_off()
> > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c#L107
> > > > > > [2] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L889
> > > > > > [3] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c#L44
> > > > > > 
> > > > > > > This is more explicit than a flag that triggers some magic behaviour.
> > > > > > > It may even see uses we have not realised yet.
> > > > > > 
> > > > > > Personally it feels like more boilerplate in almost all DSI drivers,
> > > > > > and generally I see a push to remove boilerplate.
> > > > > > 
> > > > > > > It is late here - so maybe the above is not a good idea tomorrow - but
> > > > > > > right now I like the simplicity of it.
> > > > > > > 
> > > > > > > Other than the above I read that a mipi_dsi_host_init() is planned,
> > > > > > > which is also explicit and simple - good.
> > > > > > 
> > > > > > It's been raised, but the justification for most use cases hasn't been
> > > > > > made. The Exynos conversion looks to be doing the wrong thing in
> > > > > > checking state, and that's why it is currently needing it.
> > > > > > Again it's also more boilerplate.
> > > > > > 
> > > > > > TC358767 is an odd one as it wants the DSI interface enabled very
> > > > > > early in order to have a clock for the DP aux channel well before
> > > > > > video is running. I had a thought on that, but It looks like I haven't
> > > > > > hit send on a reply to Lucas on that one - too many distractions.
> > > > > > 
> > > > > > > Have we seen a new revision of some of these?
> > > > > > > Chances are high that I have missed it then.
> > > > > > 
> > > > > > No, still on V2. Other than Dmitry's comment over updating
> > > > > > parade-ps8640 and dropping drm_bridge_chain_*, no real comments had
> > > > > > been made.
> > > > > 
> > > > > It's been a while now. Do you still plan to pursue this patchset?
> > > > 
> > > > If there was anything that could actually be worked on, then I'm happy
> > > > to respin it, but if the approach is generally being rejected then I
> > > > don't want to waste the effort.
> > > > 
> > > > I'm not totally clear who the maintainers are that the final arbiters
> > > > and need to sign off on this.
> > > > drm_bridge.c falls to Maarten, Maxime, and Thomas for "DRM DRIVERS AND
> > > > MISC GPU PATCHES"
> > > > drm_panel.c falls to Thierry and Sam for "DRM PANEL DRIVERS", and then
> > > > Maarten, Maxime, and Thomas.
> > > > Only Sam has responded publicly. I have had discussions with Maxime,
> > > > but it's not directly his area of knowledge.
> > > > 
> > > > Looking at the patch series:
> > > > Patch 1: Your comment "update parade-ps8640 to use
> > > > drm_atomic_bridge_chain_". It looks like patchset [1] by Sam does
> > > > this, but the patchset went wrong and is missing patches 8-11 and
> > > > therefore hasn't been merged.
> > > > Patch 2: Comment from Jagan that it's like an old patch. It has
> > > > similarities, but isn't the same.
> > > > Patch 3: R-b by you (thank you), but concerns from Jagan which I still
> > > > don't understand. Without clarification on the issue and whether my
> > > > suggested alternative place for the hook solves the issue, IMHO it's
> > > > not worth respinning.
> > > > Patch 4: R-b Laurent.
> > > > 
> > > > This cover note got totally subverted with Exynos issues.
> > > > Sam did request use of prev / next instead of upstream / downstream,
> > > > which can be done and perhaps warrants a respin now.
> > > > 
> > > > > [personal notice: I'd prefer something less strange, e.g. an explicit
> > > > > calls to mipi_dsi_host, but as this patchset seems to fix the issues,
> > > > > I'm fine with it].
> > > > 
> > > > That can fix the power up sequence, but how do you propose telling the
> > > > DSI controller NOT to power down in post_disable before the DSI
> > > > peripheral post_disable has potentially sent DCS commands - i.e. the
> > > > case you were discussing on Friday in [2].
> > > 
> > > I thought that the same 'call the parent beforehand' switch applied to
> > > the deinit paths, didn't it?
> > 
> > My proposed flag does indeed swap the order of post_disable as well.
> > 
> > Perhaps I was misunderstanding your personal preference.
> > I was taking it as an explicit mipi_dsi_host call to initialise the
> > DSI link, which then also needs an explicit mipi_dsi_host call to tear
> > it down as well. In that situation there is a need to rework the
> > bridge chain post_disable to allow for the panel post_disable to send
> > DCS commands before the DSI host is disabled.
> > 
> > > > If a panel/bridge driver doesn't call mipi_dsi_host_init then the
> > > > expectation must be that it will be called by the DSI controller's
> > > > pre_enable, and deinit from post_disable. Likewise init & deinit would
> > > > be called if host_transfer is used when the host isn't initialised.
> > > > 
> > > > If the panel/bridge driver explicitly calls mipi_dsi_host_init, then
> > > > does that mandate that it must also call mipi_dsi_host_deinint. The
> > > > controller post_disable is then effectively a no-op. This can be
> > > > covered in documentation, but also leaves the potential for strange
> > > > behaviour if the requirement is not followed, and I can't think of a
> > > > nice place to drop a WARN to flag the issue in the driver.
> > > > 
> > > > 
> > > > TBH The lack of interest in solving the issues almost makes me want to
> > > > just document the total brokenness of it and throw in the towel.
> > > > Seeing as we as Raspberry Pi run a vendor kernel, we can run with
> > > > downstream patches until those who care finally make a decision for
> > > > mainline. I'd prefer to solve it properly, but it requires some
> > > > engagement from the community.
> > > 
> > > I see. I can probably try spinning a patchset doing explicit mipi_dsi
> > > calls. Let's see if it gains more attention.
> > 
> > Is it better to have 2 competing patchsets floating around, or try and
> > get responses on one first? I'll try and get an updated set out today.
> 
> I'm a bit in a tough position here. I can't say that I like this approach,
> but it seems to fix all the issues that we have with DSI hosts, so it's
> better than the current state.

I'd say the bridge support in general is under-maintained. Historically,
Boris and Laurent did most of the architecture work, but are either
completely drowning under patches or have moved on.

I can't really speak for Thomas and Maarten, but I don't really have
such a good knowledge about the bridge infrastructure and haven't been
very involved. I have the same impression from Maarten and Thomas
though.

Which means that it's pretty much a blindspot for us :)

I'm sorry if it's taking a while, but I'd say that if you two have a
good comprehension of the issue (and I know Dave has), if you can reach
a reasonable solution for both of you, and if there is proper
documentation for the new work, I'd consider this a net improvement.

And as far as I know from that discussion, we're pretty much there
already. So yeah, go ahead with a new version and we'll merge it.

Maxime

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

Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Dmitry Baryshkov]

On 17/11/2022 17:34, Maxime Ripard wrote:
> On Thu, Nov 17, 2022 at 03:24:07PM +0200, Dmitry Baryshkov wrote:
>> On 15/11/2022 17:38, Dave Stevenson wrote:
>>> Hi Dmitry
>>>
>>> On Tue, 15 Nov 2022 at 14:21, Dmitry Baryshkov
>>> <dmitry.baryshkov@linaro.org> wrote:
>>>>
>>>> On 15/11/2022 17:14, Dave Stevenson wrote:
>>>>> Hi Dmitry
>>>>>
>>>>> On Sun, 13 Nov 2022 at 13:06, Dmitry Baryshkov
>>>>> <dmitry.baryshkov@linaro.org> wrote:
>>>>>>
>>>>>> Hi Dave,
>>>>>>
>>>>>> On 19/07/2022 16:45, Dave Stevenson wrote:
>>>>>>> Hi Sam
>>>>>>>
>>>>>>> On Mon, 18 Jul 2022 at 21:52, Sam Ravnborg <sam@ravnborg.org> wrote:
>>>>>>>>
>>>>>>>> Hi Dave,
>>>>>>>>
>>>>>>>> a long overdue reply on this series.
>>>>>>>>
>>>>>>>> On Fri, Mar 04, 2022 at 03:17:55PM +0000, Dave Stevenson wrote:
>>>>>>>>> Hi All
>>>>>>>>>
>>>>>>>>> Changes from v1:
>>>>>>>>> - New patch to refactor drm_bridge_chain_post_disable and drm_bridge_chain_pre_enable
>>>>>>>>>       to reuse drm_atomic_bridge_chain_post_disable / drm_atomic_bridge_chain_pre_enable
>>>>>>>>>       but with a NULL state.
>>>>>>>>> - New patch that adds a pre_enable_upstream_first to drm_panel.
>>>>>>>>> - changed from an OPS flag to a bool "pre_enable_upstream_first" in drm_bridge.
>>>>>>>>> - Followed Andrzej's suggestion of using continue in the main loop to avoid
>>>>>>>>>       needing 2 additional loops (one forward to find the last bridge wanting
>>>>>>>>>       upstream first, and the second backwards again).
>>>>>>>>> - Actioned Laurent's review comments on docs patch.
>>>>>>>>>
>>>>>>>>> Original cover letter:
>>>>>>>>>
>>>>>>>>> Hopefully I've cc'ed all those that have bashed this problem around previously,
>>>>>>>>> or are otherwise linked to DRM bridges.
>>>>>>>>>
>>>>>>>>> There have been numerous discussions around how DSI support is currently broken
>>>>>>>>> as it doesn't support initialising the PHY to LP-11 and potentially the clock
>>>>>>>>> lane to HS prior to configuring the DSI peripheral. There is no op where the
>>>>>>>>> interface is initialised but HS video isn't also being sent.
>>>>>>>>> Currently you have:
>>>>>>>>> - peripheral pre_enable (host not initialised yet)
>>>>>>>>> - host pre_enable
>>>>>>>>> - encoder enable
>>>>>>>>> - host enable
>>>>>>>>> - peripheral enable (video already running)
>>>>>>>>>
>>>>>>>>> vc4 and exynos currently implement the DSI host as an encoder, and split the
>>>>>>>>> bridge_chain. This fails if you want to switch to being a bridge and/or use
>>>>>>>>> atomic calls as the state of all the elements split off are not added by
>>>>>>>>> drm_atomic_add_encoder_bridges.
>>>>>>>>
>>>>>>>> A typically chain looks like this:
>>>>>>>>
>>>>>>>> CRTC => Encoder => Bridge A => Bridge B
>>>>>>>>
>>>>>>>> We have in DRM bridges established what is the "next" bridge - indicated
>>>>>>>> with the direction of the arrows in the drawing.
>>>>>>>>
>>>>>>>> This set of patches introduces the concept of "upstream" bridges.
>>>>>>>>
>>>>>>>> pre_enable_prev_bridge_first would be easier to understand as it uses
>>>>>>>> the current terminology.
>>>>>>>> I get that "upstream" is used in the DSI specification - but we are
>>>>>>>> dealing with bridges that happens to support DSI and more, and mixing
>>>>>>>> the two terminologies is not good.
>>>>>>>>
>>>>>>>> Note: Upstream is also used in a bridge doc section - here it should
>>>>>>>>           most likely be updated too.
>>>>>>>
>>>>>>> Sure, I have no issues with switching to prev/next from upstream/downstream.
>>>>>>> To the outsider it can be confusing - in pre_enable and disable, the
>>>>>>> next bridge to be called is the previous one. At least it is
>>>>>>> documented.
>>>>>>>
>>>>>>>> The current approach set a flag that magically makes the core do something
>>>>>>>> else. Have you considered a much more explicit approach?
>>>>>>>>
>>>>>>>> A few helpers like:
>>>>>>>>
>>>>>>>>             drm_bridge_pre_enable_prev_bridge()
>>>>>>>>             drm_bridge_enable_prev_bridge()
>>>>>>>>             drm_bridge_disable_prev_bridge()
>>>>>>>>             drm_bridge_post_disable_prev_bridge()
>>>>>>>
>>>>>>> No point in drm_bridge_enable_prev_bridge() and
>>>>>>> drm_bridge_post_disable_prev_bridge() as the call order down the chain
>>>>>>> will mean that they have already been called.
>>>>>>> drm_bridge_enable_next_bridge() and
>>>>>>> drm_bridge_post_disable_next_bridge() possibly.
>>>>>>>
>>>>>>>> And then update the core so the relevant function is only called once
>>>>>>>> for a bridge.
>>>>>>>> Then the need for DSI lanes in LP-11 can be archived by a call to
>>>>>>>>
>>>>>>>>             drm_bridge_pre_enable_prev_bridge()
>>>>>>>
>>>>>>> Unfortunately it gets ugly with post_disable.
>>>>>>> The DSI host controller post_disable will have been called before the
>>>>>>> DSI peripheral's post_disable, and there are conditions where the
>>>>>>> peripheral needs to send DSI commands in post_disable (eg
>>>>>>> panel-asus-z00t-tm5p5-n35596 [1]). Changing all DSI hosts to call
>>>>>>> drm_bridge_post_disable_next_bridge feels like the wrong thing to do.
>>>>>>> There are currently hacks in dw-mipi-dsi that do call the next
>>>>>>> panel/bridge post_disable [2] and it would be nice to get rid of them.
>>>>>>> Currently the calls aren't tracked for state, so you end up with
>>>>>>> post_disable being called twice, and panels having to track state (eg
>>>>>>> jdi_lt070me050000 [3]).
>>>>>>>
>>>>>>> [1] tm5p5_nt35596_unprepare() calls tm5p5_nt35596_off()
>>>>>>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c#L107
>>>>>>> [2] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L889
>>>>>>> [3] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c#L44
>>>>>>>
>>>>>>>> This is more explicit than a flag that triggers some magic behaviour.
>>>>>>>> It may even see uses we have not realised yet.
>>>>>>>
>>>>>>> Personally it feels like more boilerplate in almost all DSI drivers,
>>>>>>> and generally I see a push to remove boilerplate.
>>>>>>>
>>>>>>>> It is late here - so maybe the above is not a good idea tomorrow - but
>>>>>>>> right now I like the simplicity of it.
>>>>>>>>
>>>>>>>> Other than the above I read that a mipi_dsi_host_init() is planned,
>>>>>>>> which is also explicit and simple - good.
>>>>>>>
>>>>>>> It's been raised, but the justification for most use cases hasn't been
>>>>>>> made. The Exynos conversion looks to be doing the wrong thing in
>>>>>>> checking state, and that's why it is currently needing it.
>>>>>>> Again it's also more boilerplate.
>>>>>>>
>>>>>>> TC358767 is an odd one as it wants the DSI interface enabled very
>>>>>>> early in order to have a clock for the DP aux channel well before
>>>>>>> video is running. I had a thought on that, but It looks like I haven't
>>>>>>> hit send on a reply to Lucas on that one - too many distractions.
>>>>>>>
>>>>>>>> Have we seen a new revision of some of these?
>>>>>>>> Chances are high that I have missed it then.
>>>>>>>
>>>>>>> No, still on V2. Other than Dmitry's comment over updating
>>>>>>> parade-ps8640 and dropping drm_bridge_chain_*, no real comments had
>>>>>>> been made.
>>>>>>
>>>>>> It's been a while now. Do you still plan to pursue this patchset?
>>>>>
>>>>> If there was anything that could actually be worked on, then I'm happy
>>>>> to respin it, but if the approach is generally being rejected then I
>>>>> don't want to waste the effort.
>>>>>
>>>>> I'm not totally clear who the maintainers are that the final arbiters
>>>>> and need to sign off on this.
>>>>> drm_bridge.c falls to Maarten, Maxime, and Thomas for "DRM DRIVERS AND
>>>>> MISC GPU PATCHES"
>>>>> drm_panel.c falls to Thierry and Sam for "DRM PANEL DRIVERS", and then
>>>>> Maarten, Maxime, and Thomas.
>>>>> Only Sam has responded publicly. I have had discussions with Maxime,
>>>>> but it's not directly his area of knowledge.
>>>>>
>>>>> Looking at the patch series:
>>>>> Patch 1: Your comment "update parade-ps8640 to use
>>>>> drm_atomic_bridge_chain_". It looks like patchset [1] by Sam does
>>>>> this, but the patchset went wrong and is missing patches 8-11 and
>>>>> therefore hasn't been merged.
>>>>> Patch 2: Comment from Jagan that it's like an old patch. It has
>>>>> similarities, but isn't the same.
>>>>> Patch 3: R-b by you (thank you), but concerns from Jagan which I still
>>>>> don't understand. Without clarification on the issue and whether my
>>>>> suggested alternative place for the hook solves the issue, IMHO it's
>>>>> not worth respinning.
>>>>> Patch 4: R-b Laurent.
>>>>>
>>>>> This cover note got totally subverted with Exynos issues.
>>>>> Sam did request use of prev / next instead of upstream / downstream,
>>>>> which can be done and perhaps warrants a respin now.
>>>>>
>>>>>> [personal notice: I'd prefer something less strange, e.g. an explicit
>>>>>> calls to mipi_dsi_host, but as this patchset seems to fix the issues,
>>>>>> I'm fine with it].
>>>>>
>>>>> That can fix the power up sequence, but how do you propose telling the
>>>>> DSI controller NOT to power down in post_disable before the DSI
>>>>> peripheral post_disable has potentially sent DCS commands - i.e. the
>>>>> case you were discussing on Friday in [2].
>>>>
>>>> I thought that the same 'call the parent beforehand' switch applied to
>>>> the deinit paths, didn't it?
>>>
>>> My proposed flag does indeed swap the order of post_disable as well.
>>>
>>> Perhaps I was misunderstanding your personal preference.
>>> I was taking it as an explicit mipi_dsi_host call to initialise the
>>> DSI link, which then also needs an explicit mipi_dsi_host call to tear
>>> it down as well. In that situation there is a need to rework the
>>> bridge chain post_disable to allow for the panel post_disable to send
>>> DCS commands before the DSI host is disabled.
>>>
>>>>> If a panel/bridge driver doesn't call mipi_dsi_host_init then the
>>>>> expectation must be that it will be called by the DSI controller's
>>>>> pre_enable, and deinit from post_disable. Likewise init & deinit would
>>>>> be called if host_transfer is used when the host isn't initialised.
>>>>>
>>>>> If the panel/bridge driver explicitly calls mipi_dsi_host_init, then
>>>>> does that mandate that it must also call mipi_dsi_host_deinint. The
>>>>> controller post_disable is then effectively a no-op. This can be
>>>>> covered in documentation, but also leaves the potential for strange
>>>>> behaviour if the requirement is not followed, and I can't think of a
>>>>> nice place to drop a WARN to flag the issue in the driver.
>>>>>
>>>>>
>>>>> TBH The lack of interest in solving the issues almost makes me want to
>>>>> just document the total brokenness of it and throw in the towel.
>>>>> Seeing as we as Raspberry Pi run a vendor kernel, we can run with
>>>>> downstream patches until those who care finally make a decision for
>>>>> mainline. I'd prefer to solve it properly, but it requires some
>>>>> engagement from the community.
>>>>
>>>> I see. I can probably try spinning a patchset doing explicit mipi_dsi
>>>> calls. Let's see if it gains more attention.
>>>
>>> Is it better to have 2 competing patchsets floating around, or try and
>>> get responses on one first? I'll try and get an updated set out today.
>>
>> I'm a bit in a tough position here. I can't say that I like this approach,
>> but it seems to fix all the issues that we have with DSI hosts, so it's
>> better than the current state.
> 
> I'd say the bridge support in general is under-maintained. Historically,
> Boris and Laurent did most of the architecture work, but are either
> completely drowning under patches or have moved on.
> 
> I can't really speak for Thomas and Maarten, but I don't really have
> such a good knowledge about the bridge infrastructure and haven't been
> very involved. I have the same impression from Maarten and Thomas
> though.
> 
> Which means that it's pretty much a blindspot for us :)
> 
> I'm sorry if it's taking a while, but I'd say that if you two have a
> good comprehension of the issue (and I know Dave has), if you can reach
> a reasonable solution for both of you, and if there is proper
> documentation for the new work, I'd consider this a net improvement.
> 
> And as far as I know from that discussion, we're pretty much there
> already. So yeah, go ahead with a new version and we'll merge it.

Ack, then I'll hold on my proposal to let Dave's version to be merged.

-- 
With best wishes
Dmitry