[Intel-gfx] [PATCH 6/7] drm/i915/ttm, drm/ttm: Introduce a TTM i915 gem object backend

[Thomas Hellström]

On Wed, 2021-05-12 at 09:09 +0200, Christian König wrote:
> Am 12.05.21 um 09:05 schrieb Thomas Hellström:
> > On Wed, 2021-05-12 at 08:57 +0200, Christian König wrote:
> > > Am 11.05.21 um 16:28 schrieb Thomas Hellström:
> > > > On 5/11/21 4:09 PM, Christian König wrote:
> > > > > 
> > > > > Am 11.05.21 um 16:06 schrieb Thomas Hellström (Intel):
> > > > > > On 5/11/21 3:58 PM, Christian König wrote:
> > > > > > > Am 11.05.21 um 15:25 schrieb Thomas Hellström:
> > > > > > > > Most logical place to introduce TTM buffer objects is
> > > > > > > > as an
> > > > > > > > i915
> > > > > > > > gem object backend. We need to add some ops to account
> > > > > > > > for
> > > > > > > > added
> > > > > > > > functionality like delayed delete and LRU list
> > > > > > > > manipulation.
> > > > > > > > 
> > > > > > > > Initially we support only LMEM and SYSTEM memory, but
> > > > > > > > SYSTEM
> > > > > > > > (which in this case means evicted LMEM objects) is not
> > > > > > > > visible to i915 GEM yet. The plan is to move the i915
> > > > > > > > gem
> > > > > > > > system
> > > > > > > > region
> > > > > > > > over to the TTM system memory type in upcoming patches.
> > > > > > > > 
> > > > > > > > We set up GPU bindings directly both from LMEM and from
> > > > > > > > the
> > > > > > > > system
> > > > > > > > region,
> > > > > > > > as there is no need to use the legacy TTM_TT memory
> > > > > > > > type.
> > > > > > > > We reserve
> > > > > > > > that for future porting of GGTT bindings to TTM.
> > > > > > > > 
> > > > > > > > There are some changes to TTM to allow for purging
> > > > > > > > system
> > > > > > > > memory
> > > > > > > > buffer
> > > > > > > > objects and to refuse swapping of some objects:
> > > > > > > > Unfortunately i915
> > > > > > > > gem
> > > > > > > > still relies heavily on short-term object pinning, and
> > > > > > > > we've
> > > > > > > > chosen to
> > > > > > > > keep short-term-pinned buffer objects on the TTM LRU
> > > > > > > > lists
> > > > > > > > for now,
> > > > > > > > meaning that we need some sort of mechanism to tell TTM
> > > > > > > > they are not
> > > > > > > > swappable. A longer term goal is to get rid of the
> > > > > > > > short-
> > > > > > > > term
> > > > > > > > pinning.
> > > > > > > Well just use the eviction_valuable interface for this.
> > > > > > Yes, we do that for vram/lmem eviction, but we have nothing
> > > > > > similar
> > > > > > for system swapping. Do I understand you correctly that you
> > > > > > want me
> > > > > > to add a call to eviction_valuable() also for that instead
> > > > > > of
> > > > > > swap_possible()?
> > > > > You should already have that. eviction_valuable is called in
> > > > > both
> > > > > cases.
> > > > > 
> > > > Hmm. I can only see it called from ttm_mem_evict_first() which
> > > > is
> > > > not
> > > > in the swapping path? Or do I miss something?
> > > Mhm, looks like my recollection was wrong. We should probably
> > > move
> > > the
> > > call into the ttm_bo_evict_swapout_allowable() function.
> > Yes, I think we also need a convention whether it's called dma_resv
> > locked or not, since the helper accesses bo->mem, which should
> > really
> > only be done under reservation. At the same point, there is value
> > in
> > calling this function while holding the LRU lock.
> 
> You actually need to call it while holding the lock because eviction 
> otherwise ends up in an endless loop.
> 
> Trying to fix that for years, but so far no luck with that.
> 
> > Also, I wonder whether implementations of this callback might
> > encounter
> > unexpected data when called from the swapout path, because at least
> > the
> > helper assumes it not in system memory, since it is accessing bo-
> > > mem.start.
> > So unless we use a separate callback for swapout, there's some
> > auditing
> > to be done.
> 
> Please audit the existing callbacks and move the callback into the 
> function after doing that.
> 
> Thanks,
> Christian.

Would it be OK if I also move the kref_get_unless_zero() to before 
ttm_bo_evict_swapout_allowable() to make the code less sensitive to
surprises?

/Thomas


> 
> > 
> > Pls let me know what you think.
> > Thanks,
> > Thomas
> > 
> > 
> > 
> > > Christian.
> > > 
> > > > Thanks,
> > > > 
> > > > Thomas
> > > > 
> > > > 
> > > > 
> > 
>