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

[Thomas Hellström]

On 5/18/21 6:30 PM, Christian König wrote:
> Am 18.05.21 um 18:07 schrieb Thomas Hellström:
>>
>> On 5/18/21 5:42 PM, Christian König wrote:
>>> 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 at amd.com>
>>>>>>>>> Signed-off-by: Thomas Hellström 
>>>>>>>>> <thomas.hellstrom at 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.
>>
>> Hmm, the pipeline_gutting function seems ok, but overly complex if 
>> the bo is already idle, Am I allowed to optimize it slightly for the 
>> latter case?
>
> Yeah, sure. We just never hat that use case so far.

One thing about the code here that makes me worried is that the 
"destination" ttm_tt is allocated *after* pipeline_gutting. We're not 
really allowed to fail here because that would leave the BO in a state 
where codepaths (fault for example) try to access a NULL ttm_tt. While 
the idle case can get away with ttm_tt_unpopulate, for the async case, 
ttm_tt really needs to be pre-allocated, so that we can leave the bo in 
a consistent state.

/Thomas


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