[PATCH v5 1/2] x86 / i915 iosf_mbi / PMIC bus access fixes

[PATCH v5 1/2] x86 / i915 iosf_mbi / PMIC bus access fixes

[Hans de Goede]

Hi All,

Here is a split-up version of my "drm/i915: Acquire PUNIT->PMIC bus for
intel_uncore_forcewake_reset()" patch, this time with the
arch/x86/platform/intel/iosf_mbi.c split out into a separate patch.

Since the iosf_mbi changes are fairly isolated, ideally this entire
series would go upstream through the drm tree. X86 maintainers, can
we have an ack for that from one of you please ?

Regards,

Hans

[PATCH v5 1/2] x86 / i915 iosf_mbi / PMIC bus access fixes

[Hans de Goede]

Hi All,

Here is a split-up version of my "drm/i915: Acquire PUNIT->PMIC bus for
intel_uncore_forcewake_reset()" patch, this time with the
arch/x86/platform/intel/iosf_mbi.c split out into a separate patch.

Since the iosf_mbi changes are fairly isolated, ideally this entire
series would go upstream through the drm tree. X86 maintainers, can
we have an ack for that from one of you please ?

Regards,

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

[PATCH v5 1/2] x86/platform/intel/iosf_mbi: Add unlocked PMIC bus access notifier unregister

[Hans de Goede]

For race free unregistration drivers may need to acquire PMIC bus access
through iosf_mbi_punit_acquire() and then (un)register the notifier without
dropping the lock.

This commit adds an unlocked variant of
iosf_mbi_unregister_pmic_bus_access_notifier for this use case.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 arch/x86/include/asm/iosf_mbi.h    | 25 +++++++++++++++++++++++++
 arch/x86/platform/intel/iosf_mbi.c | 19 +++++++++++++++++--
 2 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h
index c313cac36f56..eb959cd77994 100644
--- a/arch/x86/include/asm/iosf_mbi.h
+++ b/arch/x86/include/asm/iosf_mbi.h
@@ -145,6 +145,18 @@ int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb);
  */
 int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb);
 
+/**
+ * iosf_mbi_unregister_pmic_bus_access_notifier_unlocked - Unregister PMIC bus
+ *                                                         notifier, unlocked
+ *
+ * Like iosf_mbi_unregister_pmic_bus_access_notifier(), but for use when the
+ * caller has already called iosf_mbi_punit_acquire() itself.
+ *
+ * @nb: notifier_block to unregister
+ */
+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 +165,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)
@@ -196,12 +213,20 @@ int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb)
 	return 0;
 }
 
+static inline int
+iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(struct notifier_block *nb)
+{
+	return 0;
+}
+
 static inline
 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..6f37a2137a79 100644
--- a/arch/x86/platform/intel/iosf_mbi.c
+++ b/arch/x86/platform/intel/iosf_mbi.c
@@ -218,14 +218,23 @@ 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_unlocked(
+	struct notifier_block *nb)
+{
+	iosf_mbi_assert_punit_acquired();
+
+	return blocking_notifier_chain_unregister(
+				&iosf_mbi_pmic_bus_access_notifier, nb);
+}
+EXPORT_SYMBOL(iosf_mbi_unregister_pmic_bus_access_notifier_unlocked);
+
 int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb)
 {
 	int ret;
 
 	/* Wait for the bus to go inactive before unregistering */
 	mutex_lock(&iosf_mbi_punit_mutex);
-	ret = blocking_notifier_chain_unregister(
-				&iosf_mbi_pmic_bus_access_notifier, nb);
+	ret = iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(nb);
 	mutex_unlock(&iosf_mbi_punit_mutex);
 
 	return ret;
@@ -239,6 +248,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;
-- 
2.14.2

[PATCH v5 1/2] x86/platform/intel/iosf_mbi: Add unlocked PMIC bus access notifier unregister

[Hans de Goede]

For race free unregistration drivers may need to acquire PMIC bus access
through iosf_mbi_punit_acquire() and then (un)register the notifier without
dropping the lock.

This commit adds an unlocked variant of
iosf_mbi_unregister_pmic_bus_access_notifier for this use case.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 arch/x86/include/asm/iosf_mbi.h    | 25 +++++++++++++++++++++++++
 arch/x86/platform/intel/iosf_mbi.c | 19 +++++++++++++++++--
 2 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h
index c313cac36f56..eb959cd77994 100644
--- a/arch/x86/include/asm/iosf_mbi.h
+++ b/arch/x86/include/asm/iosf_mbi.h
@@ -145,6 +145,18 @@ int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb);
  */
 int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb);
 
+/**
+ * iosf_mbi_unregister_pmic_bus_access_notifier_unlocked - Unregister PMIC bus
+ *                                                         notifier, unlocked
+ *
+ * Like iosf_mbi_unregister_pmic_bus_access_notifier(), but for use when the
+ * caller has already called iosf_mbi_punit_acquire() itself.
+ *
+ * @nb: notifier_block to unregister
+ */
+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 +165,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)
@@ -196,12 +213,20 @@ int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb)
 	return 0;
 }
 
+static inline int
+iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(struct notifier_block *nb)
+{
+	return 0;
+}
+
 static inline
 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..6f37a2137a79 100644
--- a/arch/x86/platform/intel/iosf_mbi.c
+++ b/arch/x86/platform/intel/iosf_mbi.c
@@ -218,14 +218,23 @@ 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_unlocked(
+	struct notifier_block *nb)
+{
+	iosf_mbi_assert_punit_acquired();
+
+	return blocking_notifier_chain_unregister(
+				&iosf_mbi_pmic_bus_access_notifier, nb);
+}
+EXPORT_SYMBOL(iosf_mbi_unregister_pmic_bus_access_notifier_unlocked);
+
 int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb)
 {
 	int ret;
 
 	/* Wait for the bus to go inactive before unregistering */
 	mutex_lock(&iosf_mbi_punit_mutex);
-	ret = blocking_notifier_chain_unregister(
-				&iosf_mbi_pmic_bus_access_notifier, nb);
+	ret = iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(nb);
 	mutex_unlock(&iosf_mbi_punit_mutex);
 
 	return ret;
@@ -239,6 +248,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;
-- 
2.14.2

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

[PATCH v5 2/2] 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.

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 acquire surrounding intel_uncore_forcewake_reset() inside
 intel_uncore_check_forcewake_domains()
-Add Imre's Reviewed-by

Changes in v5:
-Separate out arch/x86 iosf_mbi changes into a separate patch
---
 drivers/gpu/drm/i915/intel_uncore.c           | 17 +++++++++++++----
 drivers/gpu/drm/i915/selftests/intel_uncore.c |  3 +++
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 8c2ce81f01c2..0da81faf3981 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)
@@ -1315,12 +1322,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();
 }
 
 static const struct reg_whitelist {
diff --git a/drivers/gpu/drm/i915/selftests/intel_uncore.c b/drivers/gpu/drm/i915/selftests/intel_uncore.c
index 3cac22eb47ce..733d87fe7737 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.14.2

[PATCH v5 2/2] 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.

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 acquire surrounding intel_uncore_forcewake_reset() inside
 intel_uncore_check_forcewake_domains()
-Add Imre's Reviewed-by

Changes in v5:
-Separate out arch/x86 iosf_mbi changes into a separate patch
---
 drivers/gpu/drm/i915/intel_uncore.c           | 17 +++++++++++++----
 drivers/gpu/drm/i915/selftests/intel_uncore.c |  3 +++
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 8c2ce81f01c2..0da81faf3981 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)
@@ -1315,12 +1322,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();
 }
 
 static const struct reg_whitelist {
diff --git a/drivers/gpu/drm/i915/selftests/intel_uncore.c b/drivers/gpu/drm/i915/selftests/intel_uncore.c
index 3cac22eb47ce..733d87fe7737 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.14.2

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

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

[Ingo Molnar]

* Hans de Goede <j.w.r.degoede@gmail.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.
> 
> 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 acquire surrounding intel_uncore_forcewake_reset() inside
>  intel_uncore_check_forcewake_domains()
> -Add Imre's Reviewed-by
> 
> Changes in v5:
> -Separate out arch/x86 iosf_mbi changes into a separate patch
> ---
>  drivers/gpu/drm/i915/intel_uncore.c           | 17 +++++++++++++----
>  drivers/gpu/drm/i915/selftests/intel_uncore.c |  3 +++
>  2 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 8c2ce81f01c2..0da81faf3981 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)
> @@ -1315,12 +1322,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();
>  }
>  
>  static const struct reg_whitelist {
> diff --git a/drivers/gpu/drm/i915/selftests/intel_uncore.c b/drivers/gpu/drm/i915/selftests/intel_uncore.c
> index 3cac22eb47ce..733d87fe7737 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);

This patch looks like one massive layering violation. Why does the GPU code muck 
with the uncore hardware?

Thanks,

	Ingo

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

[Ingo Molnar]

* Hans de Goede <j.w.r.degoede@gmail.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.
> 
> 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 acquire surrounding intel_uncore_forcewake_reset() inside
>  intel_uncore_check_forcewake_domains()
> -Add Imre's Reviewed-by
> 
> Changes in v5:
> -Separate out arch/x86 iosf_mbi changes into a separate patch
> ---
>  drivers/gpu/drm/i915/intel_uncore.c           | 17 +++++++++++++----
>  drivers/gpu/drm/i915/selftests/intel_uncore.c |  3 +++
>  2 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 8c2ce81f01c2..0da81faf3981 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)
> @@ -1315,12 +1322,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();
>  }
>  
>  static const struct reg_whitelist {
> diff --git a/drivers/gpu/drm/i915/selftests/intel_uncore.c b/drivers/gpu/drm/i915/selftests/intel_uncore.c
> index 3cac22eb47ce..733d87fe7737 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);

This patch looks like one massive layering violation. Why does the GPU code muck 
with the uncore hardware?

Thanks,

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

Re: [Intel-gfx] [PATCH v5 2/2] drm/i915: Acquire PUNIT->PMIC bus for intel_uncore_forcewake_reset()

[Daniel Vetter]

On Tue, Oct 31, 2017 at 10:50:06AM +0100, Ingo Molnar wrote:
> 
> * Hans de Goede <j.w.r.degoede@gmail.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.
> > 
> > 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 acquire surrounding intel_uncore_forcewake_reset() inside
> >  intel_uncore_check_forcewake_domains()
> > -Add Imre's Reviewed-by
> > 
> > Changes in v5:
> > -Separate out arch/x86 iosf_mbi changes into a separate patch
> > ---
> >  drivers/gpu/drm/i915/intel_uncore.c           | 17 +++++++++++++----
> >  drivers/gpu/drm/i915/selftests/intel_uncore.c |  3 +++
> >  2 files changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index 8c2ce81f01c2..0da81faf3981 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)
> > @@ -1315,12 +1322,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();
> >  }
> >  
> >  static const struct reg_whitelist {
> > diff --git a/drivers/gpu/drm/i915/selftests/intel_uncore.c b/drivers/gpu/drm/i915/selftests/intel_uncore.c
> > index 3cac22eb47ce..733d87fe7737 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);
> 
> This patch looks like one massive layering violation. Why does the GPU code muck 
> with the uncore hardware?

Because the hw is an even worse layering violation. There's way too much
"magic" stuff going on in the background, which then sometimes doesn't
work and we end up implementing hacks in drivers to paper over it.

Slightly more details: The gpu can also get access to the pmic, through
its own register window, except the synchronization at the hw/fw level is
screwed up and doesn't work, breaking the illusion that the gpu is a
prefectly isolated pci device. In reality, at the SoC level, it's anything
but. But because those deps aren't clearly expressed (people hoped it
would work with the magic, which was all added to make running Windows on
top of this possible), so it all looks like horrible hacks instead of the
much cleaner design arm-soc platforms have with DT describing all these
deps much more explicitly.

Aside: There's a lot more mmio windows of other devices and special
backdoors to other stuff in the gpu "pci" mmio bar than just this. Mostly
they work as designed.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH v5 2/2] drm/i915: Acquire PUNIT->PMIC bus for intel_uncore_forcewake_reset()

[Daniel Vetter]

On Tue, Oct 31, 2017 at 10:50:06AM +0100, Ingo Molnar wrote:
> 
> * Hans de Goede <j.w.r.degoede@gmail.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.
> > 
> > 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 acquire surrounding intel_uncore_forcewake_reset() inside
> >  intel_uncore_check_forcewake_domains()
> > -Add Imre's Reviewed-by
> > 
> > Changes in v5:
> > -Separate out arch/x86 iosf_mbi changes into a separate patch
> > ---
> >  drivers/gpu/drm/i915/intel_uncore.c           | 17 +++++++++++++----
> >  drivers/gpu/drm/i915/selftests/intel_uncore.c |  3 +++
> >  2 files changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index 8c2ce81f01c2..0da81faf3981 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)
> > @@ -1315,12 +1322,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();
> >  }
> >  
> >  static const struct reg_whitelist {
> > diff --git a/drivers/gpu/drm/i915/selftests/intel_uncore.c b/drivers/gpu/drm/i915/selftests/intel_uncore.c
> > index 3cac22eb47ce..733d87fe7737 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);
> 
> This patch looks like one massive layering violation. Why does the GPU code muck 
> with the uncore hardware?

Because the hw is an even worse layering violation. There's way too much
"magic" stuff going on in the background, which then sometimes doesn't
work and we end up implementing hacks in drivers to paper over it.

Slightly more details: The gpu can also get access to the pmic, through
its own register window, except the synchronization at the hw/fw level is
screwed up and doesn't work, breaking the illusion that the gpu is a
prefectly isolated pci device. In reality, at the SoC level, it's anything
but. But because those deps aren't clearly expressed (people hoped it
would work with the magic, which was all added to make running Windows on
top of this possible), so it all looks like horrible hacks instead of the
much cleaner design arm-soc platforms have with DT describing all these
deps much more explicitly.

Aside: There's a lot more mmio windows of other devices and special
backdoors to other stuff in the gpu "pci" mmio bar than just this. Mostly
they work as designed.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v5 2/2] drm/i915: Acquire PUNIT->PMIC bus for intel_uncore_forcewake_reset()

[Ingo Molnar]

* Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Oct 31, 2017 at 10:50:06AM +0100, Ingo Molnar wrote:
> > 
> > * Hans de Goede <j.w.r.degoede@gmail.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.
> > > 
> > > 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 acquire surrounding intel_uncore_forcewake_reset() inside
> > >  intel_uncore_check_forcewake_domains()
> > > -Add Imre's Reviewed-by
> > > 
> > > Changes in v5:
> > > -Separate out arch/x86 iosf_mbi changes into a separate patch
> > > ---
> > >  drivers/gpu/drm/i915/intel_uncore.c           | 17 +++++++++++++----
> > >  drivers/gpu/drm/i915/selftests/intel_uncore.c |  3 +++
> > >  2 files changed, 16 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > > index 8c2ce81f01c2..0da81faf3981 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)
> > > @@ -1315,12 +1322,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();
> > >  }
> > >  
> > >  static const struct reg_whitelist {
> > > diff --git a/drivers/gpu/drm/i915/selftests/intel_uncore.c b/drivers/gpu/drm/i915/selftests/intel_uncore.c
> > > index 3cac22eb47ce..733d87fe7737 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);
> > 
> > This patch looks like one massive layering violation. Why does the GPU code muck 
> > with the uncore hardware?
> 
> Because the hw is an even worse layering violation. There's way too much
> "magic" stuff going on in the background, which then sometimes doesn't
> work and we end up implementing hacks in drivers to paper over it.

Ok, fair enough I guess:

  Acked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

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

[Hans de Goede]

Hi,

On 31-10-17 10:50, Ingo Molnar wrote:
> 
> * Hans de Goede <j.w.r.degoede@gmail.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.
>>
>> 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 acquire surrounding intel_uncore_forcewake_reset() inside
>>   intel_uncore_check_forcewake_domains()
>> -Add Imre's Reviewed-by
>>
>> Changes in v5:
>> -Separate out arch/x86 iosf_mbi changes into a separate patch
>> ---
>>   drivers/gpu/drm/i915/intel_uncore.c           | 17 +++++++++++++----
>>   drivers/gpu/drm/i915/selftests/intel_uncore.c |  3 +++
>>   2 files changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index 8c2ce81f01c2..0da81faf3981 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)
>> @@ -1315,12 +1322,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();
>>   }
>>   
>>   static const struct reg_whitelist {
>> diff --git a/drivers/gpu/drm/i915/selftests/intel_uncore.c b/drivers/gpu/drm/i915/selftests/intel_uncore.c
>> index 3cac22eb47ce..733d87fe7737 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);
> 
> This patch looks like one massive layering violation. Why does the GPU code muck
> with the uncore hardware?

AFAIK uncore stands for not-core, so all the peripheral stuff
we have on a CPU (or rather really SoC) nowadays, the GPU is part
of those peripherals and again AFAIK the GPU driver needs to make
sure certain power-domains are awake when accessing various parts
of the GPU. But perhaps one of the i915 devs can answer this
question better.

Now as for the specifics of this patch-set, in most Intel systems
the CPU/SoC communicates to the PMIC to set various power-domain
voltages through a dedicated SVID bus.

But the Bay Trail CR (cost reduced) and Cherry Trail platforms
which use an AXP288 PMIC are special. The AXP288 PMIC does not
support the SVID bus. Instead the PUnit inside the SoC
communicates over i2c with the PMIC. This i2c bus is shared
with regular in kernel i2c drivers, which unfortunately
is necessary to implement various ACPI opregions.

To solve the shared i2c bus access the PUnit has a PMIC bus
access semaphore which gets accessed via the iosf_mbi bus.
Unfortunately having the i2c-host controller acquire this
semaphore alone is not enough protection. It seems this only
stops the PUnit from doing power-transitions requiring the
i2c bus on itself. If some code running on the CPU, like
the GPU driver wants to change certain power-domains
explicitly the kernel needs to make sure that no i2c
accesses to the PMIC are happening at the same time.

One mechanism used here is a notification from the i2c-host
controller driver to the GPU code that it is going to use
the i2c bus, which this patch set is about.

Note that the layering issues you talk about are already in
place and do not get changed in anyway by these 2 patches.

All these 2 patches do is change the code to unregister the
notifier so that a single lock can be held over both
unregistering the notifier and making powerdomain changes,
fixing a race.

Regards,

Hans

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

[Hans de Goede]

Hi,

On 31-10-17 10:50, Ingo Molnar wrote:
> 
> * Hans de Goede <j.w.r.degoede@gmail.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.
>>
>> 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 acquire surrounding intel_uncore_forcewake_reset() inside
>>   intel_uncore_check_forcewake_domains()
>> -Add Imre's Reviewed-by
>>
>> Changes in v5:
>> -Separate out arch/x86 iosf_mbi changes into a separate patch
>> ---
>>   drivers/gpu/drm/i915/intel_uncore.c           | 17 +++++++++++++----
>>   drivers/gpu/drm/i915/selftests/intel_uncore.c |  3 +++
>>   2 files changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index 8c2ce81f01c2..0da81faf3981 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)
>> @@ -1315,12 +1322,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();
>>   }
>>   
>>   static const struct reg_whitelist {
>> diff --git a/drivers/gpu/drm/i915/selftests/intel_uncore.c b/drivers/gpu/drm/i915/selftests/intel_uncore.c
>> index 3cac22eb47ce..733d87fe7737 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);
> 
> This patch looks like one massive layering violation. Why does the GPU code muck
> with the uncore hardware?

AFAIK uncore stands for not-core, so all the peripheral stuff
we have on a CPU (or rather really SoC) nowadays, the GPU is part
of those peripherals and again AFAIK the GPU driver needs to make
sure certain power-domains are awake when accessing various parts
of the GPU. But perhaps one of the i915 devs can answer this
question better.

Now as for the specifics of this patch-set, in most Intel systems
the CPU/SoC communicates to the PMIC to set various power-domain
voltages through a dedicated SVID bus.

But the Bay Trail CR (cost reduced) and Cherry Trail platforms
which use an AXP288 PMIC are special. The AXP288 PMIC does not
support the SVID bus. Instead the PUnit inside the SoC
communicates over i2c with the PMIC. This i2c bus is shared
with regular in kernel i2c drivers, which unfortunately
is necessary to implement various ACPI opregions.

To solve the shared i2c bus access the PUnit has a PMIC bus
access semaphore which gets accessed via the iosf_mbi bus.
Unfortunately having the i2c-host controller acquire this
semaphore alone is not enough protection. It seems this only
stops the PUnit from doing power-transitions requiring the
i2c bus on itself. If some code running on the CPU, like
the GPU driver wants to change certain power-domains
explicitly the kernel needs to make sure that no i2c
accesses to the PMIC are happening at the same time.

One mechanism used here is a notification from the i2c-host
controller driver to the GPU code that it is going to use
the i2c bus, which this patch set is about.

Note that the layering issues you talk about are already in
place and do not get changed in anyway by these 2 patches.

All these 2 patches do is change the code to unregister the
notifier so that a single lock can be held over both
unregistering the notifier and making powerdomain changes,
fixing a race.

Regards,

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

✗ Fi.CI.BAT: failure for series starting with [v5,1/2] x86/platform/intel/iosf_mbi: Add unlocked PMIC bus access notifier unregister

[Patchwork]

== Series Details ==

Series: series starting with [v5,1/2] x86/platform/intel/iosf_mbi: Add unlocked PMIC bus access notifier unregister
URL   : https://patchwork.freedesktop.org/series/32288/
State : failure

== Summary ==

Series 32288v1 series starting with [v5,1/2] x86/platform/intel/iosf_mbi: Add unlocked PMIC bus access notifier unregister
https://patchwork.freedesktop.org/api/1.0/series/32288/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                pass       -> INCOMPLETE (fi-skl-6700hq)

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:448s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:448s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:371s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:540s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:262s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:508s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:503s
fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:493s
fi-byt-n2820     total:289  pass:249  dwarn:1   dfail:0   fail:0   skip:39  time:478s
fi-cfl-s         total:289  pass:253  dwarn:4   dfail:0   fail:0   skip:32  time:557s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:416s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:251s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:582s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:444s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:429s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:429s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:507s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:457s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:486s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:575s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:479s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:579s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:543s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:449s
fi-skl-6700hq    total:245  pass:220  dwarn:0   dfail:0   fail:0   skip:24 
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:518s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:500s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:456s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:564s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:419s

93f001963c9915c52b5ad84500075d231e008ced drm-tip: 2017y-10m-19d-10h-52m-17s UTC integration manifest
2e0038ee0daa drm/i915: Acquire PUNIT->PMIC bus for intel_uncore_forcewake_reset()
ef37838a00fd x86/platform/intel/iosf_mbi: Add unlocked PMIC bus access notifier unregister

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6101/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: failure for series starting with [v5,1/2] x86/platform/intel/iosf_mbi: Add unlocked PMIC bus access notifier unregister

[Hans de Goede]

Hi,

On 19-10-17 13:50, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [v5,1/2] x86/platform/intel/iosf_mbi: Add unlocked PMIC bus access notifier unregister
> URL   : https://patchwork.freedesktop.org/series/32288/
> State : failure

This seems to be a random failure getting assigned to this series, this series
only affects systems which have a PUNIT mutex as detected by:
drivers/i2c/busses/i2c-designware-baytrail.c

Which I'm pretty sure this system does not have.

Regards,

Hans




> 
> == Summary ==
> 
> Series 32288v1 series starting with [v5,1/2] x86/platform/intel/iosf_mbi: Add unlocked PMIC bus access notifier unregister
> https://patchwork.freedesktop.org/api/1.0/series/32288/revisions/1/mbox/
> 
> Test kms_pipe_crc_basic:
>          Subgroup suspend-read-crc-pipe-a:
>                  pass       -> INCOMPLETE (fi-skl-6700hq)
> 
> fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:448s
> fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:448s
> fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:371s
> fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:540s
> fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:262s
> fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:508s
> fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:503s
> fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:493s
> fi-byt-n2820     total:289  pass:249  dwarn:1   dfail:0   fail:0   skip:39  time:478s
> fi-cfl-s         total:289  pass:253  dwarn:4   dfail:0   fail:0   skip:32  time:557s
> fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:416s
> fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:251s
> fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:582s
> fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:444s
> fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:429s
> fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:429s
> fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:507s
> fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:457s
> fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:486s
> fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:575s
> fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:479s
> fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:579s
> fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:543s
> fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:449s
> fi-skl-6700hq    total:245  pass:220  dwarn:0   dfail:0   fail:0   skip:24
> fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:518s
> fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:500s
> fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:456s
> fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:564s
> fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:419s
> 
> 93f001963c9915c52b5ad84500075d231e008ced drm-tip: 2017y-10m-19d-10h-52m-17s UTC integration manifest
> 2e0038ee0daa drm/i915: Acquire PUNIT->PMIC bus for intel_uncore_forcewake_reset()
> ef37838a00fd x86/platform/intel/iosf_mbi: Add unlocked PMIC bus access notifier unregister
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6101/
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: failure for series starting with [v5,1/2] x86/platform/intel/iosf_mbi: Add unlocked PMIC bus access notifier unregister

[Hans de Goede]

Hi,

On 19-10-17 15:04, Hans de Goede wrote:
> Hi,
> 
> On 19-10-17 13:50, Patchwork wrote:
>> == Series Details ==
>>
>> Series: series starting with [v5,1/2] x86/platform/intel/iosf_mbi: Add unlocked PMIC bus access notifier unregister
>> URL   : https://patchwork.freedesktop.org/series/32288/
>> State : failure
> 
> This seems to be a random failure getting assigned to this series, this series
> only affects systems which have a PUNIT mutex as detected by:
> drivers/i2c/busses/i2c-designware-baytrail.c
> 
> Which I'm pretty sure this system does not have.

Looking closer, this series does slightly modify the timing of
intel_uncore_resume_early(), as that now takes and releases
the mutex behind iosf_mbi_punit_acquire(); twice rather then
just once. I could extend the first patch in the series
to also add an unlocked variant of iosf_mbi_register_pmic_bus_access_notifier
and only take the lock once, from a race pov this is not
necessary, but it would save having to take the lock
twice.

I still think there is another root cause to the problem
the CI caught though, the problem is the
GEM_BUG_ON in drivers/gpu/drm/i915/intel_lrc.c at
line 782, which has a big comment on top on why
calling intel_runtime_pm_get() is not necessary there
maybe there is a path where it is actually necessary
and we are hitting a race here ?

Regards,

Hans









> Regards,
> 
> Hans
> 
> 
> 
> 
>>
>> == Summary ==
>>
>> Series 32288v1 series starting with [v5,1/2] x86/platform/intel/iosf_mbi: Add unlocked PMIC bus access notifier unregister
>> https://patchwork.freedesktop.org/api/1.0/series/32288/revisions/1/mbox/
>>
>> Test kms_pipe_crc_basic:
>>          Subgroup suspend-read-crc-pipe-a:
>>                  pass       -> INCOMPLETE (fi-skl-6700hq)
>>
>> fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:448s
>> fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:448s
>> fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:371s
>> fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:540s
>> fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:262s
>> fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:508s
>> fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:503s
>> fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:493s
>> fi-byt-n2820     total:289  pass:249  dwarn:1   dfail:0   fail:0   skip:39  time:478s
>> fi-cfl-s         total:289  pass:253  dwarn:4   dfail:0   fail:0   skip:32  time:557s
>> fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:416s
>> fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:251s
>> fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:582s
>> fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:444s
>> fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:429s
>> fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:429s
>> fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:507s
>> fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:457s
>> fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:486s
>> fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:575s
>> fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:479s
>> fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:579s
>> fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:543s
>> fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:449s
>> fi-skl-6700hq    total:245  pass:220  dwarn:0   dfail:0   fail:0   skip:24
>> fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:518s
>> fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:500s
>> fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:456s
>> fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:564s
>> fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:419s
>>
>> 93f001963c9915c52b5ad84500075d231e008ced drm-tip: 2017y-10m-19d-10h-52m-17s UTC integration manifest
>> 2e0038ee0daa drm/i915: Acquire PUNIT->PMIC bus for intel_uncore_forcewake_reset()
>> ef37838a00fd x86/platform/intel/iosf_mbi: Add unlocked PMIC bus access notifier unregister
>>
>> == Logs ==
>>
>> For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6101/
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 1/2] x86 / i915 iosf_mbi / PMIC bus access fixes

[Hans de Goede]

Thomas, Ingo,

Ping? Can we please get an ack for taking this upstream
through the drm subsys tree ?

Regards,

Hans


On 19-10-17 13:16, Hans de Goede wrote:
> Hi All,
> 
> Here is a split-up version of my "drm/i915: Acquire PUNIT->PMIC bus for
> intel_uncore_forcewake_reset()" patch, this time with the
> arch/x86/platform/intel/iosf_mbi.c split out into a separate patch.
> 
> Since the iosf_mbi changes are fairly isolated, ideally this entire
> series would go upstream through the drm tree. X86 maintainers, can
> we have an ack for that from one of you please ?
> 
> Regards,
> 
> Hans
> 

Re: [PATCH v5 1/2] x86 / i915 iosf_mbi / PMIC bus access fixes

[Hans de Goede]

Thomas, Ingo,

Ping? Can we please get an ack for taking this upstream
through the drm subsys tree ?

Regards,

Hans


On 19-10-17 13:16, Hans de Goede wrote:
> Hi All,
> 
> Here is a split-up version of my "drm/i915: Acquire PUNIT->PMIC bus for
> intel_uncore_forcewake_reset()" patch, this time with the
> arch/x86/platform/intel/iosf_mbi.c split out into a separate patch.
> 
> Since the iosf_mbi changes are fairly isolated, ideally this entire
> series would go upstream through the drm tree. X86 maintainers, can
> we have an ack for that from one of you please ?
> 
> Regards,
> 
> Hans
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [v5,1/2] x86/platform/intel/iosf_mbi: Add unlocked PMIC bus access notifier unregister

[Patchwork]

== Series Details ==

Series: series starting with [v5,1/2] x86/platform/intel/iosf_mbi: Add unlocked PMIC bus access notifier unregister
URL   : https://patchwork.freedesktop.org/series/32288/
State : success

== Summary ==

