* Re: [PATCH] drm/i915: Invalidate the guc ggtt TLB upon insertion
2016-11-15 11:16 [PATCH] drm/i915: Invalidate the guc ggtt TLB upon insertion Chris Wilson
@ 2016-11-15 11:28 ` Chris Wilson
2016-11-15 11:45 ` ✓ Fi.CI.BAT: success for " Patchwork
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Chris Wilson @ 2016-11-15 11:28 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply [flat|nested] 7+ messages in thread* ✓ Fi.CI.BAT: success for drm/i915: Invalidate the guc ggtt TLB upon insertion
2016-11-15 11:16 [PATCH] drm/i915: Invalidate the guc ggtt TLB upon insertion Chris Wilson
2016-11-15 11:28 ` Chris Wilson
@ 2016-11-15 11:45 ` Patchwork
2016-11-15 12:13 ` [PATCH] " Daniel Vetter
2016-11-16 8:49 ` Tvrtko Ursulin
3 siblings, 0 replies; 7+ messages in thread
From: Patchwork @ 2016-11-15 11:45 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: Invalidate the guc ggtt TLB upon insertion
2016-11-15 11:16 [PATCH] drm/i915: Invalidate the guc ggtt TLB upon insertion Chris Wilson
2016-11-15 11:28 ` Chris Wilson
2016-11-15 11:45 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2016-11-15 12:13 ` Daniel Vetter
2016-11-16 8:49 ` Tvrtko Ursulin
3 siblings, 0 replies; 7+ messages in thread
From: Daniel Vetter @ 2016-11-15 12:13 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] drm/i915: Invalidate the guc ggtt TLB upon insertion
2016-11-15 11:16 [PATCH] drm/i915: Invalidate the guc ggtt TLB upon insertion Chris Wilson
` (2 preceding siblings ...)
2016-11-15 12:13 ` [PATCH] " Daniel Vetter
@ 2016-11-16 8:49 ` Tvrtko Ursulin
2016-11-16 8:55 ` Chris Wilson
3 siblings, 1 reply; 7+ messages in thread
From: Tvrtko Ursulin @ 2016-11-16 8:49 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] drm/i915: Invalidate the guc ggtt TLB upon insertion
2016-11-16 8:49 ` Tvrtko Ursulin
@ 2016-11-16 8:55 ` Chris Wilson
2016-11-16 9:02 ` Tvrtko Ursulin
0 siblings, 1 reply; 7+ messages in thread
From: Chris Wilson @ 2016-11-16 8:55 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] drm/i915: Invalidate the guc ggtt TLB upon insertion
2016-11-16 8:55 ` Chris Wilson
@ 2016-11-16 9:02 ` Tvrtko Ursulin
0 siblings, 0 replies; 7+ messages in thread
From: Tvrtko Ursulin @ 2016-11-16 9:02 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
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
^ permalink raw reply [flat|nested] 7+ messages in thread