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

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

On Thu, Jun 16, 2016 at 05:39:35PM +0300, Imre Deak wrote:
> 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.

Yeah.

> 
> > 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.

I was thinking that might simply move the current save code to
LVDS init, and then add a .reset hook for LVDS to restore the
registers at resume time. But I msut admit that I didn't spend
much time thinking about it.

> 
> > >  
> > > @@ -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
> > 

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