[PATCH 0/5] BDW unclaimed registers

[PATCH 0/5] BDW unclaimed registers

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Hi

Even though this feature is super useful for hardware enabling, we ended up not
enabling it on BDW, so we still silently hit some unclaimed registers on this
platform. This series first fixes the bugs fround by the feature, then
introduces the feature later. It also introduces some rework on the unclaimed
register code, and I'd like to hear opinions and suggestsions about it.

Also notice that a patch to detect unclaimed registers on BDW was also sent to
this list on Feb 21.

I tested these patches briefly before my BDW machine died. They worked at that
time, so I hope they keep working now. Anyway, reviewers and QA should catch
anything wrong with these, right? :)

Thanks,
Paulo

Paulo Zanoni (5):
  drm/i915: don't write powered down IRQ registers on Gen 8
  drm/i915: HSW_BLC_PWM2_CTL doesn't exist on BDW
  drm/i915: extract and improve gen8_irq_power_well_post_enable
  drm/i915: reorganize the unclaimed register detection code
  drm/i915: BDW can also detect unclaimed registers

 drivers/gpu/drm/i915/i915_drv.c      |  4 ++
 drivers/gpu/drm/i915/i915_drv.h      |  1 +
 drivers/gpu/drm/i915/i915_irq.c      | 23 ++++++++++--
 drivers/gpu/drm/i915/i915_params.c   |  6 +++
 drivers/gpu/drm/i915/intel_display.c |  5 ++-
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_pm.c      | 18 +--------
 drivers/gpu/drm/i915/intel_uncore.c  | 71 ++++++++++++++++++++++++++++++++----
 8 files changed, 101 insertions(+), 28 deletions(-)

-- 
2.0.0

[PATCH 1/5] drm/i915: don't write powered down IRQ registers on Gen 8

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

If we enable unclaimed register reporting on Gen 8, we will discover
that the IRQ registers for pipes B and C are also on the power well,
so writes to them when the power well is disabled result in unclaimed
register errors.

Also, hsw_power_well_post_enable() already takes care of re-enabling
them once the power well is enabled.

Testcase: igt/pm_rpm/rte
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 1c1ec22..2e116e9d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3193,7 +3193,9 @@ static void gen8_irq_reset(struct drm_device *dev)
 	gen8_gt_irq_reset(dev_priv);
 
 	for_each_pipe(pipe)
-		GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
+		if (intel_display_power_enabled(dev_priv,
+						POWER_DOMAIN_PIPE(pipe)))
+			GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
 
 	GEN5_IRQ_RESET(GEN8_DE_PORT_);
 	GEN5_IRQ_RESET(GEN8_DE_MISC_);
@@ -3526,8 +3528,11 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
 	dev_priv->de_irq_mask[PIPE_C] = ~de_pipe_masked;
 
 	for_each_pipe(pipe)
-		GEN8_IRQ_INIT_NDX(DE_PIPE, pipe, dev_priv->de_irq_mask[pipe],
-				  de_pipe_enables);
+		if (intel_display_power_enabled(dev_priv,
+				POWER_DOMAIN_PIPE(pipe)))
+			GEN8_IRQ_INIT_NDX(DE_PIPE, pipe,
+					  dev_priv->de_irq_mask[pipe],
+					  de_pipe_enables);
 
 	GEN5_IRQ_INIT(GEN8_DE_PORT_, ~GEN8_AUX_CHANNEL_A, GEN8_AUX_CHANNEL_A);
 }
-- 
2.0.0

[PATCH 2/5] drm/i915: HSW_BLC_PWM2_CTL doesn't exist on BDW

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

So don't write it, otherwise we will trigger unclaimed register
errors.

Testcase: igt/pm_rpm/rte
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c12a5da..14505a1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7345,8 +7345,9 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
 	WARN(I915_READ(PCH_PP_STATUS) & PP_ON, "Panel power on\n");
 	WARN(I915_READ(BLC_PWM_CPU_CTL2) & BLM_PWM_ENABLE,
 	     "CPU PWM1 enabled\n");
-	WARN(I915_READ(HSW_BLC_PWM2_CTL) & BLM_PWM_ENABLE,
-	     "CPU PWM2 enabled\n");
+	if (IS_HASWELL(dev))
+		WARN(I915_READ(HSW_BLC_PWM2_CTL) & BLM_PWM_ENABLE,
+		     "CPU PWM2 enabled\n");
 	WARN(I915_READ(BLC_PWM_PCH_CTL1) & BLM_PCH_PWM_ENABLE,
 	     "PCH PWM1 enabled\n");
 	WARN(I915_READ(UTIL_PIN_CTL) & UTIL_PIN_ENABLE,
-- 
2.0.0

[PATCH 3/5] drm/i915: extract and improve gen8_irq_power_well_post_enable

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Move it from hsw_power_well_post_enable() (intel_pm.c) to i915_irq.c
so we can reuse the nice IRQ macros we have there. The main difference
is that now we're going to check if the IIR register is non-zero when
we try to re-enable the interrupts.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c  | 12 ++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c  | 18 ++----------------
 3 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 2e116e9d..a8b8b6b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3204,6 +3204,18 @@ static void gen8_irq_reset(struct drm_device *dev)
 	ibx_irq_reset(dev);
 }
 
+void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv)
+{
+	unsigned long irqflags;
+
+	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+	GEN8_IRQ_INIT_NDX(DE_PIPE, PIPE_B, dev_priv->de_irq_mask[PIPE_B],
+			  ~dev_priv->de_irq_mask[PIPE_B]);
+	GEN8_IRQ_INIT_NDX(DE_PIPE, PIPE_C, dev_priv->de_irq_mask[PIPE_C],
+			  ~dev_priv->de_irq_mask[PIPE_C]);
+	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+}
+
 static void cherryview_irq_preinstall(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5f7c7bd..46a3a09 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -687,6 +687,7 @@ void intel_runtime_pm_disable_interrupts(struct drm_device *dev);
 void intel_runtime_pm_restore_interrupts(struct drm_device *dev);
 int intel_get_crtc_scanline(struct intel_crtc *crtc);
 void i9xx_check_fifo_underruns(struct drm_device *dev);
+void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv);
 
 
 /* intel_crt.c */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 31ae2b4..4cc9e5c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5913,7 +5913,6 @@ bool intel_display_power_enabled(struct drm_i915_private *dev_priv,
 static void hsw_power_well_post_enable(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
-	unsigned long irqflags;
 
 	/*
 	 * After we re-enable the power well, if we touch VGA register 0x3d5
@@ -5929,21 +5928,8 @@ static void hsw_power_well_post_enable(struct drm_i915_private *dev_priv)
 	outb(inb(VGA_MSR_READ), VGA_MSR_WRITE);
 	vga_put(dev->pdev, VGA_RSRC_LEGACY_IO);
 
-	if (IS_BROADWELL(dev)) {
-		spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
-		I915_WRITE(GEN8_DE_PIPE_IMR(PIPE_B),
-			   dev_priv->de_irq_mask[PIPE_B]);
-		I915_WRITE(GEN8_DE_PIPE_IER(PIPE_B),
-			   ~dev_priv->de_irq_mask[PIPE_B] |
-			   GEN8_PIPE_VBLANK);
-		I915_WRITE(GEN8_DE_PIPE_IMR(PIPE_C),
-			   dev_priv->de_irq_mask[PIPE_C]);
-		I915_WRITE(GEN8_DE_PIPE_IER(PIPE_C),
-			   ~dev_priv->de_irq_mask[PIPE_C] |
-			   GEN8_PIPE_VBLANK);
-		POSTING_READ(GEN8_DE_PIPE_IER(PIPE_C));
-		spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
-	}
+	if (IS_BROADWELL(dev))
+		gen8_irq_power_well_post_enable(dev_priv);
 }
 
 static void hsw_set_power_well(struct drm_i915_private *dev_priv,
-- 
2.0.0

[PATCH 4/5] drm/i915: reorganize the unclaimed register detection code

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

The current code only runs when we do an I915_WRITE operation. It
checks if the unclaimed register flag is set before we do the
operation, and then it checks it again after we do the operation. This
double check allows us to find out if the I915_WRITE operation in
question is the bad one, or if some previous code is the bad one. When
it finds a problem, our code uses DRM_ERROR to signal it.

The good thing about the current code is that it detects the problem,
so at least we can know we did something wrong. The problem is that
even though we find the problem, we don't really have much information
to actually debug it. So whenever I see one of these DRM_ERROR
messages on my systems, the first thing I do is apply a patch to
change the DRM_ERROR to a WARN and also check for unclaimed registers
on I915_READ operations. This local patch makes things even slower,
but it usually helps a lot in finding the bad code.

The first point here is that since the current code is only useful to
detect whether we have a problem or not, but it is not really good to
find the cause of the problem, I don't think we should be checking
both before and after every I915_WRITE operation: just doing the check
once should be enough for us to quickly detect problems. With this
change, the code that runs by default for every single user will only
do 1 read operation for every single I915_WRITE, instead of 2. This
patch does this change.

The second point is that the local patch I have should be upstream,
but since it makes things slower it should be disabled by default. So
I added the i915.mmio_debug option to enable it.

So after this patch, this is what will happen:
 - By default, we will try to detect unclaimed registers once after
   every I915_WRITE operation. Previously we tried twice for every
   I915_WRITE.
 - When we find an unclaimed register we will still print a DRM_ERROR
   message, but we will now tell the user to try again with
   i915.mmio_debug=1.
 - When we use i915.mmio_debug=1 we will try to find unclaimed
   registers both before and after every I915_READ and I915_WRITE
   operation, and we will print stack traces in case we find them.
   This should really help locating the exact point of the bad code
   (or at least finding out that i915.ko is not the problem).

This commit also opens space for really-slow register debugging
operations on other platforms. In theory we can now add lots and lots
of debug code behind i915.mmio_debug, enable this option on our tests,
and catch more problems.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h     |  1 +
 drivers/gpu/drm/i915/i915_params.c  |  6 ++++
 drivers/gpu/drm/i915/intel_uncore.c | 68 +++++++++++++++++++++++++++++++++----
 3 files changed, 68 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ac06c0f..51d867f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2092,6 +2092,7 @@ struct i915_params {
 	bool disable_display;
 	bool disable_vtd_wa;
 	int use_mmio_flip;
+	bool mmio_debug;
 };
 extern struct i915_params i915 __read_mostly;
 
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 8145729..7977872 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -49,6 +49,7 @@ struct i915_params i915 __read_mostly = {
 	.enable_cmd_parser = 1,
 	.disable_vtd_wa = 0,
 	.use_mmio_flip = 0,
+	.mmio_debug = 0,
 };
 
 module_param_named(modeset, i915.modeset, int, 0400);
@@ -161,3 +162,8 @@ MODULE_PARM_DESC(enable_cmd_parser,
 module_param_named(use_mmio_flip, i915.use_mmio_flip, int, 0600);
 MODULE_PARM_DESC(use_mmio_flip,
 		 "use MMIO flips (-1=never, 0=driver discretion [default], 1=always)");
+
+module_param_named(mmio_debug, i915.mmio_debug, bool, 0600);
+MODULE_PARM_DESC(disable_vtd_wa,
+	"Enable the MMIO debug code (default: false). This may negatively "
+	"affect performance.");
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 29145df..de5402f 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -513,21 +513,72 @@ ilk_dummy_write(struct drm_i915_private *dev_priv)
 	__raw_i915_write32(dev_priv, MI_MODE, 0);
 }
 
+/**
+ * hsw_unclaimed_reg_debug - tries to find unclaimed registers
+ * @dev_priv: device private data
+ * @reg: register we're about to touch or just have touched
+ * @read: are we reading or writing a register now?
+ * @before: are we running this before or after we touch the register?
+ *
+ * This function tries to detect unclaimed registers and provide as much
+ * information as possible. It will only do something if the i915.mmio_debug
+ * option is enabled.
+ *
+ * If we detect an unclaimed register when @before is true, it means some
+ * unknown code wrote to an unclaimed register, and we're just detecting it now.
+ * The bad code can be i915.ko, some other Kernel module (e.g., snd_hda_intel,
+ * vgacon) or even user space. If we detect it when @before is false, then
+ * there's a really good chance that the read/write operation we just did to
+ * @reg is what triggered the unclaimed register message, but there's also the
+ * possibility that after the operation we did, and before this function,
+ * something else touched another unclaimed register, and we are detecting it
+ * now.
+ *
+ * The unclaimed register bit can get set when we touch a register that does not
+ * exist and when we touch a register that exists but is powered down. Please
+ * also notice that the HW can only check some register ranges, so there's no
+ * guarantee that all read and write operations to inexistent registers will be
+ * caught by this function.
+ *
+ * Also please notice that we don't run this function on __raw operations and in
+ * many other cases, so the case where @before is true is quite common.
+ */
 static void
-hsw_unclaimed_reg_clear(struct drm_i915_private *dev_priv, u32 reg)
+hsw_unclaimed_reg_debug(struct drm_i915_private *dev_priv, u32 reg, bool read,
+			bool before)
 {
+	const char *op = read ? "reading" : "writing to";
+	const char *when = before ? "before" : "after";
+
+	if (!i915.mmio_debug)
+		return;
+
 	if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
-		DRM_ERROR("Unknown unclaimed register before writing to %x\n",
-			  reg);
+		WARN(1, "Unclaimed register detected %s %s register 0x%x\n",
+		     when, op, reg);
 		__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
 	}
 }
 
+/**
+ * hsw_unclaimed_reg_detect - tries to find unclaimed registers
+ * @dev_priv: device private data
+ *
+ * This function will try to detect if something touched and unclaimed register,
+ * triggering the FPGA_DBG bit. If this happens, we will print a message telling
+ * the user to use i915.mmio_debug=1 so we can properly debug the problem.
+ *
+ * This way, we can still detect our bugs without giving the user the
+ * performance impact of hsw_unclaimed_reg_debug().
+ */
 static void
-hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg)
+hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
 {
+	if (i915.mmio_debug)
+		return;
+
 	if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
-		DRM_ERROR("Unclaimed write to %x\n", reg);
+		DRM_ERROR("Unclaimed register detected. Please use the i915.mmio_debug=1 to debug this problem.");
 		__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
 	}
 }
@@ -564,6 +615,7 @@ gen5_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
 static u##x \
 gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
 	REG_READ_HEADER(x); \
+	hsw_unclaimed_reg_debug(dev_priv, reg, true, true); \
 	if (dev_priv->uncore.forcewake_count == 0 && \
 	    NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
 		dev_priv->uncore.funcs.force_wake_get(dev_priv, \
@@ -574,6 +626,7 @@ gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
 	} else { \
 		val = __raw_i915_read##x(dev_priv, reg); \
 	} \
+	hsw_unclaimed_reg_debug(dev_priv, reg, true, false); \
 	REG_READ_FOOTER; \
 }
 
@@ -700,12 +753,13 @@ hsw_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace)
 	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
 		__fifo_ret = __gen6_gt_wait_for_fifo(dev_priv); \
 	} \
-	hsw_unclaimed_reg_clear(dev_priv, reg); \
+	hsw_unclaimed_reg_debug(dev_priv, reg, false, true); \
 	__raw_i915_write##x(dev_priv, reg, val); \
 	if (unlikely(__fifo_ret)) { \
 		gen6_gt_check_fifodbg(dev_priv); \
 	} \
-	hsw_unclaimed_reg_check(dev_priv, reg); \
+	hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
+	hsw_unclaimed_reg_detect(dev_priv); \
 	REG_WRITE_FOOTER; \
 }
 
-- 
2.0.0

[PATCH 5/5] drm/i915: BDW can also detect unclaimed registers

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

By the time I wrote this patch, it allowed me to catch some problems.
But due to patch reordering - in order to prevent fake "regression"
reports - this patch may be merged after the fixes of the problems
identified by this patch.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c     | 4 ++++
 drivers/gpu/drm/i915/intel_uncore.c | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 8a0cb0c..bdb223c 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -303,6 +303,7 @@ static const struct intel_device_info intel_broadwell_d_info = {
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
 	.has_llc = 1,
 	.has_ddi = 1,
+	.has_fpga_dbg = 1,
 	.has_fbc = 1,
 	GEN_DEFAULT_PIPEOFFSETS,
 	IVB_CURSOR_OFFSETS,
@@ -314,6 +315,7 @@ static const struct intel_device_info intel_broadwell_m_info = {
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
 	.has_llc = 1,
 	.has_ddi = 1,
+	.has_fpga_dbg = 1,
 	.has_fbc = 1,
 	GEN_DEFAULT_PIPEOFFSETS,
 	IVB_CURSOR_OFFSETS,
@@ -325,6 +327,7 @@ static const struct intel_device_info intel_broadwell_gt3d_info = {
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
 	.has_llc = 1,
 	.has_ddi = 1,
+	.has_fpga_dbg = 1,
 	.has_fbc = 1,
 	GEN_DEFAULT_PIPEOFFSETS,
 	IVB_CURSOR_OFFSETS,
@@ -336,6 +339,7 @@ static const struct intel_device_info intel_broadwell_gt3m_info = {
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
 	.has_llc = 1,
 	.has_ddi = 1,
+	.has_fpga_dbg = 1,
 	.has_fbc = 1,
 	GEN_DEFAULT_PIPEOFFSETS,
 	IVB_CURSOR_OFFSETS,
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index de5402f..1fcf78b 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -788,6 +788,7 @@ static bool is_gen8_shadowed(struct drm_i915_private *dev_priv, u32 reg)
 static void \
 gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
 	REG_WRITE_HEADER; \
+	hsw_unclaimed_reg_debug(dev_priv, reg, false, true); \
 	if (reg < 0x40000 && !is_gen8_shadowed(dev_priv, reg)) { \
 		if (dev_priv->uncore.forcewake_count == 0) \
 			dev_priv->uncore.funcs.force_wake_get(dev_priv,	\
@@ -799,6 +800,8 @@ gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace
 	} else { \
 		__raw_i915_write##x(dev_priv, reg, val); \
 	} \
+	hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
+	hsw_unclaimed_reg_detect(dev_priv); \
 	REG_WRITE_FOOTER; \
 }
 
-- 
2.0.0

Re: [PATCH 1/5] drm/i915: don't write powered down IRQ registers on Gen 8

[Daniel Vetter]

On Fri, Jul 04, 2014 at 11:50:29AM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> If we enable unclaimed register reporting on Gen 8, we will discover
> that the IRQ registers for pipes B and C are also on the power well,
> so writes to them when the power well is disabled result in unclaimed
> register errors.
> 
> Also, hsw_power_well_post_enable() already takes care of re-enabling
> them once the power well is enabled.
> 
> Testcase: igt/pm_rpm/rte
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Hm, shouldn't we split this into only setting up pipe A here and the pipe
B stuff once we fire up the power well?

I just want to avoid duplicating logic all over the place ...
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 1c1ec22..2e116e9d 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3193,7 +3193,9 @@ static void gen8_irq_reset(struct drm_device *dev)
>  	gen8_gt_irq_reset(dev_priv);
>  
>  	for_each_pipe(pipe)
> -		GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
> +		if (intel_display_power_enabled(dev_priv,
> +						POWER_DOMAIN_PIPE(pipe)))
> +			GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
>  
>  	GEN5_IRQ_RESET(GEN8_DE_PORT_);
>  	GEN5_IRQ_RESET(GEN8_DE_MISC_);
> @@ -3526,8 +3528,11 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
>  	dev_priv->de_irq_mask[PIPE_C] = ~de_pipe_masked;
>  
>  	for_each_pipe(pipe)
> -		GEN8_IRQ_INIT_NDX(DE_PIPE, pipe, dev_priv->de_irq_mask[pipe],
> -				  de_pipe_enables);
> +		if (intel_display_power_enabled(dev_priv,
> +				POWER_DOMAIN_PIPE(pipe)))
> +			GEN8_IRQ_INIT_NDX(DE_PIPE, pipe,
> +					  dev_priv->de_irq_mask[pipe],
> +					  de_pipe_enables);
>  
>  	GEN5_IRQ_INIT(GEN8_DE_PORT_, ~GEN8_AUX_CHANNEL_A, GEN8_AUX_CHANNEL_A);
>  }
> -- 
> 2.0.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/5] drm/i915: reorganize the unclaimed register detection code

[Daniel Vetter]

On Fri, Jul 04, 2014 at 11:50:32AM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> The current code only runs when we do an I915_WRITE operation. It
> checks if the unclaimed register flag is set before we do the
> operation, and then it checks it again after we do the operation. This
> double check allows us to find out if the I915_WRITE operation in
> question is the bad one, or if some previous code is the bad one. When
> it finds a problem, our code uses DRM_ERROR to signal it.
> 
> The good thing about the current code is that it detects the problem,
> so at least we can know we did something wrong. The problem is that
> even though we find the problem, we don't really have much information
> to actually debug it. So whenever I see one of these DRM_ERROR
> messages on my systems, the first thing I do is apply a patch to
> change the DRM_ERROR to a WARN and also check for unclaimed registers
> on I915_READ operations. This local patch makes things even slower,
> but it usually helps a lot in finding the bad code.
> 
> The first point here is that since the current code is only useful to
> detect whether we have a problem or not, but it is not really good to
> find the cause of the problem, I don't think we should be checking
> both before and after every I915_WRITE operation: just doing the check
> once should be enough for us to quickly detect problems. With this
> change, the code that runs by default for every single user will only
> do 1 read operation for every single I915_WRITE, instead of 2. This
> patch does this change.
> 
> The second point is that the local patch I have should be upstream,
> but since it makes things slower it should be disabled by default. So
> I added the i915.mmio_debug option to enable it.
> 
> So after this patch, this is what will happen:
>  - By default, we will try to detect unclaimed registers once after
>    every I915_WRITE operation. Previously we tried twice for every
>    I915_WRITE.
>  - When we find an unclaimed register we will still print a DRM_ERROR
>    message, but we will now tell the user to try again with
>    i915.mmio_debug=1.
>  - When we use i915.mmio_debug=1 we will try to find unclaimed
>    registers both before and after every I915_READ and I915_WRITE
>    operation, and we will print stack traces in case we find them.
>    This should really help locating the exact point of the bad code
>    (or at least finding out that i915.ko is not the problem).
> 
> This commit also opens space for really-slow register debugging
> operations on other platforms. In theory we can now add lots and lots
> of debug code behind i915.mmio_debug, enable this option on our tests,
> and catch more problems.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h     |  1 +
>  drivers/gpu/drm/i915/i915_params.c  |  6 ++++
>  drivers/gpu/drm/i915/intel_uncore.c | 68 +++++++++++++++++++++++++++++++++----
>  3 files changed, 68 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ac06c0f..51d867f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2092,6 +2092,7 @@ struct i915_params {
>  	bool disable_display;
>  	bool disable_vtd_wa;
>  	int use_mmio_flip;
> +	bool mmio_debug;
>  };
>  extern struct i915_params i915 __read_mostly;
>  
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 8145729..7977872 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -49,6 +49,7 @@ struct i915_params i915 __read_mostly = {
>  	.enable_cmd_parser = 1,
>  	.disable_vtd_wa = 0,
>  	.use_mmio_flip = 0,
> +	.mmio_debug = 0,
>  };
>  
>  module_param_named(modeset, i915.modeset, int, 0400);
> @@ -161,3 +162,8 @@ MODULE_PARM_DESC(enable_cmd_parser,
>  module_param_named(use_mmio_flip, i915.use_mmio_flip, int, 0600);
>  MODULE_PARM_DESC(use_mmio_flip,
>  		 "use MMIO flips (-1=never, 0=driver discretion [default], 1=always)");
> +
> +module_param_named(mmio_debug, i915.mmio_debug, bool, 0600);
> +MODULE_PARM_DESC(disable_vtd_wa,
> +	"Enable the MMIO debug code (default: false). This may negatively "
> +	"affect performance.");
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 29145df..de5402f 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -513,21 +513,72 @@ ilk_dummy_write(struct drm_i915_private *dev_priv)
>  	__raw_i915_write32(dev_priv, MI_MODE, 0);
>  }
>  
> +/**
> + * hsw_unclaimed_reg_debug - tries to find unclaimed registers
> + * @dev_priv: device private data
> + * @reg: register we're about to touch or just have touched
> + * @read: are we reading or writing a register now?
> + * @before: are we running this before or after we touch the register?
> + *
> + * This function tries to detect unclaimed registers and provide as much
> + * information as possible. It will only do something if the i915.mmio_debug
> + * option is enabled.
> + *
> + * If we detect an unclaimed register when @before is true, it means some
> + * unknown code wrote to an unclaimed register, and we're just detecting it now.
> + * The bad code can be i915.ko, some other Kernel module (e.g., snd_hda_intel,
> + * vgacon) or even user space. If we detect it when @before is false, then
> + * there's a really good chance that the read/write operation we just did to
> + * @reg is what triggered the unclaimed register message, but there's also the
> + * possibility that after the operation we did, and before this function,
> + * something else touched another unclaimed register, and we are detecting it
> + * now.
> + *
> + * The unclaimed register bit can get set when we touch a register that does not
> + * exist and when we touch a register that exists but is powered down. Please
> + * also notice that the HW can only check some register ranges, so there's no
> + * guarantee that all read and write operations to inexistent registers will be
> + * caught by this function.
> + *
> + * Also please notice that we don't run this function on __raw operations and in
> + * many other cases, so the case where @before is true is quite common.
> + */

Pet peeve of mine: Comments have a cost since they get stale really fast
and then can be awfully misleading. We have way too many examples for
that. My rule of interface kerneldoc for functions is that we should only
have it for exported functions used in multiple different places in the
driver. If there's only one caller (like for _init/fini) functions the
comment should be very terse at most.

For static inline helpers in the same file otoh code should simply be
readable enough to not require comments. Imo this is the case here, so the
kerneldoc should imo be dropped.

If you want to keep some of it I guess we could document i915.mmio_debug
with kerneldoc and then also pull it into our i915 section in the drm
docbook. Maybe under a new "debug options" setting. We probably need a
DOC: section to make this work. A few sentences should be more than
enough.

>  static void
> -hsw_unclaimed_reg_clear(struct drm_i915_private *dev_priv, u32 reg)
> +hsw_unclaimed_reg_debug(struct drm_i915_private *dev_priv, u32 reg, bool read,
> +			bool before)
>  {
> +	const char *op = read ? "reading" : "writing to";
> +	const char *when = before ? "before" : "after";
> +
> +	if (!i915.mmio_debug)
> +		return;
> +
>  	if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
> -		DRM_ERROR("Unknown unclaimed register before writing to %x\n",
> -			  reg);
> +		WARN(1, "Unclaimed register detected %s %s register 0x%x\n",
> +		     when, op, reg);
>  		__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
>  	}
>  }
>  
> +/**
> + * hsw_unclaimed_reg_detect - tries to find unclaimed registers
> + * @dev_priv: device private data
> + *
> + * This function will try to detect if something touched and unclaimed register,
> + * triggering the FPGA_DBG bit. If this happens, we will print a message telling
> + * the user to use i915.mmio_debug=1 so we can properly debug the problem.
> + *
> + * This way, we can still detect our bugs without giving the user the
> + * performance impact of hsw_unclaimed_reg_debug().
> + */
>  static void
> -hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg)
> +hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
>  {
> +	if (i915.mmio_debug)
> +		return;
> +
>  	if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
> -		DRM_ERROR("Unclaimed write to %x\n", reg);
> +		DRM_ERROR("Unclaimed register detected. Please use the i915.mmio_debug=1 to debug this problem.");

But imo really the only piece that's required is this helpful message here
into dmesg.

Anyway, the concept looks solid.
-Daniel

>  		__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
>  	}
>  }
> @@ -564,6 +615,7 @@ gen5_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
>  static u##x \
>  gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
>  	REG_READ_HEADER(x); \
> +	hsw_unclaimed_reg_debug(dev_priv, reg, true, true); \
>  	if (dev_priv->uncore.forcewake_count == 0 && \
>  	    NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
>  		dev_priv->uncore.funcs.force_wake_get(dev_priv, \
> @@ -574,6 +626,7 @@ gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
>  	} else { \
>  		val = __raw_i915_read##x(dev_priv, reg); \
>  	} \
> +	hsw_unclaimed_reg_debug(dev_priv, reg, true, false); \
>  	REG_READ_FOOTER; \
>  }
>  
> @@ -700,12 +753,13 @@ hsw_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace)
>  	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
>  		__fifo_ret = __gen6_gt_wait_for_fifo(dev_priv); \
>  	} \
> -	hsw_unclaimed_reg_clear(dev_priv, reg); \
> +	hsw_unclaimed_reg_debug(dev_priv, reg, false, true); \
>  	__raw_i915_write##x(dev_priv, reg, val); \
>  	if (unlikely(__fifo_ret)) { \
>  		gen6_gt_check_fifodbg(dev_priv); \
>  	} \
> -	hsw_unclaimed_reg_check(dev_priv, reg); \
> +	hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
> +	hsw_unclaimed_reg_detect(dev_priv); \
>  	REG_WRITE_FOOTER; \
>  }
>  
> -- 
> 2.0.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/5] drm/i915: don't write powered down IRQ registers on Gen 8

[Paulo Zanoni]

2014-07-07 18:23 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> On Fri, Jul 04, 2014 at 11:50:29AM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> If we enable unclaimed register reporting on Gen 8, we will discover
>> that the IRQ registers for pipes B and C are also on the power well,
>> so writes to them when the power well is disabled result in unclaimed
>> register errors.
>>
>> Also, hsw_power_well_post_enable() already takes care of re-enabling
>> them once the power well is enabled.
>>
>> Testcase: igt/pm_rpm/rte
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Hm, shouldn't we split this into only setting up pipe A here and the pipe
> B stuff once we fire up the power well?
>

No because these functions might be called when the power wells are
already enabled.


> I just want to avoid duplicating logic all over the place ...
> -Daniel
>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index 1c1ec22..2e116e9d 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -3193,7 +3193,9 @@ static void gen8_irq_reset(struct drm_device *dev)
>>       gen8_gt_irq_reset(dev_priv);
>>
>>       for_each_pipe(pipe)
>> -             GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
>> +             if (intel_display_power_enabled(dev_priv,
>> +                                             POWER_DOMAIN_PIPE(pipe)))
>> +                     GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
>>
>>       GEN5_IRQ_RESET(GEN8_DE_PORT_);
>>       GEN5_IRQ_RESET(GEN8_DE_MISC_);
>> @@ -3526,8 +3528,11 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
>>       dev_priv->de_irq_mask[PIPE_C] = ~de_pipe_masked;
>>
>>       for_each_pipe(pipe)
>> -             GEN8_IRQ_INIT_NDX(DE_PIPE, pipe, dev_priv->de_irq_mask[pipe],
>> -                               de_pipe_enables);
>> +             if (intel_display_power_enabled(dev_priv,
>> +                             POWER_DOMAIN_PIPE(pipe)))
>> +                     GEN8_IRQ_INIT_NDX(DE_PIPE, pipe,
>> +                                       dev_priv->de_irq_mask[pipe],
>> +                                       de_pipe_enables);
>>
>>       GEN5_IRQ_INIT(GEN8_DE_PORT_, ~GEN8_AUX_CHANNEL_A, GEN8_AUX_CHANNEL_A);
>>  }
>> --
>> 2.0.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

Re: [PATCH 1/5] drm/i915: don't write powered down IRQ registers on Gen 8

[Daniel Vetter]

On Tue, Jul 08, 2014 at 11:15:03AM -0300, Paulo Zanoni wrote:
> 2014-07-07 18:23 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> > On Fri, Jul 04, 2014 at 11:50:29AM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> If we enable unclaimed register reporting on Gen 8, we will discover
> >> that the IRQ registers for pipes B and C are also on the power well,
> >> so writes to them when the power well is disabled result in unclaimed
> >> register errors.
> >>
> >> Also, hsw_power_well_post_enable() already takes care of re-enabling
> >> them once the power well is enabled.
> >>
> >> Testcase: igt/pm_rpm/rte
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > Hm, shouldn't we split this into only setting up pipe A here and the pipe
> > B stuff once we fire up the power well?
> >
> 
> No because these functions might be called when the power wells are
> already enabled.

Hm, where does this still happen? bdw has power well support and chv has a
different display block ...

This code changed too often and I have no idea any more what's up and
what's down here ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/5] drm/i915: don't write powered down IRQ registers on Gen 8

[Paulo Zanoni]

2014-07-08 11:58 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> On Tue, Jul 08, 2014 at 11:15:03AM -0300, Paulo Zanoni wrote:
>> 2014-07-07 18:23 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
>> > On Fri, Jul 04, 2014 at 11:50:29AM -0300, Paulo Zanoni wrote:
>> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >>
>> >> If we enable unclaimed register reporting on Gen 8, we will discover
>> >> that the IRQ registers for pipes B and C are also on the power well,
>> >> so writes to them when the power well is disabled result in unclaimed
>> >> register errors.
>> >>
>> >> Also, hsw_power_well_post_enable() already takes care of re-enabling
>> >> them once the power well is enabled.
>> >>
>> >> Testcase: igt/pm_rpm/rte
>> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >
>> > Hm, shouldn't we split this into only setting up pipe A here and the pipe
>> > B stuff once we fire up the power well?
>> >
>>
>> No because these functions might be called when the power wells are
>> already enabled.
>
> Hm, where does this still happen? bdw has power well support and chv has a
> different display block ...

At driver init time... If you load i915.ko and the power wells are
already enabled, we have to do it here.

>
> This code changed too often and I have no idea any more what's up and
> what's down here ;-)
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

Re: [PATCH 1/5] drm/i915: don't write powered down IRQ registers on Gen 8

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 1894 bytes --]

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


On Thu, Jul 10, 2014 at 12:31 PM, Paulo Zanoni <przanoni@gmail.com> wrote:

> 2014-07-08 11:58 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> > On Tue, Jul 08, 2014 at 11:15:03AM -0300, Paulo Zanoni wrote:
> >> 2014-07-07 18:23 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> >> > On Fri, Jul 04, 2014 at 11:50:29AM -0300, Paulo Zanoni wrote:
> >> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> >>
> >> >> If we enable unclaimed register reporting on Gen 8, we will discover
> >> >> that the IRQ registers for pipes B and C are also on the power well,
> >> >> so writes to them when the power well is disabled result in unclaimed
> >> >> register errors.
> >> >>
> >> >> Also, hsw_power_well_post_enable() already takes care of re-enabling
> >> >> them once the power well is enabled.
> >> >>
> >> >> Testcase: igt/pm_rpm/rte
> >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> >
> >> > Hm, shouldn't we split this into only setting up pipe A here and the
> pipe
> >> > B stuff once we fire up the power well?
> >> >
> >>
> >> No because these functions might be called when the power wells are
> >> already enabled.
> >
> > Hm, where does this still happen? bdw has power well support and chv has
> a
> > different display block ...
>
> At driver init time... If you load i915.ko and the power wells are
> already enabled, we have to do it here.
>
> >
> > This code changed too often and I have no idea any more what's up and
> > what's down here ;-)
> > -Daniel
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>
>
>
> --
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[-- Attachment #1.2: Type: text/html, Size: 3373 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 2/5] drm/i915: HSW_BLC_PWM2_CTL doesn't exist on BDW

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 1570 bytes --]

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


