From: "He, Roger" <Hongbo.He@amd.com> To: "He, Roger" <Hongbo.He@amd.com>, "Koenig, Christian" <Christian.Koenig@amd.com>, "amd-gfx@lists.freedesktop.org" <amd-gfx@lists.freedesktop.org>, "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org> Subject: RE: [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and swapout. Date: Sat, 24 Feb 2018 03:46:03 +0000 [thread overview] Message-ID: <MWHPR1201MB0127B30637C83130A8167C12FDC30@MWHPR1201MB0127.namprd12.prod.outlook.com> (raw) In-Reply-To: <MWHPR1201MB01278E573419210283D68640FDC30@MWHPR1201MB0127.namprd12.prod.outlook.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: "He, Roger" <Hongbo.He-5C7GfCeVMHo@public.gmane.org> To: "He, Roger" <Hongbo.He-5C7GfCeVMHo@public.gmane.org>, "Koenig, Christian" <Christian.Koenig-5C7GfCeVMHo@public.gmane.org>, "amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org" <amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>, "dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org" <dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>, "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> Subject: RE: [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and swapout. Date: Sat, 24 Feb 2018 03:46:03 +0000 [thread overview] Message-ID: <MWHPR1201MB0127B30637C83130A8167C12FDC30@MWHPR1201MB0127.namprd12.prod.outlook.com> (raw) In-Reply-To: <MWHPR1201MB01278E573419210283D68640FDC30-3iK1xFAIwjq9imrIu4W8xGrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org> 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 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
next prev parent reply other threads:[~2018-02-24 3:46 UTC|newest] Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-02-20 12:58 [PATCH 1/4] locking/ww_mutex: add ww_mutex_is_owned_by function v3 Christian König 2018-02-20 12:58 ` Christian König 2018-02-20 12:58 ` [PATCH 2/4] drm/amdgpu: use new ww_mutex_is_owned_by function Christian König 2018-02-20 12:58 ` Christian König 2018-02-23 9:48 ` He, Roger 2018-02-23 9:48 ` He, Roger 2018-02-20 12:58 ` [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and swapout Christian König 2018-02-20 12:58 ` Christian König 2018-02-23 9:46 ` He, Roger 2018-02-23 9:46 ` He, Roger 2018-02-23 12:05 ` Christian König 2018-02-23 12:05 ` Christian König 2018-02-24 3:36 ` He, Roger 2018-02-24 3:36 ` He, Roger 2018-02-24 3:46 ` He, Roger [this message] 2018-02-24 3:46 ` He, Roger 2018-02-20 12:58 ` [PATCH 4/4] drm/ttm: keep BOs reserved until end of eviction Christian König 2018-02-20 12:58 ` Christian König 2018-02-23 9:29 ` He, Roger 2018-02-23 9:29 ` He, Roger 2018-02-20 13:12 ` [PATCH 1/4] locking/ww_mutex: add ww_mutex_is_owned_by function v3 Peter Zijlstra 2018-02-20 13:12 ` Peter Zijlstra 2018-02-20 13:26 ` Christian König 2018-02-20 13:26 ` Christian König 2018-02-20 13:57 ` Peter Zijlstra 2018-02-20 13:57 ` Peter Zijlstra 2018-02-20 14:34 ` Christian König 2018-02-20 14:34 ` Christian König 2018-02-20 14:54 ` Peter Zijlstra 2018-02-20 14:54 ` Peter Zijlstra 2018-02-20 15:05 ` Christian König 2018-02-20 15:05 ` Christian König 2018-02-20 15:21 ` Peter Zijlstra 2018-02-20 15:21 ` Peter Zijlstra 2018-02-20 23:56 ` Daniel Vetter 2018-02-20 23:56 ` Daniel Vetter 2018-02-21 10:54 ` Maarten Lankhorst 2018-02-21 10:54 ` Maarten Lankhorst 2018-02-21 11:50 ` Christian König 2018-02-21 11:50 ` Christian König 2018-02-21 21:10 ` Emil Velikov 2018-02-21 21:10 ` Emil Velikov 2018-02-20 14:02 ` Daniel Vetter 2018-02-20 14:02 ` Daniel Vetter -- strict thread matches above, loose matches on Subject: below -- 2018-02-19 9:56 [PATCH 1/4] locking/ww_mutex: add ww_mutex_is_owned_by function Christian König 2018-02-19 9:56 ` [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and swapout Christian König 2018-02-19 9:56 ` Christian König
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=MWHPR1201MB0127B30637C83130A8167C12FDC30@MWHPR1201MB0127.namprd12.prod.outlook.com \ --to=hongbo.he@amd.com \ --cc=Christian.Koenig@amd.com \ --cc=amd-gfx@lists.freedesktop.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=linux-kernel@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.