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.
> 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>
> ---
> 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);
>
> /*
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html