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

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

On Fri, 2016-05-13 at 20:53 +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@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.

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

Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  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);
>  
>  	/*