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

[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?

My assumption was that drm_gem_vmap()/vunmap() is always called with the 
lock held, but Dmitry's work is now suggesting otherwise.

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);
>