[PATCH 1/2] drm/ttm: fix locking in vmap/vunmap TTM GEM helpers

[Christian König]

Am 19.07.22 um 09:19 schrieb Thomas Zimmermann:
> Hi
>
> Am 15.07.22 um 17:58 schrieb Christian König:
>> Am 15.07.22 um 17:36 schrieb Thomas Zimmermann:
>>> Hi
>>>
>>> Am 15.07.22 um 13:15 schrieb Christian König:
>>>> I've stumbled over this while reviewing patches for DMA-buf and it 
>>>> looks
>>>> like we completely messed the locking up here.
>>>>
>>>> In general most TTM function should only be called while holding the
>>>> appropriate BO resv lock. Without this we could break the internal
>>>> buffer object state here.
>>>
>>> I remember that I tried to fix this before and you mentioned that 
>>> it's not allowed to hold this lock here. It would possibly deadlock 
>>> with exported buffers. Did this change with Dmitry's rework of the 
>>> locking semantics?
>>
>> No, can you point me to that?
>
> I thought it was somewhere in the lengthy discussion at
>
>
> https://lore.kernel.org/dri-devel/20201112132117.27228-1-tzimmermann@suse.de/ 
>
>
> but now I cannot find it any longer. :/
>
>>
>> My assumption was that drm_gem_vmap()/vunmap() is always called with 
>> the lock held, but Dmitry's work is now suggesting otherwise.
>
> IIRC the lock was supposed to be held, but every drivers does it 
> differently. And dma-buf locking essentially worked by chance.

I guess we where just lucky that nobody moved the BO while it was being 
mapped.

I've now pushed the patch to take the look to drm-misc-fixes. If some 
call path really called this while already holding the locks we will 
obviously run into trouble, but I think that should be pretty easy to 
sort out.

Regards,
Christian.

>
> Best regards
> Thomas
>
>>
>> We certainly need to hold the lock when we call 
>> ttm_bo_vmap()/vunmap() because it needs to access the bo->resource.
>>
>> Thanks,
>> Christian.
>>
>>>
>>> Best regards
>>> Thomas
>>>
>>>>
>>>> Only compile tested!
>>>>
>>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>>> Fixes: 43676605f890 drm/ttm: Add vmap/vunmap to TTM and TTM GEM 
>>>> helpers
>>>> ---
>>>>   drivers/gpu/drm/drm_gem_ttm_helper.c | 9 ++++++++-
>>>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_gem_ttm_helper.c 
>>>> b/drivers/gpu/drm/drm_gem_ttm_helper.c
>>>> index d5962a34c01d..e5fc875990c4 100644
>>>> --- a/drivers/gpu/drm/drm_gem_ttm_helper.c
>>>> +++ b/drivers/gpu/drm/drm_gem_ttm_helper.c
>>>> @@ -64,8 +64,13 @@ int drm_gem_ttm_vmap(struct drm_gem_object *gem,
>>>>                struct iosys_map *map)
>>>>   {
>>>>       struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
>>>> +    int ret;
>>>> +
>>>> +    dma_resv_lock(gem->resv, NULL);
>>>> +    ret = ttm_bo_vmap(bo, map);
>>>> +    dma_resv_unlock(gem->resv);
>>>>   -    return ttm_bo_vmap(bo, map);
>>>> +    return ret;
>>>>   }
>>>>   EXPORT_SYMBOL(drm_gem_ttm_vmap);
>>>>   @@ -82,7 +87,9 @@ void drm_gem_ttm_vunmap(struct drm_gem_object 
>>>> *gem,
>>>>   {
>>>>       struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
>>>>   +    dma_resv_lock(gem->resv, NULL);
>>>>       ttm_bo_vunmap(bo, map);
>>>> +    dma_resv_unlock(gem->resv);
>>>>   }
>>>>   EXPORT_SYMBOL(drm_gem_ttm_vunmap);
>>>
>>
>