On 20/07/2022 08:13, Mauro Carvalho Chehab wrote:

On Mon, 18 Jul 2022 14:52:05 +0100 Tvrtko Ursulin tvrtko.ursulin@linux.intel.com wrote:

...

On 14/07/2022 13:06, Mauro Carvalho Chehab wrote:

...

From: Chris Wilson chris.p.wilson@intel.com

Invalidate TLB in patch, in order to reduce performance regressions.

"in batches"?

Yeah. Will fix it.

...

...

diff --git a/drivers/gpu/drm/i915/gt/intel_ppgtt.c b/drivers/gpu/drm/i915/gt/intel_ppgtt.c index d8b94d638559..2da6c82a8bd2 100644 --- a/drivers/gpu/drm/i915/gt/intel_ppgtt.c +++ b/drivers/gpu/drm/i915/gt/intel_ppgtt.c @@ -206,8 +206,12 @@ void ppgtt_bind_vma(struct i915_address_space *vm, void ppgtt_unbind_vma(struct i915_address_space *vm, struct i915_vma_resource *vma_res) {

• if (vma_res->allocated)

• vm->clear_range(vm, vma_res->start, vma_res->vma_size);
  

• if (!vma_res->allocated)

• return;
  

• vm->clear_range(vm, vma_res->start, vma_res->vma_size);
• if (vma_res->tlb)

• vma_invalidate_tlb(vm, *vma_res->tlb);
  

The patch is about more than batching? If there is a security hole in this area (unbind) with the current code?

No, I don't think there's a security hole. The rationale for this is not due to it.

In this case obvious question is why are these changes in the patch which declares itself to be about batching invalidations? Because...

Since commit 2f6b90da9192 ("drm/i915: Use vma resources for async unbinding"), VMA unbind can happen either sync or async.

So, the logic needs to do TLB invalidate on two places. After this patch, the code at __i915_vma_evict is:

struct dma_fence *__i915_vma_evict(struct i915_vma *vma, bool async) { ... if (async) unbind_fence = i915_vma_resource_unbind(vma_res, &vma->obj->mm.tlb); else unbind_fence = i915_vma_resource_unbind(vma_res, NULL);

vma->resource = NULL;

atomic_and(~(I915_VMA_BIND_MASK | I915_VMA_ERROR | I915_VMA_GGTT_WRITE),
	   &vma->flags);

i915_vma_detach(vma);

if (!async) {
	if (unbind_fence) {
		dma_fence_wait(unbind_fence, false);
		dma_fence_put(unbind_fence);
		unbind_fence = NULL;
	}
	vma_invalidate_tlb(vma->vm, vma->obj->mm.tlb);
}

...

So, basically, if !async, __i915_vma_evict() will do TLB cache invalidation.

However, when async is used, the actual page release will happen later, at this function:

void ppgtt_unbind_vma(struct i915_address_space *vm, struct i915_vma_resource *vma_res) { if (!vma_res->allocated) return;

vm->clear_range(vm, vma_res->start, vma_res->vma_size);
if (vma_res->tlb)
	vma_invalidate_tlb(vm, *vma_res->tlb);

}

.. frankly I don't follow since I don't see any page release happening in here. Just PTE clearing.

I am explaining why it looks to me that the patch is doing two things. Implementing batching _and_ adding invalidation points at VMA unbind sites, while so far we had it at backing store release only. Maybe I am wrong and perhaps I am too slow to pick up on the explanation here.

So if the patch is doing two things please split it up.

I am further confused by the invalidation call site in evict and in unbind - why there can't be one logical site since the logical sequence is evict -> unbind.

Regards,

Tvrtko