Re: [PATCH 1/4] [RFC]drm: add syncobj timeline support v6

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Am 19.09.2018 um 10:03 schrieb Zhou, David(ChunMing):

-----Original Message-----
From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of
Christian K?nig
Sent: Wednesday, September 19, 2018 3:45 PM
To: Zhou, David(ChunMing) <David1.Zhou@xxxxxxx>; Zhou,
David(ChunMing) <David1.Zhou@xxxxxxx>; dri-
devel@xxxxxxxxxxxxxxxxxxxxx
Cc: Dave Airlie <airlied@xxxxxxxxxx>; Rakos, Daniel
<Daniel.Rakos@xxxxxxx>; Daniel Vetter <daniel@xxxxxxxx>; amd-
gfx@xxxxxxxxxxxxxxxxxxxxx
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux