Skip to contentSkip to conversation

drm syncobjs - running out of tag lines

PATCH In review Dave Airlie 5 participants 12 messages Jun 1, 2017 – Jun 17, 2017 5 messages not in the archive

Subsystems (4)
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): 20170601010643.28616-1-airlied-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

#2 lore raw

PATCH 1/5 drm: introduce sync objects (v4)

From: Dave Airlie <[email protected]>

Sync objects are new toplevel drm object, that contain a pointer to a fence. This fence can be updated via command submission ioctls via drivers.

There is also a generic wait obj API modelled on the vulkan wait API (with code modelled on some amdgpu code).

These objects can be converted to an opaque fd that can be passes between processes.

v2: rename reference/unreference to put/get (Chris) fix leaked reference (David Zhou)
drop mutex in favour of cmpxchg (Chris)
v3: cleanups from danvet, rebase on drm_fops rename check fd_flags is 0 in ioctls.
v4: export find/free, change replace fence to take a syncobj. In order to support lookup first, replace later semantics which seem in the end to be cleaner.

---
 Documentation/gpu/drm-internals.rst |   3 +
 Documentation/gpu/drm-mm.rst        |  12 ++
 drivers/gpu/drm/Makefile            |   2 +-
 drivers/gpu/drm/drm_file.c          |   8 +
 drivers/gpu/drm/drm_internal.h      |  13 ++
 drivers/gpu/drm/drm_ioctl.c         |  12 ++
 drivers/gpu/drm/drm_syncobj.c       | 382 ++++++++++++++++++++++++++++++++++++
 include/drm/drmP.h                  |   1 -
 include/drm/drm_drv.h               |   1 +
 include/drm/drm_file.h              |   5 +
 include/drm/drm_syncobj.h           |  90 +++++++++
 include/uapi/drm/drm.h              |  24 +++
 12 files changed, 551 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_syncobj.c
 create mode 100644 include/drm/drm_syncobj.h
Documentation/gpu/drm-internals.rst +3 -0
index babfb61..2b23d78 100644
@@ -98,6 +98,9 @@ DRIVER_ATOMIC
     implement appropriate obj->atomic_get_property() vfuncs for any
     modeset objects with driver specific properties.
 
+DRIVER_SYNCOBJ
+    Driver support drm sync objects.
+
 Major, Minor and Patchlevel
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
Documentation/gpu/drm-mm.rst +12 -0
index 96b9c34..9412798 100644
@@ -484,3 +484,15 @@ DRM Cache Handling
 
 .. kernel-doc:: drivers/gpu/drm/drm_cache.c
    :export:
+
+DRM Sync Objects
+===========================
+
+.. kernel-doc:: drivers/gpu/drm/drm_syncobj.c
+   :doc: Overview
+
+.. kernel-doc:: include/drm/drm_syncobj.h
+   :export:
+
+.. kernel-doc:: drivers/gpu/drm/drm_syncobj.c
+   :export:
drivers/gpu/drm/Makefile +1 -1
index 59f0f9b..6f42188 100644
@@ -16,7 +16,7 @@ drm-y       :=	drm_auth.o drm_bufs.o drm_cache.o \
 		drm_framebuffer.o drm_connector.o drm_blend.o \
 		drm_encoder.o drm_mode_object.o drm_property.o \
 		drm_plane.o drm_color_mgmt.o drm_print.o \
-		drm_dumb_buffers.o drm_mode_config.o
+		drm_dumb_buffers.o drm_mode_config.o drm_syncobj.o
 
 drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
 drm-$(CONFIG_DRM_VM) += drm_vm.o
drivers/gpu/drm/drm_file.c +8 -0
index 3783b65..a20d6a9 100644
@@ -229,6 +229,9 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor)
 	if (drm_core_check_feature(dev, DRIVER_GEM))
 		drm_gem_open(dev, priv);
 
+	if (drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+		drm_syncobj_open(priv);
+
 	if (drm_core_check_feature(dev, DRIVER_PRIME))
 		drm_prime_init_file_private(&priv->prime);
 
@@ -276,6 +279,8 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor)
 out_prime_destroy:
 	if (drm_core_check_feature(dev, DRIVER_PRIME))
 		drm_prime_destroy_file_private(&priv->prime);
+	if (drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+		drm_syncobj_release(priv);
 	if (drm_core_check_feature(dev, DRIVER_GEM))
 		drm_gem_release(dev, priv);
 	put_pid(priv->pid);
@@ -398,6 +403,9 @@ int drm_release(struct inode *inode, struct file *filp)
 		drm_property_destroy_user_blobs(dev, file_priv);
 	}
 
+	if (drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+		drm_syncobj_release(file_priv);
+
 	if (drm_core_check_feature(dev, DRIVER_GEM))
 		drm_gem_release(dev, file_priv);
 
drivers/gpu/drm/drm_internal.h +13 -0
index 3d8e8f8..3fdef2c 100644
@@ -142,4 +142,17 @@ static inline int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
 {
 	return 0;
 }
+
 #endif
+
+/* drm_syncobj.c */
+void drm_syncobj_open(struct drm_file *file_private);
+void drm_syncobj_release(struct drm_file *file_private);
+int drm_syncobj_create_ioctl(struct drm_device *dev, void *data,
+			     struct drm_file *file_private);
+int drm_syncobj_destroy_ioctl(struct drm_device *dev, void *data,
+			      struct drm_file *file_private);
+int drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data,
+				   struct drm_file *file_private);
+int drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
+				   struct drm_file *file_private);
drivers/gpu/drm/drm_ioctl.c +12 -0
index 865e3ee..f1e5681 100644
@@ -241,6 +241,9 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
 		req->value |= dev->driver->prime_fd_to_handle ? DRM_PRIME_CAP_IMPORT : 0;
 		req->value |= dev->driver->prime_handle_to_fd ? DRM_PRIME_CAP_EXPORT : 0;
 		return 0;
+	case DRM_CAP_SYNCOBJ:
+		req->value = drm_core_check_feature(dev, DRIVER_SYNCOBJ);
+		return 0;
 	}
 
 	/* Other caps only work with KMS drivers */
@@ -645,6 +648,15 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_ATOMIC, drm_mode_atomic_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATEPROPBLOB, drm_mode_createblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROYPROPBLOB, drm_mode_destroyblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
+
+	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_CREATE, drm_syncobj_create_ioctl,
+		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_DESTROY, drm_syncobj_destroy_ioctl,
+		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD, drm_syncobj_handle_to_fd_ioctl,
+		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE, drm_syncobj_fd_to_handle_ioctl,
+		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
 };
 
 #define DRM_CORE_IOCTL_COUNT	ARRAY_SIZE( drm_ioctls )
drivers/gpu/drm/drm_syncobj.c new file +382 -0

382 lines not on this page · raw message

include/drm/drmP.h +0 -1
index e1daa4f..4fad9f2 100644
@@ -319,7 +319,6 @@ struct pci_controller;
 
 #define DRM_IF_VERSION(maj, min) (maj << 16 | min)
 
-
 /* Flags and return codes for get_vblank_timestamp() driver function. */
 #define DRM_CALLED_FROM_VBLIRQ 1
 #define DRM_VBLANKTIME_SCANOUTPOS_METHOD (1 << 0)
include/drm/drm_drv.h +1 -0
index 53b9832..1c66c1c 100644
@@ -53,6 +53,7 @@ struct drm_mode_create_dumb;
 #define DRIVER_RENDER			0x8000
 #define DRIVER_ATOMIC			0x10000
 #define DRIVER_KMS_LEGACY_CONTEXT	0x20000
+#define DRIVER_SYNCOBJ                  0x40000
 
 /**
  * struct drm_driver - DRM driver structure
include/drm/drm_file.h +5 -0
index 5dd27ae..cea7354 100644
@@ -231,6 +231,11 @@ struct drm_file {
 	/** @table_lock: Protects @object_idr. */
 	spinlock_t table_lock;
 
+	/** @syncobj_idr: Mapping of sync object handles to object pointers. */
+	struct idr syncobj_idr;
+	/** @syncobj_table_lock: Protects @syncobj_idr. */
+	spinlock_t syncobj_table_lock;
+
 	/** @filp: Pointer to the core file structure. */
 	struct file *filp;
 
include/drm/drm_syncobj.h new file +90 -0

90 lines not on this page · raw message

