| Message ID | MWHPR1201MB0127CE75F5FD6DE84A0B93C6FDCC0@MWHPR1201MB0127.namprd12.prod.outlook.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 23.02.2018 um 10:46 schrieb He, Roger: > > -----Original Message----- > From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of Christian K?nig > Sent: Tuesday, February 20, 2018 8:58 PM > To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org > Subject: [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and swapout. > > This solves the problem that when we swapout a BO from a domain we sometimes couldn't make room for it because holding the lock blocks all other BOs with this reservation object. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/ttm/ttm_bo.c | 33 ++++++++++++++++----------------- > 1 file changed, 16 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index d90b1cf10b27..3a44c2ee4155 100644 > --- a/drivers/gpu/drm/ttm/ttm_bo.c > +++ b/drivers/gpu/drm/ttm/ttm_bo.c > @@ -713,31 +713,30 @@ bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo, EXPORT_SYMBOL(ttm_bo_eviction_valuable); > > /** > - * Check the target bo is allowable to be evicted or swapout, including cases: > - * > - * a. if share same reservation object with ctx->resv, have assumption > - * reservation objects should already be locked, so not lock again and > - * return true directly when either the opreation allow_reserved_eviction > - * or the target bo already is in delayed free list; > - * > - * b. Otherwise, trylock it. > + * Check if the target bo is allowed to be evicted or swapedout. > */ > static bool ttm_bo_evict_swapout_allowable(struct ttm_buffer_object *bo, > - struct ttm_operation_ctx *ctx, bool *locked) > + struct ttm_operation_ctx *ctx, > + bool *locked) > { > - bool ret = false; > + /* First check if we can lock it */ > + *locked = reservation_object_trylock(bo->resv); > + if (*locked) > + return true; > > - *locked = false; > + /* Check if it's locked because it is part of the current operation */ > if (bo->resv == ctx->resv) { > reservation_object_assert_held(bo->resv); > - if (ctx->allow_reserved_eviction || !list_empty(&bo->ddestroy)) > - ret = true; > - } else { > - *locked = reservation_object_trylock(bo->resv); > - ret = *locked; > + return ctx->allow_reserved_eviction || > + !list_empty(&bo->ddestroy); > } > > - return ret; > + /* Check if it's locked because it was already evicted */ > + if (ww_mutex_is_owned_by(&bo->resv->lock, NULL)) > + return true; > > For the special case: when command submission with Per-VM-BO enabled, > All BOs a/b/c are always valid BO. After the validation of BOs a and b, > when validation of BO c, is it possible to return true and then evict BO a and b by mistake ? > Because a/b/c share same task_struct. No, that's why I check the context as well. BOs explicitly reserved have a non NULL context while BOs trylocked for swapout have a NULL context. Christian. > > Thanks > Roger(Hongbo.He) > > + /* Some other thread is using it, don't touch it */ > + return false; > } > > static int ttm_mem_evict_first(struct ttm_bo_device *bdev, > -- > 2.14.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
-----Original Message----- From: Christian König [mailto:ckoenig.leichtzumerken@gmail.com] Sent: Friday, February 23, 2018 8:06 PM To: He, Roger <Hongbo.He@amd.com>; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and swapout. Am 23.02.2018 um 10:46 schrieb He, Roger: > > -----Original Message----- > From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On > Behalf Of Christian K?nig > Sent: Tuesday, February 20, 2018 8:58 PM > To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; > linux-kernel@vger.kernel.org > Subject: [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and swapout. > > This solves the problem that when we swapout a BO from a domain we sometimes couldn't make room for it because holding the lock blocks all other BOs with this reservation object. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/ttm/ttm_bo.c | 33 ++++++++++++++++----------------- > 1 file changed, 16 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c > b/drivers/gpu/drm/ttm/ttm_bo.c index d90b1cf10b27..3a44c2ee4155 100644 > --- a/drivers/gpu/drm/ttm/ttm_bo.c > +++ b/drivers/gpu/drm/ttm/ttm_bo.c > @@ -713,31 +713,30 @@ bool ttm_bo_eviction_valuable(struct > ttm_buffer_object *bo, EXPORT_SYMBOL(ttm_bo_eviction_valuable); > > /** > - * Check the target bo is allowable to be evicted or swapout, including cases: > - * > - * a. if share same reservation object with ctx->resv, have > assumption > - * reservation objects should already be locked, so not lock again > and > - * return true directly when either the opreation > allow_reserved_eviction > - * or the target bo already is in delayed free list; > - * > - * b. Otherwise, trylock it. > + * Check if the target bo is allowed to be evicted or swapedout. > */ > static bool ttm_bo_evict_swapout_allowable(struct ttm_buffer_object *bo, > - struct ttm_operation_ctx *ctx, bool *locked) > + struct ttm_operation_ctx *ctx, > + bool *locked) > { > - bool ret = false; > + /* First check if we can lock it */ > + *locked = reservation_object_trylock(bo->resv); > + if (*locked) > + return true; > > - *locked = false; > + /* Check if it's locked because it is part of the current operation > +*/ > if (bo->resv == ctx->resv) { > reservation_object_assert_held(bo->resv); > - if (ctx->allow_reserved_eviction || !list_empty(&bo->ddestroy)) > - ret = true; > - } else { > - *locked = reservation_object_trylock(bo->resv); > - ret = *locked; > + return ctx->allow_reserved_eviction || > + !list_empty(&bo->ddestroy); > } > > - return ret; > + /* Check if it's locked because it was already evicted */ > + if (ww_mutex_is_owned_by(&bo->resv->lock, NULL)) > + return true; > > For the special case: when command submission with Per-VM-BO enabled, > All BOs a/b/c are always valid BO. After the validation of BOs a and > b, when validation of BO c, is it possible to return true and then evict BO a and b by mistake ? > Because a/b/c share same task_struct. No, that's why I check the context as well. BOs explicitly reserved have a non NULL context while BOs trylocked for swapout have a NULL context. When BOs have a non NULL context only when command submission and reserved by ttm_eu_re6serve_buffers . But for Per-VM-BO a/b/c they always are not in BO list, so they will be not reserved and have always NULL context. So above case also can happen. Anything missing here? Thanks Roger(Hongbo.He) > > + /* Some other thread is using it, don't touch it */ > + return false; > } > > static int ttm_mem_evict_first(struct ttm_bo_device *bdev, > -- > 2.14.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
I missed the Per-VM-BO share the reservation object with root bo. So context is not NULL here. So, this patch is: Reviewed-by: Roger He <Hongbo.He@amd.com> Thanks Roger(Hongbo.He) -----Original Message----- From: Christian König [mailto:ckoenig.leichtzumerken@gmail.com] Sent: Friday, February 23, 2018 8:06 PM To: He, Roger <Hongbo.He@amd.com>; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and swapout. Am 23.02.2018 um 10:46 schrieb He, Roger: > > -----Original Message----- > From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On > Behalf Of Christian K?nig > Sent: Tuesday, February 20, 2018 8:58 PM > To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; > linux-kernel@vger.kernel.org > Subject: [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and swapout. > > This solves the problem that when we swapout a BO from a domain we sometimes couldn't make room for it because holding the lock blocks all other BOs with this reservation object. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/ttm/ttm_bo.c | 33 ++++++++++++++++----------------- > 1 file changed, 16 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c > b/drivers/gpu/drm/ttm/ttm_bo.c index d90b1cf10b27..3a44c2ee4155 100644 > --- a/drivers/gpu/drm/ttm/ttm_bo.c > +++ b/drivers/gpu/drm/ttm/ttm_bo.c > @@ -713,31 +713,30 @@ bool ttm_bo_eviction_valuable(struct > ttm_buffer_object *bo, EXPORT_SYMBOL(ttm_bo_eviction_valuable); > > /** > - * Check the target bo is allowable to be evicted or swapout, including cases: > - * > - * a. if share same reservation object with ctx->resv, have > assumption > - * reservation objects should already be locked, so not lock again > and > - * return true directly when either the opreation > allow_reserved_eviction > - * or the target bo already is in delayed free list; > - * > - * b. Otherwise, trylock it. > + * Check if the target bo is allowed to be evicted or swapedout. > */ > static bool ttm_bo_evict_swapout_allowable(struct ttm_buffer_object *bo, > - struct ttm_operation_ctx *ctx, bool *locked) > + struct ttm_operation_ctx *ctx, > + bool *locked) > { > - bool ret = false; > + /* First check if we can lock it */ > + *locked = reservation_object_trylock(bo->resv); > + if (*locked) > + return true; > > - *locked = false; > + /* Check if it's locked because it is part of the current operation > +*/ > if (bo->resv == ctx->resv) { > reservation_object_assert_held(bo->resv); > - if (ctx->allow_reserved_eviction || !list_empty(&bo->ddestroy)) > - ret = true; > - } else { > - *locked = reservation_object_trylock(bo->resv); > - ret = *locked; > + return ctx->allow_reserved_eviction || > + !list_empty(&bo->ddestroy); > } > > - return ret; > + /* Check if it's locked because it was already evicted */ > + if (ww_mutex_is_owned_by(&bo->resv->lock, NULL)) > + return true; > > For the special case: when command submission with Per-VM-BO enabled, > All BOs a/b/c are always valid BO. After the validation of BOs a and > b, when validation of BO c, is it possible to return true and then evict BO a and b by mistake ? > Because a/b/c share same task_struct. No, that's why I check the context as well. BOs explicitly reserved have a non NULL context while BOs trylocked for swapout have a NULL context. BOs have a non NULL context only when command submission and reserved by ttm_eu_re6serve_buffers . But for Per-VM-BO a/b/c they always are not in BO list, so they will be not reserved and have always NULL context. So above case also can happen. Anything missing here? > > + /* Some other thread is using it, don't touch it */ > + return false; > } > > static int ttm_mem_evict_first(struct ttm_bo_device *bdev, > -- > 2.14.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index d90b1cf10b27..3a44c2ee4155 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -713,31 +713,30 @@ bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo, EXPORT_SYMBOL(ttm_bo_eviction_valuable); /** - * Check the target bo is allowable to be evicted or swapout, including cases: - * - * a. if share same reservation object with ctx->resv, have assumption - * reservation objects should already be locked, so not lock again and - * return true directly when either the opreation allow_reserved_eviction - * or the target bo already is in delayed free list; - * - * b. Otherwise, trylock it. + * Check if the target bo is allowed to be evicted or swapedout. */ static bool ttm_bo_evict_swapout_allowable(struct ttm_buffer_object *bo, - struct ttm_operation_ctx *ctx, bool *locked) + struct ttm_operation_ctx *ctx, + bool *locked) { - bool ret = false; + /* First check if we can lock it */ + *locked = reservation_object_trylock(bo->resv); + if (*locked) + return true; - *locked = false; + /* Check if it's locked because it is part of the current operation */ if (bo->resv == ctx->resv) { reservation_object_assert_held(bo->resv); - if (ctx->allow_reserved_eviction || !list_empty(&bo->ddestroy)) - ret = true; - } else { - *locked = reservation_object_trylock(bo->resv); - ret = *locked; + return ctx->allow_reserved_eviction || + !list_empty(&bo->ddestroy); } - return ret; + /* Check if it's locked because it was already evicted */ + if (ww_mutex_is_owned_by(&bo->resv->lock, NULL)) + return true; For the special case: when command submission with Per-VM-BO enabled, All BOs a/b/c are always valid BO. After the validation of BOs a and b, when validation of BO c, is it possible to return true and then evict BO a and b by mistake ? Because a/b/c share same task_struct. Thanks