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

[Chris Wilson]

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?

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.

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?

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

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

-- 
Chris Wilson, Intel Open Source Technology Centre