| Message ID | 20171109085909.1653-4-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/11/17 09:59 AM, Christian König wrote: > Consistently use the reservation object wrappers instead of accessing > the ww_mutex directly. > > Additional to that use the reservation object wrappers directly instead of > calling __ttm_bo_reserve with fixed parameters. > > Signed-off-by: Christian König <christian.koenig@amd.com> [...] > @@ -1823,7 +1823,9 @@ int ttm_bo_wait_unreserved(struct ttm_buffer_object *bo) > return -ERESTARTSYS; > if (!ww_mutex_is_locked(&bo->resv->lock)) > goto out_unlock; > - ret = __ttm_bo_reserve(bo, true, false, NULL); > + ret = reservation_object_lock_interruptible(bo->resv, NULL); > + if (ret = -EINTR) > + ret = -ERESTARTSYS; Typo in the test, must be if (ret == -EINTR) This bug caused the Xorg process to hang for me when trying to run glxgears, requiring a hard reboot. Did you accidentally send an untested version of this patch?
Am 09.11.2017 um 17:50 schrieb Michel Dänzer: > On 09/11/17 09:59 AM, Christian König wrote: >> Consistently use the reservation object wrappers instead of accessing >> the ww_mutex directly. >> >> Additional to that use the reservation object wrappers directly instead of >> calling __ttm_bo_reserve with fixed parameters. >> >> Signed-off-by: Christian König <christian.koenig@amd.com> > [...] > >> @@ -1823,7 +1823,9 @@ int ttm_bo_wait_unreserved(struct ttm_buffer_object *bo) >> return -ERESTARTSYS; >> if (!ww_mutex_is_locked(&bo->resv->lock)) >> goto out_unlock; >> - ret = __ttm_bo_reserve(bo, true, false, NULL); >> + ret = reservation_object_lock_interruptible(bo->resv, NULL); >> + if (ret = -EINTR) >> + ret = -ERESTARTSYS; > Typo in the test, must be > > if (ret == -EINTR) > > > This bug caused the Xorg process to hang for me when trying to run > glxgears, requiring a hard reboot. Did you accidentally send an untested > version of this patch? Yeah, just stumbled over this as well. I accidentally merged the fix for this into a later patch which I didn't send out yet. Consider it fixed, Christian.
On 09/11/17 06:13 PM, Christian König wrote: > Am 09.11.2017 um 17:50 schrieb Michel Dänzer: >> On 09/11/17 09:59 AM, Christian König wrote: >>> Consistently use the reservation object wrappers instead of accessing >>> the ww_mutex directly. >>> >>> Additional to that use the reservation object wrappers directly >>> instead of >>> calling __ttm_bo_reserve with fixed parameters. >>> >>> Signed-off-by: Christian König <christian.koenig@amd.com> >> [...] >> >>> @@ -1823,7 +1823,9 @@ int ttm_bo_wait_unreserved(struct >>> ttm_buffer_object *bo) >>> return -ERESTARTSYS; >>> if (!ww_mutex_is_locked(&bo->resv->lock)) >>> goto out_unlock; >>> - ret = __ttm_bo_reserve(bo, true, false, NULL); >>> + ret = reservation_object_lock_interruptible(bo->resv, NULL); >>> + if (ret = -EINTR) >>> + ret = -ERESTARTSYS; >> Typo in the test, must be >> >> if (ret == -EINTR) >> >> >> This bug caused the Xorg process to hang for me when trying to run >> glxgears, requiring a hard reboot. Did you accidentally send an untested >> version of this patch? > Yeah, just stumbled over this as well. I accidentally merged the fix for > this into a later patch which I didn't send out yet. > > Consider it fixed, Thanks. With that, patches 1-6 are Reviewed-and-Tested-by: Michel Dänzer <michel.daenzer@amd.com> I sent another comment on patch 7.
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 6f55310a9d09..6f5d18366e6e 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -446,7 +446,7 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo) } spin_lock(&glob->lru_lock); - ret = __ttm_bo_reserve(bo, false, true, NULL); + ret = reservation_object_trylock(bo->resv) ? 0 : -EBUSY; if (!ret) { if (reservation_object_test_signaled_rcu(&bo->ttm_resv, true)) { ttm_bo_del_from_lru(bo); @@ -531,7 +531,7 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo, return -EBUSY; spin_lock(&glob->lru_lock); - ret = __ttm_bo_reserve(bo, false, true, NULL); + ret = reservation_object_trylock(bo->resv) ? 0 : -EBUSY; /* * We raced, and lost, someone else holds the reservation now, @@ -592,10 +592,10 @@ static int ttm_bo_delayed_delete(struct ttm_bo_device *bdev, bool remove_all) kref_get(&nentry->list_kref); } - ret = __ttm_bo_reserve(entry, false, true, NULL); + ret = reservation_object_trylock(entry->resv) ? 0 : -EBUSY; if (remove_all && ret) { spin_unlock(&glob->lru_lock); - ret = __ttm_bo_reserve(entry, false, false, NULL); + ret = reservation_object_lock(entry->resv, NULL); spin_lock(&glob->lru_lock); } @@ -744,7 +744,7 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev, spin_lock(&glob->lru_lock); for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) { list_for_each_entry(bo, &man->lru[i], lru) { - ret = __ttm_bo_reserve(bo, false, true, NULL); + ret = reservation_object_trylock(bo->resv) ? 0 : -EBUSY; if (ret) continue; @@ -1719,7 +1719,7 @@ static int ttm_bo_swapout(struct ttm_mem_shrink *shrink) spin_lock(&glob->lru_lock); for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) { list_for_each_entry(bo, &glob->swap_lru[i], swap) { - ret = __ttm_bo_reserve(bo, false, true, NULL); + ret = reservation_object_trylock(bo->resv) ? 0 : -EBUSY; if (!ret) break; } @@ -1823,7 +1823,9 @@ int ttm_bo_wait_unreserved(struct ttm_buffer_object *bo) return -ERESTARTSYS; if (!ww_mutex_is_locked(&bo->resv->lock)) goto out_unlock; - ret = __ttm_bo_reserve(bo, true, false, NULL); + ret = reservation_object_lock_interruptible(bo->resv, NULL); + if (ret = -EINTR) + ret = -ERESTARTSYS; if (unlikely(ret != 0)) goto out_unlock; reservation_object_unlock(bo->resv); diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h index 389359a0006b..3659cf6150d2 100644 --- a/include/drm/ttm/ttm_bo_driver.h +++ b/include/drm/ttm/ttm_bo_driver.h @@ -836,14 +836,14 @@ static inline int __ttm_bo_reserve(struct ttm_buffer_object *bo, if (WARN_ON(ticket)) return -EBUSY; - success = ww_mutex_trylock(&bo->resv->lock); + success = reservation_object_trylock(bo->resv); return success ? 0 : -EBUSY; } if (interruptible) - ret = ww_mutex_lock_interruptible(&bo->resv->lock, ticket); + ret = reservation_object_lock_interruptible(bo->resv, ticket); else - ret = ww_mutex_lock(&bo->resv->lock, ticket); + ret = reservation_object_lock(bo->resv, ticket); if (ret == -EINTR) return -ERESTARTSYS; return ret;
Consistently use the reservation object wrappers instead of accessing the ww_mutex directly. Additional to that use the reservation object wrappers directly instead of calling __ttm_bo_reserve with fixed parameters. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/ttm/ttm_bo.c | 16 +++++++++------- include/drm/ttm/ttm_bo_driver.h | 6 +++--- 2 files changed, 12 insertions(+), 10 deletions(-)