Series 32288v1 series starting with [v5,1/2] x86/platform/intel/iosf_mbi: Add unlocked PMIC bus access notifier unregister
https://patchwork.freedesktop.org/api/1.0/series/32288/revisions/1/mbox/

Test gem_exec_reloc:
        Subgroup basic-write-gtt-active:
                fail       -> PASS       (fi-gdg-551) fdo#102582
Test kms_busy:
        Subgroup basic-flip-b:
                fail       -> PASS       (fi-bwr-2160)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-a-frame-sequence:
                none       -> INCOMPLETE (fi-cfl-s)
Test drv_module_reload:
        Subgroup basic-no-display:
                fail       -> PASS       (fi-hsw-4770r) fdo#103534

fdo#102582 https://bugs.freedesktop.org/show_bug.cgi?id=102582
fdo#103534 https://bugs.freedesktop.org/show_bug.cgi?id=103534

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:446s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:452s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:381s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:535s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:277s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:504s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:504s
fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:509s
fi-byt-n2820     total:289  pass:249  dwarn:1   dfail:0   fail:0   skip:39  time:491s
fi-cfl-s         total:240  pass:211  dwarn:0   dfail:0   fail:0   skip:28 
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:430s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:264s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:584s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:431s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:433s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:423s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:488s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:458s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:497s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:576s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:478s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:587s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:572s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:443s
fi-skl-6600u     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:594s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:653s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:524s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:503s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:459s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:571s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:432s

de359919ae463cdaef6bc6890156df84e19dee2a drm-tip: 2017y-11m-03d-14h-00m-37s UTC integration manifest
9d0815239328 drm/i915: Acquire PUNIT->PMIC bus for intel_uncore_forcewake_reset()
94acab084274 x86/platform/intel/iosf_mbi: Add unlocked PMIC bus access notifier unregister

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6947/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [v5,1/2] x86/platform/intel/iosf_mbi: Add unlocked PMIC bus access notifier unregister

[Patchwork]

== Series Details ==

Series: series starting with [v5,1/2] x86/platform/intel/iosf_mbi: Add unlocked PMIC bus access notifier unregister
URL   : https://patchwork.freedesktop.org/series/32288/
State : success

== Summary ==

Test kms_setmode:
        Subgroup basic:
                fail       -> PASS       (shard-hsw) fdo#99912

fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912

shard-hsw        total:2539 pass:1434 dwarn:0   dfail:1   fail:7   skip:1097 time:9241s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6947/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [v5,1/2] x86/platform/intel/iosf_mbi: Add unlocked PMIC bus access notifier unregister

[Patchwork]

== Series Details ==

Series: series starting with [v5,1/2] x86/platform/intel/iosf_mbi: Add unlocked PMIC bus access notifier unregister
URL   : https://patchwork.freedesktop.org/series/32288/
State : success

== Summary ==

Series 32288v1 series starting with [v5,1/2] x86/platform/intel/iosf_mbi: Add unlocked PMIC bus access notifier unregister
https://patchwork.freedesktop.org/api/1.0/series/32288/revisions/1/mbox/

Test gem_exec_reloc:
        Subgroup basic-write-gtt-active:
                fail       -> PASS       (fi-gdg-551) fdo#102582
Test kms_busy:
        Subgroup basic-flip-b:
                fail       -> PASS       (fi-bwr-2160) fdo#103182
Test drv_module_reload:
        Subgroup basic-no-display:
                fail       -> PASS       (fi-hsw-4770r) fdo#103534

fdo#102582 https://bugs.freedesktop.org/show_bug.cgi?id=102582
fdo#103182 https://bugs.freedesktop.org/show_bug.cgi?id=103182
fdo#103534 https://bugs.freedesktop.org/show_bug.cgi?id=103534

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:446s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:452s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:381s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:535s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:277s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:504s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:504s
fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:509s
fi-byt-n2820     total:289  pass:249  dwarn:1   dfail:0   fail:0   skip:39  time:491s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:430s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:264s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:584s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:431s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:433s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:423s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:488s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:458s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:497s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:576s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:478s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:587s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:572s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:443s
fi-skl-6600u     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:594s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:653s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:524s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:503s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:459s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:571s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:432s

de359919ae463cdaef6bc6890156df84e19dee2a drm-tip: 2017y-11m-03d-14h-00m-37s UTC integration manifest
9d0815239328 drm/i915: Acquire PUNIT->PMIC bus for intel_uncore_forcewake_reset()
94acab084274 x86/platform/intel/iosf_mbi: Add unlocked PMIC bus access notifier unregister

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6947/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx