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

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

On Thu, Nov 12, 2015 at 10:02:36AM +0100, Patrik Jakobsson wrote:
> On Wed, Nov 11, 2015 at 08:37:36PM +0200, Ville Syrjälä wrote:
> > On Wed, Nov 11, 2015 at 08:22:03PM +0200, Imre Deak wrote:
> > > 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.
> > 
> > Yeah, so there's no dedicated AUX block for port E, and instead VBT
> > tells us which AUX block to use. The current code doin that is rather
> > messy, but I have a cleaned it up during my register type-safety
> > journey. I just reposted the remaining AUX related patches [1] from
> > that series.
> > 
> > So I was thinking that we could include an 'enum port aux_port' inside
> > intel_dp, and use that to pick the right power domain.
> > 
> > [1] http://lists.freedesktop.org/archives/intel-gfx/2015-November/079918.html
> 
> Ok so one of Aux A-D is hardwired to port E and only VBT can tell us which?

Yep.

> Do
> we need to change anything in this patch or can we add the aux_port enum later
> on?

Hmm. Lemme think. Before, we too the port power domain, so DDI_E, and
after the patch we would hit the default: case and get the WARN.
So that would be a regression of sorts. I guess as a quick hack you
could return eg. POWER_DOMAIN_AUX_D for port E, and add a FIXME that it
needs to be fixed, if you don't want to tackle the aux_port idea (or
something silimar right now).

On a side note maybe we should add MISSING_CASE_ONCE() to get some
better debug output from these kinds of cases...

> 
> > 
> > > 
> > > > +
> > > >  #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);
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC

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