[PATCH 1/2] drm/i915: Move long hpd handling into the hotplug work

[PATCH 1/2] drm/i915: Move long hpd handling into the hotplug work

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We can't rely on connector->status in the detect() hook if the long hpd
was already handled by the dig_port_work as that won't update
connector->status. Thus we have to defer the long hpd handling entirely
until the hotplug work runs to avoid the double long hpd handling
the "detect_done" flag is trying to prevent.

Cc: Damien Cassou <damien@cassou.me>
Cc: freedesktop.org@gp.mailgun.org
Cc: Arno <blouin.arno@gmail.com>
Cc: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
Cc: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com>
Cc: stable@vger.kernel.org
Tested-by: Arno <blouin.arno@gmail.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=83348
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 48 ++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 9448d898d80b..96caa469e3a8 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4827,36 +4827,34 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 		      port_name(intel_dig_port->port),
 		      long_hpd ? "long" : "short");
 
+	if (long_hpd) {
+		intel_dp->detect_done = false;
+		return IRQ_NONE;
+	}
+
 	power_domain = intel_display_port_aux_power_domain(intel_encoder);
 	intel_display_power_get(dev_priv, power_domain);
 
-	if (long_hpd) {
-		intel_dp_long_pulse(intel_dp->attached_connector);
-		if (intel_dp->is_mst)
-			ret = IRQ_HANDLED;
-		goto put_power;
-
-	} else {
-		if (intel_dp->is_mst) {
-			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) {
+		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);
+			intel_dp->detect_done = false;
+			goto put_power;
 		}
+	}
 
-		if (!intel_dp->is_mst) {
-			if (!intel_dp_short_pulse(intel_dp)) {
-				intel_dp_long_pulse(intel_dp->attached_connector);
-				goto put_power;
-			}
+	if (!intel_dp->is_mst) {
+		if (!intel_dp_short_pulse(intel_dp)) {
+			intel_dp->detect_done = false;
+			goto put_power;
 		}
 	}
 
-- 
2.7.4

[PATCH 2/2] drm/i915: Allow DP to work w/o EDID

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Allow returning "connected" or "unknown" connector status for DP branch
devices that don't have an EDID. Currently we'd claim the thing as
"disconnected" if there is no EDID.

This stuff used to broken already, I think, but it got more broken by
commit f21a21983ef1 ("drm/i915: Splitting intel_dp_detect")

Cc: Damien Cassou <damien@cassou.me>
Cc: freedesktop.org@gp.mailgun.org
Cc: Arno <blouin.arno@gmail.com>
Cc: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
Cc: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com>
Cc: stable@vger.kernel.org
Tested-by: Arno <blouin.arno@gmail.com>
Fixes: f21a21983ef1 ("drm/i915: Splitting intel_dp_detect")
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=83348
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 96caa469e3a8..5992093e1814 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4333,7 +4333,7 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
 	intel_dp->has_audio = false;
 }
 
-static void
+static enum drm_connector_status
 intel_dp_long_pulse(struct intel_connector *intel_connector)
 {
 	struct drm_connector *connector = &intel_connector->base;
@@ -4357,7 +4357,7 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 	else
 		status = connector_status_disconnected;
 
-	if (status != connector_status_connected) {
+	if (status == connector_status_disconnected) {
 		intel_dp->compliance_test_active = 0;
 		intel_dp->compliance_test_type = 0;
 		intel_dp->compliance_test_data = 0;
@@ -4419,8 +4419,8 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 	intel_dp->aux.i2c_defer_count = 0;
 
 	intel_dp_set_edid(intel_dp);
-
-	status = connector_status_connected;
+	if (is_edp(intel_dp) || intel_connector->detect_edid)
+		status = connector_status_connected;
 	intel_dp->detect_done = true;
 
 	/* Try to read the source of the interrupt */
@@ -4439,12 +4439,11 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 	}
 
 out:
-	if ((status != connector_status_connected) &&
-	    (intel_dp->is_mst == false))
+	if (status != connector_status_connected && !intel_dp->is_mst)
 		intel_dp_unset_edid(intel_dp);
 
 	intel_display_power_put(to_i915(dev), power_domain);
-	return;
+	return status;
 }
 
 static enum drm_connector_status
@@ -4453,7 +4452,7 @@ 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);
+	enum drm_connector_status status = connector->status;
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
 		      connector->base.id, connector->name);
@@ -4468,14 +4467,11 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 
 	/* If full detect is not performed yet, do a full detect */
 	if (!intel_dp->detect_done)
-		intel_dp_long_pulse(intel_dp->attached_connector);
+		status = intel_dp_long_pulse(intel_dp->attached_connector);
 
 	intel_dp->detect_done = false;
 
-	if (is_edp(intel_dp) || intel_connector->detect_edid)
-		return connector_status_connected;
-	else
-		return connector_status_disconnected;
+	return status;
 }
 
 static void
-- 
2.7.4

Re: [PATCH 1/2] drm/i915: Move long hpd handling into the hotplug work

[Ander Conselvan De Oliveira]

