[PATCH 0/2] DSI host and peripheral initialisation ordering

[PATCH 0/2] DSI host and peripheral initialisation ordering

[Dave Stevenson]

Hi All

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

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

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

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

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

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

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

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

Thanks
  Dave

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


Dave Stevenson (2):
  drm: Introduce DRM_BRIDGE_OP_UPSTREAM_FIRST to alter bridge init order
  drm/bridge: Document the expected behaviour of DSI host controllers

 Documentation/gpu/drm-kms-helpers.rst |   7 +
 drivers/gpu/drm/drm_bridge.c          | 235 ++++++++++++++++++++++++++++++----
 include/drm/drm_bridge.h              |   8 ++
 3 files changed, 225 insertions(+), 25 deletions(-)

-- 
2.7.4

[PATCH 1/2] drm: Introduce DRM_BRIDGE_OP_UPSTREAM_FIRST to alter bridge init order

[Dave Stevenson]

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

Add a new flag DRM_BRIDGE_OP_UPSTREAM_FIRST that any bridge can set
to swap the order of pre_enable (and post_disable) so that any upstream
bridges are called first to create the desired state.

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

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

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index c96847fc0ebc..7c24e8340efa 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -522,21 +522,58 @@ EXPORT_SYMBOL(drm_bridge_chain_disable);
  * Calls &drm_bridge_funcs.post_disable op for all the bridges in the
  * encoder chain, starting from the first bridge to the last. These are called
  * after completing the encoder's prepare op.
+ * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the post_disable for
+ * that bridge will be called before the previous one to reverse the pre_enable
+ * calling direction.
  *
  * Note: the bridge passed should be the one closest to the encoder
  */
 void drm_bridge_chain_post_disable(struct drm_bridge *bridge)
 {
 	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) {
+		limit = NULL;
+
+		if (!list_is_last(&bridge->chain_node, &encoder->bridge_chain)) {
+			next = list_next_entry(bridge, chain_node);
+
+			if (next->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
+				limit = next;
+
+				list_for_each_entry_from(next, &encoder->bridge_chain,
+							 chain_node) {
+					if (!(next->ops &
+						DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
+						next = list_prev_entry(next, chain_node);
+						limit = next;
+						break;
+					}
+				}
+
+				list_for_each_entry_from_reverse(next, &encoder->bridge_chain,
+								 chain_node) {
+					if (next == bridge)
+						break;
+
+					if (next->funcs->post_disable)
+						next->funcs->post_disable(next);
+				}
+			}
+		}
+
 		if (bridge->funcs->post_disable)
 			bridge->funcs->post_disable(bridge);
+
+		if (limit)
+			bridge = limit;
 	}
+
 }
 EXPORT_SYMBOL(drm_bridge_chain_post_disable);
 
@@ -577,22 +614,53 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set);
  * Calls &drm_bridge_funcs.pre_enable op for all the bridges in the encoder
  * chain, starting from the last bridge to the first. These are called
  * before calling the encoder's commit op.
+ * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the pre_enable for
+ * the previous bridge will be called before pre_enable of this bridge.
  *
  * Note: the bridge passed should be the one closest to the encoder
  */
 void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
 {
 	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->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
+			next = iter;
+			limit = bridge;
+			list_for_each_entry_from_reverse(next,
+							 &encoder->bridge_chain,
+							 chain_node) {
+				if (next == bridge)
+					break;
+
+				if (!(next->ops &
+					DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
+					limit = list_prev_entry(next, chain_node);
+					break;
+				}
+			}
+
+			list_for_each_entry_from(next, &encoder->bridge_chain, chain_node) {
+				if (next == iter)
+					break;
+
+				if (next->funcs->pre_enable)
+					next->funcs->pre_enable(next);
+			}
+		}
+
 		if (iter->funcs->pre_enable)
 			iter->funcs->pre_enable(iter);
 
+		if (iter->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST)
+			iter = limit;
+
 		if (iter == bridge)
 			break;
 	}
@@ -667,6 +735,25 @@ void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge,
 }
 EXPORT_SYMBOL(drm_atomic_bridge_chain_disable);
 
+static void drm_atomic_bridge_call_post_disable(struct drm_bridge *bridge,
+						struct drm_atomic_state *old_state)
+{
+	if (bridge->funcs->atomic_post_disable) {
+		struct drm_bridge_state *old_bridge_state;
+
+		old_bridge_state =
+			drm_atomic_get_old_bridge_state(old_state,
+							bridge);
+		if (WARN_ON(!old_bridge_state))
+			return;
+
+		bridge->funcs->atomic_post_disable(bridge,
+						   old_bridge_state);
+	} else if (bridge->funcs->post_disable) {
+		bridge->funcs->post_disable(bridge);
+	}
+}
+
 /**
  * drm_atomic_bridge_chain_post_disable - cleans up after disabling all bridges
  *					  in the encoder chain
@@ -677,6 +764,9 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_disable);
  * &drm_bridge_funcs.post_disable) op for all the bridges in the encoder chain,
  * starting from the first bridge to the last. These are called after completing
  * &drm_encoder_helper_funcs.atomic_disable
+ * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the post_disable for
+ * that bridge will be called before the previous one to reverse the pre_enable
+ * calling direction.
  *
  * Note: the bridge passed should be the one closest to the encoder
  */
@@ -684,30 +774,69 @@ void drm_atomic_bridge_chain_post_disable(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_post_disable) {
-			struct drm_bridge_state *old_bridge_state;
+		limit = NULL;
+
+		if (!list_is_last(&bridge->chain_node, &encoder->bridge_chain)) {
+			next = list_next_entry(bridge, chain_node);
+
+			if (next->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
+				limit = next;
+
+				list_for_each_entry_from(next, &encoder->bridge_chain,
+							 chain_node) {
+					if (!(next->ops &
+						DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
+						next = list_prev_entry(next, chain_node);
+						limit = next;
+						break;
+					}
+				}
+
+				list_for_each_entry_from_reverse(next, &encoder->bridge_chain,
+								 chain_node) {
+					if (next == bridge)
+						break;
+
+					drm_atomic_bridge_call_post_disable(next,
+									    old_state);
+				}
+			}
+		}
 
-			old_bridge_state =
-				drm_atomic_get_old_bridge_state(old_state,
-								bridge);
-			if (WARN_ON(!old_bridge_state))
-				return;
+		drm_atomic_bridge_call_post_disable(bridge, old_state);
 
-			bridge->funcs->atomic_post_disable(bridge,
-							   old_bridge_state);
-		} else if (bridge->funcs->post_disable) {
-			bridge->funcs->post_disable(bridge);
-		}
+		if (limit)
+			bridge = limit;
 	}
 }
 EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
 
+static void drm_atomic_bridge_call_pre_enable(struct drm_bridge *bridge,
+					      struct drm_atomic_state *old_state)
+{
+	if (bridge->funcs->atomic_pre_enable) {
+		struct drm_bridge_state *old_bridge_state;
+
+		old_bridge_state =
+			drm_atomic_get_old_bridge_state(old_state,
+							bridge);
+		if (WARN_ON(!old_bridge_state))
+			return;
+
+		bridge->funcs->atomic_pre_enable(bridge, old_bridge_state);
+	} else if (bridge->funcs->pre_enable) {
+		bridge->funcs->pre_enable(bridge);
+	}
+}
+
 /**
  * drm_atomic_bridge_chain_pre_enable - prepares for enabling all bridges in
  *					the encoder chain
@@ -718,6 +847,8 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
  * &drm_bridge_funcs.pre_enable) op for all the bridges in the encoder chain,
  * starting from the last bridge to the first. These are called before calling
  * &drm_encoder_helper_funcs.atomic_enable
+ * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the pre_enable for
+ * the previous bridge will be called before pre_enable of this bridge.
  *
  * Note: the bridge passed should be the one closest to the encoder
  */
@@ -725,26 +856,42 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
 					struct drm_atomic_state *old_state)
 {
 	struct drm_encoder *encoder;
-	struct drm_bridge *iter;
+	struct drm_bridge *iter, *next, *limit;
 
 	if (!bridge)
 		return;
 
 	encoder = bridge->encoder;
+
 	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
-		if (iter->funcs->atomic_pre_enable) {
-			struct drm_bridge_state *old_bridge_state;
+		if (iter->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
+			next = iter;
+			limit = bridge;
+			list_for_each_entry_from_reverse(next,
+							 &encoder->bridge_chain,
+							 chain_node) {
+				if (next == bridge)
+					break;
+
+				if (!(next->ops &
+					DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
+					limit = list_prev_entry(next, chain_node);
+					break;
+				}
+			}
+
+			list_for_each_entry_from(next, &encoder->bridge_chain, chain_node) {
+				if (next == iter)
+					break;
+
+				drm_atomic_bridge_call_pre_enable(next, old_state);
+			}
+		}
 
-			old_bridge_state =
-				drm_atomic_get_old_bridge_state(old_state,
-								iter);
-			if (WARN_ON(!old_bridge_state))
-				return;
+		drm_atomic_bridge_call_pre_enable(iter, old_state);
 
-			iter->funcs->atomic_pre_enable(iter, old_bridge_state);
-		} else if (iter->funcs->pre_enable) {
-			iter->funcs->pre_enable(iter);
-		}
+		if (iter->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST)
+			iter = limit;
 
 		if (iter == bridge)
 			break;
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index f27b4060faa2..523ec9d8f3f8 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -725,6 +725,14 @@ enum drm_bridge_ops {
 	 * this flag shall implement the &drm_bridge_funcs->get_modes callback.
 	 */
 	DRM_BRIDGE_OP_MODES = BIT(3),
+	/**
+	 * @DRM_BRIDGE_OP_UPSTREAM_FIRST: The bridge can requires
+	 * that the upstream node pre_enable is called before its pre_enable,
+	 * and conversely for post_disables. This is most frequently a
+	 * requirement for DSI devices which need the host to be initialised
+	 * before them.
+	 */
+	DRM_BRIDGE_OP_UPSTREAM_FIRST = BIT(4),
 };
 
 /**
-- 
2.7.4

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

[Dave Stevenson]

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

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 Documentation/gpu/drm-kms-helpers.rst |  7 +++++++
 drivers/gpu/drm/drm_bridge.c          | 38 +++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

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

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

[Maxime Ripard]

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

Hi Dave,

On Wed, Feb 16, 2022 at 04:59:42PM +0000, Dave Stevenson wrote:
> Hi All
> 
> Hopefully I've cc'ed all those that have bashed this problem around previously,
> or are otherwise linked to DRM bridges.
> 
> There have been numerous discussions around how DSI support is currently broken
> as it doesn't support initialising the PHY to LP-11 and potentially the clock
> lane to HS prior to configuring the DSI peripheral. There is no op where the
> interface is initialised but HS video isn't also being sent.
> Currently you have:
> - peripheral pre_enable (host not initialised yet)
> - host pre_enable
> - encoder enable
> - host enable
> - peripheral enable (video already running)
> 
> vc4 and exynos currently implement the DSI host as an encoder, and split the
> bridge_chain. This fails if you want to switch to being a bridge and/or use
> atomic calls as the state of all the elements split off are not added by
> drm_atomic_add_encoder_bridges.
> 
> dw-mipi-dsi[1] and now msm[2] use the mode_set hook to initialise the PHY, so
> the bridge/panel pre_enable can send commands. In their post_disable they then
> call the downstream bridge/panel post_disable op manually so that shutdown
> commands can be sent before shutting down the PHY. Nothing handles that fact,
> so the framework then continues down the bridge chain and calls the post_disable
> again, so we get unbalanced panel prepare/unprepare calls being reported [3].
> 
> There have been patches[4] proposing reversing the entire direction of
> pre_enable and post_disable, but that risks driving voltage into devices that
> have yet to be powered up.
> There have been discussions about adding either a pre_pre_enable, or adding a
> DSI host_op to initialise the host[5]. Both require significant reworking to all
> existing drivers in moving initialisation phases.
> We have patches that look like they may well be addressing race conditions in
> starting up a DSI peripheral[6].
> 
> This patch takes a hybrid of the two: an optional reversing of the order for
> specific links within the bridge chain within pre_enable and post_disable done
> within the drm_bridge framework.
> I'm more than happy to move where the flag exists in structures (currently as
> DRM_BRIDGE_OP_UPSTREAM_FIRST in drm_bridge_ops, but it isn't an op), but does
> this solve the problem posed? If not, then can you describe the actual scenario
> it doesn't cover?
> A DSI peripheral can set the flag to get the DSI host initialised first, and
> therefore it has a stable LP-11 state before pre_enable. Likewise the peripheral
> can still send shutdown commands prior to the DSI host being shut down in
> post_disable. It also handles the case where there are multiple devices in the
> chain that all want their upstream bridge enabled first, so should there be a
> DSI mux between host and peripheral, then it can still get the host to the
> correct state.
> 
> An example tree is at [7] which is drm-misc-next with these patches and then a
> conversion of vc4_dsi to use the atomic bridge functions (will be upstreamed
> once we're over this hurdle). It is working happily with the Toshiba TC358762 on
> a Raspberry Pi 7" panel.
> The same approach but on our vendor 5.15 tree[8] has also been tested
> successfully on a TI SN65DSI83 and LVDS panel.
> 
> Whilst here, I've also documented the expected behaviour of DSI hosts and
> peripherals to aid those who come along after.

I'd still like the review of someone with a bit more knowledge in the
bridge framework, but it is

Acked-by: Maxime Ripard <maxime@cerno.tech>

Thanks!
Maxime

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

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

[Dmitry Baryshkov]

On Wed, 16 Feb 2022 at 20:00, Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi All
>
> Hopefully I've cc'ed all those that have bashed this problem around previously,
> or are otherwise linked to DRM bridges.
>
> There have been numerous discussions around how DSI support is currently broken
> as it doesn't support initialising the PHY to LP-11 and potentially the clock
> lane to HS prior to configuring the DSI peripheral. There is no op where the
> interface is initialised but HS video isn't also being sent.
> Currently you have:
> - peripheral pre_enable (host not initialised yet)
> - host pre_enable
> - encoder enable
> - host enable
> - peripheral enable (video already running)
>
> vc4 and exynos currently implement the DSI host as an encoder, and split the
> bridge_chain. This fails if you want to switch to being a bridge and/or use
> atomic calls as the state of all the elements split off are not added by
> drm_atomic_add_encoder_bridges.
>
> dw-mipi-dsi[1] and now msm[2] use the mode_set hook to initialise the PHY, so
> the bridge/panel pre_enable can send commands. In their post_disable they then
> call the downstream bridge/panel post_disable op manually so that shutdown
> commands can be sent before shutting down the PHY. Nothing handles that fact,
> so the framework then continues down the bridge chain and calls the post_disable
> again, so we get unbalanced panel prepare/unprepare calls being reported [3].
>
> There have been patches[4] proposing reversing the entire direction of
> pre_enable and post_disable, but that risks driving voltage into devices that
> have yet to be powered up.
> There have been discussions about adding either a pre_pre_enable, or adding a
> DSI host_op to initialise the host[5]. Both require significant reworking to all
> existing drivers in moving initialisation phases.
> We have patches that look like they may well be addressing race conditions in
> starting up a DSI peripheral[6].

I have been thinking about a similar approach:
- Add power_up() and power_down() ops to drm_bridge_ops
- Add separate drm_bridge_chain_power_up() and _power_down() with
power_up being called from encoder to the connector end of the chain
and power_down being called in the opposite direction.
- Make those functions call power_up()/power_down() if it's available
and call pre_enable()/post_disable() if it is not
- Make drm_bridge_chain_pre_enable()/_post_disable() call
pre_enable()/post_disable() only if power_up()/power_down() are not
present.

This removes the immediate need for the rework of the drivers.
On the other hand the DSI hosts can be brought up in the power_up
stage (to the LP-11). Then bridge's pre_enable() can communicate with
the actual hardware in time.
So does bridge's post_disable()


> This patch takes a hybrid of the two: an optional reversing of the order for
> specific links within the bridge chain within pre_enable and post_disable done
> within the drm_bridge framework.
> I'm more than happy to move where the flag exists in structures (currently as
> DRM_BRIDGE_OP_UPSTREAM_FIRST in drm_bridge_ops, but it isn't an op), but does
> this solve the problem posed? If not, then can you describe the actual scenario
> it doesn't cover?

My general feeling is that this makes the chain traversing functions
unnecessarily complex.

> A DSI peripheral can set the flag to get the DSI host initialised first, and
> therefore it has a stable LP-11 state before pre_enable. Likewise the peripheral
> can still send shutdown commands prior to the DSI host being shut down in
> post_disable. It also handles the case where there are multiple devices in the
> chain that all want their upstream bridge enabled first, so should there be a
> DSI mux between host and peripheral, then it can still get the host to the
> correct state.
>
> An example tree is at [7] which is drm-misc-next with these patches and then a
> conversion of vc4_dsi to use the atomic bridge functions (will be upstreamed
> once we're over this hurdle). It is working happily with the Toshiba TC358762 on
> a Raspberry Pi 7" panel.
> The same approach but on our vendor 5.15 tree[8] has also been tested
> successfully on a TI SN65DSI83 and LVDS panel.
>
> Whilst here, I've also documented the expected behaviour of DSI hosts and
> peripherals to aid those who come along after.
>
> Thanks
>   Dave
>
> [1] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L940
> [2] https://lists.freedesktop.org/archives/dri-devel/2022-January/337769.html
> [3] https://lists.freedesktop.org/archives/dri-devel/2021-December/333908.html
> [4] https://lists.freedesktop.org/archives/dri-devel/2021-October/328476.html
> [5] https://lists.freedesktop.org/archives/dri-devel/2021-October/325853.html
> [6] https://lists.freedesktop.org/archives/dri-devel/2022-February/341852.html
> [7] https://github.com/6by9/linux/tree/drm-misc-next-vc4_dsi
> [8] https://github.com/6by9/linux/tree/rpi-5.15.y-sn65dsi83
>
>
> Dave Stevenson (2):
>   drm: Introduce DRM_BRIDGE_OP_UPSTREAM_FIRST to alter bridge init order
>   drm/bridge: Document the expected behaviour of DSI host controllers
>
>  Documentation/gpu/drm-kms-helpers.rst |   7 +
>  drivers/gpu/drm/drm_bridge.c          | 235 ++++++++++++++++++++++++++++++----
>  include/drm/drm_bridge.h              |   8 ++
>  3 files changed, 225 insertions(+), 25 deletions(-)
>
> --
> 2.7.4
>


--
With best wishes
Dmitry

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

[Andrzej Hajda]

Hi,

On 16.02.2022 17:59, Dave Stevenson wrote:
> Hi All
>
> Hopefully I've cc'ed all those that have bashed this problem around previously,
> or are otherwise linked to DRM bridges.
>
> There have been numerous discussions around how DSI support is currently broken
> as it doesn't support initialising the PHY to LP-11 and potentially the clock
> lane to HS prior to configuring the DSI peripheral. There is no op where the
> interface is initialised but HS video isn't also being sent.
> Currently you have:
> - peripheral pre_enable (host not initialised yet)
> - host pre_enable
> - encoder enable
> - host enable
> - peripheral enable (video already running)
>
> vc4 and exynos currently implement the DSI host as an encoder, and split the
> bridge_chain. This fails if you want to switch to being a bridge and/or use
> atomic calls as the state of all the elements split off are not added by
> drm_atomic_add_encoder_bridges.
>
> dw-mipi-dsi[1] and now msm[2] use the mode_set hook to initialise the PHY, so
> the bridge/panel pre_enable can send commands. In their post_disable they then
> call the downstream bridge/panel post_disable op manually so that shutdown
> commands can be sent before shutting down the PHY. Nothing handles that fact,
> so the framework then continues down the bridge chain and calls the post_disable
> again, so we get unbalanced panel prepare/unprepare calls being reported [3].
>
> There have been patches[4] proposing reversing the entire direction of
> pre_enable and post_disable, but that risks driving voltage into devices that
> have yet to be powered up.
> There have been discussions about adding either a pre_pre_enable, or adding a
> DSI host_op to initialise the host[5]. Both require significant reworking to all
> existing drivers in moving initialisation phases.
> We have patches that look like they may well be addressing race conditions in
> starting up a DSI peripheral[6].
>
> This patch takes a hybrid of the two: an optional reversing of the order for
> specific links within the bridge chain within pre_enable and post_disable done
> within the drm_bridge framework.
> I'm more than happy to move where the flag exists in structures (currently as
> DRM_BRIDGE_OP_UPSTREAM_FIRST in drm_bridge_ops, but it isn't an op), but does
> this solve the problem posed? If not, then can you describe the actual scenario
> it doesn't cover?
> A DSI peripheral can set the flag to get the DSI host initialised first, and
> therefore it has a stable LP-11 state before pre_enable. Likewise the peripheral
> can still send shutdown commands prior to the DSI host being shut down in
> post_disable. It also handles the case where there are multiple devices in the
> chain that all want their upstream bridge enabled first, so should there be a
> DSI mux between host and peripheral, then it can still get the host to the
> correct state.
>
> An example tree is at [7] which is drm-misc-next with these patches and then a
> conversion of vc4_dsi to use the atomic bridge functions (will be upstreamed
> once we're over this hurdle). It is working happily with the Toshiba TC358762 on
> a Raspberry Pi 7" panel.
> The same approach but on our vendor 5.15 tree[8] has also been tested
> successfully on a TI SN65DSI83 and LVDS panel.
>
> Whilst here, I've also documented the expected behaviour of DSI hosts and
> peripherals to aid those who come along after.

Good summary, of multiple attempts of solving the issue (however I still 
could add some more :) ).
I think the main issue is that we try to squeeze different hardware 
protocol requirements into one quite restrictive framework - whole 
crtc->encoder->bridges->(panel ||connector) is managed directly by drm core.
No place to negotiate configuration directly between players 
(bridges/panels).
This patchset slightly looses the restrictions, so hopefully will help 
for some time, but still every developer needs to solve riddles what to 
put into callbacks, to allow driver working in different pipelines.
<DREAM MODE ON>
Ideally I would like to drop idea of the bridge/panel and build 
abstraction on data links.
So for example DSI/EDP bridge during probe would register DSI sink with 
their ops, and EDP source with their ops or just look for EDP sink (what 
will suit better).
To establish data link they could use their ops and helpers to provide 
two-way conversation.
This way if we need add support for new data link type or extend 
existing one we do not need to touch whole framework and pray to not 
break some strange bridge, or to add ops which will not be used by most 
of users.
<DREAM MODE OFF>

Putting dreams off, I think this patchset can add some value, at the 
price of call chain complication. Lets see opinion of others.

Regards
Andrzej

>
> Thanks
>    Dave
>
> [1] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L940
> [2] https://lists.freedesktop.org/archives/dri-devel/2022-January/337769.html
> [3] https://lists.freedesktop.org/archives/dri-devel/2021-December/333908.html
> [4] https://lists.freedesktop.org/archives/dri-devel/2021-October/328476.html
> [5] https://lists.freedesktop.org/archives/dri-devel/2021-October/325853.html
> [6] https://lists.freedesktop.org/archives/dri-devel/2022-February/341852.html
> [7] https://github.com/6by9/linux/tree/drm-misc-next-vc4_dsi
> [8] https://github.com/6by9/linux/tree/rpi-5.15.y-sn65dsi83
>
>
> Dave Stevenson (2):
>    drm: Introduce DRM_BRIDGE_OP_UPSTREAM_FIRST to alter bridge init order
>    drm/bridge: Document the expected behaviour of DSI host controllers
>
>   Documentation/gpu/drm-kms-helpers.rst |   7 +
>   drivers/gpu/drm/drm_bridge.c          | 235 ++++++++++++++++++++++++++++++----
>   include/drm/drm_bridge.h              |   8 ++
>   3 files changed, 225 insertions(+), 25 deletions(-)
>

Re: [PATCH 2/2] drm/bridge: Document the expected behaviour of DSI host controllers

[Laurent Pinchart]

Hi Dave,

Thank you for the patch.


On Wed, Feb 16, 2022 at 04:59:44PM +0000, Dave Stevenson wrote:
> The exact behaviour of DSI host controllers is not specified,
> therefore define it.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  Documentation/gpu/drm-kms-helpers.rst |  7 +++++++
>  drivers/gpu/drm/drm_bridge.c          | 38 +++++++++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
> index c3ce91eecbc1..362afdb867c6 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -185,6 +185,13 @@ Bridge Helper Reference
>  .. kernel-doc:: drivers/gpu/drm/drm_bridge.c
>     :export:
>  
> +MIPI-DSI bridge operation
> +-------------------------
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_bridge.c
> +   :doc: dsi bridge operations
> +
> +
>  Bridge Connector Helper Reference
>  ---------------------------------
>  
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 7c24e8340efa..14c2ee9e0328 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -152,6 +152,44 @@
>   * situation when probing.
>   */
>  
> +/**
> + * DOC: dsi bridge operations
> + *
> + * DSI host interfaces are expected to be implemented as bridges rather than
> + * encoders, however there are a few aspects of their operation that need to
> + * be defined in order to provide a consistent interface.
> + *
> + * A DSI host should keep the PHY powered down until the pre_enable op is

I'd write "operation" in full everywhere to avoid mixing the two.

> + * called. All lanes should be in an idle state (not LP-11) up to this point.

Is the idle state LP-00 ? If so I'd state that explicitly.

"[...] in an idle state (LP-00, not LP-11) [...]"

> + * pre_enable should initialise the PHY, set the data lanes to LP-11, and the
> + * clock lane to either LP-11 or HS dependent on the mode_flag

s/dependent/depending/ ?

> + * MIPI_DSI_CLOCK_NON_CONTINUOUS.
> + *
> + * Ordinarily the downstream bridge DSI peripheral pre_enable will have been
> + * called before the DSI host. If the DSI peripheral requires LP-11 and/or
> + * the clock lane to be in HS mode prior to pre_enable, then it can set the
> + * DRM_BRIDGE_OP_UPSTREAM_FIRST flag to request the pre_enable (and
> + * post_disable) order to be altered to enable the DSI host first.
> + *
> + * Either the CRTC being enabled, or the DSI host enable op should switch the
> + * host to actively transmitting video on the data lanes.
> + *
> + * The reverse also applies. The DSI host disable op or stopping the CRTC should
> + * stop transmitting video, and the data lanes should return to the LP-11 state.
> + * The DSI host post_disable op should disable the PHY.
> + * If the DRM_BRIDGE_OP_UPSTREAM_FIRST flag is set, then the DSI peripheral's
> + * bridge post_disable will be called before the DSI host's post_disable.
> + *
> + * Whilst it is valid to call host_transfer prior to pre_enable or after
> + * post_disable, the exact state of the lanes is undefined at this point. The
> + * DSI host should initialise the interface, transmit the data, and then disable
> + * the interface again.
> + *
> + * Ultra Low Power State (ULPS) is not explicitly supported by DRM. If
> + * implemented, it therefore needs to be either handled entirely within the DSI

s/either // (or you need an "or ..." :-))

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> + * Host driver.
> + */
> +
>  static DEFINE_MUTEX(bridge_lock);
>  static LIST_HEAD(bridge_list);
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] drm: Introduce DRM_BRIDGE_OP_UPSTREAM_FIRST to alter bridge init order

[Laurent Pinchart]

Hi Dave,

Thank you for the patch.

On Wed, Feb 16, 2022 at 04:59:43PM +0000, Dave Stevenson wrote:
> DSI sink devices typically want the DSI host powered up and configured
> before they are powered up. pre_enable is the place this would normally
> happen, but they are called in reverse order from panel/connector towards
> the encoder, which is the "wrong" order.
> 
> Add a new flag DRM_BRIDGE_OP_UPSTREAM_FIRST that any bridge can set
> to swap the order of pre_enable (and post_disable) so that any upstream
> bridges are called first to create the desired state.
> 
> eg:
> - Panel
> - Bridge 1
> - Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST
> - Bridge 3
> - Encoder
> Would result in pre_enable's being called as Panel, Bridge 1, Bridge 3,
> Bridge 2.

If there was a Bridge 4 between Bridge 3 and Encoder, would it be 

Panel, Bridge 1, Bridge 3, Bridge 4, Bridge 2

? I'd capture that here, to be explicit.

> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/gpu/drm/drm_bridge.c | 197 +++++++++++++++++++++++++++++++++++++------
>  include/drm/drm_bridge.h     |   8 ++
>  2 files changed, 180 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index c96847fc0ebc..7c24e8340efa 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -522,21 +522,58 @@ EXPORT_SYMBOL(drm_bridge_chain_disable);
>   * Calls &drm_bridge_funcs.post_disable op for all the bridges in the
>   * encoder chain, starting from the first bridge to the last. These are called
>   * after completing the encoder's prepare op.

Missing blank line, as well as in three locations below.

> + * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the post_disable for
> + * that bridge will be called before the previous one to reverse the pre_enable
> + * calling direction.
>   *
>   * Note: the bridge passed should be the one closest to the encoder
>   */
>  void drm_bridge_chain_post_disable(struct drm_bridge *bridge)
>  {
>  	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) {
> +		limit = NULL;
> +
> +		if (!list_is_last(&bridge->chain_node, &encoder->bridge_chain)) {
> +			next = list_next_entry(bridge, chain_node);
> +
> +			if (next->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
> +				limit = next;
> +
> +				list_for_each_entry_from(next, &encoder->bridge_chain,
> +							 chain_node) {
> +					if (!(next->ops &
> +						DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
> +						next = list_prev_entry(next, chain_node);
> +						limit = next;
> +						break;
> +					}
> +				}
> +
> +				list_for_each_entry_from_reverse(next, &encoder->bridge_chain,
> +								 chain_node) {
> +					if (next == bridge)
> +						break;
> +
> +					if (next->funcs->post_disable)
> +						next->funcs->post_disable(next);
> +				}
> +			}
> +		}
> +
>  		if (bridge->funcs->post_disable)
>  			bridge->funcs->post_disable(bridge);
> +
> +		if (limit)
> +			bridge = limit;
>  	}
> +
>  }
>  EXPORT_SYMBOL(drm_bridge_chain_post_disable);
>  
> @@ -577,22 +614,53 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set);
>   * Calls &drm_bridge_funcs.pre_enable op for all the bridges in the encoder
>   * chain, starting from the last bridge to the first. These are called
>   * before calling the encoder's commit op.
> + * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the pre_enable for
> + * the previous bridge will be called before pre_enable of this bridge.
>   *
>   * Note: the bridge passed should be the one closest to the encoder
>   */
>  void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
>  {
>  	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->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
> +			next = iter;
> +			limit = bridge;
> +			list_for_each_entry_from_reverse(next,
> +							 &encoder->bridge_chain,
> +							 chain_node) {
> +				if (next == bridge)
> +					break;
> +
> +				if (!(next->ops &
> +					DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
> +					limit = list_prev_entry(next, chain_node);
> +					break;
> +				}
> +			}
> +
> +			list_for_each_entry_from(next, &encoder->bridge_chain, chain_node) {
> +				if (next == iter)
> +					break;
> +
> +				if (next->funcs->pre_enable)
> +					next->funcs->pre_enable(next);
> +			}
> +		}
> +
>  		if (iter->funcs->pre_enable)
>  			iter->funcs->pre_enable(iter);
>  
> +		if (iter->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST)
> +			iter = limit;
> +
>  		if (iter == bridge)
>  			break;
>  	}
> @@ -667,6 +735,25 @@ void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge,
>  }
>  EXPORT_SYMBOL(drm_atomic_bridge_chain_disable);
>  
> +static void drm_atomic_bridge_call_post_disable(struct drm_bridge *bridge,
> +						struct drm_atomic_state *old_state)
> +{
> +	if (bridge->funcs->atomic_post_disable) {
> +		struct drm_bridge_state *old_bridge_state;
> +
> +		old_bridge_state =
> +			drm_atomic_get_old_bridge_state(old_state,
> +							bridge);
> +		if (WARN_ON(!old_bridge_state))
> +			return;
> +
> +		bridge->funcs->atomic_post_disable(bridge,
> +						   old_bridge_state);
> +	} else if (bridge->funcs->post_disable) {
> +		bridge->funcs->post_disable(bridge);
> +	}
> +}
> +
>  /**
>   * drm_atomic_bridge_chain_post_disable - cleans up after disabling all bridges
>   *					  in the encoder chain
> @@ -677,6 +764,9 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_disable);
>   * &drm_bridge_funcs.post_disable) op for all the bridges in the encoder chain,
>   * starting from the first bridge to the last. These are called after completing
>   * &drm_encoder_helper_funcs.atomic_disable
> + * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the post_disable for
> + * that bridge will be called before the previous one to reverse the pre_enable
> + * calling direction.
>   *
>   * Note: the bridge passed should be the one closest to the encoder
>   */
> @@ -684,30 +774,69 @@ void drm_atomic_bridge_chain_post_disable(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_post_disable) {
> -			struct drm_bridge_state *old_bridge_state;
> +		limit = NULL;
> +
> +		if (!list_is_last(&bridge->chain_node, &encoder->bridge_chain)) {
> +			next = list_next_entry(bridge, chain_node);
> +
> +			if (next->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
> +				limit = next;
> +
> +				list_for_each_entry_from(next, &encoder->bridge_chain,
> +							 chain_node) {
> +					if (!(next->ops &
> +						DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
> +						next = list_prev_entry(next, chain_node);
> +						limit = next;
> +						break;
> +					}
> +				}
> +
> +				list_for_each_entry_from_reverse(next, &encoder->bridge_chain,
> +								 chain_node) {
> +					if (next == bridge)
> +						break;
> +
> +					drm_atomic_bridge_call_post_disable(next,
> +									    old_state);
> +				}
> +			}
> +		}
>  
> -			old_bridge_state =
> -				drm_atomic_get_old_bridge_state(old_state,
> -								bridge);
> -			if (WARN_ON(!old_bridge_state))
> -				return;
> +		drm_atomic_bridge_call_post_disable(bridge, old_state);
>  
> -			bridge->funcs->atomic_post_disable(bridge,
> -							   old_bridge_state);
> -		} else if (bridge->funcs->post_disable) {
> -			bridge->funcs->post_disable(bridge);
> -		}
> +		if (limit)
> +			bridge = limit;
>  	}
>  }
>  EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
>  
> +static void drm_atomic_bridge_call_pre_enable(struct drm_bridge *bridge,
> +					      struct drm_atomic_state *old_state)
> +{
> +	if (bridge->funcs->atomic_pre_enable) {
> +		struct drm_bridge_state *old_bridge_state;
> +
> +		old_bridge_state =
> +			drm_atomic_get_old_bridge_state(old_state,
> +							bridge);
> +		if (WARN_ON(!old_bridge_state))
> +			return;
> +
> +		bridge->funcs->atomic_pre_enable(bridge, old_bridge_state);
> +	} else if (bridge->funcs->pre_enable) {
> +		bridge->funcs->pre_enable(bridge);
> +	}
> +}
> +
>  /**
>   * drm_atomic_bridge_chain_pre_enable - prepares for enabling all bridges in
>   *					the encoder chain
> @@ -718,6 +847,8 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
>   * &drm_bridge_funcs.pre_enable) op for all the bridges in the encoder chain,
>   * starting from the last bridge to the first. These are called before calling
>   * &drm_encoder_helper_funcs.atomic_enable
> + * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the pre_enable for
> + * the previous bridge will be called before pre_enable of this bridge.
>   *
>   * Note: the bridge passed should be the one closest to the encoder
>   */
> @@ -725,26 +856,42 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
>  					struct drm_atomic_state *old_state)
>  {
>  	struct drm_encoder *encoder;
> -	struct drm_bridge *iter;
> +	struct drm_bridge *iter, *next, *limit;
>  
>  	if (!bridge)
>  		return;
>  
>  	encoder = bridge->encoder;
> +
>  	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> -		if (iter->funcs->atomic_pre_enable) {
> -			struct drm_bridge_state *old_bridge_state;
> +		if (iter->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
> +			next = iter;
> +			limit = bridge;
> +			list_for_each_entry_from_reverse(next,
> +							 &encoder->bridge_chain,
> +							 chain_node) {
> +				if (next == bridge)
> +					break;
> +
> +				if (!(next->ops &
> +					DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
> +					limit = list_prev_entry(next, chain_node);
> +					break;
> +				}
> +			}
> +
> +			list_for_each_entry_from(next, &encoder->bridge_chain, chain_node) {
> +				if (next == iter)
> +					break;
> +
> +				drm_atomic_bridge_call_pre_enable(next, old_state);
> +			}
> +		}

This is hard to understand, I have trouble figuring out if it does the
right thing when multiple bridges set the DRM_BRIDGE_OP_UPSTREAM_FIRST
flag (or actually even when a single bridge does so). Comments would
help, but I wonder if it wouldn't be simpler to switch to a recursive
implementation.

It also seems that merging the legacy and atomic versions of the code
would be a good idea. They could both call into a shared implementation,
with the legacy version passing a NULL state, and the atomic op being
considered only if the state is not NULL.

>  
> -			old_bridge_state =
> -				drm_atomic_get_old_bridge_state(old_state,
> -								iter);
> -			if (WARN_ON(!old_bridge_state))
> -				return;
> +		drm_atomic_bridge_call_pre_enable(iter, old_state);
>  
> -			iter->funcs->atomic_pre_enable(iter, old_bridge_state);
> -		} else if (iter->funcs->pre_enable) {
> -			iter->funcs->pre_enable(iter);
> -		}
> +		if (iter->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST)
> +			iter = limit;
>  
>  		if (iter == bridge)
>  			break;
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index f27b4060faa2..523ec9d8f3f8 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -725,6 +725,14 @@ enum drm_bridge_ops {
>  	 * this flag shall implement the &drm_bridge_funcs->get_modes callback.
>  	 */
>  	DRM_BRIDGE_OP_MODES = BIT(3),
> +	/**
> +	 * @DRM_BRIDGE_OP_UPSTREAM_FIRST: The bridge can requires

s/can //

> +	 * that the upstream node pre_enable is called before its pre_enable,

s/node/bridge/ ?

> +	 * and conversely for post_disables. This is most frequently a

s/post_disables/post_disable/

Bonus points if you use the correct markup to link to those operations.

> +	 * requirement for DSI devices which need the host to be initialised
> +	 * before them.
> +	 */
> +	DRM_BRIDGE_OP_UPSTREAM_FIRST = BIT(4),
>  };
>  
>  /**

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hello,

On Fri, Feb 18, 2022 at 02:20:19PM +0100, Andrzej Hajda wrote:
> On 16.02.2022 17:59, Dave Stevenson wrote:
> > Hi All
> >
> > Hopefully I've cc'ed all those that have bashed this problem around previously,
> > or are otherwise linked to DRM bridges.
> >
> > There have been numerous discussions around how DSI support is currently broken
> > as it doesn't support initialising the PHY to LP-11 and potentially the clock
> > lane to HS prior to configuring the DSI peripheral. There is no op where the
> > interface is initialised but HS video isn't also being sent.
> > Currently you have:
> > - peripheral pre_enable (host not initialised yet)
> > - host pre_enable
> > - encoder enable
> > - host enable
> > - peripheral enable (video already running)
> >
> > vc4 and exynos currently implement the DSI host as an encoder, and split the
> > bridge_chain. This fails if you want to switch to being a bridge and/or use
> > atomic calls as the state of all the elements split off are not added by
> > drm_atomic_add_encoder_bridges.
> >
> > dw-mipi-dsi[1] and now msm[2] use the mode_set hook to initialise the PHY, so
> > the bridge/panel pre_enable can send commands. In their post_disable they then
> > call the downstream bridge/panel post_disable op manually so that shutdown
> > commands can be sent before shutting down the PHY. Nothing handles that fact,
> > so the framework then continues down the bridge chain and calls the post_disable
> > again, so we get unbalanced panel prepare/unprepare calls being reported [3].
> >
> > There have been patches[4] proposing reversing the entire direction of
> > pre_enable and post_disable, but that risks driving voltage into devices that
> > have yet to be powered up.
> > There have been discussions about adding either a pre_pre_enable, or adding a
> > DSI host_op to initialise the host[5]. Both require significant reworking to all
> > existing drivers in moving initialisation phases.
> > We have patches that look like they may well be addressing race conditions in
> > starting up a DSI peripheral[6].
> >
> > This patch takes a hybrid of the two: an optional reversing of the order for
> > specific links within the bridge chain within pre_enable and post_disable done
> > within the drm_bridge framework.
> > I'm more than happy to move where the flag exists in structures (currently as
> > DRM_BRIDGE_OP_UPSTREAM_FIRST in drm_bridge_ops, but it isn't an op),

API-wise that's my only concern, the flag should go somewhere else.

> > but does
> > this solve the problem posed? If not, then can you describe the actual scenario
> > it doesn't cover?
> > A DSI peripheral can set the flag to get the DSI host initialised first, and
> > therefore it has a stable LP-11 state before pre_enable. Likewise the peripheral
> > can still send shutdown commands prior to the DSI host being shut down in
> > post_disable. It also handles the case where there are multiple devices in the
> > chain that all want their upstream bridge enabled first, so should there be a
> > DSI mux between host and peripheral, then it can still get the host to the
> > correct state.
> >
> > An example tree is at [7] which is drm-misc-next with these patches and then a
> > conversion of vc4_dsi to use the atomic bridge functions (will be upstreamed
> > once we're over this hurdle). It is working happily with the Toshiba TC358762 on
> > a Raspberry Pi 7" panel.
> > The same approach but on our vendor 5.15 tree[8] has also been tested
> > successfully on a TI SN65DSI83 and LVDS panel.
> >
> > Whilst here, I've also documented the expected behaviour of DSI hosts and
> > peripherals to aid those who come along after.
> 
> Good summary, of multiple attempts of solving the issue (however I still 
> could add some more :) ).

Definitely good, thank you very much Dave for tackling this issue.

> I think the main issue is that we try to squeeze different hardware 
> protocol requirements into one quite restrictive framework - whole 
> crtc->encoder->bridges->(panel ||connector) is managed directly by drm core.
> No place to negotiate configuration directly between players 
> (bridges/panels).
> This patchset slightly looses the restrictions, so hopefully will help 
> for some time, but still every developer needs to solve riddles what to 
> put into callbacks, to allow driver working in different pipelines.

That's true, but documentation can help a lot there. Patch 2/2 turns the
riddle-solving task into documentation reading. Granted, not everybody
will read the documentation (and we should probably link to it from the
documentation of the pre_enable and post_disable operations), but the
behaviour is now defined, which is a major step forward.

> <DREAM MODE ON>
> Ideally I would like to drop idea of the bridge/panel and build 
> abstraction on data links.
> So for example DSI/EDP bridge during probe would register DSI sink with 
> their ops, and EDP source with their ops or just look for EDP sink (what 
> will suit better).
> To establish data link they could use their ops and helpers to provide 
> two-way conversation.
> This way if we need add support for new data link type or extend 
> existing one we do not need to touch whole framework and pray to not 
> break some strange bridge, or to add ops which will not be used by most 
> of users.
> <DREAM MODE OFF>

Protocol-specific operations can help, but I don't think they will
fundamentally change the problem. Yes, in some case, we can have
hardware requirements that are hard to express through generic
operations, but in most case the issue is more about defining the
semantic of the generic operations for a particular protocol than about
a need for a specific operation.

The core issue, in my opinion, is that we have a mechanism that
essentially works from source to sink, with the source controlling the
sink. With some protocols (DSI in particular), the start sequence
requires more fine-grained control of the operations, and the sink
should be in control. We should ideally start a pipeline by calling the
enable operation on the last element (connector or panel), whose driver
will then call operations on its source, and interleave those calls with
control of the local device, in the exact sequence required by the
device. That's how the omapdrm driver operated before I ported it to
drm_bridge. Reversing the order of the pipeline enable was a huge piece
of work for a single driver. Doing it again in the other direction for
*all* drivers seems like an even bigger dream (or nightmare) than yours
Andrzej :-)

> Putting dreams off, I think this patchset can add some value, at the 
> price of call chain complication. Lets see opinion of others.

I agree, I think it's a reasonable middleground. It improves the
situation, adds very little complexity in the API, has documentation to
specifies how the operations are meant to be implemented, and has a
reasonable increase of complexity for the pre_enable and post_disable
helpers (and the implementation could probably be simplified by moving
to recursive calls). I like this.

> > [1] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L940
> > [2] https://lists.freedesktop.org/archives/dri-devel/2022-January/337769.html
> > [3] https://lists.freedesktop.org/archives/dri-devel/2021-December/333908.html
> > [4] https://lists.freedesktop.org/archives/dri-devel/2021-October/328476.html
> > [5] https://lists.freedesktop.org/archives/dri-devel/2021-October/325853.html
> > [6] https://lists.freedesktop.org/archives/dri-devel/2022-February/341852.html
> > [7] https://github.com/6by9/linux/tree/drm-misc-next-vc4_dsi
> > [8] https://github.com/6by9/linux/tree/rpi-5.15.y-sn65dsi83
> >
> >
> > Dave Stevenson (2):
> >    drm: Introduce DRM_BRIDGE_OP_UPSTREAM_FIRST to alter bridge init order
> >    drm/bridge: Document the expected behaviour of DSI host controllers
> >
> >   Documentation/gpu/drm-kms-helpers.rst |   7 +
> >   drivers/gpu/drm/drm_bridge.c          | 235 ++++++++++++++++++++++++++++++----
> >   include/drm/drm_bridge.h              |   8 ++
> >   3 files changed, 225 insertions(+), 25 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] drm/bridge: Document the expected behaviour of DSI host controllers

[Dave Stevenson]

Hi Laurent

On Tue, 22 Feb 2022 at 06:22, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Dave,
>
> Thank you for the patch.
>
>
> On Wed, Feb 16, 2022 at 04:59:44PM +0000, Dave Stevenson wrote:
> > The exact behaviour of DSI host controllers is not specified,
> > therefore define it.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  Documentation/gpu/drm-kms-helpers.rst |  7 +++++++
> >  drivers/gpu/drm/drm_bridge.c          | 38 +++++++++++++++++++++++++++++++++++
> >  2 files changed, 45 insertions(+)
> >
> > diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
> > index c3ce91eecbc1..362afdb867c6 100644
> > --- a/Documentation/gpu/drm-kms-helpers.rst
> > +++ b/Documentation/gpu/drm-kms-helpers.rst
> > @@ -185,6 +185,13 @@ Bridge Helper Reference
> >  .. kernel-doc:: drivers/gpu/drm/drm_bridge.c
> >     :export:
> >
> > +MIPI-DSI bridge operation
> > +-------------------------
> > +
> > +.. kernel-doc:: drivers/gpu/drm/drm_bridge.c
> > +   :doc: dsi bridge operations
> > +
> > +
> >  Bridge Connector Helper Reference
> >  ---------------------------------
> >
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index 7c24e8340efa..14c2ee9e0328 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -152,6 +152,44 @@
> >   * situation when probing.
> >   */
> >
> > +/**
> > + * DOC: dsi bridge operations
> > + *
> > + * DSI host interfaces are expected to be implemented as bridges rather than
> > + * encoders, however there are a few aspects of their operation that need to
> > + * be defined in order to provide a consistent interface.
> > + *
> > + * A DSI host should keep the PHY powered down until the pre_enable op is
>
> I'd write "operation" in full everywhere to avoid mixing the two.

Ack on this and the other minor changes.

> > + * called. All lanes should be in an idle state (not LP-11) up to this point.
>
> Is the idle state LP-00 ? If so I'd state that explicitly.

I'd avoided specifying LP-00 as that is a specific state in the
transition from LP to HS (LP-11 -> LP-01 -> LP-00 -> HS).
LP-00 also implies that the line is being actively driven, whereas
this is "powered down" so potentially just has passive pull resistors
on the line.
Looking at the D-PHY spec (I happen to have 1.1 to hand), "Figure 25
Clock Lane Module State Diagram" and we're looking at the "Init
Master" state.

This would also be a point where ULPS might be implemented.

Perhaps replace with "All lanes are in an undefined idle state up to
this point" to allow for differences in hardware implementation?

  Dave

> "[...] in an idle state (LP-00, not LP-11) [...]"
>
> > + * pre_enable should initialise the PHY, set the data lanes to LP-11, and the
> > + * clock lane to either LP-11 or HS dependent on the mode_flag
>
> s/dependent/depending/ ?
>
> > + * MIPI_DSI_CLOCK_NON_CONTINUOUS.
> > + *
> > + * Ordinarily the downstream bridge DSI peripheral pre_enable will have been
> > + * called before the DSI host. If the DSI peripheral requires LP-11 and/or
> > + * the clock lane to be in HS mode prior to pre_enable, then it can set the
> > + * DRM_BRIDGE_OP_UPSTREAM_FIRST flag to request the pre_enable (and
> > + * post_disable) order to be altered to enable the DSI host first.
> > + *
> > + * Either the CRTC being enabled, or the DSI host enable op should switch the
> > + * host to actively transmitting video on the data lanes.
> > + *
> > + * The reverse also applies. The DSI host disable op or stopping the CRTC should
> > + * stop transmitting video, and the data lanes should return to the LP-11 state.
> > + * The DSI host post_disable op should disable the PHY.
> > + * If the DRM_BRIDGE_OP_UPSTREAM_FIRST flag is set, then the DSI peripheral's
> > + * bridge post_disable will be called before the DSI host's post_disable.
> > + *
> > + * Whilst it is valid to call host_transfer prior to pre_enable or after
> > + * post_disable, the exact state of the lanes is undefined at this point. The
> > + * DSI host should initialise the interface, transmit the data, and then disable
> > + * the interface again.
> > + *
> > + * Ultra Low Power State (ULPS) is not explicitly supported by DRM. If
> > + * implemented, it therefore needs to be either handled entirely within the DSI
>
> s/either // (or you need an "or ..." :-))
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > + * Host driver.
> > + */
> > +
> >  static DEFINE_MUTEX(bridge_lock);
> >  static LIST_HEAD(bridge_list);
> >
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH 1/2] drm: Introduce DRM_BRIDGE_OP_UPSTREAM_FIRST to alter bridge init order

[Dave Stevenson]

Hi Laurent.

Thanks for the review.

On Tue, 22 Feb 2022 at 06:34, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Dave,
>
> Thank you for the patch.
>
> On Wed, Feb 16, 2022 at 04:59:43PM +0000, Dave Stevenson wrote:
> > DSI sink devices typically want the DSI host powered up and configured
> > before they are powered up. pre_enable is the place this would normally
> > happen, but they are called in reverse order from panel/connector towards
> > the encoder, which is the "wrong" order.
> >
> > Add a new flag DRM_BRIDGE_OP_UPSTREAM_FIRST that any bridge can set
> > to swap the order of pre_enable (and post_disable) so that any upstream
> > bridges are called first to create the desired state.
> >
> > eg:
> > - Panel
> > - Bridge 1
> > - Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST
> > - Bridge 3
> > - Encoder
> > Would result in pre_enable's being called as Panel, Bridge 1, Bridge 3,
> > Bridge 2.
>
> If there was a Bridge 4 between Bridge 3 and Encoder, would it be
>
> Panel, Bridge 1, Bridge 3, Bridge 4, Bridge 2
>
> ? I'd capture that here, to be explicit.

No.
 - Panel
 - Bridge 1
 - Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST
 - Bridge 3
 - Bridge 4
  - Encoder
Would result in pre_enable's being called as Panel, Bridge 1, Bridge
3, Bridge 2, Bridge 4, Encoder.
ie it only swaps the order of bridges 2 & 3.

 - Panel
 - Bridge 1
 - Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST
 - Bridge 3 DRM_BRIDGE_OP_UPSTREAM_FIRST
 - Bridge 4
 - Encoder
Would result in pre_enable's being called as Panel, Bridge 1, Bridge
4, Bridge 3, Bridge 2, Encoder.
(Bridge 2&3 have asked for upstream to be enabled first, which means
bridge 4. Bridge 2 wants upstream enabled first, which means bridge
3).

 - Panel
 - Bridge 1
 - Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST
 - Bridge 3
 - Bridge 4 DRM_BRIDGE_OP_UPSTREAM_FIRST
 - Bridge 5
 - Encoder
Would result in Panel, Bridge 1, Bridge 3, Bridge 2, Bridge 5, Bridge
4, Encoder.

So we only reverse the order whilst the bridges request that they want
upstream enabled first, but we can do that multiple times within the
chain. I hope that makes sense.

> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/gpu/drm/drm_bridge.c | 197 +++++++++++++++++++++++++++++++++++++------
> >  include/drm/drm_bridge.h     |   8 ++
> >  2 files changed, 180 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index c96847fc0ebc..7c24e8340efa 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -522,21 +522,58 @@ EXPORT_SYMBOL(drm_bridge_chain_disable);
> >   * Calls &drm_bridge_funcs.post_disable op for all the bridges in the
> >   * encoder chain, starting from the first bridge to the last. These are called
> >   * after completing the encoder's prepare op.
>
> Missing blank line, as well as in three locations below.
>
> > + * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the post_disable for
> > + * that bridge will be called before the previous one to reverse the pre_enable
> > + * calling direction.
> >   *
> >   * Note: the bridge passed should be the one closest to the encoder
> >   */
> >  void drm_bridge_chain_post_disable(struct drm_bridge *bridge)
> >  {
> >       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) {
> > +             limit = NULL;
> > +
> > +             if (!list_is_last(&bridge->chain_node, &encoder->bridge_chain)) {
> > +                     next = list_next_entry(bridge, chain_node);
> > +
> > +                     if (next->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
> > +                             limit = next;
> > +
> > +                             list_for_each_entry_from(next, &encoder->bridge_chain,
> > +                                                      chain_node) {
> > +                                     if (!(next->ops &
> > +                                             DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
> > +                                             next = list_prev_entry(next, chain_node);
> > +                                             limit = next;
> > +                                             break;
> > +                                     }
> > +                             }
> > +
> > +                             list_for_each_entry_from_reverse(next, &encoder->bridge_chain,
> > +                                                              chain_node) {
> > +                                     if (next == bridge)
> > +                                             break;
> > +
> > +                                     if (next->funcs->post_disable)
> > +                                             next->funcs->post_disable(next);
> > +                             }
> > +                     }
> > +             }
> > +
> >               if (bridge->funcs->post_disable)
> >                       bridge->funcs->post_disable(bridge);
> > +
> > +             if (limit)
> > +                     bridge = limit;
> >       }
> > +
> >  }
> >  EXPORT_SYMBOL(drm_bridge_chain_post_disable);
> >
> > @@ -577,22 +614,53 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set);
> >   * Calls &drm_bridge_funcs.pre_enable op for all the bridges in the encoder
> >   * chain, starting from the last bridge to the first. These are called
> >   * before calling the encoder's commit op.
> > + * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the pre_enable for
> > + * the previous bridge will be called before pre_enable of this bridge.
> >   *
> >   * Note: the bridge passed should be the one closest to the encoder
> >   */
> >  void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
> >  {
> >       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->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
> > +                     next = iter;
> > +                     limit = bridge;
> > +                     list_for_each_entry_from_reverse(next,
> > +                                                      &encoder->bridge_chain,
> > +                                                      chain_node) {
> > +                             if (next == bridge)
> > +                                     break;
> > +
> > +                             if (!(next->ops &
> > +                                     DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
> > +                                     limit = list_prev_entry(next, chain_node);
> > +                                     break;
> > +                             }
> > +                     }
> > +
> > +                     list_for_each_entry_from(next, &encoder->bridge_chain, chain_node) {
> > +                             if (next == iter)
> > +                                     break;
> > +
> > +                             if (next->funcs->pre_enable)
> > +                                     next->funcs->pre_enable(next);
> > +                     }
> > +             }
> > +
> >               if (iter->funcs->pre_enable)
> >                       iter->funcs->pre_enable(iter);
> >
> > +             if (iter->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST)
> > +                     iter = limit;
> > +
> >               if (iter == bridge)
> >                       break;
> >       }
> > @@ -667,6 +735,25 @@ void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge,
> >  }
> >  EXPORT_SYMBOL(drm_atomic_bridge_chain_disable);
> >
> > +static void drm_atomic_bridge_call_post_disable(struct drm_bridge *bridge,
> > +                                             struct drm_atomic_state *old_state)
> > +{
> > +     if (bridge->funcs->atomic_post_disable) {
> > +             struct drm_bridge_state *old_bridge_state;
> > +
> > +             old_bridge_state =
> > +                     drm_atomic_get_old_bridge_state(old_state,
> > +                                                     bridge);
> > +             if (WARN_ON(!old_bridge_state))
> > +                     return;
> > +
> > +             bridge->funcs->atomic_post_disable(bridge,
> > +                                                old_bridge_state);
> > +     } else if (bridge->funcs->post_disable) {
> > +             bridge->funcs->post_disable(bridge);
> > +     }
> > +}
> > +
> >  /**
> >   * drm_atomic_bridge_chain_post_disable - cleans up after disabling all bridges
> >   *                                     in the encoder chain
> > @@ -677,6 +764,9 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_disable);
> >   * &drm_bridge_funcs.post_disable) op for all the bridges in the encoder chain,
> >   * starting from the first bridge to the last. These are called after completing
> >   * &drm_encoder_helper_funcs.atomic_disable
> > + * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the post_disable for
> > + * that bridge will be called before the previous one to reverse the pre_enable
> > + * calling direction.
> >   *
> >   * Note: the bridge passed should be the one closest to the encoder
> >   */
> > @@ -684,30 +774,69 @@ void drm_atomic_bridge_chain_post_disable(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_post_disable) {
> > -                     struct drm_bridge_state *old_bridge_state;
> > +             limit = NULL;
> > +
> > +             if (!list_is_last(&bridge->chain_node, &encoder->bridge_chain)) {
> > +                     next = list_next_entry(bridge, chain_node);
> > +
> > +                     if (next->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
> > +                             limit = next;
> > +
> > +                             list_for_each_entry_from(next, &encoder->bridge_chain,
> > +                                                      chain_node) {
> > +                                     if (!(next->ops &
> > +                                             DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
> > +                                             next = list_prev_entry(next, chain_node);
> > +                                             limit = next;
> > +                                             break;
> > +                                     }
> > +                             }
> > +
> > +                             list_for_each_entry_from_reverse(next, &encoder->bridge_chain,
> > +                                                              chain_node) {
> > +                                     if (next == bridge)
> > +                                             break;
> > +
> > +                                     drm_atomic_bridge_call_post_disable(next,
> > +                                                                         old_state);
> > +                             }
> > +                     }
> > +             }
> >
> > -                     old_bridge_state =
> > -                             drm_atomic_get_old_bridge_state(old_state,
> > -                                                             bridge);
> > -                     if (WARN_ON(!old_bridge_state))
> > -                             return;
> > +             drm_atomic_bridge_call_post_disable(bridge, old_state);
> >
> > -                     bridge->funcs->atomic_post_disable(bridge,
> > -                                                        old_bridge_state);
> > -             } else if (bridge->funcs->post_disable) {
> > -                     bridge->funcs->post_disable(bridge);
> > -             }
> > +             if (limit)
> > +                     bridge = limit;
> >       }
> >  }
> >  EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
> >
> > +static void drm_atomic_bridge_call_pre_enable(struct drm_bridge *bridge,
> > +                                           struct drm_atomic_state *old_state)
> > +{
> > +     if (bridge->funcs->atomic_pre_enable) {
> > +             struct drm_bridge_state *old_bridge_state;
> > +
> > +             old_bridge_state =
> > +                     drm_atomic_get_old_bridge_state(old_state,
> > +                                                     bridge);
> > +             if (WARN_ON(!old_bridge_state))
> > +                     return;
> > +
> > +             bridge->funcs->atomic_pre_enable(bridge, old_bridge_state);
> > +     } else if (bridge->funcs->pre_enable) {
> > +             bridge->funcs->pre_enable(bridge);
> > +     }
> > +}
> > +
> >  /**
> >   * drm_atomic_bridge_chain_pre_enable - prepares for enabling all bridges in
> >   *                                   the encoder chain
> > @@ -718,6 +847,8 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
> >   * &drm_bridge_funcs.pre_enable) op for all the bridges in the encoder chain,
> >   * starting from the last bridge to the first. These are called before calling
> >   * &drm_encoder_helper_funcs.atomic_enable
> > + * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the pre_enable for
> > + * the previous bridge will be called before pre_enable of this bridge.
> >   *
> >   * Note: the bridge passed should be the one closest to the encoder
> >   */
> > @@ -725,26 +856,42 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
> >                                       struct drm_atomic_state *old_state)
> >  {
> >       struct drm_encoder *encoder;
> > -     struct drm_bridge *iter;
> > +     struct drm_bridge *iter, *next, *limit;
> >
> >       if (!bridge)
> >               return;
> >
> >       encoder = bridge->encoder;
> > +
> >       list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> > -             if (iter->funcs->atomic_pre_enable) {
> > -                     struct drm_bridge_state *old_bridge_state;
> > +             if (iter->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
> > +                     next = iter;
> > +                     limit = bridge;
> > +                     list_for_each_entry_from_reverse(next,
> > +                                                      &encoder->bridge_chain,
> > +                                                      chain_node) {
> > +                             if (next == bridge)
> > +                                     break;
> > +
> > +                             if (!(next->ops &
> > +                                     DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
> > +                                     limit = list_prev_entry(next, chain_node);
> > +                                     break;
> > +                             }
> > +                     }
> > +
> > +                     list_for_each_entry_from(next, &encoder->bridge_chain, chain_node) {
> > +                             if (next == iter)
> > +                                     break;
> > +
> > +                             drm_atomic_bridge_call_pre_enable(next, old_state);
> > +                     }
> > +             }
>
> This is hard to understand, I have trouble figuring out if it does the
> right thing when multiple bridges set the DRM_BRIDGE_OP_UPSTREAM_FIRST
> flag (or actually even when a single bridge does so). Comments would
> help, but I wonder if it wouldn't be simpler to switch to a recursive
> implementation.

Recursive - such joys!
With the explanation above, I'm not sure that recursive helps, but
certainly can add comments.

> It also seems that merging the legacy and atomic versions of the code
> would be a good idea. They could both call into a shared implementation,
> with the legacy version passing a NULL state, and the atomic op being
> considered only if the state is not NULL.

OK, I'll look into that.

> >
> > -                     old_bridge_state =
> > -                             drm_atomic_get_old_bridge_state(old_state,
> > -                                                             iter);
> > -                     if (WARN_ON(!old_bridge_state))
> > -                             return;
> > +             drm_atomic_bridge_call_pre_enable(iter, old_state);
> >
> > -                     iter->funcs->atomic_pre_enable(iter, old_bridge_state);
> > -             } else if (iter->funcs->pre_enable) {
> > -                     iter->funcs->pre_enable(iter);
> > -             }
> > +             if (iter->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST)
> > +                     iter = limit;
> >
> >               if (iter == bridge)
> >                       break;
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index f27b4060faa2..523ec9d8f3f8 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -725,6 +725,14 @@ enum drm_bridge_ops {
> >        * this flag shall implement the &drm_bridge_funcs->get_modes callback.
> >        */
> >       DRM_BRIDGE_OP_MODES = BIT(3),
> > +     /**
> > +      * @DRM_BRIDGE_OP_UPSTREAM_FIRST: The bridge can requires
>
> s/can //
>
> > +      * that the upstream node pre_enable is called before its pre_enable,
>
> s/node/bridge/ ?
>
> > +      * and conversely for post_disables. This is most frequently a
>
> s/post_disables/post_disable/

Ack on this and the other small changes.

> Bonus points if you use the correct markup to link to those operations.

I looked at the markup, but found it so haphazardly used that it sort
of seemed not worth it. Damned if you do, and damned if you don't.

  Dave

> > +      * requirement for DSI devices which need the host to be initialised
> > +      * before them.
> > +      */
> > +     DRM_BRIDGE_OP_UPSTREAM_FIRST = BIT(4),
> >  };
> >
> >  /**
>
> --
> Regards,
>
> Laurent Pinchart

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

[Dave Stevenson]

Hi Laurent.

On Tue, 22 Feb 2022 at 06:43, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hello,
>
> On Fri, Feb 18, 2022 at 02:20:19PM +0100, Andrzej Hajda wrote:
> > On 16.02.2022 17:59, Dave Stevenson wrote:
> > > Hi All
> > >
> > > Hopefully I've cc'ed all those that have bashed this problem around previously,
> > > or are otherwise linked to DRM bridges.
> > >
> > > There have been numerous discussions around how DSI support is currently broken
> > > as it doesn't support initialising the PHY to LP-11 and potentially the clock
> > > lane to HS prior to configuring the DSI peripheral. There is no op where the
> > > interface is initialised but HS video isn't also being sent.
> > > Currently you have:
> > > - peripheral pre_enable (host not initialised yet)
> > > - host pre_enable
> > > - encoder enable
> > > - host enable
> > > - peripheral enable (video already running)
> > >
> > > vc4 and exynos currently implement the DSI host as an encoder, and split the
> > > bridge_chain. This fails if you want to switch to being a bridge and/or use
> > > atomic calls as the state of all the elements split off are not added by
> > > drm_atomic_add_encoder_bridges.
> > >
> > > dw-mipi-dsi[1] and now msm[2] use the mode_set hook to initialise the PHY, so
> > > the bridge/panel pre_enable can send commands. In their post_disable they then
> > > call the downstream bridge/panel post_disable op manually so that shutdown
> > > commands can be sent before shutting down the PHY. Nothing handles that fact,
> > > so the framework then continues down the bridge chain and calls the post_disable
> > > again, so we get unbalanced panel prepare/unprepare calls being reported [3].
> > >
> > > There have been patches[4] proposing reversing the entire direction of
> > > pre_enable and post_disable, but that risks driving voltage into devices that
> > > have yet to be powered up.
> > > There have been discussions about adding either a pre_pre_enable, or adding a
> > > DSI host_op to initialise the host[5]. Both require significant reworking to all
> > > existing drivers in moving initialisation phases.
> > > We have patches that look like they may well be addressing race conditions in
> > > starting up a DSI peripheral[6].
> > >
> > > This patch takes a hybrid of the two: an optional reversing of the order for
> > > specific links within the bridge chain within pre_enable and post_disable done
> > > within the drm_bridge framework.
> > > I'm more than happy to move where the flag exists in structures (currently as
> > > DRM_BRIDGE_OP_UPSTREAM_FIRST in drm_bridge_ops, but it isn't an op),
>
> API-wise that's my only concern, the flag should go somewhere else.

Ah, the million dollar question then - where does it go? It is only
true or false, so a bool in struct drm_bridge, same as
interlace_allowed?

I've had the realisation that this needs to be accessible from the
panel drivers so DSI panel drivers such as panel-ilitek-ili9881 can
set it too. I'll have a slight rethink over that one, but it is
probably a similar extra flag in struct drm_panel.

  Dave

> > > but does
> > > this solve the problem posed? If not, then can you describe the actual scenario
> > > it doesn't cover?
> > > A DSI peripheral can set the flag to get the DSI host initialised first, and
> > > therefore it has a stable LP-11 state before pre_enable. Likewise the peripheral
> > > can still send shutdown commands prior to the DSI host being shut down in
> > > post_disable. It also handles the case where there are multiple devices in the
> > > chain that all want their upstream bridge enabled first, so should there be a
> > > DSI mux between host and peripheral, then it can still get the host to the
> > > correct state.
> > >
> > > An example tree is at [7] which is drm-misc-next with these patches and then a
> > > conversion of vc4_dsi to use the atomic bridge functions (will be upstreamed
> > > once we're over this hurdle). It is working happily with the Toshiba TC358762 on
> > > a Raspberry Pi 7" panel.
> > > The same approach but on our vendor 5.15 tree[8] has also been tested
> > > successfully on a TI SN65DSI83 and LVDS panel.
> > >
> > > Whilst here, I've also documented the expected behaviour of DSI hosts and
> > > peripherals to aid those who come along after.
> >
> > Good summary, of multiple attempts of solving the issue (however I still
> > could add some more :) ).
>
> Definitely good, thank you very much Dave for tackling this issue.
>
> > I think the main issue is that we try to squeeze different hardware
> > protocol requirements into one quite restrictive framework - whole
> > crtc->encoder->bridges->(panel ||connector) is managed directly by drm core.
> > No place to negotiate configuration directly between players
> > (bridges/panels).
> > This patchset slightly looses the restrictions, so hopefully will help
> > for some time, but still every developer needs to solve riddles what to
> > put into callbacks, to allow driver working in different pipelines.
>
> That's true, but documentation can help a lot there. Patch 2/2 turns the
> riddle-solving task into documentation reading. Granted, not everybody
> will read the documentation (and we should probably link to it from the
> documentation of the pre_enable and post_disable operations), but the
> behaviour is now defined, which is a major step forward.
>
> > <DREAM MODE ON>
> > Ideally I would like to drop idea of the bridge/panel and build
> > abstraction on data links.
> > So for example DSI/EDP bridge during probe would register DSI sink with
> > their ops, and EDP source with their ops or just look for EDP sink (what
> > will suit better).
> > To establish data link they could use their ops and helpers to provide
> > two-way conversation.
> > This way if we need add support for new data link type or extend
> > existing one we do not need to touch whole framework and pray to not
> > break some strange bridge, or to add ops which will not be used by most
> > of users.
> > <DREAM MODE OFF>
>
> Protocol-specific operations can help, but I don't think they will
> fundamentally change the problem. Yes, in some case, we can have
> hardware requirements that are hard to express through generic
> operations, but in most case the issue is more about defining the
> semantic of the generic operations for a particular protocol than about
> a need for a specific operation.
>
> The core issue, in my opinion, is that we have a mechanism that
> essentially works from source to sink, with the source controlling the
> sink. With some protocols (DSI in particular), the start sequence
> requires more fine-grained control of the operations, and the sink
> should be in control. We should ideally start a pipeline by calling the
> enable operation on the last element (connector or panel), whose driver
> will then call operations on its source, and interleave those calls with
> control of the local device, in the exact sequence required by the
> device. That's how the omapdrm driver operated before I ported it to
> drm_bridge. Reversing the order of the pipeline enable was a huge piece
> of work for a single driver. Doing it again in the other direction for
> *all* drivers seems like an even bigger dream (or nightmare) than yours
> Andrzej :-)
>
> > Putting dreams off, I think this patchset can add some value, at the
> > price of call chain complication. Lets see opinion of others.
>
> I agree, I think it's a reasonable middleground. It improves the
> situation, adds very little complexity in the API, has documentation to
> specifies how the operations are meant to be implemented, and has a
> reasonable increase of complexity for the pre_enable and post_disable
> helpers (and the implementation could probably be simplified by moving
> to recursive calls). I like this.
>
> > > [1] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L940
> > > [2] https://lists.freedesktop.org/archives/dri-devel/2022-January/337769.html
> > > [3] https://lists.freedesktop.org/archives/dri-devel/2021-December/333908.html
> > > [4] https://lists.freedesktop.org/archives/dri-devel/2021-October/328476.html
> > > [5] https://lists.freedesktop.org/archives/dri-devel/2021-October/325853.html
> > > [6] https://lists.freedesktop.org/archives/dri-devel/2022-February/341852.html
> > > [7] https://github.com/6by9/linux/tree/drm-misc-next-vc4_dsi
> > > [8] https://github.com/6by9/linux/tree/rpi-5.15.y-sn65dsi83
> > >
> > >
> > > Dave Stevenson (2):
> > >    drm: Introduce DRM_BRIDGE_OP_UPSTREAM_FIRST to alter bridge init order
> > >    drm/bridge: Document the expected behaviour of DSI host controllers
> > >
> > >   Documentation/gpu/drm-kms-helpers.rst |   7 +
> > >   drivers/gpu/drm/drm_bridge.c          | 235 ++++++++++++++++++++++++++++++----
> > >   include/drm/drm_bridge.h              |   8 ++
> > >   3 files changed, 225 insertions(+), 25 deletions(-)
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH 1/2] drm: Introduce DRM_BRIDGE_OP_UPSTREAM_FIRST to alter bridge init order

[Andrzej Hajda]

On 22.02.2022 09:43, Dave Stevenson wrote:
> Hi Laurent.
>
> Thanks for the review.
>
> On Tue, 22 Feb 2022 at 06:34, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>> Hi Dave,
>>
>> Thank you for the patch.
>>
>> On Wed, Feb 16, 2022 at 04:59:43PM +0000, Dave Stevenson wrote:
>>> DSI sink devices typically want the DSI host powered up and configured
>>> before they are powered up. pre_enable is the place this would normally
>>> happen, but they are called in reverse order from panel/connector towards
>>> the encoder, which is the "wrong" order.
>>>
>>> Add a new flag DRM_BRIDGE_OP_UPSTREAM_FIRST that any bridge can set
>>> to swap the order of pre_enable (and post_disable) so that any upstream
>>> bridges are called first to create the desired state.
>>>
>>> eg:
>>> - Panel
>>> - Bridge 1
>>> - Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST
>>> - Bridge 3
>>> - Encoder
>>> Would result in pre_enable's being called as Panel, Bridge 1, Bridge 3,
>>> Bridge 2.
>> If there was a Bridge 4 between Bridge 3 and Encoder, would it be
>>
>> Panel, Bridge 1, Bridge 3, Bridge 4, Bridge 2
>>
>> ? I'd capture that here, to be explicit.
> No.
>   - Panel
>   - Bridge 1
>   - Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST
>   - Bridge 3
>   - Bridge 4
>    - Encoder
> Would result in pre_enable's being called as Panel, Bridge 1, Bridge
> 3, Bridge 2, Bridge 4, Encoder.
> ie it only swaps the order of bridges 2 & 3.
>
>   - Panel
>   - Bridge 1
>   - Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST
>   - Bridge 3 DRM_BRIDGE_OP_UPSTREAM_FIRST
>   - Bridge 4
>   - Encoder
> Would result in pre_enable's being called as Panel, Bridge 1, Bridge
> 4, Bridge 3, Bridge 2, Encoder.
> (Bridge 2&3 have asked for upstream to be enabled first, which means
> bridge 4. Bridge 2 wants upstream enabled first, which means bridge
> 3).
>
>   - Panel
>   - Bridge 1
>   - Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST
>   - Bridge 3
>   - Bridge 4 DRM_BRIDGE_OP_UPSTREAM_FIRST
>   - Bridge 5
>   - Encoder
> Would result in Panel, Bridge 1, Bridge 3, Bridge 2, Bridge 5, Bridge
> 4, Encoder.
>
> So we only reverse the order whilst the bridges request that they want
> upstream enabled first, but we can do that multiple times within the
> chain. I hope that makes sense.
>
>>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>> ---
>>>   drivers/gpu/drm/drm_bridge.c | 197 +++++++++++++++++++++++++++++++++++++------
>>>   include/drm/drm_bridge.h     |   8 ++
>>>   2 files changed, 180 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>>> index c96847fc0ebc..7c24e8340efa 100644
>>> --- a/drivers/gpu/drm/drm_bridge.c
>>> +++ b/drivers/gpu/drm/drm_bridge.c
>>> @@ -522,21 +522,58 @@ EXPORT_SYMBOL(drm_bridge_chain_disable);
>>>    * Calls &drm_bridge_funcs.post_disable op for all the bridges in the
>>>    * encoder chain, starting from the first bridge to the last. These are called
>>>    * after completing the encoder's prepare op.
>> Missing blank line, as well as in three locations below.
>>
>>> + * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the post_disable for
>>> + * that bridge will be called before the previous one to reverse the pre_enable
>>> + * calling direction.
>>>    *
>>>    * Note: the bridge passed should be the one closest to the encoder
>>>    */
>>>   void drm_bridge_chain_post_disable(struct drm_bridge *bridge)
>>>   {
>>>        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) {
>>> +             limit = NULL;
>>> +
>>> +             if (!list_is_last(&bridge->chain_node, &encoder->bridge_chain)) {
>>> +                     next = list_next_entry(bridge, chain_node);
>>> +
>>> +                     if (next->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
>>> +                             limit = next;
>>> +
>>> +                             list_for_each_entry_from(next, &encoder->bridge_chain,
>>> +                                                      chain_node) {
>>> +                                     if (!(next->ops &
>>> +                                             DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
>>> +                                             next = list_prev_entry(next, chain_node);
>>> +                                             limit = next;
>>> +                                             break;
>>> +                                     }
>>> +                             }
>>> +
>>> +                             list_for_each_entry_from_reverse(next, &encoder->bridge_chain,
>>> +                                                              chain_node) {
>>> +                                     if (next == bridge)
>>> +                                             break;
>>> +
>>> +                                     if (next->funcs->post_disable)
>>> +                                             next->funcs->post_disable(next);
>>> +                             }
>>> +                     }
>>> +             }
>>> +
>>>                if (bridge->funcs->post_disable)
>>>                        bridge->funcs->post_disable(bridge);
>>> +
>>> +             if (limit)
>>> +                     bridge = limit;
>>>        }
>>> +
>>>   }
>>>   EXPORT_SYMBOL(drm_bridge_chain_post_disable);
>>>
>>> @@ -577,22 +614,53 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set);
>>>    * Calls &drm_bridge_funcs.pre_enable op for all the bridges in the encoder
>>>    * chain, starting from the last bridge to the first. These are called
>>>    * before calling the encoder's commit op.
>>> + * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the pre_enable for
>>> + * the previous bridge will be called before pre_enable of this bridge.
>>>    *
>>>    * Note: the bridge passed should be the one closest to the encoder
>>>    */
>>>   void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
>>>   {
>>>        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->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
>>> +                     next = iter;
>>> +                     limit = bridge;
>>> +                     list_for_each_entry_from_reverse(next,
>>> +                                                      &encoder->bridge_chain,
>>> +                                                      chain_node) {
>>> +                             if (next == bridge)
>>> +                                     break;
>>> +
>>> +                             if (!(next->ops &
>>> +                                     DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
>>> +                                     limit = list_prev_entry(next, chain_node);
>>> +                                     break;
>>> +                             }
>>> +                     }
>>> +
>>> +                     list_for_each_entry_from(next, &encoder->bridge_chain, chain_node) {
>>> +                             if (next == iter)
>>> +                                     break;
>>> +
>>> +                             if (next->funcs->pre_enable)
>>> +                                     next->funcs->pre_enable(next);
>>> +                     }
>>> +             }
>>> +
>>>                if (iter->funcs->pre_enable)
>>>                        iter->funcs->pre_enable(iter);
>>>
>>> +             if (iter->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST)
>>> +                     iter = limit;
>>> +
>>>                if (iter == bridge)
>>>                        break;
>>>        }
>>> @@ -667,6 +735,25 @@ void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge,
>>>   }
>>>   EXPORT_SYMBOL(drm_atomic_bridge_chain_disable);
>>>
>>> +static void drm_atomic_bridge_call_post_disable(struct drm_bridge *bridge,
>>> +                                             struct drm_atomic_state *old_state)
>>> +{
>>> +     if (bridge->funcs->atomic_post_disable) {
>>> +             struct drm_bridge_state *old_bridge_state;
>>> +
>>> +             old_bridge_state =
>>> +                     drm_atomic_get_old_bridge_state(old_state,
>>> +                                                     bridge);
>>> +             if (WARN_ON(!old_bridge_state))
>>> +                     return;
>>> +
>>> +             bridge->funcs->atomic_post_disable(bridge,
>>> +                                                old_bridge_state);
>>> +     } else if (bridge->funcs->post_disable) {
>>> +             bridge->funcs->post_disable(bridge);
>>> +     }
>>> +}
>>> +
>>>   /**
>>>    * drm_atomic_bridge_chain_post_disable - cleans up after disabling all bridges
>>>    *                                     in the encoder chain
>>> @@ -677,6 +764,9 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_disable);
>>>    * &drm_bridge_funcs.post_disable) op for all the bridges in the encoder chain,
>>>    * starting from the first bridge to the last. These are called after completing
>>>    * &drm_encoder_helper_funcs.atomic_disable
>>> + * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the post_disable for
>>> + * that bridge will be called before the previous one to reverse the pre_enable
>>> + * calling direction.
>>>    *
>>>    * Note: the bridge passed should be the one closest to the encoder
>>>    */
>>> @@ -684,30 +774,69 @@ void drm_atomic_bridge_chain_post_disable(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_post_disable) {
>>> -                     struct drm_bridge_state *old_bridge_state;
>>> +             limit = NULL;
>>> +
>>> +             if (!list_is_last(&bridge->chain_node, &encoder->bridge_chain)) {
>>> +                     next = list_next_entry(bridge, chain_node);
>>> +
>>> +                     if (next->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
>>> +                             limit = next;
>>> +
>>> +                             list_for_each_entry_from(next, &encoder->bridge_chain,
>>> +                                                      chain_node) {
>>> +                                     if (!(next->ops &
>>> +                                             DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
>>> +                                             next = list_prev_entry(next, chain_node);
>>> +                                             limit = next;
>>> +                                             break;
>>> +                                     }
>>> +                             }
>>> +
>>> +                             list_for_each_entry_from_reverse(next, &encoder->bridge_chain,
>>> +                                                              chain_node) {
>>> +                                     if (next == bridge)
>>> +                                             break;
>>> +
>>> +                                     drm_atomic_bridge_call_post_disable(next,
>>> +                                                                         old_state);
>>> +                             }
>>> +                     }
>>> +             }
>>>
>>> -                     old_bridge_state =
>>> -                             drm_atomic_get_old_bridge_state(old_state,
>>> -                                                             bridge);
>>> -                     if (WARN_ON(!old_bridge_state))
>>> -                             return;
>>> +             drm_atomic_bridge_call_post_disable(bridge, old_state);
>>>
>>> -                     bridge->funcs->atomic_post_disable(bridge,
>>> -                                                        old_bridge_state);
>>> -             } else if (bridge->funcs->post_disable) {
>>> -                     bridge->funcs->post_disable(bridge);
>>> -             }
>>> +             if (limit)
>>> +                     bridge = limit;
>>>        }
>>>   }
>>>   EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
>>>
>>> +static void drm_atomic_bridge_call_pre_enable(struct drm_bridge *bridge,
>>> +                                           struct drm_atomic_state *old_state)
>>> +{
>>> +     if (bridge->funcs->atomic_pre_enable) {
>>> +             struct drm_bridge_state *old_bridge_state;
>>> +
>>> +             old_bridge_state =
>>> +                     drm_atomic_get_old_bridge_state(old_state,
>>> +                                                     bridge);
>>> +             if (WARN_ON(!old_bridge_state))
>>> +                     return;
>>> +
>>> +             bridge->funcs->atomic_pre_enable(bridge, old_bridge_state);
>>> +     } else if (bridge->funcs->pre_enable) {
>>> +             bridge->funcs->pre_enable(bridge);
>>> +     }
>>> +}
>>> +
>>>   /**
>>>    * drm_atomic_bridge_chain_pre_enable - prepares for enabling all bridges in
>>>    *                                   the encoder chain
>>> @@ -718,6 +847,8 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
>>>    * &drm_bridge_funcs.pre_enable) op for all the bridges in the encoder chain,
>>>    * starting from the last bridge to the first. These are called before calling
>>>    * &drm_encoder_helper_funcs.atomic_enable
>>> + * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the pre_enable for
>>> + * the previous bridge will be called before pre_enable of this bridge.
>>>    *
>>>    * Note: the bridge passed should be the one closest to the encoder
>>>    */
>>> @@ -725,26 +856,42 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
>>>                                        struct drm_atomic_state *old_state)
>>>   {
>>>        struct drm_encoder *encoder;
>>> -     struct drm_bridge *iter;
>>> +     struct drm_bridge *iter, *next, *limit;
>>>
>>>        if (!bridge)
>>>                return;
>>>
>>>        encoder = bridge->encoder;
>>> +
>>>        list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
>>> -             if (iter->funcs->atomic_pre_enable) {
>>> -                     struct drm_bridge_state *old_bridge_state;
>>> +             if (iter->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
>>> +                     next = iter;
>>> +                     limit = bridge;
>>> +                     list_for_each_entry_from_reverse(next,
>>> +                                                      &encoder->bridge_chain,
>>> +                                                      chain_node) {
>>> +                             if (next == bridge)
>>> +                                     break;
>>> +
>>> +                             if (!(next->ops &
>>> +                                     DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
>>> +                                     limit = list_prev_entry(next, chain_node);
>>> +                                     break;
>>> +                             }
>>> +                     }
>>> +
>>> +                     list_for_each_entry_from(next, &encoder->bridge_chain, chain_node) {
>>> +                             if (next == iter)
>>> +                                     break;
>>> +
>>> +                             drm_atomic_bridge_call_pre_enable(next, old_state);
>>> +                     }
>>> +             }
>> This is hard to understand, I have trouble figuring out if it does the
>> right thing when multiple bridges set the DRM_BRIDGE_OP_UPSTREAM_FIRST
>> flag (or actually even when a single bridge does so). Comments would
>> help, but I wonder if it wouldn't be simpler to switch to a recursive
>> implementation.
> Recursive - such joys!
> With the explanation above, I'm not sure that recursive helps, but
> certainly can add comments.

It could be recursive, or just proper iteration, for example pseudocode:

list_for_each_entry_reverse(...) {
	if (iter->upstream_first && iter->prev)
		continue;
	tmp = iter;
	do {
		call_op
		tmp = tmp->next;
	} while (tmp && tmp->upstream_first);
}


Regards
Andrzej

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

[Doug Anderson]

Hi,

On Wed, Feb 16, 2022 at 9:00 AM Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi All
>
> Hopefully I've cc'ed all those that have bashed this problem around previously,
> or are otherwise linked to DRM bridges.
>
> There have been numerous discussions around how DSI support is currently broken
> as it doesn't support initialising the PHY to LP-11 and potentially the clock
> lane to HS prior to configuring the DSI peripheral. There is no op where the
> interface is initialised but HS video isn't also being sent.
> Currently you have:
> - peripheral pre_enable (host not initialised yet)
> - host pre_enable
> - encoder enable
> - host enable
> - peripheral enable (video already running)
>
> vc4 and exynos currently implement the DSI host as an encoder, and split the
> bridge_chain. This fails if you want to switch to being a bridge and/or use
> atomic calls as the state of all the elements split off are not added by
> drm_atomic_add_encoder_bridges.
>
> dw-mipi-dsi[1] and now msm[2] use the mode_set hook to initialise the PHY, so
> the bridge/panel pre_enable can send commands. In their post_disable they then
> call the downstream bridge/panel post_disable op manually so that shutdown
> commands can be sent before shutting down the PHY. Nothing handles that fact,
> so the framework then continues down the bridge chain and calls the post_disable
> again, so we get unbalanced panel prepare/unprepare calls being reported [3].
>
> There have been patches[4] proposing reversing the entire direction of
> pre_enable and post_disable, but that risks driving voltage into devices that
> have yet to be powered up.
> There have been discussions about adding either a pre_pre_enable, or adding a
> DSI host_op to initialise the host[5]. Both require significant reworking to all
> existing drivers in moving initialisation phases.
> We have patches that look like they may well be addressing race conditions in
> starting up a DSI peripheral[6].

In general I'm happy to let the more senior people in DRM set the
direction here so I probably won't do lots of review, but I will point
out that I did have another proposal that sorta got lost in the noise
of the whole "reversing the entire direction". That's basically:

https://lists.freedesktop.org/archives/dri-devel/2021-October/328934.html

I have no idea if something like that would work for your use case,
but after analyzing it it felt like a surprisingly clean proposal even
if my first instinct when I thought about it was that it was a hack.
;-) I suspect (but haven't analyzed your code) that it might be
equivalent to your proposal of using a flag but maybe easier to wrap
ones head around?


-Doug

Re: [PATCH 1/2] drm: Introduce DRM_BRIDGE_OP_UPSTREAM_FIRST to alter bridge init order

[Dave Stevenson]

Hi Andrzej

On Mon, 28 Feb 2022 at 15:36, Andrzej Hajda <andrzej.hajda@intel.com> wrote:
>
>
>
> On 22.02.2022 09:43, Dave Stevenson wrote:
> > Hi Laurent.
> >
> > Thanks for the review.
> >
> > On Tue, 22 Feb 2022 at 06:34, Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> >> Hi Dave,
> >>
> >> Thank you for the patch.
> >>
> >> On Wed, Feb 16, 2022 at 04:59:43PM +0000, Dave Stevenson wrote:
> >>> DSI sink devices typically want the DSI host powered up and configured
> >>> before they are powered up. pre_enable is the place this would normally
> >>> happen, but they are called in reverse order from panel/connector towards
> >>> the encoder, which is the "wrong" order.
> >>>
> >>> Add a new flag DRM_BRIDGE_OP_UPSTREAM_FIRST that any bridge can set
> >>> to swap the order of pre_enable (and post_disable) so that any upstream
> >>> bridges are called first to create the desired state.
> >>>
> >>> eg:
> >>> - Panel
> >>> - Bridge 1
> >>> - Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST
> >>> - Bridge 3
> >>> - Encoder
> >>> Would result in pre_enable's being called as Panel, Bridge 1, Bridge 3,
> >>> Bridge 2.
> >> If there was a Bridge 4 between Bridge 3 and Encoder, would it be
> >>
> >> Panel, Bridge 1, Bridge 3, Bridge 4, Bridge 2
> >>
> >> ? I'd capture that here, to be explicit.
> > No.
> >   - Panel
> >   - Bridge 1
> >   - Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST
> >   - Bridge 3
> >   - Bridge 4
> >    - Encoder
> > Would result in pre_enable's being called as Panel, Bridge 1, Bridge
> > 3, Bridge 2, Bridge 4, Encoder.
> > ie it only swaps the order of bridges 2 & 3.
> >
> >   - Panel
> >   - Bridge 1
> >   - Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST
> >   - Bridge 3 DRM_BRIDGE_OP_UPSTREAM_FIRST
> >   - Bridge 4
> >   - Encoder
> > Would result in pre_enable's being called as Panel, Bridge 1, Bridge
> > 4, Bridge 3, Bridge 2, Encoder.
> > (Bridge 2&3 have asked for upstream to be enabled first, which means
> > bridge 4. Bridge 2 wants upstream enabled first, which means bridge
> > 3).
> >
> >   - Panel
> >   - Bridge 1
> >   - Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST
> >   - Bridge 3
> >   - Bridge 4 DRM_BRIDGE_OP_UPSTREAM_FIRST
> >   - Bridge 5
> >   - Encoder
> > Would result in Panel, Bridge 1, Bridge 3, Bridge 2, Bridge 5, Bridge
> > 4, Encoder.
> >
> > So we only reverse the order whilst the bridges request that they want
> > upstream enabled first, but we can do that multiple times within the
> > chain. I hope that makes sense.
> >
> >>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >>> ---
> >>>   drivers/gpu/drm/drm_bridge.c | 197 +++++++++++++++++++++++++++++++++++++------
> >>>   include/drm/drm_bridge.h     |   8 ++
> >>>   2 files changed, 180 insertions(+), 25 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> >>> index c96847fc0ebc..7c24e8340efa 100644
> >>> --- a/drivers/gpu/drm/drm_bridge.c
> >>> +++ b/drivers/gpu/drm/drm_bridge.c
> >>> @@ -522,21 +522,58 @@ EXPORT_SYMBOL(drm_bridge_chain_disable);
> >>>    * Calls &drm_bridge_funcs.post_disable op for all the bridges in the
> >>>    * encoder chain, starting from the first bridge to the last. These are called
> >>>    * after completing the encoder's prepare op.
> >> Missing blank line, as well as in three locations below.
> >>
> >>> + * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the post_disable for
> >>> + * that bridge will be called before the previous one to reverse the pre_enable
> >>> + * calling direction.
> >>>    *
> >>>    * Note: the bridge passed should be the one closest to the encoder
> >>>    */
> >>>   void drm_bridge_chain_post_disable(struct drm_bridge *bridge)
> >>>   {
> >>>        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) {
> >>> +             limit = NULL;
> >>> +
> >>> +             if (!list_is_last(&bridge->chain_node, &encoder->bridge_chain)) {
> >>> +                     next = list_next_entry(bridge, chain_node);
> >>> +
> >>> +                     if (next->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
> >>> +                             limit = next;
> >>> +
> >>> +                             list_for_each_entry_from(next, &encoder->bridge_chain,
> >>> +                                                      chain_node) {
> >>> +                                     if (!(next->ops &
> >>> +                                             DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
> >>> +                                             next = list_prev_entry(next, chain_node);
> >>> +                                             limit = next;
> >>> +                                             break;
> >>> +                                     }
> >>> +                             }
> >>> +
> >>> +                             list_for_each_entry_from_reverse(next, &encoder->bridge_chain,
> >>> +                                                              chain_node) {
> >>> +                                     if (next == bridge)
> >>> +                                             break;
> >>> +
> >>> +                                     if (next->funcs->post_disable)
> >>> +                                             next->funcs->post_disable(next);
> >>> +                             }
> >>> +                     }
> >>> +             }
> >>> +
> >>>                if (bridge->funcs->post_disable)
> >>>                        bridge->funcs->post_disable(bridge);
> >>> +
> >>> +             if (limit)
> >>> +                     bridge = limit;
> >>>        }
> >>> +
> >>>   }
> >>>   EXPORT_SYMBOL(drm_bridge_chain_post_disable);
> >>>
> >>> @@ -577,22 +614,53 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set);
> >>>    * Calls &drm_bridge_funcs.pre_enable op for all the bridges in the encoder
> >>>    * chain, starting from the last bridge to the first. These are called
> >>>    * before calling the encoder's commit op.
> >>> + * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the pre_enable for
> >>> + * the previous bridge will be called before pre_enable of this bridge.
> >>>    *
> >>>    * Note: the bridge passed should be the one closest to the encoder
> >>>    */
> >>>   void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
> >>>   {
> >>>        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->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
> >>> +                     next = iter;
> >>> +                     limit = bridge;
> >>> +                     list_for_each_entry_from_reverse(next,
> >>> +                                                      &encoder->bridge_chain,
> >>> +                                                      chain_node) {
> >>> +                             if (next == bridge)
> >>> +                                     break;
> >>> +
> >>> +                             if (!(next->ops &
> >>> +                                     DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
> >>> +                                     limit = list_prev_entry(next, chain_node);
> >>> +                                     break;
> >>> +                             }
> >>> +                     }
> >>> +
> >>> +                     list_for_each_entry_from(next, &encoder->bridge_chain, chain_node) {
> >>> +                             if (next == iter)
> >>> +                                     break;
> >>> +
> >>> +                             if (next->funcs->pre_enable)
> >>> +                                     next->funcs->pre_enable(next);
> >>> +                     }
> >>> +             }
> >>> +
> >>>                if (iter->funcs->pre_enable)
> >>>                        iter->funcs->pre_enable(iter);
> >>>
> >>> +             if (iter->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST)
> >>> +                     iter = limit;
> >>> +
> >>>                if (iter == bridge)
> >>>                        break;
> >>>        }
> >>> @@ -667,6 +735,25 @@ void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge,
> >>>   }
> >>>   EXPORT_SYMBOL(drm_atomic_bridge_chain_disable);
> >>>
> >>> +static void drm_atomic_bridge_call_post_disable(struct drm_bridge *bridge,
> >>> +                                             struct drm_atomic_state *old_state)
> >>> +{
> >>> +     if (bridge->funcs->atomic_post_disable) {
> >>> +             struct drm_bridge_state *old_bridge_state;
> >>> +
> >>> +             old_bridge_state =
> >>> +                     drm_atomic_get_old_bridge_state(old_state,
> >>> +                                                     bridge);
> >>> +             if (WARN_ON(!old_bridge_state))
> >>> +                     return;
> >>> +
> >>> +             bridge->funcs->atomic_post_disable(bridge,
> >>> +                                                old_bridge_state);
> >>> +     } else if (bridge->funcs->post_disable) {
> >>> +             bridge->funcs->post_disable(bridge);
> >>> +     }
> >>> +}
> >>> +
> >>>   /**
> >>>    * drm_atomic_bridge_chain_post_disable - cleans up after disabling all bridges
> >>>    *                                     in the encoder chain
> >>> @@ -677,6 +764,9 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_disable);
> >>>    * &drm_bridge_funcs.post_disable) op for all the bridges in the encoder chain,
> >>>    * starting from the first bridge to the last. These are called after completing
> >>>    * &drm_encoder_helper_funcs.atomic_disable
> >>> + * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the post_disable for
> >>> + * that bridge will be called before the previous one to reverse the pre_enable
> >>> + * calling direction.
> >>>    *
> >>>    * Note: the bridge passed should be the one closest to the encoder
> >>>    */
> >>> @@ -684,30 +774,69 @@ void drm_atomic_bridge_chain_post_disable(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_post_disable) {
> >>> -                     struct drm_bridge_state *old_bridge_state;
> >>> +             limit = NULL;
> >>> +
> >>> +             if (!list_is_last(&bridge->chain_node, &encoder->bridge_chain)) {
> >>> +                     next = list_next_entry(bridge, chain_node);
> >>> +
> >>> +                     if (next->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
> >>> +                             limit = next;
> >>> +
> >>> +                             list_for_each_entry_from(next, &encoder->bridge_chain,
> >>> +                                                      chain_node) {
> >>> +                                     if (!(next->ops &
> >>> +                                             DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
> >>> +                                             next = list_prev_entry(next, chain_node);
> >>> +                                             limit = next;
> >>> +                                             break;
> >>> +                                     }
> >>> +                             }
> >>> +
> >>> +                             list_for_each_entry_from_reverse(next, &encoder->bridge_chain,
> >>> +                                                              chain_node) {
> >>> +                                     if (next == bridge)
> >>> +                                             break;
> >>> +
> >>> +                                     drm_atomic_bridge_call_post_disable(next,
> >>> +                                                                         old_state);
> >>> +                             }
> >>> +                     }
> >>> +             }
> >>>
> >>> -                     old_bridge_state =
> >>> -                             drm_atomic_get_old_bridge_state(old_state,
> >>> -                                                             bridge);
> >>> -                     if (WARN_ON(!old_bridge_state))
> >>> -                             return;
> >>> +             drm_atomic_bridge_call_post_disable(bridge, old_state);
> >>>
> >>> -                     bridge->funcs->atomic_post_disable(bridge,
> >>> -                                                        old_bridge_state);
> >>> -             } else if (bridge->funcs->post_disable) {
> >>> -                     bridge->funcs->post_disable(bridge);
> >>> -             }
> >>> +             if (limit)
> >>> +                     bridge = limit;
> >>>        }
> >>>   }
> >>>   EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
> >>>
> >>> +static void drm_atomic_bridge_call_pre_enable(struct drm_bridge *bridge,
> >>> +                                           struct drm_atomic_state *old_state)
> >>> +{
> >>> +     if (bridge->funcs->atomic_pre_enable) {
> >>> +             struct drm_bridge_state *old_bridge_state;
> >>> +
> >>> +             old_bridge_state =
> >>> +                     drm_atomic_get_old_bridge_state(old_state,
> >>> +                                                     bridge);
> >>> +             if (WARN_ON(!old_bridge_state))
> >>> +                     return;
> >>> +
> >>> +             bridge->funcs->atomic_pre_enable(bridge, old_bridge_state);
> >>> +     } else if (bridge->funcs->pre_enable) {
> >>> +             bridge->funcs->pre_enable(bridge);
> >>> +     }
> >>> +}
> >>> +
> >>>   /**
> >>>    * drm_atomic_bridge_chain_pre_enable - prepares for enabling all bridges in
> >>>    *                                   the encoder chain
> >>> @@ -718,6 +847,8 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
> >>>    * &drm_bridge_funcs.pre_enable) op for all the bridges in the encoder chain,
> >>>    * starting from the last bridge to the first. These are called before calling
> >>>    * &drm_encoder_helper_funcs.atomic_enable
> >>> + * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the pre_enable for
> >>> + * the previous bridge will be called before pre_enable of this bridge.
> >>>    *
> >>>    * Note: the bridge passed should be the one closest to the encoder
> >>>    */
> >>> @@ -725,26 +856,42 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
> >>>                                        struct drm_atomic_state *old_state)
> >>>   {
> >>>        struct drm_encoder *encoder;
> >>> -     struct drm_bridge *iter;
> >>> +     struct drm_bridge *iter, *next, *limit;
> >>>
> >>>        if (!bridge)
> >>>                return;
> >>>
> >>>        encoder = bridge->encoder;
> >>> +
> >>>        list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> >>> -             if (iter->funcs->atomic_pre_enable) {
> >>> -                     struct drm_bridge_state *old_bridge_state;
> >>> +             if (iter->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
> >>> +                     next = iter;
> >>> +                     limit = bridge;
> >>> +                     list_for_each_entry_from_reverse(next,
> >>> +                                                      &encoder->bridge_chain,
> >>> +                                                      chain_node) {
> >>> +                             if (next == bridge)
> >>> +                                     break;
> >>> +
> >>> +                             if (!(next->ops &
> >>> +                                     DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
> >>> +                                     limit = list_prev_entry(next, chain_node);
> >>> +                                     break;
> >>> +                             }
> >>> +                     }
> >>> +
> >>> +                     list_for_each_entry_from(next, &encoder->bridge_chain, chain_node) {
> >>> +                             if (next == iter)
> >>> +                                     break;
> >>> +
> >>> +                             drm_atomic_bridge_call_pre_enable(next, old_state);
> >>> +                     }
> >>> +             }
> >> This is hard to understand, I have trouble figuring out if it does the
> >> right thing when multiple bridges set the DRM_BRIDGE_OP_UPSTREAM_FIRST
> >> flag (or actually even when a single bridge does so). Comments would
> >> help, but I wonder if it wouldn't be simpler to switch to a recursive
> >> implementation.
> > Recursive - such joys!
> > With the explanation above, I'm not sure that recursive helps, but
> > certainly can add comments.
>
> It could be recursive, or just proper iteration, for example pseudocode:
>
> list_for_each_entry_reverse(...) {
>         if (iter->upstream_first && iter->prev)
>                 continue;
>         tmp = iter;
>         do {
>                 call_op
>                 tmp = tmp->next;
>         } while (tmp && tmp->upstream_first);
> }

You're right, using continue to skip over list entries, and then
having a single loop to work backwards works quite nicely. I'm sorting
a V2 that does that.

  Dave

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

[Dave Stevenson]

Hi Doug

On Wed, 2 Mar 2022 at 00:13, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Feb 16, 2022 at 9:00 AM Dave Stevenson
> <dave.stevenson@raspberrypi.com> wrote:
> >
> > Hi All
> >
> > Hopefully I've cc'ed all those that have bashed this problem around previously,
> > or are otherwise linked to DRM bridges.
> >
> > There have been numerous discussions around how DSI support is currently broken
> > as it doesn't support initialising the PHY to LP-11 and potentially the clock
> > lane to HS prior to configuring the DSI peripheral. There is no op where the
> > interface is initialised but HS video isn't also being sent.
> > Currently you have:
> > - peripheral pre_enable (host not initialised yet)
> > - host pre_enable
> > - encoder enable
> > - host enable
> > - peripheral enable (video already running)
> >
> > vc4 and exynos currently implement the DSI host as an encoder, and split the
> > bridge_chain. This fails if you want to switch to being a bridge and/or use
> > atomic calls as the state of all the elements split off are not added by
> > drm_atomic_add_encoder_bridges.
> >
> > dw-mipi-dsi[1] and now msm[2] use the mode_set hook to initialise the PHY, so
> > the bridge/panel pre_enable can send commands. In their post_disable they then
> > call the downstream bridge/panel post_disable op manually so that shutdown
> > commands can be sent before shutting down the PHY. Nothing handles that fact,
> > so the framework then continues down the bridge chain and calls the post_disable
> > again, so we get unbalanced panel prepare/unprepare calls being reported [3].
> >
> > There have been patches[4] proposing reversing the entire direction of
> > pre_enable and post_disable, but that risks driving voltage into devices that
> > have yet to be powered up.
> > There have been discussions about adding either a pre_pre_enable, or adding a
> > DSI host_op to initialise the host[5]. Both require significant reworking to all
> > existing drivers in moving initialisation phases.
> > We have patches that look like they may well be addressing race conditions in
> > starting up a DSI peripheral[6].
>
> In general I'm happy to let the more senior people in DRM set the
> direction here so I probably won't do lots of review, but I will point
> out that I did have another proposal that sorta got lost in the noise
> of the whole "reversing the entire direction". That's basically:
>
> https://lists.freedesktop.org/archives/dri-devel/2021-October/328934.html
>
> I have no idea if something like that would work for your use case,
> but after analyzing it it felt like a surprisingly clean proposal even
> if my first instinct when I thought about it was that it was a hack.
> ;-) I suspect (but haven't analyzed your code) that it might be
> equivalent to your proposal of using a flag but maybe easier to wrap
> ones head around?

If I'm reading that right, then you're proposing adding
after_pre_enable and before_post_disable hooks.
That's almost the same as the power_up() and power_down() ops that
Dmitry suggested earlier, or pre_pre_enable / post_post_disable that
had also been considered.

Neither of those options handles the case of a longer chain in which
two non-consecutive links want their upstream bridge enabled first.
As per the clarification in patch 1/2, considering the chain
- Panel
 - Bridge 1
 - Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST
 - Bridge 3
 - Bridge 4 DRM_BRIDGE_OP_UPSTREAM_FIRST
 - Bridge 5
 - Encoder
With the flag option we call pre_enables as Panel, Bridge 1, Bridge 3,
Bridge 2, Bridge 5, Bridge 4, Encoder.
If adding after_pre_enable, then we end up with Panel, Bridge 1,
Bridge 3, Bridge 5, Bridge 4 (after_pre_enable), Bridge 2
(after_pre_enable), Encoder.
(power_on / pre_pre_enable from encoder to connector would end up with
Bridge 5 (power_on), Bridge 3 (power_on), Bridge 1 (power_on), Panel,
Bridge 2, Bridge 4, Encoder).
Those potentially work, but it seems a less logical order compared to
using a flag to swap only the bridges of interest. I think power_on /
pre_pre_enable covers DSI better than after_pre_enable.

Adding the extra ops requires the source bridge (eg DSI host) to know
the behaviour the sink bridge/panel wants. So do all DSI hosts have to
implement power_on to power up and enter LP-11. Some DSI peripherals
may be quite happy or even prefer to have the bus totally idle /
powered down at pre_enable, but there would be no way of implementing
that.

You seem to be looking at DP, which I have very little knowledge of,
and I don't quite understand your comments about the AUX bus and how
ordering should be configured. If your panel isn't a generic driver,
couldn't it request that the upstream bridge is pre_enabled first?

  Dave

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

[Doug Anderson]

Hi,

On Wed, Mar 2, 2022 at 9:20 AM Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi Doug
>
> On Wed, 2 Mar 2022 at 00:13, Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Wed, Feb 16, 2022 at 9:00 AM Dave Stevenson
> > <dave.stevenson@raspberrypi.com> wrote:
> > >
> > > Hi All
> > >
> > > Hopefully I've cc'ed all those that have bashed this problem around previously,
> > > or are otherwise linked to DRM bridges.
> > >
> > > There have been numerous discussions around how DSI support is currently broken
> > > as it doesn't support initialising the PHY to LP-11 and potentially the clock
> > > lane to HS prior to configuring the DSI peripheral. There is no op where the
> > > interface is initialised but HS video isn't also being sent.
> > > Currently you have:
> > > - peripheral pre_enable (host not initialised yet)
> > > - host pre_enable
> > > - encoder enable
> > > - host enable
> > > - peripheral enable (video already running)
> > >
> > > vc4 and exynos currently implement the DSI host as an encoder, and split the
> > > bridge_chain. This fails if you want to switch to being a bridge and/or use
> > > atomic calls as the state of all the elements split off are not added by
> > > drm_atomic_add_encoder_bridges.
> > >
> > > dw-mipi-dsi[1] and now msm[2] use the mode_set hook to initialise the PHY, so
> > > the bridge/panel pre_enable can send commands. In their post_disable they then
> > > call the downstream bridge/panel post_disable op manually so that shutdown
> > > commands can be sent before shutting down the PHY. Nothing handles that fact,
> > > so the framework then continues down the bridge chain and calls the post_disable
> > > again, so we get unbalanced panel prepare/unprepare calls being reported [3].
> > >
> > > There have been patches[4] proposing reversing the entire direction of
> > > pre_enable and post_disable, but that risks driving voltage into devices that
> > > have yet to be powered up.
> > > There have been discussions about adding either a pre_pre_enable, or adding a
> > > DSI host_op to initialise the host[5]. Both require significant reworking to all
> > > existing drivers in moving initialisation phases.
> > > We have patches that look like they may well be addressing race conditions in
> > > starting up a DSI peripheral[6].
> >
> > In general I'm happy to let the more senior people in DRM set the
> > direction here so I probably won't do lots of review, but I will point
> > out that I did have another proposal that sorta got lost in the noise
> > of the whole "reversing the entire direction". That's basically:
> >
> > https://lists.freedesktop.org/archives/dri-devel/2021-October/328934.html
> >
> > I have no idea if something like that would work for your use case,
> > but after analyzing it it felt like a surprisingly clean proposal even
> > if my first instinct when I thought about it was that it was a hack.
> > ;-) I suspect (but haven't analyzed your code) that it might be
> > equivalent to your proposal of using a flag but maybe easier to wrap
> > ones head around?
>
> If I'm reading that right, then you're proposing adding
> after_pre_enable and before_post_disable hooks.
> That's almost the same as the power_up() and power_down() ops that
> Dmitry suggested earlier, or pre_pre_enable / post_post_disable that
> had also been considered.
>
> Neither of those options handles the case of a longer chain in which
> two non-consecutive links want their upstream bridge enabled first.
> As per the clarification in patch 1/2, considering the chain
> - Panel
>  - Bridge 1
>  - Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST
>  - Bridge 3
>  - Bridge 4 DRM_BRIDGE_OP_UPSTREAM_FIRST
>  - Bridge 5
>  - Encoder
> With the flag option we call pre_enables as Panel, Bridge 1, Bridge 3,
> Bridge 2, Bridge 5, Bridge 4, Encoder.
> If adding after_pre_enable, then we end up with Panel, Bridge 1,
> Bridge 3, Bridge 5, Bridge 4 (after_pre_enable), Bridge 2
> (after_pre_enable), Encoder.
> (power_on / pre_pre_enable from encoder to connector would end up with
> Bridge 5 (power_on), Bridge 3 (power_on), Bridge 1 (power_on), Panel,
> Bridge 2, Bridge 4, Encoder).
> Those potentially work, but it seems a less logical order compared to
> using a flag to swap only the bridges of interest. I think power_on /
> pre_pre_enable covers DSI better than after_pre_enable.
>
> Adding the extra ops requires the source bridge (eg DSI host) to know
> the behaviour the sink bridge/panel wants. So do all DSI hosts have to
> implement power_on to power up and enter LP-11. Some DSI peripherals
> may be quite happy or even prefer to have the bus totally idle /
> powered down at pre_enable, but there would be no way of implementing
> that.

Ah, that makes it super clear, thanks! :-) If the local swap of just
two components that you're doing is more useful to you than the two
stage approach and everyone likes it then I have no objections.


> You seem to be looking at DP, which I have very little knowledge of,
> and I don't quite understand your comments about the AUX bus and how
> ordering should be configured. If your panel isn't a generic driver,
> couldn't it request that the upstream bridge is pre_enabled first?

I basically ended up solving my problem in a different way, so I have
no immediate need of swapping the order right now. I am happy you are
tackling it, though, and I can definitely see myself needing something
like this in the future. :-)

-Doug