Am 15.06.19 um 15:56 schrieb Daniel Vetter:
On Fri, Jun 14, 2019 at 10:30 PM Daniel Vetter <daniel@xxxxxxxx> wrote:
On Fri, Jun 14, 2019 at 08:51:11PM +0200, Christian König wrote:
Am 14.06.19 um 20:24 schrieb Daniel Vetter:
On Fri, Jun 14, 2019 at 8:10 PM Christian König <ckoenig.leichtzumerken@xxxxxxxxx> wrote:
[SNIP]
WW_MUTEX_LOCK_BEGIN()
lock(lru_lock);
while (bo = list_first(lru)) {
if (kref_get_unless_zero(bo)) {
unlock(lru_lock);
WW_MUTEX_LOCK(bo->ww_mutex);
lock(lru_lock);
} else {
/* bo is getting freed, steal it from the freeing process
* or just ignore */
}
}
unlock(lru_lock)
WW_MUTEX_LOCK_END;
Ah, now I know what you mean. And NO, that approach doesn't work.
See for the correct ww_mutex dance we need to use the iterator multiple
times.
Once to give us the BOs which needs to be locked and another time to give us
the BOs which needs to be unlocked in case of a contention.
That won't work with the approach you suggest here.
A right, drat.
Maybe give up on the idea to make this work for ww_mutex in general, and
just for drm_gem_buffer_object? I'm just about to send out a patch series
which makes sure that a lot more drivers set gem_bo.resv correctly (it
will alias with ttm_bo.resv for ttm drivers ofc). Then we could add a
list_head to gem_bo (won't really matter, but not something we can do with
ww_mutex really), so that the unlock walking doesn't need to reuse the
same iterator. That should work I think ...
Also, it would almost cover everything you want to do. For ttm we'd need
to make ttm_bo a subclass of gem_bo (and maybe not initialize that
embedded gem_bo for vmwgfx and shadow bo and driver internal stuff).
Just some ideas, since copypasting the ww_mutex dance into all drivers is
indeed not great.
Even better we don't need to force everyone to use drm_gem_object, the
hard work is already done with the reservation_object. We could add a
list_head there for unwinding, and then the locking helpers would look
a lot cleaner and simpler imo. reservation_unlock_all() would even be
a real function! And if we do this then I think we should also have a
reservation_acquire_ctx, to store the list_head and maybe anything
else.
Plus all the code you want to touch is dealing with
reservation_object, so that's all covered. And it mirros quite a bit
what we've done with struct drm_modeset_lock, to wrap ww_mutex is
something easier to deal with for kms.
That's a rather interesting idea.
I wouldn't use a list_head cause that has a rather horrible caching
footprint for something you want to use during CS, but apart from that
the idea sounds like it would also solve a bunch of problem during eviction.
Going to give that a try,
Christian.
-Daniel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
