Enable PM Interrupts for BDW
Subsystems (2)
cover Enable PM Interrupts for BDW
From: Deepak S <[email protected]>
Added patch to enable PM Interrupts
Ben Widawsky (1):
drm/i915/bdw: Implement a basic PM interrupt handler
Deepak S (1):
drm/i915: Enable PM Interrupts target via Display Interface.
drivers/gpu/drm/i915/i915_irq.c | 81 +++++++++++++++++++++++++++++++++++++--- drivers/gpu/drm/i915/i915_reg.h | 2 + drivers/gpu/drm/i915/intel_drv.h | 3 +- drivers/gpu/drm/i915/intel_pm.c | 40 +++++++++++++++++++- 4 files changed, 118 insertions(+), 8 deletions(-)
Signature · 1 line
1.8.5.2
PATCH 1/2 drm/i915/bdw: Implement a basic PM interrupt handler
From: Ben Widawsky <[email protected]>
Almost all of it is reusable from the existing code. The primary difference is we need to do even less in the interrupt handler, since interrupts are not shared in the same way.
The patch is mostly a copy-paste of the existing snb+ code, with updates to the relevant parts requiring changes to the interrupt handling. As such it /should/ be relatively trivial. It's highly likely that I missed some places where I need a gen8 version of the PM interrupts, but it has become invisible to me by now.
This patch could probably be split into adding the new functions, followed by actually handling the interrupts. Since the code is currently disabled (and broken) I think the patch stands better by itself.
v2: Move the commit about not touching the ringbuffer interrupt to the snb_* function where it belongs (Rodrigo)
v3: Rebased on Paulo's runtime PM changes
v4: Not well validated, but rebase on
commit 730488b2eddded4497f63f70867b1256cd9e117c
Author: Paulo Zanoni <[email protected]> Date: Fri Mar 7 20:12:32 2014 -0300
drm/i915: kill dev_priv->pm.regsave
v5: Rebased on latest code base. (Deepak)
Conflicts:
drivers/gpu/drm/i915/i915_irq.c --- drivers/gpu/drm/i915/i915_irq.c | 81 +++++++++++++++++++++++++++++++++++++--- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_drv.h | 3 +- drivers/gpu/drm/i915/intel_pm.c | 38 ++++++++++++++++++- 4 files changed, 115 insertions(+), 8 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +76 -5
@@ -248,6 +248,50 @@ static bool ivb_can_enable_err_int(struct drm_device *dev)return true; } +/** +* bdw_update_pm_irq - update GT interrupt 2 +* @dev_priv: driver private +* @interrupt_mask: mask of interrupt bits to update +* @enabled_irq_mask: mask of interrupt bits to enable +* +* Copied from the snb function, updated with relevant register offsets +*/ +static void bdw_update_pm_irq (struct drm_i915_private *dev_priv, +uint32_t interrupt_mask, +uint32_t enabled_irq_mask) +{ +uint32_t new_val; + +assert_spin_locked (&dev_priv->irq_lock); + +if (dev_priv->pm.irqs_disabled) { +WARN (1 ,"IRQs disabled\n" ); +return ; + } + + new_val = dev_priv->pm_irq_mask; + new_val &= ~interrupt_mask; + new_val |= (~enabled_irq_mask & interrupt_mask); + +if (new_val != dev_priv->pm_irq_mask) { + dev_priv->pm_irq_mask = new_val; +I915_WRITE (GEN8_GT_IMR (2 ),I915_READ (GEN8_GT_IMR (2 )) | + dev_priv->pm_irq_mask); +POSTING_READ (GEN8_GT_IMR (2 )); + } +} + +void bdw_enable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask) +{ +bdw_update_pm_irq (dev_priv, mask, mask); +} + +void bdw_disable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask) +{ +bdw_update_pm_irq (dev_priv, mask,0 ); +} + +static bool cpt_can_enable_serr_int (struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1126,13 +1170,17 @@ static void gen6_pm_rps_work(struct work_struct *work)spin_lock_irq (&dev_priv->irq_lock); pm_iir = dev_priv->rps.pm_iir; dev_priv->rps.pm_iir =0 ; -/* Make sure not to corrupt PMIMR state used by ringbuffer code */ -snb_enable_pm_irq (dev_priv, dev_priv->pm_rps_events); +if (IS_BROADWELL (dev_priv->dev)) +bdw_enable_pm_irq (dev_priv, dev_priv->pm_rps_events); +else { +/* Make sure not to corrupt PMIMR state used by ringbuffer */ +snb_enable_pm_irq (dev_priv, dev_priv->pm_rps_events); +/* Make sure we didn't queue anything we're not going to +* process. */ +WARN_ON (pm_iir & ~dev_priv->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 & ~dev_priv->pm_rps_events); -if ((pm_iir & dev_priv->pm_rps_events) ==0 )return ;
@@ -1324,6 +1372,19 @@ static void snb_gt_irq_handler(struct drm_device *dev,ivybridge_parity_error_irq_handler (dev, gt_iir); } +static void gen8_rps_irq_handler (struct drm_i915_private *dev_priv,u32 pm_iir) +{ +if ((pm_iir & dev_priv->pm_rps_events) ==0 ) +return ; + +spin_lock (&dev_priv->irq_lock); + dev_priv->rps.pm_iir |= pm_iir & dev_priv->pm_rps_events; +bdw_disable_pm_irq (dev_priv, pm_iir & dev_priv->pm_rps_events); +spin_unlock (&dev_priv->irq_lock); + +queue_work (dev_priv->wq, &dev_priv->rps.work); +} +static irqreturn_t gen8_gt_irq_handler (struct drm_device *dev,struct drm_i915_private *dev_priv,u32 master_ctl)
@@ -1359,6 +1420,16 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,DRM_ERROR ("The master control interrupt lied (GT1)!\n" ); } +if (master_ctl &GEN8_GT_PM_IRQ ) { + tmp =I915_READ (GEN8_GT_IIR (2 )); +if (tmp & dev_priv->pm_rps_events) { + ret =IRQ_HANDLED ; +gen8_rps_irq_handler (dev_priv, tmp); +I915_WRITE (GEN8_GT_IIR (1 ), tmp & dev_priv->pm_rps_events); + }else +DRM_ERROR ("The master control interrupt lied (PM)!\n" ); + } +if (master_ctl &GEN8_GT_VECS_IRQ ) { tmp =I915_READ (GEN8_GT_IIR (3 ));if (tmp) {
drivers/gpu/drm/i915/i915_reg.h +1 -0
@@ -4193,6 +4193,7 @@ enum punit_power_well {#define GEN8_DE_PIPE_A_IRQ (1<<16)#define GEN8_DE_PIPE_IRQ (pipe) (1<<(16+pipe))#define GEN8_GT_VECS_IRQ (1<<6) +#define GEN8_GT_PM_IRQ (1<<4)#define GEN8_GT_VCS2_IRQ (1<<3)#define GEN8_GT_VCS1_IRQ (1<<2)#define GEN8_GT_BCS_IRQ (1<<1)
drivers/gpu/drm/i915/intel_drv.h +2 -1
@@ -650,10 +650,11 @@ void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);void ilk_disable_gt_irq (struct drm_i915_private *dev_priv,uint32_t mask);void snb_enable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask);void snb_disable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask); +void bdw_enable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask); +void bdw_disable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask);void intel_runtime_pm_disable_interrupts (struct drm_device *dev);void intel_runtime_pm_restore_interrupts (struct drm_device *dev); -/* intel_crt.c */ void intel_crt_init (struct drm_device *dev);
drivers/gpu/drm/i915/intel_pm.c +36 -2
@@ -3210,6 +3210,25 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)trace_intel_gpu_freq_change (vlv_gpu_freq (dev_priv, val)); } +static void gen8_disable_rps_interrupts (struct drm_device *dev) +{ +struct drm_i915_private *dev_priv = dev->dev_private; + +I915_WRITE (GEN6_PMINTRMSK ,0xffffffff ); +I915_WRITE (GEN8_GT_IER (2 ),I915_READ (GEN8_GT_IER (2 )) & + ~dev_priv->pm_rps_events); +/* Complete PM interrupt masking here doesn't race with the rps work +* item again unmasking PM interrupts because that is using a different +* register (PMIMR) to mask PM interrupts. The only risk is in leaving +* stale bits in PMIIR and PMIMR which gen6_enable_rps will clean up. */ + +spin_lock_irq (&dev_priv->irq_lock); + dev_priv->rps.pm_iir =0 ; +spin_unlock_irq (&dev_priv->irq_lock); + +I915_WRITE (GEN8_GT_IIR (2 ), dev_priv->pm_rps_events); +} +static void gen6_disable_rps_interrupts (struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3236,7 +3255,10 @@ static void gen6_disable_rps(struct drm_device *dev)I915_WRITE (GEN6_RC_CONTROL ,0 );I915_WRITE (GEN6_RPNSWREQ ,1 <<31 ); -gen6_disable_rps_interrupts (dev); +if (IS_BROADWELL (dev)) +gen8_disable_rps_interrupts (dev); +else +gen6_disable_rps_interrupts (dev); }static void valleyview_disable_rps (struct drm_device *dev)
@@ -3276,6 +3298,18 @@ int intel_enable_rc6(const struct drm_device *dev)return INTEL_RC6_ENABLE ; } +static void gen8_enable_rps_interrupts (struct drm_device *dev) +{ +struct drm_i915_private *dev_priv = dev->dev_private; + +spin_lock_irq (&dev_priv->irq_lock); +WARN_ON (dev_priv->rps.pm_iir); +bdw_enable_pm_irq (dev_priv, dev_priv->pm_rps_events); +I915_WRITE (GEN8_GT_IIR (2 ), dev_priv->pm_rps_events); +spin_unlock_irq (&dev_priv->irq_lock); + +} +static void gen6_enable_rps_interrupts (struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3378,7 +3412,7 @@ static void gen8_enable_rps(struct drm_device *dev)gen6_set_rps (dev, (I915_READ (GEN6_GT_PERF_STATUS ) &0xff00 ) >>8 ); -gen6_enable_rps_interrupts (dev); +gen8_enable_rps_interrupts (dev);gen6_gt_force_wake_put (dev_priv,FORCEWAKE_ALL ); }
Signature · 1 line
1.8.5.2
Re: [PATCH 1/2] drm/i915/bdw: Implement a basic PM interrupt handler
Quoted 27 lines · On Mon, Apr 14, 2014 at 10:41:14PM +0530, [email protected] wrote:
From: Ben Widawsky <[email protected]>
Almost all of it is reusable from the existing code. The primary difference is we need to do even less in the interrupt handler, since interrupts are not shared in the same way.
The patch is mostly a copy-paste of the existing snb+ code, with updates to the relevant parts requiring changes to the interrupt handling. As such it /should/ be relatively trivial. It's highly likely that I missed some places where I need a gen8 version of the PM interrupts, but it has become invisible to me by now.
This patch could probably be split into adding the new functions, followed by actually handling the interrupts. Since the code is currently disabled (and broken) I think the patch stands better by itself.
v2: Move the commit about not touching the ringbuffer interrupt to the snb_* function where it belongs (Rodrigo)
v3: Rebased on Paulo's runtime PM changes
v4: Not well validated, but rebase on
commit730488b2eddded4497f63f70867b1256cd9e117cAuthor: Paulo Zanoni <[email protected]> Date: Fri Mar 7 20:12:32 2014 -0300
Quoted 8 lines
drm/i915: kill dev_priv->pm.regsavev5: Rebased on latest code base. (Deepak)
Signed-off-by Ben Widawsky <[email protected]>Conflicts:
drivers/gpu/drm/i915/i915_irq.c
IIRC Daniel doesn't like these conflict markers. So should be dropped.
Quoted 49 lines
--- drivers/gpu/drm/i915/i915_irq.c | 81 +++++++++++++++++++++++++++++++++++++--- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_drv.h | 3 +- drivers/gpu/drm/i915/intel_pm.c | 38 ++++++++++++++++++- 4 files changed, 115 insertions(+), 8 deletions(-)drivers/gpu/drm/i915/i915_irq.c +34
-0@@ -248,6 +248,50 @@ static bool ivb_can_enable_err_int(struct drm_device *dev)return true; } +/** +* bdw_update_pm_irq - update GT interrupt 2 +* @dev_priv: driver private +* @interrupt_mask: mask of interrupt bits to update +* @enabled_irq_mask: mask of interrupt bits to enable +* +* Copied from the snb function, updated with relevant register offsets +*/ +static void bdw_update_pm_irq (struct drm_i915_private *dev_priv, +uint32_t interrupt_mask, +uint32_t enabled_irq_mask) +{ +uint32_t new_val; + +assert_spin_locked (&dev_priv->irq_lock); + +if (dev_priv->pm.irqs_disabled) { +WARN (1 ,"IRQs disabled\n" ); +return ; + } + + new_val = dev_priv->pm_irq_mask; + new_val &= ~interrupt_mask; + new_val |= (~enabled_irq_mask & interrupt_mask); + +if (new_val != dev_priv->pm_irq_mask) { + dev_priv->pm_irq_mask = new_val; +I915_WRITE (GEN8_GT_IMR (2 ),I915_READ (GEN8_GT_IMR (2 )) | + dev_priv->pm_irq_mask); +POSTING_READ (GEN8_GT_IMR (2 )); + } +} + +void bdw_enable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask)
drivers/gpu/drm/i915/i915_irq.c static bool ivb_can_enable_err_int(struct drm_device *dev) · line 294
Quoted 10 lines
drivers/gpu/drm/i915/i915_irq.c +10
-0+{ +bdw_update_pm_irq (dev_priv, mask, mask); +} + +void bdw_disable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask) +{ +bdw_update_pm_irq (dev_priv, mask,0 ); +} + +
Unnecessary empty line.
Quoted 12 lines
drivers/gpu/drm/i915/i915_irq.c +3
-2static bool cpt_can_enable_serr_int (struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private;@@ -1126,13 +1170,17 @@ static void gen6_pm_rps_work(struct work_struct *work)spin_lock_irq (&dev_priv->irq_lock); pm_iir = dev_priv->rps.pm_iir; dev_priv->rps.pm_iir =0 ; -/* Make sure not to corrupt PMIMR state used by ringbuffer code */ -snb_enable_pm_irq (dev_priv, dev_priv->pm_rps_events); +if (IS_BROADWELL (dev_priv->dev)) +bdw_enable_pm_irq (dev_priv, dev_priv->pm_rps_events); +else {
drivers/gpu/drm/i915/i915_irq.c static void gen6_pm_rps_work(struct work_struct *work) · line 1134
Quoted 10 lines
drivers/gpu/drm/i915/i915_irq.c +6
-2+/* Make sure not to corrupt PMIMR state used by ringbuffer */ +snb_enable_pm_irq (dev_priv, dev_priv->pm_rps_events); +/* Make sure we didn't queue anything we're not going to +* process. */ +WARN_ON (pm_iir & ~dev_priv->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 & ~dev_priv->pm_rps_events);
Isn't this WARN equally valid for bdw?
Quoted 25 lines
drivers/gpu/drm/i915/i915_irq.c +13
-1-if ((pm_iir & dev_priv->pm_rps_events) ==0 )return ;@@ -1324,6 +1372,19 @@ static void snb_gt_irq_handler(struct drm_device *dev,ivybridge_parity_error_irq_handler (dev, gt_iir); } +static void gen8_rps_irq_handler (struct drm_i915_private *dev_priv,u32 pm_iir) +{ +if ((pm_iir & dev_priv->pm_rps_events) ==0 ) +return ; + +spin_lock (&dev_priv->irq_lock); + dev_priv->rps.pm_iir |= pm_iir & dev_priv->pm_rps_events; +bdw_disable_pm_irq (dev_priv, pm_iir & dev_priv->pm_rps_events); +spin_unlock (&dev_priv->irq_lock); + +queue_work (dev_priv->wq, &dev_priv->rps.work); +} +static irqreturn_t gen8_gt_irq_handler (struct drm_device *dev,struct drm_i915_private *dev_priv,u32 master_ctl)
drivers/gpu/drm/i915/i915_irq.c static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev, · line 1428
Quoted 9 lines
drivers/gpu/drm/i915/i915_irq.c +6
-0@@ -1359,6 +1420,16 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,DRM_ERROR ("The master control interrupt lied (GT1)!\n" ); } +if (master_ctl &GEN8_GT_PM_IRQ ) { + tmp =I915_READ (GEN8_GT_IIR (2 )); +if (tmp & dev_priv->pm_rps_events) { + ret =IRQ_HANDLED ; +gen8_rps_irq_handler (dev_priv, tmp); +I915_WRITE (GEN8_GT_IIR (1 ), tmp & dev_priv->pm_rps_events);
^^^^^^^^^^^^^^
GEN8_GT_IIR(2)
Quoted 24 lines
drivers/gpu/drm/i915/i915_irq.c +4
-0+ }else +DRM_ERROR ("The master control interrupt lied (PM)!\n" ); + } +if (master_ctl &GEN8_GT_VECS_IRQ ) { tmp =I915_READ (GEN8_GT_IIR (3 ));if (tmp) {drivers/gpu/drm/i915/i915_reg.h +1
-0@@ -4193,6 +4193,7 @@ enum punit_power_well {#define GEN8_DE_PIPE_A_IRQ (1<<16)#define GEN8_DE_PIPE_IRQ (pipe) (1<<(16+pipe))#define GEN8_GT_VECS_IRQ (1<<6) +#define GEN8_GT_PM_IRQ (1<<4)#define GEN8_GT_VCS2_IRQ (1<<3)#define GEN8_GT_VCS1_IRQ (1<<2)#define GEN8_GT_BCS_IRQ (1<<1)drivers/gpu/drm/i915/intel_drv.h +0
-0
drivers/gpu/drm/i915/intel_drv.h void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask); · line 656
Quoted 9 lines
drivers/gpu/drm/i915/intel_drv.h +2
-1@@ -650,10 +650,11 @@ void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);void ilk_disable_gt_irq (struct drm_i915_private *dev_priv,uint32_t mask);void snb_enable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask);void snb_disable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask); +void bdw_enable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask); +void bdw_disable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask);void intel_runtime_pm_disable_interrupts (struct drm_device *dev);void intel_runtime_pm_restore_interrupts (struct drm_device *dev); -
Unrelated whitespace change. IIRC someone left these two empty lines here on purpose.
Quoted 12 lines
drivers/gpu/drm/i915/intel_drv.h +0
-0/* intel_crt.c */ void intel_crt_init (struct drm_device *dev);drivers/gpu/drm/i915/intel_pm.c +1
-0@@ -3210,6 +3210,25 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)trace_intel_gpu_freq_change (vlv_gpu_freq (dev_priv, val)); } +static void gen8_disable_rps_interrupts (struct drm_device *dev)
drivers/gpu/drm/i915/intel_pm.c void valleyview_set_rps(struct drm_device *dev, u8 val) · line 3223
Quoted 10 lines
drivers/gpu/drm/i915/intel_pm.c +10
-0+{ +struct drm_i915_private *dev_priv = dev->dev_private; + +I915_WRITE (GEN6_PMINTRMSK ,0xffffffff ); +I915_WRITE (GEN8_GT_IER (2 ),I915_READ (GEN8_GT_IER (2 )) & + ~dev_priv->pm_rps_events); +/* Complete PM interrupt masking here doesn't race with the rps work +* item again unmasking PM interrupts because that is using a different +* register (PMIMR) to mask PM interrupts. The only risk is in leaving +* stale bits in PMIIR and PMIMR which gen6_enable_rps will clean up. */
This comment would need a bit of update for gen8.
Quoted 25 lines
drivers/gpu/drm/i915/intel_pm.c +12
-1+ +spin_lock_irq (&dev_priv->irq_lock); + dev_priv->rps.pm_iir =0 ; +spin_unlock_irq (&dev_priv->irq_lock); + +I915_WRITE (GEN8_GT_IIR (2 ), dev_priv->pm_rps_events); +} +static void gen6_disable_rps_interrupts (struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private;@@ -3236,7 +3255,10 @@ static void gen6_disable_rps(struct drm_device *dev)I915_WRITE (GEN6_RC_CONTROL ,0 );I915_WRITE (GEN6_RPNSWREQ ,1 <<31 ); -gen6_disable_rps_interrupts (dev); +if (IS_BROADWELL (dev)) +gen8_disable_rps_interrupts (dev); +else +gen6_disable_rps_interrupts (dev); }static void valleyview_disable_rps (struct drm_device *dev)@@ -3276,6 +3298,18 @@ int intel_enable_rc6(const struct drm_device *dev)return INTEL_RC6_ENABLE ;
drivers/gpu/drm/i915/intel_pm.c int intel_enable_rc6(const struct drm_device *dev) · line 3308
Quoted 10 lines
drivers/gpu/drm/i915/intel_pm.c +8
-0} +static void gen8_enable_rps_interrupts (struct drm_device *dev) +{ +struct drm_i915_private *dev_priv = dev->dev_private; + +spin_lock_irq (&dev_priv->irq_lock); +WARN_ON (dev_priv->rps.pm_iir); +bdw_enable_pm_irq (dev_priv, dev_priv->pm_rps_events); +I915_WRITE (GEN8_GT_IIR (2 ), dev_priv->pm_rps_events);
The order of "unmask first then clear" seems just wrong. The same issues is already present in the gen6 code however. So this should be fixed in both places, or if it's like that on purpose it needs a comment.
drivers/gpu/drm/i915/intel_pm.c int intel_enable_rc6(const struct drm_device *dev) · line 3310
Quoted 2 lines
drivers/gpu/drm/i915/intel_pm.c +2
-0+spin_unlock_irq (&dev_priv->irq_lock); +
Useless empty line.
Quoted 21 lines
drivers/gpu/drm/i915/intel_pm.c +3
-1+} +static void gen6_enable_rps_interrupts (struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private;@@ -3378,7 +3412,7 @@ static void gen8_enable_rps(struct drm_device *dev)gen6_set_rps (dev, (I915_READ (GEN6_GT_PERF_STATUS ) &0xff00 ) >>8 ); -gen6_enable_rps_interrupts (dev); +gen8_enable_rps_interrupts (dev);gen6_gt_force_wake_put (dev_priv,FORCEWAKE_ALL ); }Signature · 6 lines
1.8.5.2 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 2 lines
Ville Syrjälä Intel OTC
Re: [PATCH 1/2] drm/i915/bdw: Implement a basic PM interrupt handler
Quoted 30 lines · Ville Syrjälä
Quoted 29 lines · On Mon, Apr 14, 2014 at 10:41:14PM +0530, [email protected] wrote:
From: Ben Widawsky <[email protected]>
Almost all of it is reusable from the existing code. The primary difference is we need to do even less in the interrupt handler, since interrupts are not shared in the same way.
The patch is mostly a copy-paste of the existing snb+ code, with updates to the relevant parts requiring changes to the interrupt handling. As such it /should/ be relatively trivial. It's highly likely that I missed some places where I need a gen8 version of the PM interrupts, but it has become invisible to me by now.
This patch could probably be split into adding the new functions, followed by actually handling the interrupts. Since the code is currently disabled (and broken) I think the patch stands better by itself.
v2: Move the commit about not touching the ringbuffer interrupt to the snb_* function where it belongs (Rodrigo)
v3: Rebased on Paulo's runtime PM changes
v4: Not well validated, but rebase on
commit730488b2eddded4497f63f70867b1256cd9e117cAuthor: Paulo Zanoni <[email protected]> Date: Fri Mar 7 20:12:32 2014 -0300drm/i915: kill dev_priv->pm.regsave
Quoted 9 lines
Quoted 6 lines
v5: Rebased on latest code base. (Deepak)
Signed-off-by Ben Widawsky <[email protected]>Conflicts:
drivers/gpu/drm/i915/i915_irq.cIIRC Daniel doesn't like these conflict markers. So should be dropped.
I like the conflict markers generally. Daniel can kill it if he likes, but thanks for the input. I've killed it this time around, but I don't plan on it for the future.
Quoted 53 lines
Quoted 53 lines
--- drivers/gpu/drm/i915/i915_irq.c | 81 +++++++++++++++++++++++++++++++++++++--- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_drv.h | 3 +- drivers/gpu/drm/i915/intel_pm.c | 38 ++++++++++++++++++- 4 files changed, 115 insertions(+), 8 deletions(-)drivers/gpu/drm/i915/i915_irq.c +38
-0@@ -248,6 +248,50 @@ static bool ivb_can_enable_err_int(struct drm_device *dev)return true; } +/** +* bdw_update_pm_irq - update GT interrupt 2 +* @dev_priv: driver private +* @interrupt_mask: mask of interrupt bits to update +* @enabled_irq_mask: mask of interrupt bits to enable +* +* Copied from the snb function, updated with relevant register offsets +*/ +static void bdw_update_pm_irq (struct drm_i915_private *dev_priv, +uint32_t interrupt_mask, +uint32_t enabled_irq_mask) +{ +uint32_t new_val; + +assert_spin_locked (&dev_priv->irq_lock); + +if (dev_priv->pm.irqs_disabled) { +WARN (1 ,"IRQs disabled\n" ); +return ; + } + + new_val = dev_priv->pm_irq_mask; + new_val &= ~interrupt_mask; + new_val |= (~enabled_irq_mask & interrupt_mask); + +if (new_val != dev_priv->pm_irq_mask) { + dev_priv->pm_irq_mask = new_val; +I915_WRITE (GEN8_GT_IMR (2 ),I915_READ (GEN8_GT_IMR (2 )) | + dev_priv->pm_irq_mask); +POSTING_READ (GEN8_GT_IMR (2 )); + } +} + +void bdw_enable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask) +{ +bdw_update_pm_irq (dev_priv, mask, mask); +} +
Quoted 9 lines
drivers/gpu/drm/i915/i915_irq.c static bool ivb_can_enable_err_int(struct drm_device *dev) · line 294
Quoted 6 lines
drivers/gpu/drm/i915/i915_irq.c +6
-0+void bdw_disable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask) +{ +bdw_update_pm_irq (dev_priv, mask,0 ); +} + +Unnecessary empty line.
Got it, thanks.
Quoted 20 lines
Quoted 20 lines
drivers/gpu/drm/i915/i915_irq.c +9
-2static bool cpt_can_enable_serr_int (struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private;@@ -1126,13 +1170,17 @@ static void gen6_pm_rps_work(struct work_struct *work)spin_lock_irq (&dev_priv->irq_lock); pm_iir = dev_priv->rps.pm_iir; dev_priv->rps.pm_iir =0 ; -/* Make sure not to corrupt PMIMR state used by ringbuffer code */ -snb_enable_pm_irq (dev_priv, dev_priv->pm_rps_events); +if (IS_BROADWELL (dev_priv->dev)) +bdw_enable_pm_irq (dev_priv, dev_priv->pm_rps_events); +else { +/* Make sure not to corrupt PMIMR state used by ringbuffer */ +snb_enable_pm_irq (dev_priv, dev_priv->pm_rps_events); +/* Make sure we didn't queue anything we're not going to +* process. */ +WARN_ON (pm_iir & ~dev_priv->pm_rps_events); + }spin_unlock_irq (&dev_priv->irq_lock);
Quoted 5 lines
drivers/gpu/drm/i915/i915_irq.c static void gen6_pm_rps_work(struct work_struct *work) · line 1134
Quoted 2 lines
drivers/gpu/drm/i915/i915_irq.c +0
-2-/* Make sure we didn't queue anything we're not going to process. */ -WARN_ON (pm_iir & ~dev_priv->pm_rps_events);Isn't this WARN equally valid for bdw?
So first, let me just mention, this has moved slightly in my latest version of this patch, but it's still a valid question.
The answer is ambiguous actually. The WARN is always valid technically The distinction in BDW (at least for the time being) is that unlike pre-BDW, PM IIR isn't shared. I can add it, but for BDW, at least right now, DRM_ERROR (or BUG) is more appropriate.
Quoted 36 lines
Quoted 25 lines
drivers/gpu/drm/i915/i915_irq.c +13
-1-if ((pm_iir & dev_priv->pm_rps_events) ==0 )return ;@@ -1324,6 +1372,19 @@ static void snb_gt_irq_handler(struct drm_device *dev,ivybridge_parity_error_irq_handler (dev, gt_iir); } +static void gen8_rps_irq_handler (struct drm_i915_private *dev_priv,u32 pm_iir) +{ +if ((pm_iir & dev_priv->pm_rps_events) ==0 ) +return ; + +spin_lock (&dev_priv->irq_lock); + dev_priv->rps.pm_iir |= pm_iir & dev_priv->pm_rps_events; +bdw_disable_pm_irq (dev_priv, pm_iir & dev_priv->pm_rps_events); +spin_unlock (&dev_priv->irq_lock); + +queue_work (dev_priv->wq, &dev_priv->rps.work); +} +static irqreturn_t gen8_gt_irq_handler (struct drm_device *dev,struct drm_i915_private *dev_priv,u32 master_ctl)drivers/gpu/drm/i915/i915_irq.c static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev, · line 1428
Quoted 9 lines
drivers/gpu/drm/i915/i915_irq.c +6
-0@@ -1359,6 +1420,16 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,DRM_ERROR ("The master control interrupt lied (GT1)!\n" ); } +if (master_ctl &GEN8_GT_PM_IRQ ) { + tmp =I915_READ (GEN8_GT_IIR (2 )); +if (tmp & dev_priv->pm_rps_events) { + ret =IRQ_HANDLED ; +gen8_rps_irq_handler (dev_priv, tmp); +I915_WRITE (GEN8_GT_IIR (1 ), tmp & dev_priv->pm_rps_events);^^^^^^^^^^^^^^
Quoted 2 lines
GEN8_GT_IIR(2)
Nice catch, I guess I need to retest after this one.
Quoted 32 lines
Quoted 32 lines
drivers/gpu/drm/i915/i915_irq.c +4
-0+ }else +DRM_ERROR ("The master control interrupt lied (PM)!\n" ); + } +if (master_ctl &GEN8_GT_VECS_IRQ ) { tmp =I915_READ (GEN8_GT_IIR (3 ));if (tmp) {drivers/gpu/drm/i915/i915_reg.h +1
-0@@ -4193,6 +4193,7 @@ enum punit_power_well {#define GEN8_DE_PIPE_A_IRQ (1<<16)#define GEN8_DE_PIPE_IRQ (pipe) (1<<(16+pipe))#define GEN8_GT_VECS_IRQ (1<<6) +#define GEN8_GT_PM_IRQ (1<<4)#define GEN8_GT_VCS2_IRQ (1<<3)#define GEN8_GT_VCS1_IRQ (1<<2)#define GEN8_GT_BCS_IRQ (1<<1)drivers/gpu/drm/i915/intel_drv.h +2
-0@@ -650,10 +650,11 @@ void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);void ilk_disable_gt_irq (struct drm_i915_private *dev_priv,uint32_t mask);void snb_enable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask);void snb_disable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask); +void bdw_enable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask); +void bdw_disable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask);void intel_runtime_pm_disable_interrupts (struct drm_device *dev);void intel_runtime_pm_restore_interrupts (struct drm_device *dev);
Quoted 5 lines
drivers/gpu/drm/i915/intel_drv.h void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask); · line 656
Quoted 1 line
drivers/gpu/drm/i915/intel_drv.h +0
-1-Unrelated whitespace change. IIRC someone left these two empty lines here on purpose.
Got it, thanks.
Quoted 11 lines
Quoted 11 lines
drivers/gpu/drm/i915/intel_drv.h +0
-0/* intel_crt.c */ void intel_crt_init (struct drm_device *dev);drivers/gpu/drm/i915/intel_pm.c +0
-0@@ -3210,6 +3210,25 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)trace_intel_gpu_freq_change (vlv_gpu_freq (dev_priv, val)); }
Quoted 14 lines
drivers/gpu/drm/i915/intel_pm.c void valleyview_set_rps(struct drm_device *dev, u8 val) · line 3223
Quoted 11 lines
drivers/gpu/drm/i915/intel_pm.c +11
-0+static void gen8_disable_rps_interrupts (struct drm_device *dev) +{ +struct drm_i915_private *dev_priv = dev->dev_private; + +I915_WRITE (GEN6_PMINTRMSK ,0xffffffff ); +I915_WRITE (GEN8_GT_IER (2 ),I915_READ (GEN8_GT_IER (2 )) & + ~dev_priv->pm_rps_events); +/* Complete PM interrupt masking here doesn't race with the rps work +* item again unmasking PM interrupts because that is using a different +* register (PMIMR) to mask PM interrupts. The only risk is in leaving +* stale bits in PMIIR and PMIMR which gen6_enable_rps will clean up. */ This comment would need a bit of update for gen8.
I've killed this comment locally. Somewhere I had written that we can rewrite some of this for gen8, but I seem to have lost that in the sands of time.
Quoted 27 lines
Quoted 27 lines
drivers/gpu/drm/i915/intel_pm.c +12
-1+ +spin_lock_irq (&dev_priv->irq_lock); + dev_priv->rps.pm_iir =0 ; +spin_unlock_irq (&dev_priv->irq_lock); + +I915_WRITE (GEN8_GT_IIR (2 ), dev_priv->pm_rps_events); +} +static void gen6_disable_rps_interrupts (struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private;@@ -3236,7 +3255,10 @@ static void gen6_disable_rps(struct drm_device *dev)I915_WRITE (GEN6_RC_CONTROL ,0 );I915_WRITE (GEN6_RPNSWREQ ,1 <<31 ); -gen6_disable_rps_interrupts (dev); +if (IS_BROADWELL (dev)) +gen8_disable_rps_interrupts (dev); +else +gen6_disable_rps_interrupts (dev); }static void valleyview_disable_rps (struct drm_device *dev)@@ -3276,6 +3298,18 @@ int intel_enable_rc6(const struct drm_device *dev)return INTEL_RC6_ENABLE ; }
Quoted 13 lines
drivers/gpu/drm/i915/intel_pm.c int intel_enable_rc6(const struct drm_device *dev) · line 3308
Quoted 8 lines
drivers/gpu/drm/i915/intel_pm.c +8
-0+static void gen8_enable_rps_interrupts (struct drm_device *dev) +{ +struct drm_i915_private *dev_priv = dev->dev_private; + +spin_lock_irq (&dev_priv->irq_lock); +WARN_ON (dev_priv->rps.pm_iir); +bdw_enable_pm_irq (dev_priv, dev_priv->pm_rps_events); +I915_WRITE (GEN8_GT_IIR (2 ), dev_priv->pm_rps_events);The order of "unmask first then clear" seems just wrong. The same issues is already present in the gen6 code however. So this should be fixed in both places, or if it's like that on purpose it needs a comment.
Well, if you want to fix it, we can do it in a separate patch. Although I don't think I agree. Why does it seem wrong to you?
Quoted 5 lines
drivers/gpu/drm/i915/intel_pm.c int intel_enable_rc6(const struct drm_device *dev) · line 3310
Quoted 2 lines
drivers/gpu/drm/i915/intel_pm.c +2
-0+spin_unlock_irq (&dev_priv->irq_lock); +Useless empty line.
If I was in an arguing mood, I would do, but fine.
Quoted 7 lines
Quoted 7 lines
drivers/gpu/drm/i915/intel_pm.c +2
-0+} +static void gen6_enable_rps_interrupts (struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private;@@ -3378,7 +3412,7 @@ static void gen8_enable_rps(struct drm_device *dev)
Quoted 7 lines
drivers/gpu/drm/i915/intel_pm.c static void gen8_enable_rps(struct drm_device *dev) · line 3418
Quoted 7 lines
drivers/gpu/drm/i915/intel_pm.c +1
-1gen6_set_rps (dev, (I915_READ (GEN6_GT_PERF_STATUS ) &0xff00 ) >>8 ); -gen6_enable_rps_interrupts (dev); +gen8_enable_rps_interrupts (dev);gen6_gt_force_wake_put (dev_priv,FORCEWAKE_ALL ); }
Thanks for the review.
Signature · 5 lines
Ben Widawsky, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 1/2] drm/i915/bdw: Implement a basic PM interrupt handler
Quoted 38 lines · Ben Widawsky
Quoted 37 lines · Ville Syrjälä
Quoted 35 lines · On Mon, Apr 14, 2014 at 10:41:14PM +0530, [email protected] wrote:
From: Ben Widawsky <[email protected]>
Almost all of it is reusable from the existing code. The primary difference is we need to do even less in the interrupt handler, since interrupts are not shared in the same way.
The patch is mostly a copy-paste of the existing snb+ code, with updates to the relevant parts requiring changes to the interrupt handling. As such it /should/ be relatively trivial. It's highly likely that I missed some places where I need a gen8 version of the PM interrupts, but it has become invisible to me by now.
This patch could probably be split into adding the new functions, followed by actually handling the interrupts. Since the code is currently disabled (and broken) I think the patch stands better by itself.
v2: Move the commit about not touching the ringbuffer interrupt to the snb_* function where it belongs (Rodrigo)
v3: Rebased on Paulo's runtime PM changes
v4: Not well validated, but rebase on
commit730488b2eddded4497f63f70867b1256cd9e117cAuthor: Paulo Zanoni <[email protected]> Date: Fri Mar 7 20:12:32 2014 -0300drm/i915: kill dev_priv->pm.regsavev5: Rebased on latest code base. (Deepak)
Signed-off-by Ben Widawsky <[email protected]>Conflicts:
drivers/gpu/drm/i915/i915_irq.c
Quoted 6 lines
Quoted 2 lines
IIRC Daniel doesn't like these conflict markers. So should be dropped.
I like the conflict markers generally. Daniel can kill it if he likes, but thanks for the input. I've killed it this time around, but I don't plan on it for the future.
Imo leaving them in is just lazy. Either there was some real conflict that required real work, and then you should rev the patch revision and take note about the changes in the in-patch changelog.
Or the conflict was trivial, in which case the left-behind marker is meaningless and doesn't add anything useful to the commit message.
So please remove them and if it makes sense augment the patch revision log. See e.g. the rebase notes (where I mention the upstream changes that required the rebase) I add for -internal patches.
-Daniel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Re: [PATCH 1/2] drm/i915/bdw: Implement a basic PM interrupt handler
In reply to Ben WidawskyQuoted 136 lines · Ben Widawsky
Quoted 30 lines · Ville Syrjälä
Quoted 29 lines · On Mon, Apr 14, 2014 at 10:41:14PM +0530, [email protected] wrote:
From: Ben Widawsky <[email protected]>
Almost all of it is reusable from the existing code. The primary difference is we need to do even less in the interrupt handler, since interrupts are not shared in the same way.
The patch is mostly a copy-paste of the existing snb+ code, with updates to the relevant parts requiring changes to the interrupt handling. As such it /should/ be relatively trivial. It's highly likely that I missed some places where I need a gen8 version of the PM interrupts, but it has become invisible to me by now.
This patch could probably be split into adding the new functions, followed by actually handling the interrupts. Since the code is currently disabled (and broken) I think the patch stands better by itself.
v2: Move the commit about not touching the ringbuffer interrupt to the snb_* function where it belongs (Rodrigo)
v3: Rebased on Paulo's runtime PM changes
v4: Not well validated, but rebase on
commit730488b2eddded4497f63f70867b1256cd9e117cAuthor: Paulo Zanoni <[email protected]> Date: Fri Mar 7 20:12:32 2014 -0300drm/i915: kill dev_priv->pm.regsaveQuoted 9 lines
Quoted 6 lines
v5: Rebased on latest code base. (Deepak)
Signed-off-by Ben Widawsky <[email protected]>Conflicts:
drivers/gpu/drm/i915/i915_irq.cIIRC Daniel doesn't like these conflict markers. So should be dropped.
I like the conflict markers generally. Daniel can kill it if he likes, but thanks for the input. I've killed it this time around, but I don't plan on it for the future.
Quoted 53 lines
Quoted 53 lines
--- drivers/gpu/drm/i915/i915_irq.c | 81 +++++++++++++++++++++++++++++++++++++--- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_drv.h | 3 +- drivers/gpu/drm/i915/intel_pm.c | 38 ++++++++++++++++++- 4 files changed, 115 insertions(+), 8 deletions(-)drivers/gpu/drm/i915/i915_irq.c +38
-0@@ -248,6 +248,50 @@ static bool ivb_can_enable_err_int(struct drm_device *dev)return true; } +/** +* bdw_update_pm_irq - update GT interrupt 2 +* @dev_priv: driver private +* @interrupt_mask: mask of interrupt bits to update +* @enabled_irq_mask: mask of interrupt bits to enable +* +* Copied from the snb function, updated with relevant register offsets +*/ +static void bdw_update_pm_irq (struct drm_i915_private *dev_priv, +uint32_t interrupt_mask, +uint32_t enabled_irq_mask) +{ +uint32_t new_val; + +assert_spin_locked (&dev_priv->irq_lock); + +if (dev_priv->pm.irqs_disabled) { +WARN (1 ,"IRQs disabled\n" ); +return ; + } + + new_val = dev_priv->pm_irq_mask; + new_val &= ~interrupt_mask; + new_val |= (~enabled_irq_mask & interrupt_mask); + +if (new_val != dev_priv->pm_irq_mask) { + dev_priv->pm_irq_mask = new_val; +I915_WRITE (GEN8_GT_IMR (2 ),I915_READ (GEN8_GT_IMR (2 )) | + dev_priv->pm_irq_mask); +POSTING_READ (GEN8_GT_IMR (2 )); + } +} + +void bdw_enable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask) +{ +bdw_update_pm_irq (dev_priv, mask, mask); +} +Quoted 9 lines
drivers/gpu/drm/i915/i915_irq.c static bool ivb_can_enable_err_int(struct drm_device *dev) · line 294
Quoted 6 lines
drivers/gpu/drm/i915/i915_irq.c +6
-0+void bdw_disable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask) +{ +bdw_update_pm_irq (dev_priv, mask,0 ); +} + +Unnecessary empty line.
Got it, thanks.
Quoted 20 lines
Quoted 20 lines
drivers/gpu/drm/i915/i915_irq.c +9
-2static bool cpt_can_enable_serr_int (struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private;@@ -1126,13 +1170,17 @@ static void gen6_pm_rps_work(struct work_struct *work)spin_lock_irq (&dev_priv->irq_lock); pm_iir = dev_priv->rps.pm_iir; dev_priv->rps.pm_iir =0 ; -/* Make sure not to corrupt PMIMR state used by ringbuffer code */ -snb_enable_pm_irq (dev_priv, dev_priv->pm_rps_events); +if (IS_BROADWELL (dev_priv->dev)) +bdw_enable_pm_irq (dev_priv, dev_priv->pm_rps_events); +else { +/* Make sure not to corrupt PMIMR state used by ringbuffer */ +snb_enable_pm_irq (dev_priv, dev_priv->pm_rps_events); +/* Make sure we didn't queue anything we're not going to +* process. */ +WARN_ON (pm_iir & ~dev_priv->pm_rps_events); + }spin_unlock_irq (&dev_priv->irq_lock);Quoted 5 lines
drivers/gpu/drm/i915/i915_irq.c static void gen6_pm_rps_work(struct work_struct *work) · line 1134
Quoted 2 lines
drivers/gpu/drm/i915/i915_irq.c +0
-2-/* Make sure we didn't queue anything we're not going to process. */ -WARN_ON (pm_iir & ~dev_priv->pm_rps_events);Isn't this WARN equally valid for bdw?
Quoted 7 lines
So first, let me just mention, this has moved slightly in my latest version of this patch, but it's still a valid question.
The answer is ambiguous actually. The WARN is always valid technically The distinction in BDW (at least for the time being) is that unlike pre-BDW, PM IIR isn't shared. I can add it, but for BDW, at least right now,
DRM_ERROR(or BUG) is more appropriate.
I don't see a reason for bdw to be different here. There are other bits in the relevant IIR registers, so the same WARN should be fine.
Quoted 38 lines
Quoted 38 lines
Quoted 27 lines
drivers/gpu/drm/i915/i915_irq.c +13
-1-if ((pm_iir & dev_priv->pm_rps_events) ==0 )return ;@@ -1324,6 +1372,19 @@ static void snb_gt_irq_handler(struct drm_device *dev,ivybridge_parity_error_irq_handler (dev, gt_iir); } +static void gen8_rps_irq_handler (struct drm_i915_private *dev_priv,u32 pm_iir) +{ +if ((pm_iir & dev_priv->pm_rps_events) ==0 ) +return ; + +spin_lock (&dev_priv->irq_lock); + dev_priv->rps.pm_iir |= pm_iir & dev_priv->pm_rps_events; +bdw_disable_pm_irq (dev_priv, pm_iir & dev_priv->pm_rps_events); +spin_unlock (&dev_priv->irq_lock); + +queue_work (dev_priv->wq, &dev_priv->rps.work); +} +static irqreturn_t gen8_gt_irq_handler (struct drm_device *dev,struct drm_i915_private *dev_priv,u32 master_ctl)drivers/gpu/drm/i915/i915_irq.c static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev, · line 1428
Quoted 9 lines
drivers/gpu/drm/i915/i915_irq.c +6
-0@@ -1359,6 +1420,16 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,DRM_ERROR ("The master control interrupt lied (GT1)!\n" ); } +if (master_ctl &GEN8_GT_PM_IRQ ) { + tmp =I915_READ (GEN8_GT_IIR (2 )); +if (tmp & dev_priv->pm_rps_events) { + ret =IRQ_HANDLED ; +gen8_rps_irq_handler (dev_priv, tmp); +I915_WRITE (GEN8_GT_IIR (1 ), tmp & dev_priv->pm_rps_events);^^^^^^^^^^^^^^
Quoted 4 lines
Quoted 2 lines
GEN8_GT_IIR(2)Nice catch, I guess I need to retest after this one.
I'm actually wondering how/if you managed to test this before Deepak's PMINTRMSK interrupt redirect bit fix. W/o that you shouldn't be getting any interrupt AFAICS.
Quoted 112 lines
Quoted 33 lines
Quoted 33 lines
drivers/gpu/drm/i915/i915_irq.c +4
-0+ }else +DRM_ERROR ("The master control interrupt lied (PM)!\n" ); + } +if (master_ctl &GEN8_GT_VECS_IRQ ) { tmp =I915_READ (GEN8_GT_IIR (3 ));if (tmp) {drivers/gpu/drm/i915/i915_reg.h +1
-0@@ -4193,6 +4193,7 @@ enum punit_power_well {#define GEN8_DE_PIPE_A_IRQ (1<<16)#define GEN8_DE_PIPE_IRQ (pipe) (1<<(16+pipe))#define GEN8_GT_VECS_IRQ (1<<6) +#define GEN8_GT_PM_IRQ (1<<4)#define GEN8_GT_VCS2_IRQ (1<<3)#define GEN8_GT_VCS1_IRQ (1<<2)#define GEN8_GT_BCS_IRQ (1<<1)drivers/gpu/drm/i915/intel_drv.h +2
-0@@ -650,10 +650,11 @@ void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);void ilk_disable_gt_irq (struct drm_i915_private *dev_priv,uint32_t mask);void snb_enable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask);void snb_disable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask); +void bdw_enable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask); +void bdw_disable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask);void intel_runtime_pm_disable_interrupts (struct drm_device *dev);void intel_runtime_pm_restore_interrupts (struct drm_device *dev);Quoted 5 lines
drivers/gpu/drm/i915/intel_drv.h void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask); · line 656
Quoted 1 line
drivers/gpu/drm/i915/intel_drv.h +0
-1-Unrelated whitespace change. IIRC someone left these two empty lines here on purpose.
Got it, thanks.
Quoted 11 lines
Quoted 11 lines
drivers/gpu/drm/i915/intel_drv.h +0
-0/* intel_crt.c */ void intel_crt_init (struct drm_device *dev);drivers/gpu/drm/i915/intel_pm.c +0
-0@@ -3210,6 +3210,25 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)trace_intel_gpu_freq_change (vlv_gpu_freq (dev_priv, val)); }Quoted 14 lines
drivers/gpu/drm/i915/intel_pm.c void valleyview_set_rps(struct drm_device *dev, u8 val) · line 3223
Quoted 11 lines
drivers/gpu/drm/i915/intel_pm.c +11
-0+static void gen8_disable_rps_interrupts (struct drm_device *dev) +{ +struct drm_i915_private *dev_priv = dev->dev_private; + +I915_WRITE (GEN6_PMINTRMSK ,0xffffffff ); +I915_WRITE (GEN8_GT_IER (2 ),I915_READ (GEN8_GT_IER (2 )) & + ~dev_priv->pm_rps_events); +/* Complete PM interrupt masking here doesn't race with the rps work +* item again unmasking PM interrupts because that is using a different +* register (PMIMR) to mask PM interrupts. The only risk is in leaving +* stale bits in PMIIR and PMIMR which gen6_enable_rps will clean up. */ This comment would need a bit of update for gen8.
I've killed this comment locally. Somewhere I had written that we can rewrite some of this for gen8, but I seem to have lost that in the sands of time.
Quoted 27 lines
Quoted 27 lines
drivers/gpu/drm/i915/intel_pm.c +12
-1+ +spin_lock_irq (&dev_priv->irq_lock); + dev_priv->rps.pm_iir =0 ; +spin_unlock_irq (&dev_priv->irq_lock); + +I915_WRITE (GEN8_GT_IIR (2 ), dev_priv->pm_rps_events); +} +static void gen6_disable_rps_interrupts (struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private;@@ -3236,7 +3255,10 @@ static void gen6_disable_rps(struct drm_device *dev)I915_WRITE (GEN6_RC_CONTROL ,0 );I915_WRITE (GEN6_RPNSWREQ ,1 <<31 ); -gen6_disable_rps_interrupts (dev); +if (IS_BROADWELL (dev)) +gen8_disable_rps_interrupts (dev); +else +gen6_disable_rps_interrupts (dev); }static void valleyview_disable_rps (struct drm_device *dev)@@ -3276,6 +3298,18 @@ int intel_enable_rc6(const struct drm_device *dev)return INTEL_RC6_ENABLE ; }Quoted 13 lines
drivers/gpu/drm/i915/intel_pm.c int intel_enable_rc6(const struct drm_device *dev) · line 3308
Quoted 8 lines
drivers/gpu/drm/i915/intel_pm.c +8
-0+static void gen8_enable_rps_interrupts (struct drm_device *dev) +{ +struct drm_i915_private *dev_priv = dev->dev_private; + +spin_lock_irq (&dev_priv->irq_lock); +WARN_ON (dev_priv->rps.pm_iir); +bdw_enable_pm_irq (dev_priv, dev_priv->pm_rps_events); +I915_WRITE (GEN8_GT_IIR (2 ), dev_priv->pm_rps_events);The order of "unmask first then clear" seems just wrong. The same issues is already present in the gen6 code however. So this should be fixed in both places, or if it's like that on purpose it needs a comment.
Quoted 2 lines
Well, if you want to fix it, we can do it in a separate patch. Although I don't think I agree. Why does it seem wrong to you?
If you enable before clearing and there's already an IIR bit high the interrupt should trigger immediately before you get to clear it. And then on a SMP machine the write to clear it would even race with the interrupt handler if there's no locking. So the clearing after enabling+unmasking is pointless. The normal approach for enabling interrupts is to clear first and then enable.
Hmm. Actually here we alreay have the interrupts enabled in IER, so if any relevant IIR bit would be high, we should have already gotten the interrupt. But since we always clear the IIR after masking the interrupts in IMR, we should never have any relevant IIR bit set here.
So looks to me like we can just drop the IIR writes here. They serve no purpose other than racing with the interrupt handler if a real interrupt happens just after we've unmasked it.
Quoted 28 lines
Quoted 6 lines
drivers/gpu/drm/i915/intel_pm.c int intel_enable_rc6(const struct drm_device *dev) · line 3310
Quoted 3 lines
drivers/gpu/drm/i915/intel_pm.c +2
-0+spin_unlock_irq (&dev_priv->irq_lock); +Useless empty line.
If I was in an arguing mood, I would do, but fine.
Quoted 7 lines
Quoted 7 lines
drivers/gpu/drm/i915/intel_pm.c +2
-0+} +static void gen6_enable_rps_interrupts (struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private;@@ -3378,7 +3412,7 @@ static void gen8_enable_rps(struct drm_device *dev)Quoted 7 lines
drivers/gpu/drm/i915/intel_pm.c static void gen8_enable_rps(struct drm_device *dev) · line 3418
Quoted 7 lines
drivers/gpu/drm/i915/intel_pm.c +1
-1gen6_set_rps (dev, (I915_READ (GEN6_GT_PERF_STATUS ) &0xff00 ) >>8 ); -gen6_enable_rps_interrupts (dev); +gen8_enable_rps_interrupts (dev);gen6_gt_force_wake_put (dev_priv,FORCEWAKE_ALL ); }Thanks for the review.
Signature · 1 line
Ben Widawsky, Intel Open Source Technology Center
Signature · 2 lines
Ville Syrjälä Intel OTC
Re: [PATCH 1/2] drm/i915/bdw: Implement a basic PM interrupt handler
Quoted 317 lines · Ville Syrjälä
Quoted 136 lines · Ben Widawsky
Quoted 30 lines · Ville Syrjälä
Quoted 29 lines · On Mon, Apr 14, 2014 at 10:41:14PM +0530, [email protected] wrote:
From: Ben Widawsky <[email protected]>
Almost all of it is reusable from the existing code. The primary difference is we need to do even less in the interrupt handler, since interrupts are not shared in the same way.
The patch is mostly a copy-paste of the existing snb+ code, with updates to the relevant parts requiring changes to the interrupt handling. As such it /should/ be relatively trivial. It's highly likely that I missed some places where I need a gen8 version of the PM interrupts, but it has become invisible to me by now.
This patch could probably be split into adding the new functions, followed by actually handling the interrupts. Since the code is currently disabled (and broken) I think the patch stands better by itself.
v2: Move the commit about not touching the ringbuffer interrupt to the snb_* function where it belongs (Rodrigo)
v3: Rebased on Paulo's runtime PM changes
v4: Not well validated, but rebase on
commit730488b2eddded4497f63f70867b1256cd9e117cAuthor: Paulo Zanoni <[email protected]> Date: Fri Mar 7 20:12:32 2014 -0300drm/i915: kill dev_priv->pm.regsaveQuoted 9 lines
Quoted 6 lines
v5: Rebased on latest code base. (Deepak)
Signed-off-by Ben Widawsky <[email protected]>Conflicts:
drivers/gpu/drm/i915/i915_irq.cIIRC Daniel doesn't like these conflict markers. So should be dropped.
I like the conflict markers generally. Daniel can kill it if he likes, but thanks for the input. I've killed it this time around, but I don't plan on it for the future.
Quoted 53 lines
Quoted 53 lines
--- drivers/gpu/drm/i915/i915_irq.c | 81 +++++++++++++++++++++++++++++++++++++--- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_drv.h | 3 +- drivers/gpu/drm/i915/intel_pm.c | 38 ++++++++++++++++++- 4 files changed, 115 insertions(+), 8 deletions(-)drivers/gpu/drm/i915/i915_irq.c +38
-0@@ -248,6 +248,50 @@ static bool ivb_can_enable_err_int(struct drm_device *dev)return true; } +/** +* bdw_update_pm_irq - update GT interrupt 2 +* @dev_priv: driver private +* @interrupt_mask: mask of interrupt bits to update +* @enabled_irq_mask: mask of interrupt bits to enable +* +* Copied from the snb function, updated with relevant register offsets +*/ +static void bdw_update_pm_irq (struct drm_i915_private *dev_priv, +uint32_t interrupt_mask, +uint32_t enabled_irq_mask) +{ +uint32_t new_val; + +assert_spin_locked (&dev_priv->irq_lock); + +if (dev_priv->pm.irqs_disabled) { +WARN (1 ,"IRQs disabled\n" ); +return ; + } + + new_val = dev_priv->pm_irq_mask; + new_val &= ~interrupt_mask; + new_val |= (~enabled_irq_mask & interrupt_mask); + +if (new_val != dev_priv->pm_irq_mask) { + dev_priv->pm_irq_mask = new_val; +I915_WRITE (GEN8_GT_IMR (2 ),I915_READ (GEN8_GT_IMR (2 )) | + dev_priv->pm_irq_mask); +POSTING_READ (GEN8_GT_IMR (2 )); + } +} + +void bdw_enable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask) +{ +bdw_update_pm_irq (dev_priv, mask, mask); +} +Quoted 9 lines
drivers/gpu/drm/i915/i915_irq.c static bool ivb_can_enable_err_int(struct drm_device *dev) · line 294
Quoted 6 lines
drivers/gpu/drm/i915/i915_irq.c +6
-0+void bdw_disable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask) +{ +bdw_update_pm_irq (dev_priv, mask,0 ); +} + +Unnecessary empty line.
Got it, thanks.
Quoted 20 lines
Quoted 20 lines
drivers/gpu/drm/i915/i915_irq.c +9
-2static bool cpt_can_enable_serr_int (struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private;@@ -1126,13 +1170,17 @@ static void gen6_pm_rps_work(struct work_struct *work)spin_lock_irq (&dev_priv->irq_lock); pm_iir = dev_priv->rps.pm_iir; dev_priv->rps.pm_iir =0 ; -/* Make sure not to corrupt PMIMR state used by ringbuffer code */ -snb_enable_pm_irq (dev_priv, dev_priv->pm_rps_events); +if (IS_BROADWELL (dev_priv->dev)) +bdw_enable_pm_irq (dev_priv, dev_priv->pm_rps_events); +else { +/* Make sure not to corrupt PMIMR state used by ringbuffer */ +snb_enable_pm_irq (dev_priv, dev_priv->pm_rps_events); +/* Make sure we didn't queue anything we're not going to +* process. */ +WARN_ON (pm_iir & ~dev_priv->pm_rps_events); + }spin_unlock_irq (&dev_priv->irq_lock);Quoted 5 lines
drivers/gpu/drm/i915/i915_irq.c static void gen6_pm_rps_work(struct work_struct *work) · line 1134
Quoted 2 lines
drivers/gpu/drm/i915/i915_irq.c +0
-2-/* Make sure we didn't queue anything we're not going to process. */ -WARN_ON (pm_iir & ~dev_priv->pm_rps_events);Isn't this WARN equally valid for bdw?
Quoted 7 lines
So first, let me just mention, this has moved slightly in my latest version of this patch, but it's still a valid question.
The answer is ambiguous actually. The WARN is always valid technically The distinction in BDW (at least for the time being) is that unlike pre-BDW, PM IIR isn't shared. I can add it, but for BDW, at least right now,
DRM_ERROR(or BUG) is more appropriate.I don't see a reason for bdw to be different here. There are other bits in the relevant IIR registers, so the same WARN should be fine.
Quoted 38 lines
Quoted 38 lines
Quoted 27 lines
drivers/gpu/drm/i915/i915_irq.c +13
-1-if ((pm_iir & dev_priv->pm_rps_events) ==0 )return ;@@ -1324,6 +1372,19 @@ static void snb_gt_irq_handler(struct drm_device *dev,ivybridge_parity_error_irq_handler (dev, gt_iir); } +static void gen8_rps_irq_handler (struct drm_i915_private *dev_priv,u32 pm_iir) +{ +if ((pm_iir & dev_priv->pm_rps_events) ==0 ) +return ; + +spin_lock (&dev_priv->irq_lock); + dev_priv->rps.pm_iir |= pm_iir & dev_priv->pm_rps_events; +bdw_disable_pm_irq (dev_priv, pm_iir & dev_priv->pm_rps_events); +spin_unlock (&dev_priv->irq_lock); + +queue_work (dev_priv->wq, &dev_priv->rps.work); +} +static irqreturn_t gen8_gt_irq_handler (struct drm_device *dev,struct drm_i915_private *dev_priv,u32 master_ctl)drivers/gpu/drm/i915/i915_irq.c static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev, · line 1428
Quoted 9 lines
drivers/gpu/drm/i915/i915_irq.c +6
-0@@ -1359,6 +1420,16 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,DRM_ERROR ("The master control interrupt lied (GT1)!\n" ); } +if (master_ctl &GEN8_GT_PM_IRQ ) { + tmp =I915_READ (GEN8_GT_IIR (2 )); +if (tmp & dev_priv->pm_rps_events) { + ret =IRQ_HANDLED ; +gen8_rps_irq_handler (dev_priv, tmp); +I915_WRITE (GEN8_GT_IIR (1 ), tmp & dev_priv->pm_rps_events);^^^^^^^^^^^^^^Quoted 4 lines
Quoted 2 lines
GEN8_GT_IIR(2)Nice catch, I guess I need to retest after this one.
I'm actually wondering how/if you managed to test this before Deepak's PMINTRMSK interrupt redirect bit fix. W/o that you shouldn't be getting any interrupt AFAICS.
Quoted 112 lines
Quoted 33 lines
Quoted 33 lines
drivers/gpu/drm/i915/i915_irq.c +4
-0+ }else +DRM_ERROR ("The master control interrupt lied (PM)!\n" ); + } +if (master_ctl &GEN8_GT_VECS_IRQ ) { tmp =I915_READ (GEN8_GT_IIR (3 ));if (tmp) {drivers/gpu/drm/i915/i915_reg.h +1
-0@@ -4193,6 +4193,7 @@ enum punit_power_well {#define GEN8_DE_PIPE_A_IRQ (1<<16)#define GEN8_DE_PIPE_IRQ (pipe) (1<<(16+pipe))#define GEN8_GT_VECS_IRQ (1<<6) +#define GEN8_GT_PM_IRQ (1<<4)#define GEN8_GT_VCS2_IRQ (1<<3)#define GEN8_GT_VCS1_IRQ (1<<2)#define GEN8_GT_BCS_IRQ (1<<1)drivers/gpu/drm/i915/intel_drv.h +2
-0@@ -650,10 +650,11 @@ void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);void ilk_disable_gt_irq (struct drm_i915_private *dev_priv,uint32_t mask);void snb_enable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask);void snb_disable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask); +void bdw_enable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask); +void bdw_disable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask);void intel_runtime_pm_disable_interrupts (struct drm_device *dev);void intel_runtime_pm_restore_interrupts (struct drm_device *dev);Quoted 5 lines
drivers/gpu/drm/i915/intel_drv.h void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask); · line 656
Quoted 1 line
drivers/gpu/drm/i915/intel_drv.h +0
-1-Unrelated whitespace change. IIRC someone left these two empty lines here on purpose.
Got it, thanks.
Quoted 11 lines
Quoted 11 lines
drivers/gpu/drm/i915/intel_drv.h +0
-0/* intel_crt.c */ void intel_crt_init (struct drm_device *dev);drivers/gpu/drm/i915/intel_pm.c +0
-0@@ -3210,6 +3210,25 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)trace_intel_gpu_freq_change (vlv_gpu_freq (dev_priv, val)); }Quoted 14 lines
drivers/gpu/drm/i915/intel_pm.c void valleyview_set_rps(struct drm_device *dev, u8 val) · line 3223
Quoted 11 lines
drivers/gpu/drm/i915/intel_pm.c +11
-0+static void gen8_disable_rps_interrupts (struct drm_device *dev) +{ +struct drm_i915_private *dev_priv = dev->dev_private; + +I915_WRITE (GEN6_PMINTRMSK ,0xffffffff ); +I915_WRITE (GEN8_GT_IER (2 ),I915_READ (GEN8_GT_IER (2 )) & + ~dev_priv->pm_rps_events); +/* Complete PM interrupt masking here doesn't race with the rps work +* item again unmasking PM interrupts because that is using a different +* register (PMIMR) to mask PM interrupts. The only risk is in leaving +* stale bits in PMIIR and PMIMR which gen6_enable_rps will clean up. */ This comment would need a bit of update for gen8.
I've killed this comment locally. Somewhere I had written that we can rewrite some of this for gen8, but I seem to have lost that in the sands of time.
Quoted 27 lines
Quoted 27 lines
drivers/gpu/drm/i915/intel_pm.c +12
-1+ +spin_lock_irq (&dev_priv->irq_lock); + dev_priv->rps.pm_iir =0 ; +spin_unlock_irq (&dev_priv->irq_lock); + +I915_WRITE (GEN8_GT_IIR (2 ), dev_priv->pm_rps_events); +} +static void gen6_disable_rps_interrupts (struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private;@@ -3236,7 +3255,10 @@ static void gen6_disable_rps(struct drm_device *dev)I915_WRITE (GEN6_RC_CONTROL ,0 );I915_WRITE (GEN6_RPNSWREQ ,1 <<31 ); -gen6_disable_rps_interrupts (dev); +if (IS_BROADWELL (dev)) +gen8_disable_rps_interrupts (dev); +else +gen6_disable_rps_interrupts (dev); }static void valleyview_disable_rps (struct drm_device *dev)@@ -3276,6 +3298,18 @@ int intel_enable_rc6(const struct drm_device *dev)return INTEL_RC6_ENABLE ; }Quoted 13 lines
drivers/gpu/drm/i915/intel_pm.c int intel_enable_rc6(const struct drm_device *dev) · line 3308
Quoted 8 lines
drivers/gpu/drm/i915/intel_pm.c +8
-0+static void gen8_enable_rps_interrupts (struct drm_device *dev) +{ +struct drm_i915_private *dev_priv = dev->dev_private; + +spin_lock_irq (&dev_priv->irq_lock); +WARN_ON (dev_priv->rps.pm_iir); +bdw_enable_pm_irq (dev_priv, dev_priv->pm_rps_events); +I915_WRITE (GEN8_GT_IIR (2 ), dev_priv->pm_rps_events);The order of "unmask first then clear" seems just wrong. The same issues is already present in the gen6 code however. So this should be fixed in both places, or if it's like that on purpose it needs a comment.
Quoted 2 lines
Well, if you want to fix it, we can do it in a separate patch. Although I don't think I agree. Why does it seem wrong to you?
If you enable before clearing and there's already an IIR bit high the interrupt should trigger immediately before you get to clear it. And then on a SMP machine the write to clear it would even race with the interrupt handler if there's no locking. So the clearing after enabling+unmasking is pointless. The normal approach for enabling interrupts is to clear first and then enable.
Quoted 8 lines
Hmm. Actually here we alreay have the interrupts enabled in IER, so if any relevant IIR bit would be high, we should have already gotten the interrupt. But since we always clear the IIR after masking the interrupts in IMR, we should never have any relevant IIR bit set here.
So looks to me like we can just drop the IIR writes here. They serve no purpose other than racing with the interrupt handler if a real interrupt happens just after we've unmasked it.
I think we should create a separate patch to fix this issue for both gen6 and gen8
Quoted 21 lines
Quoted 21 lines
Quoted 6 lines
drivers/gpu/drm/i915/intel_pm.c int intel_enable_rc6(const struct drm_device *dev) · line 3310
Quoted 3 lines
drivers/gpu/drm/i915/intel_pm.c +2
-0+spin_unlock_irq (&dev_priv->irq_lock); +Useless empty line.
If I was in an arguing mood, I would do, but fine.
Quoted 12 lines
Quoted 12 lines
drivers/gpu/drm/i915/intel_pm.c +3
-1+} +static void gen6_enable_rps_interrupts (struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private;@@ -3378,7 +3412,7 @@ static void gen8_enable_rps(struct drm_device *dev)gen6_set_rps (dev, (I915_READ (GEN6_GT_PERF_STATUS ) &0xff00 ) >>8 ); -gen6_enable_rps_interrupts (dev); +gen8_enable_rps_interrupts (dev);
Quoted 8 lines
Quoted 7 lines
Quoted 2 lines
drivers/gpu/drm/i915/intel_pm.c static void gen8_enable_rps(struct drm_device *dev) · line 3418
Quoted 2 lines
drivers/gpu/drm/i915/intel_pm.c +0
-0gen6_gt_force_wake_put (dev_priv,FORCEWAKE_ALL ); }Thanks for the review.
--
Ben Widawsky, Intel Open Source Technology Center
Thanks for the review.
Re: [PATCH 1/2] drm/i915/bdw: Implement a basic PM interrupt handler
In reply to Ville SyrjäläQuoted 317 lines · Ville Syrjälä
Quoted 136 lines · Ben Widawsky
Quoted 30 lines · Ville Syrjälä
Quoted 29 lines · On Mon, Apr 14, 2014 at 10:41:14PM +0530, [email protected] wrote:
From: Ben Widawsky <[email protected]>
Almost all of it is reusable from the existing code. The primary difference is we need to do even less in the interrupt handler, since interrupts are not shared in the same way.
The patch is mostly a copy-paste of the existing snb+ code, with updates to the relevant parts requiring changes to the interrupt handling. As such it /should/ be relatively trivial. It's highly likely that I missed some places where I need a gen8 version of the PM interrupts, but it has become invisible to me by now.
This patch could probably be split into adding the new functions, followed by actually handling the interrupts. Since the code is currently disabled (and broken) I think the patch stands better by itself.
v2: Move the commit about not touching the ringbuffer interrupt to the snb_* function where it belongs (Rodrigo)
v3: Rebased on Paulo's runtime PM changes
v4: Not well validated, but rebase on
commit730488b2eddded4497f63f70867b1256cd9e117cAuthor: Paulo Zanoni <[email protected]> Date: Fri Mar 7 20:12:32 2014 -0300drm/i915: kill dev_priv->pm.regsaveQuoted 9 lines
Quoted 6 lines
v5: Rebased on latest code base. (Deepak)
Signed-off-by Ben Widawsky <[email protected]>Conflicts:
drivers/gpu/drm/i915/i915_irq.cIIRC Daniel doesn't like these conflict markers. So should be dropped.
I like the conflict markers generally. Daniel can kill it if he likes, but thanks for the input. I've killed it this time around, but I don't plan on it for the future.
Quoted 53 lines
Quoted 53 lines
--- drivers/gpu/drm/i915/i915_irq.c | 81 +++++++++++++++++++++++++++++++++++++--- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_drv.h | 3 +- drivers/gpu/drm/i915/intel_pm.c | 38 ++++++++++++++++++- 4 files changed, 115 insertions(+), 8 deletions(-)drivers/gpu/drm/i915/i915_irq.c +38
-0@@ -248,6 +248,50 @@ static bool ivb_can_enable_err_int(struct drm_device *dev)return true; } +/** +* bdw_update_pm_irq - update GT interrupt 2 +* @dev_priv: driver private +* @interrupt_mask: mask of interrupt bits to update +* @enabled_irq_mask: mask of interrupt bits to enable +* +* Copied from the snb function, updated with relevant register offsets +*/ +static void bdw_update_pm_irq (struct drm_i915_private *dev_priv, +uint32_t interrupt_mask, +uint32_t enabled_irq_mask) +{ +uint32_t new_val; + +assert_spin_locked (&dev_priv->irq_lock); + +if (dev_priv->pm.irqs_disabled) { +WARN (1 ,"IRQs disabled\n" ); +return ; + } + + new_val = dev_priv->pm_irq_mask; + new_val &= ~interrupt_mask; + new_val |= (~enabled_irq_mask & interrupt_mask); + +if (new_val != dev_priv->pm_irq_mask) { + dev_priv->pm_irq_mask = new_val; +I915_WRITE (GEN8_GT_IMR (2 ),I915_READ (GEN8_GT_IMR (2 )) | + dev_priv->pm_irq_mask); +POSTING_READ (GEN8_GT_IMR (2 )); + } +} + +void bdw_enable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask) +{ +bdw_update_pm_irq (dev_priv, mask, mask); +} +Quoted 9 lines
drivers/gpu/drm/i915/i915_irq.c static bool ivb_can_enable_err_int(struct drm_device *dev) · line 294
Quoted 6 lines
drivers/gpu/drm/i915/i915_irq.c +6
-0+void bdw_disable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask) +{ +bdw_update_pm_irq (dev_priv, mask,0 ); +} + +Unnecessary empty line.
Got it, thanks.
Quoted 20 lines
Quoted 20 lines
drivers/gpu/drm/i915/i915_irq.c +9
-2static bool cpt_can_enable_serr_int (struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private;@@ -1126,13 +1170,17 @@ static void gen6_pm_rps_work(struct work_struct *work)spin_lock_irq (&dev_priv->irq_lock); pm_iir = dev_priv->rps.pm_iir; dev_priv->rps.pm_iir =0 ; -/* Make sure not to corrupt PMIMR state used by ringbuffer code */ -snb_enable_pm_irq (dev_priv, dev_priv->pm_rps_events); +if (IS_BROADWELL (dev_priv->dev)) +bdw_enable_pm_irq (dev_priv, dev_priv->pm_rps_events); +else { +/* Make sure not to corrupt PMIMR state used by ringbuffer */ +snb_enable_pm_irq (dev_priv, dev_priv->pm_rps_events); +/* Make sure we didn't queue anything we're not going to +* process. */ +WARN_ON (pm_iir & ~dev_priv->pm_rps_events); + }spin_unlock_irq (&dev_priv->irq_lock);Quoted 5 lines
drivers/gpu/drm/i915/i915_irq.c static void gen6_pm_rps_work(struct work_struct *work) · line 1134
Quoted 2 lines
drivers/gpu/drm/i915/i915_irq.c +0
-2-/* Make sure we didn't queue anything we're not going to process. */ -WARN_ON (pm_iir & ~dev_priv->pm_rps_events);Isn't this WARN equally valid for bdw?
Quoted 7 lines
So first, let me just mention, this has moved slightly in my latest version of this patch, but it's still a valid question.
The answer is ambiguous actually. The WARN is always valid technically The distinction in BDW (at least for the time being) is that unlike pre-BDW, PM IIR isn't shared. I can add it, but for BDW, at least right now,
DRM_ERROR(or BUG) is more appropriate.I don't see a reason for bdw to be different here. There are other bits in the relevant IIR registers, so the same WARN should be fine.
Quoted 38 lines
Quoted 38 lines
Quoted 27 lines
drivers/gpu/drm/i915/i915_irq.c +13
-1-if ((pm_iir & dev_priv->pm_rps_events) ==0 )return ;@@ -1324,6 +1372,19 @@ static void snb_gt_irq_handler(struct drm_device *dev,ivybridge_parity_error_irq_handler (dev, gt_iir); } +static void gen8_rps_irq_handler (struct drm_i915_private *dev_priv,u32 pm_iir) +{ +if ((pm_iir & dev_priv->pm_rps_events) ==0 ) +return ; + +spin_lock (&dev_priv->irq_lock); + dev_priv->rps.pm_iir |= pm_iir & dev_priv->pm_rps_events; +bdw_disable_pm_irq (dev_priv, pm_iir & dev_priv->pm_rps_events); +spin_unlock (&dev_priv->irq_lock); + +queue_work (dev_priv->wq, &dev_priv->rps.work); +} +static irqreturn_t gen8_gt_irq_handler (struct drm_device *dev,struct drm_i915_private *dev_priv,u32 master_ctl)drivers/gpu/drm/i915/i915_irq.c static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev, · line 1428
Quoted 9 lines
drivers/gpu/drm/i915/i915_irq.c +6
-0@@ -1359,6 +1420,16 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,DRM_ERROR ("The master control interrupt lied (GT1)!\n" ); } +if (master_ctl &GEN8_GT_PM_IRQ ) { + tmp =I915_READ (GEN8_GT_IIR (2 )); +if (tmp & dev_priv->pm_rps_events) { + ret =IRQ_HANDLED ; +gen8_rps_irq_handler (dev_priv, tmp); +I915_WRITE (GEN8_GT_IIR (1 ), tmp & dev_priv->pm_rps_events);^^^^^^^^^^^^^^Quoted 4 lines
Quoted 2 lines
GEN8_GT_IIR(2)Nice catch, I guess I need to retest after this one.
I'm actually wondering how/if you managed to test this before Deepak's PMINTRMSK interrupt redirect bit fix. W/o that you shouldn't be getting any interrupt AFAICS.
Quoted 112 lines
Quoted 33 lines
Quoted 33 lines
drivers/gpu/drm/i915/i915_irq.c +4
-0+ }else +DRM_ERROR ("The master control interrupt lied (PM)!\n" ); + } +if (master_ctl &GEN8_GT_VECS_IRQ ) { tmp =I915_READ (GEN8_GT_IIR (3 ));if (tmp) {drivers/gpu/drm/i915/i915_reg.h +1
-0@@ -4193,6 +4193,7 @@ enum punit_power_well {#define GEN8_DE_PIPE_A_IRQ (1<<16)#define GEN8_DE_PIPE_IRQ (pipe) (1<<(16+pipe))#define GEN8_GT_VECS_IRQ (1<<6) +#define GEN8_GT_PM_IRQ (1<<4)#define GEN8_GT_VCS2_IRQ (1<<3)#define GEN8_GT_VCS1_IRQ (1<<2)#define GEN8_GT_BCS_IRQ (1<<1)drivers/gpu/drm/i915/intel_drv.h +2
-0@@ -650,10 +650,11 @@ void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);void ilk_disable_gt_irq (struct drm_i915_private *dev_priv,uint32_t mask);void snb_enable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask);void snb_disable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask); +void bdw_enable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask); +void bdw_disable_pm_irq (struct drm_i915_private *dev_priv,uint32_t mask);void intel_runtime_pm_disable_interrupts (struct drm_device *dev);void intel_runtime_pm_restore_interrupts (struct drm_device *dev);Quoted 5 lines
drivers/gpu/drm/i915/intel_drv.h void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask); · line 656
Quoted 1 line
drivers/gpu/drm/i915/intel_drv.h +0
-1-Unrelated whitespace change. IIRC someone left these two empty lines here on purpose.
Got it, thanks.
Quoted 11 lines
Quoted 11 lines
drivers/gpu/drm/i915/intel_drv.h +0
-0/* intel_crt.c */ void intel_crt_init (struct drm_device *dev);drivers/gpu/drm/i915/intel_pm.c +0
-0@@ -3210,6 +3210,25 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)trace_intel_gpu_freq_change (vlv_gpu_freq (dev_priv, val)); }Quoted 14 lines
drivers/gpu/drm/i915/intel_pm.c void valleyview_set_rps(struct drm_device *dev, u8 val) · line 3223
Quoted 11 lines
drivers/gpu/drm/i915/intel_pm.c +11
-0+static void gen8_disable_rps_interrupts (struct drm_device *dev) +{ +struct drm_i915_private *dev_priv = dev->dev_private; + +I915_WRITE (GEN6_PMINTRMSK ,0xffffffff ); +I915_WRITE (GEN8_GT_IER (2 ),I915_READ (GEN8_GT_IER (2 )) & + ~dev_priv->pm_rps_events); +/* Complete PM interrupt masking here doesn't race with the rps work +* item again unmasking PM interrupts because that is using a different +* register (PMIMR) to mask PM interrupts. The only risk is in leaving +* stale bits in PMIIR and PMIMR which gen6_enable_rps will clean up. */ This comment would need a bit of update for gen8.
I've killed this comment locally. Somewhere I had written that we can rewrite some of this for gen8, but I seem to have lost that in the sands of time.
Quoted 27 lines
Quoted 27 lines
drivers/gpu/drm/i915/intel_pm.c +12
-1+ +spin_lock_irq (&dev_priv->irq_lock); + dev_priv->rps.pm_iir =0 ; +spin_unlock_irq (&dev_priv->irq_lock); + +I915_WRITE (GEN8_GT_IIR (2 ), dev_priv->pm_rps_events); +} +static void gen6_disable_rps_interrupts (struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private;@@ -3236,7 +3255,10 @@ static void gen6_disable_rps(struct drm_device *dev)I915_WRITE (GEN6_RC_CONTROL ,0 );I915_WRITE (GEN6_RPNSWREQ ,1 <<31 ); -gen6_disable_rps_interrupts (dev); +if (IS_BROADWELL (dev)) +gen8_disable_rps_interrupts (dev); +else +gen6_disable_rps_interrupts (dev); }static void valleyview_disable_rps (struct drm_device *dev)@@ -3276,6 +3298,18 @@ int intel_enable_rc6(const struct drm_device *dev)return INTEL_RC6_ENABLE ; }Quoted 13 lines
drivers/gpu/drm/i915/intel_pm.c int intel_enable_rc6(const struct drm_device *dev) · line 3308
Quoted 8 lines
drivers/gpu/drm/i915/intel_pm.c +8
-0+static void gen8_enable_rps_interrupts (struct drm_device *dev) +{ +struct drm_i915_private *dev_priv = dev->dev_private; + +spin_lock_irq (&dev_priv->irq_lock); +WARN_ON (dev_priv->rps.pm_iir); +bdw_enable_pm_irq (dev_priv, dev_priv->pm_rps_events); +I915_WRITE (GEN8_GT_IIR (2 ), dev_priv->pm_rps_events);The order of "unmask first then clear" seems just wrong. The same issues is already present in the gen6 code however. So this should be fixed in both places, or if it's like that on purpose it needs a comment.
Quoted 2 lines
Well, if you want to fix it, we can do it in a separate patch. Although I don't think I agree. Why does it seem wrong to you?
If you enable before clearing and there's already an IIR bit high the interrupt should trigger immediately before you get to clear it. And then on a SMP machine the write to clear it would even race with the interrupt handler if there's no locking. So the clearing after enabling+unmasking is pointless. The normal approach for enabling interrupts is to clear first and then enable.
Quoted 8 lines
Hmm. Actually here we alreay have the interrupts enabled in IER, so if any relevant IIR bit would be high, we should have already gotten the interrupt. But since we always clear the IIR after masking the interrupts in IMR, we should never have any relevant IIR bit set here.
So looks to me like we can just drop the IIR writes here. They serve no purpose other than racing with the interrupt handler if a real interrupt happens just after we've unmasked it.
I think we should create a separate patch to fix this issue for both gen6 and gen8
Quoted 21 lines
Quoted 21 lines
Quoted 6 lines
drivers/gpu/drm/i915/intel_pm.c int intel_enable_rc6(const struct drm_device *dev) · line 3310
Quoted 3 lines
drivers/gpu/drm/i915/intel_pm.c +2
-0+spin_unlock_irq (&dev_priv->irq_lock); +Useless empty line.
If I was in an arguing mood, I would do, but fine.
Quoted 12 lines
Quoted 12 lines
drivers/gpu/drm/i915/intel_pm.c +3
-1+} +static void gen6_enable_rps_interrupts (struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private;@@ -3378,7 +3412,7 @@ static void gen8_enable_rps(struct drm_device *dev)gen6_set_rps (dev, (I915_READ (GEN6_GT_PERF_STATUS ) &0xff00 ) >>8 ); -gen6_enable_rps_interrupts (dev); +gen8_enable_rps_interrupts (dev);
Quoted 8 lines
Quoted 7 lines
Quoted 2 lines
drivers/gpu/drm/i915/intel_pm.c static void gen8_enable_rps(struct drm_device *dev) · line 3418
Quoted 2 lines
drivers/gpu/drm/i915/intel_pm.c +0
-0gen6_gt_force_wake_put (dev_priv,FORCEWAKE_ALL ); }Thanks for the review.
--
Ben Widawsky, Intel Open Source Technology Center
Thanks for the review.
PATCH 2/2 drm/i915: Enable PM Interrupts target via Display Interface.
In reply to [email protected]From: Deepak S <[email protected]>
In BDW, Apart from unmasking up/down threshold interrupts. we need to umask bit 32 of PM_INTRMASK to route interrupts to target via Display Interface.
--- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_pm.c | 2 ++ 2 files changed, 3 insertions(+)
drivers/gpu/drm/i915/i915_reg.h +1 -0
@@ -5105,6 +5105,7 @@ enum punit_power_well {#define GEN6_RC6p_THRESHOLD 0xA0BC#define GEN6_RC6pp_THRESHOLD 0xA0C0#define GEN6_PMINTRMSK 0xA168 +#define GEN8_PMINTR_REDIRECT_TO_NON_DISP 0x7FFFFFFF#define GEN6_PMISR 0x44020#define GEN6_PMIMR 0x44024/* rps_lock */
drivers/gpu/drm/i915/intel_pm.c +2 -0
@@ -3066,6 +3066,8 @@ static u32 gen6_rps_pm_mask(struct drm_i915_private *dev_priv, u8 val)if (INTEL_INFO (dev_priv->dev)->gen <=7 && !IS_HASWELL (dev_priv->dev)) mask |=GEN6_PM_RP_UP_EI_EXPIRED ; + mask &=GEN8_PMINTR_REDIRECT_TO_NON_DISP ; +return ~mask; }
Signature · 1 line
1.8.5.2
Re: [PATCH 2/2] drm/i915: Enable PM Interrupts target via Display Interface.
Quoted 17 lines · On Mon, Apr 14, 2014 at 10:41:15PM +0530, [email protected] wrote:
From: Deepak S <[email protected]>
In BDW, Apart from unmasking up/down threshold interrupts. we need to umask bit 32 of
PM_INTRMASKto route interrupts to target via Display Interface.Signed-off-by Deepak S <[email protected]>--- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_pm.c | 2 ++ 2 files changed, 3 insertions(+)drivers/gpu/drm/i915/i915_reg.h +0
-0
drivers/gpu/drm/i915/i915_reg.h enum punit_power_well { · line 5108
Quoted 4 lines
drivers/gpu/drm/i915/i915_reg.h +1
-0@@ -5105,6 +5105,7 @@ enum punit_power_well {#define GEN6_RC6p_THRESHOLD 0xA0BC#define GEN6_RC6pp_THRESHOLD 0xA0C0#define GEN6_PMINTRMSK 0xA168 +#define GEN8_PMINTR_REDIRECT_TO_NON_DISP 0x7FFFFFFF
Defining is as (1<<31) would make more sense to me.
Quoted 23 lines
drivers/gpu/drm/i915/i915_reg.h +0
-0#define GEN6_PMISR 0x44020#define GEN6_PMIMR 0x44024/* rps_lock */ drivers/gpu/drm/i915/intel_pm.c +2
-0@@ -3066,6 +3066,8 @@ static u32 gen6_rps_pm_mask(struct drm_i915_private *dev_priv, u8 val)if (INTEL_INFO (dev_priv->dev)->gen <=7 && !IS_HASWELL (dev_priv->dev)) mask |=GEN6_PM_RP_UP_EI_EXPIRED ; + mask &=GEN8_PMINTR_REDIRECT_TO_NON_DISP ; +return ~mask; }Signature · 6 lines
1.8.5.2 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 2 lines
Ville Syrjälä Intel OTC
Re: [PATCH 2/2] drm/i915: Enable PM Interrupts target via Display Interface.
Quoted 36 lines · Ville Syrjälä
Quoted 17 lines · On Mon, Apr 14, 2014 at 10:41:15PM +0530, [email protected] wrote:
From: Deepak S <[email protected]>
In BDW, Apart from unmasking up/down threshold interrupts. we need to umask bit 32 of
PM_INTRMASKto route interrupts to target via Display Interface.Signed-off-by Deepak S <[email protected]>--- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_pm.c | 2 ++ 2 files changed, 3 insertions(+)drivers/gpu/drm/i915/i915_reg.h +0
-0drivers/gpu/drm/i915/i915_reg.h enum punit_power_well { · line 5108
Quoted 4 lines
drivers/gpu/drm/i915/i915_reg.h +1
-0@@ -5105,6 +5105,7 @@ enum punit_power_well {#define GEN6_RC6p_THRESHOLD 0xA0BC#define GEN6_RC6pp_THRESHOLD 0xA0C0#define GEN6_PMINTRMSK 0xA168 +#define GEN8_PMINTR_REDIRECT_TO_NON_DISP 0x7FFFFFFFDefining is as (1<<31) would make more sense to me.
Quoted 11 lines
drivers/gpu/drm/i915/i915_reg.h +0
-0#define GEN6_PMISR 0x44020#define GEN6_PMIMR 0x44024/* rps_lock */ drivers/gpu/drm/i915/intel_pm.c +0
-0@@ -3066,6 +3066,8 @@ static u32 gen6_rps_pm_mask(struct drm_i915_private *dev_priv, u8 val)if (INTEL_INFO (dev_priv->dev)->gen <=7 && !IS_HASWELL (dev_priv->dev)) mask |=GEN6_PM_RP_UP_EI_EXPIRED ;
Quoted 3 lines
drivers/gpu/drm/i915/intel_pm.c static u32 gen6_rps_pm_mask(struct drm_i915_private *dev_priv, u8 val) · line 3071
Quoted 3 lines
drivers/gpu/drm/i915/intel_pm.c +2
-0+ mask &=GEN8_PMINTR_REDIRECT_TO_NON_DISP ; +return ~mask;
Oh and just noticed this doesn't actually do anything. & must come after ~ to get the expected result.
Quoted 13 lines
Quoted 9 lines
drivers/gpu/drm/i915/intel_pm.c +0
-0}Signature · 6 lines
1.8.5.2 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfxSignature · 2 lines
Ville Syrjälä Intel OTC
Signature · 2 lines
Ville Syrjälä Intel OTC
Re: [PATCH 2/2] drm/i915: Enable PM Interrupts target via Display Interface.
Quoted 36 lines · Ville Syrjälä
Quoted 35 lines · Ville Syrjälä
Quoted 17 lines · On Mon, Apr 14, 2014 at 10:41:15PM +0530, [email protected] wrote:
From: Deepak S <[email protected]>
In BDW, Apart from unmasking up/down threshold interrupts. we need to umask bit 32 of
PM_INTRMASKto route interrupts to target via Display Interface.Signed-off-by Deepak S <[email protected]>--- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_pm.c | 2 ++ 2 files changed, 3 insertions(+)drivers/gpu/drm/i915/i915_reg.h +0
-0drivers/gpu/drm/i915/i915_reg.h enum punit_power_well { · line 5108
Quoted 4 lines
drivers/gpu/drm/i915/i915_reg.h +1
-0@@ -5105,6 +5105,7 @@ enum punit_power_well {#define GEN6_RC6p_THRESHOLD 0xA0BC#define GEN6_RC6pp_THRESHOLD 0xA0C0#define GEN6_PMINTRMSK 0xA168 +#define GEN8_PMINTR_REDIRECT_TO_NON_DISP 0x7FFFFFFFDefining is as (1<<31) would make more sense to me.
Quoted 11 lines
drivers/gpu/drm/i915/i915_reg.h +0
-0#define GEN6_PMISR 0x44020#define GEN6_PMIMR 0x44024/* rps_lock */ drivers/gpu/drm/i915/intel_pm.c +0
-0@@ -3066,6 +3066,8 @@ static u32 gen6_rps_pm_mask(struct drm_i915_private *dev_priv, u8 val)if (INTEL_INFO (dev_priv->dev)->gen <=7 && !IS_HASWELL (dev_priv->dev)) mask |=GEN6_PM_RP_UP_EI_EXPIRED ;
Quoted 18 lines
Quoted 3 lines
drivers/gpu/drm/i915/intel_pm.c static u32 gen6_rps_pm_mask(struct drm_i915_private *dev_priv, u8 val) · line 3071
Quoted 3 lines
drivers/gpu/drm/i915/intel_pm.c +2
-0+ mask &=GEN8_PMINTR_REDIRECT_TO_NON_DISP ; +return ~mask;Oh and just noticed this doesn't actually do anything. & must come after ~ to get the expected result.
Quoted 12 lines
Quoted 9 lines
drivers/gpu/drm/i915/intel_pm.c +0
-0}Signature · 6 lines
1.8.5.2 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfxSignature · 2 lines
Ville Syrjälä Intel OTC
Thanks for review