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

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

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

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220719/37711abc/attachment-0001.sig>