On Mon, 2016-10-03 at 10:55 +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We can't rely on connector->status in the detect() hook if the long hpd
> was already handled by the dig_port_work as that won't update
> connector->status. Thus we have to defer the long hpd handling entirely
> until the hotplug work runs to avoid the double long hpd handling
> the "detect_done" flag is trying to prevent.

This is better indeed. But perhaps add a note here about the next patch, since
this one doesn't actually change the use of connector->status usage in detect().

For both patches:

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

> 
> Cc: Damien Cassou <damien@cassou.me>
> Cc: freedesktop.org@gp.mailgun.org
> Cc: Arno <blouin.arno@gmail.com>
> Cc: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> Cc: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com>
> Cc: stable@vger.kernel.org
> Tested-by: Arno <blouin.arno@gmail.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=83348
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 48 ++++++++++++++++++++------------------
> ---
>  1 file changed, 23 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 9448d898d80b..96caa469e3a8 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4827,36 +4827,34 @@ intel_dp_hpd_pulse(struct intel_digital_port
> *intel_dig_port, bool long_hpd)
>  		      port_name(intel_dig_port->port),
>  		      long_hpd ? "long" : "short");
>  
> +	if (long_hpd) {
> +		intel_dp->detect_done = false;
> +		return IRQ_NONE;
> +	}
> +
>  	power_domain = intel_display_port_aux_power_domain(intel_encoder);
>  	intel_display_power_get(dev_priv, power_domain);
>  
> -	if (long_hpd) {
> -		intel_dp_long_pulse(intel_dp->attached_connector);
> -		if (intel_dp->is_mst)
> -			ret = IRQ_HANDLED;
> -		goto put_power;
> -
> -	} else {
> -		if (intel_dp->is_mst) {
> -			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) {
> +		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);
> +			intel_dp->detect_done = false;
> +			goto put_power;
>  		}
> +	}
>  
> -		if (!intel_dp->is_mst) {
> -			if (!intel_dp_short_pulse(intel_dp)) {
> -				intel_dp_long_pulse(intel_dp-
> >attached_connector);
> -				goto put_power;
> -			}
> +	if (!intel_dp->is_mst) {
> +		if (!intel_dp_short_pulse(intel_dp)) {
> +			intel_dp->detect_done = false;
> +			goto put_power;
>  		}
>  	}
>  

Re: [PATCH 1/2] drm/i915: Move long hpd handling into the hotplug work

[Ville Syrjälä]

On Mon, Oct 03, 2016 at 02:39:16PM +0300, Ander Conselvan De Oliveira wrote:
> On Mon, 2016-10-03 at 10:55 +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > 
> > We can't rely on connector->status in the detect() hook if the long hpd
> > was already handled by the dig_port_work as that won't update
> > connector->status. Thus we have to defer the long hpd handling entirely
> > until the hotplug work runs to avoid the double long hpd handling
> > the "detect_done" flag is trying to prevent.
> 
> This is better indeed. But perhaps add a note here about the next patch, since
> this one doesn't actually change the use of connector->status usage in detect().
> 
> For both patches:
> 
> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

Thanks. I added a small note to 1/2 and pushed both patches to dinq.

> 
> > 
> > Cc: Damien Cassou <damien@cassou.me>
> > Cc: freedesktop.org@gp.mailgun.org
> > Cc: Arno <blouin.arno@gmail.com>
> > Cc: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> > Cc: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> > Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com>
> > Cc: stable@vger.kernel.org
> > Tested-by: Arno <blouin.arno@gmail.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=83348
> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > ---
> > �drivers/gpu/drm/i915/intel_dp.c | 48 ++++++++++++++++++++------------------
> > ---
> > �1 file changed, 23 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 9448d898d80b..96caa469e3a8 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4827,36 +4827,34 @@ intel_dp_hpd_pulse(struct intel_digital_port
> > *intel_dig_port, bool long_hpd)
> > �		������port_name(intel_dig_port->port),
> > �		������long_hpd ? "long" : "short");
> > �
> > +	if (long_hpd) {
> > +		intel_dp->detect_done = false;
> > +		return IRQ_NONE;
> > +	}
> > +
> > �	power_domain = intel_display_port_aux_power_domain(intel_encoder);
> > �	intel_display_power_get(dev_priv, power_domain);
> > �
> > -	if (long_hpd) {
> > -		intel_dp_long_pulse(intel_dp->attached_connector);
> > -		if (intel_dp->is_mst)
> > -			ret = IRQ_HANDLED;
> > -		goto put_power;
> > -
> > -	} else {
> > -		if (intel_dp->is_mst) {
> > -			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) {
> > +		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);
> > +			intel_dp->detect_done = false;
> > +			goto put_power;
> > �		}
> > +	}
> > �
> > -		if (!intel_dp->is_mst) {
> > -			if (!intel_dp_short_pulse(intel_dp)) {
> > -				intel_dp_long_pulse(intel_dp-
> > >attached_connector);
> > -				goto put_power;
> > -			}
> > +	if (!intel_dp->is_mst) {
> > +		if (!intel_dp_short_pulse(intel_dp)) {
> > +			intel_dp->detect_done = false;
> > +			goto put_power;
> > �		}
> > �	}
> > �

-- 
Ville Syrj�l�
Intel OTC