Re: [PATCH 2/2] drm/i915/cnl: New power domain for AUX IO.

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

On Thu, Feb 22, 2018 at 12:28:11PM +0200, Imre Deak wrote:
> On Wed, Feb 21, 2018 at 11:28:56PM -0800, Dhinakaran Pandiyan wrote:
> > PSR on CNL requires AUX IO wells to be kept on and the existing AUX domain
> > for AUX-A enables DC_OFF well too. This is not required, so add a new
> > AUX_IO_A domain for AUX-A to allow DC states to remain enabled. Other AUX
> > channels re-use the existing AUX domains as they do need power well 2.
> > 
> > v2: Add AUX IO domain only for AUX-A
> >     Rebased on top of Ville's AUX series.
> > 
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Suggested-by: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.h    |  1 +
> >  drivers/gpu/drm/i915/intel_dp.c         |  2 +-
> >  drivers/gpu/drm/i915/intel_drv.h        |  1 +
> >  drivers/gpu/drm/i915/intel_psr.c        | 37 +++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +++
> >  5 files changed, 43 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > index f5733a2576e7..4e7418b345bc 100644
> > --- a/drivers/gpu/drm/i915/intel_display.h
> > +++ b/drivers/gpu/drm/i915/intel_display.h
> > @@ -186,6 +186,7 @@ enum intel_display_power_domain {
> >  	POWER_DOMAIN_AUX_C,
> >  	POWER_DOMAIN_AUX_D,
> >  	POWER_DOMAIN_AUX_F,
> > +	POWER_DOMAIN_AUX_IO_A,
> >  	POWER_DOMAIN_GMBUS,
> >  	POWER_DOMAIN_MODESET,
> >  	POWER_DOMAIN_GT_IRQ,
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index eeb8a026fd08..777682a925c9 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1331,7 +1331,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> >  	return ret;
> >  }
> >  
> > -static enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> > +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> >  {
> >  	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index b70ed154c4ce..725a5b8ab611 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1684,6 +1684,7 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
> >  int intel_dp_link_required(int pixel_clock, int bpp);
> >  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
> >  bool intel_digital_port_connected(struct intel_encoder *encoder);
> > +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp);
> >  
> >  /* intel_dp_aux_backlight.c */
> >  int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index 2ef374f936b9..ff77b505534c 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -56,6 +56,39 @@
> >  #include "intel_drv.h"
> >  #include "i915_drv.h"
> >  
> > +static void psr_power_get(struct intel_dp *intel_dp)
> > +{
> > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> > +	enum intel_display_power_domain aux_domain;
> > +
> > +	if (INTEL_GEN(dev_priv) < 10)
> > +		return;
> > +
> > +	/* CNL HW requires corresponding AUX IOs to be powered up for PSR, AUX-A
> > +	 * does not require the driver to disable DC states, but the rest do.
> > +	 * Although PSR is enabled only on Port A currently, let's do this
> > +	 * correctly for other ports too.
> > +	 */
> > +	aux_domain = intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
> 
> The AUX power well is port specific, while there could be an alternative
> port->aux-channel mapping. So we should check the port here. 

At least that's how the driver worked so far .. but that was incorrect
as confirmed now by Art, and Ville's patch fixed it. So you can ignore
the above:/

It's still unclear how this works on ICL onwards at least in TBT and
USBC modes, the selection there could be port specific. Will try to
clarify that with Art.

--Imre

> 
> > +						    intel_dp->aux_power_domain;
> > +	intel_display_power_get(dev_priv, aux_domain);
> > +}
> > +
> > +static void psr_power_put(struct intel_dp *intel_dp)
> > +{
> > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
> > +	enum intel_display_power_domain aux_domain;
> > +
> > +	if (INTEL_GEN(dev_priv) < 10)
> > +		return;
> > +
> > +	aux_domain = intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
> > +						    intel_dp->aux_power_domain;
> > +	intel_display_power_put(dev_priv, aux_domain);
> > +}
> > +
> >  static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > @@ -459,6 +492,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> >  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> >  	u32 chicken;
> >  
> > +	psr_power_get(intel_dp);
> > +
> >  	if (dev_priv->psr.psr2_support) {
> >  		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
> >  		if (dev_priv->psr.y_cord_support)
> > @@ -617,6 +652,8 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
> >  		else
> >  			WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> >  	}
> > +
> > +	psr_power_put(intel_dp);
> >  }
> >  
> >  /**
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index b7924feb9f27..53ea564f971e 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -130,6 +130,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
> >  		return "AUX_D";
> >  	case POWER_DOMAIN_AUX_F:
> >  		return "AUX_F";
> > +	case POWER_DOMAIN_AUX_IO_A:
> > +		return "AUX_IO_A";
> >  	case POWER_DOMAIN_GMBUS:
> >  		return "GMBUS";
> >  	case POWER_DOMAIN_INIT:
> > @@ -1853,6 +1855,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> >  	BIT_ULL(POWER_DOMAIN_INIT))
> >  #define CNL_DISPLAY_AUX_A_POWER_DOMAINS (		\
> >  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
> > +	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
> >  	BIT_ULL(POWER_DOMAIN_INIT))
> >  #define CNL_DISPLAY_AUX_B_POWER_DOMAINS (		\
> >  	BIT_ULL(POWER_DOMAIN_AUX_B) |			\
> > -- 
> > 2.14.1
> > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx