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

[Matthew Auld <matthew.auld@intel.com>]

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?

> 
> 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,
>>   };
>