On 5/28/2024 06:06, Michal Wajdeczko wrote:
This drm printer wrapper can be used to increase the robustness of
the captured output generated by any other drm_printer to make sure
we didn't lost any intermediate lines of the output by adding line
numbers to each output line. Helpful for capturing some crash data.
Signed-off-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
Cc: John Harrison <John.C.Harrison@xxxxxxxxx>
---
v2: don't abuse prefix, use union instead (Jani)
don't use 'dp' as name, prefer 'p' (Jani)
add support for unique series identifier (John)
---
drivers/gpu/drm/drm_print.c | 14 ++++++++
include/drm/drm_print.h | 68 ++++++++++++++++++++++++++++++++++++-
2 files changed, 81 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index cf2efb44722c..be9cbebff5b3 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -214,6 +214,20 @@ void __drm_printfn_err(struct drm_printer *p, struct va_format *vaf)
}
EXPORT_SYMBOL(__drm_printfn_err);
+void __drm_printfn_line(struct drm_printer *p, struct va_format *vaf)
+{
+ unsigned int counter = ++p->line.counter;
Wrong units, but see below anyway...
+ const char *prefix = p->prefix ?: "";
+ const char *pad = p->prefix ? " " : "";
+
+ if (p->line.series)
+ drm_printf(p->arg, "%s%s%u.%u: %pV",
+ prefix, pad, p->line.series, counter, vaf);
+ else
+ drm_printf(p->arg, "%s%s%u: %pV", prefix, pad, counter, vaf);
+}
+EXPORT_SYMBOL(__drm_printfn_line);
+
/**
* drm_puts - print a const string to a &drm_printer stream
* @p: the &drm printer
diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index 089950ad8681..f4d9b98d7909 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -176,7 +176,13 @@ struct drm_printer {
void (*puts)(struct drm_printer *p, const char *str);
void *arg;
const char *prefix;
- enum drm_debug_category category;
+ union {
+ enum drm_debug_category category;
+ struct {
+ unsigned short series;
+ unsigned short counter;
Any particular reason for using 'short' rather than 'int'? Short is only
16bits right? That might seem huge but a GuC log buffer with 16MB debug
log (and minimum sizes for the other sections) when dumped via the
original debugfs hexdump mechanism is 1,245,444 lines. That line count
goes down a lot when you start using longer lines for the dump, but it
is still in the tens of thousands of lines. So limiting to 16 bits is
an overflow just waiting to happen.
+ } line;
+ };
};
void __drm_printfn_coredump(struct drm_printer *p, struct va_format *vaf);
@@ -186,6 +192,7 @@ 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_dbg(struct drm_printer *p, struct va_format *vaf);
void __drm_printfn_err(struct drm_printer *p, struct va_format *vaf);
+void __drm_printfn_line(struct drm_printer *p, struct va_format *vaf);
__printf(2, 3)
void drm_printf(struct drm_printer *p, const char *f, ...);
@@ -357,6 +364,65 @@ static inline struct drm_printer drm_err_printer(struct drm_device *drm,
return p;
}
+/**
+ * drm_line_printer - construct a &drm_printer that prefixes outputs with line numbers
+ * @p: the &struct drm_printer which actually generates the output
+ * @prefix: optional output prefix, or NULL for no prefix
+ * @series: optional unique series identifier, or 0 to omit identifier in the output
+ *
+ * This printer can be used to increase the robustness of the captured output
+ * to make sure we didn't lost any intermediate lines of the output. Helpful
+ * while capturing some crash data.
+ *
+ * Example 1::
+ *
+ * void crash_dump(struct drm_device *drm)
+ * {
+ * static unsigned short id;
+ * struct drm_printer p = drm_err_printer(drm, "crash");
+ * struct drm_printer lp = drm_line_printer(&p, "dump", ++id);
Is there any benefit or other reason to split the prefix across two
separate printers? It seems like a source of confusion. As in, the code
will allow a double prefix, there is not much you can do about that
because losing the prefix from drm_line_printer would mean no prefix at
all when not using drm_err_printer underneath. But why explicitly split
the message across both printers in the usage example? This is saying
that this is the recommended way to use the interface, but with no
explanation of why the split is required or how the split should be done.
Also, there is really no specific connection to crashes. The purpose of
this is for allowing the dumping of multi-line blocks of data. One use
is for debugging crashes. But there are many debug operations that
require the same dump and do not involve a crash. And this support is
certainly not intended to be used on general end-user GPU hangs. For
those, everything should be part of the devcoredump core file that is
produced and accessible via sysfs. We absolutely should not be dumping
huge data blocks in customer release drivers except in very extreme
circumstances.
+ *
+ * drm_printf(&lp, "foo");
+ * drm_printf(&lp, "bar");
+ * }
+ *
+ * Above code will print into the dmesg something like::
+ *
+ * [ ] 0000:00:00.0: [drm] *ERROR* crash dump 1.1: foo
+ * [ ] 0000:00:00.0: [drm] *ERROR* crash dump 1.2: bar
+ *
+ * Example 2::
+ *
+ * void line_dump(struct device *dev)
+ * {
+ * struct drm_printer p = drm_info_printer(dev);
+ * struct drm_printer lp = drm_line_printer(&p, NULL, 0);
+ *
+ * drm_printf(&lp, "foo");
+ * drm_printf(&lp, "bar");
+ * }
+ *
+ * Above code will print::
+ *
+ * [ ] 0000:00:00.0: [drm] 1: foo
+ * [ ] 0000:00:00.0: [drm] 2: bar
Not really seeing the point of having two examples listed. The first
includes all the optional extras, the second is just repeating with no
new information.
John.
+ *
+ * RETURNS:
+ * The &drm_printer object
+ */
+static inline struct drm_printer drm_line_printer(struct drm_printer *p,
+ const char *prefix,
+ unsigned short series)
+{
+ struct drm_printer lp = {
+ .printfn = __drm_printfn_line,
+ .arg = p,
+ .prefix = prefix,
+ .line = { .series = series, },
+ };
+ return lp;
+}
+
/*
* struct device based logging
*
