[PATCH 1/9] drm/i915/mst: Debug log connector name in destroy_connector()

[PATCH 1/9] drm/i915/mst: Debug log connector name in destroy_connector()

[Dhinakaran Pandiyan]

Print connector name in destroy_connect() and this doesn't add any extra
lines to dmesg. The debug macro has been moved before the unregister
call so that we don't lose the connector name and id.

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 8e3aad0ea60b..88d1d2b9ac56 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -494,6 +494,7 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 	struct intel_connector *intel_connector = to_intel_connector(connector);
 	struct drm_i915_private *dev_priv = to_i915(connector->dev);
 
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, connector->name);
 	drm_connector_unregister(connector);
 
 	if (dev_priv->fbdev)
@@ -505,7 +506,6 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 	drm_modeset_unlock(&connector->dev->mode_config.connection_mutex);
 
 	drm_connector_unreference(connector);
-	DRM_DEBUG_KMS("\n");
 }
 
 static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/9] drm/i915/mst: Print active mst links after update

[Dhinakaran Pandiyan]

Both mst_disable_dp and mst_post_disable_dp print number of active links
before the variable has been updated. Move the print statement in
mst_disable_dp after the decrement so that the printed values indicate
the disabing of a mst connector. Also, add some text to clarify what we
are printing.

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 88d1d2b9ac56..9a396f483f8b 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -133,7 +133,7 @@ static void intel_mst_disable_dp(struct intel_encoder *encoder,
 		to_intel_connector(old_conn_state->connector);
 	int ret;
 
-	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
+	DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
 
 	drm_dp_mst_reset_vcpi_slots(&intel_dp->mst_mgr, connector->port);
 
@@ -155,8 +155,6 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
 	struct intel_connector *connector =
 		to_intel_connector(old_conn_state->connector);
 
-	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
-
 	/* this can fail */
 	drm_dp_check_act_status(&intel_dp->mst_mgr);
 	/* and this can also fail */
@@ -173,6 +171,7 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
 
 		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
 	}
+	DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
 }
 
 static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
@@ -195,7 +194,7 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
 	connector->encoder = encoder;
 	intel_mst->connector = connector;
 
-	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
+	DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
 
 	if (intel_dp->active_mst_links == 0)
 		intel_dig_port->base.pre_enable(&intel_dig_port->base,
@@ -229,7 +228,7 @@ static void intel_mst_enable_dp(struct intel_encoder *encoder,
 	enum port port = intel_dig_port->port;
 	int ret;
 
-	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
+	DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
 
 	if (intel_wait_for_register(dev_priv,
 				    DP_TP_STATUS(port),
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/9] drm/i915/dp: Fix buffer size for sink_irq_esi read

[Dhinakaran Pandiyan]

The buffer size defined is 16 bytes whereas only 14 bytes are read. Add a
macro to avoid this discrepancy.

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 887953c0f495..98e7b96ca826 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -42,6 +42,7 @@
 #include "i915_drv.h"
 
 #define DP_LINK_CHECK_TIMEOUT	(10 * 1000)
+#define DP_DPRX_ESI_LEN 14
 
 /* Compliance test status bits  */
 #define INTEL_DP_RESOLUTION_SHIFT_MASK	0
@@ -3991,15 +3992,9 @@ intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
 static bool
 intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
 {
-	int ret;
-
-	ret = drm_dp_dpcd_read(&intel_dp->aux,
-					     DP_SINK_COUNT_ESI,
-					     sink_irq_vector, 14);
-	if (ret != 14)
-		return false;
-
-	return true;
+	return drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_COUNT_ESI,
+				sink_irq_vector, DP_DPRX_ESI_LEN) ==
+		DP_DPRX_ESI_LEN;
 }
 
 static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
@@ -4199,7 +4194,7 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
 	bool bret;
 
 	if (intel_dp->is_mst) {
-		u8 esi[16] = { 0 };
+		u8 esi[DP_DPRX_ESI_LEN] = { 0 };
 		int ret = 0;
 		int retry;
 		bool handled;
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/9] drm/i915/dp: Avoid more dpcd transactions after resume failure

[Dhinakaran Pandiyan]

drm_dp_mst_topology_mgr_resume() fails if there are dpcd failures, so
there's no need to try that again in _check_mst_status()

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 98e7b96ca826..b3d95cb7546b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -6224,12 +6224,16 @@ void intel_dp_mst_resume(struct drm_device *dev)
 	for (i = 0; i < I915_MAX_PORTS; i++) {
 		struct intel_digital_port *intel_dig_port = dev_priv->hotplug.irq_port[i];
 		int ret;
+		struct intel_dp *intel_dp = &intel_dig_port->dp;
 
-		if (!intel_dig_port || !intel_dig_port->dp.can_mst)
+		if (!intel_dig_port || !intel_dp->can_mst)
 			continue;
 
-		ret = drm_dp_mst_topology_mgr_resume(&intel_dig_port->dp.mst_mgr);
-		if (ret)
-			intel_dp_check_mst_status(&intel_dig_port->dp);
+		ret = drm_dp_mst_topology_mgr_resume(&intel_dp->mst_mgr);
+		if (ret) {
+			intel_dp->is_mst = false;
+			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
+			drm_kms_helper_hotplug_event(dev);
+		}
 	}
 }
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/9] drm/i915/dp: Remove intel_dp->is_mst check in intel_dp_check_mst_status

[Dhinakaran Pandiyan]

There is just only one caller now, which already checks for
intel_dp->is_mst. So, remove this and fix some braces while at it.

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 85 ++++++++++++++++++++---------------------
 1 file changed, 42 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b3d95cb7546b..3f2ca10ccbcd 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4192,57 +4192,56 @@ static int
 intel_dp_check_mst_status(struct intel_dp *intel_dp)
 {
 	bool bret;
+	u8 esi[DP_DPRX_ESI_LEN] = { 0 };
+	int ret = 0;
+	int retry;
+	bool handled;
 
-	if (intel_dp->is_mst) {
-		u8 esi[DP_DPRX_ESI_LEN] = { 0 };
-		int ret = 0;
-		int retry;
-		bool handled;
-		bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
+	bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
 go_again:
-		if (bret == true) {
-
-			/* check link status - esi[10] = 0x200c */
-			if (intel_dp->active_mst_links &&
-			    !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
-				DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
-				intel_dp_start_link_train(intel_dp);
-				intel_dp_stop_link_train(intel_dp);
-			}
+	if (bret == true) {
+
+		/* check link status - esi[10] = 0x200c */
+		if (intel_dp->active_mst_links &&
+		    !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
+			DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
+			intel_dp_start_link_train(intel_dp);
+			intel_dp_stop_link_train(intel_dp);
+		}
 
-			DRM_DEBUG_KMS("got esi %3ph\n", esi);
-			ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
-
-			if (handled) {
-				for (retry = 0; retry < 3; retry++) {
-					int wret;
-					wret = drm_dp_dpcd_write(&intel_dp->aux,
-								 DP_SINK_COUNT_ESI+1,
-								 &esi[1], 3);
-					if (wret == 3) {
-						break;
-					}
-				}
+		DRM_DEBUG_KMS("got esi %3ph\n", esi);
+		ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
 
-				bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
-				if (bret == true) {
-					DRM_DEBUG_KMS("got esi2 %3ph\n", esi);
-					goto go_again;
-				}
-			} else
-				ret = 0;
+		if (handled) {
+			for (retry = 0; retry < 3; retry++) {
+				int wret;
 
-			return ret;
+				wret = drm_dp_dpcd_write(&intel_dp->aux,
+							 DP_SINK_COUNT_ESI+1,
+							 &esi[1], 3);
+				if (wret == 3)
+					break;
+			}
+
+			bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
+			if (bret == true) {
+				DRM_DEBUG_KMS("got esi2 %3ph\n", esi);
+				goto go_again;
+			}
 		} else {
-			struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-			DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
-			intel_dp->is_mst = false;
-			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
-			/* send a hotplug event */
-			drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
+			ret = 0;
 		}
+	} else {
+		struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+
+		DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
+		intel_dp->is_mst = false;
+		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
+						intel_dp->is_mst);
+		drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
+		ret = -EINVAL;
 	}
-	return -EINVAL;
+	return ret;
 }
 
 static void
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/9] drm/i915/dp: Handle check_mst_status() failure in one place

[Dhinakaran Pandiyan]

The caller already has code to handle failure, no need to duplicate
that.

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 3f2ca10ccbcd..2886a2ef1591 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4232,13 +4232,7 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
 			ret = 0;
 		}
 	} else {
-		struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-
 		DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
-		intel_dp->is_mst = false;
-		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
-						intel_dp->is_mst);
-		drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
 		ret = -EINVAL;
 	}
 	return ret;
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 7/9] drm/i915/dp: Avoid double-printing esi regs

[Dhinakaran Pandiyan]

We debug log the esi dpcd's twice after reading them back, which is not
necessary.

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 2886a2ef1591..aab9ba31f79e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4224,10 +4224,8 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
 			}
 
 			bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
-			if (bret == true) {
-				DRM_DEBUG_KMS("got esi2 %3ph\n", esi);
+			if (bret == true)
 				goto go_again;
-			}
 		} else {
 			ret = 0;
 		}
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 8/9] drm/i915/dp: Protect link training with connection mutex

[Dhinakaran Pandiyan]

The other instances of link training are protected with
connection_mutex, so do the same in check_mst_status() too.

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index aab9ba31f79e..644463ba313e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4191,6 +4191,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
 static int
 intel_dp_check_mst_status(struct intel_dp *intel_dp)
 {
+	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	bool bret;
 	u8 esi[DP_DPRX_ESI_LEN] = { 0 };
 	int ret = 0;
@@ -4205,8 +4206,11 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
 		if (intel_dp->active_mst_links &&
 		    !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
 			DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
+
+			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
 			intel_dp_start_link_train(intel_dp);
 			intel_dp_stop_link_train(intel_dp);
+			drm_modeset_unlock(&dev->mode_config.connection_mutex);
 		}
 
 		DRM_DEBUG_KMS("got esi %3ph\n", esi);
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 9/9] drm/i915/dp: Remove redundant can_mst checks in intel_dp_configure_mst()

[Dhinakaran Pandiyan]

intel_dp_can_mst() performs these checks.

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 644463ba313e..a4465b46bb27 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3845,12 +3845,6 @@ intel_dp_can_mst(struct intel_dp *intel_dp)
 static void
 intel_dp_configure_mst(struct intel_dp *intel_dp)
 {
-	if (!i915.enable_dp_mst)
-		return;
-
-	if (!intel_dp->can_mst)
-		return;
-
 	intel_dp->is_mst = intel_dp_can_mst(intel_dp);
 
 	if (intel_dp->is_mst)
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [1/9] drm/i915/mst: Debug log connector name in destroy_connector()

[Patchwork]

== Series Details ==

Series: series starting with [1/9] drm/i915/mst: Debug log connector name in destroy_connector()
URL   : https://patchwork.freedesktop.org/series/30250/
State : failure

== Summary ==

Series 30250v1 series starting with [1/9] drm/i915/mst: Debug log connector name in destroy_connector()
https://patchwork.freedesktop.org/api/1.0/series/30250/revisions/1/mbox/

Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-atomic:
                fail       -> PASS       (fi-snb-2600) fdo#100215
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                pass       -> FAIL       (fi-kbl-r)

fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:447s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:457s
fi-blb-e6850     total:289  pass:224  dwarn:1   dfail:0   fail:0   skip:64  time:375s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:537s
fi-bwr-2160      total:289  pass:184  dwarn:0   dfail:0   fail:0   skip:105 time:269s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:478s
fi-byt-j1900     total:289  pass:254  dwarn:1   dfail:0   fail:0   skip:34  time:502s
fi-byt-n2820     total:289  pass:250  dwarn:1   dfail:0   fail:0   skip:38  time:495s
fi-cfl-s         total:289  pass:250  dwarn:4   dfail:0   fail:0   skip:35  time:448s
fi-elk-e7500     total:289  pass:230  dwarn:0   dfail:0   fail:0   skip:59  time:456s
fi-glk-2a        total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:566s
fi-hsw-4770r     total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:411s
fi-ilk-650       total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:426s
fi-ivb-3520m     total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:489s
fi-ivb-3770      total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:464s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:487s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:563s
fi-kbl-r         total:289  pass:261  dwarn:0   dfail:0   fail:1   skip:27  time:552s
fi-pnv-d510      total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:548s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:460s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:782s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:503s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:467s
fi-skl-x1585l    total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:477s
fi-snb-2520m     total:289  pass:251  dwarn:0   dfail:0   fail:0   skip:38  time:572s
fi-snb-2600      total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:421s

c9e787e0e616ba39a2ef195cb24d2e79b628bb9a drm-tip: 2017y-09m-12d-20h-02m-24s UTC integration manifest
855dacf6da80 drm/i915/dp: Remove redundant can_mst checks in intel_dp_configure_mst()
2b0def9797ca drm/i915/dp: Protect link training with connection mutex
53df94b117ab drm/i915/dp: Avoid double-printing esi regs
ab2e0660d53c drm/i915/dp: Handle check_mst_status() failure in one place
42b6914d0a96 drm/i915/dp: Remove intel_dp->is_mst check in intel_dp_check_mst_status
477a2fb3b949 drm/i915/dp: Avoid more dpcd transactions after resume failure
c11032688bb7 drm/i915/dp: Fix buffer size for sink_irq_esi read
3c930225c7cf drm/i915/mst: Print active mst links after update
c42ee9091c0c drm/i915/mst: Debug log connector name in destroy_connector()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5670/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/9] drm/i915/dp: Remove intel_dp->is_mst check in intel_dp_check_mst_status

[Jani Nikula]

On Tue, 12 Sep 2017, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote:
> There is just only one caller now, which already checks for
> intel_dp->is_mst. So, remove this and fix some braces while at it.
>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 85 ++++++++++++++++++++---------------------
>  1 file changed, 42 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index b3d95cb7546b..3f2ca10ccbcd 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4192,57 +4192,56 @@ static int
>  intel_dp_check_mst_status(struct intel_dp *intel_dp)
>  {
>  	bool bret;
> +	u8 esi[DP_DPRX_ESI_LEN] = { 0 };
> +	int ret = 0;
> +	int retry;
> +	bool handled;
>  
> -	if (intel_dp->is_mst) {
> -		u8 esi[DP_DPRX_ESI_LEN] = { 0 };
> -		int ret = 0;
> -		int retry;
> -		bool handled;
> -		bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
> +	bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
>  go_again:
> -		if (bret == true) {
> -
> -			/* check link status - esi[10] = 0x200c */

Drive-by observation unrelated to this patch: 0x200c isn't even defined
in drm_dp_helper.h... care to add that, please?

BR,
Jani.

> -			if (intel_dp->active_mst_links &&
> -			    !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
> -				DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
> -				intel_dp_start_link_train(intel_dp);
> -				intel_dp_stop_link_train(intel_dp);
> -			}
> +	if (bret == true) {
> +
> +		/* check link status - esi[10] = 0x200c */
> +		if (intel_dp->active_mst_links &&
> +		    !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
> +			DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
> +			intel_dp_start_link_train(intel_dp);
> +			intel_dp_stop_link_train(intel_dp);
> +		}
>  
> -			DRM_DEBUG_KMS("got esi %3ph\n", esi);
> -			ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
> -
> -			if (handled) {
> -				for (retry = 0; retry < 3; retry++) {
> -					int wret;
> -					wret = drm_dp_dpcd_write(&intel_dp->aux,
> -								 DP_SINK_COUNT_ESI+1,
> -								 &esi[1], 3);
> -					if (wret == 3) {
> -						break;
> -					}
> -				}
> +		DRM_DEBUG_KMS("got esi %3ph\n", esi);
> +		ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
>  
> -				bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
> -				if (bret == true) {
> -					DRM_DEBUG_KMS("got esi2 %3ph\n", esi);
> -					goto go_again;
> -				}
> -			} else
> -				ret = 0;
> +		if (handled) {
> +			for (retry = 0; retry < 3; retry++) {
> +				int wret;
>  
> -			return ret;
> +				wret = drm_dp_dpcd_write(&intel_dp->aux,
> +							 DP_SINK_COUNT_ESI+1,
> +							 &esi[1], 3);
> +				if (wret == 3)
> +					break;
> +			}
> +
> +			bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
> +			if (bret == true) {
> +				DRM_DEBUG_KMS("got esi2 %3ph\n", esi);
> +				goto go_again;
> +			}
>  		} else {
> -			struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> -			DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
> -			intel_dp->is_mst = false;
> -			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
> -			/* send a hotplug event */
> -			drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
> +			ret = 0;
>  		}
> +	} else {
> +		struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +
> +		DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
> +		intel_dp->is_mst = false;
> +		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> +						intel_dp->is_mst);
> +		drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
> +		ret = -EINVAL;
>  	}
> -	return -EINVAL;
> +	return ret;
>  }
>  
>  static void

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/9] drm/i915/dp: Avoid more dpcd transactions after resume failure

[Ville Syrjälä]

On Tue, Sep 12, 2017 at 04:57:25PM -0700, Dhinakaran Pandiyan wrote:
> drm_dp_mst_topology_mgr_resume() fails if there are dpcd failures, so
> there's no need to try that again in _check_mst_status()

That commit message somehow doesn't seem to match this patch. You're not
removing anything from _check_mst_status() here.

> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 98e7b96ca826..b3d95cb7546b 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -6224,12 +6224,16 @@ void intel_dp_mst_resume(struct drm_device *dev)
>  	for (i = 0; i < I915_MAX_PORTS; i++) {
>  		struct intel_digital_port *intel_dig_port = dev_priv->hotplug.irq_port[i];
>  		int ret;
> +		struct intel_dp *intel_dp = &intel_dig_port->dp;
>  
> -		if (!intel_dig_port || !intel_dig_port->dp.can_mst)
> +		if (!intel_dig_port || !intel_dp->can_mst)
>  			continue;
>  
> -		ret = drm_dp_mst_topology_mgr_resume(&intel_dig_port->dp.mst_mgr);
> -		if (ret)
> -			intel_dp_check_mst_status(&intel_dig_port->dp);
> +		ret = drm_dp_mst_topology_mgr_resume(&intel_dp->mst_mgr);
> +		if (ret) {
> +			intel_dp->is_mst = false;
> +			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
> +			drm_kms_helper_hotplug_event(dev);
> +		}
>  	}
>  }
> -- 
> 2.11.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/9] drm/i915/dp: Remove intel_dp->is_mst check in intel_dp_check_mst_status

[Ville Syrjälä]

On Tue, Sep 12, 2017 at 04:57:26PM -0700, Dhinakaran Pandiyan wrote:
> There is just only one caller now, which already checks for
> intel_dp->is_mst. So, remove this and fix some braces while at it.

Hmm. So this depends on the subtle detail that
drm_dp_mst_topology_mgr_resume() will "fail" if didn't enable mst
prior to calling it. That fact isn't obvious at all, so I would
suggest noting that in the commit message at least (maybe we should
even have a comment in the code?)

I'm assuming there are not other changes beyond just dropping the
top level 'if (mst) { }' thing? This is one reason I prefer flat code.
If the function had been flat to begin with the diff would have
actually turned out legible. IIRC I even posted a flattening patch at
some point but I guess it didn't go in. I would suggest that you take
the opportunity to flatten out the rest of this function while you
in the neighbourhood.

> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 85 ++++++++++++++++++++---------------------
>  1 file changed, 42 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index b3d95cb7546b..3f2ca10ccbcd 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4192,57 +4192,56 @@ static int
>  intel_dp_check_mst_status(struct intel_dp *intel_dp)
>  {
>  	bool bret;
> +	u8 esi[DP_DPRX_ESI_LEN] = { 0 };
> +	int ret = 0;
> +	int retry;
> +	bool handled;
>  
> -	if (intel_dp->is_mst) {
> -		u8 esi[DP_DPRX_ESI_LEN] = { 0 };
> -		int ret = 0;
> -		int retry;
> -		bool handled;
> -		bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
> +	bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
>  go_again:
> -		if (bret == true) {
> -
> -			/* check link status - esi[10] = 0x200c */
> -			if (intel_dp->active_mst_links &&
> -			    !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
> -				DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
> -				intel_dp_start_link_train(intel_dp);
> -				intel_dp_stop_link_train(intel_dp);
> -			}
> +	if (bret == true) {
> +
> +		/* check link status - esi[10] = 0x200c */
> +		if (intel_dp->active_mst_links &&
> +		    !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
> +			DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
> +			intel_dp_start_link_train(intel_dp);
> +			intel_dp_stop_link_train(intel_dp);
> +		}
>  
> -			DRM_DEBUG_KMS("got esi %3ph\n", esi);
> -			ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
> -
> -			if (handled) {
> -				for (retry = 0; retry < 3; retry++) {
> -					int wret;
> -					wret = drm_dp_dpcd_write(&intel_dp->aux,
> -								 DP_SINK_COUNT_ESI+1,
> -								 &esi[1], 3);
> -					if (wret == 3) {
> -						break;
> -					}
> -				}
> +		DRM_DEBUG_KMS("got esi %3ph\n", esi);
> +		ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
>  
> -				bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
> -				if (bret == true) {
> -					DRM_DEBUG_KMS("got esi2 %3ph\n", esi);
> -					goto go_again;
> -				}
> -			} else
> -				ret = 0;
> +		if (handled) {
> +			for (retry = 0; retry < 3; retry++) {
> +				int wret;
>  
> -			return ret;
> +				wret = drm_dp_dpcd_write(&intel_dp->aux,
> +							 DP_SINK_COUNT_ESI+1,
> +							 &esi[1], 3);
> +				if (wret == 3)
> +					break;
> +			}
> +
> +			bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
> +			if (bret == true) {
> +				DRM_DEBUG_KMS("got esi2 %3ph\n", esi);
> +				goto go_again;
> +			}
>  		} else {
> -			struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> -			DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
> -			intel_dp->is_mst = false;
> -			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
> -			/* send a hotplug event */
> -			drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
> +			ret = 0;
>  		}
> +	} else {
> +		struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +
> +		DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
> +		intel_dp->is_mst = false;
> +		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> +						intel_dp->is_mst);
> +		drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
> +		ret = -EINVAL;
>  	}
> -	return -EINVAL;
> +	return ret;
>  }
>  
>  static void
> -- 
> 2.11.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/9] drm/i915/dp: Remove intel_dp->is_mst check in intel_dp_check_mst_status

[Pandiyan, Dhinakaran]

On Wed, 2017-09-13 at 12:32 +0300, Jani Nikula wrote:
> On Tue, 12 Sep 2017, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote:
> > There is just only one caller now, which already checks for
> > intel_dp->is_mst. So, remove this and fix some braces while at it.
> >
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 85 ++++++++++++++++++++---------------------
> >  1 file changed, 42 insertions(+), 43 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index b3d95cb7546b..3f2ca10ccbcd 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4192,57 +4192,56 @@ static int
> >  intel_dp_check_mst_status(struct intel_dp *intel_dp)
> >  {
> >  	bool bret;
> > +	u8 esi[DP_DPRX_ESI_LEN] = { 0 };
> > +	int ret = 0;
> > +	int retry;
> > +	bool handled;
> >  
> > -	if (intel_dp->is_mst) {
> > -		u8 esi[DP_DPRX_ESI_LEN] = { 0 };
> > -		int ret = 0;
> > -		int retry;
> > -		bool handled;
> > -		bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
> > +	bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
> >  go_again:
> > -		if (bret == true) {
> > -
> > -			/* check link status - esi[10] = 0x200c */
> 
> Drive-by observation unrelated to this patch: 0x200c isn't even defined
> in drm_dp_helper.h... care to add that, please?

Sure.


> 
> BR,
> Jani.
> 
> > -			if (intel_dp->active_mst_links &&
> > -			    !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
> > -				DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
> > -				intel_dp_start_link_train(intel_dp);
> > -				intel_dp_stop_link_train(intel_dp);
> > -			}
> > +	if (bret == true) {
> > +
> > +		/* check link status - esi[10] = 0x200c */
> > +		if (intel_dp->active_mst_links &&
> > +		    !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
> > +			DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
> > +			intel_dp_start_link_train(intel_dp);
> > +			intel_dp_stop_link_train(intel_dp);
> > +		}
> >  
> > -			DRM_DEBUG_KMS("got esi %3ph\n", esi);
> > -			ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
> > -
> > -			if (handled) {
> > -				for (retry = 0; retry < 3; retry++) {
> > -					int wret;
> > -					wret = drm_dp_dpcd_write(&intel_dp->aux,
> > -								 DP_SINK_COUNT_ESI+1,
> > -								 &esi[1], 3);
> > -					if (wret == 3) {
> > -						break;
> > -					}
> > -				}
> > +		DRM_DEBUG_KMS("got esi %3ph\n", esi);
> > +		ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
> >  
> > -				bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
> > -				if (bret == true) {
> > -					DRM_DEBUG_KMS("got esi2 %3ph\n", esi);
> > -					goto go_again;
> > -				}
> > -			} else
> > -				ret = 0;
> > +		if (handled) {
> > +			for (retry = 0; retry < 3; retry++) {
> > +				int wret;
> >  
> > -			return ret;
> > +				wret = drm_dp_dpcd_write(&intel_dp->aux,
> > +							 DP_SINK_COUNT_ESI+1,
> > +							 &esi[1], 3);
> > +				if (wret == 3)
> > +					break;
> > +			}
> > +
> > +			bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
> > +			if (bret == true) {
> > +				DRM_DEBUG_KMS("got esi2 %3ph\n", esi);
> > +				goto go_again;
> > +			}
> >  		} else {
> > -			struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > -			DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
> > -			intel_dp->is_mst = false;
> > -			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
> > -			/* send a hotplug event */
> > -			drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
> > +			ret = 0;
> >  		}
> > +	} else {
> > +		struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > +
> > +		DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
> > +		intel_dp->is_mst = false;
> > +		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> > +						intel_dp->is_mst);
> > +		drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
> > +		ret = -EINVAL;
> >  	}
> > -	return -EINVAL;
> > +	return ret;
> >  }
> >  
> >  static void
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/9] drm/i915/dp: Avoid more dpcd transactions after resume failure

[Pandiyan, Dhinakaran]

On Wed, 2017-09-13 at 16:24 +0300, Ville Syrjälä wrote:
> On Tue, Sep 12, 2017 at 04:57:25PM -0700, Dhinakaran Pandiyan wrote:
> > drm_dp_mst_topology_mgr_resume() fails if there are dpcd failures, so
> > there's no need to try that again in _check_mst_status()
> 
> That commit message somehow doesn't seem to match this patch. You're not
> removing anything from _check_mst_status() here.
> 
You are right, I meant to write "there's no need to call
_check_mst_status to try dpcd reads again"

Is that good?


> > 
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 98e7b96ca826..b3d95cb7546b 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -6224,12 +6224,16 @@ void intel_dp_mst_resume(struct drm_device *dev)
> >  	for (i = 0; i < I915_MAX_PORTS; i++) {
> >  		struct intel_digital_port *intel_dig_port = dev_priv->hotplug.irq_port[i];
> >  		int ret;
> > +		struct intel_dp *intel_dp = &intel_dig_port->dp;
> >  
> > -		if (!intel_dig_port || !intel_dig_port->dp.can_mst)
> > +		if (!intel_dig_port || !intel_dp->can_mst)
> >  			continue;
> >  
> > -		ret = drm_dp_mst_topology_mgr_resume(&intel_dig_port->dp.mst_mgr);
> > -		if (ret)
> > -			intel_dp_check_mst_status(&intel_dig_port->dp);
> > +		ret = drm_dp_mst_topology_mgr_resume(&intel_dp->mst_mgr);
> > +		if (ret) {
> > +			intel_dp->is_mst = false;
> > +			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
> > +			drm_kms_helper_hotplug_event(dev);
> > +		}
> >  	}
> >  }
> > -- 
> > 2.11.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/9] drm/i915/dp: Remove intel_dp->is_mst check in intel_dp_check_mst_status

[Pandiyan, Dhinakaran]

On Wed, 2017-09-13 at 16:31 +0300, Ville Syrjälä wrote:
> On Tue, Sep 12, 2017 at 04:57:26PM -0700, Dhinakaran Pandiyan wrote:
> > There is just only one caller now, which already checks for
> > intel_dp->is_mst. So, remove this and fix some braces while at it.
> 
> Hmm. So this depends on the subtle detail that
> drm_dp_mst_topology_mgr_resume() will "fail" if didn't enable mst
> prior to calling it. That fact isn't obvious at all, so I would
> suggest noting that in the commit message at least (maybe we should
> even have a comment in the code?)
> 
> I'm assuming there are not other changes beyond just dropping the
> top level 'if (mst) { }' thing? This is one reason I prefer flat code.
> If the function had been flat to begin with the diff would have
> actually turned out legible. IIRC I even posted a flattening patch at
> some point but I guess it didn't go in. I would suggest that you take
> the opportunity to flatten out the rest of this function while you
> in the neighbourhood.

That was the goal :) Then I decided to simplify the function a little
bit in patches 4, 5, 6 and 7 before working on modifying it's structure.

> 
> > 
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 85 ++++++++++++++++++++---------------------
> >  1 file changed, 42 insertions(+), 43 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index b3d95cb7546b..3f2ca10ccbcd 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4192,57 +4192,56 @@ static int
> >  intel_dp_check_mst_status(struct intel_dp *intel_dp)
> >  {
> >  	bool bret;
> > +	u8 esi[DP_DPRX_ESI_LEN] = { 0 };
> > +	int ret = 0;
> > +	int retry;
> > +	bool handled;
> >  
> > -	if (intel_dp->is_mst) {
> > -		u8 esi[DP_DPRX_ESI_LEN] = { 0 };
> > -		int ret = 0;
> > -		int retry;
> > -		bool handled;
> > -		bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
> > +	bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
> >  go_again:
> > -		if (bret == true) {
> > -
> > -			/* check link status - esi[10] = 0x200c */
> > -			if (intel_dp->active_mst_links &&
> > -			    !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
> > -				DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
> > -				intel_dp_start_link_train(intel_dp);
> > -				intel_dp_stop_link_train(intel_dp);
> > -			}
> > +	if (bret == true) {
> > +
> > +		/* check link status - esi[10] = 0x200c */
> > +		if (intel_dp->active_mst_links &&
> > +		    !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
> > +			DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
> > +			intel_dp_start_link_train(intel_dp);
> > +			intel_dp_stop_link_train(intel_dp);
> > +		}
> >  
> > -			DRM_DEBUG_KMS("got esi %3ph\n", esi);
> > -			ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
> > -
> > -			if (handled) {
> > -				for (retry = 0; retry < 3; retry++) {
> > -					int wret;
> > -					wret = drm_dp_dpcd_write(&intel_dp->aux,
> > -								 DP_SINK_COUNT_ESI+1,
> > -								 &esi[1], 3);
> > -					if (wret == 3) {
> > -						break;
> > -					}
> > -				}
> > +		DRM_DEBUG_KMS("got esi %3ph\n", esi);
> > +		ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
> >  
> > -				bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
> > -				if (bret == true) {
> > -					DRM_DEBUG_KMS("got esi2 %3ph\n", esi);
> > -					goto go_again;
> > -				}
> > -			} else
> > -				ret = 0;
> > +		if (handled) {
> > +			for (retry = 0; retry < 3; retry++) {
> > +				int wret;
> >  
> > -			return ret;
> > +				wret = drm_dp_dpcd_write(&intel_dp->aux,
> > +							 DP_SINK_COUNT_ESI+1,
> > +							 &esi[1], 3);
> > +				if (wret == 3)
> > +					break;
> > +			}
> > +
> > +			bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
> > +			if (bret == true) {
> > +				DRM_DEBUG_KMS("got esi2 %3ph\n", esi);
> > +				goto go_again;
> > +			}
> >  		} else {
> > -			struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > -			DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
> > -			intel_dp->is_mst = false;
> > -			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
> > -			/* send a hotplug event */
> > -			drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
> > +			ret = 0;
> >  		}
> > +	} else {
> > +		struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > +
> > +		DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
> > +		intel_dp->is_mst = false;
> > +		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> > +						intel_dp->is_mst);
> > +		drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
> > +		ret = -EINVAL;
> >  	}
> > -	return -EINVAL;
> > +	return ret;
> >  }
> >  
> >  static void
> > -- 
> > 2.11.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/9] drm/i915/mst: Debug log connector name in destroy_connector()

[Ausmus, James]

On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan
<dhinakaran.pandiyan@intel.com> wrote:
> Print connector name in destroy_connect() and this doesn't add any extra
> lines to dmesg. The debug macro has been moved before the unregister
> call so that we don't lose the connector name and id.
>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

Reviewed-by: James Ausmus <james.ausmus@intel.com>

While I was looking around here, though, I noticed we don't currently
handle error returns from the drm layer calls in
intel_dp_add_mst_connector. I'll try to send a patch along tomorrow
for that.

> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 8e3aad0ea60b..88d1d2b9ac56 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -494,6 +494,7 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
>         struct intel_connector *intel_connector = to_intel_connector(connector);
>         struct drm_i915_private *dev_priv = to_i915(connector->dev);
>
> +       DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, connector->name);
>         drm_connector_unregister(connector);
>
>         if (dev_priv->fbdev)
> @@ -505,7 +506,6 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
>         drm_modeset_unlock(&connector->dev->mode_config.connection_mutex);
>
>         drm_connector_unreference(connector);
> -       DRM_DEBUG_KMS("\n");
>  }
>
>  static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
> --
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 


James Ausmus
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/9] drm/i915/mst: Debug log connector name in destroy_connector()

[Pandiyan, Dhinakaran]

On Wed, 2017-09-13 at 17:20 -0700, Ausmus, James wrote:
> On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan
> <dhinakaran.pandiyan@intel.com> wrote:
> > Print connector name in destroy_connect() and this doesn't add any extra
> > lines to dmesg. The debug macro has been moved before the unregister
> > call so that we don't lose the connector name and id.
> >
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> 
> Reviewed-by: James Ausmus <james.ausmus@intel.com>
> 
> While I was looking around here, though, I noticed we don't currently
> handle error returns from the drm layer calls in
> intel_dp_add_mst_connector. I'll try to send a patch along tomorrow
> for that.
> 

Thanks for the review and yeah, that does look wrong.


> > ---
> >  drivers/gpu/drm/i915/intel_dp_mst.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index 8e3aad0ea60b..88d1d2b9ac56 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -494,6 +494,7 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
> >         struct intel_connector *intel_connector = to_intel_connector(connector);
> >         struct drm_i915_private *dev_priv = to_i915(connector->dev);
> >
> > +       DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, connector->name);
> >         drm_connector_unregister(connector);
> >
> >         if (dev_priv->fbdev)
> > @@ -505,7 +506,6 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
> >         drm_modeset_unlock(&connector->dev->mode_config.connection_mutex);
> >
> >         drm_connector_unreference(connector);
> > -       DRM_DEBUG_KMS("\n");
> >  }
> >
> >  static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
> > --
> > 2.11.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/9] drm/i915/dp: Avoid double-printing esi regs

[Ausmus, James]

On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan
<dhinakaran.pandiyan@intel.com> wrote:
> We debug log the esi dpcd's twice after reading them back, which is not
> necessary.
>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

While you're cleaning this up, why not move the do_again label beneath
the if - the only time the goto happens is immediately after you've
already checked if (bret == true), so no need to check again.

Either way -

Reviewed-by: James Ausmus <james.ausmus@intel.com>


> ---
>  drivers/gpu/drm/i915/intel_dp.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 2886a2ef1591..aab9ba31f79e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4224,10 +4224,8 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
>                         }
>
>                         bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
> -                       if (bret == true) {
> -                               DRM_DEBUG_KMS("got esi2 %3ph\n", esi);
> +                       if (bret == true)
>                                 goto go_again;
> -                       }
>                 } else {
>                         ret = 0;
>                 }
> --
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 


James Ausmus
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/9] drm/i915/mst: Print active mst links after update

[Ausmus, James]

On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan
<dhinakaran.pandiyan@intel.com> wrote:
> Both mst_disable_dp and mst_post_disable_dp print number of active links
> before the variable has been updated. Move the print statement in
> mst_disable_dp after the decrement so that the printed values indicate
> the disabing of a mst connector. Also, add some text to clarify what we
> are printing.
>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

