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

[Chris Wilson]

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.

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

-- 
Chris Wilson, Intel Open Source Technology Centre