From 5e338405119a80aa59e811626739122d1c15045d Mon Sep 17 00:00:00 2001 From: Maarten Lankhorst Date: Thu, 27 Jun 2013 13:48:19 +0200 Subject: [PATCH] drm/ttm: convert to the reservation api 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 Reviewed-by: Jerome Glisse Signed-off-by: Dave Airlie --- 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 e35d4688711c..2b2077d1e2f7 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 b4fd89fbd8b7..ee7ad79ce781 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 b912375b9c18..5f9fe8044afc 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 af894584dd90..319cf4127c5b 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 efcb734e5543..7392da557be2 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 0a992b016fe9..31ad860c10cd 100644 --- a/include/drm/ttm/ttm_bo_api.h +++ b/include/drm/ttm/ttm_bo_api.h @@ -39,6 +39,7 @@ #include #include #include +#include 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 ba71ef91f4d5..ec8a1d306510 100644 --- a/include/drm/ttm/ttm_execbuf_util.h +++ b/include/drm/ttm/ttm_execbuf_util.h @@ -33,7 +33,6 @@ #include #include -#include /** * struct ttm_validate_buffer -- 2.30.2