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

[Thomas Hellström]

On 5/12/21 3:05 PM, Christian König wrote:
> 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.

Ugh. yes, you're right.

/Thomas