Re: [PATCH v3] drm/i915/psr: New power domain for AUX IO.

["Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>]

On Thu, 2018-02-22 at 12:44 -0800, Rodrigo Vivi wrote:
> On Thu, Feb 22, 2018 at 12:28:50PM -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.
> > 
> > v3: Extract aux domain selection into a function (Ville)
> > 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        | 38 +++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +++
> >  5 files changed, 44 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 005735a7fc29..b78b9972ebae 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 5132f6a58c6d..bcd6dc9fb13d 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1685,6 +1685,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..03814f7bc2d3 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -56,6 +56,40 @@
> >  #include "intel_drv.h"
> >  #include "i915_drv.h"
> >  
> > +static inline enum intel_display_power_domain
> > +psr_aux_domain(struct intel_dp *intel_dp)
> > +{
> > +	/* 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.
> 
> This phrase is strange. Specially "the rest do" part.
> It seems that we need to disable DC states on other ports than A,
> what it is not true.

Okay, let's back up a little bit.

AUX domain for port A enables DC_OFF and AUX-IO. AUX domains for non-A
ports enable DC_OFF, power well 2 and AUX-IO, in fact all domains that
require power well 2 enable DC_OFF too. I can't see this is in bspec but
that's what the code does currently. So, this is for AUX transfers.

which means, this comment for the previous iteration of the patch
"I mean, on CNL

POWER_DOMAIN_AUX_{B,C,D,F} are exactly the same as
POWER_DOMAIN_AUX__IO_{B,C,D,F}"

was not accurate. _AUX_IO_{B,C,D,F} domains enable only AUX_IO well and
_AUX_{B,C,D,F} domains enable power well 2, DC_OFF and AUX_IO.


For PSR, we want only the AUX_IO to be enabled for any port because the
spec says -
"PSR spontaneously sends Aux transactions.  If PSR is enabled on a port,
then the associated Aux IO must be kept powered up."

which means, this comment
"Hm, so in general (for AUX transfers) to enable AUX-A we first need to
disable DC states _except_ if we enable AUX-A for PSR where we want
DC5/6 to stay enabled." applies to all ports.

To just enable the AUX-IO well, the correct version is ->
https://patchwork.freedesktop.org/patch/205328/



-DK 
> 
> > +	 * Although PSR is enabled only on Port A currently, let's do this
> > +	 * correctly for other ports too.
> > +	 */
> > +	return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
> > +					      intel_dp->aux_power_domain;
> > +}
> > +
> > +static void psr_power_get(struct intel_dp *intel_dp)
> 
> nip: psr_aux_io_power_get ?!
> 
> > +{
> > +	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);
> > +
> > +	if (INTEL_GEN(dev_priv) < 10)
> > +		return;
> > +
> > +	intel_display_power_get(dev_priv, psr_aux_domain(intel_dp));
> > +}
> > +
> > +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);
> > +
> > +	if (INTEL_GEN(dev_priv) < 10)
> > +		return;
> > +
> > +	intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
> > +}
> > +
> >  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 +493,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 +653,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