| Message ID | 1476358446-11621-1-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 13 Oct 2016, Imre Deak <imre.deak@intel.com> wrote: > Currently the display INIT power domain disabling/enabling happens in a > mismatched way in the suspend/resume_early hooks respectively. This can > leave display power wells incorrectly disabled in the resume hook if the > suspend sequence is aborted for some reason resulting in the > suspend/resume hooks getting called but the suspend_late/resume_early > hooks being skipped. In particular this change fixes "Unclaimed read > from register 0x1e1204" on BYT/BSW triggered from i915_drm_resume()-> > intel_pps_unlock_regs_wa() when suspending with /sys/power/pm_test set > to devices. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: David Weinehall <david.weinehall@intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> Fixes: ? Cc: stable@vger.kernel.org ? > --- > drivers/gpu/drm/i915/i915_drv.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index e9b3bfc..5adabac 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1430,8 +1430,6 @@ static int i915_drm_suspend(struct drm_device *dev) > > dev_priv->suspend_count++; > > - intel_display_set_init_power(dev_priv, false); > - > intel_csr_ucode_suspend(dev_priv); > > out: > @@ -1449,6 +1447,8 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation) > > disable_rpm_wakeref_asserts(dev_priv); > > + intel_display_set_init_power(dev_priv, false); > + > fw_csr = !IS_BROXTON(dev_priv) && > suspend_to_idle(dev_priv) && dev_priv->csr.dmc_payload; > /*
On Thu, Oct 13, 2016 at 02:34:06PM +0300, Imre Deak wrote: > Currently the display INIT power domain disabling/enabling happens in a > mismatched way in the suspend/resume_early hooks respectively. This can > leave display power wells incorrectly disabled in the resume hook if the > suspend sequence is aborted for some reason resulting in the > suspend/resume hooks getting called but the suspend_late/resume_early > hooks being skipped. In particular this change fixes "Unclaimed read > from register 0x1e1204" on BYT/BSW triggered from i915_drm_resume()-> > intel_pps_unlock_regs_wa() when suspending with /sys/power/pm_test set > to devices. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: David Weinehall <david.weinehall@intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index e9b3bfc..5adabac 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1430,8 +1430,6 @@ static int i915_drm_suspend(struct drm_device *dev) > > dev_priv->suspend_count++; > > - intel_display_set_init_power(dev_priv, false); > - > intel_csr_ucode_suspend(dev_priv); > > out: > @@ -1449,6 +1447,8 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation) > > disable_rpm_wakeref_asserts(dev_priv); > > + intel_display_set_init_power(dev_priv, false); > + Makes sense in most cases. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > fw_csr = !IS_BROXTON(dev_priv) && > suspend_to_idle(dev_priv) && dev_priv->csr.dmc_payload; However this part is quite confusing. We do the init_power_put unconditionally, but the init_power_get is done from the init_hw() which is conditional on this supend_to_idle special casing. So it seems quite assymmetrical, and I can no longer claim to understand it. > /* > -- > 2.5.0
On to, 2016-10-13 at 15:43 +0300, Ville Syrjälä wrote: > On Thu, Oct 13, 2016 at 02:34:06PM +0300, Imre Deak wrote: > > Currently the display INIT power domain disabling/enabling happens in a > > mismatched way in the suspend/resume_early hooks respectively. This can > > leave display power wells incorrectly disabled in the resume hook if the > > suspend sequence is aborted for some reason resulting in the > > suspend/resume hooks getting called but the suspend_late/resume_early > > hooks being skipped. In particular this change fixes "Unclaimed read > > from register 0x1e1204" on BYT/BSW triggered from i915_drm_resume()-> > > intel_pps_unlock_regs_wa() when suspending with /sys/power/pm_test set > > to devices. > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: David Weinehall <david.weinehall@intel.com> > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index e9b3bfc..5adabac 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -1430,8 +1430,6 @@ static int i915_drm_suspend(struct drm_device *dev) > > > > dev_priv->suspend_count++; > > > > - intel_display_set_init_power(dev_priv, false); > > - > > intel_csr_ucode_suspend(dev_priv); > > > > out: > > @@ -1449,6 +1447,8 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation) > > > > disable_rpm_wakeref_asserts(dev_priv); > > > > + intel_display_set_init_power(dev_priv, false); > > + > > Makes sense in most cases. > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > fw_csr = !IS_BROXTON(dev_priv) && > > suspend_to_idle(dev_priv) && dev_priv->csr.dmc_payload; > > However this part is quite confusing. We do the init_power_put > unconditionally, but the init_power_get is done from the init_hw() > which is conditional on this supend_to_idle special casing. So it seems > quite assymmetrical, and I can no longer claim to understand it. Yep, that's a problem of its own for the suspend-to-idle case, enabling the INIT domain should be also unconditional and happen from this function. I suppose this happened to work so far since it only applies to SKL where nothing depended on the INIT domain, everything takes its own domain ref. Will look into fixing this. --Imre
On to, 2016-10-13 at 15:21 +0300, Jani Nikula wrote: > On Thu, 13 Oct 2016, Imre Deak <imre.deak@intel.com> wrote: > > Currently the display INIT power domain disabling/enabling happens in a > > mismatched way in the suspend/resume_early hooks respectively. This can > > leave display power wells incorrectly disabled in the resume hook if the > > suspend sequence is aborted for some reason resulting in the > > suspend/resume hooks getting called but the suspend_late/resume_early > > hooks being skipped. In particular this change fixes "Unclaimed read > > from register 0x1e1204" on BYT/BSW triggered from i915_drm_resume()-> > > intel_pps_unlock_regs_wa() when suspending with /sys/power/pm_test set > > to devices. > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: David Weinehall <david.weinehall@intel.com> > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > Fixes: ? Fixes: 85e90679335f ("drm/i915: disable power wells on suspend") > Cc: stable@vger.kernel.org ? It matters only for an error path which is rare (another device needs to fail suspending between its suspend and suspend_late steps) and pm_test. So I'd say this isn't for fixes or stable. --Imre > > > --- > > drivers/gpu/drm/i915/i915_drv.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > > b/drivers/gpu/drm/i915/i915_drv.c > > index e9b3bfc..5adabac 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -1430,8 +1430,6 @@ static int i915_drm_suspend(struct drm_device > > *dev) > > > > dev_priv->suspend_count++; > > > > - intel_display_set_init_power(dev_priv, false); > > - > > intel_csr_ucode_suspend(dev_priv); > > > > out: > > @@ -1449,6 +1447,8 @@ static int i915_drm_suspend_late(struct > > drm_device *dev, bool hibernation) > > > > disable_rpm_wakeref_asserts(dev_priv); > > > > + intel_display_set_init_power(dev_priv, false); > > + > > fw_csr = !IS_BROXTON(dev_priv) && > > suspend_to_idle(dev_priv) && dev_priv- > > >csr.dmc_payload; > > /* >
On pe, 2016-10-14 at 11:01 +0000, Patchwork wrote: > == Series Details == > > Series: drm/i915: Fix mismatched INIT power domain disabling during > suspend > URL : https://patchwork.freedesktop.org/series/13723/ > State : warning > > == Summary == > > Series 13723v1 drm/i915: Fix mismatched INIT power domain disabling > during suspend > https://patchwork.freedesktop.org/api/1.0/series/13723/revisions/1/mb > ox/ > > Test drv_module_reload_basic: > skip -> PASS (fi-skl-6770hq) > Test kms_pipe_crc_basic: > Subgroup nonblocking-crc-pipe-a: > pass -> DMESG-WARN (fi-ilk-650) [ 469.277956] [drm:intel_dp_check_link_status [i915]] DP C: channel EQ not ok, retraining [ 469.278371] [drm:intel_dp_set_signal_levels [i915]] Using signal levels 00000000 [ 469.278414] [drm:intel_dp_set_signal_levels [i915]] Using vswing level 0 [ 469.278455] [drm:intel_dp_set_signal_levels [i915]] Using pre-emphasis level 0 [ 469.278499] [drm:intel_dp_program_link_training_pattern [i915]] Using DP training pattern TPS1 [ 469.279490] [drm:intel_dp_set_signal_levels [i915]] Using signal levels 02000000 [ 469.279533] [drm:intel_dp_set_signal_levels [i915]] Using vswing level 1 [ 469.279575] [drm:intel_dp_set_signal_levels [i915]] Using pre-emphasis level 0 [ 469.280337] [drm:intel_dp_start_link_train [i915]] clock recovery OK [ 469.280354] [drm:intel_dp_program_link_training_pattern [i915]] Using DP training pattern TPS2 [ 469.281372] [drm:intel_dp_start_link_train [i915]] Channel EQ done. DP Training successful [ 469.281869] [drm:drm_edid_to_eld] ELD monitor LEN LT2452pwC [ 469.281871] [drm:drm_edid_to_eld] ELD size 36, SAD count 1 [ 469.281915] [drm:intel_pch_fifo_underrun_irq_handler [i915]] *ERROR* PCH transcoder B FIFO underrun [ 469.281979] [drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe B FIFO underrun This happened earlier on the same machine, Ville is looking into suppressing the errors during link retraining. Thanks for the review, pushed the patch to -dinq. > > fi-bdw- > 5557u total:246 pass:231 dwarn:0 dfail:0 fail:0 skip:15 > fi-bsw- > n3050 total:246 pass:204 dwarn:0 dfail:0 fail:0 skip:42 > fi-bxt- > t5700 total:246 pass:216 dwarn:0 dfail:0 fail:0 skip:30 > fi-byt- > j1900 total:246 pass:212 dwarn:2 dfail:0 fail:1 skip:31 > fi-byt- > n2820 total:246 pass:210 dwarn:0 dfail:0 fail:1 skip:35 > fi-hsw- > 4770 total:246 pass:224 dwarn:0 dfail:0 fail:0 skip:22 > fi-hsw- > 4770r total:246 pass:224 dwarn:0 dfail:0 fail:0 skip:22 > fi-ilk- > 650 total:246 pass:183 dwarn:1 dfail:0 fail:2 skip:60 > fi-ivb- > 3520m total:246 pass:221 dwarn:0 dfail:0 fail:0 skip:25 > fi-ivb- > 3770 total:246 pass:221 dwarn:0 dfail:0 fail:0 skip:25 > fi-kbl- > 7200u total:246 pass:222 dwarn:0 dfail:0 fail:0 skip:24 > fi-skl- > 6260u total:246 pass:232 dwarn:0 dfail:0 fail:0 skip:14 > fi-skl- > 6700hq total:246 pass:223 dwarn:0 dfail:0 fail:0 skip:23 > fi-skl- > 6700k total:246 pass:221 dwarn:1 dfail:0 fail:0 skip:24 > fi-skl- > 6770hq total:246 pass:230 dwarn:1 dfail:0 fail:1 skip:14 > fi-snb- > 2520m total:246 pass:210 dwarn:0 dfail:0 fail:0 skip:36 > fi-snb- > 2600 total:246 pass:209 dwarn:0 dfail:0 fail:0 skip:37 > > Results at /archive/results/CI_IGT_test/Patchwork_2715/ > > fc23e80caa1a394f92ca132b2d19ac926bd2c803 drm-intel-nightly: 2016y- > 10m-14d-09h-02m-51s UTC integration manifest > 6a2e371 drm/i915: Fix mismatched INIT power domain disabling during > suspend >
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index e9b3bfc..5adabac 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1430,8 +1430,6 @@ static int i915_drm_suspend(struct drm_device *dev) dev_priv->suspend_count++; - intel_display_set_init_power(dev_priv, false); - intel_csr_ucode_suspend(dev_priv); out: @@ -1449,6 +1447,8 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation) disable_rpm_wakeref_asserts(dev_priv); + intel_display_set_init_power(dev_priv, false); + fw_csr = !IS_BROXTON(dev_priv) && suspend_to_idle(dev_priv) && dev_priv->csr.dmc_payload; /*
Currently the display INIT power domain disabling/enabling happens in a mismatched way in the suspend/resume_early hooks respectively. This can leave display power wells incorrectly disabled in the resume hook if the suspend sequence is aborted for some reason resulting in the suspend/resume hooks getting called but the suspend_late/resume_early hooks being skipped. In particular this change fixes "Unclaimed read from register 0x1e1204" on BYT/BSW triggered from i915_drm_resume()-> intel_pps_unlock_regs_wa() when suspending with /sys/power/pm_test set to devices. Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: David Weinehall <david.weinehall@intel.com> Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)