include/uapi/drm/drm.h +24 -0
index 42d9f64..96c5c78 100644
@@ -648,6 +648,7 @@ struct drm_gem_open {
 #define DRM_CAP_ADDFB2_MODIFIERS	0x10
 #define DRM_CAP_PAGE_FLIP_TARGET	0x11
 #define DRM_CAP_CRTC_IN_VBLANK_EVENT	0x12
+#define DRM_CAP_SYNCOBJ		0x13
 
 /** DRM_IOCTL_GET_CAP ioctl argument type */
 struct drm_get_cap {
@@ -697,6 +698,24 @@ struct drm_prime_handle {
 	__s32 fd;
 };
 
+struct drm_syncobj_create {
+	__u32 handle;
+	__u32 flags;
+};
+
+struct drm_syncobj_destroy {
+	__u32 handle;
+	__u32 pad;
+};
+
+struct drm_syncobj_handle {
+	__u32 handle;
+	__u32 flags;
+
+	__s32 fd;
+	__u32 pad;
+};
+
 #if defined(__cplusplus)
 }
 #endif
@@ -815,6 +834,11 @@ extern "C" {
 #define DRM_IOCTL_MODE_CREATEPROPBLOB	DRM_IOWR(0xBD, struct drm_mode_create_blob)
 #define DRM_IOCTL_MODE_DESTROYPROPBLOB	DRM_IOWR(0xBE, struct drm_mode_destroy_blob)
 
+#define DRM_IOCTL_SYNCOBJ_CREATE	DRM_IOWR(0xBF, struct drm_syncobj_create)
+#define DRM_IOCTL_SYNCOBJ_DESTROY	DRM_IOWR(0xC0, struct drm_syncobj_destroy)
+#define DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD	DRM_IOWR(0xC1, struct drm_syncobj_handle)
+#define DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE	DRM_IOWR(0xC2, struct drm_syncobj_handle)
+
 /**
  * Device specific ioctls should only be in their respective headers
  * The device specific ioctl range is from 0x40 to 0x9f.
Signature · 6 lines
2.9.4

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

A message that is not in the archive (sent off-list or not yet received): 20170601010643.28616-2-airlied-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

Reviewed-by
#4 lore raw

Re: [PATCH 1/5] drm: introduce sync objects (v4)

Quoted 21 lines · Dave Airlie

From: Dave Airlie <[email protected]>

Sync objects are new toplevel drm object, that contain a pointer to a fence. This fence can be updated via command submission ioctls via drivers.

There is also a generic wait obj API modelled on the vulkan wait API (with code modelled on some amdgpu code).

These objects can be converted to an opaque fd that can be passes between processes.

v2: rename reference/unreference to put/get (Chris) fix leaked reference (David Zhou)
drop mutex in favour of cmpxchg (Chris)
v3: cleanups from danvet, rebase on drm_fops rename check fd_flags is 0 in ioctls.
v4: export find/free, change replace fence to take a syncobj. In order to support lookup first, replace later semantics which seem in the end to be cleaner.

Quoted 2 lines

Thanks for find/replace, saves me having to export them later :)

-Chris

Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
#5 lore raw

PATCH 2/5 drm/syncobj: add sync obj wait interface. (v4)

In reply to a message not in the archive

From: Dave Airlie <[email protected]>

This interface will allow sync object to be used to back Vulkan fences. This API is pretty much the vulkan fence waiting API, and I've ported the code from amdgpu.

v2: accept relative timeout, pass remaining time back to userspace.
v3: return to absolute timeouts.
v4: absolute zero = poll,
rewrite any/all code to have same operation for arrays return -EINVAL for 0 fences.

---
 drivers/gpu/drm/drm_internal.h |   2 +
 drivers/gpu/drm/drm_ioctl.c    |   2 +
 drivers/gpu/drm/drm_syncobj.c  | 129 +++++++++++++++++++++++++++++++++++++++++
 include/uapi/drm/drm.h         |  14 +++++
 4 files changed, 147 insertions(+)
drivers/gpu/drm/drm_internal.h +2 -0
index 3fdef2c..53e3f6b 100644
@@ -156,3 +156,5 @@ int drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data,
 				   struct drm_file *file_private);
 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);
drivers/gpu/drm/drm_ioctl.c +2 -0
index f1e5681..385ce74 100644
@@ -657,6 +657,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE, drm_syncobj_fd_to_handle_ioctl,
 		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_WAIT, drm_syncobj_wait_ioctl,
+		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
 };
 
 #define DRM_CORE_IOCTL_COUNT	ARRAY_SIZE( drm_ioctls )
drivers/gpu/drm/drm_syncobj.c +129 -0

143 lines not on this page · raw message

include/uapi/drm/drm.h +14 -0
index 96c5c78..d6e2f62 100644
@@ -716,6 +716,19 @@ struct drm_syncobj_handle {
 	__u32 pad;
 };
 
+#define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL (1 << 0)
+struct drm_syncobj_wait {
+	__u64 handles;
+	/* absolute timeout */
+	__u64 timeout_ns;
+	/* remaining timeout */
+	__u64 out_timeout_ns;
+	__u32 count_handles;
+	__u32 flags;
+	__u32 out_status;
+	__u32 first_signaled; /* on valid when not waiting all */
+};
+
 #if defined(__cplusplus)
 }
 #endif
@@ -838,6 +851,7 @@ extern "C" {
 #define DRM_IOCTL_SYNCOBJ_DESTROY	DRM_IOWR(0xC0, struct drm_syncobj_destroy)
 #define DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD	DRM_IOWR(0xC1, struct drm_syncobj_handle)
 #define DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE	DRM_IOWR(0xC2, struct drm_syncobj_handle)
+#define DRM_IOCTL_SYNCOBJ_WAIT		DRM_IOWR(0xC3, struct drm_syncobj_wait)
 
 /**
  * Device specific ioctls should only be in their respective headers
Signature · 6 lines
2.9.4

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
#6 lore raw

Re: [PATCH 2/5] drm/syncobj: add sync obj wait interface. (v4)

Quoted 79 lines · Dave Airlie

From: Dave Airlie <[email protected]>

This interface will allow sync object to be used to back Vulkan fences. This API is pretty much the vulkan fence waiting API, and I've ported the code from amdgpu.

v2: accept relative timeout, pass remaining time back to userspace.
v3: return to absolute timeouts.
v4: absolute zero = poll,
rewrite any/all code to have same operation for arrays return -EINVAL for 0 fences.

---
 drivers/gpu/drm/drm_internal.h |   2 +
 drivers/gpu/drm/drm_ioctl.c    |   2 +
 drivers/gpu/drm/drm_syncobj.c  | 129 +++++++++++++++++++++++++++++++++++++++++
 include/uapi/drm/drm.h         |  14 +++++
 4 files changed, 147 insertions(+)
drivers/gpu/drm/drm_internal.h +2 -0
index 3fdef2c..53e3f6b 100644
@@ -156,3 +156,5 @@ int drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data,
 				   struct drm_file *file_private);
 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);
drivers/gpu/drm/drm_ioctl.c +2 -0
index f1e5681..385ce74 100644
@@ -657,6 +657,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE, drm_syncobj_fd_to_handle_ioctl,
 		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_WAIT, drm_syncobj_wait_ioctl,
+		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
 };
 
 #define DRM_CORE_IOCTL_COUNT	ARRAY_SIZE( drm_ioctls )
drivers/gpu/drm/drm_syncobj.c +14 -0
index 23bb197..ec462bd 100644
@@ -1,5 +1,7 @@
 /*
  * Copyright 2017 Red Hat
+ * Parts ported from amdgpu (fence wait code).
+ * Copyright 2016 Advanced Micro Devices, Inc.
  *
  * Permission is hereby granted, free of charge, to any person obtaining a
  * copy of this software and associated documentation files (the "Software"),
@@ -31,6 +33,9 @@
  * that contain an optional fence. The fence can be updated with a new
  * fence, or be NULL.
  *
+ * syncobj's can be waited upon, where it will wait for the underlying
+ * fence.
+ *
  * syncobj's can be export to fd's and back, these fd's are opaque and
  * have no other use case, except passing the syncobj between processes.
  *
@@ -380,3 +385,127 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
 	return drm_syncobj_fd_to_handle(file_private, args->fd,
 					&args->handle);
 }
+
+
+/**
+ * drm_timeout_abs_to_jiffies - calculate jiffies timeout from absolute value
+ *
+ * @timeout_ns: timeout in ns, 0 for poll
+ *
+ * Calculate the timeout in jiffies from an absolute timeout in ns.
+ */

drivers/gpu/drm/drm_syncobj.c drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data, · line 406

Quoted 10 lines
drivers/gpu/drm/drm_syncobj.c +10 -0
+static unsigned long drm_timeout_abs_to_jiffies(uint64_t timeout_ns)
+{
+	unsigned long timeout_jiffies;
+	ktime_t timeout;
+
+	/* make 0 timeout means poll - absolute 0 doesn't seem valid */
+	if (timeout_ns == 0)
+		return 0;
+
+	/* clamp timeout if it's to large */
	/* Negative timeout means to wait forever */

drivers/gpu/drm/drm_syncobj.c drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data, · line 419

Quoted 13 lines
drivers/gpu/drm/drm_syncobj.c +13 -0
+	if (((int64_t)timeout_ns) < 0)
+		return MAX_SCHEDULE_TIMEOUT;
+
+	timeout = ktime_sub(ns_to_ktime(timeout_ns), ktime_get());
+	if (ktime_to_ns(timeout) < 0)
+		return 0;
+
+	timeout_jiffies = nsecs_to_jiffies(ktime_to_ns(timeout));
+	/*  clamp timeout to avoid infinite timeout */
+	if (timeout_jiffies >= MAX_SCHEDULE_TIMEOUT)
+		return MAX_SCHEDULE_TIMEOUT - 1;
+
+	return timeout_jiffies + 1;

timeout == MAX_SCHEDULE_TIMEOUT - 1 just returned infinity.

/* clamp timeout to avoid infinite timeout */ return min(timeout_jiffies + 1, MAX_SCHEDULE_TIMEOUT - 1);

Hmm, except that nsecs_to_jiffies doesn't apply a clamp so we may overflow with timeout_jiffies + 1.

* Unlike {m,u}secs_to_jiffies, type of input is not unsigned int but u64.
* And this doesn't return MAX_JIFFY_OFFSET since this function is designed
* for scheduler, not for use in device drivers to calculate timeout
* value.

Hmm, it is also EXPORT_SYMBOL_GPL which may cause a conflict with us exporting syncobj as EXPORT_SYMBOL ? Funnily it is just a wrapper around nsecs_to_jiffies64 which is !GPL compatible! So much for being scheduler only.

Quoted 10 lines
drivers/gpu/drm/drm_syncobj.c +10 -0
+}
+
+static int drm_syncobj_wait_fences(struct drm_device *dev,
+				   struct drm_file *file_private,
+				   struct drm_syncobj_wait *wait,
+				   struct dma_fence **fences)
+{
+	unsigned long timeout = drm_timeout_abs_to_jiffies(wait->timeout_ns);
+	int ret = 0;
+	uint32_t first = ~0;

drivers/gpu/drm/drm_syncobj.c drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data, · line 439

Quoted 10 lines
drivers/gpu/drm/drm_syncobj.c +10 -0
+
+	if (wait->flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL) {
+		int i;
+		for (i = 0; i < wait->count_handles; i++) {
+			ret = dma_fence_wait_timeout(fences[i], true, timeout);
+
+			if (ret < 0)
+				return ret;
+			if (ret == 0)
+				break;

With current dma_fence_default_wait and polling with timeout=0:
1st fence busy, returns 1
2+ fence idle, returns 1
wait->out_timeous_ns is set to 10ms

Or
1st fence busy, returns 1
2nd fence busy, returns 0
Takes 10ms for a poll.

Nevertheless the syncobj code is correct (just maybe appease checkpatch about the unbalanced if {} else).

Quoted 6 lines
drivers/gpu/drm/drm_syncobj.c +6 -0
+			timeout = ret;
+		}
+		first = 0;
+	} else
+		ret = dma_fence_wait_any_timeout(fences,
+						 wait->count_handles,

drivers/gpu/drm/drm_syncobj.c drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data, · line 454

Quoted 10 lines
drivers/gpu/drm/drm_syncobj.c +9 -0
+						 true, timeout,
+						 &first);
+	if (ret < 0)
+		return ret;
+
+	wait->out_timeout_ns = jiffies_to_nsecs(ret);
+	wait->out_status = (ret > 0);
+	wait->first_signaled = first;
 
+	/* set return value 0 to indicate success */

Feels a little superfluous.

Quoted 20 lines
drivers/gpu/drm/drm_syncobj.c +20 -0
+	return 0;
+}
+
+int
+drm_syncobj_wait_ioctl(struct drm_device *dev, void *data,
+		       struct drm_file *file_private)
+{
+	struct drm_syncobj_wait *args = data;
+	uint32_t *handles;
+	struct dma_fence **fences;
+	int ret = 0;
+	int i;
+
+	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+		return -ENODEV;
+
+	if (args->flags != 0 && args->flags != DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL)
+		return -EINVAL;
+
+	if (args->count_handles == 0)

drivers/gpu/drm/drm_syncobj.c drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data, · line 484

Quoted 10 lines
drivers/gpu/drm/drm_syncobj.c +10 -0
+		return -EINVAL;
+
+	/* Get the handles from userspace */
+	handles = kmalloc_array(args->count_handles, sizeof(uint32_t),
+				GFP_KERNEL);
+	if (handles == NULL)
+		return -ENOMEM;
+
+	if (copy_from_user(handles,
+			   (void __user *)(unsigned long)(args->handles),

u64_to_user_ptr(args->handles)

drivers/gpu/drm/drm_syncobj.c drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data, · line 491

Quoted 7 lines
drivers/gpu/drm/drm_syncobj.c +7 -0
+			   sizeof(uint32_t) * args->count_handles)) {
+		ret = -EFAULT;
+		goto err_free_handles;
+	}
+
+	fences = kcalloc(args->count_handles,
+			 sizeof(struct dma_fence *), GFP_KERNEL);

if (!fences) /* blah */

Quoted 20 lines
drivers/gpu/drm/drm_syncobj.c +20 -0
+
+	for (i = 0; i < args->count_handles; i++) {
+		ret = drm_syncobj_fence_get(file_private, handles[i],
+					    &fences[i]);
+		if (ret)
+			goto err_free_fence_array;
+	}
+
+	ret = drm_syncobj_wait_fences(dev, file_private,
+				      args, fences);
+
+err_free_fence_array:
+	for (i = 0; i < args->count_handles; i++)
+		dma_fence_put(fences[i]);
+	kfree(fences);
+err_free_handles:
+	kfree(handles);
+
+	return ret;
+}
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#7 lore raw

PATCH 4/5 amdgpu/cs: split out fence dependency checking

In reply to a message not in the archive

From: Dave Airlie <[email protected]>

This just splits out the fence depenency checking into it's own function to make it easier to add semaphore dependencies.

---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 85 +++++++++++++++++++---------------
 1 file changed, 47 insertions(+), 38 deletions(-)
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +47 -38
index 4e6b950..30225d7 100644
@@ -995,56 +995,65 @@ static int amdgpu_cs_ib_fill(struct amdgpu_device *adev,
 	return 0;
 }
 
-static int amdgpu_cs_dependencies(struct amdgpu_device *adev,
-				  struct amdgpu_cs_parser *p)
+static int amdgpu_cs_process_fence_dep(struct amdgpu_cs_parser *p,
+				       struct amdgpu_cs_chunk *chunk)
 {
 	struct amdgpu_fpriv *fpriv = p->filp->driver_priv;
-	int i, j, r;
-
-	for (i = 0; i < p->nchunks; ++i) {
-		struct drm_amdgpu_cs_chunk_dep *deps;
-		struct amdgpu_cs_chunk *chunk;
-		unsigned num_deps;
+	unsigned num_deps;
+	int i, r;
+	struct drm_amdgpu_cs_chunk_dep *deps;
 
-		chunk = &p->chunks[i];
+	deps = (struct drm_amdgpu_cs_chunk_dep *)chunk->kdata;
+	num_deps = chunk->length_dw * 4 /
+		sizeof(struct drm_amdgpu_cs_chunk_dep);
 
-		if (chunk->chunk_id != AMDGPU_CHUNK_ID_DEPENDENCIES)
-			continue;
+	for (i = 0; i < num_deps; ++i) {
+		struct amdgpu_ring *ring;
+		struct amdgpu_ctx *ctx;
+		struct dma_fence *fence;
 
-		deps = (struct drm_amdgpu_cs_chunk_dep *)chunk->kdata;
-		num_deps = chunk->length_dw * 4 /
-			sizeof(struct drm_amdgpu_cs_chunk_dep);
+		r = amdgpu_cs_get_ring(p->adev, deps[i].ip_type,
+				       deps[i].ip_instance,
+				       deps[i].ring, &ring);
+		if (r)
+			return r;
 
-		for (j = 0; j < num_deps; ++j) {
-			struct amdgpu_ring *ring;
-			struct amdgpu_ctx *ctx;
-			struct dma_fence *fence;
+		ctx = amdgpu_ctx_get(fpriv, deps[i].ctx_id);
+		if (ctx == NULL)
+			return -EINVAL;
 
-			r = amdgpu_cs_get_ring(adev, deps[j].ip_type,
-					       deps[j].ip_instance,
-					       deps[j].ring, &ring);
+		fence = amdgpu_ctx_get_fence(ctx, ring,
+					     deps[i].handle);
+		if (IS_ERR(fence)) {
+			r = PTR_ERR(fence);
+			amdgpu_ctx_put(ctx);
+			return r;
+		} else if (fence) {
+			r = amdgpu_sync_fence(p->adev, &p->job->sync,
+					      fence);
+			dma_fence_put(fence);
+			amdgpu_ctx_put(ctx);
 			if (r)
 				return r;
+		}
+	}
+	return 0;
+}
 
-			ctx = amdgpu_ctx_get(fpriv, deps[j].ctx_id);
-			if (ctx == NULL)
-				return -EINVAL;
+static int amdgpu_cs_dependencies(struct amdgpu_device *adev,
+				  struct amdgpu_cs_parser *p)
+{
+	int i, r;
 
-			fence = amdgpu_ctx_get_fence(ctx, ring,
-						     deps[j].handle);
-			if (IS_ERR(fence)) {
-				r = PTR_ERR(fence);
-				amdgpu_ctx_put(ctx);
-				return r;
+	for (i = 0; i < p->nchunks; ++i) {
+		struct amdgpu_cs_chunk *chunk;
 
-			} else if (fence) {
-				r = amdgpu_sync_fence(adev, &p->job->sync,
-						      fence);
-				dma_fence_put(fence);
-				amdgpu_ctx_put(ctx);
-				if (r)
-					return r;
-			}
+		chunk = &p->chunks[i];
+
+		if (chunk->chunk_id == AMDGPU_CHUNK_ID_DEPENDENCIES) {
+			r = amdgpu_cs_process_fence_dep(p, chunk);
+			if (r)
+				return r;
 		}
 	}
 
Signature · 6 lines
2.9.4

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
#8 lore raw

PATCH 5/5 amdgpu: use drm sync objects for shared semaphores (v5)

In reply to a message not in the archive

From: Dave Airlie <[email protected]>

This creates a new command submission chunk for amdgpu to add in and out sync objects around the submission.

Sync objects are managed via the drm syncobj ioctls.

The command submission interface is enhanced with two new chunks, one for syncobj pre submission dependencies, and one for post submission sync obj signalling, and just takes a list of handles for each.

This is based on work originally done by David Zhou at AMD, with input from Christian Konig on what things should look like.

In theory VkFences could be backed with sync objects and just get passed into the cs as syncobj handles as well.

NOTE: this interface addition needs a version bump to expose it to userspace.

TODO: update to dep_sync when rebasing onto amdgpu master. (with this - r-b from Christian)

v1.1: keep file reference on import.
v2: move to using syncobjs
v2.1: change some APIs to just use p pointer.
v3: make more robust against CS failures, we now add the wait sems but only remove them once the CS job has been submitted.
v4: rewrite names of API and base on new syncobj code.
v5: move post deps earlier, rename some apis
v6: lookup post deps earlier, and just replace fences in post deps stage (Christian)

---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  3 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  | 88 ++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  2 +-
 include/uapi/drm/amdgpu_drm.h           |  6 +++
 4 files changed, 97 insertions(+), 2 deletions(-)
drivers/gpu/drm/amd/amdgpu/amdgpu.h +3 -0
index 833c3c1..f520ba0 100644
@@ -1109,6 +1109,9 @@ struct amdgpu_cs_parser {
 
 	/* user fence */
 	struct amdgpu_bo_list_entry	uf_entry;
+
+	unsigned num_post_dep_syncobjs;
+	struct drm_syncobj **post_dep_syncobjs;
 };
 
 #define AMDGPU_PREAMBLE_IB_PRESENT          (1 << 0) /* bit set means command submit involves a preamble IB */
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +87 -1

136 lines not on this page · raw message

drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +1 -1
index f2d705e..5329e7f 100644
@@ -719,7 +719,7 @@ static struct drm_driver kms_driver = {
 	.driver_features =
 	    DRIVER_USE_AGP |
 	    DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | DRIVER_GEM |
-	    DRIVER_PRIME | DRIVER_RENDER | DRIVER_MODESET,
+	    DRIVER_PRIME | DRIVER_RENDER | DRIVER_MODESET | DRIVER_SYNCOBJ,
 	.load = amdgpu_driver_load_kms,
 	.open = amdgpu_driver_open_kms,
 	.postclose = amdgpu_driver_postclose_kms,
include/uapi/drm/amdgpu_drm.h +6 -0
index 6c249e5..cc702ca 100644
@@ -416,6 +416,8 @@ struct drm_amdgpu_gem_va {
 #define AMDGPU_CHUNK_ID_IB		0x01
 #define AMDGPU_CHUNK_ID_FENCE		0x02
 #define AMDGPU_CHUNK_ID_DEPENDENCIES	0x03
+#define AMDGPU_CHUNK_ID_SYNCOBJ_IN      0x04
+#define AMDGPU_CHUNK_ID_SYNCOBJ_OUT     0x05
 
 struct drm_amdgpu_cs_chunk {
 	__u32		chunk_id;
@@ -483,6 +485,10 @@ struct drm_amdgpu_cs_chunk_fence {
 	__u32 offset;
 };
 
+struct drm_amdgpu_cs_chunk_sem {
+	__u32 handle;
+};
+
 struct drm_amdgpu_cs_chunk_data {
 	union {
 		struct drm_amdgpu_cs_chunk_ib		ib_data;
Signature · 6 lines
2.9.4

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
maintainer
#9 lore raw

Re: [PATCH 5/5] amdgpu: use drm sync objects for shared semaphores (v5)

Quoted 15 lines · Dave Airlie

From: Dave Airlie <[email protected]>

This creates a new command submission chunk for amdgpu to add in and out sync objects around the submission.

Sync objects are managed via the drm syncobj ioctls.

The command submission interface is enhanced with two new chunks, one for syncobj pre submission dependencies, and one for post submission sync obj signalling, and just takes a list of handles for each.

This is based on work originally done by David Zhou at AMD, with input from Christian Konig on what things should look like.

Quoted 8 lines

In theory VkFences could be backed with sync objects and just get passed into the cs as syncobj handles as well.

NOTE: this interface addition needs a version bump to expose it to userspace.

TODO: update to dep_sync when rebasing onto amdgpu master. (with this - r-b from Christian)

Hey Christian,

did you have a chance to re-review this, I think this has the last changes you asked for done properly.

If so I'd like to get Alex to get drm-next + pull these in on top at some point, or if I already have the dep_sync changes in my tree I can just fix it up and apply them there.

Dave.
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

A message that is not in the archive (sent off-list or not yet received): CAPM=9twtavgA+D+vgZYz7UagnjxX6pGFBBUycCs2ofGjhOcBrw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

#11 lore raw

Re: [PATCH 5/5] amdgpu: use drm sync objects for shared semaphores (v5)

Am 15.06.2017 um 05:59 schrieb Dave Airlie:

Quoted 22 lines · Dave Airlie
Quoted 21 lines

From: Dave Airlie <[email protected]>

This creates a new command submission chunk for amdgpu to add in and out sync objects around the submission.

Sync objects are managed via the drm syncobj ioctls.

The command submission interface is enhanced with two new chunks, one for syncobj pre submission dependencies, and one for post submission sync obj signalling, and just takes a list of handles for each.

This is based on work originally done by David Zhou at AMD, with input from Christian Konig on what things should look like.

In theory VkFences could be backed with sync objects and just get passed into the cs as syncobj handles as well.

NOTE: this interface addition needs a version bump to expose it to userspace.

Quoted 6 lines
Quoted 2 lines

TODO: update to dep_sync when rebasing onto amdgpu master. (with this - r-b from Christian)

Hey Christian,

did you have a chance to re-review this, I think this has the last changes you asked for done properly.

Sorry for the delay, holidays and dentist visits seem to eat up all my time at the moment.

Patches #1-#3 in this series are Acked-by: Christian König <[email protected]>.

Patch #4 already has my rb.

Patch #5 in this Series is Reviewed-by: Christian König <[email protected]>.

Quoted 3 lines

If so I'd like to get Alex to get drm-next + pull these in on top at some point, or if I already have the dep_sync changes in my tree I can just fix it up and apply them there.

Any approach works for me as long as Alex is fine with it.

Additional to your set I synced up two more ideas with out internal Vulkan team which seem to make a lot of sense to upstream as well:

1. An IOCTL to reset a sync object to it's initial state. E.g. reset the fence the sync objects wraps back to NULL.

2. The ability to merge multiple sync objects into one. Essentially the same thing we have for the sync file, but handle based instead of fd.

I'm going to work on that in the next week or so. Shouldn't be to much of a problem to come up with some sane code, but probably finding use cases for this in the open source userspace might be challenging.

Regards,
Christian.

Quoted 2 lines

Dave.

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

A message that is not in the archive (sent off-list or not yet received): e8021851-d630-3cbd-40e2-58f83a366a29-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org

#13 lore raw

Re: [PATCH 5/5] amdgpu: use drm sync objects for shared semaphores (v5)

Quoted 11 lines

team which seem to make a lot of sense to upstream as well:

1. An IOCTL to reset a sync object to it's initial state. E.g. reset the fence the sync objects wraps back to NULL.

2. The ability to merge multiple sync objects into one. Essentially the same thing we have for the sync file, but handle based instead of fd.

I'm going to work on that in the next week or so. Shouldn't be to much of a problem to come up with some sane code, but probably finding use cases for this in the open source userspace might be challenging.

Do we have use cases for these in the closed source stack? I'm sure if they are useful enough, radv could use them for similiar things.

Dave.
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

maintainer
#14 lore raw

PATCH 3/5 drm/syncobj: add sync_file interaction. (v1.2)

In reply to Dave Airlie

From: Dave Airlie <[email protected]>

This interface allows importing the fence from a sync_file into an existing drm sync object, or exporting the fence attached to an existing drm sync object into a new sync file object.

This should only be used to interact with sync files where necessary.

v1.1: fence put fixes (Chris), drop fence from ioctl names (Chris) fixup for new fence replace API.

---
 drivers/gpu/drm/drm_syncobj.c | 75 +++++++++++++++++++++++++++++++++++++++++--
 include/uapi/drm/drm.h        |  2 ++
 2 files changed, 75 insertions(+), 2 deletions(-)
drivers/gpu/drm/drm_syncobj.c +73 -2
index ec462bd..5814495 100644
@@ -50,6 +50,7 @@
 #include <linux/file.h>
 #include <linux/fs.h>
 #include <linux/anon_inodes.h>
+#include <linux/sync_file.h>
 
 #include "drm_internal.h"
 #include <drm/drm_syncobj.h>
@@ -281,6 +282,59 @@ static int drm_syncobj_fd_to_handle(struct drm_file *file_private,
 	return 0;
 }
 
+int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
+				       int fd, int handle)
+{
+	struct dma_fence *fence = sync_file_get_fence(fd);
+	struct drm_syncobj *syncobj;
+
+	if (!fence)
+		return -EINVAL;
+
+	syncobj = drm_syncobj_find(file_private, handle);
+	if (!syncobj) {
+		dma_fence_put(fence);
+		return -ENOENT;
+	}
+
+	drm_syncobj_replace_fence(file_private, syncobj, fence);
+	dma_fence_put(fence);
+	drm_syncobj_put(syncobj);
+	return 0;
+}
+
+int drm_syncobj_export_sync_file(struct drm_file *file_private,
+				 int handle, int *p_fd)
+{
+	int ret;
+	struct dma_fence *fence;
+	struct sync_file *sync_file;
+	int fd = get_unused_fd_flags(O_CLOEXEC);
+
+	if (fd < 0)
+		return fd;
+
+	ret = drm_syncobj_fence_get(file_private, handle, &fence);
+	if (ret)
+		goto err_put_fd;
+
+	sync_file = sync_file_create(fence);
+
+	dma_fence_put(fence);
+
+	if (!sync_file) {
+		ret = -EINVAL;
+		goto err_put_fd;
+	}
+
+	fd_install(fd, sync_file->file);
+
+	*p_fd = fd;
+	return 0;
+err_put_fd:
+	put_unused_fd(fd);
+	return ret;
+}
 /**
  * drm_syncobj_open - initalizes syncobj file-private structures at devnode open time
  * @dev: drm_device which is being opened by userspace
@@ -363,9 +417,17 @@ drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data,
 	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
 		return -ENODEV;
 
-	if (args->pad || args->flags)
+	if (args->pad)
 		return -EINVAL;
 
+	if (args->flags != 0 &&
+	    args->flags != DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE)
+		return -EINVAL;
+
+	if (args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE)
+		return drm_syncobj_export_sync_file(file_private, args->handle,
+						    &args->fd);
+
 	return drm_syncobj_handle_to_fd(file_private, args->handle,
 					&args->fd);
 }
@@ -379,9 +441,18 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
 	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
 		return -ENODEV;
 
-	if (args->pad || args->flags)
+	if (args->pad)
+		return -EINVAL;
+
+	if (args->flags != 0 &&
+	    args->flags != DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE)
 		return -EINVAL;
 
+	if (args->flags & DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE)
+		return drm_syncobj_import_sync_file_fence(file_private,
+							  args->fd,
+							  args->handle);
+
 	return drm_syncobj_fd_to_handle(file_private, args->fd,
 					&args->handle);
 }
include/uapi/drm/drm.h +2 -0
index d6e2f62..49c4e69 100644
@@ -708,6 +708,8 @@ struct drm_syncobj_destroy {
 	__u32 pad;
 };
 
+#define DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE (1 << 0)
+#define DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE (1 << 0)
 struct drm_syncobj_handle {
 	__u32 handle;
 	__u32 flags;
Signature · 6 lines
2.9.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

A message that is not in the archive (sent off-list or not yet received): 20170601010643.28616-4-airlied-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

Reviewed-by
#16 lore raw

Re: [PATCH 3/5] drm/syncobj: add sync_file interaction. (v1.2)

Quoted 6 lines · Dave Airlie

From: Dave Airlie <[email protected]>

This interface allows importing the fence from a sync_file into an existing drm sync object, or exporting the fence attached to an existing drm sync object into a new sync file object.

Quoted 7 lines

This should only be used to interact with sync files where necessary.

v1.1: fence put fixes (Chris), drop fence from ioctl names (Chris) fixup for new fence replace API.

-Chris

Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx