[03/10] drm/i915: Do not get aux power for disconnected DP ports
diff mbox series

Message ID 20181002175054.15010-3-jose.souza@intel.com
State New
Headers show
Series
  • [01/10] drm: Do not call drm_dp_cec_set_edid() while registering DP connectors
Related show

Commit Message

Souza, Jose Oct. 2, 2018, 5:50 p.m. UTC
For ICL type-c ports there is a aux power restriction, it can only be
enabled while there is sink connected.

BSpec: 21750

v2:
- rebased on top of the refactored version of intel_dp_detect()
- fixing CI errors by getting runtime_pm(), for VLV/CHV it is also
necessary get the DPIO power well reference

Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

Comments

Ville Syrjälä Oct. 2, 2018, 8:04 p.m. UTC | #1
On Tue, Oct 02, 2018 at 10:50:47AM -0700, José Roberto de Souza wrote:
> For ICL type-c ports there is a aux power restriction, it can only be
> enabled while there is sink connected.
> 
> BSpec: 21750
> 
> v2:
> - rebased on top of the refactored version of intel_dp_detect()
> - fixing CI errors by getting runtime_pm(), for VLV/CHV it is also
> necessary get the DPIO power well reference

The hpd logic is in the disp2d well.

> 
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 5a84a929bc7d..2cd2dc564181 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4996,10 +4996,28 @@ bool intel_digital_port_connected(struct intel_encoder *encoder)
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  
>  	if (HAS_GMCH_DISPLAY(dev_priv)) {
> +		enum intel_display_power_domain domain = 0;
> +		bool ret;
> +
> +		/*
> +		 * Oddly VLV/CHV needs the DPIO power well on to be able to
> +		 * read the hotplug status, otherwise it will read a bogus value
> +		 * and throw a unclaimed register warning
> +		 */
> +		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> +			domain = intel_port_to_power_domain(encoder->port);
> +			intel_display_power_get(dev_priv, domain);
> +		}
> +
>  		if (IS_GM45(dev_priv))
> -			return gm45_digital_port_connected(encoder);
> +			ret = gm45_digital_port_connected(encoder);
>  		else
> -			return g4x_digital_port_connected(encoder);
> +			ret = g4x_digital_port_connected(encoder);
> +
> +		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> +			intel_display_power_put(dev_priv, domain);
> +
> +		return ret;
>  	}
>  
>  	if (IS_GEN5(dev_priv))
> @@ -5075,7 +5093,7 @@ intel_dp_detect(struct drm_connector *connector,
>  		      connector->base.id, connector->name);
>  	WARN_ON(!drm_modeset_is_locked(&dev_priv->drm.mode_config.connection_mutex));
>  
> -	intel_display_power_get(dev_priv, intel_dp->aux_power_domain);
> +	intel_runtime_pm_get(dev_priv);
>  
>  	/* Is port connected? eDP can't be disconnected */
>  	if (!intel_dp_is_edp(intel_dp) &&
> @@ -5084,6 +5102,8 @@ intel_dp_detect(struct drm_connector *connector,
>  		goto port_disconnected;
>  	}
>  
> +	intel_display_power_get(dev_priv, intel_dp->aux_power_domain);
> +
>  	if (intel_dp_is_edp(intel_dp))
>  		status = edp_detect(intel_dp);
>  	else
> @@ -5116,6 +5136,7 @@ intel_dp_detect(struct drm_connector *connector,
>  		 * with EDID on it
>  		 */
>  		intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
> +		intel_runtime_pm_put(dev_priv);
>  		return connector_status_disconnected;
>  	}
>  
> @@ -5130,6 +5151,7 @@ intel_dp_detect(struct drm_connector *connector,
>  		if (ret) {
>  			intel_display_power_put(dev_priv,
>  						intel_dp->aux_power_domain);
> +			intel_runtime_pm_put(dev_priv);
>  			return ret;
>  		}
>  	}
> @@ -5153,9 +5175,11 @@ intel_dp_detect(struct drm_connector *connector,
>  	intel_dp_check_service_irq(intel_dp);
>  
>  	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
> +	intel_runtime_pm_put(dev_priv);
>  	return status;
>  
>  port_not_detected:
> +	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
>  port_disconnected:
>  	memset(&intel_dp->compliance, 0, sizeof(intel_dp->compliance));
>  
> @@ -5168,7 +5192,7 @@ intel_dp_detect(struct drm_connector *connector,
>  	} else
>  		intel_dp_unset_edid(intel_dp);
>  
> -	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
> +	intel_runtime_pm_put(dev_priv);
>  	return status;
>  }
>  
> -- 
> 2.19.0
Ville Syrjälä Oct. 2, 2018, 8:49 p.m. UTC | #2
On Tue, Oct 02, 2018 at 10:50:47AM -0700, José Roberto de Souza wrote:
> For ICL type-c ports there is a aux power restriction, it can only be
> enabled while there is sink connected.
> 
> BSpec: 21750
> 
> v2:
> - rebased on top of the refactored version of intel_dp_detect()
> - fixing CI errors by getting runtime_pm(), for VLV/CHV it is also
> necessary get the DPIO power well reference
> 
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 5a84a929bc7d..2cd2dc564181 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4996,10 +4996,28 @@ bool intel_digital_port_connected(struct intel_encoder *encoder)
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  
>  	if (HAS_GMCH_DISPLAY(dev_priv)) {
> +		enum intel_display_power_domain domain = 0;
> +		bool ret;
> +
> +		/*
> +		 * Oddly VLV/CHV needs the DPIO power well on to be able to
> +		 * read the hotplug status, otherwise it will read a bogus value
> +		 * and throw a unclaimed register warning
> +		 */
> +		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> +			domain = intel_port_to_power_domain(encoder->port);
> +			intel_display_power_get(dev_priv, domain);
> +		}
> +
>  		if (IS_GM45(dev_priv))
> -			return gm45_digital_port_connected(encoder);
> +			ret = gm45_digital_port_connected(encoder);
>  		else
> -			return g4x_digital_port_connected(encoder);
> +			ret = g4x_digital_port_connected(encoder);
> +
> +		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> +			intel_display_power_put(dev_priv, domain);
> +
> +		return ret;
>  	}
>  
>  	if (IS_GEN5(dev_priv))
> @@ -5075,7 +5093,7 @@ intel_dp_detect(struct drm_connector *connector,
>  		      connector->base.id, connector->name);
>  	WARN_ON(!drm_modeset_is_locked(&dev_priv->drm.mode_config.connection_mutex));
>  
> -	intel_display_power_get(dev_priv, intel_dp->aux_power_domain);
> +	intel_runtime_pm_get(dev_priv);

