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

[Christian König]

Am 12.05.21 um 15:02 schrieb 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?

No, because then you need a kref_put while holding the spinlock which is 
not allowed.

Christian.

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