Re: [PATCH 2/6] [RFC]drm: add syncobj timeline support v7
Am 20.09.2018 um 11:45 schrieb Zhou, David(ChunMing): -Original Message- From: amd-gfx On Behalf Of Christian K?nig Sent: Thursday, September 20, 2018 5:35 PM To: Zhou, David(ChunMing) ; dri- de...@lists.freedesktop.org Cc: Dave Airlie ; Rakos, Daniel ; Daniel Vetter ; amd- g...@lists.freedesktop.org Subject: Re: [PATCH 2/6] [RFC]drm: add syncobj timeline support v7 The only thing I can still see is that you use wait_event_timeout() instead of wait_event_interruptible(). Any particular reason for that? I tried again after you said last thread, CTS always fail, and syncobj unit test fails as well. Can you figure out why that happened? It sounds like an IOCTL is not correctly restarted after an signal. Apart from that it now looks good to me. Thanks, Can I get your RB on it? No that the wait isn't interruptible is still a blocker. Btw, I realize Vulkan spec names semaphore type as binary and timeline, so how about change _TYPE_INDIVIDUAL to _TYPE_BINARY ? Not a bad name either. Christian. Regards, David Zhou Christian. Am 20.09.2018 um 11:29 schrieb Zhou, David(ChunMing): Ping... -Original Message- From: amd-gfx On Behalf Of Chunming Zhou Sent: Wednesday, September 19, 2018 5:18 PM To: dri-de...@lists.freedesktop.org Cc: Zhou, David(ChunMing) ; amd- g...@lists.freedesktop.org; Rakos, Daniel ; Daniel Vetter ; Dave Airlie ; Koenig, Christian Subject: [PATCH 2/6] [RFC]drm: add syncobj timeline support v7 This patch is for VK_KHR_timeline_semaphore extension, semaphore is called syncobj in kernel side: This extension introduces a new type of syncobj that has an integer payload identifying a point in a timeline. Such timeline syncobjs support the following operations: * CPU query - A host operation that allows querying the payload of the timeline syncobj. * CPU wait - A host operation that allows a blocking wait for a timeline syncobj to reach a specified value. * Device wait - A device operation that allows waiting for a timeline syncobj to reach a specified value. * Device signal - A device operation that allows advancing the timeline syncobj to a specified value. v1: Since it's a timeline, that means the front time point(PT) always is signaled before the late PT. a. signal PT design: Signal PT fence N depends on PT[N-1] fence and signal opertion fence, when PT[N] fence is signaled, the timeline will increase to value of PT[N]. b. wait PT design: Wait PT fence is signaled by reaching timeline point value, when timeline is increasing, will compare wait PTs value with new timeline value, if PT value is lower than timeline value, then wait PT will be signaled, otherwise keep in list. syncobj wait operation can wait on any point of timeline, so need a RB tree to order them. And wait PT could ahead of signal PT, we need a sumission fence to perform that. v2: 1. remove unused DRM_SYNCOBJ_CREATE_TYPE_NORMAL. (Christian) 2. move unexposed denitions to .c file. (Daniel Vetter) 3. split up the change to drm_syncobj_find_fence() in a separate patch. (Christian) 4. split up the change to drm_syncobj_replace_fence() in a separate patch. 5. drop the submission_fence implementation and instead use wait_event() for that. (Christian) 6. WARN_ON(point != 0) for NORMAL type syncobj case. (Daniel Vetter) v3: 1. replace normal syncobj with timeline implemenation. (Vetter and Christian) a. normal syncobj signal op will create a signal PT to tail of signal pt list. b. normal syncobj wait op will create a wait pt with last signal point, and this wait PT is only signaled by related signal point PT. 2. many bug fix and clean up 3. stub fence moving is moved to other patch. v4： 1. fix RB tree loop with while(node=rb_first(...)). (Christian) 2. fix syncobj lifecycle. (Christian) 3. only enable_signaling when there is wait_pt. (Christian) 4. fix timeline path issues. 5. write a timeline test in libdrm v5: (Christian) 1. semaphore is called syncobj in kernel side. 2. don't need 'timeline' characters in some function name. 3. keep syncobj cb. v6: (Christian) 1. merge syncobj_timeline to syncobj structure. 2. simplify some check sentences. 3. some misc change. 4. fix CTS failed issue. v7: (Christian) 1. error handling when creating signal pt. 2. remove timeline naming in func. 3. export flags in find_fence. 4. allow reset timeline. individual syncobj is tested by ./deqp-vk -n dEQP-VK*semaphore* timeline syncobj is tested by ./amdgpu_test -s 9 Signed-off-by: Chunming Zhou Cc: Christian Konig Cc: Dave Airlie Cc: Daniel Rakos Cc: Daniel Vetter --- drivers/gpu/drm/drm_syncobj.c | 293 ++--- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +- include/drm/drm_syncobj.h | 65 ++--- include/uapi/drm/drm.h | 1 + 4 files changed, 287 insertions(+), 74 deletions(-) diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
RE: [PATCH 2/6] [RFC]drm: add syncobj timeline support v7
> -Original Message- > From: amd-gfx On Behalf Of > Christian K?nig > Sent: Thursday, September 20, 2018 5:35 PM > To: Zhou, David(ChunMing) ; dri- > de...@lists.freedesktop.org > Cc: Dave Airlie ; Rakos, Daniel > ; Daniel Vetter ; amd- > g...@lists.freedesktop.org > Subject: Re: [PATCH 2/6] [RFC]drm: add syncobj timeline support v7 > > The only thing I can still see is that you use wait_event_timeout() instead of > wait_event_interruptible(). > > Any particular reason for that? I tried again after you said last thread, CTS always fail, and syncobj unit test fails as well. > > Apart from that it now looks good to me. Thanks, Can I get your RB on it? Btw, I realize Vulkan spec names semaphore type as binary and timeline, so how about change _TYPE_INDIVIDUAL to _TYPE_BINARY ? Regards, David Zhou > > Christian. > > Am 20.09.2018 um 11:29 schrieb Zhou, David(ChunMing): > > Ping... > > > >> -Original Message- > >> From: amd-gfx On Behalf Of > >> Chunming Zhou > >> Sent: Wednesday, September 19, 2018 5:18 PM > >> To: dri-de...@lists.freedesktop.org > >> Cc: Zhou, David(ChunMing) ; amd- > >> g...@lists.freedesktop.org; Rakos, Daniel ; > >> Daniel Vetter ; Dave Airlie ; > >> Koenig, Christian > >> Subject: [PATCH 2/6] [RFC]drm: add syncobj timeline support v7 > >> > >> This patch is for VK_KHR_timeline_semaphore extension, semaphore is > >> called syncobj in kernel side: > >> This extension introduces a new type of syncobj that has an integer > >> payload identifying a point in a timeline. Such timeline syncobjs > >> support the following > >> operations: > >> * CPU query - A host operation that allows querying the payload of the > >> timeline syncobj. > >> * CPU wait - A host operation that allows a blocking wait for a > >> timeline syncobj to reach a specified value. > >> * Device wait - A device operation that allows waiting for a > >> timeline syncobj to reach a specified value. > >> * Device signal - A device operation that allows advancing the > >> timeline syncobj to a specified value. > >> > >> v1: > >> Since it's a timeline, that means the front time point(PT) always is > >> signaled before the late PT. > >> a. signal PT design: > >> Signal PT fence N depends on PT[N-1] fence and signal opertion fence, > >> when PT[N] fence is signaled, the timeline will increase to value of PT[N]. > >> b. wait PT design: > >> Wait PT fence is signaled by reaching timeline point value, when > >> timeline is increasing, will compare wait PTs value with new timeline > >> value, if PT value is lower than timeline value, then wait PT will be > >> signaled, > otherwise keep in list. > >> syncobj wait operation can wait on any point of timeline, so need a > >> RB tree to order them. And wait PT could ahead of signal PT, we need > >> a sumission fence to perform that. > >> > >> v2: > >> 1. remove unused DRM_SYNCOBJ_CREATE_TYPE_NORMAL. (Christian) 2. > >> move unexposed denitions to .c file. (Daniel Vetter) 3. split up the > >> change to > >> drm_syncobj_find_fence() in a separate patch. (Christian) 4. split up > >> the change to drm_syncobj_replace_fence() in a separate patch. > >> 5. drop the submission_fence implementation and instead use > >> wait_event() for that. (Christian) 6. WARN_ON(point != 0) for NORMAL > type syncobj case. > >> (Daniel Vetter) > >> > >> v3: > >> 1. replace normal syncobj with timeline implemenation. (Vetter and > Christian) > >> a. normal syncobj signal op will create a signal PT to tail of signal > >> pt list. > >> b. normal syncobj wait op will create a wait pt with last signal > >> point, and this wait PT is only signaled by related signal point PT. > >> 2. many bug fix and clean up > >> 3. stub fence moving is moved to other patch. > >> > >> v4： > >> 1. fix RB tree loop with while(node=rb_first(...)). (Christian) 2. > >> fix syncobj lifecycle. (Christian) 3. only enable_signaling when > >> there is wait_pt. (Christian) 4. fix timeline path issues. > >> 5. write a timeline test in libdrm > >> > >> v5: (Christian) > >> 1. semaphore is called syncobj in kernel side. > >> 2. don't need 'timeline' characters in some function name. > >> 3. keep syncobj cb. > >> > >> v6: (Christian) &
Re: [PATCH 2/6] [RFC]drm: add syncobj timeline support v7
The only thing I can still see is that you use wait_event_timeout()
instead of wait_event_interruptible().
Any particular reason for that?
Apart from that it now looks good to me.
Christian.
Am 20.09.2018 um 11:29 schrieb Zhou, David(ChunMing):
Ping...
-Original Message-
From: amd-gfx On Behalf Of
Chunming Zhou
Sent: Wednesday, September 19, 2018 5:18 PM
To: dri-de...@lists.freedesktop.org
Cc: Zhou, David(ChunMing) ; amd-
g...@lists.freedesktop.org; Rakos, Daniel ; Daniel
Vetter ; Dave Airlie ; Koenig,
Christian
Subject: [PATCH 2/6] [RFC]drm: add syncobj timeline support v7
This patch is for VK_KHR_timeline_semaphore extension, semaphore is
called syncobj in kernel side:
This extension introduces a new type of syncobj that has an integer payload
identifying a point in a timeline. Such timeline syncobjs support the following
operations:
* CPU query - A host operation that allows querying the payload of the
timeline syncobj.
* CPU wait - A host operation that allows a blocking wait for a
timeline syncobj to reach a specified value.
* Device wait - A device operation that allows waiting for a
timeline syncobj to reach a specified value.
* Device signal - A device operation that allows advancing the
timeline syncobj to a specified value.
v1:
Since it's a timeline, that means the front time point(PT) always is signaled
before the late PT.
a. signal PT design:
Signal PT fence N depends on PT[N-1] fence and signal opertion fence, when
PT[N] fence is signaled, the timeline will increase to value of PT[N].
b. wait PT design:
Wait PT fence is signaled by reaching timeline point value, when timeline is
increasing, will compare wait PTs value with new timeline value, if PT value is
lower than timeline value, then wait PT will be signaled, otherwise keep in
list.
syncobj wait operation can wait on any point of timeline, so need a RB tree to
order them. And wait PT could ahead of signal PT, we need a sumission fence
to perform that.
v2:
1. remove unused DRM_SYNCOBJ_CREATE_TYPE_NORMAL. (Christian) 2.
move unexposed denitions to .c file. (Daniel Vetter) 3. split up the change to
drm_syncobj_find_fence() in a separate patch. (Christian) 4. split up the
change to drm_syncobj_replace_fence() in a separate patch.
5. drop the submission_fence implementation and instead use wait_event()
for that. (Christian) 6. WARN_ON(point != 0) for NORMAL type syncobj case.
(Daniel Vetter)
v3:
1. replace normal syncobj with timeline implemenation. (Vetter and Christian)
a. normal syncobj signal op will create a signal PT to tail of signal pt
list.
b. normal syncobj wait op will create a wait pt with last signal point,
and this
wait PT is only signaled by related signal point PT.
2. many bug fix and clean up
3. stub fence moving is moved to other patch.
v4：
1. fix RB tree loop with while(node=rb_first(...)). (Christian) 2. fix syncobj
lifecycle. (Christian) 3. only enable_signaling when there is wait_pt.
(Christian)
4. fix timeline path issues.
5. write a timeline test in libdrm
v5: (Christian)
1. semaphore is called syncobj in kernel side.
2. don't need 'timeline' characters in some function name.
3. keep syncobj cb.
v6: (Christian)
1. merge syncobj_timeline to syncobj structure.
2. simplify some check sentences.
3. some misc change.
4. fix CTS failed issue.
v7: (Christian)
1. error handling when creating signal pt.
2. remove timeline naming in func.
3. export flags in find_fence.
4. allow reset timeline.
individual syncobj is tested by ./deqp-vk -n dEQP-VK*semaphore* timeline
syncobj is tested by ./amdgpu_test -s 9
Signed-off-by: Chunming Zhou
Cc: Christian Konig
Cc: Dave Airlie
Cc: Daniel Rakos
Cc: Daniel Vetter
---
drivers/gpu/drm/drm_syncobj.c | 293 ++---
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
include/drm/drm_syncobj.h | 65 ++---
include/uapi/drm/drm.h | 1 +
4 files changed, 287 insertions(+), 74 deletions(-)
diff --git a/drivers/gpu/drm/drm_syncobj.c
b/drivers/gpu/drm/drm_syncobj.c index f796c9fc3858..95b60ac045c6 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -56,6 +56,9 @@
#include "drm_internal.h"
#include
+/* merge normal syncobj to timeline syncobj, the point interval is 1 */
+#define DRM_SYNCOBJ_INDIVIDUAL_POINT 1
+
struct drm_syncobj_stub_fence {
struct dma_fence base;
spinlock_t lock;
@@ -82,6 +85,11 @@ static const struct dma_fence_ops
drm_syncobj_stub_fence_ops = {
.release = drm_syncobj_stub_fence_release, };
+struct drm_syncobj_signal_pt {
+ struct dma_fence_array *base;
+ u64value;
+ struct list_head list;
+};
/**
* drm_syncobj_find - lookup and reference a sync object.
@@ -124,8 +132,8 @@ static int
drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj, {
int ret;
- *fence = drm_syncobj_fence_get(syncobj);
- if
RE: [PATCH 2/6] [RFC]drm: add syncobj timeline support v7
Ping...
> -Original Message-
> From: amd-gfx On Behalf Of
> Chunming Zhou
> Sent: Wednesday, September 19, 2018 5:18 PM
> To: dri-de...@lists.freedesktop.org
> Cc: Zhou, David(ChunMing) ; amd-
> g...@lists.freedesktop.org; Rakos, Daniel ; Daniel
> Vetter ; Dave Airlie ; Koenig,
> Christian
> Subject: [PATCH 2/6] [RFC]drm: add syncobj timeline support v7
>
> This patch is for VK_KHR_timeline_semaphore extension, semaphore is
> called syncobj in kernel side:
> This extension introduces a new type of syncobj that has an integer payload
> identifying a point in a timeline. Such timeline syncobjs support the
> following
> operations:
>* CPU query - A host operation that allows querying the payload of the
> timeline syncobj.
>* CPU wait - A host operation that allows a blocking wait for a
> timeline syncobj to reach a specified value.
>* Device wait - A device operation that allows waiting for a
> timeline syncobj to reach a specified value.
>* Device signal - A device operation that allows advancing the
> timeline syncobj to a specified value.
>
> v1:
> Since it's a timeline, that means the front time point(PT) always is signaled
> before the late PT.
> a. signal PT design:
> Signal PT fence N depends on PT[N-1] fence and signal opertion fence, when
> PT[N] fence is signaled, the timeline will increase to value of PT[N].
> b. wait PT design:
> Wait PT fence is signaled by reaching timeline point value, when timeline is
> increasing, will compare wait PTs value with new timeline value, if PT value
> is
> lower than timeline value, then wait PT will be signaled, otherwise keep in
> list.
> syncobj wait operation can wait on any point of timeline, so need a RB tree to
> order them. And wait PT could ahead of signal PT, we need a sumission fence
> to perform that.
>
> v2:
> 1. remove unused DRM_SYNCOBJ_CREATE_TYPE_NORMAL. (Christian) 2.
> move unexposed denitions to .c file. (Daniel Vetter) 3. split up the change to
> drm_syncobj_find_fence() in a separate patch. (Christian) 4. split up the
> change to drm_syncobj_replace_fence() in a separate patch.
> 5. drop the submission_fence implementation and instead use wait_event()
> for that. (Christian) 6. WARN_ON(point != 0) for NORMAL type syncobj case.
> (Daniel Vetter)
>
> v3:
> 1. replace normal syncobj with timeline implemenation. (Vetter and Christian)
> a. normal syncobj signal op will create a signal PT to tail of signal pt
> list.
> b. normal syncobj wait op will create a wait pt with last signal point,
> and this
> wait PT is only signaled by related signal point PT.
> 2. many bug fix and clean up
> 3. stub fence moving is moved to other patch.
>
> v4：
> 1. fix RB tree loop with while(node=rb_first(...)). (Christian) 2. fix syncobj
> lifecycle. (Christian) 3. only enable_signaling when there is wait_pt.
> (Christian)
> 4. fix timeline path issues.
> 5. write a timeline test in libdrm
>
> v5: (Christian)
> 1. semaphore is called syncobj in kernel side.
> 2. don't need 'timeline' characters in some function name.
> 3. keep syncobj cb.
>
> v6: (Christian)
> 1. merge syncobj_timeline to syncobj structure.
> 2. simplify some check sentences.
> 3. some misc change.
> 4. fix CTS failed issue.
>
> v7: (Christian)
> 1. error handling when creating signal pt.
> 2. remove timeline naming in func.
> 3. export flags in find_fence.
> 4. allow reset timeline.
>
> individual syncobj is tested by ./deqp-vk -n dEQP-VK*semaphore* timeline
> syncobj is tested by ./amdgpu_test -s 9
>
> Signed-off-by: Chunming Zhou
> Cc: Christian Konig
> Cc: Dave Airlie
> Cc: Daniel Rakos
> Cc: Daniel Vetter
> ---
> drivers/gpu/drm/drm_syncobj.c | 293 ++---
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
> include/drm/drm_syncobj.h | 65 ++---
> include/uapi/drm/drm.h | 1 +
> 4 files changed, 287 insertions(+), 74 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_syncobj.c
> b/drivers/gpu/drm/drm_syncobj.c index f796c9fc3858..95b60ac045c6 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -56,6 +56,9 @@
> #include "drm_internal.h"
> #include
>
> +/* merge normal syncobj to timeline syncobj, the point interval is 1 */
> +#define DRM_SYNCOBJ_INDIVIDUAL_POINT 1
> +
> struct drm_syncobj_stub_fence {
> struct dma_fence base;
> spinlock_t lock;
> @@ -82,6 +85,11 @@ static const struct dma_fence_ops
> drm_syncobj_stub_fence_ops = {
> .release = drm_syncobj_stub_fence_release, };
>
> +struct drm_syncobj_signal_pt {
> + struct dma_fence_array *base;
> + u64value;
> + struct list_head list;
> +};
>
> /**
> * drm_syncobj_find - lookup and reference a sync object.
> @@ -124,8 +132,8 @@ static int
> drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj, {
> int ret;
>
> - *fence = drm_syncobj_fence_get(syncobj);
> - if (*fence)
> + ret =
