| Message ID | 1463162036-27931-1-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 13, 2016 at 11:13:19PM -0000, Patchwork wrote: > == Series Details == > > Series: drm/i915: Refresh cached DP port register value on resume > URL : https://patchwork.freedesktop.org/series/7163/ > State : failure > > == Summary == > > Series 7163v1 drm/i915: Refresh cached DP port register value on resume > http://patchwork.freedesktop.org/api/1.0/series/7163/revisions/1/mbox > > Test drv_hangman: > Subgroup error-state-basic: > pass -> FAIL (ro-ilk1-i5-650) (drv_hangman:6249) CRITICAL: Test assertion failure function assert_dfs_entry_not, file drv_hangman.c:124: (drv_hangman:6249) CRITICAL: Failed assertion: !(compare_dfs_entry(fname, s) == 0) (drv_hangman:6249) CRITICAL: contents of i915_error_state: 'no error state collected' (expected not 'no error state collected' Subtest error-state-basic failed. https://bugs.freedesktop.org/show_bug.cgi?id=94305 > Test gem_exec_flush: > Subgroup basic-batch-kernel-default-cmd: > fail -> PASS (ro-byt-n2820) > Test kms_flip: > Subgroup basic-flip-vs-wf_vblank: > pass -> FAIL (ro-ivb2-i7-3770) (kms_flip:7723) DEBUG: name = vblank last_ts = 513.961115 usec last_received_ts = 513.960588 usec last_seq = 1174 current_ts = 514.127775 usec current_received_ts = 514.127250 usec current_seq = 1184 count = 9 seq_step = 10 (kms_flip:7723) CRITICAL: Test assertion failure function check_final_state, file kms_flip.c:1192: (kms_flip:7723) CRITICAL: Failed assertion: count >= expected * 99/100 && count <= expected * 101/100 (kms_flip:7723) CRITICAL: Last errno: 25, Inappropriate ioctl for device (kms_flip:7723) CRITICAL: dropped frames, expected 99, counted 100, encoder type 2 https://bugs.freedesktop.org/show_bug.cgi?id=95380 > pass -> FAIL (ro-byt-n2820) (kms_flip:7476) DEBUG: name = flip last_ts = 518.968324 usec last_received_ts = 518.968203 usec last_seq = 14895 current_ts = 519.285148 usec current_received_ts = 519.284928 usec current_seq = 14905 count = 1 seq_step = 1 (kms_flip:7476) CRITICAL: Test assertion failure function check_state, file kms_flip.c:698: (kms_flip:7476) CRITICAL: Failed assertion: fabs((((double) diff.tv_usec) - usec_interflip) / usec_interflip) <= 0.005 (kms_flip:7476) CRITICAL: Last errno: 25, Inappropriate ioctl for device (kms_flip:7476) CRITICAL: inter-flip ts jitter: 0s, 316824usec https://bugs.freedesktop.org/show_bug.cgi?id=94294 > > ro-bdw-i5-5250u total:219 pass:181 dwarn:0 dfail:0 fail:0 skip:38 > ro-bdw-i7-5557U total:219 pass:206 dwarn:0 dfail:0 fail:0 skip:13 > ro-bdw-i7-5600u total:219 pass:187 dwarn:0 dfail:0 fail:0 skip:32 > ro-bsw-n3050 total:219 pass:175 dwarn:0 dfail:0 fail:2 skip:42 > ro-byt-n2820 total:218 pass:174 dwarn:0 dfail:0 fail:3 skip:41 > ro-hsw-i3-4010u total:218 pass:193 dwarn:0 dfail:0 fail:0 skip:25 > ro-hsw-i7-4770r total:219 pass:194 dwarn:0 dfail:0 fail:0 skip:25 > ro-ilk-i7-620lm total:219 pass:151 dwarn:0 dfail:0 fail:1 skip:67 > ro-ilk1-i5-650 total:214 pass:151 dwarn:0 dfail:0 fail:2 skip:61 > ro-ivb-i7-3770 total:219 pass:183 dwarn:0 dfail:0 fail:0 skip:36 > ro-ivb2-i7-3770 total:219 pass:186 dwarn:0 dfail:0 fail:1 skip:32 > ro-skl-i7-6700hq total:214 pass:190 dwarn:0 dfail:0 fail:0 skip:24 > ro-snb-i7-2620M total:219 pass:177 dwarn:0 dfail:0 fail:1 skip:41 > > Results at /archive/results/CI_IGT_test/RO_Patchwork_899/ > > 1a536db drm-intel-nightly: 2016y-05m-13d-21h-21m-06s UTC integration manifest > 9b3616e drm/i915: Refresh cached DP port register value on resume
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); > > /*
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); > > > > /*
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); /*