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.gor...@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.gor...@intel.com>
> > Tested-By: PRC QA PRTS (Patch Regression Test System Contact:
> > shuang...@intel.com)
> > Signed-off-by: Rodrigo Vivi <rodrigo.v...@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
