[PATCH] drm/ttm: fix ttm_bo_unreserve

[Zeng, Oak]

Regards,
Oak

-----Original Message-----
From: Christian König <ckoenig.leichtzumerken at gmail.com> 
Sent: Wednesday, June 5, 2019 7:25 AM
To: Zeng, Oak <Oak.Zeng at amd.com>; Kuehling, Felix <Felix.Kuehling at amd.com>; dri-devel at lists.freedesktop.org; amd-gfx at lists.freedesktop.org
Subject: Re: [PATCH] drm/ttm: fix ttm_bo_unreserve

Am 04.06.19 um 21:03 schrieb Zeng, Oak:
>
> Regards,
> Oak
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of 
> Kuehling, Felix
> Sent: Tuesday, June 4, 2019 2:47 PM
> To: Christian König <ckoenig.leichtzumerken at gmail.com>; 
> dri-devel at lists.freedesktop.org; amd-gfx at lists.freedesktop.org
> Subject: Re: [PATCH] drm/ttm: fix ttm_bo_unreserve
>
> On 2019-06-04 11:23, Christian König wrote:
>
>> Since we now keep BOs on the LRU we need to make sure that they are 
>> removed when they are pinned.
>>
>> Signed-off-by: Christian König <christian.koenig at amd.com>
>> ---
>>    include/drm/ttm/ttm_bo_driver.h | 14 ++++++--------
>>    1 file changed, 6 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/drm/ttm/ttm_bo_driver.h 
>> b/include/drm/ttm/ttm_bo_driver.h index 9f54cf9c60df..c9b8ba492f24
>> 100644
>> --- a/include/drm/ttm/ttm_bo_driver.h
>> +++ b/include/drm/ttm/ttm_bo_driver.h
>> @@ -767,14 +767,12 @@ static inline int ttm_bo_reserve_slowpath(struct ttm_buffer_object *bo,
>>     */
>>    static inline void ttm_bo_unreserve(struct ttm_buffer_object *bo)
>>    {
>> -	if (!(bo->mem.placement & TTM_PL_FLAG_NO_EVICT)) {
>> -		spin_lock(&bo->bdev->glob->lru_lock);
>> -		if (list_empty(&bo->lru))
>> -			ttm_bo_add_to_lru(bo);
>> -		else
>> -			ttm_bo_move_to_lru_tail(bo, NULL);
>> -		spin_unlock(&bo->bdev->glob->lru_lock);
>> -	}
>> +	spin_lock(&bo->bdev->glob->lru_lock);
>> +	if (list_empty(&bo->lru))
>> +		ttm_bo_add_to_lru(bo);
>> +	else
>> +		ttm_bo_move_to_lru_tail(bo, NULL);
> Going just by the function names, this seems to do the exact opposite of what the change description says.
>
> [Oak] +1, when I read the description, I also get lost...So please do add a more accurate description.

I'm puzzled why you are confused. We now keep the BOs on the LRU while they are reserved, so on unreserve we now need to explicitly remove them from the LRU when they are pinned.

[Oak] When I read the description, I though you meant to remove bo from LRU on a pin action, but from codes, it is done on unreserve. In other words, it is better to say "if it is pinned" than  "when it is pinned". Sorry being picky....Also from codes before your change, there was a condition "!(bo->mem.placement & TTM_PL_FLAG_NO_EVICT)". Is this condition to check whether bo is no pinned? How do you check whether bo is pinned in the new codes? To me condition " list_empty(&bo->lru)" only means this bo is currently not on LRU list, I am not sure whether this also means it is not pinned. Also, can ttm_bo_move_to_lru_tail be replaced with ttm_bo_del_from_lru - from your description, this is more like a function to remove it from LRU. Sorry too many questions. I really don't know the context here...

>
> Anway, this patch is Reviewed-by: Felix Kuehling <Felix.Kuehling at amd.com>
>
> BTW, this fix is needed for KFD. It fixes our eviction test that was broken by your previous patch series. This test specifically triggers interactions between KFD and graphics under memory pressure. It's something we rarely see in real world compute application testing without a targeted test. But when it breaks it leads to some painful intermittent failures that are hard to regress and debug.
>
> Do you have any targeted tests to trigger evictions when you work on TTM internals?

Cat amdgpu_evict_gtt in debugfs is a good test for this.

Christian.

>
> Regards,
>     Felix
>
>
>> +	spin_unlock(&bo->bdev->glob->lru_lock);
>>    	reservation_object_unlock(bo->resv);
>>    }
>>    
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx