[Intel-gfx] [PATCH 19/21] drm/i915: Move the get/put irq locking into the caller

[Chris Wilson]

On Tue, Jun 07, 2016 at 01:46:53PM +0100, Tvrtko Ursulin wrote:
> 
> On 03/06/16 17:08, Chris Wilson wrote:
> >With only a single callsite for intel_engine_cs->irq_get and ->irq_put,
> >we can reduce the code size by moving the common preamble into the
> >caller, and we can also eliminate the reference counting.
> >
> >For completeness, as we are no longer doing reference counting on irq,
> >rename the get/put vfunctions to enable/disable respectively.
> >
> >Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >---
> >  drivers/gpu/drm/i915/i915_irq.c          |   8 +-
> >  drivers/gpu/drm/i915/intel_breadcrumbs.c |  10 +-
> >  drivers/gpu/drm/i915/intel_lrc.c         |  34 +---
> >  drivers/gpu/drm/i915/intel_ringbuffer.c  | 269 ++++++++++---------------------
> >  drivers/gpu/drm/i915/intel_ringbuffer.h  |   5 +-
> >  5 files changed, 108 insertions(+), 218 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >index 14b3d65bb604..5bdb433dde8c 100644
> >--- a/drivers/gpu/drm/i915/i915_irq.c
> >+++ b/drivers/gpu/drm/i915/i915_irq.c
> >@@ -259,12 +259,12 @@ static void ilk_update_gt_irq(struct drm_i915_private *dev_priv,
> >  	dev_priv->gt_irq_mask &= ~interrupt_mask;
> >  	dev_priv->gt_irq_mask |= (~enabled_irq_mask & interrupt_mask);
> >  	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> >-	POSTING_READ(GTIMR);
> >  }
> >
> >  void gen5_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> >  {
> >  	ilk_update_gt_irq(dev_priv, mask, mask);
> >+	POSTING_READ_FW(GTIMR);
> >  }
> 
> Unrelated hunks?
> 
> How is POSTING_READ_FW correct?

The requirement here is an uncached read of the mmio register in order
to flush the previous write to hw. A grander scheme would be to convert
all posting reads, but that requires double checking to see if anyone
has been cheating!

> Also removes the posting read from disable, OK?

Correct, we only depend upon the ordering with hw on the enable path.
This is one of those rare instances where the barrier is required (and
UC write is not enough!), if we don't we check the "post-enabled seqno"
before the interrupt is ready.

> >+	I915_WRITE_IMR(engine,
> >+		       ~(engine->irq_enable_mask | engine->irq_keep_mask));
> >+	POSTING_READ_FW(RING_IMR(engine->mmio_base));
> 
> Hm, more of _FW following normal access. What am I missing? You are
> not by any chance banking on the auto-release window?

Nope.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre