[PATCH] drm/ttm: make ttm_bo_unpin more defensive

[Thomas Hellström (Intel)]

On 3/15/21 8:48 AM, Thomas Zimmermann wrote:
> Hi
>
> Am 13.03.21 um 19:29 schrieb Thomas Hellström (Intel):
>> Hi, Christian
>>
>> On 3/12/21 10:38 AM, Christian König wrote:
>>> We seem to have some more driver bugs than thought.
>>>
>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>> ---
>>>   include/drm/ttm/ttm_bo_api.h | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/drm/ttm/ttm_bo_api.h 
>>> b/include/drm/ttm/ttm_bo_api.h
>>> index 4fb523dfab32..df9fe596e7c5 100644
>>> --- a/include/drm/ttm/ttm_bo_api.h
>>> +++ b/include/drm/ttm/ttm_bo_api.h
>>> @@ -603,9 +603,11 @@ static inline void ttm_bo_pin(struct 
>>> ttm_buffer_object *bo)
>>>   static inline void ttm_bo_unpin(struct ttm_buffer_object *bo)
>>>   {
>>>       dma_resv_assert_held(bo->base.resv);
>>> -    WARN_ON_ONCE(!bo->pin_count);
>>>       WARN_ON_ONCE(!kref_read(&bo->kref));
>>> -    --bo->pin_count;
>>> +    if (bo->pin_count)
>>> +        --bo->pin_count;
>>> +    else
>>> +        WARN_ON_ONCE(true);
>>>   }
>>>   int ttm_mem_evict_first(struct ttm_device *bdev,
>>
>> Since I now have been staring for half a year at the code of the 
>> driver that made pinning an art, I have a couple of suggestions here, 
>> Could we use an atomic for pin_count, allowing unlocked unpinning but 
>> require the lock only for pin_count transition 0->1, (but unlocked 
>> for pin_if_already_pinned). In particular I think vmwgfx would 
>> benefit from unlocked unpins. Also if the atomic were a refcount_t, 
>> that would probably give you the above behaviour?
>
> What's the benefit?
>
> I'm asking because, there's been talk about streamlining all the GEM 
> locking and actually allowing dma-buf resv locking in pin and vmap 
> operations. Atomic ops might not contradict this, but also might not 
> be useful in the long term.

The benefit would be that at unpinning time it might be tricky to take 
the reservation lock, because you might be already in a ww transaction.

But Christian pointed out the LRU complications...

/Thomas