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 > > > > > > > > > > > > > > >
