[Intel-gfx] [PATCH 10/11] drm/i915: add irq_barrier operation for synchronising reads

[Chris Wilson]

On Wed, Jan 28, 2015 at 10:55:53AM +0100, Daniel Vetter wrote:
> On Mon, Jan 26, 2015 at 04:43:24AM -0800, Rodrigo Vivi wrote:
> > From: Dave Gordon <david.s.gordon at intel.com>
> > 
> > On some generations of chips, it is necessary to read an MMIO register
> > before getting the sequence number from the status page in main memory,
> > in order to ensure coherency; and on all generations this should be
> > either helpful or harmless.
> > 
> > In general, we want this operation to be the cheapest possible, since
> > we require only the side-effect of DMA completion and don't interpret
> > the result of the read, and don't require any coordination with other
> > threads, power domains, or anything else.
> > 
> > However, finding a suitable register may be problematic; on GEN6 chips
> > the ACTHD register was used, but on VLV et al access to this register
> > requires FORCEWAKE and therefore many complications involving spinlocks
> > and polling.
> > 
> > So this commit introduces this synchronising operation as a distinct
> > vfunc in the engine structure, so that it can be GEN- or chip-specific
> > if needed.
> > 
> > And there are three implementations; a dummy one, for chips where no
> > synchronising read is needed, a gen6(+) version that issues a posting
> > read (to TAIL), and a VLV-specific one that issues a raw read instead,
> > avoiding touching FORCEWAKE and GTFIFO and other such complications.
> > 
> > We then change gen6_ring_get_seqno() to use this new irq_barrier rather
> > than a POSTING_READ of ACTHD. Note that both older (pre-GEN6) and newer
> > (GEN8+) devices running in LRC mode do not currently include any posting
> > read in their own get_seqno() implementations, so this change only
> > makes a difference on VLV (and not CHV+).
> > 
> > Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
> > Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he at intel.com)
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ringbuffer.c | 37 +++++++++++++++++++++++++++++++--
> >  drivers/gpu/drm/i915/intel_ringbuffer.h |  1 +
> >  2 files changed, 36 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index 23020d6..97473ed 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -1227,6 +1227,28 @@ pc_render_add_request(struct intel_engine_cs *ring)
> >  	return 0;
> >  }
> >  
> > +static void
> > +dummy_irq_barrier(struct intel_engine_cs *ring)
> > +{
> > +}
> > +
> > +static void
> > +gen6_irq_barrier(struct intel_engine_cs *ring)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(ring->dev);
> > +	POSTING_READ(RING_TAIL(ring->mmio_base));
> > +}
> > +
> > +#define __raw_i915_read32(dev_priv__, reg__)	readl((dev_priv__)->regs + (reg__))
> > +#define RAW_POSTING_READ(reg__)			(void)__raw_i915_read32(dev_priv, reg__)
> > +
> > +static void
> > +vlv_irq_barrier(struct intel_engine_cs *ring)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(ring->dev);
> > +	RAW_POSTING_READ(RING_TAIL(ring->mmio_base));
> > +}
> > +
> >  static u32
> >  gen6_ring_get_seqno(struct intel_engine_cs *ring, bool lazy_coherency)
> >  {
> > @@ -1234,8 +1256,7 @@ gen6_ring_get_seqno(struct intel_engine_cs *ring, bool lazy_coherency)
> >  	 * ivb (and maybe also on snb) by reading from a CS register (like
> >  	 * ACTHD) before reading the status page. */
> >  	if (!lazy_coherency) {
> > -		struct drm_i915_private *dev_priv = ring->dev->dev_private;
> > -		POSTING_READ(RING_ACTHD(ring->mmio_base));
> > +		ring->irq_barrier(ring);
> >  	}
> 
> Imo just do a vlv_ring_get_seqno if this is a problem. Adding a vfunc with
> mostly empty or same implemenation to another very tiny vfunc isn't doing
> a whole lot of good to the codebase.

Or rather since there is just one place that cares about the
irq_barrier, just call it from that callsite and simplify get_seqno.

But the whole vlv is special argument still seems nebulous in the first
place.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre