On Wed, 20 Jul 2022 11:49:59 +0100
Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
> 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.
> > +void vma_invalidate_tlb(struct i915_address_space *vm, u32 tlb)
> > +{
> > + /*
> > + * Before we release the pages that were bound by this vma, we
> > + * must invalidate all the TLBs that may still have a reference
> > + * back to our physical address. It only needs to be done once,
> > + * so after updating the PTE to point away from the pages, record
> > + * the most recent TLB invalidation seqno, and if we have not yet
> > + * flushed the TLBs upon release, perform a full invalidation.
> > + */
> > + WRITE_ONCE(tlb, intel_gt_next_invalidate_tlb_full(vm->gt));
>
> Shouldn't tlb be a pointer for this to make sense?
Oh, my mistake! Will fix at the next version.
> >
> >>> 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...
Because vma_invalidate_tlb() basically stores a TLB seqno, but the
actual invalidation is deferred to when the pages are unset, at
__i915_gem_object_unset_pages().
So, what happens is:
- on VMA sync mode, the need to invalidate TLB is marked at
__vma_put_pages(), before VMA unbind;
- on async, this is deferred to happen at ppgtt_unbind_vma(), where
it marks the need to invalidate TLBs.
On both cases, __i915_gem_object_unset_pages() is called later,
when the driver is ready to unmap the page.
> 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.
The invalidation happens only on one place: __i915_gem_object_unset_pages().
Despite its name, vma_invalidate_tlb() just marks the need of doing TLB
invalidation.
Regards,
Mauro
