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

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 at intel.com>

Regards,

Tvrtko