| Message ID | 20180828114043.13447-1-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Don't check power domains state in intel_power_domains_init_hw() | expand |
Quoting Imre Deak (2018-08-28 12:40:43) > During power domains initialization we acquire power well references for > power wells in the INIT power domain. The rest of power wells - which > BIOS could have left enabled - we can only acquire references as needed > during display HW readout. Thus during initialization these latter power > wells can have a refcount of 0 while still being enabled. To avoid the > false-positive state mismatch error this causes remove the check from > intel_power_domains_init_hw() and rely on the state check in > intel_power_domains_enable() which follows the HW readout. Missing from above is a quick explanation of how those extraneous powerwells are sanitizied. If we don't do the HW readout (i915.disable_display?) do we not then leave the powerwell active and so complain in a later verify_state()? -Chris
On Tue, Aug 28, 2018 at 12:45:31PM +0100, Chris Wilson wrote: > Quoting Imre Deak (2018-08-28 12:40:43) > > During power domains initialization we acquire power well references for > > power wells in the INIT power domain. The rest of power wells - which > > BIOS could have left enabled - we can only acquire references as needed > > during display HW readout. Thus during initialization these latter power > > wells can have a refcount of 0 while still being enabled. To avoid the > > false-positive state mismatch error this causes remove the check from > > intel_power_domains_init_hw() and rely on the state check in > > intel_power_domains_enable() which follows the HW readout. > > Missing from above is a quick explanation of how those extraneous > powerwells are sanitizied. If we don't do the HW readout > (i915.disable_display?) do we not then leave the powerwell active and so > complain in a later verify_state()? These power wells (AUX and DDI on ICL) can only be enabled/disabled at a specific spot in the modeset sequence, otherwise the power well enable / disable operation will time out. That's the reason they're not part of the INIT domain. For these we will acquire the references during HW readout when noticing that the corresponding display HW block is active and drop them when disabling these HW blocks (normally or as part of display state sanitizatoionin). That way we'll ensure the proper spot mentioned above for power well enabling/disabling. --Imre
On Tue, Aug 28, 2018 at 02:52:20PM +0300, Imre Deak wrote: > On Tue, Aug 28, 2018 at 12:45:31PM +0100, Chris Wilson wrote: > > Quoting Imre Deak (2018-08-28 12:40:43) > > > During power domains initialization we acquire power well references for > > > power wells in the INIT power domain. The rest of power wells - which > > > BIOS could have left enabled - we can only acquire references as needed > > > during display HW readout. Thus during initialization these latter power > > > wells can have a refcount of 0 while still being enabled. To avoid the > > > false-positive state mismatch error this causes remove the check from > > > intel_power_domains_init_hw() and rely on the state check in > > > intel_power_domains_enable() which follows the HW readout. > > > > Missing from above is a quick explanation of how those extraneous > > powerwells are sanitizied. If we don't do the HW readout > > (i915.disable_display?) do we not then leave the powerwell active and so > > complain in a later verify_state()? > > These power wells (AUX and DDI on ICL) can only be enabled/disabled at > a specific spot in the modeset sequence, otherwise the power well > enable / disable operation will time out. That's the reason they're not > part of the INIT domain. For these we will acquire the references during > HW readout when noticing that the corresponding display HW block is active > and drop them when disabling these HW blocks (normally or as part of > display state sanitizatoionin). That way we'll ensure the proper spot > mentioned above for power well enabling/disabling. And that's also the reason why display HW readout had to be added back for i915.disable_display . --Imre
Quoting Imre Deak (2018-08-28 12:56:38) > On Tue, Aug 28, 2018 at 02:52:20PM +0300, Imre Deak wrote: > > On Tue, Aug 28, 2018 at 12:45:31PM +0100, Chris Wilson wrote: > > > Quoting Imre Deak (2018-08-28 12:40:43) > > > > During power domains initialization we acquire power well references for > > > > power wells in the INIT power domain. The rest of power wells - which > > > > BIOS could have left enabled - we can only acquire references as needed > > > > during display HW readout. Thus during initialization these latter power > > > > wells can have a refcount of 0 while still being enabled. To avoid the > > > > false-positive state mismatch error this causes remove the check from > > > > intel_power_domains_init_hw() and rely on the state check in > > > > intel_power_domains_enable() which follows the HW readout. > > > > > > Missing from above is a quick explanation of how those extraneous > > > powerwells are sanitizied. If we don't do the HW readout > > > (i915.disable_display?) do we not then leave the powerwell active and so > > > complain in a later verify_state()? > > > > These power wells (AUX and DDI on ICL) can only be enabled/disabled at > > a specific spot in the modeset sequence, otherwise the power well > > enable / disable operation will time out. That's the reason they're not > > part of the INIT domain. For these we will acquire the references during > > HW readout when noticing that the corresponding display HW block is active > > and drop them when disabling these HW blocks (normally or as part of > > display state sanitizatoionin). That way we'll ensure the proper spot > > mentioned above for power well enabling/disabling. > > And that's also the reason why display HW readout had to be added back > for i915.disable_display . Just add the snippet to the changelog: "...as needed during display HW readout and so must defer sanitization until then (also implying that we must always do HW readout to cleanup unused powerwells)." A reminder in code as well: /* powerwells are sanitized by i_forget_the_function_name() */ -Chris
On Tue, Aug 28, 2018 at 01:49:01PM +0000, Patchwork wrote: > == Series Details == > > Series: drm/i915: Don't check power domains state in intel_power_domains_init_hw() (rev2) > URL : https://patchwork.freedesktop.org/series/48794/ > State : success Pushed to -dinq, thanks for the review. > > == Summary == > > = CI Bug Log - changes from CI_DRM_4714_full -> Patchwork_10028_full = > > == Summary - SUCCESS == > > No regressions found. > > > > == Known issues == > > Here are the changes found in Patchwork_10028_full that come from known issues: > > === IGT changes === > > ==== Issues hit ==== > > igt@drv_suspend@shrink: > shard-kbl: PASS -> INCOMPLETE (fdo#106886, fdo#103665) > > igt@gem_ppgtt@blt-vs-render-ctx0: > shard-kbl: PASS -> INCOMPLETE (fdo#103665, fdo#106023) > > > ==== Possible fixes ==== > > igt@gem_ctx_isolation@vcs0-s3: > shard-kbl: INCOMPLETE (fdo#103665, fdo#107556) -> PASS > > igt@kms_setmode@basic: > shard-apl: FAIL (fdo#99912) -> PASS > > igt@perf@blocking: > shard-hsw: FAIL (fdo#102252) -> PASS > > > fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252 > fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665 > fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023 > fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886 > fdo#107556 https://bugs.freedesktop.org/show_bug.cgi?id=107556 > fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912 > > > == Participating hosts (5 -> 5) == > > No changes in participating hosts > > > == Build changes == > > * Linux: CI_DRM_4714 -> Patchwork_10028 > > CI_DRM_4714: 06fa287b780cfea38a1c599cc1fc3611262668cf @ git://anongit.freedesktop.org/gfx-ci/linux > IGT_4610: c40743d3fce5055682d31610519758dd7379c0f8 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools > Patchwork_10028: 51662a027a87f68fb7fb910b64c83adc91fa4dcb @ git://anongit.freedesktop.org/gfx-ci/linux > piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit > > == Logs == > > For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10028/shards.html
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index 2852395125cd..73d3a6ed3446 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -3767,9 +3767,8 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume) if (!i915_modparams.disable_power_well) intel_display_power_get(dev_priv, POWER_DOMAIN_INIT); intel_power_domains_sync_hw(dev_priv); + power_domains->initializing = false; - - intel_power_domains_verify_state(dev_priv); } /**
During power domains initialization we acquire power well references for power wells in the INIT power domain. The rest of power wells - which BIOS could have left enabled - we can only acquire references as needed during display HW readout. Thus during initialization these latter power wells can have a refcount of 0 while still being enabled. To avoid the false-positive state mismatch error this causes remove the check from intel_power_domains_init_hw() and rely on the state check in intel_power_domains_enable() which follows the HW readout. Fixes: 6dfc4a8f134f ("drm/i915: Verify power domains after enabling them") Cc: Chris Wilson <chris@chris-wilson.co.uk> References: https://bugs.freedesktop.org/show_bug.cgi?id=107411 Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/intel_runtime_pm.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)