[Intel-gfx] [PATCH] drm/i915: Invalidate the guc ggtt TLB upon insertion

[Tvrtko Ursulin]

On 15/11/2016 11:16, 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).
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c        | 15 +++++++++++----
>  drivers/gpu/drm/i915/i915_guc_submission.c |  3 ---
>  drivers/gpu/drm/i915/intel_guc_loader.c    |  3 ---
>  drivers/gpu/drm/i915/intel_lrc.c           |  6 ------
>  4 files changed, 11 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 98420ba87b0d..6aaffcf529c8 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2389,6 +2389,15 @@ static void gen8_set_pte(void __iomem *addr, gen8_pte_t pte)
>  	writeq(pte, addr);
>  }
>
> +static void gen8_ggtt_invalidate(struct drm_i915_private *dev_priv)
> +{
> +	I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
> +	POSTING_READ(GFX_FLSH_CNTL_GEN6);
> +
> +	if (i915.enable_guc_submission)
> +		I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
> +}

Cost is more mmio on every page table updates, even when GuC doesn't 
care, no?

> +
>  static void gen8_ggtt_insert_page(struct i915_address_space *vm,
>  				  dma_addr_t addr,
>  				  uint64_t offset,
> @@ -2402,8 +2411,7 @@ static void gen8_ggtt_insert_page(struct i915_address_space *vm,
>
>  	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);
> +	gen8_ggtt_invalidate(dev_priv);
>  }
>
>  static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
> @@ -2440,8 +2448,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);
> +	gen8_ggtt_invalidate(dev_priv);
>  }
>
>  struct insert_entries {
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 088f5a99ecfc..ea6d2cc74edf 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -770,9 +770,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 e41d728e47d1..cda6747d7e70 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);

I don't know GuC and I don't know if this is safe. Maybe it needs to be 
prodded once the firmware is loaded like this? You think it is OK to 
remove it?

CI run with GuC enabled at least would be required. :)

Regards,

Tvrtko

> -
>  	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>
>  	/* init WOPCM */
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 706191a9b028..e23b6a2600fb 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -873,12 +873,6 @@ static int intel_lr_context_pin(struct i915_gem_context *ctx,
>
>  	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;
>
>