drm/msm: GPU debugging enhancements

PATCH New Rob Clark 2 participants 7 messages Oct 24, 2017

Subsystems (4)

DRM drivers Arm Qualcomm mailing list DRM driver for Qualcomm display hardware DRM driver for Qualcomm Adreno GPUs

lore

Share & tools

lore

Rob Clark

author

2017-10-24 13:22 UTC #0 lore raw

cover drm/msm: GPU debugging enhancements

Something that I've been using locally for a while, and found quite helpful for debugging gallium driver issues:

1) Better messages about what process triggered a crash, which is in particular particularly useful for piglit
2) Mechanism to dump just submits which triggered GPU hangs.

Eventually I want to extend #2 to also dump submits that trigger faults, but this is going to require spiffing out IOMMU framework somewhat.. in particular we can't dump from IRQ context, so a workqueue is needed, but we want to keep the IOMMU stalled until after we dump the submit. Maybe we'll get something useable out of the currently discussed SVM patches.

Rob Clark (6):
drm/msm: show task cmdline in gpu recovery messages drm/msm: add special _get_vaddr_active() for cmdstream dumps drm/msm: split rd debugfs file
drm/msm/rd: allow adding addition msg to top of dump drm/msm: preserve IOVAs in submit's bo table drm/msm: dump submits which triggered gpu hang

 drivers/gpu/drm/msm/msm_drv.h        |   7 ++-
 drivers/gpu/drm/msm/msm_gem.c        |  22 ++++++-
 drivers/gpu/drm/msm/msm_gem_submit.c |  11 ++--
 drivers/gpu/drm/msm/msm_gpu.c        |  90 +++++++++++++++++++--------
 drivers/gpu/drm/msm/msm_rd.c         | 116 ++++++++++++++++++++++++++---------
 5 files changed, 183 insertions(+), 63 deletions(-)

Signature · 6 lines

2.13.6

_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno

Rob Clark

2017-10-24 13:22 UTC #1 lore raw

PATCH 1/6 drm/msm: show task cmdline in gpu recovery messages

Now that freedreno gallium driver defaults to using submit_queue task (render reordering), just showing task->comm is not so useful (ie. it is always "flush_queue:0"), so also dump the cmdline. This should also be more useful for piglit/shader_runner.

Signed-off-by Rob Clark <[email protected]>

---
 drivers/gpu/drm/msm/msm_gpu.c | 54 +++++++++++++++++++++++++++++++++----------
 1 file changed, 42 insertions(+), 12 deletions(-)

drivers/gpu/drm/msm/msm_gpu.c +42 ~~-12~~

index a05aa119f22b..d26a7282466e 100644
@@ -235,6 +235,20 @@ static void update_fences(struct msm_gpu *gpu, struct msm_ringbuffer *ring,
 	}
 }
 
+static struct msm_gem_submit *
+find_submit(struct msm_ringbuffer *ring, uint32_t fence)
+{
+	struct msm_gem_submit *submit;
+
+	WARN_ON(!mutex_is_locked(&ring->gpu->dev->struct_mutex));
+
+	list_for_each_entry(submit, &ring->submits, node)
+		if (submit->seqno == fence)
+			return submit;
+
+	return NULL;
+}
+
 static void retire_submits(struct msm_gpu *gpu);
 
 static void recover_worker(struct work_struct *work)

@@ -268,19 +282,35 @@ static void recover_worker(struct work_struct *work)
 	dev_err(dev->dev, "%s: hangcheck recover!\n", gpu->name);
 	fence = cur_ring->memptrs->fence + 1;
 
-	list_for_each_entry(submit, &cur_ring->submits, node) {
-		if (submit->seqno == fence) {
-			struct task_struct *task;
-
-			rcu_read_lock();
-			task = pid_task(submit->pid, PIDTYPE_PID);
-			if (task) {
-				dev_err(dev->dev, "%s: offending task: %s\n",
-						gpu->name, task->comm);
-			}
-			rcu_read_unlock();
-			break;
+	submit = find_submit(cur_ring, fence);
+	if (submit) {
+		struct task_struct *task;
+
+		rcu_read_lock();
+		task = pid_task(submit->pid, PIDTYPE_PID);
+		if (task) {
+			char buf[256];
+			int len;
+
+			/*
+			 * so slightly annoying, in other paths like mmap'ing gem
+			 * buffers, mmap_sem is acquired before struct_mutex, which
+			 * means we can't hold struct_mutex across the call to
+			 * get_cmdline().  But we don't need 'submit' any further
+			 * so no issue if hypothetically it goes away when dropping
+			 * struct_mutex.  For good measure null out the submit ptr
+			 * to make this obvious.
+			 */
+			submit = NULL;
+			mutex_unlock(&dev->struct_mutex);
+			len = get_cmdline(task, buf, sizeof(buf));
+			mutex_lock(&dev->struct_mutex);
+
+			dev_err(dev->dev, "%s: offending task: %s (%-*s)\n",
+					gpu->name, task->comm, len, buf);
 		}
+		rcu_read_unlock();
+
 	}
 
 	if (msm_gpu_active(gpu)) {

Signature · 6 lines

2.13.6

_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno

Rob Clark

2017-10-24 13:22 UTC #2 lore raw

PATCH 2/6 drm/msm: add special _get_vaddr_active() for cmdstream dumps

In reply to Rob Clark

Prep work for adding a debugfs file that dumps just submits which trigger hangs/faults. In this case the bo may already be in the MADV_DONTNEED state, but will be still on the active list (since the submit hasn't completed yet). So the normal check that the bo is in the WILLNEED state does not apply. (But of course the bo should definitely not be in the PURGED state!)

Signed-off-by Rob Clark <[email protected]>

---
 drivers/gpu/drm/msm/msm_drv.h |  1 +
 drivers/gpu/drm/msm/msm_gem.c | 22 ++++++++++++++++++++--
 drivers/gpu/drm/msm/msm_rd.c  |  2 +-
 3 files changed, 22 insertions(+), 3 deletions(-)

drivers/gpu/drm/msm/msm_drv.h +1 -0

index c46dc12dbbcc..666fce66f9dd 100644
@@ -213,6 +213,7 @@ struct drm_gem_object *msm_gem_prime_import_sg_table(struct drm_device *dev,
 int msm_gem_prime_pin(struct drm_gem_object *obj);
 void msm_gem_prime_unpin(struct drm_gem_object *obj);
 void *msm_gem_get_vaddr(struct drm_gem_object *obj);
+void *msm_gem_get_vaddr_active(struct drm_gem_object *obj);
 void msm_gem_put_vaddr(struct drm_gem_object *obj);
 int msm_gem_madvise(struct drm_gem_object *obj, unsigned madv);
 int msm_gem_sync_object(struct drm_gem_object *obj,

drivers/gpu/drm/msm/msm_gem.c +20 -2

index ea5bb0e1632c..81fe6d6740ce 100644
@@ -470,14 +470,16 @@ int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
 	return ret;
 }
 
-void *msm_gem_get_vaddr(struct drm_gem_object *obj)
+static void *get_vaddr(struct drm_gem_object *obj, unsigned madv)
 {
 	struct msm_gem_object *msm_obj = to_msm_bo(obj);
 	int ret = 0;
 
 	mutex_lock(&msm_obj->lock);
 
-	if (WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED)) {
+	if (WARN_ON(msm_obj->madv > madv)) {
+		dev_err(obj->dev->dev, "Invalid madv state: %u vs %u\n",
+			msm_obj->madv, madv);
 		mutex_unlock(&msm_obj->lock);
 		return ERR_PTR(-EBUSY);
 	}

@@ -513,6 +515,22 @@ void *msm_gem_get_vaddr(struct drm_gem_object *obj)
 	return ERR_PTR(ret);
 }
 
+void *msm_gem_get_vaddr(struct drm_gem_object *obj)
+{
+	return get_vaddr(obj, MSM_MADV_WILLNEED);
+}
+
+/*
+ * Don't use this!  It is for the very special case of dumping
+ * submits from GPU hangs or faults, were the bo may already
+ * be MSM_MADV_DONTNEED, but we know the buffer is still on the
+ * active list.
+ */
+void *msm_gem_get_vaddr_active(struct drm_gem_object *obj)
+{
+	return get_vaddr(obj, __MSM_MADV_PURGED);
+}
+
 void msm_gem_put_vaddr(struct drm_gem_object *obj)
 {
 	struct msm_gem_object *msm_obj = to_msm_bo(obj);

drivers/gpu/drm/msm/msm_rd.c +1 -1

index 35da0a07e567..981bb17c11a7 100644
@@ -294,7 +294,7 @@ static void snapshot_buf(struct msm_rd_state *rd,
 	if (!(submit->bos[idx].flags & MSM_SUBMIT_BO_READ))
 		return;
 
-	buf = msm_gem_get_vaddr(&obj->base);
+	buf = msm_gem_get_vaddr_active(&obj->base);
 	if (IS_ERR(buf))
 		return;

Signature · 6 lines

2.13.6

_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno

Rob Clark

2017-10-24 13:22 UTC #3 lore raw

PATCH 3/6 drm/msm: split rd debugfs file

In reply to Rob Clark

Split into two instances, the existing $debugfs/rd which continues to dump all submits, and $debugfs/hangrd which will be used to dump just submits that cause gpu hangs (and eventually faults, but that will require some iommu framework enhancements).

Signed-off-by Rob Clark <[email protected]>

---
 drivers/gpu/drm/msm/msm_drv.h |   5 ++-
 drivers/gpu/drm/msm/msm_gpu.c |   2 +-
 drivers/gpu/drm/msm/msm_rd.c  | 101 +++++++++++++++++++++++++++++++-----------
 3 files changed, 79 insertions(+), 29 deletions(-)

drivers/gpu/drm/msm/msm_drv.h +3 -2

index 666fce66f9dd..b01bd7fa9c2b 100644
@@ -108,7 +108,8 @@ struct msm_drm_private {
 
 	struct drm_fb_helper *fbdev;
 
-	struct msm_rd_state *rd;
+	struct msm_rd_state *rd;       /* debugfs to dump all submits */
+	struct msm_rd_state *hangrd;   /* debugfs to dump hanging submits */
 	struct msm_perf_state *perf;
 
 	/* list of GEM objects: */

@@ -298,7 +299,7 @@ void msm_framebuffer_describe(struct drm_framebuffer *fb, struct seq_file *m);
 int msm_debugfs_late_init(struct drm_device *dev);
 int msm_rd_debugfs_init(struct drm_minor *minor);
 void msm_rd_debugfs_cleanup(struct msm_drm_private *priv);
-void msm_rd_dump_submit(struct msm_gem_submit *submit);
+void msm_rd_dump_submit(struct msm_rd_state *rd, struct msm_gem_submit *submit);
 int msm_perf_debugfs_init(struct drm_minor *minor);
 void msm_perf_debugfs_cleanup(struct msm_drm_private *priv);
 #else

drivers/gpu/drm/msm/msm_gpu.c +1 -1

index d26a7282466e..5baeb34f098e 100644
@@ -556,7 +556,7 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit,
 
 	list_add_tail(&submit->node, &ring->submits);
 
-	msm_rd_dump_submit(submit);
+	msm_rd_dump_submit(priv->rd, submit);
 
 	update_sw_cntrs(gpu);

drivers/gpu/drm/msm/msm_rd.c +75 ~~-26~~

159 lines not on this page · raw message

Signature · 6 lines

2.13.6

_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno

Rob Clark

2017-10-24 13:22 UTC #4 lore raw

PATCH 4/6 drm/msm/rd: allow adding addition msg to top of dump

In reply to Rob Clark

For faults or hangs, it is nice to be able to include a bit more information.

Signed-off-by Rob Clark <[email protected]>

---
 drivers/gpu/drm/msm/msm_drv.h |  3 ++-
 drivers/gpu/drm/msm/msm_gpu.c |  2 +-
 drivers/gpu/drm/msm/msm_rd.c  | 15 +++++++++++++--
 3 files changed, 16 insertions(+), 4 deletions(-)

drivers/gpu/drm/msm/msm_drv.h +2 -1

index b01bd7fa9c2b..a007bbf4d94d 100644
@@ -299,7 +299,8 @@ void msm_framebuffer_describe(struct drm_framebuffer *fb, struct seq_file *m);
 int msm_debugfs_late_init(struct drm_device *dev);
 int msm_rd_debugfs_init(struct drm_minor *minor);
 void msm_rd_debugfs_cleanup(struct msm_drm_private *priv);
-void msm_rd_dump_submit(struct msm_rd_state *rd, struct msm_gem_submit *submit);
+void msm_rd_dump_submit(struct msm_rd_state *rd, struct msm_gem_submit *submit,
+		const char *fmt, ...);
 int msm_perf_debugfs_init(struct drm_minor *minor);
 void msm_perf_debugfs_cleanup(struct msm_drm_private *priv);
 #else

drivers/gpu/drm/msm/msm_gpu.c +1 -1

index 5baeb34f098e..403baea19329 100644
@@ -556,7 +556,7 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit,
 
 	list_add_tail(&submit->node, &ring->submits);
 
-	msm_rd_dump_submit(priv->rd, submit);
+	msm_rd_dump_submit(priv->rd, submit, NULL);
 
 	update_sw_cntrs(gpu);

drivers/gpu/drm/msm/msm_rd.c +13 -2

index d689407c809a..3aa8a8576abe 100644
@@ -346,11 +346,12 @@ static void snapshot_buf(struct msm_rd_state *rd,
 }
 
 /* called under struct_mutex */
-void msm_rd_dump_submit(struct msm_rd_state *rd, struct msm_gem_submit *submit)
+void msm_rd_dump_submit(struct msm_rd_state *rd, struct msm_gem_submit *submit,
+		const char *fmt, ...)
 {
 	struct drm_device *dev = submit->dev;
 	struct task_struct *task;
-	char msg[128];
+	char msg[256];
 	int i, n;
 
 	if (!rd->open)

@@ -361,6 +362,16 @@ void msm_rd_dump_submit(struct msm_rd_state *rd, struct msm_gem_submit *submit)
 	 */
 	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
 
+	if (fmt) {
+		va_list args;
+
+		va_start(args, fmt);
+		n = vsnprintf(msg, sizeof(msg), fmt, args);
+		va_end(args);
+
+		rd_write_section(rd, RD_CMD, msg, ALIGN(n, 4));
+	}
+
 	rcu_read_lock();
 	task = pid_task(submit->pid, PIDTYPE_PID);
 	if (task) {

Signature · 6 lines

2.13.6

_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno

Rob Clark

2017-10-24 13:22 UTC #5 lore raw

PATCH 5/6 drm/msm: preserve IOVAs in submit's bo table

In reply to Rob Clark

We need this if we want to dump the submit after cleanup (ie. from hang or fault). But in the backoff/unpin case we want to clear them. So add a flag so we can skip clearing the IOVAs in at cleanup.

Signed-off-by Rob Clark <[email protected]>

---
 drivers/gpu/drm/msm/msm_gem_submit.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

drivers/gpu/drm/msm/msm_gem_submit.c +6 -5

index 69868682f8f4..b8dc8f96caf2 100644
@@ -161,7 +161,8 @@ static int submit_lookup_objects(struct msm_gem_submit *submit,
 	return ret;
 }
 
-static void submit_unlock_unpin_bo(struct msm_gem_submit *submit, int i)
+static void submit_unlock_unpin_bo(struct msm_gem_submit *submit,
+		int i, bool backoff)
 {
 	struct msm_gem_object *msm_obj = submit->bos[i].obj;

@@ -171,7 +172,7 @@ static void submit_unlock_unpin_bo(struct msm_gem_submit *submit, int i)
 	if (submit->bos[i].flags & BO_LOCKED)
 		ww_mutex_unlock(&msm_obj->resv->lock);
 
-	if (!(submit->bos[i].flags & BO_VALID))
+	if (backoff && !(submit->bos[i].flags & BO_VALID))
 		submit->bos[i].iova = 0;
 
 	submit->bos[i].flags &= ~(BO_LOCKED | BO_PINNED);

@@ -206,10 +207,10 @@ static int submit_lock_objects(struct msm_gem_submit *submit)
 
 fail:
 	for (; i >= 0; i--)
-		submit_unlock_unpin_bo(submit, i);
+		submit_unlock_unpin_bo(submit, i, true);
 
 	if (slow_locked > 0)
-		submit_unlock_unpin_bo(submit, slow_locked);
+		submit_unlock_unpin_bo(submit, slow_locked, true);
 
 	if (ret == -EDEADLK) {
 		struct msm_gem_object *msm_obj = submit->bos[contended].obj;

@@ -393,7 +394,7 @@ static void submit_cleanup(struct msm_gem_submit *submit)
 
 	for (i = 0; i < submit->nr_bos; i++) {
 		struct msm_gem_object *msm_obj = submit->bos[i].obj;
-		submit_unlock_unpin_bo(submit, i);
+		submit_unlock_unpin_bo(submit, i, false);
 		list_del_init(&msm_obj->submit_entry);
 		drm_gem_object_unreference(&msm_obj->base);
 	}

Signature · 6 lines

2.13.6

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

Rob Clark

2017-10-24 13:22 UTC #6 lore raw

PATCH 6/6 drm/msm: dump submits which triggered gpu hang

In reply to Rob Clark

Note we need to move update_fences() to after msm_rd_dump_submit(), otherwise the bo's referenced by the submit may no longer be valid.

Signed-off-by Rob Clark <[email protected]>

---
 drivers/gpu/drm/msm/msm_gpu.c | 52 +++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 22 deletions(-)

drivers/gpu/drm/msm/msm_gpu.c +30 ~~-22~~

index 403baea19329..939ea98908b8 100644
@@ -255,34 +255,16 @@ static void recover_worker(struct work_struct *work)
 {
 	struct msm_gpu *gpu = container_of(work, struct msm_gpu, recover_work);
 	struct drm_device *dev = gpu->dev;
+	struct msm_drm_private *priv = dev->dev_private;
 	struct msm_gem_submit *submit;
 	struct msm_ringbuffer *cur_ring = gpu->funcs->active_ring(gpu);
-	uint64_t fence;
 	int i;
 
-	/* Update all the rings with the latest and greatest fence */
-	for (i = 0; i < ARRAY_SIZE(gpu->rb); i++) {
-		struct msm_ringbuffer *ring = gpu->rb[i];
-
-		fence = ring->memptrs->fence;
-
-		/*
-		 * For the current (faulting?) ring/submit advance the fence by
-		 * one more to clear the faulting submit
-		 */
-		if (ring == cur_ring)
-			fence = fence + 1;
-
-		update_fences(gpu, ring, fence);
-	}
-
 	mutex_lock(&dev->struct_mutex);
 
-
 	dev_err(dev->dev, "%s: hangcheck recover!\n", gpu->name);
-	fence = cur_ring->memptrs->fence + 1;
 
-	submit = find_submit(cur_ring, fence);
+	submit = find_submit(cur_ring, cur_ring->memptrs->fence + 1);
 	if (submit) {
 		struct task_struct *task;

@@ -306,11 +288,37 @@ static void recover_worker(struct work_struct *work)
 			len = get_cmdline(task, buf, sizeof(buf));
 			mutex_lock(&dev->struct_mutex);
 
-			dev_err(dev->dev, "%s: offending task: %s (%-*s)\n",
-					gpu->name, task->comm, len, buf);
+			dev_err(dev->dev, "%s: offending task: %s (%.*s)\n",
+				gpu->name, task->comm, len, buf);
+
+			msm_rd_dump_submit(priv->hangrd, submit,
+				"offending task: %s (%.*s)", task->comm,
+				len, buf);
+		} else {
+			msm_rd_dump_submit(priv->hangrd, submit, NULL);
 		}
 		rcu_read_unlock();
+	}
+
+
+	/*
+	 * Update all the rings with the latest and greatest fence.. this
+	 * needs to happen after msm_rd_dump_submit() to ensure that the
+	 * bo's referenced by the offending submit are still around.
+	 */
+	for (i = 0; i < ARRAY_SIZE(gpu->rb); i++) {
+		struct msm_ringbuffer *ring = gpu->rb[i];
+
+		uint32_t fence = ring->memptrs->fence;
 
+		/*
+		 * For the current (faulting?) ring/submit advance the fence by
+		 * one more to clear the faulting submit
+		 */
+		if (ring == cur_ring)
+			fence++;
+
+		update_fences(gpu, ring, fence);
 	}
 
 	if (msm_gpu_active(gpu)) {

Signature · 6 lines

2.13.6

_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno