Convert TTM to the new fence interface.

-----Original Message-----
From: Maarten Lankhorst [mailto:[email protected]]
Sent: Wednesday, July 09, 2014 8:30 AM
To: [email protected]
Cc: [email protected]; [email protected]; linux- [email protected]; [email protected]; [email protected]; Deucher, Alexander; Koenig, Christian
Subject: [PATCH 09/17] drm/radeon: use common fence implementation for fences

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/radeon/radeon.h        |   15 +-
 drivers/gpu/drm/radeon/radeon_device.c |   60 ++++++++-
 drivers/gpu/drm/radeon/radeon_fence.c  |  223
++++++++++++++++++++++++++------
 3 files changed, 248 insertions(+), 50 deletions(-)

drivers/gpu/drm/radeon/radeon.h +6 -9

index 29d9cc04c04e..03a5567f2c2f 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>

@@ -116,9 +117,6 @@ extern int radeon_deep_color;
 #define RADEONFB_CONN_LIMIT			4
 #define RADEON_BIOS_NUM_SCRATCH			8
 
-/* 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

@@ -350,12 +348,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);

@@ -2268,6 +2269,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;

@@ -2358,11 +2360,6 @@ u32 cik_mm_rdoorbell(struct radeon_device
 *rdev, u32 index);
 void cik_mm_wdoorbell(struct radeon_device *rdev, u32 index, 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 03686fab842d..86699df7c8f3 100644
@@ -1213,6 +1213,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-

Quoted 1 line

device,

drivers/gpu/drm/radeon/radeon_device.c +48 -0

@@ -1607,6 +1608,54 @@ int radeon_resume_kms(struct drm_device *dev,
 bool resume, bool fbcon)
 	return 0;
 }
 
+static uint32_t radeon_gpu_mask_sw_irq(struct radeon_device *rdev)
+{
+	uint32_t mask = 0;
+	int i;
+
+	if (!rdev->ddev->irq_enabled)
+		return mask;
+
+	/*
+	 * increase refcount on sw interrupts for all rings to stop
+	 * enabling interrupts in radeon_fence_enable_signaling during
+	 * gpu reset.
+	 */
+
+	for (i = 0; i < RADEON_NUM_RINGS; ++i) {
+		if (!rdev->ring[i].ready)
+			continue;
+
+		atomic_inc(&rdev->irq.ring_int[i]);
+		mask |= 1 << i;
+	}
+	return mask;
+}
+
+static void radeon_gpu_unmask_sw_irq(struct radeon_device *rdev,
 uint32_t mask)
+{
+	unsigned long irqflags;
+	int i;
+
+	if (!mask)
+		return;
+
+	/*
+	 * undo refcount increase, and reset irqs to correct value.
+	 */
+
+	for (i = 0; i < RADEON_NUM_RINGS; ++i) {
+		if (!(mask & (1 << i)))
+			continue;
+
+		atomic_dec(&rdev->irq.ring_int[i]);
+	}
+
+	spin_lock_irqsave(&rdev->irq.lock, irqflags);
+	radeon_irq_set(rdev);
+	spin_unlock_irqrestore(&rdev->irq.lock, irqflags);
+}
+
 /**

* radeon_gpu_reset - reset the asic
*

drivers/gpu/drm/radeon/radeon_device.c +10 -1

@@ -1624,6 +1673,7 @@ int radeon_gpu_reset(struct radeon_device *rdev)
 
 	int i, r;
 	int resched;
+	uint32_t sw_mask;
 
 	down_write(&rdev->exclusive_lock);
 

@@ -1637,6 +1687,7 @@ int radeon_gpu_reset(struct radeon_device *rdev)
 	radeon_save_bios_scratch_regs(rdev);
 	/* block TTM */
 	resched = ttm_bo_lock_delayed_workqueue(&rdev->mman.bdev);
+	sw_mask = radeon_gpu_mask_sw_irq(rdev);
 	radeon_pm_suspend(rdev);
 	radeon_suspend(rdev);
 

@@ -1686,13 +1737,20 @@ retry:
 	radeon_pm_resume(rdev);
 	drm_helper_resume_force_mode(rdev->ddev);
 
+	radeon_gpu_unmask_sw_irq(rdev, sw_mask);
 	ttm_bo_unlock_delayed_workqueue(&rdev->mman.bdev,
 resched);
 	if (r) {
 		/* bad news, how to tell it to userspace ? */
 		dev_info(rdev->dev, "GPU reset failed\n");
 	}
 
-	up_write(&rdev->exclusive_lock);
+	/*
+	 * force all waiters to recheck, some may have been
+	 * added while the exclusive_lock was unavailable
+	 */
+	downgrade_write(&rdev->exclusive_lock);
+	wake_up_all(&rdev->fence_queue);
+	up_read(&rdev->exclusive_lock);
 	return r;
 }

drivers/gpu/drm/radeon/radeon_fence.c +27 -10

index 6435719fd45b..81c98f6ff0ca 100644
@@ -39,6 +39,15 @@
 #include "radeon.h"
 #include "radeon_trace.h"
 
+static const struct fence_ops radeon_fence_ops;
+
+#define to_radeon_fence(p) \
+	({								\
+		struct radeon_fence *__f;				\
+		__f = container_of((p), struct radeon_fence, base);	\
+		__f->base.ops == &radeon_fence_ops ? __f : NULL;	\
+	})
+
 /*
  * Fences
  * Fences mark an event in the GPUs pipeline and are used

@@ -111,30 +120,55 @@ int radeon_fence_emit(struct radeon_device
 *rdev,
 		      struct radeon_fence **fence,
 		      int ring)
 {
+	u64 seq = ++rdev->fence_drv[ring].sync_seq[ring];
+
 	/* we are protected by the ring emission mutex */
 	*fence = kmalloc(sizeof(struct radeon_fence), GFP_KERNEL);
 	if ((*fence) == NULL) {
 		return -ENOMEM;
 	}
-	kref_init(&((*fence)->kref));
-	(*fence)->rdev = rdev;
-	(*fence)->seq = ++rdev->fence_drv[ring].sync_seq[ring];
 	(*fence)->ring = ring;
+	fence_init(&(*fence)->base, &radeon_fence_ops,
+		   &rdev->fence_queue.lock, rdev->fence_context + ring,
 seq);
+	(*fence)->rdev = rdev;
+	(*fence)->seq = seq;
 	radeon_fence_ring_emit(rdev, ring, *fence);
 	trace_radeon_fence_emit(rdev->ddev, ring, (*fence)->seq);
 	return 0;
 }
 
 /**
- * radeon_fence_process - process a fence
+ * radeon_fence_check_signaled - callback from fence_queue
  *
- * @rdev: radeon_device pointer
- * @ring: ring index the fence is associated with
- *
- * Checks the current fence value and wakes the fence queue
- * if the sequence number has increased (all asics).
+ * this function is called with fence_queue lock held, which is also used
+ * for the fence locking itself, so unlocked variants are used for
+ * fence_signal, and remove_wait_queue.
  */
-void radeon_fence_process(struct radeon_device *rdev, int ring)
+static int radeon_fence_check_signaled(wait_queue_t *wait, unsigned
 mode, int flags, void *key)
+{
+	struct radeon_fence *fence;
+	u64 seq;
+
+	fence = container_of(wait, struct radeon_fence, fence_wake);
+
+	seq = atomic64_read(&fence->rdev->fence_drv[fence-

Quoted 1 line

ring].last_seq);

drivers/gpu/drm/radeon/radeon_fence.c +5 -0

+	if (seq >= fence->seq) {
+		int ret = fence_signal_locked(&fence->base);
+
+		if (!ret)
+			FENCE_TRACE(&fence->base, "signaled from irq

context\n");

drivers/gpu/drm/radeon/radeon_fence.c +2 -0

+		else
+			FENCE_TRACE(&fence->base, "was already

signaled\n");

drivers/gpu/drm/radeon/radeon_fence.c +3 -0

+
+		radeon_irq_kms_sw_irq_put(fence->rdev, fence->ring);
+		__remove_wait_queue(&fence->rdev->fence_queue,

&fence->fence_wake);

drivers/gpu/drm/radeon/radeon_fence.c +16 -10

+		fence_put(&fence->base);
+	} else
+		FENCE_TRACE(&fence->base, "pending\n");
+	return 0;
+}
+
+static bool __radeon_fence_process(struct radeon_device *rdev, int ring)
 {
 	uint64_t seq, last_seq, last_emitted;
 	unsigned count_loop = 0;

@@ -190,23 +224,22 @@ void radeon_fence_process(struct radeon_device
 *rdev, int ring)
 		}
 	} while (atomic64_xchg(&rdev->fence_drv[ring].last_seq, seq) >
 seq);
 
-	if (wake)
-		wake_up_all(&rdev->fence_queue);
+	return wake;
 }
 
 /**
- * radeon_fence_destroy - destroy a fence
+ * radeon_fence_process - process a fence
  *
- * @kref: fence kref
+ * @rdev: radeon_device pointer
+ * @ring: ring index the fence is associated with
  *
- * Frees the fence object (all asics).
+ * Checks the current fence value and wakes the fence queue
+ * if the sequence number has increased (all asics).
  */
-static void radeon_fence_destroy(struct kref *kref)
+void radeon_fence_process(struct radeon_device *rdev, int ring)
 {
-	struct radeon_fence *fence;
-
-	fence = container_of(kref, struct radeon_fence, kref);
-	kfree(fence);
+	if (__radeon_fence_process(rdev, ring))
+		wake_up_all(&rdev->fence_queue);
 }

/**

drivers/gpu/drm/radeon/radeon_fence.c +36 -0

@@ -237,6 +270,69 @@ static bool radeon_fence_seq_signaled(struct
 radeon_device *rdev,
 	return false;
 }
 
+static bool __radeon_fence_signaled(struct fence *f)
+{
+	struct radeon_fence *fence = to_radeon_fence(f);
+	struct radeon_device *rdev = fence->rdev;
+	unsigned ring = fence->ring;
+	u64 seq = fence->seq;
+
+	if (atomic64_read(&rdev->fence_drv[ring].last_seq) >= seq) {
+		return true;
+	}
+
+	if (down_read_trylock(&rdev->exclusive_lock)) {
+		radeon_fence_process(rdev, ring);
+		up_read(&rdev->exclusive_lock);
+
+		if (atomic64_read(&rdev->fence_drv[ring].last_seq) >= seq)
 {
+			return true;
+		}
+	}
+	return false;
+}
+
+/**
+ * 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)
+{
+	struct radeon_fence *fence = to_radeon_fence(f);
+	struct radeon_device *rdev = fence->rdev;
+
+	if (atomic64_read(&rdev->fence_drv[fence->ring].last_seq) >=

fence->seq ||

drivers/gpu/drm/radeon/radeon_fence.c +14 -0

+	    !rdev->ddev->irq_enabled)
+		return false;
+
+	radeon_irq_kms_sw_irq_get(rdev, fence->ring);
+
+	if (down_read_trylock(&rdev->exclusive_lock)) {
+		if (__radeon_fence_process(rdev, fence->ring))
+			wake_up_all_locked(&rdev->fence_queue);
+
+		up_read(&rdev->exclusive_lock);
+	}
+
+	/* did fence get signaled after we enabled the sw irq? */
+	if (atomic64_read(&rdev->fence_drv[fence->ring].last_seq) >=

fence->seq) {

drivers/gpu/drm/radeon/radeon_fence.c +19 -4

+		radeon_irq_kms_sw_irq_put(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(&rdev->fence_queue, &fence->fence_wake);
+	fence_get(f);
+
+	return true;
+}
+
 /**
  * radeon_fence_signaled - check if a fence has signaled
  *

@@ -250,11 +346,13 @@ bool radeon_fence_signaled(struct radeon_fence
 *fence)
 	if (!fence) {
 		return true;
 	}
-	if (fence->seq == RADEON_FENCE_SIGNALED_SEQ) {
-		return true;
-	}
+
 	if (radeon_fence_seq_signaled(fence->rdev, fence->seq, fence-
 >ring)) {
-		fence->seq = RADEON_FENCE_SIGNALED_SEQ;
+		int ret;
+
+		ret = fence_signal(&fence->base);
+		if (!ret)
+			FENCE_TRACE(&fence->base, "signaled from
 radeon_fence_signaled\n");

drivers/gpu/drm/radeon/radeon_fence.c +5 -8

 		return true;
 	}
 	return false;

@@ -413,21 +511,18 @@ int radeon_fence_wait(struct radeon_fence
 *fence, bool intr)
 	uint64_t seq[RADEON_NUM_RINGS] = {};
 	long r;
 
-	if (fence == NULL) {
-		WARN(1, "Querying an invalid fence : %p !\n", fence);
-		return -EINVAL;
-	}
-
-	seq[fence->ring] = fence->seq;
-	if (seq[fence->ring] == RADEON_FENCE_SIGNALED_SEQ)
+	if (test_bit(FENCE_FLAG_SIGNALED_BIT, &fence->base.flags))
 		return 0;
 
+	seq[fence->ring] = fence->seq;
 	r = radeon_fence_wait_seq_timeout(fence->rdev, seq, intr,
 MAX_SCHEDULE_TIMEOUT);
 	if (r < 0) {
 		return r;
 	}
 
-	fence->seq = RADEON_FENCE_SIGNALED_SEQ;
+	r = fence_signal(&fence->base);
+	if (!r)
+		FENCE_TRACE(&fence->base, "signaled from
 fence_wait\n");

drivers/gpu/drm/radeon/radeon_fence.c +5 -4

 	return 0;
 }

@@ -459,12 +554,13 @@ int radeon_fence_wait_any(struct radeon_device
 *rdev,
 			continue;
 		}
 
+		if (test_bit(FENCE_FLAG_SIGNALED_BIT, &fences[i]-
 >base.flags)) {
+			/* already signaled */
+			return 0;
+		}
+
 		seq[i] = fences[i]->seq;
 		++num_rings;
-
-		/* test if something was allready signaled */
-		if (seq[i] == RADEON_FENCE_SIGNALED_SEQ)
-			return 0;
 	}

drivers/gpu/drm/radeon/radeon_fence.c +3 -4

 	/* nothing to wait for ? */

@@ -545,7 +641,7 @@ int radeon_fence_wait_empty(struct radeon_device
 *rdev, int ring)
  */
 struct radeon_fence *radeon_fence_ref(struct radeon_fence *fence)
 {
-	kref_get(&fence->kref);
+	fence_get(&fence->base);
 	return fence;
 }

@@ -561,9 +657,8 @@ void radeon_fence_unref(struct radeon_fence
 **fence)
 	struct radeon_fence *tmp = *fence;
 
 	*fence = NULL;
-	if (tmp) {
-		kref_put(&tmp->kref, radeon_fence_destroy);
-	}
+	if (tmp)
+		fence_put(&tmp->base);
 }
 

/**

drivers/gpu/drm/radeon/radeon_fence.c +12 -0

@@ -872,3 +967,51 @@ int radeon_debugfs_fence_init(struct
 radeon_device *rdev)
 	return 0;
 #endif
 }
+
+static long __radeon_fence_wait(struct fence *f, bool intr, long timeout)
+{
+	struct radeon_fence *fence = to_radeon_fence(f);
+	u64 target_seq[RADEON_NUM_RINGS] = {};
+	struct radeon_device *rdev = fence->rdev;
+	long r;
+
+	target_seq[fence->ring] = fence->seq;
+
+	down_read(&rdev->exclusive_lock);
+	r = radeon_fence_wait_seq_timeout(fence->rdev, target_seq, intr,

timeout);

drivers/gpu/drm/radeon/radeon_fence.c +3 -0

+
+	if (r > 0 && !fence_signal(&fence->base))
+		FENCE_TRACE(&fence->base, "signaled from

__radeon_fence_wait\n");

drivers/gpu/drm/radeon/radeon_fence.c +11 -0

+
+	up_read(&rdev->exclusive_lock);
+	return r;
+
+}
+
+static const char *radeon_fence_get_driver_name(struct fence *fence)
+{
+	return "radeon";
+}
+

drivers/gpu/drm/radeon/radeon_fence.c +12 -0

+	default: WARN_ON_ONCE(1); return "radeon.unk";
+	}
+}
+
+static const struct fence_ops radeon_fence_ops = {
+	.get_driver_name = radeon_fence_get_driver_name,
+	.get_timeline_name = radeon_fence_get_timeline_name,
+	.enable_signaling = radeon_fence_enable_signaling,
+	.signaled = __radeon_fence_signaled,
+	.wait = __radeon_fence_wait,
+	.release = NULL,
+};

Review

Quoted 11 lines

<snip>

(quoted diff) +10 -0

+static const char *radeon_fence_get_timeline_name(struct fence *f)
+{
+	struct radeon_fence *fence = to_radeon_fence(f);
+	switch (fence->ring) {
+	case RADEON_RING_TYPE_GFX_INDEX: return "radeon.gfx";
+	case CAYMAN_RING_TYPE_CP1_INDEX: return "radeon.cp1";
+	case CAYMAN_RING_TYPE_CP2_INDEX: return "radeon.cp2";
+	case R600_RING_TYPE_DMA_INDEX: return "radeon.dma";
+	case CAYMAN_RING_TYPE_DMA1_INDEX: return "radeon.dma1";
+	case R600_RING_TYPE_UVD_INDEX: return "radeon.uvd";

Radeon supports vce rings on newer ascis. Probably want to add the case for those here too.

Alex

op 09-07-14 14:57, Deucher, Alexander schreef:

Quoted 13 lines

drivers/gpu/drm/radeon/radeon_fence.c int radeon_debugfs_fence_init(struct radeon_device *rdev) · line 1005

Quoted 11 lines

<snip>

drivers/gpu/drm/radeon/radeon_fence.c +10 -0

+static const char *radeon_fence_get_timeline_name(struct fence *f)
+{
+    struct radeon_fence *fence = to_radeon_fence(f);
+    switch (fence->ring) {
+    case RADEON_RING_TYPE_GFX_INDEX: return "radeon.gfx";
+    case CAYMAN_RING_TYPE_CP1_INDEX: return "radeon.cp1";
+    case CAYMAN_RING_TYPE_CP2_INDEX: return "radeon.cp2";
+    case R600_RING_TYPE_DMA_INDEX: return "radeon.dma";
+    case CAYMAN_RING_TYPE_DMA1_INDEX: return "radeon.dma1";
+    case R600_RING_TYPE_UVD_INDEX: return "radeon.uvd";

Radeon supports vce rings on newer ascis. Probably want to add the case for those here too.

Quoted 2 lines

Alex

Indeed, how about this?

----------8<-------

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/radeon/radeon.h        |  15 +--
 drivers/gpu/drm/radeon/radeon_device.c |  60 ++++++++-
 drivers/gpu/drm/radeon/radeon_fence.c  | 225 +++++++++++++++++++++++++++------
 3 files changed, 250 insertions(+), 50 deletions(-)

drivers/gpu/drm/radeon/radeon.h +6 -9

index 29d9cc04c04e..03a5567f2c2f 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>

@@ -116,9 +117,6 @@ extern int radeon_deep_color;
 #define RADEONFB_CONN_LIMIT                    4
 #define RADEON_BIOS_NUM_SCRATCH                        8
 
-/* 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

@@ -350,12 +348,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);

@@ -2268,6 +2269,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;

@@ -2358,11 +2360,6 @@ u32 cik_mm_rdoorbell(struct radeon_device *rdev, u32 index);
 void cik_mm_wdoorbell(struct radeon_device *rdev, u32 index, 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 +59 -1

index 03686fab842d..86699df7c8f3 100644
@@ -1213,6 +1213,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,

@@ -1607,6 +1608,54 @@ int radeon_resume_kms(struct drm_device *dev, bool resume, bool fbcon)
        return 0;
 }
 
+static uint32_t radeon_gpu_mask_sw_irq(struct radeon_device *rdev)
+{
+       uint32_t mask = 0;
+       int i;
+
+       if (!rdev->ddev->irq_enabled)
+               return mask;
+
+       /*
+        * increase refcount on sw interrupts for all rings to stop
+        * enabling interrupts in radeon_fence_enable_signaling during
+        * gpu reset.
+        */
+
+       for (i = 0; i < RADEON_NUM_RINGS; ++i) {
+               if (!rdev->ring[i].ready)
+                       continue;
+
+               atomic_inc(&rdev->irq.ring_int[i]);
+               mask |= 1 << i;
+       }
+       return mask;
+}
+
+static void radeon_gpu_unmask_sw_irq(struct radeon_device *rdev, uint32_t mask)
+{
+       unsigned long irqflags;
+       int i;
+
+       if (!mask)
+               return;
+
+       /*
+        * undo refcount increase, and reset irqs to correct value.
+        */
+
+       for (i = 0; i < RADEON_NUM_RINGS; ++i) {
+               if (!(mask & (1 << i)))
+                       continue;
+
+               atomic_dec(&rdev->irq.ring_int[i]);
+       }
+
+       spin_lock_irqsave(&rdev->irq.lock, irqflags);
+       radeon_irq_set(rdev);
+       spin_unlock_irqrestore(&rdev->irq.lock, irqflags);
+}
+
 /**
  * radeon_gpu_reset - reset the asic
  *

@@ -1624,6 +1673,7 @@ int radeon_gpu_reset(struct radeon_device *rdev)
 
        int i, r;
        int resched;
+       uint32_t sw_mask;
 
        down_write(&rdev->exclusive_lock);
 

@@ -1637,6 +1687,7 @@ int radeon_gpu_reset(struct radeon_device *rdev)
        radeon_save_bios_scratch_regs(rdev);
        /* block TTM */
        resched = ttm_bo_lock_delayed_workqueue(&rdev->mman.bdev);
+       sw_mask = radeon_gpu_mask_sw_irq(rdev);
        radeon_pm_suspend(rdev);
        radeon_suspend(rdev);
 

@@ -1686,13 +1737,20 @@ retry:
        radeon_pm_resume(rdev);
        drm_helper_resume_force_mode(rdev->ddev);
 
+       radeon_gpu_unmask_sw_irq(rdev, sw_mask);
        ttm_bo_unlock_delayed_workqueue(&rdev->mman.bdev, resched);
        if (r) {
                /* bad news, how to tell it to userspace ? */
                dev_info(rdev->dev, "GPU reset failed\n");
        }
 
-       up_write(&rdev->exclusive_lock);
+       /*
+        * force all waiters to recheck, some may have been
+        * added while the exclusive_lock was unavailable
+        */
+       downgrade_write(&rdev->exclusive_lock);
+       wake_up_all(&rdev->fence_queue);
+       up_read(&rdev->exclusive_lock);
        return r;
 }
 

drivers/gpu/drm/radeon/radeon_fence.c +185 -41

index 6435719fd45b..763b7928026d 100644
@@ -39,6 +39,15 @@
 #include "radeon.h"
 #include "radeon_trace.h"
 
+static const struct fence_ops radeon_fence_ops;
+
+#define to_radeon_fence(p) \
+       ({                                                              \
+               struct radeon_fence *__f;                               \
+               __f = container_of((p), struct radeon_fence, base);     \
+               __f->base.ops == &radeon_fence_ops ? __f : NULL;        \
+       })
+
 /*
  * Fences
  * Fences mark an event in the GPUs pipeline and are used

@@ -111,30 +120,55 @@ int radeon_fence_emit(struct radeon_device *rdev,
                      struct radeon_fence **fence,
                      int ring)
 {
+       u64 seq = ++rdev->fence_drv[ring].sync_seq[ring];
+
        /* we are protected by the ring emission mutex */
        *fence = kmalloc(sizeof(struct radeon_fence), GFP_KERNEL);
        if ((*fence) == NULL) {
                return -ENOMEM;
        }
-       kref_init(&((*fence)->kref));
-       (*fence)->rdev = rdev;
-       (*fence)->seq = ++rdev->fence_drv[ring].sync_seq[ring];
        (*fence)->ring = ring;
+       fence_init(&(*fence)->base, &radeon_fence_ops,
+                  &rdev->fence_queue.lock, rdev->fence_context + ring, seq);
+       (*fence)->rdev = rdev;
+       (*fence)->seq = seq;
        radeon_fence_ring_emit(rdev, ring, *fence);
        trace_radeon_fence_emit(rdev->ddev, ring, (*fence)->seq);
        return 0;
 }
 
 /**
- * radeon_fence_process - process a fence
+ * radeon_fence_check_signaled - callback from fence_queue
  *
- * @rdev: radeon_device pointer
- * @ring: ring index the fence is associated with
- *
- * Checks the current fence value and wakes the fence queue
- * if the sequence number has increased (all asics).
+ * this function is called with fence_queue lock held, which is also used
+ * for the fence locking itself, so unlocked variants are used for
+ * fence_signal, and remove_wait_queue.
  */
-void radeon_fence_process(struct radeon_device *rdev, int ring)
+static int radeon_fence_check_signaled(wait_queue_t *wait, unsigned mode, int flags, void *key)
+{
+       struct radeon_fence *fence;
+       u64 seq;
+
+       fence = container_of(wait, struct radeon_fence, fence_wake);
+
+       seq = atomic64_read(&fence->rdev->fence_drv[fence->ring].last_seq);
+       if (seq >= fence->seq) {
+               int ret = fence_signal_locked(&fence->base);
+
+               if (!ret)
+                       FENCE_TRACE(&fence->base, "signaled from irq context\n");
+               else
+                       FENCE_TRACE(&fence->base, "was already signaled\n");
+
+               radeon_irq_kms_sw_irq_put(fence->rdev, fence->ring);
+               __remove_wait_queue(&fence->rdev->fence_queue, &fence->fence_wake);
+               fence_put(&fence->base);
+       } else
+               FENCE_TRACE(&fence->base, "pending\n");
+       return 0;
+}
+
+static bool __radeon_fence_process(struct radeon_device *rdev, int ring)
 {
        uint64_t seq, last_seq, last_emitted;
        unsigned count_loop = 0;

@@ -190,23 +224,22 @@ void radeon_fence_process(struct radeon_device *rdev, int ring)
                }
        } while (atomic64_xchg(&rdev->fence_drv[ring].last_seq, seq) > seq);
 
-       if (wake)
-               wake_up_all(&rdev->fence_queue);
+       return wake;
 }
 
 /**
- * radeon_fence_destroy - destroy a fence
+ * radeon_fence_process - process a fence
  *
- * @kref: fence kref
+ * @rdev: radeon_device pointer
+ * @ring: ring index the fence is associated with
  *
- * Frees the fence object (all asics).
+ * Checks the current fence value and wakes the fence queue
+ * if the sequence number has increased (all asics).
  */
-static void radeon_fence_destroy(struct kref *kref)
+void radeon_fence_process(struct radeon_device *rdev, int ring)
 {
-       struct radeon_fence *fence;
-
-       fence = container_of(kref, struct radeon_fence, kref);
-       kfree(fence);
+       if (__radeon_fence_process(rdev, ring))
+               wake_up_all(&rdev->fence_queue);
 }
 
 /**

@@ -237,6 +270,69 @@ static bool radeon_fence_seq_signaled(struct radeon_device *rdev,
        return false;
 }
 
+static bool __radeon_fence_signaled(struct fence *f)
+{
+       struct radeon_fence *fence = to_radeon_fence(f);
+       struct radeon_device *rdev = fence->rdev;
+       unsigned ring = fence->ring;
+       u64 seq = fence->seq;
+
+       if (atomic64_read(&rdev->fence_drv[ring].last_seq) >= seq) {
+               return true;
+       }
+
+       if (down_read_trylock(&rdev->exclusive_lock)) {
+               radeon_fence_process(rdev, ring);
+               up_read(&rdev->exclusive_lock);
+
+               if (atomic64_read(&rdev->fence_drv[ring].last_seq) >= seq) {
+                       return true;
+               }
+       }
+       return false;
+}
+
+/**
+ * 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)
+{
+       struct radeon_fence *fence = to_radeon_fence(f);
+       struct radeon_device *rdev = fence->rdev;
+
+       if (atomic64_read(&rdev->fence_drv[fence->ring].last_seq) >= fence->seq ||
+           !rdev->ddev->irq_enabled)
+               return false;
+
+       radeon_irq_kms_sw_irq_get(rdev, fence->ring);
+
+       if (down_read_trylock(&rdev->exclusive_lock)) {
+               if (__radeon_fence_process(rdev, fence->ring))
+                       wake_up_all_locked(&rdev->fence_queue);
+
+               up_read(&rdev->exclusive_lock);
+       }
+
+       /* did fence get signaled after we enabled the sw irq? */
+       if (atomic64_read(&rdev->fence_drv[fence->ring].last_seq) >= fence->seq) {
+               radeon_irq_kms_sw_irq_put(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(&rdev->fence_queue, &fence->fence_wake);
+       fence_get(f);
+
+       return true;
+}
+
 /**
  * radeon_fence_signaled - check if a fence has signaled
  *

@@ -250,11 +346,13 @@ bool radeon_fence_signaled(struct radeon_fence *fence)
        if (!fence) {
                return true;
        }
-       if (fence->seq == RADEON_FENCE_SIGNALED_SEQ) {
-               return true;
-       }
+
        if (radeon_fence_seq_signaled(fence->rdev, fence->seq, fence->ring)) {
-               fence->seq = RADEON_FENCE_SIGNALED_SEQ;
+               int ret;
+
+               ret = fence_signal(&fence->base);
+               if (!ret)
+                       FENCE_TRACE(&fence->base, "signaled from radeon_fence_signaled\n");
                return true;
        }
        return false;

@@ -413,21 +511,18 @@ int radeon_fence_wait(struct radeon_fence *fence, bool intr)
        uint64_t seq[RADEON_NUM_RINGS] = {};
        long r;
 
-       if (fence == NULL) {
-               WARN(1, "Querying an invalid fence : %p !\n", fence);
-               return -EINVAL;
-       }
-
-       seq[fence->ring] = fence->seq;
-       if (seq[fence->ring] == RADEON_FENCE_SIGNALED_SEQ)
+       if (test_bit(FENCE_FLAG_SIGNALED_BIT, &fence->base.flags))
                return 0;
 
+       seq[fence->ring] = fence->seq;
        r = radeon_fence_wait_seq_timeout(fence->rdev, seq, intr, MAX_SCHEDULE_TIMEOUT);
        if (r < 0) {
                return r;
        }
 
-       fence->seq = RADEON_FENCE_SIGNALED_SEQ;
+       r = fence_signal(&fence->base);
+       if (!r)
+               FENCE_TRACE(&fence->base, "signaled from fence_wait\n");
        return 0;
 }
 

@@ -459,12 +554,13 @@ int radeon_fence_wait_any(struct radeon_device *rdev,
                        continue;
                }
 
+               if (test_bit(FENCE_FLAG_SIGNALED_BIT, &fences[i]->base.flags)) {
+                       /* already signaled */
+                       return 0;
+               }
+
                seq[i] = fences[i]->seq;
                ++num_rings;
-
-               /* test if something was allready signaled */
-               if (seq[i] == RADEON_FENCE_SIGNALED_SEQ)
-                       return 0;
        }
 
        /* nothing to wait for ? */

@@ -545,7 +641,7 @@ int radeon_fence_wait_empty(struct radeon_device *rdev, int ring)
  */
 struct radeon_fence *radeon_fence_ref(struct radeon_fence *fence)
 {
-       kref_get(&fence->kref);
+       fence_get(&fence->base);
        return fence;
 }
 

@@ -561,9 +657,8 @@ void radeon_fence_unref(struct radeon_fence **fence)
        struct radeon_fence *tmp = *fence;
 
        *fence = NULL;
-       if (tmp) {
-               kref_put(&tmp->kref, radeon_fence_destroy);
-       }
+       if (tmp)
+               fence_put(&tmp->base);
 }
 
 /**

@@ -872,3 +967,53 @@ int radeon_debugfs_fence_init(struct radeon_device *rdev)
        return 0;
 #endif
 }
+
+static long __radeon_fence_wait(struct fence *f, bool intr, long timeout)
+{
+       struct radeon_fence *fence = to_radeon_fence(f);
+       u64 target_seq[RADEON_NUM_RINGS] = {};
+       struct radeon_device *rdev = fence->rdev;
+       long r;
+
+       target_seq[fence->ring] = fence->seq;
+
+       down_read(&rdev->exclusive_lock);
+       r = radeon_fence_wait_seq_timeout(fence->rdev, target_seq, intr, timeout);
+
+       if (r > 0 && !fence_signal(&fence->base))
+               FENCE_TRACE(&fence->base, "signaled from __radeon_fence_wait\n");
+
+       up_read(&rdev->exclusive_lock);
+       return r;
+
+}
+
+static const char *radeon_fence_get_driver_name(struct fence *fence)
+{
+       return "radeon";
+}
+
+static const char *radeon_fence_get_timeline_name(struct fence *f)
+{
+       struct radeon_fence *fence = to_radeon_fence(f);
+       switch (fence->ring) {
+       case RADEON_RING_TYPE_GFX_INDEX: return "radeon.gfx";
+       case CAYMAN_RING_TYPE_CP1_INDEX: return "radeon.cp1";
+       case CAYMAN_RING_TYPE_CP2_INDEX: return "radeon.cp2";
+       case R600_RING_TYPE_DMA_INDEX: return "radeon.dma";
+       case CAYMAN_RING_TYPE_DMA1_INDEX: return "radeon.dma1";
+       case R600_RING_TYPE_UVD_INDEX: return "radeon.uvd";
+       case TN_RING_TYPE_VCE1_INDEX: return "radeon.vce1";
+       case TN_RING_TYPE_VCE2_INDEX: return "radeon.vce2";
+       default: WARN_ON_ONCE(1); return "radeon.unk";
+       }
+}
+
+static const struct fence_ops radeon_fence_ops = {
+       .get_driver_name = radeon_fence_get_driver_name,
+       .get_timeline_name = radeon_fence_get_timeline_name,
+       .enable_signaling = radeon_fence_enable_signaling,
+       .signaled = __radeon_fence_signaled,
+       .wait = __radeon_fence_wait,
+       .release = NULL,
+};
--

2.0.0

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

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/radeon/radeon.h        |   15 +-
 drivers/gpu/drm/radeon/radeon_device.c |   60 ++++++++-
 drivers/gpu/drm/radeon/radeon_fence.c  |  223 ++++++++++++++++++++++++++------
 3 files changed, 248 insertions(+), 50 deletions(-)

From what I can see this is still suffering from the problem that we

Quoted 7 lines

Signed-off-by Maarten Lankhorst <[email protected]>

---
  drivers/gpu/drm/radeon/radeon.h        |   15 +-
  drivers/gpu/drm/radeon/radeon_device.c |   60 ++++++++-
  drivers/gpu/drm/radeon/radeon_fence.c  |  223 ++++++++++++++++++++++++++------
  3 files changed, 248 insertions(+), 50 deletions(-)

From what I can see this is still suffering from the problem that we
need to find a proper solution to,

My summary of the issues after talking to Jerome and Ben and re-reading things is:

We really need to work out a better interface into the drivers to be able to avoid random atomic entrypoints,

I'm sure you have some ideas and I think you really need to investigate them to move this thing forward,
even it if means some issues with android sync pts.

but none of the two major drivers seem to want the interface as-is so something needs to give

My major question is why we need an atomic callback here at all, what scenario does it cover?

Surely we can use a workqueue based callback to ask a driver to check its signalling, is it really
that urgent?

Dave.

Am 22.07.2014 06:05, schrieb Dave Airlie:

Quoted 11 lines · Maarten Lankhorst

Quoted 7 lines

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/radeon/radeon.h        |   15 +-
 drivers/gpu/drm/radeon/radeon_device.c |   60 ++++++++-
 drivers/gpu/drm/radeon/radeon_fence.c  |  223 ++++++++++++++++++++++++++------
 3 files changed, 248 insertions(+), 50 deletions(-)

From what I can see this is still suffering from the problem that we need to find a proper solution to,

Quoted 5 lines

My summary of the issues after talking to Jerome and Ben and re-reading things is:

We really need to work out a better interface into the drivers to be able to avoid random atomic entrypoints,

Which is exactly what I criticized from the very first beginning. Good to know that I'm not the only one thinking that this isn't such a good idea.

Quoted 3 lines

I'm sure you have some ideas and I think you really need to investigate them to move this thing forward,
even it if means some issues with android sync pts.

Actually I think that TTMs fence interface already gave quite a good hint how it might look like. I can only guess that this won't fit with the Android stuff, otherwise I can't see a good reason why we didn't stick with that.

Quoted 5 lines

but none of the two major drivers seem to want the interface as-is so something needs to give

My major question is why we need an atomic callback here at all, what scenario does it cover?

Agree totally. As far as I can see all current uses of the interface are of the kind of waiting for a fence to signal.

No need for any callback from one driver into another, especially not in atomic context. If a driver needs such a functionality it should just start up a kernel thread and do it's waiting there.

This obviously shouldn't be an obstacle for pure hardware implementations where one driver signals a semaphore another driver is waiting for, or a high signal on an interrupt line directly wired between two chips. And I think this is a completely different topic and not necessarily part of the common fence interface we should currently focus on.

Quoted 12 lines · Christian König

Am 22.07.2014 06:05, schrieb Dave Airlie:

Quoted 11 lines · Maarten Lankhorst

Quoted 7 lines

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/radeon/radeon.h        |   15 +-
 drivers/gpu/drm/radeon/radeon_device.c |   60 ++++++++-
 drivers/gpu/drm/radeon/radeon_fence.c  |  223 ++++++++++++++++++++++++++------
 3 files changed, 248 insertions(+), 50 deletions(-)

From what I can see this is still suffering from the problem that we need to find a proper solution to,

Quoted 8 lines

Quoted 5 lines

My summary of the issues after talking to Jerome and Ben and re-reading things is:

We really need to work out a better interface into the drivers to be able to avoid random atomic entrypoints,

Which is exactly what I criticized from the very first beginning. Good to know that I'm not the only one thinking that this isn't such a good idea.

I guess I've lost context a bit, but which atomic entry point are we talking about? Afaics the only one that's mandatory is the is fence->signaled callback to check whether a fence really has been signalled. It's used internally by the fence code to avoid spurious wakeups. Afaik that should be doable already on any hardware. If that's not the case then we can always track the signalled state in software and double-check in a worker thread before updating the sw state. And wrap this all up into a special fence class if there's more than one driver needing this.

There is nothing else that forces callbacks from atomic contexts upon you. You can use them if you see it fit, but really if it doesn't suit your driver you can just ignore that part and do process based waits everywhere.

Quoted 12 lines · Christian König

Am 22.07.2014 06:05, schrieb Dave Airlie:

Quoted 11 lines · Maarten Lankhorst

Quoted 7 lines

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/radeon/radeon.h        |   15 +-
 drivers/gpu/drm/radeon/radeon_device.c |   60 ++++++++-
 drivers/gpu/drm/radeon/radeon_fence.c  |  223 ++++++++++++++++++++++++++------
 3 files changed, 248 insertions(+), 50 deletions(-)

From what I can see this is still suffering from the problem that we need to find a proper solution to,

Quoted 8 lines

Quoted 5 lines

My summary of the issues after talking to Jerome and Ben and re-reading things is:

We really need to work out a better interface into the drivers to be able to avoid random atomic entrypoints,

Which is exactly what I criticized from the very first beginning. Good to know that I'm not the only one thinking that this isn't such a good idea.

I guess I've lost context a bit, but which atomic entry point are we talking about? Afaics the only one that's mandatory is the is fence->signaled callback to check whether a fence really has been signalled. It's used internally by the fence code to avoid spurious wakeups. Afaik that should be doable already on any hardware. If that's not the case then we can always track the signalled state in software and double-check in a worker thread before updating the sw state. And wrap this all up into a special fence class if there's more than one driver needing this.

Quoted 20 lines · Christian König

Quoted 12 lines

Am 22.07.2014 06:05, schrieb Dave Airlie:

Quoted 11 lines · Maarten Lankhorst

Quoted 7 lines

Signed-off-by Maarten Lankhorst <[email protected]>

---
  drivers/gpu/drm/radeon/radeon.h        |   15 +-
  drivers/gpu/drm/radeon/radeon_device.c |   60 ++++++++-
  drivers/gpu/drm/radeon/radeon_fence.c  |  223 ++++++++++++++++++++++++++------
  3 files changed, 248 insertions(+), 50 deletions(-)

From what I can see this is still suffering from the problem that we
need to find a proper solution to,

Quoted 7 lines

Quoted 5 lines

My summary of the issues after talking to Jerome and Ben and re-reading things is:

We really need to work out a better interface into the drivers to be able to avoid random atomic entrypoints,

Which is exactly what I criticized from the very first beginning. Good to know that I'm not the only one thinking that this isn't such a good idea.

Quoted 9 lines

I guess I've lost context a bit, but which atomic entry point are we talking about? Afaics the only one that's mandatory is the is fence->signaled callback to check whether a fence really has been signalled. It's used internally by the fence code to avoid spurious wakeups. Afaik that should be doable already on any hardware. If that's not the case then we can always track the signalled state in software and double-check in a worker thread before updating the sw state. And wrap this all up into a special fence class if there's more than one driver needing this.

One thing I've forgotten: The i915 scheduler that's floating around runs its bottom half from irq context. So I really want to be able to check fence state from irq context and I also want to make it possible (possible! not mandatory) to register callbacks which are run from any context asap after the fence is signalled.

If the radeon hw/driver doesn't want to cope with that complexity we can fully insolate it with the sw tracked fence state if you don't like Maarten's radeon implementation. But forcing everyone to forgoe this just because you don't like it and don't want to use it in radeon doesn't sound right.

Am 22.07.2014 13:57, schrieb Daniel Vetter:

Quoted 30 lines · Daniel Vetter

Quoted 20 lines · Christian König

Quoted 12 lines

Am 22.07.2014 06:05, schrieb Dave Airlie:

Quoted 11 lines · Maarten Lankhorst

Quoted 7 lines

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/radeon/radeon.h        |   15 +-
 drivers/gpu/drm/radeon/radeon_device.c |   60 ++++++++-
 drivers/gpu/drm/radeon/radeon_fence.c  |  223 ++++++++++++++++++++++++++------
 3 files changed, 248 insertions(+), 50 deletions(-)

From what I can see this is still suffering from the problem that we need to find a proper solution to,

Quoted 7 lines

Quoted 5 lines

My summary of the issues after talking to Jerome and Ben and re-reading things is:

We really need to work out a better interface into the drivers to be able to avoid random atomic entrypoints,

Which is exactly what I criticized from the very first beginning. Good to know that I'm not the only one thinking that this isn't such a good idea.

Quoted 9 lines

I guess I've lost context a bit, but which atomic entry point are we talking about? Afaics the only one that's mandatory is the is fence->signaled callback to check whether a fence really has been signalled. It's used internally by the fence code to avoid spurious wakeups. Afaik that should be doable already on any hardware. If that's not the case then we can always track the signalled state in software and double-check in a worker thread before updating the sw state. And wrap this all up into a special fence class if there's more than one driver needing this.

Quoted 5 lines

One thing I've forgotten: The i915 scheduler that's floating around runs its bottom half from irq context. So I really want to be able to check fence state from irq context and I also want to make it possible (possible! not mandatory) to register callbacks which are run from any context asap after the fence is signalled.

NAK, that's just the bad design I've talked about. Checking fence state inside the same driver from interrupt context is OK, because it's the drivers interrupt that we are talking about here.

Checking fence status from another drivers interrupt context is what really concerns me here, cause your driver doesn't have the slightest idea if the called driver is really capable of checking the fence right now.

Quoted 36 lines

Am 22.07.2014 13:57, schrieb Daniel Vetter:

Quoted 30 lines · Daniel Vetter

Quoted 20 lines · Christian König

Quoted 12 lines

Am 22.07.2014 06:05, schrieb Dave Airlie:

Quoted 11 lines · Maarten Lankhorst

Quoted 7 lines

Signed-off-by Maarten Lankhorst <[email protected]>

---
  drivers/gpu/drm/radeon/radeon.h        |   15 +-
  drivers/gpu/drm/radeon/radeon_device.c |   60 ++++++++-
  drivers/gpu/drm/radeon/radeon_fence.c  |  223 ++++++++++++++++++++++++++------
  3 files changed, 248 insertions(+), 50 deletions(-)

From what I can see this is still suffering from the problem that we
need to find a proper solution to,

Quoted 7 lines

Quoted 5 lines

My summary of the issues after talking to Jerome and Ben and re-reading things is:

We really need to work out a better interface into the drivers to be able to avoid random atomic entrypoints,

Which is exactly what I criticized from the very first beginning. Good to know that I'm not the only one thinking that this isn't such a good idea.

Quoted 9 lines

I guess I've lost context a bit, but which atomic entry point are we talking about? Afaics the only one that's mandatory is the is fence->signaled callback to check whether a fence really has been signalled. It's used internally by the fence code to avoid spurious wakeups. Afaik that should be doable already on any hardware. If that's not the case then we can always track the signalled state in software and double-check in a worker thread before updating the sw state. And wrap this all up into a special fence class if there's more than one driver needing this.

Quoted 5 lines

One thing I've forgotten: The i915 scheduler that's floating around runs its bottom half from irq context. So I really want to be able to check fence state from irq context and I also want to make it possible (possible! not mandatory) to register callbacks which are run from any context asap after the fence is signalled.

Quoted 7 lines

NAK, that's just the bad design I've talked about. Checking fence state inside the same driver from interrupt context is OK, because it's the drivers interrupt that we are talking about here.

Checking fence status from another drivers interrupt context is what really concerns me here, cause your driver doesn't have the slightest idea if the called driver is really capable of checking the fence right now.

I guess my mail hasn't been clear then. If you don't like it we could add a bit of glue to insulate the madness and bad design i915 might do from radeon. That imo doesn't invalidate the overall fence interfaces.

So what about the following:
- fence->enabling_signaling is restricted to be called from process context. We don't use any different yet, so would boild down to adding a WARN_ON(in_interrupt) or so to fence_enable_sw_signalling.

- Make fence->signaled optional (already the case) and don't implement it in readon (i.e. reduce this patch here). Only downside is that radeon needs to correctly (i.e. without races or so) call fence_signal. And the cross-driver synchronization might be a bit less efficient. Note that you can call fence_signal from wherever you want to, so hopefully that doesn't restrict your implementation.

End result: No one calls into radeon from interrupt context, and this is guaranteed.

Would that be something you can agree to?

Like I've said I think restricting the insanity other people are willing to live with just because you don't like it isn't right. But it is certainly right for you to insist on not being forced into any such design. I think the above would achieve this.

Am 22.07.2014 15:26, schrieb Daniel Vetter:

Quoted 60 lines · Christian König

Quoted 36 lines

Am 22.07.2014 13:57, schrieb Daniel Vetter:

Quoted 30 lines · Daniel Vetter

Quoted 20 lines · Christian König

Quoted 12 lines

Am 22.07.2014 06:05, schrieb Dave Airlie:

Quoted 11 lines · Maarten Lankhorst

Quoted 7 lines

Signed-off-by Maarten Lankhorst <[email protected]>

---
  drivers/gpu/drm/radeon/radeon.h        |   15 +-
  drivers/gpu/drm/radeon/radeon_device.c |   60 ++++++++-
  drivers/gpu/drm/radeon/radeon_fence.c  |  223 ++++++++++++++++++++++++++------
  3 files changed, 248 insertions(+), 50 deletions(-)

From what I can see this is still suffering from the problem that we
need to find a proper solution to,

Quoted 7 lines

Quoted 5 lines

My summary of the issues after talking to Jerome and Ben and re-reading things is:

We really need to work out a better interface into the drivers to be able to avoid random atomic entrypoints,

Which is exactly what I criticized from the very first beginning. Good to know that I'm not the only one thinking that this isn't such a good idea.

Quoted 9 lines

I guess I've lost context a bit, but which atomic entry point are we talking about? Afaics the only one that's mandatory is the is fence->signaled callback to check whether a fence really has been signalled. It's used internally by the fence code to avoid spurious wakeups. Afaik that should be doable already on any hardware. If that's not the case then we can always track the signalled state in software and double-check in a worker thread before updating the sw state. And wrap this all up into a special fence class if there's more than one driver needing this.

Quoted 5 lines

One thing I've forgotten: The i915 scheduler that's floating around runs its bottom half from irq context. So I really want to be able to check fence state from irq context and I also want to make it possible (possible! not mandatory) to register callbacks which are run from any context asap after the fence is signalled.

Quoted 7 lines

NAK, that's just the bad design I've talked about. Checking fence state inside the same driver from interrupt context is OK, because it's the drivers interrupt that we are talking about here.

Checking fence status from another drivers interrupt context is what really concerns me here, cause your driver doesn't have the slightest idea if the called driver is really capable of checking the fence right now.

I guess my mail hasn't been clear then. If you don't like it we could add a bit of glue to insulate the madness and bad design i915 might do from radeon. That imo doesn't invalidate the overall fence interfaces.

So what about the following:
- fence->enabling_signaling is restricted to be called from process context. We don't use any different yet, so would boild down to adding a WARN_ON(in_interrupt) or so to fence_enable_sw_signalling.

- Make fence->signaled optional (already the case) and don't implement it in readon (i.e. reduce this patch here). Only downside is that radeon needs to correctly (i.e. without races or so) call fence_signal. And the cross-driver synchronization might be a bit less efficient. Note that you can call fence_signal from wherever you want to, so hopefully that doesn't restrict your implementation.

Quoted 4 lines

End result: No one calls into radeon from interrupt context, and this is guaranteed.

Would that be something you can agree to?

No, the whole enable_signaling stuff should go away. No callback from the driver into the fence code, only the other way around.

fence->signaled as well as fence->wait should become mandatory and only called from process context without holding any locks, neither atomic nor any mutex/semaphore (rcu might be ok).

Quoted 4 lines

Like I've said I think restricting the insanity other people are willing to live with just because you don't like it isn't right. But it is certainly right for you to insist on not being forced into any such design. I think the above would achieve this.

I don't think so. If it's just me I would say that I'm just to cautious and the idea is still save to apply to the whole kernel.

But since Dave, Jerome and Ben seems to have similar concerns I think we need to agree to a minimum and save interface for all drivers.

Christian.

op 22-07-14 15:45, Christian König schreef:

Quoted 61 lines

Am 22.07.2014 15:26, schrieb Daniel Vetter:

Quoted 60 lines · Christian König

Quoted 36 lines

Am 22.07.2014 13:57, schrieb Daniel Vetter:

Quoted 30 lines · Daniel Vetter

Quoted 20 lines · Christian König

Quoted 12 lines

Am 22.07.2014 06:05, schrieb Dave Airlie:

Quoted 11 lines · Maarten Lankhorst

Quoted 7 lines

Signed-off-by Maarten Lankhorst <[email protected]>

---
   drivers/gpu/drm/radeon/radeon.h        |   15 +-
   drivers/gpu/drm/radeon/radeon_device.c |   60 ++++++++-
   drivers/gpu/drm/radeon/radeon_fence.c  |  223 ++++++++++++++++++++++++++------
   3 files changed, 248 insertions(+), 50 deletions(-)

From what I can see this is still suffering from the problem that we
need to find a proper solution to,

Quoted 7 lines

Quoted 5 lines

My summary of the issues after talking to Jerome and Ben and re-reading things is:

We really need to work out a better interface into the drivers to be able to avoid random atomic entrypoints,

Which is exactly what I criticized from the very first beginning. Good to know that I'm not the only one thinking that this isn't such a good idea.

Quoted 9 lines

I guess I've lost context a bit, but which atomic entry point are we talking about? Afaics the only one that's mandatory is the is fence->signaled callback to check whether a fence really has been signalled. It's used internally by the fence code to avoid spurious wakeups. Afaik that should be doable already on any hardware. If that's not the case then we can always track the signalled state in software and double-check in a worker thread before updating the sw state. And wrap this all up into a special fence class if there's more than one driver needing this.

Quoted 5 lines

One thing I've forgotten: The i915 scheduler that's floating around runs its bottom half from irq context. So I really want to be able to check fence state from irq context and I also want to make it possible (possible! not mandatory) to register callbacks which are run from any context asap after the fence is signalled.

Quoted 7 lines

NAK, that's just the bad design I've talked about. Checking fence state inside the same driver from interrupt context is OK, because it's the drivers interrupt that we are talking about here.

Checking fence status from another drivers interrupt context is what really concerns me here, cause your driver doesn't have the slightest idea if the called driver is really capable of checking the fence right now.

I guess my mail hasn't been clear then. If you don't like it we could add a bit of glue to insulate the madness and bad design i915 might do from radeon. That imo doesn't invalidate the overall fence interfaces.

So what about the following:
- fence->enabling_signaling is restricted to be called from process context. We don't use any different yet, so would boild down to adding a WARN_ON(in_interrupt) or so to fence_enable_sw_signalling.

- Make fence->signaled optional (already the case) and don't implement it in readon (i.e. reduce this patch here). Only downside is that radeon needs to correctly (i.e. without races or so) call fence_signal. And the cross-driver synchronization might be a bit less efficient. Note that you can call fence_signal from wherever you want to, so hopefully that doesn't restrict your implementation.

Quoted 7 lines

Quoted 4 lines

End result: No one calls into radeon from interrupt context, and this is guaranteed.

Would that be something you can agree to?

No, the whole enable_signaling stuff should go away. No callback from the driver into the fence code, only the other way around.

fence->signaled as well as fence->wait should become mandatory and only called from process context without holding any locks, neither atomic nor any mutex/semaphore (rcu might be ok).

fence->wait is mandatory, and already requires sleeping.

If .signaled is not implemented there is no guarantee the fence will be signaled sometime soon, this is also why enable_signaling exists, to allow the driver to flush. I get it that it doesn't apply to radeon and nouveau, but for other drivers that could be necessary, like vmwgfx.

Ironically that is also a part of the ttm fence, except it was called flush there.

I would also like to note that ttm_bo_wait currently is also a function that currently uses is_signaled from atomic_context...

For the more complicated locking worries: Lockdep is your friend, use PROVE_LOCKING and find bugs before they trigger. ;-)

Quoted 10 lines

Quoted 4 lines

Like I've said I think restricting the insanity other people are willing to live with just because you don't like it isn't right. But it is certainly right for you to insist on not being forced into any such design. I think the above would achieve this.

I don't think so. If it's just me I would say that I'm just to cautious and the idea is still save to apply to the whole kernel.

But since Dave, Jerome and Ben seems to have similar concerns I think we need to agree to a minimum and save interface for all drivers.

Christian.

Am 22.07.2014 16:44, schrieb Maarten Lankhorst:

Quoted 71 lines

op 22-07-14 15:45, Christian König schreef:

Quoted 61 lines

Am 22.07.2014 15:26, schrieb Daniel Vetter:

Quoted 60 lines · Christian König

Quoted 36 lines

Am 22.07.2014 13:57, schrieb Daniel Vetter:

Quoted 30 lines · Daniel Vetter

Quoted 20 lines · Christian König

Quoted 12 lines

Am 22.07.2014 06:05, schrieb Dave Airlie:

Quoted 11 lines · Maarten Lankhorst

Quoted 7 lines

Signed-off-by Maarten Lankhorst <[email protected]>

---
   drivers/gpu/drm/radeon/radeon.h        |   15 +-
   drivers/gpu/drm/radeon/radeon_device.c |   60 ++++++++-
   drivers/gpu/drm/radeon/radeon_fence.c  |  223 ++++++++++++++++++++++++++------
   3 files changed, 248 insertions(+), 50 deletions(-)

From what I can see this is still suffering from the problem that we
need to find a proper solution to,

Quoted 7 lines

Quoted 5 lines

My summary of the issues after talking to Jerome and Ben and re-reading things is:

We really need to work out a better interface into the drivers to be able to avoid random atomic entrypoints,

Which is exactly what I criticized from the very first beginning. Good to know that I'm not the only one thinking that this isn't such a good idea.

Quoted 9 lines

I guess I've lost context a bit, but which atomic entry point are we talking about? Afaics the only one that's mandatory is the is fence->signaled callback to check whether a fence really has been signalled. It's used internally by the fence code to avoid spurious wakeups. Afaik that should be doable already on any hardware. If that's not the case then we can always track the signalled state in software and double-check in a worker thread before updating the sw state. And wrap this all up into a special fence class if there's more than one driver needing this.

Quoted 5 lines

One thing I've forgotten: The i915 scheduler that's floating around runs its bottom half from irq context. So I really want to be able to check fence state from irq context and I also want to make it possible (possible! not mandatory) to register callbacks which are run from any context asap after the fence is signalled.

Quoted 7 lines

NAK, that's just the bad design I've talked about. Checking fence state inside the same driver from interrupt context is OK, because it's the drivers interrupt that we are talking about here.

Checking fence status from another drivers interrupt context is what really concerns me here, cause your driver doesn't have the slightest idea if the called driver is really capable of checking the fence right now.

I guess my mail hasn't been clear then. If you don't like it we could add a bit of glue to insulate the madness and bad design i915 might do from radeon. That imo doesn't invalidate the overall fence interfaces.

So what about the following:
- fence->enabling_signaling is restricted to be called from process context. We don't use any different yet, so would boild down to adding a WARN_ON(in_interrupt) or so to fence_enable_sw_signalling.

- Make fence->signaled optional (already the case) and don't implement it in readon (i.e. reduce this patch here). Only downside is that radeon needs to correctly (i.e. without races or so) call fence_signal. And the cross-driver synchronization might be a bit less efficient. Note that you can call fence_signal from wherever you want to, so hopefully that doesn't restrict your implementation.

Quoted 7 lines

Quoted 4 lines

End result: No one calls into radeon from interrupt context, and this is guaranteed.

Would that be something you can agree to?

No, the whole enable_signaling stuff should go away. No callback from the driver into the fence code, only the other way around.

fence->signaled as well as fence->wait should become mandatory and only called from process context without holding any locks, neither atomic nor any mutex/semaphore (rcu might be ok).

fence->wait is mandatory, and already requires sleeping.

Quoted 6 lines

If .signaled is not implemented there is no guarantee the fence will be signaled sometime soon, this is also why enable_signaling exists, to allow the driver to flush. I get it that it doesn't apply to radeon and nouveau, but for other drivers that could be necessary, like vmwgfx.

Ironically that is also a part of the ttm fence, except it was called flush there.

Then call it flush again and make it optional like in TTM.

Quoted 1 line

I would also like to note that ttm_bo_wait currently is also a function that currently uses is_signaled from atomic_context...

I know, but TTM is only called from inside a single driver, no inter driver needs here. We currently even call the internal fence implementation from interrupt context as well and at more than one occasion assume that TTM only uses radeon fences.

Quoted 1 line

Would that be something you can agree to?

No, the whole enable_signaling stuff should go away. No callback from the driver into the fence code, only the other way around.

fence->signaled as well as fence->wait should become mandatory and only called from process context without holding any locks, neither atomic nor any mutex/semaphore (rcu might be ok).

Quoted 4 lines

Like I've said I think restricting the insanity other people are willing to live with just because you don't like it isn't right. But it is certainly right for you to insist on not being forced into any such design. I think the above would achieve this.

I don't think so. If it's just me I would say that I'm just to cautious and the idea is still save to apply to the whole kernel.

But since Dave, Jerome and Ben seems to have similar concerns I think we need to agree to a minimum and save interface for all drivers.

Quoted 8 lines · Christian König

Quoted 1 line

Would that be something you can agree to?

No, the whole enable_signaling stuff should go away. No callback from the driver into the fence code, only the other way around.

fence->signaled as well as fence->wait should become mandatory and only called from process context without holding any locks, neither atomic nor any mutex/semaphore (rcu might be ok).

So for the enable_signaling, that's optional already. It's only for drivers that don't want to keep interrupts enabled all the time. You can opt out of that easily.

Wrt holding no locks at all while calling into any fence functions, that's just not going to work out. The point here is to make different drivers work together and we can rework all the ttm and i915 code to work locklessly in all cases where they need to wait for someone to complete rendering. Or at least I don't think that's feasible. So if you insist that no one might call into radeon code then we simply need to exclude radeon from participating in any shared fencing. But that's a bit pointless.

Quoted 5 lines

Quoted 4 lines

Like I've said I think restricting the insanity other people are willing to live with just because you don't like it isn't right. But it is certainly right for you to insist on not being forced into any such design. I think the above would achieve this.

Quoted 5 lines

I don't think so. If it's just me I would say that I'm just to cautious and the idea is still save to apply to the whole kernel.

But since Dave, Jerome and Ben seems to have similar concerns I think we need to agree to a minimum and save interface for all drivers.

Well I haven't yet seen a proposal that actually works.

From an intel
pov I don't care that much since we don't care about desktop prime, so if radeon/nouveau don't want to do that, meh. Imo the design as-is is fairly sound, and as simple as it can get given the requirements. I haven't heard an argument convincing me otherwise, so I guess we won't have prime support on linux that actually works, ever. -Daniel

Drivers exporting fences need to provide a fence->signaled and a fence->wait function, everything else like fence->enable_signaling or calling fence_signaled() from the driver is optional.

Drivers wanting to use exported fences don't call fence->signaled or fence->wait in atomic or interrupt context, and not with holding any global locking primitives (like mmap_sem etc...). Holding locking primitives local to the driver is ok, as long as they don't conflict with anything possible used by their own fence implementation.

Quoted 4 lines · Christian König

Drivers exporting fences need to provide a fence->signaled and a fence->wait function, everything else like fence->enable_signaling or calling fence_signaled() from the driver is optional.

Quoted 5 lines

Drivers wanting to use exported fences don't call fence->signaled or fence->wait in atomic or interrupt context, and not with holding any global locking primitives (like mmap_sem etc...). Holding locking primitives local to the driver is ok, as long as they don't conflict with anything possible used by their own fence implementation.

Well that's almost what we have right now with the exception that drivers are allowed (actually must for correctness when updating fences) the ww_mutexes for dma-bufs (or other buffer objects).

Locking
correctness is enforced with some extremely nasty lockdep annotations
+ additional debugging infrastructure enabled with CONFIG_DEBUG_WW_MUTEX_SLOWPATH. We really need to be able to hold dma-buf ww_mutexes while updating fences or waiting for them. And obviously for ->wait we need non-atomic context, not just non-interrupt.

Agreed that any shared locks are out of the way (especially stuff like dev->struct_mutex or other non-strictly driver-private stuff, i915 is really bad here still).

So from the core fence framework I think we already have exactly this, and we only need to adjust the radeon implementation a bit to make it less risky and invasive to the radeon driver logic.

-Daniel

Am 22.07.2014 17:42, schrieb Daniel Vetter:

Quoted 17 lines

Quoted 6 lines · Christian König

Drivers exporting fences need to provide a fence->signaled and a fence->wait
function, everything else like fence->enable_signaling or calling fence_signaled() from the driver is optional.

Quoted 8 lines

Drivers wanting to use exported fences don't call fence->signaled or fence->wait in atomic or interrupt context, and not with holding any global
locking primitives (like mmap_sem etc...). Holding locking primitives local
to the driver is ok, as long as they don't conflict with anything possible
used by their own fence implementation.

Quoted 3 lines

Well that's almost what we have right now with the exception that drivers are allowed (actually must for correctness when updating fences) the ww_mutexes for dma-bufs (or other buffer objects).

In this case sorry for so much noise. I really haven't looked in so much detail into anything but Maarten's Radeon patches.

But how does that then work right now? My impression was that it's mandatory for drivers to call fence_signaled()?

Quoted 7 lines

Locking
correctness is enforced with some extremely nasty lockdep annotations
+ additional debugging infrastructure enabled with CONFIG_DEBUG_WW_MUTEX_SLOWPATH. We really need to be able to hold dma-buf ww_mutexes while updating fences or waiting for them. And obviously for ->wait we need non-atomic context, not just non-interrupt.

Sounds mostly reasonable, but for holding the dma-buf ww_mutex, wouldn't be an RCU be more appropriate here? E.g. aren't we just interested that the current assigned fence at some point is signaled?

Something like grab ww_mutexes, grab a reference to the current fence object, release ww_mutex, wait for fence, release reference to the fence object.

Quoted 3 lines

Agreed that any shared locks are out of the way (especially stuff like dev->struct_mutex or other non-strictly driver-private stuff, i915 is really bad here still).

Yeah that's also an point I've wanted to note on Maartens patch. Radeon grabs the read side of it's exclusive semaphore while waiting for fences (because it assumes that the fence it waits for is a Radeon fence).

Assuming that we need to wait in both directions with Prime (e.g. Intel driver needs to wait for Radeon to finish rendering and Radeon needs to wait for Intel to finish displaying), this might become a perfect example of locking inversion.

Quoted 3 lines

So from the core fence framework I think we already have exactly this, and we only need to adjust the radeon implementation a bit to make it less risky and invasive to the radeon driver logic.

Agree. Well the biggest problem I see is that exclusive semaphore I need to take when anything calls into the driver. For the fence code I need to move that down into the fence->signaled handler, cause that now can be called from outside the driver.

Maarten solved this by telling the driver in the lockup handler (where we grab the write side of the exclusive lock) that all interrupts are already enabled, so that fence->signaled hopefully wouldn't mess with the hardware at all. While this probably works, it just leaves me with a feeling that we are doing something wrong here.

Maybe I've mixed things up a bit in my description. There is fence_signal which the implementor/exporter of a fence must call when the fence is completed. If the exporter has an ->enable_signaling callback it can delay that call to fence_signal for as long as it wishes as long as enable_signaling isn't called yet. But that's just the optimization to not required irqs to be turned on all the time.

The other function is fence_is_signaled, which is used by code that is interested in the fence state, together with fence_wait if it wants to block and not just wants to know the momentary fence state. All the other functions (the stuff that adds callbacks and the various _locked and other versions) are just for fancy special cases.

Quoted 21 lines · Christian König

Am 22.07.2014 17:42, schrieb Daniel Vetter:

Quoted 15 lines

Quoted 5 lines · Christian König

Drivers exporting fences need to provide a fence->signaled and a fence->wait
function, everything else like fence->enable_signaling or calling fence_signaled() from the driver is optional.

Quoted 8 lines

Drivers wanting to use exported fences don't call fence->signaled or fence->wait in atomic or interrupt context, and not with holding any global
locking primitives (like mmap_sem etc...). Holding locking primitives local
to the driver is ok, as long as they don't conflict with anything possible
used by their own fence implementation.

Quoted 3 lines

Well that's almost what we have right now with the exception that drivers are allowed (actually must for correctness when updating fences) the ww_mutexes for dma-bufs (or other buffer objects).

Quoted 5 lines

In this case sorry for so much noise. I really haven't looked in so much detail into anything but Maarten's Radeon patches.

But how does that then work right now? My impression was that it's mandatory for drivers to call fence_signaled()?

Maybe I've mixed things up a bit in my description. There is fence_signal which the implementor/exporter of a fence must call when the fence is completed. If the exporter has an ->enable_signaling callback it can delay that call to fence_signal for as long as it wishes as long as enable_signaling isn't called yet. But that's just the optimization to not required irqs to be turned on all the time.

The other function is fence_is_signaled, which is used by code that is interested in the fence state, together with fence_wait if it wants to block and not just wants to know the momentary fence state. All the other functions (the stuff that adds callbacks and the various _locked and other versions) are just for fancy special cases.

Quoted 11 lines

Quoted 7 lines

Locking
correctness is enforced with some extremely nasty lockdep annotations
+ additional debugging infrastructure enabled with CONFIG_DEBUG_WW_MUTEX_SLOWPATH. We really need to be able to hold dma-buf ww_mutexes while updating fences or waiting for them. And obviously for ->wait we need non-atomic context, not just non-interrupt.

Sounds mostly reasonable, but for holding the dma-buf ww_mutex, wouldn't be an RCU be more appropriate here? E.g. aren't we just interested that the current assigned fence at some point is signaled?

Yeah, as an optimization you can get the set of currently attached fences to a dma-buf with just rcu. But if you update the set of fences attached to a dma-buf (e.g. radeon blits the newly rendered frame to a dma-buf exported by i915 for scanout on i915) then you need a write lock on that buffer. Which is what the ww_mutex is for, to make sure that you don't deadlock with i915 doing concurrent ops on the same underlying buffer.

Quoted 3 lines

Something like grab ww_mutexes, grab a reference to the current fence object, release ww_mutex, wait for fence, release reference to the fence object.

Yeah, if the only thing you want to do is wait for fences, then the rcu-protected fence ref grabbing + lockless waiting is all you need. But e.g. in an execbuf you also need to update fences and maybe deep down in the reservation code you notice that you need to evict some stuff and so need to wait on some other guy to finish, and it's too complicated to drop and reacquire all the locks. Or you simply need to do a blocking wait on other gpus (because there's no direct hw sync mechanism) and again dropping locks would needlessly complicate the code. So I think we should allow this just to avoid too hairy/brittle (and almost definitely little tested code) in drivers.

Afaik this is also the same way ttm currently handles things wrt buffer reservation and eviction.

Quoted 4 lines

Quoted 3 lines

Agreed that any shared locks are out of the way (especially stuff like dev->struct_mutex or other non-strictly driver-private stuff, i915 is really bad here still).

Quoted 8 lines

Yeah that's also an point I've wanted to note on Maartens patch. Radeon grabs the read side of it's exclusive semaphore while waiting for fences (because it assumes that the fence it waits for is a Radeon fence).

Assuming that we need to wait in both directions with Prime (e.g. Intel driver needs to wait for Radeon to finish rendering and Radeon needs to wait for Intel to finish displaying), this might become a perfect example of locking inversion.

fence updates are atomic on a dma-buf, protected by ww_mutex. The neat trick of ww_mutex is that they enforce a global ordering, so in your scenario either i915 or radeon would be first and you can't deadlock. There is no way to interleave anything even if you have lots of buffers shared between i915/radeon. Wrt deadlocking it's exactly the same guarantees as the magic ttm provides for just one driver with concurrent command submission since it's the same idea.

Quoted 9 lines

Quoted 3 lines

So from the core fence framework I think we already have exactly this, and we only need to adjust the radeon implementation a bit to make it less risky and invasive to the radeon driver logic.

Agree. Well the biggest problem I see is that exclusive semaphore I need to take when anything calls into the driver. For the fence code I need to move that down into the fence->signaled handler, cause that now can be called from outside the driver.

Quoted 5 lines

Maarten solved this by telling the driver in the lockup handler (where we grab the write side of the exclusive lock) that all interrupts are already enabled, so that fence->signaled hopefully wouldn't mess with the hardware at all. While this probably works, it just leaves me with a feeling that we are doing something wrong here.

I'm not versed on the details in readon, but on i915 we can attach a memory location and cookie value to each fence and just do a memory fetch to figure out whether the fence has passed or not. So no locking needed at all. Of course the fence itself needs to lock a reference onto that memory location, which is a neat piece of integration work that we still need to tackle in some cases - there's conflicting patch series all over this ;-)

But like I've said fence->signaled is optional so you don't need this necessarily, as long as radeon eventually calls fence_signaled once the fence has completed.
-Daniel

Quoted 7 lines

Maybe I've mixed things up a bit in my description. There is fence_signal which the implementor/exporter of a fence must call when the fence is completed. If the exporter has an ->enable_signaling callback it can delay that call to fence_signal for as long as it wishes as long as enable_signaling isn't called yet. But that's just the optimization to not required irqs to be turned on all the time.

Quoted 5 lines

The other function is fence_is_signaled, which is used by code that is interested in the fence state, together with fence_wait if it wants to block and not just wants to know the momentary fence state. All the other functions (the stuff that adds callbacks and the various _locked and other versions) are just for fancy special cases.

Well that's rather bad, cause IRQs aren't reliable enough on Radeon HW for such a thing. Especially on Prime systems and Macs.

That's why we have this fancy HZ/2 timeout on all fence wait operations to manually check if the fence is signaled or not.

To guarantee that a fence is signaled after enable_signaling is called we would need to fire up a kernel thread which periodically calls fence->signaled.

But like I've said fence->signaled is optional so you don't need this necessarily, as long as radeon eventually calls fence_signaled once the fence has completed.

Am 22.07.2014 17:42, schrieb Daniel Vetter:

Quoted 11 lines

Quoted 9 lines · Christian König

Drivers exporting fences need to provide a fence->signaled and a fence->wait function, everything else like fence->enable_signaling or calling fence_signaled() from the driver is optional.

Drivers wanting to use exported fences don't call fence->signaled or fence->wait in atomic or interrupt context, and not with holding any global locking primitives (like mmap_sem etc...). Holding locking primitives local to the driver is ok, as long as they don't conflict with anything possible used by their own fence implementation.

Quoted 3 lines

Well that's almost what we have right now with the exception that drivers are allowed (actually must for correctness when updating fences) the ww_mutexes for dma-bufs (or other buffer objects).

In this case sorry for so much noise. I really haven't looked in so much detail into anything but Maarten's Radeon patches.

But how does that then work right now? My impression was that it's mandatory for drivers to call fence_signaled()?

Quoted 7 lines

Locking
correctness is enforced with some extremely nasty lockdep annotations
+ additional debugging infrastructure enabled with CONFIG_DEBUG_WW_MUTEX_SLOWPATH. We really need to be able to hold dma-buf ww_mutexes while updating fences or waiting for them. And obviously for ->wait we need non-atomic context, not just non-interrupt.

Sounds mostly reasonable, but for holding the dma-buf ww_mutex, wouldn't be an RCU be more appropriate here? E.g. aren't we just interested that the current assigned fence at some point is signaled?

Something like grab ww_mutexes, grab a reference to the current fence object, release ww_mutex, wait for fence, release reference to the fence object.

Quoted 4 lines

Agreed that any shared locks are out of the way (especially stuff like dev->struct_mutex or other non-strictly driver-private stuff, i915 is really bad here still).

Yeah that's also an point I've wanted to note on Maartens patch. Radeon grabs the read side of it's exclusive semaphore while waiting for fences (because it assumes that the fence it waits for is a Radeon fence).

Assuming that we need to wait in both directions with Prime (e.g. Intel driver needs to wait for Radeon to finish rendering and Radeon needs to wait for Intel to finish displaying), this might become a perfect example of locking inversion.

Quoted 3 lines

So from the core fence framework I think we already have exactly this, and we only need to adjust the radeon implementation a bit to make it less risky and invasive to the radeon driver logic.

Agree. Well the biggest problem I see is that exclusive semaphore I need to take when anything calls into the driver. For the fence code I need to move that down into the fence->signaled handler, cause that now can be called from outside the driver.

Maarten solved this by telling the driver in the lockup handler (where we grab the write side of the exclusive lock) that all interrupts are already enabled, so that fence->signaled hopefully wouldn't mess with the hardware at all. While this probably works, it just leaves me with a feeling that we are doing something wrong here.

op 22-07-14 17:59, Christian König schreef:

Quoted 12 lines

Am 22.07.2014 17:42, schrieb Daniel Vetter:

Quoted 11 lines

Quoted 9 lines · Christian König

Drivers exporting fences need to provide a fence->signaled and a fence->wait function, everything else like fence->enable_signaling or calling fence_signaled() from the driver is optional.

Drivers wanting to use exported fences don't call fence->signaled or fence->wait in atomic or interrupt context, and not with holding any global locking primitives (like mmap_sem etc...). Holding locking primitives local to the driver is ok, as long as they don't conflict with anything possible used by their own fence implementation.

Quoted 6 lines

Quoted 3 lines

Well that's almost what we have right now with the exception that drivers are allowed (actually must for correctness when updating fences) the ww_mutexes for dma-bufs (or other buffer objects).

In this case sorry for so much noise. I really haven't looked in so much detail into anything but Maarten's Radeon patches.

But how does that then work right now? My impression was that it's mandatory for drivers to call fence_signaled()?

It's only mandatory to call fence_signal() if the .enable_signaling callback has been called, else you can get away with never calling signaling a fence at all before dropping the last refcount to it. This allows you to keep interrupts disabled when you don't need them.

Quoted 6 lines

Quoted 3 lines

Agreed that any shared locks are out of the way (especially stuff like dev->struct_mutex or other non-strictly driver-private stuff, i915 is really bad here still).

Yeah that's also an point I've wanted to note on Maartens patch. Radeon grabs the read side of it's exclusive semaphore while waiting for fences (because it assumes that the fence it waits for is a Radeon fence).

Assuming that we need to wait in both directions with Prime (e.g. Intel driver needs to wait for Radeon to finish rendering and Radeon needs to wait for Intel to finish displaying), this might become a perfect example of locking inversion.

In the preliminary patches where I can sync radeon with other GPU's I've been very careful in all the places that call into fences, to make sure that radeon wouldn't try to handle lockups for a different (possibly also radeon) card.

This is also why fence_is_signaled should never block, and why it trylocks the exclusive_lock. :-) I think lockdep would complain if I grabbed exclusive_lock while blocking in is_signaled.

Quoted 3 lines

Quoted 3 lines

So from the core fence framework I think we already have exactly this, and we only need to adjust the radeon implementation a bit to make it less risky and invasive to the radeon driver logic.

Quoted 3 lines

Agree. Well the biggest problem I see is that exclusive semaphore I need to take when anything calls into the driver. For the fence code I need to move that down into the fence->signaled handler, cause that now can be called from outside the driver.

Maarten solved this by telling the driver in the lockup handler (where we grab the write side of the exclusive lock) that all interrupts are already enabled, so that fence->signaled hopefully wouldn't mess with the hardware at all. While this probably works, it just leaves me with a feeling that we are doing something wrong here.

There is unfortunately no global mechanism to say 'this card is locked up, please don't call into any of my fences', and I don't associate fences with devices, and radeon doesn't keep a global list of fences. If all of that existed, it would complicate the interface and its callers a lot, while I like to keep things simple.
So I did the best I could, and simply prevented the fence calls from fiddling with the hardware. Fortunately gpu lockup is not a common operation. :-)

~Maarten

Quoted 4 lines

In the preliminary patches where I can sync radeon with other GPU's I've been very careful in all the places that call into fences, to make sure that radeon wouldn't try to handle lockups for a different (possibly also radeon) card.

That's actually not such a good idea.

In case of a lockup we need to handle the lockup cause otherwise it could happen that radeon waits for the lockup to be resolved and the lockup handling needs to wait for a fence that's never signaled because of the lockup.

Am 23.07.2014 08:40, schrieb Maarten Lankhorst:

Quoted 16 lines

op 22-07-14 17:59, Christian König schreef:

Quoted 15 lines

Am 22.07.2014 17:42, schrieb Daniel Vetter:

Quoted 14 lines

Quoted 9 lines · Christian König

Drivers exporting fences need to provide a fence->signaled and a fence->wait function, everything else like fence->enable_signaling or calling fence_signaled() from the driver is optional.

Drivers wanting to use exported fences don't call fence->signaled or fence->wait in atomic or interrupt context, and not with holding any global locking primitives (like mmap_sem etc...). Holding locking primitives local to the driver is ok, as long as they don't conflict with anything possible used by their own fence implementation.

Well that's almost what we have right now with the exception that drivers are allowed (actually must for correctness when updating fences) the ww_mutexes for dma-bufs (or other buffer objects).

Quoted 5 lines

Quoted 3 lines

In this case sorry for so much noise. I really haven't looked in so much detail into anything but Maarten's Radeon patches.

But how does that then work right now? My impression was that it's mandatory for drivers to call fence_signaled()?

It's only mandatory to call fence_signal() if the .enable_signaling callback has been called, else you can get away with never calling signaling a fence at all before dropping the last refcount to it. This allows you to keep interrupts disabled when you don't need them.

Can we somehow avoid the need to call fence_signal() at all? The interrupts at least on radeon are way to unreliable for such a thing. Can enable_signalling fail? What's the reason for fence_signaled() in the first place?

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

Agreed that any shared locks are out of the way (especially stuff like dev->struct_mutex or other non-strictly driver-private stuff, i915 is really bad here still).

Yeah that's also an point I've wanted to note on Maartens patch. Radeon grabs the read side of it's exclusive semaphore while waiting for fences (because it assumes that the fence it waits for is a Radeon fence).

Assuming that we need to wait in both directions with Prime (e.g. Intel driver needs to wait for Radeon to finish rendering and Radeon needs to wait for Intel to finish displaying), this might become a perfect example of locking inversion.

In the preliminary patches where I can sync radeon with other GPU's I've been very careful in all the places that call into fences, to make sure that radeon wouldn't try to handle lockups for a different (possibly also radeon) card.

That's actually not such a good idea.

In case of a lockup we need to handle the lockup cause otherwise it could happen that radeon waits for the lockup to be resolved and the lockup handling needs to wait for a fence that's never signaled because of the lockup.

Quoted 1 line

Can we somehow avoid the need to call fence_signal() at all? The interrupts at least on radeon are way to unreliable for such a thing. Can enable_signalling fail? What's the reason for fence_signaled() in the first place?

It doesn't need to be completely reliable, or finish immediately.

And any time wake_up_all(&rdev->fence_queue) is called all the fences that were enabled will be rechecked.

Quoted 4 lines · Maarten Lankhorst

Quoted 1 line

Can we somehow avoid the need to call fence_signal() at all? The interrupts at least on radeon are way to unreliable for such a thing. Can enable_signalling fail? What's the reason for fence_signaled() in the first place?

It doesn't need to be completely reliable, or finish immediately.

And any time wake_up_all(&rdev->fence_queue) is called all the fences that were enabled will be rechecked.

I raised this already somewhere else, but should we have some common infrastructure in the core fence code to recheck fences periodically? radeon doesn't seem to be the only hw where this isn't reliable enough. Of course timer-based rechecking would only work if the driver provides the fence->signalled callback to recheck actual fence state.

-Daniel

Am 23.07.2014 09:09, schrieb Daniel Vetter:

Quoted 11 lines

Quoted 4 lines · Maarten Lankhorst

Quoted 1 line

Can we somehow avoid the need to call fence_signal() at all? The interrupts at least on radeon are way to unreliable for such a thing. Can enable_signalling fail? What's the reason for fence_signaled() in the first place?

It doesn't need to be completely reliable, or finish immediately.

And any time wake_up_all(&rdev->fence_queue) is called all the fences that were enabled will be rechecked.

I raised this already somewhere else, but should we have some common infrastructure in the core fence code to recheck fences periodically? radeon doesn't seem to be the only hw where this isn't reliable enough. Of course timer-based rechecking would only work if the driver provides the fence->signalled callback to recheck actual fence state.

Yeah, agree. The proposal won't work reliable at all with radeon.

Interrupts are accumulated before sending them to the CPU, e.g. you can get one interrupt for multiple fences finished. If it's just the interrupt for the last fence submitted that gets lost you are completely screwed up because you won't get another interrupt.

I had that problem multiple times while working on UVD support, resulting in the driver thinking that it can't submit more jobs because non of the interrupts for the already submitted fence cam through.

Apart from that interrupts on Macs usually don't work at all, so we really need a solution where calling fence_signaled() is completely optional.

op 23-07-14 09:15, Christian König schreef:

Quoted 12 lines

Am 23.07.2014 09:09, schrieb Daniel Vetter:

Quoted 11 lines

Quoted 4 lines · Maarten Lankhorst

Quoted 1 line

Can we somehow avoid the need to call fence_signal() at all? The interrupts at least on radeon are way to unreliable for such a thing. Can enable_signalling fail? What's the reason for fence_signaled() in the first place?

It doesn't need to be completely reliable, or finish immediately.

And any time wake_up_all(&rdev->fence_queue) is called all the fences that were enabled will be rechecked.

I raised this already somewhere else, but should we have some common infrastructure in the core fence code to recheck fences periodically? radeon doesn't seem to be the only hw where this isn't reliable enough. Of course timer-based rechecking would only work if the driver provides the fence->signalled callback to recheck actual fence state.

Quoted 5 lines

Yeah, agree. The proposal won't work reliable at all with radeon.

Interrupts are accumulated before sending them to the CPU, e.g. you can get one interrupt for multiple fences finished. If it's just the interrupt for the last fence submitted that gets lost you are completely screwed up because you won't get another interrupt.

I had that problem multiple times while working on UVD support, resulting in the driver thinking that it can't submit more jobs because non of the interrupts for the already submitted fence cam through.

Yeah but all the fences that have .enable_signaling will get signaled from a single interrupt, or when any waiter calls radeon_fence_process.

Quoted 2 lines

Apart from that interrupts on Macs usually don't work at all, so we really need a solution where calling fence_signaled() is completely optional.

I haven't had a problem with interrupts on my mbp after d1f9809ed1315c4cdc5760cf2f59626fd3276952, but it should be trivial to start a timer that periodically does wake_up_all, and gets its timeout reset in a call to radeon_fence_process. It could either be added as a work item, or as a normal timer (disabled during gpu lockup recovery to prevent checks from fiddling with things it shouldn't).

~Maarten

op 23-07-14 08:52, Christian König schreef:

Quoted 17 lines

Am 23.07.2014 08:40, schrieb Maarten Lankhorst:

Quoted 16 lines

op 22-07-14 17:59, Christian König schreef:

Quoted 15 lines

Am 22.07.2014 17:42, schrieb Daniel Vetter:

Quoted 14 lines

Quoted 9 lines · Christian König

Drivers exporting fences need to provide a fence->signaled and a fence->wait function, everything else like fence->enable_signaling or calling fence_signaled() from the driver is optional.

Drivers wanting to use exported fences don't call fence->signaled or fence->wait in atomic or interrupt context, and not with holding any global locking primitives (like mmap_sem etc...). Holding locking primitives local to the driver is ok, as long as they don't conflict with anything possible used by their own fence implementation.

Well that's almost what we have right now with the exception that drivers are allowed (actually must for correctness when updating fences) the ww_mutexes for dma-bufs (or other buffer objects).

Quoted 6 lines

Quoted 5 lines

Quoted 3 lines

In this case sorry for so much noise. I really haven't looked in so much detail into anything but Maarten's Radeon patches.

But how does that then work right now? My impression was that it's mandatory for drivers to call fence_signaled()?

It's only mandatory to call fence_signal() if the .enable_signaling callback has been called, else you can get away with never calling signaling a fence at all before dropping the last refcount to it. This allows you to keep interrupts disabled when you don't need them.

Can we somehow avoid the need to call fence_signal() at all? The interrupts at least on radeon are way to unreliable for such a thing. Can enable_signalling fail? What's the reason for fence_signaled() in the first place?

It doesn't need to be completely reliable, or finish immediately.

And any time wake_up_all(&rdev->fence_queue) is called all the fences that were enabled will be rechecked.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

Agreed that any shared locks are out of the way (especially stuff like dev->struct_mutex or other non-strictly driver-private stuff, i915 is really bad here still).

Yeah that's also an point I've wanted to note on Maartens patch. Radeon grabs the read side of it's exclusive semaphore while waiting for fences (because it assumes that the fence it waits for is a Radeon fence).

Quoted 5 lines

Quoted 2 lines

Quoted 1 line

Assuming that we need to wait in both directions with Prime (e.g. Intel driver needs to wait for Radeon to finish rendering and Radeon needs to wait for Intel to finish displaying), this might become a perfect example of locking inversion.

In the preliminary patches where I can sync radeon with other GPU's I've been very careful in all the places that call into fences, to make sure that radeon wouldn't try to handle lockups for a different (possibly also radeon) card.

That's actually not such a good idea.

In case of a lockup we need to handle the lockup cause otherwise it could happen that radeon waits for the lockup to be resolved and the lockup handling needs to wait for a fence that's never signaled because of the lockup.

The lockup handling calls radeon_fence_wait, not the generic fence_wait. It doesn't call the exported wait function that takes the exclusive_lock in read mode. And lockdep should have complained if I screwed that up. :-)

~Maarten

It's not a locking problem I'm talking about here. Radeons lockup handling kicks in when anything calls into the driver from the outside, if you have a fence wait function that's called from the outside but doesn't handle lockups you essentially rely on somebody else calling another radeon function for the lockup to be resolved.

Quoted 5 lines · Christian König

It's not a locking problem I'm talking about here. Radeons lockup handling kicks in when anything calls into the driver from the outside, if you have a fence wait function that's called from the outside but doesn't handle lockups you essentially rely on somebody else calling another radeon function for the lockup to be resolved.

So you don't have a timer in radeon that periodically checks whether progress is still being made? That's the approach we're using in i915, together with some tricks to kick any stuck waiters so that we can reliably step in and grab locks for the reset.

-Daniel

Am 23.07.2014 09:31, schrieb Daniel Vetter:

Quoted 7 lines

Quoted 5 lines · Christian König

It's not a locking problem I'm talking about here. Radeons lockup handling kicks in when anything calls into the driver from the outside, if you have a fence wait function that's called from the outside but doesn't handle lockups you essentially rely on somebody else calling another radeon function for the lockup to be resolved.

Quoted 4 lines

So you don't have a timer in radeon that periodically checks whether progress is still being made? That's the approach we're using in i915, together with some tricks to kick any stuck waiters so that we can reliably step in and grab locks for the reset.

We tried this approach, but it didn't worked at all.

I already considered trying it again because of the upcoming fence implementation, but reconsidering that when a driver is forced to change it's handling because of the fence implementation that's just another hint that there is something wrong here.

Regardless of the fence implementation, why would it be a good idea to do a full lockup recovery when some other driver is calling your wait function? That doesn't seem to be a nice thing to do, so I think a timeout is the best error you could return here, other drivers have to deal with that anyway.

op 23-07-14 09:37, Christian König schreef:

Quoted 8 lines

Am 23.07.2014 09:31, schrieb Daniel Vetter:

Quoted 7 lines

Quoted 5 lines · Christian König

It's not a locking problem I'm talking about here. Radeons lockup handling kicks in when anything calls into the driver from the outside, if you have a fence wait function that's called from the outside but doesn't handle lockups you essentially rely on somebody else calling another radeon function for the lockup to be resolved.

Quoted 7 lines

Quoted 4 lines

So you don't have a timer in radeon that periodically checks whether progress is still being made? That's the approach we're using in i915, together with some tricks to kick any stuck waiters so that we can reliably step in and grab locks for the reset.

We tried this approach, but it didn't worked at all.

I already considered trying it again because of the upcoming fence implementation, but reconsidering that when a driver is forced to change it's handling because of the fence implementation that's just another hint that there is something wrong here.

As far as I can tell it wouldn't need to be reworked for the fence implementation currently, only the moment you want to allow callers outside of radeon. :-) Doing a GPU lockup recovery in the wait function would be messy even right now, you would hit a deadlock in ttm_bo_delayed_delete -> ttm_bo_cleanup_refs_and_unlock.

Regardless of the fence implementation, why would it be a good idea to do a full lockup recovery when some other driver is calling your wait function? That doesn't seem to be a nice thing to do, so I think a timeout is the best error you could return here, other drivers have to deal with that anyway.

~Maarten

Just imagine an application using prime is locking up Radeon and because of that gets killed by the user. Nothing else in the system would use the Radeon hardware any more and so radeon gets only called by another driver waiting patiently for radeon to finish rendering which never happens because the whole thing is locked up and we don't get a chance to recover.

Quoted 5 lines · Christian König

Just imagine an application using prime is locking up Radeon and because of that gets killed by the user. Nothing else in the system would use the Radeon hardware any more and so radeon gets only called by another driver waiting patiently for radeon to finish rendering which never happens because the whole thing is locked up and we don't get a chance to recover.

But isn't that possible already without fences? X hangs radeon, user crashes X for unrelated reasons before radeon will notice the hang. Then no one uses radeon any longer and the hang stays undetected.

-Daniel