On Tue, Feb 11, 2020 at 4:23 PM Christian König ckoenig.leichtzumerken@gmail.com wrote:
Am 11.02.20 um 16:02 schrieb Pan, Xinhui:
2020年2月11日 22:14，Daniel Vetter daniel@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@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@lists.freedesktop.org https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.free...
-- Daniel Vetter Software Engineer, Intel Corporation https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll....