[Intel-gfx] [PATCH 1/9] drm/i915: Drop posting reads to flush master interrupts

[Chris Wilson]

Quoting Ville Syrjälä (2018-06-28 14:06:40)
> On Thu, Jun 28, 2018 at 01:33:43PM +0100, Chris Wilson wrote:
> > We do not need to do a posting read of our uncached mmio write to
> > re-enable the master interrupt lines after handling an interrupt, so
> > don't. This saves us a slow UC read before we can process the interrupt,
> > most noticeable in execlists where any stalls imposes extra latency on
> > GPU command execution.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 9 +--------
> >  1 file changed, 1 insertion(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 7a7c4a2bd778..e83fcedcbf1d 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -2168,7 +2168,6 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
> >  
> >               I915_WRITE(VLV_IER, ier);
> >               I915_WRITE(VLV_MASTER_IER, MASTER_INTERRUPT_ENABLE);
> > -             POSTING_READ(VLV_MASTER_IER);
> >  
> >               if (gt_iir)
> >                       snb_gt_irq_handler(dev_priv, gt_iir);
> > @@ -2253,7 +2252,6 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
> >  
> >               I915_WRITE(VLV_IER, ier);
> >               I915_WRITE(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
> > -             POSTING_READ(GEN8_MASTER_IRQ);
> >  
> >               gen8_gt_irq_handler(dev_priv, master_ctl, gt_iir);
> >  
> > @@ -2622,7 +2620,6 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
> >       /* disable master interrupt before clearing iir  */
> >       de_ier = I915_READ(DEIER);
> >       I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
> > -     POSTING_READ(DEIER);
> >  
> >       /* Disable south interrupts. We'll only write to SDEIIR once, so further
> >        * interrupts will will be stored on its back queue, and then we'll be
> > @@ -2632,7 +2629,6 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
> >       if (!HAS_PCH_NOP(dev_priv)) {
> >               sde_ier = I915_READ(SDEIER);
> >               I915_WRITE(SDEIER, 0);
> > -             POSTING_READ(SDEIER);
> >       }
> >  
> >       /* Find, clear, then process each source of interrupt */
> > @@ -2667,11 +2663,8 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
> >       }
> >  
> >       I915_WRITE(DEIER, de_ier);
> > -     POSTING_READ(DEIER);
> > -     if (!HAS_PCH_NOP(dev_priv)) {
> > +     if (!HAS_PCH_NOP(dev_priv))
> >               I915_WRITE(SDEIER, sde_ier);
> > -             POSTING_READ(SDEIER);
> > -     }
> 
> Not 100% sure about the SDEIER thing. I believe it goes over the link
> to the PCH so I suppose it might be posted. No idea how it would get
> ordered wrt. subsequent accesses to CPU side registers. Hmm. Oh we
> actually restore master first, then SDEIER. But I suppose the order
> doesn't actually matter. As long as SDEIER is 0 at some point we should
> get a new edge in the end if SDEIIR still has something asserted.

I have vague memories of you saying "oh master is done before sdeier",
that's odd (or that might have been me)!
 
> Yeah, seems fine by me:
> Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>

Ta,
-Chris