[PATCH v2 1/4] drm/i915: Fix false-positive assert_rpm_wakelock_held in i915_pmic_bus_access_notifier

[PATCH v2 1/4] drm/i915: Fix false-positive assert_rpm_wakelock_held in i915_pmic_bus_access_notifier

[Hans de Goede]

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>
---
Changes in v2:
-Rebase on current (July 6th 2017) drm-next
---
 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 9882724bc2b6..168b28a87f76 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.
+		 *
+		 * This notifier may get called between intel_runtime_pm_put()
+		 * doing atomic_dec(wakeref_count) and intel_runtime_resume()
+		 * unregistering this notifier, which leads to false-positive
+		 * assert_rpm_wakelock_held() triggering.
 		 */
+		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.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 2/4] drm/i915: Re-register PMIC bus access notifier on runtime resume

[Hans de Goede]

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>
---
Changes in v2:
-Rebase on current (July 6th 2017) drm-next
---
 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 ee2325b180e7..ce31d9ed23dc 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2510,6 +2510,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 168b28a87f76..4a547cdfafa9 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.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 3/4] drm/i915: Call uncore_suspend before platform suspend handlers

[Hans de Goede]

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>
---
Changes in v2:
-Rebase on current (July 6th 2017) drm-next
---
 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 ce31d9ed23dc..4a6cd3176e0a 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2415,6 +2415,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);
@@ -2427,6 +2429,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);
@@ -2434,8 +2438,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.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 4/4] drm/i915: Acquire PUNIT->PMIC bus for intel_uncore_forcewake_reset()

[Hans de Goede]

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 the pmic bus access race this causes
by making intel_uncore_forcewake_reset() call iosf_mbi_punit_acquire()
(and iosf_mbi_punit_release() when done).

Note that iosf_mbi_punit_acquire() locks a mutex and thus
intel_uncore_forcewake_reset() may sleep after this commit. I've checked
all callers and they all already take other mutexes, so this is not a
problem.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Rebase on current (July 6th 2017) drm-next
---
 drivers/gpu/drm/i915/intel_uncore.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 4a547cdfafa9..f9441c9ae226 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -237,6 +237,9 @@ static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
 	int retry_count = 100;
 	enum forcewake_domains fw, active_domains;
 
+	/* Acquire the PUNIT->PMIC bus before modifying forcewake settings */
+	iosf_mbi_punit_acquire();
+
 	/* Hold uncore.lock across reset to prevent any register access
 	 * with forcewake not set correctly. Wait until all pending
 	 * timers are run before holding.
@@ -294,6 +297,7 @@ static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
 		assert_forcewakes_inactive(dev_priv);
 
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
+	iosf_mbi_punit_release();
 }
 
 static u64 gen9_edram_size(struct drm_i915_private *dev_priv)
-- 
2.13.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/4] drm/i915: Fix false-positive assert_rpm_wakelock_held in i915_pmic_bus_access_notifier

[Imre Deak]

Hi,

On Thu, Jul 06, 2017 at 09:24:47PM +0200, Hans de Goede wrote:
> 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>
> ---
> Changes in v2:
> -Rebase on current (July 6th 2017) drm-next
> ---
>  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 9882724bc2b6..168b28a87f76 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.
> +		 *
> +		 * This notifier may get called between intel_runtime_pm_put()
> +		 * doing atomic_dec(wakeref_count) and intel_runtime_resume()
> +		 * unregistering this notifier, which leads to false-positive
> +		 * assert_rpm_wakelock_held() triggering.

the following would describe better the reason for disabling wakeref asserts.
That is we access the HW without holding a runtime PM reference, but it's ok
here since it's handled as a special case during runtime suspend:

		* The notifier is unregistered during intel_runtime_suspend(),
		* so it's ok to access the HW here without holding an RPM
		* wake reference -> disable wakeref asserts for the time of
		* the access.

With that this looks ok:
Reviewed-by: Imre Deak <imre.deak@intel.com>


>  		 */
> +		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.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/4] drm/i915: Re-register PMIC bus access notifier on runtime resume

[Imre Deak]

On Thu, Jul 06, 2017 at 09:24:48PM +0200, Hans de Goede wrote:
> 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
> ---
>  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 ee2325b180e7..ce31d9ed23dc 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -2510,6 +2510,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 168b28a87f76..4a547cdfafa9 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.0
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/4] drm/i915: Call uncore_suspend before platform suspend handlers

[Imre Deak]

On Thu, Jul 06, 2017 at 09:24:49PM +0200, Hans de Goede wrote:
> 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>

> ---
> Changes in v2:
> -Rebase on current (July 6th 2017) drm-next
> ---
>  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 ce31d9ed23dc..4a6cd3176e0a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -2415,6 +2415,8 @@ static int intel_runtime_suspend(struct device *kdev)
>  
>  	intel_runtime_pm_disable_interrupts(dev_priv);
>  
> +	intel_uncore_suspend(dev_priv);
> +

Yep, this fixes a problem on VLV independent of your IOSF/forcewake
fix. vlv_suspend_complete() calls vlv_allow_gt_wake(false) after which
we shouldn't have any pending forcewakes until resume.

AFAICS after this point there isn't anything requiring forcewake until
resume so the change looks ok:

Reviewed-by: Imre Deak <imre.deak@intel.com>

>  	ret = 0;
>  	if (IS_GEN9_LP(dev_priv)) {
>  		bxt_display_core_uninit(dev_priv);
> @@ -2427,6 +2429,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);
> @@ -2434,8 +2438,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.0
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/4] drm/i915: Acquire PUNIT->PMIC bus for intel_uncore_forcewake_reset()

[Imre Deak]

On Thu, Jul 06, 2017 at 09:24:50PM +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 the pmic bus access race this causes
> by making intel_uncore_forcewake_reset() call iosf_mbi_punit_acquire()
> (and iosf_mbi_punit_release() when done).
> 
> Note that iosf_mbi_punit_acquire() locks a mutex and thus
> intel_uncore_forcewake_reset() may sleep after this commit. I've checked
> all callers and they all already take other mutexes, so this is not a
> problem.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Rebase on current (July 6th 2017) drm-next
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 4a547cdfafa9..f9441c9ae226 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -237,6 +237,9 @@ static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
>  	int retry_count = 100;
>  	enum forcewake_domains fw, active_domains;
>  
> +	/* Acquire the PUNIT->PMIC bus before modifying forcewake settings */
> +	iosf_mbi_punit_acquire();
> +
>  	/* Hold uncore.lock across reset to prevent any register access
>  	 * with forcewake not set correctly. Wait until all pending
>  	 * timers are run before holding.
> @@ -294,6 +297,7 @@ static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
>  		assert_forcewakes_inactive(dev_priv);
>  
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> +	iosf_mbi_punit_release();
>  }

Looks ok in general, but during intel_uncore_suspend() and
intel_uncore_fini() this still allows the following race:

1. (task 1) i915 MMIO access requiring forcewake, arms
   intel_uncore_fw_release_timer().
2. (task 1) intel_uncore_suspend()->iosf_mbi_unregister_pmic_bus_access_notifier()
3. (task 2) DW I2C access start
4. (hrtimer irq) intel_uncore_fw_release_timer() does forcewake disable
5. (task 2) DW I2C access end

One way to avoid this would be holding iosf_mbi_punit_mutex() around the
whole

iosf_mbi_unregister_pmic_bus_access_notifier()
intel_uncore_forcewake_reset()

sequence.

--Imre

>  
>  static u64 gen9_edram_size(struct drm_i915_private *dev_priv)
> -- 
> 2.13.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/4] drm/i915: Fix false-positive assert_rpm_wakelock_held in i915_pmic_bus_access_notifier

[Hans de Goede]

Hi,

On 27-07-17 16:35, Imre Deak wrote:
> Hi,
> 
> On Thu, Jul 06, 2017 at 09:24:47PM +0200, Hans de Goede wrote:
>> 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>
>> ---
>> Changes in v2:
>> -Rebase on current (July 6th 2017) drm-next
>> ---
>>   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 9882724bc2b6..168b28a87f76 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.
>> +		 *
>> +		 * This notifier may get called between intel_runtime_pm_put()
>> +		 * doing atomic_dec(wakeref_count) and intel_runtime_resume()
>> +		 * unregistering this notifier, which leads to false-positive
>> +		 * assert_rpm_wakelock_held() triggering.
> 
> the following would describe better the reason for disabling wakeref asserts.
> That is we access the HW without holding a runtime PM reference, but it's ok
> here since it's handled as a special case during runtime suspend:
> 
> 		* The notifier is unregistered during intel_runtime_suspend(),
> 		* so it's ok to access the HW here without holding an RPM
> 		* wake reference -> disable wakeref asserts for the time of
> 		* the access.
> 
> With that this looks ok:
> Reviewed-by: Imre Deak <imre.deak@intel.com>

Thank you for the review, unfortunately I've been a bit swamped with other
stuff. But I'm catching up now.

I've made the suggest change to the comment and added your
Reviewed-by for the upcoming v3 of this patch.

Regards,

Hans


> 
> 
>>   		 */
>> +		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.0
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel