On Wed, 2025-01-22 at 21:11 -0800, Lucas De Marchi wrote:
> 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.
>
> Cc: John Harrison <John.C.Harrison@xxxxxxxxx>
> Cc: Julia Filipchuk <julia.filipchuk@xxxxxxxxx>
> Cc: José Roberto de Souza <jose.souza@xxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> 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@xxxxxxxxx>
> ---
> 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 a7946a76777e7..d9b71bb690860 100644
> --- a/drivers/gpu/drm/xe/xe_devcoredump.c
> +++ b/drivers/gpu/drm/xe/xe_devcoredump.c
> @@ -391,42 +391,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
> * @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,
just missing document the suffix.
With that this patch is:
Reviewed-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> 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);
> @@ -458,7 +446,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);
> @@ -470,10 +457,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;
> }
> }
