[PATCH] drm/ttm: fix ttm_bo_unreserve

[Kuehling, Felix]

On 2019-06-05 9:56, Michel Dänzer wrote:
> On 2019-06-05 1:24 p.m., Christian König wrote:
>> Am 04.06.19 um 21:03 schrieb Zeng, Oak:
>>> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of
>>> Kuehling, Felix
>>> On 2019-06-04 11:23, Christian König wrote:
[snip]
>>> --- 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.
> I don't know about Felix and Oak, but for me "remove from the LRU" is
> confusing, as I don't see that in the code, only adding to the LRU or
> moving to its tail.

Exactly. The names of the functions being called imply that something 
gets added or moved on the LRU list. You have to go look at the 
implementation of those functions to find out that they do something 
else for pinned BOs (that have TTM_PL_FLAG_NO_EVICT set in their 
placement flags).

Fixing the function names would probably be overkill: 
ttm_bo_add_lru_unless_pinned and 
ttm_bo_move_to_lru_tail_or_remove_if_pinned. But maybe a comment in 
ttm_bo_unreserve would help.

Regards,
   Felix


>
>