Re: [PATCH 72/89] drm/i915/skl: Enable/disable power well for aux transaction

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

[-- Attachment #1.1: Type: text/plain, Size: 10667 bytes --]

On Thu, 2014-09-04 at 12:27 +0100, Damien Lespiau wrote:
> From: Satheeshakrishna M <satheeshakrishna.m@intel.com>
> 
> This patch enables power well 2 required for any aux transaction.
> 
> v2: Implemented Imre's comments
> 	- In EDID/DPCD related routines, request AUX power well in SKL
> 
> v3: Implemented Imre's comments
> 	- Call AUX power well domain unconditionally for all platforms
> 
> v4: Remove the check on the output type for the AUX power domain
> 
> v5: Rebase on top of drm-intel-nightly (Damien)
> 
> v6: Rebase on top of -nightly (minor conflict in intel_drv.h) (Damien)
> 
> v7: Remove platform check while getting power well for port (Imre)
> 
> v8: Fix aux power handling around Vdd on/off (Damien)
> 
> v9: Acquire aux power domain on enabling vdd in
>     intel_edp_panel_vdd_sanitize() (Satheesh)
> 
> v10: Rebase on top of Ville's patch to return early in this function intead of
>      having a big indented block. (Damien)
> 
> v12: Rebase on top of Chris EDID caching work (Damien)
> 
> Signed-off-by: Satheeshakrishna M <satheeshakrishna.m@intel.com> (v4, v9)
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

This patch needs to be rebased on the recent PPS changes at least
getting/putting the AUX power domain in pps_lock()/pps_unlock() too.
Also it should be squashed into 69/89. Some more comments below.

> ---
>  drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++
>  drivers/gpu/drm/i915/intel_dp.c      | 62 ++++++++++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>  3 files changed, 68 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0a4dd00..abd4201 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4503,6 +4503,27 @@ intel_display_port_power_domain(struct intel_encoder *intel_encoder)
>  	}
>  }
>  
> +enum intel_display_power_domain
> +intel_display_aux_power_domain(struct intel_encoder *intel_encoder)
> +{
> +	struct intel_digital_port *intel_dig_port;
> +
> +	intel_dig_port = enc_to_dig_port(&intel_encoder->base);
> +	switch (intel_dig_port->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 0;
> +	}
> +}
> +
>  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 93bd9bf..a983b40 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1233,6 +1233,9 @@ static bool edp_panel_vdd_on(struct intel_dp *intel_dp)
>  	power_domain = intel_display_port_power_domain(intel_encoder);
>  	intel_display_power_get(dev_priv, power_domain);
>  
> +	power_domain = intel_display_aux_power_domain(intel_encoder);
> +	intel_display_power_get(dev_priv, power_domain);
> +

The AUX power domains were added to save power when only AUX
functionality is needed, since then we don't need to power on the power
domain needed for full port functionality. With the above change and
everywhere else below we'll end up enabling both power domains, though
we only need AUX functionality.

The power wells needed for AUX are a subset of those needed for full
port functionality on all platforms (at least atm), so this patch won't
change anything. The patch would make sense, if you requested only the
AUX domains.

>  	DRM_DEBUG_KMS("Turning eDP VDD on\n");
>  
>  	if (!edp_have_panel_power(intel_dp))
> @@ -1309,8 +1312,12 @@ static void edp_panel_vdd_off_sync(struct intel_dp *intel_dp)
>  
>  	power_domain = intel_display_port_power_domain(intel_encoder);
>  	intel_display_power_put(dev_priv, power_domain);
> +
> +	power_domain = intel_display_aux_power_domain(intel_encoder);
> +	intel_display_power_put(dev_priv, power_domain);
>  }

intel_edp_panel_off() needs to be changed too accordingly.

>  
> +

Extra w/s.

>  static void edp_panel_vdd_work(struct work_struct *__work)
>  {
>  	struct intel_dp *intel_dp = container_of(to_delayed_work(__work),
> @@ -3836,7 +3843,13 @@ g4x_dp_detect(struct intel_dp *intel_dp)
>  static struct edid *
>  intel_dp_get_edid(struct intel_dp *intel_dp)
>  {
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> +	struct drm_device *dev = intel_encoder->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_connector *intel_connector = intel_dp->attached_connector;
> +	enum intel_display_power_domain power_domain;
> +	struct edid *edid;
>  
>  	/* use cached edid if we have one */
>  	if (intel_connector->edid) {
> @@ -3845,9 +3858,16 @@ intel_dp_get_edid(struct intel_dp *intel_dp)
>  			return NULL;
>  
>  		return drm_edid_duplicate(intel_connector->edid);
> -	} else
> -		return drm_get_edid(&intel_connector->base,
> -				    &intel_dp->aux.ddc);
> +	} else {
> +		power_domain = intel_display_aux_power_domain(intel_encoder);
> +		intel_display_power_get(dev_priv, power_domain);

This is redundant, the higher level intel_dp_detect(), intel_dp_force()
already get/put the needed power domains.

> +
> +		edid = drm_get_edid(&intel_connector->base, &intel_dp->aux.ddc);
> +
> +		intel_display_power_put(dev_priv, power_domain);
> +
> +		return edid;
> +	}
>  }
>  
>  static void
> @@ -3876,24 +3896,30 @@ 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)
> +static void intel_dp_power_get(struct intel_dp *dp)
>  {
>  	struct intel_encoder *encoder = &dp_to_dig_port(dp)->base;
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	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;
> +	power_domain = intel_display_aux_power_domain(encoder);
> +	intel_display_power_get(dev_priv, power_domain);
>  }
>  
> -static void
> -intel_dp_power_put(struct intel_dp *dp,
> -		   enum intel_display_power_domain power_domain)
> +static void intel_dp_power_put(struct intel_dp *dp)
>  {
>  	struct intel_encoder *encoder = &dp_to_dig_port(dp)->base;
> -	intel_display_power_put(to_i915(encoder->base.dev), power_domain);
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	enum intel_display_power_domain power_domain;
> +
> +	power_domain = intel_display_port_power_domain(encoder);
> +	intel_display_power_put(dev_priv, power_domain);
> +
> +	power_domain = intel_display_aux_power_domain(encoder);
> +	intel_display_power_put(dev_priv, power_domain);
>  }
>  
>  static enum drm_connector_status
> @@ -3904,7 +3930,6 @@ intel_dp_detect(struct drm_connector *connector, bool force)
>  	struct intel_encoder *intel_encoder = &intel_dig_port->base;
>  	struct drm_device *dev = connector->dev;
>  	enum drm_connector_status status;
> -	enum intel_display_power_domain power_domain;
>  	bool ret;
>  
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> @@ -3918,7 +3943,7 @@ intel_dp_detect(struct drm_connector *connector, bool force)
>  		return connector_status_disconnected;
>  	}
>  
> -	power_domain = intel_dp_power_get(intel_dp);
> +	intel_dp_power_get(intel_dp);

Based on the above intel_dp_power_get() would only enable the AUX power
domain, which means we could keep the current prototype for
intel_dp_power_get()/put().

>  
>  	/* Can't disconnect eDP, but you can close the lid... */
>  	if (is_edp(intel_dp))
> @@ -3949,7 +3974,7 @@ intel_dp_detect(struct drm_connector *connector, bool force)
>  	status = connector_status_connected;
>  
>  out:
> -	intel_dp_power_put(intel_dp, power_domain);
> +	intel_dp_power_put(intel_dp);
>  	return status;
>  }
>  
> @@ -3958,7 +3983,6 @@ 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;
> -	enum intel_display_power_domain power_domain;
>  
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>  		      connector->base.id, connector->name);
> @@ -3967,11 +3991,11 @@ intel_dp_force(struct drm_connector *connector)
>  	if (connector->status != connector_status_connected)
>  		return;
>  
> -	power_domain = intel_dp_power_get(intel_dp);
> +	intel_dp_power_get(intel_dp);
>  
>  	intel_dp_set_edid(intel_dp);
>  
> -	intel_dp_power_put(intel_dp, power_domain);
> +	intel_dp_power_put(intel_dp);
>  
>  	if (intel_encoder->type != INTEL_OUTPUT_EDP)
>  		intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> @@ -4205,7 +4229,8 @@ 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_aux_power_domain(intel_encoder);
> +
>  	intel_display_power_get(dev_priv, power_domain);
>  
>  	if (long_hpd) {
> @@ -4648,6 +4673,9 @@ void intel_edp_panel_vdd_sanitize(struct intel_encoder *intel_encoder)
>  	power_domain = intel_display_port_power_domain(intel_encoder);
>  	intel_display_power_get(dev_priv, power_domain);
>  
> +	power_domain = intel_display_aux_power_domain(intel_encoder);
> +	intel_display_power_get(dev_priv, power_domain);
> +
>  	edp_panel_vdd_schedule_off(intel_dp);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 9558f07..a407d04 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -916,6 +916,8 @@ void hsw_disable_ips(struct intel_crtc *crtc);
>  void intel_display_set_init_power(struct drm_i915_private *dev, bool enable);
>  enum intel_display_power_domain
>  intel_display_port_power_domain(struct intel_encoder *intel_encoder);
> +enum intel_display_power_domain
> +intel_display_aux_power_domain(struct intel_encoder *intel_encoder);
>  void intel_mode_from_pipe_config(struct drm_display_mode *mode,
>  				 struct intel_crtc_config *pipe_config);
>  int intel_format_to_fourcc(int format);


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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