[RFT PATCH v2 1/3] drm/bridge: tc358762: Set pre_enable_prev_first

[RFT PATCH v2 1/3] drm/bridge: tc358762: Set pre_enable_prev_first

[Douglas Anderson]

Set the "pre_enable_prev_first" as provided by commit 4fb912e5e190
("drm/bridge: Introduce pre_enable_prev_first to alter bridge init
order"). This should allow us to revert commit ec7981e6c614
("drm/msm/dsi: don't powerup at modeset time for parade-ps8640") and
commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
time").

Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

(no changes since v1)

 drivers/gpu/drm/bridge/tc358762.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/bridge/tc358762.c b/drivers/gpu/drm/bridge/tc358762.c
index 0b6a28436885..77f7f7f54757 100644
--- a/drivers/gpu/drm/bridge/tc358762.c
+++ b/drivers/gpu/drm/bridge/tc358762.c
@@ -229,6 +229,7 @@ static int tc358762_probe(struct mipi_dsi_device *dsi)
 	ctx->bridge.funcs = &tc358762_bridge_funcs;
 	ctx->bridge.type = DRM_MODE_CONNECTOR_DPI;
 	ctx->bridge.of_node = dev->of_node;
+	ctx->bridge.pre_enable_prev_first = true;
 
 	drm_bridge_add(&ctx->bridge);
 
-- 
2.39.1.456.gfc5497dd1b-goog

[RFT PATCH v2 1/3] drm/bridge: tc358762: Set pre_enable_prev_first

[Douglas Anderson]

Set the "pre_enable_prev_first" as provided by commit 4fb912e5e190
("drm/bridge: Introduce pre_enable_prev_first to alter bridge init
order"). This should allow us to revert commit ec7981e6c614
("drm/msm/dsi: don't powerup at modeset time for parade-ps8640") and
commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
time").

Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

(no changes since v1)

 drivers/gpu/drm/bridge/tc358762.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/bridge/tc358762.c b/drivers/gpu/drm/bridge/tc358762.c
index 0b6a28436885..77f7f7f54757 100644
--- a/drivers/gpu/drm/bridge/tc358762.c
+++ b/drivers/gpu/drm/bridge/tc358762.c
@@ -229,6 +229,7 @@ static int tc358762_probe(struct mipi_dsi_device *dsi)
 	ctx->bridge.funcs = &tc358762_bridge_funcs;
 	ctx->bridge.type = DRM_MODE_CONNECTOR_DPI;
 	ctx->bridge.of_node = dev->of_node;
+	ctx->bridge.pre_enable_prev_first = true;
 
 	drm_bridge_add(&ctx->bridge);
 
-- 
2.39.1.456.gfc5497dd1b-goog

[RFT PATCH v2 2/3] drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset

[Douglas Anderson]

In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
time"), we moved powering up DSI hosts to modeset time. This wasn't
because it was an elegant design, but there were no better options.

That commit actually ended up breaking ps8640, and thus was born
commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time for
parade-ps8640") as a temporary hack to un-break ps8640 by moving it to
the old way of doing things. It turns out that ps8640 _really_ doesn't
like its pre_enable() function to be called after
dsi_mgr_bridge_power_on(). Specifically (from experimentation, not
because I have any inside knowledge), it looks like the assertion of
"RST#" in the ps8640 runtime resume handler seems like it's not
allowed to happen after dsi_mgr_bridge_power_on()

Recently, Dave Stevenson's series landed allowing bridges some control
over pre_enable ordering. The meaty commit for our purposes is commit
4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter
bridge init order"). As documented by that series, if a bridge doesn't
set "pre_enable_prev_first" then we should use the old ordering.

Now that we have the commit ("drm/bridge: tc358762: Set
pre_enable_prev_first") we can go back to the old ordering, which also
allows us to remove the ps8640 special case.

One last note is that even without reverting commit 7d8e9a90509f
("drm/msm/dsi: move DSI host powerup to modeset time"), if you _just_
revert the ps8640 special case and try it out then it doesn't seem to
fail anymore. I spent time bisecting / debugging this and it turns out
to be mostly luck, so we still want this patch to make sure it's
solid. Specifically the reason it sorta works these days is because
we implemented wait_hpd_asserted() in ps8640 now, plus the magic of
"pm_runtime" autosuspend. The fact that we have wait_hpd_asserted()
implemented means that we actually power the bridge chip up just a wee
bit earlier and then the bridge happens to stay on because of
autosuspend and thus ends up powered before dsi_mgr_bridge_power_on().

Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- Don't fold dsi_mgr_bridge_power_on() back into dsi_mgr_bridge_pre_enable()

 drivers/gpu/drm/msm/dsi/dsi_manager.c | 38 +--------------------------
 1 file changed, 1 insertion(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 1bbac72dad35..2197a54b9b96 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -34,32 +34,6 @@ static struct msm_dsi_manager msm_dsim_glb;
 #define IS_SYNC_NEEDED()	(msm_dsim_glb.is_sync_needed)
 #define IS_MASTER_DSI_LINK(id)	(msm_dsim_glb.master_dsi_link_id == id)
 
-#ifdef CONFIG_OF
-static bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
-{
-	struct drm_bridge *next_bridge = drm_bridge_get_next_bridge(bridge);
-
-	/*
-	 * If the next bridge in the chain is the Parade ps8640 bridge chip
-	 * then don't power on early since it seems to violate the expectations
-	 * of the firmware that the bridge chip is running.
-	 *
-	 * NOTE: this is expected to be a temporary special case. It's expected
-	 * that we'll eventually have a framework that allows the next level
-	 * bridge to indicate whether it needs us to power on before it or
-	 * after it. When that framework is in place then we'll use it and
-	 * remove this special case.
-	 */
-	return !(next_bridge && next_bridge->of_node &&
-		 of_device_is_compatible(next_bridge->of_node, "parade,ps8640"));
-}
-#else
-static inline bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
-{
-	return true;
-}
-#endif
-
 static inline struct msm_dsi *dsi_mgr_get_dsi(int id)
 {
 	return msm_dsim_glb.dsi[id];
@@ -265,12 +239,6 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
 	int ret;
 
 	DBG("id=%d", id);
-	if (!msm_dsi_device_connected(msm_dsi))
-		return;
-
-	/* Do nothing with the host if it is slave-DSI in case of bonded DSI */
-	if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
-		return;
 
 	ret = dsi_mgr_phy_enable(id, phy_shared_timings);
 	if (ret)
@@ -327,8 +295,7 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
 	if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
 		return;
 
-	if (!dsi_mgr_power_on_early(bridge))
-		dsi_mgr_bridge_power_on(bridge);
+	dsi_mgr_bridge_power_on(bridge);
 
 	ret = msm_dsi_host_enable(host);
 	if (ret) {
@@ -438,9 +405,6 @@ static void dsi_mgr_bridge_mode_set(struct drm_bridge *bridge,
 	msm_dsi_host_set_display_mode(host, adjusted_mode);
 	if (is_bonded_dsi && other_dsi)
 		msm_dsi_host_set_display_mode(other_dsi->host, adjusted_mode);
-
-	if (dsi_mgr_power_on_early(bridge))
-		dsi_mgr_bridge_power_on(bridge);
 }
 
 static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,
-- 
2.39.1.456.gfc5497dd1b-goog

[RFT PATCH v2 2/3] drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset

[Douglas Anderson]

In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
time"), we moved powering up DSI hosts to modeset time. This wasn't
because it was an elegant design, but there were no better options.

That commit actually ended up breaking ps8640, and thus was born
commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time for
parade-ps8640") as a temporary hack to un-break ps8640 by moving it to
the old way of doing things. It turns out that ps8640 _really_ doesn't
like its pre_enable() function to be called after
dsi_mgr_bridge_power_on(). Specifically (from experimentation, not
because I have any inside knowledge), it looks like the assertion of
"RST#" in the ps8640 runtime resume handler seems like it's not
allowed to happen after dsi_mgr_bridge_power_on()

Recently, Dave Stevenson's series landed allowing bridges some control
over pre_enable ordering. The meaty commit for our purposes is commit
4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter
bridge init order"). As documented by that series, if a bridge doesn't
set "pre_enable_prev_first" then we should use the old ordering.

Now that we have the commit ("drm/bridge: tc358762: Set
pre_enable_prev_first") we can go back to the old ordering, which also
allows us to remove the ps8640 special case.

One last note is that even without reverting commit 7d8e9a90509f
("drm/msm/dsi: move DSI host powerup to modeset time"), if you _just_
revert the ps8640 special case and try it out then it doesn't seem to
fail anymore. I spent time bisecting / debugging this and it turns out
to be mostly luck, so we still want this patch to make sure it's
solid. Specifically the reason it sorta works these days is because
we implemented wait_hpd_asserted() in ps8640 now, plus the magic of
"pm_runtime" autosuspend. The fact that we have wait_hpd_asserted()
implemented means that we actually power the bridge chip up just a wee
bit earlier and then the bridge happens to stay on because of
autosuspend and thus ends up powered before dsi_mgr_bridge_power_on().

Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- Don't fold dsi_mgr_bridge_power_on() back into dsi_mgr_bridge_pre_enable()

 drivers/gpu/drm/msm/dsi/dsi_manager.c | 38 +--------------------------
 1 file changed, 1 insertion(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 1bbac72dad35..2197a54b9b96 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -34,32 +34,6 @@ static struct msm_dsi_manager msm_dsim_glb;
 #define IS_SYNC_NEEDED()	(msm_dsim_glb.is_sync_needed)
 #define IS_MASTER_DSI_LINK(id)	(msm_dsim_glb.master_dsi_link_id == id)
 
-#ifdef CONFIG_OF
-static bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
-{
-	struct drm_bridge *next_bridge = drm_bridge_get_next_bridge(bridge);
-
-	/*
-	 * If the next bridge in the chain is the Parade ps8640 bridge chip
-	 * then don't power on early since it seems to violate the expectations
-	 * of the firmware that the bridge chip is running.
-	 *
-	 * NOTE: this is expected to be a temporary special case. It's expected
-	 * that we'll eventually have a framework that allows the next level
-	 * bridge to indicate whether it needs us to power on before it or
-	 * after it. When that framework is in place then we'll use it and
-	 * remove this special case.
-	 */
-	return !(next_bridge && next_bridge->of_node &&
-		 of_device_is_compatible(next_bridge->of_node, "parade,ps8640"));
-}
-#else
-static inline bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
-{
-	return true;
-}
-#endif
-
 static inline struct msm_dsi *dsi_mgr_get_dsi(int id)
 {
 	return msm_dsim_glb.dsi[id];
@@ -265,12 +239,6 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
 	int ret;
 
 	DBG("id=%d", id);
-	if (!msm_dsi_device_connected(msm_dsi))
-		return;
-
-	/* Do nothing with the host if it is slave-DSI in case of bonded DSI */
-	if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
-		return;
 
 	ret = dsi_mgr_phy_enable(id, phy_shared_timings);
 	if (ret)
@@ -327,8 +295,7 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
 	if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
 		return;
 
-	if (!dsi_mgr_power_on_early(bridge))
-		dsi_mgr_bridge_power_on(bridge);
+	dsi_mgr_bridge_power_on(bridge);
 
 	ret = msm_dsi_host_enable(host);
 	if (ret) {
@@ -438,9 +405,6 @@ static void dsi_mgr_bridge_mode_set(struct drm_bridge *bridge,
 	msm_dsi_host_set_display_mode(host, adjusted_mode);
 	if (is_bonded_dsi && other_dsi)
 		msm_dsi_host_set_display_mode(other_dsi->host, adjusted_mode);
-
-	if (dsi_mgr_power_on_early(bridge))
-		dsi_mgr_bridge_power_on(bridge);
 }
 
 static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,
-- 
2.39.1.456.gfc5497dd1b-goog

[RFT PATCH v2 3/3] drm/msm/dsi: More properly handle errors in regards to dsi_mgr_bridge_power_on()

[Douglas Anderson]

In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
time") the error handling with regards to dsi_mgr_bridge_power_on()
got a bit worse. Specifically if we failed to power the bridge on then
nothing would really notice. The modeset function couldn't return an
error and thus we'd blindly go forward and try to do the pre-enable.

In commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time
for parade-ps8640") we added a special case to move the powerup back
to pre-enable time for ps8640. When we did that, we didn't try to
recover the old/better error handling just for ps8640.

In the patch ("drm/msm/dsi: Stop unconditionally powering up DSI hosts
at modeset") we've now moved the powering up back to exclusively being
during pre-enable. That means we can add the better error handling
back in, so let's do it. To do so we'll add a new function
dsi_mgr_bridge_power_off() that's matches how errors were handled
prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to
modeset time").

NOTE: Now that we have dsi_mgr_bridge_power_off(), it feels as if we
should be calling it in dsi_mgr_bridge_post_disable(). That would make
some sense, but doing so would change the current behavior and thus
should be a separate patch. Specifically:
* dsi_mgr_bridge_post_disable() always calls dsi_mgr_phy_disable()
  even in the slave-DSI case of bonded DSI. We'd need to add special
  handling for this if it's truly needed.
* dsi_mgr_bridge_post_disable() calls msm_dsi_phy_pll_save_state()
  midway through the poweroff.
* dsi_mgr_bridge_post_disable() has a different order of some of the
  poweroffs / IRQ disables.
For now we'll leave dsi_mgr_bridge_post_disable() alone.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- ("More properly handle errors...") new for v2.

 drivers/gpu/drm/msm/dsi/dsi_manager.c | 32 ++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 2197a54b9b96..28b8012a21f2 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -228,7 +228,7 @@ static void msm_dsi_manager_set_split_display(u8 id)
 	}
 }
 
-static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
+static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
 {
 	int id = dsi_mgr_bridge_get_id(bridge);
 	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
@@ -268,14 +268,31 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
 	if (is_bonded_dsi && msm_dsi1)
 		msm_dsi_host_enable_irq(msm_dsi1->host);
 
-	return;
+	return 0;
 
 host1_on_fail:
 	msm_dsi_host_power_off(host);
 host_on_fail:
 	dsi_mgr_phy_disable(id);
 phy_en_fail:
-	return;
+	return ret;
+}
+
+static void dsi_mgr_bridge_power_off(struct drm_bridge *bridge)
+{
+	int id = dsi_mgr_bridge_get_id(bridge);
+	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
+	struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1);
+	struct mipi_dsi_host *host = msm_dsi->host;
+	bool is_bonded_dsi = IS_BONDED_DSI();
+
+	msm_dsi_host_disable_irq(host);
+	if (is_bonded_dsi && msm_dsi1) {
+		msm_dsi_host_disable_irq(msm_dsi1->host);
+		msm_dsi_host_power_off(msm_dsi1->host);
+	}
+	msm_dsi_host_power_off(host);
+	dsi_mgr_phy_disable(id);
 }
 
 static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
@@ -295,7 +312,11 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
 	if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
 		return;
 
-	dsi_mgr_bridge_power_on(bridge);
+	ret = dsi_mgr_bridge_power_on(bridge);
+	if (ret) {
+		dev_err(&msm_dsi->pdev->dev, "Power on failed: %d\n", ret);
+		return;
+	}
 
 	ret = msm_dsi_host_enable(host);
 	if (ret) {
@@ -316,8 +337,7 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
 host1_en_fail:
 	msm_dsi_host_disable(host);
 host_en_fail:
-
-	return;
+	dsi_mgr_bridge_power_off(bridge);
 }
 
 void msm_dsi_manager_tpg_enable(void)
-- 
2.39.1.456.gfc5497dd1b-goog

[RFT PATCH v2 3/3] drm/msm/dsi: More properly handle errors in regards to dsi_mgr_bridge_power_on()

[Douglas Anderson]

In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
time") the error handling with regards to dsi_mgr_bridge_power_on()
got a bit worse. Specifically if we failed to power the bridge on then
nothing would really notice. The modeset function couldn't return an
error and thus we'd blindly go forward and try to do the pre-enable.

In commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time
for parade-ps8640") we added a special case to move the powerup back
to pre-enable time for ps8640. When we did that, we didn't try to
recover the old/better error handling just for ps8640.

In the patch ("drm/msm/dsi: Stop unconditionally powering up DSI hosts
at modeset") we've now moved the powering up back to exclusively being
during pre-enable. That means we can add the better error handling
back in, so let's do it. To do so we'll add a new function
dsi_mgr_bridge_power_off() that's matches how errors were handled
prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to
modeset time").

NOTE: Now that we have dsi_mgr_bridge_power_off(), it feels as if we
should be calling it in dsi_mgr_bridge_post_disable(). That would make
some sense, but doing so would change the current behavior and thus
should be a separate patch. Specifically:
* dsi_mgr_bridge_post_disable() always calls dsi_mgr_phy_disable()
  even in the slave-DSI case of bonded DSI. We'd need to add special
  handling for this if it's truly needed.
* dsi_mgr_bridge_post_disable() calls msm_dsi_phy_pll_save_state()
  midway through the poweroff.
* dsi_mgr_bridge_post_disable() has a different order of some of the
  poweroffs / IRQ disables.
For now we'll leave dsi_mgr_bridge_post_disable() alone.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- ("More properly handle errors...") new for v2.

 drivers/gpu/drm/msm/dsi/dsi_manager.c | 32 ++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 2197a54b9b96..28b8012a21f2 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -228,7 +228,7 @@ static void msm_dsi_manager_set_split_display(u8 id)
 	}
 }
 
-static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
+static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
 {
 	int id = dsi_mgr_bridge_get_id(bridge);
 	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
@@ -268,14 +268,31 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
 	if (is_bonded_dsi && msm_dsi1)
 		msm_dsi_host_enable_irq(msm_dsi1->host);
 
-	return;
+	return 0;
 
 host1_on_fail:
 	msm_dsi_host_power_off(host);
 host_on_fail:
 	dsi_mgr_phy_disable(id);
 phy_en_fail:
-	return;
+	return ret;
+}
+
+static void dsi_mgr_bridge_power_off(struct drm_bridge *bridge)
+{
+	int id = dsi_mgr_bridge_get_id(bridge);
+	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
+	struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1);
+	struct mipi_dsi_host *host = msm_dsi->host;
+	bool is_bonded_dsi = IS_BONDED_DSI();
+
+	msm_dsi_host_disable_irq(host);
+	if (is_bonded_dsi && msm_dsi1) {
+		msm_dsi_host_disable_irq(msm_dsi1->host);
+		msm_dsi_host_power_off(msm_dsi1->host);
+	}
+	msm_dsi_host_power_off(host);
+	dsi_mgr_phy_disable(id);
 }
 
 static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
@@ -295,7 +312,11 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
 	if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
 		return;
 
-	dsi_mgr_bridge_power_on(bridge);
+	ret = dsi_mgr_bridge_power_on(bridge);
+	if (ret) {
+		dev_err(&msm_dsi->pdev->dev, "Power on failed: %d\n", ret);
+		return;
+	}
 
 	ret = msm_dsi_host_enable(host);
 	if (ret) {
@@ -316,8 +337,7 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
 host1_en_fail:
 	msm_dsi_host_disable(host);
 host_en_fail:
-
-	return;
+	dsi_mgr_bridge_power_off(bridge);
 }
 
 void msm_dsi_manager_tpg_enable(void)
-- 
2.39.1.456.gfc5497dd1b-goog

Re: [RFT PATCH v2 2/3] drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset

[Abhinav Kumar]

On 1/31/2023 2:18 PM, Douglas Anderson wrote:
> In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> time"), we moved powering up DSI hosts to modeset time. This wasn't
> because it was an elegant design, but there were no better options.
> 
> That commit actually ended up breaking ps8640, and thus was born
> commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time for
> parade-ps8640") as a temporary hack to un-break ps8640 by moving it to
> the old way of doing things. It turns out that ps8640 _really_ doesn't
> like its pre_enable() function to be called after
> dsi_mgr_bridge_power_on(). Specifically (from experimentation, not
> because I have any inside knowledge), it looks like the assertion of
> "RST#" in the ps8640 runtime resume handler seems like it's not
> allowed to happen after dsi_mgr_bridge_power_on()
> 
> Recently, Dave Stevenson's series landed allowing bridges some control
> over pre_enable ordering. The meaty commit for our purposes is commit
> 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter
> bridge init order"). As documented by that series, if a bridge doesn't
> set "pre_enable_prev_first" then we should use the old ordering.
> 
> Now that we have the commit ("drm/bridge: tc358762: Set
> pre_enable_prev_first") we can go back to the old ordering, which also
> allows us to remove the ps8640 special case.
> 
> One last note is that even without reverting commit 7d8e9a90509f
> ("drm/msm/dsi: move DSI host powerup to modeset time"), if you _just_
> revert the ps8640 special case and try it out then it doesn't seem to
> fail anymore. I spent time bisecting / debugging this and it turns out
> to be mostly luck, so we still want this patch to make sure it's
> solid. Specifically the reason it sorta works these days is because
> we implemented wait_hpd_asserted() in ps8640 now, plus the magic of
> "pm_runtime" autosuspend. The fact that we have wait_hpd_asserted()
> implemented means that we actually power the bridge chip up just a wee
> bit earlier and then the bridge happens to stay on because of
> autosuspend and thus ends up powered before dsi_mgr_bridge_power_on().
> 
> Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
> Changes in v2:
> - Don't fold dsi_mgr_bridge_power_on() back into dsi_mgr_bridge_pre_enable()
> 
>   drivers/gpu/drm/msm/dsi/dsi_manager.c | 38 +--------------------------
>   1 file changed, 1 insertion(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> index 1bbac72dad35..2197a54b9b96 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> @@ -34,32 +34,6 @@ static struct msm_dsi_manager msm_dsim_glb;
>   #define IS_SYNC_NEEDED()	(msm_dsim_glb.is_sync_needed)
>   #define IS_MASTER_DSI_LINK(id)	(msm_dsim_glb.master_dsi_link_id == id)
>   
> -#ifdef CONFIG_OF
> -static bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
> -{
> -	struct drm_bridge *next_bridge = drm_bridge_get_next_bridge(bridge);
> -
> -	/*
> -	 * If the next bridge in the chain is the Parade ps8640 bridge chip
> -	 * then don't power on early since it seems to violate the expectations
> -	 * of the firmware that the bridge chip is running.
> -	 *
> -	 * NOTE: this is expected to be a temporary special case. It's expected
> -	 * that we'll eventually have a framework that allows the next level
> -	 * bridge to indicate whether it needs us to power on before it or
> -	 * after it. When that framework is in place then we'll use it and
> -	 * remove this special case.
> -	 */
> -	return !(next_bridge && next_bridge->of_node &&
> -		 of_device_is_compatible(next_bridge->of_node, "parade,ps8640"));
> -}
> -#else
> -static inline bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
> -{
> -	return true;
> -}
> -#endif
> -
>   static inline struct msm_dsi *dsi_mgr_get_dsi(int id)
>   {
>   	return msm_dsim_glb.dsi[id];
> @@ -265,12 +239,6 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
>   	int ret;
>   
>   	DBG("id=%d", id);
> -	if (!msm_dsi_device_connected(msm_dsi))
> -		return;
> -
> -	/* Do nothing with the host if it is slave-DSI in case of bonded DSI */
> -	if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
> -		return;
>   

Why are these two checks removed?

>   	ret = dsi_mgr_phy_enable(id, phy_shared_timings);
>   	if (ret)
> @@ -327,8 +295,7 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
>   	if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
>   		return;
>   
> -	if (!dsi_mgr_power_on_early(bridge))
> -		dsi_mgr_bridge_power_on(bridge);
> +	dsi_mgr_bridge_power_on(bridge);
>   
>   	ret = msm_dsi_host_enable(host);
>   	if (ret) {
> @@ -438,9 +405,6 @@ static void dsi_mgr_bridge_mode_set(struct drm_bridge *bridge,
>   	msm_dsi_host_set_display_mode(host, adjusted_mode);
>   	if (is_bonded_dsi && other_dsi)
>   		msm_dsi_host_set_display_mode(other_dsi->host, adjusted_mode);
> -
> -	if (dsi_mgr_power_on_early(bridge))
> -		dsi_mgr_bridge_power_on(bridge);
>   }
>   
>   static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,

Re: [RFT PATCH v2 2/3] drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset

[Abhinav Kumar]

On 1/31/2023 2:18 PM, Douglas Anderson wrote:
> In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> time"), we moved powering up DSI hosts to modeset time. This wasn't
> because it was an elegant design, but there were no better options.
> 
> That commit actually ended up breaking ps8640, and thus was born
> commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time for
> parade-ps8640") as a temporary hack to un-break ps8640 by moving it to
> the old way of doing things. It turns out that ps8640 _really_ doesn't
> like its pre_enable() function to be called after
> dsi_mgr_bridge_power_on(). Specifically (from experimentation, not
> because I have any inside knowledge), it looks like the assertion of
> "RST#" in the ps8640 runtime resume handler seems like it's not
> allowed to happen after dsi_mgr_bridge_power_on()
> 
> Recently, Dave Stevenson's series landed allowing bridges some control
> over pre_enable ordering. The meaty commit for our purposes is commit
> 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter
> bridge init order"). As documented by that series, if a bridge doesn't
> set "pre_enable_prev_first" then we should use the old ordering.
> 
> Now that we have the commit ("drm/bridge: tc358762: Set
> pre_enable_prev_first") we can go back to the old ordering, which also
> allows us to remove the ps8640 special case.
> 
> One last note is that even without reverting commit 7d8e9a90509f
> ("drm/msm/dsi: move DSI host powerup to modeset time"), if you _just_
> revert the ps8640 special case and try it out then it doesn't seem to
> fail anymore. I spent time bisecting / debugging this and it turns out
> to be mostly luck, so we still want this patch to make sure it's
> solid. Specifically the reason it sorta works these days is because
> we implemented wait_hpd_asserted() in ps8640 now, plus the magic of
> "pm_runtime" autosuspend. The fact that we have wait_hpd_asserted()
> implemented means that we actually power the bridge chip up just a wee
> bit earlier and then the bridge happens to stay on because of
> autosuspend and thus ends up powered before dsi_mgr_bridge_power_on().
> 
> Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
> Changes in v2:
> - Don't fold dsi_mgr_bridge_power_on() back into dsi_mgr_bridge_pre_enable()
> 
>   drivers/gpu/drm/msm/dsi/dsi_manager.c | 38 +--------------------------
>   1 file changed, 1 insertion(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> index 1bbac72dad35..2197a54b9b96 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> @@ -34,32 +34,6 @@ static struct msm_dsi_manager msm_dsim_glb;
>   #define IS_SYNC_NEEDED()	(msm_dsim_glb.is_sync_needed)
>   #define IS_MASTER_DSI_LINK(id)	(msm_dsim_glb.master_dsi_link_id == id)
>   
> -#ifdef CONFIG_OF
> -static bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
> -{
> -	struct drm_bridge *next_bridge = drm_bridge_get_next_bridge(bridge);
> -
> -	/*
> -	 * If the next bridge in the chain is the Parade ps8640 bridge chip
> -	 * then don't power on early since it seems to violate the expectations
> -	 * of the firmware that the bridge chip is running.
> -	 *
> -	 * NOTE: this is expected to be a temporary special case. It's expected
> -	 * that we'll eventually have a framework that allows the next level
> -	 * bridge to indicate whether it needs us to power on before it or
> -	 * after it. When that framework is in place then we'll use it and
> -	 * remove this special case.
> -	 */
> -	return !(next_bridge && next_bridge->of_node &&
> -		 of_device_is_compatible(next_bridge->of_node, "parade,ps8640"));
> -}
> -#else
> -static inline bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
> -{
> -	return true;
> -}
> -#endif
> -
>   static inline struct msm_dsi *dsi_mgr_get_dsi(int id)
>   {
>   	return msm_dsim_glb.dsi[id];
> @@ -265,12 +239,6 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
>   	int ret;
>   
>   	DBG("id=%d", id);
> -	if (!msm_dsi_device_connected(msm_dsi))
> -		return;
> -
> -	/* Do nothing with the host if it is slave-DSI in case of bonded DSI */
> -	if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
> -		return;
>   

Why are these two checks removed?

>   	ret = dsi_mgr_phy_enable(id, phy_shared_timings);
>   	if (ret)
> @@ -327,8 +295,7 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
>   	if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
>   		return;
>   
> -	if (!dsi_mgr_power_on_early(bridge))
> -		dsi_mgr_bridge_power_on(bridge);
> +	dsi_mgr_bridge_power_on(bridge);
>   
>   	ret = msm_dsi_host_enable(host);
>   	if (ret) {
> @@ -438,9 +405,6 @@ static void dsi_mgr_bridge_mode_set(struct drm_bridge *bridge,
>   	msm_dsi_host_set_display_mode(host, adjusted_mode);
>   	if (is_bonded_dsi && other_dsi)
>   		msm_dsi_host_set_display_mode(other_dsi->host, adjusted_mode);
> -
> -	if (dsi_mgr_power_on_early(bridge))
> -		dsi_mgr_bridge_power_on(bridge);
>   }
>   
>   static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,

Re: [RFT PATCH v2 1/3] drm/bridge: tc358762: Set pre_enable_prev_first

[Dave Stevenson]

On Tue, 31 Jan 2023 at 22:22, Douglas Anderson <dianders@chromium.org> wrote:
>
> Set the "pre_enable_prev_first" as provided by commit 4fb912e5e190
> ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init
> order"). This should allow us to revert commit ec7981e6c614
> ("drm/msm/dsi: don't powerup at modeset time for parade-ps8640") and
> commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> time").

I see no reference in the TC358762 datasheet to requiring the DSI
interface to be in any particular state.
However, setting this flag does mean that the DSI host doesn't need to
power up and down for each host_transfer request from
tc358762_pre_enable/tc358762_init, so on that basis I'm good with it.

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> (no changes since v1)
>
>  drivers/gpu/drm/bridge/tc358762.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/bridge/tc358762.c b/drivers/gpu/drm/bridge/tc358762.c
> index 0b6a28436885..77f7f7f54757 100644
> --- a/drivers/gpu/drm/bridge/tc358762.c
> +++ b/drivers/gpu/drm/bridge/tc358762.c
> @@ -229,6 +229,7 @@ static int tc358762_probe(struct mipi_dsi_device *dsi)
>         ctx->bridge.funcs = &tc358762_bridge_funcs;
>         ctx->bridge.type = DRM_MODE_CONNECTOR_DPI;
>         ctx->bridge.of_node = dev->of_node;
> +       ctx->bridge.pre_enable_prev_first = true;
>
>         drm_bridge_add(&ctx->bridge);
>
> --
> 2.39.1.456.gfc5497dd1b-goog
>

Re: [RFT PATCH v2 1/3] drm/bridge: tc358762: Set pre_enable_prev_first

[Dave Stevenson]

On Tue, 31 Jan 2023 at 22:22, Douglas Anderson <dianders@chromium.org> wrote:
>
> Set the "pre_enable_prev_first" as provided by commit 4fb912e5e190
> ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init
> order"). This should allow us to revert commit ec7981e6c614
> ("drm/msm/dsi: don't powerup at modeset time for parade-ps8640") and
> commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> time").

I see no reference in the TC358762 datasheet to requiring the DSI
interface to be in any particular state.
However, setting this flag does mean that the DSI host doesn't need to
power up and down for each host_transfer request from
tc358762_pre_enable/tc358762_init, so on that basis I'm good with it.

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> (no changes since v1)
>
>  drivers/gpu/drm/bridge/tc358762.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/bridge/tc358762.c b/drivers/gpu/drm/bridge/tc358762.c
> index 0b6a28436885..77f7f7f54757 100644
> --- a/drivers/gpu/drm/bridge/tc358762.c
> +++ b/drivers/gpu/drm/bridge/tc358762.c
> @@ -229,6 +229,7 @@ static int tc358762_probe(struct mipi_dsi_device *dsi)
>         ctx->bridge.funcs = &tc358762_bridge_funcs;
>         ctx->bridge.type = DRM_MODE_CONNECTOR_DPI;
>         ctx->bridge.of_node = dev->of_node;
> +       ctx->bridge.pre_enable_prev_first = true;
>
>         drm_bridge_add(&ctx->bridge);
>
> --
> 2.39.1.456.gfc5497dd1b-goog
>

Re: [RFT PATCH v2 2/3] drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset

[Doug Anderson]

Hi,

On Tue, Jan 31, 2023 at 3:32 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> On 1/31/2023 2:18 PM, Douglas Anderson wrote:
> > In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> > time"), we moved powering up DSI hosts to modeset time. This wasn't
> > because it was an elegant design, but there were no better options.
> >
> > That commit actually ended up breaking ps8640, and thus was born
> > commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time for
> > parade-ps8640") as a temporary hack to un-break ps8640 by moving it to
> > the old way of doing things. It turns out that ps8640 _really_ doesn't
> > like its pre_enable() function to be called after
> > dsi_mgr_bridge_power_on(). Specifically (from experimentation, not
> > because I have any inside knowledge), it looks like the assertion of
> > "RST#" in the ps8640 runtime resume handler seems like it's not
> > allowed to happen after dsi_mgr_bridge_power_on()
> >
> > Recently, Dave Stevenson's series landed allowing bridges some control
> > over pre_enable ordering. The meaty commit for our purposes is commit
> > 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter
> > bridge init order"). As documented by that series, if a bridge doesn't
> > set "pre_enable_prev_first" then we should use the old ordering.
> >
> > Now that we have the commit ("drm/bridge: tc358762: Set
> > pre_enable_prev_first") we can go back to the old ordering, which also
> > allows us to remove the ps8640 special case.
> >
> > One last note is that even without reverting commit 7d8e9a90509f
> > ("drm/msm/dsi: move DSI host powerup to modeset time"), if you _just_
> > revert the ps8640 special case and try it out then it doesn't seem to
> > fail anymore. I spent time bisecting / debugging this and it turns out
> > to be mostly luck, so we still want this patch to make sure it's
> > solid. Specifically the reason it sorta works these days is because
> > we implemented wait_hpd_asserted() in ps8640 now, plus the magic of
> > "pm_runtime" autosuspend. The fact that we have wait_hpd_asserted()
> > implemented means that we actually power the bridge chip up just a wee
> > bit earlier and then the bridge happens to stay on because of
> > autosuspend and thus ends up powered before dsi_mgr_bridge_power_on().
> >
> > Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> > Changes in v2:
> > - Don't fold dsi_mgr_bridge_power_on() back into dsi_mgr_bridge_pre_enable()
> >
> >   drivers/gpu/drm/msm/dsi/dsi_manager.c | 38 +--------------------------
> >   1 file changed, 1 insertion(+), 37 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > index 1bbac72dad35..2197a54b9b96 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > @@ -34,32 +34,6 @@ static struct msm_dsi_manager msm_dsim_glb;
> >   #define IS_SYNC_NEEDED()    (msm_dsim_glb.is_sync_needed)
> >   #define IS_MASTER_DSI_LINK(id)      (msm_dsim_glb.master_dsi_link_id == id)
> >
> > -#ifdef CONFIG_OF
> > -static bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
> > -{
> > -     struct drm_bridge *next_bridge = drm_bridge_get_next_bridge(bridge);
> > -
> > -     /*
> > -      * If the next bridge in the chain is the Parade ps8640 bridge chip
> > -      * then don't power on early since it seems to violate the expectations
> > -      * of the firmware that the bridge chip is running.
> > -      *
> > -      * NOTE: this is expected to be a temporary special case. It's expected
> > -      * that we'll eventually have a framework that allows the next level
> > -      * bridge to indicate whether it needs us to power on before it or
> > -      * after it. When that framework is in place then we'll use it and
> > -      * remove this special case.
> > -      */
> > -     return !(next_bridge && next_bridge->of_node &&
> > -              of_device_is_compatible(next_bridge->of_node, "parade,ps8640"));
> > -}
> > -#else
> > -static inline bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
> > -{
> > -     return true;
> > -}
> > -#endif
> > -
> >   static inline struct msm_dsi *dsi_mgr_get_dsi(int id)
> >   {
> >       return msm_dsim_glb.dsi[id];
> > @@ -265,12 +239,6 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
> >       int ret;
> >
> >       DBG("id=%d", id);
> > -     if (!msm_dsi_device_connected(msm_dsi))
> > -             return;
> > -
> > -     /* Do nothing with the host if it is slave-DSI in case of bonded DSI */
> > -     if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
> > -             return;
> >
>
> Why are these two checks removed?

After this patch there is now one caller to this function and the one
caller does those exact same two checks immediately before calling
this function. Thus, they no longer do anything useful.

-Doug

Re: [RFT PATCH v2 2/3] drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset

[Doug Anderson]

Hi,

On Tue, Jan 31, 2023 at 3:32 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> On 1/31/2023 2:18 PM, Douglas Anderson wrote:
> > In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> > time"), we moved powering up DSI hosts to modeset time. This wasn't
> > because it was an elegant design, but there were no better options.
> >
> > That commit actually ended up breaking ps8640, and thus was born
> > commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time for
> > parade-ps8640") as a temporary hack to un-break ps8640 by moving it to
> > the old way of doing things. It turns out that ps8640 _really_ doesn't
> > like its pre_enable() function to be called after
> > dsi_mgr_bridge_power_on(). Specifically (from experimentation, not
> > because I have any inside knowledge), it looks like the assertion of
> > "RST#" in the ps8640 runtime resume handler seems like it's not
> > allowed to happen after dsi_mgr_bridge_power_on()
> >
> > Recently, Dave Stevenson's series landed allowing bridges some control
> > over pre_enable ordering. The meaty commit for our purposes is commit
> > 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter
> > bridge init order"). As documented by that series, if a bridge doesn't
> > set "pre_enable_prev_first" then we should use the old ordering.
> >
> > Now that we have the commit ("drm/bridge: tc358762: Set
> > pre_enable_prev_first") we can go back to the old ordering, which also
> > allows us to remove the ps8640 special case.
> >
> > One last note is that even without reverting commit 7d8e9a90509f
> > ("drm/msm/dsi: move DSI host powerup to modeset time"), if you _just_
> > revert the ps8640 special case and try it out then it doesn't seem to
> > fail anymore. I spent time bisecting / debugging this and it turns out
> > to be mostly luck, so we still want this patch to make sure it's
> > solid. Specifically the reason it sorta works these days is because
> > we implemented wait_hpd_asserted() in ps8640 now, plus the magic of
> > "pm_runtime" autosuspend. The fact that we have wait_hpd_asserted()
> > implemented means that we actually power the bridge chip up just a wee
> > bit earlier and then the bridge happens to stay on because of
> > autosuspend and thus ends up powered before dsi_mgr_bridge_power_on().
> >
> > Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> > Changes in v2:
> > - Don't fold dsi_mgr_bridge_power_on() back into dsi_mgr_bridge_pre_enable()
> >
> >   drivers/gpu/drm/msm/dsi/dsi_manager.c | 38 +--------------------------
> >   1 file changed, 1 insertion(+), 37 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > index 1bbac72dad35..2197a54b9b96 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > @@ -34,32 +34,6 @@ static struct msm_dsi_manager msm_dsim_glb;
> >   #define IS_SYNC_NEEDED()    (msm_dsim_glb.is_sync_needed)
> >   #define IS_MASTER_DSI_LINK(id)      (msm_dsim_glb.master_dsi_link_id == id)
> >
> > -#ifdef CONFIG_OF
> > -static bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
> > -{
> > -     struct drm_bridge *next_bridge = drm_bridge_get_next_bridge(bridge);
> > -
> > -     /*
> > -      * If the next bridge in the chain is the Parade ps8640 bridge chip
> > -      * then don't power on early since it seems to violate the expectations
> > -      * of the firmware that the bridge chip is running.
> > -      *
> > -      * NOTE: this is expected to be a temporary special case. It's expected
> > -      * that we'll eventually have a framework that allows the next level
> > -      * bridge to indicate whether it needs us to power on before it or
> > -      * after it. When that framework is in place then we'll use it and
> > -      * remove this special case.
> > -      */
> > -     return !(next_bridge && next_bridge->of_node &&
> > -              of_device_is_compatible(next_bridge->of_node, "parade,ps8640"));
> > -}
> > -#else
> > -static inline bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
> > -{
> > -     return true;
> > -}
> > -#endif
> > -
> >   static inline struct msm_dsi *dsi_mgr_get_dsi(int id)
> >   {
> >       return msm_dsim_glb.dsi[id];
> > @@ -265,12 +239,6 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
> >       int ret;
> >
> >       DBG("id=%d", id);
> > -     if (!msm_dsi_device_connected(msm_dsi))
> > -             return;
> > -
> > -     /* Do nothing with the host if it is slave-DSI in case of bonded DSI */
> > -     if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
> > -             return;
> >
>
> Why are these two checks removed?

After this patch there is now one caller to this function and the one
caller does those exact same two checks immediately before calling
this function. Thus, they no longer do anything useful.

-Doug

Re: [RFT PATCH v2 2/3] drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset

[Abhinav Kumar]

On 2/1/2023 6:33 AM, Doug Anderson wrote:
> Hi,
> 
> On Tue, Jan 31, 2023 at 3:32 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>> On 1/31/2023 2:18 PM, Douglas Anderson wrote:
>>> In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
>>> time"), we moved powering up DSI hosts to modeset time. This wasn't
>>> because it was an elegant design, but there were no better options.
>>>
>>> That commit actually ended up breaking ps8640, and thus was born
>>> commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time for
>>> parade-ps8640") as a temporary hack to un-break ps8640 by moving it to
>>> the old way of doing things. It turns out that ps8640 _really_ doesn't
>>> like its pre_enable() function to be called after
>>> dsi_mgr_bridge_power_on(). Specifically (from experimentation, not
>>> because I have any inside knowledge), it looks like the assertion of
>>> "RST#" in the ps8640 runtime resume handler seems like it's not
>>> allowed to happen after dsi_mgr_bridge_power_on()
>>>
>>> Recently, Dave Stevenson's series landed allowing bridges some control
>>> over pre_enable ordering. The meaty commit for our purposes is commit
>>> 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter
>>> bridge init order"). As documented by that series, if a bridge doesn't
>>> set "pre_enable_prev_first" then we should use the old ordering.
>>>
>>> Now that we have the commit ("drm/bridge: tc358762: Set
>>> pre_enable_prev_first") we can go back to the old ordering, which also
>>> allows us to remove the ps8640 special case.
>>>
>>> One last note is that even without reverting commit 7d8e9a90509f
>>> ("drm/msm/dsi: move DSI host powerup to modeset time"), if you _just_
>>> revert the ps8640 special case and try it out then it doesn't seem to
>>> fail anymore. I spent time bisecting / debugging this and it turns out
>>> to be mostly luck, so we still want this patch to make sure it's
>>> solid. Specifically the reason it sorta works these days is because
>>> we implemented wait_hpd_asserted() in ps8640 now, plus the magic of
>>> "pm_runtime" autosuspend. The fact that we have wait_hpd_asserted()
>>> implemented means that we actually power the bridge chip up just a wee
>>> bit earlier and then the bridge happens to stay on because of
>>> autosuspend and thus ends up powered before dsi_mgr_bridge_power_on().
>>>
>>> Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>> ---
>>>
>>> Changes in v2:
>>> - Don't fold dsi_mgr_bridge_power_on() back into dsi_mgr_bridge_pre_enable()
>>>
>>>    drivers/gpu/drm/msm/dsi/dsi_manager.c | 38 +--------------------------
>>>    1 file changed, 1 insertion(+), 37 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>> index 1bbac72dad35..2197a54b9b96 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>> @@ -34,32 +34,6 @@ static struct msm_dsi_manager msm_dsim_glb;
>>>    #define IS_SYNC_NEEDED()    (msm_dsim_glb.is_sync_needed)
>>>    #define IS_MASTER_DSI_LINK(id)      (msm_dsim_glb.master_dsi_link_id == id)
>>>
>>> -#ifdef CONFIG_OF
>>> -static bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
>>> -{
>>> -     struct drm_bridge *next_bridge = drm_bridge_get_next_bridge(bridge);
>>> -
>>> -     /*
>>> -      * If the next bridge in the chain is the Parade ps8640 bridge chip
>>> -      * then don't power on early since it seems to violate the expectations
>>> -      * of the firmware that the bridge chip is running.
>>> -      *
>>> -      * NOTE: this is expected to be a temporary special case. It's expected
>>> -      * that we'll eventually have a framework that allows the next level
>>> -      * bridge to indicate whether it needs us to power on before it or
>>> -      * after it. When that framework is in place then we'll use it and
>>> -      * remove this special case.
>>> -      */
>>> -     return !(next_bridge && next_bridge->of_node &&
>>> -              of_device_is_compatible(next_bridge->of_node, "parade,ps8640"));
>>> -}
>>> -#else
>>> -static inline bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
>>> -{
>>> -     return true;
>>> -}
>>> -#endif
>>> -
>>>    static inline struct msm_dsi *dsi_mgr_get_dsi(int id)
>>>    {
>>>        return msm_dsim_glb.dsi[id];
>>> @@ -265,12 +239,6 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
>>>        int ret;
>>>
>>>        DBG("id=%d", id);
>>> -     if (!msm_dsi_device_connected(msm_dsi))
>>> -             return;
>>> -
>>> -     /* Do nothing with the host if it is slave-DSI in case of bonded DSI */
>>> -     if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
>>> -             return;
>>>
>>
>> Why are these two checks removed?
> 
> After this patch there is now one caller to this function and the one
> caller does those exact same two checks immediately before calling
> this function. Thus, they no longer do anything useful.
> 
> -Doug

Ack, understood. dsi_mgr_bridge_pre_enable() has the same checks. Hence,

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

Re: [RFT PATCH v2 2/3] drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset

[Abhinav Kumar]

On 2/1/2023 6:33 AM, Doug Anderson wrote:
> Hi,
> 
> On Tue, Jan 31, 2023 at 3:32 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>> On 1/31/2023 2:18 PM, Douglas Anderson wrote:
>>> In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
>>> time"), we moved powering up DSI hosts to modeset time. This wasn't
>>> because it was an elegant design, but there were no better options.
>>>
>>> That commit actually ended up breaking ps8640, and thus was born
>>> commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time for
>>> parade-ps8640") as a temporary hack to un-break ps8640 by moving it to
>>> the old way of doing things. It turns out that ps8640 _really_ doesn't
>>> like its pre_enable() function to be called after
>>> dsi_mgr_bridge_power_on(). Specifically (from experimentation, not
>>> because I have any inside knowledge), it looks like the assertion of
>>> "RST#" in the ps8640 runtime resume handler seems like it's not
>>> allowed to happen after dsi_mgr_bridge_power_on()
>>>
>>> Recently, Dave Stevenson's series landed allowing bridges some control
>>> over pre_enable ordering. The meaty commit for our purposes is commit
>>> 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter
>>> bridge init order"). As documented by that series, if a bridge doesn't
>>> set "pre_enable_prev_first" then we should use the old ordering.
>>>
>>> Now that we have the commit ("drm/bridge: tc358762: Set
>>> pre_enable_prev_first") we can go back to the old ordering, which also
>>> allows us to remove the ps8640 special case.
>>>
>>> One last note is that even without reverting commit 7d8e9a90509f
>>> ("drm/msm/dsi: move DSI host powerup to modeset time"), if you _just_
>>> revert the ps8640 special case and try it out then it doesn't seem to
>>> fail anymore. I spent time bisecting / debugging this and it turns out
>>> to be mostly luck, so we still want this patch to make sure it's
>>> solid. Specifically the reason it sorta works these days is because
>>> we implemented wait_hpd_asserted() in ps8640 now, plus the magic of
>>> "pm_runtime" autosuspend. The fact that we have wait_hpd_asserted()
>>> implemented means that we actually power the bridge chip up just a wee
>>> bit earlier and then the bridge happens to stay on because of
>>> autosuspend and thus ends up powered before dsi_mgr_bridge_power_on().
>>>
>>> Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>> ---
>>>
>>> Changes in v2:
>>> - Don't fold dsi_mgr_bridge_power_on() back into dsi_mgr_bridge_pre_enable()
>>>
>>>    drivers/gpu/drm/msm/dsi/dsi_manager.c | 38 +--------------------------
>>>    1 file changed, 1 insertion(+), 37 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>> index 1bbac72dad35..2197a54b9b96 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>> @@ -34,32 +34,6 @@ static struct msm_dsi_manager msm_dsim_glb;
>>>    #define IS_SYNC_NEEDED()    (msm_dsim_glb.is_sync_needed)
>>>    #define IS_MASTER_DSI_LINK(id)      (msm_dsim_glb.master_dsi_link_id == id)
>>>
>>> -#ifdef CONFIG_OF
>>> -static bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
>>> -{
>>> -     struct drm_bridge *next_bridge = drm_bridge_get_next_bridge(bridge);
>>> -
>>> -     /*
>>> -      * If the next bridge in the chain is the Parade ps8640 bridge chip
>>> -      * then don't power on early since it seems to violate the expectations
>>> -      * of the firmware that the bridge chip is running.
>>> -      *
>>> -      * NOTE: this is expected to be a temporary special case. It's expected
>>> -      * that we'll eventually have a framework that allows the next level
>>> -      * bridge to indicate whether it needs us to power on before it or
>>> -      * after it. When that framework is in place then we'll use it and
>>> -      * remove this special case.
>>> -      */
>>> -     return !(next_bridge && next_bridge->of_node &&
>>> -              of_device_is_compatible(next_bridge->of_node, "parade,ps8640"));
>>> -}
>>> -#else
>>> -static inline bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
>>> -{
>>> -     return true;
>>> -}
>>> -#endif
>>> -
>>>    static inline struct msm_dsi *dsi_mgr_get_dsi(int id)
>>>    {
>>>        return msm_dsim_glb.dsi[id];
>>> @@ -265,12 +239,6 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
>>>        int ret;
>>>
>>>        DBG("id=%d", id);
>>> -     if (!msm_dsi_device_connected(msm_dsi))
>>> -             return;
>>> -
>>> -     /* Do nothing with the host if it is slave-DSI in case of bonded DSI */
>>> -     if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
>>> -             return;
>>>
>>
>> Why are these two checks removed?
> 
> After this patch there is now one caller to this function and the one
> caller does those exact same two checks immediately before calling
> this function. Thus, they no longer do anything useful.
> 
> -Doug

Ack, understood. dsi_mgr_bridge_pre_enable() has the same checks. Hence,

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

Re: [Freedreno] [RFT PATCH v2 3/3] drm/msm/dsi: More properly handle errors in regards to dsi_mgr_bridge_power_on()

[Abhinav Kumar]

Hi Doug

On 1/31/2023 2:18 PM, Douglas Anderson wrote:
> In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> time") the error handling with regards to dsi_mgr_bridge_power_on()
> got a bit worse. Specifically if we failed to power the bridge on then
> nothing would really notice. The modeset function couldn't return an
> error and thus we'd blindly go forward and try to do the pre-enable.
> 
> In commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time
> for parade-ps8640") we added a special case to move the powerup back
> to pre-enable time for ps8640. When we did that, we didn't try to
> recover the old/better error handling just for ps8640.
> 
> In the patch ("drm/msm/dsi: Stop unconditionally powering up DSI hosts
> at modeset") we've now moved the powering up back to exclusively being
> during pre-enable. That means we can add the better error handling
> back in, so let's do it. To do so we'll add a new function
> dsi_mgr_bridge_power_off() that's matches how errors were handled
> prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to
> modeset time").
> 
> NOTE: Now that we have dsi_mgr_bridge_power_off(), it feels as if we
> should be calling it in dsi_mgr_bridge_post_disable(). That would make
> some sense, but doing so would change the current behavior and thus
> should be a separate patch. Specifically:
> * dsi_mgr_bridge_post_disable() always calls dsi_mgr_phy_disable()
>    even in the slave-DSI case of bonded DSI. We'd need to add special
>    handling for this if it's truly needed.
> * dsi_mgr_bridge_post_disable() calls msm_dsi_phy_pll_save_state()
>    midway through the poweroff.
> * dsi_mgr_bridge_post_disable() has a different order of some of the
>    poweroffs / IRQ disables.
> For now we'll leave dsi_mgr_bridge_post_disable() alone.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
> Changes in v2:
> - ("More properly handle errors...") new for v2.
> 
>   drivers/gpu/drm/msm/dsi/dsi_manager.c | 32 ++++++++++++++++++++++-----
>   1 file changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> index 2197a54b9b96..28b8012a21f2 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> @@ -228,7 +228,7 @@ static void msm_dsi_manager_set_split_display(u8 id)
>   	}
>   }
>   
> -static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
> +static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
>   {
>   	int id = dsi_mgr_bridge_get_id(bridge);
>   	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
> @@ -268,14 +268,31 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
>   	if (is_bonded_dsi && msm_dsi1)
>   		msm_dsi_host_enable_irq(msm_dsi1->host);
>   
> -	return;
> +	return 0;
>   
>   host1_on_fail:
>   	msm_dsi_host_power_off(host);
>   host_on_fail:
>   	dsi_mgr_phy_disable(id);
>   phy_en_fail:
> -	return;
> +	return ret;
> +}
> +
> +static void dsi_mgr_bridge_power_off(struct drm_bridge *bridge)
> +{
> +	int id = dsi_mgr_bridge_get_id(bridge);
> +	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
> +	struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1);
> +	struct mipi_dsi_host *host = msm_dsi->host;
> +	bool is_bonded_dsi = IS_BONDED_DSI();
> +
> +	msm_dsi_host_disable_irq(host);
> +	if (is_bonded_dsi && msm_dsi1) {
> +		msm_dsi_host_disable_irq(msm_dsi1->host);
> +		msm_dsi_host_power_off(msm_dsi1->host);
> +	}

The order of disabling the IRQs should be opposite of how they were enabled.

So while enabling it was DSI0 and then DSI1.

Hence while disabling it should be DSI1 and then DSI0.

So the order here should be

DSI1 irq disable
DSI0 irq disable
DSI1 host power off
DSI0 host power off

> +	msm_dsi_host_power_off(host);
> +	dsi_mgr_phy_disable(id);
>   }
>   
>   static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
> @@ -295,7 +312,11 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
>   	if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
>   		return;
>   
> -	dsi_mgr_bridge_power_on(bridge);
> +	ret = dsi_mgr_bridge_power_on(bridge);
> +	if (ret) {
> +		dev_err(&msm_dsi->pdev->dev, "Power on failed: %d\n", ret);
> +		return;
> +	}
>   
>   	ret = msm_dsi_host_enable(host);
>   	if (ret) {
> @@ -316,8 +337,7 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
>   host1_en_fail:
>   	msm_dsi_host_disable(host);
>   host_en_fail:
> -
> -	return;
> +	dsi_mgr_bridge_power_off(bridge);
>   }
>   
>   void msm_dsi_manager_tpg_enable(void)

Re: [Freedreno] [RFT PATCH v2 3/3] drm/msm/dsi: More properly handle errors in regards to dsi_mgr_bridge_power_on()

[Abhinav Kumar]

Hi Doug

On 1/31/2023 2:18 PM, Douglas Anderson wrote:
> In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> time") the error handling with regards to dsi_mgr_bridge_power_on()
> got a bit worse. Specifically if we failed to power the bridge on then
> nothing would really notice. The modeset function couldn't return an
> error and thus we'd blindly go forward and try to do the pre-enable.
> 
> In commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time
> for parade-ps8640") we added a special case to move the powerup back
> to pre-enable time for ps8640. When we did that, we didn't try to
> recover the old/better error handling just for ps8640.
> 
> In the patch ("drm/msm/dsi: Stop unconditionally powering up DSI hosts
> at modeset") we've now moved the powering up back to exclusively being
> during pre-enable. That means we can add the better error handling
> back in, so let's do it. To do so we'll add a new function
> dsi_mgr_bridge_power_off() that's matches how errors were handled
> prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to
> modeset time").
> 
> NOTE: Now that we have dsi_mgr_bridge_power_off(), it feels as if we
> should be calling it in dsi_mgr_bridge_post_disable(). That would make
> some sense, but doing so would change the current behavior and thus
> should be a separate patch. Specifically:
> * dsi_mgr_bridge_post_disable() always calls dsi_mgr_phy_disable()
>    even in the slave-DSI case of bonded DSI. We'd need to add special
>    handling for this if it's truly needed.
> * dsi_mgr_bridge_post_disable() calls msm_dsi_phy_pll_save_state()
>    midway through the poweroff.
> * dsi_mgr_bridge_post_disable() has a different order of some of the
>    poweroffs / IRQ disables.
> For now we'll leave dsi_mgr_bridge_post_disable() alone.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
> Changes in v2:
> - ("More properly handle errors...") new for v2.
> 
>   drivers/gpu/drm/msm/dsi/dsi_manager.c | 32 ++++++++++++++++++++++-----
>   1 file changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> index 2197a54b9b96..28b8012a21f2 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> @@ -228,7 +228,7 @@ static void msm_dsi_manager_set_split_display(u8 id)
>   	}
>   }
>   
> -static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
> +static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
>   {
>   	int id = dsi_mgr_bridge_get_id(bridge);
>   	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
> @@ -268,14 +268,31 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
>   	if (is_bonded_dsi && msm_dsi1)
>   		msm_dsi_host_enable_irq(msm_dsi1->host);
>   
> -	return;
> +	return 0;
>   
>   host1_on_fail:
>   	msm_dsi_host_power_off(host);
>   host_on_fail:
>   	dsi_mgr_phy_disable(id);
>   phy_en_fail:
> -	return;
> +	return ret;
> +}
> +
> +static void dsi_mgr_bridge_power_off(struct drm_bridge *bridge)
> +{
> +	int id = dsi_mgr_bridge_get_id(bridge);
> +	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
> +	struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1);
> +	struct mipi_dsi_host *host = msm_dsi->host;
> +	bool is_bonded_dsi = IS_BONDED_DSI();
> +
> +	msm_dsi_host_disable_irq(host);
> +	if (is_bonded_dsi && msm_dsi1) {
> +		msm_dsi_host_disable_irq(msm_dsi1->host);
> +		msm_dsi_host_power_off(msm_dsi1->host);
> +	}

The order of disabling the IRQs should be opposite of how they were enabled.

So while enabling it was DSI0 and then DSI1.

Hence while disabling it should be DSI1 and then DSI0.

So the order here should be

DSI1 irq disable
DSI0 irq disable
DSI1 host power off
DSI0 host power off

> +	msm_dsi_host_power_off(host);
> +	dsi_mgr_phy_disable(id);
>   }
>   
>   static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
> @@ -295,7 +312,11 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
>   	if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
>   		return;
>   
> -	dsi_mgr_bridge_power_on(bridge);
> +	ret = dsi_mgr_bridge_power_on(bridge);
> +	if (ret) {
> +		dev_err(&msm_dsi->pdev->dev, "Power on failed: %d\n", ret);
> +		return;
> +	}
>   
>   	ret = msm_dsi_host_enable(host);
>   	if (ret) {
> @@ -316,8 +337,7 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
>   host1_en_fail:
>   	msm_dsi_host_disable(host);
>   host_en_fail:
> -
> -	return;
> +	dsi_mgr_bridge_power_off(bridge);
>   }
>   
>   void msm_dsi_manager_tpg_enable(void)

Re: [Freedreno] [RFT PATCH v2 3/3] drm/msm/dsi: More properly handle errors in regards to dsi_mgr_bridge_power_on()

[Doug Anderson]

Hi,

On Thu, Feb 2, 2023 at 2:37 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> Hi Doug
>
> On 1/31/2023 2:18 PM, Douglas Anderson wrote:
> > In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> > time") the error handling with regards to dsi_mgr_bridge_power_on()
> > got a bit worse. Specifically if we failed to power the bridge on then
> > nothing would really notice. The modeset function couldn't return an
> > error and thus we'd blindly go forward and try to do the pre-enable.
> >
> > In commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time
> > for parade-ps8640") we added a special case to move the powerup back
> > to pre-enable time for ps8640. When we did that, we didn't try to
> > recover the old/better error handling just for ps8640.
> >
> > In the patch ("drm/msm/dsi: Stop unconditionally powering up DSI hosts
> > at modeset") we've now moved the powering up back to exclusively being
> > during pre-enable. That means we can add the better error handling
> > back in, so let's do it. To do so we'll add a new function
> > dsi_mgr_bridge_power_off() that's matches how errors were handled
> > prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to
> > modeset time").
> >
> > NOTE: Now that we have dsi_mgr_bridge_power_off(), it feels as if we
> > should be calling it in dsi_mgr_bridge_post_disable(). That would make
> > some sense, but doing so would change the current behavior and thus
> > should be a separate patch. Specifically:
> > * dsi_mgr_bridge_post_disable() always calls dsi_mgr_phy_disable()
> >    even in the slave-DSI case of bonded DSI. We'd need to add special
> >    handling for this if it's truly needed.
> > * dsi_mgr_bridge_post_disable() calls msm_dsi_phy_pll_save_state()
> >    midway through the poweroff.
> > * dsi_mgr_bridge_post_disable() has a different order of some of the
> >    poweroffs / IRQ disables.
> > For now we'll leave dsi_mgr_bridge_post_disable() alone.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> > Changes in v2:
> > - ("More properly handle errors...") new for v2.
> >
> >   drivers/gpu/drm/msm/dsi/dsi_manager.c | 32 ++++++++++++++++++++++-----
> >   1 file changed, 26 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > index 2197a54b9b96..28b8012a21f2 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > @@ -228,7 +228,7 @@ static void msm_dsi_manager_set_split_display(u8 id)
> >       }
> >   }
> >
> > -static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
> > +static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
> >   {
> >       int id = dsi_mgr_bridge_get_id(bridge);
> >       struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
> > @@ -268,14 +268,31 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
> >       if (is_bonded_dsi && msm_dsi1)
> >               msm_dsi_host_enable_irq(msm_dsi1->host);
> >
> > -     return;
> > +     return 0;
> >
> >   host1_on_fail:
> >       msm_dsi_host_power_off(host);
> >   host_on_fail:
> >       dsi_mgr_phy_disable(id);
> >   phy_en_fail:
> > -     return;
> > +     return ret;
> > +}
> > +
> > +static void dsi_mgr_bridge_power_off(struct drm_bridge *bridge)
> > +{
> > +     int id = dsi_mgr_bridge_get_id(bridge);
> > +     struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
> > +     struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1);
> > +     struct mipi_dsi_host *host = msm_dsi->host;
> > +     bool is_bonded_dsi = IS_BONDED_DSI();
> > +
> > +     msm_dsi_host_disable_irq(host);
> > +     if (is_bonded_dsi && msm_dsi1) {
> > +             msm_dsi_host_disable_irq(msm_dsi1->host);
> > +             msm_dsi_host_power_off(msm_dsi1->host);
> > +     }
>
> The order of disabling the IRQs should be opposite of how they were enabled.
>
> So while enabling it was DSI0 and then DSI1.
>
> Hence while disabling it should be DSI1 and then DSI0.
>
> So the order here should be
>
> DSI1 irq disable
> DSI0 irq disable
> DSI1 host power off
> DSI0 host power off

Right. Normally you want to go opposite. I guess a few points, though:

1. As talked about in the commit message, the order I have matches the
order we had prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host
powerup to modeset time").

2. I'd be curious if it matters. The order you request means we need
to check for `(is_bonded_dsi && msm_dsi1)` twice. While that's not a
big deal if it's important, it's nice not to have to do so.

3. As talked about in the commit message, eventually we should
probably resolve this order with the order of things in
dsi_mgr_bridge_post_disable(), which is yet a different ordering.
Ideally this resolution would be done by someone who actually has
proper documentation of the hardware and how it's supposed to work
(AKA not me).

So my preference would be to either land or drop ${SUBJECT} patch
(either is fine with me) and then someone at Qualcomm could then take
over further cleanup.

-Doug

Re: [Freedreno] [RFT PATCH v2 3/3] drm/msm/dsi: More properly handle errors in regards to dsi_mgr_bridge_power_on()

[Doug Anderson]

Hi,

On Thu, Feb 2, 2023 at 2:37 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> Hi Doug
>
> On 1/31/2023 2:18 PM, Douglas Anderson wrote:
> > In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> > time") the error handling with regards to dsi_mgr_bridge_power_on()
> > got a bit worse. Specifically if we failed to power the bridge on then
> > nothing would really notice. The modeset function couldn't return an
> > error and thus we'd blindly go forward and try to do the pre-enable.
> >
> > In commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time
> > for parade-ps8640") we added a special case to move the powerup back
> > to pre-enable time for ps8640. When we did that, we didn't try to
> > recover the old/better error handling just for ps8640.
> >
> > In the patch ("drm/msm/dsi: Stop unconditionally powering up DSI hosts
> > at modeset") we've now moved the powering up back to exclusively being
> > during pre-enable. That means we can add the better error handling
> > back in, so let's do it. To do so we'll add a new function
> > dsi_mgr_bridge_power_off() that's matches how errors were handled
> > prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to
> > modeset time").
> >
> > NOTE: Now that we have dsi_mgr_bridge_power_off(), it feels as if we
> > should be calling it in dsi_mgr_bridge_post_disable(). That would make
> > some sense, but doing so would change the current behavior and thus
> > should be a separate patch. Specifically:
> > * dsi_mgr_bridge_post_disable() always calls dsi_mgr_phy_disable()
> >    even in the slave-DSI case of bonded DSI. We'd need to add special
> >    handling for this if it's truly needed.
> > * dsi_mgr_bridge_post_disable() calls msm_dsi_phy_pll_save_state()
> >    midway through the poweroff.
> > * dsi_mgr_bridge_post_disable() has a different order of some of the
> >    poweroffs / IRQ disables.
> > For now we'll leave dsi_mgr_bridge_post_disable() alone.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> > Changes in v2:
> > - ("More properly handle errors...") new for v2.
> >
> >   drivers/gpu/drm/msm/dsi/dsi_manager.c | 32 ++++++++++++++++++++++-----
> >   1 file changed, 26 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > index 2197a54b9b96..28b8012a21f2 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > @@ -228,7 +228,7 @@ static void msm_dsi_manager_set_split_display(u8 id)
> >       }
> >   }
> >
> > -static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
> > +static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
> >   {
> >       int id = dsi_mgr_bridge_get_id(bridge);
> >       struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
> > @@ -268,14 +268,31 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
> >       if (is_bonded_dsi && msm_dsi1)
> >               msm_dsi_host_enable_irq(msm_dsi1->host);
> >
> > -     return;
> > +     return 0;
> >
> >   host1_on_fail:
> >       msm_dsi_host_power_off(host);
> >   host_on_fail:
> >       dsi_mgr_phy_disable(id);
> >   phy_en_fail:
> > -     return;
> > +     return ret;
> > +}
> > +
> > +static void dsi_mgr_bridge_power_off(struct drm_bridge *bridge)
> > +{
> > +     int id = dsi_mgr_bridge_get_id(bridge);
> > +     struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
> > +     struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1);
> > +     struct mipi_dsi_host *host = msm_dsi->host;
> > +     bool is_bonded_dsi = IS_BONDED_DSI();
> > +
> > +     msm_dsi_host_disable_irq(host);
> > +     if (is_bonded_dsi && msm_dsi1) {
> > +             msm_dsi_host_disable_irq(msm_dsi1->host);
> > +             msm_dsi_host_power_off(msm_dsi1->host);
> > +     }
>
> The order of disabling the IRQs should be opposite of how they were enabled.
>
> So while enabling it was DSI0 and then DSI1.
>
> Hence while disabling it should be DSI1 and then DSI0.
>
> So the order here should be
>
> DSI1 irq disable
> DSI0 irq disable
> DSI1 host power off
> DSI0 host power off

Right. Normally you want to go opposite. I guess a few points, though:

1. As talked about in the commit message, the order I have matches the
order we had prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host
powerup to modeset time").

2. I'd be curious if it matters. The order you request means we need
to check for `(is_bonded_dsi && msm_dsi1)` twice. While that's not a
big deal if it's important, it's nice not to have to do so.

3. As talked about in the commit message, eventually we should
probably resolve this order with the order of things in
dsi_mgr_bridge_post_disable(), which is yet a different ordering.
Ideally this resolution would be done by someone who actually has
proper documentation of the hardware and how it's supposed to work
(AKA not me).

So my preference would be to either land or drop ${SUBJECT} patch
(either is fine with me) and then someone at Qualcomm could then take
over further cleanup.

-Doug

Re: [Freedreno] [RFT PATCH v2 3/3] drm/msm/dsi: More properly handle errors in regards to dsi_mgr_bridge_power_on()

[Abhinav Kumar]

Hi Doug

Sorry for the delayed response.

On 2/2/2023 2:46 PM, Doug Anderson wrote:
> Hi,
> 
> On Thu, Feb 2, 2023 at 2:37 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>> Hi Doug
>>
>> On 1/31/2023 2:18 PM, Douglas Anderson wrote:
>>> In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
>>> time") the error handling with regards to dsi_mgr_bridge_power_on()
>>> got a bit worse. Specifically if we failed to power the bridge on then
>>> nothing would really notice. The modeset function couldn't return an
>>> error and thus we'd blindly go forward and try to do the pre-enable.
>>>
>>> In commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time
>>> for parade-ps8640") we added a special case to move the powerup back
>>> to pre-enable time for ps8640. When we did that, we didn't try to
>>> recover the old/better error handling just for ps8640.
>>>
>>> In the patch ("drm/msm/dsi: Stop unconditionally powering up DSI hosts
>>> at modeset") we've now moved the powering up back to exclusively being
>>> during pre-enable. That means we can add the better error handling
>>> back in, so let's do it. To do so we'll add a new function
>>> dsi_mgr_bridge_power_off() that's matches how errors were handled
>>> prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to
>>> modeset time").
>>>
>>> NOTE: Now that we have dsi_mgr_bridge_power_off(), it feels as if we
>>> should be calling it in dsi_mgr_bridge_post_disable(). That would make
>>> some sense, but doing so would change the current behavior and thus
>>> should be a separate patch. Specifically:
>>> * dsi_mgr_bridge_post_disable() always calls dsi_mgr_phy_disable()
>>>     even in the slave-DSI case of bonded DSI. We'd need to add special
>>>     handling for this if it's truly needed.
>>> * dsi_mgr_bridge_post_disable() calls msm_dsi_phy_pll_save_state()
>>>     midway through the poweroff.
>>> * dsi_mgr_bridge_post_disable() has a different order of some of the
>>>     poweroffs / IRQ disables.
>>> For now we'll leave dsi_mgr_bridge_post_disable() alone.
>>>
>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>> ---
>>>
>>> Changes in v2:
>>> - ("More properly handle errors...") new for v2.
>>>
>>>    drivers/gpu/drm/msm/dsi/dsi_manager.c | 32 ++++++++++++++++++++++-----
>>>    1 file changed, 26 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>> index 2197a54b9b96..28b8012a21f2 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>> @@ -228,7 +228,7 @@ static void msm_dsi_manager_set_split_display(u8 id)
>>>        }
>>>    }
>>>
>>> -static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
>>> +static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
>>>    {
>>>        int id = dsi_mgr_bridge_get_id(bridge);
>>>        struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
>>> @@ -268,14 +268,31 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
>>>        if (is_bonded_dsi && msm_dsi1)
>>>                msm_dsi_host_enable_irq(msm_dsi1->host);
>>>
>>> -     return;
>>> +     return 0;
>>>
>>>    host1_on_fail:
>>>        msm_dsi_host_power_off(host);
>>>    host_on_fail:
>>>        dsi_mgr_phy_disable(id);
>>>    phy_en_fail:
>>> -     return;
>>> +     return ret;
>>> +}
>>> +
>>> +static void dsi_mgr_bridge_power_off(struct drm_bridge *bridge)
>>> +{
>>> +     int id = dsi_mgr_bridge_get_id(bridge);
>>> +     struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
>>> +     struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1);
>>> +     struct mipi_dsi_host *host = msm_dsi->host;
>>> +     bool is_bonded_dsi = IS_BONDED_DSI();
>>> +
>>> +     msm_dsi_host_disable_irq(host);
>>> +     if (is_bonded_dsi && msm_dsi1) {
>>> +             msm_dsi_host_disable_irq(msm_dsi1->host);
>>> +             msm_dsi_host_power_off(msm_dsi1->host);
>>> +     }
>>
>> The order of disabling the IRQs should be opposite of how they were enabled.
>>
>> So while enabling it was DSI0 and then DSI1.
>>
>> Hence while disabling it should be DSI1 and then DSI0.
>>
>> So the order here should be
>>
>> DSI1 irq disable
>> DSI0 irq disable
>> DSI1 host power off
>> DSI0 host power off
> 
> Right. Normally you want to go opposite. I guess a few points, though:
> 
> 1. As talked about in the commit message, the order I have matches the
> order we had prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host
> powerup to modeset time").
> 
> 2. I'd be curious if it matters. The order you request means we need
> to check for `(is_bonded_dsi && msm_dsi1)` twice. While that's not a
> big deal if it's important, it's nice not to have to do so.
> 
> 3. As talked about in the commit message, eventually we should
> probably resolve this order with the order of things in
> dsi_mgr_bridge_post_disable(), which is yet a different ordering.
> Ideally this resolution would be done by someone who actually has
> proper documentation of the hardware and how it's supposed to work
> (AKA not me).
> 
> So my preference would be to either land or drop ${SUBJECT} patch
> (either is fine with me) and then someone at Qualcomm could then take
> over further cleanup.
> 

I do think the ordering matters but you are right, this change brings 
back the ordering we had before so lets handle the re-ordering of all 
places in a separate change. I am okay with this change to go-in, hence

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

What is the plan to land the patches?

2 & 3 go in msm-next but 1 goes in drm-misc?

Thanks

Abhinav


> -Doug

Re: [Freedreno] [RFT PATCH v2 3/3] drm/msm/dsi: More properly handle errors in regards to dsi_mgr_bridge_power_on()

[Abhinav Kumar]

Hi Doug

Sorry for the delayed response.

On 2/2/2023 2:46 PM, Doug Anderson wrote:
> Hi,
> 
> On Thu, Feb 2, 2023 at 2:37 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>> Hi Doug
>>
>> On 1/31/2023 2:18 PM, Douglas Anderson wrote:
>>> In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
>>> time") the error handling with regards to dsi_mgr_bridge_power_on()
>>> got a bit worse. Specifically if we failed to power the bridge on then
>>> nothing would really notice. The modeset function couldn't return an
>>> error and thus we'd blindly go forward and try to do the pre-enable.
>>>
>>> In commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time
>>> for parade-ps8640") we added a special case to move the powerup back
>>> to pre-enable time for ps8640. When we did that, we didn't try to
>>> recover the old/better error handling just for ps8640.
>>>
>>> In the patch ("drm/msm/dsi: Stop unconditionally powering up DSI hosts
>>> at modeset") we've now moved the powering up back to exclusively being
>>> during pre-enable. That means we can add the better error handling
>>> back in, so let's do it. To do so we'll add a new function
>>> dsi_mgr_bridge_power_off() that's matches how errors were handled
>>> prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to
>>> modeset time").
>>>
>>> NOTE: Now that we have dsi_mgr_bridge_power_off(), it feels as if we
>>> should be calling it in dsi_mgr_bridge_post_disable(). That would make
>>> some sense, but doing so would change the current behavior and thus
>>> should be a separate patch. Specifically:
>>> * dsi_mgr_bridge_post_disable() always calls dsi_mgr_phy_disable()
>>>     even in the slave-DSI case of bonded DSI. We'd need to add special
>>>     handling for this if it's truly needed.
>>> * dsi_mgr_bridge_post_disable() calls msm_dsi_phy_pll_save_state()
>>>     midway through the poweroff.
>>> * dsi_mgr_bridge_post_disable() has a different order of some of the
>>>     poweroffs / IRQ disables.
>>> For now we'll leave dsi_mgr_bridge_post_disable() alone.
>>>
>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>> ---
>>>
>>> Changes in v2:
>>> - ("More properly handle errors...") new for v2.
>>>
>>>    drivers/gpu/drm/msm/dsi/dsi_manager.c | 32 ++++++++++++++++++++++-----
>>>    1 file changed, 26 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>> index 2197a54b9b96..28b8012a21f2 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>> @@ -228,7 +228,7 @@ static void msm_dsi_manager_set_split_display(u8 id)
>>>        }
>>>    }
>>>
>>> -static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
>>> +static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
>>>    {
>>>        int id = dsi_mgr_bridge_get_id(bridge);
>>>        struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
>>> @@ -268,14 +268,31 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
>>>        if (is_bonded_dsi && msm_dsi1)
>>>                msm_dsi_host_enable_irq(msm_dsi1->host);
>>>
>>> -     return;
>>> +     return 0;
>>>
>>>    host1_on_fail:
>>>        msm_dsi_host_power_off(host);
>>>    host_on_fail:
>>>        dsi_mgr_phy_disable(id);
>>>    phy_en_fail:
>>> -     return;
>>> +     return ret;
>>> +}
>>> +
>>> +static void dsi_mgr_bridge_power_off(struct drm_bridge *bridge)
>>> +{
>>> +     int id = dsi_mgr_bridge_get_id(bridge);
>>> +     struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
>>> +     struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1);
>>> +     struct mipi_dsi_host *host = msm_dsi->host;
>>> +     bool is_bonded_dsi = IS_BONDED_DSI();
>>> +
>>> +     msm_dsi_host_disable_irq(host);
>>> +     if (is_bonded_dsi && msm_dsi1) {
>>> +             msm_dsi_host_disable_irq(msm_dsi1->host);
>>> +             msm_dsi_host_power_off(msm_dsi1->host);
>>> +     }
>>
>> The order of disabling the IRQs should be opposite of how they were enabled.
>>
>> So while enabling it was DSI0 and then DSI1.
>>
>> Hence while disabling it should be DSI1 and then DSI0.
>>
>> So the order here should be
>>
>> DSI1 irq disable
>> DSI0 irq disable
>> DSI1 host power off
>> DSI0 host power off
> 
> Right. Normally you want to go opposite. I guess a few points, though:
> 
> 1. As talked about in the commit message, the order I have matches the
> order we had prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host
> powerup to modeset time").
> 
> 2. I'd be curious if it matters. The order you request means we need
> to check for `(is_bonded_dsi && msm_dsi1)` twice. While that's not a
> big deal if it's important, it's nice not to have to do so.
> 
> 3. As talked about in the commit message, eventually we should
> probably resolve this order with the order of things in
> dsi_mgr_bridge_post_disable(), which is yet a different ordering.
> Ideally this resolution would be done by someone who actually has
> proper documentation of the hardware and how it's supposed to work
> (AKA not me).
> 
> So my preference would be to either land or drop ${SUBJECT} patch
> (either is fine with me) and then someone at Qualcomm could then take
> over further cleanup.
> 

I do think the ordering matters but you are right, this change brings 
back the ordering we had before so lets handle the re-ordering of all 
places in a separate change. I am okay with this change to go-in, hence

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

What is the plan to land the patches?

2 & 3 go in msm-next but 1 goes in drm-misc?

Thanks

Abhinav


> -Doug

Re: [Freedreno] [RFT PATCH v2 3/3] drm/msm/dsi: More properly handle errors in regards to dsi_mgr_bridge_power_on()

[Doug Anderson]

Hi,

On Mon, Feb 13, 2023 at 6:02 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> Hi Doug
>
> Sorry for the delayed response.
>
> On 2/2/2023 2:46 PM, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Feb 2, 2023 at 2:37 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >> Hi Doug
> >>
> >> On 1/31/2023 2:18 PM, Douglas Anderson wrote:
> >>> In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> >>> time") the error handling with regards to dsi_mgr_bridge_power_on()
> >>> got a bit worse. Specifically if we failed to power the bridge on then
> >>> nothing would really notice. The modeset function couldn't return an
> >>> error and thus we'd blindly go forward and try to do the pre-enable.
> >>>
> >>> In commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time
> >>> for parade-ps8640") we added a special case to move the powerup back
> >>> to pre-enable time for ps8640. When we did that, we didn't try to
> >>> recover the old/better error handling just for ps8640.
> >>>
> >>> In the patch ("drm/msm/dsi: Stop unconditionally powering up DSI hosts
> >>> at modeset") we've now moved the powering up back to exclusively being
> >>> during pre-enable. That means we can add the better error handling
> >>> back in, so let's do it. To do so we'll add a new function
> >>> dsi_mgr_bridge_power_off() that's matches how errors were handled
> >>> prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to
> >>> modeset time").
> >>>
> >>> NOTE: Now that we have dsi_mgr_bridge_power_off(), it feels as if we
> >>> should be calling it in dsi_mgr_bridge_post_disable(). That would make
> >>> some sense, but doing so would change the current behavior and thus
> >>> should be a separate patch. Specifically:
> >>> * dsi_mgr_bridge_post_disable() always calls dsi_mgr_phy_disable()
> >>>     even in the slave-DSI case of bonded DSI. We'd need to add special
> >>>     handling for this if it's truly needed.
> >>> * dsi_mgr_bridge_post_disable() calls msm_dsi_phy_pll_save_state()
> >>>     midway through the poweroff.
> >>> * dsi_mgr_bridge_post_disable() has a different order of some of the
> >>>     poweroffs / IRQ disables.
> >>> For now we'll leave dsi_mgr_bridge_post_disable() alone.
> >>>
> >>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >>> ---
> >>>
> >>> Changes in v2:
> >>> - ("More properly handle errors...") new for v2.
> >>>
> >>>    drivers/gpu/drm/msm/dsi/dsi_manager.c | 32 ++++++++++++++++++++++-----
> >>>    1 file changed, 26 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> >>> index 2197a54b9b96..28b8012a21f2 100644
> >>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> >>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> >>> @@ -228,7 +228,7 @@ static void msm_dsi_manager_set_split_display(u8 id)
> >>>        }
> >>>    }
> >>>
> >>> -static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
> >>> +static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
> >>>    {
> >>>        int id = dsi_mgr_bridge_get_id(bridge);
> >>>        struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
> >>> @@ -268,14 +268,31 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
> >>>        if (is_bonded_dsi && msm_dsi1)
> >>>                msm_dsi_host_enable_irq(msm_dsi1->host);
> >>>
> >>> -     return;
> >>> +     return 0;
> >>>
> >>>    host1_on_fail:
> >>>        msm_dsi_host_power_off(host);
> >>>    host_on_fail:
> >>>        dsi_mgr_phy_disable(id);
> >>>    phy_en_fail:
> >>> -     return;
> >>> +     return ret;
> >>> +}
> >>> +
> >>> +static void dsi_mgr_bridge_power_off(struct drm_bridge *bridge)
> >>> +{
> >>> +     int id = dsi_mgr_bridge_get_id(bridge);
> >>> +     struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
> >>> +     struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1);
> >>> +     struct mipi_dsi_host *host = msm_dsi->host;
> >>> +     bool is_bonded_dsi = IS_BONDED_DSI();
> >>> +
> >>> +     msm_dsi_host_disable_irq(host);
> >>> +     if (is_bonded_dsi && msm_dsi1) {
> >>> +             msm_dsi_host_disable_irq(msm_dsi1->host);
> >>> +             msm_dsi_host_power_off(msm_dsi1->host);
> >>> +     }
> >>
> >> The order of disabling the IRQs should be opposite of how they were enabled.
> >>
> >> So while enabling it was DSI0 and then DSI1.
> >>
> >> Hence while disabling it should be DSI1 and then DSI0.
> >>
> >> So the order here should be
> >>
> >> DSI1 irq disable
> >> DSI0 irq disable
> >> DSI1 host power off
> >> DSI0 host power off
> >
> > Right. Normally you want to go opposite. I guess a few points, though:
> >
> > 1. As talked about in the commit message, the order I have matches the
> > order we had prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host
> > powerup to modeset time").
> >
> > 2. I'd be curious if it matters. The order you request means we need
> > to check for `(is_bonded_dsi && msm_dsi1)` twice. While that's not a
> > big deal if it's important, it's nice not to have to do so.
> >
> > 3. As talked about in the commit message, eventually we should
> > probably resolve this order with the order of things in
> > dsi_mgr_bridge_post_disable(), which is yet a different ordering.
> > Ideally this resolution would be done by someone who actually has
> > proper documentation of the hardware and how it's supposed to work
> > (AKA not me).
> >
> > So my preference would be to either land or drop ${SUBJECT} patch
> > (either is fine with me) and then someone at Qualcomm could then take
> > over further cleanup.
> >
>
> I do think the ordering matters but you are right, this change brings
> back the ordering we had before so lets handle the re-ordering of all
> places in a separate change. I am okay with this change to go-in, hence
>
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>
> What is the plan to land the patches?
>
> 2 & 3 go in msm-next but 1 goes in drm-misc?

We can do that and I'm happy to land patch #1 in drm-misc. Then I
assume we'd want to wait until the change makes its way into mainline
before landing patch #2/#3?

Given how tiny patch #1 is, though, it sure seems like it would be
nice / easier if they all went through the msm tree. I guess we'd want
one of the drm-misc maintainers (not just a committer like me) to Ack
this? Maybe it's not worth it and we should just go the slow route?

-Doug

Re: [Freedreno] [RFT PATCH v2 3/3] drm/msm/dsi: More properly handle errors in regards to dsi_mgr_bridge_power_on()

[Doug Anderson]

Hi,

On Mon, Feb 13, 2023 at 6:02 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> Hi Doug
>
> Sorry for the delayed response.
>
> On 2/2/2023 2:46 PM, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Feb 2, 2023 at 2:37 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >> Hi Doug
> >>
> >> On 1/31/2023 2:18 PM, Douglas Anderson wrote:
> >>> In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> >>> time") the error handling with regards to dsi_mgr_bridge_power_on()
> >>> got a bit worse. Specifically if we failed to power the bridge on then
> >>> nothing would really notice. The modeset function couldn't return an
> >>> error and thus we'd blindly go forward and try to do the pre-enable.
> >>>
> >>> In commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time
> >>> for parade-ps8640") we added a special case to move the powerup back
> >>> to pre-enable time for ps8640. When we did that, we didn't try to
> >>> recover the old/better error handling just for ps8640.
> >>>
> >>> In the patch ("drm/msm/dsi: Stop unconditionally powering up DSI hosts
> >>> at modeset") we've now moved the powering up back to exclusively being
> >>> during pre-enable. That means we can add the better error handling
> >>> back in, so let's do it. To do so we'll add a new function
> >>> dsi_mgr_bridge_power_off() that's matches how errors were handled
> >>> prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to
> >>> modeset time").
> >>>
> >>> NOTE: Now that we have dsi_mgr_bridge_power_off(), it feels as if we
> >>> should be calling it in dsi_mgr_bridge_post_disable(). That would make
> >>> some sense, but doing so would change the current behavior and thus
> >>> should be a separate patch. Specifically:
> >>> * dsi_mgr_bridge_post_disable() always calls dsi_mgr_phy_disable()
> >>>     even in the slave-DSI case of bonded DSI. We'd need to add special
> >>>     handling for this if it's truly needed.
> >>> * dsi_mgr_bridge_post_disable() calls msm_dsi_phy_pll_save_state()
> >>>     midway through the poweroff.
> >>> * dsi_mgr_bridge_post_disable() has a different order of some of the
> >>>     poweroffs / IRQ disables.
> >>> For now we'll leave dsi_mgr_bridge_post_disable() alone.
> >>>
> >>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >>> ---
> >>>
> >>> Changes in v2:
> >>> - ("More properly handle errors...") new for v2.
> >>>
> >>>    drivers/gpu/drm/msm/dsi/dsi_manager.c | 32 ++++++++++++++++++++++-----
> >>>    1 file changed, 26 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> >>> index 2197a54b9b96..28b8012a21f2 100644
> >>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> >>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> >>> @@ -228,7 +228,7 @@ static void msm_dsi_manager_set_split_display(u8 id)
> >>>        }
> >>>    }
> >>>
> >>> -static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
> >>> +static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
> >>>    {
> >>>        int id = dsi_mgr_bridge_get_id(bridge);
> >>>        struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
> >>> @@ -268,14 +268,31 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
> >>>        if (is_bonded_dsi && msm_dsi1)
> >>>                msm_dsi_host_enable_irq(msm_dsi1->host);
> >>>
> >>> -     return;
> >>> +     return 0;
> >>>
> >>>    host1_on_fail:
> >>>        msm_dsi_host_power_off(host);
> >>>    host_on_fail:
> >>>        dsi_mgr_phy_disable(id);
> >>>    phy_en_fail:
> >>> -     return;
> >>> +     return ret;
> >>> +}
> >>> +
> >>> +static void dsi_mgr_bridge_power_off(struct drm_bridge *bridge)
> >>> +{
> >>> +     int id = dsi_mgr_bridge_get_id(bridge);
> >>> +     struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
> >>> +     struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1);
> >>> +     struct mipi_dsi_host *host = msm_dsi->host;
> >>> +     bool is_bonded_dsi = IS_BONDED_DSI();
> >>> +
> >>> +     msm_dsi_host_disable_irq(host);
> >>> +     if (is_bonded_dsi && msm_dsi1) {
> >>> +             msm_dsi_host_disable_irq(msm_dsi1->host);
> >>> +             msm_dsi_host_power_off(msm_dsi1->host);
> >>> +     }
> >>
> >> The order of disabling the IRQs should be opposite of how they were enabled.
> >>
> >> So while enabling it was DSI0 and then DSI1.
> >>
> >> Hence while disabling it should be DSI1 and then DSI0.
> >>
> >> So the order here should be
> >>
> >> DSI1 irq disable
> >> DSI0 irq disable
> >> DSI1 host power off
> >> DSI0 host power off
> >
> > Right. Normally you want to go opposite. I guess a few points, though:
> >
> > 1. As talked about in the commit message, the order I have matches the
> > order we had prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host
> > powerup to modeset time").
> >
> > 2. I'd be curious if it matters. The order you request means we need
> > to check for `(is_bonded_dsi && msm_dsi1)` twice. While that's not a
> > big deal if it's important, it's nice not to have to do so.
> >
> > 3. As talked about in the commit message, eventually we should
> > probably resolve this order with the order of things in
> > dsi_mgr_bridge_post_disable(), which is yet a different ordering.
> > Ideally this resolution would be done by someone who actually has
> > proper documentation of the hardware and how it's supposed to work
> > (AKA not me).
> >
> > So my preference would be to either land or drop ${SUBJECT} patch
> > (either is fine with me) and then someone at Qualcomm could then take
> > over further cleanup.
> >
>
> I do think the ordering matters but you are right, this change brings
> back the ordering we had before so lets handle the re-ordering of all
> places in a separate change. I am okay with this change to go-in, hence
>
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>
> What is the plan to land the patches?
>
> 2 & 3 go in msm-next but 1 goes in drm-misc?

We can do that and I'm happy to land patch #1 in drm-misc. Then I
assume we'd want to wait until the change makes its way into mainline
before landing patch #2/#3?

Given how tiny patch #1 is, though, it sure seems like it would be
nice / easier if they all went through the msm tree. I guess we'd want
one of the drm-misc maintainers (not just a committer like me) to Ack
this? Maybe it's not worth it and we should just go the slow route?

-Doug

Re: [RFT PATCH v2 1/3] drm/bridge: tc358762: Set pre_enable_prev_first

[Doug Anderson]

Hi,

On Wed, Feb 1, 2023 at 1:51 AM Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> On Tue, 31 Jan 2023 at 22:22, Douglas Anderson <dianders@chromium.org> wrote:
> >
> > Set the "pre_enable_prev_first" as provided by commit 4fb912e5e190
> > ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init
> > order"). This should allow us to revert commit ec7981e6c614
> > ("drm/msm/dsi: don't powerup at modeset time for parade-ps8640") and
> > commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> > time").
>
> I see no reference in the TC358762 datasheet to requiring the DSI
> interface to be in any particular state.
> However, setting this flag does mean that the DSI host doesn't need to
> power up and down for each host_transfer request from
> tc358762_pre_enable/tc358762_init, so on that basis I'm good with it.
>
> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>
> > Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> > (no changes since v1)
> >
> >  drivers/gpu/drm/bridge/tc358762.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/tc358762.c b/drivers/gpu/drm/bridge/tc358762.c
> > index 0b6a28436885..77f7f7f54757 100644
> > --- a/drivers/gpu/drm/bridge/tc358762.c
> > +++ b/drivers/gpu/drm/bridge/tc358762.c
> > @@ -229,6 +229,7 @@ static int tc358762_probe(struct mipi_dsi_device *dsi)
> >         ctx->bridge.funcs = &tc358762_bridge_funcs;
> >         ctx->bridge.type = DRM_MODE_CONNECTOR_DPI;
> >         ctx->bridge.of_node = dev->of_node;
> > +       ctx->bridge.pre_enable_prev_first = true;
> >
> >         drm_bridge_add(&ctx->bridge);

Abhinav asked what the plan was for landing this [1]. Since this isn't
urgent, I guess the plan is to land patch #1 in drm-misc-next. Then we
sit and wait until it percolates into mainline and, once it does, then
patch #2 and #3 can land.

Since I have Dave's review I can commit this to drm-misc-next myself.
My plan will be to wait until Thursday or Friday of this week (to give
people a bit of time to object) and then land patch #1. Then I'll
snooze things for a while and poke Abhinav and Dmitry to land patch #2
/ #3 when I notice it in mainline. If, at any point, someone comes out
of the woodwork and yells that this is breaking them then, worst case,
we can revert.

[1] https://lore.kernel.org/r/1f204585-88e2-abae-1216-92f739ac9e91@quicinc.com/

Re: [RFT PATCH v2 1/3] drm/bridge: tc358762: Set pre_enable_prev_first

[Doug Anderson]

Hi,

On Wed, Feb 1, 2023 at 1:51 AM Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> On Tue, 31 Jan 2023 at 22:22, Douglas Anderson <dianders@chromium.org> wrote:
> >
> > Set the "pre_enable_prev_first" as provided by commit 4fb912e5e190
> > ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init
> > order"). This should allow us to revert commit ec7981e6c614
> > ("drm/msm/dsi: don't powerup at modeset time for parade-ps8640") and
> > commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> > time").
>
> I see no reference in the TC358762 datasheet to requiring the DSI
> interface to be in any particular state.
> However, setting this flag does mean that the DSI host doesn't need to
> power up and down for each host_transfer request from
> tc358762_pre_enable/tc358762_init, so on that basis I'm good with it.
>
> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>
> > Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> > (no changes since v1)
> >
> >  drivers/gpu/drm/bridge/tc358762.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/tc358762.c b/drivers/gpu/drm/bridge/tc358762.c
> > index 0b6a28436885..77f7f7f54757 100644
> > --- a/drivers/gpu/drm/bridge/tc358762.c
> > +++ b/drivers/gpu/drm/bridge/tc358762.c
> > @@ -229,6 +229,7 @@ static int tc358762_probe(struct mipi_dsi_device *dsi)
> >         ctx->bridge.funcs = &tc358762_bridge_funcs;
> >         ctx->bridge.type = DRM_MODE_CONNECTOR_DPI;
> >         ctx->bridge.of_node = dev->of_node;
> > +       ctx->bridge.pre_enable_prev_first = true;
> >
> >         drm_bridge_add(&ctx->bridge);

Abhinav asked what the plan was for landing this [1]. Since this isn't
urgent, I guess the plan is to land patch #1 in drm-misc-next. Then we
sit and wait until it percolates into mainline and, once it does, then
patch #2 and #3 can land.

Since I have Dave's review I can commit this to drm-misc-next myself.
My plan will be to wait until Thursday or Friday of this week (to give
people a bit of time to object) and then land patch #1. Then I'll
snooze things for a while and poke Abhinav and Dmitry to land patch #2
/ #3 when I notice it in mainline. If, at any point, someone comes out
of the woodwork and yells that this is breaking them then, worst case,
we can revert.

[1] https://lore.kernel.org/r/1f204585-88e2-abae-1216-92f739ac9e91@quicinc.com/

Re: [RFT PATCH v2 1/3] drm/bridge: tc358762: Set pre_enable_prev_first

[Dmitry Baryshkov]

On 28/02/2023 02:26, Doug Anderson wrote:
> Hi,
> 
> On Wed, Feb 1, 2023 at 1:51 AM Dave Stevenson
> <dave.stevenson@raspberrypi.com> wrote:
>>
>> On Tue, 31 Jan 2023 at 22:22, Douglas Anderson <dianders@chromium.org> wrote:
>>>
>>> Set the "pre_enable_prev_first" as provided by commit 4fb912e5e190
>>> ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init
>>> order"). This should allow us to revert commit ec7981e6c614
>>> ("drm/msm/dsi: don't powerup at modeset time for parade-ps8640") and
>>> commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
>>> time").
>>
>> I see no reference in the TC358762 datasheet to requiring the DSI
>> interface to be in any particular state.
>> However, setting this flag does mean that the DSI host doesn't need to
>> power up and down for each host_transfer request from
>> tc358762_pre_enable/tc358762_init, so on that basis I'm good with it.
>>
>> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>
>>> Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>> ---
>>>
>>> (no changes since v1)
>>>
>>>   drivers/gpu/drm/bridge/tc358762.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/tc358762.c b/drivers/gpu/drm/bridge/tc358762.c
>>> index 0b6a28436885..77f7f7f54757 100644
>>> --- a/drivers/gpu/drm/bridge/tc358762.c
>>> +++ b/drivers/gpu/drm/bridge/tc358762.c
>>> @@ -229,6 +229,7 @@ static int tc358762_probe(struct mipi_dsi_device *dsi)
>>>          ctx->bridge.funcs = &tc358762_bridge_funcs;
>>>          ctx->bridge.type = DRM_MODE_CONNECTOR_DPI;
>>>          ctx->bridge.of_node = dev->of_node;
>>> +       ctx->bridge.pre_enable_prev_first = true;
>>>
>>>          drm_bridge_add(&ctx->bridge);
> 
> Abhinav asked what the plan was for landing this [1]. Since this isn't
> urgent, I guess the plan is to land patch #1 in drm-misc-next. Then we
> sit and wait until it percolates into mainline and, once it does, then
> patch #2 and #3 can land.
> 
> Since I have Dave's review I can commit this to drm-misc-next myself.
> My plan will be to wait until Thursday or Friday of this week (to give
> people a bit of time to object) and then land patch #1. Then I'll
> snooze things for a while and poke Abhinav and Dmitry to land patch #2
> / #3 when I notice it in mainline. If, at any point, someone comes out
> of the woodwork and yells that this is breaking them then, worst case,
> we can revert.

This plan sounds good to me.

> 
> [1] https://lore.kernel.org/r/1f204585-88e2-abae-1216-92f739ac9e91@quicinc.com/

-- 
With best wishes
Dmitry

Re: [RFT PATCH v2 1/3] drm/bridge: tc358762: Set pre_enable_prev_first

[Dmitry Baryshkov]

On 28/02/2023 02:26, Doug Anderson wrote:
> Hi,
> 
> On Wed, Feb 1, 2023 at 1:51 AM Dave Stevenson
> <dave.stevenson@raspberrypi.com> wrote:
>>
>> On Tue, 31 Jan 2023 at 22:22, Douglas Anderson <dianders@chromium.org> wrote:
>>>
>>> Set the "pre_enable_prev_first" as provided by commit 4fb912e5e190
>>> ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init
>>> order"). This should allow us to revert commit ec7981e6c614
>>> ("drm/msm/dsi: don't powerup at modeset time for parade-ps8640") and
>>> commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
>>> time").
>>
>> I see no reference in the TC358762 datasheet to requiring the DSI
>> interface to be in any particular state.
>> However, setting this flag does mean that the DSI host doesn't need to
>> power up and down for each host_transfer request from
>> tc358762_pre_enable/tc358762_init, so on that basis I'm good with it.
>>
>> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>
>>> Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>> ---
>>>
>>> (no changes since v1)
>>>
>>>   drivers/gpu/drm/bridge/tc358762.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/tc358762.c b/drivers/gpu/drm/bridge/tc358762.c
>>> index 0b6a28436885..77f7f7f54757 100644
>>> --- a/drivers/gpu/drm/bridge/tc358762.c
>>> +++ b/drivers/gpu/drm/bridge/tc358762.c
>>> @@ -229,6 +229,7 @@ static int tc358762_probe(struct mipi_dsi_device *dsi)
>>>          ctx->bridge.funcs = &tc358762_bridge_funcs;
>>>          ctx->bridge.type = DRM_MODE_CONNECTOR_DPI;
>>>          ctx->bridge.of_node = dev->of_node;
>>> +       ctx->bridge.pre_enable_prev_first = true;
>>>
>>>          drm_bridge_add(&ctx->bridge);
> 
> Abhinav asked what the plan was for landing this [1]. Since this isn't
> urgent, I guess the plan is to land patch #1 in drm-misc-next. Then we
> sit and wait until it percolates into mainline and, once it does, then
> patch #2 and #3 can land.
> 
> Since I have Dave's review I can commit this to drm-misc-next myself.
> My plan will be to wait until Thursday or Friday of this week (to give
> people a bit of time to object) and then land patch #1. Then I'll
> snooze things for a while and poke Abhinav and Dmitry to land patch #2
> / #3 when I notice it in mainline. If, at any point, someone comes out
> of the woodwork and yells that this is breaking them then, worst case,
> we can revert.

This plan sounds good to me.

> 
> [1] https://lore.kernel.org/r/1f204585-88e2-abae-1216-92f739ac9e91@quicinc.com/

-- 
With best wishes
Dmitry

Re: [RFT PATCH v2 1/3] drm/bridge: tc358762: Set pre_enable_prev_first

[Dmitry Baryshkov]

On 01/02/2023 00:18, Douglas Anderson wrote:
> Set the "pre_enable_prev_first" as provided by commit 4fb912e5e190
> ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init
> order"). This should allow us to revert commit ec7981e6c614
> ("drm/msm/dsi: don't powerup at modeset time for parade-ps8640") and
> commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> time").
> 
> Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

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

-- 
With best wishes
Dmitry

Re: [RFT PATCH v2 1/3] drm/bridge: tc358762: Set pre_enable_prev_first

[Dmitry Baryshkov]

On 01/02/2023 00:18, Douglas Anderson wrote:
> Set the "pre_enable_prev_first" as provided by commit 4fb912e5e190
> ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init
> order"). This should allow us to revert commit ec7981e6c614
> ("drm/msm/dsi: don't powerup at modeset time for parade-ps8640") and
> commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> time").
> 
> Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

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

-- 
With best wishes
Dmitry

Re: [RFT PATCH v2 2/3] drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset

[Dmitry Baryshkov]

On 01/02/2023 00:18, Douglas Anderson wrote:
> In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> time"), we moved powering up DSI hosts to modeset time. This wasn't
> because it was an elegant design, but there were no better options.
> 
> That commit actually ended up breaking ps8640, and thus was born
> commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time for
> parade-ps8640") as a temporary hack to un-break ps8640 by moving it to
> the old way of doing things. It turns out that ps8640 _really_ doesn't
> like its pre_enable() function to be called after
> dsi_mgr_bridge_power_on(). Specifically (from experimentation, not
> because I have any inside knowledge), it looks like the assertion of
> "RST#" in the ps8640 runtime resume handler seems like it's not
> allowed to happen after dsi_mgr_bridge_power_on()
> 
> Recently, Dave Stevenson's series landed allowing bridges some control
> over pre_enable ordering. The meaty commit for our purposes is commit
> 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter
> bridge init order"). As documented by that series, if a bridge doesn't
> set "pre_enable_prev_first" then we should use the old ordering.
> 
> Now that we have the commit ("drm/bridge: tc358762: Set
> pre_enable_prev_first") we can go back to the old ordering, which also
> allows us to remove the ps8640 special case.
> 
> One last note is that even without reverting commit 7d8e9a90509f
> ("drm/msm/dsi: move DSI host powerup to modeset time"), if you _just_
> revert the ps8640 special case and try it out then it doesn't seem to
> fail anymore. I spent time bisecting / debugging this and it turns out
> to be mostly luck, so we still want this patch to make sure it's
> solid. Specifically the reason it sorta works these days is because
> we implemented wait_hpd_asserted() in ps8640 now, plus the magic of
> "pm_runtime" autosuspend. The fact that we have wait_hpd_asserted()
> implemented means that we actually power the bridge chip up just a wee
> bit earlier and then the bridge happens to stay on because of
> autosuspend and thus ends up powered before dsi_mgr_bridge_power_on().
> 
> Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---

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

-- 
With best wishes
Dmitry

Re: [RFT PATCH v2 2/3] drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset

[Dmitry Baryshkov]

On 01/02/2023 00:18, Douglas Anderson wrote:
> In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> time"), we moved powering up DSI hosts to modeset time. This wasn't
> because it was an elegant design, but there were no better options.
> 
> That commit actually ended up breaking ps8640, and thus was born
> commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time for
> parade-ps8640") as a temporary hack to un-break ps8640 by moving it to
> the old way of doing things. It turns out that ps8640 _really_ doesn't
> like its pre_enable() function to be called after
> dsi_mgr_bridge_power_on(). Specifically (from experimentation, not
> because I have any inside knowledge), it looks like the assertion of
> "RST#" in the ps8640 runtime resume handler seems like it's not
> allowed to happen after dsi_mgr_bridge_power_on()
> 
> Recently, Dave Stevenson's series landed allowing bridges some control
> over pre_enable ordering. The meaty commit for our purposes is commit
> 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter
> bridge init order"). As documented by that series, if a bridge doesn't
> set "pre_enable_prev_first" then we should use the old ordering.
> 
> Now that we have the commit ("drm/bridge: tc358762: Set
> pre_enable_prev_first") we can go back to the old ordering, which also
> allows us to remove the ps8640 special case.
> 
> One last note is that even without reverting commit 7d8e9a90509f
> ("drm/msm/dsi: move DSI host powerup to modeset time"), if you _just_
> revert the ps8640 special case and try it out then it doesn't seem to
> fail anymore. I spent time bisecting / debugging this and it turns out
> to be mostly luck, so we still want this patch to make sure it's
> solid. Specifically the reason it sorta works these days is because
> we implemented wait_hpd_asserted() in ps8640 now, plus the magic of
> "pm_runtime" autosuspend. The fact that we have wait_hpd_asserted()
> implemented means that we actually power the bridge chip up just a wee
> bit earlier and then the bridge happens to stay on because of
> autosuspend and thus ends up powered before dsi_mgr_bridge_power_on().
> 
> Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---

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

-- 
With best wishes
Dmitry

Re: [RFT PATCH v2 3/3] drm/msm/dsi: More properly handle errors in regards to dsi_mgr_bridge_power_on()

[Dmitry Baryshkov]

On 01/02/2023 00:18, Douglas Anderson wrote:
> In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> time") the error handling with regards to dsi_mgr_bridge_power_on()
> got a bit worse. Specifically if we failed to power the bridge on then
> nothing would really notice. The modeset function couldn't return an
> error and thus we'd blindly go forward and try to do the pre-enable.
> 
> In commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time
> for parade-ps8640") we added a special case to move the powerup back
> to pre-enable time for ps8640. When we did that, we didn't try to
> recover the old/better error handling just for ps8640.
> 
> In the patch ("drm/msm/dsi: Stop unconditionally powering up DSI hosts
> at modeset") we've now moved the powering up back to exclusively being
> during pre-enable. That means we can add the better error handling
> back in, so let's do it. To do so we'll add a new function
> dsi_mgr_bridge_power_off() that's matches how errors were handled
> prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to
> modeset time").
> 
> NOTE: Now that we have dsi_mgr_bridge_power_off(), it feels as if we
> should be calling it in dsi_mgr_bridge_post_disable(). That would make
> some sense, but doing so would change the current behavior and thus
> should be a separate patch. Specifically:
> * dsi_mgr_bridge_post_disable() always calls dsi_mgr_phy_disable()
>    even in the slave-DSI case of bonded DSI. We'd need to add special
>    handling for this if it's truly needed.
> * dsi_mgr_bridge_post_disable() calls msm_dsi_phy_pll_save_state()
>    midway through the poweroff.
> * dsi_mgr_bridge_post_disable() has a different order of some of the
>    poweroffs / IRQ disables.
> For now we'll leave dsi_mgr_bridge_post_disable() alone.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---

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

-- 
With best wishes
Dmitry

Re: [RFT PATCH v2 3/3] drm/msm/dsi: More properly handle errors in regards to dsi_mgr_bridge_power_on()

[Dmitry Baryshkov]

On 01/02/2023 00:18, Douglas Anderson wrote:
> In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> time") the error handling with regards to dsi_mgr_bridge_power_on()
> got a bit worse. Specifically if we failed to power the bridge on then
> nothing would really notice. The modeset function couldn't return an
> error and thus we'd blindly go forward and try to do the pre-enable.
> 
> In commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time
> for parade-ps8640") we added a special case to move the powerup back
> to pre-enable time for ps8640. When we did that, we didn't try to
> recover the old/better error handling just for ps8640.
> 
> In the patch ("drm/msm/dsi: Stop unconditionally powering up DSI hosts
> at modeset") we've now moved the powering up back to exclusively being
> during pre-enable. That means we can add the better error handling
> back in, so let's do it. To do so we'll add a new function
> dsi_mgr_bridge_power_off() that's matches how errors were handled
> prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to
> modeset time").
> 
> NOTE: Now that we have dsi_mgr_bridge_power_off(), it feels as if we
> should be calling it in dsi_mgr_bridge_post_disable(). That would make
> some sense, but doing so would change the current behavior and thus
> should be a separate patch. Specifically:
> * dsi_mgr_bridge_post_disable() always calls dsi_mgr_phy_disable()
>    even in the slave-DSI case of bonded DSI. We'd need to add special
>    handling for this if it's truly needed.
> * dsi_mgr_bridge_post_disable() calls msm_dsi_phy_pll_save_state()
>    midway through the poweroff.
> * dsi_mgr_bridge_post_disable() has a different order of some of the
>    poweroffs / IRQ disables.
> For now we'll leave dsi_mgr_bridge_post_disable() alone.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---

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

-- 
With best wishes
Dmitry

Re: [RFT PATCH v2 1/3] drm/bridge: tc358762: Set pre_enable_prev_first

[Doug Anderson]

Hi,

On Mon, Feb 27, 2023 at 5:24 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 28/02/2023 02:26, Doug Anderson wrote:
> > Hi,
> >
> > On Wed, Feb 1, 2023 at 1:51 AM Dave Stevenson
> > <dave.stevenson@raspberrypi.com> wrote:
> >>
> >> On Tue, 31 Jan 2023 at 22:22, Douglas Anderson <dianders@chromium.org> wrote:
> >>>
> >>> Set the "pre_enable_prev_first" as provided by commit 4fb912e5e190
> >>> ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init
> >>> order"). This should allow us to revert commit ec7981e6c614
> >>> ("drm/msm/dsi: don't powerup at modeset time for parade-ps8640") and
> >>> commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> >>> time").
> >>
> >> I see no reference in the TC358762 datasheet to requiring the DSI
> >> interface to be in any particular state.
> >> However, setting this flag does mean that the DSI host doesn't need to
> >> power up and down for each host_transfer request from
> >> tc358762_pre_enable/tc358762_init, so on that basis I'm good with it.
> >>
> >> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >>
> >>> Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >>> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
> >>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >>> ---
> >>>
> >>> (no changes since v1)
> >>>
> >>>   drivers/gpu/drm/bridge/tc358762.c | 1 +
> >>>   1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/bridge/tc358762.c b/drivers/gpu/drm/bridge/tc358762.c
> >>> index 0b6a28436885..77f7f7f54757 100644
> >>> --- a/drivers/gpu/drm/bridge/tc358762.c
> >>> +++ b/drivers/gpu/drm/bridge/tc358762.c
> >>> @@ -229,6 +229,7 @@ static int tc358762_probe(struct mipi_dsi_device *dsi)
> >>>          ctx->bridge.funcs = &tc358762_bridge_funcs;
> >>>          ctx->bridge.type = DRM_MODE_CONNECTOR_DPI;
> >>>          ctx->bridge.of_node = dev->of_node;
> >>> +       ctx->bridge.pre_enable_prev_first = true;
> >>>
> >>>          drm_bridge_add(&ctx->bridge);
> >
> > Abhinav asked what the plan was for landing this [1]. Since this isn't
> > urgent, I guess the plan is to land patch #1 in drm-misc-next. Then we
> > sit and wait until it percolates into mainline and, once it does, then
> > patch #2 and #3 can land.
> >
> > Since I have Dave's review I can commit this to drm-misc-next myself.
> > My plan will be to wait until Thursday or Friday of this week (to give
> > people a bit of time to object) and then land patch #1. Then I'll
> > snooze things for a while and poke Abhinav and Dmitry to land patch #2
> > / #3 when I notice it in mainline. If, at any point, someone comes out
> > of the woodwork and yells that this is breaking them then, worst case,
> > we can revert.
>
> This plan sounds good to me.

Pushed to drm-misc-next:

55cac10739d5 drm/bridge: tc358762: Set pre_enable_prev_first

If my math is right then I'd expect that to get into mainline for
6.4-rc1. I guess that means it'll be in Linus's tree mid-May. I'll
schedule a reminder to suggest landing at patches #2 and #3 again in
late May.

-Doug

Re: [RFT PATCH v2 1/3] drm/bridge: tc358762: Set pre_enable_prev_first

[Doug Anderson]

Hi,

On Mon, Feb 27, 2023 at 5:24 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 28/02/2023 02:26, Doug Anderson wrote:
> > Hi,
> >
> > On Wed, Feb 1, 2023 at 1:51 AM Dave Stevenson
> > <dave.stevenson@raspberrypi.com> wrote:
> >>
> >> On Tue, 31 Jan 2023 at 22:22, Douglas Anderson <dianders@chromium.org> wrote:
> >>>
> >>> Set the "pre_enable_prev_first" as provided by commit 4fb912e5e190
> >>> ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init
> >>> order"). This should allow us to revert commit ec7981e6c614
> >>> ("drm/msm/dsi: don't powerup at modeset time for parade-ps8640") and
> >>> commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> >>> time").
> >>
> >> I see no reference in the TC358762 datasheet to requiring the DSI
> >> interface to be in any particular state.
> >> However, setting this flag does mean that the DSI host doesn't need to
> >> power up and down for each host_transfer request from
> >> tc358762_pre_enable/tc358762_init, so on that basis I'm good with it.
> >>
> >> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >>
> >>> Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >>> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
> >>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >>> ---
> >>>
> >>> (no changes since v1)
> >>>
> >>>   drivers/gpu/drm/bridge/tc358762.c | 1 +
> >>>   1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/bridge/tc358762.c b/drivers/gpu/drm/bridge/tc358762.c
> >>> index 0b6a28436885..77f7f7f54757 100644
> >>> --- a/drivers/gpu/drm/bridge/tc358762.c
> >>> +++ b/drivers/gpu/drm/bridge/tc358762.c
> >>> @@ -229,6 +229,7 @@ static int tc358762_probe(struct mipi_dsi_device *dsi)
> >>>          ctx->bridge.funcs = &tc358762_bridge_funcs;
> >>>          ctx->bridge.type = DRM_MODE_CONNECTOR_DPI;
> >>>          ctx->bridge.of_node = dev->of_node;
> >>> +       ctx->bridge.pre_enable_prev_first = true;
> >>>
> >>>          drm_bridge_add(&ctx->bridge);
> >
> > Abhinav asked what the plan was for landing this [1]. Since this isn't
> > urgent, I guess the plan is to land patch #1 in drm-misc-next. Then we
> > sit and wait until it percolates into mainline and, once it does, then
> > patch #2 and #3 can land.
> >
> > Since I have Dave's review I can commit this to drm-misc-next myself.
> > My plan will be to wait until Thursday or Friday of this week (to give
> > people a bit of time to object) and then land patch #1. Then I'll
> > snooze things for a while and poke Abhinav and Dmitry to land patch #2
> > / #3 when I notice it in mainline. If, at any point, someone comes out
> > of the woodwork and yells that this is breaking them then, worst case,
> > we can revert.
>
> This plan sounds good to me.

Pushed to drm-misc-next:

55cac10739d5 drm/bridge: tc358762: Set pre_enable_prev_first

If my math is right then I'd expect that to get into mainline for
6.4-rc1. I guess that means it'll be in Linus's tree mid-May. I'll
schedule a reminder to suggest landing at patches #2 and #3 again in
late May.

-Doug

Re: [RFT PATCH v2 1/3] drm/bridge: tc358762: Set pre_enable_prev_first

[Dmitry Baryshkov]

On Thu, 2 Mar 2023 at 19:26, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Feb 27, 2023 at 5:24 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On 28/02/2023 02:26, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Wed, Feb 1, 2023 at 1:51 AM Dave Stevenson
> > > <dave.stevenson@raspberrypi.com> wrote:
> > >>
> > >> On Tue, 31 Jan 2023 at 22:22, Douglas Anderson <dianders@chromium.org> wrote:
> > >>>
> > >>> Set the "pre_enable_prev_first" as provided by commit 4fb912e5e190
> > >>> ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init
> > >>> order"). This should allow us to revert commit ec7981e6c614
> > >>> ("drm/msm/dsi: don't powerup at modeset time for parade-ps8640") and
> > >>> commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> > >>> time").
> > >>
> > >> I see no reference in the TC358762 datasheet to requiring the DSI
> > >> interface to be in any particular state.
> > >> However, setting this flag does mean that the DSI host doesn't need to
> > >> power up and down for each host_transfer request from
> > >> tc358762_pre_enable/tc358762_init, so on that basis I'm good with it.
> > >>
> > >> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > >>
> > >>> Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > >>> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > >>> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
> > >>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > >>> ---
> > >>>
> > >>> (no changes since v1)
> > >>>
> > >>>   drivers/gpu/drm/bridge/tc358762.c | 1 +
> > >>>   1 file changed, 1 insertion(+)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/bridge/tc358762.c b/drivers/gpu/drm/bridge/tc358762.c
> > >>> index 0b6a28436885..77f7f7f54757 100644
> > >>> --- a/drivers/gpu/drm/bridge/tc358762.c
> > >>> +++ b/drivers/gpu/drm/bridge/tc358762.c
> > >>> @@ -229,6 +229,7 @@ static int tc358762_probe(struct mipi_dsi_device *dsi)
> > >>>          ctx->bridge.funcs = &tc358762_bridge_funcs;
> > >>>          ctx->bridge.type = DRM_MODE_CONNECTOR_DPI;
> > >>>          ctx->bridge.of_node = dev->of_node;
> > >>> +       ctx->bridge.pre_enable_prev_first = true;
> > >>>
> > >>>          drm_bridge_add(&ctx->bridge);
> > >
> > > Abhinav asked what the plan was for landing this [1]. Since this isn't
> > > urgent, I guess the plan is to land patch #1 in drm-misc-next. Then we
> > > sit and wait until it percolates into mainline and, once it does, then
> > > patch #2 and #3 can land.
> > >
> > > Since I have Dave's review I can commit this to drm-misc-next myself.
> > > My plan will be to wait until Thursday or Friday of this week (to give
> > > people a bit of time to object) and then land patch #1. Then I'll
> > > snooze things for a while and poke Abhinav and Dmitry to land patch #2
> > > / #3 when I notice it in mainline. If, at any point, someone comes out
> > > of the woodwork and yells that this is breaking them then, worst case,
> > > we can revert.
> >
> > This plan sounds good to me.
>
> Pushed to drm-misc-next:
>
> 55cac10739d5 drm/bridge: tc358762: Set pre_enable_prev_first
>
> If my math is right then I'd expect that to get into mainline for
> 6.4-rc1. I guess that means it'll be in Linus's tree mid-May. I'll
> schedule a reminder to suggest landing at patches #2 and #3 again in
> late May.

It might be earlier, if msm-next merges drm-misc earlier (e.g. for the
PSR patches).

-- 
With best wishes
Dmitry

Re: [RFT PATCH v2 1/3] drm/bridge: tc358762: Set pre_enable_prev_first

[Dmitry Baryshkov]

On Thu, 2 Mar 2023 at 19:26, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Feb 27, 2023 at 5:24 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On 28/02/2023 02:26, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Wed, Feb 1, 2023 at 1:51 AM Dave Stevenson
> > > <dave.stevenson@raspberrypi.com> wrote:
> > >>
> > >> On Tue, 31 Jan 2023 at 22:22, Douglas Anderson <dianders@chromium.org> wrote:
> > >>>
> > >>> Set the "pre_enable_prev_first" as provided by commit 4fb912e5e190
> > >>> ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init
> > >>> order"). This should allow us to revert commit ec7981e6c614
> > >>> ("drm/msm/dsi: don't powerup at modeset time for parade-ps8640") and
> > >>> commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> > >>> time").
> > >>
> > >> I see no reference in the TC358762 datasheet to requiring the DSI
> > >> interface to be in any particular state.
> > >> However, setting this flag does mean that the DSI host doesn't need to
> > >> power up and down for each host_transfer request from
> > >> tc358762_pre_enable/tc358762_init, so on that basis I'm good with it.
> > >>
> > >> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > >>
> > >>> Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > >>> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > >>> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
> > >>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > >>> ---
> > >>>
> > >>> (no changes since v1)
> > >>>
> > >>>   drivers/gpu/drm/bridge/tc358762.c | 1 +
> > >>>   1 file changed, 1 insertion(+)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/bridge/tc358762.c b/drivers/gpu/drm/bridge/tc358762.c
> > >>> index 0b6a28436885..77f7f7f54757 100644
> > >>> --- a/drivers/gpu/drm/bridge/tc358762.c
> > >>> +++ b/drivers/gpu/drm/bridge/tc358762.c
> > >>> @@ -229,6 +229,7 @@ static int tc358762_probe(struct mipi_dsi_device *dsi)
> > >>>          ctx->bridge.funcs = &tc358762_bridge_funcs;
> > >>>          ctx->bridge.type = DRM_MODE_CONNECTOR_DPI;
> > >>>          ctx->bridge.of_node = dev->of_node;
> > >>> +       ctx->bridge.pre_enable_prev_first = true;
> > >>>
> > >>>          drm_bridge_add(&ctx->bridge);
> > >
> > > Abhinav asked what the plan was for landing this [1]. Since this isn't
> > > urgent, I guess the plan is to land patch #1 in drm-misc-next. Then we
> > > sit and wait until it percolates into mainline and, once it does, then
> > > patch #2 and #3 can land.
> > >
> > > Since I have Dave's review I can commit this to drm-misc-next myself.
> > > My plan will be to wait until Thursday or Friday of this week (to give
> > > people a bit of time to object) and then land patch #1. Then I'll
> > > snooze things for a while and poke Abhinav and Dmitry to land patch #2
> > > / #3 when I notice it in mainline. If, at any point, someone comes out
> > > of the woodwork and yells that this is breaking them then, worst case,
> > > we can revert.
> >
> > This plan sounds good to me.
>
> Pushed to drm-misc-next:
>
> 55cac10739d5 drm/bridge: tc358762: Set pre_enable_prev_first
>
> If my math is right then I'd expect that to get into mainline for
> 6.4-rc1. I guess that means it'll be in Linus's tree mid-May. I'll
> schedule a reminder to suggest landing at patches #2 and #3 again in
> late May.

It might be earlier, if msm-next merges drm-misc earlier (e.g. for the
PSR patches).

-- 
With best wishes
Dmitry