[PATCH 1/7] drm/i915: Consolidate unclaimed mmio detection

[PATCH 1/7] drm/i915: Consolidate unclaimed mmio detection

[Mika Kuoppala]

Access the unclaimed reg detection register through
one helper which also does cleanup. Note that we now access
the register only if the platform has the actual non claimed
access bit. This prevents reading the register with gens that
doesn't have the register or the unclaimed bit,
when debug_mmio > 0.

Note that we post after clearing the bit. This makes sure
that the next unclaimed write access would get detected
also if it happened right after clearing, and not fold
into the previous detection.

v2: s/unclaimed_reg_access/check_for_unclaimed_mmio (Chris)
    debug log on unclaimed detection on uncore init (Joonas)

Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Paulo Zanoni <przanoni@gmail.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 38 +++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index fcf04fe..95e7803 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -334,13 +334,34 @@ static void intel_uncore_ellc_detect(struct drm_device *dev)
 	}
 }
 
+static bool
+check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
+{
+	u32 dbg;
+
+	if (!HAS_FPGA_DBG_UNCLAIMED(dev_priv))
+		return false;
+
+	dbg = __raw_i915_read32(dev_priv, FPGA_DBG);
+	if (likely(!(dbg & FPGA_DBG_RM_NOCLAIM)))
+		return false;
+
+	__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
+
+	/* We want to clear it asap, so post */
+	__raw_i915_read32(dev_priv, FPGA_DBG);
+
+	return true;
+}
+
 static void __intel_uncore_early_sanitize(struct drm_device *dev,
 					  bool restore_forcewake)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (HAS_FPGA_DBG_UNCLAIMED(dev))
-		__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
+	/* clear out unclaimed reg detection bit */
+	if (check_for_unclaimed_mmio(dev_priv))
+		DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n");
 
 	/* clear out old GT FIFO errors */
 	if (IS_GEN6(dev) || IS_GEN7(dev))
@@ -601,10 +622,9 @@ hsw_unclaimed_reg_debug(struct drm_i915_private *dev_priv,
 	if (!i915.mmio_debug)
 		return;
 
-	if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
+	if (check_for_unclaimed_mmio(dev_priv)) {
 		WARN(1, "Unclaimed register detected %s %s register 0x%x\n",
 		     when, op, i915_mmio_reg_offset(reg));
-		__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
 		i915.mmio_debug--; /* Only report the first N failures */
 	}
 }
@@ -617,11 +637,10 @@ hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
 	if (i915.mmio_debug || !mmio_debug_once)
 		return;
 
-	if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
+	if (check_for_unclaimed_mmio(dev_priv)) {
 		DRM_DEBUG("Unclaimed register detected, "
 			  "enabling oneshot unclaimed register reporting. "
 			  "Please use i915.mmio_debug=N for more information.\n");
-		__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
 		i915.mmio_debug = mmio_debug_once--;
 	}
 }
@@ -1589,11 +1608,6 @@ bool intel_has_gpu_reset(struct drm_device *dev)
 
 void intel_uncore_check_errors(struct drm_device *dev)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	if (HAS_FPGA_DBG_UNCLAIMED(dev) &&
-	    (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM)) {
+	if (check_for_unclaimed_mmio(to_i915(dev)))
 		DRM_ERROR("Unclaimed register before interrupt\n");
-		__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
-	}
 }
-- 
2.5.0

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

[PATCH 2/7] drm/i915: Introduce intel_uncore_unclaimed_mmio

[Mika Kuoppala]

Currently interrupt code is the only place checking
for the unclaimed register access prior to actual register
macros using the same functionality. Rename the function
and make it return bool so that the possible error message
context is clear in the caller side. The motivation is to allow
usage of unclaimed detection on arbitrary places.

v2: rebase, s/access/mmio, s/dev/dev_priv

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h     | 2 +-
 drivers/gpu/drm/i915/i915_irq.c     | 3 ++-
 drivers/gpu/drm/i915/intel_uncore.c | 5 ++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9124085..82c43b6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2731,7 +2731,7 @@ extern void intel_uncore_sanitize(struct drm_device *dev);
 extern void intel_uncore_early_sanitize(struct drm_device *dev,
 					bool restore_forcewake);
 extern void intel_uncore_init(struct drm_device *dev);
-extern void intel_uncore_check_errors(struct drm_device *dev);
+extern bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv);
 extern void intel_uncore_fini(struct drm_device *dev);
 extern void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore);
 const char *intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 86664d1..a20dc64 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2167,7 +2167,8 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 
 	/* We get interrupts on unclaimed registers, so check for this before we
 	 * do any I915_{READ,WRITE}. */
-	intel_uncore_check_errors(dev);
+	if (intel_uncore_unclaimed_mmio(dev_priv))
+		DRM_ERROR("Unclaimed register before interrupt\n");
 
 	/* disable master interrupt before clearing iir  */
 	de_ier = I915_READ(DEIER);
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 95e7803..34b60cb 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1606,8 +1606,7 @@ bool intel_has_gpu_reset(struct drm_device *dev)
 	return intel_get_gpu_reset(dev) != NULL;
 }
 
-void intel_uncore_check_errors(struct drm_device *dev)
+bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv)
 {
-	if (check_for_unclaimed_mmio(to_i915(dev)))
-		DRM_ERROR("Unclaimed register before interrupt\n");
+	return check_for_unclaimed_mmio(dev_priv);
 }
-- 
2.5.0

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

[PATCH 3/7] drm/i915: Detect and clear unclaimed access on resume

[Mika Kuoppala]

If something, the usual suspect being bios, access hw
behind our back, don't let it slide into situation where
normal register access will detect this and spit out
a warn on into dmesg. On some bdw bioses this happens
during igt/bat run always and as there is not much we can
do about it, its better just to detect and flush this
explicitly on resume and only print a debug message.

v2: use DRM_DEBUG_DRIVER (Chris)
v3: s/access/mmio, s/prior/prior to, s/dev/dev_priv

Testcase: igt/pm_rpm/basic-rte
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Paulo Zanoni <przanoni@gmail.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 8ddfcce..3f2a1d0 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1478,6 +1478,9 @@ static int intel_runtime_suspend(struct device *device)
 	}
 
 	intel_uncore_forcewake_reset(dev, false);
+	if (intel_uncore_unclaimed_mmio(dev_priv))
+		DRM_ERROR("Unclaimed access detected prior to suspending\n");
+
 	dev_priv->pm.suspended = true;
 
 	/*
@@ -1523,6 +1526,8 @@ static int intel_runtime_resume(struct device *device)
 
 	intel_opregion_notify_adapter(dev, PCI_D0);
 	dev_priv->pm.suspended = false;
+	if (intel_uncore_unclaimed_mmio(dev_priv))
+		DRM_DEBUG_DRIVER("Unclaimed access during suspend, bios?\n");
 
 	intel_guc_resume(dev);
 
-- 
2.5.0

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

[PATCH 4/7] drm/i915: Do one shot unclaimed mmio detection less frequently

[Mika Kuoppala]

We have done unclaimed register access check in normal
(mmio_debug=0) mode once per write. This adds probability
of finding the exact sequence where we did the bad access, but
also adds burden to each write.

As we have mmio_debug available for more fine grained analysis,
give up accuracy of detecting correct spot at the first occurrence
by doing the one shot detection and arming of mmio_debug in hangcheck
and in modeset. This removes the write path performance burden.

Note that in hangcheck when we arm the fine grained debug
facilities, there is no context even if the unclaimed access
is already set and thus notifying in this point has no added value.
In contrary, in display path, we add a debug message if the bit
was set on arming.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Paulo Zanoni <przanoni@gmail.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  3 +++
 drivers/gpu/drm/i915/i915_irq.c      | 10 ++++-----
 drivers/gpu/drm/i915/intel_display.c |  3 +++
 drivers/gpu/drm/i915/intel_uncore.c  | 40 +++++++++++++++++++-----------------
 4 files changed, 32 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 82c43b6..625aae9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -723,6 +723,8 @@ struct intel_uncore {
 		i915_reg_t reg_post;
 		u32 val_reset;
 	} fw_domain[FW_DOMAIN_ID_COUNT];
+
+	int unclaimed_mmio_check;
 };
 
 /* Iterate over initialised fw domains */
@@ -2732,6 +2734,7 @@ extern void intel_uncore_early_sanitize(struct drm_device *dev,
 					bool restore_forcewake);
 extern void intel_uncore_init(struct drm_device *dev);
 extern bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv);
+extern bool intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv);
 extern void intel_uncore_fini(struct drm_device *dev);
 extern void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore);
 const char *intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a20dc64..725a340 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2165,11 +2165,6 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 	if (!intel_irqs_enabled(dev_priv))
 		return IRQ_NONE;
 
-	/* We get interrupts on unclaimed registers, so check for this before we
-	 * do any I915_{READ,WRITE}. */
-	if (intel_uncore_unclaimed_mmio(dev_priv))
-		DRM_ERROR("Unclaimed register before interrupt\n");
-
 	/* disable master interrupt before clearing iir  */
 	de_ier = I915_READ(DEIER);
 	I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
@@ -2990,6 +2985,11 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
 	if (!i915.enable_hangcheck)
 		return;
 
+	/* Periodic arming of mmio_debug if hw detects mmio
+	 * access to out of range or to an unpowered block
+	 */
+	intel_uncore_arm_unclaimed_mmio_detection(dev_priv);
+
 	for_each_ring(ring, dev_priv, i) {
 		u64 acthd;
 		u32 seqno;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 471f120..3a9229b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13545,6 +13545,9 @@ static int intel_atomic_commit(struct drm_device *dev,
 
 	drm_atomic_state_free(state);
 
+	if (intel_uncore_arm_unclaimed_mmio_detection(dev_priv))
+		DRM_DEBUG_DRIVER("%s return with unclaimed access\n", __func__);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 34b60cb..911f189 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -629,22 +629,6 @@ hsw_unclaimed_reg_debug(struct drm_i915_private *dev_priv,
 	}
 }
 
-static void
-hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
-{
-	static bool mmio_debug_once = true;
-
-	if (i915.mmio_debug || !mmio_debug_once)
-		return;
-
-	if (check_for_unclaimed_mmio(dev_priv)) {
-		DRM_DEBUG("Unclaimed register detected, "
-			  "enabling oneshot unclaimed register reporting. "
-			  "Please use i915.mmio_debug=N for more information.\n");
-		i915.mmio_debug = mmio_debug_once--;
-	}
-}
-
 #define GEN2_READ_HEADER(x) \
 	u##x val = 0; \
 	assert_device_not_suspended(dev_priv);
@@ -924,7 +908,6 @@ hsw_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, bool t
 		gen6_gt_check_fifodbg(dev_priv); \
 	} \
 	hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
-	hsw_unclaimed_reg_detect(dev_priv); \
 	GEN6_WRITE_FOOTER; \
 }
 
@@ -959,7 +942,6 @@ gen8_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, bool
 		__force_wake_get(dev_priv, FORCEWAKE_RENDER); \
 	__raw_i915_write##x(dev_priv, reg, val); \
 	hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
-	hsw_unclaimed_reg_detect(dev_priv); \
 	GEN6_WRITE_FOOTER; \
 }
 
@@ -1029,7 +1011,6 @@ gen9_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, \
 		__force_wake_get(dev_priv, fw_engine); \
 	__raw_i915_write##x(dev_priv, reg, val); \
 	hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
-	hsw_unclaimed_reg_detect(dev_priv); \
 	GEN6_WRITE_FOOTER; \
 }
 
@@ -1249,6 +1230,8 @@ void intel_uncore_init(struct drm_device *dev)
 	intel_uncore_fw_domains_init(dev);
 	__intel_uncore_early_sanitize(dev, false);
 
+	dev_priv->uncore.unclaimed_mmio_check = 1;
+
 	switch (INTEL_INFO(dev)->gen) {
 	default:
 	case 9:
@@ -1610,3 +1593,22 @@ bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv)
 {
 	return check_for_unclaimed_mmio(dev_priv);
 }
+
+bool
+intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv)
+{
+	if (unlikely(i915.mmio_debug ||
+		     dev_priv->uncore.unclaimed_mmio_check <= 0))
+		return false;
+
+	if (unlikely(intel_uncore_unclaimed_mmio(dev_priv))) {
+		DRM_DEBUG("Unclaimed register detected, "
+			  "enabling oneshot unclaimed register reporting. "
+			  "Please use i915.mmio_debug=N for more information.\n");
+		i915.mmio_debug++;
+		dev_priv->uncore.unclaimed_mmio_check--;
+		return true;
+	}
+
+	return false;
+}
-- 
2.5.0

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

[PATCH 5/7] drm/i915: Streamline unclaimed reg debug trace

[Mika Kuoppala]

Remove char* assignments and add branching hint and
also constify the parameters.

This results in a 35 bytes shorter fast path, so author
boldly assumes it helps without doing in-depth assembly
analysis.

v2: use WARN's branching (Chris), commit name (Joonas)

Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 911f189..02bad32 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -614,19 +614,19 @@ ilk_dummy_write(struct drm_i915_private *dev_priv)
 
 static void
 hsw_unclaimed_reg_debug(struct drm_i915_private *dev_priv,
-			i915_reg_t reg, bool read, bool before)
+			const i915_reg_t reg,
+			const bool read,
+			const bool before)
 {
-	const char *op = read ? "reading" : "writing to";
-	const char *when = before ? "before" : "after";
-
-	if (!i915.mmio_debug)
+	if (likely(!i915.mmio_debug))
 		return;
 
-	if (check_for_unclaimed_mmio(dev_priv)) {
-		WARN(1, "Unclaimed register detected %s %s register 0x%x\n",
-		     when, op, i915_mmio_reg_offset(reg));
+	if (WARN(check_for_unclaimed_mmio(dev_priv),
+		 "Unclaimed register detected %s %s register 0x%x\n",
+		 before ? "before" : "after",
+		 read ? "reading" : "writing to",
+		 i915_mmio_reg_offset(reg)))
 		i915.mmio_debug--; /* Only report the first N failures */
-	}
 }
 
 #define GEN2_READ_HEADER(x) \
-- 
2.5.0

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

[PATCH 6/7] drm/i915: Add unclaimed mmio check to runtime pm get/put

[Mika Kuoppala]

This is a natural boundary to peek if our power domain
management is accurate.

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 2c2151f..5eea177 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2230,6 +2230,7 @@ void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
 		return;
 
 	pm_runtime_get_sync(device);
+	WARN_ON(intel_uncore_arm_unclaimed_mmio_detection(dev_priv));
 	WARN(dev_priv->pm.suspended, "Device still suspended.\n");
 }
 
@@ -2275,6 +2276,8 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv)
 	struct drm_device *dev = dev_priv->dev;
 	struct device *device = &dev->pdev->dev;
 
+	WARN_ON(intel_uncore_arm_unclaimed_mmio_detection(dev_priv));
+
 	if (!HAS_RUNTIME_PM(dev))
 		return;
 
-- 
2.5.0

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

[PATCH 7/7] drm/i915/chv: Add non claimed mmio checking

[Mika Kuoppala]

Imre mentioned that chv might also have capability to
track unclaimed mmio accesses. And it has, so take it
into use.

Cc: Imre Deak <imre.deak@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h     |  4 ++++
 drivers/gpu/drm/i915/intel_uncore.c | 34 ++++++++++++++++++++++++++++++----
 2 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 007ae83..24686ab 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1711,6 +1711,10 @@ enum skl_disp_power_wells {
 #define FPGA_DBG		_MMIO(0x42300)
 #define   FPGA_DBG_RM_NOCLAIM	(1<<31)
 
+#define CLAIM_ER		_MMIO(0x182028)
+#define   CLAIM_ER_CLR		(1<<31)
+#define   CLAIM_ER_CTR_MASK	(0xffff)
+
 #define DERRMR		_MMIO(0x44050)
 /* Note that HBLANK events are reserved on bdw+ */
 #define   DERRMR_PIPEA_SCANLINE		(1<<0)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 02bad32..ea8fcd4 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -335,13 +335,10 @@ static void intel_uncore_ellc_detect(struct drm_device *dev)
 }
 
 static bool
-check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
+fpga_check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
 {
 	u32 dbg;
 
-	if (!HAS_FPGA_DBG_UNCLAIMED(dev_priv))
-		return false;
-
 	dbg = __raw_i915_read32(dev_priv, FPGA_DBG);
 	if (likely(!(dbg & FPGA_DBG_RM_NOCLAIM)))
 		return false;
@@ -354,6 +351,35 @@ check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
 	return true;
 }
 
+static bool
+chv_check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
+{
+	u32 cer;
+
+	cer = __raw_i915_read32(dev_priv, CLAIM_ER);
+	if (likely(!(cer & CLAIM_ER_CTR_MASK)))
+		return false;
+
+	__raw_i915_write32(dev_priv, CLAIM_ER, CLAIM_ER_CLR);
+
+	/* We want to clear it asap, so post */
+	__raw_i915_read32(dev_priv, CLAIM_ER);
+
+	return true;
+}
+
+static bool
+check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
+{
+	if (HAS_FPGA_DBG_UNCLAIMED(dev_priv))
+		return fpga_check_for_unclaimed_mmio(dev_priv);
+
+	if (IS_CHERRYVIEW(dev_priv))
+		return chv_check_for_unclaimed_mmio(dev_priv);
+
+	return false;
+}
+
 static void __intel_uncore_early_sanitize(struct drm_device *dev,
 					  bool restore_forcewake)
 {
-- 
2.5.0

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

Re: [PATCH 6/7] drm/i915: Add unclaimed mmio check to runtime pm get/put

[Chris Wilson]

On Tue, Dec 15, 2015 at 04:25:11PM +0200, Mika Kuoppala wrote:
> This is a natural boundary to peek if our power domain
> management is accurate.

No, I have plans for this to be very frequent. In that bold plan,
the suspend/resume boundary is more sensible.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/7] drm/i915: Consolidate unclaimed mmio detection

[Chris Wilson]

On Tue, Dec 15, 2015 at 04:25:06PM +0200, Mika Kuoppala wrote:
> Access the unclaimed reg detection register through
> one helper which also does cleanup. Note that we now access
> the register only if the platform has the actual non claimed
> access bit. This prevents reading the register with gens that
> doesn't have the register or the unclaimed bit,
> when debug_mmio > 0.
> 
> Note that we post after clearing the bit. This makes sure
> that the next unclaimed write access would get detected
> also if it happened right after clearing, and not fold
> into the previous detection.

I didn't like 6 (adding checks to rpm get/put), and I didn't see the
removal of the check from the interrupt? Or adding it to the modesetting
code?

Did I just miss those?

The other patches (1-5?) (aside from adding chv as I haven't validated the
registers myself) are
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/7] drm/i915: Consolidate unclaimed mmio detection

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> On Tue, Dec 15, 2015 at 04:25:06PM +0200, Mika Kuoppala wrote:
>> Access the unclaimed reg detection register through
>> one helper which also does cleanup. Note that we now access
>> the register only if the platform has the actual non claimed
>> access bit. This prevents reading the register with gens that
>> doesn't have the register or the unclaimed bit,
>> when debug_mmio > 0.
>> 
>> Note that we post after clearing the bit. This makes sure
>> that the next unclaimed write access would get detected
>> also if it happened right after clearing, and not fold
>> into the previous detection.
>
> I didn't like 6 (adding checks to rpm get/put), and I didn't see the
> removal of the check from the interrupt? Or adding it to the modesetting
> code?
>
> Did I just miss those?
>

They should be included in 4/7.
-Mika

> The other patches (1-5?) (aside from adding chv as I haven't validated the
> registers myself) are
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/7] drm/i915/chv: Add non claimed mmio checking

[Ville Syrjälä]

On Tue, Dec 15, 2015 at 04:25:12PM +0200, Mika Kuoppala wrote:
> Imre mentioned that chv might also have capability to
> track unclaimed mmio accesses. And it has, so take it
> into use.

VLV too.

My old patch:
http://lists.freedesktop.org/archives/intel-gfx/2013-May/027599.html

> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h     |  4 ++++
>  drivers/gpu/drm/i915/intel_uncore.c | 34 ++++++++++++++++++++++++++++++----
>  2 files changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 007ae83..24686ab 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1711,6 +1711,10 @@ enum skl_disp_power_wells {
>  #define FPGA_DBG		_MMIO(0x42300)
>  #define   FPGA_DBG_RM_NOCLAIM	(1<<31)
>  
> +#define CLAIM_ER		_MMIO(0x182028)
> +#define   CLAIM_ER_CLR		(1<<31)

Looks like you forgot the overflow bit.

> +#define   CLAIM_ER_CTR_MASK	(0xffff)

Needless parens.

> +
>  #define DERRMR		_MMIO(0x44050)
>  /* Note that HBLANK events are reserved on bdw+ */
>  #define   DERRMR_PIPEA_SCANLINE		(1<<0)
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 02bad32..ea8fcd4 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -335,13 +335,10 @@ static void intel_uncore_ellc_detect(struct drm_device *dev)
>  }
>  
>  static bool
> -check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
> +fpga_check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
>  {
>  	u32 dbg;
>  
> -	if (!HAS_FPGA_DBG_UNCLAIMED(dev_priv))
> -		return false;
> -
>  	dbg = __raw_i915_read32(dev_priv, FPGA_DBG);
>  	if (likely(!(dbg & FPGA_DBG_RM_NOCLAIM)))
>  		return false;
> @@ -354,6 +351,35 @@ check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
>  	return true;
>  }
>  
> +static bool
> +chv_check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
> +{
> +	u32 cer;
> +
> +	cer = __raw_i915_read32(dev_priv, CLAIM_ER);
> +	if (likely(!(cer & CLAIM_ER_CTR_MASK)))
> +		return false;
> +
> +	__raw_i915_write32(dev_priv, CLAIM_ER, CLAIM_ER_CLR);
> +
> +	/* We want to clear it asap, so post */
> +	__raw_i915_read32(dev_priv, CLAIM_ER);

__raw_posting_read()

But not sure why we'd want to do this read really. Seems like pointless
overhead.

The other thing is that this only detect unclaimed RM cycles, so display
registers only. Might be nice to only do the checks when accessing
display registers so that we don't add overhead to the GT stuff.
I think the same holds for HSW+ actually.

> +
> +	return true;
> +}
> +
> +static bool
> +check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
> +{
> +	if (HAS_FPGA_DBG_UNCLAIMED(dev_priv))
> +		return fpga_check_for_unclaimed_mmio(dev_priv);
> +
> +	if (IS_CHERRYVIEW(dev_priv))
> +		return chv_check_for_unclaimed_mmio(dev_priv);
> +
> +	return false;
> +}
> +
>  static void __intel_uncore_early_sanitize(struct drm_device *dev,
>  					  bool restore_forcewake)
>  {
> -- 
> 2.5.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915: Introduce intel_uncore_unclaimed_mmio

[Ville Syrjälä]

On Tue, Dec 15, 2015 at 04:25:07PM +0200, Mika Kuoppala wrote:
> Currently interrupt code is the only place checking
> for the unclaimed register access prior to actual register
> macros using the same functionality. Rename the function
> and make it return bool so that the possible error message
> context is clear in the caller side. The motivation is to allow
> usage of unclaimed detection on arbitrary places.
> 
> v2: rebase, s/access/mmio, s/dev/dev_priv
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_drv.h     | 2 +-
>  drivers/gpu/drm/i915/i915_irq.c     | 3 ++-
>  drivers/gpu/drm/i915/intel_uncore.c | 5 ++---
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9124085..82c43b6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2731,7 +2731,7 @@ extern void intel_uncore_sanitize(struct drm_device *dev);
>  extern void intel_uncore_early_sanitize(struct drm_device *dev,
>  					bool restore_forcewake);
>  extern void intel_uncore_init(struct drm_device *dev);
> -extern void intel_uncore_check_errors(struct drm_device *dev);
> +extern bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv);
>  extern void intel_uncore_fini(struct drm_device *dev);
>  extern void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore);
>  const char *intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 86664d1..a20dc64 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2167,7 +2167,8 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  
>  	/* We get interrupts on unclaimed registers, so check for this before we
>  	 * do any I915_{READ,WRITE}. */
> -	intel_uncore_check_errors(dev);
> +	if (intel_uncore_unclaimed_mmio(dev_priv))
> +		DRM_ERROR("Unclaimed register before interrupt\n");

bdw+ too? Or do those work differently?

>  
>  	/* disable master interrupt before clearing iir  */
>  	de_ier = I915_READ(DEIER);
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 95e7803..34b60cb 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1606,8 +1606,7 @@ bool intel_has_gpu_reset(struct drm_device *dev)
>  	return intel_get_gpu_reset(dev) != NULL;
>  }
>  
> -void intel_uncore_check_errors(struct drm_device *dev)
> +bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv)
>  {
> -	if (check_for_unclaimed_mmio(to_i915(dev)))
> -		DRM_ERROR("Unclaimed register before interrupt\n");
> +	return check_for_unclaimed_mmio(dev_priv);
>  }
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] drm/i915: Do one shot unclaimed mmio detection less frequently

[Chris Wilson]

On Tue, Dec 15, 2015 at 04:25:09PM +0200, Mika Kuoppala wrote:
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 471f120..3a9229b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13545,6 +13545,9 @@ static int intel_atomic_commit(struct drm_device *dev,
>  
>  	drm_atomic_state_free(state);
>  
> +	if (intel_uncore_arm_unclaimed_mmio_detection(dev_priv))
> +		DRM_DEBUG_DRIVER("%s return with unclaimed access\n", __func__);

DRM_DEBUG() already includes the "%s" _func__.

This would seem to duplicate the DRM_DEBUG from
intel_uncore_arm_unclaimed_mmio_detection() which as you point out
should have been DRM_DEBUG_DRIVER() in the first place.

Minor quible aside (and because this turned up late!),
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] drm/i915: Do one shot unclaimed mmio detection less frequently

[Ville Syrjälä]

On Tue, Dec 15, 2015 at 04:25:09PM +0200, Mika Kuoppala wrote:
> We have done unclaimed register access check in normal
> (mmio_debug=0) mode once per write. This adds probability
> of finding the exact sequence where we did the bad access, but
> also adds burden to each write.
> 
> As we have mmio_debug available for more fine grained analysis,
> give up accuracy of detecting correct spot at the first occurrence
> by doing the one shot detection and arming of mmio_debug in hangcheck
> and in modeset. This removes the write path performance burden.
> 
> Note that in hangcheck when we arm the fine grained debug
> facilities, there is no context even if the unclaimed access
> is already set and thus notifying in this point has no added value.
> In contrary, in display path, we add a debug message if the bit
> was set on arming.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Paulo Zanoni <przanoni@gmail.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  3 +++
>  drivers/gpu/drm/i915/i915_irq.c      | 10 ++++-----
>  drivers/gpu/drm/i915/intel_display.c |  3 +++
>  drivers/gpu/drm/i915/intel_uncore.c  | 40 +++++++++++++++++++-----------------
>  4 files changed, 32 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 82c43b6..625aae9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -723,6 +723,8 @@ struct intel_uncore {
>  		i915_reg_t reg_post;
>  		u32 val_reset;
>  	} fw_domain[FW_DOMAIN_ID_COUNT];
> +
> +	int unclaimed_mmio_check;
>  };
>  
>  /* Iterate over initialised fw domains */
> @@ -2732,6 +2734,7 @@ extern void intel_uncore_early_sanitize(struct drm_device *dev,
>  					bool restore_forcewake);
>  extern void intel_uncore_init(struct drm_device *dev);
>  extern bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv);
> +extern bool intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv);
>  extern void intel_uncore_fini(struct drm_device *dev);
>  extern void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore);
>  const char *intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index a20dc64..725a340 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2165,11 +2165,6 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  	if (!intel_irqs_enabled(dev_priv))
>  		return IRQ_NONE;
>  
> -	/* We get interrupts on unclaimed registers, so check for this before we
> -	 * do any I915_{READ,WRITE}. */
> -	if (intel_uncore_unclaimed_mmio(dev_priv))
> -		DRM_ERROR("Unclaimed register before interrupt\n");
> -
>  	/* disable master interrupt before clearing iir  */
>  	de_ier = I915_READ(DEIER);
>  	I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
> @@ -2990,6 +2985,11 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
>  	if (!i915.enable_hangcheck)
>  		return;
>  
> +	/* Periodic arming of mmio_debug if hw detects mmio
> +	 * access to out of range or to an unpowered block
> +	 */
> +	intel_uncore_arm_unclaimed_mmio_detection(dev_priv);

It's a bit weird to have this here since it'll only detect display
register accesses AFAIK. But I guess it's cheaper that adding a
dedicated timer or anything for it.

> +
>  	for_each_ring(ring, dev_priv, i) {
>  		u64 acthd;
>  		u32 seqno;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 471f120..3a9229b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13545,6 +13545,9 @@ static int intel_atomic_commit(struct drm_device *dev,
>  
>  	drm_atomic_state_free(state);
>  
> +	if (intel_uncore_arm_unclaimed_mmio_detection(dev_priv))
> +		DRM_DEBUG_DRIVER("%s return with unclaimed access\n", __func__);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 34b60cb..911f189 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -629,22 +629,6 @@ hsw_unclaimed_reg_debug(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> -static void
> -hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
> -{
> -	static bool mmio_debug_once = true;
> -
> -	if (i915.mmio_debug || !mmio_debug_once)
> -		return;
> -
> -	if (check_for_unclaimed_mmio(dev_priv)) {
> -		DRM_DEBUG("Unclaimed register detected, "
> -			  "enabling oneshot unclaimed register reporting. "
> -			  "Please use i915.mmio_debug=N for more information.\n");
> -		i915.mmio_debug = mmio_debug_once--;
> -	}
> -}
> -
>  #define GEN2_READ_HEADER(x) \
>  	u##x val = 0; \
>  	assert_device_not_suspended(dev_priv);
> @@ -924,7 +908,6 @@ hsw_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, bool t
>  		gen6_gt_check_fifodbg(dev_priv); \
>  	} \
>  	hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
> -	hsw_unclaimed_reg_detect(dev_priv); \
>  	GEN6_WRITE_FOOTER; \
>  }
>  
> @@ -959,7 +942,6 @@ gen8_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, bool
>  		__force_wake_get(dev_priv, FORCEWAKE_RENDER); \
>  	__raw_i915_write##x(dev_priv, reg, val); \
>  	hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
> -	hsw_unclaimed_reg_detect(dev_priv); \
>  	GEN6_WRITE_FOOTER; \
>  }
>  
> @@ -1029,7 +1011,6 @@ gen9_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, \
>  		__force_wake_get(dev_priv, fw_engine); \
>  	__raw_i915_write##x(dev_priv, reg, val); \
>  	hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
> -	hsw_unclaimed_reg_detect(dev_priv); \
>  	GEN6_WRITE_FOOTER; \
>  }
>  
> @@ -1249,6 +1230,8 @@ void intel_uncore_init(struct drm_device *dev)
>  	intel_uncore_fw_domains_init(dev);
>  	__intel_uncore_early_sanitize(dev, false);
>  
> +	dev_priv->uncore.unclaimed_mmio_check = 1;
> +
>  	switch (INTEL_INFO(dev)->gen) {
>  	default:
>  	case 9:
> @@ -1610,3 +1593,22 @@ bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv)
>  {
>  	return check_for_unclaimed_mmio(dev_priv);
>  }
> +
> +bool
> +intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv)
> +{
> +	if (unlikely(i915.mmio_debug ||
> +		     dev_priv->uncore.unclaimed_mmio_check <= 0))
> +		return false;
> +
> +	if (unlikely(intel_uncore_unclaimed_mmio(dev_priv))) {
> +		DRM_DEBUG("Unclaimed register detected, "
> +			  "enabling oneshot unclaimed register reporting. "
> +			  "Please use i915.mmio_debug=N for more information.\n");
> +		i915.mmio_debug++;
> +		dev_priv->uncore.unclaimed_mmio_check--;
> +		return true;
> +	}
> +
> +	return false;
> +}
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/7] drm/i915/chv: Add non claimed mmio checking

[Ville Syrjälä]

On Tue, Dec 15, 2015 at 04:44:25PM +0200, Ville Syrjälä wrote:
> On Tue, Dec 15, 2015 at 04:25:12PM +0200, Mika Kuoppala wrote:
> > Imre mentioned that chv might also have capability to
> > track unclaimed mmio accesses. And it has, so take it
> > into use.
> 
> VLV too.
> 
> My old patch:
> http://lists.freedesktop.org/archives/intel-gfx/2013-May/027599.html
> 
> > 
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h     |  4 ++++
> >  drivers/gpu/drm/i915/intel_uncore.c | 34 ++++++++++++++++++++++++++++++----
> >  2 files changed, 34 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 007ae83..24686ab 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1711,6 +1711,10 @@ enum skl_disp_power_wells {
> >  #define FPGA_DBG		_MMIO(0x42300)
> >  #define   FPGA_DBG_RM_NOCLAIM	(1<<31)
> >  
> > +#define CLAIM_ER		_MMIO(0x182028)
> > +#define   CLAIM_ER_CLR		(1<<31)
> 
> Looks like you forgot the overflow bit.
> 
> > +#define   CLAIM_ER_CTR_MASK	(0xffff)
> 
> Needless parens.
> 
> > +
> >  #define DERRMR		_MMIO(0x44050)
> >  /* Note that HBLANK events are reserved on bdw+ */
> >  #define   DERRMR_PIPEA_SCANLINE		(1<<0)
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index 02bad32..ea8fcd4 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -335,13 +335,10 @@ static void intel_uncore_ellc_detect(struct drm_device *dev)
> >  }
> >  
> >  static bool
> > -check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
> > +fpga_check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
> >  {
> >  	u32 dbg;
> >  
> > -	if (!HAS_FPGA_DBG_UNCLAIMED(dev_priv))
> > -		return false;
> > -
> >  	dbg = __raw_i915_read32(dev_priv, FPGA_DBG);
> >  	if (likely(!(dbg & FPGA_DBG_RM_NOCLAIM)))
> >  		return false;
> > @@ -354,6 +351,35 @@ check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
> >  	return true;
> >  }
> >  
> > +static bool
> > +chv_check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
> > +{
> > +	u32 cer;
> > +
> > +	cer = __raw_i915_read32(dev_priv, CLAIM_ER);
> > +	if (likely(!(cer & CLAIM_ER_CTR_MASK)))
> > +		return false;
> > +
> > +	__raw_i915_write32(dev_priv, CLAIM_ER, CLAIM_ER_CLR);
> > +
> > +	/* We want to clear it asap, so post */
> > +	__raw_i915_read32(dev_priv, CLAIM_ER);
> 
> __raw_posting_read()
> 
> But not sure why we'd want to do this read really. Seems like pointless
> overhead.
> 
> The other thing is that this only detect unclaimed RM cycles, so display
> registers only. Might be nice to only do the checks when accessing
> display registers so that we don't add overhead to the GT stuff.
> I think the same holds for HSW+ actually.

Oh, after reading some more it doesn't look like you're adding the
checks to the register accesses for chv. So no overhead there.

I think one natural place for some explicit unclaimer reg checks would
be the disp2d power well enable/disable, since a disabled power well
might be the cause for the error. IIRC another could be simply a bogus
register offset.

> 
> > +
> > +	return true;
> > +}
> > +
> > +static bool
> > +check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
> > +{
> > +	if (HAS_FPGA_DBG_UNCLAIMED(dev_priv))
> > +		return fpga_check_for_unclaimed_mmio(dev_priv);
> > +
> > +	if (IS_CHERRYVIEW(dev_priv))
> > +		return chv_check_for_unclaimed_mmio(dev_priv);
> > +
> > +	return false;
> > +}
> > +
> >  static void __intel_uncore_early_sanitize(struct drm_device *dev,
> >  					  bool restore_forcewake)
> >  {
> > -- 
> > 2.5.0
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/7] drm/i915/chv: Add non claimed mmio checking

[Mika Kuoppala]

Ville Syrjälä <ville.syrjala@linux.intel.com> writes:

> On Tue, Dec 15, 2015 at 04:25:12PM +0200, Mika Kuoppala wrote:
>> Imre mentioned that chv might also have capability to
>> track unclaimed mmio accesses. And it has, so take it
>> into use.
>
> VLV too.
>
> My old patch:
> http://lists.freedesktop.org/archives/intel-gfx/2013-May/027599.html
>
>> 
>> Cc: Imre Deak <imre.deak@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h     |  4 ++++
>>  drivers/gpu/drm/i915/intel_uncore.c | 34 ++++++++++++++++++++++++++++++----
>>  2 files changed, 34 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 007ae83..24686ab 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -1711,6 +1711,10 @@ enum skl_disp_power_wells {
>>  #define FPGA_DBG		_MMIO(0x42300)
>>  #define   FPGA_DBG_RM_NOCLAIM	(1<<31)
>>  
>> +#define CLAIM_ER		_MMIO(0x182028)
>> +#define   CLAIM_ER_CLR		(1<<31)
>
> Looks like you forgot the overflow bit.
>
>> +#define   CLAIM_ER_CTR_MASK	(0xffff)
>
> Needless parens.
>
>> +
>>  #define DERRMR		_MMIO(0x44050)
>>  /* Note that HBLANK events are reserved on bdw+ */
>>  #define   DERRMR_PIPEA_SCANLINE		(1<<0)
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index 02bad32..ea8fcd4 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -335,13 +335,10 @@ static void intel_uncore_ellc_detect(struct drm_device *dev)
>>  }
>>  
>>  static bool
>> -check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
>> +fpga_check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
>>  {
>>  	u32 dbg;
>>  
>> -	if (!HAS_FPGA_DBG_UNCLAIMED(dev_priv))
>> -		return false;
>> -
>>  	dbg = __raw_i915_read32(dev_priv, FPGA_DBG);
>>  	if (likely(!(dbg & FPGA_DBG_RM_NOCLAIM)))
>>  		return false;
>> @@ -354,6 +351,35 @@ check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
>>  	return true;
>>  }
>>  
>> +static bool
>> +chv_check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
>> +{
>> +	u32 cer;
>> +
>> +	cer = __raw_i915_read32(dev_priv, CLAIM_ER);
>> +	if (likely(!(cer & CLAIM_ER_CTR_MASK)))
>> +		return false;
>> +
>> +	__raw_i915_write32(dev_priv, CLAIM_ER, CLAIM_ER_CLR);
>> +
>> +	/* We want to clear it asap, so post */
>> +	__raw_i915_read32(dev_priv, CLAIM_ER);
>
> __raw_posting_read()
>
> But not sure why we'd want to do this read really. Seems like pointless
> overhead.
>

I saw the bit stick for a short time even after write. That lead
me to think that perhaps the detection logic in hw rearms
based on this bit. So by flushing it quickly we dont coalesce
the potential next write into the previous detect.

Also as this is now in the slow path (mmio_debug==0),
the extra overhead should not be an issue.

But pointless overhead is, pointless. My worries about coalescing
the detect bit might be theoretical at best, so I dont mind removing
the posting.

-Mika

> The other thing is that this only detect unclaimed RM cycles, so display
> registers only. Might be nice to only do the checks when accessing
> display registers so that we don't add overhead to the GT stuff.
> I think the same holds for HSW+ actually.
>
>> +
>> +	return true;
>> +}
>> +
>> +static bool
>> +check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
>> +{
>> +	if (HAS_FPGA_DBG_UNCLAIMED(dev_priv))
>> +		return fpga_check_for_unclaimed_mmio(dev_priv);
>> +
>> +	if (IS_CHERRYVIEW(dev_priv))
>> +		return chv_check_for_unclaimed_mmio(dev_priv);
>> +
>> +	return false;
>> +}
>> +
>>  static void __intel_uncore_early_sanitize(struct drm_device *dev,
>>  					  bool restore_forcewake)
>>  {
>> -- 
>> 2.5.0
>
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915: Introduce intel_uncore_unclaimed_mmio

[Mika Kuoppala]

Ville Syrjälä <ville.syrjala@linux.intel.com> writes:

> On Tue, Dec 15, 2015 at 04:25:07PM +0200, Mika Kuoppala wrote:
>> Currently interrupt code is the only place checking
>> for the unclaimed register access prior to actual register
>> macros using the same functionality. Rename the function
>> and make it return bool so that the possible error message
>> context is clear in the caller side. The motivation is to allow
>> usage of unclaimed detection on arbitrary places.
>> 
>> v2: rebase, s/access/mmio, s/dev/dev_priv
>> 
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h     | 2 +-
>>  drivers/gpu/drm/i915/i915_irq.c     | 3 ++-
>>  drivers/gpu/drm/i915/intel_uncore.c | 5 ++---
>>  3 files changed, 5 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 9124085..82c43b6 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2731,7 +2731,7 @@ extern void intel_uncore_sanitize(struct drm_device *dev);
>>  extern void intel_uncore_early_sanitize(struct drm_device *dev,
>>  					bool restore_forcewake);
>>  extern void intel_uncore_init(struct drm_device *dev);
>> -extern void intel_uncore_check_errors(struct drm_device *dev);
>> +extern bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv);
>>  extern void intel_uncore_fini(struct drm_device *dev);
>>  extern void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore);
>>  const char *intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id);
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index 86664d1..a20dc64 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -2167,7 +2167,8 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>>  
>>  	/* We get interrupts on unclaimed registers, so check for this before we
>>  	 * do any I915_{READ,WRITE}. */
>> -	intel_uncore_check_errors(dev);
>> +	if (intel_uncore_unclaimed_mmio(dev_priv))
>> +		DRM_ERROR("Unclaimed register before interrupt\n");
>
> bdw+ too? Or do those work differently?

bdw too yes. But we throw the irq arming out in 4/7
completely.

-Mika

>
>>  
>>  	/* disable master interrupt before clearing iir  */
>>  	de_ier = I915_READ(DEIER);
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index 95e7803..34b60cb 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -1606,8 +1606,7 @@ bool intel_has_gpu_reset(struct drm_device *dev)
>>  	return intel_get_gpu_reset(dev) != NULL;
>>  }
>>  
>> -void intel_uncore_check_errors(struct drm_device *dev)
>> +bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv)
>>  {
>> -	if (check_for_unclaimed_mmio(to_i915(dev)))
>> -		DRM_ERROR("Unclaimed register before interrupt\n");
>> +	return check_for_unclaimed_mmio(dev_priv);
>>  }
>> -- 
>> 2.5.0
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/7] drm/i915: Consolidate unclaimed mmio detection

[Mika Kuoppala]

Access the unclaimed reg detection register through
one helper which also does cleanup. Note that we now access
the register only if the platform has the actual non claimed
access bit. This prevents reading the register with gens that
doesn't have the register or the unclaimed bit,
when debug_mmio > 0.

Note that we post after clearing the bit. This makes sure
that the next unclaimed write access would get detected
also if it happened right after clearing, and not fold
into the previous detection.

v2: s/unclaimed_reg_access/check_for_unclaimed_mmio (Chris)
    debug log on unclaimed detection on uncore init (Joonas)

v3: remove posting read (Ville)

Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Paulo Zanoni <przanoni@gmail.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_uncore.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index fcf04fe..fb13b5c 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -334,13 +334,31 @@ static void intel_uncore_ellc_detect(struct drm_device *dev)
 	}
 }
 
+static bool
+check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
+{
+	u32 dbg;
+
+	if (!HAS_FPGA_DBG_UNCLAIMED(dev_priv))
+		return false;
+
+	dbg = __raw_i915_read32(dev_priv, FPGA_DBG);
+	if (likely(!(dbg & FPGA_DBG_RM_NOCLAIM)))
+		return false;
+
+	__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
+
+	return true;
+}
+
 static void __intel_uncore_early_sanitize(struct drm_device *dev,
 					  bool restore_forcewake)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (HAS_FPGA_DBG_UNCLAIMED(dev))
-		__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
+	/* clear out unclaimed reg detection bit */
+	if (check_for_unclaimed_mmio(dev_priv))
+		DRM_DEBUG("unclaimed mmio detected on uncore init, clearing\n");
 
 	/* clear out old GT FIFO errors */
 	if (IS_GEN6(dev) || IS_GEN7(dev))
@@ -601,10 +619,9 @@ hsw_unclaimed_reg_debug(struct drm_i915_private *dev_priv,
 	if (!i915.mmio_debug)
 		return;
 
-	if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
+	if (check_for_unclaimed_mmio(dev_priv)) {
 		WARN(1, "Unclaimed register detected %s %s register 0x%x\n",
 		     when, op, i915_mmio_reg_offset(reg));
-		__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
 		i915.mmio_debug--; /* Only report the first N failures */
 	}
 }
@@ -617,11 +634,10 @@ hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
 	if (i915.mmio_debug || !mmio_debug_once)
 		return;
 
-	if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
+	if (check_for_unclaimed_mmio(dev_priv)) {
 		DRM_DEBUG("Unclaimed register detected, "
 			  "enabling oneshot unclaimed register reporting. "
 			  "Please use i915.mmio_debug=N for more information.\n");
-		__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
 		i915.mmio_debug = mmio_debug_once--;
 	}
 }
@@ -1589,11 +1605,6 @@ bool intel_has_gpu_reset(struct drm_device *dev)
 
 void intel_uncore_check_errors(struct drm_device *dev)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	if (HAS_FPGA_DBG_UNCLAIMED(dev) &&
-	    (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM)) {
+	if (check_for_unclaimed_mmio(to_i915(dev)))
 		DRM_ERROR("Unclaimed register before interrupt\n");
-		__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
-	}
 }
-- 
2.5.0

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

[PATCH 7/7] drm/i915: Add non claimed mmio checking for vlv/chv

[Mika Kuoppala]

Imre mentioned that chv might also have capability to
track unclaimed mmio accesses. Ville added that
both chv and vlv has this capability and he had already
made this way back [1]. Mimic what Ville's patch does
but adapt on top of less frequent mmio accesses by
omitting checking always on reg writes.

This patch is untested as of now.

v2: overflow handling and posting omitted (Ville)

References: [1] http://lists.freedesktop.org/archives/intel-gfx/2013-May/027599.html
Cc: Imre Deak <imre.deak@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h     |  5 +++++
 drivers/gpu/drm/i915/intel_uncore.c | 31 +++++++++++++++++++++++++++----
 2 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 007ae83..0a98889 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1711,6 +1711,11 @@ enum skl_disp_power_wells {
 #define FPGA_DBG		_MMIO(0x42300)
 #define   FPGA_DBG_RM_NOCLAIM	(1<<31)
 
+#define CLAIM_ER		_MMIO(VLV_DISPLAY_BASE + 0x2028)
+#define   CLAIM_ER_CLR		(1 << 31)
+#define   CLAIM_ER_OVERFLOW	(1 << 16)
+#define   CLAIM_ER_CTR_MASK	0xffff
+
 #define DERRMR		_MMIO(0x44050)
 /* Note that HBLANK events are reserved on bdw+ */
 #define   DERRMR_PIPEA_SCANLINE		(1<<0)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index b6910eb..61179ae 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -335,13 +335,10 @@ static void intel_uncore_ellc_detect(struct drm_device *dev)
 }
 
 static bool
-check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
+fpga_check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
 {
 	u32 dbg;
 
-	if (!HAS_FPGA_DBG_UNCLAIMED(dev_priv))
-		return false;
-
 	dbg = __raw_i915_read32(dev_priv, FPGA_DBG);
 	if (likely(!(dbg & FPGA_DBG_RM_NOCLAIM)))
 		return false;
@@ -351,6 +348,32 @@ check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
 	return true;
 }
 
+static bool
+vlv_check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
+{
+	u32 cer;
+
+	cer = __raw_i915_read32(dev_priv, CLAIM_ER);
+	if (likely(!(cer & (CLAIM_ER_OVERFLOW | CLAIM_ER_CTR_MASK))))
+		return false;
+
+	__raw_i915_write32(dev_priv, CLAIM_ER, CLAIM_ER_CLR);
+
+	return true;
+}
+
+static bool
+check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
+{
+	if (HAS_FPGA_DBG_UNCLAIMED(dev_priv))
+		return fpga_check_for_unclaimed_mmio(dev_priv);
+
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+		return vlv_check_for_unclaimed_mmio(dev_priv);
+
+	return false;
+}
+
 static void __intel_uncore_early_sanitize(struct drm_device *dev,
 					  bool restore_forcewake)
 {
-- 
2.5.0

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

Re: [PATCH 7/7] drm/i915: Add non claimed mmio checking for vlv/chv

[Ville Syrjälä]

On Tue, Dec 15, 2015 at 07:45:42PM +0200, Mika Kuoppala wrote:
> Imre mentioned that chv might also have capability to
> track unclaimed mmio accesses. Ville added that
> both chv and vlv has this capability and he had already
> made this way back [1]. Mimic what Ville's patch does
> but adapt on top of less frequent mmio accesses by
> omitting checking always on reg writes.
> 
> This patch is untested as of now.
> 
> v2: overflow handling and posting omitted (Ville)
> 
> References: [1] http://lists.freedesktop.org/archives/intel-gfx/2013-May/027599.html
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>

lgtm

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_reg.h     |  5 +++++
>  drivers/gpu/drm/i915/intel_uncore.c | 31 +++++++++++++++++++++++++++----
>  2 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 007ae83..0a98889 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1711,6 +1711,11 @@ enum skl_disp_power_wells {
>  #define FPGA_DBG		_MMIO(0x42300)
>  #define   FPGA_DBG_RM_NOCLAIM	(1<<31)
>  
> +#define CLAIM_ER		_MMIO(VLV_DISPLAY_BASE + 0x2028)
> +#define   CLAIM_ER_CLR		(1 << 31)
> +#define   CLAIM_ER_OVERFLOW	(1 << 16)
> +#define   CLAIM_ER_CTR_MASK	0xffff
> +
>  #define DERRMR		_MMIO(0x44050)
>  /* Note that HBLANK events are reserved on bdw+ */
>  #define   DERRMR_PIPEA_SCANLINE		(1<<0)
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index b6910eb..61179ae 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -335,13 +335,10 @@ static void intel_uncore_ellc_detect(struct drm_device *dev)
>  }
>  
>  static bool
> -check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
> +fpga_check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
>  {
>  	u32 dbg;
>  
> -	if (!HAS_FPGA_DBG_UNCLAIMED(dev_priv))
> -		return false;
> -
>  	dbg = __raw_i915_read32(dev_priv, FPGA_DBG);
>  	if (likely(!(dbg & FPGA_DBG_RM_NOCLAIM)))
>  		return false;
> @@ -351,6 +348,32 @@ check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
>  	return true;
>  }
>  
> +static bool
> +vlv_check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
> +{
> +	u32 cer;
> +
> +	cer = __raw_i915_read32(dev_priv, CLAIM_ER);
> +	if (likely(!(cer & (CLAIM_ER_OVERFLOW | CLAIM_ER_CTR_MASK))))
> +		return false;
> +
> +	__raw_i915_write32(dev_priv, CLAIM_ER, CLAIM_ER_CLR);
> +
> +	return true;
> +}
> +
> +static bool
> +check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
> +{
> +	if (HAS_FPGA_DBG_UNCLAIMED(dev_priv))
> +		return fpga_check_for_unclaimed_mmio(dev_priv);
> +
> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> +		return vlv_check_for_unclaimed_mmio(dev_priv);
> +
> +	return false;
> +}
> +
>  static void __intel_uncore_early_sanitize(struct drm_device *dev,
>  					  bool restore_forcewake)
>  {
> -- 
> 2.5.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/7] drm/i915: Do one shot unclaimed mmio detection less frequently

[Mika Kuoppala]

We have done unclaimed register access check in normal
(mmio_debug=0) mode once per write. This adds probability
of finding the exact sequence where we did the bad access, but
also adds burden to each write.

As we have mmio_debug available for more fine grained analysis,
give up accuracy of detecting correct spot at the first occurrence
by doing the one shot detection and arming of mmio_debug in hangcheck
and in modeset. This removes the write path performance burden.

v2: Remove gratuitous DRM_DEBUG and return value, comments (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Paulo Zanoni <przanoni@gmail.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h      |  3 +++
 drivers/gpu/drm/i915/i915_irq.c      | 11 ++++++-----
 drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
 drivers/gpu/drm/i915/intel_uncore.c  | 37 ++++++++++++++++++------------------
 4 files changed, 40 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 82c43b6..554092e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -723,6 +723,8 @@ struct intel_uncore {
 		i915_reg_t reg_post;
 		u32 val_reset;
 	} fw_domain[FW_DOMAIN_ID_COUNT];
+
+	int unclaimed_mmio_check;
 };
 
 /* Iterate over initialised fw domains */
@@ -2732,6 +2734,7 @@ extern void intel_uncore_early_sanitize(struct drm_device *dev,
 					bool restore_forcewake);
 extern void intel_uncore_init(struct drm_device *dev);
 extern bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv);
+extern void intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv);
 extern void intel_uncore_fini(struct drm_device *dev);
 extern void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore);
 const char *intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a20dc64..5128422 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2165,11 +2165,6 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 	if (!intel_irqs_enabled(dev_priv))
 		return IRQ_NONE;
 
-	/* We get interrupts on unclaimed registers, so check for this before we
-	 * do any I915_{READ,WRITE}. */
-	if (intel_uncore_unclaimed_mmio(dev_priv))
-		DRM_ERROR("Unclaimed register before interrupt\n");
-
 	/* disable master interrupt before clearing iir  */
 	de_ier = I915_READ(DEIER);
 	I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
@@ -2990,6 +2985,12 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
 	if (!i915.enable_hangcheck)
 		return;
 
+	/* As enabling the GPU requires fairly extensive mmio access,
+	 * periodically arm the mmio checker to see if we are triggering
+	 * any invalid access.
+	 */
+	intel_uncore_arm_unclaimed_mmio_detection(dev_priv);
+
 	for_each_ring(ring, dev_priv, i) {
 		u64 acthd;
 		u32 seqno;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 471f120..7dcabda 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13545,6 +13545,19 @@ static int intel_atomic_commit(struct drm_device *dev,
 
 	drm_atomic_state_free(state);
 
+	/* As one of the primary mmio accessors, KMS has a high likelihood
+	 * of triggering bugs in unclaimed access. After we finish
+	 * modesetting, see if an error has been flagged, and if so
+	 * enable debugging for the next modeset - and hope we catch
+	 * the culprit.
+	 *
+	 * XXX note that we assume display power is on at this point.
+	 * This might hold true now but we need to add pm helper to check
+	 * unclaimed only when the hardware is on, as atomic commits
+	 * can happen also when the device is completely off.
+	 */
+	intel_uncore_arm_unclaimed_mmio_detection(dev_priv);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 3d8af69..51d3036 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -626,22 +626,6 @@ hsw_unclaimed_reg_debug(struct drm_i915_private *dev_priv,
 	}
 }
 
-static void
-hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
-{
-	static bool mmio_debug_once = true;
-
-	if (i915.mmio_debug || !mmio_debug_once)
-		return;
-
-	if (check_for_unclaimed_mmio(dev_priv)) {
-		DRM_DEBUG("Unclaimed register detected, "
-			  "enabling oneshot unclaimed register reporting. "
-			  "Please use i915.mmio_debug=N for more information.\n");
-		i915.mmio_debug = mmio_debug_once--;
-	}
-}
-
 #define GEN2_READ_HEADER(x) \
 	u##x val = 0; \
 	assert_device_not_suspended(dev_priv);
@@ -921,7 +905,6 @@ hsw_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, bool t
 		gen6_gt_check_fifodbg(dev_priv); \
 	} \
 	hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
-	hsw_unclaimed_reg_detect(dev_priv); \
 	GEN6_WRITE_FOOTER; \
 }
 
@@ -956,7 +939,6 @@ gen8_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, bool
 		__force_wake_get(dev_priv, FORCEWAKE_RENDER); \
 	__raw_i915_write##x(dev_priv, reg, val); \
 	hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
-	hsw_unclaimed_reg_detect(dev_priv); \
 	GEN6_WRITE_FOOTER; \
 }
 
@@ -1026,7 +1008,6 @@ gen9_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, \
 		__force_wake_get(dev_priv, fw_engine); \
 	__raw_i915_write##x(dev_priv, reg, val); \
 	hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
-	hsw_unclaimed_reg_detect(dev_priv); \
 	GEN6_WRITE_FOOTER; \
 }
 
@@ -1246,6 +1227,8 @@ void intel_uncore_init(struct drm_device *dev)
 	intel_uncore_fw_domains_init(dev);
 	__intel_uncore_early_sanitize(dev, false);
 
+	dev_priv->uncore.unclaimed_mmio_check = 1;
+
 	switch (INTEL_INFO(dev)->gen) {
 	default:
 	case 9:
@@ -1607,3 +1590,19 @@ bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv)
 {
 	return check_for_unclaimed_mmio(dev_priv);
 }
+
+void
+intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv)
+{
+	if (unlikely(i915.mmio_debug ||
+		     dev_priv->uncore.unclaimed_mmio_check <= 0))
+		return;
+
+	if (unlikely(intel_uncore_unclaimed_mmio(dev_priv))) {
+		DRM_DEBUG("Unclaimed register detected, "
+			  "enabling oneshot unclaimed register reporting. "
+			  "Please use i915.mmio_debug=N for more information.\n");
+		i915.mmio_debug++;
+		dev_priv->uncore.unclaimed_mmio_check--;
+	}
+}
-- 
2.5.0

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

Re: [PATCH 1/7] drm/i915: Consolidate unclaimed mmio detection

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> On Tue, Dec 15, 2015 at 04:25:06PM +0200, Mika Kuoppala wrote:
>> Access the unclaimed reg detection register through
>> one helper which also does cleanup. Note that we now access
>> the register only if the platform has the actual non claimed
>> access bit. This prevents reading the register with gens that
>> doesn't have the register or the unclaimed bit,
>> when debug_mmio > 0.
>> 
>> Note that we post after clearing the bit. This makes sure
>> that the next unclaimed write access would get detected
>> also if it happened right after clearing, and not fold
>> into the previous detection.
>
> I didn't like 6 (adding checks to rpm get/put), and I didn't see the
> removal of the check from the interrupt? Or adding it to the modesetting
> code?
>
> Did I just miss those?
>
> The other patches (1-5?) (aside from adding chv as I haven't validated the
> registers myself) are
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

1-5 queued for -next. Thanks for review.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/7] drm/i915: Add non claimed mmio checking for vlv/chv

[Mika Kuoppala]

Ville Syrjälä <ville.syrjala@linux.intel.com> writes:

> On Tue, Dec 15, 2015 at 07:45:42PM +0200, Mika Kuoppala wrote:
>> Imre mentioned that chv might also have capability to
>> track unclaimed mmio accesses. Ville added that
>> both chv and vlv has this capability and he had already
>> made this way back [1]. Mimic what Ville's patch does
>> but adapt on top of less frequent mmio accesses by
>> omitting checking always on reg writes.
>> 
>> This patch is untested as of now.
>> 
>> v2: overflow handling and posting omitted (Ville)
>> 
>> References: [1] http://lists.freedesktop.org/archives/intel-gfx/2013-May/027599.html
>> Cc: Imre Deak <imre.deak@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>
> lgtm
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Queued for -next. Thanks for review.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx