Re: [Intel-gfx] [PATCH 2/5] drm/i915/bdw: Implement a basic PM interrupt handler

2014-05-16 Thread Daniel Vetter 
On Fri, May 16, 2014 at 01:38:18AM +, O'Rourke, Tom wrote:
 +static void gen8_disable_rps_interrupts(struct drm_device *dev) {
 +struct drm_i915_private *dev_priv = dev-dev_private;
 +
 +I915_WRITE(GEN6_PMINTRMSK, 0x);
 
 [TOR:] Please note that for Broadwell, bit 31 in GEN6_PMINTRMSK is not an 
 interrupt disable bit.
 In drm/i915: Enable PM Interrupts target via Display Interface. this bit is 
 defined as:
 +#define GEN8_PMINTR_REDIRECT_TO_NON_DISP (131)
 
 Writing this bit here could have unintended consequences.

Hm, we seem to unconditionally set this bit on gen8 anyway. Still harmful?
Mika, Ville?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/5] drm/i915/bdw: Implement a basic PM interrupt handler

2014-05-16 Thread Ville Syrjälä 
On Fri, May 16, 2014 at 11:09:35AM +0200, Daniel Vetter wrote:
 On Fri, May 16, 2014 at 01:38:18AM +, O'Rourke, Tom wrote:
  +static void gen8_disable_rps_interrupts(struct drm_device *dev) {
  +  struct drm_i915_private *dev_priv = dev-dev_private;
  +
  +  I915_WRITE(GEN6_PMINTRMSK, 0x);
  
  [TOR:] Please note that for Broadwell, bit 31 in GEN6_PMINTRMSK is not an 
  interrupt disable bit.
  In drm/i915: Enable PM Interrupts target via   Display Interface. 
  this bit is defined as:
  +#define GEN8_PMINTR_REDIRECT_TO_NON_DISP   (131)
  
  Writing this bit here could have unintended consequences.
 
 Hm, we seem to unconditionally set this bit on gen8 anyway. Still harmful?
 Mika, Ville?

I was thinking that since we mask all the interrupts anyway it
doesn't really matter which way we set the bit here. But I'm
not actually sure what happens if there's an already pending
interrupt when we flip the bit. That is I have no idea if it
could raise an interrupt on the non-disp side or not.

So leaving the bit as zero here might be the safer option, and
at least it would be more consistent with the rest of the gen8
pm irq code. So if someone wants to make that change, my r-b
will still stand.

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/5] drm/i915/bdw: Implement a basic PM interrupt handler

2014-05-16 Thread Daniel Vetter 
On Fri, May 16, 2014 at 12:46:00PM +0300, Ville Syrjälä wrote:
 On Fri, May 16, 2014 at 11:09:35AM +0200, Daniel Vetter wrote:
  On Fri, May 16, 2014 at 01:38:18AM +, O'Rourke, Tom wrote:
   +static void gen8_disable_rps_interrupts(struct drm_device *dev) {
   +struct drm_i915_private *dev_priv = dev-dev_private;
   +
   +I915_WRITE(GEN6_PMINTRMSK, 0x);
   
   [TOR:] Please note that for Broadwell, bit 31 in GEN6_PMINTRMSK is not an 
   interrupt disable bit.
   In drm/i915: Enable PM Interrupts target via Display Interface. 
   this bit is defined as:
   +#define GEN8_PMINTR_REDIRECT_TO_NON_DISP (131)
   
   Writing this bit here could have unintended consequences.
  
  Hm, we seem to unconditionally set this bit on gen8 anyway. Still harmful?
  Mika, Ville?
 
 I was thinking that since we mask all the interrupts anyway it
 doesn't really matter which way we set the bit here. But I'm
 not actually sure what happens if there's an already pending
 interrupt when we flip the bit. That is I have no idea if it
 could raise an interrupt on the non-disp side or not.
 
 So leaving the bit as zero here might be the safer option, and
 at least it would be more consistent with the rest of the gen8
 pm irq code. So if someone wants to make that change, my r-b
 will still stand.

Imo better as a follow-up patch with the Bspec quote above in the commit
message.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/5] drm/i915/bdw: Implement a basic PM interrupt handler

2014-05-15 Thread O'Rourke, Tom 
+static void gen8_disable_rps_interrupts(struct drm_device *dev) {
+  struct drm_i915_private *dev_priv = dev-dev_private;
+
+  I915_WRITE(GEN6_PMINTRMSK, 0x);

[TOR:] Please note that for Broadwell, bit 31 in GEN6_PMINTRMSK is not an 
interrupt disable bit.
In drm/i915: Enable PM Interrupts target via   Display Interface. this bit is 
defined as:
+#define GEN8_PMINTR_REDIRECT_TO_NON_DISP   (131)

Writing this bit here could have unintended consequences.

Thanks,
Tom

+  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 (GEN8_GT_IMR(2)) to mask PM interrupts. The only risk is in
+   * leaving stale bits in GEN8_GT_IIR(2) and GEN8_GT_IMR(2) which
+   * gen8_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); }
+
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx