[PATCH] [VPG HSW-A] drm/i915: BUN vol4g[DevHSW] Vblank interrupt on disabled pipe

[PATCH] [VPG HSW-A] drm/i915: BUN vol4g[DevHSW] Vblank interrupt on disabled pipe

[Sandeep Ramankutty]

This change is to comply with the BUN - Vblank interrupt on disabled pipe.
BUN states - Do not unmask and enable a vertical blank interrupt on a pipe
that is not enabled. Do not leave this interrupt enabled and unmasked after
the associated pipe is disabled. If the vblank interrupt is unmasked and
enabled on a disabled pipe it will block package C3, wasting power.

Issue: HSD link - https://hsdhsw.intel.com/hsd/haswell_platform/default.aspx#sighting/default.aspx?sighting_id=4391617
Change Details:
drmP.h
Add two new functions to enable and disable the IER bit for Vblank interrupt.
i915_irq.c
Set these functions to interrupt_enable and interrupt_disable.
intel_display.c
Call interrupt_enable(), from intel_enable_pipe(), after pipe is enabled.
Call interrupt_disable(), from intel_disable_pipe(), after pipe is disabled.

Change-Id: I982654f6ca9a5d48b3d7bc569069a1b0b38f4280
Signed-off-by: Sandeep Ramankutty <sandeepx.ramakutty@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      |   51 ++++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_display.c |    6 +++-
 include/drm/drmP.h                   |   20 +++++++++++++
 3 files changed, 70 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index bc14d30..89a5793 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -27,7 +27,6 @@
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include <linux/sysrq.h>
 #include <linux/slab.h>
 #include <drm/drmP.h>
@@ -2216,6 +2215,35 @@ static int ironlake_enable_vblank(struct drm_device *dev, int pipe)
 	return 0;
 }
 
+/* Enable Vblank interrupt for the pipe. set IER bit. Bit - 0(A),5(B),10(c) */
+static void hsw_enable_interrupt(struct drm_device *dev, int pipe)
+{
+	u32 de_ier;
+	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
+	/* Enable Vblank interrupt only if pipe is enabled */
+	if (IS_HASWELL(dev) && i915_pipe_enabled(dev, pipe)) {
+		de_ier = I915_READ(DEIER);
+		de_ier |= (DE_PIPEA_VBLANK_IVB << (5 * pipe));
+		I915_WRITE(DEIER, de_ier);
+		POSTING_READ(DEIMR);
+	} else
+		DRM_ERROR("Vblank Interrupt enable not allowed\n");
+}
+
+/* Disable Vblank interrupt for the pipe. set IER bit to 0. */
+static void hsw_disable_interrupt(struct drm_device *dev, int pipe)
+{
+	u32 de_ier;
+	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
+	if (IS_HASWELL(dev)) {
+		de_ier = I915_READ(DEIER);
+		de_ier &= (~(DE_PIPEA_VBLANK_IVB << (5 * pipe)));
+		I915_WRITE(DEIER, de_ier);
+		POSTING_READ(DEIMR);
+	} else
+		DRM_ERROR("Vblank Interrupt disable failed\n");
+}
+
 static int ivybridge_enable_vblank(struct drm_device *dev, int pipe)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
@@ -2768,11 +2796,17 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
 	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,
-		   display_mask |
-		   DE_PIPEC_VBLANK_IVB |
-		   DE_PIPEB_VBLANK_IVB |
-		   DE_PIPEA_VBLANK_IVB);
+
+	/* For Haswell, do not enable the Vblank interrupt by default.
+		Enable the interrupt when pipe is enabled */
+	if (IS_HASWELL(dev)) {
+		I915_WRITE(DEIER, display_mask);
+	} else
+		I915_WRITE(DEIER,
+			display_mask |
+			DE_PIPEC_VBLANK_IVB |
+			DE_PIPEB_VBLANK_IVB |
+			DE_PIPEA_VBLANK_IVB);
 	POSTING_READ(DEIER);
 
 	dev_priv->gt_irq_mask = ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
@@ -3623,6 +3657,11 @@ void intel_irq_init(struct drm_device *dev)
 		dev->driver->irq_uninstall = ironlake_irq_uninstall;
 		dev->driver->enable_vblank = ivybridge_enable_vblank;
 		dev->driver->disable_vblank = ivybridge_disable_vblank;
+		/* handlers to enable and disable vblank interrupt */
+		if (IS_HASWELL(dev)) {
+			dev->driver->enable_interrupt = hsw_enable_interrupt;
+			dev->driver->disable_interrupt = hsw_disable_interrupt;
+		}
 		dev_priv->display.hpd_irq_setup = ibx_hpd_irq_setup;
 	} else if (HAS_PCH_SPLIT(dev)) {
 		dev->driver->irq_handler = ironlake_irq_handler;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 24fa298..3efc25a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1646,6 +1646,9 @@ static void intel_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe,
 		return;
 
 	I915_WRITE(reg, val | PIPECONF_ENABLE);
+	/* Enable Vblank interrupt for the pipe */
+	dev_priv->dev->driver->enable_interrupt(dev_priv->dev, pipe);
+
 	intel_wait_for_vblank(dev_priv->dev, pipe);
 }
 
@@ -1684,7 +1687,8 @@ static void intel_disable_pipe(struct drm_i915_private *dev_priv,
 	val = I915_READ(reg);
 	if ((val & PIPECONF_ENABLE) == 0)
 		return;
-
+	/* Disable Vblank interrupt for the pipe */
+	dev_priv->dev->driver->disable_interrupt(dev_priv->dev, pipe);
 	I915_WRITE(reg, val & ~PIPECONF_ENABLE);
 	intel_wait_for_pipe_off(dev_priv->dev, pipe);
 }
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 4ccc955..78ad5dc 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -805,6 +805,26 @@ struct drm_driver {
 	void (*disable_vblank) (struct drm_device *dev, int crtc);
 
 	/**
+	* enable_interrupt - enable Vblank interrupt
+	* @dev: DRM device
+	* @crtc: which pipe to enable
+	*
+	* Enable Vblank interrupt for @crtc.  If the device is connected
+	* i.e. the pipe is enabled, the Vblank interrupt is enabled.
+	*/
+	void (*enable_interrupt) (struct drm_device *dev, int crtc);
+
+	/**
+	* disable_interrupt - disable interrupt
+	* @dev: DRM device
+	* @crtc: which pipe to enable
+	*
+	* Disable vblank interrupts for @crtc.  If the device is disconnected
+	* or pipe is disabled the interrupt is disabled.
+	*/
+	void (*disable_interrupt) (struct drm_device *dev, int crtc);
+
+	/**
 	 * Called by \c drm_device_is_agp.  Typically used to determine if a
 	 * card is really attached to AGP or not.
 	 *
-- 
1.7.9.5

Re: [PATCH] [VPG HSW-A] drm/i915: BUN vol4g[DevHSW] Vblank interrupt on disabled pipe

[Chris Wilson]

On Wed, Sep 04, 2013 at 05:14:02PM +0530, Sandeep Ramankutty wrote:
> This change is to comply with the BUN - Vblank interrupt on disabled pipe.
> BUN states - Do not unmask and enable a vertical blank interrupt on a pipe
> that is not enabled. Do not leave this interrupt enabled and unmasked after
> the associated pipe is disabled. If the vblank interrupt is unmasked and
> enabled on a disabled pipe it will block package C3, wasting power.

See drm_vblank_off().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] [VPG HSW-A] drm/i915: BUN vol4g[DevHSW] Vblank interrupt on disabled pipe

[Ville Syrjälä]

On Wed, Sep 04, 2013 at 05:14:02PM +0530, Sandeep Ramankutty wrote:
> This change is to comply with the BUN - Vblank interrupt on disabled pipe.
> BUN states - Do not unmask and enable a vertical blank interrupt on a pipe
> that is not enabled. Do not leave this interrupt enabled and unmasked after
> the associated pipe is disabled. If the vblank interrupt is unmasked and
> enabled on a disabled pipe it will block package C3, wasting power.

We mask the interrupt in IMR when we don't need it, but we do leave it
enabled in IER. Is that a problem on HSW? I'm guessing not, since
powertop is telling me that I'm currently reaching pc8 w/ displays off,
and pc7 w/ eDP on.

In any case the patch would be buggy since it has no locking around
DEIER RMW accesses. But since powertop is telling me everything is
already fine, we shouldn't need this patch anyway.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] [VPG HSW-A] drm/i915: BUN vol4g[DevHSW] Vblank interrupt on disabled pipe

[Ramakutty, SandeepX]

Hi Ville,

Thanks for the feedback.

The BUN indicates this as a workaround for hardware limitation. It specifies that if the interrupt is not disabled on a pipe that is disabled, the system will not go to C3 state. This affects situations when only edp is connected and hdmi is not connected. That is Pipe B is disabled.  Since this is generic for all haswell platforms not sure whether non Android systems goes to the C3 state if the BUN is not implemented.

Details can be found in North Display Engine Registers-> North Display Engine Shared Functions -> Interrupts 

Regards,
Sandeep

-----Original Message-----
From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
Sent: Wednesday, September 04, 2013 6:16 PM
To: Ramakutty, SandeepX
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] [VPG HSW-A] drm/i915: BUN vol4g[DevHSW] Vblank interrupt on disabled pipe

On Wed, Sep 04, 2013 at 05:14:02PM +0530, Sandeep Ramankutty wrote:
> This change is to comply with the BUN - Vblank interrupt on disabled pipe.
> BUN states - Do not unmask and enable a vertical blank interrupt on a 
> pipe that is not enabled. Do not leave this interrupt enabled and 
> unmasked after the associated pipe is disabled. If the vblank 
> interrupt is unmasked and enabled on a disabled pipe it will block package C3, wasting power.

We mask the interrupt in IMR when we don't need it, but we do leave it enabled in IER. Is that a problem on HSW? I'm guessing not, since powertop is telling me that I'm currently reaching pc8 w/ displays off, and pc7 w/ eDP on.

In any case the patch would be buggy since it has no locking around DEIER RMW accesses. But since powertop is telling me everything is already fine, we shouldn't need this patch anyway.

--
Ville Syrjälä
Intel OTC

Re: [PATCH] [VPG HSW-A] drm/i915: BUN vol4g[DevHSW] Vblank interrupt on disabled pipe

[Ville Syrjälä]

On Thu, Nov 07, 2013 at 11:56:22AM +0000, Ramakutty, SandeepX wrote:
> Hi Ville,
> 
> Thanks for the feedback.
> 
> The BUN indicates this as a workaround for hardware limitation. It specifies that if the interrupt is not disabled on a pipe that is disabled, the system will not go to C3 state. This affects situations when only edp is connected and hdmi is not connected. That is Pipe B is disabled.  Since this is generic for all haswell platforms not sure whether non Android systems goes to the C3 state if the BUN is not implemented.
> 
> Details can be found in North Display Engine Registers-> North Display Engine Shared Functions -> Interrupts 

It says "enabled and _unmasked_". We mask it, so there's no problem.

> 
> Regards,
> Sandeep
> 
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
> Sent: Wednesday, September 04, 2013 6:16 PM
> To: Ramakutty, SandeepX
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] [VPG HSW-A] drm/i915: BUN vol4g[DevHSW] Vblank interrupt on disabled pipe
> 
> On Wed, Sep 04, 2013 at 05:14:02PM +0530, Sandeep Ramankutty wrote:
> > This change is to comply with the BUN - Vblank interrupt on disabled pipe.
> > BUN states - Do not unmask and enable a vertical blank interrupt on a 
> > pipe that is not enabled. Do not leave this interrupt enabled and 
> > unmasked after the associated pipe is disabled. If the vblank 
> > interrupt is unmasked and enabled on a disabled pipe it will block package C3, wasting power.
> 
> We mask the interrupt in IMR when we don't need it, but we do leave it enabled in IER. Is that a problem on HSW? I'm guessing not, since powertop is telling me that I'm currently reaching pc8 w/ displays off, and pc7 w/ eDP on.
> 
> In any case the patch would be buggy since it has no locking around DEIER RMW accesses. But since powertop is telling me everything is already fine, we shouldn't need this patch anyway.
> 
> --
> Ville Syrjälä
> Intel OTC

-- 
Ville Syrjälä
Intel OTC