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

2018-09-19 Thread zhoucm1 



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  On Behalf Of
Christian K?nig
Sent: Wednesday, September 19, 2018 3:45 PM
To: Zhou, David(ChunMing) ; Zhou,
David(ChunMing) ; dri-
de...@lists.freedesktop.org
Cc: Dave Airlie ; Rakos, Daniel
; Daniel Vetter ; amd-
g...@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(>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(>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(>refcount);
   INIT_LIST_HEAD(>cb_list);
   spin_lock_init(>loc

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

2018-09-19 Thread Christian König 

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



-Original Message-
From: amd-gfx  On Behalf Of
Christian K?nig
Sent: Wednesday, September 19, 2018 3:45 PM
To: Zhou, David(ChunMing) ; Zhou,
David(ChunMing) ; dri-
de...@lists.freedesktop.org
Cc: Dave Airlie ; Rakos, Daniel
; Daniel Vetter ; amd-
g...@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(>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(>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(>refcount);
   INIT_LIST_HEAD(>cb_list);
   spin_lock_init(>lock);
+    if (flags & DRM_SYNCOBJ_CREATE_TYPE_TIMELINE)
+    syncobj->type = DRM_SYNCOBJ_TYPE_TIMELINE;
+    el

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

2018-09-19 Thread Zhou, David(ChunMing) 


> -Original Message-
> From: amd-gfx  On Behalf Of
> Christian K?nig
> Sent: Wednesday, September 19, 2018 3:45 PM
> To: Zhou, David(ChunMing) ; Zhou,
> David(ChunMing) ; dri-
> de...@lists.freedesktop.org
> Cc: Dave Airlie ; Rakos, Daniel
> ; Daniel Vetter ; amd-
> g...@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(>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(>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;
> >>>   -    *fe

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

2018-09-19 Thread Christian König 

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(>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(>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(>refcount);
  INIT_LIST_HEAD(>cb_list);
  spin_lock_init(>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(>lock)));
+    drm_syncobj_search_fence(syncobj, 0, 0, >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) {
   

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

2018-09-19 Thread 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(>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(>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(>refcount);
  INIT_LIST_HEAD(>cb_list);
  spin_lock_init(>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(>lock)));
+    drm_syncobj_search_fence(syncobj, 0, 0, >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,
+   [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) {
+   

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

2018-09-19 Thread 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(>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(>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(>refcount);
  INIT_LIST_HEAD(>cb_list);
  spin_lock_init(>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(>lock)));
+    drm_syncobj_search_fence(syncobj, 0, 0, >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,
+   [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) {
+   

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

2018-09-19 Thread Christian König 

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 
Cc: Christian Konig 
Cc: Dave Airlie 
Cc: Daniel Rakos 
Cc: Daniel Vetter 
---
  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 
  
+/* merge normal syncobj to timeline syncobj, the point interval is 1 */

+#define DRM_SYNCOBJ_INDIVIDUAL_POINT 1
+
  struct drm_syncobj_stub_fence {
struct dma_fence base;
spinlock_t lock;
@@ -82,6 +85,11 @@ static const struct dma_fence_ops drm_syncobj_stub_fence_ops 
= {
.release = drm_syncobj_stub_fence_release,
  };
  
+struct drm_syncobj_signal_pt {

+   struct dma_fence_array *base;
+   u64value;
+   struct list_head list;
+};
  
  /**

   * drm_syncobj_find - lookup and reference a sync object.
@@ -117,6 +125,9 @@ static void drm_syncobj_add_callback_locked(struct 
drm_syncobj *syncobj,
list_add_tail(>node, >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(>lock);

@@ -133,10 +144,12 @@ static int

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

2018-09-18 Thread 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 
Cc: Christian Konig 
Cc: Dave Airlie 
Cc: Daniel Rakos 
Cc: Daniel Vetter 
---
 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 
 
+/* merge normal syncobj to timeline syncobj, the point interval is 1 */
+#define DRM_SYNCOBJ_INDIVIDUAL_POINT 1
+
 struct drm_syncobj_stub_fence {
struct dma_fence base;
spinlock_t lock;
@@ -82,6 +85,11 @@ static const struct dma_fence_ops drm_syncobj_stub_fence_ops 
= {
.release = drm_syncobj_stub_fence_release,
 };
 
+struct drm_syncobj_signal_pt {
+   struct dma_fence_array *base;
+   u64value;
+   struct list_head list;
+};
 
 /**
  * drm_syncobj_find - lookup and reference a sync object.
@@ -117,6 +125,9 @@ static void drm_syncobj_add_callback_locked(struct 
drm_syncobj *syncobj,
list_add_tail(>node, >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(>lock);
@@ -133,10 +144,12 @@ static int drm_syncobj_fence_get_or_add_callback(struct 
drm_syncobj *syncobj,