Re: [PATCH v2 09/15] drm/ttm, drm/amdgpu: Allow the driver some control over swapping

From: "Christian König" <christian.koenig@amd.com>
To: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>,
	intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2 09/15] drm/ttm, drm/amdgpu: Allow the driver some control over swapping
Date: Tue, 18 May 2021 17:42:23 +0200	[thread overview]
Message-ID: <fb9a4898-5844-c1e5-7a24-cb50a9ad6df7@amd.com> (raw)
In-Reply-To: <f5c008cb-5047-7cbf-0361-e4e58e38d6e0@linux.intel.com>

Am 18.05.21 um 17:38 schrieb Thomas Hellström:
>
> On 5/18/21 5:28 PM, Christian König wrote:
>> Am 18.05.21 um 17:20 schrieb Thomas Hellström:
>>>
>>> On 5/18/21 5:18 PM, Christian König wrote:
>>>>
>>>>
>>>> Am 18.05.21 um 17:15 schrieb Thomas Hellström:
>>>>>
>>>>> On 5/18/21 10:26 AM, Thomas Hellström wrote:
>>>>>> We are calling the eviction_valuable driver callback at eviction 
>>>>>> time to
>>>>>> determine whether we actually can evict a buffer object.
>>>>>> The upcoming i915 TTM backend needs the same functionality for 
>>>>>> swapout,
>>>>>> and that might actually be beneficial to other drivers as well.
>>>>>>
>>>>>> Add an eviction_valuable call also in the swapout path. Try to 
>>>>>> keep the
>>>>>> current behaviour for all drivers by returning true if the buffer 
>>>>>> object
>>>>>> is already in the TTM_PL_SYSTEM placement. We change behaviour 
>>>>>> for the
>>>>>> case where a buffer object is in a TT backed placement when 
>>>>>> swapped out,
>>>>>> in which case the drivers normal eviction_valuable path is run.
>>>>>>
>>>>>> Finally export ttm_tt_unpopulate() and don't swap out bos
>>>>>> that are not populated. This allows a driver to purge a bo at
>>>>>> swapout time if its content is no longer valuable rather than to
>>>>>> have TTM swap the contents out.
>>>>>>
>>>>>> Cc: Christian König <christian.koenig@amd.com>
>>>>>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>>>>
>>>>> Christian,
>>>>>
>>>>> Here we have a ttm_tt_unpopulate() export as well at the end. I 
>>>>> figure you will push back on that one. What we really need is a 
>>>>> functionality to just drop the bo contents and end up in system 
>>>>> memory unpopulated. Should I perhaps add a utility function to do 
>>>>> that instead? like ttm_bo_purge()?
>>>>
>>>> We already have that. Just call ttm_bo_validate() without any place 
>>>> to put the buffer.
>>>>
>>>> See how ttm_bo_pipeline_gutting() is used.
>>>>
>>>> Christian.
>>>
>>> OK, so is that reentrant from the move() or swap_notify() callback.
>>
>> That sounds like a design bug to me since you should never need to do 
>> this.
>>
>> When you want to destroy the backing store of a buffer during 
>> eviction you should just do this by returning an empty placement from 
>> the evict_flags callback.
>
> So this is for the functionality where the user has indicated that the 
> contents is no longer of value, but the buffer itself
> is cached until evicted or swapped out for performance reasons. So the 
> above would work for eviction, but what about swapout. Could we add 
> some similar functionality there?

Amdgpu has the same functionality and you don't need to handle swap at all.

Just return from the evict_flags that you want to drop the backing store 
as soon as the BO leaves the GTT domain.

Christian.

>
> /Thomas
>
>>
>> Regards,
>> Christian.
>>
>>>
>>> /Thomas
>>>
>>>
>>>
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Thomas
>>>>>
>>>>>
>>>>
>>