Skip to contentSkip to conversation

fence: dma-buf cross-device synchronization (v14)

PATCH New Maarten Lankhorst 2 participants 10 messages Aug 15, 2013 – Aug 20, 2013

Subsystems (9)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 fence: dma-buf cross-device synchronization (v14)

A fence can be attached to a buffer which is being filled or consumed by hw, to allow userspace to pass the buffer without waiting to another device. For example, userspace can call page_flip ioctl to display the next frame of graphics after kicking the GPU but while the GPU is still rendering. The display device sharing the buffer with the GPU would attach a callback to get notified when the GPU's rendering-complete IRQ fires, to update the scan-out address of the display, without having to wake up userspace.

A driver must allocate a fence context for each execution ring that can run in parallel. The function for this takes an argument with how many contexts to allocate:
+ fence_context_alloc()

A fence is transient, one-shot deal. It is allocated and attached to one or more dma-buf's. When the one that attached it is done, with the pending operation, it can signal the fence:
+ fence_signal()

To have a rough approximation whether a fence is fired, call:
+ fence_is_signaled()

The dma-buf-mgr handles tracking, and waiting on, the fences associated with a dma-buf.

The one pending on the fence can add an async callback:
+ fence_add_callback()

The callback can optionally be cancelled with:
+ fence_remove_callback()

To wait synchronously, optionally with a timeout:
+ fence_wait()
+ fence_wait_timeout()

A default software-only implementation is provided, which can be used by drivers attaching a fence to a buffer when they have no other means for hw sync. But a memory backed fence is also envisioned, because it is common that GPU's can write to, or poll on some memory location for synchronization. For example:

  fence = custom_get_fence(...);
  if ((seqno_fence = to_seqno_fence(fence)) != NULL) {
    dma_buf *fence_buf = fence->sync_buf;
    get_dma_buf(fence_buf);

... tell the hw the memory location to wait ... custom_wait_on(fence_buf, fence->seqno_ofs, fence->seqno);

  } else {
    /* fall-back to sw sync * /
    fence_add_callback(fence, my_cb);

}

On SoC platforms, if some other hw mechanism is provided for synchronizing between IP blocks, it could be supported as an alternate implementation with it's own fence ops in a similar way.

enable_signaling callback is used to provide sw signaling in case a cpu waiter is requested or no compatible hardware signaling could be used.

The intention is to provide a userspace interface (presumably via eventfd) later, to be used in conjunction with dma-buf's mmap support for sw access to buffers (or for userspace apps that would prefer to do their own synchronization).

v1: Original
v2: After discussion w/ danvet and mlankhorst on #dri-devel, we decided
that dma-fence didn't need to care about the sw->hw signaling path (it can be handled same as sw->sw case), and therefore the fence->ops can be simplified and more handled in the core. So remove the signal, add_callback, cancel_callback, and wait ops, and replace with a simple enable_signaling() op which can be used to inform a fence supporting hw->hw signaling that one or more devices which do not support hw signaling are waiting (and therefore it should enable an irq or do whatever is necessary in order that the CPU is notified when the fence is passed).
v3: Fix locking fail in attach_fence() and get_fence()
v4: Remove tie-in w/ dma-buf.. after discussion w/ danvet and mlankorst
we decided that we need to be able to attach one fence to N dma-buf's, so using the list_head in dma-fence struct would be problematic.
v5: [ Maarten Lankhorst ] Updated for dma-bikeshed-fence and dma-buf-manager.
v6: [ Maarten Lankhorst ] I removed dma_fence_cancel_callback and some comments
about checking if fence fired or not. This is broken by design. waitqueue_active during destruction is now fatal, since the signaller should be holding a reference in enable_signalling until it signalled the fence. Pass the original dma_fence_cb along, and call __remove_wait in the dma_fence_callback handler, so that no cleanup needs to be performed.
v7: [ Maarten Lankhorst ] Set cb->func and only enable sw signaling if
fence wasn't signaled yet, for example for hardware fences that may choose to signal blindly.
v8: [ Maarten Lankhorst ] Tons of tiny fixes, moved __dma_fence_init to
header and fixed include mess. dma-fence.h now includes dma-buf.h All members are now initialized, so kmalloc can be used for allocating a dma-fence. More documentation added.
v9: Change compiler bitfields to flags, change return type of
enable_signaling to bool. Rework dma_fence_wait. Added dma_fence_is_signaled and dma_fence_wait_timeout.
s/dma// and change exports to non GPL. Added fence_is_signaled and fence_enable_sw_signaling calls, add ability to override default wait operation.
v10: remove event_queue, use a custom list, export try_to_wake_up from
scheduler. Remove fence lock and use a global spinlock instead, this should hopefully remove all the locking headaches I was having on trying to implement this. enable_signaling is called with this lock held.
v11:
Use atomic ops for flags, lifting the need for some spin_lock_irqsaves. However I kept the guarantee that after fence_signal returns, it is guaranteed that enable_signaling has either been called to completion, or will not be called any more.

Add contexts and seqno to base fence implementation. This allows you to wait for less fences, by testing for seqno + signaled, and then only wait on the later fence.

Add FENCE_TRACE, FENCE_WARN, and FENCE_ERR. This makes debugging easier. An CONFIG_DEBUG_FENCE will be added to turn off the FENCE_TRACE spam, and another runtime option can turn it off at runtime.
v12:

    Add CONFIG_FENCE_TRACE. Add missing documentation for the fence->context
    and fence->seqno members.

v13:
Fixup CONFIG_FENCE_TRACE kconfig description. Move fence_context_alloc to fence. Simplify fence_later.
Kill priv member to fence_cb.
v14:
Remove priv argument from fence_add_callback, oops!

---
 Documentation/DocBook/device-drivers.tmpl |    2 
 drivers/base/Kconfig                      |   10 +
 drivers/base/Makefile                     |    2 
 drivers/base/fence.c                      |  311 ++++++++++++++++++++++++++
 include/linux/fence.h                     |  344 +++++++++++++++++++++++++++++
 5 files changed, 668 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/fence.c
 create mode 100644 include/linux/fence.h
Documentation/DocBook/device-drivers.tmpl +2 -0
index fe397f9..95d0db9 100644
@@ -126,6 +126,8 @@ X!Edrivers/base/interface.c
      </sect1>
      <sect1><title>Device Drivers DMA Management</title>
 !Edrivers/base/dma-buf.c
+!Edrivers/base/fence.c
+!Iinclude/linux/fence.h
 !Edrivers/base/reservation.c
 !Iinclude/linux/reservation.h
 !Edrivers/base/dma-coherent.c
drivers/base/Kconfig +10 -0
index 5daa259..2bf0add 100644
@@ -200,6 +200,16 @@ config DMA_SHARED_BUFFER
 	  APIs extension; the file's descriptor can then be passed on to other
 	  driver.
 
+config FENCE_TRACE
+	bool "Enable verbose FENCE_TRACE messages"
+	default n
+	depends on DMA_SHARED_BUFFER
+	help
+	  Enable the FENCE_TRACE printks. This will add extra
+	  spam to the console log, but will make it easier to diagnose
+	  lockup related problems for dma-buffers shared across multiple
+	  devices.
+
 config CMA
 	bool "Contiguous Memory Allocator"
 	depends on HAVE_DMA_CONTIGUOUS && HAVE_MEMBLOCK
drivers/base/Makefile +1 -1
index 48029aa..8a55cb9 100644
@@ -10,7 +10,7 @@ obj-$(CONFIG_CMA) += dma-contiguous.o
 obj-y			+= power/
 obj-$(CONFIG_HAS_DMA)	+= dma-mapping.o
 obj-$(CONFIG_HAVE_GENERIC_DMA_COHERENT) += dma-coherent.o
-obj-$(CONFIG_DMA_SHARED_BUFFER) += dma-buf.o reservation.o
+obj-$(CONFIG_DMA_SHARED_BUFFER) += dma-buf.o fence.o reservation.o
 obj-$(CONFIG_ISA)	+= isa.o
 obj-$(CONFIG_FW_LOADER)	+= firmware_class.o
 obj-$(CONFIG_NUMA)	+= node.o
drivers/base/fence.c new file +311 -0

311 lines not on this page · raw message

include/linux/fence.h new file +344 -0

344 lines not on this page · raw message

#1 lore raw

RFC PATCH 1/1 drm/nouveau: rework to new fence interface

nouveau was a bit tricky, it has no support for interrupts on <nv84, so I added an extra call to nouveau_fence_update in nouveau_fence_emit to increase the chance slightly that deferred work gets triggered.

This patch depends on the vblank locking fix for the definitions of nouveau_event_enable_locked and nouveau_event_disable_locked.

---

drivers/gpu/drm/nouveau/nouveau_fence.c +212 -141
index be31499..78714e4 100644
@@ -35,88 +35,115 @@
 
 #include <engine/fifo.h>
 
-struct fence_work {
-	struct work_struct base;
-	struct list_head head;
-	void (*func)(void *);
-	void *data;
-};
+static const struct fence_ops nouveau_fence_ops_uevent;
+static const struct fence_ops nouveau_fence_ops_legacy;
 
 static void
 nouveau_fence_signal(struct nouveau_fence *fence)
 {
-	struct fence_work *work, *temp;
+	__fence_signal(&fence->base);
+	list_del(&fence->head);
 
-	list_for_each_entry_safe(work, temp, &fence->work, head) {
-		schedule_work(&work->base);
-		list_del(&work->head);
+	if (fence->base.ops == &nouveau_fence_ops_uevent &&
+	    fence->event.head.next) {
+		struct nouveau_event *event;
+
+		list_del(&fence->event.head);
+		fence->event.head.next = NULL;
+
+		event = container_of(fence->base.lock, typeof(*event), lock);
+		if (!--event->index[0].refs)
+			event->disable(event, 0);
 	}
 
-	fence->channel = NULL;
-	list_del(&fence->head);
+	fence_put(&fence->base);
 }
 
 void
 nouveau_fence_context_del(struct nouveau_fence_chan *fctx)
 {
 	struct nouveau_fence *fence, *fnext;
-	spin_lock(&fctx->lock);
-	list_for_each_entry_safe(fence, fnext, &fctx->pending, head) {
+
+	spin_lock_irq(fctx->lock);
+	list_for_each_entry_safe(fence, fnext, &fctx->pending, head)
 		nouveau_fence_signal(fence);
-	}
-	spin_unlock(&fctx->lock);
+	spin_unlock_irq(fctx->lock);
 }
 
 void
-nouveau_fence_context_new(struct nouveau_fence_chan *fctx)
+nouveau_fence_context_new(struct nouveau_channel *chan, struct nouveau_fence_chan *fctx)
 {
+	struct nouveau_fifo *pfifo = nouveau_fifo(chan->drm->device);
+
+	fctx->lock = &pfifo->uevent->lock;
 	INIT_LIST_HEAD(&fctx->flip);
 	INIT_LIST_HEAD(&fctx->pending);
-	spin_lock_init(&fctx->lock);
 }
 
+struct nouveau_fence_work {
+	struct work_struct work;
+	struct fence_cb cb;
+	void (*func)(void *);
+	void *data;
+};
+
 static void
 nouveau_fence_work_handler(struct work_struct *kwork)
 {
-	struct fence_work *work = container_of(kwork, typeof(*work), base);
+	struct nouveau_fence_work *work = container_of(kwork, typeof(*work), work);
 	work->func(work->data);
 	kfree(work);
 }
 
+static void nouveau_fence_work_cb(struct fence *fence, struct fence_cb *cb)
+{
+	struct nouveau_fence_work *work = container_of(cb, typeof(*work), cb);
+
+	schedule_work(&work->work);
+}
+
+/*
+ * In an ideal world, read would not assume the channel context is still alive.
+ * This function may be called from another device, running into free memory as a
+ * result. The drm node should still be there, so we can derive the index from
+ * the fence context.
+ */
+static bool nouveau_fence_is_signaled(struct fence *f)
+{
+	struct nouveau_fence *fence = container_of(f, struct nouveau_fence, base);
+	struct nouveau_channel *chan = fence->channel;
+	struct nouveau_fence_chan *fctx = chan->fence;
+
+	return (int)(fctx->read(chan) - fence->base.seqno) >= 0;
+}
+
 void
 nouveau_fence_work(struct nouveau_fence *fence,
 		   void (*func)(void *), void *data)
 {
-	struct nouveau_channel *chan = fence->channel;
-	struct nouveau_fence_chan *fctx;
-	struct fence_work *work = NULL;
+	struct nouveau_fence_work *work;
 
-	if (nouveau_fence_done(fence)) {
-		func(data);
-		return;
-	}
+	if (fence_is_signaled(&fence->base))
+		goto err;
 
-	fctx = chan->fence;
 	work = kmalloc(sizeof(*work), GFP_KERNEL);
 	if (!work) {
 		WARN_ON(nouveau_fence_wait(fence, false, false));
-		func(data);
-		return;
-	}
-
-	spin_lock(&fctx->lock);
-	if (!fence->channel) {
-		spin_unlock(&fctx->lock);
-		kfree(work);
-		func(data);
-		return;
+		goto err;
 	}
 
-	INIT_WORK(&work->base, nouveau_fence_work_handler);
+	INIT_WORK(&work->work, nouveau_fence_work_handler);
 	work->func = func;
 	work->data = data;
-	list_add(&work->head, &fence->work);
-	spin_unlock(&fctx->lock);
+
+	if (fence_add_callback(&fence->base, &work->cb, nouveau_fence_work_cb) < 0)
+		goto err_free;
+	return;
+
+err_free:
+	kfree(work);
+err:
+	func(data);
 }
 
 static void
@@ -125,33 +152,44 @@ nouveau_fence_update(struct nouveau_channel *chan)
 	struct nouveau_fence_chan *fctx = chan->fence;
 	struct nouveau_fence *fence, *fnext;
 
-	spin_lock(&fctx->lock);
+	u32 seq = fctx->read(chan);
+
 	list_for_each_entry_safe(fence, fnext, &fctx->pending, head) {
-		if (fctx->read(chan) < fence->sequence)
+		if ((int)(seq - fence->base.seqno) < 0)
 			break;
 
 		nouveau_fence_signal(fence);
-		nouveau_fence_unref(&fence);
 	}
-	spin_unlock(&fctx->lock);
 }
 
 int
 nouveau_fence_emit(struct nouveau_fence *fence, struct nouveau_channel *chan)
 {
 	struct nouveau_fence_chan *fctx = chan->fence;
+	struct nouveau_fifo *pfifo = nouveau_fifo(chan->drm->device);
+	struct nouveau_fifo_chan *fifo = (void*)chan->object;
+	struct nouveau_fence_priv *priv = (void*)chan->drm->fence;
 	int ret;
 
 	fence->channel  = chan;
 	fence->timeout  = jiffies + (15 * DRM_HZ);
-	fence->sequence = ++fctx->sequence;
+
+	if (priv->uevent)
+		__fence_init(&fence->base, &nouveau_fence_ops_uevent,
+			    &pfifo->uevent->lock,
+			    priv->context_base + fifo->chid, ++fctx->sequence);
+	else
+		__fence_init(&fence->base, &nouveau_fence_ops_legacy,
+			    &pfifo->uevent->lock,
+			    priv->context_base + fifo->chid, ++fctx->sequence);
 
 	ret = fctx->emit(fence);
 	if (!ret) {
-		kref_get(&fence->kref);
-		spin_lock(&fctx->lock);
+		fence_get(&fence->base);
+		spin_lock_irq(fctx->lock);
+		nouveau_fence_update(chan);
 		list_add_tail(&fence->head, &fctx->pending);
-		spin_unlock(&fctx->lock);
+		spin_unlock_irq(fctx->lock);
 	}
 
 	return ret;
@@ -160,107 +198,71 @@ nouveau_fence_emit(struct nouveau_fence *fence, struct nouveau_channel *chan)
 bool
 nouveau_fence_done(struct nouveau_fence *fence)
 {
-	if (fence->channel)
+	if (fence->base.ops == &nouveau_fence_ops_legacy ||
+	    fence->base.ops == &nouveau_fence_ops_uevent) {
+		struct nouveau_fence_chan *fctx;
+		unsigned long flags;
+
+		if (test_bit(FENCE_FLAG_SIGNALED_BIT, &fence->base.flags))
+			return true;
+
+		fctx = fence->channel->fence;
+		spin_lock_irqsave(fctx->lock, flags);
 		nouveau_fence_update(fence->channel);
-	return !fence->channel;
+		spin_unlock_irqrestore(fctx->lock, flags);
+	}
+	return fence_is_signaled(&fence->base);
 }
 
-struct nouveau_fence_uevent {
-	struct nouveau_eventh handler;
-	struct nouveau_fence_priv *priv;
-};
-
-static int
-nouveau_fence_wait_uevent_handler(struct nouveau_eventh *event, int index)
+static long
+nouveau_fence_wait_legacy(struct fence *f, bool intr, long wait)
 {
-	struct nouveau_fence_uevent *uevent =
-		container_of(event, struct nouveau_fence_uevent, handler);
-	wake_up_all(&uevent->priv->waiting);
-	return NVKM_EVENT_KEEP;
-}
+	struct nouveau_fence *fence = container_of(f, typeof(*fence), base);
+	unsigned long sleep_time = NSEC_PER_MSEC / 1000;
+	unsigned long t = jiffies, timeout = t + wait;
 
-static int
-nouveau_fence_wait_uevent(struct nouveau_fence *fence, bool intr)
+	while (!nouveau_fence_done(fence)) {
+		ktime_t kt;
 
-{
-	struct nouveau_channel *chan = fence->channel;
-	struct nouveau_fifo *pfifo = nouveau_fifo(chan->drm->device);
-	struct nouveau_fence_priv *priv = chan->drm->fence;
-	struct nouveau_fence_uevent uevent = {
-		.handler.func = nouveau_fence_wait_uevent_handler,
-		.priv = priv,
-	};
-	int ret = 0;
+		t = jiffies;
 
-	nouveau_event_get(pfifo->uevent, 0, &uevent.handler);
-
-	if (fence->timeout) {
-		unsigned long timeout = fence->timeout - jiffies;
-
-		if (time_before(jiffies, fence->timeout)) {
-			if (intr) {
-				ret = wait_event_interruptible_timeout(
-						priv->waiting,
-						nouveau_fence_done(fence),
-						timeout);
-			} else {
-				ret = wait_event_timeout(priv->waiting,
-						nouveau_fence_done(fence),
-						timeout);
-			}
+		if (wait != MAX_SCHEDULE_TIMEOUT && time_after_eq(t, timeout)) {
+			__set_current_state(TASK_RUNNING);
+			return 0;
 		}
 
-		if (ret >= 0) {
-			fence->timeout = jiffies + ret;
-			if (time_after_eq(jiffies, fence->timeout))
-				ret = -EBUSY;
-		}
-	} else {
-		if (intr) {
-			ret = wait_event_interruptible(priv->waiting,
-					nouveau_fence_done(fence));
-		} else {
-			wait_event(priv->waiting, nouveau_fence_done(fence));
-		}
+		__set_current_state(intr ? TASK_INTERRUPTIBLE :
+					   TASK_UNINTERRUPTIBLE);
+
+		kt = ktime_set(0, sleep_time);
+		schedule_hrtimeout(&kt, HRTIMER_MODE_REL);
+		sleep_time *= 2;
+		if (sleep_time > NSEC_PER_MSEC)
+			sleep_time = NSEC_PER_MSEC;
+
+		if (intr && signal_pending(current))
+			return -ERESTARTSYS;
 	}
 
-	nouveau_event_put(pfifo->uevent, 0, &uevent.handler);
-	if (unlikely(ret < 0))
-		return ret;
+	__set_current_state(TASK_RUNNING);
 
-	return 0;
+	return timeout - t;
 }
 
-int
-nouveau_fence_wait(struct nouveau_fence *fence, bool lazy, bool intr)
+static int
+nouveau_fence_wait_busy(struct nouveau_fence *fence, bool intr)
 {
-	struct nouveau_channel *chan = fence->channel;
-	struct nouveau_fence_priv *priv = chan ? chan->drm->fence : NULL;
-	unsigned long sleep_time = NSEC_PER_MSEC / 1000;
-	ktime_t t;
 	int ret = 0;
 
-	while (priv && priv->uevent && lazy && !nouveau_fence_done(fence)) {
-		ret = nouveau_fence_wait_uevent(fence, intr);
-		if (ret < 0)
-			return ret;
-	}
-
 	while (!nouveau_fence_done(fence)) {
-		if (fence->timeout && time_after_eq(jiffies, fence->timeout)) {
+		if (time_after_eq(jiffies, fence->timeout)) {
 			ret = -EBUSY;
 			break;
 		}
 
-		__set_current_state(intr ? TASK_INTERRUPTIBLE :
-					   TASK_UNINTERRUPTIBLE);
-		if (lazy) {
-			t = ktime_set(0, sleep_time);
-			schedule_hrtimeout(&t, HRTIMER_MODE_REL);
-			sleep_time *= 2;
-			if (sleep_time > NSEC_PER_MSEC)
-				sleep_time = NSEC_PER_MSEC;
-		}
+		__set_current_state(intr ?
+				    TASK_INTERRUPTIBLE :
+				    TASK_UNINTERRUPTIBLE);
 
 		if (intr && signal_pending(current)) {
 			ret = -ERESTARTSYS;
@@ -273,14 +275,31 @@ nouveau_fence_wait(struct nouveau_fence *fence, bool lazy, bool intr)
 }
 
 int
+nouveau_fence_wait(struct nouveau_fence *fence, bool lazy, bool intr)
+{
+	long ret;
+
+	if (!lazy)
+		return nouveau_fence_wait_busy(fence, intr);
+
+	ret = fence_wait_timeout(&fence->base, intr, 15 * DRM_HZ);
+	if (ret < 0)
+		return ret;
+	else if (!ret)
+		return -EBUSY;
+	else
+		return 0;
+}
+
+int
 nouveau_fence_sync(struct nouveau_fence *fence, struct nouveau_channel *chan)
 {
 	struct nouveau_fence_chan *fctx = chan->fence;
-	struct nouveau_channel *prev;
 	int ret = 0;
 
-	prev = fence ? fence->channel : NULL;
-	if (prev) {
+	if (fence) {
+		struct nouveau_channel *prev = fence->channel;
+
 		if (unlikely(prev != chan && !nouveau_fence_done(fence))) {
 			ret = fctx->sync(fence, prev, chan);
 			if (unlikely(ret))
@@ -291,25 +310,18 @@ nouveau_fence_sync(struct nouveau_fence *fence, struct nouveau_channel *chan)
 	return ret;
 }
 
-static void
-nouveau_fence_del(struct kref *kref)
-{
-	struct nouveau_fence *fence = container_of(kref, typeof(*fence), kref);
-	kfree(fence);
-}
-
 void
 nouveau_fence_unref(struct nouveau_fence **pfence)
 {
 	if (*pfence)
-		kref_put(&(*pfence)->kref, nouveau_fence_del);
+		fence_put(&(*pfence)->base);
 	*pfence = NULL;
 }
 
 struct nouveau_fence *
 nouveau_fence_ref(struct nouveau_fence *fence)
 {
-	kref_get(&fence->kref);
+	fence_get(&fence->base);
 	return fence;
 }
 
@@ -327,9 +339,7 @@ nouveau_fence_new(struct nouveau_channel *chan, bool sysmem,
 	if (!fence)
 		return -ENOMEM;
 
-	INIT_LIST_HEAD(&fence->work);
 	fence->sysmem = sysmem;
-	kref_init(&fence->kref);
 
 	ret = nouveau_fence_emit(fence, chan);
 	if (ret)
@@ -338,3 +348,64 @@ nouveau_fence_new(struct nouveau_channel *chan, bool sysmem,
 	*pfence = fence;
 	return ret;
 }
+
+
+static bool nouveau_fence_no_signaling(struct fence *f)
+{
+	/*
+	 * This needs uevents to work correctly, but fence_add_callback relies on
+	 * being able to enable signaling. It will still get signaled eventually,
+	 * just not right away.
+	 */
+	if (nouveau_fence_is_signaled(f))
+		return false;
+
+	return true;
+}
+
+static const struct fence_ops nouveau_fence_ops_legacy = {
+	.enable_signaling = nouveau_fence_no_signaling,
+	.signaled = nouveau_fence_is_signaled,
+	.wait = nouveau_fence_wait_legacy,
+	.release = NULL
+};
+
+static int
+nouveau_fence_wait_uevent_handler(struct nouveau_eventh *event, int index)
+{
+	struct nouveau_fence *fence =
+		container_of(event, struct nouveau_fence, event);
+
+	if (nouveau_fence_is_signaled(&fence->base))
+		nouveau_fence_signal(fence);
+
+	/*
+	 * NVKM_EVENT_DROP is never appropriate here, nouveau_fence_signal
+	 * will unlink and free the event if needed.
+	 */
+	return NVKM_EVENT_KEEP;
+}
+
+static bool nouveau_fence_enable_signaling(struct fence *f)
+{
+	struct nouveau_fence *fence = container_of(f, struct nouveau_fence, base);
+	struct nouveau_event *event = container_of(f->lock, struct nouveau_event, lock);
+
+	nouveau_event_enable_locked(event, 0);
+	if (nouveau_fence_is_signaled(f)) {
+		nouveau_event_disable_locked(event, 0, 1);
+		return false;
+	}
+
+	list_add_tail(&fence->event.head, &event->index[0].list);
+	fence->event.func = nouveau_fence_wait_uevent_handler;
+
+	return true;
+}
+
+static const struct fence_ops nouveau_fence_ops_uevent = {
+	.enable_signaling = nouveau_fence_enable_signaling,
+	.signaled = nouveau_fence_is_signaled,
+	.wait = fence_default_wait,
+	.release = NULL
+};
drivers/gpu/drm/nouveau/nouveau_fence.h +10 -6
index c57bb61..1df933c 100644
@@ -1,18 +1,20 @@
 #ifndef __NOUVEAU_FENCE_H__
 #define __NOUVEAU_FENCE_H__
 
+#include <linux/fence.h>
+
 struct nouveau_drm;
 
 struct nouveau_fence {
+	struct fence base;
+
 	struct list_head head;
-	struct list_head work;
-	struct kref kref;
+	struct nouveau_eventh event;
 
 	bool sysmem;
 
 	struct nouveau_channel *channel;
 	unsigned long timeout;
-	u32 sequence;
 };
 
 int  nouveau_fence_new(struct nouveau_channel *, bool sysmem,
@@ -38,8 +40,9 @@ struct nouveau_fence_chan {
 	int  (*emit32)(struct nouveau_channel *, u64, u32);
 	int  (*sync32)(struct nouveau_channel *, u64, u32);
 
-	spinlock_t lock;
+	spinlock_t *lock;
 	u32 sequence;
+	u32 context;
 };
 
 struct nouveau_fence_priv {
@@ -49,13 +52,14 @@ struct nouveau_fence_priv {
 	int  (*context_new)(struct nouveau_channel *);
 	void (*context_del)(struct nouveau_channel *);
 
-	wait_queue_head_t waiting;
 	bool uevent;
+
+	u32 contexts, context_base;
 };
 
 #define nouveau_fence(drm) ((struct nouveau_fence_priv *)(drm)->fence)
 
-void nouveau_fence_context_new(struct nouveau_fence_chan *);
+void nouveau_fence_context_new(struct nouveau_channel *, struct nouveau_fence_chan *);
 void nouveau_fence_context_del(struct nouveau_fence_chan *);
 
 int nv04_fence_create(struct nouveau_drm *);
drivers/gpu/drm/nouveau/nv04_fence.c +2 -2
index 94eadd1..997c541 100644
@@ -43,7 +43,7 @@ nv04_fence_emit(struct nouveau_fence *fence)
 	int ret = RING_SPACE(chan, 2);
 	if (ret == 0) {
 		BEGIN_NV04(chan, NvSubSw, 0x0150, 1);
-		OUT_RING  (chan, fence->sequence);
+		OUT_RING  (chan, fence->base.seqno);
 		FIRE_RING (chan);
 	}
 	return ret;
@@ -77,7 +77,7 @@ nv04_fence_context_new(struct nouveau_channel *chan)
 {
 	struct nv04_fence_chan *fctx = kzalloc(sizeof(*fctx), GFP_KERNEL);
 	if (fctx) {
-		nouveau_fence_context_new(&fctx->base);
+		nouveau_fence_context_new(chan, &fctx->base);
 		fctx->base.emit = nv04_fence_emit;
 		fctx->base.sync = nv04_fence_sync;
 		fctx->base.read = nv04_fence_read;
drivers/gpu/drm/nouveau/nv10_fence.c +2 -2
index 06f434f..e8f73f7 100644
@@ -36,7 +36,7 @@ nv10_fence_emit(struct nouveau_fence *fence)
 	int ret = RING_SPACE(chan, 2);
 	if (ret == 0) {
 		BEGIN_NV04(chan, 0, NV10_SUBCHAN_REF_CNT, 1);
-		OUT_RING  (chan, fence->sequence);
+		OUT_RING  (chan, fence->base.seqno);
 		FIRE_RING (chan);
 	}
 	return ret;
@@ -74,7 +74,7 @@ nv10_fence_context_new(struct nouveau_channel *chan)
 	if (!fctx)
 		return -ENOMEM;
 
-	nouveau_fence_context_new(&fctx->base);
+	nouveau_fence_context_new(chan, &fctx->base);
 	fctx->base.emit = nv10_fence_emit;
 	fctx->base.read = nv10_fence_read;
 	fctx->base.sync = nv10_fence_sync;
drivers/gpu/drm/nouveau/nv17_fence.c +1 -1
index 22aa996..e404bab 100644
@@ -83,7 +83,7 @@ nv17_fence_context_new(struct nouveau_channel *chan)
 	if (!fctx)
 		return -ENOMEM;
 
-	nouveau_fence_context_new(&fctx->base);
+	nouveau_fence_context_new(chan, &fctx->base);
 	fctx->base.emit = nv10_fence_emit;
 	fctx->base.read = nv10_fence_read;
 	fctx->base.sync = nv17_fence_sync;
drivers/gpu/drm/nouveau/nv50_fence.c +1 -1
index 0ee3638..19f6fcc 100644
@@ -47,7 +47,7 @@ nv50_fence_context_new(struct nouveau_channel *chan)
 	if (!fctx)
 		return -ENOMEM;
 
-	nouveau_fence_context_new(&fctx->base);
+	nouveau_fence_context_new(chan, &fctx->base);
 	fctx->base.emit = nv10_fence_emit;
 	fctx->base.read = nv10_fence_read;
 	fctx->base.sync = nv17_fence_sync;
drivers/gpu/drm/nouveau/nv84_fence.c +7 -4
index 9fd475c..8a06727 100644
@@ -89,7 +89,7 @@ nv84_fence_emit(struct nouveau_fence *fence)
 	else
 		addr += fctx->vma.offset;
 
-	return fctx->base.emit32(chan, addr, fence->sequence);
+	return fctx->base.emit32(chan, addr, fence->base.seqno);
 }
 
 static int
@@ -105,7 +105,7 @@ nv84_fence_sync(struct nouveau_fence *fence,
 	else
 		addr += fctx->vma.offset;
 
-	return fctx->base.sync32(chan, addr, fence->sequence);
+	return fctx->base.sync32(chan, addr, fence->base.seqno);
 }
 
 static u32
@@ -149,12 +149,14 @@ nv84_fence_context_new(struct nouveau_channel *chan)
 	if (!fctx)
 		return -ENOMEM;
 
-	nouveau_fence_context_new(&fctx->base);
+	nouveau_fence_context_new(chan, &fctx->base);
 	fctx->base.emit = nv84_fence_emit;
 	fctx->base.sync = nv84_fence_sync;
 	fctx->base.read = nv84_fence_read;
 	fctx->base.emit32 = nv84_fence_emit32;
 	fctx->base.sync32 = nv84_fence_sync32;
+	fctx->base.sequence = nv84_fence_read(chan);
+	fctx->base.context = priv->base.context_base + fifo->chid;
 
 	ret = nouveau_bo_vma_add(priv->bo, client->vm, &fctx->vma);
 	if (ret == 0) {
@@ -239,7 +241,8 @@ nv84_fence_create(struct nouveau_drm *drm)
 	priv->base.context_new = nv84_fence_context_new;
 	priv->base.context_del = nv84_fence_context_del;
 
-	init_waitqueue_head(&priv->base.waiting);
+	priv->base.contexts = pfifo->max + 1;
+	priv->base.context_base = fence_context_alloc(priv->base.contexts);
 	priv->base.uevent = true;
 
 	ret = nouveau_bo_new(drm->dev, 16 * (pfifo->max + 1), 0,
#2 lore raw

RFC PATCH 1/1 drm/radeon: rework to new fence interface

In reply to Maarten Lankhorst

---

drivers/gpu/drm/radeon/radeon.h +6 -9
index 9f19259..971284e 100644
@@ -64,6 +64,7 @@
 #include <linux/wait.h>
 #include <linux/list.h>
 #include <linux/kref.h>
+#include <linux/fence.h>
 
 #include <ttm/ttm_bo_api.h>
 #include <ttm/ttm_bo_driver.h>
@@ -114,9 +115,6 @@ extern int radeon_aspm;
 /* max number of rings */
 #define RADEON_NUM_RINGS			6
 
-/* fence seq are set to this number when signaled */
-#define RADEON_FENCE_SIGNALED_SEQ		0LL
-
 /* internal ring indices */
 /* r1xx+ has gfx CP ring */
 #define RADEON_RING_TYPE_GFX_INDEX	0
@@ -285,12 +283,15 @@ struct radeon_fence_driver {
 };
 
 struct radeon_fence {
+	struct fence base;
+
 	struct radeon_device		*rdev;
-	struct kref			kref;
 	/* protected by radeon_fence.lock */
 	uint64_t			seq;
 	/* RB, DMA, etc. */
 	unsigned			ring;
+
+	wait_queue_t fence_wake;
 };
 
 int radeon_fence_driver_start_ring(struct radeon_device *rdev, int ring);
@@ -2039,6 +2040,7 @@ struct radeon_device {
 	struct radeon_mman		mman;
 	struct radeon_fence_driver	fence_drv[RADEON_NUM_RINGS];
 	wait_queue_head_t		fence_queue;
+	unsigned			fence_context;
 	struct mutex			ring_lock;
 	struct radeon_ring		ring[RADEON_NUM_RINGS];
 	bool				ib_pool_ready;
@@ -2117,11 +2119,6 @@ u32 cik_mm_rdoorbell(struct radeon_device *rdev, u32 offset);
 void cik_mm_wdoorbell(struct radeon_device *rdev, u32 offset, u32 v);
 
 /*
- * Cast helper
- */
-#define to_radeon_fence(p) ((struct radeon_fence *)(p))
-
-/*
  * Registers read & write functions.
  */
 #define RREG8(reg) readb((rdev->rmmio) + (reg))
drivers/gpu/drm/radeon/radeon_device.c +1 -0
index 63398ae..d76a187 100644
@@ -1150,6 +1150,7 @@ int radeon_device_init(struct radeon_device *rdev,
 	for (i = 0; i < RADEON_NUM_RINGS; i++) {
 		rdev->ring[i].idx = i;
 	}
+	rdev->fence_context = fence_context_alloc(RADEON_NUM_RINGS);
 
 	DRM_INFO("initializing kernel modesetting (%s 0x%04X:0x%04X 0x%04X:0x%04X).\n",
 		radeon_family_name[rdev->family], pdev->vendor, pdev->device,
drivers/gpu/drm/radeon/radeon_fence.c +121 -36

249 lines not on this page · raw message

#3 lore raw

Re: [RFC PATCH] drm/radeon: rework to new fence interface

Am 19.08.2013 12:17, schrieb Maarten Lankhorst:

drivers/gpu/drm/radeon/radeon_fence.c void radeon_fence_process(struct radeon_device *rdev, int ring) · line 231

Quoted 12 lines

[SNIP]

drivers/gpu/drm/radeon/radeon_fence.c +2 -3
@@ -190,25 +225,24 @@ void radeon_fence_process(struct radeon_device *rdev, int ring)
  		}
  	} while (atomic64_xchg(&rdev->fence_drv[ring].last_seq, seq) > seq);
 
-	if (wake) {
+	if (wake)
  		rdev->fence_drv[ring].last_activity = jiffies;
-		wake_up_all(&rdev->fence_queue);
-	}
+	return wake;
  }

Very bad idea, when sequence numbers change, you always want to wake up the whole fence queue here.

Quoted 7 lines

[SNIP]

drivers/gpu/drm/radeon/radeon_fence.c +6 -0
+/**
+ * radeon_fence_enable_signaling - enable signalling on fence
+ * @fence: fence
+ *
+ * This function is called with fence_queue lock held, and adds a callback
+ * to fence_queue that checks if this fence is signaled, and if so it

drivers/gpu/drm/radeon/radeon_fence.c static bool radeon_fence_seq_signaled(struct radeon_device *rdev, · line 298

Quoted 10 lines
drivers/gpu/drm/radeon/radeon_fence.c +10 -0
+ * signals the fence and removes itself.
+ */
+static bool radeon_fence_enable_signaling(struct fence *f)
+{
+	struct radeon_fence *fence = to_radeon_fence(f);
+
+	if (atomic64_read(&fence->rdev->fence_drv[fence->ring].last_seq) >= fence->seq ||
+	    !fence->rdev->ddev->irq_enabled)
+		return false;
+

Do I get that right that you rely on IRQs to be enabled and working here? Cause that would be a quite bad idea from the conceptual side.

Quoted 14 lines
drivers/gpu/drm/radeon/radeon_fence.c +13 -0
+	radeon_irq_kms_sw_irq_get(fence->rdev, fence->ring);
+
+	if (__radeon_fence_process(fence->rdev, fence->ring))
+		wake_up_all_locked(&fence->rdev->fence_queue);
+
+	/* did fence get signaled after we enabled the sw irq? */
+	if (atomic64_read(&fence->rdev->fence_drv[fence->ring].last_seq) >= fence->seq) {
+		radeon_irq_kms_sw_irq_put(fence->rdev, fence->ring);
+		return false;
+	}
+
+	fence->fence_wake.flags = 0;
+	fence->fence_wake.private = NULL;

drivers/gpu/drm/radeon/radeon_fence.c static bool radeon_fence_seq_signaled(struct radeon_device *rdev, · line 321

Quoted 10 lines
drivers/gpu/drm/radeon/radeon_fence.c +7 -0
+	fence->fence_wake.func = radeon_fence_check_signaled;
+	__add_wait_queue(&fence->rdev->fence_queue, &fence->fence_wake);
+	fence_get(f);
+
+	return true;
+}
+
  /**
   * radeon_fence_signaled - check if a fence has signaled
   *

Christian.

#4 lore raw

Re: [RFC PATCH] drm/radeon: rework to new fence interface

Op 19-08-13 14:35, Christian König schreef:

Quoted 14 lines

Am 19.08.2013 12:17, schrieb Maarten Lankhorst:

drivers/gpu/drm/radeon/radeon_fence.c void radeon_fence_process(struct radeon_device *rdev, int ring) · line 231

Quoted 11 lines

[SNIP]

drivers/gpu/drm/radeon/radeon_fence.c +2 -2
@@ -190,25 +225,24 @@ void radeon_fence_process(struct radeon_device *rdev, int ring)
          }
      } while (atomic64_xchg(&rdev->fence_drv[ring].last_seq, seq) > seq);
  -    if (wake) {
+    if (wake)
          rdev->fence_drv[ring].last_activity = jiffies;
-        wake_up_all(&rdev->fence_queue);
-    }
+    return wake;
  }

Very bad idea, when sequence numbers change, you always want to wake up the whole fence queue here.

Yes, and the callers of this function call wake_up_all or wake_up_all_locked themselves, based on the return value..

Quoted 11 lines
Quoted 11 lines

[SNIP]

drivers/gpu/drm/radeon/radeon_fence.c +10 -0
+/**
+ * radeon_fence_enable_signaling - enable signalling on fence
+ * @fence: fence
+ *
+ * This function is called with fence_queue lock held, and adds a callback
+ * to fence_queue that checks if this fence is signaled, and if so it
+ * signals the fence and removes itself.
+ */
+static bool radeon_fence_enable_signaling(struct fence *f)
+{
Quoted 8 lines

drivers/gpu/drm/radeon/radeon_fence.c static bool radeon_fence_seq_signaled(struct radeon_device *rdev, · line 298

Quoted 6 lines
drivers/gpu/drm/radeon/radeon_fence.c +6 -0
+    struct radeon_fence *fence = to_radeon_fence(f);
+
+    if (atomic64_read(&fence->rdev->fence_drv[fence->ring].last_seq) >= fence->seq ||
+        !fence->rdev->ddev->irq_enabled)
+        return false;
+

Do I get that right that you rely on IRQs to be enabled and working here? Cause that would be a quite bad idea from the conceptual side.

For cross-device synchronization it would be nice to have working irqs, it allows signalling fences faster, and it allows for callbacks on completion to be called. For internal usage it's no more required than it was before.

Quoted 18 lines
Quoted 18 lines
drivers/gpu/drm/radeon/radeon_fence.c +18 -0
+    radeon_irq_kms_sw_irq_get(fence->rdev, fence->ring);
+
+    if (__radeon_fence_process(fence->rdev, fence->ring))
+        wake_up_all_locked(&fence->rdev->fence_queue);
+
+    /* did fence get signaled after we enabled the sw irq? */
+    if (atomic64_read(&fence->rdev->fence_drv[fence->ring].last_seq) >= fence->seq) {
+        radeon_irq_kms_sw_irq_put(fence->rdev, fence->ring);
+        return false;
+    }
+
+    fence->fence_wake.flags = 0;
+    fence->fence_wake.private = NULL;
+    fence->fence_wake.func = radeon_fence_check_signaled;
+    __add_wait_queue(&fence->rdev->fence_queue, &fence->fence_wake);
+    fence_get(f);
+
+    return true;
Quoted 9 lines

drivers/gpu/drm/radeon/radeon_fence.c static bool radeon_fence_seq_signaled(struct radeon_device *rdev, · line 321

Quoted 6 lines
drivers/gpu/drm/radeon/radeon_fence.c +2 -0
+}
+
  /**
   * radeon_fence_signaled - check if a fence has signaled
   *

Christian.

~Maarten

#5 lore raw

Re: [RFC PATCH] drm/radeon: rework to new fence interface

Am 19.08.2013 21:37, schrieb Maarten Lankhorst:

Quoted 14 lines

Op 19-08-13 14:35, Christian König schreef:

Quoted 13 lines

Am 19.08.2013 12:17, schrieb Maarten Lankhorst:

drivers/gpu/drm/radeon/radeon_fence.c void radeon_fence_process(struct radeon_device *rdev, int ring) · line 231

Quoted 11 lines

[SNIP]

drivers/gpu/drm/radeon/radeon_fence.c +2 -2
@@ -190,25 +225,24 @@ void radeon_fence_process(struct radeon_device *rdev, int ring)
           }
       } while (atomic64_xchg(&rdev->fence_drv[ring].last_seq, seq) > seq);
   -    if (wake) {
+    if (wake)
           rdev->fence_drv[ring].last_activity = jiffies;
-        wake_up_all(&rdev->fence_queue);
-    }
+    return wake;
   }

Very bad idea, when sequence numbers change, you always want to wake up the whole fence queue here.

Quoted 1 line

Yes, and the callers of this function call wake_up_all or wake_up_all_locked themselves, based on the return value..

And as I said that's a very bad idea. The fence processing shouldn't be called with any locks held and should be self responsible for activating any waiters.

Quoted 19 lines
Quoted 11 lines
Quoted 11 lines

[SNIP]

drivers/gpu/drm/radeon/radeon_fence.c +9 -0
+/**
+ * radeon_fence_enable_signaling - enable signalling on fence
+ * @fence: fence
+ *
+ * This function is called with fence_queue lock held, and adds a callback
+ * to fence_queue that checks if this fence is signaled, and if so it
+ * signals the fence and removes itself.
+ */
+static bool radeon_fence_enable_signaling(struct fence *f)
Quoted 8 lines

drivers/gpu/drm/radeon/radeon_fence.c static bool radeon_fence_seq_signaled(struct radeon_device *rdev, · line 298

Quoted 7 lines
drivers/gpu/drm/radeon/radeon_fence.c +7 -0
+{
+    struct radeon_fence *fence = to_radeon_fence(f);
+
+    if (atomic64_read(&fence->rdev->fence_drv[fence->ring].last_seq) >= fence->seq ||
+        !fence->rdev->ddev->irq_enabled)
+        return false;
+

Do I get that right that you rely on IRQs to be enabled and working here? Cause that would be a quite bad idea from the conceptual side.

Quoted 2 lines

For cross-device synchronization it would be nice to have working irqs, it allows signalling fences faster, and it allows for callbacks on completion to be called. For internal usage it's no more required than it was before.

That's a big NAK.

The fence processing is actually very fine tuned to avoid IRQs and as far as I can see you just leave them enabled by decrementing the atomic from IRQ context. Additional to that we need allot of special handling in case of a hardware lockup here, which isn't done if you abuse the fence interface like this.

Also your approach of leaking the IRQ context outside of the driver is a very bad idea from the conceptual side. Please don't modify the fence interface at all and instead use the wait functions already exposed by radeon_fence.c. If you need some kind of signaling mechanism then wait inside a workqueue instead.

Christian.

#6 lore raw

Re: [RFC PATCH] drm/radeon: rework to new fence interface

Op 20-08-13 10:37, Christian König schreef:

Quoted 15 lines

Am 19.08.2013 21:37, schrieb Maarten Lankhorst:

Quoted 14 lines

Op 19-08-13 14:35, Christian König schreef:

Quoted 13 lines

Am 19.08.2013 12:17, schrieb Maarten Lankhorst:

drivers/gpu/drm/radeon/radeon_fence.c void radeon_fence_process(struct radeon_device *rdev, int ring) · line 231

Quoted 11 lines

[SNIP]

drivers/gpu/drm/radeon/radeon_fence.c +2 -2
@@ -190,25 +225,24 @@ void radeon_fence_process(struct radeon_device *rdev, int ring)
           }
       } while (atomic64_xchg(&rdev->fence_drv[ring].last_seq, seq) > seq);
   -    if (wake) {
+    if (wake)
           rdev->fence_drv[ring].last_activity = jiffies;
-        wake_up_all(&rdev->fence_queue);
-    }
+    return wake;
   }

Very bad idea, when sequence numbers change, you always want to wake up the whole fence queue here.

Quoted 3 lines
Quoted 1 line

Yes, and the callers of this function call wake_up_all or wake_up_all_locked themselves, based on the return value..

And as I said that's a very bad idea. The fence processing shouldn't be called with any locks held and should be self responsible for activating any waiters.

The call point (enable_signaling) only needs to know whether its own counter has passed or not. This prevents the race where the counter has elapsed, but the irq was not yet enabled.

I don't really care if enable_signaling updates last_seq or not, it only needs to check if it's own fence has been signaled after enabling sw_irqs.

Quoted 19 lines
Quoted 19 lines
Quoted 19 lines
Quoted 8 lines

[SNIP]

drivers/gpu/drm/radeon/radeon_fence.c +6 -0
+/**
+ * radeon_fence_enable_signaling - enable signalling on fence
+ * @fence: fence
+ *
+ * This function is called with fence_queue lock held, and adds a callback
+ * to fence_queue that checks if this fence is signaled, and if so it

drivers/gpu/drm/radeon/radeon_fence.c static bool radeon_fence_seq_signaled(struct radeon_device *rdev, · line 298

Quoted 10 lines
drivers/gpu/drm/radeon/radeon_fence.c +10 -0
+ * signals the fence and removes itself.
+ */
+static bool radeon_fence_enable_signaling(struct fence *f)
+{
+    struct radeon_fence *fence = to_radeon_fence(f);
+
+    if (atomic64_read(&fence->rdev->fence_drv[fence->ring].last_seq) >= fence->seq ||
+        !fence->rdev->ddev->irq_enabled)
+        return false;
+

Do I get that right that you rely on IRQs to be enabled and working here? Cause that would be a quite bad idea from the conceptual side.

Quoted 6 lines
Quoted 2 lines

For cross-device synchronization it would be nice to have working irqs, it allows signalling fences faster, and it allows for callbacks on completion to be called. For internal usage it's no more required than it was before.

That's a big NAK.

The fence processing is actually very fine tuned to avoid IRQs and as far as I can see you just leave them enabled by decrementing the atomic from IRQ context. Additional to that we need allot of special handling in case of a hardware lockup here, which isn't done if you abuse the fence interface like this.

I think it's not needed to leave the irq enabled, it's a leftover from when I was debugging the mac and no interrupt occurred at all.

Quoted 1 line

Also your approach of leaking the IRQ context outside of the driver is a very bad idea from the conceptual side. Please don't modify the fence interface at all and instead use the wait functions already exposed by radeon_fence.c. If you need some kind of signaling mechanism then wait inside a workqueue instead.

The fence takes up the role of a single shot workqueue here. Manually resetting the counter and calling wake_up_all would end up waking all active fences, there's no special handling needed inside radeon for this.
The fence api does provide a synchronous wait function, but this causes a stall of whomever waits on it. When I was testing this with intel I used the fence callback to poke a register in i915, this allowed it to not block until it hits the wait op in the command stream, and even then only if the callback was not called first.

It's documented that the callbacks can be called from any context and will be called with irqs disabled, so nothing scary should be done. The kernel provides enough debug mechanisms to find any violators. PROVE_LOCKING and DEBUG_ATOMIC_SLEEP for example.

~Maarten

#7 lore raw

Re: [RFC PATCH] drm/radeon: rework to new fence interface

Am 20.08.2013 11:36, schrieb Maarten Lankhorst:
[SNIP]

Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
Quoted 10 lines
Quoted 10 lines

[SNIP]

drivers/gpu/drm/radeon/radeon_fence.c +9 -0
+/**
+ * radeon_fence_enable_signaling - enable signalling on fence
+ * @fence: fence
+ *
+ * This function is called with fence_queue lock held, and adds a callback
+ * to fence_queue that checks if this fence is signaled, and if so it
+ * signals the fence and removes itself.
+ */
+static bool radeon_fence_enable_signaling(struct fence *f)
Quoted 8 lines

drivers/gpu/drm/radeon/radeon_fence.c static bool radeon_fence_seq_signaled(struct radeon_device *rdev, · line 298

Quoted 7 lines
drivers/gpu/drm/radeon/radeon_fence.c +7 -0
+{
+    struct radeon_fence *fence = to_radeon_fence(f);
+
+    if (atomic64_read(&fence->rdev->fence_drv[fence->ring].last_seq) >= fence->seq ||
+        !fence->rdev->ddev->irq_enabled)
+        return false;
+

Do I get that right that you rely on IRQs to be enabled and working here? Cause that would be a quite bad idea from the conceptual side.

For cross-device synchronization it would be nice to have working irqs, it allows signalling fences faster, and it allows for callbacks on completion to be called. For internal usage it's no more required than it was before.

Quoted 7 lines
Quoted 3 lines

That's a big NAK.

The fence processing is actually very fine tuned to avoid IRQs and as far as I can see you just leave them enabled by decrementing the atomic from IRQ context. Additional to that we need allot of special handling in case of a hardware lockup here, which isn't done if you abuse the fence interface like this.

I think it's not needed to leave the irq enabled, it's a leftover from when I was debugging the mac and no interrupt occurred at all.

Quoted 1 line

Also your approach of leaking the IRQ context outside of the driver is a very bad idea from the conceptual side. Please don't modify the fence interface at all and instead use the wait functions already exposed by radeon_fence.c. If you need some kind of signaling mechanism then wait inside a workqueue instead.

The fence takes up the role of a single shot workqueue here. Manually resetting the counter and calling wake_up_all would end up waking all active fences, there's no special handling needed inside radeon for this.

Yeah that's actually the point here, you NEED to activate ALL fences, otherwise the fence handling inside the driver won't work.

Quoted 1 line

The fence api does provide a synchronous wait function, but this causes a stall of whomever waits on it.

Which is perfectly fine. What actually is the use case of not stalling a process who wants to wait for something?

Quoted 4 lines

When I was testing this with intel I used the fence callback to poke a register in i915, this allowed it to not block until it hits the wait op in the command stream, and even then only if the callback was not called first.

It's documented that the callbacks can be called from any context and will be called with irqs disabled, so nothing scary should be done. The kernel provides enough debug mechanisms to find any violators. PROVE_LOCKING and DEBUG_ATOMIC_SLEEP for example.

No thanks, we even abandoned that concept internal in the driver. Please use the blocking wait functions instead.

Christian.

#8 lore raw

Re: [RFC PATCH] drm/radeon: rework to new fence interface

Op 20-08-13 11:51, Christian König schreef:

Quoted 25 lines

Am 20.08.2013 11:36, schrieb Maarten Lankhorst:
[SNIP]

Quoted 22 lines
Quoted 22 lines
Quoted 18 lines
Quoted 10 lines
Quoted 10 lines

[SNIP]

drivers/gpu/drm/radeon/radeon_fence.c +9 -0
+/**
+ * radeon_fence_enable_signaling - enable signalling on fence
+ * @fence: fence
+ *
+ * This function is called with fence_queue lock held, and adds a callback
+ * to fence_queue that checks if this fence is signaled, and if so it
+ * signals the fence and removes itself.
+ */
+static bool radeon_fence_enable_signaling(struct fence *f)
Quoted 8 lines

drivers/gpu/drm/radeon/radeon_fence.c static bool radeon_fence_seq_signaled(struct radeon_device *rdev, · line 298

Quoted 7 lines
drivers/gpu/drm/radeon/radeon_fence.c +7 -0
+{
+    struct radeon_fence *fence = to_radeon_fence(f);
+
+    if (atomic64_read(&fence->rdev->fence_drv[fence->ring].last_seq) >= fence->seq ||
+        !fence->rdev->ddev->irq_enabled)
+        return false;
+

Do I get that right that you rely on IRQs to be enabled and working here? Cause that would be a quite bad idea from the conceptual side.

Quoted 2 lines

For cross-device synchronization it would be nice to have working irqs, it allows signalling fences faster, and it allows for callbacks on completion to be called. For internal usage it's no more required than it was before.

That's a big NAK.

Quoted 7 lines
Quoted 5 lines
Quoted 1 line

The fence processing is actually very fine tuned to avoid IRQs and as far as I can see you just leave them enabled by decrementing the atomic from IRQ context. Additional to that we need allot of special handling in case of a hardware lockup here, which isn't done if you abuse the fence interface like this.

I think it's not needed to leave the irq enabled, it's a leftover from when I was debugging the mac and no interrupt occurred at all.

Quoted 1 line

Also your approach of leaking the IRQ context outside of the driver is a very bad idea from the conceptual side. Please don't modify the fence interface at all and instead use the wait functions already exposed by radeon_fence.c. If you need some kind of signaling mechanism then wait inside a workqueue instead.

The fence takes up the role of a single shot workqueue here. Manually resetting the counter and calling wake_up_all would end up waking all active fences, there's no special handling needed inside radeon for this.

Yeah that's actually the point here, you NEED to activate ALL fences, otherwise the fence handling inside the driver won't work.

It's done in a lazy fashion. If there's no need for an activated fence the interrupt will not be enabled.

Quoted 3 lines
Quoted 1 line

The fence api does provide a synchronous wait function, but this causes a stall of whomever waits on it.

Which is perfectly fine. What actually is the use case of not stalling a process who wants to wait for something?

Does radeon call ttm_bo_wait on all bo's before doing a command submission? No? Why should other drivers do that..

Quoted 6 lines
Quoted 4 lines

When I was testing this with intel I used the fence callback to poke a register in i915, this allowed it to not block until it hits the wait op in the command stream, and even then only if the callback was not called first.

It's documented that the callbacks can be called from any context and will be called with irqs disabled, so nothing scary should be done. The kernel provides enough debug mechanisms to find any violators. PROVE_LOCKING and DEBUG_ATOMIC_SLEEP for example.

No thanks, we even abandoned that concept internal in the driver. Please use the blocking wait functions instead.

No, this just stalls all gpu's that share a bo.

The idea is to provide a standardized api so bo's can be synchronized without stalling. The first step to this is ww_mutex. If this lock is shared between multiple gpu's the same object can be reserved between multiple devices without causing a deadlock with circular dependencies. With some small patches it's possible to do this already between multiple drivers that use ttm. ttm_bo_reserve, ttm_bo_unreserve and all the other code dealing with ttm reservations have been converted to use ww_mutex locking.

Fencing is the next step. When all buffers are locked a callback should be added to any previous fence, and a single new fence signaling completion of the command submission should be placed on all locked objects. Because the common path is that no objects are shared, the callback and FIFO stalling will only be needed for dma-bufs. When all callbacks have fired the FIFO can be unblocked. This prevents having to sync the gpu to the cpu. If a bo is submitted to 1 gpu, and then immediately to another it will not stall unless needed. For example in a optimus configuration an application could copy a rendered frame from VRAM to a shared dma-buf (xorg's buffer), then have Xorg copying it again (on intel's gpu) from the dma-buf to a framebuffer .

~Maarten

#9 lore raw

Re: [RFC PATCH] drm/radeon: rework to new fence interface

Am 20.08.2013 15:21, schrieb Maarten Lankhorst:

Quoted 29 lines

Op 20-08-13 11:51, Christian König schreef:

Quoted 28 lines

Am 20.08.2013 11:36, schrieb Maarten Lankhorst:
[SNIP]

Quoted 25 lines
Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
Quoted 7 lines

[SNIP]

(quoted diff) +6 -0
+/**
+ * radeon_fence_enable_signaling - enable signalling on fence
+ * @fence: fence
+ *
+ * This function is called with fence_queue lock held, and adds a callback
+ * to fence_queue that checks if this fence is signaled, and if so it

Review

Quoted 10 lines
(quoted diff) +10 -0
+ * signals the fence and removes itself.
+ */
+static bool radeon_fence_enable_signaling(struct fence *f)
+{
+    struct radeon_fence *fence = to_radeon_fence(f);
+
+    if (atomic64_read(&fence->rdev->fence_drv[fence->ring].last_seq) >= fence->seq ||
+        !fence->rdev->ddev->irq_enabled)
+        return false;
+

Do I get that right that you rely on IRQs to be enabled and working here? Cause that would be a quite bad idea from the conceptual side.

Quoted 5 lines
Quoted 2 lines

For cross-device synchronization it would be nice to have working irqs, it allows signalling fences faster, and it allows for callbacks on completion to be called. For internal usage it's no more required than it was before.

That's a big NAK.

The fence processing is actually very fine tuned to avoid IRQs and as far as I can see you just leave them enabled by decrementing the atomic from IRQ context. Additional to that we need allot of special handling in case of a hardware lockup here, which isn't done if you abuse the fence interface like this.

I think it's not needed to leave the irq enabled, it's a leftover from when I was debugging the mac and no interrupt occurred at all.

Quoted 7 lines
Quoted 3 lines
Quoted 2 lines
Quoted 1 line

Also your approach of leaking the IRQ context outside of the driver is a very bad idea from the conceptual side. Please don't modify the fence interface at all and instead use the wait functions already exposed by radeon_fence.c. If you need some kind of signaling mechanism then wait inside a workqueue instead.

The fence takes up the role of a single shot workqueue here. Manually resetting the counter and calling wake_up_all would end up waking all active fences, there's no special handling needed inside radeon for this.

Yeah that's actually the point here, you NEED to activate ALL fences, otherwise the fence handling inside the driver won't work.

It's done in a lazy fashion. If there's no need for an activated fence the interrupt will not be enabled.

Quoted 2 lines
Quoted 1 line

The fence api does provide a synchronous wait function, but this causes a stall of whomever waits on it.

Which is perfectly fine. What actually is the use case of not stalling a process who wants to wait for something?

Does radeon call ttm_bo_wait on all bo's before doing a command submission? No? Why should other drivers do that..

Sure it does if hardware synchronization isn't supported.

Quoted 13 lines
Quoted 5 lines
Quoted 4 lines

When I was testing this with intel I used the fence callback to poke a register in i915, this allowed it to not block until it hits the wait op in the command stream, and even then only if the callback was not called first.

It's documented that the callbacks can be called from any context and will be called with irqs disabled, so nothing scary should be done. The kernel provides enough debug mechanisms to find any violators. PROVE_LOCKING and DEBUG_ATOMIC_SLEEP for example.

No thanks, we even abandoned that concept internal in the driver. Please use the blocking wait functions instead.

No, this just stalls all gpu's that share a bo.

The idea is to provide a standardized api so bo's can be synchronized without stalling. The first step to this is ww_mutex. If this lock is shared between multiple gpu's the same object can be reserved between multiple devices without causing a deadlock with circular dependencies. With some small patches it's possible to do this already between multiple drivers that use ttm. ttm_bo_reserve, ttm_bo_unreserve and all the other code dealing with ttm reservations have been converted to use ww_mutex locking.

Quoted 6 lines

Fencing is the next step. When all buffers are locked a callback should be added to any previous fence, and a single new fence signaling completion of the command submission should be placed on all locked objects. Because the common path is that no objects are shared, the callback and FIFO stalling will only be needed for dma-bufs. When all callbacks have fired the FIFO can be unblocked. This prevents having to sync the gpu to the cpu. If a bo is submitted to 1 gpu, and then immediately to another it will not stall unless needed. For example in a optimus configuration an application could copy a rendered frame from VRAM to a shared dma-buf (xorg's buffer), then have Xorg copying it again (on intel's gpu) from the dma-buf to a framebuffer .

Yeah, that's the same concept we used to have for multiple rings, to avoid stalling if a buffer is currently used in a different part of the GPU than the current command submission wants it to. After allot of internal discussion we came to the conclusion that it just doesn't worth the effort.

Have you thought over all the consequences that are implied by having a serialized stream of command submissions?

When a buffer is in use by another device you basically have only two options: Either block for the buffer to become unused or use a hardware method (semaphores) to sync up your operations. Of course you can optimize by using a multiple reader / single writers model, but for this you basically just have to teach ttm that there might be more than one fence for each bo.

Anyway those things are only optimizations, and in the end it always comes down to blocking the command submission if there is no other way of hardware synchronization and I absolutely don't see any reason why we shouldn't do this with just a normal blocking call to a waitqueue.

Christian.