Re: [PATCH 12/14] drm/i915: unify GT/PM irq postinstall code

From: Paulo Zanoni <przanoni@gmail.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 12/14] drm/i915: unify GT/PM irq postinstall code
Date: Wed, 10 Jul 2013 17:48:46 -0300	[thread overview]
Message-ID: <CA+gsUGSnJpHWaDOaha3S4LwthAhJE5tFHXtLbyvd81iB124TWQ@mail.gmail.com> (raw)
In-Reply-To: <1372973734-7601-13-git-send-email-daniel.vetter@ffwll.ch>

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

-- 
Paulo Zanoni