Haswell Package C8+
Subsystems (2)
cover Haswell Package C8+
From: Paulo Zanoni <[email protected]>
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(-)
Signature · 1 line
1.8.1.2
PATCH 1/9 drm/i915: add the FCLK case to intel_ddi_get_cdclk_freq
From: Paulo Zanoni <[email protected]>
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+.
--- drivers/gpu/drm/i915/intel_ddi.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/gpu/drm/i915/intel_ddi.c +6 -3
@@ -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 ;
Signature · 1 line
1.8.1.2
Re: [PATCH 1/9] drm/i915: add the FCLK case to intel_ddi_get_cdclk_freq
Thanks for pointing me the doc that explains why 800 MHz when using FCLK input. ;)
Quoted 39 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
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 <[email protected]>--- drivers/gpu/drm/i915/intel_ddi.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)drivers/gpu/drm/i915/intel_ddi.c +6
-3@@ -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 ;Signature · 6 lines
1.8.1.2 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 2/9 drm/i915: wrap GTIMR changes
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
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.
--- 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(-)
drivers/gpu/drm/i915/i915_irq.c +30 -4
@@ -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);
drivers/gpu/drm/i915/intel_drv.h +3 -0
@@ -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__ */
drivers/gpu/drm/i915/intel_ringbuffer.c +6 -16
@@ -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);
Signature · 1 line
1.8.1.2
Re: [PATCH 2/9] drm/i915: wrap GTIMR changes
Quoted 27 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
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 insidei915_irq.c.The implementation is based on
ibx_display_interrupt_update.Signed-off-by Paulo Zanoni <[email protected]>--- 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(-)drivers/gpu/drm/i915/i915_irq.c +4
-0@@ -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
drivers/gpu/drm/i915/i915_irq.c ironlake_disable_display_irq(drm_i915_private_t *dev_priv, u32 mask) · line 120
Quoted 10 lines
drivers/gpu/drm/i915/i915_irq.c +10
-0+* @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 ;)
Quoted 111 lines
drivers/gpu/drm/i915/i915_irq.c +16
-4+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);drivers/gpu/drm/i915/intel_drv.h +3
-0@@ -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__ */ drivers/gpu/drm/i915/intel_ringbuffer.c +6
-16@@ -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);Signature · 6 lines
1.8.1.2 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 2/9] drm/i915: wrap GTIMR changes
2013/8/14 Rodrigo Vivi <[email protected]>
Quoted 27 lines
Reviewed-by Rodrigo Vivi <[email protected]>Quoted 23 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
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 insidei915_irq.c.The implementation is based on
ibx_display_interrupt_update.Signed-off-by Paulo Zanoni <[email protected]>--- 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(-)drivers/gpu/drm/i915/i915_irq.c +0
-0@@ -104,6 +104,34 @@ ironlake_disable_display_irq(drm_i915_private_t *dev_priv, u32 mask) } }
Quoted 16 lines
Quoted 4 lines
drivers/gpu/drm/i915/i915_irq.c +4
-0+/** +* ilk_update_gt_irq - update GTIMR +* @dev_priv: driver private +* @interrupt_mask: mask of interrupt bits to update drivers/gpu/drm/i915/i915_irq.c ironlake_disable_display_irq(drm_i915_private_t *dev_priv, u32 mask) · line 120
Quoted 10 lines
drivers/gpu/drm/i915/i915_irq.c +10
-0+* @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 :)
Quoted 113 lines
Quoted 113 lines
drivers/gpu/drm/i915/i915_irq.c +16
-4+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);drivers/gpu/drm/i915/intel_drv.h +3
-0@@ -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__ */ drivers/gpu/drm/i915/intel_ringbuffer.c +6
-17@@ -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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 1 line
Paulo Zanoni
PATCH 3/9 drm/i915: wrap GEN6_PMIMR changes
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
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.
--- 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(-)
drivers/gpu/drm/i915/i915_irq.c +40 -7
@@ -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);
drivers/gpu/drm/i915/intel_drv.h +3 -0
@@ -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__ */
drivers/gpu/drm/i915/intel_pm.c +1 -1
@@ -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 */
drivers/gpu/drm/i915/intel_ringbuffer.c +2 -6
@@ -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); }
Signature · 1 line
1.8.1.2
Re: [PATCH 3/9] drm/i915: wrap GEN6_PMIMR changes
Quoted 46 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
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 <[email protected]>--- 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(-)drivers/gpu/drm/i915/i915_irq.c +23
-0@@ -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);
drivers/gpu/drm/i915/i915_irq.c void ilk_disable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask) · line 167
Quoted 10 lines
drivers/gpu/drm/i915/i915_irq.c +10
-0+} + +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!
Quoted 105 lines
drivers/gpu/drm/i915/i915_irq.c +7
-7+} +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);drivers/gpu/drm/i915/intel_drv.h +3
-0@@ -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__ */ drivers/gpu/drm/i915/intel_pm.c +1
-1@@ -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 */ drivers/gpu/drm/i915/intel_ringbuffer.c +2
-6@@ -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); }Signature · 6 lines
1.8.1.2 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 3/9] drm/i915: wrap GEN6_PMIMR changes
2013/8/14 Rodrigo Vivi <[email protected]>:
Quoted 53 lines
Reviewed-by Rodrigo Vivi <[email protected]>Quoted 50 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
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 <[email protected]>--- 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(-)drivers/gpu/drm/i915/i915_irq.c +27
-0@@ -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) +{
Quoted 8 lines
drivers/gpu/drm/i915/i915_irq.c void ilk_disable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask) · line 167
Quoted 6 lines
drivers/gpu/drm/i915/i915_irq.c +6
-0+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 :)
Quoted 106 lines
Quoted 106 lines
drivers/gpu/drm/i915/i915_irq.c +7
-7+} +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);drivers/gpu/drm/i915/intel_drv.h +3
-0@@ -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__ */ drivers/gpu/drm/i915/intel_pm.c +1
-1@@ -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 */ drivers/gpu/drm/i915/intel_ringbuffer.c +2
-7@@ -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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 1 line
Paulo Zanoni
PATCH 4/9 drm/i915: don't update GEN6_PMIMR when it's not needed
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
I did some brief tests and the "new_val = pmimr" condition usually happens a few times after exiting games.
--- drivers/gpu/drm/i915/i915_irq.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +9 -5
@@ -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)
Signature · 1 line
1.8.1.2
Re: [PATCH 4/9] drm/i915: don't update GEN6_PMIMR when it's not needed
Quoted 6 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
I did some brief tests and the "
new_val= pmimr" condition usually happens a few times after exiting games.Signed-off-by Paulo Zanoni <[email protected]>
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
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
Re: [PATCH 4/9] drm/i915: don't update GEN6_PMIMR when it's not needed
2013/8/6 Chris Wilson <[email protected]>:
Quoted 11 lines · Paulo Zanoni
Quoted 6 lines
From: Paulo Zanoni <[email protected]>
I did some brief tests and the "
new_val= pmimr" condition usually happens a few times after exiting games.Signed-off-by Paulo Zanoni <[email protected]>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
Quoted 4 lines
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
Signature · 1 line
Paulo Zanoni
Re: [PATCH 4/9] drm/i915: don't update GEN6_PMIMR when it's not needed
Quoted 9 lines · Paulo Zanoni
2013/8/6 Chris Wilson <[email protected]>:
Quoted 8 lines · Paulo Zanoni
Quoted 6 lines
From: Paulo Zanoni <[email protected]>
I did some brief tests and the "
new_val= pmimr" condition usually happens a few times after exiting games.Signed-off-by Paulo Zanoni <[email protected]>
Quoted 8 lines
Quoted 3 lines
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.
Quoted 2 lines
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
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
Re: [PATCH 4/9] drm/i915: don't update GEN6_PMIMR when it's not needed
Quoted 19 lines · Chris Wilson
Quoted 9 lines · Paulo Zanoni
2013/8/6 Chris Wilson <[email protected]>:
Quoted 8 lines · Paulo Zanoni
Quoted 6 lines
From: Paulo Zanoni <[email protected]>
I did some brief tests and the "
new_val= pmimr" condition usually happens a few times after exiting games.Signed-off-by Paulo Zanoni <[email protected]>Quoted 8 lines
Quoted 3 lines
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.
Quoted 5 lines
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
Signature · 3 lines
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
In reply to Paulo ZanoniI liked this very much... we should do this kind of check in more places...
Quoted 45 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
I did some brief tests and the "
new_val= pmimr" condition usually happens a few times after exiting games.Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_irq.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)drivers/gpu/drm/i915/i915_irq.c +9
-5@@ -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)Signature · 6 lines
1.8.1.2 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 5/9 drm/i915: add dev_priv->pm_irq_mask
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
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.
--- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_irq.c | 12 +++++++----- 2 files changed, 8 insertions(+), 5 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +1 -0
@@ -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;
drivers/gpu/drm/i915/i915_irq.c +7 -5
@@ -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 ); }
Signature · 1 line
1.8.1.2
Re: [PATCH 5/9] drm/i915: add dev_priv->pm_irq_mask
Quoted 51 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
Just like
irq_maskandgt_irq_mask, use it to track the status ofGEN6_PMIMRso we don't need to read it again every time we callsnb_update_pm_irq.Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_irq.c | 12 +++++++----- 2 files changed, 8 insertions(+), 5 deletions(-)drivers/gpu/drm/i915/i915_drv.h +1
-0@@ -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;drivers/gpu/drm/i915/i915_irq.c +5
-4@@ -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 ); } }
drivers/gpu/drm/i915/i915_irq.c static void gen5_gt_irq_postinstall(struct drm_device *dev) · line 2225
Quoted 6 lines
drivers/gpu/drm/i915/i915_irq.c +1
-1@@ -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...
Quoted 6 lines
drivers/gpu/drm/i915/i915_irq.c +1
-0+I915_WRITE (GEN6_PMIMR , dev_priv->pm_irq_mask);I915_WRITE (GEN6_PMIER , pm_irqs);POSTING_READ (GEN6_PMIER ); }Signature · 1 line
1.8.1.2
Anyways, feel free to use:
Quoted 5 lines
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 5/9] drm/i915: add dev_priv->pm_irq_mask
2013/8/14 Rodrigo Vivi <[email protected]>:
Quoted 55 lines · Paulo Zanoni
Quoted 54 lines
From: Paulo Zanoni <[email protected]>
Just like
irq_maskandgt_irq_mask, use it to track the status ofGEN6_PMIMRso we don't need to read it again every time we callsnb_update_pm_irq.Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_irq.c | 12 +++++++----- 2 files changed, 8 insertions(+), 5 deletions(-)drivers/gpu/drm/i915/i915_drv.h +1
-0@@ -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;drivers/gpu/drm/i915/i915_irq.c +5
-4@@ -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 ;
Quoted 6 lines
drivers/gpu/drm/i915/i915_irq.c static void gen5_gt_irq_postinstall(struct drm_device *dev) · line 2225
Quoted 3 lines
drivers/gpu/drm/i915/i915_irq.c +1
-1+ 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 agen5_func using a GEN6 reg, but maybe we have to use this samepm_irq_maskthere 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.
Quoted 11 lines
Quoted 8 lines
drivers/gpu/drm/i915/i915_irq.c +1
-1+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 <[email protected]>
Thanks for the reviews!
Quoted 6 lines
Quoted 6 lines
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 1 line
Paulo Zanoni
PATCH 6/9 drm/i915: don't disable/reenable IVB error interrupts when not needed
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
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)
--- drivers/gpu/drm/i915/i915_irq.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +5 -2
@@ -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 );
Signature · 1 line
1.8.1.2
Re: [PATCH 6/9] drm/i915: don't disable/reenable IVB error interrupts when not needed
Quoted 53 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
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 <[email protected]>--- drivers/gpu/drm/i915/i915_irq.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)drivers/gpu/drm/i915/i915_irq.c +5
-2@@ -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 );Signature · 6 lines
1.8.1.2 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 6/9] drm/i915: don't disable/reenable IVB error interrupts when not needed
In reply to Paulo ZanoniQuoted 27 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
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 <[email protected]>--- drivers/gpu/drm/i915/i915_irq.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)drivers/gpu/drm/i915/i915_irq.c +1
-0@@ -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);
drivers/gpu/drm/i915/i915_irq.c static irqreturn_t ironlake_irq_handler(int irq, void *arg) · line 1407
Quoted 7 lines
drivers/gpu/drm/i915/i915_irq.c +3
-1@@ -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
Quoted 19 lines
drivers/gpu/drm/i915/i915_irq.c +1
-1spin_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 );Signature · 6 lines
1.8.1.2 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 3 lines
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
2013/8/20 Daniel Vetter <[email protected]>:
Quoted 27 lines · Paulo Zanoni
Quoted 26 lines
From: Paulo Zanoni <[email protected]>
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 <[email protected]>--- drivers/gpu/drm/i915/i915_irq.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)drivers/gpu/drm/i915/i915_irq.c +1
-0@@ -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);
Quoted 13 lines
drivers/gpu/drm/i915/i915_irq.c static irqreturn_t ironlake_irq_handler(int irq, void *arg) · line 1407
Quoted 8 lines
drivers/gpu/drm/i915/i915_irq.c +3
-1@@ -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.
Quoted 30 lines
Maybe I'm just engaging in wishful thinking here a bit ;-)
Patch applied.
-DanielQuoted 15 lines
drivers/gpu/drm/i915/i915_irq.c +1
-2spin_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
Quoted 4 lines
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
Signature · 1 line
Paulo Zanoni
Re: [PATCH 6/9] drm/i915: don't disable/reenable IVB error interrupts when not needed
Quoted 42 lines · Paulo Zanoni
2013/8/20 Daniel Vetter <[email protected]>:
Quoted 27 lines · Paulo Zanoni
Quoted 26 lines
From: Paulo Zanoni <[email protected]>
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 <[email protected]>--- drivers/gpu/drm/i915/i915_irq.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)drivers/gpu/drm/i915/i915_irq.c +1
-0@@ -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);Quoted 13 lines
drivers/gpu/drm/i915/i915_irq.c static irqreturn_t ironlake_irq_handler(int irq, void *arg) · line 1407
Quoted 8 lines
drivers/gpu/drm/i915/i915_irq.c +3
-1@@ -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?
Quoted 2 lines
Can you please explain more? We still check
ivb_can_enable_err_intbefore 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
Signature · 3 lines
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
2013/8/20 Daniel Vetter <[email protected]>:
Quoted 57 lines · Paulo Zanoni
Quoted 42 lines
2013/8/20 Daniel Vetter <[email protected]>:
Quoted 27 lines · Paulo Zanoni
Quoted 26 lines
From: Paulo Zanoni <[email protected]>
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 <[email protected]>--- drivers/gpu/drm/i915/i915_irq.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)drivers/gpu/drm/i915/i915_irq.c +1
-0@@ -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);Quoted 13 lines
drivers/gpu/drm/i915/i915_irq.c static irqreturn_t ironlake_irq_handler(int irq, void *arg) · line 1407
Quoted 8 lines
drivers/gpu/drm/i915/i915_irq.c +3
-1@@ -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?
Quoted 2 lines
Can you please explain more? We still check
ivb_can_enable_err_intbefore 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 modesetstart interrupt handler,
check that DERRR is off,
do nothing
Quoted 5 lines
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.
Quoted 6 lines
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
Signature · 1 line
Paulo Zanoni
PATCH 7/9 drm/i915: allow package C8+ states on Haswell (disabled)
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
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
--- 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(+)
drivers/gpu/drm/i915/i915_dma.c +10 -0
@@ -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);
drivers/gpu/drm/i915/i915_drv.c +11 -0
@@ -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);
drivers/gpu/drm/i915/i915_drv.h +72 -0
90 lines not on this page · raw message
drivers/gpu/drm/i915/i915_irq.c +106 -0
139 lines not on this page · raw message
drivers/gpu/drm/i915/intel_display.c +164 -0
184 lines not on this page · raw message
drivers/gpu/drm/i915/intel_dp.c +3 -0
@@ -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; }
drivers/gpu/drm/i915/intel_drv.h +8 -0
@@ -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__ */
drivers/gpu/drm/i915/intel_i2c.c +2 -0
@@ -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; }
drivers/gpu/drm/i915/intel_pm.c +11 -0
@@ -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) {
drivers/gpu/drm/i915/intel_ringbuffer.c +2 -0
@@ -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 ;
Signature · 1 line
1.8.1.2
Re: [PATCH 7/9] drm/i915: allow package C8+ states on Haswell (disabled)
drivers/gpu/drm/i915/intel_ringbuffer.c void intel_ring_advance(struct intel_ring_buffer *ring) · line 1594
Quoted 10 lines · Paulo Zanoni
drivers/gpu/drm/i915/intel_ringbuffer.c +2
-0@@ -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
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
Re: [PATCH 7/9] drm/i915: allow package C8+ states on Haswell (disabled)
2013/8/6 Chris Wilson <[email protected]>:
Quoted 9 lines · Paulo Zanoni
Quoted 8 lines
drivers/gpu/drm/i915/intel_ringbuffer.c +0
-0@@ -1590,6 +1590,8 @@ void intel_ring_advance(struct intel_ring_buffer *ring) {struct drm_i915_private *dev_priv = ring->dev->dev_private;
Quoted 6 lines
drivers/gpu/drm/i915/intel_ringbuffer.c void intel_ring_advance(struct intel_ring_buffer *ring) · line 1594
Quoted 2 lines
drivers/gpu/drm/i915/intel_ringbuffer.c +2
-0+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.
Quoted 5 lines
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_ERRORsshould 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.
Quoted 4 lines
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
Signature · 1 line
Paulo Zanoni
Re: [PATCH 7/9] drm/i915: allow package C8+ states on Haswell (disabled)
Quoted 17 lines · Paulo Zanoni
2013/8/6 Chris Wilson <[email protected]>:
Quoted 9 lines · Paulo Zanoni
Quoted 8 lines
drivers/gpu/drm/i915/intel_ringbuffer.c +0
-0@@ -1590,6 +1590,8 @@ void intel_ring_advance(struct intel_ring_buffer *ring) {struct drm_i915_private *dev_priv = ring->dev->dev_private;Quoted 6 lines
drivers/gpu/drm/i915/intel_ringbuffer.c void intel_ring_advance(struct intel_ring_buffer *ring) · line 1594
Quoted 2 lines
drivers/gpu/drm/i915/intel_ringbuffer.c +2
-0+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...
Quoted 4 lines
You're suggesting me to use
intel_mark_busy? I have to admit I saw it, but I noticedintel_mark_busyis called afterintel_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
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
Re: [PATCH 7/9] drm/i915: allow package C8+ states on Haswell (disabled)
Quoted 23 lines · Chris Wilson
Quoted 17 lines · Paulo Zanoni
2013/8/6 Chris Wilson <[email protected]>:
Quoted 9 lines · Paulo Zanoni
Quoted 8 lines
drivers/gpu/drm/i915/intel_ringbuffer.c +0
-0@@ -1590,6 +1590,8 @@ void intel_ring_advance(struct intel_ring_buffer *ring) {struct drm_i915_private *dev_priv = ring->dev->dev_private;Quoted 6 lines
drivers/gpu/drm/i915/intel_ringbuffer.c void intel_ring_advance(struct intel_ring_buffer *ring) · line 1594
Quoted 2 lines
drivers/gpu/drm/i915/intel_ringbuffer.c +2
-0+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...Quoted 4 lines
You're suggesting me to use
intel_mark_busy? I have to admit I saw it, but I noticedintel_mark_busyis called afterintel_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.
Quoted 7 lines
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 inintel_ring_begin(). Ultimately, though we should be able to squash it into a single operation such that we never submit a command beforeintel_mark_busy(). Hmm, easier in fact just to moveintel_mark_busy()tointel_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
Signature · 3 lines
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)
From: Paulo Zanoni <[email protected]>
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!
--- 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(-)
drivers/gpu/drm/i915/i915_dma.c +10 -0
@@ -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);
drivers/gpu/drm/i915/i915_drv.c +11 -0
@@ -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);
drivers/gpu/drm/i915/i915_drv.h +72 -0
90 lines not on this page · raw message
drivers/gpu/drm/i915/i915_gem.c +2 -0
@@ -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 ;
drivers/gpu/drm/i915/i915_irq.c +106 -0
139 lines not on this page · raw message
drivers/gpu/drm/i915/intel_display.c +173 -1
198 lines not on this page · raw message
drivers/gpu/drm/i915/intel_dp.c +3 -0
@@ -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; }
drivers/gpu/drm/i915/intel_drv.h +7 -0
@@ -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__ */
drivers/gpu/drm/i915/intel_i2c.c +2 -0
@@ -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; }
drivers/gpu/drm/i915/intel_pm.c +11 -0
@@ -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) {
Signature · 1 line
1.8.1.2
Re: [PATCH 7/9] drm/i915: allow package C8+ states on Haswell (disabled)
Quick note...
Review
Quoted 1 line · Paulo Zanoni
(quoted diff) +1
-0+ WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
Preferred form is now lockdep_assert_held(&dev_priv->pc8.lock); -Chris
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
Re: [PATCH 7/9] drm/i915: allow package C8+ states on Haswell (disabled)
2013/8/9 Chris Wilson <[email protected]>:
Quoted 6 lines
Quick note...
Quoted 1 line · Paulo Zanoni
+ 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.
Quoted 4 lines
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
Signature · 1 line
Paulo Zanoni
Re: [PATCH 7/9] drm/i915: allow package C8+ states on Haswell (disabled)
Quoted 6 lines · Paulo Zanoni
2013/8/9 Chris Wilson <[email protected]>:
Quoted 5 lines
Quick note...
Quoted 1 line · Paulo Zanoni
+ WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
Quoted 7 lines
Quoted 1 line
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
Signature · 3 lines
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)
Quoted 15 lines · Daniel Vetter
Quoted 6 lines · Paulo Zanoni
2013/8/9 Chris Wilson <[email protected]>:
Quoted 5 lines
Quick note...
Quoted 1 line · Paulo Zanoni
+ WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
Quoted 7 lines
Quoted 1 line
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.
Quoted 7 lines
Tbh I don't understand really why
lockdep_assert_heldis 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 withoutCONFIG_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
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
Re: [PATCH 7/9] drm/i915: allow package C8+ states on Haswell (disabled)
2013/8/10 Chris Wilson <[email protected]>:
Quoted 24 lines · Daniel Vetter
Quoted 15 lines · Paulo Zanoni
Quoted 6 lines
2013/8/9 Chris Wilson <[email protected]>:
Quoted 5 lines
Quick note...
Quoted 1 line · Paulo Zanoni
+ WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
Quoted 7 lines
Quoted 1 line
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.Quoted 7 lines
Tbh I don't understand really why
lockdep_assert_heldis 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 withoutCONFIG_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 ...
Quoted 4 lines
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.
Quoted 4 lines
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
Signature · 1 line
Paulo Zanoni
Re: [PATCH 7/9] drm/i915: allow package C8+ states on Haswell (disabled)
In reply to Paulo ZanoniQuoted 15 lines · Paulo Zanoni
drivers/gpu/drm/i915/i915_irq.c +15
-0+/* 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 );
drivers/gpu/drm/i915/i915_irq.c void intel_hpd_init(struct drm_device *dev) · line 3181
Quoted 10 lines
drivers/gpu/drm/i915/i915_irq.c +10
-0+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
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
Re: [PATCH 7/9] drm/i915: allow package C8+ states on Haswell (disabled)
2013/8/9 Chris Wilson <[email protected]>:
Quoted 23 lines · Paulo Zanoni
Quoted 22 lines
drivers/gpu/drm/i915/i915_irq.c +22
-0+/* 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" );
Quoted 8 lines
drivers/gpu/drm/i915/i915_irq.c void intel_hpd_init(struct drm_device *dev) · line 3181
Quoted 3 lines
drivers/gpu/drm/i915/i915_irq.c +3
-0+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_lockand 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.
Quoted 2 lines
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.
Quoted 4 lines
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
Signature · 1 line
Paulo Zanoni
PATCH 8/9 drm/i915: add i915_pc8_status debugfs file
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
For now it just prints the value of the forbid_refcount and allowing variables.
v2: Update to recent renames and add the new fields.
--- drivers/gpu/drm/i915/i915_debugfs.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)
drivers/gpu/drm/i915/i915_debugfs.c +25 -0
@@ -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)
Signature · 1 line
1.8.1.2
PATCH 9/9 drm/i915: enable Package C8+ by default
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
This should be working, so enable it by default. Also easy to revert.
v2: Rebase, s/allow/enable/.
--- drivers/gpu/drm/i915/i915_drv.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/i915_drv.c +2 -2
@@ -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 );
Signature · 1 line
1.8.1.2
Quoted 8 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
Hi
Here's my next attempt at the Haswell PC8+ feature.
What's new?
Quoted 13 lines
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
Quoted 68 lines
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_HASWELLchecks, 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,
PauloPaulo Zanoni (9):
drm/i915: add the FCLK case tointel_ddi_get_cdclk_freqdrm/i915: wrap GTIMR changes
drm/i915: wrapGEN6_PMIMRchanges
drm/i915: don't updateGEN6_PMIMRwhen it's not needed drm/i915: adddev_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: addi915_pc8_statusdebugfs file
drm/i915: enable Package C8+ by defaultdrivers/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(-)Signature · 6 lines
1.8.1.2 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
2013/8/6 Daniel Vetter <[email protected]>:
Quoted 28 lines · Paulo Zanoni
Quoted 8 lines
From: Paulo Zanoni <[email protected]>
Hi
Here's my next attempt at the Haswell PC8+ feature.
What's new?
Quoted 13 lines
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_seqnoand actually goes to sleep on the waitqueue.
Quoted 4 lines
If you pimp the your igt testcase in the
test_batchcode with the dummy workload andwait_renderingstuff 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_seqnowould 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.
Quoted 75 lines
-Daniel
Quoted 61 lines
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_HASWELLchecks, 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,
PauloPaulo Zanoni (9):
drm/i915: add the FCLK case tointel_ddi_get_cdclk_freqdrm/i915: wrap GTIMR changes
drm/i915: wrapGEN6_PMIMRchanges
drm/i915: don't updateGEN6_PMIMRwhen it's not needed drm/i915: adddev_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: addi915_pc8_statusdebugfs file
drm/i915: enable Package C8+ by defaultdrivers/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(-)Quoted 7 lines
--
1.8.1.2_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
Signature · 1 line
Paulo Zanoni
PATCH 1/1 drm/i915: don't queue PM events we won't process
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
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.
--- drivers/gpu/drm/i915/i915_irq.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +3 -3
@@ -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 ;
Signature · 1 line
1.8.1.2
PATCH 1/1 drm/i915: fix how we mask PMIMR when adding work to the queue
From: Paulo Zanoni <[email protected]>
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 :)
--- drivers/gpu/drm/i915/i915_irq.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +2 -9
@@ -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);
Signature · 1 line
1.8.1.2
Re: [PATCH 6.2/9] drm/i915: fix how we mask PMIMR when adding work to the queue
Quoted 34 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
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 onlyGEN6_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_irqshould be doing the right thing.With i915.enable_pc8=1 I was getting ocasional "
GEN6_PMIIRis not 0" error messages due to the fact that we unmask those unrelated interrupts but don't enable them.
Quoted 7 lines
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_postinstallor whoever needs the accidentally unmasked interrupts. Or maybe I was just wrong and we need to revert this patch :)Signed-off-by Paulo Zanoni <[email protected]>
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
Quoted 47 lines
--- drivers/gpu/drm/i915/i915_irq.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)drivers/gpu/drm/i915/i915_irq.c +2
-9@@ -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);Signature · 6 lines
1.8.1.2 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
PATCH 1/1 drm/i915: merge HSW and SNB PM irq handlers
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
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.
--- drivers/gpu/drm/i915/i915_irq.c | 50 ++++++++++------------------------------- 1 file changed, 12 insertions(+), 38 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +12 -38
@@ -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 ; }
Signature · 1 line
1.8.1.2
Re: [PATCH 6.3/9] drm/i915: merge HSW and SNB PM irq handlers
Quoted 65 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
Because
hsw_pm_irq_handlerdoes exactly whatgen6_rps_irq_handlerdoes and also processes the 2 additional VEBOX bits. So merge those functions and wrap the VEBOX bits on anIS_HASWELLcheck. 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 <[email protected]>--- drivers/gpu/drm/i915/i915_irq.c | 50 ++++++++++------------------------------- 1 file changed, 12 insertions(+), 38 deletions(-)drivers/gpu/drm/i915/i915_irq.c +4
-29@@ -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);
drivers/gpu/drm/i915/i915_irq.c static void hsw_pm_irq_handler(struct drm_i915_private *dev_priv, · line 1024
Quoted 8 lines
drivers/gpu/drm/i915/i915_irq.c +3
-2@@ -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()
Quoted 13 lines
drivers/gpu/drm/i915/i915_irq.c +4
-3-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 ) {
drivers/gpu/drm/i915/i915_irq.c static irqreturn_t ironlake_irq_handler(int irq, void *arg) · line 1407
Quoted 10 lines
drivers/gpu/drm/i915/i915_irq.c +1
-4u32 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:
Though after the IRC discussion you may want to discount my review.
Signature · 1 line
Ben Widawsky, Intel Open Source Technology Center
PATCH 1/1 drm/i915: merge HSW and SNB PM irq handlers
From: Paulo Zanoni <[email protected]>
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
--- drivers/gpu/drm/i915/i915_irq.c | 50 ++++++++++------------------------------- 1 file changed, 12 insertions(+), 38 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +12 -38
@@ -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 ; }
Signature · 1 line
1.8.1.2
Re: [PATCH 6.3/9] drm/i915: merge HSW and SNB PM irq handlers
Quoted 8 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
Because
hsw_pm_irq_handlerdoes exactly whatgen6_rps_irq_handlerdoes and also processes the 2 additional VEBOX bits. So merge those functions and wrap the VEBOX bits on aHAS_VEBOXcheck. 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.
Quoted 4 lines
v2: - Change
IS_HASWELLcheck toHAS_VEBOXSigned-off-by Paulo Zanoni <[email protected]>Reviewed-by Ben Widawsky <[email protected]>
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
Quoted 94 lines
--- drivers/gpu/drm/i915/i915_irq.c | 50 ++++++++++------------------------------- 1 file changed, 12 insertions(+), 38 deletions(-)drivers/gpu/drm/i915/i915_irq.c +12
-38@@ -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 ; }Signature · 6 lines
1.8.1.2 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 3 lines
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
In reply to Paulo ZanoniQuoted 40 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
On SNB/IVB/VLV we only call
gen6_rps_irq_handlerif one of the IIR bits set is part ofGEN6_PM_RPS_EVENTS, but atgen6_rps_irq_handlerwe add all the enabled IIR bits to the work queue, not only the ones that are part ofGEN6_PM_RPS_EVENTS. But thengen6_pm_rps_workonly processesGEN6_PM_RPS_EVENTS, so it's useless to add anything that's notGEN6_PM_RPS_EVENTSto the work queue.As a bonus,
gen6_rps_irq_handlerlooks more similar tohsw_pm_irq_handler, so we may be able to merge them in the future.Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_irq.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)drivers/gpu/drm/i915/i915_irq.c +2
-2@@ -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);
drivers/gpu/drm/i915/i915_irq.c static irqreturn_t ironlake_irq_handler(int irq, void *arg) · line 1439
Quoted 8 lines
drivers/gpu/drm/i915/i915_irq.c +1
-1@@ -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:
Signature · 1 line
Ben Widawsky, Intel Open Source Technology Center
PATCH 1/1 drm/i915: don't queue PM events we won't process
From: Paulo Zanoni <[email protected]>
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.
--- drivers/gpu/drm/i915/i915_irq.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +6 -3
@@ -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 ;
Signature · 1 line
1.8.1.2