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@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.
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@xxxxxxxxx>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
