* [PATCH v2] drm/amdgpu: Sync KFD fence only for move/evict
@ 2017-10-20 17:18 Harish Kasiviswanathan
[not found] ` <1508519937-6081-1-git-send-email-Harish.Kasiviswanathan-5C7GfCeVMHo@public.gmane.org>
0 siblings, 1 reply; 4+ messages in thread
From: Harish Kasiviswanathan @ 2017-10-20 17:18 UTC (permalink / raw)
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW; +Cc: Harish Kasiviswanathan
A single KFD eviction fence is attached to all the BOs of a process
including BOs imported. This fence ensures that all BOs belonging to
that process stays resident when the process queues are active.
Don't add this eviction fence to any sync object unless it is a move or
evict job. These jobs are identified by the fence owner
AMDGPU_FENCE_OWNER_UNDEFINED
v2: Always sync to exclusive fence
Change-Id: I8752d1cf6b2a1c4f2a57292b7c2cd308d5b6f9b7
Signed-off-by: Harish Kasiviswanathan <Harish.Kasiviswanathan@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
index a4f0ecf..88e49f7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
@@ -199,10 +199,10 @@ int amdgpu_sync_resv(struct amdgpu_device *adev,
return -EINVAL;
f = reservation_object_get_excl(resv);
- fence_owner = amdgpu_sync_get_owner(f);
- if (fence_owner != AMDGPU_FENCE_OWNER_KFD ||
- owner != AMDGPU_FENCE_OWNER_VM)
+ if (f) {
r = amdgpu_sync_fence(adev, sync, f);
+ return r;
+ }
if (explicit_sync)
return r;
@@ -216,7 +216,7 @@ int amdgpu_sync_resv(struct amdgpu_device *adev,
reservation_object_held(resv));
fence_owner = amdgpu_sync_get_owner(f);
if (fence_owner == AMDGPU_FENCE_OWNER_KFD &&
- owner == AMDGPU_FENCE_OWNER_VM)
+ owner != AMDGPU_FENCE_OWNER_UNDEFINED)
continue;
if (amdgpu_sync_same_dev(adev, f)) {
--
1.9.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 4+ messages in thread[parent not found: <1508519937-6081-1-git-send-email-Harish.Kasiviswanathan-5C7GfCeVMHo@public.gmane.org>]
* Re: [PATCH v2] drm/amdgpu: Sync KFD fence only for move/evict [not found] ` <1508519937-6081-1-git-send-email-Harish.Kasiviswanathan-5C7GfCeVMHo@public.gmane.org> @ 2017-10-20 18:37 ` Christian König [not found] ` <a501c93e-0181-2ab1-2a85-6d2d177ae8a3-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 0 siblings, 1 reply; 4+ messages in thread From: Christian König @ 2017-10-20 18:37 UTC (permalink / raw) To: Harish Kasiviswanathan, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Am 20.10.2017 um 19:18 schrieb Harish Kasiviswanathan: > A single KFD eviction fence is attached to all the BOs of a process > including BOs imported. This fence ensures that all BOs belonging to > that process stays resident when the process queues are active. > > Don't add this eviction fence to any sync object unless it is a move or > evict job. These jobs are identified by the fence owner > AMDGPU_FENCE_OWNER_UNDEFINED > > v2: Always sync to exclusive fence > > Change-Id: I8752d1cf6b2a1c4f2a57292b7c2cd308d5b6f9b7 > Signed-off-by: Harish Kasiviswanathan <Harish.Kasiviswanathan@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c > index a4f0ecf..88e49f7 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c > @@ -199,10 +199,10 @@ int amdgpu_sync_resv(struct amdgpu_device *adev, > return -EINVAL; > > f = reservation_object_get_excl(resv); > - fence_owner = amdgpu_sync_get_owner(f); > - if (fence_owner != AMDGPU_FENCE_OWNER_KFD || > - owner != AMDGPU_FENCE_OWNER_VM) > + if (f) { > r = amdgpu_sync_fence(adev, sync, f); > + return r; > + } That is not correct either. It must look like this: > /* always sync to the exclusive fence */ > f = reservation_object_get_excl(resv); > r = amdgpu_sync_fence(adev, sync, f); > > flist = reservation_object_get_list(resv); > if (!flist || r) > return r Otherwise you ignore all shared fences and completely mess up the dependencies handling. I suggest to just revert the patch which originally changed the code away from what it looks like on amd-staging-drm-next. Regards, Christian. > > if (explicit_sync) > return r; > @@ -216,7 +216,7 @@ int amdgpu_sync_resv(struct amdgpu_device *adev, > reservation_object_held(resv)); > fence_owner = amdgpu_sync_get_owner(f); > if (fence_owner == AMDGPU_FENCE_OWNER_KFD && > - owner == AMDGPU_FENCE_OWNER_VM) > + owner != AMDGPU_FENCE_OWNER_UNDEFINED) > continue; > > if (amdgpu_sync_same_dev(adev, f)) { _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 4+ messages in thread
[parent not found: <a501c93e-0181-2ab1-2a85-6d2d177ae8a3-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* RE: [PATCH v2] drm/amdgpu: Sync KFD fence only for move/evict [not found] ` <a501c93e-0181-2ab1-2a85-6d2d177ae8a3-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2017-10-20 18:53 ` Kasiviswanathan, Harish [not found] ` <DM3PR1201MB10388630530FC1B8B34908758C430-BBcFnVpqZhWjUUTFdQAMQmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org> 0 siblings, 1 reply; 4+ messages in thread From: Kasiviswanathan, Harish @ 2017-10-20 18:53 UTC (permalink / raw) To: Koenig, Christian, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW -----Original Message----- From: Christian König [mailto:ckoenig.leichtzumerken@gmail.com] Sent: Friday, October 20, 2017 2:38 PM To: Kasiviswanathan, Harish <Harish.Kasiviswanathan@amd.com>; amd-gfx@lists.freedesktop.org Subject: Re: [PATCH v2] drm/amdgpu: Sync KFD fence only for move/evict Am 20.10.2017 um 19:18 schrieb Harish Kasiviswanathan: > A single KFD eviction fence is attached to all the BOs of a process > including BOs imported. This fence ensures that all BOs belonging to > that process stays resident when the process queues are active. > > Don't add this eviction fence to any sync object unless it is a move > or evict job. These jobs are identified by the fence owner > AMDGPU_FENCE_OWNER_UNDEFINED > > v2: Always sync to exclusive fence > > Change-Id: I8752d1cf6b2a1c4f2a57292b7c2cd308d5b6f9b7 > Signed-off-by: Harish Kasiviswanathan <Harish.Kasiviswanathan@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c > index a4f0ecf..88e49f7 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c > @@ -199,10 +199,10 @@ int amdgpu_sync_resv(struct amdgpu_device *adev, > return -EINVAL; > > f = reservation_object_get_excl(resv); > - fence_owner = amdgpu_sync_get_owner(f); > - if (fence_owner != AMDGPU_FENCE_OWNER_KFD || > - owner != AMDGPU_FENCE_OWNER_VM) > + if (f) { > r = amdgpu_sync_fence(adev, sync, f); > + return r; > + } That is not correct either. [HK]: I thought resv. object can have either an exclusive fence or shared fences but not both at the same time. Is that correct? It must look like this: > /* always sync to the exclusive fence */ > f = reservation_object_get_excl(resv); > r = amdgpu_sync_fence(adev, sync, f); > > flist = reservation_object_get_list(resv); > if (!flist || r) > return r Otherwise you ignore all shared fences and completely mess up the dependencies handling. I suggest to just revert the patch which originally changed the code away from what it looks like on amd-staging-drm-next. [HK]: This patch is going to 17.40 release branch. There is a serious performance issue in SSG benchmark and this is caused by unnecessary evictions. Regards, Christian. > > if (explicit_sync) > return r; > @@ -216,7 +216,7 @@ int amdgpu_sync_resv(struct amdgpu_device *adev, > reservation_object_held(resv)); > fence_owner = amdgpu_sync_get_owner(f); > if (fence_owner == AMDGPU_FENCE_OWNER_KFD && > - owner == AMDGPU_FENCE_OWNER_VM) > + owner != AMDGPU_FENCE_OWNER_UNDEFINED) > continue; > > if (amdgpu_sync_same_dev(adev, f)) { _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 4+ messages in thread
[parent not found: <DM3PR1201MB10388630530FC1B8B34908758C430-BBcFnVpqZhWjUUTFdQAMQmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>]
* Re: [PATCH v2] drm/amdgpu: Sync KFD fence only for move/evict [not found] ` <DM3PR1201MB10388630530FC1B8B34908758C430-BBcFnVpqZhWjUUTFdQAMQmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org> @ 2017-10-21 21:03 ` Christian König 0 siblings, 0 replies; 4+ messages in thread From: Christian König @ 2017-10-21 21:03 UTC (permalink / raw) To: Kasiviswanathan, Harish, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW Am 20.10.2017 um 20:53 schrieb Kasiviswanathan, Harish: > > -----Original Message----- > From: Christian König [mailto:ckoenig.leichtzumerken@gmail.com] > Sent: Friday, October 20, 2017 2:38 PM > To: Kasiviswanathan, Harish <Harish.Kasiviswanathan@amd.com>; amd-gfx@lists.freedesktop.org > Subject: Re: [PATCH v2] drm/amdgpu: Sync KFD fence only for move/evict > > Am 20.10.2017 um 19:18 schrieb Harish Kasiviswanathan: >> A single KFD eviction fence is attached to all the BOs of a process >> including BOs imported. This fence ensures that all BOs belonging to >> that process stays resident when the process queues are active. >> >> Don't add this eviction fence to any sync object unless it is a move >> or evict job. These jobs are identified by the fence owner >> AMDGPU_FENCE_OWNER_UNDEFINED >> >> v2: Always sync to exclusive fence >> >> Change-Id: I8752d1cf6b2a1c4f2a57292b7c2cd308d5b6f9b7 >> Signed-off-by: Harish Kasiviswanathan <Harish.Kasiviswanathan@amd.com> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c >> index a4f0ecf..88e49f7 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c >> @@ -199,10 +199,10 @@ int amdgpu_sync_resv(struct amdgpu_device *adev, >> return -EINVAL; >> >> f = reservation_object_get_excl(resv); >> - fence_owner = amdgpu_sync_get_owner(f); >> - if (fence_owner != AMDGPU_FENCE_OWNER_KFD || >> - owner != AMDGPU_FENCE_OWNER_VM) >> + if (f) { >> r = amdgpu_sync_fence(adev, sync, f); >> + return r; >> + } > That is not correct either. > > [HK]: I thought resv. object can have either an exclusive fence or shared fences but not both at the same time. Is that correct? No, that assumption is incorrect. A reservation object can has exclusive and shared fences at the same time (and that is actually the normal case). > > It must look like this: >> /* always sync to the exclusive fence */ >> f = reservation_object_get_excl(resv); >> r = amdgpu_sync_fence(adev, sync, f); >> >> flist = reservation_object_get_list(resv); >> if (!flist || r) >> return r > Otherwise you ignore all shared fences and completely mess up the dependencies handling. > > I suggest to just revert the patch which originally changed the code away from what it looks like on amd-staging-drm-next. > > [HK]: This patch is going to 17.40 release branch. There is a serious performance issue in SSG benchmark and this is caused by unnecessary evictions. Please revert that ASAP and get back to how the code looks like on amd-staging-drm-next. Otherwise you completely break correct synchronization. All you need is the hunk below for shared fences. Regards, Christian. > > Regards, > Christian. > > >> >> if (explicit_sync) >> return r; >> @@ -216,7 +216,7 @@ int amdgpu_sync_resv(struct amdgpu_device *adev, >> reservation_object_held(resv)); >> fence_owner = amdgpu_sync_get_owner(f); >> if (fence_owner == AMDGPU_FENCE_OWNER_KFD && >> - owner == AMDGPU_FENCE_OWNER_VM) >> + owner != AMDGPU_FENCE_OWNER_UNDEFINED) >> continue; >> >> if (amdgpu_sync_same_dev(adev, f)) { > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-10-21 21:03 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-20 17:18 [PATCH v2] drm/amdgpu: Sync KFD fence only for move/evict Harish Kasiviswanathan
[not found] ` <1508519937-6081-1-git-send-email-Harish.Kasiviswanathan-5C7GfCeVMHo@public.gmane.org>
2017-10-20 18:37 ` Christian König
[not found] ` <a501c93e-0181-2ab1-2a85-6d2d177ae8a3-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-10-20 18:53 ` Kasiviswanathan, Harish
[not found] ` <DM3PR1201MB10388630530FC1B8B34908758C430-BBcFnVpqZhWjUUTFdQAMQmrFom/aUZj6nBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>
2017-10-21 21:03 ` Christian König
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.