Re: [PATCH v4 10/14] drm/i915/ttm: hide shmem objects from TTM LRU

["Christian König" <christian.koenig@amd.com>]

Am 22.09.21 um 15:34 schrieb Matthew Auld:
> On 21/09/2021 12:48, Christian König wrote:
>> Am 21.09.21 um 13:01 schrieb Matthew Auld:
>>> This is probably a NAK. But ideally we need to somehow prevent TTM from
>>> seeing shmem objects when doing its LRU swap walk. Since these are
>>> EXTERNAL they are ignored anyway, but keeping them in the LRU seems
>>> pretty wasteful.  Trying to use bo_pin() for this is all kinds of nasty
>>> since we need to be able to do the bo_unpin() from the unpopulate hook,
>>> but since that can be called from the BO destroy path we will likely go
>>> down in flames.
>>>
>>> An alternative is to maybe just add EXTERNAL objects to some
>>> bdev->external LRU in TTM, or just don't add them at all?
>>
>> Yeah, that goes into the same direction as why I want to push the LRU 
>> into the resource for some time.
>>
>> The problem is that the LRU is needed for multiple things. E.g. 
>> swapping, GART management, resource constrains, IOMMU teardown etc..
>>
>> So for now I think that everything should be on the LRU even if it 
>> isn't valid to be there for some use case.
>
> Ok. Is it a no-go to keep TT_FLAG_EXTERNAL on say bdev->external?

We could add that as a workaround, but I would rather aim for cleaning 
that up more thoughtfully.

Regards,
Christian.

>
>>
>> Regards,
>> Christian.
>>
>>>
>>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>> Cc: Christian König <christian.koenig@amd.com>
>>> ---
>>>   drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 17 +++++++++++++++++
>>>   1 file changed, 17 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c 
>>> b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>>> index 174aebe11264..b438ddb52764 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>>> @@ -800,6 +800,22 @@ static unsigned long i915_ttm_io_mem_pfn(struct 
>>> ttm_buffer_object *bo,
>>>       return ((base + sg_dma_address(sg)) >> PAGE_SHIFT) + ofs;
>>>   }
>>> +static void i915_ttm_del_from_lru_notify(struct ttm_buffer_object *bo)
>>> +{
>>> +    struct i915_ttm_tt *i915_tt =
>>> +        container_of(bo->ttm, typeof(*i915_tt), ttm);
>>> +
>>> +    /* Idealy we need to prevent TTM from seeing shmem objects when 
>>> doing
>>> +     * its LRU swap walk. Since these are EXTERNAL they are ignored 
>>> anyway,
>>> +     * but keeping them in the LRU is pretty waseful. Trying to use 
>>> bo_pin()
>>> +     * for this is very nasty since we need to be able to do the 
>>> bo_unpin()
>>> +     * from the unpopulate hook, but since that can be called from 
>>> the BO
>>> +     * destroy path we will go down in flames.
>>> +     */
>>> +    if (bo->ttm && ttm_tt_is_populated(bo->ttm) && i915_tt->is_shmem)
>>> +        list_del_init(&bo->lru);
>>> +}
>>> +
>>>   static struct ttm_device_funcs i915_ttm_bo_driver = {
>>>       .ttm_tt_create = i915_ttm_tt_create,
>>>       .ttm_tt_populate = i915_ttm_tt_populate,
>>> @@ -810,6 +826,7 @@ static struct ttm_device_funcs 
>>> i915_ttm_bo_driver = {
>>>       .move = i915_ttm_move,
>>>       .swap_notify = i915_ttm_swap_notify,
>>>       .delete_mem_notify = i915_ttm_delete_mem_notify,
>>> +    .del_from_lru_notify = i915_ttm_del_from_lru_notify,
>>>       .io_mem_reserve = i915_ttm_io_mem_reserve,
>>>       .io_mem_pfn = i915_ttm_io_mem_pfn,
>>>   };
>>