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@xxxxxxxxxxxxxxxxxx>
> >Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> >---
> > 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
