| Message ID | 20180628123351.31240-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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. Yeah, seems fine by me: Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > /* IRQs are synced during runtime_suspend, we don't require a wakeref */ > enable_rpm_wakeref_asserts(dev_priv); > -- > 2.18.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@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@linux.intel.com> Ta, -Chris
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); - } /* IRQs are synced during runtime_suspend, we don't require a wakeref */ enable_rpm_wakeref_asserts(dev_priv);
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@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_irq.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)