I still maintain that it can't be the "is there a sink or not?" thing
that makes this fail. The only thing that makes sense to me is the
port tbt vs. tc (or whatever it was) knob. Otherwise we can race
with display hotplug all day long.

Also this is hardly the only place where we grab the power reference
so we'd need the duct tape applied quite a bit more liberally if we
wanted it to work.

So there seems to be a fundemental problem with the whole tbt vs. tc
mode thing in the current code. We should fix that before we add
piles of duct tape:
- correctly track which mode the port is used in
- probably split the connector into tbt vs. tc variants (to make it
  clear which mode is being used)
- make sure it stays in that mode until current users are done
- block/reject other uses of the port while it's in the wrong mode
- do the approriate flows when acquiring/reqlinguishing control
  of the port
- test the heck out of it with eg. genconnector, forced connctor
  status, /dev/aux thing, i2c, etc.

>  
>  	/* Is port connected? eDP can't be disconnected */
>  	if (!intel_dp_is_edp(intel_dp) &&
> @@ -5084,6 +5102,8 @@ intel_dp_detect(struct drm_connector *connector,
>  		goto port_disconnected;
>  	}
>  
> +	intel_display_power_get(dev_priv, intel_dp->aux_power_domain);
> +
>  	if (intel_dp_is_edp(intel_dp))
>  		status = edp_detect(intel_dp);
>  	else
> @@ -5116,6 +5136,7 @@ intel_dp_detect(struct drm_connector *connector,
>  		 * with EDID on it
>  		 */
>  		intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
> +		intel_runtime_pm_put(dev_priv);
>  		return connector_status_disconnected;
>  	}
>  
> @@ -5130,6 +5151,7 @@ intel_dp_detect(struct drm_connector *connector,
>  		if (ret) {
>  			intel_display_power_put(dev_priv,
>  						intel_dp->aux_power_domain);
> +			intel_runtime_pm_put(dev_priv);
>  			return ret;
>  		}
>  	}
> @@ -5153,9 +5175,11 @@ intel_dp_detect(struct drm_connector *connector,
>  	intel_dp_check_service_irq(intel_dp);
>  
>  	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
> +	intel_runtime_pm_put(dev_priv);
>  	return status;
>  
>  port_not_detected:
> +	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
>  port_disconnected:
>  	memset(&intel_dp->compliance, 0, sizeof(intel_dp->compliance));
>  
> @@ -5168,7 +5192,7 @@ intel_dp_detect(struct drm_connector *connector,
>  	} else
>  		intel_dp_unset_edid(intel_dp);
>  
> -	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
> +	intel_runtime_pm_put(dev_priv);
>  	return status;
>  }
>  
> -- 
> 2.19.0

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 5a84a929bc7d..2cd2dc564181 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4996,10 +4996,28 @@  bool intel_digital_port_connected(struct intel_encoder *encoder)
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 
 	if (HAS_GMCH_DISPLAY(dev_priv)) {
+		enum intel_display_power_domain domain = 0;
+		bool ret;
+
+		/*
+		 * Oddly VLV/CHV needs the DPIO power well on to be able to
+		 * read the hotplug status, otherwise it will read a bogus value
+		 * and throw a unclaimed register warning
+		 */
+		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
+			domain = intel_port_to_power_domain(encoder->port);
+			intel_display_power_get(dev_priv, domain);
+		}
+
 		if (IS_GM45(dev_priv))
-			return gm45_digital_port_connected(encoder);
+			ret = gm45_digital_port_connected(encoder);
 		else
-			return g4x_digital_port_connected(encoder);
+			ret = g4x_digital_port_connected(encoder);
+
+		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+			intel_display_power_put(dev_priv, domain);
+
+		return ret;
 	}
 
 	if (IS_GEN5(dev_priv))
@@ -5075,7 +5093,7 @@  intel_dp_detect(struct drm_connector *connector,
 		      connector->base.id, connector->name);
 	WARN_ON(!drm_modeset_is_locked(&dev_priv->drm.mode_config.connection_mutex));
 
-	intel_display_power_get(dev_priv, intel_dp->aux_power_domain);
+	intel_runtime_pm_get(dev_priv);
 
 	/* Is port connected? eDP can't be disconnected */
 	if (!intel_dp_is_edp(intel_dp) &&
@@ -5084,6 +5102,8 @@  intel_dp_detect(struct drm_connector *connector,
 		goto port_disconnected;
 	}
 
+	intel_display_power_get(dev_priv, intel_dp->aux_power_domain);
+
 	if (intel_dp_is_edp(intel_dp))
 		status = edp_detect(intel_dp);
 	else
@@ -5116,6 +5136,7 @@  intel_dp_detect(struct drm_connector *connector,
 		 * with EDID on it
 		 */
 		intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
+		intel_runtime_pm_put(dev_priv);
 		return connector_status_disconnected;
 	}
 
@@ -5130,6 +5151,7 @@  intel_dp_detect(struct drm_connector *connector,
 		if (ret) {
 			intel_display_power_put(dev_priv,
 						intel_dp->aux_power_domain);
+			intel_runtime_pm_put(dev_priv);
 			return ret;
 		}
 	}
@@ -5153,9 +5175,11 @@  intel_dp_detect(struct drm_connector *connector,
 	intel_dp_check_service_irq(intel_dp);
 
 	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
+	intel_runtime_pm_put(dev_priv);
 	return status;
 
 port_not_detected:
+	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
 port_disconnected:
 	memset(&intel_dp->compliance, 0, sizeof(intel_dp->compliance));
 
@@ -5168,7 +5192,7 @@  intel_dp_detect(struct drm_connector *connector,
 	} else
 		intel_dp_unset_edid(intel_dp);
 
-	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
+	intel_runtime_pm_put(dev_priv);
 	return status;
 }