Re: [PATCH 03/10] drm/i915: Do not get aux power for disconnected DP ports

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

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