* [PATCH 1/4] [RFC]drm: add syncobj timeline support v6
@ 2018-09-19 4:26 Chunming Zhou
[not found] ` <20180919042659.3769-1-david1.zhou-5C7GfCeVMHo@public.gmane.org>
0 siblings, 1 reply; 11+ messages in thread
From: Chunming Zhou @ 2018-09-19 4:26 UTC (permalink / raw)
To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Cc: Chunming Zhou, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
Daniel Rakos, Daniel Vetter, Dave Airlie, Christian Konig
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
normal syncobj is tested by ./deqp-vk -n dEQP-VK*semaphore*
timeline syncobj is tested by ./amdgpu_test -s 9
Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
Cc: Christian Konig <christian.koenig@amd.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Daniel Rakos <Daniel.Rakos@amd.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
drivers/gpu/drm/drm_syncobj.c | 307 ++++++++++++++++++---
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
include/drm/drm_syncobj.h | 65 ++---
include/uapi/drm/drm.h | 1 +
4 files changed, 299 insertions(+), 76 deletions(-)
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index e9ce623d049e..987b5a120b65 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -56,6 +56,9 @@
#include "drm_internal.h"
#include <drm/drm_syncobj.h>
+/* 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;
+ u64 value;
+ struct list_head list;
+};
/**
* drm_syncobj_find - lookup and reference a sync object.
@@ -117,6 +125,9 @@ static void drm_syncobj_add_callback_locked(struct drm_syncobj *syncobj,
list_add_tail(&cb->node, &syncobj->cb_list);
}
+static int drm_syncobj_search_fence(struct drm_syncobj *syncobj, u64 point,
+ u64 flags, struct dma_fence **fence);
+
static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj,
struct dma_fence **fence,
struct drm_syncobj_cb *cb,
@@ -124,8 +135,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 = drm_syncobj_search_fence(syncobj, 0, 0, fence);
+ if (!ret)
return 1;
spin_lock(&syncobj->lock);
@@ -133,10 +144,12 @@ static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj,
* have the lock, try one more time just to be sure we don't add a
* callback when a fence has already been set.
*/
- if (syncobj->fence) {
- *fence = dma_fence_get(rcu_dereference_protected(syncobj->fence,
- lockdep_is_held(&syncobj->lock)));
- ret = 1;
+ if (fence && !list_empty(&syncobj->signal_pt_list)) {
+ spin_unlock(&syncobj->lock);
+ drm_syncobj_search_fence(syncobj, 0, 0, fence);
+ if (*fence)
+ ret = 1;
+ spin_lock(&syncobj->lock);
} else {
*fence = NULL;
drm_syncobj_add_callback_locked(syncobj, cb, func);
@@ -164,6 +177,153 @@ void drm_syncobj_remove_callback(struct drm_syncobj *syncobj,
spin_unlock(&syncobj->lock);
}
+static void drm_syncobj_timeline_init(struct drm_syncobj *syncobj)
+{
+ spin_lock(&syncobj->lock);
+ syncobj->timeline_context = dma_fence_context_alloc(1);
+ syncobj->timeline = 0;
+ syncobj->signal_point = 0;
+ init_waitqueue_head(&syncobj->wq);
+
+ INIT_LIST_HEAD(&syncobj->signal_pt_list);
+ spin_unlock(&syncobj->lock);
+}
+
+static void drm_syncobj_timeline_fini(struct drm_syncobj *syncobj)
+{
+ struct drm_syncobj_signal_pt *signal_pt = NULL, *tmp;
+
+ spin_lock(&syncobj->lock);
+ list_for_each_entry_safe(signal_pt, tmp,
+ &syncobj->signal_pt_list, list) {
+ list_del(&signal_pt->list);
+ dma_fence_put(&signal_pt->base->base);
+ kfree(signal_pt);
+ }
+ spin_unlock(&syncobj->lock);
+}
+
+static struct dma_fence
+*drm_syncobj_find_signal_pt_for_point(struct drm_syncobj *syncobj,
+ uint64_t point)
+{
+ struct drm_syncobj_signal_pt *signal_pt;
+
+ if ((syncobj->type == DRM_SYNCOBJ_TYPE_TIMELINE) &&
+ (point <= syncobj->timeline)) {
+ struct drm_syncobj_stub_fence *fence =
+ kzalloc(sizeof(struct drm_syncobj_stub_fence),
+ GFP_KERNEL);
+
+ if (!fence)
+ return NULL;
+ spin_lock_init(&fence->lock);
+ dma_fence_init(&fence->base,
+ &drm_syncobj_stub_fence_ops,
+ &fence->lock,
+ syncobj->timeline_context,
+ point);
+
+ dma_fence_signal(&fence->base);
+ return &fence->base;
+ }
+
+ list_for_each_entry(signal_pt, &syncobj->signal_pt_list, list) {
+ if (point > signal_pt->value)
+ continue;
+ if ((syncobj->type == DRM_SYNCOBJ_TYPE_INDIVIDUAL) &&
+ (point != signal_pt->value))
+ continue;
+ return dma_fence_get(&signal_pt->base->base);
+ }
+ return NULL;
+}
+
+static int drm_syncobj_create_signal_pt(struct drm_syncobj *syncobj,
+ struct dma_fence *fence,
+ u64 point)
+{
+ struct drm_syncobj_signal_pt *signal_pt =
+ kzalloc(sizeof(struct drm_syncobj_signal_pt), GFP_KERNEL);
+ struct drm_syncobj_signal_pt *tail_pt;
+ struct dma_fence **fences;
+ int num_fences = 0;
+ int ret = 0, i;
+
+ if (!signal_pt)
+ return -ENOMEM;
+ if (syncobj->signal_point >= point) {
+ DRM_WARN("A later signal is ready!");
+ goto out;
+ }
+ if (!fence)
+ goto out;
+
+ fences = kmalloc_array(sizeof(void *), 2, GFP_KERNEL);
+ if (!fences)
+ goto out;
+ fences[num_fences++] = dma_fence_get(fence);
+ /* timeline syncobj must take this dependency */
+ if (syncobj->type == DRM_SYNCOBJ_TYPE_TIMELINE) {
+ spin_lock(&syncobj->lock);
+ if (!list_empty(&syncobj->signal_pt_list)) {
+ tail_pt = list_last_entry(&syncobj->signal_pt_list,
+ struct drm_syncobj_signal_pt, list);
+ fences[num_fences++] = dma_fence_get(&tail_pt->base->base);
+ }
+ spin_unlock(&syncobj->lock);
+ }
+ signal_pt->base = dma_fence_array_create(num_fences, fences,
+ syncobj->timeline_context,
+ point, false);
+ if (!signal_pt->base)
+ goto fail;
+
+ spin_lock(&syncobj->lock);
+ signal_pt->value = point;
+ INIT_LIST_HEAD(&signal_pt->list);
+ list_add_tail(&signal_pt->list, &syncobj->signal_pt_list);
+ syncobj->signal_point = point;
+ spin_unlock(&syncobj->lock);
+ wake_up_all(&syncobj->wq);
+
+ return 0;
+fail:
+ for (i = 0; i < num_fences; i++)
+ dma_fence_put(fences[i]);
+ kfree(fences);
+out:
+ kfree(signal_pt);
+ return ret;
+}
+
+static void drm_syncobj_garbage_collection(struct drm_syncobj *syncobj)
+{
+ struct drm_syncobj_signal_pt *signal_pt, *tmp, *tail_pt;
+
+ spin_lock(&syncobj->lock);
+ tail_pt = list_last_entry(&syncobj->signal_pt_list,
+ struct drm_syncobj_signal_pt,
+ list);
+ list_for_each_entry_safe(signal_pt, tmp,
+ &syncobj->signal_pt_list, list) {
+ if (syncobj->type == DRM_SYNCOBJ_TYPE_INDIVIDUAL &&
+ signal_pt == tail_pt)
+ continue;
+ if (dma_fence_is_signaled(&signal_pt->base->base)) {
+ syncobj->timeline = signal_pt->value;
+ list_del(&signal_pt->list);
+ dma_fence_put(&signal_pt->base->base);
+ kfree(signal_pt);
+ } else {
+ /*signal_pt is in order in list, from small to big, so
+ * the later must not be signal either */
+ break;
+ }
+ }
+
+ spin_unlock(&syncobj->lock);
+}
/**
* drm_syncobj_replace_fence - replace fence in a sync object.
* @syncobj: Sync object to replace fence in
@@ -176,28 +336,29 @@ void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
u64 point,
struct dma_fence *fence)
{
- struct dma_fence *old_fence;
- struct drm_syncobj_cb *cur, *tmp;
-
- if (fence)
- dma_fence_get(fence);
-
- spin_lock(&syncobj->lock);
-
- old_fence = rcu_dereference_protected(syncobj->fence,
- lockdep_is_held(&syncobj->lock));
- rcu_assign_pointer(syncobj->fence, fence);
+ u64 pt_value = point;
+
+ drm_syncobj_garbage_collection(syncobj);
+ if (syncobj->type == DRM_SYNCOBJ_TYPE_INDIVIDUAL) {
+ if (!fence) {
+ drm_syncobj_timeline_fini(syncobj);
+ drm_syncobj_timeline_init(syncobj);
+ return;
+ }
+ pt_value = syncobj->signal_point +
+ DRM_SYNCOBJ_INDIVIDUAL_POINT;
+ }
+ drm_syncobj_create_signal_pt(syncobj, fence, pt_value);
+ if (fence) {
+ struct drm_syncobj_cb *cur, *tmp;
- if (fence != old_fence) {
+ spin_lock(&syncobj->lock);
list_for_each_entry_safe(cur, tmp, &syncobj->cb_list, node) {
list_del_init(&cur->node);
cur->func(syncobj, cur);
}
+ spin_unlock(&syncobj->lock);
}
-
- spin_unlock(&syncobj->lock);
-
- dma_fence_put(old_fence);
}
EXPORT_SYMBOL(drm_syncobj_replace_fence);
@@ -220,6 +381,60 @@ static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj)
return 0;
}
+#define WAIT_FOR_SUBMIT_MSECS 10000
+
+static struct dma_fence *
+drm_syncobj_point_get(struct drm_syncobj *syncobj, u64 point, u64 flags)
+{
+ struct dma_fence *fence;
+ int ret = 0;
+
+ if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
+ ret = wait_event_timeout(syncobj->wq,
+ point <= syncobj->signal_point,
+ msecs_to_jiffies(WAIT_FOR_SUBMIT_MSECS));
+ if (ret <= 0)
+ return NULL;
+ }
+ spin_lock(&syncobj->lock);
+ fence = drm_syncobj_find_signal_pt_for_point(syncobj, point);
+ spin_unlock(&syncobj->lock);
+ return fence;
+}
+
+static int drm_syncobj_search_fence(struct drm_syncobj *syncobj, u64 point,
+ u64 flags, struct dma_fence **fence)
+{
+ u64 pt_value = point;
+ int ret = 0;
+
+ if (!syncobj)
+ return -ENOENT;
+
+ drm_syncobj_garbage_collection(syncobj);
+ if (syncobj->type == DRM_SYNCOBJ_TYPE_INDIVIDUAL) {
+ /*INDIVIDUAL syncobj always wait on last pt */
+ pt_value = syncobj->signal_point;
+
+ if (pt_value == 0)
+ pt_value += DRM_SYNCOBJ_INDIVIDUAL_POINT;
+ }
+ *fence = drm_syncobj_point_get(syncobj, pt_value, flags);
+ if (!*fence) {
+ ret = -EINVAL;
+ }
+ return ret;
+}
+
+int drm_syncobj_lookup_fence(struct drm_syncobj *syncobj, u64 point,
+ struct dma_fence **fence) {
+
+ return drm_syncobj_search_fence(syncobj, point,
+ DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT,
+ fence);
+}
+EXPORT_SYMBOL(drm_syncobj_lookup_fence);
+
/**
* drm_syncobj_find_fence - lookup and reference the fence in a sync object
* @file_private: drm file private pointer
@@ -228,7 +443,7 @@ static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj)
* @fence: out parameter for the fence
*
* This is just a convenience function that combines drm_syncobj_find() and
- * drm_syncobj_fence_get().
+ * drm_syncobj_lookup_fence().
*
* Returns 0 on success or a negative error value on failure. On success @fence
* contains a reference to the fence, which must be released by calling
@@ -236,18 +451,11 @@ static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj)
*/
int drm_syncobj_find_fence(struct drm_file *file_private,
u32 handle, u64 point,
- struct dma_fence **fence)
-{
+ struct dma_fence **fence) {
struct drm_syncobj *syncobj = drm_syncobj_find(file_private, handle);
- int ret = 0;
-
- if (!syncobj)
- return -ENOENT;
+ int ret;
- *fence = drm_syncobj_fence_get(syncobj);
- if (!*fence) {
- ret = -EINVAL;
- }
+ ret = drm_syncobj_lookup_fence(syncobj, point, fence);
drm_syncobj_put(syncobj);
return ret;
}
@@ -264,7 +472,7 @@ void drm_syncobj_free(struct kref *kref)
struct drm_syncobj *syncobj = container_of(kref,
struct drm_syncobj,
refcount);
- drm_syncobj_replace_fence(syncobj, 0, NULL);
+ drm_syncobj_timeline_fini(syncobj);
kfree(syncobj);
}
EXPORT_SYMBOL(drm_syncobj_free);
@@ -294,6 +502,11 @@ int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags,
kref_init(&syncobj->refcount);
INIT_LIST_HEAD(&syncobj->cb_list);
spin_lock_init(&syncobj->lock);
+ if (flags & DRM_SYNCOBJ_CREATE_TYPE_TIMELINE)
+ syncobj->type = DRM_SYNCOBJ_TYPE_TIMELINE;
+ else
+ syncobj->type = DRM_SYNCOBJ_TYPE_INDIVIDUAL;
+ drm_syncobj_timeline_init(syncobj);
if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) {
ret = drm_syncobj_assign_null_handle(syncobj);
@@ -576,7 +789,8 @@ drm_syncobj_create_ioctl(struct drm_device *dev, void *data,
return -ENODEV;
/* no valid flags yet */
- if (args->flags & ~DRM_SYNCOBJ_CREATE_SIGNALED)
+ if (args->flags & ~(DRM_SYNCOBJ_CREATE_SIGNALED |
+ DRM_SYNCOBJ_CREATE_TYPE_TIMELINE))
return -EINVAL;
return drm_syncobj_create_as_handle(file_private,
@@ -669,9 +883,8 @@ static void syncobj_wait_syncobj_func(struct drm_syncobj *syncobj,
struct syncobj_wait_entry *wait =
container_of(cb, struct syncobj_wait_entry, syncobj_cb);
- /* This happens inside the syncobj lock */
- wait->fence = dma_fence_get(rcu_dereference_protected(syncobj->fence,
- lockdep_is_held(&syncobj->lock)));
+ drm_syncobj_search_fence(syncobj, 0, 0, &wait->fence);
+
wake_up_process(wait->task);
}
@@ -698,7 +911,8 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
signaled_count = 0;
for (i = 0; i < count; ++i) {
entries[i].task = current;
- entries[i].fence = drm_syncobj_fence_get(syncobjs[i]);
+ ret = drm_syncobj_search_fence(syncobjs[i], 0, 0,
+ &entries[i].fence);
if (!entries[i].fence) {
if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
continue;
@@ -970,12 +1184,19 @@ drm_syncobj_reset_ioctl(struct drm_device *dev, void *data,
if (ret < 0)
return ret;
- for (i = 0; i < args->count_handles; i++)
- drm_syncobj_replace_fence(syncobjs[i], 0, NULL);
-
+ for (i = 0; i < args->count_handles; i++) {
+ if (syncobjs[i]->type == DRM_SYNCOBJ_TYPE_TIMELINE) {
+ DRM_ERROR("timeline syncobj cannot reset!\n");
+ ret = -EINVAL;
+ goto out;
+ }
+ drm_syncobj_timeline_fini(syncobjs[i]);
+ drm_syncobj_timeline_init(syncobjs[i]);
+ }
+out:
drm_syncobj_array_free(syncobjs, args->count_handles);
- return 0;
+ return ret;
}
int
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 0a8d2d64f380..a040fa3281df 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -2137,7 +2137,7 @@ await_fence_array(struct i915_execbuffer *eb,
if (!(flags & I915_EXEC_FENCE_WAIT))
continue;
- fence = drm_syncobj_fence_get(syncobj);
+ drm_syncobj_lookup_fence(syncobj, 0, &fence);
if (!fence)
return -EINVAL;
diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
index 425432b85a87..6dac479fedfe 100644
--- a/include/drm/drm_syncobj.h
+++ b/include/drm/drm_syncobj.h
@@ -30,6 +30,11 @@
struct drm_syncobj_cb;
+enum drm_syncobj_type {
+ DRM_SYNCOBJ_TYPE_INDIVIDUAL,
+ DRM_SYNCOBJ_TYPE_TIMELINE
+};
+
/**
* struct drm_syncobj - sync object.
*
@@ -41,19 +46,36 @@ struct drm_syncobj {
*/
struct kref refcount;
/**
- * @fence:
- * NULL or a pointer to the fence bound to this object.
- *
- * This field should not be used directly. Use drm_syncobj_fence_get()
- * and drm_syncobj_replace_fence() instead.
+ * @type: indicate syncobj type
+ */
+ enum drm_syncobj_type type;
+ /**
+ * @wq: wait signal operation work queue
+ */
+ wait_queue_head_t wq;
+ /**
+ * @timeline_context: fence context used by timeline
*/
- struct dma_fence __rcu *fence;
+ u64 timeline_context;
/**
- * @cb_list: List of callbacks to call when the &fence gets replaced.
+ * @timeline: syncobj timeline value, which indicates point is signaled.
*/
+ u64 timeline;
+ /**
+ * @signal_point: which indicates the latest signaler point.
+ */
+ u64 signal_point;
+ /**
+ * @signal_pt_list: signaler point list.
+ */
+ struct list_head signal_pt_list;
+
+ /**
+ * @cb_list: List of callbacks to call when the &fence gets replaced.
+ */
struct list_head cb_list;
/**
- * @lock: Protects &cb_list and write-locks &fence.
+ * @lock: Protects syncobj list and write-locks &fence.
*/
spinlock_t lock;
/**
@@ -68,7 +90,7 @@ typedef void (*drm_syncobj_func_t)(struct drm_syncobj *syncobj,
/**
* struct drm_syncobj_cb - callback for drm_syncobj_add_callback
* @node: used by drm_syncob_add_callback to append this struct to
- * &drm_syncobj.cb_list
+ * &drm_syncobj.cb_list
* @func: drm_syncobj_func_t to call
*
* This struct will be initialized by drm_syncobj_add_callback, additional
@@ -106,29 +128,6 @@ drm_syncobj_put(struct drm_syncobj *obj)
kref_put(&obj->refcount, drm_syncobj_free);
}
-/**
- * drm_syncobj_fence_get - get a reference to a fence in a sync object
- * @syncobj: sync object.
- *
- * This acquires additional reference to &drm_syncobj.fence contained in @obj,
- * if not NULL. It is illegal to call this without already holding a reference.
- * No locks required.
- *
- * Returns:
- * Either the fence of @obj or NULL if there's none.
- */
-static inline struct dma_fence *
-drm_syncobj_fence_get(struct drm_syncobj *syncobj)
-{
- struct dma_fence *fence;
-
- rcu_read_lock();
- fence = dma_fence_get_rcu_safe(&syncobj->fence);
- rcu_read_unlock();
-
- return fence;
-}
-
struct drm_syncobj *drm_syncobj_find(struct drm_file *file_private,
u32 handle);
void drm_syncobj_replace_fence(struct drm_syncobj *syncobj, u64 point,
@@ -142,5 +141,7 @@ int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags,
int drm_syncobj_get_handle(struct drm_file *file_private,
struct drm_syncobj *syncobj, u32 *handle);
int drm_syncobj_get_fd(struct drm_syncobj *syncobj, int *p_fd);
+int drm_syncobj_lookup_fence(struct drm_syncobj *syncobj, u64 point,
+ struct dma_fence **fence);
#endif
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 300f336633f2..cebdb2541eb7 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -717,6 +717,7 @@ struct drm_prime_handle {
struct drm_syncobj_create {
__u32 handle;
#define DRM_SYNCOBJ_CREATE_SIGNALED (1 << 0)
+#define DRM_SYNCOBJ_CREATE_TYPE_TIMELINE (1 << 1)
__u32 flags;
};
--
2.17.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/4] drm: add support of syncobj timeline point wait v2
[not found] ` <20180919042659.3769-1-david1.zhou-5C7GfCeVMHo@public.gmane.org>
@ 2018-09-19 4:26 ` Chunming Zhou
2018-09-19 4:26 ` [PATCH 3/4] drm: add timeline syncobj payload query ioctl Chunming Zhou
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: Chunming Zhou @ 2018-09-19 4:26 UTC (permalink / raw)
To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Cc: Chunming Zhou, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
points array is one-to-one match with syncobjs array.
v2:
add seperate ioctl for timeline point wait, otherwise break uapi.
Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
---
drivers/gpu/drm/drm_internal.h | 2 +
drivers/gpu/drm/drm_ioctl.c | 2 +
drivers/gpu/drm/drm_syncobj.c | 99 +++++++++++++++++++++++++++++-----
include/uapi/drm/drm.h | 14 +++++
4 files changed, 103 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 0c4eb4a9ab31..566d44e3c782 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -183,6 +183,8 @@ int drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_private);
int drm_syncobj_wait_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_private);
+int drm_syncobj_timeline_wait_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_private);
int drm_syncobj_reset_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_private);
int drm_syncobj_signal_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 6b4a633b4240..c0891614f516 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -669,6 +669,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
DRM_UNLOCKED|DRM_RENDER_ALLOW),
DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_WAIT, drm_syncobj_wait_ioctl,
DRM_UNLOCKED|DRM_RENDER_ALLOW),
+ DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT, drm_syncobj_timeline_wait_ioctl,
+ DRM_UNLOCKED|DRM_RENDER_ALLOW),
DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_RESET, drm_syncobj_reset_ioctl,
DRM_UNLOCKED|DRM_RENDER_ALLOW),
DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_SIGNAL, drm_syncobj_signal_ioctl,
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 987b5a120b65..311ea7ba6f57 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -129,13 +129,14 @@ static int drm_syncobj_search_fence(struct drm_syncobj *syncobj, u64 point,
u64 flags, struct dma_fence **fence);
static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj,
+ u64 point,
struct dma_fence **fence,
struct drm_syncobj_cb *cb,
drm_syncobj_func_t func)
{
int ret;
- ret = drm_syncobj_search_fence(syncobj, 0, 0, fence);
+ ret = drm_syncobj_search_fence(syncobj, point, 0, fence);
if (!ret)
return 1;
@@ -146,7 +147,7 @@ static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj,
*/
if (fence && !list_empty(&syncobj->signal_pt_list)) {
spin_unlock(&syncobj->lock);
- drm_syncobj_search_fence(syncobj, 0, 0, fence);
+ drm_syncobj_search_fence(syncobj, point, 0, fence);
if (*fence)
ret = 1;
spin_lock(&syncobj->lock);
@@ -355,7 +356,9 @@ void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
spin_lock(&syncobj->lock);
list_for_each_entry_safe(cur, tmp, &syncobj->cb_list, node) {
list_del_init(&cur->node);
+ spin_unlock(&syncobj->lock);
cur->func(syncobj, cur);
+ spin_lock(&syncobj->lock);
}
spin_unlock(&syncobj->lock);
}
@@ -866,6 +869,7 @@ struct syncobj_wait_entry {
struct dma_fence *fence;
struct dma_fence_cb fence_cb;
struct drm_syncobj_cb syncobj_cb;
+ u64 point;
};
static void syncobj_wait_fence_func(struct dma_fence *fence,
@@ -883,12 +887,13 @@ static void syncobj_wait_syncobj_func(struct drm_syncobj *syncobj,
struct syncobj_wait_entry *wait =
container_of(cb, struct syncobj_wait_entry, syncobj_cb);
- drm_syncobj_search_fence(syncobj, 0, 0, &wait->fence);
+ drm_syncobj_search_fence(syncobj, wait->point, 0, &wait->fence);
wake_up_process(wait->task);
}
static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
+ void __user *user_points,
uint32_t count,
uint32_t flags,
signed long timeout,
@@ -896,13 +901,27 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
{
struct syncobj_wait_entry *entries;
struct dma_fence *fence;
+ uint64_t *points;
signed long ret;
uint32_t signaled_count, i;
- entries = kcalloc(count, sizeof(*entries), GFP_KERNEL);
- if (!entries)
+ points = kmalloc_array(count, sizeof(*points), GFP_KERNEL);
+ if (points == NULL)
return -ENOMEM;
+ if (!user_points) {
+ memset(points, 0, count * sizeof(uint64_t));
+ } else if (copy_from_user(points, user_points, sizeof(uint64_t) * count)) {
+ ret = -EFAULT;
+ goto err_free_points;
+ }
+
+
+ entries = kcalloc(count, sizeof(*entries), GFP_KERNEL);
+ if (!entries) {
+ ret = -ENOMEM;
+ goto err_free_points;
+ }
/* Walk the list of sync objects and initialize entries. We do
* this up-front so that we can properly return -EINVAL if there is
* a syncobj with a missing fence and then never have the chance of
@@ -911,7 +930,8 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
signaled_count = 0;
for (i = 0; i < count; ++i) {
entries[i].task = current;
- ret = drm_syncobj_search_fence(syncobjs[i], 0, 0,
+ entries[i].point = points[i];
+ ret = drm_syncobj_search_fence(syncobjs[i], points[i], 0,
&entries[i].fence);
if (!entries[i].fence) {
if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
@@ -950,6 +970,7 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
for (i = 0; i < count; ++i) {
drm_syncobj_fence_get_or_add_callback(syncobjs[i],
+ entries[i].point,
&entries[i].fence,
&entries[i].syncobj_cb,
syncobj_wait_syncobj_func);
@@ -1013,6 +1034,9 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
}
kfree(entries);
+err_free_points:
+ kfree(points);
+
return ret;
}
@@ -1051,20 +1075,33 @@ static signed long drm_timeout_abs_to_jiffies(int64_t timeout_nsec)
static int drm_syncobj_array_wait(struct drm_device *dev,
struct drm_file *file_private,
struct drm_syncobj_wait *wait,
- struct drm_syncobj **syncobjs)
+ struct drm_syncobj_timeline_wait *timeline_wait,
+ struct drm_syncobj **syncobjs, bool timeline)
{
- signed long timeout = drm_timeout_abs_to_jiffies(wait->timeout_nsec);
+ signed long timeout = 0;
signed long ret = 0;
uint32_t first = ~0;
- ret = drm_syncobj_array_wait_timeout(syncobjs,
- wait->count_handles,
- wait->flags,
- timeout, &first);
+ if (!timeline) {
+ timeout = drm_timeout_abs_to_jiffies(wait->timeout_nsec);
+ ret = drm_syncobj_array_wait_timeout(syncobjs,
+ NULL,
+ wait->count_handles,
+ wait->flags,
+ timeout, &first);
+ wait->first_signaled = first;
+ } else {
+ timeout = drm_timeout_abs_to_jiffies(timeline_wait->timeout_nsec);
+ ret = drm_syncobj_array_wait_timeout(syncobjs,
+ u64_to_user_ptr(timeline_wait->points),
+ timeline_wait->count_handles,
+ timeline_wait->flags,
+ timeout, &first);
+ timeline_wait->first_signaled = first;
+ }
if (ret < 0)
return ret;
- wait->first_signaled = first;
if (ret == 0)
return -ETIME;
return 0;
@@ -1152,13 +1189,47 @@ drm_syncobj_wait_ioctl(struct drm_device *dev, void *data,
return ret;
ret = drm_syncobj_array_wait(dev, file_private,
- args, syncobjs);
+ args, NULL, syncobjs, false);
drm_syncobj_array_free(syncobjs, args->count_handles);
return ret;
}
+int
+drm_syncobj_timeline_wait_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_private)
+{
+ struct drm_syncobj_timeline_wait *args = data;
+ struct drm_syncobj **syncobjs;
+ int ret = 0;
+
+ if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+ return -ENODEV;
+
+ if (args->flags & ~(DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL |
+ DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT))
+ return -EINVAL;
+
+ if (args->count_handles == 0)
+ return -EINVAL;
+
+ ret = drm_syncobj_array_find(file_private,
+ u64_to_user_ptr(args->handles),
+ args->count_handles,
+ &syncobjs);
+ if (ret < 0)
+ return ret;
+
+ ret = drm_syncobj_array_wait(dev, file_private,
+ NULL, args, syncobjs, true);
+
+ drm_syncobj_array_free(syncobjs, args->count_handles);
+
+ return ret;
+}
+
+
int
drm_syncobj_reset_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_private)
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index cebdb2541eb7..501e86d81f47 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -748,6 +748,19 @@ struct drm_syncobj_wait {
__u32 pad;
};
+struct drm_syncobj_timeline_wait {
+ __u64 handles;
+ /* wait on specific timeline point for every handles*/
+ __u64 points;
+ /* absolute timeout */
+ __s64 timeout_nsec;
+ __u32 count_handles;
+ __u32 flags;
+ __u32 first_signaled; /* only valid when not waiting all */
+ __u32 pad;
+};
+
+
struct drm_syncobj_array {
__u64 handles;
__u32 count_handles;
@@ -910,6 +923,7 @@ extern "C" {
#define DRM_IOCTL_MODE_GET_LEASE DRM_IOWR(0xC8, struct drm_mode_get_lease)
#define DRM_IOCTL_MODE_REVOKE_LEASE DRM_IOWR(0xC9, struct drm_mode_revoke_lease)
+#define DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT DRM_IOWR(0xCA, struct drm_syncobj_timeline_wait)
/**
* Device specific ioctls should only be in their respective headers
* The device specific ioctl range is from 0x40 to 0x9f.
--
2.17.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/4] drm: add timeline syncobj payload query ioctl
[not found] ` <20180919042659.3769-1-david1.zhou-5C7GfCeVMHo@public.gmane.org>
2018-09-19 4:26 ` [PATCH 2/4] drm: add support of syncobj timeline point wait v2 Chunming Zhou
@ 2018-09-19 4:26 ` Chunming Zhou
2018-09-19 4:26 ` [PATCH 4/4] drm/amdgpu: add timeline support in amdgpu CS Chunming Zhou
2018-09-19 7:18 ` [PATCH 1/4] [RFC]drm: add syncobj timeline support v6 Christian König
3 siblings, 0 replies; 11+ messages in thread
From: Chunming Zhou @ 2018-09-19 4:26 UTC (permalink / raw)
To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Cc: Chunming Zhou, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
user mode can query timeline payload.
Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
---
drivers/gpu/drm/drm_internal.h | 2 ++
drivers/gpu/drm/drm_ioctl.c | 2 ++
drivers/gpu/drm/drm_syncobj.c | 53 ++++++++++++++++++++++++++++++++++
include/uapi/drm/drm.h | 11 +++++++
4 files changed, 68 insertions(+)
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 566d44e3c782..9c4826411a3c 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -189,6 +189,8 @@ int drm_syncobj_reset_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_private);
int drm_syncobj_signal_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_private);
+int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_private);
/* drm_framebuffer.c */
void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index c0891614f516..c3c0617e6372 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -675,6 +675,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
DRM_UNLOCKED|DRM_RENDER_ALLOW),
DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_SIGNAL, drm_syncobj_signal_ioctl,
DRM_UNLOCKED|DRM_RENDER_ALLOW),
+ DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_QUERY, drm_syncobj_query_ioctl,
+ DRM_UNLOCKED|DRM_RENDER_ALLOW),
DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl, DRM_UNLOCKED),
DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, drm_crtc_queue_sequence_ioctl, DRM_UNLOCKED),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl, DRM_MASTER|DRM_UNLOCKED),
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 311ea7ba6f57..51aec09cb18a 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -1305,3 +1305,56 @@ drm_syncobj_signal_ioctl(struct drm_device *dev, void *data,
return ret;
}
+
+static void drm_syncobj_timeline_query_payload(struct drm_syncobj *syncobj,
+ uint64_t *point)
+{
+ if (syncobj->type != DRM_SYNCOBJ_TYPE_TIMELINE) {
+ DRM_ERROR("Normal syncobj cann't be queried!");
+ *point = 0;
+ return;
+ }
+ *point = syncobj->signal_point;
+}
+
+int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_private)
+{
+ struct drm_syncobj_timeline_query *args = data;
+ struct drm_syncobj **syncobjs;
+ uint64_t *points;
+ uint32_t i;
+ int ret;
+
+ if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+ return -ENODEV;
+
+ if (args->count_handles == 0)
+ return -EINVAL;
+
+ ret = drm_syncobj_array_find(file_private,
+ u64_to_user_ptr(args->handles),
+ args->count_handles,
+ &syncobjs);
+ if (ret < 0)
+ return ret;
+
+
+ points = kmalloc_array(args->count_handles, sizeof(*points),
+ GFP_KERNEL);
+ if (points == NULL) {
+ ret = -ENOMEM;
+ goto out;
+ }
+ for (i = 0; i < args->count_handles; i++) {
+ drm_syncobj_timeline_query_payload(syncobjs[i], &points[i]);
+ copy_to_user(u64_to_user_ptr(args->points), points,
+ sizeof(uint64_t) * args->count_handles);
+ }
+
+ kfree(points);
+out:
+ drm_syncobj_array_free(syncobjs, args->count_handles);
+
+ return ret;
+}
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 501e86d81f47..188b63f1975b 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -767,6 +767,15 @@ struct drm_syncobj_array {
__u32 pad;
};
+struct drm_syncobj_timeline_query {
+ __u64 handles;
+ /* points are timeline syncobjs payloads returned by query ioctl */
+ __u64 points;
+ __u32 count_handles;
+ __u32 pad;
+};
+
+
/* Query current scanout sequence number */
struct drm_crtc_get_sequence {
__u32 crtc_id; /* requested crtc_id */
@@ -924,6 +933,8 @@ extern "C" {
#define DRM_IOCTL_MODE_REVOKE_LEASE DRM_IOWR(0xC9, struct drm_mode_revoke_lease)
#define DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT DRM_IOWR(0xCA, struct drm_syncobj_timeline_wait)
+#define DRM_IOCTL_SYNCOBJ_QUERY DRM_IOWR(0xCB, struct drm_syncobj_timeline_query)
+
/**
* Device specific ioctls should only be in their respective headers
* The device specific ioctl range is from 0x40 to 0x9f.
--
2.17.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 4/4] drm/amdgpu: add timeline support in amdgpu CS
[not found] ` <20180919042659.3769-1-david1.zhou-5C7GfCeVMHo@public.gmane.org>
2018-09-19 4:26 ` [PATCH 2/4] drm: add support of syncobj timeline point wait v2 Chunming Zhou
2018-09-19 4:26 ` [PATCH 3/4] drm: add timeline syncobj payload query ioctl Chunming Zhou
@ 2018-09-19 4:26 ` Chunming Zhou
2018-09-19 7:18 ` [PATCH 1/4] [RFC]drm: add syncobj timeline support v6 Christian König
3 siblings, 0 replies; 11+ messages in thread
From: Chunming Zhou @ 2018-09-19 4:26 UTC (permalink / raw)
To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Cc: Chunming Zhou, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 8 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 111 +++++++++++++++++++------
include/uapi/drm/amdgpu_drm.h | 9 ++
3 files changed, 100 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 447c4c7a36d6..6e4a3db56833 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -975,6 +975,11 @@ struct amdgpu_cs_chunk {
void *kdata;
};
+struct amdgpu_cs_syncobj_post_dep {
+ struct drm_syncobj *post_dep_syncobj;
+ u64 point;
+};
+
struct amdgpu_cs_parser {
struct amdgpu_device *adev;
struct drm_file *filp;
@@ -1003,9 +1008,8 @@ struct amdgpu_cs_parser {
/* user fence */
struct amdgpu_bo_list_entry uf_entry;
-
+ struct amdgpu_cs_syncobj_post_dep *post_dep_syncobjs;
unsigned num_post_dep_syncobjs;
- struct drm_syncobj **post_dep_syncobjs;
};
static inline u32 amdgpu_get_ib_value(struct amdgpu_cs_parser *p,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index d9d2ede96490..aa829c6ef08e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -204,6 +204,8 @@ static int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, union drm_amdgpu_cs
case AMDGPU_CHUNK_ID_DEPENDENCIES:
case AMDGPU_CHUNK_ID_SYNCOBJ_IN:
case AMDGPU_CHUNK_ID_SYNCOBJ_OUT:
+ case AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_WAIT:
+ case AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_SIGNAL:
break;
default:
@@ -783,7 +785,7 @@ static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser, int error,
&parser->validated);
for (i = 0; i < parser->num_post_dep_syncobjs; i++)
- drm_syncobj_put(parser->post_dep_syncobjs[i]);
+ drm_syncobj_put(parser->post_dep_syncobjs[i].post_dep_syncobj);
kfree(parser->post_dep_syncobjs);
dma_fence_put(parser->fence);
@@ -1098,11 +1100,11 @@ static int amdgpu_cs_process_fence_dep(struct amdgpu_cs_parser *p,
}
static int amdgpu_syncobj_lookup_and_add_to_sync(struct amdgpu_cs_parser *p,
- uint32_t handle)
+ uint32_t handle, u64 point)
{
int r;
struct dma_fence *fence;
- r = drm_syncobj_find_fence(p->filp, handle, 0, &fence);
+ r = drm_syncobj_find_fence(p->filp, handle, point, &fence);
if (r)
return r;
@@ -1113,48 +1115,90 @@ static int amdgpu_syncobj_lookup_and_add_to_sync(struct amdgpu_cs_parser *p,
}
static int amdgpu_cs_process_syncobj_in_dep(struct amdgpu_cs_parser *p,
- struct amdgpu_cs_chunk *chunk)
+ struct amdgpu_cs_chunk *chunk,
+ bool timeline)
{
unsigned num_deps;
int i, r;
- struct drm_amdgpu_cs_chunk_sem *deps;
- deps = (struct drm_amdgpu_cs_chunk_sem *)chunk->kdata;
- num_deps = chunk->length_dw * 4 /
- sizeof(struct drm_amdgpu_cs_chunk_sem);
+ if (!timeline) {
+ struct drm_amdgpu_cs_chunk_sem *deps;
- for (i = 0; i < num_deps; ++i) {
- r = amdgpu_syncobj_lookup_and_add_to_sync(p, deps[i].handle);
+ deps = (struct drm_amdgpu_cs_chunk_sem *)chunk->kdata;
+ num_deps = chunk->length_dw * 4 /
+ sizeof(struct drm_amdgpu_cs_chunk_sem);
+ for (i = 0; i < num_deps; ++i) {
+ r = amdgpu_syncobj_lookup_and_add_to_sync(p, deps[i].handle,
+ 0);
if (r)
return r;
+ }
+ } else {
+ struct drm_amdgpu_cs_chunk_syncobj *syncobj_deps;
+
+ syncobj_deps = (struct drm_amdgpu_cs_chunk_syncobj *)chunk->kdata;
+ num_deps = chunk->length_dw * 4 /
+ sizeof(struct drm_amdgpu_cs_chunk_syncobj);
+ for (i = 0; i < num_deps; ++i) {
+ r = amdgpu_syncobj_lookup_and_add_to_sync(p, syncobj_deps[i].handle,
+ syncobj_deps[i].point);
+ if (r)
+ return r;
+ }
}
+
return 0;
}
static int amdgpu_cs_process_syncobj_out_dep(struct amdgpu_cs_parser *p,
- struct amdgpu_cs_chunk *chunk)
+ struct amdgpu_cs_chunk *chunk,
+ bool timeline)
{
unsigned num_deps;
int i;
struct drm_amdgpu_cs_chunk_sem *deps;
- deps = (struct drm_amdgpu_cs_chunk_sem *)chunk->kdata;
- num_deps = chunk->length_dw * 4 /
- sizeof(struct drm_amdgpu_cs_chunk_sem);
+ struct drm_amdgpu_cs_chunk_syncobj *syncobj_deps;
+
+ if (!timeline) {
+ deps = (struct drm_amdgpu_cs_chunk_sem *)chunk->kdata;
+ num_deps = chunk->length_dw * 4 /
+ sizeof(struct drm_amdgpu_cs_chunk_sem);
+ } else {
+ syncobj_deps = (struct drm_amdgpu_cs_chunk_syncobj *)chunk->kdata;
+ num_deps = chunk->length_dw * 4 /
+ sizeof(struct drm_amdgpu_cs_chunk_syncobj);
+ }
+
p->post_dep_syncobjs = kmalloc_array(num_deps,
- sizeof(struct drm_syncobj *),
+ sizeof(struct amdgpu_cs_syncobj_post_dep),
GFP_KERNEL);
p->num_post_dep_syncobjs = 0;
if (!p->post_dep_syncobjs)
return -ENOMEM;
- for (i = 0; i < num_deps; ++i) {
- p->post_dep_syncobjs[i] = drm_syncobj_find(p->filp, deps[i].handle);
- if (!p->post_dep_syncobjs[i])
- return -EINVAL;
- p->num_post_dep_syncobjs++;
+
+ if (!timeline) {
+ for (i = 0; i < num_deps; ++i) {
+ p->post_dep_syncobjs[i].post_dep_syncobj =
+ drm_syncobj_find(p->filp, deps[i].handle);
+ if (!p->post_dep_syncobjs[i].post_dep_syncobj)
+ return -EINVAL;
+ p->post_dep_syncobjs[i].point = 0;
+ p->num_post_dep_syncobjs++;
+ }
+ } else {
+ for (i = 0; i < num_deps; ++i) {
+ p->post_dep_syncobjs[i].post_dep_syncobj =
+ drm_syncobj_find(p->filp, syncobj_deps[i].handle);
+ if (!p->post_dep_syncobjs[i].post_dep_syncobj)
+ return -EINVAL;
+ p->post_dep_syncobjs[i].point = syncobj_deps[i].point;
+ p->num_post_dep_syncobjs++;
+ }
}
+
return 0;
}
@@ -1168,18 +1212,32 @@ static int amdgpu_cs_dependencies(struct amdgpu_device *adev,
chunk = &p->chunks[i];
- if (chunk->chunk_id == AMDGPU_CHUNK_ID_DEPENDENCIES) {
+ switch (chunk->chunk_id) {
+ case AMDGPU_CHUNK_ID_DEPENDENCIES:
r = amdgpu_cs_process_fence_dep(p, chunk);
if (r)
return r;
- } else if (chunk->chunk_id == AMDGPU_CHUNK_ID_SYNCOBJ_IN) {
- r = amdgpu_cs_process_syncobj_in_dep(p, chunk);
+ break;
+ case AMDGPU_CHUNK_ID_SYNCOBJ_IN:
+ r = amdgpu_cs_process_syncobj_in_dep(p, chunk, false);
if (r)
return r;
- } else if (chunk->chunk_id == AMDGPU_CHUNK_ID_SYNCOBJ_OUT) {
- r = amdgpu_cs_process_syncobj_out_dep(p, chunk);
+ break;
+ case AMDGPU_CHUNK_ID_SYNCOBJ_OUT:
+ r = amdgpu_cs_process_syncobj_out_dep(p, chunk, false);
+ if (r)
+ return r;
+ break;
+ case AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_WAIT:
+ r = amdgpu_cs_process_syncobj_in_dep(p, chunk, true);
if (r)
return r;
+ break;
+ case AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_SIGNAL:
+ r = amdgpu_cs_process_syncobj_out_dep(p, chunk, true);
+ if (r)
+ return r;
+ break;
}
}
@@ -1191,7 +1249,8 @@ static void amdgpu_cs_post_dependencies(struct amdgpu_cs_parser *p)
int i;
for (i = 0; i < p->num_post_dep_syncobjs; ++i)
- drm_syncobj_replace_fence(p->post_dep_syncobjs[i], 0, p->fence);
+ drm_syncobj_replace_fence(p->post_dep_syncobjs[i].post_dep_syncobj,
+ p->post_dep_syncobjs[i].point, p->fence);
}
static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
index 1ceec56de015..0f3eecda676a 100644
--- a/include/uapi/drm/amdgpu_drm.h
+++ b/include/uapi/drm/amdgpu_drm.h
@@ -517,6 +517,8 @@ struct drm_amdgpu_gem_va {
#define AMDGPU_CHUNK_ID_SYNCOBJ_IN 0x04
#define AMDGPU_CHUNK_ID_SYNCOBJ_OUT 0x05
#define AMDGPU_CHUNK_ID_BO_HANDLES 0x06
+#define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_WAIT 0x07
+#define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_SIGNAL 0x08
struct drm_amdgpu_cs_chunk {
__u32 chunk_id;
@@ -592,6 +594,13 @@ struct drm_amdgpu_cs_chunk_sem {
__u32 handle;
};
+struct drm_amdgpu_cs_chunk_syncobj {
+ __u32 handle;
+ __u32 pad;
+ __u64 point;
+};
+
+
#define AMDGPU_FENCE_TO_HANDLE_GET_SYNCOBJ 0
#define AMDGPU_FENCE_TO_HANDLE_GET_SYNCOBJ_FD 1
#define AMDGPU_FENCE_TO_HANDLE_GET_SYNC_FILE_FD 2
--
2.17.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] [RFC]drm: add syncobj timeline support v6
[not found] ` <20180919042659.3769-1-david1.zhou-5C7GfCeVMHo@public.gmane.org>
` (2 preceding siblings ...)
2018-09-19 4:26 ` [PATCH 4/4] drm/amdgpu: add timeline support in amdgpu CS Chunming Zhou
@ 2018-09-19 7:18 ` Christian König
[not found] ` <04e91247-387c-ed5d-0793-9eb1d37d12bd-5C7GfCeVMHo@public.gmane.org>
3 siblings, 1 reply; 11+ messages in thread
From: Christian König @ 2018-09-19 7:18 UTC (permalink / raw)
To: Chunming Zhou, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Cc: Dave Airlie, Daniel Rakos, Daniel Vetter,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Am 19.09.2018 um 06:26 schrieb Chunming Zhou:
> 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
>
> normal syncobj is tested by ./deqp-vk -n dEQP-VK*semaphore*
> timeline syncobj is tested by ./amdgpu_test -s 9
>
> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
> Cc: Christian Konig <christian.koenig@amd.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Daniel Rakos <Daniel.Rakos@amd.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
> drivers/gpu/drm/drm_syncobj.c | 307 ++++++++++++++++++---
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
> include/drm/drm_syncobj.h | 65 ++---
> include/uapi/drm/drm.h | 1 +
> 4 files changed, 299 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index e9ce623d049e..987b5a120b65 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -56,6 +56,9 @@
> #include "drm_internal.h"
> #include <drm/drm_syncobj.h>
>
> +/* 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;
> + u64 value;
> + struct list_head list;
> +};
>
> /**
> * drm_syncobj_find - lookup and reference a sync object.
> @@ -117,6 +125,9 @@ static void drm_syncobj_add_callback_locked(struct drm_syncobj *syncobj,
> list_add_tail(&cb->node, &syncobj->cb_list);
> }
>
> +static int drm_syncobj_search_fence(struct drm_syncobj *syncobj, u64 point,
> + u64 flags, struct dma_fence **fence);
> +
> static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj,
> struct dma_fence **fence,
> struct drm_syncobj_cb *cb,
> @@ -124,8 +135,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 = drm_syncobj_search_fence(syncobj, 0, 0, fence);
> + if (!ret)
> return 1;
>
> spin_lock(&syncobj->lock);
> @@ -133,10 +144,12 @@ static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj,
> * have the lock, try one more time just to be sure we don't add a
> * callback when a fence has already been set.
> */
> - if (syncobj->fence) {
> - *fence = dma_fence_get(rcu_dereference_protected(syncobj->fence,
> - lockdep_is_held(&syncobj->lock)));
> - ret = 1;
> + if (fence && !list_empty(&syncobj->signal_pt_list)) {
I think the "fence" condition is superfluous here, if the fence pointer
isn't provided we would have already crashed before.
> + spin_unlock(&syncobj->lock);
> + drm_syncobj_search_fence(syncobj, 0, 0, fence);
> + if (*fence)
> + ret = 1;
> + spin_lock(&syncobj->lock);
Can't we just use "return 1;" here?
> } else {
> *fence = NULL;
> drm_syncobj_add_callback_locked(syncobj, cb, func);
> @@ -164,6 +177,153 @@ void drm_syncobj_remove_callback(struct drm_syncobj *syncobj,
> spin_unlock(&syncobj->lock);
> }
>
> +static void drm_syncobj_timeline_init(struct drm_syncobj *syncobj)
We still have _timeline_ in the name here.
> +{
> + spin_lock(&syncobj->lock);
> + syncobj->timeline_context = dma_fence_context_alloc(1);
> + syncobj->timeline = 0;
> + syncobj->signal_point = 0;
> + init_waitqueue_head(&syncobj->wq);
> +
> + INIT_LIST_HEAD(&syncobj->signal_pt_list);
> + spin_unlock(&syncobj->lock);
> +}
> +
> +static void drm_syncobj_timeline_fini(struct drm_syncobj *syncobj)
> +{
> + struct drm_syncobj_signal_pt *signal_pt = NULL, *tmp;
> +
> + spin_lock(&syncobj->lock);
> + list_for_each_entry_safe(signal_pt, tmp,
> + &syncobj->signal_pt_list, list) {
> + list_del(&signal_pt->list);
> + dma_fence_put(&signal_pt->base->base);
> + kfree(signal_pt);
> + }
> + spin_unlock(&syncobj->lock);
> +}
> +
> +static struct dma_fence
> +*drm_syncobj_find_signal_pt_for_point(struct drm_syncobj *syncobj,
> + uint64_t point)
> +{
> + struct drm_syncobj_signal_pt *signal_pt;
> +
> + if ((syncobj->type == DRM_SYNCOBJ_TYPE_TIMELINE) &&
> + (point <= syncobj->timeline)) {
> + struct drm_syncobj_stub_fence *fence =
> + kzalloc(sizeof(struct drm_syncobj_stub_fence),
> + GFP_KERNEL);
> +
> + if (!fence)
> + return NULL;
> + spin_lock_init(&fence->lock);
> + dma_fence_init(&fence->base,
> + &drm_syncobj_stub_fence_ops,
> + &fence->lock,
> + syncobj->timeline_context,
> + point);
> +
> + dma_fence_signal(&fence->base);
> + return &fence->base;
> + }
> +
> + list_for_each_entry(signal_pt, &syncobj->signal_pt_list, list) {
> + if (point > signal_pt->value)
> + continue;
> + if ((syncobj->type == DRM_SYNCOBJ_TYPE_INDIVIDUAL) &&
> + (point != signal_pt->value))
> + continue;
> + return dma_fence_get(&signal_pt->base->base);
> + }
> + return NULL;
> +}
> +
> +static int drm_syncobj_create_signal_pt(struct drm_syncobj *syncobj,
> + struct dma_fence *fence,
> + u64 point)
> +{
> + struct drm_syncobj_signal_pt *signal_pt =
> + kzalloc(sizeof(struct drm_syncobj_signal_pt), GFP_KERNEL);
> + struct drm_syncobj_signal_pt *tail_pt;
> + struct dma_fence **fences;
> + int num_fences = 0;
> + int ret = 0, i;
> +
> + if (!signal_pt)
> + return -ENOMEM;
> + if (syncobj->signal_point >= point) {
> + DRM_WARN("A later signal is ready!");
> + goto out;
> + }
> + if (!fence)
> + goto out;
> +
> + fences = kmalloc_array(sizeof(void *), 2, GFP_KERNEL);
> + if (!fences)
> + goto out;
> + fences[num_fences++] = dma_fence_get(fence);
> + /* timeline syncobj must take this dependency */
> + if (syncobj->type == DRM_SYNCOBJ_TYPE_TIMELINE) {
> + spin_lock(&syncobj->lock);
> + if (!list_empty(&syncobj->signal_pt_list)) {
> + tail_pt = list_last_entry(&syncobj->signal_pt_list,
> + struct drm_syncobj_signal_pt, list);
> + fences[num_fences++] = dma_fence_get(&tail_pt->base->base);
> + }
> + spin_unlock(&syncobj->lock);
> + }
> + signal_pt->base = dma_fence_array_create(num_fences, fences,
> + syncobj->timeline_context,
> + point, false);
> + if (!signal_pt->base)
> + goto fail;
ret need to be set to an error code here.
> +
> + spin_lock(&syncobj->lock);
Mhm, what happens if the functions is called by two threads at the same
time?
I think what we need to do is move the "if (syncobj->signal_point >=
point) {" here and double check if the previous fence is still the same.
> + signal_pt->value = point;
> + INIT_LIST_HEAD(&signal_pt->list);
That is superfluous, will be overwritten by the following list_add_tail
anyway.
> + list_add_tail(&signal_pt->list, &syncobj->signal_pt_list);
> + syncobj->signal_point = point;
> + spin_unlock(&syncobj->lock);
> + wake_up_all(&syncobj->wq);
> +
> + return 0;
> +fail:
> + for (i = 0; i < num_fences; i++)
> + dma_fence_put(fences[i]);
> + kfree(fences);
> +out:
> + kfree(signal_pt);
> + return ret;
> +}
> +
> +static void drm_syncobj_garbage_collection(struct drm_syncobj *syncobj)
> +{
> + struct drm_syncobj_signal_pt *signal_pt, *tmp, *tail_pt;
> +
> + spin_lock(&syncobj->lock);
> + tail_pt = list_last_entry(&syncobj->signal_pt_list,
> + struct drm_syncobj_signal_pt,
> + list);
> + list_for_each_entry_safe(signal_pt, tmp,
> + &syncobj->signal_pt_list, list) {
> + if (syncobj->type == DRM_SYNCOBJ_TYPE_INDIVIDUAL &&
> + signal_pt == tail_pt)
> + continue;
> + if (dma_fence_is_signaled(&signal_pt->base->base)) {
> + syncobj->timeline = signal_pt->value;
> + list_del(&signal_pt->list);
> + dma_fence_put(&signal_pt->base->base);
> + kfree(signal_pt);
> + } else {
> + /*signal_pt is in order in list, from small to big, so
> + * the later must not be signal either */
> + break;
> + }
> + }
> +
> + spin_unlock(&syncobj->lock);
> +}
> /**
> * drm_syncobj_replace_fence - replace fence in a sync object.
> * @syncobj: Sync object to replace fence in
> @@ -176,28 +336,29 @@ void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
> u64 point,
> struct dma_fence *fence)
> {
> - struct dma_fence *old_fence;
> - struct drm_syncobj_cb *cur, *tmp;
> -
> - if (fence)
> - dma_fence_get(fence);
> -
> - spin_lock(&syncobj->lock);
> -
> - old_fence = rcu_dereference_protected(syncobj->fence,
> - lockdep_is_held(&syncobj->lock));
> - rcu_assign_pointer(syncobj->fence, fence);
> + u64 pt_value = point;
> +
> + drm_syncobj_garbage_collection(syncobj);
> + if (syncobj->type == DRM_SYNCOBJ_TYPE_INDIVIDUAL) {
> + if (!fence) {
> + drm_syncobj_timeline_fini(syncobj);
> + drm_syncobj_timeline_init(syncobj);
> + return;
> + }
> + pt_value = syncobj->signal_point +
> + DRM_SYNCOBJ_INDIVIDUAL_POINT;
> + }
> + drm_syncobj_create_signal_pt(syncobj, fence, pt_value);
> + if (fence) {
> + struct drm_syncobj_cb *cur, *tmp;
>
> - if (fence != old_fence) {
> + spin_lock(&syncobj->lock);
> list_for_each_entry_safe(cur, tmp, &syncobj->cb_list, node) {
> list_del_init(&cur->node);
> cur->func(syncobj, cur);
> }
> + spin_unlock(&syncobj->lock);
> }
> -
> - spin_unlock(&syncobj->lock);
> -
> - dma_fence_put(old_fence);
> }
> EXPORT_SYMBOL(drm_syncobj_replace_fence);
>
> @@ -220,6 +381,60 @@ static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj)
> return 0;
> }
>
> +#define WAIT_FOR_SUBMIT_MSECS 10000
> +
> +static struct dma_fence *
> +drm_syncobj_point_get(struct drm_syncobj *syncobj, u64 point, u64 flags)
> +{
> + struct dma_fence *fence;
> + int ret = 0;
> +
> + if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
> + ret = wait_event_timeout(syncobj->wq,
> + point <= syncobj->signal_point,
> + msecs_to_jiffies(WAIT_FOR_SUBMIT_MSECS));
> + if (ret <= 0)
> + return NULL;
> + }
> + spin_lock(&syncobj->lock);
> + fence = drm_syncobj_find_signal_pt_for_point(syncobj, point);
> + spin_unlock(&syncobj->lock);
> + return fence;
> +}
> +
> +static int drm_syncobj_search_fence(struct drm_syncobj *syncobj, u64 point,
> + u64 flags, struct dma_fence **fence)
> +{
> + u64 pt_value = point;
> + int ret = 0;
> +
> + if (!syncobj)
> + return -ENOENT;
> +
> + drm_syncobj_garbage_collection(syncobj);
> + if (syncobj->type == DRM_SYNCOBJ_TYPE_INDIVIDUAL) {
> + /*INDIVIDUAL syncobj always wait on last pt */
> + pt_value = syncobj->signal_point;
> +
> + if (pt_value == 0)
> + pt_value += DRM_SYNCOBJ_INDIVIDUAL_POINT;
> + }
> + *fence = drm_syncobj_point_get(syncobj, pt_value, flags);
> + if (!*fence) {
> + ret = -EINVAL;
> + }
> + return ret;
> +}
> +
> +int drm_syncobj_lookup_fence(struct drm_syncobj *syncobj, u64 point,
> + struct dma_fence **fence) {
> +
> + return drm_syncobj_search_fence(syncobj, point,
> + DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT,
I still have a bad feeling setting that flag as default cause it might
change the behavior for the UAPI.
Maybe export drm_syncobj_search_fence directly? E.g. with the flags
parameter.
Regards,
Christian.
> + fence);
> +}
> +EXPORT_SYMBOL(drm_syncobj_lookup_fence);
> +
> /**
> * drm_syncobj_find_fence - lookup and reference the fence in a sync object
> * @file_private: drm file private pointer
> @@ -228,7 +443,7 @@ static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj)
> * @fence: out parameter for the fence
> *
> * This is just a convenience function that combines drm_syncobj_find() and
> - * drm_syncobj_fence_get().
> + * drm_syncobj_lookup_fence().
> *
> * Returns 0 on success or a negative error value on failure. On success @fence
> * contains a reference to the fence, which must be released by calling
> @@ -236,18 +451,11 @@ static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj)
> */
> int drm_syncobj_find_fence(struct drm_file *file_private,
> u32 handle, u64 point,
> - struct dma_fence **fence)
> -{
> + struct dma_fence **fence) {
> struct drm_syncobj *syncobj = drm_syncobj_find(file_private, handle);
> - int ret = 0;
> -
> - if (!syncobj)
> - return -ENOENT;
> + int ret;
>
> - *fence = drm_syncobj_fence_get(syncobj);
> - if (!*fence) {
> - ret = -EINVAL;
> - }
> + ret = drm_syncobj_lookup_fence(syncobj, point, fence);
> drm_syncobj_put(syncobj);
> return ret;
> }
> @@ -264,7 +472,7 @@ void drm_syncobj_free(struct kref *kref)
> struct drm_syncobj *syncobj = container_of(kref,
> struct drm_syncobj,
> refcount);
> - drm_syncobj_replace_fence(syncobj, 0, NULL);
> + drm_syncobj_timeline_fini(syncobj);
> kfree(syncobj);
> }
> EXPORT_SYMBOL(drm_syncobj_free);
> @@ -294,6 +502,11 @@ int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags,
> kref_init(&syncobj->refcount);
> INIT_LIST_HEAD(&syncobj->cb_list);
> spin_lock_init(&syncobj->lock);
> + if (flags & DRM_SYNCOBJ_CREATE_TYPE_TIMELINE)
> + syncobj->type = DRM_SYNCOBJ_TYPE_TIMELINE;
> + else
> + syncobj->type = DRM_SYNCOBJ_TYPE_INDIVIDUAL;
> + drm_syncobj_timeline_init(syncobj);
>
> if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) {
> ret = drm_syncobj_assign_null_handle(syncobj);
> @@ -576,7 +789,8 @@ drm_syncobj_create_ioctl(struct drm_device *dev, void *data,
> return -ENODEV;
>
> /* no valid flags yet */
> - if (args->flags & ~DRM_SYNCOBJ_CREATE_SIGNALED)
> + if (args->flags & ~(DRM_SYNCOBJ_CREATE_SIGNALED |
> + DRM_SYNCOBJ_CREATE_TYPE_TIMELINE))
> return -EINVAL;
>
> return drm_syncobj_create_as_handle(file_private,
> @@ -669,9 +883,8 @@ static void syncobj_wait_syncobj_func(struct drm_syncobj *syncobj,
> struct syncobj_wait_entry *wait =
> container_of(cb, struct syncobj_wait_entry, syncobj_cb);
>
> - /* This happens inside the syncobj lock */
> - wait->fence = dma_fence_get(rcu_dereference_protected(syncobj->fence,
> - lockdep_is_held(&syncobj->lock)));
> + drm_syncobj_search_fence(syncobj, 0, 0, &wait->fence);
> +
> wake_up_process(wait->task);
> }
>
> @@ -698,7 +911,8 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
> signaled_count = 0;
> for (i = 0; i < count; ++i) {
> entries[i].task = current;
> - entries[i].fence = drm_syncobj_fence_get(syncobjs[i]);
> + ret = drm_syncobj_search_fence(syncobjs[i], 0, 0,
> + &entries[i].fence);
> if (!entries[i].fence) {
> if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
> continue;
> @@ -970,12 +1184,19 @@ drm_syncobj_reset_ioctl(struct drm_device *dev, void *data,
> if (ret < 0)
> return ret;
>
> - for (i = 0; i < args->count_handles; i++)
> - drm_syncobj_replace_fence(syncobjs[i], 0, NULL);
> -
> + for (i = 0; i < args->count_handles; i++) {
> + if (syncobjs[i]->type == DRM_SYNCOBJ_TYPE_TIMELINE) {
> + DRM_ERROR("timeline syncobj cannot reset!\n");
> + ret = -EINVAL;
> + goto out;
> + }
> + drm_syncobj_timeline_fini(syncobjs[i]);
> + drm_syncobj_timeline_init(syncobjs[i]);
> + }
> +out:
> drm_syncobj_array_free(syncobjs, args->count_handles);
>
> - return 0;
> + return ret;
> }
>
> int
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 0a8d2d64f380..a040fa3281df 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -2137,7 +2137,7 @@ await_fence_array(struct i915_execbuffer *eb,
> if (!(flags & I915_EXEC_FENCE_WAIT))
> continue;
>
> - fence = drm_syncobj_fence_get(syncobj);
> + drm_syncobj_lookup_fence(syncobj, 0, &fence);
> if (!fence)
> return -EINVAL;
>
> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
> index 425432b85a87..6dac479fedfe 100644
> --- a/include/drm/drm_syncobj.h
> +++ b/include/drm/drm_syncobj.h
> @@ -30,6 +30,11 @@
>
> struct drm_syncobj_cb;
>
> +enum drm_syncobj_type {
> + DRM_SYNCOBJ_TYPE_INDIVIDUAL,
> + DRM_SYNCOBJ_TYPE_TIMELINE
> +};
> +
> /**
> * struct drm_syncobj - sync object.
> *
> @@ -41,19 +46,36 @@ struct drm_syncobj {
> */
> struct kref refcount;
> /**
> - * @fence:
> - * NULL or a pointer to the fence bound to this object.
> - *
> - * This field should not be used directly. Use drm_syncobj_fence_get()
> - * and drm_syncobj_replace_fence() instead.
> + * @type: indicate syncobj type
> + */
> + enum drm_syncobj_type type;
> + /**
> + * @wq: wait signal operation work queue
> + */
> + wait_queue_head_t wq;
> + /**
> + * @timeline_context: fence context used by timeline
> */
> - struct dma_fence __rcu *fence;
> + u64 timeline_context;
> /**
> - * @cb_list: List of callbacks to call when the &fence gets replaced.
> + * @timeline: syncobj timeline value, which indicates point is signaled.
> */
> + u64 timeline;
> + /**
> + * @signal_point: which indicates the latest signaler point.
> + */
> + u64 signal_point;
> + /**
> + * @signal_pt_list: signaler point list.
> + */
> + struct list_head signal_pt_list;
> +
> + /**
> + * @cb_list: List of callbacks to call when the &fence gets replaced.
> + */
> struct list_head cb_list;
> /**
> - * @lock: Protects &cb_list and write-locks &fence.
> + * @lock: Protects syncobj list and write-locks &fence.
> */
> spinlock_t lock;
> /**
> @@ -68,7 +90,7 @@ typedef void (*drm_syncobj_func_t)(struct drm_syncobj *syncobj,
> /**
> * struct drm_syncobj_cb - callback for drm_syncobj_add_callback
> * @node: used by drm_syncob_add_callback to append this struct to
> - * &drm_syncobj.cb_list
> + * &drm_syncobj.cb_list
> * @func: drm_syncobj_func_t to call
> *
> * This struct will be initialized by drm_syncobj_add_callback, additional
> @@ -106,29 +128,6 @@ drm_syncobj_put(struct drm_syncobj *obj)
> kref_put(&obj->refcount, drm_syncobj_free);
> }
>
> -/**
> - * drm_syncobj_fence_get - get a reference to a fence in a sync object
> - * @syncobj: sync object.
> - *
> - * This acquires additional reference to &drm_syncobj.fence contained in @obj,
> - * if not NULL. It is illegal to call this without already holding a reference.
> - * No locks required.
> - *
> - * Returns:
> - * Either the fence of @obj or NULL if there's none.
> - */
> -static inline struct dma_fence *
> -drm_syncobj_fence_get(struct drm_syncobj *syncobj)
> -{
> - struct dma_fence *fence;
> -
> - rcu_read_lock();
> - fence = dma_fence_get_rcu_safe(&syncobj->fence);
> - rcu_read_unlock();
> -
> - return fence;
> -}
> -
> struct drm_syncobj *drm_syncobj_find(struct drm_file *file_private,
> u32 handle);
> void drm_syncobj_replace_fence(struct drm_syncobj *syncobj, u64 point,
> @@ -142,5 +141,7 @@ int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags,
> int drm_syncobj_get_handle(struct drm_file *file_private,
> struct drm_syncobj *syncobj, u32 *handle);
> int drm_syncobj_get_fd(struct drm_syncobj *syncobj, int *p_fd);
> +int drm_syncobj_lookup_fence(struct drm_syncobj *syncobj, u64 point,
> + struct dma_fence **fence);
>
> #endif
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 300f336633f2..cebdb2541eb7 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -717,6 +717,7 @@ struct drm_prime_handle {
> struct drm_syncobj_create {
> __u32 handle;
> #define DRM_SYNCOBJ_CREATE_SIGNALED (1 << 0)
> +#define DRM_SYNCOBJ_CREATE_TYPE_TIMELINE (1 << 1)
> __u32 flags;
> };
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] [RFC]drm: add syncobj timeline support v6
[not found] ` <04e91247-387c-ed5d-0793-9eb1d37d12bd-5C7GfCeVMHo@public.gmane.org>
@ 2018-09-19 7:32 ` zhoucm1
2018-09-19 7:32 ` zhoucm1
1 sibling, 0 replies; 11+ messages in thread
From: zhoucm1 @ 2018-09-19 7:32 UTC (permalink / raw)
To: Christian König, Chunming Zhou,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Cc: Dave Airlie, Daniel Rakos, Daniel Vetter,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
On 2018年09月19日 15:18, Christian König wrote:
> Am 19.09.2018 um 06:26 schrieb Chunming Zhou:
[snip]
>> *fence = NULL;
>> drm_syncobj_add_callback_locked(syncobj, cb, func);
>> @@ -164,6 +177,153 @@ void drm_syncobj_remove_callback(struct
>> drm_syncobj *syncobj,
>> spin_unlock(&syncobj->lock);
>> }
>> +static void drm_syncobj_timeline_init(struct drm_syncobj *syncobj)
>
> We still have _timeline_ in the name here.
the func is relevant to timeline members, or which name is proper?
>
>> +{
>> + spin_lock(&syncobj->lock);
>> + syncobj->timeline_context = dma_fence_context_alloc(1);
[snip]
>> +}
>> +
>> +int drm_syncobj_lookup_fence(struct drm_syncobj *syncobj, u64 point,
>> + struct dma_fence **fence) {
>> +
>> + return drm_syncobj_search_fence(syncobj, point,
>> + DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT,
>
> I still have a bad feeling setting that flag as default cause it might
> change the behavior for the UAPI.
>
> Maybe export drm_syncobj_search_fence directly? E.g. with the flags
> parameter.
previous v5 indeed do this, you let me wrap it, need change back?
Regards,
David Zhou
>
> Regards,
> Christian.
>
>> + fence);
>> +}
>> +EXPORT_SYMBOL(drm_syncobj_lookup_fence);
>> +
>> /**
>> * drm_syncobj_find_fence - lookup and reference the fence in a
>> sync object
>> * @file_private: drm file private pointer
>> @@ -228,7 +443,7 @@ static int drm_syncobj_assign_null_handle(struct
>> drm_syncobj *syncobj)
>> * @fence: out parameter for the fence
>> *
>> * This is just a convenience function that combines
>> drm_syncobj_find() and
>> - * drm_syncobj_fence_get().
>> + * drm_syncobj_lookup_fence().
>> *
>> * Returns 0 on success or a negative error value on failure. On
>> success @fence
>> * contains a reference to the fence, which must be released by
>> calling
>> @@ -236,18 +451,11 @@ static int
>> drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj)
>> */
>> int drm_syncobj_find_fence(struct drm_file *file_private,
>> u32 handle, u64 point,
>> - struct dma_fence **fence)
>> -{
>> + struct dma_fence **fence) {
>> struct drm_syncobj *syncobj = drm_syncobj_find(file_private,
>> handle);
>> - int ret = 0;
>> -
>> - if (!syncobj)
>> - return -ENOENT;
>> + int ret;
>> - *fence = drm_syncobj_fence_get(syncobj);
>> - if (!*fence) {
>> - ret = -EINVAL;
>> - }
>> + ret = drm_syncobj_lookup_fence(syncobj, point, fence);
>> drm_syncobj_put(syncobj);
>> return ret;
>> }
>> @@ -264,7 +472,7 @@ void drm_syncobj_free(struct kref *kref)
>> struct drm_syncobj *syncobj = container_of(kref,
>> struct drm_syncobj,
>> refcount);
>> - drm_syncobj_replace_fence(syncobj, 0, NULL);
>> + drm_syncobj_timeline_fini(syncobj);
>> kfree(syncobj);
>> }
>> EXPORT_SYMBOL(drm_syncobj_free);
>> @@ -294,6 +502,11 @@ int drm_syncobj_create(struct drm_syncobj
>> **out_syncobj, uint32_t flags,
>> kref_init(&syncobj->refcount);
>> INIT_LIST_HEAD(&syncobj->cb_list);
>> spin_lock_init(&syncobj->lock);
>> + if (flags & DRM_SYNCOBJ_CREATE_TYPE_TIMELINE)
>> + syncobj->type = DRM_SYNCOBJ_TYPE_TIMELINE;
>> + else
>> + syncobj->type = DRM_SYNCOBJ_TYPE_INDIVIDUAL;
>> + drm_syncobj_timeline_init(syncobj);
>> if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) {
>> ret = drm_syncobj_assign_null_handle(syncobj);
>> @@ -576,7 +789,8 @@ drm_syncobj_create_ioctl(struct drm_device *dev,
>> void *data,
>> return -ENODEV;
>> /* no valid flags yet */
>> - if (args->flags & ~DRM_SYNCOBJ_CREATE_SIGNALED)
>> + if (args->flags & ~(DRM_SYNCOBJ_CREATE_SIGNALED |
>> + DRM_SYNCOBJ_CREATE_TYPE_TIMELINE))
>> return -EINVAL;
>> return drm_syncobj_create_as_handle(file_private,
>> @@ -669,9 +883,8 @@ static void syncobj_wait_syncobj_func(struct
>> drm_syncobj *syncobj,
>> struct syncobj_wait_entry *wait =
>> container_of(cb, struct syncobj_wait_entry, syncobj_cb);
>> - /* This happens inside the syncobj lock */
>> - wait->fence =
>> dma_fence_get(rcu_dereference_protected(syncobj->fence,
>> - lockdep_is_held(&syncobj->lock)));
>> + drm_syncobj_search_fence(syncobj, 0, 0, &wait->fence);
>> +
>> wake_up_process(wait->task);
>> }
>> @@ -698,7 +911,8 @@ static signed long
>> drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
>> signaled_count = 0;
>> for (i = 0; i < count; ++i) {
>> entries[i].task = current;
>> - entries[i].fence = drm_syncobj_fence_get(syncobjs[i]);
>> + ret = drm_syncobj_search_fence(syncobjs[i], 0, 0,
>> + &entries[i].fence);
>> if (!entries[i].fence) {
>> if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
>> continue;
>> @@ -970,12 +1184,19 @@ drm_syncobj_reset_ioctl(struct drm_device
>> *dev, void *data,
>> if (ret < 0)
>> return ret;
>> - for (i = 0; i < args->count_handles; i++)
>> - drm_syncobj_replace_fence(syncobjs[i], 0, NULL);
>> -
>> + for (i = 0; i < args->count_handles; i++) {
>> + if (syncobjs[i]->type == DRM_SYNCOBJ_TYPE_TIMELINE) {
>> + DRM_ERROR("timeline syncobj cannot reset!\n");
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> + drm_syncobj_timeline_fini(syncobjs[i]);
>> + drm_syncobj_timeline_init(syncobjs[i]);
>> + }
>> +out:
>> drm_syncobj_array_free(syncobjs, args->count_handles);
>> - return 0;
>> + return ret;
>> }
>> int
>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> index 0a8d2d64f380..a040fa3281df 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> @@ -2137,7 +2137,7 @@ await_fence_array(struct i915_execbuffer *eb,
>> if (!(flags & I915_EXEC_FENCE_WAIT))
>> continue;
>> - fence = drm_syncobj_fence_get(syncobj);
>> + drm_syncobj_lookup_fence(syncobj, 0, &fence);
>> if (!fence)
>> return -EINVAL;
>> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
>> index 425432b85a87..6dac479fedfe 100644
>> --- a/include/drm/drm_syncobj.h
>> +++ b/include/drm/drm_syncobj.h
>> @@ -30,6 +30,11 @@
>> struct drm_syncobj_cb;
>> +enum drm_syncobj_type {
>> + DRM_SYNCOBJ_TYPE_INDIVIDUAL,
>> + DRM_SYNCOBJ_TYPE_TIMELINE
>> +};
>> +
>> /**
>> * struct drm_syncobj - sync object.
>> *
>> @@ -41,19 +46,36 @@ struct drm_syncobj {
>> */
>> struct kref refcount;
>> /**
>> - * @fence:
>> - * NULL or a pointer to the fence bound to this object.
>> - *
>> - * This field should not be used directly. Use
>> drm_syncobj_fence_get()
>> - * and drm_syncobj_replace_fence() instead.
>> + * @type: indicate syncobj type
>> + */
>> + enum drm_syncobj_type type;
>> + /**
>> + * @wq: wait signal operation work queue
>> + */
>> + wait_queue_head_t wq;
>> + /**
>> + * @timeline_context: fence context used by timeline
>> */
>> - struct dma_fence __rcu *fence;
>> + u64 timeline_context;
>> /**
>> - * @cb_list: List of callbacks to call when the &fence gets
>> replaced.
>> + * @timeline: syncobj timeline value, which indicates point is
>> signaled.
>> */
>> + u64 timeline;
>> + /**
>> + * @signal_point: which indicates the latest signaler point.
>> + */
>> + u64 signal_point;
>> + /**
>> + * @signal_pt_list: signaler point list.
>> + */
>> + struct list_head signal_pt_list;
>> +
>> + /**
>> + * @cb_list: List of callbacks to call when the &fence gets
>> replaced.
>> + */
>> struct list_head cb_list;
>> /**
>> - * @lock: Protects &cb_list and write-locks &fence.
>> + * @lock: Protects syncobj list and write-locks &fence.
>> */
>> spinlock_t lock;
>> /**
>> @@ -68,7 +90,7 @@ typedef void (*drm_syncobj_func_t)(struct
>> drm_syncobj *syncobj,
>> /**
>> * struct drm_syncobj_cb - callback for drm_syncobj_add_callback
>> * @node: used by drm_syncob_add_callback to append this struct to
>> - * &drm_syncobj.cb_list
>> + * &drm_syncobj.cb_list
>> * @func: drm_syncobj_func_t to call
>> *
>> * This struct will be initialized by drm_syncobj_add_callback,
>> additional
>> @@ -106,29 +128,6 @@ drm_syncobj_put(struct drm_syncobj *obj)
>> kref_put(&obj->refcount, drm_syncobj_free);
>> }
>> -/**
>> - * drm_syncobj_fence_get - get a reference to a fence in a sync object
>> - * @syncobj: sync object.
>> - *
>> - * This acquires additional reference to &drm_syncobj.fence
>> contained in @obj,
>> - * if not NULL. It is illegal to call this without already holding a
>> reference.
>> - * No locks required.
>> - *
>> - * Returns:
>> - * Either the fence of @obj or NULL if there's none.
>> - */
>> -static inline struct dma_fence *
>> -drm_syncobj_fence_get(struct drm_syncobj *syncobj)
>> -{
>> - struct dma_fence *fence;
>> -
>> - rcu_read_lock();
>> - fence = dma_fence_get_rcu_safe(&syncobj->fence);
>> - rcu_read_unlock();
>> -
>> - return fence;
>> -}
>> -
>> struct drm_syncobj *drm_syncobj_find(struct drm_file *file_private,
>> u32 handle);
>> void drm_syncobj_replace_fence(struct drm_syncobj *syncobj, u64 point,
>> @@ -142,5 +141,7 @@ int drm_syncobj_create(struct drm_syncobj
>> **out_syncobj, uint32_t flags,
>> int drm_syncobj_get_handle(struct drm_file *file_private,
>> struct drm_syncobj *syncobj, u32 *handle);
>> int drm_syncobj_get_fd(struct drm_syncobj *syncobj, int *p_fd);
>> +int drm_syncobj_lookup_fence(struct drm_syncobj *syncobj, u64 point,
>> + struct dma_fence **fence);
>> #endif
>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>> index 300f336633f2..cebdb2541eb7 100644
>> --- a/include/uapi/drm/drm.h
>> +++ b/include/uapi/drm/drm.h
>> @@ -717,6 +717,7 @@ struct drm_prime_handle {
>> struct drm_syncobj_create {
>> __u32 handle;
>> #define DRM_SYNCOBJ_CREATE_SIGNALED (1 << 0)
>> +#define DRM_SYNCOBJ_CREATE_TYPE_TIMELINE (1 << 1)
>> __u32 flags;
>> };
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] [RFC]drm: add syncobj timeline support v6
[not found] ` <04e91247-387c-ed5d-0793-9eb1d37d12bd-5C7GfCeVMHo@public.gmane.org>
2018-09-19 7:32 ` zhoucm1
@ 2018-09-19 7:32 ` zhoucm1
[not found] ` <392512eb-271d-3d16-263b-f2429dedfffc-5C7GfCeVMHo@public.gmane.org>
1 sibling, 1 reply; 11+ messages in thread
From: zhoucm1 @ 2018-09-19 7:32 UTC (permalink / raw)
To: Christian König, Chunming Zhou,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Cc: Dave Airlie, Daniel Rakos, Daniel Vetter,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
On 2018年09月19日 15:18, Christian König wrote:
> Am 19.09.2018 um 06:26 schrieb Chunming Zhou:
[snip]
>> *fence = NULL;
>> drm_syncobj_add_callback_locked(syncobj, cb, func);
>> @@ -164,6 +177,153 @@ void drm_syncobj_remove_callback(struct
>> drm_syncobj *syncobj,
>> spin_unlock(&syncobj->lock);
>> }
>> +static void drm_syncobj_timeline_init(struct drm_syncobj *syncobj)
>
> We still have _timeline_ in the name here.
the func is relevant to timeline members, or which name is proper?
>
>> +{
>> + spin_lock(&syncobj->lock);
>> + syncobj->timeline_context = dma_fence_context_alloc(1);
[snip]
>> +}
>> +
>> +int drm_syncobj_lookup_fence(struct drm_syncobj *syncobj, u64 point,
>> + struct dma_fence **fence) {
>> +
>> + return drm_syncobj_search_fence(syncobj, point,
>> + DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT,
>
> I still have a bad feeling setting that flag as default cause it might
> change the behavior for the UAPI.
>
> Maybe export drm_syncobj_search_fence directly? E.g. with the flags
> parameter.
previous v5 indeed do this, you let me wrap it, need change back?
Regards,
David Zhou
>
> Regards,
> Christian.
>
>> + fence);
>> +}
>> +EXPORT_SYMBOL(drm_syncobj_lookup_fence);
>> +
>> /**
>> * drm_syncobj_find_fence - lookup and reference the fence in a
>> sync object
>> * @file_private: drm file private pointer
>> @@ -228,7 +443,7 @@ static int drm_syncobj_assign_null_handle(struct
>> drm_syncobj *syncobj)
>> * @fence: out parameter for the fence
>> *
>> * This is just a convenience function that combines
>> drm_syncobj_find() and
>> - * drm_syncobj_fence_get().
>> + * drm_syncobj_lookup_fence().
>> *
>> * Returns 0 on success or a negative error value on failure. On
>> success @fence
>> * contains a reference to the fence, which must be released by
>> calling
>> @@ -236,18 +451,11 @@ static int
>> drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj)
>> */
>> int drm_syncobj_find_fence(struct drm_file *file_private,
>> u32 handle, u64 point,
>> - struct dma_fence **fence)
>> -{
>> + struct dma_fence **fence) {
>> struct drm_syncobj *syncobj = drm_syncobj_find(file_private,
>> handle);
>> - int ret = 0;
>> -
>> - if (!syncobj)
>> - return -ENOENT;
>> + int ret;
>> - *fence = drm_syncobj_fence_get(syncobj);
>> - if (!*fence) {
>> - ret = -EINVAL;
>> - }
>> + ret = drm_syncobj_lookup_fence(syncobj, point, fence);
>> drm_syncobj_put(syncobj);
>> return ret;
>> }
>> @@ -264,7 +472,7 @@ void drm_syncobj_free(struct kref *kref)
>> struct drm_syncobj *syncobj = container_of(kref,
>> struct drm_syncobj,
>> refcount);
>> - drm_syncobj_replace_fence(syncobj, 0, NULL);
>> + drm_syncobj_timeline_fini(syncobj);
>> kfree(syncobj);
>> }
>> EXPORT_SYMBOL(drm_syncobj_free);
>> @@ -294,6 +502,11 @@ int drm_syncobj_create(struct drm_syncobj
>> **out_syncobj, uint32_t flags,
>> kref_init(&syncobj->refcount);
>> INIT_LIST_HEAD(&syncobj->cb_list);
>> spin_lock_init(&syncobj->lock);
>> + if (flags & DRM_SYNCOBJ_CREATE_TYPE_TIMELINE)
>> + syncobj->type = DRM_SYNCOBJ_TYPE_TIMELINE;
>> + else
>> + syncobj->type = DRM_SYNCOBJ_TYPE_INDIVIDUAL;
>> + drm_syncobj_timeline_init(syncobj);
>> if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) {
>> ret = drm_syncobj_assign_null_handle(syncobj);
>> @@ -576,7 +789,8 @@ drm_syncobj_create_ioctl(struct drm_device *dev,
>> void *data,
>> return -ENODEV;
>> /* no valid flags yet */
>> - if (args->flags & ~DRM_SYNCOBJ_CREATE_SIGNALED)
>> + if (args->flags & ~(DRM_SYNCOBJ_CREATE_SIGNALED |
>> + DRM_SYNCOBJ_CREATE_TYPE_TIMELINE))
>> return -EINVAL;
>> return drm_syncobj_create_as_handle(file_private,
>> @@ -669,9 +883,8 @@ static void syncobj_wait_syncobj_func(struct
>> drm_syncobj *syncobj,
>> struct syncobj_wait_entry *wait =
>> container_of(cb, struct syncobj_wait_entry, syncobj_cb);
>> - /* This happens inside the syncobj lock */
>> - wait->fence =
>> dma_fence_get(rcu_dereference_protected(syncobj->fence,
>> - lockdep_is_held(&syncobj->lock)));
>> + drm_syncobj_search_fence(syncobj, 0, 0, &wait->fence);
>> +
>> wake_up_process(wait->task);
>> }
>> @@ -698,7 +911,8 @@ static signed long
>> drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
>> signaled_count = 0;
>> for (i = 0; i < count; ++i) {
>> entries[i].task = current;
>> - entries[i].fence = drm_syncobj_fence_get(syncobjs[i]);
>> + ret = drm_syncobj_search_fence(syncobjs[i], 0, 0,
>> + &entries[i].fence);
>> if (!entries[i].fence) {
>> if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
>> continue;
>> @@ -970,12 +1184,19 @@ drm_syncobj_reset_ioctl(struct drm_device
>> *dev, void *data,
>> if (ret < 0)
>> return ret;
>> - for (i = 0; i < args->count_handles; i++)
>> - drm_syncobj_replace_fence(syncobjs[i], 0, NULL);
>> -
>> + for (i = 0; i < args->count_handles; i++) {
>> + if (syncobjs[i]->type == DRM_SYNCOBJ_TYPE_TIMELINE) {
>> + DRM_ERROR("timeline syncobj cannot reset!\n");
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> + drm_syncobj_timeline_fini(syncobjs[i]);
>> + drm_syncobj_timeline_init(syncobjs[i]);
>> + }
>> +out:
>> drm_syncobj_array_free(syncobjs, args->count_handles);
>> - return 0;
>> + return ret;
>> }
>> int
>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> index 0a8d2d64f380..a040fa3281df 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> @@ -2137,7 +2137,7 @@ await_fence_array(struct i915_execbuffer *eb,
>> if (!(flags & I915_EXEC_FENCE_WAIT))
>> continue;
>> - fence = drm_syncobj_fence_get(syncobj);
>> + drm_syncobj_lookup_fence(syncobj, 0, &fence);
>> if (!fence)
>> return -EINVAL;
>> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
>> index 425432b85a87..6dac479fedfe 100644
>> --- a/include/drm/drm_syncobj.h
>> +++ b/include/drm/drm_syncobj.h
>> @@ -30,6 +30,11 @@
>> struct drm_syncobj_cb;
>> +enum drm_syncobj_type {
>> + DRM_SYNCOBJ_TYPE_INDIVIDUAL,
>> + DRM_SYNCOBJ_TYPE_TIMELINE
>> +};
>> +
>> /**
>> * struct drm_syncobj - sync object.
>> *
>> @@ -41,19 +46,36 @@ struct drm_syncobj {
>> */
>> struct kref refcount;
>> /**
>> - * @fence:
>> - * NULL or a pointer to the fence bound to this object.
>> - *
>> - * This field should not be used directly. Use
>> drm_syncobj_fence_get()
>> - * and drm_syncobj_replace_fence() instead.
>> + * @type: indicate syncobj type
>> + */
>> + enum drm_syncobj_type type;
>> + /**
>> + * @wq: wait signal operation work queue
>> + */
>> + wait_queue_head_t wq;
>> + /**
>> + * @timeline_context: fence context used by timeline
>> */
>> - struct dma_fence __rcu *fence;
>> + u64 timeline_context;
>> /**
>> - * @cb_list: List of callbacks to call when the &fence gets
>> replaced.
>> + * @timeline: syncobj timeline value, which indicates point is
>> signaled.
>> */
>> + u64 timeline;
>> + /**
>> + * @signal_point: which indicates the latest signaler point.
>> + */
>> + u64 signal_point;
>> + /**
>> + * @signal_pt_list: signaler point list.
>> + */
>> + struct list_head signal_pt_list;
>> +
>> + /**
>> + * @cb_list: List of callbacks to call when the &fence gets
>> replaced.
>> + */
>> struct list_head cb_list;
>> /**
>> - * @lock: Protects &cb_list and write-locks &fence.
>> + * @lock: Protects syncobj list and write-locks &fence.
>> */
>> spinlock_t lock;
>> /**
>> @@ -68,7 +90,7 @@ typedef void (*drm_syncobj_func_t)(struct
>> drm_syncobj *syncobj,
>> /**
>> * struct drm_syncobj_cb - callback for drm_syncobj_add_callback
>> * @node: used by drm_syncob_add_callback to append this struct to
>> - * &drm_syncobj.cb_list
>> + * &drm_syncobj.cb_list
>> * @func: drm_syncobj_func_t to call
>> *
>> * This struct will be initialized by drm_syncobj_add_callback,
>> additional
>> @@ -106,29 +128,6 @@ drm_syncobj_put(struct drm_syncobj *obj)
>> kref_put(&obj->refcount, drm_syncobj_free);
>> }
>> -/**
>> - * drm_syncobj_fence_get - get a reference to a fence in a sync object
>> - * @syncobj: sync object.
>> - *
>> - * This acquires additional reference to &drm_syncobj.fence
>> contained in @obj,
>> - * if not NULL. It is illegal to call this without already holding a
>> reference.
>> - * No locks required.
>> - *
>> - * Returns:
>> - * Either the fence of @obj or NULL if there's none.
>> - */
>> -static inline struct dma_fence *
>> -drm_syncobj_fence_get(struct drm_syncobj *syncobj)
>> -{
>> - struct dma_fence *fence;
>> -
>> - rcu_read_lock();
>> - fence = dma_fence_get_rcu_safe(&syncobj->fence);
>> - rcu_read_unlock();
>> -
>> - return fence;
>> -}
>> -
>> struct drm_syncobj *drm_syncobj_find(struct drm_file *file_private,
>> u32 handle);
>> void drm_syncobj_replace_fence(struct drm_syncobj *syncobj, u64 point,
>> @@ -142,5 +141,7 @@ int drm_syncobj_create(struct drm_syncobj
>> **out_syncobj, uint32_t flags,
>> int drm_syncobj_get_handle(struct drm_file *file_private,
>> struct drm_syncobj *syncobj, u32 *handle);
>> int drm_syncobj_get_fd(struct drm_syncobj *syncobj, int *p_fd);
>> +int drm_syncobj_lookup_fence(struct drm_syncobj *syncobj, u64 point,
>> + struct dma_fence **fence);
>> #endif
>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>> index 300f336633f2..cebdb2541eb7 100644
>> --- a/include/uapi/drm/drm.h
>> +++ b/include/uapi/drm/drm.h
>> @@ -717,6 +717,7 @@ struct drm_prime_handle {
>> struct drm_syncobj_create {
>> __u32 handle;
>> #define DRM_SYNCOBJ_CREATE_SIGNALED (1 << 0)
>> +#define DRM_SYNCOBJ_CREATE_TYPE_TIMELINE (1 << 1)
>> __u32 flags;
>> };
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] [RFC]drm: add syncobj timeline support v6
[not found] ` <392512eb-271d-3d16-263b-f2429dedfffc-5C7GfCeVMHo@public.gmane.org>
@ 2018-09-19 7:45 ` Christian König
[not found] ` <9bdce2dd-2129-3ed9-56f0-db4b14005194-5C7GfCeVMHo@public.gmane.org>
0 siblings, 1 reply; 11+ messages in thread
From: Christian König @ 2018-09-19 7:45 UTC (permalink / raw)
To: zhoucm1, Chunming Zhou, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Cc: Dave Airlie, Daniel Rakos, Daniel Vetter,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Am 19.09.2018 um 09:32 schrieb zhoucm1:
>
>
> On 2018年09月19日 15:18, Christian König wrote:
>> Am 19.09.2018 um 06:26 schrieb Chunming Zhou:
> [snip]
>>> *fence = NULL;
>>> drm_syncobj_add_callback_locked(syncobj, cb, func);
>>> @@ -164,6 +177,153 @@ void drm_syncobj_remove_callback(struct
>>> drm_syncobj *syncobj,
>>> spin_unlock(&syncobj->lock);
>>> }
>>> +static void drm_syncobj_timeline_init(struct drm_syncobj *syncobj)
>>
>> We still have _timeline_ in the name here.
> the func is relevant to timeline members, or which name is proper?
Yeah, but we now use the timeline implementation for the individual
syncobj as well.
Not a big issue, but I would just name it
drm_syncobj_init()/drm_syncobj_fini.
>
>>
>>> +{
>>> + spin_lock(&syncobj->lock);
>>> + syncobj->timeline_context = dma_fence_context_alloc(1);
> [snip]
>>> +}
>>> +
>>> +int drm_syncobj_lookup_fence(struct drm_syncobj *syncobj, u64 point,
>>> + struct dma_fence **fence) {
>>> +
>>> + return drm_syncobj_search_fence(syncobj, point,
>>> + DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT,
>>
>> I still have a bad feeling setting that flag as default cause it
>> might change the behavior for the UAPI.
>>
>> Maybe export drm_syncobj_search_fence directly? E.g. with the flags
>> parameter.
> previous v5 indeed do this, you let me wrap it, need change back?
No, the problem is that drm_syncobj_find_fence() is still using
drm_syncobj_lookup_fence() which sets the flag instead of
drm_syncobj_search_fence() without the flag.
That changes the UAPI behavior because previously we would have returned
an error code and now we block for a fence to appear.
So I think the right solution would be to add the flags parameter to
drm_syncobj_find_fence() and let the driver decide if we need to block
or get -ENOENT.
Regards,
Christian.
>
> Regards,
> David Zhou
>>
>> Regards,
>> Christian.
>>
>>> + fence);
>>> +}
>>> +EXPORT_SYMBOL(drm_syncobj_lookup_fence);
>>> +
>>> /**
>>> * drm_syncobj_find_fence - lookup and reference the fence in a
>>> sync object
>>> * @file_private: drm file private pointer
>>> @@ -228,7 +443,7 @@ static int drm_syncobj_assign_null_handle(struct
>>> drm_syncobj *syncobj)
>>> * @fence: out parameter for the fence
>>> *
>>> * This is just a convenience function that combines
>>> drm_syncobj_find() and
>>> - * drm_syncobj_fence_get().
>>> + * drm_syncobj_lookup_fence().
>>> *
>>> * Returns 0 on success or a negative error value on failure. On
>>> success @fence
>>> * contains a reference to the fence, which must be released by
>>> calling
>>> @@ -236,18 +451,11 @@ static int
>>> drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj)
>>> */
>>> int drm_syncobj_find_fence(struct drm_file *file_private,
>>> u32 handle, u64 point,
>>> - struct dma_fence **fence)
>>> -{
>>> + struct dma_fence **fence) {
>>> struct drm_syncobj *syncobj = drm_syncobj_find(file_private,
>>> handle);
>>> - int ret = 0;
>>> -
>>> - if (!syncobj)
>>> - return -ENOENT;
>>> + int ret;
>>> - *fence = drm_syncobj_fence_get(syncobj);
>>> - if (!*fence) {
>>> - ret = -EINVAL;
>>> - }
>>> + ret = drm_syncobj_lookup_fence(syncobj, point, fence);
>>> drm_syncobj_put(syncobj);
>>> return ret;
>>> }
>>> @@ -264,7 +472,7 @@ void drm_syncobj_free(struct kref *kref)
>>> struct drm_syncobj *syncobj = container_of(kref,
>>> struct drm_syncobj,
>>> refcount);
>>> - drm_syncobj_replace_fence(syncobj, 0, NULL);
>>> + drm_syncobj_timeline_fini(syncobj);
>>> kfree(syncobj);
>>> }
>>> EXPORT_SYMBOL(drm_syncobj_free);
>>> @@ -294,6 +502,11 @@ int drm_syncobj_create(struct drm_syncobj
>>> **out_syncobj, uint32_t flags,
>>> kref_init(&syncobj->refcount);
>>> INIT_LIST_HEAD(&syncobj->cb_list);
>>> spin_lock_init(&syncobj->lock);
>>> + if (flags & DRM_SYNCOBJ_CREATE_TYPE_TIMELINE)
>>> + syncobj->type = DRM_SYNCOBJ_TYPE_TIMELINE;
>>> + else
>>> + syncobj->type = DRM_SYNCOBJ_TYPE_INDIVIDUAL;
>>> + drm_syncobj_timeline_init(syncobj);
>>> if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) {
>>> ret = drm_syncobj_assign_null_handle(syncobj);
>>> @@ -576,7 +789,8 @@ drm_syncobj_create_ioctl(struct drm_device *dev,
>>> void *data,
>>> return -ENODEV;
>>> /* no valid flags yet */
>>> - if (args->flags & ~DRM_SYNCOBJ_CREATE_SIGNALED)
>>> + if (args->flags & ~(DRM_SYNCOBJ_CREATE_SIGNALED |
>>> + DRM_SYNCOBJ_CREATE_TYPE_TIMELINE))
>>> return -EINVAL;
>>> return drm_syncobj_create_as_handle(file_private,
>>> @@ -669,9 +883,8 @@ static void syncobj_wait_syncobj_func(struct
>>> drm_syncobj *syncobj,
>>> struct syncobj_wait_entry *wait =
>>> container_of(cb, struct syncobj_wait_entry, syncobj_cb);
>>> - /* This happens inside the syncobj lock */
>>> - wait->fence =
>>> dma_fence_get(rcu_dereference_protected(syncobj->fence,
>>> - lockdep_is_held(&syncobj->lock)));
>>> + drm_syncobj_search_fence(syncobj, 0, 0, &wait->fence);
>>> +
>>> wake_up_process(wait->task);
>>> }
>>> @@ -698,7 +911,8 @@ static signed long
>>> drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
>>> signaled_count = 0;
>>> for (i = 0; i < count; ++i) {
>>> entries[i].task = current;
>>> - entries[i].fence = drm_syncobj_fence_get(syncobjs[i]);
>>> + ret = drm_syncobj_search_fence(syncobjs[i], 0, 0,
>>> + &entries[i].fence);
>>> if (!entries[i].fence) {
>>> if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
>>> continue;
>>> @@ -970,12 +1184,19 @@ drm_syncobj_reset_ioctl(struct drm_device
>>> *dev, void *data,
>>> if (ret < 0)
>>> return ret;
>>> - for (i = 0; i < args->count_handles; i++)
>>> - drm_syncobj_replace_fence(syncobjs[i], 0, NULL);
>>> -
>>> + for (i = 0; i < args->count_handles; i++) {
>>> + if (syncobjs[i]->type == DRM_SYNCOBJ_TYPE_TIMELINE) {
>>> + DRM_ERROR("timeline syncobj cannot reset!\n");
>>> + ret = -EINVAL;
>>> + goto out;
>>> + }
>>> + drm_syncobj_timeline_fini(syncobjs[i]);
>>> + drm_syncobj_timeline_init(syncobjs[i]);
>>> + }
>>> +out:
>>> drm_syncobj_array_free(syncobjs, args->count_handles);
>>> - return 0;
>>> + return ret;
>>> }
>>> int
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>> index 0a8d2d64f380..a040fa3281df 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>> @@ -2137,7 +2137,7 @@ await_fence_array(struct i915_execbuffer *eb,
>>> if (!(flags & I915_EXEC_FENCE_WAIT))
>>> continue;
>>> - fence = drm_syncobj_fence_get(syncobj);
>>> + drm_syncobj_lookup_fence(syncobj, 0, &fence);
>>> if (!fence)
>>> return -EINVAL;
>>> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
>>> index 425432b85a87..6dac479fedfe 100644
>>> --- a/include/drm/drm_syncobj.h
>>> +++ b/include/drm/drm_syncobj.h
>>> @@ -30,6 +30,11 @@
>>> struct drm_syncobj_cb;
>>> +enum drm_syncobj_type {
>>> + DRM_SYNCOBJ_TYPE_INDIVIDUAL,
>>> + DRM_SYNCOBJ_TYPE_TIMELINE
>>> +};
>>> +
>>> /**
>>> * struct drm_syncobj - sync object.
>>> *
>>> @@ -41,19 +46,36 @@ struct drm_syncobj {
>>> */
>>> struct kref refcount;
>>> /**
>>> - * @fence:
>>> - * NULL or a pointer to the fence bound to this object.
>>> - *
>>> - * This field should not be used directly. Use
>>> drm_syncobj_fence_get()
>>> - * and drm_syncobj_replace_fence() instead.
>>> + * @type: indicate syncobj type
>>> + */
>>> + enum drm_syncobj_type type;
>>> + /**
>>> + * @wq: wait signal operation work queue
>>> + */
>>> + wait_queue_head_t wq;
>>> + /**
>>> + * @timeline_context: fence context used by timeline
>>> */
>>> - struct dma_fence __rcu *fence;
>>> + u64 timeline_context;
>>> /**
>>> - * @cb_list: List of callbacks to call when the &fence gets
>>> replaced.
>>> + * @timeline: syncobj timeline value, which indicates point is
>>> signaled.
>>> */
>>> + u64 timeline;
>>> + /**
>>> + * @signal_point: which indicates the latest signaler point.
>>> + */
>>> + u64 signal_point;
>>> + /**
>>> + * @signal_pt_list: signaler point list.
>>> + */
>>> + struct list_head signal_pt_list;
>>> +
>>> + /**
>>> + * @cb_list: List of callbacks to call when the &fence gets
>>> replaced.
>>> + */
>>> struct list_head cb_list;
>>> /**
>>> - * @lock: Protects &cb_list and write-locks &fence.
>>> + * @lock: Protects syncobj list and write-locks &fence.
>>> */
>>> spinlock_t lock;
>>> /**
>>> @@ -68,7 +90,7 @@ typedef void (*drm_syncobj_func_t)(struct
>>> drm_syncobj *syncobj,
>>> /**
>>> * struct drm_syncobj_cb - callback for drm_syncobj_add_callback
>>> * @node: used by drm_syncob_add_callback to append this struct to
>>> - * &drm_syncobj.cb_list
>>> + * &drm_syncobj.cb_list
>>> * @func: drm_syncobj_func_t to call
>>> *
>>> * This struct will be initialized by drm_syncobj_add_callback,
>>> additional
>>> @@ -106,29 +128,6 @@ drm_syncobj_put(struct drm_syncobj *obj)
>>> kref_put(&obj->refcount, drm_syncobj_free);
>>> }
>>> -/**
>>> - * drm_syncobj_fence_get - get a reference to a fence in a sync object
>>> - * @syncobj: sync object.
>>> - *
>>> - * This acquires additional reference to &drm_syncobj.fence
>>> contained in @obj,
>>> - * if not NULL. It is illegal to call this without already holding
>>> a reference.
>>> - * No locks required.
>>> - *
>>> - * Returns:
>>> - * Either the fence of @obj or NULL if there's none.
>>> - */
>>> -static inline struct dma_fence *
>>> -drm_syncobj_fence_get(struct drm_syncobj *syncobj)
>>> -{
>>> - struct dma_fence *fence;
>>> -
>>> - rcu_read_lock();
>>> - fence = dma_fence_get_rcu_safe(&syncobj->fence);
>>> - rcu_read_unlock();
>>> -
>>> - return fence;
>>> -}
>>> -
>>> struct drm_syncobj *drm_syncobj_find(struct drm_file *file_private,
>>> u32 handle);
>>> void drm_syncobj_replace_fence(struct drm_syncobj *syncobj, u64
>>> point,
>>> @@ -142,5 +141,7 @@ int drm_syncobj_create(struct drm_syncobj
>>> **out_syncobj, uint32_t flags,
>>> int drm_syncobj_get_handle(struct drm_file *file_private,
>>> struct drm_syncobj *syncobj, u32 *handle);
>>> int drm_syncobj_get_fd(struct drm_syncobj *syncobj, int *p_fd);
>>> +int drm_syncobj_lookup_fence(struct drm_syncobj *syncobj, u64 point,
>>> + struct dma_fence **fence);
>>> #endif
>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>>> index 300f336633f2..cebdb2541eb7 100644
>>> --- a/include/uapi/drm/drm.h
>>> +++ b/include/uapi/drm/drm.h
>>> @@ -717,6 +717,7 @@ struct drm_prime_handle {
>>> struct drm_syncobj_create {
>>> __u32 handle;
>>> #define DRM_SYNCOBJ_CREATE_SIGNALED (1 << 0)
>>> +#define DRM_SYNCOBJ_CREATE_TYPE_TIMELINE (1 << 1)
>>> __u32 flags;
>>> };
>>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 1/4] [RFC]drm: add syncobj timeline support v6
[not found] ` <9bdce2dd-2129-3ed9-56f0-db4b14005194-5C7GfCeVMHo@public.gmane.org>
@ 2018-09-19 8:03 ` Zhou, David(ChunMing)
[not found] ` <BY1PR12MB05021AF8F3FD552CC133BCC6B41C0-PicGAnIBOobrCwm+z9iKNgdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
0 siblings, 1 reply; 11+ messages in thread
From: Zhou, David(ChunMing) @ 2018-09-19 8:03 UTC (permalink / raw)
To: Koenig, Christian, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Cc: Dave Airlie, Rakos, Daniel,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Daniel Vetter
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Christian K?nig
> Sent: Wednesday, September 19, 2018 3:45 PM
> To: Zhou, David(ChunMing) <David1.Zhou@amd.com>; Zhou,
> David(ChunMing) <David1.Zhou@amd.com>; dri-
> devel@lists.freedesktop.org
> Cc: Dave Airlie <airlied@redhat.com>; Rakos, Daniel
> <Daniel.Rakos@amd.com>; Daniel Vetter <daniel@ffwll.ch>; amd-
> gfx@lists.freedesktop.org
> Subject: Re: [PATCH 1/4] [RFC]drm: add syncobj timeline support v6
>
> Am 19.09.2018 um 09:32 schrieb zhoucm1:
> >
> >
> > On 2018年09月19日 15:18, Christian König wrote:
> >> Am 19.09.2018 um 06:26 schrieb Chunming Zhou:
> > [snip]
> >>> *fence = NULL;
> >>> drm_syncobj_add_callback_locked(syncobj, cb, func); @@
> >>> -164,6 +177,153 @@ void drm_syncobj_remove_callback(struct
> >>> drm_syncobj *syncobj,
> >>> spin_unlock(&syncobj->lock);
> >>> }
> >>> +static void drm_syncobj_timeline_init(struct drm_syncobj
> >>> *syncobj)
> >>
> >> We still have _timeline_ in the name here.
> > the func is relevant to timeline members, or which name is proper?
>
> Yeah, but we now use the timeline implementation for the individual syncobj
> as well.
>
> Not a big issue, but I would just name it
> drm_syncobj_init()/drm_syncobj_fini.
There is already drm_syncobj_init/fini in drm_syncboj.c , any other name can be suggested?
>
> >
> >>
> >>> +{
> >>> + spin_lock(&syncobj->lock);
> >>> + syncobj->timeline_context = dma_fence_context_alloc(1);
> > [snip]
> >>> +}
> >>> +
> >>> +int drm_syncobj_lookup_fence(struct drm_syncobj *syncobj, u64
> >>> +point,
> >>> + struct dma_fence **fence) {
> >>> +
> >>> + return drm_syncobj_search_fence(syncobj, point,
> >>> + DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT,
> >>
> >> I still have a bad feeling setting that flag as default cause it
> >> might change the behavior for the UAPI.
> >>
> >> Maybe export drm_syncobj_search_fence directly? E.g. with the flags
> >> parameter.
> > previous v5 indeed do this, you let me wrap it, need change back?
>
> No, the problem is that drm_syncobj_find_fence() is still using
> drm_syncobj_lookup_fence() which sets the flag instead of
> drm_syncobj_search_fence() without the flag.
>
> That changes the UAPI behavior because previously we would have returned
> an error code and now we block for a fence to appear.
>
> So I think the right solution would be to add the flags parameter to
> drm_syncobj_find_fence() and let the driver decide if we need to block or
> get -ENOENT.
Got your means,
Exporting flag in func is easy,
but driver doesn't pass flag, which flag is proper by default? We still need to give a default flag in patch, don't we?
Thanks,
David Zhou
>
> Regards,
> Christian.
>
> >
> > Regards,
> > David Zhou
> >>
> >> Regards,
> >> Christian.
> >>
> >>> + fence);
> >>> +}
> >>> +EXPORT_SYMBOL(drm_syncobj_lookup_fence);
> >>> +
> >>> /**
> >>> * drm_syncobj_find_fence - lookup and reference the fence in a
> >>> sync object
> >>> * @file_private: drm file private pointer @@ -228,7 +443,7 @@
> >>> static int drm_syncobj_assign_null_handle(struct
> >>> drm_syncobj *syncobj)
> >>> * @fence: out parameter for the fence
> >>> *
> >>> * This is just a convenience function that combines
> >>> drm_syncobj_find() and
> >>> - * drm_syncobj_fence_get().
> >>> + * drm_syncobj_lookup_fence().
> >>> *
> >>> * Returns 0 on success or a negative error value on failure. On
> >>> success @fence
> >>> * contains a reference to the fence, which must be released by
> >>> calling @@ -236,18 +451,11 @@ static int
> >>> drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj)
> >>> */
> >>> int drm_syncobj_find_fence(struct drm_file *file_private,
> >>> u32 handle, u64 point,
> >>> - struct dma_fence **fence) -{
> >>> + struct dma_fence **fence) {
> >>> struct drm_syncobj *syncobj = drm_syncobj_find(file_private,
> >>> handle);
> >>> - int ret = 0;
> >>> -
> >>> - if (!syncobj)
> >>> - return -ENOENT;
> >>> + int ret;
> >>> - *fence = drm_syncobj_fence_get(syncobj);
> >>> - if (!*fence) {
> >>> - ret = -EINVAL;
> >>> - }
> >>> + ret = drm_syncobj_lookup_fence(syncobj, point, fence);
> >>> drm_syncobj_put(syncobj);
> >>> return ret;
> >>> }
> >>> @@ -264,7 +472,7 @@ void drm_syncobj_free(struct kref *kref)
> >>> struct drm_syncobj *syncobj = container_of(kref,
> >>> struct drm_syncobj,
> >>> refcount);
> >>> - drm_syncobj_replace_fence(syncobj, 0, NULL);
> >>> + drm_syncobj_timeline_fini(syncobj);
> >>> kfree(syncobj);
> >>> }
> >>> EXPORT_SYMBOL(drm_syncobj_free);
> >>> @@ -294,6 +502,11 @@ int drm_syncobj_create(struct drm_syncobj
> >>> **out_syncobj, uint32_t flags,
> >>> kref_init(&syncobj->refcount);
> >>> INIT_LIST_HEAD(&syncobj->cb_list);
> >>> spin_lock_init(&syncobj->lock);
> >>> + if (flags & DRM_SYNCOBJ_CREATE_TYPE_TIMELINE)
> >>> + syncobj->type = DRM_SYNCOBJ_TYPE_TIMELINE;
> >>> + else
> >>> + syncobj->type = DRM_SYNCOBJ_TYPE_INDIVIDUAL;
> >>> + drm_syncobj_timeline_init(syncobj);
> >>> if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) {
> >>> ret = drm_syncobj_assign_null_handle(syncobj);
> >>> @@ -576,7 +789,8 @@ drm_syncobj_create_ioctl(struct drm_device
> *dev,
> >>> void *data,
> >>> return -ENODEV;
> >>> /* no valid flags yet */
> >>> - if (args->flags & ~DRM_SYNCOBJ_CREATE_SIGNALED)
> >>> + if (args->flags & ~(DRM_SYNCOBJ_CREATE_SIGNALED |
> >>> + DRM_SYNCOBJ_CREATE_TYPE_TIMELINE))
> >>> return -EINVAL;
> >>> return drm_syncobj_create_as_handle(file_private,
> >>> @@ -669,9 +883,8 @@ static void syncobj_wait_syncobj_func(struct
> >>> drm_syncobj *syncobj,
> >>> struct syncobj_wait_entry *wait =
> >>> container_of(cb, struct syncobj_wait_entry, syncobj_cb);
> >>> - /* This happens inside the syncobj lock */
> >>> - wait->fence =
> >>> dma_fence_get(rcu_dereference_protected(syncobj->fence,
> >>> - lockdep_is_held(&syncobj->lock)));
> >>> + drm_syncobj_search_fence(syncobj, 0, 0, &wait->fence);
> >>> +
> >>> wake_up_process(wait->task);
> >>> }
> >>> @@ -698,7 +911,8 @@ static signed long
> >>> drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
> >>> signaled_count = 0;
> >>> for (i = 0; i < count; ++i) {
> >>> entries[i].task = current;
> >>> - entries[i].fence = drm_syncobj_fence_get(syncobjs[i]);
> >>> + ret = drm_syncobj_search_fence(syncobjs[i], 0, 0,
> >>> + &entries[i].fence);
> >>> if (!entries[i].fence) {
> >>> if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
> >>> continue;
> >>> @@ -970,12 +1184,19 @@ drm_syncobj_reset_ioctl(struct drm_device
> >>> *dev, void *data,
> >>> if (ret < 0)
> >>> return ret;
> >>> - for (i = 0; i < args->count_handles; i++)
> >>> - drm_syncobj_replace_fence(syncobjs[i], 0, NULL);
> >>> -
> >>> + for (i = 0; i < args->count_handles; i++) {
> >>> + if (syncobjs[i]->type == DRM_SYNCOBJ_TYPE_TIMELINE) {
> >>> + DRM_ERROR("timeline syncobj cannot reset!\n");
> >>> + ret = -EINVAL;
> >>> + goto out;
> >>> + }
> >>> + drm_syncobj_timeline_fini(syncobjs[i]);
> >>> + drm_syncobj_timeline_init(syncobjs[i]);
> >>> + }
> >>> +out:
> >>> drm_syncobj_array_free(syncobjs, args->count_handles);
> >>> - return 0;
> >>> + return ret;
> >>> }
> >>> int
> >>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >>> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >>> index 0a8d2d64f380..a040fa3281df 100644
> >>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >>> @@ -2137,7 +2137,7 @@ await_fence_array(struct i915_execbuffer *eb,
> >>> if (!(flags & I915_EXEC_FENCE_WAIT))
> >>> continue;
> >>> - fence = drm_syncobj_fence_get(syncobj);
> >>> + drm_syncobj_lookup_fence(syncobj, 0, &fence);
> >>> if (!fence)
> >>> return -EINVAL;
> >>> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
> >>> index 425432b85a87..6dac479fedfe 100644
> >>> --- a/include/drm/drm_syncobj.h
> >>> +++ b/include/drm/drm_syncobj.h
> >>> @@ -30,6 +30,11 @@
> >>> struct drm_syncobj_cb;
> >>> +enum drm_syncobj_type {
> >>> + DRM_SYNCOBJ_TYPE_INDIVIDUAL,
> >>> + DRM_SYNCOBJ_TYPE_TIMELINE
> >>> +};
> >>> +
> >>> /**
> >>> * struct drm_syncobj - sync object.
> >>> *
> >>> @@ -41,19 +46,36 @@ struct drm_syncobj {
> >>> */
> >>> struct kref refcount;
> >>> /**
> >>> - * @fence:
> >>> - * NULL or a pointer to the fence bound to this object.
> >>> - *
> >>> - * This field should not be used directly. Use
> >>> drm_syncobj_fence_get()
> >>> - * and drm_syncobj_replace_fence() instead.
> >>> + * @type: indicate syncobj type
> >>> + */
> >>> + enum drm_syncobj_type type;
> >>> + /**
> >>> + * @wq: wait signal operation work queue
> >>> + */
> >>> + wait_queue_head_t wq;
> >>> + /**
> >>> + * @timeline_context: fence context used by timeline
> >>> */
> >>> - struct dma_fence __rcu *fence;
> >>> + u64 timeline_context;
> >>> /**
> >>> - * @cb_list: List of callbacks to call when the &fence gets
> >>> replaced.
> >>> + * @timeline: syncobj timeline value, which indicates point is
> >>> signaled.
> >>> */
> >>> + u64 timeline;
> >>> + /**
> >>> + * @signal_point: which indicates the latest signaler point.
> >>> + */
> >>> + u64 signal_point;
> >>> + /**
> >>> + * @signal_pt_list: signaler point list.
> >>> + */
> >>> + struct list_head signal_pt_list;
> >>> +
> >>> + /**
> >>> + * @cb_list: List of callbacks to call when the &fence gets
> >>> replaced.
> >>> + */
> >>> struct list_head cb_list;
> >>> /**
> >>> - * @lock: Protects &cb_list and write-locks &fence.
> >>> + * @lock: Protects syncobj list and write-locks &fence.
> >>> */
> >>> spinlock_t lock;
> >>> /**
> >>> @@ -68,7 +90,7 @@ typedef void (*drm_syncobj_func_t)(struct
> >>> drm_syncobj *syncobj,
> >>> /**
> >>> * struct drm_syncobj_cb - callback for drm_syncobj_add_callback
> >>> * @node: used by drm_syncob_add_callback to append this struct to
> >>> - * &drm_syncobj.cb_list
> >>> + * &drm_syncobj.cb_list
> >>> * @func: drm_syncobj_func_t to call
> >>> *
> >>> * This struct will be initialized by drm_syncobj_add_callback,
> >>> additional @@ -106,29 +128,6 @@ drm_syncobj_put(struct drm_syncobj
> >>> *obj)
> >>> kref_put(&obj->refcount, drm_syncobj_free);
> >>> }
> >>> -/**
> >>> - * drm_syncobj_fence_get - get a reference to a fence in a sync
> >>> object
> >>> - * @syncobj: sync object.
> >>> - *
> >>> - * This acquires additional reference to &drm_syncobj.fence
> >>> contained in @obj,
> >>> - * if not NULL. It is illegal to call this without already holding
> >>> a reference.
> >>> - * No locks required.
> >>> - *
> >>> - * Returns:
> >>> - * Either the fence of @obj or NULL if there's none.
> >>> - */
> >>> -static inline struct dma_fence *
> >>> -drm_syncobj_fence_get(struct drm_syncobj *syncobj) -{
> >>> - struct dma_fence *fence;
> >>> -
> >>> - rcu_read_lock();
> >>> - fence = dma_fence_get_rcu_safe(&syncobj->fence);
> >>> - rcu_read_unlock();
> >>> -
> >>> - return fence;
> >>> -}
> >>> -
> >>> struct drm_syncobj *drm_syncobj_find(struct drm_file
> >>> *file_private,
> >>> u32 handle);
> >>> void drm_syncobj_replace_fence(struct drm_syncobj *syncobj, u64
> >>> point, @@ -142,5 +141,7 @@ int drm_syncobj_create(struct
> drm_syncobj
> >>> **out_syncobj, uint32_t flags,
> >>> int drm_syncobj_get_handle(struct drm_file *file_private,
> >>> struct drm_syncobj *syncobj, u32 *handle);
> >>> int drm_syncobj_get_fd(struct drm_syncobj *syncobj, int *p_fd);
> >>> +int drm_syncobj_lookup_fence(struct drm_syncobj *syncobj, u64
> >>> +point,
> >>> + struct dma_fence **fence);
> >>> #endif
> >>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index
> >>> 300f336633f2..cebdb2541eb7 100644
> >>> --- a/include/uapi/drm/drm.h
> >>> +++ b/include/uapi/drm/drm.h
> >>> @@ -717,6 +717,7 @@ struct drm_prime_handle {
> >>> struct drm_syncobj_create {
> >>> __u32 handle;
> >>> #define DRM_SYNCOBJ_CREATE_SIGNALED (1 << 0)
> >>> +#define DRM_SYNCOBJ_CREATE_TYPE_TIMELINE (1 << 1)
> >>> __u32 flags;
> >>> };
> >>
> >
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] [RFC]drm: add syncobj timeline support v6
[not found] ` <BY1PR12MB05021AF8F3FD552CC133BCC6B41C0-PicGAnIBOobrCwm+z9iKNgdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
@ 2018-09-19 8:07 ` Christian König
[not found] ` <3ed55b7b-5a61-5001-e767-96039dc07bbe-5C7GfCeVMHo@public.gmane.org>
0 siblings, 1 reply; 11+ messages in thread
From: Christian König @ 2018-09-19 8:07 UTC (permalink / raw)
To: Zhou, David(ChunMing), dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Cc: Dave Airlie, Rakos, Daniel,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Daniel Vetter
Am 19.09.2018 um 10:03 schrieb Zhou, David(ChunMing):
>
>> -----Original Message-----
>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
>> Christian K?nig
>> Sent: Wednesday, September 19, 2018 3:45 PM
>> To: Zhou, David(ChunMing) <David1.Zhou@amd.com>; Zhou,
>> David(ChunMing) <David1.Zhou@amd.com>; dri-
>> devel@lists.freedesktop.org
>> Cc: Dave Airlie <airlied@redhat.com>; Rakos, Daniel
>> <Daniel.Rakos@amd.com>; Daniel Vetter <daniel@ffwll.ch>; amd-
>> gfx@lists.freedesktop.org
>> Subject: Re: [PATCH 1/4] [RFC]drm: add syncobj timeline support v6
>>
>> Am 19.09.2018 um 09:32 schrieb zhoucm1:
>>>
>>> On 2018年09月19日 15:18, Christian König wrote:
>>>> Am 19.09.2018 um 06:26 schrieb Chunming Zhou:
>>> [snip]
>>>>> *fence = NULL;
>>>>> drm_syncobj_add_callback_locked(syncobj, cb, func); @@
>>>>> -164,6 +177,153 @@ void drm_syncobj_remove_callback(struct
>>>>> drm_syncobj *syncobj,
>>>>> spin_unlock(&syncobj->lock);
>>>>> }
>>>>> +static void drm_syncobj_timeline_init(struct drm_syncobj
>>>>> *syncobj)
>>>> We still have _timeline_ in the name here.
>>> the func is relevant to timeline members, or which name is proper?
>> Yeah, but we now use the timeline implementation for the individual syncobj
>> as well.
>>
>> Not a big issue, but I would just name it
>> drm_syncobj_init()/drm_syncobj_fini.
> There is already drm_syncobj_init/fini in drm_syncboj.c , any other name can be suggested?
Hui what? I actually checked that there is no
drm_syncobj_init()/drm_syncobj_fini() in drm_syncobj.c before suggesting
it. Am I missing something?
>>>>> +{
>>>>> + spin_lock(&syncobj->lock);
>>>>> + syncobj->timeline_context = dma_fence_context_alloc(1);
>>> [snip]
>>>>> +}
>>>>> +
>>>>> +int drm_syncobj_lookup_fence(struct drm_syncobj *syncobj, u64
>>>>> +point,
>>>>> + struct dma_fence **fence) {
>>>>> +
>>>>> + return drm_syncobj_search_fence(syncobj, point,
>>>>> + DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT,
>>>> I still have a bad feeling setting that flag as default cause it
>>>> might change the behavior for the UAPI.
>>>>
>>>> Maybe export drm_syncobj_search_fence directly? E.g. with the flags
>>>> parameter.
>>> previous v5 indeed do this, you let me wrap it, need change back?
>> No, the problem is that drm_syncobj_find_fence() is still using
>> drm_syncobj_lookup_fence() which sets the flag instead of
>> drm_syncobj_search_fence() without the flag.
>>
>> That changes the UAPI behavior because previously we would have returned
>> an error code and now we block for a fence to appear.
>>
>> So I think the right solution would be to add the flags parameter to
>> drm_syncobj_find_fence() and let the driver decide if we need to block or
>> get -ENOENT.
> Got your means,
> Exporting flag in func is easy,
> but driver doesn't pass flag, which flag is proper by default? We still need to give a default flag in patch, don't we?
Well proper solution is to keep the old behavior as it is for now.
So passing 0 as flag by default and making sure we get a -ENOENT in that
case sounds like the right approach to me.
Adding the DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT flag can happen when
the driver starts to provide a proper point as well.
Christian.
>
> Thanks,
> David Zhou
>
>> Regards,
>> Christian.
>>
>>> Regards,
>>> David Zhou
>>>> Regards,
>>>> Christian.
>>>>
>>>>> + fence);
>>>>> +}
>>>>> +EXPORT_SYMBOL(drm_syncobj_lookup_fence);
>>>>> +
>>>>> /**
>>>>> * drm_syncobj_find_fence - lookup and reference the fence in a
>>>>> sync object
>>>>> * @file_private: drm file private pointer @@ -228,7 +443,7 @@
>>>>> static int drm_syncobj_assign_null_handle(struct
>>>>> drm_syncobj *syncobj)
>>>>> * @fence: out parameter for the fence
>>>>> *
>>>>> * This is just a convenience function that combines
>>>>> drm_syncobj_find() and
>>>>> - * drm_syncobj_fence_get().
>>>>> + * drm_syncobj_lookup_fence().
>>>>> *
>>>>> * Returns 0 on success or a negative error value on failure. On
>>>>> success @fence
>>>>> * contains a reference to the fence, which must be released by
>>>>> calling @@ -236,18 +451,11 @@ static int
>>>>> drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj)
>>>>> */
>>>>> int drm_syncobj_find_fence(struct drm_file *file_private,
>>>>> u32 handle, u64 point,
>>>>> - struct dma_fence **fence) -{
>>>>> + struct dma_fence **fence) {
>>>>> struct drm_syncobj *syncobj = drm_syncobj_find(file_private,
>>>>> handle);
>>>>> - int ret = 0;
>>>>> -
>>>>> - if (!syncobj)
>>>>> - return -ENOENT;
>>>>> + int ret;
>>>>> - *fence = drm_syncobj_fence_get(syncobj);
>>>>> - if (!*fence) {
>>>>> - ret = -EINVAL;
>>>>> - }
>>>>> + ret = drm_syncobj_lookup_fence(syncobj, point, fence);
>>>>> drm_syncobj_put(syncobj);
>>>>> return ret;
>>>>> }
>>>>> @@ -264,7 +472,7 @@ void drm_syncobj_free(struct kref *kref)
>>>>> struct drm_syncobj *syncobj = container_of(kref,
>>>>> struct drm_syncobj,
>>>>> refcount);
>>>>> - drm_syncobj_replace_fence(syncobj, 0, NULL);
>>>>> + drm_syncobj_timeline_fini(syncobj);
>>>>> kfree(syncobj);
>>>>> }
>>>>> EXPORT_SYMBOL(drm_syncobj_free);
>>>>> @@ -294,6 +502,11 @@ int drm_syncobj_create(struct drm_syncobj
>>>>> **out_syncobj, uint32_t flags,
>>>>> kref_init(&syncobj->refcount);
>>>>> INIT_LIST_HEAD(&syncobj->cb_list);
>>>>> spin_lock_init(&syncobj->lock);
>>>>> + if (flags & DRM_SYNCOBJ_CREATE_TYPE_TIMELINE)
>>>>> + syncobj->type = DRM_SYNCOBJ_TYPE_TIMELINE;
>>>>> + else
>>>>> + syncobj->type = DRM_SYNCOBJ_TYPE_INDIVIDUAL;
>>>>> + drm_syncobj_timeline_init(syncobj);
>>>>> if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) {
>>>>> ret = drm_syncobj_assign_null_handle(syncobj);
>>>>> @@ -576,7 +789,8 @@ drm_syncobj_create_ioctl(struct drm_device
>> *dev,
>>>>> void *data,
>>>>> return -ENODEV;
>>>>> /* no valid flags yet */
>>>>> - if (args->flags & ~DRM_SYNCOBJ_CREATE_SIGNALED)
>>>>> + if (args->flags & ~(DRM_SYNCOBJ_CREATE_SIGNALED |
>>>>> + DRM_SYNCOBJ_CREATE_TYPE_TIMELINE))
>>>>> return -EINVAL;
>>>>> return drm_syncobj_create_as_handle(file_private,
>>>>> @@ -669,9 +883,8 @@ static void syncobj_wait_syncobj_func(struct
>>>>> drm_syncobj *syncobj,
>>>>> struct syncobj_wait_entry *wait =
>>>>> container_of(cb, struct syncobj_wait_entry, syncobj_cb);
>>>>> - /* This happens inside the syncobj lock */
>>>>> - wait->fence =
>>>>> dma_fence_get(rcu_dereference_protected(syncobj->fence,
>>>>> - lockdep_is_held(&syncobj->lock)));
>>>>> + drm_syncobj_search_fence(syncobj, 0, 0, &wait->fence);
>>>>> +
>>>>> wake_up_process(wait->task);
>>>>> }
>>>>> @@ -698,7 +911,8 @@ static signed long
>>>>> drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
>>>>> signaled_count = 0;
>>>>> for (i = 0; i < count; ++i) {
>>>>> entries[i].task = current;
>>>>> - entries[i].fence = drm_syncobj_fence_get(syncobjs[i]);
>>>>> + ret = drm_syncobj_search_fence(syncobjs[i], 0, 0,
>>>>> + &entries[i].fence);
>>>>> if (!entries[i].fence) {
>>>>> if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
>>>>> continue;
>>>>> @@ -970,12 +1184,19 @@ drm_syncobj_reset_ioctl(struct drm_device
>>>>> *dev, void *data,
>>>>> if (ret < 0)
>>>>> return ret;
>>>>> - for (i = 0; i < args->count_handles; i++)
>>>>> - drm_syncobj_replace_fence(syncobjs[i], 0, NULL);
>>>>> -
>>>>> + for (i = 0; i < args->count_handles; i++) {
>>>>> + if (syncobjs[i]->type == DRM_SYNCOBJ_TYPE_TIMELINE) {
>>>>> + DRM_ERROR("timeline syncobj cannot reset!\n");
>>>>> + ret = -EINVAL;
>>>>> + goto out;
>>>>> + }
>>>>> + drm_syncobj_timeline_fini(syncobjs[i]);
>>>>> + drm_syncobj_timeline_init(syncobjs[i]);
>>>>> + }
>>>>> +out:
>>>>> drm_syncobj_array_free(syncobjs, args->count_handles);
>>>>> - return 0;
>>>>> + return ret;
>>>>> }
>>>>> int
>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>>>> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>>>> index 0a8d2d64f380..a040fa3281df 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>>>> @@ -2137,7 +2137,7 @@ await_fence_array(struct i915_execbuffer *eb,
>>>>> if (!(flags & I915_EXEC_FENCE_WAIT))
>>>>> continue;
>>>>> - fence = drm_syncobj_fence_get(syncobj);
>>>>> + drm_syncobj_lookup_fence(syncobj, 0, &fence);
>>>>> if (!fence)
>>>>> return -EINVAL;
>>>>> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
>>>>> index 425432b85a87..6dac479fedfe 100644
>>>>> --- a/include/drm/drm_syncobj.h
>>>>> +++ b/include/drm/drm_syncobj.h
>>>>> @@ -30,6 +30,11 @@
>>>>> struct drm_syncobj_cb;
>>>>> +enum drm_syncobj_type {
>>>>> + DRM_SYNCOBJ_TYPE_INDIVIDUAL,
>>>>> + DRM_SYNCOBJ_TYPE_TIMELINE
>>>>> +};
>>>>> +
>>>>> /**
>>>>> * struct drm_syncobj - sync object.
>>>>> *
>>>>> @@ -41,19 +46,36 @@ struct drm_syncobj {
>>>>> */
>>>>> struct kref refcount;
>>>>> /**
>>>>> - * @fence:
>>>>> - * NULL or a pointer to the fence bound to this object.
>>>>> - *
>>>>> - * This field should not be used directly. Use
>>>>> drm_syncobj_fence_get()
>>>>> - * and drm_syncobj_replace_fence() instead.
>>>>> + * @type: indicate syncobj type
>>>>> + */
>>>>> + enum drm_syncobj_type type;
>>>>> + /**
>>>>> + * @wq: wait signal operation work queue
>>>>> + */
>>>>> + wait_queue_head_t wq;
>>>>> + /**
>>>>> + * @timeline_context: fence context used by timeline
>>>>> */
>>>>> - struct dma_fence __rcu *fence;
>>>>> + u64 timeline_context;
>>>>> /**
>>>>> - * @cb_list: List of callbacks to call when the &fence gets
>>>>> replaced.
>>>>> + * @timeline: syncobj timeline value, which indicates point is
>>>>> signaled.
>>>>> */
>>>>> + u64 timeline;
>>>>> + /**
>>>>> + * @signal_point: which indicates the latest signaler point.
>>>>> + */
>>>>> + u64 signal_point;
>>>>> + /**
>>>>> + * @signal_pt_list: signaler point list.
>>>>> + */
>>>>> + struct list_head signal_pt_list;
>>>>> +
>>>>> + /**
>>>>> + * @cb_list: List of callbacks to call when the &fence gets
>>>>> replaced.
>>>>> + */
>>>>> struct list_head cb_list;
>>>>> /**
>>>>> - * @lock: Protects &cb_list and write-locks &fence.
>>>>> + * @lock: Protects syncobj list and write-locks &fence.
>>>>> */
>>>>> spinlock_t lock;
>>>>> /**
>>>>> @@ -68,7 +90,7 @@ typedef void (*drm_syncobj_func_t)(struct
>>>>> drm_syncobj *syncobj,
>>>>> /**
>>>>> * struct drm_syncobj_cb - callback for drm_syncobj_add_callback
>>>>> * @node: used by drm_syncob_add_callback to append this struct to
>>>>> - * &drm_syncobj.cb_list
>>>>> + * &drm_syncobj.cb_list
>>>>> * @func: drm_syncobj_func_t to call
>>>>> *
>>>>> * This struct will be initialized by drm_syncobj_add_callback,
>>>>> additional @@ -106,29 +128,6 @@ drm_syncobj_put(struct drm_syncobj
>>>>> *obj)
>>>>> kref_put(&obj->refcount, drm_syncobj_free);
>>>>> }
>>>>> -/**
>>>>> - * drm_syncobj_fence_get - get a reference to a fence in a sync
>>>>> object
>>>>> - * @syncobj: sync object.
>>>>> - *
>>>>> - * This acquires additional reference to &drm_syncobj.fence
>>>>> contained in @obj,
>>>>> - * if not NULL. It is illegal to call this without already holding
>>>>> a reference.
>>>>> - * No locks required.
>>>>> - *
>>>>> - * Returns:
>>>>> - * Either the fence of @obj or NULL if there's none.
>>>>> - */
>>>>> -static inline struct dma_fence *
>>>>> -drm_syncobj_fence_get(struct drm_syncobj *syncobj) -{
>>>>> - struct dma_fence *fence;
>>>>> -
>>>>> - rcu_read_lock();
>>>>> - fence = dma_fence_get_rcu_safe(&syncobj->fence);
>>>>> - rcu_read_unlock();
>>>>> -
>>>>> - return fence;
>>>>> -}
>>>>> -
>>>>> struct drm_syncobj *drm_syncobj_find(struct drm_file
>>>>> *file_private,
>>>>> u32 handle);
>>>>> void drm_syncobj_replace_fence(struct drm_syncobj *syncobj, u64
>>>>> point, @@ -142,5 +141,7 @@ int drm_syncobj_create(struct
>> drm_syncobj
>>>>> **out_syncobj, uint32_t flags,
>>>>> int drm_syncobj_get_handle(struct drm_file *file_private,
>>>>> struct drm_syncobj *syncobj, u32 *handle);
>>>>> int drm_syncobj_get_fd(struct drm_syncobj *syncobj, int *p_fd);
>>>>> +int drm_syncobj_lookup_fence(struct drm_syncobj *syncobj, u64
>>>>> +point,
>>>>> + struct dma_fence **fence);
>>>>> #endif
>>>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index
>>>>> 300f336633f2..cebdb2541eb7 100644
>>>>> --- a/include/uapi/drm/drm.h
>>>>> +++ b/include/uapi/drm/drm.h
>>>>> @@ -717,6 +717,7 @@ struct drm_prime_handle {
>>>>> struct drm_syncobj_create {
>>>>> __u32 handle;
>>>>> #define DRM_SYNCOBJ_CREATE_SIGNALED (1 << 0)
>>>>> +#define DRM_SYNCOBJ_CREATE_TYPE_TIMELINE (1 << 1)
>>>>> __u32 flags;
>>>>> };
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] [RFC]drm: add syncobj timeline support v6
[not found] ` <3ed55b7b-5a61-5001-e767-96039dc07bbe-5C7GfCeVMHo@public.gmane.org>
@ 2018-09-19 9:00 ` zhoucm1
0 siblings, 0 replies; 11+ messages in thread
From: zhoucm1 @ 2018-09-19 9:00 UTC (permalink / raw)
To: Christian König, Zhou, David(ChunMing),
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Cc: Dave Airlie, Rakos, Daniel,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Daniel Vetter
On 2018年09月19日 16:07, Christian König wrote:
> Am 19.09.2018 um 10:03 schrieb Zhou, David(ChunMing):
>>
>>> -----Original Message-----
>>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
>>> Christian K?nig
>>> Sent: Wednesday, September 19, 2018 3:45 PM
>>> To: Zhou, David(ChunMing) <David1.Zhou@amd.com>; Zhou,
>>> David(ChunMing) <David1.Zhou@amd.com>; dri-
>>> devel@lists.freedesktop.org
>>> Cc: Dave Airlie <airlied@redhat.com>; Rakos, Daniel
>>> <Daniel.Rakos@amd.com>; Daniel Vetter <daniel@ffwll.ch>; amd-
>>> gfx@lists.freedesktop.org
>>> Subject: Re: [PATCH 1/4] [RFC]drm: add syncobj timeline support v6
>>>
>>> Am 19.09.2018 um 09:32 schrieb zhoucm1:
>>>>
>>>> On 2018年09月19日 15:18, Christian König wrote:
>>>>> Am 19.09.2018 um 06:26 schrieb Chunming Zhou:
>>>> [snip]
>>>>>> *fence = NULL;
>>>>>> drm_syncobj_add_callback_locked(syncobj, cb, func); @@
>>>>>> -164,6 +177,153 @@ void drm_syncobj_remove_callback(struct
>>>>>> drm_syncobj *syncobj,
>>>>>> spin_unlock(&syncobj->lock);
>>>>>> }
>>>>>> +static void drm_syncobj_timeline_init(struct drm_syncobj
>>>>>> *syncobj)
>>>>> We still have _timeline_ in the name here.
>>>> the func is relevant to timeline members, or which name is proper?
>>> Yeah, but we now use the timeline implementation for the individual
>>> syncobj
>>> as well.
>>>
>>> Not a big issue, but I would just name it
>>> drm_syncobj_init()/drm_syncobj_fini.
>> There is already drm_syncobj_init/fini in drm_syncboj.c , any other
>> name can be suggested?
>
> Hui what? I actually checked that there is no
> drm_syncobj_init()/drm_syncobj_fini() in drm_syncobj.c before
> suggesting it. Am I missing something?
messed syncobj_create/destroy in brain :(
>
>>>>>> +{
>>>>>> + spin_lock(&syncobj->lock);
>>>>>> + syncobj->timeline_context = dma_fence_context_alloc(1);
>>>> [snip]
>>>>>> +}
>>>>>> +
>>>>>> +int drm_syncobj_lookup_fence(struct drm_syncobj *syncobj, u64
>>>>>> +point,
>>>>>> + struct dma_fence **fence) {
>>>>>> +
>>>>>> + return drm_syncobj_search_fence(syncobj, point,
>>>>>> + DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT,
>>>>> I still have a bad feeling setting that flag as default cause it
>>>>> might change the behavior for the UAPI.
>>>>>
>>>>> Maybe export drm_syncobj_search_fence directly? E.g. with the flags
>>>>> parameter.
>>>> previous v5 indeed do this, you let me wrap it, need change back?
>>> No, the problem is that drm_syncobj_find_fence() is still using
>>> drm_syncobj_lookup_fence() which sets the flag instead of
>>> drm_syncobj_search_fence() without the flag.
>>>
>>> That changes the UAPI behavior because previously we would have
>>> returned
>>> an error code and now we block for a fence to appear.
>>>
>>> So I think the right solution would be to add the flags parameter to
>>> drm_syncobj_find_fence() and let the driver decide if we need to
>>> block or
>>> get -ENOENT.
>> Got your means,
>> Exporting flag in func is easy,
>> but driver doesn't pass flag, which flag is proper by default? We
>> still need to give a default flag in patch, don't we?
>
> Well proper solution is to keep the old behavior as it is for now.
>
> So passing 0 as flag by default and making sure we get a -ENOENT in
> that case sounds like the right approach to me.
>
> Adding the DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT flag can happen when
> the driver starts to provide a proper point as well.
Sounds more flexible.
v7 will come soon.
thanks,
David Zhou
>
> Christian.
>
>>
>> Thanks,
>> David Zhou
>>
>>> Regards,
>>> Christian.
>>>
>>>> Regards,
>>>> David Zhou
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>>> + fence);
>>>>>> +}
>>>>>> +EXPORT_SYMBOL(drm_syncobj_lookup_fence);
>>>>>> +
>>>>>> /**
>>>>>> * drm_syncobj_find_fence - lookup and reference the fence in a
>>>>>> sync object
>>>>>> * @file_private: drm file private pointer @@ -228,7 +443,7 @@
>>>>>> static int drm_syncobj_assign_null_handle(struct
>>>>>> drm_syncobj *syncobj)
>>>>>> * @fence: out parameter for the fence
>>>>>> *
>>>>>> * This is just a convenience function that combines
>>>>>> drm_syncobj_find() and
>>>>>> - * drm_syncobj_fence_get().
>>>>>> + * drm_syncobj_lookup_fence().
>>>>>> *
>>>>>> * Returns 0 on success or a negative error value on failure. On
>>>>>> success @fence
>>>>>> * contains a reference to the fence, which must be released by
>>>>>> calling @@ -236,18 +451,11 @@ static int
>>>>>> drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj)
>>>>>> */
>>>>>> int drm_syncobj_find_fence(struct drm_file *file_private,
>>>>>> u32 handle, u64 point,
>>>>>> - struct dma_fence **fence) -{
>>>>>> + struct dma_fence **fence) {
>>>>>> struct drm_syncobj *syncobj = drm_syncobj_find(file_private,
>>>>>> handle);
>>>>>> - int ret = 0;
>>>>>> -
>>>>>> - if (!syncobj)
>>>>>> - return -ENOENT;
>>>>>> + int ret;
>>>>>> - *fence = drm_syncobj_fence_get(syncobj);
>>>>>> - if (!*fence) {
>>>>>> - ret = -EINVAL;
>>>>>> - }
>>>>>> + ret = drm_syncobj_lookup_fence(syncobj, point, fence);
>>>>>> drm_syncobj_put(syncobj);
>>>>>> return ret;
>>>>>> }
>>>>>> @@ -264,7 +472,7 @@ void drm_syncobj_free(struct kref *kref)
>>>>>> struct drm_syncobj *syncobj = container_of(kref,
>>>>>> struct drm_syncobj,
>>>>>> refcount);
>>>>>> - drm_syncobj_replace_fence(syncobj, 0, NULL);
>>>>>> + drm_syncobj_timeline_fini(syncobj);
>>>>>> kfree(syncobj);
>>>>>> }
>>>>>> EXPORT_SYMBOL(drm_syncobj_free);
>>>>>> @@ -294,6 +502,11 @@ int drm_syncobj_create(struct drm_syncobj
>>>>>> **out_syncobj, uint32_t flags,
>>>>>> kref_init(&syncobj->refcount);
>>>>>> INIT_LIST_HEAD(&syncobj->cb_list);
>>>>>> spin_lock_init(&syncobj->lock);
>>>>>> + if (flags & DRM_SYNCOBJ_CREATE_TYPE_TIMELINE)
>>>>>> + syncobj->type = DRM_SYNCOBJ_TYPE_TIMELINE;
>>>>>> + else
>>>>>> + syncobj->type = DRM_SYNCOBJ_TYPE_INDIVIDUAL;
>>>>>> + drm_syncobj_timeline_init(syncobj);
>>>>>> if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) {
>>>>>> ret = drm_syncobj_assign_null_handle(syncobj);
>>>>>> @@ -576,7 +789,8 @@ drm_syncobj_create_ioctl(struct drm_device
>>> *dev,
>>>>>> void *data,
>>>>>> return -ENODEV;
>>>>>> /* no valid flags yet */
>>>>>> - if (args->flags & ~DRM_SYNCOBJ_CREATE_SIGNALED)
>>>>>> + if (args->flags & ~(DRM_SYNCOBJ_CREATE_SIGNALED |
>>>>>> + DRM_SYNCOBJ_CREATE_TYPE_TIMELINE))
>>>>>> return -EINVAL;
>>>>>> return drm_syncobj_create_as_handle(file_private,
>>>>>> @@ -669,9 +883,8 @@ static void syncobj_wait_syncobj_func(struct
>>>>>> drm_syncobj *syncobj,
>>>>>> struct syncobj_wait_entry *wait =
>>>>>> container_of(cb, struct syncobj_wait_entry, syncobj_cb);
>>>>>> - /* This happens inside the syncobj lock */
>>>>>> - wait->fence =
>>>>>> dma_fence_get(rcu_dereference_protected(syncobj->fence,
>>>>>> - lockdep_is_held(&syncobj->lock)));
>>>>>> + drm_syncobj_search_fence(syncobj, 0, 0, &wait->fence);
>>>>>> +
>>>>>> wake_up_process(wait->task);
>>>>>> }
>>>>>> @@ -698,7 +911,8 @@ static signed long
>>>>>> drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
>>>>>> signaled_count = 0;
>>>>>> for (i = 0; i < count; ++i) {
>>>>>> entries[i].task = current;
>>>>>> - entries[i].fence = drm_syncobj_fence_get(syncobjs[i]);
>>>>>> + ret = drm_syncobj_search_fence(syncobjs[i], 0, 0,
>>>>>> + &entries[i].fence);
>>>>>> if (!entries[i].fence) {
>>>>>> if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
>>>>>> continue;
>>>>>> @@ -970,12 +1184,19 @@ drm_syncobj_reset_ioctl(struct drm_device
>>>>>> *dev, void *data,
>>>>>> if (ret < 0)
>>>>>> return ret;
>>>>>> - for (i = 0; i < args->count_handles; i++)
>>>>>> - drm_syncobj_replace_fence(syncobjs[i], 0, NULL);
>>>>>> -
>>>>>> + for (i = 0; i < args->count_handles; i++) {
>>>>>> + if (syncobjs[i]->type == DRM_SYNCOBJ_TYPE_TIMELINE) {
>>>>>> + DRM_ERROR("timeline syncobj cannot reset!\n");
>>>>>> + ret = -EINVAL;
>>>>>> + goto out;
>>>>>> + }
>>>>>> + drm_syncobj_timeline_fini(syncobjs[i]);
>>>>>> + drm_syncobj_timeline_init(syncobjs[i]);
>>>>>> + }
>>>>>> +out:
>>>>>> drm_syncobj_array_free(syncobjs, args->count_handles);
>>>>>> - return 0;
>>>>>> + return ret;
>>>>>> }
>>>>>> int
>>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>>>>> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>>>>> index 0a8d2d64f380..a040fa3281df 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>>>>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>>>>> @@ -2137,7 +2137,7 @@ await_fence_array(struct i915_execbuffer *eb,
>>>>>> if (!(flags & I915_EXEC_FENCE_WAIT))
>>>>>> continue;
>>>>>> - fence = drm_syncobj_fence_get(syncobj);
>>>>>> + drm_syncobj_lookup_fence(syncobj, 0, &fence);
>>>>>> if (!fence)
>>>>>> return -EINVAL;
>>>>>> diff --git a/include/drm/drm_syncobj.h
>>>>>> b/include/drm/drm_syncobj.h
>>>>>> index 425432b85a87..6dac479fedfe 100644
>>>>>> --- a/include/drm/drm_syncobj.h
>>>>>> +++ b/include/drm/drm_syncobj.h
>>>>>> @@ -30,6 +30,11 @@
>>>>>> struct drm_syncobj_cb;
>>>>>> +enum drm_syncobj_type {
>>>>>> + DRM_SYNCOBJ_TYPE_INDIVIDUAL,
>>>>>> + DRM_SYNCOBJ_TYPE_TIMELINE
>>>>>> +};
>>>>>> +
>>>>>> /**
>>>>>> * struct drm_syncobj - sync object.
>>>>>> *
>>>>>> @@ -41,19 +46,36 @@ struct drm_syncobj {
>>>>>> */
>>>>>> struct kref refcount;
>>>>>> /**
>>>>>> - * @fence:
>>>>>> - * NULL or a pointer to the fence bound to this object.
>>>>>> - *
>>>>>> - * This field should not be used directly. Use
>>>>>> drm_syncobj_fence_get()
>>>>>> - * and drm_syncobj_replace_fence() instead.
>>>>>> + * @type: indicate syncobj type
>>>>>> + */
>>>>>> + enum drm_syncobj_type type;
>>>>>> + /**
>>>>>> + * @wq: wait signal operation work queue
>>>>>> + */
>>>>>> + wait_queue_head_t wq;
>>>>>> + /**
>>>>>> + * @timeline_context: fence context used by timeline
>>>>>> */
>>>>>> - struct dma_fence __rcu *fence;
>>>>>> + u64 timeline_context;
>>>>>> /**
>>>>>> - * @cb_list: List of callbacks to call when the &fence gets
>>>>>> replaced.
>>>>>> + * @timeline: syncobj timeline value, which indicates point is
>>>>>> signaled.
>>>>>> */
>>>>>> + u64 timeline;
>>>>>> + /**
>>>>>> + * @signal_point: which indicates the latest signaler point.
>>>>>> + */
>>>>>> + u64 signal_point;
>>>>>> + /**
>>>>>> + * @signal_pt_list: signaler point list.
>>>>>> + */
>>>>>> + struct list_head signal_pt_list;
>>>>>> +
>>>>>> + /**
>>>>>> + * @cb_list: List of callbacks to call when the &fence gets
>>>>>> replaced.
>>>>>> + */
>>>>>> struct list_head cb_list;
>>>>>> /**
>>>>>> - * @lock: Protects &cb_list and write-locks &fence.
>>>>>> + * @lock: Protects syncobj list and write-locks &fence.
>>>>>> */
>>>>>> spinlock_t lock;
>>>>>> /**
>>>>>> @@ -68,7 +90,7 @@ typedef void (*drm_syncobj_func_t)(struct
>>>>>> drm_syncobj *syncobj,
>>>>>> /**
>>>>>> * struct drm_syncobj_cb - callback for drm_syncobj_add_callback
>>>>>> * @node: used by drm_syncob_add_callback to append this
>>>>>> struct to
>>>>>> - * &drm_syncobj.cb_list
>>>>>> + * &drm_syncobj.cb_list
>>>>>> * @func: drm_syncobj_func_t to call
>>>>>> *
>>>>>> * This struct will be initialized by drm_syncobj_add_callback,
>>>>>> additional @@ -106,29 +128,6 @@ drm_syncobj_put(struct drm_syncobj
>>>>>> *obj)
>>>>>> kref_put(&obj->refcount, drm_syncobj_free);
>>>>>> }
>>>>>> -/**
>>>>>> - * drm_syncobj_fence_get - get a reference to a fence in a sync
>>>>>> object
>>>>>> - * @syncobj: sync object.
>>>>>> - *
>>>>>> - * This acquires additional reference to &drm_syncobj.fence
>>>>>> contained in @obj,
>>>>>> - * if not NULL. It is illegal to call this without already holding
>>>>>> a reference.
>>>>>> - * No locks required.
>>>>>> - *
>>>>>> - * Returns:
>>>>>> - * Either the fence of @obj or NULL if there's none.
>>>>>> - */
>>>>>> -static inline struct dma_fence *
>>>>>> -drm_syncobj_fence_get(struct drm_syncobj *syncobj) -{
>>>>>> - struct dma_fence *fence;
>>>>>> -
>>>>>> - rcu_read_lock();
>>>>>> - fence = dma_fence_get_rcu_safe(&syncobj->fence);
>>>>>> - rcu_read_unlock();
>>>>>> -
>>>>>> - return fence;
>>>>>> -}
>>>>>> -
>>>>>> struct drm_syncobj *drm_syncobj_find(struct drm_file
>>>>>> *file_private,
>>>>>> u32 handle);
>>>>>> void drm_syncobj_replace_fence(struct drm_syncobj *syncobj, u64
>>>>>> point, @@ -142,5 +141,7 @@ int drm_syncobj_create(struct
>>> drm_syncobj
>>>>>> **out_syncobj, uint32_t flags,
>>>>>> int drm_syncobj_get_handle(struct drm_file *file_private,
>>>>>> struct drm_syncobj *syncobj, u32 *handle);
>>>>>> int drm_syncobj_get_fd(struct drm_syncobj *syncobj, int *p_fd);
>>>>>> +int drm_syncobj_lookup_fence(struct drm_syncobj *syncobj, u64
>>>>>> +point,
>>>>>> + struct dma_fence **fence);
>>>>>> #endif
>>>>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index
>>>>>> 300f336633f2..cebdb2541eb7 100644
>>>>>> --- a/include/uapi/drm/drm.h
>>>>>> +++ b/include/uapi/drm/drm.h
>>>>>> @@ -717,6 +717,7 @@ struct drm_prime_handle {
>>>>>> struct drm_syncobj_create {
>>>>>> __u32 handle;
>>>>>> #define DRM_SYNCOBJ_CREATE_SIGNALED (1 << 0)
>>>>>> +#define DRM_SYNCOBJ_CREATE_TYPE_TIMELINE (1 << 1)
>>>>>> __u32 flags;
>>>>>> };
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-09-19 9:00 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-19 4:26 [PATCH 1/4] [RFC]drm: add syncobj timeline support v6 Chunming Zhou
[not found] ` <20180919042659.3769-1-david1.zhou-5C7GfCeVMHo@public.gmane.org>
2018-09-19 4:26 ` [PATCH 2/4] drm: add support of syncobj timeline point wait v2 Chunming Zhou
2018-09-19 4:26 ` [PATCH 3/4] drm: add timeline syncobj payload query ioctl Chunming Zhou
2018-09-19 4:26 ` [PATCH 4/4] drm/amdgpu: add timeline support in amdgpu CS Chunming Zhou
2018-09-19 7:18 ` [PATCH 1/4] [RFC]drm: add syncobj timeline support v6 Christian König
[not found] ` <04e91247-387c-ed5d-0793-9eb1d37d12bd-5C7GfCeVMHo@public.gmane.org>
2018-09-19 7:32 ` zhoucm1
2018-09-19 7:32 ` zhoucm1
[not found] ` <392512eb-271d-3d16-263b-f2429dedfffc-5C7GfCeVMHo@public.gmane.org>
2018-09-19 7:45 ` Christian König
[not found] ` <9bdce2dd-2129-3ed9-56f0-db4b14005194-5C7GfCeVMHo@public.gmane.org>
2018-09-19 8:03 ` Zhou, David(ChunMing)
[not found] ` <BY1PR12MB05021AF8F3FD552CC133BCC6B41C0-PicGAnIBOobrCwm+z9iKNgdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
2018-09-19 8:07 ` Christian König
[not found] ` <3ed55b7b-5a61-5001-e767-96039dc07bbe-5C7GfCeVMHo@public.gmane.org>
2018-09-19 9:00 ` zhoucm1
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.