* [PATCH v4 1/4] drm/i915: Fix false-positive assert_rpm_wakelock_held in i915_pmic_bus_access_notifier
@ 2017-08-20 12:59 Hans de Goede
2017-08-20 12:59 ` [PATCH v4 2/4] drm/i915: Re-register PMIC bus access notifier on runtime resume Hans de Goede
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Hans de Goede @ 2017-08-20 12:59 UTC (permalink / raw)
To: Daniel Vetter, Jani Nikula, Ville Syrjälä, Imre Deak
Cc: Hans de Goede, intel-gfx, dri-devel
assert_rpm_wakelock_held is triggered from i915_pmic_bus_access_notifier
even though it gets unregistered on (runtime) suspend, this is caused
by a race happening under the following circumstances:
intel_runtime_pm_put does:
atomic_dec(&dev_priv->pm.wakeref_count);
pm_runtime_mark_last_busy(kdev);
pm_runtime_put_autosuspend(kdev);
And pm_runtime_put_autosuspend calls intel_runtime_suspend from
a workqueue, so there is ample of time between the atomic_dec() and
intel_runtime_suspend() unregistering the notifier. If the notifier
gets called in this windowd assert_rpm_wakelock_held falsely triggers
(at this point we're not runtime-suspended yet).
This commit adds disable_rpm_wakeref_asserts and
enable_rpm_wakeref_asserts calls around the
intel_uncore_forcewake_get(FORCEWAKE_ALL) call in
i915_pmic_bus_access_notifier fixing the false-positive WARN_ON.
Reported-by: FKr <bugs-freedesktop@ubermail.me>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Imre Deak <imre.deak@intel.com>
---
Changes in v2:
-Rebase on current (July 6th 2017) drm-next
Changes in v3:
-Reword comment explaining why disabling the wakeref asserts is
ok and necessary
-Add Imre's Reviewed-by
---
drivers/gpu/drm/i915/intel_uncore.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 1d7b879cc68c..2d3aad319229 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1171,8 +1171,15 @@ static int i915_pmic_bus_access_notifier(struct notifier_block *nb,
* bus, which will be busy after this notification, leading to:
* "render: timed out waiting for forcewake ack request."
* errors.
+ *
+ * The notifier is unregistered during intel_runtime_suspend(),
+ * so it's ok to access the HW here without holding a RPM
+ * wake reference -> disable wakeref asserts for the time of
+ * the access.
*/
+ disable_rpm_wakeref_asserts(dev_priv);
intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
+ enable_rpm_wakeref_asserts(dev_priv);
break;
case MBI_PMIC_BUS_ACCESS_END:
intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
--
2.13.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v4 2/4] drm/i915: Re-register PMIC bus access notifier on runtime resume 2017-08-20 12:59 [PATCH v4 1/4] drm/i915: Fix false-positive assert_rpm_wakelock_held in i915_pmic_bus_access_notifier Hans de Goede @ 2017-08-20 12:59 ` Hans de Goede 2017-08-20 12:59 ` [PATCH v4 3/4] drm/i915: Call uncore_suspend before platform suspend handlers Hans de Goede ` (2 subsequent siblings) 3 siblings, 0 replies; 9+ messages in thread From: Hans de Goede @ 2017-08-20 12:59 UTC (permalink / raw) To: Daniel Vetter, Jani Nikula, Ville Syrjälä, Imre Deak Cc: Hans de Goede, intel-gfx, dri-devel intel_uncore_suspend() unregisters the uncore code's PMIC bus access notifier and gets called on both normal and runtime suspend. intel_uncore_resume_early() re-registers the notifier, but only on normal resume. Add a new intel_uncore_runtime_resume() function which only re-registers the notifier and call that on runtime resume. Reported-by: Imre Deak <imre.deak@intel.com> Signed-off-by: Hans de Goede <hdegoede@redhat.com> Reviewed-by: Imre Deak <imre.deak@intel.com> --- Changes in v2: -Rebase on current (July 6th 2017) drm-next Changes in v3: -Add Imre's Reviewed-by --- drivers/gpu/drm/i915/i915_drv.c | 2 ++ drivers/gpu/drm/i915/intel_uncore.c | 6 ++++++ drivers/gpu/drm/i915/intel_uncore.h | 1 + 3 files changed, 9 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 43100229613c..4715f320c8fa 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -2585,6 +2585,8 @@ static int intel_runtime_resume(struct device *kdev) ret = vlv_resume_prepare(dev_priv, true); } + intel_uncore_runtime_resume(dev_priv); + /* * No point of rolling back things in case of an error, as the best * we can do is to hope that things will still work (and disable RPM). diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 2d3aad319229..e9ed02518406 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -434,6 +434,12 @@ void intel_uncore_resume_early(struct drm_i915_private *dev_priv) i915_check_and_clear_faults(dev_priv); } +void intel_uncore_runtime_resume(struct drm_i915_private *dev_priv) +{ + iosf_mbi_register_pmic_bus_access_notifier( + &dev_priv->uncore.pmic_bus_access_nb); +} + void intel_uncore_sanitize(struct drm_i915_private *dev_priv) { i915.enable_rc6 = sanitize_rc6_option(dev_priv, i915.enable_rc6); diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h index 5f90278da461..0bdc3fcc0e64 100644 --- a/drivers/gpu/drm/i915/intel_uncore.h +++ b/drivers/gpu/drm/i915/intel_uncore.h @@ -121,6 +121,7 @@ bool intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv void intel_uncore_fini(struct drm_i915_private *dev_priv); void intel_uncore_suspend(struct drm_i915_private *dev_priv); void intel_uncore_resume_early(struct drm_i915_private *dev_priv); +void intel_uncore_runtime_resume(struct drm_i915_private *dev_priv); u64 intel_uncore_edram_size(struct drm_i915_private *dev_priv); void assert_forcewakes_inactive(struct drm_i915_private *dev_priv); -- 2.13.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v4 3/4] drm/i915: Call uncore_suspend before platform suspend handlers 2017-08-20 12:59 [PATCH v4 1/4] drm/i915: Fix false-positive assert_rpm_wakelock_held in i915_pmic_bus_access_notifier Hans de Goede 2017-08-20 12:59 ` [PATCH v4 2/4] drm/i915: Re-register PMIC bus access notifier on runtime resume Hans de Goede @ 2017-08-20 12:59 ` Hans de Goede 2017-08-20 12:59 ` [PATCH v4 4/4] drm/i915: Acquire PUNIT->PMIC bus for intel_uncore_forcewake_reset() Hans de Goede 2017-08-20 13:36 ` ✓ Fi.CI.BAT: success for series starting with [v4,1/4] drm/i915: Fix false-positive assert_rpm_wakelock_held in i915_pmic_bus_access_notifier Patchwork 3 siblings, 0 replies; 9+ messages in thread From: Hans de Goede @ 2017-08-20 12:59 UTC (permalink / raw) To: Daniel Vetter, Jani Nikula, Ville Syrjälä, Imre Deak Cc: Hans de Goede, intel-gfx, dri-devel Quoting Ville: "the forcewake timer might still be active until the uncore suspend, and having active forcewakes while we've already told the GT wake stuff to stop acting normally doesn't seem quite right to me." Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Suggested-by: Imre Deak <imre.deak@intel.com> Signed-off-by: Hans de Goede <hdegoede@redhat.com> Reviewed-by: Imre Deak <imre.deak@intel.com> --- Changes in v2: -Rebase on current (July 6th 2017) drm-next Changes in v3: -Add Imre's Reviewed-by --- drivers/gpu/drm/i915/i915_drv.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 4715f320c8fa..5bf231abe010 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -2490,6 +2490,8 @@ static int intel_runtime_suspend(struct device *kdev) intel_runtime_pm_disable_interrupts(dev_priv); + intel_uncore_suspend(dev_priv); + ret = 0; if (IS_GEN9_LP(dev_priv)) { bxt_display_core_uninit(dev_priv); @@ -2502,6 +2504,8 @@ static int intel_runtime_suspend(struct device *kdev) if (ret) { DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret); + intel_uncore_runtime_resume(dev_priv); + intel_runtime_pm_enable_interrupts(dev_priv); enable_rpm_wakeref_asserts(dev_priv); @@ -2509,8 +2513,6 @@ static int intel_runtime_suspend(struct device *kdev) return ret; } - intel_uncore_suspend(dev_priv); - enable_rpm_wakeref_asserts(dev_priv); WARN_ON_ONCE(atomic_read(&dev_priv->pm.wakeref_count)); -- 2.13.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v4 4/4] drm/i915: Acquire PUNIT->PMIC bus for intel_uncore_forcewake_reset() 2017-08-20 12:59 [PATCH v4 1/4] drm/i915: Fix false-positive assert_rpm_wakelock_held in i915_pmic_bus_access_notifier Hans de Goede 2017-08-20 12:59 ` [PATCH v4 2/4] drm/i915: Re-register PMIC bus access notifier on runtime resume Hans de Goede 2017-08-20 12:59 ` [PATCH v4 3/4] drm/i915: Call uncore_suspend before platform suspend handlers Hans de Goede @ 2017-08-20 12:59 ` Hans de Goede 2017-08-22 9:00 ` Imre Deak 2017-09-04 13:23 ` Jani Nikula 2017-08-20 13:36 ` ✓ Fi.CI.BAT: success for series starting with [v4,1/4] drm/i915: Fix false-positive assert_rpm_wakelock_held in i915_pmic_bus_access_notifier Patchwork 3 siblings, 2 replies; 9+ messages in thread From: Hans de Goede @ 2017-08-20 12:59 UTC (permalink / raw) To: Daniel Vetter, Jani Nikula, Ville Syrjälä, Imre Deak Cc: Hans de Goede, intel-gfx, dri-devel intel_uncore_forcewake_reset() does forcewake puts and gets as such we need to make sure that no-one tries to access the PUNIT->PMIC bus (on systems where this bus is shared) while it runs, otherwise bad things happen. Normally this is taken care of by the i915_pmic_bus_access_notifier() which does an intel_uncore_forcewake_get(FORCEWAKE_ALL) when some other driver tries to access the PMIC bus, so that later forcewake gets are no-ops (for the duration of the bus access). But intel_uncore_forcewake_reset gets called in 3 cases: 1) Before registering the pmic_bus_access_notifier 2) After unregistering the pmic_bus_access_notifier 3) To reset forcewake state on a GPU reset In all 3 cases the i915_pmic_bus_access_notifier() protection is insufficient. This commit fixes this race by calling iosf_mbi_punit_acquire() before calling intel_uncore_forcewake_reset(). In the case where it is called directly after unregistering the pmic_bus_access_notifier, we need to hold the punit-lock over both calls to avoid a race where intel_uncore_fw_release_timer() may execute between the 2 calls. To allow holding the lock over both calls we need an unlocked variant of iosf_mbi_unregister_pmic_bus_access_notifier. Since intel_uncore.c is the only user of this function, we simply modify it in this commit. Doing this in a separate commit would require first adding an unlocked variant, then this commit and then removing the unused normal variant. Signed-off-by: Hans de Goede <hdegoede@redhat.com> Reviewed-by: Imre Deak <imre.deak@intel.com> --- Changes in v2: -Rebase on current (July 6th 2017) drm-next Changes in v3: -Keep punit acquired / locked over the unregister + forcewake_reset call combo to avoid a race hitting between the 2 calls -This requires modifying iosf_mbi_unregister_pmic_bus_access_notifier to not take the lock itself, since we are the only users this is done in this same commit Changes in v4: -Fix missing rename in doc-comment -Add and use iosf_mbi_assert_punit_acquired() helper -Add missing acquiqre surrounding intel_uncore_forcewake_reset() inside intel_uncore_check_forcewake_domains() -Add Imre's Reviewed-by --- arch/x86/include/asm/iosf_mbi.h | 20 +++++++++++++++++--- arch/x86/platform/intel/iosf_mbi.c | 20 +++++++++++--------- drivers/gpu/drm/i915/intel_uncore.c | 17 +++++++++++++---- drivers/gpu/drm/i915/selftests/intel_uncore.c | 3 +++ 4 files changed, 44 insertions(+), 16 deletions(-) diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h index c313cac36f56..0f0de4303180 100644 --- a/arch/x86/include/asm/iosf_mbi.h +++ b/arch/x86/include/asm/iosf_mbi.h @@ -139,11 +139,17 @@ void iosf_mbi_punit_release(void); int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb); /** - * iosf_mbi_register_pmic_bus_access_notifier - Unregister PMIC bus notifier + * iosf_mbi_register_pmic_bus_access_notifier_unlocked - Unregister PMIC bus + * notifier + * + * Note the caller must call iosf_mbi_punit_acquire() before calling this + * to ensure the bus is inactive before unregistering (and call + * iosf_mbi_punit_release() afterwards). * * @nb: notifier_block to unregister */ -int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb); +int iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( + struct notifier_block *nb); /** * iosf_mbi_call_pmic_bus_access_notifier_chain - Call PMIC bus notifier chain @@ -153,6 +159,11 @@ int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb); */ int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v); +/** + * iosf_mbi_assert_punit_acquired - Assert that the P-Unit has been acquired. + */ +void iosf_mbi_assert_punit_acquired(void); + #else /* CONFIG_IOSF_MBI is not enabled */ static inline bool iosf_mbi_available(void) @@ -191,7 +202,8 @@ int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb) } static inline -int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb) +int iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( + struct notifier_block *nb) { return 0; } @@ -202,6 +214,8 @@ int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v) return 0; } +static inline void iosf_mbi_assert_punit_acquired(void) {} + #endif /* CONFIG_IOSF_MBI */ #endif /* IOSF_MBI_SYMS_H */ diff --git a/arch/x86/platform/intel/iosf_mbi.c b/arch/x86/platform/intel/iosf_mbi.c index a952ac199741..a5361fd11e6e 100644 --- a/arch/x86/platform/intel/iosf_mbi.c +++ b/arch/x86/platform/intel/iosf_mbi.c @@ -218,19 +218,15 @@ int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb) } EXPORT_SYMBOL(iosf_mbi_register_pmic_bus_access_notifier); -int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb) +int iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( + struct notifier_block *nb) { - int ret; + WARN_ON(!mutex_is_locked(&iosf_mbi_punit_mutex)); - /* Wait for the bus to go inactive before unregistering */ - mutex_lock(&iosf_mbi_punit_mutex); - ret = blocking_notifier_chain_unregister( + return blocking_notifier_chain_unregister( &iosf_mbi_pmic_bus_access_notifier, nb); - mutex_unlock(&iosf_mbi_punit_mutex); - - return ret; } -EXPORT_SYMBOL(iosf_mbi_unregister_pmic_bus_access_notifier); +EXPORT_SYMBOL(iosf_mbi_unregister_pmic_bus_access_notifier_unlocked); int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v) { @@ -239,6 +235,12 @@ int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v) } EXPORT_SYMBOL(iosf_mbi_call_pmic_bus_access_notifier_chain); +void iosf_mbi_assert_punit_acquired(void) +{ + WARN_ON(!mutex_is_locked(&iosf_mbi_punit_mutex)); +} +EXPORT_SYMBOL(iosf_mbi_assert_punit_acquired); + #ifdef CONFIG_IOSF_MBI_DEBUG static u32 dbg_mdr; static u32 dbg_mcr; diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index e9ed02518406..80e75c029e59 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -229,6 +229,7 @@ intel_uncore_fw_release_timer(struct hrtimer *timer) return HRTIMER_NORESTART; } +/* Note callers must have acquired the PUNIT->PMIC bus, before calling this. */ static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv, bool restore) { @@ -237,6 +238,8 @@ static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv, int retry_count = 100; enum forcewake_domains fw, active_domains; + iosf_mbi_assert_punit_acquired(); + /* Hold uncore.lock across reset to prevent any register access * with forcewake not set correctly. Wait until all pending * timers are run before holding. @@ -416,14 +419,18 @@ static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv, GT_FIFO_CTL_RC6_POLICY_STALL); } + iosf_mbi_punit_acquire(); intel_uncore_forcewake_reset(dev_priv, restore_forcewake); + iosf_mbi_punit_release(); } void intel_uncore_suspend(struct drm_i915_private *dev_priv) { - iosf_mbi_unregister_pmic_bus_access_notifier( + iosf_mbi_punit_acquire(); + iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( &dev_priv->uncore.pmic_bus_access_nb); intel_uncore_forcewake_reset(dev_priv, false); + iosf_mbi_punit_release(); } void intel_uncore_resume_early(struct drm_i915_private *dev_priv) @@ -1246,12 +1253,14 @@ void intel_uncore_init(struct drm_i915_private *dev_priv) void intel_uncore_fini(struct drm_i915_private *dev_priv) { - iosf_mbi_unregister_pmic_bus_access_notifier( - &dev_priv->uncore.pmic_bus_access_nb); - /* Paranoia: make sure we have disabled everything before we exit. */ intel_uncore_sanitize(dev_priv); + + iosf_mbi_punit_acquire(); + iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( + &dev_priv->uncore.pmic_bus_access_nb); intel_uncore_forcewake_reset(dev_priv, false); + iosf_mbi_punit_release(); } #define GEN_RANGE(l, h) GENMASK((h) - 1, (l) - 1) diff --git a/drivers/gpu/drm/i915/selftests/intel_uncore.c b/drivers/gpu/drm/i915/selftests/intel_uncore.c index 2d0fef2cfca6..55d0ef4fbcef 100644 --- a/drivers/gpu/drm/i915/selftests/intel_uncore.c +++ b/drivers/gpu/drm/i915/selftests/intel_uncore.c @@ -148,7 +148,10 @@ static int intel_uncore_check_forcewake_domains(struct drm_i915_private *dev_pri for_each_set_bit(offset, valid, FW_RANGE) { i915_reg_t reg = { offset }; + iosf_mbi_punit_acquire(); intel_uncore_forcewake_reset(dev_priv, false); + iosf_mbi_punit_release(); + check_for_unclaimed_mmio(dev_priv); (void)I915_READ(reg); -- 2.13.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v4 4/4] drm/i915: Acquire PUNIT->PMIC bus for intel_uncore_forcewake_reset() 2017-08-20 12:59 ` [PATCH v4 4/4] drm/i915: Acquire PUNIT->PMIC bus for intel_uncore_forcewake_reset() Hans de Goede @ 2017-08-22 9:00 ` Imre Deak 2017-08-22 10:05 ` Hans de Goede 2017-09-04 13:23 ` Jani Nikula 1 sibling, 1 reply; 9+ messages in thread From: Imre Deak @ 2017-08-22 9:00 UTC (permalink / raw) To: Hans de Goede; +Cc: intel-gfx, dri-devel, Daniel Vetter On Sun, Aug 20, 2017 at 02:59:20PM +0200, Hans de Goede wrote: > intel_uncore_forcewake_reset() does forcewake puts and gets as such > we need to make sure that no-one tries to access the PUNIT->PMIC bus > (on systems where this bus is shared) while it runs, otherwise bad > things happen. > > Normally this is taken care of by the i915_pmic_bus_access_notifier() > which does an intel_uncore_forcewake_get(FORCEWAKE_ALL) when some other > driver tries to access the PMIC bus, so that later forcewake gets are > no-ops (for the duration of the bus access). > > But intel_uncore_forcewake_reset gets called in 3 cases: > 1) Before registering the pmic_bus_access_notifier > 2) After unregistering the pmic_bus_access_notifier > 3) To reset forcewake state on a GPU reset > > In all 3 cases the i915_pmic_bus_access_notifier() protection is > insufficient. > > This commit fixes this race by calling iosf_mbi_punit_acquire() before > calling intel_uncore_forcewake_reset(). In the case where it is called > directly after unregistering the pmic_bus_access_notifier, we need to > hold the punit-lock over both calls to avoid a race where > intel_uncore_fw_release_timer() may execute between the 2 calls. > > To allow holding the lock over both calls we need an unlocked > variant of iosf_mbi_unregister_pmic_bus_access_notifier. Since > intel_uncore.c is the only user of this function, we simply > modify it in this commit. Doing this in a separate commit would > require first adding an unlocked variant, then this commit and > then removing the unused normal variant. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > Reviewed-by: Imre Deak <imre.deak@intel.com> > --- > Changes in v2: > -Rebase on current (July 6th 2017) drm-next > > Changes in v3: > -Keep punit acquired / locked over the unregister + forcewake_reset > call combo to avoid a race hitting between the 2 calls > -This requires modifying iosf_mbi_unregister_pmic_bus_access_notifier > to not take the lock itself, since we are the only users this is done > in this same commit > > Changes in v4: > -Fix missing rename in doc-comment > -Add and use iosf_mbi_assert_punit_acquired() helper > -Add missing acquiqre surrounding intel_uncore_forcewake_reset() inside > intel_uncore_check_forcewake_domains() > -Add Imre's Reviewed-by > --- > arch/x86/include/asm/iosf_mbi.h | 20 +++++++++++++++++--- > arch/x86/platform/intel/iosf_mbi.c | 20 +++++++++++--------- > drivers/gpu/drm/i915/intel_uncore.c | 17 +++++++++++++---- > drivers/gpu/drm/i915/selftests/intel_uncore.c | 3 +++ > 4 files changed, 44 insertions(+), 16 deletions(-) > > diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h > index c313cac36f56..0f0de4303180 100644 > --- a/arch/x86/include/asm/iosf_mbi.h > +++ b/arch/x86/include/asm/iosf_mbi.h > @@ -139,11 +139,17 @@ void iosf_mbi_punit_release(void); > int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb); > > /** > - * iosf_mbi_register_pmic_bus_access_notifier - Unregister PMIC bus notifier > + * iosf_mbi_register_pmic_bus_access_notifier_unlocked - Unregister PMIC bus > + * notifier > + * > + * Note the caller must call iosf_mbi_punit_acquire() before calling this > + * to ensure the bus is inactive before unregistering (and call > + * iosf_mbi_punit_release() afterwards). > * > * @nb: notifier_block to unregister > */ > -int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb); > +int iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( > + struct notifier_block *nb); > > /** > * iosf_mbi_call_pmic_bus_access_notifier_chain - Call PMIC bus notifier chain > @@ -153,6 +159,11 @@ int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb); > */ > int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v); > > +/** > + * iosf_mbi_assert_punit_acquired - Assert that the P-Unit has been acquired. > + */ > +void iosf_mbi_assert_punit_acquired(void); > + > #else /* CONFIG_IOSF_MBI is not enabled */ > static inline > bool iosf_mbi_available(void) > @@ -191,7 +202,8 @@ int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb) > } > > static inline > -int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb) > +int iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( > + struct notifier_block *nb) > { > return 0; > } > @@ -202,6 +214,8 @@ int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v) > return 0; > } > > +static inline void iosf_mbi_assert_punit_acquired(void) {} > + > #endif /* CONFIG_IOSF_MBI */ > > #endif /* IOSF_MBI_SYMS_H */ > diff --git a/arch/x86/platform/intel/iosf_mbi.c b/arch/x86/platform/intel/iosf_mbi.c > index a952ac199741..a5361fd11e6e 100644 > --- a/arch/x86/platform/intel/iosf_mbi.c > +++ b/arch/x86/platform/intel/iosf_mbi.c > @@ -218,19 +218,15 @@ int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb) > } > EXPORT_SYMBOL(iosf_mbi_register_pmic_bus_access_notifier); > > -int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb) > +int iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( > + struct notifier_block *nb) > { > - int ret; > + WARN_ON(!mutex_is_locked(&iosf_mbi_punit_mutex)); Missed to change this to iosf_mbi_assert_punit_acquired(). The rest looks ok to me. I can change the above while applying, no need to resend for this. Adding also Chris. > > - /* Wait for the bus to go inactive before unregistering */ > - mutex_lock(&iosf_mbi_punit_mutex); > - ret = blocking_notifier_chain_unregister( > + return blocking_notifier_chain_unregister( > &iosf_mbi_pmic_bus_access_notifier, nb); > - mutex_unlock(&iosf_mbi_punit_mutex); > - > - return ret; > } > -EXPORT_SYMBOL(iosf_mbi_unregister_pmic_bus_access_notifier); > +EXPORT_SYMBOL(iosf_mbi_unregister_pmic_bus_access_notifier_unlocked); > > int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v) > { > @@ -239,6 +235,12 @@ int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v) > } > EXPORT_SYMBOL(iosf_mbi_call_pmic_bus_access_notifier_chain); > > +void iosf_mbi_assert_punit_acquired(void) > +{ > + WARN_ON(!mutex_is_locked(&iosf_mbi_punit_mutex)); > +} > +EXPORT_SYMBOL(iosf_mbi_assert_punit_acquired); > + > #ifdef CONFIG_IOSF_MBI_DEBUG > static u32 dbg_mdr; > static u32 dbg_mcr; > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index e9ed02518406..80e75c029e59 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -229,6 +229,7 @@ intel_uncore_fw_release_timer(struct hrtimer *timer) > return HRTIMER_NORESTART; > } > > +/* Note callers must have acquired the PUNIT->PMIC bus, before calling this. */ > static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv, > bool restore) > { > @@ -237,6 +238,8 @@ static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv, > int retry_count = 100; > enum forcewake_domains fw, active_domains; > > + iosf_mbi_assert_punit_acquired(); > + > /* Hold uncore.lock across reset to prevent any register access > * with forcewake not set correctly. Wait until all pending > * timers are run before holding. > @@ -416,14 +419,18 @@ static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv, > GT_FIFO_CTL_RC6_POLICY_STALL); > } > > + iosf_mbi_punit_acquire(); > intel_uncore_forcewake_reset(dev_priv, restore_forcewake); > + iosf_mbi_punit_release(); > } > > void intel_uncore_suspend(struct drm_i915_private *dev_priv) > { > - iosf_mbi_unregister_pmic_bus_access_notifier( > + iosf_mbi_punit_acquire(); > + iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( > &dev_priv->uncore.pmic_bus_access_nb); > intel_uncore_forcewake_reset(dev_priv, false); > + iosf_mbi_punit_release(); > } > > void intel_uncore_resume_early(struct drm_i915_private *dev_priv) > @@ -1246,12 +1253,14 @@ void intel_uncore_init(struct drm_i915_private *dev_priv) > > void intel_uncore_fini(struct drm_i915_private *dev_priv) > { > - iosf_mbi_unregister_pmic_bus_access_notifier( > - &dev_priv->uncore.pmic_bus_access_nb); > - > /* Paranoia: make sure we have disabled everything before we exit. */ > intel_uncore_sanitize(dev_priv); > + > + iosf_mbi_punit_acquire(); > + iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( > + &dev_priv->uncore.pmic_bus_access_nb); > intel_uncore_forcewake_reset(dev_priv, false); > + iosf_mbi_punit_release(); > } > > #define GEN_RANGE(l, h) GENMASK((h) - 1, (l) - 1) > diff --git a/drivers/gpu/drm/i915/selftests/intel_uncore.c b/drivers/gpu/drm/i915/selftests/intel_uncore.c > index 2d0fef2cfca6..55d0ef4fbcef 100644 > --- a/drivers/gpu/drm/i915/selftests/intel_uncore.c > +++ b/drivers/gpu/drm/i915/selftests/intel_uncore.c > @@ -148,7 +148,10 @@ static int intel_uncore_check_forcewake_domains(struct drm_i915_private *dev_pri > for_each_set_bit(offset, valid, FW_RANGE) { > i915_reg_t reg = { offset }; > > + iosf_mbi_punit_acquire(); > intel_uncore_forcewake_reset(dev_priv, false); > + iosf_mbi_punit_release(); > + > check_for_unclaimed_mmio(dev_priv); > > (void)I915_READ(reg); > -- > 2.13.4 > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 4/4] drm/i915: Acquire PUNIT->PMIC bus for intel_uncore_forcewake_reset() 2017-08-22 9:00 ` Imre Deak @ 2017-08-22 10:05 ` Hans de Goede 0 siblings, 0 replies; 9+ messages in thread From: Hans de Goede @ 2017-08-22 10:05 UTC (permalink / raw) To: imre.deak; +Cc: intel-gfx, dri-devel, Daniel Vetter Hi, On 22-08-17 11:00, Imre Deak wrote: > On Sun, Aug 20, 2017 at 02:59:20PM +0200, Hans de Goede wrote: <snip> >> diff --git a/arch/x86/platform/intel/iosf_mbi.c b/arch/x86/platform/intel/iosf_mbi.c >> index a952ac199741..a5361fd11e6e 100644 >> --- a/arch/x86/platform/intel/iosf_mbi.c >> +++ b/arch/x86/platform/intel/iosf_mbi.c >> @@ -218,19 +218,15 @@ int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb) >> } >> EXPORT_SYMBOL(iosf_mbi_register_pmic_bus_access_notifier); >> >> -int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb) >> +int iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( >> + struct notifier_block *nb) >> { >> - int ret; >> + WARN_ON(!mutex_is_locked(&iosf_mbi_punit_mutex)); > > Missed to change this to iosf_mbi_assert_punit_acquired(). The rest > looks ok to me. I can change the above while applying, no need to > resend for this. Cool, thank you. Regards, Hans _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 4/4] drm/i915: Acquire PUNIT->PMIC bus for intel_uncore_forcewake_reset() 2017-08-20 12:59 ` [PATCH v4 4/4] drm/i915: Acquire PUNIT->PMIC bus for intel_uncore_forcewake_reset() Hans de Goede 2017-08-22 9:00 ` Imre Deak @ 2017-09-04 13:23 ` Jani Nikula 1 sibling, 0 replies; 9+ messages in thread From: Jani Nikula @ 2017-09-04 13:23 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86 Cc: dri-devel, Hans de Goede, Daniel Vetter, intel-gfx Thomas, Ingo, Peter - Can I have your ack on merging the below patch via drm/i915? Unfortunately it wasn't originally sent to the proper mailing list. You can see the full series it's part of at [1]. BR, Jani. [1] https://patchwork.freedesktop.org/series/29058/ On Sun, 20 Aug 2017, Hans de Goede <hdegoede@redhat.com> wrote: > intel_uncore_forcewake_reset() does forcewake puts and gets as such > we need to make sure that no-one tries to access the PUNIT->PMIC bus > (on systems where this bus is shared) while it runs, otherwise bad > things happen. > > Normally this is taken care of by the i915_pmic_bus_access_notifier() > which does an intel_uncore_forcewake_get(FORCEWAKE_ALL) when some other > driver tries to access the PMIC bus, so that later forcewake gets are > no-ops (for the duration of the bus access). > > But intel_uncore_forcewake_reset gets called in 3 cases: > 1) Before registering the pmic_bus_access_notifier > 2) After unregistering the pmic_bus_access_notifier > 3) To reset forcewake state on a GPU reset > > In all 3 cases the i915_pmic_bus_access_notifier() protection is > insufficient. > > This commit fixes this race by calling iosf_mbi_punit_acquire() before > calling intel_uncore_forcewake_reset(). In the case where it is called > directly after unregistering the pmic_bus_access_notifier, we need to > hold the punit-lock over both calls to avoid a race where > intel_uncore_fw_release_timer() may execute between the 2 calls. > > To allow holding the lock over both calls we need an unlocked > variant of iosf_mbi_unregister_pmic_bus_access_notifier. Since > intel_uncore.c is the only user of this function, we simply > modify it in this commit. Doing this in a separate commit would > require first adding an unlocked variant, then this commit and > then removing the unused normal variant. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > Reviewed-by: Imre Deak <imre.deak@intel.com> > --- > Changes in v2: > -Rebase on current (July 6th 2017) drm-next > > Changes in v3: > -Keep punit acquired / locked over the unregister + forcewake_reset > call combo to avoid a race hitting between the 2 calls > -This requires modifying iosf_mbi_unregister_pmic_bus_access_notifier > to not take the lock itself, since we are the only users this is done > in this same commit > > Changes in v4: > -Fix missing rename in doc-comment > -Add and use iosf_mbi_assert_punit_acquired() helper > -Add missing acquiqre surrounding intel_uncore_forcewake_reset() inside > intel_uncore_check_forcewake_domains() > -Add Imre's Reviewed-by > --- > arch/x86/include/asm/iosf_mbi.h | 20 +++++++++++++++++--- > arch/x86/platform/intel/iosf_mbi.c | 20 +++++++++++--------- > drivers/gpu/drm/i915/intel_uncore.c | 17 +++++++++++++---- > drivers/gpu/drm/i915/selftests/intel_uncore.c | 3 +++ > 4 files changed, 44 insertions(+), 16 deletions(-) > > diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h > index c313cac36f56..0f0de4303180 100644 > --- a/arch/x86/include/asm/iosf_mbi.h > +++ b/arch/x86/include/asm/iosf_mbi.h > @@ -139,11 +139,17 @@ void iosf_mbi_punit_release(void); > int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb); > > /** > - * iosf_mbi_register_pmic_bus_access_notifier - Unregister PMIC bus notifier > + * iosf_mbi_register_pmic_bus_access_notifier_unlocked - Unregister PMIC bus > + * notifier > + * > + * Note the caller must call iosf_mbi_punit_acquire() before calling this > + * to ensure the bus is inactive before unregistering (and call > + * iosf_mbi_punit_release() afterwards). > * > * @nb: notifier_block to unregister > */ > -int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb); > +int iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( > + struct notifier_block *nb); > > /** > * iosf_mbi_call_pmic_bus_access_notifier_chain - Call PMIC bus notifier chain > @@ -153,6 +159,11 @@ int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb); > */ > int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v); > > +/** > + * iosf_mbi_assert_punit_acquired - Assert that the P-Unit has been acquired. > + */ > +void iosf_mbi_assert_punit_acquired(void); > + > #else /* CONFIG_IOSF_MBI is not enabled */ > static inline > bool iosf_mbi_available(void) > @@ -191,7 +202,8 @@ int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb) > } > > static inline > -int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb) > +int iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( > + struct notifier_block *nb) > { > return 0; > } > @@ -202,6 +214,8 @@ int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v) > return 0; > } > > +static inline void iosf_mbi_assert_punit_acquired(void) {} > + > #endif /* CONFIG_IOSF_MBI */ > > #endif /* IOSF_MBI_SYMS_H */ > diff --git a/arch/x86/platform/intel/iosf_mbi.c b/arch/x86/platform/intel/iosf_mbi.c > index a952ac199741..a5361fd11e6e 100644 > --- a/arch/x86/platform/intel/iosf_mbi.c > +++ b/arch/x86/platform/intel/iosf_mbi.c > @@ -218,19 +218,15 @@ int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb) > } > EXPORT_SYMBOL(iosf_mbi_register_pmic_bus_access_notifier); > > -int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb) > +int iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( > + struct notifier_block *nb) > { > - int ret; > + WARN_ON(!mutex_is_locked(&iosf_mbi_punit_mutex)); > > - /* Wait for the bus to go inactive before unregistering */ > - mutex_lock(&iosf_mbi_punit_mutex); > - ret = blocking_notifier_chain_unregister( > + return blocking_notifier_chain_unregister( > &iosf_mbi_pmic_bus_access_notifier, nb); > - mutex_unlock(&iosf_mbi_punit_mutex); > - > - return ret; > } > -EXPORT_SYMBOL(iosf_mbi_unregister_pmic_bus_access_notifier); > +EXPORT_SYMBOL(iosf_mbi_unregister_pmic_bus_access_notifier_unlocked); > > int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v) > { > @@ -239,6 +235,12 @@ int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v) > } > EXPORT_SYMBOL(iosf_mbi_call_pmic_bus_access_notifier_chain); > > +void iosf_mbi_assert_punit_acquired(void) > +{ > + WARN_ON(!mutex_is_locked(&iosf_mbi_punit_mutex)); > +} > +EXPORT_SYMBOL(iosf_mbi_assert_punit_acquired); > + > #ifdef CONFIG_IOSF_MBI_DEBUG > static u32 dbg_mdr; > static u32 dbg_mcr; > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index e9ed02518406..80e75c029e59 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -229,6 +229,7 @@ intel_uncore_fw_release_timer(struct hrtimer *timer) > return HRTIMER_NORESTART; > } > > +/* Note callers must have acquired the PUNIT->PMIC bus, before calling this. */ > static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv, > bool restore) > { > @@ -237,6 +238,8 @@ static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv, > int retry_count = 100; > enum forcewake_domains fw, active_domains; > > + iosf_mbi_assert_punit_acquired(); > + > /* Hold uncore.lock across reset to prevent any register access > * with forcewake not set correctly. Wait until all pending > * timers are run before holding. > @@ -416,14 +419,18 @@ static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv, > GT_FIFO_CTL_RC6_POLICY_STALL); > } > > + iosf_mbi_punit_acquire(); > intel_uncore_forcewake_reset(dev_priv, restore_forcewake); > + iosf_mbi_punit_release(); > } > > void intel_uncore_suspend(struct drm_i915_private *dev_priv) > { > - iosf_mbi_unregister_pmic_bus_access_notifier( > + iosf_mbi_punit_acquire(); > + iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( > &dev_priv->uncore.pmic_bus_access_nb); > intel_uncore_forcewake_reset(dev_priv, false); > + iosf_mbi_punit_release(); > } > > void intel_uncore_resume_early(struct drm_i915_private *dev_priv) > @@ -1246,12 +1253,14 @@ void intel_uncore_init(struct drm_i915_private *dev_priv) > > void intel_uncore_fini(struct drm_i915_private *dev_priv) > { > - iosf_mbi_unregister_pmic_bus_access_notifier( > - &dev_priv->uncore.pmic_bus_access_nb); > - > /* Paranoia: make sure we have disabled everything before we exit. */ > intel_uncore_sanitize(dev_priv); > + > + iosf_mbi_punit_acquire(); > + iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( > + &dev_priv->uncore.pmic_bus_access_nb); > intel_uncore_forcewake_reset(dev_priv, false); > + iosf_mbi_punit_release(); > } > > #define GEN_RANGE(l, h) GENMASK((h) - 1, (l) - 1) > diff --git a/drivers/gpu/drm/i915/selftests/intel_uncore.c b/drivers/gpu/drm/i915/selftests/intel_uncore.c > index 2d0fef2cfca6..55d0ef4fbcef 100644 > --- a/drivers/gpu/drm/i915/selftests/intel_uncore.c > +++ b/drivers/gpu/drm/i915/selftests/intel_uncore.c > @@ -148,7 +148,10 @@ static int intel_uncore_check_forcewake_domains(struct drm_i915_private *dev_pri > for_each_set_bit(offset, valid, FW_RANGE) { > i915_reg_t reg = { offset }; > > + iosf_mbi_punit_acquire(); > intel_uncore_forcewake_reset(dev_priv, false); > + iosf_mbi_punit_release(); > + > check_for_unclaimed_mmio(dev_priv); > > (void)I915_READ(reg); -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [v4,1/4] drm/i915: Fix false-positive assert_rpm_wakelock_held in i915_pmic_bus_access_notifier 2017-08-20 12:59 [PATCH v4 1/4] drm/i915: Fix false-positive assert_rpm_wakelock_held in i915_pmic_bus_access_notifier Hans de Goede ` (2 preceding siblings ...) 2017-08-20 12:59 ` [PATCH v4 4/4] drm/i915: Acquire PUNIT->PMIC bus for intel_uncore_forcewake_reset() Hans de Goede @ 2017-08-20 13:36 ` Patchwork 3 siblings, 0 replies; 9+ messages in thread From: Patchwork @ 2017-08-20 13:36 UTC (permalink / raw) To: Hans de Goede; +Cc: intel-gfx == Series Details == Series: series starting with [v4,1/4] drm/i915: Fix false-positive assert_rpm_wakelock_held in i915_pmic_bus_access_notifier URL : https://patchwork.freedesktop.org/series/29058/ State : success == Summary == Series 29058v1 Series without cover letter https://patchwork.freedesktop.org/api/1.0/series/29058/revisions/1/mbox/ Test drv_module_reload: Subgroup basic-no-display: dmesg-warn -> PASS (fi-kbl-7260u) Subgroup basic-reload-inject: dmesg-warn -> PASS (fi-kbl-7260u) fdo#102295 fdo#102295 https://bugs.freedesktop.org/show_bug.cgi?id=102295 fi-bdw-5557u total:279 pass:268 dwarn:0 dfail:0 fail:0 skip:11 time:454s fi-bdw-gvtdvm total:279 pass:265 dwarn:0 dfail:0 fail:0 skip:14 time:438s fi-blb-e6850 total:279 pass:224 dwarn:1 dfail:0 fail:0 skip:54 time:368s fi-bsw-n3050 total:279 pass:243 dwarn:0 dfail:0 fail:0 skip:36 time:552s fi-bxt-j4205 total:279 pass:260 dwarn:0 dfail:0 fail:0 skip:19 time:520s fi-byt-j1900 total:279 pass:254 dwarn:1 dfail:0 fail:0 skip:24 time:529s fi-byt-n2820 total:279 pass:251 dwarn:0 dfail:0 fail:0 skip:28 time:516s fi-glk-2a total:279 pass:260 dwarn:0 dfail:0 fail:0 skip:19 time:616s fi-hsw-4770 total:279 pass:263 dwarn:0 dfail:0 fail:0 skip:16 time:444s fi-hsw-4770r total:279 pass:263 dwarn:0 dfail:0 fail:0 skip:16 time:421s fi-ilk-650 total:279 pass:229 dwarn:0 dfail:0 fail:0 skip:50 time:419s fi-ivb-3520m total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:503s fi-ivb-3770 total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:486s fi-kbl-7260u total:279 pass:268 dwarn:1 dfail:0 fail:0 skip:10 time:497s fi-kbl-7500u total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:480s fi-kbl-7560u total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:598s fi-kbl-r total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:599s fi-pnv-d510 total:279 pass:223 dwarn:1 dfail:0 fail:0 skip:55 time:523s fi-skl-6260u total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:473s fi-skl-6700k total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:479s fi-skl-6770hq total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:494s fi-skl-gvtdvm total:279 pass:266 dwarn:0 dfail:0 fail:0 skip:13 time:445s fi-skl-x1585l total:279 pass:268 dwarn:0 dfail:0 fail:0 skip:11 time:480s fi-snb-2520m total:279 pass:251 dwarn:0 dfail:0 fail:0 skip:28 time:546s fi-snb-2600 total:279 pass:250 dwarn:0 dfail:0 fail:0 skip:29 time:401s 5b151fd0dafb8130ce4b809e3795146a5ef6bc2e drm-tip: 2017y-08m-18d-22h-38m-58s UTC integration manifest 08f249b7832b drm/i915: Acquire PUNIT->PMIC bus for intel_uncore_forcewake_reset() 93cc69173cba drm/i915: Call uncore_suspend before platform suspend handlers 2b98192604f2 drm/i915: Re-register PMIC bus access notifier on runtime resume 6f2112b0af5e drm/i915: Fix false-positive assert_rpm_wakelock_held in i915_pmic_bus_access_notifier == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5450/ _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v4 1/4] drm/i915: Fix false-positive assert_rpm_wakelock_held in i915_pmic_bus_access_notifier @ 2017-08-20 12:51 Hans de Goede 2017-08-20 12:51 ` [PATCH v4 2/4] drm/i915: Re-register PMIC bus access notifier on runtime resume Hans de Goede 0 siblings, 1 reply; 9+ messages in thread From: Hans de Goede @ 2017-08-20 12:51 UTC (permalink / raw) To: Daniel Vetter, Jani Nikula, Ville Syrjälä, Imre Deak Cc: Hans de Goede, intel-gfx, dri-devel assert_rpm_wakelock_held is triggered from i915_pmic_bus_access_notifier even though it gets unregistered on (runtime) suspend, this is caused by a race happening under the following circumstances: intel_runtime_pm_put does: atomic_dec(&dev_priv->pm.wakeref_count); pm_runtime_mark_last_busy(kdev); pm_runtime_put_autosuspend(kdev); And pm_runtime_put_autosuspend calls intel_runtime_suspend from a workqueue, so there is ample of time between the atomic_dec() and intel_runtime_suspend() unregistering the notifier. If the notifier gets called in this windowd assert_rpm_wakelock_held falsely triggers (at this point we're not runtime-suspended yet). This commit adds disable_rpm_wakeref_asserts and enable_rpm_wakeref_asserts calls around the intel_uncore_forcewake_get(FORCEWAKE_ALL) call in i915_pmic_bus_access_notifier fixing the false-positive WARN_ON. Reported-by: FKr <bugs-freedesktop@ubermail.me> Signed-off-by: Hans de Goede <hdegoede@redhat.com> Reviewed-by: Imre Deak <imre.deak@intel.com> --- Changes in v2: -Rebase on current (July 6th 2017) drm-next Changes in v3: -Reword comment explaining why disabling the wakeref asserts is ok and necessary -Add Imre's Reviewed-by --- drivers/gpu/drm/i915/intel_uncore.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 1d7b879cc68c..2d3aad319229 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -1171,8 +1171,15 @@ static int i915_pmic_bus_access_notifier(struct notifier_block *nb, * bus, which will be busy after this notification, leading to: * "render: timed out waiting for forcewake ack request." * errors. + * + * The notifier is unregistered during intel_runtime_suspend(), + * so it's ok to access the HW here without holding a RPM + * wake reference -> disable wakeref asserts for the time of + * the access. */ + disable_rpm_wakeref_asserts(dev_priv); intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); + enable_rpm_wakeref_asserts(dev_priv); break; case MBI_PMIC_BUS_ACCESS_END: intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL); -- 2.13.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v4 2/4] drm/i915: Re-register PMIC bus access notifier on runtime resume 2017-08-20 12:51 [PATCH v4 1/4] " Hans de Goede @ 2017-08-20 12:51 ` Hans de Goede 0 siblings, 0 replies; 9+ messages in thread From: Hans de Goede @ 2017-08-20 12:51 UTC (permalink / raw) To: Daniel Vetter, Jani Nikula, Ville Syrjälä, Imre Deak Cc: Hans de Goede, intel-gfx, dri-devel intel_uncore_suspend() unregisters the uncore code's PMIC bus access notifier and gets called on both normal and runtime suspend. intel_uncore_resume_early() re-registers the notifier, but only on normal resume. Add a new intel_uncore_runtime_resume() function which only re-registers the notifier and call that on runtime resume. Reported-by: Imre Deak <imre.deak@intel.com> Signed-off-by: Hans de Goede <hdegoede@redhat.com> Reviewed-by: Imre Deak <imre.deak@intel.com> --- Changes in v2: -Rebase on current (July 6th 2017) drm-next Changes in v3: -Add Imre's Reviewed-by --- drivers/gpu/drm/i915/i915_drv.c | 2 ++ drivers/gpu/drm/i915/intel_uncore.c | 6 ++++++ drivers/gpu/drm/i915/intel_uncore.h | 1 + 3 files changed, 9 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 43100229613c..4715f320c8fa 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -2585,6 +2585,8 @@ static int intel_runtime_resume(struct device *kdev) ret = vlv_resume_prepare(dev_priv, true); } + intel_uncore_runtime_resume(dev_priv); + /* * No point of rolling back things in case of an error, as the best * we can do is to hope that things will still work (and disable RPM). diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 2d3aad319229..e9ed02518406 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -434,6 +434,12 @@ void intel_uncore_resume_early(struct drm_i915_private *dev_priv) i915_check_and_clear_faults(dev_priv); } +void intel_uncore_runtime_resume(struct drm_i915_private *dev_priv) +{ + iosf_mbi_register_pmic_bus_access_notifier( + &dev_priv->uncore.pmic_bus_access_nb); +} + void intel_uncore_sanitize(struct drm_i915_private *dev_priv) { i915.enable_rc6 = sanitize_rc6_option(dev_priv, i915.enable_rc6); diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h index 5f90278da461..0bdc3fcc0e64 100644 --- a/drivers/gpu/drm/i915/intel_uncore.h +++ b/drivers/gpu/drm/i915/intel_uncore.h @@ -121,6 +121,7 @@ bool intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv void intel_uncore_fini(struct drm_i915_private *dev_priv); void intel_uncore_suspend(struct drm_i915_private *dev_priv); void intel_uncore_resume_early(struct drm_i915_private *dev_priv); +void intel_uncore_runtime_resume(struct drm_i915_private *dev_priv); u64 intel_uncore_edram_size(struct drm_i915_private *dev_priv); void assert_forcewakes_inactive(struct drm_i915_private *dev_priv); -- 2.13.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-09-04 13:23 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-08-20 12:59 [PATCH v4 1/4] drm/i915: Fix false-positive assert_rpm_wakelock_held in i915_pmic_bus_access_notifier Hans de Goede 2017-08-20 12:59 ` [PATCH v4 2/4] drm/i915: Re-register PMIC bus access notifier on runtime resume Hans de Goede 2017-08-20 12:59 ` [PATCH v4 3/4] drm/i915: Call uncore_suspend before platform suspend handlers Hans de Goede 2017-08-20 12:59 ` [PATCH v4 4/4] drm/i915: Acquire PUNIT->PMIC bus for intel_uncore_forcewake_reset() Hans de Goede 2017-08-22 9:00 ` Imre Deak 2017-08-22 10:05 ` Hans de Goede 2017-09-04 13:23 ` Jani Nikula 2017-08-20 13:36 ` ✓ Fi.CI.BAT: success for series starting with [v4,1/4] drm/i915: Fix false-positive assert_rpm_wakelock_held in i915_pmic_bus_access_notifier Patchwork -- strict thread matches above, loose matches on Subject: below -- 2017-08-20 12:51 [PATCH v4 1/4] " Hans de Goede 2017-08-20 12:51 ` [PATCH v4 2/4] drm/i915: Re-register PMIC bus access notifier on runtime resume Hans de Goede
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.