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

[Christian König]

Am 19.05.21 um 08:27 schrieb 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.

Well the original plan was to make tt allocation purely optional.

But I didn't had the time so far to actually fix that.

Christian.

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