On Fri, Jul 4, 2014 at 7:50 AM, Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> So don't write it, otherwise we will trigger unclaimed register
> errors.
>
> Testcase: igt/pm_rpm/rte
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index c12a5da..14505a1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7345,8 +7345,9 @@ static void assert_can_disable_lcpll(struct
> drm_i915_private *dev_priv)
>         WARN(I915_READ(PCH_PP_STATUS) & PP_ON, "Panel power on\n");
>         WARN(I915_READ(BLC_PWM_CPU_CTL2) & BLM_PWM_ENABLE,
>              "CPU PWM1 enabled\n");
> -       WARN(I915_READ(HSW_BLC_PWM2_CTL) & BLM_PWM_ENABLE,
> -            "CPU PWM2 enabled\n");
> +       if (IS_HASWELL(dev))
> +               WARN(I915_READ(HSW_BLC_PWM2_CTL) & BLM_PWM_ENABLE,
> +                    "CPU PWM2 enabled\n");
>         WARN(I915_READ(BLC_PWM_PCH_CTL1) & BLM_PCH_PWM_ENABLE,
>              "PCH PWM1 enabled\n");
>         WARN(I915_READ(UTIL_PIN_CTL) & UTIL_PIN_ENABLE,
> --
> 2.0.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[-- Attachment #1.2: Type: text/html, Size: 2581 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 3/5] drm/i915: extract and improve gen8_irq_power_well_post_enable

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 4219 bytes --]

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


On Fri, Jul 4, 2014 at 7:50 AM, Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Move it from hsw_power_well_post_enable() (intel_pm.c) to i915_irq.c
> so we can reuse the nice IRQ macros we have there. The main difference
> is that now we're going to check if the IIR register is non-zero when
> we try to re-enable the interrupts.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c  | 12 ++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c  | 18 ++----------------
>  3 files changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c
> b/drivers/gpu/drm/i915/i915_irq.c
> index 2e116e9d..a8b8b6b 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3204,6 +3204,18 @@ static void gen8_irq_reset(struct drm_device *dev)
>         ibx_irq_reset(dev);
>  }
>
> +void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv)
> +{
> +       unsigned long irqflags;
> +
> +       spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> +       GEN8_IRQ_INIT_NDX(DE_PIPE, PIPE_B, dev_priv->de_irq_mask[PIPE_B],
> +                         ~dev_priv->de_irq_mask[PIPE_B]);
> +       GEN8_IRQ_INIT_NDX(DE_PIPE, PIPE_C, dev_priv->de_irq_mask[PIPE_C],
> +                         ~dev_priv->de_irq_mask[PIPE_C]);
> +       spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> +}
> +
>  static void cherryview_irq_preinstall(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 5f7c7bd..46a3a09 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -687,6 +687,7 @@ void intel_runtime_pm_disable_interrupts(struct
> drm_device *dev);
>  void intel_runtime_pm_restore_interrupts(struct drm_device *dev);
>  int intel_get_crtc_scanline(struct intel_crtc *crtc);
>  void i9xx_check_fifo_underruns(struct drm_device *dev);
> +void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv);
>
>
>  /* intel_crt.c */
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 31ae2b4..4cc9e5c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5913,7 +5913,6 @@ bool intel_display_power_enabled(struct
> drm_i915_private *dev_priv,
>  static void hsw_power_well_post_enable(struct drm_i915_private *dev_priv)
>  {
>         struct drm_device *dev = dev_priv->dev;
> -       unsigned long irqflags;
>
>         /*
>          * After we re-enable the power well, if we touch VGA register
> 0x3d5
> @@ -5929,21 +5928,8 @@ static void hsw_power_well_post_enable(struct
> drm_i915_private *dev_priv)
>         outb(inb(VGA_MSR_READ), VGA_MSR_WRITE);
>         vga_put(dev->pdev, VGA_RSRC_LEGACY_IO);
>
> -       if (IS_BROADWELL(dev)) {
> -               spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> -               I915_WRITE(GEN8_DE_PIPE_IMR(PIPE_B),
> -                          dev_priv->de_irq_mask[PIPE_B]);
> -               I915_WRITE(GEN8_DE_PIPE_IER(PIPE_B),
> -                          ~dev_priv->de_irq_mask[PIPE_B] |
> -                          GEN8_PIPE_VBLANK);
> -               I915_WRITE(GEN8_DE_PIPE_IMR(PIPE_C),
> -                          dev_priv->de_irq_mask[PIPE_C]);
> -               I915_WRITE(GEN8_DE_PIPE_IER(PIPE_C),
> -                          ~dev_priv->de_irq_mask[PIPE_C] |
> -                          GEN8_PIPE_VBLANK);
> -               POSTING_READ(GEN8_DE_PIPE_IER(PIPE_C));
> -               spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> -       }
> +       if (IS_BROADWELL(dev))
> +               gen8_irq_power_well_post_enable(dev_priv);
>  }
>
>  static void hsw_set_power_well(struct drm_i915_private *dev_priv,
> --
> 2.0.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[-- Attachment #1.2: Type: text/html, Size: 5569 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 4/5] drm/i915: reorganize the unclaimed register detection code

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 11933 bytes --]

On Mon, Jul 7, 2014 at 2:34 PM, Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, Jul 04, 2014 at 11:50:32AM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > The current code only runs when we do an I915_WRITE operation. It
> > checks if the unclaimed register flag is set before we do the
> > operation, and then it checks it again after we do the operation. This
> > double check allows us to find out if the I915_WRITE operation in
> > question is the bad one, or if some previous code is the bad one. When
> > it finds a problem, our code uses DRM_ERROR to signal it.
> >
> > The good thing about the current code is that it detects the problem,
> > so at least we can know we did something wrong. The problem is that
> > even though we find the problem, we don't really have much information
> > to actually debug it. So whenever I see one of these DRM_ERROR
> > messages on my systems, the first thing I do is apply a patch to
> > change the DRM_ERROR to a WARN and also check for unclaimed registers
> > on I915_READ operations. This local patch makes things even slower,
> > but it usually helps a lot in finding the bad code.
> >
> > The first point here is that since the current code is only useful to
> > detect whether we have a problem or not, but it is not really good to
> > find the cause of the problem, I don't think we should be checking
> > both before and after every I915_WRITE operation: just doing the check
> > once should be enough for us to quickly detect problems. With this
> > change, the code that runs by default for every single user will only
> > do 1 read operation for every single I915_WRITE, instead of 2. This
> > patch does this change.
> >
> > The second point is that the local patch I have should be upstream,
> > but since it makes things slower it should be disabled by default. So
> > I added the i915.mmio_debug option to enable it.
> >
> > So after this patch, this is what will happen:
> >  - By default, we will try to detect unclaimed registers once after
> >    every I915_WRITE operation. Previously we tried twice for every
> >    I915_WRITE.
> >  - When we find an unclaimed register we will still print a DRM_ERROR
> >    message, but we will now tell the user to try again with
> >    i915.mmio_debug=1.
> >  - When we use i915.mmio_debug=1 we will try to find unclaimed
> >    registers both before and after every I915_READ and I915_WRITE
> >    operation, and we will print stack traces in case we find them.
> >    This should really help locating the exact point of the bad code
> >    (or at least finding out that i915.ko is not the problem).
> >
> > This commit also opens space for really-slow register debugging
> > operations on other platforms. In theory we can now add lots and lots
> > of debug code behind i915.mmio_debug, enable this option on our tests,
> > and catch more problems.
> >
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h     |  1 +
> >  drivers/gpu/drm/i915/i915_params.c  |  6 ++++
> >  drivers/gpu/drm/i915/intel_uncore.c | 68
> +++++++++++++++++++++++++++++++++----
> >  3 files changed, 68 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> > index ac06c0f..51d867f 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2092,6 +2092,7 @@ struct i915_params {
> >       bool disable_display;
> >       bool disable_vtd_wa;
> >       int use_mmio_flip;
> > +     bool mmio_debug;
> >  };
> >  extern struct i915_params i915 __read_mostly;
> >
> > diff --git a/drivers/gpu/drm/i915/i915_params.c
> b/drivers/gpu/drm/i915/i915_params.c
> > index 8145729..7977872 100644
> > --- a/drivers/gpu/drm/i915/i915_params.c
> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > @@ -49,6 +49,7 @@ struct i915_params i915 __read_mostly = {
> >       .enable_cmd_parser = 1,
> >       .disable_vtd_wa = 0,
> >       .use_mmio_flip = 0,
> > +     .mmio_debug = 0,
> >  };
> >
> >  module_param_named(modeset, i915.modeset, int, 0400);
> > @@ -161,3 +162,8 @@ MODULE_PARM_DESC(enable_cmd_parser,
> >  module_param_named(use_mmio_flip, i915.use_mmio_flip, int, 0600);
> >  MODULE_PARM_DESC(use_mmio_flip,
> >                "use MMIO flips (-1=never, 0=driver discretion [default],
> 1=always)");
> > +
> > +module_param_named(mmio_debug, i915.mmio_debug, bool, 0600);
> > +MODULE_PARM_DESC(disable_vtd_wa,
>

wrong parameter here!


> > +     "Enable the MMIO debug code (default: false). This may negatively "
> > +     "affect performance.");
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c
> b/drivers/gpu/drm/i915/intel_uncore.c
> > index 29145df..de5402f 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -513,21 +513,72 @@ ilk_dummy_write(struct drm_i915_private *dev_priv)
> >       __raw_i915_write32(dev_priv, MI_MODE, 0);
> >  }
> >
> > +/**
> > + * hsw_unclaimed_reg_debug - tries to find unclaimed registers
> > + * @dev_priv: device private data
> > + * @reg: register we're about to touch or just have touched
> > + * @read: are we reading or writing a register now?
> > + * @before: are we running this before or after we touch the register?
> > + *
> > + * This function tries to detect unclaimed registers and provide as much
> > + * information as possible. It will only do something if the
> i915.mmio_debug
> > + * option is enabled.
> > + *
> > + * If we detect an unclaimed register when @before is true, it means
> some
> > + * unknown code wrote to an unclaimed register, and we're just
> detecting it now.
> > + * The bad code can be i915.ko, some other Kernel module (e.g.,
> snd_hda_intel,
> > + * vgacon) or even user space. If we detect it when @before is false,
> then
> > + * there's a really good chance that the read/write operation we just
> did to
> > + * @reg is what triggered the unclaimed register message, but there's
> also the
> > + * possibility that after the operation we did, and before this
> function,
> > + * something else touched another unclaimed register, and we are
> detecting it
> > + * now.
> > + *
> > + * The unclaimed register bit can get set when we touch a register that
> does not
> > + * exist and when we touch a register that exists but is powered down.
> Please
> > + * also notice that the HW can only check some register ranges, so
> there's no
> > + * guarantee that all read and write operations to inexistent registers
> will be
> > + * caught by this function.
> > + *
> > + * Also please notice that we don't run this function on __raw
> operations and in
> > + * many other cases, so the case where @before is true is quite common.
> > + */
>
> Pet peeve of mine: Comments have a cost since they get stale really fast
> and then can be awfully misleading. We have way too many examples for
> that. My rule of interface kerneldoc for functions is that we should only
> have it for exported functions used in multiple different places in the
> driver. If there's only one caller (like for _init/fini) functions the
> comment should be very terse at most.
>
> For static inline helpers in the same file otoh code should simply be
> readable enough to not require comments. Imo this is the case here, so the
> kerneldoc should imo be dropped.
>
> If you want to keep some of it I guess we could document i915.mmio_debug
> with kerneldoc and then also pull it into our i915 section in the drm
> docbook. Maybe under a new "debug options" setting. We probably need a
> DOC: section to make this work. A few sentences should be more than
> enough.
>
> >  static void
> > -hsw_unclaimed_reg_clear(struct drm_i915_private *dev_priv, u32 reg)
> > +hsw_unclaimed_reg_debug(struct drm_i915_private *dev_priv, u32 reg,
> bool read,
> > +                     bool before)
> >  {
> > +     const char *op = read ? "reading" : "writing to";
> > +     const char *when = before ? "before" : "after";
> > +
> > +     if (!i915.mmio_debug)
> > +             return;
> > +
> >       if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
> > -             DRM_ERROR("Unknown unclaimed register before writing to
> %x\n",
> > -                       reg);
> > +             WARN(1, "Unclaimed register detected %s %s register
> 0x%x\n",
> > +                  when, op, reg);
> >               __raw_i915_write32(dev_priv, FPGA_DBG,
> FPGA_DBG_RM_NOCLAIM);
> >       }
> >  }
> >
> > +/**
> > + * hsw_unclaimed_reg_detect - tries to find unclaimed registers
> > + * @dev_priv: device private data
> > + *
> > + * This function will try to detect if something touched and unclaimed
> register,
> > + * triggering the FPGA_DBG bit. If this happens, we will print a
> message telling
> > + * the user to use i915.mmio_debug=1 so we can properly debug the
> problem.
> > + *
> > + * This way, we can still detect our bugs without giving the user the
> > + * performance impact of hsw_unclaimed_reg_debug().
> > + */
> >  static void
> > -hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg)
> > +hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
> >  {
> > +     if (i915.mmio_debug)
> > +             return;
> > +
> >       if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
> > -             DRM_ERROR("Unclaimed write to %x\n", reg);
> > +             DRM_ERROR("Unclaimed register detected. Please use the
> i915.mmio_debug=1 to debug this problem.");
>
> But imo really the only piece that's required is this helpful message here
> into dmesg.
>
> Anyway, the concept looks solid.
> -Daniel
>
> >               __raw_i915_write32(dev_priv, FPGA_DBG,
> FPGA_DBG_RM_NOCLAIM);
> >       }
> >  }
> > @@ -564,6 +615,7 @@ gen5_read##x(struct drm_i915_private *dev_priv,
> off_t reg, bool trace) { \
> >  static u##x \
> >  gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace)
> { \
> >       REG_READ_HEADER(x); \
> > +     hsw_unclaimed_reg_debug(dev_priv, reg, true, true); \
> >       if (dev_priv->uncore.forcewake_count == 0 && \
> >           NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
> >               dev_priv->uncore.funcs.force_wake_get(dev_priv, \
> > @@ -574,6 +626,7 @@ gen6_read##x(struct drm_i915_private *dev_priv,
> off_t reg, bool trace) { \
> >       } else { \
> >               val = __raw_i915_read##x(dev_priv, reg); \
> >       } \
> > +     hsw_unclaimed_reg_debug(dev_priv, reg, true, false); \
> >       REG_READ_FOOTER; \
> >  }
> >
> > @@ -700,12 +753,13 @@ hsw_write##x(struct drm_i915_private *dev_priv,
> off_t reg, u##x val, bool trace)
> >       if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
> >               __fifo_ret = __gen6_gt_wait_for_fifo(dev_priv); \
> >       } \
> > -     hsw_unclaimed_reg_clear(dev_priv, reg); \
> > +     hsw_unclaimed_reg_debug(dev_priv, reg, false, true); \
> >       __raw_i915_write##x(dev_priv, reg, val); \
> >       if (unlikely(__fifo_ret)) { \
> >               gen6_gt_check_fifodbg(dev_priv); \
> >       } \
> > -     hsw_unclaimed_reg_check(dev_priv, reg); \
> > +     hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
> > +     hsw_unclaimed_reg_detect(dev_priv); \
> >       REG_WRITE_FOOTER; \
> >  }
> >
> > --
> > 2.0.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

The rest looks good. With that fixed and probably with comment removed as
Daniel mentioned, feel free to use:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[-- Attachment #1.2: Type: text/html, Size: 15130 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 5/5] drm/i915: BDW can also detect unclaimed registers

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 3452 bytes --]

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


On Fri, Jul 4, 2014 at 7:50 AM, Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> By the time I wrote this patch, it allowed me to catch some problems.
> But due to patch reordering - in order to prevent fake "regression"
> reports - this patch may be merged after the fixes of the problems
> identified by this patch.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c     | 4 ++++
>  drivers/gpu/drm/i915/intel_uncore.c | 3 +++
>  2 files changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c
> index 8a0cb0c..bdb223c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -303,6 +303,7 @@ static const struct intel_device_info
> intel_broadwell_d_info = {
>         .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
>         .has_llc = 1,
>         .has_ddi = 1,
> +       .has_fpga_dbg = 1,
>         .has_fbc = 1,
>         GEN_DEFAULT_PIPEOFFSETS,
>         IVB_CURSOR_OFFSETS,
> @@ -314,6 +315,7 @@ static const struct intel_device_info
> intel_broadwell_m_info = {
>         .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
>         .has_llc = 1,
>         .has_ddi = 1,
> +       .has_fpga_dbg = 1,
>         .has_fbc = 1,
>         GEN_DEFAULT_PIPEOFFSETS,
>         IVB_CURSOR_OFFSETS,
> @@ -325,6 +327,7 @@ static const struct intel_device_info
> intel_broadwell_gt3d_info = {
>         .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING |
> BSD2_RING,
>         .has_llc = 1,
>         .has_ddi = 1,
> +       .has_fpga_dbg = 1,
>         .has_fbc = 1,
>         GEN_DEFAULT_PIPEOFFSETS,
>         IVB_CURSOR_OFFSETS,
> @@ -336,6 +339,7 @@ static const struct intel_device_info
> intel_broadwell_gt3m_info = {
>         .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING |
> BSD2_RING,
>         .has_llc = 1,
>         .has_ddi = 1,
> +       .has_fpga_dbg = 1,
>         .has_fbc = 1,
>         GEN_DEFAULT_PIPEOFFSETS,
>         IVB_CURSOR_OFFSETS,
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c
> b/drivers/gpu/drm/i915/intel_uncore.c
> index de5402f..1fcf78b 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -788,6 +788,7 @@ static bool is_gen8_shadowed(struct drm_i915_private
> *dev_priv, u32 reg)
>  static void \
>  gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val,
> bool trace) { \
>         REG_WRITE_HEADER; \
> +       hsw_unclaimed_reg_debug(dev_priv, reg, false, true); \
>         if (reg < 0x40000 && !is_gen8_shadowed(dev_priv, reg)) { \
>                 if (dev_priv->uncore.forcewake_count == 0) \
>                         dev_priv->uncore.funcs.force_wake_get(dev_priv, \
> @@ -799,6 +800,8 @@ gen8_write##x(struct drm_i915_private *dev_priv, off_t
> reg, u##x val, bool trace
>         } else { \
>                 __raw_i915_write##x(dev_priv, reg, val); \
>         } \
> +       hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
> +       hsw_unclaimed_reg_detect(dev_priv); \
>         REG_WRITE_FOOTER; \
>  }
>
> --
> 2.0.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[-- Attachment #1.2: Type: text/html, Size: 4632 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 5/5] drm/i915: BDW can also detect unclaimed registers

[Daniel Vetter]

On Tue, Jul 15, 2014 at 12:20:01PM -0700, Rodrigo Vivi wrote:
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> 
> On Fri, Jul 4, 2014 at 7:50 AM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > By the time I wrote this patch, it allowed me to catch some problems.
> > But due to patch reordering - in order to prevent fake "regression"
> > reports - this patch may be merged after the fixes of the problems
> > identified by this patch.
> >
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Merged all patches except patch 4 (the one that adds the module option).
Thanks.
-Daniel

> > ---
> >  drivers/gpu/drm/i915/i915_drv.c     | 4 ++++
> >  drivers/gpu/drm/i915/intel_uncore.c | 3 +++
> >  2 files changed, 7 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index 8a0cb0c..bdb223c 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -303,6 +303,7 @@ static const struct intel_device_info
> > intel_broadwell_d_info = {
> >         .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
> >         .has_llc = 1,
> >         .has_ddi = 1,
> > +       .has_fpga_dbg = 1,
> >         .has_fbc = 1,
> >         GEN_DEFAULT_PIPEOFFSETS,
> >         IVB_CURSOR_OFFSETS,
> > @@ -314,6 +315,7 @@ static const struct intel_device_info
> > intel_broadwell_m_info = {
> >         .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
> >         .has_llc = 1,
> >         .has_ddi = 1,
> > +       .has_fpga_dbg = 1,
> >         .has_fbc = 1,
> >         GEN_DEFAULT_PIPEOFFSETS,
> >         IVB_CURSOR_OFFSETS,
> > @@ -325,6 +327,7 @@ static const struct intel_device_info
> > intel_broadwell_gt3d_info = {
> >         .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING |
> > BSD2_RING,
> >         .has_llc = 1,
> >         .has_ddi = 1,
> > +       .has_fpga_dbg = 1,
> >         .has_fbc = 1,
> >         GEN_DEFAULT_PIPEOFFSETS,
> >         IVB_CURSOR_OFFSETS,
> > @@ -336,6 +339,7 @@ static const struct intel_device_info
> > intel_broadwell_gt3m_info = {
> >         .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING |
> > BSD2_RING,
> >         .has_llc = 1,
> >         .has_ddi = 1,
> > +       .has_fpga_dbg = 1,
> >         .has_fbc = 1,
> >         GEN_DEFAULT_PIPEOFFSETS,
> >         IVB_CURSOR_OFFSETS,
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c
> > b/drivers/gpu/drm/i915/intel_uncore.c
> > index de5402f..1fcf78b 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -788,6 +788,7 @@ static bool is_gen8_shadowed(struct drm_i915_private
> > *dev_priv, u32 reg)
> >  static void \
> >  gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val,
> > bool trace) { \
> >         REG_WRITE_HEADER; \
> > +       hsw_unclaimed_reg_debug(dev_priv, reg, false, true); \
> >         if (reg < 0x40000 && !is_gen8_shadowed(dev_priv, reg)) { \
> >                 if (dev_priv->uncore.forcewake_count == 0) \
> >                         dev_priv->uncore.funcs.force_wake_get(dev_priv, \
> > @@ -799,6 +800,8 @@ gen8_write##x(struct drm_i915_private *dev_priv, off_t
> > reg, u##x val, bool trace
> >         } else { \
> >                 __raw_i915_write##x(dev_priv, reg, val); \
> >         } \
> > +       hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
> > +       hsw_unclaimed_reg_detect(dev_priv); \
> >         REG_WRITE_FOOTER; \
> >  }
> >
> > --
> > 2.0.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

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


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 1/2] drm/i915: reorganize the unclaimed register detection code

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

The current code only runs when we do an I915_WRITE operation. It
checks if the unclaimed register flag is set before we do the
operation, and then it checks it again after we do the operation. This
double check allows us to find out if the I915_WRITE operation in
question is the bad one, or if some previous code is the bad one. When
it finds a problem, our code uses DRM_ERROR to signal it.

The good thing about the current code is that it detects the problem,
so at least we can know we did something wrong. The problem is that
even though we find the problem, we don't really have much information
to actually debug it. So whenever I see one of these DRM_ERROR
messages on my systems, the first thing I do is apply a patch to
change the DRM_ERROR to a WARN and also check for unclaimed registers
on I915_READ operations. This local patch makes things even slower,
but it usually helps a lot in finding the bad code.

The first point here is that since the current code is only useful to
detect whether we have a problem or not, but it is not really good to
find the cause of the problem, I don't think we should be checking
both before and after every I915_WRITE operation: just doing the check
once should be enough for us to quickly detect problems. With this
change, the code that runs by default for every single user will only
do 1 read operation for every single I915_WRITE, instead of 2. This
patch does this change.

The second point is that the local patch I have should be upstream,
but since it makes things slower it should be disabled by default. So
I added the i915.mmio_debug option to enable it.

So after this patch, this is what will happen:
 - By default, we will try to detect unclaimed registers once after
   every I915_WRITE operation. Previously we tried twice for every
   I915_WRITE.
 - When we find an unclaimed register we will still print a DRM_ERROR
   message, but we will now tell the user to try again with
   i915.mmio_debug=1.
 - When we use i915.mmio_debug=1 we will try to find unclaimed
   registers both before and after every I915_READ and I915_WRITE
   operation, and we will print stack traces in case we find them.
   This should really help locating the exact point of the bad code
   (or at least finding out that i915.ko is not the problem).

This commit also opens space for really-slow register debugging
operations on other platforms. In theory we can now add lots and lots
of debug code behind i915.mmio_debug, enable this option on our tests,
and catch more problems.

v2: - Remove not-so-useful comments (Daniel)
    - Fix the param definition macros (Rodrigo)

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h     |  1 +
 drivers/gpu/drm/i915/i915_params.c  |  6 ++++++
 drivers/gpu/drm/i915/intel_uncore.c | 27 ++++++++++++++++++++-------
 3 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 991b663..ca0a9dd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2135,6 +2135,7 @@ struct i915_params {
 	bool disable_display;
 	bool disable_vtd_wa;
 	int use_mmio_flip;
+	bool mmio_debug;
 };
 extern struct i915_params i915 __read_mostly;
 
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index bbdee21..62ee830 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -49,6 +49,7 @@ struct i915_params i915 __read_mostly = {
 	.enable_cmd_parser = 1,
 	.disable_vtd_wa = 0,
 	.use_mmio_flip = 0,
+	.mmio_debug = 0,
 };
 
 module_param_named(modeset, i915.modeset, int, 0400);
@@ -161,3 +162,8 @@ MODULE_PARM_DESC(enable_cmd_parser,
 module_param_named(use_mmio_flip, i915.use_mmio_flip, int, 0600);
 MODULE_PARM_DESC(use_mmio_flip,
 		 "use MMIO flips (-1=never, 0=driver discretion [default], 1=always)");
+
+module_param_named(mmio_debug, i915.mmio_debug, bool, 0600);
+MODULE_PARM_DESC(mmio_debug,
+	"Enable the MMIO debug code (default: false). This may negatively "
+	"affect performance.");
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index e0f0843..6fee122 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -514,20 +514,30 @@ ilk_dummy_write(struct drm_i915_private *dev_priv)
 }
 
 static void
-hsw_unclaimed_reg_clear(struct drm_i915_private *dev_priv, u32 reg)
+hsw_unclaimed_reg_debug(struct drm_i915_private *dev_priv, u32 reg, bool read,
+			bool before)
 {
+	const char *op = read ? "reading" : "writing to";
+	const char *when = before ? "before" : "after";
+
+	if (!i915.mmio_debug)
+		return;
+
 	if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
-		DRM_ERROR("Unknown unclaimed register before writing to %x\n",
-			  reg);
+		WARN(1, "Unclaimed register detected %s %s register 0x%x\n",
+		     when, op, reg);
 		__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
 	}
 }
 
 static void
-hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg)
+hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
 {
+	if (i915.mmio_debug)
+		return;
+
 	if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
-		DRM_ERROR("Unclaimed write to %x\n", reg);
+		DRM_ERROR("Unclaimed register detected. Please use the i915.mmio_debug=1 to debug this problem.");
 		__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
 	}
 }
@@ -564,6 +574,7 @@ gen5_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
 static u##x \
 gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
 	REG_READ_HEADER(x); \
+	hsw_unclaimed_reg_debug(dev_priv, reg, true, true); \
 	if (dev_priv->uncore.forcewake_count == 0 && \
 	    NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
 		dev_priv->uncore.funcs.force_wake_get(dev_priv, \
@@ -574,6 +585,7 @@ gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
 	} else { \
 		val = __raw_i915_read##x(dev_priv, reg); \
 	} \
+	hsw_unclaimed_reg_debug(dev_priv, reg, true, false); \
 	REG_READ_FOOTER; \
 }
 
@@ -700,12 +712,13 @@ hsw_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace)
 	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
 		__fifo_ret = __gen6_gt_wait_for_fifo(dev_priv); \
 	} \
-	hsw_unclaimed_reg_clear(dev_priv, reg); \
+	hsw_unclaimed_reg_debug(dev_priv, reg, false, true); \
 	__raw_i915_write##x(dev_priv, reg, val); \
 	if (unlikely(__fifo_ret)) { \
 		gen6_gt_check_fifodbg(dev_priv); \
 	} \
-	hsw_unclaimed_reg_check(dev_priv, reg); \
+	hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
+	hsw_unclaimed_reg_detect(dev_priv); \
 	REG_WRITE_FOOTER; \
 }
 
-- 
2.0.0

[PATCH 2/2] drm/i915: BDW can also detect unclaimed registers

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

By the time I wrote this patch, it allowed me to catch some problems.
But due to patch reordering - in order to prevent fake "regression"
reports - this patch may be merged after the fixes of the problems
identified by this patch.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c     | 4 ++++
 drivers/gpu/drm/i915/intel_uncore.c | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 5e4fefd..3315358 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -303,6 +303,7 @@ static const struct intel_device_info intel_broadwell_d_info = {
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
 	.has_llc = 1,
 	.has_ddi = 1,
+	.has_fpga_dbg = 1,
 	.has_fbc = 1,
 	GEN_DEFAULT_PIPEOFFSETS,
 	IVB_CURSOR_OFFSETS,
@@ -314,6 +315,7 @@ static const struct intel_device_info intel_broadwell_m_info = {
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
 	.has_llc = 1,
 	.has_ddi = 1,
+	.has_fpga_dbg = 1,
 	.has_fbc = 1,
 	GEN_DEFAULT_PIPEOFFSETS,
 	IVB_CURSOR_OFFSETS,
@@ -325,6 +327,7 @@ static const struct intel_device_info intel_broadwell_gt3d_info = {
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
 	.has_llc = 1,
 	.has_ddi = 1,
+	.has_fpga_dbg = 1,
 	.has_fbc = 1,
 	GEN_DEFAULT_PIPEOFFSETS,
 	IVB_CURSOR_OFFSETS,
@@ -336,6 +339,7 @@ static const struct intel_device_info intel_broadwell_gt3m_info = {
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
 	.has_llc = 1,
 	.has_ddi = 1,
+	.has_fpga_dbg = 1,
 	.has_fbc = 1,
 	GEN_DEFAULT_PIPEOFFSETS,
 	IVB_CURSOR_OFFSETS,
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 6fee122..e81bc3b 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -747,6 +747,7 @@ static bool is_gen8_shadowed(struct drm_i915_private *dev_priv, u32 reg)
 static void \
 gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
 	REG_WRITE_HEADER; \
+	hsw_unclaimed_reg_debug(dev_priv, reg, false, true); \
 	if (reg < 0x40000 && !is_gen8_shadowed(dev_priv, reg)) { \
 		if (dev_priv->uncore.forcewake_count == 0) \
 			dev_priv->uncore.funcs.force_wake_get(dev_priv,	\
@@ -758,6 +759,8 @@ gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace
 	} else { \
 		__raw_i915_write##x(dev_priv, reg, val); \
 	} \
+	hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
+	hsw_unclaimed_reg_detect(dev_priv); \
 	REG_WRITE_FOOTER; \
 }
 
-- 
2.0.0

Re: [PATCH 2/2] drm/i915: BDW can also detect unclaimed registers

[Daniel Vetter]

On Wed, Jul 16, 2014 at 05:49:30PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> By the time I wrote this patch, it allowed me to catch some problems.
> But due to patch reordering - in order to prevent fake "regression"
> reports - this patch may be merged after the fixes of the problems
> identified by this patch.
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Pulled in, thanks for the resend. And my apology for the mess I've made
yesterday.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.c     | 4 ++++
>  drivers/gpu/drm/i915/intel_uncore.c | 3 +++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 5e4fefd..3315358 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -303,6 +303,7 @@ static const struct intel_device_info intel_broadwell_d_info = {
>  	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
>  	.has_llc = 1,
>  	.has_ddi = 1,
> +	.has_fpga_dbg = 1,
>  	.has_fbc = 1,
>  	GEN_DEFAULT_PIPEOFFSETS,
>  	IVB_CURSOR_OFFSETS,
> @@ -314,6 +315,7 @@ static const struct intel_device_info intel_broadwell_m_info = {
>  	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
>  	.has_llc = 1,
>  	.has_ddi = 1,
> +	.has_fpga_dbg = 1,
>  	.has_fbc = 1,
>  	GEN_DEFAULT_PIPEOFFSETS,
>  	IVB_CURSOR_OFFSETS,
> @@ -325,6 +327,7 @@ static const struct intel_device_info intel_broadwell_gt3d_info = {
>  	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
>  	.has_llc = 1,
>  	.has_ddi = 1,
> +	.has_fpga_dbg = 1,
>  	.has_fbc = 1,
>  	GEN_DEFAULT_PIPEOFFSETS,
>  	IVB_CURSOR_OFFSETS,
> @@ -336,6 +339,7 @@ static const struct intel_device_info intel_broadwell_gt3m_info = {
>  	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
>  	.has_llc = 1,
>  	.has_ddi = 1,
> +	.has_fpga_dbg = 1,
>  	.has_fbc = 1,
>  	GEN_DEFAULT_PIPEOFFSETS,
>  	IVB_CURSOR_OFFSETS,
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 6fee122..e81bc3b 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -747,6 +747,7 @@ static bool is_gen8_shadowed(struct drm_i915_private *dev_priv, u32 reg)
>  static void \
>  gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
>  	REG_WRITE_HEADER; \
> +	hsw_unclaimed_reg_debug(dev_priv, reg, false, true); \
>  	if (reg < 0x40000 && !is_gen8_shadowed(dev_priv, reg)) { \
>  		if (dev_priv->uncore.forcewake_count == 0) \
>  			dev_priv->uncore.funcs.force_wake_get(dev_priv,	\
> @@ -758,6 +759,8 @@ gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace
>  	} else { \
>  		__raw_i915_write##x(dev_priv, reg, val); \
>  	} \
> +	hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
> +	hsw_unclaimed_reg_detect(dev_priv); \
>  	REG_WRITE_FOOTER; \
>  }
>  
> -- 
> 2.0.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/2] drm/i915: reorganize the unclaimed register detection code

[Chris Wilson]

On Wed, Jul 16, 2014 at 05:49:29PM -0300, Paulo Zanoni wrote:
>  static void
> -hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg)
> +hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
>  {
> +	if (i915.mmio_debug)
> +		return;
> +
>  	if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
> -		DRM_ERROR("Unclaimed write to %x\n", reg);
> +		DRM_ERROR("Unclaimed register detected. Please use the i915.mmio_debug=1 to debug this problem.");
>  		__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
>  	}

What was the point here? You still add an extra read to every register
write and then repeat the request to enable mmio_debug ad infinitum. And
you still check for illegal writes in the irq handler.

Just kill this code.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: reorganize the unclaimed register detection code

[Paulo Zanoni]

2014-08-26 7:22 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Wed, Jul 16, 2014 at 05:49:29PM -0300, Paulo Zanoni wrote:
>>  static void
>> -hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg)
>> +hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
>>  {
>> +     if (i915.mmio_debug)
>> +             return;
>> +
>>       if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
>> -             DRM_ERROR("Unclaimed write to %x\n", reg);
>> +             DRM_ERROR("Unclaimed register detected. Please use the i915.mmio_debug=1 to debug this problem.");
>>               __raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
>>       }
>
> What was the point here? You still add an extra read to every register
> write

Well, we previously had 2 extra reads instead of 1, so with this patch
we're in a better position :)


> and then repeat the request to enable mmio_debug ad infinitum.

Yeah, this could be avoided. OTOH, on most cases it's not gonna happen
frequently enough to annoy the user.


> And
> you still check for illegal writes in the irq handler.

That just happens on HSW, not BDW+.

>
> Just kill this code.

If we do it, we won't be checking for unclaimed registers on BDW+
without i915.mmio_debug=1.


> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni

Re: [PATCH 1/2] drm/i915: reorganize the unclaimed register detection code

[Chris Wilson]

On Tue, Aug 26, 2014 at 09:17:11AM -0300, Paulo Zanoni wrote:
> 2014-08-26 7:22 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Wed, Jul 16, 2014 at 05:49:29PM -0300, Paulo Zanoni wrote:
> >>  static void
> >> -hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg)
> >> +hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
> >>  {
> >> +     if (i915.mmio_debug)
> >> +             return;
> >> +
> >>       if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
> >> -             DRM_ERROR("Unclaimed write to %x\n", reg);
> >> +             DRM_ERROR("Unclaimed register detected. Please use the i915.mmio_debug=1 to debug this problem.");
> >>               __raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
> >>       }
> >
> > What was the point here? You still add an extra read to every register
> > write
> 
> Well, we previously had 2 extra reads instead of 1, so with this patch
> we're in a better position :)
> 
> 
> > and then repeat the request to enable mmio_debug ad infinitum.
> 
> Yeah, this could be avoided. OTOH, on most cases it's not gonna happen
> frequently enough to annoy the user.

How do you think I came across this?

> > And
> > you still check for illegal writes in the irq handler.
> 
> That just happens on HSW, not BDW+.

Even on hsw, it should be killed. Checking inside the irq handler is
just insane. Just move it to one of the periodic checks since we can't
get any more information than an error occurred, and ask to be re-run
with mmio_debug (and then shut up) - heck you could even automatically
enable it for a one-shot operation.

> >
> > Just kill this code.
> 
> If we do it, we won't be checking for unclaimed registers on BDW+
> without i915.mmio_debug=1.

Then do as above.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: reorganize the unclaimed register detection code

[Paulo Zanoni]

2014-08-26 9:42 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Tue, Aug 26, 2014 at 09:17:11AM -0300, Paulo Zanoni wrote:
>> 2014-08-26 7:22 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
>> > On Wed, Jul 16, 2014 at 05:49:29PM -0300, Paulo Zanoni wrote:
>> >>  static void
>> >> -hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg)
>> >> +hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
>> >>  {
>> >> +     if (i915.mmio_debug)
>> >> +             return;
>> >> +
>> >>       if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
>> >> -             DRM_ERROR("Unclaimed write to %x\n", reg);
>> >> +             DRM_ERROR("Unclaimed register detected. Please use the i915.mmio_debug=1 to debug this problem.");
>> >>               __raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
>> >>       }
>> >
>> > What was the point here? You still add an extra read to every register
>> > write
>>
>> Well, we previously had 2 extra reads instead of 1, so with this patch
>> we're in a better position :)
>>
>>
>> > and then repeat the request to enable mmio_debug ad infinitum.
>>
>> Yeah, this could be avoided. OTOH, on most cases it's not gonna happen
>> frequently enough to annoy the user.
>
> How do you think I came across this?
>
>> > And
>> > you still check for illegal writes in the irq handler.
>>
>> That just happens on HSW, not BDW+.
>
> Even on hsw, it should be killed. Checking inside the irq handler is
> just insane. Just move it to one of the periodic checks since we can't
> get any more information than an error occurred, and ask to be re-run
> with mmio_debug (and then shut up) - heck you could even automatically
> enable it for a one-shot operation.

Yeah, looking at how the IRQ handler situation is _today_, I agree it
doesn't really make too much sense. I know it was different in the
past, so I wonder how we ended up reaching this point :)

Anyway, if we just remove the call to intel_uncore_check_errors() that
happens on the irq handler, we'll still end up checking for errors as
soon as we I915_WRITE(DEIER), so that won't be too much helpful. One
thing we could do would be to remove the check from the I915_WRITE
macros and put it on a real periodic check that could be executed at
other specific points of our code (less frequent than every
i915_write), or put it at its own workqueue that gets run every X
jiffies. Or perhaps change the implementation of
hsw_unclaimed_reg_detect() to not do anything when we're in an
interrupt/spinlock context. Which one do you think is better to do?

Of course, we can also implement the one-shot thing on top of the
above, but it won't really help us reducing the amount of reads on the
"happy case" where we never got the error before.

All I have to say in my defense is that I did ask you to look at this
patch before it was merged :)

>
>> >
>> > Just kill this code.
>>
>> If we do it, we won't be checking for unclaimed registers on BDW+
>> without i915.mmio_debug=1.
>
> Then do as above.
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni

Re: [PATCH 1/2] drm/i915: reorganize the unclaimed register detection code

[Chris Wilson]

On Tue, Aug 26, 2014 at 10:04:22AM -0300, Paulo Zanoni wrote:
> 2014-08-26 9:42 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Tue, Aug 26, 2014 at 09:17:11AM -0300, Paulo Zanoni wrote:
> >> 2014-08-26 7:22 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> >> > On Wed, Jul 16, 2014 at 05:49:29PM -0300, Paulo Zanoni wrote:
> >> >>  static void
> >> >> -hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg)
> >> >> +hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
> >> >>  {
> >> >> +     if (i915.mmio_debug)
> >> >> +             return;
> >> >> +
> >> >>       if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
> >> >> -             DRM_ERROR("Unclaimed write to %x\n", reg);
> >> >> +             DRM_ERROR("Unclaimed register detected. Please use the i915.mmio_debug=1 to debug this problem.");
> >> >>               __raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
> >> >>       }
> >> >
> >> > What was the point here? You still add an extra read to every register
> >> > write
> >>
> >> Well, we previously had 2 extra reads instead of 1, so with this patch
> >> we're in a better position :)
> >>
> >>
> >> > and then repeat the request to enable mmio_debug ad infinitum.
> >>
> >> Yeah, this could be avoided. OTOH, on most cases it's not gonna happen
> >> frequently enough to annoy the user.
> >
> > How do you think I came across this?
> >
> >> > And
> >> > you still check for illegal writes in the irq handler.
> >>
> >> That just happens on HSW, not BDW+.
> >
> > Even on hsw, it should be killed. Checking inside the irq handler is
> > just insane. Just move it to one of the periodic checks since we can't
> > get any more information than an error occurred, and ask to be re-run
> > with mmio_debug (and then shut up) - heck you could even automatically
> > enable it for a one-shot operation.
> 
> Yeah, looking at how the IRQ handler situation is _today_, I agree it
> doesn't really make too much sense. I know it was different in the
> past, so I wonder how we ended up reaching this point :)
> 
> Anyway, if we just remove the call to intel_uncore_check_errors() that
> happens on the irq handler, we'll still end up checking for errors as
> soon as we I915_WRITE(DEIER), so that won't be too much helpful. One
> thing we could do would be to remove the check from the I915_WRITE
> macros and put it on a real periodic check that could be executed at
> other specific points of our code (less frequent than every
> i915_write), or put it at its own workqueue that gets run every X
> jiffies. Or perhaps change the implementation of
> hsw_unclaimed_reg_detect() to not do anything when we're in an
> interrupt/spinlock context. Which one do you think is better to do?

>From the irq handler, we can use just use raw mmio interfaces and skip
all the debug and forcewake. I think the solution I prefer is to
instrument modesetting (say check_state) and warn if an error had occurred
outside of mmio_debug.
 
> Of course, we can also implement the one-shot thing on top of the
> above, but it won't really help us reducing the amount of reads on the
> "happy case" where we never got the error before.

Actually I am tempted to dynamically patch the mmio vfuncs to avoid even
the forcewake spinlock when we already hold it. So there won't be any
such logic except when enabled by the user.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: reorganize the unclaimed register detection code

[Paulo Zanoni]

2014-08-26 10:18 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Tue, Aug 26, 2014 at 10:04:22AM -0300, Paulo Zanoni wrote:
>> 2014-08-26 9:42 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
>> > On Tue, Aug 26, 2014 at 09:17:11AM -0300, Paulo Zanoni wrote:
>> >> 2014-08-26 7:22 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
>> >> > On Wed, Jul 16, 2014 at 05:49:29PM -0300, Paulo Zanoni wrote:
>> >> >>  static void
>> >> >> -hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg)
>> >> >> +hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
>> >> >>  {
>> >> >> +     if (i915.mmio_debug)
>> >> >> +             return;
>> >> >> +
>> >> >>       if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) {
>> >> >> -             DRM_ERROR("Unclaimed write to %x\n", reg);
>> >> >> +             DRM_ERROR("Unclaimed register detected. Please use the i915.mmio_debug=1 to debug this problem.");
>> >> >>               __raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
>> >> >>       }
>> >> >
>> >> > What was the point here? You still add an extra read to every register
>> >> > write
>> >>
>> >> Well, we previously had 2 extra reads instead of 1, so with this patch
>> >> we're in a better position :)
>> >>
>> >>
>> >> > and then repeat the request to enable mmio_debug ad infinitum.
>> >>
>> >> Yeah, this could be avoided. OTOH, on most cases it's not gonna happen
>> >> frequently enough to annoy the user.
>> >
>> > How do you think I came across this?
>> >
>> >> > And
>> >> > you still check for illegal writes in the irq handler.
>> >>
>> >> That just happens on HSW, not BDW+.
>> >
>> > Even on hsw, it should be killed. Checking inside the irq handler is
>> > just insane. Just move it to one of the periodic checks since we can't
>> > get any more information than an error occurred, and ask to be re-run
>> > with mmio_debug (and then shut up) - heck you could even automatically
>> > enable it for a one-shot operation.
>>
>> Yeah, looking at how the IRQ handler situation is _today_, I agree it
>> doesn't really make too much sense. I know it was different in the
>> past, so I wonder how we ended up reaching this point :)
>>
>> Anyway, if we just remove the call to intel_uncore_check_errors() that
>> happens on the irq handler, we'll still end up checking for errors as
>> soon as we I915_WRITE(DEIER), so that won't be too much helpful. One
>> thing we could do would be to remove the check from the I915_WRITE
>> macros and put it on a real periodic check that could be executed at
>> other specific points of our code (less frequent than every
>> i915_write), or put it at its own workqueue that gets run every X
>> jiffies. Or perhaps change the implementation of
>> hsw_unclaimed_reg_detect() to not do anything when we're in an
>> interrupt/spinlock context. Which one do you think is better to do?
>
> From the irq handler, we can use just use raw mmio interfaces and skip
> all the debug and forcewake. I think the solution I prefer is to
> instrument modesetting (say check_state) and warn if an error had occurred
> outside of mmio_debug.

My only problem with checking at modesetting is that we often spend
hours and hours without doing a modeset, so it could lead to problems
not ever being detected. So maybe there's a better place, but if
that's what we want I won't block any patches.

>
>> Of course, we can also implement the one-shot thing on top of the
>> above, but it won't really help us reducing the amount of reads on the
>> "happy case" where we never got the error before.
>
> Actually I am tempted to dynamically patch the mmio vfuncs to avoid even
> the forcewake spinlock when we already hold it. So there won't be any
> such logic except when enabled by the user.

Should I expect a patch from you, or should I go and write the patch
based on what we already discussed?

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni

Re: [PATCH 1/2] drm/i915: reorganize the unclaimed register detection code

[Daniel Vetter]

On Tue, Aug 26, 2014 at 10:29:23AM -0300, Paulo Zanoni wrote:
> 2014-08-26 10:18 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Tue, Aug 26, 2014 at 10:04:22AM -0300, Paulo Zanoni wrote:
> >> Of course, we can also implement the one-shot thing on top of the
> >> above, but it won't really help us reducing the amount of reads on the
> >> "happy case" where we never got the error before.
> >
> > Actually I am tempted to dynamically patch the mmio vfuncs to avoid even
> > the forcewake spinlock when we already hold it. So there won't be any
> > such logic except when enabled by the user.
> 
> Should I expect a patch from you, or should I go and write the patch
> based on what we already discussed?

Imo this is crazy - we have no control over what the compiler does and
when exactly it loads vtable entries, so patching them at runtime would be
an interesting excercise at best.

Adding a special version of I915_READ/WRITE for the irq hotpath makes
sense, but only if we can actually show a benefit in benchmarks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/2] drm/i915: reorganize the unclaimed register detection code

[Chris Wilson]

On Tue, Aug 26, 2014 at 03:34:07PM +0200, Daniel Vetter wrote:
> On Tue, Aug 26, 2014 at 10:29:23AM -0300, Paulo Zanoni wrote:
> > 2014-08-26 10:18 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > > On Tue, Aug 26, 2014 at 10:04:22AM -0300, Paulo Zanoni wrote:
> > >> Of course, we can also implement the one-shot thing on top of the
> > >> above, but it won't really help us reducing the amount of reads on the
> > >> "happy case" where we never got the error before.
> > >
> > > Actually I am tempted to dynamically patch the mmio vfuncs to avoid even
> > > the forcewake spinlock when we already hold it. So there won't be any
> > > such logic except when enabled by the user.
> > 
> > Should I expect a patch from you, or should I go and write the patch
> > based on what we already discussed?
> 
> Imo this is crazy - we have no control over what the compiler does and
> when exactly it loads vtable entries, so patching them at runtime would be
> an interesting excercise at best.

Wtf?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: reorganize the unclaimed register detection code

[Daniel Vetter]

On Tue, Aug 26, 2014 at 02:46:25PM +0100, Chris Wilson wrote:
> On Tue, Aug 26, 2014 at 03:34:07PM +0200, Daniel Vetter wrote:
> > On Tue, Aug 26, 2014 at 10:29:23AM -0300, Paulo Zanoni wrote:
> > > 2014-08-26 10:18 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > > > On Tue, Aug 26, 2014 at 10:04:22AM -0300, Paulo Zanoni wrote:
> > > >> Of course, we can also implement the one-shot thing on top of the
> > > >> above, but it won't really help us reducing the amount of reads on the
> > > >> "happy case" where we never got the error before.
> > > >
> > > > Actually I am tempted to dynamically patch the mmio vfuncs to avoid even
> > > > the forcewake spinlock when we already hold it. So there won't be any
> > > > such logic except when enabled by the user.
> > > 
> > > Should I expect a patch from you, or should I go and write the patch
> > > based on what we already discussed?
> > 
> > Imo this is crazy - we have no control over what the compiler does and
> > when exactly it loads vtable entries, so patching them at runtime would be
> > an interesting excercise at best.
> 
> Wtf?

I've assumed you'd runtime patch the vtables or something like that. And I
didn't see any other less crazy way to ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch