Message-ID: <a5605b3a-173d-d4a4-fccf-7cf6ba559913@linux.intel.com>
Date:   Thu, 28 Jul 2022 11:11:27 +0100
From:   Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>
To:     Mauro Carvalho Chehab <mauro.chehab@...ux.intel.com>
Cc:     stable@...r.kernel.org,
        Thomas Hellström 
        <thomas.hellstrom@...ux.intel.com>, linux-media@...r.kernel.org,
        David Airlie <airlied@...ux.ie>,
        intel-gfx@...ts.freedesktop.org,
        Lucas De Marchi <lucas.demarchi@...el.com>,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        Christian König <christian.koenig@....com>,
        linaro-mm-sig@...ts.linaro.org,
        Chris Wilson <chris.p.wilson@...el.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        Dave Airlie <airlied@...hat.com>,
        Tomas Winkler <tomas.winkler@...el.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Matthew Auld <matthew.auld@...el.com>
Subject: Re: [Intel-gfx] [PATCH v2 06/21] drm/i915/gt: Batch TLB invalidations


On 28/07/2022 07:32, Mauro Carvalho Chehab wrote:
> On Wed, 27 Jul 2022 13:56:50 +0100
> Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com> wrote:
> 
>>> 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.
>>
>> Sorry still not clear to me why is the patch moving marking of the need
>> to invalidate (regardless if it a bit like today, or a seqno like in
>> this patch) from bind to unbind?
>>
>> What if the seqno was stored in i915_vma_bind, where the bit is set
>> today, and all the hunks which touch the unbind and evict would
>> disappear from the patch. What wouldn't work in that case, if anything?
> 
> Ah, now I see your point.
> 
> I can't see any sense on having a sequence number at VMA bind, as the
> unbind order can be different. The need of doing a full TLB invalidation
> or not depends on the unbind order.

Sorry yes that was stupid from me.. What I was really thinking was the 
approach I initially used for coalescing. Keeping the set_bit in bind 
and then once the code enters intel_gt_invalidate_tlbs, takes a "ticket" 
and waits on the mutex. Once it gets the mutex checks the ticket against 
the GT copy and if two invalidations have passed since it was waiting on 
the mutex it can immediately exit. That would seem like a minimal 
improvement to batch things up.

But I guess it would still emit needless invalidations if there is no 
contention, just a stream of serialized put pages. While the approach 
from this patch can skip all but truly required.

Okay, go for it and thanks for the explanations.

Acked-by: Tvrtko Ursulin <tvrtko.ursulin@...el.com>

Regards,

Tvrtko

P.S. The last remaining "ugliness" is the 2nd call to invalidation from 
evict. It would be nicer if there was a single common place to do it on 
vma unbind but okay, I do not plan to dig into it so fine.

> 
> The way the current algorithm works is that drm_i915_gem_object can be
> created on any order, and, at unbind/evict, they receive a seqno.
> 
> The seqno is incremented at intel_gt_invalidate_tlb():
> 
>      void intel_gt_invalidate_tlb(struct intel_gt *gt, u32 seqno)
>      {
> 	with_intel_gt_pm_if_awake(gt, wakeref) {
> 		mutex_lock(&gt->tlb.invalidate_lock);
> 		if (tlb_seqno_passed(gt, seqno))
> 				goto unlock;
> 
> 		mmio_invalidate_full(gt);
> 
> 		write_seqcount_invalidate(&gt->tlb.seqno);	// increment seqno
> 		
> 
> So, let's say 3 objects were created, on this order:
> 
> 	obj1
> 	obj2
> 	obj3
> 
> They would be unbind/evict on a different order. On that time,
> the mm.tlb will be stamped with a seqno, using the number from the
> last TLB flush, plus 1.
> 
> As different threads can be used to handle TLB flushes, let's imagine
> two threads (just for the sake of having an example). On such case,
> what we would have is:
> 
> seqno		Thread 0			Thread 1
> 
> seqno=2		unbind/evict event
> 		obj3.mm.tlb = seqno | 1
> seqno=2		unbind/evict event
> 		obj1.mm.tlb = seqno | 1
> 						__i915_gem_object_unset_pages()
> 						called for obj3, TLB flush happened,
> 						invalidating both obj1 and obj2.
> 						seqno += 2					
> seqno=4		unbind/evict event
> 		obj1.mm.tlb = seqno | 1
> 						__i915_gem_object_unset_pages()
> 						called for obj1, don't flush.
> ...
> 						__i915_gem_object_unset_pages() called for obj2, TLB flush happened
> 						seqno += 2
> seqno=6
> 
> So, basically the seqno is used to track when the object data stopped
> being updated, because of an unbind/evict event, being later used by
> intel_gt_invalidate_tlb() when called from __i915_gem_object_unset_pages(),
> in order to check if a previous invalidation call was enough to invalidate
> the object, or if a new call is needed.
> 
> Now, if seqno is stored at bind, data can still leak, as the assumption
> made by intel_gt_invalidate_tlb() that the data stopped being used at
> seqno is not true anymore.
> 
> Still, I agree that this logic is complex and should be better
> documented. So, if you're now OK with this patch, I'll add the above
> explanation inside a kernel-doc comment.
> 
> Regards,
> Mauro

Powered by blists - more mailing lists