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

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

[Chris Wilson]

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@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@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);
+}
+
 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);
-
 	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;
 
-- 
2.10.2

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

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

[Chris Wilson]

On Tue, Nov 15, 2016 at 11:16:50AM +0000, 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@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@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);

Can we just make it unconditional?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Invalidate the guc ggtt TLB upon insertion

[Patchwork]

== Series Details ==

Series: drm/i915: Invalidate the guc ggtt TLB upon insertion
URL   : https://patchwork.freedesktop.org/series/15336/
State : success

== Summary ==

Series 15336v1 drm/i915: Invalidate the guc ggtt TLB upon insertion
https://patchwork.freedesktop.org/api/1.0/series/15336/revisions/1/mbox/


fi-bdw-5557u     total:244  pass:229  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:244  pass:204  dwarn:0   dfail:0   fail:0   skip:40 
fi-bxt-t5700     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-j1900     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-ilk-650       total:244  pass:191  dwarn:0   dfail:0   fail:0   skip:53 
fi-ivb-3520m     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-ivb-3770      total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-7200u     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:244  pass:223  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:244  pass:222  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-snb-2600      total:244  pass:211  dwarn:0   dfail:0   fail:0   skip:33 

9836c958f440147b2c98839a877fef61e650df8d drm-intel-nightly: 2016y-11m-15d-10h-04m-27s UTC integration manifest
4b6c716 drm/i915: Invalidate the guc ggtt TLB upon insertion

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_2998/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Tue, Nov 15, 2016 at 11:16:50AM +0000, 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@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>

Makes sense.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@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;
>
>

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

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

[Chris Wilson]

On Wed, Nov 16, 2016 at 08:49:24AM +0000, Tvrtko Ursulin wrote:
> 
> 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@chris-wilson.co.uk>
> >Cc: Tvrtko Ursulin <tvrtko.ursulin@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?

Not a huge cost. One extra uc write (and we can kill the uc read)
following a series of wc writes. So in total it evens out :)

And GGTT operations should be relatively rare - more or less new contexts
or memory pressure. At any rate, I feel much safer having the GTT
invalidated at the point of change than having the code dotted about the
place.

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

But we haven't removed it. The invalidate is still here, just done as
the call above. So we aren't changing the flow here.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

On 16/11/2016 08:55, Chris Wilson wrote:
> On Wed, Nov 16, 2016 at 08:49:24AM +0000, Tvrtko Ursulin wrote:
>>
>> 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@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@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?
>
> Not a huge cost. One extra uc write (and we can kill the uc read)
> following a series of wc writes. So in total it evens out :)
>
> And GGTT operations should be relatively rare - more or less new contexts
> or memory pressure. At any rate, I feel much safer having the GTT
> invalidated at the point of change than having the code dotted about the
> place.

Okay, makes sense.

>>> 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?
>
> But we haven't removed it. The invalidate is still here, just done as
> the call above. So we aren't changing the flow here.

Ah yes, you are right. For some reason I saw the old invalidate site as 
after the firmware load, not simply after the VMA pinning.

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