drm/msm: Capture and dump the GPU crash state

The i915 DRM driver very cleverly used ascii85 encoding for their GPU state file. Move the encode functions to a general header file to support other drivers that might be interested in the same functionality.

v3: Fix error_puts -> err_puts pointed out by the 01.org bot
v2: Update API to be cleaner for the caller as suggested by Chris Wilson

Signed-off-by Jordan Crouse <[email protected]>

---

Add a drm printer suitable for use with the read callback for devcoredump or other suitable buffer based output format that isn't otherwise covered by seq_file.

Signed-off-by Jordan Crouse <[email protected]>

---
 drivers/gpu/drm/drm_print.c | 74 +++++++++++++++++++++++++++++++++++++
 include/drm/drm_print.h     | 27 ++++++++++++++
 2 files changed, 101 insertions(+)

drivers/gpu/drm/drm_print.c +0 -0

index b25f98f33f6c..03d1f98e5ac7 100644
@@ -30,6 +30,80 @@
 #include <drm/drmP.h>
 #include <drm/drm_print.h>
 

Quoting Jordan Crouse (2018-07-12 19:59:19)

Quoted 18 lines

Add a drm printer suitable for use with the read callback for devcoredump or other suitable buffer based output format that isn't otherwise covered by seq_file.

Signed-off-by Jordan Crouse <[email protected]>

---
 drivers/gpu/drm/drm_print.c | 74 +++++++++++++++++++++++++++++++++++++
 include/drm/drm_print.h     | 27 ++++++++++++++
 2 files changed, 101 insertions(+)

drivers/gpu/drm/drm_print.c +0 -0

index b25f98f33f6c..03d1f98e5ac7 100644
@@ -30,6 +30,80 @@
 #include <drm/drmP.h>
 #include <drm/drm_print.h>
 

drivers/gpu/drm/drm_print.c line 42

Quoted 10 lines

drivers/gpu/drm/drm_print.c +10 -0

+void __drm_printfn_coredump(struct drm_printer *p, struct va_format *vaf)
+{
+       struct drm_print_iterator *iterator = p->arg;
+       ssize_t len;
+
+       if (!iterator->remain)
+               return;
+
+       /* Figure out how big the string will be */
+       len = snprintf(NULL, 0, "%pV", vaf);

I was thinking there's some duplication here (kmalloc + snprintf) that could be reduced to kasprintf here. Is avoiding that allocation important or frequent enough to merit open coding?

It's pity the kernel's printk doesn't support %n, so that leaves with

buf = kasprintf(GFP_... , "%pV", vaf);
if (!buf)

	return;

len = strlen(buf);

and even the copy + increment looks like it can then be factored to share more code.

Quoting Jordan Crouse (2018-07-12 19:59:19)

Quoted 18 lines

Add a drm printer suitable for use with the read callback for devcoredump or other suitable buffer based output format that isn't otherwise covered by seq_file.

Signed-off-by Jordan Crouse <[email protected]>

---
 drivers/gpu/drm/drm_print.c | 74 +++++++++++++++++++++++++++++++++++++
 include/drm/drm_print.h     | 27 ++++++++++++++
 2 files changed, 101 insertions(+)

drivers/gpu/drm/drm_print.c +0 -0

index b25f98f33f6c..03d1f98e5ac7 100644
@@ -30,6 +30,80 @@
 #include <drm/drmP.h>
 #include <drm/drm_print.h>
 

drivers/gpu/drm/drm_print.c line 42

Quoted 10 lines

drivers/gpu/drm/drm_print.c +10 -0

+void __drm_printfn_coredump(struct drm_printer *p, struct va_format *vaf)
+{
+       struct drm_print_iterator *iterator = p->arg;
+       ssize_t len;
+
+       if (!iterator->remain)
+               return;
+
+       /* Figure out how big the string will be */
+       len = snprintf(NULL, 0, "%pV", vaf);

I was thinking there's some duplication here (kmalloc + snprintf) that could be reduced to kasprintf here. Is avoiding that allocation important or frequent enough to merit open coding?

It's pity the kernel's printk doesn't support %n, so that leaves with

buf = kasprintf(GFP_... , "%pV", vaf);
if (!buf)

	return;

len = strlen(buf);

and even the copy + increment looks like it can then be factored to share more code.

Add a drm printer suitable for use with the read callback for devcoredump or other suitable buffer based output format that isn't otherwise covered by seq_file.

Signed-off-by Jordan Crouse <[email protected]>

---
 drivers/gpu/drm/drm_print.c | 74 +++++++++++++++++++++++++++++++++++++
 include/drm/drm_print.h     | 27 ++++++++++++++
 2 files changed, 101 insertions(+)

drivers/gpu/drm/drm_print.c +74 -0

index b25f98f33f6c..03d1f98e5ac7 100644
@@ -30,6 +30,80 @@
 #include <drm/drmP.h>
 #include <drm/drm_print.h>
 
+void __drm_printfn_coredump(struct drm_printer *p, struct va_format *vaf)
+{
+ struct drm_print_iterator *iterator = p->arg;
+ ssize_t len;
+
+ if (!iterator->remain)
+ return;
+
+ /* Figure out how big the string will be */
+ len = snprintf(NULL, 0, "%pV", vaf);
+
+ if (iterator->offset < iterator->start) {
+ char *buf;
+ ssize_t copy;
+
+ if (iterator->offset + len <= iterator->start) {
+ iterator->offset += len;
+ return;
+ }
+
+ /* Print the string into a temporary buffer */
+ buf = kmalloc(len + 1,
+ GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY);
+ if (!buf)
+ return;
+
+ snprintf(buf, len + 1, "%pV", vaf);
+
+ copy = len - (iterator->start - iterator->offset);
+
+ if (copy > iterator->remain)
+ copy = iterator->remain;
+
+ /* Copy out the bit of the string that we need */
+ memcpy(iterator->data,
+ buf + (iterator->start - iterator->offset), copy);
+
+ iterator->offset = iterator->start + copy;
+ iterator->remain -= copy;
+
+ kfree(buf);
+ } else {
+ char *buf;
+ ssize_t pos = iterator->offset - iterator->start;
+
+ if (len < iterator->remain) {
+ snprintf(((char *) iterator->data) + pos,
+ iterator->remain, "%pV", vaf);
+
+ iterator->offset += len;
+ iterator->remain -= len;
+
+ return;
+ }
+
+ /* Print the string into a temporary buffer */
+ buf = kmalloc(len + 1,
+ GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY);
+ if (!buf)
+ return;
+
+ snprintf(buf, len + 1, "%pV", vaf);
+
+ /* Copy out the remaining bits */
+ memcpy(iterator->data + pos, buf, iterator->remain);
+
+ iterator->offset += iterator->remain;
+ iterator->remain = 0;
+
+ kfree(buf);
+ }
+}
+EXPORT_SYMBOL(__drm_printfn_coredump);
+
 void __drm_printfn_seq_file(struct drm_printer *p, struct va_format *vaf)
 {
  seq_printf(p->arg, "%pV", vaf);

include/drm/drm_print.h +3 -0

index e1a46e9991cc..0ea440fb5ec3 100644
@@ -73,6 +73,7 @@ struct drm_printer {
  const char *prefix;
 };
 
+void __drm_printfn_coredump(struct drm_printer *p, struct va_format *vaf);
 void __drm_printfn_seq_file(struct drm_printer *p, struct va_format *vaf);
 void __drm_printfn_info(struct drm_printer *p, struct va_format *vaf);
 void __drm_printfn_debug(struct drm_printer *p, struct va_format *vaf);

@@ -104,6 +105,32 @@ drm_vprintf(struct drm_printer *p, const char *fmt, va_list *va)
 #define drm_printf_indent(printer, indent, fmt, ...) \
  drm_printf((printer), "%.*s" fmt, (indent), "\t\t\t\t\tX", ##__VA_ARGS__)
 
+struct drm_print_iterator {
+ void *data;

include/drm/drm_print.h +14 -1

+ *
+ * RETURNS:
+ * The &drm_printer object
+ */
+static inline struct drm_printer
+drm_coredump_printer(struct drm_print_iterator *iter)
+{
+ struct drm_printer p = {
+ .printfn = __drm_printfn_coredump,
+ .arg = iter,
+ };
+ return p;
+}
+
 /**
  * drm_seq_file_printer - construct a &drm_printer that outputs to &seq_file
  * @f:  the &struct seq_file to output to
--

2.17.1

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

Quoted 4 lines · Jordan Crouse

Add a drm printer suitable for use with the read callback for devcoredump or other suitable buffer based output format that isn't otherwise covered by seq_file.

Quoted 1 line

Signed-off-by Jordan Crouse <[email protected]>

Hm, why not add seq_file support to dev_coredump? Neither git blame nor google sched any light on why seq_file wasn't picked over the custom read interface ...

Adding Johannes and Greg about this.

If we go with this, one comment below.

Quoted 108 lines

---
 drivers/gpu/drm/drm_print.c | 74 +++++++++++++++++++++++++++++++++++++
 include/drm/drm_print.h     | 27 ++++++++++++++
 2 files changed, 101 insertions(+)

drivers/gpu/drm/drm_print.c +74 -0

index b25f98f33f6c..03d1f98e5ac7 100644
@@ -30,6 +30,80 @@
 #include <drm/drmP.h>
 #include <drm/drm_print.h>
 
+void __drm_printfn_coredump(struct drm_printer *p, struct va_format *vaf)
+{
+ struct drm_print_iterator *iterator = p->arg;
+ ssize_t len;
+
+ if (!iterator->remain)
+ return;
+
+ /* Figure out how big the string will be */
+ len = snprintf(NULL, 0, "%pV", vaf);
+
+ if (iterator->offset < iterator->start) {
+ char *buf;
+ ssize_t copy;
+
+ if (iterator->offset + len <= iterator->start) {
+ iterator->offset += len;
+ return;
+ }
+
+ /* Print the string into a temporary buffer */
+ buf = kmalloc(len + 1,
+ GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY);
+ if (!buf)
+ return;
+
+ snprintf(buf, len + 1, "%pV", vaf);
+
+ copy = len - (iterator->start - iterator->offset);
+
+ if (copy > iterator->remain)
+ copy = iterator->remain;
+
+ /* Copy out the bit of the string that we need */
+ memcpy(iterator->data,
+ buf + (iterator->start - iterator->offset), copy);
+
+ iterator->offset = iterator->start + copy;
+ iterator->remain -= copy;
+
+ kfree(buf);
+ } else {
+ char *buf;
+ ssize_t pos = iterator->offset - iterator->start;
+
+ if (len < iterator->remain) {
+ snprintf(((char *) iterator->data) + pos,
+ iterator->remain, "%pV", vaf);
+
+ iterator->offset += len;
+ iterator->remain -= len;
+
+ return;
+ }
+
+ /* Print the string into a temporary buffer */
+ buf = kmalloc(len + 1,
+ GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY);
+ if (!buf)
+ return;
+
+ snprintf(buf, len + 1, "%pV", vaf);
+
+ /* Copy out the remaining bits */
+ memcpy(iterator->data + pos, buf, iterator->remain);
+
+ iterator->offset += iterator->remain;
+ iterator->remain = 0;
+
+ kfree(buf);
+ }
+}
+EXPORT_SYMBOL(__drm_printfn_coredump);
+
 void __drm_printfn_seq_file(struct drm_printer *p, struct va_format *vaf)
 {
  seq_printf(p->arg, "%pV", vaf);

include/drm/drm_print.h +3 -0

index e1a46e9991cc..0ea440fb5ec3 100644
@@ -73,6 +73,7 @@ struct drm_printer {
  const char *prefix;
 };
 
+void __drm_printfn_coredump(struct drm_printer *p, struct va_format *vaf);
 void __drm_printfn_seq_file(struct drm_printer *p, struct va_format *vaf);
 void __drm_printfn_info(struct drm_printer *p, struct va_format *vaf);
 void __drm_printfn_debug(struct drm_printer *p, struct va_format *vaf);

@@ -104,6 +105,32 @@ drm_vprintf(struct drm_printer *p, const char *fmt, va_list *va)
 #define drm_printf_indent(printer, indent, fmt, ...) \
  drm_printf((printer), "%.*s" fmt, (indent), "\t\t\t\t\tX", ##__VA_ARGS__)
 
+struct drm_print_iterator {
+ void *data;

include/drm/drm_print.h drm_vprintf(struct drm_printer *p, const char *fmt, va_list *va) · line 119

Quoted 10 lines

include/drm/drm_print.h +10 -0

+
+ ssize_t start;
+ ssize_t offset;
+ ssize_t remain;
+};
+
+/**
+ * drm_coredump_printer - construct a &drm_printer that can output to a buffer
+ * from the read function for devcoredump
+ * @iter: A pointer to a struct drm_print_iterator for the read instance

Bit more flesh for the kerneldoc would be good here, maybe even with a small in-line example. Definitely a link to dev_coredumpm() which I assume is the function you're going to use this with.

Pls also make sure it all looks nice using make htmldocs.

-Daniel

Quoted 24 lines

include/drm/drm_print.h +14 -1

+ *
+ * RETURNS:
+ * The &drm_printer object
+ */
+static inline struct drm_printer
+drm_coredump_printer(struct drm_print_iterator *iter)
+{
+ struct drm_printer p = {
+ .printfn = __drm_printfn_coredump,
+ .arg = iter,
+ };
+ return p;
+}
+
 /**
  * drm_seq_file_printer - construct a &drm_printer that outputs to &seq_file
  * @f:  the &struct seq_file to output to
--

2.17.1

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

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Quoted 5 lines

Hm, why not add seq_file support to dev_coredump? Neither git blame nor google sched any light on why seq_file wasn't picked over the custom read interface ...

Adding Johannes and Greg about this.

Main reason was that this is used for devcoredump which has its own similar but not quite seq_file compatible callback. If there is synergy to be had there that would be great because reinventing the wheel isn't fun.

Add drm_puts() for a much faster path to print constant strings into a drm_printer object with memcpy and friends. This can shave seconds off of really large outputs such as GPU dumps.

If the drm_printer object supports a custom puts function then use that otherwise fall back to the slower legacy printf call.

Signed-off-by Jordan Crouse <[email protected]>

---
 drivers/gpu/drm/drm_print.c | 9 +++++++++
 include/drm/drm_print.h     | 2 ++
 2 files changed, 11 insertions(+)

drivers/gpu/drm/drm_print.c +0 -0

index 03d1f98e5ac7..8fd489248a50 100644

drivers/gpu/drm/drm_print.c +9 -0

+void drm_puts(struct drm_printer *p, const char *str)
+{
+	if (p->puts)
+		p->puts(p, str);
+	else
+		drm_printf(p, "%s", str);
+}
+EXPORT_SYMBOL(drm_puts);
+
 /**
  * drm_printf - print to a &drm_printer stream
  * @p: the &drm_printer

include/drm/drm_print.h +2 -0

index 0ea440fb5ec3..b16f4ecaa984 100644
@@ -69,6 +69,7 @@
 struct drm_printer {
 	/* private: */
 	void (*printfn)(struct drm_printer *p, struct va_format *vaf);
+	void (*puts)(struct drm_printer *p, const char *str);
 	void *arg;
 	const char *prefix;
 };

@@ -80,6 +81,7 @@ void __drm_printfn_debug(struct drm_printer *p, struct va_format *vaf);
 
 __printf(2, 3)
 void drm_printf(struct drm_printer *p, const char *f, ...);
+void drm_puts(struct drm_printer *p, const char *str);
 
 __printf(2, 0)
 /**

Signature · 6 lines

2.17.1

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

Add a puts() function to use seq_puts() to help speed up up print time for constant strings.

Signed-off-by Jordan Crouse <[email protected]>

---
 drivers/gpu/drm/drm_print.c | 6 ++++++
 include/drm/drm_print.h     | 2 ++
 2 files changed, 8 insertions(+)

drivers/gpu/drm/drm_print.c +6 -0

index 8fd489248a50..bef8f0ec5d73 100644
@@ -104,6 +104,12 @@ void __drm_printfn_coredump(struct drm_printer *p, struct va_format *vaf)
 }
 EXPORT_SYMBOL(__drm_printfn_coredump);
 
+void __drm_puts_seq_file(struct drm_printer *p, const char *str)
+{
+	seq_puts(p->arg, str);
+}
+EXPORT_SYMBOL(__drm_puts_seq_file);
+
 void __drm_printfn_seq_file(struct drm_printer *p, struct va_format *vaf)
 {
 	seq_printf(p->arg, "%pV", vaf);

include/drm/drm_print.h +2 -0

index b16f4ecaa984..3bc6ba4b7b2c 100644
@@ -76,6 +76,7 @@ struct drm_printer {
 
 void __drm_printfn_coredump(struct drm_printer *p, struct va_format *vaf);
 void __drm_printfn_seq_file(struct drm_printer *p, struct va_format *vaf);
+void __drm_puts_seq_file(struct drm_printer *p, const char *str);
 void __drm_printfn_info(struct drm_printer *p, struct va_format *vaf);
 void __drm_printfn_debug(struct drm_printer *p, struct va_format *vaf);
 

@@ -144,6 +145,7 @@ static inline struct drm_printer drm_seq_file_printer(struct seq_file *f)
 {
 	struct drm_printer p = {
 		.printfn = __drm_printfn_seq_file,
+		.puts = __drm_puts_seq_file,
 		.arg = f,
 	};
 	return p;

Signature · 6 lines

2.17.1

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

Add a put function for the coredump printer to bypass printf() for constant strings for a speed boost.

v2: Add EXPORT_SYMBOL for _drm_puts_coredump

Signed-off-by Jordan Crouse <[email protected]>

---
 drivers/gpu/drm/drm_print.c | 43 +++++++++++++++++++++++++++++++++++++
 include/drm/drm_print.h     |  2 ++
 2 files changed, 45 insertions(+)

drivers/gpu/drm/drm_print.c +4 -0

index bef8f0ec5d73..ff20f4a764c8 100644
@@ -30,6 +30,49 @@
 #include <drm/drmP.h>
 #include <drm/drm_print.h>
 
+void __drm_puts_coredump(struct drm_printer *p, const char *str)
+{
+       struct drm_print_iterator *iterator = p->arg;
+

Add a put function for the coredump printer to bypass printf() for constant strings for a speed boost.

v2: Add EXPORT_SYMBOL for _drm_puts_coredump

Signed-off-by Jordan Crouse <[email protected]>

---
 drivers/gpu/drm/drm_print.c | 43 +++++++++++++++++++++++++++++++++++++
 include/drm/drm_print.h     |  2 ++
 2 files changed, 45 insertions(+)

drivers/gpu/drm/drm_print.c +43 -0

index bef8f0ec5d73..ff20f4a764c8 100644
@@ -30,6 +30,49 @@
 #include <drm/drmP.h>
 #include <drm/drm_print.h>
 
+void __drm_puts_coredump(struct drm_printer *p, const char *str)
+{
+       struct drm_print_iterator *iterator = p->arg;
+
+       ssize_t len;
+
+       if (!iterator->remain)
+               return;
+
+       if (iterator->offset < iterator->start) {
+               ssize_t copy;
+
+               len = strlen(str);
+
+               if (iterator->offset + len <= iterator->start) {
+                       iterator->offset += len;
+                       return;
+               }
+
+               copy = len - (iterator->start - iterator->offset);
+
+               if (copy > iterator->remain)
+                       copy = iterator->remain;
+
+               /* Copy out the bit of the string that we need */
+               memcpy(iterator->data,
+                       str + (iterator->start - iterator->offset), copy);
+
+               iterator->offset = iterator->start + copy;
+               iterator->remain -= copy;
+       } else {
+               ssize_t pos = iterator->offset - iterator->start;
+
+               len = min_t(ssize_t, strlen(str), iterator->remain);
+
+               memcpy(iterator->data + pos, str, len);
+
+               iterator->offset += len;
+               iterator->remain -= len;
+       }
+}
+EXPORT_SYMBOL(__drm_puts_coredump);
+
 void __drm_printfn_coredump(struct drm_printer *p, struct va_format *vaf)
 {
        struct drm_print_iterator *iterator = p->arg;

include/drm/drm_print.h +2 -1

index 3bc6ba4b7b2c..2a903ee7b428 100644
@@ -75,6 +75,7 @@ struct drm_printer {
 };
 
 void __drm_printfn_coredump(struct drm_printer *p, struct va_format *vaf);
+void __drm_puts_coredump(struct drm_printer *p, const char *str);
 void __drm_printfn_seq_file(struct drm_printer *p, struct va_format *vaf);
 void __drm_puts_seq_file(struct drm_printer *p, const char *str);
 void __drm_printfn_info(struct drm_printer *p, struct va_format *vaf);

@@ -129,6 +130,7 @@ drm_coredump_printer(struct drm_print_iterator *iter)
 {
        struct drm_printer p = {
                .printfn = __drm_printfn_coredump,
+               .puts = __drm_puts_coredump,
                .arg = iter,
        };
        return p;
--

2.17.1

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

Convert the existing GPU show function to use the GPU state to dump the information rather than reading it directly from the hardware. This will require an additional step to capture the state before dumping it for the existing nodes but it will greatly facilitate reusing the same code for dumping a previously captured state from a GPU hang.

Signed-off-by Jordan Crouse <[email protected]>

---
 drivers/gpu/drm/msm/adreno/a3xx_gpu.c   | 11 +--
 drivers/gpu/drm/msm/adreno/a4xx_gpu.c   | 12 +---
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c   | 18 +----
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 30 ++++----
 drivers/gpu/drm/msm/adreno/adreno_gpu.h |  3 +-
 drivers/gpu/drm/msm/msm_debugfs.c       | 92 ++++++++++++++++++++++---
 drivers/gpu/drm/msm/msm_gpu.h           |  3 +-
 7 files changed, 104 insertions(+), 65 deletions(-)

drivers/gpu/drm/msm/adreno/a3xx_gpu.c +1 -10

index b707b5bca9ab..4cffec2b6adc 100644
@@ -411,15 +411,6 @@ static const unsigned int a3xx_registers[] = {
        ~0   /* sentinel */
 };
 
-#ifdef CONFIG_DEBUG_FS
-static void a3xx_show(struct msm_gpu *gpu, struct seq_file *m)
-{
-       seq_printf(m, "status:   %08x\n",
-                       gpu_read(gpu, REG_A3XX_RBBM_STATUS));
-       adreno_show(gpu, m);
-}
-#endif
-
 /* would be nice to not have to duplicate the _show() stuff with printk(): */
 static void a3xx_dump(struct msm_gpu *gpu)
 {

@@ -464,7 +455,7 @@ static const struct adreno_gpu_funcs funcs = {
                .irq = a3xx_irq,
                .destroy = a3xx_destroy,
 #ifdef CONFIG_DEBUG_FS
-               .show = a3xx_show,
+               .show = adreno_show,
 #endif
                .gpu_state_get = a3xx_gpu_state_get,
                .gpu_state_put = adreno_gpu_state_put,

drivers/gpu/drm/msm/adreno/a4xx_gpu.c +1 -11

index 17e97ebc1077..95f08c22e8d7 100644
@@ -455,16 +455,6 @@ static const unsigned int a4xx_registers[] = {
        ~0 /* sentinel */
 };
 
-#ifdef CONFIG_DEBUG_FS
-static void a4xx_show(struct msm_gpu *gpu, struct seq_file *m)
-{
-       seq_printf(m, "status:   %08x\n",
-                       gpu_read(gpu, REG_A4XX_RBBM_STATUS));
-       adreno_show(gpu, m);
-
-}
-#endif
-
 static struct msm_gpu_state *a4xx_gpu_state_get(struct msm_gpu *gpu)
 {
        struct msm_gpu_state *state = adreno_gpu_state_get(gpu);

@@ -551,7 +541,7 @@ static const struct adreno_gpu_funcs funcs = {
                .irq = a4xx_irq,
                .destroy = a4xx_destroy,
 #ifdef CONFIG_DEBUG_FS
-               .show = a4xx_show,
+               .show = adreno_show,
 #endif
                .gpu_state_get = a4xx_gpu_state_get,
                .gpu_state_put = adreno_gpu_state_put,

drivers/gpu/drm/msm/adreno/a5xx_gpu.c +1 -17

index 9e85e4f7016d..5f1aab3c1cb1 100644
@@ -1215,22 +1215,6 @@ static struct msm_gpu_state *a5xx_gpu_state_get(struct msm_gpu *gpu)
        return state;
 }
 
-#ifdef CONFIG_DEBUG_FS
-static void a5xx_show(struct msm_gpu *gpu, struct seq_file *m)
-{
-       seq_printf(m, "status:   %08x\n",
-                       gpu_read(gpu, REG_A5XX_RBBM_STATUS));
-
-       /*
-        * Temporarily disable hardware clock gating before going into
-        * adreno_show to avoid issues while reading the registers
-        */
-       a5xx_set_hwcg(gpu, false);
-       adreno_show(gpu, m);
-       a5xx_set_hwcg(gpu, true);
-}
-#endif
-
 static struct msm_ringbuffer *a5xx_active_ring(struct msm_gpu *gpu)
 {
        struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);

@@ -1260,7 +1244,7 @@ static const struct adreno_gpu_funcs funcs = {
                .irq = a5xx_irq,
                .destroy = a5xx_destroy,
 #ifdef CONFIG_DEBUG_FS
-               .show = a5xx_show,
+               .show = adreno_show,
                .debugfs_init = a5xx_debugfs_init,
 #endif
                .gpu_busy = a5xx_gpu_busy,

drivers/gpu/drm/msm/adreno/adreno_gpu.c +13 -17

index c7a0d278c59e..0e937eedcec5 100644
@@ -423,38 +423,34 @@ void adreno_gpu_state_put(struct msm_gpu_state *state)
 }
 
 #ifdef CONFIG_DEBUG_FS
-void adreno_show(struct msm_gpu *gpu, struct seq_file *m)
+void adreno_show(struct msm_gpu *gpu, struct msm_gpu_state *state,
+               struct seq_file *m)
 {
        struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
        int i;
 
+       if (IS_ERR_OR_NULL(state))
+               return;
+
+       seq_printf(m, "status:   %08x\n", state->rbbm_status);
        seq_printf(m, "revision: %d (%d.%d.%d.%d)\n",
                        adreno_gpu->info->revn, adreno_gpu->rev.core,
                        adreno_gpu->rev.major, adreno_gpu->rev.minor,
                        adreno_gpu->rev.patchid);
 
        for (i = 0; i < gpu->nr_rings; i++) {
-               struct msm_ringbuffer *ring = gpu->rb[i];
-
                seq_printf(m, "rb %d: fence:    %d/%d\n", i,
-                       ring->memptrs->fence, ring->seqno);
+                       state->ring[i].fence, state->ring[i].seqno);
 
-               seq_printf(m, "      rptr:     %d\n",
-                       get_rptr(adreno_gpu, ring));
-               seq_printf(m, "rb wptr:  %d\n", get_wptr(ring));
+               seq_printf(m, "      rptr:     %d\n", state->ring[i].rptr);
+               seq_printf(m, "rb wptr:  %d\n", state->ring[i].wptr);
        }
 
-       /* dump these out in a form that can be parsed by demsm: */
        seq_printf(m, "IO:region %s 00000000 00020000\n", gpu->name);
-       for (i = 0; adreno_gpu->registers[i] != ~0; i += 2) {
-               uint32_t start = adreno_gpu->registers[i];
-               uint32_t end   = adreno_gpu->registers[i+1];
-               uint32_t addr;
-
-               for (addr = start; addr <= end; addr++) {
-                       uint32_t val = gpu_read(gpu, addr);
-                       seq_printf(m, "IO:R %08x %08x\n", addr<<2, val);
-               }
+       for (i = 0; i < state->nr_registers; i++) {
+               seq_printf(m, "IO:R %08x %08x\n",
+                       state->registers[i * 2] << 2,
+                       state->registers[(i * 2) + 1]);
        }
 }
 #endif

drivers/gpu/drm/msm/adreno/adreno_gpu.h +2 -1

index 734e31a9631f..90b6b59252af 100644
@@ -216,7 +216,8 @@ void adreno_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit,
 void adreno_flush(struct msm_gpu *gpu, struct msm_ringbuffer *ring);
 bool adreno_idle(struct msm_gpu *gpu, struct msm_ringbuffer *ring);
 #ifdef CONFIG_DEBUG_FS
-void adreno_show(struct msm_gpu *gpu, struct seq_file *m);
+void adreno_show(struct msm_gpu *gpu, struct msm_gpu_state *state,
+               struct seq_file *m);
 #endif
 void adreno_dump_info(struct msm_gpu *gpu);
 void adreno_dump(struct msm_gpu *gpu);

drivers/gpu/drm/msm/msm_debugfs.c +6 -1

index 1ff3fda245d1..c3da12179888 100644
@@ -16,26 +16,100 @@
  */
 
 #ifdef CONFIG_DEBUG_FS
+#include <linux/debugfs.h>
 #include "msm_drv.h"
 #include "msm_gpu.h"
 #include "msm_kms.h"
 #include "msm_debugfs.h"
 
-static int msm_gpu_show(struct drm_device *dev, struct seq_file *m)
+struct msm_gpu_show_priv {
+       struct msm_gpu_state *state;
+       struct drm_device *dev;
+};
+

Do a bit of cleanup to prepare for upcoming changes to pass the hanging task comm and cmdline to the crash dump function.

Signed-off-by Jordan Crouse <[email protected]>

---
 drivers/gpu/drm/msm/msm_gpu.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

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

index 1c09acfb4028..2ca354047250 100644
@@ -314,6 +314,7 @@ static void recover_worker(struct work_struct *work)
        struct msm_drm_private *priv = dev->dev_private;
        struct msm_gem_submit *submit;
        struct msm_ringbuffer *cur_ring = gpu->funcs->active_ring(gpu);
+       char *comm = NULL, *cmd = NULL;
        int i;
 
        mutex_lock(&dev->struct_mutex);

Quoting Jordan Crouse (2018-07-12 19:59:25)

Quoted 19 lines

Do a bit of cleanup to prepare for upcoming changes to pass the hanging task comm and cmdline to the crash dump function.

Signed-off-by Jordan Crouse <[email protected]>

---
 drivers/gpu/drm/msm/msm_gpu.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

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

index 1c09acfb4028..2ca354047250 100644
@@ -314,6 +314,7 @@ static void recover_worker(struct work_struct *work)
        struct msm_drm_private *priv = dev->dev_private;
        struct msm_gem_submit *submit;
        struct msm_ringbuffer *cur_ring = gpu->funcs->active_ring(gpu);
+       char *comm = NULL, *cmd = NULL;
        int i;
 

drivers/gpu/drm/msm/msm_gpu.c static void recover_worker(struct work_struct *work) · line 331

Quoted 7 lines

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

        mutex_lock(&dev->struct_mutex);

@@ -327,7 +328,7 @@ static void recover_worker(struct work_struct *work)
                rcu_read_lock();
                task = pid_task(submit->pid, PIDTYPE_PID);
                if (task) {
-                       char *cmd;
+                       comm = kstrdup(task->comm, GFP_KERNEL);

Under rcu_read_lock(), GFP_KERNEL is not allowed, you need GFP_NOWAIT or some such (or grab a reference to the pid and drop rcu then GFP_KERNEL).

Quoted 21 lines · Chris Wilson

Quoting Jordan Crouse (2018-07-12 19:59:25)

Quoted 19 lines

Do a bit of cleanup to prepare for upcoming changes to pass the hanging task comm and cmdline to the crash dump function.

Signed-off-by Jordan Crouse <[email protected]>

---
  drivers/gpu/drm/msm/msm_gpu.c | 18 ++++++++++--------
  1 file changed, 10 insertions(+), 8 deletions(-)

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

index 1c09acfb4028..2ca354047250 100644
@@ -314,6 +314,7 @@ static void recover_worker(struct work_struct *work)
         struct msm_drm_private *priv = dev->dev_private;
         struct msm_gem_submit *submit;
         struct msm_ringbuffer *cur_ring = gpu->funcs->active_ring(gpu);
+       char *comm = NULL, *cmd = NULL;
         int i;
 

Quoted 10 lines

drivers/gpu/drm/msm/msm_gpu.c static void recover_worker(struct work_struct *work) · line 331

Quoted 7 lines

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

         mutex_lock(&dev->struct_mutex);

@@ -327,7 +328,7 @@ static void recover_worker(struct work_struct *work)
                 rcu_read_lock();
                 task = pid_task(submit->pid, PIDTYPE_PID);
                 if (task) {
-                       char *cmd;
+                       comm = kstrdup(task->comm, GFP_KERNEL);

Under rcu_read_lock(), GFP_KERNEL is not allowed, you need GFP_NOWAIT or some such (or grab a reference to the pid and drop rcu then GFP_KERNEL).

I started looking at a similar issue w/ our use of kstrdup_quotable_cmdline() under rcu_read_lock().. I *guess* I hadn't noticed it before due to different RCU kconfig?

I can use GFP_ATOMIC, and I can fix kstrdup_quotable_cmdline() to actually use gfp flags passed in for kmalloc() (and similar bug in kstrdup_quotable_file()).. but get_cmdline() still grabs mmap_sem which complains under rcu_read_lock()..

is there any way to ensure the tast_struct sticks around long enough to get it's cmdline without holding rcu_read_lock()? I couldn't find any refcnt'ing on task_struct itself, which makes this seem a bit unsolveable :-/

BR,
-R
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Convert the format of the 'show' debugfs file and the crash dump to a format resembling YAML. This should be easier to parse and be more flexible for future changes and expansions.

Signed-off-by Jordan Crouse <[email protected]>

---
 Documentation/gpu/drm-msm-crash-dump.txt | 46 ++++++++++++++++++++++++

 drivers/gpu/drm/msm/adreno/adreno_gpu.c  | 20 ++++++-----
 2 files changed, 58 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/gpu/drm-msm-crash-dump.txt

Documentation/gpu/drm-msm-crash-dump.txt new file +46 -0

new file mode 100644
index 000000000000..930e4c970a62
@@ -0,0 +1,46 @@
+# drm/msm GPU crash dump format
+#
+# This is a description of the format of the drm/msm GPU crash dump format that
+# can be read from /sys/kernel/dri/X/show or from devcoredump following a GPU
+# hang or fault
+
+---
+kernel:		# [string] The kernel version as printed by UTS_RELEASE
+module:		# [string] The module that generated the crash dump
+time:		# [seconds.microseconds] The kernel time at crash
+comm:		# [string] comm string for the binary that generated the fault
+		# (if known)
+cmdline:	# [string] the cmdline for the binary that generated the fault
+		# (if known)
+revision:	# [ id core.major.minor.patchlevel] The GPU id followed by the
+		# individual components of the id separated by dots
+rbbm-status:	# [hex] The current value of RBBM_STATUS which shows what GPU
+		# components were in use at the time of the crash
+ringbuffer:	# Ringbuffer data. There will be a sequence for each ringbuffer
+  -id:			# [decimal] Ringbuffer identifier (0 based index)
+   last-fence:		# [decimal] The last fence issued on the ring
+   retired-fence:	# [decimal] THe last fence retired on the ring
+   rptr:		# [decimal] The current read pointer (rptr) for the ring
+   wptr:		# [decimal] The current write pointer (wptr) for the
+			# ring
+   size:		# [decimal] The maximum size of the ring programmed in
+			# the hardware
+   data:		# [ascii85] The contents of the ring encoded as ascii85.
+			# Only the unused portions of the ring will be printed
+			# (up to a maximum of 'size' bytes)
+bos:		# List of buffers from the hanging submission (if known)
+  -iova:		# [hex] GPU address of the buffer
+   size:		# [decimal] Size of the buffer (in bytes)
+   data:		# [ascii85] The contents of the buffer encoded as
+			# ascii85. Only the contents of buffers marked as
+			# readable are dumped. Trailing zeros at the end of the
+			# buffer won't be dumped.
+registers:	# Sets of register values. This section can be used multiple
+		# times for different ranges of registers. Each register will be
+		# on its own line.
+  - [offset, value]	# offset: [hex] byte offset of the register
+			# value: [hex] value of the register
+
+registers-hlsq: # (5xx only) Same format as registers. Register data that
+		# only accessible from the HLSQ aperture captured by the
+		# HW based crashdumper

drivers/gpu/drm/msm/adreno/adreno_gpu.c +12 -8

index 163542487e2c..15fe0d029ba6 100644
@@ -444,23 +444,27 @@ void adreno_show(struct msm_gpu *gpu, struct msm_gpu_state *state,
 	if (IS_ERR_OR_NULL(state))
 		return;
 
-	drm_printf(p, "status:   %08x\n", state->rbbm_status);
 	drm_printf(p, "revision: %d (%d.%d.%d.%d)\n",
 			adreno_gpu->info->revn, adreno_gpu->rev.core,
 			adreno_gpu->rev.major, adreno_gpu->rev.minor,
 			adreno_gpu->rev.patchid);
 
-	for (i = 0; i < gpu->nr_rings; i++) {
-		drm_printf(p, "rb %d: fence:    %d/%d\n", i,
-			state->ring[i].fence, state->ring[i].seqno);
+	drm_printf(p, "rbbm-status: 0x%08x\n", state->rbbm_status);
+
+	drm_puts(p, "ringbuffer:\n");
 
-		drm_printf(p, "      rptr:     %d\n", state->ring[i].rptr);
-		drm_printf(p, "rb wptr:  %d\n", state->ring[i].wptr);
+	for (i = 0; i < gpu->nr_rings; i++) {
+		drm_printf(p, "  - id: %d\n", i);
+		drm_printf(p, "    last-fence: %d\n", state->ring[i].seqno);
+		drm_printf(p, "    retired-fence: %d\n", state->ring[i].fence);
+		drm_printf(p, "    rptr: %d\n", state->ring[i].rptr);
+		drm_printf(p, "    wptr: %d\n", state->ring[i].wptr);
 	}
 
-	drm_printf(p, "IO:region %s 00000000 00020000\n", gpu->name);
+	drm_puts(p, "registers:\n");
+
 	for (i = 0; i < state->nr_registers; i++) {
-		drm_printf(p, "IO:R %08x %08x\n",
+		drm_printf(p, "  - { offset: 0x%04x, value: 0x%08x }\n",
 			state->registers[i * 2] << 2,
 			state->registers[(i * 2) + 1]);
 	}

Signature · 6 lines

2.17.1

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