| Message ID | MWHPR1201MB01278A0A2AF027374729D006FD540@MWHPR1201MB0127.namprd12.prod.outlook.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Good catch and yeah that is actually the real problem why my original patch didn't worked as expected. Going to fix this in v2, Christian. Am 10.11.2017 um 10:55 schrieb He, Roger: > static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo, > bool interruptible, bool no_wait_gpu, > bool unlock_resv) > { > ...... > ttm_bo_del_from_lru(bo); > list_del_init(&bo->ddestroy); > kref_put(&bo->list_kref, ttm_bo_ref_bug); > > spin_unlock(&glob->lru_lock); > ttm_bo_cleanup_memtype_use(bo); > > if (unlock_resv) //[Roger]: not sure we should add condition here as well. If not, for per-vm-bo, eviction would unlock the VM root BO resv obj which is not we want I think. > reservation_object_unlock(bo->resv); > > return 0; > } > > > Thanks > Roger(Hongbo.He) > > -----Original Message----- > From: Christian König [mailto:ckoenig.leichtzumerken@gmail.com] > Sent: Thursday, November 09, 2017 4:59 PM > To: dri-devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; He, Roger <Hongbo.He@amd.com>; michel@daenzer.net > Subject: [PATCH 6/7] drm/ttm: make unlocking in ttm_bo_cleanup_refs optional > > Needed for the next patch. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/ttm/ttm_bo.c | 52 ++++++++++++++++++++++++-------------------- > 1 file changed, 28 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 6f5d18366e6e..50a678b504f3 100644 > --- a/drivers/gpu/drm/ttm/ttm_bo.c > +++ b/drivers/gpu/drm/ttm/ttm_bo.c > @@ -486,20 +486,21 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo) } > > /** > - * function ttm_bo_cleanup_refs_and_unlock > + * function ttm_bo_cleanup_refs > * If bo idle, remove from delayed- and lru lists, and unref. > * If not idle, do nothing. > * > * Must be called with lru_lock and reservation held, this function > - * will drop both before returning. > + * will drop the lru lock and optionally the reservation lock before returning. > * > * @interruptible Any sleeps should occur interruptibly. > * @no_wait_gpu Never wait for gpu. Return -EBUSY instead. > + * @unlock_resv Unlock the reservation lock as well. > */ > > -static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo, > - bool interruptible, > - bool no_wait_gpu) > +static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo, > + bool interruptible, bool no_wait_gpu, > + bool unlock_resv) > { > struct ttm_bo_global *glob = bo->glob; > struct reservation_object *resv; > @@ -518,7 +519,8 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo, > if (ret && !no_wait_gpu) { > long lret; > > - reservation_object_unlock(bo->resv); > + if (unlock_resv) > + reservation_object_unlock(bo->resv); > spin_unlock(&glob->lru_lock); > > lret = reservation_object_wait_timeout_rcu(resv, true, @@ -531,19 +533,22 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo, > return -EBUSY; > > spin_lock(&glob->lru_lock); > - ret = reservation_object_trylock(bo->resv) ? 0 : -EBUSY; > - > - /* > - * We raced, and lost, someone else holds the reservation now, > - * and is probably busy in ttm_bo_cleanup_memtype_use. > - * > - * Even if it's not the case, because we finished waiting any > - * delayed destruction would succeed, so just return success > - * here. > - */ > - if (ret) { > - spin_unlock(&glob->lru_lock); > - return 0; > + if (unlock_resv) { > + ret = reservation_object_trylock(bo->resv) ? 0 : -EBUSY; > + /* > + * We raced, and lost, someone else holds the reservation now, > + * and is probably busy in ttm_bo_cleanup_memtype_use. > + * > + * Even if it's not the case, because we finished waiting any > + * delayed destruction would succeed, so just return success > + * here. > + */ > + if (ret) { > + spin_unlock(&glob->lru_lock); > + return 0; > + } > + } else { > + ret = 0; > } > } > > @@ -600,8 +605,8 @@ static int ttm_bo_delayed_delete(struct ttm_bo_device *bdev, bool remove_all) > } > > if (!ret) > - ret = ttm_bo_cleanup_refs_and_unlock(entry, false, > - !remove_all); > + ret = ttm_bo_cleanup_refs(entry, false, !remove_all, > + true); > else > spin_unlock(&glob->lru_lock); > > @@ -770,8 +775,7 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev, > kref_get(&bo->list_kref); > > if (!list_empty(&bo->ddestroy)) { > - ret = ttm_bo_cleanup_refs_and_unlock(bo, interruptible, > - no_wait_gpu); > + ret = ttm_bo_cleanup_refs(bo, interruptible, no_wait_gpu, true); > kref_put(&bo->list_kref, ttm_bo_release_list); > return ret; > } > @@ -1735,7 +1739,7 @@ static int ttm_bo_swapout(struct ttm_mem_shrink *shrink) > kref_get(&bo->list_kref); > > if (!list_empty(&bo->ddestroy)) { > - ret = ttm_bo_cleanup_refs_and_unlock(bo, false, false); > + ret = ttm_bo_cleanup_refs(bo, false, false, true); > kref_put(&bo->list_kref, ttm_bo_release_list); > return ret; > } > -- > 2.11.0 >
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 6f5d18366e6e..50a678b504f3 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -486,20 +486,21 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo) } /** - * function ttm_bo_cleanup_refs_and_unlock + * function ttm_bo_cleanup_refs * If bo idle, remove from delayed- and lru lists, and unref. * If not idle, do nothing. * * Must be called with lru_lock and reservation held, this function - * will drop both before returning. + * will drop the lru lock and optionally the reservation lock before returning. * * @interruptible Any sleeps should occur interruptibly. * @no_wait_gpu Never wait for gpu. Return -EBUSY instead. + * @unlock_resv Unlock the reservation lock as well. */ -static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo, - bool interruptible, - bool no_wait_gpu) +static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo, + bool interruptible, bool no_wait_gpu, + bool unlock_resv) { struct ttm_bo_global *glob = bo->glob; struct reservation_object *resv; @@ -518,7 +519,8 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo, bool interruptible, bool no_wait_gpu, bool unlock_resv) { ...... ttm_bo_del_from_lru(bo); list_del_init(&bo->ddestroy); kref_put(&bo->list_kref, ttm_bo_ref_bug); spin_unlock(&glob->lru_lock); ttm_bo_cleanup_memtype_use(bo); if (unlock_resv) //[Roger]: not sure we should add condition here as well. If not, for per-vm-bo, eviction would unlock the VM root BO resv obj which is not we want I think. reservation_object_unlock(bo->resv); return 0; } Thanks Roger(Hongbo.He) -----Original Message----- From: Christian König [mailto:ckoenig.leichtzumerken@gmail.com] Sent: Thursday, November 09, 2017 4:59 PM To: dri-devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; He, Roger <Hongbo.He@amd.com>; michel@daenzer.net Subject: [PATCH 6/7] drm/ttm: make unlocking in ttm_bo_cleanup_refs optional Needed for the next patch. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/ttm/ttm_bo.c | 52 ++++++++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 24 deletions(-) if (ret && !no_wait_gpu) { long lret; - reservation_object_unlock(bo->resv); + if (unlock_resv) + reservation_object_unlock(bo->resv); spin_unlock(&glob->lru_lock); lret = reservation_object_wait_timeout_rcu(resv, true, @@ -531,19 +533,22 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo, return -EBUSY; spin_lock(&glob->lru_lock); - ret = reservation_object_trylock(bo->resv) ? 0 : -EBUSY; - - /* - * We raced, and lost, someone else holds the reservation now, - * and is probably busy in ttm_bo_cleanup_memtype_use. - * - * Even if it's not the case, because we finished waiting any - * delayed destruction would succeed, so just return success - * here. - */ - if (ret) { - spin_unlock(&glob->lru_lock); - return 0; + if (unlock_resv) { + ret = reservation_object_trylock(bo->resv) ? 0 : -EBUSY; + /* + * We raced, and lost, someone else holds the reservation now, + * and is probably busy in ttm_bo_cleanup_memtype_use. + * + * Even if it's not the case, because we finished waiting any + * delayed destruction would succeed, so just return success + * here. + */ + if (ret) { + spin_unlock(&glob->lru_lock); + return 0; + } + } else { + ret = 0; } } @@ -600,8 +605,8 @@ static int ttm_bo_delayed_delete(struct ttm_bo_device *bdev, bool remove_all) } if (!ret) - ret = ttm_bo_cleanup_refs_and_unlock(entry, false, - !remove_all); + ret = ttm_bo_cleanup_refs(entry, false, !remove_all, + true); else spin_unlock(&glob->lru_lock); @@ -770,8 +775,7 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev, kref_get(&bo->list_kref); if (!list_empty(&bo->ddestroy)) { - ret = ttm_bo_cleanup_refs_and_unlock(bo, interruptible, - no_wait_gpu); + ret = ttm_bo_cleanup_refs(bo, interruptible, no_wait_gpu, true); kref_put(&bo->list_kref, ttm_bo_release_list); return ret; } @@ -1735,7 +1739,7 @@ static int ttm_bo_swapout(struct ttm_mem_shrink *shrink) kref_get(&bo->list_kref); if (!list_empty(&bo->ddestroy)) { - ret = ttm_bo_cleanup_refs_and_unlock(bo, false, false); + ret = ttm_bo_cleanup_refs(bo, false, false, true); kref_put(&bo->list_kref, ttm_bo_release_list); return ret; } -- 2.11.0