On Mon, May 22, 2017 at 10:18:31PM +0000, Bloomfield, Jon wrote: > > -----Original Message----- > > From: Chris Wilson [mailto:chris@xxxxxxxxxxxxxxxxxx] > > Sent: Monday, May 22, 2017 1:05 PM > > To: Bloomfield, Jon <jon.bloomfield@xxxxxxxxx> > > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > Subject: Re: [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@xxxxxxxxx> > > > > 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@xxxxxxxxx> > > > --- > > > 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(>t_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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
