[PATCH] drm/ttm: fix ttm_bo_unreserve

[Koenig, Christian]

Am 05.06.19 um 16:33 schrieb 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.

Ah! Yes of course, I thought you mean the ttm_bo_unreserve function name.

Going to add a comment when we start to rename the functions.

Christian.

>
> Regards,
>     Felix
>
>
>>