Skip to contentSkip to conversation

drm/radeon: take exclusive_lock in read mode during ring tests

PATCH Superseded Maarten Lankhorst 3 participants 13 messages Aug 18, 2014

Subsystems (2)
DRM drivers Radeon and amdgpu DRM drivers

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Maarten Lankhorst

author
#0 lore raw

PATCH 1/3 drm/radeon: take exclusive_lock in read mode during ring tests

This is needed for the next commit, because the lockup detection will need the read lock to run.

Signed-off-by Maarten Lankhorst <[email protected]>
---
 drivers/gpu/drm/radeon/radeon.h        |  2 +-
 drivers/gpu/drm/radeon/radeon_device.c | 61 ++++++++++++++++++++--------------
 2 files changed, 37 insertions(+), 26 deletions(-)
drivers/gpu/drm/radeon/radeon.h +1 -1
index 9e1732eb402c..1d806983ec7b 100644
@@ -2312,7 +2312,7 @@ struct radeon_device {
 	bool				need_dma32;
 	bool				accel_working;
 	bool				fastfb_working; /* IGP feature*/
-	bool				needs_reset;
+	bool				needs_reset, in_reset;
 	struct radeon_surface_reg surface_regs[RADEON_GEM_MAX_SURFACES];
 	const struct firmware *me_fw;	/* all family ME firmware */
 	const struct firmware *pfp_fw;	/* r6/700 PFP firmware */
drivers/gpu/drm/radeon/radeon_device.c +36 -25

105 lines not on this page · raw message

Signature · 1 line
2.0.4
author
#1 lore raw

PATCH 2/3 drm/radeon: handle lockup in delayed work, v3

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

---
V1 had a nasty bug breaking gpu lockup recovery. The fix is not allowing radeon_fence_driver_check_lockup to take exclusive_lock, and kill it during lockup recovery instead.
V2 used delayed work that ran during lockup recovery, but required read lock. I've fixed this by downgrading the write, and retrying if recovery fails.

---
 drivers/gpu/drm/radeon/radeon.h       |   2 +
 drivers/gpu/drm/radeon/radeon_fence.c | 115 +++++++++++++++++-----------------
 2 files changed, 61 insertions(+), 56 deletions(-)
drivers/gpu/drm/radeon/radeon.h +2 -0
index 1d806983ec7b..29504efe8971 100644
@@ -355,6 +355,8 @@ struct radeon_fence_driver {
 	uint64_t			sync_seq[RADEON_NUM_RINGS];
 	atomic64_t			last_seq;
 	bool				initialized;
+	struct delayed_work		fence_check_work;
+	struct radeon_device		*rdev;
 };
 
 struct radeon_fence {
drivers/gpu/drm/radeon/radeon_fence.c +59 -56

164 lines not on this page · raw message

Signature · 1 line
2.0.4
author
#5 lore raw

PATCH 3/3 drm/radeon: add timeout argument to radeon_fence_wait_seq

In reply to Maarten Lankhorst

This makes it possible to wait for a specific amount of time, rather than wait until infinity.

Signed-off-by Maarten Lankhorst <[email protected]>
Reviewed-by Christian König <[email protected]>
---
 drivers/gpu/drm/radeon/radeon_fence.c | 50 +++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 20 deletions(-)
drivers/gpu/drm/radeon/radeon_fence.c +30 -20
index 94eca53d99f8..e8ea423648ab 100644
@@ -320,22 +320,25 @@ static bool radeon_fence_any_seq_signaled(struct radeon_device *rdev, u64 *seq)
 }
 
 /**
- * radeon_fence_wait_seq - wait for a specific sequence numbers
+ * radeon_fence_wait_seq_timeout - wait for a specific sequence numbers
  *
  * @rdev: radeon device pointer
  * @target_seq: sequence number(s) we want to wait for
  * @intr: use interruptable sleep
+ * @timeout: maximum time to wait, or MAX_SCHEDULE_TIMEOUT for infinite wait
  *
  * Wait for the requested sequence number(s) to be written by any ring
  * (all asics).  Sequnce number array is indexed by ring id.
  * @intr selects whether to use interruptable (true) or non-interruptable
  * (false) sleep when waiting for the sequence number.  Helper function
  * for radeon_fence_wait_*().
- * Returns 0 if the sequence number has passed, error for all other cases.
+ * Returns remaining time if the sequence number has passed, 0 when
+ * the wait timeout, or an error for all other cases.
  * -EDEADLK is returned when a GPU lockup has been detected.
  */
-static int radeon_fence_wait_seq(struct radeon_device *rdev, u64 *target_seq,
-				 bool intr)
+static long radeon_fence_wait_seq_timeout(struct radeon_device *rdev,
+					  u64 *target_seq, bool intr,
+					  long timeout)
 {
 	uint64_t last_seq[RADEON_NUM_RINGS];
 	bool signaled;

@@ -357,11 +360,11 @@ static int radeon_fence_wait_seq(struct radeon_device *rdev, u64 *target_seq,
 		if (intr) {
 			r = wait_event_interruptible_timeout(rdev->fence_queue, (
 				(signaled = radeon_fence_any_seq_signaled(rdev, target_seq))
-				 || rdev->needs_reset), MAX_SCHEDULE_TIMEOUT);
+				 || rdev->needs_reset), timeout);
 		} else {
 			r = wait_event_timeout(rdev->fence_queue, (
 				(signaled = radeon_fence_any_seq_signaled(rdev, target_seq))
-				 || rdev->needs_reset), MAX_SCHEDULE_TIMEOUT);
+				 || rdev->needs_reset), timeout);
 		}
 
 		for (i = 0; i < RADEON_NUM_RINGS; ++i) {

@@ -372,20 +375,22 @@ static int radeon_fence_wait_seq(struct radeon_device *rdev, u64 *target_seq,
 			trace_radeon_fence_wait_end(rdev->ddev, i, target_seq[i]);
 		}
 
-		if (r < 0)
+		if (r <= 0)
 			return r;
 
 		if (rdev->needs_reset)
 			return -EDEADLK;
+
+		timeout = r;
 	}
-	return 0;
+	return timeout;
 }
 
 /**
  * radeon_fence_wait - wait for a fence to signal
  *
  * @fence: radeon fence object
- * @intr: use interruptable sleep
+ * @intr: use interruptible sleep
  *
  * Wait for the requested fence to signal (all asics).
  * @intr selects whether to use interruptable (true) or non-interruptable

@@ -395,7 +400,7 @@ static int radeon_fence_wait_seq(struct radeon_device *rdev, u64 *target_seq,
 int radeon_fence_wait(struct radeon_fence *fence, bool intr)
 {
 	uint64_t seq[RADEON_NUM_RINGS] = {};
-	int r;
+	long r;
 
 	if (fence == NULL) {
 		WARN(1, "Querying an invalid fence : %p !\n", fence);

@@ -406,9 +411,10 @@ int radeon_fence_wait(struct radeon_fence *fence, bool intr)
 	if (seq[fence->ring] == RADEON_FENCE_SIGNALED_SEQ)
 		return 0;
 
-	r = radeon_fence_wait_seq(fence->rdev, seq, intr);
-	if (r)
+	r = radeon_fence_wait_seq_timeout(fence->rdev, seq, intr, MAX_SCHEDULE_TIMEOUT);
+	if (r < 0) {
 		return r;
+	}
 
 	fence->seq = RADEON_FENCE_SIGNALED_SEQ;
 	return 0;

@@ -433,7 +439,7 @@ int radeon_fence_wait_any(struct radeon_device *rdev,
 {
 	uint64_t seq[RADEON_NUM_RINGS];
 	unsigned i, num_rings = 0;
-	int r;
+	long r;
 
 	for (i = 0; i < RADEON_NUM_RINGS; ++i) {
 		seq[i] = 0;

@@ -454,8 +460,8 @@ int radeon_fence_wait_any(struct radeon_device *rdev,
 	if (num_rings == 0)
 		return -ENOENT;
 
-	r = radeon_fence_wait_seq(rdev, seq, intr);
-	if (r) {
+	r = radeon_fence_wait_seq_timeout(rdev, seq, intr, MAX_SCHEDULE_TIMEOUT);
+	if (r < 0) {
 		return r;
 	}
 	return 0;

@@ -474,6 +480,7 @@ int radeon_fence_wait_any(struct radeon_device *rdev,
 int radeon_fence_wait_next(struct radeon_device *rdev, int ring)
 {
 	uint64_t seq[RADEON_NUM_RINGS] = {};
+	long r;
 
 	seq[ring] = atomic64_read(&rdev->fence_drv[ring].last_seq) + 1ULL;
 	if (seq[ring] >= rdev->fence_drv[ring].sync_seq[ring]) {

@@ -481,7 +488,10 @@ int radeon_fence_wait_next(struct radeon_device *rdev, int ring)
 		   already the last emited fence */
 		return -ENOENT;
 	}
-	return radeon_fence_wait_seq(rdev, seq, false);
+	r = radeon_fence_wait_seq_timeout(rdev, seq, false, MAX_SCHEDULE_TIMEOUT);
+	if (r < 0)
+		return r;
+	return 0;
 }
 
 /**

@@ -497,18 +507,18 @@ int radeon_fence_wait_next(struct radeon_device *rdev, int ring)
 int radeon_fence_wait_empty(struct radeon_device *rdev, int ring)
 {
 	uint64_t seq[RADEON_NUM_RINGS] = {};
-	int r;
+	long r;
 
 	seq[ring] = rdev->fence_drv[ring].sync_seq[ring];
 	if (!seq[ring])
 		return 0;
 
-	r = radeon_fence_wait_seq(rdev, seq, false);
-	if (r) {
+	r = radeon_fence_wait_seq_timeout(rdev, seq, false, MAX_SCHEDULE_TIMEOUT);
+	if (r < 0) {
 		if (r == -EDEADLK)
 			return -EDEADLK;
 
-		dev_err(rdev->dev, "error waiting for ring[%d] to become idle (%d)\n",
+		dev_err(rdev->dev, "error waiting for ring[%d] to become idle (%ld)\n",
 			ring, r);
 	}
 	return 0;
Signature · 7 lines
2.0.4


_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
#6 lore raw
In reply to Maarten Lankhorst

Am 18.08.2014 um 16:45 schrieb Maarten Lankhorst:

Quoted 47 lines

This is needed for the next commit, because the lockup detection will need the read lock to run.

Signed-off-by Maarten Lankhorst <[email protected]>
---
  drivers/gpu/drm/radeon/radeon.h        |  2 +-
  drivers/gpu/drm/radeon/radeon_device.c | 61 ++++++++++++++++++++--------------
  2 files changed, 37 insertions(+), 26 deletions(-)
drivers/gpu/drm/radeon/radeon.h +1 -1
index 9e1732eb402c..1d806983ec7b 100644
@@ -2312,7 +2312,7 @@ struct radeon_device {
  	bool				need_dma32;
  	bool				accel_working;
  	bool				fastfb_working; /* IGP feature*/
-	bool				needs_reset;
+	bool				needs_reset, in_reset;
  	struct radeon_surface_reg surface_regs[RADEON_GEM_MAX_SURFACES];
  	const struct firmware *me_fw;	/* all family ME firmware */
  	const struct firmware *pfp_fw;	/* r6/700 PFP firmware */
drivers/gpu/drm/radeon/radeon_device.c +1 -9
index c8ea050c8fa4..82633fdd399d 100644
@@ -1671,29 +1671,34 @@ int radeon_gpu_reset(struct radeon_device *rdev)
  	down_write(&rdev->exclusive_lock);
 
  	if (!rdev->needs_reset) {
+		WARN_ON(rdev->in_reset);
  		up_write(&rdev->exclusive_lock);
  		return 0;
  	}
 
  	rdev->needs_reset = false;
-
-	radeon_save_bios_scratch_regs(rdev);
-	/* block TTM */
  	resched = ttm_bo_lock_delayed_workqueue(&rdev->mman.bdev);
-	radeon_pm_suspend(rdev);
-	radeon_suspend(rdev);
 
-	for (i = 0; i < RADEON_NUM_RINGS; ++i) {
-		ring_sizes[i] = radeon_ring_backup(rdev, &rdev->ring[i],
-						   &ring_data[i]);
-		if (ring_sizes[i]) {

drivers/gpu/drm/radeon/radeon_device.c int radeon_gpu_reset(struct radeon_device *rdev) · line 1688

Quoted 10 lines
drivers/gpu/drm/radeon/radeon_device.c +7 -3
-			saved = true;
-			dev_info(rdev->dev, "Saved %d dwords of commands "
-				 "on ring %d.\n", ring_sizes[i], i);
+	if (!rdev->in_reset) {
+		rdev->in_reset = true;
+
+		radeon_save_bios_scratch_regs(rdev);
+		/* block TTM */
+		radeon_pm_suspend(rdev);
+		radeon_suspend(rdev);

That won't work correctly because you might end up with calling pm_resume more often than suspend and that can only lead to a crash. Saying this we probably already have a bug in the reset code at this point anyway, but see below.

Quoted 19 lines
drivers/gpu/drm/radeon/radeon_device.c +11 -2
+
+		for (i = 0; i < RADEON_NUM_RINGS; ++i) {
+			ring_sizes[i] = radeon_ring_backup(rdev, &rdev->ring[i],
+							   &ring_data[i]);
+			if (ring_sizes[i]) {
+				saved = true;
+				dev_info(rdev->dev, "Saved %d dwords of commands "
+					 "on ring %d.\n", ring_sizes[i], i);
+			}
  		}
-	}
+	} else
+		memset(ring_data, 0, sizeof(ring_data));
 
-retry:
  	r = radeon_asic_reset(rdev);
  	if (!r) {
  		dev_info(rdev->dev, "GPU reset succeeded, trying to resume\n");

drivers/gpu/drm/radeon/radeon_device.c retry: · line 1708

Quoted 2 lines
drivers/gpu/drm/radeon/radeon_device.c +0 -0
@@ -1702,40 +1707,46 @@ retry:
 
  	radeon_restore_bios_scratch_regs(rdev);

We should resume PM here as well.

Quoted 8 lines
drivers/gpu/drm/radeon/radeon_device.c +1 -2
-	if (!r) {
+	if (!r && saved) {
  		for (i = 0; i < RADEON_NUM_RINGS; ++i) {
  			radeon_ring_restore(rdev, &rdev->ring[i],
  					    ring_sizes[i], ring_data[i]);
-			ring_sizes[i] = 0;
  			ring_data[i] = NULL;

drivers/gpu/drm/radeon/radeon_device.c retry: · line 1724

Quoted 10 lines
drivers/gpu/drm/radeon/radeon_device.c +9 -0
  		}
+	} else {
+		radeon_fence_driver_force_completion(rdev);
+
+		for (i = 0; i < RADEON_NUM_RINGS; ++i) {
+			kfree(ring_data[i]);
+		}
+	}
+	downgrade_write(&rdev->exclusive_lock);
+	ttm_bo_unlock_delayed_workqueue(&rdev->mman.bdev, resched);

I would unlock the delayed_workqueue first and then downgrade the readlock.

Quoted 14 lines
drivers/gpu/drm/radeon/radeon_device.c +5 -2
+	if (!r) {
  		r = radeon_ib_ring_tests(rdev);
  		if (r) {
  			dev_err(rdev->dev, "ib ring test failed (%d).\n", r);
  			if (saved) {
-				saved = false;
+				/* if reset fails, try without saving data */
+				rdev->needs_reset = true;
  				radeon_suspend(rdev);
-				goto retry;
+				up_read(&rdev->exclusive_lock);
+				return -EAGAIN;
  			}

drivers/gpu/drm/radeon/radeon_device.c retry:

Quoted 10 lines
drivers/gpu/drm/radeon/radeon_device.c +0 -5
  		}
-	} else {
-		radeon_fence_driver_force_completion(rdev);
-		for (i = 0; i < RADEON_NUM_RINGS; ++i) {
-			kfree(ring_data[i]);
-		}
  	}
 
 
  	radeon_pm_resume(rdev);

Move this more up.

Alex is more into this, but it's probably a bug in the current reset code that this is after the IB tests, cause the IB tests needs everything powered up and with PM handling suspended it is possible that individual blocks are powered down.

Thanks,
Christian.

Quoted 14 lines
drivers/gpu/drm/radeon/radeon_device.c +2 -2
  	drm_helper_resume_force_mode(rdev->ddev);
 
-	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);
+	rdev->in_reset = false;
+	up_read(&rdev->exclusive_lock);
  	return r;
  }
#2 lore raw

Re: [PATCH 2/3] drm/radeon: handle lockup in delayed work, v3

In reply to Maarten Lankhorst

Am 18.08.2014 um 16:45 schrieb Maarten Lankhorst:

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

---
V1 had a nasty bug breaking gpu lockup recovery. The fix is not allowing radeon_fence_driver_check_lockup to take exclusive_lock, and kill it during lockup recovery instead.
V2 used delayed work that ran during lockup recovery, but required read lock. I've fixed this by downgrading the write, and retrying if recovery fails.

---
  drivers/gpu/drm/radeon/radeon.h       |   2 +
  drivers/gpu/drm/radeon/radeon_fence.c | 115 +++++++++++++++++-----------------
  2 files changed, 61 insertions(+), 56 deletions(-)
drivers/gpu/drm/radeon/radeon.h +0 -0
index 1d806983ec7b..29504efe8971 100644

drivers/gpu/drm/radeon/radeon.h struct radeon_fence_driver { · line 359

Quoted 5 lines
drivers/gpu/drm/radeon/radeon.h +2 -0
@@ -355,6 +355,8 @@ struct radeon_fence_driver {
  	uint64_t			sync_seq[RADEON_NUM_RINGS];
  	atomic64_t			last_seq;
  	bool				initialized;
+	struct delayed_work		fence_check_work;
+	struct radeon_device		*rdev;

Put the reference to the device as the first field in the structure.

Quoted 12 lines
drivers/gpu/drm/radeon/radeon.h +0 -0
  };
 
  struct radeon_fence {
drivers/gpu/drm/radeon/radeon_fence.c +0 -1
index 913787085dfa..94eca53d99f8 100644
@@ -125,16 +125,7 @@ int radeon_fence_emit(struct radeon_device *rdev,
  	return 0;
  }
 
-/**

drivers/gpu/drm/radeon/radeon_fence.c int radeon_fence_emit(struct radeon_device *rdev, · line 128

Quoted 10 lines
drivers/gpu/drm/radeon/radeon_fence.c +1 -9
- * radeon_fence_process - process a fence
- *
- * @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).
- */
-void radeon_fence_process(struct radeon_device *rdev, int ring)
+static bool __radeon_fence_process(struct radeon_device *rdev, int ring)

Don't use "__" for internal radeon function names and especially don't remove the function documentation.

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

Quoted 12 lines
drivers/gpu/drm/radeon/radeon_fence.c +4 -1
  {
  	uint64_t seq, last_seq, last_emitted;
  	unsigned count_loop = 0;

@@ -190,7 +181,53 @@ void radeon_fence_process(struct radeon_device *rdev, int ring)
  		}
  	} while (atomic64_xchg(&rdev->fence_drv[ring].last_seq, seq) > seq);
 
-	if (wake)
+	if (seq < last_emitted && !rdev->in_reset)
+		mod_delayed_work(system_power_efficient_wq,
+				 &rdev->fence_drv[ring].fence_check_work,
+				 RADEON_FENCE_JIFFIES_TIMEOUT);

Am I wrong or do you queue the work only after radeon_fence_process is called for the first time?

Might be a good idea to have an explicit queue_delayed_work in radeon_fence_emit as well.

Quoted 144 lines
drivers/gpu/drm/radeon/radeon_fence.c +54 -45
+
+	return wake;
+}
+
+static void radeon_fence_driver_check_lockup(struct work_struct *work)
+{
+	struct radeon_fence_driver *fence_drv;
+	struct radeon_device *rdev;
+	unsigned long iring;
+
+	fence_drv = container_of(work, struct radeon_fence_driver, fence_check_work.work);
+	rdev = fence_drv->rdev;
+	iring = fence_drv - &rdev->fence_drv[0];
+
+	down_read(&rdev->exclusive_lock);
+	if (__radeon_fence_process(rdev, iring))
+		wake_up_all(&rdev->fence_queue);
+	else if (radeon_ring_is_lockup(rdev, iring, &rdev->ring[iring])) {
+		/* good news we believe it's a lockup */
+		dev_warn(rdev->dev, "GPU lockup (current fence id "
+			 "0x%016llx last fence id 0x%016llx on ring %ld)\n",
+			 (uint64_t)atomic64_read(&fence_drv->last_seq),
+			 fence_drv->sync_seq[iring], iring);
+
+		/* remember that we need an reset */
+		rdev->needs_reset = true;
+		wake_up_all(&rdev->fence_queue);
+	}
+	up_read(&rdev->exclusive_lock);
+}
+
+/**
+ * radeon_fence_process - process a fence
+ *
+ * @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).
+ */
+void radeon_fence_process(struct radeon_device *rdev, int ring)
+{
+	if (__radeon_fence_process(rdev, ring))
  		wake_up_all(&rdev->fence_queue);
  }
 

@@ -302,9 +339,10 @@ static int radeon_fence_wait_seq(struct radeon_device *rdev, u64 *target_seq,
  {
  	uint64_t last_seq[RADEON_NUM_RINGS];
  	bool signaled;
-	int i, r;
+	int i;
 
  	while (!radeon_fence_any_seq_signaled(rdev, target_seq)) {
+		long r;
 
  		/* Save current sequence values, used to check for GPU lockups */
  		for (i = 0; i < RADEON_NUM_RINGS; ++i) {

@@ -319,11 +357,11 @@ static int radeon_fence_wait_seq(struct radeon_device *rdev, u64 *target_seq,
  		if (intr) {
  			r = wait_event_interruptible_timeout(rdev->fence_queue, (
  				(signaled = radeon_fence_any_seq_signaled(rdev, target_seq))
-				 || rdev->needs_reset), RADEON_FENCE_JIFFIES_TIMEOUT);
+				 || rdev->needs_reset), MAX_SCHEDULE_TIMEOUT);
  		} else {
  			r = wait_event_timeout(rdev->fence_queue, (
  				(signaled = radeon_fence_any_seq_signaled(rdev, target_seq))
-				 || rdev->needs_reset), RADEON_FENCE_JIFFIES_TIMEOUT);
+				 || rdev->needs_reset), MAX_SCHEDULE_TIMEOUT);
  		}
 
  		for (i = 0; i < RADEON_NUM_RINGS; ++i) {

@@ -334,50 +372,11 @@ static int radeon_fence_wait_seq(struct radeon_device *rdev, u64 *target_seq,
  			trace_radeon_fence_wait_end(rdev->ddev, i, target_seq[i]);
  		}
 
-		if (unlikely(r < 0))
+		if (r < 0)
  			return r;
 
-		if (unlikely(!signaled)) {
-			if (rdev->needs_reset)
-				return -EDEADLK;
-
-			/* we were interrupted for some reason and fence
-			 * isn't signaled yet, resume waiting */
-			if (r)
-				continue;
-
-			for (i = 0; i < RADEON_NUM_RINGS; ++i) {
-				if (!target_seq[i])
-					continue;
-
-				if (last_seq[i] != atomic64_read(&rdev->fence_drv[i].last_seq))
-					break;
-			}
-
-			if (i != RADEON_NUM_RINGS)
-				continue;
-
-			for (i = 0; i < RADEON_NUM_RINGS; ++i) {
-				if (!target_seq[i])
-					continue;
-
-				if (radeon_ring_is_lockup(rdev, i, &rdev->ring[i]))
-					break;
-			}
-
-			if (i < RADEON_NUM_RINGS) {
-				/* good news we believe it's a lockup */
-				dev_warn(rdev->dev, "GPU lockup (waiting for "
-					 "0x%016llx last fence id 0x%016llx on"
-					 " ring %d)\n",
-					 target_seq[i], last_seq[i], i);
-
-				/* remember that we need an reset */
-				rdev->needs_reset = true;
-				wake_up_all(&rdev->fence_queue);
-				return -EDEADLK;
-			}
-		}
+		if (rdev->needs_reset)
+			return -EDEADLK;
  	}
  	return 0;
  }

@@ -711,6 +710,9 @@ static void radeon_fence_driver_init_ring(struct radeon_device *rdev, int ring)
  		rdev->fence_drv[ring].sync_seq[i] = 0;
  	atomic64_set(&rdev->fence_drv[ring].last_seq, 0);
  	rdev->fence_drv[ring].initialized = false;
+	INIT_DELAYED_WORK(&rdev->fence_drv[ring].fence_check_work,
+			  radeon_fence_driver_check_lockup);
+	rdev->fence_drv[ring].rdev = rdev;
  }
 
  /**

@@ -760,6 +762,7 @@ void radeon_fence_driver_fini(struct radeon_device *rdev)
  			/* no need to trigger GPU reset as we are unloading */
  			radeon_fence_driver_force_completion(rdev);
  		}
+		cancel_delayed_work_sync(&rdev->fence_drv[ring].fence_check_work);
  		wake_up_all(&rdev->fence_queue);
  		radeon_scratch_free(rdev, rdev->fence_drv[ring].scratch_reg);
  		rdev->fence_drv[ring].initialized = false;
author
#7 lore raw

PATCH 1/3 drm/radeon: take exclusive_lock in read mode during ring tests, v2

In reply to Christian König

This is needed for the next commit, because the lockup detection will need the read lock to run.

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

---
Changes since v1:

Changed order of resuming a bit according to ckoenig's feedback.

Reset handling seems just as unreliable as before this commit, but at least it restores mode correctly now. :-)

 drivers/gpu/drm/radeon/radeon.h        |  2 +-
 drivers/gpu/drm/radeon/radeon_device.c | 64 ++++++++++++++++++++--------------
 2 files changed, 39 insertions(+), 27 deletions(-)
drivers/gpu/drm/radeon/radeon.h +1 -1
index 9e1732eb402c..1d806983ec7b 100644
@@ -2312,7 +2312,7 @@ struct radeon_device {
 	bool				need_dma32;
 	bool				accel_working;
 	bool				fastfb_working; /* IGP feature*/
-	bool				needs_reset;
+	bool				needs_reset, in_reset;
 	struct radeon_surface_reg surface_regs[RADEON_GEM_MAX_SURFACES];
 	const struct firmware *me_fw;	/* all family ME firmware */
 	const struct firmware *pfp_fw;	/* r6/700 PFP firmware */
drivers/gpu/drm/radeon/radeon_device.c +38 -26

108 lines not on this page · raw message

Signature · 1 line
2.0.4
author
#3 lore raw

Re: [PATCH 2/3] drm/radeon: handle lockup in delayed work, v3

In reply to Christian König

Op 18-08-14 om 17:12 schreef Christian König:

Quoted 41 lines

Am 18.08.2014 um 16:45 schrieb Maarten Lankhorst:

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

---
V1 had a nasty bug breaking gpu lockup recovery. The fix is not allowing radeon_fence_driver_check_lockup to take exclusive_lock, and kill it during lockup recovery instead.
V2 used delayed work that ran during lockup recovery, but required read lock. I've fixed this by downgrading the write, and retrying if recovery fails.

---
  drivers/gpu/drm/radeon/radeon.h       |   2 +
  drivers/gpu/drm/radeon/radeon_fence.c | 115 +++++++++++++++++-----------------
  2 files changed, 61 insertions(+), 56 deletions(-)
drivers/gpu/drm/radeon/radeon.h +0 -0
index 1d806983ec7b..29504efe8971 100644

drivers/gpu/drm/radeon/radeon.h struct radeon_fence_driver { · line 359

Quoted 5 lines
drivers/gpu/drm/radeon/radeon.h +2 -0
@@ -355,6 +355,8 @@ struct radeon_fence_driver {
      uint64_t            sync_seq[RADEON_NUM_RINGS];
      atomic64_t            last_seq;
      bool                initialized;
+    struct delayed_work        fence_check_work;
+    struct radeon_device        *rdev;

Put the reference to the device as the first field in the structure.

Quoted 14 lines
drivers/gpu/drm/radeon/radeon.h +0 -0
  };
    struct radeon_fence {
drivers/gpu/drm/radeon/radeon_fence.c +0 -4
index 913787085dfa..94eca53d99f8 100644
@@ -125,16 +125,7 @@ int radeon_fence_emit(struct radeon_device *rdev,
      return 0;
  }
  -/**
- * radeon_fence_process - process a fence
- *
- * @rdev: radeon_device pointer
- * @ring: ring index the fence is associated with
Quoted 8 lines

drivers/gpu/drm/radeon/radeon_fence.c int radeon_fence_emit(struct radeon_device *rdev, · line 128

Quoted 6 lines
drivers/gpu/drm/radeon/radeon_fence.c +1 -5
- *
- * Checks the current fence value and wakes the fence queue
- * if the sequence number has increased (all asics).
- */
-void radeon_fence_process(struct radeon_device *rdev, int ring)
+static bool __radeon_fence_process(struct radeon_device *rdev, int ring)

Don't use "__" for internal radeon function names and especially don't remove the function documentation.

I've moved the documentation to the new radeon_fence_process function that calls the __radeon_fence_process one,
which is an internal function that is only used by the driver. I guess I can rename it to __radeon_fence_process_nowake or something instead, but documentation doesn't make sense since it's not used outside of radeon_fence.c

Quoted 14 lines

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

Quoted 11 lines
drivers/gpu/drm/radeon/radeon_fence.c +4 -0
  {
      uint64_t seq, last_seq, last_emitted;
      unsigned count_loop = 0;

@@ -190,7 +181,53 @@ void radeon_fence_process(struct radeon_device *rdev, int ring)
          }
      } while (atomic64_xchg(&rdev->fence_drv[ring].last_seq, seq) > seq);
  -    if (wake)
+    if (seq < last_emitted && !rdev->in_reset)
+        mod_delayed_work(system_power_efficient_wq,
+                 &rdev->fence_drv[ring].fence_check_work,
+                 RADEON_FENCE_JIFFIES_TIMEOUT);

Am I wrong or do you queue the work only after radeon_fence_process is called for the first time?

Quoted 1 line

Might be a good idea to have an explicit queue_delayed_work in radeon_fence_emit as well.

Yeah might as well, with these changes it makes sense to run it as soon as possible.

But if there are no waiters, there's no real need. It's a tree falling in a forest with no-one around to hear it. ;-)

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

#8 lore raw
In reply to Christian König
Quoted 145 lines · Christian König

Am 18.08.2014 um 16:45 schrieb Maarten Lankhorst:

Quoted 47 lines

This is needed for the next commit, because the lockup detection will need the read lock to run.

Signed-off-by Maarten Lankhorst <[email protected]>
---
  drivers/gpu/drm/radeon/radeon.h        |  2 +-
  drivers/gpu/drm/radeon/radeon_device.c | 61
++++++++++++++++++++--------------
  2 files changed, 37 insertions(+), 26 deletions(-)
drivers/gpu/drm/radeon/radeon.h +1 -1
index 9e1732eb402c..1d806983ec7b 100644
@@ -2312,7 +2312,7 @@ struct radeon_device {
        bool                            need_dma32;
        bool                            accel_working;
        bool                            fastfb_working; /* IGP feature*/
-       bool                            needs_reset;
+       bool                            needs_reset, in_reset;
        struct radeon_surface_reg surface_regs[RADEON_GEM_MAX_SURFACES];
        const struct firmware *me_fw;   /* all family ME firmware */
        const struct firmware *pfp_fw;  /* r6/700 PFP firmware */
drivers/gpu/drm/radeon/radeon_device.c +1 -8
index c8ea050c8fa4..82633fdd399d 100644
@@ -1671,29 +1671,34 @@ int radeon_gpu_reset(struct radeon_device *rdev)
        down_write(&rdev->exclusive_lock);
        if (!rdev->needs_reset) {
+               WARN_ON(rdev->in_reset);
                up_write(&rdev->exclusive_lock);
                return 0;
        }
        rdev->needs_reset = false;
-
-       radeon_save_bios_scratch_regs(rdev);
-       /* block TTM */
        resched = ttm_bo_lock_delayed_workqueue(&rdev->mman.bdev);
-       radeon_pm_suspend(rdev);
-       radeon_suspend(rdev);
  -     for (i = 0; i < RADEON_NUM_RINGS; ++i) {
-               ring_sizes[i] = radeon_ring_backup(rdev, &rdev->ring[i],
-                                                  &ring_data[i]);
-               if (ring_sizes[i]) {

drivers/gpu/drm/radeon/radeon_device.c int radeon_gpu_reset(struct radeon_device *rdev) · line 1686

Quoted 10 lines
drivers/gpu/drm/radeon/radeon_device.c +7 -3
-                       saved = true;
-                       dev_info(rdev->dev, "Saved %d dwords of commands "
-                                "on ring %d.\n", ring_sizes[i], i);
+       if (!rdev->in_reset) {
+               rdev->in_reset = true;
+
+               radeon_save_bios_scratch_regs(rdev);
+               /* block TTM */
+               radeon_pm_suspend(rdev);
+               radeon_suspend(rdev);

That won't work correctly because you might end up with calling pm_resume more often than suspend and that can only lead to a crash. Saying this we probably already have a bug in the reset code at this point anyway, but see below.

Quoted 22 lines
drivers/gpu/drm/radeon/radeon_device.c +7 -0
+
+               for (i = 0; i < RADEON_NUM_RINGS; ++i) {
+                       ring_sizes[i] = radeon_ring_backup(rdev,
 &rdev->ring[i],
+                                                          &ring_data[i]);
+                       if (ring_sizes[i]) {
+                               saved = true;
+                               dev_info(rdev->dev, "Saved %d dwords of

commands "
+ "on ring %d.\n", ring_sizes[i],
i);

drivers/gpu/drm/radeon/radeon_device.c +3 -1
+                       }
                }
-       }
+       } else
+               memset(ring_data, 0, sizeof(ring_data));
  -retry:
        r = radeon_asic_reset(rdev);
        if (!r) {
                dev_info(rdev->dev, "GPU reset succeeded, trying to

resume\n");

drivers/gpu/drm/radeon/radeon_device.c retry: · line 1707

Quoted 1 line
drivers/gpu/drm/radeon/radeon_device.c +0 -0
@@ -1702,40 +1707,46 @@ retry:
        radeon_restore_bios_scratch_regs(rdev);

We should resume PM here as well.

Quoted 7 lines
drivers/gpu/drm/radeon/radeon_device.c +1 -1
  -     if (!r) {
+       if (!r && saved) {
                for (i = 0; i < RADEON_NUM_RINGS; ++i) {
                        radeon_ring_restore(rdev, &rdev->ring[i],
                                            ring_sizes[i], ring_data[i]);
-                       ring_sizes[i] = 0;
                        ring_data[i] = NULL;

drivers/gpu/drm/radeon/radeon_device.c retry: · line 1723

Quoted 10 lines
drivers/gpu/drm/radeon/radeon_device.c +9 -0
                }
+       } else {
+               radeon_fence_driver_force_completion(rdev);
+
+               for (i = 0; i < RADEON_NUM_RINGS; ++i) {
+                       kfree(ring_data[i]);
+               }
+       }
+       downgrade_write(&rdev->exclusive_lock);
+       ttm_bo_unlock_delayed_workqueue(&rdev->mman.bdev, resched);

I would unlock the delayed_workqueue first and then downgrade the readlock.

Quoted 16 lines
drivers/gpu/drm/radeon/radeon_device.c +4 -2
  +     if (!r) {
                r = radeon_ib_ring_tests(rdev);
                if (r) {
                        dev_err(rdev->dev, "ib ring test failed (%d).\n",
 r);
                        if (saved) {
-                               saved = false;
+                               /* if reset fails, try without saving data
 */
+                               rdev->needs_reset = true;
                                radeon_suspend(rdev);
-                               goto retry;
+                               up_read(&rdev->exclusive_lock);
+                               return -EAGAIN;
                        }
                }

drivers/gpu/drm/radeon/radeon_device.c retry: · line 1742

Quoted 10 lines
drivers/gpu/drm/radeon/radeon_device.c +0 -5
-       } else {
-               radeon_fence_driver_force_completion(rdev);
-               for (i = 0; i < RADEON_NUM_RINGS; ++i) {
-                       kfree(ring_data[i]);
-               }
        }
 
 
 
        radeon_pm_resume(rdev);
Quoted 6 lines

Move this more up.

Alex is more into this, but it's probably a bug in the current reset code that this is after the IB tests, cause the IB tests needs everything powered up and with PM handling suspended it is possible that individual blocks are powered down.

Yeah, looks like a bug. I think the attached patch should fix it.

Alex

Quoted 19 lines

Thanks,
Christian.

drivers/gpu/drm/radeon/radeon_device.c retry:

Quoted 12 lines
drivers/gpu/drm/radeon/radeon_device.c +2 -0
        drm_helper_resume_force_mode(rdev->ddev);
  -     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);
+       rdev->in_reset = false;
+       up_read(&rdev->exclusive_lock);
        return r;
  }
Quoted 4 lines

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

From 43021a83cf04064e9771964233487fe9f49fa3db Mon Sep 17 00:00:00 2001
From: Alex Deucher <[email protected]>
Date: Mon, 18 Aug 2014 11:57:28 -0400
Subject: [PATCH] drm/radeon: fix pm handling in radeon_gpu_reset

pm_suspend is handled in the radeon_suspend callbacks. pm_resume has special handling depending on whether dpm or legacy pm is enabled. Change radeon_gpu_reset to mirror the behavior in the suspend and resume pathes.

Signed-off-by Alex Deucher <[email protected]>
Cc [email protected]
---
 drivers/gpu/drm/radeon/radeon_device.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)
drivers/gpu/drm/radeon/radeon_device.c +16 -2
index c8ea050..8e61870 100644
@@ -1680,7 +1680,6 @@ 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);
-	radeon_pm_suspend(rdev);
 	radeon_suspend(rdev);
 
 	for (i = 0; i < RADEON_NUM_RINGS; ++i) {

@@ -1726,9 +1725,24 @@ retry:
 		}
 	}
 
-	radeon_pm_resume(rdev);
+	if ((rdev->pm.pm_method == PM_METHOD_DPM) && rdev->pm.dpm_enabled) {
+		/* do dpm late init */
+		r = radeon_pm_late_init(rdev);
+		if (r) {
+			rdev->pm.dpm_enabled = false;
+			DRM_ERROR("radeon_pm_late_init failed, disabling dpm\n");
+		}
+	} else {
+		/* resume old pm late */
+		radeon_pm_resume(rdev);
+	}
+
 	drm_helper_resume_force_mode(rdev->ddev);
 
+	/* set the power state here in case we are a PX system or headless */
+	if ((rdev->pm.pm_method == PM_METHOD_DPM) && rdev->pm.dpm_enabled)
+		radeon_pm_compute_clocks(rdev);
+
 	ttm_bo_unlock_delayed_workqueue(&rdev->mman.bdev, resched);
 	if (r) {
 		/* bad news, how to tell it to userspace ? */
Signature · 1 line
1.8.3.1

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

#9 lore raw
Quoted 1 line

Yeah, looks like a bug. I think the attached patch should fix it.

Sounds logical and the patch is Reviewed-by: Christian König <[email protected]>

Going to apply Maartens patch on top and test that one a bit to make sure it works as expected.

Regards,
Christian.

Am 18.08.2014 um 18:03 schrieb Alex Deucher:

Quoted 173 lines
Quoted 138 lines · Christian König

Am 18.08.2014 um 16:45 schrieb Maarten Lankhorst:

Quoted 47 lines

This is needed for the next commit, because the lockup detection will need the read lock to run.

Signed-off-by Maarten Lankhorst <[email protected]>
---
   drivers/gpu/drm/radeon/radeon.h        |  2 +-
   drivers/gpu/drm/radeon/radeon_device.c | 61
++++++++++++++++++++--------------
   2 files changed, 37 insertions(+), 26 deletions(-)
drivers/gpu/drm/radeon/radeon.h +1 -1
index 9e1732eb402c..1d806983ec7b 100644
@@ -2312,7 +2312,7 @@ struct radeon_device {
         bool                            need_dma32;
         bool                            accel_working;
         bool                            fastfb_working; /* IGP feature*/
-       bool                            needs_reset;
+       bool                            needs_reset, in_reset;
         struct radeon_surface_reg surface_regs[RADEON_GEM_MAX_SURFACES];
         const struct firmware *me_fw;   /* all family ME firmware */
         const struct firmware *pfp_fw;  /* r6/700 PFP firmware */
drivers/gpu/drm/radeon/radeon_device.c +1 -8
index c8ea050c8fa4..82633fdd399d 100644
@@ -1671,29 +1671,34 @@ int radeon_gpu_reset(struct radeon_device *rdev)
         down_write(&rdev->exclusive_lock);
         if (!rdev->needs_reset) {
+               WARN_ON(rdev->in_reset);
                 up_write(&rdev->exclusive_lock);
                 return 0;
         }
         rdev->needs_reset = false;
-
-       radeon_save_bios_scratch_regs(rdev);
-       /* block TTM */
         resched = ttm_bo_lock_delayed_workqueue(&rdev->mman.bdev);
-       radeon_pm_suspend(rdev);
-       radeon_suspend(rdev);
   -     for (i = 0; i < RADEON_NUM_RINGS; ++i) {
-               ring_sizes[i] = radeon_ring_backup(rdev, &rdev->ring[i],
-                                                  &ring_data[i]);
-               if (ring_sizes[i]) {

drivers/gpu/drm/radeon/radeon_device.c int radeon_gpu_reset(struct radeon_device *rdev) · line 1686

Quoted 10 lines
drivers/gpu/drm/radeon/radeon_device.c +7 -3
-                       saved = true;
-                       dev_info(rdev->dev, "Saved %d dwords of commands "
-                                "on ring %d.\n", ring_sizes[i], i);
+       if (!rdev->in_reset) {
+               rdev->in_reset = true;
+
+               radeon_save_bios_scratch_regs(rdev);
+               /* block TTM */
+               radeon_pm_suspend(rdev);
+               radeon_suspend(rdev);

That won't work correctly because you might end up with calling pm_resume more often than suspend and that can only lead to a crash. Saying this we probably already have a bug in the reset code at this point anyway, but see below.

Quoted 22 lines
drivers/gpu/drm/radeon/radeon_device.c +7 -0
+
+               for (i = 0; i < RADEON_NUM_RINGS; ++i) {
+                       ring_sizes[i] = radeon_ring_backup(rdev,
 &rdev->ring[i],
+                                                          &ring_data[i]);
+                       if (ring_sizes[i]) {
+                               saved = true;
+                               dev_info(rdev->dev, "Saved %d dwords of

commands "
+ "on ring %d.\n", ring_sizes[i],
i);

drivers/gpu/drm/radeon/radeon_device.c +3 -1
+                       }
                 }
-       }
+       } else
+               memset(ring_data, 0, sizeof(ring_data));
   -retry:
         r = radeon_asic_reset(rdev);
         if (!r) {
                 dev_info(rdev->dev, "GPU reset succeeded, trying to

resume\n");

drivers/gpu/drm/radeon/radeon_device.c retry: · line 1707

Quoted 1 line
drivers/gpu/drm/radeon/radeon_device.c +0 -0
@@ -1702,40 +1707,46 @@ retry:
         radeon_restore_bios_scratch_regs(rdev);

We should resume PM here as well.

Quoted 7 lines
drivers/gpu/drm/radeon/radeon_device.c +1 -1
   -     if (!r) {
+       if (!r && saved) {
                 for (i = 0; i < RADEON_NUM_RINGS; ++i) {
                         radeon_ring_restore(rdev, &rdev->ring[i],
                                             ring_sizes[i], ring_data[i]);
-                       ring_sizes[i] = 0;
                         ring_data[i] = NULL;

drivers/gpu/drm/radeon/radeon_device.c retry: · line 1723

Quoted 10 lines
drivers/gpu/drm/radeon/radeon_device.c +9 -0
                 }
+       } else {
+               radeon_fence_driver_force_completion(rdev);
+
+               for (i = 0; i < RADEON_NUM_RINGS; ++i) {
+                       kfree(ring_data[i]);
+               }
+       }
+       downgrade_write(&rdev->exclusive_lock);
+       ttm_bo_unlock_delayed_workqueue(&rdev->mman.bdev, resched);

I would unlock the delayed_workqueue first and then downgrade the readlock.

Quoted 23 lines
drivers/gpu/drm/radeon/radeon_device.c +4 -7
   +     if (!r) {
                 r = radeon_ib_ring_tests(rdev);
                 if (r) {
                         dev_err(rdev->dev, "ib ring test failed (%d).\n",
 r);
                         if (saved) {
-                               saved = false;
+                               /* if reset fails, try without saving data
 */
+                               rdev->needs_reset = true;
                                 radeon_suspend(rdev);
-                               goto retry;
+                               up_read(&rdev->exclusive_lock);
+                               return -EAGAIN;
                         }
                 }
-       } else {
-               radeon_fence_driver_force_completion(rdev);
-               for (i = 0; i < RADEON_NUM_RINGS; ++i) {
-                       kfree(ring_data[i]);
-               }
         }
Quoted 8 lines

drivers/gpu/drm/radeon/radeon_device.c retry: · line 1740

Quoted 1 line
drivers/gpu/drm/radeon/radeon_device.c +0 -0
         radeon_pm_resume(rdev);

Move this more up.

Alex is more into this, but it's probably a bug in the current reset code that this is after the IB tests, cause the IB tests needs everything powered up and with PM handling suspended it is possible that individual blocks are powered down.

Yeah, looks like a bug. I think the attached patch should fix it.

Alex

Quoted 21 lines

Thanks,
Christian.

drivers/gpu/drm/radeon/radeon_device.c retry: · line 1752

Quoted 12 lines
drivers/gpu/drm/radeon/radeon_device.c +2 -0
         drm_helper_resume_force_mode(rdev->ddev);
   -     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);
+       rdev->in_reset = false;
+       up_read(&rdev->exclusive_lock);
         return r;
   }

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

#10 lore raw
Quoted 7 lines · Christian König
Quoted 1 line

Yeah, looks like a bug. I think the attached patch should fix it.

Sounds logical and the patch is Reviewed-by: Christian König <[email protected]>

Going to apply Maartens patch on top and test that one a bit to make sure it works as expected.

pushed my current -fixes queue to my drm-fixes-3.17-wip branch if that helps.

Alex

Quoted 198 lines

Regards,
Christian.

Am 18.08.2014 um 18:03 schrieb Alex Deucher:

Quoted 190 lines
Quoted 154 lines · Christian König

Am 18.08.2014 um 16:45 schrieb Maarten Lankhorst:

Quoted 52 lines

This is needed for the next commit, because the lockup detection will need the read lock to run.

Signed-off-by Maarten Lankhorst <[email protected]>
---
   drivers/gpu/drm/radeon/radeon.h        |  2 +-
   drivers/gpu/drm/radeon/radeon_device.c | 61
++++++++++++++++++++--------------
   2 files changed, 37 insertions(+), 26 deletions(-)
drivers/gpu/drm/radeon/radeon.h +0 -0
index 9e1732eb402c..1d806983ec7b 100644
@@ -2312,7 +2312,7 @@ struct radeon_device {
         bool                            need_dma32;
         bool                            accel_working;
         bool                            fastfb_working; /* IGP

feature*/

drivers/gpu/drm/radeon/radeon.h +1 -1
-       bool                            needs_reset;
+       bool                            needs_reset, in_reset;
         struct radeon_surface_reg

surface_regs[RADEON_GEM_MAX_SURFACES];
const struct firmware *me_fw; /* all family ME firmware */
const struct firmware *pfp_fw; /* r6/700 PFP firmware */

drivers/gpu/drm/radeon/radeon_device.c +1 -10
index c8ea050c8fa4..82633fdd399d 100644
@@ -1671,29 +1671,34 @@ int radeon_gpu_reset(struct radeon_device *rdev)
         down_write(&rdev->exclusive_lock);
         if (!rdev->needs_reset) {
+               WARN_ON(rdev->in_reset);
                 up_write(&rdev->exclusive_lock);
                 return 0;
         }
         rdev->needs_reset = false;
-
-       radeon_save_bios_scratch_regs(rdev);
-       /* block TTM */
         resched = ttm_bo_lock_delayed_workqueue(&rdev->mman.bdev);
-       radeon_pm_suspend(rdev);
-       radeon_suspend(rdev);
   -     for (i = 0; i < RADEON_NUM_RINGS; ++i) {
-               ring_sizes[i] = radeon_ring_backup(rdev, &rdev->ring[i],
-                                                  &ring_data[i]);
-               if (ring_sizes[i]) {
-                       saved = true;
-                       dev_info(rdev->dev, "Saved %d dwords of commands

"

drivers/gpu/drm/radeon/radeon_device.c

Quoted 8 lines
drivers/gpu/drm/radeon/radeon_device.c +7 -1
-                                "on ring %d.\n", ring_sizes[i], i);
+       if (!rdev->in_reset) {
+               rdev->in_reset = true;
+
+               radeon_save_bios_scratch_regs(rdev);
+               /* block TTM */
+               radeon_pm_suspend(rdev);
+               radeon_suspend(rdev);

That won't work correctly because you might end up with calling pm_resume more often than suspend and that can only lead to a crash. Saying this we probably already have a bug in the reset code at this point anyway, but see
below.

Quoted 23 lines
drivers/gpu/drm/radeon/radeon_device.c +3 -0
+
+               for (i = 0; i < RADEON_NUM_RINGS; ++i) {
+                       ring_sizes[i] = radeon_ring_backup(rdev,

&rdev->ring[i],
+
&ring_data[i]);

drivers/gpu/drm/radeon/radeon_device.c +3 -0
+                       if (ring_sizes[i]) {
+                               saved = true;
+                               dev_info(rdev->dev, "Saved %d dwords of

commands "
+ "on ring %d.\n", ring_sizes[i],
i);

drivers/gpu/drm/radeon/radeon_device.c +3 -1
+                       }
                 }
-       }
+       } else
+               memset(ring_data, 0, sizeof(ring_data));
   -retry:
         r = radeon_asic_reset(rdev);
         if (!r) {
                 dev_info(rdev->dev, "GPU reset succeeded, trying to

resume\n");

drivers/gpu/drm/radeon/radeon_device.c retry: · line 1707

Quoted 1 line
drivers/gpu/drm/radeon/radeon_device.c +0 -0
@@ -1702,40 +1707,46 @@ retry:
         radeon_restore_bios_scratch_regs(rdev);

We should resume PM here as well.

Quoted 8 lines
drivers/gpu/drm/radeon/radeon_device.c +1 -1
   -     if (!r) {
+       if (!r && saved) {
                 for (i = 0; i < RADEON_NUM_RINGS; ++i) {
                         radeon_ring_restore(rdev, &rdev->ring[i],
                                             ring_sizes[i],
 ring_data[i]);
-                       ring_sizes[i] = 0;
                         ring_data[i] = NULL;

drivers/gpu/drm/radeon/radeon_device.c retry: · line 1724

Quoted 10 lines
drivers/gpu/drm/radeon/radeon_device.c +9 -0
                 }
+       } else {
+               radeon_fence_driver_force_completion(rdev);
+
+               for (i = 0; i < RADEON_NUM_RINGS; ++i) {
+                       kfree(ring_data[i]);
+               }
+       }
+       downgrade_write(&rdev->exclusive_lock);
+       ttm_bo_unlock_delayed_workqueue(&rdev->mman.bdev, resched);

I would unlock the delayed_workqueue first and then downgrade the readlock.

Quoted 17 lines
drivers/gpu/drm/radeon/radeon_device.c +1 -1
   +     if (!r) {
                 r = radeon_ib_ring_tests(rdev);
                 if (r) {
                         dev_err(rdev->dev, "ib ring test failed
 (%d).\n",
 r);
                         if (saved) {
-                               saved = false;
+                               /* if reset fails, try without saving

data
*/

drivers/gpu/drm/radeon/radeon_device.c +3 -1
+                               rdev->needs_reset = true;
                                 radeon_suspend(rdev);
-                               goto retry;
+                               up_read(&rdev->exclusive_lock);
+                               return -EAGAIN;
                         }

drivers/gpu/drm/radeon/radeon_device.c

Quoted 10 lines
drivers/gpu/drm/radeon/radeon_device.c +0 -5
                 }
-       } else {
-               radeon_fence_driver_force_completion(rdev);
-               for (i = 0; i < RADEON_NUM_RINGS; ++i) {
-                       kfree(ring_data[i]);
-               }
         }
 
 
         radeon_pm_resume(rdev);
Quoted 8 lines

Move this more up.

Alex is more into this, but it's probably a bug in the current reset code that this is after the IB tests, cause the IB tests needs everything powered
up and with PM handling suspended it is possible that individual blocks are
powered down.

Yeah, looks like a bug. I think the attached patch should fix it.

Alex

Quoted 21 lines

Thanks,
Christian.

drivers/gpu/drm/radeon/radeon_device.c

Quoted 12 lines
         drm_helper_resume_force_mode(rdev->ddev);
   -     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);

drivers/gpu/drm/radeon/radeon_device.c +2 -0
+       rdev->in_reset = false;
+       up_read(&rdev->exclusive_lock);
         return r;
   }

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

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

#4 lore raw

Re: [PATCH 2/3] drm/radeon: handle lockup in delayed work, v3

In reply to Maarten Lankhorst

Am 18.08.2014 um 17:28 schrieb Maarten Lankhorst:

Quoted 48 lines

Op 18-08-14 om 17:12 schreef Christian König:

Quoted 39 lines

Am 18.08.2014 um 16:45 schrieb Maarten Lankhorst:

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

---
V1 had a nasty bug breaking gpu lockup recovery. The fix is not allowing radeon_fence_driver_check_lockup to take exclusive_lock, and kill it during lockup recovery instead.
V2 used delayed work that ran during lockup recovery, but required read lock. I've fixed this by downgrading the write, and retrying if recovery fails.

---
   drivers/gpu/drm/radeon/radeon.h       |   2 +
   drivers/gpu/drm/radeon/radeon_fence.c | 115 +++++++++++++++++-----------------
   2 files changed, 61 insertions(+), 56 deletions(-)
drivers/gpu/drm/radeon/radeon.h +0 -0
index 1d806983ec7b..29504efe8971 100644

drivers/gpu/drm/radeon/radeon.h struct radeon_fence_driver { · line 359

Quoted 5 lines
drivers/gpu/drm/radeon/radeon.h +2 -0
@@ -355,6 +355,8 @@ struct radeon_fence_driver {
       uint64_t            sync_seq[RADEON_NUM_RINGS];
       atomic64_t            last_seq;
       bool                initialized;
+    struct delayed_work        fence_check_work;
+    struct radeon_device        *rdev;

Put the reference to the device as the first field in the structure.

Quoted 13 lines
drivers/gpu/drm/radeon/radeon.h +0 -0
   };
     struct radeon_fence {
drivers/gpu/drm/radeon/radeon_fence.c +0 -3
index 913787085dfa..94eca53d99f8 100644
@@ -125,16 +125,7 @@ int radeon_fence_emit(struct radeon_device *rdev,
       return 0;
   }
   -/**
- * radeon_fence_process - process a fence
- *
- * @rdev: radeon_device pointer
Quoted 8 lines

drivers/gpu/drm/radeon/radeon_fence.c int radeon_fence_emit(struct radeon_device *rdev, · line 128

Quoted 7 lines
drivers/gpu/drm/radeon/radeon_fence.c +1 -6
- * @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).
- */
-void radeon_fence_process(struct radeon_device *rdev, int ring)
+static bool __radeon_fence_process(struct radeon_device *rdev, int ring)

Don't use "__" for internal radeon function names and especially don't remove the function documentation.

Quoted 3 lines

I've moved the documentation to the new radeon_fence_process function that calls the __radeon_fence_process one,
which is an internal function that is only used by the driver. I guess I can rename it to __radeon_fence_process_nowake or something instead, but documentation doesn't make sense since it's not used outside of radeon_fence.c

We try to document even static functions in doxygen style even if you can't see them outside the C file.

Quoted 13 lines
Quoted 13 lines

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

Quoted 11 lines
drivers/gpu/drm/radeon/radeon_fence.c +4 -0
   {
       uint64_t seq, last_seq, last_emitted;
       unsigned count_loop = 0;

@@ -190,7 +181,53 @@ void radeon_fence_process(struct radeon_device *rdev, int ring)
           }
       } while (atomic64_xchg(&rdev->fence_drv[ring].last_seq, seq) > seq);
   -    if (wake)
+    if (seq < last_emitted && !rdev->in_reset)
+        mod_delayed_work(system_power_efficient_wq,
+                 &rdev->fence_drv[ring].fence_check_work,
+                 RADEON_FENCE_JIFFIES_TIMEOUT);

Am I wrong or do you queue the work only after radeon_fence_process is called for the first time?

Quoted 4 lines
Quoted 1 line

Might be a good idea to have an explicit queue_delayed_work in radeon_fence_emit as well.

Yeah might as well, with these changes it makes sense to run it as soon as possible.

But if there are no waiters, there's no real need. It's a tree falling in a forest with no-one around to hear it. ;-)

That's why I suggested to schedule the work item only when the IRQ is enabled for waiting, otherwise it indeed doesn't make much sense.

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

author
#11 lore raw
In reply to Alex Deucher

Hey,

Quoted 11 lines · Alex Deucher
Quoted 7 lines · Christian König
Quoted 1 line

Yeah, looks like a bug. I think the attached patch should fix it.

Sounds logical and the patch is Reviewed-by: Christian König <[email protected]>

Going to apply Maartens patch on top and test that one a bit to make sure it works as expected.

pushed my current -fixes queue to my drm-fixes-3.17-wip branch if that helps.

Thanks, maybe that fixes uvd on resume for me. :-)

I'll have to rework it to include the changes, but does resuming everything in the order of my v2 patch look sane? Then as a final act I'm downgrading to read, and run the tests.

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

#12 lore raw
Quoted 13 lines · Maarten Lankhorst

Hey,

Quoted 10 lines · Alex Deucher
Quoted 7 lines · Christian König
Quoted 1 line

Yeah, looks like a bug. I think the attached patch should fix it.

Sounds logical and the patch is Reviewed-by: Christian König <[email protected]>

Going to apply Maartens patch on top and test that one a bit to make sure it works as expected.

Quoted 6 lines
Quoted 1 line

pushed my current -fixes queue to my drm-fixes-3.17-wip branch if that helps.

Thanks, maybe that fixes uvd on resume for me. :-)

I'll have to rework it to include the changes, but does resuming everything in the order of my v2 patch look sane? Then as a final act I'm downgrading to read, and run the tests.

Seems sane. Looks like we probably also need this attached patch as well to be on the safe side for displays. It would be nice to unify the suspend/resume and gpu_reset paths at some point.

Alex

From e8cfbe410871de7a4805f13c6f18c3551741c639 Mon Sep 17 00:00:00 2001
From: Alex Deucher <[email protected]>
Date: Mon, 18 Aug 2014 16:51:46 -0400
Subject: [PATCH] drm/radeon: fix display handling in radeon_gpu_reset

If the display hw was reset or a hard reset was used, we need to re-init some of the common display hardware as well.

Signed-off-by Alex Deucher <[email protected]>
Cc [email protected]
---
 drivers/gpu/drm/radeon/radeon_device.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
drivers/gpu/drm/radeon/radeon_device.c +16 -0
index 8e61870..6a219bc 100644
@@ -1681,6 +1681,7 @@ int radeon_gpu_reset(struct radeon_device *rdev)
 	/* block TTM */
 	resched = ttm_bo_lock_delayed_workqueue(&rdev->mman.bdev);
 	radeon_suspend(rdev);
+	radeon_hpd_fini(rdev);
 
 	for (i = 0; i < RADEON_NUM_RINGS; ++i) {
 		ring_sizes[i] = radeon_ring_backup(rdev, &rdev->ring[i],

@@ -1737,6 +1738,21 @@ retry:
 		radeon_pm_resume(rdev);
 	}
 
+	/* init dig PHYs, disp eng pll */
+	if (rdev->is_atom_bios) {
+		radeon_atom_encoder_init(rdev);
+		radeon_atom_disp_eng_pll_init(rdev);
+		/* turn on the BL */
+		if (rdev->mode_info.bl_encoder) {
+			u8 bl_level = radeon_get_backlight_level(rdev,
+								 rdev->mode_info.bl_encoder);
+			radeon_set_backlight_level(rdev, rdev->mode_info.bl_encoder,
+						   bl_level);
+		}
+	}
+	/* reset hpd state */
+	radeon_hpd_init(rdev);
+
 	drm_helper_resume_force_mode(rdev->ddev);
 
 	/* set the power state here in case we are a PX system or headless */
Signature · 1 line
1.8.3.1

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