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

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

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@xxxxxxx/

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@xxxxxxx>
> > > 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

Attachment: OpenPGP_signature
Description: OpenPGP digital signature