drm/bridge: cdns-dsi: Fix the color-shift issue

From: Aradhya Bhatia <[email protected]>

Instead of manually finding the next bridge/panel, and maintaining the panel-bridge (in-case the next entity is a panel), switch to using the automatically managing devm_drm_of_get_bridge() API.

Drop the drm_panel support completely from the driver while at it.

Reviewed-by Tomi Valkeinen <[email protected]>

Signed-off-by Aradhya Bhatia <[email protected]>

Signed-off-by Aradhya Bhatia <[email protected]>

---

From: Aradhya Bhatia <[email protected]>

Initialize the Phy during the cdns-dsi _resume(), and de-initialize it during the _suspend().

Also power-off the Phy from bridge_disable.

Fixes fced5a364dee ("drm/bridge: cdns: Convert to phy framework")

Signed-off-by Aradhya Bhatia <[email protected]>

Signed-off-by Aradhya Bhatia <[email protected]>

---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c +4 -0

index 5159c3f0853e..d89c32bae2b9 100644
@@ -672,6 +672,10 @@ static void cdns_dsi_bridge_disable(struct drm_bridge *bridge)
 	if (dsi->platform_ops && dsi->platform_ops->disable)
 		dsi->platform_ops->disable(dsi);
 
+	phy_power_off(dsi->dphy);
+	dsi->link_initialized = false;
+	dsi->phy_initialized = false;
+
 	pm_runtime_put(dsi->base.dev);
 }
 

drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c +4 -1

 	phy_set_mode(dsi->dphy, PHY_MODE_MIPI_DPHY);
 	phy_configure(dsi->dphy, &output->phy_opts);
 	phy_power_on(dsi->dphy);

@@ -1120,6 +1123,8 @@ static int __maybe_unused cdns_dsi_resume(struct device *dev)
 	clk_prepare_enable(dsi->dsi_p_clk);
 	clk_prepare_enable(dsi->dsi_sys_clk);
 
+	phy_init(dsi->dphy);
+
 	return 0;
 }
 

@@ -1127,10 +1132,11 @@ static int __maybe_unused cdns_dsi_suspend(struct device *dev)
 {
 	struct cdns_dsi *dsi = dev_get_drvdata(dev);
 
+	phy_exit(dsi->dphy);
+
 	clk_disable_unprepare(dsi->dsi_sys_clk);
 	clk_disable_unprepare(dsi->dsi_p_clk);
 	reset_control_assert(dsi->dsi_p_rst);
-	dsi->link_initialized = false;
 	return 0;
 }
 

Signature · 1 line

2.34.1

From: Aradhya Bhatia <[email protected]>

Check for the return value of the phy_mipi_dphy_get_default_config() call, and incase of an error, return back the same.

Fixes fced5a364dee ("drm/bridge: cdns: Convert to phy framework")

Signed-off-by Aradhya Bhatia <[email protected]>

Signed-off-by Aradhya Bhatia <[email protected]>

---
  drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 8 +++++---
  1 file changed, 5 insertions(+), 3 deletions(-)

drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c +5 -3

index 2fc24352d989..e4c0968313af 100644
@@ -575,9 +575,11 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
  	if (ret)
  		return ret;
 
-	phy_mipi_dphy_get_default_config(mode_clock * 1000,
-					 mipi_dsi_pixel_format_to_bpp(output->dev->format),
-					 nlanes, phy_cfg);
+	ret = phy_mipi_dphy_get_default_config(mode_clock * 1000,
+					       mipi_dsi_pixel_format_to_bpp(output->dev->format),
+					       nlanes, phy_cfg);
+	if (ret)
+		return ret;
 
  	ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, mode, mode_valid_check);
  	if (ret)

From: Aradhya Bhatia <[email protected]>

Hi Aradhya,

Thanks for the patch.

Quoted 1 line · Aradhya Bhatia

From: Aradhya Bhatia <[email protected]>

[...]

drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c static void cdns_dsi_bridge_enable(struct drm_bridge *bridge) · line 796

Quoted 10 lines

drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c +10 -0

+	/*
+	 * Now that the DSI Link and DSI Phy are initialized,
+	 * wait for the CLK and Data Lanes to be ready.
+	 */
+	tmp = CLK_LANE_RDY;
+	for (int i = 0; i < nlanes; i++)
+		tmp |= DATA_LANE_RDY(i);
+
+	if (readl_poll_timeout(dsi->regs + MCTL_MAIN_STS, status,
+			       status & tmp, 100, 500000))

The above would mark the condition as true even if one data lane gets ready. I think we need to poll until all data lanes are marked as ready. Also good to give a warning in case we time out.

IMHO below should fix this:

        WARN_ON_ONCE(readl_poll_timeout(dsi->regs + MCTL_MAIN_STS, status,
                                       (tmp == (status & tmp)), 100, 0));

From: Aradhya Bhatia <[email protected]>

At present, the DSI mode configuration check happens during the _atomic_enable() phase, which is not really the best place for this. Moreover, if the mode is not valid, the driver gives a warning and continues the hardware configuration.

Move the DSI mode configuration check to _atomic_check() instead, which can properly report back any invalid mode, before the _enable phase even begins.

Signed-off-by Aradhya Bhatia <[email protected]>

Signed-off-by Aradhya Bhatia <[email protected]>

---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 18 +++++++++++++++---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.h |  1 +
 2 files changed, 16 insertions(+), 3 deletions(-)

drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c +1 -3

index 7341e609dc8b..79d8c2264c14 100644
@@ -769,7 +769,7 @@ static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
 	struct drm_display_mode *mode;
 	struct phy_configure_opts_mipi_dphy *phy_cfg = &output->phy_opts.mipi_dphy;
 	unsigned long tx_byte_period;
-	struct cdns_dsi_cfg dsi_cfg;
+	struct cdns_dsi_cfg dsi_cfg = dsi->dsi_cfg;
 	u32 tmp, reg_wakeup, div, status;
 	int nlanes;
 

@@ -782,8 +782,6 @@ static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
 	mode = &bridge->encoder->crtc->state->adjusted_mode;
 	nlanes = output->dev->lanes;
 
-	WARN_ON_ONCE(cdns_dsi_check_conf(dsi, mode, &dsi_cfg, false));
-
 	cdns_dsi_init_link(dsi);
 	cdns_dsi_hs_init(dsi);
 

@@ -954,6 +952,19 @@ static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
 	return input_fmts;
 }

drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c +5 -0

+
+	return cdns_dsi_check_conf(dsi, mode, dsi_cfg, false);
+}
+
 static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
 	.attach = cdns_dsi_bridge_attach,
 	.mode_valid = cdns_dsi_bridge_mode_valid,

@@ -961,6 +972,7 @@ static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
 	.atomic_pre_enable = cdns_dsi_bridge_atomic_pre_enable,
 	.atomic_enable = cdns_dsi_bridge_atomic_enable,
 	.atomic_post_disable = cdns_dsi_bridge_atomic_post_disable,
+	.atomic_check = cdns_dsi_bridge_atomic_check,
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
 	.atomic_reset = drm_atomic_helper_bridge_reset,

drivers/gpu/drm/bridge/cadence/cdns-dsi-core.h +1 -0

index 5db5dbbbcaad..b785df45bc59 100644
@@ -77,6 +77,7 @@ struct cdns_dsi {
 	bool link_initialized;
 	bool phy_initialized;
 	struct phy *dphy;
+	struct cdns_dsi_cfg dsi_cfg;
 };
 
 #endif /* !__CDNS_DSI_H__ */

Signature · 1 line

2.34.1

From: Aradhya Bhatia <[email protected]>

The way any singular display pipeline, in need of a modeset, gets enabled is as follows -

CRTC Enable
All Bridge Pre-Enable
Encoder Enable
All Bridge Enable

- and the disable path is exactly the reverse of this.

The CRTC enable/disable occurs by looping over the old and new CRTC states, while the bridge pre-enable, encoder enable, and bridge enable occur by looping through the new connector states of the display pipelines.

At the moment, the encoder and bridge operations are grouped together and occur under the same loops.

Separate out the enable/disable loops of the encoder and bridge operations into a different function, to make way for the re-ordering of the enable and disable sequence of all these display elements.

This patch doesn't alter in any way the ordering of CRTC/encoder/bridge enable and disable, but merely helps to cleanly set up for the next patch and to make sure that the bisectibility remains intact.

Signed-off-by Aradhya Bhatia <[email protected]>

Signed-off-by Aradhya Bhatia <[email protected]>

---
 drivers/gpu/drm/drm_atomic_helper.c | 97 +++++++++++++++++------------
 1 file changed, 57 insertions(+), 40 deletions(-)

drivers/gpu/drm/drm_atomic_helper.c +56 -4

index 5186d2114a50..7741fbcc8fc7 100644
@@ -1122,11 +1122,10 @@ crtc_needs_disable(struct drm_crtc_state *old_state,
 }
 
 static void
-disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
+encoder_bridge_chain_disable(struct drm_device *dev, struct drm_atomic_state *old_state)
 {
 	struct drm_connector *connector;
 	struct drm_connector_state *old_conn_state, *new_conn_state;
-	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	int i;
 

@@ -1189,6 +1188,16 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 
 		drm_atomic_bridge_chain_post_disable(bridge, old_state);
 	}
+}
+
+static void
+disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
+{
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+	int i;
+
+	encoder_bridge_chain_disable(dev, old_state);
 
 	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
 		const struct drm_crtc_helper_funcs *funcs;

@@ -1445,6 +1454,51 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
 	}
 }
 
+static void
+encoder_bridge_chain_enable(struct drm_device *dev, struct drm_atomic_state *old_state)
+{
+	struct drm_connector *connector;
+	struct drm_connector_state *new_conn_state;
+	int i;
+
+	for_each_new_connector_in_state(old_state, connector, new_conn_state, i) {
+		const struct drm_encoder_helper_funcs *funcs;
+		struct drm_encoder *encoder;
+		struct drm_bridge *bridge;
+
+		if (!new_conn_state->best_encoder)
+			continue;
+
+		if (!new_conn_state->crtc->state->active ||
+		    !drm_atomic_crtc_needs_modeset(new_conn_state->crtc->state))
+			continue;
+
+		encoder = new_conn_state->best_encoder;
+		funcs = encoder->helper_private;
+
+		drm_dbg_atomic(dev, "enabling [ENCODER:%d:%s]\n",
+			       encoder->base.id, encoder->name);
+
+		/*
+		 * Each encoder has at most one connector (since we always steal
+		 * it away), so we won't call enable hooks twice.
+		 */
+		bridge = drm_bridge_chain_get_first_bridge(encoder);
+		drm_atomic_bridge_chain_pre_enable(bridge, old_state);
+
+		if (funcs) {
+			if (funcs->atomic_enable)
+				funcs->atomic_enable(encoder, old_state);
+			else if (funcs->enable)
+				funcs->enable(encoder);
+			else if (funcs->commit)
+				funcs->commit(encoder);
+		}
+
+		drm_atomic_bridge_chain_enable(bridge, old_state);
+	}
+}
+
 /**
  * drm_atomic_helper_commit_modeset_enables - modeset commit to enable outputs
  * @dev: DRM device

@@ -1465,8 +1519,6 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state;
 	struct drm_crtc_state *new_crtc_state;
-	struct drm_connector *connector;
-	struct drm_connector_state *new_conn_state;
 	int i;
 
 	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {

drivers/gpu/drm/drm_atomic_helper.c +1 -36

-	for_each_new_connector_in_state(old_state, connector, new_conn_state, i) {
-		const struct drm_encoder_helper_funcs *funcs;
-		struct drm_encoder *encoder;
-		struct drm_bridge *bridge;
-
-		if (!new_conn_state->best_encoder)
-			continue;
-
-		if (!new_conn_state->crtc->state->active ||
-		    !drm_atomic_crtc_needs_modeset(new_conn_state->crtc->state))
-			continue;
-
-		encoder = new_conn_state->best_encoder;
-		funcs = encoder->helper_private;
-
-		drm_dbg_atomic(dev, "enabling [ENCODER:%d:%s]\n",
-			       encoder->base.id, encoder->name);
-
-		/*
-		 * Each encoder has at most one connector (since we always steal
-		 * it away), so we won't call enable hooks twice.
-		 */
-		bridge = drm_bridge_chain_get_first_bridge(encoder);
-		drm_atomic_bridge_chain_pre_enable(bridge, old_state);
-
-		if (funcs) {
-			if (funcs->atomic_enable)
-				funcs->atomic_enable(encoder, old_state);
-			else if (funcs->enable)
-				funcs->enable(encoder);
-			else if (funcs->commit)
-				funcs->commit(encoder);
-		}
-
-		drm_atomic_bridge_chain_enable(bridge, old_state);
-	}
+	encoder_bridge_chain_enable(dev, old_state);
 
 	drm_atomic_helper_commit_writebacks(dev, old_state);
 }

Signature · 1 line

2.34.1

From: Aradhya Bhatia <[email protected]>

Move the bridge pre_enable call before crtc enable, and the bridge post_disable call after the crtc disable.

The sequence of enable after this patch will look like:

	bridge[n]_pre_enable
	...
	bridge[1]_pre_enable

	crtc_enable
	encoder_enable

	bridge[1]_enable
	...
	bridge[n]_enable

and vice-versa for the bridge chain disable sequence.

The definition of bridge pre_enable hook says that,
"The display pipe (i.e. clocks and timing signals) feeding this bridge will not yet be running when this callback is called".

Since CRTC is also a source feeding the bridge, it should not be enabled before the bridges in the pipeline are pre_enabled. Fix that by re-ordering the sequence of bridge pre_enable and bridge post_disable.

Signed-off-by Aradhya Bhatia <[email protected]>

Signed-off-by Aradhya Bhatia <[email protected]>

---
 drivers/gpu/drm/drm_atomic_helper.c | 102 ++++++++++++++++++----------
 include/drm/drm_atomic_helper.h     |   5 ++
 2 files changed, 71 insertions(+), 36 deletions(-)

drivers/gpu/drm/drm_atomic_helper.c +66 -36

index 7741fbcc8fc7..6ebd869df79b 100644
@@ -1122,7 +1122,8 @@ crtc_needs_disable(struct drm_crtc_state *old_state,
 }
 
 static void
-encoder_bridge_chain_disable(struct drm_device *dev, struct drm_atomic_state *old_state)
+encoder_bridge_chain_disable(struct drm_device *dev, struct drm_atomic_state *old_state,
+			     enum bridge_chain_operation_type op_type)
 {
 	struct drm_connector *connector;
 	struct drm_connector_state *old_conn_state, *new_conn_state;

@@ -1162,31 +1163,43 @@ encoder_bridge_chain_disable(struct drm_device *dev, struct drm_atomic_state *ol
 		if (WARN_ON(!encoder))
 			continue;
 
-		funcs = encoder->helper_private;
-
-		drm_dbg_atomic(dev, "disabling [ENCODER:%d:%s]\n",
-			       encoder->base.id, encoder->name);
-
 		/*
 		 * Each encoder has at most one connector (since we always steal
 		 * it away), so we won't call disable hooks twice.
 		 */
 		bridge = drm_bridge_chain_get_first_bridge(encoder);
-		drm_atomic_bridge_chain_disable(bridge, old_state);
 
-		/* Right function depends upon target state. */
-		if (funcs) {
-			if (funcs->atomic_disable)
-				funcs->atomic_disable(encoder, old_state);
-			else if (new_conn_state->crtc && funcs->prepare)
-				funcs->prepare(encoder);
-			else if (funcs->disable)
-				funcs->disable(encoder);
-			else if (funcs->dpms)
-				funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
-		}
+		switch (op_type) {
+		case DRM_BRIDGE_ENABLE_DISABLE:
+			funcs = encoder->helper_private;
+
+			drm_dbg_atomic(dev, "disabling [ENCODER:%d:%s]\n",
+				       encoder->base.id, encoder->name);
+
+			drm_atomic_bridge_chain_disable(bridge, old_state);
+
+			/* Right function depends upon target state. */
+			if (funcs) {
+				if (funcs->atomic_disable)
+					funcs->atomic_disable(encoder, old_state);
+				else if (new_conn_state->crtc && funcs->prepare)
+					funcs->prepare(encoder);
+				else if (funcs->disable)
+					funcs->disable(encoder);
+				else if (funcs->dpms)
+					funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
+			}
 
-		drm_atomic_bridge_chain_post_disable(bridge, old_state);
+			break;
+
+		case DRM_BRIDGE_PRE_ENABLE_POST_DISABLE:
+			drm_atomic_bridge_chain_post_disable(bridge, old_state);
+			break;
+
+		default:
+			drm_err(dev, "Unrecognized Encoder/Bridge Operation (%d).\n", op_type);
+			break;
+		}
 	}
 }
 

@@ -1197,7 +1210,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	int i;
 
-	encoder_bridge_chain_disable(dev, old_state);
+	encoder_bridge_chain_disable(dev, old_state, DRM_BRIDGE_ENABLE_DISABLE);
 
 	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
 		const struct drm_crtc_helper_funcs *funcs;

@@ -1243,6 +1256,8 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 		if (ret == 0)
 			drm_crtc_vblank_put(crtc);
 	}
+
+	encoder_bridge_chain_disable(dev, old_state, DRM_BRIDGE_PRE_ENABLE_POST_DISABLE);
 }
 
 /**

@@ -1455,7 +1470,8 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
 }
 
 static void
-encoder_bridge_chain_enable(struct drm_device *dev, struct drm_atomic_state *old_state)
+encoder_bridge_chain_enable(struct drm_device *dev, struct drm_atomic_state *old_state,
+			    enum bridge_chain_operation_type op_type)
 {
 	struct drm_connector *connector;
 	struct drm_connector_state *new_conn_state;

@@ -1474,28 +1490,40 @@ encoder_bridge_chain_enable(struct drm_device *dev, struct drm_atomic_state *old
 			continue;
 
 		encoder = new_conn_state->best_encoder;
-		funcs = encoder->helper_private;
-
-		drm_dbg_atomic(dev, "enabling [ENCODER:%d:%s]\n",
-			       encoder->base.id, encoder->name);
 
 		/*
 		 * Each encoder has at most one connector (since we always steal
 		 * it away), so we won't call enable hooks twice.
 		 */
 		bridge = drm_bridge_chain_get_first_bridge(encoder);
-		drm_atomic_bridge_chain_pre_enable(bridge, old_state);
 
-		if (funcs) {
-			if (funcs->atomic_enable)
-				funcs->atomic_enable(encoder, old_state);
-			else if (funcs->enable)
-				funcs->enable(encoder);
-			else if (funcs->commit)
-				funcs->commit(encoder);
-		}
+		switch (op_type) {
+		case DRM_BRIDGE_PRE_ENABLE_POST_DISABLE:
+			drm_atomic_bridge_chain_pre_enable(bridge, old_state);
+			break;
+
+		case DRM_BRIDGE_ENABLE_DISABLE:
+			funcs = encoder->helper_private;
+
+			drm_dbg_atomic(dev, "enabling [ENCODER:%d:%s]\n",
+				       encoder->base.id, encoder->name);
 
-		drm_atomic_bridge_chain_enable(bridge, old_state);
+			if (funcs) {
+				if (funcs->atomic_enable)
+					funcs->atomic_enable(encoder, old_state);
+				else if (funcs->enable)
+					funcs->enable(encoder);
+				else if (funcs->commit)
+					funcs->commit(encoder);
+			}
+
+			drm_atomic_bridge_chain_enable(bridge, old_state);
+			break;
+
+		default:
+			drm_err(dev, "Unrecognized Encoder/Bridge Operation (%d).\n", op_type);
+			break;
+		}
 	}
 }
 

@@ -1521,6 +1549,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 	struct drm_crtc_state *new_crtc_state;
 	int i;
 
+	encoder_bridge_chain_enable(dev, old_state, DRM_BRIDGE_PRE_ENABLE_POST_DISABLE);
+
 	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
 		const struct drm_crtc_helper_funcs *funcs;
 

@@ -1543,7 +1573,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 		}
 	}
 
-	encoder_bridge_chain_enable(dev, old_state);
+	encoder_bridge_chain_enable(dev, old_state, DRM_BRIDGE_ENABLE_DISABLE);
 
 	drm_atomic_helper_commit_writebacks(dev, old_state);
 }

include/drm/drm_atomic_helper.h +0 -0

index 9aa0a05aa072..92a5812adc6c 100644

include/drm/drm_atomic_helper.h +0 -0

 struct drm_atomic_state;
 struct drm_private_obj;
 struct drm_private_state;

Signature · 1 line

2.34.1

From: Aradhya Bhatia <[email protected]>

The cdns-dsi controller requires that it be turned on completely before the input DPI's source has begun streaming[0]. Not having that, allows for a small window before cdns-dsi enable and after cdns-dsi disable where the previous entity (in this case tidss's videoport) to continue streaming DPI video signals. This small window where cdns-dsi is disabled but is still receiving signals causes the input FIFO of cdns-dsi to get corrupted. This causes the colors to shift on the output display. The colors can either shift by one color component (R->G, G->B, B->R), or by two color components (R->B, G->R, B->G).

Since tidss's videoport starts streaming via crtc enable hooks, we need cdns-dsi to be up and running before that. Now that the bridges are pre_enabled before crtc is enabled, and post_disabled after crtc is disabled, use the pre_enable and post_disable hooks to get cdns-dsi ready and running before the tidss videoport to get pass the color shift issues.

[0]: See section 12.6.5.7.3 "Start-up Procedure" in J721E SoC TRM
TRM Link: http://www.ti.com/lit/pdf/spruil1

Reviewed-by Tomi Valkeinen <[email protected]>

Signed-off-by Aradhya Bhatia <[email protected]>

Signed-off-by Aradhya Bhatia <[email protected]>

---
 .../gpu/drm/bridge/cadence/cdns-dsi-core.c    | 62 ++++++++++---------
 1 file changed, 34 insertions(+), 28 deletions(-)

drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c +34 -28

index 79d8c2264c14..dfeb53841ebc 100644
@@ -658,13 +658,28 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
 	return MODE_OK;
 }
 
-static void cdns_dsi_bridge_atomic_disable(struct drm_bridge *bridge,
-					   struct drm_bridge_state *old_bridge_state)
+static void cdns_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge,
+						struct drm_bridge_state *old_bridge_state)
 {
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);
 	u32 val;
 
+	/*
+	 * The cdns-dsi controller needs to be disabled after it's DPI source
+	 * has stopped streaming. If this is not followed, there is a brief
+	 * window before DPI source is disabled and after cdns-dsi controller
+	 * has been disabled where the DPI stream is still on, but the cdns-dsi
+	 * controller is not ready anymore to accept the incoming signals. This
+	 * is one of the reasons why a shift in pixel colors is observed on
+	 * displays that have cdns-dsi as one of the bridges.
+	 *
+	 * To mitigate this, disable this bridge from the bridge post_disable()
+	 * hook, instead of the bridge _disable() hook. The bridge post_disable()
+	 * hook gets called after the CRTC disable, where often many DPI sources
+	 * disable their streams.
+	 */
+
 	val = readl(dsi->regs + MCTL_MAIN_DATA_CTL);
 	val &= ~(IF_VID_SELECT_MASK | IF_VID_MODE | VID_EN | HOST_EOT_GEN |
 		 DISP_EOT_GEN);

@@ -683,15 +698,6 @@ static void cdns_dsi_bridge_atomic_disable(struct drm_bridge *bridge,
 	pm_runtime_put(dsi->base.dev);
 }
 
-static void cdns_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge,
-						struct drm_bridge_state *old_bridge_state)
-{
-	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
-	struct cdns_dsi *dsi = input_to_dsi(input);
-
-	pm_runtime_put(dsi->base.dev);
-}
-
 static void cdns_dsi_hs_init(struct cdns_dsi *dsi)
 {
 	struct cdns_dsi_output *output = &dsi->output;

@@ -760,8 +766,8 @@ static void cdns_dsi_init_link(struct cdns_dsi *dsi)
 	dsi->link_initialized = true;
 }
 
-static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
-					  struct drm_bridge_state *old_bridge_state)
+static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
+					      struct drm_bridge_state *old_bridge_state)
 {
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);

@@ -776,6 +782,21 @@ static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
 	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
 		return;
 
+	/*
+	 * The cdns-dsi controller needs to be enabled before it's DPI source
+	 * has begun streaming. If this is not followed, there is a brief window
+	 * after DPI source enable and before cdns-dsi controller enable where
+	 * the DPI stream is on, but the cdns-dsi controller is not ready to
+	 * accept the incoming signals. This is one of the reasons why a shift
+	 * in pixel colors is observed on displays that have cdns-dsi as one of
+	 * the bridges.
+	 *
+	 * To mitigate this, enable this bridge from the bridge pre_enable()
+	 * hook, instead of the bridge _enable() hook. The bridge pre_enable()
+	 * hook gets called before the CRTC enable, where often many DPI sources
+	 * enable their streams.
+	 */
+
 	if (dsi->platform_ops && dsi->platform_ops->enable)
 		dsi->platform_ops->enable(dsi);
 

@@ -912,19 +933,6 @@ static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
 	writel(tmp, dsi->regs + MCTL_MAIN_EN);
 }
 
-static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
-					      struct drm_bridge_state *old_bridge_state)
-{
-	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
-	struct cdns_dsi *dsi = input_to_dsi(input);
-
-	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
-		return;
-
-	cdns_dsi_init_link(dsi);
-	cdns_dsi_hs_init(dsi);
-}
-
 static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
 					       struct drm_bridge_state *bridge_state,
 					       struct drm_crtc_state *crtc_state,

@@ -968,9 +976,7 @@ static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
 static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
 	.attach = cdns_dsi_bridge_attach,
 	.mode_valid = cdns_dsi_bridge_mode_valid,
-	.atomic_disable = cdns_dsi_bridge_atomic_disable,
 	.atomic_pre_enable = cdns_dsi_bridge_atomic_pre_enable,
-	.atomic_enable = cdns_dsi_bridge_atomic_enable,
 	.atomic_post_disable = cdns_dsi_bridge_atomic_post_disable,
 	.atomic_check = cdns_dsi_bridge_atomic_check,
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,

Signature · 1 line

2.34.1

Quoted 12 lines · Aradhya Bhatia

From: Aradhya Bhatia <[email protected]>

The cdns-dsi controller requires that it be turned on completely before the input DPI's source has begun streaming[0]. Not having that, allows for a small window before cdns-dsi enable and after cdns-dsi disable where the previous entity (in this case tidss's videoport) to continue streaming DPI video signals. This small window where cdns-dsi is disabled but is still receiving signals causes the input FIFO of cdns-dsi to get corrupted. This causes the colors to shift on the output display. The colors can either shift by one color component (R->G, G->B, B->R), or by two color components (R->B, G->R, B->G).

Quoted 7 lines

Since tidss's videoport starts streaming via crtc enable hooks, we need cdns-dsi to be up and running before that. Now that the bridges are pre_enabled before crtc is enabled, and post_disabled after crtc is disabled, use the pre_enable and post_disable hooks to get cdns-dsi ready and running before the tidss videoport to get pass the color shift issues.

Not being an expert in the TI DSS driver, would it be more proper to handle that in the TI driver instead? I mean, sending out DPI signals isn't a part of the CRTC setup, it's a job of the encoder.

Quoted 128 lines

[0]: See section 12.6.5.7.3 "Start-up Procedure" in J721E SoC TRM
TRM Link: http://www.ti.com/lit/pdf/spruil1

Reviewed-by Tomi Valkeinen <[email protected]>

Signed-off-by Aradhya Bhatia <[email protected]>

Signed-off-by Aradhya Bhatia <[email protected]>

---
 .../gpu/drm/bridge/cadence/cdns-dsi-core.c    | 62 ++++++++++---------
 1 file changed, 34 insertions(+), 28 deletions(-)

drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c +34 -28

index 79d8c2264c14..dfeb53841ebc 100644
@@ -658,13 +658,28 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
 	return MODE_OK;
 }
 
-static void cdns_dsi_bridge_atomic_disable(struct drm_bridge *bridge,
-					   struct drm_bridge_state *old_bridge_state)
+static void cdns_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge,
+						struct drm_bridge_state *old_bridge_state)
 {
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);
 	u32 val;
 
+	/*
+	 * The cdns-dsi controller needs to be disabled after it's DPI source
+	 * has stopped streaming. If this is not followed, there is a brief
+	 * window before DPI source is disabled and after cdns-dsi controller
+	 * has been disabled where the DPI stream is still on, but the cdns-dsi
+	 * controller is not ready anymore to accept the incoming signals. This
+	 * is one of the reasons why a shift in pixel colors is observed on
+	 * displays that have cdns-dsi as one of the bridges.
+	 *
+	 * To mitigate this, disable this bridge from the bridge post_disable()
+	 * hook, instead of the bridge _disable() hook. The bridge post_disable()
+	 * hook gets called after the CRTC disable, where often many DPI sources
+	 * disable their streams.
+	 */
+
 	val = readl(dsi->regs + MCTL_MAIN_DATA_CTL);
 	val &= ~(IF_VID_SELECT_MASK | IF_VID_MODE | VID_EN | HOST_EOT_GEN |
 		 DISP_EOT_GEN);

@@ -683,15 +698,6 @@ static void cdns_dsi_bridge_atomic_disable(struct drm_bridge *bridge,
 	pm_runtime_put(dsi->base.dev);
 }
 
-static void cdns_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge,
-						struct drm_bridge_state *old_bridge_state)
-{
-	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
-	struct cdns_dsi *dsi = input_to_dsi(input);
-
-	pm_runtime_put(dsi->base.dev);
-}
-
 static void cdns_dsi_hs_init(struct cdns_dsi *dsi)
 {
 	struct cdns_dsi_output *output = &dsi->output;

@@ -760,8 +766,8 @@ static void cdns_dsi_init_link(struct cdns_dsi *dsi)
 	dsi->link_initialized = true;
 }
 
-static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
-					  struct drm_bridge_state *old_bridge_state)
+static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
+					      struct drm_bridge_state *old_bridge_state)
 {
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);

@@ -776,6 +782,21 @@ static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
 	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
 		return;
 
+	/*
+	 * The cdns-dsi controller needs to be enabled before it's DPI source
+	 * has begun streaming. If this is not followed, there is a brief window
+	 * after DPI source enable and before cdns-dsi controller enable where
+	 * the DPI stream is on, but the cdns-dsi controller is not ready to
+	 * accept the incoming signals. This is one of the reasons why a shift
+	 * in pixel colors is observed on displays that have cdns-dsi as one of
+	 * the bridges.
+	 *
+	 * To mitigate this, enable this bridge from the bridge pre_enable()
+	 * hook, instead of the bridge _enable() hook. The bridge pre_enable()
+	 * hook gets called before the CRTC enable, where often many DPI sources
+	 * enable their streams.
+	 */
+
 	if (dsi->platform_ops && dsi->platform_ops->enable)
 		dsi->platform_ops->enable(dsi);
 

@@ -912,19 +933,6 @@ static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
 	writel(tmp, dsi->regs + MCTL_MAIN_EN);
 }
 
-static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
-					      struct drm_bridge_state *old_bridge_state)
-{
-	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
-	struct cdns_dsi *dsi = input_to_dsi(input);
-
-	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
-		return;
-
-	cdns_dsi_init_link(dsi);
-	cdns_dsi_hs_init(dsi);
-}
-
 static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
 					       struct drm_bridge_state *bridge_state,
 					       struct drm_crtc_state *crtc_state,

@@ -968,9 +976,7 @@ static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
 static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
 	.attach = cdns_dsi_bridge_attach,
 	.mode_valid = cdns_dsi_bridge_mode_valid,
-	.atomic_disable = cdns_dsi_bridge_atomic_disable,
 	.atomic_pre_enable = cdns_dsi_bridge_atomic_pre_enable,
-	.atomic_enable = cdns_dsi_bridge_atomic_enable,
 	.atomic_post_disable = cdns_dsi_bridge_atomic_post_disable,
 	.atomic_check = cdns_dsi_bridge_atomic_check,
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,

Signature · 1 line

2.34.1

Hi Dmitry,

Thank you for reviewing the patches!

Quoted 21 lines · Dmitry Baryshkov

Quoted 12 lines · Aradhya Bhatia

From: Aradhya Bhatia <[email protected]>

The cdns-dsi controller requires that it be turned on completely before the input DPI's source has begun streaming[0]. Not having that, allows for a small window before cdns-dsi enable and after cdns-dsi disable where the previous entity (in this case tidss's videoport) to continue streaming DPI video signals. This small window where cdns-dsi is disabled but is still receiving signals causes the input FIFO of cdns-dsi to get corrupted. This causes the colors to shift on the output display. The colors can either shift by one color component (R->G, G->B, B->R), or by two color components (R->B, G->R, B->G).

Quoted 7 lines

Since tidss's videoport starts streaming via crtc enable hooks, we need cdns-dsi to be up and running before that. Now that the bridges are pre_enabled before crtc is enabled, and post_disabled after crtc is disabled, use the pre_enable and post_disable hooks to get cdns-dsi ready and running before the tidss videoport to get pass the color shift issues.

Quoted 3 lines

Not being an expert in the TI DSS driver, would it be more proper to handle that in the TI driver instead? I mean, sending out DPI signals isn't a part of the CRTC setup, it's a job of the encoder.

I haven't done a feasibility analysis of moving the CRTC bits of TIDSS into the encoder, but even if it were possible, it wouldn't solve the issue.

The bridge_enable() sequence gets called _after_ the encoder has been enabled - causing the TIDSS's DPI (enabled from encoder) to still be up and running before the DSI has had a chance to be ready.

Quoted 35 lines · Aradhya Bhatia

Hi Dmitry,

Thank you for reviewing the patches!

Quoted 21 lines · Dmitry Baryshkov

Quoted 12 lines · Aradhya Bhatia

From: Aradhya Bhatia <[email protected]>

The cdns-dsi controller requires that it be turned on completely before the input DPI's source has begun streaming[0]. Not having that, allows for a small window before cdns-dsi enable and after cdns-dsi disable where the previous entity (in this case tidss's videoport) to continue streaming DPI video signals. This small window where cdns-dsi is disabled but is still receiving signals causes the input FIFO of cdns-dsi to get corrupted. This causes the colors to shift on the output display. The colors can either shift by one color component (R->G, G->B, B->R), or by two color components (R->B, G->R, B->G).

Quoted 7 lines

Since tidss's videoport starts streaming via crtc enable hooks, we need cdns-dsi to be up and running before that. Now that the bridges are pre_enabled before crtc is enabled, and post_disabled after crtc is disabled, use the pre_enable and post_disable hooks to get cdns-dsi ready and running before the tidss videoport to get pass the color shift issues.

Quoted 3 lines

Not being an expert in the TI DSS driver, would it be more proper to handle that in the TI driver instead? I mean, sending out DPI signals isn't a part of the CRTC setup, it's a job of the encoder.

I haven't done a feasibility analysis of moving the CRTC bits of TIDSS into the encoder, but even if it were possible, it wouldn't solve the issue.

Quoted 3 lines

The bridge_enable() sequence gets called _after_ the encoder has been enabled - causing the TIDSS's DPI (enabled from encoder) to still be up and running before the DSI has had a chance to be ready.

But then you can move CDSI setup to pre_enable, so that all setup happens before encoder's (= DPI) being enabled.