[PATCH v7 10/12] drm/bridge: Implement enable_next_first to alter bridge init order

[PATCH v7 10/12] drm/bridge: Implement enable_next_first to alter bridge init order

[Jagan Teki]

DSI sink devices typically send the MIPI-DCS commands to the DSI host
via general MIPI_DSI_DCS read and write API.

The classical DSI sequence mentioned that the DSI host receives MIPI-DCS
commands from the DSI sink first in order to switch HS mode properly.
Once the DSI host switches to HS mode any MIPI-DCS commands from the
DSI sink are unfunctional.

DSI sink uses the @enable function to send the MIPI-DCS commands. In a
typical DSI host, sink pipeline the @enable call chain start with the
DSI host, and then the DSI sink which is the "wrong" order as DSI host
@enable is called and switched to HS mode before DSI sink @enable.

If the DSI host enables with the @enable_next_first flag then the
@enable for the DSI sink will be called first before the @enable of
the DSI host. This alter bridge init order makes sure that the MIPI-DCS
commands send first and then switch to the HS mode properly by DSI host.

This new flag @enable_next_first that any bridg can set to swap the
order of @enable (and #disable) for that and the immediately next bridge.

[enable]
If a bridge sets @enable_next_first, then the @enable for the next bridge
will be called first before enable of this bridge.

[disable]
If a bridge sets @enable_next_first, then the @disable for the next bridge
will be called first before @disable of this bridge to reverse the @enable
calling direction.

eg:
- Panel
- Bridge 1
- Bridge 2 enable_next_first
- Bridge 3
- Bridge 4 enable_next_first
- Bridge 5 enable_next_first
- Bridge 6
- Encoder

Would result in enable's being called as Encoder, Bridge 6, Bridge 3,
Bridge 4, Bridge 5, Bridge 1, Bridge 2, Panel.

and the result in disable's being called as Panel, Bridge 2, Bridge 1,
Bridge 5, Bridge 4, Bridge 3, Bridge 6, Encoder.

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

 drivers/gpu/drm/drm_bridge.c | 171 ++++++++++++++++++++++++++++++-----
 include/drm/drm_bridge.h     |   8 ++
 2 files changed, 154 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index caf0f341e524..cdc2669b3512 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -577,6 +577,24 @@ void drm_bridge_chain_mode_set(struct drm_bridge *bridge,
 }
 EXPORT_SYMBOL(drm_bridge_chain_mode_set);
 
+static void drm_atomic_bridge_call_disable(struct drm_bridge *bridge,
+					   struct drm_atomic_state *old_state)
+{
+	if (old_state && bridge->funcs->atomic_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_disable(bridge, old_bridge_state);
+	} else if (bridge->funcs->disable) {
+		bridge->funcs->disable(bridge);
+	}
+}
+
 /**
  * drm_atomic_bridge_chain_disable - disables all bridges in the encoder chain
  * @bridge: bridge control structure
@@ -587,33 +605,73 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set);
  * starting from the last bridge to the first. These are called before calling
  * &drm_encoder_helper_funcs.atomic_disable
  *
+ * If a bridge sets @enable_next_first, then the @disable for the next bridge
+ * will be called first before @disable of this bridge to reverse the @enable
+ * calling direction.
+ *
+ * Example:
+ * Bridge A ---> Bridge B ---> Bridge C ---> Bridge D ---> Bridge E
+ *
+ * With enable_next_first flag enable in Bridge A, C, D then the resulting
+ * @disable order would be,
+ * Bridge C, Bridge D, Bridge E, Bridge A, Bridge B.
+ *
  * Note: the bridge passed should be the one closest to the encoder
  */
 void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge,
 				     struct drm_atomic_state *old_state)
 {
 	struct drm_encoder *encoder;
-	struct drm_bridge *iter;
+	struct drm_bridge *iter, *next, *limit;
 
 	if (!bridge)
 		return;
 
 	encoder = bridge->encoder;
+
 	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
-		if (iter->funcs->atomic_disable) {
-			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;
-
-			iter->funcs->atomic_disable(iter, old_bridge_state);
-		} else if (iter->funcs->disable) {
-			iter->funcs->disable(iter);
+		limit = NULL;
+
+		if (!list_is_first(&iter->chain_node, &encoder->bridge_chain)) {
+			next = list_prev_entry(iter, chain_node);
+
+			if (next->enable_next_first) {
+				limit = bridge;
+				list_for_each_entry_from_reverse(next,
+							 &encoder->bridge_chain,
+							 chain_node) {
+					if (next == bridge)
+						break;
+
+					if (!next->enable_next_first) {
+						/* Found first bridge that does NOT
+						 * request next to be enabled first
+						 */
+						next = list_next_entry(next, chain_node);
+						limit = next;
+						break;
+					}
+				}
+
+				list_for_each_entry_from(next, &encoder->bridge_chain, chain_node) {
+					/* Call requested next bridge enable in order */
+					if (next == iter)
+						/* At the first bridge to request next
+						 * bridges called first.
+						 */
+						break;
+
+					drm_atomic_bridge_call_disable(next, old_state);
+				}
+			}
 		}
 
+		drm_atomic_bridge_call_disable(iter, old_state);
+
+		if (limit)
+			/* Jump all bridges that we have already disabled */
+			iter = limit;
+
 		if (iter == bridge)
 			break;
 	}
@@ -822,6 +880,24 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
 }
 EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable);
 
+static void drm_atomic_bridge_call_enable(struct drm_bridge *bridge,
+					  struct drm_atomic_state *old_state)
+{
+	if (old_state && bridge->funcs->atomic_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_enable(bridge, old_bridge_state);
+	} else if (bridge->funcs->enable) {
+		bridge->funcs->enable(bridge);
+	}
+}
+
 /**
  * drm_atomic_bridge_chain_enable - enables all bridges in the encoder chain
  * @bridge: bridge control structure
@@ -832,31 +908,76 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable);
  * starting from the first bridge to the last. These are called after completing
  * &drm_encoder_helper_funcs.atomic_enable
  *
+ * If a bridge sets @enable_next_first, then the @enable for the next bridge
+ * will be called first before enable of this bridge.
+ *
+ * Example:
+ * Bridge A ---> Bridge B ---> Bridge C ---> Bridge D ---> Bridge E
+ *
+ * With enable_next_first flag enable in Bridge A, C, D then the resulting
+ * @enable order would be,
+ * Bridge B, Bridge A, Bridge E, Bridge D, Bridge C.
+ *
  * Note: the bridge passed should be the one closest to the encoder
  */
 void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge,
 				    struct drm_atomic_state *old_state)
 {
 	struct drm_encoder *encoder;
+	struct drm_bridge *next, *limit;
 
 	if (!bridge)
 		return;
 
 	encoder = bridge->encoder;
+
 	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
-		if (bridge->funcs->atomic_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_enable(bridge, old_bridge_state);
-		} else if (bridge->funcs->enable) {
-			bridge->funcs->enable(bridge);
+		limit = NULL;
+
+		if (!list_is_last(&bridge->chain_node, &encoder->bridge_chain)) {
+			if (bridge->enable_next_first) {
+				/* next bridge had requested that next
+				 * was enabled first, so disabled last
+				 */
+				next = list_next_entry(bridge, chain_node);
+				limit = next;
+
+				list_for_each_entry_from(next, &encoder->bridge_chain,
+							 chain_node) {
+					/* Find the next bridge that has NOT requested
+					 * next to be enabled first / disabled last
+					 */
+					if (!next->enable_next_first) {
+						limit = next;
+						break;
+					}
+
+					/* Last bridge has requested next to be limit
+					 * otherwise the control move to end of chain
+					 */
+					if (list_is_last(&next->chain_node,
+							 &encoder->bridge_chain)) {
+						limit = next;
+						break;
+					}
+				}
+
+				/* Call these bridges in reverse order */
+				list_for_each_entry_from_reverse(next, &encoder->bridge_chain,
+								 chain_node) {
+					if (next == bridge)
+						break;
+
+					drm_atomic_bridge_call_enable(next, old_state);
+				}
+			}
 		}
+
+		drm_atomic_bridge_call_enable(bridge, old_state);
+
+		if (limit)
+			/* Jump all bridges that we have already enabled */
+			bridge = limit;
 	}
 }
 EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index a1a31704b917..9879129047e4 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -752,6 +752,14 @@ struct drm_bridge {
 	 * before the peripheral.
 	 */
 	bool pre_enable_prev_first;
+	/**
+	 * @enable_next_first: The bridge requires that the next bridge @enable
+	 * function is called first before its @enable, and conversely for
+	 * @disable. This is most frequently a requirement for a DSI host to
+	 * receive MIPI-DCS commands from DSI sink first in order to switch
+	 * HS mode properly.
+	 */
+	bool enable_next_first;
 	/**
 	 * @ddc: Associated I2C adapter for DDC access, if any.
 	 */
-- 
2.25.1

[PATCH v7 10/12] drm/bridge: Implement enable_next_first to alter bridge init order

[Jagan Teki]

DSI sink devices typically send the MIPI-DCS commands to the DSI host
via general MIPI_DSI_DCS read and write API.

The classical DSI sequence mentioned that the DSI host receives MIPI-DCS
commands from the DSI sink first in order to switch HS mode properly.
Once the DSI host switches to HS mode any MIPI-DCS commands from the
DSI sink are unfunctional.

DSI sink uses the @enable function to send the MIPI-DCS commands. In a
typical DSI host, sink pipeline the @enable call chain start with the
DSI host, and then the DSI sink which is the "wrong" order as DSI host
@enable is called and switched to HS mode before DSI sink @enable.

If the DSI host enables with the @enable_next_first flag then the
@enable for the DSI sink will be called first before the @enable of
the DSI host. This alter bridge init order makes sure that the MIPI-DCS
commands send first and then switch to the HS mode properly by DSI host.

This new flag @enable_next_first that any bridg can set to swap the
order of @enable (and #disable) for that and the immediately next bridge.

[enable]
If a bridge sets @enable_next_first, then the @enable for the next bridge
will be called first before enable of this bridge.

[disable]
If a bridge sets @enable_next_first, then the @disable for the next bridge
will be called first before @disable of this bridge to reverse the @enable
calling direction.

eg:
- Panel
- Bridge 1
- Bridge 2 enable_next_first
- Bridge 3
- Bridge 4 enable_next_first
- Bridge 5 enable_next_first
- Bridge 6
- Encoder

Would result in enable's being called as Encoder, Bridge 6, Bridge 3,
Bridge 4, Bridge 5, Bridge 1, Bridge 2, Panel.

and the result in disable's being called as Panel, Bridge 2, Bridge 1,
Bridge 5, Bridge 4, Bridge 3, Bridge 6, Encoder.

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

 drivers/gpu/drm/drm_bridge.c | 171 ++++++++++++++++++++++++++++++-----
 include/drm/drm_bridge.h     |   8 ++
 2 files changed, 154 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index caf0f341e524..cdc2669b3512 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -577,6 +577,24 @@ void drm_bridge_chain_mode_set(struct drm_bridge *bridge,
 }
 EXPORT_SYMBOL(drm_bridge_chain_mode_set);
 
+static void drm_atomic_bridge_call_disable(struct drm_bridge *bridge,
+					   struct drm_atomic_state *old_state)
+{
+	if (old_state && bridge->funcs->atomic_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_disable(bridge, old_bridge_state);
+	} else if (bridge->funcs->disable) {
+		bridge->funcs->disable(bridge);
+	}
+}
+
 /**
  * drm_atomic_bridge_chain_disable - disables all bridges in the encoder chain
  * @bridge: bridge control structure
@@ -587,33 +605,73 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set);
  * starting from the last bridge to the first. These are called before calling
  * &drm_encoder_helper_funcs.atomic_disable
  *
+ * If a bridge sets @enable_next_first, then the @disable for the next bridge
+ * will be called first before @disable of this bridge to reverse the @enable
+ * calling direction.
+ *
+ * Example:
+ * Bridge A ---> Bridge B ---> Bridge C ---> Bridge D ---> Bridge E
+ *
+ * With enable_next_first flag enable in Bridge A, C, D then the resulting
+ * @disable order would be,
+ * Bridge C, Bridge D, Bridge E, Bridge A, Bridge B.
+ *
  * Note: the bridge passed should be the one closest to the encoder
  */
 void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge,
 				     struct drm_atomic_state *old_state)
 {
 	struct drm_encoder *encoder;
-	struct drm_bridge *iter;
+	struct drm_bridge *iter, *next, *limit;
 
 	if (!bridge)
 		return;
 
 	encoder = bridge->encoder;
+
 	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
-		if (iter->funcs->atomic_disable) {
-			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;
-
-			iter->funcs->atomic_disable(iter, old_bridge_state);
-		} else if (iter->funcs->disable) {
-			iter->funcs->disable(iter);
+		limit = NULL;
+
+		if (!list_is_first(&iter->chain_node, &encoder->bridge_chain)) {
+			next = list_prev_entry(iter, chain_node);
+
+			if (next->enable_next_first) {
+				limit = bridge;
+				list_for_each_entry_from_reverse(next,
+							 &encoder->bridge_chain,
+							 chain_node) {
+					if (next == bridge)
+						break;
+
+					if (!next->enable_next_first) {
+						/* Found first bridge that does NOT
+						 * request next to be enabled first
+						 */
+						next = list_next_entry(next, chain_node);
+						limit = next;
+						break;
+					}
+				}
+
+				list_for_each_entry_from(next, &encoder->bridge_chain, chain_node) {
+					/* Call requested next bridge enable in order */
+					if (next == iter)
+						/* At the first bridge to request next
+						 * bridges called first.
+						 */
+						break;
+
+					drm_atomic_bridge_call_disable(next, old_state);
+				}
+			}
 		}
 
+		drm_atomic_bridge_call_disable(iter, old_state);
+
+		if (limit)
+			/* Jump all bridges that we have already disabled */
+			iter = limit;
+
 		if (iter == bridge)
 			break;
 	}
@@ -822,6 +880,24 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
 }
 EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable);
 
+static void drm_atomic_bridge_call_enable(struct drm_bridge *bridge,
+					  struct drm_atomic_state *old_state)
+{
+	if (old_state && bridge->funcs->atomic_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_enable(bridge, old_bridge_state);
+	} else if (bridge->funcs->enable) {
+		bridge->funcs->enable(bridge);
+	}
+}
+
 /**
  * drm_atomic_bridge_chain_enable - enables all bridges in the encoder chain
  * @bridge: bridge control structure
@@ -832,31 +908,76 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable);
  * starting from the first bridge to the last. These are called after completing
  * &drm_encoder_helper_funcs.atomic_enable
  *
+ * If a bridge sets @enable_next_first, then the @enable for the next bridge
+ * will be called first before enable of this bridge.
+ *
+ * Example:
+ * Bridge A ---> Bridge B ---> Bridge C ---> Bridge D ---> Bridge E
+ *
+ * With enable_next_first flag enable in Bridge A, C, D then the resulting
+ * @enable order would be,
+ * Bridge B, Bridge A, Bridge E, Bridge D, Bridge C.
+ *
  * Note: the bridge passed should be the one closest to the encoder
  */
 void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge,
 				    struct drm_atomic_state *old_state)
 {
 	struct drm_encoder *encoder;
+	struct drm_bridge *next, *limit;
 
 	if (!bridge)
 		return;
 
 	encoder = bridge->encoder;
+
 	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
-		if (bridge->funcs->atomic_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_enable(bridge, old_bridge_state);
-		} else if (bridge->funcs->enable) {
-			bridge->funcs->enable(bridge);
+		limit = NULL;
+
+		if (!list_is_last(&bridge->chain_node, &encoder->bridge_chain)) {
+			if (bridge->enable_next_first) {
+				/* next bridge had requested that next
+				 * was enabled first, so disabled last
+				 */
+				next = list_next_entry(bridge, chain_node);
+				limit = next;
+
+				list_for_each_entry_from(next, &encoder->bridge_chain,
+							 chain_node) {
+					/* Find the next bridge that has NOT requested
+					 * next to be enabled first / disabled last
+					 */
+					if (!next->enable_next_first) {
+						limit = next;
+						break;
+					}
+
+					/* Last bridge has requested next to be limit
+					 * otherwise the control move to end of chain
+					 */
+					if (list_is_last(&next->chain_node,
+							 &encoder->bridge_chain)) {
+						limit = next;
+						break;
+					}
+				}
+
+				/* Call these bridges in reverse order */
+				list_for_each_entry_from_reverse(next, &encoder->bridge_chain,
+								 chain_node) {
+					if (next == bridge)
+						break;
+
+					drm_atomic_bridge_call_enable(next, old_state);
+				}
+			}
 		}
+
+		drm_atomic_bridge_call_enable(bridge, old_state);
+
+		if (limit)
+			/* Jump all bridges that we have already enabled */
+			bridge = limit;
 	}
 }
 EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index a1a31704b917..9879129047e4 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -752,6 +752,14 @@ struct drm_bridge {
 	 * before the peripheral.
 	 */
 	bool pre_enable_prev_first;
+	/**
+	 * @enable_next_first: The bridge requires that the next bridge @enable
+	 * function is called first before its @enable, and conversely for
+	 * @disable. This is most frequently a requirement for a DSI host to
+	 * receive MIPI-DCS commands from DSI sink first in order to switch
+	 * HS mode properly.
+	 */
+	bool enable_next_first;
 	/**
 	 * @ddc: Associated I2C adapter for DDC access, if any.
 	 */
-- 
2.25.1

[PATCH v7 10/12] drm/bridge: Implement enable_next_first to alter bridge init order

[Jagan Teki]

DSI sink devices typically send the MIPI-DCS commands to the DSI host
via general MIPI_DSI_DCS read and write API.

The classical DSI sequence mentioned that the DSI host receives MIPI-DCS
commands from the DSI sink first in order to switch HS mode properly.
Once the DSI host switches to HS mode any MIPI-DCS commands from the
DSI sink are unfunctional.

DSI sink uses the @enable function to send the MIPI-DCS commands. In a
typical DSI host, sink pipeline the @enable call chain start with the
DSI host, and then the DSI sink which is the "wrong" order as DSI host
@enable is called and switched to HS mode before DSI sink @enable.

If the DSI host enables with the @enable_next_first flag then the
@enable for the DSI sink will be called first before the @enable of
the DSI host. This alter bridge init order makes sure that the MIPI-DCS
commands send first and then switch to the HS mode properly by DSI host.

This new flag @enable_next_first that any bridg can set to swap the
order of @enable (and #disable) for that and the immediately next bridge.

[enable]
If a bridge sets @enable_next_first, then the @enable for the next bridge
will be called first before enable of this bridge.

[disable]
If a bridge sets @enable_next_first, then the @disable for the next bridge
will be called first before @disable of this bridge to reverse the @enable
calling direction.

eg:
- Panel
- Bridge 1
- Bridge 2 enable_next_first
- Bridge 3
- Bridge 4 enable_next_first
- Bridge 5 enable_next_first
- Bridge 6
- Encoder

Would result in enable's being called as Encoder, Bridge 6, Bridge 3,
Bridge 4, Bridge 5, Bridge 1, Bridge 2, Panel.

and the result in disable's being called as Panel, Bridge 2, Bridge 1,
Bridge 5, Bridge 4, Bridge 3, Bridge 6, Encoder.

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

 drivers/gpu/drm/drm_bridge.c | 171 ++++++++++++++++++++++++++++++-----
 include/drm/drm_bridge.h     |   8 ++
 2 files changed, 154 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index caf0f341e524..cdc2669b3512 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -577,6 +577,24 @@ void drm_bridge_chain_mode_set(struct drm_bridge *bridge,
 }
 EXPORT_SYMBOL(drm_bridge_chain_mode_set);
 
+static void drm_atomic_bridge_call_disable(struct drm_bridge *bridge,
+					   struct drm_atomic_state *old_state)
+{
+	if (old_state && bridge->funcs->atomic_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_disable(bridge, old_bridge_state);
+	} else if (bridge->funcs->disable) {
+		bridge->funcs->disable(bridge);
+	}
+}
+
 /**
  * drm_atomic_bridge_chain_disable - disables all bridges in the encoder chain
  * @bridge: bridge control structure
@@ -587,33 +605,73 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set);
  * starting from the last bridge to the first. These are called before calling
  * &drm_encoder_helper_funcs.atomic_disable
  *
+ * If a bridge sets @enable_next_first, then the @disable for the next bridge
+ * will be called first before @disable of this bridge to reverse the @enable
+ * calling direction.
+ *
+ * Example:
+ * Bridge A ---> Bridge B ---> Bridge C ---> Bridge D ---> Bridge E
+ *
+ * With enable_next_first flag enable in Bridge A, C, D then the resulting
+ * @disable order would be,
+ * Bridge C, Bridge D, Bridge E, Bridge A, Bridge B.
+ *
  * Note: the bridge passed should be the one closest to the encoder
  */
 void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge,
 				     struct drm_atomic_state *old_state)
 {
 	struct drm_encoder *encoder;
-	struct drm_bridge *iter;
+	struct drm_bridge *iter, *next, *limit;
 
 	if (!bridge)
 		return;
 
 	encoder = bridge->encoder;
+
 	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
-		if (iter->funcs->atomic_disable) {
-			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;
-
-			iter->funcs->atomic_disable(iter, old_bridge_state);
-		} else if (iter->funcs->disable) {
-			iter->funcs->disable(iter);
+		limit = NULL;
+
+		if (!list_is_first(&iter->chain_node, &encoder->bridge_chain)) {
+			next = list_prev_entry(iter, chain_node);
+
+			if (next->enable_next_first) {
+				limit = bridge;
+				list_for_each_entry_from_reverse(next,
+							 &encoder->bridge_chain,
+							 chain_node) {
+					if (next == bridge)
+						break;
+
+					if (!next->enable_next_first) {
+						/* Found first bridge that does NOT
+						 * request next to be enabled first
+						 */
+						next = list_next_entry(next, chain_node);
+						limit = next;
+						break;
+					}
+				}
+
+				list_for_each_entry_from(next, &encoder->bridge_chain, chain_node) {
+					/* Call requested next bridge enable in order */
+					if (next == iter)
+						/* At the first bridge to request next
+						 * bridges called first.
+						 */
+						break;
+
+					drm_atomic_bridge_call_disable(next, old_state);
+				}
+			}
 		}
 
+		drm_atomic_bridge_call_disable(iter, old_state);
+
+		if (limit)
+			/* Jump all bridges that we have already disabled */
+			iter = limit;
+
 		if (iter == bridge)
 			break;
 	}
@@ -822,6 +880,24 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
 }
 EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable);
 
