[Intel-gfx] [PATCH] drm/i915: Refresh cached DP port register value on resume

[Ville Syrjälä]

On Fri, Jun 17, 2016 at 09:40:45PM +0300, Imre Deak wrote:
> On Fri, 2016-05-13 at 20:53 +0300, ville.syrjala at linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > 
> > During hibernation the cached DP port register value will be left with
> > whatever value we have there when we create the hibernation image.
> > Currently that means the port (and eDP PLL) will be off in the cached
> > value. However when we resume there is no guarantee that the value
> > in the actual register will match the cached value. If i915 isn't
> > loaded in the kernel that loads the hibernation image, the port may
> > well be on (eg. left on by the BIOS). The encoder state readout
> > does the right thing in this case and updates our encoder state
> > to reflect the actual hardware state. However the post-resume modeset
> > will then use the stale cached port register value in
> > intel_dp_link_down() and potentially confuse the hardware.
> > 
> > This was caught by the following assert
> >  WARNING: CPU: 3 PID: 5288 at ../drivers/gpu/drm/i915/intel_dp.c:2184 assert_edp_pll+0x99/0xa0 [i915]
> >  eDP PLL state assertion failure (expected on, current off)
> > on account of the eDP PLL getting prematurely turned off when
> > shutting down the port, since the DP_PLL_ENABLE bit wasn't set
> > in the cached register value.
> > 
> > Presumably I introduced this problem in
> > commit 6fec76628333 ("drm/i915: Use intel_dp->DP in eDP PLL setup")
> > as before that we didn't update the cached value after shuttting the
> > port down. That's assuming the port got enabled at least once prior
> > to hibernating. If that didn't happen then the cached value would
> > still have been totally out of sync with reality (eg. first boot w/o
> > eDP on, then hibernate, and then resume with eDP on).
> > 
> > So, let's fix this properly and refresh the cached register value from
> > the hardware register during resume.
> > 
> > DDI platforms shouldn't use the cached value during port disable at
> > least, so shouldn't have this particular issue. They might still have
> > issues if we skip the initial modeset and then try to retrain the link
> > or something. But untangling this DP vs. DDI mess is a bigger topic,
> > so let's jut punt on DDI for now.
> 
> Since the DDI link retraining code seems to reset all relevant parts of
> intel_dp->DP (lane, vswing, port enabled) would the above scenario be
> really a problem? But syncing intel_dp->DP the same way for DDI makes
> sense to me in any case.

I didn't look too closely at the DDI case so far, so not sure what's
going on there.

> 
> > Cc: Jani Nikula <jani.nikula at intel.com>
> > Cc: stable at vger.kernel.org
> > Fixes: 6fec76628333 ("drm/i915: Use intel_dp->DP in eDP PLL setup")
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> Reviewed-by: Imre Deak <imre.deak at intel.com>

Pushed to dinq. Thanks for the review.

> 
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 36330026ceff..a3f38115a3bd 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4522,13 +4522,15 @@ static void intel_edp_panel_vdd_sanitize(struct intel_dp *intel_dp)
> >  
> >  void intel_dp_encoder_reset(struct drm_encoder *encoder)
> >  {
> > -	struct intel_dp *intel_dp;
> > +	struct drm_i915_private *dev_priv = to_i915(encoder->dev);
> > +	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > +
> > +	if (!HAS_DDI(dev_priv))
> > +		intel_dp->DP = I915_READ(intel_dp->output_reg);
> >  
> >  	if (to_intel_encoder(encoder)->type != INTEL_OUTPUT_EDP)
> >  		return;
> >  
> > -	intel_dp = enc_to_intel_dp(encoder);
> > -
> >  	pps_lock(intel_dp);
> >  
> >  	/*

-- 
Ville Syrjälä
Intel OTC