[PATCH 00/10] Haswell unclaimed register fixes and power well enabling

[PATCH 00/10] Haswell unclaimed register fixes and power well enabling

[Paulo Zanoni]

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

Hi

This series fixes a few of our current "unclaimed register" messages, then it
introduces the code to disable the power well when we're using eDP only, and
then it fixes a few more "unclaimed register" messages brought by the power down
code. In the last patches we remove the GEN7_ERR_INT checking from I915_WRITE
and move it to the interrupt code.

>From my tests, after this series I still only see 1 "unclaimed register" message
that seems to be caused by our driver. All the other messages (there are a lot
of messages that happen all together when booting) stop when we blacklist
snd_hda_intel and its friends. Still, the plan is to get rid of all the
remaining messages.

Cheers,
Paulo

Daniel Vetter (1):
  drm/i915: dynamic Haswell display power well support

Paulo Zanoni (9):
  drm/i915: don't save/restore DSPARB on gen5+
  drm/i915: don't read DP_TP_STATUS(PORT_A)
  drm/i915: fix intel_init_power_wells
  drm/i915: only disable enabled planes on intel_fb_restore_mode
  drm/i915: check the power down well on assert_pipe()
  drm/i915: turn on the power well before suspending
  drm/i915: set TRANSCODER_EDP even earlier
  drm/i915: print IVB/HSW display error interrupts
  drm/i915: remove "unclaimed register" checks from I915_WRITE

 drivers/gpu/drm/i915/i915_drv.c      |   10 +----
 drivers/gpu/drm/i915/i915_irq.c      |   56 ++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_reg.h      |   31 ++++++++++++---
 drivers/gpu/drm/i915/i915_suspend.c  |    6 ++-
 drivers/gpu/drm/i915/intel_ddi.c     |    8 +++-
 drivers/gpu/drm/i915/intel_display.c |   58 ++++++++++++++++++++++------
 drivers/gpu/drm/i915/intel_dp.c      |    9 +++--
 drivers/gpu/drm/i915/intel_drv.h     |    3 +-
 drivers/gpu/drm/i915/intel_fb.c      |    3 +-
 drivers/gpu/drm/i915/intel_pm.c      |   70 +++++++++++++++++++++++++---------
 10 files changed, 202 insertions(+), 52 deletions(-)

-- 
1.7.10.4

[PATCH 01/10] drm/i915: don't save/restore DSPARB on gen5+

[Paulo Zanoni]

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

Because the register does not exist in gen5+.

This patch solves "unclaimed register" messages on Haswell after
suspend/resume.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_suspend.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 63d4d30..95c0582 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -620,7 +620,8 @@ static void i915_save_display(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	/* Display arbitration control */
-	dev_priv->regfile.saveDSPARB = I915_READ(DSPARB);
+	if (INTEL_INFO(dev)->gen <= 4)
+		dev_priv->regfile.saveDSPARB = I915_READ(DSPARB);
 
 	/* This is only meaningful in non-KMS mode */
 	/* Don't regfile.save them in KMS mode */
@@ -707,7 +708,8 @@ static void i915_restore_display(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	/* Display arbitration */
-	I915_WRITE(DSPARB, dev_priv->regfile.saveDSPARB);
+	if (INTEL_INFO(dev)->gen <= 4)
+		I915_WRITE(DSPARB, dev_priv->regfile.saveDSPARB);
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET)) {
 		/* Display port ratios (must be done before clock is set) */
-- 
1.7.10.4

Re: [PATCH 01/10] drm/i915: don't save/restore DSPARB on gen5+

[Jani Nikula]

On Fri, 18 Jan 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Because the register does not exist in gen5+.
>
> This patch solves "unclaimed register" messages on Haswell after
> suspend/resume.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_suspend.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> index 63d4d30..95c0582 100644
> --- a/drivers/gpu/drm/i915/i915_suspend.c
> +++ b/drivers/gpu/drm/i915/i915_suspend.c
> @@ -620,7 +620,8 @@ static void i915_save_display(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	/* Display arbitration control */
> -	dev_priv->regfile.saveDSPARB = I915_READ(DSPARB);
> +	if (INTEL_INFO(dev)->gen <= 4)
> +		dev_priv->regfile.saveDSPARB = I915_READ(DSPARB);
>  
>  	/* This is only meaningful in non-KMS mode */
>  	/* Don't regfile.save them in KMS mode */
> @@ -707,7 +708,8 @@ static void i915_restore_display(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	/* Display arbitration */
> -	I915_WRITE(DSPARB, dev_priv->regfile.saveDSPARB);
> +	if (INTEL_INFO(dev)->gen <= 4)
> +		I915_WRITE(DSPARB, dev_priv->regfile.saveDSPARB);
>  
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET)) {
>  		/* Display port ratios (must be done before clock is set) */
> -- 
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/10] drm/i915: don't save/restore DSPARB on gen5+

[Daniel Vetter]

On Thu, Jan 24, 2013 at 11:26:13AM +0200, Jani Nikula wrote:
> On Fri, 18 Jan 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > Because the register does not exist in gen5+.
> >
> > This patch solves "unclaimed register" messages on Haswell after
> > suspend/resume.
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 02/10] drm/i915: don't read DP_TP_STATUS(PORT_A)

[Paulo Zanoni]

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

Our documentation is wrong, this register does not exist on port A, so
sleep 800us since it's the timeout mentioned on the mode set sequence
document.

This fixes error messages, including "unclaimed register".

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1492706..d83c279 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1788,9 +1788,12 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
 			temp |= DP_TP_CTL_LINK_TRAIN_IDLE;
 			I915_WRITE(DP_TP_CTL(port), temp);
 
-			if (wait_for((I915_READ(DP_TP_STATUS(port)) &
-				      DP_TP_STATUS_IDLE_DONE), 1))
-				DRM_ERROR("Timed out waiting for DP idle patterns\n");
+			if (port == PORT_A)
+				udelay(800);
+			else
+				if (wait_for((I915_READ(DP_TP_STATUS(port)) &
+					     DP_TP_STATUS_IDLE_DONE), 1))
+					DRM_ERROR("Timed out waiting for DP idle patterns\n");
 
 			temp &= ~DP_TP_CTL_LINK_TRAIN_MASK;
 			temp |= DP_TP_CTL_LINK_TRAIN_NORMAL;
-- 
1.7.10.4

Re: [PATCH 02/10] drm/i915: don't read DP_TP_STATUS(PORT_A)

[Jani Nikula]

On Fri, 18 Jan 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Our documentation is wrong, this register does not exist on port A, so
> sleep 800us since it's the timeout mentioned on the mode set sequence
> document.

Would be nice to have some reference, like documentation, to back up the
claim that the documentation is wrong...

Maybe a comment near #define DP_TP_STATUS() in i915_reg.h would be in
order too.

>
> This fixes error messages, including "unclaimed register".
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 1492706..d83c279 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1788,9 +1788,12 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
>  			temp |= DP_TP_CTL_LINK_TRAIN_IDLE;
>  			I915_WRITE(DP_TP_CTL(port), temp);
>  
> -			if (wait_for((I915_READ(DP_TP_STATUS(port)) &
> -				      DP_TP_STATUS_IDLE_DONE), 1))
> -				DRM_ERROR("Timed out waiting for DP idle patterns\n");
> +			if (port == PORT_A)
> +				udelay(800);

usleep_range()?

> +			else
> +				if (wait_for((I915_READ(DP_TP_STATUS(port)) &
> +					     DP_TP_STATUS_IDLE_DONE), 1))
> +					DRM_ERROR("Timed out waiting for DP idle patterns\n");
>  
>  			temp &= ~DP_TP_CTL_LINK_TRAIN_MASK;
>  			temp |= DP_TP_CTL_LINK_TRAIN_NORMAL;
> -- 
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 03/10] drm/i915: fix intel_init_power_wells

[Paulo Zanoni]

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

The current code was wrong in many different ways, so this is a full
rewrite. We don't have "different power wells for different parts of
the GPU", we have a single power well, but we have multiple registers
that can be used to request enabling/disabling the power well. So
let's be a good citizen and only use the register we're supposed to
use, except when we're loading the driver, where we clear the request
made by the BIOS.

If any of the registers is requesting the power well to be enabled, it
will be enabled. If none of the registers is requesting the power well
to be enabled, it will be disabled.

For now we're just forcing the power well to be enabled, but in the
next commits we'll change this.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |    8 ++--
 drivers/gpu/drm/i915/intel_display.c |    5 +--
 drivers/gpu/drm/i915/intel_drv.h     |    2 +-
 drivers/gpu/drm/i915/intel_pm.c      |   70 +++++++++++++++++++++++++---------
 4 files changed, 59 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 2521617..f054554 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4414,10 +4414,10 @@
 #define   AUDIO_CP_READY_C		(1<<9)
 
 /* HSW Power Wells */
-#define HSW_PWR_WELL_CTL1			0x45400 /* BIOS */
-#define HSW_PWR_WELL_CTL2			0x45404 /* Driver */
-#define HSW_PWR_WELL_CTL3			0x45408 /* KVMR */
-#define HSW_PWR_WELL_CTL4			0x4540C /* Debug */
+#define HSW_PWR_WELL_BIOS			0x45400 /* CTL1 */
+#define HSW_PWR_WELL_DRIVER			0x45404 /* CTL2 */
+#define HSW_PWR_WELL_KVMR			0x45408 /* CTL3 */
+#define HSW_PWR_WELL_DEBUG			0x4540C /* CTL4 */
 #define   HSW_PWR_WELL_ENABLE			(1<<31)
 #define   HSW_PWR_WELL_STATE			(1<<30)
 #define HSW_PWR_WELL_CTL5			0x45410
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b35902e..4a9f048 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8647,10 +8647,7 @@ static void i915_disable_vga(struct drm_device *dev)
 
 void intel_modeset_init_hw(struct drm_device *dev)
 {
-	/* We attempt to init the necessary power wells early in the initialization
-	 * time, so the subsystems that expect power to be enabled can work.
-	 */
-	intel_init_power_wells(dev);
+	intel_init_power_well(dev);
 
 	intel_prepare_ddi(dev);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index aeff0d1..8cfad75 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -666,7 +666,7 @@ extern void intel_update_fbc(struct drm_device *dev);
 extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
 extern void intel_gpu_ips_teardown(void);
 
-extern void intel_init_power_wells(struct drm_device *dev);
+extern void intel_init_power_well(struct drm_device *dev);
 extern void intel_enable_gt_powersave(struct drm_device *dev);
 extern void intel_disable_gt_powersave(struct drm_device *dev);
 extern void gen6_gt_check_fifodbg(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 5a8a72c..2273b9c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4043,33 +4043,69 @@ void intel_init_clock_gating(struct drm_device *dev)
 	dev_priv->display.init_clock_gating(dev);
 }
 
-/* Starting with Haswell, we have different power wells for
- * different parts of the GPU. This attempts to enable them all.
+static void intel_set_power_well(struct drm_device *dev, bool enable)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	bool is_enabled, enable_requested;
+	uint32_t tmp;
+
+	tmp = I915_READ(HSW_PWR_WELL_DRIVER);
+	is_enabled = !!(tmp & HSW_PWR_WELL_STATE);
+	enable_requested = !!(tmp & HSW_PWR_WELL_ENABLE);
+
+	if (enable) {
+		if (!enable_requested)
+			I915_WRITE(HSW_PWR_WELL_DRIVER, HSW_PWR_WELL_ENABLE);
+
+		if (!is_enabled) {
+			DRM_DEBUG_KMS("Enabling power well\n");
+			if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) &
+				      HSW_PWR_WELL_STATE), 20))
+				DRM_ERROR("Timeout enabling power well\n");
+		}
+	} else {
+		if (enable_requested)
+			I915_WRITE(HSW_PWR_WELL_DRIVER, 0);
+
+		if (is_enabled) {
+			if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE)
+				DRM_DEBUG_KMS("Not disabling power well: requested by BIOS\n");
+			else if (I915_READ(HSW_PWR_WELL_KVMR) & HSW_PWR_WELL_ENABLE)
+				DRM_DEBUG_KMS("Not disabling power well: requested by KVMR\n");
+			else if (I915_READ(HSW_PWR_WELL_DEBUG) & HSW_PWR_WELL_ENABLE)
+				DRM_DEBUG_KMS("Not disabling power well: requested by DEBUG\n");
+			else {
+				DRM_DEBUG_KMS("Disabling power well\n");
+				if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) &
+					      HSW_PWR_WELL_STATE) == 0, 20))
+					DRM_ERROR("Timeout disabling power well\n");
+			}
+		}
+	}
+}
+
+/*
+ * Starting with Haswell, we have a "Power Down Well" that can be turned off
+ * when not needed anymore. We have 4 registers that can request the power well
+ * to be enabled, and it will only be disabled if none of the registers is
+ * requesting it to be enabled.
  */
-void intel_init_power_wells(struct drm_device *dev)
+void intel_init_power_well(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	unsigned long power_wells[] = {
-		HSW_PWR_WELL_CTL1,
-		HSW_PWR_WELL_CTL2,
-		HSW_PWR_WELL_CTL4
-	};
-	int i;
 
 	if (!IS_HASWELL(dev))
 		return;
 
 	mutex_lock(&dev->struct_mutex);
 
-	for (i = 0; i < ARRAY_SIZE(power_wells); i++) {
-		int well = I915_READ(power_wells[i]);
+	/* For now, we need the power well to be always enabled. */
+	intel_set_power_well(dev, true);
 
-		if ((well & HSW_PWR_WELL_STATE) == 0) {
-			I915_WRITE(power_wells[i], well & HSW_PWR_WELL_ENABLE);
-			if (wait_for((I915_READ(power_wells[i]) & HSW_PWR_WELL_STATE), 20))
-				DRM_ERROR("Error enabling power well %lx\n", power_wells[i]);
-		}
-	}
+	/* We're taking over the BIOS, so clear any requests made by it since
+	 * the driver is in charge now. */
+	if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE)
+		I915_WRITE(HSW_PWR_WELL_BIOS, 0);
 
 	mutex_unlock(&dev->struct_mutex);
 }
-- 
1.7.10.4

Re: [PATCH 03/10] drm/i915: fix intel_init_power_wells

[Ville Syrjälä]

On Fri, Jan 18, 2013 at 06:29:05PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> The current code was wrong in many different ways, so this is a full
> rewrite. We don't have "different power wells for different parts of
> the GPU", we have a single power well, but we have multiple registers
> that can be used to request enabling/disabling the power well. So
> let's be a good citizen and only use the register we're supposed to
> use, except when we're loading the driver, where we clear the request
> made by the BIOS.
> 
> If any of the registers is requesting the power well to be enabled, it
> will be enabled. If none of the registers is requesting the power well
> to be enabled, it will be disabled.
> 
> For now we're just forcing the power well to be enabled, but in the
> next commits we'll change this.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |    8 ++--
>  drivers/gpu/drm/i915/intel_display.c |    5 +--
>  drivers/gpu/drm/i915/intel_drv.h     |    2 +-
>  drivers/gpu/drm/i915/intel_pm.c      |   70 +++++++++++++++++++++++++---------
>  4 files changed, 59 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 2521617..f054554 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4414,10 +4414,10 @@
>  #define   AUDIO_CP_READY_C		(1<<9)
>  
>  /* HSW Power Wells */
> -#define HSW_PWR_WELL_CTL1			0x45400 /* BIOS */
> -#define HSW_PWR_WELL_CTL2			0x45404 /* Driver */
> -#define HSW_PWR_WELL_CTL3			0x45408 /* KVMR */
> -#define HSW_PWR_WELL_CTL4			0x4540C /* Debug */
> +#define HSW_PWR_WELL_BIOS			0x45400 /* CTL1 */
> +#define HSW_PWR_WELL_DRIVER			0x45404 /* CTL2 */
> +#define HSW_PWR_WELL_KVMR			0x45408 /* CTL3 */
> +#define HSW_PWR_WELL_DEBUG			0x4540C /* CTL4 */
>  #define   HSW_PWR_WELL_ENABLE			(1<<31)
>  #define   HSW_PWR_WELL_STATE			(1<<30)
>  #define HSW_PWR_WELL_CTL5			0x45410
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b35902e..4a9f048 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8647,10 +8647,7 @@ static void i915_disable_vga(struct drm_device *dev)
>  
>  void intel_modeset_init_hw(struct drm_device *dev)
>  {
> -	/* We attempt to init the necessary power wells early in the initialization
> -	 * time, so the subsystems that expect power to be enabled can work.
> -	 */
> -	intel_init_power_wells(dev);
> +	intel_init_power_well(dev);
>  
>  	intel_prepare_ddi(dev);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index aeff0d1..8cfad75 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -666,7 +666,7 @@ extern void intel_update_fbc(struct drm_device *dev);
>  extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
>  extern void intel_gpu_ips_teardown(void);
>  
> -extern void intel_init_power_wells(struct drm_device *dev);
> +extern void intel_init_power_well(struct drm_device *dev);
>  extern void intel_enable_gt_powersave(struct drm_device *dev);
>  extern void intel_disable_gt_powersave(struct drm_device *dev);
>  extern void gen6_gt_check_fifodbg(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 5a8a72c..2273b9c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4043,33 +4043,69 @@ void intel_init_clock_gating(struct drm_device *dev)
>  	dev_priv->display.init_clock_gating(dev);
>  }
>  
> -/* Starting with Haswell, we have different power wells for
> - * different parts of the GPU. This attempts to enable them all.
> +static void intel_set_power_well(struct drm_device *dev, bool enable)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	bool is_enabled, enable_requested;
> +	uint32_t tmp;
> +
> +	tmp = I915_READ(HSW_PWR_WELL_DRIVER);
> +	is_enabled = !!(tmp & HSW_PWR_WELL_STATE);
> +	enable_requested = !!(tmp & HSW_PWR_WELL_ENABLE);
> +
> +	if (enable) {
> +		if (!enable_requested)
> +			I915_WRITE(HSW_PWR_WELL_DRIVER, HSW_PWR_WELL_ENABLE);
> +
> +		if (!is_enabled) {
> +			DRM_DEBUG_KMS("Enabling power well\n");
> +			if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) &
> +				      HSW_PWR_WELL_STATE), 20))
> +				DRM_ERROR("Timeout enabling power well\n");
> +		}
> +	} else {
> +		if (enable_requested)
> +			I915_WRITE(HSW_PWR_WELL_DRIVER, 0);
> +
> +		if (is_enabled) {
> +			if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE)
> +				DRM_DEBUG_KMS("Not disabling power well: requested by BIOS\n");
> +			else if (I915_READ(HSW_PWR_WELL_KVMR) & HSW_PWR_WELL_ENABLE)
> +				DRM_DEBUG_KMS("Not disabling power well: requested by KVMR\n");
> +			else if (I915_READ(HSW_PWR_WELL_DEBUG) & HSW_PWR_WELL_ENABLE)
> +				DRM_DEBUG_KMS("Not disabling power well: requested by DEBUG\n");
> +			else {
> +				DRM_DEBUG_KMS("Disabling power well\n");
> +				if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) &
> +					      HSW_PWR_WELL_STATE) == 0, 20))
> +					DRM_ERROR("Timeout disabling power well\n");
> +			}
> +		}
> +	}

The documentation says not to touch the enable bits if there's a state
transition already in progress. I have no idea how we're supposed to do
that without all kinds of race conditions due multiple entities making
requests simultaneosly. Do you know something about this that's not in
the documentation?

Apart from that the patch looks good to me.

> +}
> +
> +/*
> + * Starting with Haswell, we have a "Power Down Well" that can be turned off
> + * when not needed anymore. We have 4 registers that can request the power well
> + * to be enabled, and it will only be disabled if none of the registers is
> + * requesting it to be enabled.
>   */
> -void intel_init_power_wells(struct drm_device *dev)
> +void intel_init_power_well(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	unsigned long power_wells[] = {
> -		HSW_PWR_WELL_CTL1,
> -		HSW_PWR_WELL_CTL2,
> -		HSW_PWR_WELL_CTL4
> -	};
> -	int i;
>  
>  	if (!IS_HASWELL(dev))
>  		return;
>  
>  	mutex_lock(&dev->struct_mutex);
>  
> -	for (i = 0; i < ARRAY_SIZE(power_wells); i++) {
> -		int well = I915_READ(power_wells[i]);
> +	/* For now, we need the power well to be always enabled. */
> +	intel_set_power_well(dev, true);
>  
> -		if ((well & HSW_PWR_WELL_STATE) == 0) {
> -			I915_WRITE(power_wells[i], well & HSW_PWR_WELL_ENABLE);
> -			if (wait_for((I915_READ(power_wells[i]) & HSW_PWR_WELL_STATE), 20))
> -				DRM_ERROR("Error enabling power well %lx\n", power_wells[i]);
> -		}
> -	}
> +	/* We're taking over the BIOS, so clear any requests made by it since
> +	 * the driver is in charge now. */
> +	if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE)
> +		I915_WRITE(HSW_PWR_WELL_BIOS, 0);
>  
>  	mutex_unlock(&dev->struct_mutex);
>  }
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 03/10] drm/i915: fix intel_init_power_wells

[Daniel Vetter]

On Mon, Jan 21, 2013 at 03:37:42PM +0200, Ville Syrjälä wrote:
> On Fri, Jan 18, 2013 at 06:29:05PM -0200, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > The current code was wrong in many different ways, so this is a full
> > rewrite. We don't have "different power wells for different parts of
> > the GPU", we have a single power well, but we have multiple registers
> > that can be used to request enabling/disabling the power well. So
> > let's be a good citizen and only use the register we're supposed to
> > use, except when we're loading the driver, where we clear the request
> > made by the BIOS.
> > 
> > If any of the registers is requesting the power well to be enabled, it
> > will be enabled. If none of the registers is requesting the power well
> > to be enabled, it will be disabled.
> > 
> > For now we're just forcing the power well to be enabled, but in the
> > next commits we'll change this.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h      |    8 ++--
> >  drivers/gpu/drm/i915/intel_display.c |    5 +--
> >  drivers/gpu/drm/i915/intel_drv.h     |    2 +-
> >  drivers/gpu/drm/i915/intel_pm.c      |   70 +++++++++++++++++++++++++---------
> >  4 files changed, 59 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 2521617..f054554 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4414,10 +4414,10 @@
> >  #define   AUDIO_CP_READY_C		(1<<9)
> >  
> >  /* HSW Power Wells */
> > -#define HSW_PWR_WELL_CTL1			0x45400 /* BIOS */
> > -#define HSW_PWR_WELL_CTL2			0x45404 /* Driver */
> > -#define HSW_PWR_WELL_CTL3			0x45408 /* KVMR */
> > -#define HSW_PWR_WELL_CTL4			0x4540C /* Debug */
> > +#define HSW_PWR_WELL_BIOS			0x45400 /* CTL1 */
> > +#define HSW_PWR_WELL_DRIVER			0x45404 /* CTL2 */
> > +#define HSW_PWR_WELL_KVMR			0x45408 /* CTL3 */
> > +#define HSW_PWR_WELL_DEBUG			0x4540C /* CTL4 */
> >  #define   HSW_PWR_WELL_ENABLE			(1<<31)
> >  #define   HSW_PWR_WELL_STATE			(1<<30)
> >  #define HSW_PWR_WELL_CTL5			0x45410
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index b35902e..4a9f048 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -8647,10 +8647,7 @@ static void i915_disable_vga(struct drm_device *dev)
> >  
> >  void intel_modeset_init_hw(struct drm_device *dev)
> >  {
> > -	/* We attempt to init the necessary power wells early in the initialization
> > -	 * time, so the subsystems that expect power to be enabled can work.
> > -	 */
> > -	intel_init_power_wells(dev);
> > +	intel_init_power_well(dev);
> >  
> >  	intel_prepare_ddi(dev);
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index aeff0d1..8cfad75 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -666,7 +666,7 @@ extern void intel_update_fbc(struct drm_device *dev);
> >  extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
> >  extern void intel_gpu_ips_teardown(void);
> >  
> > -extern void intel_init_power_wells(struct drm_device *dev);
> > +extern void intel_init_power_well(struct drm_device *dev);
> >  extern void intel_enable_gt_powersave(struct drm_device *dev);
> >  extern void intel_disable_gt_powersave(struct drm_device *dev);
> >  extern void gen6_gt_check_fifodbg(struct drm_i915_private *dev_priv);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 5a8a72c..2273b9c 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4043,33 +4043,69 @@ void intel_init_clock_gating(struct drm_device *dev)
> >  	dev_priv->display.init_clock_gating(dev);
> >  }
> >  
> > -/* Starting with Haswell, we have different power wells for
> > - * different parts of the GPU. This attempts to enable them all.
> > +static void intel_set_power_well(struct drm_device *dev, bool enable)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	bool is_enabled, enable_requested;
> > +	uint32_t tmp;
> > +
> > +	tmp = I915_READ(HSW_PWR_WELL_DRIVER);
> > +	is_enabled = !!(tmp & HSW_PWR_WELL_STATE);
> > +	enable_requested = !!(tmp & HSW_PWR_WELL_ENABLE);
> > +
> > +	if (enable) {
> > +		if (!enable_requested)
> > +			I915_WRITE(HSW_PWR_WELL_DRIVER, HSW_PWR_WELL_ENABLE);
> > +
> > +		if (!is_enabled) {
> > +			DRM_DEBUG_KMS("Enabling power well\n");
> > +			if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) &
> > +				      HSW_PWR_WELL_STATE), 20))
> > +				DRM_ERROR("Timeout enabling power well\n");
> > +		}
> > +	} else {
> > +		if (enable_requested)
> > +			I915_WRITE(HSW_PWR_WELL_DRIVER, 0);
> > +
> > +		if (is_enabled) {
> > +			if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE)
> > +				DRM_DEBUG_KMS("Not disabling power well: requested by BIOS\n");
> > +			else if (I915_READ(HSW_PWR_WELL_KVMR) & HSW_PWR_WELL_ENABLE)
> > +				DRM_DEBUG_KMS("Not disabling power well: requested by KVMR\n");
> > +			else if (I915_READ(HSW_PWR_WELL_DEBUG) & HSW_PWR_WELL_ENABLE)
> > +				DRM_DEBUG_KMS("Not disabling power well: requested by DEBUG\n");
> > +			else {
> > +				DRM_DEBUG_KMS("Disabling power well\n");
> > +				if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) &
> > +					      HSW_PWR_WELL_STATE) == 0, 20))
> > +					DRM_ERROR("Timeout disabling power well\n");
> > +			}
> > +		}
> > +	}
> 
> The documentation says not to touch the enable bits if there's a state
> transition already in progress. I have no idea how we're supposed to do
> that without all kinds of race conditions due multiple entities making
> requests simultaneosly. Do you know something about this that's not in
> the documentation?
> 
> Apart from that the patch looks good to me.

We should be protected by dev->mode_config.mutex in all callsite (safe for
maybe setup/resume, but that's just cosmetics). The dev->struct_mutex
locking we already have seems to be cargo-culting afaict. Paulo, can you
add a follow-up patch to unconfuse matters here a bit?

Thanks, Daniel
> 
> > +}
> > +
> > +/*
> > + * Starting with Haswell, we have a "Power Down Well" that can be turned off
> > + * when not needed anymore. We have 4 registers that can request the power well
> > + * to be enabled, and it will only be disabled if none of the registers is
> > + * requesting it to be enabled.
> >   */
> > -void intel_init_power_wells(struct drm_device *dev)
> > +void intel_init_power_well(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	unsigned long power_wells[] = {
> > -		HSW_PWR_WELL_CTL1,
> > -		HSW_PWR_WELL_CTL2,
> > -		HSW_PWR_WELL_CTL4
> > -	};
> > -	int i;
> >  
> >  	if (!IS_HASWELL(dev))
> >  		return;
> >  
> >  	mutex_lock(&dev->struct_mutex);
> >  
> > -	for (i = 0; i < ARRAY_SIZE(power_wells); i++) {
> > -		int well = I915_READ(power_wells[i]);
> > +	/* For now, we need the power well to be always enabled. */
> > +	intel_set_power_well(dev, true);
> >  
> > -		if ((well & HSW_PWR_WELL_STATE) == 0) {
> > -			I915_WRITE(power_wells[i], well & HSW_PWR_WELL_ENABLE);
> > -			if (wait_for((I915_READ(power_wells[i]) & HSW_PWR_WELL_STATE), 20))
> > -				DRM_ERROR("Error enabling power well %lx\n", power_wells[i]);
> > -		}
> > -	}
> > +	/* We're taking over the BIOS, so clear any requests made by it since
> > +	 * the driver is in charge now. */
> > +	if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE)
> > +		I915_WRITE(HSW_PWR_WELL_BIOS, 0);
> >  
> >  	mutex_unlock(&dev->struct_mutex);
> >  }
> > -- 
> > 1.7.10.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 03/10] drm/i915: fix intel_init_power_wells

[Ville Syrjälä]

On Tue, Jan 22, 2013 at 02:02:26PM +0100, Daniel Vetter wrote:
> On Mon, Jan 21, 2013 at 03:37:42PM +0200, Ville Syrjälä wrote:
> > On Fri, Jan 18, 2013 at 06:29:05PM -0200, Paulo Zanoni wrote:
> > > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > 
> > > The current code was wrong in many different ways, so this is a full
> > > rewrite. We don't have "different power wells for different parts of
> > > the GPU", we have a single power well, but we have multiple registers
> > > that can be used to request enabling/disabling the power well. So
> > > let's be a good citizen and only use the register we're supposed to
> > > use, except when we're loading the driver, where we clear the request
> > > made by the BIOS.
> > > 
> > > If any of the registers is requesting the power well to be enabled, it
> > > will be enabled. If none of the registers is requesting the power well
> > > to be enabled, it will be disabled.
> > > 
> > > For now we're just forcing the power well to be enabled, but in the
> > > next commits we'll change this.
> > > 
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h      |    8 ++--
> > >  drivers/gpu/drm/i915/intel_display.c |    5 +--
> > >  drivers/gpu/drm/i915/intel_drv.h     |    2 +-
> > >  drivers/gpu/drm/i915/intel_pm.c      |   70 +++++++++++++++++++++++++---------
> > >  4 files changed, 59 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 2521617..f054554 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -4414,10 +4414,10 @@
> > >  #define   AUDIO_CP_READY_C		(1<<9)
> > >  
> > >  /* HSW Power Wells */
> > > -#define HSW_PWR_WELL_CTL1			0x45400 /* BIOS */
> > > -#define HSW_PWR_WELL_CTL2			0x45404 /* Driver */
> > > -#define HSW_PWR_WELL_CTL3			0x45408 /* KVMR */
> > > -#define HSW_PWR_WELL_CTL4			0x4540C /* Debug */
> > > +#define HSW_PWR_WELL_BIOS			0x45400 /* CTL1 */
> > > +#define HSW_PWR_WELL_DRIVER			0x45404 /* CTL2 */
> > > +#define HSW_PWR_WELL_KVMR			0x45408 /* CTL3 */
> > > +#define HSW_PWR_WELL_DEBUG			0x4540C /* CTL4 */
> > >  #define   HSW_PWR_WELL_ENABLE			(1<<31)
> > >  #define   HSW_PWR_WELL_STATE			(1<<30)
> > >  #define HSW_PWR_WELL_CTL5			0x45410
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index b35902e..4a9f048 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -8647,10 +8647,7 @@ static void i915_disable_vga(struct drm_device *dev)
> > >  
> > >  void intel_modeset_init_hw(struct drm_device *dev)
> > >  {
> > > -	/* We attempt to init the necessary power wells early in the initialization
> > > -	 * time, so the subsystems that expect power to be enabled can work.
> > > -	 */
> > > -	intel_init_power_wells(dev);
> > > +	intel_init_power_well(dev);
> > >  
> > >  	intel_prepare_ddi(dev);
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index aeff0d1..8cfad75 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -666,7 +666,7 @@ extern void intel_update_fbc(struct drm_device *dev);
> > >  extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
> > >  extern void intel_gpu_ips_teardown(void);
> > >  
> > > -extern void intel_init_power_wells(struct drm_device *dev);
> > > +extern void intel_init_power_well(struct drm_device *dev);
> > >  extern void intel_enable_gt_powersave(struct drm_device *dev);
> > >  extern void intel_disable_gt_powersave(struct drm_device *dev);
> > >  extern void gen6_gt_check_fifodbg(struct drm_i915_private *dev_priv);
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 5a8a72c..2273b9c 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -4043,33 +4043,69 @@ void intel_init_clock_gating(struct drm_device *dev)
> > >  	dev_priv->display.init_clock_gating(dev);
> > >  }
> > >  
> > > -/* Starting with Haswell, we have different power wells for
> > > - * different parts of the GPU. This attempts to enable them all.
> > > +static void intel_set_power_well(struct drm_device *dev, bool enable)
> > > +{
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +	bool is_enabled, enable_requested;
> > > +	uint32_t tmp;
> > > +
> > > +	tmp = I915_READ(HSW_PWR_WELL_DRIVER);
> > > +	is_enabled = !!(tmp & HSW_PWR_WELL_STATE);
> > > +	enable_requested = !!(tmp & HSW_PWR_WELL_ENABLE);
> > > +
> > > +	if (enable) {
> > > +		if (!enable_requested)
> > > +			I915_WRITE(HSW_PWR_WELL_DRIVER, HSW_PWR_WELL_ENABLE);
> > > +
> > > +		if (!is_enabled) {
> > > +			DRM_DEBUG_KMS("Enabling power well\n");
> > > +			if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) &
> > > +				      HSW_PWR_WELL_STATE), 20))
> > > +				DRM_ERROR("Timeout enabling power well\n");
> > > +		}
> > > +	} else {
> > > +		if (enable_requested)
> > > +			I915_WRITE(HSW_PWR_WELL_DRIVER, 0);
> > > +
> > > +		if (is_enabled) {
> > > +			if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE)
> > > +				DRM_DEBUG_KMS("Not disabling power well: requested by BIOS\n");
> > > +			else if (I915_READ(HSW_PWR_WELL_KVMR) & HSW_PWR_WELL_ENABLE)
> > > +				DRM_DEBUG_KMS("Not disabling power well: requested by KVMR\n");
> > > +			else if (I915_READ(HSW_PWR_WELL_DEBUG) & HSW_PWR_WELL_ENABLE)
> > > +				DRM_DEBUG_KMS("Not disabling power well: requested by DEBUG\n");
> > > +			else {
> > > +				DRM_DEBUG_KMS("Disabling power well\n");
> > > +				if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) &
> > > +					      HSW_PWR_WELL_STATE) == 0, 20))
> > > +					DRM_ERROR("Timeout disabling power well\n");
> > > +			}
> > > +		}
> > > +	}
> > 
> > The documentation says not to touch the enable bits if there's a state
> > transition already in progress. I have no idea how we're supposed to do
> > that without all kinds of race conditions due multiple entities making
> > requests simultaneosly. Do you know something about this that's not in
> > the documentation?
> > 
> > Apart from that the patch looks good to me.
> 
> We should be protected by dev->mode_config.mutex in all callsite (safe for
> maybe setup/resume, but that's just cosmetics). The dev->struct_mutex
> locking we already have seems to be cargo-culting afaict. Paulo, can you
> add a follow-up patch to unconfuse matters here a bit?

I'm not concerned about out driver, but mainly KVMR. It can
enable/disable the power well at any time. Unless of course KVMR is
never used on Linux machines, in which case my worries are pointless.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 03/10] drm/i915: fix intel_init_power_wells

[Ville Syrjälä]

On Fri, Jan 18, 2013 at 06:29:05PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> The current code was wrong in many different ways, so this is a full
> rewrite. We don't have "different power wells for different parts of
> the GPU", we have a single power well, but we have multiple registers
> that can be used to request enabling/disabling the power well. So
> let's be a good citizen and only use the register we're supposed to
> use, except when we're loading the driver, where we clear the request
> made by the BIOS.
> 
> If any of the registers is requesting the power well to be enabled, it
> will be enabled. If none of the registers is requesting the power well
> to be enabled, it will be disabled.
> 
> For now we're just forcing the power well to be enabled, but in the
> next commits we'll change this.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |    8 ++--
>  drivers/gpu/drm/i915/intel_display.c |    5 +--
>  drivers/gpu/drm/i915/intel_drv.h     |    2 +-
>  drivers/gpu/drm/i915/intel_pm.c      |   70 +++++++++++++++++++++++++---------
>  4 files changed, 59 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 2521617..f054554 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4414,10 +4414,10 @@
>  #define   AUDIO_CP_READY_C		(1<<9)
>  
>  /* HSW Power Wells */
> -#define HSW_PWR_WELL_CTL1			0x45400 /* BIOS */
> -#define HSW_PWR_WELL_CTL2			0x45404 /* Driver */
> -#define HSW_PWR_WELL_CTL3			0x45408 /* KVMR */
> -#define HSW_PWR_WELL_CTL4			0x4540C /* Debug */
> +#define HSW_PWR_WELL_BIOS			0x45400 /* CTL1 */
> +#define HSW_PWR_WELL_DRIVER			0x45404 /* CTL2 */
> +#define HSW_PWR_WELL_KVMR			0x45408 /* CTL3 */
> +#define HSW_PWR_WELL_DEBUG			0x4540C /* CTL4 */
>  #define   HSW_PWR_WELL_ENABLE			(1<<31)
>  #define   HSW_PWR_WELL_STATE			(1<<30)
>  #define HSW_PWR_WELL_CTL5			0x45410
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b35902e..4a9f048 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8647,10 +8647,7 @@ static void i915_disable_vga(struct drm_device *dev)
>  
>  void intel_modeset_init_hw(struct drm_device *dev)
>  {
> -	/* We attempt to init the necessary power wells early in the initialization
> -	 * time, so the subsystems that expect power to be enabled can work.
> -	 */
> -	intel_init_power_wells(dev);
> +	intel_init_power_well(dev);
>  
>  	intel_prepare_ddi(dev);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index aeff0d1..8cfad75 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -666,7 +666,7 @@ extern void intel_update_fbc(struct drm_device *dev);
>  extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
>  extern void intel_gpu_ips_teardown(void);
>  
> -extern void intel_init_power_wells(struct drm_device *dev);
> +extern void intel_init_power_well(struct drm_device *dev);
>  extern void intel_enable_gt_powersave(struct drm_device *dev);
>  extern void intel_disable_gt_powersave(struct drm_device *dev);
>  extern void gen6_gt_check_fifodbg(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 5a8a72c..2273b9c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4043,33 +4043,69 @@ void intel_init_clock_gating(struct drm_device *dev)
>  	dev_priv->display.init_clock_gating(dev);
>  }
>  
> -/* Starting with Haswell, we have different power wells for
> - * different parts of the GPU. This attempts to enable them all.
> +static void intel_set_power_well(struct drm_device *dev, bool enable)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	bool is_enabled, enable_requested;
> +	uint32_t tmp;
> +
> +	tmp = I915_READ(HSW_PWR_WELL_DRIVER);
> +	is_enabled = !!(tmp & HSW_PWR_WELL_STATE);
> +	enable_requested = !!(tmp & HSW_PWR_WELL_ENABLE);
> +
> +	if (enable) {
> +		if (!enable_requested)
> +			I915_WRITE(HSW_PWR_WELL_DRIVER, HSW_PWR_WELL_ENABLE);
> +
> +		if (!is_enabled) {
> +			DRM_DEBUG_KMS("Enabling power well\n");
> +			if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) &
> +				      HSW_PWR_WELL_STATE), 20))
> +				DRM_ERROR("Timeout enabling power well\n");
> +		}
> +	} else {
> +		if (enable_requested)
> +			I915_WRITE(HSW_PWR_WELL_DRIVER, 0);
> +
> +		if (is_enabled) {
> +			if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE)
> +				DRM_DEBUG_KMS("Not disabling power well: requested by BIOS\n");
> +			else if (I915_READ(HSW_PWR_WELL_KVMR) & HSW_PWR_WELL_ENABLE)
> +				DRM_DEBUG_KMS("Not disabling power well: requested by KVMR\n");
> +			else if (I915_READ(HSW_PWR_WELL_DEBUG) & HSW_PWR_WELL_ENABLE)
> +				DRM_DEBUG_KMS("Not disabling power well: requested by DEBUG\n");
> +			else {
> +				DRM_DEBUG_KMS("Disabling power well\n");
> +				if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) &
> +					      HSW_PWR_WELL_STATE) == 0, 20))
> +					DRM_ERROR("Timeout disabling power well\n");
> +			}

After some irc discussions Daniel came to the conclusion that the
status bit is likely just a delayed version of the request bit. I
tend to agree, nothing else makes much sense. So we should just
always poll the status bit after changing the request bit, and
completely ignore the KVMR/BIOS/DEBUG registers.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 03/10] drm/i915: fix intel_init_power_wells

[Jani Nikula]

On Fri, 18 Jan 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> The current code was wrong in many different ways, so this is a full
> rewrite. We don't have "different power wells for different parts of
> the GPU", we have a single power well, but we have multiple registers
> that can be used to request enabling/disabling the power well. So
> let's be a good citizen and only use the register we're supposed to
> use, except when we're loading the driver, where we clear the request
> made by the BIOS.
>
> If any of the registers is requesting the power well to be enabled, it
> will be enabled. If none of the registers is requesting the power well
> to be enabled, it will be disabled.
>
> For now we're just forcing the power well to be enabled, but in the
> next commits we'll change this.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |    8 ++--
>  drivers/gpu/drm/i915/intel_display.c |    5 +--
>  drivers/gpu/drm/i915/intel_drv.h     |    2 +-
>  drivers/gpu/drm/i915/intel_pm.c      |   70 +++++++++++++++++++++++++---------
>  4 files changed, 59 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 2521617..f054554 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4414,10 +4414,10 @@
>  #define   AUDIO_CP_READY_C		(1<<9)
>  
>  /* HSW Power Wells */
> -#define HSW_PWR_WELL_CTL1			0x45400 /* BIOS */
> -#define HSW_PWR_WELL_CTL2			0x45404 /* Driver */
> -#define HSW_PWR_WELL_CTL3			0x45408 /* KVMR */
> -#define HSW_PWR_WELL_CTL4			0x4540C /* Debug */
> +#define HSW_PWR_WELL_BIOS			0x45400 /* CTL1 */
> +#define HSW_PWR_WELL_DRIVER			0x45404 /* CTL2 */
> +#define HSW_PWR_WELL_KVMR			0x45408 /* CTL3 */
> +#define HSW_PWR_WELL_DEBUG			0x4540C /* CTL4 */
>  #define   HSW_PWR_WELL_ENABLE			(1<<31)
>  #define   HSW_PWR_WELL_STATE			(1<<30)
>  #define HSW_PWR_WELL_CTL5			0x45410
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b35902e..4a9f048 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8647,10 +8647,7 @@ static void i915_disable_vga(struct drm_device *dev)
>  
>  void intel_modeset_init_hw(struct drm_device *dev)
>  {
> -	/* We attempt to init the necessary power wells early in the initialization
> -	 * time, so the subsystems that expect power to be enabled can work.
> -	 */
> -	intel_init_power_wells(dev);
> +	intel_init_power_well(dev);
>  
>  	intel_prepare_ddi(dev);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index aeff0d1..8cfad75 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -666,7 +666,7 @@ extern void intel_update_fbc(struct drm_device *dev);
>  extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
>  extern void intel_gpu_ips_teardown(void);
>  
> -extern void intel_init_power_wells(struct drm_device *dev);
> +extern void intel_init_power_well(struct drm_device *dev);
>  extern void intel_enable_gt_powersave(struct drm_device *dev);
>  extern void intel_disable_gt_powersave(struct drm_device *dev);
>  extern void gen6_gt_check_fifodbg(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 5a8a72c..2273b9c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4043,33 +4043,69 @@ void intel_init_clock_gating(struct drm_device *dev)
>  	dev_priv->display.init_clock_gating(dev);
>  }
>  
> -/* Starting with Haswell, we have different power wells for
> - * different parts of the GPU. This attempts to enable them all.
> +static void intel_set_power_well(struct drm_device *dev, bool enable)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	bool is_enabled, enable_requested;
> +	uint32_t tmp;
> +
> +	tmp = I915_READ(HSW_PWR_WELL_DRIVER);
> +	is_enabled = !!(tmp & HSW_PWR_WELL_STATE);
> +	enable_requested = !!(tmp & HSW_PWR_WELL_ENABLE);
> +
> +	if (enable) {
> +		if (!enable_requested)
> +			I915_WRITE(HSW_PWR_WELL_DRIVER, HSW_PWR_WELL_ENABLE);
> +
> +		if (!is_enabled) {
> +			DRM_DEBUG_KMS("Enabling power well\n");
> +			if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) &
> +				      HSW_PWR_WELL_STATE), 20))
> +				DRM_ERROR("Timeout enabling power well\n");
> +		}
> +	} else {
> +		if (enable_requested)
> +			I915_WRITE(HSW_PWR_WELL_DRIVER, 0);
> +
> +		if (is_enabled) {
> +			if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE)
> +				DRM_DEBUG_KMS("Not disabling power well: requested by BIOS\n");
> +			else if (I915_READ(HSW_PWR_WELL_KVMR) & HSW_PWR_WELL_ENABLE)
> +				DRM_DEBUG_KMS("Not disabling power well: requested by KVMR\n");
> +			else if (I915_READ(HSW_PWR_WELL_DEBUG) & HSW_PWR_WELL_ENABLE)
> +				DRM_DEBUG_KMS("Not disabling power well: requested by DEBUG\n");
> +			else {
> +				DRM_DEBUG_KMS("Disabling power well\n");
> +				if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) &
> +					      HSW_PWR_WELL_STATE) == 0, 20))
> +					DRM_ERROR("Timeout disabling power well\n");
> +			}
> +		}
> +	}
> +}
> +
> +/*
> + * Starting with Haswell, we have a "Power Down Well" that can be turned off
> + * when not needed anymore. We have 4 registers that can request the power well
> + * to be enabled, and it will only be disabled if none of the registers is
> + * requesting it to be enabled.
>   */
> -void intel_init_power_wells(struct drm_device *dev)
> +void intel_init_power_well(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	unsigned long power_wells[] = {
> -		HSW_PWR_WELL_CTL1,
> -		HSW_PWR_WELL_CTL2,
> -		HSW_PWR_WELL_CTL4
> -	};
> -	int i;
>  
>  	if (!IS_HASWELL(dev))
>  		return;
>  
>  	mutex_lock(&dev->struct_mutex);
>  
> -	for (i = 0; i < ARRAY_SIZE(power_wells); i++) {
> -		int well = I915_READ(power_wells[i]);
> +	/* For now, we need the power well to be always enabled. */
> +	intel_set_power_well(dev, true);
>  
> -		if ((well & HSW_PWR_WELL_STATE) == 0) {
> -			I915_WRITE(power_wells[i], well & HSW_PWR_WELL_ENABLE);
> -			if (wait_for((I915_READ(power_wells[i]) & HSW_PWR_WELL_STATE), 20))
> -				DRM_ERROR("Error enabling power well %lx\n", power_wells[i]);
> -		}
> -	}
> +	/* We're taking over the BIOS, so clear any requests made by it since
> +	 * the driver is in charge now. */
> +	if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE)
> +		I915_WRITE(HSW_PWR_WELL_BIOS, 0);
>  
>  	mutex_unlock(&dev->struct_mutex);
>  }
> -- 
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 04/10] drm/i915: dynamic Haswell display power well support

[Paulo Zanoni]

From: Daniel Vetter <daniel.vetter@ffwll.ch>

We can disable (almost) all the display hw if we only use pipe A, with
the integrated edp transcoder on port A. Because we don't set the cpu
transcoder that early (yet), we need to help us with a trick to simply
check for any edp encoders.

And wrt the old code: Can anyone explain what that struct mutex
grabbing was supposed to protect?

v2: Paulo Zanoni pointed out that we also need to configure the eDP
cpu transcoder correctly.

v3: Made by Paulo Zanoni
  - Rebase patch to be on top of "fix intel_init_power_wells" patch
  - Fix typos
  - Fix a small bug by adding a "connectors_active" check
  - Restore the initial code that unconditionally enables the power
    well when taking over from the BIOS

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c     |    8 +++++++-
 drivers/gpu/drm/i915/intel_display.c |   31 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |    1 +
 drivers/gpu/drm/i915/intel_pm.c      |    2 +-
 4 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 2e904a5..a4dd55a 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -987,7 +987,13 @@ void intel_ddi_enable_pipe_func(struct drm_crtc *crtc)
 	if (cpu_transcoder == TRANSCODER_EDP) {
 		switch (pipe) {
 		case PIPE_A:
-			temp |= TRANS_DDI_EDP_INPUT_A_ONOFF;
+			/* Can only use the always-on power well for eDP when
+			 * not using the panel fitter, and when not using motion
+			 * blur mitigation (which we don't support). */
+			if (dev_priv->pch_pf_size)
+				temp |= TRANS_DDI_EDP_INPUT_A_ONOFF;
+			else
+				temp |= TRANS_DDI_EDP_INPUT_A_ON;
 			break;
 		case PIPE_B:
 			temp |= TRANS_DDI_EDP_INPUT_B_ONOFF;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4a9f048..a7fb7e1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5595,6 +5595,35 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	return fdi_config_ok ? ret : -EINVAL;
 }
 
+static void haswell_modeset_global_resources(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	bool enable = false;
+	struct intel_crtc *crtc;
+	struct intel_encoder *encoder;
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) {
+		if (crtc->pipe != PIPE_A && crtc->base.enabled)
+			enable = true;
+		/* XXX: Should check for edp transcoder here, but thanks to init
+		 * sequence that's not yet availble. Just in case desktop eDP on
+		 * PORT D is possible on haswell, too. */
+	}
+
+	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
+			    base.head) {
+		if (encoder->type != INTEL_OUTPUT_EDP &&
+		    encoder->connectors_active)
+			enable = true;
+	}
+
+	/* Even the eDP panel fitter is outside the always-on well. */
+	if (dev_priv->pch_pf_size)
+		enable = true;
+
+	intel_set_power_well(dev, enable);
+}
+
 static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 				 struct drm_display_mode *mode,
 				 struct drm_display_mode *adjusted_mode,
@@ -8477,6 +8506,8 @@ static void intel_init_display(struct drm_device *dev)
 		} else if (IS_HASWELL(dev)) {
 			dev_priv->display.fdi_link_train = hsw_fdi_link_train;
 			dev_priv->display.write_eld = haswell_write_eld;
+			dev_priv->display.modeset_global_resources =
+				haswell_modeset_global_resources;
 		}
 	} else if (IS_G4X(dev)) {
 		dev_priv->display.write_eld = g4x_write_eld;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8cfad75..7cea8e2 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -667,6 +667,7 @@ extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
 extern void intel_gpu_ips_teardown(void);
 
 extern void intel_init_power_well(struct drm_device *dev);
+extern void intel_set_power_well(struct drm_device *dev, bool enable);
 extern void intel_enable_gt_powersave(struct drm_device *dev);
 extern void intel_disable_gt_powersave(struct drm_device *dev);
 extern void gen6_gt_check_fifodbg(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2273b9c..9a4f754 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4043,7 +4043,7 @@ void intel_init_clock_gating(struct drm_device *dev)
 	dev_priv->display.init_clock_gating(dev);
 }
 
-static void intel_set_power_well(struct drm_device *dev, bool enable)
+void intel_set_power_well(struct drm_device *dev, bool enable)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	bool is_enabled, enable_requested;
-- 
1.7.10.4

Re: [PATCH 04/10] drm/i915: dynamic Haswell display power well support

[Jani Nikula]

On Fri, 18 Jan 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> We can disable (almost) all the display hw if we only use pipe A, with
> the integrated edp transcoder on port A. Because we don't set the cpu
> transcoder that early (yet), we need to help us with a trick to simply
> check for any edp encoders.
>
> And wrt the old code: Can anyone explain what that struct mutex
> grabbing was supposed to protect?
>
> v2: Paulo Zanoni pointed out that we also need to configure the eDP
> cpu transcoder correctly.
>
> v3: Made by Paulo Zanoni
>   - Rebase patch to be on top of "fix intel_init_power_wells" patch
>   - Fix typos
>   - Fix a small bug by adding a "connectors_active" check
>   - Restore the initial code that unconditionally enables the power
>     well when taking over from the BIOS
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c     |    8 +++++++-
>  drivers/gpu/drm/i915/intel_display.c |   31 +++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |    1 +
>  drivers/gpu/drm/i915/intel_pm.c      |    2 +-
>  4 files changed, 40 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 2e904a5..a4dd55a 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -987,7 +987,13 @@ void intel_ddi_enable_pipe_func(struct drm_crtc *crtc)
>  	if (cpu_transcoder == TRANSCODER_EDP) {
>  		switch (pipe) {
>  		case PIPE_A:
> -			temp |= TRANS_DDI_EDP_INPUT_A_ONOFF;
> +			/* Can only use the always-on power well for eDP when
> +			 * not using the panel fitter, and when not using motion
> +			 * blur mitigation (which we don't support). */
> +			if (dev_priv->pch_pf_size)
> +				temp |= TRANS_DDI_EDP_INPUT_A_ONOFF;
> +			else
> +				temp |= TRANS_DDI_EDP_INPUT_A_ON;
>  			break;
>  		case PIPE_B:
>  			temp |= TRANS_DDI_EDP_INPUT_B_ONOFF;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4a9f048..a7fb7e1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5595,6 +5595,35 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	return fdi_config_ok ? ret : -EINVAL;
>  }
>  
> +static void haswell_modeset_global_resources(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	bool enable = false;
> +	struct intel_crtc *crtc;
> +	struct intel_encoder *encoder;
> +
> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) {
> +		if (crtc->pipe != PIPE_A && crtc->base.enabled)
> +			enable = true;
> +		/* XXX: Should check for edp transcoder here, but thanks to init
> +		 * sequence that's not yet availble. Just in case desktop eDP on
> +		 * PORT D is possible on haswell, too. */

Obligatory nitpick on something: s/availble/available/

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> +	}
> +
> +	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
> +			    base.head) {
> +		if (encoder->type != INTEL_OUTPUT_EDP &&
> +		    encoder->connectors_active)
> +			enable = true;
> +	}
> +
> +	/* Even the eDP panel fitter is outside the always-on well. */
> +	if (dev_priv->pch_pf_size)
> +		enable = true;
> +
> +	intel_set_power_well(dev, enable);
> +}
> +
>  static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>  				 struct drm_display_mode *mode,
>  				 struct drm_display_mode *adjusted_mode,
> @@ -8477,6 +8506,8 @@ static void intel_init_display(struct drm_device *dev)
>  		} else if (IS_HASWELL(dev)) {
>  			dev_priv->display.fdi_link_train = hsw_fdi_link_train;
>  			dev_priv->display.write_eld = haswell_write_eld;
> +			dev_priv->display.modeset_global_resources =
> +				haswell_modeset_global_resources;
>  		}
>  	} else if (IS_G4X(dev)) {
>  		dev_priv->display.write_eld = g4x_write_eld;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 8cfad75..7cea8e2 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -667,6 +667,7 @@ extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
>  extern void intel_gpu_ips_teardown(void);
>  
>  extern void intel_init_power_well(struct drm_device *dev);
> +extern void intel_set_power_well(struct drm_device *dev, bool enable);
>  extern void intel_enable_gt_powersave(struct drm_device *dev);
>  extern void intel_disable_gt_powersave(struct drm_device *dev);
>  extern void gen6_gt_check_fifodbg(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 2273b9c..9a4f754 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4043,7 +4043,7 @@ void intel_init_clock_gating(struct drm_device *dev)
>  	dev_priv->display.init_clock_gating(dev);
>  }
>  
> -static void intel_set_power_well(struct drm_device *dev, bool enable)
> +void intel_set_power_well(struct drm_device *dev, bool enable)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	bool is_enabled, enable_requested;
> -- 
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 05/10] drm/i915: only disable enabled planes on intel_fb_restore_mode

[Paulo Zanoni]

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

We should avoid touching registers that are on the power down well
when we don't need to, because if we touch these registers when the
power well is disabled we'll get tons of "unclaimed register"
messages. This commit fixes some of these messages.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fb.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index ce5f544..6591029 100644
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -304,7 +304,8 @@ void intel_fb_restore_mode(struct drm_device *dev)
 
 	/* Be sure to shut off any planes that may be active */
 	list_for_each_entry(plane, &config->plane_list, head)
-		plane->funcs->disable_plane(plane);
+		if (plane->enabled)
+			plane->funcs->disable_plane(plane);
 
 	mutex_unlock(&dev->mode_config.mutex);
 }
-- 
1.7.10.4

[PATCH 06/10] drm/i915: check the power down well on assert_pipe()

[Paulo Zanoni]

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

If the power well is disabled, we should not try to read its
registers, otherwise we'll get "unclaimed register" messages.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a7fb7e1..921b020 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1214,9 +1214,15 @@ void assert_pipe(struct drm_i915_private *dev_priv,
 	if (pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE)
 		state = true;
 
-	reg = PIPECONF(cpu_transcoder);
-	val = I915_READ(reg);
-	cur_state = !!(val & PIPECONF_ENABLE);
+	if (cpu_transcoder == TRANSCODER_EDP ||
+	    (I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_STATE)) {
+		reg = PIPECONF(cpu_transcoder);
+		val = I915_READ(reg);
+		cur_state = !!(val & PIPECONF_ENABLE);
+	} else {
+		cur_state = false;
+	}
+
 	WARN(cur_state != state,
 	     "pipe %c assertion failure (expected %s, current %s)\n",
 	     pipe_name(pipe), state_string(state), state_string(cur_state));
-- 
1.7.10.4

Re: [PATCH 06/10] drm/i915: check the power down well on assert_pipe()

[Ville Syrjälä]

On Fri, Jan 18, 2013 at 06:29:08PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> If the power well is disabled, we should not try to read its
> registers, otherwise we'll get "unclaimed register" messages.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a7fb7e1..921b020 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1214,9 +1214,15 @@ void assert_pipe(struct drm_i915_private *dev_priv,
>  	if (pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE)
>  		state = true;
>  
> -	reg = PIPECONF(cpu_transcoder);
> -	val = I915_READ(reg);
> -	cur_state = !!(val & PIPECONF_ENABLE);
> +	if (cpu_transcoder == TRANSCODER_EDP ||
> +	    (I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_STATE)) {

Should that also check HSW_PWR_WELL_ENABLE? KVMR might have the well
enabled, while the driver has it disabled. But KVMR might have already
disabled the well, and it might get disabled just after this check,
and then you would hit the unclaimed register issue again.

> +		reg = PIPECONF(cpu_transcoder);
> +		val = I915_READ(reg);
> +		cur_state = !!(val & PIPECONF_ENABLE);
> +	} else {
> +		cur_state = false;
> +	}
> +
>  	WARN(cur_state != state,
>  	     "pipe %c assertion failure (expected %s, current %s)\n",
>  	     pipe_name(pipe), state_string(state), state_string(cur_state));
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 06/10] drm/i915: check the power down well on assert_pipe()

[Daniel Vetter]

On Mon, Jan 21, 2013 at 03:45:48PM +0200, Ville Syrjälä wrote:
> On Fri, Jan 18, 2013 at 06:29:08PM -0200, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > If the power well is disabled, we should not try to read its
> > registers, otherwise we'll get "unclaimed register" messages.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |   12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index a7fb7e1..921b020 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1214,9 +1214,15 @@ void assert_pipe(struct drm_i915_private *dev_priv,
> >  	if (pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE)
> >  		state = true;
> >  
> > -	reg = PIPECONF(cpu_transcoder);
> > -	val = I915_READ(reg);
> > -	cur_state = !!(val & PIPECONF_ENABLE);
> > +	if (cpu_transcoder == TRANSCODER_EDP ||
> > +	    (I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_STATE)) {
> 
> Should that also check HSW_PWR_WELL_ENABLE? KVMR might have the well
> enabled, while the driver has it disabled. But KVMR might have already
> disabled the well, and it might get disabled just after this check,
> and then you would hit the unclaimed register issue again.

The important matter is to not read registers in the power well if it's
off, for which checking just one of the three bits is enough. If the kvm
keeps the power well on, we just avoid checking the pipe state if we don't
need the power well, but otherwise no side effect. Otoh just one set bit
makes sure that the power well is on and we can read the regs).

So looks good to me.
-Daniel
> 
> > +		reg = PIPECONF(cpu_transcoder);
> > +		val = I915_READ(reg);
> > +		cur_state = !!(val & PIPECONF_ENABLE);
> > +	} else {
> > +		cur_state = false;
> > +	}
> > +
> >  	WARN(cur_state != state,
> >  	     "pipe %c assertion failure (expected %s, current %s)\n",
> >  	     pipe_name(pipe), state_string(state), state_string(cur_state));
> > -- 
> > 1.7.10.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 06/10] drm/i915: check the power down well on assert_pipe()

[Ville Syrjälä]

On Tue, Jan 22, 2013 at 02:04:09PM +0100, Daniel Vetter wrote:
> On Mon, Jan 21, 2013 at 03:45:48PM +0200, Ville Syrjälä wrote:
> > On Fri, Jan 18, 2013 at 06:29:08PM -0200, Paulo Zanoni wrote:
> > > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > 
> > > If the power well is disabled, we should not try to read its
> > > registers, otherwise we'll get "unclaimed register" messages.
> > > 
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c |   12 +++++++++---
> > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index a7fb7e1..921b020 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -1214,9 +1214,15 @@ void assert_pipe(struct drm_i915_private *dev_priv,
> > >  	if (pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE)
> > >  		state = true;
> > >  
> > > -	reg = PIPECONF(cpu_transcoder);
> > > -	val = I915_READ(reg);
> > > -	cur_state = !!(val & PIPECONF_ENABLE);
> > > +	if (cpu_transcoder == TRANSCODER_EDP ||
> > > +	    (I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_STATE)) {
> > 
> > Should that also check HSW_PWR_WELL_ENABLE? KVMR might have the well
> > enabled, while the driver has it disabled. But KVMR might have already
> > disabled the well, and it might get disabled just after this check,
> > and then you would hit the unclaimed register issue again.
> 
> The important matter is to not read registers in the power well if it's
> off, for which checking just one of the three bits is enough. If the kvm
> keeps the power well on, we just avoid checking the pipe state if we don't
> need the power well, but otherwise no side effect. Otoh just one set bit
> makes sure that the power well is on and we can read the regs).

The power well may be on when you're reading the status bit, but
assuming it was KVMR who caused the power well to be powered on,
we can't be sure the power well will remain powered long enough
to read the registers.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 06/10] drm/i915: check the power down well on assert_pipe()

[Paulo Zanoni]

Hi

2013/1/21 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> On Fri, Jan 18, 2013 at 06:29:08PM -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> If the power well is disabled, we should not try to read its
>> registers, otherwise we'll get "unclaimed register" messages.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c |   12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index a7fb7e1..921b020 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -1214,9 +1214,15 @@ void assert_pipe(struct drm_i915_private *dev_priv,
>>       if (pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE)
>>               state = true;
>>
>> -     reg = PIPECONF(cpu_transcoder);
>> -     val = I915_READ(reg);
>> -     cur_state = !!(val & PIPECONF_ENABLE);
>> +     if (cpu_transcoder == TRANSCODER_EDP ||
>> +         (I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_STATE)) {
>
> Should that also check HSW_PWR_WELL_ENABLE? KVMR might have the well
> enabled, while the driver has it disabled. But KVMR might have already
> disabled the well, and it might get disabled just after this check,
> and then you would hit the unclaimed register issue again.

By reading your text it seems you think that if bit 31 is 0 then bit
30 will necessarily be 0. This is not true. If any entity (including
KVMr) needs the power well enabled (and is requesting so), then bit 30
of HSW_PWR_WELL_DRIVER will be 1, no matter the status of bit 31.
Another thing to mention is that no entity can force the power well to
be "off", it can only force to be "on".

Still, your text does contain an example of a problem with the current
code. Let me explicitly write it:
- Our driver doesn't need it to be enabled, so bit 31 of
HSW_PWR_WELL_DRIVER is 0
- KVMr needs it enabled, so HSW_PWR_WELL_DRIVER bit 30 is 1
- We do the I915_READ above
- After the read, KVMr says it doesn't need the power well to be
enabled anymore, so it gets disabled because no one else is requesting
it
- Then we touch an unclaimed register.

I think the only policy to avoid these race conditions should be "if I
don't need the power well to be enabled, then I won't touch registers
that require it to be enabled, no matter what the real state of the
power well is".This should prevent any kinds of problems, because if I
need the power well to be enabled, then I requested it to be enabled
and it is enabled because I asked so. I'll send a second version of
the patch with this.

>
>> +             reg = PIPECONF(cpu_transcoder);
>> +             val = I915_READ(reg);
>> +             cur_state = !!(val & PIPECONF_ENABLE);
>> +     } else {
>> +             cur_state = false;
>> +     }
>> +
>>       WARN(cur_state != state,
>>            "pipe %c assertion failure (expected %s, current %s)\n",
>>            pipe_name(pipe), state_string(state), state_string(cur_state));
>> --
>> 1.7.10.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC



-- 
Paulo Zanoni

Re: [PATCH 06/10] drm/i915: check the power down well on assert_pipe()

[Ville Syrjälä]

On Fri, Jan 25, 2013 at 01:40:08PM -0200, Paulo Zanoni wrote:
> Hi
> 
> 2013/1/21 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > On Fri, Jan 18, 2013 at 06:29:08PM -0200, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> If the power well is disabled, we should not try to read its
> >> registers, otherwise we'll get "unclaimed register" messages.
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_display.c |   12 +++++++++---
> >>  1 file changed, 9 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index a7fb7e1..921b020 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -1214,9 +1214,15 @@ void assert_pipe(struct drm_i915_private *dev_priv,
> >>       if (pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE)
> >>               state = true;
> >>
> >> -     reg = PIPECONF(cpu_transcoder);
> >> -     val = I915_READ(reg);
> >> -     cur_state = !!(val & PIPECONF_ENABLE);
> >> +     if (cpu_transcoder == TRANSCODER_EDP ||
> >> +         (I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_STATE)) {
> >
> > Should that also check HSW_PWR_WELL_ENABLE? KVMR might have the well
> > enabled, while the driver has it disabled. But KVMR might have already
> > disabled the well, and it might get disabled just after this check,
> > and then you would hit the unclaimed register issue again.
> 
> By reading your text it seems you think that if bit 31 is 0 then bit
> 30 will necessarily be 0. This is not true. If any entity (including
> KVMr) needs the power well enabled (and is requesting so), then bit 30
> of HSW_PWR_WELL_DRIVER will be 1, no matter the status of bit 31.

Did you verify that on real HW?

It makes little sense when you consider that the spec says not
to toggle the request bit when there's a state transition currently
happening. If all the status bits read as the same thing, then
there's no way to know when a transition is actually happening,
and hence the text in the spec is complete nonsense.

The only sensible interpretation of that text I could think of is
that you're not supposed to abort a state transition you yourself
caused. That would be achieved by polling the status bit after
toggling the request bit, assuming that the status bit is just a
delayed version of request bit.

> Another thing to mention is that no entity can force the power well to
> be "off", it can only force to be "on".
> 
> Still, your text does contain an example of a problem with the current
> code. Let me explicitly write it:
> - Our driver doesn't need it to be enabled, so bit 31 of
> HSW_PWR_WELL_DRIVER is 0
> - KVMr needs it enabled, so HSW_PWR_WELL_DRIVER bit 30 is 1
> - We do the I915_READ above
> - After the read, KVMr says it doesn't need the power well to be
> enabled anymore, so it gets disabled because no one else is requesting
> it
> - Then we touch an unclaimed register.
> 
> I think the only policy to avoid these race conditions should be "if I
> don't need the power well to be enabled, then I won't touch registers
> that require it to be enabled, no matter what the real state of the
> power well is".This should prevent any kinds of problems, because if I
> need the power well to be enabled, then I requested it to be enabled
> and it is enabled because I asked so. I'll send a second version of
> the patch with this.
> 
> >
> >> +             reg = PIPECONF(cpu_transcoder);
> >> +             val = I915_READ(reg);
> >> +             cur_state = !!(val & PIPECONF_ENABLE);
> >> +     } else {
> >> +             cur_state = false;
> >> +     }
> >> +
> >>       WARN(cur_state != state,
> >>            "pipe %c assertion failure (expected %s, current %s)\n",
> >>            pipe_name(pipe), state_string(state), state_string(cur_state));
> >> --
> >> 1.7.10.4
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 06/10] drm/i915: check the power down well on assert_pipe()

[Paulo Zanoni]

Hi

2013/1/25 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> On Fri, Jan 25, 2013 at 01:40:08PM -0200, Paulo Zanoni wrote:
>> Hi
>>
>> 2013/1/21 Ville Syrjälä <ville.syrjala@linux.intel.com>:
>> > On Fri, Jan 18, 2013 at 06:29:08PM -0200, Paulo Zanoni wrote:
>> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >>
>> >> If the power well is disabled, we should not try to read its
>> >> registers, otherwise we'll get "unclaimed register" messages.
>> >>
>> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/intel_display.c |   12 +++++++++---
>> >>  1 file changed, 9 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> >> index a7fb7e1..921b020 100644
>> >> --- a/drivers/gpu/drm/i915/intel_display.c
>> >> +++ b/drivers/gpu/drm/i915/intel_display.c
>> >> @@ -1214,9 +1214,15 @@ void assert_pipe(struct drm_i915_private *dev_priv,
>> >>       if (pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE)
>> >>               state = true;
>> >>
>> >> -     reg = PIPECONF(cpu_transcoder);
>> >> -     val = I915_READ(reg);
>> >> -     cur_state = !!(val & PIPECONF_ENABLE);
>> >> +     if (cpu_transcoder == TRANSCODER_EDP ||
>> >> +         (I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_STATE)) {
>> >
>> > Should that also check HSW_PWR_WELL_ENABLE? KVMR might have the well
>> > enabled, while the driver has it disabled. But KVMR might have already
>> > disabled the well, and it might get disabled just after this check,
>> > and then you would hit the unclaimed register issue again.
>>
>> By reading your text it seems you think that if bit 31 is 0 then bit
>> 30 will necessarily be 0. This is not true. If any entity (including
>> KVMr) needs the power well enabled (and is requesting so), then bit 30
>> of HSW_PWR_WELL_DRIVER will be 1, no matter the status of bit 31.
>
> Did you verify that on real HW?

Yes.

>
> It makes little sense when you consider that the spec says not
> to toggle the request bit when there's a state transition currently
> happening. If all the status bits read as the same thing, then
> there's no way to know when a transition is actually happening,
> and hence the text in the spec is complete nonsense.
>

I agree... I'm investigating this problem. That's why there is still
no new version of patch 3/10 here :)

> The only sensible interpretation of that text I could think of is
> that you're not supposed to abort a state transition you yourself
> caused. That would be achieved by polling the status bit after
> toggling the request bit, assuming that the status bit is just a
> delayed version of request bit.

If we're enabling then it's fine: we requested it to enable, then at
some point it will be enabled, then we can start using the registers.
We can always avoid touching the registers when the well is still
being enabled. The only situation where the "restriction" doesn't make
sense is on the "disable" case.

OTOH, if we requested it to be disabled, the fact that we can't
predict whether it's really going to be disabled or not shouldn't
really matter that much since we're not supposed to touch the
"possibly disabled" registers, and if anybody needs to touch those
registers they will have to request the well to be enabled first...
This still doesn't explain the "Restriction", but maybe that
restriction is just for the "enable" case.

>
>> Another thing to mention is that no entity can force the power well to
>> be "off", it can only force to be "on".
>>
>> Still, your text does contain an example of a problem with the current
>> code. Let me explicitly write it:
>> - Our driver doesn't need it to be enabled, so bit 31 of
>> HSW_PWR_WELL_DRIVER is 0
>> - KVMr needs it enabled, so HSW_PWR_WELL_DRIVER bit 30 is 1
>> - We do the I915_READ above
>> - After the read, KVMr says it doesn't need the power well to be
>> enabled anymore, so it gets disabled because no one else is requesting
>> it
>> - Then we touch an unclaimed register.
>>
>> I think the only policy to avoid these race conditions should be "if I
>> don't need the power well to be enabled, then I won't touch registers
>> that require it to be enabled, no matter what the real state of the
>> power well is".This should prevent any kinds of problems, because if I
>> need the power well to be enabled, then I requested it to be enabled
>> and it is enabled because I asked so. I'll send a second version of
>> the patch with this.
>>
>> >
>> >> +             reg = PIPECONF(cpu_transcoder);
>> >> +             val = I915_READ(reg);
>> >> +             cur_state = !!(val & PIPECONF_ENABLE);
>> >> +     } else {
>> >> +             cur_state = false;
>> >> +     }
>> >> +
>> >>       WARN(cur_state != state,
>> >>            "pipe %c assertion failure (expected %s, current %s)\n",
>> >>            pipe_name(pipe), state_string(state), state_string(cur_state));
>> >> --
>> >> 1.7.10.4
>> >>
>> >> _______________________________________________
>> >> Intel-gfx mailing list
>> >> Intel-gfx@lists.freedesktop.org
>> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >
>> > --
>> > Ville Syrjälä
>> > Intel OTC
>>
>>
>>
>> --
>> Paulo Zanoni
>
> --
> Ville Syrjälä
> Intel OTC



-- 
Paulo Zanoni

Re: [PATCH 06/10] drm/i915: check the power down well on assert_pipe()

[Ville Syrjälä]

On Fri, Jan 25, 2013 at 02:04:03PM -0200, Paulo Zanoni wrote:
> Hi
> 
> 2013/1/25 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > On Fri, Jan 25, 2013 at 01:40:08PM -0200, Paulo Zanoni wrote:
> >> Hi
> >>
> >> 2013/1/21 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> >> > On Fri, Jan 18, 2013 at 06:29:08PM -0200, Paulo Zanoni wrote:
> >> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> >>
> >> >> If the power well is disabled, we should not try to read its
> >> >> registers, otherwise we'll get "unclaimed register" messages.
> >> >>
> >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> >> ---
> >> >>  drivers/gpu/drm/i915/intel_display.c |   12 +++++++++---
> >> >>  1 file changed, 9 insertions(+), 3 deletions(-)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> >> index a7fb7e1..921b020 100644
> >> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> >> @@ -1214,9 +1214,15 @@ void assert_pipe(struct drm_i915_private *dev_priv,
> >> >>       if (pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE)
> >> >>               state = true;
> >> >>
> >> >> -     reg = PIPECONF(cpu_transcoder);
> >> >> -     val = I915_READ(reg);
> >> >> -     cur_state = !!(val & PIPECONF_ENABLE);
> >> >> +     if (cpu_transcoder == TRANSCODER_EDP ||
> >> >> +         (I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_STATE)) {
> >> >
> >> > Should that also check HSW_PWR_WELL_ENABLE? KVMR might have the well
> >> > enabled, while the driver has it disabled. But KVMR might have already
> >> > disabled the well, and it might get disabled just after this check,
> >> > and then you would hit the unclaimed register issue again.
> >>
> >> By reading your text it seems you think that if bit 31 is 0 then bit
> >> 30 will necessarily be 0. This is not true. If any entity (including
> >> KVMr) needs the power well enabled (and is requesting so), then bit 30
> >> of HSW_PWR_WELL_DRIVER will be 1, no matter the status of bit 31.
> >
> > Did you verify that on real HW?
> 
> Yes.
> 
> >
> > It makes little sense when you consider that the spec says not
> > to toggle the request bit when there's a state transition currently
> > happening. If all the status bits read as the same thing, then
> > there's no way to know when a transition is actually happening,
> > and hence the text in the spec is complete nonsense.
> >
> 
> I agree... I'm investigating this problem. That's why there is still
> no new version of patch 3/10 here :)
> 
> > The only sensible interpretation of that text I could think of is
> > that you're not supposed to abort a state transition you yourself
> > caused. That would be achieved by polling the status bit after
> > toggling the request bit, assuming that the status bit is just a
> > delayed version of request bit.
> 
> If we're enabling then it's fine: we requested it to enable, then at
> some point it will be enabled, then we can start using the registers.
> We can always avoid touching the registers when the well is still
> being enabled.

I think you can only assume this if the status bit goes to 0 immediately
when the well starts powering down. And it won't go back to 1 until the
well is fully powered again. Well, I guess that's the only design that
would make sense anyway. But even then you can't tell the fully powered
down case apart from the currently going down case.

> The only situation where the "restriction" doesn't make
> sense is on the "disable" case.
> 
> OTOH, if we requested it to be disabled, the fact that we can't
> predict whether it's really going to be disabled or not shouldn't
> really matter that much since we're not supposed to touch the
> "possibly disabled" registers, and if anybody needs to touch those
> registers they will have to request the well to be enabled first...
> This still doesn't explain the "Restriction", but maybe that
> restriction is just for the "enable" case.
> 
> >
> >> Another thing to mention is that no entity can force the power well to
> >> be "off", it can only force to be "on".
> >>
> >> Still, your text does contain an example of a problem with the current
> >> code. Let me explicitly write it:
> >> - Our driver doesn't need it to be enabled, so bit 31 of
> >> HSW_PWR_WELL_DRIVER is 0
> >> - KVMr needs it enabled, so HSW_PWR_WELL_DRIVER bit 30 is 1
> >> - We do the I915_READ above
> >> - After the read, KVMr says it doesn't need the power well to be
> >> enabled anymore, so it gets disabled because no one else is requesting
> >> it
> >> - Then we touch an unclaimed register.
> >>
> >> I think the only policy to avoid these race conditions should be "if I
> >> don't need the power well to be enabled, then I won't touch registers
> >> that require it to be enabled, no matter what the real state of the
> >> power well is".This should prevent any kinds of problems, because if I
> >> need the power well to be enabled, then I requested it to be enabled
> >> and it is enabled because I asked so. I'll send a second version of
> >> the patch with this.
> >>
> >> >
> >> >> +             reg = PIPECONF(cpu_transcoder);
> >> >> +             val = I915_READ(reg);
> >> >> +             cur_state = !!(val & PIPECONF_ENABLE);
> >> >> +     } else {
> >> >> +             cur_state = false;
> >> >> +     }
> >> >> +
> >> >>       WARN(cur_state != state,
> >> >>            "pipe %c assertion failure (expected %s, current %s)\n",
> >> >>            pipe_name(pipe), state_string(state), state_string(cur_state));
> >> >> --
> >> >> 1.7.10.4
> >> >>
> >> >> _______________________________________________
> >> >> Intel-gfx mailing list
> >> >> Intel-gfx@lists.freedesktop.org
> >> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >> >
> >> > --
> >> > Ville Syrjälä
> >> > Intel OTC
> >>
> >>
> >>
> >> --
> >> Paulo Zanoni
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC

[PATCH 07/10] drm/i915: turn on the power well before suspending

[Paulo Zanoni]

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

Our suspend code touches a lot of registers all over the place, so we
need to enable the power well before suspending.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index c8cbc32..7935606 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -468,6 +468,8 @@ static int i915_drm_freeze(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	intel_set_power_well(dev, true);
+
 	drm_kms_helper_poll_disable(dev);
 
 	pci_save_state(dev->pdev);
-- 
1.7.10.4

Re: [PATCH 07/10] drm/i915: turn on the power well before suspending

[Jani Nikula]

On Fri, 18 Jan 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Our suspend code touches a lot of registers all over the place, so we
> need to enable the power well before suspending.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c |    2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index c8cbc32..7935606 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -468,6 +468,8 @@ static int i915_drm_freeze(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> +	intel_set_power_well(dev, true);
> +
>  	drm_kms_helper_poll_disable(dev);
>  
>  	pci_save_state(dev->pdev);
> -- 
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 08/10] drm/i915: set TRANSCODER_EDP even earlier

[Paulo Zanoni]

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

Instead of setting it at the beginning of haswell_crtc_mode_set, let's
set it at the beginning of intel_crtc_mode_set. When
intel_crt_mode_set calls drm_vblank_pre_modeset we already need to
have the transcoder_edp correctly set, because eventually
drm_vblank_pre_modeset calls functions that call i915_pipe_enabled
from i915_irq.c, which will read PIPECONF(cpu_transcoder).

This is a bug that affects us since we added support for
TRANSCODER_EDP, but I was only able to see the problem after
suspending a machine with the power well disabled (got an "unclaimed
register" error.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 921b020..e9a2d4e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5662,11 +5662,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 		num_connectors++;
 	}
 
-	if (is_cpu_edp)
-		intel_crtc->cpu_transcoder = TRANSCODER_EDP;
-	else
-		intel_crtc->cpu_transcoder = pipe;
-
 	/* We are not sure yet this won't happen. */
 	WARN(!HAS_PCH_LPT(dev), "Unexpected PCH type %d\n",
 	     INTEL_PCH_TYPE(dev));
@@ -5731,6 +5726,11 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc,
 	int pipe = intel_crtc->pipe;
 	int ret;
 
+	if (IS_HASWELL(dev) && intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))
+		intel_crtc->cpu_transcoder = TRANSCODER_EDP;
+	else
+		intel_crtc->cpu_transcoder = pipe;
+
 	drm_vblank_pre_modeset(dev, pipe);
 
 	ret = dev_priv->display.crtc_mode_set(crtc, mode, adjusted_mode,
-- 
1.7.10.4

Re: [PATCH 08/10] drm/i915: set TRANSCODER_EDP even earlier

[Jani Nikula]

On Fri, 18 Jan 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Instead of setting it at the beginning of haswell_crtc_mode_set, let's
> set it at the beginning of intel_crtc_mode_set. When
> intel_crt_mode_set calls drm_vblank_pre_modeset we already need to
> have the transcoder_edp correctly set, because eventually
> drm_vblank_pre_modeset calls functions that call i915_pipe_enabled
> from i915_irq.c, which will read PIPECONF(cpu_transcoder).

Side note, we confusingly have two i915_pipe_enabled() functions, the
one referred to here in i915_irq.c and the other in i915_suspend.c.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>
> This is a bug that affects us since we added support for
> TRANSCODER_EDP, but I was only able to see the problem after
> suspending a machine with the power well disabled (got an "unclaimed
> register" error.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 921b020..e9a2d4e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5662,11 +5662,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>  		num_connectors++;
>  	}
>  
> -	if (is_cpu_edp)
> -		intel_crtc->cpu_transcoder = TRANSCODER_EDP;
> -	else
> -		intel_crtc->cpu_transcoder = pipe;
> -
>  	/* We are not sure yet this won't happen. */
>  	WARN(!HAS_PCH_LPT(dev), "Unexpected PCH type %d\n",
>  	     INTEL_PCH_TYPE(dev));
> @@ -5731,6 +5726,11 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc,
>  	int pipe = intel_crtc->pipe;
>  	int ret;
>  
> +	if (IS_HASWELL(dev) && intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))
> +		intel_crtc->cpu_transcoder = TRANSCODER_EDP;
> +	else
> +		intel_crtc->cpu_transcoder = pipe;
> +
>  	drm_vblank_pre_modeset(dev, pipe);
>  
>  	ret = dev_priv->display.crtc_mode_set(crtc, mode, adjusted_mode,
> -- 
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 09/10] drm/i915: print IVB/HSW display error interrupts

[Paulo Zanoni]

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

If we get one of these messages it means we did something wrong, and
the first step to fix wrong things is to detect them and recognize
they exist.

For now, leave these messages as DRM_DEBUG_KMS. After we conclude that
a certain message should not be print since our code is correct, then
we can promote that specific message to DRM_ERROR.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c |   56 ++++++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_reg.h |   23 ++++++++++++++--
 2 files changed, 76 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index cc49a6d..2f17b54 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -697,6 +697,54 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
 					 I915_READ(FDI_RX_IIR(pipe)));
 }
 
+static void ivb_err_int_handler(struct drm_device *dev)
+{
+	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
+	u32 err_int = I915_READ(GEN7_ERR_INT);
+
+	if (err_int & ERR_INT_POISON)
+		DRM_DEBUG_KMS("Poison interrupt\n");
+	if (err_int & ERR_INT_INVALID_GTT_PTE)
+		DRM_DEBUG_KMS("Invalid GTT PTE\n");
+	if (err_int & ERR_INT_INVALID_PTE_DATA)
+		DRM_DEBUG_KMS("Invalid GTT PTE data\n");
+	if (err_int & ERR_INT_SPRITE_GTT_FAULT_C)
+		DRM_DEBUG_KMS("Sprite C GTT fault\n");
+	if (err_int & ERR_INT_PRIMARY_GTT_FAULT_C)
+		DRM_DEBUG_KMS("Primary place C GTT fault\n");
+	if (err_int & ERR_INT_CURSOR_GTT_FAULT_C)
+		DRM_DEBUG_KMS("Cursor C GTT fault\n");
+	if (err_int & ERR_INT_SPRITE_GTT_FAULT_B)
+		DRM_DEBUG_KMS("Sprite B GTT fault\n");
+	if (err_int & ERR_INT_PRIMARY_GTT_FAULT_B)
+		DRM_DEBUG_KMS("Primary place B GTT fault\n");
+	if (err_int & ERR_INT_CURSOR_GTT_FAULT_B)
+		DRM_DEBUG_KMS("Cursor B GTT fault\n");
+	if (err_int & ERR_INT_SPRITE_GTT_FAULT_A)
+		DRM_DEBUG_KMS("Sprite A GTT fault\n");
+	if (err_int & ERR_INT_PRIMARY_GTT_FAULT_A)
+		DRM_DEBUG_KMS("Primary place A GTT fault\n");
+	if (err_int & ERR_INT_CURSOR_GTT_FAULT_A)
+		DRM_DEBUG_KMS("Cursor A GTT fault\n");
+	if (err_int & ERR_INT_PIPE_CRR_ERROR_C)
+		DRM_DEBUG_KMS("Pipe C CRC error\n");
+	if (err_int & ERR_INT_PIPE_FIFO_UNDERRRUN_C)
+		DRM_DEBUG_KMS("Pipe C FIFO underrun\n");
+	if (err_int & ERR_INT_PIPE_CRR_ERROR_B)
+		DRM_DEBUG_KMS("Pipe B CRC error\n");
+	if (err_int & ERR_INT_PIPE_FIFO_UNDERRRUN_B)
+		DRM_DEBUG_KMS("Pipe B FIFO underrun\n");
+	if (err_int & ERR_INT_PIPE_CRR_ERROR_A)
+		DRM_DEBUG_KMS("Pipe A CRC error\n");
+	if (err_int & ERR_INT_PIPE_FIFO_UNDERRRUN_A)
+		DRM_DEBUG_KMS("Pipe A FIFO underrun\n");
+
+	if (IS_HASWELL(dev) && (err_int & ERR_INT_MMIO_UNCLAIMED))
+		DRM_DEBUG_KMS("MMIO cycle not claimed\n");
+
+	I915_WRITE(GEN7_ERR_INT, err_int);
+}
+
 static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
 {
 	struct drm_device *dev = (struct drm_device *) arg;
@@ -720,6 +768,9 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
 
 	de_iir = I915_READ(DEIIR);
 	if (de_iir) {
+		if (de_iir & DE_ERR_INT_IVB)
+			ivb_err_int_handler(dev);
+
 		if (de_iir & DE_AUX_CHANNEL_A_IVB)
 			dp_aux_irq_handler(dev);
 
@@ -1964,7 +2015,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
 		DE_PLANEC_FLIP_DONE_IVB |
 		DE_PLANEB_FLIP_DONE_IVB |
 		DE_PLANEA_FLIP_DONE_IVB |
-		DE_AUX_CHANNEL_A_IVB;
+		DE_AUX_CHANNEL_A_IVB | DE_ERR_INT_IVB;
 	u32 render_irqs;
 	u32 hotplug_mask;
 	u32 pch_irq_mask;
@@ -1972,6 +2023,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
 	dev_priv->irq_mask = ~display_mask;
 
 	/* should always can generate irq */
+	I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
 	I915_WRITE(DEIIR, I915_READ(DEIIR));
 	I915_WRITE(DEIMR, dev_priv->irq_mask);
 	I915_WRITE(DEIER,
@@ -2135,6 +2187,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 	I915_WRITE(DEIMR, 0xffffffff);
 	I915_WRITE(DEIER, 0x0);
 	I915_WRITE(DEIIR, I915_READ(DEIIR));
+	if (IS_GEN7(dev))
+		I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
 
 	I915_WRITE(GTIMR, 0xffffffff);
 	I915_WRITE(GTIER, 0x0);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f054554..e8ecdc4 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -511,7 +511,26 @@
 
 #define ERROR_GEN6	0x040a0
 #define GEN7_ERR_INT	0x44040
-#define   ERR_INT_MMIO_UNCLAIMED (1<<13)
+#define  ERR_INT_POISON			(1<<31)
+#define  ERR_INT_INVALID_GTT_PTE	(1<<29)
+#define  ERR_INT_INVALID_PTE_DATA	(1<<28)
+#define  ERR_INT_SPRITE_GTT_FAULT_C	(1<<23)
+#define  ERR_INT_PRIMARY_GTT_FAULT_C	(1<<22)
+#define  ERR_INT_CURSOR_GTT_FAULT_C	(1<<21)
+#define  ERR_INT_SPRITE_GTT_FAULT_B	(1<<20)
+#define  ERR_INT_PRIMARY_GTT_FAULT_B	(1<<19)
+#define  ERR_INT_CURSOR_GTT_FAULT_B	(1<<18)
+#define  ERR_INT_SPRITE_GTT_FAULT_A	(1<<17)
+#define  ERR_INT_PRIMARY_GTT_FAULT_A	(1<<16)
+#define  ERR_INT_CURSOR_GTT_FAULT_A	(1<<15)
+#define  ERR_INT_PIPE_CRR_ERROR_C	(1<<7)
+#define  ERR_INT_PIPE_FIFO_UNDERRRUN_C	(1<<6)
+#define  ERR_INT_PIPE_CRR_ERROR_B	(1<<4)
+#define  ERR_INT_PIPE_FIFO_UNDERRRUN_B	(1<<3)
+#define  ERR_INT_PIPE_CRR_ERROR_A	(1<<1)
+#define  ERR_INT_PIPE_FIFO_UNDERRRUN_A	(1<<0)
+/* Haswell only: */
+#define  ERR_INT_MMIO_UNCLAIMED		(1<<13)
 
 /* GM45+ chicken bits -- debug workaround bits that may be required
  * for various sorts of correct behavior.  The top 16 bits of each are
@@ -3374,7 +3393,7 @@
 #define DE_PIPEA_FIFO_UNDERRUN  (1 << 0)
 
 /* More Ivybridge lolz */
-#define DE_ERR_DEBUG_IVB		(1<<30)
+#define DE_ERR_INT_IVB			(1<<30)
 #define DE_GSE_IVB			(1<<29)
 #define DE_PCH_EVENT_IVB		(1<<28)
 #define DE_DP_A_HOTPLUG_IVB		(1<<27)
-- 
1.7.10.4

Re: [PATCH 09/10] drm/i915: print IVB/HSW display error interrupts

[Ben Widawsky]

On Fri, Jan 18, 2013 at 06:29:11PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> If we get one of these messages it means we did something wrong, and
> the first step to fix wrong things is to detect them and recognize
> they exist.
> 
> For now, leave these messages as DRM_DEBUG_KMS. After we conclude that
> a certain message should not be print since our code is correct, then
> we can promote that specific message to DRM_ERROR.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

A couple of recommendations below, but either way it is
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

> ---
>  drivers/gpu/drm/i915/i915_irq.c |   56 ++++++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_reg.h |   23 ++++++++++++++--
>  2 files changed, 76 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index cc49a6d..2f17b54 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -697,6 +697,54 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
>  					 I915_READ(FDI_RX_IIR(pipe)));
>  }
>  
> +static void ivb_err_int_handler(struct drm_device *dev)
> +{
> +	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> +	u32 err_int = I915_READ(GEN7_ERR_INT);
> +
> +	if (err_int & ERR_INT_POISON)
> +		DRM_DEBUG_KMS("Poison interrupt\n");
> +	if (err_int & ERR_INT_INVALID_GTT_PTE)
> +		DRM_DEBUG_KMS("Invalid GTT PTE\n");
> +	if (err_int & ERR_INT_INVALID_PTE_DATA)
> +		DRM_DEBUG_KMS("Invalid GTT PTE data\n");
> +	if (err_int & ERR_INT_SPRITE_GTT_FAULT_C)
> +		DRM_DEBUG_KMS("Sprite C GTT fault\n");
> +	if (err_int & ERR_INT_PRIMARY_GTT_FAULT_C)
> +		DRM_DEBUG_KMS("Primary place C GTT fault\n");
> +	if (err_int & ERR_INT_CURSOR_GTT_FAULT_C)
> +		DRM_DEBUG_KMS("Cursor C GTT fault\n");
> +	if (err_int & ERR_INT_SPRITE_GTT_FAULT_B)
> +		DRM_DEBUG_KMS("Sprite B GTT fault\n");
> +	if (err_int & ERR_INT_PRIMARY_GTT_FAULT_B)
> +		DRM_DEBUG_KMS("Primary place B GTT fault\n");
> +	if (err_int & ERR_INT_CURSOR_GTT_FAULT_B)
> +		DRM_DEBUG_KMS("Cursor B GTT fault\n");
> +	if (err_int & ERR_INT_SPRITE_GTT_FAULT_A)
> +		DRM_DEBUG_KMS("Sprite A GTT fault\n");
> +	if (err_int & ERR_INT_PRIMARY_GTT_FAULT_A)
> +		DRM_DEBUG_KMS("Primary place A GTT fault\n");
> +	if (err_int & ERR_INT_CURSOR_GTT_FAULT_A)
> +		DRM_DEBUG_KMS("Cursor A GTT fault\n");
> +	if (err_int & ERR_INT_PIPE_CRR_ERROR_C)
> +		DRM_DEBUG_KMS("Pipe C CRC error\n");
> +	if (err_int & ERR_INT_PIPE_FIFO_UNDERRRUN_C)
> +		DRM_DEBUG_KMS("Pipe C FIFO underrun\n");
> +	if (err_int & ERR_INT_PIPE_CRR_ERROR_B)
> +		DRM_DEBUG_KMS("Pipe B CRC error\n");
> +	if (err_int & ERR_INT_PIPE_FIFO_UNDERRRUN_B)
> +		DRM_DEBUG_KMS("Pipe B FIFO underrun\n");
> +	if (err_int & ERR_INT_PIPE_CRR_ERROR_A)
> +		DRM_DEBUG_KMS("Pipe A CRC error\n");
> +	if (err_int & ERR_INT_PIPE_FIFO_UNDERRRUN_A)
> +		DRM_DEBUG_KMS("Pipe A FIFO underrun\n");
> +
> +	if (IS_HASWELL(dev) && (err_int & ERR_INT_MMIO_UNCLAIMED))
> +		DRM_DEBUG_KMS("MMIO cycle not claimed\n");
> +
> +	I915_WRITE(GEN7_ERR_INT, err_int);
> +}
> +

We probably don't need the IS_HASWELL() check, but it shouldn't hurt
either.

I shutter to think of debugging what will happen if this handler itself
generates ERR_INT_MMIO_UNCLAIMED. Hopefully the DRM_DEBUG_KMS will get a
chance to get spewed out.

Since the only thing you're doing is printing, you could do something
like:
u32 err_int = I915_READ(GEN7_ERR_INT);
if (drm_debug & DRM_UT_KMS == 0) {
	I915_WRITE(GEN7_ERR_INT, err_int);
	return;
}

>  static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>  {
>  	struct drm_device *dev = (struct drm_device *) arg;
> @@ -720,6 +768,9 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>  
>  	de_iir = I915_READ(DEIIR);
>  	if (de_iir) {
> +		if (de_iir & DE_ERR_INT_IVB)
> +			ivb_err_int_handler(dev);
> +
>  		if (de_iir & DE_AUX_CHANNEL_A_IVB)
>  			dp_aux_irq_handler(dev);
>  
> @@ -1964,7 +2015,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
>  		DE_PLANEC_FLIP_DONE_IVB |
>  		DE_PLANEB_FLIP_DONE_IVB |
>  		DE_PLANEA_FLIP_DONE_IVB |
> -		DE_AUX_CHANNEL_A_IVB;
> +		DE_AUX_CHANNEL_A_IVB | DE_ERR_INT_IVB;
>  	u32 render_irqs;
>  	u32 hotplug_mask;
>  	u32 pch_irq_mask;
> @@ -1972,6 +2023,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
>  	dev_priv->irq_mask = ~display_mask;
>  
>  	/* should always can generate irq */
> +	I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
>  	I915_WRITE(DEIIR, I915_READ(DEIIR));
>  	I915_WRITE(DEIMR, dev_priv->irq_mask);
>  	I915_WRITE(DEIER,
> @@ -2135,6 +2187,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
>  	I915_WRITE(DEIMR, 0xffffffff);
>  	I915_WRITE(DEIER, 0x0);
>  	I915_WRITE(DEIIR, I915_READ(DEIIR));
> +	if (IS_GEN7(dev))
> +		I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
>  
>  	I915_WRITE(GTIMR, 0xffffffff);
>  	I915_WRITE(GTIER, 0x0);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f054554..e8ecdc4 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -511,7 +511,26 @@
>  
>  #define ERROR_GEN6	0x040a0
>  #define GEN7_ERR_INT	0x44040
> -#define   ERR_INT_MMIO_UNCLAIMED (1<<13)
> +#define  ERR_INT_POISON			(1<<31)
> +#define  ERR_INT_INVALID_GTT_PTE	(1<<29)
> +#define  ERR_INT_INVALID_PTE_DATA	(1<<28)
> +#define  ERR_INT_SPRITE_GTT_FAULT_C	(1<<23)
> +#define  ERR_INT_PRIMARY_GTT_FAULT_C	(1<<22)
> +#define  ERR_INT_CURSOR_GTT_FAULT_C	(1<<21)
> +#define  ERR_INT_SPRITE_GTT_FAULT_B	(1<<20)
> +#define  ERR_INT_PRIMARY_GTT_FAULT_B	(1<<19)
> +#define  ERR_INT_CURSOR_GTT_FAULT_B	(1<<18)
> +#define  ERR_INT_SPRITE_GTT_FAULT_A	(1<<17)
> +#define  ERR_INT_PRIMARY_GTT_FAULT_A	(1<<16)
> +#define  ERR_INT_CURSOR_GTT_FAULT_A	(1<<15)
> +#define  ERR_INT_PIPE_CRR_ERROR_C	(1<<7)
> +#define  ERR_INT_PIPE_FIFO_UNDERRRUN_C	(1<<6)
> +#define  ERR_INT_PIPE_CRR_ERROR_B	(1<<4)
> +#define  ERR_INT_PIPE_FIFO_UNDERRRUN_B	(1<<3)
> +#define  ERR_INT_PIPE_CRR_ERROR_A	(1<<1)
> +#define  ERR_INT_PIPE_FIFO_UNDERRRUN_A	(1<<0)
> +/* Haswell only: */
> +#define  ERR_INT_MMIO_UNCLAIMED		(1<<13)
>  
>  /* GM45+ chicken bits -- debug workaround bits that may be required
>   * for various sorts of correct behavior.  The top 16 bits of each are
> @@ -3374,7 +3393,7 @@
>  #define DE_PIPEA_FIFO_UNDERRUN  (1 << 0)
>  
>  /* More Ivybridge lolz */
> -#define DE_ERR_DEBUG_IVB		(1<<30)
> +#define DE_ERR_INT_IVB			(1<<30)
>  #define DE_GSE_IVB			(1<<29)
>  #define DE_PCH_EVENT_IVB		(1<<28)
>  #define DE_DP_A_HOTPLUG_IVB		(1<<27)
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 09/10] drm/i915: print IVB/HSW display error interrupts

[Jani Nikula]

On Fri, 18 Jan 2013, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Fri, Jan 18, 2013 at 06:29:11PM -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> 
>> If we get one of these messages it means we did something wrong, and
>> the first step to fix wrong things is to detect them and recognize
>> they exist.
>> 
>> For now, leave these messages as DRM_DEBUG_KMS. After we conclude that
>> a certain message should not be print since our code is correct, then
>> we can promote that specific message to DRM_ERROR.
>> 
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> A couple of recommendations below, but either way it is
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c |   56 ++++++++++++++++++++++++++++++++++++++-
>>  drivers/gpu/drm/i915/i915_reg.h |   23 ++++++++++++++--
>>  2 files changed, 76 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index cc49a6d..2f17b54 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -697,6 +697,54 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
>>  					 I915_READ(FDI_RX_IIR(pipe)));
>>  }
>>  
>> +static void ivb_err_int_handler(struct drm_device *dev)
>> +{
>> +	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>> +	u32 err_int = I915_READ(GEN7_ERR_INT);
>> +
>> +	if (err_int & ERR_INT_POISON)
>> +		DRM_DEBUG_KMS("Poison interrupt\n");
>> +	if (err_int & ERR_INT_INVALID_GTT_PTE)
>> +		DRM_DEBUG_KMS("Invalid GTT PTE\n");
>> +	if (err_int & ERR_INT_INVALID_PTE_DATA)
>> +		DRM_DEBUG_KMS("Invalid GTT PTE data\n");
>> +	if (err_int & ERR_INT_SPRITE_GTT_FAULT_C)
>> +		DRM_DEBUG_KMS("Sprite C GTT fault\n");
>> +	if (err_int & ERR_INT_PRIMARY_GTT_FAULT_C)
>> +		DRM_DEBUG_KMS("Primary place C GTT fault\n");
>> +	if (err_int & ERR_INT_CURSOR_GTT_FAULT_C)
>> +		DRM_DEBUG_KMS("Cursor C GTT fault\n");
>> +	if (err_int & ERR_INT_SPRITE_GTT_FAULT_B)
>> +		DRM_DEBUG_KMS("Sprite B GTT fault\n");
>> +	if (err_int & ERR_INT_PRIMARY_GTT_FAULT_B)
>> +		DRM_DEBUG_KMS("Primary place B GTT fault\n");
>> +	if (err_int & ERR_INT_CURSOR_GTT_FAULT_B)
>> +		DRM_DEBUG_KMS("Cursor B GTT fault\n");
>> +	if (err_int & ERR_INT_SPRITE_GTT_FAULT_A)
>> +		DRM_DEBUG_KMS("Sprite A GTT fault\n");
>> +	if (err_int & ERR_INT_PRIMARY_GTT_FAULT_A)
>> +		DRM_DEBUG_KMS("Primary place A GTT fault\n");
>> +	if (err_int & ERR_INT_CURSOR_GTT_FAULT_A)
>> +		DRM_DEBUG_KMS("Cursor A GTT fault\n");
>> +	if (err_int & ERR_INT_PIPE_CRR_ERROR_C)
>> +		DRM_DEBUG_KMS("Pipe C CRC error\n");
>> +	if (err_int & ERR_INT_PIPE_FIFO_UNDERRRUN_C)
>> +		DRM_DEBUG_KMS("Pipe C FIFO underrun\n");
>> +	if (err_int & ERR_INT_PIPE_CRR_ERROR_B)
>> +		DRM_DEBUG_KMS("Pipe B CRC error\n");
>> +	if (err_int & ERR_INT_PIPE_FIFO_UNDERRRUN_B)
>> +		DRM_DEBUG_KMS("Pipe B FIFO underrun\n");
>> +	if (err_int & ERR_INT_PIPE_CRR_ERROR_A)
>> +		DRM_DEBUG_KMS("Pipe A CRC error\n");
>> +	if (err_int & ERR_INT_PIPE_FIFO_UNDERRRUN_A)
>> +		DRM_DEBUG_KMS("Pipe A FIFO underrun\n");
>> +
>> +	if (IS_HASWELL(dev) && (err_int & ERR_INT_MMIO_UNCLAIMED))
>> +		DRM_DEBUG_KMS("MMIO cycle not claimed\n");
>> +
>> +	I915_WRITE(GEN7_ERR_INT, err_int);
>> +}
>> +
>
> We probably don't need the IS_HASWELL() check, but it shouldn't hurt
> either.
>
> I shutter to think of debugging what will happen if this handler itself
> generates ERR_INT_MMIO_UNCLAIMED. Hopefully the DRM_DEBUG_KMS will get a
> chance to get spewed out.
>
> Since the only thing you're doing is printing, you could do something
> like:
> u32 err_int = I915_READ(GEN7_ERR_INT);
> if (drm_debug & DRM_UT_KMS == 0) {

Review-of-review, that needs braces or it's always false.

BR,
Jani.


> 	I915_WRITE(GEN7_ERR_INT, err_int);
> 	return;
> }
>
>>  static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>>  {
>>  	struct drm_device *dev = (struct drm_device *) arg;
>> @@ -720,6 +768,9 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>>  
>>  	de_iir = I915_READ(DEIIR);
>>  	if (de_iir) {
>> +		if (de_iir & DE_ERR_INT_IVB)
>> +			ivb_err_int_handler(dev);
>> +
>>  		if (de_iir & DE_AUX_CHANNEL_A_IVB)
>>  			dp_aux_irq_handler(dev);
>>  
>> @@ -1964,7 +2015,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
>>  		DE_PLANEC_FLIP_DONE_IVB |
>>  		DE_PLANEB_FLIP_DONE_IVB |
>>  		DE_PLANEA_FLIP_DONE_IVB |
>> -		DE_AUX_CHANNEL_A_IVB;
>> +		DE_AUX_CHANNEL_A_IVB | DE_ERR_INT_IVB;
>>  	u32 render_irqs;
>>  	u32 hotplug_mask;
>>  	u32 pch_irq_mask;
>> @@ -1972,6 +2023,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
>>  	dev_priv->irq_mask = ~display_mask;
>>  
>>  	/* should always can generate irq */
>> +	I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
>>  	I915_WRITE(DEIIR, I915_READ(DEIIR));
>>  	I915_WRITE(DEIMR, dev_priv->irq_mask);
>>  	I915_WRITE(DEIER,
>> @@ -2135,6 +2187,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
>>  	I915_WRITE(DEIMR, 0xffffffff);
>>  	I915_WRITE(DEIER, 0x0);
>>  	I915_WRITE(DEIIR, I915_READ(DEIIR));
>> +	if (IS_GEN7(dev))
>> +		I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
>>  
>>  	I915_WRITE(GTIMR, 0xffffffff);
>>  	I915_WRITE(GTIER, 0x0);
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index f054554..e8ecdc4 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -511,7 +511,26 @@
>>  
>>  #define ERROR_GEN6	0x040a0
>>  #define GEN7_ERR_INT	0x44040
>> -#define   ERR_INT_MMIO_UNCLAIMED (1<<13)
>> +#define  ERR_INT_POISON			(1<<31)
>> +#define  ERR_INT_INVALID_GTT_PTE	(1<<29)
>> +#define  ERR_INT_INVALID_PTE_DATA	(1<<28)
>> +#define  ERR_INT_SPRITE_GTT_FAULT_C	(1<<23)
>> +#define  ERR_INT_PRIMARY_GTT_FAULT_C	(1<<22)
>> +#define  ERR_INT_CURSOR_GTT_FAULT_C	(1<<21)
>> +#define  ERR_INT_SPRITE_GTT_FAULT_B	(1<<20)
>> +#define  ERR_INT_PRIMARY_GTT_FAULT_B	(1<<19)
>> +#define  ERR_INT_CURSOR_GTT_FAULT_B	(1<<18)
>> +#define  ERR_INT_SPRITE_GTT_FAULT_A	(1<<17)
>> +#define  ERR_INT_PRIMARY_GTT_FAULT_A	(1<<16)
>> +#define  ERR_INT_CURSOR_GTT_FAULT_A	(1<<15)
>> +#define  ERR_INT_PIPE_CRR_ERROR_C	(1<<7)
>> +#define  ERR_INT_PIPE_FIFO_UNDERRRUN_C	(1<<6)
>> +#define  ERR_INT_PIPE_CRR_ERROR_B	(1<<4)
>> +#define  ERR_INT_PIPE_FIFO_UNDERRRUN_B	(1<<3)
>> +#define  ERR_INT_PIPE_CRR_ERROR_A	(1<<1)
>> +#define  ERR_INT_PIPE_FIFO_UNDERRRUN_A	(1<<0)
>> +/* Haswell only: */
>> +#define  ERR_INT_MMIO_UNCLAIMED		(1<<13)
>>  
>>  /* GM45+ chicken bits -- debug workaround bits that may be required
>>   * for various sorts of correct behavior.  The top 16 bits of each are
>> @@ -3374,7 +3393,7 @@
>>  #define DE_PIPEA_FIFO_UNDERRUN  (1 << 0)
>>  
>>  /* More Ivybridge lolz */
>> -#define DE_ERR_DEBUG_IVB		(1<<30)
>> +#define DE_ERR_INT_IVB			(1<<30)
>>  #define DE_GSE_IVB			(1<<29)
>>  #define DE_PCH_EVENT_IVB		(1<<28)
>>  #define DE_DP_A_HOTPLUG_IVB		(1<<27)
>> -- 
>> 1.7.10.4
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Ben Widawsky, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/10] drm/i915: print IVB/HSW display error interrupts

[Jani Nikula]

On Fri, 18 Jan 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> If we get one of these messages it means we did something wrong, and
> the first step to fix wrong things is to detect them and recognize
> they exist.
>
> For now, leave these messages as DRM_DEBUG_KMS. After we conclude that
> a certain message should not be print since our code is correct, then
> we can promote that specific message to DRM_ERROR.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c |   56 ++++++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_reg.h |   23 ++++++++++++++--
>  2 files changed, 76 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index cc49a6d..2f17b54 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -697,6 +697,54 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
>  					 I915_READ(FDI_RX_IIR(pipe)));
>  }
>  
> +static void ivb_err_int_handler(struct drm_device *dev)
> +{
> +	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;

dev->dev_private being a void *, the cast is unnecessary.

> +	u32 err_int = I915_READ(GEN7_ERR_INT);
> +
> +	if (err_int & ERR_INT_POISON)
> +		DRM_DEBUG_KMS("Poison interrupt\n");
> +	if (err_int & ERR_INT_INVALID_GTT_PTE)
> +		DRM_DEBUG_KMS("Invalid GTT PTE\n");
> +	if (err_int & ERR_INT_INVALID_PTE_DATA)
> +		DRM_DEBUG_KMS("Invalid GTT PTE data\n");
> +	if (err_int & ERR_INT_SPRITE_GTT_FAULT_C)
> +		DRM_DEBUG_KMS("Sprite C GTT fault\n");
> +	if (err_int & ERR_INT_PRIMARY_GTT_FAULT_C)
> +		DRM_DEBUG_KMS("Primary place C GTT fault\n");
> +	if (err_int & ERR_INT_CURSOR_GTT_FAULT_C)
> +		DRM_DEBUG_KMS("Cursor C GTT fault\n");
> +	if (err_int & ERR_INT_SPRITE_GTT_FAULT_B)
> +		DRM_DEBUG_KMS("Sprite B GTT fault\n");
> +	if (err_int & ERR_INT_PRIMARY_GTT_FAULT_B)
> +		DRM_DEBUG_KMS("Primary place B GTT fault\n");
> +	if (err_int & ERR_INT_CURSOR_GTT_FAULT_B)
> +		DRM_DEBUG_KMS("Cursor B GTT fault\n");
> +	if (err_int & ERR_INT_SPRITE_GTT_FAULT_A)
> +		DRM_DEBUG_KMS("Sprite A GTT fault\n");
> +	if (err_int & ERR_INT_PRIMARY_GTT_FAULT_A)
> +		DRM_DEBUG_KMS("Primary place A GTT fault\n");
> +	if (err_int & ERR_INT_CURSOR_GTT_FAULT_A)
> +		DRM_DEBUG_KMS("Cursor A GTT fault\n");
> +	if (err_int & ERR_INT_PIPE_CRR_ERROR_C)

CRR? Should it be CRC? Ditto for the others below.

Also, how about trading some clarity of error messages to less
duplication of info with something like:

#define err_print(status, bit) do { \
	if ((status) & (bit)) \
		DRM_DEBUG_KMS("Error interrupt %s\n", #bit);\
	} while (0)

	err_print(err_int, ERR_INT_POISON);
        err_print(err_int, ERR_INT_INVALID_GTT_PTE);
        err_print(err_int, ERR_INT_INVALID_PTE_DATA);
        ...

#undef err_print

This is just bikeshedding though, up to you.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> +		DRM_DEBUG_KMS("Pipe C CRC error\n");
> +	if (err_int & ERR_INT_PIPE_FIFO_UNDERRRUN_C)
> +		DRM_DEBUG_KMS("Pipe C FIFO underrun\n");
> +	if (err_int & ERR_INT_PIPE_CRR_ERROR_B)
> +		DRM_DEBUG_KMS("Pipe B CRC error\n");
> +	if (err_int & ERR_INT_PIPE_FIFO_UNDERRRUN_B)
> +		DRM_DEBUG_KMS("Pipe B FIFO underrun\n");
> +	if (err_int & ERR_INT_PIPE_CRR_ERROR_A)
> +		DRM_DEBUG_KMS("Pipe A CRC error\n");
> +	if (err_int & ERR_INT_PIPE_FIFO_UNDERRRUN_A)
> +		DRM_DEBUG_KMS("Pipe A FIFO underrun\n");
> +
> +	if (IS_HASWELL(dev) && (err_int & ERR_INT_MMIO_UNCLAIMED))
> +		DRM_DEBUG_KMS("MMIO cycle not claimed\n");
> +
> +	I915_WRITE(GEN7_ERR_INT, err_int);
> +}
> +
>  static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>  {
>  	struct drm_device *dev = (struct drm_device *) arg;
> @@ -720,6 +768,9 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>  
>  	de_iir = I915_READ(DEIIR);
>  	if (de_iir) {
> +		if (de_iir & DE_ERR_INT_IVB)
> +			ivb_err_int_handler(dev);
> +
>  		if (de_iir & DE_AUX_CHANNEL_A_IVB)
>  			dp_aux_irq_handler(dev);
>  
> @@ -1964,7 +2015,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
>  		DE_PLANEC_FLIP_DONE_IVB |
>  		DE_PLANEB_FLIP_DONE_IVB |
>  		DE_PLANEA_FLIP_DONE_IVB |
> -		DE_AUX_CHANNEL_A_IVB;
> +		DE_AUX_CHANNEL_A_IVB | DE_ERR_INT_IVB;
>  	u32 render_irqs;
>  	u32 hotplug_mask;
>  	u32 pch_irq_mask;
> @@ -1972,6 +2023,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
>  	dev_priv->irq_mask = ~display_mask;
>  
>  	/* should always can generate irq */
> +	I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
>  	I915_WRITE(DEIIR, I915_READ(DEIIR));
>  	I915_WRITE(DEIMR, dev_priv->irq_mask);
>  	I915_WRITE(DEIER,
> @@ -2135,6 +2187,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
>  	I915_WRITE(DEIMR, 0xffffffff);
>  	I915_WRITE(DEIER, 0x0);
>  	I915_WRITE(DEIIR, I915_READ(DEIIR));
> +	if (IS_GEN7(dev))
> +		I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
>  
>  	I915_WRITE(GTIMR, 0xffffffff);
>  	I915_WRITE(GTIER, 0x0);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f054554..e8ecdc4 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -511,7 +511,26 @@
>  
>  #define ERROR_GEN6	0x040a0
>  #define GEN7_ERR_INT	0x44040
> -#define   ERR_INT_MMIO_UNCLAIMED (1<<13)
> +#define  ERR_INT_POISON			(1<<31)
> +#define  ERR_INT_INVALID_GTT_PTE	(1<<29)
> +#define  ERR_INT_INVALID_PTE_DATA	(1<<28)
> +#define  ERR_INT_SPRITE_GTT_FAULT_C	(1<<23)
> +#define  ERR_INT_PRIMARY_GTT_FAULT_C	(1<<22)
> +#define  ERR_INT_CURSOR_GTT_FAULT_C	(1<<21)
> +#define  ERR_INT_SPRITE_GTT_FAULT_B	(1<<20)
> +#define  ERR_INT_PRIMARY_GTT_FAULT_B	(1<<19)
> +#define  ERR_INT_CURSOR_GTT_FAULT_B	(1<<18)
> +#define  ERR_INT_SPRITE_GTT_FAULT_A	(1<<17)
> +#define  ERR_INT_PRIMARY_GTT_FAULT_A	(1<<16)
> +#define  ERR_INT_CURSOR_GTT_FAULT_A	(1<<15)
> +#define  ERR_INT_PIPE_CRR_ERROR_C	(1<<7)
> +#define  ERR_INT_PIPE_FIFO_UNDERRRUN_C	(1<<6)
> +#define  ERR_INT_PIPE_CRR_ERROR_B	(1<<4)
> +#define  ERR_INT_PIPE_FIFO_UNDERRRUN_B	(1<<3)
> +#define  ERR_INT_PIPE_CRR_ERROR_A	(1<<1)
> +#define  ERR_INT_PIPE_FIFO_UNDERRRUN_A	(1<<0)
> +/* Haswell only: */
> +#define  ERR_INT_MMIO_UNCLAIMED		(1<<13)
>  
>  /* GM45+ chicken bits -- debug workaround bits that may be required
>   * for various sorts of correct behavior.  The top 16 bits of each are
> @@ -3374,7 +3393,7 @@
>  #define DE_PIPEA_FIFO_UNDERRUN  (1 << 0)
>  
>  /* More Ivybridge lolz */
> -#define DE_ERR_DEBUG_IVB		(1<<30)
> +#define DE_ERR_INT_IVB			(1<<30)
>  #define DE_GSE_IVB			(1<<29)
>  #define DE_PCH_EVENT_IVB		(1<<28)
>  #define DE_DP_A_HOTPLUG_IVB		(1<<27)
> -- 
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/10] drm/i915: remove "unclaimed register" checks from I915_WRITE

[Paulo Zanoni]

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

Some developers don't really like this code polluting I915_WRITE, and
we've never really measured its negative impacts. So now that we
properly print ERR_INT interrupts, let's remove the I915_WRITE code
and promote the interrupt error message to DRM_ERROR.

The downside of this change is that we lose the ability to check the
register and print nice backtraces, but at this point most of the
errors have already been fixed and we're investigating the few
remaining cases.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c |    8 --------
 drivers/gpu/drm/i915/i915_irq.c |    2 +-
 2 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 7935606..0fe9711 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1260,10 +1260,6 @@ void i915_write##x(struct drm_i915_private *dev_priv, u32 reg, u##x val) { \
 	} \
 	if (IS_GEN5(dev_priv->dev)) \
 		ilk_dummy_write(dev_priv); \
-	if (IS_HASWELL(dev_priv->dev) && (I915_READ_NOTRACE(GEN7_ERR_INT) & ERR_INT_MMIO_UNCLAIMED)) { \
-		DRM_ERROR("Unknown unclaimed register before writing to %x\n", reg); \
-		I915_WRITE_NOTRACE(GEN7_ERR_INT, ERR_INT_MMIO_UNCLAIMED); \
-	} \
 	if (IS_VALLEYVIEW(dev_priv->dev) && IS_DISPLAYREG(reg)) { \
 		write##y(val, dev_priv->regs + reg + 0x180000);		\
 	} else {							\
@@ -1272,10 +1268,6 @@ void i915_write##x(struct drm_i915_private *dev_priv, u32 reg, u##x val) { \
 	if (unlikely(__fifo_ret)) { \
 		gen6_gt_check_fifodbg(dev_priv); \
 	} \
-	if (IS_HASWELL(dev_priv->dev) && (I915_READ_NOTRACE(GEN7_ERR_INT) & ERR_INT_MMIO_UNCLAIMED)) { \
-		DRM_ERROR("Unclaimed write to %x\n", reg); \
-		writel(ERR_INT_MMIO_UNCLAIMED, dev_priv->regs + GEN7_ERR_INT);	\
-	} \
 }
 __i915_write(8, b)
 __i915_write(16, w)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 2f17b54..f0c53d7 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -740,7 +740,7 @@ static void ivb_err_int_handler(struct drm_device *dev)
 		DRM_DEBUG_KMS("Pipe A FIFO underrun\n");
 
 	if (IS_HASWELL(dev) && (err_int & ERR_INT_MMIO_UNCLAIMED))
-		DRM_DEBUG_KMS("MMIO cycle not claimed\n");
+		DRM_ERROR("MMIO cycle not claimed\n");
 
 	I915_WRITE(GEN7_ERR_INT, err_int);
 }
-- 
1.7.10.4

Re: [PATCH 10/10] drm/i915: remove "unclaimed register" checks from I915_WRITE

[Ben Widawsky]

On Fri, Jan 18, 2013 at 06:29:12PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Some developers don't really like this code polluting I915_WRITE, and
> we've never really measured its negative impacts. So now that we
> properly print ERR_INT interrupts, let's remove the I915_WRITE code
> and promote the interrupt error message to DRM_ERROR.
> 
> The downside of this change is that we lose the ability to check the
> register and print nice backtraces, but at this point most of the
> errors have already been fixed and we're investigating the few
> remaining cases.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

I'm really sad to see this go. Especially since our time between new
platform bring-up is decreasing so much. If I were to guess right, every
developer working on a new platform would want this. So while HSW may be
in the clear, HSW+1 suffers.

Could I request that you measure the negative impacts?

Re: [PATCH 10/10] drm/i915: remove "unclaimed register" checks from I915_WRITE

[Chris Wilson]

On Fri, 18 Jan 2013 12:49:13 -0800, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Fri, Jan 18, 2013 at 06:29:12PM -0200, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > Some developers don't really like this code polluting I915_WRITE, and
> > we've never really measured its negative impacts. So now that we
> > properly print ERR_INT interrupts, let's remove the I915_WRITE code
> > and promote the interrupt error message to DRM_ERROR.
> > 
> > The downside of this change is that we lose the ability to check the
> > register and print nice backtraces, but at this point most of the
> > errors have already been fixed and we're investigating the few
> > remaining cases.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> I'm really sad to see this go. Especially since our time between new
> platform bring-up is decreasing so much. If I were to guess right, every
> developer working on a new platform would want this. So while HSW may be
> in the clear, HSW+1 suffers.

I don't like the extra work per iowrite32, but I can live with for the
error-detection. I would rather remove it for special cases where it has
demonstrable impact, perhaps in execbuffer. However, you can equally lay
the blame there for execbuffer hitting i915_write32 too often. So even
there I am not convinced that speeding up i915_write32 is the best
approach.

So NAKed-by: Chris Wilson <chris@chris-wilson.co.uk> unless you can find
a way to work out the faulting register address in the interrupt or that
is the only method going forward.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre