| Message ID | 1372973734-7601-13-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2013/7/4 Daniel Vetter <daniel.vetter@ffwll.ch>: > 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 correclty (mayb that explains the strange > RING_IMR clearing in the preinstall hook?) clearly justified the > shared code. > > Also kill the PMIER setting in the async rps enable work. I should > have been save, but also clearly looked rather fragile. > > 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. > > - 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+. > > 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 f4babaa..f4707a2 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -2723,6 +2723,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; > @@ -2733,7 +2772,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; > > @@ -2744,21 +2782,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); Which means we're now initializing GEN6_PM* code on SNB, which seems good. You might want to dedicate a paragraph for this on the commit message. On the IRQ patches I wrote (but did not sent yet) I unified the ILK/SNB irq_handler vfuncs with IVB/HSW ones. I guess bugs like the one you've just fixed here and in the previous patch are a good way to justify my patches :) > > ibx_irq_postinstall(dev); > > @@ -2787,8 +2811,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; > > @@ -2803,30 +2825,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); > > @@ -2836,7 +2835,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; > @@ -2876,13 +2874,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); Besides fixing the uninitialized mask, we're also now initializing GEN6_PM* regs on VLV. You should also mention this explicitly. This patch contains a few bug fixes. I certainly won't complain if you split them into separate commits, and then merge the code in the final commit. But I can live without that, it's your choice :) > > /* 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 a9be0d1..96f0872 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); It's even outside the irq_lock :) Do we want to at least read the bit and WARN in case it's not enabled? > 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); > -- > 1.8.1.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Jul 10, 2013 at 10:48 PM, Paulo Zanoni <przanoni@gmail.com> wrote: > Which means we're now initializing GEN6_PM* code on SNB, which seems > good. You might want to dedicate a paragraph for this on the commit > message. > > On the IRQ patches I wrote (but did not sent yet) I unified the > ILK/SNB irq_handler vfuncs with IVB/HSW ones. I guess bugs like the > one you've just fixed here and in the previous patch are a good way to > justify my patches :) I think we have a language communication fail going on here in these few patches. With "unify" I don't mean "extract identical code, simple refactoring with no functional change but "make them work the same way since currently they don't". Note that with the exception of the gt_irq_mask initialization on vlv there's no bugfix in here: PM interrupte setup simply works differently on snb/vlv than on ivb/hsw after Ben's VECS enabling patches. What these few patches here try to do is unify the sequences again so all platforms set up the PM registers the same way. Note that the PMIER update in gen6_enable_rps isn't a bugfix either: The interrupt handler doesn't touch this register. The only other place is the hsw vecs interrupt setup in the ivybridge irq vfuncs, but those two codepaths can never run in parrallel due to init/teardown sequence ordering (even though that gen6_enable_rps runs from a work item). I've read through the commit message again and for me it seems to be clear what's going on (but I'm obviously biased). So again, do you have ideas for improvements? Wrt patch splitting I'm not sure whether it's worth it, since the current code is simply a bit a confusing mess imo. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index f4babaa..f4707a2 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2723,6 +2723,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; @@ -2733,7 +2772,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; @@ -2744,21 +2782,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); @@ -2787,8 +2811,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; @@ -2803,30 +2825,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); @@ -2836,7 +2835,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; @@ -2876,13 +2874,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 a9be0d1..96f0872 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 correclty (mayb that explains the strange RING_IMR clearing in the preinstall hook?) clearly justified the shared code. Also kill the PMIER setting in the async rps enable work. I should have been save, but also clearly looked rather fragile. 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. - 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+. 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(-)