Messages in this thread | | | Subject | Re: [PATCH 3/6] drm/gem: use new ww_mutex_(un)lock_for_each macros | From | Christian König <> | Date | Mon, 17 Jun 2019 11:30:45 +0200 |
| |
Am 15.06.19 um 15:56 schrieb Daniel Vetter: > On Fri, Jun 14, 2019 at 10:30 PM Daniel Vetter <daniel@ffwll.ch> 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@gmail.com> 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 >
| |