| Message ID | 1373661807-17184-2-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 12, 2013 at 10:43:26PM +0200, Daniel Vetter wrote: > Again extract a common helper. For the postinstall hook things are a > bit more complicated since we have more cases on ilk-hsw/vlv here. > > But since vlv was clearly broken by failing to initialize > dev_priv->gt_irq_mask correctly the shared code is clearly justified. > > Also kill the PMIER setting in the async rps enable work. I should > have been save, but also clearly looked rather fragile. PMIER setup is Can you fix up this sentence. It's a bit weird, and I'm not positive what you're trying to say. > not all down in the irq pre/postinstall hooks. > > With this we now have the usual interrupt register sequence for GT/PM > irq registers: > > - IER is setup once with all the interrupts we ever need in the > postinstall hook and never touched again. Exceptions are SDEIER, > which is touched in the preinstall hook (when the irq handler isn't > enabled) and then only from the irq handler. And DEIER/VLV_IER with > is used in the irq handler but also written to once in the > postinstall hook. But since that write is essentially what enables > the interrupt and we should always have MSI interrupts we should be > save. In case we ever have non-MSI interrupts we'd be screwed. > > - IIR is cleared in the postinstall hook before we enable/unmask the > respective interrupt sources. Hence we can't steal an interrupt > event an accidentally trigger the spurious interrupt logic in the > core kernel. Note that after some discussion with Ben Widawsky we > think that we actually should clear the IIR registers in the > preinstall hook. But doing that is a much larger patch series. > > - IMR regs are (usually) all masked off. Those are the only regs > changed at runtime, which is all protected by dev_priv->irq_lock. > This comment block is really nice. Maybe supplement it with explanation about how the ring interrupts work, and shove it in i915_irq.c? > This unification also kills the cargo-culted read-modify-write PM > register setup for VECS. Interrupt setup is done without userspace > being able to interfere, so we better know what values we want to put > into those registers. RMW cycles otoh are really good at papering over > races, until stuff magically blows up and no one has a clue why. > > v2: Touch the gen6+ PM interrupt registers only on gen6+. > > v3: Improve the commit message to more clearly spell out why we want > to unify the code and what exactly changes. > > Cc: Ben Widawsky <ben@bwidawsk.net> > Cc: Paulo Zanoni <przanoni@gmail.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_irq.c | 92 +++++++++++++++++++---------------------- > drivers/gpu/drm/i915/intel_pm.c | 4 -- > 2 files changed, 42 insertions(+), 54 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index d5c3bef..ba61bc7 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -2725,6 +2725,45 @@ static void ibx_irq_postinstall(struct drm_device *dev) > I915_WRITE(SDEIMR, ~mask); > } > > +static void gen5_gt_irq_postinstall(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + u32 pm_irqs, gt_irqs; > + > + pm_irqs = gt_irqs = 0; > + > + dev_priv->gt_irq_mask = ~0; > + if (HAS_L3_GPU_CACHE(dev)) { > + dev_priv->gt_irq_mask = ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT; > + gt_irqs |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT; > + } Maybe while you're doing this, explain why the L3 parity interrupt is special, in a comment. It's the only one to touch dev_priv->gt_irq_mask > + > + gt_irqs |= GT_RENDER_USER_INTERRUPT; > + if (IS_GEN5(dev)) { > + gt_irqs |= GT_RENDER_PIPECTL_NOTIFY_INTERRUPT | > + ILK_BSD_USER_INTERRUPT; > + } else { > + gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT; > + } > + > + I915_WRITE(GTIIR, I915_READ(GTIIR)); > + I915_WRITE(GTIMR, dev_priv->gt_irq_mask); > + I915_WRITE(GTIER, gt_irqs); > + POSTING_READ(GTIER); > + > + if (INTEL_INFO(dev)->gen >= 6) { > + pm_irqs |= GEN6_PM_RPS_EVENTS; > + > + if (HAS_VEBOX(dev)) > + pm_irqs |= PM_VEBOX_USER_INTERRUPT; > + > + I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR)); > + I915_WRITE(GEN6_PMIMR, 0xffffffff); > + I915_WRITE(GEN6_PMIER, pm_irqs); > + POSTING_READ(GEN6_PMIER); > + } > +} > + The ordering still looks funky here to me and your comment in the commit message hasn't convinced me otherwise. The order should be: MASK CLEAR ENABLE In your order there is a theoretical race after you've cleared, but before you've masked. This is trivial to fix, if you want to avoid reposting, it's fine with me. > static int ironlake_irq_postinstall(struct drm_device *dev) > { > unsigned long irqflags; > @@ -2735,7 +2774,6 @@ static int ironlake_irq_postinstall(struct drm_device *dev) > DE_PLANEA_FLIP_DONE | DE_PLANEB_FLIP_DONE | > DE_AUX_CHANNEL_A | DE_PIPEB_FIFO_UNDERRUN | > DE_PIPEA_FIFO_UNDERRUN | DE_POISON; > - u32 gt_irqs; > > dev_priv->irq_mask = ~display_mask; > > @@ -2746,21 +2784,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev) > DE_PIPEA_VBLANK | DE_PIPEB_VBLANK | DE_PCU_EVENT); > POSTING_READ(DEIER); > > - dev_priv->gt_irq_mask = ~0; > - > - I915_WRITE(GTIIR, I915_READ(GTIIR)); > - I915_WRITE(GTIMR, dev_priv->gt_irq_mask); > - > - gt_irqs = GT_RENDER_USER_INTERRUPT; > - > - if (IS_GEN6(dev)) > - gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT; > - else > - gt_irqs |= GT_RENDER_PIPECTL_NOTIFY_INTERRUPT | > - ILK_BSD_USER_INTERRUPT; > - > - I915_WRITE(GTIER, gt_irqs); > - POSTING_READ(GTIER); > + gen5_gt_irq_postinstall(dev); > > ibx_irq_postinstall(dev); > > @@ -2789,8 +2813,6 @@ static int ivybridge_irq_postinstall(struct drm_device *dev) > DE_PLANEA_FLIP_DONE_IVB | > DE_AUX_CHANNEL_A_IVB | > DE_ERR_INT_IVB; > - u32 pm_irqs = GEN6_PM_RPS_EVENTS; > - u32 gt_irqs; > > dev_priv->irq_mask = ~display_mask; > > @@ -2805,30 +2827,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev) > DE_PIPEA_VBLANK_IVB); > POSTING_READ(DEIER); > > - dev_priv->gt_irq_mask = ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT; > - > - I915_WRITE(GTIIR, I915_READ(GTIIR)); > - I915_WRITE(GTIMR, dev_priv->gt_irq_mask); > - > - gt_irqs = GT_RENDER_USER_INTERRUPT | GT_BSD_USER_INTERRUPT | > - GT_BLT_USER_INTERRUPT | GT_RENDER_L3_PARITY_ERROR_INTERRUPT; > - I915_WRITE(GTIER, gt_irqs); > - POSTING_READ(GTIER); > - > - I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR)); > - if (HAS_VEBOX(dev)) > - pm_irqs |= PM_VEBOX_USER_INTERRUPT; > - > - /* Our enable/disable rps functions may touch these registers so > - * make sure to set a known state for only the non-RPS bits. > - * The RMW is extra paranoia since this should be called after being set > - * to a known state in preinstall. > - * */ > - I915_WRITE(GEN6_PMIMR, > - (I915_READ(GEN6_PMIMR) | ~GEN6_PM_RPS_EVENTS) & ~pm_irqs); > - I915_WRITE(GEN6_PMIER, > - (I915_READ(GEN6_PMIER) & GEN6_PM_RPS_EVENTS) | pm_irqs); > - POSTING_READ(GEN6_PMIER); > + gen5_gt_irq_postinstall(dev); > > ibx_irq_postinstall(dev); > > @@ -2838,7 +2837,6 @@ static int ivybridge_irq_postinstall(struct drm_device *dev) > static int valleyview_irq_postinstall(struct drm_device *dev) > { > drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; > - u32 gt_irqs; > u32 enable_mask; > u32 pipestat_enable = PLANE_FLIP_DONE_INT_EN_VLV; > unsigned long irqflags; > @@ -2878,13 +2876,7 @@ static int valleyview_irq_postinstall(struct drm_device *dev) > I915_WRITE(VLV_IIR, 0xffffffff); > I915_WRITE(VLV_IIR, 0xffffffff); > > - I915_WRITE(GTIIR, I915_READ(GTIIR)); > - I915_WRITE(GTIMR, dev_priv->gt_irq_mask); > - > - gt_irqs = GT_RENDER_USER_INTERRUPT | GT_BSD_USER_INTERRUPT | > - GT_BLT_USER_INTERRUPT; > - I915_WRITE(GTIER, gt_irqs); > - POSTING_READ(GTIER); > + gen5_gt_irq_postinstall(dev); > > /* ack & enable invalid PTE error interrupts */ > #if 0 /* FIXME: add support to irq handler for checking these bits */ > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index fb4afaa..e609232 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3319,8 +3319,6 @@ static void gen6_enable_rps(struct drm_device *dev) > > gen6_set_rps(dev_priv->dev, (gt_perf_status & 0xff00) >> 8); > > - /* requires MSI enabled */ > - I915_WRITE(GEN6_PMIER, I915_READ(GEN6_PMIER) | GEN6_PM_RPS_EVENTS); > spin_lock_irq(&dev_priv->irq_lock); > /* FIXME: Our interrupt enabling sequence is bonghits. > * dev_priv->rps.pm_iir really should be 0 here. */ > @@ -3599,8 +3597,6 @@ static void valleyview_enable_rps(struct drm_device *dev) > > valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay); > > - /* requires MSI enabled */ > - I915_WRITE(GEN6_PMIER, GEN6_PM_RPS_EVENTS); > spin_lock_irq(&dev_priv->irq_lock); > WARN_ON(dev_priv->rps.pm_iir != 0); > I915_WRITE(GEN6_PMIMR, 0); With my comments in 1 & 2 address, they're both: Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
On Sun, Jul 14, 2013 at 01:55:20PM -0700, Ben Widawsky wrote: > On Fri, Jul 12, 2013 at 10:43:26PM +0200, Daniel Vetter wrote: > > Again extract a common helper. For the postinstall hook things are a > > bit more complicated since we have more cases on ilk-hsw/vlv here. > > > > But since vlv was clearly broken by failing to initialize > > dev_priv->gt_irq_mask correctly the shared code is clearly justified. > > > > Also kill the PMIER setting in the async rps enable work. I should > > have been save, but also clearly looked rather fragile. PMIER setup is > Can you fix up this sentence. It's a bit weird, and I'm not positive > what you're trying to say. Stab at your VECS enabling patches which touch PMIER in the rps enabling work. After this patch PMIER is only set up (once) in the postinstall hook with all interrupts we ever want already enabled. We only update the PMIMR register when we enable rps, like with all other interrupts enabled after irq install time. > > not all down in the irq pre/postinstall hooks. s/not/now/ here probably clarifies it? > > > > With this we now have the usual interrupt register sequence for GT/PM > > irq registers: > > > > - IER is setup once with all the interrupts we ever need in the > > postinstall hook and never touched again. Exceptions are SDEIER, > > which is touched in the preinstall hook (when the irq handler isn't > > enabled) and then only from the irq handler. And DEIER/VLV_IER with > > is used in the irq handler but also written to once in the > > postinstall hook. But since that write is essentially what enables > > the interrupt and we should always have MSI interrupts we should be > > save. In case we ever have non-MSI interrupts we'd be screwed. > > > > - IIR is cleared in the postinstall hook before we enable/unmask the > > respective interrupt sources. Hence we can't steal an interrupt > > event an accidentally trigger the spurious interrupt logic in the > > core kernel. Note that after some discussion with Ben Widawsky we > > think that we actually should clear the IIR registers in the > > preinstall hook. But doing that is a much larger patch series. > > > > - IMR regs are (usually) all masked off. Those are the only regs > > changed at runtime, which is all protected by dev_priv->irq_lock. > > > > This comment block is really nice. Maybe supplement it with explanation > about how the ring interrupts work, and shove it in i915_irq.c? I'm not a fan of big overview comments. The get outdated really fast and no one ever bothers to update them. In the commit message they're annotated to a clear point in our history with the implication that they have been reviewed to be correct at that point. > > > This unification also kills the cargo-culted read-modify-write PM > > register setup for VECS. Interrupt setup is done without userspace > > being able to interfere, so we better know what values we want to put > > into those registers. RMW cycles otoh are really good at papering over > > races, until stuff magically blows up and no one has a clue why. > > > > v2: Touch the gen6+ PM interrupt registers only on gen6+. > > > > v3: Improve the commit message to more clearly spell out why we want > > to unify the code and what exactly changes. > > > > Cc: Ben Widawsky <ben@bwidawsk.net> > > Cc: Paulo Zanoni <przanoni@gmail.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > --- > > drivers/gpu/drm/i915/i915_irq.c | 92 +++++++++++++++++++---------------------- > > drivers/gpu/drm/i915/intel_pm.c | 4 -- > > 2 files changed, 42 insertions(+), 54 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > index d5c3bef..ba61bc7 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -2725,6 +2725,45 @@ static void ibx_irq_postinstall(struct drm_device *dev) > > I915_WRITE(SDEIMR, ~mask); > > } > > > > +static void gen5_gt_irq_postinstall(struct drm_device *dev) > > +{ > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + u32 pm_irqs, gt_irqs; > > + > > + pm_irqs = gt_irqs = 0; > > + > > + dev_priv->gt_irq_mask = ~0; > > + if (HAS_L3_GPU_CACHE(dev)) { > > + dev_priv->gt_irq_mask = ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT; > > + gt_irqs |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT; > > + } > > Maybe while you're doing this, explain why the L3 parity interrupt is > special, in a comment. It's the only one to touch dev_priv->gt_irq_mask I'll add /* L3 parity interrupt is always unmasked. */ before the gt_irq_mask assignemnt. > > > + > > + gt_irqs |= GT_RENDER_USER_INTERRUPT; > > + if (IS_GEN5(dev)) { > > + gt_irqs |= GT_RENDER_PIPECTL_NOTIFY_INTERRUPT | > > + ILK_BSD_USER_INTERRUPT; > > + } else { > > + gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT; > > + } > > + > > + I915_WRITE(GTIIR, I915_READ(GTIIR)); > > + I915_WRITE(GTIMR, dev_priv->gt_irq_mask); > > + I915_WRITE(GTIER, gt_irqs); > > + POSTING_READ(GTIER); > > + > > + if (INTEL_INFO(dev)->gen >= 6) { > > + pm_irqs |= GEN6_PM_RPS_EVENTS; > > + > > + if (HAS_VEBOX(dev)) > > + pm_irqs |= PM_VEBOX_USER_INTERRUPT; > > + > > + I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR)); > > + I915_WRITE(GEN6_PMIMR, 0xffffffff); > > + I915_WRITE(GEN6_PMIER, pm_irqs); > > + POSTING_READ(GEN6_PMIER); > > + } > > +} > > + > > The ordering still looks funky here to me and your comment in the commit > message hasn't convinced me otherwise. The order should be: > MASK > CLEAR > ENABLE > > In your order there is a theoretical race after you've cleared, but > before you've masked. This is trivial to fix, if you want to avoid > reposting, it's fine with me. Imo fixing that up is a separate patch series, I'm sticking to being consistent whats there already. In any case I vote for a bit of refactoring so that all the I.R register sequences use the same code, i.e. something like the DISABLE_AND_CLEAR_INTERRUPT macro I've proposed in another thread. > > > static int ironlake_irq_postinstall(struct drm_device *dev) > > { > > unsigned long irqflags; > > @@ -2735,7 +2774,6 @@ static int ironlake_irq_postinstall(struct drm_device *dev) > > DE_PLANEA_FLIP_DONE | DE_PLANEB_FLIP_DONE | > > DE_AUX_CHANNEL_A | DE_PIPEB_FIFO_UNDERRUN | > > DE_PIPEA_FIFO_UNDERRUN | DE_POISON; > > - u32 gt_irqs; > > > > dev_priv->irq_mask = ~display_mask; > > > > @@ -2746,21 +2784,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev) > > DE_PIPEA_VBLANK | DE_PIPEB_VBLANK | DE_PCU_EVENT); > > POSTING_READ(DEIER); > > > > - dev_priv->gt_irq_mask = ~0; > > - > > - I915_WRITE(GTIIR, I915_READ(GTIIR)); > > - I915_WRITE(GTIMR, dev_priv->gt_irq_mask); > > - > > - gt_irqs = GT_RENDER_USER_INTERRUPT; > > - > > - if (IS_GEN6(dev)) > > - gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT; > > - else > > - gt_irqs |= GT_RENDER_PIPECTL_NOTIFY_INTERRUPT | > > - ILK_BSD_USER_INTERRUPT; > > - > > - I915_WRITE(GTIER, gt_irqs); > > - POSTING_READ(GTIER); > > + gen5_gt_irq_postinstall(dev); > > > > ibx_irq_postinstall(dev); > > > > @@ -2789,8 +2813,6 @@ static int ivybridge_irq_postinstall(struct drm_device *dev) > > DE_PLANEA_FLIP_DONE_IVB | > > DE_AUX_CHANNEL_A_IVB | > > DE_ERR_INT_IVB; > > - u32 pm_irqs = GEN6_PM_RPS_EVENTS; > > - u32 gt_irqs; > > > > dev_priv->irq_mask = ~display_mask; > > > > @@ -2805,30 +2827,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev) > > DE_PIPEA_VBLANK_IVB); > > POSTING_READ(DEIER); > > > > - dev_priv->gt_irq_mask = ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT; > > - > > - I915_WRITE(GTIIR, I915_READ(GTIIR)); > > - I915_WRITE(GTIMR, dev_priv->gt_irq_mask); > > - > > - gt_irqs = GT_RENDER_USER_INTERRUPT | GT_BSD_USER_INTERRUPT | > > - GT_BLT_USER_INTERRUPT | GT_RENDER_L3_PARITY_ERROR_INTERRUPT; > > - I915_WRITE(GTIER, gt_irqs); > > - POSTING_READ(GTIER); > > - > > - I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR)); > > - if (HAS_VEBOX(dev)) > > - pm_irqs |= PM_VEBOX_USER_INTERRUPT; > > - > > - /* Our enable/disable rps functions may touch these registers so > > - * make sure to set a known state for only the non-RPS bits. > > - * The RMW is extra paranoia since this should be called after being set > > - * to a known state in preinstall. > > - * */ > > - I915_WRITE(GEN6_PMIMR, > > - (I915_READ(GEN6_PMIMR) | ~GEN6_PM_RPS_EVENTS) & ~pm_irqs); > > - I915_WRITE(GEN6_PMIER, > > - (I915_READ(GEN6_PMIER) & GEN6_PM_RPS_EVENTS) | pm_irqs); > > - POSTING_READ(GEN6_PMIER); > > + gen5_gt_irq_postinstall(dev); > > > > ibx_irq_postinstall(dev); > > > > @@ -2838,7 +2837,6 @@ static int ivybridge_irq_postinstall(struct drm_device *dev) > > static int valleyview_irq_postinstall(struct drm_device *dev) > > { > > drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; > > - u32 gt_irqs; > > u32 enable_mask; > > u32 pipestat_enable = PLANE_FLIP_DONE_INT_EN_VLV; > > unsigned long irqflags; > > @@ -2878,13 +2876,7 @@ static int valleyview_irq_postinstall(struct drm_device *dev) > > I915_WRITE(VLV_IIR, 0xffffffff); > > I915_WRITE(VLV_IIR, 0xffffffff); > > > > - I915_WRITE(GTIIR, I915_READ(GTIIR)); > > - I915_WRITE(GTIMR, dev_priv->gt_irq_mask); > > - > > - gt_irqs = GT_RENDER_USER_INTERRUPT | GT_BSD_USER_INTERRUPT | > > - GT_BLT_USER_INTERRUPT; > > - I915_WRITE(GTIER, gt_irqs); > > - POSTING_READ(GTIER); > > + gen5_gt_irq_postinstall(dev); > > > > /* ack & enable invalid PTE error interrupts */ > > #if 0 /* FIXME: add support to irq handler for checking these bits */ > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index fb4afaa..e609232 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -3319,8 +3319,6 @@ static void gen6_enable_rps(struct drm_device *dev) > > > > gen6_set_rps(dev_priv->dev, (gt_perf_status & 0xff00) >> 8); > > > > - /* requires MSI enabled */ > > - I915_WRITE(GEN6_PMIER, I915_READ(GEN6_PMIER) | GEN6_PM_RPS_EVENTS); > > spin_lock_irq(&dev_priv->irq_lock); > > /* FIXME: Our interrupt enabling sequence is bonghits. > > * dev_priv->rps.pm_iir really should be 0 here. */ > > @@ -3599,8 +3597,6 @@ static void valleyview_enable_rps(struct drm_device *dev) > > > > valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay); > > > > - /* requires MSI enabled */ > > - I915_WRITE(GEN6_PMIER, GEN6_PM_RPS_EVENTS); > > spin_lock_irq(&dev_priv->irq_lock); > > WARN_ON(dev_priv->rps.pm_iir != 0); > > I915_WRITE(GEN6_PMIMR, 0); > > With my comments in 1 & 2 address, they're both: > Reviewed-by: Ben Widawsky <ben@bwidawsk.net> If you don't object to my comments above I'll bikeshed the patch while applying and add the missing comment and fix up the commit message. -Daniel
On Sun, Jul 14, 2013 at 11:31:29PM +0200, Daniel Vetter wrote: > On Sun, Jul 14, 2013 at 01:55:20PM -0700, Ben Widawsky wrote: > > On Fri, Jul 12, 2013 at 10:43:26PM +0200, Daniel Vetter wrote: > > > Again extract a common helper. For the postinstall hook things are a > > > bit more complicated since we have more cases on ilk-hsw/vlv here. > > > > > > But since vlv was clearly broken by failing to initialize > > > dev_priv->gt_irq_mask correctly the shared code is clearly justified. > > > > > > Also kill the PMIER setting in the async rps enable work. I should > > > have been save, but also clearly looked rather fragile. PMIER setup is > > Can you fix up this sentence. It's a bit weird, and I'm not positive > > what you're trying to say. > > Stab at your VECS enabling patches which touch PMIER in the rps enabling > work. After this patch PMIER is only set up (once) in the postinstall hook > with all interrupts we ever want already enabled. We only update the PMIMR > register when we enable rps, like with all other interrupts enabled after > irq install time. > > > > not all down in the irq pre/postinstall hooks. > > s/not/now/ here probably clarifies it? > > > > > > > With this we now have the usual interrupt register sequence for GT/PM > > > irq registers: > > > > > > - IER is setup once with all the interrupts we ever need in the > > > postinstall hook and never touched again. Exceptions are SDEIER, > > > which is touched in the preinstall hook (when the irq handler isn't > > > enabled) and then only from the irq handler. And DEIER/VLV_IER with > > > is used in the irq handler but also written to once in the > > > postinstall hook. But since that write is essentially what enables > > > the interrupt and we should always have MSI interrupts we should be > > > save. In case we ever have non-MSI interrupts we'd be screwed. > > > > > > - IIR is cleared in the postinstall hook before we enable/unmask the > > > respective interrupt sources. Hence we can't steal an interrupt > > > event an accidentally trigger the spurious interrupt logic in the > > > core kernel. Note that after some discussion with Ben Widawsky we > > > think that we actually should clear the IIR registers in the > > > preinstall hook. But doing that is a much larger patch series. > > > > > > - IMR regs are (usually) all masked off. Those are the only regs > > > changed at runtime, which is all protected by dev_priv->irq_lock. > > > > > > > This comment block is really nice. Maybe supplement it with explanation > > about how the ring interrupts work, and shove it in i915_irq.c? > > I'm not a fan of big overview comments. The get outdated really fast and > no one ever bothers to update them. In the commit message they're > annotated to a clear point in our history with the implication that they > have been reviewed to be correct at that point. > > > > > > This unification also kills the cargo-culted read-modify-write PM > > > register setup for VECS. Interrupt setup is done without userspace > > > being able to interfere, so we better know what values we want to put > > > into those registers. RMW cycles otoh are really good at papering over > > > races, until stuff magically blows up and no one has a clue why. > > > > > > v2: Touch the gen6+ PM interrupt registers only on gen6+. > > > > > > v3: Improve the commit message to more clearly spell out why we want > > > to unify the code and what exactly changes. > > > > > > Cc: Ben Widawsky <ben@bwidawsk.net> > > > Cc: Paulo Zanoni <przanoni@gmail.com> > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > --- > > > drivers/gpu/drm/i915/i915_irq.c | 92 +++++++++++++++++++---------------------- > > > drivers/gpu/drm/i915/intel_pm.c | 4 -- > > > 2 files changed, 42 insertions(+), 54 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > > index d5c3bef..ba61bc7 100644 > > > --- a/drivers/gpu/drm/i915/i915_irq.c > > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > > @@ -2725,6 +2725,45 @@ static void ibx_irq_postinstall(struct drm_device *dev) > > > I915_WRITE(SDEIMR, ~mask); > > > } > > > > > > +static void gen5_gt_irq_postinstall(struct drm_device *dev) > > > +{ > > > + struct drm_i915_private *dev_priv = dev->dev_private; > > > + u32 pm_irqs, gt_irqs; > > > + > > > + pm_irqs = gt_irqs = 0; > > > + > > > + dev_priv->gt_irq_mask = ~0; > > > + if (HAS_L3_GPU_CACHE(dev)) { > > > + dev_priv->gt_irq_mask = ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT; > > > + gt_irqs |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT; > > > + } > > > > Maybe while you're doing this, explain why the L3 parity interrupt is > > special, in a comment. It's the only one to touch dev_priv->gt_irq_mask > > I'll add > /* L3 parity interrupt is always unmasked. */ > > before the gt_irq_mask assignemnt. > > > > > > + > > > + gt_irqs |= GT_RENDER_USER_INTERRUPT; > > > + if (IS_GEN5(dev)) { > > > + gt_irqs |= GT_RENDER_PIPECTL_NOTIFY_INTERRUPT | > > > + ILK_BSD_USER_INTERRUPT; > > > + } else { > > > + gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT; > > > + } > > > + > > > + I915_WRITE(GTIIR, I915_READ(GTIIR)); > > > + I915_WRITE(GTIMR, dev_priv->gt_irq_mask); > > > + I915_WRITE(GTIER, gt_irqs); > > > + POSTING_READ(GTIER); > > > + > > > + if (INTEL_INFO(dev)->gen >= 6) { > > > + pm_irqs |= GEN6_PM_RPS_EVENTS; > > > + > > > + if (HAS_VEBOX(dev)) > > > + pm_irqs |= PM_VEBOX_USER_INTERRUPT; > > > + > > > + I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR)); > > > + I915_WRITE(GEN6_PMIMR, 0xffffffff); > > > + I915_WRITE(GEN6_PMIER, pm_irqs); > > > + POSTING_READ(GEN6_PMIER); > > > + } > > > +} > > > + > > > > The ordering still looks funky here to me and your comment in the commit > > message hasn't convinced me otherwise. The order should be: > > MASK > > CLEAR > > ENABLE > > > > In your order there is a theoretical race after you've cleared, but > > before you've masked. This is trivial to fix, if you want to avoid > > reposting, it's fine with me. > > Imo fixing that up is a separate patch series, I'm sticking to being > consistent whats there already. In any case I vote for a bit of > refactoring so that all the I.R register sequences use the same code, i.e. > something like the DISABLE_AND_CLEAR_INTERRUPT macro I've proposed in > another thread. > > > > > > static int ironlake_irq_postinstall(struct drm_device *dev) > > > { > > > unsigned long irqflags; > > > @@ -2735,7 +2774,6 @@ static int ironlake_irq_postinstall(struct drm_device *dev) > > > DE_PLANEA_FLIP_DONE | DE_PLANEB_FLIP_DONE | > > > DE_AUX_CHANNEL_A | DE_PIPEB_FIFO_UNDERRUN | > > > DE_PIPEA_FIFO_UNDERRUN | DE_POISON; > > > - u32 gt_irqs; > > > > > > dev_priv->irq_mask = ~display_mask; > > > > > > @@ -2746,21 +2784,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev) > > > DE_PIPEA_VBLANK | DE_PIPEB_VBLANK | DE_PCU_EVENT); > > > POSTING_READ(DEIER); > > > > > > - dev_priv->gt_irq_mask = ~0; > > > - > > > - I915_WRITE(GTIIR, I915_READ(GTIIR)); > > > - I915_WRITE(GTIMR, dev_priv->gt_irq_mask); > > > - > > > - gt_irqs = GT_RENDER_USER_INTERRUPT; > > > - > > > - if (IS_GEN6(dev)) > > > - gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT; > > > - else > > > - gt_irqs |= GT_RENDER_PIPECTL_NOTIFY_INTERRUPT | > > > - ILK_BSD_USER_INTERRUPT; > > > - > > > - I915_WRITE(GTIER, gt_irqs); > > > - POSTING_READ(GTIER); > > > + gen5_gt_irq_postinstall(dev); > > > > > > ibx_irq_postinstall(dev); > > > > > > @@ -2789,8 +2813,6 @@ static int ivybridge_irq_postinstall(struct drm_device *dev) > > > DE_PLANEA_FLIP_DONE_IVB | > > > DE_AUX_CHANNEL_A_IVB | > > > DE_ERR_INT_IVB; > > > - u32 pm_irqs = GEN6_PM_RPS_EVENTS; > > > - u32 gt_irqs; > > > > > > dev_priv->irq_mask = ~display_mask; > > > > > > @@ -2805,30 +2827,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev) > > > DE_PIPEA_VBLANK_IVB); > > > POSTING_READ(DEIER); > > > > > > - dev_priv->gt_irq_mask = ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT; > > > - > > > - I915_WRITE(GTIIR, I915_READ(GTIIR)); > > > - I915_WRITE(GTIMR, dev_priv->gt_irq_mask); > > > - > > > - gt_irqs = GT_RENDER_USER_INTERRUPT | GT_BSD_USER_INTERRUPT | > > > - GT_BLT_USER_INTERRUPT | GT_RENDER_L3_PARITY_ERROR_INTERRUPT; > > > - I915_WRITE(GTIER, gt_irqs); > > > - POSTING_READ(GTIER); > > > - > > > - I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR)); > > > - if (HAS_VEBOX(dev)) > > > - pm_irqs |= PM_VEBOX_USER_INTERRUPT; > > > - > > > - /* Our enable/disable rps functions may touch these registers so > > > - * make sure to set a known state for only the non-RPS bits. > > > - * The RMW is extra paranoia since this should be called after being set > > > - * to a known state in preinstall. > > > - * */ > > > - I915_WRITE(GEN6_PMIMR, > > > - (I915_READ(GEN6_PMIMR) | ~GEN6_PM_RPS_EVENTS) & ~pm_irqs); > > > - I915_WRITE(GEN6_PMIER, > > > - (I915_READ(GEN6_PMIER) & GEN6_PM_RPS_EVENTS) | pm_irqs); > > > - POSTING_READ(GEN6_PMIER); > > > + gen5_gt_irq_postinstall(dev); > > > > > > ibx_irq_postinstall(dev); > > > > > > @@ -2838,7 +2837,6 @@ static int ivybridge_irq_postinstall(struct drm_device *dev) > > > static int valleyview_irq_postinstall(struct drm_device *dev) > > > { > > > drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; > > > - u32 gt_irqs; > > > u32 enable_mask; > > > u32 pipestat_enable = PLANE_FLIP_DONE_INT_EN_VLV; > > > unsigned long irqflags; > > > @@ -2878,13 +2876,7 @@ static int valleyview_irq_postinstall(struct drm_device *dev) > > > I915_WRITE(VLV_IIR, 0xffffffff); > > > I915_WRITE(VLV_IIR, 0xffffffff); > > > > > > - I915_WRITE(GTIIR, I915_READ(GTIIR)); > > > - I915_WRITE(GTIMR, dev_priv->gt_irq_mask); > > > - > > > - gt_irqs = GT_RENDER_USER_INTERRUPT | GT_BSD_USER_INTERRUPT | > > > - GT_BLT_USER_INTERRUPT; > > > - I915_WRITE(GTIER, gt_irqs); > > > - POSTING_READ(GTIER); > > > + gen5_gt_irq_postinstall(dev); > > > > > > /* ack & enable invalid PTE error interrupts */ > > > #if 0 /* FIXME: add support to irq handler for checking these bits */ > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > index fb4afaa..e609232 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -3319,8 +3319,6 @@ static void gen6_enable_rps(struct drm_device *dev) > > > > > > gen6_set_rps(dev_priv->dev, (gt_perf_status & 0xff00) >> 8); > > > > > > - /* requires MSI enabled */ > > > - I915_WRITE(GEN6_PMIER, I915_READ(GEN6_PMIER) | GEN6_PM_RPS_EVENTS); > > > spin_lock_irq(&dev_priv->irq_lock); > > > /* FIXME: Our interrupt enabling sequence is bonghits. > > > * dev_priv->rps.pm_iir really should be 0 here. */ > > > @@ -3599,8 +3597,6 @@ static void valleyview_enable_rps(struct drm_device *dev) > > > > > > valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay); > > > > > > - /* requires MSI enabled */ > > > - I915_WRITE(GEN6_PMIER, GEN6_PM_RPS_EVENTS); > > > spin_lock_irq(&dev_priv->irq_lock); > > > WARN_ON(dev_priv->rps.pm_iir != 0); > > > I915_WRITE(GEN6_PMIMR, 0); > > > > With my comments in 1 & 2 address, they're both: > > Reviewed-by: Ben Widawsky <ben@bwidawsk.net> > > If you don't object to my comments above I'll bikeshed the patch while > applying and add the missing comment and fix up the commit message. > -Daniel ack
On Sun, Jul 14, 2013 at 11:31:29PM +0200, Daniel Vetter wrote: > On Sun, Jul 14, 2013 at 01:55:20PM -0700, Ben Widawsky wrote: > > On Fri, Jul 12, 2013 at 10:43:26PM +0200, Daniel Vetter wrote: [snip] > > > > Maybe while you're doing this, explain why the L3 parity interrupt is > > special, in a comment. It's the only one to touch dev_priv->gt_irq_mask > > I'll add > /* L3 parity interrupt is always unmasked. */ > > before the gt_irq_mask assignemnt. Actually, I was thinking more along the lines of "L3 parity interrupt is always unmasked. L3 parity interrupts are asynchronous with regard to batch submission, however they are delivered through ring interrupt registers." [snip]
On Sun, Jul 14, 2013 at 05:13:34PM -0700, Ben Widawsky wrote: > On Sun, Jul 14, 2013 at 11:31:29PM +0200, Daniel Vetter wrote: > > On Sun, Jul 14, 2013 at 01:55:20PM -0700, Ben Widawsky wrote: > > > On Fri, Jul 12, 2013 at 10:43:26PM +0200, Daniel Vetter wrote: > [snip] > > > > > > > Maybe while you're doing this, explain why the L3 parity interrupt is > > > special, in a comment. It's the only one to touch dev_priv->gt_irq_mask > > > > I'll add > > /* L3 parity interrupt is always unmasked. */ > > > > before the gt_irq_mask assignemnt. > > Actually, I was thinking more along the lines of "L3 parity interrupt is > always unmasked. L3 parity interrupts are asynchronous with regard to > batch submission, however they are delivered through ring interrupt > registers." We have more suche cases now with VECS going through PM interrupts. So I don't think the fact that hw engineers sometimes steal bits from odd places requires special mention. So I've opted for the simple version. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index d5c3bef..ba61bc7 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2725,6 +2725,45 @@ static void ibx_irq_postinstall(struct drm_device *dev) I915_WRITE(SDEIMR, ~mask); } +static void gen5_gt_irq_postinstall(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + u32 pm_irqs, gt_irqs; + + pm_irqs = gt_irqs = 0; + + dev_priv->gt_irq_mask = ~0; + if (HAS_L3_GPU_CACHE(dev)) { + dev_priv->gt_irq_mask = ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT; + gt_irqs |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT; + } + + gt_irqs |= GT_RENDER_USER_INTERRUPT; + if (IS_GEN5(dev)) { + gt_irqs |= GT_RENDER_PIPECTL_NOTIFY_INTERRUPT | + ILK_BSD_USER_INTERRUPT; + } else { + gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT; + } + + I915_WRITE(GTIIR, I915_READ(GTIIR)); + I915_WRITE(GTIMR, dev_priv->gt_irq_mask); + I915_WRITE(GTIER, gt_irqs); + POSTING_READ(GTIER); + + if (INTEL_INFO(dev)->gen >= 6) { + pm_irqs |= GEN6_PM_RPS_EVENTS; + + if (HAS_VEBOX(dev)) + pm_irqs |= PM_VEBOX_USER_INTERRUPT; + + I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR)); + I915_WRITE(GEN6_PMIMR, 0xffffffff); + I915_WRITE(GEN6_PMIER, pm_irqs); + POSTING_READ(GEN6_PMIER); + } +} + static int ironlake_irq_postinstall(struct drm_device *dev) { unsigned long irqflags; @@ -2735,7 +2774,6 @@ static int ironlake_irq_postinstall(struct drm_device *dev) DE_PLANEA_FLIP_DONE | DE_PLANEB_FLIP_DONE | DE_AUX_CHANNEL_A | DE_PIPEB_FIFO_UNDERRUN | DE_PIPEA_FIFO_UNDERRUN | DE_POISON; - u32 gt_irqs; dev_priv->irq_mask = ~display_mask; @@ -2746,21 +2784,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev) DE_PIPEA_VBLANK | DE_PIPEB_VBLANK | DE_PCU_EVENT); POSTING_READ(DEIER); - dev_priv->gt_irq_mask = ~0; - - I915_WRITE(GTIIR, I915_READ(GTIIR)); - I915_WRITE(GTIMR, dev_priv->gt_irq_mask); - - gt_irqs = GT_RENDER_USER_INTERRUPT; - - if (IS_GEN6(dev)) - gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT; - else - gt_irqs |= GT_RENDER_PIPECTL_NOTIFY_INTERRUPT | - ILK_BSD_USER_INTERRUPT; - - I915_WRITE(GTIER, gt_irqs); - POSTING_READ(GTIER); + gen5_gt_irq_postinstall(dev); ibx_irq_postinstall(dev); @@ -2789,8 +2813,6 @@ static int ivybridge_irq_postinstall(struct drm_device *dev) DE_PLANEA_FLIP_DONE_IVB | DE_AUX_CHANNEL_A_IVB | DE_ERR_INT_IVB; - u32 pm_irqs = GEN6_PM_RPS_EVENTS; - u32 gt_irqs; dev_priv->irq_mask = ~display_mask; @@ -2805,30 +2827,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev) DE_PIPEA_VBLANK_IVB); POSTING_READ(DEIER); - dev_priv->gt_irq_mask = ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT; - - I915_WRITE(GTIIR, I915_READ(GTIIR)); - I915_WRITE(GTIMR, dev_priv->gt_irq_mask); - - gt_irqs = GT_RENDER_USER_INTERRUPT | GT_BSD_USER_INTERRUPT | - GT_BLT_USER_INTERRUPT | GT_RENDER_L3_PARITY_ERROR_INTERRUPT; - I915_WRITE(GTIER, gt_irqs); - POSTING_READ(GTIER); - - I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR)); - if (HAS_VEBOX(dev)) - pm_irqs |= PM_VEBOX_USER_INTERRUPT; - - /* Our enable/disable rps functions may touch these registers so - * make sure to set a known state for only the non-RPS bits. - * The RMW is extra paranoia since this should be called after being set - * to a known state in preinstall. - * */ - I915_WRITE(GEN6_PMIMR, - (I915_READ(GEN6_PMIMR) | ~GEN6_PM_RPS_EVENTS) & ~pm_irqs); - I915_WRITE(GEN6_PMIER, - (I915_READ(GEN6_PMIER) & GEN6_PM_RPS_EVENTS) | pm_irqs); - POSTING_READ(GEN6_PMIER); + gen5_gt_irq_postinstall(dev); ibx_irq_postinstall(dev); @@ -2838,7 +2837,6 @@ static int ivybridge_irq_postinstall(struct drm_device *dev) static int valleyview_irq_postinstall(struct drm_device *dev) { drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; - u32 gt_irqs; u32 enable_mask; u32 pipestat_enable = PLANE_FLIP_DONE_INT_EN_VLV; unsigned long irqflags; @@ -2878,13 +2876,7 @@ static int valleyview_irq_postinstall(struct drm_device *dev) I915_WRITE(VLV_IIR, 0xffffffff); I915_WRITE(VLV_IIR, 0xffffffff); - I915_WRITE(GTIIR, I915_READ(GTIIR)); - I915_WRITE(GTIMR, dev_priv->gt_irq_mask); - - gt_irqs = GT_RENDER_USER_INTERRUPT | GT_BSD_USER_INTERRUPT | - GT_BLT_USER_INTERRUPT; - I915_WRITE(GTIER, gt_irqs); - POSTING_READ(GTIER); + gen5_gt_irq_postinstall(dev); /* ack & enable invalid PTE error interrupts */ #if 0 /* FIXME: add support to irq handler for checking these bits */ diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index fb4afaa..e609232 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3319,8 +3319,6 @@ static void gen6_enable_rps(struct drm_device *dev) gen6_set_rps(dev_priv->dev, (gt_perf_status & 0xff00) >> 8); - /* requires MSI enabled */ - I915_WRITE(GEN6_PMIER, I915_READ(GEN6_PMIER) | GEN6_PM_RPS_EVENTS); spin_lock_irq(&dev_priv->irq_lock); /* FIXME: Our interrupt enabling sequence is bonghits. * dev_priv->rps.pm_iir really should be 0 here. */ @@ -3599,8 +3597,6 @@ static void valleyview_enable_rps(struct drm_device *dev) valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay); - /* requires MSI enabled */ - I915_WRITE(GEN6_PMIER, GEN6_PM_RPS_EVENTS); spin_lock_irq(&dev_priv->irq_lock); WARN_ON(dev_priv->rps.pm_iir != 0); I915_WRITE(GEN6_PMIMR, 0);
Again extract a common helper. For the postinstall hook things are a bit more complicated since we have more cases on ilk-hsw/vlv here. But since vlv was clearly broken by failing to initialize dev_priv->gt_irq_mask correctly the shared code is clearly justified. Also kill the PMIER setting in the async rps enable work. I should have been save, but also clearly looked rather fragile. PMIER setup is not all down in the irq pre/postinstall hooks. With this we now have the usual interrupt register sequence for GT/PM irq registers: - IER is setup once with all the interrupts we ever need in the postinstall hook and never touched again. Exceptions are SDEIER, which is touched in the preinstall hook (when the irq handler isn't enabled) and then only from the irq handler. And DEIER/VLV_IER with is used in the irq handler but also written to once in the postinstall hook. But since that write is essentially what enables the interrupt and we should always have MSI interrupts we should be save. In case we ever have non-MSI interrupts we'd be screwed. - IIR is cleared in the postinstall hook before we enable/unmask the respective interrupt sources. Hence we can't steal an interrupt event an accidentally trigger the spurious interrupt logic in the core kernel. Note that after some discussion with Ben Widawsky we think that we actually should clear the IIR registers in the preinstall hook. But doing that is a much larger patch series. - IMR regs are (usually) all masked off. Those are the only regs changed at runtime, which is all protected by dev_priv->irq_lock. This unification also kills the cargo-culted read-modify-write PM register setup for VECS. Interrupt setup is done without userspace being able to interfere, so we better know what values we want to put into those registers. RMW cycles otoh are really good at papering over races, until stuff magically blows up and no one has a clue why. v2: Touch the gen6+ PM interrupt registers only on gen6+. v3: Improve the commit message to more clearly spell out why we want to unify the code and what exactly changes. Cc: Ben Widawsky <ben@bwidawsk.net> Cc: Paulo Zanoni <przanoni@gmail.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_irq.c | 92 +++++++++++++++++++---------------------- drivers/gpu/drm/i915/intel_pm.c | 4 -- 2 files changed, 42 insertions(+), 54 deletions(-)