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

[PATCH v3 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>
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 9882724bc2b6..91a9c316ef6f 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

[PATCH v3 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>
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 fc307e03943c..cf9e7341877a 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2512,6 +2512,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 91a9c316ef6f..569d115918eb 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

[PATCH v3 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>
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 cf9e7341877a..6791fb4144d5 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2417,6 +2417,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);
@@ -2429,6 +2431,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);
@@ -2436,8 +2440,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

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

[PATCH v3 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 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>
---
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
---
 arch/x86/include/asm/iosf_mbi.h     | 10 ++++++++--
 arch/x86/platform/intel/iosf_mbi.c  | 14 +++++---------
 drivers/gpu/drm/i915/intel_uncore.c | 17 +++++++++++++----
 3 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h
index c313cac36f56..f8841bb06d98 100644
--- a/arch/x86/include/asm/iosf_mbi.h
+++ b/arch/x86/include/asm/iosf_mbi.h
@@ -141,9 +141,14 @@ int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb);
 /**
  * iosf_mbi_register_pmic_bus_access_notifier - 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
@@ -191,7 +196,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;
 }
diff --git a/arch/x86/platform/intel/iosf_mbi.c b/arch/x86/platform/intel/iosf_mbi.c
index a952ac199741..5596a3ec1b89 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)
 {
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 569d115918eb..7be6150520ed 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)
 {
@@ -416,14 +417,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 +1251,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)
@@ -1524,7 +1531,9 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
 
 	ret = gen6_hw_domain_reset(dev_priv, hw_mask);
 
+	iosf_mbi_punit_acquire();
 	intel_uncore_forcewake_reset(dev_priv, true);
+	iosf_mbi_punit_release();
 
 	return ret;
 }
-- 
2.13.4

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

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

[Imre Deak]

On Mon, Aug 14, 2017 at 09:58:32PM +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>
> ---
> 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
> ---
>  arch/x86/include/asm/iosf_mbi.h     | 10 ++++++++--
>  arch/x86/platform/intel/iosf_mbi.c  | 14 +++++---------
>  drivers/gpu/drm/i915/intel_uncore.c | 17 +++++++++++++----
>  3 files changed, 26 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h
> index c313cac36f56..f8841bb06d98 100644
> --- a/arch/x86/include/asm/iosf_mbi.h
> +++ b/arch/x86/include/asm/iosf_mbi.h
> @@ -141,9 +141,14 @@ int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb);
>  /**
>   * iosf_mbi_register_pmic_bus_access_notifier - Unregister PMIC bus notifier

You missed the rename in the doc.

>   *
> + * 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
> @@ -191,7 +196,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;
>  }
> diff --git a/arch/x86/platform/intel/iosf_mbi.c b/arch/x86/platform/intel/iosf_mbi.c
> index a952ac199741..5596a3ec1b89 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)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 569d115918eb..7be6150520ed 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. */

Nit: could use an iosf_assert_mbi_punit_mutex_held() helper instead of
the comment. (taking CONFIG_IOSF_MBI into account)

>  static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
>  					 bool restore)
>  {
> @@ -416,14 +417,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 +1251,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)
> @@ -1524,7 +1531,9 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
>  
>  	ret = gen6_hw_domain_reset(dev_priv, hw_mask);
>  
> +	iosf_mbi_punit_acquire();
>  	intel_uncore_forcewake_reset(dev_priv, true);
> +	iosf_mbi_punit_release();
>  
>  	return ret;
>  }

There is one more spot in intel_uncore_check_forcewake_domains() calling
intel_uncore_forcewake_reset() you missed.

With the above fixes and optional change for the nit looks ok:
Reviewed-by: Imre Deak <imre.deak@intel.com>

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

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

[Hans de Goede]

Hi,

On 17-08-17 14:22, Imre Deak wrote:
> On Mon, Aug 14, 2017 at 09:58:32PM +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>
>> ---
>> 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
>> ---
>>   arch/x86/include/asm/iosf_mbi.h     | 10 ++++++++--
>>   arch/x86/platform/intel/iosf_mbi.c  | 14 +++++---------
>>   drivers/gpu/drm/i915/intel_uncore.c | 17 +++++++++++++----
>>   3 files changed, 26 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h
>> index c313cac36f56..f8841bb06d98 100644
>> --- a/arch/x86/include/asm/iosf_mbi.h
>> +++ b/arch/x86/include/asm/iosf_mbi.h
>> @@ -141,9 +141,14 @@ int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb);
>>   /**
>>    * iosf_mbi_register_pmic_bus_access_notifier - Unregister PMIC bus notifier
> 
> You missed the rename in the doc.

Thx, fixed for v4.

>>    *
>> + * 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
>> @@ -191,7 +196,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;
>>   }
>> diff --git a/arch/x86/platform/intel/iosf_mbi.c b/arch/x86/platform/intel/iosf_mbi.c
>> index a952ac199741..5596a3ec1b89 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)
>>   {
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index 569d115918eb..7be6150520ed 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. */
> 
> Nit: could use an iosf_assert_mbi_punit_mutex_held() helper instead of
> the comment. (taking CONFIG_IOSF_MBI into account)

Good idea, done for v4 (as iosf_mbi_assert_punit_acquired).

>>   static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
>>   					 bool restore)
>>   {
>> @@ -416,14 +417,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 +1251,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)
>> @@ -1524,7 +1531,9 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
>>   
>>   	ret = gen6_hw_domain_reset(dev_priv, hw_mask);
>>   
>> +	iosf_mbi_punit_acquire();
>>   	intel_uncore_forcewake_reset(dev_priv, true);
>> +	iosf_mbi_punit_release();
>>   
>>   	return ret;
>>   }
> 
> There is one more spot in intel_uncore_check_forcewake_domains() calling
> intel_uncore_forcewake_reset() you missed.

Ugh, I did not check for intel_uncore_forcewake_reset() usage outside
of intel_uncore.c as it is a static function, the include magic used
for the selftests caught me by suprise there.

> With the above fixes and optional change for the nit looks ok:
> Reviewed-by: Imre Deak <imre.deak@intel.com>

Thanks, v4 with everything fixed is coming up.

Regards,

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