* Re: [PATCH 1/3] drm/xe: Fix and re-enable xe_print_blob_ascii85()
2025-01-23 17:59 ` [PATCH 1/3] drm/xe: Fix and re-enable xe_print_blob_ascii85() José Roberto de Souza
@ 2025-01-23 18:20 ` John Harrison
2025-01-23 19:37 ` Lucas De Marchi
1 sibling, 0 replies; 3+ messages in thread
From: John Harrison @ 2025-01-23 18:20 UTC (permalink / raw)
To: José Roberto de Souza, intel-xe
Cc: Julia Filipchuk, stable, Lucas De Marchi
On 1/23/2025 09:59, José Roberto de Souza wrote:
> From: Lucas De Marchi <lucas.demarchi@intel.com>
>
> Commit 70fb86a85dc9 ("drm/xe: Revert some changes that break a mesa
> debug tool") partially reverted some changes to workaround breakage
> caused to mesa tools. However, in doing so it also broke fetching the
> GuC log via debugfs since xe_print_blob_ascii85() simply bails out.
>
> The fix is to avoid the extra newlines: the devcoredump interface is
> line-oriented and adding random newlines in the middle breaks it. If a
> tool is able to parse it by looking at the data and checking for chars
> that are out of the ascii85 space, it can still do so. A format change
> that breaks the line-oriented output on devcoredump however needs better
> coordination with existing tools.
>
> Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> Cc: John Harrison <John.C.Harrison@Intel.com>
> Cc: Julia Filipchuk <julia.filipchuk@intel.com>
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Cc: stable@vger.kernel.org
> Fixes: 70fb86a85dc9 ("drm/xe: Revert some changes that break a mesa debug tool")
> Fixes: ec1455ce7e35 ("drm/xe/devcoredump: Add ASCII85 dump helper function")
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
> drivers/gpu/drm/xe/xe_devcoredump.c | 30 +++++++++--------------------
> drivers/gpu/drm/xe/xe_devcoredump.h | 2 +-
> drivers/gpu/drm/xe/xe_guc_ct.c | 3 ++-
> drivers/gpu/drm/xe/xe_guc_log.c | 4 +++-
> 4 files changed, 15 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c b/drivers/gpu/drm/xe/xe_devcoredump.c
> index 81dc7795c0651..1c86e6456d60f 100644
> --- a/drivers/gpu/drm/xe/xe_devcoredump.c
> +++ b/drivers/gpu/drm/xe/xe_devcoredump.c
> @@ -395,42 +395,30 @@ int xe_devcoredump_init(struct xe_device *xe)
> /**
> * xe_print_blob_ascii85 - print a BLOB to some useful location in ASCII85
> *
> - * The output is split to multiple lines because some print targets, e.g. dmesg
> - * cannot handle arbitrarily long lines. Note also that printing to dmesg in
> - * piece-meal fashion is not possible, each separate call to drm_puts() has a
> - * line-feed automatically added! Therefore, the entire output line must be
> - * constructed in a local buffer first, then printed in one atomic output call.
> + * The output is split to multiple print calls because some print targets, e.g.
> + * dmesg cannot handle arbitrarily long lines. These targets may add newline
> + * between calls.
Newlines between calls does not help.
> *
> * There is also a scheduler yield call to prevent the 'task has been stuck for
> * 120s' kernel hang check feature from firing when printing to a slow target
> * such as dmesg over a serial port.
> *
> - * TODO: Add compression prior to the ASCII85 encoding to shrink huge buffers down.
> - *
> * @p: the printer object to output to
> * @prefix: optional prefix to add to output string
> * @blob: the Binary Large OBject to dump out
> * @offset: offset in bytes to skip from the front of the BLOB, must be a multiple of sizeof(u32)
> * @size: the size in bytes of the BLOB, must be a multiple of sizeof(u32)
> */
> -void xe_print_blob_ascii85(struct drm_printer *p, const char *prefix,
> +void xe_print_blob_ascii85(struct drm_printer *p, const char *prefix, char suffix,
> const void *blob, size_t offset, size_t size)
> {
> const u32 *blob32 = (const u32 *)blob;
> char buff[ASCII85_BUFSZ], *line_buff;
> size_t line_pos = 0;
>
> - /*
> - * Splitting blobs across multiple lines is not compatible with the mesa
> - * debug decoder tool. Note that even dropping the explicit '\n' below
> - * doesn't help because the GuC log is so big some underlying implementation
> - * still splits the lines at 512K characters. So just bail completely for
> - * the moment.
> - */
> - return;
> -
> #define DMESG_MAX_LINE_LEN 800
> -#define MIN_SPACE (ASCII85_BUFSZ + 2) /* 85 + "\n\0" */
> + /* Always leave space for the suffix char and the \0 */
> +#define MIN_SPACE (ASCII85_BUFSZ + 2) /* 85 + "<suffix>\0" */
>
> if (size & 3)
> drm_printf(p, "Size not word aligned: %zu", size);
> @@ -462,7 +450,6 @@ void xe_print_blob_ascii85(struct drm_printer *p, const char *prefix,
> line_pos += strlen(line_buff + line_pos);
>
> if ((line_pos + MIN_SPACE) >= DMESG_MAX_LINE_LEN) {
> - line_buff[line_pos++] = '\n';
If you remove this then dmesg output is broken. You can make it optional
in some way so it only happens for dmesg output, but removing it
completely does not work.
John.
> line_buff[line_pos++] = 0;
>
> drm_puts(p, line_buff);
> @@ -474,10 +461,11 @@ void xe_print_blob_ascii85(struct drm_printer *p, const char *prefix,
> }
> }
>
> + if (suffix)
> + line_buff[line_pos++] = suffix;
> +
> if (line_pos) {
> - line_buff[line_pos++] = '\n';
> line_buff[line_pos++] = 0;
> -
> drm_puts(p, line_buff);
> }
>
> diff --git a/drivers/gpu/drm/xe/xe_devcoredump.h b/drivers/gpu/drm/xe/xe_devcoredump.h
> index 6a17e6d601022..5391a80a4d1ba 100644
> --- a/drivers/gpu/drm/xe/xe_devcoredump.h
> +++ b/drivers/gpu/drm/xe/xe_devcoredump.h
> @@ -29,7 +29,7 @@ static inline int xe_devcoredump_init(struct xe_device *xe)
> }
> #endif
>
> -void xe_print_blob_ascii85(struct drm_printer *p, const char *prefix,
> +void xe_print_blob_ascii85(struct drm_printer *p, const char *prefix, char suffix,
> const void *blob, size_t offset, size_t size);
>
> #endif
> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
> index 8b65c5e959cc2..50c8076b51585 100644
> --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
> @@ -1724,7 +1724,8 @@ void xe_guc_ct_snapshot_print(struct xe_guc_ct_snapshot *snapshot,
> snapshot->g2h_outstanding);
>
> if (snapshot->ctb)
> - xe_print_blob_ascii85(p, "CTB data", snapshot->ctb, 0, snapshot->ctb_size);
> + xe_print_blob_ascii85(p, "CTB data", '\n',
> + snapshot->ctb, 0, snapshot->ctb_size);
> } else {
> drm_puts(p, "CT disabled\n");
> }
> diff --git a/drivers/gpu/drm/xe/xe_guc_log.c b/drivers/gpu/drm/xe/xe_guc_log.c
> index 80151ff6a71f8..44482ea919924 100644
> --- a/drivers/gpu/drm/xe/xe_guc_log.c
> +++ b/drivers/gpu/drm/xe/xe_guc_log.c
> @@ -207,8 +207,10 @@ void xe_guc_log_snapshot_print(struct xe_guc_log_snapshot *snapshot, struct drm_
> remain = snapshot->size;
> for (i = 0; i < snapshot->num_chunks; i++) {
> size_t size = min(GUC_LOG_CHUNK_SIZE, remain);
> + const char *prefix = i ? NULL : "Log data";
> + char suffix = i == snapshot->num_chunks - 1 ? '\n' : 0;
>
> - xe_print_blob_ascii85(p, i ? NULL : "Log data", snapshot->copy[i], 0, size);
> + xe_print_blob_ascii85(p, prefix, suffix, snapshot->copy[i], 0, size);
> remain -= size;
> }
> }
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH 1/3] drm/xe: Fix and re-enable xe_print_blob_ascii85()
2025-01-23 17:59 ` [PATCH 1/3] drm/xe: Fix and re-enable xe_print_blob_ascii85() José Roberto de Souza
2025-01-23 18:20 ` John Harrison
@ 2025-01-23 19:37 ` Lucas De Marchi
1 sibling, 0 replies; 3+ messages in thread
From: Lucas De Marchi @ 2025-01-23 19:37 UTC (permalink / raw)
To: José Roberto de Souza
Cc: intel-xe, John Harrison, Julia Filipchuk, stable
On Thu, Jan 23, 2025 at 09:59:41AM -0800, Jose Souza wrote:
>From: Lucas De Marchi <lucas.demarchi@intel.com>
>
>Commit 70fb86a85dc9 ("drm/xe: Revert some changes that break a mesa
>debug tool") partially reverted some changes to workaround breakage
>caused to mesa tools. However, in doing so it also broke fetching the
>GuC log via debugfs since xe_print_blob_ascii85() simply bails out.
>
>The fix is to avoid the extra newlines: the devcoredump interface is
>line-oriented and adding random newlines in the middle breaks it. If a
>tool is able to parse it by looking at the data and checking for chars
>that are out of the ascii85 space, it can still do so. A format change
>that breaks the line-oriented output on devcoredump however needs better
>coordination with existing tools.
>
>Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
>Cc: John Harrison <John.C.Harrison@Intel.com>
>Cc: Julia Filipchuk <julia.filipchuk@intel.com>
>Cc: José Roberto de Souza <jose.souza@intel.com>
>Cc: stable@vger.kernel.org
>Fixes: 70fb86a85dc9 ("drm/xe: Revert some changes that break a mesa debug tool")
>Fixes: ec1455ce7e35 ("drm/xe/devcoredump: Add ASCII85 dump helper function")
>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>---
> drivers/gpu/drm/xe/xe_devcoredump.c | 30 +++++++++--------------------
> drivers/gpu/drm/xe/xe_devcoredump.h | 2 +-
> drivers/gpu/drm/xe/xe_guc_ct.c | 3 ++-
> drivers/gpu/drm/xe/xe_guc_log.c | 4 +++-
> 4 files changed, 15 insertions(+), 24 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c b/drivers/gpu/drm/xe/xe_devcoredump.c
>index 81dc7795c0651..1c86e6456d60f 100644
>--- a/drivers/gpu/drm/xe/xe_devcoredump.c
>+++ b/drivers/gpu/drm/xe/xe_devcoredump.c
>@@ -395,42 +395,30 @@ int xe_devcoredump_init(struct xe_device *xe)
> /**
> * xe_print_blob_ascii85 - print a BLOB to some useful location in ASCII85
> *
>- * The output is split to multiple lines because some print targets, e.g. dmesg
>- * cannot handle arbitrarily long lines. Note also that printing to dmesg in
>- * piece-meal fashion is not possible, each separate call to drm_puts() has a
>- * line-feed automatically added! Therefore, the entire output line must be
>- * constructed in a local buffer first, then printed in one atomic output call.
>+ * The output is split to multiple print calls because some print targets, e.g.
>+ * dmesg cannot handle arbitrarily long lines. These targets may add newline
>+ * between calls.
> *
> * There is also a scheduler yield call to prevent the 'task has been stuck for
> * 120s' kernel hang check feature from firing when printing to a slow target
> * such as dmesg over a serial port.
> *
>- * TODO: Add compression prior to the ASCII85 encoding to shrink huge buffers down.
>- *
> * @p: the printer object to output to
> * @prefix: optional prefix to add to output string
let's just make sure to document suffix
* @suffix: optional suffix to add at the end. 0 disables it and is
* not added to the output, which is useful when using multiple calls
* to dump data to @p
Lucas De Marchi
> * @blob: the Binary Large OBject to dump out
> * @offset: offset in bytes to skip from the front of the BLOB, must be a multiple of sizeof(u32)
> * @size: the size in bytes of the BLOB, must be a multiple of sizeof(u32)
> */
>-void xe_print_blob_ascii85(struct drm_printer *p, const char *prefix,
>+void xe_print_blob_ascii85(struct drm_printer *p, const char *prefix, char suffix,
> const void *blob, size_t offset, size_t size)
> {
> const u32 *blob32 = (const u32 *)blob;
> char buff[ASCII85_BUFSZ], *line_buff;
> size_t line_pos = 0;
>
>- /*
>- * Splitting blobs across multiple lines is not compatible with the mesa
>- * debug decoder tool. Note that even dropping the explicit '\n' below
>- * doesn't help because the GuC log is so big some underlying implementation
>- * still splits the lines at 512K characters. So just bail completely for
>- * the moment.
>- */
>- return;
>-
> #define DMESG_MAX_LINE_LEN 800
>-#define MIN_SPACE (ASCII85_BUFSZ + 2) /* 85 + "\n\0" */
>+ /* Always leave space for the suffix char and the \0 */
>+#define MIN_SPACE (ASCII85_BUFSZ + 2) /* 85 + "<suffix>\0" */
>
> if (size & 3)
> drm_printf(p, "Size not word aligned: %zu", size);
>@@ -462,7 +450,6 @@ void xe_print_blob_ascii85(struct drm_printer *p, const char *prefix,
> line_pos += strlen(line_buff + line_pos);
>
> if ((line_pos + MIN_SPACE) >= DMESG_MAX_LINE_LEN) {
>- line_buff[line_pos++] = '\n';
> line_buff[line_pos++] = 0;
>
> drm_puts(p, line_buff);
>@@ -474,10 +461,11 @@ void xe_print_blob_ascii85(struct drm_printer *p, const char *prefix,
> }
> }
>
>+ if (suffix)
>+ line_buff[line_pos++] = suffix;
>+
> if (line_pos) {
>- line_buff[line_pos++] = '\n';
> line_buff[line_pos++] = 0;
>-
> drm_puts(p, line_buff);
> }
>
>diff --git a/drivers/gpu/drm/xe/xe_devcoredump.h b/drivers/gpu/drm/xe/xe_devcoredump.h
>index 6a17e6d601022..5391a80a4d1ba 100644
>--- a/drivers/gpu/drm/xe/xe_devcoredump.h
>+++ b/drivers/gpu/drm/xe/xe_devcoredump.h
>@@ -29,7 +29,7 @@ static inline int xe_devcoredump_init(struct xe_device *xe)
> }
> #endif
>
>-void xe_print_blob_ascii85(struct drm_printer *p, const char *prefix,
>+void xe_print_blob_ascii85(struct drm_printer *p, const char *prefix, char suffix,
> const void *blob, size_t offset, size_t size);
>
> #endif
>diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
>index 8b65c5e959cc2..50c8076b51585 100644
>--- a/drivers/gpu/drm/xe/xe_guc_ct.c
>+++ b/drivers/gpu/drm/xe/xe_guc_ct.c
>@@ -1724,7 +1724,8 @@ void xe_guc_ct_snapshot_print(struct xe_guc_ct_snapshot *snapshot,
> snapshot->g2h_outstanding);
>
> if (snapshot->ctb)
>- xe_print_blob_ascii85(p, "CTB data", snapshot->ctb, 0, snapshot->ctb_size);
>+ xe_print_blob_ascii85(p, "CTB data", '\n',
>+ snapshot->ctb, 0, snapshot->ctb_size);
> } else {
> drm_puts(p, "CT disabled\n");
> }
>diff --git a/drivers/gpu/drm/xe/xe_guc_log.c b/drivers/gpu/drm/xe/xe_guc_log.c
>index 80151ff6a71f8..44482ea919924 100644
>--- a/drivers/gpu/drm/xe/xe_guc_log.c
>+++ b/drivers/gpu/drm/xe/xe_guc_log.c
>@@ -207,8 +207,10 @@ void xe_guc_log_snapshot_print(struct xe_guc_log_snapshot *snapshot, struct drm_
> remain = snapshot->size;
> for (i = 0; i < snapshot->num_chunks; i++) {
> size_t size = min(GUC_LOG_CHUNK_SIZE, remain);
>+ const char *prefix = i ? NULL : "Log data";
>+ char suffix = i == snapshot->num_chunks - 1 ? '\n' : 0;
>
>- xe_print_blob_ascii85(p, i ? NULL : "Log data", snapshot->copy[i], 0, size);
>+ xe_print_blob_ascii85(p, prefix, suffix, snapshot->copy[i], 0, size);
> remain -= size;
> }
> }
>--
>2.48.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread