[PATCH 0/9] Haswell Package C8+

[PATCH 0/9] Haswell Package C8+

[Paulo Zanoni]

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

Hi

Here's my next attempt at the Haswell PC8+ feature.

What's new?

 1. I hope I implemented the feedback from Chris. Many function renames, a few
    style changes. Chris' biggest concern was about the interrupts. So in this
    series you'll see patches 2-6 moving some interrupt code around, so all
    changes to each IMR register should go through a single function. With this
    change we can easily detect when someone wants to change IMR but interrupts
    are disabled. As he suggested, we also don't enable the interrupts in that
    case.
 2. Another difference is that now we also disable PC8 when there's GPU work to
    do. The lack of interrupts made things like glxgears work at 2 FPS. I talked
    to Ben about this and what I understood is that the lack of interrupts is
    not really a problem, it just makes things slower. But still I decided to
    write the code to disable PC8 when we have GPU work to do, so background
    apps can run faster than 2 FPS.
 3. Another difference is that now we enable PC8 on a delayed work function that
    only gets called if we stay with 0 refcount for at least 5 seconds. So when
    someone runs "xrandr" we won't enable/disable PC8 dozens of times. Also, on
    "xset dpms force off" we'll only get to PC8 if the desktop environment
    decides to not do rendering every second. Not getting to PC8? Fix your
    desktop environment!
 4. Due to 3 and the fact that Daniel didn't seem to like my "do DP AUX and
    GMBUS without interrupts" approach, the previous patches that implemented
    this just got dropped: we now have the delayed work thing which should
    replace them.
 5. I also added code to wake up from PC8 when doing suspend, we need this.

Despite this, the other thing to discuss is about the size of patch 7. I can
certainly try to split it, but I really think that if all you want is to take a
brief look at the code just and drop some bikesheds, then having a big patch
that implements everything in one piece seems better. I can split this later if
needed. I'm also open to ideas on how to really split this patch. Also notice
that even if the patch is big, it doesn't remove a single line of the current
code. And it adds PC8 disabled by default, so if git bisect points to that patch
the surface to look will be small.

Another thing worth mentioning is that all this code doesn't have IS_HASWELL
checks, and on non-Haswell platforms the refcount will never reach 0, so we
won't ever try to enable PC8. I'm not sure if that's what we want, so please
comment on that.

Even if we decide to delay patch 7, we could try to merge patches 1-6 if they
look acceptable. I'm also not really sure if we want the last patch yet, but
it's there just in case.

Also, I couldn't do i-g-t regression testing since the i-g-t test suite is quite
unreliable on Haswell right now.

Thanks,
Paulo

Paulo Zanoni (9):
  drm/i915: add the FCLK case to intel_ddi_get_cdclk_freq
  drm/i915: wrap GTIMR changes
  drm/i915: wrap GEN6_PMIMR changes
  drm/i915: don't update GEN6_PMIMR when it's not needed
  drm/i915: add dev_priv->pm_irq_mask
  drm/i915: don't disable/reenable IVB error interrupts when not needed
  drm/i915: allow package C8+ states on Haswell (disabled)
  drm/i915: add i915_pc8_status debugfs file
  drm/i915: enable Package C8+ by default

 drivers/gpu/drm/i915/i915_debugfs.c     |  25 ++++
 drivers/gpu/drm/i915/i915_dma.c         |  10 ++
 drivers/gpu/drm/i915/i915_drv.c         |  11 ++
 drivers/gpu/drm/i915/i915_drv.h         |  73 ++++++++++++
 drivers/gpu/drm/i915/i915_irq.c         | 202 +++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_ddi.c        |   9 +-
 drivers/gpu/drm/i915/intel_display.c    | 164 ++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp.c         |   3 +
 drivers/gpu/drm/i915/intel_drv.h        |  14 +++
 drivers/gpu/drm/i915/intel_i2c.c        |   2 +
 drivers/gpu/drm/i915/intel_pm.c         |  13 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c |  32 ++---
 12 files changed, 518 insertions(+), 40 deletions(-)

-- 
1.8.1.2

[PATCH 1/9] drm/i915: add the FCLK case to intel_ddi_get_cdclk_freq

[Paulo Zanoni]

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

We already have code to disable LCPLL and switch to FCLK, so we need this too.
We still don't call the code to disable LCPLL, but we'll call it when we add
support for Package C8+.

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

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index b8c096b..63aca49 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1139,10 +1139,13 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
 
 int intel_ddi_get_cdclk_freq(struct drm_i915_private *dev_priv)
 {
-	if (I915_READ(HSW_FUSE_STRAP) & HSW_CDCLK_LIMIT)
+	uint32_t lcpll = I915_READ(LCPLL_CTL);
+
+	if (lcpll & LCPLL_CD_SOURCE_FCLK)
+		return 800000;
+	else if (I915_READ(HSW_FUSE_STRAP) & HSW_CDCLK_LIMIT)
 		return 450000;
-	else if ((I915_READ(LCPLL_CTL) & LCPLL_CLK_FREQ_MASK) ==
-		 LCPLL_CLK_FREQ_450)
+	else if ((lcpll & LCPLL_CLK_FREQ_MASK) == LCPLL_CLK_FREQ_450)
 		return 450000;
 	else if (IS_ULT(dev_priv->dev))
 		return 337500;
-- 
1.8.1.2

Re: [PATCH 1/9] drm/i915: add the FCLK case to intel_ddi_get_cdclk_freq

[Rodrigo Vivi]

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

Thanks for pointing me the doc that explains why 800 MHz when using FCLK input. ;)

On Tue, Aug 06, 2013 at 06:57:11PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We already have code to disable LCPLL and switch to FCLK, so we need this too.
> We still don't call the code to disable LCPLL, but we'll call it when we add
> support for Package C8+.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index b8c096b..63aca49 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1139,10 +1139,13 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
>  
>  int intel_ddi_get_cdclk_freq(struct drm_i915_private *dev_priv)
>  {
> -	if (I915_READ(HSW_FUSE_STRAP) & HSW_CDCLK_LIMIT)
> +	uint32_t lcpll = I915_READ(LCPLL_CTL);
> +
> +	if (lcpll & LCPLL_CD_SOURCE_FCLK)
> +		return 800000;
> +	else if (I915_READ(HSW_FUSE_STRAP) & HSW_CDCLK_LIMIT)
>  		return 450000;
> -	else if ((I915_READ(LCPLL_CTL) & LCPLL_CLK_FREQ_MASK) ==
> -		 LCPLL_CLK_FREQ_450)
> +	else if ((lcpll & LCPLL_CLK_FREQ_MASK) == LCPLL_CLK_FREQ_450)
>  		return 450000;
>  	else if (IS_ULT(dev_priv->dev))
>  		return 337500;
> -- 
> 1.8.1.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/9] drm/i915: wrap GTIMR changes

[Paulo Zanoni]

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

Just like the functions that touch DEIMR and SDEIMR, but for GTIMR.
The new functions contain a POSTING_READ(GTIMR) which was not present
at the 2 callers inside i915_irq.c.

The implementation is based on ibx_display_interrupt_update.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c         | 34 +++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_drv.h        |  3 +++
 drivers/gpu/drm/i915/intel_ringbuffer.c | 22 ++++++---------------
 3 files changed, 39 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 6a1c207..a6e98ea 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -104,6 +104,34 @@ ironlake_disable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
 	}
 }
 
+/**
+ * ilk_update_gt_irq - update GTIMR
+ * @dev_priv: driver private
+ * @interrupt_mask: mask of interrupt bits to update
+ * @enabled_irq_mask: mask of interrupt bits to enable
+ */
+static void ilk_update_gt_irq(struct drm_i915_private *dev_priv,
+			      uint32_t interrupt_mask,
+			      uint32_t enabled_irq_mask)
+{
+	assert_spin_locked(&dev_priv->irq_lock);
+
+	dev_priv->gt_irq_mask &= ~interrupt_mask;
+	dev_priv->gt_irq_mask |= (~enabled_irq_mask & interrupt_mask);
+	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
+	POSTING_READ(GTIMR);
+}
+
+void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask)
+{
+	ilk_update_gt_irq(dev_priv, mask, mask);
+}
+
+void ilk_disable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask)
+{
+	ilk_update_gt_irq(dev_priv, mask, 0);
+}
+
 static bool ivb_can_enable_err_int(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -806,8 +834,7 @@ static void ivybridge_parity_work(struct work_struct *work)
 	I915_WRITE(GEN7_MISCCPCTL, misccpctl);
 
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
-	dev_priv->gt_irq_mask &= ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
-	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
+	ilk_enable_gt_irq(dev_priv, GT_RENDER_L3_PARITY_ERROR_INTERRUPT);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
 
 	mutex_unlock(&dev_priv->dev->struct_mutex);
@@ -837,8 +864,7 @@ static void ivybridge_parity_error_irq_handler(struct drm_device *dev)
 		return;
 
 	spin_lock(&dev_priv->irq_lock);
-	dev_priv->gt_irq_mask |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
-	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
+	ilk_disable_gt_irq(dev_priv, GT_RENDER_L3_PARITY_ERROR_INTERRUPT);
 	spin_unlock(&dev_priv->irq_lock);
 
 	queue_work(dev_priv->wq, &dev_priv->l3_parity.error_work);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 54e389d..82bc78e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -838,5 +838,8 @@ extern void intel_edp_psr_update(struct drm_device *dev);
 extern void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
 			      bool switch_to_fclk, bool allow_power_down);
 extern void hsw_restore_lcpll(struct drm_i915_private *dev_priv);
+extern void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
+extern void ilk_disable_gt_irq(struct drm_i915_private *dev_priv,
+			       uint32_t mask);
 
 #endif /* __INTEL_DRV_H__ */
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 74d02a7..6eeca1e 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -836,11 +836,8 @@ gen5_ring_get_irq(struct intel_ring_buffer *ring)
 		return false;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
-	if (ring->irq_refcount++ == 0) {
-		dev_priv->gt_irq_mask &= ~ring->irq_enable_mask;
-		I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
-		POSTING_READ(GTIMR);
-	}
+	if (ring->irq_refcount++ == 0)
+		ilk_enable_gt_irq(dev_priv, ring->irq_enable_mask);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
 
 	return true;
@@ -854,11 +851,8 @@ gen5_ring_put_irq(struct intel_ring_buffer *ring)
 	unsigned long flags;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
-	if (--ring->irq_refcount == 0) {
-		dev_priv->gt_irq_mask |= ring->irq_enable_mask;
-		I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
-		POSTING_READ(GTIMR);
-	}
+	if (--ring->irq_refcount == 0)
+		ilk_disable_gt_irq(dev_priv, ring->irq_enable_mask);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
 }
 
@@ -1028,9 +1022,7 @@ gen6_ring_get_irq(struct intel_ring_buffer *ring)
 					 GT_RENDER_L3_PARITY_ERROR_INTERRUPT));
 		else
 			I915_WRITE_IMR(ring, ~ring->irq_enable_mask);
-		dev_priv->gt_irq_mask &= ~ring->irq_enable_mask;
-		I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
-		POSTING_READ(GTIMR);
+		ilk_enable_gt_irq(dev_priv, ring->irq_enable_mask);
 	}
 	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
 
@@ -1051,9 +1043,7 @@ gen6_ring_put_irq(struct intel_ring_buffer *ring)
 				       ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT);
 		else
 			I915_WRITE_IMR(ring, ~0);
-		dev_priv->gt_irq_mask |= ring->irq_enable_mask;
-		I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
-		POSTING_READ(GTIMR);
+		ilk_disable_gt_irq(dev_priv, ring->irq_enable_mask);
 	}
 	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
 
-- 
1.8.1.2

Re: [PATCH 2/9] drm/i915: wrap GTIMR changes

[Rodrigo Vivi]

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

.On Tue, Aug 06, 2013 at 06:57:12PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Just like the functions that touch DEIMR and SDEIMR, but for GTIMR.
> The new functions contain a POSTING_READ(GTIMR) which was not present
> at the 2 callers inside i915_irq.c.
> 
> The implementation is based on ibx_display_interrupt_update.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c         | 34 +++++++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_drv.h        |  3 +++
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 22 ++++++---------------
>  3 files changed, 39 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 6a1c207..a6e98ea 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -104,6 +104,34 @@ ironlake_disable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
>  	}
>  }
>  
> +/**
> + * ilk_update_gt_irq - update GTIMR
> + * @dev_priv: driver private
> + * @interrupt_mask: mask of interrupt bits to update
> + * @enabled_irq_mask: mask of interrupt bits to enable
> + */
> +static void ilk_update_gt_irq(struct drm_i915_private *dev_priv,
> +			      uint32_t interrupt_mask,
> +			      uint32_t enabled_irq_mask)
> +{
> +	assert_spin_locked(&dev_priv->irq_lock);
> +
> +	dev_priv->gt_irq_mask &= ~interrupt_mask;
> +	dev_priv->gt_irq_mask |= (~enabled_irq_mask & interrupt_mask);

my little mind got confused with logic above, but after some minutes I convinced myself this works ;)

> +	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> +	POSTING_READ(GTIMR);
> +}
> +
> +void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> +{
> +	ilk_update_gt_irq(dev_priv, mask, mask);
> +}
> +
> +void ilk_disable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> +{
> +	ilk_update_gt_irq(dev_priv, mask, 0);
> +}
> +
>  static bool ivb_can_enable_err_int(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -806,8 +834,7 @@ static void ivybridge_parity_work(struct work_struct *work)
>  	I915_WRITE(GEN7_MISCCPCTL, misccpctl);
>  
>  	spin_lock_irqsave(&dev_priv->irq_lock, flags);
> -	dev_priv->gt_irq_mask &= ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
> -	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> +	ilk_enable_gt_irq(dev_priv, GT_RENDER_L3_PARITY_ERROR_INTERRUPT);
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
>  
>  	mutex_unlock(&dev_priv->dev->struct_mutex);
> @@ -837,8 +864,7 @@ static void ivybridge_parity_error_irq_handler(struct drm_device *dev)
>  		return;
>  
>  	spin_lock(&dev_priv->irq_lock);
> -	dev_priv->gt_irq_mask |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
> -	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> +	ilk_disable_gt_irq(dev_priv, GT_RENDER_L3_PARITY_ERROR_INTERRUPT);
>  	spin_unlock(&dev_priv->irq_lock);
>  
>  	queue_work(dev_priv->wq, &dev_priv->l3_parity.error_work);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 54e389d..82bc78e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -838,5 +838,8 @@ extern void intel_edp_psr_update(struct drm_device *dev);
>  extern void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
>  			      bool switch_to_fclk, bool allow_power_down);
>  extern void hsw_restore_lcpll(struct drm_i915_private *dev_priv);
> +extern void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
> +extern void ilk_disable_gt_irq(struct drm_i915_private *dev_priv,
> +			       uint32_t mask);
>  
>  #endif /* __INTEL_DRV_H__ */
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 74d02a7..6eeca1e 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -836,11 +836,8 @@ gen5_ring_get_irq(struct intel_ring_buffer *ring)
>  		return false;
>  
>  	spin_lock_irqsave(&dev_priv->irq_lock, flags);
> -	if (ring->irq_refcount++ == 0) {
> -		dev_priv->gt_irq_mask &= ~ring->irq_enable_mask;
> -		I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> -		POSTING_READ(GTIMR);
> -	}
> +	if (ring->irq_refcount++ == 0)
> +		ilk_enable_gt_irq(dev_priv, ring->irq_enable_mask);
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
>  
>  	return true;
> @@ -854,11 +851,8 @@ gen5_ring_put_irq(struct intel_ring_buffer *ring)
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&dev_priv->irq_lock, flags);
> -	if (--ring->irq_refcount == 0) {
> -		dev_priv->gt_irq_mask |= ring->irq_enable_mask;
> -		I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> -		POSTING_READ(GTIMR);
> -	}
> +	if (--ring->irq_refcount == 0)
> +		ilk_disable_gt_irq(dev_priv, ring->irq_enable_mask);
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
>  }
>  
> @@ -1028,9 +1022,7 @@ gen6_ring_get_irq(struct intel_ring_buffer *ring)
>  					 GT_RENDER_L3_PARITY_ERROR_INTERRUPT));
>  		else
>  			I915_WRITE_IMR(ring, ~ring->irq_enable_mask);
> -		dev_priv->gt_irq_mask &= ~ring->irq_enable_mask;
> -		I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> -		POSTING_READ(GTIMR);
> +		ilk_enable_gt_irq(dev_priv, ring->irq_enable_mask);
>  	}
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
>  
> @@ -1051,9 +1043,7 @@ gen6_ring_put_irq(struct intel_ring_buffer *ring)
>  				       ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT);
>  		else
>  			I915_WRITE_IMR(ring, ~0);
> -		dev_priv->gt_irq_mask |= ring->irq_enable_mask;
> -		I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> -		POSTING_READ(GTIMR);
> +		ilk_disable_gt_irq(dev_priv, ring->irq_enable_mask);
>  	}
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
>  
> -- 
> 1.8.1.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/9] drm/i915: wrap GTIMR changes

[Paulo Zanoni]

2013/8/14 Rodrigo Vivi <rodrigo.vivi@gmail.com>
>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>
> .On Tue, Aug 06, 2013 at 06:57:12PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > Just like the functions that touch DEIMR and SDEIMR, but for GTIMR.
> > The new functions contain a POSTING_READ(GTIMR) which was not present
> > at the 2 callers inside i915_irq.c.
> >
> > The implementation is based on ibx_display_interrupt_update.
> >
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c         | 34 +++++++++++++++++++++++++++++----
> >  drivers/gpu/drm/i915/intel_drv.h        |  3 +++
> >  drivers/gpu/drm/i915/intel_ringbuffer.c | 22 ++++++---------------
> >  3 files changed, 39 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 6a1c207..a6e98ea 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -104,6 +104,34 @@ ironlake_disable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
> >       }
> >  }
> >
> > +/**
> > + * ilk_update_gt_irq - update GTIMR
> > + * @dev_priv: driver private
> > + * @interrupt_mask: mask of interrupt bits to update
> > + * @enabled_irq_mask: mask of interrupt bits to enable
> > + */
> > +static void ilk_update_gt_irq(struct drm_i915_private *dev_priv,
> > +                           uint32_t interrupt_mask,
> > +                           uint32_t enabled_irq_mask)
> > +{
> > +     assert_spin_locked(&dev_priv->irq_lock);
> > +
> > +     dev_priv->gt_irq_mask &= ~interrupt_mask;
> > +     dev_priv->gt_irq_mask |= (~enabled_irq_mask & interrupt_mask);
>
> my little mind got confused with logic above, but after some minutes I convinced myself this works ;)

And if this contains a bug, then ibx_display_interrupt_update also
contains a bug :)


>
>
> > +     I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> > +     POSTING_READ(GTIMR);
> > +}
> > +
> > +void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> > +{
> > +     ilk_update_gt_irq(dev_priv, mask, mask);
> > +}
> > +
> > +void ilk_disable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> > +{
> > +     ilk_update_gt_irq(dev_priv, mask, 0);
> > +}
> > +
> >  static bool ivb_can_enable_err_int(struct drm_device *dev)
> >  {
> >       struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -806,8 +834,7 @@ static void ivybridge_parity_work(struct work_struct *work)
> >       I915_WRITE(GEN7_MISCCPCTL, misccpctl);
> >
> >       spin_lock_irqsave(&dev_priv->irq_lock, flags);
> > -     dev_priv->gt_irq_mask &= ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
> > -     I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> > +     ilk_enable_gt_irq(dev_priv, GT_RENDER_L3_PARITY_ERROR_INTERRUPT);
> >       spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
> >
> >       mutex_unlock(&dev_priv->dev->struct_mutex);
> > @@ -837,8 +864,7 @@ static void ivybridge_parity_error_irq_handler(struct drm_device *dev)
> >               return;
> >
> >       spin_lock(&dev_priv->irq_lock);
> > -     dev_priv->gt_irq_mask |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
> > -     I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> > +     ilk_disable_gt_irq(dev_priv, GT_RENDER_L3_PARITY_ERROR_INTERRUPT);
> >       spin_unlock(&dev_priv->irq_lock);
> >
> >       queue_work(dev_priv->wq, &dev_priv->l3_parity.error_work);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 54e389d..82bc78e 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -838,5 +838,8 @@ extern void intel_edp_psr_update(struct drm_device *dev);
> >  extern void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
> >                             bool switch_to_fclk, bool allow_power_down);
> >  extern void hsw_restore_lcpll(struct drm_i915_private *dev_priv);
> > +extern void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
> > +extern void ilk_disable_gt_irq(struct drm_i915_private *dev_priv,
> > +                            uint32_t mask);
> >
> >  #endif /* __INTEL_DRV_H__ */
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index 74d02a7..6eeca1e 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -836,11 +836,8 @@ gen5_ring_get_irq(struct intel_ring_buffer *ring)
> >               return false;
> >
> >       spin_lock_irqsave(&dev_priv->irq_lock, flags);
> > -     if (ring->irq_refcount++ == 0) {
> > -             dev_priv->gt_irq_mask &= ~ring->irq_enable_mask;
> > -             I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> > -             POSTING_READ(GTIMR);
> > -     }
> > +     if (ring->irq_refcount++ == 0)
> > +             ilk_enable_gt_irq(dev_priv, ring->irq_enable_mask);
> >       spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
> >
> >       return true;
> > @@ -854,11 +851,8 @@ gen5_ring_put_irq(struct intel_ring_buffer *ring)
> >       unsigned long flags;
> >
> >       spin_lock_irqsave(&dev_priv->irq_lock, flags);
> > -     if (--ring->irq_refcount == 0) {
> > -             dev_priv->gt_irq_mask |= ring->irq_enable_mask;
> > -             I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> > -             POSTING_READ(GTIMR);
> > -     }
> > +     if (--ring->irq_refcount == 0)
> > +             ilk_disable_gt_irq(dev_priv, ring->irq_enable_mask);
> >       spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
> >  }
> >
> > @@ -1028,9 +1022,7 @@ gen6_ring_get_irq(struct intel_ring_buffer *ring)
> >                                        GT_RENDER_L3_PARITY_ERROR_INTERRUPT));
> >               else
> >                       I915_WRITE_IMR(ring, ~ring->irq_enable_mask);
> > -             dev_priv->gt_irq_mask &= ~ring->irq_enable_mask;
> > -             I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> > -             POSTING_READ(GTIMR);
> > +             ilk_enable_gt_irq(dev_priv, ring->irq_enable_mask);
> >       }
> >       spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
> >
> > @@ -1051,9 +1043,7 @@ gen6_ring_put_irq(struct intel_ring_buffer *ring)
> >                                      ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT);
> >               else
> >                       I915_WRITE_IMR(ring, ~0);
> > -             dev_priv->gt_irq_mask |= ring->irq_enable_mask;
> > -             I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> > -             POSTING_READ(GTIMR);
> > +             ilk_disable_gt_irq(dev_priv, ring->irq_enable_mask);
> >       }
> >       spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
> >
> > --
> > 1.8.1.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx




-- 
Paulo Zanoni

[PATCH 3/9] drm/i915: wrap GEN6_PMIMR changes

[Paulo Zanoni]

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

Just like we're doing with the other IMR changes.

One of the functional changes is that not every caller was doing the
POSTING_READ.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c         | 47 ++++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_drv.h        |  3 +++
 drivers/gpu/drm/i915/intel_pm.c         |  2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c |  8 ++----
 4 files changed, 46 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a6e98ea..a00fe05 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -132,6 +132,41 @@ void ilk_disable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask)
 	ilk_update_gt_irq(dev_priv, mask, 0);
 }
 
+/**
+  * snb_update_pm_irq - update GEN6_PMIMR
+  * @dev_priv: driver private
+  * @interrupt_mask: mask of interrupt bits to update
+  * @enabled_irq_mask: mask of interrupt bits to enable
+  */
+static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
+			      uint32_t interrupt_mask,
+			      uint32_t enabled_irq_mask)
+{
+	uint32_t pmimr = I915_READ(GEN6_PMIMR);
+	pmimr &= ~interrupt_mask;
+	pmimr |= (~enabled_irq_mask & interrupt_mask);
+
+	assert_spin_locked(&dev_priv->irq_lock);
+
+	I915_WRITE(GEN6_PMIMR, pmimr);
+	POSTING_READ(GEN6_PMIMR);
+}
+
+void snb_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
+{
+	snb_update_pm_irq(dev_priv, mask, mask);
+}
+
+void snb_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
+{
+	snb_update_pm_irq(dev_priv, mask, 0);
+}
+
+static void snb_set_pm_irq(struct drm_i915_private *dev_priv, uint32_t val)
+{
+	snb_update_pm_irq(dev_priv, 0xffffffff, ~val);
+}
+
 static bool ivb_can_enable_err_int(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -739,15 +774,14 @@ static void gen6_pm_rps_work(struct work_struct *work)
 {
 	drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t,
 						    rps.work);
-	u32 pm_iir, pm_imr;
+	u32 pm_iir;
 	u8 new_delay;
 
 	spin_lock_irq(&dev_priv->irq_lock);
 	pm_iir = dev_priv->rps.pm_iir;
 	dev_priv->rps.pm_iir = 0;
-	pm_imr = I915_READ(GEN6_PMIMR);
 	/* Make sure not to corrupt PMIMR state used by ringbuffer code */
-	I915_WRITE(GEN6_PMIMR, pm_imr & ~GEN6_PM_RPS_EVENTS);
+	snb_enable_pm_irq(dev_priv, GEN6_PM_RPS_EVENTS);
 	spin_unlock_irq(&dev_priv->irq_lock);
 
 	if ((pm_iir & GEN6_PM_RPS_EVENTS) == 0)
@@ -921,8 +955,7 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv,
 
 	spin_lock(&dev_priv->irq_lock);
 	dev_priv->rps.pm_iir |= pm_iir;
-	I915_WRITE(GEN6_PMIMR, dev_priv->rps.pm_iir);
-	POSTING_READ(GEN6_PMIMR);
+	snb_set_pm_irq(dev_priv, dev_priv->rps.pm_iir);
 	spin_unlock(&dev_priv->irq_lock);
 
 	queue_work(dev_priv->wq, &dev_priv->rps.work);
@@ -1005,8 +1038,8 @@ static void hsw_pm_irq_handler(struct drm_i915_private *dev_priv,
 	if (pm_iir & GEN6_PM_RPS_EVENTS) {
 		spin_lock(&dev_priv->irq_lock);
 		dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_RPS_EVENTS;
-		I915_WRITE(GEN6_PMIMR, dev_priv->rps.pm_iir);
-		/* never want to mask useful interrupts. (also posting read) */
+		snb_set_pm_irq(dev_priv, dev_priv->rps.pm_iir);
+		/* never want to mask useful interrupts. */
 		WARN_ON(I915_READ_NOTRACE(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
 		spin_unlock(&dev_priv->irq_lock);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 82bc78e..db7cbd5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -841,5 +841,8 @@ extern void hsw_restore_lcpll(struct drm_i915_private *dev_priv);
 extern void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 extern void ilk_disable_gt_irq(struct drm_i915_private *dev_priv,
 			       uint32_t mask);
+extern void snb_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
+extern void snb_disable_pm_irq(struct drm_i915_private *dev_priv,
+			       uint32_t mask);
 
 #endif /* __INTEL_DRV_H__ */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 9b8c90ea..984250d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3324,7 +3324,7 @@ static void gen6_enable_rps_interrupts(struct drm_device *dev)
 
 	spin_lock_irq(&dev_priv->irq_lock);
 	WARN_ON(dev_priv->rps.pm_iir);
-	I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
+	snb_enable_pm_irq(dev_priv, GEN6_PM_RPS_EVENTS);
 	I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
 	spin_unlock_irq(&dev_priv->irq_lock);
 	/* unmask all PM interrupts */
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 6eeca1e..2ef4335 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1062,10 +1062,8 @@ hsw_vebox_get_irq(struct intel_ring_buffer *ring)
 
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
 	if (ring->irq_refcount++ == 0) {
-		u32 pm_imr = I915_READ(GEN6_PMIMR);
 		I915_WRITE_IMR(ring, ~ring->irq_enable_mask);
-		I915_WRITE(GEN6_PMIMR, pm_imr & ~ring->irq_enable_mask);
-		POSTING_READ(GEN6_PMIMR);
+		snb_enable_pm_irq(dev_priv, ring->irq_enable_mask);
 	}
 	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
 
@@ -1084,10 +1082,8 @@ hsw_vebox_put_irq(struct intel_ring_buffer *ring)
 
 	spin_lock_irqsave(&dev_priv->irq_lock, flags);
 	if (--ring->irq_refcount == 0) {
-		u32 pm_imr = I915_READ(GEN6_PMIMR);
 		I915_WRITE_IMR(ring, ~0);
-		I915_WRITE(GEN6_PMIMR, pm_imr | ring->irq_enable_mask);
-		POSTING_READ(GEN6_PMIMR);
+		snb_disable_pm_irq(dev_priv, ring->irq_enable_mask);
 	}
 	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
 }
-- 
1.8.1.2

Re: [PATCH 3/9] drm/i915: wrap GEN6_PMIMR changes

[Rodrigo Vivi]

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

On Tue, Aug 06, 2013 at 06:57:13PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Just like we're doing with the other IMR changes.
> 
> One of the functional changes is that not every caller was doing the
> POSTING_READ.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c         | 47 ++++++++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_drv.h        |  3 +++
>  drivers/gpu/drm/i915/intel_pm.c         |  2 +-
>  drivers/gpu/drm/i915/intel_ringbuffer.c |  8 ++----
>  4 files changed, 46 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index a6e98ea..a00fe05 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -132,6 +132,41 @@ void ilk_disable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask)
>  	ilk_update_gt_irq(dev_priv, mask, 0);
>  }
>  
> +/**
> +  * snb_update_pm_irq - update GEN6_PMIMR
> +  * @dev_priv: driver private
> +  * @interrupt_mask: mask of interrupt bits to update
> +  * @enabled_irq_mask: mask of interrupt bits to enable
> +  */
> +static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
> +			      uint32_t interrupt_mask,
> +			      uint32_t enabled_irq_mask)
> +{
> +	uint32_t pmimr = I915_READ(GEN6_PMIMR);
> +	pmimr &= ~interrupt_mask;
> +	pmimr |= (~enabled_irq_mask & interrupt_mask);
> +
> +	assert_spin_locked(&dev_priv->irq_lock);
> +
> +	I915_WRITE(GEN6_PMIMR, pmimr);
> +	POSTING_READ(GEN6_PMIMR);
> +}
> +
> +void snb_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> +{
> +	snb_update_pm_irq(dev_priv, mask, mask);
> +}
> +
> +void snb_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> +{
> +	snb_update_pm_irq(dev_priv, mask, 0);
> +}
> +
> +static void snb_set_pm_irq(struct drm_i915_private *dev_priv, uint32_t val)
> +{
> +	snb_update_pm_irq(dev_priv, 0xffffffff, ~val);

this confused more than the first one, but it works!

> +}
> +
>  static bool ivb_can_enable_err_int(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -739,15 +774,14 @@ static void gen6_pm_rps_work(struct work_struct *work)
>  {
>  	drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t,
>  						    rps.work);
> -	u32 pm_iir, pm_imr;
> +	u32 pm_iir;
>  	u8 new_delay;
>  
>  	spin_lock_irq(&dev_priv->irq_lock);
>  	pm_iir = dev_priv->rps.pm_iir;
>  	dev_priv->rps.pm_iir = 0;
> -	pm_imr = I915_READ(GEN6_PMIMR);
>  	/* Make sure not to corrupt PMIMR state used by ringbuffer code */
> -	I915_WRITE(GEN6_PMIMR, pm_imr & ~GEN6_PM_RPS_EVENTS);
> +	snb_enable_pm_irq(dev_priv, GEN6_PM_RPS_EVENTS);
>  	spin_unlock_irq(&dev_priv->irq_lock);
>  
>  	if ((pm_iir & GEN6_PM_RPS_EVENTS) == 0)
> @@ -921,8 +955,7 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv,
>  
>  	spin_lock(&dev_priv->irq_lock);
>  	dev_priv->rps.pm_iir |= pm_iir;
> -	I915_WRITE(GEN6_PMIMR, dev_priv->rps.pm_iir);
> -	POSTING_READ(GEN6_PMIMR);
> +	snb_set_pm_irq(dev_priv, dev_priv->rps.pm_iir);
>  	spin_unlock(&dev_priv->irq_lock);
>  
>  	queue_work(dev_priv->wq, &dev_priv->rps.work);
> @@ -1005,8 +1038,8 @@ static void hsw_pm_irq_handler(struct drm_i915_private *dev_priv,
>  	if (pm_iir & GEN6_PM_RPS_EVENTS) {
>  		spin_lock(&dev_priv->irq_lock);
>  		dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_RPS_EVENTS;
> -		I915_WRITE(GEN6_PMIMR, dev_priv->rps.pm_iir);
> -		/* never want to mask useful interrupts. (also posting read) */
> +		snb_set_pm_irq(dev_priv, dev_priv->rps.pm_iir);
> +		/* never want to mask useful interrupts. */
>  		WARN_ON(I915_READ_NOTRACE(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
>  		spin_unlock(&dev_priv->irq_lock);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 82bc78e..db7cbd5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -841,5 +841,8 @@ extern void hsw_restore_lcpll(struct drm_i915_private *dev_priv);
>  extern void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
>  extern void ilk_disable_gt_irq(struct drm_i915_private *dev_priv,
>  			       uint32_t mask);
> +extern void snb_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
> +extern void snb_disable_pm_irq(struct drm_i915_private *dev_priv,
> +			       uint32_t mask);
>  
>  #endif /* __INTEL_DRV_H__ */
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 9b8c90ea..984250d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3324,7 +3324,7 @@ static void gen6_enable_rps_interrupts(struct drm_device *dev)
>  
>  	spin_lock_irq(&dev_priv->irq_lock);
>  	WARN_ON(dev_priv->rps.pm_iir);
> -	I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
> +	snb_enable_pm_irq(dev_priv, GEN6_PM_RPS_EVENTS);
>  	I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
>  	spin_unlock_irq(&dev_priv->irq_lock);
>  	/* unmask all PM interrupts */
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 6eeca1e..2ef4335 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1062,10 +1062,8 @@ hsw_vebox_get_irq(struct intel_ring_buffer *ring)
>  
>  	spin_lock_irqsave(&dev_priv->irq_lock, flags);
>  	if (ring->irq_refcount++ == 0) {
> -		u32 pm_imr = I915_READ(GEN6_PMIMR);
>  		I915_WRITE_IMR(ring, ~ring->irq_enable_mask);
> -		I915_WRITE(GEN6_PMIMR, pm_imr & ~ring->irq_enable_mask);
> -		POSTING_READ(GEN6_PMIMR);
> +		snb_enable_pm_irq(dev_priv, ring->irq_enable_mask);
>  	}
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
>  
> @@ -1084,10 +1082,8 @@ hsw_vebox_put_irq(struct intel_ring_buffer *ring)
>  
>  	spin_lock_irqsave(&dev_priv->irq_lock, flags);
>  	if (--ring->irq_refcount == 0) {
> -		u32 pm_imr = I915_READ(GEN6_PMIMR);
>  		I915_WRITE_IMR(ring, ~0);
> -		I915_WRITE(GEN6_PMIMR, pm_imr | ring->irq_enable_mask);
> -		POSTING_READ(GEN6_PMIMR);
> +		snb_disable_pm_irq(dev_priv, ring->irq_enable_mask);
>  	}
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
>  }
> -- 
> 1.8.1.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/9] drm/i915: wrap GEN6_PMIMR changes

[Paulo Zanoni]

2013/8/14 Rodrigo Vivi <rodrigo.vivi@gmail.com>:
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>
> On Tue, Aug 06, 2013 at 06:57:13PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Just like we're doing with the other IMR changes.
>>
>> One of the functional changes is that not every caller was doing the
>> POSTING_READ.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c         | 47 ++++++++++++++++++++++++++++-----
>>  drivers/gpu/drm/i915/intel_drv.h        |  3 +++
>>  drivers/gpu/drm/i915/intel_pm.c         |  2 +-
>>  drivers/gpu/drm/i915/intel_ringbuffer.c |  8 ++----
>>  4 files changed, 46 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index a6e98ea..a00fe05 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -132,6 +132,41 @@ void ilk_disable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask)
>>       ilk_update_gt_irq(dev_priv, mask, 0);
>>  }
>>
>> +/**
>> +  * snb_update_pm_irq - update GEN6_PMIMR
>> +  * @dev_priv: driver private
>> +  * @interrupt_mask: mask of interrupt bits to update
>> +  * @enabled_irq_mask: mask of interrupt bits to enable
>> +  */
>> +static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
>> +                           uint32_t interrupt_mask,
>> +                           uint32_t enabled_irq_mask)
>> +{
>> +     uint32_t pmimr = I915_READ(GEN6_PMIMR);
>> +     pmimr &= ~interrupt_mask;
>> +     pmimr |= (~enabled_irq_mask & interrupt_mask);
>> +
>> +     assert_spin_locked(&dev_priv->irq_lock);
>> +
>> +     I915_WRITE(GEN6_PMIMR, pmimr);
>> +     POSTING_READ(GEN6_PMIMR);
>> +}
>> +
>> +void snb_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
>> +{
>> +     snb_update_pm_irq(dev_priv, mask, mask);
>> +}
>> +
>> +void snb_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
>> +{
>> +     snb_update_pm_irq(dev_priv, mask, 0);
>> +}
>> +
>> +static void snb_set_pm_irq(struct drm_i915_private *dev_priv, uint32_t val)
>> +{
>> +     snb_update_pm_irq(dev_priv, 0xffffffff, ~val);
>
> this confused more than the first one, but it works!

I agree it's confusing: I implemented it wrong on my first try. But
snb_set_pm_irq dies on patch 6.2 :)


>
>> +}
>> +
>>  static bool ivb_can_enable_err_int(struct drm_device *dev)
>>  {
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>> @@ -739,15 +774,14 @@ static void gen6_pm_rps_work(struct work_struct *work)
>>  {
>>       drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t,
>>                                                   rps.work);
>> -     u32 pm_iir, pm_imr;
>> +     u32 pm_iir;
>>       u8 new_delay;
>>
>>       spin_lock_irq(&dev_priv->irq_lock);
>>       pm_iir = dev_priv->rps.pm_iir;
>>       dev_priv->rps.pm_iir = 0;
>> -     pm_imr = I915_READ(GEN6_PMIMR);
>>       /* Make sure not to corrupt PMIMR state used by ringbuffer code */
>> -     I915_WRITE(GEN6_PMIMR, pm_imr & ~GEN6_PM_RPS_EVENTS);
>> +     snb_enable_pm_irq(dev_priv, GEN6_PM_RPS_EVENTS);
>>       spin_unlock_irq(&dev_priv->irq_lock);
>>
>>       if ((pm_iir & GEN6_PM_RPS_EVENTS) == 0)
>> @@ -921,8 +955,7 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv,
>>
>>       spin_lock(&dev_priv->irq_lock);
>>       dev_priv->rps.pm_iir |= pm_iir;
>> -     I915_WRITE(GEN6_PMIMR, dev_priv->rps.pm_iir);
>> -     POSTING_READ(GEN6_PMIMR);
>> +     snb_set_pm_irq(dev_priv, dev_priv->rps.pm_iir);
>>       spin_unlock(&dev_priv->irq_lock);
>>
>>       queue_work(dev_priv->wq, &dev_priv->rps.work);
>> @@ -1005,8 +1038,8 @@ static void hsw_pm_irq_handler(struct drm_i915_private *dev_priv,
>>       if (pm_iir & GEN6_PM_RPS_EVENTS) {
>>               spin_lock(&dev_priv->irq_lock);
>>               dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_RPS_EVENTS;
>> -             I915_WRITE(GEN6_PMIMR, dev_priv->rps.pm_iir);
>> -             /* never want to mask useful interrupts. (also posting read) */
>> +             snb_set_pm_irq(dev_priv, dev_priv->rps.pm_iir);
>> +             /* never want to mask useful interrupts. */
>>               WARN_ON(I915_READ_NOTRACE(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
>>               spin_unlock(&dev_priv->irq_lock);
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 82bc78e..db7cbd5 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -841,5 +841,8 @@ extern void hsw_restore_lcpll(struct drm_i915_private *dev_priv);
>>  extern void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
>>  extern void ilk_disable_gt_irq(struct drm_i915_private *dev_priv,
>>                              uint32_t mask);
>> +extern void snb_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
>> +extern void snb_disable_pm_irq(struct drm_i915_private *dev_priv,
>> +                            uint32_t mask);
>>
>>  #endif /* __INTEL_DRV_H__ */
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 9b8c90ea..984250d 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -3324,7 +3324,7 @@ static void gen6_enable_rps_interrupts(struct drm_device *dev)
>>
>>       spin_lock_irq(&dev_priv->irq_lock);
>>       WARN_ON(dev_priv->rps.pm_iir);
>> -     I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
>> +     snb_enable_pm_irq(dev_priv, GEN6_PM_RPS_EVENTS);
>>       I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
>>       spin_unlock_irq(&dev_priv->irq_lock);
>>       /* unmask all PM interrupts */
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> index 6eeca1e..2ef4335 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> @@ -1062,10 +1062,8 @@ hsw_vebox_get_irq(struct intel_ring_buffer *ring)
>>
>>       spin_lock_irqsave(&dev_priv->irq_lock, flags);
>>       if (ring->irq_refcount++ == 0) {
>> -             u32 pm_imr = I915_READ(GEN6_PMIMR);
>>               I915_WRITE_IMR(ring, ~ring->irq_enable_mask);
>> -             I915_WRITE(GEN6_PMIMR, pm_imr & ~ring->irq_enable_mask);
>> -             POSTING_READ(GEN6_PMIMR);
>> +             snb_enable_pm_irq(dev_priv, ring->irq_enable_mask);
>>       }
>>       spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
>>
>> @@ -1084,10 +1082,8 @@ hsw_vebox_put_irq(struct intel_ring_buffer *ring)
>>
>>       spin_lock_irqsave(&dev_priv->irq_lock, flags);
>>       if (--ring->irq_refcount == 0) {
>> -             u32 pm_imr = I915_READ(GEN6_PMIMR);
>>               I915_WRITE_IMR(ring, ~0);
>> -             I915_WRITE(GEN6_PMIMR, pm_imr | ring->irq_enable_mask);
>> -             POSTING_READ(GEN6_PMIMR);
>> +             snb_disable_pm_irq(dev_priv, ring->irq_enable_mask);
>>       }
>>       spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
>>  }
>> --
>> 1.8.1.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

[PATCH 4/9] drm/i915: don't update GEN6_PMIMR when it's not needed

[Paulo Zanoni]

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

I did some brief tests and the "new_val = pmimr" condition usually
happens a few times after exiting games.

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a00fe05..a1255da 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -142,14 +142,18 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
 			      uint32_t interrupt_mask,
 			      uint32_t enabled_irq_mask)
 {
-	uint32_t pmimr = I915_READ(GEN6_PMIMR);
-	pmimr &= ~interrupt_mask;
-	pmimr |= (~enabled_irq_mask & interrupt_mask);
+	uint32_t pmimr, new_val;
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	I915_WRITE(GEN6_PMIMR, pmimr);
-	POSTING_READ(GEN6_PMIMR);
+	pmimr = new_val = I915_READ(GEN6_PMIMR);
+	new_val &= ~interrupt_mask;
+	new_val |= (~enabled_irq_mask & interrupt_mask);
+
+	if (new_val != pmimr) {
+		I915_WRITE(GEN6_PMIMR, new_val);
+		POSTING_READ(GEN6_PMIMR);
+	}
 }
 
 void snb_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
-- 
1.8.1.2

Re: [PATCH 4/9] drm/i915: don't update GEN6_PMIMR when it's not needed

[Chris Wilson]

On Tue, Aug 06, 2013 at 06:57:14PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> I did some brief tests and the "new_val = pmimr" condition usually
> happens a few times after exiting games.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

I'm not sure of the value of this patch by itself. It did make me wonder
what you were micro-optimising, and then I saw patch 5 and it made more
sense.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/9] drm/i915: don't update GEN6_PMIMR when it's not needed

[Paulo Zanoni]

2013/8/6 Chris Wilson <chris@chris-wilson.co.uk>:
> On Tue, Aug 06, 2013 at 06:57:14PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> I did some brief tests and the "new_val = pmimr" condition usually
>> happens a few times after exiting games.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> I'm not sure of the value of this patch by itself. It did make me wonder
> what you were micro-optimising, and then I saw patch 5 and it made more
> sense.

Patches 4 and 5 are just micro optimizations and shouldn't be needed
for the PC8+, but I thought they would be useful. If you think they're
not worth it, we can discard them. I was trying to make the code
similar to the other IMR-changing functions.

If we massage the code a little bit more we could make all the
IMR-changing functions share the same code


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



-- 
Paulo Zanoni

Re: [PATCH 4/9] drm/i915: don't update GEN6_PMIMR when it's not needed

[Chris Wilson]

On Wed, Aug 07, 2013 at 10:34:11AM -0300, Paulo Zanoni wrote:
> 2013/8/6 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Tue, Aug 06, 2013 at 06:57:14PM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> I did some brief tests and the "new_val = pmimr" condition usually
> >> happens a few times after exiting games.
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > I'm not sure of the value of this patch by itself. It did make me wonder
> > what you were micro-optimising, and then I saw patch 5 and it made more
> > sense.
> 
> Patches 4 and 5 are just micro optimizations and shouldn't be needed
> for the PC8+, but I thought they would be useful. If you think they're
> not worth it, we can discard them. I was trying to make the code
> similar to the other IMR-changing functions.

Combined together, I think the micro-optimisation makes sense and would
say it was less of a micro-optimisation than a consistent design to use
the bookkeeping instead of touching registers. Just on its own this
patch caused me to do a double-take and question what your motivation
was.
 
> If we massage the code a little bit more we could make all the
> IMR-changing functions share the same code

Sure, that may be worthwhile. Probably borderline though, I envisage it
will take more code to setup than it will save.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/9] drm/i915: don't update GEN6_PMIMR when it's not needed

[Daniel Vetter]

On Wed, Aug 07, 2013 at 03:14:51PM +0100, Chris Wilson wrote:
> On Wed, Aug 07, 2013 at 10:34:11AM -0300, Paulo Zanoni wrote:
> > 2013/8/6 Chris Wilson <chris@chris-wilson.co.uk>:
> > > On Tue, Aug 06, 2013 at 06:57:14PM -0300, Paulo Zanoni wrote:
> > >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > >>
> > >> I did some brief tests and the "new_val = pmimr" condition usually
> > >> happens a few times after exiting games.
> > >>
> > >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > >
> > > I'm not sure of the value of this patch by itself. It did make me wonder
> > > what you were micro-optimising, and then I saw patch 5 and it made more
> > > sense.
> > 
> > Patches 4 and 5 are just micro optimizations and shouldn't be needed
> > for the PC8+, but I thought they would be useful. If you think they're
> > not worth it, we can discard them. I was trying to make the code
> > similar to the other IMR-changing functions.
> 
> Combined together, I think the micro-optimisation makes sense and would
> say it was less of a micro-optimisation than a consistent design to use
> the bookkeeping instead of touching registers. Just on its own this
> patch caused me to do a double-take and question what your motivation
> was.

I've added a small note to the commit message to (hopefully) capture this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/9] drm/i915: don't update GEN6_PMIMR when it's not needed

[Rodrigo Vivi]

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

I liked this very much... we should do this kind of check in more places...

On Tue, Aug 06, 2013 at 06:57:14PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> I did some brief tests and the "new_val = pmimr" condition usually
> happens a few times after exiting games.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index a00fe05..a1255da 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -142,14 +142,18 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
>  			      uint32_t interrupt_mask,
>  			      uint32_t enabled_irq_mask)
>  {
> -	uint32_t pmimr = I915_READ(GEN6_PMIMR);
> -	pmimr &= ~interrupt_mask;
> -	pmimr |= (~enabled_irq_mask & interrupt_mask);
> +	uint32_t pmimr, new_val;
>  
>  	assert_spin_locked(&dev_priv->irq_lock);
>  
> -	I915_WRITE(GEN6_PMIMR, pmimr);
> -	POSTING_READ(GEN6_PMIMR);
> +	pmimr = new_val = I915_READ(GEN6_PMIMR);
> +	new_val &= ~interrupt_mask;
> +	new_val |= (~enabled_irq_mask & interrupt_mask);
> +
> +	if (new_val != pmimr) {
> +		I915_WRITE(GEN6_PMIMR, new_val);
> +		POSTING_READ(GEN6_PMIMR);
> +	}
>  }
>  
>  void snb_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> -- 
> 1.8.1.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/9] drm/i915: add dev_priv->pm_irq_mask

[Paulo Zanoni]

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

Just like irq_mask and gt_irq_mask, use it to track the status of
GEN6_PMIMR so we don't need to read it again every time we call
snb_update_pm_irq.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9ff09a2..b621f5e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1097,6 +1097,7 @@ typedef struct drm_i915_private {
 	/** Cached value of IMR to avoid reads in updating the bitfield */
 	u32 irq_mask;
 	u32 gt_irq_mask;
+	u32 pm_irq_mask;
 
 	struct work_struct hotplug_work;
 	bool enable_hotplug_processing;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a1255da..d96bd1b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -142,16 +142,17 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
 			      uint32_t interrupt_mask,
 			      uint32_t enabled_irq_mask)
 {
-	uint32_t pmimr, new_val;
+	uint32_t new_val;
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	pmimr = new_val = I915_READ(GEN6_PMIMR);
+	new_val = dev_priv->pm_irq_mask;
 	new_val &= ~interrupt_mask;
 	new_val |= (~enabled_irq_mask & interrupt_mask);
 
-	if (new_val != pmimr) {
-		I915_WRITE(GEN6_PMIMR, new_val);
+	if (new_val != dev_priv->pm_irq_mask) {
+		dev_priv->pm_irq_mask = new_val;
+		I915_WRITE(GEN6_PMIMR, dev_priv->pm_irq_mask);
 		POSTING_READ(GEN6_PMIMR);
 	}
 }
@@ -2221,8 +2222,9 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)
 		if (HAS_VEBOX(dev))
 			pm_irqs |= PM_VEBOX_USER_INTERRUPT;
 
+		dev_priv->pm_irq_mask = 0xffffffff;
 		I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
-		I915_WRITE(GEN6_PMIMR, 0xffffffff);
+		I915_WRITE(GEN6_PMIMR, dev_priv->pm_irq_mask);
 		I915_WRITE(GEN6_PMIER, pm_irqs);
 		POSTING_READ(GEN6_PMIER);
 	}
-- 
1.8.1.2

Re: [PATCH 5/9] drm/i915: add dev_priv->pm_irq_mask

[Rodrigo Vivi]

On Tue, Aug 06, 2013 at 06:57:15PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Just like irq_mask and gt_irq_mask, use it to track the status of
> GEN6_PMIMR so we don't need to read it again every time we call
> snb_update_pm_irq.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  1 +
>  drivers/gpu/drm/i915/i915_irq.c | 12 +++++++-----
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9ff09a2..b621f5e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1097,6 +1097,7 @@ typedef struct drm_i915_private {
>  	/** Cached value of IMR to avoid reads in updating the bitfield */
>  	u32 irq_mask;
>  	u32 gt_irq_mask;
> +	u32 pm_irq_mask;
>  
>  	struct work_struct hotplug_work;
>  	bool enable_hotplug_processing;
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index a1255da..d96bd1b 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -142,16 +142,17 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
>  			      uint32_t interrupt_mask,
>  			      uint32_t enabled_irq_mask)
>  {
> -	uint32_t pmimr, new_val;
> +	uint32_t new_val;
>  
>  	assert_spin_locked(&dev_priv->irq_lock);
>  
> -	pmimr = new_val = I915_READ(GEN6_PMIMR);
> +	new_val = dev_priv->pm_irq_mask;
>  	new_val &= ~interrupt_mask;
>  	new_val |= (~enabled_irq_mask & interrupt_mask);
>  
> -	if (new_val != pmimr) {
> -		I915_WRITE(GEN6_PMIMR, new_val);
> +	if (new_val != dev_priv->pm_irq_mask) {
> +		dev_priv->pm_irq_mask = new_val;
> +		I915_WRITE(GEN6_PMIMR, dev_priv->pm_irq_mask);
>  		POSTING_READ(GEN6_PMIMR);
>  	}
>  }
> @@ -2221,8 +2222,9 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)
>  		if (HAS_VEBOX(dev))
>  			pm_irqs |= PM_VEBOX_USER_INTERRUPT;
>  
> +		dev_priv->pm_irq_mask = 0xffffffff;
>  		I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
> -		I915_WRITE(GEN6_PMIMR, 0xffffffff);

Same write happening at gen5_gt_irq_preinstall...
it is already strange a gen5_ func using a GEN6 reg, but maybe we have to use this same pm_irq_mask there also...


> +		I915_WRITE(GEN6_PMIMR, dev_priv->pm_irq_mask);
>  		I915_WRITE(GEN6_PMIER, pm_irqs);
>  		POSTING_READ(GEN6_PMIER);
>  	}
> -- 
> 1.8.1.2

Anyways, feel free to use:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

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

Re: [PATCH 5/9] drm/i915: add dev_priv->pm_irq_mask

[Paulo Zanoni]

2013/8/14 Rodrigo Vivi <rodrigo.vivi@gmail.com>:
> On Tue, Aug 06, 2013 at 06:57:15PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Just like irq_mask and gt_irq_mask, use it to track the status of
>> GEN6_PMIMR so we don't need to read it again every time we call
>> snb_update_pm_irq.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h |  1 +
>>  drivers/gpu/drm/i915/i915_irq.c | 12 +++++++-----
>>  2 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 9ff09a2..b621f5e 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1097,6 +1097,7 @@ typedef struct drm_i915_private {
>>       /** Cached value of IMR to avoid reads in updating the bitfield */
>>       u32 irq_mask;
>>       u32 gt_irq_mask;
>> +     u32 pm_irq_mask;
>>
>>       struct work_struct hotplug_work;
>>       bool enable_hotplug_processing;
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index a1255da..d96bd1b 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -142,16 +142,17 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
>>                             uint32_t interrupt_mask,
>>                             uint32_t enabled_irq_mask)
>>  {
>> -     uint32_t pmimr, new_val;
>> +     uint32_t new_val;
>>
>>       assert_spin_locked(&dev_priv->irq_lock);
>>
>> -     pmimr = new_val = I915_READ(GEN6_PMIMR);
>> +     new_val = dev_priv->pm_irq_mask;
>>       new_val &= ~interrupt_mask;
>>       new_val |= (~enabled_irq_mask & interrupt_mask);
>>
>> -     if (new_val != pmimr) {
>> -             I915_WRITE(GEN6_PMIMR, new_val);
>> +     if (new_val != dev_priv->pm_irq_mask) {
>> +             dev_priv->pm_irq_mask = new_val;
>> +             I915_WRITE(GEN6_PMIMR, dev_priv->pm_irq_mask);
>>               POSTING_READ(GEN6_PMIMR);
>>       }
>>  }
>> @@ -2221,8 +2222,9 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)
>>               if (HAS_VEBOX(dev))
>>                       pm_irqs |= PM_VEBOX_USER_INTERRUPT;
>>
>> +             dev_priv->pm_irq_mask = 0xffffffff;
>>               I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
>> -             I915_WRITE(GEN6_PMIMR, 0xffffffff);
>
> Same write happening at gen5_gt_irq_preinstall...
> it is already strange a gen5_ func using a GEN6 reg, but maybe we have to use this same pm_irq_mask there also...

The confusing semantics between preinstall, postinstall and uninstall
were addressed in another patch series which I sent a while ago, but I
need to resend it based on the comments I received. I'm going to do
this after I finish the work on PC8+.

For the gen5_ prefix having gen6 code: naming a function
genX_something usually means that this function runs on genX and newer
platforms, so it's acceptable to see genX+1 code there, but never
genX-1. Also, ILK/SNB/IVB/HSW share the same functions for most of the
IRQ code already.

>
>
>> +             I915_WRITE(GEN6_PMIMR, dev_priv->pm_irq_mask);
>>               I915_WRITE(GEN6_PMIER, pm_irqs);
>>               POSTING_READ(GEN6_PMIER);
>>       }
>> --
>> 1.8.1.2
>
> Anyways, feel free to use:
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

Thanks for the reviews!

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



-- 
Paulo Zanoni

[PATCH 6/9] drm/i915: don't disable/reenable IVB error interrupts when not needed

[Paulo Zanoni]

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

If the error interrupts are already disabled, don't disable and
reenable them. This is going to be needed when we're in PC8+, where
all the interrupts are disabled so we won't risk re-enabling
DE_ERR_INT_IVB.

v2: Use dev_priv->irq_mask (Chris)

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d96bd1b..5e7e6f6 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1373,6 +1373,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	u32 de_iir, gt_iir, de_ier, sde_ier = 0;
 	irqreturn_t ret = IRQ_NONE;
+	bool err_int_reenable = false;
 
 	atomic_inc(&dev_priv->irq_received);
 
@@ -1401,7 +1402,9 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 	 * handler. */
 	if (IS_HASWELL(dev)) {
 		spin_lock(&dev_priv->irq_lock);
-		ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
+		err_int_reenable = ~dev_priv->irq_mask & DE_ERR_INT_IVB;
+		if (err_int_reenable)
+			ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
 		spin_unlock(&dev_priv->irq_lock);
 	}
 
@@ -1437,7 +1440,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 		}
 	}
 
-	if (IS_HASWELL(dev)) {
+	if (err_int_reenable) {
 		spin_lock(&dev_priv->irq_lock);
 		if (ivb_can_enable_err_int(dev))
 			ironlake_enable_display_irq(dev_priv, DE_ERR_INT_IVB);
-- 
1.8.1.2

Re: [PATCH 6/9] drm/i915: don't disable/reenable IVB error interrupts when not needed

[Rodrigo Vivi]

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

On Tue, Aug 06, 2013 at 06:57:16PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> If the error interrupts are already disabled, don't disable and
> reenable them. This is going to be needed when we're in PC8+, where
> all the interrupts are disabled so we won't risk re-enabling
> DE_ERR_INT_IVB.
> 
> v2: Use dev_priv->irq_mask (Chris)
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index d96bd1b..5e7e6f6 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1373,6 +1373,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>  	u32 de_iir, gt_iir, de_ier, sde_ier = 0;
>  	irqreturn_t ret = IRQ_NONE;
> +	bool err_int_reenable = false;
>  
>  	atomic_inc(&dev_priv->irq_received);
>  
> @@ -1401,7 +1402,9 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  	 * handler. */
>  	if (IS_HASWELL(dev)) {
>  		spin_lock(&dev_priv->irq_lock);
> -		ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
> +		err_int_reenable = ~dev_priv->irq_mask & DE_ERR_INT_IVB;
> +		if (err_int_reenable)
> +			ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
>  		spin_unlock(&dev_priv->irq_lock);
>  	}
>  
> @@ -1437,7 +1440,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  		}
>  	}
>  
> -	if (IS_HASWELL(dev)) {
> +	if (err_int_reenable) {
>  		spin_lock(&dev_priv->irq_lock);
>  		if (ivb_can_enable_err_int(dev))
>  			ironlake_enable_display_irq(dev_priv, DE_ERR_INT_IVB);
> -- 
> 1.8.1.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/9] drm/i915: don't disable/reenable IVB error interrupts when not needed

[Daniel Vetter]

On Tue, Aug 06, 2013 at 06:57:16PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> If the error interrupts are already disabled, don't disable and
> reenable them. This is going to be needed when we're in PC8+, where
> all the interrupts are disabled so we won't risk re-enabling
> DE_ERR_INT_IVB.
> 
> v2: Use dev_priv->irq_mask (Chris)
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index d96bd1b..5e7e6f6 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1373,6 +1373,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>  	u32 de_iir, gt_iir, de_ier, sde_ier = 0;
>  	irqreturn_t ret = IRQ_NONE;
> +	bool err_int_reenable = false;
>  
>  	atomic_inc(&dev_priv->irq_received);
>  
> @@ -1401,7 +1402,9 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  	 * handler. */
>  	if (IS_HASWELL(dev)) {
>  		spin_lock(&dev_priv->irq_lock);
> -		ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
> +		err_int_reenable = ~dev_priv->irq_mask & DE_ERR_INT_IVB;
> +		if (err_int_reenable)
> +			ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);

Hm, that reminds me that this entire logic here is racy wrt concurrent
interrupt enabling on a different cpu core (e.g. due to a modeset now
again allowing display error interrupts). Do we still need this or could
we just ditch this entire complexity?

Maybe I'm just engaging in wishful thinking here a bit ;-)

Patch applied.
-Daniel

>  		spin_unlock(&dev_priv->irq_lock);
>  	}
>  
> @@ -1437,7 +1440,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  		}
>  	}
>  
> -	if (IS_HASWELL(dev)) {
> +	if (err_int_reenable) {
>  		spin_lock(&dev_priv->irq_lock);
>  		if (ivb_can_enable_err_int(dev))
>  			ironlake_enable_display_irq(dev_priv, DE_ERR_INT_IVB);
> -- 
> 1.8.1.2
> 
> _______________________________________________
> 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 6/9] drm/i915: don't disable/reenable IVB error interrupts when not needed

[Paulo Zanoni]

2013/8/20 Daniel Vetter <daniel@ffwll.ch>:
> On Tue, Aug 06, 2013 at 06:57:16PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> If the error interrupts are already disabled, don't disable and
>> reenable them. This is going to be needed when we're in PC8+, where
>> all the interrupts are disabled so we won't risk re-enabling
>> DE_ERR_INT_IVB.
>>
>> v2: Use dev_priv->irq_mask (Chris)
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index d96bd1b..5e7e6f6 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -1373,6 +1373,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>>       drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>>       u32 de_iir, gt_iir, de_ier, sde_ier = 0;
>>       irqreturn_t ret = IRQ_NONE;
>> +     bool err_int_reenable = false;
>>
>>       atomic_inc(&dev_priv->irq_received);
>>
>> @@ -1401,7 +1402,9 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>>        * handler. */
>>       if (IS_HASWELL(dev)) {
>>               spin_lock(&dev_priv->irq_lock);
>> -             ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
>> +             err_int_reenable = ~dev_priv->irq_mask & DE_ERR_INT_IVB;
>> +             if (err_int_reenable)
>> +                     ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
>
> Hm, that reminds me that this entire logic here is racy wrt concurrent
> interrupt enabling on a different cpu core (e.g. due to a modeset now
> again allowing display error interrupts). Do we still need this or could
> we just ditch this entire complexity?

Can you please explain more? We still check ivb_can_enable_err_int
before reenabling.


>
> Maybe I'm just engaging in wishful thinking here a bit ;-)
>
> Patch applied.
> -Daniel
>
>>               spin_unlock(&dev_priv->irq_lock);
>>       }
>>
>> @@ -1437,7 +1440,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>>               }
>>       }
>>
>> -     if (IS_HASWELL(dev)) {
>> +     if (err_int_reenable) {
>>               spin_lock(&dev_priv->irq_lock);
>>               if (ivb_can_enable_err_int(dev))
>>                       ironlake_enable_display_irq(dev_priv, DE_ERR_INT_IVB);
>> --
>> 1.8.1.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

Re: [PATCH 6/9] drm/i915: don't disable/reenable IVB error interrupts when not needed

[Daniel Vetter]

On Tue, Aug 20, 2013 at 11:43:46AM -0300, Paulo Zanoni wrote:
> 2013/8/20 Daniel Vetter <daniel@ffwll.ch>:
> > On Tue, Aug 06, 2013 at 06:57:16PM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> If the error interrupts are already disabled, don't disable and
> >> reenable them. This is going to be needed when we're in PC8+, where
> >> all the interrupts are disabled so we won't risk re-enabling
> >> DE_ERR_INT_IVB.
> >>
> >> v2: Use dev_priv->irq_mask (Chris)
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_irq.c | 7 +++++--
> >>  1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >> index d96bd1b..5e7e6f6 100644
> >> --- a/drivers/gpu/drm/i915/i915_irq.c
> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> @@ -1373,6 +1373,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
> >>       drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> >>       u32 de_iir, gt_iir, de_ier, sde_ier = 0;
> >>       irqreturn_t ret = IRQ_NONE;
> >> +     bool err_int_reenable = false;
> >>
> >>       atomic_inc(&dev_priv->irq_received);
> >>
> >> @@ -1401,7 +1402,9 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
> >>        * handler. */
> >>       if (IS_HASWELL(dev)) {
> >>               spin_lock(&dev_priv->irq_lock);
> >> -             ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
> >> +             err_int_reenable = ~dev_priv->irq_mask & DE_ERR_INT_IVB;
> >> +             if (err_int_reenable)
> >> +                     ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
> >
> > Hm, that reminds me that this entire logic here is racy wrt concurrent
> > interrupt enabling on a different cpu core (e.g. due to a modeset now
> > again allowing display error interrupts). Do we still need this or could
> > we just ditch this entire complexity?
> 
> Can you please explain more? We still check ivb_can_enable_err_int
> before reenabling.

Yeah, but in-between someone could sneak in and enable the display error
interrupt (since modeset doesn't block it any more), but while the
interrupt is still running. I.e.

CPU 0			CPU 1
			disable DERR due to modeset

start interrupt handler,
check that DERRR is off,
do nothing

			reanable DERR due to modeset done

-> interrupt handler still running, but DERR is enabled

end interrupt handler

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

Re: [PATCH 6/9] drm/i915: don't disable/reenable IVB error interrupts when not needed

[Paulo Zanoni]

2013/8/20 Daniel Vetter <daniel@ffwll.ch>:
> On Tue, Aug 20, 2013 at 11:43:46AM -0300, Paulo Zanoni wrote:
>> 2013/8/20 Daniel Vetter <daniel@ffwll.ch>:
>> > On Tue, Aug 06, 2013 at 06:57:16PM -0300, Paulo Zanoni wrote:
>> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >>
>> >> If the error interrupts are already disabled, don't disable and
>> >> reenable them. This is going to be needed when we're in PC8+, where
>> >> all the interrupts are disabled so we won't risk re-enabling
>> >> DE_ERR_INT_IVB.
>> >>
>> >> v2: Use dev_priv->irq_mask (Chris)
>> >>
>> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/i915_irq.c | 7 +++++--
>> >>  1 file changed, 5 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> >> index d96bd1b..5e7e6f6 100644
>> >> --- a/drivers/gpu/drm/i915/i915_irq.c
>> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> >> @@ -1373,6 +1373,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>> >>       drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>> >>       u32 de_iir, gt_iir, de_ier, sde_ier = 0;
>> >>       irqreturn_t ret = IRQ_NONE;
>> >> +     bool err_int_reenable = false;
>> >>
>> >>       atomic_inc(&dev_priv->irq_received);
>> >>
>> >> @@ -1401,7 +1402,9 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>> >>        * handler. */
>> >>       if (IS_HASWELL(dev)) {
>> >>               spin_lock(&dev_priv->irq_lock);
>> >> -             ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
>> >> +             err_int_reenable = ~dev_priv->irq_mask & DE_ERR_INT_IVB;
>> >> +             if (err_int_reenable)
>> >> +                     ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
>> >
>> > Hm, that reminds me that this entire logic here is racy wrt concurrent
>> > interrupt enabling on a different cpu core (e.g. due to a modeset now
>> > again allowing display error interrupts). Do we still need this or could
>> > we just ditch this entire complexity?
>>
>> Can you please explain more? We still check ivb_can_enable_err_int
>> before reenabling.
>
> Yeah, but in-between someone could sneak in and enable the display error
> interrupt (since modeset doesn't block it any more), but while the
> interrupt is still running. I.e.
>
> CPU 0                   CPU 1
>                         disable DERR due to modeset
>
> start interrupt handler,
> check that DERRR is off,
> do nothing
>
>                         reanable DERR due to modeset done
>
> -> interrupt handler still running, but DERR is enabled
>
> end interrupt handler

Oh, I see. The reason we disable ERR_INT at the irq handler is only
because if we ever hit "unclaimed register"s during interrupts we
won't keep getting new interrupts all over again. So your concerns are
valid, but this all is just a problem we find a way to trigger ERR_INT
interrupts from inside the irq handler.

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



-- 
Paulo Zanoni

[PATCH 7/9] drm/i915: allow package C8+ states on Haswell (disabled)

[Paulo Zanoni]

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

This patch allows PC8+ states on Haswell. These states can only be
reached when all the display outputs are disabled, and they allow some
more power savings.

The fact that the graphics device is allowing PC8+ doesn't mean that
the machine will actually enter PC8+: all the other devices also need
to allow PC8+.

For now this option is disabled by default. You need i915.allow_pc8=1
if you want it.

This patch adds a big comment inside i915_drv.h explaining how it
works and how it tracks things. Read it.

v2: (this is not really v2, many previous versions were already sent,
     but they had different names)
    - Use the new functions to enable/disable GTIMR and GEN6_PMIMR
    - Rename almost all variables and functions to names suggested by
      Chris
    - More WARNs on the IRQ handling code
    - Also disable PC8 when there's GPU work to do (thanks to Ben for
      the help on this), so apps can run caster
    - Enable PC8 on a delayed work function that is delayed for 5
      seconds. This makes sure we only enable PC8+ if we're really
      idle
    - Make sure we're not in PC8+ when suspending

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c         |  10 ++
 drivers/gpu/drm/i915/i915_drv.c         |  11 +++
 drivers/gpu/drm/i915/i915_drv.h         |  72 ++++++++++++++
 drivers/gpu/drm/i915/i915_irq.c         | 106 +++++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c    | 164 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp.c         |   3 +
 drivers/gpu/drm/i915/intel_drv.h        |   8 ++
 drivers/gpu/drm/i915/intel_i2c.c        |   2 +
 drivers/gpu/drm/i915/intel_pm.c         |  11 +++
 drivers/gpu/drm/i915/intel_ringbuffer.c |   2 +
 10 files changed, 389 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index e4e98df..7721bdb 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1483,6 +1483,14 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	mutex_init(&dev_priv->rps.hw_lock);
 	mutex_init(&dev_priv->modeset_restore_lock);
 
+	mutex_init(&dev_priv->pc8.lock);
+	dev_priv->pc8.requirements_met = false;
+	dev_priv->pc8.gpu_idle = false;
+	dev_priv->pc8.irqs_disabled = false;
+	dev_priv->pc8.enabled = false;
+	dev_priv->pc8.disable_count = 2; /* requirements_met + gpu_idle */
+	INIT_DELAYED_WORK(&dev_priv->pc8.enable_work, hsw_enable_pc8_work);
+
 	i915_dump_device_info(dev_priv);
 
 	if (i915_get_bridge_dev(dev)) {
@@ -1724,6 +1732,8 @@ int i915_driver_unload(struct drm_device *dev)
 	cancel_work_sync(&dev_priv->gpu_error.work);
 	i915_destroy_error_state(dev);
 
+	cancel_delayed_work_sync(&dev_priv->pc8.enable_work);
+
 	if (dev->pdev->msi_enabled)
 		pci_disable_msi(dev->pdev);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 01d63a0..1e53002 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -141,6 +141,10 @@ module_param_named(fastboot, i915_fastboot, bool, 0600);
 MODULE_PARM_DESC(fastboot, "Try to skip unnecessary mode sets at boot time "
 		 "(default: false)");
 
+int i915_enable_pc8 __read_mostly = 0;
+module_param_named(enable_pc8, i915_enable_pc8, int, 0600);
+MODULE_PARM_DESC(enable_pc8, "Enable support for low power package C states (PC8+) (default: false)");
+
 bool i915_prefault_disable __read_mostly;
 module_param_named(prefault_disable, i915_prefault_disable, bool, 0600);
 MODULE_PARM_DESC(prefault_disable,
@@ -557,6 +561,9 @@ static int i915_drm_freeze(struct drm_device *dev)
 	dev_priv->modeset_restore = MODESET_SUSPENDED;
 	mutex_unlock(&dev_priv->modeset_restore_lock);
 
+	/* We do a lot of poking in a lot of registers, make sure they work
+	 * properly. */
+	hsw_disable_package_c8(dev_priv);
 	intel_set_power_well(dev, true);
 
 	drm_kms_helper_poll_disable(dev);
@@ -713,6 +720,10 @@ static int __i915_drm_thaw(struct drm_device *dev)
 		schedule_work(&dev_priv->console_resume_work);
 	}
 
+	/* Undo what we did at i915_drm_freeze so the refcount goes back to the
+	 * expected level. */
+	hsw_enable_package_c8(dev_priv);
+
 	mutex_lock(&dev_priv->modeset_restore_lock);
 	dev_priv->modeset_restore = MODESET_DONE;
 	mutex_unlock(&dev_priv->modeset_restore_lock);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b621f5e..037dd19 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1050,6 +1050,75 @@ struct intel_vbt_data {
 	struct child_device_config *child_dev;
 };
 
+/*
+ * This struct tracks the state needed for the Package C8+ feature.
+ *
+ * Package states C8 and deeper are really deep PC states that can only be
+ * reached when all the devices on the system allow it, so even if the graphics
+ * device allows PC8+, it doesn't mean the system will actually get to these
+ * states.
+ *
+ * Our driver only allows PC8+ when all the outputs are disabled, the power well
+ * is disabled and the GPU is idle. When these conditions are met, we manually
+ * do the other conditions: disable the interrupts, clocks and switch LCPLL
+ * refclk to Fclk.
+ *
+ * When we really reach PC8 or deeper states (not just when we allow it) we lose
+ * the state of some registers, so when we come back from PC8+ we need to
+ * restore this state. We don't get into PC8+ if we're not in RC6, so we don't
+ * need to take care of the registers kept by RC6.
+ *
+ * The interrupt disabling is part of the requirements. We can only leave the
+ * PCH HPD interrupts enabled. If we're in PC8+ and we get another interrupt we
+ * can lock the machine.
+ *
+ * Ideally every piece of our code that needs PC8+ disabled would call
+ * hsw_disable_package_c8, which would increment disable_count and prevent the
+ * system from reaching PC8+. But we don't have a symmetric way to do this for
+ * everything, so we have the requirements_met and gpu_idle variables. When we
+ * switch requirements_met or gpu_idle to true we decrease disable_count, and
+ * increase it in the opposite case. The requirements_met variable is true when
+ * all the CRTCs, encoders and the power well are disabled. The gpu_idle
+ * variable is true when the GPU is idle.
+ *
+ * In addition to everything, we only actually enable PC8+ if disable_count
+ * stays at zero for at least some seconds. This is implemented with the
+ * enable_work variable. We do this so we don't enable/disable PC8 dozens of
+ * consecutive times when all screens are disabled and some background app
+ * queries the state of our connectors, or we have some application constantly
+ * waking up to use the GPU. Only after the enable_work function actually
+ * enables PC8+ the "enable" variable will become true, which means that it can
+ * be false even if disable_count is 0.
+ *
+ * The irqs_disabled variable becomes true exactly after we disable the IRQs and
+ * goes back to false exactly before we reenable the IRQs. We use this variable
+ * to check if someone is trying to enable/disable IRQs while they're supposed
+ * to be disabled. This shouldn't happen and we'll print some error messages in
+ * case it happens, but if it actually happens we'll also update the variables
+ * inside struct regsave so when we restore the IRQs they will contain the
+ * latest expected values.
+ *
+ * For more, read "Display Sequences for Package C8" on our documentation.
+ */
+struct i915_package_c8 {
+	bool requirements_met;
+	bool gpu_idle;
+	bool irqs_disabled;
+	/* Only true after the delayed work task actually enables it. */
+	bool enabled;
+	int disable_count;
+	struct mutex lock;
+	struct delayed_work enable_work;
+
+	struct {
+		uint32_t deimr;
+		uint32_t sdeimr;
+		uint32_t gtimr;
+		uint32_t gtier;
+		uint32_t gen6_pmimr;
+	} regsave;
+};
+
 typedef struct drm_i915_private {
 	struct drm_device *dev;
 	struct kmem_cache *slab;
@@ -1234,6 +1303,8 @@ typedef struct drm_i915_private {
 		uint16_t cur_latency[5];
 	} wm;
 
+	struct i915_package_c8 pc8;
+
 	/* Old dri1 support infrastructure, beware the dragons ya fools entering
 	 * here! */
 	struct i915_dri1_state dri1;
@@ -1610,6 +1681,7 @@ extern unsigned int i915_preliminary_hw_support __read_mostly;
 extern int i915_disable_power_well __read_mostly;
 extern int i915_enable_ips __read_mostly;
 extern bool i915_fastboot __read_mostly;
+extern int i915_enable_pc8 __read_mostly;
 extern bool i915_prefault_disable __read_mostly;
 
 extern int i915_suspend(struct drm_device *dev, pm_message_t state);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 5e7e6f6..7aa11f3 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -85,6 +85,12 @@ ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
 {
 	assert_spin_locked(&dev_priv->irq_lock);
 
+	if (dev_priv->pc8.irqs_disabled) {
+		DRM_ERROR("IRQs disabled\n");
+		dev_priv->pc8.regsave.deimr &= ~mask;
+		return;
+	}
+
 	if ((dev_priv->irq_mask & mask) != 0) {
 		dev_priv->irq_mask &= ~mask;
 		I915_WRITE(DEIMR, dev_priv->irq_mask);
@@ -97,6 +103,12 @@ ironlake_disable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
 {
 	assert_spin_locked(&dev_priv->irq_lock);
 
+	if (dev_priv->pc8.irqs_disabled) {
+		DRM_ERROR("IRQs disabled\n");
+		dev_priv->pc8.regsave.deimr |= mask;
+		return;
+	}
+
 	if ((dev_priv->irq_mask & mask) != mask) {
 		dev_priv->irq_mask |= mask;
 		I915_WRITE(DEIMR, dev_priv->irq_mask);
@@ -116,6 +128,14 @@ static void ilk_update_gt_irq(struct drm_i915_private *dev_priv,
 {
 	assert_spin_locked(&dev_priv->irq_lock);
 
+	if (dev_priv->pc8.irqs_disabled) {
+		DRM_ERROR("IRQs disabled\n");
+		dev_priv->pc8.regsave.gtimr &= ~interrupt_mask;
+		dev_priv->pc8.regsave.gtimr |= (~enabled_irq_mask &
+						interrupt_mask);
+		return;
+	}
+
 	dev_priv->gt_irq_mask &= ~interrupt_mask;
 	dev_priv->gt_irq_mask |= (~enabled_irq_mask & interrupt_mask);
 	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
@@ -146,6 +166,14 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
+	if (dev_priv->pc8.irqs_disabled) {
+		DRM_ERROR("IRQs disabled\n");
+		dev_priv->pc8.regsave.gen6_pmimr &= ~interrupt_mask;
+		dev_priv->pc8.regsave.gen6_pmimr |= (~enabled_irq_mask &
+						     interrupt_mask);
+		return;
+	}
+
 	new_val = dev_priv->pm_irq_mask;
 	new_val &= ~interrupt_mask;
 	new_val |= (~enabled_irq_mask & interrupt_mask);
@@ -262,6 +290,15 @@ static void ibx_display_interrupt_update(struct drm_i915_private *dev_priv,
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
+	if (dev_priv->pc8.irqs_disabled &&
+	    (interrupt_mask & SDE_HOTPLUG_MASK_CPT)) {
+		DRM_ERROR("IRQs disabled\n");
+		dev_priv->pc8.regsave.sdeimr &= ~interrupt_mask;
+		dev_priv->pc8.regsave.sdeimr |= (~enabled_irq_mask &
+						 interrupt_mask);
+		return;
+	}
+
 	I915_WRITE(SDEIMR, sdeimr);
 	POSTING_READ(SDEIMR);
 }
@@ -3149,3 +3186,72 @@ void intel_hpd_init(struct drm_device *dev)
 		dev_priv->display.hpd_irq_setup(dev);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
+
+/* Disable interrupts so we can allow Package C8+. */
+void hsw_pc8_disable_interrupts(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	unsigned long irqflags;
+
+	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+
+	dev_priv->pc8.regsave.deimr = I915_READ(DEIMR);
+	dev_priv->pc8.regsave.sdeimr = I915_READ(SDEIMR);
+	dev_priv->pc8.regsave.gtimr = I915_READ(GTIMR);
+	dev_priv->pc8.regsave.gtier = I915_READ(GTIER);
+	dev_priv->pc8.regsave.gen6_pmimr = I915_READ(GEN6_PMIMR);
+
+	ironlake_disable_display_irq(dev_priv, ~DE_PCH_EVENT_IVB);
+	ibx_disable_display_interrupt(dev_priv, ~SDE_HOTPLUG_MASK_CPT);
+	ilk_disable_gt_irq(dev_priv, 0xffffffff);
+	snb_disable_pm_irq(dev_priv, 0xffffffff);
+
+	dev_priv->pc8.irqs_disabled = true;
+
+	WARN(I915_READ(DEIIR), "DEIIR is not 0\n");
+	WARN(I915_READ(SDEIIR), "SDEIIR is not 0\n");
+	WARN(I915_READ(GTIIR), "GTIIR is not 0\n");
+	WARN(I915_READ(GEN6_PMIIR), "GEN6_PMIIR is not 0\n");
+
+	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+}
+
+/* Restore interrupts so we can recover from Package C8+. */
+void hsw_pc8_restore_interrupts(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	unsigned long irqflags;
+	uint32_t val, expected;
+
+	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+
+	val = I915_READ(DEIMR);
+	expected = ~DE_PCH_EVENT_IVB;
+	WARN(val != expected, "DEIMR is 0x%08x, not 0x%08x\n", val, expected);
+
+	val = I915_READ(SDEIMR) & ~SDE_HOTPLUG_MASK_CPT;
+	expected = ~SDE_HOTPLUG_MASK_CPT;
+	WARN(val != expected, "SDEIMR non-HPD bits are 0x%08x, not 0x%08x\n",
+	     val, expected);
+
+	val = I915_READ(GTIMR);
+	expected = 0xffffffff;
+	WARN(val != expected, "GTIMR is 0x%08x, not 0x%08x\n", val, expected);
+
+	val = I915_READ(GEN6_PMIMR);
+	expected = 0xffffffff;
+	WARN(val != expected, "GEN6_PMIMR is 0x%08x, not 0x%08x\n", val,
+	     expected);
+
+	dev_priv->pc8.irqs_disabled = false;
+
+	ironlake_enable_display_irq(dev_priv, ~dev_priv->pc8.regsave.deimr);
+	ibx_enable_display_interrupt(dev_priv,
+				     ~dev_priv->pc8.regsave.sdeimr &
+				     ~SDE_HOTPLUG_MASK_CPT);
+	ilk_enable_gt_irq(dev_priv, dev_priv->pc8.regsave.gtimr);
+	snb_enable_pm_irq(dev_priv, dev_priv->pc8.regsave.gen6_pmimr);
+	I915_WRITE(GTIER, dev_priv->pc8.regsave.gtier);
+
+	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a0914db..44871fa 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6070,6 +6070,165 @@ void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 	}
 }
 
+void hsw_enable_pc8_work(struct work_struct *__work)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(to_delayed_work(__work), struct drm_i915_private,
+			     pc8.enable_work);
+	struct drm_device *dev = dev_priv->dev;
+	uint32_t val;
+
+	if (dev_priv->pc8.enabled)
+		return;
+
+	DRM_DEBUG_KMS("Enabling package C8+\n");
+
+	dev_priv->pc8.enabled = true;
+
+	if (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
+		val = I915_READ(SOUTH_DSPCLK_GATE_D);
+		val &= ~PCH_LP_PARTITION_LEVEL_DISABLE;
+		I915_WRITE(SOUTH_DSPCLK_GATE_D, val);
+	}
+
+	cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
+	lpt_disable_clkout_dp(dev);
+	hsw_pc8_disable_interrupts(dev);
+	hsw_disable_lcpll(dev_priv, true, true);
+}
+
+static void __hsw_enable_package_c8(struct drm_i915_private *dev_priv)
+{
+	WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
+	WARN(dev_priv->pc8.disable_count < 1,
+	     "pc8.disable_count: %d\n", dev_priv->pc8.disable_count);
+
+	dev_priv->pc8.disable_count--;
+	if (dev_priv->pc8.disable_count != 0)
+		return;
+
+	schedule_delayed_work(&dev_priv->pc8.enable_work,
+			      msecs_to_jiffies(5 * 1000));
+}
+
+static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+	uint32_t val;
+
+	WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
+	WARN(dev_priv->pc8.disable_count < 0,
+	     "pc8.disable_count: %d\n", dev_priv->pc8.disable_count);
+
+	dev_priv->pc8.disable_count++;
+	if (dev_priv->pc8.disable_count != 1)
+		return;
+
+	cancel_delayed_work_sync(&dev_priv->pc8.enable_work);
+	if (!dev_priv->pc8.enabled)
+		return;
+
+	DRM_DEBUG_KMS("Disabling package C8+\n");
+
+	hsw_restore_lcpll(dev_priv);
+	hsw_pc8_restore_interrupts(dev);
+	lpt_init_pch_refclk(dev);
+
+	if (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
+		val = I915_READ(SOUTH_DSPCLK_GATE_D);
+		val |= PCH_LP_PARTITION_LEVEL_DISABLE;
+		I915_WRITE(SOUTH_DSPCLK_GATE_D, val);
+	}
+
+	intel_prepare_ddi(dev);
+	i915_gem_init_swizzling(dev);
+	intel_enable_gt_powersave(dev);
+	dev_priv->pc8.enabled = false;
+}
+
+void hsw_enable_package_c8(struct drm_i915_private *dev_priv)
+{
+	mutex_lock(&dev_priv->pc8.lock);
+	__hsw_enable_package_c8(dev_priv);
+	mutex_unlock(&dev_priv->pc8.lock);
+}
+
+void hsw_disable_package_c8(struct drm_i915_private *dev_priv)
+{
+	mutex_lock(&dev_priv->pc8.lock);
+	__hsw_disable_package_c8(dev_priv);
+	mutex_unlock(&dev_priv->pc8.lock);
+}
+
+static bool hsw_can_enable_package_c8(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+	struct intel_crtc *crtc;
+	uint32_t val;
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head)
+		if (crtc->base.enabled)
+			return false;
+
+	/* This case is still possible since we have the i915.disable_power_well
+	 * parameter and also the KVMr or something else might be requesting the
+	 * power well. */
+	val = I915_READ(HSW_PWR_WELL_DRIVER);
+	if (val != 0) {
+		DRM_DEBUG_KMS("Not enabling PC8: power well on\n");
+		return false;
+	}
+
+	return true;
+}
+
+/* Since we're called from modeset_global_resources there's no way to
+ * symmetrically increase and decrease the refcount, so we use
+ * dev_priv->pc8.requirements_met to track whether we already have the refcount
+ * or not.
+ */
+static void hsw_update_package_c8(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	bool allow;
+
+	if (!i915_enable_pc8)
+		return;
+
+	mutex_lock(&dev_priv->pc8.lock);
+
+	allow = hsw_can_enable_package_c8(dev_priv);
+
+	if (allow == dev_priv->pc8.requirements_met)
+		goto done;
+
+	dev_priv->pc8.requirements_met = allow;
+
+	if (allow)
+		__hsw_enable_package_c8(dev_priv);
+	else
+		__hsw_disable_package_c8(dev_priv);
+
+done:
+	mutex_unlock(&dev_priv->pc8.lock);
+}
+
+static void hsw_package_c8_gpu_idle(struct drm_i915_private *dev_priv)
+{
+	if (!dev_priv->pc8.gpu_idle) {
+		dev_priv->pc8.gpu_idle = true;
+		hsw_enable_package_c8(dev_priv);
+	}
+}
+
+void hsw_package_c8_gpu_busy(struct drm_i915_private *dev_priv)
+{
+	if (dev_priv->pc8.gpu_idle) {
+		dev_priv->pc8.gpu_idle = false;
+		hsw_disable_package_c8(dev_priv);
+	}
+}
+
 static void haswell_modeset_global_resources(struct drm_device *dev)
 {
 	bool enable = false;
@@ -6085,6 +6244,8 @@ static void haswell_modeset_global_resources(struct drm_device *dev)
 	}
 
 	intel_set_power_well(dev, enable);
+
+	hsw_update_package_c8(dev);
 }
 
 static int haswell_crtc_mode_set(struct drm_crtc *crtc,
@@ -7321,8 +7482,11 @@ void intel_mark_busy(struct drm_device *dev)
 
 void intel_mark_idle(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
 
+	hsw_package_c8_gpu_idle(dev_priv);
+
 	if (!i915_powersave)
 		return;
 
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 63b6722d..b150d23 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -344,6 +344,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 	else
 		precharge = 5;
 
+	intel_aux_display_runtime_get(dev_priv);
+
 	/* Try to wait for any previous AUX channel activity */
 	for (try = 0; try < 3; try++) {
 		status = I915_READ_NOTRACE(ch_ctl);
@@ -434,6 +436,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 	ret = recv_bytes;
 out:
 	pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
+	intel_aux_display_runtime_put(dev_priv);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index db7cbd5..796e2de 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -844,5 +844,13 @@ extern void ilk_disable_gt_irq(struct drm_i915_private *dev_priv,
 extern void snb_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 extern void snb_disable_pm_irq(struct drm_i915_private *dev_priv,
 			       uint32_t mask);
+extern void hsw_enable_pc8_work(struct work_struct *__work);
+extern void hsw_enable_package_c8(struct drm_i915_private *dev_priv);
+extern void hsw_disable_package_c8(struct drm_i915_private *dev_priv);
+extern void hsw_pc8_disable_interrupts(struct drm_device *dev);
+extern void hsw_pc8_restore_interrupts(struct drm_device *dev);
+extern void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv);
+extern void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv);
+extern void hsw_package_c8_gpu_busy(struct drm_i915_private *dev_priv);
 
 #endif /* __INTEL_DRV_H__ */
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 639fe19..d1c1e0f7 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -398,6 +398,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
 	int i, reg_offset;
 	int ret = 0;
 
+	intel_aux_display_runtime_get(dev_priv);
 	mutex_lock(&dev_priv->gmbus_mutex);
 
 	if (bus->force_bit) {
@@ -497,6 +498,7 @@ timeout:
 
 out:
 	mutex_unlock(&dev_priv->gmbus_mutex);
+	intel_aux_display_runtime_put(dev_priv);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 984250d..b4563f8 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5303,6 +5303,17 @@ void intel_init_power_well(struct drm_device *dev)
 		I915_WRITE(HSW_PWR_WELL_BIOS, 0);
 }
 
+/* Disables PC8 so we can use the GMBUS and DP AUX interrupts. */
+void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv)
+{
+	hsw_disable_package_c8(dev_priv);
+}
+
+void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv)
+{
+	hsw_enable_package_c8(dev_priv);
+}
+
 /* Set up chip specific power management-related functions */
 void intel_init_pm(struct drm_device *dev)
 {
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 2ef4335..7f6ec9e 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1590,6 +1590,8 @@ void intel_ring_advance(struct intel_ring_buffer *ring)
 {
 	struct drm_i915_private *dev_priv = ring->dev->dev_private;
 
+	hsw_package_c8_gpu_busy(dev_priv);
+
 	ring->tail &= ring->size - 1;
 	if (dev_priv->gpu_error.stop_rings & intel_ring_flag(ring))
 		return;
-- 
1.8.1.2

Re: [PATCH 7/9] drm/i915: allow package C8+ states on Haswell (disabled)

[Chris Wilson]

On Tue, Aug 06, 2013 at 06:57:17PM -0300, Paulo Zanoni wrote:
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 2ef4335..7f6ec9e 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1590,6 +1590,8 @@ void intel_ring_advance(struct intel_ring_buffer *ring)
>  {
>  	struct drm_i915_private *dev_priv = ring->dev->dev_private;
>  
> +	hsw_package_c8_gpu_busy(dev_priv);
> +

Ahem, what is this doing here? If only we had something that was the
opposite of intel_mark_idle... At this point, I am going to get some
sleep...

Overall it looks much, much better. I am still uneasy about the
interrupt race whilst disabling them, so I need to think some more about
that. The new DRM_ERRORs should be WARN and a few other minor niggles. I
really do like the new names though, thanks.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 7/9] drm/i915: allow package C8+ states on Haswell (disabled)

[Paulo Zanoni]

2013/8/6 Chris Wilson <chris@chris-wilson.co.uk>:
> On Tue, Aug 06, 2013 at 06:57:17PM -0300, Paulo Zanoni wrote:
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> index 2ef4335..7f6ec9e 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> @@ -1590,6 +1590,8 @@ void intel_ring_advance(struct intel_ring_buffer *ring)
>>  {
>>       struct drm_i915_private *dev_priv = ring->dev->dev_private;
>>
>> +     hsw_package_c8_gpu_busy(dev_priv);
>> +
>
> Ahem, what is this doing here? If only we had something that was the
> opposite of intel_mark_idle... At this point, I am going to get some
> sleep...

You're suggesting me to use intel_mark_busy? I have to admit I saw it,
but I noticed intel_mark_busy is called after intel_ring_advance, and
I was trying to follow what Ben suggested me. If you're suggesting
this, I guess it's ok, so I will test this possibility.

>
> Overall it looks much, much better. I am still uneasy about the
> interrupt race whilst disabling them, so I need to think some more about
> that. The new DRM_ERRORs should be WARN and a few other minor niggles. I
> really do like the new names though, thanks.

Thanks for the reviews! I will change the errors to WARNs.

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



-- 
Paulo Zanoni

Re: [PATCH 7/9] drm/i915: allow package C8+ states on Haswell (disabled)

[Chris Wilson]

On Wed, Aug 07, 2013 at 10:38:19AM -0300, Paulo Zanoni wrote:
> 2013/8/6 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Tue, Aug 06, 2013 at 06:57:17PM -0300, Paulo Zanoni wrote:
> >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> index 2ef4335..7f6ec9e 100644
> >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> @@ -1590,6 +1590,8 @@ void intel_ring_advance(struct intel_ring_buffer *ring)
> >>  {
> >>       struct drm_i915_private *dev_priv = ring->dev->dev_private;
> >>
> >> +     hsw_package_c8_gpu_busy(dev_priv);
> >> +
> >
> > Ahem, what is this doing here? If only we had something that was the
> > opposite of intel_mark_idle... At this point, I am going to get some
> > sleep...
> 
> You're suggesting me to use intel_mark_busy? I have to admit I saw it,
> but I noticed intel_mark_busy is called after intel_ring_advance, and
> I was trying to follow what Ben suggested me. If you're suggesting
> this, I guess it's ok, so I will test this possibility.

intel_mark_busy() is where we note the transition in userspace activity,
it should be where we put all the little hooks and pm tweaks required.
If you however need the pc8 disable earlier than the ring register write,
we should do it in intel_ring_begin(). Ultimately, though we should be
able to squash it into a single operation such that we never submit a
command before intel_mark_busy(). Hmm, easier in fact just to move
intel_mark_busy() to intel_ring_begin() and I think everybody is happy.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 7/9] drm/i915: allow package C8+ states on Haswell (disabled)

[Daniel Vetter]

On Wed, Aug 07, 2013 at 03:20:09PM +0100, Chris Wilson wrote:
> On Wed, Aug 07, 2013 at 10:38:19AM -0300, Paulo Zanoni wrote:
> > 2013/8/6 Chris Wilson <chris@chris-wilson.co.uk>:
> > > On Tue, Aug 06, 2013 at 06:57:17PM -0300, Paulo Zanoni wrote:
> > >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > >> index 2ef4335..7f6ec9e 100644
> > >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > >> @@ -1590,6 +1590,8 @@ void intel_ring_advance(struct intel_ring_buffer *ring)
> > >>  {
> > >>       struct drm_i915_private *dev_priv = ring->dev->dev_private;
> > >>
> > >> +     hsw_package_c8_gpu_busy(dev_priv);
> > >> +
> > >
> > > Ahem, what is this doing here? If only we had something that was the
> > > opposite of intel_mark_idle... At this point, I am going to get some
> > > sleep...
> > 
> > You're suggesting me to use intel_mark_busy? I have to admit I saw it,
> > but I noticed intel_mark_busy is called after intel_ring_advance, and
> > I was trying to follow what Ben suggested me. If you're suggesting
> > this, I guess it's ok, so I will test this possibility.
> 
> intel_mark_busy() is where we note the transition in userspace activity,
> it should be where we put all the little hooks and pm tweaks required.
> If you however need the pc8 disable earlier than the ring register write,
> we should do it in intel_ring_begin(). Ultimately, though we should be
> able to squash it into a single operation such that we never submit a
> command before intel_mark_busy(). Hmm, easier in fact just to move
> intel_mark_busy() to intel_ring_begin() and I think everybody is happy.

We have oddball places where we add stuff to the ring but not a request,
so we'd miss the eventual mark_idle. But since this is only to make sure
we have working interrupts the current place of mark_busy should be good
enough.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 7/9] drm/i915: allow package C8+ states on Haswell (disabled)

[Paulo Zanoni]

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

This patch allows PC8+ states on Haswell. These states can only be
reached when all the display outputs are disabled, and they allow some
more power savings.

The fact that the graphics device is allowing PC8+ doesn't mean that
the machine will actually enter PC8+: all the other devices also need
to allow PC8+.

For now this option is disabled by default. You need i915.allow_pc8=1
if you want it.

This patch adds a big comment inside i915_drv.h explaining how it
works and how it tracks things. Read it.

v2: (this is not really v2, many previous versions were already sent,
     but they had different names)
    - Use the new functions to enable/disable GTIMR and GEN6_PMIMR
    - Rename almost all variables and functions to names suggested by
      Chris
    - More WARNs on the IRQ handling code
    - Also disable PC8 when there's GPU work to do (thanks to Ben for
      the help on this), so apps can run caster
    - Enable PC8 on a delayed work function that is delayed for 5
      seconds. This makes sure we only enable PC8+ if we're really
      idle
    - Make sure we're not in PC8+ when suspending
v3: - WARN if IRQs are disabled on __wait_seqno
    - Replace some DRM_ERRORs with WARNs
    - Fix calls to restore GT and PM interrupts
    - Use intel_mark_busy instead of intel_ring_advance to disable PC8
v4: - Use the force_wake, Luke!

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c      |  10 ++
 drivers/gpu/drm/i915/i915_drv.c      |  11 +++
 drivers/gpu/drm/i915/i915_drv.h      |  72 +++++++++++++++
 drivers/gpu/drm/i915/i915_gem.c      |   2 +
 drivers/gpu/drm/i915/i915_irq.c      | 106 +++++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c | 174 ++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_dp.c      |   3 +
 drivers/gpu/drm/i915/intel_drv.h     |   7 ++
 drivers/gpu/drm/i915/intel_i2c.c     |   2 +
 drivers/gpu/drm/i915/intel_pm.c      |  11 +++
 10 files changed, 397 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 0adfe40..9fc22f9 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1483,6 +1483,14 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	mutex_init(&dev_priv->rps.hw_lock);
 	mutex_init(&dev_priv->modeset_restore_lock);
 
+	mutex_init(&dev_priv->pc8.lock);
+	dev_priv->pc8.requirements_met = false;
+	dev_priv->pc8.gpu_idle = false;
+	dev_priv->pc8.irqs_disabled = false;
+	dev_priv->pc8.enabled = false;
+	dev_priv->pc8.disable_count = 2; /* requirements_met + gpu_idle */
+	INIT_DELAYED_WORK(&dev_priv->pc8.enable_work, hsw_enable_pc8_work);
+
 	i915_dump_device_info(dev_priv);
 
 	if (i915_get_bridge_dev(dev)) {
@@ -1724,6 +1732,8 @@ int i915_driver_unload(struct drm_device *dev)
 	cancel_work_sync(&dev_priv->gpu_error.work);
 	i915_destroy_error_state(dev);
 
+	cancel_delayed_work_sync(&dev_priv->pc8.enable_work);
+
 	if (dev->pdev->msi_enabled)
 		pci_disable_msi(dev->pdev);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 13457e3e..6169c92 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -141,6 +141,10 @@ module_param_named(fastboot, i915_fastboot, bool, 0600);
 MODULE_PARM_DESC(fastboot, "Try to skip unnecessary mode sets at boot time "
 		 "(default: false)");
 
+int i915_enable_pc8 __read_mostly = 0;
+module_param_named(enable_pc8, i915_enable_pc8, int, 0600);
+MODULE_PARM_DESC(enable_pc8, "Enable support for low power package C states (PC8+) (default: false)");
+
 bool i915_prefault_disable __read_mostly;
 module_param_named(prefault_disable, i915_prefault_disable, bool, 0600);
 MODULE_PARM_DESC(prefault_disable,
@@ -557,6 +561,9 @@ static int i915_drm_freeze(struct drm_device *dev)
 	dev_priv->modeset_restore = MODESET_SUSPENDED;
 	mutex_unlock(&dev_priv->modeset_restore_lock);
 
+	/* We do a lot of poking in a lot of registers, make sure they work
+	 * properly. */
+	hsw_disable_package_c8(dev_priv);
 	intel_set_power_well(dev, true);
 
 	drm_kms_helper_poll_disable(dev);
@@ -713,6 +720,10 @@ static int __i915_drm_thaw(struct drm_device *dev)
 		schedule_work(&dev_priv->console_resume_work);
 	}
 
+	/* Undo what we did at i915_drm_freeze so the refcount goes back to the
+	 * expected level. */
+	hsw_enable_package_c8(dev_priv);
+
 	mutex_lock(&dev_priv->modeset_restore_lock);
 	dev_priv->modeset_restore = MODESET_DONE;
 	mutex_unlock(&dev_priv->modeset_restore_lock);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2c7c098..fbcb0ca 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1071,6 +1071,75 @@ struct intel_wm_level {
 	uint32_t fbc_val;
 };
 
+/*
+ * This struct tracks the state needed for the Package C8+ feature.
+ *
+ * Package states C8 and deeper are really deep PC states that can only be
+ * reached when all the devices on the system allow it, so even if the graphics
+ * device allows PC8+, it doesn't mean the system will actually get to these
+ * states.
+ *
+ * Our driver only allows PC8+ when all the outputs are disabled, the power well
+ * is disabled and the GPU is idle. When these conditions are met, we manually
+ * do the other conditions: disable the interrupts, clocks and switch LCPLL
+ * refclk to Fclk.
+ *
+ * When we really reach PC8 or deeper states (not just when we allow it) we lose
+ * the state of some registers, so when we come back from PC8+ we need to
+ * restore this state. We don't get into PC8+ if we're not in RC6, so we don't
+ * need to take care of the registers kept by RC6.
+ *
+ * The interrupt disabling is part of the requirements. We can only leave the
+ * PCH HPD interrupts enabled. If we're in PC8+ and we get another interrupt we
+ * can lock the machine.
+ *
+ * Ideally every piece of our code that needs PC8+ disabled would call
+ * hsw_disable_package_c8, which would increment disable_count and prevent the
+ * system from reaching PC8+. But we don't have a symmetric way to do this for
+ * everything, so we have the requirements_met and gpu_idle variables. When we
+ * switch requirements_met or gpu_idle to true we decrease disable_count, and
+ * increase it in the opposite case. The requirements_met variable is true when
+ * all the CRTCs, encoders and the power well are disabled. The gpu_idle
+ * variable is true when the GPU is idle.
+ *
+ * In addition to everything, we only actually enable PC8+ if disable_count
+ * stays at zero for at least some seconds. This is implemented with the
+ * enable_work variable. We do this so we don't enable/disable PC8 dozens of
+ * consecutive times when all screens are disabled and some background app
+ * queries the state of our connectors, or we have some application constantly
+ * waking up to use the GPU. Only after the enable_work function actually
+ * enables PC8+ the "enable" variable will become true, which means that it can
+ * be false even if disable_count is 0.
+ *
+ * The irqs_disabled variable becomes true exactly after we disable the IRQs and
+ * goes back to false exactly before we reenable the IRQs. We use this variable
+ * to check if someone is trying to enable/disable IRQs while they're supposed
+ * to be disabled. This shouldn't happen and we'll print some error messages in
+ * case it happens, but if it actually happens we'll also update the variables
+ * inside struct regsave so when we restore the IRQs they will contain the
+ * latest expected values.
+ *
+ * For more, read "Display Sequences for Package C8" on our documentation.
+ */
+struct i915_package_c8 {
+	bool requirements_met;
+	bool gpu_idle;
+	bool irqs_disabled;
+	/* Only true after the delayed work task actually enables it. */
+	bool enabled;
+	int disable_count;
+	struct mutex lock;
+	struct delayed_work enable_work;
+
+	struct {
+		uint32_t deimr;
+		uint32_t sdeimr;
+		uint32_t gtimr;
+		uint32_t gtier;
+		uint32_t gen6_pmimr;
+	} regsave;
+};
+
 typedef struct drm_i915_private {
 	struct drm_device *dev;
 	struct kmem_cache *slab;
@@ -1255,6 +1324,8 @@ typedef struct drm_i915_private {
 		uint16_t cur_latency[5];
 	} wm;
 
+	struct i915_package_c8 pc8;
+
 	/* Old dri1 support infrastructure, beware the dragons ya fools entering
 	 * here! */
 	struct i915_dri1_state dri1;
@@ -1629,6 +1700,7 @@ extern unsigned int i915_preliminary_hw_support __read_mostly;
 extern int i915_disable_power_well __read_mostly;
 extern int i915_enable_ips __read_mostly;
 extern bool i915_fastboot __read_mostly;
+extern int i915_enable_pc8 __read_mostly;
 extern bool i915_prefault_disable __read_mostly;
 
 extern int i915_suspend(struct drm_device *dev, pm_message_t state);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 498ef8a..683ad09 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -986,6 +986,8 @@ static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
 	bool wait_forever = true;
 	int ret;
 
+	WARN(dev_priv->pc8.irqs_disabled, "IRQs disabled\n");
+
 	if (i915_seqno_passed(ring->get_seqno(ring, true), seqno))
 		return 0;
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 8ba5d0a..00e5f64 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -85,6 +85,12 @@ ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
 {
 	assert_spin_locked(&dev_priv->irq_lock);
 
+	if (dev_priv->pc8.irqs_disabled) {
+		WARN(1, "IRQs disabled\n");
+		dev_priv->pc8.regsave.deimr &= ~mask;
+		return;
+	}
+
 	if ((dev_priv->irq_mask & mask) != 0) {
 		dev_priv->irq_mask &= ~mask;
 		I915_WRITE(DEIMR, dev_priv->irq_mask);
@@ -97,6 +103,12 @@ ironlake_disable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
 {
 	assert_spin_locked(&dev_priv->irq_lock);
 
+	if (dev_priv->pc8.irqs_disabled) {
+		WARN(1, "IRQs disabled\n");
+		dev_priv->pc8.regsave.deimr |= mask;
+		return;
+	}
+
 	if ((dev_priv->irq_mask & mask) != mask) {
 		dev_priv->irq_mask |= mask;
 		I915_WRITE(DEIMR, dev_priv->irq_mask);
@@ -116,6 +128,14 @@ static void ilk_update_gt_irq(struct drm_i915_private *dev_priv,
 {
 	assert_spin_locked(&dev_priv->irq_lock);
 
+	if (dev_priv->pc8.irqs_disabled) {
+		WARN(1, "IRQs disabled\n");
+		dev_priv->pc8.regsave.gtimr &= ~interrupt_mask;
+		dev_priv->pc8.regsave.gtimr |= (~enabled_irq_mask &
+						interrupt_mask);
+		return;
+	}
+
 	dev_priv->gt_irq_mask &= ~interrupt_mask;
 	dev_priv->gt_irq_mask |= (~enabled_irq_mask & interrupt_mask);
 	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
@@ -146,6 +166,14 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
+	if (dev_priv->pc8.irqs_disabled) {
+		WARN(1, "IRQs disabled\n");
+		dev_priv->pc8.regsave.gen6_pmimr &= ~interrupt_mask;
+		dev_priv->pc8.regsave.gen6_pmimr |= (~enabled_irq_mask &
+						     interrupt_mask);
+		return;
+	}
+
 	new_val = dev_priv->pm_irq_mask;
 	new_val &= ~interrupt_mask;
 	new_val |= (~enabled_irq_mask & interrupt_mask);
@@ -257,6 +285,15 @@ static void ibx_display_interrupt_update(struct drm_i915_private *dev_priv,
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
+	if (dev_priv->pc8.irqs_disabled &&
+	    (interrupt_mask & SDE_HOTPLUG_MASK_CPT)) {
+		WARN(1, "IRQs disabled\n");
+		dev_priv->pc8.regsave.sdeimr &= ~interrupt_mask;
+		dev_priv->pc8.regsave.sdeimr |= (~enabled_irq_mask &
+						 interrupt_mask);
+		return;
+	}
+
 	I915_WRITE(SDEIMR, sdeimr);
 	POSTING_READ(SDEIMR);
 }
@@ -3116,3 +3153,72 @@ void intel_hpd_init(struct drm_device *dev)
 		dev_priv->display.hpd_irq_setup(dev);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
+
+/* Disable interrupts so we can allow Package C8+. */
+void hsw_pc8_disable_interrupts(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	unsigned long irqflags;
+
+	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+
+	dev_priv->pc8.regsave.deimr = I915_READ(DEIMR);
+	dev_priv->pc8.regsave.sdeimr = I915_READ(SDEIMR);
+	dev_priv->pc8.regsave.gtimr = I915_READ(GTIMR);
+	dev_priv->pc8.regsave.gtier = I915_READ(GTIER);
+	dev_priv->pc8.regsave.gen6_pmimr = I915_READ(GEN6_PMIMR);
+
+	ironlake_disable_display_irq(dev_priv, ~DE_PCH_EVENT_IVB);
+	ibx_disable_display_interrupt(dev_priv, ~SDE_HOTPLUG_MASK_CPT);
+	ilk_disable_gt_irq(dev_priv, 0xffffffff);
+	snb_disable_pm_irq(dev_priv, 0xffffffff);
+
+	dev_priv->pc8.irqs_disabled = true;
+
+	WARN(I915_READ(DEIIR), "DEIIR is not 0\n");
+	WARN(I915_READ(SDEIIR), "SDEIIR is not 0\n");
+	WARN(I915_READ(GTIIR), "GTIIR is not 0\n");
+	WARN(I915_READ(GEN6_PMIIR), "GEN6_PMIIR is not 0\n");
+
+	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+}
+
+/* Restore interrupts so we can recover from Package C8+. */
+void hsw_pc8_restore_interrupts(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	unsigned long irqflags;
+	uint32_t val, expected;
+
+	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+
+	val = I915_READ(DEIMR);
+	expected = ~DE_PCH_EVENT_IVB;
+	WARN(val != expected, "DEIMR is 0x%08x, not 0x%08x\n", val, expected);
+
+	val = I915_READ(SDEIMR) & ~SDE_HOTPLUG_MASK_CPT;
+	expected = ~SDE_HOTPLUG_MASK_CPT;
+	WARN(val != expected, "SDEIMR non-HPD bits are 0x%08x, not 0x%08x\n",
+	     val, expected);
+
+	val = I915_READ(GTIMR);
+	expected = 0xffffffff;
+	WARN(val != expected, "GTIMR is 0x%08x, not 0x%08x\n", val, expected);
+
+	val = I915_READ(GEN6_PMIMR);
+	expected = 0xffffffff;
+	WARN(val != expected, "GEN6_PMIMR is 0x%08x, not 0x%08x\n", val,
+	     expected);
+
+	dev_priv->pc8.irqs_disabled = false;
+
+	ironlake_enable_display_irq(dev_priv, ~dev_priv->pc8.regsave.deimr);
+	ibx_enable_display_interrupt(dev_priv,
+				     ~dev_priv->pc8.regsave.sdeimr &
+				     ~SDE_HOTPLUG_MASK_CPT);
+	ilk_enable_gt_irq(dev_priv, ~dev_priv->pc8.regsave.gtimr);
+	snb_enable_pm_irq(dev_priv, ~dev_priv->pc8.regsave.gen6_pmimr);
+	I915_WRITE(GTIER, dev_priv->pc8.regsave.gtier);
+
+	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6bb46fd..ca19461 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6064,6 +6064,170 @@ void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 	}
 }
 
+void hsw_enable_pc8_work(struct work_struct *__work)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(to_delayed_work(__work), struct drm_i915_private,
+			     pc8.enable_work);
+	struct drm_device *dev = dev_priv->dev;
+	uint32_t val;
+
+	if (dev_priv->pc8.enabled)
+		return;
+
+	DRM_DEBUG_KMS("Enabling package C8+\n");
+
+	dev_priv->pc8.enabled = true;
+
+	if (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
+		val = I915_READ(SOUTH_DSPCLK_GATE_D);
+		val &= ~PCH_LP_PARTITION_LEVEL_DISABLE;
+		I915_WRITE(SOUTH_DSPCLK_GATE_D, val);
+	}
+
+	cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
+	lpt_disable_clkout_dp(dev);
+	hsw_pc8_disable_interrupts(dev);
+	hsw_disable_lcpll(dev_priv, true, true);
+}
+
+static void __hsw_enable_package_c8(struct drm_i915_private *dev_priv)
+{
+	WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
+	WARN(dev_priv->pc8.disable_count < 1,
+	     "pc8.disable_count: %d\n", dev_priv->pc8.disable_count);
+
+	dev_priv->pc8.disable_count--;
+	if (dev_priv->pc8.disable_count != 0)
+		return;
+
+	schedule_delayed_work(&dev_priv->pc8.enable_work,
+			      msecs_to_jiffies(5 * 1000));
+}
+
+static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+	uint32_t val;
+
+	WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
+	WARN(dev_priv->pc8.disable_count < 0,
+	     "pc8.disable_count: %d\n", dev_priv->pc8.disable_count);
+
+	dev_priv->pc8.disable_count++;
+	if (dev_priv->pc8.disable_count != 1)
+		return;
+
+	cancel_delayed_work_sync(&dev_priv->pc8.enable_work);
+	if (!dev_priv->pc8.enabled)
+		return;
+
+	/* Make sure we're not on PC8 state before disabling PC8, otherwise
+	 * we'll hang the machine! */
+	dev_priv->uncore.funcs.force_wake_get(dev_priv);
+
+	DRM_DEBUG_KMS("Disabling package C8+\n");
+
+	hsw_restore_lcpll(dev_priv);
+	hsw_pc8_restore_interrupts(dev);
+	lpt_init_pch_refclk(dev);
+
+	if (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
+		val = I915_READ(SOUTH_DSPCLK_GATE_D);
+		val |= PCH_LP_PARTITION_LEVEL_DISABLE;
+		I915_WRITE(SOUTH_DSPCLK_GATE_D, val);
+	}
+
+	intel_prepare_ddi(dev);
+	i915_gem_init_swizzling(dev);
+	intel_enable_gt_powersave(dev);
+	dev_priv->pc8.enabled = false;
+	dev_priv->uncore.funcs.force_wake_put(dev_priv);
+}
+
+void hsw_enable_package_c8(struct drm_i915_private *dev_priv)
+{
+	mutex_lock(&dev_priv->pc8.lock);
+	__hsw_enable_package_c8(dev_priv);
+	mutex_unlock(&dev_priv->pc8.lock);
+}
+
+void hsw_disable_package_c8(struct drm_i915_private *dev_priv)
+{
+	mutex_lock(&dev_priv->pc8.lock);
+	__hsw_disable_package_c8(dev_priv);
+	mutex_unlock(&dev_priv->pc8.lock);
+}
+
+static bool hsw_can_enable_package_c8(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+	struct intel_crtc *crtc;
+	uint32_t val;
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head)
+		if (crtc->base.enabled)
+			return false;
+
+	/* This case is still possible since we have the i915.disable_power_well
+	 * parameter and also the KVMr or something else might be requesting the
+	 * power well. */
+	val = I915_READ(HSW_PWR_WELL_DRIVER);
+	if (val != 0) {
+		DRM_DEBUG_KMS("Not enabling PC8: power well on\n");
+		return false;
+	}
+
+	return true;
+}
+
+/* Since we're called from modeset_global_resources there's no way to
+ * symmetrically increase and decrease the refcount, so we use
+ * dev_priv->pc8.requirements_met to track whether we already have the refcount
+ * or not.
+ */
+static void hsw_update_package_c8(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	bool allow;
+
+	if (!i915_enable_pc8)
+		return;
+
+	mutex_lock(&dev_priv->pc8.lock);
+
+	allow = hsw_can_enable_package_c8(dev_priv);
+
+	if (allow == dev_priv->pc8.requirements_met)
+		goto done;
+
+	dev_priv->pc8.requirements_met = allow;
+
+	if (allow)
+		__hsw_enable_package_c8(dev_priv);
+	else
+		__hsw_disable_package_c8(dev_priv);
+
+done:
+	mutex_unlock(&dev_priv->pc8.lock);
+}
+
+static void hsw_package_c8_gpu_idle(struct drm_i915_private *dev_priv)
+{
+	if (!dev_priv->pc8.gpu_idle) {
+		dev_priv->pc8.gpu_idle = true;
+		hsw_enable_package_c8(dev_priv);
+	}
+}
+
+static void hsw_package_c8_gpu_busy(struct drm_i915_private *dev_priv)
+{
+	if (dev_priv->pc8.gpu_idle) {
+		dev_priv->pc8.gpu_idle = false;
+		hsw_disable_package_c8(dev_priv);
+	}
+}
+
 static void haswell_modeset_global_resources(struct drm_device *dev)
 {
 	bool enable = false;
@@ -6079,6 +6243,8 @@ static void haswell_modeset_global_resources(struct drm_device *dev)
 	}
 
 	intel_set_power_well(dev, enable);
+
+	hsw_update_package_c8(dev);
 }
 
 static int haswell_crtc_mode_set(struct drm_crtc *crtc,
@@ -7310,13 +7476,19 @@ static void intel_decrease_pllclock(struct drm_crtc *crtc)
 
 void intel_mark_busy(struct drm_device *dev)
 {
-	i915_update_gfx_val(dev->dev_private);
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	hsw_package_c8_gpu_busy(dev_priv);
+	i915_update_gfx_val(dev_priv);
 }
 
 void intel_mark_idle(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
 
+	hsw_package_c8_gpu_idle(dev_priv);
+
 	if (!i915_powersave)
 		return;
 
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 63b6722d..b150d23 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -344,6 +344,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 	else
 		precharge = 5;
 
+	intel_aux_display_runtime_get(dev_priv);
+
 	/* Try to wait for any previous AUX channel activity */
 	for (try = 0; try < 3; try++) {
 		status = I915_READ_NOTRACE(ch_ctl);
@@ -434,6 +436,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 	ret = recv_bytes;
 out:
 	pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
+	intel_aux_display_runtime_put(dev_priv);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8222f24..39b4530 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -784,5 +784,12 @@ extern void ilk_disable_gt_irq(struct drm_i915_private *dev_priv,
 extern void snb_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 extern void snb_disable_pm_irq(struct drm_i915_private *dev_priv,
 			       uint32_t mask);
+extern void hsw_enable_pc8_work(struct work_struct *__work);
+extern void hsw_enable_package_c8(struct drm_i915_private *dev_priv);
+extern void hsw_disable_package_c8(struct drm_i915_private *dev_priv);
+extern void hsw_pc8_disable_interrupts(struct drm_device *dev);
+extern void hsw_pc8_restore_interrupts(struct drm_device *dev);
+extern void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv);
+extern void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv);
 
 #endif /* __INTEL_DRV_H__ */
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 639fe19..d1c1e0f7 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -398,6 +398,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
 	int i, reg_offset;
 	int ret = 0;
 
+	intel_aux_display_runtime_get(dev_priv);
 	mutex_lock(&dev_priv->gmbus_mutex);
 
 	if (bus->force_bit) {
@@ -497,6 +498,7 @@ timeout:
 
 out:
 	mutex_unlock(&dev_priv->gmbus_mutex);
+	intel_aux_display_runtime_put(dev_priv);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 70d840d..c88c317 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5428,6 +5428,17 @@ void intel_init_power_well(struct drm_device *dev)
 		I915_WRITE(HSW_PWR_WELL_BIOS, 0);
 }
 
+/* Disables PC8 so we can use the GMBUS and DP AUX interrupts. */
+void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv)
+{
+	hsw_disable_package_c8(dev_priv);
+}
+
+void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv)
+{
+	hsw_enable_package_c8(dev_priv);
+}
+
 /* Set up chip specific power management-related functions */
 void intel_init_pm(struct drm_device *dev)
 {
-- 
1.8.1.2

Re: [PATCH 7/9] drm/i915: allow package C8+ states on Haswell (disabled)

[Chris Wilson]

Quick note...

On Fri, Aug 09, 2013 at 05:10:05PM -0300, Paulo Zanoni wrote:
> +	WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));

Preferred form is now lockdep_assert_held(&dev_priv->pc8.lock);
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 7/9] drm/i915: allow package C8+ states on Haswell (disabled)

[Paulo Zanoni]

2013/8/9 Chris Wilson <chris@chris-wilson.co.uk>:
> Quick note...
>
> On Fri, Aug 09, 2013 at 05:10:05PM -0300, Paulo Zanoni wrote:
>> +     WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
>
> Preferred form is now lockdep_assert_held(&dev_priv->pc8.lock);

Should I also convert all our other usages of
WARN_ON(!mutex_is_locked()) and BUG_ON(!mutex_is_locked()) too? On a
separate patch, of course. We have currently no usage of
lockdep_assert_held, and I like consistency, so fully switching to the
preferred form is good IMHO.

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



-- 
Paulo Zanoni

Re: [PATCH 7/9] drm/i915: allow package C8+ states on Haswell (disabled)

[Daniel Vetter]

On Fri, Aug 9, 2013 at 11:34 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2013/8/9 Chris Wilson <chris@chris-wilson.co.uk>:
>> Quick note...
>>
>> On Fri, Aug 09, 2013 at 05:10:05PM -0300, Paulo Zanoni wrote:
>>> +     WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
>>
>> Preferred form is now lockdep_assert_held(&dev_priv->pc8.lock);
>
> Should I also convert all our other usages of
> WARN_ON(!mutex_is_locked()) and BUG_ON(!mutex_is_locked()) too? On a
> separate patch, of course. We have currently no usage of
> lockdep_assert_held, and I like consistency, so fully switching to the
> preferred form is good IMHO.

Tbh I don't understand really why lockdep_assert_held is better ...
it's right that it also checks that indeed the current task is holding
the lock (and not some random other imposter). But the downside is
that it's a noop without CONFIG_PROVE_LOCKING. And due to the massive
perf impact of that option not many people actually run with it. At
least I tend to only enable it when doing tricky locking work on my
dev machines and not in general ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 7/9] drm/i915: allow package C8+ states on Haswell (disabled)

[Chris Wilson]

On Sat, Aug 10, 2013 at 09:55:14AM +0200, Daniel Vetter wrote:
> On Fri, Aug 9, 2013 at 11:34 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> > 2013/8/9 Chris Wilson <chris@chris-wilson.co.uk>:
> >> Quick note...
> >>
> >> On Fri, Aug 09, 2013 at 05:10:05PM -0300, Paulo Zanoni wrote:
> >>> +     WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
> >>
> >> Preferred form is now lockdep_assert_held(&dev_priv->pc8.lock);
> >
> > Should I also convert all our other usages of
> > WARN_ON(!mutex_is_locked()) and BUG_ON(!mutex_is_locked()) too? On a
> > separate patch, of course. We have currently no usage of
> > lockdep_assert_held, and I like consistency, so fully switching to the
> > preferred form is good IMHO.
> 
> Tbh I don't understand really why lockdep_assert_held is better ...
> it's right that it also checks that indeed the current task is holding
> the lock (and not some random other imposter). But the downside is
> that it's a noop without CONFIG_PROVE_LOCKING. And due to the massive
> perf impact of that option not many people actually run with it. At
> least I tend to only enable it when doing tricky locking work on my
> dev machines and not in general ...

It doesn't shout so much and people are starting to complain about all
the sanity checks existing outside of the grand lockdep.

Who doesn't have a few machines running with lockdep all the time? ;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 7/9] drm/i915: allow package C8+ states on Haswell (disabled)

[Paulo Zanoni]

2013/8/10 Chris Wilson <chris@chris-wilson.co.uk>:
> On Sat, Aug 10, 2013 at 09:55:14AM +0200, Daniel Vetter wrote:
>> On Fri, Aug 9, 2013 at 11:34 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> > 2013/8/9 Chris Wilson <chris@chris-wilson.co.uk>:
>> >> Quick note...
>> >>
>> >> On Fri, Aug 09, 2013 at 05:10:05PM -0300, Paulo Zanoni wrote:
>> >>> +     WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
>> >>
>> >> Preferred form is now lockdep_assert_held(&dev_priv->pc8.lock);
>> >
>> > Should I also convert all our other usages of
>> > WARN_ON(!mutex_is_locked()) and BUG_ON(!mutex_is_locked()) too? On a
>> > separate patch, of course. We have currently no usage of
>> > lockdep_assert_held, and I like consistency, so fully switching to the
>> > preferred form is good IMHO.
>>
>> Tbh I don't understand really why lockdep_assert_held is better ...
>> it's right that it also checks that indeed the current task is holding
>> the lock (and not some random other imposter). But the downside is
>> that it's a noop without CONFIG_PROVE_LOCKING. And due to the massive
>> perf impact of that option not many people actually run with it. At
>> least I tend to only enable it when doing tricky locking work on my
>> dev machines and not in general ...
>
> It doesn't shout so much and people are starting to complain about all
> the sanity checks existing outside of the grand lockdep.
>
> Who doesn't have a few machines running with lockdep all the time? ;-)

Considering we still don't have a consensus, I'll keep the WARNs so
our driver stays consistent. If I see patches converting everything on
our driver to lockdep_assert_held, then I'll update this patch. IMHO
we should either convert everything or nothing.


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



-- 
Paulo Zanoni

Re: [PATCH 7/9] drm/i915: allow package C8+ states on Haswell (disabled)

[Chris Wilson]

On Fri, Aug 09, 2013 at 05:10:05PM -0300, Paulo Zanoni wrote:
> +/* Disable interrupts so we can allow Package C8+. */
> +void hsw_pc8_disable_interrupts(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	unsigned long irqflags;
> +
> +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> +
> +	dev_priv->pc8.regsave.deimr = I915_READ(DEIMR);
> +	dev_priv->pc8.regsave.sdeimr = I915_READ(SDEIMR);
> +	dev_priv->pc8.regsave.gtimr = I915_READ(GTIMR);
> +	dev_priv->pc8.regsave.gtier = I915_READ(GTIER);
> +	dev_priv->pc8.regsave.gen6_pmimr = I915_READ(GEN6_PMIMR);
> +
> +	ironlake_disable_display_irq(dev_priv, ~DE_PCH_EVENT_IVB);
> +	ibx_disable_display_interrupt(dev_priv, ~SDE_HOTPLUG_MASK_CPT);
> +	ilk_disable_gt_irq(dev_priv, 0xffffffff);
> +	snb_disable_pm_irq(dev_priv, 0xffffffff);
> +
> +	dev_priv->pc8.irqs_disabled = true;
> +
> +	WARN(I915_READ(DEIIR), "DEIIR is not 0\n");
> +	WARN(I915_READ(SDEIIR), "SDEIIR is not 0\n");
> +	WARN(I915_READ(GTIIR), "GTIIR is not 0\n");
> +	WARN(I915_READ(GEN6_PMIIR), "GEN6_PMIIR is not 0\n");

I keep looking at this, because we will hit these warns. But I also
don't think it a problem, as the interrupt handle will run as soon as we
release the irq_lock and that will be the final time until we reenable
interrupts. (Just kill the WARNs.)

Now, why IMR and not IER?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 7/9] drm/i915: allow package C8+ states on Haswell (disabled)

[Paulo Zanoni]

2013/8/9 Chris Wilson <chris@chris-wilson.co.uk>:
> On Fri, Aug 09, 2013 at 05:10:05PM -0300, Paulo Zanoni wrote:
>> +/* Disable interrupts so we can allow Package C8+. */
>> +void hsw_pc8_disable_interrupts(struct drm_device *dev)
>> +{
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>> +     unsigned long irqflags;
>> +
>> +     spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>> +
>> +     dev_priv->pc8.regsave.deimr = I915_READ(DEIMR);
>> +     dev_priv->pc8.regsave.sdeimr = I915_READ(SDEIMR);
>> +     dev_priv->pc8.regsave.gtimr = I915_READ(GTIMR);
>> +     dev_priv->pc8.regsave.gtier = I915_READ(GTIER);
>> +     dev_priv->pc8.regsave.gen6_pmimr = I915_READ(GEN6_PMIMR);
>> +
>> +     ironlake_disable_display_irq(dev_priv, ~DE_PCH_EVENT_IVB);
>> +     ibx_disable_display_interrupt(dev_priv, ~SDE_HOTPLUG_MASK_CPT);
>> +     ilk_disable_gt_irq(dev_priv, 0xffffffff);
>> +     snb_disable_pm_irq(dev_priv, 0xffffffff);
>> +
>> +     dev_priv->pc8.irqs_disabled = true;
>> +
>> +     WARN(I915_READ(DEIIR), "DEIIR is not 0\n");
>> +     WARN(I915_READ(SDEIIR), "SDEIIR is not 0\n");
>> +     WARN(I915_READ(GTIIR), "GTIIR is not 0\n");
>> +     WARN(I915_READ(GEN6_PMIIR), "GEN6_PMIIR is not 0\n");
>
> I keep looking at this, because we will hit these warns. But I also
> don't think it a problem, as the interrupt handle will run as soon as we
> release the irq_lock and that will be the final time until we reenable
> interrupts. (Just kill the WARNs.)

I also thought we were going to hit these WARNs, but I don't ever hit
any of them in my current tree, even if I change the default PC8
timeout from 5s to 0.1s (which makes us enter/leave PC8+ very often),
so they never bothered me enough to be killed. Maybe the race
condition is too difficult to hit... But I can remove the WARNs.

>
> Now, why IMR and not IER?

Because on our driver, when we want to enable/disable interrupts while
everything is running we only use IMR, so our code is designed in a
way that if you grab irq_lock and update IMR with the appropriate
function you should be safe. On the other hand, some IER registers are
updated inside the irq handlers (like DEIER and SDEIER at
ironlake_irq_handler), so touching these is not as trivial. The only
advantage of using IER would be to preserve the IIR bits, but
according to Arthur the HW does not save/restore the IIR registers
when entering/leaving PC8, so no advantage.


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



-- 
Paulo Zanoni

[PATCH 8/9] drm/i915: add i915_pc8_status debugfs file

[Paulo Zanoni]

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

For now it just prints the value of the forbid_refcount and allowing
variables.

v2: Update to recent renames and add the new fields.

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 748af58..14abbb7 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1692,6 +1692,30 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 	return 0;
 }
 
+static int i915_pc8_status(struct seq_file *m, void *unused)
+{
+	struct drm_info_node *node = (struct drm_info_node *) m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (!IS_HASWELL(dev)) {
+		seq_puts(m, "not supported\n");
+		return 0;
+	}
+
+	mutex_lock(&dev_priv->pc8.lock);
+	seq_printf(m, "Requirements met: %s\n",
+		   yesno(dev_priv->pc8.requirements_met));
+	seq_printf(m, "GPU idle: %s\n", yesno(dev_priv->pc8.gpu_idle));
+	seq_printf(m, "Disable count: %d\n", dev_priv->pc8.disable_count);
+	seq_printf(m, "IRQs disabled: %s\n",
+		   yesno(dev_priv->pc8.irqs_disabled));
+	seq_printf(m, "Enabled: %s\n", yesno(dev_priv->pc8.enabled));
+	mutex_unlock(&dev_priv->pc8.lock);
+
+	return 0;
+}
+
 static int
 i915_wedged_get(void *data, u64 *val)
 {
@@ -2127,6 +2151,7 @@ static struct drm_info_list i915_debugfs_list[] = {
 	{"i915_dpio", i915_dpio_info, 0},
 	{"i915_llc", i915_llc, 0},
 	{"i915_edp_psr_status", i915_edp_psr_status, 0},
+	{"i915_pc8_status", i915_pc8_status, 0},
 };
 #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
 
-- 
1.8.1.2

[PATCH 9/9] drm/i915: enable Package C8+ by default

[Paulo Zanoni]

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

This should be working, so enable it by default. Also easy to revert.

v2: Rebase, s/allow/enable/.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 1e53002..3209efe 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -141,9 +141,9 @@ module_param_named(fastboot, i915_fastboot, bool, 0600);
 MODULE_PARM_DESC(fastboot, "Try to skip unnecessary mode sets at boot time "
 		 "(default: false)");
 
-int i915_enable_pc8 __read_mostly = 0;
+int i915_enable_pc8 __read_mostly = 1;
 module_param_named(enable_pc8, i915_enable_pc8, int, 0600);
-MODULE_PARM_DESC(enable_pc8, "Enable support for low power package C states (PC8+) (default: false)");
+MODULE_PARM_DESC(enable_pc8, "Enable support for low power package C states (PC8+) (default: true)");
 
 bool i915_prefault_disable __read_mostly;
 module_param_named(prefault_disable, i915_prefault_disable, bool, 0600);
-- 
1.8.1.2

Re: [PATCH 0/9] Haswell Package C8+

[Daniel Vetter]

On Tue, Aug 06, 2013 at 06:57:10PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Hi
> 
> Here's my next attempt at the Haswell PC8+ feature.
> 
> What's new?
> 
>  1. I hope I implemented the feedback from Chris. Many function renames, a few
>     style changes. Chris' biggest concern was about the interrupts. So in this
>     series you'll see patches 2-6 moving some interrupt code around, so all
>     changes to each IMR register should go through a single function. With this
>     change we can easily detect when someone wants to change IMR but interrupts
>     are disabled. As he suggested, we also don't enable the interrupts in that
>     case.
>  2. Another difference is that now we also disable PC8 when there's GPU work to
>     do. The lack of interrupts made things like glxgears work at 2 FPS. I talked
>     to Ben about this and what I understood is that the lack of interrupts is
>     not really a problem, it just makes things slower. But still I decided to
>     write the code to disable PC8 when we have GPU work to do, so background
>     apps can run faster than 2 FPS.

Ben just mislead you here, this is only true due to an implemenation
detail on our hw simulation enviroment. On real hw running gem workloads
without interrupt support very much blows up as soon as something hits
__wait_seqno and actually goes to sleep on the waitqueue.

If you pimp the your igt testcase in the test_batch code with the dummy
workload and wait_rendering stuff you should be able to hit this fairly
easily. I haven't looked yet but I think adding a "are interrupts
working/am I out of pc8+" check to __wait_seqno would be good.
-Daniel

>  3. Another difference is that now we enable PC8 on a delayed work function that
>     only gets called if we stay with 0 refcount for at least 5 seconds. So when
>     someone runs "xrandr" we won't enable/disable PC8 dozens of times. Also, on
>     "xset dpms force off" we'll only get to PC8 if the desktop environment
>     decides to not do rendering every second. Not getting to PC8? Fix your
>     desktop environment!
>  4. Due to 3 and the fact that Daniel didn't seem to like my "do DP AUX and
>     GMBUS without interrupts" approach, the previous patches that implemented
>     this just got dropped: we now have the delayed work thing which should
>     replace them.
>  5. I also added code to wake up from PC8 when doing suspend, we need this.
> 
> Despite this, the other thing to discuss is about the size of patch 7. I can
> certainly try to split it, but I really think that if all you want is to take a
> brief look at the code just and drop some bikesheds, then having a big patch
> that implements everything in one piece seems better. I can split this later if
> needed. I'm also open to ideas on how to really split this patch. Also notice
> that even if the patch is big, it doesn't remove a single line of the current
> code. And it adds PC8 disabled by default, so if git bisect points to that patch
> the surface to look will be small.
> 
> Another thing worth mentioning is that all this code doesn't have IS_HASWELL
> checks, and on non-Haswell platforms the refcount will never reach 0, so we
> won't ever try to enable PC8. I'm not sure if that's what we want, so please
> comment on that.
> 
> Even if we decide to delay patch 7, we could try to merge patches 1-6 if they
> look acceptable. I'm also not really sure if we want the last patch yet, but
> it's there just in case.
> 
> Also, I couldn't do i-g-t regression testing since the i-g-t test suite is quite
> unreliable on Haswell right now.
> 
> Thanks,
> Paulo
> 
> Paulo Zanoni (9):
>   drm/i915: add the FCLK case to intel_ddi_get_cdclk_freq
>   drm/i915: wrap GTIMR changes
>   drm/i915: wrap GEN6_PMIMR changes
>   drm/i915: don't update GEN6_PMIMR when it's not needed
>   drm/i915: add dev_priv->pm_irq_mask
>   drm/i915: don't disable/reenable IVB error interrupts when not needed
>   drm/i915: allow package C8+ states on Haswell (disabled)
>   drm/i915: add i915_pc8_status debugfs file
>   drm/i915: enable Package C8+ by default
> 
>  drivers/gpu/drm/i915/i915_debugfs.c     |  25 ++++
>  drivers/gpu/drm/i915/i915_dma.c         |  10 ++
>  drivers/gpu/drm/i915/i915_drv.c         |  11 ++
>  drivers/gpu/drm/i915/i915_drv.h         |  73 ++++++++++++
>  drivers/gpu/drm/i915/i915_irq.c         | 202 +++++++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_ddi.c        |   9 +-
>  drivers/gpu/drm/i915/intel_display.c    | 164 ++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dp.c         |   3 +
>  drivers/gpu/drm/i915/intel_drv.h        |  14 +++
>  drivers/gpu/drm/i915/intel_i2c.c        |   2 +
>  drivers/gpu/drm/i915/intel_pm.c         |  13 +-
>  drivers/gpu/drm/i915/intel_ringbuffer.c |  32 ++---
>  12 files changed, 518 insertions(+), 40 deletions(-)
> 
> -- 
> 1.8.1.2
> 
> _______________________________________________
> 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 0/9] Haswell Package C8+

[Paulo Zanoni]

2013/8/6 Daniel Vetter <daniel@ffwll.ch>:
> On Tue, Aug 06, 2013 at 06:57:10PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Hi
>>
>> Here's my next attempt at the Haswell PC8+ feature.
>>
>> What's new?
>>
>>  1. I hope I implemented the feedback from Chris. Many function renames, a few
>>     style changes. Chris' biggest concern was about the interrupts. So in this
>>     series you'll see patches 2-6 moving some interrupt code around, so all
>>     changes to each IMR register should go through a single function. With this
>>     change we can easily detect when someone wants to change IMR but interrupts
>>     are disabled. As he suggested, we also don't enable the interrupts in that
>>     case.
>>  2. Another difference is that now we also disable PC8 when there's GPU work to
>>     do. The lack of interrupts made things like glxgears work at 2 FPS. I talked
>>     to Ben about this and what I understood is that the lack of interrupts is
>>     not really a problem, it just makes things slower. But still I decided to
>>     write the code to disable PC8 when we have GPU work to do, so background
>>     apps can run faster than 2 FPS.
>
> Ben just mislead you here, this is only true due to an implemenation
> detail on our hw simulation enviroment. On real hw running gem workloads
> without interrupt support very much blows up as soon as something hits
> __wait_seqno and actually goes to sleep on the waitqueue.
>
> If you pimp the your igt testcase in the test_batch code with the dummy
> workload and wait_rendering stuff you should be able to hit this fairly
> easily. I haven't looked yet but I think adding a "are interrupts
> working/am I out of pc8+" check to __wait_seqno would be good.

Anyway, the current patches are not supposed to be broken since we try
to disable PC8 when there's stuff to render, but I'll add the check
and test everything.


> -Daniel
>
>>  3. Another difference is that now we enable PC8 on a delayed work function that
>>     only gets called if we stay with 0 refcount for at least 5 seconds. So when
>>     someone runs "xrandr" we won't enable/disable PC8 dozens of times. Also, on
>>     "xset dpms force off" we'll only get to PC8 if the desktop environment
>>     decides to not do rendering every second. Not getting to PC8? Fix your
>>     desktop environment!
>>  4. Due to 3 and the fact that Daniel didn't seem to like my "do DP AUX and
>>     GMBUS without interrupts" approach, the previous patches that implemented
>>     this just got dropped: we now have the delayed work thing which should
>>     replace them.
>>  5. I also added code to wake up from PC8 when doing suspend, we need this.
>>
>> Despite this, the other thing to discuss is about the size of patch 7. I can
>> certainly try to split it, but I really think that if all you want is to take a
>> brief look at the code just and drop some bikesheds, then having a big patch
>> that implements everything in one piece seems better. I can split this later if
>> needed. I'm also open to ideas on how to really split this patch. Also notice
>> that even if the patch is big, it doesn't remove a single line of the current
>> code. And it adds PC8 disabled by default, so if git bisect points to that patch
>> the surface to look will be small.
>>
>> Another thing worth mentioning is that all this code doesn't have IS_HASWELL
>> checks, and on non-Haswell platforms the refcount will never reach 0, so we
>> won't ever try to enable PC8. I'm not sure if that's what we want, so please
>> comment on that.
>>
>> Even if we decide to delay patch 7, we could try to merge patches 1-6 if they
>> look acceptable. I'm also not really sure if we want the last patch yet, but
>> it's there just in case.
>>
>> Also, I couldn't do i-g-t regression testing since the i-g-t test suite is quite
>> unreliable on Haswell right now.
>>
>> Thanks,
>> Paulo
>>
>> Paulo Zanoni (9):
>>   drm/i915: add the FCLK case to intel_ddi_get_cdclk_freq
>>   drm/i915: wrap GTIMR changes
>>   drm/i915: wrap GEN6_PMIMR changes
>>   drm/i915: don't update GEN6_PMIMR when it's not needed
>>   drm/i915: add dev_priv->pm_irq_mask
>>   drm/i915: don't disable/reenable IVB error interrupts when not needed
>>   drm/i915: allow package C8+ states on Haswell (disabled)
>>   drm/i915: add i915_pc8_status debugfs file
>>   drm/i915: enable Package C8+ by default
>>
>>  drivers/gpu/drm/i915/i915_debugfs.c     |  25 ++++
>>  drivers/gpu/drm/i915/i915_dma.c         |  10 ++
>>  drivers/gpu/drm/i915/i915_drv.c         |  11 ++
>>  drivers/gpu/drm/i915/i915_drv.h         |  73 ++++++++++++
>>  drivers/gpu/drm/i915/i915_irq.c         | 202 +++++++++++++++++++++++++++++---
>>  drivers/gpu/drm/i915/intel_ddi.c        |   9 +-
>>  drivers/gpu/drm/i915/intel_display.c    | 164 ++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_dp.c         |   3 +
>>  drivers/gpu/drm/i915/intel_drv.h        |  14 +++
>>  drivers/gpu/drm/i915/intel_i2c.c        |   2 +
>>  drivers/gpu/drm/i915/intel_pm.c         |  13 +-
>>  drivers/gpu/drm/i915/intel_ringbuffer.c |  32 ++---
>>  12 files changed, 518 insertions(+), 40 deletions(-)
>>
>> --
>> 1.8.1.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

[PATCH 6.1/9] drm/i915: don't queue PM events we won't process

[Paulo Zanoni]

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

On SNB/IVB/VLV we only call gen6_rps_irq_handler if one of the IIR
bits set is part of GEN6_PM_RPS_EVENTS, but at gen6_rps_irq_handler we
add all the enabled IIR bits to the work queue, not only the ones that
are part of GEN6_PM_RPS_EVENTS. But then gen6_pm_rps_work only
processes GEN6_PM_RPS_EVENTS, so it's useless to add anything that's
not GEN6_PM_RPS_EVENTS to the work queue.

As a bonus, gen6_rps_irq_handler looks more similar to
hsw_pm_irq_handler, so we may be able to merge them in the future.

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 0f46d33..5b51c43 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -959,7 +959,7 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv,
 	 */
 
 	spin_lock(&dev_priv->irq_lock);
-	dev_priv->rps.pm_iir |= pm_iir;
+	dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_RPS_EVENTS;
 	snb_set_pm_irq(dev_priv, dev_priv->rps.pm_iir);
 	spin_unlock(&dev_priv->irq_lock);
 
@@ -1128,7 +1128,7 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
 		if (pipe_stats[0] & PIPE_GMBUS_INTERRUPT_STATUS)
 			gmbus_irq_handler(dev);
 
-		if (pm_iir & GEN6_PM_RPS_EVENTS)
+		if (pm_iir)
 			gen6_rps_irq_handler(dev_priv, pm_iir);
 
 		I915_WRITE(GTIIR, gt_iir);
@@ -1433,7 +1433,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 		if (pm_iir) {
 			if (IS_HASWELL(dev))
 				hsw_pm_irq_handler(dev_priv, pm_iir);
-			else if (pm_iir & GEN6_PM_RPS_EVENTS)
+			else
 				gen6_rps_irq_handler(dev_priv, pm_iir);
 			I915_WRITE(GEN6_PMIIR, pm_iir);
 			ret = IRQ_HANDLED;
-- 
1.8.1.2

[PATCH 6.2/9] drm/i915: fix how we mask PMIMR when adding work to the queue

[Paulo Zanoni]

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

It seems we've been doing this ever since we started processing the
RPS events on a work queue, on commit "drm/i915: move gen6 rps
handling to workqueue", 4912d04193733a825216b926ffd290fada88ab07.

The problem is: when we add work to the queue, instead of just masking
the bits we queued and leaving all the others on their current state,
we mask the bits we queued and unmask all the others. This basically
means we'll be unmasking a bunch of interrupts we're not going to
process. And if you look at gen6_pm_rps_work, we unmask back only
GEN6_PM_RPS_EVENTS, which means the bits we unmasked when adding work
to the queue will remain unmasked after we process the queue.

Notice that even though we unmask those unrelated interrupts, we never
enable them on IER, so they don't fire our interrupt handler, they
just stay there on IIR waiting to be cleared when something else
triggers the interrupt handler.

So this patch does what seems to make more sense: mask only the bits
we add to the queue, without unmasking anything else, and so we'll
unmask them after we process the queue.

As a side effect we also have to remove that WARN, because it is not
only making sure we don't mask useful interrupts, it is also making
sure we do unmask useless interrupts! That piece of code should not be
responsible for knowing which bits should be unmasked, so just don't
assert anything, and trust that snb_disable_pm_irq should be doing the
right thing.

With i915.enable_pc8=1 I was getting ocasional "GEN6_PMIIR is not 0"
error messages due to the fact that we unmask those unrelated
interrupts but don't enable them.

Note: if bugs start bisecting to this patch, then it probably means
someone was relying on the fact that we unmask everything by accident,
then we should fix gen5_gt_irq_postinstall or whoever needs the
accidentally unmasked interrupts. Or maybe I was just wrong and we
need to revert this patch :)

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 5b51c43..d9ebfb6 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -167,11 +167,6 @@ void snb_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
 	snb_update_pm_irq(dev_priv, mask, 0);
 }
 
-static void snb_set_pm_irq(struct drm_i915_private *dev_priv, uint32_t val)
-{
-	snb_update_pm_irq(dev_priv, 0xffffffff, ~val);
-}
-
 static bool ivb_can_enable_err_int(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -960,7 +955,7 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv,
 
 	spin_lock(&dev_priv->irq_lock);
 	dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_RPS_EVENTS;
-	snb_set_pm_irq(dev_priv, dev_priv->rps.pm_iir);
+	snb_disable_pm_irq(dev_priv, pm_iir & GEN6_PM_RPS_EVENTS);
 	spin_unlock(&dev_priv->irq_lock);
 
 	queue_work(dev_priv->wq, &dev_priv->rps.work);
@@ -1043,9 +1038,7 @@ static void hsw_pm_irq_handler(struct drm_i915_private *dev_priv,
 	if (pm_iir & GEN6_PM_RPS_EVENTS) {
 		spin_lock(&dev_priv->irq_lock);
 		dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_RPS_EVENTS;
-		snb_set_pm_irq(dev_priv, dev_priv->rps.pm_iir);
-		/* never want to mask useful interrupts. */
-		WARN_ON(I915_READ_NOTRACE(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
+		snb_disable_pm_irq(dev_priv, pm_iir & GEN6_PM_RPS_EVENTS);
 		spin_unlock(&dev_priv->irq_lock);
 
 		queue_work(dev_priv->wq, &dev_priv->rps.work);
-- 
1.8.1.2

Re: [PATCH 6.2/9] drm/i915: fix how we mask PMIMR when adding work to the queue

[Daniel Vetter]

On Fri, Aug 09, 2013 at 05:04:36PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> It seems we've been doing this ever since we started processing the
> RPS events on a work queue, on commit "drm/i915: move gen6 rps
> handling to workqueue", 4912d04193733a825216b926ffd290fada88ab07.
> 
> The problem is: when we add work to the queue, instead of just masking
> the bits we queued and leaving all the others on their current state,
> we mask the bits we queued and unmask all the others. This basically
> means we'll be unmasking a bunch of interrupts we're not going to
> process. And if you look at gen6_pm_rps_work, we unmask back only
> GEN6_PM_RPS_EVENTS, which means the bits we unmasked when adding work
> to the queue will remain unmasked after we process the queue.
> 
> Notice that even though we unmask those unrelated interrupts, we never
> enable them on IER, so they don't fire our interrupt handler, they
> just stay there on IIR waiting to be cleared when something else
> triggers the interrupt handler.
> 
> So this patch does what seems to make more sense: mask only the bits
> we add to the queue, without unmasking anything else, and so we'll
> unmask them after we process the queue.
> 
> As a side effect we also have to remove that WARN, because it is not
> only making sure we don't mask useful interrupts, it is also making
> sure we do unmask useless interrupts! That piece of code should not be
> responsible for knowing which bits should be unmasked, so just don't
> assert anything, and trust that snb_disable_pm_irq should be doing the
> right thing.
> 
> With i915.enable_pc8=1 I was getting ocasional "GEN6_PMIIR is not 0"
> error messages due to the fact that we unmask those unrelated
> interrupts but don't enable them.
> 
> Note: if bugs start bisecting to this patch, then it probably means
> someone was relying on the fact that we unmask everything by accident,
> then we should fix gen5_gt_irq_postinstall or whoever needs the
> accidentally unmasked interrupts. Or maybe I was just wrong and we
> need to revert this patch :)
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

I've added another note here explaining that with the addition of VEBOX
this little bug started to matter: After the first rps interrupt we'll
never mask the VEBOX user interrupt again and so blow through cpu time
needlessly when running video workloads using VEBOX.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 5b51c43..d9ebfb6 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -167,11 +167,6 @@ void snb_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
>  	snb_update_pm_irq(dev_priv, mask, 0);
>  }
>  
> -static void snb_set_pm_irq(struct drm_i915_private *dev_priv, uint32_t val)
> -{
> -	snb_update_pm_irq(dev_priv, 0xffffffff, ~val);
> -}
> -
>  static bool ivb_can_enable_err_int(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -960,7 +955,7 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv,
>  
>  	spin_lock(&dev_priv->irq_lock);
>  	dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_RPS_EVENTS;
> -	snb_set_pm_irq(dev_priv, dev_priv->rps.pm_iir);
> +	snb_disable_pm_irq(dev_priv, pm_iir & GEN6_PM_RPS_EVENTS);
>  	spin_unlock(&dev_priv->irq_lock);
>  
>  	queue_work(dev_priv->wq, &dev_priv->rps.work);
> @@ -1043,9 +1038,7 @@ static void hsw_pm_irq_handler(struct drm_i915_private *dev_priv,
>  	if (pm_iir & GEN6_PM_RPS_EVENTS) {
>  		spin_lock(&dev_priv->irq_lock);
>  		dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_RPS_EVENTS;
> -		snb_set_pm_irq(dev_priv, dev_priv->rps.pm_iir);
> -		/* never want to mask useful interrupts. */
> -		WARN_ON(I915_READ_NOTRACE(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
> +		snb_disable_pm_irq(dev_priv, pm_iir & GEN6_PM_RPS_EVENTS);
>  		spin_unlock(&dev_priv->irq_lock);
>  
>  		queue_work(dev_priv->wq, &dev_priv->rps.work);
> -- 
> 1.8.1.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH 6.3/9] drm/i915: merge HSW and SNB PM irq handlers

[Paulo Zanoni]

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

Because hsw_pm_irq_handler does exactly what gen6_rps_irq_handler does
and also processes the 2 additional VEBOX bits. So merge those
functions and wrap the VEBOX bits on an IS_HASWELL check. This HSW
check isn't really necessary since the bits are reserved on
SNB/IVB/VLV, but it's a good documentation on who uses them.

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d9ebfb6..8ba5d0a 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -939,28 +939,6 @@ static void snb_gt_irq_handler(struct drm_device *dev,
 		ivybridge_parity_error_irq_handler(dev);
 }
 
-/* Legacy way of handling PM interrupts */
-static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv,
-				 u32 pm_iir)
-{
-	/*
-	 * IIR bits should never already be set because IMR should
-	 * prevent an interrupt from being shown in IIR. The warning
-	 * displays a case where we've unsafely cleared
-	 * dev_priv->rps.pm_iir. Although missing an interrupt of the same
-	 * type is not a problem, it displays a problem in the logic.
-	 *
-	 * The mask bit in IMR is cleared by dev_priv->rps.work.
-	 */
-
-	spin_lock(&dev_priv->irq_lock);
-	dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_RPS_EVENTS;
-	snb_disable_pm_irq(dev_priv, pm_iir & GEN6_PM_RPS_EVENTS);
-	spin_unlock(&dev_priv->irq_lock);
-
-	queue_work(dev_priv->wq, &dev_priv->rps.work);
-}
-
 #define HPD_STORM_DETECT_PERIOD 1000
 #define HPD_STORM_THRESHOLD 5
 
@@ -1027,13 +1005,10 @@ static void dp_aux_irq_handler(struct drm_device *dev)
 	wake_up_all(&dev_priv->gmbus_wait_queue);
 }
 
-/* Unlike gen6_rps_irq_handler() from which this function is originally derived,
- * we must be able to deal with other PM interrupts. This is complicated because
- * of the way in which we use the masks to defer the RPS work (which for
- * posterity is necessary because of forcewake).
- */
-static void hsw_pm_irq_handler(struct drm_i915_private *dev_priv,
-			       u32 pm_iir)
+/* The RPS events need forcewake, so we add them to a work queue and mask their
+ * IMR bits until the work is done. Other interrupts can be processed without
+ * the work queue. */
+static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
 {
 	if (pm_iir & GEN6_PM_RPS_EVENTS) {
 		spin_lock(&dev_priv->irq_lock);
@@ -1044,12 +1019,14 @@ static void hsw_pm_irq_handler(struct drm_i915_private *dev_priv,
 		queue_work(dev_priv->wq, &dev_priv->rps.work);
 	}
 
-	if (pm_iir & PM_VEBOX_USER_INTERRUPT)
-		notify_ring(dev_priv->dev, &dev_priv->ring[VECS]);
+	if (IS_HASWELL(dev_priv->dev)) {
+		if (pm_iir & PM_VEBOX_USER_INTERRUPT)
+			notify_ring(dev_priv->dev, &dev_priv->ring[VECS]);
 
-	if (pm_iir & PM_VEBOX_CS_ERROR_INTERRUPT) {
-		DRM_ERROR("VEBOX CS error interrupt 0x%08x\n", pm_iir);
-		i915_handle_error(dev_priv->dev, false);
+		if (pm_iir & PM_VEBOX_CS_ERROR_INTERRUPT) {
+			DRM_ERROR("VEBOX CS error interrupt 0x%08x\n", pm_iir);
+			i915_handle_error(dev_priv->dev, false);
+		}
 	}
 }
 
@@ -1424,10 +1401,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 	if (INTEL_INFO(dev)->gen >= 6) {
 		u32 pm_iir = I915_READ(GEN6_PMIIR);
 		if (pm_iir) {
-			if (IS_HASWELL(dev))
-				hsw_pm_irq_handler(dev_priv, pm_iir);
-			else
-				gen6_rps_irq_handler(dev_priv, pm_iir);
+			gen6_rps_irq_handler(dev_priv, pm_iir);
 			I915_WRITE(GEN6_PMIIR, pm_iir);
 			ret = IRQ_HANDLED;
 		}
-- 
1.8.1.2

Re: [PATCH 6.3/9] drm/i915: merge HSW and SNB PM irq handlers

[Ben Widawsky]

On Fri, Aug 09, 2013 at 05:04:37PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Because hsw_pm_irq_handler does exactly what gen6_rps_irq_handler does
> and also processes the 2 additional VEBOX bits. So merge those
> functions and wrap the VEBOX bits on an IS_HASWELL check. This HSW
> check isn't really necessary since the bits are reserved on
> SNB/IVB/VLV, but it's a good documentation on who uses them.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 50 ++++++++++-------------------------------
>  1 file changed, 12 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index d9ebfb6..8ba5d0a 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -939,28 +939,6 @@ static void snb_gt_irq_handler(struct drm_device *dev,
>  		ivybridge_parity_error_irq_handler(dev);
>  }
>  
> -/* Legacy way of handling PM interrupts */
> -static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv,
> -				 u32 pm_iir)
> -{
> -	/*
> -	 * IIR bits should never already be set because IMR should
> -	 * prevent an interrupt from being shown in IIR. The warning
> -	 * displays a case where we've unsafely cleared
> -	 * dev_priv->rps.pm_iir. Although missing an interrupt of the same
> -	 * type is not a problem, it displays a problem in the logic.
> -	 *
> -	 * The mask bit in IMR is cleared by dev_priv->rps.work.
> -	 */
> -
> -	spin_lock(&dev_priv->irq_lock);
> -	dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_RPS_EVENTS;
> -	snb_disable_pm_irq(dev_priv, pm_iir & GEN6_PM_RPS_EVENTS);
> -	spin_unlock(&dev_priv->irq_lock);
> -
> -	queue_work(dev_priv->wq, &dev_priv->rps.work);
> -}
> -
>  #define HPD_STORM_DETECT_PERIOD 1000
>  #define HPD_STORM_THRESHOLD 5
>  
> @@ -1027,13 +1005,10 @@ static void dp_aux_irq_handler(struct drm_device *dev)
>  	wake_up_all(&dev_priv->gmbus_wait_queue);
>  }
>  
> -/* Unlike gen6_rps_irq_handler() from which this function is originally derived,
> - * we must be able to deal with other PM interrupts. This is complicated because
> - * of the way in which we use the masks to defer the RPS work (which for
> - * posterity is necessary because of forcewake).
> - */
> -static void hsw_pm_irq_handler(struct drm_i915_private *dev_priv,
> -			       u32 pm_iir)
> +/* The RPS events need forcewake, so we add them to a work queue and mask their
> + * IMR bits until the work is done. Other interrupts can be processed without
> + * the work queue. */
> +static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
>  {
>  	if (pm_iir & GEN6_PM_RPS_EVENTS) {
>  		spin_lock(&dev_priv->irq_lock);
> @@ -1044,12 +1019,14 @@ static void hsw_pm_irq_handler(struct drm_i915_private *dev_priv,
>  		queue_work(dev_priv->wq, &dev_priv->rps.work);
>  	}
>  
> -	if (pm_iir & PM_VEBOX_USER_INTERRUPT)
> -		notify_ring(dev_priv->dev, &dev_priv->ring[VECS]);
> +	if (IS_HASWELL(dev_priv->dev)) {
> +		if (pm_iir & PM_VEBOX_USER_INTERRUPT)
> +			notify_ring(dev_priv->dev, &dev_priv->ring[VECS]);

Make this HAS_VEBOX() instead of IS_HASWELL()
>  
> -	if (pm_iir & PM_VEBOX_CS_ERROR_INTERRUPT) {
> -		DRM_ERROR("VEBOX CS error interrupt 0x%08x\n", pm_iir);
> -		i915_handle_error(dev_priv->dev, false);
> +		if (pm_iir & PM_VEBOX_CS_ERROR_INTERRUPT) {
> +			DRM_ERROR("VEBOX CS error interrupt 0x%08x\n", pm_iir);
> +			i915_handle_error(dev_priv->dev, false);
> +		}
>  	}
>  }
>  
> @@ -1424,10 +1401,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  	if (INTEL_INFO(dev)->gen >= 6) {
>  		u32 pm_iir = I915_READ(GEN6_PMIIR);
>  		if (pm_iir) {
> -			if (IS_HASWELL(dev))
> -				hsw_pm_irq_handler(dev_priv, pm_iir);
> -			else
> -				gen6_rps_irq_handler(dev_priv, pm_iir);
> +			gen6_rps_irq_handler(dev_priv, pm_iir);
>  			I915_WRITE(GEN6_PMIIR, pm_iir);
>  			ret = IRQ_HANDLED;
>  		}

With the couple of small comments, these 3 patches are:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

Though after the IRC discussion you may want to discount my review.

-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH 6.3/9] drm/i915: merge HSW and SNB PM irq handlers

[Paulo Zanoni]

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

Because hsw_pm_irq_handler does exactly what gen6_rps_irq_handler does
and also processes the 2 additional VEBOX bits. So merge those
functions and wrap the VEBOX bits on a HAS_VEBOX check. This
check isn't really necessary since the bits are reserved on
SNB/IVB/VLV, but it's a good documentation on who uses them.

v2: - Change IS_HASWELL check to HAS_VEBOX

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_irq.c | 50 ++++++++++-------------------------------
 1 file changed, 12 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index e0c6f7d..caf83da 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -942,28 +942,6 @@ static void snb_gt_irq_handler(struct drm_device *dev,
 		ivybridge_parity_error_irq_handler(dev);
 }
 
-/* Legacy way of handling PM interrupts */
-static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv,
-				 u32 pm_iir)
-{
-	/*
-	 * IIR bits should never already be set because IMR should
-	 * prevent an interrupt from being shown in IIR. The warning
-	 * displays a case where we've unsafely cleared
-	 * dev_priv->rps.pm_iir. Although missing an interrupt of the same
-	 * type is not a problem, it displays a problem in the logic.
-	 *
-	 * The mask bit in IMR is cleared by dev_priv->rps.work.
-	 */
-
-	spin_lock(&dev_priv->irq_lock);
-	dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_RPS_EVENTS;
-	snb_disable_pm_irq(dev_priv, pm_iir & GEN6_PM_RPS_EVENTS);
-	spin_unlock(&dev_priv->irq_lock);
-
-	queue_work(dev_priv->wq, &dev_priv->rps.work);
-}
-
 #define HPD_STORM_DETECT_PERIOD 1000
 #define HPD_STORM_THRESHOLD 5
 
@@ -1030,13 +1008,10 @@ static void dp_aux_irq_handler(struct drm_device *dev)
 	wake_up_all(&dev_priv->gmbus_wait_queue);
 }
 
-/* Unlike gen6_rps_irq_handler() from which this function is originally derived,
- * we must be able to deal with other PM interrupts. This is complicated because
- * of the way in which we use the masks to defer the RPS work (which for
- * posterity is necessary because of forcewake).
- */
-static void hsw_pm_irq_handler(struct drm_i915_private *dev_priv,
-			       u32 pm_iir)
+/* The RPS events need forcewake, so we add them to a work queue and mask their
+ * IMR bits until the work is done. Other interrupts can be processed without
+ * the work queue. */
+static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
 {
 	if (pm_iir & GEN6_PM_RPS_EVENTS) {
 		spin_lock(&dev_priv->irq_lock);
@@ -1047,12 +1022,14 @@ static void hsw_pm_irq_handler(struct drm_i915_private *dev_priv,
 		queue_work(dev_priv->wq, &dev_priv->rps.work);
 	}
 
-	if (pm_iir & PM_VEBOX_USER_INTERRUPT)
-		notify_ring(dev_priv->dev, &dev_priv->ring[VECS]);
+	if (HAS_VEBOX(dev_priv->dev)) {
+		if (pm_iir & PM_VEBOX_USER_INTERRUPT)
+			notify_ring(dev_priv->dev, &dev_priv->ring[VECS]);
 
-	if (pm_iir & PM_VEBOX_CS_ERROR_INTERRUPT) {
-		DRM_ERROR("VEBOX CS error interrupt 0x%08x\n", pm_iir);
-		i915_handle_error(dev_priv->dev, false);
+		if (pm_iir & PM_VEBOX_CS_ERROR_INTERRUPT) {
+			DRM_ERROR("VEBOX CS error interrupt 0x%08x\n", pm_iir);
+			i915_handle_error(dev_priv->dev, false);
+		}
 	}
 }
 
@@ -1427,10 +1404,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 	if (INTEL_INFO(dev)->gen >= 6) {
 		u32 pm_iir = I915_READ(GEN6_PMIIR);
 		if (pm_iir) {
-			if (IS_HASWELL(dev))
-				hsw_pm_irq_handler(dev_priv, pm_iir);
-			else
-				gen6_rps_irq_handler(dev_priv, pm_iir);
+			gen6_rps_irq_handler(dev_priv, pm_iir);
 			I915_WRITE(GEN6_PMIIR, pm_iir);
 			ret = IRQ_HANDLED;
 		}
-- 
1.8.1.2

Re: [PATCH 6.3/9] drm/i915: merge HSW and SNB PM irq handlers

[Daniel Vetter]

On Thu, Aug 15, 2013 at 11:51:32AM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Because hsw_pm_irq_handler does exactly what gen6_rps_irq_handler does
> and also processes the 2 additional VEBOX bits. So merge those
> functions and wrap the VEBOX bits on a HAS_VEBOX check. This
> check isn't really necessary since the bits are reserved on
> SNB/IVB/VLV, but it's a good documentation on who uses them.
> 
> v2: - Change IS_HASWELL check to HAS_VEBOX
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

Ok, I've merged everything up to this patch. Please double check that I
didn't botch the job and merge an older version of a revised patch ;-)

Thanks for patches & review.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 50 ++++++++++-------------------------------
>  1 file changed, 12 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index e0c6f7d..caf83da 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -942,28 +942,6 @@ static void snb_gt_irq_handler(struct drm_device *dev,
>  		ivybridge_parity_error_irq_handler(dev);
>  }
>  
> -/* Legacy way of handling PM interrupts */
> -static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv,
> -				 u32 pm_iir)
> -{
> -	/*
> -	 * IIR bits should never already be set because IMR should
> -	 * prevent an interrupt from being shown in IIR. The warning
> -	 * displays a case where we've unsafely cleared
> -	 * dev_priv->rps.pm_iir. Although missing an interrupt of the same
> -	 * type is not a problem, it displays a problem in the logic.
> -	 *
> -	 * The mask bit in IMR is cleared by dev_priv->rps.work.
> -	 */
> -
> -	spin_lock(&dev_priv->irq_lock);
> -	dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_RPS_EVENTS;
> -	snb_disable_pm_irq(dev_priv, pm_iir & GEN6_PM_RPS_EVENTS);
> -	spin_unlock(&dev_priv->irq_lock);
> -
> -	queue_work(dev_priv->wq, &dev_priv->rps.work);
> -}
> -
>  #define HPD_STORM_DETECT_PERIOD 1000
>  #define HPD_STORM_THRESHOLD 5
>  
> @@ -1030,13 +1008,10 @@ static void dp_aux_irq_handler(struct drm_device *dev)
>  	wake_up_all(&dev_priv->gmbus_wait_queue);
>  }
>  
> -/* Unlike gen6_rps_irq_handler() from which this function is originally derived,
> - * we must be able to deal with other PM interrupts. This is complicated because
> - * of the way in which we use the masks to defer the RPS work (which for
> - * posterity is necessary because of forcewake).
> - */
> -static void hsw_pm_irq_handler(struct drm_i915_private *dev_priv,
> -			       u32 pm_iir)
> +/* The RPS events need forcewake, so we add them to a work queue and mask their
> + * IMR bits until the work is done. Other interrupts can be processed without
> + * the work queue. */
> +static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
>  {
>  	if (pm_iir & GEN6_PM_RPS_EVENTS) {
>  		spin_lock(&dev_priv->irq_lock);
> @@ -1047,12 +1022,14 @@ static void hsw_pm_irq_handler(struct drm_i915_private *dev_priv,
>  		queue_work(dev_priv->wq, &dev_priv->rps.work);
>  	}
>  
> -	if (pm_iir & PM_VEBOX_USER_INTERRUPT)
> -		notify_ring(dev_priv->dev, &dev_priv->ring[VECS]);
> +	if (HAS_VEBOX(dev_priv->dev)) {
> +		if (pm_iir & PM_VEBOX_USER_INTERRUPT)
> +			notify_ring(dev_priv->dev, &dev_priv->ring[VECS]);
>  
> -	if (pm_iir & PM_VEBOX_CS_ERROR_INTERRUPT) {
> -		DRM_ERROR("VEBOX CS error interrupt 0x%08x\n", pm_iir);
> -		i915_handle_error(dev_priv->dev, false);
> +		if (pm_iir & PM_VEBOX_CS_ERROR_INTERRUPT) {
> +			DRM_ERROR("VEBOX CS error interrupt 0x%08x\n", pm_iir);
> +			i915_handle_error(dev_priv->dev, false);
> +		}
>  	}
>  }
>  
> @@ -1427,10 +1404,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  	if (INTEL_INFO(dev)->gen >= 6) {
>  		u32 pm_iir = I915_READ(GEN6_PMIIR);
>  		if (pm_iir) {
> -			if (IS_HASWELL(dev))
> -				hsw_pm_irq_handler(dev_priv, pm_iir);
> -			else
> -				gen6_rps_irq_handler(dev_priv, pm_iir);
> +			gen6_rps_irq_handler(dev_priv, pm_iir);
>  			I915_WRITE(GEN6_PMIIR, pm_iir);
>  			ret = IRQ_HANDLED;
>  		}
> -- 
> 1.8.1.2
> 
> _______________________________________________
> 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 6.1/9] drm/i915: don't queue PM events we won't process

[Ben Widawsky]

On Fri, Aug 09, 2013 at 05:04:35PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> On SNB/IVB/VLV we only call gen6_rps_irq_handler if one of the IIR
> bits set is part of GEN6_PM_RPS_EVENTS, but at gen6_rps_irq_handler we
> add all the enabled IIR bits to the work queue, not only the ones that
> are part of GEN6_PM_RPS_EVENTS. But then gen6_pm_rps_work only
> processes GEN6_PM_RPS_EVENTS, so it's useless to add anything that's
> not GEN6_PM_RPS_EVENTS to the work queue.
> 
> As a bonus, gen6_rps_irq_handler looks more similar to
> hsw_pm_irq_handler, so we may be able to merge them in the future.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 0f46d33..5b51c43 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -959,7 +959,7 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv,
>  	 */
>  
>  	spin_lock(&dev_priv->irq_lock);
> -	dev_priv->rps.pm_iir |= pm_iir;
> +	dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_RPS_EVENTS;
>  	snb_set_pm_irq(dev_priv, dev_priv->rps.pm_iir);
>  	spin_unlock(&dev_priv->irq_lock);
>  
> @@ -1128,7 +1128,7 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>  		if (pipe_stats[0] & PIPE_GMBUS_INTERRUPT_STATUS)
>  			gmbus_irq_handler(dev);
>  
> -		if (pm_iir & GEN6_PM_RPS_EVENTS)
> +		if (pm_iir)
>  			gen6_rps_irq_handler(dev_priv, pm_iir);
>  
>  		I915_WRITE(GTIIR, gt_iir);
> @@ -1433,7 +1433,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  		if (pm_iir) {
>  			if (IS_HASWELL(dev))
>  				hsw_pm_irq_handler(dev_priv, pm_iir);
> -			else if (pm_iir & GEN6_PM_RPS_EVENTS)
> +			else
>  				gen6_rps_irq_handler(dev_priv, pm_iir);
>  			I915_WRITE(GEN6_PMIIR, pm_iir);
>  			ret = IRQ_HANDLED;

Can you please add WARN_ON(pm_iir & ~GEN6_PM_RPS_EVENTS) somewhere in
the code path to make me happy?

Otherwise it's:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH 6.1/9] drm/i915: don't queue PM events we won't process

[Paulo Zanoni]

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

On SNB/IVB/VLV we only call gen6_rps_irq_handler if one of the IIR
bits set is part of GEN6_PM_RPS_EVENTS, but at gen6_rps_irq_handler we
add all the enabled IIR bits to the work queue, not only the ones that
are part of GEN6_PM_RPS_EVENTS. But then gen6_pm_rps_work only
processes GEN6_PM_RPS_EVENTS, so it's useless to add anything that's
not GEN6_PM_RPS_EVENTS to the work queue.

As a bonus, gen6_rps_irq_handler looks more similar to
hsw_pm_irq_handler, so we may be able to merge them in the future.

v2: - Add a WARN in case we queued something we're not going to
      process.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net> (v1)
---
 drivers/gpu/drm/i915/i915_irq.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 976113a..c10d2f1 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -789,6 +789,9 @@ static void gen6_pm_rps_work(struct work_struct *work)
 	snb_enable_pm_irq(dev_priv, GEN6_PM_RPS_EVENTS);
 	spin_unlock_irq(&dev_priv->irq_lock);
 
+	/* Make sure we didn't queue anything we're not going to process. */
+	WARN_ON(pm_iir & ~GEN6_PM_RPS_EVENTS);
+
 	if ((pm_iir & GEN6_PM_RPS_EVENTS) == 0)
 		return;
 
@@ -959,7 +962,7 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv,
 	 */
 
 	spin_lock(&dev_priv->irq_lock);
-	dev_priv->rps.pm_iir |= pm_iir;
+	dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_RPS_EVENTS;
 	snb_set_pm_irq(dev_priv, dev_priv->rps.pm_iir);
 	spin_unlock(&dev_priv->irq_lock);
 
@@ -1128,7 +1131,7 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
 		if (pipe_stats[0] & PIPE_GMBUS_INTERRUPT_STATUS)
 			gmbus_irq_handler(dev);
 
-		if (pm_iir & GEN6_PM_RPS_EVENTS)
+		if (pm_iir)
 			gen6_rps_irq_handler(dev_priv, pm_iir);
 
 		I915_WRITE(GTIIR, gt_iir);
@@ -1433,7 +1436,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 		if (pm_iir) {
 			if (IS_HASWELL(dev))
 				hsw_pm_irq_handler(dev_priv, pm_iir);
-			else if (pm_iir & GEN6_PM_RPS_EVENTS)
+			else
 				gen6_rps_irq_handler(dev_priv, pm_iir);
 			I915_WRITE(GEN6_PMIIR, pm_iir);
 			ret = IRQ_HANDLED;
-- 
1.8.1.2