[RFC] Sync points/fences for i915

[RFC] Sync points/fences for i915

[Jesse Barnes]

This hasn't seen any testing yet, but I'm still interested in any bugs
people see in review, I'll fix them up.

If there are no major objections, I'll add some tests and a man page to
libdrm for this and we can move forward into the brave new world of
fences, giving userspace a lot more rope to hang itself with, and/or tie
cool knots.

[PATCH] drm/i915: Android sync points for i915

[Jesse Barnes]

Expose an ioctl to create Android fences based on the Android sync point
infrastructure (which in turn is based on DMA-buf fences).  Just a
sketch at this point, no testing has been done.

There are a couple of goals here:
  1) allow applications and libraries to create fences without an
     associated buffer
  2) re-use a common API so userspace doesn't have to impedance mismatch
     between different driver implementations too much
  3) allow applications and libraries to use explicit synchronization if
     they choose by exposing fences directly

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/Kconfig     |    2 +
 drivers/gpu/drm/i915/Makefile    |    1 +
 drivers/gpu/drm/i915/i915_dma.c  |    1 +
 drivers/gpu/drm/i915/i915_drv.h  |   11 ++
 drivers/gpu/drm/i915/i915_gem.c  |    5 +
 drivers/gpu/drm/i915/i915_irq.c  |    7 +-
 drivers/gpu/drm/i915/i915_sync.c |  301 ++++++++++++++++++++++++++++++++++++++
 include/uapi/drm/i915_drm.h      |   23 +++
 8 files changed, 350 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/i915/i915_sync.c

diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index 4e39ab3..cd0f2ec 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -6,6 +6,8 @@ config DRM_I915
 	select INTEL_GTT
 	select AGP_INTEL if AGP
 	select INTERVAL_TREE
+	select ANDROID
+	select SYNC
 	# we need shmfs for the swappable backing store, and in particular
 	# the shmem_readpage() which depends upon tmpfs
 	select SHMEM
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 91bd167..61a3eb5c 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -25,6 +25,7 @@ i915-y += i915_cmd_parser.o \
 	  i915_gem_execbuffer.o \
 	  i915_gem_gtt.o \
 	  i915_gem.o \
+	  i915_sync.o \
 	  i915_gem_stolen.o \
 	  i915_gem_tiling.o \
 	  i915_gem_userptr.o \
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 2e7f03a..84086e1 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -2043,6 +2043,7 @@ const struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_REG_READ, i915_reg_read_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_GET_RESET_STATS, i915_get_reset_stats_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_GEM_USERPTR, i915_gem_userptr_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(I915_GEM_FENCE, i915_sync_create_fence_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
 };
 
 int i915_max_ioctl = ARRAY_SIZE(i915_ioctls);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d604f4f..40ce444 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1388,6 +1388,8 @@ struct i915_frontbuffer_tracking {
 	unsigned flip_bits;
 };
 
+struct i915_sync_timeline;
+
 struct drm_i915_private {
 	struct drm_device *dev;
 	struct kmem_cache *slab;
@@ -1422,6 +1424,8 @@ struct drm_i915_private {
 	struct drm_i915_gem_object *semaphore_obj;
 	uint32_t last_seqno, next_seqno;
 
+	struct i915_sync_timeline *sync_tl[I915_NUM_RINGS];
+
 	drm_dma_handle_t *status_page_dmah;
 	struct resource mch_res;
 
@@ -2275,6 +2279,13 @@ void i915_init_vm(struct drm_i915_private *dev_priv,
 void i915_gem_free_object(struct drm_gem_object *obj);
 void i915_gem_vma_destroy(struct i915_vma *vma);
 
+/* i915_sync.c */
+int i915_sync_init(struct drm_i915_private *dev_priv);
+void i915_sync_fini(struct drm_i915_private *dev_priv);
+void i915_sync_signal(struct intel_engine_cs *ring);
+int i915_sync_create_fence_ioctl(struct drm_device *dev, void *data,
+				 struct drm_file *file);
+
 #define PIN_MAPPABLE 0x1
 #define PIN_NONBLOCK 0x2
 #define PIN_GLOBAL 0x4
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index dcd8d7b..cc82407 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4775,6 +4775,9 @@ int i915_gem_init(struct drm_device *dev)
 		atomic_set_mask(I915_WEDGED, &dev_priv->gpu_error.reset_counter);
 		ret = 0;
 	}
+
+	i915_sync_init(dev_priv);
+
 	mutex_unlock(&dev->struct_mutex);
 
 	/* Allow hardware batchbuffers unless told otherwise, but not for KMS. */
@@ -4970,6 +4973,8 @@ void i915_gem_release(struct drm_device *dev, struct drm_file *file)
 		request->file_priv = NULL;
 	}
 	spin_unlock(&file_priv->mm.lock);
+
+	i915_sync_fini(dev->dev_private);
 }
 
 static void
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 98abc22..48436dc 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -33,6 +33,7 @@
 #include <linux/circ_buf.h>
 #include <drm/drmP.h>
 #include <drm/i915_drm.h>
+#include "../../../staging/android/sync.h"
 #include "i915_drv.h"
 #include "i915_trace.h"
 #include "intel_drv.h"
@@ -1269,6 +1270,7 @@ static void notify_ring(struct drm_device *dev,
 		intel_notify_mmio_flip(ring);
 
 	wake_up_all(&ring->irq_queue);
+	i915_sync_signal(ring);
 	i915_queue_hangcheck(dev);
 }
 
@@ -2617,8 +2619,10 @@ static void i915_error_wake_up(struct drm_i915_private *dev_priv,
 	 */
 
 	/* Wake up __wait_seqno, potentially holding dev->struct_mutex. */
-	for_each_ring(ring, dev_priv, i)
+	for_each_ring(ring, dev_priv, i) {
 		wake_up_all(&ring->irq_queue);
+		i915_sync_signal(ring);
+	}
 
 	/* Wake up intel_crtc_wait_for_pending_flips, holding crtc->mutex. */
 	wake_up_all(&dev_priv->pending_flip_queue);
@@ -3269,6 +3273,7 @@ static void i915_hangcheck_elapsed(unsigned long data)
 							DRM_INFO("Fake missed irq on %s\n",
 								 ring->name);
 						wake_up_all(&ring->irq_queue);
+						i915_sync_signal(ring);
 					}
 					/* Safeguard against driver failure */
 					ring->hangcheck.score += BUSY;
diff --git a/drivers/gpu/drm/i915/i915_sync.c b/drivers/gpu/drm/i915/i915_sync.c
new file mode 100644
index 0000000..b40a4ec
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_sync.c
@@ -0,0 +1,301 @@
+/*
+ * Copyright © 2014 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Jesse Barnes <jbarnes@virtuousgeek.org>
+ *
+ */
+
+#include <drm/drmP.h>
+#include <drm/drm_vma_manager.h>
+#include <drm/i915_drm.h>
+#include "i915_drv.h"
+#include "i915_trace.h"
+#include "intel_drv.h"
+#include <linux/oom.h>
+#include <linux/shmem_fs.h>
+#include <linux/slab.h>
+#include <linux/swap.h>
+#include <linux/pci.h>
+#include <linux/dma-buf.h>
+#include "../../../staging/android/sync.h"
+
+/*
+ * i915 fences on sync timelines
+ *
+ * We implement sync points in terms of i915 seqnos.  They're exposed
+ * through the new DRM_I915_GEM_FENCE ioctl, and can be mixed and matched
+ * with other Android timelines and aggregated into sync_fences, etc.
+ *
+ * TODO:
+ *   rebase on top of Chris's seqno/request stuff and use requests
+ *   allow non-RCS fences (need ring/context association)
+ */
+
+struct i915_sync_timeline {
+	struct sync_timeline obj;
+	struct intel_engine_cs *ring;
+	struct drm_i915_private *dev_priv;
+};
+
+struct i915_sync_pt {
+	struct sync_pt pt;
+	u32 seqno;
+};
+
+static struct sync_pt *i915_sync_pt_create(struct i915_sync_timeline *tl)
+{
+	struct intel_engine_cs *ring = tl->ring;
+	struct i915_sync_pt *pt;
+	int ret;
+
+	pt = (struct i915_sync_pt *)sync_pt_create(&tl->obj,
+						   sizeof(struct i915_sync_pt));
+	if (!pt)
+		return NULL;
+
+	ret = ring->add_request(ring);
+	if (ret) {
+		DRM_ERROR("add_request failed\n");
+		sync_pt_free((struct sync_pt *)pt);
+		return NULL;
+	}
+
+	pt->seqno = ring->outstanding_lazy_seqno;
+
+	return (struct sync_pt *)pt;
+}
+
+static struct sync_pt *i915_sync_dup(struct sync_pt *sync_pt)
+{
+	struct i915_sync_pt *dst, *src = (struct i915_sync_pt *)sync_pt;
+	struct i915_sync_timeline *obj =
+		(struct i915_sync_timeline *)sync_pt_parent(sync_pt);
+
+	dst = (struct i915_sync_pt *)i915_sync_pt_create(obj);
+	if (!dst)
+		return NULL;
+
+	dst->seqno = src->seqno;
+
+	return (struct sync_pt *)dst;
+}
+
+static int i915_sync_signaled(struct sync_pt *sync_pt)
+{
+	struct i915_sync_pt *pt = (struct i915_sync_pt *)sync_pt;
+	struct i915_sync_timeline *obj =
+		(struct i915_sync_timeline *)sync_pt_parent(sync_pt);
+	struct intel_engine_cs *ring = obj->ring;
+
+	return i915_seqno_passed(ring->get_seqno(ring, false), pt->seqno);
+}
+
+static int i915_sync_compare(struct sync_pt *pta, struct sync_pt *ptb)
+{
+	struct i915_sync_pt *pt1 = (struct i915_sync_pt *)pta;
+	struct i915_sync_pt *pt2 = (struct i915_sync_pt *)ptb;
+	u32 a = pt1->seqno, b = pt2->seqno;
+
+	if (a == b)
+		return 0;
+
+	return ((s32)a - (s32)b) < 0 ? -1 : 1;
+}
+
+static int i915_sync_fill_driver_data(struct sync_pt *sync_pt, void *data,
+				      int size)
+{
+	struct i915_sync_pt *pt = (struct i915_sync_pt *)sync_pt;
+
+	if (size < sizeof(pt->seqno))
+		return -ENOMEM;
+
+	memcpy(data, &pt->seqno, sizeof(pt->seqno));
+
+	return sizeof(pt->seqno);
+}
+
+static void i915_sync_timeline_value_str(struct sync_timeline *timeline,
+					 char *str, int size)
+{
+	struct i915_sync_timeline *obj = (struct i915_sync_timeline *)timeline;
+	struct intel_engine_cs *ring = obj->ring;
+
+	snprintf(str, size, "%u", ring->get_seqno(ring, false));
+}
+
+static void i915_pt_value_str(struct sync_pt *sync_pt, char *str, int size)
+{
+	struct i915_sync_pt *pt = (struct i915_sync_pt *)sync_pt;
+
+	snprintf(str, size, "%u", pt->seqno);
+}
+
+static struct sync_timeline_ops i915_sync_ops = {
+	.driver_name = 		"i915_sync",
+	.dup =			i915_sync_dup,
+	.has_signaled =		i915_sync_signaled,
+	.compare =		i915_sync_compare,
+	.fill_driver_data =	i915_sync_fill_driver_data,
+	.timeline_value_str =	i915_sync_timeline_value_str,
+	.pt_value_str =		i915_pt_value_str,
+};
+
+/**
+ * i915_sync_create_fence_ioctl - fence creation function
+ * @dev: drm device
+ * @data: ioctl data
+ * @file: file struct
+ *
+ * This function creates a fence given a context and ring, and returns
+ * it to the caller in the form of a file descriptor.
+ *
+ * The returned descriptor is a sync fence fd, and can be used with all
+ * the usual sync fence operations (poll, ioctl, etc).
+ *
+ * The process fd limit should prevent an overallocation of fence objects,
+ * which need to be destroyed manually with a close() call.
+ */
+int i915_sync_create_fence_ioctl(struct drm_device *dev, void *data,
+				 struct drm_file *file)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_gem_fence *fdata = data;
+	struct sync_pt *pt;
+	struct sync_fence *fence;
+	struct i915_sync_timeline *obj;
+	struct intel_engine_cs *ring;
+	struct intel_context *ctx;
+	u32 ctx_id = fdata->ctx_id;
+	int fd = get_unused_fd_flags(O_CLOEXEC);
+	int ret = 0;
+
+	if (file == NULL) {
+		DRM_ERROR("no file priv?\n");
+		return -EINVAL;
+	}
+
+	ret = i915_mutex_lock_interruptible(dev);
+	if (ret) {
+		DRM_ERROR("mutex interrupted\n");
+		goto out;
+	}
+
+	ctx = i915_gem_context_get(file->driver_priv, ctx_id);
+	if (ctx == NULL) {
+		DRM_ERROR("context lookup failed\n");
+		ret = -ENOENT;
+		goto err;
+	}
+
+	ring = &dev_priv->ring[RCS];
+
+	if (!ring) {
+		DRM_ERROR("context has no last ring\n");
+		ret = -EIO;
+		goto err;
+	}
+
+	if (!intel_ring_initialized(ring)) {
+		DRM_ERROR("ring not ready\n");
+		ret = -EIO;
+		goto err;
+	}
+
+	obj = dev_priv->sync_tl[ring->id];
+
+	pt = i915_sync_pt_create(obj);
+	if (!pt) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	fdata->name[sizeof(fdata->name) - 1] = '\0';
+	fence = sync_fence_create(fdata->name, pt);
+	if (!fence) {
+		sync_pt_free(pt);
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	fdata->fd = fd;
+
+	sync_fence_install(fence, fd);
+
+	mutex_unlock(&dev->struct_mutex);
+out:
+	return ret;
+
+err:
+	mutex_unlock(&dev->struct_mutex);
+	put_unused_fd(fd);
+	return ret;
+}
+
+/* FIXME: handle hangs */
+void i915_sync_signal(struct intel_engine_cs *ring)
+{
+	struct drm_i915_private *dev_priv = ring->dev->dev_private;
+
+	sync_timeline_signal(&dev_priv->sync_tl[ring->id]->obj);
+}
+
+int i915_sync_init(struct drm_i915_private *dev_priv)
+{
+	struct intel_engine_cs *ring;
+	struct i915_sync_timeline *tl;
+	int i, ret = 0;
+
+	for_each_ring(ring, dev_priv, i) {
+		tl = (struct i915_sync_timeline *)
+			sync_timeline_create(&i915_sync_ops,
+					     sizeof(struct i915_sync_timeline),
+					     ring->name);
+		if (!tl) {
+			ret = -ENOMEM;
+			goto out_err;
+		}
+		tl->dev_priv = dev_priv;
+		tl->ring = ring;
+		dev_priv->sync_tl[ring->id] = tl;
+	}
+
+	return ret;
+
+out_err:
+	for (i = 0; i < I915_NUM_RINGS; i++)
+		kfree(dev_priv->sync_tl[i]);
+
+	return ret;
+}
+
+void i915_sync_fini(struct drm_i915_private *dev_priv)
+{
+	int i;
+
+	for (i = 0; i < I915_NUM_RINGS; i++) {
+		struct i915_sync_timeline *tl = dev_priv->sync_tl[i];
+
+		sync_timeline_destroy(&tl->obj);
+	}
+}
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index ff57f07..65bd271 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -224,6 +224,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_I915_REG_READ		0x31
 #define DRM_I915_GET_RESET_STATS	0x32
 #define DRM_I915_GEM_USERPTR		0x33
+#define DRM_I915_GEM_FENCE		0x34
 
 #define DRM_IOCTL_I915_INIT		DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t)
 #define DRM_IOCTL_I915_FLUSH		DRM_IO ( DRM_COMMAND_BASE + DRM_I915_FLUSH)
@@ -275,6 +276,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_REG_READ			DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_REG_READ, struct drm_i915_reg_read)
 #define DRM_IOCTL_I915_GET_RESET_STATS		DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GET_RESET_STATS, struct drm_i915_reset_stats)
 #define DRM_IOCTL_I915_GEM_USERPTR			DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_USERPTR, struct drm_i915_gem_userptr)
+#define DRM_IOCTL_I915_GEM_FENCE			DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_FENCE, struct drm_i915_gem_fence)
 
 /* Allow drivers to submit batchbuffers directly to hardware, relying
  * on the security mechanisms provided by hardware.
@@ -1066,4 +1068,25 @@ struct drm_i915_gem_userptr {
 	__u32 handle;
 };
 
+/**
+ * drm_i915_gem_fence - create a fence
+ * @fd: fd for fence
+ * @ctx_id: context ID for fence
+ * @flags: flags for operation
+ *
+ * Creates a fence in @fd and returns it to the caller.  This fd can be
+ * passed around between processes as any other fd, and can be poll'd
+ * and read for status.
+ *
+ * RETURNS:
+ * A valid fd in the @fd field or an errno on error.
+ */
+struct drm_i915_gem_fence {
+	__s32 fd;
+	__u32 ctx_id;
+	__u32 flags;
+	__u32 pad;
+	char name[32];
+};
+
 #endif /* _UAPI_I915_DRM_H_ */
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Android sync points for i915

[Maarten Lankhorst]

Hey,
On 31-07-14 20:58, Jesse Barnes wrote:
> Expose an ioctl to create Android fences based on the Android sync point
> infrastructure (which in turn is based on DMA-buf fences).  Just a
> sketch at this point, no testing has been done.
>
> There are a couple of goals here:
>   1) allow applications and libraries to create fences without an
>      associated buffer
>   2) re-use a common API so userspace doesn't have to impedance mismatch
>      between different driver implementations too much
>   3) allow applications and libraries to use explicit synchronization if
>      they choose by exposing fences directly
>
Do you really need to use android timelines here? I think you should be 
able to use fence directly, and export android userspace fences anyway, 
you could easily wrap them.

~Maarten

Untested uncompiled patch for that below, looking from the documentation I may have
caused a memory leak by taking a reference on the sync_pt..
---
diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c
index ba0d69e..201b088 100644
--- a/drivers/staging/android/sync.c
+++ b/drivers/staging/android/sync.c
@@ -179,7 +179,7 @@ static void fence_check_cb_func(struct fence *f, struct fence_cb *cb)
 }
 
 /* TODO: implement a create which takes more that one sync_pt */
-struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt)
+struct sync_fence *sync_fence_create_dma(const char *name, struct fence *pt)
 {
 	struct sync_fence *fence;
 
@@ -190,16 +190,21 @@ struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt)
 	fence->num_fences = 1;
 	atomic_set(&fence->status, 1);
 
-	fence_get(&pt->base);
-	fence->cbs[0].sync_pt = &pt->base;
+	fence->cbs[0].sync_pt = fence_get(pt);
 	fence->cbs[0].fence = fence;
-	if (fence_add_callback(&pt->base, &fence->cbs[0].cb, fence_check_cb_func))
+	if (fence_add_callback(pt, &fence->cbs[0].cb, fence_check_cb_func))
 		atomic_dec(&fence->status);
 
 	sync_fence_debug_add(fence);
 
 	return fence;
 }
+EXPORT_SYMBOL(sync_fence_create_dma);
+
+struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt)
+{
+	return sync_fence_create_dma(name, &pt->base);
+}
 EXPORT_SYMBOL(sync_fence_create);
 
 struct sync_fence *sync_fence_fdget(int fd)
diff --git a/drivers/staging/android/sync.h b/drivers/staging/android/sync.h
index bcdd119..6d1682a 100644
--- a/drivers/staging/android/sync.h
+++ b/drivers/staging/android/sync.h
@@ -242,8 +242,9 @@ void sync_pt_free(struct sync_pt *pt);
  * @pt:		sync_pt to add to the fence
  *
  * Creates a fence containg @pt.  Once this is called, the fence takes
- * ownership of @pt.
+ * a reference on @pt.
  */
+struct sync_fence *sync_fence_create_dma(const char *name, struct fence *pt);
 struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt);
 
 /*

[PATCH] drm/i915: Android sync points for i915 v2

[Jesse Barnes]

Expose an ioctl to create Android fences based on the Android sync point
infrastructure (which in turn is based on DMA-buf fences).  Just a
sketch at this point, no testing has been done.

There are a couple of goals here:
  1) allow applications and libraries to create fences without an
     associated buffer
  2) re-use a common API so userspace doesn't have to impedance mismatch
     between different driver implementations too much
  3) allow applications and libraries to use explicit synchronization if
     they choose by exposing fences directly

v2: use struct fence directly using Maarten's new interface

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/Kconfig     |   2 +
 drivers/gpu/drm/i915/Makefile    |   1 +
 drivers/gpu/drm/i915/i915_dma.c  |   1 +
 drivers/gpu/drm/i915/i915_drv.h  |  10 ++
 drivers/gpu/drm/i915/i915_gem.c  |  15 +-
 drivers/gpu/drm/i915/i915_irq.c  |   4 +-
 drivers/gpu/drm/i915/i915_sync.c | 323 +++++++++++++++++++++++++++++++++++++++
 include/uapi/drm/i915_drm.h      |  23 +++
 8 files changed, 373 insertions(+), 6 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/i915_sync.c

diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index 4e39ab3..cd0f2ec 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -6,6 +6,8 @@ config DRM_I915
 	select INTEL_GTT
 	select AGP_INTEL if AGP
 	select INTERVAL_TREE
+	select ANDROID
+	select SYNC
 	# we need shmfs for the swappable backing store, and in particular
 	# the shmem_readpage() which depends upon tmpfs
 	select SHMEM
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 91bd167..61a3eb5c 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -25,6 +25,7 @@ i915-y += i915_cmd_parser.o \
 	  i915_gem_execbuffer.o \
 	  i915_gem_gtt.o \
 	  i915_gem.o \
+	  i915_sync.o \
 	  i915_gem_stolen.o \
 	  i915_gem_tiling.o \
 	  i915_gem_userptr.o \
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 2e7f03a..84086e1 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -2043,6 +2043,7 @@ const struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_REG_READ, i915_reg_read_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_GET_RESET_STATS, i915_get_reset_stats_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_GEM_USERPTR, i915_gem_userptr_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(I915_GEM_FENCE, i915_sync_create_fence_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
 };
 
 int i915_max_ioctl = ARRAY_SIZE(i915_ioctls);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d604f4f..6eb119e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1388,6 +1388,8 @@ struct i915_frontbuffer_tracking {
 	unsigned flip_bits;
 };
 
+struct i915_sync_timeline;
+
 struct drm_i915_private {
 	struct drm_device *dev;
 	struct kmem_cache *slab;
@@ -1422,6 +1424,8 @@ struct drm_i915_private {
 	struct drm_i915_gem_object *semaphore_obj;
 	uint32_t last_seqno, next_seqno;
 
+	struct i915_sync_timeline *sync_tl[I915_NUM_RINGS];
+
 	drm_dma_handle_t *status_page_dmah;
 	struct resource mch_res;
 
@@ -2275,6 +2279,12 @@ void i915_init_vm(struct drm_i915_private *dev_priv,
 void i915_gem_free_object(struct drm_gem_object *obj);
 void i915_gem_vma_destroy(struct i915_vma *vma);
 
+/* i915_sync.c */
+int i915_sync_init(struct drm_i915_private *dev_priv);
+void i915_sync_fini(struct drm_i915_private *dev_priv);
+int i915_sync_create_fence_ioctl(struct drm_device *dev, void *data,
+				 struct drm_file *file);
+
 #define PIN_MAPPABLE 0x1
 #define PIN_NONBLOCK 0x2
 #define PIN_GLOBAL 0x4
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index dcd8d7b..ace716e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1146,11 +1146,11 @@ static bool can_wait_boost(struct drm_i915_file_private *file_priv)
  * Returns 0 if the seqno was found within the alloted time. Else returns the
  * errno with remaining time filled in timeout argument.
  */
-static int __wait_seqno(struct intel_engine_cs *ring, u32 seqno,
-			unsigned reset_counter,
-			bool interruptible,
-			struct timespec *timeout,
-			struct drm_i915_file_private *file_priv)
+int __wait_seqno(struct intel_engine_cs *ring, u32 seqno,
+		 unsigned reset_counter,
+		 bool interruptible,
+		 struct timespec *timeout,
+		 struct drm_i915_file_private *file_priv)
 {
 	struct drm_device *dev = ring->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -4775,6 +4775,9 @@ int i915_gem_init(struct drm_device *dev)
 		atomic_set_mask(I915_WEDGED, &dev_priv->gpu_error.reset_counter);
 		ret = 0;
 	}
+
+	i915_sync_init(dev_priv);
+
 	mutex_unlock(&dev->struct_mutex);
 
 	/* Allow hardware batchbuffers unless told otherwise, but not for KMS. */
@@ -4970,6 +4973,8 @@ void i915_gem_release(struct drm_device *dev, struct drm_file *file)
 		request->file_priv = NULL;
 	}
 	spin_unlock(&file_priv->mm.lock);
+
+	i915_sync_fini(dev->dev_private);
 }
 
 static void
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 98abc22..149e083 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -33,6 +33,7 @@
 #include <linux/circ_buf.h>
 #include <drm/drmP.h>
 #include <drm/i915_drm.h>
+#include "../../../staging/android/sync.h"
 #include "i915_drv.h"
 #include "i915_trace.h"
 #include "intel_drv.h"
@@ -2617,8 +2618,9 @@ static void i915_error_wake_up(struct drm_i915_private *dev_priv,
 	 */
 
 	/* Wake up __wait_seqno, potentially holding dev->struct_mutex. */
-	for_each_ring(ring, dev_priv, i)
+	for_each_ring(ring, dev_priv, i) {
 		wake_up_all(&ring->irq_queue);
+	}
 
 	/* Wake up intel_crtc_wait_for_pending_flips, holding crtc->mutex. */
 	wake_up_all(&dev_priv->pending_flip_queue);
diff --git a/drivers/gpu/drm/i915/i915_sync.c b/drivers/gpu/drm/i915/i915_sync.c
new file mode 100644
index 0000000..4938616
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_sync.c
@@ -0,0 +1,323 @@
+/*
+ * Copyright © 2014 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Jesse Barnes <jbarnes@virtuousgeek.org>
+ *
+ */
+
+#include <drm/drmP.h>
+#include <drm/drm_vma_manager.h>
+#include <drm/i915_drm.h>
+#include "i915_drv.h"
+#include "i915_trace.h"
+#include "intel_drv.h"
+#include <linux/oom.h>
+#include <linux/shmem_fs.h>
+#include <linux/slab.h>
+#include <linux/swap.h>
+#include <linux/pci.h>
+#include <linux/dma-buf.h>
+#include "../../../staging/android/sync.h"
+
+/* Nothing really to protect here... */
+spinlock_t fence_lock;
+
+/*
+ * i915 fences on sync timelines
+ *
+ * We implement sync points in terms of i915 seqnos.  They're exposed
+ * through the new DRM_I915_GEM_FENCE ioctl, and can be mixed and matched
+ * with other Android timelines and aggregated into sync_fences, etc.
+ *
+ * TODO:
+ *   rebase on top of Chris's seqno/request stuff and use requests
+ *   allow non-RCS fences (need ring/context association)
+ */
+
+struct i915_fence {
+	struct fence base;
+	struct intel_engine_cs *ring;
+	struct intel_context *ctx;
+	wait_queue_t wait;
+	u32 seqno;
+};
+
+#define to_intel_fence(x) container_of(x, struct i915_fence, base)
+
+int __wait_seqno(struct intel_engine_cs *ring, u32 seqno,
+		 unsigned reset_counter,
+		 bool interruptible,
+		 struct timespec *timeout,
+		 struct drm_i915_file_private *file_priv);
+
+static const char *i915_fence_get_driver_name(struct fence *fence)
+{
+	return "i915";
+}
+
+static const char *i915_fence_get_timeline_name(struct fence *fence)
+{
+	struct i915_fence *intel_fence = to_intel_fence(fence);
+
+	return intel_fence->ring->name;
+}
+
+static int i915_fence_check(wait_queue_t *wait, unsigned mode, int flags,
+			    void *key)
+{
+	struct i915_fence *intel_fence = wait->private;
+	struct intel_engine_cs *ring = intel_fence->ring;
+
+	if (!i915_seqno_passed(ring->get_seqno(ring, false),
+			       intel_fence->seqno))
+		return 0;
+
+	fence_signal_locked(&intel_fence->base);
+
+	__remove_wait_queue(&ring->irq_queue, wait);
+	fence_put(&intel_fence->base);
+	ring->irq_put(ring);
+
+	return 0;
+}
+
+static bool i915_fence_enable_signaling(struct fence *fence)
+{
+	struct i915_fence *intel_fence = to_intel_fence(fence);
+	struct intel_engine_cs *ring = intel_fence->ring;
+	struct drm_i915_private *dev_priv = ring->dev->dev_private;
+	wait_queue_t *wait = &intel_fence->wait;
+
+	/* queue fence wait queue on irq queue and get fence */
+	if (i915_seqno_passed(ring->get_seqno(ring, false),
+			      intel_fence->seqno) ||
+	    i915_terminally_wedged(&dev_priv->gpu_error))
+		return false;
+
+	if (!ring->irq_get(ring))
+		return false;
+
+	wait->flags = 0;
+	wait->private = intel_fence;
+	wait->func = i915_fence_check;
+
+	__add_wait_queue(&ring->irq_queue, wait);
+	fence_get(fence);
+
+	return true;
+}
+
+static bool i915_fence_signaled(struct fence *fence)
+{
+	struct i915_fence *intel_fence = to_intel_fence(fence);
+	struct intel_engine_cs *ring = intel_fence->ring;
+
+	return i915_seqno_passed(ring->get_seqno(ring, false),
+				 intel_fence->seqno);
+}
+
+static signed long i915_fence_wait(struct fence *fence, bool intr,
+				   signed long timeout)
+{
+	struct i915_fence *intel_fence = to_intel_fence(fence);
+	struct drm_i915_private *dev_priv = intel_fence->ring->dev->dev_private;
+	struct timespec ts;
+	int ret;
+
+	jiffies_to_timespec(timeout, &ts);
+
+	ret = __wait_seqno(intel_fence->ring, intel_fence->seqno,
+			   atomic_read(&dev_priv->gpu_error.reset_counter),
+			   intr, &ts, NULL);
+	if (ret == -ETIME)
+		return timespec_to_jiffies(&ts);
+
+	return ret;
+}
+
+static int i915_fence_fill_driver_data(struct fence *fence, void *data,
+				      int size)
+{
+	struct i915_fence *intel_fence = to_intel_fence(fence);
+
+	if (size < sizeof(intel_fence->seqno))
+		return -ENOMEM;
+
+	memcpy(data, &intel_fence->seqno, sizeof(intel_fence->seqno));
+
+	return sizeof(intel_fence->seqno);
+}
+
+static void i915_fence_value_str(struct fence *fence, char *str, int size)
+{
+	struct i915_fence *intel_fence = to_intel_fence(fence);
+
+	snprintf(str, size, "%u", intel_fence->seqno);
+}
+
+static void i915_fence_timeline_value_str(struct fence *fence, char *str,
+					  int size)
+{
+	struct i915_fence *intel_fence = to_intel_fence(fence);
+	struct intel_engine_cs *ring = intel_fence->ring;
+
+	snprintf(str, size, "%u", ring->get_seqno(ring, false));
+}
+
+static struct fence_ops i915_fence_ops = {
+	.get_driver_name = 	i915_fence_get_driver_name,
+	.get_timeline_name =	i915_fence_get_timeline_name,
+	.enable_signaling =	i915_fence_enable_signaling,
+	.signaled =		i915_fence_signaled,
+	.wait =			i915_fence_wait,
+	.fill_driver_data =	i915_fence_fill_driver_data,
+	.fence_value_str =	i915_fence_value_str,
+	.timeline_value_str =	i915_fence_timeline_value_str,
+};
+
+static struct fence *i915_fence_create(struct intel_engine_cs *ring,
+				       struct intel_context *ctx)
+{
+	struct i915_fence *fence;
+	int ret;
+
+	fence = kzalloc(sizeof(*fence), GFP_KERNEL);
+	if (!fence)
+		return NULL;
+
+	ret = ring->add_request(ring);
+	if (ret) {
+		DRM_ERROR("add_request failed\n");
+		fence_free((struct fence *)fence);
+		return NULL;
+	}
+
+	fence->ring = ring;
+	fence->ctx = ctx;
+	fence->seqno = ring->outstanding_lazy_seqno;
+	fence_init(&fence->base, &i915_fence_ops, &fence_lock, ctx->user_handle,
+		   fence->seqno);
+
+	return &fence->base;
+}
+
+/**
+ * i915_sync_create_fence_ioctl - fence creation function
+ * @dev: drm device
+ * @data: ioctl data
+ * @file: file struct
+ *
+ * This function creates a fence given a context and ring, and returns
+ * it to the caller in the form of a file descriptor.
+ *
+ * The returned descriptor is a sync fence fd, and can be used with all
+ * the usual sync fence operations (poll, ioctl, etc).
+ *
+ * The process fd limit should prevent an overallocation of fence objects,
+ * which need to be destroyed manually with a close() call.
+ */
+int i915_sync_create_fence_ioctl(struct drm_device *dev, void *data,
+				 struct drm_file *file)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_gem_fence *fdata = data;
+	struct fence *fence;
+	struct sync_fence *sfence;
+	struct intel_engine_cs *ring;
+	struct intel_context *ctx;
+	u32 ctx_id = fdata->ctx_id;
+	int fd = get_unused_fd_flags(O_CLOEXEC);
+	int ret = 0;
+
+	if (file == NULL) {
+		DRM_ERROR("no file priv?\n");
+		return -EINVAL;
+	}
+
+	ret = i915_mutex_lock_interruptible(dev);
+	if (ret) {
+		DRM_ERROR("mutex interrupted\n");
+		goto out;
+	}
+
+	ctx = i915_gem_context_get(file->driver_priv, ctx_id);
+	if (ctx == NULL) {
+		DRM_ERROR("context lookup failed\n");
+		ret = -ENOENT;
+		goto err;
+	}
+
+	ring = &dev_priv->ring[RCS];
+
+	if (!intel_ring_initialized(ring)) {
+		DRM_ERROR("ring not ready\n");
+		ret = -EIO;
+		goto err;
+	}
+
+	fence = i915_fence_create(ring, ctx);
+	if (!fence) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	fdata->name[sizeof(fdata->name) - 1] = '\0';
+	sfence = sync_fence_create_dma(fdata->name, fence);
+	if (!sfence) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	fdata->fd = fd;
+
+	sync_fence_install(sfence, fd);
+
+	mutex_unlock(&dev->struct_mutex);
+out:
+	return ret;
+
+err:
+	mutex_unlock(&dev->struct_mutex);
+	put_unused_fd(fd);
+	return ret;
+}
+
+int i915_sync_init(struct drm_i915_private *dev_priv)
+{
+	struct intel_engine_cs *ring;
+	int i, ret = 0;
+
+	for_each_ring(ring, dev_priv, i) {
+		/* FIXME: non-RCS fences */
+	}
+
+	return ret;
+}
+
+void i915_sync_fini(struct drm_i915_private *dev_priv)
+{
+	int i;
+
+	for (i = 0; i < I915_NUM_RINGS; i++) {
+	}
+}
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index ff57f07..65bd271 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -224,6 +224,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_I915_REG_READ		0x31
 #define DRM_I915_GET_RESET_STATS	0x32
 #define DRM_I915_GEM_USERPTR		0x33
+#define DRM_I915_GEM_FENCE		0x34
 
 #define DRM_IOCTL_I915_INIT		DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t)
 #define DRM_IOCTL_I915_FLUSH		DRM_IO ( DRM_COMMAND_BASE + DRM_I915_FLUSH)
@@ -275,6 +276,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_REG_READ			DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_REG_READ, struct drm_i915_reg_read)
 #define DRM_IOCTL_I915_GET_RESET_STATS		DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GET_RESET_STATS, struct drm_i915_reset_stats)
 #define DRM_IOCTL_I915_GEM_USERPTR			DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_USERPTR, struct drm_i915_gem_userptr)
+#define DRM_IOCTL_I915_GEM_FENCE			DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_FENCE, struct drm_i915_gem_fence)
 
 /* Allow drivers to submit batchbuffers directly to hardware, relying
  * on the security mechanisms provided by hardware.
@@ -1066,4 +1068,25 @@ struct drm_i915_gem_userptr {
 	__u32 handle;
 };
 
+/**
+ * drm_i915_gem_fence - create a fence
+ * @fd: fd for fence
+ * @ctx_id: context ID for fence
+ * @flags: flags for operation
+ *
+ * Creates a fence in @fd and returns it to the caller.  This fd can be
+ * passed around between processes as any other fd, and can be poll'd
+ * and read for status.
+ *
+ * RETURNS:
+ * A valid fd in the @fd field or an errno on error.
+ */
+struct drm_i915_gem_fence {
+	__s32 fd;
+	__u32 ctx_id;
+	__u32 flags;
+	__u32 pad;
+	char name[32];
+};
+
 #endif /* _UAPI_I915_DRM_H_ */
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Android sync points for i915 v2

[Daniel Vetter]

On Tue, Aug 5, 2014 at 1:18 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> +#define DRM_IOCTL_I915_GEM_FENCE                       DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_FENCE, struct drm_i915_gem_fence)
>
>  /* Allow drivers to submit batchbuffers directly to hardware, relying
>   * on the security mechanisms provided by hardware.
> @@ -1066,4 +1068,25 @@ struct drm_i915_gem_userptr {
>         __u32 handle;
>  };
>
> +/**
> + * drm_i915_gem_fence - create a fence
> + * @fd: fd for fence
> + * @ctx_id: context ID for fence
> + * @flags: flags for operation
> + *
> + * Creates a fence in @fd and returns it to the caller.  This fd can be
> + * passed around between processes as any other fd, and can be poll'd
> + * and read for status.
> + *
> + * RETURNS:
> + * A valid fd in the @fd field or an errno on error.
> + */
> +struct drm_i915_gem_fence {
> +       __s32 fd;
> +       __u32 ctx_id;
> +       __u32 flags;
> +       __u32 pad;
> +       char name[32];
> +};
> +

This doesn't really look like the interface I'd expected. Imo we just
need to add a flag to execbuf so that userspace can tell the kernel to
create a fence for that execbuf, and switch one of the leftover rsvd
fields to __s32 as an outparam for the fd.

Then we need similar flags for vblank events and pageflips to do the
same (obviously those are drm core patches) and it's all there. That
should probably integrated as a special type of drm_event, so that
drivers don't need to change a single line of code.

Also this should be based on top of Chris' patch to refcount requests
and make them first-class structures. Then we can simply replace the
embedded struct kref with a struct fence, i.e. we'll always create a
fence, but only give userspace an fd handle for it when it asks for
it.

For merging there's a few things we need:
- Some open-source user, either the open-source android-ia project or
something else.
- The android syncpt stuff obviously needs to be de-staged. From my
side that means an ABI review of what's there (and getting the buy-in
from google guys if we need to change it) plus a full set of testcases
(if google doesn't already have something we could integrate easily).
Adding Greg and relevant people.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Android sync points for i915 v2

[Jesse Barnes]

On Tue, 5 Aug 2014 09:44:00 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Aug 5, 2014 at 1:18 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > +#define DRM_IOCTL_I915_GEM_FENCE                       DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_FENCE, struct drm_i915_gem_fence)
> >
> >  /* Allow drivers to submit batchbuffers directly to hardware, relying
> >   * on the security mechanisms provided by hardware.
> > @@ -1066,4 +1068,25 @@ struct drm_i915_gem_userptr {
> >         __u32 handle;
> >  };
> >
> > +/**
> > + * drm_i915_gem_fence - create a fence
> > + * @fd: fd for fence
> > + * @ctx_id: context ID for fence
> > + * @flags: flags for operation
> > + *
> > + * Creates a fence in @fd and returns it to the caller.  This fd can be
> > + * passed around between processes as any other fd, and can be poll'd
> > + * and read for status.
> > + *
> > + * RETURNS:
> > + * A valid fd in the @fd field or an errno on error.
> > + */
> > +struct drm_i915_gem_fence {
> > +       __s32 fd;
> > +       __u32 ctx_id;
> > +       __u32 flags;
> > +       __u32 pad;
> > +       char name[32];
> > +};
> > +
> 
> This doesn't really look like the interface I'd expected. Imo we just
> need to add a flag to execbuf so that userspace can tell the kernel to
> create a fence for that execbuf, and switch one of the leftover rsvd
> fields to __s32 as an outparam for the fd.

Given that I've got a new execbuf coming too, I just wanted to keep
them separate.  Any compelling reason to try to wedge it into execbuf?

> Then we need similar flags for vblank events and pageflips to do the
> same (obviously those are drm core patches) and it's all there. That
> should probably integrated as a special type of drm_event, so that
> drivers don't need to change a single line of code.

Except for actually using the fences...

> Also this should be based on top of Chris' patch to refcount requests
> and make them first-class structures. Then we can simply replace the
> embedded struct kref with a struct fence, i.e. we'll always create a
> fence, but only give userspace an fd handle for it when it asks for
> it.

Yeah I think that was mentioned in the commit.  Once Chris's stuff
lands this should look even simpler.

> For merging there's a few things we need:
> - Some open-source user, either the open-source android-ia project or
> something else.
> - The android syncpt stuff obviously needs to be de-staged. From my
> side that means an ABI review of what's there (and getting the buy-in
> from google guys if we need to change it) plus a full set of testcases
> (if google doesn't already have something we could integrate easily).
> Adding Greg and relevant people.

Yep, I'm hoping Chris has a use for this too in the DDX.  I think
Wayland wants it too.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Android sync points for i915 v2

[Maarten Lankhorst]

op 05-08-14 16:59, Jesse Barnes schreef:
> On Tue, 5 Aug 2014 09:44:00 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
>
>> On Tue, Aug 5, 2014 at 1:18 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>>> +#define DRM_IOCTL_I915_GEM_FENCE                       DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_FENCE, struct drm_i915_gem_fence)
>>>
>>>  /* Allow drivers to submit batchbuffers directly to hardware, relying
>>>   * on the security mechanisms provided by hardware.
>>> @@ -1066,4 +1068,25 @@ struct drm_i915_gem_userptr {
>>>         __u32 handle;
>>>  };
>>>
>>> +/**
>>> + * drm_i915_gem_fence - create a fence
>>> + * @fd: fd for fence
>>> + * @ctx_id: context ID for fence
>>> + * @flags: flags for operation
>>> + *
>>> + * Creates a fence in @fd and returns it to the caller.  This fd can be
>>> + * passed around between processes as any other fd, and can be poll'd
>>> + * and read for status.
>>> + *
>>> + * RETURNS:
>>> + * A valid fd in the @fd field or an errno on error.
>>> + */
>>> +struct drm_i915_gem_fence {
>>> +       __s32 fd;
>>> +       __u32 ctx_id;
>>> +       __u32 flags;
>>> +       __u32 pad;
>>> +       char name[32];
>>> +};
>>> +
>> This doesn't really look like the interface I'd expected. Imo we just
>> need to add a flag to execbuf so that userspace can tell the kernel to
>> create a fence for that execbuf, and switch one of the leftover rsvd
>> fields to __s32 as an outparam for the fd.
> Given that I've got a new execbuf coming too, I just wanted to keep
> them separate.  Any compelling reason to try to wedge it into execbuf?
It's the right place to put it, and you can get the fence for nearly free because you just created it.

~Maarten

Re: [PATCH] drm/i915: Android sync points for i915 v2

[Daniel Vetter]

On Tue, Aug 5, 2014 at 4:59 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> This doesn't really look like the interface I'd expected. Imo we just
>> need to add a flag to execbuf so that userspace can tell the kernel to
>> create a fence for that execbuf, and switch one of the leftover rsvd
>> fields to __s32 as an outparam for the fd.
>
> Given that I've got a new execbuf coming too, I just wanted to keep
> them separate.  Any compelling reason to try to wedge it into execbuf?

The new execbuf is for svm, and there we obviously need fences. But we
also need proper fence support everywhere else (hence also the comment
that we need support for fences in drm events).

>> Then we need similar flags for vblank events and pageflips to do the
>> same (obviously those are drm core patches) and it's all there. That
>> should probably integrated as a special type of drm_event, so that
>> drivers don't need to change a single line of code.
>
> Except for actually using the fences...

Actually no, nothing needed - drivers already signal drm_events in all
the right places, so we really only need to change
drm_send_vblank_event. And ofc we need to rework the code in the
pageflip/atomic/vblank_wait ioctl code in the drm core to create a
fence (and return it to userspace) instead of a normal drm event.

>> Also this should be based on top of Chris' patch to refcount requests
>> and make them first-class structures. Then we can simply replace the
>> embedded struct kref with a struct fence, i.e. we'll always create a
>> fence, but only give userspace an fd handle for it when it asks for
>> it.
>
> Yeah I think that was mentioned in the commit.  Once Chris's stuff
> lands this should look even simpler.
>
>> For merging there's a few things we need:
>> - Some open-source user, either the open-source android-ia project or
>> something else.
>> - The android syncpt stuff obviously needs to be de-staged. From my
>> side that means an ABI review of what's there (and getting the buy-in
>> from google guys if we need to change it) plus a full set of testcases
>> (if google doesn't already have something we could integrate easily).
>> Adding Greg and relevant people.
>
> Yep, I'm hoping Chris has a use for this too in the DDX.  I think
> Wayland wants it too.

Well since syncpts are originally from Android I'd really prefere an
Android based implementation - otherwise we might create something by
accident that's not suitable for Android.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Android sync points for i915 v2

[Jesse Barnes]

On Tue, 5 Aug 2014 17:08:22 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Aug 5, 2014 at 4:59 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> >> This doesn't really look like the interface I'd expected. Imo we just
> >> need to add a flag to execbuf so that userspace can tell the kernel to
> >> create a fence for that execbuf, and switch one of the leftover rsvd
> >> fields to __s32 as an outparam for the fd.
> >
> > Given that I've got a new execbuf coming too, I just wanted to keep
> > them separate.  Any compelling reason to try to wedge it into execbuf?
> 
> The new execbuf is for svm, and there we obviously need fences. But we
> also need proper fence support everywhere else (hence also the comment
> that we need support for fences in drm events).
> 
> >> Then we need similar flags for vblank events and pageflips to do the
> >> same (obviously those are drm core patches) and it's all there. That
> >> should probably integrated as a special type of drm_event, so that
> >> drivers don't need to change a single line of code.
> >
> > Except for actually using the fences...
> 
> Actually no, nothing needed - drivers already signal drm_events in all
> the right places, so we really only need to change
> drm_send_vblank_event. And ofc we need to rework the code in the
> pageflip/atomic/vblank_wait ioctl code in the drm core to create a
> fence (and return it to userspace) instead of a normal drm event.

Actually yes.  You get back a fence object and want to do something
with it, right?  That means new code.  Plus modifying current execbuf
users that want fences to pass in a flag.

> >> Also this should be based on top of Chris' patch to refcount requests
> >> and make them first-class structures. Then we can simply replace the
> >> embedded struct kref with a struct fence, i.e. we'll always create a
> >> fence, but only give userspace an fd handle for it when it asks for
> >> it.
> >
> > Yeah I think that was mentioned in the commit.  Once Chris's stuff
> > lands this should look even simpler.
> >
> >> For merging there's a few things we need:
> >> - Some open-source user, either the open-source android-ia project or
> >> something else.
> >> - The android syncpt stuff obviously needs to be de-staged. From my
> >> side that means an ABI review of what's there (and getting the buy-in
> >> from google guys if we need to change it) plus a full set of testcases
> >> (if google doesn't already have something we could integrate easily).
> >> Adding Greg and relevant people.
> >
> > Yep, I'm hoping Chris has a use for this too in the DDX.  I think
> > Wayland wants it too.
> 
> Well since syncpts are originally from Android I'd really prefere an
> Android based implementation - otherwise we might create something by
> accident that's not suitable for Android.

I don't see how using the Android sync points API might make something
not suitable for Android?

But yes, I want the Android guys to try this out too.  I've already
pinged them internally to check things out.  Probably the biggest
remaining opens there would be having a timeline for the display side
of things too that covers vblank and page flip events as fences in a
separate namespace.

Which makes me think going back to just using the Android structs
directly might be easier...

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Android sync points for i915 v2

[Daniel Vetter]

On Tue, Aug 5, 2014 at 6:05 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Tue, 5 Aug 2014 17:08:22 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
>
>> On Tue, Aug 5, 2014 at 4:59 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> >> This doesn't really look like the interface I'd expected. Imo we just
>> >> need to add a flag to execbuf so that userspace can tell the kernel to
>> >> create a fence for that execbuf, and switch one of the leftover rsvd
>> >> fields to __s32 as an outparam for the fd.
>> >
>> > Given that I've got a new execbuf coming too, I just wanted to keep
>> > them separate.  Any compelling reason to try to wedge it into execbuf?
>>
>> The new execbuf is for svm, and there we obviously need fences. But we
>> also need proper fence support everywhere else (hence also the comment
>> that we need support for fences in drm events).
>>
>> >> Then we need similar flags for vblank events and pageflips to do the
>> >> same (obviously those are drm core patches) and it's all there. That
>> >> should probably integrated as a special type of drm_event, so that
>> >> drivers don't need to change a single line of code.
>> >
>> > Except for actually using the fences...
>>
>> Actually no, nothing needed - drivers already signal drm_events in all
>> the right places, so we really only need to change
>> drm_send_vblank_event. And ofc we need to rework the code in the
>> pageflip/atomic/vblank_wait ioctl code in the drm core to create a
>> fence (and return it to userspace) instead of a normal drm event.
>
> Actually yes.  You get back a fence object and want to do something
> with it, right?  That means new code.  Plus modifying current execbuf
> users that want fences to pass in a flag.

This comment was specifically about vblank and pageflips, _not_ about
execbuf. At least that's been what I've thought while writing the
original mail and reading your reply. Looks like we have a
misunderstanding here. Since for vblank and pageflip we really can do
it all in the drm core.

>> >> Also this should be based on top of Chris' patch to refcount requests
>> >> and make them first-class structures. Then we can simply replace the
>> >> embedded struct kref with a struct fence, i.e. we'll always create a
>> >> fence, but only give userspace an fd handle for it when it asks for
>> >> it.
>> >
>> > Yeah I think that was mentioned in the commit.  Once Chris's stuff
>> > lands this should look even simpler.
>> >
>> >> For merging there's a few things we need:
>> >> - Some open-source user, either the open-source android-ia project or
>> >> something else.
>> >> - The android syncpt stuff obviously needs to be de-staged. From my
>> >> side that means an ABI review of what's there (and getting the buy-in
>> >> from google guys if we need to change it) plus a full set of testcases
>> >> (if google doesn't already have something we could integrate easily).
>> >> Adding Greg and relevant people.
>> >
>> > Yep, I'm hoping Chris has a use for this too in the DDX.  I think
>> > Wayland wants it too.
>>
>> Well since syncpts are originally from Android I'd really prefere an
>> Android based implementation - otherwise we might create something by
>> accident that's not suitable for Android.
>
> I don't see how using the Android sync points API might make something
> not suitable for Android?

Well for de-staging someone gets to do a review of the android synpts
interface. And depending upon how syncpts are used in the hwc and gl
extensions to get at those fences and use them we'll have different
requirements for the i915-specific execbuf support. And for the
generic support in the drm core for pageflips.

> But yes, I want the Android guys to try this out too.  I've already
> pinged them internally to check things out.  Probably the biggest
> remaining opens there would be having a timeline for the display side
> of things too that covers vblank and page flip events as fences in a
> separate namespace.
>
> Which makes me think going back to just using the Android structs
> directly might be easier...

Why that? They should really just provide the userspace interface on top ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Android sync points for i915 v2

[Jesse Barnes]

On Tue, 5 Aug 2014 18:08:16 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Aug 5, 2014 at 6:05 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > But yes, I want the Android guys to try this out too.  I've already
> > pinged them internally to check things out.  Probably the biggest
> > remaining opens there would be having a timeline for the display side
> > of things too that covers vblank and page flip events as fences in a
> > separate namespace.
> >
> > Which makes me think going back to just using the Android structs
> > directly might be easier...
> 
> Why that? They should really just provide the userspace interface on top ...

Because the internal implementation has the nice timeline/sync pt/fence
abstraction.  It just fits nicely, conceptually, with the various
timelines we have in gfx.

For fences, I guess we'll need separate fence contexts for display and
the rings.  And we can still combine them with the sync_fence api, so
it should be equivalent.  I'm just still thinking in terms of the
Android bits and not the struct fence bits; I'll probably come around
eventually.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Android sync points for i915 v2

[Jesse Barnes]

On Tue, 5 Aug 2014 18:08:16 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Aug 5, 2014 at 6:05 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > On Tue, 5 Aug 2014 17:08:22 +0200
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> >> On Tue, Aug 5, 2014 at 4:59 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> >> >> This doesn't really look like the interface I'd expected. Imo we just
> >> >> need to add a flag to execbuf so that userspace can tell the kernel to
> >> >> create a fence for that execbuf, and switch one of the leftover rsvd
> >> >> fields to __s32 as an outparam for the fd.
> >> >
> >> > Given that I've got a new execbuf coming too, I just wanted to keep
> >> > them separate.  Any compelling reason to try to wedge it into execbuf?
> >>
> >> The new execbuf is for svm, and there we obviously need fences. But we
> >> also need proper fence support everywhere else (hence also the comment
> >> that we need support for fences in drm events).
> >>
> >> >> Then we need similar flags for vblank events and pageflips to do the
> >> >> same (obviously those are drm core patches) and it's all there. That
> >> >> should probably integrated as a special type of drm_event, so that
> >> >> drivers don't need to change a single line of code.
> >> >
> >> > Except for actually using the fences...
> >>
> >> Actually no, nothing needed - drivers already signal drm_events in all
> >> the right places, so we really only need to change
> >> drm_send_vblank_event. And ofc we need to rework the code in the
> >> pageflip/atomic/vblank_wait ioctl code in the drm core to create a
> >> fence (and return it to userspace) instead of a normal drm event.
> >
> > Actually yes.  You get back a fence object and want to do something
> > with it, right?  That means new code.  Plus modifying current execbuf
> > users that want fences to pass in a flag.
> 
> This comment was specifically about vblank and pageflips, _not_ about
> execbuf. At least that's been what I've thought while writing the
> original mail and reading your reply. Looks like we have a
> misunderstanding here. Since for vblank and pageflip we really can do
> it all in the drm core.

I was thinking of userspace drivers and userspace, not kernel
internals...  that's the disconnect.

Re: [PATCH] drm/i915: Android sync points for i915 v2

[Daniel Vetter]

On Tue, Aug 5, 2014 at 7:09 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> >> >> Then we need similar flags for vblank events and pageflips to do the
>> >> >> same (obviously those are drm core patches) and it's all there. That
>> >> >> should probably integrated as a special type of drm_event, so that
>> >> >> drivers don't need to change a single line of code.
>> >> >
>> >> > Except for actually using the fences...
>> >>
>> >> Actually no, nothing needed - drivers already signal drm_events in all
>> >> the right places, so we really only need to change
>> >> drm_send_vblank_event. And ofc we need to rework the code in the
>> >> pageflip/atomic/vblank_wait ioctl code in the drm core to create a
>> >> fence (and return it to userspace) instead of a normal drm event.
>> >
>> > Actually yes.  You get back a fence object and want to do something
>> > with it, right?  That means new code.  Plus modifying current execbuf
>> > users that want fences to pass in a flag.
>>
>> This comment was specifically about vblank and pageflips, _not_ about
>> execbuf. At least that's been what I've thought while writing the
>> original mail and reading your reply. Looks like we have a
>> misunderstanding here. Since for vblank and pageflip we really can do
>> it all in the drm core.
>
> I was thinking of userspace drivers and userspace, not kernel
> internals...  that's the disconnect.

Ah yeah, userspace needs to be adjusted of course to use fences as
flip completion events instead of drm events received through the drm
fd. But if we have a generic kms userspace (android hwc or wayland or
whatever) that part shouldn't have any driver-specific logic either.
And I've thought hwc expects fences for flip completion so that it
knows when it can start to reuse buffers and render into them again.
So at least on Android I think we need this, and I guess wayland
wouldn't mind if it could deal with fences exclusively.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Android sync points for i915 v2

[Jesse Barnes]

On Tue, 5 Aug 2014 19:43:22 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Aug 5, 2014 at 7:09 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> >> >> >> Then we need similar flags for vblank events and pageflips to do the
> >> >> >> same (obviously those are drm core patches) and it's all there. That
> >> >> >> should probably integrated as a special type of drm_event, so that
> >> >> >> drivers don't need to change a single line of code.
> >> >> >
> >> >> > Except for actually using the fences...
> >> >>
> >> >> Actually no, nothing needed - drivers already signal drm_events in all
> >> >> the right places, so we really only need to change
> >> >> drm_send_vblank_event. And ofc we need to rework the code in the
> >> >> pageflip/atomic/vblank_wait ioctl code in the drm core to create a
> >> >> fence (and return it to userspace) instead of a normal drm event.
> >> >
> >> > Actually yes.  You get back a fence object and want to do something
> >> > with it, right?  That means new code.  Plus modifying current execbuf
> >> > users that want fences to pass in a flag.
> >>
> >> This comment was specifically about vblank and pageflips, _not_ about
> >> execbuf. At least that's been what I've thought while writing the
> >> original mail and reading your reply. Looks like we have a
> >> misunderstanding here. Since for vblank and pageflip we really can do
> >> it all in the drm core.
> >
> > I was thinking of userspace drivers and userspace, not kernel
> > internals...  that's the disconnect.
> 
> Ah yeah, userspace needs to be adjusted of course to use fences as
> flip completion events instead of drm events received through the drm
> fd. But if we have a generic kms userspace (android hwc or wayland or
> whatever) that part shouldn't have any driver-specific logic either.
> And I've thought hwc expects fences for flip completion so that it
> knows when it can start to reuse buffers and render into them again.
> So at least on Android I think we need this, and I guess wayland
> wouldn't mind if it could deal with fences exclusively.

Yeah, Wayland doesn't currently use them, but it could for
synchronizing client buffers against what it wants to draw for the next
frame (i.e. if a client fence has passed when the compositor decides to
draw its next frame, it gets included, otherwise the previous frame is
included in the newly composed display buffer).

I think they can probably be made generic, either by adding a new flag
to mode sets/flips/vblank calls to return a fence, or by creating a
separate display timeline to be passed in to a fence ioctl call.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Android sync points for i915 v2

[Maarten Lankhorst]

op 05-08-14 01:18, Jesse Barnes schreef:
> Expose an ioctl to create Android fences based on the Android sync point
> infrastructure (which in turn is based on DMA-buf fences).  Just a
> sketch at this point, no testing has been done.
>
> There are a couple of goals here:
>   1) allow applications and libraries to create fences without an
>      associated buffer
>   2) re-use a common API so userspace doesn't have to impedance mismatch
>      between different driver implementations too much
>   3) allow applications and libraries to use explicit synchronization if
>      they choose by exposing fences directly
>
> v2: use struct fence directly using Maarten's new interface

> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d604f4f..6eb119e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1388,6 +1388,8 @@ struct i915_frontbuffer_tracking {
>  	unsigned flip_bits;
>  };
>  
> +struct i915_sync_timeline;
> +
>  struct drm_i915_private {
>  	struct drm_device *dev;
>  	struct kmem_cache *slab;
> @@ -1422,6 +1424,8 @@ struct drm_i915_private {
>  	struct drm_i915_gem_object *semaphore_obj;
>  	uint32_t last_seqno, next_seqno;
>  
> +	struct i915_sync_timeline *sync_tl[I915_NUM_RINGS];
> +
>  	drm_dma_handle_t *status_page_dmah;
>  	struct resource mch_res;
>  
Leftover remnant?

I think you should rebase on top of Chris' seqno/request stuff like you said in TODO, it would reduce the patch to just the ioctl. ;-)

~Maarten

Re: [PATCH] drm/i915: Android sync points for i915 v2

[Jesse Barnes]

On Tue, 05 Aug 2014 10:09:56 +0200
Maarten Lankhorst <maarten.lankhorst@canonical.com> wrote:

> op 05-08-14 01:18, Jesse Barnes schreef:
> > Expose an ioctl to create Android fences based on the Android sync point
> > infrastructure (which in turn is based on DMA-buf fences).  Just a
> > sketch at this point, no testing has been done.
> >
> > There are a couple of goals here:
> >   1) allow applications and libraries to create fences without an
> >      associated buffer
> >   2) re-use a common API so userspace doesn't have to impedance mismatch
> >      between different driver implementations too much
> >   3) allow applications and libraries to use explicit synchronization if
> >      they choose by exposing fences directly
> >
> > v2: use struct fence directly using Maarten's new interface
> 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index d604f4f..6eb119e 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1388,6 +1388,8 @@ struct i915_frontbuffer_tracking {
> >  	unsigned flip_bits;
> >  };
> >  
> > +struct i915_sync_timeline;
> > +
> >  struct drm_i915_private {
> >  	struct drm_device *dev;
> >  	struct kmem_cache *slab;
> > @@ -1422,6 +1424,8 @@ struct drm_i915_private {
> >  	struct drm_i915_gem_object *semaphore_obj;
> >  	uint32_t last_seqno, next_seqno;
> >  
> > +	struct i915_sync_timeline *sync_tl[I915_NUM_RINGS];
> > +
> >  	drm_dma_handle_t *status_page_dmah;
> >  	struct resource mch_res;
> >  
> Leftover remnant?

Oops, yeah.  I still need to add a way to request a fence on a specific
ring, and that ties into our context API, so that's still open.  But I
definitely don't want a sync timeline here with this approach.

> I think you should rebase on top of Chris' seqno/request stuff like you said in TODO, it would reduce the patch to just the ioctl. ;-)

I need to check if it landed yet, but yes it will make things even
simpler.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Android sync points for i915 v2

[Daniel Vetter]

On Tue, Aug 5, 2014 at 5:03 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> Oops, yeah.  I still need to add a way to request a fence on a specific
> ring, and that ties into our context API, so that's still open.  But I
> definitely don't want a sync timeline here with this approach.

Wrt timelines I think we want one per ring and per ctx (especially
with execlist) due to scheduling and all that. So we'll need _lots_ of
them ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Android sync points for i915

[Tvrtko Ursulin]

Hi Jesse,

On 07/31/2014 07:58 PM, Jesse Barnes wrote:
> Expose an ioctl to create Android fences based on the Android sync point
> infrastructure (which in turn is based on DMA-buf fences).  Just a
> sketch at this point, no testing has been done.
>
> There are a couple of goals here:
>    1) allow applications and libraries to create fences without an
>       associated buffer
>    2) re-use a common API so userspace doesn't have to impedance mismatch
>       between different driver implementations too much
>    3) allow applications and libraries to use explicit synchronization if
>       they choose by exposing fences directly
>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

[snip]

> +
> +/*
> + * i915 fences on sync timelines
> + *
> + * We implement sync points in terms of i915 seqnos.  They're exposed
> + * through the new DRM_I915_GEM_FENCE ioctl, and can be mixed and matched
> + * with other Android timelines and aggregated into sync_fences, etc.
> + *
> + * TODO:
> + *   rebase on top of Chris's seqno/request stuff and use requests
> + *   allow non-RCS fences (need ring/context association)
> + */
> +
> +struct i915_sync_timeline {
> +	struct sync_timeline obj;
> +	struct intel_engine_cs *ring;
> +	struct drm_i915_private *dev_priv;
> +};
> +
> +struct i915_sync_pt {
> +	struct sync_pt pt;
> +	u32 seqno;
> +};

In case one day more than seqno needs to be exported to userspace, 
perhaps it would be handy to version the driver data somehow to allow 
for some forward/backward compatibility? Unless kernel/libdrm are 
supposed to be updated in lock-step already.

Regards,

Tvrtko

Re: [PATCH] drm/i915: Android sync points for i915

[Jesse Barnes]

On Fri, 01 Aug 2014 10:04:55 +0100
Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:

> Hi Jesse,
> 
> On 07/31/2014 07:58 PM, Jesse Barnes wrote:
> > Expose an ioctl to create Android fences based on the Android sync point
> > infrastructure (which in turn is based on DMA-buf fences).  Just a
> > sketch at this point, no testing has been done.
> >
> > There are a couple of goals here:
> >    1) allow applications and libraries to create fences without an
> >       associated buffer
> >    2) re-use a common API so userspace doesn't have to impedance mismatch
> >       between different driver implementations too much
> >    3) allow applications and libraries to use explicit synchronization if
> >       they choose by exposing fences directly
> >
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> [snip]
> 
> > +
> > +/*
> > + * i915 fences on sync timelines
> > + *
> > + * We implement sync points in terms of i915 seqnos.  They're exposed
> > + * through the new DRM_I915_GEM_FENCE ioctl, and can be mixed and matched
> > + * with other Android timelines and aggregated into sync_fences, etc.
> > + *
> > + * TODO:
> > + *   rebase on top of Chris's seqno/request stuff and use requests
> > + *   allow non-RCS fences (need ring/context association)
> > + */
> > +
> > +struct i915_sync_timeline {
> > +	struct sync_timeline obj;
> > +	struct intel_engine_cs *ring;
> > +	struct drm_i915_private *dev_priv;
> > +};
> > +
> > +struct i915_sync_pt {
> > +	struct sync_pt pt;
> > +	u32 seqno;
> > +};
> 
> In case one day more than seqno needs to be exported to userspace, 
> perhaps it would be handy to version the driver data somehow to allow 
> for some forward/backward compatibility? Unless kernel/libdrm are 
> supposed to be updated in lock-step already.

This is the structure we expose to userspace:

struct drm_i915_gem_fence {
	__s32 fd;
	__u32 ctx_id;
	__u32 flags;
	__u32 pad;
	char name[32];
};

It might be good to version it, but fundamentally we're talking about
fences on a given context's command stream, with an opaque fd, so this
seems sufficient, even if we did want to add additional seqnos in the
internals later on.

-- 
Jesse Barnes, Intel Open Source Technology Center