+static void drm_atomic_bridge_call_enable(struct drm_bridge *bridge,
+					  struct drm_atomic_state *old_state)
+{
+	if (old_state && bridge->funcs->atomic_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_enable(bridge, old_bridge_state);
+	} else if (bridge->funcs->enable) {
+		bridge->funcs->enable(bridge);
+	}
+}
+
 /**
  * drm_atomic_bridge_chain_enable - enables all bridges in the encoder chain
  * @bridge: bridge control structure
@@ -832,31 +908,76 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable);
  * starting from the first bridge to the last. These are called after completing
  * &drm_encoder_helper_funcs.atomic_enable
  *
+ * If a bridge sets @enable_next_first, then the @enable for the next bridge
+ * will be called first before enable of this bridge.
+ *
+ * Example:
+ * Bridge A ---> Bridge B ---> Bridge C ---> Bridge D ---> Bridge E
+ *
+ * With enable_next_first flag enable in Bridge A, C, D then the resulting
+ * @enable order would be,
+ * Bridge B, Bridge A, Bridge E, Bridge D, Bridge C.
+ *
  * Note: the bridge passed should be the one closest to the encoder
  */
 void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge,
 				    struct drm_atomic_state *old_state)
 {
 	struct drm_encoder *encoder;
+	struct drm_bridge *next, *limit;
 
 	if (!bridge)
 		return;
 
 	encoder = bridge->encoder;
+
 	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
-		if (bridge->funcs->atomic_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_enable(bridge, old_bridge_state);
-		} else if (bridge->funcs->enable) {
-			bridge->funcs->enable(bridge);
+		limit = NULL;
+
+		if (!list_is_last(&bridge->chain_node, &encoder->bridge_chain)) {
+			if (bridge->enable_next_first) {
+				/* next bridge had requested that next
+				 * was enabled first, so disabled last
+				 */
+				next = list_next_entry(bridge, chain_node);
+				limit = next;
+
+				list_for_each_entry_from(next, &encoder->bridge_chain,
+							 chain_node) {
+					/* Find the next bridge that has NOT requested
+					 * next to be enabled first / disabled last
+					 */
+					if (!next->enable_next_first) {
+						limit = next;
+						break;
+					}
+
+					/* Last bridge has requested next to be limit
+					 * otherwise the control move to end of chain
+					 */
+					if (list_is_last(&next->chain_node,
+							 &encoder->bridge_chain)) {
+						limit = next;
+						break;
+					}
+				}
+
+				/* Call these bridges in reverse order */
+				list_for_each_entry_from_reverse(next, &encoder->bridge_chain,
+								 chain_node) {
+					if (next == bridge)
+						break;
+
+					drm_atomic_bridge_call_enable(next, old_state);
+				}
+			}
 		}
+
+		drm_atomic_bridge_call_enable(bridge, old_state);
+
+		if (limit)
+			/* Jump all bridges that we have already enabled */
+			bridge = limit;
 	}
 }
 EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index a1a31704b917..9879129047e4 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -752,6 +752,14 @@ struct drm_bridge {
 	 * before the peripheral.
 	 */
 	bool pre_enable_prev_first;
+	/**
+	 * @enable_next_first: The bridge requires that the next bridge @enable
+	 * function is called first before its @enable, and conversely for
+	 * @disable. This is most frequently a requirement for a DSI host to
+	 * receive MIPI-DCS commands from DSI sink first in order to switch
+	 * HS mode properly.
+	 */
+	bool enable_next_first;
 	/**
 	 * @ddc: Associated I2C adapter for DDC access, if any.
 	 */
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v7 11/12] drm/bridge: Document bridge init order with enable_next_first

[Jagan Teki]

In order to switch HS mode properly by DSI host, the DSI sink has to
send the MIPI-DCS commands first before the DSI host switches to HS
mode.

This behavior requires a bridge init alter in @enable and @disable
function calls with the help of @enable_next_first.

Document the affected bridge init order with a proper explanation.

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

 drivers/gpu/drm/drm_bridge.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index cdc2669b3512..3c6c9937537a 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -190,6 +190,21 @@
  * 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.
+ *
+ * DSI sink devices typically send the MIPI-DCS commands to the DSI host via
+ * general MIPI_DSI_DCS read and write API. The classical DSI sequence
+ * mentioned that the DSI host receives MIPI-DCS commands from the DSI sink
+ * first in order to switch HS mode properly. Once the DSI host switches to
+ * HS mode any MIPI-DCS commands from the DSI sink are unfunctional.
+ *
+ * DSI sink uses the @enable function to send the MIPI-DCS commands. In a
+ * typical DSI host, sink pipeline the @enable call chain start with the
+ * DSI host, and then the DSI sink which is the "wrong" order as DSI host
+ * @enable is called and switched to HS mode before DSI sink @enable. If
+ * the DSI host enables with the @enable_next_first flag then the @enable
+ * for the DSI sink will be called first before the @enable of the DSI host.
+ * This alter bridge init order makes sure that the MIPI-DCS commands send
+ * first and then switch to the HS mode properly by the DSI host.
  */
 
 static DEFINE_MUTEX(bridge_lock);
-- 
2.25.1

[PATCH v7 11/12] drm/bridge: Document bridge init order with enable_next_first

[Jagan Teki]

In order to switch HS mode properly by DSI host, the DSI sink has to
send the MIPI-DCS commands first before the DSI host switches to HS
mode.

This behavior requires a bridge init alter in @enable and @disable
function calls with the help of @enable_next_first.

Document the affected bridge init order with a proper explanation.

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

 drivers/gpu/drm/drm_bridge.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index cdc2669b3512..3c6c9937537a 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -190,6 +190,21 @@
  * 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.
+ *
+ * DSI sink devices typically send the MIPI-DCS commands to the DSI host via
+ * general MIPI_DSI_DCS read and write API. The classical DSI sequence
+ * mentioned that the DSI host receives MIPI-DCS commands from the DSI sink
+ * first in order to switch HS mode properly. Once the DSI host switches to
+ * HS mode any MIPI-DCS commands from the DSI sink are unfunctional.
+ *
+ * DSI sink uses the @enable function to send the MIPI-DCS commands. In a
+ * typical DSI host, sink pipeline the @enable call chain start with the
+ * DSI host, and then the DSI sink which is the "wrong" order as DSI host
+ * @enable is called and switched to HS mode before DSI sink @enable. If
+ * the DSI host enables with the @enable_next_first flag then the @enable
+ * for the DSI sink will be called first before the @enable of the DSI host.
+ * This alter bridge init order makes sure that the MIPI-DCS commands send
+ * first and then switch to the HS mode properly by the DSI host.
  */
 
 static DEFINE_MUTEX(bridge_lock);
-- 
2.25.1

[PATCH v7 11/12] drm/bridge: Document bridge init order with enable_next_first

[Jagan Teki]

In order to switch HS mode properly by DSI host, the DSI sink has to
send the MIPI-DCS commands first before the DSI host switches to HS
mode.

This behavior requires a bridge init alter in @enable and @disable
function calls with the help of @enable_next_first.

Document the affected bridge init order with a proper explanation.

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

 drivers/gpu/drm/drm_bridge.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index cdc2669b3512..3c6c9937537a 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -190,6 +190,21 @@
  * 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.
+ *
+ * DSI sink devices typically send the MIPI-DCS commands to the DSI host via
+ * general MIPI_DSI_DCS read and write API. The classical DSI sequence
+ * mentioned that the DSI host receives MIPI-DCS commands from the DSI sink
+ * first in order to switch HS mode properly. Once the DSI host switches to
+ * HS mode any MIPI-DCS commands from the DSI sink are unfunctional.
+ *
+ * DSI sink uses the @enable function to send the MIPI-DCS commands. In a
+ * typical DSI host, sink pipeline the @enable call chain start with the
+ * DSI host, and then the DSI sink which is the "wrong" order as DSI host
+ * @enable is called and switched to HS mode before DSI sink @enable. If
+ * the DSI host enables with the @enable_next_first flag then the @enable
+ * for the DSI sink will be called first before the @enable of the DSI host.
+ * This alter bridge init order makes sure that the MIPI-DCS commands send
+ * first and then switch to the HS mode properly by the DSI host.
  */
 
 static DEFINE_MUTEX(bridge_lock);
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v7 12/12] drm: sun4: dsi: Convert to bridge driver

[Jagan Teki]

Convert the encoder to bridge driver in order to standardize on a
single API by supporting all varients of downstream bridge devices.

The drm_encoder can't be removed as it's exposed to userspace, so it
then becomes a dumb encoder, without any operation implemented.

Tested on DSI Panel, DSI Bridge, I2C-Configured DSI Bridge.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v7:
- drop bridge call chain
- use drmm_of_dsi_get_bridge
- switch to atomic bridge calls
- use atomic_pre_enable and atomic_enable for previous enable
Changes for v6:
- support donwstream bridge
- drop bridge conversion
- devm_drm_of_get_bridge() require child lookup
https://patchwork.kernel.org/project/dri-devel/cover/20211207054747.461029-1-jagan@amarulasolutions.com/
Changes for v5:
- add atomic APIs
- find host and device variant DSI devices.
Changes for v4, v3:
- none
 
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 143 ++++++++++---------------
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h |  10 +-
 2 files changed, 61 insertions(+), 92 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index 760ff05eabf4..71951a6dc914 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -20,8 +20,8 @@
 #include <linux/slab.h>
 
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_bridge.h>
 #include <drm/drm_mipi_dsi.h>
-#include <drm/drm_panel.h>
 #include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_simple_kms_helper.h>
@@ -713,10 +713,11 @@ static int sun6i_dsi_start(struct sun6i_dsi *dsi,
 	return 0;
 }
 
-static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
+static void sun6i_dsi_bridge_pre_enable(struct drm_bridge *bridge,
+					struct drm_bridge_state *old_state)
 {
-	struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode;
-	struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder);
+	struct drm_display_mode *mode = &bridge->encoder->crtc->state->adjusted_mode;
+	struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge);
 	struct mipi_dsi_device *device = dsi->device;
 	union phy_configure_opts opts = { };
 	struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy;
@@ -768,9 +769,12 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
 	phy_set_mode(dsi->dphy, PHY_MODE_MIPI_DPHY);
 	phy_configure(dsi->dphy, &opts);
 	phy_power_on(dsi->dphy);
+}
 
-	if (dsi->panel)
-		drm_panel_prepare(dsi->panel);
+static void sun6i_dsi_bridge_enable(struct drm_bridge *bridge,
+					struct drm_bridge_state *old_state)
+{
+	struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge);
 
 	/*
 	 * FIXME: This should be moved after the switch to HS mode.
@@ -784,9 +788,6 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
 	 * ordering on the panels I've tested it with, so I guess this
 	 * will do for now, until that IP is better understood.
 	 */
-	if (dsi->panel)
-		drm_panel_enable(dsi->panel);
-
 	sun6i_dsi_start(dsi, DSI_START_HSC);
 
 	udelay(1000);
@@ -794,17 +795,13 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
 	sun6i_dsi_start(dsi, DSI_START_HSD);
 }
 
-static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
+static void sun6i_dsi_bridge_disable(struct drm_bridge *bridge,
+				     struct drm_bridge_state *old_state)
 {
-	struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder);
+	struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge);
 
 	DRM_DEBUG_DRIVER("Disabling DSI output\n");
 
-	if (dsi->panel) {
-		drm_panel_disable(dsi->panel);
-		drm_panel_unprepare(dsi->panel);
-	}
-
 	phy_power_off(dsi->dphy);
 	phy_exit(dsi->dphy);
 
@@ -813,38 +810,23 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
 	regulator_disable(dsi->regulator);
 }
 
-static int sun6i_dsi_get_modes(struct drm_connector *connector)
-{
-	struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector);
-
-	return drm_panel_get_modes(dsi->panel, connector);
-}
-
-static const struct drm_connector_helper_funcs sun6i_dsi_connector_helper_funcs = {
-	.get_modes	= sun6i_dsi_get_modes,
-};
-
-static enum drm_connector_status
-sun6i_dsi_connector_detect(struct drm_connector *connector, bool force)
+static int sun6i_dsi_bridge_attach(struct drm_bridge *bridge,
+				   enum drm_bridge_attach_flags flags)
 {
-	struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector);
+	struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge);
 
-	return dsi->panel ? connector_status_connected :
-			    connector_status_disconnected;
+	return drm_bridge_attach(bridge->encoder, dsi->out_bridge,
+				 &dsi->bridge, flags);
 }
 
-static const struct drm_connector_funcs sun6i_dsi_connector_funcs = {
-	.detect			= sun6i_dsi_connector_detect,
-	.fill_modes		= drm_helper_probe_single_connector_modes,
-	.destroy		= drm_connector_cleanup,
-	.reset			= drm_atomic_helper_connector_reset,
-	.atomic_duplicate_state	= drm_atomic_helper_connector_duplicate_state,
-	.atomic_destroy_state	= drm_atomic_helper_connector_destroy_state,
-};
-
-static const struct drm_encoder_helper_funcs sun6i_dsi_enc_helper_funcs = {
-	.disable	= sun6i_dsi_encoder_disable,
-	.enable		= sun6i_dsi_encoder_enable,
+static const struct drm_bridge_funcs sun6i_mipi_dsi_bridge_funcs = {
+	.atomic_duplicate_state	= drm_atomic_helper_bridge_duplicate_state,
+	.atomic_destroy_state	= drm_atomic_helper_bridge_destroy_state,
+	.atomic_reset		= drm_atomic_helper_bridge_reset,
+	.atomic_pre_enable	= sun6i_dsi_bridge_pre_enable,
+	.atomic_enable		= sun6i_dsi_bridge_enable,
+	.atomic_disable		= sun6i_dsi_bridge_disable,
+	.attach			= sun6i_dsi_bridge_attach,
 };
 
 static u32 sun6i_dsi_dcs_build_pkt_hdr(struct sun6i_dsi *dsi,
@@ -959,20 +941,27 @@ static int sun6i_dsi_dcs_read(struct sun6i_dsi *dsi,
 	return 1;
 }
 
+static const struct component_ops sun6i_dsi_ops;
+
 static int sun6i_dsi_attach(struct mipi_dsi_host *host,
 			    struct mipi_dsi_device *device)
 {
 	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
-	struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);
+	int ret;
+
+	dsi->device = device;
+
+	drm_bridge_add(&dsi->bridge);
+
+	ret = component_add(dsi->dev, &sun6i_dsi_ops);
+	if (ret) {
+		dev_err(dsi->dev, "Couldn't register our component\n");
+		return ret;
+	}
 
-	if (IS_ERR(panel))
-		return PTR_ERR(panel);
 	if (!dsi->drm || !dsi->drm->registered)
 		return -EPROBE_DEFER;
 
-	dsi->panel = panel;
-	dsi->device = device;
-
 	drm_kms_helper_hotplug_event(dsi->drm);
 
 	dev_info(host->dev, "Attached device %s\n", device->name);
@@ -985,11 +974,10 @@ static int sun6i_dsi_detach(struct mipi_dsi_host *host,
 {
 	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
 
-	dsi->panel = NULL;
+	component_del(dsi->dev, &sun6i_dsi_ops);
+	drm_bridge_remove(&dsi->bridge);
 	dsi->device = NULL;
 
-	drm_kms_helper_hotplug_event(dsi->drm);
-
 	return 0;
 }
 
@@ -1054,8 +1042,13 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master,
 	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
 	int ret;
 
-	drm_encoder_helper_add(&dsi->encoder,
-			       &sun6i_dsi_enc_helper_funcs);
+	dsi->out_bridge = drmm_of_dsi_get_bridge(drm, dev->of_node, 0, 1);
+	if (IS_ERR(dsi->out_bridge)) {
+		ret = PTR_ERR(dsi->out_bridge);
+		DRM_DEV_ERROR(dsi->dev, "failed to find the bridge: %d\n", ret);
+		return ret;
+	}
+
 	ret = drm_simple_encoder_init(drm, &dsi->encoder,
 				      DRM_MODE_ENCODER_DSI);
 	if (ret) {
@@ -1064,39 +1057,19 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master,
 	}
 	dsi->encoder.possible_crtcs = BIT(0);
 
-	drm_connector_helper_add(&dsi->connector,
-				 &sun6i_dsi_connector_helper_funcs);
-	ret = drm_connector_init(drm, &dsi->connector,
-				 &sun6i_dsi_connector_funcs,
-				 DRM_MODE_CONNECTOR_DSI);
+	ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, 0);
 	if (ret) {
-		dev_err(dsi->dev,
-			"Couldn't initialise the DSI connector\n");
-		goto err_cleanup_connector;
+		dev_err(dsi->dev, "Couldn't attach the DSI bridge\n");
+		return ret;
 	}
 
-	drm_connector_attach_encoder(&dsi->connector, &dsi->encoder);
-
 	dsi->drm = drm;
 
 	return 0;
-
-err_cleanup_connector:
-	drm_encoder_cleanup(&dsi->encoder);
-	return ret;
-}
-
-static void sun6i_dsi_unbind(struct device *dev, struct device *master,
-			    void *data)
-{
-	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
-
-	dsi->drm = NULL;
 }
 
 static const struct component_ops sun6i_dsi_ops = {
 	.bind	= sun6i_dsi_bind,
-	.unbind	= sun6i_dsi_unbind,
 };
 
 static int sun6i_dsi_probe(struct platform_device *pdev)
@@ -1175,22 +1148,19 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
 		goto err_unprotect_clk;
 	}
 
+	dsi->bridge.funcs = &sun6i_mipi_dsi_bridge_funcs;
+	dsi->bridge.of_node = dev->of_node;
+	dsi->bridge.type = DRM_MODE_CONNECTOR_DSI;
+	dsi->bridge.enable_next_first = true;
+
 	ret = mipi_dsi_host_register(&dsi->host);
 	if (ret) {
 		dev_err(dev, "Couldn't register MIPI-DSI host\n");
 		goto err_unprotect_clk;
 	}
 
-	ret = component_add(&pdev->dev, &sun6i_dsi_ops);
-	if (ret) {
-		dev_err(dev, "Couldn't register our component\n");
-		goto err_remove_dsi_host;
-	}
-
 	return 0;
 
-err_remove_dsi_host:
-	mipi_dsi_host_unregister(&dsi->host);
 err_unprotect_clk:
 	if (dsi->variant->has_mod_clk && dsi->variant->set_mod_clk)
 		clk_rate_exclusive_put(dsi->mod_clk);
@@ -1205,7 +1175,6 @@ static int sun6i_dsi_remove(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
 
-	component_del(&pdev->dev, &sun6i_dsi_ops);
 	mipi_dsi_host_unregister(&dsi->host);
 	if (dsi->variant->has_mod_clk && dsi->variant->set_mod_clk)
 		clk_rate_exclusive_put(dsi->mod_clk);
diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
index f1ddefe0f554..8b9263e0f4ef 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
@@ -21,9 +21,9 @@ struct sun6i_dsi_variant {
 };
 
 struct sun6i_dsi {
-	struct drm_connector	connector;
 	struct drm_encoder	encoder;
 	struct mipi_dsi_host	host;
+	struct drm_bridge	bridge;
 
 	struct clk		*bus_clk;
 	struct clk		*mod_clk;
@@ -35,7 +35,7 @@ struct sun6i_dsi {
 	struct device		*dev;
 	struct mipi_dsi_device	*device;
 	struct drm_device	*drm;
-	struct drm_panel	*panel;
+	struct drm_bridge	*out_bridge;
 
 	const struct sun6i_dsi_variant *variant;
 };
@@ -45,10 +45,10 @@ static inline struct sun6i_dsi *host_to_sun6i_dsi(struct mipi_dsi_host *host)
 	return container_of(host, struct sun6i_dsi, host);
 };
 
-static inline struct sun6i_dsi *connector_to_sun6i_dsi(struct drm_connector *connector)
+static inline struct sun6i_dsi *bridge_to_sun6i_dsi(struct drm_bridge *bridge)
 {
-	return container_of(connector, struct sun6i_dsi, connector);
-};
+	return container_of(bridge, struct sun6i_dsi, bridge);
+}
 
 static inline struct sun6i_dsi *encoder_to_sun6i_dsi(const struct drm_encoder *encoder)
 {
-- 
2.25.1

[PATCH v7 12/12] drm: sun4: dsi: Convert to bridge driver

[Jagan Teki]

Convert the encoder to bridge driver in order to standardize on a
single API by supporting all varients of downstream bridge devices.

The drm_encoder can't be removed as it's exposed to userspace, so it
then becomes a dumb encoder, without any operation implemented.

Tested on DSI Panel, DSI Bridge, I2C-Configured DSI Bridge.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v7:
- drop bridge call chain
- use drmm_of_dsi_get_bridge
- switch to atomic bridge calls
- use atomic_pre_enable and atomic_enable for previous enable
Changes for v6:
- support donwstream bridge
- drop bridge conversion
- devm_drm_of_get_bridge() require child lookup
https://patchwork.kernel.org/project/dri-devel/cover/20211207054747.461029-1-jagan@amarulasolutions.com/
Changes for v5:
- add atomic APIs
- find host and device variant DSI devices.
Changes for v4, v3:
- none
 
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 143 ++++++++++---------------
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h |  10 +-
 2 files changed, 61 insertions(+), 92 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index 760ff05eabf4..71951a6dc914 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -20,8 +20,8 @@
 #include <linux/slab.h>
 
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_bridge.h>
 #include <drm/drm_mipi_dsi.h>
-#include <drm/drm_panel.h>
 #include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_simple_kms_helper.h>
@@ -713,10 +713,11 @@ static int sun6i_dsi_start(struct sun6i_dsi *dsi,
 	return 0;
 }
 
-static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
+static void sun6i_dsi_bridge_pre_enable(struct drm_bridge *bridge,
+					struct drm_bridge_state *old_state)
 {
-	struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode;
-	struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder);
+	struct drm_display_mode *mode = &bridge->encoder->crtc->state->adjusted_mode;
+	struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge);
 	struct mipi_dsi_device *device = dsi->device;
 	union phy_configure_opts opts = { };
 	struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy;
@@ -768,9 +769,12 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
 	phy_set_mode(dsi->dphy, PHY_MODE_MIPI_DPHY);
 	phy_configure(dsi->dphy, &opts);
 	phy_power_on(dsi->dphy);
+}
 
-	if (dsi->panel)
-		drm_panel_prepare(dsi->panel);
+static void sun6i_dsi_bridge_enable(struct drm_bridge *bridge,
+					struct drm_bridge_state *old_state)
+{
+	struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge);
 
 	/*
 	 * FIXME: This should be moved after the switch to HS mode.
@@ -784,9 +788,6 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
 	 * ordering on the panels I've tested it with, so I guess this
 	 * will do for now, until that IP is better understood.
 	 */
-	if (dsi->panel)
-		drm_panel_enable(dsi->panel);
-
 	sun6i_dsi_start(dsi, DSI_START_HSC);
 
 	udelay(1000);
@@ -794,17 +795,13 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
 	sun6i_dsi_start(dsi, DSI_START_HSD);
 }
 
-static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
+static void sun6i_dsi_bridge_disable(struct drm_bridge *bridge,
+				     struct drm_bridge_state *old_state)
 {
-	struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder);
+	struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge);
 
 	DRM_DEBUG_DRIVER("Disabling DSI output\n");
 
-	if (dsi->panel) {
-		drm_panel_disable(dsi->panel);
-		drm_panel_unprepare(dsi->panel);
-	}
-
 	phy_power_off(dsi->dphy);
 	phy_exit(dsi->dphy);
 
@@ -813,38 +810,23 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
 	regulator_disable(dsi->regulator);
 }
 
-static int sun6i_dsi_get_modes(struct drm_connector *connector)
-{
-	struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector);
-
-	return drm_panel_get_modes(dsi->panel, connector);
-}
-
-static const struct drm_connector_helper_funcs sun6i_dsi_connector_helper_funcs = {
-	.get_modes	= sun6i_dsi_get_modes,
-};
-
-static enum drm_connector_status
-sun6i_dsi_connector_detect(struct drm_connector *connector, bool force)
+static int sun6i_dsi_bridge_attach(struct drm_bridge *bridge,
+				   enum drm_bridge_attach_flags flags)
 {
-	struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector);
+	struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge);
 
-	return dsi->panel ? connector_status_connected :
-			    connector_status_disconnected;
+	return drm_bridge_attach(bridge->encoder, dsi->out_bridge,
+				 &dsi->bridge, flags);
 }
 
-static const struct drm_connector_funcs sun6i_dsi_connector_funcs = {
-	.detect			= sun6i_dsi_connector_detect,
-	.fill_modes		= drm_helper_probe_single_connector_modes,
-	.destroy		= drm_connector_cleanup,
-	.reset			= drm_atomic_helper_connector_reset,
-	.atomic_duplicate_state	= drm_atomic_helper_connector_duplicate_state,
-	.atomic_destroy_state	= drm_atomic_helper_connector_destroy_state,
-};
-
-static const struct drm_encoder_helper_funcs sun6i_dsi_enc_helper_funcs = {
-	.disable	= sun6i_dsi_encoder_disable,
-	.enable		= sun6i_dsi_encoder_enable,
+static const struct drm_bridge_funcs sun6i_mipi_dsi_bridge_funcs = {
+	.atomic_duplicate_state	= drm_atomic_helper_bridge_duplicate_state,
+	.atomic_destroy_state	= drm_atomic_helper_bridge_destroy_state,
+	.atomic_reset		= drm_atomic_helper_bridge_reset,
+	.atomic_pre_enable	= sun6i_dsi_bridge_pre_enable,
+	.atomic_enable		= sun6i_dsi_bridge_enable,
+	.atomic_disable		= sun6i_dsi_bridge_disable,
+	.attach			= sun6i_dsi_bridge_attach,
 };
 
 static u32 sun6i_dsi_dcs_build_pkt_hdr(struct sun6i_dsi *dsi,
@@ -959,20 +941,27 @@ static int sun6i_dsi_dcs_read(struct sun6i_dsi *dsi,
 	return 1;
 }
 
+static const struct component_ops sun6i_dsi_ops;
+
 static int sun6i_dsi_attach(struct mipi_dsi_host *host,
 			    struct mipi_dsi_device *device)
 {
 	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
-	struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);
+	int ret;
+
+	dsi->device = device;
+
+	drm_bridge_add(&dsi->bridge);
+
+	ret = component_add(dsi->dev, &sun6i_dsi_ops);
+	if (ret) {
+		dev_err(dsi->dev, "Couldn't register our component\n");
+		return ret;
+	}
 
-	if (IS_ERR(panel))
-		return PTR_ERR(panel);
 	if (!dsi->drm || !dsi->drm->registered)
 		return -EPROBE_DEFER;
 
-	dsi->panel = panel;
-	dsi->device = device;
-
 	drm_kms_helper_hotplug_event(dsi->drm);
 
 	dev_info(host->dev, "Attached device %s\n", device->name);
@@ -985,11 +974,10 @@ static int sun6i_dsi_detach(struct mipi_dsi_host *host,
 {
 	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
 
-	dsi->panel = NULL;
+	component_del(dsi->dev, &sun6i_dsi_ops);
+	drm_bridge_remove(&dsi->bridge);
 	dsi->device = NULL;
 
-	drm_kms_helper_hotplug_event(dsi->drm);
-
 	return 0;
 }
 
@@ -1054,8 +1042,13 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master,
 	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
 	int ret;
 
-	drm_encoder_helper_add(&dsi->encoder,
-			       &sun6i_dsi_enc_helper_funcs);
+	dsi->out_bridge = drmm_of_dsi_get_bridge(drm, dev->of_node, 0, 1);
+	if (IS_ERR(dsi->out_bridge)) {
+		ret = PTR_ERR(dsi->out_bridge);
+		DRM_DEV_ERROR(dsi->dev, "failed to find the bridge: %d\n", ret);
+		return ret;
+	}
+
 	ret = drm_simple_encoder_init(drm, &dsi->encoder,
 				      DRM_MODE_ENCODER_DSI);
 	if (ret) {
@@ -1064,39 +1057,19 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master,
 	}
 	dsi->encoder.possible_crtcs = BIT(0);
 
-	drm_connector_helper_add(&dsi->connector,
-				 &sun6i_dsi_connector_helper_funcs);
-	ret = drm_connector_init(drm, &dsi->connector,
-				 &sun6i_dsi_connector_funcs,
-				 DRM_MODE_CONNECTOR_DSI);
+	ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, 0);
 	if (ret) {
-		dev_err(dsi->dev,
-			"Couldn't initialise the DSI connector\n");
-		goto err_cleanup_connector;
+		dev_err(dsi->dev, "Couldn't attach the DSI bridge\n");
+		return ret;
 	}
 
-	drm_connector_attach_encoder(&dsi->connector, &dsi->encoder);
-
 	dsi->drm = drm;
 
 	return 0;
-
-err_cleanup_connector:
-	drm_encoder_cleanup(&dsi->encoder);
-	return ret;
-}
-
-static void sun6i_dsi_unbind(struct device *dev, struct device *master,
-			    void *data)
-{
-	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
-
-	dsi->drm = NULL;
 }
 
 static const struct component_ops sun6i_dsi_ops = {
 	.bind	= sun6i_dsi_bind,
-	.unbind	= sun6i_dsi_unbind,
 };
 
 static int sun6i_dsi_probe(struct platform_device *pdev)
@@ -1175,22 +1148,19 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
 		goto err_unprotect_clk;
 	}
 
+	dsi->bridge.funcs = &sun6i_mipi_dsi_bridge_funcs;
+	dsi->bridge.of_node = dev->of_node;
+	dsi->bridge.type = DRM_MODE_CONNECTOR_DSI;
+	dsi->bridge.enable_next_first = true;
+
 	ret = mipi_dsi_host_register(&dsi->host);
 	if (ret) {
 		dev_err(dev, "Couldn't register MIPI-DSI host\n");
 		goto err_unprotect_clk;
 	}
 
-	ret = component_add(&pdev->dev, &sun6i_dsi_ops);
-	if (ret) {
-		dev_err(dev, "Couldn't register our component\n");
-		goto err_remove_dsi_host;
-	}
-
 	return 0;
 
-err_remove_dsi_host:
-	mipi_dsi_host_unregister(&dsi->host);
 err_unprotect_clk:
 	if (dsi->variant->has_mod_clk && dsi->variant->set_mod_clk)
 		clk_rate_exclusive_put(dsi->mod_clk);
@@ -1205,7 +1175,6 @@ static int sun6i_dsi_remove(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
 
-	component_del(&pdev->dev, &sun6i_dsi_ops);
 	mipi_dsi_host_unregister(&dsi->host);
 	if (dsi->variant->has_mod_clk && dsi->variant->set_mod_clk)
 		clk_rate_exclusive_put(dsi->mod_clk);
diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
index f1ddefe0f554..8b9263e0f4ef 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
@@ -21,9 +21,9 @@ struct sun6i_dsi_variant {
 };
 
 struct sun6i_dsi {
-	struct drm_connector	connector;
 	struct drm_encoder	encoder;
 	struct mipi_dsi_host	host;
+	struct drm_bridge	bridge;
 
 	struct clk		*bus_clk;
 	struct clk		*mod_clk;
@@ -35,7 +35,7 @@ struct sun6i_dsi {
 	struct device		*dev;
 	struct mipi_dsi_device	*device;
 	struct drm_device	*drm;
-	struct drm_panel	*panel;
+	struct drm_bridge	*out_bridge;
 
 	const struct sun6i_dsi_variant *variant;
 };
@@ -45,10 +45,10 @@ static inline struct sun6i_dsi *host_to_sun6i_dsi(struct mipi_dsi_host *host)
 	return container_of(host, struct sun6i_dsi, host);
 };
 
-static inline struct sun6i_dsi *connector_to_sun6i_dsi(struct drm_connector *connector)
+static inline struct sun6i_dsi *bridge_to_sun6i_dsi(struct drm_bridge *bridge)
 {
-	return container_of(connector, struct sun6i_dsi, connector);
-};
+	return container_of(bridge, struct sun6i_dsi, bridge);
+}
 
 static inline struct sun6i_dsi *encoder_to_sun6i_dsi(const struct drm_encoder *encoder)
 {
-- 
2.25.1

[PATCH v7 12/12] drm: sun4: dsi: Convert to bridge driver

[Jagan Teki]

Convert the encoder to bridge driver in order to standardize on a
single API by supporting all varients of downstream bridge devices.

The drm_encoder can't be removed as it's exposed to userspace, so it
then becomes a dumb encoder, without any operation implemented.

Tested on DSI Panel, DSI Bridge, I2C-Configured DSI Bridge.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v7:
- drop bridge call chain
- use drmm_of_dsi_get_bridge
- switch to atomic bridge calls
- use atomic_pre_enable and atomic_enable for previous enable
Changes for v6:
- support donwstream bridge
- drop bridge conversion
- devm_drm_of_get_bridge() require child lookup
https://patchwork.kernel.org/project/dri-devel/cover/20211207054747.461029-1-jagan@amarulasolutions.com/
Changes for v5:
- add atomic APIs
- find host and device variant DSI devices.
Changes for v4, v3:
- none
 
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 143 ++++++++++---------------
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h |  10 +-
 2 files changed, 61 insertions(+), 92 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index 760ff05eabf4..71951a6dc914 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -20,8 +20,8 @@
 #include <linux/slab.h>
 
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_bridge.h>
 #include <drm/drm_mipi_dsi.h>
-#include <drm/drm_panel.h>
 #include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_simple_kms_helper.h>
@@ -713,10 +713,11 @@ static int sun6i_dsi_start(struct sun6i_dsi *dsi,
 	return 0;
 }
 
-static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
+static void sun6i_dsi_bridge_pre_enable(struct drm_bridge *bridge,
+					struct drm_bridge_state *old_state)
 {
-	struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode;
-	struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder);
+	struct drm_display_mode *mode = &bridge->encoder->crtc->state->adjusted_mode;
+	struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge);
 	struct mipi_dsi_device *device = dsi->device;
 	union phy_configure_opts opts = { };
 	struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy;
@@ -768,9 +769,12 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
 	phy_set_mode(dsi->dphy, PHY_MODE_MIPI_DPHY);
 	phy_configure(dsi->dphy, &opts);
 	phy_power_on(dsi->dphy);
+}
 
-	if (dsi->panel)
-		drm_panel_prepare(dsi->panel);
+static void sun6i_dsi_bridge_enable(struct drm_bridge *bridge,
+					struct drm_bridge_state *old_state)
+{
+	struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge);
 
 	/*
 	 * FIXME: This should be moved after the switch to HS mode.
@@ -784,9 +788,6 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
 	 * ordering on the panels I've tested it with, so I guess this
 	 * will do for now, until that IP is better understood.
 	 */
-	if (dsi->panel)
-		drm_panel_enable(dsi->panel);
-
 	sun6i_dsi_start(dsi, DSI_START_HSC);
 
 	udelay(1000);
@@ -794,17 +795,13 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
 	sun6i_dsi_start(dsi, DSI_START_HSD);
 }
 
-static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
+static void sun6i_dsi_bridge_disable(struct drm_bridge *bridge,
+				     struct drm_bridge_state *old_state)
 {
-	struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder);
+	struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge);
 
 	DRM_DEBUG_DRIVER("Disabling DSI output\n");
 
-	if (dsi->panel) {
-		drm_panel_disable(dsi->panel);
-		drm_panel_unprepare(dsi->panel);
-	}
-
 	phy_power_off(dsi->dphy);
 	phy_exit(dsi->dphy);
 
@@ -813,38 +810,23 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
 	regulator_disable(dsi->regulator);
 }
 
-static int sun6i_dsi_get_modes(struct drm_connector *connector)
-{
-	struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector);
-
-	return drm_panel_get_modes(dsi->panel, connector);
-}
-
-static const struct drm_connector_helper_funcs sun6i_dsi_connector_helper_funcs = {
-	.get_modes	= sun6i_dsi_get_modes,
-};
-
-static enum drm_connector_status
-sun6i_dsi_connector_detect(struct drm_connector *connector, bool force)
+static int sun6i_dsi_bridge_attach(struct drm_bridge *bridge,
+				   enum drm_bridge_attach_flags flags)
 {
-	struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector);
+	struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge);
 
-	return dsi->panel ? connector_status_connected :
-			    connector_status_disconnected;
+	return drm_bridge_attach(bridge->encoder, dsi->out_bridge,
+				 &dsi->bridge, flags);
 }
 
-static const struct drm_connector_funcs sun6i_dsi_connector_funcs = {
-	.detect			= sun6i_dsi_connector_detect,
-	.fill_modes		= drm_helper_probe_single_connector_modes,
-	.destroy		= drm_connector_cleanup,
-	.reset			= drm_atomic_helper_connector_reset,
-	.atomic_duplicate_state	= drm_atomic_helper_connector_duplicate_state,
-	.atomic_destroy_state	= drm_atomic_helper_connector_destroy_state,
-};
-
-static const struct drm_encoder_helper_funcs sun6i_dsi_enc_helper_funcs = {
-	.disable	= sun6i_dsi_encoder_disable,
-	.enable		= sun6i_dsi_encoder_enable,
+static const struct drm_bridge_funcs sun6i_mipi_dsi_bridge_funcs = {
+	.atomic_duplicate_state	= drm_atomic_helper_bridge_duplicate_state,
+	.atomic_destroy_state	= drm_atomic_helper_bridge_destroy_state,
+	.atomic_reset		= drm_atomic_helper_bridge_reset,
+	.atomic_pre_enable	= sun6i_dsi_bridge_pre_enable,
+	.atomic_enable		= sun6i_dsi_bridge_enable,
+	.atomic_disable		= sun6i_dsi_bridge_disable,
+	.attach			= sun6i_dsi_bridge_attach,
 };
 
 static u32 sun6i_dsi_dcs_build_pkt_hdr(struct sun6i_dsi *dsi,
@@ -959,20 +941,27 @@ static int sun6i_dsi_dcs_read(struct sun6i_dsi *dsi,
 	return 1;
 }
 
+static const struct component_ops sun6i_dsi_ops;
+
 static int sun6i_dsi_attach(struct mipi_dsi_host *host,
 			    struct mipi_dsi_device *device)
 {
 	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
-	struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);
+	int ret;
+
+	dsi->device = device;
+
+	drm_bridge_add(&dsi->bridge);
+
+	ret = component_add(dsi->dev, &sun6i_dsi_ops);
+	if (ret) {
+		dev_err(dsi->dev, "Couldn't register our component\n");
+		return ret;
+	}
 
-	if (IS_ERR(panel))
-		return PTR_ERR(panel);
 	if (!dsi->drm || !dsi->drm->registered)
 		return -EPROBE_DEFER;
 
-	dsi->panel = panel;
-	dsi->device = device;
-
 	drm_kms_helper_hotplug_event(dsi->drm);
 
 	dev_info(host->dev, "Attached device %s\n", device->name);
@@ -985,11 +974,10 @@ static int sun6i_dsi_detach(struct mipi_dsi_host *host,
 {
 	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
 
-	dsi->panel = NULL;
+	component_del(dsi->dev, &sun6i_dsi_ops);
+	drm_bridge_remove(&dsi->bridge);
 	dsi->device = NULL;
 
-	drm_kms_helper_hotplug_event(dsi->drm);
-
 	return 0;
 }
 
@@ -1054,8 +1042,13 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master,
 	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
 	int ret;
 
-	drm_encoder_helper_add(&dsi->encoder,
-			       &sun6i_dsi_enc_helper_funcs);
+	dsi->out_bridge = drmm_of_dsi_get_bridge(drm, dev->of_node, 0, 1);
+	if (IS_ERR(dsi->out_bridge)) {
+		ret = PTR_ERR(dsi->out_bridge);
+		DRM_DEV_ERROR(dsi->dev, "failed to find the bridge: %d\n", ret);
+		return ret;
+	}
+
 	ret = drm_simple_encoder_init(drm, &dsi->encoder,
 				      DRM_MODE_ENCODER_DSI);
 	if (ret) {
@@ -1064,39 +1057,19 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master,
 	}
 	dsi->encoder.possible_crtcs = BIT(0);
 
-	drm_connector_helper_add(&dsi->connector,
-				 &sun6i_dsi_connector_helper_funcs);
-	ret = drm_connector_init(drm, &dsi->connector,
-				 &sun6i_dsi_connector_funcs,
-				 DRM_MODE_CONNECTOR_DSI);
+	ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, 0);
 	if (ret) {
-		dev_err(dsi->dev,
-			"Couldn't initialise the DSI connector\n");
-		goto err_cleanup_connector;
+		dev_err(dsi->dev, "Couldn't attach the DSI bridge\n");
+		return ret;
 	}
 
-	drm_connector_attach_encoder(&dsi->connector, &dsi->encoder);
-
 	dsi->drm = drm;
 
 	return 0;
-
-err_cleanup_connector:
-	drm_encoder_cleanup(&dsi->encoder);
-	return ret;
-}
-
-static void sun6i_dsi_unbind(struct device *dev, struct device *master,
-			    void *data)
-{
-	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
-
-	dsi->drm = NULL;
 }
 
 static const struct component_ops sun6i_dsi_ops = {
 	.bind	= sun6i_dsi_bind,
-	.unbind	= sun6i_dsi_unbind,
 };
 
 static int sun6i_dsi_probe(struct platform_device *pdev)
@@ -1175,22 +1148,19 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
 		goto err_unprotect_clk;
 	}
 
+	dsi->bridge.funcs = &sun6i_mipi_dsi_bridge_funcs;
+	dsi->bridge.of_node = dev->of_node;
+	dsi->bridge.type = DRM_MODE_CONNECTOR_DSI;
+	dsi->bridge.enable_next_first = true;
+
 	ret = mipi_dsi_host_register(&dsi->host);
 	if (ret) {
 		dev_err(dev, "Couldn't register MIPI-DSI host\n");
 		goto err_unprotect_clk;
 	}
 
-	ret = component_add(&pdev->dev, &sun6i_dsi_ops);
-	if (ret) {
-		dev_err(dev, "Couldn't register our component\n");
-		goto err_remove_dsi_host;
-	}
-
 	return 0;
 
-err_remove_dsi_host:
-	mipi_dsi_host_unregister(&dsi->host);
 err_unprotect_clk:
 	if (dsi->variant->has_mod_clk && dsi->variant->set_mod_clk)
 		clk_rate_exclusive_put(dsi->mod_clk);
@@ -1205,7 +1175,6 @@ static int sun6i_dsi_remove(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
 
-	component_del(&pdev->dev, &sun6i_dsi_ops);
 	mipi_dsi_host_unregister(&dsi->host);
 	if (dsi->variant->has_mod_clk && dsi->variant->set_mod_clk)
 		clk_rate_exclusive_put(dsi->mod_clk);
diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
index f1ddefe0f554..8b9263e0f4ef 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
@@ -21,9 +21,9 @@ struct sun6i_dsi_variant {
 };
 
 struct sun6i_dsi {
-	struct drm_connector	connector;
 	struct drm_encoder	encoder;
 	struct mipi_dsi_host	host;
+	struct drm_bridge	bridge;
 
 	struct clk		*bus_clk;
 	struct clk		*mod_clk;
@@ -35,7 +35,7 @@ struct sun6i_dsi {
 	struct device		*dev;
 	struct mipi_dsi_device	*device;
 	struct drm_device	*drm;
-	struct drm_panel	*panel;
+	struct drm_bridge	*out_bridge;
 
 	const struct sun6i_dsi_variant *variant;
 };
@@ -45,10 +45,10 @@ static inline struct sun6i_dsi *host_to_sun6i_dsi(struct mipi_dsi_host *host)
 	return container_of(host, struct sun6i_dsi, host);
 };
 
-static inline struct sun6i_dsi *connector_to_sun6i_dsi(struct drm_connector *connector)
+static inline struct sun6i_dsi *bridge_to_sun6i_dsi(struct drm_bridge *bridge)
 {
-	return container_of(connector, struct sun6i_dsi, connector);
-};
+	return container_of(bridge, struct sun6i_dsi, bridge);
+}
 
 static inline struct sun6i_dsi *encoder_to_sun6i_dsi(const struct drm_encoder *encoder)
 {
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 12/12] drm: sun4: dsi: Convert to bridge driver

[Maxime Ripard]

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

Hi,

The patch prefix should be drm/sun4i:

On Wed, Mar 29, 2023 at 06:49:29PM +0530, Jagan Teki wrote:
> Convert the encoder to bridge driver in order to standardize on a
> single API by supporting all varients of downstream bridge devices.

Which variant, and why do we need to convert to a bridge to support all of them?

> The drm_encoder can't be removed as it's exposed to userspace, so it
> then becomes a dumb encoder, without any operation implemented.
> 
> Tested on DSI Panel, DSI Bridge, I2C-Configured DSI Bridge.
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>

[...]

> +static const struct component_ops sun6i_dsi_ops;
> +
>  static int sun6i_dsi_attach(struct mipi_dsi_host *host,
>  			    struct mipi_dsi_device *device)
>  {
>  	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> -	struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);

That one looks unrelated. Why do you need that change?

Maxime

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

Re: [PATCH v7 12/12] drm: sun4: dsi: Convert to bridge driver

[Maxime Ripard]

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

Hi,

The patch prefix should be drm/sun4i:

On Wed, Mar 29, 2023 at 06:49:29PM +0530, Jagan Teki wrote:
> Convert the encoder to bridge driver in order to standardize on a
> single API by supporting all varients of downstream bridge devices.

Which variant, and why do we need to convert to a bridge to support all of them?

> The drm_encoder can't be removed as it's exposed to userspace, so it
> then becomes a dumb encoder, without any operation implemented.
> 
> Tested on DSI Panel, DSI Bridge, I2C-Configured DSI Bridge.
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>

[...]

> +static const struct component_ops sun6i_dsi_ops;
> +
>  static int sun6i_dsi_attach(struct mipi_dsi_host *host,
>  			    struct mipi_dsi_device *device)
>  {
>  	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> -	struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);

That one looks unrelated. Why do you need that change?

Maxime

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

Re: [PATCH v7 12/12] drm: sun4: dsi: Convert to bridge driver

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 974 bytes --]

Hi,

The patch prefix should be drm/sun4i:

On Wed, Mar 29, 2023 at 06:49:29PM +0530, Jagan Teki wrote:
> Convert the encoder to bridge driver in order to standardize on a
> single API by supporting all varients of downstream bridge devices.

Which variant, and why do we need to convert to a bridge to support all of them?

> The drm_encoder can't be removed as it's exposed to userspace, so it
> then becomes a dumb encoder, without any operation implemented.
> 
> Tested on DSI Panel, DSI Bridge, I2C-Configured DSI Bridge.
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>

[...]

> +static const struct component_ops sun6i_dsi_ops;
> +
>  static int sun6i_dsi_attach(struct mipi_dsi_host *host,
>  			    struct mipi_dsi_device *device)
>  {
>  	struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> -	struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);

That one looks unrelated. Why do you need that change?

Maxime

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 12/12] drm: sun4: dsi: Convert to bridge driver

[Jagan Teki]

On Wed, Mar 29, 2023 at 8:29 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi,
>
> The patch prefix should be drm/sun4i:

I did follow my previous prefix, I will update this.

>
> On Wed, Mar 29, 2023 at 06:49:29PM +0530, Jagan Teki wrote:
> > Convert the encoder to bridge driver in order to standardize on a
> > single API by supporting all varients of downstream bridge devices.
>
> Which variant, and why do we need to convert to a bridge to support all of them?

Downstream bridge variants like DSI panel, DSI bridge and
I2C-Configured DSI bridges. Bridge conversion would be required for
the DSI host to access the more variety and complex downstream bridges
in a standardized bridge chain way which is indeed complex for encoder
driven DSI hosts.

>
> > The drm_encoder can't be removed as it's exposed to userspace, so it
> > then becomes a dumb encoder, without any operation implemented.
> >
> > Tested on DSI Panel, DSI Bridge, I2C-Configured DSI Bridge.
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>
> [...]
>
> > +static const struct component_ops sun6i_dsi_ops;
> > +
> >  static int sun6i_dsi_attach(struct mipi_dsi_host *host,
> >                           struct mipi_dsi_device *device)
> >  {
> >       struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> > -     struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);
>
> That one looks unrelated. Why do you need that change?

This was replaced with drmm_of_dsi_get_bridge for lookup of both panel
and bridge. I think I will separate this into another patch.

Thanks,
Jagan.

Re: [PATCH v7 12/12] drm: sun4: dsi: Convert to bridge driver

[Jagan Teki]

On Wed, Mar 29, 2023 at 8:29 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi,
>
> The patch prefix should be drm/sun4i:

I did follow my previous prefix, I will update this.

>
> On Wed, Mar 29, 2023 at 06:49:29PM +0530, Jagan Teki wrote:
> > Convert the encoder to bridge driver in order to standardize on a
> > single API by supporting all varients of downstream bridge devices.
>
> Which variant, and why do we need to convert to a bridge to support all of them?

Downstream bridge variants like DSI panel, DSI bridge and
I2C-Configured DSI bridges. Bridge conversion would be required for
the DSI host to access the more variety and complex downstream bridges
in a standardized bridge chain way which is indeed complex for encoder
driven DSI hosts.

>
> > The drm_encoder can't be removed as it's exposed to userspace, so it
> > then becomes a dumb encoder, without any operation implemented.
> >
> > Tested on DSI Panel, DSI Bridge, I2C-Configured DSI Bridge.
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>
> [...]
>
> > +static const struct component_ops sun6i_dsi_ops;
> > +
> >  static int sun6i_dsi_attach(struct mipi_dsi_host *host,
> >                           struct mipi_dsi_device *device)
> >  {
> >       struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> > -     struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);
>
> That one looks unrelated. Why do you need that change?

This was replaced with drmm_of_dsi_get_bridge for lookup of both panel
and bridge. I think I will separate this into another patch.

Thanks,
Jagan.

Re: [PATCH v7 12/12] drm: sun4: dsi: Convert to bridge driver

[Jagan Teki]

On Wed, Mar 29, 2023 at 8:29 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi,
>
> The patch prefix should be drm/sun4i:

I did follow my previous prefix, I will update this.

>
> On Wed, Mar 29, 2023 at 06:49:29PM +0530, Jagan Teki wrote:
> > Convert the encoder to bridge driver in order to standardize on a
> > single API by supporting all varients of downstream bridge devices.
>
> Which variant, and why do we need to convert to a bridge to support all of them?

Downstream bridge variants like DSI panel, DSI bridge and
I2C-Configured DSI bridges. Bridge conversion would be required for
the DSI host to access the more variety and complex downstream bridges
in a standardized bridge chain way which is indeed complex for encoder
driven DSI hosts.

>
> > The drm_encoder can't be removed as it's exposed to userspace, so it
> > then becomes a dumb encoder, without any operation implemented.
> >
> > Tested on DSI Panel, DSI Bridge, I2C-Configured DSI Bridge.
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>
> [...]
>
> > +static const struct component_ops sun6i_dsi_ops;
> > +
> >  static int sun6i_dsi_attach(struct mipi_dsi_host *host,
> >                           struct mipi_dsi_device *device)
> >  {
> >       struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> > -     struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);
>
> That one looks unrelated. Why do you need that change?

This was replaced with drmm_of_dsi_get_bridge for lookup of both panel
and bridge. I think I will separate this into another patch.

Thanks,
Jagan.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 12/12] drm: sun4: dsi: Convert to bridge driver

[Maxime Ripard]

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

On Wed, Mar 29, 2023 at 09:08:17PM +0530, Jagan Teki wrote:
> On Wed, Mar 29, 2023 at 8:29 PM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > Hi,
> >
> > The patch prefix should be drm/sun4i:
> 
> I did follow my previous prefix, I will update this.
> 
> >
> > On Wed, Mar 29, 2023 at 06:49:29PM +0530, Jagan Teki wrote:
> > > Convert the encoder to bridge driver in order to standardize on a
> > > single API by supporting all varients of downstream bridge devices.
> >
> > Which variant, and why do we need to convert to a bridge to support all of them?
> 
> Downstream bridge variants like DSI panel, DSI bridge and
> I2C-Configured DSI bridges. Bridge conversion would be required for
> the DSI host to access the more variety and complex downstream bridges
> in a standardized bridge chain way which is indeed complex for encoder
> driven DSI hosts.
> 
> >
> > > The drm_encoder can't be removed as it's exposed to userspace, so it
> > > then becomes a dumb encoder, without any operation implemented.
> > >
> > > Tested on DSI Panel, DSI Bridge, I2C-Configured DSI Bridge.
> > >
> > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> >
> > [...]
> >
> > > +static const struct component_ops sun6i_dsi_ops;
> > > +
> > >  static int sun6i_dsi_attach(struct mipi_dsi_host *host,
> > >                           struct mipi_dsi_device *device)
> > >  {
> > >       struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> > > -     struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);
> >
> > That one looks unrelated. Why do you need that change?
> 
> This was replaced with drmm_of_dsi_get_bridge for lookup of both panel
> and bridge. I think I will separate this into another patch.

So, it looks to me that you're doing two (unrelated) things in that patch:

  - You modify the existing driver to be a bridge

  - And you support downstream device being bridges.

Both are orthogonal, can (and should!) be done separately, and I'm
pretty sure you don't actually need to do the former at all.

Maxime

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

Re: [PATCH v7 12/12] drm: sun4: dsi: Convert to bridge driver

[Maxime Ripard]

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

On Wed, Mar 29, 2023 at 09:08:17PM +0530, Jagan Teki wrote:
> On Wed, Mar 29, 2023 at 8:29 PM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > Hi,
> >
> > The patch prefix should be drm/sun4i:
> 
> I did follow my previous prefix, I will update this.
> 
> >
> > On Wed, Mar 29, 2023 at 06:49:29PM +0530, Jagan Teki wrote:
> > > Convert the encoder to bridge driver in order to standardize on a
> > > single API by supporting all varients of downstream bridge devices.
> >
> > Which variant, and why do we need to convert to a bridge to support all of them?
> 
> Downstream bridge variants like DSI panel, DSI bridge and
> I2C-Configured DSI bridges. Bridge conversion would be required for
> the DSI host to access the more variety and complex downstream bridges
> in a standardized bridge chain way which is indeed complex for encoder
> driven DSI hosts.
> 
> >
> > > The drm_encoder can't be removed as it's exposed to userspace, so it
> > > then becomes a dumb encoder, without any operation implemented.
> > >
> > > Tested on DSI Panel, DSI Bridge, I2C-Configured DSI Bridge.
> > >
> > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> >
> > [...]
> >
> > > +static const struct component_ops sun6i_dsi_ops;
> > > +
> > >  static int sun6i_dsi_attach(struct mipi_dsi_host *host,
> > >                           struct mipi_dsi_device *device)
> > >  {
> > >       struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> > > -     struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);
> >
> > That one looks unrelated. Why do you need that change?
> 
> This was replaced with drmm_of_dsi_get_bridge for lookup of both panel
> and bridge. I think I will separate this into another patch.

So, it looks to me that you're doing two (unrelated) things in that patch:

  - You modify the existing driver to be a bridge

  - And you support downstream device being bridges.

Both are orthogonal, can (and should!) be done separately, and I'm
pretty sure you don't actually need to do the former at all.

Maxime

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

Re: [PATCH v7 12/12] drm: sun4: dsi: Convert to bridge driver

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 2081 bytes --]

On Wed, Mar 29, 2023 at 09:08:17PM +0530, Jagan Teki wrote:
> On Wed, Mar 29, 2023 at 8:29 PM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > Hi,
> >
> > The patch prefix should be drm/sun4i:
> 
> I did follow my previous prefix, I will update this.
> 
> >
> > On Wed, Mar 29, 2023 at 06:49:29PM +0530, Jagan Teki wrote:
> > > Convert the encoder to bridge driver in order to standardize on a
> > > single API by supporting all varients of downstream bridge devices.
> >
> > Which variant, and why do we need to convert to a bridge to support all of them?
> 
> Downstream bridge variants like DSI panel, DSI bridge and
> I2C-Configured DSI bridges. Bridge conversion would be required for
> the DSI host to access the more variety and complex downstream bridges
> in a standardized bridge chain way which is indeed complex for encoder
> driven DSI hosts.
> 
> >
> > > The drm_encoder can't be removed as it's exposed to userspace, so it
> > > then becomes a dumb encoder, without any operation implemented.
> > >
> > > Tested on DSI Panel, DSI Bridge, I2C-Configured DSI Bridge.
> > >
> > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> >
> > [...]
> >
> > > +static const struct component_ops sun6i_dsi_ops;
> > > +
> > >  static int sun6i_dsi_attach(struct mipi_dsi_host *host,
> > >                           struct mipi_dsi_device *device)
> > >  {
> > >       struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> > > -     struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);
> >
> > That one looks unrelated. Why do you need that change?
> 
> This was replaced with drmm_of_dsi_get_bridge for lookup of both panel
> and bridge. I think I will separate this into another patch.

So, it looks to me that you're doing two (unrelated) things in that patch:

  - You modify the existing driver to be a bridge

  - And you support downstream device being bridges.

Both are orthogonal, can (and should!) be done separately, and I'm
pretty sure you don't actually need to do the former at all.

Maxime

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 10/12] drm/bridge: Implement enable_next_first to alter bridge init order

[Dave Stevenson]

Hi Jagan

On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> DSI sink devices typically send the MIPI-DCS commands to the DSI host
> via general MIPI_DSI_DCS read and write API.
>
> The classical DSI sequence mentioned that the DSI host receives MIPI-DCS
> commands from the DSI sink first in order to switch HS mode properly.
> Once the DSI host switches to HS mode any MIPI-DCS commands from the
> DSI sink are unfunctional.

That statement contradicts the spec.
The DSI spec section 8.11.1 Transmission Packet Sequences says that
during any BLLP (Blanking or Low Power) period the host can do any of:
- remain in LP-11
- transmit one or more non-video packets from host to peripheral in escape mode
- transmit one or more non-video packets from host to peripheral in
using HS mode
- receive one or more packets from peripheral to host using escape mode
- transmit data on a different virtual channel.

Indeed if the sink doesn't set MIPI_DSI_MODE_LPM /
MIPI_DSI_MSG_USE_LPM, then the expectation is that any data transfer
will be in HS mode.

That makes me confused as to the need for this patch.

  Dave

> DSI sink uses the @enable function to send the MIPI-DCS commands. In a
> typical DSI host, sink pipeline the @enable call chain start with the
> DSI host, and then the DSI sink which is the "wrong" order as DSI host
> @enable is called and switched to HS mode before DSI sink @enable.
>
> If the DSI host enables with the @enable_next_first flag then the
> @enable for the DSI sink will be called first before the @enable of
> the DSI host. This alter bridge init order makes sure that the MIPI-DCS
> commands send first and then switch to the HS mode properly by DSI host.
>
> This new flag @enable_next_first that any bridg can set to swap the
> order of @enable (and #disable) for that and the immediately next bridge.
>
> [enable]
> If a bridge sets @enable_next_first, then the @enable for the next bridge
> will be called first before enable of this bridge.
>
> [disable]
> If a bridge sets @enable_next_first, then the @disable for the next bridge
> will be called first before @disable of this bridge to reverse the @enable
> calling direction.
>
> eg:
> - Panel
> - Bridge 1
> - Bridge 2 enable_next_first
> - Bridge 3
> - Bridge 4 enable_next_first
> - Bridge 5 enable_next_first
> - Bridge 6
> - Encoder
>
> Would result in enable's being called as Encoder, Bridge 6, Bridge 3,
> Bridge 4, Bridge 5, Bridge 1, Bridge 2, Panel.
>
> and the result in disable's being called as Panel, Bridge 2, Bridge 1,
> Bridge 5, Bridge 4, Bridge 3, Bridge 6, Encoder.
>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> Changes for v7:
> - new patch
>
>  drivers/gpu/drm/drm_bridge.c | 171 ++++++++++++++++++++++++++++++-----
>  include/drm/drm_bridge.h     |   8 ++
>  2 files changed, 154 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index caf0f341e524..cdc2669b3512 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -577,6 +577,24 @@ void drm_bridge_chain_mode_set(struct drm_bridge *bridge,
>  }
>  EXPORT_SYMBOL(drm_bridge_chain_mode_set);
>
> +static void drm_atomic_bridge_call_disable(struct drm_bridge *bridge,
> +                                          struct drm_atomic_state *old_state)
> +{
> +       if (old_state && bridge->funcs->atomic_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_disable(bridge, old_bridge_state);
> +       } else if (bridge->funcs->disable) {
> +               bridge->funcs->disable(bridge);
> +       }
> +}
> +
>  /**
>   * drm_atomic_bridge_chain_disable - disables all bridges in the encoder chain
>   * @bridge: bridge control structure
> @@ -587,33 +605,73 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set);
>   * starting from the last bridge to the first. These are called before calling
>   * &drm_encoder_helper_funcs.atomic_disable
>   *
> + * If a bridge sets @enable_next_first, then the @disable for the next bridge
> + * will be called first before @disable of this bridge to reverse the @enable
> + * calling direction.
> + *
> + * Example:
> + * Bridge A ---> Bridge B ---> Bridge C ---> Bridge D ---> Bridge E
> + *
> + * With enable_next_first flag enable in Bridge A, C, D then the resulting
> + * @disable order would be,
> + * Bridge C, Bridge D, Bridge E, Bridge A, Bridge B.
> + *
>   * Note: the bridge passed should be the one closest to the encoder
>   */
>  void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge,
>                                      struct drm_atomic_state *old_state)
>  {
>         struct drm_encoder *encoder;
> -       struct drm_bridge *iter;
> +       struct drm_bridge *iter, *next, *limit;
>
>         if (!bridge)
>                 return;
>
>         encoder = bridge->encoder;
> +
>         list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> -               if (iter->funcs->atomic_disable) {
> -                       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;
> -
> -                       iter->funcs->atomic_disable(iter, old_bridge_state);
> -               } else if (iter->funcs->disable) {
> -                       iter->funcs->disable(iter);
> +               limit = NULL;
> +
> +               if (!list_is_first(&iter->chain_node, &encoder->bridge_chain)) {
> +                       next = list_prev_entry(iter, chain_node);
> +
> +                       if (next->enable_next_first) {
> +                               limit = bridge;
> +                               list_for_each_entry_from_reverse(next,
> +                                                        &encoder->bridge_chain,
> +                                                        chain_node) {
> +                                       if (next == bridge)
> +                                               break;
> +
> +                                       if (!next->enable_next_first) {
> +                                               /* Found first bridge that does NOT
> +                                                * request next to be enabled first
> +                                                */
> +                                               next = list_next_entry(next, chain_node);
> +                                               limit = next;
> +                                               break;
> +                                       }
> +                               }
> +
> +                               list_for_each_entry_from(next, &encoder->bridge_chain, chain_node) {
> +                                       /* Call requested next bridge enable in order */
> +                                       if (next == iter)
> +                                               /* At the first bridge to request next
> +                                                * bridges called first.
> +                                                */
> +                                               break;
> +
> +                                       drm_atomic_bridge_call_disable(next, old_state);
> +                               }
> +                       }
>                 }
>
> +               drm_atomic_bridge_call_disable(iter, old_state);
> +
> +               if (limit)
> +                       /* Jump all bridges that we have already disabled */
> +                       iter = limit;
> +
>                 if (iter == bridge)
>                         break;
>         }
> @@ -822,6 +880,24 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
>  }
>  EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable);
>
> +static void drm_atomic_bridge_call_enable(struct drm_bridge *bridge,
> +                                         struct drm_atomic_state *old_state)
> +{
> +       if (old_state && bridge->funcs->atomic_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_enable(bridge, old_bridge_state);
> +       } else if (bridge->funcs->enable) {
> +               bridge->funcs->enable(bridge);
> +       }
> +}
> +
>  /**
>   * drm_atomic_bridge_chain_enable - enables all bridges in the encoder chain
>   * @bridge: bridge control structure
> @@ -832,31 +908,76 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable);
>   * starting from the first bridge to the last. These are called after completing
>   * &drm_encoder_helper_funcs.atomic_enable
>   *
> + * If a bridge sets @enable_next_first, then the @enable for the next bridge
> + * will be called first before enable of this bridge.
> + *
> + * Example:
> + * Bridge A ---> Bridge B ---> Bridge C ---> Bridge D ---> Bridge E
> + *
> + * With enable_next_first flag enable in Bridge A, C, D then the resulting
> + * @enable order would be,
> + * Bridge B, Bridge A, Bridge E, Bridge D, Bridge C.
> + *
>   * Note: the bridge passed should be the one closest to the encoder
>   */
>  void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge,
>                                     struct drm_atomic_state *old_state)
>  {
>         struct drm_encoder *encoder;
> +       struct drm_bridge *next, *limit;
>
>         if (!bridge)
>                 return;
>
>         encoder = bridge->encoder;
> +
>         list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> -               if (bridge->funcs->atomic_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_enable(bridge, old_bridge_state);
> -               } else if (bridge->funcs->enable) {
> -                       bridge->funcs->enable(bridge);
> +               limit = NULL;
> +
> +               if (!list_is_last(&bridge->chain_node, &encoder->bridge_chain)) {
> +                       if (bridge->enable_next_first) {
> +                               /* next bridge had requested that next
> +                                * was enabled first, so disabled last
> +                                */
> +                               next = list_next_entry(bridge, chain_node);
> +                               limit = next;
> +
> +                               list_for_each_entry_from(next, &encoder->bridge_chain,
> +                                                        chain_node) {
> +                                       /* Find the next bridge that has NOT requested
> +                                        * next to be enabled first / disabled last
> +                                        */
> +                                       if (!next->enable_next_first) {
> +                                               limit = next;
> +                                               break;
> +                                       }
> +
> +                                       /* Last bridge has requested next to be limit
> +                                        * otherwise the control move to end of chain
> +                                        */
> +                                       if (list_is_last(&next->chain_node,
> +                                                        &encoder->bridge_chain)) {
> +                                               limit = next;
> +                                               break;
> +                                       }
> +                               }
> +
> +                               /* Call these bridges in reverse order */
> +                               list_for_each_entry_from_reverse(next, &encoder->bridge_chain,
> +                                                                chain_node) {
> +                                       if (next == bridge)
> +                                               break;
> +
> +                                       drm_atomic_bridge_call_enable(next, old_state);
> +                               }
> +                       }
>                 }
> +
> +               drm_atomic_bridge_call_enable(bridge, old_state);
> +
> +               if (limit)
> +                       /* Jump all bridges that we have already enabled */
> +                       bridge = limit;
>         }
>  }
>  EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index a1a31704b917..9879129047e4 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -752,6 +752,14 @@ struct drm_bridge {
>          * before the peripheral.
>          */
>         bool pre_enable_prev_first;
> +       /**
> +        * @enable_next_first: The bridge requires that the next bridge @enable
> +        * function is called first before its @enable, and conversely for
> +        * @disable. This is most frequently a requirement for a DSI host to
> +        * receive MIPI-DCS commands from DSI sink first in order to switch
> +        * HS mode properly.
> +        */
> +       bool enable_next_first;
>         /**
>          * @ddc: Associated I2C adapter for DDC access, if any.
>          */
> --
> 2.25.1
>

Re: [PATCH v7 10/12] drm/bridge: Implement enable_next_first to alter bridge init order

[Dave Stevenson]

Hi Jagan

On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> DSI sink devices typically send the MIPI-DCS commands to the DSI host
> via general MIPI_DSI_DCS read and write API.
>
> The classical DSI sequence mentioned that the DSI host receives MIPI-DCS
> commands from the DSI sink first in order to switch HS mode properly.
> Once the DSI host switches to HS mode any MIPI-DCS commands from the
> DSI sink are unfunctional.

That statement contradicts the spec.
The DSI spec section 8.11.1 Transmission Packet Sequences says that
during any BLLP (Blanking or Low Power) period the host can do any of:
- remain in LP-11
- transmit one or more non-video packets from host to peripheral in escape mode
- transmit one or more non-video packets from host to peripheral in
using HS mode
- receive one or more packets from peripheral to host using escape mode
- transmit data on a different virtual channel.

Indeed if the sink doesn't set MIPI_DSI_MODE_LPM /
MIPI_DSI_MSG_USE_LPM, then the expectation is that any data transfer
will be in HS mode.

That makes me confused as to the need for this patch.

  Dave

> DSI sink uses the @enable function to send the MIPI-DCS commands. In a
> typical DSI host, sink pipeline the @enable call chain start with the
> DSI host, and then the DSI sink which is the "wrong" order as DSI host
> @enable is called and switched to HS mode before DSI sink @enable.
>
> If the DSI host enables with the @enable_next_first flag then the
> @enable for the DSI sink will be called first before the @enable of
> the DSI host. This alter bridge init order makes sure that the MIPI-DCS
> commands send first and then switch to the HS mode properly by DSI host.
>
> This new flag @enable_next_first that any bridg can set to swap the
> order of @enable (and #disable) for that and the immediately next bridge.
>
> [enable]
> If a bridge sets @enable_next_first, then the @enable for the next bridge
> will be called first before enable of this bridge.
>
> [disable]
> If a bridge sets @enable_next_first, then the @disable for the next bridge
> will be called first before @disable of this bridge to reverse the @enable
> calling direction.
>
> eg:
> - Panel
> - Bridge 1
> - Bridge 2 enable_next_first
> - Bridge 3
> - Bridge 4 enable_next_first
> - Bridge 5 enable_next_first
> - Bridge 6
> - Encoder
>
> Would result in enable's being called as Encoder, Bridge 6, Bridge 3,
> Bridge 4, Bridge 5, Bridge 1, Bridge 2, Panel.
>
> and the result in disable's being called as Panel, Bridge 2, Bridge 1,
> Bridge 5, Bridge 4, Bridge 3, Bridge 6, Encoder.
>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> Changes for v7:
> - new patch
>
>  drivers/gpu/drm/drm_bridge.c | 171 ++++++++++++++++++++++++++++++-----
>  include/drm/drm_bridge.h     |   8 ++
>  2 files changed, 154 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index caf0f341e524..cdc2669b3512 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -577,6 +577,24 @@ void drm_bridge_chain_mode_set(struct drm_bridge *bridge,
>  }
>  EXPORT_SYMBOL(drm_bridge_chain_mode_set);
>
> +static void drm_atomic_bridge_call_disable(struct drm_bridge *bridge,
> +                                          struct drm_atomic_state *old_state)
> +{
> +       if (old_state && bridge->funcs->atomic_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_disable(bridge, old_bridge_state);
> +       } else if (bridge->funcs->disable) {
> +               bridge->funcs->disable(bridge);
> +       }
> +}
> +
>  /**
>   * drm_atomic_bridge_chain_disable - disables all bridges in the encoder chain
>   * @bridge: bridge control structure
> @@ -587,33 +605,73 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set);
>   * starting from the last bridge to the first. These are called before calling
>   * &drm_encoder_helper_funcs.atomic_disable
>   *
> + * If a bridge sets @enable_next_first, then the @disable for the next bridge
> + * will be called first before @disable of this bridge to reverse the @enable
> + * calling direction.
> + *
> + * Example:
> + * Bridge A ---> Bridge B ---> Bridge C ---> Bridge D ---> Bridge E
> + *
> + * With enable_next_first flag enable in Bridge A, C, D then the resulting
> + * @disable order would be,
> + * Bridge C, Bridge D, Bridge E, Bridge A, Bridge B.
> + *
>   * Note: the bridge passed should be the one closest to the encoder
>   */
>  void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge,
>                                      struct drm_atomic_state *old_state)
>  {
>         struct drm_encoder *encoder;
> -       struct drm_bridge *iter;
> +       struct drm_bridge *iter, *next, *limit;
>
>         if (!bridge)
>                 return;
>
>         encoder = bridge->encoder;
> +
>         list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> -               if (iter->funcs->atomic_disable) {
> -                       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;
> -
> -                       iter->funcs->atomic_disable(iter, old_bridge_state);
> -               } else if (iter->funcs->disable) {
> -                       iter->funcs->disable(iter);
> +               limit = NULL;
> +
> +               if (!list_is_first(&iter->chain_node, &encoder->bridge_chain)) {
> +                       next = list_prev_entry(iter, chain_node);
> +
> +                       if (next->enable_next_first) {
> +                               limit = bridge;
> +                               list_for_each_entry_from_reverse(next,
> +                                                        &encoder->bridge_chain,
> +                                                        chain_node) {
> +                                       if (next == bridge)
> +                                               break;
> +
> +                                       if (!next->enable_next_first) {
> +                                               /* Found first bridge that does NOT
> +                                                * request next to be enabled first
> +                                                */
> +                                               next = list_next_entry(next, chain_node);
> +                                               limit = next;
> +                                               break;
> +                                       }
> +                               }
> +
> +                               list_for_each_entry_from(next, &encoder->bridge_chain, chain_node) {
> +                                       /* Call requested next bridge enable in order */
> +                                       if (next == iter)
> +                                               /* At the first bridge to request next
> +                                                * bridges called first.
> +                                                */
> +                                               break;
> +
> +                                       drm_atomic_bridge_call_disable(next, old_state);
> +                               }
> +                       }
>                 }
>
> +               drm_atomic_bridge_call_disable(iter, old_state);
> +
> +               if (limit)
> +                       /* Jump all bridges that we have already disabled */
> +                       iter = limit;
> +
>                 if (iter == bridge)
>                         break;
>         }
> @@ -822,6 +880,24 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
>  }
>  EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable);
>
> +static void drm_atomic_bridge_call_enable(struct drm_bridge *bridge,
> +                                         struct drm_atomic_state *old_state)
> +{
> +       if (old_state && bridge->funcs->atomic_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_enable(bridge, old_bridge_state);
> +       } else if (bridge->funcs->enable) {
> +               bridge->funcs->enable(bridge);
> +       }
> +}
> +
>  /**
>   * drm_atomic_bridge_chain_enable - enables all bridges in the encoder chain
>   * @bridge: bridge control structure
> @@ -832,31 +908,76 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable);
>   * starting from the first bridge to the last. These are called after completing
>   * &drm_encoder_helper_funcs.atomic_enable
>   *
> + * If a bridge sets @enable_next_first, then the @enable for the next bridge
> + * will be called first before enable of this bridge.
> + *
> + * Example:
> + * Bridge A ---> Bridge B ---> Bridge C ---> Bridge D ---> Bridge E
> + *
> + * With enable_next_first flag enable in Bridge A, C, D then the resulting
> + * @enable order would be,
> + * Bridge B, Bridge A, Bridge E, Bridge D, Bridge C.
> + *
>   * Note: the bridge passed should be the one closest to the encoder
>   */
>  void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge,
>                                     struct drm_atomic_state *old_state)
>  {
>         struct drm_encoder *encoder;
> +       struct drm_bridge *next, *limit;
>
>         if (!bridge)
>                 return;
>
>         encoder = bridge->encoder;
> +
>         list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> -               if (bridge->funcs->atomic_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_enable(bridge, old_bridge_state);
> -               } else if (bridge->funcs->enable) {
> -                       bridge->funcs->enable(bridge);
> +               limit = NULL;
> +
> +               if (!list_is_last(&bridge->chain_node, &encoder->bridge_chain)) {
> +                       if (bridge->enable_next_first) {
> +                               /* next bridge had requested that next
> +                                * was enabled first, so disabled last
> +                                */
> +                               next = list_next_entry(bridge, chain_node);
> +                               limit = next;
> +
> +                               list_for_each_entry_from(next, &encoder->bridge_chain,
> +                                                        chain_node) {
> +                                       /* Find the next bridge that has NOT requested
> +                                        * next to be enabled first / disabled last
> +                                        */
> +                                       if (!next->enable_next_first) {
> +                                               limit = next;
> +                                               break;
> +                                       }
> +
> +                                       /* Last bridge has requested next to be limit
> +                                        * otherwise the control move to end of chain
> +                                        */
> +                                       if (list_is_last(&next->chain_node,
> +                                                        &encoder->bridge_chain)) {
> +                                               limit = next;
> +                                               break;
> +                                       }
> +                               }
> +
> +                               /* Call these bridges in reverse order */
> +                               list_for_each_entry_from_reverse(next, &encoder->bridge_chain,
> +                                                                chain_node) {
> +                                       if (next == bridge)
> +                                               break;
> +
> +                                       drm_atomic_bridge_call_enable(next, old_state);
> +                               }
> +                       }
>                 }
> +
> +               drm_atomic_bridge_call_enable(bridge, old_state);
> +
> +               if (limit)
> +                       /* Jump all bridges that we have already enabled */
> +                       bridge = limit;
>         }
>  }
>  EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index a1a31704b917..9879129047e4 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -752,6 +752,14 @@ struct drm_bridge {
>          * before the peripheral.
>          */
>         bool pre_enable_prev_first;
> +       /**
> +        * @enable_next_first: The bridge requires that the next bridge @enable
> +        * function is called first before its @enable, and conversely for
> +        * @disable. This is most frequently a requirement for a DSI host to
> +        * receive MIPI-DCS commands from DSI sink first in order to switch
> +        * HS mode properly.
> +        */
> +       bool enable_next_first;
>         /**
>          * @ddc: Associated I2C adapter for DDC access, if any.
>          */
> --
> 2.25.1
>

Re: [PATCH v7 10/12] drm/bridge: Implement enable_next_first to alter bridge init order

[Dave Stevenson]

Hi Jagan

On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> DSI sink devices typically send the MIPI-DCS commands to the DSI host
> via general MIPI_DSI_DCS read and write API.
>
> The classical DSI sequence mentioned that the DSI host receives MIPI-DCS
> commands from the DSI sink first in order to switch HS mode properly.
> Once the DSI host switches to HS mode any MIPI-DCS commands from the
> DSI sink are unfunctional.

That statement contradicts the spec.
The DSI spec section 8.11.1 Transmission Packet Sequences says that
during any BLLP (Blanking or Low Power) period the host can do any of:
- remain in LP-11
- transmit one or more non-video packets from host to peripheral in escape mode
- transmit one or more non-video packets from host to peripheral in
using HS mode
- receive one or more packets from peripheral to host using escape mode
- transmit data on a different virtual channel.

Indeed if the sink doesn't set MIPI_DSI_MODE_LPM /
MIPI_DSI_MSG_USE_LPM, then the expectation is that any data transfer
will be in HS mode.

That makes me confused as to the need for this patch.

  Dave

> DSI sink uses the @enable function to send the MIPI-DCS commands. In a
> typical DSI host, sink pipeline the @enable call chain start with the
> DSI host, and then the DSI sink which is the "wrong" order as DSI host
> @enable is called and switched to HS mode before DSI sink @enable.
>
> If the DSI host enables with the @enable_next_first flag then the
> @enable for the DSI sink will be called first before the @enable of
> the DSI host. This alter bridge init order makes sure that the MIPI-DCS
> commands send first and then switch to the HS mode properly by DSI host.
>
> This new flag @enable_next_first that any bridg can set to swap the
> order of @enable (and #disable) for that and the immediately next bridge.
>
> [enable]
> If a bridge sets @enable_next_first, then the @enable for the next bridge
> will be called first before enable of this bridge.
>
> [disable]
> If a bridge sets @enable_next_first, then the @disable for the next bridge
> will be called first before @disable of this bridge to reverse the @enable
> calling direction.
>
> eg:
> - Panel
> - Bridge 1
> - Bridge 2 enable_next_first
> - Bridge 3
> - Bridge 4 enable_next_first
> - Bridge 5 enable_next_first
> - Bridge 6
> - Encoder
>
> Would result in enable's being called as Encoder, Bridge 6, Bridge 3,
> Bridge 4, Bridge 5, Bridge 1, Bridge 2, Panel.
>
> and the result in disable's being called as Panel, Bridge 2, Bridge 1,
> Bridge 5, Bridge 4, Bridge 3, Bridge 6, Encoder.
>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> Changes for v7:
> - new patch
>
>  drivers/gpu/drm/drm_bridge.c | 171 ++++++++++++++++++++++++++++++-----
>  include/drm/drm_bridge.h     |   8 ++
>  2 files changed, 154 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index caf0f341e524..cdc2669b3512 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -577,6 +577,24 @@ void drm_bridge_chain_mode_set(struct drm_bridge *bridge,
>  }
>  EXPORT_SYMBOL(drm_bridge_chain_mode_set);
>
> +static void drm_atomic_bridge_call_disable(struct drm_bridge *bridge,
> +                                          struct drm_atomic_state *old_state)
> +{
> +       if (old_state && bridge->funcs->atomic_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_disable(bridge, old_bridge_state);
> +       } else if (bridge->funcs->disable) {
> +               bridge->funcs->disable(bridge);
> +       }
> +}
> +
>  /**
>   * drm_atomic_bridge_chain_disable - disables all bridges in the encoder chain
>   * @bridge: bridge control structure
> @@ -587,33 +605,73 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set);
>   * starting from the last bridge to the first. These are called before calling
>   * &drm_encoder_helper_funcs.atomic_disable
>   *
> + * If a bridge sets @enable_next_first, then the @disable for the next bridge
> + * will be called first before @disable of this bridge to reverse the @enable
> + * calling direction.
> + *
> + * Example:
> + * Bridge A ---> Bridge B ---> Bridge C ---> Bridge D ---> Bridge E
> + *
> + * With enable_next_first flag enable in Bridge A, C, D then the resulting
> + * @disable order would be,
> + * Bridge C, Bridge D, Bridge E, Bridge A, Bridge B.
> + *
>   * Note: the bridge passed should be the one closest to the encoder
>   */
>  void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge,
>                                      struct drm_atomic_state *old_state)
>  {
>         struct drm_encoder *encoder;
> -       struct drm_bridge *iter;
> +       struct drm_bridge *iter, *next, *limit;
>
>         if (!bridge)
>                 return;
>
>         encoder = bridge->encoder;
> +
>         list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> -               if (iter->funcs->atomic_disable) {
> -                       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;
> -
> -                       iter->funcs->atomic_disable(iter, old_bridge_state);
> -               } else if (iter->funcs->disable) {
> -                       iter->funcs->disable(iter);
> +               limit = NULL;
> +
> +               if (!list_is_first(&iter->chain_node, &encoder->bridge_chain)) {
> +                       next = list_prev_entry(iter, chain_node);
> +
> +                       if (next->enable_next_first) {
> +                               limit = bridge;
> +                               list_for_each_entry_from_reverse(next,
> +                                                        &encoder->bridge_chain,
> +                                                        chain_node) {
> +                                       if (next == bridge)
> +                                               break;
> +
> +                                       if (!next->enable_next_first) {
> +                                               /* Found first bridge that does NOT
> +                                                * request next to be enabled first
> +                                                */
> +                                               next = list_next_entry(next, chain_node);
> +                                               limit = next;
> +                                               break;
> +                                       }
> +                               }
> +
> +                               list_for_each_entry_from(next, &encoder->bridge_chain, chain_node) {
> +                                       /* Call requested next bridge enable in order */
> +                                       if (next == iter)
> +                                               /* At the first bridge to request next
> +                                                * bridges called first.
> +                                                */
> +                                               break;
> +
> +                                       drm_atomic_bridge_call_disable(next, old_state);
> +                               }
> +                       }
>                 }
>
> +               drm_atomic_bridge_call_disable(iter, old_state);
> +
> +               if (limit)
> +                       /* Jump all bridges that we have already disabled */
> +                       iter = limit;
> +
>                 if (iter == bridge)
>                         break;
>         }
> @@ -822,6 +880,24 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
>  }
>  EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable);
>
> +static void drm_atomic_bridge_call_enable(struct drm_bridge *bridge,
> +                                         struct drm_atomic_state *old_state)
> +{
> +       if (old_state && bridge->funcs->atomic_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_enable(bridge, old_bridge_state);
> +       } else if (bridge->funcs->enable) {
> +               bridge->funcs->enable(bridge);
> +       }
> +}
> +
>  /**
>   * drm_atomic_bridge_chain_enable - enables all bridges in the encoder chain
>   * @bridge: bridge control structure
> @@ -832,31 +908,76 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable);
>   * starting from the first bridge to the last. These are called after completing
>   * &drm_encoder_helper_funcs.atomic_enable
>   *
> + * If a bridge sets @enable_next_first, then the @enable for the next bridge
> + * will be called first before enable of this bridge.
> + *
> + * Example:
> + * Bridge A ---> Bridge B ---> Bridge C ---> Bridge D ---> Bridge E
> + *
> + * With enable_next_first flag enable in Bridge A, C, D then the resulting
> + * @enable order would be,
> + * Bridge B, Bridge A, Bridge E, Bridge D, Bridge C.
> + *
>   * Note: the bridge passed should be the one closest to the encoder
>   */
>  void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge,
>                                     struct drm_atomic_state *old_state)
>  {
>         struct drm_encoder *encoder;
> +       struct drm_bridge *next, *limit;
>
>         if (!bridge)
>                 return;
>
>         encoder = bridge->encoder;
> +
>         list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> -               if (bridge->funcs->atomic_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_enable(bridge, old_bridge_state);
> -               } else if (bridge->funcs->enable) {
> -                       bridge->funcs->enable(bridge);
> +               limit = NULL;
> +
> +               if (!list_is_last(&bridge->chain_node, &encoder->bridge_chain)) {
> +                       if (bridge->enable_next_first) {
> +                               /* next bridge had requested that next
> +                                * was enabled first, so disabled last
> +                                */
> +                               next = list_next_entry(bridge, chain_node);
> +                               limit = next;
> +
> +                               list_for_each_entry_from(next, &encoder->bridge_chain,
> +                                                        chain_node) {
> +                                       /* Find the next bridge that has NOT requested
> +                                        * next to be enabled first / disabled last
> +                                        */
> +                                       if (!next->enable_next_first) {
> +                                               limit = next;
> +                                               break;
> +                                       }
> +
> +                                       /* Last bridge has requested next to be limit
> +                                        * otherwise the control move to end of chain
> +                                        */
> +                                       if (list_is_last(&next->chain_node,
> +                                                        &encoder->bridge_chain)) {
> +                                               limit = next;
> +                                               break;
> +                                       }
> +                               }
> +
> +                               /* Call these bridges in reverse order */
> +                               list_for_each_entry_from_reverse(next, &encoder->bridge_chain,
> +                                                                chain_node) {
> +                                       if (next == bridge)
> +                                               break;
> +
> +                                       drm_atomic_bridge_call_enable(next, old_state);
> +                               }
> +                       }
>                 }
> +
> +               drm_atomic_bridge_call_enable(bridge, old_state);
> +
> +               if (limit)
> +                       /* Jump all bridges that we have already enabled */
> +                       bridge = limit;
>         }
>  }
>  EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index a1a31704b917..9879129047e4 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -752,6 +752,14 @@ struct drm_bridge {
>          * before the peripheral.
>          */
>         bool pre_enable_prev_first;
> +       /**
> +        * @enable_next_first: The bridge requires that the next bridge @enable
> +        * function is called first before its @enable, and conversely for
> +        * @disable. This is most frequently a requirement for a DSI host to
> +        * receive MIPI-DCS commands from DSI sink first in order to switch
> +        * HS mode properly.
> +        */
> +       bool enable_next_first;
>         /**
>          * @ddc: Associated I2C adapter for DDC access, if any.
>          */
> --
> 2.25.1
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 10/12] drm/bridge: Implement enable_next_first to alter bridge init order

[Maxime Ripard]

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

On Wed, Mar 29, 2023 at 05:28:28PM +0100, Dave Stevenson wrote:
> On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > DSI sink devices typically send the MIPI-DCS commands to the DSI host
> > via general MIPI_DSI_DCS read and write API.
> >
> > The classical DSI sequence mentioned that the DSI host receives MIPI-DCS
> > commands from the DSI sink first in order to switch HS mode properly.
> > Once the DSI host switches to HS mode any MIPI-DCS commands from the
> > DSI sink are unfunctional.
> 
> That statement contradicts the spec.
> The DSI spec section 8.11.1 Transmission Packet Sequences says that
> during any BLLP (Blanking or Low Power) period the host can do any of:
> - remain in LP-11
> - transmit one or more non-video packets from host to peripheral in escape mode
> - transmit one or more non-video packets from host to peripheral in
> using HS mode
> - receive one or more packets from peripheral to host using escape mode
> - transmit data on a different virtual channel.
> 
> Indeed if the sink doesn't set MIPI_DSI_MODE_LPM /
> MIPI_DSI_MSG_USE_LPM, then the expectation is that any data transfer
> will be in HS mode.
> 
> That makes me confused as to the need for this patch.

Yeah, and it looks like that would break the expectation that, in
enable, a bridge can expect its controller to be in HS mode.

I think that was Jagan is trying to do is to work around an issue with
the Allwinner DSI driver:
https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L775

This is working mostly fine since we only have panel support and can
control that, but with bridge support added in the latest patch, then it
probably doesn't work anymore.

The proper way to fix this isn't to put more logic into the framework,
it's to make the DSI driver behave as expected by KMS.

Unfortunately, that controller is not documented, so it's not clear to
me how we can fix it.

IIRC, it's basically a state machine where you would encode the
transitions between one DSI state and the next depending on what your
expectations are.

I think there's two problem with the driver that need to be addressed:

  - First the driver will drop back to LP11 mode to submit commands. I
    don't think it's needed and could even be hurtful to the video
    stream if it was to happen during HS mode:
    https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L877

  - And then, it looks like, in HSD mode, we never get to go to the
    state LPTX is in (LPDT). It would be interesting to test whether
    adding a transition to that state makes it work or not.

Maxime

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

Re: [PATCH v7 10/12] drm/bridge: Implement enable_next_first to alter bridge init order

[Maxime Ripard]

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

On Wed, Mar 29, 2023 at 05:28:28PM +0100, Dave Stevenson wrote:
> On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > DSI sink devices typically send the MIPI-DCS commands to the DSI host
> > via general MIPI_DSI_DCS read and write API.
> >
> > The classical DSI sequence mentioned that the DSI host receives MIPI-DCS
> > commands from the DSI sink first in order to switch HS mode properly.
> > Once the DSI host switches to HS mode any MIPI-DCS commands from the
> > DSI sink are unfunctional.
> 
> That statement contradicts the spec.
> The DSI spec section 8.11.1 Transmission Packet Sequences says that
> during any BLLP (Blanking or Low Power) period the host can do any of:
> - remain in LP-11
> - transmit one or more non-video packets from host to peripheral in escape mode
> - transmit one or more non-video packets from host to peripheral in
> using HS mode
> - receive one or more packets from peripheral to host using escape mode
> - transmit data on a different virtual channel.
> 
> Indeed if the sink doesn't set MIPI_DSI_MODE_LPM /
> MIPI_DSI_MSG_USE_LPM, then the expectation is that any data transfer
> will be in HS mode.
> 
> That makes me confused as to the need for this patch.

Yeah, and it looks like that would break the expectation that, in
enable, a bridge can expect its controller to be in HS mode.

I think that was Jagan is trying to do is to work around an issue with
the Allwinner DSI driver:
https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L775

This is working mostly fine since we only have panel support and can
control that, but with bridge support added in the latest patch, then it
probably doesn't work anymore.

The proper way to fix this isn't to put more logic into the framework,
it's to make the DSI driver behave as expected by KMS.

Unfortunately, that controller is not documented, so it's not clear to
me how we can fix it.

IIRC, it's basically a state machine where you would encode the
transitions between one DSI state and the next depending on what your
expectations are.

I think there's two problem with the driver that need to be addressed:

  - First the driver will drop back to LP11 mode to submit commands. I
    don't think it's needed and could even be hurtful to the video
    stream if it was to happen during HS mode:
    https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L877

  - And then, it looks like, in HSD mode, we never get to go to the
    state LPTX is in (LPDT). It would be interesting to test whether
    adding a transition to that state makes it work or not.

Maxime

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

Re: [PATCH v7 10/12] drm/bridge: Implement enable_next_first to alter bridge init order

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 2722 bytes --]

On Wed, Mar 29, 2023 at 05:28:28PM +0100, Dave Stevenson wrote:
> On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > DSI sink devices typically send the MIPI-DCS commands to the DSI host
> > via general MIPI_DSI_DCS read and write API.
> >
> > The classical DSI sequence mentioned that the DSI host receives MIPI-DCS
> > commands from the DSI sink first in order to switch HS mode properly.
> > Once the DSI host switches to HS mode any MIPI-DCS commands from the
> > DSI sink are unfunctional.
> 
> That statement contradicts the spec.
> The DSI spec section 8.11.1 Transmission Packet Sequences says that
> during any BLLP (Blanking or Low Power) period the host can do any of:
> - remain in LP-11
> - transmit one or more non-video packets from host to peripheral in escape mode
> - transmit one or more non-video packets from host to peripheral in
> using HS mode
> - receive one or more packets from peripheral to host using escape mode
> - transmit data on a different virtual channel.
> 
> Indeed if the sink doesn't set MIPI_DSI_MODE_LPM /
> MIPI_DSI_MSG_USE_LPM, then the expectation is that any data transfer
> will be in HS mode.
> 
> That makes me confused as to the need for this patch.

Yeah, and it looks like that would break the expectation that, in
enable, a bridge can expect its controller to be in HS mode.

I think that was Jagan is trying to do is to work around an issue with
the Allwinner DSI driver:
https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L775

This is working mostly fine since we only have panel support and can
control that, but with bridge support added in the latest patch, then it
probably doesn't work anymore.

The proper way to fix this isn't to put more logic into the framework,
it's to make the DSI driver behave as expected by KMS.

Unfortunately, that controller is not documented, so it's not clear to
me how we can fix it.

IIRC, it's basically a state machine where you would encode the
transitions between one DSI state and the next depending on what your
expectations are.

I think there's two problem with the driver that need to be addressed:

  - First the driver will drop back to LP11 mode to submit commands. I
    don't think it's needed and could even be hurtful to the video
    stream if it was to happen during HS mode:
    https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L877

  - And then, it looks like, in HSD mode, we never get to go to the
    state LPTX is in (LPDT). It would be interesting to test whether
    adding a transition to that state makes it work or not.

Maxime

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 10/12] drm/bridge: Implement enable_next_first to alter bridge init order

[Dave Stevenson]

Hi Maxime

On Wed, 29 Mar 2023 at 17:46, Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Wed, Mar 29, 2023 at 05:28:28PM +0100, Dave Stevenson wrote:
> > On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@amarulasolutions.com> wrote:
> > >
> > > DSI sink devices typically send the MIPI-DCS commands to the DSI host
> > > via general MIPI_DSI_DCS read and write API.
> > >
> > > The classical DSI sequence mentioned that the DSI host receives MIPI-DCS
> > > commands from the DSI sink first in order to switch HS mode properly.
> > > Once the DSI host switches to HS mode any MIPI-DCS commands from the
> > > DSI sink are unfunctional.
> >
> > That statement contradicts the spec.
> > The DSI spec section 8.11.1 Transmission Packet Sequences says that
> > during any BLLP (Blanking or Low Power) period the host can do any of:
> > - remain in LP-11
> > - transmit one or more non-video packets from host to peripheral in escape mode
> > - transmit one or more non-video packets from host to peripheral in
> > using HS mode
> > - receive one or more packets from peripheral to host using escape mode
> > - transmit data on a different virtual channel.
> >
> > Indeed if the sink doesn't set MIPI_DSI_MODE_LPM /
> > MIPI_DSI_MSG_USE_LPM, then the expectation is that any data transfer
> > will be in HS mode.
> >
> > That makes me confused as to the need for this patch.
>
> Yeah, and it looks like that would break the expectation that, in
> enable, a bridge can expect its controller to be in HS mode.
>
> I think that was Jagan is trying to do is to work around an issue with
> the Allwinner DSI driver:
> https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L775
>
> This is working mostly fine since we only have panel support and can
> control that, but with bridge support added in the latest patch, then it
> probably doesn't work anymore.
>
> The proper way to fix this isn't to put more logic into the framework,
> it's to make the DSI driver behave as expected by KMS.
>
> Unfortunately, that controller is not documented, so it's not clear to
> me how we can fix it.
>
> IIRC, it's basically a state machine where you would encode the
> transitions between one DSI state and the next depending on what your
> expectations are.
>
> I think there's two problem with the driver that need to be addressed:
>
>   - First the driver will drop back to LP11 mode to submit commands. I
>     don't think it's needed and could even be hurtful to the video
>     stream if it was to happen during HS mode:
>     https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L877
>
>   - And then, it looks like, in HSD mode, we never get to go to the
>     state LPTX is in (LPDT). It would be interesting to test whether
>     adding a transition to that state makes it work or not.

Ooh, not fun.
I'll agree with your assessment - it looks like sunxi driver has
significant limitations on the modes of operation it supports. If
there is no information on sending HS commands, I wonder if it's
possible to note the video state in transfer and stop video, send the
command, and resume video again. Ugly as heck, but possibly the only
real option without documentation. It does raise the question of do
other blocks (eg crtc) need to be stopped as well, or does stopping
the PHY and/or DSI block stop the pixel data getting clocked out.

I can only guess at the meaning of the enum sun6i_dsi_start_inst and
enum sun6i_dsi_inst_id states. LPTX and LPRX are largely obvious, but
HSC(ommand) and HSD(ata) perhaps?
I thought on initial reading that the setup in sun6i_dsi_start made
sense as a sequence of commands, but looking closer at the bitmasking
and shifting I'm not so convinced. Are the DSI_INST_ID_xxx defines
shifts or the bitmask values to or in, as they get used for both.

  Dave

Re: [PATCH v7 10/12] drm/bridge: Implement enable_next_first to alter bridge init order

[Dave Stevenson]

Hi Maxime

On Wed, 29 Mar 2023 at 17:46, Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Wed, Mar 29, 2023 at 05:28:28PM +0100, Dave Stevenson wrote:
> > On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@amarulasolutions.com> wrote:
> > >
> > > DSI sink devices typically send the MIPI-DCS commands to the DSI host
> > > via general MIPI_DSI_DCS read and write API.
> > >
> > > The classical DSI sequence mentioned that the DSI host receives MIPI-DCS
> > > commands from the DSI sink first in order to switch HS mode properly.
> > > Once the DSI host switches to HS mode any MIPI-DCS commands from the
> > > DSI sink are unfunctional.
> >
> > That statement contradicts the spec.
> > The DSI spec section 8.11.1 Transmission Packet Sequences says that
> > during any BLLP (Blanking or Low Power) period the host can do any of:
> > - remain in LP-11
> > - transmit one or more non-video packets from host to peripheral in escape mode
> > - transmit one or more non-video packets from host to peripheral in
> > using HS mode
> > - receive one or more packets from peripheral to host using escape mode
> > - transmit data on a different virtual channel.
> >
> > Indeed if the sink doesn't set MIPI_DSI_MODE_LPM /
> > MIPI_DSI_MSG_USE_LPM, then the expectation is that any data transfer
> > will be in HS mode.
> >
> > That makes me confused as to the need for this patch.
>
> Yeah, and it looks like that would break the expectation that, in
> enable, a bridge can expect its controller to be in HS mode.
>
> I think that was Jagan is trying to do is to work around an issue with
> the Allwinner DSI driver:
> https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L775
>
> This is working mostly fine since we only have panel support and can
> control that, but with bridge support added in the latest patch, then it
> probably doesn't work anymore.
>
> The proper way to fix this isn't to put more logic into the framework,
> it's to make the DSI driver behave as expected by KMS.
>
> Unfortunately, that controller is not documented, so it's not clear to
> me how we can fix it.
>
> IIRC, it's basically a state machine where you would encode the
> transitions between one DSI state and the next depending on what your
> expectations are.
>
> I think there's two problem with the driver that need to be addressed:
>
>   - First the driver will drop back to LP11 mode to submit commands. I
>     don't think it's needed and could even be hurtful to the video
>     stream if it was to happen during HS mode:
>     https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L877
>
>   - And then, it looks like, in HSD mode, we never get to go to the
>     state LPTX is in (LPDT). It would be interesting to test whether
>     adding a transition to that state makes it work or not.

Ooh, not fun.
I'll agree with your assessment - it looks like sunxi driver has
significant limitations on the modes of operation it supports. If
there is no information on sending HS commands, I wonder if it's
possible to note the video state in transfer and stop video, send the
command, and resume video again. Ugly as heck, but possibly the only
real option without documentation. It does raise the question of do
other blocks (eg crtc) need to be stopped as well, or does stopping
the PHY and/or DSI block stop the pixel data getting clocked out.

I can only guess at the meaning of the enum sun6i_dsi_start_inst and
enum sun6i_dsi_inst_id states. LPTX and LPRX are largely obvious, but
HSC(ommand) and HSD(ata) perhaps?
I thought on initial reading that the setup in sun6i_dsi_start made
sense as a sequence of commands, but looking closer at the bitmasking
and shifting I'm not so convinced. Are the DSI_INST_ID_xxx defines
shifts or the bitmask values to or in, as they get used for both.

  Dave

Re: [PATCH v7 10/12] drm/bridge: Implement enable_next_first to alter bridge init order

[Dave Stevenson]

Hi Maxime

On Wed, 29 Mar 2023 at 17:46, Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Wed, Mar 29, 2023 at 05:28:28PM +0100, Dave Stevenson wrote:
> > On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@amarulasolutions.com> wrote:
> > >
> > > DSI sink devices typically send the MIPI-DCS commands to the DSI host
> > > via general MIPI_DSI_DCS read and write API.
> > >
> > > The classical DSI sequence mentioned that the DSI host receives MIPI-DCS
> > > commands from the DSI sink first in order to switch HS mode properly.
> > > Once the DSI host switches to HS mode any MIPI-DCS commands from the
> > > DSI sink are unfunctional.
> >
> > That statement contradicts the spec.
> > The DSI spec section 8.11.1 Transmission Packet Sequences says that
> > during any BLLP (Blanking or Low Power) period the host can do any of:
> > - remain in LP-11
> > - transmit one or more non-video packets from host to peripheral in escape mode
> > - transmit one or more non-video packets from host to peripheral in
> > using HS mode
> > - receive one or more packets from peripheral to host using escape mode
> > - transmit data on a different virtual channel.
> >
> > Indeed if the sink doesn't set MIPI_DSI_MODE_LPM /
> > MIPI_DSI_MSG_USE_LPM, then the expectation is that any data transfer
> > will be in HS mode.
> >
> > That makes me confused as to the need for this patch.
>
> Yeah, and it looks like that would break the expectation that, in
> enable, a bridge can expect its controller to be in HS mode.
>
> I think that was Jagan is trying to do is to work around an issue with
> the Allwinner DSI driver:
> https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L775
>
> This is working mostly fine since we only have panel support and can
> control that, but with bridge support added in the latest patch, then it
> probably doesn't work anymore.
>
> The proper way to fix this isn't to put more logic into the framework,
> it's to make the DSI driver behave as expected by KMS.
>
> Unfortunately, that controller is not documented, so it's not clear to
> me how we can fix it.
>
> IIRC, it's basically a state machine where you would encode the
> transitions between one DSI state and the next depending on what your
> expectations are.
>
> I think there's two problem with the driver that need to be addressed:
>
>   - First the driver will drop back to LP11 mode to submit commands. I
>     don't think it's needed and could even be hurtful to the video
>     stream if it was to happen during HS mode:
>     https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L877
>
>   - And then, it looks like, in HSD mode, we never get to go to the
>     state LPTX is in (LPDT). It would be interesting to test whether
>     adding a transition to that state makes it work or not.

Ooh, not fun.
I'll agree with your assessment - it looks like sunxi driver has
significant limitations on the modes of operation it supports. If
there is no information on sending HS commands, I wonder if it's
possible to note the video state in transfer and stop video, send the
command, and resume video again. Ugly as heck, but possibly the only
real option without documentation. It does raise the question of do
other blocks (eg crtc) need to be stopped as well, or does stopping
the PHY and/or DSI block stop the pixel data getting clocked out.

I can only guess at the meaning of the enum sun6i_dsi_start_inst and
enum sun6i_dsi_inst_id states. LPTX and LPRX are largely obvious, but
HSC(ommand) and HSD(ata) perhaps?
I thought on initial reading that the setup in sun6i_dsi_start made
sense as a sequence of commands, but looking closer at the bitmasking
and shifting I'm not so convinced. Are the DSI_INST_ID_xxx defines
shifts or the bitmask values to or in, as they get used for both.

  Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 12/12] drm: sun4: dsi: Convert to bridge driver

[Jagan Teki]

On Wed, Mar 29, 2023 at 9:36 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Wed, Mar 29, 2023 at 09:08:17PM +0530, Jagan Teki wrote:
> > On Wed, Mar 29, 2023 at 8:29 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > Hi,
> > >
> > > The patch prefix should be drm/sun4i:
> >
> > I did follow my previous prefix, I will update this.
> >
> > >
> > > On Wed, Mar 29, 2023 at 06:49:29PM +0530, Jagan Teki wrote:
> > > > Convert the encoder to bridge driver in order to standardize on a
> > > > single API by supporting all varients of downstream bridge devices.
> > >
> > > Which variant, and why do we need to convert to a bridge to support all of them?
> >
> > Downstream bridge variants like DSI panel, DSI bridge and
> > I2C-Configured DSI bridges. Bridge conversion would be required for
> > the DSI host to access the more variety and complex downstream bridges
> > in a standardized bridge chain way which is indeed complex for encoder
> > driven DSI hosts.
> >
> > >
> > > > The drm_encoder can't be removed as it's exposed to userspace, so it
> > > > then becomes a dumb encoder, without any operation implemented.
> > > >
> > > > Tested on DSI Panel, DSI Bridge, I2C-Configured DSI Bridge.
> > > >
> > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > >
> > > [...]
> > >
> > > > +static const struct component_ops sun6i_dsi_ops;
> > > > +
> > > >  static int sun6i_dsi_attach(struct mipi_dsi_host *host,
> > > >                           struct mipi_dsi_device *device)
> > > >  {
> > > >       struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> > > > -     struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);
> > >
> > > That one looks unrelated. Why do you need that change?
> >
> > This was replaced with drmm_of_dsi_get_bridge for lookup of both panel
> > and bridge. I think I will separate this into another patch.
>
> So, it looks to me that you're doing two (unrelated) things in that patch:

Correct.

>
>   - You modify the existing driver to be a bridge

Yes, Convert to bridge driver - register drm_bridge_add and replace
encoder ops with bridge ops.

>
>   - And you support downstream device being bridges.

Yes, Support the downstream bridge. (If I'm correct we can still use
encoder ops with this).

If we see the hierarchy of support it would
1. support the downstream bridge.
2. convert to the bridge driver.

>
> Both are orthogonal, can (and should!) be done separately, and I'm
> pretty sure you don't actually need to do the former at all.

Do you mean converting to bridge driver is not needed?

Thanks,
Jagan.

Re: [PATCH v7 12/12] drm: sun4: dsi: Convert to bridge driver

[Jagan Teki]

On Wed, Mar 29, 2023 at 9:36 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Wed, Mar 29, 2023 at 09:08:17PM +0530, Jagan Teki wrote:
> > On Wed, Mar 29, 2023 at 8:29 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > Hi,
> > >
> > > The patch prefix should be drm/sun4i:
> >
> > I did follow my previous prefix, I will update this.
> >
> > >
> > > On Wed, Mar 29, 2023 at 06:49:29PM +0530, Jagan Teki wrote:
> > > > Convert the encoder to bridge driver in order to standardize on a
> > > > single API by supporting all varients of downstream bridge devices.
> > >
> > > Which variant, and why do we need to convert to a bridge to support all of them?
> >
> > Downstream bridge variants like DSI panel, DSI bridge and
> > I2C-Configured DSI bridges. Bridge conversion would be required for
> > the DSI host to access the more variety and complex downstream bridges
> > in a standardized bridge chain way which is indeed complex for encoder
> > driven DSI hosts.
> >
> > >
> > > > The drm_encoder can't be removed as it's exposed to userspace, so it
> > > > then becomes a dumb encoder, without any operation implemented.
> > > >
> > > > Tested on DSI Panel, DSI Bridge, I2C-Configured DSI Bridge.
> > > >
> > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > >
> > > [...]
> > >
> > > > +static const struct component_ops sun6i_dsi_ops;
> > > > +
> > > >  static int sun6i_dsi_attach(struct mipi_dsi_host *host,
> > > >                           struct mipi_dsi_device *device)
> > > >  {
> > > >       struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> > > > -     struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);
> > >
> > > That one looks unrelated. Why do you need that change?
> >
> > This was replaced with drmm_of_dsi_get_bridge for lookup of both panel
> > and bridge. I think I will separate this into another patch.
>
> So, it looks to me that you're doing two (unrelated) things in that patch:

Correct.

>
>   - You modify the existing driver to be a bridge

Yes, Convert to bridge driver - register drm_bridge_add and replace
encoder ops with bridge ops.

>
>   - And you support downstream device being bridges.

Yes, Support the downstream bridge. (If I'm correct we can still use
encoder ops with this).

If we see the hierarchy of support it would
1. support the downstream bridge.
2. convert to the bridge driver.

>
> Both are orthogonal, can (and should!) be done separately, and I'm
> pretty sure you don't actually need to do the former at all.

Do you mean converting to bridge driver is not needed?

Thanks,
Jagan.

Re: [PATCH v7 12/12] drm: sun4: dsi: Convert to bridge driver

[Jagan Teki]

On Wed, Mar 29, 2023 at 9:36 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Wed, Mar 29, 2023 at 09:08:17PM +0530, Jagan Teki wrote:
> > On Wed, Mar 29, 2023 at 8:29 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > Hi,
> > >
> > > The patch prefix should be drm/sun4i:
> >
> > I did follow my previous prefix, I will update this.
> >
> > >
> > > On Wed, Mar 29, 2023 at 06:49:29PM +0530, Jagan Teki wrote:
> > > > Convert the encoder to bridge driver in order to standardize on a
> > > > single API by supporting all varients of downstream bridge devices.
> > >
> > > Which variant, and why do we need to convert to a bridge to support all of them?
> >
> > Downstream bridge variants like DSI panel, DSI bridge and
> > I2C-Configured DSI bridges. Bridge conversion would be required for
> > the DSI host to access the more variety and complex downstream bridges
> > in a standardized bridge chain way which is indeed complex for encoder
> > driven DSI hosts.
> >
> > >
> > > > The drm_encoder can't be removed as it's exposed to userspace, so it
> > > > then becomes a dumb encoder, without any operation implemented.
> > > >
> > > > Tested on DSI Panel, DSI Bridge, I2C-Configured DSI Bridge.
> > > >
> > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > >
> > > [...]
> > >
> > > > +static const struct component_ops sun6i_dsi_ops;
> > > > +
> > > >  static int sun6i_dsi_attach(struct mipi_dsi_host *host,
> > > >                           struct mipi_dsi_device *device)
> > > >  {
> > > >       struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> > > > -     struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);
> > >
> > > That one looks unrelated. Why do you need that change?
> >
> > This was replaced with drmm_of_dsi_get_bridge for lookup of both panel
> > and bridge. I think I will separate this into another patch.
>
> So, it looks to me that you're doing two (unrelated) things in that patch:

Correct.

>
>   - You modify the existing driver to be a bridge

Yes, Convert to bridge driver - register drm_bridge_add and replace
encoder ops with bridge ops.

>
>   - And you support downstream device being bridges.

Yes, Support the downstream bridge. (If I'm correct we can still use
encoder ops with this).

If we see the hierarchy of support it would
1. support the downstream bridge.
2. convert to the bridge driver.

>
> Both are orthogonal, can (and should!) be done separately, and I'm
> pretty sure you don't actually need to do the former at all.

Do you mean converting to bridge driver is not needed?

Thanks,
Jagan.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 10/12] drm/bridge: Implement enable_next_first to alter bridge init order

[Jagan Teki]

On Wed, Mar 29, 2023 at 10:16 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Wed, Mar 29, 2023 at 05:28:28PM +0100, Dave Stevenson wrote:
> > On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@amarulasolutions.com> wrote:
> > >
> > > DSI sink devices typically send the MIPI-DCS commands to the DSI host
> > > via general MIPI_DSI_DCS read and write API.
> > >
> > > The classical DSI sequence mentioned that the DSI host receives MIPI-DCS
> > > commands from the DSI sink first in order to switch HS mode properly.
> > > Once the DSI host switches to HS mode any MIPI-DCS commands from the
> > > DSI sink are unfunctional.
> >
> > That statement contradicts the spec.
> > The DSI spec section 8.11.1 Transmission Packet Sequences says that
> > during any BLLP (Blanking or Low Power) period the host can do any of:
> > - remain in LP-11
> > - transmit one or more non-video packets from host to peripheral in escape mode
> > - transmit one or more non-video packets from host to peripheral in
> > using HS mode
> > - receive one or more packets from peripheral to host using escape mode
> > - transmit data on a different virtual channel.
> >
> > Indeed if the sink doesn't set MIPI_DSI_MODE_LPM /
> > MIPI_DSI_MSG_USE_LPM, then the expectation is that any data transfer
> > will be in HS mode.
> >
> > That makes me confused as to the need for this patch.
>
> Yeah, and it looks like that would break the expectation that, in
> enable, a bridge can expect its controller to be in HS mode.
>
> I think that was Jagan is trying to do is to work around an issue with
> the Allwinner DSI driver:
> https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L775

Correct and I can see it seems to be a classic DSI sequence observed
in dw-mipi-dsi as well - based on Neil's comments.
https://lore.kernel.org/all/9aa3d19d-4378-aaf3-6857-c40be5d252c7@baylibre.com/

In fact, I did follow and initialize the command-mode mode_set which
set low-speed DCS and switch back to video-mode @enable and switch to
HS but could see the same issue as the host cannot accept DCS as
before (I might implement improper sequence, but not sure due to lack
of documentation). But this sequence has issues with calling
post_disable twice even on dw-mipi-dsi.

May be Neill, can comment here?

Thanks,
Jagan.

Re: [PATCH v7 10/12] drm/bridge: Implement enable_next_first to alter bridge init order

[Jagan Teki]

On Wed, Mar 29, 2023 at 10:16 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Wed, Mar 29, 2023 at 05:28:28PM +0100, Dave Stevenson wrote:
> > On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@amarulasolutions.com> wrote:
> > >
> > > DSI sink devices typically send the MIPI-DCS commands to the DSI host
> > > via general MIPI_DSI_DCS read and write API.
> > >
> > > The classical DSI sequence mentioned that the DSI host receives MIPI-DCS
> > > commands from the DSI sink first in order to switch HS mode properly.
> > > Once the DSI host switches to HS mode any MIPI-DCS commands from the
> > > DSI sink are unfunctional.
> >
> > That statement contradicts the spec.
> > The DSI spec section 8.11.1 Transmission Packet Sequences says that
> > during any BLLP (Blanking or Low Power) period the host can do any of:
> > - remain in LP-11
> > - transmit one or more non-video packets from host to peripheral in escape mode
> > - transmit one or more non-video packets from host to peripheral in
> > using HS mode
> > - receive one or more packets from peripheral to host using escape mode
> > - transmit data on a different virtual channel.
> >
> > Indeed if the sink doesn't set MIPI_DSI_MODE_LPM /
> > MIPI_DSI_MSG_USE_LPM, then the expectation is that any data transfer
> > will be in HS mode.
> >
> > That makes me confused as to the need for this patch.
>
> Yeah, and it looks like that would break the expectation that, in
> enable, a bridge can expect its controller to be in HS mode.
>
> I think that was Jagan is trying to do is to work around an issue with
> the Allwinner DSI driver:
> https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L775

Correct and I can see it seems to be a classic DSI sequence observed
in dw-mipi-dsi as well - based on Neil's comments.
https://lore.kernel.org/all/9aa3d19d-4378-aaf3-6857-c40be5d252c7@baylibre.com/

In fact, I did follow and initialize the command-mode mode_set which
set low-speed DCS and switch back to video-mode @enable and switch to
HS but could see the same issue as the host cannot accept DCS as
before (I might implement improper sequence, but not sure due to lack
of documentation). But this sequence has issues with calling
post_disable twice even on dw-mipi-dsi.

May be Neill, can comment here?

Thanks,
Jagan.

Re: [PATCH v7 10/12] drm/bridge: Implement enable_next_first to alter bridge init order

[Jagan Teki]

On Wed, Mar 29, 2023 at 10:16 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Wed, Mar 29, 2023 at 05:28:28PM +0100, Dave Stevenson wrote:
> > On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@amarulasolutions.com> wrote:
> > >
> > > DSI sink devices typically send the MIPI-DCS commands to the DSI host
> > > via general MIPI_DSI_DCS read and write API.
> > >
> > > The classical DSI sequence mentioned that the DSI host receives MIPI-DCS
> > > commands from the DSI sink first in order to switch HS mode properly.
> > > Once the DSI host switches to HS mode any MIPI-DCS commands from the
> > > DSI sink are unfunctional.
> >
> > That statement contradicts the spec.
> > The DSI spec section 8.11.1 Transmission Packet Sequences says that
> > during any BLLP (Blanking or Low Power) period the host can do any of:
> > - remain in LP-11
> > - transmit one or more non-video packets from host to peripheral in escape mode
> > - transmit one or more non-video packets from host to peripheral in
> > using HS mode
> > - receive one or more packets from peripheral to host using escape mode
> > - transmit data on a different virtual channel.
> >
> > Indeed if the sink doesn't set MIPI_DSI_MODE_LPM /
> > MIPI_DSI_MSG_USE_LPM, then the expectation is that any data transfer
> > will be in HS mode.
> >
> > That makes me confused as to the need for this patch.
>
> Yeah, and it looks like that would break the expectation that, in
> enable, a bridge can expect its controller to be in HS mode.
>
> I think that was Jagan is trying to do is to work around an issue with
> the Allwinner DSI driver:
> https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L775

Correct and I can see it seems to be a classic DSI sequence observed
in dw-mipi-dsi as well - based on Neil's comments.
https://lore.kernel.org/all/9aa3d19d-4378-aaf3-6857-c40be5d252c7@baylibre.com/

In fact, I did follow and initialize the command-mode mode_set which
set low-speed DCS and switch back to video-mode @enable and switch to
HS but could see the same issue as the host cannot accept DCS as
before (I might implement improper sequence, but not sure due to lack
of documentation). But this sequence has issues with calling
post_disable twice even on dw-mipi-dsi.

May be Neill, can comment here?

Thanks,
Jagan.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 12/12] drm: sun4: dsi: Convert to bridge driver

[Maxime Ripard]

On Thu, Mar 30, 2023 at 12:15:49PM +0530, Jagan Teki wrote:
> On Wed, Mar 29, 2023 at 9:36 PM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > On Wed, Mar 29, 2023 at 09:08:17PM +0530, Jagan Teki wrote:
> > > On Wed, Mar 29, 2023 at 8:29 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > > >
> > > > Hi,
> > > >
> > > > The patch prefix should be drm/sun4i:
> > >
> > > I did follow my previous prefix, I will update this.
> > >
> > > >
> > > > On Wed, Mar 29, 2023 at 06:49:29PM +0530, Jagan Teki wrote:
> > > > > Convert the encoder to bridge driver in order to standardize on a
> > > > > single API by supporting all varients of downstream bridge devices.
> > > >
> > > > Which variant, and why do we need to convert to a bridge to support all of them?
> > >
> > > Downstream bridge variants like DSI panel, DSI bridge and
> > > I2C-Configured DSI bridges. Bridge conversion would be required for
> > > the DSI host to access the more variety and complex downstream bridges
> > > in a standardized bridge chain way which is indeed complex for encoder
> > > driven DSI hosts.
> > >
> > > >
> > > > > The drm_encoder can't be removed as it's exposed to userspace, so it
> > > > > then becomes a dumb encoder, without any operation implemented.
> > > > >
> > > > > Tested on DSI Panel, DSI Bridge, I2C-Configured DSI Bridge.
> > > > >
> > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > >
> > > > [...]
> > > >
> > > > > +static const struct component_ops sun6i_dsi_ops;
> > > > > +
> > > > >  static int sun6i_dsi_attach(struct mipi_dsi_host *host,
> > > > >                           struct mipi_dsi_device *device)
> > > > >  {
> > > > >       struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> > > > > -     struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);
> > > >
> > > > That one looks unrelated. Why do you need that change?
> > >
> > > This was replaced with drmm_of_dsi_get_bridge for lookup of both panel
> > > and bridge. I think I will separate this into another patch.
> >
> > So, it looks to me that you're doing two (unrelated) things in that patch:
> 
> Correct.
> 
> >
> >   - You modify the existing driver to be a bridge
> 
> Yes, Convert to bridge driver - register drm_bridge_add and replace
> encoder ops with bridge ops.
> 
> >
> >   - And you support downstream device being bridges.
> 
> Yes, Support the downstream bridge. (If I'm correct we can still use
> encoder ops with this).
> 
> If we see the hierarchy of support it would
> 1. support the downstream bridge.
> 2. convert to the bridge driver.
> 
> >
> > Both are orthogonal, can (and should!) be done separately, and I'm
> > pretty sure you don't actually need to do the former at all.
> 
> Do you mean converting to bridge driver is not needed?

Yes, and given the current state of the DCS-in-HS discussion, I even
think it's does more harm than good.

Maxime

Re: [PATCH v7 12/12] drm: sun4: dsi: Convert to bridge driver

[Maxime Ripard]

On Thu, Mar 30, 2023 at 12:15:49PM +0530, Jagan Teki wrote:
> On Wed, Mar 29, 2023 at 9:36 PM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > On Wed, Mar 29, 2023 at 09:08:17PM +0530, Jagan Teki wrote:
> > > On Wed, Mar 29, 2023 at 8:29 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > > >
> > > > Hi,
> > > >
> > > > The patch prefix should be drm/sun4i:
> > >
> > > I did follow my previous prefix, I will update this.
> > >
> > > >
> > > > On Wed, Mar 29, 2023 at 06:49:29PM +0530, Jagan Teki wrote:
> > > > > Convert the encoder to bridge driver in order to standardize on a
> > > > > single API by supporting all varients of downstream bridge devices.
> > > >
> > > > Which variant, and why do we need to convert to a bridge to support all of them?
> > >
> > > Downstream bridge variants like DSI panel, DSI bridge and
> > > I2C-Configured DSI bridges. Bridge conversion would be required for
> > > the DSI host to access the more variety and complex downstream bridges
> > > in a standardized bridge chain way which is indeed complex for encoder
> > > driven DSI hosts.
> > >
> > > >
> > > > > The drm_encoder can't be removed as it's exposed to userspace, so it
> > > > > then becomes a dumb encoder, without any operation implemented.
> > > > >
> > > > > Tested on DSI Panel, DSI Bridge, I2C-Configured DSI Bridge.
> > > > >
> > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > >
> > > > [...]
> > > >
> > > > > +static const struct component_ops sun6i_dsi_ops;
> > > > > +
> > > > >  static int sun6i_dsi_attach(struct mipi_dsi_host *host,
> > > > >                           struct mipi_dsi_device *device)
> > > > >  {
> > > > >       struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> > > > > -     struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);
> > > >
> > > > That one looks unrelated. Why do you need that change?
> > >
> > > This was replaced with drmm_of_dsi_get_bridge for lookup of both panel
> > > and bridge. I think I will separate this into another patch.
> >
> > So, it looks to me that you're doing two (unrelated) things in that patch:
> 
> Correct.
> 
> >
> >   - You modify the existing driver to be a bridge
> 
> Yes, Convert to bridge driver - register drm_bridge_add and replace
> encoder ops with bridge ops.
> 
> >
> >   - And you support downstream device being bridges.
> 
> Yes, Support the downstream bridge. (If I'm correct we can still use
> encoder ops with this).
> 
> If we see the hierarchy of support it would
> 1. support the downstream bridge.
> 2. convert to the bridge driver.
> 
> >
> > Both are orthogonal, can (and should!) be done separately, and I'm
> > pretty sure you don't actually need to do the former at all.
> 
> Do you mean converting to bridge driver is not needed?

Yes, and given the current state of the DCS-in-HS discussion, I even
think it's does more harm than good.

Maxime

Re: [PATCH v7 12/12] drm: sun4: dsi: Convert to bridge driver

[Maxime Ripard]

On Thu, Mar 30, 2023 at 12:15:49PM +0530, Jagan Teki wrote:
> On Wed, Mar 29, 2023 at 9:36 PM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > On Wed, Mar 29, 2023 at 09:08:17PM +0530, Jagan Teki wrote:
> > > On Wed, Mar 29, 2023 at 8:29 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > > >
> > > > Hi,
> > > >
> > > > The patch prefix should be drm/sun4i:
> > >
> > > I did follow my previous prefix, I will update this.
> > >
> > > >
> > > > On Wed, Mar 29, 2023 at 06:49:29PM +0530, Jagan Teki wrote:
> > > > > Convert the encoder to bridge driver in order to standardize on a
> > > > > single API by supporting all varients of downstream bridge devices.
> > > >
> > > > Which variant, and why do we need to convert to a bridge to support all of them?
> > >
> > > Downstream bridge variants like DSI panel, DSI bridge and
> > > I2C-Configured DSI bridges. Bridge conversion would be required for
> > > the DSI host to access the more variety and complex downstream bridges
> > > in a standardized bridge chain way which is indeed complex for encoder
> > > driven DSI hosts.
> > >
> > > >
> > > > > The drm_encoder can't be removed as it's exposed to userspace, so it
> > > > > then becomes a dumb encoder, without any operation implemented.
> > > > >
> > > > > Tested on DSI Panel, DSI Bridge, I2C-Configured DSI Bridge.
> > > > >
> > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > >
> > > > [...]
> > > >
> > > > > +static const struct component_ops sun6i_dsi_ops;
> > > > > +
> > > > >  static int sun6i_dsi_attach(struct mipi_dsi_host *host,
> > > > >                           struct mipi_dsi_device *device)
> > > > >  {
> > > > >       struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> > > > > -     struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);
> > > >
> > > > That one looks unrelated. Why do you need that change?
> > >
> > > This was replaced with drmm_of_dsi_get_bridge for lookup of both panel
> > > and bridge. I think I will separate this into another patch.
> >
> > So, it looks to me that you're doing two (unrelated) things in that patch:
> 
> Correct.
> 
> >
> >   - You modify the existing driver to be a bridge
> 
> Yes, Convert to bridge driver - register drm_bridge_add and replace
> encoder ops with bridge ops.
> 
> >
> >   - And you support downstream device being bridges.
> 
> Yes, Support the downstream bridge. (If I'm correct we can still use
> encoder ops with this).
> 
> If we see the hierarchy of support it would
> 1. support the downstream bridge.
> 2. convert to the bridge driver.
> 
> >
> > Both are orthogonal, can (and should!) be done separately, and I'm
> > pretty sure you don't actually need to do the former at all.
> 
> Do you mean converting to bridge driver is not needed?

Yes, and given the current state of the DCS-in-HS discussion, I even
think it's does more harm than good.

Maxime

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 10/12] drm/bridge: Implement enable_next_first to alter bridge init order

[Dave Stevenson]

Hi Jagan

On Thu, 30 Mar 2023 at 07:56, Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> On Wed, Mar 29, 2023 at 10:16 PM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > On Wed, Mar 29, 2023 at 05:28:28PM +0100, Dave Stevenson wrote:
> > > On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > >
> > > > DSI sink devices typically send the MIPI-DCS commands to the DSI host
> > > > via general MIPI_DSI_DCS read and write API.
> > > >
> > > > The classical DSI sequence mentioned that the DSI host receives MIPI-DCS
> > > > commands from the DSI sink first in order to switch HS mode properly.
> > > > Once the DSI host switches to HS mode any MIPI-DCS commands from the
> > > > DSI sink are unfunctional.
> > >
> > > That statement contradicts the spec.
> > > The DSI spec section 8.11.1 Transmission Packet Sequences says that
> > > during any BLLP (Blanking or Low Power) period the host can do any of:
> > > - remain in LP-11
> > > - transmit one or more non-video packets from host to peripheral in escape mode
> > > - transmit one or more non-video packets from host to peripheral in
> > > using HS mode
> > > - receive one or more packets from peripheral to host using escape mode
> > > - transmit data on a different virtual channel.
> > >
> > > Indeed if the sink doesn't set MIPI_DSI_MODE_LPM /
> > > MIPI_DSI_MSG_USE_LPM, then the expectation is that any data transfer
> > > will be in HS mode.
> > >
> > > That makes me confused as to the need for this patch.
> >
> > Yeah, and it looks like that would break the expectation that, in
> > enable, a bridge can expect its controller to be in HS mode.
> >
> > I think that was Jagan is trying to do is to work around an issue with
> > the Allwinner DSI driver:
> > https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L775
>
> Correct and I can see it seems to be a classic DSI sequence observed
> in dw-mipi-dsi as well - based on Neil's comments.
> https://lore.kernel.org/all/9aa3d19d-4378-aaf3-6857-c40be5d252c7@baylibre.com/

Neil's comments are from 2021, and his response would appear to be
with regard the PHY power up sequence issues that
pre_enable_prev_first should solve. The DSI host pre_enable can now be
called before the sink's pre_enable, therefore allowing the PHY to be
configured in pre_enable. Hacking the PHY init into mode_set is
therefore not required.

I don't see any restriction in dw-mipi-dsi over when transfer can be
called (as long as it is between pre_enable and post_disable), and it
supports MIPI_DSI_MSG_USE_LPM for requesting the command be sent in
either LP or HS mode.

> In fact, I did follow and initialize the command-mode mode_set which
> set low-speed DCS and switch back to video-mode @enable and switch to
> HS but could see the same issue as the host cannot accept DCS as
> before (I might implement improper sequence, but not sure due to lack
> of documentation). But this sequence has issues with calling
> post_disable twice even on dw-mipi-dsi.

Calling up/down the bridge chain from within other bridge elements is
going to have issues and shouldn't be necessary.

The comment in dw-mipi-dsi post_disable[1]
* TODO Only way found to call panel-bridge post_disable &
* panel unprepare before the dsi "final" disable...
* This needs to be fixed in the drm_bridge framework and the API
* needs to be updated to manage our own call chains...

It has now been fixed up with pre_enable_prev_first.

I seem to recall seeing a patchset for one of the DSI hosts (other
than vc4) that was moving the init from mode_set to pre_enable - I
think it is probably [2] for msm.

Cheers
  Dave

[1] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L862-L867
[2] https://github.com/torvalds/linux/commit/ec7981e6c614254937b37ce0af9eac09901c05c5

> May be Neill, can comment here?
>
> Thanks,
> Jagan.

Re: [PATCH v7 10/12] drm/bridge: Implement enable_next_first to alter bridge init order

[Dave Stevenson]

Hi Jagan

On Thu, 30 Mar 2023 at 07:56, Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> On Wed, Mar 29, 2023 at 10:16 PM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > On Wed, Mar 29, 2023 at 05:28:28PM +0100, Dave Stevenson wrote:
> > > On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > >
> > > > DSI sink devices typically send the MIPI-DCS commands to the DSI host
> > > > via general MIPI_DSI_DCS read and write API.
> > > >
> > > > The classical DSI sequence mentioned that the DSI host receives MIPI-DCS
> > > > commands from the DSI sink first in order to switch HS mode properly.
> > > > Once the DSI host switches to HS mode any MIPI-DCS commands from the
> > > > DSI sink are unfunctional.
> > >
> > > That statement contradicts the spec.
> > > The DSI spec section 8.11.1 Transmission Packet Sequences says that
> > > during any BLLP (Blanking or Low Power) period the host can do any of:
> > > - remain in LP-11
> > > - transmit one or more non-video packets from host to peripheral in escape mode
> > > - transmit one or more non-video packets from host to peripheral in
> > > using HS mode
> > > - receive one or more packets from peripheral to host using escape mode
> > > - transmit data on a different virtual channel.
> > >
> > > Indeed if the sink doesn't set MIPI_DSI_MODE_LPM /
> > > MIPI_DSI_MSG_USE_LPM, then the expectation is that any data transfer
> > > will be in HS mode.
> > >
> > > That makes me confused as to the need for this patch.
> >
> > Yeah, and it looks like that would break the expectation that, in
> > enable, a bridge can expect its controller to be in HS mode.
> >
> > I think that was Jagan is trying to do is to work around an issue with
> > the Allwinner DSI driver:
> > https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L775
>
> Correct and I can see it seems to be a classic DSI sequence observed
> in dw-mipi-dsi as well - based on Neil's comments.
> https://lore.kernel.org/all/9aa3d19d-4378-aaf3-6857-c40be5d252c7@baylibre.com/

Neil's comments are from 2021, and his response would appear to be
with regard the PHY power up sequence issues that
pre_enable_prev_first should solve. The DSI host pre_enable can now be
called before the sink's pre_enable, therefore allowing the PHY to be
configured in pre_enable. Hacking the PHY init into mode_set is
therefore not required.

I don't see any restriction in dw-mipi-dsi over when transfer can be
called (as long as it is between pre_enable and post_disable), and it
supports MIPI_DSI_MSG_USE_LPM for requesting the command be sent in
either LP or HS mode.

> In fact, I did follow and initialize the command-mode mode_set which
> set low-speed DCS and switch back to video-mode @enable and switch to
> HS but could see the same issue as the host cannot accept DCS as
> before (I might implement improper sequence, but not sure due to lack
> of documentation). But this sequence has issues with calling
> post_disable twice even on dw-mipi-dsi.

Calling up/down the bridge chain from within other bridge elements is
going to have issues and shouldn't be necessary.

The comment in dw-mipi-dsi post_disable[1]
* TODO Only way found to call panel-bridge post_disable &
* panel unprepare before the dsi "final" disable...
* This needs to be fixed in the drm_bridge framework and the API
* needs to be updated to manage our own call chains...

It has now been fixed up with pre_enable_prev_first.

I seem to recall seeing a patchset for one of the DSI hosts (other
than vc4) that was moving the init from mode_set to pre_enable - I
think it is probably [2] for msm.

Cheers
  Dave

[1] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L862-L867
[2] https://github.com/torvalds/linux/commit/ec7981e6c614254937b37ce0af9eac09901c05c5

> May be Neill, can comment here?
>
> Thanks,
> Jagan.

Re: [PATCH v7 10/12] drm/bridge: Implement enable_next_first to alter bridge init order

[Dave Stevenson]

Hi Jagan

On Thu, 30 Mar 2023 at 07:56, Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> On Wed, Mar 29, 2023 at 10:16 PM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > On Wed, Mar 29, 2023 at 05:28:28PM +0100, Dave Stevenson wrote:
> > > On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > >
> > > > DSI sink devices typically send the MIPI-DCS commands to the DSI host
> > > > via general MIPI_DSI_DCS read and write API.
> > > >
> > > > The classical DSI sequence mentioned that the DSI host receives MIPI-DCS
> > > > commands from the DSI sink first in order to switch HS mode properly.
> > > > Once the DSI host switches to HS mode any MIPI-DCS commands from the
> > > > DSI sink are unfunctional.
> > >
> > > That statement contradicts the spec.
> > > The DSI spec section 8.11.1 Transmission Packet Sequences says that
> > > during any BLLP (Blanking or Low Power) period the host can do any of:
> > > - remain in LP-11
> > > - transmit one or more non-video packets from host to peripheral in escape mode
> > > - transmit one or more non-video packets from host to peripheral in
> > > using HS mode
> > > - receive one or more packets from peripheral to host using escape mode
> > > - transmit data on a different virtual channel.
> > >
> > > Indeed if the sink doesn't set MIPI_DSI_MODE_LPM /
> > > MIPI_DSI_MSG_USE_LPM, then the expectation is that any data transfer
> > > will be in HS mode.
> > >
> > > That makes me confused as to the need for this patch.
> >
> > Yeah, and it looks like that would break the expectation that, in
> > enable, a bridge can expect its controller to be in HS mode.
> >
> > I think that was Jagan is trying to do is to work around an issue with
> > the Allwinner DSI driver:
> > https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L775
>
> Correct and I can see it seems to be a classic DSI sequence observed
> in dw-mipi-dsi as well - based on Neil's comments.
> https://lore.kernel.org/all/9aa3d19d-4378-aaf3-6857-c40be5d252c7@baylibre.com/

Neil's comments are from 2021, and his response would appear to be
with regard the PHY power up sequence issues that
pre_enable_prev_first should solve. The DSI host pre_enable can now be
called before the sink's pre_enable, therefore allowing the PHY to be
configured in pre_enable. Hacking the PHY init into mode_set is
therefore not required.

I don't see any restriction in dw-mipi-dsi over when transfer can be
called (as long as it is between pre_enable and post_disable), and it
supports MIPI_DSI_MSG_USE_LPM for requesting the command be sent in
either LP or HS mode.

> In fact, I did follow and initialize the command-mode mode_set which
> set low-speed DCS and switch back to video-mode @enable and switch to
> HS but could see the same issue as the host cannot accept DCS as
> before (I might implement improper sequence, but not sure due to lack
> of documentation). But this sequence has issues with calling
> post_disable twice even on dw-mipi-dsi.

Calling up/down the bridge chain from within other bridge elements is
going to have issues and shouldn't be necessary.

The comment in dw-mipi-dsi post_disable[1]
* TODO Only way found to call panel-bridge post_disable &
* panel unprepare before the dsi "final" disable...
* This needs to be fixed in the drm_bridge framework and the API
* needs to be updated to manage our own call chains...

It has now been fixed up with pre_enable_prev_first.

I seem to recall seeing a patchset for one of the DSI hosts (other
than vc4) that was moving the init from mode_set to pre_enable - I
think it is probably [2] for msm.

Cheers
  Dave

[1] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L862-L867
[2] https://github.com/torvalds/linux/commit/ec7981e6c614254937b37ce0af9eac09901c05c5

> May be Neill, can comment here?
>
> Thanks,
> Jagan.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 10/12] drm/bridge: Implement enable_next_first to alter bridge init order

[Neil Armstrong]

On 30/03/2023 12:01, Dave Stevenson wrote:
> Hi Jagan
> 
> On Thu, 30 Mar 2023 at 07:56, Jagan Teki <jagan@amarulasolutions.com> wrote:
>>
>> On Wed, Mar 29, 2023 at 10:16 PM Maxime Ripard <maxime@cerno.tech> wrote:
>>>
>>> On Wed, Mar 29, 2023 at 05:28:28PM +0100, Dave Stevenson wrote:
>>>> On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@amarulasolutions.com> wrote:
>>>>>
>>>>> DSI sink devices typically send the MIPI-DCS commands to the DSI host
>>>>> via general MIPI_DSI_DCS read and write API.
>>>>>
>>>>> The classical DSI sequence mentioned that the DSI host receives MIPI-DCS
>>>>> commands from the DSI sink first in order to switch HS mode properly.
>>>>> Once the DSI host switches to HS mode any MIPI-DCS commands from the
>>>>> DSI sink are unfunctional.
>>>>
>>>> That statement contradicts the spec.
>>>> The DSI spec section 8.11.1 Transmission Packet Sequences says that
>>>> during any BLLP (Blanking or Low Power) period the host can do any of:
>>>> - remain in LP-11
>>>> - transmit one or more non-video packets from host to peripheral in escape mode
>>>> - transmit one or more non-video packets from host to peripheral in
>>>> using HS mode
>>>> - receive one or more packets from peripheral to host using escape mode
>>>> - transmit data on a different virtual channel.
>>>>
>>>> Indeed if the sink doesn't set MIPI_DSI_MODE_LPM /
>>>> MIPI_DSI_MSG_USE_LPM, then the expectation is that any data transfer
>>>> will be in HS mode.
>>>>
>>>> That makes me confused as to the need for this patch.
>>>
>>> Yeah, and it looks like that would break the expectation that, in
>>> enable, a bridge can expect its controller to be in HS mode.
>>>
>>> I think that was Jagan is trying to do is to work around an issue with
>>> the Allwinner DSI driver:
>>> https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L775
>>
>> Correct and I can see it seems to be a classic DSI sequence observed
>> in dw-mipi-dsi as well - based on Neil's comments.
>> https://lore.kernel.org/all/9aa3d19d-4378-aaf3-6857-c40be5d252c7@baylibre.com/
> 
> Neil's comments are from 2021, and his response would appear to be
> with regard the PHY power up sequence issues that
> pre_enable_prev_first should solve. The DSI host pre_enable can now be
> called before the sink's pre_enable, therefore allowing the PHY to be
> configured in pre_enable. Hacking the PHY init into mode_set is
> therefore not required.

Yes this part is not solved, but is seems the assumption the DSI controller
can switch to HS to LS & then to HS back after a command while in video mode
isn't true in the Allwinner's case. As I understood it's one of the problems.

We're hitting a limit of the DSI controller model in Linux where we cannot
express all the DSI capabilities (Video mode, Command mode, dynamic framerate
switching, DSC, ...) since from the Panel or Bridge PoV we're blind and
we do not know what are the features supported by the DSI controller and
we lack knowledge of any operation mode we must try to achieve.

> 
> I don't see any restriction in dw-mipi-dsi over when transfer can be
> called (as long as it is between pre_enable and post_disable), and it
> supports MIPI_DSI_MSG_USE_LPM for requesting the command be sent in
> either LP or HS mode.
> 
>> In fact, I did follow and initialize the command-mode mode_set which
>> set low-speed DCS and switch back to video-mode @enable and switch to
>> HS but could see the same issue as the host cannot accept DCS as
>> before (I might implement improper sequence, but not sure due to lack
>> of documentation). But this sequence has issues with calling
>> post_disable twice even on dw-mipi-dsi.
> 
> Calling up/down the bridge chain from within other bridge elements is
> going to have issues and shouldn't be necessary.
> 
> The comment in dw-mipi-dsi post_disable[1]
> * TODO Only way found to call panel-bridge post_disable &
> * panel unprepare before the dsi "final" disable...
> * This needs to be fixed in the drm_bridge framework and the API
> * needs to be updated to manage our own call chains...
> 
> It has now been fixed up with pre_enable_prev_first.
> 
> I seem to recall seeing a patchset for one of the DSI hosts (other
> than vc4) that was moving the init from mode_set to pre_enable - I
> think it is probably [2] for msm.
> 
> Cheers
>    Dave
> 
> [1] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L862-L867
> [2] https://github.com/torvalds/linux/commit/ec7981e6c614254937b37ce0af9eac09901c05c5
> 
>> May be Neill, can comment here?
>>
>> Thanks,
>> Jagan.

Re: [PATCH v7 10/12] drm/bridge: Implement enable_next_first to alter bridge init order

[Neil Armstrong]

On 30/03/2023 12:01, Dave Stevenson wrote:
> Hi Jagan
> 
> On Thu, 30 Mar 2023 at 07:56, Jagan Teki <jagan@amarulasolutions.com> wrote:
>>
>> On Wed, Mar 29, 2023 at 10:16 PM Maxime Ripard <maxime@cerno.tech> wrote:
>>>
>>> On Wed, Mar 29, 2023 at 05:28:28PM +0100, Dave Stevenson wrote:
>>>> On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@amarulasolutions.com> wrote:
>>>>>
>>>>> DSI sink devices typically send the MIPI-DCS commands to the DSI host
>>>>> via general MIPI_DSI_DCS read and write API.
>>>>>
>>>>> The classical DSI sequence mentioned that the DSI host receives MIPI-DCS
>>>>> commands from the DSI sink first in order to switch HS mode properly.
>>>>> Once the DSI host switches to HS mode any MIPI-DCS commands from the
>>>>> DSI sink are unfunctional.
>>>>
>>>> That statement contradicts the spec.
>>>> The DSI spec section 8.11.1 Transmission Packet Sequences says that
>>>> during any BLLP (Blanking or Low Power) period the host can do any of:
>>>> - remain in LP-11
>>>> - transmit one or more non-video packets from host to peripheral in escape mode
>>>> - transmit one or more non-video packets from host to peripheral in
>>>> using HS mode
>>>> - receive one or more packets from peripheral to host using escape mode
>>>> - transmit data on a different virtual channel.
>>>>
>>>> Indeed if the sink doesn't set MIPI_DSI_MODE_LPM /
>>>> MIPI_DSI_MSG_USE_LPM, then the expectation is that any data transfer
>>>> will be in HS mode.
>>>>
>>>> That makes me confused as to the need for this patch.
>>>
>>> Yeah, and it looks like that would break the expectation that, in
>>> enable, a bridge can expect its controller to be in HS mode.
>>>
>>> I think that was Jagan is trying to do is to work around an issue with
>>> the Allwinner DSI driver:
>>> https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L775
>>
>> Correct and I can see it seems to be a classic DSI sequence observed
>> in dw-mipi-dsi as well - based on Neil's comments.
>> https://lore.kernel.org/all/9aa3d19d-4378-aaf3-6857-c40be5d252c7@baylibre.com/
> 
> Neil's comments are from 2021, and his response would appear to be
> with regard the PHY power up sequence issues that
> pre_enable_prev_first should solve. The DSI host pre_enable can now be
> called before the sink's pre_enable, therefore allowing the PHY to be
> configured in pre_enable. Hacking the PHY init into mode_set is
> therefore not required.

Yes this part is not solved, but is seems the assumption the DSI controller
can switch to HS to LS & then to HS back after a command while in video mode
isn't true in the Allwinner's case. As I understood it's one of the problems.

We're hitting a limit of the DSI controller model in Linux where we cannot
express all the DSI capabilities (Video mode, Command mode, dynamic framerate
switching, DSC, ...) since from the Panel or Bridge PoV we're blind and
we do not know what are the features supported by the DSI controller and
we lack knowledge of any operation mode we must try to achieve.

> 
> I don't see any restriction in dw-mipi-dsi over when transfer can be
> called (as long as it is between pre_enable and post_disable), and it
> supports MIPI_DSI_MSG_USE_LPM for requesting the command be sent in
> either LP or HS mode.
> 
>> In fact, I did follow and initialize the command-mode mode_set which
>> set low-speed DCS and switch back to video-mode @enable and switch to
>> HS but could see the same issue as the host cannot accept DCS as
>> before (I might implement improper sequence, but not sure due to lack
>> of documentation). But this sequence has issues with calling
>> post_disable twice even on dw-mipi-dsi.
> 
> Calling up/down the bridge chain from within other bridge elements is
> going to have issues and shouldn't be necessary.
> 
> The comment in dw-mipi-dsi post_disable[1]
> * TODO Only way found to call panel-bridge post_disable &
> * panel unprepare before the dsi "final" disable...
> * This needs to be fixed in the drm_bridge framework and the API
> * needs to be updated to manage our own call chains...
> 
> It has now been fixed up with pre_enable_prev_first.
> 
> I seem to recall seeing a patchset for one of the DSI hosts (other
> than vc4) that was moving the init from mode_set to pre_enable - I
> think it is probably [2] for msm.
> 
> Cheers
>    Dave
> 
> [1] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L862-L867
> [2] https://github.com/torvalds/linux/commit/ec7981e6c614254937b37ce0af9eac09901c05c5
> 
>> May be Neill, can comment here?
>>
>> Thanks,
>> Jagan.

Re: [PATCH v7 10/12] drm/bridge: Implement enable_next_first to alter bridge init order

[Neil Armstrong]

On 30/03/2023 12:01, Dave Stevenson wrote:
> Hi Jagan
> 
> On Thu, 30 Mar 2023 at 07:56, Jagan Teki <jagan@amarulasolutions.com> wrote:
>>
>> On Wed, Mar 29, 2023 at 10:16 PM Maxime Ripard <maxime@cerno.tech> wrote:
>>>
>>> On Wed, Mar 29, 2023 at 05:28:28PM +0100, Dave Stevenson wrote:
>>>> On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@amarulasolutions.com> wrote:
>>>>>
>>>>> DSI sink devices typically send the MIPI-DCS commands to the DSI host
>>>>> via general MIPI_DSI_DCS read and write API.
>>>>>
>>>>> The classical DSI sequence mentioned that the DSI host receives MIPI-DCS
>>>>> commands from the DSI sink first in order to switch HS mode properly.
>>>>> Once the DSI host switches to HS mode any MIPI-DCS commands from the
>>>>> DSI sink are unfunctional.
>>>>
>>>> That statement contradicts the spec.
>>>> The DSI spec section 8.11.1 Transmission Packet Sequences says that
>>>> during any BLLP (Blanking or Low Power) period the host can do any of:
>>>> - remain in LP-11
>>>> - transmit one or more non-video packets from host to peripheral in escape mode
>>>> - transmit one or more non-video packets from host to peripheral in
>>>> using HS mode
>>>> - receive one or more packets from peripheral to host using escape mode
>>>> - transmit data on a different virtual channel.
>>>>
>>>> Indeed if the sink doesn't set MIPI_DSI_MODE_LPM /
>>>> MIPI_DSI_MSG_USE_LPM, then the expectation is that any data transfer
>>>> will be in HS mode.
>>>>
>>>> That makes me confused as to the need for this patch.
>>>
>>> Yeah, and it looks like that would break the expectation that, in
>>> enable, a bridge can expect its controller to be in HS mode.
>>>
>>> I think that was Jagan is trying to do is to work around an issue with
>>> the Allwinner DSI driver:
>>> https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L775
>>
>> Correct and I can see it seems to be a classic DSI sequence observed
>> in dw-mipi-dsi as well - based on Neil's comments.
>> https://lore.kernel.org/all/9aa3d19d-4378-aaf3-6857-c40be5d252c7@baylibre.com/
> 
> Neil's comments are from 2021, and his response would appear to be
> with regard the PHY power up sequence issues that
> pre_enable_prev_first should solve. The DSI host pre_enable can now be
> called before the sink's pre_enable, therefore allowing the PHY to be
> configured in pre_enable. Hacking the PHY init into mode_set is
> therefore not required.

Yes this part is not solved, but is seems the assumption the DSI controller
can switch to HS to LS & then to HS back after a command while in video mode
isn't true in the Allwinner's case. As I understood it's one of the problems.

We're hitting a limit of the DSI controller model in Linux where we cannot
express all the DSI capabilities (Video mode, Command mode, dynamic framerate
switching, DSC, ...) since from the Panel or Bridge PoV we're blind and
we do not know what are the features supported by the DSI controller and
we lack knowledge of any operation mode we must try to achieve.

> 
> I don't see any restriction in dw-mipi-dsi over when transfer can be
> called (as long as it is between pre_enable and post_disable), and it
> supports MIPI_DSI_MSG_USE_LPM for requesting the command be sent in
> either LP or HS mode.
> 
>> In fact, I did follow and initialize the command-mode mode_set which
>> set low-speed DCS and switch back to video-mode @enable and switch to
>> HS but could see the same issue as the host cannot accept DCS as
>> before (I might implement improper sequence, but not sure due to lack
>> of documentation). But this sequence has issues with calling
>> post_disable twice even on dw-mipi-dsi.
> 
> Calling up/down the bridge chain from within other bridge elements is
> going to have issues and shouldn't be necessary.
> 
> The comment in dw-mipi-dsi post_disable[1]
> * TODO Only way found to call panel-bridge post_disable &
> * panel unprepare before the dsi "final" disable...
> * This needs to be fixed in the drm_bridge framework and the API
> * needs to be updated to manage our own call chains...
> 
> It has now been fixed up with pre_enable_prev_first.
> 
> I seem to recall seeing a patchset for one of the DSI hosts (other
> than vc4) that was moving the init from mode_set to pre_enable - I
> think it is probably [2] for msm.
> 
> Cheers
>    Dave
> 
> [1] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L862-L867
> [2] https://github.com/torvalds/linux/commit/ec7981e6c614254937b37ce0af9eac09901c05c5
> 
>> May be Neill, can comment here?
>>
>> Thanks,
>> Jagan.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 10/12] drm/bridge: Implement enable_next_first to alter bridge init order

[Jagan Teki]

Hi Dave,

On Thu, Mar 30, 2023 at 3:31 PM Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi Jagan
>
> On Thu, 30 Mar 2023 at 07:56, Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > On Wed, Mar 29, 2023 at 10:16 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > On Wed, Mar 29, 2023 at 05:28:28PM +0100, Dave Stevenson wrote:
> > > > On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > > >
> > > > > DSI sink devices typically send the MIPI-DCS commands to the DSI host
> > > > > via general MIPI_DSI_DCS read and write API.
> > > > >
> > > > > The classical DSI sequence mentioned that the DSI host receives MIPI-DCS
> > > > > commands from the DSI sink first in order to switch HS mode properly.
> > > > > Once the DSI host switches to HS mode any MIPI-DCS commands from the
> > > > > DSI sink are unfunctional.
> > > >
> > > > That statement contradicts the spec.
> > > > The DSI spec section 8.11.1 Transmission Packet Sequences says that
> > > > during any BLLP (Blanking or Low Power) period the host can do any of:
> > > > - remain in LP-11
> > > > - transmit one or more non-video packets from host to peripheral in escape mode
> > > > - transmit one or more non-video packets from host to peripheral in
> > > > using HS mode
> > > > - receive one or more packets from peripheral to host using escape mode
> > > > - transmit data on a different virtual channel.
> > > >
> > > > Indeed if the sink doesn't set MIPI_DSI_MODE_LPM /
> > > > MIPI_DSI_MSG_USE_LPM, then the expectation is that any data transfer
> > > > will be in HS mode.
> > > >
> > > > That makes me confused as to the need for this patch.
> > >
> > > Yeah, and it looks like that would break the expectation that, in
> > > enable, a bridge can expect its controller to be in HS mode.
> > >
> > > I think that was Jagan is trying to do is to work around an issue with
> > > the Allwinner DSI driver:
> > > https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L775
> >
> > Correct and I can see it seems to be a classic DSI sequence observed
> > in dw-mipi-dsi as well - based on Neil's comments.
> > https://lore.kernel.org/all/9aa3d19d-4378-aaf3-6857-c40be5d252c7@baylibre.com/
>
> Neil's comments are from 2021, and his response would appear to be
> with regard the PHY power up sequence issues that
> pre_enable_prev_first should solve. The DSI host pre_enable can now be
> called before the sink's pre_enable, therefore allowing the PHY to be
> configured in pre_enable. Hacking the PHY init into mode_set is
> therefore not required.
>
> I don't see any restriction in dw-mipi-dsi over when transfer can be
> called (as long as it is between pre_enable and post_disable), and it
> supports MIPI_DSI_MSG_USE_LPM for requesting the command be sent in
> either LP or HS mode.
>
> > In fact, I did follow and initialize the command-mode mode_set which
> > set low-speed DCS and switch back to video-mode @enable and switch to
> > HS but could see the same issue as the host cannot accept DCS as
> > before (I might implement improper sequence, but not sure due to lack
> > of documentation). But this sequence has issues with calling
> > post_disable twice even on dw-mipi-dsi.
>
> Calling up/down the bridge chain from within other bridge elements is
> going to have issues and shouldn't be necessary.
>
> The comment in dw-mipi-dsi post_disable[1]
> * TODO Only way found to call panel-bridge post_disable &
> * panel unprepare before the dsi "final" disable...
> * This needs to be fixed in the drm_bridge framework and the API
> * needs to be updated to manage our own call chains...
>
> It has now been fixed up with pre_enable_prev_first.

This seems not fixed in dw-mipi-dsi and it often still requires the
forth and back switch of HS mode even if pre_enable_prev_first.

Thanks,
Jagan.

Re: [PATCH v7 10/12] drm/bridge: Implement enable_next_first to alter bridge init order

[Jagan Teki]

Hi Dave,

On Thu, Mar 30, 2023 at 3:31 PM Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi Jagan
>
> On Thu, 30 Mar 2023 at 07:56, Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > On Wed, Mar 29, 2023 at 10:16 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > On Wed, Mar 29, 2023 at 05:28:28PM +0100, Dave Stevenson wrote:
> > > > On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > > >
> > > > > DSI sink devices typically send the MIPI-DCS commands to the DSI host
> > > > > via general MIPI_DSI_DCS read and write API.
> > > > >
> > > > > The classical DSI sequence mentioned that the DSI host receives MIPI-DCS
> > > > > commands from the DSI sink first in order to switch HS mode properly.
> > > > > Once the DSI host switches to HS mode any MIPI-DCS commands from the
> > > > > DSI sink are unfunctional.
> > > >
> > > > That statement contradicts the spec.
> > > > The DSI spec section 8.11.1 Transmission Packet Sequences says that
> > > > during any BLLP (Blanking or Low Power) period the host can do any of:
> > > > - remain in LP-11
> > > > - transmit one or more non-video packets from host to peripheral in escape mode
> > > > - transmit one or more non-video packets from host to peripheral in
> > > > using HS mode
> > > > - receive one or more packets from peripheral to host using escape mode
> > > > - transmit data on a different virtual channel.
> > > >
> > > > Indeed if the sink doesn't set MIPI_DSI_MODE_LPM /
> > > > MIPI_DSI_MSG_USE_LPM, then the expectation is that any data transfer
> > > > will be in HS mode.
> > > >
> > > > That makes me confused as to the need for this patch.
> > >
> > > Yeah, and it looks like that would break the expectation that, in
> > > enable, a bridge can expect its controller to be in HS mode.
> > >
> > > I think that was Jagan is trying to do is to work around an issue with
> > > the Allwinner DSI driver:
> > > https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L775
> >
> > Correct and I can see it seems to be a classic DSI sequence observed
> > in dw-mipi-dsi as well - based on Neil's comments.
> > https://lore.kernel.org/all/9aa3d19d-4378-aaf3-6857-c40be5d252c7@baylibre.com/
>
> Neil's comments are from 2021, and his response would appear to be
> with regard the PHY power up sequence issues that
> pre_enable_prev_first should solve. The DSI host pre_enable can now be
> called before the sink's pre_enable, therefore allowing the PHY to be
> configured in pre_enable. Hacking the PHY init into mode_set is
> therefore not required.
>
> I don't see any restriction in dw-mipi-dsi over when transfer can be
> called (as long as it is between pre_enable and post_disable), and it
> supports MIPI_DSI_MSG_USE_LPM for requesting the command be sent in
> either LP or HS mode.
>
> > In fact, I did follow and initialize the command-mode mode_set which
> > set low-speed DCS and switch back to video-mode @enable and switch to
> > HS but could see the same issue as the host cannot accept DCS as
> > before (I might implement improper sequence, but not sure due to lack
> > of documentation). But this sequence has issues with calling
> > post_disable twice even on dw-mipi-dsi.
>
> Calling up/down the bridge chain from within other bridge elements is
> going to have issues and shouldn't be necessary.
>
> The comment in dw-mipi-dsi post_disable[1]
> * TODO Only way found to call panel-bridge post_disable &
> * panel unprepare before the dsi "final" disable...
> * This needs to be fixed in the drm_bridge framework and the API
> * needs to be updated to manage our own call chains...
>
> It has now been fixed up with pre_enable_prev_first.

This seems not fixed in dw-mipi-dsi and it often still requires the
forth and back switch of HS mode even if pre_enable_prev_first.

Thanks,
Jagan.

Re: [PATCH v7 10/12] drm/bridge: Implement enable_next_first to alter bridge init order

[Jagan Teki]

Hi Dave,

On Thu, Mar 30, 2023 at 3:31 PM Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi Jagan
>
> On Thu, 30 Mar 2023 at 07:56, Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > On Wed, Mar 29, 2023 at 10:16 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > On Wed, Mar 29, 2023 at 05:28:28PM +0100, Dave Stevenson wrote:
> > > > On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > > >
> > > > > DSI sink devices typically send the MIPI-DCS commands to the DSI host
> > > > > via general MIPI_DSI_DCS read and write API.
> > > > >
> > > > > The classical DSI sequence mentioned that the DSI host receives MIPI-DCS
> > > > > commands from the DSI sink first in order to switch HS mode properly.
> > > > > Once the DSI host switches to HS mode any MIPI-DCS commands from the
> > > > > DSI sink are unfunctional.
> > > >
> > > > That statement contradicts the spec.
> > > > The DSI spec section 8.11.1 Transmission Packet Sequences says that
> > > > during any BLLP (Blanking or Low Power) period the host can do any of:
> > > > - remain in LP-11
> > > > - transmit one or more non-video packets from host to peripheral in escape mode
> > > > - transmit one or more non-video packets from host to peripheral in
> > > > using HS mode
> > > > - receive one or more packets from peripheral to host using escape mode
> > > > - transmit data on a different virtual channel.
> > > >
> > > > Indeed if the sink doesn't set MIPI_DSI_MODE_LPM /
> > > > MIPI_DSI_MSG_USE_LPM, then the expectation is that any data transfer
> > > > will be in HS mode.
> > > >
> > > > That makes me confused as to the need for this patch.
> > >
> > > Yeah, and it looks like that would break the expectation that, in
> > > enable, a bridge can expect its controller to be in HS mode.
> > >
> > > I think that was Jagan is trying to do is to work around an issue with
> > > the Allwinner DSI driver:
> > > https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L775
> >
> > Correct and I can see it seems to be a classic DSI sequence observed
> > in dw-mipi-dsi as well - based on Neil's comments.
> > https://lore.kernel.org/all/9aa3d19d-4378-aaf3-6857-c40be5d252c7@baylibre.com/
>
> Neil's comments are from 2021, and his response would appear to be
> with regard the PHY power up sequence issues that
> pre_enable_prev_first should solve. The DSI host pre_enable can now be
> called before the sink's pre_enable, therefore allowing the PHY to be
> configured in pre_enable. Hacking the PHY init into mode_set is
> therefore not required.
>
> I don't see any restriction in dw-mipi-dsi over when transfer can be
> called (as long as it is between pre_enable and post_disable), and it
> supports MIPI_DSI_MSG_USE_LPM for requesting the command be sent in
> either LP or HS mode.
>
> > In fact, I did follow and initialize the command-mode mode_set which
> > set low-speed DCS and switch back to video-mode @enable and switch to
> > HS but could see the same issue as the host cannot accept DCS as
> > before (I might implement improper sequence, but not sure due to lack
> > of documentation). But this sequence has issues with calling
> > post_disable twice even on dw-mipi-dsi.
>
> Calling up/down the bridge chain from within other bridge elements is
> going to have issues and shouldn't be necessary.
>
> The comment in dw-mipi-dsi post_disable[1]
> * TODO Only way found to call panel-bridge post_disable &
> * panel unprepare before the dsi "final" disable...
> * This needs to be fixed in the drm_bridge framework and the API
> * needs to be updated to manage our own call chains...
>
> It has now been fixed up with pre_enable_prev_first.

This seems not fixed in dw-mipi-dsi and it often still requires the
forth and back switch of HS mode even if pre_enable_prev_first.

Thanks,
Jagan.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel