> -----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. > > 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. > > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
