[Intel-gfx] [PATCH] drm/i915: Serialize GTT Updates on BXT

[Bloomfield, Jon]

> -----Original Message-----
> From: Chris Wilson [mailto:chris at chris-wilson.co.uk]
> Sent: Tuesday, May 23, 2017 12:33 AM
> To: Bloomfield, Jon <jon.bloomfield at intel.com>
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Serialize GTT Updates on BXT
> 
> On Mon, May 22, 2017 at 10:18:31PM +0000, Bloomfield, Jon wrote:
> > > -----Original Message-----
> > > From: Chris Wilson [mailto:chris at chris-wilson.co.uk]
> > > Sent: Monday, May 22, 2017 1:05 PM
> > > To: Bloomfield, Jon <jon.bloomfield at intel.com>
> > > Cc: intel-gfx at lists.freedesktop.org
> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Serialize GTT Updates on BXT
> > >
> > > On Mon, May 22, 2017 at 11:07:25AM -0700, Jon Bloomfield wrote:
> > > > BXT requires accesses to the GTT (i.e. PTE updates) to be serialized
> > > > when IOMMU is enabled.
> > >
> > > Serialised with what, since all writes are serialized already?
> >
> > Fair cop guv. I'll reword.
> >
> > >
> > > The reason is that you need to explain the hw model you are protecting,
> for
> > > example do clears need to be protected?
> > >
> > > > This patch guarantees this by wrapping all updates in stop_machine and
> > > > using a flushing read to guarantee that the GTT writes have reached
> > > > their destination before restarting.
> > >
> > > If you mention which patch you are reinstating (for a new problem) and
> cc
> > > the author, he might point out what has changed in the meantime.
> >
> > I don't understand. I'm not re-instating any patches to my knowledge, so
> it's a bit hard to cc the author.
> 
> Please then review history before submitting *copied* code.
> 
> > > Note, the flush here is not about ensuring the GTT writes reach their
> > > destination.
> > >
> > > > Signed-off-by: Jon Bloomfield <jon.bloomfield at intel.com>
> > >
> > > If you are the author and sender, what is John's s-o-b doing afterwards?
> > This patch was previously signed off by John.
> >
> > >
> > > > Signed-off-by: John Harrison <john.C.Harrison at intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_gem_gtt.c | 106
> > > > ++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 106 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > index 7c769d7..6360d92 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > @@ -2191,6 +2191,100 @@ static void gen8_ggtt_clear_range(struct
> > > i915_address_space *vm,
> > > >  		gen8_set_pte(&gtt_base[i], scratch_pte);  }
> > > >
> > > > +#ifdef CONFIG_INTEL_IOMMU
> > > > +struct insert_page {
> > > > +	struct i915_address_space *vm;
> > > > +	dma_addr_t addr;
> > > > +	u64 offset;
> > > > +	enum i915_cache_level level;
> > > > +};
> > > > +
> > > > +static int gen8_ggtt_insert_page__cb(void *_arg) {
> > > > +	struct insert_page *arg = _arg;
> > > > +
> > > > +	struct drm_i915_private *dev_priv = arg->vm->i915;
> > > > +
> > > > +	gen8_ggtt_insert_page(arg->vm, arg->addr,
> > > > +				arg->offset, arg->level, 0);
> > > > +
> > > > +	POSTING_READ(GFX_FLSH_CNTL_GEN6);
> > >
> > > This is now just a call to i915_ggtt_invalidate() because we are now also
> > > responsible for invalidating the guc tlbs as well as the chipset.
> > > And more importantly it is already done by gen8_ggtt_insert_page.
> > >
> > > All the POSTING_READ(GFX_FLSH_CNTL_GEN6) are spurious.
> >
> > Are you sure - The purpose of the register read is to ensure that all the PTE
> writes are flushed from the h/w queue
> > before we restart the machine. It is critical that all the PTE writes have left
> this queue before any other accesses are
> > allowed to begin.
> > Isn't the invalidate a posted write ? If so, it won't drain the queue.
> > Even if the invalidate is guaranteed to effect this pipeline flish, the
> clear_page path doesn't call invalidate, so it's
> > certainly required there.
> 
> It's an uncached mmio write. It is strongly ordered and serial. Besides
> if you feel it is wrong, fix the bug at source.

Strongly ordered is not enough, nor is being uncached - that just ensures the PTE
writes have left the CPU. We need to ensure they have left the GAM before we
allow any following Aperture accesses to reach the GAM. On the other hand it
may be that the write to the flushctl reg will explicitly cause the h/w to clear the
fifo. I'll check with hw.

Re fixing at source: Assuming you mean just put the read into the gtt functions 
next to the invalidate, then I can do that, but it would mean either another 
bxt/iommu test or executing the read unconditionally for all gens. Are you happy
with that, and if so which ?

> 
> > > >  static void gen6_ggtt_clear_range(struct i915_address_space *vm,
> > > >  				  u64 start, u64 length)
> > > >  {
> > > > @@ -2789,6 +2883,18 @@ static int gen8_gmch_probe(struct i915_ggtt
> > > > *ggtt)
> > > >
> > > >  	ggtt->base.insert_entries = gen8_ggtt_insert_entries;
> > > >
> > > > +#ifdef CONFIG_INTEL_IOMMU
> > > > +	/* Serialize GTT updates on BXT if VT-d is on. */
> > > > +	if (IS_BROXTON(dev_priv) && intel_iommu_gfx_mapped) {
> > >
> > > Move to a header and don't ifdef out the users. A small cost in object side
> for
> > > the benefit of keeping these ifdef out of code.
> > Move what to a header ? You mean create a macro for the test, the whole
> block, or something else ?
> >
> > I was following the pattern used elsewhere in the code in the vain hope
> that by following established
> > convention we might avoid bike-shedding. Every single use of
> intel_iommu_gfx_mapped in this file is protected by
> > the #ifdef.
> 
> Otoh, the recent patches adding more intel_iommu_gfx_mapped have
> avoided
> adding ifdefs to the code.

I've searched the i915 directory on drm-intel-nightly and not found one instance
of intel_iommu_gfx_mapped that is outside a #ifdef. But I'm happy to make my
patch the only one.

> -Chris
> 
> --
> Chris Wilson, Intel Open Source Technology Centre