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 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;
  };



_______________________________________________
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