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

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

[ville.syrjala]

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.

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

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

[Imre Deak]

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

Re: [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@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.

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

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