Re: [PATCH 05/14] drm/i915: Use a consistent order between IIR, IER, IMR writes on vlv/chv

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Paulo Zanoni <przanoni@gmail.com>
Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 05/14] drm/i915: Use a consistent order between IIR, IER, IMR writes on vlv/chv
Date: Thu, 30 Oct 2014 21:39:07 +0200	[thread overview]
Message-ID: <20141030193907.GY10649@intel.com> (raw)
In-Reply-To: <CA+gsUGSsajLQAYT6hoKUexPrEnmvhq_Dx__0=z=50dkARAQBPA@mail.gmail.com>

On Thu, Oct 30, 2014 at 05:24:05PM -0200, Paulo Zanoni wrote:
> 2014-10-30 15:42 GMT-02:00  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Follow the same ordering rules for the IIR,IER,IMR writes on vlv/chv
> > that we do on other gen5+ platforms.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 29 ++++++++++++++++++-----------
> >  1 file changed, 18 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 1e4062d..589ae51 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -3128,10 +3128,11 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
> >         I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
> >         for_each_pipe(dev_priv, pipe)
> >                 I915_WRITE(PIPESTAT(pipe), 0xffff);
> > -       I915_WRITE(VLV_IIR, 0xffffffff);
> >         I915_WRITE(VLV_IMR, 0xffffffff);
> >         I915_WRITE(VLV_IER, 0x0);
> > -       POSTING_READ(VLV_IER);
> > +       I915_WRITE(VLV_IIR, 0xffffffff);
> > +       I915_WRITE(VLV_IIR, 0xffffffff);
> > +       POSTING_READ(VLV_IIR);
> 
> This is also a "fix" since clearing IIR before IMR doesn't guarantee
> us anything. The same applies in many chunks below.
> 
> 
> >  }
> >
> >  static void gen8_gt_irq_reset(struct drm_i915_private *dev_priv)
> > @@ -3199,6 +3200,7 @@ static void cherryview_irq_preinstall(struct drm_device *dev)
> >         I915_WRITE(VLV_IMR, 0xffffffff);
> >         I915_WRITE(VLV_IER, 0x0);
> >         I915_WRITE(VLV_IIR, 0xffffffff);
> > +       I915_WRITE(VLV_IIR, 0xffffffff);
> >         POSTING_READ(VLV_IIR);
> >  }
> >
> > @@ -3362,9 +3364,9 @@ static void valleyview_display_irqs_install(struct drm_i915_private *dev_priv)
> >
> >         I915_WRITE(VLV_IIR, iir_mask);
> >         I915_WRITE(VLV_IIR, iir_mask);
> > -       I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> >         I915_WRITE(VLV_IER, ~dev_priv->irq_mask);
> > -       POSTING_READ(VLV_IER);
> > +       I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> > +       POSTING_READ(VLV_IMR);
> 
> At this point you should probably just be asserting that IIR should
> still be zero, since this seems to run at the postinstall stage, and
> we already cleared IIR/IMR/IER at preinstall, so in theory it should
> be impossible for IIR to be non-zero. But I see there's also a call
> from intel_runtime_pm.c, so I don't know...
> 
> The "only check IIR at poinstinstall since we already disabled it at
> preinstall" argument is also valid in some chunks below.
> 
> Anyway, this commit is already an improvement so if you don't plan to
> change anything for now:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Yeah I did eye the INIT() macros lustfully for a while, but decided that
switching over would probably require a bit of extra diligence on my
part. So definitely something I want to try doing at some point, but I
figured I should avoid piling on too many patches in this one series.

> 
> >  }
> >
> >  static void valleyview_display_irqs_uninstall(struct drm_i915_private *dev_priv)
> > @@ -3377,8 +3379,8 @@ static void valleyview_display_irqs_uninstall(struct drm_i915_private *dev_priv)
> >                    I915_DISPLAY_PIPE_B_EVENT_INTERRUPT;
> >
> >         dev_priv->irq_mask |= iir_mask;
> > -       I915_WRITE(VLV_IER, ~dev_priv->irq_mask);
> >         I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> > +       I915_WRITE(VLV_IER, ~dev_priv->irq_mask);
> >         I915_WRITE(VLV_IIR, iir_mask);
> >         I915_WRITE(VLV_IIR, iir_mask);
> >         POSTING_READ(VLV_IIR);
> > @@ -3432,10 +3434,11 @@ static int valleyview_irq_postinstall(struct drm_device *dev)
> >         I915_WRITE(PORT_HOTPLUG_EN, 0);
> >         POSTING_READ(PORT_HOTPLUG_EN);
> >
> > -       I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> > -       I915_WRITE(VLV_IER, ~dev_priv->irq_mask);
> >         I915_WRITE(VLV_IIR, 0xffffffff);
> > -       POSTING_READ(VLV_IER);
> > +       I915_WRITE(VLV_IIR, 0xffffffff);
> > +       I915_WRITE(VLV_IER, ~dev_priv->irq_mask);
> > +       I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> > +       POSTING_READ(VLV_IMR);
> >
> >         /* Interrupt setup is already guaranteed to be single-threaded, this is
> >          * just to make the assert_spin_locked check happy. */
> > @@ -3559,8 +3562,10 @@ static int cherryview_irq_postinstall(struct drm_device *dev)
> >         spin_unlock_irq(&dev_priv->irq_lock);
> >
> >         I915_WRITE(VLV_IIR, 0xffffffff);
> > -       I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> > +       I915_WRITE(VLV_IIR, 0xffffffff);
> >         I915_WRITE(VLV_IER, enable_mask);
> > +       I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> > +       POSTING_READ(VLV_IMR);
> >
> >         gen8_gt_irq_postinstall(dev_priv);
> >
> > @@ -3606,10 +3611,11 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
> >
> >         dev_priv->irq_mask = 0;
> >
> > -       I915_WRITE(VLV_IIR, 0xffffffff);
> >         I915_WRITE(VLV_IMR, 0xffffffff);
> >         I915_WRITE(VLV_IER, 0x0);
> > -       POSTING_READ(VLV_IER);
> > +       I915_WRITE(VLV_IIR, 0xffffffff);
> > +       I915_WRITE(VLV_IIR, 0xffffffff);
> > +       POSTING_READ(VLV_IIR);
> >  }
> >
> >  static void cherryview_irq_uninstall(struct drm_device *dev)
> > @@ -3636,6 +3642,7 @@ static void cherryview_irq_uninstall(struct drm_device *dev)
> >         I915_WRITE(VLV_IMR, 0xffffffff);
> >         I915_WRITE(VLV_IER, 0x0);
> >         I915_WRITE(VLV_IIR, 0xffffffff);
> > +       I915_WRITE(VLV_IIR, 0xffffffff);
> >         POSTING_READ(VLV_IIR);
> >  }
> >
> > --
> > 2.0.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

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