[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)

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 | 62 ++++++++++++++++++++++++++---------------
 1 file changed, 39 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1761254..8969ff9 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,17 +4632,14 @@ 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;
-		status = connector_status_disconnected;
+	if (ret)
 		goto out;
-	}
 
 	/*
 	 * Clearing NACK and defer counts to get their exact values
@@ -4662,8 +4651,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 +4668,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 | 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

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

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

[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 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

[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 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

[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

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 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

[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.

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)

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 82ee18d..f8d9611 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 */
@@ -5072,11 +5082,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

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

[Ander Conselvan De Oliveira]

On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote:
> 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

Please also mention why we should do that. With that fixed, this is

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

>  or not so
> intel_dp_get_link_status() performs dpcd read first
> then based on crtc active / enabled it will
> perform the link training.
> 
> 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)
> 
> 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 82ee18d..f8d9611 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 */
> @@ -5072,11 +5082,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;
_______________________________________________
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]

Sink count can change between short pulse hpd hence this patch
adds a member variable to intel_dp so we can track any changes
between short pulse interrupts.

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)

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  | 18 +++++++++++-------
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f8d9611..cdf4919 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3855,6 +3855,15 @@ 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;
+
+	intel_dp->sink_count = DP_GET_SINK_COUNT(intel_dp->sink_count);
+
+	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 +4381,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

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

[Ander Conselvan De Oliveira]

The patch summary is now too long. Just use the one from patch 5 ("read
sink_count dpcd always"). You can mention in the commit message that the value
of sink count is now stored in struct intel_dp, but that doesn't need to be in
the summary.

One more comment below.
On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote:
> Sink count can change between short pulse hpd hence this patch
> adds a member variable to intel_dp so we can track any changes
> between short pulse interrupts.
> 
> 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)
> 
> 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  | 18 +++++++++++-------
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f8d9611..cdf4919 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3855,6 +3855,15 @@ 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;
> +
> +	intel_dp->sink_count = DP_GET_SINK_COUNT(intel_dp->sink_count);
> +
> +	if (!intel_dp->sink_count)
> +		return false;
> +

You convinced me on the other email about this early return. Just add a comment
here with that justification.

With those two things, this is

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

>  	/* Check if the panel supports PSR */
>  	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
>  	if (is_edp(intel_dp)) {
> @@ -4372,14 +4381,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;
_______________________________________________
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>
---
 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 cdf4919..120d263 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4325,12 +4325,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
@@ -4340,9 +4347,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 */
@@ -4362,6 +4377,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 */
@@ -5086,8 +5103,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

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

[Ander Conselvan De Oliveira]

On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote:
> 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 cdf4919..120d263 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4325,12 +4325,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
> @@ -4340,9 +4347,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 */
> @@ -4362,6 +4377,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 */
> @@ -5086,8 +5103,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;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Shrivastava, Shubhangi]

Hi Daniel,

Is something else required for this patch series (5 patches) to be merged?

Thanks and Regards,
Shubhangi Shrivastava.

-----Original Message-----
From: Ander Conselvan De Oliveira [mailto:conselvan2@gmail.com] 
Sent: Wednesday, January 20, 2016 8:07 PM
To: Shrivastava, Shubhangi <shubhangi.shrivastava@intel.com>; intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 5/5] drm/i915: force full detect on sink count change

On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote:
> 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 cdf4919..120d263 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4325,12 +4325,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 @@ -4340,9 
> +4347,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 */ @@ -4362,6 +4377,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 */ @@ 
> -5086,8 +5103,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;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ander Conselvan De Oliveira]

On Thu, 2016-03-24 at 12:21 +0000, Shrivastava, Shubhangi wrote:
> Hi Daniel,
> 
> Is something else required for this patch series (5 patches) to be merged?

It needs to please CI. The errors reported are probably not caused by this
series, but at this point is probably better to rebase and resend. CI seems to
be a lot happier lately. :)

Ander

> 
> Thanks and Regards,
> Shubhangi Shrivastava.
> 
> -----Original Message-----
> From: Ander Conselvan De Oliveira [mailto:conselvan2@gmail.com] 
> Sent: Wednesday, January 20, 2016 8:07 PM
> To: Shrivastava, Shubhangi <shubhangi.shrivastava@intel.com>; 
> intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 5/5] drm/i915: force full detect on sink count
> change
> 
> On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote:
> > 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 cdf4919..120d263 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4325,12 +4325,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 @@ -4340,9
> > +4347,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 */ @@ -4362,6 +4377,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 */ @@ 
> > -5086,8 +5103,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;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Shubhangi Shrivastava]

On Wednesday 30 March 2016 04:39 PM, Ander Conselvan De Oliveira wrote:
> On Thu, 2016-03-24 at 12:21 +0000, Shrivastava, Shubhangi wrote:
>> Hi Daniel,
>>
>> Is something else required for this patch series (5 patches) to be merged?
> It needs to please CI. The errors reported are probably not caused by this
> series, but at this point is probably better to rebase and resend. CI seems to
> be a lot happier lately. :)
>
> Ander

Alright.. Rebased and resent.. Finally, CI has become happy.. :)
Thanks Ander!!

Shubhangi
>
>> Thanks and Regards,
>> Shubhangi Shrivastava.
>>
>> -----Original Message-----
>> From: Ander Conselvan De Oliveira [mailto:conselvan2@gmail.com]
>> Sent: Wednesday, January 20, 2016 8:07 PM
>> To: Shrivastava, Shubhangi <shubhangi.shrivastava@intel.com>;
>> intel-gfx@lists.freedesktop.org
>> Subject: Re: [Intel-gfx] [PATCH 5/5] drm/i915: force full detect on sink count
>> change
>>
>> On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote:
>>> 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 cdf4919..120d263 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -4325,12 +4325,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 @@ -4340,9
>>> +4347,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 */ @@ -4362,6 +4377,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 */ @@
>>> -5086,8 +5103,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;

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

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

[Patchwork]

== Summary ==

Built on 00a0c7d1ae09b1259c7af8e5a088b0b225d805df drm-intel-nightly: 2016y-01m-18d-16h-50m-37s UTC integration manifest

Test gem_storedw_loop:
        Subgroup basic-render:
                pass       -> DMESG-WARN (skl-i5k-2) UNSTABLE
                pass       -> DMESG-WARN (bdw-nuci7) UNSTABLE
                pass       -> DMESG-WARN (bdw-ultra) UNSTABLE
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                dmesg-warn -> PASS       (skl-i5k-2)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-b:
                pass       -> DMESG-WARN (ilk-hp8440p)

bdw-nuci7        total:140  pass:130  dwarn:1   dfail:0   fail:0   skip:9  
bdw-ultra        total:140  pass:132  dwarn:1   dfail:1   fail:0   skip:6  
byt-nuc          total:143  pass:125  dwarn:3   dfail:0   fail:0   skip:15 
hsw-brixbox      total:143  pass:136  dwarn:0   dfail:0   fail:0   skip:7  
hsw-gt2          total:143  pass:139  dwarn:0   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:143  pass:101  dwarn:4   dfail:0   fail:0   skip:38 
ivb-t430s        total:137  pass:124  dwarn:3   dfail:4   fail:0   skip:6  
skl-i5k-2        total:143  pass:133  dwarn:2   dfail:0   fail:0   skip:8  
snb-dellxps      total:143  pass:124  dwarn:5   dfail:0   fail:0   skip:14 
snb-x220t        total:143  pass:124  dwarn:5   dfail:0   fail:1   skip:13 

Results at /archive/results/CI_IGT_test/Patchwork_1218/

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

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

[Ander Conselvan De Oliveira]

On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote:
> 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)
> 
> 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 | 62 ++++++++++++++++++++++++++--------------
> -
>  1 file changed, 39 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 1761254..8969ff9 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,17 +4632,14 @@ 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;
> -		status = connector_status_disconnected;
> +	if (ret)
>  		goto out;

There is still the removal of "status = connector_status_disconnected;", so when
we goto to out, status is still connected and we don't call
intel_dp_unset_edid(), which will cause the return value of intel_dp_detect() to
be somewhat random. With that fixed, this is 

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


Ander

> -	}
>  
>  	/*
>  	 * Clearing NACK and defer counts to get their exact values
> @@ -4662,8 +4651,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 +4668,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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[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>
Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.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 a073f04..042283a 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)
 {
@@ -4584,6 +4585,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;
 
@@ -4604,9 +4606,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;
@@ -4616,17 +4619,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);
 
@@ -4647,14 +4639,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;
 	}
@@ -4669,8 +4665,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 */
@@ -4688,8 +4682,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
https://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>
Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.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 9fe78e1..a834c5f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3862,6 +3862,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)) {
@@ -4379,14 +4400,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 3d003d6..c04db07 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -752,6 +752,7 @@ struct intel_dp {
 	uint32_t DP;
 	int link_rate;
 	uint8_t lane_count;
+	uint8_t sink_count;
 	bool has_audio;
 	bool detect_done;
 	enum hdmi_force_audio force_audio;
-- 
2.6.1

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

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

[Ander Conselvan De Oliveira]

I think I mentioned something about the patch summary (the title) being too big
before. The kernel documentation on submitting patches [1] says: "the "summary"
must be no more than 70-75 characters".

[1] https://www.kernel.org/doc/Documentation/SubmittingPatches

Cheers,
Ander

On Wed, 2016-02-10 at 14:34 +0530, Shubhangi Shrivastava wrote:
> 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>
> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.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 9fe78e1..a834c5f 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3862,6 +3862,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)) {
> @@ -4379,14 +4400,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 3d003d6..c04db07 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -752,6 +752,7 @@ struct intel_dp {
>  	uint32_t DP;
>  	int link_rate;
>  	uint8_t lane_count;
> +	uint8_t sink_count;
>  	bool has_audio;
>  	bool detect_done;
>  	enum hdmi_force_audio force_audio;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[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 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 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)

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 | 62 ++++++++++++++++++++++++++---------------
 1 file changed, 39 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1761254..8969ff9 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,17 +4632,14 @@ 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;
-		status = connector_status_disconnected;
+	if (ret)
 		goto out;
-	}
 
 	/*
 	 * Clearing NACK and defer counts to get their exact values
@@ -4662,8 +4651,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 +4668,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 4/5] drm/i915: Save sink_count for tracking changes to it and read sink_count dpcd always

[Shubhangi Shrivastava]

Sink count can change between short pulse hpd hence this patch
adds a member variable to intel_dp so we can track any changes
between short pulse interrupts.

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)

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  | 18 +++++++++++-------
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f8d9611..cdf4919 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3855,6 +3855,15 @@ 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;
+
+	intel_dp->sink_count = DP_GET_SINK_COUNT(intel_dp->sink_count);
+
+	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 +4381,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