Re: [PATCH 3/6] drm: add support of syncobj timeline point wait v2

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

 



Am 21.09.2018 um 09:15 schrieb Zhou, David(ChunMing):

-----Original Message-----
From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of
Christian K?nig
Sent: Thursday, September 20, 2018 7:11 PM
To: Zhou, David(ChunMing) <David1.Zhou@xxxxxxx>; dri-
devel@xxxxxxxxxxxxxxxxxxxxx
Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
Subject: Re: [PATCH 3/6] drm: add support of syncobj timeline point wait v2

Am 20.09.2018 um 13:03 schrieb Chunming Zhou:
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@xxxxxxx>
---
   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 67472bd77c83..a43de0e4616c
100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -126,13 +126,14 @@ static void
drm_syncobj_add_callback_locked(struct drm_syncobj *syncobj,
   }

   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;

@@ -143,7 +144,7 @@ static int
drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj,
   	 */
   	if (!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)
   			return 1;
   		spin_lock(&syncobj->lock);
@@ -358,7 +359,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);
That looks fishy to me. Why do we need to unlock
Cb func will call _search_fence, which will need to grab the lock, otherwise deadlock.


and who guarantees that
tmp is still valid when we grab the lock again?
Sorry for that, quickly  fix deadlock and forget to care that when debug.
How about splice to a tmp list, and then list_for _xxx without lock?

Yeah, that should work. Alternative is to use something like "while (!list_empty()) { e = list_first_entry(); list_del(e)....". But either way should work.

Any other comment on patch series and libdrm patches?   That one comment increases a patch set version seems be overcommit.

On a first glance that stuff looked good, but give me till monday for that. I'm currently in the middle of debugging issues.

Thanks,
Christian.


Thanks,
David Zhou

Apart from that can't see anything obvious wrong, but I certainly need to
take a closer look.

Christian.

   		}
   		spin_unlock(&syncobj->lock);
   	}
@@ -856,6 +859,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, @@
-873,12 +877,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,
@@ -886,13 +891,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
@@ -901,7 +920,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) { @@ -940,6
+960,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); @@ -1003,6 +1024,9 @@
static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj
**syncobjs,
   	}
   	kfree(entries);

+err_free_points:
+	kfree(points);
+
   	return ret;
   }

@@ -1041,20 +1065,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;
@@ -1142,13 +1179,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.
_______________________________________________
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