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@xxxxxxxxxxxxxxxxxxxxx> 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
>
>
>>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
