[PATCH 1/5] drm/i915: Splitting intel_dp_detect

[PATCH 1/5] drm/i915: Splitting intel_dp_detect

[Shubhangi Shrivastava]

intel_dp_detect() is called for not just detection but
during modes enumeration as well. Repeating the whole
sequence during each of these calls is wasteful and
time consuming.
This patch moves probing for panel, DPCD read etc done in
intel_dp_detect() to a new function intel_dp_long_pulse().
Note that the behavior of intel_dp_detect() is changed to
report connected or disconnected depending on whether the
EDID is available or not.
This change will be required by further patches in the series
to avoid performing duplicated DPCD operations on hotplug.

v2: Moved a hunk to next patch of the series.
    Moved intel_dp_unset_edid to out. (Ander)
v3: Rephrased commit message and intel_dp_unset_dp() is called
    within intel_dp_set_dp() to free the previous EDID. (Ander)
v4: Added overriding of status to disconnected for MST. (Ander)

Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 63 ++++++++++++++++++++++++++++-------------
 1 file changed, 43 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1761254..65028a4 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -129,6 +129,7 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
 static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp);
 static void vlv_steal_power_sequencer(struct drm_device *dev,
 				      enum pipe pipe);
+static void intel_dp_unset_edid(struct intel_dp *intel_dp);
 
 static unsigned int intel_dp_unused_lane_mask(int lane_count)
 {
@@ -4577,6 +4578,7 @@ intel_dp_set_edid(struct intel_dp *intel_dp)
 	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	struct edid *edid;
 
+	intel_dp_unset_edid(intel_dp);
 	edid = intel_dp_get_edid(intel_dp);
 	intel_connector->detect_edid = edid;
 
@@ -4597,9 +4599,10 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
 	intel_dp->has_audio = false;
 }
 
-static enum drm_connector_status
-intel_dp_detect(struct drm_connector *connector, bool force)
+static void
+intel_dp_long_pulse(struct intel_connector *intel_connector)
 {
+	struct drm_connector *connector = &intel_connector->base;
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct intel_encoder *intel_encoder = &intel_dig_port->base;
@@ -4609,17 +4612,6 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	bool ret;
 	u8 sink_irq_vector;
 
-	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
-		      connector->base.id, connector->name);
-	intel_dp_unset_edid(intel_dp);
-
-	if (intel_dp->is_mst) {
-		/* MST devices are disconnected from a monitor POV */
-		if (intel_encoder->type != INTEL_OUTPUT_EDP)
-			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
-		return connector_status_disconnected;
-	}
-
 	power_domain = intel_display_port_aux_power_domain(intel_encoder);
 	intel_display_power_get(to_i915(dev), power_domain);
 
@@ -4640,14 +4632,18 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 		goto out;
 	}
 
+	if (intel_encoder->type != INTEL_OUTPUT_EDP)
+		intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
+
 	intel_dp_probe_oui(intel_dp);
 
 	ret = intel_dp_probe_mst(intel_dp);
 	if (ret) {
-		/* if we are in MST mode then this connector
-		   won't appear connected or have anything with EDID on it */
-		if (intel_encoder->type != INTEL_OUTPUT_EDP)
-			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
+		/*
+		 * If we are in MST mode then this connector
+		 * won't appear connected or have anything
+		 * with EDID on it
+		 */
 		status = connector_status_disconnected;
 		goto out;
 	}
@@ -4662,8 +4658,6 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 
 	intel_dp_set_edid(intel_dp);
 
-	if (intel_encoder->type != INTEL_OUTPUT_EDP)
-		intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
 	status = connector_status_connected;
 
 	/* Try to read the source of the interrupt */
@@ -4681,8 +4675,37 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	}
 
 out:
+	if (status != connector_status_connected)
+		intel_dp_unset_edid(intel_dp);
 	intel_display_power_put(to_i915(dev), power_domain);
-	return status;
+	return;
+}
+
+static enum drm_connector_status
+intel_dp_detect(struct drm_connector *connector, bool force)
+{
+	struct intel_dp *intel_dp = intel_attached_dp(connector);
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct intel_encoder *intel_encoder = &intel_dig_port->base;
+	struct intel_connector *intel_connector = to_intel_connector(connector);
+
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
+		     connector->base.id, connector->name);
+
+	if (intel_dp->is_mst) {
+		/* MST devices are disconnected from a monitor POV */
+		intel_dp_unset_edid(intel_dp);
+		if (intel_encoder->type != INTEL_OUTPUT_EDP)
+			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
+		return connector_status_disconnected;
+	}
+
+	intel_dp_long_pulse(intel_dp->attached_connector);
+
+	if (intel_connector->detect_edid)
+		return connector_status_connected;
+	else
+		return connector_status_disconnected;
 }
 
 static void
-- 
2.6.1

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

[PATCH 2/5] drm/i915: Cleaning up intel_dp_hpd_pulse

[Shubhangi Shrivastava]

Current DP detection has DPCD operations split across
intel_dp_hpd_pulse and intel_dp_detect which contains
duplicates as well. Also intel_dp_detect is called
during modes enumeration as well which will result
in multiple dpcd operations. So this patch tries
to solve both these by bringing all DPCD operations
in one single function and make intel_dp_detect
use existing values instead of repeating same steps.

v2: Pulled in a hunk from last patch of the series to
    this patch. (Ander)
v3: Added MST hotplug handling. (Ander)

Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 65 +++++++++++++++++++++++++----------------
 1 file changed, 40 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 65028a4..ce99d59 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4646,6 +4646,16 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 		 */
 		status = connector_status_disconnected;
 		goto out;
+	} else if (connector->status == connector_status_connected) {
+		/*
+		 * If display was connected already and is still connected
+		 * check links status, there has been known issues of
+		 * link loss triggerring long pulse!!!!
+		 */
+		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+		intel_dp_check_link_status(intel_dp);
+		drm_modeset_unlock(&dev->mode_config.connection_mutex);
+		goto out;
 	}
 
 	/*
@@ -4675,8 +4685,21 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 	}
 
 out:
-	if (status != connector_status_connected)
+	if (status != connector_status_connected) {
 		intel_dp_unset_edid(intel_dp);
+		/*
+		 * If we were in MST mode, and device is not there,
+		 * get out of MST mode
+		 */
+		if (intel_dp->is_mst) {
+			DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
+				intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
+			intel_dp->is_mst = false;
+			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
+							intel_dp->is_mst);
+		}
+	}
+
 	intel_display_power_put(to_i915(dev), power_domain);
 	return;
 }
@@ -5033,25 +5056,25 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 		/* indicate that we need to restart link training */
 		intel_dp->train_set_valid = false;
 
-		if (!intel_digital_port_connected(dev_priv, intel_dig_port))
-			goto mst_fail;
+		intel_dp_long_pulse(intel_dp->attached_connector);
+		if (intel_dp->is_mst)
+			ret = IRQ_HANDLED;
+		goto put_power;
 
-		if (!intel_dp_get_dpcd(intel_dp)) {
-			goto mst_fail;
-		}
-
-		intel_dp_probe_oui(intel_dp);
-
-		if (!intel_dp_probe_mst(intel_dp)) {
-			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-			intel_dp_check_link_status(intel_dp);
-			drm_modeset_unlock(&dev->mode_config.connection_mutex);
-			goto mst_fail;
-		}
 	} else {
 		if (intel_dp->is_mst) {
-			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
-				goto mst_fail;
+			if (intel_dp_check_mst_status(intel_dp) == -EINVAL) {
+				/*
+				 * If we were in MST mode, and device is not
+				 * there, get out of MST mode
+				 */
+				DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
+					intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
+				intel_dp->is_mst = false;
+				drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
+								intel_dp->is_mst);
+				goto put_power;
+			}
 		}
 
 		if (!intel_dp->is_mst) {
@@ -5063,14 +5086,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 
 	ret = IRQ_HANDLED;
 
-	goto put_power;
-mst_fail:
-	/* if we were in MST mode, and device is not there get out of MST mode */
-	if (intel_dp->is_mst) {
-		DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
-		intel_dp->is_mst = false;
-		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
-	}
 put_power:
 	intel_display_power_put(dev_priv, power_domain);
 
-- 
2.6.1

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

[PATCH 3/5] drm/i915: Reorganizing intel_dp_check_link_status

[Shubhangi Shrivastava]

When created originally intel_dp_check_link_status()
was supposed to handle only link training for short
pulse but has grown into handler for short pulse itself.
This patch cleans up this function by splitting it into
two halves. First intel_dp_short_pulse() is called,
which will be entry point and handle all logic for
short pulse handling while intel_dp_check_link_status()
will retain its original purpose of only doing link
status related work.

intel_dp_short_pulse: All existing code other than
link status read and link training upon error status.

intel_dp_check_link_status:
The link status should be read on short pulse
irrespective of panel being enabled or not so
intel_dp_get_link_status() performs dpcd read first
then based on crtc active / enabled it will
perform the link training.

This is because short pulse is a generic interrupt
which should always be handled, because it may mean:
	1. Hotplug/unplug of MST panel
	2. Hotplug/unplug of dongle
	3. Link status change for other DP panels

v2: Added WARN_ON to intel_dp_check_link_status()
    Removed a call to intel_dp_get_link_status() (Ander)

v3: Changed commit message to explain need of link status
    being read before performing encoder checks (Daniel)

v4: Changed commit message to explain need of reading
    link status on short pulse (Ander)

Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 65 +++++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index ce99d59..9cb6ea9 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4279,6 +4279,36 @@ go_again:
 	return -EINVAL;
 }
 
+static void
+intel_dp_check_link_status(struct intel_dp *intel_dp)
+{
+	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
+	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	u8 link_status[DP_LINK_STATUS_SIZE];
+
+	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
+
+	if (!intel_dp_get_link_status(intel_dp, link_status)) {
+		DRM_ERROR("Failed to get link status\n");
+		return;
+	}
+
+	if (!intel_encoder->base.crtc)
+		return;
+
+	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
+		return;
+
+	/* if link training is requested we should perform it always */
+	if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) ||
+		(!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
+		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
+				intel_encoder->base.name);
+		intel_dp_start_link_train(intel_dp);
+		intel_dp_stop_link_train(intel_dp);
+	}
+}
+
 /*
  * According to DP spec
  * 5.1.2:
@@ -4288,14 +4318,10 @@ go_again:
  *  4. Check link status on receipt of hot-plug interrupt
  */
 static void
-intel_dp_check_link_status(struct intel_dp *intel_dp)
+intel_dp_short_pulse(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
-	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
 	u8 sink_irq_vector;
-	u8 link_status[DP_LINK_STATUS_SIZE];
-
-	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
 	/*
 	 * Clearing compliance test variables to allow capturing
@@ -4305,17 +4331,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 	intel_dp->compliance_test_type = 0;
 	intel_dp->compliance_test_data = 0;
 
-	if (!intel_encoder->base.crtc)
-		return;
-
-	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
-		return;
-
-	/* Try to read receiver status if the link appears to be up */
-	if (!intel_dp_get_link_status(intel_dp, link_status)) {
-		return;
-	}
-
 	/* Now read the DPCD to see if it's actually running */
 	if (!intel_dp_get_dpcd(intel_dp)) {
 		return;
@@ -4335,14 +4350,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
 	}
 
-	/* if link training is requested we should perform it always */
-	if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) ||
-		(!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
-		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
-			      intel_encoder->base.name);
-		intel_dp_start_link_train(intel_dp);
-		intel_dp_stop_link_train(intel_dp);
-	}
+	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+	intel_dp_check_link_status(intel_dp);
+	drm_modeset_unlock(&dev->mode_config.connection_mutex);
 }
 
 /* XXX this is probably wrong for multiple downstream ports */
@@ -5077,11 +5087,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 			}
 		}
 
-		if (!intel_dp->is_mst) {
-			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-			intel_dp_check_link_status(intel_dp);
-			drm_modeset_unlock(&dev->mode_config.connection_mutex);
-		}
+		if (!intel_dp->is_mst)
+			intel_dp_short_pulse(intel_dp);
 	}
 
 	ret = IRQ_HANDLED;
-- 
2.6.1

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

[PATCH 4/5] drm/i915: Save sink_count for tracking changes to it and read sink_count dpcd always

[Shubhangi Shrivastava]

This patch reads sink_count dpcd always and removes its
read operation based on values in downstream port dpcd.

SINK_COUNT dpcd is not dependent on DOWNSTREAM_PORT_PRESENT dpcd.
SINK_COUNT denotes if a display is attached, while
DOWNSTREAM_PORT_PRESET indicates how many ports are available
in the dongle where display can be attached. so it is possible
for sink count to change irrespective of value in downstream
port dpcd.

Here is a table of possible values and scenarios

sink_count      downstream_port
                present
0               0               no display is attached
0               1               dongle is connected without display
1               0               display connected directly
1               1               display connected through dongle

v2: Storing value of intel_dp->sink_count that is ready
    for consumption. (Ander)
    Squashing two commits into one. (Ander)

v3: Added comment to explain the need of early return when
    sink count is 0. (Ander)

Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 30 +++++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 9cb6ea9..539f5a3 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3855,6 +3855,27 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	if (intel_dp->dpcd[DP_DPCD_REV] == 0)
 		return false; /* DPCD not present */
 
+	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
+				    &intel_dp->sink_count, 1) < 0)
+		return false;
+
+	/*
+	 * Sink count can change between short pulse hpd hence
+	 * a member variable in intel_dp will track any changes
+	 * between short pulse interrupts.
+	 */
+	intel_dp->sink_count = DP_GET_SINK_COUNT(intel_dp->sink_count);
+
+	/*
+	 * SINK_COUNT == 0 and DOWNSTREAM_PORT_PRESENT == 1 implies that
+	 * a dongle is present but no display. Unless we require to know
+	 * if a dongle is present or not, we don't need to update
+	 * downstream port information. So, an early return here saves
+	 * time from performing other operations which are not required.
+	 */
+	if (!intel_dp->sink_count)
+		return false;
+
 	/* Check if the panel supports PSR */
 	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
 	if (is_edp(intel_dp)) {
@@ -4372,14 +4393,9 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
 	/* If we're HPD-aware, SINK_COUNT changes dynamically */
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
 	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
-		uint8_t reg;
-
-		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
-					    &reg, 1) < 0)
-			return connector_status_unknown;
 
-		return DP_GET_SINK_COUNT(reg) ? connector_status_connected
-					      : connector_status_disconnected;
+		return intel_dp->sink_count ?
+		connector_status_connected : connector_status_disconnected;
 	}
 
 	/* If no HPD, poke DDC gently */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 059b46e..0879466 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -774,6 +774,7 @@ struct intel_dp {
 	uint32_t DP;
 	int link_rate;
 	uint8_t lane_count;
+	uint8_t sink_count;
 	bool has_audio;
 	enum hdmi_force_audio force_audio;
 	bool limited_color_range;
-- 
2.6.1

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

[PATCH 5/5] drm/i915: force full detect on sink count change

[Shubhangi Shrivastava]

This patch checks for changes in sink count between short pulse
hpds and forces full detect when there is a change.

This will allow both detection of hotplug and unplug of panels
through dongles that give only short pulse for such events.

v2: changed variable type from u8 to bool (Jani)
    return immediately if perform_full_detect is set(Siva)

v3: changed method of determining full detection from using
    pointer to return code (Siva)

v4: changed comments to indicate meaning of return value of
    intel_dp_short_pulse and explain the use of return value
    from intel_dp_get_dpcd in intel_dp_short_pulse (Ander)

Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 539f5a3..dd433e7 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4337,12 +4337,19 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
  *  2. Configure link according to Receiver Capabilities
  *  3. Use Link Training from 2.5.3.3 and 3.5.1.3
  *  4. Check link status on receipt of hot-plug interrupt
+ *
+ * intel_dp_short_pulse -  handles short pulse interrupts
+ * when full detection is not required.
+ * Returns %true if short pulse is handled and full detection
+ * is NOT required and %false otherwise.
  */
-static void
+static bool
 intel_dp_short_pulse(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	u8 sink_irq_vector;
+	u8 old_sink_count = intel_dp->sink_count;
+	bool ret;
 
 	/*
 	 * Clearing compliance test variables to allow capturing
@@ -4352,9 +4359,17 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
 	intel_dp->compliance_test_type = 0;
 	intel_dp->compliance_test_data = 0;
 
-	/* Now read the DPCD to see if it's actually running */
-	if (!intel_dp_get_dpcd(intel_dp)) {
-		return;
+	/*
+	 * Now read the DPCD to see if it's actually running
+	 * If the current value of sink count doesn't match with
+	 * the value that was stored earlier or dpcd read failed
+	 * we need to do full detection
+	 */
+	ret = intel_dp_get_dpcd(intel_dp);
+
+	if ((old_sink_count != intel_dp->sink_count) || !ret) {
+		/* No need to proceed if we are going to do full detect */
+		return false;
 	}
 
 	/* Try to read the source of the interrupt */
@@ -4374,6 +4389,8 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
 	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
 	intel_dp_check_link_status(intel_dp);
 	drm_modeset_unlock(&dev->mode_config.connection_mutex);
+
+	return true;
 }
 
 /* XXX this is probably wrong for multiple downstream ports */
@@ -5103,8 +5120,12 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 			}
 		}
 
-		if (!intel_dp->is_mst)
-			intel_dp_short_pulse(intel_dp);
+		if (!intel_dp->is_mst) {
+			if (!intel_dp_short_pulse(intel_dp)) {
+				intel_dp_long_pulse(intel_dp->attached_connector);
+				goto put_power;
+			}
+		}
 	}
 
 	ret = IRQ_HANDLED;
-- 
2.6.1

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

✓ Fi.CI.BAT: success for series starting with [1/5] drm/i915: Splitting intel_dp_detect

[Patchwork]

== Summary ==

Series 2978v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/2978/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                incomplete -> PASS       (hsw-gt2)
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> PASS       (bsw-nuc-2)

bdw-nuci7        total:156  pass:147  dwarn:0   dfail:0   fail:0   skip:9  
bdw-ultra        total:159  pass:147  dwarn:0   dfail:0   fail:0   skip:12 
bsw-nuc-2        total:159  pass:129  dwarn:0   dfail:0   fail:0   skip:30 
byt-nuc          total:159  pass:136  dwarn:0   dfail:0   fail:0   skip:23 
hsw-brixbox      total:159  pass:146  dwarn:0   dfail:0   fail:0   skip:13 
hsw-gt2          total:159  pass:149  dwarn:0   dfail:0   fail:0   skip:10 
ilk-hp8440p      total:159  pass:111  dwarn:0   dfail:0   fail:0   skip:48 
ivb-t430s        total:159  pass:145  dwarn:0   dfail:0   fail:0   skip:14 
skl-i5k-2        total:159  pass:144  dwarn:1   dfail:0   fail:0   skip:14 
snb-dellxps      total:159  pass:137  dwarn:0   dfail:0   fail:0   skip:22 
snb-x220t        total:159  pass:137  dwarn:0   dfail:0   fail:1   skip:21 

Results at /archive/results/CI_IGT_test/Patchwork_1335/

6b1049b84dcd979f631d15b2ada325d8e5b2c4e1 drm-intel-nightly: 2016y-01m-29d-22h-50m-57s UTC integration manifest
115af193cac6d62145809df8292fadbd3c370fb2 drm/i915: force full detect on sink count change
c8dced60ddfc26d53e37a7607767046880cc5178 drm/i915: Save sink_count for tracking changes to it and read sink_count dpcd always
9e65fb79da24330663dcbc888afeb61300507e71 drm/i915: Reorganizing intel_dp_check_link_status
c2e3381bf553cade9bbe15bc9f21d1677e8940e8 drm/i915: Cleaning up intel_dp_hpd_pulse
88fa9d50f75d5d5d939f49ce6441fd39c181acc2 drm/i915: Splitting intel_dp_detect

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

Re: [PATCH 2/5] drm/i915: Cleaning up intel_dp_hpd_pulse

[Chris Wilson]

On Fri, Nov 18, 2016 at 11:41:37AM +0200, Ander Conselvan De Oliveira wrote:
> On Thu, 2016-11-17 at 22:01 +0000, Chris Wilson wrote:
> > On Mon, Feb 01, 2016 at 11:13:08AM +0200, Ander Conselvan De Oliveira wrote:
> > > 
> > > On Mon, 2016-02-01 at 11:50 +0530, Thulasimani, Sivakumar wrote:
> > > > 
> > > > 
> > > > On 1/29/2016 5:33 PM, Ander Conselvan De Oliveira wrote:
> > > > > 
> > > > > On Fri, 2016-01-29 at 14:31 +0530, Shubhangi Shrivastava wrote:
> > > > > > 
> > > > > > On Tuesday 26 January 2016 06:52 PM, Ander Conselvan De Oliveira
> > > > > > wrote:
> > > > > > > 
> > > > > > > On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote:
> > > > > > > > 
> > > > > > > > Current DP detection has DPCD operations split across
> > > > > > > > intel_dp_hpd_pulse and intel_dp_detect which contains
> > > > > > > > duplicates as well. Also intel_dp_detect is called
> > > > > > > > during modes enumeration as well which will result
> > > > > > > > in multiple dpcd operations. So this patch tries
> > > > > > > > to solve both these by bringing all DPCD operations
> > > > > > > > in one single function and make intel_dp_detect
> > > > > > > > use existing values instead of repeating same steps.
> > > > > > > > 
> > > > > > > > v2: Pulled in a hunk from last patch of the series to
> > > > > > > >       this patch. (Ander)
> > > > > > > > v3: Added MST hotplug handling. (Ander)
> > > > > > > > 
> > > > > > > > Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
> > > > > > > > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.
> > > > > > > > com>
> > > > > > > > Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.
> > > > > > > > com>
> > > > > > > > ---
> > > > > > > >    drivers/gpu/drm/i915/intel_dp.c | 71 +++++++++++++++++++++++++-
> > > > > > > > ----
> > > > > > > > -----
> > > > > > > > -----
> > > > > > > > -
> > > > > > > >    1 file changed, 44 insertions(+), 27 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > > index 8969ff9..82ee18d 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > [...]
> > > > > > > 
> > > > > > > > 
> > > > > > > > @@ -4693,7 +4717,8 @@ intel_dp_detect(struct drm_connector
> > > > > > > > *connector,
> > > > > > > > bool
> > > > > > > > force)
> > > > > > > >    		return connector_status_disconnected;
> > > > > > > >    	}
> > > > > > > >    
> > > > > > > > -	intel_dp_long_pulse(intel_dp->attached_connector);
> > > > > > > > +	if (force)
> > > > > > > > +		intel_dp_long_pulse(intel_dp-
> > > > > > > > >attached_connector);
> > > > > > > I didn't notice this at first, but force is not the right thing to
> > > > > > > check
> > > > > > > for
> > > > > > > here. It is basically intended to avoid doing load detection (see
> > > > > > > intel_get_load_detect_pipe()) on automated polling. But we still
> > > > > > > have to
> > > > > > > try
> > > > > > > detection here when force = false, otherwise this will cause a
> > > > > > > regression.
> > > > > > > 
> > > > > > > If you plug in a DP device while suspended, that device won't get
> > > > > > > detected,
> > > > > > > since we don't get an HPD for it. Previously, the call do
> > > > > > > intel_dp_detect()
> > > > > > > with
> > > > > > > force = false from intel_drm_resume() (via
> > > > > > > drm_helper_hpd_irq_event())
> > > > > > > would
> > > > > > > cause a full detection.
> > > > > > > 
> > > > > > > To avoid the repeated DPCD operations, I think we need a more
> > > > > > > explicit
> > > > > > > mechanism
> > > > > > > to signal that we already handled the long pulse via the HPD
> > > > > > > handler. In
> > > > > > > intel_dp_hpd_pulse() we could set a flag that tells we just handled
> > > > > > > a
> > > > > > > long
> > > > > > > pulse
> > > > > > > for the given port. The call to intel_dp_long_pulse() in
> > > > > > > intel_dp_detect()
> > > > > > > would
> > > > > > > then be dependent on that flag.
> > > > > > > 
> > > > > > > For that reason I have to retract my R-b from this patch.
> > > > > > > 
> > > > > > > Ander
> > > > > > Call to intel_dp_detect() from get_modes is with force set to true
> > > > > > while
> > > > > > from resume the call is with force set to false.. It should be in the
> > > > > > opposite manner as get_modes should not require full detection whereas
> > > > > > resume should. So, this needs to be cleaned up there. After merge of
> > > > > > these patches, will look into cleaning up that part of the code.
> > > > > That really depends on what the force parameter is supposed to mean. The
> > > > > documentation states that "force is set to false whilst polling, true
> > > > > when
> > > > > checking the connector due to a user request". A look through git
> > > > > history
> > > > > shows
> > > > > the parameter was added to reduce time wasted doing load detection
> > > > > (doing a
> > > > > mode
> > > > > set in order to check if there is a device connected) for hardware that
> > > > > needs it
> > > > > (commit 7b334fcb45b7).
> > > > > 
> > > > > As far as I can tell, across all the drm drivers, that parameter is only
> > > > > used to
> > > > > avoid doing load detection.
> > > > > 
> > > > > Another thing to consider is that the driver may switch to polling if it
> > > > > detects
> > > > > an HPD storm. When the detect calls come from polling, the code in this
> > > > > patch
> > > > > will simply avoid any detection.
> > > > > 
> > > > hmm i think this discussion will prolong for a while :)
> > > > how about we call intel_dp_long_pulse() always for now.
> > > > this will be a compromise to not break any of the existing code
> > > > but will still result in getting a clean detection code, which
> > > > will can then be improved upon in the next iteration ?
> > > > i.e post the change it should look like. i.e skip this change alone
> > > > 
> > > > 	intel_dp_long_pulse(intel_dp->attached_connector);
> > > Sounds good. The code gets cleaner and there is no regression in terms of
> > > repeated DPCD operations.
> > So this conversation seems to have had little bearing on reality,
> > especially in terms of how intel_dp_detect() is supposed to behave. This
> > patch is causing WARNs as it presumed that intel_dp_detect() is only
> > called after a modeset.
> > 
> > Should we send a revert to stable?
> 
> Ville's 27d4efc5591a ("drm/i915: Move long hpd handling into the hotplug work")
> and 16c83fad79ca ("drm/i915: Allow DP to work w/o EDID") were sent to stable and
> are in 4.8.6. Are the WARNs happening with those patches too?

Yes. The WARNs are still happening in -nightly, they only take calling
GETCONNECTOR twice on a connected monitor before setting a mode.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915: Cleaning up intel_dp_hpd_pulse

[Ander Conselvan De Oliveira]

On Thu, 2016-11-17 at 22:01 +0000, Chris Wilson wrote:
> On Mon, Feb 01, 2016 at 11:13:08AM +0200, Ander Conselvan De Oliveira wrote:
> > 
> > On Mon, 2016-02-01 at 11:50 +0530, Thulasimani, Sivakumar wrote:
> > > 
> > > 
> > > On 1/29/2016 5:33 PM, Ander Conselvan De Oliveira wrote:
> > > > 
> > > > On Fri, 2016-01-29 at 14:31 +0530, Shubhangi Shrivastava wrote:
> > > > > 
> > > > > On Tuesday 26 January 2016 06:52 PM, Ander Conselvan De Oliveira
> > > > > wrote:
> > > > > > 
> > > > > > On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote:
> > > > > > > 
> > > > > > > Current DP detection has DPCD operations split across
> > > > > > > intel_dp_hpd_pulse and intel_dp_detect which contains
> > > > > > > duplicates as well. Also intel_dp_detect is called
> > > > > > > during modes enumeration as well which will result
> > > > > > > in multiple dpcd operations. So this patch tries
> > > > > > > to solve both these by bringing all DPCD operations
> > > > > > > in one single function and make intel_dp_detect
> > > > > > > use existing values instead of repeating same steps.
> > > > > > > 
> > > > > > > v2: Pulled in a hunk from last patch of the series to
> > > > > > >       this patch. (Ander)
> > > > > > > v3: Added MST hotplug handling. (Ander)
> > > > > > > 
> > > > > > > Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
> > > > > > > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.
> > > > > > > com>
> > > > > > > Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.
> > > > > > > com>
> > > > > > > ---
> > > > > > >    drivers/gpu/drm/i915/intel_dp.c | 71 +++++++++++++++++++++++++-
> > > > > > > ----
> > > > > > > -----
> > > > > > > -----
> > > > > > > -
> > > > > > >    1 file changed, 44 insertions(+), 27 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > index 8969ff9..82ee18d 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > [...]
> > > > > > 
> > > > > > > 
> > > > > > > @@ -4693,7 +4717,8 @@ intel_dp_detect(struct drm_connector
> > > > > > > *connector,
> > > > > > > bool
> > > > > > > force)
> > > > > > >    		return connector_status_disconnected;
> > > > > > >    	}
> > > > > > >    
> > > > > > > -	intel_dp_long_pulse(intel_dp->attached_connector);
> > > > > > > +	if (force)
> > > > > > > +		intel_dp_long_pulse(intel_dp-
> > > > > > > >attached_connector);
> > > > > > I didn't notice this at first, but force is not the right thing to
> > > > > > check
> > > > > > for
> > > > > > here. It is basically intended to avoid doing load detection (see
> > > > > > intel_get_load_detect_pipe()) on automated polling. But we still
> > > > > > have to
> > > > > > try
> > > > > > detection here when force = false, otherwise this will cause a
> > > > > > regression.
> > > > > > 
> > > > > > If you plug in a DP device while suspended, that device won't get
> > > > > > detected,
> > > > > > since we don't get an HPD for it. Previously, the call do
> > > > > > intel_dp_detect()
> > > > > > with
> > > > > > force = false from intel_drm_resume() (via
> > > > > > drm_helper_hpd_irq_event())
> > > > > > would
> > > > > > cause a full detection.
> > > > > > 
> > > > > > To avoid the repeated DPCD operations, I think we need a more
> > > > > > explicit
> > > > > > mechanism
> > > > > > to signal that we already handled the long pulse via the HPD
> > > > > > handler. In
> > > > > > intel_dp_hpd_pulse() we could set a flag that tells we just handled
> > > > > > a
> > > > > > long
> > > > > > pulse
> > > > > > for the given port. The call to intel_dp_long_pulse() in
> > > > > > intel_dp_detect()
> > > > > > would
> > > > > > then be dependent on that flag.
> > > > > > 
> > > > > > For that reason I have to retract my R-b from this patch.
> > > > > > 
> > > > > > Ander
> > > > > Call to intel_dp_detect() from get_modes is with force set to true
> > > > > while
> > > > > from resume the call is with force set to false.. It should be in the
> > > > > opposite manner as get_modes should not require full detection whereas
> > > > > resume should. So, this needs to be cleaned up there. After merge of
> > > > > these patches, will look into cleaning up that part of the code.
> > > > That really depends on what the force parameter is supposed to mean. The
> > > > documentation states that "force is set to false whilst polling, true
> > > > when
> > > > checking the connector due to a user request". A look through git
> > > > history
> > > > shows
> > > > the parameter was added to reduce time wasted doing load detection
> > > > (doing a
> > > > mode
> > > > set in order to check if there is a device connected) for hardware that
> > > > needs it
> > > > (commit 7b334fcb45b7).
> > > > 
> > > > As far as I can tell, across all the drm drivers, that parameter is only
> > > > used to
> > > > avoid doing load detection.
> > > > 
> > > > Another thing to consider is that the driver may switch to polling if it
> > > > detects
> > > > an HPD storm. When the detect calls come from polling, the code in this
> > > > patch
> > > > will simply avoid any detection.
> > > > 
> > > hmm i think this discussion will prolong for a while :)
> > > how about we call intel_dp_long_pulse() always for now.
> > > this will be a compromise to not break any of the existing code
> > > but will still result in getting a clean detection code, which
> > > will can then be improved upon in the next iteration ?
> > > i.e post the change it should look like. i.e skip this change alone
> > > 
> > > 	intel_dp_long_pulse(intel_dp->attached_connector);
> > Sounds good. The code gets cleaner and there is no regression in terms of
> > repeated DPCD operations.
> So this conversation seems to have had little bearing on reality,
> especially in terms of how intel_dp_detect() is supposed to behave. This
> patch is causing WARNs as it presumed that intel_dp_detect() is only
> called after a modeset.
> 
> Should we send a revert to stable?

Ville's 27d4efc5591a ("drm/i915: Move long hpd handling into the hotplug work")
and 16c83fad79ca ("drm/i915: Allow DP to work w/o EDID") were sent to stable and
are in 4.8.6. Are the WARNs happening with those patches too?

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

Re: [PATCH 2/5] drm/i915: Cleaning up intel_dp_hpd_pulse

[Chris Wilson]

On Mon, Feb 01, 2016 at 11:13:08AM +0200, Ander Conselvan De Oliveira wrote:
> On Mon, 2016-02-01 at 11:50 +0530, Thulasimani, Sivakumar wrote:
> > 
> > On 1/29/2016 5:33 PM, Ander Conselvan De Oliveira wrote:
> > > On Fri, 2016-01-29 at 14:31 +0530, Shubhangi Shrivastava wrote:
> > > > On Tuesday 26 January 2016 06:52 PM, Ander Conselvan De Oliveira wrote:
> > > > > On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote:
> > > > > > Current DP detection has DPCD operations split across
> > > > > > intel_dp_hpd_pulse and intel_dp_detect which contains
> > > > > > duplicates as well. Also intel_dp_detect is called
> > > > > > during modes enumeration as well which will result
> > > > > > in multiple dpcd operations. So this patch tries
> > > > > > to solve both these by bringing all DPCD operations
> > > > > > in one single function and make intel_dp_detect
> > > > > > use existing values instead of repeating same steps.
> > > > > > 
> > > > > > v2: Pulled in a hunk from last patch of the series to
> > > > > >       this patch. (Ander)
> > > > > > v3: Added MST hotplug handling. (Ander)
> > > > > > 
> > > > > > Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
> > > > > > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> > > > > > Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> > > > > > ---
> > > > > >    drivers/gpu/drm/i915/intel_dp.c | 71 +++++++++++++++++++++++++-----
> > > > > > -----
> > > > > > -----
> > > > > > -
> > > > > >    1 file changed, 44 insertions(+), 27 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > index 8969ff9..82ee18d 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > [...]
> > > > > 
> > > > > > @@ -4693,7 +4717,8 @@ intel_dp_detect(struct drm_connector *connector,
> > > > > > bool
> > > > > > force)
> > > > > >    		return connector_status_disconnected;
> > > > > >    	}
> > > > > >    
> > > > > > -	intel_dp_long_pulse(intel_dp->attached_connector);
> > > > > > +	if (force)
> > > > > > +		intel_dp_long_pulse(intel_dp->attached_connector);
> > > > > I didn't notice this at first, but force is not the right thing to check
> > > > > for
> > > > > here. It is basically intended to avoid doing load detection (see
> > > > > intel_get_load_detect_pipe()) on automated polling. But we still have to
> > > > > try
> > > > > detection here when force = false, otherwise this will cause a
> > > > > regression.
> > > > > 
> > > > > If you plug in a DP device while suspended, that device won't get
> > > > > detected,
> > > > > since we don't get an HPD for it. Previously, the call do
> > > > > intel_dp_detect()
> > > > > with
> > > > > force = false from intel_drm_resume() (via drm_helper_hpd_irq_event())
> > > > > would
> > > > > cause a full detection.
> > > > > 
> > > > > To avoid the repeated DPCD operations, I think we need a more explicit
> > > > > mechanism
> > > > > to signal that we already handled the long pulse via the HPD handler. In
> > > > > intel_dp_hpd_pulse() we could set a flag that tells we just handled a
> > > > > long
> > > > > pulse
> > > > > for the given port. The call to intel_dp_long_pulse() in
> > > > > intel_dp_detect()
> > > > > would
> > > > > then be dependent on that flag.
> > > > > 
> > > > > For that reason I have to retract my R-b from this patch.
> > > > > 
> > > > > Ander
> > > > Call to intel_dp_detect() from get_modes is with force set to true while
> > > > from resume the call is with force set to false.. It should be in the
> > > > opposite manner as get_modes should not require full detection whereas
> > > > resume should. So, this needs to be cleaned up there. After merge of
> > > > these patches, will look into cleaning up that part of the code.
> > > That really depends on what the force parameter is supposed to mean. The
> > > documentation states that "force is set to false whilst polling, true when
> > > checking the connector due to a user request". A look through git history
> > > shows
> > > the parameter was added to reduce time wasted doing load detection (doing a
> > > mode
> > > set in order to check if there is a device connected) for hardware that
> > > needs it
> > > (commit 7b334fcb45b7).
> > > 
> > > As far as I can tell, across all the drm drivers, that parameter is only
> > > used to
> > > avoid doing load detection.
> > > 
> > > Another thing to consider is that the driver may switch to polling if it
> > > detects
> > > an HPD storm. When the detect calls come from polling, the code in this
> > > patch
> > > will simply avoid any detection.
> > > 
> > hmm i think this discussion will prolong for a while :)
> > how about we call intel_dp_long_pulse() always for now.
> > this will be a compromise to not break any of the existing code
> > but will still result in getting a clean detection code, which
> > will can then be improved upon in the next iteration ?
> > i.e post the change it should look like. i.e skip this change alone
> > 
> > 	intel_dp_long_pulse(intel_dp->attached_connector);
> 
> Sounds good. The code gets cleaner and there is no regression in terms of
> repeated DPCD operations.

So this conversation seems to have had little bearing on reality,
especially in terms of how intel_dp_detect() is supposed to behave. This
patch is causing WARNs as it presumed that intel_dp_detect() is only
called after a modeset.

Should we send a revert to stable?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/5] drm/i915: Cleaning up intel_dp_hpd_pulse

[Shubhangi Shrivastava]

Current DP detection has DPCD operations split across
intel_dp_hpd_pulse and intel_dp_detect which contains
duplicates as well. Also intel_dp_detect is called
during modes enumeration as well which will result
in multiple dpcd operations. So this patch tries
to solve both these by bringing all DPCD operations
in one single function and make intel_dp_detect
use existing values instead of repeating same steps.

v2: Pulled in a hunk from last patch of the series to
    this patch. (Ander)

v3: Added MST hotplug handling. (Ander)

v4: Added a flag to check if detect is performed to
    prevent multiple detects on hotplug. (Ander)

Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 72 +++++++++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 2 files changed, 47 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b4cff63..0e16f74 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4582,6 +4582,16 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 		 */
 		status = connector_status_disconnected;
 		goto out;
+	} else if (connector->status == connector_status_connected) {
+		/*
+		 * If display was connected already and is still connected
+		 * check links status, there has been known issues of
+		 * link loss triggerring long pulse!!!!
+		 */
+		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+		intel_dp_check_link_status(intel_dp);
+		drm_modeset_unlock(&dev->mode_config.connection_mutex);
+		goto out;
 	}
 
 	/*
@@ -4595,6 +4605,7 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 	intel_dp_set_edid(intel_dp);
 
 	status = connector_status_connected;
+	intel_dp->detect_done = true;
 
 	/* Try to read the source of the interrupt */
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
@@ -4611,8 +4622,21 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 	}
 
 out:
-	if (status != connector_status_connected)
+	if (status != connector_status_connected) {
 		intel_dp_unset_edid(intel_dp);
+		/*
+		 * If we were in MST mode, and device is not there,
+		 * get out of MST mode
+		 */
+		if (intel_dp->is_mst) {
+			DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
+				intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
+			intel_dp->is_mst = false;
+			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
+							intel_dp->is_mst);
+		}
+	}
+
 	intel_display_power_put(to_i915(dev), power_domain);
 	return;
 }
@@ -4636,7 +4660,11 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 		return connector_status_disconnected;
 	}
 
-	intel_dp_long_pulse(intel_dp->attached_connector);
+	/* If full detect is not performed yet, do a full detect */
+	if (!intel_dp->detect_done)
+		intel_dp_long_pulse(intel_dp->attached_connector);
+
+	intel_dp->detect_done = false;
 
 	if (intel_connector->detect_edid)
 		return connector_status_connected;
@@ -4968,25 +4996,25 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 		/* indicate that we need to restart link training */
 		intel_dp->train_set_valid = false;
 
-		if (!intel_digital_port_connected(dev_priv, intel_dig_port))
-			goto mst_fail;
-
-		if (!intel_dp_get_dpcd(intel_dp)) {
-			goto mst_fail;
-		}
-
-		intel_dp_probe_oui(intel_dp);
+		intel_dp_long_pulse(intel_dp->attached_connector);
+		if (intel_dp->is_mst)
+			ret = IRQ_HANDLED;
+		goto put_power;
 
-		if (!intel_dp_probe_mst(intel_dp)) {
-			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-			intel_dp_check_link_status(intel_dp);
-			drm_modeset_unlock(&dev->mode_config.connection_mutex);
-			goto mst_fail;
-		}
 	} else {
 		if (intel_dp->is_mst) {
-			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
-				goto mst_fail;
+			if (intel_dp_check_mst_status(intel_dp) == -EINVAL) {
+				/*
+				 * If we were in MST mode, and device is not
+				 * there, get out of MST mode
+				 */
+				DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
+					intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
+				intel_dp->is_mst = false;
+				drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
+								intel_dp->is_mst);
+				goto put_power;
+			}
 		}
 
 		if (!intel_dp->is_mst) {
@@ -4998,14 +5026,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 
 	ret = IRQ_HANDLED;
 
-	goto put_power;
-mst_fail:
-	/* if we were in MST mode, and device is not there get out of MST mode */
-	if (intel_dp->is_mst) {
-		DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
-		intel_dp->is_mst = false;
-		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
-	}
 put_power:
 	intel_display_power_put(dev_priv, power_domain);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c87b450..3ce9391 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -797,6 +797,7 @@ struct intel_dp {
 	int link_rate;
 	uint8_t lane_count;
 	bool has_audio;
+	bool detect_done;
 	enum hdmi_force_audio force_audio;
 	bool limited_color_range;
 	bool color_range_auto;
-- 
2.6.1

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

[PATCH 2/5] drm/i915: Cleaning up intel_dp_hpd_pulse

[Shubhangi Shrivastava]

Current DP detection has DPCD operations split across
intel_dp_hpd_pulse and intel_dp_detect which contains
duplicates as well. Also intel_dp_detect is called
during modes enumeration as well which will result
in multiple dpcd operations. So this patch tries
to solve both these by bringing all DPCD operations
in one single function and make intel_dp_detect
use existing values instead of repeating same steps.

v2: Pulled in a hunk from last patch of the series to
    this patch. (Ander)

v3: Added MST hotplug handling. (Ander)

v4: Added a flag to check if detect is performed to
    prevent multiple detects on hotplug. (Ander)

Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 72 +++++++++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 2 files changed, 47 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index c632e2e..7c7be17 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4650,6 +4650,16 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 		 */
 		status = connector_status_disconnected;
 		goto out;
+	} else if (connector->status == connector_status_connected) {
+		/*
+		 * If display was connected already and is still connected
+		 * check links status, there has been known issues of
+		 * link loss triggerring long pulse!!!!
+		 */
+		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+		intel_dp_check_link_status(intel_dp);
+		drm_modeset_unlock(&dev->mode_config.connection_mutex);
+		goto out;
 	}
 
 	/*
@@ -4663,6 +4673,7 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 	intel_dp_set_edid(intel_dp);
 
 	status = connector_status_connected;
+	intel_dp->detect_done = true;
 
 	/* Try to read the source of the interrupt */
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
@@ -4679,8 +4690,21 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 	}
 
 out:
-	if (status != connector_status_connected)
+	if (status != connector_status_connected) {
 		intel_dp_unset_edid(intel_dp);
+		/*
+		 * If we were in MST mode, and device is not there,
+		 * get out of MST mode
+		 */
+		if (intel_dp->is_mst) {
+			DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
+				intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
+			intel_dp->is_mst = false;
+			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
+							intel_dp->is_mst);
+		}
+	}
+
 	intel_display_power_put(to_i915(dev), power_domain);
 	return;
 }
@@ -4704,7 +4728,11 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 		return connector_status_disconnected;
 	}
 
-	intel_dp_long_pulse(intel_dp->attached_connector);
+	/* If full detect is not performed yet, do a full detect */
+	if (!intel_dp->detect_done)
+		intel_dp_long_pulse(intel_dp->attached_connector);
+
+	intel_dp->detect_done = false;
 
 	if (intel_connector->detect_edid)
 		return connector_status_connected;
@@ -5036,25 +5064,25 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 		/* indicate that we need to restart link training */
 		intel_dp->train_set_valid = false;
 
-		if (!intel_digital_port_connected(dev_priv, intel_dig_port))
-			goto mst_fail;
-
-		if (!intel_dp_get_dpcd(intel_dp)) {
-			goto mst_fail;
-		}
-
-		intel_dp_probe_oui(intel_dp);
+		intel_dp_long_pulse(intel_dp->attached_connector);
+		if (intel_dp->is_mst)
+			ret = IRQ_HANDLED;
+		goto put_power;
 
-		if (!intel_dp_probe_mst(intel_dp)) {
-			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-			intel_dp_check_link_status(intel_dp);
-			drm_modeset_unlock(&dev->mode_config.connection_mutex);
-			goto mst_fail;
-		}
 	} else {
 		if (intel_dp->is_mst) {
-			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
-				goto mst_fail;
+			if (intel_dp_check_mst_status(intel_dp) == -EINVAL) {
+				/*
+				 * If we were in MST mode, and device is not
+				 * there, get out of MST mode
+				 */
+				DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
+					intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
+				intel_dp->is_mst = false;
+				drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
+								intel_dp->is_mst);
+				goto put_power;
+			}
 		}
 
 		if (!intel_dp->is_mst) {
@@ -5066,14 +5094,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 
 	ret = IRQ_HANDLED;
 
-	goto put_power;
-mst_fail:
-	/* if we were in MST mode, and device is not there get out of MST mode */
-	if (intel_dp->is_mst) {
-		DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
-		intel_dp->is_mst = false;
-		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
-	}
 put_power:
 	intel_display_power_put(dev_priv, power_domain);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3cae376..0a1bdfb 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -753,6 +753,7 @@ struct intel_dp {
 	int link_rate;
 	uint8_t lane_count;
 	bool has_audio;
+	bool detect_done;
 	enum hdmi_force_audio force_audio;
 	bool limited_color_range;
 	bool color_range_auto;
-- 
2.6.1

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

Re: [PATCH 2/5] drm/i915: Cleaning up intel_dp_hpd_pulse

[Ander Conselvan De Oliveira]

On Wed, 2016-02-10 at 14:34 +0530, Shubhangi Shrivastava wrote:
> Current DP detection has DPCD operations split across
> intel_dp_hpd_pulse and intel_dp_detect which contains
> duplicates as well. Also intel_dp_detect is called
> during modes enumeration as well which will result
> in multiple dpcd operations. So this patch tries
> to solve both these by bringing all DPCD operations
> in one single function and make intel_dp_detect
> use existing values instead of repeating same steps.
> 
> v2: Pulled in a hunk from last patch of the series to
>     this patch. (Ander)
> v3: Added MST hotplug handling. (Ander)

The change log for this version of the patch is missing. With that, this is

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

> 
> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 72 +++++++++++++++++++++++++--------------
> -
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  2 files changed, 47 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 042283a..ad5ec3b 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4653,6 +4653,16 @@ intel_dp_long_pulse(struct intel_connector
> *intel_connector)
>  		 */
>  		status = connector_status_disconnected;
>  		goto out;
> +	} else if (connector->status == connector_status_connected) {
> +		/*
> +		 * If display was connected already and is still connected
> +		 * check links status, there has been known issues of
> +		 * link loss triggerring long pulse!!!!
> +		 */
> +		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> +		intel_dp_check_link_status(intel_dp);
> +		drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +		goto out;
>  	}
>  
>  	/*
> @@ -4666,6 +4676,7 @@ intel_dp_long_pulse(struct intel_connector
> *intel_connector)
>  	intel_dp_set_edid(intel_dp);
>  
>  	status = connector_status_connected;
> +	intel_dp->detect_done = true;
>  
>  	/* Try to read the source of the interrupt */
>  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> @@ -4682,8 +4693,21 @@ intel_dp_long_pulse(struct intel_connector
> *intel_connector)
>  	}
>  
>  out:
> -	if (status != connector_status_connected)
> +	if (status != connector_status_connected) {
>  		intel_dp_unset_edid(intel_dp);
> +		/*
> +		 * If we were in MST mode, and device is not there,
> +		 * get out of MST mode
> +		 */
> +		if (intel_dp->is_mst) {
> +			DRM_DEBUG_KMS("MST device may have disappeared %d vs
> %d\n",
> +				intel_dp->is_mst, intel_dp
> ->mst_mgr.mst_state);
> +			intel_dp->is_mst = false;
> +			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> +							intel_dp->is_mst);
> +		}
> +	}
> +
>  	intel_display_power_put(to_i915(dev), power_domain);
>  	return;
>  }
> @@ -4707,7 +4731,11 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
>  		return connector_status_disconnected;
>  	}
>  
> -	intel_dp_long_pulse(intel_dp->attached_connector);
> +	/* If full detect is not performed yet, do a full detect */
> +	if (!intel_dp->detect_done)
> +		intel_dp_long_pulse(intel_dp->attached_connector);
> +
> +	intel_dp->detect_done = false;
>  
>  	if (intel_connector->detect_edid)
>  		return connector_status_connected;
> @@ -5040,25 +5068,25 @@ intel_dp_hpd_pulse(struct intel_digital_port
> *intel_dig_port, bool long_hpd)
>  		/* indicate that we need to restart link training */
>  		intel_dp->train_set_valid = false;
>  
> -		if (!intel_digital_port_connected(dev_priv, intel_dig_port))
> -			goto mst_fail;
> -
> -		if (!intel_dp_get_dpcd(intel_dp)) {
> -			goto mst_fail;
> -		}
> -
> -		intel_dp_probe_oui(intel_dp);
> +		intel_dp_long_pulse(intel_dp->attached_connector);
> +		if (intel_dp->is_mst)
> +			ret = IRQ_HANDLED;
> +		goto put_power;
>  
> -		if (!intel_dp_probe_mst(intel_dp)) {
> -			drm_modeset_lock(&dev->mode_config.connection_mutex,
> NULL);
> -			intel_dp_check_link_status(intel_dp);
> -			drm_modeset_unlock(&dev
> ->mode_config.connection_mutex);
> -			goto mst_fail;
> -		}
>  	} else {
>  		if (intel_dp->is_mst) {
> -			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
> -				goto mst_fail;
> +			if (intel_dp_check_mst_status(intel_dp) == -EINVAL) {
> +				/*
> +				 * If we were in MST mode, and device is not
> +				 * there, get out of MST mode
> +				 */
> +				DRM_DEBUG_KMS("MST device may have
> disappeared %d vs %d\n",
> +					intel_dp->is_mst, intel_dp
> ->mst_mgr.mst_state);
> +				intel_dp->is_mst = false;
> +				drm_dp_mst_topology_mgr_set_mst(&intel_dp
> ->mst_mgr,
> +								intel_dp
> ->is_mst);
> +				goto put_power;
> +			}
>  		}
>  
>  		if (!intel_dp->is_mst) {
> @@ -5070,14 +5098,6 @@ intel_dp_hpd_pulse(struct intel_digital_port
> *intel_dig_port, bool long_hpd)
>  
>  	ret = IRQ_HANDLED;
>  
> -	goto put_power;
> -mst_fail:
> -	/* if we were in MST mode, and device is not there get out of MST
> mode */
> -	if (intel_dp->is_mst) {
> -		DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
> intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
> -		intel_dp->is_mst = false;
> -		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp
> ->is_mst);
> -	}
>  put_power:
>  	intel_display_power_put(dev_priv, power_domain);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 878172a..3d003d6 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -753,6 +753,7 @@ struct intel_dp {
>  	int link_rate;
>  	uint8_t lane_count;
>  	bool has_audio;
> +	bool detect_done;
>  	enum hdmi_force_audio force_audio;
>  	bool limited_color_range;
>  	bool color_range_auto;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/5] drm/i915: Cleaning up intel_dp_hpd_pulse

[Shubhangi Shrivastava]

Current DP detection has DPCD operations split across
intel_dp_hpd_pulse and intel_dp_detect which contains
duplicates as well. Also intel_dp_detect is called
during modes enumeration as well which will result
in multiple dpcd operations. So this patch tries
to solve both these by bringing all DPCD operations
in one single function and make intel_dp_detect
use existing values instead of repeating same steps.

v2: Pulled in a hunk from last patch of the series to
    this patch. (Ander)
v3: Added MST hotplug handling. (Ander)

Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 72 +++++++++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 2 files changed, 47 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 042283a..ad5ec3b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4653,6 +4653,16 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 		 */
 		status = connector_status_disconnected;
 		goto out;
+	} else if (connector->status == connector_status_connected) {
+		/*
+		 * If display was connected already and is still connected
+		 * check links status, there has been known issues of
+		 * link loss triggerring long pulse!!!!
+		 */
+		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+		intel_dp_check_link_status(intel_dp);
+		drm_modeset_unlock(&dev->mode_config.connection_mutex);
+		goto out;
 	}
 
 	/*
@@ -4666,6 +4676,7 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 	intel_dp_set_edid(intel_dp);
 
 	status = connector_status_connected;
+	intel_dp->detect_done = true;
 
 	/* Try to read the source of the interrupt */
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
@@ -4682,8 +4693,21 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 	}
 
 out:
-	if (status != connector_status_connected)
+	if (status != connector_status_connected) {
 		intel_dp_unset_edid(intel_dp);
+		/*
+		 * If we were in MST mode, and device is not there,
+		 * get out of MST mode
+		 */
+		if (intel_dp->is_mst) {
+			DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
+				intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
+			intel_dp->is_mst = false;
+			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
+							intel_dp->is_mst);
+		}
+	}
+
 	intel_display_power_put(to_i915(dev), power_domain);
 	return;
 }
@@ -4707,7 +4731,11 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 		return connector_status_disconnected;
 	}
 
-	intel_dp_long_pulse(intel_dp->attached_connector);
+	/* If full detect is not performed yet, do a full detect */
+	if (!intel_dp->detect_done)
+		intel_dp_long_pulse(intel_dp->attached_connector);
+
+	intel_dp->detect_done = false;
 
 	if (intel_connector->detect_edid)
 		return connector_status_connected;
@@ -5040,25 +5068,25 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 		/* indicate that we need to restart link training */
 		intel_dp->train_set_valid = false;
 
-		if (!intel_digital_port_connected(dev_priv, intel_dig_port))
-			goto mst_fail;
-
-		if (!intel_dp_get_dpcd(intel_dp)) {
-			goto mst_fail;
-		}
-
-		intel_dp_probe_oui(intel_dp);
+		intel_dp_long_pulse(intel_dp->attached_connector);
+		if (intel_dp->is_mst)
+			ret = IRQ_HANDLED;
+		goto put_power;
 
-		if (!intel_dp_probe_mst(intel_dp)) {
-			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-			intel_dp_check_link_status(intel_dp);
-			drm_modeset_unlock(&dev->mode_config.connection_mutex);
-			goto mst_fail;
-		}
 	} else {
 		if (intel_dp->is_mst) {
-			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
-				goto mst_fail;
+			if (intel_dp_check_mst_status(intel_dp) == -EINVAL) {
+				/*
+				 * If we were in MST mode, and device is not
+				 * there, get out of MST mode
+				 */
+				DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
+					intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
+				intel_dp->is_mst = false;
+				drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
+								intel_dp->is_mst);
+				goto put_power;
+			}
 		}
 
 		if (!intel_dp->is_mst) {
@@ -5070,14 +5098,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 
 	ret = IRQ_HANDLED;
 
-	goto put_power;
-mst_fail:
-	/* if we were in MST mode, and device is not there get out of MST mode */
-	if (intel_dp->is_mst) {
-		DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
-		intel_dp->is_mst = false;
-		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
-	}
 put_power:
 	intel_display_power_put(dev_priv, power_domain);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 878172a..3d003d6 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -753,6 +753,7 @@ struct intel_dp {
 	int link_rate;
 	uint8_t lane_count;
 	bool has_audio;
+	bool detect_done;
 	enum hdmi_force_audio force_audio;
 	bool limited_color_range;
 	bool color_range_auto;
-- 
2.6.1

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

Re: [PATCH 2/5] drm/i915: Cleaning up intel_dp_hpd_pulse

[Ander Conselvan De Oliveira]

On Mon, 2016-02-01 at 11:50 +0530, Thulasimani, Sivakumar wrote:
> 
> On 1/29/2016 5:33 PM, Ander Conselvan De Oliveira wrote:
> > On Fri, 2016-01-29 at 14:31 +0530, Shubhangi Shrivastava wrote:
> > > On Tuesday 26 January 2016 06:52 PM, Ander Conselvan De Oliveira wrote:
> > > > On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote:
> > > > > Current DP detection has DPCD operations split across
> > > > > intel_dp_hpd_pulse and intel_dp_detect which contains
> > > > > duplicates as well. Also intel_dp_detect is called
> > > > > during modes enumeration as well which will result
> > > > > in multiple dpcd operations. So this patch tries
> > > > > to solve both these by bringing all DPCD operations
> > > > > in one single function and make intel_dp_detect
> > > > > use existing values instead of repeating same steps.
> > > > > 
> > > > > v2: Pulled in a hunk from last patch of the series to
> > > > >       this patch. (Ander)
> > > > > v3: Added MST hotplug handling. (Ander)
> > > > > 
> > > > > Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
> > > > > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> > > > > Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> > > > > ---
> > > > >    drivers/gpu/drm/i915/intel_dp.c | 71 +++++++++++++++++++++++++-----
> > > > > -----
> > > > > -----
> > > > > -
> > > > >    1 file changed, 44 insertions(+), 27 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > index 8969ff9..82ee18d 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > [...]
> > > > 
> > > > > @@ -4693,7 +4717,8 @@ intel_dp_detect(struct drm_connector *connector,
> > > > > bool
> > > > > force)
> > > > >    		return connector_status_disconnected;
> > > > >    	}
> > > > >    
> > > > > -	intel_dp_long_pulse(intel_dp->attached_connector);
> > > > > +	if (force)
> > > > > +		intel_dp_long_pulse(intel_dp->attached_connector);
> > > > I didn't notice this at first, but force is not the right thing to check
> > > > for
> > > > here. It is basically intended to avoid doing load detection (see
> > > > intel_get_load_detect_pipe()) on automated polling. But we still have to
> > > > try
> > > > detection here when force = false, otherwise this will cause a
> > > > regression.
> > > > 
> > > > If you plug in a DP device while suspended, that device won't get
> > > > detected,
> > > > since we don't get an HPD for it. Previously, the call do
> > > > intel_dp_detect()
> > > > with
> > > > force = false from intel_drm_resume() (via drm_helper_hpd_irq_event())
> > > > would
> > > > cause a full detection.
> > > > 
> > > > To avoid the repeated DPCD operations, I think we need a more explicit
> > > > mechanism
> > > > to signal that we already handled the long pulse via the HPD handler. In
> > > > intel_dp_hpd_pulse() we could set a flag that tells we just handled a
> > > > long
> > > > pulse
> > > > for the given port. The call to intel_dp_long_pulse() in
> > > > intel_dp_detect()
> > > > would
> > > > then be dependent on that flag.
> > > > 
> > > > For that reason I have to retract my R-b from this patch.
> > > > 
> > > > Ander
> > > Call to intel_dp_detect() from get_modes is with force set to true while
> > > from resume the call is with force set to false.. It should be in the
> > > opposite manner as get_modes should not require full detection whereas
> > > resume should. So, this needs to be cleaned up there. After merge of
> > > these patches, will look into cleaning up that part of the code.
> > That really depends on what the force parameter is supposed to mean. The
> > documentation states that "force is set to false whilst polling, true when
> > checking the connector due to a user request". A look through git history
> > shows
> > the parameter was added to reduce time wasted doing load detection (doing a
> > mode
> > set in order to check if there is a device connected) for hardware that
> > needs it
> > (commit 7b334fcb45b7).
> > 
> > As far as I can tell, across all the drm drivers, that parameter is only
> > used to
> > avoid doing load detection.
> > 
> > Another thing to consider is that the driver may switch to polling if it
> > detects
> > an HPD storm. When the detect calls come from polling, the code in this
> > patch
> > will simply avoid any detection.
> > 
> hmm i think this discussion will prolong for a while :)
> how about we call intel_dp_long_pulse() always for now.
> this will be a compromise to not break any of the existing code
> but will still result in getting a clean detection code, which
> will can then be improved upon in the next iteration ?
> i.e post the change it should look like. i.e skip this change alone
> 
> 	intel_dp_long_pulse(intel_dp->attached_connector);

Sounds good. The code gets cleaner and there is no regression in terms of
repeated DPCD operations.

Ander

> 
> 
> regards,
> Sivakumar
> > > Moreover, intel_dp_detect() will be called from
> > > drm_helper_hpd_irq_event() in polling scenarios only (when
> > > DRM_CONNECTOR_POLL_HPD flag is set in connector->polled). So, seems like
> > > this code here, doesn't really create a regression for realtime scenarios.
> > I don't know what you mean by realtime scenarios, but the regression is very
> > real. Using a kernel with your patches applied, suspend while there is no DP
> > monitor attached, attach the monitor while suspended and then wake up.
> > Notice
> > how the connector state doesn't change. You can check the i915_display_info
> > file
> > in debugfs, for instance.
> > 
> > 
> > Ander
> > 
> > > >    
> > > > >    	if (intel_connector->detect_edid)
> > > > >    		return connector_status_connected;
> > > > > @@ -5026,25 +5051,25 @@ intel_dp_hpd_pulse(struct intel_digital_port
> > > > > *intel_dig_port, bool long_hpd)
> > > > >    		/* indicate that we need to restart link training
> > > > > */
> > > > >    		intel_dp->train_set_valid = false;
> > > > >    
> > > > > -		if (!intel_digital_port_connected(dev_priv,
> > > > > intel_dig_port))
> > > > > -			goto mst_fail;
> > > > > -
> > > > > -		if (!intel_dp_get_dpcd(intel_dp)) {
> > > > > -			goto mst_fail;
> > > > > -		}
> > > > > -
> > > > > -		intel_dp_probe_oui(intel_dp);
> > > > > +		intel_dp_long_pulse(intel_dp->attached_connector);
> > > > > +		if (intel_dp->is_mst)
> > > > > +			ret = IRQ_HANDLED;
> > > > > +		goto put_power;
> > > > >    
> > > > > -		if (!intel_dp_probe_mst(intel_dp)) {
> > > > > -			drm_modeset_lock(&dev
> > > > > ->mode_config.connection_mutex,
> > > > > NULL);
> > > > > -			intel_dp_check_link_status(intel_dp);
> > > > > -			drm_modeset_unlock(&dev
> > > > > ->mode_config.connection_mutex);
> > > > > -			goto mst_fail;
> > > > > -		}
> > > > >    	} else {
> > > > >    		if (intel_dp->is_mst) {
> > > > > -			if (intel_dp_check_mst_status(intel_dp) ==
> > > > > -EINVAL)
> > > > > -				goto mst_fail;
> > > > > +			if (intel_dp_check_mst_status(intel_dp) ==
> > > > > -EINVAL) {
> > > > > +				/*
> > > > > +				 * If we were in MST mode, and device
> > > > > is
> > > > > not
> > > > > +				 * there, get out of MST mode
> > > > > +				 */
> > > > > +				DRM_DEBUG_KMS("MST device may have
> > > > > disappeared %d vs %d\n",
> > > > > +					intel_dp->is_mst, intel_dp
> > > > > ->mst_mgr.mst_state);
> > > > > +				intel_dp->is_mst = false;
> > > > > +				drm_dp_mst_topology_mgr_set_mst(&inte
> > > > > l_dp
> > > > > ->mst_mgr,
> > > > > +								intel
> > > > > _dp
> > > > > ->is_mst);
> > > > > +				goto put_power;
> > > > > +			}
> > > > >    		}
> > > > >    
> > > > >    		if (!intel_dp->is_mst) {
> > > > > @@ -5056,14 +5081,6 @@ intel_dp_hpd_pulse(struct intel_digital_port
> > > > > *intel_dig_port, bool long_hpd)
> > > > >    
> > > > >    	ret = IRQ_HANDLED;
> > > > >    
> > > > > -	goto put_power;
> > > > > -mst_fail:
> > > > > -	/* if we were in MST mode, and device is not there get out of
> > > > > MST
> > > > > mode */
> > > > > -	if (intel_dp->is_mst) {
> > > > > -		DRM_DEBUG_KMS("MST device may have disappeared %d vs
> > > > > %d\n",
> > > > > intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
> > > > > -		intel_dp->is_mst = false;
> > > > > -		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> > > > > intel_dp
> > > > > ->is_mst);
> > > > > -	}
> > > > >    put_power:
> > > > >    	intel_display_power_put(dev_priv, power_domain);
> > > > >    
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915: Cleaning up intel_dp_hpd_pulse

[Thulasimani, Sivakumar]

On 1/29/2016 5:33 PM, Ander Conselvan De Oliveira wrote:
> On Fri, 2016-01-29 at 14:31 +0530, Shubhangi Shrivastava wrote:
>> On Tuesday 26 January 2016 06:52 PM, Ander Conselvan De Oliveira wrote:
>>> On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote:
>>>> Current DP detection has DPCD operations split across
>>>> intel_dp_hpd_pulse and intel_dp_detect which contains
>>>> duplicates as well. Also intel_dp_detect is called
>>>> during modes enumeration as well which will result
>>>> in multiple dpcd operations. So this patch tries
>>>> to solve both these by bringing all DPCD operations
>>>> in one single function and make intel_dp_detect
>>>> use existing values instead of repeating same steps.
>>>>
>>>> v2: Pulled in a hunk from last patch of the series to
>>>>       this patch. (Ander)
>>>> v3: Added MST hotplug handling. (Ander)
>>>>
>>>> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
>>>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>>>> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_dp.c | 71 +++++++++++++++++++++++++----------
>>>> -----
>>>> -
>>>>    1 file changed, 44 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>>> b/drivers/gpu/drm/i915/intel_dp.c
>>>> index 8969ff9..82ee18d 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> [...]
>>>
>>>> @@ -4693,7 +4717,8 @@ intel_dp_detect(struct drm_connector *connector,
>>>> bool
>>>> force)
>>>>    		return connector_status_disconnected;
>>>>    	}
>>>>    
>>>> -	intel_dp_long_pulse(intel_dp->attached_connector);
>>>> +	if (force)
>>>> +		intel_dp_long_pulse(intel_dp->attached_connector);
>>> I didn't notice this at first, but force is not the right thing to check for
>>> here. It is basically intended to avoid doing load detection (see
>>> intel_get_load_detect_pipe()) on automated polling. But we still have to try
>>> detection here when force = false, otherwise this will cause a regression.
>>>
>>> If you plug in a DP device while suspended, that device won't get detected,
>>> since we don't get an HPD for it. Previously, the call do intel_dp_detect()
>>> with
>>> force = false from intel_drm_resume() (via drm_helper_hpd_irq_event()) would
>>> cause a full detection.
>>>
>>> To avoid the repeated DPCD operations, I think we need a more explicit
>>> mechanism
>>> to signal that we already handled the long pulse via the HPD handler. In
>>> intel_dp_hpd_pulse() we could set a flag that tells we just handled a long
>>> pulse
>>> for the given port. The call to intel_dp_long_pulse() in intel_dp_detect()
>>> would
>>> then be dependent on that flag.
>>>
>>> For that reason I have to retract my R-b from this patch.
>>>
>>> Ander
>> Call to intel_dp_detect() from get_modes is with force set to true while
>> from resume the call is with force set to false.. It should be in the
>> opposite manner as get_modes should not require full detection whereas
>> resume should. So, this needs to be cleaned up there. After merge of
>> these patches, will look into cleaning up that part of the code.
> That really depends on what the force parameter is supposed to mean. The
> documentation states that "force is set to false whilst polling, true when
> checking the connector due to a user request". A look through git history shows
> the parameter was added to reduce time wasted doing load detection (doing a mode
> set in order to check if there is a device connected) for hardware that needs it
> (commit 7b334fcb45b7).
>
> As far as I can tell, across all the drm drivers, that parameter is only used to
> avoid doing load detection.
>
> Another thing to consider is that the driver may switch to polling if it detects
> an HPD storm. When the detect calls come from polling, the code in this patch
> will simply avoid any detection.
>
hmm i think this discussion will prolong for a while :)
how about we call intel_dp_long_pulse() always for now.
this will be a compromise to not break any of the existing code
but will still result in getting a clean detection code, which
will can then be improved upon in the next iteration ?
i.e post the change it should look like. i.e skip this change alone

	intel_dp_long_pulse(intel_dp->attached_connector);


regards,
Sivakumar
>> Moreover, intel_dp_detect() will be called from
>> drm_helper_hpd_irq_event() in polling scenarios only (when
>> DRM_CONNECTOR_POLL_HPD flag is set in connector->polled). So, seems like
>> this code here, doesn't really create a regression for realtime scenarios.
> I don't know what you mean by realtime scenarios, but the regression is very
> real. Using a kernel with your patches applied, suspend while there is no DP
> monitor attached, attach the monitor while suspended and then wake up. Notice
> how the connector state doesn't change. You can check the i915_display_info file
> in debugfs, for instance.
>
>
> Ander
>
>>>    
>>>>    	if (intel_connector->detect_edid)
>>>>    		return connector_status_connected;
>>>> @@ -5026,25 +5051,25 @@ intel_dp_hpd_pulse(struct intel_digital_port
>>>> *intel_dig_port, bool long_hpd)
>>>>    		/* indicate that we need to restart link training */
>>>>    		intel_dp->train_set_valid = false;
>>>>    
>>>> -		if (!intel_digital_port_connected(dev_priv,
>>>> intel_dig_port))
>>>> -			goto mst_fail;
>>>> -
>>>> -		if (!intel_dp_get_dpcd(intel_dp)) {
>>>> -			goto mst_fail;
>>>> -		}
>>>> -
>>>> -		intel_dp_probe_oui(intel_dp);
>>>> +		intel_dp_long_pulse(intel_dp->attached_connector);
>>>> +		if (intel_dp->is_mst)
>>>> +			ret = IRQ_HANDLED;
>>>> +		goto put_power;
>>>>    
>>>> -		if (!intel_dp_probe_mst(intel_dp)) {
>>>> -			drm_modeset_lock(&dev
>>>> ->mode_config.connection_mutex,
>>>> NULL);
>>>> -			intel_dp_check_link_status(intel_dp);
>>>> -			drm_modeset_unlock(&dev
>>>> ->mode_config.connection_mutex);
>>>> -			goto mst_fail;
>>>> -		}
>>>>    	} else {
>>>>    		if (intel_dp->is_mst) {
>>>> -			if (intel_dp_check_mst_status(intel_dp) ==
>>>> -EINVAL)
>>>> -				goto mst_fail;
>>>> +			if (intel_dp_check_mst_status(intel_dp) ==
>>>> -EINVAL) {
>>>> +				/*
>>>> +				 * If we were in MST mode, and device is
>>>> not
>>>> +				 * there, get out of MST mode
>>>> +				 */
>>>> +				DRM_DEBUG_KMS("MST device may have
>>>> disappeared %d vs %d\n",
>>>> +					intel_dp->is_mst, intel_dp
>>>> ->mst_mgr.mst_state);
>>>> +				intel_dp->is_mst = false;
>>>> +				drm_dp_mst_topology_mgr_set_mst(&intel_dp
>>>> ->mst_mgr,
>>>> +								intel_dp
>>>> ->is_mst);
>>>> +				goto put_power;
>>>> +			}
>>>>    		}
>>>>    
>>>>    		if (!intel_dp->is_mst) {
>>>> @@ -5056,14 +5081,6 @@ intel_dp_hpd_pulse(struct intel_digital_port
>>>> *intel_dig_port, bool long_hpd)
>>>>    
>>>>    	ret = IRQ_HANDLED;
>>>>    
>>>> -	goto put_power;
>>>> -mst_fail:
>>>> -	/* if we were in MST mode, and device is not there get out of MST
>>>> mode */
>>>> -	if (intel_dp->is_mst) {
>>>> -		DRM_DEBUG_KMS("MST device may have disappeared %d vs
>>>> %d\n",
>>>> intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
>>>> -		intel_dp->is_mst = false;
>>>> -		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
>>>> intel_dp
>>>> ->is_mst);
>>>> -	}
>>>>    put_power:
>>>>    	intel_display_power_put(dev_priv, power_domain);
>>>>    
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/5] drm/i915: Cleaning up intel_dp_hpd_pulse

[Ander Conselvan De Oliveira]

On Fri, 2016-01-29 at 14:31 +0530, Shubhangi Shrivastava wrote:
> 
> On Tuesday 26 January 2016 06:52 PM, Ander Conselvan De Oliveira wrote:
> > On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote:
> > > Current DP detection has DPCD operations split across
> > > intel_dp_hpd_pulse and intel_dp_detect which contains
> > > duplicates as well. Also intel_dp_detect is called
> > > during modes enumeration as well which will result
> > > in multiple dpcd operations. So this patch tries
> > > to solve both these by bringing all DPCD operations
> > > in one single function and make intel_dp_detect
> > > use existing values instead of repeating same steps.
> > > 
> > > v2: Pulled in a hunk from last patch of the series to
> > >      this patch. (Ander)
> > > v3: Added MST hotplug handling. (Ander)
> > > 
> > > Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
> > > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> > > Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/intel_dp.c | 71 +++++++++++++++++++++++++----------
> > > -----
> > > -
> > >   1 file changed, 44 insertions(+), 27 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index 8969ff9..82ee18d 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > [...]
> > 
> > > @@ -4693,7 +4717,8 @@ intel_dp_detect(struct drm_connector *connector,
> > > bool
> > > force)
> > >   		return connector_status_disconnected;
> > >   	}
> > >   
> > > -	intel_dp_long_pulse(intel_dp->attached_connector);
> > > +	if (force)
> > > +		intel_dp_long_pulse(intel_dp->attached_connector);
> > I didn't notice this at first, but force is not the right thing to check for
> > here. It is basically intended to avoid doing load detection (see
> > intel_get_load_detect_pipe()) on automated polling. But we still have to try
> > detection here when force = false, otherwise this will cause a regression.
> > 
> > If you plug in a DP device while suspended, that device won't get detected,
> > since we don't get an HPD for it. Previously, the call do intel_dp_detect()
> > with
> > force = false from intel_drm_resume() (via drm_helper_hpd_irq_event()) would
> > cause a full detection.
> > 
> > To avoid the repeated DPCD operations, I think we need a more explicit
> > mechanism
> > to signal that we already handled the long pulse via the HPD handler. In
> > intel_dp_hpd_pulse() we could set a flag that tells we just handled a long
> > pulse
> > for the given port. The call to intel_dp_long_pulse() in intel_dp_detect()
> > would
> > then be dependent on that flag.
> > 
> > For that reason I have to retract my R-b from this patch.
> > 
> > Ander
> 
> Call to intel_dp_detect() from get_modes is with force set to true while 
> from resume the call is with force set to false.. It should be in the 
> opposite manner as get_modes should not require full detection whereas 
> resume should. So, this needs to be cleaned up there. After merge of 
> these patches, will look into cleaning up that part of the code.

That really depends on what the force parameter is supposed to mean. The
documentation states that "force is set to false whilst polling, true when
checking the connector due to a user request". A look through git history shows
the parameter was added to reduce time wasted doing load detection (doing a mode
set in order to check if there is a device connected) for hardware that needs it
(commit 7b334fcb45b7).

As far as I can tell, across all the drm drivers, that parameter is only used to
avoid doing load detection.

Another thing to consider is that the driver may switch to polling if it detects
an HPD storm. When the detect calls come from polling, the code in this patch
will simply avoid any detection.


> Moreover, intel_dp_detect() will be called from 
> drm_helper_hpd_irq_event() in polling scenarios only (when 
> DRM_CONNECTOR_POLL_HPD flag is set in connector->polled). So, seems like 
> this code here, doesn't really create a regression for realtime scenarios.

I don't know what you mean by realtime scenarios, but the regression is very
real. Using a kernel with your patches applied, suspend while there is no DP
monitor attached, attach the monitor while suspended and then wake up. Notice
how the connector state doesn't change. You can check the i915_display_info file
in debugfs, for instance.


Ander

> >   
> > >   	if (intel_connector->detect_edid)
> > >   		return connector_status_connected;
> > > @@ -5026,25 +5051,25 @@ intel_dp_hpd_pulse(struct intel_digital_port
> > > *intel_dig_port, bool long_hpd)
> > >   		/* indicate that we need to restart link training */
> > >   		intel_dp->train_set_valid = false;
> > >   
> > > -		if (!intel_digital_port_connected(dev_priv,
> > > intel_dig_port))
> > > -			goto mst_fail;
> > > -
> > > -		if (!intel_dp_get_dpcd(intel_dp)) {
> > > -			goto mst_fail;
> > > -		}
> > > -
> > > -		intel_dp_probe_oui(intel_dp);
> > > +		intel_dp_long_pulse(intel_dp->attached_connector);
> > > +		if (intel_dp->is_mst)
> > > +			ret = IRQ_HANDLED;
> > > +		goto put_power;
> > >   
> > > -		if (!intel_dp_probe_mst(intel_dp)) {
> > > -			drm_modeset_lock(&dev
> > > ->mode_config.connection_mutex,
> > > NULL);
> > > -			intel_dp_check_link_status(intel_dp);
> > > -			drm_modeset_unlock(&dev
> > > ->mode_config.connection_mutex);
> > > -			goto mst_fail;
> > > -		}
> > >   	} else {
> > >   		if (intel_dp->is_mst) {
> > > -			if (intel_dp_check_mst_status(intel_dp) == 
> > > -EINVAL)
> > > -				goto mst_fail;
> > > +			if (intel_dp_check_mst_status(intel_dp) == 
> > > -EINVAL) {
> > > +				/*
> > > +				 * If we were in MST mode, and device is
> > > not
> > > +				 * there, get out of MST mode
> > > +				 */
> > > +				DRM_DEBUG_KMS("MST device may have
> > > disappeared %d vs %d\n",
> > > +					intel_dp->is_mst, intel_dp
> > > ->mst_mgr.mst_state);
> > > +				intel_dp->is_mst = false;
> > > +				drm_dp_mst_topology_mgr_set_mst(&intel_dp
> > > ->mst_mgr,
> > > +								intel_dp
> > > ->is_mst);
> > > +				goto put_power;
> > > +			}
> > >   		}
> > >   
> > >   		if (!intel_dp->is_mst) {
> > > @@ -5056,14 +5081,6 @@ intel_dp_hpd_pulse(struct intel_digital_port
> > > *intel_dig_port, bool long_hpd)
> > >   
> > >   	ret = IRQ_HANDLED;
> > >   
> > > -	goto put_power;
> > > -mst_fail:
> > > -	/* if we were in MST mode, and device is not there get out of MST
> > > mode */
> > > -	if (intel_dp->is_mst) {
> > > -		DRM_DEBUG_KMS("MST device may have disappeared %d vs
> > > %d\n",
> > > intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
> > > -		intel_dp->is_mst = false;
> > > -		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> > > intel_dp
> > > ->is_mst);
> > > -	}
> > >   put_power:
> > >   	intel_display_power_put(dev_priv, power_domain);
> > >   
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915: Cleaning up intel_dp_hpd_pulse

[Shubhangi Shrivastava]

On Tuesday 26 January 2016 06:52 PM, Ander Conselvan De Oliveira wrote:
> On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote:
>> Current DP detection has DPCD operations split across
>> intel_dp_hpd_pulse and intel_dp_detect which contains
>> duplicates as well. Also intel_dp_detect is called
>> during modes enumeration as well which will result
>> in multiple dpcd operations. So this patch tries
>> to solve both these by bringing all DPCD operations
>> in one single function and make intel_dp_detect
>> use existing values instead of repeating same steps.
>>
>> v2: Pulled in a hunk from last patch of the series to
>>      this patch. (Ander)
>> v3: Added MST hotplug handling. (Ander)
>>
>> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c | 71 +++++++++++++++++++++++++---------------
>> -
>>   1 file changed, 44 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 8969ff9..82ee18d 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
> [...]
>
>> @@ -4693,7 +4717,8 @@ intel_dp_detect(struct drm_connector *connector, bool
>> force)
>>   		return connector_status_disconnected;
>>   	}
>>   
>> -	intel_dp_long_pulse(intel_dp->attached_connector);
>> +	if (force)
>> +		intel_dp_long_pulse(intel_dp->attached_connector);
> I didn't notice this at first, but force is not the right thing to check for
> here. It is basically intended to avoid doing load detection (see
> intel_get_load_detect_pipe()) on automated polling. But we still have to try
> detection here when force = false, otherwise this will cause a regression.
>
> If you plug in a DP device while suspended, that device won't get detected,
> since we don't get an HPD for it. Previously, the call do intel_dp_detect() with
> force = false from intel_drm_resume() (via drm_helper_hpd_irq_event()) would
> cause a full detection.
>
> To avoid the repeated DPCD operations, I think we need a more explicit mechanism
> to signal that we already handled the long pulse via the HPD handler. In
> intel_dp_hpd_pulse() we could set a flag that tells we just handled a long pulse
> for the given port. The call to intel_dp_long_pulse() in intel_dp_detect() would
> then be dependent on that flag.
>
> For that reason I have to retract my R-b from this patch.
>
> Ander

Call to intel_dp_detect() from get_modes is with force set to true while 
from resume the call is with force set to false.. It should be in the 
opposite manner as get_modes should not require full detection whereas 
resume should. So, this needs to be cleaned up there. After merge of 
these patches, will look into cleaning up that part of the code.

Moreover, intel_dp_detect() will be called from 
drm_helper_hpd_irq_event() in polling scenarios only (when 
DRM_CONNECTOR_POLL_HPD flag is set in connector->polled). So, seems like 
this code here, doesn't really create a regression for realtime scenarios.

>   
>>   	if (intel_connector->detect_edid)
>>   		return connector_status_connected;
>> @@ -5026,25 +5051,25 @@ intel_dp_hpd_pulse(struct intel_digital_port
>> *intel_dig_port, bool long_hpd)
>>   		/* indicate that we need to restart link training */
>>   		intel_dp->train_set_valid = false;
>>   
>> -		if (!intel_digital_port_connected(dev_priv, intel_dig_port))
>> -			goto mst_fail;
>> -
>> -		if (!intel_dp_get_dpcd(intel_dp)) {
>> -			goto mst_fail;
>> -		}
>> -
>> -		intel_dp_probe_oui(intel_dp);
>> +		intel_dp_long_pulse(intel_dp->attached_connector);
>> +		if (intel_dp->is_mst)
>> +			ret = IRQ_HANDLED;
>> +		goto put_power;
>>   
>> -		if (!intel_dp_probe_mst(intel_dp)) {
>> -			drm_modeset_lock(&dev->mode_config.connection_mutex,
>> NULL);
>> -			intel_dp_check_link_status(intel_dp);
>> -			drm_modeset_unlock(&dev
>> ->mode_config.connection_mutex);
>> -			goto mst_fail;
>> -		}
>>   	} else {
>>   		if (intel_dp->is_mst) {
>> -			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
>> -				goto mst_fail;
>> +			if (intel_dp_check_mst_status(intel_dp) == -EINVAL) {
>> +				/*
>> +				 * If we were in MST mode, and device is not
>> +				 * there, get out of MST mode
>> +				 */
>> +				DRM_DEBUG_KMS("MST device may have
>> disappeared %d vs %d\n",
>> +					intel_dp->is_mst, intel_dp
>> ->mst_mgr.mst_state);
>> +				intel_dp->is_mst = false;
>> +				drm_dp_mst_topology_mgr_set_mst(&intel_dp
>> ->mst_mgr,
>> +								intel_dp
>> ->is_mst);
>> +				goto put_power;
>> +			}
>>   		}
>>   
>>   		if (!intel_dp->is_mst) {
>> @@ -5056,14 +5081,6 @@ intel_dp_hpd_pulse(struct intel_digital_port
>> *intel_dig_port, bool long_hpd)
>>   
>>   	ret = IRQ_HANDLED;
>>   
>> -	goto put_power;
>> -mst_fail:
>> -	/* if we were in MST mode, and device is not there get out of MST
>> mode */
>> -	if (intel_dp->is_mst) {
>> -		DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
>> intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
>> -		intel_dp->is_mst = false;
>> -		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp
>> ->is_mst);
>> -	}
>>   put_power:
>>   	intel_display_power_put(dev_priv, power_domain);
>>   

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

Re: [PATCH 2/5] drm/i915: Cleaning up intel_dp_hpd_pulse

[Ander Conselvan De Oliveira]

On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote:
> Current DP detection has DPCD operations split across
> intel_dp_hpd_pulse and intel_dp_detect which contains
> duplicates as well. Also intel_dp_detect is called
> during modes enumeration as well which will result
> in multiple dpcd operations. So this patch tries
> to solve both these by bringing all DPCD operations
> in one single function and make intel_dp_detect
> use existing values instead of repeating same steps.
> 
> v2: Pulled in a hunk from last patch of the series to
>     this patch. (Ander)
> v3: Added MST hotplug handling. (Ander)
> 
> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 71 +++++++++++++++++++++++++---------------
> -
>  1 file changed, 44 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 8969ff9..82ee18d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c

[...]

> @@ -4693,7 +4717,8 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
>  		return connector_status_disconnected;
>  	}
>  
> -	intel_dp_long_pulse(intel_dp->attached_connector);
> +	if (force)
> +		intel_dp_long_pulse(intel_dp->attached_connector);

I didn't notice this at first, but force is not the right thing to check for
here. It is basically intended to avoid doing load detection (see
intel_get_load_detect_pipe()) on automated polling. But we still have to try
detection here when force = false, otherwise this will cause a regression.

If you plug in a DP device while suspended, that device won't get detected,
since we don't get an HPD for it. Previously, the call do intel_dp_detect() with
force = false from intel_drm_resume() (via drm_helper_hpd_irq_event()) would
cause a full detection.

To avoid the repeated DPCD operations, I think we need a more explicit mechanism
to signal that we already handled the long pulse via the HPD handler. In
intel_dp_hpd_pulse() we could set a flag that tells we just handled a long pulse
for the given port. The call to intel_dp_long_pulse() in intel_dp_detect() would
then be dependent on that flag.

For that reason I have to retract my R-b from this patch.

Ander
 
>  	if (intel_connector->detect_edid)
>  		return connector_status_connected;
> @@ -5026,25 +5051,25 @@ intel_dp_hpd_pulse(struct intel_digital_port
> *intel_dig_port, bool long_hpd)
>  		/* indicate that we need to restart link training */
>  		intel_dp->train_set_valid = false;
>  
> -		if (!intel_digital_port_connected(dev_priv, intel_dig_port))
> -			goto mst_fail;
> -
> -		if (!intel_dp_get_dpcd(intel_dp)) {
> -			goto mst_fail;
> -		}
> -
> -		intel_dp_probe_oui(intel_dp);
> +		intel_dp_long_pulse(intel_dp->attached_connector);
> +		if (intel_dp->is_mst)
> +			ret = IRQ_HANDLED;
> +		goto put_power;
>  
> -		if (!intel_dp_probe_mst(intel_dp)) {
> -			drm_modeset_lock(&dev->mode_config.connection_mutex,
> NULL);
> -			intel_dp_check_link_status(intel_dp);
> -			drm_modeset_unlock(&dev
> ->mode_config.connection_mutex);
> -			goto mst_fail;
> -		}
>  	} else {
>  		if (intel_dp->is_mst) {
> -			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
> -				goto mst_fail;
> +			if (intel_dp_check_mst_status(intel_dp) == -EINVAL) {
> +				/*
> +				 * If we were in MST mode, and device is not
> +				 * there, get out of MST mode
> +				 */
> +				DRM_DEBUG_KMS("MST device may have
> disappeared %d vs %d\n",
> +					intel_dp->is_mst, intel_dp
> ->mst_mgr.mst_state);
> +				intel_dp->is_mst = false;
> +				drm_dp_mst_topology_mgr_set_mst(&intel_dp
> ->mst_mgr,
> +								intel_dp
> ->is_mst);
> +				goto put_power;
> +			}
>  		}
>  
>  		if (!intel_dp->is_mst) {
> @@ -5056,14 +5081,6 @@ intel_dp_hpd_pulse(struct intel_digital_port
> *intel_dig_port, bool long_hpd)
>  
>  	ret = IRQ_HANDLED;
>  
> -	goto put_power;
> -mst_fail:
> -	/* if we were in MST mode, and device is not there get out of MST
> mode */
> -	if (intel_dp->is_mst) {
> -		DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
> intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
> -		intel_dp->is_mst = false;
> -		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp
> ->is_mst);
> -	}
>  put_power:
>  	intel_display_power_put(dev_priv, power_domain);
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915: Cleaning up intel_dp_hpd_pulse

[Ander Conselvan De Oliveira]

On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote:
> Current DP detection has DPCD operations split across
> intel_dp_hpd_pulse and intel_dp_detect which contains
> duplicates as well. Also intel_dp_detect is called
> during modes enumeration as well which will result
> in multiple dpcd operations. So this patch tries
> to solve both these by bringing all DPCD operations
> in one single function and make intel_dp_detect
> use existing values instead of repeating same steps.
> 
> v2: Pulled in a hunk from last patch of the series to
>     this patch. (Ander)
> v3: Added MST hotplug handling. (Ander)

Please pick up previous review tags when resending patches. In this case, this
was already

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 71 +++++++++++++++++++++++++---------------
> -
>  1 file changed, 44 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 8969ff9..82ee18d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4638,8 +4638,19 @@ intel_dp_long_pulse(struct intel_connector
> *intel_connector)
>  	intel_dp_probe_oui(intel_dp);
>  
>  	ret = intel_dp_probe_mst(intel_dp);
> -	if (ret)
> +	if (ret) {
> +		goto out;
> +	} else if (connector->status == connector_status_connected) {
> +		/*
> +		 * If display was connected already and is still connected
> +		 * check links status, there has been known issues of
> +		 * link loss triggerring long pulse!!!!
> +		 */
> +		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> +		intel_dp_check_link_status(intel_dp);
> +		drm_modeset_unlock(&dev->mode_config.connection_mutex);
>  		goto out;
> +	}
>  
>  	/*
>  	 * Clearing NACK and defer counts to get their exact values
> @@ -4668,8 +4679,21 @@ intel_dp_long_pulse(struct intel_connector
> *intel_connector)
>  	}
>  
>  out:
> -	if (status != connector_status_connected)
> +	if (status != connector_status_connected) {
>  		intel_dp_unset_edid(intel_dp);
> +		/*
> +		 * If we were in MST mode, and device is not there,
> +		 * get out of MST mode
> +		 */
> +		if (intel_dp->is_mst) {
> +			DRM_DEBUG_KMS("MST device may have disappeared %d vs
> %d\n",
> +				intel_dp->is_mst, intel_dp
> ->mst_mgr.mst_state);
> +			intel_dp->is_mst = false;
> +			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> +							intel_dp->is_mst);
> +		}
> +	}
> +
>  	intel_display_power_put(to_i915(dev), power_domain);
>  	return;
>  }
> @@ -4693,7 +4717,8 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
>  		return connector_status_disconnected;
>  	}
>  
> -	intel_dp_long_pulse(intel_dp->attached_connector);
> +	if (force)
> +		intel_dp_long_pulse(intel_dp->attached_connector);
>  
>  	if (intel_connector->detect_edid)
>  		return connector_status_connected;
> @@ -5026,25 +5051,25 @@ intel_dp_hpd_pulse(struct intel_digital_port
> *intel_dig_port, bool long_hpd)
>  		/* indicate that we need to restart link training */
>  		intel_dp->train_set_valid = false;
>  
> -		if (!intel_digital_port_connected(dev_priv, intel_dig_port))
> -			goto mst_fail;
> -
> -		if (!intel_dp_get_dpcd(intel_dp)) {
> -			goto mst_fail;
> -		}
> -
> -		intel_dp_probe_oui(intel_dp);
> +		intel_dp_long_pulse(intel_dp->attached_connector);
> +		if (intel_dp->is_mst)
> +			ret = IRQ_HANDLED;
> +		goto put_power;
>  
> -		if (!intel_dp_probe_mst(intel_dp)) {
> -			drm_modeset_lock(&dev->mode_config.connection_mutex,
> NULL);
> -			intel_dp_check_link_status(intel_dp);
> -			drm_modeset_unlock(&dev
> ->mode_config.connection_mutex);
> -			goto mst_fail;
> -		}
>  	} else {
>  		if (intel_dp->is_mst) {
> -			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
> -				goto mst_fail;
> +			if (intel_dp_check_mst_status(intel_dp) == -EINVAL) {
> +				/*
> +				 * If we were in MST mode, and device is not
> +				 * there, get out of MST mode
> +				 */
> +				DRM_DEBUG_KMS("MST device may have
> disappeared %d vs %d\n",
> +					intel_dp->is_mst, intel_dp
> ->mst_mgr.mst_state);
> +				intel_dp->is_mst = false;
> +				drm_dp_mst_topology_mgr_set_mst(&intel_dp
> ->mst_mgr,
> +								intel_dp
> ->is_mst);
> +				goto put_power;
> +			}
>  		}
>  
>  		if (!intel_dp->is_mst) {
> @@ -5056,14 +5081,6 @@ intel_dp_hpd_pulse(struct intel_digital_port
> *intel_dig_port, bool long_hpd)
>  
>  	ret = IRQ_HANDLED;
>  
> -	goto put_power;
> -mst_fail:
> -	/* if we were in MST mode, and device is not there get out of MST
> mode */
> -	if (intel_dp->is_mst) {
> -		DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
> intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
> -		intel_dp->is_mst = false;
> -		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp
> ->is_mst);
> -	}
>  put_power:
>  	intel_display_power_put(dev_priv, power_domain);
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/5] drm/i915: Cleaning up intel_dp_hpd_pulse

[Shubhangi Shrivastava]

Current DP detection has DPCD operations split across
intel_dp_hpd_pulse and intel_dp_detect which contains
duplicates as well. Also intel_dp_detect is called
during modes enumeration as well which will result
in multiple dpcd operations. So this patch tries
to solve both these by bringing all DPCD operations
in one single function and make intel_dp_detect
use existing values instead of repeating same steps.

v2: Pulled in a hunk from last patch of the series to
    this patch. (Ander)
v3: Added MST hotplug handling. (Ander)

Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 71 +++++++++++++++++++++++++----------------
 1 file changed, 44 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8969ff9..82ee18d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4638,8 +4638,19 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 	intel_dp_probe_oui(intel_dp);
 
 	ret = intel_dp_probe_mst(intel_dp);
-	if (ret)
+	if (ret) {
+		goto out;
+	} else if (connector->status == connector_status_connected) {
+		/*
+		 * If display was connected already and is still connected
+		 * check links status, there has been known issues of
+		 * link loss triggerring long pulse!!!!
+		 */
+		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+		intel_dp_check_link_status(intel_dp);
+		drm_modeset_unlock(&dev->mode_config.connection_mutex);
 		goto out;
+	}
 
 	/*
 	 * Clearing NACK and defer counts to get their exact values
@@ -4668,8 +4679,21 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 	}
 
 out:
-	if (status != connector_status_connected)
+	if (status != connector_status_connected) {
 		intel_dp_unset_edid(intel_dp);
+		/*
+		 * If we were in MST mode, and device is not there,
+		 * get out of MST mode
+		 */
+		if (intel_dp->is_mst) {
+			DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
+				intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
+			intel_dp->is_mst = false;
+			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
+							intel_dp->is_mst);
+		}
+	}
+
 	intel_display_power_put(to_i915(dev), power_domain);
 	return;
 }
@@ -4693,7 +4717,8 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 		return connector_status_disconnected;
 	}
 
-	intel_dp_long_pulse(intel_dp->attached_connector);
+	if (force)
+		intel_dp_long_pulse(intel_dp->attached_connector);
 
 	if (intel_connector->detect_edid)
 		return connector_status_connected;
@@ -5026,25 +5051,25 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 		/* indicate that we need to restart link training */
 		intel_dp->train_set_valid = false;
 
-		if (!intel_digital_port_connected(dev_priv, intel_dig_port))
-			goto mst_fail;
-
-		if (!intel_dp_get_dpcd(intel_dp)) {
-			goto mst_fail;
-		}
-
-		intel_dp_probe_oui(intel_dp);
+		intel_dp_long_pulse(intel_dp->attached_connector);
+		if (intel_dp->is_mst)
+			ret = IRQ_HANDLED;
+		goto put_power;
 
-		if (!intel_dp_probe_mst(intel_dp)) {
-			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-			intel_dp_check_link_status(intel_dp);
-			drm_modeset_unlock(&dev->mode_config.connection_mutex);
-			goto mst_fail;
-		}
 	} else {
 		if (intel_dp->is_mst) {
-			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
-				goto mst_fail;
+			if (intel_dp_check_mst_status(intel_dp) == -EINVAL) {
+				/*
+				 * If we were in MST mode, and device is not
+				 * there, get out of MST mode
+				 */
+				DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
+					intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
+				intel_dp->is_mst = false;
+				drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
+								intel_dp->is_mst);
+				goto put_power;
+			}
 		}
 
 		if (!intel_dp->is_mst) {
@@ -5056,14 +5081,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 
 	ret = IRQ_HANDLED;
 
-	goto put_power;
-mst_fail:
-	/* if we were in MST mode, and device is not there get out of MST mode */
-	if (intel_dp->is_mst) {
-		DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
-		intel_dp->is_mst = false;
-		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
-	}
 put_power:
 	intel_display_power_put(dev_priv, power_domain);
 
-- 
2.6.1

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

[PATCH 2/5] drm/i915: Cleaning up intel_dp_hpd_pulse

[Shubhangi Shrivastava]

Current DP detection has DPCD operations split across
intel_dp_hpd_pulse and intel_dp_detect which contains
duplicates as well. Also intel_dp_detect is called
during modes enumeration as well which will result
in multiple dpcd operations. So this patch tries
to solve both these by bringing all DPCD operations
in one single function and make intel_dp_detect
use existing values instead of repeating same steps.

v2: Pulled in a hunk from last patch of the series to
    this patch. (Ander)
v3: Added MST hotplug handling. (Ander)

Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 71 +++++++++++++++++++++++++----------------
 1 file changed, 44 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8969ff9..82ee18d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4638,8 +4638,19 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 	intel_dp_probe_oui(intel_dp);
 
 	ret = intel_dp_probe_mst(intel_dp);
-	if (ret)
+	if (ret) {
+		goto out;
+	} else if (connector->status == connector_status_connected) {
+		/*
+		 * If display was connected already and is still connected
+		 * check links status, there has been known issues of
+		 * link loss triggerring long pulse!!!!
+		 */
+		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+		intel_dp_check_link_status(intel_dp);
+		drm_modeset_unlock(&dev->mode_config.connection_mutex);
 		goto out;
+	}
 
 	/*
 	 * Clearing NACK and defer counts to get their exact values
@@ -4668,8 +4679,21 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 	}
 
 out:
-	if (status != connector_status_connected)
+	if (status != connector_status_connected) {
 		intel_dp_unset_edid(intel_dp);
+		/*
+		 * If we were in MST mode, and device is not there,
+		 * get out of MST mode
+		 */
+		if (intel_dp->is_mst) {
+			DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
+				intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
+			intel_dp->is_mst = false;
+			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
+							intel_dp->is_mst);
+		}
+	}
+
 	intel_display_power_put(to_i915(dev), power_domain);
 	return;
 }
@@ -4693,7 +4717,8 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 		return connector_status_disconnected;
 	}
 
-	intel_dp_long_pulse(intel_dp->attached_connector);
+	if (force)
+		intel_dp_long_pulse(intel_dp->attached_connector);
 
 	if (intel_connector->detect_edid)
 		return connector_status_connected;
@@ -5026,25 +5051,25 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 		/* indicate that we need to restart link training */
 		intel_dp->train_set_valid = false;
 
-		if (!intel_digital_port_connected(dev_priv, intel_dig_port))
-			goto mst_fail;
-
-		if (!intel_dp_get_dpcd(intel_dp)) {
-			goto mst_fail;
-		}
-
-		intel_dp_probe_oui(intel_dp);
+		intel_dp_long_pulse(intel_dp->attached_connector);
+		if (intel_dp->is_mst)
+			ret = IRQ_HANDLED;
+		goto put_power;
 
-		if (!intel_dp_probe_mst(intel_dp)) {
-			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-			intel_dp_check_link_status(intel_dp);
-			drm_modeset_unlock(&dev->mode_config.connection_mutex);
-			goto mst_fail;
-		}
 	} else {
 		if (intel_dp->is_mst) {
-			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
-				goto mst_fail;
+			if (intel_dp_check_mst_status(intel_dp) == -EINVAL) {
+				/*
+				 * If we were in MST mode, and device is not
+				 * there, get out of MST mode
+				 */
+				DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
+					intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
+				intel_dp->is_mst = false;
+				drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
+								intel_dp->is_mst);
+				goto put_power;
+			}
 		}
 
 		if (!intel_dp->is_mst) {
@@ -5056,14 +5081,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 
 	ret = IRQ_HANDLED;
 
-	goto put_power;
-mst_fail:
-	/* if we were in MST mode, and device is not there get out of MST mode */
-	if (intel_dp->is_mst) {
-		DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
-		intel_dp->is_mst = false;
-		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
-	}
 put_power:
 	intel_display_power_put(dev_priv, power_domain);
 
-- 
2.6.1

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