Reviewed-by: James Ausmus <james.ausmus@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 88d1d2b9ac56..9a396f483f8b 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -133,7 +133,7 @@ static void intel_mst_disable_dp(struct intel_encoder *encoder,
>                 to_intel_connector(old_conn_state->connector);
>         int ret;
>
> -       DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> +       DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
>
>         drm_dp_mst_reset_vcpi_slots(&intel_dp->mst_mgr, connector->port);
>
> @@ -155,8 +155,6 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
>         struct intel_connector *connector =
>                 to_intel_connector(old_conn_state->connector);
>
> -       DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> -
>         /* this can fail */
>         drm_dp_check_act_status(&intel_dp->mst_mgr);
>         /* and this can also fail */
> @@ -173,6 +171,7 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
>
>                 intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
>         }
> +       DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
>  }
>
>  static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
> @@ -195,7 +194,7 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
>         connector->encoder = encoder;
>         intel_mst->connector = connector;
>
> -       DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> +       DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
>
>         if (intel_dp->active_mst_links == 0)
>                 intel_dig_port->base.pre_enable(&intel_dig_port->base,
> @@ -229,7 +228,7 @@ static void intel_mst_enable_dp(struct intel_encoder *encoder,
>         enum port port = intel_dig_port->port;
>         int ret;
>
> -       DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> +       DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
>
>         if (intel_wait_for_register(dev_priv,
>                                     DP_TP_STATUS(port),
> --
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 


James Ausmus
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/9] drm/i915/dp: Fix buffer size for sink_irq_esi read

[Ausmus, James]

On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan
<dhinakaran.pandiyan@intel.com> wrote:
> The buffer size defined is 16 bytes whereas only 14 bytes are read. Add a
> macro to avoid this discrepancy.
>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

Reviewed-by: James Ausmus <james.ausmus@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 887953c0f495..98e7b96ca826 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -42,6 +42,7 @@
>  #include "i915_drv.h"
>
>  #define DP_LINK_CHECK_TIMEOUT  (10 * 1000)
> +#define DP_DPRX_ESI_LEN 14
>
>  /* Compliance test status bits  */
>  #define INTEL_DP_RESOLUTION_SHIFT_MASK 0
> @@ -3991,15 +3992,9 @@ intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>  static bool
>  intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>  {
> -       int ret;
> -
> -       ret = drm_dp_dpcd_read(&intel_dp->aux,
> -                                            DP_SINK_COUNT_ESI,
> -                                            sink_irq_vector, 14);
> -       if (ret != 14)
> -               return false;
> -
> -       return true;
> +       return drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_COUNT_ESI,
> +                               sink_irq_vector, DP_DPRX_ESI_LEN) ==
> +               DP_DPRX_ESI_LEN;
>  }
>
>  static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
> @@ -4199,7 +4194,7 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
>         bool bret;
>
>         if (intel_dp->is_mst) {
> -               u8 esi[16] = { 0 };
> +               u8 esi[DP_DPRX_ESI_LEN] = { 0 };
>                 int ret = 0;
>                 int retry;
>                 bool handled;
> --
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 


James Ausmus
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/9] drm/i915/dp: Handle check_mst_status() failure in one place

[Ausmus, James]

On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan
<dhinakaran.pandiyan@intel.com> wrote:
> The caller already has code to handle failure, no need to duplicate
> that.
>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 6 ------
>  1 file changed, 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 3f2ca10ccbcd..2886a2ef1591 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4232,13 +4232,7 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
>                         ret = 0;
>                 }
>         } else {
> -               struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> -
>                 DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
> -               intel_dp->is_mst = false;
> -               drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> -                                               intel_dp->is_mst);
> -               drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);

It looks like intel_dp_hpd_pulse doesn't call
drm_kms_helper_hotplug_event on a failure in
intel_dp_check_mst_status, so we would lose that path with this patch
- do we need that?

>                 ret = -EINVAL;
>         }
>         return ret;
> --
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 


James Ausmus
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/9] drm/i915/dp: Handle check_mst_status() failure in one place

[Pandiyan, Dhinakaran]

On Thu, 2017-09-14 at 14:32 -0700, Ausmus, James wrote:
> On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan
> <dhinakaran.pandiyan@intel.com> wrote:
> > The caller already has code to handle failure, no need to duplicate
> > that.
> >
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 6 ------
> >  1 file changed, 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 3f2ca10ccbcd..2886a2ef1591 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4232,13 +4232,7 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
> >                         ret = 0;
> >                 }
> >         } else {
> > -               struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > -
> >                 DRM_DEBUG_KMS("failed to get ESI - device may have failed\n");
> > -               intel_dp->is_mst = false;
> > -               drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> > -                                               intel_dp->is_mst);
> > -               drm_kms_helper_hotplug_event(intel_dig_port->base.base.dev);
> 
> It looks like intel_dp_hpd_pulse doesn't call
> drm_kms_helper_hotplug_event on a failure in
> intel_dp_check_mst_status, so we would lose that path with this patch
> - do we need that?
> 

Because we return IRQ_NONE when that happens, intel_dp_detect() should
eventually get called from i915_digport_work_func(). Further, when the
connector status gets updated in
intel_dp_detect()->intel_dp_long_pulse(), a hotplug_event ends up being
sent.

But this patch only should be removing redundant code, not make alter
the code path. I'll fix that up. Thanks for catching this.



> >                 ret = -EINVAL;
> >         }
> >         return ret;
> > --
> > 2.11.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 8/9] drm/i915/dp: Protect link training with connection mutex

[Ausmus, James]

On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan
<dhinakaran.pandiyan@intel.com> wrote:
> The other instances of link training are protected with
> connection_mutex, so do the same in check_mst_status() too.

We don't seem to be taking connection_mutex around
intel_dp_start/stop_link_train in the intel_enable_dp or
intel_ddi_pre_enable_dp paths (unless it's taken higher in the stack)
- is it needed in all instances?

>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index aab9ba31f79e..644463ba313e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4191,6 +4191,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>  static int
>  intel_dp_check_mst_status(struct intel_dp *intel_dp)
>  {
> +       struct drm_device *dev = intel_dp_to_dev(intel_dp);
>         bool bret;
>         u8 esi[DP_DPRX_ESI_LEN] = { 0 };
>         int ret = 0;
> @@ -4205,8 +4206,11 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
>                 if (intel_dp->active_mst_links &&
>                     !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
>                         DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
> +
> +                       drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>                         intel_dp_start_link_train(intel_dp);
>                         intel_dp_stop_link_train(intel_dp);
> +                       drm_modeset_unlock(&dev->mode_config.connection_mutex);
>                 }
>
>                 DRM_DEBUG_KMS("got esi %3ph\n", esi);
> --
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 


James Ausmus
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 9/9] drm/i915/dp: Remove redundant can_mst checks in intel_dp_configure_mst()

[Ausmus, James]

On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan
<dhinakaran.pandiyan@intel.com> wrote:
> intel_dp_can_mst() performs these checks.
>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 6 ------
>  1 file changed, 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 644463ba313e..a4465b46bb27 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3845,12 +3845,6 @@ intel_dp_can_mst(struct intel_dp *intel_dp)
>  static void
>  intel_dp_configure_mst(struct intel_dp *intel_dp)
>  {
> -       if (!i915.enable_dp_mst)
> -               return;
> -
> -       if (!intel_dp->can_mst)
> -               return;
> -

By dropping these returns, we will now always get DRM_DEBUG_KMS output
on whether the sink is MST capable, even if the i915.enable_dp_mst
param is false - maybe drop these from intel_dp_can_mst instead?


>         intel_dp->is_mst = intel_dp_can_mst(intel_dp);
>
>         if (intel_dp->is_mst)
> --
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 


James Ausmus
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 8/9] drm/i915/dp: Protect link training with connection mutex

[Ville Syrjälä]

On Tue, Sep 12, 2017 at 04:57:29PM -0700, Dhinakaran Pandiyan wrote:
> The other instances of link training are protected with
> connection_mutex, so do the same in check_mst_status() too.
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index aab9ba31f79e..644463ba313e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4191,6 +4191,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>  static int
>  intel_dp_check_mst_status(struct intel_dp *intel_dp)
>  {
> +	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  	bool bret;
>  	u8 esi[DP_DPRX_ESI_LEN] = { 0 };
>  	int ret = 0;
> @@ -4205,8 +4206,11 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
>  		if (intel_dp->active_mst_links &&
>  		    !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
>  			DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
> +
> +			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>  			intel_dp_start_link_train(intel_dp);
>  			intel_dp_stop_link_train(intel_dp);
> +			drm_modeset_unlock(&dev->mode_config.connection_mutex);

This can deadlock. We should not grab any modeset locks from the
dig_work. I had some patches at some point to move the link training to
the hotplug work so SST. I don't think I had the MST side really sorted
out at any point.

>  		}
>  
>  		DRM_DEBUG_KMS("got esi %3ph\n", esi);
> -- 
> 2.11.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 8/9] drm/i915/dp: Protect link training with connection mutex

[Pandiyan, Dhinakaran]

On Thu, 2017-09-14 at 15:26 -0700, Ausmus, James wrote:
> On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan
> <dhinakaran.pandiyan@intel.com> wrote:
> > The other instances of link training are protected with
> > connection_mutex, so do the same in check_mst_status() too.
> 
> We don't seem to be taking connection_mutex around
> intel_dp_start/stop_link_train in the intel_enable_dp or
> intel_ddi_pre_enable_dp paths (unless it's taken higher in the stack)

That is right, it is take when the modeset begins.

> - is it needed in all instances?


> 
> >
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index aab9ba31f79e..644463ba313e 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4191,6 +4191,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
> >  static int
> >  intel_dp_check_mst_status(struct intel_dp *intel_dp)
> >  {
> > +       struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >         bool bret;
> >         u8 esi[DP_DPRX_ESI_LEN] = { 0 };
> >         int ret = 0;
> > @@ -4205,8 +4206,11 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
> >                 if (intel_dp->active_mst_links &&
> >                     !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
> >                         DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
> > +
> > +                       drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> >                         intel_dp_start_link_train(intel_dp);
> >                         intel_dp_stop_link_train(intel_dp);
> > +                       drm_modeset_unlock(&dev->mode_config.connection_mutex);
> >                 }
> >
> >                 DRM_DEBUG_KMS("got esi %3ph\n", esi);
> > --
> > 2.11.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 9/9] drm/i915/dp: Remove redundant can_mst checks in intel_dp_configure_mst()

[Pandiyan, Dhinakaran]

On Thu, 2017-09-14 at 15:36 -0700, Ausmus, James wrote:
> On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan
> <dhinakaran.pandiyan@intel.com> wrote:
> > intel_dp_can_mst() performs these checks.
> >
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 6 ------
> >  1 file changed, 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 644463ba313e..a4465b46bb27 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3845,12 +3845,6 @@ intel_dp_can_mst(struct intel_dp *intel_dp)
> >  static void
> >  intel_dp_configure_mst(struct intel_dp *intel_dp)
> >  {
> > -       if (!i915.enable_dp_mst)
> > -               return;
> > -
> > -       if (!intel_dp->can_mst)
> > -               return;
> > -
> 
> By dropping these returns, we will now always get DRM_DEBUG_KMS output
> on whether the sink is MST capable, even if the i915.enable_dp_mst
> param is false 

You are right, that debug msg is unnecessary.

> - maybe drop these from intel_dp_can_mst instead?
> 
Those checks are needed by the other caller - intel_detect_dpcd() 


On second thought, the early returns are indeed useful. Thanks for the
review.

> >         intel_dp->is_mst = intel_dp_can_mst(intel_dp);
> >
> >         if (intel_dp->is_mst)
> > --
> > 2.11.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 8/9] drm/i915/dp: Protect link training with connection mutex

[Manasi Navare]

On Thu, Sep 14, 2017 at 03:26:37PM -0700, Ausmus, James wrote:
> On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan
> <dhinakaran.pandiyan@intel.com> wrote:
> > The other instances of link training are protected with
> > connection_mutex, so do the same in check_mst_status() too.
> 
> We don't seem to be taking connection_mutex around
> intel_dp_start/stop_link_train in the intel_enable_dp or
> intel_ddi_pre_enable_dp paths (unless it's taken higher in the stack)
> - is it needed in all instances?
>

intel_ddi_pre_enable_dp() gets called from the crtc_enable hook higher
up in the stack which gets called essentially from atomic_commit() hook
that traces back eventually to the drm_mode_setcrtc call that grabs the
mod_config mutex there: mutex_lock(&crtc->dev->mode_config.mutex);

Manasi

> >
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index aab9ba31f79e..644463ba313e 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4191,6 +4191,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
> >  static int
> >  intel_dp_check_mst_status(struct intel_dp *intel_dp)
> >  {
> > +       struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >         bool bret;
> >         u8 esi[DP_DPRX_ESI_LEN] = { 0 };
> >         int ret = 0;
> > @@ -4205,8 +4206,11 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
> >                 if (intel_dp->active_mst_links &&
> >                     !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
> >                         DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
> > +
> > +                       drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> >                         intel_dp_start_link_train(intel_dp);
> >                         intel_dp_stop_link_train(intel_dp);
> > +                       drm_modeset_unlock(&dev->mode_config.connection_mutex);
> >                 }
> >
> >                 DRM_DEBUG_KMS("got esi %3ph\n", esi);
> > --
> > 2.11.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> 
> 
> James Ausmus
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 8/9] drm/i915/dp: Protect link training with connection mutex

[Ausmus, James]

On Fri, Sep 15, 2017 at 11:38 AM, Manasi Navare
<manasi.d.navare@intel.com> wrote:
> On Thu, Sep 14, 2017 at 03:26:37PM -0700, Ausmus, James wrote:
>> On Tue, Sep 12, 2017 at 4:57 PM, Dhinakaran Pandiyan
>> <dhinakaran.pandiyan@intel.com> wrote:
>> > The other instances of link training are protected with
>> > connection_mutex, so do the same in check_mst_status() too.
>>
>> We don't seem to be taking connection_mutex around
>> intel_dp_start/stop_link_train in the intel_enable_dp or
>> intel_ddi_pre_enable_dp paths (unless it's taken higher in the stack)
>> - is it needed in all instances?
>>
>
> intel_ddi_pre_enable_dp() gets called from the crtc_enable hook higher
> up in the stack which gets called essentially from atomic_commit() hook
> that traces back eventually to the drm_mode_setcrtc call that grabs the
> mod_config mutex there: mutex_lock(&crtc->dev->mode_config.mutex);

Got it - thanks for the pointer!

-James

>
> Manasi
>
>> >
>> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_dp.c | 4 ++++
>> >  1 file changed, 4 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> > index aab9ba31f79e..644463ba313e 100644
>> > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > @@ -4191,6 +4191,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>> >  static int
>> >  intel_dp_check_mst_status(struct intel_dp *intel_dp)
>> >  {
>> > +       struct drm_device *dev = intel_dp_to_dev(intel_dp);
>> >         bool bret;
>> >         u8 esi[DP_DPRX_ESI_LEN] = { 0 };
>> >         int ret = 0;
>> > @@ -4205,8 +4206,11 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
>> >                 if (intel_dp->active_mst_links &&
>> >                     !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
>> >                         DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
>> > +
>> > +                       drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> >                         intel_dp_start_link_train(intel_dp);
>> >                         intel_dp_stop_link_train(intel_dp);
>> > +                       drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> >                 }
>> >
>> >                 DRM_DEBUG_KMS("got esi %3ph\n", esi);
>> > --
>> > 2.11.0
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
>> --
>>
>>
>> James Ausmus
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 


James Ausmus
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 8/9] drm/i915/dp: Protect link training with connection mutex

[Pandiyan, Dhinakaran]

On Fri, 2017-09-15 at 13:10 +0300, Ville Syrjälä wrote:
> On Tue, Sep 12, 2017 at 04:57:29PM -0700, Dhinakaran Pandiyan wrote:
> > The other instances of link training are protected with
> > connection_mutex, so do the same in check_mst_status() too.
> > 
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index aab9ba31f79e..644463ba313e 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4191,6 +4191,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
> >  static int
> >  intel_dp_check_mst_status(struct intel_dp *intel_dp)
> >  {
> > +	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >  	bool bret;
> >  	u8 esi[DP_DPRX_ESI_LEN] = { 0 };
> >  	int ret = 0;
> > @@ -4205,8 +4206,11 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
> >  		if (intel_dp->active_mst_links &&
> >  		    !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
> >  			DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
> > +
> > +			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> >  			intel_dp_start_link_train(intel_dp);
> >  			intel_dp_stop_link_train(intel_dp);
> > +			drm_modeset_unlock(&dev->mode_config.connection_mutex);
> 
> This can deadlock. We should not grab any modeset locks from the
> dig_work. I had some patches at some point to move the link training to
> the hotplug work so SST. I don't think I had the MST side really sorted
> out at any point.

Interesting, the only lock we grab in this path from the work function
is the connection_mutex. So, I am not clear how this will deadlock. This
lock around link training is also at the same depth as the one around
link training in intel_dp_short_pulse(). Wouldn't that also deadlock if
that's the case?

 


> 
> >  		}
> >  
> >  		DRM_DEBUG_KMS("got esi %3ph\n", esi);
> > -- 
> > 2.11.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 8/9] drm/i915/dp: Protect link training with connection mutex

[Ville Syrjälä]

On Mon, Sep 18, 2017 at 09:50:30PM +0000, Pandiyan, Dhinakaran wrote:
> On Fri, 2017-09-15 at 13:10 +0300, Ville Syrjälä wrote:
> > On Tue, Sep 12, 2017 at 04:57:29PM -0700, Dhinakaran Pandiyan wrote:
> > > The other instances of link training are protected with
> > > connection_mutex, so do the same in check_mst_status() too.
> > > 
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index aab9ba31f79e..644463ba313e 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -4191,6 +4191,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
> > >  static int
> > >  intel_dp_check_mst_status(struct intel_dp *intel_dp)
> > >  {
> > > +	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> > >  	bool bret;
> > >  	u8 esi[DP_DPRX_ESI_LEN] = { 0 };
> > >  	int ret = 0;
> > > @@ -4205,8 +4206,11 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
> > >  		if (intel_dp->active_mst_links &&
> > >  		    !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
> > >  			DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
> > > +
> > > +			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > >  			intel_dp_start_link_train(intel_dp);
> > >  			intel_dp_stop_link_train(intel_dp);
> > > +			drm_modeset_unlock(&dev->mode_config.connection_mutex);
> > 
> > This can deadlock. We should not grab any modeset locks from the
> > dig_work. I had some patches at some point to move the link training to
> > the hotplug work so SST. I don't think I had the MST side really sorted
> > out at any point.
> 
> Interesting, the only lock we grab in this path from the work function
> is the connection_mutex. So, I am not clear how this will deadlock. This
> lock around link training is also at the same depth as the one around
> link training in intel_dp_short_pulse(). Wouldn't that also deadlock if
> that's the case?

Theoretically. Though I think the deadlock is only likely to happen with
MST since that requires sideband to work during a modeset when we have
connection_mutex already locked. So modeset code will be waiting for
sideband to happen, and hpd_pulse which is responsible for doing sideband
is waiting on the lock already held by the modeset. Thus we're stuck.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 8/9] drm/i915/dp: Protect link training with connection mutex

[Pandiyan, Dhinakaran]

On Tue, 2017-09-19 at 15:42 +0300, Ville Syrjälä wrote:
> On Mon, Sep 18, 2017 at 09:50:30PM +0000, Pandiyan, Dhinakaran wrote:
> > On Fri, 2017-09-15 at 13:10 +0300, Ville Syrjälä wrote:
> > > On Tue, Sep 12, 2017 at 04:57:29PM -0700, Dhinakaran Pandiyan wrote:
> > > > The other instances of link training are protected with
> > > > connection_mutex, so do the same in check_mst_status() too.
> > > > 
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > index aab9ba31f79e..644463ba313e 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -4191,6 +4191,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
> > > >  static int
> > > >  intel_dp_check_mst_status(struct intel_dp *intel_dp)
> > > >  {
> > > > +	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> > > >  	bool bret;
> > > >  	u8 esi[DP_DPRX_ESI_LEN] = { 0 };
> > > >  	int ret = 0;
> > > > @@ -4205,8 +4206,11 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
> > > >  		if (intel_dp->active_mst_links &&
> > > >  		    !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
> > > >  			DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
> > > > +
> > > > +			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > > >  			intel_dp_start_link_train(intel_dp);
> > > >  			intel_dp_stop_link_train(intel_dp);
> > > > +			drm_modeset_unlock(&dev->mode_config.connection_mutex);
> > > 
> > > This can deadlock. We should not grab any modeset locks from the
> > > dig_work. I had some patches at some point to move the link training to
> > > the hotplug work so SST. I don't think I had the MST side really sorted
> > > out at any point.
> > 
> > Interesting, the only lock we grab in this path from the work function
> > is the connection_mutex. So, I am not clear how this will deadlock. This
> > lock around link training is also at the same depth as the one around
> > link training in intel_dp_short_pulse(). Wouldn't that also deadlock if
> > that's the case?
> 
> Theoretically. Though I think the deadlock is only likely to happen with
> MST since that requires sideband to work during a modeset when we have
> connection_mutex already locked. So modeset code will be waiting for
> sideband to happen, and hpd_pulse which is responsible for doing sideband
> is waiting on the lock already held by the modeset. Thus we're stuck.
> 

disable_dp and enable_dp are the ones that use sideband messages during
modeset. But, there doesn't seem to be any real dependency on the
sideband messages, mostly because there is not much we do differently
when a downstream reply does not arrive.

Having said that, now that I know you (moving link training over to
hotplug work) and Maarten (link training vs modeset) already have code
to deal with this properly, this patch should be dropped. Thanks for the
review!


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx