[Intel-gfx] [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 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?

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