| Message ID | 20171108145936.27071-2-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/11/17 03:59 PM, Christian König wrote: > Instead of having a pointless wrapper or call the underlying ww_mutex > function directly. Not sure I can agree it's pointless, since it recently took me a while to realize that unlocking bo->resv is essentially the same as unreserving the BO. Anyway, this breaks the qxl driver: drivers/gpu/drm//qxl/qxl_release.c: In function ‘qxl_release_fence_buffer_objects’: drivers/gpu/drm//qxl/qxl_release.c:472:3: error: implicit declaration of function ‘__ttm_bo_unreserve’; did you mean ‘ttm_bo_unreserve’? [-Werror=implicit-function-declaration] __ttm_bo_unreserve(bo); ^~~~~~~~~~~~~~~~~~ ttm_bo_unreserve
Am 08.11.2017 um 17:36 schrieb Michel Dänzer: > On 08/11/17 03:59 PM, Christian König wrote: >> Instead of having a pointless wrapper or call the underlying ww_mutex >> function directly. > Not sure I can agree it's pointless, since it recently took me a while > to realize that unlocking bo->resv is essentially the same as > unreserving the BO. Ok in this case let's call this confusing. But yeah that __ttm_bo_unreserv(bo), reservation_object_unlock(bo->resv) and ww_mutex_unlock(&bo->resv->lock) are essentially the same thing is exactly what I want to fix here. > Anyway, this breaks the qxl driver: > > drivers/gpu/drm//qxl/qxl_release.c: In function ‘qxl_release_fence_buffer_objects’: > drivers/gpu/drm//qxl/qxl_release.c:472:3: error: implicit declaration of function ‘__ttm_bo_unreserve’; did you mean ‘ttm_bo_unreserve’? [-Werror=implicit-function-declaration] > __ttm_bo_unreserve(bo); > ^~~~~~~~~~~~~~~~~~ > ttm_bo_unreserve Thanks for pointing this out, going to respin. Christian.
On 08/11/17 05:41 PM, Christian König wrote: > Am 08.11.2017 um 17:36 schrieb Michel Dänzer: >> On 08/11/17 03:59 PM, Christian König wrote: >>> Instead of having a pointless wrapper or call the underlying ww_mutex >>> function directly. >> Not sure I can agree it's pointless, since it recently took me a while >> to realize that unlocking bo->resv is essentially the same as >> unreserving the BO. > > Ok in this case let's call this confusing. But yeah that > __ttm_bo_unreserv(bo), reservation_object_unlock(bo->resv) and > ww_mutex_unlock(&bo->resv->lock) are essentially the same thing is > exactly what I want to fix here. How about always using __ttm_bo_unreserve instead? The first piglit run with this series applied hit the BUG_ON in ttm_bo_add_to_lru, see the attached dmesg excerpt. The machine hung, couldn't reboot cleanly. Haven't been able to reproduce it in three more attempts though, so I'm not sure it's caused by this series, but I don't remember seeing it before. P.S. I just noticed I haven't had CONFIG_PROVE_LOCKING enabled in my kernels for a while, I'll enable it again. Any other options that should be enabled for testing?
Am 08.11.2017 um 18:37 schrieb Michel Dänzer: > On 08/11/17 05:41 PM, Christian König wrote: >> Am 08.11.2017 um 17:36 schrieb Michel Dänzer: >>> On 08/11/17 03:59 PM, Christian König wrote: >>>> Instead of having a pointless wrapper or call the underlying ww_mutex >>>> function directly. >>> Not sure I can agree it's pointless, since it recently took me a while >>> to realize that unlocking bo->resv is essentially the same as >>> unreserving the BO. >> Ok in this case let's call this confusing. But yeah that >> __ttm_bo_unreserv(bo), reservation_object_unlock(bo->resv) and >> ww_mutex_unlock(&bo->resv->lock) are essentially the same thing is >> exactly what I want to fix here. > How about always using __ttm_bo_unreserve instead? I actually want to get rid of __ttm_bo_reserve as well. Cause what is easier to understand: __ttm_bo_reserve(bo, false, false, NULL) vs. reservation_object_lock(bo->resv); __ttm_bo_reserve(bo, false, true, NULL) vs. reservation_object_trylock(bo->resv); __ttm_bo_reserve(bo, true, false, NULL) vs. reservation_object_lock_interruptible(bo->resv); > The first piglit run with this series applied hit the BUG_ON in > ttm_bo_add_to_lru, see the attached dmesg excerpt. The machine hung, > couldn't reboot cleanly. Haven't been able to reproduce it in three more > attempts though, so I'm not sure it's caused by this series, but I don't > remember seeing it before. It's most likely caused by this change, I will take another look tomorrow. > P.S. I just noticed I haven't had CONFIG_PROVE_LOCKING enabled in my > kernels for a while, I'll enable it again. Any other options that should > be enabled for testing? kmemleak is quite nice to have available as well, but doesn't need to run automatically all the time. Christian.
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 9905cf41cba6..6f55310a9d09 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -471,7 +471,7 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo) ttm_bo_add_to_lru(bo); } - __ttm_bo_unreserve(bo); + reservation_object_unlock(bo->resv); } if (bo->resv != &bo->ttm_resv) reservation_object_unlock(&bo->ttm_resv); @@ -517,7 +517,8 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo, if (ret && !no_wait_gpu) { long lret; - ww_mutex_unlock(&bo->resv->lock); + + reservation_object_unlock(bo->resv); spin_unlock(&glob->lru_lock); lret = reservation_object_wait_timeout_rcu(resv, true, @@ -547,7 +548,7 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo, } if (ret || unlikely(list_empty(&bo->ddestroy))) { - __ttm_bo_unreserve(bo); + reservation_object_unlock(bo->resv); spin_unlock(&glob->lru_lock); return ret; } @@ -749,7 +750,7 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev, if (place && !bdev->driver->eviction_valuable(bo, place)) { - __ttm_bo_unreserve(bo); + reservation_object_unlock(bo->resv); ret = -EBUSY; continue; } @@ -1788,7 +1789,7 @@ static int ttm_bo_swapout(struct ttm_mem_shrink *shrink) * already swapped buffer. */ - __ttm_bo_unreserve(bo); + reservation_object_unlock(bo->resv); kref_put(&bo->list_kref, ttm_bo_release_list); return ret; } @@ -1825,7 +1826,7 @@ int ttm_bo_wait_unreserved(struct ttm_buffer_object *bo) ret = __ttm_bo_reserve(bo, true, false, NULL); if (unlikely(ret != 0)) goto out_unlock; - __ttm_bo_unreserve(bo); + reservation_object_unlock(bo->resv); out_unlock: mutex_unlock(&bo->wu_mutex); diff --git a/drivers/gpu/drm/ttm/ttm_execbuf_util.c b/drivers/gpu/drm/ttm/ttm_execbuf_util.c index 5e1bcabffef5..373ced0b2fc2 100644 --- a/drivers/gpu/drm/ttm/ttm_execbuf_util.c +++ b/drivers/gpu/drm/ttm/ttm_execbuf_util.c @@ -38,7 +38,7 @@ static void ttm_eu_backoff_reservation_reverse(struct list_head *list, list_for_each_entry_continue_reverse(entry, list, head) { struct ttm_buffer_object *bo = entry->bo; - __ttm_bo_unreserve(bo); + reservation_object_unlock(bo->resv); } } @@ -69,7 +69,7 @@ void ttm_eu_backoff_reservation(struct ww_acquire_ctx *ticket, struct ttm_buffer_object *bo = entry->bo; ttm_bo_add_to_lru(bo); - __ttm_bo_unreserve(bo); + reservation_object_unlock(bo->resv); } spin_unlock(&glob->lru_lock); @@ -112,7 +112,7 @@ int ttm_eu_reserve_buffers(struct ww_acquire_ctx *ticket, ret = __ttm_bo_reserve(bo, intr, (ticket == NULL), ticket); if (!ret && unlikely(atomic_read(&bo->cpu_writers) > 0)) { - __ttm_bo_unreserve(bo); + reservation_object_unlock(bo->resv); ret = -EBUSY; @@ -203,7 +203,7 @@ void ttm_eu_fence_buffer_objects(struct ww_acquire_ctx *ticket, else reservation_object_add_excl_fence(bo->resv, fence); ttm_bo_add_to_lru(bo); - __ttm_bo_unreserve(bo); + reservation_object_unlock(bo->resv); } spin_unlock(&glob->lru_lock); if (ticket) diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h index 5f821a9b3a1f..389359a0006b 100644 --- a/include/drm/ttm/ttm_bo_driver.h +++ b/include/drm/ttm/ttm_bo_driver.h @@ -941,18 +941,6 @@ static inline int ttm_bo_reserve_slowpath(struct ttm_buffer_object *bo, } /** - * __ttm_bo_unreserve - * @bo: A pointer to a struct ttm_buffer_object. - * - * Unreserve a previous reservation of @bo where the buffer object is - * already on lru lists. - */ -static inline void __ttm_bo_unreserve(struct ttm_buffer_object *bo) -{ - ww_mutex_unlock(&bo->resv->lock); -} - -/** * ttm_bo_unreserve * * @bo: A pointer to a struct ttm_buffer_object. @@ -966,7 +954,7 @@ static inline void ttm_bo_unreserve(struct ttm_buffer_object *bo) ttm_bo_add_to_lru(bo); spin_unlock(&bo->glob->lru_lock); } - __ttm_bo_unreserve(bo); + reservation_object_unlock(bo->resv); } /**
Instead of having a pointless wrapper or call the underlying ww_mutex function directly. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/ttm/ttm_bo.c | 13 +++++++------ drivers/gpu/drm/ttm/ttm_execbuf_util.c | 8 ++++---- include/drm/ttm/ttm_bo_driver.h | 14 +------------- 3 files changed, 12 insertions(+), 23 deletions(-)