On Fri, Jun 17, 2016 at 09:40:45PM +0300, Imre Deak wrote:
> On Fri, 2016-05-13 at 20:53 +0300, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > Fixes: 6fec76628333 ("drm/i915: Use intel_dp->DP in eDP PLL setup")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> Reviewed-by: Imre Deak <imre.deak@xxxxxxxxx>
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
