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 at bwidawsk.net>
> > Cc: Paulo Zanoni <przanoni at gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter at 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 at 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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
