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

[Christian König <christian.koenig@xxxxxxx>]

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

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