Re: [PATCH 1/4] drm/i915/bxt: Fix PPS lost state after suspend breaking eDP link training

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

On to, 2016-06-16 at 16:58 +0300, Ville Syrjälä wrote:
> On Thu, Jun 16, 2016 at 04:37:20PM +0300, Imre Deak wrote:
> > The PPS registers are backed by power well #0 and as such may be reset
> > after system or runtime suspend (both implying a possible DC9
> > transition). Fix this by reusing the VLV/CHV PPS pipe-reassignment
> > logic. The difference on BXT is that the PPS instances are not pipe but
> > port (or more accurately pin) specific, so we only need to care about
> > the lost HW state. As opposed to VLV/CHV the SW state is fixed and
> > initialized during connector init.
> > 
> > This also paves the way towards using the actual port->PPS instance
> > mapping based on VBT.
> > 
> > This fixes eDP link training errors on BXT after suspend, where we
> > started the link training too early due to an incorrect T3 (panel power
> > on) register value.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96436
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c         | 71 +++++++++++++++++++++++----------
> >  drivers/gpu/drm/i915/intel_drv.h        |  7 +++-
> >  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +-
> >  3 files changed, 58 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index be08351..19a8bbe 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -426,6 +426,37 @@ vlv_power_sequencer_pipe(struct intel_dp *intel_dp)
> >  	return intel_dp->pps_pipe;
> >  }
> >  
> > +static int
> > +bxt_power_sequencer_idx(struct intel_dp *intel_dp)
> > +{
> > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > +	struct drm_device *dev = intel_dig_port->base.base.dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +	lockdep_assert_held(&dev_priv->pps_mutex);
> > +
> > +	/* We should never land here with regular DP ports */
> > +	WARN_ON(!is_edp(intel_dp));
> > +
> > +	/*
> > +	 * TODO: BXT has 2 PPS instances. The correct port->PPS instance
> > +	 * mapping needs to be retrieved from VBT, for now just hard-code to
> > +	 * use instance #0 always.
> > +	 */
> > +	if (!intel_dp->pps_reset)
> > +		return 0;
> > +
> > +	intel_dp->pps_reset = false;
> > +
> > +	/*
> > +	 * Only the HW needs to be reprogrammed, the SW state is fixed and
> > +	 * has been setup during connector init.
> > +	 */
> > +	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp);
> > +
> > +	return 0;
> > +}
> > +
> >  typedef bool (*vlv_pipe_check)(struct drm_i915_private *dev_priv,
> >  			       enum pipe pipe);
> >  
> > @@ -507,12 +538,13 @@ vlv_initial_power_sequencer_setup(struct intel_dp *intel_dp)
> >  	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp);
> >  }
> >  
> > -void vlv_power_sequencer_reset(struct drm_i915_private *dev_priv)
> > +void intel_power_sequencer_reset(struct drm_i915_private *dev_priv)
> >  {
> >  	struct drm_device *dev = dev_priv->dev;
> >  	struct intel_encoder *encoder;
> >  
> > -	if (WARN_ON(!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)))
> > +	if (WARN_ON(!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev) &&
> > +	    !IS_BROXTON(dev)))
> >  		return;
> >  
> >  	/*
> > @@ -532,7 +564,10 @@ void vlv_power_sequencer_reset(struct drm_i915_private *dev_priv)
> >  			continue;
> >  
> >  		intel_dp = enc_to_intel_dp(&encoder->base);
> > -		intel_dp->pps_pipe = INVALID_PIPE;
> > +		if (IS_BROXTON(dev))
> > +			intel_dp->pps_reset = true;
> > +		else
> > +			intel_dp->pps_pipe = INVALID_PIPE;
> >  	}
> >  }
> >  
> > @@ -542,7 +577,7 @@ _pp_ctrl_reg(struct intel_dp *intel_dp)
> >  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >  
> >  	if (IS_BROXTON(dev))
> > -		return BXT_PP_CONTROL(0);
> > +		return BXT_PP_CONTROL(bxt_power_sequencer_idx(intel_dp));
> >  	else if (HAS_PCH_SPLIT(dev))
> >  		return PCH_PP_CONTROL;
> >  	else
> > @@ -555,7 +590,7 @@ _pp_stat_reg(struct intel_dp *intel_dp)
> >  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >  
> >  	if (IS_BROXTON(dev))
> > -		return BXT_PP_STATUS(0);
> > +		return BXT_PP_STATUS(bxt_power_sequencer_idx(intel_dp));
> >  	else if (HAS_PCH_SPLIT(dev))
> >  		return PCH_PP_STATUS;
> >  	else
> > @@ -4722,14 +4757,11 @@ intel_dp_init_panel_power_sequencer(struct drm_device *dev,
> >  		return;
> >  
> >  	if (IS_BROXTON(dev)) {
> > -		/*
> > -		 * TODO: BXT has 2 sets of PPS registers.
> > -		 * Correct Register for Broxton need to be identified
> > -		 * using VBT. hardcoding for now
> > -		 */
> > -		pp_ctrl_reg = BXT_PP_CONTROL(0);
> > -		pp_on_reg = BXT_PP_ON_DELAYS(0);
> > -		pp_off_reg = BXT_PP_OFF_DELAYS(0);
> > +		int idx = bxt_power_sequencer_idx(intel_dp);
> > +
> > +		pp_ctrl_reg = BXT_PP_CONTROL(idx);
> > +		pp_on_reg = BXT_PP_ON_DELAYS(idx);
> > +		pp_off_reg = BXT_PP_OFF_DELAYS(idx);
> >  	} else if (HAS_PCH_SPLIT(dev)) {
> >  		pp_ctrl_reg = PCH_PP_CONTROL;
> >  		pp_on_reg = PCH_PP_ON_DELAYS;
> > @@ -4842,14 +4874,11 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
> >  	lockdep_assert_held(&dev_priv->pps_mutex);
> >  
> >  	if (IS_BROXTON(dev)) {
> > -		/*
> > -		 * TODO: BXT has 2 sets of PPS registers.
> > -		 * Correct Register for Broxton need to be identified
> > -		 * using VBT. hardcoding for now
> > -		 */
> > -		pp_ctrl_reg = BXT_PP_CONTROL(0);
> > -		pp_on_reg = BXT_PP_ON_DELAYS(0);
> > -		pp_off_reg = BXT_PP_OFF_DELAYS(0);
> > +		int idx = bxt_power_sequencer_idx(intel_dp);
> > +
> > +		pp_ctrl_reg = BXT_PP_CONTROL(idx);
> > +		pp_on_reg = BXT_PP_ON_DELAYS(idx);
> > +		pp_off_reg = BXT_PP_OFF_DELAYS(idx);
> >  
> >  	} else if (HAS_PCH_SPLIT(dev)) {
> >  		pp_on_reg = PCH_PP_ON_DELAYS;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 8dc67ad..870849e 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -869,6 +869,11 @@ struct intel_dp {
> >  	 * this port. Only relevant on VLV/CHV.
> >  	 */
> >  	enum pipe pps_pipe;
> > +	/*
> > +	 * Set if the sequencer may be reset due to a power transition,
> > +	 * requiring a reinitialization. Only relevant on BXT.
> > +	 */
> > +	bool pps_reset;
> >  	struct edp_power_seq pps_delays;
> >  
> >  	bool can_mst; /* this port supports mst */
> > @@ -1348,7 +1353,7 @@ void intel_dp_mst_resume(struct drm_device *dev);
> >  int intel_dp_max_link_rate(struct intel_dp *intel_dp);
> >  int intel_dp_rate_select(struct intel_dp *intel_dp, int rate);
> >  void intel_dp_hot_plug(struct intel_encoder *intel_encoder);
> > -void vlv_power_sequencer_reset(struct drm_i915_private *dev_priv);
> > +void intel_power_sequencer_reset(struct drm_i915_private *dev_priv);
> >  uint32_t intel_dp_pack_aux(const uint8_t *src, int src_bytes);
> >  void intel_plane_destroy(struct drm_plane *plane);
> >  void intel_edp_drrs_enable(struct intel_dp *intel_dp);
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index e856d49..22b46f5 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -578,6 +578,7 @@ void bxt_enable_dc9(struct drm_i915_private *dev_priv)
> >  
> >  	DRM_DEBUG_KMS("Enabling DC9\n");
> >  
> > +	intel_power_sequencer_reset(dev_priv);
> >  	gen9_set_dc_state(dev_priv, DC_STATE_EN_DC9);
> >  }
> 
> I was wondering how we deal with coming out of S4 now, but I suppose we
> currently enable DC9 in .freeze as well. When/if we remove that (to optimize
> away the blinks during hibernation), I think we'll need something different
> to deal with the unknown PPS state on .restore.

Hm yea, I assume we could reset the state in .restore then. But we'd
still need the DC9 time reset even in that case.

> Are you going to do something about PCH platforms as well, or are you going
> leave that for someone else?

I can take a look at that later if no one else does. AFAICS it would
amount to replacing the PPS save/restore in i915_suspend.c with a reset
during system suspend+register re-init based on pps_reset
in intel_pps_get_registers(). I'd have to also check how this would
affect LVDS.

> >  
> > @@ -1112,7 +1113,7 @@ static void vlv_display_power_well_deinit(struct drm_i915_private *dev_priv)
> >  	/* make sure we're done processing display irqs */
> >  	synchronize_irq(dev_priv->dev->irq);
> >  
> > -	vlv_power_sequencer_reset(dev_priv);
> > +	intel_power_sequencer_reset(dev_priv);
> >  }
> >  
> >  static void vlv_display_power_well_enable(struct drm_i915_private *dev_priv,
> > -- 
> > 2.5.0
> > 
> > _______________________________________________
> > 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