| Message ID | 1372333708-29884-5-git-send-email-maarten.lankhorst@canonical.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Thu, Jun 27, 2013 at 01:48:19PM +0200, Maarten Lankhorst wrote: > Now that the code is compatible in semantics, flip the switch. > Use ww_mutex instead of the homegrown implementation. > > ww_mutex uses -EDEADLK to signal that the caller has to back off, > and -EALREADY to indicate this buffer is already held by the caller. > > ttm used -EAGAIN and -EDEADLK for those, respectively. So some changes > were needed to handle this correctly. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> This one change the radeon cs kernel API. We will now return -EALREADY if userspace try to send a cs with same bo two time on the list. Which is bug in itself, but a bug that might have been abuse by some to force trigger gpu reset (even if i am convince that i sent a patch at one point to avoid that). I am not against that change just thought i should point it off. Reviewed-by: Jerome Glisse <jglisse@redhat.com> > --- > drivers/gpu/drm/nouveau/nouveau_gem.c | 2 +- > drivers/gpu/drm/qxl/qxl_object.h | 5 - > drivers/gpu/drm/ttm/ttm_bo.c | 190 +++++++++------------------------ > drivers/gpu/drm/ttm/ttm_bo_util.c | 6 +- > drivers/gpu/drm/ttm/ttm_execbuf_util.c | 43 +++----- > include/drm/ttm/ttm_bo_api.h | 25 ++--- > include/drm/ttm/ttm_execbuf_util.h | 1 - > 7 files changed, 79 insertions(+), 193 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c > index e35d468..2b2077d 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_gem.c > +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c > @@ -368,7 +368,7 @@ retry: > ret = ttm_bo_reserve(&nvbo->bo, true, false, true, &op->ticket); > if (ret) { > validate_fini_no_ticket(op, NULL); > - if (unlikely(ret == -EAGAIN)) { > + if (unlikely(ret == -EDEADLK)) { > ret = ttm_bo_reserve_slowpath(&nvbo->bo, true, > &op->ticket); > if (!ret) > diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h > index b4fd89f..ee7ad79 100644 > --- a/drivers/gpu/drm/qxl/qxl_object.h > +++ b/drivers/gpu/drm/qxl/qxl_object.h > @@ -57,11 +57,6 @@ static inline unsigned long qxl_bo_size(struct qxl_bo *bo) > return bo->tbo.num_pages << PAGE_SHIFT; > } > > -static inline bool qxl_bo_is_reserved(struct qxl_bo *bo) > -{ > - return !!atomic_read(&bo->tbo.reserved); > -} > - > static inline u64 qxl_bo_mmap_offset(struct qxl_bo *bo) > { > return bo->tbo.addr_space_offset; > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c > index b912375..5f9fe80 100644 > --- a/drivers/gpu/drm/ttm/ttm_bo.c > +++ b/drivers/gpu/drm/ttm/ttm_bo.c > @@ -150,6 +150,9 @@ static void ttm_bo_release_list(struct kref *list_kref) > if (bo->ttm) > ttm_tt_destroy(bo->ttm); > atomic_dec(&bo->glob->bo_count); > + if (bo->resv == &bo->ttm_resv) > + reservation_object_fini(&bo->ttm_resv); > + > if (bo->destroy) > bo->destroy(bo); > else { > @@ -158,18 +161,6 @@ static void ttm_bo_release_list(struct kref *list_kref) > ttm_mem_global_free(bdev->glob->mem_glob, acc_size); > } > > -static int ttm_bo_wait_unreserved(struct ttm_buffer_object *bo, > - bool interruptible) > -{ > - if (interruptible) { > - return wait_event_interruptible(bo->event_queue, > - !ttm_bo_is_reserved(bo)); > - } else { > - wait_event(bo->event_queue, !ttm_bo_is_reserved(bo)); > - return 0; > - } > -} > - > void ttm_bo_add_to_lru(struct ttm_buffer_object *bo) > { > struct ttm_bo_device *bdev = bo->bdev; > @@ -218,65 +209,27 @@ int ttm_bo_reserve_nolru(struct ttm_buffer_object *bo, > bool no_wait, bool use_ticket, > struct ww_acquire_ctx *ticket) > { > - int ret; > + int ret = 0; > > - while (unlikely(atomic_xchg(&bo->reserved, 1) != 0)) { > - /** > - * Deadlock avoidance for multi-bo reserving. > - */ > - if (use_ticket && bo->seq_valid) { > - /** > - * We've already reserved this one. > - */ > - if (unlikely(ticket->stamp == bo->val_seq)) > - return -EDEADLK; > - /** > - * Already reserved by a thread that will not back > - * off for us. We need to back off. > - */ > - if (unlikely(ticket->stamp - bo->val_seq <= LONG_MAX)) > - return -EAGAIN; > - } > + if (no_wait) { > + bool success; > > - if (no_wait) > + /* not valid any more, fix your locking! */ > + if (WARN_ON(ticket)) > return -EBUSY; > > - ret = ttm_bo_wait_unreserved(bo, interruptible); > - > - if (unlikely(ret)) > - return ret; > - } > - > - if (use_ticket) { > - bool wake_up = false; > - > - /** > - * Wake up waiters that may need to recheck for deadlock, > - * if we decreased the sequence number. > - */ > - if (unlikely((bo->val_seq - ticket->stamp <= LONG_MAX) > - || !bo->seq_valid)) > - wake_up = true; > - > - /* > - * In the worst case with memory ordering these values can be > - * seen in the wrong order. However since we call wake_up_all > - * in that case, this will hopefully not pose a problem, > - * and the worst case would only cause someone to accidentally > - * hit -EAGAIN in ttm_bo_reserve when they see old value of > - * val_seq. However this would only happen if seq_valid was > - * written before val_seq was, and just means some slightly > - * increased cpu usage > - */ > - bo->val_seq = ticket->stamp; > - bo->seq_valid = true; > - if (wake_up) > - wake_up_all(&bo->event_queue); > - } else { > - bo->seq_valid = false; > + success = ww_mutex_trylock(&bo->resv->lock); > + return success ? 0 : -EBUSY; > } > > - return 0; > + if (interruptible) > + ret = ww_mutex_lock_interruptible(&bo->resv->lock, > + ticket); > + else > + ret = ww_mutex_lock(&bo->resv->lock, ticket); > + if (ret == -EINTR) > + return -ERESTARTSYS; > + return ret; > } > EXPORT_SYMBOL(ttm_bo_reserve); > > @@ -313,50 +266,27 @@ int ttm_bo_reserve(struct ttm_buffer_object *bo, > return ret; > } > > -int ttm_bo_reserve_slowpath_nolru(struct ttm_buffer_object *bo, > - bool interruptible, > - struct ww_acquire_ctx *ticket) > -{ > - bool wake_up = false; > - int ret; > - > - while (unlikely(atomic_xchg(&bo->reserved, 1) != 0)) { > - WARN_ON(bo->seq_valid && ticket->stamp == bo->val_seq); > - > - ret = ttm_bo_wait_unreserved(bo, interruptible); > - > - if (unlikely(ret)) > - return ret; > - } > - > - if (bo->val_seq - ticket->stamp < LONG_MAX || !bo->seq_valid) > - wake_up = true; > - > - /** > - * Wake up waiters that may need to recheck for deadlock, > - * if we decreased the sequence number. > - */ > - bo->val_seq = ticket->stamp; > - bo->seq_valid = true; > - if (wake_up) > - wake_up_all(&bo->event_queue); > - > - return 0; > -} > - > int ttm_bo_reserve_slowpath(struct ttm_buffer_object *bo, > bool interruptible, struct ww_acquire_ctx *ticket) > { > struct ttm_bo_global *glob = bo->glob; > - int put_count, ret; > + int put_count = 0; > + int ret = 0; > + > + if (interruptible) > + ret = ww_mutex_lock_slow_interruptible(&bo->resv->lock, > + ticket); > + else > + ww_mutex_lock_slow(&bo->resv->lock, ticket); > > - ret = ttm_bo_reserve_slowpath_nolru(bo, interruptible, ticket); > - if (likely(!ret)) { > + if (likely(ret == 0)) { > spin_lock(&glob->lru_lock); > put_count = ttm_bo_del_from_lru(bo); > spin_unlock(&glob->lru_lock); > ttm_bo_list_ref_sub(bo, put_count, true); > - } > + } else if (ret == -EINTR) > + ret = -ERESTARTSYS; > + > return ret; > } > EXPORT_SYMBOL(ttm_bo_reserve_slowpath); > @@ -364,8 +294,7 @@ EXPORT_SYMBOL(ttm_bo_reserve_slowpath); > void ttm_bo_unreserve_ticket_locked(struct ttm_buffer_object *bo, struct ww_acquire_ctx *ticket) > { > ttm_bo_add_to_lru(bo); > - atomic_set(&bo->reserved, 0); > - wake_up_all(&bo->event_queue); > + ww_mutex_unlock(&bo->resv->lock); > } > > void ttm_bo_unreserve(struct ttm_buffer_object *bo) > @@ -558,17 +487,7 @@ static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo) > } > ttm_bo_mem_put(bo, &bo->mem); > > - atomic_set(&bo->reserved, 0); > - wake_up_all(&bo->event_queue); > - > - /* > - * Since the final reference to this bo may not be dropped by > - * the current task we have to put a memory barrier here to make > - * sure the changes done in this function are always visible. > - * > - * This function only needs protection against the final kref_put. > - */ > - smp_mb__before_atomic_dec(); > + ww_mutex_unlock (&bo->resv->lock); > } > > static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo) > @@ -600,10 +519,8 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo) > sync_obj = driver->sync_obj_ref(bo->sync_obj); > spin_unlock(&bdev->fence_lock); > > - if (!ret) { > - atomic_set(&bo->reserved, 0); > - wake_up_all(&bo->event_queue); > - } > + if (!ret) > + ww_mutex_unlock(&bo->resv->lock); > > kref_get(&bo->list_kref); > list_add_tail(&bo->ddestroy, &bdev->ddestroy); > @@ -653,8 +570,7 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo, > sync_obj = driver->sync_obj_ref(bo->sync_obj); > spin_unlock(&bdev->fence_lock); > > - atomic_set(&bo->reserved, 0); > - wake_up_all(&bo->event_queue); > + ww_mutex_unlock(&bo->resv->lock); > spin_unlock(&glob->lru_lock); > > ret = driver->sync_obj_wait(sync_obj, false, interruptible); > @@ -692,8 +608,7 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo, > spin_unlock(&bdev->fence_lock); > > if (ret || unlikely(list_empty(&bo->ddestroy))) { > - atomic_set(&bo->reserved, 0); > - wake_up_all(&bo->event_queue); > + ww_mutex_unlock(&bo->resv->lock); > spin_unlock(&glob->lru_lock); > return ret; > } > @@ -1253,6 +1168,7 @@ int ttm_bo_init(struct ttm_bo_device *bdev, > int ret = 0; > unsigned long num_pages; > struct ttm_mem_global *mem_glob = bdev->glob->mem_glob; > + bool locked; > > ret = ttm_mem_global_alloc(mem_glob, acc_size, false, false); > if (ret) { > @@ -1279,8 +1195,6 @@ int ttm_bo_init(struct ttm_bo_device *bdev, > kref_init(&bo->kref); > kref_init(&bo->list_kref); > atomic_set(&bo->cpu_writers, 0); > - atomic_set(&bo->reserved, 1); > - init_waitqueue_head(&bo->event_queue); > INIT_LIST_HEAD(&bo->lru); > INIT_LIST_HEAD(&bo->ddestroy); > INIT_LIST_HEAD(&bo->swap); > @@ -1298,37 +1212,34 @@ int ttm_bo_init(struct ttm_bo_device *bdev, > bo->mem.bus.io_reserved_count = 0; > bo->priv_flags = 0; > bo->mem.placement = (TTM_PL_FLAG_SYSTEM | TTM_PL_FLAG_CACHED); > - bo->seq_valid = false; > bo->persistent_swap_storage = persistent_swap_storage; > bo->acc_size = acc_size; > bo->sg = sg; > + bo->resv = &bo->ttm_resv; > + reservation_object_init(bo->resv); > atomic_inc(&bo->glob->bo_count); > > ret = ttm_bo_check_placement(bo, placement); > - if (unlikely(ret != 0)) > - goto out_err; > > /* > * For ttm_bo_type_device buffers, allocate > * address space from the device. > */ > - if (bo->type == ttm_bo_type_device || > - bo->type == ttm_bo_type_sg) { > + if (likely(!ret) && > + (bo->type == ttm_bo_type_device || > + bo->type == ttm_bo_type_sg)) > ret = ttm_bo_setup_vm(bo); > - if (ret) > - goto out_err; > - } > > - ret = ttm_bo_validate(bo, placement, interruptible, false); > - if (ret) > - goto out_err; > + locked = ww_mutex_trylock(&bo->resv->lock); > + WARN_ON(!locked); > > - ttm_bo_unreserve(bo); > - return 0; > + if (likely(!ret)) > + ret = ttm_bo_validate(bo, placement, interruptible, false); > > -out_err: > ttm_bo_unreserve(bo); > - ttm_bo_unref(&bo); > + > + if (unlikely(ret)) > + ttm_bo_unref(&bo); > > return ret; > } > @@ -1941,8 +1852,7 @@ out: > * already swapped buffer. > */ > > - atomic_set(&bo->reserved, 0); > - wake_up_all(&bo->event_queue); > + ww_mutex_unlock(&bo->resv->lock); > kref_put(&bo->list_kref, ttm_bo_release_list); > return ret; > } > diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c > index af89458..319cf41 100644 > --- a/drivers/gpu/drm/ttm/ttm_bo_util.c > +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c > @@ -433,6 +433,7 @@ static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo, > struct ttm_buffer_object *fbo; > struct ttm_bo_device *bdev = bo->bdev; > struct ttm_bo_driver *driver = bdev->driver; > + int ret; > > fbo = kmalloc(sizeof(*fbo), GFP_KERNEL); > if (!fbo) > @@ -445,7 +446,6 @@ static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo, > * TODO: Explicit member copy would probably be better here. > */ > > - init_waitqueue_head(&fbo->event_queue); > INIT_LIST_HEAD(&fbo->ddestroy); > INIT_LIST_HEAD(&fbo->lru); > INIT_LIST_HEAD(&fbo->swap); > @@ -463,6 +463,10 @@ static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo, > kref_init(&fbo->kref); > fbo->destroy = &ttm_transfered_destroy; > fbo->acc_size = 0; > + fbo->resv = &fbo->ttm_resv; > + reservation_object_init(fbo->resv); > + ret = ww_mutex_trylock(&fbo->resv->lock); > + WARN_ON(!ret); > > *new_obj = fbo; > return 0; > diff --git a/drivers/gpu/drm/ttm/ttm_execbuf_util.c b/drivers/gpu/drm/ttm/ttm_execbuf_util.c > index efcb734..7392da5 100644 > --- a/drivers/gpu/drm/ttm/ttm_execbuf_util.c > +++ b/drivers/gpu/drm/ttm/ttm_execbuf_util.c > @@ -48,8 +48,7 @@ static void ttm_eu_backoff_reservation_locked(struct list_head *list, > entry->removed = false; > > } else { > - atomic_set(&bo->reserved, 0); > - wake_up_all(&bo->event_queue); > + ww_mutex_unlock(&bo->resv->lock); > } > } > } > @@ -134,8 +133,6 @@ int ttm_eu_reserve_buffers(struct ww_acquire_ctx *ticket, > glob = entry->bo->glob; > > ww_acquire_init(ticket, &reservation_ww_class); > - spin_lock(&glob->lru_lock); > - > retry: > list_for_each_entry(entry, list, head) { > struct ttm_buffer_object *bo = entry->bo; > @@ -144,42 +141,34 @@ retry: > if (entry->reserved) > continue; > > - ret = ttm_bo_reserve_nolru(bo, true, true, true, ticket); > - switch (ret) { > - case 0: > - break; > - case -EBUSY: > - ttm_eu_del_from_lru_locked(list); > - spin_unlock(&glob->lru_lock); > - ret = ttm_bo_reserve_nolru(bo, true, false, > - true, ticket); > - spin_lock(&glob->lru_lock); > - > - if (!ret) > - break; > > - if (unlikely(ret != -EAGAIN)) > - goto err; > + ret = ttm_bo_reserve_nolru(bo, true, false, true, ticket); > > - /* fallthrough */ > - case -EAGAIN: > + if (ret == -EDEADLK) { > + /* uh oh, we lost out, drop every reservation and try > + * to only reserve this buffer, then start over if > + * this succeeds. > + */ > + spin_lock(&glob->lru_lock); > ttm_eu_backoff_reservation_locked(list, ticket); > spin_unlock(&glob->lru_lock); > ttm_eu_list_ref_sub(list); > - ret = ttm_bo_reserve_slowpath_nolru(bo, true, ticket); > - if (unlikely(ret != 0)) > + ret = ww_mutex_lock_slow_interruptible(&bo->resv->lock, > + ticket); > + if (unlikely(ret != 0)) { > + if (ret == -EINTR) > + ret = -ERESTARTSYS; > goto err_fini; > + } > > - spin_lock(&glob->lru_lock); > entry->reserved = true; > if (unlikely(atomic_read(&bo->cpu_writers) > 0)) { > ret = -EBUSY; > goto err; > } > goto retry; > - default: > + } else if (ret) > goto err; > - } > > entry->reserved = true; > if (unlikely(atomic_read(&bo->cpu_writers) > 0)) { > @@ -189,12 +178,14 @@ retry: > } > > ww_acquire_done(ticket); > + spin_lock(&glob->lru_lock); > ttm_eu_del_from_lru_locked(list); > spin_unlock(&glob->lru_lock); > ttm_eu_list_ref_sub(list); > return 0; > > err: > + spin_lock(&glob->lru_lock); > ttm_eu_backoff_reservation_locked(list, ticket); > spin_unlock(&glob->lru_lock); > ttm_eu_list_ref_sub(list); > diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h > index 0a992b0..31ad860 100644 > --- a/include/drm/ttm/ttm_bo_api.h > +++ b/include/drm/ttm/ttm_bo_api.h > @@ -39,6 +39,7 @@ > #include <linux/mm.h> > #include <linux/rbtree.h> > #include <linux/bitmap.h> > +#include <linux/reservation.h> > > struct ttm_bo_device; > > @@ -153,7 +154,6 @@ struct ttm_tt; > * Lru lists may keep one refcount, the delayed delete list, and kref != 0 > * keeps one refcount. When this refcount reaches zero, > * the object is destroyed. > - * @event_queue: Queue for processes waiting on buffer object status change. > * @mem: structure describing current placement. > * @persistent_swap_storage: Usually the swap storage is deleted for buffers > * pinned in physical memory. If this behaviour is not desired, this member > @@ -164,12 +164,6 @@ struct ttm_tt; > * @lru: List head for the lru list. > * @ddestroy: List head for the delayed destroy list. > * @swap: List head for swap LRU list. > - * @val_seq: Sequence of the validation holding the @reserved lock. > - * Used to avoid starvation when many processes compete to validate the > - * buffer. This member is protected by the bo_device::lru_lock. > - * @seq_valid: The value of @val_seq is valid. This value is protected by > - * the bo_device::lru_lock. > - * @reserved: Deadlock-free lock used for synchronization state transitions. > * @sync_obj: Pointer to a synchronization object. > * @priv_flags: Flags describing buffer object internal state. > * @vm_rb: Rb node for the vm rb tree. > @@ -209,10 +203,9 @@ struct ttm_buffer_object { > > struct kref kref; > struct kref list_kref; > - wait_queue_head_t event_queue; > > /** > - * Members protected by the bo::reserved lock. > + * Members protected by the bo::resv::reserved lock. > */ > > struct ttm_mem_reg mem; > @@ -234,15 +227,6 @@ struct ttm_buffer_object { > struct list_head ddestroy; > struct list_head swap; > struct list_head io_reserve_lru; > - unsigned long val_seq; > - bool seq_valid; > - > - /** > - * Members protected by the bdev::lru_lock > - * only when written to. > - */ > - > - atomic_t reserved; > > /** > * Members protected by struct buffer_object_device::fence_lock > @@ -272,6 +256,9 @@ struct ttm_buffer_object { > uint32_t cur_placement; > > struct sg_table *sg; > + > + struct reservation_object *resv; > + struct reservation_object ttm_resv; > }; > > /** > @@ -736,7 +723,7 @@ extern void ttm_bo_swapout_all(struct ttm_bo_device *bdev); > */ > static inline bool ttm_bo_is_reserved(struct ttm_buffer_object *bo) > { > - return atomic_read(&bo->reserved); > + return ww_mutex_is_locked(&bo->resv->lock); > } > > #endif > diff --git a/include/drm/ttm/ttm_execbuf_util.h b/include/drm/ttm/ttm_execbuf_util.h > index ba71ef9..ec8a1d3 100644 > --- a/include/drm/ttm/ttm_execbuf_util.h > +++ b/include/drm/ttm/ttm_execbuf_util.h > @@ -33,7 +33,6 @@ > > #include <ttm/ttm_bo_api.h> > #include <linux/list.h> > -#include <linux/reservation.h> > > /** > * struct ttm_validate_buffer > -- > 1.8.3.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c index e35d468..2b2077d 100644 --- a/drivers/gpu/drm/nouveau/nouveau_gem.c +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c @@ -368,7 +368,7 @@ retry: ret = ttm_bo_reserve(&nvbo->bo, true, false, true, &op->ticket); if (ret) { validate_fini_no_ticket(op, NULL); - if (unlikely(ret == -EAGAIN)) { + if (unlikely(ret == -EDEADLK)) { ret = ttm_bo_reserve_slowpath(&nvbo->bo, true, &op->ticket); if (!ret) diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h index b4fd89f..ee7ad79 100644 --- a/drivers/gpu/drm/qxl/qxl_object.h +++ b/drivers/gpu/drm/qxl/qxl_object.h @@ -57,11 +57,6 @@ static inline unsigned long qxl_bo_size(struct qxl_bo *bo) return bo->tbo.num_pages << PAGE_SHIFT; } -static inline bool qxl_bo_is_reserved(struct qxl_bo *bo) -{ - return !!atomic_read(&bo->tbo.reserved); -} - static inline u64 qxl_bo_mmap_offset(struct qxl_bo *bo) { return bo->tbo.addr_space_offset; diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index b912375..5f9fe80 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -150,6 +150,9 @@ static void ttm_bo_release_list(struct kref *list_kref) if (bo->ttm) ttm_tt_destroy(bo->ttm); atomic_dec(&bo->glob->bo_count); + if (bo->resv == &bo->ttm_resv) + reservation_object_fini(&bo->ttm_resv); + if (bo->destroy) bo->destroy(bo); else { @@ -158,18 +161,6 @@ static void ttm_bo_release_list(struct kref *list_kref) ttm_mem_global_free(bdev->glob->mem_glob, acc_size); } -static int ttm_bo_wait_unreserved(struct ttm_buffer_object *bo, - bool interruptible) -{ - if (interruptible) { - return wait_event_interruptible(bo->event_queue, - !ttm_bo_is_reserved(bo)); - } else { - wait_event(bo->event_queue, !ttm_bo_is_reserved(bo)); - return 0; - } -} - void ttm_bo_add_to_lru(struct ttm_buffer_object *bo) { struct ttm_bo_device *bdev = bo->bdev; @@ -218,65 +209,27 @@ int ttm_bo_reserve_nolru(struct ttm_buffer_object *bo, bool no_wait, bool use_ticket, struct ww_acquire_ctx *ticket) { - int ret; + int ret = 0; - while (unlikely(atomic_xchg(&bo->reserved, 1) != 0)) { - /** - * Deadlock avoidance for multi-bo reserving. - */ - if (use_ticket && bo->seq_valid) { - /** - * We've already reserved this one. - */ - if (unlikely(ticket->stamp == bo->val_seq)) - return -EDEADLK; - /** - * Already reserved by a thread that will not back - * off for us. We need to back off. - */ - if (unlikely(ticket->stamp - bo->val_seq <= LONG_MAX)) - return -EAGAIN; - } + if (no_wait) { + bool success; - if (no_wait) + /* not valid any more, fix your locking! */ + if (WARN_ON(ticket)) return -EBUSY; - ret = ttm_bo_wait_unreserved(bo, interruptible); - - if (unlikely(ret)) - return ret; - } - - if (use_ticket) { - bool wake_up = false; - - /** - * Wake up waiters that may need to recheck for deadlock, - * if we decreased the sequence number. - */ - if (unlikely((bo->val_seq - ticket->stamp <= LONG_MAX) - || !bo->seq_valid)) - wake_up = true; - - /* - * In the worst case with memory ordering these values can be - * seen in the wrong order. However since we call wake_up_all - * in that case, this will hopefully not pose a problem, - * and the worst case would only cause someone to accidentally - * hit -EAGAIN in ttm_bo_reserve when they see old value of - * val_seq. However this would only happen if seq_valid was - * written before val_seq was, and just means some slightly - * increased cpu usage - */ - bo->val_seq = ticket->stamp; - bo->seq_valid = true; - if (wake_up) - wake_up_all(&bo->event_queue); - } else { - bo->seq_valid = false; + success = ww_mutex_trylock(&bo->resv->lock); + return success ? 0 : -EBUSY; } - return 0; + if (interruptible) + ret = ww_mutex_lock_interruptible(&bo->resv->lock, + ticket); + else + ret = ww_mutex_lock(&bo->resv->lock, ticket); + if (ret == -EINTR) + return -ERESTARTSYS; + return ret; } EXPORT_SYMBOL(ttm_bo_reserve); @@ -313,50 +266,27 @@ int ttm_bo_reserve(struct ttm_buffer_object *bo, return ret; } -int ttm_bo_reserve_slowpath_nolru(struct ttm_buffer_object *bo, - bool interruptible, - struct ww_acquire_ctx *ticket) -{ - bool wake_up = false; - int ret; - - while (unlikely(atomic_xchg(&bo->reserved, 1) != 0)) { - WARN_ON(bo->seq_valid && ticket->stamp == bo->val_seq); - - ret = ttm_bo_wait_unreserved(bo, interruptible); - - if (unlikely(ret)) - return ret; - } - - if (bo->val_seq - ticket->stamp < LONG_MAX || !bo->seq_valid) - wake_up = true; - - /** - * Wake up waiters that may need to recheck for deadlock, - * if we decreased the sequence number. - */ - bo->val_seq = ticket->stamp; - bo->seq_valid = true; - if (wake_up) - wake_up_all(&bo->event_queue); - - return 0; -} - int ttm_bo_reserve_slowpath(struct ttm_buffer_object *bo, bool interruptible, struct ww_acquire_ctx *ticket) { struct ttm_bo_global *glob = bo->glob; - int put_count, ret; + int put_count = 0; + int ret = 0; + + if (interruptible) + ret = ww_mutex_lock_slow_interruptible(&bo->resv->lock, + ticket); + else + ww_mutex_lock_slow(&bo->resv->lock, ticket); - ret = ttm_bo_reserve_slowpath_nolru(bo, interruptible, ticket); - if (likely(!ret)) { + if (likely(ret == 0)) { spin_lock(&glob->lru_lock); put_count = ttm_bo_del_from_lru(bo); spin_unlock(&glob->lru_lock); ttm_bo_list_ref_sub(bo, put_count, true); - } + } else if (ret == -EINTR) + ret = -ERESTARTSYS; + return ret; } EXPORT_SYMBOL(ttm_bo_reserve_slowpath); @@ -364,8 +294,7 @@ EXPORT_SYMBOL(ttm_bo_reserve_slowpath); void ttm_bo_unreserve_ticket_locked(struct ttm_buffer_object *bo, struct ww_acquire_ctx *ticket) { ttm_bo_add_to_lru(bo); - atomic_set(&bo->reserved, 0); - wake_up_all(&bo->event_queue); + ww_mutex_unlock(&bo->resv->lock); } void ttm_bo_unreserve(struct ttm_buffer_object *bo) @@ -558,17 +487,7 @@ static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo) } ttm_bo_mem_put(bo, &bo->mem); - atomic_set(&bo->reserved, 0); - wake_up_all(&bo->event_queue); - - /* - * Since the final reference to this bo may not be dropped by - * the current task we have to put a memory barrier here to make - * sure the changes done in this function are always visible. - * - * This function only needs protection against the final kref_put. - */ - smp_mb__before_atomic_dec(); + ww_mutex_unlock (&bo->resv->lock); } static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo) @@ -600,10 +519,8 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo) sync_obj = driver->sync_obj_ref(bo->sync_obj); spin_unlock(&bdev->fence_lock); - if (!ret) { - atomic_set(&bo->reserved, 0); - wake_up_all(&bo->event_queue); - } + if (!ret) + ww_mutex_unlock(&bo->resv->lock); kref_get(&bo->list_kref); list_add_tail(&bo->ddestroy, &bdev->ddestroy); @@ -653,8 +570,7 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo, sync_obj = driver->sync_obj_ref(bo->sync_obj); spin_unlock(&bdev->fence_lock); - atomic_set(&bo->reserved, 0); - wake_up_all(&bo->event_queue); + ww_mutex_unlock(&bo->resv->lock); spin_unlock(&glob->lru_lock); ret = driver->sync_obj_wait(sync_obj, false, interruptible); @@ -692,8 +608,7 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo, spin_unlock(&bdev->fence_lock); if (ret || unlikely(list_empty(&bo->ddestroy))) { - atomic_set(&bo->reserved, 0); - wake_up_all(&bo->event_queue); + ww_mutex_unlock(&bo->resv->lock); spin_unlock(&glob->lru_lock); return ret; } @@ -1253,6 +1168,7 @@ int ttm_bo_init(struct ttm_bo_device *bdev, int ret = 0; unsigned long num_pages; struct ttm_mem_global *mem_glob = bdev->glob->mem_glob; + bool locked; ret = ttm_mem_global_alloc(mem_glob, acc_size, false, false); if (ret) { @@ -1279,8 +1195,6 @@ int ttm_bo_init(struct ttm_bo_device *bdev, kref_init(&bo->kref); kref_init(&bo->list_kref); atomic_set(&bo->cpu_writers, 0); - atomic_set(&bo->reserved, 1); - init_waitqueue_head(&bo->event_queue); INIT_LIST_HEAD(&bo->lru); INIT_LIST_HEAD(&bo->ddestroy); INIT_LIST_HEAD(&bo->swap); @@ -1298,37 +1212,34 @@ int ttm_bo_init(struct ttm_bo_device *bdev, bo->mem.bus.io_reserved_count = 0; bo->priv_flags = 0; bo->mem.placement = (TTM_PL_FLAG_SYSTEM | TTM_PL_FLAG_CACHED); - bo->seq_valid = false; bo->persistent_swap_storage = persistent_swap_storage; bo->acc_size = acc_size; bo->sg = sg; + bo->resv = &bo->ttm_resv; + reservation_object_init(bo->resv); atomic_inc(&bo->glob->bo_count); ret = ttm_bo_check_placement(bo, placement); - if (unlikely(ret != 0)) - goto out_err; /* * For ttm_bo_type_device buffers, allocate * address space from the device. */ - if (bo->type == ttm_bo_type_device || - bo->type == ttm_bo_type_sg) { + if (likely(!ret) && + (bo->type == ttm_bo_type_device || + bo->type == ttm_bo_type_sg)) ret = ttm_bo_setup_vm(bo); - if (ret) - goto out_err; - } - ret = ttm_bo_validate(bo, placement, interruptible, false); - if (ret) - goto out_err; + locked = ww_mutex_trylock(&bo->resv->lock); + WARN_ON(!locked); - ttm_bo_unreserve(bo); - return 0; + if (likely(!ret)) + ret = ttm_bo_validate(bo, placement, interruptible, false); -out_err: ttm_bo_unreserve(bo); - ttm_bo_unref(&bo); + + if (unlikely(ret)) + ttm_bo_unref(&bo); return ret; } @@ -1941,8 +1852,7 @@ out: * already swapped buffer. */ - atomic_set(&bo->reserved, 0); - wake_up_all(&bo->event_queue); + ww_mutex_unlock(&bo->resv->lock); kref_put(&bo->list_kref, ttm_bo_release_list); return ret; } diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c index af89458..319cf41 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_util.c +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c @@ -433,6 +433,7 @@ static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo, struct ttm_buffer_object *fbo; struct ttm_bo_device *bdev = bo->bdev; struct ttm_bo_driver *driver = bdev->driver; + int ret; fbo = kmalloc(sizeof(*fbo), GFP_KERNEL); if (!fbo) @@ -445,7 +446,6 @@ static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo, * TODO: Explicit member copy would probably be better here. */ - init_waitqueue_head(&fbo->event_queue); INIT_LIST_HEAD(&fbo->ddestroy); INIT_LIST_HEAD(&fbo->lru); INIT_LIST_HEAD(&fbo->swap); @@ -463,6 +463,10 @@ static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo, kref_init(&fbo->kref); fbo->destroy = &ttm_transfered_destroy; fbo->acc_size = 0; + fbo->resv = &fbo->ttm_resv; + reservation_object_init(fbo->resv); + ret = ww_mutex_trylock(&fbo->resv->lock); + WARN_ON(!ret); *new_obj = fbo; return 0; diff --git a/drivers/gpu/drm/ttm/ttm_execbuf_util.c b/drivers/gpu/drm/ttm/ttm_execbuf_util.c index efcb734..7392da5 100644 --- a/drivers/gpu/drm/ttm/ttm_execbuf_util.c +++ b/drivers/gpu/drm/ttm/ttm_execbuf_util.c @@ -48,8 +48,7 @@ static void ttm_eu_backoff_reservation_locked(struct list_head *list, entry->removed = false; } else { - atomic_set(&bo->reserved, 0); - wake_up_all(&bo->event_queue); + ww_mutex_unlock(&bo->resv->lock); } } } @@ -134,8 +133,6 @@ int ttm_eu_reserve_buffers(struct ww_acquire_ctx *ticket, glob = entry->bo->glob; ww_acquire_init(ticket, &reservation_ww_class); - spin_lock(&glob->lru_lock); - retry: list_for_each_entry(entry, list, head) { struct ttm_buffer_object *bo = entry->bo; @@ -144,42 +141,34 @@ retry: if (entry->reserved) continue; - ret = ttm_bo_reserve_nolru(bo, true, true, true, ticket); - switch (ret) { - case 0: - break; - case -EBUSY: - ttm_eu_del_from_lru_locked(list); - spin_unlock(&glob->lru_lock); - ret = ttm_bo_reserve_nolru(bo, true, false, - true, ticket); - spin_lock(&glob->lru_lock); - - if (!ret) - break; - if (unlikely(ret != -EAGAIN)) - goto err; + ret = ttm_bo_reserve_nolru(bo, true, false, true, ticket); - /* fallthrough */ - case -EAGAIN: + if (ret == -EDEADLK) { + /* uh oh, we lost out, drop every reservation and try + * to only reserve this buffer, then start over if + * this succeeds. + */ + spin_lock(&glob->lru_lock); ttm_eu_backoff_reservation_locked(list, ticket); spin_unlock(&glob->lru_lock); ttm_eu_list_ref_sub(list); - ret = ttm_bo_reserve_slowpath_nolru(bo, true, ticket); - if (unlikely(ret != 0)) + ret = ww_mutex_lock_slow_interruptible(&bo->resv->lock, + ticket); + if (unlikely(ret != 0)) { + if (ret == -EINTR) + ret = -ERESTARTSYS; goto err_fini; + } - spin_lock(&glob->lru_lock); entry->reserved = true; if (unlikely(atomic_read(&bo->cpu_writers) > 0)) { ret = -EBUSY; goto err; } goto retry; - default: + } else if (ret) goto err; - } entry->reserved = true; if (unlikely(atomic_read(&bo->cpu_writers) > 0)) { @@ -189,12 +178,14 @@ retry: } ww_acquire_done(ticket); + spin_lock(&glob->lru_lock); ttm_eu_del_from_lru_locked(list); spin_unlock(&glob->lru_lock); ttm_eu_list_ref_sub(list); return 0; err: + spin_lock(&glob->lru_lock); ttm_eu_backoff_reservation_locked(list, ticket); spin_unlock(&glob->lru_lock); ttm_eu_list_ref_sub(list); diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h index 0a992b0..31ad860 100644 --- a/include/drm/ttm/ttm_bo_api.h +++ b/include/drm/ttm/ttm_bo_api.h @@ -39,6 +39,7 @@ #include <linux/mm.h> #include <linux/rbtree.h> #include <linux/bitmap.h> +#include <linux/reservation.h> struct ttm_bo_device; @@ -153,7 +154,6 @@ struct ttm_tt; * Lru lists may keep one refcount, the delayed delete list, and kref != 0 * keeps one refcount. When this refcount reaches zero, * the object is destroyed. - * @event_queue: Queue for processes waiting on buffer object status change. * @mem: structure describing current placement. * @persistent_swap_storage: Usually the swap storage is deleted for buffers * pinned in physical memory. If this behaviour is not desired, this member @@ -164,12 +164,6 @@ struct ttm_tt; * @lru: List head for the lru list. * @ddestroy: List head for the delayed destroy list. * @swap: List head for swap LRU list. - * @val_seq: Sequence of the validation holding the @reserved lock. - * Used to avoid starvation when many processes compete to validate the - * buffer. This member is protected by the bo_device::lru_lock. - * @seq_valid: The value of @val_seq is valid. This value is protected by - * the bo_device::lru_lock. - * @reserved: Deadlock-free lock used for synchronization state transitions. * @sync_obj: Pointer to a synchronization object. * @priv_flags: Flags describing buffer object internal state. * @vm_rb: Rb node for the vm rb tree. @@ -209,10 +203,9 @@ struct ttm_buffer_object { struct kref kref; struct kref list_kref; - wait_queue_head_t event_queue; /** - * Members protected by the bo::reserved lock. + * Members protected by the bo::resv::reserved lock. */ struct ttm_mem_reg mem; @@ -234,15 +227,6 @@ struct ttm_buffer_object { struct list_head ddestroy; struct list_head swap; struct list_head io_reserve_lru; - unsigned long val_seq; - bool seq_valid; - - /** - * Members protected by the bdev::lru_lock - * only when written to. - */ - - atomic_t reserved; /** * Members protected by struct buffer_object_device::fence_lock @@ -272,6 +256,9 @@ struct ttm_buffer_object { uint32_t cur_placement; struct sg_table *sg; + + struct reservation_object *resv; + struct reservation_object ttm_resv; }; /** @@ -736,7 +723,7 @@ extern void ttm_bo_swapout_all(struct ttm_bo_device *bdev); */ static inline bool ttm_bo_is_reserved(struct ttm_buffer_object *bo) { - return atomic_read(&bo->reserved); + return ww_mutex_is_locked(&bo->resv->lock); } #endif diff --git a/include/drm/ttm/ttm_execbuf_util.h b/include/drm/ttm/ttm_execbuf_util.h index ba71ef9..ec8a1d3 100644 --- a/include/drm/ttm/ttm_execbuf_util.h +++ b/include/drm/ttm/ttm_execbuf_util.h @@ -33,7 +33,6 @@ #include <ttm/ttm_bo_api.h> #include <linux/list.h> -#include <linux/reservation.h> /** * struct ttm_validate_buffer
Now that the code is compatible in semantics, flip the switch. Use ww_mutex instead of the homegrown implementation. ww_mutex uses -EDEADLK to signal that the caller has to back off, and -EALREADY to indicate this buffer is already held by the caller. ttm used -EAGAIN and -EDEADLK for those, respectively. So some changes were needed to handle this correctly. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> --- drivers/gpu/drm/nouveau/nouveau_gem.c | 2 +- drivers/gpu/drm/qxl/qxl_object.h | 5 - drivers/gpu/drm/ttm/ttm_bo.c | 190 +++++++++------------------------ drivers/gpu/drm/ttm/ttm_bo_util.c | 6 +- drivers/gpu/drm/ttm/ttm_execbuf_util.c | 43 +++----- include/drm/ttm/ttm_bo_api.h | 25 ++--- include/drm/ttm/ttm_execbuf_util.h | 1 - 7 files changed, 79 insertions(+), 193 deletions(-)