[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: 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.

> 
> 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.

> 
> >  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.

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