[PATCH 5/6] drm/ttm: replace dma_resv object on deleted BOs v2

[Daniel Vetter]

On Tue, Feb 11, 2020 at 4:23 PM Christian König
<ckoenig.leichtzumerken at gmail.com> wrote:
>
> Am 11.02.20 um 16:02 schrieb Pan, Xinhui:
> >
> >> 2020年2月11日 22:14，Daniel Vetter <daniel at ffwll.ch> 写道：
> >>
> >> On Mon, Feb 10, 2020 at 04:09:06PM +0100, Christian König wrote:
> >>> When non-imported BOs are resurrected for delayed delete we replace
> >>> the dma_resv object to allow for easy reclaiming of the resources.
> >>>
> >>> v2: move that to ttm_bo_individualize_resv
> >>>
> >>> Signed-off-by: Christian König <christian.koenig at amd.com>
> >>> ---
> >>> drivers/gpu/drm/ttm/ttm_bo.c | 10 +++++++++-
> >>> 1 file changed, 9 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> >>> index d0624685f5d2..4d161038de98 100644
> >>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> >>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> >>> @@ -393,6 +393,14 @@ static int ttm_bo_individualize_resv(struct ttm_buffer_object *bo)
> >>>
> >>>     r = dma_resv_copy_fences(&bo->base._resv, bo->base.resv);
> >>>     dma_resv_unlock(&bo->base._resv);
> >>> +   if (r)
> >>> +           return r;
> >>> +
> >>> +   if (bo->type != ttm_bo_type_sg) {
> >>> +           spin_lock(&ttm_bo_glob.lru_lock);
> >>> +           bo->base.resv = &bo->base._resv;
> >> Having the dma_resv pointer be protected by the lru_lock for ttm internal
> >> stuff, but invariant everywhere else is really confusing. Not sure that's
> > I think this is reader VS writer.
> > To avoid any internal functions using the old resv,  using an existing spin lock is acceptable.
> > Maybe RCU is better? That will need a lot of effort.
> > Anyway, ttm sucks. We HAS done a lot of work on it to make it better running on modern system.
>
> Yeah that summarize my recent presentation about TTM pretty much :)
>
> Double checked that and the only reason we have the lock is that in
> ttm_mem_evict_first() we trylock first and then grab a reference.

Well even with the refcount stuff going on I think you're missing a
pile of barriers if you drop the spinlock. Refcounts are otherwise
unordered atomics (and the kref_get_unless_zero trick always needs
something else that guarantees that there's at least a weak reference,
i.e. something which cause a full synchronization somehow with the
release/free code for that thing). So would minimally needs rcu, or
like you do here the lru list spinlock.

> So we should probably rework that code as well and then we can also drop
> that lock here, but that should come later.

_If_ the trylock in evict is the only one then I agree this should be
safe. But definitely needs a comment explaining what exactly is going
on. Imo at least :-)

Cheers, Daniel
>
> Christian.
>
> >
> >
> >> a great idea, I've just chased some ttm code around freaking out about
> >> that.
> >> -Daniel
> >>
> >>> +           spin_unlock(&ttm_bo_glob.lru_lock);
> >>> +   }
> >>>
> >>>     return r;
> >>> }
> >>> @@ -720,7 +728,7 @@ static bool ttm_bo_evict_swapout_allowable(struct ttm_buffer_object *bo,
> >>>
> >>>     if (bo->base.resv == ctx->resv) {
> >>>             dma_resv_assert_held(bo->base.resv);
> >>> -           if (ctx->flags & TTM_OPT_FLAG_ALLOW_RES_EVICT || bo->deleted)
> >>> +           if (ctx->flags & TTM_OPT_FLAG_ALLOW_RES_EVICT)
> >>>                     ret = true;
> >>>             *locked = false;
> >>>             if (busy)
> >>> --
> >>> 2.17.1
> >>>
> >>> _______________________________________________
> >>> dri-devel mailing list
> >>> dri-devel at lists.freedesktop.org
> >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=02%7C01%7CXinhui.Pan%40amd.com%7Cee67310e26b64ca9e79008d7aefca7b4%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637170272481765904&sdata=ZpnP9MNBP1csQCKPR275ejIvsZ3b8xL80tmSlpf7MPA%3D&reserved=0
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch&data=02%7C01%7CXinhui.Pan%40amd.com%7Cee67310e26b64ca9e79008d7aefca7b4%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637170272481765904&sdata=fk28jtHhAnE312CFMgVXaZtaS2YNqJjmyJ317FWjAoM%3D&reserved=0
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch