Re: [PATCH 03/12] drm/i915: Clean up AUX power domain handling

[Imre Deak <imre.deak@intel.com>]

On ma, 2015-11-09 at 16:48 +0100, Patrik Jakobsson wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Introduce intel_display_port_aux_power_domain() which simply returns
> the appropriate AUX power domain for a specific port, and then
> replace
> the intel_display_port_power_domain() with calls to the new function
> in the DP code. As long as we're not actually enabling the port we
> don't
> need the lane power domains, and those are handled now purely from
> modeset_update_crtc_power_domains().
> 
> My initial motivation for this was to see if I could keep the DPIO
> power
> wells powered down while doing AUX on CHV, but turns out I can't so
> this
> doesn't change anything for CHV at least. But I think it's still a
> worthwile change.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 40
> ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dp.c      | 48 +++++++++++---------------
> ----------
>  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>  3 files changed, 56 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index d0fec07..c2578d9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5143,6 +5143,23 @@ static enum intel_display_power_domain
> port_to_power_domain(enum port port)
>  	}
>  }
>  
> +static enum intel_display_power_domain port_to_aux_power_domain(enum
> port port)
> +{
> +	switch (port) {
> +	case PORT_A:
> +		return POWER_DOMAIN_AUX_A;
> +	case PORT_B:
> +		return POWER_DOMAIN_AUX_B;
> +	case PORT_C:
> +		return POWER_DOMAIN_AUX_C;
> +	case PORT_D:
> +		return POWER_DOMAIN_AUX_D;
> +	default:
> +		WARN_ON_ONCE(1);
> +		return POWER_DOMAIN_AUX_A;
> +	}
> +}

Looks like port E is missing. I chatted with Ville he has some idea to
fix this.

> +
>  #define for_each_power_domain(domain, mask)				
> \
>  	for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++)	
> \
>  		if ((1 << (domain)) & (mask))
> @@ -5174,6 +5191,29 @@ intel_display_port_power_domain(struct
> intel_encoder *intel_encoder)
>  	}
>  }
>  
> +enum intel_display_power_domain
> +intel_display_port_aux_power_domain(struct intel_encoder
> *intel_encoder)
> +{
> +	struct drm_device *dev = intel_encoder->base.dev;
> +	struct intel_digital_port *intel_dig_port;
> +
> +	switch (intel_encoder->type) {
> +	case INTEL_OUTPUT_UNKNOWN:
> +		/* Only DDI platforms should ever use this output
> type */
> +		WARN_ON_ONCE(!HAS_DDI(dev));
> +	case INTEL_OUTPUT_DISPLAYPORT:
> +	case INTEL_OUTPUT_EDP:
> +		intel_dig_port = enc_to_dig_port(&intel_encoder
> ->base);
> +		return port_to_aux_power_domain(intel_dig_port
> ->port);
> +	case INTEL_OUTPUT_DP_MST:
> +		intel_dig_port = enc_to_mst(&intel_encoder->base)
> ->primary;
> +		return port_to_aux_power_domain(intel_dig_port
> ->port);
> +	default:
> +		WARN_ON_ONCE(1);
> +		return POWER_DOMAIN_AUX_A;
> +	}
> +}
> +
>  static unsigned long get_crtc_power_domains(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> index 4655af0..3978540 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -277,7 +277,7 @@ static void pps_lock(struct intel_dp *intel_dp)
>  	 * See vlv_power_sequencer_reset() why we need
>  	 * a power domain reference here.
>  	 */
> -	power_domain = intel_display_port_power_domain(encoder);
> +	power_domain = intel_display_port_aux_power_domain(encoder);
>  	intel_display_power_get(dev_priv, power_domain);
>  
>  	mutex_lock(&dev_priv->pps_mutex);
> @@ -293,7 +293,7 @@ static void pps_unlock(struct intel_dp *intel_dp)
>  
>  	mutex_unlock(&dev_priv->pps_mutex);
>  
> -	power_domain = intel_display_port_power_domain(encoder);
> +	power_domain = intel_display_port_aux_power_domain(encoder);
>  	intel_display_power_put(dev_priv, power_domain);
>  }
>  
> @@ -816,8 +816,6 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  
>  	intel_dp_check_edp(intel_dp);
>  
> -	intel_aux_display_runtime_get(dev_priv);
> -
> 	/* Try to wait for any previous AUX channel activity */
>  	for (try = 0; try < 3; try++) {
>  		status = I915_READ_NOTRACE(ch_ctl);
> @@ -926,7 +924,6 @@ done:
>  	ret = recv_bytes;
>  out:
>  	pm_qos_update_request(&dev_priv->pm_qos,
> PM_QOS_DEFAULT_VALUE);
> -	intel_aux_display_runtime_put(dev_priv);
>  
>  	if (vdd)
>  		edp_panel_vdd_off(intel_dp, false);
> @@ -1784,7 +1781,7 @@ static bool edp_panel_vdd_on(struct intel_dp
> *intel_dp)
>  	if (edp_have_panel_vdd(intel_dp))
>  		return need_to_disable;
>  
> -	power_domain =
> intel_display_port_power_domain(intel_encoder);
> +	power_domain =
> intel_display_port_aux_power_domain(intel_encoder);
>  	intel_display_power_get(dev_priv, power_domain);
>  
>  	DRM_DEBUG_KMS("Turning eDP port %c VDD on\n",
> @@ -1874,7 +1871,7 @@ static void edp_panel_vdd_off_sync(struct
> intel_dp *intel_dp)
>  	if ((pp & POWER_TARGET_ON) == 0)
>  		intel_dp->last_power_cycle = jiffies;
>  
> -	power_domain =
> intel_display_port_power_domain(intel_encoder);
> +	power_domain =
> intel_display_port_aux_power_domain(intel_encoder);
>  	intel_display_power_put(dev_priv, power_domain);
>  }
>  
> @@ -2025,7 +2022,7 @@ static void edp_panel_off(struct intel_dp
> *intel_dp)
>  	wait_panel_off(intel_dp);
>  
>  	/* We got a reference when we enabled the VDD. */
> -	power_domain =
> intel_display_port_power_domain(intel_encoder);
> +	power_domain =
> intel_display_port_aux_power_domain(intel_encoder);
>  	intel_display_power_put(dev_priv, power_domain);
>  }
>  
> @@ -4765,26 +4762,6 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
>  	intel_dp->has_audio = false;
>  }
>  
> -static enum intel_display_power_domain
> -intel_dp_power_get(struct intel_dp *dp)
> -{
> -	struct intel_encoder *encoder = &dp_to_dig_port(dp)->base;
> -	enum intel_display_power_domain power_domain;
> -
> -	power_domain = intel_display_port_power_domain(encoder);
> -	intel_display_power_get(to_i915(encoder->base.dev),
> power_domain);
> -
> -	return power_domain;
> -}
> -
> -static void
> -intel_dp_power_put(struct intel_dp *dp,
> -		   enum intel_display_power_domain power_domain)
> -{
> -	struct intel_encoder *encoder = &dp_to_dig_port(dp)->base;
> -	intel_display_power_put(to_i915(encoder->base.dev),
> power_domain);
> -}
> -
>  static enum drm_connector_status
>  intel_dp_detect(struct drm_connector *connector, bool force)
>  {
> @@ -4808,7 +4785,8 @@ intel_dp_detect(struct drm_connector
> *connector, bool force)
>  		return connector_status_disconnected;
>  	}
>  
> -	power_domain = intel_dp_power_get(intel_dp);
> +	power_domain =
> intel_display_port_aux_power_domain(intel_encoder);
> +	intel_display_power_get(to_i915(dev), power_domain);
>  
>  	/* Can't disconnect eDP, but you can close the lid... */
>  	if (is_edp(intel_dp))
> @@ -4853,7 +4831,7 @@ intel_dp_detect(struct drm_connector
> *connector, bool force)
>  	}
>  
>  out:
> -	intel_dp_power_put(intel_dp, power_domain);
> +	intel_display_power_put(to_i915(dev), power_domain);
>  	return status;
>  }
>  
> @@ -4862,6 +4840,7 @@ intel_dp_force(struct drm_connector *connector)
>  {
>  	struct intel_dp *intel_dp = intel_attached_dp(connector);
>  	struct intel_encoder *intel_encoder =
> &dp_to_dig_port(intel_dp)->base;
> +	struct drm_i915_private *dev_priv = to_i915(intel_encoder
> ->base.dev);
>  	enum intel_display_power_domain power_domain;
>  
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> @@ -4871,11 +4850,12 @@ intel_dp_force(struct drm_connector
> *connector)
>  	if (connector->status != connector_status_connected)
>  		return;
>  
> -	power_domain = intel_dp_power_get(intel_dp);
> +	power_domain =
> intel_display_port_aux_power_domain(intel_encoder);
> +	intel_display_power_get(dev_priv, power_domain);
>  
>  	intel_dp_set_edid(intel_dp);
>  
> -	intel_dp_power_put(intel_dp, power_domain);
> +	intel_display_power_put(dev_priv, power_domain);
>  
>  	if (intel_encoder->type != INTEL_OUTPUT_EDP)
>  		intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> @@ -5091,7 +5071,7 @@ static void intel_edp_panel_vdd_sanitize(struct
> intel_dp *intel_dp)
>  	 * indefinitely.
>  	 */
>  	DRM_DEBUG_KMS("VDD left on by BIOS, adjusting state
> tracking\n");
> -	power_domain =
> intel_display_port_power_domain(&intel_dig_port->base);
> +	power_domain =
> intel_display_port_aux_power_domain(&intel_dig_port->base);
>  	intel_display_power_get(dev_priv, power_domain);
>  
>  	edp_panel_vdd_schedule_off(intel_dp);
> @@ -5172,7 +5152,7 @@ intel_dp_hpd_pulse(struct intel_digital_port
> *intel_dig_port, bool long_hpd)
>  		      port_name(intel_dig_port->port),
>  		      long_hpd ? "long" : "short");
>  
> -	power_domain =
> intel_display_port_power_domain(intel_encoder);
> +	power_domain =
> intel_display_port_aux_power_domain(intel_encoder);
>  	intel_display_power_get(dev_priv, power_domain);
>  
>  	if (long_hpd) {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index a68b6cd..7d11aa0 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1188,6 +1188,8 @@ void hsw_enable_ips(struct intel_crtc *crtc);
>  void hsw_disable_ips(struct intel_crtc *crtc);
>  enum intel_display_power_domain
>  intel_display_port_power_domain(struct intel_encoder
> *intel_encoder);
> +enum intel_display_power_domain
> +intel_display_port_aux_power_domain(struct intel_encoder
> *intel_encoder);
>  void intel_mode_from_pipe_config(struct drm_display_mode *mode,
>  				 struct intel_crtc_state
> *pipe_config);
>  void intel_modeset_preclose(struct drm_device *dev, struct drm_file
> *file);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx