Re: [PATCH 1/3] drm/i915: Invalidate the guc ggtt TLB upon insertion

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 11/01/2017 13:13, Chris Wilson wrote:
> Move the GuC invalidation of its ggtt TLB to where we perform the ggtt
> modification rather than proliferate it into all the callers of the
> insert (which may or may not in fact have to do the insertion).
>
> v2: Just do the guc invalidate unconditionally, (afaict) it has no impact
> without the guc loaded on gen8+
> v3: Conditionally invalidate the guc - just in case that register has
> not been validated for other modes.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c        | 78 +++++++++++++++++++-----------
>  drivers/gpu/drm/i915/i915_gem_gtt.h        |  3 ++
>  drivers/gpu/drm/i915/i915_guc_submission.c |  3 --
>  drivers/gpu/drm/i915/intel_guc_loader.c    |  7 +--
>  drivers/gpu/drm/i915/intel_lrc.c           |  6 ---
>  5 files changed, 57 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 0ed99adfd0da..ed120a1e7f93 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -110,6 +110,30 @@ const struct i915_ggtt_view i915_ggtt_view_rotated = {
>  	.type = I915_GGTT_VIEW_ROTATED,
>  };
>
> +static void gen6_ggtt_invalidate(struct drm_i915_private *dev_priv)
> +{
> +	/* Note that as an uncached mmio write, this should flush the
> +	 * WCB of the writes into the GGTT before it triggers the invalidate.
> +	 */
> +	I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
> +}
> +
> +static void guc_ggtt_invalidate(struct drm_i915_private *dev_priv)
> +{
> +	gen6_ggtt_invalidate(dev_priv);
> +	I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
> +}
> +
> +static void gmch_ggtt_invalidate(struct drm_i915_private *dev_priv)
> +{
> +	intel_gtt_chipset_flush();
> +}
> +
> +static inline void i915_ggtt_invalidate(struct drm_i915_private *i915)
> +{
> +	i915->ggtt.invalidate(i915);
> +}
> +
>  int intel_sanitize_enable_ppgtt(struct drm_i915_private *dev_priv,
>  			       	int enable_ppgtt)
>  {
> @@ -2307,16 +2331,6 @@ void i915_check_and_clear_faults(struct drm_i915_private *dev_priv)
>  		POSTING_READ(RING_FAULT_REG(dev_priv->engine[RCS]));
>  }
>
> -static void i915_ggtt_flush(struct drm_i915_private *dev_priv)
> -{
> -	if (INTEL_INFO(dev_priv)->gen < 6) {
> -		intel_gtt_chipset_flush();
> -	} else {
> -		I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
> -		POSTING_READ(GFX_FLSH_CNTL_GEN6);
> -	}
> -}
> -
>  void i915_gem_suspend_gtt_mappings(struct drm_i915_private *dev_priv)
>  {
>  	struct i915_ggtt *ggtt = &dev_priv->ggtt;
> @@ -2331,7 +2345,7 @@ void i915_gem_suspend_gtt_mappings(struct drm_i915_private *dev_priv)
>
>  	ggtt->base.clear_range(&ggtt->base, ggtt->base.start, ggtt->base.total);
>
> -	i915_ggtt_flush(dev_priv);
> +	i915_ggtt_invalidate(dev_priv);
>  }
>
>  int i915_gem_gtt_prepare_pages(struct drm_i915_gem_object *obj,
> @@ -2370,15 +2384,13 @@ static void gen8_ggtt_insert_page(struct i915_address_space *vm,
>  				  enum i915_cache_level level,
>  				  u32 unused)
>  {
> -	struct drm_i915_private *dev_priv = vm->i915;
> +	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
>  	gen8_pte_t __iomem *pte =
> -		(gen8_pte_t __iomem *)dev_priv->ggtt.gsm +
> -		(offset >> PAGE_SHIFT);
> +		(gen8_pte_t __iomem *)ggtt->gsm + (offset >> PAGE_SHIFT);
>
>  	gen8_set_pte(pte, gen8_pte_encode(addr, level));
>
> -	I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
> -	POSTING_READ(GFX_FLSH_CNTL_GEN6);
> +	ggtt->invalidate(vm->i915);
>  }
>
>  static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
> @@ -2386,7 +2398,6 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
>  				     uint64_t start,
>  				     enum i915_cache_level level, u32 unused)
>  {
> -	struct drm_i915_private *dev_priv = vm->i915;
>  	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
>  	struct sgt_iter sgt_iter;
>  	gen8_pte_t __iomem *gtt_entries;
> @@ -2415,8 +2426,7 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
>  	 * want to flush the TLBs only after we're certain all the PTE updates
>  	 * have finished.
>  	 */
> -	I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
> -	POSTING_READ(GFX_FLSH_CNTL_GEN6);
> +	ggtt->invalidate(vm->i915);
>  }
>
>  struct insert_entries {
> @@ -2451,15 +2461,13 @@ static void gen6_ggtt_insert_page(struct i915_address_space *vm,
>  				  enum i915_cache_level level,
>  				  u32 flags)
>  {
> -	struct drm_i915_private *dev_priv = vm->i915;
> +	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
>  	gen6_pte_t __iomem *pte =
> -		(gen6_pte_t __iomem *)dev_priv->ggtt.gsm +
> -		(offset >> PAGE_SHIFT);
> +		(gen6_pte_t __iomem *)ggtt->gsm + (offset >> PAGE_SHIFT);
>
>  	iowrite32(vm->pte_encode(addr, level, flags), pte);
>
> -	I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
> -	POSTING_READ(GFX_FLSH_CNTL_GEN6);
> +	ggtt->invalidate(vm->i915);
>  }
>
>  /*
> @@ -2473,7 +2481,6 @@ static void gen6_ggtt_insert_entries(struct i915_address_space *vm,
>  				     uint64_t start,
>  				     enum i915_cache_level level, u32 flags)
>  {
> -	struct drm_i915_private *dev_priv = vm->i915;
>  	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
>  	struct sgt_iter sgt_iter;
>  	gen6_pte_t __iomem *gtt_entries;
> @@ -2501,8 +2508,7 @@ static void gen6_ggtt_insert_entries(struct i915_address_space *vm,
>  	 * want to flush the TLBs only after we're certain all the PTE updates
>  	 * have finished.
>  	 */
> -	I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
> -	POSTING_READ(GFX_FLSH_CNTL_GEN6);
> +	ggtt->invalidate(vm->i915);
>  }
>
>  static void nop_clear_range(struct i915_address_space *vm,
> @@ -3062,6 +3068,8 @@ static int gen8_gmch_probe(struct i915_ggtt *ggtt)
>  	if (IS_CHERRYVIEW(dev_priv))
>  		ggtt->base.insert_entries = gen8_ggtt_insert_entries__BKL;
>
> +	ggtt->invalidate = gen6_ggtt_invalidate;
> +
>  	return ggtt_probe_common(ggtt, size);
>  }
>
> @@ -3099,6 +3107,8 @@ static int gen6_gmch_probe(struct i915_ggtt *ggtt)
>  	ggtt->base.unbind_vma = ggtt_unbind_vma;
>  	ggtt->base.cleanup = gen6_gmch_remove;
>
> +	ggtt->invalidate = gen6_ggtt_invalidate;
> +
>  	if (HAS_EDRAM(dev_priv))
>  		ggtt->base.pte_encode = iris_pte_encode;
>  	else if (IS_HASWELL(dev_priv))
> @@ -3142,6 +3152,8 @@ static int i915_gmch_probe(struct i915_ggtt *ggtt)
>  	ggtt->base.unbind_vma = ggtt_unbind_vma;
>  	ggtt->base.cleanup = i915_gmch_remove;
>
> +	ggtt->invalidate = gmch_ggtt_invalidate;
> +
>  	if (unlikely(ggtt->do_idle_maps))
>  		DRM_INFO("applying Ironlake quirks for intel_iommu\n");
>
> @@ -3260,6 +3272,16 @@ int i915_ggtt_enable_hw(struct drm_i915_private *dev_priv)
>  	return 0;
>  }
>
> +void i915_ggtt_enable_guc(struct drm_i915_private *i915)
> +{
> +	i915->ggtt.invalidate = guc_ggtt_invalidate;
> +}
> +
> +void i915_ggtt_disable_guc(struct drm_i915_private *i915)
> +{
> +	i915->ggtt.invalidate = gen6_ggtt_invalidate;
> +}
> +
>  void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv)
>  {
>  	struct i915_ggtt *ggtt = &dev_priv->ggtt;
> @@ -3323,7 +3345,7 @@ void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv)
>  		}
>  	}
>
> -	i915_ggtt_flush(dev_priv);
> +	i915_ggtt_invalidate(dev_priv);
>  }
>
>  struct i915_vma *
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 3e031a057f78..6c40088f8cf4 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -336,6 +336,7 @@ struct i915_ggtt {
>
>  	/** "Graphics Stolen Memory" holds the global PTEs */
>  	void __iomem *gsm;
> +	void (*invalidate)(struct drm_i915_private *dev_priv);
>
>  	bool do_idle_maps;
>
> @@ -504,6 +505,8 @@ i915_vm_to_ggtt(struct i915_address_space *vm)
>  int i915_ggtt_probe_hw(struct drm_i915_private *dev_priv);
>  int i915_ggtt_init_hw(struct drm_i915_private *dev_priv);
>  int i915_ggtt_enable_hw(struct drm_i915_private *dev_priv);
> +void i915_ggtt_enable_guc(struct drm_i915_private *i915);
> +void i915_ggtt_disable_guc(struct drm_i915_private *i915);
>  int i915_gem_init_ggtt(struct drm_i915_private *dev_priv);
>  void i915_ggtt_cleanup_hw(struct drm_i915_private *dev_priv);
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 710fbb9fc63f..913d87358972 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -579,9 +579,6 @@ static struct i915_vma *guc_allocate_vma(struct intel_guc *guc, u32 size)
>  		goto err;
>  	}
>
> -	/* Invalidate GuC TLB to let GuC take the latest updates to GTT. */
> -	I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
> -
>  	return vma;
>
>  err:
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index aa2b866474be..4d26965e3f7f 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -366,9 +366,6 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
>  		return PTR_ERR(vma);
>  	}
>
> -	/* Invalidate GuC TLB to let GuC take the latest updates to GTT. */
> -	I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
> -
>  	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>
>  	/* init WOPCM */
> @@ -486,6 +483,9 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>  	guc_interrupts_release(dev_priv);
>  	gen9_reset_guc_interrupts(dev_priv);
>
> +	/* We need to notify the guc whenever we change the GGTT */
> +	i915_ggtt_enable_guc(dev_priv);
> +
>  	guc_fw->guc_fw_load_status = GUC_FIRMWARE_PENDING;
>
>  	DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
> @@ -547,6 +547,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>  	guc_interrupts_release(dev_priv);
>  	i915_guc_submission_disable(dev_priv);
>  	i915_guc_submission_fini(dev_priv);
> +	i915_ggtt_disable_guc(dev_priv);
>
>  	/*
>  	 * We've failed to load the firmware :(
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 81665a9eb43f..d9de455da131 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -811,12 +811,6 @@ static int execlists_context_pin(struct intel_engine_cs *engine,
>
>  	ce->state->obj->mm.dirty = true;
>
> -	/* Invalidate GuC TLB. */
> -	if (i915.enable_guc_submission) {
> -		struct drm_i915_private *dev_priv = ctx->i915;
> -		I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
> -	}
> -
>  	i915_gem_context_get(ctx);
>  	return 0;
>
>

Looks good to me.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx