Re: [Intel-gfx] [PATCH v2 06/21] drm/i915/gt: Batch TLB invalidations

[Tvrtko Ursulin]

On 20/07/2022 08:13, Mauro Carvalho Chehab wrote:
> On Mon, 18 Jul 2022 14:52:05 +0100
> Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
>
> > On 14/07/2022 13:06, Mauro Carvalho Chehab wrote:
> > > From: Chris Wilson <chris.p.wilson@xxxxxxxxx>
> > >
> > > 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