| Message ID | 20180817122613.5449-1-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Verify power domains after enabling them | expand |
Quoting Imre Deak (2018-08-17 13:26:13) > After > commit 2cd9a689e97b ("Refactor intel_display_set_init_power() logic") > it makes more sense to check the power domain/well refcounts after > enabling the power domains functionality. Before that it's guaranteed > that most power wells (in the INIT domain) will have a reference held, > so not an interesting state. Indeed, that is true. But it is also used to check that we do acquire the powerwells on init. I think it would sensible to include a verify state at the end of power_domains_init_hw, or do you think the sync_hw makes that superfluous? -Chris
On Fri, Aug 17, 2018 at 01:32:24PM +0100, Chris Wilson wrote: > Quoting Imre Deak (2018-08-17 13:26:13) > > After > > commit 2cd9a689e97b ("Refactor intel_display_set_init_power() logic") > > it makes more sense to check the power domain/well refcounts after > > enabling the power domains functionality. Before that it's guaranteed > > that most power wells (in the INIT domain) will have a reference held, > > so not an interesting state. > > Indeed, that is true. But it is also used to check that we do acquire > the powerwells on init. Yes, intel_power_domains_enable() is called both during init and system resume, after HW readout and acquiring the needed power wells. > I think it would sensible to include a verify state at the end of > power_domains_init_hw, or do you think the sync_hw makes that > superfluous? We have all power wells in the INIT domain enabled there, so I thought it's less interesting, but yes I can also add the check to init_hw, fini_hw, enable/disable and suspend/resume steps. --Imre
Quoting Imre Deak (2018-08-17 13:53:06) > On Fri, Aug 17, 2018 at 01:32:24PM +0100, Chris Wilson wrote: > > Quoting Imre Deak (2018-08-17 13:26:13) > > > After > > > commit 2cd9a689e97b ("Refactor intel_display_set_init_power() logic") > > > it makes more sense to check the power domain/well refcounts after > > > enabling the power domains functionality. Before that it's guaranteed > > > that most power wells (in the INIT domain) will have a reference held, > > > so not an interesting state. > > > > Indeed, that is true. But it is also used to check that we do acquire > > the powerwells on init. > > Yes, intel_power_domains_enable() is called both during init and system > resume, after HW readout and acquiring the needed power wells. > > > I think it would sensible to include a verify state at the end of > > power_domains_init_hw, or do you think the sync_hw makes that > > superfluous? > > We have all power wells in the INIT domain enabled there, so I thought > it's less interesting, but yes I can also add the check to init_hw, > fini_hw, enable/disable and suspend/resume steps. Yup, agree on the less interesting, but still useful to nip errors in the bud where we require a powerwell but failed to bring the HW up. And the pm state change is so infrequent (albeit suspend/resume are latency sensitive for some environments) that a bit of sanity checking is worthwhile. On the other hand, if it does take over 1ms to verify the state, perhaps not for resume. ;) -Chris
On Fri, Aug 17, 2018 at 01:44:57PM +0000, Patchwork wrote: > == Series Details == > > Series: drm/i915: Verify power domains after enabling them (rev2) > URL : https://patchwork.freedesktop.org/series/48394/ > State : failure > > == Summary == > > = CI Bug Log - changes from CI_DRM_4685 -> Patchwork_9971 = > > == Summary - FAILURE == > > Serious unknown changes coming with Patchwork_9971 absolutely need to be > verified manually. > > If you think the reported changes have nothing to do with the changes > introduced in Patchwork_9971, please notify your bug team to allow them > to document this new failure mode, which will reduce false positives in CI. > > External URL: https://patchwork.freedesktop.org/api/1.0/series/48394/revisions/2/mbox/ > > == Possible new issues == > > Here are the unknown changes that may have been introduced in Patchwork_9971: > > === IGT changes === > > ==== Possible regressions ==== > > igt@gem_exec_suspend@basic-s3: > fi-skl-6770hq: PASS -> DMESG-WARN +3 > {fi-cfl-8109u}: PASS -> DMESG-WARN +3 > fi-cfl-s3: PASS -> DMESG-WARN +3 > fi-skl-6260u: PASS -> DMESG-WARN +3 > fi-kbl-7567u: PASS -> DMESG-WARN +3 > fi-kbl-guc: PASS -> DMESG-WARN > {fi-kbl-8809g}: PASS -> DMESG-WARN Err, [ 304.564287] [drm:intel_power_domains_verify_state [i915]] *ERROR* power well DC off state mismatch (refcount 0/enabled 1) After deiniting display core/DMC firmware DC states will be disabled, even though we don't hold a reference on the dc_off power well. Will fix the check for that case in v3. > > igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a: > fi-whl-u: PASS -> DMESG-WARN +3 > fi-cfl-guc: PASS -> DMESG-WARN +3 > fi-glk-j4005: PASS -> DMESG-WARN +3 > fi-kbl-7500u: PASS -> DMESG-WARN +3 > fi-kbl-7560u: PASS -> DMESG-WARN +3 > > igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b: > {fi-skl-iommu}: PASS -> DMESG-WARN +3 > fi-skl-6700k2: PASS -> DMESG-WARN +4 > fi-skl-6700hq: PASS -> DMESG-WARN +3 > > igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c: > fi-glk-dsi: PASS -> DMESG-WARN +3 > fi-cfl-8700k: PASS -> DMESG-WARN +3 > fi-skl-guc: PASS -> DMESG-WARN +3 > fi-bxt-j4205: PASS -> DMESG-WARN +3 > fi-bxt-dsi: PASS -> DMESG-WARN +3 > fi-cnl-psr: PASS -> DMESG-WARN +3 > fi-skl-gvtdvm: PASS -> DMESG-WARN +3 > fi-skl-6600u: PASS -> DMESG-WARN +3 > > > ==== Warnings ==== > > {igt@pm_rpm@module-reload}: > {fi-icl-u}: WARN (fdo#107602) -> DMESG-FAIL > > > == Known issues == > > Here are the changes found in Patchwork_9971 that come from known issues: > > === IGT changes === > > ==== Issues hit ==== > > igt@drv_selftest@live_guc: > {fi-icl-u}: PASS -> DMESG-WARN (fdo#107591) +14 > > igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c: > {fi-icl-u}: PASS -> DMESG-WARN (fdo#107382) +3 > > {igt@pm_rpm@module-reload}: > fi-byt-j1900: NOTRUN -> WARN (fdo#107602) > > > ==== Possible fixes ==== > > igt@gem_exec_basic@readonly-blt: > fi-byt-n2820: FAIL (fdo#105900) -> PASS > > igt@kms_frontbuffer_tracking@basic: > fi-hsw-peppy: DMESG-FAIL (fdo#102614) -> PASS > > igt@prime_vgem@basic-fence-flip: > fi-ivb-3770: FAIL (fdo#104008) -> PASS > > > {name}: This element is suppressed. This means it is ignored when computing > the status of the difference (SUCCESS, WARNING, or FAILURE). > > fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614 > fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008 > fdo#105900 https://bugs.freedesktop.org/show_bug.cgi?id=105900 > fdo#107382 https://bugs.freedesktop.org/show_bug.cgi?id=107382 > fdo#107591 https://bugs.freedesktop.org/show_bug.cgi?id=107591 > fdo#107602 https://bugs.freedesktop.org/show_bug.cgi?id=107602 > > > == Participating hosts (52 -> 49) == > > Additional (1): fi-byt-j1900 > Missing (4): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-hsw-4200u > > > == Build changes == > > * Linux: CI_DRM_4685 -> Patchwork_9971 > > CI_DRM_4685: df7e8eddc3830216d3fec15e2c7d0b6ec97e7bae @ git://anongit.freedesktop.org/gfx-ci/linux > IGT_4606: 38a44003774e35c587c67c8766b35e75dbb993b8 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools > Patchwork_9971: 3f95a53c8ade0421b75908a68a0dad7a3e6d106c @ git://anongit.freedesktop.org/gfx-ci/linux > > > == Linux commits == > > 3f95a53c8ade drm/i915: Verify power domains after enabling them > > == Logs == > > For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9971/issues.html
Quoting Patchwork (2018-08-17 14:44:57) > == Series Details == > > Series: drm/i915: Verify power domains after enabling them (rev2) > URL : https://patchwork.freedesktop.org/series/48394/ > State : failure > > == Summary == > > = CI Bug Log - changes from CI_DRM_4685 -> Patchwork_9971 = > > == Summary - FAILURE == > > Serious unknown changes coming with Patchwork_9971 absolutely need to be > verified manually. > > If you think the reported changes have nothing to do with the changes > introduced in Patchwork_9971, please notify your bug team to allow them > to document this new failure mode, which will reduce false positives in CI. > > External URL: https://patchwork.freedesktop.org/api/1.0/series/48394/revisions/2/mbox/ > > == Possible new issues == > > Here are the unknown changes that may have been introduced in Patchwork_9971: > > === IGT changes === > > ==== Possible regressions ==== > > igt@gem_exec_suspend@basic-s3: > fi-skl-6770hq: PASS -> DMESG-WARN +3 > {fi-cfl-8109u}: PASS -> DMESG-WARN +3 > fi-cfl-s3: PASS -> DMESG-WARN +3 > fi-skl-6260u: PASS -> DMESG-WARN +3 > fi-kbl-7567u: PASS -> DMESG-WARN +3 > fi-kbl-guc: PASS -> DMESG-WARN > {fi-kbl-8809g}: PASS -> DMESG-WARN All CSR platforms. There's some floating state across suspend? -Chris
On Fri, Aug 17, 2018 at 03:24:40PM +0000, Patchwork wrote: > == Series Details == > > Series: drm/i915: Verify power domains after enabling them (rev3) > URL : https://patchwork.freedesktop.org/series/48394/ > State : success Pushed to -dinq with i915_welcome_messages() updated, thanks for the review. > > == Summary == > > = CI Bug Log - changes from CI_DRM_4685 -> Patchwork_9973 = > > == Summary - SUCCESS == > > No regressions found. > > External URL: https://patchwork.freedesktop.org/api/1.0/series/48394/revisions/3/mbox/ > > == Possible new issues == > > Here are the unknown changes that may have been introduced in Patchwork_9973: > > === IGT changes === > > ==== Warnings ==== > > {igt@pm_rpm@module-reload}: > {fi-icl-u}: WARN (fdo#107602) -> DMESG-FAIL > > > == Known issues == > > Here are the changes found in Patchwork_9973 that come from known issues: > > === IGT changes === > > ==== Issues hit ==== > > igt@drv_selftest@live_guc: > {fi-icl-u}: PASS -> DMESG-WARN (fdo#107591) +14 > > igt@drv_selftest@live_hangcheck: > fi-skl-6600u: PASS -> DMESG-FAIL (fdo#106560, fdo#107174) > > igt@gem_exec_suspend@basic-s4-devices: > fi-kbl-7500u: PASS -> DMESG-WARN (fdo#107139, fdo#105128) > > igt@kms_frontbuffer_tracking@basic: > {fi-byt-clapper}: PASS -> FAIL (fdo#103167) > > igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a: > {fi-byt-clapper}: PASS -> FAIL (fdo#107362) > > {igt@pm_rpm@module-reload}: > fi-byt-j1900: NOTRUN -> WARN (fdo#107602) > > > ==== Possible fixes ==== > > igt@drv_selftest@live_hangcheck: > fi-skl-guc: DMESG-FAIL (fdo#107174) -> PASS > > igt@gem_exec_basic@readonly-blt: > fi-byt-n2820: FAIL (fdo#105900) -> PASS > > igt@kms_frontbuffer_tracking@basic: > fi-hsw-peppy: DMESG-FAIL (fdo#102614) -> PASS > > igt@prime_vgem@basic-fence-flip: > fi-ivb-3770: FAIL (fdo#104008) -> PASS > > > {name}: This element is suppressed. This means it is ignored when computing > the status of the difference (SUCCESS, WARNING, or FAILURE). > > fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614 > fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167 > fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008 > fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128 > fdo#105900 https://bugs.freedesktop.org/show_bug.cgi?id=105900 > fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560 > fdo#107139 https://bugs.freedesktop.org/show_bug.cgi?id=107139 > fdo#107174 https://bugs.freedesktop.org/show_bug.cgi?id=107174 > fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362 > fdo#107591 https://bugs.freedesktop.org/show_bug.cgi?id=107591 > fdo#107602 https://bugs.freedesktop.org/show_bug.cgi?id=107602 > > > == Participating hosts (52 -> 49) == > > Additional (1): fi-byt-j1900 > Missing (4): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-hsw-4200u > > > == Build changes == > > * Linux: CI_DRM_4685 -> Patchwork_9973 > > CI_DRM_4685: df7e8eddc3830216d3fec15e2c7d0b6ec97e7bae @ git://anongit.freedesktop.org/gfx-ci/linux > IGT_4606: 38a44003774e35c587c67c8766b35e75dbb993b8 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools > Patchwork_9973: c7bd0b63b5794846f480ad271ce92866cb41c5eb @ git://anongit.freedesktop.org/gfx-ci/linux > > > == Linux commits == > > c7bd0b63b579 drm/i915: Verify power domains after enabling them > > == Logs == > > For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9973/issues.html
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 09d62b0c62cb..ad0f0e5389d9 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15907,8 +15907,6 @@ intel_modeset_setup_hw_state(struct drm_device *dev, intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); - intel_power_domains_verify_state(dev_priv); - intel_fbc_init_pipe_state(dev_priv); } diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index 6153d5be5cf6..fad39239ef9c 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -3805,6 +3805,8 @@ void intel_power_domains_fini_hw(struct drm_i915_private *dev_priv) void intel_power_domains_enable(struct drm_i915_private *dev_priv) { intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); + + intel_power_domains_verify_state(dev_priv); } /**
After commit 2cd9a689e97b ("Refactor intel_display_set_init_power() logic") it makes more sense to check the power domain/well refcounts after enabling the power domains functionality. Before that it's guaranteed that most power wells (in the INIT domain) will have a reference held, so not an interesting state. Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 2 -- drivers/gpu/drm/i915/intel_runtime_pm.c | 2 ++ 2 files changed, 2 insertions(+), 2 deletions(-)